qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model
Date: Sun, 30 Oct 2022 01:10:30 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.4.0

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?



reply via email to

[Prev in Thread] Current Thread [Next in Thread]