[7/7] 4460sdp/blaze/panda: hwmod: Prevent gpio1 reset during hwmod init

Commit Message

Rajendra Nayak July 1, 2011, 2:08 a.m. UTC

For 4460sdp/blaze/panda, GPIO-7 of bank1 is used for controlling
the TPS modes, hence GPIO1 should not be reset
during init as reset will cause the TPS voltage to
drop to 0.9 V preventing the system from continuing the boot.

Signed-off-by: Nishanth Menon <[e-mail]>
Signed-off-by: Moiz Sonasath <[e-mail]>
Signed-off-by: Rajendra Nayak <[e-mail]>
---
 arch/arm/mach-omap2/board-4430sdp.c    |   14 ++++++++++++++
 arch/arm/mach-omap2/board-omap4panda.c |   14 ++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

Comments

Tony Lindgren July 1, 2011, 6:32 a.m. UTC | #1

* Rajendra Nayak <[e-mail]> [110630 19:03]:
> For 4460sdp/blaze/panda, GPIO-7 of bank1 is used for controlling
> the TPS modes, hence GPIO1 should not be reset
> during init as reset will cause the TPS voltage to
> drop to 0.9 V preventing the system from continuing the boot.

NAK for this patch. We don't want any of this in init_early.

The problem is with hwmod core code that wrongly assumes it
can just reset all devices.

We should fix the hwmod code to lazily only reset devices as they
are enabled, and only reset unused devices with late_initcall
when we have decent debug output. And the reset of unused devices
should be possible to turn off with some kernel cmdline option.

Regards,

Tony


