On Sun, Oct 30, 2022 at 1:10 AM Philippe Mathieu-Daudé
<philmd@linaro.org <mailto:philmd@linaro.org>> wrote:
On 29/10/22 20:28, Bernhard Beschow wrote:
> Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow
<shentey@gmail.com <mailto:shentey@gmail.com>>:
>> Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow
<shentey@gmail.com <mailto:shentey@gmail.com>>:
>>> Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe
Mathieu-Daudé" <philmd@linaro.org <mailto: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
<mailto: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?