Re: [PATCH v2 0/3] Add Watchdog for H6

Re: [PATCH v2 0/3] Add Watchdog for H6

[Clément Péron]

On Thu, 11 Apr 2019 at 17:34, Clément Péron <peron.clem@gmail.com> wrote:
>
> Hi,
>
> Allwinner H6 SoC has two watchdogs. The documented watchdog is identicaly
> the same reading the user manual as the one used in the A64.
>
> After investigation it seems that on some SoC the first watchdog doesn't
> make the board reboot.
> Some tests has been performed and it seems that first H6 version aren't
> affected only the latest one.
>
> Chen-Yu Tsai's boards:
> Pine H64 - H6 V200-AWIN H6448BA 7782 => OK
> OrangePi Lite 2 - H6 V200-AWIN H8068BA 61C2 => KO
>
> Clément Péron's board:
> Beelink GS1 - H6 V200-AWIN H7309BA 6842 => KO
>
> I think it could good to add it with a comment anyway.
> In case a user want to activate it and isn't aware of this issue.
>
> The r_watchdog is still available and usable on all the H6 boards.

Changes since v1:
 - Use A64 compatible instead of H6
 - Remove dt-bindings patch
 - Change watchdog status to disabled
 - Add r_watchdog node patch
 - Add enable sunxi watchdog patch

>
> Thanks,
> Clément
>
> Clément Péron (3):
>   arm64: dts: allwinner: h6: add watchdog node
>   arm64: dts: allwinner: h6: add r_watchog node
>   arm64: defconfig: Enable Sunxi watchdog
>
>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 16 ++++++++++++++++
>  arch/arm64/configs/defconfig                 |  1 +
>  2 files changed, 17 insertions(+)
>
> --
> 2.17.1
>

[PATCH v2 0/3] Add Watchdog for H6

[Clément Péron]

Hi,

Allwinner H6 SoC has two watchdogs. The documented watchdog is identicaly
the same reading the user manual as the one used in the A64.

After investigation it seems that on some SoC the first watchdog doesn't
make the board reboot.
Some tests has been performed and it seems that first H6 version aren't
affected only the latest one.

Chen-Yu Tsai's boards:
Pine H64 - H6 V200-AWIN H6448BA 7782 => OK
OrangePi Lite 2 - H6 V200-AWIN H8068BA 61C2 => KO

Clément Péron's board:
Beelink GS1 - H6 V200-AWIN H7309BA 6842 => KO

I think it could good to add it with a comment anyway.
In case a user want to activate it and isn't aware of this issue.

The r_watchdog is still available and usable on all the H6 boards.

Thanks,
Clément

Clément Péron (3):
  arm64: dts: allwinner: h6: add watchdog node
  arm64: dts: allwinner: h6: add r_watchog node
  arm64: defconfig: Enable Sunxi watchdog

 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 16 ++++++++++++++++
 arch/arm64/configs/defconfig                 |  1 +
 2 files changed, 17 insertions(+)

-- 
2.17.1

[PATCH v2 1/3] arm64: dts: allwinner: h6: add watchdog node

[Clément Péron]

Allwinner H6 has a watchog identical to A64 one.

Declare it in the device tree but leave it disable
with a comment as it is broken on some H6 revisions.

Test has been performs on 3 boards.

Chen-Yu Tsai boards:
Pine H64 - H6 V200-AWIN H6448BA 7782 => OK
OrangePi Lite 2 - H6 V200-AWIN H8068BA 61C2 => KO

Clément Péron board:
Beelink GS1 - H6 V200-AWIN H7309BA 6842 => KO

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index e0dc4a05c1ba..5c2f5451227b 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -208,6 +208,15 @@
 			reg = <0x03006000 0x400>;
 		};
 
+		watchdog: watchdog@30090a0 {
+			compatible = "allwinner,sun50i-a64-wdt",
+				     "allwinner,sun6i-a31-wdt";
+			reg = <0x030090a0 0x20>;
+			interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
+			/* Broken on some H6 rev */
+			status = "disabled";
+		};
+
 		pio: pinctrl@300b000 {
 			compatible = "allwinner,sun50i-h6-pinctrl";
 			reg = <0x0300b000 0x400>;
-- 
2.17.1

[PATCH v2 2/3] arm64: dts: allwinner: h6: add r_watchog node

[Clément Péron]

Allwinner H6 has a r_watchdog similar to A64.

Declare it in the device-tree.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 5c2f5451227b..66dc684a378e 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -622,6 +622,13 @@
 			#reset-cells = <1>;
 		};
 
