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

[Bernhard Beschow]

Am 1. November 2022 10:49:20 UTC schrieb 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?

Looks like Cederic already had a repo [1]. Let's see what I can do.

Best regards,
Bernhard

[1] https://github.com/legoater/qemu-ppc-boot/tree/main/buildroot

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