> --- a/arch/arm/mach-omap2/board-4430sdp.c
> +++ b/arch/arm/mach-omap2/board-4430sdp.c
> @@ -36,6 +36,7 @@
>  #include <plat/usb.h>
>  #include <plat/mmc.h>
>  #include <plat/omap4-keypad.h>
> +#include <plat/omap_hwmod.h>
>  #include <video/omapdss.h>
>  
>  #include "mux.h"
> @@ -298,6 +299,19 @@ static void __init omap_4430sdp_init_early(void)
>  #ifdef CONFIG_OMAP_32K_TIMER
>  	omap2_gp_clockevent_set_gptimer(1);
>  #endif
> +	/*
> +	 * For 4460sdp/blaze, GPIO-7 of bank1 is used for controlling
> +	 * the TPS modes, hence GPIO1 should not be reset
> +	 * during init as reset will cause the TPS voltage to
> +	 * drop to 0.9 V  preventing the system from continuing the boot.
> +	 */
> +	if (cpu_is_omap446x()) {
> +		struct omap_hwmod *gpio1 = omap_hwmod_lookup("gpio1");
> +		if (gpio1)
> +			omap_hwmod_no_setup_reset(gpio1);
> +		else
> +			pr_err("%s: gpio1 hwmod lookup failed\n", __func__);
> +	}
>  }
>  
>  static struct omap_musb_board_data musb_board_data = {
> diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c
> index 0cfe200..75a847c 100644
> --- a/arch/arm/mach-omap2/board-omap4panda.c
> +++ b/arch/arm/mach-omap2/board-omap4panda.c
> @@ -40,6 +40,7 @@
>  #include <plat/common.h>
>  #include <plat/usb.h>
>  #include <plat/mmc.h>
> +#include <plat/omap_hwmod.h>
>  #include <video/omap-panel-generic-dpi.h>
>  #include "timer-gp.h"
>  
> @@ -100,6 +101,19 @@ static void __init omap4_panda_init_early(void)
>  {
>  	omap2_init_common_infrastructure();
>  	omap2_init_common_devices(NULL, NULL);
> +	/*
> +	 * For 4460panda, GPIO-7 of bank1 is used for controling
> +	 * the TPS modes, hence GPIO1 should not be reset
> +	 * during init as reset will cause the TPS voltage to
> +	 * drop to 0.9 V preventing the system from continuing the boot.
> +	 */
> +	if (cpu_is_omap446x()) {
> +		struct omap_hwmod *gpio1 = omap_hwmod_lookup("gpio1");
> +		if (gpio1)
> +			omap_hwmod_no_setup_reset(gpio1);
> +		else
> +			pr_err("%s: gpio1 hwmod lookup failed\n", __func__);
> +	}
>  }
>  
>  static const struct usbhs_omap_board_data usbhs_bdata __initconst = {
> -- 
> 1.7.4.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Walmsley July 3, 2011, 4:14 a.m. UTC | #2

Hi Tony

On Thu, 30 Jun 2011, Tony Lindgren wrote:

> NAK for this patch. We don't want any of this in init_early.
> 
> The problem is with hwmod core code that wrongly assumes it
> can just reset all devices.

I don't think the hwmod core code has any way of knowing which devices 
shouldn't be reset unless the board file specifically tells it.  Even if 
the reset happens right before the hwmod enable (as called by the device 
driver), the 4460 boards would still crash when the GPIO driver starts.

What you suggested below should allow those omap_hwmod_no_setup_reset() 
calls to live in init_machine, rather than init_early.  Hopefully that is 
acceptable?  So I did a test implementation of your idea, and learned some 
good news and bad news.

The good news is that it seems to work for the PM runtime-converted 
drivers.  omap_hwmod_no_setup_reset() calls can go into init_machine code.  
We should also be able to get rid of the postsetup code in 
mach-omap2/io.c.

The bad news is that the unused IP block reset code will reset IP blocks 
used by drivers that haven't been converted to use runtime PM.  The hwmod 
core code doesn't know that those IP blocks are in use, since 
omap_hwmod_enable() is never called for them.  The unused IP block reset 
code will then reset those blocks after the drivers have already probed, 
and the drivers are not expecting this :-)

GPTIMER and HSMMC drivers are the obvious problems in terms of getting a 
successful boot, but DSS is another one that may cause some problems here.  
There are HSMMC and DSS driver PM runtime conversion patches posted, 
hopefully they will go into mainline soon, but there are probably some 
other important drivers yet to be converted or yet to be pushed.

Here are some options that come to mind:

1. Wait until the driver runtime PM conversion is finished before doing 
   anything.  In the meantime, boards with IP blocks that can't be reset 
   - N810, TI 4460 boards - will have problems.

2. Merge the lazy/unused hwmod reset code, but prevent IP blocks 
   controlled by non-runtime PM drivers from being reset.  We'd have to
   maintain a list of these somewhere, perhaps in some common code called 
   by board file init_machine code.  Then we'd need to redact that list as 
   new driver runtime PM conversions complete.

3. Merge the lazy/unused hwmod reset code, but disable the unused hwmod 
   reset code until the driver runtime PM conversion is finished.  This
   could cause problems with driverless devices that are left configured
   by bootloaders or ROM code, and that problem would reoccur for each new
   OMAP chip.

Do you have a preference as to which approach to take?

> We should fix the hwmod code to lazily only reset devices as they
> are enabled, and only reset unused devices with late_initcall
> when we have decent debug output. And the reset of unused devices
> should be possible to turn off with some kernel cmdline option.


- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren July 4, 2011, 8:53 a.m. UTC | #3

* Paul Walmsley <[e-mail]> [110702 21:09]:
> Hi Tony
> 
> On Thu, 30 Jun 2011, Tony Lindgren wrote:
> 
> > NAK for this patch. We don't want any of this in init_early.
> > 
> > The problem is with hwmod core code that wrongly assumes it
> > can just reset all devices.
> 
> I don't think the hwmod core code has any way of knowing which devices 
> shouldn't be reset unless the board file specifically tells it.  Even if 
> the reset happens right before the hwmod enable (as called by the device 
> driver), the 4460 boards would still crash when the GPIO driver starts.
> 
> What you suggested below should allow those omap_hwmod_no_setup_reset() 
> calls to live in init_machine, rather than init_early.  Hopefully that is 
> acceptable?  So I did a test implementation of your idea, and learned some 
> good news and bad news.

Yes later on that should be fine, but preferrably not until late_initcall
so we have decent debug output even without DEBUG_LL being enabled.
 
> The good news is that it seems to work for the PM runtime-converted 
> drivers.  omap_hwmod_no_setup_reset() calls can go into init_machine code.  
> We should also be able to get rid of the postsetup code in 
> mach-omap2/io.c.
> 
> The bad news is that the unused IP block reset code will reset IP blocks 
> used by drivers that haven't been converted to use runtime PM.  The hwmod 
> core code doesn't know that those IP blocks are in use, since 
> omap_hwmod_enable() is never called for them.  The unused IP block reset 
> code will then reset those blocks after the drivers have already probed, 
> and the drivers are not expecting this :-)

