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 <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
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);
+ 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);
+ 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;
} SDHCIState;
+/* Controller does not provide transfer-complete interrupt when not busy */
+#define SDHCI_QUIRK_NO_BUSY_IRQ 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
On 6 November 2017 at 15:47, Andrey Smirnov <andrew.smirnov@gmail.com> 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 <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
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
On Tue, Nov 21, 2017 at 10:02 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 6 November 2017 at 15:47, Andrey Smirnov <andrew.smirnov@gmail.com> 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 <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>
> 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
On 22 November 2017 at 20:43, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > On Tue, Nov 21, 2017 at 10:02 AM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> On 6 November 2017 at 15:47, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: >>> +/* 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). That's fine, but if so we should have a comment that that's what we're doing, so that subsequent additions by other contributors follow the same convention. thanks -- PMM
© 2016 - 2026 Red Hat, Inc.