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

[Jamin Lin]

Hi Philippe,

> Subject: Re: [PATCH v2 3/6] hw:sdhci: Introduce a new "capareg" class member
> to set the different Capability Registers.
> 
> 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().
> 
Thanks for suggestion.
Will update it.

Jamin

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