I guess we should keep it disabled for now :)
 
> GPTIMER and HSMMC drivers are the obvious problems in terms of getting a 
> successful boot, but DSS is another one that may cause some problems here.  
> There are HSMMC and DSS driver PM runtime conversion patches posted, 
> hopefully they will go into mainline soon, but there are probably some 
> other important drivers yet to be converted or yet to be pushed.
> 
> Here are some options that come to mind:
> 
> 1. Wait until the driver runtime PM conversion is finished before doing 
>    anything.  In the meantime, boards with IP blocks that can't be reset 
>    - N810, TI 4460 boards - will have problems.
> 
> 2. Merge the lazy/unused hwmod reset code, but prevent IP blocks 
>    controlled by non-runtime PM drivers from being reset.  We'd have to
>    maintain a list of these somewhere, perhaps in some common code called 
>    by board file init_machine code.  Then we'd need to redact that list as 
>    new driver runtime PM conversions complete.
> 
> 3. Merge the lazy/unused hwmod reset code, but disable the unused hwmod 
>    reset code until the driver runtime PM conversion is finished.  This
>    could cause problems with driverless devices that are left configured
>    by bootloaders or ROM code, and that problem would reoccur for each new
>    OMAP chip.
> 
> Do you have a preference as to which approach to take?

I think #3 above is the safest option. How about make it only happen with
hwmod_reset=1 cmdline with 0 being the default value?

Regards,

Tony 
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Walmsley July 5, 2011, 7:40 a.m. UTC | #4

Hi Tony

On Mon, 4 Jul 2011, Tony Lindgren wrote:

> * Paul Walmsley <[e-mail]> [110702 21:09]:
> 
> > Here are some options that come to mind:
> > 
> > 1. Wait until the driver runtime PM conversion is finished before doing 
> >    anything.  In the meantime, boards with IP blocks that can't be reset 
> >    - N810, TI 4460 boards - will have problems.
> > 
> > 2. Merge the lazy/unused hwmod reset code, but prevent IP blocks 
> >    controlled by non-runtime PM drivers from being reset.  We'd have to
> >    maintain a list of these somewhere, perhaps in some common code called 
> >    by board file init_machine code.  Then we'd need to redact that list as 
> >    new driver runtime PM conversions complete.
> > 
> > 3. Merge the lazy/unused hwmod reset code, but disable the unused hwmod 
> >    reset code until the driver runtime PM conversion is finished.  This
> >    could cause problems with driverless devices that are left configured
> >    by bootloaders or ROM code, and that problem would reoccur for each new
> >    OMAP chip.
> > 
> > Do you have a preference as to which approach to take?
> 
> I think #3 above is the safest option. How about make it only happen with
> hwmod_reset=1 cmdline with 0 being the default value?

With the patch that was posted, that would disable all reset.  Probably we 
want to reset devices that have drivers with PM runtime support?  That 
would allow drivers to assume that they are starting from consistent 
device state.  It also should prevent some power management problems 
that are dependent on particular bootloaders.   How about if we add a 
second parameter, hwmod_reset_unused?  The default could be 'no' and then 
only devices with PM runtime-enabled drivers would be reset first.


- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren July 5, 2011, 10:45 a.m. UTC | #5

