On Mon, 14 Nov 2022 at 17:29, Strahinja Jankovic
<strahinjapjanko...@gmail.com> wrote:
>
> Hi,
>
> Thank you for your reply.
>
> On Mon, Nov 14, 2022 at 4:42 PM Peter Maydell <peter.mayd...@linaro.org>
> wrote:
> >
> > On Sat, 12 Nov 2022 at 21:49, Strahinja Jankovic
> > <strahinjapjanko...@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.janko...@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.mayd...@linaro.org>
thanks
-- PMM
