diff mbox

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

Message ID 1425049769-24929-1-git-send-email-joshc@ni.com (mailing list archive)
State New, archived
Headers show

Commit Message

Josh Cartwright Feb. 27, 2015, 3:09 p.m. UTC
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(-)

Comments

Josh Cartwright March 11, 2015, 3:41 p.m. UTC | #1
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
Michal Simek March 19, 2015, 10:44 a.m. UTC | #2
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
Josh Cartwright March 19, 2015, 12:44 p.m. UTC | #3
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
Michal Simek March 19, 2015, 1:19 p.m. UTC | #4
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
Josh Cartwright March 19, 2015, 1:37 p.m. UTC | #5
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
diff mbox

Patch

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