| Message ID | 20221101222934.52444-3-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ppc/e500: Add support for eSDHC | expand |
Hi, On Tue, Nov 01, 2022 at 11:29:33PM +0100, Philippe Mathieu-Daudé wrote: > Some SDHCI IP can be synthetized in various endianness: > https://github.com/u-boot/u-boot/blob/v2021.04/doc/README.fsl-esdhc > > - CONFIG_SYS_FSL_ESDHC_BE > > ESDHC IP is in big-endian mode. Accessing ESDHC registers can be > determined by ESDHC IP's endian mode or processor's endian mode. > > Our current implementation is little-endian. In order to support > big endianness: > > - Rename current MemoryRegionOps as sdhci_mmio_le_ops ('le') > - Add an 'endianness' property to SDHCIState (default little endian) > - Set the 'io_ops' field in realize() after checking the property > - Add the sdhci_mmio_be_ops (big-endian) MemoryRegionOps. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> With this patch in place (in qemu v8.0), I can no longer boot linux from SD card with sabrelite, mcimx6ul-evk, and mcimx7d-sabre emulations. I get the following persistent errors. [ 12.210101] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. [ 12.213222] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. [ 12.215072] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. [ 12.218766] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. [ 12.220441] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. [ 12.221542] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. [ 12.241544] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. [ 12.242608] sdhci-esdhc-imx 2190000.mmc: card clock still not stable in 100us!. The emulations start to work again after reverting this patch. Guenter > --- > hw/sd/sdhci-internal.h | 1 + > hw/sd/sdhci.c | 32 +++++++++++++++++++++++++++++--- > include/hw/sd/sdhci.h | 1 + > 3 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h > index 964570f8e8..5f3765f12d 100644 > --- a/hw/sd/sdhci-internal.h > +++ b/hw/sd/sdhci-internal.h > @@ -308,6 +308,7 @@ extern const VMStateDescription sdhci_vmstate; > #define SDHC_CAPAB_REG_DEFAULT 0x057834b4 > > #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \ > + DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), \ > DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \ > DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \ > DEFINE_PROP_UINT8("vendor", _state, vendor, SDHCI_VENDOR_NONE), \ > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index 22c758ad91..289baa879e 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -1329,7 +1329,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) > value >> shift, value >> shift); > } > > -static const MemoryRegionOps sdhci_mmio_ops = { > +static const MemoryRegionOps sdhci_mmio_le_ops = { > .read = sdhci_read, > .write = sdhci_write, > .impl = { > @@ -1344,6 +1344,21 @@ static const MemoryRegionOps sdhci_mmio_ops = { > .endianness = DEVICE_LITTLE_ENDIAN, > }; > > +static const MemoryRegionOps sdhci_mmio_be_ops = { > + .read = sdhci_read, > + .write = sdhci_write, > + .impl = { > + .min_access_size = 4, > + .max_access_size = 4, > + }, > + .valid = { > + .min_access_size = 1, > + .max_access_size = 4, > + .unaligned = false > + }, > + .endianness = DEVICE_BIG_ENDIAN, > +}; > + > static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp) > { > ERRP_GUARD(); > @@ -1371,8 +1386,6 @@ 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; > } > > void sdhci_uninitfn(SDHCIState *s) > @@ -1388,10 +1401,23 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) > { > ERRP_GUARD(); > > + switch (s->endianness) { > + case DEVICE_LITTLE_ENDIAN: > + s->io_ops = &sdhci_mmio_le_ops; > + break; > + case DEVICE_BIG_ENDIAN: > + s->io_ops = &sdhci_mmio_be_ops; > + break; > + default: > + error_setg(errp, "Incorrect endianness"); > + return; > + } > + > sdhci_init_readonly_registers(s, errp); > if (*errp) { > return; > } > + > s->buf_maxsz = sdhci_get_fifolen(s); > s->fifo_buffer = g_malloc0(s->buf_maxsz); > > diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h > index 01a64c5442..a989fca3b2 100644 > --- a/include/hw/sd/sdhci.h > +++ b/include/hw/sd/sdhci.h > @@ -96,6 +96,7 @@ struct SDHCIState { > /* Configurable properties */ > bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */ > uint32_t quirks; > + uint8_t endianness; > uint8_t sd_spec_version; > uint8_t uhs_mode; > uint8_t vendor; /* For vendor specific functionality */
On Sat, Apr 29, 2023 at 01:46:26PM -0700, Guenter Roeck wrote: > Hi, > > On Tue, Nov 01, 2022 at 11:29:33PM +0100, Philippe Mathieu-Daudé wrote: > > Some SDHCI IP can be synthetized in various endianness: > > https://github.com/u-boot/u-boot/blob/v2021.04/doc/README.fsl-esdhc > > > > - CONFIG_SYS_FSL_ESDHC_BE > > > > ESDHC IP is in big-endian mode. Accessing ESDHC registers can be > > determined by ESDHC IP's endian mode or processor's endian mode. > > > > Our current implementation is little-endian. In order to support > > big endianness: > > > > - Rename current MemoryRegionOps as sdhci_mmio_le_ops ('le') > > - Add an 'endianness' property to SDHCIState (default little endian) > > - Set the 'io_ops' field in realize() after checking the property > > - Add the sdhci_mmio_be_ops (big-endian) MemoryRegionOps. > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > With this patch in place (in qemu v8.0), I can no longer boot linux > from SD card with sabrelite, mcimx6ul-evk, and mcimx7d-sabre emulations. > I get the following persistent errors. > > [ 12.210101] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. > [ 12.213222] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. > [ 12.215072] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. > [ 12.218766] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. > [ 12.220441] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. > [ 12.221542] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. > [ 12.241544] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!. > [ 12.242608] sdhci-esdhc-imx 2190000.mmc: card clock still not stable in 100us!. > > The emulations start to work again after reverting this patch. > Cause explained below. > Guenter > > > --- > > hw/sd/sdhci-internal.h | 1 + > > hw/sd/sdhci.c | 32 +++++++++++++++++++++++++++++--- > > include/hw/sd/sdhci.h | 1 + > > 3 files changed, 31 insertions(+), 3 deletions(-) > > > > diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h > > index 964570f8e8..5f3765f12d 100644 > > --- a/hw/sd/sdhci-internal.h > > +++ b/hw/sd/sdhci-internal.h > > @@ -308,6 +308,7 @@ extern const VMStateDescription sdhci_vmstate; > > #define SDHC_CAPAB_REG_DEFAULT 0x057834b4 > > > > #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \ > > + DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), \ > > DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \ > > DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \ > > DEFINE_PROP_UINT8("vendor", _state, vendor, SDHCI_VENDOR_NONE), \ > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > > index 22c758ad91..289baa879e 100644 > > --- a/hw/sd/sdhci.c > > +++ b/hw/sd/sdhci.c > > @@ -1329,7 +1329,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) > > value >> shift, value >> shift); > > } > > > > -static const MemoryRegionOps sdhci_mmio_ops = { > > +static const MemoryRegionOps sdhci_mmio_le_ops = { > > .read = sdhci_read, > > .write = sdhci_write, > > .impl = { > > @@ -1344,6 +1344,21 @@ static const MemoryRegionOps sdhci_mmio_ops = { > > .endianness = DEVICE_LITTLE_ENDIAN, > > }; > > > > +static const MemoryRegionOps sdhci_mmio_be_ops = { > > + .read = sdhci_read, > > + .write = sdhci_write, > > + .impl = { > > + .min_access_size = 4, > > + .max_access_size = 4, > > + }, > > + .valid = { > > + .min_access_size = 1, > > + .max_access_size = 4, > > + .unaligned = false > > + }, > > + .endianness = DEVICE_BIG_ENDIAN, > > +}; > > + > > static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp) > > { > > ERRP_GUARD(); > > @@ -1371,8 +1386,6 @@ 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; The reason for initializing io_ops here is that the same driver also supports imx_usdhc. That function also sets io_ops to usdhc specific io ops. This is now overwritten by ... > > } > > > > void sdhci_uninitfn(SDHCIState *s) > > @@ -1388,10 +1401,23 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) > > { > > ERRP_GUARD(); > > > > + switch (s->endianness) { > > + case DEVICE_LITTLE_ENDIAN: > > + s->io_ops = &sdhci_mmio_le_ops; > > + break; > > + case DEVICE_BIG_ENDIAN: > > + s->io_ops = &sdhci_mmio_be_ops; > > + break; > > + default: > > + error_setg(errp, "Incorrect endianness"); > > + return; > > + } > > + ... this code which runs later and never sets usdhc_mmio_ops. Consequently io_ops now points to the wrong ops functions for imx. Guenter
diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h index 964570f8e8..5f3765f12d 100644 --- a/hw/sd/sdhci-internal.h +++ b/hw/sd/sdhci-internal.h @@ -308,6 +308,7 @@ extern const VMStateDescription sdhci_vmstate; #define SDHC_CAPAB_REG_DEFAULT 0x057834b4 #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \ + DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), \ DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \ DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \ DEFINE_PROP_UINT8("vendor", _state, vendor, SDHCI_VENDOR_NONE), \ diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 22c758ad91..289baa879e 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1329,7 +1329,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) value >> shift, value >> shift); } -static const MemoryRegionOps sdhci_mmio_ops = { +static const MemoryRegionOps sdhci_mmio_le_ops = { .read = sdhci_read, .write = sdhci_write, .impl = { @@ -1344,6 +1344,21 @@ static const MemoryRegionOps sdhci_mmio_ops = { .endianness = DEVICE_LITTLE_ENDIAN, }; +static const MemoryRegionOps sdhci_mmio_be_ops = { + .read = sdhci_read, + .write = sdhci_write, + .impl = { + .min_access_size = 4, + .max_access_size = 4, + }, + .valid = { + .min_access_size = 1, + .max_access_size = 4, + .unaligned = false + }, + .endianness = DEVICE_BIG_ENDIAN, +}; + static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp) { ERRP_GUARD(); @@ -1371,8 +1386,6 @@ 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; } void sdhci_uninitfn(SDHCIState *s) @@ -1388,10 +1401,23 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) { ERRP_GUARD(); + switch (s->endianness) { + case DEVICE_LITTLE_ENDIAN: + s->io_ops = &sdhci_mmio_le_ops; + break; + case DEVICE_BIG_ENDIAN: + s->io_ops = &sdhci_mmio_be_ops; + break; + default: + error_setg(errp, "Incorrect endianness"); + return; + } + sdhci_init_readonly_registers(s, errp); if (*errp) { return; } + s->buf_maxsz = sdhci_get_fifolen(s); s->fifo_buffer = g_malloc0(s->buf_maxsz); diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h index 01a64c5442..a989fca3b2 100644 --- a/include/hw/sd/sdhci.h +++ b/include/hw/sd/sdhci.h @@ -96,6 +96,7 @@ struct SDHCIState { /* Configurable properties */ bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */ uint32_t quirks; + uint8_t endianness; uint8_t sd_spec_version; uint8_t uhs_mode; uint8_t vendor; /* For vendor specific functionality */
Some SDHCI IP can be synthetized in various endianness: https://github.com/u-boot/u-boot/blob/v2021.04/doc/README.fsl-esdhc - CONFIG_SYS_FSL_ESDHC_BE ESDHC IP is in big-endian mode. Accessing ESDHC registers can be determined by ESDHC IP's endian mode or processor's endian mode. Our current implementation is little-endian. In order to support big endianness: - Rename current MemoryRegionOps as sdhci_mmio_le_ops ('le') - Add an 'endianness' property to SDHCIState (default little endian) - Set the 'io_ops' field in realize() after checking the property - Add the sdhci_mmio_be_ops (big-endian) MemoryRegionOps. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/sd/sdhci-internal.h | 1 + hw/sd/sdhci.c | 32 +++++++++++++++++++++++++++++--- include/hw/sd/sdhci.h | 1 + 3 files changed, 31 insertions(+), 3 deletions(-)