| Message ID | 20200809160303.8773-1-pali@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | Lokesh Vutla |
| Headers | show |
| Series | Revert "mmc: disable UHS modes if Vcc cannot be switched on and off" | expand |
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
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
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?
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
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
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
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
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
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;
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(-)