[PATCH v5 00/14] hw/sd/sdhci: Set reset value of interrupt registers

[PATCH v5 00/14] hw/sd/sdhci: Set reset value of interrupt registers

[Philippe Mathieu-Daudé]

Since v4:
- Convert quirks (Zoltan)
- Cache class to avoid invalid cast with PCI
- Remove 'endianness' property (Bernhard)

Since v3:
- Fix "hw/qdev-properties-system.h" (first patch)
- Convert to EndianMode (patch #10)

Rainy saturday, time for some hobbyist contributions :)

In this series we try to address the issue Zoltan reported
and try to fix in [*], but using a more generic approach.
The SDHCI code ends up better consolidated and ready to
scale for more vendor implementations.

[*] https://lore.kernel.org/qemu-devel/20250210160329.DDA7F4E600E@zero.eik.bme.hu/

Philippe Mathieu-Daudé (14):
  hw/qdev-properties-system: Include missing 'qapi/qapi-types-common.h'
  hw/sd/sdhci: Remove need for SDHCIState::vendor field
  hw/sd/sdhci: Redefine SDHCI_QUIRK_NO_BUSY_IRQ bitmask as bit
  hw/sd/sdhci: Include 'wp-inverted' property in quirk bitmask
  hw/sd/sdhci: Include 'pending-insert-quirk' property in quirk bitmask
  hw/sd/sdhci: Introduce SDHCIClass stub
  hw/sd/sdhci: Make quirks a class property
  hw/sd/sdhci: Make I/O region size a class property
  hw/sd/sdhci: Allow SDHCI classes to register their own MemoryRegionOps
  hw/sd/sdhci: Allow SDHCI classes to register their own read-only regs
  hw/sd/sdhci: Allow SDHCI classes to have different register reset
    values
  hw/sd/sdhci: Implement Freescale eSDHC as TYPE_FSL_ESDHC
  hw/ppc/e500: Replace generic SDHCI by Freescale eSDHC
  hw/sd/sdhci: Remove unnecessary 'endianness' property

 hw/sd/sdhci-internal.h              |  26 ++---
 include/hw/qdev-properties-system.h |   1 +
 include/hw/sd/sdhci.h               |  83 ++++++++++----
 hw/arm/aspeed.c                     |   2 +-
 hw/arm/fsl-imx25.c                  |   2 -
 hw/arm/fsl-imx6.c                   |   2 -
 hw/arm/fsl-imx6ul.c                 |   2 -
 hw/arm/fsl-imx7.c                   |   2 -
 hw/arm/fsl-imx8mp.c                 |   2 -
 hw/ppc/e500.c                       |  12 +-
 hw/sd/sdhci-pci.c                   |   1 +
 hw/sd/sdhci.c                       | 166 ++++++++++++++++++----------
 12 files changed, 184 insertions(+), 117 deletions(-)

-- 
2.47.1

[PATCH v5 01/14] hw/qdev-properties-system: Include missing 'qapi/qapi-types-common.h'

[Philippe Mathieu-Daudé]

DEFINE_PROP_ENDIAN_NODEFAULT() macro uses ENDIAN_MODE_UNSPECIFIED
which is defined in "qapi/qapi-types-common.h".

Fixes: 4ec96630f93 ("hw/qdev-properties-system: Introduce EndianMode QAPI enum")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/qdev-properties-system.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
index b921392c525..49a3825eb46 100644
--- a/include/hw/qdev-properties-system.h
+++ b/include/hw/qdev-properties-system.h
@@ -1,6 +1,7 @@
 #ifndef HW_QDEV_PROPERTIES_SYSTEM_H
 #define HW_QDEV_PROPERTIES_SYSTEM_H
 
+#include "qapi/qapi-types-common.h"
 #include "hw/qdev-properties.h"
 
 bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str,
-- 
2.47.1

Re: [PATCH v5 01/14] hw/qdev-properties-system: Include missing 'qapi/qapi-types-common.h'

[Philippe Mathieu-Daudé]

+Markus

On 10/3/25 01:06, Philippe Mathieu-Daudé wrote:
> DEFINE_PROP_ENDIAN_NODEFAULT() macro uses ENDIAN_MODE_UNSPECIFIED
> which is defined in "qapi/qapi-types-common.h".
> 
> Fixes: 4ec96630f93 ("hw/qdev-properties-system: Introduce EndianMode QAPI enum")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/qdev-properties-system.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
> index b921392c525..49a3825eb46 100644
> --- a/include/hw/qdev-properties-system.h
> +++ b/include/hw/qdev-properties-system.h
> @@ -1,6 +1,7 @@
>   #ifndef HW_QDEV_PROPERTIES_SYSTEM_H
>   #define HW_QDEV_PROPERTIES_SYSTEM_H
>   
> +#include "qapi/qapi-types-common.h"
>   #include "hw/qdev-properties.h"
>   
>   bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str,

[PATCH v5 02/14] hw/sd/sdhci: Remove need for SDHCIState::vendor field

[Philippe Mathieu-Daudé]

All instances of TYPE_IMX_USDHC set vendor=SDHCI_VENDOR_IMX.
No need to special-case it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/sd/sdhci-internal.h |  1 -
 include/hw/sd/sdhci.h  |  4 ----
 hw/arm/fsl-imx25.c     |  2 --
 hw/arm/fsl-imx6.c      |  2 --
 hw/arm/fsl-imx6ul.c    |  2 --
 hw/arm/fsl-imx7.c      |  2 --
 hw/arm/fsl-imx8mp.c    |  2 --
 hw/sd/sdhci.c          | 14 ++++----------
 8 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 9f768c418e0..9072b06bdde 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -311,7 +311,6 @@ extern const VMStateDescription sdhci_vmstate;
     DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), \
     DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
     DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
-    DEFINE_PROP_UINT8("vendor", _state, vendor, SDHCI_VENDOR_NONE), \
     \
     /* Capabilities registers provide information on supported
      * features of this specific host controller implementation */ \
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 38c08e28598..48247e9a20f 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -99,7 +99,6 @@ struct SDHCIState {
     uint8_t endianness;
     uint8_t sd_spec_version;
     uint8_t uhs_mode;
-    uint8_t vendor;        /* For vendor specific functionality */
     /*
      * Write Protect pin default active low for detecting SD card
      * to be protected. Set wp_inverted to invert the signal.
@@ -108,9 +107,6 @@ struct SDHCIState {
 };
 typedef struct SDHCIState SDHCIState;
 
-#define SDHCI_VENDOR_NONE       0
-#define SDHCI_VENDOR_IMX        1
-
 /*
  * Controller does not provide transfer-complete interrupt when not
  * busy.
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 5359a6d8d3b..02214ca1a1c 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -243,8 +243,6 @@ static void fsl_imx25_realize(DeviceState *dev, Error **errp)
                                  &error_abort);
         object_property_set_uint(OBJECT(&s->esdhc[i]), "capareg",
                                  IMX25_ESDHC_CAPABILITIES, &error_abort);
-        object_property_set_uint(OBJECT(&s->esdhc[i]), "vendor",
-                                 SDHCI_VENDOR_IMX, &error_abort);
         if (!sysbus_realize(SYS_BUS_DEVICE(&s->esdhc[i]), errp)) {
             return;
         }
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index dc86338b3a5..a114dc0d63d 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -327,8 +327,6 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
                                  &error_abort);
         object_property_set_uint(OBJECT(&s->esdhc[i]), "capareg",
                                  IMX6_ESDHC_CAPABILITIES, &error_abort);
-        object_property_set_uint(OBJECT(&s->esdhc[i]), "vendor",
-                                 SDHCI_VENDOR_IMX, &error_abort);
         if (!sysbus_realize(SYS_BUS_DEVICE(&s->esdhc[i]), errp)) {
             return;
         }
diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
index 34c4aa15cd0..ce8d3ef535f 100644
--- a/hw/arm/fsl-imx6ul.c
+++ b/hw/arm/fsl-imx6ul.c
@@ -531,8 +531,6 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error **errp)
             FSL_IMX6UL_USDHC2_IRQ,
         };
 
-        object_property_set_uint(OBJECT(&s->usdhc[i]), "vendor",
-                                 SDHCI_VENDOR_IMX, &error_abort);
         sysbus_realize(SYS_BUS_DEVICE(&s->usdhc[i]), &error_abort);
 
         sysbus_mmio_map(SYS_BUS_DEVICE(&s->usdhc[i]), 0,
diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
index 3374018cde0..ed1f10bca26 100644
--- a/hw/arm/fsl-imx7.c
+++ b/hw/arm/fsl-imx7.c
@@ -471,8 +471,6 @@ static void fsl_imx7_realize(DeviceState *dev, Error **errp)
             FSL_IMX7_USDHC3_IRQ,
         };
 
-        object_property_set_uint(OBJECT(&s->usdhc[i]), "vendor",
-                                 SDHCI_VENDOR_IMX, &error_abort);
         sysbus_realize(SYS_BUS_DEVICE(&s->usdhc[i]), &error_abort);
 
         sysbus_mmio_map(SYS_BUS_DEVICE(&s->usdhc[i]), 0,
diff --git a/hw/arm/fsl-imx8mp.c b/hw/arm/fsl-imx8mp.c
index 1ea98e14635..c3f6da63220 100644
--- a/hw/arm/fsl-imx8mp.c
+++ b/hw/arm/fsl-imx8mp.c
@@ -524,8 +524,6 @@ static void fsl_imx8mp_realize(DeviceState *dev, Error **errp)
             { fsl_imx8mp_memmap[FSL_IMX8MP_USDHC3].addr, FSL_IMX8MP_USDHC3_IRQ },
         };
 
-        object_property_set_uint(OBJECT(&s->usdhc[i]), "vendor",
-                                 SDHCI_VENDOR_IMX, &error_abort);
         if (!sysbus_realize(SYS_BUS_DEVICE(&s->usdhc[i]), errp)) {
             return;
         }
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 1f45a77566c..149b748cbee 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1731,16 +1731,10 @@ usdhc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
 
     case USDHC_VENDOR_SPEC:
         s->vendor_spec = value;
-        switch (s->vendor) {
-        case SDHCI_VENDOR_IMX:
-            if (value & USDHC_IMX_FRC_SDCLK_ON) {
-                s->prnsts &= ~SDHC_IMX_CLOCK_GATE_OFF;
-            } else {
-                s->prnsts |= SDHC_IMX_CLOCK_GATE_OFF;
-            }
-            break;
-        default:
-            break;
+        if (value & USDHC_IMX_FRC_SDCLK_ON) {
+            s->prnsts &= ~SDHC_IMX_CLOCK_GATE_OFF;
+        } else {
+            s->prnsts |= SDHC_IMX_CLOCK_GATE_OFF;
         }
         break;
 
-- 
2.47.1

[PATCH v5 03/14] hw/sd/sdhci: Redefine SDHCI_QUIRK_NO_BUSY_IRQ bitmask as bit

[Philippe Mathieu-Daudé]

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

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 48247e9a20f..096d607f4b7 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -108,13 +108,13 @@ struct SDHCIState {
 typedef struct SDHCIState SDHCIState;
 
 /*
- * Controller does not provide transfer-complete interrupt when not
- * busy.
- *
  * NOTE: This definition is taken out of Linux kernel and so the
  * original bit number is preserved
  */
-#define SDHCI_QUIRK_NO_BUSY_IRQ    BIT(14)
+enum {
+    /* Controller does not provide transfer-complete interrupt when not busy. */
+    SDHCI_QUIRK_NO_BUSY_IRQ                     = 14,
+};
 
 #define TYPE_PCI_SDHCI "sdhci-pci"
 DECLARE_INSTANCE_CHECKER(SDHCIState, PCI_SDHCI,
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 149b748cbee..1dc942a0e06 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -366,7 +366,7 @@ static void sdhci_send_command(SDHCIState *s)
             }
         }
 
