[GIT PULL] gpio/omap: cleanups for v3.5

[GIT PULL] gpio/omap: cleanups for v3.5

[Kevin Hilman]

Hi Grant,

Here's the final round of GPIO cleanups for v3.5.  This branch is based
on my for_3.5/fixes/gpio branch you just pulled.

Kevin


The following changes since commit 6edd94db250038c8fdf176f23ca4017d2f312509:

  gpio/omap: fix incorrect initialization of omap_gpio_mod_init (2012-05-10 07:16:15 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.5/gpio/cleanup

for you to fetch changes up to 1b1287032df3a69d3ef9a486b444f4ffcca50d01:

  gpio/omap: fix missing check in *_runtime_suspend() (2012-05-11 17:08:40 -0700)

----------------------------------------------------------------
Tarun Kanti DebBarma (8):
      gpio/omap: remove virtual_irq_start variable
      gpio/omap: remove saved_fallingdetect, saved_risingdetect
      gpio/omap: remove suspend_wakeup field from struct gpio_bank
      gpio/omap: remove saved_wakeup field from struct gpio_bank
      gpio/omap: remove retrigger variable in gpio_irq_handler
      gpio/omap: remove suspend/resume callbacks
      gpio/omap: remove cpu_is_omapxxxx() checks from *_runtime_resume()
      gpio/omap: fix missing check in *_runtime_suspend()

 arch/arm/mach-omap1/gpio15xx.c         |    2 -
 arch/arm/mach-omap1/gpio16xx.c         |    5 --
 arch/arm/mach-omap1/gpio7xx.c          |    7 ---
 arch/arm/mach-omap2/gpio.c             |    3 +-
 arch/arm/plat-omap/include/plat/gpio.h |    3 +-
 drivers/gpio/gpio-omap.c               |  103 +++++++-------------------------
 6 files changed, 27 insertions(+), 96 deletions(-)

[GIT PULL] gpio/omap: cleanups for v3.5

[Grant Likely]

On Fri, 11 May 2012 17:30:48 -0700, Kevin Hilman <khilman@ti.com> wrote:
> Hi Grant,
> 
> Here's the final round of GPIO cleanups for v3.5.  This branch is based
> on my for_3.5/fixes/gpio branch you just pulled.
> 
> Kevin
> 
> 
> The following changes since commit 6edd94db250038c8fdf176f23ca4017d2f312509:
> 
>   gpio/omap: fix incorrect initialization of omap_gpio_mod_init (2012-05-10 07:16:15 -0700)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.5/gpio/cleanup

Applied, thanks.

g.

> 
> for you to fetch changes up to 1b1287032df3a69d3ef9a486b444f4ffcca50d01:
> 
>   gpio/omap: fix missing check in *_runtime_suspend() (2012-05-11 17:08:40 -0700)
> 
> ----------------------------------------------------------------
> Tarun Kanti DebBarma (8):
>       gpio/omap: remove virtual_irq_start variable
>       gpio/omap: remove saved_fallingdetect, saved_risingdetect
>       gpio/omap: remove suspend_wakeup field from struct gpio_bank
>       gpio/omap: remove saved_wakeup field from struct gpio_bank
>       gpio/omap: remove retrigger variable in gpio_irq_handler
>       gpio/omap: remove suspend/resume callbacks
>       gpio/omap: remove cpu_is_omapxxxx() checks from *_runtime_resume()
>       gpio/omap: fix missing check in *_runtime_suspend()
> 
>  arch/arm/mach-omap1/gpio15xx.c         |    2 -
>  arch/arm/mach-omap1/gpio16xx.c         |    5 --
>  arch/arm/mach-omap1/gpio7xx.c          |    7 ---
>  arch/arm/mach-omap2/gpio.c             |    3 +-
>  arch/arm/plat-omap/include/plat/gpio.h |    3 +-
>  drivers/gpio/gpio-omap.c               |  103 +++++++-------------------------
>  6 files changed, 27 insertions(+), 96 deletions(-)

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

[GIT PULL] gpio/omap: cleanups for v3.5

[NeilBrown]

On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <khilman@ti.com> wrote:

> Hi Grant,
> 
> Here's the final round of GPIO cleanups for v3.5.  This branch is based
> on my for_3.5/fixes/gpio branch you just pulled.
> 
> Kevin

Hi.

 I'm not sure if it was this series or the following cleanups which broke
 things for me, but I've been trying 3.5-rc2 on my GTA04 and the serial
 console (ttyO2) dies as soon as the omap-gpio driver initialises.

 After some digging I came up with this patch to gpio-omap.c

@@ -1124,6 +1124,9 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, bank);
 
+	if (bank->get_context_loss_count)
+		bank->context_loss_count =
+				bank->get_context_loss_count(bank->dev);
 	pm_runtime_enable(bank->dev);
 	pm_runtime_irq_safe(bank->dev);
 	pm_runtime_get_sync(bank->dev);

which fixes it.

What was happening  was that when omap_gpio_probe calls pm_runtime_get_sync,
it calls 
  _od_runtime_resume -> pm_generic_runtime_resume -> omap_gpio_runtime_resume
  -> omap_gpio_restore_context

and then the serial port stops.
I reasoned that the context probably hadn't been set up yet, so restoring
from it broke things.
Initialising bank->context_loss_count seems sensible and would ensure that
we didn't try to restore the context until it has actually been lost.

Thanks,
NeilBrown
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 828 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120614/a6c9c2c9/attachment.sig>

[GIT PULL] gpio/omap: cleanups for v3.5

[DebBarma, Tarun Kanti]

On Thu, Jun 14, 2012 at 5:45 AM, NeilBrown <neilb@suse.de> wrote:
> On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <khilman@ti.com> wrote:
>
>> Hi Grant,
>>
>> Here's the final round of GPIO cleanups for v3.5. ?This branch is based
>> on my for_3.5/fixes/gpio branch you just pulled.
>>
>> Kevin
>
> Hi.
>
> ?I'm not sure if it was this series or the following cleanups which broke
> ?things for me, but I've been trying 3.5-rc2 on my GTA04 and the serial
> ?console (ttyO2) dies as soon as the omap-gpio driver initialises.
>
> ?After some digging I came up with this patch to gpio-omap.c
>
> @@ -1124,6 +1124,9 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>
> ? ? ? ?platform_set_drvdata(pdev, bank);
>
> + ? ? ? if (bank->get_context_loss_count)
> + ? ? ? ? ? ? ? bank->context_loss_count =
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
> ? ? ? ?pm_runtime_enable(bank->dev);
> ? ? ? ?pm_runtime_irq_safe(bank->dev);
> ? ? ? ?pm_runtime_get_sync(bank->dev);
>
> which fixes it.
>
> What was happening ?was that when omap_gpio_probe calls pm_runtime_get_sync,
> it calls
> ?_od_runtime_resume -> pm_generic_runtime_resume -> omap_gpio_runtime_resume
> ?-> omap_gpio_restore_context
>
> and then the serial port stops.
> I reasoned that the context probably hadn't been set up yet, so restoring
> from it broke things.
> Initialising bank->context_loss_count seems sensible and would ensure that
> we didn't try to restore the context until it has actually been lost.

