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]
>
