gpio/omap: cleanups for v3.5

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(-)

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

Quoted 6 lines · On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <[email protected]> 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

(quoted diff) +3 -0

@@ -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

Quoted 35 lines · NeilBrown

Quoted 6 lines · On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <[email protected]> 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

(quoted diff) +3 -0

@@ -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

Quoted 5 lines

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

Quoted 1 line

Quoted 2 lines

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

Quoted 35 lines · NeilBrown

Quoted 6 lines · On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <[email protected]> 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

(quoted diff) +3 -0

@@ -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

Quoted 5 lines

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;
                }
        }

...
}

Quoted 49 lines · Tarun Kanti

Quoted 35 lines · NeilBrown

Quoted 6 lines · On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <[email protected]> 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

(quoted diff) +3 -0

@@ -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

Quoted 5 lines

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....

Quoted 15 lines

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

Quoted 97 lines · NeilBrown

Quoted 49 lines · Tarun Kanti

Quoted 35 lines · NeilBrown

Quoted 6 lines · On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <[email protected]> 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

(quoted diff) +3 -0

@@ -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

Quoted 5 lines

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....

Quoted 15 lines

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;

Quoted 6 lines

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

Quoted 5 lines

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.

Quoted 1 line

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

Quoted 107 lines · Tarun Kanti

Quoted 97 lines · NeilBrown

Quoted 49 lines · Tarun Kanti

Quoted 35 lines · NeilBrown

Quoted 6 lines · On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <[email protected]> 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

(quoted diff) +3 -0

@@ -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

Quoted 5 lines

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....

Quoted 15 lines

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;

Quoted 6 lines

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'.

Quoted 13 lines

[ ? ?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

Quoted 12 lines

--
Tarun

Quoted 6 lines

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

Quoted 124 lines · NeilBrown

Quoted 120 lines · Tarun Kanti

Quoted 97 lines · NeilBrown

Quoted 49 lines · Tarun Kanti

Quoted 35 lines · NeilBrown

Quoted 6 lines · On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <[email protected]> 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

(quoted diff) +3 -0

@@ -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

Quoted 5 lines

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....

Quoted 15 lines

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;

Quoted 6 lines

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

Quoted 2 lines

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.

Quoted 125 lines · Tarun Kanti

Quoted 124 lines · NeilBrown

Quoted 120 lines · Tarun Kanti

Quoted 97 lines · NeilBrown

Quoted 62 lines · Tarun Kanti

Quoted 35 lines · NeilBrown

Quoted 6 lines · On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <[email protected]> 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

(quoted diff) +3 -0

@@ -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

Quoted 5 lines

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;
                 }
         }

Quoted 2 lines

...
}

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;

Quoted 6 lines

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

Quoted 6 lines

Quoted 2 lines

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?