I thought the following code exactly does that. That is context_lost_cnt_after
would be zero until there is context loss. The bank->context_loss_count is zero
at the beginning. So, (context_lost_cnt_after != bank->context_loss_count) would
be false and hence context restore should NOT happen? Not sure if I am
over looking
anything here....

omap_gpio_runtime_resume(...)
{
...
        if (bank->get_context_loss_count) {
                context_lost_cnt_after =
                        bank->get_context_loss_count(bank->dev);
                if (context_lost_cnt_after != bank->context_loss_count) {
                        omap_gpio_restore_context(bank);
                } else {
                        spin_unlock_irqrestore(&bank->lock, flags);
                        return 0;
                }
        }
...
}
--
Tarun
>
> Thanks,
> NeilBrown

[GIT PULL] gpio/omap: cleanups for v3.5

[NeilBrown]

On Thu, 14 Jun 2012 23:24:10 +0530 "DebBarma, Tarun Kanti"
<tarun.kanti@ti.com> wrote:

> On Thu, Jun 14, 2012 at 5:45 AM, NeilBrown <neilb@suse.de> wrote:
> > On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <khilman@ti.com> wrote:
> >
> >> Hi Grant,
> >>
> >> Here's the final round of GPIO cleanups for v3.5. ?This branch is based
> >> on my for_3.5/fixes/gpio branch you just pulled.
> >>
> >> Kevin
> >
> > Hi.
> >
> > ?I'm not sure if it was this series or the following cleanups which broke
> > ?things for me, but I've been trying 3.5-rc2 on my GTA04 and the serial
> > ?console (ttyO2) dies as soon as the omap-gpio driver initialises.
> >
> > ?After some digging I came up with this patch to gpio-omap.c
> >
> > @@ -1124,6 +1124,9 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
> >
> > ? ? ? ?platform_set_drvdata(pdev, bank);
> >
> > + ? ? ? if (bank->get_context_loss_count)
> > + ? ? ? ? ? ? ? bank->context_loss_count =
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
> > ? ? ? ?pm_runtime_enable(bank->dev);
> > ? ? ? ?pm_runtime_irq_safe(bank->dev);
> > ? ? ? ?pm_runtime_get_sync(bank->dev);
> >
> > which fixes it.
> >
> > What was happening ?was that when omap_gpio_probe calls pm_runtime_get_sync,
> > it calls
> > ?_od_runtime_resume -> pm_generic_runtime_resume -> omap_gpio_runtime_resume
> > ?-> omap_gpio_restore_context
> >
> > and then the serial port stops.
> > I reasoned that the context probably hadn't been set up yet, so restoring
> > from it broke things.
> > Initialising bank->context_loss_count seems sensible and would ensure that
> > we didn't try to restore the context until it has actually been lost.
> 
> I thought the following code exactly does that. That is context_lost_cnt_after
> would be zero until there is context loss. The bank->context_loss_count is zero
> at the beginning. So, (context_lost_cnt_after != bank->context_loss_count) would
> be false and hence context restore should NOT happen? Not sure if I am
> over looking
> anything here....

Ahhh... I think I know what might be happening.

I found a while ago that if I actually enable off_mode some things
didn't work properly, but if I just set mpu_deepest_state to PWRDM_POWER_OFF
in next_valid_state, it all does work and I use less power.  So I did that.
It has now come back to bite me I expect.

I'll revert that, enable off mode properly, and see what happens.

Thanks,

NeilBrown


> 
> omap_gpio_runtime_resume(...)
> {
> ...
>         if (bank->get_context_loss_count) {
>                 context_lost_cnt_after =
>                         bank->get_context_loss_count(bank->dev);
>                 if (context_lost_cnt_after != bank->context_loss_count) {
>                         omap_gpio_restore_context(bank);
>                 } else {
>                         spin_unlock_irqrestore(&bank->lock, flags);
>                         return 0;
>                 }
>         }
> ...
> }
> --
> Tarun
> >
> > Thanks,
> > NeilBrown
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 828 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120615/50978419/attachment.sig>

[GIT PULL] gpio/omap: cleanups for v3.5

[NeilBrown]

On Thu, 14 Jun 2012 23:24:10 +0530 "DebBarma, Tarun Kanti"
<tarun.kanti@ti.com> wrote:

> On Thu, Jun 14, 2012 at 5:45 AM, NeilBrown <neilb@suse.de> wrote:
> > On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <khilman@ti.com> wrote:
> >
> >> Hi Grant,
> >>
> >> Here's the final round of GPIO cleanups for v3.5. ?This branch is based
> >> on my for_3.5/fixes/gpio branch you just pulled.
> >>
> >> Kevin
> >
> > Hi.
> >
> > ?I'm not sure if it was this series or the following cleanups which broke
> > ?things for me, but I've been trying 3.5-rc2 on my GTA04 and the serial
> > ?console (ttyO2) dies as soon as the omap-gpio driver initialises.
> >
> > ?After some digging I came up with this patch to gpio-omap.c
> >
> > @@ -1124,6 +1124,9 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
> >
> > ? ? ? ?platform_set_drvdata(pdev, bank);
> >
> > + ? ? ? if (bank->get_context_loss_count)
> > + ? ? ? ? ? ? ? bank->context_loss_count =
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
> > ? ? ? ?pm_runtime_enable(bank->dev);
> > ? ? ? ?pm_runtime_irq_safe(bank->dev);
> > ? ? ? ?pm_runtime_get_sync(bank->dev);
> >
> > which fixes it.
> >
> > What was happening ?was that when omap_gpio_probe calls pm_runtime_get_sync,
> > it calls
> > ?_od_runtime_resume -> pm_generic_runtime_resume -> omap_gpio_runtime_resume
> > ?-> omap_gpio_restore_context
> >
> > and then the serial port stops.
> > I reasoned that the context probably hadn't been set up yet, so restoring
> > from it broke things.
> > Initialising bank->context_loss_count seems sensible and would ensure that
> > we didn't try to restore the context until it has actually been lost.
> 
> I thought the following code exactly does that. That is context_lost_cnt_after
> would be zero until there is context loss. The bank->context_loss_count is zero
> at the beginning. So, (context_lost_cnt_after != bank->context_loss_count) would
> be false and hence context restore should NOT happen? Not sure if I am
> over looking
> anything here....
> 
> omap_gpio_runtime_resume(...)
> {
> ...
>         if (bank->get_context_loss_count) {
>                 context_lost_cnt_after =
>                         bank->get_context_loss_count(bank->dev);
>                 if (context_lost_cnt_after != bank->context_loss_count) {
>                         omap_gpio_restore_context(bank);
>                 } else {
>                         spin_unlock_irqrestore(&bank->lock, flags);
>                         return 0;
>                 }
>         }
> ...
> }

