diff mbox

[v2,1/3] power: reset: read priority from device tree

Message ID 1417453389-1588-2-git-send-email-stefan@agner.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Stefan Agner Dec. 1, 2014, 5:03 p.m. UTC
This patch adds an optional property which allows to specify the
reset source priority. This priority is used by the kernel restart
handler call chain to sort out the proper reset/restart method.
Depending on the power design of a board or other machine/board
specific peculiarity, it is not possible to pick a generic priority.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
 drivers/power/reset/syscon-reboot.c                             | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

Mark Rutland Dec. 1, 2014, 5:11 p.m. UTC | #1
On Mon, Dec 01, 2014 at 05:03:07PM +0000, Stefan Agner wrote:
> This patch adds an optional property which allows to specify the
> reset source priority. This priority is used by the kernel restart
> handler call chain to sort out the proper reset/restart method.
> Depending on the power design of a board or other machine/board
> specific peculiarity, it is not possible to pick a generic priority.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
>  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> index 1190631..ee41d9c 100644
> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> @@ -11,6 +11,9 @@ Required properties:
>  - offset: offset in the register map for the reboot register (in bytes)
>  - mask: the reset value written to the reboot register (32 bit access)
>  
> +Optional properties:
> +- priority: define the priority of the reset (0-255, defaults to 128)
> +

NAK. This is a leak of Linux-internal details.

What is this necessary for?

Mark.

>  Default will be little endian mode, 32 bit access only.
>  
>  Examples:
> diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
> index 815b901..3060d6b 100644
> --- a/drivers/power/reset/syscon-reboot.c
> +++ b/drivers/power/reset/syscon-reboot.c
> @@ -67,8 +67,11 @@ static int syscon_reboot_probe(struct platform_device *pdev)
>  	if (of_property_read_u32(pdev->dev.of_node, "mask", &ctx->mask))
>  		return -EINVAL;
>  
> -	ctx->restart_handler.notifier_call = syscon_restart_handle;
>  	ctx->restart_handler.priority = 128;
> +	of_property_read_u32(pdev->dev.of_node, "priority",
> +			     &ctx->restart_handler.priority);
> +
> +	ctx->restart_handler.notifier_call = syscon_restart_handle;
>  	err = register_restart_handler(&ctx->restart_handler);
>  	if (err)
>  		dev_err(dev, "can't register restart notifier (err=%d)\n", err);
> -- 
> 2.1.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Feng Kan Dec. 1, 2014, 5:15 p.m. UTC | #2
On Mon, Dec 1, 2014 at 9:03 AM, Stefan Agner <stefan@agner.ch> wrote:
> This patch adds an optional property which allows to specify the
> reset source priority. This priority is used by the kernel restart
> handler call chain to sort out the proper reset/restart method.
> Depending on the power design of a board or other machine/board
> specific peculiarity, it is not possible to pick a generic priority.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
>  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> index 1190631..ee41d9c 100644
> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> @@ -11,6 +11,9 @@ Required properties:
>  - offset: offset in the register map for the reboot register (in bytes)
>  - mask: the reset value written to the reboot register (32 bit access)
>
> +Optional properties:
> +- priority: define the priority of the reset (0-255, defaults to 128)
> +
>  Default will be little endian mode, 32 bit access only.
>
>  Examples:
> diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
> index 815b901..3060d6b 100644
> --- a/drivers/power/reset/syscon-reboot.c
> +++ b/drivers/power/reset/syscon-reboot.c
> @@ -67,8 +67,11 @@ static int syscon_reboot_probe(struct platform_device *pdev)
>         if (of_property_read_u32(pdev->dev.of_node, "mask", &ctx->mask))
>                 return -EINVAL;
>
> -       ctx->restart_handler.notifier_call = syscon_restart_handle;
>         ctx->restart_handler.priority = 128;
> +       of_property_read_u32(pdev->dev.of_node, "priority",
> +                            &ctx->restart_handler.priority);

What is this for?
> +
> +       ctx->restart_handler.notifier_call = syscon_restart_handle;
>         err = register_restart_handler(&ctx->restart_handler);
>         if (err)
>                 dev_err(dev, "can't register restart notifier (err=%d)\n", err);
> --
> 2.1.3
>
Guenter Roeck Dec. 1, 2014, 5:22 p.m. UTC | #3
On Mon, Dec 01, 2014 at 05:11:26PM +0000, Mark Rutland wrote:
> On Mon, Dec 01, 2014 at 05:03:07PM +0000, Stefan Agner wrote:
> > This patch adds an optional property which allows to specify the
> > reset source priority. This priority is used by the kernel restart
> > handler call chain to sort out the proper reset/restart method.
> > Depending on the power design of a board or other machine/board
> > specific peculiarity, it is not possible to pick a generic priority.
> > 
> > Signed-off-by: Stefan Agner <stefan@agner.ch>
> > ---
> >  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
> >  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > index 1190631..ee41d9c 100644
> > --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > @@ -11,6 +11,9 @@ Required properties:
> >  - offset: offset in the register map for the reboot register (in bytes)
> >  - mask: the reset value written to the reboot register (32 bit access)
> >  
> > +Optional properties:
> > +- priority: define the priority of the reset (0-255, defaults to 128)
> > +
> 
> NAK. This is a leak of Linux-internal details.
> 
> What is this necessary for?
> 
It establishes which restart handler to use if there more than one means
to reset the system. See documentation to register_restart_handler() in
kernel/reboot.c.

