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

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

Regards,

Phil.