watchdog: imx2: handle WMCR only being available on i.MX35 and later

Hello,

this is my approach to fix the issue reported by Vladimir Zapolskiy.

IMHO we don't need the third commit because I'm convinced most machines just don't do anything when the WDOG signal becomes active. An affected machine powers off 16 seconds after startup. This would be noticed during development of the bootloader and so I assume all affected machines having bootloaders that make the compat code unimportant.

If you want to know if your machine is affected, do:

	mw -w $(watchdog_base_addr) 0x10

The machines I tested this on (among a few customer boards a Phycore i.MX35 and a Freescale i.MX53 Quick Start Board) don't power off.

I don't have manuals handy for ls1021a, ls1043a and ls1046a but assume they use the i.MX35 type, too. This needs to be fixed before this series is applied.

Compared to Vladimir's patch machines still using board files are fixed, too.

No, I weight old dtb compatibility which IMHO doesn't buy us anything in this case against maintenance overhead.

I want to disagree with this. I replied the first time with my concerns about the compat code on December 11, so I'm not responsible for the time before that date. Also on December 23 I said that if Vladimir really wants that compat code he should add it. The delay between December 11 and December 23 isn't mine either. In this thread I am a reviewer and so it's unfair to point at me when wailing about delays. I mentioned my doubts and if you don't agree or understand the patch submitter is free to discuss or ask the maintainer to overrule my opinion.

Best regards
Uwe

Uwe Kleine-K?nig (3):
watchdog: imx2: Only i.MX35 and later have a WMCR register
dts: teach newer i.MX machines to have the i.MX35 type watchdog
watchdog: imx2: add compatibility for new i.MX35 type watchdog

 .../devicetree/bindings/watchdog/fsl-imx-wdt.txt   |  2 +-
 arch/arm/boot/dts/imx25.dtsi                       |  2 +-
 arch/arm/boot/dts/imx35.dtsi                       |  2 +-
 arch/arm/boot/dts/imx50.dtsi                       |  2 +-
 arch/arm/boot/dts/imx51.dtsi                       |  2 +-
 arch/arm/boot/dts/imx53.dtsi                       |  2 +-
 arch/arm/boot/dts/imx6qdl.dtsi                     |  2 +-
 arch/arm/boot/dts/imx6sl.dtsi                      |  4 +-
 arch/arm/boot/dts/imx6sx.dtsi                      |  6 +-
 arch/arm/boot/dts/imx6ul.dtsi                      |  4 +-
 arch/arm/boot/dts/imx7s.dtsi                       |  8 +-
 arch/arm/boot/dts/vfxxx.dtsi                       |  2 +-
 arch/arm/mach-imx/devices/devices-common.h         |  1 +
 arch/arm/mach-imx/devices/platform-imx2-wdt.c      | 13 +--
 drivers/watchdog/imx2_wdt.c                        | 97 ++++++++++++++++++++--
 15 files changed, 116 insertions(+), 33 deletions(-)

2.11.0

Commit 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot") introduced a write to the WMCR register that doesn't exist on i.MX21, i.MX27 and i.MX31 and so makes the SoC hang during probe.

So teach the driver to differentiate between these two types. Note that this effectively undoes commit 5fe65ce7ccbb for machines using dt until their dtb is updated accordingly. This is critical iff the bootloader doesn't disable the power down counter and the #WDOG signal actually does something to the machine.

---
 arch/arm/mach-imx/devices/devices-common.h    |  1 +
 arch/arm/mach-imx/devices/platform-imx2-wdt.c | 13 +++---
 drivers/watchdog/imx2_wdt.c                   | 59 +++++++++++++++++++++++----
 3 files changed, 59 insertions(+), 14 deletions(-)

