[PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched on and off"

[PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched on and off"

[Pali Rohár]

This reverts commit 04a2ea248f58b3b6216d0cd0a6b8698df8b14355.

That commit is causing reboot loops on Nokia N900 and basically make U-Boot
on Nokia N900 unusable. Revert it for now until problem is solved.

After reverting that commit U-Boot on Nokia N900 is working again.

Signed-off-by: Pali Roh?r <pali@kernel.org>
---
 drivers/mmc/mmc.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index d79cdef62e..48c1629a19 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -2800,18 +2800,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;
 
-- 
2.20.1

[PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched on and off"

[Peng Fan]

Faiz, Jean

> Subject: [PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched
> on and off"

Got time to take a look?

Thanks,
Peng.

> 
> This reverts commit 04a2ea248f58b3b6216d0cd0a6b8698df8b14355.
> 
> That commit is causing reboot loops on Nokia N900 and basically make
> U-Boot on Nokia N900 unusable. Revert it for now until problem is solved.
> 
> After reverting that commit U-Boot on Nokia N900 is working again.
> 
> Signed-off-by: Pali Roh?r <pali@kernel.org>
> ---
>  drivers/mmc/mmc.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index
> d79cdef62e..48c1629a19 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -2800,18 +2800,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;
> 
> --
> 2.20.1

[PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched on and off"

[Faiz Abbas]

Pali, Peng,

On 10/08/20 6:25 am, Peng Fan wrote:
> Faiz, Jean
> 
>> Subject: [PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched
>> on and off"
> 
> Got time to take a look?
> 

When this issue was reported in the last thread, Pali said that he was unable to get
prints because of the constant reboot loops? Shouldn't it be easy to put while(1)
at different points in this execution and see which step causes the reboot loop?

Thanks,
Faiz

[PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched on and off"

[Pali Rohár]

On Tuesday 11 August 2020 08:39:24 Faiz Abbas wrote:
> Pali, Peng,
> 
> On 10/08/20 6:25 am, Peng Fan wrote:
> > Faiz, Jean
> > 
> >> Subject: [PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched
> >> on and off"
> > 
> > Got time to take a look?
> > 
> 
> When this issue was reported in the last thread, Pali said that he was unable to get
> prints because of the constant reboot loops? Shouldn't it be easy to put while(1)
> at different points in this execution and see which step causes the reboot loop?

In May in that last thread I wrote details which I was able to gather:

    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.

I could try with that while(1) loop to print detailed log message and
read/capture it. But what information for debugging you need? Or what do
you want to print which could help you?

[PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched on and off"

[Faiz Abbas]

Pali,

On 11/08/20 1:19 pm, Pali Roh?r wrote:
> On Tuesday 11 August 2020 08:39:24 Faiz Abbas wrote:
>> Pali, Peng,
>>
>> On 10/08/20 6:25 am, Peng Fan wrote:
>>> Faiz, Jean
>>>
>>>> Subject: [PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched
>>>> on and off"
>>>
>>> Got time to take a look?
>>>
>>
>> When this issue was reported in the last thread, Pali said that he was unable to get
>> prints because of the constant reboot loops? Shouldn't it be easy to put while(1)
>> at different points in this execution and see which step causes the reboot loop?
> 
> In May in that last thread I wrote details which I was able to gather:
> 
>     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.
> 
> I could try with that while(1) loop to print detailed log message and
> read/capture it. But what information for debugging you need? Or what do
> you want to print which could help you?
> 

You can continue to bisect into omap_hsmmc_set_ios() to see at what point the NULL pointer
gets triggered and what the NULL pointer is. Also can you point to your config file?

Thanks,
Faiz

[PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched on and off"

[Pali Rohár]

On Tuesday 11 August 2020 21:33:19 Faiz Abbas wrote:
> Pali,
> 
> On 11/08/20 1:19 pm, Pali Roh?r wrote:
> > On Tuesday 11 August 2020 08:39:24 Faiz Abbas wrote:
> >> Pali, Peng,
> >>
> >> On 10/08/20 6:25 am, Peng Fan wrote:
> >>> Faiz, Jean
> >>>
> >>>> Subject: [PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched
> >>>> on and off"
> >>>
> >>> Got time to take a look?
> >>>
> >>
> >> When this issue was reported in the last thread, Pali said that he was unable to get
> >> prints because of the constant reboot loops? Shouldn't it be easy to put while(1)
> >> at different points in this execution and see which step causes the reboot loop?
> > 
> > In May in that last thread I wrote details which I was able to gather:
> > 
> >     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.
> > 
> > I could try with that while(1) loop to print detailed log message and
> > read/capture it. But what information for debugging you need? Or what do
> > you want to print which could help you?
> > 
> 
> You can continue to bisect into omap_hsmmc_set_ios() to see at what point the NULL pointer
> gets triggered and what the NULL pointer is.

I could try it, but I do not think I would be able to gather more data.
I will try to find free time for this debugging on device either at the
end of this week or end of next week.

As I wrote in previous thread, main issue which makes it hard to debug
is that this error is not triggered in emulator.

> Also can you point to your config file?

I'm using standard config file without any modifications. It is:
configs/nokia_rx51_defconfig

In case you are interested, I'm compiling u-boot by commands:

export ARCH=arm
export CROSS_COMPILE=arm-linux-gnueabi-
make nokia_rx51_config
make -j12 u-boot.bin

[PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched on and off"

[Pali Rohár]

On Tuesday 11 August 2020 18:27:23 Pali Roh?r wrote:
> On Tuesday 11 August 2020 21:33:19 Faiz Abbas wrote:
> > Pali,
> > 
> > On 11/08/20 1:19 pm, Pali Roh?r wrote:
> > > On Tuesday 11 August 2020 08:39:24 Faiz Abbas wrote:
> > >> Pali, Peng,
> > >>
> > >> On 10/08/20 6:25 am, Peng Fan wrote:
> > >>> Faiz, Jean
> > >>>
> > >>>> Subject: [PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched
> > >>>> on and off"
> > >>>
> > >>> Got time to take a look?
> > >>>
> > >>
> > >> When this issue was reported in the last thread, Pali said that he was unable to get
> > >> prints because of the constant reboot loops? Shouldn't it be easy to put while(1)
> > >> at different points in this execution and see which step causes the reboot loop?
> > > 
> > > In May in that last thread I wrote details which I was able to gather:
> > > 
> > >     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.
> > > 
> > > I could try with that while(1) loop to print detailed log message and
> > > read/capture it. But what information for debugging you need? Or what do
> > > you want to print which could help you?
> > > 
> > 
> > You can continue to bisect into omap_hsmmc_set_ios() to see at what point the NULL pointer
> > gets triggered and what the NULL pointer is.
> 
> I could try it, but I do not think I would be able to gather more data.
> I will try to find free time for this debugging on device either at the
> end of this week or end of next week.

Here are more details. Crash is in omap_hsmmc_stop_clock() function when
trying to dereference mmc_base->sysctl.

Call trace is:

mmc_set_ios --> mmc->cfg->ops->set_ios --> omap_hsmmc_set_ios --> omap_hsmmc_stop_clock

I applied following diff

diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
index 715eee0e3e..d4bbfd7b97 100644
--- a/drivers/mmc/omap_hsmmc.c
+++ b/drivers/mmc/omap_hsmmc.c
@@ -22,6 +22,8 @@
  * MA 02111-1307 USA
  */
 
+#define DEBUG
+
 #include <config.h>
 #include <common.h>
 #include <cpu_func.h>
@@ -1335,7 +1337,16 @@ static int mmc_write_data(struct hsmmc *mmc_base, const char *buf,
 #endif
 static void omap_hsmmc_stop_clock(struct hsmmc *mmc_base)
 {
+	debug("omap_hsmmc_stop_clock: before writel\n");
+	debug("mmc_base=%p\n", mmc_base);
+	debug("barrier\n");
+	asm volatile ("");
+	debug("mmc_base->sysctl=%x\n", mmc_base->sysctl);
+	debug("barrier\n");
+	asm volatile ("");
+	debug("readl(&mmc_base->sysctl)=%x\n", readl(&mmc_base->sysctl));
 	writel(readl(&mmc_base->sysctl) & ~CEN_ENABLE, &mmc_base->sysctl);
+	debug("omap_hsmmc_stop_clock: after writel\n");
 }
 
 static void omap_hsmmc_start_clock(struct hsmmc *mmc_base)

and I see that first word "barrier" is written. There is no
"mmc_base->sysctl=" string on output display.

If you are interested, mmc_base has value 480b4000.

That is probably all what I can do.

> As I wrote in previous thread, main issue which makes it hard to debug
> is that this error is not triggered in emulator.
> 
> > Also can you point to your config file?
> 
> I'm using standard config file without any modifications. It is:
> configs/nokia_rx51_defconfig
> 
> In case you are interested, I'm compiling u-boot by commands:
> 
> export ARCH=arm
> export CROSS_COMPILE=arm-linux-gnueabi-
> make nokia_rx51_config
> make -j12 u-boot.bin

[PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched on and off"

[Pali Rohár]

Hello Faiz! Do you need any other details for this issue?

On Friday 14 August 2020 22:16:00 Pali Roh?r wrote:
> On Tuesday 11 August 2020 18:27:23 Pali Roh?r wrote:
> > On Tuesday 11 August 2020 21:33:19 Faiz Abbas wrote:
> > > Pali,
> > > 
> > > On 11/08/20 1:19 pm, Pali Roh?r wrote:
> > > > On Tuesday 11 August 2020 08:39:24 Faiz Abbas wrote:
> > > >> Pali, Peng,
> > > >>
> > > >> On 10/08/20 6:25 am, Peng Fan wrote:
> > > >>> Faiz, Jean
> > > >>>
> > > >>>> Subject: [PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched
> > > >>>> on and off"
> > > >>>
> > > >>> Got time to take a look?
> > > >>>
> > > >>
> > > >> When this issue was reported in the last thread, Pali said that he was unable to get
> > > >> prints because of the constant reboot loops? Shouldn't it be easy to put while(1)
> > > >> at different points in this execution and see which step causes the reboot loop?
> > > > 
> > > > In May in that last thread I wrote details which I was able to gather:
> > > > 
> > > >     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.
> > > > 
> > > > I could try with that while(1) loop to print detailed log message and
> > > > read/capture it. But what information for debugging you need? Or what do
> > > > you want to print which could help you?
> > > > 
> > > 
> > > You can continue to bisect into omap_hsmmc_set_ios() to see at what point the NULL pointer
> > > gets triggered and what the NULL pointer is.
> > 
> > I could try it, but I do not think I would be able to gather more data.
> > I will try to find free time for this debugging on device either at the
> > end of this week or end of next week.
> 
> Here are more details. Crash is in omap_hsmmc_stop_clock() function when
> trying to dereference mmc_base->sysctl.
> 
> Call trace is:
> 
> mmc_set_ios --> mmc->cfg->ops->set_ios --> omap_hsmmc_set_ios --> omap_hsmmc_stop_clock
> 
> I applied following diff
> 
> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
> index 715eee0e3e..d4bbfd7b97 100644
> --- a/drivers/mmc/omap_hsmmc.c
> +++ b/drivers/mmc/omap_hsmmc.c
> @@ -22,6 +22,8 @@
>   * MA 02111-1307 USA
>   */
>  
> +#define DEBUG
> +
>  #include <config.h>
>  #include <common.h>
>  #include <cpu_func.h>
> @@ -1335,7 +1337,16 @@ static int mmc_write_data(struct hsmmc *mmc_base, const char *buf,
>  #endif
>  static void omap_hsmmc_stop_clock(struct hsmmc *mmc_base)
>  {
> +	debug("omap_hsmmc_stop_clock: before writel\n");
> +	debug("mmc_base=%p\n", mmc_base);
> +	debug("barrier\n");
> +	asm volatile ("");
> +	debug("mmc_base->sysctl=%x\n", mmc_base->sysctl);
> +	debug("barrier\n");
> +	asm volatile ("");
> +	debug("readl(&mmc_base->sysctl)=%x\n", readl(&mmc_base->sysctl));
>  	writel(readl(&mmc_base->sysctl) & ~CEN_ENABLE, &mmc_base->sysctl);
> +	debug("omap_hsmmc_stop_clock: after writel\n");
>  }
>  
>  static void omap_hsmmc_start_clock(struct hsmmc *mmc_base)
> 
> and I see that first word "barrier" is written. There is no
> "mmc_base->sysctl=" string on output display.
> 
> If you are interested, mmc_base has value 480b4000.
> 
> That is probably all what I can do.
> 
> > As I wrote in previous thread, main issue which makes it hard to debug
> > is that this error is not triggered in emulator.
> > 
> > > Also can you point to your config file?
> > 
> > I'm using standard config file without any modifications. It is:
> > configs/nokia_rx51_defconfig
> > 
> > In case you are interested, I'm compiling u-boot by commands:
> > 
> > export ARCH=arm
> > export CROSS_COMPILE=arm-linux-gnueabi-
> > make nokia_rx51_config
> > make -j12 u-boot.bin

[PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched on and off"

[Pali Rohár]

Ivo found workaround for this issue. I will send a patch for it.

On Friday 25 September 2020 00:03:24 Pali Roh?r wrote:
> Hello Faiz! Do you need any other details for this issue?
> 
> On Friday 14 August 2020 22:16:00 Pali Roh?r wrote:
> > On Tuesday 11 August 2020 18:27:23 Pali Roh?r wrote:
> > > On Tuesday 11 August 2020 21:33:19 Faiz Abbas wrote:
> > > > Pali,
> > > > 
> > > > On 11/08/20 1:19 pm, Pali Roh?r wrote:
> > > > > On Tuesday 11 August 2020 08:39:24 Faiz Abbas wrote:
> > > > >> Pali, Peng,
> > > > >>
> > > > >> On 10/08/20 6:25 am, Peng Fan wrote:
> > > > >>> Faiz, Jean
> > > > >>>
> > > > >>>> Subject: [PATCH] Revert "mmc: disable UHS modes if Vcc cannot be switched
> > > > >>>> on and off"
> > > > >>>
> > > > >>> Got time to take a look?
> > > > >>>
> > > > >>
> > > > >> When this issue was reported in the last thread, Pali said that he was unable to get
> > > > >> prints because of the constant reboot loops? Shouldn't it be easy to put while(1)
> > > > >> at different points in this execution and see which step causes the reboot loop?
> > > > > 
> > > > > In May in that last thread I wrote details which I was able to gather:
> > > > > 
> > > > >     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.
> > > > > 
> > > > > I could try with that while(1) loop to print detailed log message and
> > > > > read/capture it. But what information for debugging you need? Or what do
> > > > > you want to print which could help you?
> > > > > 
> > > > 
> > > > You can continue to bisect into omap_hsmmc_set_ios() to see at what point the NULL pointer
> > > > gets triggered and what the NULL pointer is.
> > > 
> > > I could try it, but I do not think I would be able to gather more data.
> > > I will try to find free time for this debugging on device either at the
> > > end of this week or end of next week.
> > 
> > Here are more details. Crash is in omap_hsmmc_stop_clock() function when
> > trying to dereference mmc_base->sysctl.
> > 
> > Call trace is:
> > 
> > mmc_set_ios --> mmc->cfg->ops->set_ios --> omap_hsmmc_set_ios --> omap_hsmmc_stop_clock
> > 
> > I applied following diff
> > 
> > diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
> > index 715eee0e3e..d4bbfd7b97 100644
> > --- a/drivers/mmc/omap_hsmmc.c
> > +++ b/drivers/mmc/omap_hsmmc.c
> > @@ -22,6 +22,8 @@
> >   * MA 02111-1307 USA
> >   */
> >  
> > +#define DEBUG
> > +
> >  #include <config.h>
> >  #include <common.h>
> >  #include <cpu_func.h>
> > @@ -1335,7 +1337,16 @@ static int mmc_write_data(struct hsmmc *mmc_base, const char *buf,
> >  #endif
> >  static void omap_hsmmc_stop_clock(struct hsmmc *mmc_base)
> >  {
> > +	debug("omap_hsmmc_stop_clock: before writel\n");
> > +	debug("mmc_base=%p\n", mmc_base);
> > +	debug("barrier\n");
> > +	asm volatile ("");
> > +	debug("mmc_base->sysctl=%x\n", mmc_base->sysctl);
> > +	debug("barrier\n");
> > +	asm volatile ("");
> > +	debug("readl(&mmc_base->sysctl)=%x\n", readl(&mmc_base->sysctl));
> >  	writel(readl(&mmc_base->sysctl) & ~CEN_ENABLE, &mmc_base->sysctl);
> > +	debug("omap_hsmmc_stop_clock: after writel\n");
> >  }
> >  
> >  static void omap_hsmmc_start_clock(struct hsmmc *mmc_base)
> > 
> > and I see that first word "barrier" is written. There is no
> > "mmc_base->sysctl=" string on output display.
> > 
> > If you are interested, mmc_base has value 480b4000.
> > 
> > That is probably all what I can do.
> > 
> > > As I wrote in previous thread, main issue which makes it hard to debug
> > > is that this error is not triggered in emulator.
> > > 
> > > > Also can you point to your config file?
> > > 
> > > I'm using standard config file without any modifications. It is:
> > > configs/nokia_rx51_defconfig
> > > 
> > > In case you are interested, I'm compiling u-boot by commands:
> > > 
> > > export ARCH=arm
> > > export CROSS_COMPILE=arm-linux-gnueabi-
> > > make nokia_rx51_config
> > > make -j12 u-boot.bin