Guenter
Stefan Agner Dec. 1, 2014, 5:24 p.m. UTC | #4
On 2014-12-01 18:11, Mark Rutland wrote:
> On Mon, Dec 01, 2014 at 05:03:07PM +0000, Stefan Agner wrote:
>> This patch adds an optional property which allows to specify the
>> reset source priority. This priority is used by the kernel restart
>> handler call chain to sort out the proper reset/restart method.
>> Depending on the power design of a board or other machine/board
>> specific peculiarity, it is not possible to pick a generic priority.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
>>  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> index 1190631..ee41d9c 100644
>> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> @@ -11,6 +11,9 @@ Required properties:
>>  - offset: offset in the register map for the reboot register (in bytes)
>>  - mask: the reset value written to the reboot register (32 bit access)
>>
>> +Optional properties:
>> +- priority: define the priority of the reset (0-255, defaults to 128)
>> +
> 
> NAK. This is a leak of Linux-internal details.
> 
> What is this necessary for?
> 
> Mark.

Hi Mark,

In my case, it is necessary to be called ahead of the watchdog, which
has a priority of 128. This syscon-reboot driver currently has a default
priority of 128 too. I could live with a higher default priority for the
syscon-reboot driver, in fact I proposed that in the discussion of v1 of
that patch:
https://lkml.org/lkml/2014/11/28/484

IMHO, this priority might make sense for most cases, I guess that
dedicated "syscon" capabilities are usually better suited as a reboot
source than watchdog.

If dt, then the question which arises: If there are different
capabilities to reset/reboot a whole system, how do we reflect which is
the best suited one in dt?

--
Stefan

> 
>>  Default will be little endian mode, 32 bit access only.
>>
>>  Examples:
>> diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
>> index 815b901..3060d6b 100644
>> --- a/drivers/power/reset/syscon-reboot.c
>> +++ b/drivers/power/reset/syscon-reboot.c
>> @@ -67,8 +67,11 @@ static int syscon_reboot_probe(struct platform_device *pdev)
>>  	if (of_property_read_u32(pdev->dev.of_node, "mask", &ctx->mask))
>>  		return -EINVAL;
>>
>> -	ctx->restart_handler.notifier_call = syscon_restart_handle;
>>  	ctx->restart_handler.priority = 128;
>> +	of_property_read_u32(pdev->dev.of_node, "priority",
>> +			     &ctx->restart_handler.priority);
>> +
>> +	ctx->restart_handler.notifier_call = syscon_restart_handle;
>>  	err = register_restart_handler(&ctx->restart_handler);
>>  	if (err)
>>  		dev_err(dev, "can't register restart notifier (err=%d)\n", err);
>> --
>> 2.1.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
Stefan Agner Dec. 1, 2014, 5:29 p.m. UTC | #5
On 2014-12-01 18:15, Feng Kan wrote:
> On Mon, Dec 1, 2014 at 9:03 AM, Stefan Agner <stefan@agner.ch> wrote:
>> This patch adds an optional property which allows to specify the
>> reset source priority. This priority is used by the kernel restart
>> handler call chain to sort out the proper reset/restart method.
>> Depending on the power design of a board or other machine/board
>> specific peculiarity, it is not possible to pick a generic priority.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
>>  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> index 1190631..ee41d9c 100644
>> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> @@ -11,6 +11,9 @@ Required properties:
>>  - offset: offset in the register map for the reboot register (in bytes)
>>  - mask: the reset value written to the reboot register (32 bit access)
>>
>> +Optional properties:
>> +- priority: define the priority of the reset (0-255, defaults to 128)
>> +
>>  Default will be little endian mode, 32 bit access only.
>>
>>  Examples:
>> diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
>> index 815b901..3060d6b 100644
>> --- a/drivers/power/reset/syscon-reboot.c
>> +++ b/drivers/power/reset/syscon-reboot.c
>> @@ -67,8 +67,11 @@ static int syscon_reboot_probe(struct platform_device *pdev)
>>         if (of_property_read_u32(pdev->dev.of_node, "mask", &ctx->mask))
>>                 return -EINVAL;
>>
>> -       ctx->restart_handler.notifier_call = syscon_restart_handle;
>>         ctx->restart_handler.priority = 128;
>> +       of_property_read_u32(pdev->dev.of_node, "priority",
>> +                            &ctx->restart_handler.priority);
> 
> What is this for?

What do you mean? The moved line "ctx->restart_handler.notifier_call =
syscon_restart_handle;"? When one reads the diff, it looks like that
line was moved, in fact I tried to keep the of_property_read function
calls together. But I had to move the assignation of the default
priority in front of restart_handler.priority. That's what is the
outcome...

--
Stefan

