ARM: omap4: gpio: fix setting IRQWAKEN bits
diff mbox

Message ID 1307214239-16316-1-git-send-email-ccross@android.com
State New, archived
Headers show

Commit Message

Colin Cross June 4, 2011, 7:03 p.m. UTC
Setting the IRQWAKEN bit was overwriting previous IRQWAKEN bits,
causing only the last bit set to take effect, resulting in lost
wakeups when the GPIO controller is in idle.

Replace direct writes to IRQWAKEN with writes to SETWKUENA and
CLEARWKUEN.

Signed-off-by: Colin Cross <ccross@android.com>
---
 arch/arm/plat-omap/gpio.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

Comments

Colin Cross June 5, 2011, 2:37 a.m. UTC | #1
On Sat, Jun 4, 2011 at 12:03 PM, Colin Cross <ccross@android.com> wrote:
> Setting the IRQWAKEN bit was overwriting previous IRQWAKEN bits,
> causing only the last bit set to take effect, resulting in lost
> wakeups when the GPIO controller is in idle.
>
> Replace direct writes to IRQWAKEN with writes to SETWKUENA and
> CLEARWKUEN.
>
> Signed-off-by: Colin Cross <ccross@android.com>
> ---
>  arch/arm/plat-omap/gpio.c |   14 +++++---------
>  1 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index c985652..23ac7b6 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -539,7 +539,6 @@ static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
>  {
>        void __iomem *base = bank->base;
>        u32 gpio_bit = 1 << gpio;
> -       u32 val;
>
>        if (cpu_is_omap44xx()) {
>                MOD_REG_BIT(OMAP4_GPIO_LEVELDETECT0, gpio_bit,
> @@ -563,14 +562,11 @@ static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
>        if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
>                if (cpu_is_omap44xx()) {
>                        if (trigger != 0)
> -                               __raw_writel(1 << gpio, bank->base+
> -                                               OMAP4_GPIO_IRQWAKEN0);
> -                       else {
> -                               val = __raw_readl(bank->base +
> -                                                       OMAP4_GPIO_IRQWAKEN0);
> -                               __raw_writel(val & (~(1 << gpio)), bank->base +
> -                                                        OMAP4_GPIO_IRQWAKEN0);
> -                       }
> +                               __raw_writel(gpio_bit,
> +                                       bank->base + OMAP4_GPIO_SETWKUENA);
> +                       else
> +                               __raw_writel(gpio_bit,
> +                                       bank->base + OMAP4_GPIO_CLEARWKUENA);

Todd pointed out that the OMAP4 TRM says not to use SETWKUENA and
CLEARWKUENA.  I'll send another patch that applies to v3.0-rc1 that
uses MOD_REG_BIT on IRQWAKEN_0, and another patch that adds the
necessary locking around the read-modify-writes in
_set_gpio_triggering.

>                } else {
>                        /*
>                         * GPIO wakeup request can only be generated on edge
> --
> 1.7.4.1
>
>
Santosh Shilimkar June 6, 2011, 7:02 a.m. UTC | #2
On 6/5/2011 12:33 AM, Colin Cross wrote:
> Setting the IRQWAKEN bit was overwriting previous IRQWAKEN bits,
> causing only the last bit set to take effect, resulting in lost
> wakeups when the GPIO controller is in idle.
>
> Replace direct writes to IRQWAKEN with writes to SETWKUENA and
> CLEARWKUEN.
>
> Signed-off-by: Colin Cross<ccross@android.com>

Looks good. This needs to be fixed on top of the clean-up
branch where GPIO movemnt to driver/gpio/* happening.

For this change,
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Tarun Kanti DebBarma June 6, 2011, 7:23 a.m. UTC | #3
> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Shilimkar, Santosh
> Sent: Monday, June 06, 2011 12:32 PM
> To: Colin Cross
> Cc: linux-omap@vger.kernel.org; Tony Lindgren; Russell King; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH] ARM: omap4: gpio: fix setting IRQWAKEN bits
> 
> On 6/5/2011 12:33 AM, Colin Cross wrote:
> > Setting the IRQWAKEN bit was overwriting previous IRQWAKEN bits,
> > causing only the last bit set to take effect, resulting in lost
> > wakeups when the GPIO controller is in idle.
> >
> > Replace direct writes to IRQWAKEN with writes to SETWKUENA and
> > CLEARWKUEN.
> >
> > Signed-off-by: Colin Cross<ccross@android.com>
> 
> Looks good. This needs to be fixed on top of the clean-up
> branch where GPIO movemnt to driver/gpio/* happening.
> 
> For this change,
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
This is also available as part of following cleanup series we are doing.
However, since this is a defect we can prioritize it over the cleanup.
--
Tarun
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman June 6, 2011, 10:32 p.m. UTC | #4
Colin Cross <ccross@android.com> writes:

> On Sat, Jun 4, 2011 at 12:03 PM, Colin Cross <ccross@android.com> wrote:
>> Setting the IRQWAKEN bit was overwriting previous IRQWAKEN bits,
>> causing only the last bit set to take effect, resulting in lost
>> wakeups when the GPIO controller is in idle.
>>
>> Replace direct writes to IRQWAKEN with writes to SETWKUENA and
>> CLEARWKUEN.
>>
>> Signed-off-by: Colin Cross <ccross@android.com>

[...]

>
> Todd pointed out that the OMAP4 TRM says not to use SETWKUENA and
> CLEARWKUENA.  

In my GPIO cleanups, I was wondering why the set/clear registers were
not used here. Todd, can you give the TRM version & reference for this?
I didn't find anything after a quick scan/search.

Thanks,

Kevin
Todd Poynor June 6, 2011, 11:05 p.m. UTC | #5
On Mon, Jun 06, 2011 at 03:32:29PM -0700, Kevin Hilman wrote:
> Colin Cross <ccross@android.com> writes:
> 
> > On Sat, Jun 4, 2011 at 12:03 PM, Colin Cross <ccross@android.com> wrote:
> >> Setting the IRQWAKEN bit was overwriting previous IRQWAKEN bits,
> >> causing only the last bit set to take effect, resulting in lost
> >> wakeups when the GPIO controller is in idle.
> >>
> >> Replace direct writes to IRQWAKEN with writes to SETWKUENA and
> >> CLEARWKUEN.
> >>
> >> Signed-off-by: Colin Cross <ccross@android.com>
> 
> [...]
> 
> >
> > Todd pointed out that the OMAP4 TRM says not to use SETWKUENA and
> > CLEARWKUENA.  
> 
> In my GPIO cleanups, I was wondering why the set/clear registers were
> not used here. Todd, can you give the TRM version & reference for this?
> I didn't find anything after a quick scan/search.

http://focus.ti.com/general/docs/wtbu/wtbudocumentcenter.tsp?templateId=6123&navigationId=12037

OMAP4460 Multimedia Device Silicon Revision 1.0 Version E
February 2011–Revised May 2011

p. 5357 Table 25-68. GPIO_CLEARWKUPENA

"Caution: This register is kept for backward software compatibility. Use
GPIO_IRQWAKEN_0 and GPIO_IRQWAKEN_1 instead."

OMAP4430 TRM is similar.


Todd
Kevin Hilman June 6, 2011, 11:25 p.m. UTC | #6
Todd Poynor <toddpoynor@google.com> writes:

> On Mon, Jun 06, 2011 at 03:32:29PM -0700, Kevin Hilman wrote:
>> Colin Cross <ccross@android.com> writes:
>> 
>> > On Sat, Jun 4, 2011 at 12:03 PM, Colin Cross <ccross@android.com> wrote:
>> >> Setting the IRQWAKEN bit was overwriting previous IRQWAKEN bits,
>> >> causing only the last bit set to take effect, resulting in lost
>> >> wakeups when the GPIO controller is in idle.
>> >>
>> >> Replace direct writes to IRQWAKEN with writes to SETWKUENA and
>> >> CLEARWKUEN.
>> >>
>> >> Signed-off-by: Colin Cross <ccross@android.com>
>> 
>> [...]
>> 
>> >
>> > Todd pointed out that the OMAP4 TRM says not to use SETWKUENA and
>> > CLEARWKUENA.  
>> 
>> In my GPIO cleanups, I was wondering why the set/clear registers were
>> not used here. Todd, can you give the TRM version & reference for this?
>> I didn't find anything after a quick scan/search.
>
> http://focus.ti.com/general/docs/wtbu/wtbudocumentcenter.tsp?templateId=6123&navigationId=12037
>
> OMAP4460 Multimedia Device Silicon Revision 1.0 Version E
> February 2011–Revised May 2011
>
> p. 5357 Table 25-68. GPIO_CLEARWKUPENA
>
> "Caution: This register is kept for backward software compatibility. Use
> GPIO_IRQWAKEN_0 and GPIO_IRQWAKEN_1 instead."

OK, I see it now.

Thanks,

Kevin

Patch
diff mbox

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index c985652..23ac7b6 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -539,7 +539,6 @@  static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
 {
 	void __iomem *base = bank->base;
 	u32 gpio_bit = 1 << gpio;
-	u32 val;
 
 	if (cpu_is_omap44xx()) {
 		MOD_REG_BIT(OMAP4_GPIO_LEVELDETECT0, gpio_bit,
@@ -563,14 +562,11 @@  static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
 	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
 		if (cpu_is_omap44xx()) {
 			if (trigger != 0)
-				__raw_writel(1 << gpio, bank->base+
-						OMAP4_GPIO_IRQWAKEN0);
-			else {
-				val = __raw_readl(bank->base +
-							OMAP4_GPIO_IRQWAKEN0);
-				__raw_writel(val & (~(1 << gpio)), bank->base +
-							 OMAP4_GPIO_IRQWAKEN0);
-			}
+				__raw_writel(gpio_bit,
+					bank->base + OMAP4_GPIO_SETWKUENA);
+			else
+				__raw_writel(gpio_bit,
+					bank->base + OMAP4_GPIO_CLEARWKUENA);
 		} else {
 			/*
 			 * GPIO wakeup request can only be generated on edge