On 11 December 2017 at 21:30, 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>
> ---
> + case ESDHC_DLL_CTRL:
> + case ESDHC_TUNE_CTRL_STATUS:
> + case 0x6c:
Isn't there a name we can give 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.
> + */
Thanks for this explanation, it's very helpful in figuring out what's
going on.
> + 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 */
We generally write this lowercase: /* fallthrough */
> + default:
> + sdhci_write(opaque, offset, val, size);
> + break;
> + }
> +}
> 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;
I asked for this not to be unsigned long in the last round of review.
> } SDHCIState;
>
> +/* Controller does not provide transfer-complete interrupt when not busy */
> +#define SDHCI_QUIRK_NO_BUSY_IRQ BIT(14)
I asked for a comment saying we're following the Linux kernel's
quirk bit numbering in the last round of review.
> +
> #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.14.3
Otherwise
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
thanks
-- PMM
