diff mbox series

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

Message ID 20250308213640.13138-10-philmd@linaro.org
State New
Headers show
Series hw/sd/sdhci: Set reset value of interrupt registers | expand

Commit Message

Philippe Mathieu-Daudé March 8, 2025, 9:36 p.m. UTC 
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(-)

Comments

Bernhard Beschow March 9, 2025, 9:27 a.m. UTC | #1 
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;
>     }
diff mbox series

Patch

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;
     }