On Wed, Feb 5, 2014 at 9:01 PM, Peter Maydell <peter.mayd...@linaro.org> wrote: > On 5 February 2014 06:59, Peter Crosthwaite > <peter.crosthwa...@xilinx.com> wrote: >> cc Alistair, this may conflict with his timer work.
It seems fine to me. None of Peter's code changes conflict with mine, except for adding the ".accessfn = pmreg_access," to the PMCCNTR register. My patch never implemented the return EXCP_UDEF; so no action is needed there >> >> 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 >