[PATCH v2] ARM: zynq: use restart_handler mechanism for slcr reset

[PATCH v2] ARM: zynq: use restart_handler mechanism for slcr reset

[Josh Cartwright]

By making use of the restart_handler chain mechanism, the SLCR-based
reset mechanism can be prioritized amongst other mechanisms available on
a particular board.

Choose a default high-ish priority of 192 for this restart mechanism.

Signed-off-by: Josh Cartwright <joshc@ni.com>
---
v1 -> v2: Also drop zynq_slcr_system_reset prototype from common.h

 arch/arm/mach-zynq/common.c |  6 ------
 arch/arm/mach-zynq/common.h |  1 -
 arch/arm/mach-zynq/slcr.c   | 16 ++++++++++++----
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
index c887196..39c1c7d4 100644
--- a/arch/arm/mach-zynq/common.c
+++ b/arch/arm/mach-zynq/common.c
@@ -190,11 +190,6 @@ static void __init zynq_irq_init(void)
 	irqchip_init();
 }
 
-static void zynq_system_reset(enum reboot_mode mode, const char *cmd)
-{
-	zynq_slcr_system_reset();
-}
-
 static const char * const zynq_dt_match[] = {
 	"xlnx,zynq-7000",
 	NULL
@@ -212,5 +207,4 @@ DT_MACHINE_START(XILINX_EP107, "Xilinx Zynq Platform")
 	.init_time	= zynq_timer_init,
 	.dt_compat	= zynq_dt_match,
 	.reserve	= zynq_memory_init,
-	.restart	= zynq_system_reset,
 MACHINE_END
diff --git a/arch/arm/mach-zynq/common.h b/arch/arm/mach-zynq/common.h
index 382c60e..f2f0bf2 100644
--- a/arch/arm/mach-zynq/common.h
+++ b/arch/arm/mach-zynq/common.h
@@ -21,7 +21,6 @@ void zynq_secondary_startup(void);
 
 extern int zynq_slcr_init(void);
 extern int zynq_early_slcr_init(void);
-extern void zynq_slcr_system_reset(void);
 extern void zynq_slcr_cpu_stop(int cpu);
 extern void zynq_slcr_cpu_start(int cpu);
 extern bool zynq_slcr_cpu_state_read(int cpu);
diff --git a/arch/arm/mach-zynq/slcr.c b/arch/arm/mach-zynq/slcr.c
index c3c24fd8..e92b319 100644
--- a/arch/arm/mach-zynq/slcr.c
+++ b/arch/arm/mach-zynq/slcr.c
@@ -15,6 +15,7 @@
  */
 
 #include <linux/io.h>
+#include <linux/reboot.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of_address.h>
 #include <linux/regmap.h>
@@ -91,10 +92,9 @@ u32 zynq_slcr_get_device_id(void)
 	return val;
 }
 
-/**
- * zynq_slcr_system_reset - Reset the entire system.
- */
-void zynq_slcr_system_reset(void)
+static
+int zynq_slcr_system_restart(struct notifier_block *nb,
+			     unsigned long action, void *data)
 {
 	u32 reboot;
 
@@ -113,8 +113,14 @@ void zynq_slcr_system_reset(void)
 	zynq_slcr_read(&reboot, SLCR_REBOOT_STATUS_OFFSET);
 	zynq_slcr_write(reboot & 0xF0FFFFFF, SLCR_REBOOT_STATUS_OFFSET);
 	zynq_slcr_write(1, SLCR_PS_RST_CTRL_OFFSET);
+	return 0;
 }
 
+static struct notifier_block zynq_slcr_restart_nb = {
+	.notifier_call	= zynq_slcr_system_restart,
+	.priority	= 192,
+};
+
 /**
  * zynq_slcr_cpu_start - Start cpu
  * @cpu:	cpu number
@@ -219,6 +225,8 @@ int __init zynq_early_slcr_init(void)
 	/* unlock the SLCR so that registers can be changed */
 	zynq_slcr_unlock();
 
+	register_restart_handler(&zynq_slcr_restart_nb);
+
 	pr_info("%s mapped to %p\n", np->name, zynq_slcr_base);
 
 	of_node_put(np);
-- 
2.3.0

[PATCH v2] ARM: zynq: use restart_handler mechanism for slcr reset

[Josh Cartwright]

On Fri, Feb 27, 2015 at 09:09:29AM -0600, Josh Cartwright wrote:
> By making use of the restart_handler chain mechanism, the SLCR-based
> reset mechanism can be prioritized amongst other mechanisms available on
> a particular board.

