mbox series

[v3,0/4] Allwinner H6 watchdog support

Message ID 20190518152355.11134-1-peron.clem@gmail.com
Headers show
Series Allwinner H6 watchdog support | expand

Message

Clément Péron May 18, 2019, 3:23 p.m. UTC 
Hi,

Allwinner H6 SoC has two watchdogs.

As we are not sure that both A64 and H6 are stricly identical, I have
introduced the H6 bindings.

After investigation it seems that on some boards the first watchdog doesn't
make it properly reboot. Please see details in the commit log.

I think it's proper to add it with a comment anyway.

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

Maybe it would be proper to introduce a "allwinner,sun50i-h6-r-wdt" bindings?

Thanks,
Clément

Changes since v2:
 - Reintroduce H6 bindings
 - Add watchdog Maintainters / ML
 - Add Martin Ayotte test results

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

Clément Péron (4):
  dt-bindings: watchdog: add Allwinner H6 watchdog
  arm64: dts: allwinner: h6: add watchdog node
  arm64: dts: allwinner: h6: add r_watchog node
  arm64: defconfig: enable sunxi watchdog

 .../devicetree/bindings/watchdog/sunxi-wdt.txt | 10 ++++++----
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi   | 18 ++++++++++++++++++
 arch/arm64/configs/defconfig                   |  1 +
 3 files changed, 25 insertions(+), 4 deletions(-)

Comments

Maxime Ripard May 20, 2019, 7:36 a.m. UTC | #1 
On Sat, May 18, 2019 at 05:23:53PM +0200, Clément Péron wrote:
> Allwinner H6 has a watchog node which seems broken
> on some boards.
>
> Test has been performed on several boards.
>
> Chen-Yu Tsai boards:
> Pine H64 - H6448BA 7782 => OK
> OrangePi Lite 2 - H8068BA 61C2 => KO
>
> Martin Ayotte boards:
> Pine H64 - H8069BA 6892 => OK
> OrangePi 3 - HA047BA 69W2 => KO
> OrangePi One Plus - H7310BA 6842 => KO
> OrangePi Lite2 - H6448BA 6662 => KO
>
> Clément Péron board:
> Beelink GS1 - H7309BA 6842 => KO
>
> As it seems not fixable for now, declare the node
> but leave it disable with a comment.
>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>

If it doesn't work most boards, then why do we need to merge that
patch in the first place?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Clément Péron May 20, 2019, 8:21 a.m. UTC | #2 
Hi,

On Mon, 20 May 2019 at 09:36, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Sat, May 18, 2019 at 05:23:53PM +0200, Clément Péron wrote:
> > Allwinner H6 has a watchog node which seems broken
> > on some boards.
> >
> > Test has been performed on several boards.
> >
> > Chen-Yu Tsai boards:
> > Pine H64 - H6448BA 7782 => OK
> > OrangePi Lite 2 - H8068BA 61C2 => KO
> >
> > Martin Ayotte boards:
> > Pine H64 - H8069BA 6892 => OK
> > OrangePi 3 - HA047BA 69W2 => KO
> > OrangePi One Plus - H7310BA 6842 => KO
> > OrangePi Lite2 - H6448BA 6662 => KO
> >
> > Clément Péron board:
> > Beelink GS1 - H7309BA 6842 => KO
> >
> > As it seems not fixable for now, declare the node
> > but leave it disable with a comment.
> >
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
>
> If it doesn't work most boards, then why do we need to merge that
> patch in the first place?

My personnal opinion, is that having the IP declared and disabled with
a comment saying "it's broken on some boards" in the device-tree is
better than not having at all.

This will explicit say "the IP exist but don't use it!".
Maybe some people with a functionnal board would like to explicitly
use it on their dts.

Again just my personnal opinion,
Thanks for the review,
Clément