Hi,
 I've looked more closely at this now.

The problem is that the initial context loss count is *not* zero.  Not always.
The context loss count is the sum of 

	count = pwrdm->state_counter[PWRDM_POWER_OFF];
	count += pwrdm->ret_logic_off_counter;

	for (i = 0; i < pwrdm->banks; i++)
		count += pwrdm->ret_mem_off_counter[i];

(from  pwrdm_get_context_loss_count()).

These are initlialised in _pwrdm_register

	/* Initialize the powerdomain's state counter */
	for (i = 0; i < PWRDM_MAX_PWRSTS; i++)
		pwrdm->state_counter[i] = 0;

	pwrdm->ret_logic_off_counter = 0;
	for (i = 0; i < pwrdm->banks; i++)
		pwrdm->ret_mem_off_counter[i] = 0;

	pwrdm_wait_transition(pwrdm);
	pwrdm->state = pwrdm_read_pwrst(pwrdm);
	pwrdm->state_counter[pwrdm->state] = 1;


What I'm seeing is that for wkup_pwrdm and dpll{3,4,5}_pwrdm,
the state that pwrdm_read_pwrst returns is PWRDM_POWER_OFF.
So that state_counter gets initialised to '1', and so the initial
context_loss_count, which includes that counter, is also '1'.
I think it is the wkup_pwrdm that covers the GPIOs that are causing problems
for me.

So either there is something seriously wrong with pwrdm_read_pwrst and it
shouldn't be reporting that the wkup_pwrdm is off, or we need to initialise
bank->context_loss_count like my patch does.

NeilBrown
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 828 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120621/1b8947b3/attachment-0001.sig>

[GIT PULL] gpio/omap: cleanups for v3.5

[DebBarma, Tarun Kanti]

On Thu, Jun 21, 2012 at 8:46 AM, NeilBrown <neilb@suse.de> wrote:
> On Thu, 14 Jun 2012 23:24:10 +0530 "DebBarma, Tarun Kanti"
> <tarun.kanti@ti.com> wrote:
>
>> On Thu, Jun 14, 2012 at 5:45 AM, NeilBrown <neilb@suse.de> wrote:
>> > On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <khilman@ti.com> wrote:
>> >
>> >> Hi Grant,
>> >>
>> >> Here's the final round of GPIO cleanups for v3.5. ?This branch is based
>> >> on my for_3.5/fixes/gpio branch you just pulled.
>> >>
>> >> Kevin
>> >
>> > Hi.
>> >
>> > ?I'm not sure if it was this series or the following cleanups which broke
>> > ?things for me, but I've been trying 3.5-rc2 on my GTA04 and the serial
>> > ?console (ttyO2) dies as soon as the omap-gpio driver initialises.
>> >
>> > ?After some digging I came up with this patch to gpio-omap.c
>> >
>> > @@ -1124,6 +1124,9 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>> >
>> > ? ? ? ?platform_set_drvdata(pdev, bank);
>> >
>> > + ? ? ? if (bank->get_context_loss_count)
>> > + ? ? ? ? ? ? ? bank->context_loss_count =
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
>> > ? ? ? ?pm_runtime_enable(bank->dev);
>> > ? ? ? ?pm_runtime_irq_safe(bank->dev);
>> > ? ? ? ?pm_runtime_get_sync(bank->dev);
>> >
>> > which fixes it.
>> >
>> > What was happening ?was that when omap_gpio_probe calls pm_runtime_get_sync,
>> > it calls
>> > ?_od_runtime_resume -> pm_generic_runtime_resume -> omap_gpio_runtime_resume
>> > ?-> omap_gpio_restore_context
>> >
>> > and then the serial port stops.
>> > I reasoned that the context probably hadn't been set up yet, so restoring
>> > from it broke things.
>> > Initialising bank->context_loss_count seems sensible and would ensure that
>> > we didn't try to restore the context until it has actually been lost.
>>
>> I thought the following code exactly does that. That is context_lost_cnt_after
>> would be zero until there is context loss. The bank->context_loss_count is zero
>> at the beginning. So, (context_lost_cnt_after != bank->context_loss_count) would
>> be false and hence context restore should NOT happen? Not sure if I am
>> over looking
>> anything here....
>>
>> omap_gpio_runtime_resume(...)
>> {
>> ...
>> ? ? ? ? if (bank->get_context_loss_count) {
>> ? ? ? ? ? ? ? ? context_lost_cnt_after =
>> ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
>> ? ? ? ? ? ? ? ? if (context_lost_cnt_after != bank->context_loss_count) {
>> ? ? ? ? ? ? ? ? ? ? ? ? omap_gpio_restore_context(bank);
>> ? ? ? ? ? ? ? ? } else {
>> ? ? ? ? ? ? ? ? ? ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>> ? ? ? ? ? ? ? ? ? ? ? ? return 0;
>> ? ? ? ? ? ? ? ? }
>> ? ? ? ? }
>> ...
>> }
>
> Hi,
> ?I've looked more closely at this now.
>
> The problem is that the initial context loss count is *not* zero. ?Not always.
> The context loss count is the sum of
>
> ? ? ? ?count = pwrdm->state_counter[PWRDM_POWER_OFF];
> ? ? ? ?count += pwrdm->ret_logic_off_counter;
>
> ? ? ? ?for (i = 0; i < pwrdm->banks; i++)
> ? ? ? ? ? ? ? ?count += pwrdm->ret_mem_off_counter[i];
>
> (from ?pwrdm_get_context_loss_count()).
>
> These are initlialised in _pwrdm_register
>
> ? ? ? ?/* Initialize the powerdomain's state counter */
> ? ? ? ?for (i = 0; i < PWRDM_MAX_PWRSTS; i++)
> ? ? ? ? ? ? ? ?pwrdm->state_counter[i] = 0;
>
> ? ? ? ?pwrdm->ret_logic_off_counter = 0;
> ? ? ? ?for (i = 0; i < pwrdm->banks; i++)
> ? ? ? ? ? ? ? ?pwrdm->ret_mem_off_counter[i] = 0;
>
> ? ? ? ?pwrdm_wait_transition(pwrdm);
> ? ? ? ?pwrdm->state = pwrdm_read_pwrst(pwrdm);
> ? ? ? ?pwrdm->state_counter[pwrdm->state] = 1;
>
>
> What I'm seeing is that for wkup_pwrdm and dpll{3,4,5}_pwrdm,
> the state that pwrdm_read_pwrst returns is PWRDM_POWER_OFF.
> So that state_counter gets initialised to '1', and so the initial
> context_loss_count, which includes that counter, is also '1'.
> I think it is the wkup_pwrdm that covers the GPIOs that are causing problems
> for me.
I just put a log in omap_gpio_probe() to see the value of context_loss_count.
GPIO Bank 0 (WKUP Domain) always shows the count as '1'.

