Re: Re: Re: Re: Increasing stabilization time in sunxi_mmc_core_init

["Jernej Škrabec" <jernej.skrabec@gmail.com>]

Dne četrtek, 21. julij 2022 ob 23:23:28 CEST je Da Xue napisal(a):
> On Thu, Jul 21, 2022 at 4:58 PM Da Xue <da@libre.computer> wrote:
> > On Thu, Jul 21, 2022 at 4:49 PM Jernej Škrabec <jernej.skrabec@gmail.com> 
wrote:
> > > Dne četrtek, 21. julij 2022 ob 22:33:09 CEST je Da Xue napisal(a):
> > > > On Thu, Jul 21, 2022 at 4:05 PM Jernej Škrabec
> > > > <jernej.skrabec@gmail.com>
> > > 
> > > wrote:
> > > > > Dne četrtek, 21. julij 2022 ob 21:56:35 CEST je Da Xue napisal(a):
> > > > > > On Thu, Jul 21, 2022 at 11:14 AM Jernej Škrabec
> > > > > > 
> > > > > > <jernej.skrabec@gmail.com> wrote:
> > > > > > > Hi!
> > > > > > > 
> > > > > > > Dne četrtek, 21. julij 2022 ob 13:28:59 CEST je Andre Przywara
> > > 
> > > napisal(a):
> > > > > > > > On 21/07/2022 12:03, Da Xue wrote:
> > > > > > > > 
> > > > > > > > Hi Da,
> > > > > > > > 
> > > > > > > > > Users were reporting non-boot on our H5 boards
> > > > > > > > > (ALL-H3-CC-H5).
> > > > > > > > > u-boot
> > > > > > > > > gets stuck in SPL with this message for SD/eMMC
> > > > > > > > > respectively.
> > > > > > > > > 
> > > > > > > > > Trying to boot from MMC1 or Trying to boot from MMC2
> > > > > > > > > 
> > > > > > > > > I tested about 20 MicroSD cards from different brands and
> > > > > > > > > some
> > > > > > > > > were
> > > > > > > > > happy and some were not. Increasing the udelay to 8-10ms in
> > > > > > > > > drivers/mmc/sunxi_mmc.c sunxi_mmc_core_init after reset
> > > > > > > > > seems to
> > > > > > > > > fix
> > > > > > > > > the issue for the MicroSD cards.
> > > > > > > > 
> > > > > > > > That's interesting, thanks for the report. I don't remember
> > > > > > > > hearing
> > > > > > > > of
> > > > > > > > issues with MMC before, at least not in the SPL.
> > > > > > > 
> > > > > > > I certainly experienced this issue on board in question. I
> > > > > > > vaguely
> > > > > > > remember
> > > > > > > asking about this issue on IRC. I also tried all sorts of
> > > > > > > tweaks, but
> > > > > > > it
> > > > > > > never occured to me that mmc reset timeout would be too short.
> > > > > > > 
> > > > > > > Da, how did you find this?
> > > > > > 
> > > > > > Someone on the Armbian forum posted that they had the same problem
> > > > > > with eMMC so I got suspicious. I scoped the MicroSD clock line and
> > > > > > realized the frequency goes high and then drops to very low as if
> > > > > > it
> > > > > > never found the card.
> > > > > > I had a hunch it was a stabilization delay and got lucky.
> > > > > > 
> > > > > > > I only test one other H5 board occasionally, namely OrangePi
> > > > > > > PC2, but
> > > > > > > I
> > > > > > > never observed such issue there. I always needed about 5
> > > > > > > attempts to
> > > > > > > boot
> > > > > > > ALL-H3-CC- H5 board, but once it's cold booted, warm boots
> > > > > > > always
> > > > > > > succeed.
> > > > > > 
> > > > > > I had run into this too so it didn't make any sense. I tried 5ms
> > > > > > and
> > > > > > it helped on some cards but not others.
> > > > > > I know the Orange Pis do not have the series resistor for ESD
> > > > > > protection of the SD GPIOs but that shouldn't affect this.
> > > > > > So...who knows?
> > > > > > 
> > > > > > > Best regards,
> > > > > > > Jernej
> > > > > > > 
> > > > > > > > It's a bit odd that waiting after the *controller* reset
> > > > > > > > should
> > > > > > > > affect
> > > > > > > > SD cards, and 1ms seems plenty for just the reset.
> > > > > > > > I just checked and at least the SOFT_RESET and FIFO_RESET bits
> > > > > > > > are
> > > > > > > > self
> > > > > > > > clearing. Can you try to use wait_for_bit_le32() to wait for
> > > > > > > > those
> > > > > > > > parts
> > > > > > > > to finish? See sun8i_emac_eth_start() for an example.
> 
> After more extensive testing with 5 MicroSD cards, 2 of them are still
> inconsistent with waiting for SOFT_RESET and FIFO_RESET vs a hard 20ms
> delay. I even tried putting a delay of 1ms after they cleared to no
> avail.

