[U-Boot] [PATCH 0/3] fix bcm6345 watchdog on broadcom board
Since the commit: commit 06985289d452 ("watchdog: Implement generic watchdog_reset() version"), the watchdog is always started and a default timeout of 60000 ms is used. But the driver for the bcm6345 watchdog use this timeout in ms as tick. So a board using this driver reboot immediately.
The first commit in this serie fix the driver of the bcm6345 watchdog by converting the timeout in ms to tick before writing the register. The two others commits fix the clock used by boards bcm96858xref and bcm963158.
This serie was tested on: - bcm6838 (mips) - bcm96858xref (arm) - bcm963158 (arm)
Philippe Reynes (3): watchdog: bcm6345: callback start use tick instead of ms dt: bcm6858: watchdog should use a 50Mhz clock dt: bcm63158: watchdog should use a 50Mhz clock
arch/arm/dts/bcm63158.dtsi | 10 ++++++++-- arch/arm/dts/bcm6858.dtsi | 10 ++++++++-- drivers/watchdog/bcm6345_wdt.c | 21 ++++++++++++++++----- 3 files changed, 32 insertions(+), 9 deletions(-)
The function bcm6345_wdt_start use the argument timeout as tick but it should be used as milliseconds.
A clock is added as requirement for this driver. The frequency of the clock is then used to convert the millisecond to ticks in the function bcm6345_wdt_start.
Signed-off-by: Philippe Reynes philippe.reynes@softathome.com --- drivers/watchdog/bcm6345_wdt.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/watchdog/bcm6345_wdt.c b/drivers/watchdog/bcm6345_wdt.c index 44f5662..9f14e7d 100644 --- a/drivers/watchdog/bcm6345_wdt.c +++ b/drivers/watchdog/bcm6345_wdt.c @@ -10,6 +10,7 @@ #include <common.h> #include <dm.h> #include <wdt.h> +#include <clk.h> #include <asm/io.h>
/* WDT Value register */ @@ -26,6 +27,7 @@
struct bcm6345_wdt_priv { void __iomem *regs; + unsigned long clk_rate; };
static int bcm6345_wdt_reset(struct udevice *dev) @@ -41,16 +43,17 @@ static int bcm6345_wdt_reset(struct udevice *dev) static int bcm6345_wdt_start(struct udevice *dev, u64 timeout, ulong flags) { struct bcm6345_wdt_priv *priv = dev_get_priv(dev); + u32 val = priv->clk_rate / 1000 * timeout;
- if (timeout < WDT_VAL_MIN) { + if (val < WDT_VAL_MIN) { debug("watchdog won't fire with less than 2 ticks\n"); - timeout = WDT_VAL_MIN; - } else if (timeout > WDT_VAL_MAX) { + val = WDT_VAL_MIN; + } else if (val > WDT_VAL_MAX) { debug("maximum watchdog timeout exceeded\n"); - timeout = WDT_VAL_MAX; + val = WDT_VAL_MAX; }
- writel(timeout, priv->regs + WDT_VAL_REG); + writel(val, priv->regs + WDT_VAL_REG);
return bcm6345_wdt_reset(dev); } @@ -85,11 +88,19 @@ static const struct udevice_id bcm6345_wdt_ids[] = { static int bcm6345_wdt_probe(struct udevice *dev) { struct bcm6345_wdt_priv *priv = dev_get_priv(dev); + struct clk clk; + int ret;
priv->regs = dev_remap_addr(dev); if (!priv->regs) return -EINVAL;
+ ret = clk_get_by_index(dev, 0, &clk); + if (!ret) + priv->clk_rate = clk_get_rate(&clk); + else + return -EINVAL; + bcm6345_wdt_stop(dev);
return 0;
On 03.05.19 19:43, Philippe Reynes wrote:
The function bcm6345_wdt_start use the argument timeout as tick but it should be used as milliseconds.
A clock is added as requirement for this driver. The frequency of the clock is then used to convert the millisecond to ticks in the function bcm6345_wdt_start.
Signed-off-by: Philippe Reynes philippe.reynes@softathome.com
You might want to emphasize, that this is a pretty severe fix to this driver, as without this patch, boards using this driver will end in a endless reboot loop. So it should be applied to this release.
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
drivers/watchdog/bcm6345_wdt.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/watchdog/bcm6345_wdt.c b/drivers/watchdog/bcm6345_wdt.c index 44f5662..9f14e7d 100644 --- a/drivers/watchdog/bcm6345_wdt.c +++ b/drivers/watchdog/bcm6345_wdt.c @@ -10,6 +10,7 @@ #include <common.h> #include <dm.h> #include <wdt.h> +#include <clk.h> #include <asm/io.h>
/* WDT Value register */ @@ -26,6 +27,7 @@
struct bcm6345_wdt_priv { void __iomem *regs;
unsigned long clk_rate; };
static int bcm6345_wdt_reset(struct udevice *dev)
@@ -41,16 +43,17 @@ static int bcm6345_wdt_reset(struct udevice *dev) static int bcm6345_wdt_start(struct udevice *dev, u64 timeout, ulong flags) { struct bcm6345_wdt_priv *priv = dev_get_priv(dev);
- u32 val = priv->clk_rate / 1000 * timeout;
- if (timeout < WDT_VAL_MIN) {
- if (val < WDT_VAL_MIN) { debug("watchdog won't fire with less than 2 ticks\n");
timeout = WDT_VAL_MIN;- } else if (timeout > WDT_VAL_MAX) {
val = WDT_VAL_MIN;- } else if (val > WDT_VAL_MAX) { debug("maximum watchdog timeout exceeded\n");
timeout = WDT_VAL_MAX;
val = WDT_VAL_MAX;
- writel(timeout, priv->regs + WDT_VAL_REG);
writel(val, priv->regs + WDT_VAL_REG);
return bcm6345_wdt_reset(dev); }
@@ -85,11 +88,19 @@ static const struct udevice_id bcm6345_wdt_ids[] = { static int bcm6345_wdt_probe(struct udevice *dev) { struct bcm6345_wdt_priv *priv = dev_get_priv(dev);
struct clk clk;
int ret;
priv->regs = dev_remap_addr(dev); if (!priv->regs) return -EINVAL;
ret = clk_get_by_index(dev, 0, &clk);
if (!ret)
priv->clk_rate = clk_get_rate(&clk);else
return -EINVAL;bcm6345_wdt_stop(dev);
return 0;
Viele Grüße, Stefan
On Fri, May 03, 2019 at 07:43:06PM +0200, Philippe Reynes wrote:
The function bcm6345_wdt_start use the argument timeout as tick but it should be used as milliseconds.
A clock is added as requirement for this driver. The frequency of the clock is then used to convert the millisecond to ticks in the function bcm6345_wdt_start.
Signed-off-by: Philippe Reynes philippe.reynes@softathome.com Reviewed-by: Stefan Roese sr@denx.de
Applied to u-boot/master, thanks!
The watchdog should use a clock at 50 Mhz, so instead of using the clock osc (200 Mhz), we define a reference clock at 50Mhz and use it for both watchdog.
Signed-off-by: Philippe Reynes philippe.reynes@softathome.com --- arch/arm/dts/bcm6858.dtsi | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/arm/dts/bcm6858.dtsi b/arch/arm/dts/bcm6858.dtsi index 76ba0ea..91f7787 100644 --- a/arch/arm/dts/bcm6858.dtsi +++ b/arch/arm/dts/bcm6858.dtsi @@ -66,6 +66,12 @@ clock-frequency = <200000000>; u-boot,dm-pre-reloc; }; + + refclk50mhz: refclk50mhz { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <50000000>; + }; };
ubus { @@ -92,13 +98,13 @@ wdt1: watchdog@ff802780 { compatible = "brcm,bcm6345-wdt"; reg = <0x0 0xff802780 0x0 0x14>; - clocks = <&periph_osc>; + clocks = <&refclk50mhz>; };
wdt2: watchdog@ff8027c0 { compatible = "brcm,bcm6345-wdt"; reg = <0x0 0xff8027c0 0x0 0x14>; - clocks = <&periph_osc>; + clocks = <&refclk50mhz>; };
wdt-reboot {
On 03.05.19 19:43, Philippe Reynes wrote:
The watchdog should use a clock at 50 Mhz, so instead of using the clock osc (200 Mhz), we define a reference clock at 50Mhz and use it for both watchdog.
Just curious: Why is this the case? Is this also what's done in the Linux DT version?
Other than that:
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
Signed-off-by: Philippe Reynes philippe.reynes@softathome.com
arch/arm/dts/bcm6858.dtsi | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/arm/dts/bcm6858.dtsi b/arch/arm/dts/bcm6858.dtsi index 76ba0ea..91f7787 100644 --- a/arch/arm/dts/bcm6858.dtsi +++ b/arch/arm/dts/bcm6858.dtsi @@ -66,6 +66,12 @@ clock-frequency = <200000000>; u-boot,dm-pre-reloc; };
refclk50mhz: refclk50mhz {compatible = "fixed-clock";#clock-cells = <0>;clock-frequency = <50000000>;};};
ubus {
@@ -92,13 +98,13 @@ wdt1: watchdog@ff802780 { compatible = "brcm,bcm6345-wdt"; reg = <0x0 0xff802780 0x0 0x14>;
clocks = <&periph_osc>;
clocks = <&refclk50mhz>;};
wdt2: watchdog@ff8027c0 { compatible = "brcm,bcm6345-wdt"; reg = <0x0 0xff8027c0 0x0 0x14>;
clocks = <&periph_osc>;
clocks = <&refclk50mhz>;};
wdt-reboot {
Viele Grüße, Stefan
On Fri, May 03, 2019 at 07:43:07PM +0200, Philippe Reynes wrote:
The watchdog should use a clock at 50 Mhz, so instead of using the clock osc (200 Mhz), we define a reference clock at 50Mhz and use it for both watchdog.
Signed-off-by: Philippe Reynes philippe.reynes@softathome.com Reviewed-by: Stefan Roese sr@denx.de
Applied to u-boot/master, thanks!
The watchdog should use a clock at 50 Mhz, so instead of using the clock osc (200 Mhz), we define a reference clock at 50Mhz and use it for both watchdog.
Signed-off-by: Philippe Reynes philippe.reynes@softathome.com --- arch/arm/dts/bcm63158.dtsi | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/arm/dts/bcm63158.dtsi b/arch/arm/dts/bcm63158.dtsi index 4b2eaee..175af38 100644 --- a/arch/arm/dts/bcm63158.dtsi +++ b/arch/arm/dts/bcm63158.dtsi @@ -66,6 +66,12 @@ clock-frequency = <0xbebc200>; u-boot,dm-pre-reloc; }; + + refclk50mhz: refclk50mhz { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <50000000>; + }; };
ubus { @@ -92,13 +98,13 @@ wdt1: watchdog@ff800480 { compatible = "brcm,bcm6345-wdt"; reg = <0x0 0xff800480 0x0 0x14>; - clocks = <&periph_osc>; + clocks = <&refclk50mhz>; };
wdt2: watchdog@ff8004c0 { compatible = "brcm,bcm6345-wdt"; reg = <0x0 0xff8004c0 0x0 0x14>; - clocks = <&periph_osc>; + clocks = <&refclk50mhz>; };
wdt-reboot {
On 03.05.19 19:43, Philippe Reynes wrote:
The watchdog should use a clock at 50 Mhz, so instead of using the clock osc (200 Mhz), we define a reference clock at 50Mhz and use it for both watchdog.
Signed-off-by: Philippe Reynes philippe.reynes@softathome.com
Just curious: Why is this the case? Is this also what's done in the Linux DT version?
Other than that:
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
arch/arm/dts/bcm63158.dtsi | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/arm/dts/bcm63158.dtsi b/arch/arm/dts/bcm63158.dtsi index 4b2eaee..175af38 100644 --- a/arch/arm/dts/bcm63158.dtsi +++ b/arch/arm/dts/bcm63158.dtsi @@ -66,6 +66,12 @@ clock-frequency = <0xbebc200>; u-boot,dm-pre-reloc; };
refclk50mhz: refclk50mhz {compatible = "fixed-clock";#clock-cells = <0>;clock-frequency = <50000000>;};};
ubus {
@@ -92,13 +98,13 @@ wdt1: watchdog@ff800480 { compatible = "brcm,bcm6345-wdt"; reg = <0x0 0xff800480 0x0 0x14>;
clocks = <&periph_osc>;
clocks = <&refclk50mhz>;};
wdt2: watchdog@ff8004c0 { compatible = "brcm,bcm6345-wdt"; reg = <0x0 0xff8004c0 0x0 0x14>;
clocks = <&periph_osc>;
clocks = <&refclk50mhz>;};
wdt-reboot {
Viele Grüße, Stefan
Hi Stefan,
On 03.05.19 19:43, Philippe Reynes wrote:
The watchdog should use a clock at 50 Mhz, so instead of using the clock osc (200 Mhz), we define a reference clock at 50Mhz and use it for both watchdog.
Signed-off-by: Philippe Reynes philippe.reynes@softathome.com
Just curious: Why is this the case? Is this also what's done in the Linux DT version?
From my understanding, in the linux kernel, the driver doesn't compute
the timeout for the watchdog counter register. Every second, the driver set the maximum value in the watchdog counter register and compute a logical tick. If this tick decrease below zero, the watchdog isn't restarted, so when the watchdog counter reach zero, the board is resetted.
In u-boot, the driver compute the expected timeout and set it in the watchdog register.
Other than that:
Reviewed-by: Stefan Roese sr@denx.de
Thanks
Thanks, Stefan
Regards, Philippe
arch/arm/dts/bcm63158.dtsi | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/arm/dts/bcm63158.dtsi b/arch/arm/dts/bcm63158.dtsi index 4b2eaee..175af38 100644 --- a/arch/arm/dts/bcm63158.dtsi +++ b/arch/arm/dts/bcm63158.dtsi @@ -66,6 +66,12 @@ clock-frequency = <0xbebc200>; u-boot,dm-pre-reloc; };
- refclk50mhz: refclk50mhz {
- compatible = "fixed-clock";
- #clock-cells = <0>;
- clock-frequency = <50000000>;
- };
};
ubus { @@ -92,13 +98,13 @@ wdt1: watchdog@ff800480 { compatible = "brcm,bcm6345-wdt"; reg = <0x0 0xff800480 0x0 0x14>;
- clocks = <&periph_osc>;
- clocks = <&refclk50mhz>;
};
wdt2: watchdog@ff8004c0 { compatible = "brcm,bcm6345-wdt"; reg = <0x0 0xff8004c0 0x0 0x14>;
- clocks = <&periph_osc>;
- clocks = <&refclk50mhz>;
};
wdt-reboot {
Viele Grüße, Stefan
-- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
Hi Philippe,
On 06.05.19 14:38, Philippe REYNES wrote:
Hi Stefan,
On 03.05.19 19:43, Philippe Reynes wrote:
The watchdog should use a clock at 50 Mhz, so instead of using the clock osc (200 Mhz), we define a reference clock at 50Mhz and use it for both watchdog.
Signed-off-by: Philippe Reynes philippe.reynes@softathome.com
Just curious: Why is this the case? Is this also what's done in the Linux DT version?
From my understanding, in the linux kernel, the driver doesn't compute the timeout for the watchdog counter register. Every second, the driver set the maximum value in the watchdog counter register and compute a logical tick. If this tick decrease below zero, the watchdog isn't restarted, so when the watchdog counter reach zero, the board is resetted.
In u-boot, the driver compute the expected timeout and set it in the watchdog register.
I see. But why "should the watchdog use a clock at 50MHz" instead of the default 200MHz (from your commit text)?
I'm checking as this change most likely results in a DT difference in the U-Boot vs the Linux version, which should be avoided.
Thanks, Stefan
Hi Stefan,
Hi Philippe,
On 06.05.19 14:38, Philippe REYNES wrote:
Hi Stefan,
On 03.05.19 19:43, Philippe Reynes wrote:
The watchdog should use a clock at 50 Mhz, so instead of using the clock osc (200 Mhz), we define a reference clock at 50Mhz and use it for both watchdog.
Signed-off-by: Philippe Reynes philippe.reynes@softathome.com
Just curious: Why is this the case? Is this also what's done in the Linux DT version?
From my understanding, in the linux kernel, the driver doesn't compute the timeout for the watchdog counter register. Every second, the driver set the maximum value in the watchdog counter register and compute a logical tick. If this tick decrease below zero, the watchdog isn't restarted, so when the watchdog counter reach zero, the board is resetted.
In u-boot, the driver compute the expected timeout and set it in the watchdog register.
I see. But why "should the watchdog use a clock at 50MHz" instead of the default 200MHz (from your commit text)?
In the patch "fix bcm6345 watchdog on broadcom board", I've updated the watchdog driver for bcm6345 to compute the number of tick for the counter in the watchdog. For this computation, I need the frequency of the clock used by the watchdog. As this IP is used on many SoC (mips, arm), I've added a clock in the device tree so it's could be more generic.
I'm checking as this change most likely results in a DT difference in the U-Boot vs the Linux version, which should be avoided.
Thanks, Stefan
Regards, Philippe
On Fri, May 03, 2019 at 07:43:08PM +0200, Philippe Reynes wrote:
The watchdog should use a clock at 50 Mhz, so instead of using the clock osc (200 Mhz), we define a reference clock at 50Mhz and use it for both watchdog.
Signed-off-by: Philippe Reynes philippe.reynes@softathome.com Reviewed-by: Stefan Roese sr@denx.de
Applied to u-boot/master, thanks!
participants (4)
-
Philippe REYNES -
Philippe Reynes
-
Stefan Roese -
Tom Rini