Increasing stabilization time in sunxi_mmc_core_init

[Jernej Škrabec]

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 at 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 at 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.
> > > 
> > > 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.

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.

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 at 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