Re: [PATCH v2 1/2] hw/misc: Implementating dummy AST2600 I3C model

[Cédric Le Goater]

Hello Troy,


> > > +
> > > +    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_i3c_ops, s,
> > > +            TYPE_ASPEED_I3C, ASPEED_I3C_NR_REGS << 2);
> > > +
> > > +    sysbus_init_mmio(sbd, &s->iomem);
> >
> > I would add a container region containing all the regions :
> >
> >
> >       memory_region_init(&s->iomem_container, OBJECT(s),
> >                          TYPE_ASPEED_I3C ".container", 0x8000);
> >
> >       sysbus_init_mmio(sbd, &s->iomem_container);
> >
> >       memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_i3c_ops, s,
> >               TYPE_ASPEED_I3C ".regs", 0x70);
> >
> >       memory_region_add_subregion(&s->iomem_container, 0x0, &s->iomem);
> >
> >
> >
> > The goal is to have a stricter layout so that you can catch errors :
> >
> >       000000001e7a0000-000000001e7a7fff (prio 0, i/o): aspeed.i3c.container
> >         000000001e7a0000-000000001e7a006f (prio 0, i/o): aspeed.i3c.regs
> >         000000001e7a2000-000000001e7a22ff (prio 0, i/o): aspeed.i3c.device.0
> >         000000001e7a3000-000000001e7a32ff (prio 0, i/o): aspeed.i3c.device.1
> >         000000001e7a4000-000000001e7a42ff (prio 0, i/o): aspeed.i3c.device.2
> >         000000001e7a5000-000000001e7a52ff (prio 0, i/o): aspeed.i3c.device.3
> >         000000001e7a6000-000000001e7a62ff (prio 0, i/o): aspeed.i3c.device.4
> >         000000001e7a7000-000000001e7a72ff (prio 0, i/o): aspeed.i3c.device.5
> >
> > and if under U-Boot, you peek into unimplemented regs, you get a warning :
> >
> >       ast# md 1e7a0000
> >       1e7a0000: 00000000 00000000 00000000 00000000    ................
> >       1e7a0010: 00000000 00000000 00000000 00000000    ................
> >       1e7a0020: 00000000 00000000 00000000 00000000    ................
> >       1e7a0030: 00000000 00000000 00000000 00000000    ................
> >       1e7a0040: 00000000 00000000 00000000 00000000    ................
> >       1e7a0050: 00000000 00000000 00000000 00000000    ................
> >       1e7a0060: 00000000 00000000 00000000 00000000    ................
> >       1e7a0070:aspeed_soc.io: unimplemented device read  (size 4, offset 
> > 0x1a0070)
> >        00000000aspeed_soc.io: unimplemented device read  (size 4, offset 
> > 0x1a0074)
>
> Thanks for the code snippet, learnt and applied.
> Yes, it would be easier to catch driver problems.

That said, not all Aspeed models follow this pattern. We learned along
the way and we could improve some older implementations.

btw, I tried your series with ast2600-default-obmc.tar.gz. It boots fine.

Thanks,

C.