Re: [PATCH v4 09/14] hw/sd/sdhci: Unify default MemoryRegionOps

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

On 9/3/25 10:27, Bernhard Beschow wrote:
> 
> 
> Am 8. März 2025 21:36:35 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> Note, sdhci_mmio_le_ops[] was missing .impl.access_size = 4.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/sd/sdhci.c | 46 ++++++++++++++++++++--------------------------
>> 1 file changed, 20 insertions(+), 26 deletions(-)
>>
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index a2e7162e289..23af3958a1d 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -1372,30 +1372,22 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>>                         value >> shift, value >> shift);
>> }
>>
>> -static const MemoryRegionOps sdhci_mmio_le_ops = {
>> -    .read = sdhci_read,
>> -    .write = sdhci_write,
>> -    .valid = {
>> -        .min_access_size = 1,
>> -        .max_access_size = 4,
>> -        .unaligned = false
>> +static const MemoryRegionOps sdhci_mmio_ops[2] = {
>> +    [0 ... 1] = {
>> +        .read = sdhci_read,
>> +        .write = sdhci_write,
>> +        .impl = {
>> +            .min_access_size = 4,
>> +            .max_access_size = 4,
>> +        },
>> +        .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 = {
>> -        .min_access_size = 4,
>> -        .max_access_size = 4,
>> -    },
>> -    .valid = {
>> -        .min_access_size = 1,
>> -        .max_access_size = 4,
>> -        .unaligned = false
>> -    },
>> -    .endianness = DEVICE_BIG_ENDIAN,
>> +    [0].endianness = DEVICE_LITTLE_ENDIAN,
>> +    [1].endianness = DEVICE_BIG_ENDIAN,
>> };
> 
> We introduced sdhci_mmio_be_ops and the endianness property specifically for the e500 machine. We always lied about the endianness property to be implemented for all SDHC types, e.g. USDHC doesn't implement it. Since you'll introduce a dedicated FSL_ESDHC type a few commits later, along with its own MemoryRegionOps, I think we should drop the endianness property and sdhci_mmio_ops doesn't need to become an array. Dropping the endianness property also results in avoiding the error conditions you have to deal with in the realize method.

Great idea!