On Wed, 12 Mar 2025 at 10:44, Cédric Le Goater <c...@kaod.org> wrote: > > On 3/12/25 11:27, Philippe Mathieu-Daudé wrote: > > + Cédric for Aspeed > > > > On 12/3/25 11:20, Peter Maydell wrote: > >> The bug is that this is directly inheriting from TYPE_DEVICE, > >> not from TYPE_SYSBUS_DEVICE. Doing the former is almost always > >> wrong, because it means the device is never reset. > >> > >> (It looks like we do this wrong for other fsl SoCs too, > >> but they're marked user_creatable = false.) > > > > IIRC it is deliberately that way for the Aspeed SoCs, because > > otherwise there were issue when building the multi-SoC fby35 machines
Multi-SoC shouldn't have a problem with sysbus devices existing. They just mean that you can't use the convenient sysbus_mmio_map() function but have to be more careful with managing MemoryRegions in the different SoCs and using sysbus_mmio_get_region() to get a sysbus device's MMIO regions so you can manually map them to the right places. > AspeedSoCState was introduced long ago (2016) and the fby35 is "recent", > from 2022. I am not sure that's the reason. > > Regarding the reset, the SoC functions are sysbus devices and all have > a reset. The SoC doesn't have any. Maybe that's why we are not seeing > issues. Yeah, it works out in this case. But I think it is in general a bad idea to add new direct-inheritors from TYPE_DEVICE. If anybody needs to add a reset method to the SoC container object in future then they'll be confused about why it doesn't work, for instance. I think that in general people expect that TYPE_DEVICE is "devices are like this" and TYPE_SYSBUS_DEVICE is "if you also need the sysbus-specifics, most notably providing MMIO regions for registers". That's a totally reasonable expectation but unfortunately it's wrong, because a few key parts of "this is an internal device that we don't need users to create" are only in TYPE_SYSBUS_DEVICE. thanks -- PMM