>> +
>> +       ctx->restart_handler.notifier_call = syscon_restart_handle;
>>         err = register_restart_handler(&ctx->restart_handler);
>>         if (err)
>>                 dev_err(dev, "can't register restart notifier (err=%d)\n", err);
>> --
>> 2.1.3
>>
Feng Kan Dec. 1, 2014, 5:38 p.m. UTC | #6
On Mon, Dec 1, 2014 at 9:29 AM, Stefan Agner <stefan@agner.ch> wrote:
> On 2014-12-01 18:15, Feng Kan wrote:
>> On Mon, Dec 1, 2014 at 9:03 AM, Stefan Agner <stefan@agner.ch> wrote:
>>> This patch adds an optional property which allows to specify the
>>> reset source priority. This priority is used by the kernel restart
>>> handler call chain to sort out the proper reset/restart method.
>>> Depending on the power design of a board or other machine/board
>>> specific peculiarity, it is not possible to pick a generic priority.
>>>
>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>> ---
>>>  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
>>>  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
>>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>>> index 1190631..ee41d9c 100644
>>> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>>> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>>> @@ -11,6 +11,9 @@ Required properties:
>>>  - offset: offset in the register map for the reboot register (in bytes)
>>>  - mask: the reset value written to the reboot register (32 bit access)
>>>
>>> +Optional properties:
>>> +- priority: define the priority of the reset (0-255, defaults to 128)
>>> +
>>>  Default will be little endian mode, 32 bit access only.
>>>
>>>  Examples:
>>> diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
>>> index 815b901..3060d6b 100644
>>> --- a/drivers/power/reset/syscon-reboot.c
>>> +++ b/drivers/power/reset/syscon-reboot.c
>>> @@ -67,8 +67,11 @@ static int syscon_reboot_probe(struct platform_device *pdev)
>>>         if (of_property_read_u32(pdev->dev.of_node, "mask", &ctx->mask))
>>>                 return -EINVAL;
>>>
>>> -       ctx->restart_handler.notifier_call = syscon_restart_handle;
>>>         ctx->restart_handler.priority = 128;
>>> +       of_property_read_u32(pdev->dev.of_node, "priority",
>>> +                            &ctx->restart_handler.priority);
>>
>> What is this for?
>
> What do you mean? The moved line "ctx->restart_handler.notifier_call =
> syscon_restart_handle;"? When one reads the diff, it looks like that
> line was moved, in fact I tried to keep the of_property_read function
> calls together. But I had to move the assignation of the default
> priority in front of restart_handler.priority. That's what is the
> outcome...

I believe Guenter explained above already. Actually this help to solve one of
my problem. Thanks.
>
> --
> Stefan
>
>>> +
>>> +       ctx->restart_handler.notifier_call = syscon_restart_handle;
>>>         err = register_restart_handler(&ctx->restart_handler);
>>>         if (err)
>>>                 dev_err(dev, "can't register restart notifier (err=%d)\n", err);
>>> --
>>> 2.1.3
>>>
>
Mark Rutland Dec. 1, 2014, 5:41 p.m. UTC | #7
On Mon, Dec 01, 2014 at 05:24:37PM +0000, Stefan Agner wrote:
> On 2014-12-01 18:11, Mark Rutland wrote:
> > On Mon, Dec 01, 2014 at 05:03:07PM +0000, Stefan Agner wrote:
> >> This patch adds an optional property which allows to specify the
> >> reset source priority. This priority is used by the kernel restart
> >> handler call chain to sort out the proper reset/restart method.
> >> Depending on the power design of a board or other machine/board
> >> specific peculiarity, it is not possible to pick a generic priority.
> >>
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >>  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
> >>  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
> >>  2 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> index 1190631..ee41d9c 100644
> >> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> @@ -11,6 +11,9 @@ Required properties:
> >>  - offset: offset in the register map for the reboot register (in bytes)
> >>  - mask: the reset value written to the reboot register (32 bit access)
> >>
> >> +Optional properties:
> >> +- priority: define the priority of the reset (0-255, defaults to 128)
> >> +
> > 
> > NAK. This is a leak of Linux-internal details.
> > 
> > What is this necessary for?
> > 
> > Mark.
> 
> Hi Mark,
> 
> In my case, it is necessary to be called ahead of the watchdog, which
> has a priority of 128. This syscon-reboot driver currently has a default
> priority of 128 too. I could live with a higher default priority for the
> syscon-reboot driver, in fact I proposed that in the discussion of v1 of
> that patch:
> https://lkml.org/lkml/2014/11/28/484

Thanks for the link.

> IMHO, this priority might make sense for most cases, I guess that
> dedicated "syscon" capabilities are usually better suited as a reboot
> source than watchdog.

I would think likewise.

> If dt, then the question which arises: If there are different
> capabilities to reset/reboot a whole system, how do we reflect which is
> the best suited one in dt?

I'm not sure, but I don't think that exposing a priority variable in
this way is the best, because it implicitly relies on what the kernel
may have selected for other devices and/or FW interfaces, which may not
have been described in DT.

So if we can get away with a fixed priority for now, then I would prefer
that.

Otherwise, I would imagine that most systems have a single preferred
mechanism with some possible fallback(s), for which a single
preferred-poweroff property might suffice, and has better interaction
w.r.t. priority (in that it should _always_ be tried first). Even that's
difficult to reconcile with FW bindings though, especially EFI (which we
sometimes must use in preference for variable storage and capsule
updates).

Thanks,
Mark.
Guenter Roeck Dec. 1, 2014, 5:42 p.m. UTC | #8
On Mon, Dec 01, 2014 at 09:38:09AM -0800, Feng Kan wrote:
> On Mon, Dec 1, 2014 at 9:29 AM, Stefan Agner <stefan@agner.ch> wrote:
> > On 2014-12-01 18:15, Feng Kan wrote:
> >> On Mon, Dec 1, 2014 at 9:03 AM, Stefan Agner <stefan@agner.ch> wrote:
> >>> This patch adds an optional property which allows to specify the
> >>> reset source priority. This priority is used by the kernel restart
> >>> handler call chain to sort out the proper reset/restart method.
> >>> Depending on the power design of a board or other machine/board
> >>> specific peculiarity, it is not possible to pick a generic priority.
> >>>
> >>> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >>> ---
> >>>  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
> >>>  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
> >>>  2 files changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >>> index 1190631..ee41d9c 100644
> >>> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >>> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >>> @@ -11,6 +11,9 @@ Required properties:
> >>>  - offset: offset in the register map for the reboot register (in bytes)
> >>>  - mask: the reset value written to the reboot register (32 bit access)
> >>>
> >>> +Optional properties:
> >>> +- priority: define the priority of the reset (0-255, defaults to 128)
> >>> +
> >>>  Default will be little endian mode, 32 bit access only.
> >>>
> >>>  Examples:
> >>> diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
> >>> index 815b901..3060d6b 100644
> >>> --- a/drivers/power/reset/syscon-reboot.c
> >>> +++ b/drivers/power/reset/syscon-reboot.c
> >>> @@ -67,8 +67,11 @@ static int syscon_reboot_probe(struct platform_device *pdev)
> >>>         if (of_property_read_u32(pdev->dev.of_node, "mask", &ctx->mask))
> >>>                 return -EINVAL;
> >>>
> >>> -       ctx->restart_handler.notifier_call = syscon_restart_handle;
> >>>         ctx->restart_handler.priority = 128;
> >>> +       of_property_read_u32(pdev->dev.of_node, "priority",
> >>> +                            &ctx->restart_handler.priority);
> >>
> >> What is this for?
> >
> > What do you mean? The moved line "ctx->restart_handler.notifier_call =
> > syscon_restart_handle;"? When one reads the diff, it looks like that
> > line was moved, in fact I tried to keep the of_property_read function
> > calls together. But I had to move the assignation of the default
> > priority in front of restart_handler.priority. That's what is the
> > outcome...
> 
> I believe Guenter explained above already. Actually this help to solve one of
> my problem. Thanks.

