| Message ID | 1417453389-1588-2-git-send-email-stefan@agner.ch |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
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 > -- 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
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 > -- 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
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 -- 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
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 >> -- 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
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 >> -- 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
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 >>> > -- 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
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. -- 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
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 -- 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
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 -- 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
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. -- 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
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 -- 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
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. -- 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
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 -- 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
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 -- 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
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 -- 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
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);
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(-)