Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-27 Thread DebBarma, Tarun Kanti 
On Tue, Sep 27, 2011 at 9:52 PM, Kevin Hilman  wrote:
> "DebBarma, Tarun Kanti"  writes:
>
>> On Tue, Sep 27, 2011 at 4:40 AM, Kevin Hilman  wrote:
>>> "DebBarma, Tarun Kanti"  writes:
>>>
>>> [...]
>>>
 As pointed out by Kevin, debounce clock was not getting disabled.
 In my testing I was somehow grepping CORE power domain instead
 of PER power domain and hence missed it. The fix for the debounce
 clock issue is at the end of the email.

 - Have re-based the for_3.2/gpio-cleanup branch against 3.1-rc6.
 - Dropped [PATCH 26/26] gpio/omap: add dbclk aliases for all gpio modules
 as suggested by Kevin since it's already taken care by hwmod.
 - Added the debounce clock fix in the end.
>>>
>>> That debounce fix definitely makes things look better, but it's not
>>> solving the problem...
>>>
 With above, PER is hitting low power state in Suspend and Idle path.

 Have pushed a branch at below URL with mentioned changes.
 git://gitorious.org/omap-sw-develoment/linux-omap-dev.git
 for_3.2/kevin/gpio-cleanup
>>>
>>> I tested your branch on my 3430/n900 and PER is still not hitting
>>> retention.  Setting all debounce values in the board file to zero using
>>> the patch below[1] makes PER hit retention again.
>>>
>>> Assuming you don't have an n900 to test with, I suggest you just copy
>>> the GPIO keys init from board-rx51-peripherals.c (or some of it) into
>>> the board file you are testing with.
>>>
>>> The problem is most likely be related to having more than one GPIO in a
>>> bank with debounce enabled, or more than one bank with GPIOs enabled and
>>> your current test is not be catching it.
>>>
>> As per commit c8c9fda506945 {OMAP: PM: disable idle on suspend for
>> GPIO and UART}, the gpio code needs to be fixed once GPIO driver is
>> run-time adapted.
>
> Great, good catch.
>
>> So I did below change as per the commit and now suspend is working
>> fine even with board files change for debounce functionality. So the
>> last series + below one line change is whats needed for suspend to
>> work. Can you please see if this does help on your board ?
>
> Yeah, with your patch, PER is hitting retention in suspend on my
> 3430/n900.
>
>> I am not finished my idle testing yet but just reporting the suspend
>> results.
>
> For me, PER is not hitting retention on idle.
I am working on this right now.

>
> Also, your repost of v7 doesn't included any of the comments I made on
> it yesterday.
That's right. By the time I received the comments the series was already posted.
Anyways, I will include them along with the PER retention issue in the
Idle path.
--
Tarun
>
> Kevin
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-27 Thread Kevin Hilman 
"DebBarma, Tarun Kanti"  writes:

> On Tue, Sep 27, 2011 at 4:40 AM, Kevin Hilman  wrote:
>> "DebBarma, Tarun Kanti"  writes:
>>
>> [...]
>>
>>> As pointed out by Kevin, debounce clock was not getting disabled.
>>> In my testing I was somehow grepping CORE power domain instead
>>> of PER power domain and hence missed it. The fix for the debounce
>>> clock issue is at the end of the email.
>>>
>>> - Have re-based the for_3.2/gpio-cleanup branch against 3.1-rc6.
>>> - Dropped [PATCH 26/26] gpio/omap: add dbclk aliases for all gpio modules
>>> as suggested by Kevin since it's already taken care by hwmod.
>>> - Added the debounce clock fix in the end.
>>
>> That debounce fix definitely makes things look better, but it's not
>> solving the problem...
>>
>>> With above, PER is hitting low power state in Suspend and Idle path.
>>>
>>> Have pushed a branch at below URL with mentioned changes.
>>> git://gitorious.org/omap-sw-develoment/linux-omap-dev.git
>>> for_3.2/kevin/gpio-cleanup
>>
>> I tested your branch on my 3430/n900 and PER is still not hitting
>> retention.  Setting all debounce values in the board file to zero using
>> the patch below[1] makes PER hit retention again.
>>
>> Assuming you don't have an n900 to test with, I suggest you just copy
>> the GPIO keys init from board-rx51-peripherals.c (or some of it) into
>> the board file you are testing with.
>>
>> The problem is most likely be related to having more than one GPIO in a
>> bank with debounce enabled, or more than one bank with GPIOs enabled and
>> your current test is not be catching it.
>>
> As per commit c8c9fda506945 {OMAP: PM: disable idle on suspend for
> GPIO and UART}, the gpio code needs to be fixed once GPIO driver is
> run-time adapted.  

Great, good catch.

