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

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 */

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 */

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 */

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 */

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().

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