+		r_watchdog: watchdog@7020400 {
+			compatible = "allwinner,sun50i-a64-wdt",
+				     "allwinner,sun6i-a31-wdt";
+			reg = <0x07020400 0x20>;
+			interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
 		r_intc: interrupt-controller@7021000 {
 			compatible = "allwinner,sun50i-h6-r-intc",
 				     "allwinner,sun6i-a31-r-intc";
-- 
2.17.1

[PATCH v2 3/3] arm64: defconfig: Enable Sunxi watchdog

[Clément Péron]

The SUNXI_WATCHDOG option is required to make the
watchdog available on Allwinner H6.

Enable this option as a module.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 2d9c39033c1a..c266d6babbcc 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -426,6 +426,7 @@ CONFIG_UNIPHIER_THERMAL=y
 CONFIG_WATCHDOG=y
 CONFIG_ARM_SP805_WATCHDOG=y
 CONFIG_S3C2410_WATCHDOG=y
+CONFIG_SUNXI_WATCHDOG=m
 CONFIG_IMX2_WDT=y
 CONFIG_MESON_GXBB_WATCHDOG=m
 CONFIG_MESON_WATCHDOG=m
-- 
2.17.1

Re: [PATCH v2 2/3] arm64: dts: allwinner: h6: add r_watchog node

[Clément Péron]

Hi,

On Thu, 11 Apr 2019 at 17:34, Clément Péron <peron.clem@gmail.com> wrote:
>
> Allwinner H6 has a r_watchdog similar to A64.
>
> Declare it in the device-tree.
>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> index 5c2f5451227b..66dc684a378e 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> @@ -622,6 +622,13 @@
>                         #reset-cells = <1>;
>                 };
>
> +               r_watchdog: watchdog@7020400 {
> +                       compatible = "allwinner,sun50i-a64-wdt",
> +                                    "allwinner,sun6i-a31-wdt";
> +                       reg = <0x07020400 0x20>;
> +                       interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> +               };

I have set the same compatible as A64 because regarding the User
Manual they have exactly the same memory mapping.
However we don't know really if it's the same IP version, maybe there
will be an errata one day.
So I would like to know if it's better to define the h6-wdt also
  compatible =  "allwinner,sun50i-h6-wdt", "allwinner,sun50i-a64-wdt",
                                 "allwinner,sun6i-a31-wdt";

I would say Yes, but with this logic we would have to had a new
compatible each time there is a  new SoC.

Thanks,
Clement

> +
>                 r_intc: interrupt-controller@7021000 {
>                         compatible = "allwinner,sun50i-h6-r-intc",
>                                      "allwinner,sun6i-a31-r-intc";
> --
> 2.17.1
>

Re: [PATCH v2 2/3] arm64: dts: allwinner: h6: add r_watchog node

[Maxime Ripard]

On Fri, Apr 19, 2019 at 09:00:42PM +0200, Clément Péron wrote:
> Hi,
>
> On Thu, 11 Apr 2019 at 17:34, Clément Péron <peron.clem@gmail.com> wrote:
> >
> > Allwinner H6 has a r_watchdog similar to A64.
> >
> > Declare it in the device-tree.
> >
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> >  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > index 5c2f5451227b..66dc684a378e 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > @@ -622,6 +622,13 @@
> >                         #reset-cells = <1>;
> >                 };
> >
> > +               r_watchdog: watchdog@7020400 {
> > +                       compatible = "allwinner,sun50i-a64-wdt",
> > +                                    "allwinner,sun6i-a31-wdt";
> > +                       reg = <0x07020400 0x20>;
> > +                       interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> > +               };
>
> I have set the same compatible as A64 because regarding the User
> Manual they have exactly the same memory mapping.
> However we don't know really if it's the same IP version, maybe there
> will be an errata one day.
> So I would like to know if it's better to define the h6-wdt also
>   compatible =  "allwinner,sun50i-h6-wdt", "allwinner,sun50i-a64-wdt",
>                                  "allwinner,sun6i-a31-wdt";
>
> I would say Yes, but with this logic we would have to had a new
> compatible each time there is a  new SoC.

Why not just having the A31 compatible?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 2/3] arm64: dts: allwinner: h6: add r_watchog node

[Clément Péron]

On Tue, 23 Apr 2019 at 18:05, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Fri, Apr 19, 2019 at 09:00:42PM +0200, Clément Péron wrote:
> > Hi,
> >
> > On Thu, 11 Apr 2019 at 17:34, Clément Péron <peron.clem@gmail.com> wrote:
> > >
> > > Allwinner H6 has a r_watchdog similar to A64.
> > >
> > > Declare it in the device-tree.
> > >
> > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > ---
> > >  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > index 5c2f5451227b..66dc684a378e 100644
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > @@ -622,6 +622,13 @@
> > >                         #reset-cells = <1>;
> > >                 };
> > >
> > > +               r_watchdog: watchdog@7020400 {
> > > +                       compatible = "allwinner,sun50i-a64-wdt",
> > > +                                    "allwinner,sun6i-a31-wdt";
> > > +                       reg = <0x07020400 0x20>;
> > > +                       interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> > > +               };
> >
> > I have set the same compatible as A64 because regarding the User
> > Manual they have exactly the same memory mapping.
> > However we don't know really if it's the same IP version, maybe there
> > will be an errata one day.
> > So I would like to know if it's better to define the h6-wdt also
> >   compatible =  "allwinner,sun50i-h6-wdt", "allwinner,sun50i-a64-wdt",
> >                                  "allwinner,sun6i-a31-wdt";
> >
> > I would say Yes, but with this logic we would have to had a new
> > compatible each time there is a  new SoC.
>
Hi,

> Why not just having the A31 compatible?
H6 has a lightly differences in registers with A31 but it's the same
as 64 (WDOG_KEY_FIELD)

As the A64 is already introduced if think it's a good solution to reuse it.

Clément

>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com