> So I did below change as per the commit and now suspend is working
> fine even with board files change for debounce functionality. So the
> last series + below one line change is whats needed for suspend to
> work. Can you please see if this does help on your board ?

Yeah, with your patch, PER is hitting retention in suspend on my
3430/n900.

> I am not finished my idle testing yet but just reporting the suspend
> results.

For me, PER is not hitting retention on idle.

Also, your repost of v7 doesn't included any of the comments I made on
it yesterday.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-27 Thread DebBarma, Tarun Kanti 
On Tue, Sep 27, 2011 at 4:40 AM, Kevin Hilman  wrote:
> "DebBarma, Tarun Kanti"  writes:
>
> [...]
>
>> As pointed out by Kevin, debounce clock was not getting disabled.
>> In my testing I was somehow grepping CORE power domain instead
>> of PER power domain and hence missed it. The fix for the debounce
>> clock issue is at the end of the email.
>>
>> - Have re-based the for_3.2/gpio-cleanup branch against 3.1-rc6.
>> - Dropped [PATCH 26/26] gpio/omap: add dbclk aliases for all gpio modules
>> as suggested by Kevin since it's already taken care by hwmod.
>> - Added the debounce clock fix in the end.
>
> That debounce fix definitely makes things look better, but it's not
> solving the problem...
>
>> With above, PER is hitting low power state in Suspend and Idle path.
>>
>> Have pushed a branch at below URL with mentioned changes.
>> git://gitorious.org/omap-sw-develoment/linux-omap-dev.git
>> for_3.2/kevin/gpio-cleanup
>
> I tested your branch on my 3430/n900 and PER is still not hitting
> retention.  Setting all debounce values in the board file to zero using
> the patch below[1] makes PER hit retention again.
>
> Assuming you don't have an n900 to test with, I suggest you just copy
> the GPIO keys init from board-rx51-peripherals.c (or some of it) into
> the board file you are testing with.
>
> The problem is most likely be related to having more than one GPIO in a
> bank with debounce enabled, or more than one bank with GPIOs enabled and
> your current test is not be catching it.
>
As per commit c8c9fda506945 {OMAP: PM: disable idle on suspend for GPIO
and UART}, the gpio code needs to be fixed once GPIO driver is run-time adapted.
So I did below change as per the commit and now suspend is working fine even
with board files change for debounce functionality. So the last series
+ below one
line change is whats needed for suspend to work. Can you please see if this does
help on your board ?

I am not finished my idle testing yet but just reporting the suspend results.

diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c
index d865033..1957663 100644
--- a/arch/arm/mach-omap2/gpio.c
+++ b/arch/arm/mach-omap2/gpio.c
@@ -148,8 +148,6 @@ static int omap2_gpio_dev_init(struct omap_hwmod
*oh, void *unused)
return PTR_ERR(od);
}

-   omap_device_disable_idle_on_suspend(od);
-
return 0;
 }

--

Regards
Tarun
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-26 Thread Kevin Hilman 
"DebBarma, Tarun Kanti"  writes:

> [...]
>>> - Added the debounce clock fix in the end.
>>
>> Thanks.  Glad you found and fixed it.
>>
>> Rather than add this patch as a fix at the end, I prefer if the problem
>> is fixed in the original patches that added/created the problem.
> Sure. I will put the changes in respective patches.

Also, when you rebase, please be sure to add the Acked-by from Tony
to the various patches he ack'd.

Thanks,

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-26 Thread Kevin Hilman 
"DebBarma, Tarun Kanti"  writes:

[...]

> As pointed out by Kevin, debounce clock was not getting disabled.
> In my testing I was somehow grepping CORE power domain instead
> of PER power domain and hence missed it. The fix for the debounce
> clock issue is at the end of the email.
>
> - Have re-based the for_3.2/gpio-cleanup branch against 3.1-rc6.
> - Dropped [PATCH 26/26] gpio/omap: add dbclk aliases for all gpio modules
> as suggested by Kevin since it's already taken care by hwmod.
> - Added the debounce clock fix in the end.

That debounce fix definitely makes things look better, but it's not
solving the problem...

> With above, PER is hitting low power state in Suspend and Idle path.
>
> Have pushed a branch at below URL with mentioned changes.
> git://gitorious.org/omap-sw-develoment/linux-omap-dev.git
> for_3.2/kevin/gpio-cleanup

I tested your branch on my 3430/n900 and PER is still not hitting
retention.  Setting all debounce values in the board file to zero using
the patch below[1] makes PER hit retention again.

Assuming you don't have an n900 to test with, I suggest you just copy
the GPIO keys init from board-rx51-peripherals.c (or some of it) into
the board file you are testing with.

