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 <and...@aj.id.au>
Reviewed-by: Cédric Le Goater <c...@kaod.org>
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;
>
