Bisected: omap_hsmmc 3.3V IO voltage incompatible with N900 (Was: Re: Bisected: mmc cause reboot loops on N900)

[Pali Rohár]

On Tuesday 26 May 2020 19:49:54 Pali Rohár wrote:
> On Thursday 07 May 2020 17:19:38 Pali Rohár wrote:
> > On Thursday 07 May 2020 19:10:14 Faiz Abbas wrote:
> > > On 26/04/20 3:59 am, Pali Rohár wrote:
> > > > On Sunday 26 April 2020 00:20:07 Pali Rohár wrote:
> > > >> On Saturday 25 April 2020 23:26:15 Pali Rohár wrote:
> > > >>> Now I tried git bisect and here is problematic commit which caused whole
> > > >>> reboot loop:
> > > >>>
> > > >>> 04a2ea248f58b3b6216d0cd0a6b8698df8b14355 is the first bad commit
> > > >>> commit 04a2ea248f58b3b6216d0cd0a6b8698df8b14355
> > > >>> Author: Jean-Jacques Hiblot <jjhiblot at ti.com>
> > > >>> Date:   Thu Sep 21 16:30:08 2017 +0200
> > > >>>
> > > >>>     mmc: disable UHS modes if Vcc cannot be switched on and off
> > > >>>     
> > > >>>     If a power cycle cannot be done on Vcc, it is safer not to try the UHS
> > > >>>     modes because we wouldn't be able to recover from an error occurring
> > > >>>     during the UHS initialization.
> > > >>>     
> > > >>>     Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com>
> > > >>>
> > > >>> :040000 040000 04de51428c8311a4b2fb3ad876ac3f6071ab57ee ea7a7959a4bd591c92a2c3d413d5643a8457d2ff M      drivers
> > > >>> :040000 040000 03f639baf2a2f55003cb750981fd8accc5b4a993 fbcb9607d37959f0b5240f5d727133f58cc35379 M      include
> > > >>>
> > > >>> It changes only core mmc code, nothing platform / board specific.
> > > >>> U-Boot compiled from commit before above has fully working eMMC access
> > > >>> on real Nokia N900. I can read files on FAT eMMC partition without any
> > > >>> problem.
> > > >>>
> > > >>> I'm not sure what is happening here, but it looks like that omap hs mmc
> > > >>> driver used on Nokia N900 is maybe not correctly hooked for UHS support.
> > > >>>
> > > >>> The most suspicious it that this problem cannot be reproduced in qemu
> > > >>> n900 emulator. It happens only on real N900 hw.
> > > >>
> > > >> I took main change from above commit, reverted it on master and U-Boot
> > > >> stopped crashing! No reboot loop anymore. Here is change which fixes
> > > >> reboot loop on Nokia N900:
> > > >>
> > > >> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> > > >> index 523c055967..d07c7745da 100644
> > > >> --- a/drivers/mmc/mmc.c
> > > >> +++ b/drivers/mmc/mmc.c
> > > >> @@ -2786,18 +2786,7 @@ int mmc_get_op_cond(struct mmc *mmc)
> > > >>  		      MMC_QUIRK_RETRY_APP_CMD;
> > > >>  #endif
> > > >>  
> > > >> -	err = mmc_power_cycle(mmc);
> > > >> -	if (err) {
> > > >> -		/*
> > > >> -		 * if power cycling is not supported, we should not try
> > > >> -		 * to use the UHS modes, because we wouldn't be able to
> > > >> -		 * recover from an error during the UHS initialization.
> > > >> -		 */
> > > >> -		pr_debug("Unable to do a full power cycle. Disabling the UHS modes for safety\n");
> > > >> -		uhs_en = false;
> > > >> -		mmc->host_caps &= ~UHS_CAPS;
> > > >> -		err = mmc_power_on(mmc);
> > > >> -	}
> > > >> +	err = mmc_power_on(mmc);
> > > >>  	if (err)
> > > >>  		return err;
> > > >>  
> > > >>
> > > >> Jean, do you have any idea what is happening in above code? It looks
> > > >> like something is broken in power cycle / power on sequence if it cause
> > > >> "data abort" and reboot of board.
> > > >>
> > > >>
> > > >> But even with above my change eMMC on N900 cannot be initialized...
> > > >> I'm running second git bisect with applying above change on every
> > > >> commit. It is in progress now.
> > > > 
> > > > And second bisect finished. Result is:
> > > > 
> > > > d2c05f50e12f87128597a28146de7092aaa847c3 is the first bad commit
> > > > commit d2c05f50e12f87128597a28146de7092aaa847c3
> > > > Author: Faiz Abbas <faiz_abbas at ti.com>
> > > > Date:   Fri Apr 5 14:18:46 2019 +0530
> > > > 
> > > >     mmc: omap_hsmmc: Set 3.3V for IO voltage
> > > >     
> > > >     Pbias voltage should match the IO voltage set for the SD card. With the
> > > >     latest pbias change to 3.3V, update the capabilities and IO voltages
> > > >     settings to 3.3V.
> > > >     
> > > >     Signed-off-by: Faiz Abbas <faiz_abbas at ti.com>
> > > > 
> > > > :040000 040000 819148217b64a6beaf8247464de6b4384d4581a4 e4fd9288ddb794f33596339813d5386f3bed8fd7 M      drivers
> > > > 
> > > > I revered above commit on top of master and eMMC on Nokia N900 finally
> > > > started working again!
> > > > 
> > > > Faiz, what is the reason for above commit? It basically changes in omap
> > > > hs mmc driver IO voltage from 3.0V to 3.3V. And seems this change is
> > > > incompatible with Nokia N900. Are there any problems with 3.0V on some
> > > > omap3 boards? If not I would vote for revering that commit. Or at least
> > > > making IO voltage configurable.
> > > > 
> > > 
> > > For the power_cycle patch, it looks like there is a null pointer dereference
> > > that might be causing the reboot loops (probably because your platform doesn't
> > > have DM_MMC enabled) Can you add a few more prints to see where the data abort
> > > comes from exactly?
> > 
> > Hello Faiz!
> > 
> > Sorry, but this is problematic. Because of reboot loops I cannot read
> > what exactly is put on the display and reboot cause clearing display.
> > 
> > Month ago I was able to detect that problem is somewhere in function
> > mmc_set_ios() from mmc.c file. I put long debug line prior and also
> > after mmc_set_ios() call. And it was printed only prior. Not after.
> > So I think NULL pointer dereference is in mmc_set_ios() function.
> > 
> > > For the second patch IO voltage patch, what exactly do you see without reverting it?
> > > Are you able to reach prompt but mmc commands fail?
> > 
> > Yes, after reverting 04a2ea248f58b3b6216d0cd0a6b8698df8b14355 there is
> > no reboot loop anymore. Just mmc commands fail and eMMC is not detected
> > at all.
> > 
> > > Its possible that your platform requires a pbias of 3.0V to work.
> 
> Hello Faiz!
> 
> Now I figured out what is the root cause of second 3.0V vs 3.3V problem.
> 
> In commit d2c05f50e12f87128597a28146de7092aaa847c3 you forgot to replace
> one usage of 3.0V by 3.3V. Below is patch which changes also this last
> one usage. Applying it has same effect on Nokia N900 as reverting
> that problematic commit d2c05f50e12f87128597a28146de7092aaa847c3:
> 
> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
> index 8636cd713a..dc26e54795 100644
> --- a/drivers/mmc/omap_hsmmc.c
> +++ b/drivers/mmc/omap_hsmmc.c
> @@ -840,7 +840,7 @@ static int omap_hsmmc_init_setup(struct mmc *mmc)
>  	omap_hsmmc_conf_bus_power(mmc, (reg_val & VS33_3V3SUP) ?
>  			  MMC_SIGNAL_VOLTAGE_330 : MMC_SIGNAL_VOLTAGE_180);
>  #else
> -	writel(DTW_1_BITMODE | SDBP_PWROFF | SDVS_3V0, &mmc_base->hctl);
> +	writel(DTW_1_BITMODE | SDBP_PWROFF | SDVS_3V3, &mmc_base->hctl);
>  	writel(readl(&mmc_base->capa) | VS33_3V3SUP | VS18_1V8SUP,
>  		&mmc_base->capa);
>  #endif
> 
> So eMMC on real N900 is working fine either with 3.0V or 3.3V. But 3.3V
> needs to be configured on all places.

Hello! Could you please take a look at this issue and my above fix?

> First problem with reboot loop and crash in mmc_set_ios() still remains.
> Reverting 04a2ea248f58b3b6216d0cd0a6b8698df8b14355 fixes it.