[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 12/35] target-arm: Convert performance monito
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [PATCH v2 12/35] target-arm: Convert performance monitor reginfo to accesfn |
Date: |
Sun, 9 Feb 2014 12:59:02 +1000 |
On Wed, Feb 5, 2014 at 9:01 PM, Peter Maydell <address@hidden> wrote:
> On 5 February 2014 06:59, Peter Crosthwaite
> <address@hidden> wrote:
>> cc Alistair, this may conflict with his timer work.
>>
>> On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <address@hidden> wrote:
>>> @@ -624,37 +606,41 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>>> { .name = "PMCNTENSET", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0,
>>> .opc2 = 1,
>>> .access = PL0_RW, .resetvalue = 0,
>>> .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
>>> - .readfn = pmreg_read, .writefn = pmcntenset_write,
>>> - .raw_readfn = raw_read, .raw_writefn = raw_write },
>>> + .writefn = pmcntenset_write,
>>> + .accessfn = pmreg_access,
>>> + .raw_writefn = raw_write },
>>
>> A nit but,
>>
>> You're field ordering scheme is inconsistent, here you go, write ->
>> access -> raw_write ....
>>
>>> { .name = "PMCNTENCLR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0,
>>> .opc2 = 2,
>>> .access = PL0_RW, .fieldoffset = offsetof(CPUARMState,
>>> cp15.c9_pmcnten),
>>> - .readfn = pmreg_read, .writefn = pmcntenclr_write,
>>> + .accessfn = pmreg_access,
>>> + .writefn = pmcntenclr_write,
>>> .type = ARM_CP_NO_MIGRATE },
>>> { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 =
>>> 3,
>>> .access = PL0_RW, .fieldoffset = offsetof(CPUARMState,
>>> cp15.c9_pmovsr),
>>> - .readfn = pmreg_read, .writefn = pmovsr_write,
>>> - .raw_readfn = raw_read, .raw_writefn = raw_write },
>>> - /* Unimplemented so WI. Strictly speaking write accesses in PL0 should
>>> - * respect PMUSERENR.
>>> - */
>>> + .accessfn = pmreg_access,
>>> + .writefn = pmovsr_write,
>>> + .raw_writefn = raw_write },
>>
>> ... and this is access -> write -> raw_write. Is there a prescribed
>> order to keep new (or gradually refactored) code consistent?
>
> No, I've just been going for "whatever looks like it fits reasonably
> neatly onto not too many lines and is a fairly minimal change to existing
> structs", mostly.
I agree that we should minimise diff. Existing code trumps any new
rules we come up with here. But I picked on this one because its
inconsistent just within the new changes.
The thing I have been trying to be consistent with is
> the order for crn/crm/opc1/opc2 fields, which for new registers I've been
> making be op0/op1/crn/crm/op2, because that's the order the ARM ARM
> seems to have settled on. Unfortunately a lot of our existing definitions
> are crn/crm/op1/op2, because at the time there was variance in the
> ARM docs and that order seemed sensible to me.
>
> For the other fields, I think "name first; type/state/encoding second;
> access related fields; read and write accessors; reset stuff" is probably
> not a bad order.
Agreed. With diff-correction based on this scheme:
Reviewed-by: Peter Crosthwaite <address@hidden>
But the nice thing about having named fields is it
> doesn't actually matter what order things go in.
>
But for similar entries its much more readable if they are self-consistent.
Regards,
Peter
> thanks
> -- PMM
>