* Paul Walmsley <[e-mail]> [110705 00:35]:
> Hi Tony
> 
> On Mon, 4 Jul 2011, Tony Lindgren wrote:
> 
> > * Paul Walmsley <[e-mail]> [110702 21:09]:
> > 
> > > Here are some options that come to mind:
> > > 
> > > 1. Wait until the driver runtime PM conversion is finished before doing 
> > >    anything.  In the meantime, boards with IP blocks that can't be reset 
> > >    - N810, TI 4460 boards - will have problems.
> > > 
> > > 2. Merge the lazy/unused hwmod reset code, but prevent IP blocks 
> > >    controlled by non-runtime PM drivers from being reset.  We'd have to
> > >    maintain a list of these somewhere, perhaps in some common code called 
> > >    by board file init_machine code.  Then we'd need to redact that list as 
> > >    new driver runtime PM conversions complete.
> > > 
> > > 3. Merge the lazy/unused hwmod reset code, but disable the unused hwmod 
> > >    reset code until the driver runtime PM conversion is finished.  This
> > >    could cause problems with driverless devices that are left configured
> > >    by bootloaders or ROM code, and that problem would reoccur for each new
> > >    OMAP chip.
> > > 
> > > Do you have a preference as to which approach to take?
> > 
> > I think #3 above is the safest option. How about make it only happen with
> > hwmod_reset=1 cmdline with 0 being the default value?
> 
> With the patch that was posted, that would disable all reset.  Probably we 
> want to reset devices that have drivers with PM runtime support?

Can't we always reset the registered hwmods automatically one at a time when
omap_device_build is called?

> That would allow drivers to assume that they are starting from consistent 
> device state.  It also should prevent some power management problems 
> that are dependent on particular bootloaders.   How about if we add a 
> second parameter, hwmod_reset_unused?  The default could be 'no' and then 
> only devices with PM runtime-enabled drivers would be reset first.

Yes I think hwmod_reset_unsed would be a better name, but do we actually
need any other reset option in addition to that?

TOny
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Walmsley July 5, 2011, 9:47 p.m. UTC | #6

Hi Tony

On Tue, 5 Jul 2011, Tony Lindgren wrote:

> * Paul Walmsley <[e-mail]> [110705 00:35]:
> > On Mon, 4 Jul 2011, Tony Lindgren wrote:
> > > * Paul Walmsley <[e-mail]> [110702 21:09]:
> > > 
> > > > 3. Merge the lazy/unused hwmod reset code, but disable the unused hwmod 
> > > >    reset code until the driver runtime PM conversion is finished.  This
> > > >    could cause problems with driverless devices that are left configured
> > > >    by bootloaders or ROM code, and that problem would reoccur for each new
> > > >    OMAP chip.
> > >
> > > I think #3 above is the safest option. How about make it only happen with
> > > hwmod_reset=1 cmdline with 0 being the default value?
> > 
> > With the patch that was posted, that would disable all reset.  Probably we 
> > want to reset devices that have drivers with PM runtime support?
> 
> Can't we always reset the registered hwmods automatically one at a time when
> omap_device_build is called?

The experimental series that I wrote, but haven't posted yet, resets each 
IP block during the first time it's enabled -- which is probably going to 
be when omap_device_build() is called.

> > That would allow drivers to assume that they are starting from consistent 
> > device state.  It also should prevent some power management problems 
> > that are dependent on particular bootloaders.   How about if we add a 
> > second parameter, hwmod_reset_unused?  The default could be 'no' and then 
> > only devices with PM runtime-enabled drivers would be reset first.
> 
> Yes I think hwmod_reset_unsed would be a better name, but do we actually
> need any other reset option in addition to that?

If a driver doesn't reset its device(s) when it starts, then disabling all 
reset might allow the kernel to boot when the board file is missing some 
omap_hwmod_no_setup_reset() calls.

Consider the 4460 GPIO case.  If someone hasn't added in the 
omap_hwmod_no_setup_reset(gpioX_hwmod) call into the 4460 board file's 
init_machine(), then these boards would still crash on boot.

This would definitely be considered a bug in the board file, that it's 
missing that omap_hwmod_no_setup_reset() call.  But having a general 
'hwmod_no_reset' might make it easier during initial board bring-up to 
determine whether a problem is caused by reset.

In any case, it doesn't matter too much to me whether a 'hwmod_no_reset' 
option is preserved or not; this is just based on our discussion of the 
issue a few months ago.

regards

- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren July 6, 2011, 6:47 a.m. UTC | #7

* Paul Walmsley <[e-mail]> [110705 14:42]:
> On Tue, 5 Jul 2011, Tony Lindgren wrote:
> > 
> > Can't we always reset the registered hwmods automatically one at a time when
> > omap_device_build is called?
> 
> The experimental series that I wrote, but haven't posted yet, resets each 
> IP block during the first time it's enabled -- which is probably going to 
> be when omap_device_build() is called.