Have either of you had a chance to look at this yet?  On our boards we
have a chip external to the SoC that manages power sequencing (and a
hodgepodge of other board-level things), and we would like it to be
prioritized above the SLCR reset mechanism.

Previously we've just hacked up mach-zynq/common.c to not register a
restart handler, and set arm_pm_restart ourselves, but that doesn't seem
appropriate now that the restart chain exists.

Thanks,
  Josh

[PATCH v2] ARM: zynq: use restart_handler mechanism for slcr reset

[Michal Simek]

On 02/27/2015 04:09 PM, Josh Cartwright wrote:
> By making use of the restart_handler chain mechanism, the SLCR-based
> reset mechanism can be prioritized amongst other mechanisms available on
> a particular board.
> 
> Choose a default high-ish priority of 192 for this restart mechanism.
> 
> Signed-off-by: Josh Cartwright <joshc@ni.com>
> ---
> v1 -> v2: Also drop zynq_slcr_system_reset prototype from common.h
> 
>  arch/arm/mach-zynq/common.c |  6 ------
>  arch/arm/mach-zynq/common.h |  1 -
>  arch/arm/mach-zynq/slcr.c   | 16 ++++++++++++----
>  3 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
> index c887196..39c1c7d4 100644
> --- a/arch/arm/mach-zynq/common.c
> +++ b/arch/arm/mach-zynq/common.c
> @@ -190,11 +190,6 @@ static void __init zynq_irq_init(void)
>  	irqchip_init();
>  }
>  
> -static void zynq_system_reset(enum reboot_mode mode, const char *cmd)
> -{
> -	zynq_slcr_system_reset();
> -}
> -
>  static const char * const zynq_dt_match[] = {
>  	"xlnx,zynq-7000",
>  	NULL
> @@ -212,5 +207,4 @@ DT_MACHINE_START(XILINX_EP107, "Xilinx Zynq Platform")
>  	.init_time	= zynq_timer_init,
>  	.dt_compat	= zynq_dt_match,
>  	.reserve	= zynq_memory_init,
> -	.restart	= zynq_system_reset,
>  MACHINE_END
> diff --git a/arch/arm/mach-zynq/common.h b/arch/arm/mach-zynq/common.h
> index 382c60e..f2f0bf2 100644
> --- a/arch/arm/mach-zynq/common.h
> +++ b/arch/arm/mach-zynq/common.h
> @@ -21,7 +21,6 @@ void zynq_secondary_startup(void);
>  
>  extern int zynq_slcr_init(void);
>  extern int zynq_early_slcr_init(void);
> -extern void zynq_slcr_system_reset(void);
>  extern void zynq_slcr_cpu_stop(int cpu);
>  extern void zynq_slcr_cpu_start(int cpu);
>  extern bool zynq_slcr_cpu_state_read(int cpu);
> diff --git a/arch/arm/mach-zynq/slcr.c b/arch/arm/mach-zynq/slcr.c
> index c3c24fd8..e92b319 100644
> --- a/arch/arm/mach-zynq/slcr.c
> +++ b/arch/arm/mach-zynq/slcr.c
> @@ -15,6 +15,7 @@
>   */
>  
>  #include <linux/io.h>
> +#include <linux/reboot.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/of_address.h>
>  #include <linux/regmap.h>
> @@ -91,10 +92,9 @@ u32 zynq_slcr_get_device_id(void)
>  	return val;
>  }
>  
> -/**
> - * zynq_slcr_system_reset - Reset the entire system.
> - */
> -void zynq_slcr_system_reset(void)
> +static
> +int zynq_slcr_system_restart(struct notifier_block *nb,
> +			     unsigned long action, void *data)
>  {

First of all sorry for delay.
Any reason to remove kernel-doc format?

The rest looks good and I have also tested it.

BTW: was also thinking about syscon-reboot option but it doesn't fit to
our reset sequence. :-(

Thanks,
Michal

[PATCH v2] ARM: zynq: use restart_handler mechanism for slcr reset

[Josh Cartwright]

On Thu, Mar 19, 2015 at 11:44:23AM +0100, Michal Simek wrote:
> On 02/27/2015 04:09 PM, Josh Cartwright wrote:
[..]
> > +++ b/arch/arm/mach-zynq/slcr.c
> > @@ -15,6 +15,7 @@
> >   */
> >  
> >  #include <linux/io.h>
> > +#include <linux/reboot.h>
> >  #include <linux/mfd/syscon.h>
> >  #include <linux/of_address.h>
> >  #include <linux/regmap.h>
> > @@ -91,10 +92,9 @@ u32 zynq_slcr_get_device_id(void)
> >  	return val;
> >  }
> >  
> > -/**
> > - * zynq_slcr_system_reset - Reset the entire system.
> > - */
> > -void zynq_slcr_system_reset(void)
> > +static
> > +int zynq_slcr_system_restart(struct notifier_block *nb,
> > +			     unsigned long action, void *data)
> >  {
> 
> First of all sorry for delay.

No problem.  I suspect ZynqMP is keeping you busy.

> Any reason to remove kernel-doc format?

It didn't seem to provide anything meaningful, as it was just a
restatement of the function name, and since this function has become
static, it makes even less sense.

> The rest looks good and I have also tested it.

Great!

> BTW: was also thinking about syscon-reboot option but it doesn't fit to
> our reset sequence. :-(

Because of the code that handles this?

	/*
	 * Clear 0x0F000000 bits of reboot status register to workaround
	 * the FSBL not loading the bitstream after soft-reboot
	 * This is a temporary solution until we know more.
	 */

Has this FSBL bug been addressed?

I suspect we could also drop the zynq_slcr_unlock() as well...we unlock
the SLCR early at boot and don't lock it, AFAICT.

With those two pieces dropped, I think we'd fit the syscon-reboot model.

  Josh

[PATCH v2] ARM: zynq: use restart_handler mechanism for slcr reset

[Michal Simek]

On 03/19/2015 01:44 PM, Josh Cartwright wrote:
> On Thu, Mar 19, 2015 at 11:44:23AM +0100, Michal Simek wrote:
>> On 02/27/2015 04:09 PM, Josh Cartwright wrote:
> [..]
>>> +++ b/arch/arm/mach-zynq/slcr.c
>>> @@ -15,6 +15,7 @@
>>>   */
>>>  
>>>  #include <linux/io.h>
>>> +#include <linux/reboot.h>
>>>  #include <linux/mfd/syscon.h>
>>>  #include <linux/of_address.h>
>>>  #include <linux/regmap.h>
>>> @@ -91,10 +92,9 @@ u32 zynq_slcr_get_device_id(void)
>>>  	return val;
>>>  }
>>>  
>>> -/**
>>> - * zynq_slcr_system_reset - Reset the entire system.
>>> - */
>>> -void zynq_slcr_system_reset(void)
>>> +static
>>> +int zynq_slcr_system_restart(struct notifier_block *nb,
>>> +			     unsigned long action, void *data)
>>>  {
>>
>> First of all sorry for delay.
> 
> No problem.  I suspect ZynqMP is keeping you busy.

yes.

> 
>> Any reason to remove kernel-doc format?
> 
> It didn't seem to provide anything meaningful, as it was just a
> restatement of the function name, and since this function has become
> static, it makes even less sense.

Even static function can do something interesting. The whole file
is using kernel-doc that's why please also keep it here. If any function
misses it then it is just a bug.


>> The rest looks good and I have also tested it.
> 
> Great!
> 
>> BTW: was also thinking about syscon-reboot option but it doesn't fit to
>> our reset sequence. :-(
> 
> Because of the code that handles this?

yep

> 
> 	/*
> 	 * Clear 0x0F000000 bits of reboot status register to workaround
> 	 * the FSBL not loading the bitstream after soft-reboot
> 	 * This is a temporary solution until we know more.
> 	 */
> 
> Has this FSBL bug been addressed?

To be honest the problem is that there could be users in the field which uses
old fsbl and will start to deal with this problem.

> 
> I suspect we could also drop the zynq_slcr_unlock() as well...we unlock
> the SLCR early at boot and don't lock it, AFAICT.

yes - it can be removed. SLCR are already unlocked and this is just useless.

> 
> With those two pieces dropped, I think we'd fit the syscon-reboot model.

yep - but unfortunately I don't think we can remove the first part.

Thanks,
Michal

[PATCH v3 1/2] ARM: zynq: use restart_handler mechanism for slcr reset

[Josh Cartwright]

By making use of the restart_handler chain mechanism, the SLCR-based
reset mechanism can be prioritized amongst other mechanisms available on
a particular board.

Choose a default high-ish priority of 192 for this restart mechanism.

Signed-off-by: Josh Cartwright <joshc@ni.com>
---
v2 -> v3: Don't drop the kerneldoc
v1 -> v2: Also drop zynq_slcr_system_reset prototype from common.h
 arch/arm/mach-zynq/common.c |  6 ------
 arch/arm/mach-zynq/common.h |  1 -
 arch/arm/mach-zynq/slcr.c   | 15 +++++++++++++--
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
index 58ef2a7..616d584 100644
--- a/arch/arm/mach-zynq/common.c
+++ b/arch/arm/mach-zynq/common.c
@@ -190,11 +190,6 @@ static void __init zynq_irq_init(void)
 	irqchip_init();
 }
 
-static void zynq_system_reset(enum reboot_mode mode, const char *cmd)
-{
-	zynq_slcr_system_reset();
-}
-
 static const char * const zynq_dt_match[] = {
 	"xlnx,zynq-7000",
 	NULL
@@ -212,5 +207,4 @@ DT_MACHINE_START(XILINX_EP107, "Xilinx Zynq Platform")
 	.init_time	= zynq_timer_init,
 	.dt_compat	= zynq_dt_match,
 	.reserve	= zynq_memory_init,
-	.restart	= zynq_system_reset,
 MACHINE_END
diff --git a/arch/arm/mach-zynq/common.h b/arch/arm/mach-zynq/common.h
index 382c60e..f2f0bf2 100644
--- a/arch/arm/mach-zynq/common.h
+++ b/arch/arm/mach-zynq/common.h
@@ -21,7 +21,6 @@ void zynq_secondary_startup(void);
 
 extern int zynq_slcr_init(void);
 extern int zynq_early_slcr_init(void);
-extern void zynq_slcr_system_reset(void);
 extern void zynq_slcr_cpu_stop(int cpu);
 extern void zynq_slcr_cpu_start(int cpu);
 extern bool zynq_slcr_cpu_state_read(int cpu);
diff --git a/arch/arm/mach-zynq/slcr.c b/arch/arm/mach-zynq/slcr.c
index c3c24fd8..fa4c796 100644
--- a/arch/arm/mach-zynq/slcr.c
+++ b/arch/arm/mach-zynq/slcr.c
@@ -15,6 +15,7 @@
  */
 
 #include <linux/io.h>
+#include <linux/reboot.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of_address.h>
 #include <linux/regmap.h>
@@ -92,9 +93,11 @@ u32 zynq_slcr_get_device_id(void)
 }
 
 /**
- * zynq_slcr_system_reset - Reset the entire system.
+ * zynq_slcr_system_restart - Restart the entire system.
  */
-void zynq_slcr_system_reset(void)
+static
+int zynq_slcr_system_restart(struct notifier_block *nb,
+			     unsigned long action, void *data)
 {
 	u32 reboot;
 
@@ -113,8 +116,14 @@ void zynq_slcr_system_reset(void)
 	zynq_slcr_read(&reboot, SLCR_REBOOT_STATUS_OFFSET);
 	zynq_slcr_write(reboot & 0xF0FFFFFF, SLCR_REBOOT_STATUS_OFFSET);
 	zynq_slcr_write(1, SLCR_PS_RST_CTRL_OFFSET);
+	return 0;
 }
 
+static struct notifier_block zynq_slcr_restart_nb = {
+	.notifier_call	= zynq_slcr_system_restart,
+	.priority	= 192,
+};
+
 /**
  * zynq_slcr_cpu_start - Start cpu
  * @cpu:	cpu number
@@ -219,6 +228,8 @@ int __init zynq_early_slcr_init(void)
 	/* unlock the SLCR so that registers can be changed */
 	zynq_slcr_unlock();
 
+	register_restart_handler(&zynq_slcr_restart_nb);
+
 	pr_info("%s mapped to %p\n", np->name, zynq_slcr_base);
 
 	of_node_put(np);
-- 
2.3.3

[PATCH 2/2] ARM: zynq: drop use of slcr_unlock in zynq_slcr_system_restart

[Josh Cartwright]

The SLCR is unconditionally unlocked early on boot in zynq_slcr_init()
and not unlocked.  As such, it is not necessary to explicitly unlock in
the restart codepath.

Signed-off-by: Josh Cartwright <joshc@ni.com>
---
 arch/arm/mach-zynq/slcr.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/arch/arm/mach-zynq/slcr.c b/arch/arm/mach-zynq/slcr.c
index fa4c796..698dc3b 100644
--- a/arch/arm/mach-zynq/slcr.c
+++ b/arch/arm/mach-zynq/slcr.c
@@ -102,13 +102,6 @@ int zynq_slcr_system_restart(struct notifier_block *nb,
 	u32 reboot;
 
 	/*
-	 * Unlock the SLCR then reset the system.
-	 * Note that this seems to require raw i/o
-	 * functions or there's a lockup?
-	 */
-	zynq_slcr_unlock();
-
-	/*
 	 * Clear 0x0F000000 bits of reboot status register to workaround
 	 * the FSBL not loading the bitstream after soft-reboot
 	 * This is a temporary solution until we know more.
-- 
2.3.3

[PATCH 2/2] ARM: zynq: drop use of slcr_unlock in zynq_slcr_system_restart

[Josh Cartwright]

Ugh.

On Thu, Mar 19, 2015 at 08:33:53AM -0500, Josh Cartwright wrote:
> The SLCR is unconditionally unlocked early on boot in zynq_slcr_init()
> and not unlocked.  As such, it is not necessary to explicitly unlock in

          ^ever re-locked.

Can you fix this up when you apply?

Thanks,
  Josh

[PATCH v3 1/2] ARM: zynq: use restart_handler mechanism for slcr reset

[Michal Simek]

On 03/19/2015 02:33 PM, Josh Cartwright wrote:
> By making use of the restart_handler chain mechanism, the SLCR-based
> reset mechanism can be prioritized amongst other mechanisms available on
> a particular board.
> 
> Choose a default high-ish priority of 192 for this restart mechanism.
> 
> Signed-off-by: Josh Cartwright <joshc@ni.com>
> ---
> v2 -> v3: Don't drop the kerneldoc
> v1 -> v2: Also drop zynq_slcr_system_reset prototype from common.h
>  arch/arm/mach-zynq/common.c |  6 ------
>  arch/arm/mach-zynq/common.h |  1 -
>  arch/arm/mach-zynq/slcr.c   | 15 +++++++++++++--
>  3 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
> index 58ef2a7..616d584 100644
> --- a/arch/arm/mach-zynq/common.c
> +++ b/arch/arm/mach-zynq/common.c
> @@ -190,11 +190,6 @@ static void __init zynq_irq_init(void)
>  	irqchip_init();
>  }
>  
> -static void zynq_system_reset(enum reboot_mode mode, const char *cmd)
> -{
> -	zynq_slcr_system_reset();
> -}
> -
>  static const char * const zynq_dt_match[] = {
>  	"xlnx,zynq-7000",
>  	NULL
> @@ -212,5 +207,4 @@ DT_MACHINE_START(XILINX_EP107, "Xilinx Zynq Platform")
>  	.init_time	= zynq_timer_init,
>  	.dt_compat	= zynq_dt_match,
>  	.reserve	= zynq_memory_init,
> -	.restart	= zynq_system_reset,
>  MACHINE_END
> diff --git a/arch/arm/mach-zynq/common.h b/arch/arm/mach-zynq/common.h
> index 382c60e..f2f0bf2 100644
> --- a/arch/arm/mach-zynq/common.h
> +++ b/arch/arm/mach-zynq/common.h
> @@ -21,7 +21,6 @@ void zynq_secondary_startup(void);
>  
>  extern int zynq_slcr_init(void);
>  extern int zynq_early_slcr_init(void);
> -extern void zynq_slcr_system_reset(void);
>  extern void zynq_slcr_cpu_stop(int cpu);
>  extern void zynq_slcr_cpu_start(int cpu);
>  extern bool zynq_slcr_cpu_state_read(int cpu);
> diff --git a/arch/arm/mach-zynq/slcr.c b/arch/arm/mach-zynq/slcr.c
> index c3c24fd8..fa4c796 100644
> --- a/arch/arm/mach-zynq/slcr.c
> +++ b/arch/arm/mach-zynq/slcr.c
> @@ -15,6 +15,7 @@
>   */
>  
>  #include <linux/io.h>
> +#include <linux/reboot.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/of_address.h>
>  #include <linux/regmap.h>
> @@ -92,9 +93,11 @@ u32 zynq_slcr_get_device_id(void)
>  }
>  
>  /**
> - * zynq_slcr_system_reset - Reset the entire system.
> + * zynq_slcr_system_restart - Restart the entire system.
>   */
> -void zynq_slcr_system_reset(void)
> +static
> +int zynq_slcr_system_restart(struct notifier_block *nb,
> +			     unsigned long action, void *data)

This doesn't look right to me.

[linux]$ ./scripts/kernel-doc -man -v arch/arm/mach-zynq/slcr.c >/dev/null
Info(arch/arm/mach-zynq/slcr.c:42): Scanning doc for zynq_slcr_write
Info(arch/arm/mach-zynq/slcr.c:55): Scanning doc for zynq_slcr_read
Info(arch/arm/mach-zynq/slcr.c:68): Scanning doc for zynq_slcr_unlock
Info(arch/arm/mach-zynq/slcr.c:80): Scanning doc for zynq_slcr_get_device_id
Info(arch/arm/mach-zynq/slcr.c:96): Scanning doc for zynq_slcr_system_restart
Warning(arch/arm/mach-zynq/slcr.c:101): No description found for parameter 'nb'
Warning(arch/arm/mach-zynq/slcr.c:101): No description found for parameter 'action'
Warning(arch/arm/mach-zynq/slcr.c:101): No description found for parameter 'data'
Warning(arch/arm/mach-zynq/slcr.c:101): No description found for return value of 'zynq_slcr_system_restart'

Thanks,
Michal

[PATCH v3 1/2] ARM: zynq: use restart_handler mechanism for slcr reset

[Josh Cartwright]

On Thu, Mar 19, 2015 at 03:01:13PM +0100, Michal Simek wrote:
> On 03/19/2015 02:33 PM, Josh Cartwright wrote:
[..]
> >  /**
> > - * zynq_slcr_system_reset - Reset the entire system.
> > + * zynq_slcr_system_restart - Restart the entire system.
> >   */
> > -void zynq_slcr_system_reset(void)
> > +static
> > +int zynq_slcr_system_restart(struct notifier_block *nb,
> > +			     unsigned long action, void *data)
> 
> This doesn't look right to me.
> 
> [linux]$ ./scripts/kernel-doc -man -v arch/arm/mach-zynq/slcr.c >/dev/null
> Info(arch/arm/mach-zynq/slcr.c:42): Scanning doc for zynq_slcr_write
> Info(arch/arm/mach-zynq/slcr.c:55): Scanning doc for zynq_slcr_read
> Info(arch/arm/mach-zynq/slcr.c:68): Scanning doc for zynq_slcr_unlock
> Info(arch/arm/mach-zynq/slcr.c:80): Scanning doc for zynq_slcr_get_device_id
> Info(arch/arm/mach-zynq/slcr.c:96): Scanning doc for zynq_slcr_system_restart
> Warning(arch/arm/mach-zynq/slcr.c:101): No description found for parameter 'nb'
> Warning(arch/arm/mach-zynq/slcr.c:101): No description found for parameter 'action'
> Warning(arch/arm/mach-zynq/slcr.c:101): No description found for parameter 'data'
> Warning(arch/arm/mach-zynq/slcr.c:101): No description found for return value of 'zynq_slcr_system_restart'

*Sigh*.  I guess now is as good as ever to learn how to write kerneldoc.

I can't help but feel this effort really isn't worth it.  I'll do it,
obviously, because I want this patch to go in, but I really don't
understand at all what value is being provided here.

Are you advocating for _every_ function in the kernel to have an
associated kerneldoc annotation?  Even for small/self-evident/internal
functions?

In my opinion, kerneldoc annotations make sense for those functions
which:

  1.) Are widely used (think synchronization primitives, device core, etc.)
  2.) Have non-obvious semantics, or:
  3.) Have caller-mandated requirements that aren't clear otherwise
      (locks to be acquired, lifecycle management requirements, state to
       be maintained, etc.)

