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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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