Fixed delay it is, then. Just add a comment with explanation why. People 
looking for boot time optimization might lower it.

> I'm curious how larger 1TB MicroSD cards behave now but I don't have
> any samples.
> 
> > > > > > I tested some more and here's the data:
> > > > > > sandisk ultra 64gb       9/20 with 1ms,   20/20 with 10ms
> > > > > > sandisk ultra 16gb       2/20 with 1ms,   20/20 with 10ms
> > > > > > sandisk extreme 16gb 6/20 with 10ms, 20/20 with 20ms
> > > > > > Given this, I don't think it's an issue with the bit set delays.
> > > > > > Might
> > > > > > need more than 10ms even. I didn't change the udelay in probe.
> > > > > 
> > > > > Idea here is that we wouldn't need to determine the appropriate
> > > > > delay, but
> > > > > instead, wait_for_bit_le32() would monitor reset bit and continue
> > > > > only
> > > > > after reset bit would clear itself. Hopefully that happens after
> > > > > everything is stable.
> > > > 
> > > > I changed it to 50ms delay
> > > > 
> > > > writel(SUNXI_MMC_GCTRL_RESET, &priv->reg->gctrl);
> > > > if (wait_for_bit_le32( &priv->reg->gctrl,
> > > > SUNXI_MMC_GCTRL_RESET, false, 50, false)) {
> > > > printf("%s: Timeout\n", __func__);
> > > > return ret;
> > > > }
> > > > 
> > > > and that seems to work. Shall I send the patch?
> > > 
> > > Certainly, that's major improvement. Just 1 small nitpick - you don't
> > > need to introduce ret variable, just return error -ETIMEDOUT directly.
> > 
> > OK
> > 
> > > Maybe timeout can be raised even to 100 ms, now that it continues as
> > > soon as possible. Better to be on the completely safe side. But I'll
> > > leave that decision to you and Andre.
> > 
> > I'll retest everything and check about changing the probe reset as
> > well. If everything works, I'll submit the patch.
> > 
> > > Best regards,
> > > Jernej
> > > 
> > > > > Best regards,
> > > > > Jernej
> > > > > 
> > > > > > > > And since you mentioned it's card related: can you check
> > > > > > > > whether the
> > > > > > > > delay is actually needed somewhere else, later? At some point
> > > > > > > > where
> > > > > > > > we
> > > > > > > > wait to the card to response, for instance?
> > > > > > > > 
> > > > > > > > I am not against taking this patch, if it fixes problems for
> > > > > > > > you,
> > > > > > > > but
> > > > > > > > just want to avoid that it papers over other issues.
> > > > > > > > 
> > > > > > > > Cheers,
> > > > > > > > Andre
> > > > > > > > 
> > > > > > > > > Author: Da Xue <da@libre.computer>
> > > > > > > > > Date:   Wed Jul 20 19:11:55 2022 -0400
> > > > > > > > > 
> > > > > > > > >      sunxi: raise stabilization time for mmc from 1ms to 8ms
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/mmc/sunxi_mmc.c
> > > > > > > > > b/drivers/mmc/sunxi_mmc.c
> > > > > > > > > index 1bb7b6d0e9..499e057725 100644
> > > > > > > > > --- a/drivers/mmc/sunxi_mmc.c
> > > > > > > > > +++ b/drivers/mmc/sunxi_mmc.c
> > > > > > > > > @@ -297,7 +297,7 @@ static int sunxi_mmc_core_init(struct
> > > > > > > > > mmc
> > > > > > > > > *mmc)
> > > > > > > > > 
> > > > > > > > >          /* Reset controller */
> > > > > > > > >          writel(SUNXI_MMC_GCTRL_RESET, &priv->reg->gctrl);
> > > > > > > > > 
> > > > > > > > > -       udelay(1000);
> > > > > > > > > +       udelay(8000);
> > > > > > 
> > > > > > This might need to be even higher. Like 20ms.
> > > > > > 
> > > > > > > > >          return 0;
> > > > > > > > >   
> > > > > > > > >   }
> > > > > > > > > 
> > > > > > > > > I don't know the implications of this change so I am seeking
> > > > > > > > > feedback.
> > > > > > > > > Are other boards having this issue as well or is it specific
> > > > > > > > > to
> > > > > > > > > our
> > > > > > > > > hardware?
> > > > > > > > > 
> > > > > > > > > Best,
> > > > > > > > > Da