1. Patchew
  2. QEMU
  3. [PATCH v4 00/14] hw/sd/sdhci: Set reset value of interrupt registers
  4. View patch

[PATCH v4 04/14] hw/sd/sdhci: Make quirks a class property

Philippe Mathieu-Daudé posted 14 patches 11 months, 1 week ago
There is a newer version of this series
[PATCH v4 04/14] hw/sd/sdhci: Make quirks a class property
Posted by Philippe Mathieu-Daudé 11 months, 1 week ago 
All TYPE_IMX_USDHC instances use the quirk:
move it to the class layer.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/sd/sdhci.h |  3 ++-
 hw/sd/sdhci.c         | 15 +++++++++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index c4b20db3877..0616ce3aa59 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -95,7 +95,6 @@ struct SDHCIState {
 
     /* Configurable properties */
     bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
-    uint32_t quirks;
     uint8_t endianness;
     uint8_t sd_spec_version;
     uint8_t uhs_mode;
@@ -112,6 +111,8 @@ typedef struct SDHCIClass {
         PCIDeviceClass pci_parent_class;
         SysBusDeviceClass sbd_parent_class;
     };
+
+    uint32_t quirks;
 } SDHCIClass;
 
 /*
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 4917a9b3632..2b7eb11a14a 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -345,6 +345,8 @@ static void sdhci_send_command(SDHCIState *s)
     rlen = sdbus_do_command(&s->sdbus, &request, response);
 
     if (s->cmdreg & SDHC_CMD_RESPONSE) {
+        SDHCIClass *sc = SYSBUS_SDHCI_GET_CLASS(s);
+
         if (rlen == 4) {
             s->rspreg[0] = ldl_be_p(response);
             s->rspreg[1] = s->rspreg[2] = s->rspreg[3] = 0;
@@ -366,7 +368,7 @@ static void sdhci_send_command(SDHCIState *s)
             }
         }
 
-        if (!(s->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
+        if (!(sc->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
             (s->norintstsen & SDHC_NISEN_TRSCMP) &&
             (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
             s->norintsts |= SDHC_NIS_TRSCMP;
@@ -1886,7 +1888,15 @@ static void imx_usdhc_init(Object *obj)
     SDHCIState *s = SYSBUS_SDHCI(obj);
 
     s->io_ops = &usdhc_mmio_ops;
-    s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
+}
+
+static void imx_usdhc_class_init(ObjectClass *oc, void *data)
+{
+    SDHCIClass *sc = SYSBUS_SDHCI_CLASS(oc);
+
+    sc->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
+
+    sdhci_common_class_init(oc, data);
 }
 
 /* --- qdev Samsung s3c --- */