Beyond that, it's just review churn and fighting the inevitable
code-and-documentation-out-of-sync problem.

  Josh

[PATCH v4] ARM: zynq: use restart_handler mechanism for slcr reset

[Josh Cartwright]

By making use of the restart_handler chain mechanism, the SLCR-based
reset mechanism can be prioritized amongst other mechanisms available on
a particular board.

Choose a default high-ish priority of 192 for this restart mechanism.

Signed-off-by: Josh Cartwright <joshc@ni.com>
---

v3 -> v4: Document restart notifier callback parameters.
v2 -> v3: Don't drop the kerneldoc
v1 -> v2: Also drop zynq_slcr_system_reset prototype from common.h

 arch/arm/mach-zynq/common.c |  6 ------
 arch/arm/mach-zynq/common.h |  1 -
 arch/arm/mach-zynq/slcr.c   | 21 +++++++++++++++++++--
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
index 58ef2a7..616d584 100644
--- a/arch/arm/mach-zynq/common.c
+++ b/arch/arm/mach-zynq/common.c
@@ -190,11 +190,6 @@ static void __init zynq_irq_init(void)
 	irqchip_init();
 }
 
-static void zynq_system_reset(enum reboot_mode mode, const char *cmd)
-{
-	zynq_slcr_system_reset();
-}
-
 static const char * const zynq_dt_match[] = {
 	"xlnx,zynq-7000",
 	NULL
@@ -212,5 +207,4 @@ DT_MACHINE_START(XILINX_EP107, "Xilinx Zynq Platform")
 	.init_time	= zynq_timer_init,
 	.dt_compat	= zynq_dt_match,
 	.reserve	= zynq_memory_init,
-	.restart	= zynq_system_reset,
 MACHINE_END
diff --git a/arch/arm/mach-zynq/common.h b/arch/arm/mach-zynq/common.h
index 382c60e..f2f0bf2 100644
--- a/arch/arm/mach-zynq/common.h
+++ b/arch/arm/mach-zynq/common.h
@@ -21,7 +21,6 @@ void zynq_secondary_startup(void);
 
 extern int zynq_slcr_init(void);
 extern int zynq_early_slcr_init(void);
-extern void zynq_slcr_system_reset(void);
 extern void zynq_slcr_cpu_stop(int cpu);
 extern void zynq_slcr_cpu_start(int cpu);
 extern bool zynq_slcr_cpu_state_read(int cpu);
diff --git a/arch/arm/mach-zynq/slcr.c b/arch/arm/mach-zynq/slcr.c
index c3c24fd8..af8fb50 100644
--- a/arch/arm/mach-zynq/slcr.c
+++ b/arch/arm/mach-zynq/slcr.c
@@ -15,6 +15,7 @@
  */
 
 #include <linux/io.h>
+#include <linux/reboot.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of_address.h>
 #include <linux/regmap.h>
@@ -92,9 +93,17 @@ u32 zynq_slcr_get_device_id(void)
 }
 
 /**
- * zynq_slcr_system_reset - Reset the entire system.
+ * zynq_slcr_system_restart - Restart the entire system.
+ *
+ * @nb:		Pointer to restart notifier block (unused)
+ * @action:	Reboot mode (unused)
+ * @data:	Restart handler private data (unused)
+ *
+ * Return:	a negative value on error, 0 on success
  */
