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

[Luc Michel]

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.

> >>> +};

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

-- 
Luc