This second version does essentially the same as v1, but with a lot
less code due to the usage of syscon and syscon-reboot driver. My
earlier misgivings that it might be a problem for the suspend/resume
implementation later on did not hold true: I tested my prelinear
suspend code and in case the code really needs to access some
registers form assembler, it is possible to map temporarly the
relevant region again.
I refrained from adding POWER_RESET_SYSCON as a hard dependency for
Vybrid (SOC_VF610): In its dual-core configuration, one might want
have the Cortex-M4 in charge of the system reset... Hence the user
should be able to build a kernel without the system reset (beside
the option to create a board specific device tree with disabled
SRC node).
Shawn, the defconfig change is based on the latest (almost-merged?)
defconfig patch.
Changes since v1:
- Total rework using syscon/syscon-reboot capabilities
- Enhance syscon-reboot with priority capabilities (as suggested by Guenter)
Stefan Agner (3):
power: reset: read priority from device tree
ARM: dts: vf610: add system reset controller and syscon-reboot
ARM: imx_v6_v7_defconfig: add POWER_RESET_SYSCON
.../devicetree/bindings/power/reset/syscon-reboot.txt | 3 +++
arch/arm/boot/dts/vf500.dtsi | 4 ++++
arch/arm/boot/dts/vfxxx.dtsi | 13 +++++++++++++
arch/arm/configs/imx_v6_v7_defconfig | 8 ++++----
drivers/power/reset/syscon-reboot.c | 5 ++++-
5 files changed, 28 insertions(+), 5 deletions(-)
--
2.1.3
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 <[email protected]>
---
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);
+
+ 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
Add POWER_RESET_SYSCON since Vybrid SoC's now make use of this
driver to provide software reset capabilities through the SRC
module.
Also regenerated using savedefconfig which removed the config
BACKLIGHT_LCD_SUPPORT which is now selected by default since
commit 9c8ee3c734139 ("video: mx3fb: always enable
BACKLIGHT_LCD_SUPPORT").
Signed-off-by: Stefan Agner <[email protected]>
---
arch/arm/configs/imx_v6_v7_defconfig | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index f707cd2..e928075 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -163,13 +163,14 @@ CONFIG_SPI_IMX=y
CONFIG_GPIO_SYSFS=y
CONFIG_GPIO_MC9S08DZ60=y
CONFIG_GPIO_STMPE=y
+CONFIG_POWER_SUPPLY=y
+CONFIG_POWER_RESET=y
+CONFIG_POWER_RESET_IMX=y
+CONFIG_POWER_RESET_SYSCON=y
CONFIG_SENSORS_GPIO_FAN=y
CONFIG_THERMAL=y
CONFIG_CPU_THERMAL=y
CONFIG_IMX_THERMAL=y
-CONFIG_POWER_SUPPLY=y
-CONFIG_POWER_RESET=y
-CONFIG_POWER_RESET_IMX=y
CONFIG_WATCHDOG=y
CONFIG_IMX2_WDT=y
CONFIG_MFD_DA9052_I2C=y
@@ -198,7 +199,6 @@ CONFIG_SOC_CAMERA_OV2640=y
CONFIG_IMX_IPUV3_CORE=y
CONFIG_DRM=y
CONFIG_DRM_PANEL_SIMPLE=y
-CONFIG_BACKLIGHT_LCD_SUPPORT=y
CONFIG_LCD_CLASS_DEVICE=y
CONFIG_LCD_L4F00242T03=y
CONFIG_LCD_PLATFORM=y
--
2.1.3
Add the system reset controller (SRC) module and use syscon-reboot
to register a restart handler which restarts the SoC using the
SRC SW_RST bit.
Signed-off-by: Stefan Agner <[email protected]>
---
arch/arm/boot/dts/vf500.dtsi | 4 ++++
arch/arm/boot/dts/vfxxx.dtsi | 13 +++++++++++++
2 files changed, 17 insertions(+)
diff --git a/arch/arm/boot/dts/vf500.dtsi b/arch/arm/boot/dts/vf500.dtsi
index de67005..3f38f49 100644
--- a/arch/arm/boot/dts/vf500.dtsi
+++ b/arch/arm/boot/dts/vf500.dtsi
@@ -130,6 +130,10 @@
interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
};
+&src {
+ interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>;
+};
+
&uart0 {
interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
};
diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
index 505969a..3e696d7 100644
--- a/arch/arm/boot/dts/vfxxx.dtsi
+++ b/arch/arm/boot/dts/vfxxx.dtsi
@@ -43,6 +43,14 @@
clock-frequency = <32768>;
};
+ reboot: syscon-reboot {
+ compatible = "syscon-reboot";
+ regmap = <&src>;
+ offset = <0x0>;
+ mask = <0x1000>;
+ priority = <192>;
+ };
+
soc {
#address-cells = <1>;
#size-cells = <1>;
@@ -318,6 +326,11 @@
clocks = <&clks VF610_CLK_USBC0>;
status = "disabled";
};
+
+ src: src@4006e000 {
+ compatible = "fsl,vf610-src", "syscon";
+ reg = <0x4006e000 0x1000>;
+ };
};
aips1: aips-bus@40080000 {
--
2.1.3
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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Mon, Dec 1, 2014 at 9:03 AM, Stefan Agner <[email protected]> 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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> ---
>> 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 [email protected]
>> 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 <[email protected]>
> > ---
> > 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
On 2014-12-01 18:15, Feng Kan wrote:
> On Mon, Dec 1, 2014 at 9:03 AM, Stefan Agner <[email protected]> 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 <[email protected]>
>> ---
>> 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
>>
On Mon, Dec 1, 2014 at 9:29 AM, Stefan Agner <[email protected]> wrote:
> On 2014-12-01 18:15, Feng Kan wrote:
>> On Mon, Dec 1, 2014 at 9:03 AM, Stefan Agner <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
>>>
>
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 <[email protected]>
> >> ---
> >> 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.
On Mon, Dec 01, 2014 at 09:38:09AM -0800, Feng Kan wrote:
> On Mon, Dec 1, 2014 at 9:29 AM, Stefan Agner <[email protected]> wrote:
> > On 2014-12-01 18:15, Feng Kan wrote:
> >> On Mon, Dec 1, 2014 at 9:03 AM, Stefan Agner <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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
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 <[email protected]>
> > >> ---
> > >> 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
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 <[email protected]>
> > > >> ---
> > > >> 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.
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 <[email protected]>
> > > > >> ---
> > > > >> 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
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 <[email protected]>
> > > > > >> ---
> > > > > >> 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.
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
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 <[email protected]> wrote:
>> > On 2014-12-01 18:15, Feng Kan wrote:
>> >> On Mon, Dec 1, 2014 at 9:03 AM, Stefan Agner <[email protected]> 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 <[email protected]>
>> >>> ---
>> >>> 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
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 <[email protected]> wrote:
> >> > On 2014-12-01 18:15, Feng Kan wrote:
> >> >> On Mon, Dec 1, 2014 at 9:03 AM, Stefan Agner <[email protected]> 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 <[email protected]>
> >> >>> ---
> >> >>> 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