On Mon, Nov 14, 2022 at 6:36 PM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> 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>
>
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
