| Message ID | 874nrmtf47.fsf@ti.com |
|---|---|
| State | New |
| Headers | show |
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(-)
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
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
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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >
"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
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
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(-)