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

[Jamin Lin]

Hi Cedric, 

> Subject: Re: [PATCH v3 3/7] hw:sdhci: Introduce a new "capareg" class member
> to set the different Capability Registers
> 
> On 12/4/24 09:05, Jamin Lin 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         | 52
> +++++++++++++++++++++++++++++++++---
> >   include/hw/sd/aspeed_sdhci.h | 12 +++++++--
> >   4 files changed, 63 insertions(+), 11 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..ccaeefa75b 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, @@ -166,10 +167,7 @@ static void
> aspeed_sdhci_realize(DeviceState *dev, Error **errp)
> >               return;
> >           }
> >
> > -        if (!object_property_set_uint(sdhci_slot, "capareg",
> > -                                      ASPEED_SDHCI_CAPABILITIES,
> errp)) {
> > -            return;
> > -        }
> > +        sdhci->slots[i].capareg = asc->capareg;
> 
> I think we want to keep :
> 
>          if (!object_property_set_uint(sdhci_slot, "capareg",
>                                        asc->capareg, errp)) {
>              return;
>          }
> 
> 
Got it. Thanks

> Thanks,
> 
> C.
> 
> 
> >
> >           if (!sysbus_realize(sbd_slot, errp)) {
> >               return;
> > @@ -218,13 +216,59 @@ static void aspeed_sdhci_class_init(ObjectClass
> *classp, void *data)
> >       device_class_set_props(dc, aspeed_sdhci_properties);
> >   }
> >
> > +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 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 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_sdhci_types[] = {
> >       {
> >           .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,
> > +    },
> > +    {
> > +        .name = TYPE_ASPEED_2400_SDHCI,
> > +        .parent = TYPE_ASPEED_SDHCI,
> > +        .class_init = aspeed_2400_sdhci_class_init,
> > +    },
> > +    {
> > +        .name = TYPE_ASPEED_2500_SDHCI,
> > +        .parent = TYPE_ASPEED_SDHCI,
> > +        .class_init = aspeed_2500_sdhci_class_init,
> > +    },
> > +    {
> > +        .name = TYPE_ASPEED_2600_SDHCI,
> > +        .parent = TYPE_ASPEED_SDHCI,
> > +        .class_init = aspeed_2600_sdhci_class_init,
> >       },
> >   };
> >
> >   DEFINE_TYPES(aspeed_sdhci_types)
> > +
> > +
> 
> Drop the extra lines please.
> 
> 
> Thanks,
> 
> C.
> 
> 
> 
> > 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 */