Since Mark doesn't seem to be happy with the idea of making the priority
dt-configurable, the alternative might be to just set a higher priority for
syscon triggered resets, as suggested by Stefan. Not as flexible, but it
should be ok for most use cases.

Guenter
Guenter Roeck Dec. 1, 2014, 5:51 p.m. UTC | #9
On Mon, Dec 01, 2014 at 05:41:00PM +0000, Mark Rutland wrote:
> On Mon, Dec 01, 2014 at 05:24:37PM +0000, Stefan Agner wrote:
> > On 2014-12-01 18:11, Mark Rutland wrote:
> > > On Mon, Dec 01, 2014 at 05:03:07PM +0000, Stefan Agner wrote:
> > >> This patch adds an optional property which allows to specify the
> > >> reset source priority. This priority is used by the kernel restart
> > >> handler call chain to sort out the proper reset/restart method.
> > >> Depending on the power design of a board or other machine/board
> > >> specific peculiarity, it is not possible to pick a generic priority.
> > >>
> > >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> > >> ---
> > >>  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
> > >>  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
> > >>  2 files changed, 7 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > >> index 1190631..ee41d9c 100644
> > >> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > >> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > >> @@ -11,6 +11,9 @@ Required properties:
> > >>  - offset: offset in the register map for the reboot register (in bytes)
> > >>  - mask: the reset value written to the reboot register (32 bit access)
> > >>
> > >> +Optional properties:
> > >> +- priority: define the priority of the reset (0-255, defaults to 128)
> > >> +
> > > 
> > > NAK. This is a leak of Linux-internal details.
> > > 
> > > What is this necessary for?
> > > 
> > > Mark.
> > 
> > Hi Mark,
> > 
> > In my case, it is necessary to be called ahead of the watchdog, which
> > has a priority of 128. This syscon-reboot driver currently has a default
> > priority of 128 too. I could live with a higher default priority for the
> > syscon-reboot driver, in fact I proposed that in the discussion of v1 of
> > that patch:
> > https://lkml.org/lkml/2014/11/28/484
> 
> Thanks for the link.
> 
> > IMHO, this priority might make sense for most cases, I guess that
> > dedicated "syscon" capabilities are usually better suited as a reboot
> > source than watchdog.
> 
> I would think likewise.
> 
> > If dt, then the question which arises: If there are different
> > capabilities to reset/reboot a whole system, how do we reflect which is
> > the best suited one in dt?
> 
> I'm not sure, but I don't think that exposing a priority variable in
> this way is the best, because it implicitly relies on what the kernel
> may have selected for other devices and/or FW interfaces, which may not
> have been described in DT.
> 
> So if we can get away with a fixed priority for now, then I would prefer
> that.
> 
> Otherwise, I would imagine that most systems have a single preferred
> mechanism with some possible fallback(s), for which a single
> preferred-poweroff property might suffice, and has better interaction
> w.r.t. priority (in that it should _always_ be tried first). Even that's
> difficult to reconcile with FW bindings though, especially EFI (which we
> sometimes must use in preference for variable storage and capsule
> updates).
> 
Hi mark,

reboot, not poweroff, but I like that idea; it is system independent,
and I agree that there should be only one "preferred-reboot" mechanism.
If we need more we can always add something like "fallback-reboot".

Not sure I understand the reference to EFI. Does EFI install a means
to restart the system ? Also, doesn't the proprity question apply even
if there is no dt property to establish the preference ?

