Re: [PATCH 04/14] hw/arm/raspi: add a skeleton implementation of the cprman

[Philippe Mathieu-Daudé]

On 10/3/20 1:54 PM, Luc Michel wrote:
> On 16:37 Fri 02 Oct     , Philippe Mathieu-Daudé wrote:
> 
> [snip]
> 
>>>>> +struct BCM2835CprmanState {
>>>>> +    /*< private >*/
>>>>> +    SysBusDevice parent_obj;
>>>>> +
>>>>> +    /*< public >*/
>>>>> +    MemoryRegion iomem;
>>>>> +
>>>>> +    uint32_t regs[CPRMAN_NUM_REGS];
>>>>> +    uint32_t xosc_freq;
>>>>> +
>>>>> +    Clock *xosc;
>>
>> Isn't it xosc external to the CPRMAN?
>>
> Yes on real hardware I'm pretty sure it's the oscillator we can see on
> the board itself, near the SoC (on the bottom side). This is how I first
> planned to implement it. I then realized that would add complexity to
> the BCM2835Peripherals model for no good reasons IMHO (mainly because of
> migration). So at the end I put it inside the CPRMAN for simplicity, and
> added a property to set its frequency.

OK as long as all boards have a 19.2MHz crystal, but if the property
is not easily accessible why not use a #define in
"hw/arm/raspi_platform.h" instead?

Else we should alias the property using object_property_add_alias()
in TYPE_BCM2835_PERIPHERALS.

> 
>>>>> +};
> 
> [snip]
> 
>>>>> +static const MemoryRegionOps cprman_ops = {
>>>>> +    .read = cprman_read,
>>>>> +    .write = cprman_write,
>>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>>> +    .valid      = {
>>>>> +        .min_access_size        = 4,
>>>>> +        .max_access_size        = 4,
>>>>
>>>> I couldn't find this in the public datasheets (any pointer?).
>>>>
>>>> Since your implementation is 32bit, can you explicit .impl
>>>> min/max = 4?
>>>
>>> I could not find this information either, but I assumed this is the
>>> case, mainly because of the 'PASSWORD' field in all registers.
>>
>> Good point. Do you mind adding a comment about it here please?
>>
> 
> OK
> 
>>>
>>> Regarding .impl, I thought that having .valid was enough?
>>
>> Until we eventually figure out we can do 64-bit accesses,
>> so someone change .valid.max to 8 and your model is broken :/
> 
> OK, I'll add the .impl constraints.
> 
> [snip]
>