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

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

[Philippe Mathieu-Daudé]

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.

I expect (with few QOM knowledge) this to be trivial to review.

- Remove SDHCIState::vendor field
- Convert state fields to class ones
- Simplify endianness handling
- Add default reset values as class fields

[*] 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: 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: Enforce little endianness on PCI devices
  hw/sd/sdhci: Allow SDHCI classes to register their own MemoryRegionOps
  hw/sd/sdhci: Simplify MemoryRegionOps endianness check
  hw/sd/sdhci: Unify default MemoryRegionOps
  hw/sd/sdhci: Convert SDHCIState::endianness to EndianMode
  hw/sd/sdhci: Add SDHCIClass::ro::capareg field
  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-internal.h              |  28 ++---
 include/hw/qdev-properties-system.h |   1 +
 include/hw/sd/sdhci.h               |  46 +++++++-
 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                       | 172 ++++++++++++++++++----------
 11 files changed, 170 insertions(+), 100 deletions(-)

-- 
2.47.1

[PATCH v4 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

[PATCH v4 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>
---
 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 v4 03/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. As this is not a
normal use, introduce SDHCIClass in its own commit.

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

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 48247e9a20f..c4b20db3877 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -107,6 +107,13 @@ struct SDHCIState {
 };
 typedef struct SDHCIState SDHCIState;
 
+typedef struct SDHCIClass {
+    union {
+        PCIDeviceClass pci_parent_class;
+        SysBusDeviceClass sbd_parent_class;
+    };
+} SDHCIClass;
+
 /*
  * Controller does not provide transfer-complete interrupt when not
  * busy.
@@ -123,6 +130,8 @@ DECLARE_INSTANCE_CHECKER(SDHCIState, PCI_SDHCI,
 #define TYPE_SYSBUS_SDHCI "generic-sdhci"
 DECLARE_INSTANCE_CHECKER(SDHCIState, SYSBUS_SDHCI,
                          TYPE_SYSBUS_SDHCI)
+DECLARE_CLASS_CHECKERS(SDHCIClass, SYSBUS_SDHCI,
+                       TYPE_SYSBUS_SDHCI)
 
 #define TYPE_IMX_USDHC "imx-usdhc"
 
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 149b748cbee..4917a9b3632 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1960,6 +1960,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 v4 04/14] hw/sd/sdhci: Make quirks a class property

[Philippe Mathieu-Daudé]

All TYPE_IMX_USDHC instances use the quirk:
move it to the class layer.

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

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index c4b20db3877..0616ce3aa59 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -95,7 +95,6 @@ struct SDHCIState {
 
     /* Configurable properties */
     bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
-    uint32_t quirks;
     uint8_t endianness;
     uint8_t sd_spec_version;
     uint8_t uhs_mode;
@@ -112,6 +111,8 @@ typedef struct SDHCIClass {
         PCIDeviceClass pci_parent_class;
         SysBusDeviceClass sbd_parent_class;
     };
+
+    uint32_t quirks;
 } SDHCIClass;
 
 /*
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 4917a9b3632..2b7eb11a14a 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -345,6 +345,8 @@ static void sdhci_send_command(SDHCIState *s)
     rlen = sdbus_do_command(&s->sdbus, &request, response);
 
     if (s->cmdreg & SDHC_CMD_RESPONSE) {
+        SDHCIClass *sc = SYSBUS_SDHCI_GET_CLASS(s);
+
         if (rlen == 4) {
             s->rspreg[0] = ldl_be_p(response);
             s->rspreg[1] = s->rspreg[2] = s->rspreg[3] = 0;
@@ -366,7 +368,7 @@ static void sdhci_send_command(SDHCIState *s)
             }
         }
 
-        if (!(s->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
+        if (!(sc->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
             (s->norintstsen & SDHC_NISEN_TRSCMP) &&
             (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
             s->norintsts |= SDHC_NIS_TRSCMP;
@@ -1886,7 +1888,15 @@ static void imx_usdhc_init(Object *obj)
     SDHCIState *s = SYSBUS_SDHCI(obj);
 
     s->io_ops = &usdhc_mmio_ops;
-    s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
+}
+
+static void imx_usdhc_class_init(ObjectClass *oc, void *data)
+{
+    SDHCIClass *sc = SYSBUS_SDHCI_CLASS(oc);
+
+    sc->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
+
+    sdhci_common_class_init(oc, data);
 }
 
 /* --- qdev Samsung s3c --- */
@@ -1967,6 +1977,7 @@ static const TypeInfo sdhci_types[] = {
         .name = TYPE_IMX_USDHC,
         .parent = TYPE_SYSBUS_SDHCI,
         .instance_init = imx_usdhc_init,
+        .class_init = imx_usdhc_class_init,
     },
     {
         .name = TYPE_S3C_SDHCI,
-- 
2.47.1

[PATCH v4 05/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>
---
 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 0616ce3aa59..2709a7a69d5 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -113,6 +113,7 @@ typedef struct SDHCIClass {
     };
 
     uint32_t quirks;
+    uint64_t iomem_size;
 } SDHCIClass;
 
 /*
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 2b7eb11a14a..637067fef50 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1443,6 +1443,8 @@ void sdhci_uninitfn(SDHCIState *s)
 void sdhci_common_realize(SDHCIState *s, Error **errp)
 {
     ERRP_GUARD();
+    SDHCIClass *sc = SYSBUS_SDHCI_GET_CLASS(s);
+    const char *class_name = object_get_typename(OBJECT(s));
 
     switch (s->endianness) {
     case DEVICE_LITTLE_ENDIAN:
@@ -1468,8 +1470,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)
@@ -1621,11 +1624,14 @@ static void sdhci_sysbus_unrealize(DeviceState *dev)
 static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    SDHCIClass *sc = SYSBUS_SDHCI_CLASS(klass);
 
     device_class_set_props(dc, sdhci_sysbus_properties);
     dc->realize = sdhci_sysbus_realize;
     dc->unrealize = sdhci_sysbus_unrealize;
 
+    sc->iomem_size = SDHC_REGISTERS_MAP_SIZE;
+
     sdhci_common_class_init(klass, data);
 }
 
-- 
2.47.1

[PATCH v4 06/14] hw/sd/sdhci: Enforce little endianness on PCI devices

[Philippe Mathieu-Daudé]

This is the default, but better be safe than sorry.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sdhci-pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/sd/sdhci-pci.c b/hw/sd/sdhci-pci.c
index 5268c0dee50..5f82178a76f 100644
--- a/hw/sd/sdhci-pci.c
+++ b/hw/sd/sdhci-pci.c
@@ -32,6 +32,7 @@ static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
     SDHCIState *s = PCI_SDHCI(dev);
 
     sdhci_initfn(s);
+    qdev_prop_set_uint8(DEVICE(dev), "endianness", DEVICE_LITTLE_ENDIAN);
     sdhci_common_realize(s, errp);
     if (*errp) {
         return;
-- 
2.47.1

[PATCH v4 07/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 2709a7a69d5..60a0442c805 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -112,6 +112,7 @@ typedef struct SDHCIClass {
         SysBusDeviceClass sbd_parent_class;
     };
 
+    const MemoryRegionOps *io_ops;
     uint32_t quirks;
     uint64_t iomem_size;
 } SDHCIClass;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 637067fef50..ae485f90dfe 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1427,8 +1427,6 @@ void sdhci_initfn(SDHCIState *s)
                                    sdhci_raise_insertion_irq, s);
     s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                      sdhci_data_transfer, s);
-
-    s->io_ops = &sdhci_mmio_le_ops;
 }
 
 void sdhci_uninitfn(SDHCIState *s)
@@ -1446,6 +1444,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
     SDHCIClass *sc = SYSBUS_SDHCI_GET_CLASS(s);
     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 */
@@ -1889,17 +1888,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 = SDHCI_QUIRK_NO_BUSY_IRQ;
 
     sdhci_common_class_init(oc, data);
@@ -1956,11 +1949,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[] = {
@@ -1982,13 +1977,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

[PATCH v4 08/14] hw/sd/sdhci: Simplify MemoryRegionOps endianness check

[Philippe Mathieu-Daudé]

While little endianness is the default, ome controllers
might be only implemented in big endianness.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sdhci.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index ae485f90dfe..a2e7162e289 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1444,20 +1444,10 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
     SDHCIClass *sc = SYSBUS_SDHCI_GET_CLASS(s);
     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");
+    s->io_ops = sc->io_ops ?: (s->endianness == DEVICE_BIG_ENDIAN ?
+                               &sdhci_mmio_be_ops : &sdhci_mmio_le_ops);
+    if (s->io_ops->endianness != s->endianness) {
+        error_setg(errp, "Invalid endianness for SD controller");
         return;
     }
 
-- 
2.47.1

[PATCH v4 09/14] hw/sd/sdhci: Unify default MemoryRegionOps

[Philippe Mathieu-Daudé]

Note, sdhci_mmio_le_ops[] was missing .impl.access_size = 4.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sdhci.c | 46 ++++++++++++++++++++--------------------------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index a2e7162e289..23af3958a1d 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1372,30 +1372,22 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
                        value >> shift, value >> shift);
 }
 
-static const MemoryRegionOps sdhci_mmio_le_ops = {
-    .read = sdhci_read,
-    .write = sdhci_write,
-    .valid = {
-        .min_access_size = 1,
-        .max_access_size = 4,
-        .unaligned = false
+static const MemoryRegionOps sdhci_mmio_ops[2] = {
+    [0 ... 1] = {
+        .read = sdhci_read,
+        .write = sdhci_write,
+        .impl = {
+            .min_access_size = 4,
+            .max_access_size = 4,
+        },
+        .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 = {
-        .min_access_size = 4,
-        .max_access_size = 4,
-    },
-    .valid = {
-        .min_access_size = 1,
-        .max_access_size = 4,
-        .unaligned = false
-    },
-    .endianness = DEVICE_BIG_ENDIAN,
+    [0].endianness = DEVICE_LITTLE_ENDIAN,
+    [1].endianness = DEVICE_BIG_ENDIAN,
 };
 
 static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
@@ -1443,10 +1435,12 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
     ERRP_GUARD();
     SDHCIClass *sc = SYSBUS_SDHCI_GET_CLASS(s);
     const char *class_name = object_get_typename(OBJECT(s));
+    unsigned ops_index;
 
-    s->io_ops = sc->io_ops ?: (s->endianness == DEVICE_BIG_ENDIAN ?
-                               &sdhci_mmio_be_ops : &sdhci_mmio_le_ops);
-    if (s->io_ops->endianness != s->endianness) {
+    ops_index = s->endianness == DEVICE_BIG_ENDIAN ? 1 : 0;
+
+    s->io_ops = sc->io_ops ?: &sdhci_mmio_ops[ops_index];
+    if (s->io_ops->endianness != sdhci_mmio_ops[ops_index].endianness) {
         error_setg(errp, "Invalid endianness for SD controller");
         return;
     }
-- 
2.47.1

[PATCH v4 10/14] hw/sd/sdhci: Convert SDHCIState::endianness to EndianMode

[Philippe Mathieu-Daudé]

EndianMode enum is preferred over DEVICE_BIG/LITTLE_ENDIAN
values because it is a QAPI type.

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

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 9072b06bdde..c459279f3f3 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -24,6 +24,7 @@
 #ifndef SDHCI_INTERNAL_H
 #define SDHCI_INTERNAL_H
 
+#include "hw/qdev-properties-system.h"
 #include "hw/registerfields.h"
 
 /* R/W SDMA System Address register 0x0 */
@@ -308,7 +309,7 @@ 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_ENDIAN("endianness", _state, endianness, ENDIAN_MODE_LITTLE), \
     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 60a0442c805..a91cda16cbe 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -25,6 +25,7 @@
 #ifndef SDHCI_H
 #define SDHCI_H
 
+#include "qapi/qapi-types-common.h"
 #include "hw/pci/pci_device.h"
 #include "hw/sysbus.h"
 #include "hw/sd/sd.h"
@@ -95,7 +96,7 @@ struct SDHCIState {
 
     /* Configurable properties */
     bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
-    uint8_t endianness;
+    EndianMode endianness;
     uint8_t sd_spec_version;
     uint8_t uhs_mode;
     /*
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index fe8b9f79621..e85e000f054 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1042,7 +1042,7 @@ void ppce500_init(MachineState *machine)
          */
         dev = qdev_new(TYPE_SYSBUS_SDHCI);
         qdev_prop_set_uint8(dev, "sd-spec-version", 2);
-        qdev_prop_set_uint8(dev, "endianness", DEVICE_BIG_ENDIAN);
+        qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_BIG);
         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));
diff --git a/hw/sd/sdhci-pci.c b/hw/sd/sdhci-pci.c
index 5f82178a76f..49c4f0478b4 100644
--- a/hw/sd/sdhci-pci.c
+++ b/hw/sd/sdhci-pci.c
@@ -32,7 +32,7 @@ static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
     SDHCIState *s = PCI_SDHCI(dev);
 
     sdhci_initfn(s);
-    qdev_prop_set_uint8(DEVICE(dev), "endianness", DEVICE_LITTLE_ENDIAN);
+    qdev_prop_set_enum(DEVICE(dev), "endianness", ENDIAN_MODE_LITTLE);
     sdhci_common_realize(s, errp);
     if (*errp) {
         return;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 23af3958a1d..f2bb612c665 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1437,7 +1437,12 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
     const char *class_name = object_get_typename(OBJECT(s));
     unsigned ops_index;
 
-    ops_index = s->endianness == DEVICE_BIG_ENDIAN ? 1 : 0;
+    if (s->endianness == ENDIAN_MODE_UNSPECIFIED) {
+        error_setg(errp, "%s property 'endianness'"
+                         " must be set to 'big' or 'little'", class_name);
+        return;
+    }
+    ops_index = s->endianness == ENDIAN_MODE_BIG ? 1 : 0;
 
     s->io_ops = sc->io_ops ?: &sdhci_mmio_ops[ops_index];
     if (s->io_ops->endianness != sdhci_mmio_ops[ops_index].endianness) {
-- 
2.47.1

[PATCH v4 11/14] hw/sd/sdhci: Add SDHCIClass::ro::capareg field

[Philippe Mathieu-Daudé]

Capability register is read-only.

Since we allow instances to clear/set extra bits, 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 | 5 +++++
 hw/sd/sdhci.c         | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index a91cda16cbe..15ef3a07b54 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -116,6 +116,11 @@ typedef struct SDHCIClass {
     const MemoryRegionOps *io_ops;
     uint32_t quirks;
     uint64_t iomem_size;
+
+    /* Read-only registers */
+    struct {
+        uint64_t capareg;
+    } ro;
 } SDHCIClass;
 
 /*
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index f2bb612c665..9708b52f850 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 = SYSBUS_SDHCI_GET_CLASS(s);
     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)
-- 
2.47.1

[PATCH v4 12/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>
---
 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 c459279f3f3..4ca269831f3 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -71,7 +71,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
@@ -89,7 +89,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 */
@@ -105,17 +105,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)
@@ -129,17 +129,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
@@ -152,7 +152,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
@@ -160,7 +160,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
@@ -171,7 +171,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
@@ -206,7 +206,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 15ef3a07b54..eb8380187b5 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -117,6 +117,26 @@ typedef 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 9708b52f850..234a6e4a1fe 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -294,6 +294,7 @@ static void sdhci_set_readonly(DeviceState *dev, bool level)
 
 static void sdhci_reset(SDHCIState *s)
 {
+    SDHCIClass *sc = SYSBUS_SDHCI_GET_CLASS(s);
     DeviceState *dev = DEVICE(s);
 
     timer_del(s->insert_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 v4 13/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         | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index eb8380187b5..966a1751f50 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -162,6 +162,8 @@ DECLARE_INSTANCE_CHECKER(SDHCIState, SYSBUS_SDHCI,
 DECLARE_CLASS_CHECKERS(SDHCIClass, SYSBUS_SDHCI,
                        TYPE_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 234a6e4a1fe..d5cc0bf1458 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1653,7 +1653,37 @@ 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 = {
+        .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
 
@@ -1983,6 +2013,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 v4 14/14] hw/ppc/e500: Replace generic SDHCI by Freescale eSDHC

[Philippe Mathieu-Daudé]

As Zoltan reported, some U-Boot versions seem to expect
correctly initialized registers 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>
---
 hw/ppc/e500.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index e85e000f054..7d15c926887 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1027,20 +1027,12 @@ 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_enum(dev, "endianness", ENDIAN_MODE_BIG);
         s = SYS_BUS_DEVICE(dev);
-- 
2.47.1

Re: [PATCH v4 03/14] hw/sd/sdhci: Introduce SDHCIClass stub

[BALATON Zoltan]

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

On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
> TYPE_SYSBUS_SDHCI is a bit odd because it uses an union
> to work with both SysBus / PCI parent. As this is not a
> normal use, introduce SDHCIClass in its own commit.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> include/hw/sd/sdhci.h | 9 +++++++++
> hw/sd/sdhci.c         | 1 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 48247e9a20f..c4b20db3877 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -107,6 +107,13 @@ struct SDHCIState {
> };
> typedef struct SDHCIState SDHCIState;
>
> +typedef struct SDHCIClass {
> +    union {
> +        PCIDeviceClass pci_parent_class;
> +        SysBusDeviceClass sbd_parent_class;
> +    };
> +} SDHCIClass;
> +
> /*
>  * Controller does not provide transfer-complete interrupt when not
>  * busy.
> @@ -123,6 +130,8 @@ DECLARE_INSTANCE_CHECKER(SDHCIState, PCI_SDHCI,
> #define TYPE_SYSBUS_SDHCI "generic-sdhci"
> DECLARE_INSTANCE_CHECKER(SDHCIState, SYSBUS_SDHCI,
>                          TYPE_SYSBUS_SDHCI)
> +DECLARE_CLASS_CHECKERS(SDHCIClass, SYSBUS_SDHCI,
> +                       TYPE_SYSBUS_SDHCI)

Are these two together just OBJECT_DECLARE_TYPE? Then the above typedefs 
are also not needed just the struct definitions.

Regards,
BALATON Zoltan

> #define TYPE_IMX_USDHC "imx-usdhc"
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 149b748cbee..4917a9b3632 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1960,6 +1960,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,
>     },
>     {
>

Re: [PATCH v4 08/14] hw/sd/sdhci: Simplify MemoryRegionOps endianness check

[BALATON Zoltan]

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

On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
> While little endianness is the default, ome controllers

Typo, ome -> some.

Regards,
BALATON Zoltan

> might be only implemented in big endianness.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/sd/sdhci.c | 18 ++++--------------
> 1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index ae485f90dfe..a2e7162e289 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1444,20 +1444,10 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>     SDHCIClass *sc = SYSBUS_SDHCI_GET_CLASS(s);
>     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");
> +    s->io_ops = sc->io_ops ?: (s->endianness == DEVICE_BIG_ENDIAN ?
> +                               &sdhci_mmio_be_ops : &sdhci_mmio_le_ops);
> +    if (s->io_ops->endianness != s->endianness) {
> +        error_setg(errp, "Invalid endianness for SD controller");
>         return;
>     }
>
>

Re: [PATCH v4 03/14] hw/sd/sdhci: Introduce SDHCIClass stub

[Philippe Mathieu-Daudé]

On 8/3/25 23:34, BALATON Zoltan wrote:
> On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
>> TYPE_SYSBUS_SDHCI is a bit odd because it uses an union
>> to work with both SysBus / PCI parent. As this is not a
>> normal use, introduce SDHCIClass in its own commit.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> include/hw/sd/sdhci.h | 9 +++++++++
>> hw/sd/sdhci.c         | 1 +
>> 2 files changed, 10 insertions(+)
>>
>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>> index 48247e9a20f..c4b20db3877 100644
>> --- a/include/hw/sd/sdhci.h
>> +++ b/include/hw/sd/sdhci.h
>> @@ -107,6 +107,13 @@ struct SDHCIState {
>> };
>> typedef struct SDHCIState SDHCIState;
>>
>> +typedef struct SDHCIClass {
>> +    union {
>> +        PCIDeviceClass pci_parent_class;
>> +        SysBusDeviceClass sbd_parent_class;
>> +    };
>> +} SDHCIClass;
>> +
>> /*
>>  * Controller does not provide transfer-complete interrupt when not
>>  * busy.
>> @@ -123,6 +130,8 @@ DECLARE_INSTANCE_CHECKER(SDHCIState, PCI_SDHCI,
>> #define TYPE_SYSBUS_SDHCI "generic-sdhci"
>> DECLARE_INSTANCE_CHECKER(SDHCIState, SYSBUS_SDHCI,
>>                          TYPE_SYSBUS_SDHCI)
>> +DECLARE_CLASS_CHECKERS(SDHCIClass, SYSBUS_SDHCI,
>> +                       TYPE_SYSBUS_SDHCI)
> 
> Are these two together just OBJECT_DECLARE_TYPE? Then the above typedefs 
> are also not needed just the struct definitions.

I'd like to but it isn't possible because the same object state/class is
used by distinct types (PCI & SysBus).

The following (expected to be correct) change ...:
-- >8 --
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 966a1751f50..341b130995b 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -155,10 +155,6 @@ typedef struct SDHCIClass {
  #define TYPE_PCI_SDHCI "sdhci-pci"
-DECLARE_INSTANCE_CHECKER(SDHCIState, PCI_SDHCI,
-                         TYPE_PCI_SDHCI)
+OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, PCI_SDHCI)

  #define TYPE_SYSBUS_SDHCI "generic-sdhci"
-DECLARE_INSTANCE_CHECKER(SDHCIState, SYSBUS_SDHCI,
-                         TYPE_SYSBUS_SDHCI)
-DECLARE_CLASS_CHECKERS(SDHCIClass, SYSBUS_SDHCI,
-                       TYPE_SYSBUS_SDHCI)
+OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, SYSBUS_SDHCI)
---

... produces:

FAILED: libqemu-aarch64-softmmu.a.p/hw_arm_mcimx6ul-evk.c.o
In file included from ../../hw/arm/mcimx6ul-evk.c:15:
In file included from include/hw/arm/fsl-imx6ul.h:31:
include/hw/sd/sdhci.h:165:1: error: redefinition of 
'glib_autoptr_clear_SDHCIState'
   165 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, SYSBUS_SDHCI)
       | ^
include/qom/object.h:238:5: note: expanded from macro 'OBJECT_DECLARE_TYPE'
   238 |     G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \
       |     ^
/usr/include/glib-2.0/glib/gmacros.h:1379:3: note: expanded from macro 
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
  1379 |   _GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS(TypeName, TypeName, func)
       |   ^
/usr/include/glib-2.0/glib/gmacros.h:1360:36: note: expanded from macro 
'_GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS'
  1360 |   static G_GNUC_UNUSED inline void 
_GLIB_AUTOPTR_CLEAR_FUNC_NAME(TypeName) (TypeName *_ptr) 
     \
       |                                    ^
/usr/include/glib-2.0/glib/gmacros.h:1343:49: note: expanded from macro 
'_GLIB_AUTOPTR_CLEAR_FUNC_NAME'
  1343 | #define _GLIB_AUTOPTR_CLEAR_FUNC_NAME(TypeName) 
glib_autoptr_clear_##TypeName
       |                                                 ^
<scratch space>:77:1: note: expanded from here
    77 | glib_autoptr_clear_SDHCIState
       | ^
include/hw/sd/sdhci.h:158:1: note: previous definition is here
   158 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, PCI_SDHCI)
       | ^
include/qom/object.h:238:5: note: expanded from macro 'OBJECT_DECLARE_TYPE'
   238 |     G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \
       |     ^
/usr/include/glib-2.0/glib/gmacros.h:1379:3: note: expanded from macro 
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
  1379 |   _GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS(TypeName, TypeName, func)
       |   ^
/usr/include/glib-2.0/glib/gmacros.h:1360:36: note: expanded from macro 
'_GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS'
  1360 |   static G_GNUC_UNUSED inline void 
_GLIB_AUTOPTR_CLEAR_FUNC_NAME(TypeName) (TypeName *_ptr) 
     \
       |                                    ^
/usr/include/glib-2.0/glib/gmacros.h:1343:49: note: expanded from macro 
'_GLIB_AUTOPTR_CLEAR_FUNC_NAME'
  1343 | #define _GLIB_AUTOPTR_CLEAR_FUNC_NAME(TypeName) 
glib_autoptr_clear_##TypeName
       |                                                 ^
<scratch space>:48:1: note: expanded from here
    48 | glib_autoptr_clear_SDHCIState
       | ^
In file included from ../../hw/arm/mcimx6ul-evk.c:15:
In file included from include/hw/arm/fsl-imx6ul.h:31:
include/hw/sd/sdhci.h:165:1: error: redefinition of 
'glib_autoptr_cleanup_SDHCIState'
   165 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, SYSBUS_SDHCI)
       | ^
include/qom/object.h:238:5: note: expanded from macro 'OBJECT_DECLARE_TYPE'
   238 |     G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \
       |     ^
/usr/include/glib-2.0/glib/gmacros.h:1379:3: note: expanded from macro 
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
  1379 |   _GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS(TypeName, TypeName, func)
       |   ^
/usr/include/glib-2.0/glib/gmacros.h:1362:36: note: expanded from macro 
'_GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS'
  1362 |   static G_GNUC_UNUSED inline void 
_GLIB_AUTOPTR_FUNC_NAME(TypeName) (TypeName **_ptr) 
     \
       |                                    ^
/usr/include/glib-2.0/glib/gmacros.h:1342:43: note: expanded from macro 
'_GLIB_AUTOPTR_FUNC_NAME'
  1342 | #define _GLIB_AUTOPTR_FUNC_NAME(TypeName) 
glib_autoptr_cleanup_##TypeName
       |                                           ^
<scratch space>:78:1: note: expanded from here
    78 | glib_autoptr_cleanup_SDHCIState
       | ^
include/hw/sd/sdhci.h:158:1: note: previous definition is here
   158 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, PCI_SDHCI)
       | ^
include/qom/object.h:238:5: note: expanded from macro 'OBJECT_DECLARE_TYPE'
   238 |     G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \
       |     ^
/usr/include/glib-2.0/glib/gmacros.h:1379:3: note: expanded from macro 
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
  1379 |   _GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS(TypeName, TypeName, func)
       |   ^
/usr/include/glib-2.0/glib/gmacros.h:1362:36: note: expanded from macro 
'_GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS'
  1362 |   static G_GNUC_UNUSED inline void 
_GLIB_AUTOPTR_FUNC_NAME(TypeName) (TypeName **_ptr) 
     \
       |                                    ^
/usr/include/glib-2.0/glib/gmacros.h:1342:43: note: expanded from macro 
'_GLIB_AUTOPTR_FUNC_NAME'
  1342 | #define _GLIB_AUTOPTR_FUNC_NAME(TypeName) 
glib_autoptr_cleanup_##TypeName
       |                                           ^
<scratch space>:49:1: note: expanded from here
    49 | glib_autoptr_cleanup_SDHCIState
       | ^
In file included from ../../hw/arm/mcimx6ul-evk.c:15:
In file included from include/hw/arm/fsl-imx6ul.h:31:
include/hw/sd/sdhci.h:165:1: error: redefinition of 
'glib_autoptr_destroy_SDHCIState'
   165 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, SYSBUS_SDHCI)
       | ^
include/qom/object.h:238:5: note: expanded from macro 'OBJECT_DECLARE_TYPE'
   238 |     G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \
       |     ^
/usr/include/glib-2.0/glib/gmacros.h:1379:3: note: expanded from macro 
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
  1379 |   _GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS(TypeName, TypeName, func)
       |   ^
/usr/include/glib-2.0/glib/gmacros.h:1364:36: note: expanded from macro 
'_GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS'
  1364 |   static G_GNUC_UNUSED inline void 
_GLIB_AUTOPTR_DESTROY_FUNC_NAME(TypeName) (void *_ptr) 
     \
       |                                    ^
/usr/include/glib-2.0/glib/gmacros.h:1344:51: note: expanded from macro 
'_GLIB_AUTOPTR_DESTROY_FUNC_NAME'
  1344 | #define _GLIB_AUTOPTR_DESTROY_FUNC_NAME(TypeName) 
glib_autoptr_destroy_##TypeName
       |                                                   ^
<scratch space>:80:1: note: expanded from here
    80 | glib_autoptr_destroy_SDHCIState
       | ^
include/hw/sd/sdhci.h:158:1: note: previous definition is here
   158 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, PCI_SDHCI)
       | ^
include/qom/object.h:238:5: note: expanded from macro 'OBJECT_DECLARE_TYPE'
   238 |     G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \
       |     ^
/usr/include/glib-2.0/glib/gmacros.h:1379:3: note: expanded from macro 
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
  1379 |   _GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS(TypeName, TypeName, func)
       |   ^
/usr/include/glib-2.0/glib/gmacros.h:1364:36: note: expanded from macro 
'_GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS'
  1364 |   static G_GNUC_UNUSED inline void 
_GLIB_AUTOPTR_DESTROY_FUNC_NAME(TypeName) (void *_ptr) 
     \
       |                                    ^
/usr/include/glib-2.0/glib/gmacros.h:1344:51: note: expanded from macro 
'_GLIB_AUTOPTR_DESTROY_FUNC_NAME'
  1344 | #define _GLIB_AUTOPTR_DESTROY_FUNC_NAME(TypeName) 
glib_autoptr_destroy_##TypeName
       |                                                   ^
<scratch space>:51:1: note: expanded from here
    51 | glib_autoptr_destroy_SDHCIState
       | ^
In file included from ../../hw/arm/mcimx6ul-evk.c:15:
In file included from include/hw/arm/fsl-imx6ul.h:31:
include/hw/sd/sdhci.h:165:1: error: redefinition of 
'glib_listautoptr_cleanup_SDHCIState'
   165 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, SYSBUS_SDHCI)
       | ^
include/qom/object.h:238:5: note: expanded from macro 'OBJECT_DECLARE_TYPE'
   238 |     G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \
       |     ^
/usr/include/glib-2.0/glib/gmacros.h:1379:3: note: expanded from macro 
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
  1379 |   _GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS(TypeName, TypeName, func)
       |   ^
/usr/include/glib-2.0/glib/gmacros.h:1366:36: note: expanded from macro 
'_GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS'
  1366 |   static G_GNUC_UNUSED inline void 
_GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName) (GList **_l) 
     \
       |                                    ^
/usr/include/glib-2.0/glib/gmacros.h:1346:48: note: expanded from macro 
'_GLIB_AUTOPTR_LIST_FUNC_NAME'
  1346 | #define _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName) 
glib_listautoptr_cleanup_##TypeName
       |                                                ^
<scratch space>:81:1: note: expanded from here
    81 | glib_listautoptr_cleanup_SDHCIState
       | ^
include/hw/sd/sdhci.h:158:1: note: previous definition is here
   158 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, PCI_SDHCI)
       | ^
include/qom/object.h:238:5: note: expanded from macro 'OBJECT_DECLARE_TYPE'
   238 |     G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \
       |     ^
/usr/include/glib-2.0/glib/gmacros.h:1379:3: note: expanded from macro 
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
  1379 |   _GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS(TypeName, TypeName, func)
       |   ^
/usr/include/glib-2.0/glib/gmacros.h:1366:36: note: expanded from macro 
'_GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS'
  1366 |   static G_GNUC_UNUSED inline void 
_GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName) (GList **_l) 
     \
       |                                    ^
/usr/include/glib-2.0/glib/gmacros.h:1346:48: note: expanded from macro 
'_GLIB_AUTOPTR_LIST_FUNC_NAME'
  1346 | #define _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName) 
glib_listautoptr_cleanup_##TypeName
       |                                                ^
<scratch space>:52:1: note: expanded from here
    52 | glib_listautoptr_cleanup_SDHCIState
       | ^
In file included from ../../hw/arm/mcimx6ul-evk.c:15:
In file included from include/hw/arm/fsl-imx6ul.h:31:
include/hw/sd/sdhci.h:165:1: error: redefinition of 
'glib_slistautoptr_cleanup_SDHCIState'
   165 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, SYSBUS_SDHCI)
       | ^
include/qom/object.h:238:5: note: expanded from macro 'OBJECT_DECLARE_TYPE'
   238 |     G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \
       |     ^
/usr/include/glib-2.0/glib/gmacros.h:1379:3: note: expanded from macro 
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
  1379 |   _GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS(TypeName, TypeName, func)
       |   ^
/usr/include/glib-2.0/glib/gmacros.h:1368:36: note: expanded from macro 
'_GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS'
  1368 |   static G_GNUC_UNUSED inline void 
_GLIB_AUTOPTR_SLIST_FUNC_NAME(TypeName) (GSList **_l) 
     \
       |                                    ^
/usr/include/glib-2.0/glib/gmacros.h:1348:49: note: expanded from macro 
'_GLIB_AUTOPTR_SLIST_FUNC_NAME'
  1348 | #define _GLIB_AUTOPTR_SLIST_FUNC_NAME(TypeName) 
glib_slistautoptr_cleanup_##TypeName
       |                                                 ^
<scratch space>:83:1: note: expanded from here
    83 | glib_slistautoptr_cleanup_SDHCIState
       | ^
include/hw/sd/sdhci.h:158:1: note: previous definition is here
   158 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, PCI_SDHCI)
       | ^
include/qom/object.h:238:5: note: expanded from macro 'OBJECT_DECLARE_TYPE'
   238 |     G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \
       |     ^
/usr/include/glib-2.0/glib/gmacros.h:1379:3: note: expanded from macro 
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
  1379 |   _GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS(TypeName, TypeName, func)
       |   ^
/usr/include/glib-2.0/glib/gmacros.h:1368:36: note: expanded from macro 
'_GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS'
  1368 |   static G_GNUC_UNUSED inline void 
_GLIB_AUTOPTR_SLIST_FUNC_NAME(TypeName) (GSList **_l) 
     \
       |                                    ^
/usr/include/glib-2.0/glib/gmacros.h:1348:49: note: expanded from macro 
'_GLIB_AUTOPTR_SLIST_FUNC_NAME'
  1348 | #define _GLIB_AUTOPTR_SLIST_FUNC_NAME(TypeName) 
glib_slistautoptr_cleanup_##TypeName
       |                                                 ^
<scratch space>:54:1: note: expanded from here
    54 | glib_slistautoptr_cleanup_SDHCIState
       | ^
In file included from ../../hw/arm/mcimx6ul-evk.c:15:
In file included from include/hw/arm/fsl-imx6ul.h:31:
include/hw/sd/sdhci.h:165:1: error: redefinition of 
'glib_queueautoptr_cleanup_SDHCIState'
   165 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, SYSBUS_SDHCI)
       | ^
include/qom/object.h:238:5: note: expanded from macro 'OBJECT_DECLARE_TYPE'
   238 |     G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \
       |     ^
/usr/include/glib-2.0/glib/gmacros.h:1379:3: note: expanded from macro 
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
  1379 |   _GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS(TypeName, TypeName, func)
       |   ^
/usr/include/glib-2.0/glib/gmacros.h:1370:36: note: expanded from macro 
'_GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS'
  1370 |   static G_GNUC_UNUSED inline void 
_GLIB_AUTOPTR_QUEUE_FUNC_NAME(TypeName) (GQueue **_q) 
     \
       |                                    ^
/usr/include/glib-2.0/glib/gmacros.h:1350:49: note: expanded from macro 
'_GLIB_AUTOPTR_QUEUE_FUNC_NAME'
  1350 | #define _GLIB_AUTOPTR_QUEUE_FUNC_NAME(TypeName) 
glib_queueautoptr_cleanup_##TypeName
       |                                                 ^
<scratch space>:85:1: note: expanded from here
    85 | glib_queueautoptr_cleanup_SDHCIState
       | ^
include/hw/sd/sdhci.h:158:1: note: previous definition is here
   158 | OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, PCI_SDHCI)
       | ^
include/qom/object.h:238:5: note: expanded from macro 'OBJECT_DECLARE_TYPE'
   238 |     G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \
       |     ^
/usr/include/glib-2.0/glib/gmacros.h:1379:3: note: expanded from macro 
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
  1379 |   _GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS(TypeName, TypeName, func)
       |   ^
/usr/include/glib-2.0/glib/gmacros.h:1370:36: note: expanded from macro 
'_GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS'
  1370 |   static G_GNUC_UNUSED inline void 
_GLIB_AUTOPTR_QUEUE_FUNC_NAME(TypeName) (GQueue **_q) 
     \
       |                                    ^
/usr/include/glib-2.0/glib/gmacros.h:1350:49: note: expanded from macro 
'_GLIB_AUTOPTR_QUEUE_FUNC_NAME'
  1350 | #define _GLIB_AUTOPTR_QUEUE_FUNC_NAME(TypeName) 
glib_queueautoptr_cleanup_##TypeName
       |                                                 ^
<scratch space>:56:1: note: expanded from here
    56 | glib_queueautoptr_cleanup_SDHCIState
       | ^
6 errors generated.
ninja: build stopped: subcommand failed.

Don't ask me why it is so verbose...

Meanwhile the current legacy macros seems good enough for soft freeze ;)

Re: [PATCH v4 03/14] hw/sd/sdhci: Introduce SDHCIClass stub

[BALATON Zoltan]

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

On Sun, 9 Mar 2025, Philippe Mathieu-Daudé wrote:
> On 8/3/25 23:34, BALATON Zoltan wrote:
>> On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
>>> TYPE_SYSBUS_SDHCI is a bit odd because it uses an union
>>> to work with both SysBus / PCI parent. As this is not a
>>> normal use, introduce SDHCIClass in its own commit.
>>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> include/hw/sd/sdhci.h | 9 +++++++++
>>> hw/sd/sdhci.c         | 1 +
>>> 2 files changed, 10 insertions(+)
>>> 
>>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>>> index 48247e9a20f..c4b20db3877 100644
>>> --- a/include/hw/sd/sdhci.h
>>> +++ b/include/hw/sd/sdhci.h
>>> @@ -107,6 +107,13 @@ struct SDHCIState {
>>> };
>>> typedef struct SDHCIState SDHCIState;
>>> 
>>> +typedef struct SDHCIClass {
>>> +    union {
>>> +        PCIDeviceClass pci_parent_class;
>>> +        SysBusDeviceClass sbd_parent_class;
>>> +    };
>>> +} SDHCIClass;
>>> +
>>> /*
>>>  * Controller does not provide transfer-complete interrupt when not
>>>  * busy.
>>> @@ -123,6 +130,8 @@ DECLARE_INSTANCE_CHECKER(SDHCIState, PCI_SDHCI,
>>> #define TYPE_SYSBUS_SDHCI "generic-sdhci"
>>> DECLARE_INSTANCE_CHECKER(SDHCIState, SYSBUS_SDHCI,
>>>                          TYPE_SYSBUS_SDHCI)
>>> +DECLARE_CLASS_CHECKERS(SDHCIClass, SYSBUS_SDHCI,
>>> +                       TYPE_SYSBUS_SDHCI)
>> 
>> Are these two together just OBJECT_DECLARE_TYPE? Then the above typedefs 
>> are also not needed just the struct definitions.
>
> I'd like to but it isn't possible because the same object state/class is
> used by distinct types (PCI & SysBus).
>
> The following (expected to be correct) change ...:
> -- >8 --
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 966a1751f50..341b130995b 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -155,10 +155,6 @@ typedef struct SDHCIClass {
> #define TYPE_PCI_SDHCI "sdhci-pci"
> -DECLARE_INSTANCE_CHECKER(SDHCIState, PCI_SDHCI,
> -                         TYPE_PCI_SDHCI)
> +OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, PCI_SDHCI)

It would be same as the original patch if you omit this one and only use 
OBJECT_DECLARE_TYPE below. You didn't add CLASS_CHECKER to the PCI version 
in the original patch either. But I see now it's more complex and so maybe 
it's not so easy.

> #define TYPE_SYSBUS_SDHCI "generic-sdhci"
> -DECLARE_INSTANCE_CHECKER(SDHCIState, SYSBUS_SDHCI,
> -                         TYPE_SYSBUS_SDHCI)
> -DECLARE_CLASS_CHECKERS(SDHCIClass, SYSBUS_SDHCI,
> -                       TYPE_SYSBUS_SDHCI)
> +OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, SYSBUS_SDHCI)
> ---

[...]

> Meanwhile the current legacy macros seems good enough for soft freeze ;)

Good enough for me. I was also happy with my way simpler solution. This is 
much more clean up already.

Regards,
BALATON Zoltan

Re: [PATCH v4 14/14] hw/ppc/e500: Replace generic SDHCI by Freescale eSDHC

[Bernhard Beschow]

Am 8. März 2025 21:36:40 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>As Zoltan reported, some U-Boot versions seem to expect
>correctly initialized registers 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>
>---
> hw/ppc/e500.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
>diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>index e85e000f054..7d15c926887 100644
>--- a/hw/ppc/e500.c
>+++ b/hw/ppc/e500.c
>@@ -1027,20 +1027,12 @@ void ppce500_init(MachineState *machine)
> 
>     /* eSDHC */
>     if (pmc->has_esdhc) {
>-        dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);

"hw/misc/unimp.h" include is now unused and should therefore be removed.

With that fixed:
Reviewed-by: Bernhard Beschow <shentey@gmail.com>

>-        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_enum(dev, "endianness", ENDIAN_MODE_BIG);
>         s = SYS_BUS_DEVICE(dev);

Re: [PATCH v4 09/14] hw/sd/sdhci: Unify default MemoryRegionOps

[Bernhard Beschow]

Am 8. März 2025 21:36:35 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Note, sdhci_mmio_le_ops[] was missing .impl.access_size = 4.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/sd/sdhci.c | 46 ++++++++++++++++++++--------------------------
> 1 file changed, 20 insertions(+), 26 deletions(-)
>
>diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>index a2e7162e289..23af3958a1d 100644
>--- a/hw/sd/sdhci.c
>+++ b/hw/sd/sdhci.c
>@@ -1372,30 +1372,22 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>                        value >> shift, value >> shift);
> }
> 
>-static const MemoryRegionOps sdhci_mmio_le_ops = {
>-    .read = sdhci_read,
>-    .write = sdhci_write,
>-    .valid = {
>-        .min_access_size = 1,
>-        .max_access_size = 4,
>-        .unaligned = false
>+static const MemoryRegionOps sdhci_mmio_ops[2] = {
>+    [0 ... 1] = {
>+        .read = sdhci_read,
>+        .write = sdhci_write,
>+        .impl = {
>+            .min_access_size = 4,
>+            .max_access_size = 4,
>+        },
>+        .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 = {
>-        .min_access_size = 4,
>-        .max_access_size = 4,
>-    },
>-    .valid = {
>-        .min_access_size = 1,
>-        .max_access_size = 4,
>-        .unaligned = false
>-    },
>-    .endianness = DEVICE_BIG_ENDIAN,
>+    [0].endianness = DEVICE_LITTLE_ENDIAN,
>+    [1].endianness = DEVICE_BIG_ENDIAN,
> };

We introduced sdhci_mmio_be_ops and the endianness property specifically for the e500 machine. We always lied about the endianness property to be implemented for all SDHC types, e.g. USDHC doesn't implement it. Since you'll introduce a dedicated FSL_ESDHC type a few commits later, along with its own MemoryRegionOps, I think we should drop the endianness property and sdhci_mmio_ops doesn't need to become an array. Dropping the endianness property also results in avoiding the error conditions you have to deal with in the realize method.

Best regards,
Bernhard

P.S.: IIRC, the Layerscape SoCs also use the fsl,esdhc IP core and their endianness can be switched at runtime(!). Since we don't model Layerscape we can simplify, i.e. hardcode the endianness to big endian for now.

> 
> static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
>@@ -1443,10 +1435,12 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>     ERRP_GUARD();
>     SDHCIClass *sc = SYSBUS_SDHCI_GET_CLASS(s);
>     const char *class_name = object_get_typename(OBJECT(s));
>+    unsigned ops_index;
> 
>-    s->io_ops = sc->io_ops ?: (s->endianness == DEVICE_BIG_ENDIAN ?
>-                               &sdhci_mmio_be_ops : &sdhci_mmio_le_ops);
>-    if (s->io_ops->endianness != s->endianness) {
>+    ops_index = s->endianness == DEVICE_BIG_ENDIAN ? 1 : 0;
>+
>+    s->io_ops = sc->io_ops ?: &sdhci_mmio_ops[ops_index];
>+    if (s->io_ops->endianness != sdhci_mmio_ops[ops_index].endianness) {
>         error_setg(errp, "Invalid endianness for SD controller");
>         return;
>     }

Re: [PATCH v4 13/14] hw/sd/sdhci: Implement Freescale eSDHC as TYPE_FSL_ESDHC

[BALATON Zoltan]

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

On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
> 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         | 37 ++++++++++++++++++++++++++++++++++++-
> 2 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index eb8380187b5..966a1751f50 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -162,6 +162,8 @@ DECLARE_INSTANCE_CHECKER(SDHCIState, SYSBUS_SDHCI,
> DECLARE_CLASS_CHECKERS(SDHCIClass, SYSBUS_SDHCI,
>                        TYPE_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 234a6e4a1fe..d5cc0bf1458 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1653,7 +1653,37 @@ 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 = {
> +        .min_access_size = 4,

Does this need max access too? Maybe it could work with 8 and likely 
nothing would try to access more than 4 so probably does not matter.

Regards,
BALATON Zoltan

> +        .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
>
> @@ -1983,6 +2013,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,
>

Re: [PATCH v4 05/14] hw/sd/sdhci: Make I/O region size a class property

[BALATON Zoltan]

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

On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
> 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>

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

[BALATON Zoltan]

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

On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
> 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>

Re: [PATCH v4 12/14] hw/sd/sdhci: Allow SDHCI classes to have different register reset values

[BALATON Zoltan]

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

On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
> 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>

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

[BALATON Zoltan]

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

On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
> All TYPE_IMX_USDHC instances use the quirk:
> move it to the class layer.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> include/hw/sd/sdhci.h |  3 ++-
> hw/sd/sdhci.c         | 15 +++++++++++++--
> 2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index c4b20db3877..0616ce3aa59 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -95,7 +95,6 @@ struct SDHCIState {
>
>     /* Configurable properties */
>     bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
> -    uint32_t quirks;
>     uint8_t endianness;
>     uint8_t sd_spec_version;
>     uint8_t uhs_mode;
> @@ -112,6 +111,8 @@ typedef struct SDHCIClass {
>         PCIDeviceClass pci_parent_class;
>         SysBusDeviceClass sbd_parent_class;
>     };
> +
> +    uint32_t quirks;
> } SDHCIClass;
>
> /*
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 4917a9b3632..2b7eb11a14a 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -345,6 +345,8 @@ static void sdhci_send_command(SDHCIState *s)
>     rlen = sdbus_do_command(&s->sdbus, &request, response);
>
>     if (s->cmdreg & SDHC_CMD_RESPONSE) {
> +        SDHCIClass *sc = SYSBUS_SDHCI_GET_CLASS(s);

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

Regards,
BALATON Zoltan

> +
>         if (rlen == 4) {
>             s->rspreg[0] = ldl_be_p(response);
>             s->rspreg[1] = s->rspreg[2] = s->rspreg[3] = 0;
> @@ -366,7 +368,7 @@ static void sdhci_send_command(SDHCIState *s)
>             }
>         }
>
> -        if (!(s->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
> +        if (!(sc->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
>             (s->norintstsen & SDHC_NISEN_TRSCMP) &&
>             (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
>             s->norintsts |= SDHC_NIS_TRSCMP;
> @@ -1886,7 +1888,15 @@ static void imx_usdhc_init(Object *obj)
>     SDHCIState *s = SYSBUS_SDHCI(obj);
>
>     s->io_ops = &usdhc_mmio_ops;
> -    s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
> +}
> +
> +static void imx_usdhc_class_init(ObjectClass *oc, void *data)
> +{
> +    SDHCIClass *sc = SYSBUS_SDHCI_CLASS(oc);
> +
> +    sc->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
> +
> +    sdhci_common_class_init(oc, data);
> }
>
> /* --- qdev Samsung s3c --- */
> @@ -1967,6 +1977,7 @@ static const TypeInfo sdhci_types[] = {
>         .name = TYPE_IMX_USDHC,
>         .parent = TYPE_SYSBUS_SDHCI,
>         .instance_init = imx_usdhc_init,
> +        .class_init = imx_usdhc_class_init,
>     },
>     {
>         .name = TYPE_S3C_SDHCI,
>

Re: [PATCH v4 13/14] hw/sd/sdhci: Implement Freescale eSDHC as TYPE_FSL_ESDHC

[Philippe Mathieu-Daudé]

On 9/3/25 12:04, BALATON Zoltan wrote:
> On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
>> 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         | 37 ++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 38 insertions(+), 1 deletion(-)


>> -/* --- 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 = {
>> +        .min_access_size = 4,
> 
> Does this need max access too? Maybe it could work with 8 and likely 
> nothing would try to access more than 4 so probably does not matter.

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.

There is no precision on max_access_size.

> Regards,
> BALATON Zoltan

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

[Philippe Mathieu-Daudé]

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

I thought about it and was expecting a such comment.

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

> Regards,
> BALATON Zoltan

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

[BALATON Zoltan]

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

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

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

Regards,
BALATON Zoltan

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

[Bernhard Beschow]

Am 8. März 2025 21:36:28 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>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>
>---
> 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;
> 

Reviewed-by: Bernhard Beschow <shentey@gmail.com>

Re: [PATCH v4 14/14] hw/ppc/e500: Replace generic SDHCI by Freescale eSDHC

[Philippe Mathieu-Daudé]

On 9/3/25 09:31, Bernhard Beschow wrote:
> 
> 
> Am 8. März 2025 21:36:40 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> As Zoltan reported, some U-Boot versions seem to expect
>> correctly initialized registers 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>
>> ---
>> hw/ppc/e500.c | 10 +---------
>> 1 file changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index e85e000f054..7d15c926887 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -1027,20 +1027,12 @@ void ppce500_init(MachineState *machine)
>>
>>      /* eSDHC */
>>      if (pmc->has_esdhc) {
>> -        dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
> 
> "hw/misc/unimp.h" include is now unused and should therefore be removed.

Good catch.

> 
> With that fixed:
> Reviewed-by: Bernhard Beschow <shentey@gmail.com>

Thanks!

Re: [PATCH v4 03/14] hw/sd/sdhci: Introduce SDHCIClass stub

[Philippe Mathieu-Daudé]

On 9/3/25 01:08, BALATON Zoltan wrote:
> On Sun, 9 Mar 2025, Philippe Mathieu-Daudé wrote:
>> On 8/3/25 23:34, BALATON Zoltan wrote:
>>> On Sat, 8 Mar 2025, Philippe Mathieu-Daudé wrote:
>>>> TYPE_SYSBUS_SDHCI is a bit odd because it uses an union
>>>> to work with both SysBus / PCI parent. As this is not a
>>>> normal use, introduce SDHCIClass in its own commit.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>> include/hw/sd/sdhci.h | 9 +++++++++
>>>> hw/sd/sdhci.c         | 1 +
>>>> 2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>>>> index 48247e9a20f..c4b20db3877 100644
>>>> --- a/include/hw/sd/sdhci.h
>>>> +++ b/include/hw/sd/sdhci.h
>>>> @@ -107,6 +107,13 @@ struct SDHCIState {
>>>> };
>>>> typedef struct SDHCIState SDHCIState;
>>>>
>>>> +typedef struct SDHCIClass {
>>>> +    union {
>>>> +        PCIDeviceClass pci_parent_class;
>>>> +        SysBusDeviceClass sbd_parent_class;
>>>> +    };
>>>> +} SDHCIClass;
>>>> +
>>>> /*
>>>>  * Controller does not provide transfer-complete interrupt when not
>>>>  * busy.
>>>> @@ -123,6 +130,8 @@ DECLARE_INSTANCE_CHECKER(SDHCIState, PCI_SDHCI,
>>>> #define TYPE_SYSBUS_SDHCI "generic-sdhci"
>>>> DECLARE_INSTANCE_CHECKER(SDHCIState, SYSBUS_SDHCI,
>>>>                          TYPE_SYSBUS_SDHCI)
>>>> +DECLARE_CLASS_CHECKERS(SDHCIClass, SYSBUS_SDHCI,
>>>> +                       TYPE_SYSBUS_SDHCI)
>>>
>>> Are these two together just OBJECT_DECLARE_TYPE? Then the above 
>>> typedefs are also not needed just the struct definitions.
>>
>> I'd like to but it isn't possible because the same object state/class is
>> used by distinct types (PCI & SysBus).
>>
>> The following (expected to be correct) change ...:
>> -- >8 --
>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>> index 966a1751f50..341b130995b 100644
>> --- a/include/hw/sd/sdhci.h
>> +++ b/include/hw/sd/sdhci.h
>> @@ -155,10 +155,6 @@ typedef struct SDHCIClass {
>> #define TYPE_PCI_SDHCI "sdhci-pci"
>> -DECLARE_INSTANCE_CHECKER(SDHCIState, PCI_SDHCI,
>> -                         TYPE_PCI_SDHCI)
>> +OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, PCI_SDHCI)
> 
> It would be same as the original patch if you omit this one and only use 
> OBJECT_DECLARE_TYPE below.

Indeed that works. Kind of a kludge. Not worth than what we have, so
I'll take it.

> You didn't add CLASS_CHECKER to the PCI 
> version in the original patch either. But I see now it's more complex 
> and so maybe it's not so easy.

Yeah, I'll postpone the QOM parentship cleanup for later.

> 
>> #define TYPE_SYSBUS_SDHCI "generic-sdhci"
>> -DECLARE_INSTANCE_CHECKER(SDHCIState, SYSBUS_SDHCI,
>> -                         TYPE_SYSBUS_SDHCI)
>> -DECLARE_CLASS_CHECKERS(SDHCIClass, SYSBUS_SDHCI,
>> -                       TYPE_SYSBUS_SDHCI)
>> +OBJECT_DECLARE_TYPE(SDHCIState, SDHCIClass, SYSBUS_SDHCI)
>> ---
> 
> [...]
> 
>> Meanwhile the current legacy macros seems good enough for soft freeze ;)
> 
> Good enough for me. I was also happy with my way simpler solution. This 
> is much more clean up already.
> 
> Regards,
> BALATON Zoltan

Re: [PATCH v4 09/14] hw/sd/sdhci: Unify default MemoryRegionOps

[Philippe Mathieu-Daudé]

On 9/3/25 10:27, Bernhard Beschow wrote:
> 
> 
> Am 8. März 2025 21:36:35 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> Note, sdhci_mmio_le_ops[] was missing .impl.access_size = 4.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/sd/sdhci.c | 46 ++++++++++++++++++++--------------------------
>> 1 file changed, 20 insertions(+), 26 deletions(-)
>>
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index a2e7162e289..23af3958a1d 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -1372,30 +1372,22 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>>                         value >> shift, value >> shift);
>> }
>>
>> -static const MemoryRegionOps sdhci_mmio_le_ops = {
>> -    .read = sdhci_read,
>> -    .write = sdhci_write,
>> -    .valid = {
>> -        .min_access_size = 1,
>> -        .max_access_size = 4,
>> -        .unaligned = false
>> +static const MemoryRegionOps sdhci_mmio_ops[2] = {
>> +    [0 ... 1] = {
>> +        .read = sdhci_read,
>> +        .write = sdhci_write,
>> +        .impl = {
>> +            .min_access_size = 4,
>> +            .max_access_size = 4,
>> +        },
>> +        .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 = {
>> -        .min_access_size = 4,
>> -        .max_access_size = 4,
>> -    },
>> -    .valid = {
>> -        .min_access_size = 1,
>> -        .max_access_size = 4,
>> -        .unaligned = false
>> -    },
>> -    .endianness = DEVICE_BIG_ENDIAN,
>> +    [0].endianness = DEVICE_LITTLE_ENDIAN,
>> +    [1].endianness = DEVICE_BIG_ENDIAN,
>> };
> 
> We introduced sdhci_mmio_be_ops and the endianness property specifically for the e500 machine. We always lied about the endianness property to be implemented for all SDHC types, e.g. USDHC doesn't implement it. Since you'll introduce a dedicated FSL_ESDHC type a few commits later, along with its own MemoryRegionOps, I think we should drop the endianness property and sdhci_mmio_ops doesn't need to become an array. Dropping the endianness property also results in avoiding the error conditions you have to deal with in the realize method.

Great idea!