Thanks,
Guenter
Mark Rutland Dec. 1, 2014, 5:59 p.m. UTC | #10
On Mon, Dec 01, 2014 at 05:51:34PM +0000, Guenter Roeck wrote:
> On Mon, Dec 01, 2014 at 05:41:00PM +0000, Mark Rutland wrote:
> > On Mon, Dec 01, 2014 at 05:24:37PM +0000, Stefan Agner wrote:
> > > On 2014-12-01 18:11, Mark Rutland wrote:
> > > > On Mon, Dec 01, 2014 at 05:03:07PM +0000, Stefan Agner wrote:
> > > >> This patch adds an optional property which allows to specify the
> > > >> reset source priority. This priority is used by the kernel restart
> > > >> handler call chain to sort out the proper reset/restart method.
> > > >> Depending on the power design of a board or other machine/board
> > > >> specific peculiarity, it is not possible to pick a generic priority.
> > > >>
> > > >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> > > >> ---
> > > >>  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
> > > >>  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
> > > >>  2 files changed, 7 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > > >> index 1190631..ee41d9c 100644
> > > >> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > > >> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > > >> @@ -11,6 +11,9 @@ Required properties:
> > > >>  - offset: offset in the register map for the reboot register (in bytes)
> > > >>  - mask: the reset value written to the reboot register (32 bit access)
> > > >>
> > > >> +Optional properties:
> > > >> +- priority: define the priority of the reset (0-255, defaults to 128)
> > > >> +
> > > > 
> > > > NAK. This is a leak of Linux-internal details.
> > > > 
> > > > What is this necessary for?
> > > > 
> > > > Mark.
> > > 
> > > Hi Mark,
> > > 
> > > In my case, it is necessary to be called ahead of the watchdog, which
> > > has a priority of 128. This syscon-reboot driver currently has a default
> > > priority of 128 too. I could live with a higher default priority for the
> > > syscon-reboot driver, in fact I proposed that in the discussion of v1 of
> > > that patch:
> > > https://lkml.org/lkml/2014/11/28/484
> > 
> > Thanks for the link.
> > 
> > > IMHO, this priority might make sense for most cases, I guess that
> > > dedicated "syscon" capabilities are usually better suited as a reboot
> > > source than watchdog.
> > 
> > I would think likewise.
> > 
> > > If dt, then the question which arises: If there are different
> > > capabilities to reset/reboot a whole system, how do we reflect which is
> > > the best suited one in dt?
> > 
> > I'm not sure, but I don't think that exposing a priority variable in
> > this way is the best, because it implicitly relies on what the kernel
> > may have selected for other devices and/or FW interfaces, which may not
> > have been described in DT.
> > 
> > So if we can get away with a fixed priority for now, then I would prefer
> > that.
> > 
> > Otherwise, I would imagine that most systems have a single preferred
> > mechanism with some possible fallback(s), for which a single
> > preferred-poweroff property might suffice, and has better interaction
> > w.r.t. priority (in that it should _always_ be tried first). Even that's
> > difficult to reconcile with FW bindings though, especially EFI (which we
> > sometimes must use in preference for variable storage and capsule
> > updates).
> > 
> Hi mark,
> 
> reboot, not poweroff, but I like that idea; it is system independent,
> and I agree that there should be only one "preferred-reboot" mechanism.
> If we need more we can always add something like "fallback-reboot".

Ah, sorry, I got myself confused regarding reboot/poweroff. Goot to hear
we agree on the preferred-$X (where X is reboot in this case).

> Not sure I understand the reference to EFI. Does EFI install a means
> to restart the system ?

Through EFI runtime services there's a mechanism to reset the machine.
In certain cases this must be called so as to allow EFI to set things up
to run after reboot (e.g. capsules for FW updates), while in other cases
it's perfectly fine to call other mechanisms directly.

> Also, doesn't the proprity question apply even if there is no dt
> property to establish the preference ?

Indeed.

My concern is having some priorities described in DT and some being
implicit in the kernel source, as the implicit priorities might change
independently of the explicit ones given in DT. At least with a single
preferred mechanism in DT we can force that to override all implicit
ones (though I'm not entirely sure how that would interact with the EFI
reboot case).

Thanks,
Mark.
Guenter Roeck Dec. 1, 2014, 6:12 p.m. UTC | #11
On Mon, Dec 01, 2014 at 05:59:08PM +0000, Mark Rutland wrote:
> On Mon, Dec 01, 2014 at 05:51:34PM +0000, Guenter Roeck wrote:
> > On Mon, Dec 01, 2014 at 05:41:00PM +0000, Mark Rutland wrote:
> > > On Mon, Dec 01, 2014 at 05:24:37PM +0000, Stefan Agner wrote:
> > > > On 2014-12-01 18:11, Mark Rutland wrote:
> > > > > On Mon, Dec 01, 2014 at 05:03:07PM +0000, Stefan Agner wrote:
> > > > >> This patch adds an optional property which allows to specify the
> > > > >> reset source priority. This priority is used by the kernel restart
> > > > >> handler call chain to sort out the proper reset/restart method.
> > > > >> Depending on the power design of a board or other machine/board
> > > > >> specific peculiarity, it is not possible to pick a generic priority.
> > > > >>
> > > > >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> > > > >> ---
> > > > >>  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
> > > > >>  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
> > > > >>  2 files changed, 7 insertions(+), 1 deletion(-)
> > > > >>
> > > > >> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > > > >> index 1190631..ee41d9c 100644
> > > > >> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > > > >> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > > > >> @@ -11,6 +11,9 @@ Required properties:
> > > > >>  - offset: offset in the register map for the reboot register (in bytes)
> > > > >>  - mask: the reset value written to the reboot register (32 bit access)
> > > > >>
> > > > >> +Optional properties:
> > > > >> +- priority: define the priority of the reset (0-255, defaults to 128)
> > > > >> +
> > > > > 
> > > > > NAK. This is a leak of Linux-internal details.
> > > > > 
> > > > > What is this necessary for?
> > > > > 
> > > > > Mark.
> > > > 
> > > > Hi Mark,
> > > > 
> > > > In my case, it is necessary to be called ahead of the watchdog, which
> > > > has a priority of 128. This syscon-reboot driver currently has a default
> > > > priority of 128 too. I could live with a higher default priority for the
> > > > syscon-reboot driver, in fact I proposed that in the discussion of v1 of
> > > > that patch:
> > > > https://lkml.org/lkml/2014/11/28/484
> > > 
> > > Thanks for the link.
> > > 
> > > > IMHO, this priority might make sense for most cases, I guess that
> > > > dedicated "syscon" capabilities are usually better suited as a reboot
> > > > source than watchdog.
> > > 
> > > I would think likewise.
> > > 
> > > > If dt, then the question which arises: If there are different
> > > > capabilities to reset/reboot a whole system, how do we reflect which is
> > > > the best suited one in dt?
> > > 
> > > I'm not sure, but I don't think that exposing a priority variable in
> > > this way is the best, because it implicitly relies on what the kernel
> > > may have selected for other devices and/or FW interfaces, which may not
> > > have been described in DT.
> > > 
> > > So if we can get away with a fixed priority for now, then I would prefer
> > > that.
> > > 
> > > Otherwise, I would imagine that most systems have a single preferred
> > > mechanism with some possible fallback(s), for which a single
> > > preferred-poweroff property might suffice, and has better interaction
> > > w.r.t. priority (in that it should _always_ be tried first). Even that's
> > > difficult to reconcile with FW bindings though, especially EFI (which we
> > > sometimes must use in preference for variable storage and capsule
> > > updates).
> > > 
> > Hi mark,
> > 
> > reboot, not poweroff, but I like that idea; it is system independent,
> > and I agree that there should be only one "preferred-reboot" mechanism.
> > If we need more we can always add something like "fallback-reboot".
> 
> Ah, sorry, I got myself confused regarding reboot/poweroff. Goot to hear
> we agree on the preferred-$X (where X is reboot in this case).
> 
In this context, note that the poweroff handler patchset is abandoned/dead.
Neither the power maintainers nor Linus liked the idea of having prioritized
poweroff handlers.

