Am 12. März 2025 11:13:01 UTC schrieb Peter Maydell <peter.mayd...@linaro.org>:
>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.
I'll send a patch fixing inheritance from TYPE_SYSBUS_DEVICE and setting
user_creatable = false.
Best regards,
Bernhard
>
>thanks
>-- PMM
