Re: [PATCH] hw/sd/sdhci: Do not force sdhci_mmio_*_ops onto all SD controllers

[Bernhard Beschow]

Am 10. Juli 2023 10:16:35 UTC schrieb "Philippe Mathieu-Daudé" 
<philmd@linaro.org>:
>On 9/7/23 10:09, Bernhard Beschow wrote:
>> Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host controller
>> interfaces" sdhci_common_realize() forces all SD card controllers to use 
>> either
>> sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness" 
>> property.
>> However, there are device models which use different MMIO ops: TYPE_IMX_USDHC
>> uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops.
>> 
>> Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board, 
>> for
>> example. Fix this by defaulting the io_ops to little endian and switch to big
>> endian in sdhci_common_realize() only if there is a matchig big endian 
>> variant
>> available.
>> 
>> Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller
>> interfaces")
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/sd/sdhci.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 6811f0f1a8..362c2c86aa 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s)
>>         s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, 
>> sdhci_raise_insertion_irq, s);
>>       s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, 
>> sdhci_data_transfer, s);
>> +
>> +    s->io_ops = &sdhci_mmio_le_ops;
>>   }
>>     void sdhci_uninitfn(SDHCIState *s)
>> @@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>>   
>
>What about simply keeping the same code guarded with 'if (!s->io_ops)'?

I chose below approach since it provides an error message when one attempts to 
set one of the other device models to BE rather than just silently ignoring it.

Also, I didn't want to make the assumption that `s->io_ops == NULL` implied 
that sdhci_mmio_*_ops is needed. That's similar material the bug fixed is made 
of, so I wanted to prevent that in the first place by being more explicit.

In combination with the new error message the limitations of the current code 
become hopefully very apparent now, and at the same time should provide enough 
hints for adding BE support to the other device models if ever needed.

Best regards,
Bernhard

>
>>       switch (s->endianness) {
>>       case DEVICE_LITTLE_ENDIAN:
>> -        s->io_ops = &sdhci_mmio_le_ops;
>> +        /* 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:
>