> > Not sure I understand the reference to EFI. Does EFI install a means
> > to restart the system ?
> 
> Through EFI runtime services there's a mechanism to reset the machine.
> In certain cases this must be called so as to allow EFI to set things up
> to run after reboot (e.g. capsules for FW updates), while in other cases
> it's perfectly fine to call other mechanisms directly.
> 
Sounds like its priority depends on the context. That might be a bit tricky,
but it might be possible if EFI uses the API to register its restart handler
and re-registers itself if needed. Would that make sense ?

Thanks,
Guenter
Mark Rutland Dec. 1, 2014, 6:18 p.m. UTC | #12
On Mon, Dec 01, 2014 at 06:12:55PM +0000, Guenter Roeck wrote:
> On Mon, Dec 01, 2014 at 05:59:08PM +0000, Mark Rutland wrote:
> > On Mon, Dec 01, 2014 at 05:51:34PM +0000, Guenter Roeck wrote:
> > > On Mon, Dec 01, 2014 at 05:41:00PM +0000, Mark Rutland wrote:
> > > > On Mon, Dec 01, 2014 at 05:24:37PM +0000, Stefan Agner wrote:
> > > > > On 2014-12-01 18:11, Mark Rutland wrote:
> > > > > > On Mon, Dec 01, 2014 at 05:03:07PM +0000, Stefan Agner wrote:
> > > > > >> This patch adds an optional property which allows to specify the
> > > > > >> reset source priority. This priority is used by the kernel restart
> > > > > >> handler call chain to sort out the proper reset/restart method.
> > > > > >> Depending on the power design of a board or other machine/board
> > > > > >> specific peculiarity, it is not possible to pick a generic priority.
> > > > > >>
> > > > > >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> > > > > >> ---
> > > > > >>  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
> > > > > >>  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
> > > > > >>  2 files changed, 7 insertions(+), 1 deletion(-)
> > > > > >>
> > > > > >> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > > > > >> index 1190631..ee41d9c 100644
> > > > > >> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > > > > >> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> > > > > >> @@ -11,6 +11,9 @@ Required properties:
> > > > > >>  - offset: offset in the register map for the reboot register (in bytes)
> > > > > >>  - mask: the reset value written to the reboot register (32 bit access)
> > > > > >>
> > > > > >> +Optional properties:
> > > > > >> +- priority: define the priority of the reset (0-255, defaults to 128)
> > > > > >> +
> > > > > > 
> > > > > > NAK. This is a leak of Linux-internal details.
> > > > > > 
> > > > > > What is this necessary for?
> > > > > > 
> > > > > > Mark.
> > > > > 
> > > > > Hi Mark,
> > > > > 
> > > > > In my case, it is necessary to be called ahead of the watchdog, which
> > > > > has a priority of 128. This syscon-reboot driver currently has a default
> > > > > priority of 128 too. I could live with a higher default priority for the
> > > > > syscon-reboot driver, in fact I proposed that in the discussion of v1 of
> > > > > that patch:
> > > > > https://lkml.org/lkml/2014/11/28/484
> > > > 
> > > > Thanks for the link.
> > > > 
> > > > > IMHO, this priority might make sense for most cases, I guess that
> > > > > dedicated "syscon" capabilities are usually better suited as a reboot
> > > > > source than watchdog.
> > > > 
> > > > I would think likewise.
> > > > 
> > > > > If dt, then the question which arises: If there are different
> > > > > capabilities to reset/reboot a whole system, how do we reflect which is
> > > > > the best suited one in dt?
> > > > 
> > > > I'm not sure, but I don't think that exposing a priority variable in
> > > > this way is the best, because it implicitly relies on what the kernel
> > > > may have selected for other devices and/or FW interfaces, which may not
> > > > have been described in DT.
> > > > 
> > > > So if we can get away with a fixed priority for now, then I would prefer
> > > > that.
> > > > 
> > > > Otherwise, I would imagine that most systems have a single preferred
> > > > mechanism with some possible fallback(s), for which a single
> > > > preferred-poweroff property might suffice, and has better interaction
> > > > w.r.t. priority (in that it should _always_ be tried first). Even that's
> > > > difficult to reconcile with FW bindings though, especially EFI (which we
> > > > sometimes must use in preference for variable storage and capsule
> > > > updates).
> > > > 
> > > Hi mark,
> > > 
> > > reboot, not poweroff, but I like that idea; it is system independent,
> > > and I agree that there should be only one "preferred-reboot" mechanism.
> > > If we need more we can always add something like "fallback-reboot".
> > 
> > Ah, sorry, I got myself confused regarding reboot/poweroff. Goot to hear
> > we agree on the preferred-$X (where X is reboot in this case).
> > 
> In this context, note that the poweroff handler patchset is abandoned/dead.
> Neither the power maintainers nor Linus liked the idea of having prioritized
> poweroff handlers.

