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

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

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