@@ -1967,6 +1977,7 @@ static const TypeInfo sdhci_types[] = {
         .name = TYPE_IMX_USDHC,
         .parent = TYPE_SYSBUS_SDHCI,
         .instance_init = imx_usdhc_init,
+        .class_init = imx_usdhc_class_init,
     },
     {
         .name = TYPE_S3C_SDHCI,
-- 
2.47.1
Re: [PATCH v4 04/14] hw/sd/sdhci: Make quirks a class property
Posted by BALATON Zoltan 11 months, 1 week ago 
On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
> All TYPE_IMX_USDHC instances use the quirk:
> move it to the class layer.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> include/hw/sd/sdhci.h |  3 ++-
> hw/sd/sdhci.c         | 15 +++++++++++++--
> 2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index c4b20db3877..0616ce3aa59 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -95,7 +95,6 @@ struct SDHCIState {
>
>     /* Configurable properties */
>     bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
> -    uint32_t quirks;
>     uint8_t endianness;
>     uint8_t sd_spec_version;
>     uint8_t uhs_mode;
> @@ -112,6 +111,8 @@ typedef struct SDHCIClass {
>         PCIDeviceClass pci_parent_class;
>         SysBusDeviceClass sbd_parent_class;
>     };
> +
> +    uint32_t quirks;
> } SDHCIClass;
>
> /*
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 4917a9b3632..2b7eb11a14a 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -345,6 +345,8 @@ static void sdhci_send_command(SDHCIState *s)
>     rlen = sdbus_do_command(&s->sdbus, &request, response);
>
>     if (s->cmdreg & SDHC_CMD_RESPONSE) {
> +        SDHCIClass *sc = SYSBUS_SDHCI_GET_CLASS(s);

I don't like this because it introduces a class look up which may be 
costly in a function that could be called frequently. Maybe you could just 
drop this patch and leave the quirk handling as it is. Changing it does 
not seem to improve the model much.

Regards,
BALATON Zoltan

> +
>         if (rlen == 4) {
>             s->rspreg[0] = ldl_be_p(response);
>             s->rspreg[1] = s->rspreg[2] = s->rspreg[3] = 0;
> @@ -366,7 +368,7 @@ static void sdhci_send_command(SDHCIState *s)
>             }
>         }
>
> -        if (!(s->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
> +        if (!(sc->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
>             (s->norintstsen & SDHC_NISEN_TRSCMP) &&
>             (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
>             s->norintsts |= SDHC_NIS_TRSCMP;
> @@ -1886,7 +1888,15 @@ static void imx_usdhc_init(Object *obj)
>     SDHCIState *s = SYSBUS_SDHCI(obj);
>
>     s->io_ops = &usdhc_mmio_ops;
> -    s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
> +}
> +
> +static void imx_usdhc_class_init(ObjectClass *oc, void *data)
> +{
> +    SDHCIClass *sc = SYSBUS_SDHCI_CLASS(oc);
> +
> +    sc->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
> +
> +    sdhci_common_class_init(oc, data);
> }
>
> /* --- qdev Samsung s3c --- */
> @@ -1967,6 +1977,7 @@ static const TypeInfo sdhci_types[] = {
>         .name = TYPE_IMX_USDHC,
>         .parent = TYPE_SYSBUS_SDHCI,
>         .instance_init = imx_usdhc_init,
> +        .class_init = imx_usdhc_class_init,
>     },
>     {
>         .name = TYPE_S3C_SDHCI,
>
Re: [PATCH v4 04/14] hw/sd/sdhci: Make quirks a class property
Posted by Philippe Mathieu-Daudé 11 months, 1 week ago 
On 9/3/25 12:43, BALATON Zoltan wrote:
> On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
>> All TYPE_IMX_USDHC instances use the quirk:
>> move it to the class layer.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> include/hw/sd/sdhci.h |  3 ++-
>> hw/sd/sdhci.c         | 15 +++++++++++++--
>> 2 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>> index c4b20db3877..0616ce3aa59 100644
>> --- a/include/hw/sd/sdhci.h
>> +++ b/include/hw/sd/sdhci.h
>> @@ -95,7 +95,6 @@ struct SDHCIState {
>>
>>     /* Configurable properties */
>>     bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert 
>> int */
>> -    uint32_t quirks;
>>     uint8_t endianness;
>>     uint8_t sd_spec_version;
>>     uint8_t uhs_mode;
>> @@ -112,6 +111,8 @@ typedef struct SDHCIClass {
>>         PCIDeviceClass pci_parent_class;
>>         SysBusDeviceClass sbd_parent_class;
>>     };
>> +
>> +    uint32_t quirks;
>> } SDHCIClass;
>>
>> /*
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 4917a9b3632..2b7eb11a14a 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -345,6 +345,8 @@ static void sdhci_send_command(SDHCIState *s)
>>     rlen = sdbus_do_command(&s->sdbus, &request, response);
>>
>>     if (s->cmdreg & SDHC_CMD_RESPONSE) {
>> +        SDHCIClass *sc = SYSBUS_SDHCI_GET_CLASS(s);
> 
> I don't like this because it introduces a class look up which may be 
> costly in a function that could be called frequently. Maybe you could 
> just drop this patch and leave the quirk handling as it is. Changing it 
> does not seem to improve the model much.

I thought about it and was expecting a such comment.

Initializing a class field in the instance_init is an anti-pattern,
so I'll keep a cached quirks in SDHCIState.

> Regards,
> BALATON Zoltan
Re: [PATCH v4 04/14] hw/sd/sdhci: Make quirks a class property
Posted by BALATON Zoltan 11 months, 1 week ago 
On Sun, 9 Mar 2025, Philippe Mathieu-Daudé wrote:
> On 9/3/25 12:43, BALATON Zoltan wrote:
>> On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
>>> All TYPE_IMX_USDHC instances use the quirk:
>>> move it to the class layer.
>>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> include/hw/sd/sdhci.h |  3 ++-
>>> hw/sd/sdhci.c         | 15 +++++++++++++--
>>> 2 files changed, 15 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>>> index c4b20db3877..0616ce3aa59 100644
>>> --- a/include/hw/sd/sdhci.h
>>> +++ b/include/hw/sd/sdhci.h
>>> @@ -95,7 +95,6 @@ struct SDHCIState {
>>> 
>>>     /* Configurable properties */
>>>     bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int 
>>> */
>>> -    uint32_t quirks;
>>>     uint8_t endianness;
>>>     uint8_t sd_spec_version;
>>>     uint8_t uhs_mode;
>>> @@ -112,6 +111,8 @@ typedef struct SDHCIClass {
>>>         PCIDeviceClass pci_parent_class;
>>>         SysBusDeviceClass sbd_parent_class;
>>>     };
>>> +
>>> +    uint32_t quirks;
>>> } SDHCIClass;
>>> 
>>> /*
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index 4917a9b3632..2b7eb11a14a 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -345,6 +345,8 @@ static void sdhci_send_command(SDHCIState *s)
>>>     rlen = sdbus_do_command(&s->sdbus, &request, response);
>>> 
>>>     if (s->cmdreg & SDHC_CMD_RESPONSE) {
>>> +        SDHCIClass *sc = SYSBUS_SDHCI_GET_CLASS(s);
>> 
>> I don't like this because it introduces a class look up which may be costly 
>> in a function that could be called frequently. Maybe you could just drop 
>> this patch and leave the quirk handling as it is. Changing it does not seem 
>> to improve the model much.
>
> I thought about it and was expecting a such comment.
>
> Initializing a class field in the instance_init is an anti-pattern,

Why? It's not a class field, it's in SDHCIState (we have another quirk for 
rpi already there which could also use the same bits). But you could also 
init in realize instead if that's better then no need to duplicate it in 
class struct just to copy it to every instance.

Regards,
BALATON Zoltan

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.