[PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Josh Cartwright]

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
-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);
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Josh Cartwright]

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
-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);
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Kumar Gala]

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

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Kumar Gala]

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

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Josh Cartwright]

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Josh Cartwright]

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Kumar Gala]

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
-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Kumar Gala]

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
-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Guenter Roeck]

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

[PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Guenter Roeck]

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

Re: [PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Guenter Roeck]

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

[PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Guenter Roeck]

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

Re: [PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Pramod Gurav]

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

[PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Pramod Gurav]

Hi Josh,

Tested this on IFC6410.
Tested-by: pramod.gurav at 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>
> ---

Re: [PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Josh Cartwright]

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Josh Cartwright]

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Josh Cartwright]

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Josh Cartwright]

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] pinctrl: qcom: use restart_notifier mechanism for ps_hold

[Josh Cartwright]

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation