linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127
@ 2020-09-21  5:48 Qiang Zhao
  2020-09-21  5:48 ` [Patch v2 2/3] rtc: pcf2127: add "no-watchdog" property Qiang Zhao
                   ` (2 more replies)
  0 siblings, 3 replies; 39+ messages in thread
From: Qiang Zhao @ 2020-09-21  5:48 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni, robh+dt
  Cc: linux-rtc, devicetree, linux-kernel, linux-watchdog, Zhao Qiang

From: Zhao Qiang <qiang.zhao@nxp.com>

Add bindings for nxp,pcf2127

Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com>
---
Changes for v2:
 - modify the format to yaml
 - add compitable "nxp,pca2129"

 .../devicetree/bindings/rtc/nxp,pcf2127.yaml       | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml

diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml
new file mode 100644
index 0000000..226a0b2
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/nxp,pcf2127.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PCF RTCs
+
+maintainers:
+  - Qiang Zhao <qiang.zhao@nxp.com>
+
+allOf:
+  - $ref: "rtc.yaml#"
+
+properties:
+  compatible:
+    enum:
+      - nxp,pcf2127
+      - nxp,pcf2129
+      - nxp,pca2129
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  no-watchdog:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      With this property, the device will not registered as a watchdog device.
+
+  start-year: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+...
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [Patch v2 2/3] rtc: pcf2127: add "no-watchdog" property
  2020-09-21  5:48 [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Qiang Zhao
@ 2020-09-21  5:48 ` Qiang Zhao
  2020-09-21  5:48 ` [Patch v2 3/3] arm64: dts: lx2160a: Add "no-watchdog" property to pcf2127 Qiang Zhao
  2020-09-23  9:44 ` [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Alexandre Belloni
  2 siblings, 0 replies; 39+ messages in thread
From: Qiang Zhao @ 2020-09-21  5:48 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni, robh+dt
  Cc: linux-rtc, devicetree, linux-kernel, linux-watchdog, Zhao Qiang

From: Zhao Qiang <qiang.zhao@nxp.com>

Commit 0e735eaae165 ("rtc: pcf2127: add watchdog feature support")
adds watchdog feature for pcf2127, then it will be registered as
a watchdog device even though it is not used as watchdog.
Pcf2127 always be registered as watchdog0(default watchdog device),
even though it is not used as watchdog, meanwhile there is another
real watchdog device registered as watchdog1.
So add "no-watchdog" property, if the node has this property,
driver will not register it as a watchdog device

Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com>
---
 drivers/rtc/rtc-pcf2127.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index ed63169..b50158a 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -624,9 +624,11 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
 
 #ifdef CONFIG_WATCHDOG
-	ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
-	if (ret)
-		return ret;
+	if (!of_property_read_bool(dev->of_node, "no-watchdog")) {
+		ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
+		if (ret)
+			return ret;
+	}
 #endif /* CONFIG_WATCHDOG */
 
 	/*
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [Patch v2 3/3] arm64: dts: lx2160a: Add "no-watchdog" property to pcf2127
  2020-09-21  5:48 [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Qiang Zhao
  2020-09-21  5:48 ` [Patch v2 2/3] rtc: pcf2127: add "no-watchdog" property Qiang Zhao
@ 2020-09-21  5:48 ` Qiang Zhao
  2020-09-23  9:44 ` [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Alexandre Belloni
  2 siblings, 0 replies; 39+ messages in thread
From: Qiang Zhao @ 2020-09-21  5:48 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni, robh+dt
  Cc: linux-rtc, devicetree, linux-kernel, linux-watchdog, Zhao Qiang

From: Zhao Qiang <qiang.zhao@nxp.com>

pcf2127 on lx2160a are not used as watchdog.

Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com>
---
 arch/arm64/boot/dts/freescale/fsl-lx2160a-rdb.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-lx2160a-rdb.dts
index 54fe8cd..6181371 100644
--- a/arch/arm64/boot/dts/freescale/fsl-lx2160a-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-rdb.dts
@@ -153,6 +153,7 @@
 		reg = <0x51>;
 		// IRQ10_B
 		interrupts = <0 150 0x4>;
+		no-watchdog;
 	};
 };
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* Re: [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127
  2020-09-21  5:48 [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Qiang Zhao
  2020-09-21  5:48 ` [Patch v2 2/3] rtc: pcf2127: add "no-watchdog" property Qiang Zhao
  2020-09-21  5:48 ` [Patch v2 3/3] arm64: dts: lx2160a: Add "no-watchdog" property to pcf2127 Qiang Zhao
@ 2020-09-23  9:44 ` Alexandre Belloni
  2020-09-24  3:20   ` Qiang Zhao
  2 siblings, 1 reply; 39+ messages in thread
From: Alexandre Belloni @ 2020-09-23  9:44 UTC (permalink / raw)
  To: Qiang Zhao
  Cc: Wim Van Sebroeck, Guenter Roeck, linux-watchdog, a.zummo,
	robh+dt, linux-rtc, devicetree, linux-kernel,
	Uwe Kleine-König

Hi,

You forgot to copy the watchdog maintainers, I think such a property
should be discussed with them.

Note that I'm still convinced this is not a complete solution, see:
https://lore.kernel.org/linux-rtc/20200716181816.GF3428@piout.net/

On 21/09/2020 13:48:19+0800, Qiang Zhao wrote:
> From: Zhao Qiang <qiang.zhao@nxp.com>
> 
> Add bindings for nxp,pcf2127
> 
> Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com>
> ---
> Changes for v2:
>  - modify the format to yaml
>  - add compitable "nxp,pca2129"
> 
>  .../devicetree/bindings/rtc/nxp,pcf2127.yaml       | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml
> 
> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml
> new file mode 100644
> index 0000000..226a0b2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/nxp,pcf2127.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: PCF RTCs
> +
> +maintainers:
> +  - Qiang Zhao <qiang.zhao@nxp.com>
> +
> +allOf:
> +  - $ref: "rtc.yaml#"
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,pcf2127
> +      - nxp,pcf2129
> +      - nxp,pca2129
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  no-watchdog:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      With this property, the device will not registered as a watchdog device.
> +
> +  start-year: true
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +...
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127
  2020-09-23  9:44 ` [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Alexandre Belloni
@ 2020-09-24  3:20   ` Qiang Zhao
  2020-09-24  7:04     ` Uwe Kleine-König
  0 siblings, 1 reply; 39+ messages in thread
From: Qiang Zhao @ 2020-09-24  3:20 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Wim Van Sebroeck, Guenter Roeck, linux-watchdog, a.zummo,
	robh+dt, linux-rtc, devicetree, linux-kernel,
	Uwe Kleine-König

On 21/09/2020 13:48:19+0800, Qiang Zhao wrote:

> -----Original Message-----
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Sent: 2020年9月23日 17:45
> To: Qiang Zhao <qiang.zhao@nxp.com>
> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>; Guenter Roeck
> <linux@roeck-us.net>; linux-watchdog@vger.kernel.org;
> a.zummo@towertech.it; robh+dt@kernel.org; linux-rtc@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de>
> Subject: Re: [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for
> nxp,pcf2127
> 
> Hi,
> 
> You forgot to copy the watchdog maintainers, I think such a property should be
> discussed with them.
> 
> Note that I'm still convinced this is not a complete solution, see:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kern
> el.org%2Flinux-rtc%2F20200716181816.GF3428%40piout.net%2F&amp;data=
> 02%7C01%7Cqiang.zhao%40nxp.com%7Cb71f79a044b0493d6d4f08d85fa551c
> b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637364510931174
> 355&amp;sdata=%2BOxrzB8RIuxM9Let5slhfCVmMm6PMNoERDeHC9%2Fdxng
> %3D&amp;reserved=0
> 

Yes, you are right, There is not a fundamental solution.
However it somewhat avoid this situation at least.

And if without this issue, 
is it correct to register a rtc device as watchdog no matter it is used as watchdog on the board? 
Every time Linux are booted up, watchdog device should be configured to the right one manually.
So the patch are useful, even though it is not for the issue.

What should we do to really resolve this issue?

Best Regards
Qiang Zhao

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127
  2020-09-24  3:20   ` Qiang Zhao
@ 2020-09-24  7:04     ` Uwe Kleine-König
  2020-09-24  7:23       ` Qiang Zhao
  0 siblings, 1 reply; 39+ messages in thread
From: Uwe Kleine-König @ 2020-09-24  7:04 UTC (permalink / raw)
  To: Qiang Zhao
  Cc: Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
	linux-watchdog, a.zummo, robh+dt, linux-rtc, devicetree,
	linux-kernel, kernel

[-- Attachment #1: Type: text/plain, Size: 2484 bytes --]

Hello,

On Thu, Sep 24, 2020 at 03:20:33AM +0000, Qiang Zhao wrote:
> On 21/09/2020 13:48:19+0800, Qiang Zhao wrote:
> 
> > -----Original Message-----
> > From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Sent: 2020年9月23日 17:45
> > To: Qiang Zhao <qiang.zhao@nxp.com>
> > Cc: Wim Van Sebroeck <wim@linux-watchdog.org>; Guenter Roeck
> > <linux@roeck-us.net>; linux-watchdog@vger.kernel.org;
> > a.zummo@towertech.it; robh+dt@kernel.org; linux-rtc@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de>
> > Subject: Re: [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for
> > nxp,pcf2127
> > 
> > Hi,
> > 
> > You forgot to copy the watchdog maintainers, I think such a property should be
> > discussed with them.
> > 
> > Note that I'm still convinced this is not a complete solution, see:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kern
> > el.org%2Flinux-rtc%2F20200716181816.GF3428%40piout.net%2F&amp;data=
> > 02%7C01%7Cqiang.zhao%40nxp.com%7Cb71f79a044b0493d6d4f08d85fa551c
> > b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637364510931174
> > 355&amp;sdata=%2BOxrzB8RIuxM9Let5slhfCVmMm6PMNoERDeHC9%2Fdxng
> > %3D&amp;reserved=0

haha

> Yes, you are right, There is not a fundamental solution.
> However it somewhat avoid this situation at least.
> 
> And if without this issue, 
> is it correct to register a rtc device as watchdog no matter it is used as watchdog on the board? 
> Every time Linux are booted up, watchdog device should be configured to the right one manually.
> So the patch are useful, even though it is not for the issue.
> 
> What should we do to really resolve this issue?

I still think we need a kernel solution here. I would expect that most
assembled pcf2127 chips are unable to act as a watchdog (i.e. don't have
the RST output connected to something that resets the machine).

So my favoured solution would be a positive property like:

	has-watchdog;

or something similar. In my eyes this is definitely something we want to
specify in the device tree because it is a relevant hardware property.
I consider it a bug to give a watchdog device to userspace that isn't
functional.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127
  2020-09-24  7:04     ` Uwe Kleine-König
@ 2020-09-24  7:23       ` Qiang Zhao
  2020-09-24  7:47         ` Alexandre Belloni
  0 siblings, 1 reply; 39+ messages in thread
From: Qiang Zhao @ 2020-09-24  7:23 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
	linux-watchdog, a.zummo, robh+dt, linux-rtc, devicetree,
	linux-kernel, kernel

On Thu, Sep 24, 2020 at 15:05AM +0000, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 2020年9月24日 15:05
> To: Qiang Zhao <qiang.zhao@nxp.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>; Wim Van Sebroeck
> <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>;
> linux-watchdog@vger.kernel.org; a.zummo@towertech.it; robh+dt@kernel.org;
> linux-rtc@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de
> Subject: Re: [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for
> nxp,pcf2127
> 
> Hello,
> 
> On Thu, Sep 24, 2020 at 03:20:33AM +0000, Qiang Zhao wrote:
> > On 21/09/2020 13:48:19+0800, Qiang Zhao wrote:
> >
> > > -----Original Message-----
> > > From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > > Sent: 2020年9月23日 17:45
> > > To: Qiang Zhao <qiang.zhao@nxp.com>
> > > Cc: Wim Van Sebroeck <wim@linux-watchdog.org>; Guenter Roeck
> > > <linux@roeck-us.net>; linux-watchdog@vger.kernel.org;
> > > a.zummo@towertech.it; robh+dt@kernel.org; linux-rtc@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Uwe
> > > Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Subject: Re: [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings
> > > for
> > > nxp,pcf2127
> > >
> > > Hi,
> > >
> > > You forgot to copy the watchdog maintainers, I think such a property
> > > should be discussed with them.
> > >
> > > Note that I'm still convinced this is not a complete solution, see:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> > > re.kern
> > >
> el.org%2Flinux-rtc%2F20200716181816.GF3428%40piout.net%2F&amp;data=
> > >
> 02%7C01%7Cqiang.zhao%40nxp.com%7Cb71f79a044b0493d6d4f08d85fa551c
> > >
> b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637364510931174
> > >
> 355&amp;sdata=%2BOxrzB8RIuxM9Let5slhfCVmMm6PMNoERDeHC9%2Fdxng
> > > %3D&amp;reserved=0
> 
> haha
> 
> > Yes, you are right, There is not a fundamental solution.
> > However it somewhat avoid this situation at least.
> >
> > And if without this issue,
> > is it correct to register a rtc device as watchdog no matter it is used as
> watchdog on the board?
> > Every time Linux are booted up, watchdog device should be configured to the
> right one manually.
> > So the patch are useful, even though it is not for the issue.
> >
> > What should we do to really resolve this issue?
> 
> I still think we need a kernel solution here. I would expect that most assembled
> pcf2127 chips are unable to act as a watchdog (i.e. don't have the RST output
> connected to something that resets the machine).
> 
> So my favoured solution would be a positive property like:
> 
> 	has-watchdog;
> 
> or something similar. In my eyes this is definitely something we want to specify
> in the device tree because it is a relevant hardware property.
> I consider it a bug to give a watchdog device to userspace that isn't functional.
> 
> Best regards
> Uwe
 
I strongly agree with you! It should be positive property.
However, we couldn't identify which board are using pcf2127 as watchdog,
So we are unable to modify the boards' dts to correct (watchdog or not) in this patchset.

I noticed that only LS series platforms and imx6 have pcf2127 node, as far as I know, the LS platforms don't use it as watchdog,
But I am not sure about imx6

> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Best Regards
Qiang Zhao

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127
  2020-09-24  7:23       ` Qiang Zhao
@ 2020-09-24  7:47         ` Alexandre Belloni
  2020-09-24 10:52           ` [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
  2020-09-24 16:59           ` [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Bruno Thomsen
  0 siblings, 2 replies; 39+ messages in thread
From: Alexandre Belloni @ 2020-09-24  7:47 UTC (permalink / raw)
  To: Qiang Zhao, Bruno Thomsen
  Cc: Uwe Kleine-König, Wim Van Sebroeck, Guenter Roeck,
	linux-watchdog, a.zummo, robh+dt, linux-rtc, devicetree,
	linux-kernel, kernel

Hi,

On 24/09/2020 07:23:18+0000, Qiang Zhao wrote:
> > > Yes, you are right, There is not a fundamental solution.
> > > However it somewhat avoid this situation at least.
> > >
> > > And if without this issue,
> > > is it correct to register a rtc device as watchdog no matter it is used as
> > watchdog on the board?
> > > Every time Linux are booted up, watchdog device should be configured to the
> > right one manually.
> > > So the patch are useful, even though it is not for the issue.
> > >
> > > What should we do to really resolve this issue?
> > 
> > I still think we need a kernel solution here. I would expect that most assembled
> > pcf2127 chips are unable to act as a watchdog (i.e. don't have the RST output
> > connected to something that resets the machine).
> > 
> > So my favoured solution would be a positive property like:
> > 
> > 	has-watchdog;
> > 
> > or something similar. In my eyes this is definitely something we want to specify
> > in the device tree because it is a relevant hardware property.
> > I consider it a bug to give a watchdog device to userspace that isn't functional.
> > 
> > Best regards
> > Uwe
>  
> I strongly agree with you! It should be positive property.
> However, we couldn't identify which board are using pcf2127 as watchdog,
> So we are unable to modify the boards' dts to correct (watchdog or not) in this patchset.
> 
> I noticed that only LS series platforms and imx6 have pcf2127 node, as far as I know, the LS platforms don't use it as watchdog,
> But I am not sure about imx6
> 

I don't think there is any user upstream and it is recent engouh that we
can probably make that a positive property.

Bruno, is it ok for you? you are the only know user of the feature.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
  2020-09-24  7:47         ` Alexandre Belloni
@ 2020-09-24 10:52           ` Uwe Kleine-König
  2020-09-24 10:52             ` [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function Uwe Kleine-König
                               ` (3 more replies)
  2020-09-24 16:59           ` [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Bruno Thomsen
  1 sibling, 4 replies; 39+ messages in thread
From: Uwe Kleine-König @ 2020-09-24 10:52 UTC (permalink / raw)
  To: Alexandre Belloni, Qiang Zhao, Bruno Thomsen
  Cc: linux-rtc, a.zummo, linux-watchdog, devicetree, linux-kernel,
	robh+dt, kernel, Wim Van Sebroeck, Guenter Roeck

Hello,

now that there are two people stumbling over the pcf2127 driver
providing a non-functional watchdog device, here comes an RFC patch to
address this.

Note this is only compile tested and dt-documentation is still missing.
Still send this series because the cleanup is nice independent of this
discussion and to have something to argue about.

Does someone can offer a better name than "has-watchdog", is there a
scheme that could be used already that I'm not aware of?

Best regards
Uwe

Uwe Kleine-König (2):
  rtc: pcf2127: move watchdog initialisation to a separate function
  [RFC] rtc: pcf2127: only use watchdog when explicitly available

 drivers/rtc/rtc-pcf2127.c | 57 ++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 25 deletions(-)

-- 
2.28.0


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function
  2020-09-24 10:52           ` [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
@ 2020-09-24 10:52             ` Uwe Kleine-König
  2020-09-27  7:50               ` Bruno Thomsen
  2020-09-24 10:52             ` [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
                               ` (2 subsequent siblings)
  3 siblings, 1 reply; 39+ messages in thread
From: Uwe Kleine-König @ 2020-09-24 10:52 UTC (permalink / raw)
  To: Alexandre Belloni, Qiang Zhao, Bruno Thomsen
  Cc: linux-rtc, a.zummo, linux-watchdog, devicetree, linux-kernel,
	robh+dt, kernel, Wim Van Sebroeck, Guenter Roeck

The obvious advantages are:

 - The linker can drop the watchdog functions if CONFIG_WATCHDOG is off.
 - All watchdog stuff grouped together with only a single function call
   left in generic code.
 - Watchdog register is only read when it is actually used.
 - Less #ifdefery

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/rtc/rtc-pcf2127.c | 56 ++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index ed6316992cbb..5b1f1949b5e5 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -335,6 +335,36 @@ static const struct watchdog_ops pcf2127_watchdog_ops = {
 	.set_timeout = pcf2127_wdt_set_timeout,
 };
 
+static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
+{
+	u32 wdd_timeout;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_WATCHDOG))
+		return 0;
+
+	pcf2127->wdd.parent = dev;
+	pcf2127->wdd.info = &pcf2127_wdt_info;
+	pcf2127->wdd.ops = &pcf2127_watchdog_ops;
+	pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
+	pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
+	pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
+	pcf2127->wdd.min_hw_heartbeat_ms = 500;
+	pcf2127->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
+
+	watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
+
+	/* Test if watchdog timer is started by bootloader */
+	ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
+	if (ret)
+		return ret;
+
+	if (wdd_timeout)
+		set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
+
+	return devm_watchdog_register_device(dev, &pcf2127->wdd);
+}
+
 /* Alarm */
 static int pcf2127_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
@@ -536,7 +566,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 			 int alarm_irq, const char *name, bool has_nvmem)
 {
 	struct pcf2127 *pcf2127;
-	u32 wdd_timeout;
 	int ret = 0;
 
 	dev_dbg(dev, "%s\n", __func__);
@@ -575,17 +604,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		pcf2127->rtc->ops = &pcf2127_rtc_alrm_ops;
 	}
 
-	pcf2127->wdd.parent = dev;
-	pcf2127->wdd.info = &pcf2127_wdt_info;
-	pcf2127->wdd.ops = &pcf2127_watchdog_ops;
-	pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
-	pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
-	pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
-	pcf2127->wdd.min_hw_heartbeat_ms = 500;
-	pcf2127->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
-
-	watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
-
 	if (has_nvmem) {
 		struct nvmem_config nvmem_cfg = {
 			.priv = pcf2127,
@@ -615,19 +633,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		return ret;
 	}
 
-	/* Test if watchdog timer is started by bootloader */
-	ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
-	if (ret)
-		return ret;
-
-	if (wdd_timeout)
-		set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
-
-#ifdef CONFIG_WATCHDOG
-	ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
-	if (ret)
-		return ret;
-#endif /* CONFIG_WATCHDOG */
+	pcf2127_watchdog_init(dev, pcf2127);
 
 	/*
 	 * Disable battery low/switch-over timestamp and interrupts.
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available
  2020-09-24 10:52           ` [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
  2020-09-24 10:52             ` [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function Uwe Kleine-König
@ 2020-09-24 10:52             ` Uwe Kleine-König
  2020-09-27  8:09               ` Bruno Thomsen
  2020-10-26  7:23             ` [PATCH 0/2] " Qiang Zhao
  2020-11-30  9:51             ` Rasmus Villemoes
  3 siblings, 1 reply; 39+ messages in thread
From: Uwe Kleine-König @ 2020-09-24 10:52 UTC (permalink / raw)
  To: Alexandre Belloni, Qiang Zhao, Bruno Thomsen
  Cc: linux-rtc, a.zummo, linux-watchdog, devicetree, linux-kernel,
	robh+dt, kernel, Wim Van Sebroeck, Guenter Roeck

Most boards using the pcf2127 chip (in my bubble) don't make use of the
watchdog functionality and the respective output is not connected. The
effect on such a board is that there is a watchdog device provided that
doesn't work.

So only register the watchdog if the device tree has a "has-watchdog"
property.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/rtc/rtc-pcf2127.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 5b1f1949b5e5..8bd89d641578 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -340,7 +340,8 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
 	u32 wdd_timeout;
 	int ret;
 
-	if (!IS_ENABLED(CONFIG_WATCHDOG))
+	if (!IS_ENABLED(CONFIG_WATCHDOG) ||
+	    !device_property_read_bool(dev, "has-watchdog"))
 		return 0;
 
 	pcf2127->wdd.parent = dev;
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* Re: [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127
  2020-09-24  7:47         ` Alexandre Belloni
  2020-09-24 10:52           ` [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
@ 2020-09-24 16:59           ` Bruno Thomsen
  1 sibling, 0 replies; 39+ messages in thread
From: Bruno Thomsen @ 2020-09-24 16:59 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Qiang Zhao, Uwe Kleine-König, Wim Van Sebroeck,
	Guenter Roeck, linux-watchdog, a.zummo, robh+dt, linux-rtc,
	devicetree, linux-kernel, kernel

Den tor. 24. sep. 2020 kl. 09.47 skrev Alexandre Belloni
<alexandre.belloni@bootlin.com>:
>
> Hi,
>
> On 24/09/2020 07:23:18+0000, Qiang Zhao wrote:
> > > > Yes, you are right, There is not a fundamental solution.
> > > > However it somewhat avoid this situation at least.
> > > >
> > > > And if without this issue,
> > > > is it correct to register a rtc device as watchdog no matter it is used as
> > > watchdog on the board?
> > > > Every time Linux are booted up, watchdog device should be configured to the
> > > right one manually.
> > > > So the patch are useful, even though it is not for the issue.
> > > >
> > > > What should we do to really resolve this issue?
> > >
> > > I still think we need a kernel solution here. I would expect that most assembled
> > > pcf2127 chips are unable to act as a watchdog (i.e. don't have the RST output
> > > connected to something that resets the machine).
> > >
> > > So my favoured solution would be a positive property like:
> > >
> > >     has-watchdog;
> > >
> > > or something similar. In my eyes this is definitely something we want to specify
> > > in the device tree because it is a relevant hardware property.
> > > I consider it a bug to give a watchdog device to userspace that isn't functional.
> > >
> > > Best regards
> > > Uwe
> >
> > I strongly agree with you! It should be positive property.
> > However, we couldn't identify which board are using pcf2127 as watchdog,
> > So we are unable to modify the boards' dts to correct (watchdog or not) in this patchset.
> >
> > I noticed that only LS series platforms and imx6 have pcf2127 node, as far as I know, the LS platforms don't use it as watchdog,
> > But I am not sure about imx6
> >
>
> I don't think there is any user upstream and it is recent engouh that we
> can probably make that a positive property.
>
> Bruno, is it ok for you? you are the only know user of the feature.

Hi

This seems like an okay solution to me.

I have a patch series on the way with a new product dts[1] that is
using the watchdog
feature in the pcf2127 chip.

I know that the watchdog feature is used by other products with
out-of-tree dts, but I
will make sure to give them a heads up.

/Bruno

[1] https://lore.kernel.org/linux-arm-kernel/20200923154024.11417-2-bruno.thomsen@gmail.com/

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function
  2020-09-24 10:52             ` [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function Uwe Kleine-König
@ 2020-09-27  7:50               ` Bruno Thomsen
  0 siblings, 0 replies; 39+ messages in thread
From: Bruno Thomsen @ 2020-09-27  7:50 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Alexandre Belloni, Qiang Zhao, linux-rtc, Alessandro Zummo,
	linux-watchdog,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, Rob Herring, Sascha Hauer, Wim Van Sebroeck,
	Guenter Roeck

Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
<u.kleine-koenig@pengutronix.de>:
>
> The obvious advantages are:
>
>  - The linker can drop the watchdog functions if CONFIG_WATCHDOG is off.
>  - All watchdog stuff grouped together with only a single function call
>    left in generic code.
>  - Watchdog register is only read when it is actually used.
>  - Less #ifdefery
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/rtc/rtc-pcf2127.c | 56 ++++++++++++++++++++++-----------------
>  1 file changed, 31 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index ed6316992cbb..5b1f1949b5e5 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -335,6 +335,36 @@ static const struct watchdog_ops pcf2127_watchdog_ops = {
>         .set_timeout = pcf2127_wdt_set_timeout,
>  };
>
> +static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
> +{
> +       u32 wdd_timeout;
> +       int ret;
> +
> +       if (!IS_ENABLED(CONFIG_WATCHDOG))
> +               return 0;
> +
> +       pcf2127->wdd.parent = dev;
> +       pcf2127->wdd.info = &pcf2127_wdt_info;
> +       pcf2127->wdd.ops = &pcf2127_watchdog_ops;
> +       pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
> +       pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
> +       pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
> +       pcf2127->wdd.min_hw_heartbeat_ms = 500;
> +       pcf2127->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> +
> +       watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
> +
> +       /* Test if watchdog timer is started by bootloader */
> +       ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
> +       if (ret)
> +               return ret;
> +
> +       if (wdd_timeout)
> +               set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
> +
> +       return devm_watchdog_register_device(dev, &pcf2127->wdd);
> +}
> +
>  /* Alarm */
>  static int pcf2127_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  {
> @@ -536,7 +566,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>                          int alarm_irq, const char *name, bool has_nvmem)
>  {
>         struct pcf2127 *pcf2127;
> -       u32 wdd_timeout;
>         int ret = 0;
>
>         dev_dbg(dev, "%s\n", __func__);
> @@ -575,17 +604,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>                 pcf2127->rtc->ops = &pcf2127_rtc_alrm_ops;
>         }
>
> -       pcf2127->wdd.parent = dev;
> -       pcf2127->wdd.info = &pcf2127_wdt_info;
> -       pcf2127->wdd.ops = &pcf2127_watchdog_ops;
> -       pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
> -       pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
> -       pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
> -       pcf2127->wdd.min_hw_heartbeat_ms = 500;
> -       pcf2127->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> -
> -       watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
> -
>         if (has_nvmem) {
>                 struct nvmem_config nvmem_cfg = {
>                         .priv = pcf2127,
> @@ -615,19 +633,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>                 return ret;
>         }
>
> -       /* Test if watchdog timer is started by bootloader */
> -       ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
> -       if (ret)
> -               return ret;
> -
> -       if (wdd_timeout)
> -               set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
> -
> -#ifdef CONFIG_WATCHDOG
> -       ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
> -       if (ret)
> -               return ret;
> -#endif /* CONFIG_WATCHDOG */
> +       pcf2127_watchdog_init(dev, pcf2127);

The code refactoring seems like a good idea Uwe, but the new
pcf2127_watchdog_init() is not a void function. If the return
value is not handled, it will change driver behavior. Correct
handling should look like this:

ret = pcf2127_watchdog_init(dev, pcf2127);
if (ret)
        return ret;

/Bruno

>         /*
>          * Disable battery low/switch-over timestamp and interrupts.
> --
> 2.28.0
>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available
  2020-09-24 10:52             ` [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
@ 2020-09-27  8:09               ` Bruno Thomsen
  2020-09-27 15:54                 ` Guenter Roeck
  0 siblings, 1 reply; 39+ messages in thread
From: Bruno Thomsen @ 2020-09-27  8:09 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Alexandre Belloni, Qiang Zhao, linux-rtc, Alessandro Zummo,
	linux-watchdog,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, Rob Herring, Sascha Hauer, Wim Van Sebroeck,
	Guenter Roeck

Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
<u.kleine-koenig@pengutronix.de>:
>
> Most boards using the pcf2127 chip (in my bubble) don't make use of the
> watchdog functionality and the respective output is not connected. The
> effect on such a board is that there is a watchdog device provided that
> doesn't work.
>
> So only register the watchdog if the device tree has a "has-watchdog"
> property.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/rtc/rtc-pcf2127.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 5b1f1949b5e5..8bd89d641578 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -340,7 +340,8 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
>         u32 wdd_timeout;
>         int ret;
>
> -       if (!IS_ENABLED(CONFIG_WATCHDOG))
> +       if (!IS_ENABLED(CONFIG_WATCHDOG) ||
> +           !device_property_read_bool(dev, "has-watchdog"))
>                 return 0;

I don't think the compiler can remove the function if
CONFIG_WATCHDOG is disabled due to the device tree
value check. Maybe it can if split into 2 conditions.

if (!IS_ENABLED(CONFIG_WATCHDOG))
        return 0;
if (!device_property_read_bool(dev, "has-watchdog"))
        return 0;

/Bruno

>
>         pcf2127->wdd.parent = dev;
> --
> 2.28.0
>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available
  2020-09-27  8:09               ` Bruno Thomsen
@ 2020-09-27 15:54                 ` Guenter Roeck
  2020-09-28  8:43                   ` Uwe Kleine-König
  0 siblings, 1 reply; 39+ messages in thread
From: Guenter Roeck @ 2020-09-27 15:54 UTC (permalink / raw)
  To: Bruno Thomsen, Uwe Kleine-König
  Cc: Alexandre Belloni, Qiang Zhao, linux-rtc, Alessandro Zummo,
	linux-watchdog,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, Rob Herring, Sascha Hauer, Wim Van Sebroeck

On 9/27/20 1:09 AM, Bruno Thomsen wrote:
> Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de>:
>>
>> Most boards using the pcf2127 chip (in my bubble) don't make use of the
>> watchdog functionality and the respective output is not connected. The
>> effect on such a board is that there is a watchdog device provided that
>> doesn't work.
>>
>> So only register the watchdog if the device tree has a "has-watchdog"
>> property.
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>>  drivers/rtc/rtc-pcf2127.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>> index 5b1f1949b5e5..8bd89d641578 100644
>> --- a/drivers/rtc/rtc-pcf2127.c
>> +++ b/drivers/rtc/rtc-pcf2127.c
>> @@ -340,7 +340,8 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
>>         u32 wdd_timeout;
>>         int ret;
>>
>> -       if (!IS_ENABLED(CONFIG_WATCHDOG))
>> +       if (!IS_ENABLED(CONFIG_WATCHDOG) ||
>> +           !device_property_read_bool(dev, "has-watchdog"))
>>                 return 0;
> 
> I don't think the compiler can remove the function if
> CONFIG_WATCHDOG is disabled due to the device tree
> value check. Maybe it can if split into 2 conditions.
> 

If the first part of the expression is always false, the second
part should not even be evaluated. Either case, the code now
hard depends on the compiler optimizing the code away.
It calls devm_watchdog_register_device() which doesn't exist
if CONFIG_WATCHDOG is not enabled. I didn't know that this is safe,
and I would personally not want to rely on it, but we live and
learn.

Guenter

> if (!IS_ENABLED(CONFIG_WATCHDOG))
>         return 0;
> if (!device_property_read_bool(dev, "has-watchdog"))
>         return 0;
> 
> /Bruno
> 
>>
>>         pcf2127->wdd.parent = dev;
>> --
>> 2.28.0
>>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available
  2020-09-27 15:54                 ` Guenter Roeck
@ 2020-09-28  8:43                   ` Uwe Kleine-König
  2020-09-28 16:26                     ` Guenter Roeck
  0 siblings, 1 reply; 39+ messages in thread
From: Uwe Kleine-König @ 2020-09-28  8:43 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Bruno Thomsen, linux-rtc, Alessandro Zummo, Alexandre Belloni,
	linux-watchdog,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, Rob Herring, Sascha Hauer, Wim Van Sebroeck,
	Qiang Zhao

[-- Attachment #1: Type: text/plain, Size: 2502 bytes --]

On Sun, Sep 27, 2020 at 08:54:47AM -0700, Guenter Roeck wrote:
> On 9/27/20 1:09 AM, Bruno Thomsen wrote:
> > Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de>:
> >>
> >> Most boards using the pcf2127 chip (in my bubble) don't make use of the
> >> watchdog functionality and the respective output is not connected. The
> >> effect on such a board is that there is a watchdog device provided that
> >> doesn't work.
> >>
> >> So only register the watchdog if the device tree has a "has-watchdog"
> >> property.
> >>
> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >> ---
> >>  drivers/rtc/rtc-pcf2127.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> >> index 5b1f1949b5e5..8bd89d641578 100644
> >> --- a/drivers/rtc/rtc-pcf2127.c
> >> +++ b/drivers/rtc/rtc-pcf2127.c
> >> @@ -340,7 +340,8 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
> >>         u32 wdd_timeout;
> >>         int ret;
> >>
> >> -       if (!IS_ENABLED(CONFIG_WATCHDOG))
> >> +       if (!IS_ENABLED(CONFIG_WATCHDOG) ||
> >> +           !device_property_read_bool(dev, "has-watchdog"))
> >>                 return 0;
> > 
> > I don't think the compiler can remove the function if
> > CONFIG_WATCHDOG is disabled due to the device tree
> > value check. Maybe it can if split into 2 conditions.
> > 
> 
> If the first part of the expression is always false, the second
> part should not even be evaluated.

This is wrong. For || the second expression isn't evaluated if the first
evaluates to true (and the whole expression becomes true). This is the
intended behaviour: If CONFIG_WATCHDOG is off, we don't need to check
for the dt property and just skip the watchdog part.

> Either case, the code now hard depends on the compiler optimizing the
> code away.
> 
> It calls devm_watchdog_register_device() which doesn't exist
> if CONFIG_WATCHDOG is not enabled. I didn't know that this is safe,
> and I would personally not want to rely on it, but we live and
> learn.

AFAICT this is save and used in other places in the kernel, too.  This
is one of the reasons why you cannot compile the kernel with -O0.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available
  2020-09-28  8:43                   ` Uwe Kleine-König
@ 2020-09-28 16:26                     ` Guenter Roeck
  0 siblings, 0 replies; 39+ messages in thread
From: Guenter Roeck @ 2020-09-28 16:26 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Bruno Thomsen, linux-rtc, Alessandro Zummo, Alexandre Belloni,
	linux-watchdog,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, Rob Herring, Sascha Hauer, Wim Van Sebroeck,
	Qiang Zhao

On Mon, Sep 28, 2020 at 10:43:43AM +0200, Uwe Kleine-König wrote:
> On Sun, Sep 27, 2020 at 08:54:47AM -0700, Guenter Roeck wrote:
> > On 9/27/20 1:09 AM, Bruno Thomsen wrote:
> > > Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de>:
> > >>
> > >> Most boards using the pcf2127 chip (in my bubble) don't make use of the
> > >> watchdog functionality and the respective output is not connected. The
> > >> effect on such a board is that there is a watchdog device provided that
> > >> doesn't work.
> > >>
> > >> So only register the watchdog if the device tree has a "has-watchdog"
> > >> property.
> > >>
> > >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > >> ---
> > >>  drivers/rtc/rtc-pcf2127.c | 3 ++-
> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > >> index 5b1f1949b5e5..8bd89d641578 100644
> > >> --- a/drivers/rtc/rtc-pcf2127.c
> > >> +++ b/drivers/rtc/rtc-pcf2127.c
> > >> @@ -340,7 +340,8 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
> > >>         u32 wdd_timeout;
> > >>         int ret;
> > >>
> > >> -       if (!IS_ENABLED(CONFIG_WATCHDOG))
> > >> +       if (!IS_ENABLED(CONFIG_WATCHDOG) ||
> > >> +           !device_property_read_bool(dev, "has-watchdog"))
> > >>                 return 0;
> > > 
> > > I don't think the compiler can remove the function if
> > > CONFIG_WATCHDOG is disabled due to the device tree
> > > value check. Maybe it can if split into 2 conditions.
> > > 
> > 
> > If the first part of the expression is always false, the second
> > part should not even be evaluated.
> 
> This is wrong. For || the second expression isn't evaluated if the first
> evaluates to true (and the whole expression becomes true). This is the
> intended behaviour: If CONFIG_WATCHDOG is off, we don't need to check
> for the dt property and just skip the watchdog part.
> 
Sorry, I meant to say "If the first part of the expression is always true".

Guenter

> > Either case, the code now hard depends on the compiler optimizing the
> > code away.
> > 
> > It calls devm_watchdog_register_device() which doesn't exist
> > if CONFIG_WATCHDOG is not enabled. I didn't know that this is safe,
> > and I would personally not want to rely on it, but we live and
> > learn.
> 
> AFAICT this is save and used in other places in the kernel, too.  This
> is one of the reasons why you cannot compile the kernel with -O0.
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |



^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
  2020-09-24 10:52           ` [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
  2020-09-24 10:52             ` [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function Uwe Kleine-König
  2020-09-24 10:52             ` [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
@ 2020-10-26  7:23             ` Qiang Zhao
  2020-10-26 20:48               ` Alexandre Belloni
  2020-11-30  9:51             ` Rasmus Villemoes
  3 siblings, 1 reply; 39+ messages in thread
From: Qiang Zhao @ 2020-10-26  7:23 UTC (permalink / raw)
  To: Uwe Kleine-König, Alexandre Belloni, Bruno Thomsen
  Cc: linux-rtc, a.zummo, linux-watchdog, devicetree, linux-kernel,
	robh+dt, kernel, Wim Van Sebroeck, Guenter Roeck

Any update for this patchset?

Best Regards
Qiang Zhao

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 2020年9月24日 18:53
> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Qiang Zhao
> <qiang.zhao@nxp.com>; Bruno Thomsen <bruno.thomsen@gmail.com>
> Cc: linux-rtc@vger.kernel.org; a.zummo@towertech.it;
> linux-watchdog@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; robh+dt@kernel.org; kernel@pengutronix.de;
> Wim Van Sebroeck <wim@linux-watchdog.org>; Guenter Roeck
> <linux@roeck-us.net>
> Subject: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
> 
> Hello,
> 
> now that there are two people stumbling over the pcf2127 driver providing a
> non-functional watchdog device, here comes an RFC patch to address this.
> 
> Note this is only compile tested and dt-documentation is still missing.
> Still send this series because the cleanup is nice independent of this discussion
> and to have something to argue about.
> 
> Does someone can offer a better name than "has-watchdog", is there a scheme
> that could be used already that I'm not aware of?
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (2):
>   rtc: pcf2127: move watchdog initialisation to a separate function
>   [RFC] rtc: pcf2127: only use watchdog when explicitly available
> 
>  drivers/rtc/rtc-pcf2127.c | 57 ++++++++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 25 deletions(-)
> 
> --
> 2.28.0


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
  2020-10-26  7:23             ` [PATCH 0/2] " Qiang Zhao
@ 2020-10-26 20:48               ` Alexandre Belloni
  2020-10-26 21:29                 ` Guenter Roeck
  0 siblings, 1 reply; 39+ messages in thread
From: Alexandre Belloni @ 2020-10-26 20:48 UTC (permalink / raw)
  To: Qiang Zhao
  Cc: Uwe Kleine-König, Bruno Thomsen, linux-rtc, a.zummo,
	linux-watchdog, devicetree, linux-kernel, robh+dt, kernel,
	Wim Van Sebroeck, Guenter Roeck

On 26/10/2020 07:23:26+0000, Qiang Zhao wrote:
> Any update for this patchset?
> 

The whole point would be to get the DT and the watchdog maintainers
agree on the property name. Once done, the driver implementation is
trivial and will get applied.

> Best Regards
> Qiang Zhao
> 
> > -----Original Message-----
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Sent: 2020年9月24日 18:53
> > To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Qiang Zhao
> > <qiang.zhao@nxp.com>; Bruno Thomsen <bruno.thomsen@gmail.com>
> > Cc: linux-rtc@vger.kernel.org; a.zummo@towertech.it;
> > linux-watchdog@vger.kernel.org; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org; robh+dt@kernel.org; kernel@pengutronix.de;
> > Wim Van Sebroeck <wim@linux-watchdog.org>; Guenter Roeck
> > <linux@roeck-us.net>
> > Subject: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
> > 
> > Hello,
> > 
> > now that there are two people stumbling over the pcf2127 driver providing a
> > non-functional watchdog device, here comes an RFC patch to address this.
> > 
> > Note this is only compile tested and dt-documentation is still missing.
> > Still send this series because the cleanup is nice independent of this discussion
> > and to have something to argue about.
> > 
> > Does someone can offer a better name than "has-watchdog", is there a scheme
> > that could be used already that I'm not aware of?
> > 
> > Best regards
> > Uwe
> > 
> > Uwe Kleine-König (2):
> >   rtc: pcf2127: move watchdog initialisation to a separate function
> >   [RFC] rtc: pcf2127: only use watchdog when explicitly available
> > 
> >  drivers/rtc/rtc-pcf2127.c | 57 ++++++++++++++++++++++-----------------
> >  1 file changed, 32 insertions(+), 25 deletions(-)
> > 
> > --
> > 2.28.0
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
  2020-10-26 20:48               ` Alexandre Belloni
@ 2020-10-26 21:29                 ` Guenter Roeck
  0 siblings, 0 replies; 39+ messages in thread
From: Guenter Roeck @ 2020-10-26 21:29 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Qiang Zhao, Uwe Kleine-König, Bruno Thomsen, linux-rtc,
	a.zummo, linux-watchdog, devicetree, linux-kernel, robh+dt,
	kernel, Wim Van Sebroeck

On Mon, Oct 26, 2020 at 09:48:11PM +0100, Alexandre Belloni wrote:
> On 26/10/2020 07:23:26+0000, Qiang Zhao wrote:
> > Any update for this patchset?
> > 
> 
> The whole point would be to get the DT and the watchdog maintainers
> agree on the property name. Once done, the driver implementation is
> trivial and will get applied.
> 

DT maintainers make that decision.

Guenter

> > Best Regards
> > Qiang Zhao
> > 
> > > -----Original Message-----
> > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Sent: 2020年9月24日 18:53
> > > To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Qiang Zhao
> > > <qiang.zhao@nxp.com>; Bruno Thomsen <bruno.thomsen@gmail.com>
> > > Cc: linux-rtc@vger.kernel.org; a.zummo@towertech.it;
> > > linux-watchdog@vger.kernel.org; devicetree@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; robh+dt@kernel.org; kernel@pengutronix.de;
> > > Wim Van Sebroeck <wim@linux-watchdog.org>; Guenter Roeck
> > > <linux@roeck-us.net>
> > > Subject: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
> > > 
> > > Hello,
> > > 
> > > now that there are two people stumbling over the pcf2127 driver providing a
> > > non-functional watchdog device, here comes an RFC patch to address this.
> > > 
> > > Note this is only compile tested and dt-documentation is still missing.
> > > Still send this series because the cleanup is nice independent of this discussion
> > > and to have something to argue about.
> > > 
> > > Does someone can offer a better name than "has-watchdog", is there a scheme
> > > that could be used already that I'm not aware of?
> > > 
> > > Best regards
> > > Uwe
> > > 
> > > Uwe Kleine-König (2):
> > >   rtc: pcf2127: move watchdog initialisation to a separate function
> > >   [RFC] rtc: pcf2127: only use watchdog when explicitly available
> > > 
> > >  drivers/rtc/rtc-pcf2127.c | 57 ++++++++++++++++++++++-----------------
> > >  1 file changed, 32 insertions(+), 25 deletions(-)
> > > 
> > > --
> > > 2.28.0
> > 
> 
> -- 
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
  2020-09-24 10:52           ` [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
                               ` (2 preceding siblings ...)
  2020-10-26  7:23             ` [PATCH 0/2] " Qiang Zhao
@ 2020-11-30  9:51             ` Rasmus Villemoes
  2020-12-04  9:27               ` Alexandre Belloni
  3 siblings, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2020-11-30  9:51 UTC (permalink / raw)
  To: Uwe Kleine-König, Alexandre Belloni, Qiang Zhao, Bruno Thomsen
  Cc: linux-rtc, a.zummo, linux-watchdog, devicetree, linux-kernel,
	robh+dt, kernel, Wim Van Sebroeck, Guenter Roeck

On 24/09/2020 12.52, Uwe Kleine-König wrote:
> Hello,
> 
> now that there are two people stumbling over the pcf2127 driver
> providing a non-functional watchdog device, here comes an RFC patch to
> address this.

I just want to add a "me too" here, as I'm also now affected by the
pcf2127 exposing a watchdog device (1) it didn't use to (affecting what
/dev/watchdog0 means) and (2) is not actually hooked up in hardware.

So can we please move forward with adding the has-watchdog opt-in DT
property so existing boards will not be affected?

Thanks,
Rasmus

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
  2020-11-30  9:51             ` Rasmus Villemoes
@ 2020-12-04  9:27               ` Alexandre Belloni
  2020-12-11 21:56                 ` [PATCH v2 0/3] add reset-source RTC binding, update pcf2127 driver Rasmus Villemoes
  0 siblings, 1 reply; 39+ messages in thread
From: Alexandre Belloni @ 2020-12-04  9:27 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Uwe Kleine-König, Qiang Zhao, Bruno Thomsen, linux-rtc,
	a.zummo, linux-watchdog, devicetree, linux-kernel, robh+dt,
	kernel, Wim Van Sebroeck, Guenter Roeck

On 30/11/2020 10:51:41+0100, Rasmus Villemoes wrote:
> On 24/09/2020 12.52, Uwe Kleine-König wrote:
> > Hello,
> > 
> > now that there are two people stumbling over the pcf2127 driver
> > providing a non-functional watchdog device, here comes an RFC patch to
> > address this.
> 
> I just want to add a "me too" here, as I'm also now affected by the
> pcf2127 exposing a watchdog device (1) it didn't use to (affecting what
> /dev/watchdog0 means) and (2) is not actually hooked up in hardware.
> 
> So can we please move forward with adding the has-watchdog opt-in DT
> property so existing boards will not be affected?
> 

Well, someone has to get that property reviewed by Rob. Maybe this could
be reset-source as we have wakeup-source.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 0/3] add reset-source RTC binding, update pcf2127 driver
  2020-12-04  9:27               ` Alexandre Belloni
@ 2020-12-11 21:56                 ` Rasmus Villemoes
  2020-12-11 21:56                   ` [PATCH v2 1/3] dt-bindings: rtc: add reset-source property Rasmus Villemoes
                                     ` (3 more replies)
  0 siblings, 4 replies; 39+ messages in thread
From: Rasmus Villemoes @ 2020-12-11 21:56 UTC (permalink / raw)
  To: linux-rtc, Alexandre Belloni
  Cc: linux-kernel, Rob Herring, Uwe Kleine-König, Qiang Zhao,
	Bruno Thomsen, Rasmus Villemoes

This is a resend of Uwe's pcf2127 patches, prepended by a dt-binding
patch adding a reset-source property, as suggested by Alexandre.

Rasmus Villemoes (1):
  dt-bindings: rtc: add reset-source property

Uwe Kleine-König (2):
  rtc: pcf2127: move watchdog initialisation to a separate function
  rtc: pcf2127: only use watchdog when explicitly available

 .../devicetree/bindings/rtc/rtc.yaml          |  5 ++
 drivers/rtc/rtc-pcf2127.c                     | 55 +++++++++++--------
 2 files changed, 37 insertions(+), 23 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 1/3] dt-bindings: rtc: add reset-source property
  2020-12-11 21:56                 ` [PATCH v2 0/3] add reset-source RTC binding, update pcf2127 driver Rasmus Villemoes
@ 2020-12-11 21:56                   ` Rasmus Villemoes
  2020-12-11 22:30                     ` Rob Herring
  2020-12-12  9:25                     ` Uwe Kleine-König
  2020-12-11 21:56                   ` [PATCH v2 2/3] rtc: pcf2127: move watchdog initialisation to a separate function Rasmus Villemoes
                                     ` (2 subsequent siblings)
  3 siblings, 2 replies; 39+ messages in thread
From: Rasmus Villemoes @ 2020-12-11 21:56 UTC (permalink / raw)
  To: linux-rtc, Alexandre Belloni
  Cc: linux-kernel, Rob Herring, Uwe Kleine-König, Qiang Zhao,
	Bruno Thomsen, Rasmus Villemoes

Some RTCs, e.g. the pcf2127, can be used as a hardware watchdog. But
if the reset pin is not actually wired up, the driver exposes a
watchdog device that doesn't actually work.

Provide a standard binding that can be used to indicate that a given
RTC can perform a reset of the machine, similar to wakeup-source.

Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 Documentation/devicetree/bindings/rtc/rtc.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
index ee237b2ed66a..b5e24dc24b0b 100644
--- a/Documentation/devicetree/bindings/rtc/rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
@@ -47,4 +47,9 @@ properties:
     description:
       Enables wake up of host system on alarm.
 
+  reset-source:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      The RTC is able to reset the machine.
+
 ...
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 2/3] rtc: pcf2127: move watchdog initialisation to a separate function
  2020-12-11 21:56                 ` [PATCH v2 0/3] add reset-source RTC binding, update pcf2127 driver Rasmus Villemoes
  2020-12-11 21:56                   ` [PATCH v2 1/3] dt-bindings: rtc: add reset-source property Rasmus Villemoes
@ 2020-12-11 21:56                   ` Rasmus Villemoes
  2020-12-11 21:56                   ` [PATCH v2 3/3] rtc: pcf2127: only use watchdog when explicitly available Rasmus Villemoes
  2020-12-18 10:10                   ` [PATCH v3 0/2] add reset-source RTC binding, update pcf2127 driver Rasmus Villemoes
  3 siblings, 0 replies; 39+ messages in thread
From: Rasmus Villemoes @ 2020-12-11 21:56 UTC (permalink / raw)
  To: linux-rtc, Alexandre Belloni
  Cc: linux-kernel, Rob Herring, Uwe Kleine-König, Qiang Zhao,
	Bruno Thomsen, Rasmus Villemoes

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

The obvious advantages are:

 - The compiler can drop the watchdog functions if CONFIG_WATCHDOG is off.
 - All watchdog stuff grouped together with only a single function call
   left in generic code.
 - Watchdog register is only read when it is actually used.
 - Less #ifdefery

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
[RV: check return value of pcf2127_watchdog_init(), per Bruno]
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/rtc-pcf2127.c | 54 ++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 07a5630ec841..aebf85fcc328 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -335,6 +335,36 @@ static const struct watchdog_ops pcf2127_watchdog_ops = {
 	.set_timeout = pcf2127_wdt_set_timeout,
 };
 
+static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
+{
+	u32 wdd_timeout;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_WATCHDOG))
+		return 0;
+
+	pcf2127->wdd.parent = dev;
+	pcf2127->wdd.info = &pcf2127_wdt_info;
+	pcf2127->wdd.ops = &pcf2127_watchdog_ops;
+	pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
+	pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
+	pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
+	pcf2127->wdd.min_hw_heartbeat_ms = 500;
+	pcf2127->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
+
+	watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
+
+	/* Test if watchdog timer is started by bootloader */
+	ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
+	if (ret)
+		return ret;
+
+	if (wdd_timeout)
+		set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
+
+	return devm_watchdog_register_device(dev, &pcf2127->wdd);
+}
+
 /* Alarm */
 static int pcf2127_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
@@ -536,7 +566,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 			 int alarm_irq, const char *name, bool has_nvmem)
 {
 	struct pcf2127 *pcf2127;
-	u32 wdd_timeout;
 	int ret = 0;
 
 	dev_dbg(dev, "%s\n", __func__);
@@ -575,17 +604,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		pcf2127->rtc->ops = &pcf2127_rtc_alrm_ops;
 	}
 
-	pcf2127->wdd.parent = dev;
-	pcf2127->wdd.info = &pcf2127_wdt_info;
-	pcf2127->wdd.ops = &pcf2127_watchdog_ops;
-	pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
-	pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
-	pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
-	pcf2127->wdd.min_hw_heartbeat_ms = 500;
-	pcf2127->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
-
-	watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
-
 	if (has_nvmem) {
 		struct nvmem_config nvmem_cfg = {
 			.priv = pcf2127,
@@ -615,19 +633,9 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		return ret;
 	}
 
-	/* Test if watchdog timer is started by bootloader */
-	ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
-	if (ret)
-		return ret;
-
-	if (wdd_timeout)
-		set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
-
-#ifdef CONFIG_WATCHDOG
-	ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
+	ret = pcf2127_watchdog_init(dev, pcf2127);
 	if (ret)
 		return ret;
-#endif /* CONFIG_WATCHDOG */
 
 	/*
 	 * Disable battery low/switch-over timestamp and interrupts.
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 3/3] rtc: pcf2127: only use watchdog when explicitly available
  2020-12-11 21:56                 ` [PATCH v2 0/3] add reset-source RTC binding, update pcf2127 driver Rasmus Villemoes
  2020-12-11 21:56                   ` [PATCH v2 1/3] dt-bindings: rtc: add reset-source property Rasmus Villemoes
  2020-12-11 21:56                   ` [PATCH v2 2/3] rtc: pcf2127: move watchdog initialisation to a separate function Rasmus Villemoes
@ 2020-12-11 21:56                   ` Rasmus Villemoes
  2020-12-18 10:10                   ` [PATCH v3 0/2] add reset-source RTC binding, update pcf2127 driver Rasmus Villemoes
  3 siblings, 0 replies; 39+ messages in thread
From: Rasmus Villemoes @ 2020-12-11 21:56 UTC (permalink / raw)
  To: linux-rtc, Alexandre Belloni
  Cc: linux-kernel, Rob Herring, Uwe Kleine-König, Qiang Zhao,
	Bruno Thomsen, Rasmus Villemoes

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Most boards using the pcf2127 chip (in my bubble) don't make use of the
watchdog functionality and the respective output is not connected. The
effect on such a board is that there is a watchdog device provided that
doesn't work.

So only register the watchdog if the device tree has a "reset-source"
property.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
[RV: s/has-watchdog/reset-source/]
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/rtc-pcf2127.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index aebf85fcc328..f3d4eac4f230 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -340,7 +340,8 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
 	u32 wdd_timeout;
 	int ret;
 
-	if (!IS_ENABLED(CONFIG_WATCHDOG))
+	if (!IS_ENABLED(CONFIG_WATCHDOG) ||
+	    !device_property_read_bool(dev, "reset-source"))
 		return 0;
 
 	pcf2127->wdd.parent = dev;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/3] dt-bindings: rtc: add reset-source property
  2020-12-11 21:56                   ` [PATCH v2 1/3] dt-bindings: rtc: add reset-source property Rasmus Villemoes
@ 2020-12-11 22:30                     ` Rob Herring
  2020-12-11 23:10                       ` Rasmus Villemoes
  2020-12-12  9:25                     ` Uwe Kleine-König
  1 sibling, 1 reply; 39+ messages in thread
From: Rob Herring @ 2020-12-11 22:30 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: open list:REAL TIME CLOCK (RTC) SUBSYSTEM, Alexandre Belloni,
	linux-kernel, Uwe Kleine-König, Qiang Zhao, Bruno Thomsen

On Fri, Dec 11, 2020 at 3:56 PM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Some RTCs, e.g. the pcf2127, can be used as a hardware watchdog. But
> if the reset pin is not actually wired up, the driver exposes a
> watchdog device that doesn't actually work.
>
> Provide a standard binding that can be used to indicate that a given
> RTC can perform a reset of the machine, similar to wakeup-source.

Why not use the watchdog 'timeout-sec' property?

Please send DT patches to the DT list.

>
> Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  Documentation/devicetree/bindings/rtc/rtc.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
> index ee237b2ed66a..b5e24dc24b0b 100644
> --- a/Documentation/devicetree/bindings/rtc/rtc.yaml
> +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
> @@ -47,4 +47,9 @@ properties:
>      description:
>        Enables wake up of host system on alarm.
>
> +  reset-source:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      The RTC is able to reset the machine.
> +
>  ...
> --
> 2.23.0
>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/3] dt-bindings: rtc: add reset-source property
  2020-12-11 22:30                     ` Rob Herring
@ 2020-12-11 23:10                       ` Rasmus Villemoes
  2020-12-17 16:51                         ` Rob Herring
  0 siblings, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2020-12-11 23:10 UTC (permalink / raw)
  To: Rob Herring
  Cc: open list:REAL TIME CLOCK (RTC) SUBSYSTEM, Alexandre Belloni,
	linux-kernel, Uwe Kleine-König, Qiang Zhao, Bruno Thomsen

On 11/12/2020 23.30, Rob Herring wrote:
> On Fri, Dec 11, 2020 at 3:56 PM Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> Some RTCs, e.g. the pcf2127, can be used as a hardware watchdog. But
>> if the reset pin is not actually wired up, the driver exposes a
>> watchdog device that doesn't actually work.
>>
>> Provide a standard binding that can be used to indicate that a given
>> RTC can perform a reset of the machine, similar to wakeup-source.
> 
> Why not use the watchdog 'timeout-sec' property?

Wouldn't that be overloading that property? AFAIU, that is used to ask
the kernel to program an initial timeout value into the watchdog device.
But what if one doesn't want to start the watchdog device at kernel
boot, but just indicate that the RTC has that capability?

It's quite possible that if it can act as a watchdog device (and
has-watchdog was also suggested), one would also want timeout-sec and
other watchdog bindings to apply. But that can be added later, by those
who actually want that.

For now, I'd really like to get my board booting again (or rather, not
get reset by the real watchdog just because the pcf2127 driver now
exposes something as /dev/wathdog0, pushing the real one to
/dev/wathcdog1 which doesn't get pinged from userspace).

Rasmus

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/3] dt-bindings: rtc: add reset-source property
  2020-12-11 21:56                   ` [PATCH v2 1/3] dt-bindings: rtc: add reset-source property Rasmus Villemoes
  2020-12-11 22:30                     ` Rob Herring
@ 2020-12-12  9:25                     ` Uwe Kleine-König
  1 sibling, 0 replies; 39+ messages in thread
From: Uwe Kleine-König @ 2020-12-12  9:25 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: linux-rtc, Alexandre Belloni, linux-kernel, Rob Herring,
	Qiang Zhao, Bruno Thomsen

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

On Fri, Dec 11, 2020 at 10:56:09PM +0100, Rasmus Villemoes wrote:
> Some RTCs, e.g. the pcf2127, can be used as a hardware watchdog. But
> if the reset pin is not actually wired up, the driver exposes a
> watchdog device that doesn't actually work.
> 
> Provide a standard binding that can be used to indicate that a given
> RTC can perform a reset of the machine, similar to wakeup-source.
> 
> Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  Documentation/devicetree/bindings/rtc/rtc.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
> index ee237b2ed66a..b5e24dc24b0b 100644
> --- a/Documentation/devicetree/bindings/rtc/rtc.yaml
> +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
> @@ -47,4 +47,9 @@ properties:
>      description:
>        Enables wake up of host system on alarm.
>  
> +  reset-source:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      The RTC is able to reset the machine.

Thanks for picking that up.

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Note that the patch "rtc: pcf2127: move watchdog initialisation to a
separate function" is already in next.

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/3] dt-bindings: rtc: add reset-source property
  2020-12-11 23:10                       ` Rasmus Villemoes
@ 2020-12-17 16:51                         ` Rob Herring
  2020-12-17 18:12                           ` Uwe Kleine-König
  0 siblings, 1 reply; 39+ messages in thread
From: Rob Herring @ 2020-12-17 16:51 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: open list:REAL TIME CLOCK (RTC) SUBSYSTEM, Alexandre Belloni,
	linux-kernel, Uwe Kleine-König, Qiang Zhao, Bruno Thomsen

On Fri, Dec 11, 2020 at 5:10 PM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 11/12/2020 23.30, Rob Herring wrote:
> > On Fri, Dec 11, 2020 at 3:56 PM Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> Some RTCs, e.g. the pcf2127, can be used as a hardware watchdog. But
> >> if the reset pin is not actually wired up, the driver exposes a
> >> watchdog device that doesn't actually work.
> >>
> >> Provide a standard binding that can be used to indicate that a given
> >> RTC can perform a reset of the machine, similar to wakeup-source.
> >
> > Why not use the watchdog 'timeout-sec' property?
>
> Wouldn't that be overloading that property? AFAIU, that is used to ask
> the kernel to program an initial timeout value into the watchdog device.
> But what if one doesn't want to start the watchdog device at kernel
> boot, but just indicate that the RTC has that capability?

Yeah, I guess you're right.

> It's quite possible that if it can act as a watchdog device (and
> has-watchdog was also suggested), one would also want timeout-sec and
> other watchdog bindings to apply. But that can be added later, by those
> who actually want that.
>
> For now, I'd really like to get my board booting again (or rather, not
> get reset by the real watchdog just because the pcf2127 driver now
> exposes something as /dev/wathdog0, pushing the real one to
> /dev/wathcdog1 which doesn't get pinged from userspace).

I'm wondering how you solve which wdog to ping when there are multiple
without relying on numbering. I guess 'reset-source' will solve that
even if that's not your current fix. So I guess I'm fine with this.

But you need to send to the DT list so checks are run.

Rob

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/3] dt-bindings: rtc: add reset-source property
  2020-12-17 16:51                         ` Rob Herring
@ 2020-12-17 18:12                           ` Uwe Kleine-König
  2020-12-17 18:23                             ` Rasmus Villemoes
  2020-12-17 19:02                             ` Rob Herring
  0 siblings, 2 replies; 39+ messages in thread
From: Uwe Kleine-König @ 2020-12-17 18:12 UTC (permalink / raw)
  To: Rob Herring
  Cc: Rasmus Villemoes, open list:REAL TIME CLOCK (RTC) SUBSYSTEM,
	Alexandre Belloni, linux-kernel, Qiang Zhao, Bruno Thomsen

[-- Attachment #1: Type: text/plain, Size: 2154 bytes --]

On Thu, Dec 17, 2020 at 10:51:08AM -0600, Rob Herring wrote:
> On Fri, Dec 11, 2020 at 5:10 PM Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
> >
> > On 11/12/2020 23.30, Rob Herring wrote:
> > > On Fri, Dec 11, 2020 at 3:56 PM Rasmus Villemoes
> > > <rasmus.villemoes@prevas.dk> wrote:
> > >>
> > >> Some RTCs, e.g. the pcf2127, can be used as a hardware watchdog. But
> > >> if the reset pin is not actually wired up, the driver exposes a
> > >> watchdog device that doesn't actually work.
> > >>
> > >> Provide a standard binding that can be used to indicate that a given
> > >> RTC can perform a reset of the machine, similar to wakeup-source.
> > >
> > > Why not use the watchdog 'timeout-sec' property?
> >
> > Wouldn't that be overloading that property? AFAIU, that is used to ask
> > the kernel to program an initial timeout value into the watchdog device.
> > But what if one doesn't want to start the watchdog device at kernel
> > boot, but just indicate that the RTC has that capability?
> 
> Yeah, I guess you're right.

I agree, too. The initial suggestion looks fine.

> > It's quite possible that if it can act as a watchdog device (and
> > has-watchdog was also suggested), one would also want timeout-sec and
> > other watchdog bindings to apply. But that can be added later, by those
> > who actually want that.
> >
> > For now, I'd really like to get my board booting again (or rather, not
> > get reset by the real watchdog just because the pcf2127 driver now
> > exposes something as /dev/wathdog0, pushing the real one to
> > /dev/wathcdog1 which doesn't get pinged from userspace).
> 
> I'm wondering how you solve which wdog to ping when there are multiple
> without relying on numbering. I guess 'reset-source' will solve that
> even if that's not your current fix. So I guess I'm fine with this.

I guess you'd need some udev magic that ensures that the right watchdog
always gets the same number.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/3] dt-bindings: rtc: add reset-source property
  2020-12-17 18:12                           ` Uwe Kleine-König
@ 2020-12-17 18:23                             ` Rasmus Villemoes
  2020-12-17 19:02                             ` Rob Herring
  1 sibling, 0 replies; 39+ messages in thread
From: Rasmus Villemoes @ 2020-12-17 18:23 UTC (permalink / raw)
  To: Uwe Kleine-König, Rob Herring
  Cc: open list:REAL TIME CLOCK (RTC) SUBSYSTEM, Alexandre Belloni,
	linux-kernel, Qiang Zhao, Bruno Thomsen

On 17/12/2020 19.12, Uwe Kleine-König wrote:
> On Thu, Dec 17, 2020 at 10:51:08AM -0600, Rob Herring wrote:
>> On Fri, Dec 11, 2020 at 5:10 PM Rasmus Villemoes
>> <rasmus.villemoes@prevas.dk> wrote:

>> I'm wondering how you solve which wdog to ping when there are multiple
>> without relying on numbering. I guess 'reset-source' will solve that
>> even if that's not your current fix. So I guess I'm fine with this.
> 
> I guess you'd need some udev magic that ensures that the right watchdog
> always gets the same number.

The thing is, my board only has one watchdog, and my userspace opens
/dev/watchdog (aka /dev/watchdog0). But when a driver suddenly exposes
another watchdog device [which happens to be non-functional], and that
just happens (probe order, link order, whatever) to be the first one
defined, things break.

Yes, in the general case one would need some udev rules to create
appropriate symlinks. But when there's only one actual device, and I
know that, using /dev/watchdog or /dev/watchdog0 is fine.

I'll resend cc'ing the dt mailing list, and taking into account the
stuff already in -next.

Thanks,
Rasmus


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/3] dt-bindings: rtc: add reset-source property
  2020-12-17 18:12                           ` Uwe Kleine-König
  2020-12-17 18:23                             ` Rasmus Villemoes
@ 2020-12-17 19:02                             ` Rob Herring
  2020-12-17 20:28                               ` Uwe Kleine-König
  1 sibling, 1 reply; 39+ messages in thread
From: Rob Herring @ 2020-12-17 19:02 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Rasmus Villemoes, open list:REAL TIME CLOCK (RTC) SUBSYSTEM,
	Alexandre Belloni, linux-kernel, Qiang Zhao, Bruno Thomsen

On Thu, Dec 17, 2020 at 12:12 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Thu, Dec 17, 2020 at 10:51:08AM -0600, Rob Herring wrote:
> > On Fri, Dec 11, 2020 at 5:10 PM Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> > >
> > > On 11/12/2020 23.30, Rob Herring wrote:
> > > > On Fri, Dec 11, 2020 at 3:56 PM Rasmus Villemoes
> > > > <rasmus.villemoes@prevas.dk> wrote:
> > > >>
> > > >> Some RTCs, e.g. the pcf2127, can be used as a hardware watchdog. But
> > > >> if the reset pin is not actually wired up, the driver exposes a
> > > >> watchdog device that doesn't actually work.
> > > >>
> > > >> Provide a standard binding that can be used to indicate that a given
> > > >> RTC can perform a reset of the machine, similar to wakeup-source.
> > > >
> > > > Why not use the watchdog 'timeout-sec' property?
> > >
> > > Wouldn't that be overloading that property? AFAIU, that is used to ask
> > > the kernel to program an initial timeout value into the watchdog device.
> > > But what if one doesn't want to start the watchdog device at kernel
> > > boot, but just indicate that the RTC has that capability?
> >
> > Yeah, I guess you're right.
>
> I agree, too. The initial suggestion looks fine.
>
> > > It's quite possible that if it can act as a watchdog device (and
> > > has-watchdog was also suggested), one would also want timeout-sec and
> > > other watchdog bindings to apply. But that can be added later, by those
> > > who actually want that.
> > >
> > > For now, I'd really like to get my board booting again (or rather, not
> > > get reset by the real watchdog just because the pcf2127 driver now
> > > exposes something as /dev/wathdog0, pushing the real one to
> > > /dev/wathcdog1 which doesn't get pinged from userspace).
> >
> > I'm wondering how you solve which wdog to ping when there are multiple
> > without relying on numbering. I guess 'reset-source' will solve that
> > even if that's not your current fix. So I guess I'm fine with this.
>
> I guess you'd need some udev magic that ensures that the right watchdog
> always gets the same number.

Why involve udev and keep the magic numbering important? Provide
enough details on watchdogs' features so an intelligent decision can
be made. It's the same thing every time folks try to number things in
DT.

Rob

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/3] dt-bindings: rtc: add reset-source property
  2020-12-17 19:02                             ` Rob Herring
@ 2020-12-17 20:28                               ` Uwe Kleine-König
  0 siblings, 0 replies; 39+ messages in thread
From: Uwe Kleine-König @ 2020-12-17 20:28 UTC (permalink / raw)
  To: Rob Herring
  Cc: Rasmus Villemoes, open list:REAL TIME CLOCK (RTC) SUBSYSTEM,
	Alexandre Belloni, linux-kernel, Qiang Zhao, Bruno Thomsen

[-- Attachment #1: Type: text/plain, Size: 3088 bytes --]

On Thu, Dec 17, 2020 at 01:02:32PM -0600, Rob Herring wrote:
> On Thu, Dec 17, 2020 at 12:12 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > On Thu, Dec 17, 2020 at 10:51:08AM -0600, Rob Herring wrote:
> > > On Fri, Dec 11, 2020 at 5:10 PM Rasmus Villemoes
> > > <rasmus.villemoes@prevas.dk> wrote:
> > > >
> > > > On 11/12/2020 23.30, Rob Herring wrote:
> > > > > On Fri, Dec 11, 2020 at 3:56 PM Rasmus Villemoes
> > > > > <rasmus.villemoes@prevas.dk> wrote:
> > > > >>
> > > > >> Some RTCs, e.g. the pcf2127, can be used as a hardware watchdog. But
> > > > >> if the reset pin is not actually wired up, the driver exposes a
> > > > >> watchdog device that doesn't actually work.
> > > > >>
> > > > >> Provide a standard binding that can be used to indicate that a given
> > > > >> RTC can perform a reset of the machine, similar to wakeup-source.
> > > > >
> > > > > Why not use the watchdog 'timeout-sec' property?
> > > >
> > > > Wouldn't that be overloading that property? AFAIU, that is used to ask
> > > > the kernel to program an initial timeout value into the watchdog device.
> > > > But what if one doesn't want to start the watchdog device at kernel
> > > > boot, but just indicate that the RTC has that capability?
> > >
> > > Yeah, I guess you're right.
> >
> > I agree, too. The initial suggestion looks fine.
> >
> > > > It's quite possible that if it can act as a watchdog device (and
> > > > has-watchdog was also suggested), one would also want timeout-sec and
> > > > other watchdog bindings to apply. But that can be added later, by those
> > > > who actually want that.
> > > >
> > > > For now, I'd really like to get my board booting again (or rather, not
> > > > get reset by the real watchdog just because the pcf2127 driver now
> > > > exposes something as /dev/wathdog0, pushing the real one to
> > > > /dev/wathcdog1 which doesn't get pinged from userspace).
> > >
> > > I'm wondering how you solve which wdog to ping when there are multiple
> > > without relying on numbering. I guess 'reset-source' will solve that
> > > even if that's not your current fix. So I guess I'm fine with this.
> >
> > I guess you'd need some udev magic that ensures that the right watchdog
> > always gets the same number.
> 
> Why involve udev and keep the magic numbering important? Provide
> enough details on watchdogs' features so an intelligent decision can
> be made. It's the same thing every time folks try to number things in
> DT.

Note that it was you wanted to solve the more complicated problem about
selecting the right watchdog from a set of several working watchdogs.
The problem here is easier: We have machines with two watchdog devices
and only one of them is actually working. So the straight forward thing
to do is to not provide the watchdog device for the rtc chip that cannot
reset the system.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v3 0/2] add reset-source RTC binding, update pcf2127 driver
  2020-12-11 21:56                 ` [PATCH v2 0/3] add reset-source RTC binding, update pcf2127 driver Rasmus Villemoes
                                     ` (2 preceding siblings ...)
  2020-12-11 21:56                   ` [PATCH v2 3/3] rtc: pcf2127: only use watchdog when explicitly available Rasmus Villemoes
@ 2020-12-18 10:10                   ` Rasmus Villemoes
  2020-12-18 10:10                     ` [PATCH v3 1/2] dt-bindings: rtc: add reset-source property Rasmus Villemoes
                                       ` (2 more replies)
  3 siblings, 3 replies; 39+ messages in thread
From: Rasmus Villemoes @ 2020-12-18 10:10 UTC (permalink / raw)
  To: linux-rtc, Alexandre Belloni
  Cc: linux-kernel, devicetree, Rob Herring, Uwe Kleine-König,
	Qiang Zhao, Bruno Thomsen, Rasmus Villemoes

This adds a reset-source RTC DT binding, as suggested by Alexandre,
and resends Uwe's patch making use of that property in pcf2127 driver
to avoid the driver exposing a watchdog that doesn't work (and
potentially shuffling the enumeration of the existing devices that do
work).

v3: elide the refactoring patch already in -next (5d78533a0c53 - rtc:
pcf2127: move watchdog initialisation to a separate function), make
sure to cc the DT binding list.

Applies on top of next-20201218.

Rasmus Villemoes (1):
  dt-bindings: rtc: add reset-source property

Uwe Kleine-König (1):
  rtc: pcf2127: only use watchdog when explicitly available

 Documentation/devicetree/bindings/rtc/rtc.yaml | 5 +++++
 drivers/rtc/rtc-pcf2127.c                      | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v3 1/2] dt-bindings: rtc: add reset-source property
  2020-12-18 10:10                   ` [PATCH v3 0/2] add reset-source RTC binding, update pcf2127 driver Rasmus Villemoes
@ 2020-12-18 10:10                     ` Rasmus Villemoes
  2020-12-18 21:19                       ` Rob Herring
  2020-12-18 10:10                     ` [PATCH v3 2/2] rtc: pcf2127: only use watchdog when explicitly available Rasmus Villemoes
  2020-12-19  0:58                     ` [PATCH v3 0/2] add reset-source RTC binding, update pcf2127 driver Alexandre Belloni
  2 siblings, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2020-12-18 10:10 UTC (permalink / raw)
  To: linux-rtc, Alexandre Belloni
  Cc: linux-kernel, devicetree, Rob Herring, Uwe Kleine-König,
	Qiang Zhao, Bruno Thomsen, Rasmus Villemoes

Some RTCs, e.g. the pcf2127, can be used as a hardware watchdog. But
if the reset pin is not actually wired up, the driver exposes a
watchdog device that doesn't actually work.

Provide a standard binding that can be used to indicate that a given
RTC can perform a reset of the machine, similar to wakeup-source.

Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 Documentation/devicetree/bindings/rtc/rtc.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
index 8acd2de3de3a..d30dc045aac6 100644
--- a/Documentation/devicetree/bindings/rtc/rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
@@ -63,6 +63,11 @@ properties:
     description:
       Enables wake up of host system on alarm.
 
+  reset-source:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      The RTC is able to reset the machine.
+
 additionalProperties: true
 
 ...
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v3 2/2] rtc: pcf2127: only use watchdog when explicitly available
  2020-12-18 10:10                   ` [PATCH v3 0/2] add reset-source RTC binding, update pcf2127 driver Rasmus Villemoes
  2020-12-18 10:10                     ` [PATCH v3 1/2] dt-bindings: rtc: add reset-source property Rasmus Villemoes
@ 2020-12-18 10:10                     ` Rasmus Villemoes
  2020-12-19  0:58                     ` [PATCH v3 0/2] add reset-source RTC binding, update pcf2127 driver Alexandre Belloni
  2 siblings, 0 replies; 39+ messages in thread
From: Rasmus Villemoes @ 2020-12-18 10:10 UTC (permalink / raw)
  To: linux-rtc, Alexandre Belloni
  Cc: linux-kernel, devicetree, Rob Herring, Uwe Kleine-König,
	Qiang Zhao, Bruno Thomsen, Rasmus Villemoes

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Most boards using the pcf2127 chip (in my bubble) don't make use of the
watchdog functionality and the respective output is not connected. The
effect on such a board is that there is a watchdog device provided that
doesn't work.

So only register the watchdog if the device tree has a "reset-source"
property.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
[RV: s/has-watchdog/reset-source/]
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/rtc-pcf2127.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 33fa8b17b79c..39a7b5116aa4 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -336,7 +336,8 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
 	u32 wdd_timeout;
 	int ret;
 
-	if (!IS_ENABLED(CONFIG_WATCHDOG))
+	if (!IS_ENABLED(CONFIG_WATCHDOG) ||
+	    !device_property_read_bool(dev, "reset-source"))
 		return 0;
 
 	pcf2127->wdd.parent = dev;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: rtc: add reset-source property
  2020-12-18 10:10                     ` [PATCH v3 1/2] dt-bindings: rtc: add reset-source property Rasmus Villemoes
@ 2020-12-18 21:19                       ` Rob Herring
  0 siblings, 0 replies; 39+ messages in thread
From: Rob Herring @ 2020-12-18 21:19 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: open list:REAL TIME CLOCK (RTC) SUBSYSTEM, Alexandre Belloni,
	linux-kernel, devicetree, Uwe Kleine-König, Qiang Zhao,
	Bruno Thomsen

On Fri, Dec 18, 2020 at 4:11 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Some RTCs, e.g. the pcf2127, can be used as a hardware watchdog. But
> if the reset pin is not actually wired up, the driver exposes a
> watchdog device that doesn't actually work.
>
> Provide a standard binding that can be used to indicate that a given
> RTC can perform a reset of the machine, similar to wakeup-source.
>
> Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  Documentation/devicetree/bindings/rtc/rtc.yaml | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/2] add reset-source RTC binding, update pcf2127 driver
  2020-12-18 10:10                   ` [PATCH v3 0/2] add reset-source RTC binding, update pcf2127 driver Rasmus Villemoes
  2020-12-18 10:10                     ` [PATCH v3 1/2] dt-bindings: rtc: add reset-source property Rasmus Villemoes
  2020-12-18 10:10                     ` [PATCH v3 2/2] rtc: pcf2127: only use watchdog when explicitly available Rasmus Villemoes
@ 2020-12-19  0:58                     ` Alexandre Belloni
  2 siblings, 0 replies; 39+ messages in thread
From: Alexandre Belloni @ 2020-12-19  0:58 UTC (permalink / raw)
  To: linux-rtc, Rasmus Villemoes
  Cc: Alexandre Belloni, Qiang Zhao, linux-kernel,
	Uwe Kleine-König, Bruno Thomsen, Rob Herring, devicetree

On Fri, 18 Dec 2020 11:10:52 +0100, Rasmus Villemoes wrote:
> This adds a reset-source RTC DT binding, as suggested by Alexandre,
> and resends Uwe's patch making use of that property in pcf2127 driver
> to avoid the driver exposing a watchdog that doesn't work (and
> potentially shuffling the enumeration of the existing devices that do
> work).
> 
> v3: elide the refactoring patch already in -next (5d78533a0c53 - rtc:
> pcf2127: move watchdog initialisation to a separate function), make
> sure to cc the DT binding list.
> 
> [...]

Applied, thanks!

[1/2] dt-bindings: rtc: add reset-source property
      commit: 320d159e2d63a97a40f24cd6dfda5a57eec65b91
[2/2] rtc: pcf2127: only use watchdog when explicitly available
      commit: 71ac13457d9d1007effde65b54818106b2c2b525

Best regards,
-- 
Alexandre Belloni <alexandre.belloni@bootlin.com>

^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2020-12-19  0:59 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-21  5:48 [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Qiang Zhao
2020-09-21  5:48 ` [Patch v2 2/3] rtc: pcf2127: add "no-watchdog" property Qiang Zhao
2020-09-21  5:48 ` [Patch v2 3/3] arm64: dts: lx2160a: Add "no-watchdog" property to pcf2127 Qiang Zhao
2020-09-23  9:44 ` [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Alexandre Belloni
2020-09-24  3:20   ` Qiang Zhao
2020-09-24  7:04     ` Uwe Kleine-König
2020-09-24  7:23       ` Qiang Zhao
2020-09-24  7:47         ` Alexandre Belloni
2020-09-24 10:52           ` [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
2020-09-24 10:52             ` [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function Uwe Kleine-König
2020-09-27  7:50               ` Bruno Thomsen
2020-09-24 10:52             ` [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
2020-09-27  8:09               ` Bruno Thomsen
2020-09-27 15:54                 ` Guenter Roeck
2020-09-28  8:43                   ` Uwe Kleine-König
2020-09-28 16:26                     ` Guenter Roeck
2020-10-26  7:23             ` [PATCH 0/2] " Qiang Zhao
2020-10-26 20:48               ` Alexandre Belloni
2020-10-26 21:29                 ` Guenter Roeck
2020-11-30  9:51             ` Rasmus Villemoes
2020-12-04  9:27               ` Alexandre Belloni
2020-12-11 21:56                 ` [PATCH v2 0/3] add reset-source RTC binding, update pcf2127 driver Rasmus Villemoes
2020-12-11 21:56                   ` [PATCH v2 1/3] dt-bindings: rtc: add reset-source property Rasmus Villemoes
2020-12-11 22:30                     ` Rob Herring
2020-12-11 23:10                       ` Rasmus Villemoes
2020-12-17 16:51                         ` Rob Herring
2020-12-17 18:12                           ` Uwe Kleine-König
2020-12-17 18:23                             ` Rasmus Villemoes
2020-12-17 19:02                             ` Rob Herring
2020-12-17 20:28                               ` Uwe Kleine-König
2020-12-12  9:25                     ` Uwe Kleine-König
2020-12-11 21:56                   ` [PATCH v2 2/3] rtc: pcf2127: move watchdog initialisation to a separate function Rasmus Villemoes
2020-12-11 21:56                   ` [PATCH v2 3/3] rtc: pcf2127: only use watchdog when explicitly available Rasmus Villemoes
2020-12-18 10:10                   ` [PATCH v3 0/2] add reset-source RTC binding, update pcf2127 driver Rasmus Villemoes
2020-12-18 10:10                     ` [PATCH v3 1/2] dt-bindings: rtc: add reset-source property Rasmus Villemoes
2020-12-18 21:19                       ` Rob Herring
2020-12-18 10:10                     ` [PATCH v3 2/2] rtc: pcf2127: only use watchdog when explicitly available Rasmus Villemoes
2020-12-19  0:58                     ` [PATCH v3 0/2] add reset-source RTC binding, update pcf2127 driver Alexandre Belloni
2020-09-24 16:59           ` [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Bruno Thomsen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).