The problem is most likely be related to having more than one GPIO in a
bank with debounce enabled, or more than one bank with GPIOs enabled and
your current test is not be catching it.

Kevin

[1]
diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c 
b/arch/arm/mach-omap2/board-rx51-peripherals.c
index 5a886cd..1853194 100644
--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
@@ -207,7 +207,7 @@ static void __init rx51_charger_init(void)
 #define RX51_GPIO_LOCK_BUTTON  113
 #define RX51_GPIO_PROXIMITY89
 
-#define RX51_GPIO_DEBOUNCE_TIMEOUT 10
+#define RX51_GPIO_DEBOUNCE_TIMEOUT 0
 
 static struct gpio_keys_button rx51_gpio_keys[] = {
{
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-26 Thread DebBarma, Tarun Kanti 
[...]
>> - Added the debounce clock fix in the end.
>
> Thanks.  Glad you found and fixed it.
>
> Rather than add this patch as a fix at the end, I prefer if the problem
> is fixed in the original patches that added/created the problem.
Sure. I will put the changes in respective patches.
--
Tarun
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-26 Thread Kevin Hilman 
"DebBarma, Tarun Kanti"  writes:

> Santosh, Kevin,
>
> [...]
 After that, pm_runtime_put_sync() is called, which will trigger the
 driver's ->runtime_suspend callback.  The ->runtime_suspend() callback
 checks bank->mod_usage as well, and if zero, doesn't do anything
 (notably, it doesn't disable debounce clocks.)
>>> I need some clarification in reproducing/testing the fix on OMAP3430SDP.
>>> The first thing I am trying to verify is the code flow of suspend.
>>>
>>> 1) With no debounce clock enabled, when I enable UART timeouts, I
>>> automatically see
>>> system going to retention. That is I don't have to type echo mem >
>>> /sys/power/state
>>> echo 5 > /sys/devices/platform/omap/omap_uart.0/sleep_timeout
>>> echo 5 > /sys/devices/platform/omap/omap_uart.1/sleep_timeout
>>> echo 5 > /sys/devices/platform/omap/omap_uart.2/sleep_timeout
>>>
>>> 2) I am do not see the print in omap_gpio_suspend/resume(), but I see
>>> the print in
>>> *_prepare_for_idle()/*_resume_after_idle().
>>>
>> Hmmm,
>>
>> This is mostly happening because you are missing a below
>> fix from Kevin in the branch you are testing with.
>>
>> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg54927.html
>> {OMAP: omap_device: fix !CONFIG_SUSPEND case in _noirq handlers}
>>
>> If you rebase, your branch against 3.1-rc6, you should already
>> have this fix. Commit {126caf1376e7}
> Yes, this patch was missing in Kevin's branch and was
> causing the suspend issue.
>
> As pointed out by Kevin, debounce clock was not getting disabled.
> In my testing I was somehow grepping CORE power domain instead
> of PER power domain and hence missed it. The fix for the debounce
> clock issue is at the end of the email.
>
> - Have re-based the for_3.2/gpio-cleanup branch against 3.1-rc6.

Not needed.   Just merge with v3.1-rc6 when testing.

> - Dropped [PATCH 26/26] gpio/omap: add dbclk aliases for all gpio modules
> as suggested by Kevin since it's already taken care by hwmod.

good

> - Added the debounce clock fix in the end.

Thanks.  Glad you found and fixed it.

Rather than add this patch as a fix at the end, I prefer if the problem
is fixed in the original patches that added/created the problem.

Kevin


> With above, PER is hitting low power state in Suspend and Idle path.
>
> Have pushed a branch at below URL with mentioned changes.
> git://gitorious.org/omap-sw-develoment/linux-omap-dev.git
> for_3.2/kevin/gpio-cleanup
>
> Regards,
> Tarun
>
> From 5d9a97197ea5426fc79b7a47dd0fd9c6b6ea Mon Sep 17 00:00:00 2001
> From: Tarun Kanti DebBarma 
> Date: Sat, 24 Sep 2011 13:32:32 +0530
> Subject: [PATCH] gpio/omap: fix debounce clock handling
>
> GPIO debounce clock can gate the PER power domain transition
> and needs to be disabled in GPIO driver suspend.
>
> The debounce clock is not getting disabled in runtime_suspend
> callback because of an un-necessary bank->mod_usage check.
> In omap_gpio_suspend/resume too, there is no need to do
> any operation if the gpio bank is not used.
>
> Remove the un-necessary bank->mod_usage check from
> suspend callbacks.
>
> Thanks to Kevin Hilman for pointing out this issue.
>
> Signed-off-by: Tarun Kanti DebBarma 
> Cc: Kevin Hilman 
> Cc: Santosh Shilimkar 
> ---
>  drivers/gpio/gpio-omap.c |   12 ++--
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index c597303..349e774 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1107,6 +1107,9 @@ static int omap_gpio_suspend(struct device *dev)
>   void __iomem *wake_status;
>   unsigned long flags;
>
> + if (!bank->mod_usage || !bank->loses_context)
> + return 0;
> +
>   if (!bank->regs->wkup_en || !bank->suspend_wakeup)
>   return 0;
>
> @@ -1128,6 +1131,9 @@ static int omap_gpio_resume(struct device *dev)
>   void __iomem *base = bank->base;
>   unsigned long flags;
>
> + if (!bank->mod_usage || !bank->loses_context)
> + return 0;
> +
>   if (!bank->regs->wkup_en || !bank->saved_wakeup)
>   return 0;
>
> @@ -1151,9 +1157,6 @@ static int omap_gpio_runtime_suspend(struct device *dev)
>   int j;
>   unsigned long flags;
>
> - if (!bank->mod_usage)
> - return 0;
> -
>   spin_lock_irqsave(&bank->lock, flags);
>   /*
>* If going to OFF, remove triggering for all
> @@ -1199,9 +1202,6 @@ static int omap_gpio_runtime_resume(struct device *dev)
>   int j;
>   unsigned long flags;
>
> - if (!bank->mod_usage)
> - return 0;
> -
>   spin_lock_irqsave(&bank->lock, flags);
>   for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
>   clk_enable(bank->dbck);
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-24 Thread DebBarma, Tarun Kanti 
Santosh, Kevin,

[...]
>>> After that, pm_runtime_put_sync() is called, which will trigger the
>>> driver's ->runtime_suspend callback.  The ->runtime_suspend() callback
>>> checks bank->mod_usage as well, and if zero, doesn't do anything
>>> (notably, it doesn't disable debounce clocks.)
>> I need some clarification in reproducing/testing the fix on OMAP3430SDP.
>> The first thing I am trying to verify is the code flow of suspend.
>>
>> 1) With no debounce clock enabled, when I enable UART timeouts, I
>> automatically see
>> system going to retention. That is I don't have to type echo mem >
>> /sys/power/state
>> echo 5 > /sys/devices/platform/omap/omap_uart.0/sleep_timeout
>> echo 5 > /sys/devices/platform/omap/omap_uart.1/sleep_timeout
>> echo 5 > /sys/devices/platform/omap/omap_uart.2/sleep_timeout
>>
>> 2) I am do not see the print in omap_gpio_suspend/resume(), but I see
>> the print in
>> *_prepare_for_idle()/*_resume_after_idle().
>>
> Hmmm,
>
> This is mostly happening because you are missing a below
> fix from Kevin in the branch you are testing with.
>
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg54927.html
> {OMAP: omap_device: fix !CONFIG_SUSPEND case in _noirq handlers}
>
> If you rebase, your branch against 3.1-rc6, you should already
> have this fix. Commit {126caf1376e7}
Yes, this patch was missing in Kevin's branch and was
causing the suspend issue.

As pointed out by Kevin, debounce clock was not getting disabled.
In my testing I was somehow grepping CORE power domain instead
of PER power domain and hence missed it. The fix for the debounce
clock issue is at the end of the email.

- Have re-based the for_3.2/gpio-cleanup branch against 3.1-rc6.
- Dropped [PATCH 26/26] gpio/omap: add dbclk aliases for all gpio modules
as suggested by Kevin since it's already taken care by hwmod.
- Added the debounce clock fix in the end.

With above, PER is hitting low power state in Suspend and Idle path.

Have pushed a branch at below URL with mentioned changes.
git://gitorious.org/omap-sw-develoment/linux-omap-dev.git
for_3.2/kevin/gpio-cleanup

Regards,
Tarun

>From 5d9a97197ea5426fc79b7a47dd0fd9c6b6ea Mon Sep 17 00:00:00 2001
From: Tarun Kanti DebBarma 
Date: Sat, 24 Sep 2011 13:32:32 +0530
Subject: [PATCH] gpio/omap: fix debounce clock handling

GPIO debounce clock can gate the PER power domain transition
and needs to be disabled in GPIO driver suspend.

The debounce clock is not getting disabled in runtime_suspend
callback because of an un-necessary bank->mod_usage check.
In omap_gpio_suspend/resume too, there is no need to do
any operation if the gpio bank is not used.

Remove the un-necessary bank->mod_usage check from
suspend callbacks.

Thanks to Kevin Hilman for pointing out this issue.

Signed-off-by: Tarun Kanti DebBarma 
Cc: Kevin Hilman 
Cc: Santosh Shilimkar 
---
 drivers/gpio/gpio-omap.c |   12 ++--
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index c597303..349e774 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1107,6 +1107,9 @@ static int omap_gpio_suspend(struct device *dev)
void __iomem *wake_status;
unsigned long flags;

+   if (!bank->mod_usage || !bank->loses_context)
+   return 0;
+
if (!bank->regs->wkup_en || !bank->suspend_wakeup)
return 0;

@@ -1128,6 +1131,9 @@ static int omap_gpio_resume(struct device *dev)
void __iomem *base = bank->base;
unsigned long flags;

+   if (!bank->mod_usage || !bank->loses_context)
+   return 0;
+
if (!bank->regs->wkup_en || !bank->saved_wakeup)
return 0;

@@ -1151,9 +1157,6 @@ static int omap_gpio_runtime_suspend(struct device *dev)
int j;
unsigned long flags;

-   if (!bank->mod_usage)
-   return 0;
-
spin_lock_irqsave(&bank->lock, flags);
/*
 * If going to OFF, remove triggering for all
@@ -1199,9 +1202,6 @@ static int omap_gpio_runtime_resume(struct device *dev)
int j;
unsigned long flags;

-   if (!bank->mod_usage)
-   return 0;
-
spin_lock_irqsave(&bank->lock, flags);
for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
clk_enable(bank->dbck);
-- 
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-24 Thread Santosh Shilimkar 
On Saturday 24 September 2011 09:26 AM, DebBarma, Tarun Kanti wrote:
> [...]
>> After debugging this myself a bit, here's what I think may be going on.
>> This may not be the only problem but here's at least one of them.
>>
>> First, debounce clocks are disabled in the runtime_suspend callback.
>>
>> When a GPIO is freed and it's the last one in the bank, bank->mod_usage
>> goes to zero.
>>
>> After that, pm_runtime_put_sync() is called, which will trigger the
>> driver's ->runtime_suspend callback.  The ->runtime_suspend() callback
>> checks bank->mod_usage as well, and if zero, doesn't do anything
>> (notably, it doesn't disable debounce clocks.)
> I need some clarification in reproducing/testing the fix on OMAP3430SDP.
> The first thing I am trying to verify is the code flow of suspend.
> 
> 1) With no debounce clock enabled, when I enable UART timeouts, I
> automatically see
> system going to retention. That is I don't have to type echo mem >
> /sys/power/state
> echo 5 > /sys/devices/platform/omap/omap_uart.0/sleep_timeout
> echo 5 > /sys/devices/platform/omap/omap_uart.1/sleep_timeout
> echo 5 > /sys/devices/platform/omap/omap_uart.2/sleep_timeout
> 
> 2) I am do not see the print in omap_gpio_suspend/resume(), but I see
> the print in
> *_prepare_for_idle()/*_resume_after_idle().
> 
Hmmm,

This is mostly happening because you are missing a below
fix from Kevin in the branch you are testing with.

http://www.mail-archive.com/linux-omap@vger.kernel.org/msg54927.html
{OMAP: omap_device: fix !CONFIG_SUSPEND case in _noirq handlers}

If you rebase, your branch against 3.1-rc6, you should already
have this fix. Commit {126caf1376e7}

Regards
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-23 Thread DebBarma, Tarun Kanti 
[...]
> After debugging this myself a bit, here's what I think may be going on.
> This may not be the only problem but here's at least one of them.
>
> First, debounce clocks are disabled in the runtime_suspend callback.
>
> When a GPIO is freed and it's the last one in the bank, bank->mod_usage
> goes to zero.
>
> After that, pm_runtime_put_sync() is called, which will trigger the
> driver's ->runtime_suspend callback.  The ->runtime_suspend() callback
> checks bank->mod_usage as well, and if zero, doesn't do anything
> (notably, it doesn't disable debounce clocks.)
I need some clarification in reproducing/testing the fix on OMAP3430SDP.
The first thing I am trying to verify is the code flow of suspend.

1) With no debounce clock enabled, when I enable UART timeouts, I
automatically see
system going to retention. That is I don't have to type echo mem >
/sys/power/state
echo 5 > /sys/devices/platform/omap/omap_uart.0/sleep_timeout
echo 5 > /sys/devices/platform/omap/omap_uart.1/sleep_timeout
echo 5 > /sys/devices/platform/omap/omap_uart.2/sleep_timeout

2) I am do not see the print in omap_gpio_suspend/resume(), but I see
the print in
*_prepare_for_idle()/*_resume_after_idle().

Folks testing on Tablet2 platform said there is dedicated button to
suspend/resume.
Is there something equivalent?
--
Tarun
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-22 Thread DebBarma, Tarun Kanti 
[...]
>>> This series is continuation of cleanup of OMAP GPIO driver and fixes.
>>> The cleanup include getting rid of cpu_is_* checks wherever possible,
>>> use of gpio_bank list instead of static array, use of unique platform
>>> specific value associated data member to OMAP platforms to avoid
>>> cpu_is_* checks. The series also include PM runtime support.*
>>>
>>> Baseline: git://gitorious.org/khilman/linux-omap-pm.git
>>> Branch: for_3.2/gpio-cleanup
>>> Commit: 8323374
>> Thanks Tony for ack'ing the patches.
>> Kevin,
>> Since Tony's has acked the patches, can you please pull them?
>
> Before merging, I still need to review and test this version, and I
> *might* still get to it this week.
That's fine. Thanks.
>
> Based on the dbclk aliases you added, I assume this has now been tested
> on platforms using some GPIOs with debounce enabled?
I have replicated gpio_debounce() code within *_gpio_mod_init() whereby
_set_gpio_debounce() is called to enable debounce.
After this, confirmed system going to off-mode through the CPU IDLE path.
--
Tarun
>
> Kevin
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-22 Thread Kevin Hilman 
Tarun Kanti DebBarma  writes:

> This series is continuation of cleanup of OMAP GPIO driver and fixes.
> The cleanup include getting rid of cpu_is_* checks wherever possible,
> use of gpio_bank list instead of static array, use of unique platform
> specific value associated data member to OMAP platforms to avoid
> cpu_is_* checks. The series also include PM runtime support.*

PER is still not hitting retention for me on 34xx/n900 when GPIOs have
debounce enabled.  Disabling debounce in the board file makes it work.

[...]

> - Add dbclk aliases for all GPIO modules. Without this, GPIO modules were not
>   getting the correct clock handle to enable/disable debounec clock.

This isn't right.  hwmod should already be adding aliases for the
optional clocks.

After debugging this myself a bit, here's what I think may be going on.
This may not be the only problem but here's at least one of them.

First, debounce clocks are disabled in the runtime_suspend callback.

When a GPIO is freed and it's the last one in the bank, bank->mod_usage
goes to zero.

After that, pm_runtime_put_sync() is called, which will trigger the
driver's ->runtime_suspend callback.  The ->runtime_suspend() callback
checks bank->mod_usage as well, and if zero, doesn't do anything
(notably, it doesn't disable debounce clocks.)

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-22 Thread Kevin Hilman 
Hi Tarun,

"DebBarma, Tarun Kanti"  writes:

> On Tue, Sep 13, 2011 at 6:32 PM, Tarun Kanti DebBarma
>  wrote:
>> This series is continuation of cleanup of OMAP GPIO driver and fixes.
>> The cleanup include getting rid of cpu_is_* checks wherever possible,
>> use of gpio_bank list instead of static array, use of unique platform
>> specific value associated data member to OMAP platforms to avoid
>> cpu_is_* checks. The series also include PM runtime support.*
>>
>> Baseline: git://gitorious.org/khilman/linux-omap-pm.git
>> Branch: for_3.2/gpio-cleanup
>> Commit: 8323374
> Thanks Tony for ack'ing the patches.
> Kevin,
> Since Tony's has acked the patches, can you please pull them?

Before merging, I still need to review and test this version, and I
*might* still get to it this week.

Based on the dbclk aliases you added, I assume this has now been tested
on platforms using some GPIOs with debounce enabled?

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-22 Thread DebBarma, Tarun Kanti 
On Tue, Sep 13, 2011 at 6:32 PM, Tarun Kanti DebBarma
 wrote:
> This series is continuation of cleanup of OMAP GPIO driver and fixes.
> The cleanup include getting rid of cpu_is_* checks wherever possible,
> use of gpio_bank list instead of static array, use of unique platform
> specific value associated data member to OMAP platforms to avoid
> cpu_is_* checks. The series also include PM runtime support.*
>
> Baseline: git://gitorious.org/khilman/linux-omap-pm.git
> Branch: for_3.2/gpio-cleanup
> Commit: 8323374
Thanks Tony for ack'ing the patches.
Kevin,
Since Tony's has acked the patches, can you please pull them?
Thanks.
--
Tarun
>
> Test Details:
> - Compile tested for omap1_defconfig and omap2plus_defconfig.
> - OMAP1710-H3: Bootup test.
> - OMAP2430/SDP, OMAP3430/SDP, OMAP4430/SDP: Functional testing.
> - PM Testing on OMAP3430-SDP: retention, off_mode, system_wide
>  suspend and gpio wakeup.
>
> v7:
> - Use pm_runtime_put() instead of pm_runtime_put_sync_suspend()
>
> - Keep *_runtime_get/put*()  outside spinlock
>
> - Remove additional checking of conditions in _restore_context()
>  From:
>  if (bank->regs->set_dataout && bank->regs->clear_dataout)
>  ...
>  To:
>  if (bank->regs->set_dataout)
>  ...
>
> - Use SET_RUNTIME_PM_OPS and SET_SYSTEM_SLEEP_PM_OPS macros
>
> - In [PATCH 19/25] gpio/omap: cleanup prepare_for_idle and resume_after_idle,
>  protect the bank data elements and register access using spinlock in
>  runtime_suspend/resume() callbacks.
>  This is because these callbacks run with interrupts enabled.
>
> - Add dbclk aliases for all GPIO modules. Without this, GPIO modules were not
>  getting the correct clock handle to enable/disable debounec clock.
>
> - Fix log comments on the following patches:
>  [PATCH 19/25] gpio/omap: cleanup prepare_for_idle and resume_after_idle
>  [PATCH 20/25] gpio/omap: skip operations in runtime callbacks
>  [PATCH 24/25] gpio/omap: restore OE only after setting the output level
>
> v6:
> - Save and restore debounce registers for proper driver operation.
> - Restore interrupt enable after all configuration to avoid spurious 
> interrupts.
> - Restore dataout register before oe register.
> - Restore dataout into dataout_set or dataout based upon the OMAP version.
> - Change register name from wkup_status to wkup_en.
> - Remove wrapper around omap_pm_get_dev_context_loss_count(). Use it directly.
>  Also, changed the signature of get_context_loss_count in pdata and bank 
> structure
>  from int to u32.
>
> - Use 'context' instead of 'ctx' for clarity wherever it is used.
> - Merged two patches into one which are related to bank_is_mpuio() 
> modification.
> - Use shift operator instead of following:
> +       .irqctrl        = OMAP_MPUIO_GPIO_INT_EDGE / 2,
>
> - Remove redundant check from the following
> +       if (bank_is_mpuio(bank)) {
> +               if (bank->regs->wkup_status) <--- redundant check
> +                       mpuio_init(bank);
>
> - Change subject of following patch
>  [PATCH v5 15/22] gpio/omap: use readl in irq_handler for all access
>  into
>  [PATCH 14/25] gpio/omap: remove unnecessary bit-masking for read access
>
> - Fix multi-line comments in
>  [PATCH v5 20/22] gpio/omap: cleanup prepare_for_idle and resume_after_idle
>
> v5:
> - Reduce runtime callback overhead when *_get/put_sync() called from probe()
>  and *_gpio_request/free().
>
> - Dynamic context save within functions where context is modified instead of
>  saving all context within a common function.
>
> - Removed call to mpuio_init() from omap_gpio_mod_init(). Both the functions 
> are
>  called once during initialization in *_gpio_probe().
>  Call to omap_gpio_mod_init() has been removed from omap_gpio_request() on the
>  first access to gpio bank. One time initialization looks sufficient.
>
> - In *_gpio_irq_handler() use *_put_sync_suspend() instead of *_put_sync().
>
> - Removed hardcoding of OMAP16xx sysconfig register value and instead defined 
> an
>  associated constant.
>
> - Removed *_get_sync() call from *_gpio_suspend() and *_put_sync() call from
>  *_gpio_resume(). They got wrongly slipped into the code.
>
> - Removed following redundant zero allocated initialization from 
> mach-omap2/gpio.c
> +       pdata->regs->irqctrl = 0;
> +       pdata->regs->edgectrl1 = 0;
> +       pdata->regs->edgectrl2 = 0;
>
> - Removed following redundant code in gpio-omap.c
>  -#define bank_is_mpuio(bank)  ((bank)->method == METHOD_MPUIO)
>
> v4:
> - since all accesses to registers are 4-byte aligned, removing special
>  checks and handling of 16 and 32-bit wide bank registers and instead
>  use 32-bit read/write access consistently.
>
> - redundant usage of MOD_REG_BIT has been corrected and replaced with
>  _gpio_rmw().
>
> - omap_gpio_mod_init() function has been simplified further using _gpio_rmw().
>
> - sysconfig register offset specific to omap16xx has been removed along
>  with its usage.
>
> - additional logic to skip from suspend/resume:
>
>  if (!bank->regs->wk

[PATCH v7 00/26] gpio/omap: driver cleanup and fixes

2011-09-13 Thread Tarun Kanti DebBarma 
This series is continuation of cleanup of OMAP GPIO driver and fixes.
The cleanup include getting rid of cpu_is_* checks wherever possible,
use of gpio_bank list instead of static array, use of unique platform
specific value associated data member to OMAP platforms to avoid
cpu_is_* checks. The series also include PM runtime support.*

Baseline: git://gitorious.org/khilman/linux-omap-pm.git
Branch: for_3.2/gpio-cleanup
Commit: 8323374

Test Details:
- Compile tested for omap1_defconfig and omap2plus_defconfig.
- OMAP1710-H3: Bootup test.
- OMAP2430/SDP, OMAP3430/SDP, OMAP4430/SDP: Functional testing.
- PM Testing on OMAP3430-SDP: retention, off_mode, system_wide
  suspend and gpio wakeup.

v7:
- Use pm_runtime_put() instead of pm_runtime_put_sync_suspend()

- Keep *_runtime_get/put*()  outside spinlock

- Remove additional checking of conditions in _restore_context()
  From:
  if (bank->regs->set_dataout && bank->regs->clear_dataout)
  ...
  To:
  if (bank->regs->set_dataout)
  ...

- Use SET_RUNTIME_PM_OPS and SET_SYSTEM_SLEEP_PM_OPS macros

- In [PATCH 19/25] gpio/omap: cleanup prepare_for_idle and resume_after_idle,
  protect the bank data elements and register access using spinlock in
  runtime_suspend/resume() callbacks.
  This is because these callbacks run with interrupts enabled.

- Add dbclk aliases for all GPIO modules. Without this, GPIO modules were not
  getting the correct clock handle to enable/disable debounec clock.

- Fix log comments on the following patches:
  [PATCH 19/25] gpio/omap: cleanup prepare_for_idle and resume_after_idle
  [PATCH 20/25] gpio/omap: skip operations in runtime callbacks
  [PATCH 24/25] gpio/omap: restore OE only after setting the output level

v6:
- Save and restore debounce registers for proper driver operation.
- Restore interrupt enable after all configuration to avoid spurious interrupts.
- Restore dataout register before oe register.
- Restore dataout into dataout_set or dataout based upon the OMAP version.
- Change register name from wkup_status to wkup_en.
- Remove wrapper around omap_pm_get_dev_context_loss_count(). Use it directly.
  Also, changed the signature of get_context_loss_count in pdata and bank 
structure
  from int to u32.

- Use 'context' instead of 'ctx' for clarity wherever it is used.
- Merged two patches into one which are related to bank_is_mpuio() modification.
- Use shift operator instead of following:
+   .irqctrl= OMAP_MPUIO_GPIO_INT_EDGE / 2,

- Remove redundant check from the following
+   if (bank_is_mpuio(bank)) {
+   if (bank->regs->wkup_status) <--- redundant check
+   mpuio_init(bank);

- Change subject of following patch
  [PATCH v5 15/22] gpio/omap: use readl in irq_handler for all access
  into
  [PATCH 14/25] gpio/omap: remove unnecessary bit-masking for read access

- Fix multi-line comments in
  [PATCH v5 20/22] gpio/omap: cleanup prepare_for_idle and resume_after_idle

v5:
- Reduce runtime callback overhead when *_get/put_sync() called from probe()
  and *_gpio_request/free().

- Dynamic context save within functions where context is modified instead of
  saving all context within a common function.

- Removed call to mpuio_init() from omap_gpio_mod_init(). Both the functions are
  called once during initialization in *_gpio_probe().
  Call to omap_gpio_mod_init() has been removed from omap_gpio_request() on the
  first access to gpio bank. One time initialization looks sufficient.

- In *_gpio_irq_handler() use *_put_sync_suspend() instead of *_put_sync().

- Removed hardcoding of OMAP16xx sysconfig register value and instead defined an
  associated constant.

- Removed *_get_sync() call from *_gpio_suspend() and *_put_sync() call from
  *_gpio_resume(). They got wrongly slipped into the code.

- Removed following redundant zero allocated initialization from 
mach-omap2/gpio.c
+   pdata->regs->irqctrl = 0;
+   pdata->regs->edgectrl1 = 0;
+   pdata->regs->edgectrl2 = 0;

- Removed following redundant code in gpio-omap.c
  -#define bank_is_mpuio(bank)  ((bank)->method == METHOD_MPUIO)

v4:
- since all accesses to registers are 4-byte aligned, removing special
  checks and handling of 16 and 32-bit wide bank registers and instead
  use 32-bit read/write access consistently.

- redundant usage of MOD_REG_BIT has been corrected and replaced with
  _gpio_rmw().

- omap_gpio_mod_init() function has been simplified further using _gpio_rmw().

- sysconfig register offset specific to omap16xx has been removed along
  with its usage.

- additional logic to skip from suspend/resume:

  if (!bank->regs->wkup_status || !bank->suspend_wakeup)
return 0;

  if (!bank->regs->wkup_status || !bank->saved_wakeup)
return 0;

- separated mpuio related changes into a different patch from the patch where
  wakeup status register related changes are done.

- Incorrect replacement of !cpu_class_is_omap2() in gpio_irq_type()
  corrected:
+   if (!bank->regs->leveldete