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