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?
