Re: [PATCH 1/2] hw/sd: Configure number of slots exposed by the ASPEED SDHCI model

[Andrew Jeffery]

On Tue, 10 Dec 2019, at 19:26, Cédric Le Goater wrote:
> On 10/12/2019 01:52, Andrew Jeffery wrote:
> > The AST2600 includes a second cut-down version of the SD/MMC controller
> > found in the AST2500, named the eMMC controller. It's cut down in the
> > sense that it only supports one slot rather than two, but it brings the
> > total number of slots supported by the AST2600 to three.
> > 
> > The existing code assumed that the SD controller always provided two
> > slots. Rework the SDHCI object to expose the number of slots as a
> > property to be set by the SoC configuration.
> > 
> > Signed-off-by: Andrew Jeffery <address@hidden>
> 
> Reviewed-by: Cédric Le Goater <address@hidden>
> 
> One minor question below.
> 
> 
> > ---
> >  hw/arm/aspeed.c              |  2 +-
> >  hw/arm/aspeed_ast2600.c      |  2 ++
> >  hw/arm/aspeed_soc.c          |  3 +++
> >  hw/sd/aspeed_sdhci.c         | 11 +++++++++--
> >  include/hw/sd/aspeed_sdhci.h |  1 +
> >  5 files changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > index 028191ff36fc..862549b1f3a9 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -259,7 +259,7 @@ static void aspeed_board_init(MachineState *machine,
> >          cfg->i2c_init(bmc);
> >      }
> >  
> > -    for (i = 0; i < ARRAY_SIZE(bmc->soc.sdhci.slots); i++) {
> > +    for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
> >          SDHCIState *sdhci = &bmc->soc.sdhci.slots[i];
> >          DriveInfo *dinfo = drive_get_next(IF_SD);
> >          BlockBackend *blk;
> > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> > index 931887ac681f..931ee5aae183 100644
> > --- a/hw/arm/aspeed_ast2600.c
> > +++ b/hw/arm/aspeed_ast2600.c
> > @@ -208,6 +208,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
> >      sysbus_init_child_obj(obj, "sdc", OBJECT(&s->sdhci), sizeof(s->sdhci),
> >                            TYPE_ASPEED_SDHCI);
> >  
> > +    object_property_set_int(OBJECT(&s->sdhci), 2, "num-slots", 
> > &error_abort);
> 
> OK. This defines 2 SDHCI slots for the ast2600 SoC, but
> 
> > +
> >      /* Init sd card slot class here so that they're under the correct 
> > parent */
> >      for (i = 0; i < ASPEED_SDHCI_NUM_SLOTS; ++i) {
> >          sysbus_init_child_obj(obj, "sdhci[*]", OBJECT(&s->sdhci.slots[i]),
> > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> > index f4fe243458fd..3498f55603f2 100644
> > --- a/hw/arm/aspeed_soc.c
> > +++ b/hw/arm/aspeed_soc.c
> > @@ -215,6 +215,9 @@ static void aspeed_soc_init(Object *obj)
> >      sysbus_init_child_obj(obj, "sdc", OBJECT(&s->sdhci), sizeof(s->sdhci),
> >                            TYPE_ASPEED_SDHCI);
> >  
> > +    object_property_set_int(OBJECT(&s->sdhci), ASPEED_SDHCI_NUM_SLOTS,
> > +                            "num-slots", &error_abort);
> 
> 
> why use ASPEED_SDHCI_NUM_SLOTS here ?

No good reason. I'll just switch it to '2' like in the 2600.

Andrew