[PATCH] gpio/omap: fix incorrect initialization of omap_gpio_mod_init

[PATCH] gpio/omap: fix incorrect initialization of omap_gpio_mod_init

[Tarun Kanti DebBarma]

Breaking commit: ab985f0f7c2c0ef90b7c832f0c04f470dda0593d

Initialization of irqenable, irqstatus registers is the common
operation done in this function for all OMAP platforms, viz.
OMAP1, OMAP2+. The latter _gpio_rmw()'s to irqenable register
was overwriting the correctly programmed OMAP1 value at the
beginning. As a result, even though it worked on OMAP2+
platforms it was breaking OMAP1 functionality. On close
observation it is found that the first _gpio_rmw() which is
supposedly done to take care of OMAP1 platform is generic enough
and takes care of OMAP2+ platform as well. Therefore remove the
latter _gpio_rmw() to irqenable as they are redundant now.

Writing to ctrl and debounce_en registers for OMAP2+ platforms
are modified to match the original(pre-cleanup) code where the
registers are initialized with 0. In the cleanup series since
we are using _gpio_rmw(reg, 0, 1), instead of __raw_writel(),
we are just reading and writing the same values to ctrl and
debounce_en. This is not an issue for debounce_en register
because it has 0x0 as the default value. But in the case of
ctrl register the default value is 0x2 (GATINGRATIO = 0x1)
so that we end up writing 0x2 instead of intended 0 value.
Therefore correcting it to _gpio_rmw(reg, l, 0), where
l = 0xffffffff so that registers are initialized to 0.

Also, changing the sequence and logic of initializing the irqstatus.

Cc: stable at vger.kernel.org
Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Reported-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
---
Tested on OMAP2+ platforms and bootup test on OMAP1710.
Janusz,
Please help me in validating the patch on OMAP1 platform.

 drivers/gpio/gpio-omap.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 1adc2ec..910fd14 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -964,19 +964,16 @@ static void omap_gpio_mod_init(struct gpio_bank *bank)
 		return;
 	}
 
+	_gpio_rmw(base, bank->regs->irqstatus, l, !bank->regs->irqenable_inv);
 	_gpio_rmw(base, bank->regs->irqenable, l, bank->regs->irqenable_inv);
-	_gpio_rmw(base, bank->regs->irqstatus, l,
-					bank->regs->irqenable_inv == false);
-	_gpio_rmw(base, bank->regs->irqenable, l, bank->regs->debounce_en != 0);
-	_gpio_rmw(base, bank->regs->irqenable, l, bank->regs->ctrl != 0);
 	if (bank->regs->debounce_en)
-		_gpio_rmw(base, bank->regs->debounce_en, 0, 1);
+		_gpio_rmw(base, bank->regs->debounce_en, l, 0);
 
 	/* Save OE default value (0xffffffff) in the context */
 	bank->context.oe = __raw_readl(bank->base + bank->regs->direction);
 	 /* Initialize interface clk ungated, module enabled */
 	if (bank->regs->ctrl)
-		_gpio_rmw(base, bank->regs->ctrl, 0, 1);
+		_gpio_rmw(base, bank->regs->ctrl, l, 0);
 }
 
 static __devinit void
-- 
1.7.0.4

[PATCH] gpio/omap: fix incorrect initialization of omap_gpio_mod_init

[Kevin Hilman]

Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:

> Breaking commit: ab985f0f7c2c0ef90b7c832f0c04f470dda0593d
>
> Initialization of irqenable, irqstatus registers is the common
> operation done in this function for all OMAP platforms, viz.
> OMAP1, OMAP2+. The latter _gpio_rmw()'s to irqenable register
> was overwriting the correctly programmed OMAP1 value at the
> beginning. As a result, even though it worked on OMAP2+
> platforms it was breaking OMAP1 functionality. On close
> observation it is found that the first _gpio_rmw() which is
> supposedly done to take care of OMAP1 platform is generic enough
> and takes care of OMAP2+ platform as well. Therefore remove the
> latter _gpio_rmw() to irqenable as they are redundant now.
>
> Writing to ctrl and debounce_en registers for OMAP2+ platforms
> are modified to match the original(pre-cleanup) code where the
> registers are initialized with 0. In the cleanup series since
> we are using _gpio_rmw(reg, 0, 1), instead of __raw_writel(),
> we are just reading and writing the same values to ctrl and
> debounce_en. This is not an issue for debounce_en register
> because it has 0x0 as the default value. But in the case of
> ctrl register the default value is 0x2 (GATINGRATIO = 0x1)
> so that we end up writing 0x2 instead of intended 0 value.
> Therefore correcting it to _gpio_rmw(reg, l, 0), where
> l = 0xffffffff so that registers are initialized to 0.
>
> Also, changing the sequence and logic of initializing the irqstatus.
>
> Cc: stable at vger.kernel.org
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Kevin Hilman <khilman@ti.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Reported-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> ---
> Tested on OMAP2+ platforms and bootup test on OMAP1710.
> Janusz,
> Please help me in validating the patch on OMAP1 platform.
>
>  drivers/gpio/gpio-omap.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 1adc2ec..910fd14 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -964,19 +964,16 @@ static void omap_gpio_mod_init(struct gpio_bank *bank)
>  		return;
>  	}
>  
> +	_gpio_rmw(base, bank->regs->irqstatus, l, !bank->regs->irqenable_inv);
>  	_gpio_rmw(base, bank->regs->irqenable, l, bank->regs->irqenable_inv);
> -	_gpio_rmw(base, bank->regs->irqstatus, l,
> -					bank->regs->irqenable_inv == false);

