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

[Cédric Le Goater]

On 3/10/25 18:38, Bernhard Beschow wrote:

Am 10. März 2025 17:31:57 UTC schrieb "Philippe Mathieu-Daudé" 
<phi...@linaro.org>:

On 10/3/25 16:56, Guenter Roeck wrote:

On 3/10/25 08:27, Philippe Mathieu-Daudé wrote:

On 10/3/25 15:09, BALATON Zoltan wrote:

On Mon, 10 Mar 2025, Philippe Mathieu-Daudé wrote:

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

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

Remove the code related to SDHCIState::endianess field.

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

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

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index d99a8493db2..e4da6c831d1 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -308,7 +308,6 @@ extern const VMStateDescription sdhci_vmstate;
#define SDHC_CAPAB_REG_DEFAULT 0x057834b4

#define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
-    DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), 
\
     DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
     DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
     \
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index e8fced5eedc..1016a5b5b77 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -54,7 +54,6 @@ struct SDHCIState {
     AddressSpace sysbus_dma_as;
     AddressSpace *dma_as;
     MemoryRegion *dma_mr;
-    const MemoryRegionOps *io_ops;

     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
     QEMUTimer *transfer_timer;
@@ -105,7 +104,6 @@ struct SDHCIState {

     /* Configurable properties */
     uint32_t quirks;
-    uint8_t endianness;
     uint8_t sd_spec_version;
     uint8_t uhs_mode;
};
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 47e4bd1a610..cbb9f4ae8c0 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1391,17 +1391,6 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
}

static const MemoryRegionOps sdhci_mmio_le_ops = {
-    .read = sdhci_read,
-    .write = sdhci_write,
-    .valid = {
-        .min_access_size = 1,
-        .max_access_size = 4,
-        .unaligned = false
-    },
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
-static const MemoryRegionOps sdhci_mmio_be_ops = {
     .read = sdhci_read,
     .write = sdhci_write,
     .impl = {
@@ -1413,7 +1402,7 @@ static const MemoryRegionOps sdhci_mmio_be_ops = {
         .max_access_size = 4,
         .unaligned = false
     },
-    .endianness = DEVICE_BIG_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
};

static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
@@ -1467,23 +1456,6 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
     SDHCIClass *sc = s->sc;
     const char *class_name = object_get_typename(OBJECT(s));

-    s->io_ops = sc->io_ops ?: &sdhci_mmio_le_ops;
-    switch (s->endianness) {
-    case DEVICE_LITTLE_ENDIAN:
-        /* s->io_ops is little endian by default */
-        break;
-    case DEVICE_BIG_ENDIAN:
-        if (s->io_ops != &sdhci_mmio_le_ops) {
-            error_setg(errp, "SD controller doesn't support big endianness");
-            return;
-        }
-        s->io_ops = &sdhci_mmio_be_ops;
-        break;
-    default:
-        error_setg(errp, "Incorrect endianness");
-        return;
-    }
-
     sdhci_init_readonly_registers(s, errp);
     if (*errp) {
         return;
@@ -1493,7 +1465,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
     s->fifo_buffer = g_malloc0(s->buf_maxsz);

     assert(sc->iomem_size >= SDHC_REGISTERS_MAP_SIZE);
-    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, class_name,
+    memory_region_init_io(&s->iomem, OBJECT(s), sc->io_ops, s, class_name,
                           sc->iomem_size);
}

@@ -1578,6 +1550,7 @@ void sdhci_common_class_init(ObjectClass *klass, const 
void *data)
     dc->vmsd = &sdhci_vmstate;
     device_class_set_legacy_reset(dc, sdhci_poweron_reset);

+    sc->io_ops = &sdhci_mmio_le_ops;

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

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

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

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

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

we can truncate to 64M the rootfs used in  :

   tests/functional/test_ppc64_e500.py

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


Thanks,

C.