-void zynq_slcr_system_reset(void)
+static
+int zynq_slcr_system_restart(struct notifier_block *nb,
+			     unsigned long action, void *data)
 {
 	u32 reboot;
 
@@ -113,8 +122,14 @@ void zynq_slcr_system_reset(void)
 	zynq_slcr_read(&reboot, SLCR_REBOOT_STATUS_OFFSET);
 	zynq_slcr_write(reboot & 0xF0FFFFFF, SLCR_REBOOT_STATUS_OFFSET);
 	zynq_slcr_write(1, SLCR_PS_RST_CTRL_OFFSET);
+	return 0;
 }
 
+static struct notifier_block zynq_slcr_restart_nb = {
+	.notifier_call	= zynq_slcr_system_restart,
+	.priority	= 192,
+};
+
 /**
  * zynq_slcr_cpu_start - Start cpu
  * @cpu:	cpu number
@@ -219,6 +234,8 @@ int __init zynq_early_slcr_init(void)
 	/* unlock the SLCR so that registers can be changed */
 	zynq_slcr_unlock();
 
+	register_restart_handler(&zynq_slcr_restart_nb);
+
 	pr_info("%s mapped to %p\n", np->name, zynq_slcr_base);
 
 	of_node_put(np);
-- 
2.3.3