I hadn't realised that either. Thanks for letting me know. :)

> > > Not sure I understand the reference to EFI. Does EFI install a means
> > > to restart the system ?
> > 
> > Through EFI runtime services there's a mechanism to reset the machine.
> > In certain cases this must be called so as to allow EFI to set things up
> > to run after reboot (e.g. capsules for FW updates), while in other cases
> > it's perfectly fine to call other mechanisms directly.
> > 
> Sounds like its priority depends on the context. That might be a bit tricky,
> but it might be possible if EFI uses the API to register its restart handler
> and re-registers itself if needed. Would that make sense ?

Sure, I would imagine the EFI code should know when an EFI-handled
reboot is necessary. It's not something I've looked into in great detail
hwoever, I'm just aware of the general requirement.

Thanks,
Mark.
Arnd Bergmann Dec. 1, 2014, 6:50 p.m. UTC | #13
On Monday 01 December 2014 17:41:00 Mark Rutland wrote:
> Otherwise, I would imagine that most systems have a single preferred
> mechanism with some possible fallback(s), for which a single
> preferred-poweroff property might suffice, and has better interaction
> w.r.t. priority (in that it should _always_ be tried first). Even that's
> difficult to reconcile with FW bindings though, especially EFI (which we
> sometimes must use in preference for variable storage and capsule
> updates).

The preferred-poweroff property sounds better to me, too. I can see
two ways of doing that though, and I'm not sure which one you mean.
Would you put that as a bool property into the device that does the reboot,
or would you put it as reference into the /chosen or /aliases node?

I think the latter would be better as it avoids any ambiguity, but
either way would work.

	Arnd
Stefan Agner Dec. 1, 2014, 10:18 p.m. UTC | #14
On 2014-12-01 18:42, Guenter Roeck wrote:
> On Mon, Dec 01, 2014 at 09:38:09AM -0800, Feng Kan wrote:
>> On Mon, Dec 1, 2014 at 9:29 AM, Stefan Agner <stefan@agner.ch> wrote:
>> > On 2014-12-01 18:15, Feng Kan wrote:
>> >> On Mon, Dec 1, 2014 at 9:03 AM, Stefan Agner <stefan@agner.ch> wrote:
>> >>> This patch adds an optional property which allows to specify the
>> >>> reset source priority. This priority is used by the kernel restart
>> >>> handler call chain to sort out the proper reset/restart method.
>> >>> Depending on the power design of a board or other machine/board
>> >>> specific peculiarity, it is not possible to pick a generic priority.
>> >>>
>> >>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> >>> ---
>> >>>  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
>> >>>  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
>> >>>  2 files changed, 7 insertions(+), 1 deletion(-)
>> >>>
>> >>> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> >>> index 1190631..ee41d9c 100644
>> >>> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> >>> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> >>> @@ -11,6 +11,9 @@ Required properties:
>> >>>  - offset: offset in the register map for the reboot register (in bytes)
>> >>>  - mask: the reset value written to the reboot register (32 bit access)
>> >>>
>> >>> +Optional properties:
>> >>> +- priority: define the priority of the reset (0-255, defaults to 128)
>> >>> +
>> >>>  Default will be little endian mode, 32 bit access only.
>> >>>
>> >>>  Examples:
>> >>> diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
>> >>> index 815b901..3060d6b 100644
>> >>> --- a/drivers/power/reset/syscon-reboot.c
>> >>> +++ b/drivers/power/reset/syscon-reboot.c
>> >>> @@ -67,8 +67,11 @@ static int syscon_reboot_probe(struct platform_device *pdev)
>> >>>         if (of_property_read_u32(pdev->dev.of_node, "mask", &ctx->mask))
>> >>>                 return -EINVAL;
>> >>>
>> >>> -       ctx->restart_handler.notifier_call = syscon_restart_handle;
>> >>>         ctx->restart_handler.priority = 128;
>> >>> +       of_property_read_u32(pdev->dev.of_node, "priority",
>> >>> +                            &ctx->restart_handler.priority);
>> >>
>> >> What is this for?
>> >
>> > What do you mean? The moved line "ctx->restart_handler.notifier_call =
>> > syscon_restart_handle;"? When one reads the diff, it looks like that
>> > line was moved, in fact I tried to keep the of_property_read function
>> > calls together. But I had to move the assignation of the default
>> > priority in front of restart_handler.priority. That's what is the
>> > outcome...
>>
>> I believe Guenter explained above already. Actually this help to solve one of
>> my problem. Thanks.
> 
> Since Mark doesn't seem to be happy with the idea of making the priority
> dt-configurable, the alternative might be to just set a higher priority for
> syscon triggered resets, as suggested by Stefan. Not as flexible, but it
> should be ok for most use cases.
> 

I read the other branch of this thread too, but IMHO, this would be the
best solution for now. It's a one line change and does the job. After
all, we have that priority for a reason, it doesn't makes sense having
all of them at a common default. As mentioned in an other reply, I think
it is especially reasonable to configure the syscon-reboot driver a bit
higher, since this driver is more likely to be used for a dedicated
reboot feature provided by the SoC, which usually is the preferred one.
If it happens one day that a SoC ends up with conflicting priorities or
other priority odds, we can add the DT support then...

