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_...@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 */
