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

[Andrey Smirnov]

On Tue, Nov 21, 2017 at 10:02 AM, Peter Maydell
<address@hidden> wrote:
> 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

Sure, will do.

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

Main reason I did this is because those bit transformations are the
inverse (or at least should be) of what Linux driver does in its
"SDHCI -> ESDHC" conversion code.

>> +        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.
>

OK, will add.

>> +        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.
>

OK, sure.

>>  } 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?
>

I took that code from Linux kernel, so I tried to keep it as is (same
with unsigned long in quirks field).

Thanks,
Andrey Smirnov