What do you think, can I go with that for now? (Guenter, Feng, Mark...?)

--
Stefan
Guenter Roeck Dec. 1, 2014, 10:35 p.m. UTC | #15
On Mon, Dec 01, 2014 at 11:18:47PM +0100, Stefan Agner wrote:
> On 2014-12-01 18:42, Guenter Roeck wrote:
> > On Mon, Dec 01, 2014 at 09:38:09AM -0800, Feng Kan wrote:
> >> On Mon, Dec 1, 2014 at 9:29 AM, Stefan Agner <stefan@agner.ch> wrote:
> >> > On 2014-12-01 18:15, Feng Kan wrote:
> >> >> On Mon, Dec 1, 2014 at 9:03 AM, Stefan Agner <stefan@agner.ch> wrote:
> >> >>> This patch adds an optional property which allows to specify the
> >> >>> reset source priority. This priority is used by the kernel restart
> >> >>> handler call chain to sort out the proper reset/restart method.
> >> >>> Depending on the power design of a board or other machine/board
> >> >>> specific peculiarity, it is not possible to pick a generic priority.
> >> >>>
> >> >>> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> >>> ---
> >> >>>  Documentation/devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
> >> >>>  drivers/power/reset/syscon-reboot.c                             | 5 ++++-
> >> >>>  2 files changed, 7 insertions(+), 1 deletion(-)
> >> >>>
> >> >>> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> >>> index 1190631..ee41d9c 100644
> >> >>> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> >>> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> >>> @@ -11,6 +11,9 @@ Required properties:
> >> >>>  - offset: offset in the register map for the reboot register (in bytes)
> >> >>>  - mask: the reset value written to the reboot register (32 bit access)
> >> >>>
> >> >>> +Optional properties:
> >> >>> +- priority: define the priority of the reset (0-255, defaults to 128)
> >> >>> +
> >> >>>  Default will be little endian mode, 32 bit access only.
> >> >>>
> >> >>>  Examples:
> >> >>> diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
> >> >>> index 815b901..3060d6b 100644
> >> >>> --- a/drivers/power/reset/syscon-reboot.c
> >> >>> +++ b/drivers/power/reset/syscon-reboot.c
> >> >>> @@ -67,8 +67,11 @@ static int syscon_reboot_probe(struct platform_device *pdev)
> >> >>>         if (of_property_read_u32(pdev->dev.of_node, "mask", &ctx->mask))
> >> >>>                 return -EINVAL;
> >> >>>
> >> >>> -       ctx->restart_handler.notifier_call = syscon_restart_handle;
> >> >>>         ctx->restart_handler.priority = 128;
> >> >>> +       of_property_read_u32(pdev->dev.of_node, "priority",
> >> >>> +                            &ctx->restart_handler.priority);
> >> >>
> >> >> What is this for?
> >> >
> >> > What do you mean? The moved line "ctx->restart_handler.notifier_call =
> >> > syscon_restart_handle;"? When one reads the diff, it looks like that
> >> > line was moved, in fact I tried to keep the of_property_read function
> >> > calls together. But I had to move the assignation of the default
> >> > priority in front of restart_handler.priority. That's what is the
> >> > outcome...
> >>
> >> I believe Guenter explained above already. Actually this help to solve one of
> >> my problem. Thanks.
> > 
> > Since Mark doesn't seem to be happy with the idea of making the priority
> > dt-configurable, the alternative might be to just set a higher priority for
> > syscon triggered resets, as suggested by Stefan. Not as flexible, but it
> > should be ok for most use cases.
> > 
> 
> I read the other branch of this thread too, but IMHO, this would be the
> best solution for now. It's a one line change and does the job. After
> all, we have that priority for a reason, it doesn't makes sense having
> all of them at a common default. As mentioned in an other reply, I think
> it is especially reasonable to configure the syscon-reboot driver a bit
> higher, since this driver is more likely to be used for a dedicated
> reboot feature provided by the SoC, which usually is the preferred one.
> If it happens one day that a SoC ends up with conflicting priorities or
> other priority odds, we can add the DT support then...
> 
> What do you think, can I go with that for now? (Guenter, Feng, Mark...?)
> 
Ok with me. After all, one only needs to remove the entry from the dt file
if not wanted/needed, so one could argue that its existence already implies
"preferred".

Guenter
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
index 1190631..ee41d9c 100644
--- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
@@ -11,6 +11,9 @@  Required properties:
 - offset: offset in the register map for the reboot register (in bytes)
 - mask: the reset value written to the reboot register (32 bit access)
 
+Optional properties:
+- priority: define the priority of the reset (0-255, defaults to 128)
+
 Default will be little endian mode, 32 bit access only.
 
 Examples:
diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
index 815b901..3060d6b 100644
--- a/drivers/power/reset/syscon-reboot.c
+++ b/drivers/power/reset/syscon-reboot.c
@@ -67,8 +67,11 @@  static int syscon_reboot_probe(struct platform_device *pdev)
 	if (of_property_read_u32(pdev->dev.of_node, "mask", &ctx->mask))
 		return -EINVAL;
 
-	ctx->restart_handler.notifier_call = syscon_restart_handle;
 	ctx->restart_handler.priority = 128;
+	of_property_read_u32(pdev->dev.of_node, "priority",
+			     &ctx->restart_handler.priority);
+
+	ctx->restart_handler.notifier_call = syscon_restart_handle;
 	err = register_restart_handler(&ctx->restart_handler);
 	if (err)
 		dev_err(dev, "can't register restart notifier (err=%d)\n", err);