-        if (!(s->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
+        if (!(s->quirks & BIT(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 +1886,7 @@ 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;
+    s->quirks = BIT(SDHCI_QUIRK_NO_BUSY_IRQ);
 }
 
 /* --- qdev Samsung s3c --- */
-- 
2.47.1

Re: [PATCH v5 03/14] hw/sd/sdhci: Redefine SDHCI_QUIRK_NO_BUSY_IRQ bitmask as bit

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2039 bytes --]

On Mon, 10 Mar 2025, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> include/hw/sd/sdhci.h | 8 ++++----
> hw/sd/sdhci.c         | 4 ++--
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 48247e9a20f..096d607f4b7 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -108,13 +108,13 @@ struct SDHCIState {
> typedef struct SDHCIState SDHCIState;
>
> /*
> - * Controller does not provide transfer-complete interrupt when not
> - * busy.
> - *
>  * NOTE: This definition is taken out of Linux kernel and so the
>  * original bit number is preserved
>  */
> -#define SDHCI_QUIRK_NO_BUSY_IRQ    BIT(14)
> +enum {
> +    /* Controller does not provide transfer-complete interrupt when not busy. */
> +    SDHCI_QUIRK_NO_BUSY_IRQ                     = 14,
> +};

I think this is better as a define, then you don't have to remember to add 
BIT() on usage and prevent this churn. I'd say drop this patch and adjust 
the comment in next patch but if you're attached to it I don't mind that 
much.

Regards,
BALATON Zoltan

> #define TYPE_PCI_SDHCI "sdhci-pci"
> DECLARE_INSTANCE_CHECKER(SDHCIState, PCI_SDHCI,
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 149b748cbee..1dc942a0e06 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -366,7 +366,7 @@ static void sdhci_send_command(SDHCIState *s)
>             }
>         }
>
> -        if (!(s->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
> +        if (!(s->quirks & BIT(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 +1886,7 @@ 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;
> +    s->quirks = BIT(SDHCI_QUIRK_NO_BUSY_IRQ);
> }
>
> /* --- qdev Samsung s3c --- */
>

[PATCH v5 04/14] hw/sd/sdhci: Include 'wp-inverted' property in quirk bitmask

[Philippe Mathieu-Daudé]

Import Linux's SDHCI_QUIRK_INVERTED_WRITE_PROTECT quirk definition.

Replace 'wp_inverted' boolean by a bit in quirk bitmask.

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

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 096d607f4b7..d2e4f0f0050 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -30,7 +30,14 @@
 #include "hw/sd/sd.h"
 #include "qom/object.h"
 
-/* SD/MMC host controller state */
+/*
+ * SD/MMC host controller state
+ *
+ * QEMU interface:
+ *  + QOM property "wp-inverted-quirk" inverts the Write Protect pin
+ *    polarity (by default the polarity is active low for detecting SD
+ *    card to be protected).
+ */
 struct SDHCIState {
     /*< private >*/
     union {
@@ -99,11 +106,6 @@ struct SDHCIState {
     uint8_t endianness;
     uint8_t sd_spec_version;
     uint8_t uhs_mode;
-    /*
-     * Write Protect pin default active low for detecting SD card
-     * to be protected. Set wp_inverted to invert the signal.
-     */
-    bool wp_inverted;
 };
 typedef struct SDHCIState SDHCIState;
 
@@ -114,6 +116,8 @@ typedef struct SDHCIState SDHCIState;
 enum {
     /* Controller does not provide transfer-complete interrupt when not busy. */
     SDHCI_QUIRK_NO_BUSY_IRQ                     = 14,
+    /* Controller reports inverted write-protect state */
+    SDHCI_QUIRK_INVERTED_WRITE_PROTECT          = 16,
 };
 
 #define TYPE_PCI_SDHCI "sdhci-pci"
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 98bf071139b..daee2376d50 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -412,7 +412,7 @@ static void aspeed_machine_init(MachineState *machine)
     if (amc->sdhci_wp_inverted) {
         for (i = 0; i < bmc->soc->sdhci.num_slots; i++) {
             object_property_set_bool(OBJECT(&bmc->soc->sdhci.slots[i]),
-                                     "wp-inverted", true, &error_abort);
+                                     "wp-inverted-quirk", true, &error_abort);
         }
     }
     if (machine->kernel_filename) {
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 1dc942a0e06..19c600d5bfc 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -274,7 +274,7 @@ static void sdhci_set_readonly(DeviceState *dev, bool level)
 {
     SDHCIState *s = (SDHCIState *)dev;
 
-    if (s->wp_inverted) {
+    if (s->quirks & BIT(SDHCI_QUIRK_INVERTED_WRITE_PROTECT)) {
         level = !level;
     }
 
@@ -1555,12 +1555,12 @@ void sdhci_common_class_init(ObjectClass *klass, const void *data)
 
 static const Property sdhci_sysbus_properties[] = {
     DEFINE_SDHCI_COMMON_PROPERTIES(SDHCIState),
+    DEFINE_PROP_BIT("wp-inverted-quirk", SDHCIState, quirks,
+                    SDHCI_QUIRK_INVERTED_WRITE_PROTECT, false),
     DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
                      false),
     DEFINE_PROP_LINK("dma", SDHCIState,
                      dma_mr, TYPE_MEMORY_REGION, MemoryRegion *),
-    DEFINE_PROP_BOOL("wp-inverted", SDHCIState,
-                     wp_inverted, false),
 };
 
 static void sdhci_sysbus_init(Object *obj)
-- 
2.47.1

Re: [PATCH v5 04/14] hw/sd/sdhci: Include 'wp-inverted' property in quirk bitmask

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3640 bytes --]

On Mon, 10 Mar 2025, Philippe Mathieu-Daudé wrote:
> Import Linux's SDHCI_QUIRK_INVERTED_WRITE_PROTECT quirk definition.
>
> Replace 'wp_inverted' boolean by a bit in quirk bitmask.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> include/hw/sd/sdhci.h | 16 ++++++++++------
> hw/arm/aspeed.c       |  2 +-
> hw/sd/sdhci.c         |  6 +++---
> 3 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 096d607f4b7..d2e4f0f0050 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -30,7 +30,14 @@
> #include "hw/sd/sd.h"
> #include "qom/object.h"
>
> -/* SD/MMC host controller state */
> +/*
> + * SD/MMC host controller state
> + *
> + * QEMU interface:
> + *  + QOM property "wp-inverted-quirk" inverts the Write Protect pin
> + *    polarity (by default the polarity is active low for detecting SD
> + *    card to be protected).
> + */
> struct SDHCIState {
>     /*< private >*/
>     union {
> @@ -99,11 +106,6 @@ struct SDHCIState {
>     uint8_t endianness;
>     uint8_t sd_spec_version;
>     uint8_t uhs_mode;
> -    /*
> -     * Write Protect pin default active low for detecting SD card
> -     * to be protected. Set wp_inverted to invert the signal.
> -     */
> -    bool wp_inverted;
> };
> typedef struct SDHCIState SDHCIState;
>
> @@ -114,6 +116,8 @@ typedef struct SDHCIState SDHCIState;

Now that we have two adjust comment above here to say "These defines"

> enum {
>     /* Controller does not provide transfer-complete interrupt when not busy. */
>     SDHCI_QUIRK_NO_BUSY_IRQ                     = 14,
> +    /* Controller reports inverted write-protect state */
> +    SDHCI_QUIRK_INVERTED_WRITE_PROTECT          = 16,
> };

and I'd say keep this defines that also matches what Linux has.

> #define TYPE_PCI_SDHCI "sdhci-pci"
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 98bf071139b..daee2376d50 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -412,7 +412,7 @@ static void aspeed_machine_init(MachineState *machine)
>     if (amc->sdhci_wp_inverted) {
>         for (i = 0; i < bmc->soc->sdhci.num_slots; i++) {
>             object_property_set_bool(OBJECT(&bmc->soc->sdhci.slots[i]),
> -                                     "wp-inverted", true, &error_abort);
> +                                     "wp-inverted-quirk", true, &error_abort);

Why rename it? That would break command lines that use this.

Regards,
BALATON Zoltan

>         }
>     }
>     if (machine->kernel_filename) {
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 1dc942a0e06..19c600d5bfc 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -274,7 +274,7 @@ static void sdhci_set_readonly(DeviceState *dev, bool level)
> {
>     SDHCIState *s = (SDHCIState *)dev;
>
> -    if (s->wp_inverted) {
> +    if (s->quirks & BIT(SDHCI_QUIRK_INVERTED_WRITE_PROTECT)) {
>         level = !level;
>     }
>
> @@ -1555,12 +1555,12 @@ void sdhci_common_class_init(ObjectClass *klass, const void *data)
>
> static const Property sdhci_sysbus_properties[] = {
>     DEFINE_SDHCI_COMMON_PROPERTIES(SDHCIState),
> +    DEFINE_PROP_BIT("wp-inverted-quirk", SDHCIState, quirks,
> +                    SDHCI_QUIRK_INVERTED_WRITE_PROTECT, false),
>     DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>                      false),
>     DEFINE_PROP_LINK("dma", SDHCIState,
>                      dma_mr, TYPE_MEMORY_REGION, MemoryRegion *),
> -    DEFINE_PROP_BOOL("wp-inverted", SDHCIState,
> -                     wp_inverted, false),
> };
>
> static void sdhci_sysbus_init(Object *obj)
>

[PATCH v5 05/14] hw/sd/sdhci: Include 'pending-insert-quirk' property in quirk bitmask

[Philippe Mathieu-Daudé]

Import Linux's SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET quirk definition.

Replace 'pending_insert_quirk' boolean (originally introduce in commit
0a7ac9f9e72 "sdhci: quirk property for card insert interrupt status
on Raspberry Pi") by a bit in quirk bitmask.

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

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index d2e4f0f0050..2e6e719df7b 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -34,6 +34,8 @@
  * SD/MMC host controller state
  *
  * QEMU interface:
+ *  + QOM property "pending-insert-quirk" re-enables pending "card inserted"
+ *    IRQ after reset (used by the Raspberry Pi controllers).
  *  + QOM property "wp-inverted-quirk" inverts the Write Protect pin
  *    polarity (by default the polarity is active low for detecting SD
  *    card to be protected).
@@ -101,7 +103,6 @@ struct SDHCIState {
     /* RO Host Controller Version Register always reads as 0x2401 */
 
     /* Configurable properties */
-    bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
     uint32_t quirks;
     uint8_t endianness;
     uint8_t sd_spec_version;
@@ -118,6 +119,8 @@ enum {
     SDHCI_QUIRK_NO_BUSY_IRQ                     = 14,
     /* Controller reports inverted write-protect state */
     SDHCI_QUIRK_INVERTED_WRITE_PROTECT          = 16,
+    /* Controller losing signal/interrupt enable states after reset */
+    SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET        = 19,
 };
 
 #define TYPE_PCI_SDHCI "sdhci-pci"
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 19c600d5bfc..d1b1b187874 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -320,7 +320,7 @@ static void sdhci_poweron_reset(DeviceState *dev)
 
     sdhci_reset(s);
 
-    if (s->pending_insert_quirk) {
+    if (s->quirks & BIT(SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET)) {
         s->pending_insert_state = true;
     }
 }
@@ -1307,7 +1307,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
          * appears when first enabled after power on
          */
         if ((s->norintstsen & SDHC_NISEN_INSERT) && s->pending_insert_state) {
-            assert(s->pending_insert_quirk);
+            assert(s->quirks & BIT(SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET));
             s->norintsts |= SDHC_NIS_INSERT;
             s->pending_insert_state = false;
         }
@@ -1557,8 +1557,8 @@ static const Property sdhci_sysbus_properties[] = {
     DEFINE_SDHCI_COMMON_PROPERTIES(SDHCIState),
     DEFINE_PROP_BIT("wp-inverted-quirk", SDHCIState, quirks,
                     SDHCI_QUIRK_INVERTED_WRITE_PROTECT, false),
-    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
-                     false),
+    DEFINE_PROP_BIT("pending-insert-quirk", SDHCIState, quirks,
+                    SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET, false),
     DEFINE_PROP_LINK("dma", SDHCIState,
                      dma_mr, TYPE_MEMORY_REGION, MemoryRegion *),
 };
-- 
2.47.1

Re: [PATCH v5 05/14] hw/sd/sdhci: Include 'pending-insert-quirk' property in quirk bitmask

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3568 bytes --]

On Mon, 10 Mar 2025, Philippe Mathieu-Daudé wrote:
> Import Linux's SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET quirk definition.
>
> Replace 'pending_insert_quirk' boolean (originally introduce in commit
> 0a7ac9f9e72 "sdhci: quirk property for card insert interrupt status
> on Raspberry Pi") by a bit in quirk bitmask.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> include/hw/sd/sdhci.h | 5 ++++-
> hw/sd/sdhci.c         | 8 ++++----
> 2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index d2e4f0f0050..2e6e719df7b 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -34,6 +34,8 @@
>  * SD/MMC host controller state
>  *
>  * QEMU interface:
> + *  + QOM property "pending-insert-quirk" re-enables pending "card inserted"
> + *    IRQ after reset (used by the Raspberry Pi controllers).
>  *  + QOM property "wp-inverted-quirk" inverts the Write Protect pin
>  *    polarity (by default the polarity is active low for detecting SD
>  *    card to be protected).
> @@ -101,7 +103,6 @@ struct SDHCIState {
>     /* RO Host Controller Version Register always reads as 0x2401 */
>
>     /* Configurable properties */
> -    bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
>     uint32_t quirks;
>     uint8_t endianness;
>     uint8_t sd_spec_version;
> @@ -118,6 +119,8 @@ enum {
>     SDHCI_QUIRK_NO_BUSY_IRQ                     = 14,
>     /* Controller reports inverted write-protect state */
>     SDHCI_QUIRK_INVERTED_WRITE_PROTECT          = 16,
> +    /* Controller losing signal/interrupt enable states after reset */
> +    SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET        = 19,
> };
>
> #define TYPE_PCI_SDHCI "sdhci-pci"
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 19c600d5bfc..d1b1b187874 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -320,7 +320,7 @@ static void sdhci_poweron_reset(DeviceState *dev)
>
>     sdhci_reset(s);
>
> -    if (s->pending_insert_quirk) {
> +    if (s->quirks & BIT(SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET)) {
>         s->pending_insert_state = true;
>     }
> }
> @@ -1307,7 +1307,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>          * appears when first enabled after power on
>          */
>         if ((s->norintstsen & SDHC_NISEN_INSERT) && s->pending_insert_state) {
> -            assert(s->pending_insert_quirk);
> +            assert(s->quirks & BIT(SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET));
>             s->norintsts |= SDHC_NIS_INSERT;
>             s->pending_insert_state = false;
>         }
> @@ -1557,8 +1557,8 @@ static const Property sdhci_sysbus_properties[] = {
>     DEFINE_SDHCI_COMMON_PROPERTIES(SDHCIState),
>     DEFINE_PROP_BIT("wp-inverted-quirk", SDHCIState, quirks,
>                     SDHCI_QUIRK_INVERTED_WRITE_PROTECT, false),
> -    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
> -                     false),
> +    DEFINE_PROP_BIT("pending-insert-quirk", SDHCIState, quirks,
> +                    SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET, false),

Now I see why you need bit defines instead of mask value. Other comments 
about renaming and adjusting comment still apply. I'm not sure if adding 
and enum for bit numbers and keeping the defines for the QUIRK to avoid 
mistake of forgetting BIT at usage might be better. Otherwise this looks 
good.

Regards,
BALATON Zoltan

>     DEFINE_PROP_LINK("dma", SDHCIState,
>                      dma_mr, TYPE_MEMORY_REGION, MemoryRegion *),
> };
>

[PATCH v5 06/14] hw/sd/sdhci: Introduce SDHCIClass stub

[Philippe Mathieu-Daudé]

TYPE_SYSBUS_SDHCI is a bit odd because it uses an union
to work with both SysBus / PCI parent.

For this reason, we can not use the OBJECT_DECLARE_TYPE()
macro twice (on SYSBUS_SDHCI & PCI_SDHCI) and we must
keep a pair of lower DECLARE_INSTANCE_CHECKER) and
DECLARE_CLASS_CHECKERS() for PCI, in order to avoid:

  include/hw/sd/sdhci.h:165:1: error: redefinition of 'glib_autoptr_clear_SDHCIState'
  include/hw/sd/sdhci.h:165:1: error: redefinition of 'glib_autoptr_cleanup_SDHCIState'
  include/hw/sd/sdhci.h:165:1: error: redefinition of 'glib_autoptr_destroy_SDHCIState'
  include/hw/sd/sdhci.h:165:1: error: redefinition of 'glib_listautoptr_cleanup_SDHCIState'
  include/hw/sd/sdhci.h:165:1: error: redefinition of 'glib_slistautoptr_cleanup_SDHCIState'
  include/hw/sd/sdhci.h:165:1: error: redefinition of 'glib_queueautoptr_cleanup_SDHCIState'
    165 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, SYSBUS_SDHCI)
        | ^
  include/hw/sd/sdhci.h:158:1: note: previous definition is here
    158 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, PCI_SDHCI)
        | ^

Since we can not use SYSBUS_SDHCI_GET_CLASS() on PCI instances,
cache the class reference in the object state as 'sc'.

As this is not a normal use, introduce SDHCIClass in its own
commit.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
I plan to remove the union later.
---
 include/hw/sd/sdhci.h | 17 +++++++++++++----
 hw/sd/sdhci-pci.c     |  1 +
 hw/sd/sdhci.c         |  3 +++
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 2e6e719df7b..c42aeab6848 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -30,6 +30,8 @@
 #include "hw/sd/sd.h"
 #include "qom/object.h"
 
+typedef struct SDHCIClass SDHCIClass;
+
 /*
  * SD/MMC host controller state
  *
@@ -41,13 +43,12 @@
  *    card to be protected).
  */
 struct SDHCIState {
-    /*< private >*/
     union {
         PCIDevice pcidev;
         SysBusDevice busdev;
     };
 
-    /*< public >*/
+    SDHCIClass *sc;
     SDBus sdbus;
     MemoryRegion iomem;
     AddressSpace sysbus_dma_as;
@@ -110,6 +111,13 @@ struct SDHCIState {
 };
 typedef struct SDHCIState SDHCIState;
 
+struct SDHCIClass {
+    union {
+        PCIDeviceClass pci_parent_class;
+        SysBusDeviceClass sbd_parent_class;
+    };
+};
+
 /*
  * NOTE: This definition is taken out of Linux kernel and so the
  * original bit number is preserved
@@ -126,10 +134,11 @@ enum {
 #define TYPE_PCI_SDHCI "sdhci-pci"
 DECLARE_INSTANCE_CHECKER(SDHCIState, PCI_SDHCI,
                          TYPE_PCI_SDHCI)
+DECLARE_CLASS_CHECKERS(SDHCIClass, PCI_SDHCI,
+                       TYPE_PCI_SDHCI)
 
 #define TYPE_SYSBUS_SDHCI "generic-sdhci"
-DECLARE_INSTANCE_CHECKER(SDHCIState, SYSBUS_SDHCI,
-                         TYPE_SYSBUS_SDHCI)
+OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, SYSBUS_SDHCI)
 
 #define TYPE_IMX_USDHC "imx-usdhc"
 
diff --git a/hw/sd/sdhci-pci.c b/hw/sd/sdhci-pci.c
index 5268c0dee50..fe62582de90 100644
--- a/hw/sd/sdhci-pci.c
+++ b/hw/sd/sdhci-pci.c
@@ -73,6 +73,7 @@ static const TypeInfo sdhci_pci_types[] = {
         .parent         = TYPE_PCI_DEVICE,
         .instance_size  = sizeof(SDHCIState),
         .class_init     = sdhci_pci_class_init,
+        .class_size     = sizeof(SDHCIClass),
         .interfaces     = (InterfaceInfo[]) {
             { INTERFACE_CONVENTIONAL_PCI_DEVICE },
             { },
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index d1b1b187874..176bee6b8f5 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1419,6 +1419,8 @@ static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
 
 void sdhci_initfn(SDHCIState *s)
 {
+    s->sc = (SDHCIClass *)object_get_class(OBJECT(s)); /* Cache QOM parent */
+
     qbus_init(&s->sdbus, sizeof(s->sdbus), TYPE_SDHCI_BUS, DEVICE(s), "sd-bus");
 
     s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
@@ -1960,6 +1962,7 @@ static const TypeInfo sdhci_types[] = {
         .instance_size = sizeof(SDHCIState),
         .instance_init = sdhci_sysbus_init,
         .instance_finalize = sdhci_sysbus_finalize,
+        .class_size = sizeof(SDHCIClass),
         .class_init = sdhci_sysbus_class_init,
     },
     {
-- 
2.47.1

[PATCH v5 07/14] hw/sd/sdhci: Make quirks a class property

[Philippe Mathieu-Daudé]

Allow to enforce implementations quirks by the class.

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 |  2 ++
 hw/sd/sdhci.c         | 12 +++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index c42aeab6848..ee1e7ef4b10 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -116,6 +116,8 @@ struct SDHCIClass {
         PCIDeviceClass pci_parent_class;
         SysBusDeviceClass sbd_parent_class;
     };
+
+    uint32_t quirks;
 };
 
 /*
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 176bee6b8f5..570d825d130 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1427,6 +1427,7 @@ void sdhci_initfn(SDHCIState *s)
                                    sdhci_raise_insertion_irq, s);
     s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                      sdhci_data_transfer, s);
+    s->quirks = s->sc->quirks;
 
     s->io_ops = &sdhci_mmio_le_ops;
 }
@@ -1888,7 +1889,15 @@ static void imx_usdhc_init(Object *obj)
     SDHCIState *s = SYSBUS_SDHCI(obj);
 
     s->io_ops = &usdhc_mmio_ops;
-    s->quirks = BIT(SDHCI_QUIRK_NO_BUSY_IRQ);
+}
+
+static void imx_usdhc_class_init(ObjectClass *oc, void *data)
+{
+    SDHCIClass *sc = SYSBUS_SDHCI_CLASS(oc);
+
+    sc->quirks = BIT(SDHCI_QUIRK_NO_BUSY_IRQ);
+
+    sdhci_common_class_init(oc, data);
 }
 
 /* --- qdev Samsung s3c --- */
@@ -1969,6 +1978,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

[PATCH v5 08/14] hw/sd/sdhci: Make I/O region size a class property

[Philippe Mathieu-Daudé]

Be ready to have SDHC implementations to cover
a wider I/O address range.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 include/hw/sd/sdhci.h |  1 +
 hw/sd/sdhci.c         | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index ee1e7ef4b10..dfa0c214036 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -118,6 +118,7 @@ struct SDHCIClass {
     };
 
     uint32_t quirks;
+    uint64_t iomem_size;
 };
 
 /*
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 570d825d130..3467385490d 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1444,6 +1444,8 @@ void sdhci_uninitfn(SDHCIState *s)
 void sdhci_common_realize(SDHCIState *s, Error **errp)
 {
     ERRP_GUARD();
+    SDHCIClass *sc = s->sc;
+    const char *class_name = object_get_typename(OBJECT(s));
 
     switch (s->endianness) {
     case DEVICE_LITTLE_ENDIAN:
@@ -1469,8 +1471,9 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
     s->buf_maxsz = sdhci_get_fifolen(s);
     s->fifo_buffer = g_malloc0(s->buf_maxsz);
 
-    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
-                          SDHC_REGISTERS_MAP_SIZE);
+    assert(sc->iomem_size >= SDHC_REGISTERS_MAP_SIZE);
+    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, class_name,
+                          sc->iomem_size);
 }
 
 void sdhci_common_unrealize(SDHCIState *s)
@@ -1548,10 +1551,13 @@ const VMStateDescription sdhci_vmstate = {
 void sdhci_common_class_init(ObjectClass *klass, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    SDHCIClass *sc = (SDHCIClass *)klass; /* No QOM cast check due to union */
 
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->vmsd = &sdhci_vmstate;
     device_class_set_legacy_reset(dc, sdhci_poweron_reset);
+
+    sc->iomem_size = SDHC_REGISTERS_MAP_SIZE;
 }
 
 /* --- qdev SysBus --- */
-- 
2.47.1

[PATCH v5 09/14] hw/sd/sdhci: Allow SDHCI classes to register their own MemoryRegionOps

[Philippe Mathieu-Daudé]

Add MemoryRegionOps as a class property. For now it is only
used by TYPE_IMX_USDHC.
Otherwise the default remains in little endian.

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

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index dfa0c214036..108bc1993c6 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -117,6 +117,7 @@ struct SDHCIClass {
         SysBusDeviceClass sbd_parent_class;
     };
 
+    const MemoryRegionOps *io_ops;
     uint32_t quirks;
     uint64_t iomem_size;
 };
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 3467385490d..6868bf68285 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1428,8 +1428,6 @@ void sdhci_initfn(SDHCIState *s)
     s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                      sdhci_data_transfer, s);
     s->quirks = s->sc->quirks;
-
-    s->io_ops = &sdhci_mmio_le_ops;
 }
 
 void sdhci_uninitfn(SDHCIState *s)
@@ -1447,6 +1445,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
     SDHCIClass *sc = s->sc;
     const char *class_name = object_get_typename(OBJECT(s));
 
+    s->io_ops = sc->io_ops ?: &sdhci_mmio_le_ops;
     switch (s->endianness) {
     case DEVICE_LITTLE_ENDIAN:
         /* s->io_ops is little endian by default */
@@ -1890,17 +1889,11 @@ static const MemoryRegionOps usdhc_mmio_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static void imx_usdhc_init(Object *obj)
-{
-    SDHCIState *s = SYSBUS_SDHCI(obj);
-
-    s->io_ops = &usdhc_mmio_ops;
-}
-
 static void imx_usdhc_class_init(ObjectClass *oc, void *data)
 {
     SDHCIClass *sc = SYSBUS_SDHCI_CLASS(oc);
 
+    sc->io_ops = &usdhc_mmio_ops;
     sc->quirks = BIT(SDHCI_QUIRK_NO_BUSY_IRQ);
 
     sdhci_common_class_init(oc, data);
@@ -1957,11 +1950,13 @@ static const MemoryRegionOps sdhci_s3c_mmio_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static void sdhci_s3c_init(Object *obj)
+static void sdhci_s3c_class_init(ObjectClass *oc, void *data)
 {
-    SDHCIState *s = SYSBUS_SDHCI(obj);
+    SDHCIClass *sc = SYSBUS_SDHCI_CLASS(oc);
 
-    s->io_ops = &sdhci_s3c_mmio_ops;
+    sc->io_ops = &sdhci_s3c_mmio_ops;
+
+    sdhci_common_class_init(oc, data);
 }
 
 static const TypeInfo sdhci_types[] = {
@@ -1983,13 +1978,12 @@ 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,
         .parent = TYPE_SYSBUS_SDHCI,
-        .instance_init = sdhci_s3c_init,
+        .class_init = sdhci_s3c_class_init,
     },
 };
 
-- 
2.47.1

Re: [PATCH v5 09/14] hw/sd/sdhci: Allow SDHCI classes to register their own MemoryRegionOps

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3157 bytes --]

On Mon, 10 Mar 2025, Philippe Mathieu-Daudé wrote:
> Add MemoryRegionOps as a class property. For now it is only
> used by TYPE_IMX_USDHC.
> Otherwise the default remains in little endian.

I still don't see why you need to do this via the class and not just set 
it init or realize as it's done already but it should work so

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> include/hw/sd/sdhci.h |  1 +
> hw/sd/sdhci.c         | 22 ++++++++--------------
> 2 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index dfa0c214036..108bc1993c6 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -117,6 +117,7 @@ struct SDHCIClass {
>         SysBusDeviceClass sbd_parent_class;
>     };
>
> +    const MemoryRegionOps *io_ops;
>     uint32_t quirks;
>     uint64_t iomem_size;
> };
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 3467385490d..6868bf68285 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1428,8 +1428,6 @@ void sdhci_initfn(SDHCIState *s)
>     s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>                                      sdhci_data_transfer, s);
>     s->quirks = s->sc->quirks;
> -
> -    s->io_ops = &sdhci_mmio_le_ops;
> }
>
> void sdhci_uninitfn(SDHCIState *s)
> @@ -1447,6 +1445,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>     SDHCIClass *sc = s->sc;
>     const char *class_name = object_get_typename(OBJECT(s));
>
> +    s->io_ops = sc->io_ops ?: &sdhci_mmio_le_ops;
>     switch (s->endianness) {
>     case DEVICE_LITTLE_ENDIAN:
>         /* s->io_ops is little endian by default */
> @@ -1890,17 +1889,11 @@ static const MemoryRegionOps usdhc_mmio_ops = {
>     .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> -static void imx_usdhc_init(Object *obj)
> -{
> -    SDHCIState *s = SYSBUS_SDHCI(obj);
> -
> -    s->io_ops = &usdhc_mmio_ops;
> -}
> -
> static void imx_usdhc_class_init(ObjectClass *oc, void *data)
> {
>     SDHCIClass *sc = SYSBUS_SDHCI_CLASS(oc);
>
> +    sc->io_ops = &usdhc_mmio_ops;
>     sc->quirks = BIT(SDHCI_QUIRK_NO_BUSY_IRQ);
>
>     sdhci_common_class_init(oc, data);
> @@ -1957,11 +1950,13 @@ static const MemoryRegionOps sdhci_s3c_mmio_ops = {
>     .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> -static void sdhci_s3c_init(Object *obj)
> +static void sdhci_s3c_class_init(ObjectClass *oc, void *data)
> {
> -    SDHCIState *s = SYSBUS_SDHCI(obj);
> +    SDHCIClass *sc = SYSBUS_SDHCI_CLASS(oc);
>
> -    s->io_ops = &sdhci_s3c_mmio_ops;
> +    sc->io_ops = &sdhci_s3c_mmio_ops;
> +
> +    sdhci_common_class_init(oc, data);
> }
>
> static const TypeInfo sdhci_types[] = {
> @@ -1983,13 +1978,12 @@ 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,
>         .parent = TYPE_SYSBUS_SDHCI,
> -        .instance_init = sdhci_s3c_init,
> +        .class_init = sdhci_s3c_class_init,
>     },
> };
>
>

[PATCH v5 10/14] hw/sd/sdhci: Allow SDHCI classes to register their own read-only regs

[Philippe Mathieu-Daudé]

Some registers are read-only.

Since we allow instances to clear/set extra bits of capareg,
log when read-only bits normally set by hardware are cleared
at board level.

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

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 108bc1993c6..eb21b64f932 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -120,6 +120,13 @@ struct SDHCIClass {
     const MemoryRegionOps *io_ops;
     uint32_t quirks;
     uint64_t iomem_size;
+
+    /* Read-only registers */
+    struct {
+        uint64_t capareg;
+        uint64_t maxcurr;
+        uint16_t version;
+    } ro;
 };
 
 /*
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 6868bf68285..eb6a0e0f939 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -73,6 +73,7 @@ static bool sdhci_check_capab_freq_range(SDHCIState *s, const char *desc,
 
 static void sdhci_check_capareg(SDHCIState *s, Error **errp)
 {
+    SDHCIClass *sc = s->sc;
     uint64_t msk = s->capareg;
     uint32_t val;
     bool y;
@@ -208,6 +209,11 @@ static void sdhci_check_capareg(SDHCIState *s, Error **errp)
         qemu_log_mask(LOG_UNIMP,
                       "SDHCI: unknown CAPAB mask: 0x%016" PRIx64 "\n", msk);
     }
+    msk = sc->ro.capareg & ~s->capareg;
+    if (msk) {
+        qemu_log_mask(LOG_UNIMP,
+                      "SDHCI: ignored CAPAB mask: 0x%016" PRIx64 "\n", msk);
+    }
 }
 
 static uint8_t sdhci_slotint(SDHCIState *s)
@@ -1407,7 +1413,9 @@ static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
         error_setg(errp, "Only Spec v2/v3 are supported");
         return;
     }
-    s->version = (SDHC_HCVER_VENDOR << 8) | (s->sd_spec_version - 1);
+    s->version = s->sc->ro.version
+                 ?: (SDHC_HCVER_VENDOR << 8) | (s->sd_spec_version - 1);
+    s->maxcurr = s->sc->ro.maxcurr;
 
     sdhci_check_capareg(s, errp);
     if (*errp) {
-- 
2.47.1

[PATCH v5 11/14] hw/sd/sdhci: Allow SDHCI classes to have different register reset values

[Philippe Mathieu-Daudé]

For the registers which are not zeroed at reset, allow the
different implementations to set particular reset values.

Remove the misleading values commented in sdhci-internal.h.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/sd/sdhci-internal.h | 24 ++++++++++++------------
 include/hw/sd/sdhci.h  | 20 ++++++++++++++++++++
 hw/sd/sdhci.c          | 14 ++++++++++++++
 3 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 9072b06bdde..d99a8493db2 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -70,7 +70,7 @@
 /* R/W Buffer Data Register 0x0 */
 #define SDHC_BDATA                     0x20
 
-/* R/ROC Present State Register 0x000A0000 */
+/* R/ROC Present State Register */
 #define SDHC_PRNSTS                    0x24
 #define SDHC_CMD_INHIBIT               0x00000001
 #define SDHC_DATA_INHIBIT              0x00000002
@@ -88,7 +88,7 @@ FIELD(SDHC_PRNSTS, CMD_LVL,            24, 1);
 #define TRANSFERRING_DATA(x)           \
     ((x) & (SDHC_DOING_READ | SDHC_DOING_WRITE))
 
-/* R/W Host control Register 0x0 */
+/* R/W Host control Register */
 #define SDHC_HOSTCTL                   0x28
 #define SDHC_CTRL_LED                  0x01
 #define SDHC_CTRL_DATATRANSFERWIDTH    0x02 /* SD mode only */
@@ -104,17 +104,17 @@ FIELD(SDHC_PRNSTS, CMD_LVL,            24, 1);
 #define SDHC_CTRL_CDTEST_INS           0x40
 #define SDHC_CTRL_CDTEST_EN            0x80
 
-/* R/W Power Control Register 0x0 */
+/* R/W Power Control Register */
 #define SDHC_PWRCON                    0x29
 #define SDHC_POWER_ON                  (1 << 0)
 FIELD(SDHC_PWRCON, BUS_VOLTAGE,        1, 3);
 
-/* R/W Block Gap Control Register 0x0 */
+/* R/W Block Gap Control Register */
 #define SDHC_BLKGAP                    0x2A
 #define SDHC_STOP_AT_GAP_REQ           0x01
 #define SDHC_CONTINUE_REQ              0x02
 
-/* R/W WakeUp Control Register 0x0 */
+/* R/W WakeUp Control Register */
 #define SDHC_WAKCON                    0x2B
 #define SDHC_WKUP_ON_INS               (1 << 1)
 #define SDHC_WKUP_ON_RMV               (1 << 2)
@@ -128,17 +128,17 @@ FIELD(SDHC_PWRCON, BUS_VOLTAGE,        1, 3);
 #define SDHC_CLOCK_IS_ON(x)            \
     (((x) & SDHC_CLOCK_CHK_MASK) == SDHC_CLOCK_CHK_MASK)
 
-/* R/W Timeout Control Register 0x0 */
+/* R/W Timeout Control Register */
 #define SDHC_TIMEOUTCON                0x2E
 FIELD(SDHC_TIMEOUTCON, COUNTER,        0, 4);
 
-/* R/W Software Reset Register 0x0 */
+/* R/W Software Reset Register */
 #define SDHC_SWRST                     0x2F
 #define SDHC_RESET_ALL                 0x01
 #define SDHC_RESET_CMD                 0x02
 #define SDHC_RESET_DATA                0x04
 
-/* ROC/RW1C Normal Interrupt Status Register 0x0 */
+/* ROC/RW1C Normal Interrupt Status Register */
 #define SDHC_NORINTSTS                 0x30
 #define SDHC_NIS_ERR                   0x8000
 #define SDHC_NIS_CMDCMP                0x0001
@@ -151,7 +151,7 @@ FIELD(SDHC_TIMEOUTCON, COUNTER,        0, 4);
 #define SDHC_NIS_REMOVE                0x0080
 #define SDHC_NIS_CARDINT               0x0100
 
-/* ROC/RW1C Error Interrupt Status Register 0x0 */
+/* ROC/RW1C Error Interrupt Status Register */
 #define SDHC_ERRINTSTS                 0x32
 #define SDHC_EIS_CMDTIMEOUT            0x0001
 #define SDHC_EIS_BLKGAP                0x0004
@@ -159,7 +159,7 @@ FIELD(SDHC_TIMEOUTCON, COUNTER,        0, 4);
 #define SDHC_EIS_CMD12ERR              0x0100
 #define SDHC_EIS_ADMAERR               0x0200
 
-/* R/W Normal Interrupt Status Enable Register 0x0 */
+/* R/W Normal Interrupt Status Enable Register */
 #define SDHC_NORINTSTSEN               0x34
 #define SDHC_NISEN_CMDCMP              0x0001
 #define SDHC_NISEN_TRSCMP              0x0002
@@ -170,7 +170,7 @@ FIELD(SDHC_TIMEOUTCON, COUNTER,        0, 4);
 #define SDHC_NISEN_REMOVE              0x0080
 #define SDHC_NISEN_CARDINT             0x0100
 
-/* R/W Error Interrupt Status Enable Register 0x0 */
+/* R/W Error Interrupt Status Enable Register */
 #define SDHC_ERRINTSTSEN               0x36
 #define SDHC_EISEN_CMDTIMEOUT          0x0001
 #define SDHC_EISEN_BLKGAP              0x0004
@@ -205,7 +205,7 @@ FIELD(SDHC_HOSTCTL2, VERSION4,        12, 1); /* since v4 */
 FIELD(SDHC_HOSTCTL2, ASYNC_INT,       14, 1);
 FIELD(SDHC_HOSTCTL2, PRESET_ENA,      15, 1);
 
-/* HWInit Capabilities Register 0x05E80080 */
+/* HWInit Capabilities Register */
 #define SDHC_CAPAB                     0x40
 FIELD(SDHC_CAPAB, TOCLKFREQ,           0, 6);
 FIELD(SDHC_CAPAB, TOUNIT,              7, 1);
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index eb21b64f932..b21adcab670 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -121,6 +121,26 @@ struct SDHCIClass {
     uint32_t quirks;
     uint64_t iomem_size;
 
+    /* Default reset values */
+    struct {
+        uint32_t sdmasysad;
+
+        uint16_t blksize;
+        uint16_t blkcnt;
+
+        uint32_t prnsts;
+
+        uint8_t  hostctl1;
+        uint8_t  pwrcon;
+        uint8_t  blkgap;
+        uint8_t  wakcon;
+
+        uint16_t clkcon;
+        uint8_t  timeoutcon;
+
+        uint16_t norintstsen;
+        uint16_t errintstsen;
+    } reset;
     /* Read-only registers */
     struct {
         uint64_t capareg;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index eb6a0e0f939..f731b1a141a 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -295,6 +295,7 @@ static void sdhci_set_readonly(DeviceState *dev, bool level)
 static void sdhci_reset(SDHCIState *s)
 {
     DeviceState *dev = DEVICE(s);
+    SDHCIClass *sc = s->sc;
 
     timer_del(s->insert_timer);
     timer_del(s->transfer_timer);
@@ -306,6 +307,19 @@ static void sdhci_reset(SDHCIState *s)
      */
     memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg - (uintptr_t)&s->sdmasysad);
 
+    s->sdmasysad = sc->reset.sdmasysad;
+    s->blksize = sc->reset.blksize;
+    s->blkcnt = sc->reset.blkcnt;
+    s->prnsts = sc->reset.prnsts;
+    s->hostctl1 = sc->reset.hostctl1;
+    s->pwrcon = sc->reset.pwrcon;
+    s->blkgap = sc->reset.blkgap;
+    s->wakcon = sc->reset.wakcon;
+    s->clkcon = sc->reset.clkcon;
+    s->timeoutcon = sc->reset.timeoutcon;
+    s->norintstsen = sc->reset.norintstsen;
+    s->errintstsen = sc->reset.errintstsen;
+
     /* Reset other state based on current card insertion/readonly status */
     sdhci_set_inserted(dev, sdbus_get_inserted(&s->sdbus));
     sdhci_set_readonly(dev, sdbus_get_readonly(&s->sdbus));
-- 
2.47.1

[PATCH v5 12/14] hw/sd/sdhci: Implement Freescale eSDHC as TYPE_FSL_ESDHC

[Philippe Mathieu-Daudé]

Per the MPC8569E reference manual, its SDHC I/O range is 4KiB
wide, mapped in big endian order, and it only accepts 32-bit
aligned access. Set the default register reset values.

Reported-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/sd/sdhci.h |  2 ++
 hw/sd/sdhci.c         | 44 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index b21adcab670..e8fced5eedc 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -171,6 +171,8 @@ DECLARE_CLASS_CHECKERS(SDHCIClass, PCI_SDHCI,
 #define TYPE_SYSBUS_SDHCI "generic-sdhci"
 OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, SYSBUS_SDHCI)
 
+#define TYPE_FSL_ESDHC "fsl-esdhc"
+
 #define TYPE_IMX_USDHC "imx-usdhc"
 
 #define TYPE_S3C_SDHCI "s3c-sdhci"
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index f731b1a141a..47e4bd1a610 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1667,7 +1667,44 @@ static void sdhci_bus_class_init(ObjectClass *klass, void *data)
     sbc->set_readonly = sdhci_set_readonly;
 }
 
-/* --- qdev i.MX eSDHC --- */
+/* --- Freescale eSDHC (MPC8569ERM Rev.2 from 06/2011) --- */
+
+static const MemoryRegionOps fsl_esdhc_mmio_ops = {
+    .read = sdhci_read,
+    .write = sdhci_write,
+    .valid = {
+        /*
+         * Per the reference manual (chapter 16):
+         *
+         * All eSDHC registers must be accessed as aligned 4-byte quantities.
+         * Accesses to the eSDHC registers that are less than 4-bytes are not
+         * supported.
+         */
+        .min_access_size = 4,
+        .unaligned = false
+    },
+    .endianness = DEVICE_BIG_ENDIAN,
+};
+
+static void fsl_esdhc_class_init(ObjectClass *oc, void *data)
+{
+    SDHCIClass *sc = SYSBUS_SDHCI_CLASS(oc);
+
+    sc->iomem_size = 0x1000;
+    sc->io_ops = &fsl_esdhc_mmio_ops;
+    sc->ro.capareg = 0x01e30000;
+    sc->reset.sdmasysad = 8;
+    sc->reset.blkcnt = 8;
+    sc->reset.prnsts = 0xff800000;
+    sc->reset.hostctl1 = 0x20; /* Endian mode (address-invariant) */
+    sc->reset.clkcon = 0x8000;
+    sc->reset.norintstsen = 0x013f;
+    sc->reset.errintstsen = 0x117f;
+
+    sdhci_common_class_init(oc, data);
+}
+
+/* --- qdev i.MX uSDHC --- */
 
 #define USDHC_MIX_CTRL                  0x48
 
@@ -1997,6 +2034,11 @@ static const TypeInfo sdhci_types[] = {
         .class_size = sizeof(SDHCIClass),
         .class_init = sdhci_sysbus_class_init,
     },
+    {
+        .name = TYPE_FSL_ESDHC,
+        .parent = TYPE_SYSBUS_SDHCI,
+        .class_init = fsl_esdhc_class_init,
+    },
     {
         .name = TYPE_IMX_USDHC,
         .parent = TYPE_SYSBUS_SDHCI,
-- 
2.47.1

[PATCH v5 13/14] hw/ppc/e500: Replace generic SDHCI by Freescale eSDHC

[Philippe Mathieu-Daudé]

Zoltan reported some U-Boot versions seem to want registers
to be initialized correctly before expecting interrupts.

Now than we have a proper Freescale eSDHC implementation,
use it.

Reported-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/e500.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index fe8b9f79621..2de7d94df9c 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -49,7 +49,6 @@
 #include "hw/i2c/i2c.h"
 #include "hw/irq.h"
 #include "hw/sd/sdhci.h"
-#include "hw/misc/unimp.h"
 
 #define EPAPR_MAGIC                (0x45504150)
 #define DTC_LOAD_PAD               0x1800000
@@ -1027,22 +1026,13 @@ void ppce500_init(MachineState *machine)
 
     /* eSDHC */
     if (pmc->has_esdhc) {
-        dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
-        qdev_prop_set_string(dev, "name", "esdhc");
-        qdev_prop_set_uint64(dev, "size", MPC85XX_ESDHC_REGS_SIZE);
-        s = SYS_BUS_DEVICE(dev);
-        sysbus_realize_and_unref(s, &error_fatal);
-        memory_region_add_subregion(ccsr_addr_space, MPC85XX_ESDHC_REGS_OFFSET,
-                                    sysbus_mmio_get_region(s, 0));
-
         /*
          * Compatible with:
          * - SD Host Controller Specification Version 2.0 Part A2
          * (See MPC8569E Reference Manual)
          */
-        dev = qdev_new(TYPE_SYSBUS_SDHCI);
+        dev = qdev_new(TYPE_FSL_ESDHC);
         qdev_prop_set_uint8(dev, "sd-spec-version", 2);
-        qdev_prop_set_uint8(dev, "endianness", DEVICE_BIG_ENDIAN);
         s = SYS_BUS_DEVICE(dev);
         sysbus_realize_and_unref(s, &error_fatal);
         sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, MPC85XX_ESDHC_IRQ));
-- 
2.47.1

[PATCH v5 14/14] hw/sd/sdhci: Remove unnecessary 'endianness' property

[Philippe Mathieu-Daudé]

The previous commit removed the single use of instance
setting the "endianness" property.

Since classes can register their io_ops with correct
endianness, no need to support different ones.

Remove the code related to SDHCIState::endianess field.

Remove the now unused SDHCIState::io_ops field, since we
directly use the class one.

Suggested-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sdhci-internal.h |  1 -
 include/hw/sd/sdhci.h  |  2 --
 hw/sd/sdhci.c          | 33 +++------------------------------
 3 files changed, 3 insertions(+), 33 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index d99a8493db2..e4da6c831d1 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -308,7 +308,6 @@ extern const VMStateDescription sdhci_vmstate;
 #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
 
 #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
-    DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), \
     DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
     DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
     \
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index e8fced5eedc..1016a5b5b77 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -54,7 +54,6 @@ struct SDHCIState {
     AddressSpace sysbus_dma_as;
     AddressSpace *dma_as;
     MemoryRegion *dma_mr;
-    const MemoryRegionOps *io_ops;
 
     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
     QEMUTimer *transfer_timer;
@@ -105,7 +104,6 @@ struct SDHCIState {
 
     /* Configurable properties */
     uint32_t quirks;
-    uint8_t endianness;
     uint8_t sd_spec_version;
     uint8_t uhs_mode;
 };
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 47e4bd1a610..cbb9f4ae8c0 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1391,17 +1391,6 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
 }
 
 static const MemoryRegionOps sdhci_mmio_le_ops = {
-    .read = sdhci_read,
-    .write = sdhci_write,
-    .valid = {
-        .min_access_size = 1,
-        .max_access_size = 4,
-        .unaligned = false
-    },
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
-static const MemoryRegionOps sdhci_mmio_be_ops = {
     .read = sdhci_read,
     .write = sdhci_write,
     .impl = {
@@ -1413,7 +1402,7 @@ static const MemoryRegionOps sdhci_mmio_be_ops = {
         .max_access_size = 4,
         .unaligned = false
     },
-    .endianness = DEVICE_BIG_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
@@ -1467,23 +1456,6 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
     SDHCIClass *sc = s->sc;
     const char *class_name = object_get_typename(OBJECT(s));
 
-    s->io_ops = sc->io_ops ?: &sdhci_mmio_le_ops;
-    switch (s->endianness) {
-    case DEVICE_LITTLE_ENDIAN:
-        /* s->io_ops is little endian by default */
-        break;
-    case DEVICE_BIG_ENDIAN:
-        if (s->io_ops != &sdhci_mmio_le_ops) {
-            error_setg(errp, "SD controller doesn't support big endianness");
-            return;
-        }
-        s->io_ops = &sdhci_mmio_be_ops;
-        break;
-    default:
-        error_setg(errp, "Incorrect endianness");
-        return;
-    }
-
     sdhci_init_readonly_registers(s, errp);
     if (*errp) {
         return;
@@ -1493,7 +1465,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
     s->fifo_buffer = g_malloc0(s->buf_maxsz);
 
     assert(sc->iomem_size >= SDHC_REGISTERS_MAP_SIZE);
-    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, class_name,
+    memory_region_init_io(&s->iomem, OBJECT(s), sc->io_ops, s, class_name,
                           sc->iomem_size);
 }
 
@@ -1578,6 +1550,7 @@ void sdhci_common_class_init(ObjectClass *klass, const void *data)
     dc->vmsd = &sdhci_vmstate;
     device_class_set_legacy_reset(dc, sdhci_poweron_reset);
 
+    sc->io_ops = &sdhci_mmio_le_ops;
     sc->iomem_size = SDHC_REGISTERS_MAP_SIZE;
 }
 
-- 
2.47.1

Re: [PATCH v5 14/14] hw/sd/sdhci: Remove unnecessary 'endianness' property

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4573 bytes --]

On Mon, 10 Mar 2025, Philippe Mathieu-Daudé wrote:
> The previous commit removed the single use of instance
> setting the "endianness" property.
>
> Since classes can register their io_ops with correct
> endianness, no need to support different ones.
>
> Remove the code related to SDHCIState::endianess field.
>
> Remove the now unused SDHCIState::io_ops field, since we
> directly use the class one.
>
> Suggested-by: Bernhard Beschow <shentey@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/sd/sdhci-internal.h |  1 -
> include/hw/sd/sdhci.h  |  2 --
> hw/sd/sdhci.c          | 33 +++------------------------------
> 3 files changed, 3 insertions(+), 33 deletions(-)
>
> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> index d99a8493db2..e4da6c831d1 100644
> --- a/hw/sd/sdhci-internal.h
> +++ b/hw/sd/sdhci-internal.h
> @@ -308,7 +308,6 @@ extern const VMStateDescription sdhci_vmstate;
> #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
>
> #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
> -    DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), \
>     DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
>     DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
>     \
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index e8fced5eedc..1016a5b5b77 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -54,7 +54,6 @@ struct SDHCIState {
>     AddressSpace sysbus_dma_as;
>     AddressSpace *dma_as;
>     MemoryRegion *dma_mr;
> -    const MemoryRegionOps *io_ops;
>
>     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
>     QEMUTimer *transfer_timer;
> @@ -105,7 +104,6 @@ struct SDHCIState {
>
>     /* Configurable properties */
>     uint32_t quirks;
> -    uint8_t endianness;
>     uint8_t sd_spec_version;
>     uint8_t uhs_mode;
> };
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 47e4bd1a610..cbb9f4ae8c0 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1391,17 +1391,6 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
> }
>
> static const MemoryRegionOps sdhci_mmio_le_ops = {
> -    .read = sdhci_read,
> -    .write = sdhci_write,
> -    .valid = {
> -        .min_access_size = 1,
> -        .max_access_size = 4,
> -        .unaligned = false
> -    },
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> -};
> -
> -static const MemoryRegionOps sdhci_mmio_be_ops = {
>     .read = sdhci_read,
>     .write = sdhci_write,
>     .impl = {
> @@ -1413,7 +1402,7 @@ static const MemoryRegionOps sdhci_mmio_be_ops = {
>         .max_access_size = 4,
>         .unaligned = false
>     },
> -    .endianness = DEVICE_BIG_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
> @@ -1467,23 +1456,6 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>     SDHCIClass *sc = s->sc;
>     const char *class_name = object_get_typename(OBJECT(s));
>
> -    s->io_ops = sc->io_ops ?: &sdhci_mmio_le_ops;
> -    switch (s->endianness) {
> -    case DEVICE_LITTLE_ENDIAN:
> -        /* s->io_ops is little endian by default */
> -        break;
> -    case DEVICE_BIG_ENDIAN:
> -        if (s->io_ops != &sdhci_mmio_le_ops) {
> -            error_setg(errp, "SD controller doesn't support big endianness");
> -            return;
> -        }
> -        s->io_ops = &sdhci_mmio_be_ops;
> -        break;
> -    default:
> -        error_setg(errp, "Incorrect endianness");
> -        return;
> -    }
> -
>     sdhci_init_readonly_registers(s, errp);
>     if (*errp) {
>         return;
> @@ -1493,7 +1465,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>     s->fifo_buffer = g_malloc0(s->buf_maxsz);
>
>     assert(sc->iomem_size >= SDHC_REGISTERS_MAP_SIZE);
> -    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, class_name,
> +    memory_region_init_io(&s->iomem, OBJECT(s), sc->io_ops, s, class_name,
>                           sc->iomem_size);
> }
>
> @@ -1578,6 +1550,7 @@ void sdhci_common_class_init(ObjectClass *klass, const void *data)
>     dc->vmsd = &sdhci_vmstate;
>     device_class_set_legacy_reset(dc, sdhci_poweron_reset);
>
> +    sc->io_ops = &sdhci_mmio_le_ops;

You call common_class_init in subclass class_inits last so this would 
overwrite what subclass has set, doesn't it? I think you either have to 
change order in subclass class_init methods or not set this here.

Regards,
BALATON Zoltan

>     sc->iomem_size = SDHC_REGISTERS_MAP_SIZE;
> }
>
>

Re: [PATCH v5 14/14] hw/sd/sdhci: Remove unnecessary 'endianness' property

[Philippe Mathieu-Daudé]

On 10/3/25 15:09, BALATON Zoltan wrote:
> On Mon, 10 Mar 2025, Philippe Mathieu-Daudé wrote:
>> The previous commit removed the single use of instance
>> setting the "endianness" property.
>>
>> Since classes can register their io_ops with correct
>> endianness, no need to support different ones.
>>
>> Remove the code related to SDHCIState::endianess field.
>>
>> Remove the now unused SDHCIState::io_ops field, since we
>> directly use the class one.
>>
>> Suggested-by: Bernhard Beschow <shentey@gmail.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/sd/sdhci-internal.h |  1 -
>> include/hw/sd/sdhci.h  |  2 --
>> hw/sd/sdhci.c          | 33 +++------------------------------
>> 3 files changed, 3 insertions(+), 33 deletions(-)
>>
>> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
>> index d99a8493db2..e4da6c831d1 100644
>> --- a/hw/sd/sdhci-internal.h
>> +++ b/hw/sd/sdhci-internal.h
>> @@ -308,7 +308,6 @@ extern const VMStateDescription sdhci_vmstate;
>> #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
>>
>> #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
>> -    DEFINE_PROP_UINT8("endianness", _state, endianness, 
>> DEVICE_LITTLE_ENDIAN), \
>>     DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
>>     DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
>>     \
>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>> index e8fced5eedc..1016a5b5b77 100644
>> --- a/include/hw/sd/sdhci.h
>> +++ b/include/hw/sd/sdhci.h
>> @@ -54,7 +54,6 @@ struct SDHCIState {
>>     AddressSpace sysbus_dma_as;
>>     AddressSpace *dma_as;
>>     MemoryRegion *dma_mr;
>> -    const MemoryRegionOps *io_ops;
>>
>>     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
>>     QEMUTimer *transfer_timer;
>> @@ -105,7 +104,6 @@ struct SDHCIState {
>>
>>     /* Configurable properties */
>>     uint32_t quirks;
>> -    uint8_t endianness;
>>     uint8_t sd_spec_version;
>>     uint8_t uhs_mode;
>> };
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 47e4bd1a610..cbb9f4ae8c0 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -1391,17 +1391,6 @@ sdhci_write(void *opaque, hwaddr offset, 
>> uint64_t val, unsigned size)
>> }
>>
>> static const MemoryRegionOps sdhci_mmio_le_ops = {
>> -    .read = sdhci_read,
>> -    .write = sdhci_write,
>> -    .valid = {
>> -        .min_access_size = 1,
>> -        .max_access_size = 4,
>> -        .unaligned = false
>> -    },
>> -    .endianness = DEVICE_LITTLE_ENDIAN,
>> -};
>> -
>> -static const MemoryRegionOps sdhci_mmio_be_ops = {
>>     .read = sdhci_read,
>>     .write = sdhci_write,
>>     .impl = {
>> @@ -1413,7 +1402,7 @@ static const MemoryRegionOps sdhci_mmio_be_ops = {
>>         .max_access_size = 4,
>>         .unaligned = false
>>     },
>> -    .endianness = DEVICE_BIG_ENDIAN,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> };
>>
>> static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
>> @@ -1467,23 +1456,6 @@ void sdhci_common_realize(SDHCIState *s, Error 
>> **errp)
>>     SDHCIClass *sc = s->sc;
>>     const char *class_name = object_get_typename(OBJECT(s));
>>
>> -    s->io_ops = sc->io_ops ?: &sdhci_mmio_le_ops;
>> -    switch (s->endianness) {
>> -    case DEVICE_LITTLE_ENDIAN:
>> -        /* s->io_ops is little endian by default */
>> -        break;
>> -    case DEVICE_BIG_ENDIAN:
>> -        if (s->io_ops != &sdhci_mmio_le_ops) {
>> -            error_setg(errp, "SD controller doesn't support big 
>> endianness");
>> -            return;
>> -        }
>> -        s->io_ops = &sdhci_mmio_be_ops;
>> -        break;
>> -    default:
>> -        error_setg(errp, "Incorrect endianness");
>> -        return;
>> -    }
>> -
>>     sdhci_init_readonly_registers(s, errp);
>>     if (*errp) {
>>         return;
>> @@ -1493,7 +1465,7 @@ void sdhci_common_realize(SDHCIState *s, Error 
>> **errp)
>>     s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>
>>     assert(sc->iomem_size >= SDHC_REGISTERS_MAP_SIZE);
>> -    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, 
>> class_name,
>> +    memory_region_init_io(&s->iomem, OBJECT(s), sc->io_ops, s, 
>> class_name,
>>                           sc->iomem_size);
>> }
>>
>> @@ -1578,6 +1550,7 @@ void sdhci_common_class_init(ObjectClass *klass, 
>> const void *data)
>>     dc->vmsd = &sdhci_vmstate;
>>     device_class_set_legacy_reset(dc, sdhci_poweron_reset);
>>
>> +    sc->io_ops = &sdhci_mmio_le_ops;
> 
> You call common_class_init in subclass class_inits last so this would 
> overwrite what subclass has set, doesn't it? I think you either have to 
> change order in subclass class_init methods or not set this here.

Oops... I'm surprised tests passed. Do we have coverage for sdhci on
e500 machines? Or are we only testing them via virtio PCI block storage?

Re: [PATCH v5 14/14] hw/sd/sdhci: Remove unnecessary 'endianness' property

[Guenter Roeck]

On 3/10/25 08:27, Philippe Mathieu-Daudé wrote:
> On 10/3/25 15:09, BALATON Zoltan wrote:
>> On Mon, 10 Mar 2025, Philippe Mathieu-Daudé wrote:
>>> The previous commit removed the single use of instance
>>> setting the "endianness" property.
>>>
>>> Since classes can register their io_ops with correct
>>> endianness, no need to support different ones.
>>>
>>> Remove the code related to SDHCIState::endianess field.
>>>
>>> Remove the now unused SDHCIState::io_ops field, since we
>>> directly use the class one.
>>>
>>> Suggested-by: Bernhard Beschow <shentey@gmail.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> hw/sd/sdhci-internal.h |  1 -
>>> include/hw/sd/sdhci.h  |  2 --
>>> hw/sd/sdhci.c          | 33 +++------------------------------
>>> 3 files changed, 3 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
>>> index d99a8493db2..e4da6c831d1 100644
>>> --- a/hw/sd/sdhci-internal.h
>>> +++ b/hw/sd/sdhci-internal.h
>>> @@ -308,7 +308,6 @@ extern const VMStateDescription sdhci_vmstate;
>>> #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
>>>
>>> #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
>>> -    DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), \
>>>     DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
>>>     DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
>>>     \
>>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>>> index e8fced5eedc..1016a5b5b77 100644
>>> --- a/include/hw/sd/sdhci.h
>>> +++ b/include/hw/sd/sdhci.h
>>> @@ -54,7 +54,6 @@ struct SDHCIState {
>>>     AddressSpace sysbus_dma_as;
>>>     AddressSpace *dma_as;
>>>     MemoryRegion *dma_mr;
>>> -    const MemoryRegionOps *io_ops;
>>>
>>>     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
>>>     QEMUTimer *transfer_timer;
>>> @@ -105,7 +104,6 @@ struct SDHCIState {
>>>
>>>     /* Configurable properties */
>>>     uint32_t quirks;
>>> -    uint8_t endianness;
>>>     uint8_t sd_spec_version;
>>>     uint8_t uhs_mode;
>>> };
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index 47e4bd1a610..cbb9f4ae8c0 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -1391,17 +1391,6 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>>> }
>>>
>>> static const MemoryRegionOps sdhci_mmio_le_ops = {
>>> -    .read = sdhci_read,
>>> -    .write = sdhci_write,
>>> -    .valid = {
>>> -        .min_access_size = 1,
>>> -        .max_access_size = 4,
>>> -        .unaligned = false
>>> -    },
>>> -    .endianness = DEVICE_LITTLE_ENDIAN,
>>> -};
>>> -
>>> -static const MemoryRegionOps sdhci_mmio_be_ops = {
>>>     .read = sdhci_read,
>>>     .write = sdhci_write,
>>>     .impl = {
>>> @@ -1413,7 +1402,7 @@ static const MemoryRegionOps sdhci_mmio_be_ops = {
>>>         .max_access_size = 4,
>>>         .unaligned = false
>>>     },
>>> -    .endianness = DEVICE_BIG_ENDIAN,
>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>> };
>>>
>>> static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
>>> @@ -1467,23 +1456,6 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>>>     SDHCIClass *sc = s->sc;
>>>     const char *class_name = object_get_typename(OBJECT(s));
>>>
>>> -    s->io_ops = sc->io_ops ?: &sdhci_mmio_le_ops;
>>> -    switch (s->endianness) {
>>> -    case DEVICE_LITTLE_ENDIAN:
>>> -        /* s->io_ops is little endian by default */
>>> -        break;
>>> -    case DEVICE_BIG_ENDIAN:
>>> -        if (s->io_ops != &sdhci_mmio_le_ops) {
>>> -            error_setg(errp, "SD controller doesn't support big endianness");
>>> -            return;
>>> -        }
>>> -        s->io_ops = &sdhci_mmio_be_ops;
>>> -        break;
>>> -    default:
>>> -        error_setg(errp, "Incorrect endianness");
>>> -        return;
>>> -    }
>>> -
>>>     sdhci_init_readonly_registers(s, errp);
>>>     if (*errp) {
>>>         return;
>>> @@ -1493,7 +1465,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>>>     s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>>
>>>     assert(sc->iomem_size >= SDHC_REGISTERS_MAP_SIZE);
>>> -    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, class_name,
>>> +    memory_region_init_io(&s->iomem, OBJECT(s), sc->io_ops, s, class_name,
>>>                           sc->iomem_size);
>>> }
>>>
>>> @@ -1578,6 +1550,7 @@ void sdhci_common_class_init(ObjectClass *klass, const void *data)
>>>     dc->vmsd = &sdhci_vmstate;
>>>     device_class_set_legacy_reset(dc, sdhci_poweron_reset);
>>>
>>> +    sc->io_ops = &sdhci_mmio_le_ops;
>>
>> You call common_class_init in subclass class_inits last so this would overwrite what subclass has set, doesn't it? I think you either have to change order in subclass class_init methods or not set this here.
> 
> Oops... I'm surprised tests passed. Do we have coverage for sdhci on
> e500 machines? Or are we only testing them via virtio PCI block storage?

Not sure if that is what you are asking, but I have been testing it with
sdhci-pci for a long time (not this series, though).

Guenter

Re: [PATCH v5 14/14] hw/sd/sdhci: Remove unnecessary 'endianness' property

[Philippe Mathieu-Daudé]

On 10/3/25 16:56, Guenter Roeck wrote:
> On 3/10/25 08:27, Philippe Mathieu-Daudé wrote:
>> On 10/3/25 15:09, BALATON Zoltan wrote:
>>> On Mon, 10 Mar 2025, Philippe Mathieu-Daudé wrote:
>>>> The previous commit removed the single use of instance
>>>> setting the "endianness" property.
>>>>
>>>> Since classes can register their io_ops with correct
>>>> endianness, no need to support different ones.
>>>>
>>>> Remove the code related to SDHCIState::endianess field.
>>>>
>>>> Remove the now unused SDHCIState::io_ops field, since we
>>>> directly use the class one.
>>>>
>>>> Suggested-by: Bernhard Beschow <shentey@gmail.com>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>> hw/sd/sdhci-internal.h |  1 -
>>>> include/hw/sd/sdhci.h  |  2 --
>>>> hw/sd/sdhci.c          | 33 +++------------------------------
>>>> 3 files changed, 3 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
>>>> index d99a8493db2..e4da6c831d1 100644
>>>> --- a/hw/sd/sdhci-internal.h
>>>> +++ b/hw/sd/sdhci-internal.h
>>>> @@ -308,7 +308,6 @@ extern const VMStateDescription sdhci_vmstate;
>>>> #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
>>>>
>>>> #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
>>>> -    DEFINE_PROP_UINT8("endianness", _state, endianness, 
>>>> DEVICE_LITTLE_ENDIAN), \
>>>>     DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
>>>>     DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
>>>>     \
>>>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>>>> index e8fced5eedc..1016a5b5b77 100644
>>>> --- a/include/hw/sd/sdhci.h
>>>> +++ b/include/hw/sd/sdhci.h
>>>> @@ -54,7 +54,6 @@ struct SDHCIState {
>>>>     AddressSpace sysbus_dma_as;
>>>>     AddressSpace *dma_as;
>>>>     MemoryRegion *dma_mr;
>>>> -    const MemoryRegionOps *io_ops;
>>>>
>>>>     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
>>>>     QEMUTimer *transfer_timer;
>>>> @@ -105,7 +104,6 @@ struct SDHCIState {
>>>>
>>>>     /* Configurable properties */
>>>>     uint32_t quirks;
>>>> -    uint8_t endianness;
>>>>     uint8_t sd_spec_version;
>>>>     uint8_t uhs_mode;
>>>> };
>>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>>> index 47e4bd1a610..cbb9f4ae8c0 100644
>>>> --- a/hw/sd/sdhci.c
>>>> +++ b/hw/sd/sdhci.c
>>>> @@ -1391,17 +1391,6 @@ sdhci_write(void *opaque, hwaddr offset, 
>>>> uint64_t val, unsigned size)
>>>> }
>>>>
>>>> static const MemoryRegionOps sdhci_mmio_le_ops = {
>>>> -    .read = sdhci_read,
>>>> -    .write = sdhci_write,
>>>> -    .valid = {
>>>> -        .min_access_size = 1,
>>>> -        .max_access_size = 4,
>>>> -        .unaligned = false
>>>> -    },
>>>> -    .endianness = DEVICE_LITTLE_ENDIAN,
>>>> -};
>>>> -
>>>> -static const MemoryRegionOps sdhci_mmio_be_ops = {
>>>>     .read = sdhci_read,
>>>>     .write = sdhci_write,
>>>>     .impl = {
>>>> @@ -1413,7 +1402,7 @@ static const MemoryRegionOps sdhci_mmio_be_ops 
>>>> = {
>>>>         .max_access_size = 4,
>>>>         .unaligned = false
>>>>     },
>>>> -    .endianness = DEVICE_BIG_ENDIAN,
>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>> };
>>>>
>>>> static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
>>>> @@ -1467,23 +1456,6 @@ void sdhci_common_realize(SDHCIState *s, 
>>>> Error **errp)
>>>>     SDHCIClass *sc = s->sc;
>>>>     const char *class_name = object_get_typename(OBJECT(s));
>>>>
>>>> -    s->io_ops = sc->io_ops ?: &sdhci_mmio_le_ops;
>>>> -    switch (s->endianness) {
>>>> -    case DEVICE_LITTLE_ENDIAN:
>>>> -        /* s->io_ops is little endian by default */
>>>> -        break;
>>>> -    case DEVICE_BIG_ENDIAN:
>>>> -        if (s->io_ops != &sdhci_mmio_le_ops) {
>>>> -            error_setg(errp, "SD controller doesn't support big 
>>>> endianness");
>>>> -            return;
>>>> -        }
>>>> -        s->io_ops = &sdhci_mmio_be_ops;
>>>> -        break;
>>>> -    default:
>>>> -        error_setg(errp, "Incorrect endianness");
>>>> -        return;
>>>> -    }
>>>> -
>>>>     sdhci_init_readonly_registers(s, errp);
>>>>     if (*errp) {
>>>>         return;
>>>> @@ -1493,7 +1465,7 @@ void sdhci_common_realize(SDHCIState *s, Error 
>>>> **errp)
>>>>     s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>>>
>>>>     assert(sc->iomem_size >= SDHC_REGISTERS_MAP_SIZE);
>>>> -    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, 
>>>> class_name,
>>>> +    memory_region_init_io(&s->iomem, OBJECT(s), sc->io_ops, s, 
>>>> class_name,
>>>>                           sc->iomem_size);
>>>> }
>>>>
>>>> @@ -1578,6 +1550,7 @@ void sdhci_common_class_init(ObjectClass 
>>>> *klass, const void *data)
>>>>     dc->vmsd = &sdhci_vmstate;
>>>>     device_class_set_legacy_reset(dc, sdhci_poweron_reset);
>>>>
>>>> +    sc->io_ops = &sdhci_mmio_le_ops;
>>>
>>> You call common_class_init in subclass class_inits last so this would 
>>> overwrite what subclass has set, doesn't it? I think you either have 
>>> to change order in subclass class_init methods or not set this here.
>>
>> Oops... I'm surprised tests passed. Do we have coverage for sdhci on
>> e500 machines? Or are we only testing them via virtio PCI block storage?
> 
> Not sure if that is what you are asking, but I have been testing it with
> sdhci-pci for a long time (not this series, though).

I'm referring to the Freescale eSDHC controller of PPC e500 machines
(see previous patch).

Re: [PATCH v5 14/14] hw/sd/sdhci: Remove unnecessary 'endianness' property

[Bernhard Beschow]

Am 10. März 2025 17:31:57 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 10/3/25 16:56, Guenter Roeck wrote:
>> On 3/10/25 08:27, Philippe Mathieu-Daudé wrote:
>>> On 10/3/25 15:09, BALATON Zoltan wrote:
>>>> On Mon, 10 Mar 2025, Philippe Mathieu-Daudé wrote:
>>>>> The previous commit removed the single use of instance
>>>>> setting the "endianness" property.
>>>>> 
>>>>> Since classes can register their io_ops with correct
>>>>> endianness, no need to support different ones.
>>>>> 
>>>>> Remove the code related to SDHCIState::endianess field.
>>>>> 
>>>>> Remove the now unused SDHCIState::io_ops field, since we
>>>>> directly use the class one.
>>>>> 
>>>>> Suggested-by: Bernhard Beschow <shentey@gmail.com>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>> hw/sd/sdhci-internal.h |  1 -
>>>>> include/hw/sd/sdhci.h  |  2 --
>>>>> hw/sd/sdhci.c          | 33 +++------------------------------
>>>>> 3 files changed, 3 insertions(+), 33 deletions(-)
>>>>> 
>>>>> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
>>>>> index d99a8493db2..e4da6c831d1 100644
>>>>> --- a/hw/sd/sdhci-internal.h
>>>>> +++ b/hw/sd/sdhci-internal.h
>>>>> @@ -308,7 +308,6 @@ extern const VMStateDescription sdhci_vmstate;
>>>>> #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
>>>>> 
>>>>> #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
>>>>> -    DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), \
>>>>>     DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
>>>>>     DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
>>>>>     \
>>>>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>>>>> index e8fced5eedc..1016a5b5b77 100644
>>>>> --- a/include/hw/sd/sdhci.h
>>>>> +++ b/include/hw/sd/sdhci.h
>>>>> @@ -54,7 +54,6 @@ struct SDHCIState {
>>>>>     AddressSpace sysbus_dma_as;
>>>>>     AddressSpace *dma_as;
>>>>>     MemoryRegion *dma_mr;
>>>>> -    const MemoryRegionOps *io_ops;
>>>>> 
>>>>>     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
>>>>>     QEMUTimer *transfer_timer;
>>>>> @@ -105,7 +104,6 @@ struct SDHCIState {
>>>>> 
>>>>>     /* Configurable properties */
>>>>>     uint32_t quirks;
>>>>> -    uint8_t endianness;
>>>>>     uint8_t sd_spec_version;
>>>>>     uint8_t uhs_mode;
>>>>> };
>>>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>>>> index 47e4bd1a610..cbb9f4ae8c0 100644
>>>>> --- a/hw/sd/sdhci.c
>>>>> +++ b/hw/sd/sdhci.c
>>>>> @@ -1391,17 +1391,6 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>>>>> }
>>>>> 
>>>>> static const MemoryRegionOps sdhci_mmio_le_ops = {
>>>>> -    .read = sdhci_read,
>>>>> -    .write = sdhci_write,
>>>>> -    .valid = {
>>>>> -        .min_access_size = 1,
>>>>> -        .max_access_size = 4,
>>>>> -        .unaligned = false
>>>>> -    },
>>>>> -    .endianness = DEVICE_LITTLE_ENDIAN,
>>>>> -};
>>>>> -
>>>>> -static const MemoryRegionOps sdhci_mmio_be_ops = {
>>>>>     .read = sdhci_read,
>>>>>     .write = sdhci_write,
>>>>>     .impl = {
>>>>> @@ -1413,7 +1402,7 @@ static const MemoryRegionOps sdhci_mmio_be_ops = {
>>>>>         .max_access_size = 4,
>>>>>         .unaligned = false
>>>>>     },
>>>>> -    .endianness = DEVICE_BIG_ENDIAN,
>>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>>> };
>>>>> 
>>>>> static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
>>>>> @@ -1467,23 +1456,6 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>>>>>     SDHCIClass *sc = s->sc;
>>>>>     const char *class_name = object_get_typename(OBJECT(s));
>>>>> 
>>>>> -    s->io_ops = sc->io_ops ?: &sdhci_mmio_le_ops;
>>>>> -    switch (s->endianness) {
>>>>> -    case DEVICE_LITTLE_ENDIAN:
>>>>> -        /* s->io_ops is little endian by default */
>>>>> -        break;
>>>>> -    case DEVICE_BIG_ENDIAN:
>>>>> -        if (s->io_ops != &sdhci_mmio_le_ops) {
>>>>> -            error_setg(errp, "SD controller doesn't support big endianness");
>>>>> -            return;
>>>>> -        }
>>>>> -        s->io_ops = &sdhci_mmio_be_ops;
>>>>> -        break;
>>>>> -    default:
>>>>> -        error_setg(errp, "Incorrect endianness");
>>>>> -        return;
>>>>> -    }
>>>>> -
>>>>>     sdhci_init_readonly_registers(s, errp);
>>>>>     if (*errp) {
>>>>>         return;
>>>>> @@ -1493,7 +1465,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>>>>>     s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>>>> 
>>>>>     assert(sc->iomem_size >= SDHC_REGISTERS_MAP_SIZE);
>>>>> -    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, class_name,
>>>>> +    memory_region_init_io(&s->iomem, OBJECT(s), sc->io_ops, s, class_name,
>>>>>                           sc->iomem_size);
>>>>> }
>>>>> 
>>>>> @@ -1578,6 +1550,7 @@ void sdhci_common_class_init(ObjectClass *klass, const void *data)
>>>>>     dc->vmsd = &sdhci_vmstate;
>>>>>     device_class_set_legacy_reset(dc, sdhci_poweron_reset);
>>>>> 
>>>>> +    sc->io_ops = &sdhci_mmio_le_ops;
>>>> 
>>>> You call common_class_init in subclass class_inits last so this would overwrite what subclass has set, doesn't it? I think you either have to change order in subclass class_init methods or not set this here.
>>> 
>>> Oops... I'm surprised tests passed. Do we have coverage for sdhci on
>>> e500 machines? Or are we only testing them via virtio PCI block storage?
>> 
>> Not sure if that is what you are asking, but I have been testing it with
>> sdhci-pci for a long time (not this series, though).
>
>I'm referring to the Freescale eSDHC controller of PPC e500 machines
>(see previous patch).

I think testing SDHCI is generally difficult since the images need to be resized to a power of two. Any idea how to do this with the new functional tests?

Re: [PATCH v5 14/14] hw/sd/sdhci: Remove unnecessary 'endianness' property

[Cédric Le Goater]

On 3/10/25 18:38, Bernhard Beschow wrote:
> 
> 
> Am 10. März 2025 17:31:57 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> On 10/3/25 16:56, Guenter Roeck wrote:
>>> On 3/10/25 08:27, Philippe Mathieu-Daudé wrote:
>>>> On 10/3/25 15:09, BALATON Zoltan wrote:
>>>>> On Mon, 10 Mar 2025, Philippe Mathieu-Daudé wrote:
>>>>>> The previous commit removed the single use of instance
>>>>>> setting the "endianness" property.
>>>>>>
>>>>>> Since classes can register their io_ops with correct
>>>>>> endianness, no need to support different ones.
>>>>>>
>>>>>> Remove the code related to SDHCIState::endianess field.
>>>>>>
>>>>>> Remove the now unused SDHCIState::io_ops field, since we
>>>>>> directly use the class one.
>>>>>>
>>>>>> Suggested-by: Bernhard Beschow <shentey@gmail.com>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> ---
>>>>>> hw/sd/sdhci-internal.h |  1 -
>>>>>> include/hw/sd/sdhci.h  |  2 --
>>>>>> hw/sd/sdhci.c          | 33 +++------------------------------
>>>>>> 3 files changed, 3 insertions(+), 33 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
>>>>>> index d99a8493db2..e4da6c831d1 100644
>>>>>> --- a/hw/sd/sdhci-internal.h
>>>>>> +++ b/hw/sd/sdhci-internal.h
>>>>>> @@ -308,7 +308,6 @@ extern const VMStateDescription sdhci_vmstate;
>>>>>> #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
>>>>>>
>>>>>> #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
>>>>>> -    DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), \
>>>>>>      DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
>>>>>>      DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
>>>>>>      \
>>>>>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>>>>>> index e8fced5eedc..1016a5b5b77 100644
>>>>>> --- a/include/hw/sd/sdhci.h
>>>>>> +++ b/include/hw/sd/sdhci.h
>>>>>> @@ -54,7 +54,6 @@ struct SDHCIState {
>>>>>>      AddressSpace sysbus_dma_as;
>>>>>>      AddressSpace *dma_as;
>>>>>>      MemoryRegion *dma_mr;
>>>>>> -    const MemoryRegionOps *io_ops;
>>>>>>
>>>>>>      QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
>>>>>>      QEMUTimer *transfer_timer;
>>>>>> @@ -105,7 +104,6 @@ struct SDHCIState {
>>>>>>
>>>>>>      /* Configurable properties */
>>>>>>      uint32_t quirks;
>>>>>> -    uint8_t endianness;
>>>>>>      uint8_t sd_spec_version;
>>>>>>      uint8_t uhs_mode;
>>>>>> };
>>>>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>>>>> index 47e4bd1a610..cbb9f4ae8c0 100644
>>>>>> --- a/hw/sd/sdhci.c
>>>>>> +++ b/hw/sd/sdhci.c
>>>>>> @@ -1391,17 +1391,6 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>>>>>> }
>>>>>>
>>>>>> static const MemoryRegionOps sdhci_mmio_le_ops = {
>>>>>> -    .read = sdhci_read,
>>>>>> -    .write = sdhci_write,
>>>>>> -    .valid = {
>>>>>> -        .min_access_size = 1,
>>>>>> -        .max_access_size = 4,
>>>>>> -        .unaligned = false
>>>>>> -    },
>>>>>> -    .endianness = DEVICE_LITTLE_ENDIAN,
>>>>>> -};
>>>>>> -
>>>>>> -static const MemoryRegionOps sdhci_mmio_be_ops = {
>>>>>>      .read = sdhci_read,
>>>>>>      .write = sdhci_write,
>>>>>>      .impl = {
>>>>>> @@ -1413,7 +1402,7 @@ static const MemoryRegionOps sdhci_mmio_be_ops = {
>>>>>>          .max_access_size = 4,
>>>>>>          .unaligned = false
>>>>>>      },
>>>>>> -    .endianness = DEVICE_BIG_ENDIAN,
>>>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>>>> };
>>>>>>
>>>>>> static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
>>>>>> @@ -1467,23 +1456,6 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>>>>>>      SDHCIClass *sc = s->sc;
>>>>>>      const char *class_name = object_get_typename(OBJECT(s));
>>>>>>
>>>>>> -    s->io_ops = sc->io_ops ?: &sdhci_mmio_le_ops;
>>>>>> -    switch (s->endianness) {
>>>>>> -    case DEVICE_LITTLE_ENDIAN:
>>>>>> -        /* s->io_ops is little endian by default */
>>>>>> -        break;
>>>>>> -    case DEVICE_BIG_ENDIAN:
>>>>>> -        if (s->io_ops != &sdhci_mmio_le_ops) {
>>>>>> -            error_setg(errp, "SD controller doesn't support big endianness");
>>>>>> -            return;
>>>>>> -        }
>>>>>> -        s->io_ops = &sdhci_mmio_be_ops;
>>>>>> -        break;
>>>>>> -    default:
>>>>>> -        error_setg(errp, "Incorrect endianness");
>>>>>> -        return;
>>>>>> -    }
>>>>>> -
>>>>>>      sdhci_init_readonly_registers(s, errp);
>>>>>>      if (*errp) {
>>>>>>          return;
>>>>>> @@ -1493,7 +1465,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>>>>>>      s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>>>>>
>>>>>>      assert(sc->iomem_size >= SDHC_REGISTERS_MAP_SIZE);
>>>>>> -    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, class_name,
>>>>>> +    memory_region_init_io(&s->iomem, OBJECT(s), sc->io_ops, s, class_name,
>>>>>>                            sc->iomem_size);
>>>>>> }
>>>>>>
>>>>>> @@ -1578,6 +1550,7 @@ void sdhci_common_class_init(ObjectClass *klass, const void *data)
>>>>>>      dc->vmsd = &sdhci_vmstate;
>>>>>>      device_class_set_legacy_reset(dc, sdhci_poweron_reset);
>>>>>>
>>>>>> +    sc->io_ops = &sdhci_mmio_le_ops;
>>>>>
>>>>> You call common_class_init in subclass class_inits last so this would overwrite what subclass has set, doesn't it? I think you either have to change order in subclass class_init methods or not set this here.
>>>>
>>>> Oops... I'm surprised tests passed. Do we have coverage for sdhci on
>>>> e500 machines? Or are we only testing them via virtio PCI block storage?
>>>
>>> Not sure if that is what you are asking, but I have been testing it with
>>> sdhci-pci for a long time (not this series, though).
>>
>> I'm referring to the Freescale eSDHC controller of PPC e500 machines
>> (see previous patch).
> 
> I think testing SDHCI is generally difficult since the images need to be resized to a power of two. Any idea how to do this with the new functional tests?

we can truncate to 64M the rootfs used in  :

    tests/functional/test_ppc64_e500.py

and boot from it in a new test if that's supported by the machine.


Thanks,

C.

Re: [PATCH v5 14/14] hw/sd/sdhci: Remove unnecessary 'endianness' property

[Philippe Mathieu-Daudé]

On 10/3/25 19:24, Cédric Le Goater wrote:
> On 3/10/25 18:38, Bernhard Beschow wrote:
>>
>>
>> Am 10. März 2025 17:31:57 UTC schrieb "Philippe Mathieu-Daudé" 
>> <philmd@linaro.org>:
>>> On 10/3/25 16:56, Guenter Roeck wrote:
>>>> On 3/10/25 08:27, Philippe Mathieu-Daudé wrote:
>>>>> On 10/3/25 15:09, BALATON Zoltan wrote:
>>>>>> On Mon, 10 Mar 2025, Philippe Mathieu-Daudé wrote:
>>>>>>> The previous commit removed the single use of instance
>>>>>>> setting the "endianness" property.
>>>>>>>
>>>>>>> Since classes can register their io_ops with correct
>>>>>>> endianness, no need to support different ones.
>>>>>>>
>>>>>>> Remove the code related to SDHCIState::endianess field.
>>>>>>>
>>>>>>> Remove the now unused SDHCIState::io_ops field, since we
>>>>>>> directly use the class one.
>>>>>>>
>>>>>>> Suggested-by: Bernhard Beschow <shentey@gmail.com>
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>> ---
>>>>>>> hw/sd/sdhci-internal.h |  1 -
>>>>>>> include/hw/sd/sdhci.h  |  2 --
>>>>>>> hw/sd/sdhci.c          | 33 +++------------------------------
>>>>>>> 3 files changed, 3 insertions(+), 33 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
>>>>>>> index d99a8493db2..e4da6c831d1 100644
>>>>>>> --- a/hw/sd/sdhci-internal.h
>>>>>>> +++ b/hw/sd/sdhci-internal.h
>>>>>>> @@ -308,7 +308,6 @@ extern const VMStateDescription sdhci_vmstate;
>>>>>>> #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
>>>>>>>
>>>>>>> #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
>>>>>>> -    DEFINE_PROP_UINT8("endianness", _state, endianness, 
>>>>>>> DEVICE_LITTLE_ENDIAN), \
>>>>>>>      DEFINE_PROP_UINT8("sd-spec-version", _state, 
>>>>>>> sd_spec_version, 2), \
>>>>>>>      DEFINE_PROP_UINT8("uhs", _state, uhs_mode, 
>>>>>>> UHS_NOT_SUPPORTED), \
>>>>>>>      \
>>>>>>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>>>>>>> index e8fced5eedc..1016a5b5b77 100644
>>>>>>> --- a/include/hw/sd/sdhci.h
>>>>>>> +++ b/include/hw/sd/sdhci.h
>>>>>>> @@ -54,7 +54,6 @@ struct SDHCIState {
>>>>>>>      AddressSpace sysbus_dma_as;
>>>>>>>      AddressSpace *dma_as;
>>>>>>>      MemoryRegion *dma_mr;
>>>>>>> -    const MemoryRegionOps *io_ops;
>>>>>>>
>>>>>>>      QEMUTimer *insert_timer;       /* timer for 'changing' sd 
>>>>>>> card. */
>>>>>>>      QEMUTimer *transfer_timer;
>>>>>>> @@ -105,7 +104,6 @@ struct SDHCIState {
>>>>>>>
>>>>>>>      /* Configurable properties */
>>>>>>>      uint32_t quirks;
>>>>>>> -    uint8_t endianness;
>>>>>>>      uint8_t sd_spec_version;
>>>>>>>      uint8_t uhs_mode;
>>>>>>> };
>>>>>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>>>>>> index 47e4bd1a610..cbb9f4ae8c0 100644
>>>>>>> --- a/hw/sd/sdhci.c
>>>>>>> +++ b/hw/sd/sdhci.c
>>>>>>> @@ -1391,17 +1391,6 @@ sdhci_write(void *opaque, hwaddr offset, 
>>>>>>> uint64_t val, unsigned size)
>>>>>>> }
>>>>>>>
>>>>>>> static const MemoryRegionOps sdhci_mmio_le_ops = {
>>>>>>> -    .read = sdhci_read,
>>>>>>> -    .write = sdhci_write,
>>>>>>> -    .valid = {
>>>>>>> -        .min_access_size = 1,
>>>>>>> -        .max_access_size = 4,
>>>>>>> -        .unaligned = false
>>>>>>> -    },
>>>>>>> -    .endianness = DEVICE_LITTLE_ENDIAN,
>>>>>>> -};
>>>>>>> -
>>>>>>> -static const MemoryRegionOps sdhci_mmio_be_ops = {
>>>>>>>      .read = sdhci_read,
>>>>>>>      .write = sdhci_write,
>>>>>>>      .impl = {
>>>>>>> @@ -1413,7 +1402,7 @@ static const MemoryRegionOps 
>>>>>>> sdhci_mmio_be_ops = {
>>>>>>>          .max_access_size = 4,
>>>>>>>          .unaligned = false
>>>>>>>      },
>>>>>>> -    .endianness = DEVICE_BIG_ENDIAN,
>>>>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>>>>> };
>>>>>>>
>>>>>>> static void sdhci_init_readonly_registers(SDHCIState *s, Error 
>>>>>>> **errp)
>>>>>>> @@ -1467,23 +1456,6 @@ void sdhci_common_realize(SDHCIState *s, 
>>>>>>> Error **errp)
>>>>>>>      SDHCIClass *sc = s->sc;
>>>>>>>      const char *class_name = object_get_typename(OBJECT(s));
>>>>>>>
>>>>>>> -    s->io_ops = sc->io_ops ?: &sdhci_mmio_le_ops;
>>>>>>> -    switch (s->endianness) {
>>>>>>> -    case DEVICE_LITTLE_ENDIAN:
>>>>>>> -        /* s->io_ops is little endian by default */
>>>>>>> -        break;
>>>>>>> -    case DEVICE_BIG_ENDIAN:
>>>>>>> -        if (s->io_ops != &sdhci_mmio_le_ops) {
>>>>>>> -            error_setg(errp, "SD controller doesn't support big 
>>>>>>> endianness");
>>>>>>> -            return;
>>>>>>> -        }
>>>>>>> -        s->io_ops = &sdhci_mmio_be_ops;
>>>>>>> -        break;
>>>>>>> -    default:
>>>>>>> -        error_setg(errp, "Incorrect endianness");
>>>>>>> -        return;
>>>>>>> -    }
>>>>>>> -
>>>>>>>      sdhci_init_readonly_registers(s, errp);
>>>>>>>      if (*errp) {
>>>>>>>          return;
>>>>>>> @@ -1493,7 +1465,7 @@ void sdhci_common_realize(SDHCIState *s, 
>>>>>>> Error **errp)
>>>>>>>      s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>>>>>>
>>>>>>>      assert(sc->iomem_size >= SDHC_REGISTERS_MAP_SIZE);
>>>>>>> -    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, 
>>>>>>> class_name,
>>>>>>> +    memory_region_init_io(&s->iomem, OBJECT(s), sc->io_ops, s, 
>>>>>>> class_name,
>>>>>>>                            sc->iomem_size);
>>>>>>> }
>>>>>>>
>>>>>>> @@ -1578,6 +1550,7 @@ void sdhci_common_class_init(ObjectClass 
>>>>>>> *klass, const void *data)
>>>>>>>      dc->vmsd = &sdhci_vmstate;
>>>>>>>      device_class_set_legacy_reset(dc, sdhci_poweron_reset);
>>>>>>>
>>>>>>> +    sc->io_ops = &sdhci_mmio_le_ops;
>>>>>>
>>>>>> You call common_class_init in subclass class_inits last so this 
>>>>>> would overwrite what subclass has set, doesn't it? I think you 
>>>>>> either have to change order in subclass class_init methods or not 
>>>>>> set this here.
>>>>>
>>>>> Oops... I'm surprised tests passed. Do we have coverage for sdhci on
>>>>> e500 machines? Or are we only testing them via virtio PCI block 
>>>>> storage?
>>>>
>>>> Not sure if that is what you are asking, but I have been testing it 
>>>> with
>>>> sdhci-pci for a long time (not this series, though).
>>>
>>> I'm referring to the Freescale eSDHC controller of PPC e500 machines
>>> (see previous patch).
>>
>> I think testing SDHCI is generally difficult since the images need to 
>> be resized to a power of two.

historical references for this "sdcard power of 2" limitation:
https://lore.kernel.org/qemu-devel/20210623180021.898286-1-f4bug@amsat.org/
https://lore.kernel.org/qemu-devel/4b846383-83bf-4252-a172-95604f2f585b@linaro.org/

> Any idea how to do this with the new 
>> functional tests?
> 
> we can truncate to 64M the rootfs used in  :
> 
>     tests/functional/test_ppc64_e500.py
> 
> and boot from it in a new test if that's supported by the machine.

Yes, that is the best we can do until we implement the async DMA.

Regards,

Phil.

Re: [PATCH v5 14/14] hw/sd/sdhci: Remove unnecessary 'endianness' property

[Guenter Roeck]

On 3/10/25 10:31, Philippe Mathieu-Daudé wrote:
...
>>> Oops... I'm surprised tests passed. Do we have coverage for sdhci on
>>> e500 machines? Or are we only testing them via virtio PCI block storage?
>>
>> Not sure if that is what you are asking, but I have been testing it with
>> sdhci-pci for a long time (not this series, though).
> 
> I'm referring to the Freescale eSDHC controller of PPC e500 machines
> (see previous patch).

Turns out I do test that as well. The tests pass with qemu v9.2.2.
With qemu mainline plus this patch series the sdhci controller
fails to initialize. Booting the ppce500 machine gets me a series
of:

mmc0: Reset 0x1 never completed.
mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
mmc0: sdhci: Sys addr:  0x08000000 | Version:  0x00000124
mmc0: sdhci: Blk size:  0x00000800 | Blk cnt:  0x00000000
mmc0: sdhci: Argument:  0x00000000 | Trn mode: 0x00000000
mmc0: sdhci: Present:   0x0000fa01 | Host ctl: 0x00000000
mmc0: sdhci: Power:     0x00000000 | Blk gap:  0x00000000
mmc0: sdhci: Wake-up:   0x00000020 | Clock:    0x00000000
mmc0: sdhci: Timeout:   0x00000080 | Int stat: 0x00000000
mmc0: sdhci: Int enab:  0x3f017f11 | Sig enab: 0x00000000
mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000124
mmc0: sdhci: Caps:      0xb4347805 | Caps_1:   0x00000000
mmc0: sdhci: Cmd:       0x00000000 | Max curr: 0x00000000
mmc0: sdhci: Resp[0]:   0x00000000 | Resp[1]:  0x00000000
mmc0: sdhci: Resp[2]:   0x00000000 | Resp[3]:  0x00000000
mmc0: sdhci: Host ctl2: 0x00000000
mmc0: sdhci: ============================================
mmc0: Unknown controller version (36). You may experience problems.

Guenter

Re: [PATCH v5 14/14] hw/sd/sdhci: Remove unnecessary 'endianness' property

[Bernhard Beschow]

Am 10. März 2025 00:06:20 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>The previous commit removed the single use of instance
>setting the "endianness" property.
>
>Since classes can register their io_ops with correct
>endianness, no need to support different ones.
>
>Remove the code related to SDHCIState::endianess field.
>
>Remove the now unused SDHCIState::io_ops field, since we
>directly use the class one.
>
>Suggested-by: Bernhard Beschow <shentey@gmail.com>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/sd/sdhci-internal.h |  1 -
> include/hw/sd/sdhci.h  |  2 --
> hw/sd/sdhci.c          | 33 +++------------------------------
> 3 files changed, 3 insertions(+), 33 deletions(-)

I don't have the code in front of me right now. IIRC, the PCI subclass sets the "endianness" property as well, but doesn't need to. This has to be removed, otherwise creation of the PCI device will fail.

Best regards,
Bernhard

>
>diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
>index d99a8493db2..e4da6c831d1 100644
>--- a/hw/sd/sdhci-internal.h
>+++ b/hw/sd/sdhci-internal.h
>@@ -308,7 +308,6 @@ extern const VMStateDescription sdhci_vmstate;
> #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
> 
> #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
>-    DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), \
>     DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
>     DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
>     \
>diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>index e8fced5eedc..1016a5b5b77 100644
>--- a/include/hw/sd/sdhci.h
>+++ b/include/hw/sd/sdhci.h
>@@ -54,7 +54,6 @@ struct SDHCIState {
>     AddressSpace sysbus_dma_as;
>     AddressSpace *dma_as;
>     MemoryRegion *dma_mr;
>-    const MemoryRegionOps *io_ops;
> 
>     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
>     QEMUTimer *transfer_timer;
>@@ -105,7 +104,6 @@ struct SDHCIState {
> 
>     /* Configurable properties */
>     uint32_t quirks;
>-    uint8_t endianness;
>     uint8_t sd_spec_version;
>     uint8_t uhs_mode;
> };
>diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>index 47e4bd1a610..cbb9f4ae8c0 100644
>--- a/hw/sd/sdhci.c
>+++ b/hw/sd/sdhci.c
>@@ -1391,17 +1391,6 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
> }
> 
> static const MemoryRegionOps sdhci_mmio_le_ops = {
>-    .read = sdhci_read,
>-    .write = sdhci_write,
>-    .valid = {
>-        .min_access_size = 1,
>-        .max_access_size = 4,
>-        .unaligned = false
>-    },
>-    .endianness = DEVICE_LITTLE_ENDIAN,
>-};
>-
>-static const MemoryRegionOps sdhci_mmio_be_ops = {
>     .read = sdhci_read,
>     .write = sdhci_write,
>     .impl = {
>@@ -1413,7 +1402,7 @@ static const MemoryRegionOps sdhci_mmio_be_ops = {
>         .max_access_size = 4,
>         .unaligned = false
>     },
>-    .endianness = DEVICE_BIG_ENDIAN,
>+    .endianness = DEVICE_LITTLE_ENDIAN,
> };
> 
> static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
>@@ -1467,23 +1456,6 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>     SDHCIClass *sc = s->sc;
>     const char *class_name = object_get_typename(OBJECT(s));
> 
>-    s->io_ops = sc->io_ops ?: &sdhci_mmio_le_ops;
>-    switch (s->endianness) {
>-    case DEVICE_LITTLE_ENDIAN:
>-        /* s->io_ops is little endian by default */
>-        break;
>-    case DEVICE_BIG_ENDIAN:
>-        if (s->io_ops != &sdhci_mmio_le_ops) {
>-            error_setg(errp, "SD controller doesn't support big endianness");
>-            return;
>-        }
>-        s->io_ops = &sdhci_mmio_be_ops;
>-        break;
>-    default:
>-        error_setg(errp, "Incorrect endianness");
>-        return;
>-    }
>-
>     sdhci_init_readonly_registers(s, errp);
>     if (*errp) {
>         return;
>@@ -1493,7 +1465,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>     s->fifo_buffer = g_malloc0(s->buf_maxsz);
> 
>     assert(sc->iomem_size >= SDHC_REGISTERS_MAP_SIZE);
>-    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, class_name,
>+    memory_region_init_io(&s->iomem, OBJECT(s), sc->io_ops, s, class_name,
>                           sc->iomem_size);
> }
> 
>@@ -1578,6 +1550,7 @@ void sdhci_common_class_init(ObjectClass *klass, const void *data)
>     dc->vmsd = &sdhci_vmstate;
>     device_class_set_legacy_reset(dc, sdhci_poweron_reset);
> 
>+    sc->io_ops = &sdhci_mmio_le_ops;
>     sc->iomem_size = SDHC_REGISTERS_MAP_SIZE;
> }
> 

Re: [PATCH v5 14/14] hw/sd/sdhci: Remove unnecessary 'endianness' property

[Philippe Mathieu-Daudé]

On 11/3/25 08:31, Bernhard Beschow wrote:
> 
> 
> Am 10. März 2025 00:06:20 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> The previous commit removed the single use of instance
>> setting the "endianness" property.
>>
>> Since classes can register their io_ops with correct
>> endianness, no need to support different ones.
>>
>> Remove the code related to SDHCIState::endianess field.
>>
>> Remove the now unused SDHCIState::io_ops field, since we
>> directly use the class one.
>>
>> Suggested-by: Bernhard Beschow <shentey@gmail.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/sd/sdhci-internal.h |  1 -
>> include/hw/sd/sdhci.h  |  2 --
>> hw/sd/sdhci.c          | 33 +++------------------------------
>> 3 files changed, 3 insertions(+), 33 deletions(-)
> 
> I don't have the code in front of me right now. IIRC, the PCI subclass sets the "endianness" property as well, but doesn't need to. This has to be removed, otherwise creation of the PCI device will fail.

There was a patch in v4 doing that, but I dropped it in v5 :)