Not sure I follow this change.  It seems the ending value in irqstatus
will be the same, but it was just moved before irqenable.

You mention changing this order in the changelog, but do not mention why.

> -	_gpio_rmw(base, bank->regs->irqenable, l, bank->regs->debounce_en != 0);
> -	_gpio_rmw(base, bank->regs->irqenable, l, bank->regs->ctrl != 0);

Why were these here in the first place?

The changelog is not very clear about *why* these can be removed.

>  	if (bank->regs->debounce_en)
> -		_gpio_rmw(base, bank->regs->debounce_en, 0, 1);
> +		_gpio_rmw(base, bank->regs->debounce_en, l, 0);

Why the read/modify/write?  Why not just go back to the __raw_write?

>  	/* Save OE default value (0xffffffff) in the context */
>  	bank->context.oe = __raw_readl(bank->base + bank->regs->direction);
>  	 /* Initialize interface clk ungated, module enabled */
>  	if (bank->regs->ctrl)
> -		_gpio_rmw(base, bank->regs->ctrl, 0, 1);
> +		_gpio_rmw(base, bank->regs->ctrl, l, 0);

same question.

>  }
>  
>  static __devinit void

Kevin

[PATCH] gpio/omap: fix incorrect initialization of omap_gpio_mod_init

[Janusz Krzysztofik]

On Friday 27 of April 2012 14:09:11 Tarun Kanti DebBarma wrote:
> Breaking commit: ab985f0f7c2c0ef90b7c832f0c04f470dda0593d
> 
> Initialization of irqenable, irqstatus registers is the common
> operation done in this function for all OMAP platforms, viz.
> OMAP1, OMAP2+. The latter _gpio_rmw()'s to irqenable register
> was overwriting the correctly programmed OMAP1 value at the
> beginning. As a result, even though it worked on OMAP2+
> platforms it was breaking OMAP1 functionality. On close
> observation it is found that the first _gpio_rmw() which is
> supposedly done to take care of OMAP1 platform is generic enough
> and takes care of OMAP2+ platform as well. Therefore remove the
> latter _gpio_rmw() to irqenable as they are redundant now.
> 
> Writing to ctrl and debounce_en registers for OMAP2+ platforms
> are modified to match the original(pre-cleanup) code where the
> registers are initialized with 0. In the cleanup series since
> we are using _gpio_rmw(reg, 0, 1), instead of __raw_writel(),
> we are just reading and writing the same values to ctrl and
> debounce_en. This is not an issue for debounce_en register
> because it has 0x0 as the default value. But in the case of
> ctrl register the default value is 0x2 (GATINGRATIO = 0x1)
> so that we end up writing 0x2 instead of intended 0 value.
> Therefore correcting it to _gpio_rmw(reg, l, 0), where
> l = 0xffffffff so that registers are initialized to 0.
> 
> Also, changing the sequence and logic of initializing the irqstatus.

Hi,
Sorry for not responding in 2 days, as I promissed. The reason is that 
there are still more issues with GPIO on my Amstrad Delta, for example 
those introduced with commit 384ebe1c2849160d040df3e68634ec506f13d9ff, 
and I'm still trying to understand and fix them. Than, it's hard for me 
to provide you with my Tested-by: before I'm confident those other issues 
are not related.

Anyway, and with respect to Kevin's comments, reverting the irqstatus 
vs. irqenable initialization order doesn't seem to break anything for 
me, and changes to debounce_en and ctrl register handling seem not 
relevant to my test machine.

Thanks,
Janusz

[PATCH] gpio/omap: fix incorrect initialization of omap_gpio_mod_init

[DebBarma, Tarun Kanti]

