qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH v2 12/15] sdhci: Add i.MX specific subtype of SDHC


From: Andrey Smirnov
Subject: Re: [Qemu-arm] [PATCH v2 12/15] sdhci: Add i.MX specific subtype of SDHCI
Date: Fri, 15 Dec 2017 10:51:21 -0800

On Thu, Dec 14, 2017 at 6:31 PM, Philippe Mathieu-Daudé <address@hidden> wrote:
> Hi Andrey, Peter.
>
> I rather disagree with this patch, however I applied it on top of my
> current tree and plan to refactor it. But if it is applied before, I can
> survive :) Not a strong NACK.
>

Umm, Philippe, I didn't really ask you to refactor my code and I'd
really appreciate if you'd engage into a proper review process with me
on this patch, instead of just telling me "I don't like the code, but,
worry not, I'll bulldoze it away later anyway".


> See comment inlined.
>
> On 12/14/2017 11:52 AM, Andrey Smirnov 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
>> Reviewed-by: Peter Maydell <address@hidden>
>> Signed-off-by: Andrey Smirnov <address@hidden>
>> ---
>>  hw/sd/sdhci-internal.h |  19 +++++
>>  hw/sd/sdhci.c          | 228 
>> ++++++++++++++++++++++++++++++++++++++++++++++++-
>>  include/hw/sd/sdhci.h  |  14 +++
>>  3 files changed, 259 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
>> index 161177cf39..b86ac0791b 100644
>> --- a/hw/sd/sdhci-internal.h
>> +++ b/hw/sd/sdhci-internal.h
>> @@ -85,12 +85,18 @@
>>
>>  /* R/W Host control Register 0x0 */
>>  #define SDHC_HOSTCTL                   0x28
>> +#define SDHC_CTRL_LED                  0x01
>>  #define SDHC_CTRL_DMA_CHECK_MASK       0x18
>>  #define SDHC_CTRL_SDMA                 0x00
>>  #define SDHC_CTRL_ADMA1_32             0x08
>>  #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
>> +#define SDHC_CTRL_CDTEST_INS           0x40
>> +#define SDHC_CTRL_CDTEST_EN            0x80
>> +
>>
>>  /* R/W Power Control Register 0x0 */
>>  #define SDHC_PWRCON                    0x29
>> @@ -229,4 +235,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..758af067f9 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,232 @@ 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:
>> +        /*
>> +         * For a detailed explanation on the following bit
>> +         * manipulation code see comments in a similar part of
>> +         * usdhc_write()
>> +         */
>> +        hostctl = SDHC_DMA_TYPE(s->hostctl) << (8 - 3);
>> +
>> +        if (s->hostctl & SDHC_CTRL_8BITBUS) {
>> +            hostctl |= ESDHC_CTRL_8BITBUS;
>> +        }
>> +
>> +        if (s->hostctl & SDHC_CTRL_4BITBUS) {
>> +            hostctl |= ESDHC_CTRL_4BITBUS;
>> +        }
>> +
>> +        ret  = hostctl;
>> +        ret |= (uint32_t)s->blkgap << 16;
>> +        ret |= (uint32_t)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;
>> +    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:
>> +        /*
>> +         * Here's What ESDHCI has at offset 0x28 (SDHC_HOSTCTL)
>> +         *
>> +         *       7         6     5      4      3      2        1      0
>> +         * |-----------+--------+--------+-----------+----------+---------|
>> +         * | Card      | Card   | Endian | DATA3     | Data     | Led     |
>> +         * | Detect    | Detect | Mode   | as Card   | Transfer | Control |
>> +         * | Signal    | Test   |        | Detection | Width    |         |
>> +         * | Selection | Level  |        | Pin       |          |         |
>> +         * |-----------+--------+--------+-----------+----------+---------|
>> +         *
>> +         * and 0x29
>> +         *
>> +         *  15      10 9    8
>> +         * |----------+------|
>> +         * | Reserved | DMA  |
>> +         * |          | Sel. |
>> +         * |          |      |
>> +         * |----------+------|
>> +         *
>> +         * and here's what SDCHI spec expects those offsets to be:
>> +         *
>> +         * 0x28 (Host Control Register)
>> +         *
>> +         *     7        6         5       4  3      2         1        0
>> +         * 
>> |--------+--------+----------+------+--------+----------+---------|
>> +         * | Card   | Card   | Extended | DMA  | High   | Data     | LED    
>>  |
>> +         * | Detect | Detect | Data     | Sel. | Speed  | Transfer | 
>> Control |
>> +         * | Signal | Test   | Transfer |      | Enable | Width    |        
>>  |
>> +         * | Sel.   | Level  | Width    |      |        |          |        
>>  |
>> +         * 
>> |--------+--------+----------+------+--------+----------+---------|
>> +         *
>> +         * and 0x29 (Power Control Register)
>> +         *
>> +         * |----------------------------------|
>> +         * | Power Control Register           |
>> +         * |                                  |
>> +         * | Description omitted,             |
>> +         * | since it has no analog in ESDHCI |
>> +         * |                                  |
>> +         * |----------------------------------|
>> +         *
>> +         * Since offsets 0x2A and 0x2B should be compatible between
>> +         * both IP specs we only need to reconcile least 16-bit of the
>> +         * word we've been given.
>> +         */
>> +
>> +        /*
>> +         * First, save bits 7 6 and 0 since they are identical
>> +         */
>> +        hostctl = value & (SDHC_CTRL_LED |
>> +                           SDHC_CTRL_CDTEST_INS |
>> +                           SDHC_CTRL_CDTEST_EN);
>> +        /*
>> +         * Second, split "Data Transfer Width" from bits 2 and 1 in to
>> +         * bits 5 and 1
>> +         */
>> +        if (value & ESDHC_CTRL_8BITBUS) {
>> +            hostctl |= SDHC_CTRL_8BITBUS;
>> +        }
>> +
>> +        if (value & ESDHC_CTRL_4BITBUS) {
>> +            hostctl |= ESDHC_CTRL_4BITBUS;
>> +        }
>> +
>> +        /*
>> +         * Third, move DMA select from bits 9 and 8 to bits 4 and 3
>> +         */
>> +        hostctl |= SDHC_DMA_TYPE(value >> (8 - 3));
>> +
>> +        /*
>> +         * Now place the corrected value into low 16-bit of the value
>> +         * we are going to give standard SDHCI write function
>> +         *
>> +         * NOTE: This transformation should be the inverse of what can
>> +         * be found in drivers/mmc/host/sdhci-esdhc-imx.c in Linux
>> +         * kernel
>> +         */
>> +        value &= ~UINT16_MAX;
>> +        value |= hostctl;
>> +        value |= (uint16_t)s->pwrcon << 8;
>> +
>> +        sdhci_write(opaque, offset, value, size);
>> +        break;
>> +
>> +    case ESDHC_MIX_CTRL:
>> +        /*
>> +         * So, when SD/MMC stack in Linux tries to write to "Transfer
>> +         * Mode Register", ESDHC i.MX quirk code will translate it
>> +         * into a write to ESDHC_MIX_CTRL, so we do the opposite in
>> +         * order to get where we started
>> +         *
>> +         * Note that Auto CMD23 Enable bit is located in a wrong place
>> +         * on i.MX, but since it is not used by QEMU we do not care.
>> +         *
>> +         * We don't want to call sdhci_write(.., SDHC_TRNMOD, ...)
>> +         * here becuase it will result in a call to
>> +         * sdhci_send_command(s) which we don't want.
>> +         *
>> +         */
>> +        s->trnmod = value & UINT16_MAX;
>> +        break;
>> +    case SDHC_TRNMOD:
>> +        /*
>> +         * Similar to above, but this time a write to "Command
>> +         * Register" will be translated into a 4-byte write to
>> +         * "Transfer Mode register" where lower 16-bit of value would
>> +         * be set to zero. So what we do is fill those bits with
>> +         * cached value from s->trnmod and let the SDHCI
>> +         * infrastructure handle the rest
>> +         */
>> +        sdhci_write(opaque, offset, val | s->trnmod, size);
>> +        break;
>> +    case SDHC_BLKSIZE:
>> +        /*
>> +         * ESDHCI does not implement "Host SDMA Buffer Boundary", and
>> +         * Linux driver will try to zero this field out which will
>> +         * break the rest of SDHCI emulation.
>> +         *
>> +         * Linux defaults to maximum possible setting (512K boundary)
>> +         * and it seems to be the only option that i.MX IP implements,
>> +         * so we artificially set it to that value.
>> +         */
>> +        val |= 0x7 << 12;
>> +        /* FALLTHROUGH */
>> +    default:
>> +        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,
>> +};
>
> I think the usdhc_mmio_ops can be avoided.
>

OK, care to actually elaborate on this?

>> +
>> +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,
>> +};
>
> there is no need to add yet another device. You can achieve the same
> using a property bit (like the "pending-insert-quirk" one).
>
> I think the correct way is to add a DEFINE_PROP_BIT64("quirks*").
>

And how's that better? Now, every i.MX user instead of instantiating a
uSDCH IP block, which matches what i.MX RM say, would have to instead
create SDHCI (confusing) as well as to set an quirk, which means they
have to know what SDHCI_QUIRK_NO_BUSY_IRQ means. What's the point of
spilling those internal implementation guts all of the place just to
avoid re-defining "yet another device"?

Thanks,
Andrey Smirnov



reply via email to

[Prev in Thread] Current Thread [Next in Thread]