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

[Cédric Le Goater]

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 ?

C.

>      /* 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/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
> index cff3eb7dd21e..939d1510dedb 100644
> --- a/hw/sd/aspeed_sdhci.c
> +++ b/hw/sd/aspeed_sdhci.c
> @@ -13,6 +13,7 @@
>  #include "qapi/error.h"
>  #include "hw/irq.h"
>  #include "migration/vmstate.h"
> +#include "hw/qdev-properties.h"
>  
>  #define ASPEED_SDHCI_INFO            0x00
>  #define  ASPEED_SDHCI_INFO_RESET     0x00030000
> @@ -120,14 +121,14 @@ static void aspeed_sdhci_realize(DeviceState *dev, 
> Error **errp)
>  
>      /* Create input irqs for the slots */
>      qdev_init_gpio_in_named_with_opaque(DEVICE(sbd), aspeed_sdhci_set_irq,
> -                                        sdhci, NULL, ASPEED_SDHCI_NUM_SLOTS);
> +                                        sdhci, NULL, sdhci->num_slots);
>  
>      sysbus_init_irq(sbd, &sdhci->irq);
>      memory_region_init_io(&sdhci->iomem, OBJECT(sdhci), &aspeed_sdhci_ops,
>                            sdhci, TYPE_ASPEED_SDHCI, 0x1000);
>      sysbus_init_mmio(sbd, &sdhci->iomem);
>  
> -    for (int i = 0; i < ASPEED_SDHCI_NUM_SLOTS; ++i) {
> +    for (int i = 0; i < sdhci->num_slots; ++i) {
>          Object *sdhci_slot = OBJECT(&sdhci->slots[i]);
>          SysBusDevice *sbd_slot = SYS_BUS_DEVICE(&sdhci->slots[i]);
>  
> @@ -174,6 +175,11 @@ static const VMStateDescription vmstate_aspeed_sdhci = {
>      },
>  };
>  
> +static Property aspeed_sdhci_properties[] = {
> +    DEFINE_PROP_UINT8("num-slots", AspeedSDHCIState, num_slots, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void aspeed_sdhci_class_init(ObjectClass *classp, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(classp);
> @@ -181,6 +187,7 @@ static void aspeed_sdhci_class_init(ObjectClass *classp, 
> void *data)
>      dc->realize = aspeed_sdhci_realize;
>      dc->reset = aspeed_sdhci_reset;
>      dc->vmsd = &vmstate_aspeed_sdhci;
> +    dc->props = aspeed_sdhci_properties;
>  }
>  
>  static TypeInfo aspeed_sdhci_info = {
> diff --git a/include/hw/sd/aspeed_sdhci.h b/include/hw/sd/aspeed_sdhci.h
> index dfdab4379021..dffbb46946b9 100644
> --- a/include/hw/sd/aspeed_sdhci.h
> +++ b/include/hw/sd/aspeed_sdhci.h
> @@ -24,6 +24,7 @@ typedef struct AspeedSDHCIState {
>      SysBusDevice parent;
>  
>      SDHCIState slots[ASPEED_SDHCI_NUM_SLOTS];
> +    uint8_t num_slots;
>  
>      MemoryRegion iomem;
>      qemu_irq irq;
>