index 6920e356f4e5..946cca627018 100644
@@ -92,6 +92,7 @@ struct platform_device *__init imx_add_imx27_coda(
 
 struct imx_imx2_wdt_data {
 	int id;
+	const char *devname;
 	resource_size_t iobase;
 	resource_size_t iosize;
 };

index 8c134c8d7500..d8d3dd58fcfe 100644
@@ -11,9 +11,10 @@
 #include "../hardware.h"
 #include "devices-common.h"
 
-#define imx_imx2_wdt_data_entry_single(soc, _id, _hwid, _size)		\
+#define imx_imx2_wdt_data_entry_single(soc, _id, _devname, _hwid, _size)\
 	{								\
 		.id = _id,						\
+		.devname = _devname,					\
 		.iobase = soc ## _WDOG ## _hwid ## _BASE_ADDR,		\
 		.iosize = _size,					\
 	}

@@ -22,22 +23,22 @@
 
 #ifdef CONFIG_SOC_IMX21
 const struct imx_imx2_wdt_data imx21_imx2_wdt_data __initconst =
-	imx_imx2_wdt_data_entry_single(MX21, 0, , SZ_4K);
+	imx_imx2_wdt_data_entry_single(MX21, 0, "imx21-wdt", , SZ_4K);
 #endif /* ifdef CONFIG_SOC_IMX21 */
 
 #ifdef CONFIG_SOC_IMX27
 const struct imx_imx2_wdt_data imx27_imx2_wdt_data __initconst =
-	imx_imx2_wdt_data_entry_single(MX27, 0, , SZ_4K);
+	imx_imx2_wdt_data_entry_single(MX27, 0, "imx21-wdt", , SZ_4K);
 #endif /* ifdef CONFIG_SOC_IMX27 */
 
 #ifdef CONFIG_SOC_IMX31
 const struct imx_imx2_wdt_data imx31_imx2_wdt_data __initconst =
-	imx_imx2_wdt_data_entry_single(MX31, 0, , SZ_16K);
+	imx_imx2_wdt_data_entry_single(MX31, 0, "imx21-wdt", , SZ_16K);
 #endif /* ifdef CONFIG_SOC_IMX31 */
 
 #ifdef CONFIG_SOC_IMX35
 const struct imx_imx2_wdt_data imx35_imx2_wdt_data __initconst =
-	imx_imx2_wdt_data_entry_single(MX35, 0, , SZ_16K);
+	imx_imx2_wdt_data_entry_single(MX35, 0, "imx35-wdt", , SZ_16K);
 #endif /* ifdef CONFIG_SOC_IMX35 */
 
 struct platform_device *__init imx_add_imx2_wdt(

@@ -50,6 +51,6 @@ struct platform_device *__init imx_add_imx2_wdt(
 			.flags = IORESOURCE_MEM,
 		},
 	};
-	return imx_add_platform_device("imx2-wdt", data->id,
+	return imx_add_platform_device(data->devname, data->id,
 			res, ARRAY_SIZE(res), NULL, 0);
 }

2.11.0

Hi Magnus,

Is it possible for you to try this series and let us know if it fixes the mx31 watchdog issue?

Thanks

Only i.MX35 and newer feature a WMCR register that should be written to. Older SoCs hang when this address is written.

---
 Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt | 2 +-
 arch/arm/boot/dts/imx25.dtsi                               | 2 +-
 arch/arm/boot/dts/imx35.dtsi                               | 2 +-
 arch/arm/boot/dts/imx50.dtsi                               | 2 +-
 arch/arm/boot/dts/imx51.dtsi                               | 2 +-
 arch/arm/boot/dts/imx53.dtsi                               | 2 +-
 arch/arm/boot/dts/imx6qdl.dtsi                             | 2 +-
 arch/arm/boot/dts/imx6sl.dtsi                              | 4 ++--
 arch/arm/boot/dts/imx6sx.dtsi                              | 6 +++---
 arch/arm/boot/dts/imx6ul.dtsi                              | 4 ++--
 arch/arm/boot/dts/imx7s.dtsi                               | 8 ++++----
 arch/arm/boot/dts/vfxxx.dtsi                               | 2 +-
 12 files changed, 19 insertions(+), 19 deletions(-)

index 107280ef0025..607b010a607a 100644
@@ -15,7 +15,7 @@ Optional properties:
 Examples:
 
 wdt at 73f98000 {
-	compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
+	compatible = "fsl,imx51-wdt", "fsl,imx35-wdt";
 	reg = <0x73f98000 0x4000>;
 	interrupts = <58>;
 	big-endian;

index 831d09a28155..d9ef338e8af7 100644
@@ -504,7 +504,7 @@
 			};
 
 			wdog at 53fdc000 {
-				compatible = "fsl,imx25-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx25-wdt", "fsl,imx35-wdt";
 				reg = <0x53fdc000 0x4000>;
 				clocks = <&clks 126>;
 				clock-names = "";

index 9f40e6229189..d9a4e77b74d8 100644
@@ -286,7 +286,7 @@
 			};
 
 			wdog: wdog at 53fdc000 {
-				compatible = "fsl,imx35-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx35-wdt";
 				reg = <0x53fdc000 0x4000>;
 				clocks = <&clks 74>;
 				clock-names = "";

index fe0221e4cbf7..cf90e3d44f1c 100644
@@ -270,7 +270,7 @@
 			};
 
 			wdog1: wdog at 53f98000 {
-				compatible = "fsl,imx50-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx50-wdt", "fsl,imx35-wdt";
 				reg = <0x53f98000 0x4000>;
 				interrupts = <58>;
 				clocks = <&clks IMX5_CLK_DUMMY>;

index 33526cade735..998bf2ffd90d 100644
@@ -347,7 +347,7 @@
 			};
 
 			wdog1: wdog at 73f98000 {
-				compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx51-wdt", "fsl,imx35-wdt";
 				reg = <0x73f98000 0x4000>;
 				interrupts = <58>;
 				clocks = <&clks IMX5_CLK_DUMMY>;

index ca51dc03e327..22becf17529a 100644
@@ -402,7 +402,7 @@
 			};
 
 			wdog1: wdog at 53f98000 {
-				compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx53-wdt", "fsl,imx35-wdt";
 				reg = <0x53f98000 0x4000>;
 				interrupts = <58>;
 				clocks = <&clks IMX5_CLK_DUMMY>;

index 89b834f3fa17..5ca0ce926ccf 100644
@@ -594,7 +594,7 @@
 			};
 
 			wdog1: wdog at 020bc000 {
-				compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx6q-wdt", "fsl,imx35-wdt";
 				reg = <0x020bc000 0x4000>;
 				interrupts = <0 80 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6QDL_CLK_DUMMY>;

index 19cbd879c448..43b10ff725e0 100644
@@ -479,14 +479,14 @@
 			};
 
 			wdog1: wdog at 020bc000 {
-				compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx6sl-wdt", "fsl,imx35-wdt";
 				reg = <0x020bc000 0x4000>;
 				interrupts = <0 80 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6SL_CLK_DUMMY>;
 			};
 
 			wdog2: wdog at 020c0000 {
-				compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx6sl-wdt", "fsl,imx35-wdt";
 				reg = <0x020c0000 0x4000>;
 				interrupts = <0 81 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6SL_CLK_DUMMY>;

index 10f333016197..94d75c55bbef 100644
@@ -534,14 +534,14 @@
 			};
 
 			wdog1: wdog at 020bc000 {
-				compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx6sx-wdt", "fsl,imx35-wdt";
 				reg = <0x020bc000 0x4000>;
 				interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6SX_CLK_DUMMY>;
 			};
 
 			wdog2: wdog at 020c0000 {
-				compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx6sx-wdt", "fsl,imx35-wdt";
 				reg = <0x020c0000 0x4000>;
 				interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6SX_CLK_DUMMY>;

@@ -1201,7 +1201,7 @@
 			};
 
 			wdog3: wdog at 02288000 {
-				compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx6sx-wdt", "fsl,imx35-wdt";
 				reg = <0x02288000 0x4000>;
 				interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6SX_CLK_DUMMY>;

index 39845a7e0463..e3816e808cfb 100644
@@ -491,14 +491,14 @@
 			};
 
 			wdog1: wdog at 020bc000 {
-				compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx6ul-wdt", "fsl,imx35-wdt";
 				reg = <0x020bc000 0x4000>;
 				interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6UL_CLK_WDOG1>;
 			};
 
 			wdog2: wdog at 020c0000 {
-				compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx6ul-wdt", "fsl,imx35-wdt";
 				reg = <0x020c0000 0x4000>;
 				interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6UL_CLK_WDOG2>;

index 8ff2cbdd8f0d..1a95803485d5 100644
@@ -399,14 +399,14 @@
 			};
 
 			wdog1: wdog at 30280000 {
-				compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx7d-wdt", "fsl,imx35-wdt";
 				reg = <0x30280000 0x10000>;
 				interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX7D_WDOG1_ROOT_CLK>;
 			};
 
 			wdog2: wdog at 30290000 {
-				compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx7d-wdt", "fsl,imx35-wdt";
 				reg = <0x30290000 0x10000>;
 				interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX7D_WDOG2_ROOT_CLK>;

@@ -414,7 +414,7 @@
 			};
 
 			wdog3: wdog at 302a0000 {
-				compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx7d-wdt", "fsl,imx35-wdt";
 				reg = <0x302a0000 0x10000>;
 				interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX7D_WDOG3_ROOT_CLK>;

@@ -422,7 +422,7 @@
 			};
 
 			wdog4: wdog at 302b0000 {
-				compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,imx7d-wdt", "fsl,imx35-wdt";
 				reg = <0x302b0000 0x10000>;
 				interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX7D_WDOG4_ROOT_CLK>;

index e9d28474c26a..4ce240790a3d 100644
@@ -326,7 +326,7 @@
 			};
 
 			wdoga5: wdog at 4003e000 {
-				compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
+				compatible = "fsl,vf610-wdt", "fsl,imx35-wdt";
 				reg = <0x4003e000 0x1000>;
 				interrupts = <20 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks VF610_CLK_WDT>;

2.11.0

Only a minor nit: Subject could: ARM: dts: imx: teach...

Ack. To reference the commit id of the first patch in the third I can prepare a branch to pull. There I'd fix the Subject. Shawn, is this OK?

Best regards
Uwe

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

This is too early for changes, which are not reviewed by Linux watchdog masters.

As I emphasized multiple times in our discussions your series is organized improperly, it has interdependencies between DTS and driver, it lacks atomicity feature of a proper fix, in the middle it breaks backward compatibility with old DTB, all that makes it impossible to send an atomic fix for linux-stable, some of the changes are captured from mine ones without preserving the authorship or even a reported-by credit.

--
With best wishes,
Vladimir

Hello Vladimir,

Oh, I must have missed that. In my inbox there are two replys to my series, the one I reply to being the first to mention these critics.

Regarding your critics:

- organized improperly: that alone is too abstract to understand what you mean.
- interdependencies between DTS and driver, lacks atomicity, impossible to send an atomic fix for linux-stable: I wonder how you want to get rid of that. Yes there are interdependencies, that's because both driver and dts need adaption. Well, I could squash together some of the changes, then there is a single patch that atomically fixes everything. Personally I prefer to have three commits, each one changing a single thing and considering all three together as a fix for stable.
- in the middle it breaks backward compatibility: There are three patches:

1) watchdog: imx2: Only i.MX35 and later have a WMCR register
2) dts: teach newer i.MX machines to have the i.MX35 type watchdog
3) watchdog: imx2: add compatibility for new i.MX35 type watchdog

Before the first patch we have:

- newer i.mx dtsi wrongly claims to have i.mx21 type watchdog
- watchdog handles i.MX21 as if it were i.MX35

After the first patch we have:

- newer i.mx dtsi wrongly claims to have i.mx21 type watchdog

And after the second everything is fine (assuming a dtb not older than the kernel). So each patch fixes a single thing.

Yes, after the first patch machines having an i.MX35 type watchdog might have a problem. That's because there are two things to fix (so there are two patches): The driver must know about the new type and the dts must make use of it. And fixing the first makes the second visible, but doesn't introduce it (after all the i.MX35 dtsi is wrong claiming to have the i.MX21 type).

Now that there are machines known that suffer from the problem under discussion it's obvious that we want the changes from the third patch. Personally I don't care much if it stays separate or is squashed into the first one.

Compared to your v1 the similarities are AFAICT:

- added .data to dt_ids array
- added an if to only conditionally execute the breaking command
- added a new compatible to several dtsi files

All these are necessary to fix the problem discussed here. The way I implemented the first two ones are different to your way. So IMHO it's at least questionable if you or I should be the author of the patches I sent.

I'm ok to add a Reported-by for you. I usually add such a tag only after a request for it. I missed to ask for that, please excuse this.

Best regards
Uwe

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

