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

[Cédric Le Goater]

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 ?

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;
    }

Thanks,

C.

> > +    .class_init = emmc_class_init,
> > + };