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

[Bernhard Beschow]

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

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?

Best regards,
Bernhard
>
>Best regards,
>Bernhard
>>
>>Best regards,
>>Bernhard
>>>
>>>Regards,
>>>
>>>Phil.
>>
>