When the watchdog driver learned about the i.MX35 type watchdog a new compatibility string was introduced. Older dtb continue to use the old fsl,imx21-wdt string and so stop writing the WMCR register which is critical iff the machine powers off when WDOG becomes active and the bootloader doesn't stop the power down counter.

It's unknown which boards are affected and I guess most of them are not. Also note this is only an issue if a new kernel is operated with an old dtb as the SoC dtsi files are fixed accordingly.

XXX: the XXX introduced needs to be changed to HEAD~2 when got a stable commit id.

---
 drivers/watchdog/imx2_wdt.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

index 509b2fedb112..f5616e2c2c58 100644
@@ -454,6 +454,44 @@ static const struct of_device_id imx2_wdt_dt_ids[] = {
 	}, {
 		.compatible = "fsl,imx35-wdt",
 		.data = &imx2_wdt_type_imx35,
+	},
+	/*
+	 * The following ids are only there to make commit XXX compatible with
+	 * older device trees. The result is that iff the machine makes use of
+	 * the WDG signal (most don't, only WDG_RST is used) and the bootloader
+	 * doesn't stop the power down counter it powers off the board after 16
+	 * seconds.
+	 */
+	{
+		.compatible = "fsl,imx25-wdt",
+		.data = &imx2_wdt_type_imx35,
+	}, {
+		.compatible = "fsl,imx50-wdt",
+		.data = &imx2_wdt_type_imx35,
+	}, {
+		.compatible = "fsl,imx51-wdt",
+		.data = &imx2_wdt_type_imx35,
+	}, {
+		.compatible = "fsl,imx53-wdt",
+		.data = &imx2_wdt_type_imx35,
+	}, {
+		.compatible = "fsl,imx6q-wdt",
+		.data = &imx2_wdt_type_imx35,
+	}, {
+		.compatible = "fsl,imx6sl-wdt",
+		.data = &imx2_wdt_type_imx35,
+	}, {
+		.compatible = "fsl,imx6sx-wdt",
+		.data = &imx2_wdt_type_imx35,
+	}, {
+		.compatible = "fsl,imx6ul-wdt",
+		.data = &imx2_wdt_type_imx35,
+	}, {
+		.compatible = "fsl,imx7d-wdt",
+		.data = &imx2_wdt_type_imx35,
+	}, {
+		.compatible = "fsl,vf610-wdt",
+		.data = &imx2_wdt_type_imx35,
 	}, {
 		/* sentinel */
 	}

