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