On 6 November 2017 at 15:47, Andrey Smirnov <andrew.smir...@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.mayd...@linaro.org> > Cc: Jason Wang <jasow...@redhat.com> > Cc: Philippe Mathieu-Daudé <f4...@amsat.org> > Cc: qemu-devel@nongnu.org > Cc: qemu-...@nongnu.org > Cc: yurov...@gmail.com > Signed-off-by: Andrey Smirnov <andrew.smir...@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