[PATCH v2 0/6] Support SDHCI and eMMC for ast2700

[PATCH v2 0/6] Support SDHCI and eMMC for ast2700

[Jamin Lin via]

change from v1:
This patch series do not support boot from an eMMC.
Only support eMMC and SD Slot 0 as storages.

change from v2:
- Add hw/sd/aspeed_sdhci: Fix coding style patch

Jamin Lin (6):
  hw/sd/aspeed_sdhci: Fix coding style
  hw/arm/aspeed: Fix coding style
  hw:sdhci: Introduce a new "capareg" class member to set the different
    Capability Registers.
  hw/sd/aspeed_sdhci: Add AST2700 Support
  aspeed/soc: Support SDHCI for AST2700
  aspeed/soc: Support eMMC for AST2700

 hw/arm/aspeed_ast2400.c      |  3 +-
 hw/arm/aspeed_ast2600.c      | 10 ++--
 hw/arm/aspeed_ast27x0.c      | 35 ++++++++++++++
 hw/sd/aspeed_sdhci.c         | 94 +++++++++++++++++++++++++++++++-----
 include/hw/sd/aspeed_sdhci.h | 13 ++++-
 5 files changed, 136 insertions(+), 19 deletions(-)

-- 
2.34.1

[PATCH v2 1/6] hw/sd/aspeed_sdhci: Fix coding style

[Jamin Lin via]

Fix coding style issues from checkpatch.pl.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/sd/aspeed_sdhci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
index 98d5460905..acd6538261 100644
--- a/hw/sd/aspeed_sdhci.c
+++ b/hw/sd/aspeed_sdhci.c
@@ -87,10 +87,12 @@ static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
         sdhci->regs[TO_REG(addr)] = (uint32_t)val & ~ASPEED_SDHCI_INFO_RESET;
         break;
     case ASPEED_SDHCI_SDIO_140:
-        sdhci->slots[0].capareg = deposit64(sdhci->slots[0].capareg, 0, 32, val);
+        sdhci->slots[0].capareg = deposit64(sdhci->slots[0].capareg,
+                                            0, 32, val);
         break;
     case ASPEED_SDHCI_SDIO_144:
-        sdhci->slots[0].capareg = deposit64(sdhci->slots[0].capareg, 32, 32, val);
+        sdhci->slots[0].capareg = deposit64(sdhci->slots[0].capareg,
+                                            32, 32, val);
         break;
     case ASPEED_SDHCI_SDIO_148:
         sdhci->slots[0].maxcurr = deposit64(sdhci->slots[0].maxcurr,
-- 
2.34.1

[PATCH v2 2/6] hw/arm/aspeed: Fix coding style

[Jamin Lin via]

Fix coding style issues from checkpatch.pl.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed_ast2600.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index be3eb70cdd..c40d3d8443 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -541,7 +541,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->gpio), errp)) {
         return;
     }
-    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->gpio), 0, sc->memmap[ASPEED_DEV_GPIO]);
+    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->gpio), 0,
+                    sc->memmap[ASPEED_DEV_GPIO]);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_GPIO));
 
-- 
2.34.1

[PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class member to set the different Capability Registers.

[Jamin Lin via]

Currently, it set the hardcode value of capability registers to all ASPEED SOCs
However, the value of capability registers should be different for all ASPEED
SOCs. For example: the bit 28 of the Capability Register 1 should be 1 for
64-bits System Bus support for AST2700.

Introduce a new "capareg" class member whose data type is uint_64 to set the
different Capability Registers to all ASPEED SOCs.

The value of Capability Register is "0x0000000001e80080" for AST2400 and
AST2500. The value of Capability Register is "0x0000000701f80080" for AST2600.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed_ast2400.c      |  3 +-
 hw/arm/aspeed_ast2600.c      |  7 ++--
 hw/sd/aspeed_sdhci.c         | 72 +++++++++++++++++++++++++++++++-----
 include/hw/sd/aspeed_sdhci.h | 12 +++++-
 4 files changed, 78 insertions(+), 16 deletions(-)

diff --git a/hw/arm/aspeed_ast2400.c b/hw/arm/aspeed_ast2400.c
index ecc81ecc79..3c1b419945 100644
--- a/hw/arm/aspeed_ast2400.c
+++ b/hw/arm/aspeed_ast2400.c
@@ -224,7 +224,8 @@ static void aspeed_ast2400_soc_init(Object *obj)
     snprintf(typename, sizeof(typename), "aspeed.gpio-%s", socname);
     object_initialize_child(obj, "gpio", &s->gpio, typename);
 
-    object_initialize_child(obj, "sdc", &s->sdhci, TYPE_ASPEED_SDHCI);
+    snprintf(typename, sizeof(typename), "aspeed.sdhci-%s", socname);
+    object_initialize_child(obj, "sdc", &s->sdhci, typename);
 
     object_property_set_int(OBJECT(&s->sdhci), "num-slots", 2, &error_abort);
 
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index c40d3d8443..b5703bd064 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -236,8 +236,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
     snprintf(typename, sizeof(typename), "aspeed.gpio-%s-1_8v", socname);
     object_initialize_child(obj, "gpio_1_8v", &s->gpio_1_8v, typename);
 
-    object_initialize_child(obj, "sd-controller", &s->sdhci,
-                            TYPE_ASPEED_SDHCI);
+    snprintf(typename, sizeof(typename), "aspeed.sdhci-%s", socname);
+    object_initialize_child(obj, "sd-controller", &s->sdhci, typename);
 
     object_property_set_int(OBJECT(&s->sdhci), "num-slots", 2, &error_abort);
 
@@ -247,8 +247,7 @@ static void aspeed_soc_ast2600_init(Object *obj)
                                 &s->sdhci.slots[i], TYPE_SYSBUS_SDHCI);
     }
 
-    object_initialize_child(obj, "emmc-controller", &s->emmc,
-                            TYPE_ASPEED_SDHCI);
+    object_initialize_child(obj, "emmc-controller", &s->emmc, typename);
 
     object_property_set_int(OBJECT(&s->emmc), "num-slots", 1, &error_abort);
 
diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
index acd6538261..93f5571021 100644
--- a/hw/sd/aspeed_sdhci.c
+++ b/hw/sd/aspeed_sdhci.c
@@ -148,6 +148,7 @@ static void aspeed_sdhci_realize(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
+    AspeedSDHCIClass *asc = ASPEED_SDHCI_GET_CLASS(sdhci);
 
     /* Create input irqs for the slots */
     qdev_init_gpio_in_named_with_opaque(DEVICE(sbd), aspeed_sdhci_set_irq,
@@ -167,7 +168,7 @@ static void aspeed_sdhci_realize(DeviceState *dev, Error **errp)
         }
 
         if (!object_property_set_uint(sdhci_slot, "capareg",
-                                      ASPEED_SDHCI_CAPABILITIES, errp)) {
+                                      asc->capareg, errp)) {
             return;
         }
 
@@ -218,13 +219,66 @@ static void aspeed_sdhci_class_init(ObjectClass *classp, void *data)
     device_class_set_props(dc, aspeed_sdhci_properties);
 }
 
-static const TypeInfo aspeed_sdhci_types[] = {
-    {
-        .name           = TYPE_ASPEED_SDHCI,
-        .parent         = TYPE_SYS_BUS_DEVICE,
-        .instance_size  = sizeof(AspeedSDHCIState),
-        .class_init     = aspeed_sdhci_class_init,
-    },
+static const TypeInfo aspeed_sdhci_info = {
+    .name           = TYPE_ASPEED_SDHCI,
+    .parent         = TYPE_SYS_BUS_DEVICE,
+    .instance_size  = sizeof(AspeedSDHCIState),
+    .class_init     = aspeed_sdhci_class_init,
+    .class_size = sizeof(AspeedSDHCIClass),
+    .abstract = true,
+};
+
+static void aspeed_2400_sdhci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSDHCIClass *asc = ASPEED_SDHCI_CLASS(klass);
+
+    dc->desc = "ASPEED 2400 SDHCI Controller";
+    asc->capareg = 0x0000000001e80080;
+}
+
+static const TypeInfo aspeed_2400_sdhci_info = {
+    .name = TYPE_ASPEED_2400_SDHCI,
+    .parent = TYPE_ASPEED_SDHCI,
+    .class_init = aspeed_2400_sdhci_class_init,
+};
+
+static void aspeed_2500_sdhci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSDHCIClass *asc = ASPEED_SDHCI_CLASS(klass);
+
+    dc->desc = "ASPEED 2500 SDHCI Controller";
+    asc->capareg = 0x0000000001e80080;
+}
+
+static const TypeInfo aspeed_2500_sdhci_info = {
+    .name = TYPE_ASPEED_2500_SDHCI,
+    .parent = TYPE_ASPEED_SDHCI,
+    .class_init = aspeed_2500_sdhci_class_init,
 };
 
-DEFINE_TYPES(aspeed_sdhci_types)
+static void aspeed_2600_sdhci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSDHCIClass *asc = ASPEED_SDHCI_CLASS(klass);
+
+    dc->desc = "ASPEED 2600 SDHCI Controller";
+    asc->capareg = 0x0000000701f80080;
+}
+
+static const TypeInfo aspeed_2600_sdhci_info = {
+    .name = TYPE_ASPEED_2600_SDHCI,
+    .parent = TYPE_ASPEED_SDHCI,
+    .class_init = aspeed_2600_sdhci_class_init,
+};
+
+static void aspeed_sdhci_register_types(void)
+{
+    type_register_static(&aspeed_sdhci_info);
+    type_register_static(&aspeed_2400_sdhci_info);
+    type_register_static(&aspeed_2500_sdhci_info);
+    type_register_static(&aspeed_2600_sdhci_info);
+}
+
+type_init(aspeed_sdhci_register_types);
diff --git a/include/hw/sd/aspeed_sdhci.h b/include/hw/sd/aspeed_sdhci.h
index 057bc5f3d1..8083797e25 100644
--- a/include/hw/sd/aspeed_sdhci.h
+++ b/include/hw/sd/aspeed_sdhci.h
@@ -13,9 +13,11 @@
 #include "qom/object.h"
 
 #define TYPE_ASPEED_SDHCI "aspeed.sdhci"
-OBJECT_DECLARE_SIMPLE_TYPE(AspeedSDHCIState, ASPEED_SDHCI)
+#define TYPE_ASPEED_2400_SDHCI TYPE_ASPEED_SDHCI "-ast2400"
+#define TYPE_ASPEED_2500_SDHCI TYPE_ASPEED_SDHCI "-ast2500"
+#define TYPE_ASPEED_2600_SDHCI TYPE_ASPEED_SDHCI "-ast2600"
+OBJECT_DECLARE_TYPE(AspeedSDHCIState, AspeedSDHCIClass, ASPEED_SDHCI)
 
-#define ASPEED_SDHCI_CAPABILITIES 0x01E80080
 #define ASPEED_SDHCI_NUM_SLOTS    2
 #define ASPEED_SDHCI_NUM_REGS     (ASPEED_SDHCI_REG_SIZE / sizeof(uint32_t))
 #define ASPEED_SDHCI_REG_SIZE     0x100
@@ -32,4 +34,10 @@ struct AspeedSDHCIState {
     uint32_t regs[ASPEED_SDHCI_NUM_REGS];
 };
 
+struct AspeedSDHCIClass {
+    SysBusDeviceClass parent_class;
+
+    uint64_t capareg;
+};
+
 #endif /* ASPEED_SDHCI_H */
-- 
2.34.1

[PATCH v2 4/6] hw/sd/aspeed_sdhci: Add AST2700 Support

[Jamin Lin via]

Introduce a new ast2700 class to support AST2700. Add a new ast2700 SDHCI class
init function and set the value of capability register to "0x0000000719f80080".

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/sd/aspeed_sdhci.c         | 16 ++++++++++++++++
 include/hw/sd/aspeed_sdhci.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
index 93f5571021..4cba934f53 100644
--- a/hw/sd/aspeed_sdhci.c
+++ b/hw/sd/aspeed_sdhci.c
@@ -273,12 +273,28 @@ static const TypeInfo aspeed_2600_sdhci_info = {
     .class_init = aspeed_2600_sdhci_class_init,
 };
 
+static void aspeed_2700_sdhci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSDHCIClass *asc = ASPEED_SDHCI_CLASS(klass);
+
+    dc->desc = "ASPEED 2700 SDHCI Controller";
+    asc->capareg = 0x0000000719f80080;
+}
+
+static const TypeInfo aspeed_2700_sdhci_info = {
+    .name = TYPE_ASPEED_2700_SDHCI,
+    .parent = TYPE_ASPEED_SDHCI,
+    .class_init = aspeed_2700_sdhci_class_init,
+};
+
 static void aspeed_sdhci_register_types(void)
 {
     type_register_static(&aspeed_sdhci_info);
     type_register_static(&aspeed_2400_sdhci_info);
     type_register_static(&aspeed_2500_sdhci_info);
     type_register_static(&aspeed_2600_sdhci_info);
+    type_register_static(&aspeed_2700_sdhci_info);
 }
 
 type_init(aspeed_sdhci_register_types);
