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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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?