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

["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?

>    Welcome to Buildroot
>    buildroot login:

Regards,

Phil.