[    0.169494] omap_gpio omap_gpio.0: context_loss_count=1
[    0.170227] gpiochip_add: registered GPIOs 0 to 31 on device: gpio
[    0.170471] OMAP GPIO hardware version 0.1
[    0.170623] omap_gpio omap_gpio.1: context_loss_count=0
[    0.170928] gpiochip_add: registered GPIOs 32 to 63 on device: gpio
[    0.171295] omap_gpio omap_gpio.2: context_loss_count=0
[    0.171600] gpiochip_add: registered GPIOs 64 to 95 on device: gpio
[    0.171936] omap_gpio omap_gpio.3: context_loss_count=0
[    0.172241] gpiochip_add: registered GPIOs 96 to 127 on device: gpio
[    0.172576] omap_gpio omap_gpio.4: context_loss_count=0
[    0.172882] gpiochip_add: registered GPIOs 128 to 159 on device: gpio
[    0.173217] omap_gpio omap_gpio.5: context_loss_count=0
[    0.173522] gpiochip_add: registered GPIOs 160 to 191 on device: gpio
--
Tarun
>
> So either there is something seriously wrong with pwrdm_read_pwrst and it
> shouldn't be reporting that the wkup_pwrdm is off, or we need to initialise
> bank->context_loss_count like my patch does.
>
> NeilBrown

[GIT PULL] gpio/omap: cleanups for v3.5

[NeilBrown]

On Thu, 21 Jun 2012 12:04:26 +0530 "DebBarma, Tarun Kanti"
<tarun.kanti@ti.com> wrote:

> On Thu, Jun 21, 2012 at 8:46 AM, NeilBrown <neilb@suse.de> wrote:
> > On Thu, 14 Jun 2012 23:24:10 +0530 "DebBarma, Tarun Kanti"
> > <tarun.kanti@ti.com> wrote:
> >
> >> On Thu, Jun 14, 2012 at 5:45 AM, NeilBrown <neilb@suse.de> wrote:
> >> > On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <khilman@ti.com> wrote:
> >> >
> >> >> Hi Grant,
> >> >>
> >> >> Here's the final round of GPIO cleanups for v3.5. ?This branch is based
> >> >> on my for_3.5/fixes/gpio branch you just pulled.
> >> >>
> >> >> Kevin
> >> >
> >> > Hi.
> >> >
> >> > ?I'm not sure if it was this series or the following cleanups which broke
> >> > ?things for me, but I've been trying 3.5-rc2 on my GTA04 and the serial
> >> > ?console (ttyO2) dies as soon as the omap-gpio driver initialises.
> >> >
> >> > ?After some digging I came up with this patch to gpio-omap.c
> >> >
> >> > @@ -1124,6 +1124,9 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
> >> >
> >> > ? ? ? ?platform_set_drvdata(pdev, bank);
> >> >
> >> > + ? ? ? if (bank->get_context_loss_count)
> >> > + ? ? ? ? ? ? ? bank->context_loss_count =
> >> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
> >> > ? ? ? ?pm_runtime_enable(bank->dev);
> >> > ? ? ? ?pm_runtime_irq_safe(bank->dev);
> >> > ? ? ? ?pm_runtime_get_sync(bank->dev);
> >> >
> >> > which fixes it.
> >> >
> >> > What was happening ?was that when omap_gpio_probe calls pm_runtime_get_sync,
> >> > it calls
> >> > ?_od_runtime_resume -> pm_generic_runtime_resume -> omap_gpio_runtime_resume
> >> > ?-> omap_gpio_restore_context
> >> >
> >> > and then the serial port stops.
> >> > I reasoned that the context probably hadn't been set up yet, so restoring
> >> > from it broke things.
> >> > Initialising bank->context_loss_count seems sensible and would ensure that
> >> > we didn't try to restore the context until it has actually been lost.
> >>
> >> I thought the following code exactly does that. That is context_lost_cnt_after
> >> would be zero until there is context loss. The bank->context_loss_count is zero
> >> at the beginning. So, (context_lost_cnt_after != bank->context_loss_count) would
> >> be false and hence context restore should NOT happen? Not sure if I am
> >> over looking
> >> anything here....
> >>
> >> omap_gpio_runtime_resume(...)
> >> {
> >> ...
> >> ? ? ? ? if (bank->get_context_loss_count) {
> >> ? ? ? ? ? ? ? ? context_lost_cnt_after =
> >> ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
> >> ? ? ? ? ? ? ? ? if (context_lost_cnt_after != bank->context_loss_count) {
> >> ? ? ? ? ? ? ? ? ? ? ? ? omap_gpio_restore_context(bank);
> >> ? ? ? ? ? ? ? ? } else {
> >> ? ? ? ? ? ? ? ? ? ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
> >> ? ? ? ? ? ? ? ? ? ? ? ? return 0;
> >> ? ? ? ? ? ? ? ? }
> >> ? ? ? ? }
> >> ...
> >> }
> >
> > Hi,
> > ?I've looked more closely at this now.
> >
> > The problem is that the initial context loss count is *not* zero. ?Not always.
> > The context loss count is the sum of
> >
> > ? ? ? ?count = pwrdm->state_counter[PWRDM_POWER_OFF];
> > ? ? ? ?count += pwrdm->ret_logic_off_counter;
> >
> > ? ? ? ?for (i = 0; i < pwrdm->banks; i++)
> > ? ? ? ? ? ? ? ?count += pwrdm->ret_mem_off_counter[i];
> >
> > (from ?pwrdm_get_context_loss_count()).
> >
> > These are initlialised in _pwrdm_register
> >
> > ? ? ? ?/* Initialize the powerdomain's state counter */
> > ? ? ? ?for (i = 0; i < PWRDM_MAX_PWRSTS; i++)
> > ? ? ? ? ? ? ? ?pwrdm->state_counter[i] = 0;
> >
> > ? ? ? ?pwrdm->ret_logic_off_counter = 0;
> > ? ? ? ?for (i = 0; i < pwrdm->banks; i++)
> > ? ? ? ? ? ? ? ?pwrdm->ret_mem_off_counter[i] = 0;
> >
> > ? ? ? ?pwrdm_wait_transition(pwrdm);
> > ? ? ? ?pwrdm->state = pwrdm_read_pwrst(pwrdm);
> > ? ? ? ?pwrdm->state_counter[pwrdm->state] = 1;
> >
> >
> > What I'm seeing is that for wkup_pwrdm and dpll{3,4,5}_pwrdm,
> > the state that pwrdm_read_pwrst returns is PWRDM_POWER_OFF.
> > So that state_counter gets initialised to '1', and so the initial
> > context_loss_count, which includes that counter, is also '1'.
> > I think it is the wkup_pwrdm that covers the GPIOs that are causing problems
> > for me.
> I just put a log in omap_gpio_probe() to see the value of context_loss_count.
> GPIO Bank 0 (WKUP Domain) always shows the count as '1'.
> 
> [    0.169494] omap_gpio omap_gpio.0: context_loss_count=1
> [    0.170227] gpiochip_add: registered GPIOs 0 to 31 on device: gpio
> [    0.170471] OMAP GPIO hardware version 0.1
> [    0.170623] omap_gpio omap_gpio.1: context_loss_count=0
> [    0.170928] gpiochip_add: registered GPIOs 32 to 63 on device: gpio
> [    0.171295] omap_gpio omap_gpio.2: context_loss_count=0
> [    0.171600] gpiochip_add: registered GPIOs 64 to 95 on device: gpio
> [    0.171936] omap_gpio omap_gpio.3: context_loss_count=0
> [    0.172241] gpiochip_add: registered GPIOs 96 to 127 on device: gpio
> [    0.172576] omap_gpio omap_gpio.4: context_loss_count=0
> [    0.172882] gpiochip_add: registered GPIOs 128 to 159 on device: gpio
> [    0.173217] omap_gpio omap_gpio.5: context_loss_count=0
> [    0.173522] gpiochip_add: registered GPIOs 160 to 191 on device: gpio