>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Maxime Ripard May 20, 2019, 2:44 p.m. UTC | #3 
On Mon, May 20, 2019 at 10:21:40AM +0200, Clément Péron wrote:
> Hi,
>
> On Mon, 20 May 2019 at 09:36, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Sat, May 18, 2019 at 05:23:53PM +0200, Clément Péron wrote:
> > > Allwinner H6 has a watchog node which seems broken
> > > on some boards.
> > >
> > > Test has been performed on several boards.
> > >
> > > Chen-Yu Tsai boards:
> > > Pine H64 - H6448BA 7782 => OK
> > > OrangePi Lite 2 - H8068BA 61C2 => KO
> > >
> > > Martin Ayotte boards:
> > > Pine H64 - H8069BA 6892 => OK
> > > OrangePi 3 - HA047BA 69W2 => KO
> > > OrangePi One Plus - H7310BA 6842 => KO
> > > OrangePi Lite2 - H6448BA 6662 => KO
> > >
> > > Clément Péron board:
> > > Beelink GS1 - H7309BA 6842 => KO
> > >
> > > As it seems not fixable for now, declare the node
> > > but leave it disable with a comment.
> > >
> > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> >
> > If it doesn't work most boards, then why do we need to merge that
> > patch in the first place?
>
> My personnal opinion, is that having the IP declared and disabled with
> a comment saying "it's broken on some boards" in the device-tree is
> better than not having at all.
>
> This will explicit say "the IP exist but don't use it!".
> Maybe some people with a functionnal board would like to explicitly
> use it on their dts.

Yeah, that makes sense. Chen-Yu, any opinion on the matter?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Chen-Yu Tsai May 20, 2019, 2:57 p.m. UTC | #4 
On Mon, May 20, 2019 at 10:44 PM Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
>
> On Mon, May 20, 2019 at 10:21:40AM +0200, Clément Péron wrote:
> > Hi,
> >
> > On Mon, 20 May 2019 at 09:36, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Sat, May 18, 2019 at 05:23:53PM +0200, Clément Péron wrote:
> > > > Allwinner H6 has a watchog node which seems broken
> > > > on some boards.
> > > >
> > > > Test has been performed on several boards.
> > > >
> > > > Chen-Yu Tsai boards:
> > > > Pine H64 - H6448BA 7782 => OK
> > > > OrangePi Lite 2 - H8068BA 61C2 => KO
> > > >
> > > > Martin Ayotte boards:
> > > > Pine H64 - H8069BA 6892 => OK
> > > > OrangePi 3 - HA047BA 69W2 => KO
> > > > OrangePi One Plus - H7310BA 6842 => KO
> > > > OrangePi Lite2 - H6448BA 6662 => KO
> > > >
> > > > Clément Péron board:
> > > > Beelink GS1 - H7309BA 6842 => KO
> > > >
> > > > As it seems not fixable for now, declare the node
> > > > but leave it disable with a comment.
> > > >
> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > >
> > > If it doesn't work most boards, then why do we need to merge that
> > > patch in the first place?
> >
> > My personnal opinion, is that having the IP declared and disabled with
> > a comment saying "it's broken on some boards" in the device-tree is
> > better than not having at all.
> >
> > This will explicit say "the IP exist but don't use it!".
> > Maybe some people with a functionnal board would like to explicitly
> > use it on their dts.
>
> Yeah, that makes sense. Chen-Yu, any opinion on the matter?

Works for me.
Clément Péron May 20, 2019, 3:19 p.m. UTC | #5 
Hi Maxime,

On Sat, 18 May 2019 at 17:24, 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 | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> index 60b47f23b2f5..27647e496269 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> @@ -632,6 +632,14 @@
>                         #reset-cells = <1>;
>                 };
>
> +               r_watchdog: watchdog@7020400 {
> +                       compatible = "allwinner,sun50i-h6-wdt",
> +                                    "allwinner,sun50i-a64-wdt",
> +                                    "allwinner,sun6i-a31-wdt";
> +                       reg = <0x07020400 0x20>;
> +                       interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;

Just want to point out that i have used the same bindings for WDT and R_WDT.
I think it would be better to also introduce a bindings like
"allwinner,sun50i-h6-r-wdt", "allwinner,sun6i-a31-wdt";

We don't have access to the datasheet of this IP but we can strongly
suppose that wdt and r-wdt are the same.

What do you think?

Regards,
Clement


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