Re: [Qemu-devel] [PATCH v3 11/30] sdhci: Add i.MX specific subtype of SDHCI

[Peter Maydell]

On 6 November 2017 at 15:47, Andrey Smirnov <address@hidden> wrote:
> IP block found on several generations of i.MX family does not use
> vanilla SDHCI implementation and it comes with a number of quirks.
>
> Introduce i.MX SDHCI subtype of SDHCI block to add code necessary to
> support unmodified Linux guest driver.
>
> Cc: Peter Maydell <address@hidden>
> Cc: Jason Wang <address@hidden>
> Cc: Philippe Mathieu-Daudé <address@hidden>
> Cc: address@hidden
> Cc: address@hidden
> Cc: address@hidden
> Signed-off-by: Andrey Smirnov <address@hidden>

Hi. Mostly this looks ok; some comments below.

> ---
>  hw/sd/sdhci-internal.h |  15 ++++++
>  hw/sd/sdhci.c          | 127 
> ++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/hw/sd/sdhci.h  |   8 ++++
>  3 files changed, 148 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> index 161177cf39..2a1b4b06ee 100644
> --- a/hw/sd/sdhci-internal.h
> +++ b/hw/sd/sdhci-internal.h
> @@ -91,6 +91,8 @@
>  #define SDHC_CTRL_ADMA2_32             0x10
>  #define SDHC_CTRL_ADMA2_64             0x18
>  #define SDHC_DMA_TYPE(x)               ((x) & SDHC_CTRL_DMA_CHECK_MASK)
> +#define SDHC_CTRL_4BITBUS              0x02
> +#define SDHC_CTRL_8BITBUS              0x20
>
>  /* R/W Power Control Register 0x0 */
>  #define SDHC_PWRCON                    0x29
> @@ -229,4 +231,17 @@ enum {
>
>  extern const VMStateDescription sdhci_vmstate;
>
> +
> +#define ESDHC_MIX_CTRL                  0x48
> +#define ESDHC_VENDOR_SPEC               0xc0
> +#define ESDHC_DLL_CTRL                  0x60
> +
> +#define ESDHC_TUNING_CTRL               0xcc
> +#define ESDHC_TUNE_CTRL_STATUS          0x68
> +#define ESDHC_WTMK_LVL                  0x44
> +
> +#define ESDHC_CTRL_4BITBUS              (0x1 << 1)
> +#define ESDHC_CTRL_8BITBUS              (0x2 << 1)
> +
> +
>  #endif
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 6d6a791ee9..f561cc44e3 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -265,7 +265,8 @@ static void sdhci_send_command(SDHCIState *s)
>              }
>          }
>
> -        if ((s->norintstsen & SDHC_NISEN_TRSCMP) &&
> +        if (!(s->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
> +            (s->norintstsen & SDHC_NISEN_TRSCMP) &&
>              (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
>              s->norintsts |= SDHC_NIS_TRSCMP;
>          }
> @@ -1191,6 +1192,8 @@ static void sdhci_initfn(SDHCIState *s)
>
>      s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, 
> sdhci_raise_insertion_irq, s);
>      s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, 
> sdhci_data_transfer, s);
> +
> +    s->io_ops = &sdhci_mmio_ops;
>  }
>
>  static void sdhci_uninitfn(SDHCIState *s)
> @@ -1347,7 +1350,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, 
> Error ** errp)
>      s->buf_maxsz = sdhci_get_fifolen(s);
>      s->fifo_buffer = g_malloc0(s->buf_maxsz);
>      sysbus_init_irq(sbd, &s->irq);
> -    memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
> +    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
>              SDHC_REGISTERS_MAP_SIZE);
>      sysbus_init_mmio(sbd, &s->iomem);
>  }
> @@ -1386,11 +1389,131 @@ static const TypeInfo sdhci_bus_info = {
>      .class_init = sdhci_bus_class_init,
>  };
>
> +static uint64_t usdhc_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    SDHCIState *s = SYSBUS_SDHCI(opaque);
> +    uint32_t ret;
> +    uint16_t hostctl;
> +
> +    switch (offset) {
> +    default:
> +        return sdhci_read(opaque, offset, size);
> +
> +    case SDHC_HOSTCTL:
> +        hostctl = SDHC_DMA_TYPE(s->hostctl) << 5;
> +
> +        if (s->hostctl & SDHC_CTRL_8BITBUS) {
> +            hostctl |= ESDHC_CTRL_8BITBUS;
> +        }
> +
> +        if (s->hostctl & SDHC_CTRL_4BITBUS) {
> +            hostctl |= ESDHC_CTRL_4BITBUS;
> +        }
> +
> +        ret = hostctl | (s->blkgap << 16) |
> +            (s->wakcon << 24);
> +
> +        break;
> +
> +    case ESDHC_DLL_CTRL:
> +    case ESDHC_TUNE_CTRL_STATUS:
> +    case 0x6c:
> +    case ESDHC_TUNING_CTRL:
> +    case ESDHC_VENDOR_SPEC:
> +    case ESDHC_MIX_CTRL:
> +    case ESDHC_WTMK_LVL:
> +        ret = 0;
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static void
> +usdhc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
> +{
> +    SDHCIState *s = SYSBUS_SDHCI(opaque);
> +    uint8_t hostctl = 0;
> +    uint32_t value = (uint32_t)val;
> +
> +    switch (offset) {
> +    case ESDHC_DLL_CTRL:
> +    case ESDHC_TUNE_CTRL_STATUS:
> +    case 0x6c:
> +    case ESDHC_TUNING_CTRL:
> +    case ESDHC_WTMK_LVL:
> +    case ESDHC_VENDOR_SPEC:
> +        break;
> +
> +    case SDHC_HOSTCTL:
> +        if (value & ESDHC_CTRL_8BITBUS) {
> +            hostctl |= SDHC_CTRL_8BITBUS;
> +        }
> +
> +        if (value & ESDHC_CTRL_4BITBUS) {
> +            hostctl |= ESDHC_CTRL_4BITBUS;
> +        }
> +
> +        hostctl |= SDHC_DMA_TYPE(value >> 5);
> +
> +        value &= ~0xFE;
> +        value |= hostctl;
> +        value &= ~0xFF00;
> +        value |= s->pwrcon;
> +
> +        sdhci_write(opaque, offset, value, size);

This is pretty confusing, because we both mess about directly
with the pwrcon field and also pass the write through
to sdhci_write(). (a) some comments would help and (b)
would it be clearer to just implement the different
SDHC_HOSTCTL behaviour entirely here rather than trying to
reuse the code in sdhci_write()? I get the impression that
at least some of this is trying to shuffle stuff around so
that code can unshuffle it.

> +        break;
> +
> +    case ESDHC_MIX_CTRL:
> +        /*
> +         * The layout of the register is slightly different, but we
> +         * don't care about those bits
> +         */
> +        s->trnmod = value & 0xFFFF;
> +        break;
> +    case SDHC_TRNMOD:
> +        sdhci_write(opaque, offset, val | s->trnmod, size);

This one's simpler, but a comment would assist.

> +        break;
> +    case SDHC_BLKSIZE:
> +        val |= 0x7 << 12;
> +    default:                    /* FALLTHROUGH */
> +        sdhci_write(opaque, offset, val, size);
> +        break;
> +    }
> +}
> +
> +
> +static const MemoryRegionOps usdhc_mmio_ops = {
> +    .read = usdhc_read,
> +    .write = usdhc_write,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4,
> +        .unaligned = false
> +    },
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void imx_usdhc_init(Object *obj)
> +{
> +    SDHCIState *s = SYSBUS_SDHCI(obj);
> +
> +    s->io_ops = &usdhc_mmio_ops;
> +    s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
> +}
> +
> +static const TypeInfo imx_usdhc_info = {
> +    .name = TYPE_IMX_USDHC,
> +    .parent = TYPE_SYSBUS_SDHCI,
> +    .instance_init = imx_usdhc_init,
> +};
> +
>  static void sdhci_register_types(void)
>  {
>      type_register_static(&sdhci_pci_info);
>      type_register_static(&sdhci_sysbus_info);
>      type_register_static(&sdhci_bus_info);
> +    type_register_static(&imx_usdhc_info);
>  }
>
>  type_init(sdhci_register_types)
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 0f0c3f1e64..dc1856a33d 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -39,6 +39,7 @@ typedef struct SDHCIState {
>      };
>      SDBus sdbus;
>      MemoryRegion iomem;
> +    const MemoryRegionOps *io_ops;
>
>      QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
>      QEMUTimer *transfer_timer;
> @@ -83,8 +84,13 @@ typedef struct SDHCIState {
>      /* Force Event Auto CMD12 Error Interrupt Reg - write only */
>      /* Force Event Error Interrupt Register- write only */
>      /* RO Host Controller Version Register always reads as 0x2401 */
> +
> +    unsigned long quirks;

Don't use 'unsigned long', it differs in size from host to host.

>  } SDHCIState;
>
> +/* Controller does not provide transfer-complete interrupt when not busy */
> +#define SDHCI_QUIRK_NO_BUSY_IRQ    BIT(14)

We only have one quirk, so why is it bit 14?

> +
>  #define TYPE_PCI_SDHCI "sdhci-pci"
>  #define PCI_SDHCI(obj) OBJECT_CHECK(SDHCIState, (obj), TYPE_PCI_SDHCI)
>
> @@ -92,4 +98,6 @@ typedef struct SDHCIState {
>  #define SYSBUS_SDHCI(obj)                               \
>       OBJECT_CHECK(SDHCIState, (obj), TYPE_SYSBUS_SDHCI)
>
> +#define TYPE_IMX_USDHC "imx-usdhc"
> +
>  #endif /* SDHCI_H */
> --
> 2.13.6

thanks
-- PMM