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

[Bernhard Beschow <shentey@gmail.com>]

Am 31. Oktober 2022 12:11:37 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 30/10/22 12:46, Bernhard Beschow wrote:
>> 
>> 
>> 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?
>
>See https://lore.kernel.org/qemu-devel/20221031115402.91912-7-philmd@linaro.org/
>
>>     Can you share branch and how to test?
>> 
>> 
>> QEMU branch: https://github.com/shentok/qemu/tree/e500-flash <https://github.com/shentok/qemu/tree/e500-flash>
>> 
>> How to test:
>> 1. `git clone -b e500 https://github.com/shentok/buildroot.git <https://github.com/shentok/buildroot.git>`
>> 2. `cd buildroot`
>> 3. `make qemu_ppc_e500mc_defconfig`
>> 4. `make`
>> 5. `cd output/images`
>> 6. `dd if=/dev/zero of=root.img bs=1M count=64 && dd if=rootfs.ext2 of=root.img bs=1M conv=notrunc`
>> 7. `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append "console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive id=mydrive,if=none,file=root.img,format=raw`
>
>Could you add an Avocado-based test?

I could give it a try at least. Where would I drop the binaries?

Best regards,
Bernhard
>
>>    Welcome to Buildroot
>>    buildroot login:
>
>Regards,
>
>Phil.