[PATCH v4] ARM: zynq: use restart_handler mechanism for slcr reset

[Michal Simek]

On 03/19/2015 03:24 PM, Josh Cartwright wrote:
> By making use of the restart_handler chain mechanism, the SLCR-based
> reset mechanism can be prioritized amongst other mechanisms available on
> a particular board.
> 
> Choose a default high-ish priority of 192 for this restart mechanism.
> 
> Signed-off-by: Josh Cartwright <joshc@ni.com>
> ---
> 
> v3 -> v4: Document restart notifier callback parameters.
> v2 -> v3: Don't drop the kerneldoc
> v1 -> v2: Also drop zynq_slcr_system_reset prototype from common.h
> 
>  arch/arm/mach-zynq/common.c |  6 ------
>  arch/arm/mach-zynq/common.h |  1 -
>  arch/arm/mach-zynq/slcr.c   | 21 +++++++++++++++++++--
>  3 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
> index 58ef2a7..616d584 100644
> --- a/arch/arm/mach-zynq/common.c
> +++ b/arch/arm/mach-zynq/common.c
> @@ -190,11 +190,6 @@ static void __init zynq_irq_init(void)
>  	irqchip_init();
>  }
>  
> -static void zynq_system_reset(enum reboot_mode mode, const char *cmd)
> -{
> -	zynq_slcr_system_reset();
> -}
> -
>  static const char * const zynq_dt_match[] = {
>  	"xlnx,zynq-7000",
>  	NULL
> @@ -212,5 +207,4 @@ DT_MACHINE_START(XILINX_EP107, "Xilinx Zynq Platform")
>  	.init_time	= zynq_timer_init,
>  	.dt_compat	= zynq_dt_match,
>  	.reserve	= zynq_memory_init,
> -	.restart	= zynq_system_reset,
>  MACHINE_END
> diff --git a/arch/arm/mach-zynq/common.h b/arch/arm/mach-zynq/common.h
> index 382c60e..f2f0bf2 100644
> --- a/arch/arm/mach-zynq/common.h
> +++ b/arch/arm/mach-zynq/common.h
> @@ -21,7 +21,6 @@ void zynq_secondary_startup(void);
>  
>  extern int zynq_slcr_init(void);
>  extern int zynq_early_slcr_init(void);
> -extern void zynq_slcr_system_reset(void);
>  extern void zynq_slcr_cpu_stop(int cpu);
>  extern void zynq_slcr_cpu_start(int cpu);
>  extern bool zynq_slcr_cpu_state_read(int cpu);
> diff --git a/arch/arm/mach-zynq/slcr.c b/arch/arm/mach-zynq/slcr.c
> index c3c24fd8..af8fb50 100644
> --- a/arch/arm/mach-zynq/slcr.c
> +++ b/arch/arm/mach-zynq/slcr.c
> @@ -15,6 +15,7 @@
>   */
>  
>  #include <linux/io.h>
> +#include <linux/reboot.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/of_address.h>
>  #include <linux/regmap.h>
> @@ -92,9 +93,17 @@ u32 zynq_slcr_get_device_id(void)
>  }
>  
>  /**
> - * zynq_slcr_system_reset - Reset the entire system.
> + * zynq_slcr_system_restart - Restart the entire system.
> + *
> + * @nb:		Pointer to restart notifier block (unused)
> + * @action:	Reboot mode (unused)
> + * @data:	Restart handler private data (unused)
> + *
> + * Return:	a negative value on error, 0 on success

:-) I can't see any negative value on error.
I will fix it myself to 0 always.

Applied both.

Cheers,
Michal

[PATCH v2] ARM: zynq: use restart_handler mechanism for slcr reset

[Josh Cartwright]

On Thu, Mar 19, 2015 at 02:19:01PM +0100, Michal Simek wrote:
> On 03/19/2015 01:44 PM, Josh Cartwright wrote:
> > On Thu, Mar 19, 2015 at 11:44:23AM +0100, Michal Simek wrote:
> >> On 02/27/2015 04:09 PM, Josh Cartwright wrote:
> > [..]
> >>> +++ b/arch/arm/mach-zynq/slcr.c
> >>> @@ -15,6 +15,7 @@
> >>>   */
> >>>  
> >>>  #include <linux/io.h>
> >>> +#include <linux/reboot.h>
> >>>  #include <linux/mfd/syscon.h>
> >>>  #include <linux/of_address.h>
> >>>  #include <linux/regmap.h>
> >>> @@ -91,10 +92,9 @@ u32 zynq_slcr_get_device_id(void)
> >>>  	return val;
> >>>  }
> >>>  
> >>> -/**
> >>> - * zynq_slcr_system_reset - Reset the entire system.
> >>> - */
> >>> -void zynq_slcr_system_reset(void)
> >>> +static
> >>> +int zynq_slcr_system_restart(struct notifier_block *nb,
> >>> +			     unsigned long action, void *data)
> >>>  {
> >>
> >> First of all sorry for delay.
> > 
> > No problem.  I suspect ZynqMP is keeping you busy.
> 
> yes.
> 
> > 
> >> Any reason to remove kernel-doc format?
> > 
> > It didn't seem to provide anything meaningful, as it was just a
> > restatement of the function name, and since this function has become
> > static, it makes even less sense.
> 
> Even static function can do something interesting. The whole file
> is using kernel-doc that's why please also keep it here. If any function
> misses it then it is just a bug.

Okay, sure.  Sent out a v3 with the kerneldoc updated.

[..]
> > 
> > Has this FSBL bug been addressed?
> 
> To be honest the problem is that there could be users in the field which uses
> old fsbl and will start to deal with this problem.

Yeah,  I suppose we're destined to carry it for long time.

Thanks,
  Josh