diff --git a/include/hw/sd/aspeed_sdhci.h b/include/hw/sd/aspeed_sdhci.h
index 8083797e25..4ef1770471 100644
--- a/include/hw/sd/aspeed_sdhci.h
+++ b/include/hw/sd/aspeed_sdhci.h
@@ -16,6 +16,7 @@
 #define TYPE_ASPEED_2400_SDHCI TYPE_ASPEED_SDHCI "-ast2400"
 #define TYPE_ASPEED_2500_SDHCI TYPE_ASPEED_SDHCI "-ast2500"
 #define TYPE_ASPEED_2600_SDHCI TYPE_ASPEED_SDHCI "-ast2600"
+#define TYPE_ASPEED_2700_SDHCI TYPE_ASPEED_SDHCI "-ast2700"
 OBJECT_DECLARE_TYPE(AspeedSDHCIState, AspeedSDHCIClass, ASPEED_SDHCI)
 
 #define ASPEED_SDHCI_NUM_SLOTS    2
-- 
2.34.1

[PATCH v2 5/6] aspeed/soc: Support SDHCI for AST2700

[Jamin Lin via]

Add SDHCI model for AST2700 SDHCI support. The SDHCI controller only support 1
slot and registers base address is start at 0x1408_0000 and its interrupt is
connected to GICINT133_INTC at bit 1.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed_ast27x0.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 63d1fcb086..baddd35ecf 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -65,6 +65,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
     [ASPEED_DEV_I2C]       =  0x14C0F000,
     [ASPEED_DEV_GPIO]      =  0x14C0B000,
     [ASPEED_DEV_RTC]       =  0x12C0F000,
+    [ASPEED_DEV_SDHCI]     =  0x14080000,
 };
 
 #define AST2700_MAX_IRQ 256
@@ -113,6 +114,7 @@ static const int aspeed_soc_ast2700_irqmap[] = {
     [ASPEED_DEV_KCS]       = 128,
     [ASPEED_DEV_DP]        = 28,
     [ASPEED_DEV_I3C]       = 131,
+    [ASPEED_DEV_SDHCI]     = 133,
 };
 
 /* GICINT 128 */
@@ -158,6 +160,7 @@ static const int aspeed_soc_ast2700_gic132_intcmap[] = {
 
 /* GICINT 133 */
 static const int aspeed_soc_ast2700_gic133_intcmap[] = {
+    [ASPEED_DEV_SDHCI]     = 1,
     [ASPEED_DEV_PECI]      = 4,
 };
 
@@ -380,6 +383,14 @@ static void aspeed_soc_ast2700_init(Object *obj)
     object_initialize_child(obj, "gpio", &s->gpio, typename);
 
     object_initialize_child(obj, "rtc", &s->rtc, TYPE_ASPEED_RTC);
+
+    snprintf(typename, sizeof(typename), "aspeed.sdhci-%s", socname);
+    object_initialize_child(obj, "sd-controller", &s->sdhci, typename);
+    object_property_set_int(OBJECT(&s->sdhci), "num-slots", 1, &error_abort);
+
+    /* Init sd card slot class here so that they're under the correct parent */
+    object_initialize_child(obj, "sd-controller.sdhci",
+                            &s->sdhci.slots[0], TYPE_SYSBUS_SDHCI);
 }
 
 /*
@@ -681,6 +692,15 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->rtc), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_RTC));
 
+    /* SDHCI */
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->sdhci), errp)) {
+        return;
+    }
+    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->sdhci), 0,
+                    sc->memmap[ASPEED_DEV_SDHCI]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
+                       aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
+
     create_unimplemented_device("ast2700.dpmcu", 0x11000000, 0x40000);
     create_unimplemented_device("ast2700.iomem0", 0x12000000, 0x01000000);
     create_unimplemented_device("ast2700.iomem1", 0x14000000, 0x01000000);
-- 
2.34.1

[PATCH v2 6/6] aspeed/soc: Support eMMC for AST2700

[Jamin Lin via]

Add SDHCI model for AST2700 eMMC support. The eMMC controller only support 1
slot and registers base address is start at 0x1209_0000 and its interrupt is
connected to GICINT 15.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed_ast27x0.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index baddd35ecf..23571584b2 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -391,6 +391,12 @@ static void aspeed_soc_ast2700_init(Object *obj)
     /* Init sd card slot class here so that they're under the correct parent */
     object_initialize_child(obj, "sd-controller.sdhci",
                             &s->sdhci.slots[0], TYPE_SYSBUS_SDHCI);
+
+    object_initialize_child(obj, "emmc-controller", &s->emmc, typename);
+    object_property_set_int(OBJECT(&s->emmc), "num-slots", 1, &error_abort);
+
+    object_initialize_child(obj, "emmc-controller.sdhci", &s->emmc.slots[0],
+                            TYPE_SYSBUS_SDHCI);
 }
 
 /*
@@ -701,6 +707,15 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
 
+    /* eMMC */
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->emmc), errp)) {
+        return;
+    }
+    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->emmc), 0,
+                    sc->memmap[ASPEED_DEV_EMMC]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->emmc), 0,
+                       aspeed_soc_get_irq(s, ASPEED_DEV_EMMC));
+
     create_unimplemented_device("ast2700.dpmcu", 0x11000000, 0x40000);
     create_unimplemented_device("ast2700.iomem0", 0x12000000, 0x01000000);
     create_unimplemented_device("ast2700.iomem1", 0x14000000, 0x01000000);
-- 
2.34.1

