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

[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(-)
>>> 
>>> 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.
>