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