On Sat, Apr 28, 2012 at 3:48 AM, Kevin Hilman <khilman@ti.com> wrote:
> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>
>> Breaking commit: ab985f0f7c2c0ef90b7c832f0c04f470dda0593d
>>
>> Initialization of irqenable, irqstatus registers is the common
>> operation done in this function for all OMAP platforms, viz.
>> OMAP1, OMAP2+. The latter _gpio_rmw()'s to irqenable register
>> was overwriting the correctly programmed OMAP1 value at the
>> beginning. As a result, even though it worked on OMAP2+
>> platforms it was breaking OMAP1 functionality. On close
>> observation it is found that the first _gpio_rmw() which is
>> supposedly done to take care of OMAP1 platform is generic enough
>> and takes care of OMAP2+ platform as well. Therefore remove the
>> latter _gpio_rmw() to irqenable as they are redundant now.
>>
>> Writing to ctrl and debounce_en registers for OMAP2+ platforms
>> are modified to match the original(pre-cleanup) code where the
>> registers are initialized with 0. In the cleanup series since
>> we are using _gpio_rmw(reg, 0, 1), instead of __raw_writel(),
>> we are just reading and writing the same values to ctrl and
>> debounce_en. This is not an issue for debounce_en register
>> because it has 0x0 as the default value. But in the case of
>> ctrl register the default value is 0x2 (GATINGRATIO = 0x1)
>> so that we end up writing 0x2 instead of intended 0 value.
>> Therefore correcting it to _gpio_rmw(reg, l, 0), where
>> l = 0xffffffff so that registers are initialized to 0.
>>
>> Also, changing the sequence and logic of initializing the irqstatus.
>>
>> Cc: stable at vger.kernel.org
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Kevin Hilman <khilman@ti.com>
>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> Reported-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>> ---
>> Tested on OMAP2+ platforms and bootup test on OMAP1710.
>> Janusz,
>> Please help me in validating the patch on OMAP1 platform.
>>
>> ?drivers/gpio/gpio-omap.c | ? ?9 +++------
>> ?1 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 1adc2ec..910fd14 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -964,19 +964,16 @@ static void omap_gpio_mod_init(struct gpio_bank *bank)
>> ? ? ? ? ? ? ? return;
>> ? ? ? }
>>
>> + ? ? _gpio_rmw(base, bank->regs->irqstatus, l, !bank->regs->irqenable_inv);
>> ? ? ? _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->irqenable_inv);
>> - ? ? _gpio_rmw(base, bank->regs->irqstatus, l,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->regs->irqenable_inv == false);
>
> Not sure I follow this change. ?It seems the ending value in irqstatus
> will be the same, but it was just moved before irqenable.
Yes, it is just moved before irqenable and instead of:
bank->regs->irqenable_inv == false, I made it !bank->regs->irqenable_inv
so that it fits in single line and avoid comparison with bool.

>
> You mention changing this order in the changelog, but do not mention why.
If we consider the overall code sequence it was something like:

_gpio_rmw(base, bank->regs->irqenable, l, <condition1>);
_gpio_rmw(base, bank->regs->irqstatus, l, <condition2>);
_gpio_rmw(base, bank->regs->irqenable, l, <condition3>);
_gpio_rmw(base, bank->regs->irqenable, l, <condition4>);

So I decided to move irqstatus first (beside the changing the logic
mentioned above)
and keep all irqenable settings together. However, with just single
irqenable setting
now this does not make any difference. In short, I can just change the
logic while
keeping it in place.

>
>> - ? ? _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->debounce_en != 0);
>> - ? ? _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->ctrl != 0);
>
> Why were these here in the first place?
These we supposedly introduced wrongly to take care of OMAP2+ platforms.
Also, I somehow made incorrect assumption that each _gpio_rmw()'s were
mutually exclusive. In fact this is what led to overwrite of initial
OMAP1 setting.

>
> The changelog is not very clear about *why* these can be removed.
I can update changelog with what I have just said above.

>
>> ? ? ? if (bank->regs->debounce_en)
>> - ? ? ? ? ? ? _gpio_rmw(base, bank->regs->debounce_en, 0, 1);
>> + ? ? ? ? ? ? _gpio_rmw(base, bank->regs->debounce_en, l, 0);
>
> Why the read/modify/write? ?Why not just go back to the __raw_write?
We can use __raw_writel() as before.

>
>> ? ? ? /* Save OE default value (0xffffffff) in the context */
>> ? ? ? bank->context.oe = __raw_readl(bank->base + bank->regs->direction);
>> ? ? ? ?/* Initialize interface clk ungated, module enabled */
>> ? ? ? if (bank->regs->ctrl)
>> - ? ? ? ? ? ? _gpio_rmw(base, bank->regs->ctrl, 0, 1);
>> + ? ? ? ? ? ? _gpio_rmw(base, bank->regs->ctrl, l, 0);
>
> same question.
Yes, here also we can use the __raw_writel().
--
Tarun
>
>> ?}
>>
>> ?static __devinit void
>
> Kevin