Re: [PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model

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

On 29/10/22 20:28, Bernhard Beschow wrote:
> Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>> Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>> Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>>> Hi Bernhard,
>>>>
>>>> On 18/10/22 23:01, Bernhard Beschow wrote:
>>>>> Will allow e500 boards to access SD cards using just their own devices.
>>>>>
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>> ---
>>>>>    hw/sd/sdhci.c         | 120 +++++++++++++++++++++++++++++++++++++++++-
>>>>>    include/hw/sd/sdhci.h |   3 ++
>>>>>    2 files changed, 122 insertions(+), 1 deletion(-)

>>>> So now, I'd create 1 UNIMP region for ESDHC_WML and map it
>>>> into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add
>>>> another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset
>>>> 0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in
>>>> hw/arm/bcm2835_peripherals.c.
>>>>
>>>> But the ESDHC_WML register has address 0x44 and fits inside the
>>>> SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the
>>>> upper part of the SDHC_CAPAB register. These bits are undefined
>>>> on the spec v2, which I see you are setting in esdhci_init().
>>>> So this register should already return 0, otherwise we have
>>>> a bug. Thus we don't need to handle this ESDHC_WML particularly.
>>
>> My idea here was to catch this unimplemented case in order to indicate this clearly to users. Perhaps it nudges somebody to provide a patch?
>>
>>>>
>>>> And your model is reduced to handling create_unimp() in esdhci_realize().
>>>>
>>>> Am I missing something?
>>>
>>> The mmio ops are big endian and need to be aligned to a 4-byte boundary. It took me quite a while to debug this. So shall I just create an additional memory region for the region above SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL?
>>
>> All in all I currently don't have a better idea than keeping the custom i/o ops for the standard region and adding an additional unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to dynamically allocate memory for it where I still need to figure out how not to leak it.
> 
> By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I was able to remove the custom implementations while having big endian and the alignments proper. However, I don't see a way of adding two memory regions - with or without a container. With a container I'd have to somehow preserve the mmio attribute which is initialized by the parent class, re-initialize it with the container, and add the preserved memory region as child. This seems very fragile, esp. since the parent class has created an alias for mmio in sysbus. Without a container, one would have two memory regions that both have to be mapped separately by the caller, i.e. it burdens the caller with an implementation detail.
> 
> Any suggestions?

Can you share branch and how to test?