Re: [RFC PATCH 11/17] hw/sd: Add eMMC support

["Cédric Le Goater" <clg@kaod.org>]

On 5/31/22 10:03, Philippe Mathieu-Daudé wrote:
> On 31/5/22 07:58, Cédric Le Goater wrote:
>> On 5/30/22 19:40, Philippe Mathieu-Daudé wrote:
>>> On 18/3/22 14:28, Cédric Le Goater wrote:
>>>> The initial eMMC support from Vincent Palatin was largely reworked to
>>>> match the current SD framework. The parameters mimick a real 4GB eMMC,
>>>> but it can be set to various sizes.
>>>>
>>>> This adds a new QOM object class for EMMC devices.
>>>>
>>>> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
>>>> Link: https://lore.kernel.org/r/1311635951-11047-5-git-send-email-vpalatin@chromium.org
>>>> [ jms: - Forward ported to QEMU 5.2 ]
>>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>>>> [ clg: - ported on aspeed-7.0 patchset
>>>>         - HPI activation ]
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>   hw/sd/sdmmc-internal.h |  97 +++++++++++++++++++
>>>>   include/hw/sd/sd.h     |   9 ++
>>>>   hw/sd/sd.c             | 205 ++++++++++++++++++++++++++++++++++++++++-
>>>>   hw/sd/sdmmc-internal.c |   2 +-
>>>>   4 files changed, 311 insertions(+), 2 deletions(-)
>>>
>>>
>>>> +static void emmc_class_init(ObjectClass *klass, void *data)
>>>> +{
>>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>>> +    SDCardClass *sc = SD_CARD_CLASS(klass);
>>>> +
>>>> +    dc->desc = "eMMC";
>>>> +    sc->proto = &sd_proto_emmc;
>>>> +    sc->spec_version = SD_PHY_SPECv3_01_VERS; /* eMMC requirement */
>>>> +    sc->set_csd = sd_emmc_set_csd;
>>>> +}
>>>> +
>>>> +static const TypeInfo emmc_info = {
>>>> +    .name = TYPE_EMMC,
>>>> +    .parent = TYPE_SD_CARD,
>>>
>>> Hmm this is odd to have the model inheriting features from SD_CARD but then behaving differently (one could enumerate QDEV objects implementing
>>> TYPE_SD_CARD then use them expecting they match the SD card protocol).
>>>
>>> Why do you need to have TYPE_SD_CARD as parent?
>>
>> Simply for the initialization.
>>> Could we simply duplicate sd_class_init() assignations instead? That
>>> would likely make it easier to modify eMMC handlers.
>>
>> May be we lack a base abstract class ?
> 
> I've been thinking about it but maybe not enough. I'll revisit.
> 
>> It would clean up this section in the realize routine :
>>
>>     sd->proto = sd->spi ? &sd_proto_spi : &sd_proto_sd;
>>
>>      if (sc->proto) {
>>          sd->proto = sc->proto;
>>      }
> 
> In v2 I moved the 'proto' field from instance to class, so we don't need
> this hack anymore.

Indeed :

    static void sd_realize(DeviceState *dev, Error **errp)
    {
        SDState *sd = SD_CARD(dev);
        SDCardClass *sc = SD_CARD_GET_CLASS(sd);
        int ret;
    
        sc->proto = sd->spi ? &sd_proto_spi : &sd_proto_sd;
        ...

but this is assigning a class attribute from an instance :/

C.