Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow <shen...@gmail.com>:
>Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe Mathieu-Daudé"
><phi...@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 <shen...@gmail.com>
>>> ---
>>> hw/sd/sdhci.c | 120 +++++++++++++++++++++++++++++++++++++++++-
>>> include/hw/sd/sdhci.h | 3 ++
>>> 2 files changed, 122 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index 306070c872..8d8ad9ff24 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -1369,6 +1369,7 @@ void sdhci_initfn(SDHCIState *s)
>>> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>>> sdhci_data_transfer, s);
>>> s->io_ops = &sdhci_mmio_ops;
>>> + s->io_registers_map_size = SDHC_REGISTERS_MAP_SIZE;
>>> }
>>> void sdhci_uninitfn(SDHCIState *s)
>>> @@ -1392,7 +1393,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>>> s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>> memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
>>> - SDHC_REGISTERS_MAP_SIZE);
>>> + s->io_registers_map_size);
>>
>>I don't think we want to change this region size. [see below]
>>
>>> void sdhci_common_unrealize(SDHCIState *s)
>>> @@ -1575,6 +1576,122 @@ static const TypeInfo sdhci_bus_info = {
>>> .class_init = sdhci_bus_class_init,
>>> };
>>> +/* --- qdev Freescale eSDHC --- */
>>> +
>>> +/* Watermark Level Register */
>>> +#define ESDHC_WML 0x44
>>> +
>>> +/* Control Register for DMA transfer */
>>> +#define ESDHC_DMA_SYSCTL 0x40c
>>> +
>>> +#define ESDHC_REGISTERS_MAP_SIZE 0x410
>>
>>My preferred approach would be to create a container region with a
>>size of ESDHC_REGISTERS_MAP_SIZE. Map the SDHC_REGISTERS_MAP region
>>in the container at offset 0, priority -1. Add 2 register regions
>>for ESDHC_WML and ESDHC_DMA_SYSCTL, and map them with priority 1 in
>>the container. ...
>>
>>> +static uint64_t esdhci_read(void *opaque, hwaddr offset, unsigned size)
>>> +{
>>> + uint64_t ret;
>>> +
>>> + switch (offset) {
>>> + case SDHC_SYSAD:
>>> + case SDHC_BLKSIZE:
>>> + case SDHC_ARGUMENT:
>>> + case SDHC_TRNMOD:
>>> + case SDHC_RSPREG0:
>>> + case SDHC_RSPREG1:
>>> + case SDHC_RSPREG2:
>>> + case SDHC_RSPREG3:
>>> + case SDHC_BDATA:
>>> + case SDHC_PRNSTS:
>>> + case SDHC_HOSTCTL:
>>> + case SDHC_CLKCON:
>>> + case SDHC_NORINTSTS:
>>> + case SDHC_NORINTSTSEN:
>>> + case SDHC_NORINTSIGEN:
>>> + case SDHC_ACMD12ERRSTS:
>>> + case SDHC_CAPAB:
>>> + case SDHC_SLOT_INT_STATUS:
>>> + ret = sdhci_read(opaque, offset, size);
>>> + break;
>>
>>... Then you don't need these cases.
>>
>>> + case ESDHC_WML:
>>> + case ESDHC_DMA_SYSCTL:
>>> + ret = 0;
>>> + qemu_log_mask(LOG_UNIMP, "ESDHC rd @0x%02" HWADDR_PRIx
>>> + " not implemented\n", offset);
>>
>>But then I realize you only treat these 2 registers as UNIMP.
>>
>>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.
Best regards,
Bernhard
>
>Best regards,
>Bernhard
>>
>>Regards,
>>
>>Phil.
>