Re: [PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class member to set the different Capability Registers.

[Philippe Mathieu-Daudé]

On 3/12/24 03:14, Jamin Lin via wrote:
> Currently, it set the hardcode value of capability registers to all ASPEED SOCs
> However, the value of capability registers should be different for all ASPEED
> SOCs. For example: the bit 28 of the Capability Register 1 should be 1 for
> 64-bits System Bus support for AST2700.
> 
> Introduce a new "capareg" class member whose data type is uint_64 to set the
> different Capability Registers to all ASPEED SOCs.
> 
> The value of Capability Register is "0x0000000001e80080" for AST2400 and
> AST2500. The value of Capability Register is "0x0000000701f80080" for AST2600.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/arm/aspeed_ast2400.c      |  3 +-
>   hw/arm/aspeed_ast2600.c      |  7 ++--
>   hw/sd/aspeed_sdhci.c         | 72 +++++++++++++++++++++++++++++++-----
>   include/hw/sd/aspeed_sdhci.h | 12 +++++-
>   4 files changed, 78 insertions(+), 16 deletions(-)


> -DEFINE_TYPES(aspeed_sdhci_types)

> +type_init(aspeed_sdhci_register_types);

Please do not re-introduce type_init() calls. We want
to replace them by DEFINE_TYPES().

Re: [PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class member to set the different Capability Registers.

[Bernhard Beschow]

Am 3. Dezember 2024 02:14:57 UTC schrieb Jamin Lin via <qemu-devel@nongnu.org>:
>Currently, it set the hardcode value of capability registers to all ASPEED SOCs
>However, the value of capability registers should be different for all ASPEED
>SOCs. For example: the bit 28 of the Capability Register 1 should be 1 for
>64-bits System Bus support for AST2700.
>
>Introduce a new "capareg" class member whose data type is uint_64 to set the
>different Capability Registers to all ASPEED SOCs.
>
>The value of Capability Register is "0x0000000001e80080" for AST2400 and
>AST2500. The value of Capability Register is "0x0000000701f80080" for AST2600.
>
>Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>---
> hw/arm/aspeed_ast2400.c      |  3 +-
> hw/arm/aspeed_ast2600.c      |  7 ++--
> hw/sd/aspeed_sdhci.c         | 72 +++++++++++++++++++++++++++++++-----
> include/hw/sd/aspeed_sdhci.h | 12 +++++-
> 4 files changed, 78 insertions(+), 16 deletions(-)
>
>diff --git a/hw/arm/aspeed_ast2400.c b/hw/arm/aspeed_ast2400.c
>index ecc81ecc79..3c1b419945 100644
>--- a/hw/arm/aspeed_ast2400.c
>+++ b/hw/arm/aspeed_ast2400.c
>@@ -224,7 +224,8 @@ static void aspeed_ast2400_soc_init(Object *obj)
>     snprintf(typename, sizeof(typename), "aspeed.gpio-%s", socname);
>     object_initialize_child(obj, "gpio", &s->gpio, typename);
> 
>-    object_initialize_child(obj, "sdc", &s->sdhci, TYPE_ASPEED_SDHCI);
>+    snprintf(typename, sizeof(typename), "aspeed.sdhci-%s", socname);
>+    object_initialize_child(obj, "sdc", &s->sdhci, typename);
> 
>     object_property_set_int(OBJECT(&s->sdhci), "num-slots", 2, &error_abort);
> 
>diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>index c40d3d8443..b5703bd064 100644
>--- a/hw/arm/aspeed_ast2600.c
>+++ b/hw/arm/aspeed_ast2600.c
>@@ -236,8 +236,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
>     snprintf(typename, sizeof(typename), "aspeed.gpio-%s-1_8v", socname);
>     object_initialize_child(obj, "gpio_1_8v", &s->gpio_1_8v, typename);
> 
>-    object_initialize_child(obj, "sd-controller", &s->sdhci,
>-                            TYPE_ASPEED_SDHCI);
>+    snprintf(typename, sizeof(typename), "aspeed.sdhci-%s", socname);
>+    object_initialize_child(obj, "sd-controller", &s->sdhci, typename);
> 
>     object_property_set_int(OBJECT(&s->sdhci), "num-slots", 2, &error_abort);
> 
>@@ -247,8 +247,7 @@ static void aspeed_soc_ast2600_init(Object *obj)
>                                 &s->sdhci.slots[i], TYPE_SYSBUS_SDHCI);
>     }
> 
>-    object_initialize_child(obj, "emmc-controller", &s->emmc,
>-                            TYPE_ASPEED_SDHCI);
>+    object_initialize_child(obj, "emmc-controller", &s->emmc, typename);
> 
>     object_property_set_int(OBJECT(&s->emmc), "num-slots", 1, &error_abort);
> 
>diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
>index acd6538261..93f5571021 100644
>--- a/hw/sd/aspeed_sdhci.c
>+++ b/hw/sd/aspeed_sdhci.c
>@@ -148,6 +148,7 @@ static void aspeed_sdhci_realize(DeviceState *dev, Error **errp)
> {
>     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>     AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
>+    AspeedSDHCIClass *asc = ASPEED_SDHCI_GET_CLASS(sdhci);
> 
>     /* Create input irqs for the slots */
>     qdev_init_gpio_in_named_with_opaque(DEVICE(sbd), aspeed_sdhci_set_irq,
>@@ -167,7 +168,7 @@ static void aspeed_sdhci_realize(DeviceState *dev, Error **errp)
>         }
> 
>         if (!object_property_set_uint(sdhci_slot, "capareg",
>-                                      ASPEED_SDHCI_CAPABILITIES, errp)) {
>+                                      asc->capareg, errp)) {
>             return;
>         }
> 
>@@ -218,13 +219,66 @@ static void aspeed_sdhci_class_init(ObjectClass *classp, void *data)
>     device_class_set_props(dc, aspeed_sdhci_properties);
> }
> 
>-static const TypeInfo aspeed_sdhci_types[] = {
>-    {
>-        .name           = TYPE_ASPEED_SDHCI,
>-        .parent         = TYPE_SYS_BUS_DEVICE,
>-        .instance_size  = sizeof(AspeedSDHCIState),
>-        .class_init     = aspeed_sdhci_class_init,
>-    },

Why not just extend this array? It's made for registering multiple types, exactly what this patch is introducing.

>+static const TypeInfo aspeed_sdhci_info = {
>+    .name           = TYPE_ASPEED_SDHCI,
>+    .parent         = TYPE_SYS_BUS_DEVICE,
>+    .instance_size  = sizeof(AspeedSDHCIState),
>+    .class_init     = aspeed_sdhci_class_init,
>+    .class_size = sizeof(AspeedSDHCIClass),
>+    .abstract = true,
>+};
>+
>+static void aspeed_2400_sdhci_class_init(ObjectClass *klass, void *data)
>+{
>+    DeviceClass *dc = DEVICE_CLASS(klass);
>+    AspeedSDHCIClass *asc = ASPEED_SDHCI_CLASS(klass);
>+
>+    dc->desc = "ASPEED 2400 SDHCI Controller";
>+    asc->capareg = 0x0000000001e80080;
>+}
>+
>+static const TypeInfo aspeed_2400_sdhci_info = {
>+    .name = TYPE_ASPEED_2400_SDHCI,
>+    .parent = TYPE_ASPEED_SDHCI,
>+    .class_init = aspeed_2400_sdhci_class_init,
>+};
>+
>+static void aspeed_2500_sdhci_class_init(ObjectClass *klass, void *data)
>+{
>+    DeviceClass *dc = DEVICE_CLASS(klass);
>+    AspeedSDHCIClass *asc = ASPEED_SDHCI_CLASS(klass);
>+
>+    dc->desc = "ASPEED 2500 SDHCI Controller";
>+    asc->capareg = 0x0000000001e80080;
>+}
>+
>+static const TypeInfo aspeed_2500_sdhci_info = {
>+    .name = TYPE_ASPEED_2500_SDHCI,
>+    .parent = TYPE_ASPEED_SDHCI,
>+    .class_init = aspeed_2500_sdhci_class_init,
> };
> 
>-DEFINE_TYPES(aspeed_sdhci_types)
>+static void aspeed_2600_sdhci_class_init(ObjectClass *klass, void *data)
>+{
>+    DeviceClass *dc = DEVICE_CLASS(klass);
>+    AspeedSDHCIClass *asc = ASPEED_SDHCI_CLASS(klass);
>+
>+    dc->desc = "ASPEED 2600 SDHCI Controller";
>+    asc->capareg = 0x0000000701f80080;
>+}
>+
>+static const TypeInfo aspeed_2600_sdhci_info = {
>+    .name = TYPE_ASPEED_2600_SDHCI,
>+    .parent = TYPE_ASPEED_SDHCI,
>+    .class_init = aspeed_2600_sdhci_class_init,
>+};
>+
>+static void aspeed_sdhci_register_types(void)
>+{
>+    type_register_static(&aspeed_sdhci_info);
>+    type_register_static(&aspeed_2400_sdhci_info);
>+    type_register_static(&aspeed_2500_sdhci_info);
>+    type_register_static(&aspeed_2600_sdhci_info);
>+}
>+
>+type_init(aspeed_sdhci_register_types);
>diff --git a/include/hw/sd/aspeed_sdhci.h b/include/hw/sd/aspeed_sdhci.h
>index 057bc5f3d1..8083797e25 100644
>--- a/include/hw/sd/aspeed_sdhci.h
>+++ b/include/hw/sd/aspeed_sdhci.h
>@@ -13,9 +13,11 @@
> #include "qom/object.h"
> 
> #define TYPE_ASPEED_SDHCI "aspeed.sdhci"
>-OBJECT_DECLARE_SIMPLE_TYPE(AspeedSDHCIState, ASPEED_SDHCI)
>+#define TYPE_ASPEED_2400_SDHCI TYPE_ASPEED_SDHCI "-ast2400"
>+#define TYPE_ASPEED_2500_SDHCI TYPE_ASPEED_SDHCI "-ast2500"
>+#define TYPE_ASPEED_2600_SDHCI TYPE_ASPEED_SDHCI "-ast2600"
>+OBJECT_DECLARE_TYPE(AspeedSDHCIState, AspeedSDHCIClass, ASPEED_SDHCI)
> 
>-#define ASPEED_SDHCI_CAPABILITIES 0x01E80080
> #define ASPEED_SDHCI_NUM_SLOTS    2
> #define ASPEED_SDHCI_NUM_REGS     (ASPEED_SDHCI_REG_SIZE / sizeof(uint32_t))
> #define ASPEED_SDHCI_REG_SIZE     0x100
>@@ -32,4 +34,10 @@ struct AspeedSDHCIState {
>     uint32_t regs[ASPEED_SDHCI_NUM_REGS];
> };
> 
>+struct AspeedSDHCIClass {
>+    SysBusDeviceClass parent_class;
>+
>+    uint64_t capareg;
>+};

The struct seems not AST-specific and could be turned into a base class for all SDHCI device models, no? That way one could also add further device-specific constants other than capareg.

Best regards,
Bernhard

>+
> #endif /* ASPEED_SDHCI_H */

RE: [PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class member to set the different Capability Registers.

[Jamin Lin]

Hi Bernhard,

> Subject: Re: [PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class member
> to set the different Capability Registers.

> Am 3. Dezember 2024 02:14:57 UTC schrieb Jamin Lin via
> <qemu-devel@nongnu.org>:
> >Currently, it set the hardcode value of capability registers to all
> >ASPEED SOCs However, the value of capability registers should be
> >different for all ASPEED SOCs. For example: the bit 28 of the
> >Capability Register 1 should be 1 for 64-bits System Bus support for AST2700.
> >
> >Introduce a new "capareg" class member whose data type is uint_64 to
> >set the different Capability Registers to all ASPEED SOCs.
> >
> >The value of Capability Register is "0x0000000001e80080" for AST2400
> >and AST2500. The value of Capability Register is "0x0000000701f80080" for
> AST2600.
> >
> >Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> >---
> > hw/arm/aspeed_ast2400.c      |  3 +-
> > hw/arm/aspeed_ast2600.c      |  7 ++--
> > hw/sd/aspeed_sdhci.c         | 72
> +++++++++++++++++++++++++++++++-----
> > include/hw/sd/aspeed_sdhci.h | 12 +++++-
> > 4 files changed, 78 insertions(+), 16 deletions(-)
> >
> >diff --git a/hw/arm/aspeed_ast2400.c b/hw/arm/aspeed_ast2400.c index
> >ecc81ecc79..3c1b419945 100644
> >--- a/hw/arm/aspeed_ast2400.c
> >+++ b/hw/arm/aspeed_ast2400.c
> >@@ -224,7 +224,8 @@ static void aspeed_ast2400_soc_init(Object *obj)
> >     snprintf(typename, sizeof(typename), "aspeed.gpio-%s", socname);
> >     object_initialize_child(obj, "gpio", &s->gpio, typename);
> >
> >-    object_initialize_child(obj, "sdc", &s->sdhci, TYPE_ASPEED_SDHCI);
> >+    snprintf(typename, sizeof(typename), "aspeed.sdhci-%s", socname);
> >+    object_initialize_child(obj, "sdc", &s->sdhci, typename);
> >
> >     object_property_set_int(OBJECT(&s->sdhci), "num-slots", 2,
> > &error_abort);
> >
> >diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c index
> >c40d3d8443..b5703bd064 100644
> >--- a/hw/arm/aspeed_ast2600.c
> >+++ b/hw/arm/aspeed_ast2600.c
> >@@ -236,8 +236,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
> >     snprintf(typename, sizeof(typename), "aspeed.gpio-%s-1_8v",
> socname);
> >     object_initialize_child(obj, "gpio_1_8v", &s->gpio_1_8v,
> >typename);
> >
> >-    object_initialize_child(obj, "sd-controller", &s->sdhci,
> >-                            TYPE_ASPEED_SDHCI);
> >+    snprintf(typename, sizeof(typename), "aspeed.sdhci-%s", socname);
> >+    object_initialize_child(obj, "sd-controller", &s->sdhci,
> >+ typename);
> >
> >     object_property_set_int(OBJECT(&s->sdhci), "num-slots", 2,
> > &error_abort);
> >
> >@@ -247,8 +247,7 @@ static void aspeed_soc_ast2600_init(Object *obj)
> >                                 &s->sdhci.slots[i],
> TYPE_SYSBUS_SDHCI);
> >     }
> >
> >-    object_initialize_child(obj, "emmc-controller", &s->emmc,
> >-                            TYPE_ASPEED_SDHCI);
> >+    object_initialize_child(obj, "emmc-controller", &s->emmc,
> >+ typename);
> >
> >     object_property_set_int(OBJECT(&s->emmc), "num-slots", 1,
> > &error_abort);
> >
> >diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c index
> >acd6538261..93f5571021 100644
> >--- a/hw/sd/aspeed_sdhci.c
> >+++ b/hw/sd/aspeed_sdhci.c
> >@@ -148,6 +148,7 @@ static void aspeed_sdhci_realize(DeviceState *dev,
> >Error **errp)  {
> >     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >     AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
> >+    AspeedSDHCIClass *asc = ASPEED_SDHCI_GET_CLASS(sdhci);
> >
> >     /* Create input irqs for the slots */
> >     qdev_init_gpio_in_named_with_opaque(DEVICE(sbd),
> >aspeed_sdhci_set_irq, @@ -167,7 +168,7 @@ static void
> aspeed_sdhci_realize(DeviceState *dev, Error **errp)
> >         }
> >
> >         if (!object_property_set_uint(sdhci_slot, "capareg",
> >-                                      ASPEED_SDHCI_CAPABILITIES,
> errp)) {
> >+                                      asc->capareg, errp)) {
> >             return;
> >         }
> >
> >@@ -218,13 +219,66 @@ static void aspeed_sdhci_class_init(ObjectClass
> *classp, void *data)
> >     device_class_set_props(dc, aspeed_sdhci_properties);  }
> >
> >-static const TypeInfo aspeed_sdhci_types[] = {
> >-    {
> >-        .name           = TYPE_ASPEED_SDHCI,
> >-        .parent         = TYPE_SYS_BUS_DEVICE,
> >-        .instance_size  = sizeof(AspeedSDHCIState),
> >-        .class_init     = aspeed_sdhci_class_init,
> >-    },
> 
> Why not just extend this array? It's made for registering multiple types, exactly
> what this patch is introducing.
> 
> >+static const TypeInfo aspeed_sdhci_info = {
> >+    .name           = TYPE_ASPEED_SDHCI,
> >+    .parent         = TYPE_SYS_BUS_DEVICE,
> >+    .instance_size  = sizeof(AspeedSDHCIState),
> >+    .class_init     = aspeed_sdhci_class_init,
> >+    .class_size = sizeof(AspeedSDHCIClass),
> >+    .abstract = true,
> >+};
> >+
> >+static void aspeed_2400_sdhci_class_init(ObjectClass *klass, void
> >+*data) {
> >+    DeviceClass *dc = DEVICE_CLASS(klass);
> >+    AspeedSDHCIClass *asc = ASPEED_SDHCI_CLASS(klass);
> >+
> >+    dc->desc = "ASPEED 2400 SDHCI Controller";
> >+    asc->capareg = 0x0000000001e80080; }
> >+
> >+static const TypeInfo aspeed_2400_sdhci_info = {
> >+    .name = TYPE_ASPEED_2400_SDHCI,
> >+    .parent = TYPE_ASPEED_SDHCI,
> >+    .class_init = aspeed_2400_sdhci_class_init, };
> >+
> >+static void aspeed_2500_sdhci_class_init(ObjectClass *klass, void
> >+*data) {
> >+    DeviceClass *dc = DEVICE_CLASS(klass);
> >+    AspeedSDHCIClass *asc = ASPEED_SDHCI_CLASS(klass);
> >+
> >+    dc->desc = "ASPEED 2500 SDHCI Controller";
> >+    asc->capareg = 0x0000000001e80080; }
> >+
> >+static const TypeInfo aspeed_2500_sdhci_info = {
> >+    .name = TYPE_ASPEED_2500_SDHCI,
> >+    .parent = TYPE_ASPEED_SDHCI,
> >+    .class_init = aspeed_2500_sdhci_class_init,
> > };
> >
> >-DEFINE_TYPES(aspeed_sdhci_types)
> >+static void aspeed_2600_sdhci_class_init(ObjectClass *klass, void
> >+*data) {
> >+    DeviceClass *dc = DEVICE_CLASS(klass);
> >+    AspeedSDHCIClass *asc = ASPEED_SDHCI_CLASS(klass);
> >+
> >+    dc->desc = "ASPEED 2600 SDHCI Controller";
> >+    asc->capareg = 0x0000000701f80080; }
> >+
> >+static const TypeInfo aspeed_2600_sdhci_info = {
> >+    .name = TYPE_ASPEED_2600_SDHCI,
> >+    .parent = TYPE_ASPEED_SDHCI,
> >+    .class_init = aspeed_2600_sdhci_class_init, };
> >+
> >+static void aspeed_sdhci_register_types(void) {
> >+    type_register_static(&aspeed_sdhci_info);
> >+    type_register_static(&aspeed_2400_sdhci_info);
> >+    type_register_static(&aspeed_2500_sdhci_info);
> >+    type_register_static(&aspeed_2600_sdhci_info);
> >+}
> >+
> >+type_init(aspeed_sdhci_register_types);
> >diff --git a/include/hw/sd/aspeed_sdhci.h
> >b/include/hw/sd/aspeed_sdhci.h index 057bc5f3d1..8083797e25 100644
> >--- a/include/hw/sd/aspeed_sdhci.h
> >+++ b/include/hw/sd/aspeed_sdhci.h
> >@@ -13,9 +13,11 @@
> > #include "qom/object.h"
> >
> > #define TYPE_ASPEED_SDHCI "aspeed.sdhci"
> >-OBJECT_DECLARE_SIMPLE_TYPE(AspeedSDHCIState, ASPEED_SDHCI)
> >+#define TYPE_ASPEED_2400_SDHCI TYPE_ASPEED_SDHCI "-ast2400"
> >+#define TYPE_ASPEED_2500_SDHCI TYPE_ASPEED_SDHCI "-ast2500"
> >+#define TYPE_ASPEED_2600_SDHCI TYPE_ASPEED_SDHCI "-ast2600"
> >+OBJECT_DECLARE_TYPE(AspeedSDHCIState, AspeedSDHCIClass,
> ASPEED_SDHCI)
> >
> >-#define ASPEED_SDHCI_CAPABILITIES 0x01E80080
> > #define ASPEED_SDHCI_NUM_SLOTS    2
> > #define ASPEED_SDHCI_NUM_REGS     (ASPEED_SDHCI_REG_SIZE /
> sizeof(uint32_t))
> > #define ASPEED_SDHCI_REG_SIZE     0x100
> >@@ -32,4 +34,10 @@ struct AspeedSDHCIState {
> >     uint32_t regs[ASPEED_SDHCI_NUM_REGS];  };
> >
> >+struct AspeedSDHCIClass {
> >+    SysBusDeviceClass parent_class;
> >+
> >+    uint64_t capareg;
> >+};
> 
> The struct seems not AST-specific and could be turned into a base class for all
> SDHCI device models, no? That way one could also add further device-specific
> constants other than capareg.
> 

