Re: [Qemu-devel] [PATCH v3 1/3] arm_generic_timer: Add the ARM Generic Timer

[Alistair Francis]

On Tue, Jan 10, 2017 at 1:42 AM, Peter Maydell <address@hidden> wrote:
> On 10 January 2017 at 01:41, Alistair Francis
> <address@hidden> wrote:
>> On Fri, Jan 6, 2017 at 3:57 AM, Peter Maydell <address@hidden> wrote:
>>> On 20 December 2016 at 22:42, Alistair Francis
>>> <address@hidden> wrote:
>
>>>> +    },{ .name = "CNTCV_LOWER",
>>>> +        .addr = A_CNTCV_LOWER,
>>>> +        .post_read = counter_low_value_postr,
>>>> +    },{ .name = "CNTCV_UPPER",
>>>> +        .addr = A_CNTCV_UPPER,
>>>> +        .post_read = counter_high_value_postr,
>>>
>>> Spec says that you should also be able to access the whole
>>> 64-bit counter value with a 64-bit access -- is this reginfo
>>> sufficient to make that work?
>>
>> How do you know, all I see if that it is a 64-bit register. All the
>> documentation about accesses specifies only 32-bit accesses.
>
> v8 ARM ARM DDI0487A.k, I3.5.3 (CNTCV register description):
> "In an implementation that supports 64-bit atomic memory accesses,
> this register must be accessible using a 64-bit atomic access."

Ok, I don't even have that section. I'm going to try to find an updated ARM ARM.

>
>
>>> This means you can't use this device in a system which
>>> doesn't implement TrustZone. I think this would be better
>>> handled by just having the board map the memory region
>>> in to only the Secure address space if it is a TZ-aware board.
>>
>> How can I map something into the secure address space? Is there an
>> example of this? The only think I can find is in the arm/virt.c
>> machine with the secure_sysmem MemoryRegion.
>
> Yeah, virt.c is the place to look -- for instance we put one
> of the UARTs in the secure address space only.
> Basically to support separate address spaces:
>  * create a MemoryRegion to be the secure view of the world
>  * add the NS MemoryRegion to it as a subregion with low
>    priority (so S sees all the NS devices -- this isn't
>    required but that's usually how h/w is set up)
>  * use object_property_set_link() to connect the S memory
>    region to each CPU's secure-memory property
>  * add any S-only devices to the S MemoryRegion

Ok, sounds easy enough.

>
>>>> +REG32(CNTCR, 0x00)
>>>> +    FIELD(CNTCR, EN, 0, 1)
>>>> +    FIELD(CNTCR, HDBG, 1, 1)
>>>> +REG32(CNTSR, 0x04)
>>>> +    FIELD(CNTSR, DBGH, 1, 1)
>>>> +REG32(CNTCV_LOWER, 0x08)
>>>> +REG32(CNTCV_UPPER, 0x0C)
>>>> +REG32(CNTFID0, 0x20)
>>>> +/* We don't model CNTFIDn */
>>>> +/* We don't model the CounterID registers either */
>>>
>>> Does the Xilinx h/w not implement them at all?
>>> (ie do we need a property for "device has the ID regs" if we add them
>>> later?)
>>
>> There is nothing in the register spec describing registers being here.
>>
>> The last register I see is called: (IOU_SCNTRS)
>> base_frequency_ID_register at address 0xFF260020.
>
> I suspect there's an implicit reads-as-zero after that.
> (The frequency table has to have at least one real entry
> plus the zero terminator -- see I3.5.5 (CNTFID0).)
>
> QEMU's implementation will only support one frequency so
> we only need one table entry too (for the moment, anyway).
>
>>> The spec says it's mandatory to have at least the entry for
>>> the counter base frequency plus end-of-table marker.
>>> I would expect EL3 firmware to want to read this table in order
>>> to write the correct values to CNTFRQ_EL0 for each CPU.
>>
>> I don't see anything like that in section I3.5.21 in the ARM ARM,
>> where are you looking?
>
> There's no I3.5.21 in my copy of the spec (rev A.k), I suspect
> you have an older rev. I think the generic timer docs got
> revamped at some point, so you might want to grab the latest rev.
>
> Anyway, it looks like you have got CNTFIDn (that's CNTFID0)
> and an implicit undocumented zero-terminator.
>
> The spec says CounterIDn are IMPDEF, which I guess is why
> your implementation doesn't bother to implement them.

I do have an out of date version. I'll find a newer one and update the
registers to follow that.

Thanks,

Alistair

>
> thanks
> -- PMM
>