[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 11/30] sdhci: Add i.MX specific subtype of SD
From: |
Andrey Smirnov |
Subject: |
Re: [Qemu-devel] [PATCH v3 11/30] sdhci: Add i.MX specific subtype of SDHCI |
Date: |
Wed, 22 Nov 2017 12:43:06 -0800 |
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
- [Qemu-devel] [PATCH v3 07/30] imx_fec: Add support for multiple Tx DMA rings, (continued)
- [Qemu-devel] [PATCH v3 13/30] i.MX: Add code to emulate i.MX7 CCM, PMU and ANALOG IP blocks, Andrey Smirnov, 2017/11/06
- [Qemu-devel] [PATCH v3 14/30] i.MX: Add code to emulate i.MX2 watchdog IP block, Andrey Smirnov, 2017/11/06
- [Qemu-devel] [PATCH v3 15/30] i.MX: Add code to emulate i.MX7 SNVS IP-block, Andrey Smirnov, 2017/11/06
- [Qemu-devel] [PATCH v3 19/30] i.MX: Add code to emulate SDMA IP block, Andrey Smirnov, 2017/11/06