OK
 
> > > That would allow drivers to assume that they are starting from consistent 
> > > device state.  It also should prevent some power management problems 
> > > that are dependent on particular bootloaders.   How about if we add a 
> > > second parameter, hwmod_reset_unused?  The default could be 'no' and then 
> > > only devices with PM runtime-enabled drivers would be reset first.
> > 
> > Yes I think hwmod_reset_unsed would be a better name, but do we actually
> > need any other reset option in addition to that?
> 
> If a driver doesn't reset its device(s) when it starts, then disabling all 
> reset might allow the kernel to boot when the board file is missing some 
> omap_hwmod_no_setup_reset() calls.
> 
> Consider the 4460 GPIO case.  If someone hasn't added in the 
> omap_hwmod_no_setup_reset(gpioX_hwmod) call into the 4460 board file's 
> init_machine(), then these boards would still crash on boot.
> 
> This would definitely be considered a bug in the board file, that it's 
> missing that omap_hwmod_no_setup_reset() call.  But having a general 
> 'hwmod_no_reset' might make it easier during initial board bring-up to 
> determine whether a problem is caused by reset.
> 
> In any case, it doesn't matter too much to me whether a 'hwmod_no_reset' 
> option is preserved or not; this is just based on our discussion of the 
> issue a few months ago.

Yeah. Sounds like for passing the board specific flags for special case
devices is best done with omap2_init_devices() like you suggested.

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/mach-omap2/board-4430sdp.c b/arch/arm/mach-omap2/board-4430sdp.c
index 63de2d3..7343209 100644
--- a/arch/arm/mach-omap2/board-4430sdp.c
+++ b/arch/arm/mach-omap2/board-4430sdp.c
@@ -36,6 +36,7 @@ 
 #include <plat/usb.h>
 #include <plat/mmc.h>
 #include <plat/omap4-keypad.h>
+#include <plat/omap_hwmod.h>
 #include <video/omapdss.h>
 
 #include "mux.h"
@@ -298,6 +299,19 @@  static void __init omap_4430sdp_init_early(void)
 #ifdef CONFIG_OMAP_32K_TIMER
 	omap2_gp_clockevent_set_gptimer(1);
 #endif
+	/*
+	 * For 4460sdp/blaze, GPIO-7 of bank1 is used for controlling
+	 * the TPS modes, hence GPIO1 should not be reset
+	 * during init as reset will cause the TPS voltage to
+	 * drop to 0.9 V  preventing the system from continuing the boot.
+	 */
+	if (cpu_is_omap446x()) {
+		struct omap_hwmod *gpio1 = omap_hwmod_lookup("gpio1");
+		if (gpio1)
+			omap_hwmod_no_setup_reset(gpio1);
+		else
+			pr_err("%s: gpio1 hwmod lookup failed\n", __func__);
+	}
 }
 
 static struct omap_musb_board_data musb_board_data = {
diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c
index 0cfe200..75a847c 100644
--- a/arch/arm/mach-omap2/board-omap4panda.c
+++ b/arch/arm/mach-omap2/board-omap4panda.c
@@ -40,6 +40,7 @@ 
 #include <plat/common.h>
 #include <plat/usb.h>
 #include <plat/mmc.h>
+#include <plat/omap_hwmod.h>
 #include <video/omap-panel-generic-dpi.h>
 #include "timer-gp.h"
 
@@ -100,6 +101,19 @@  static void __init omap4_panda_init_early(void)
 {
 	omap2_init_common_infrastructure();
 	omap2_init_common_devices(NULL, NULL);
+	/*
+	 * For 4460panda, GPIO-7 of bank1 is used for controling
+	 * the TPS modes, hence GPIO1 should not be reset
+	 * during init as reset will cause the TPS voltage to
+	 * drop to 0.9 V preventing the system from continuing the boot.
+	 */
+	if (cpu_is_omap446x()) {
+		struct omap_hwmod *gpio1 = omap_hwmod_lookup("gpio1");
+		if (gpio1)
+			omap_hwmod_no_setup_reset(gpio1);
+		else
+			pr_err("%s: gpio1 hwmod lookup failed\n", __func__);
+	}
 }
 
 static const struct usbhs_omap_board_data usbhs_bdata __initconst = {