That's consistent with what I see, and confirm that initialising the
context_lost_count to zero isn't always correct.

Thanks,
NeilBrown


> --
> Tarun
> >
> > So either there is something seriously wrong with pwrdm_read_pwrst and it
> > shouldn't be reporting that the wkup_pwrdm is off, or we need to initialise
> > bank->context_loss_count like my patch does.
> >
> > NeilBrown
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 828 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120625/ee7b2609/attachment-0001.sig>

[GIT PULL] gpio/omap: cleanups for v3.5

[DebBarma, Tarun Kanti]

On Mon, Jun 25, 2012 at 11:48 AM, NeilBrown <neilb@suse.de> wrote:
> On Thu, 21 Jun 2012 12:04:26 +0530 "DebBarma, Tarun Kanti"
> <tarun.kanti@ti.com> wrote:
>
>> On Thu, Jun 21, 2012 at 8:46 AM, NeilBrown <neilb@suse.de> wrote:
>> > On Thu, 14 Jun 2012 23:24:10 +0530 "DebBarma, Tarun Kanti"
>> > <tarun.kanti@ti.com> wrote:
>> >
>> >> On Thu, Jun 14, 2012 at 5:45 AM, NeilBrown <neilb@suse.de> wrote:
>> >> > On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <khilman@ti.com> wrote:
>> >> >
>> >> >> Hi Grant,
>> >> >>
>> >> >> Here's the final round of GPIO cleanups for v3.5. ?This branch is based
>> >> >> on my for_3.5/fixes/gpio branch you just pulled.
>> >> >>
>> >> >> Kevin
>> >> >
>> >> > Hi.
>> >> >
>> >> > ?I'm not sure if it was this series or the following cleanups which broke
>> >> > ?things for me, but I've been trying 3.5-rc2 on my GTA04 and the serial
>> >> > ?console (ttyO2) dies as soon as the omap-gpio driver initialises.
>> >> >
>> >> > ?After some digging I came up with this patch to gpio-omap.c
>> >> >
>> >> > @@ -1124,6 +1124,9 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>> >> >
>> >> > ? ? ? ?platform_set_drvdata(pdev, bank);
>> >> >
>> >> > + ? ? ? if (bank->get_context_loss_count)
>> >> > + ? ? ? ? ? ? ? bank->context_loss_count =
>> >> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
>> >> > ? ? ? ?pm_runtime_enable(bank->dev);
>> >> > ? ? ? ?pm_runtime_irq_safe(bank->dev);
>> >> > ? ? ? ?pm_runtime_get_sync(bank->dev);
>> >> >
>> >> > which fixes it.
>> >> >
>> >> > What was happening ?was that when omap_gpio_probe calls pm_runtime_get_sync,
>> >> > it calls
>> >> > ?_od_runtime_resume -> pm_generic_runtime_resume -> omap_gpio_runtime_resume
>> >> > ?-> omap_gpio_restore_context
>> >> >
>> >> > and then the serial port stops.
>> >> > I reasoned that the context probably hadn't been set up yet, so restoring
>> >> > from it broke things.
>> >> > Initialising bank->context_loss_count seems sensible and would ensure that
>> >> > we didn't try to restore the context until it has actually been lost.
>> >>
>> >> I thought the following code exactly does that. That is context_lost_cnt_after
>> >> would be zero until there is context loss. The bank->context_loss_count is zero
>> >> at the beginning. So, (context_lost_cnt_after != bank->context_loss_count) would
>> >> be false and hence context restore should NOT happen? Not sure if I am
>> >> over looking
>> >> anything here....
>> >>
>> >> omap_gpio_runtime_resume(...)
>> >> {
>> >> ...
>> >> ? ? ? ? if (bank->get_context_loss_count) {
>> >> ? ? ? ? ? ? ? ? context_lost_cnt_after =
>> >> ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
>> >> ? ? ? ? ? ? ? ? if (context_lost_cnt_after != bank->context_loss_count) {
>> >> ? ? ? ? ? ? ? ? ? ? ? ? omap_gpio_restore_context(bank);
>> >> ? ? ? ? ? ? ? ? } else {
>> >> ? ? ? ? ? ? ? ? ? ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>> >> ? ? ? ? ? ? ? ? ? ? ? ? return 0;
>> >> ? ? ? ? ? ? ? ? }
>> >> ? ? ? ? }
>> >> ...
>> >> }
>> >
>> > Hi,
>> > ?I've looked more closely at this now.
>> >
>> > The problem is that the initial context loss count is *not* zero. ?Not always.
>> > The context loss count is the sum of
>> >
>> > ? ? ? ?count = pwrdm->state_counter[PWRDM_POWER_OFF];
>> > ? ? ? ?count += pwrdm->ret_logic_off_counter;
>> >
>> > ? ? ? ?for (i = 0; i < pwrdm->banks; i++)
>> > ? ? ? ? ? ? ? ?count += pwrdm->ret_mem_off_counter[i];
>> >
>> > (from ?pwrdm_get_context_loss_count()).
>> >
>> > These are initlialised in _pwrdm_register
>> >
>> > ? ? ? ?/* Initialize the powerdomain's state counter */
>> > ? ? ? ?for (i = 0; i < PWRDM_MAX_PWRSTS; i++)
>> > ? ? ? ? ? ? ? ?pwrdm->state_counter[i] = 0;
>> >
>> > ? ? ? ?pwrdm->ret_logic_off_counter = 0;
>> > ? ? ? ?for (i = 0; i < pwrdm->banks; i++)
>> > ? ? ? ? ? ? ? ?pwrdm->ret_mem_off_counter[i] = 0;
>> >
>> > ? ? ? ?pwrdm_wait_transition(pwrdm);
>> > ? ? ? ?pwrdm->state = pwrdm_read_pwrst(pwrdm);
>> > ? ? ? ?pwrdm->state_counter[pwrdm->state] = 1;
>> >
>> >
>> > What I'm seeing is that for wkup_pwrdm and dpll{3,4,5}_pwrdm,
>> > the state that pwrdm_read_pwrst returns is PWRDM_POWER_OFF.
>> > So that state_counter gets initialised to '1', and so the initial
>> > context_loss_count, which includes that counter, is also '1'.
>> > I think it is the wkup_pwrdm that covers the GPIOs that are causing problems
>> > for me.
>> I just put a log in omap_gpio_probe() to see the value of context_loss_count.
>> GPIO Bank 0 (WKUP Domain) always shows the count as '1'.
>>
>> [ ? ?0.169494] omap_gpio omap_gpio.0: context_loss_count=1
>> [ ? ?0.170227] gpiochip_add: registered GPIOs 0 to 31 on device: gpio
>> [ ? ?0.170471] OMAP GPIO hardware version 0.1
>> [ ? ?0.170623] omap_gpio omap_gpio.1: context_loss_count=0
>> [ ? ?0.170928] gpiochip_add: registered GPIOs 32 to 63 on device: gpio
>> [ ? ?0.171295] omap_gpio omap_gpio.2: context_loss_count=0
>> [ ? ?0.171600] gpiochip_add: registered GPIOs 64 to 95 on device: gpio
>> [ ? ?0.171936] omap_gpio omap_gpio.3: context_loss_count=0
>> [ ? ?0.172241] gpiochip_add: registered GPIOs 96 to 127 on device: gpio
>> [ ? ?0.172576] omap_gpio omap_gpio.4: context_loss_count=0
>> [ ? ?0.172882] gpiochip_add: registered GPIOs 128 to 159 on device: gpio
>> [ ? ?0.173217] omap_gpio omap_gpio.5: context_loss_count=0
>> [ ? ?0.173522] gpiochip_add: registered GPIOs 160 to 191 on device: gpio
>
> That's consistent with what I see, and confirm that initialising the
> context_lost_count to zero isn't always correct.
I am just wondering if the context_lost_count = 1 for GPIO in WKUP domain
is expected. In that case we have to add additional logic in runtime callbacks
to skip context restore/save for WKUP domain GPIOs.
But let's hear what Kevin says.
--
Tarun

