Re: [PATCH] hw/sd: Fix sun4i allwinner-sdhost for U-Boot

[Strahinja Jankovic]

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