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

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

Remove the code related to SDHCIState::endianess field.

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

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

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

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

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

Best regards,
Bernhard

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

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

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

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

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

Regards,
BALATON Zoltan

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

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

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

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

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

Guenter

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

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

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

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

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

Guenter

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

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

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

we can truncate to 64M the rootfs used in  :

    tests/functional/test_ppc64_e500.py

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


Thanks,

C.

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

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

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

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

Regards,

Phil.