| Message ID | 1366230707-32681-1-git-send-email-jon-hunter@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 18 April 2013 02:01 AM, Jon Hunter wrote: > Commit a2797be (gpio/omap: force restore if context loss is not > detectable) broke gpio support for OMAP when booting with device-tree > because a restore of the gpio context being performed without ever > initialising the gpio context. In other words, the context restored was > bad. > > This problem could also occur in the non device-tree case, however, it > is much less likely because when booting without device-tree we can > detect context loss via a platform specific API and so context restore > is performed less often. > > Nevertheless we should ensure that the gpio context is initialised > during the probe for gpio banks that could lose their state regardless > of whether we are booting with device-tree or not. > > Reported-by: Tony Lindgren <tony@atomide.com> > Signed-off-by: Jon Hunter <jon-hunter@ti.com> > Tested-by: Tony Lindgren <tony@atomide.com> > --- > drivers/gpio/gpio-omap.c | 53 +++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 43 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 0557529..0ba5cb9 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -70,6 +70,7 @@ struct gpio_bank { > bool is_mpuio; > bool dbck_flag; > bool loses_context; > + bool context_valid; > int stride; > u32 width; > int context_loss_count; > @@ -1085,6 +1086,7 @@ static void omap_gpio_chip_init(struct gpio_bank *bank) > } > > static const struct of_device_id omap_gpio_match[]; > +static void omap_gpio_init_context(struct gpio_bank *p); > > static int omap_gpio_probe(struct platform_device *pdev) > { > @@ -1179,8 +1181,10 @@ static int omap_gpio_probe(struct platform_device *pdev) > omap_gpio_chip_init(bank); > omap_gpio_show_rev(bank); > > - if (bank->loses_context) > + if (bank->loses_context) { > bank->get_context_loss_count = pdata->get_context_loss_count; > + omap_gpio_init_context(bank); > + } > > pm_runtime_put(bank->dev); > > @@ -1269,6 +1273,14 @@ static int omap_gpio_runtime_resume(struct device *dev) > int c; > > spin_lock_irqsave(&bank->lock, flags); > + > + /* > + * On the first resume during the probe, the context has not > + * been initialised and so if the context is not valid return. > + */ > + if (!bank->context_valid) > + goto done; > + The use of 'context_valid' is just for the probe resume case and hence adding a state variable for GPIO bank structures looks bit too much. Ideally gpio_init_context() should make the first callback successful as well if ordered correctly but then you need clocks for that to happen which are enabled in runtime pm calls. Do you really need that "context_valid" flag per bank ? I mean every gpio bank which can "loses_context", probe time context validity is an issue, right ? In that case, won't just an __init static variable do for all banks which can loose context? Regards, Santosh -- 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 04/18/2013 03:22 AM, Santosh Shilimkar wrote: > On Thursday 18 April 2013 02:01 AM, Jon Hunter wrote: >> Commit a2797be (gpio/omap: force restore if context loss is not >> detectable) broke gpio support for OMAP when booting with device-tree >> because a restore of the gpio context being performed without ever >> initialising the gpio context. In other words, the context restored was >> bad. >> >> This problem could also occur in the non device-tree case, however, it >> is much less likely because when booting without device-tree we can >> detect context loss via a platform specific API and so context restore >> is performed less often. >> >> Nevertheless we should ensure that the gpio context is initialised >> during the probe for gpio banks that could lose their state regardless >> of whether we are booting with device-tree or not. >> >> Reported-by: Tony Lindgren <tony@atomide.com> >> Signed-off-by: Jon Hunter <jon-hunter@ti.com> >> Tested-by: Tony Lindgren <tony@atomide.com> >> --- >> drivers/gpio/gpio-omap.c | 53 +++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 43 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >> index 0557529..0ba5cb9 100644 >> --- a/drivers/gpio/gpio-omap.c >> +++ b/drivers/gpio/gpio-omap.c >> @@ -70,6 +70,7 @@ struct gpio_bank { >> bool is_mpuio; >> bool dbck_flag; >> bool loses_context; >> + bool context_valid; >> int stride; >> u32 width; >> int context_loss_count; >> @@ -1085,6 +1086,7 @@ static void omap_gpio_chip_init(struct gpio_bank *bank) >> } >> >> static const struct of_device_id omap_gpio_match[]; >> +static void omap_gpio_init_context(struct gpio_bank *p); >> >> static int omap_gpio_probe(struct platform_device *pdev) >> { >> @@ -1179,8 +1181,10 @@ static int omap_gpio_probe(struct platform_device *pdev) >> omap_gpio_chip_init(bank); >> omap_gpio_show_rev(bank); >> >> - if (bank->loses_context) >> + if (bank->loses_context) { >> bank->get_context_loss_count = pdata->get_context_loss_count; >> + omap_gpio_init_context(bank); >> + } >> >> pm_runtime_put(bank->dev); >> >> @@ -1269,6 +1273,14 @@ static int omap_gpio_runtime_resume(struct device *dev) >> int c; >> >> spin_lock_irqsave(&bank->lock, flags); >> + >> + /* >> + * On the first resume during the probe, the context has not >> + * been initialised and so if the context is not valid return. >> + */ >> + if (!bank->context_valid) >> + goto done; >> + > The use of 'context_valid' is just for the probe resume case and hence > adding a state variable for GPIO bank structures looks bit too much. Yes it is not ideal. However, it is safe. > Ideally gpio_init_context() should make the first callback successful > as well if ordered correctly but then you need clocks for that > to happen which are enabled in runtime pm calls. Right. > Do you really need that "context_valid" flag per bank ? I mean > every gpio bank which can "loses_context", probe time context > validity is an issue, right ? In that case, won't just an > __init static variable do for all banks which can loose > context? I think that it would be fragile to have a global for all banks. What happens if one bank failed during the probe? Probably unlikely but still. The best solution would be to be able to plug the pm-runtime resume/suspend handlers at the end of the probe or be able to enable the module without the callback handlers being calling during the probe. I am not sure if there is a way to do this. May be Kevin knows. Another option is to move the initialisation of the bank->loses_context to after the call to pm_runtime_get_sync() in the probe. This would prevent the context restore occurring during the initial runtime resume. However, if you look at the runtime resume function there are still some register writes that do occur (which is basically what happens today). This works but it is a little fragile, however, we can do that if preferred. May be I could add a comment to the probe to say why we initialise bank->loses_context after the pm_runtime_get_sync() so no one moves this in the future (and more importantly I don't forget!). Cheers Jon -- 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
Hi Jon, Jon Hunter <jon-hunter@ti.com> writes: > Commit a2797be (gpio/omap: force restore if context loss is not > detectable) broke gpio support for OMAP when booting with device-tree > because a restore of the gpio context being performed without ever > initialising the gpio context. In other words, the context restored was > bad. > > This problem could also occur in the non device-tree case, however, it > is much less likely because when booting without device-tree we can > detect context loss via a platform specific API and so context restore > is performed less often. > > Nevertheless we should ensure that the gpio context is initialised > during the probe for gpio banks that could lose their state regardless > of whether we are booting with device-tree or not. > > Reported-by: Tony Lindgren <tony@atomide.com> > Signed-off-by: Jon Hunter <jon-hunter@ti.com> > Tested-by: Tony Lindgren <tony@atomide.com> > --- > drivers/gpio/gpio-omap.c | 53 +++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 43 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 0557529..0ba5cb9 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -70,6 +70,7 @@ struct gpio_bank { > bool is_mpuio; > bool dbck_flag; > bool loses_context; > + bool context_valid; > int stride; > u32 width; > int context_loss_count; > @@ -1085,6 +1086,7 @@ static void omap_gpio_chip_init(struct gpio_bank *bank) > } > > static const struct of_device_id omap_gpio_match[]; > +static void omap_gpio_init_context(struct gpio_bank *p); > > static int omap_gpio_probe(struct platform_device *pdev) > { > @@ -1179,8 +1181,10 @@ static int omap_gpio_probe(struct platform_device *pdev) > omap_gpio_chip_init(bank); > omap_gpio_show_rev(bank); > > - if (bank->loses_context) > + if (bank->loses_context) { > bank->get_context_loss_count = pdata->get_context_loss_count; > + omap_gpio_init_context(bank); > + } > > pm_runtime_put(bank->dev); > > @@ -1269,6 +1273,14 @@ static int omap_gpio_runtime_resume(struct device *dev) > int c; > > spin_lock_irqsave(&bank->lock, flags); > + > + /* > + * On the first resume during the probe, the context has not > + * been initialised and so if the context is not valid return. > + */ > + if (!bank->context_valid) > + goto done; Not sure I follow the reason to separate it here and in probe. Also, this makes the first runtime_resume a special case and leaves things in a strange semi-initialized state that is confusing IMO. Why not just init context right here if bank->loses_context && !bank->context_valid? Then the first resume can continue as expected, and everything is fully initialized as expected also. IMO, this is much more readable (and maintainable, but that's your job now, so you can decide ;) Kevin -- 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
Hi Kevin, On 04/18/2013 04:34 PM, Kevin Hilman wrote: > Hi Jon, > > Jon Hunter <jon-hunter@ti.com> writes: > >> Commit a2797be (gpio/omap: force restore if context loss is not >> detectable) broke gpio support for OMAP when booting with device-tree >> because a restore of the gpio context being performed without ever >> initialising the gpio context. In other words, the context restored was >> bad. >> >> This problem could also occur in the non device-tree case, however, it >> is much less likely because when booting without device-tree we can >> detect context loss via a platform specific API and so context restore >> is performed less often. >> >> Nevertheless we should ensure that the gpio context is initialised >> during the probe for gpio banks that could lose their state regardless >> of whether we are booting with device-tree or not. >> >> Reported-by: Tony Lindgren <tony@atomide.com> >> Signed-off-by: Jon Hunter <jon-hunter@ti.com> >> Tested-by: Tony Lindgren <tony@atomide.com> >> --- >> drivers/gpio/gpio-omap.c | 53 +++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 43 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >> index 0557529..0ba5cb9 100644 >> --- a/drivers/gpio/gpio-omap.c >> +++ b/drivers/gpio/gpio-omap.c >> @@ -70,6 +70,7 @@ struct gpio_bank { >> bool is_mpuio; >> bool dbck_flag; >> bool loses_context; >> + bool context_valid; >> int stride; >> u32 width; >> int context_loss_count; >> @@ -1085,6 +1086,7 @@ static void omap_gpio_chip_init(struct gpio_bank *bank) >> } >> >> static const struct of_device_id omap_gpio_match[]; >> +static void omap_gpio_init_context(struct gpio_bank *p); >> >> static int omap_gpio_probe(struct platform_device *pdev) >> { >> @@ -1179,8 +1181,10 @@ static int omap_gpio_probe(struct platform_device *pdev) >> omap_gpio_chip_init(bank); >> omap_gpio_show_rev(bank); >> >> - if (bank->loses_context) >> + if (bank->loses_context) { >> bank->get_context_loss_count = pdata->get_context_loss_count; >> + omap_gpio_init_context(bank); >> + } >> >> pm_runtime_put(bank->dev); >> >> @@ -1269,6 +1273,14 @@ static int omap_gpio_runtime_resume(struct device *dev) >> int c; >> >> spin_lock_irqsave(&bank->lock, flags); >> + >> + /* >> + * On the first resume during the probe, the context has not >> + * been initialised and so if the context is not valid return. >> + */ >> + if (!bank->context_valid) >> + goto done; > > Not sure I follow the reason to separate it here and in probe. > > Also, this makes the first runtime_resume a special case and leaves > things in a strange semi-initialized state that is confusing IMO. The first resume has always been a special case. The "bank->get_context_loss_count" is not initialised until after the first resume (due to another issue we had found - 7b86cef gpio/omap: fix invalid context restore of gpio bank-0). This should not leave things in a strange semi-init'ed state, as on the first resume nothing is really done anyway because there is no context loss. > Why not just init context right here if bank->loses_context && > !bank->context_valid? Thanks for the suggestion. > Then the first resume can continue as expected, and everything is fully > initialized as expected also. IMO, this is much more readable (and > maintainable, but that's your job now, so you can decide ;) If the context has not been lost, which it has not on the first resume, then resume really does nothing. That's why I had just returned. However, I would agree that is not completely readable. Cheers Jon -- 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
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index 0557529..0ba5cb9 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -70,6 +70,7 @@ struct gpio_bank { bool is_mpuio; bool dbck_flag; bool loses_context; + bool context_valid; int stride; u32 width; int context_loss_count; @@ -1085,6 +1086,7 @@ static void omap_gpio_chip_init(struct gpio_bank *bank) } static const struct of_device_id omap_gpio_match[]; +static void omap_gpio_init_context(struct gpio_bank *p); static int omap_gpio_probe(struct platform_device *pdev) { @@ -1179,8 +1181,10 @@ static int omap_gpio_probe(struct platform_device *pdev) omap_gpio_chip_init(bank); omap_gpio_show_rev(bank); - if (bank->loses_context) + if (bank->loses_context) { bank->get_context_loss_count = pdata->get_context_loss_count; + omap_gpio_init_context(bank); + } pm_runtime_put(bank->dev); @@ -1269,6 +1273,14 @@ static int omap_gpio_runtime_resume(struct device *dev) int c; spin_lock_irqsave(&bank->lock, flags); + + /* + * On the first resume during the probe, the context has not + * been initialised and so if the context is not valid return. + */ + if (!bank->context_valid) + goto done; + _gpio_dbck_enable(bank); /* @@ -1287,19 +1299,15 @@ static int omap_gpio_runtime_resume(struct device *dev) omap_gpio_restore_context(bank); } else { c = bank->get_context_loss_count(bank->dev); - if (c != bank->context_loss_count) { + if (c != bank->context_loss_count) omap_gpio_restore_context(bank); - } else { - spin_unlock_irqrestore(&bank->lock, flags); - return 0; - } + else + goto done; } } - if (!bank->workaround_enabled) { - spin_unlock_irqrestore(&bank->lock, flags); - return 0; - } + if (!bank->workaround_enabled) + goto done; l = __raw_readl(bank->base + bank->regs->datain); @@ -1352,6 +1360,7 @@ static int omap_gpio_runtime_resume(struct device *dev) } bank->workaround_enabled = false; +done: spin_unlock_irqrestore(&bank->lock, flags); return 0; @@ -1385,6 +1394,29 @@ void omap2_gpio_resume_after_idle(void) } #if defined(CONFIG_PM_RUNTIME) +static void omap_gpio_init_context(struct gpio_bank *p) +{ + struct omap_gpio_reg_offs *regs = p->regs; + void __iomem *base = p->base; + + p->context.ctrl = __raw_readl(base + regs->ctrl); + p->context.oe = __raw_readl(base + regs->direction); + p->context.wake_en = __raw_readl(base + regs->wkup_en); + p->context.leveldetect0 = __raw_readl(base + regs->leveldetect0); + p->context.leveldetect1 = __raw_readl(base + regs->leveldetect1); + p->context.risingdetect = __raw_readl(base + regs->risingdetect); + p->context.fallingdetect = __raw_readl(base + regs->fallingdetect); + p->context.irqenable1 = __raw_readl(base + regs->irqenable); + p->context.irqenable2 = __raw_readl(base + regs->irqenable2); + + if (regs->set_dataout && p->regs->clr_dataout) + p->context.dataout = __raw_readl(base + regs->set_dataout); + else + p->context.dataout = __raw_readl(base + regs->dataout); + + p->context_valid = true; +} + static void omap_gpio_restore_context(struct gpio_bank *bank) { __raw_writel(bank->context.wake_en, @@ -1422,6 +1454,7 @@ static void omap_gpio_restore_context(struct gpio_bank *bank) #else #define omap_gpio_runtime_suspend NULL #define omap_gpio_runtime_resume NULL +static void omap_gpio_init_context(struct gpio_bank *p) {} #endif static const struct dev_pm_ops gpio_pm_ops = {