| Message ID | 1411079579-10172-1-git-send-email-joshc@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sep 18, 2014, at 3:32 PM, Josh Cartwright <joshc@codeaurora.org> wrote: > By converting to the restart_notifier mechanism for restart, we allow > for other mechanisms, like the watchdog, to be used for restart in the > case where PS_HOLD has failed to reset the chip. > > Choose priority 128, as according to documentation, this mechanism "is > sufficient to restart the entire system”. Will we use a higher priority for watchdog? or how would we fail over to watchdog? > > Cc: Pramod Gurav <pramod.gurav@smartplayin.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Josh Cartwright <joshc@codeaurora.org> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 38 ++++++++++++++++++++++++++------------ > 1 file changed, 26 insertions(+), 12 deletions(-) > - k
On Thu, Sep 18, 2014 at 03:47:20PM -0700, Kumar Gala wrote: > On Sep 18, 2014, at 3:32 PM, Josh Cartwright <joshc@codeaurora.org> wrote: > > > By converting to the restart_notifier mechanism for restart, we allow > > for other mechanisms, like the watchdog, to be used for restart in the > > case where PS_HOLD has failed to reset the chip. > > > > Choose priority 128, as according to documentation, this mechanism "is > > sufficient to restart the entire system?. > > Will we use a higher priority for watchdog? or how would we fail over to watchdog? The registered restart handlers are called in (descending) priority order. This driver registers as 128, but conceivably there could be some super-board-specific restart mechanism that can register itself as higher priority than this and it will be attempted first. If PS_HOLD doesn't work, the lower priority restart handlers will continue to be called, including the watchdog, which is registering with the lowest priority (0).
On Sep 18, 2014, at 3:49 PM, Josh Cartwright <joshc@codeaurora.org> wrote: > On Thu, Sep 18, 2014 at 03:47:20PM -0700, Kumar Gala wrote: >> On Sep 18, 2014, at 3:32 PM, Josh Cartwright <joshc@codeaurora.org> wrote: >> >>> By converting to the restart_notifier mechanism for restart, we allow >>> for other mechanisms, like the watchdog, to be used for restart in the >>> case where PS_HOLD has failed to reset the chip. >>> >>> Choose priority 128, as according to documentation, this mechanism "is >>> sufficient to restart the entire system?. >> >> Will we use a higher priority for watchdog? or how would we fail over to watchdog? > > The registered restart handlers are called in (descending) priority > order. This driver registers as 128, but conceivably there could be > some super-board-specific restart mechanism that can register itself as > higher priority than this and it will be attempted first. > > If PS_HOLD doesn't work, the lower priority restart handlers will > continue to be called, including the watchdog, which is registering with > the lowest priority (0). Cool, thanks. - k
On 09/18/2014 03:32 PM, Josh Cartwright wrote: > By converting to the restart_notifier mechanism for restart, we allow > for other mechanisms, like the watchdog, to be used for restart in the > case where PS_HOLD has failed to reset the chip. > > Choose priority 128, as according to documentation, this mechanism "is > sufficient to restart the entire system". > > Cc: Pramod Gurav <pramod.gurav@smartplayin.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Josh Cartwright <joshc@codeaurora.org> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 38 ++++++++++++++++++++++++++------------ > 1 file changed, 26 insertions(+), 12 deletions(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index d5ed127..9fced3b 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -27,8 +27,7 @@ > #include <linux/gpio.h> > #include <linux/interrupt.h> > #include <linux/spinlock.h> > - > -#include <asm/system_misc.h> > +#include <linux/reboot.h> > > #include "../core.h" > #include "../pinconf.h" > @@ -43,6 +42,7 @@ > * @dev: device handle. > * @pctrl: pinctrl handle. > * @chip: gpiochip handle. > + * @restart_nb: restart notifier block. > * @irq: parent irq for the TLMM irq_chip. > * @lock: Spinlock to protect register resources as well > * as msm_pinctrl data structures. > @@ -56,6 +56,7 @@ struct msm_pinctrl { > struct device *dev; > struct pinctrl_dev *pctrl; > struct gpio_chip chip; > + struct notifier_block restart_nb; > int irq; > > spinlock_t lock; > @@ -852,13 +853,14 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > return 0; > } > > -#ifdef CONFIG_ARM Unrelated to this patch, but is this going to be executed (and potentially used) on any non-arm architectures ? Reason for asking that the restart handler mechanism is currently only implemented on arm and arm64. If it is going to be used with other architectures, we'll have to add the necessary call into the architecture restart code. > -static void __iomem *msm_ps_hold; > - > -static void msm_reset(enum reboot_mode reboot_mode, const char *cmd) > +static int msm_ps_hold_restart(struct notifier_block *nb, unsigned long action, > + void *data) > { > - writel(0, msm_ps_hold); > + struct msm_pinctrl *pctrl = container_of(nb, struct msm_pinctrl, restart_nb); > + > + writel(0, pctrl->regs + PS_HOLD_OFFSET); > mdelay(10000); Sure you still want to wait for 10 seconds here ? Would it make sense to choose a more reasonable timeout ? Thanks, Guenter > + return NOTIFY_DONE; > } > > static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) > @@ -868,13 +870,16 @@ static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) > > for (; i <= pctrl->soc->nfunctions; i++) > if (!strcmp(func[i].name, "ps_hold")) { > - msm_ps_hold = pctrl->regs + PS_HOLD_OFFSET; > - arm_pm_restart = msm_reset; > + pctrl->restart_nb.notifier_call = msm_ps_hold_restart; > + pctrl->restart_nb.priority = 128; > + if (register_restart_handler(&pctrl->restart_nb)) { > + dev_err(pctrl->dev, > + "failed to setup restart handler.\n"); > + pctrl->restart_nb.notifier_call = NULL; > + } > + break; > } > } > -#else > -static void msm_pinctrl_setup_pm_reset(const struct msm_pinctrl *pctrl) {} > -#endif > > int msm_pinctrl_probe(struct platform_device *pdev, > const struct msm_pinctrl_soc_data *soc_data) > @@ -943,6 +948,15 @@ int msm_pinctrl_remove(struct platform_device *pdev) > > pinctrl_unregister(pctrl->pctrl); > > + if (pctrl->restart_nb.notifier_call) { > + ret = unregister_restart_handler(&pctrl->restart_nb); > + if (ret) { > + dev_err(&pdev->dev, > + "unable to unregister restart handler\n"); > + return ret; > + } > + } > + > return 0; > } > EXPORT_SYMBOL(msm_pinctrl_remove); >
On 09/18/2014 07:54 PM, Guenter Roeck wrote: > On 09/18/2014 03:32 PM, Josh Cartwright wrote: >> By converting to the restart_notifier mechanism for restart, we allow >> for other mechanisms, like the watchdog, to be used for restart in the >> case where PS_HOLD has failed to reset the chip. >> >> Choose priority 128, as according to documentation, this mechanism "is >> sufficient to restart the entire system". >> >> Cc: Pramod Gurav <pramod.gurav@smartplayin.com> >> Cc: Guenter Roeck <linux@roeck-us.net> >> Signed-off-by: Josh Cartwright <joshc@codeaurora.org> >> --- >> drivers/pinctrl/qcom/pinctrl-msm.c | 38 ++++++++++++++++++++++++++------------ >> 1 file changed, 26 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >> index d5ed127..9fced3b 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-msm.c >> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c >> @@ -27,8 +27,7 @@ >> #include <linux/gpio.h> >> #include <linux/interrupt.h> >> #include <linux/spinlock.h> >> - >> -#include <asm/system_misc.h> >> +#include <linux/reboot.h> >> >> #include "../core.h" >> #include "../pinconf.h" >> @@ -43,6 +42,7 @@ >> * @dev: device handle. >> * @pctrl: pinctrl handle. >> * @chip: gpiochip handle. >> + * @restart_nb: restart notifier block. >> * @irq: parent irq for the TLMM irq_chip. >> * @lock: Spinlock to protect register resources as well >> * as msm_pinctrl data structures. >> @@ -56,6 +56,7 @@ struct msm_pinctrl { >> struct device *dev; >> struct pinctrl_dev *pctrl; >> struct gpio_chip chip; >> + struct notifier_block restart_nb; >> int irq; >> >> spinlock_t lock; >> @@ -852,13 +853,14 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) >> return 0; >> } >> >> -#ifdef CONFIG_ARM > > Unrelated to this patch, but is this going to be executed (and potentially used) > on any non-arm architectures ? > > Reason for asking that the restart handler mechanism is currently only > implemented on arm and arm64. If it is going to be used with other > architectures, we'll have to add the necessary call into the architecture > restart code. > >> -static void __iomem *msm_ps_hold; >> - >> -static void msm_reset(enum reboot_mode reboot_mode, const char *cmd) >> +static int msm_ps_hold_restart(struct notifier_block *nb, unsigned long action, >> + void *data) >> { >> - writel(0, msm_ps_hold); >> + struct msm_pinctrl *pctrl = container_of(nb, struct msm_pinctrl, restart_nb); >> + >> + writel(0, pctrl->regs + PS_HOLD_OFFSET); >> mdelay(10000); > > Sure you still want to wait for 10 seconds here ? > Would it make sense to choose a more reasonable timeout ? > > Thanks, > Guenter > >> + return NOTIFY_DONE; >> } >> >> static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) >> @@ -868,13 +870,16 @@ static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) >> >> for (; i <= pctrl->soc->nfunctions; i++) >> if (!strcmp(func[i].name, "ps_hold")) { >> - msm_ps_hold = pctrl->regs + PS_HOLD_OFFSET; >> - arm_pm_restart = msm_reset; >> + pctrl->restart_nb.notifier_call = msm_ps_hold_restart; >> + pctrl->restart_nb.priority = 128; >> + if (register_restart_handler(&pctrl->restart_nb)) { >> + dev_err(pctrl->dev, >> + "failed to setup restart handler.\n"); >> + pctrl->restart_nb.notifier_call = NULL; >> + } >> + break; >> } >> } >> -#else >> -static void msm_pinctrl_setup_pm_reset(const struct msm_pinctrl *pctrl) {} >> -#endif >> >> int msm_pinctrl_probe(struct platform_device *pdev, >> const struct msm_pinctrl_soc_data *soc_data) >> @@ -943,6 +948,15 @@ int msm_pinctrl_remove(struct platform_device *pdev) >> >> pinctrl_unregister(pctrl->pctrl); >> >> + if (pctrl->restart_nb.notifier_call) { One more comment: The conditional is really unnecessary. Just let unregister_restart_handler deal with it ... >> + ret = unregister_restart_handler(&pctrl->restart_nb); and just ignore the error return. The function will only return an error if the entry was not found, and then it is a don't care. Thanks, Guenter >> + if (ret) { >> + dev_err(&pdev->dev, >> + "unable to unregister restart handler\n"); >> + return ret; >> + } >> + } >> + >> return 0; >> } >> EXPORT_SYMBOL(msm_pinctrl_remove); >> >
Hi Josh, Tested this on IFC6410. Tested-by: pramod.gurav@smartplayin.com On Friday 19 September 2014 04:02 AM, Josh Cartwright wrote: > By converting to the restart_notifier mechanism for restart, we allow > for other mechanisms, like the watchdog, to be used for restart in the > case where PS_HOLD has failed to reset the chip. > > Choose priority 128, as according to documentation, this mechanism "is > sufficient to restart the entire system". > > Cc: Pramod Gurav <pramod.gurav@smartplayin.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Josh Cartwright <joshc@codeaurora.org> > ---
On Thu, Sep 18, 2014 at 07:54:18PM -0700, Guenter Roeck wrote: > On 09/18/2014 03:32 PM, Josh Cartwright wrote: > >By converting to the restart_notifier mechanism for restart, we allow > >for other mechanisms, like the watchdog, to be used for restart in the > >case where PS_HOLD has failed to reset the chip. > > > >Choose priority 128, as according to documentation, this mechanism "is > >sufficient to restart the entire system". > > > >Cc: Pramod Gurav <pramod.gurav@smartplayin.com> > >Cc: Guenter Roeck <linux@roeck-us.net> > >Signed-off-by: Josh Cartwright <joshc@codeaurora.org> > >--- [..] > > > >-#ifdef CONFIG_ARM > > Unrelated to this patch, but is this going to be executed (and potentially used) > on any non-arm architectures ? No. Not for now. The only other conceivable platform that could make use of this would be hexagon, but I think there would be other hurdles to getting this driver working before even tackling the restart handler. [..] > >+static int msm_ps_hold_restart(struct notifier_block *nb, unsigned long action, > >+ void *data) > > { > >- writel(0, msm_ps_hold); > >+ struct msm_pinctrl *pctrl = container_of(nb, struct msm_pinctrl, restart_nb); > >+ > >+ writel(0, pctrl->regs + PS_HOLD_OFFSET); > > mdelay(10000); > > Sure you still want to wait for 10 seconds here ? > Would it make sense to choose a more reasonable timeout ? Yeah, 10s is a bit extreme. I don't quite know what a reasonable timeout is, so I'll do some rough measurements, and shorten it down in v2. Thanks again, Josh
On Thu, Sep 18, 2014 at 08:34:41PM -0700, Guenter Roeck wrote: > On 09/18/2014 07:54 PM, Guenter Roeck wrote: > >On 09/18/2014 03:32 PM, Josh Cartwright wrote: > >>By converting to the restart_notifier mechanism for restart, we allow > >>for other mechanisms, like the watchdog, to be used for restart in the > >>case where PS_HOLD has failed to reset the chip. > >> > >>Choose priority 128, as according to documentation, this mechanism "is > >>sufficient to restart the entire system". > >> > >>Cc: Pramod Gurav <pramod.gurav@smartplayin.com> > >>Cc: Guenter Roeck <linux@roeck-us.net> > >>Signed-off-by: Josh Cartwright <joshc@codeaurora.org> > >>--- [..] > >> int msm_pinctrl_probe(struct platform_device *pdev, > >> const struct msm_pinctrl_soc_data *soc_data) > >>@@ -943,6 +948,15 @@ int msm_pinctrl_remove(struct platform_device *pdev) > >> > >> pinctrl_unregister(pctrl->pctrl); > >> > >>+ if (pctrl->restart_nb.notifier_call) { > > One more comment: The conditional is really unnecessary. Just let > unregister_restart_handler deal with it ... > > >>+ ret = unregister_restart_handler(&pctrl->restart_nb); > > and just ignore the error return. The function will only return an error > if the entry was not found, and then it is a don't care. Awesome, thanks. I like simplifications :). I was hijacking notifier_call to indicate whether or not the restart notifier was registered at all (because it's conditional on the particular chipset having a "ps_hold" function). But, nice to know unregister_restart_handler() does the right thing if the handler wasn't registered in the first place. Josh
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index d5ed127..9fced3b 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -27,8 +27,7 @@ #include <linux/gpio.h> #include <linux/interrupt.h> #include <linux/spinlock.h> - -#include <asm/system_misc.h> +#include <linux/reboot.h> #include "../core.h" #include "../pinconf.h" @@ -43,6 +42,7 @@ * @dev: device handle. * @pctrl: pinctrl handle. * @chip: gpiochip handle. + * @restart_nb: restart notifier block. * @irq: parent irq for the TLMM irq_chip. * @lock: Spinlock to protect register resources as well * as msm_pinctrl data structures. @@ -56,6 +56,7 @@ struct msm_pinctrl { struct device *dev; struct pinctrl_dev *pctrl; struct gpio_chip chip; + struct notifier_block restart_nb; int irq; spinlock_t lock; @@ -852,13 +853,14 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) return 0; } -#ifdef CONFIG_ARM -static void __iomem *msm_ps_hold; - -static void msm_reset(enum reboot_mode reboot_mode, const char *cmd) +static int msm_ps_hold_restart(struct notifier_block *nb, unsigned long action, + void *data) { - writel(0, msm_ps_hold); + struct msm_pinctrl *pctrl = container_of(nb, struct msm_pinctrl, restart_nb); + + writel(0, pctrl->regs + PS_HOLD_OFFSET); mdelay(10000); + return NOTIFY_DONE; } static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) @@ -868,13 +870,16 @@ static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) for (; i <= pctrl->soc->nfunctions; i++) if (!strcmp(func[i].name, "ps_hold")) { - msm_ps_hold = pctrl->regs + PS_HOLD_OFFSET; - arm_pm_restart = msm_reset; + pctrl->restart_nb.notifier_call = msm_ps_hold_restart; + pctrl->restart_nb.priority = 128; + if (register_restart_handler(&pctrl->restart_nb)) { + dev_err(pctrl->dev, + "failed to setup restart handler.\n"); + pctrl->restart_nb.notifier_call = NULL; + } + break; } } -#else -static void msm_pinctrl_setup_pm_reset(const struct msm_pinctrl *pctrl) {} -#endif int msm_pinctrl_probe(struct platform_device *pdev, const struct msm_pinctrl_soc_data *soc_data) @@ -943,6 +948,15 @@ int msm_pinctrl_remove(struct platform_device *pdev) pinctrl_unregister(pctrl->pctrl); + if (pctrl->restart_nb.notifier_call) { + ret = unregister_restart_handler(&pctrl->restart_nb); + if (ret) { + dev_err(&pdev->dev, + "unable to unregister restart handler\n"); + return ret; + } + } + return 0; } EXPORT_SYMBOL(msm_pinctrl_remove);
By converting to the restart_notifier mechanism for restart, we allow for other mechanisms, like the watchdog, to be used for restart in the case where PS_HOLD has failed to reset the chip. Choose priority 128, as according to documentation, this mechanism "is sufficient to restart the entire system". Cc: Pramod Gurav <pramod.gurav@smartplayin.com> Cc: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Josh Cartwright <joshc@codeaurora.org> --- drivers/pinctrl/qcom/pinctrl-msm.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-)