Thanks for suggestion and review.

The common sdhci model(sdhci-internal.h) had this "capareg" property to make specific SDHCI model of ASPEED SOCs
to set the different value Capability Register such as aspeed_sdhci.c
https://github.com/qemu/qemu/blob/master/hw/sd/sdhci-internal.h#L318

In the previous design of aspeed_sdhci.c, it set the hardcode value of Capability Registers for all ASPEED SOCs.
This patch set the different value of SDHCI Capability for AST2400, AST2500, AST2600 and AST2700.
Thanks-Jamin
> Best regards,
> Bernhard
> 
> >+
> > #endif /* ASPEED_SDHCI_H */

Re: [PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class member to set the different Capability Registers.

[Philippe Mathieu-Daudé]

On 4/12/24 04:14, Jamin Lin wrote:
> Hi Bernhard,
> 
>> Subject: Re: [PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class member
>> to set the different Capability Registers.
> 
>> Am 3. Dezember 2024 02:14:57 UTC schrieb Jamin Lin via
>> <qemu-devel@nongnu.org>:
>>> Currently, it set the hardcode value of capability registers to all
>>> ASPEED SOCs However, the value of capability registers should be
>>> different for all ASPEED SOCs. For example: the bit 28 of the
>>> Capability Register 1 should be 1 for 64-bits System Bus support for AST2700.
>>>
>>> Introduce a new "capareg" class member whose data type is uint_64 to
>>> set the different Capability Registers to all ASPEED SOCs.
>>>
>>> The value of Capability Register is "0x0000000001e80080" for AST2400
>>> and AST2500. The value of Capability Register is "0x0000000701f80080" for
>> AST2600.
>>>
>>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>>> ---
>>> hw/arm/aspeed_ast2400.c      |  3 +-
>>> hw/arm/aspeed_ast2600.c      |  7 ++--
>>> hw/sd/aspeed_sdhci.c         | 72
>> +++++++++++++++++++++++++++++++-----
>>> include/hw/sd/aspeed_sdhci.h | 12 +++++-
>>> 4 files changed, 78 insertions(+), 16 deletions(-)


>>> diff --git a/include/hw/sd/aspeed_sdhci.h
>>> b/include/hw/sd/aspeed_sdhci.h index 057bc5f3d1..8083797e25 100644
>>> --- a/include/hw/sd/aspeed_sdhci.h
>>> +++ b/include/hw/sd/aspeed_sdhci.h
>>> @@ -13,9 +13,11 @@
>>> #include "qom/object.h"
>>>
>>> #define TYPE_ASPEED_SDHCI "aspeed.sdhci"
>>> -OBJECT_DECLARE_SIMPLE_TYPE(AspeedSDHCIState, ASPEED_SDHCI)
>>> +#define TYPE_ASPEED_2400_SDHCI TYPE_ASPEED_SDHCI "-ast2400"
>>> +#define TYPE_ASPEED_2500_SDHCI TYPE_ASPEED_SDHCI "-ast2500"
>>> +#define TYPE_ASPEED_2600_SDHCI TYPE_ASPEED_SDHCI "-ast2600"
>>> +OBJECT_DECLARE_TYPE(AspeedSDHCIState, AspeedSDHCIClass,
>> ASPEED_SDHCI)
>>>
>>> -#define ASPEED_SDHCI_CAPABILITIES 0x01E80080
>>> #define ASPEED_SDHCI_NUM_SLOTS    2
>>> #define ASPEED_SDHCI_NUM_REGS     (ASPEED_SDHCI_REG_SIZE /
>> sizeof(uint32_t))
>>> #define ASPEED_SDHCI_REG_SIZE     0x100
>>> @@ -32,4 +34,10 @@ struct AspeedSDHCIState {
>>>      uint32_t regs[ASPEED_SDHCI_NUM_REGS];  };
>>>
>>> +struct AspeedSDHCIClass {
>>> +    SysBusDeviceClass parent_class;
>>> +
>>> +    uint64_t capareg;
>>> +};
>>
>> The struct seems not AST-specific and could be turned into a base class for all
>> SDHCI device models, no? That way one could also add further device-specific
>> constants other than capareg.
>>
> 
> Thanks for suggestion and review.
> 
> The common sdhci model(sdhci-internal.h) had this "capareg" property to make specific SDHCI model of ASPEED SOCs
> to set the different value Capability Register such as aspeed_sdhci.c
> https://github.com/qemu/qemu/blob/master/hw/sd/sdhci-internal.h#L318

DEFINE_SDHCI_COMMON_PROPERTIES() only sets default values,
you can overwrite them in your class_init().

> 
> In the previous design of aspeed_sdhci.c, it set the hardcode value of Capability Registers for all ASPEED SOCs.
> This patch set the different value of SDHCI Capability for AST2400, AST2500, AST2600 and AST2700.
> Thanks-Jamin
>> Best regards,
>> Bernhard
>>
>>> +
>>> #endif /* ASPEED_SDHCI_H */

RE: [PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class member to set the different Capability Registers.

[Jamin Lin]

Hi Philippe,

> Subject: Re: [PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class member
> to set the different Capability Registers.
> 
> On 3/12/24 03:14, Jamin Lin via wrote:
> > Currently, it set the hardcode value of capability registers to all
> > ASPEED SOCs However, the value of capability registers should be
> > different for all ASPEED SOCs. For example: the bit 28 of the
> > Capability Register 1 should be 1 for 64-bits System Bus support for AST2700.
> >
> > Introduce a new "capareg" class member whose data type is uint_64 to
> > set the different Capability Registers to all ASPEED SOCs.
> >
> > The value of Capability Register is "0x0000000001e80080" for AST2400
> > and AST2500. The value of Capability Register is "0x0000000701f80080" for
> AST2600.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >   hw/arm/aspeed_ast2400.c      |  3 +-
> >   hw/arm/aspeed_ast2600.c      |  7 ++--
> >   hw/sd/aspeed_sdhci.c         | 72
> +++++++++++++++++++++++++++++++-----
> >   include/hw/sd/aspeed_sdhci.h | 12 +++++-
> >   4 files changed, 78 insertions(+), 16 deletions(-)
> 
> 
> > -DEFINE_TYPES(aspeed_sdhci_types)
> 
> > +type_init(aspeed_sdhci_register_types);
> 
> Please do not re-introduce type_init() calls. We want to replace them by
> DEFINE_TYPES().

Thanks for review and suggestion.
Will fix it.

Jamin

RE: [PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class member to set the different Capability Registers.

[Jamin Lin]

Hi Philippe,

> Subject: Re: [PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class member
> to set the different Capability Registers.
> 
> On 4/12/24 04:14, Jamin Lin wrote:
> > Hi Bernhard,
> >
> >> Subject: Re: [PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class
> >> member to set the different Capability Registers.
> >
> >> Am 3. Dezember 2024 02:14:57 UTC schrieb Jamin Lin via
> >> <qemu-devel@nongnu.org>:
> >>> Currently, it set the hardcode value of capability registers to all
> >>> ASPEED SOCs However, the value of capability registers should be
> >>> different for all ASPEED SOCs. For example: the bit 28 of the
> >>> Capability Register 1 should be 1 for 64-bits System Bus support for
> AST2700.
> >>>
> >>> Introduce a new "capareg" class member whose data type is uint_64 to
> >>> set the different Capability Registers to all ASPEED SOCs.
> >>>
> >>> The value of Capability Register is "0x0000000001e80080" for AST2400
> >>> and AST2500. The value of Capability Register is
> >>> "0x0000000701f80080" for
> >> AST2600.
> >>>
> >>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> >>> ---
> >>> hw/arm/aspeed_ast2400.c      |  3 +-
> >>> hw/arm/aspeed_ast2600.c      |  7 ++--
> >>> hw/sd/aspeed_sdhci.c         | 72
> >> +++++++++++++++++++++++++++++++-----
> >>> include/hw/sd/aspeed_sdhci.h | 12 +++++-
> >>> 4 files changed, 78 insertions(+), 16 deletions(-)
> 
> 
> >>> diff --git a/include/hw/sd/aspeed_sdhci.h
> >>> b/include/hw/sd/aspeed_sdhci.h index 057bc5f3d1..8083797e25 100644
> >>> --- a/include/hw/sd/aspeed_sdhci.h
> >>> +++ b/include/hw/sd/aspeed_sdhci.h
> >>> @@ -13,9 +13,11 @@
> >>> #include "qom/object.h"
> >>>
> >>> #define TYPE_ASPEED_SDHCI "aspeed.sdhci"
> >>> -OBJECT_DECLARE_SIMPLE_TYPE(AspeedSDHCIState, ASPEED_SDHCI)
> >>> +#define TYPE_ASPEED_2400_SDHCI TYPE_ASPEED_SDHCI "-ast2400"
> >>> +#define TYPE_ASPEED_2500_SDHCI TYPE_ASPEED_SDHCI "-ast2500"
> >>> +#define TYPE_ASPEED_2600_SDHCI TYPE_ASPEED_SDHCI "-ast2600"
> >>> +OBJECT_DECLARE_TYPE(AspeedSDHCIState, AspeedSDHCIClass,
> >> ASPEED_SDHCI)
> >>>
> >>> -#define ASPEED_SDHCI_CAPABILITIES 0x01E80080
> >>> #define ASPEED_SDHCI_NUM_SLOTS    2
> >>> #define ASPEED_SDHCI_NUM_REGS     (ASPEED_SDHCI_REG_SIZE /
> >> sizeof(uint32_t))
> >>> #define ASPEED_SDHCI_REG_SIZE     0x100
> >>> @@ -32,4 +34,10 @@ struct AspeedSDHCIState {
> >>>      uint32_t regs[ASPEED_SDHCI_NUM_REGS];  };
> >>>
> >>> +struct AspeedSDHCIClass {
> >>> +    SysBusDeviceClass parent_class;
> >>> +
> >>> +    uint64_t capareg;
> >>> +};
> >>
> >> The struct seems not AST-specific and could be turned into a base
> >> class for all SDHCI device models, no? That way one could also add
> >> further device-specific constants other than capareg.
> >>
> >
> > Thanks for suggestion and review.
> >
> > The common sdhci model(sdhci-internal.h) had this "capareg" property
> > to make specific SDHCI model of ASPEED SOCs to set the different value
> > Capability Register such as aspeed_sdhci.c
> > https://github.com/qemu/qemu/blob/master/hw/sd/sdhci-internal.h#L318
> 
> DEFINE_SDHCI_COMMON_PROPERTIES() only sets default values, you can
> overwrite them in your class_init().
> 
Thanks for suggestion.
Will update it.

Jamin

> >
> > In the previous design of aspeed_sdhci.c, it set the hardcode value of
> Capability Registers for all ASPEED SOCs.
> > This patch set the different value of SDHCI Capability for AST2400, AST2500,
> AST2600 and AST2700.
> > Thanks-Jamin
> >> Best regards,
> >> Bernhard
> >>
> >>> +
> >>> #endif /* ASPEED_SDHCI_H */