* [PATCH] hw/sd: Fix sun4i allwinner-sdhost for U-Boot
@ 2022-11-12 21:49 Strahinja Jankovic
2022-11-14 15:41 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Strahinja Jankovic @ 2022-11-12 21:49 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, Peter Maydell, Beniamino Galvani, Strahinja Jankovic
Trying to run U-Boot for Cubieboard (Allwinner A10) fails because it cannot
access SD card. The problem is that FIFO register in current
allwinner-sdhost implementation is at the address corresponding to
Allwinner H3, but not A10.
Linux kernel is not affected since Linux driver uses DMA access and does
not use FIFO register for reading/writing.
This patch adds new class parameter `is_sun4i` and based on that
parameter uses register at offset 0x100 either as FIFO register (if
sun4i) or as threshold register (if not sun4i; in this case register at
0x200 is FIFO register).
Tested with U-Boot and Linux kernel image built for Cubieboard and
OrangePi PC.
Signed-off-by: Strahinja Jankovic <strahinja.p.jankovic@gmail.com>
---
hw/sd/allwinner-sdhost.c | 67 ++++++++++++++++++++++----------
include/hw/sd/allwinner-sdhost.h | 1 +
2 files changed, 47 insertions(+), 21 deletions(-)
diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
index 455d6eabf6..51e5e90830 100644
--- a/hw/sd/allwinner-sdhost.c
+++ b/hw/sd/allwinner-sdhost.c
@@ -65,7 +65,7 @@ enum {
REG_SD_DLBA = 0x84, /* Descriptor List Base Address */
REG_SD_IDST = 0x88, /* Internal DMA Controller Status */
REG_SD_IDIE = 0x8C, /* Internal DMA Controller IRQ Enable */
- REG_SD_THLDC = 0x100, /* Card Threshold Control */
+ REG_SD_THLDC = 0x100, /* Card Threshold Control / FIFO (sun4i only)*/
REG_SD_DSBD = 0x10C, /* eMMC DDR Start Bit Detection Control */
REG_SD_RES_CRC = 0x110, /* Response CRC from card/eMMC */
REG_SD_DATA7_CRC = 0x114, /* CRC Data 7 from card/eMMC */
@@ -415,10 +415,29 @@ static void allwinner_sdhost_dma(AwSdHostState *s)
}
}
+static uint32_t allwinner_sdhost_fifo_read(AwSdHostState *s)
+{
+ uint32_t res = 0;
+
+ if (sdbus_data_ready(&s->sdbus)) {
+ sdbus_read_data(&s->sdbus, &res, sizeof(uint32_t));
+ le32_to_cpus(&res);
+ allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
+ allwinner_sdhost_auto_stop(s);
+ allwinner_sdhost_update_irq(s);
+ } else {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: no data ready on SD bus\n",
+ __func__);
+ }
+
+ return res;
+}
+
static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
unsigned size)
{
AwSdHostState *s = AW_SDHOST(opaque);
+ AwSdHostClass *sc = AW_SDHOST_GET_CLASS(s);
uint32_t res = 0;
switch (offset) {
@@ -508,8 +527,12 @@ static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
case REG_SD_IDIE: /* Internal DMA Controller Interrupt Enable */
res = s->dmac_irq;
break;
- case REG_SD_THLDC: /* Card Threshold Control */
- res = s->card_threshold;
+ case REG_SD_THLDC: /* Card Threshold Control or FIFO register (sun4i) */
+ if (sc->is_sun4i) {
+ res = allwinner_sdhost_fifo_read(s);
+ } else {
+ res = s->card_threshold;
+ }
break;
case REG_SD_DSBD: /* eMMC DDR Start Bit Detection Control */
res = s->startbit_detect;
@@ -531,16 +554,7 @@ static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
res = s->status_crc;
break;
case REG_SD_FIFO: /* Read/Write FIFO */
- if (sdbus_data_ready(&s->sdbus)) {
- sdbus_read_data(&s->sdbus, &res, sizeof(uint32_t));
- le32_to_cpus(&res);
- allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
- allwinner_sdhost_auto_stop(s);
- allwinner_sdhost_update_irq(s);
- } else {
- qemu_log_mask(LOG_GUEST_ERROR, "%s: no data ready on SD bus\n",
- __func__);
- }
+ res = allwinner_sdhost_fifo_read(s);
break;
default:
qemu_log_mask(LOG_GUEST_ERROR, "%s: out-of-bounds offset %"
@@ -553,11 +567,20 @@ static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
return res;
}
+static void allwinner_sdhost_fifo_write(AwSdHostState *s, uint64_t value)
+{
+ uint32_t u32 = cpu_to_le32(value);
+ sdbus_write_data(&s->sdbus, &u32, sizeof(u32));
+ allwinner_sdhost_update_transfer_cnt(s, sizeof(u32));
+ allwinner_sdhost_auto_stop(s);
+ allwinner_sdhost_update_irq(s);
+}
+
static void allwinner_sdhost_write(void *opaque, hwaddr offset,
uint64_t value, unsigned size)
{
AwSdHostState *s = AW_SDHOST(opaque);
- uint32_t u32;
+ AwSdHostClass *sc = AW_SDHOST_GET_CLASS(s);
trace_allwinner_sdhost_write(offset, value, size);
@@ -657,18 +680,18 @@ static void allwinner_sdhost_write(void *opaque, hwaddr offset,
s->dmac_irq = value;
allwinner_sdhost_update_irq(s);
break;
- case REG_SD_THLDC: /* Card Threshold Control */
- s->card_threshold = value;
+ case REG_SD_THLDC: /* Card Threshold Control or FIFO (sun4i) */
+ if (sc->is_sun4i) {
+ allwinner_sdhost_fifo_write(s, value);
+ } else {
+ s->card_threshold = value;
+ }
break;
case REG_SD_DSBD: /* eMMC DDR Start Bit Detection Control */
s->startbit_detect = value;
break;
case REG_SD_FIFO: /* Read/Write FIFO */
- u32 = cpu_to_le32(value);
- sdbus_write_data(&s->sdbus, &u32, sizeof(u32));
- allwinner_sdhost_update_transfer_cnt(s, sizeof(u32));
- allwinner_sdhost_auto_stop(s);
- allwinner_sdhost_update_irq(s);
+ allwinner_sdhost_fifo_write(s, value);
break;
case REG_SD_RES_CRC: /* Response CRC from card/eMMC */
case REG_SD_DATA7_CRC: /* CRC Data 7 from card/eMMC */
@@ -834,12 +857,14 @@ static void allwinner_sdhost_sun4i_class_init(ObjectClass *klass, void *data)
{
AwSdHostClass *sc = AW_SDHOST_CLASS(klass);
sc->max_desc_size = 8 * KiB;
+ sc->is_sun4i = true;
}
static void allwinner_sdhost_sun5i_class_init(ObjectClass *klass, void *data)
{
AwSdHostClass *sc = AW_SDHOST_CLASS(klass);
sc->max_desc_size = 64 * KiB;
+ sc->is_sun4i = false;
}
static const TypeInfo allwinner_sdhost_info = {
diff --git a/include/hw/sd/allwinner-sdhost.h b/include/hw/sd/allwinner-sdhost.h
index bfe08ff4ef..30c1e60404 100644
--- a/include/hw/sd/allwinner-sdhost.h
+++ b/include/hw/sd/allwinner-sdhost.h
@@ -130,6 +130,7 @@ struct AwSdHostClass {
/** Maximum buffer size in bytes per DMA descriptor */
size_t max_desc_size;
+ bool is_sun4i;
};
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] hw/sd: Fix sun4i allwinner-sdhost for U-Boot
2022-11-12 21:49 [PATCH] hw/sd: Fix sun4i allwinner-sdhost for U-Boot Strahinja Jankovic
@ 2022-11-14 15:41 ` Peter Maydell
2022-11-14 17:29 ` Strahinja Jankovic
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2022-11-14 15:41 UTC (permalink / raw)
To: Strahinja Jankovic
Cc: qemu-devel, qemu-arm, Beniamino Galvani, Strahinja Jankovic
On Sat, 12 Nov 2022 at 21:49, Strahinja Jankovic
<strahinjapjankovic@gmail.com> wrote:
>
> Trying to run U-Boot for Cubieboard (Allwinner A10) fails because it cannot
> access SD card. The problem is that FIFO register in current
> allwinner-sdhost implementation is at the address corresponding to
> Allwinner H3, but not A10.
> Linux kernel is not affected since Linux driver uses DMA access and does
> not use FIFO register for reading/writing.
>
> This patch adds new class parameter `is_sun4i` and based on that
> parameter uses register at offset 0x100 either as FIFO register (if
> sun4i) or as threshold register (if not sun4i; in this case register at
> 0x200 is FIFO register).
>
> Tested with U-Boot and Linux kernel image built for Cubieboard and
> OrangePi PC.
>
> Signed-off-by: Strahinja Jankovic <strahinja.p.jankovic@gmail.com>
> ---
> hw/sd/allwinner-sdhost.c | 67 ++++++++++++++++++++++----------
> include/hw/sd/allwinner-sdhost.h | 1 +
> 2 files changed, 47 insertions(+), 21 deletions(-)
>
> diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
> index 455d6eabf6..51e5e90830 100644
> --- a/hw/sd/allwinner-sdhost.c
> +++ b/hw/sd/allwinner-sdhost.c
> @@ -65,7 +65,7 @@ enum {
> REG_SD_DLBA = 0x84, /* Descriptor List Base Address */
> REG_SD_IDST = 0x88, /* Internal DMA Controller Status */
> REG_SD_IDIE = 0x8C, /* Internal DMA Controller IRQ Enable */
> - REG_SD_THLDC = 0x100, /* Card Threshold Control */
> + REG_SD_THLDC = 0x100, /* Card Threshold Control / FIFO (sun4i only)*/
> REG_SD_DSBD = 0x10C, /* eMMC DDR Start Bit Detection Control */
> REG_SD_RES_CRC = 0x110, /* Response CRC from card/eMMC */
> REG_SD_DATA7_CRC = 0x114, /* CRC Data 7 from card/eMMC */
> @@ -415,10 +415,29 @@ static void allwinner_sdhost_dma(AwSdHostState *s)
> }
> }
>
> +static uint32_t allwinner_sdhost_fifo_read(AwSdHostState *s)
> +{
> + uint32_t res = 0;
> +
> + if (sdbus_data_ready(&s->sdbus)) {
> + sdbus_read_data(&s->sdbus, &res, sizeof(uint32_t));
> + le32_to_cpus(&res);
> + allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
> + allwinner_sdhost_auto_stop(s);
> + allwinner_sdhost_update_irq(s);
> + } else {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: no data ready on SD bus\n",
> + __func__);
> + }
> +
> + return res;
> +}
> +
> static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
> unsigned size)
> {
> AwSdHostState *s = AW_SDHOST(opaque);
> + AwSdHostClass *sc = AW_SDHOST_GET_CLASS(s);
> uint32_t res = 0;
>
> switch (offset) {
> @@ -508,8 +527,12 @@ static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
> case REG_SD_IDIE: /* Internal DMA Controller Interrupt Enable */
> res = s->dmac_irq;
> break;
> - case REG_SD_THLDC: /* Card Threshold Control */
> - res = s->card_threshold;
> + case REG_SD_THLDC: /* Card Threshold Control or FIFO register (sun4i) */
> + if (sc->is_sun4i) {
> + res = allwinner_sdhost_fifo_read(s);
> + } else {
> + res = s->card_threshold;
> + }
> break;
> case REG_SD_DSBD: /* eMMC DDR Start Bit Detection Control */
> res = s->startbit_detect;
> @@ -531,16 +554,7 @@ static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
> res = s->status_crc;
> break;
> case REG_SD_FIFO: /* Read/Write FIFO */
> - if (sdbus_data_ready(&s->sdbus)) {
> - sdbus_read_data(&s->sdbus, &res, sizeof(uint32_t));
> - le32_to_cpus(&res);
> - allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
> - allwinner_sdhost_auto_stop(s);
> - allwinner_sdhost_update_irq(s);
> - } else {
> - qemu_log_mask(LOG_GUEST_ERROR, "%s: no data ready on SD bus\n",
> - __func__);
> - }
> + res = allwinner_sdhost_fifo_read(s);
Does the sun4i really have the FIFO at both addresses, or should
this one do something else for sun4i ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] hw/sd: Fix sun4i allwinner-sdhost for U-Boot
2022-11-14 15:41 ` Peter Maydell
@ 2022-11-14 17:29 ` Strahinja Jankovic
2022-11-14 17:36 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Strahinja Jankovic @ 2022-11-14 17:29 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, qemu-arm, Beniamino Galvani
Hi,
Thank you for your reply.
On Mon, Nov 14, 2022 at 4:42 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sat, 12 Nov 2022 at 21:49, Strahinja Jankovic
> <strahinjapjankovic@gmail.com> wrote:
> >
> > Trying to run U-Boot for Cubieboard (Allwinner A10) fails because it cannot
> > access SD card. The problem is that FIFO register in current
> > allwinner-sdhost implementation is at the address corresponding to
> > Allwinner H3, but not A10.
> > Linux kernel is not affected since Linux driver uses DMA access and does
> > not use FIFO register for reading/writing.
> >
> > This patch adds new class parameter `is_sun4i` and based on that
> > parameter uses register at offset 0x100 either as FIFO register (if
> > sun4i) or as threshold register (if not sun4i; in this case register at
> > 0x200 is FIFO register).
> >
> > Tested with U-Boot and Linux kernel image built for Cubieboard and
> > OrangePi PC.
> >
> > Signed-off-by: Strahinja Jankovic <strahinja.p.jankovic@gmail.com>
> > ---
> > hw/sd/allwinner-sdhost.c | 67 ++++++++++++++++++++++----------
> > include/hw/sd/allwinner-sdhost.h | 1 +
> > 2 files changed, 47 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
> > index 455d6eabf6..51e5e90830 100644
> > --- a/hw/sd/allwinner-sdhost.c
> > +++ b/hw/sd/allwinner-sdhost.c
> > @@ -65,7 +65,7 @@ enum {
> > REG_SD_DLBA = 0x84, /* Descriptor List Base Address */
> > REG_SD_IDST = 0x88, /* Internal DMA Controller Status */
> > REG_SD_IDIE = 0x8C, /* Internal DMA Controller IRQ Enable */
> > - REG_SD_THLDC = 0x100, /* Card Threshold Control */
> > + REG_SD_THLDC = 0x100, /* Card Threshold Control / FIFO (sun4i only)*/
> > REG_SD_DSBD = 0x10C, /* eMMC DDR Start Bit Detection Control */
> > REG_SD_RES_CRC = 0x110, /* Response CRC from card/eMMC */
> > REG_SD_DATA7_CRC = 0x114, /* CRC Data 7 from card/eMMC */
> > @@ -415,10 +415,29 @@ static void allwinner_sdhost_dma(AwSdHostState *s)
> > }
> > }
> >
> > +static uint32_t allwinner_sdhost_fifo_read(AwSdHostState *s)
> > +{
> > + uint32_t res = 0;
> > +
> > + if (sdbus_data_ready(&s->sdbus)) {
> > + sdbus_read_data(&s->sdbus, &res, sizeof(uint32_t));
> > + le32_to_cpus(&res);
> > + allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
> > + allwinner_sdhost_auto_stop(s);
> > + allwinner_sdhost_update_irq(s);
> > + } else {
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: no data ready on SD bus\n",
> > + __func__);
> > + }
> > +
> > + return res;
> > +}
> > +
> > static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
> > unsigned size)
> > {
> > AwSdHostState *s = AW_SDHOST(opaque);
> > + AwSdHostClass *sc = AW_SDHOST_GET_CLASS(s);
> > uint32_t res = 0;
> >
> > switch (offset) {
> > @@ -508,8 +527,12 @@ static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
> > case REG_SD_IDIE: /* Internal DMA Controller Interrupt Enable */
> > res = s->dmac_irq;
> > break;
> > - case REG_SD_THLDC: /* Card Threshold Control */
> > - res = s->card_threshold;
> > + case REG_SD_THLDC: /* Card Threshold Control or FIFO register (sun4i) */
> > + if (sc->is_sun4i) {
> > + res = allwinner_sdhost_fifo_read(s);
> > + } else {
> > + res = s->card_threshold;
> > + }
> > break;
> > case REG_SD_DSBD: /* eMMC DDR Start Bit Detection Control */
> > res = s->startbit_detect;
> > @@ -531,16 +554,7 @@ static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
> > res = s->status_crc;
> > break;
> > case REG_SD_FIFO: /* Read/Write FIFO */
> > - if (sdbus_data_ready(&s->sdbus)) {
> > - sdbus_read_data(&s->sdbus, &res, sizeof(uint32_t));
> > - le32_to_cpus(&res);
> > - allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
> > - allwinner_sdhost_auto_stop(s);
> > - allwinner_sdhost_update_irq(s);
> > - } else {
> > - qemu_log_mask(LOG_GUEST_ERROR, "%s: no data ready on SD bus\n",
> > - __func__);
> > - }
> > + res = allwinner_sdhost_fifo_read(s);
>
> Does the sun4i really have the FIFO at both addresses, or should
> this one do something else for sun4i ?
The sun4i sdhost actually has no registers with offset higher than
0x100 (offset of REG_SD_THLDC in patch), so REG_SD_DSBD, all
REG_SD_*_CRC, REG_SD_CRC_STA and REG_SD_FIFO@0x200 should not be
accessed from application code meant to run on sun4i. That is why I
only changed the FIFO/THLDC (offset 0x100) register behavior, since
that change makes U-Boot work.
I could update the patch so all of these registers with offset bigger
than 0x100 log error if sun4i is selected, so that is more clear.
Would that be ok?
Best regards,
Strahinja
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] hw/sd: Fix sun4i allwinner-sdhost for U-Boot
2022-11-14 17:29 ` Strahinja Jankovic
@ 2022-11-14 17:36 ` Peter Maydell
2022-11-14 19:22 ` Strahinja Jankovic
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2022-11-14 17:36 UTC (permalink / raw)
To: Strahinja Jankovic; +Cc: qemu-devel, qemu-arm, Beniamino Galvani
On Mon, 14 Nov 2022 at 17:29, Strahinja Jankovic
<strahinjapjankovic@gmail.com> wrote:
>
> Hi,
>
> Thank you for your reply.
>
> On Mon, Nov 14, 2022 at 4:42 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Sat, 12 Nov 2022 at 21:49, Strahinja Jankovic
> > <strahinjapjankovic@gmail.com> wrote:
> > >
> > > Trying to run U-Boot for Cubieboard (Allwinner A10) fails because it cannot
> > > access SD card. The problem is that FIFO register in current
> > > allwinner-sdhost implementation is at the address corresponding to
> > > Allwinner H3, but not A10.
> > > Linux kernel is not affected since Linux driver uses DMA access and does
> > > not use FIFO register for reading/writing.
> > >
> > > This patch adds new class parameter `is_sun4i` and based on that
> > > parameter uses register at offset 0x100 either as FIFO register (if
> > > sun4i) or as threshold register (if not sun4i; in this case register at
> > > 0x200 is FIFO register).
> > >
> > > Tested with U-Boot and Linux kernel image built for Cubieboard and
> > > OrangePi PC.
> > >
> > > Signed-off-by: Strahinja Jankovic <strahinja.p.jankovic@gmail.com>
> > > ---
> > > hw/sd/allwinner-sdhost.c | 67 ++++++++++++++++++++++----------
> > > include/hw/sd/allwinner-sdhost.h | 1 +
> > > 2 files changed, 47 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
> > > index 455d6eabf6..51e5e90830 100644
> > > --- a/hw/sd/allwinner-sdhost.c
> > > +++ b/hw/sd/allwinner-sdhost.c
> > > @@ -65,7 +65,7 @@ enum {
> > > REG_SD_DLBA = 0x84, /* Descriptor List Base Address */
> > > REG_SD_IDST = 0x88, /* Internal DMA Controller Status */
> > > REG_SD_IDIE = 0x8C, /* Internal DMA Controller IRQ Enable */
> > > - REG_SD_THLDC = 0x100, /* Card Threshold Control */
> > > + REG_SD_THLDC = 0x100, /* Card Threshold Control / FIFO (sun4i only)*/
> > > REG_SD_DSBD = 0x10C, /* eMMC DDR Start Bit Detection Control */
> > > REG_SD_RES_CRC = 0x110, /* Response CRC from card/eMMC */
> > > REG_SD_DATA7_CRC = 0x114, /* CRC Data 7 from card/eMMC */
> > > @@ -415,10 +415,29 @@ static void allwinner_sdhost_dma(AwSdHostState *s)
> > > }
> > > }
> > >
> > > +static uint32_t allwinner_sdhost_fifo_read(AwSdHostState *s)
> > > +{
> > > + uint32_t res = 0;
> > > +
> > > + if (sdbus_data_ready(&s->sdbus)) {
> > > + sdbus_read_data(&s->sdbus, &res, sizeof(uint32_t));
> > > + le32_to_cpus(&res);
> > > + allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
> > > + allwinner_sdhost_auto_stop(s);
> > > + allwinner_sdhost_update_irq(s);
> > > + } else {
> > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: no data ready on SD bus\n",
> > > + __func__);
> > > + }
> > > +
> > > + return res;
> > > +}
> > > +
> > > static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
> > > unsigned size)
> > > {
> > > AwSdHostState *s = AW_SDHOST(opaque);
> > > + AwSdHostClass *sc = AW_SDHOST_GET_CLASS(s);
> > > uint32_t res = 0;
> > >
> > > switch (offset) {
> > > @@ -508,8 +527,12 @@ static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
> > > case REG_SD_IDIE: /* Internal DMA Controller Interrupt Enable */
> > > res = s->dmac_irq;
> > > break;
> > > - case REG_SD_THLDC: /* Card Threshold Control */
> > > - res = s->card_threshold;
> > > + case REG_SD_THLDC: /* Card Threshold Control or FIFO register (sun4i) */
> > > + if (sc->is_sun4i) {
> > > + res = allwinner_sdhost_fifo_read(s);
> > > + } else {
> > > + res = s->card_threshold;
> > > + }
> > > break;
> > > case REG_SD_DSBD: /* eMMC DDR Start Bit Detection Control */
> > > res = s->startbit_detect;
> > > @@ -531,16 +554,7 @@ static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
> > > res = s->status_crc;
> > > break;
> > > case REG_SD_FIFO: /* Read/Write FIFO */
> > > - if (sdbus_data_ready(&s->sdbus)) {
> > > - sdbus_read_data(&s->sdbus, &res, sizeof(uint32_t));
> > > - le32_to_cpus(&res);
> > > - allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
> > > - allwinner_sdhost_auto_stop(s);
> > > - allwinner_sdhost_update_irq(s);
> > > - } else {
> > > - qemu_log_mask(LOG_GUEST_ERROR, "%s: no data ready on SD bus\n",
> > > - __func__);
> > > - }
> > > + res = allwinner_sdhost_fifo_read(s);
> >
> > Does the sun4i really have the FIFO at both addresses, or should
> > this one do something else for sun4i ?
>
> The sun4i sdhost actually has no registers with offset higher than
> 0x100 (offset of REG_SD_THLDC in patch), so REG_SD_DSBD, all
> REG_SD_*_CRC, REG_SD_CRC_STA and REG_SD_FIFO@0x200 should not be
> accessed from application code meant to run on sun4i. That is why I
> only changed the FIFO/THLDC (offset 0x100) register behavior, since
> that change makes U-Boot work.
>
> I could update the patch so all of these registers with offset bigger
> than 0x100 log error if sun4i is selected, so that is more clear.
> Would that be ok?
Yes, I think that's a good idea, but let's do that change as a
separate patch, so we can keep this one as it is as the bugfix.
For this patch,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] hw/sd: Fix sun4i allwinner-sdhost for U-Boot
2022-11-14 17:36 ` Peter Maydell
@ 2022-11-14 19:22 ` Strahinja Jankovic
2022-11-15 16:02 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Strahinja Jankovic @ 2022-11-14 19:22 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, qemu-arm, Beniamino Galvani
On Mon, Nov 14, 2022 at 6:36 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 14 Nov 2022 at 17:29, Strahinja Jankovic
> <strahinjapjankovic@gmail.com> wrote:
> >
> > Hi,
> >
> > Thank you for your reply.
> >
> > On Mon, Nov 14, 2022 at 4:42 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Sat, 12 Nov 2022 at 21:49, Strahinja Jankovic
> > > <strahinjapjankovic@gmail.com> wrote:
> > > >
> > > > Trying to run U-Boot for Cubieboard (Allwinner A10) fails because it cannot
> > > > access SD card. The problem is that FIFO register in current
> > > > allwinner-sdhost implementation is at the address corresponding to
> > > > Allwinner H3, but not A10.
> > > > Linux kernel is not affected since Linux driver uses DMA access and does
> > > > not use FIFO register for reading/writing.
> > > >
> > > > This patch adds new class parameter `is_sun4i` and based on that
> > > > parameter uses register at offset 0x100 either as FIFO register (if
> > > > sun4i) or as threshold register (if not sun4i; in this case register at
> > > > 0x200 is FIFO register).
> > > >
> > > > Tested with U-Boot and Linux kernel image built for Cubieboard and
> > > > OrangePi PC.
> > > >
> > > > Signed-off-by: Strahinja Jankovic <strahinja.p.jankovic@gmail.com>
> > > > ---
> > > > hw/sd/allwinner-sdhost.c | 67 ++++++++++++++++++++++----------
> > > > include/hw/sd/allwinner-sdhost.h | 1 +
> > > > 2 files changed, 47 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
> > > > index 455d6eabf6..51e5e90830 100644
> > > > --- a/hw/sd/allwinner-sdhost.c
> > > > +++ b/hw/sd/allwinner-sdhost.c
> > > > @@ -65,7 +65,7 @@ enum {
> > > > REG_SD_DLBA = 0x84, /* Descriptor List Base Address */
> > > > REG_SD_IDST = 0x88, /* Internal DMA Controller Status */
> > > > REG_SD_IDIE = 0x8C, /* Internal DMA Controller IRQ Enable */
> > > > - REG_SD_THLDC = 0x100, /* Card Threshold Control */
> > > > + REG_SD_THLDC = 0x100, /* Card Threshold Control / FIFO (sun4i only)*/
> > > > REG_SD_DSBD = 0x10C, /* eMMC DDR Start Bit Detection Control */
> > > > REG_SD_RES_CRC = 0x110, /* Response CRC from card/eMMC */
> > > > REG_SD_DATA7_CRC = 0x114, /* CRC Data 7 from card/eMMC */
> > > > @@ -415,10 +415,29 @@ static void allwinner_sdhost_dma(AwSdHostState *s)
> > > > }
> > > > }
> > > >
> > > > +static uint32_t allwinner_sdhost_fifo_read(AwSdHostState *s)
> > > > +{
> > > > + uint32_t res = 0;
> > > > +
> > > > + if (sdbus_data_ready(&s->sdbus)) {
> > > > + sdbus_read_data(&s->sdbus, &res, sizeof(uint32_t));
> > > > + le32_to_cpus(&res);
> > > > + allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
> > > > + allwinner_sdhost_auto_stop(s);
> > > > + allwinner_sdhost_update_irq(s);
> > > > + } else {
> > > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: no data ready on SD bus\n",
> > > > + __func__);
> > > > + }
> > > > +
> > > > + return res;
> > > > +}
> > > > +
> > > > static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
> > > > unsigned size)
> > > > {
> > > > AwSdHostState *s = AW_SDHOST(opaque);
> > > > + AwSdHostClass *sc = AW_SDHOST_GET_CLASS(s);
> > > > uint32_t res = 0;
> > > >
> > > > switch (offset) {
> > > > @@ -508,8 +527,12 @@ static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
> > > > case REG_SD_IDIE: /* Internal DMA Controller Interrupt Enable */
> > > > res = s->dmac_irq;
> > > > break;
> > > > - case REG_SD_THLDC: /* Card Threshold Control */
> > > > - res = s->card_threshold;
> > > > + case REG_SD_THLDC: /* Card Threshold Control or FIFO register (sun4i) */
> > > > + if (sc->is_sun4i) {
> > > > + res = allwinner_sdhost_fifo_read(s);
> > > > + } else {
> > > > + res = s->card_threshold;
> > > > + }
> > > > break;
> > > > case REG_SD_DSBD: /* eMMC DDR Start Bit Detection Control */
> > > > res = s->startbit_detect;
> > > > @@ -531,16 +554,7 @@ static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
> > > > res = s->status_crc;
> > > > break;
> > > > case REG_SD_FIFO: /* Read/Write FIFO */
> > > > - if (sdbus_data_ready(&s->sdbus)) {
> > > > - sdbus_read_data(&s->sdbus, &res, sizeof(uint32_t));
> > > > - le32_to_cpus(&res);
> > > > - allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
> > > > - allwinner_sdhost_auto_stop(s);
> > > > - allwinner_sdhost_update_irq(s);
> > > > - } else {
> > > > - qemu_log_mask(LOG_GUEST_ERROR, "%s: no data ready on SD bus\n",
> > > > - __func__);
> > > > - }
> > > > + res = allwinner_sdhost_fifo_read(s);
> > >
> > > Does the sun4i really have the FIFO at both addresses, or should
> > > this one do something else for sun4i ?
> >
> > The sun4i sdhost actually has no registers with offset higher than
> > 0x100 (offset of REG_SD_THLDC in patch), so REG_SD_DSBD, all
> > REG_SD_*_CRC, REG_SD_CRC_STA and REG_SD_FIFO@0x200 should not be
> > accessed from application code meant to run on sun4i. That is why I
> > only changed the FIFO/THLDC (offset 0x100) register behavior, since
> > that change makes U-Boot work.
> >
> > I could update the patch so all of these registers with offset bigger
> > than 0x100 log error if sun4i is selected, so that is more clear.
> > Would that be ok?
>
> Yes, I think that's a good idea, but let's do that change as a
> separate patch, so we can keep this one as it is as the bugfix.
>
> For this patch,
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
Ok, I will start preparing that separate patch for error logging for sun4i.
Since this is my first time submitting a patch, is there anything else
I need to do with this one? Thanks!
Best regards,
Strahinja
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] hw/sd: Fix sun4i allwinner-sdhost for U-Boot
2022-11-14 19:22 ` Strahinja Jankovic
@ 2022-11-15 16:02 ` Peter Maydell
2022-11-15 19:56 ` Strahinja Jankovic
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2022-11-15 16:02 UTC (permalink / raw)
To: Strahinja Jankovic; +Cc: qemu-devel, qemu-arm, Beniamino Galvani
On Mon, 14 Nov 2022 at 19:22, Strahinja Jankovic
<strahinjapjankovic@gmail.com> wrote:
> Ok, I will start preparing that separate patch for error logging for sun4i.
>
> Since this is my first time submitting a patch, is there anything else
> I need to do with this one? Thanks!
No, I'll take the patch from here. Since this is a bug fix
and we're not yet at rc2 I think we'll be able to get it
into the upcoming 7.2 release.
Thanks for submitting the patch!
-- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/sd: Fix sun4i allwinner-sdhost for U-Boot
2022-11-15 16:02 ` Peter Maydell
@ 2022-11-15 19:56 ` Strahinja Jankovic
0 siblings, 0 replies; 7+ messages in thread
From: Strahinja Jankovic @ 2022-11-15 19:56 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, qemu-arm, Beniamino Galvani
On Tue, Nov 15, 2022 at 5:02 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 14 Nov 2022 at 19:22, Strahinja Jankovic
> <strahinjapjankovic@gmail.com> wrote:
> > Ok, I will start preparing that separate patch for error logging for sun4i.
> >
> > Since this is my first time submitting a patch, is there anything else
> > I need to do with this one? Thanks!
>
> No, I'll take the patch from here. Since this is a bug fix
> and we're not yet at rc2 I think we'll be able to get it
> into the upcoming 7.2 release.
>
> Thanks for submitting the patch!
That sounds great, thank you very much!
Best regards,
Strahinja
>
> -- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-15 19:58 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-12 21:49 [PATCH] hw/sd: Fix sun4i allwinner-sdhost for U-Boot Strahinja Jankovic
2022-11-14 15:41 ` Peter Maydell
2022-11-14 17:29 ` Strahinja Jankovic
2022-11-14 17:36 ` Peter Maydell
2022-11-14 19:22 ` Strahinja Jankovic
2022-11-15 16:02 ` Peter Maydell
2022-11-15 19:56 ` Strahinja Jankovic
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).