2.11.0

At least imx31 pdk is affected.

This comment should not be here.

Other than that:

Hello Uwe,

is it a command in barebox shell?

This addition can be done on top of my changes, in my version the driver ignores WMCR on all machines with board files.

So now we have two competing series unfortunately...

--
With best wishes,
Vladimir

Hello Vladimir,

barebox cannot do $(watchdog_base_addr), but (on i.MX25) I can do:

	mw -w 0x53fdc000 0x10

. I think the corresponding U-Boot command is:

	mw.w 0x53fdc000 0x10

With mine being the better one, right? :-)

You didn't update your series for my concerns, so I did it myself instead of retrying to iterate about the changes I'd like to see on your version. Feel free to criticise my variant.

Best regards
Uwe

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

So ...

Unfortunately, I don't see any convergence on this issue. My take is that, given the lack of convergence, it can't be that severe since no one feels that it is important enough to get it fixed one way or the other to budge from your positions.

I had the other patch in my watchdog-next branch. I'll drop it, and wait for the two of you to agree to something. You can either agree to that something, or try to convince Wim to accept one or the other patch set. For my part, I'll only accept a patch or patch series which has a Signed-off: and/or Reviewed-by:
from both of you.

Maybe you can get together into a room and agree not to leave it until white smoke comes out of the chimney. Just an idea.

Thanks,
Guenter

I didn't get any critic for my patch set yet apart from being a second one to address the same problem. So until proven differently I assume that mine is fine.

Best regards
Uwe

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

Hi,

...

Tested on i.MX31 PDK board.