On Tue, Nov 21, 2017 at 10:02 AM, Peter Maydell <peter.mayd...@linaro.org> wrote: > 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
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