>
> Thanks,
> NeilBrown
>
>
>> --
>> Tarun
>> >
>> > So either there is something seriously wrong with pwrdm_read_pwrst and it
>> > shouldn't be reporting that the wkup_pwrdm is off, or we need to initialise
>> > bank->context_loss_count like my patch does.
>> >
>> > NeilBrown
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

[GIT PULL] gpio/omap: cleanups for v3.5

[Kevin Hilman]

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

> On Mon, Jun 25, 2012 at 11:48 AM, NeilBrown <neilb@suse.de> wrote:
>> On Thu, 21 Jun 2012 12:04:26 +0530 "DebBarma, Tarun Kanti"
>> <tarun.kanti@ti.com> wrote:
>>
>>> On Thu, Jun 21, 2012 at 8:46 AM, NeilBrown <neilb@suse.de> wrote:
>>> > On Thu, 14 Jun 2012 23:24:10 +0530 "DebBarma, Tarun Kanti"
>>> > <tarun.kanti@ti.com> wrote:
>>> >
>>> >> On Thu, Jun 14, 2012 at 5:45 AM, NeilBrown <neilb@suse.de> wrote:
>>> >> > On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <khilman@ti.com> wrote:
>>> >> >
>>> >> >> Hi Grant,
>>> >> >>
>>> >> >> Here's the final round of GPIO cleanups for v3.5. ?This branch is based
>>> >> >> on my for_3.5/fixes/gpio branch you just pulled.
>>> >> >>
>>> >> >> Kevin
>>> >> >
>>> >> > Hi.
>>> >> >
>>> >> > ?I'm not sure if it was this series or the following cleanups which broke
>>> >> > ?things for me, but I've been trying 3.5-rc2 on my GTA04 and the serial
>>> >> > ?console (ttyO2) dies as soon as the omap-gpio driver initialises.
>>> >> >
>>> >> > ?After some digging I came up with this patch to gpio-omap.c
>>> >> >
>>> >> > @@ -1124,6 +1124,9 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>>> >> >
>>> >> > ? ? ? ?platform_set_drvdata(pdev, bank);
>>> >> >
>>> >> > + ? ? ? if (bank->get_context_loss_count)
>>> >> > + ? ? ? ? ? ? ? bank->context_loss_count =
>>> >> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
>>> >> > ? ? ? ?pm_runtime_enable(bank->dev);
>>> >> > ? ? ? ?pm_runtime_irq_safe(bank->dev);
>>> >> > ? ? ? ?pm_runtime_get_sync(bank->dev);
>>> >> >
>>> >> > which fixes it.
>>> >> >
>>> >> > What was happening ?was that when omap_gpio_probe calls pm_runtime_get_sync,
>>> >> > it calls
>>> >> > ?_od_runtime_resume -> pm_generic_runtime_resume -> omap_gpio_runtime_resume
>>> >> > ?-> omap_gpio_restore_context
>>> >> >
>>> >> > and then the serial port stops.
>>> >> > I reasoned that the context probably hadn't been set up yet, so restoring
>>> >> > from it broke things.
>>> >> > Initialising bank->context_loss_count seems sensible and would ensure that
>>> >> > we didn't try to restore the context until it has actually been lost.
>>> >>
>>> >> I thought the following code exactly does that. That is context_lost_cnt_after
>>> >> would be zero until there is context loss. The bank->context_loss_count is zero
>>> >> at the beginning. So, (context_lost_cnt_after != bank->context_loss_count) would
>>> >> be false and hence context restore should NOT happen? Not sure if I am
>>> >> over looking
>>> >> anything here....
>>> >>
>>> >> omap_gpio_runtime_resume(...)
>>> >> {
>>> >> ...
>>> >> ? ? ? ? if (bank->get_context_loss_count) {
>>> >> ? ? ? ? ? ? ? ? context_lost_cnt_after =
>>> >> ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
>>> >> ? ? ? ? ? ? ? ? if (context_lost_cnt_after != bank->context_loss_count) {
>>> >> ? ? ? ? ? ? ? ? ? ? ? ? omap_gpio_restore_context(bank);
>>> >> ? ? ? ? ? ? ? ? } else {
>>> >> ? ? ? ? ? ? ? ? ? ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>>> >> ? ? ? ? ? ? ? ? ? ? ? ? return 0;
>>> >> ? ? ? ? ? ? ? ? }
>>> >> ? ? ? ? }
>>> >> ...
>>> >> }
>>> >
>>> > Hi,
>>> > ?I've looked more closely at this now.
>>> >
>>> > The problem is that the initial context loss count is *not* zero. ?Not always.
>>> > The context loss count is the sum of
>>> >
>>> > ? ? ? ?count = pwrdm->state_counter[PWRDM_POWER_OFF];
>>> > ? ? ? ?count += pwrdm->ret_logic_off_counter;
>>> >
>>> > ? ? ? ?for (i = 0; i < pwrdm->banks; i++)
>>> > ? ? ? ? ? ? ? ?count += pwrdm->ret_mem_off_counter[i];
>>> >
>>> > (from ?pwrdm_get_context_loss_count()).
>>> >
>>> > These are initlialised in _pwrdm_register
>>> >
>>> > ? ? ? ?/* Initialize the powerdomain's state counter */
>>> > ? ? ? ?for (i = 0; i < PWRDM_MAX_PWRSTS; i++)
>>> > ? ? ? ? ? ? ? ?pwrdm->state_counter[i] = 0;
>>> >
>>> > ? ? ? ?pwrdm->ret_logic_off_counter = 0;
>>> > ? ? ? ?for (i = 0; i < pwrdm->banks; i++)
>>> > ? ? ? ? ? ? ? ?pwrdm->ret_mem_off_counter[i] = 0;
>>> >
>>> > ? ? ? ?pwrdm_wait_transition(pwrdm);
>>> > ? ? ? ?pwrdm->state = pwrdm_read_pwrst(pwrdm);
>>> > ? ? ? ?pwrdm->state_counter[pwrdm->state] = 1;
>>> >
>>> >
>>> > What I'm seeing is that for wkup_pwrdm and dpll{3,4,5}_pwrdm,
>>> > the state that pwrdm_read_pwrst returns is PWRDM_POWER_OFF.
>>> > So that state_counter gets initialised to '1', and so the initial
>>> > context_loss_count, which includes that counter, is also '1'.
>>> > I think it is the wkup_pwrdm that covers the GPIOs that are causing problems
>>> > for me.
>>> I just put a log in omap_gpio_probe() to see the value of context_loss_count.
>>> GPIO Bank 0 (WKUP Domain) always shows the count as '1'.
>>>
>>> [ ? ?0.169494] omap_gpio omap_gpio.0: context_loss_count=1
>>> [ ? ?0.170227] gpiochip_add: registered GPIOs 0 to 31 on device: gpio
>>> [ ? ?0.170471] OMAP GPIO hardware version 0.1
>>> [ ? ?0.170623] omap_gpio omap_gpio.1: context_loss_count=0
>>> [ ? ?0.170928] gpiochip_add: registered GPIOs 32 to 63 on device: gpio
>>> [ ? ?0.171295] omap_gpio omap_gpio.2: context_loss_count=0
>>> [ ? ?0.171600] gpiochip_add: registered GPIOs 64 to 95 on device: gpio
>>> [ ? ?0.171936] omap_gpio omap_gpio.3: context_loss_count=0
>>> [ ? ?0.172241] gpiochip_add: registered GPIOs 96 to 127 on device: gpio
>>> [ ? ?0.172576] omap_gpio omap_gpio.4: context_loss_count=0
>>> [ ? ?0.172882] gpiochip_add: registered GPIOs 128 to 159 on device: gpio
>>> [ ? ?0.173217] omap_gpio omap_gpio.5: context_loss_count=0
>>> [ ? ?0.173522] gpiochip_add: registered GPIOs 160 to 191 on device: gpio
>>
>> That's consistent with what I see, and confirm that initialising the
>> context_lost_count to zero isn't always correct.
> I am just wondering if the context_lost_count = 1 for GPIO in WKUP domain
> is expected. In that case we have to add additional logic in runtime callbacks
> to skip context restore/save for WKUP domain GPIOs.
> But let's hear what Kevin says.

