On 5 February 2014 06:59, Peter Crosthwaite
<peter.crosthwa...@xilinx.com> wrote:
> cc Alistair, this may conflict with his timer work.
>
> On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <peter.mayd...@linaro.org>
> 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. 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. But the nice thing about having named fields is it
doesn't actually matter what order things go in.
thanks
-- PMM
