Re: [Qemu-devel] [PATCH target-arm v5 1/1] target-arm: Implements the ARM PMCCNTR register

[Peter Crosthwaite]

On Thu, Feb 13, 2014 at 5:32 AM, Peter Maydell <address@hidden> wrote:
> On 31 January 2014 04:44, Alistair Francis <address@hidden> wrote:
>> This patch implements the ARM PMCCNTR register including
>> the disable and reset components of the PMCR register.
>>
>> Signed-off-by: Alistair Francis <address@hidden>
>
> I'm afraid you'll need to respin this against my target-arm.next
> branch, because I've landed the patches which update the
> API for read/write functions.
>   git://git.linaro.org/people/peter.maydell/qemu-arm.git target-arm.next
>
>> +
>>  static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>>                         uint64_t *value)
>>  {
>> @@ -644,9 +734,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>>       */
>>      { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
>>        .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> -    /* Unimplemented, RAZ/WI. XXX PMUSERENR */
>
> Your patch has dropped this XXX comment without fixing the
> issue. Luckily you'll find that in target-arm.next it's been fixed,
> by providing a suitable accessfn for the register.
>
>>      { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 
>> 0,
>> -      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +      .access = PL1_RW, .resetvalue = 0, .type = ARM_CP_IO,
>
> This changes the access rights, incorrectly.
>
>> +      .readfn = pmccntr_read, .writefn = pmccntr_write,
>> +      .raw_readfn = raw_read, .raw_writefn = raw_write },
>
> This is wrong, because raw_read/write need to return the same
> value as the actual register, just without odd side effects.
>

My understanding is that the _read and _write functions as you have
implemented are this exact semantic. So you should be able to just
drop the raw_'s.

Regards,
Peter

>>      { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 
>> = 1,
>>        .access = PL0_RW,
>>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
>
> Thanks, and sorry for the inconvenience resulting from
> the clashing patchset.
>
> -- PMM
>