RE: [PATCH v3 02/21] sd: emmc: Add support for eMMC cards

From: Sai Pavan Boddu <saipava@xilinx.com>
To: "Cédric Le Goater" <clg@kaod.org>,
	"Markus Armbruster" <armbru@redhat.com>,
	"Kevin Wolf" <kwolf@redhat.com>, "Max Reitz" <mreitz@redhat.com>,
	"Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>,
	"Eric Blake" <eblake@redhat.com>, "Joel Stanley" <joel@jms.id.au>,
	"Vincent Palatin" <vpalatin@chromium.org>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	"Thomas Huth" <thuth@redhat.com>,
	"Stefan Hajnoczi" <stefanha@redhat.com>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Alistair Francis" <alistair.francis@wdc.com>,
	"Edgar Iglesias" <edgari@xilinx.com>,
	"Luc Michel" <luc.michel@greensocs.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"qemu-block@nongnu.org" <qemu-block@nongnu.org>
Subject: RE: [PATCH v3 02/21] sd: emmc: Add support for eMMC cards
Date: Wed, 3 Mar 2021 03:57:50 +0000	[thread overview]
Message-ID: <BY5PR02MB677285688FE35B025718687BCA989@BY5PR02MB6772.namprd02.prod.outlook.com> (raw)
In-Reply-To: <8485f4b6-376a-9f2e-aa05-b7fcef84c790@kaod.org>

Hi Cedric,
> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Monday, March 1, 2021 4:32 PM
> To: Sai Pavan Boddu <saipava@xilinx.com>; Markus Armbruster
> <armbru@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Max Reitz
> <mreitz@redhat.com>; Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com>; Eric Blake <eblake@redhat.com>; Joel Stanley
> <joel@jms.id.au>; Vincent Palatin <vpalatin@chromium.org>; Dr. David Alan
> Gilbert <dgilbert@redhat.com>; Thomas Huth <thuth@redhat.com>; Stefan
> Hajnoczi <stefanha@redhat.com>; Peter Maydell <peter.maydell@linaro.org>;
> Alistair Francis <alistair.francis@wdc.com>; Edgar Iglesias <edgari@xilinx.com>;
> Luc Michel <luc.michel@greensocs.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Cc: qemu-block@nongnu.org; qemu-devel@nongnu.org; Sai Pavan Boddu
> <saipava@xilinx.com>
> Subject: Re: [PATCH v3 02/21] sd: emmc: Add support for eMMC cards
> 
> On 2/28/21 8:33 PM, Sai Pavan Boddu wrote:
> > Add eMMC device built on top of SD card.
> >
> > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> > ---
> >  include/hw/sd/sd.h |  2 ++
> >  hw/sd/sd.c         | 20 ++++++++++++++++++++
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h index
> > 05ef9b7..b402dad 100644
> > --- a/include/hw/sd/sd.h
> > +++ b/include/hw/sd/sd.h
> > @@ -90,6 +90,8 @@ typedef struct {
> >  } SDRequest;
> >
> >
> > +#define TYPE_EMMC "emmc"
> > +OBJECT_DECLARE_SIMPLE_TYPE(EMMCState, EMMC)
> >  #define TYPE_SD_CARD "sd-card"
> >  OBJECT_DECLARE_TYPE(SDState, SDCardClass, SD_CARD)
> >
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index 74b9162..a23af6d 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -108,6 +108,7 @@ struct SDState {
> >      uint8_t spec_version;
> >      BlockBackend *blk;
> >      bool spi;
> > +    bool emmc;
> >
> >      /* Runtime changeables */
> >
> > @@ -143,6 +144,10 @@ struct SDState {
> >      bool cmd_line;
> >  };
> >
> > +struct EMMCState {
> > +    SDState parent;
> > +};
> > +
> >  static void sd_realize(DeviceState *dev, Error **errp);
> >
> >  static const char *sd_state_name(enum SDCardStates state) @@ -2105,6
> > +2110,13 @@ static void sd_instance_init(Object *obj)
> >      sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > sd_ocr_powerup, sd);  }
> >
> > +static void emmc_instance_init(Object *obj) {
> > +    SDState *sd = SD_CARD(obj);
> > +
> > +    sd->emmc = true;
> > +}
> I think field 'emmc' would fit better in SDCardClass since it is a device constant.
> You should not need 'struct EMMCState'. So something like below.
> Then you can add handlers for specific emmc commands.
> 
> Thanks,
> 
> C.
> 
> 
> diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h index
> 47360ba4ee98..80e7cd526a57 100644
> --- a/include/hw/sd/sd.h
> +++ b/include/hw/sd/sd.h
> @@ -93,6 +93,8 @@ typedef struct {
>  #define TYPE_SD_CARD "sd-card"
>  OBJECT_DECLARE_TYPE(SDState, SDCardClass, SD_CARD)
> 
> +#define TYPE_EMMC "emmc"
> +
>  struct SDCardClass {
>      /*< private >*/
>      DeviceClass parent_class;
> @@ -124,6 +126,8 @@ struct SDCardClass {
>      void (*enable)(SDState *sd, bool enable);
>      bool (*get_inserted)(SDState *sd);
>      bool (*get_readonly)(SDState *sd);
> +
> +    bool emmc;
>  };
> 
>  #define TYPE_SD_BUS "sd-bus"
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 660026f2a667..95608f11b36e 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2447,9 +2447,24 @@ static const TypeInfo sd_info = {
>      .instance_finalize = sd_instance_finalize,  };
> 
> +static void emmc_class_init(ObjectClass *klass, void *data) {
> +    SDCardClass *sc = SD_CARD_CLASS(klass);
> +
> +    sc->emmc = true;
> +}
> +
> +static const TypeInfo emmc_info = {
> +    .name = TYPE_EMMC,
> +    .parent = TYPE_SD_CARD,
> +    .instance_size = sizeof(SDState),
> +    .class_init = emmc_class_init,
> +};
> +
[Sai Pavan Boddu] Yes, I see your point. Let me try, I was preferring a simpler approach just to not disturb the code much but lets see how this workout.

Thanks,
Sai Pavan
>  static void sd_register_types(void)
>  {
>      type_register_static(&sd_info);
> +    type_register_static(&emmc_info);
>  }
> 
>  type_init(sd_register_types)