I think the original patch from Neil looks right.

Note that we would need something like this in the case where we built
the GPIO driver as a module and it was unloaded/reloaded where the
starting point of the context-loss count would not be zero.

Neil, care to send a patch w/changelog?

Thanks,

Kevin

[GIT PULL] gpio/omap: cleanups for v3.5

[Kevin Hilman]

On 07/02/2012 10:37 AM, Kevin Hilman wrote:
> "DebBarma, Tarun Kanti" <tarun.kanti@ti.com> writes:
>
>> On Mon, Jun 25, 2012 at 11:48 AM, NeilBrown <neilb@suse.de> wrote:
>>> On Thu, 21 Jun 2012 12:04:26 +0530 "DebBarma, Tarun Kanti"
>>> <tarun.kanti@ti.com> wrote:
>>>
>>>> On Thu, Jun 21, 2012 at 8:46 AM, NeilBrown <neilb@suse.de> wrote:
>>>>> On Thu, 14 Jun 2012 23:24:10 +0530 "DebBarma, Tarun Kanti"
>>>>> <tarun.kanti@ti.com> wrote:
>>>>>
>>>>>> On Thu, Jun 14, 2012 at 5:45 AM, NeilBrown <neilb@suse.de> wrote:
>>>>>>> On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <khilman@ti.com> wrote:
>>>>>>>
>>>>>>>> Hi Grant,
>>>>>>>>
>>>>>>>> Here's the final round of GPIO cleanups for v3.5.  This branch is based
>>>>>>>> on my for_3.5/fixes/gpio branch you just pulled.
>>>>>>>>
>>>>>>>> Kevin
>>>>>>>
>>>>>>> Hi.
>>>>>>>
>>>>>>>   I'm not sure if it was this series or the following cleanups which broke
>>>>>>>   things for me, but I've been trying 3.5-rc2 on my GTA04 and the serial
>>>>>>>   console (ttyO2) dies as soon as the omap-gpio driver initialises.
>>>>>>>
>>>>>>>   After some digging I came up with this patch to gpio-omap.c
>>>>>>>
>>>>>>> @@ -1124,6 +1124,9 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>>>>>>>
>>>>>>>         platform_set_drvdata(pdev, bank);
>>>>>>>
>>>>>>> +       if (bank->get_context_loss_count)
>>>>>>> +               bank->context_loss_count =
>>>>>>> +                               bank->get_context_loss_count(bank->dev);
>>>>>>>         pm_runtime_enable(bank->dev);
>>>>>>>         pm_runtime_irq_safe(bank->dev);
>>>>>>>         pm_runtime_get_sync(bank->dev);
>>>>>>>
>>>>>>> which fixes it.
>>>>>>>
>>>>>>> What was happening  was that when omap_gpio_probe calls pm_runtime_get_sync,
>>>>>>> it calls
>>>>>>>   _od_runtime_resume -> pm_generic_runtime_resume -> omap_gpio_runtime_resume
>>>>>>>   -> omap_gpio_restore_context
>>>>>>>
>>>>>>> and then the serial port stops.
>>>>>>> I reasoned that the context probably hadn't been set up yet, so restoring
>>>>>>> from it broke things.
>>>>>>> Initialising bank->context_loss_count seems sensible and would ensure that
>>>>>>> we didn't try to restore the context until it has actually been lost.
>>>>>>
>>>>>> I thought the following code exactly does that. That is context_lost_cnt_after
>>>>>> would be zero until there is context loss. The bank->context_loss_count is zero
>>>>>> at the beginning. So, (context_lost_cnt_after != bank->context_loss_count) would
>>>>>> be false and hence context restore should NOT happen? Not sure if I am
>>>>>> over looking
>>>>>> anything here....
>>>>>>
>>>>>> omap_gpio_runtime_resume(...)
>>>>>> {
>>>>>> ...
>>>>>>          if (bank->get_context_loss_count) {
>>>>>>                  context_lost_cnt_after =
>>>>>>                          bank->get_context_loss_count(bank->dev);
>>>>>>                  if (context_lost_cnt_after != bank->context_loss_count) {
>>>>>>                          omap_gpio_restore_context(bank);
>>>>>>                  } else {
>>>>>>                          spin_unlock_irqrestore(&bank->lock, flags);
>>>>>>                          return 0;
>>>>>>                  }
>>>>>>          }
>>>>>> ...
>>>>>> }
>>>>>
>>>>> Hi,
>>>>>   I've looked more closely at this now.
>>>>>
>>>>> The problem is that the initial context loss count is *not* zero.  Not always.
>>>>> The context loss count is the sum of
>>>>>
>>>>>         count = pwrdm->state_counter[PWRDM_POWER_OFF];
>>>>>         count += pwrdm->ret_logic_off_counter;
>>>>>
>>>>>         for (i = 0; i < pwrdm->banks; i++)
>>>>>                 count += pwrdm->ret_mem_off_counter[i];
>>>>>
>>>>> (from  pwrdm_get_context_loss_count()).
>>>>>
>>>>> These are initlialised in _pwrdm_register
>>>>>
>>>>>         /* Initialize the powerdomain's state counter */
>>>>>         for (i = 0; i < PWRDM_MAX_PWRSTS; i++)
>>>>>                 pwrdm->state_counter[i] = 0;
>>>>>
>>>>>         pwrdm->ret_logic_off_counter = 0;
>>>>>         for (i = 0; i < pwrdm->banks; i++)
>>>>>                 pwrdm->ret_mem_off_counter[i] = 0;
>>>>>
>>>>>         pwrdm_wait_transition(pwrdm);
>>>>>         pwrdm->state = pwrdm_read_pwrst(pwrdm);
>>>>>         pwrdm->state_counter[pwrdm->state] = 1;
>>>>>
>>>>>
>>>>> What I'm seeing is that for wkup_pwrdm and dpll{3,4,5}_pwrdm,
>>>>> the state that pwrdm_read_pwrst returns is PWRDM_POWER_OFF.
>>>>> So that state_counter gets initialised to '1', and so the initial
>>>>> context_loss_count, which includes that counter, is also '1'.
>>>>> I think it is the wkup_pwrdm that covers the GPIOs that are causing problems
>>>>> for me.
>>>> I just put a log in omap_gpio_probe() to see the value of context_loss_count.
>>>> GPIO Bank 0 (WKUP Domain) always shows the count as '1'.
>>>>
>>>> [    0.169494] omap_gpio omap_gpio.0: context_loss_count=1
>>>> [    0.170227] gpiochip_add: registered GPIOs 0 to 31 on device: gpio
>>>> [    0.170471] OMAP GPIO hardware version 0.1
>>>> [    0.170623] omap_gpio omap_gpio.1: context_loss_count=0
>>>> [    0.170928] gpiochip_add: registered GPIOs 32 to 63 on device: gpio
>>>> [    0.171295] omap_gpio omap_gpio.2: context_loss_count=0
>>>> [    0.171600] gpiochip_add: registered GPIOs 64 to 95 on device: gpio
>>>> [    0.171936] omap_gpio omap_gpio.3: context_loss_count=0
>>>> [    0.172241] gpiochip_add: registered GPIOs 96 to 127 on device: gpio
>>>> [    0.172576] omap_gpio omap_gpio.4: context_loss_count=0
>>>> [    0.172882] gpiochip_add: registered GPIOs 128 to 159 on device: gpio
>>>> [    0.173217] omap_gpio omap_gpio.5: context_loss_count=0
>>>> [    0.173522] gpiochip_add: registered GPIOs 160 to 191 on device: gpio
>>>
>>> That's consistent with what I see, and confirm that initialising the
>>> context_lost_count to zero isn't always correct.
>> I am just wondering if the context_lost_count = 1 for GPIO in WKUP domain
>> is expected. In that case we have to add additional logic in runtime callbacks
>> to skip context restore/save for WKUP domain GPIOs.
>> But let's hear what Kevin says.
>
> I think the original patch from Neil looks right.
>
> Note that we would need something like this in the case where we built
> the GPIO driver as a module and it was unloaded/reloaded where the
> starting point of the context-loss count would not be zero.
>
> Neil, care to send a patch w/changelog?
>

Nevermind,  this same issue has come up in another thread with a patch 
proposed.

Kevin