Re: [PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class member to set the different Capability Registers.

[Philippe Mathieu-Daudé]

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