* [PATCH v3 2/2] renesas: Renesas R-Car Gen4 watchdog driver
@ 2025-06-03 3:06 ` Shmuel Leib Melamud via B4 Relay
0 siblings, 0 replies; 12+ messages in thread
From: Shmuel Leib Melamud via B4 Relay @ 2025-06-03 3:06 UTC (permalink / raw)
To: Nobuhiro Iwamatsu, Marek Vasut, Lukasz Majewski, Sean Anderson,
Tom Rini, Stefan Roese, Mattijs Korpershoek
Cc: u-boot, Shmuel Leib Melamud
From: Shmuel Leib Melamud <smelamud@redhat.com>
Add support of Renesas R-Car Gen4 watchdog timer. Timeouts up to
8184.0s are supported (CKS1 register is not involved). The watchdog
uses the clock of type CLK_TYPE_GEN4_MDSEL.
The timeout is set in
dts/upstream/src/arm64/renesas/r8a779f0-spider-cpu.dtsi section &rwdt.
This driver is based on upstream linux commit:
e70140ba0d2b("Get rid of 'remove_new' relic from platform driver struct")
Signed-off-by: Shmuel Leib Melamud <smelamud@redhat.com>
---
drivers/watchdog/Kconfig | 8 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/renesas_wdt.c | 182 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 191 insertions(+)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 1bb67f5035231df9f6ce01adb08d074855393143..a0f2948335f9c3b74b7823f039e578ff4030e97c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -335,6 +335,14 @@ config WDT_K3_RTI_FW_FILE
endif
+config WDT_RENESAS
+ bool "Renesas watchdog timer support"
+ depends on WDT && R8A779F0
+ select CLK
+ select CLK_RENESAS
+ help
+ Enables Renesas SoC R8A779F0 watchdog timer support.
+
config WDT_SANDBOX
bool "Enable Watchdog Timer support for Sandbox"
depends on SANDBOX && WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index e6bd4c587af6133c405dde6dbada8050debc781c..c4467d6e126d0c3e119bf8014b302e5b1432f541 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_WDT_MTK) += mtk_wdt.o
obj-$(CONFIG_WDT_NPCM) += npcm_wdt.o
obj-$(CONFIG_WDT_OCTEONTX) += octeontx_wdt.o
obj-$(CONFIG_WDT_OMAP3) += omap_wdt.o
+obj-$(CONFIG_WDT_RENESAS) += renesas_wdt.o
obj-$(CONFIG_WDT_SBSA) += sbsa_gwdt.o
obj-$(CONFIG_WDT_K3_RTI) += rti_wdt.o
obj-$(CONFIG_WDT_SIEMENS_PMIC) += siemens_pmic_wdt.o
diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
new file mode 100644
index 0000000000000000000000000000000000000000..5dc5b0b51e0d19589ea96e7a66d4b790d377e7f8
--- /dev/null
+++ b/drivers/watchdog/renesas_wdt.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright 2025 Red Hat, Inc., Shmuel Leib Melamud <smelamud@redhat.com>
+
+#include <clk.h>
+#include <dm.h>
+#include <wdt.h>
+#include <asm/io.h>
+#include <dm/device_compat.h>
+#include <linux/delay.h>
+#include <linux/iopoll.h>
+
+#define usleep_range(a, b) udelay((b))
+
+struct rwdt {
+ u32 cnt;
+ u32 csra;
+ u32 csrb;
+};
+
+#define RWTCSRA_WOVF BIT(4)
+#define RWTCSRA_WRFLG BIT(5)
+#define RWTCSRA_TME BIT(7)
+
+#define CSR_MASK 0xA5A5A500
+#define CNT_MASK 0x5A5A0000
+
+/*
+ * In probe, clk_rate is checked to be not more than 16 bit * biggest clock
+ * divider (12 bits). d is only a factor to fully utilize the WDT counter and
+ * will not exceed its 16 bits. Thus, no overflow, we stay below 32 bits.
+ */
+#define MUL_BY_CLKS_PER_SEC(p, d) \
+ DIV_ROUND_UP((d) * (p)->clk_rate, clk_divs[(p)->cks])
+
+/* d is 16 bit, clk_divs 12 bit -> no 32 bit overflow */
+#define DIV_BY_CLKS_PER_SEC(p, d) ((d) * clk_divs[(p)->cks] / (p)->clk_rate)
+
+static const unsigned int clk_divs[] = { 1, 4, 16, 32, 64, 128, 1024, 4096 };
+
+struct rwdt_priv {
+ struct rwdt __iomem *wdt;
+ unsigned long clk_rate;
+ u8 cks;
+ struct clk clk;
+};
+
+static void rwdt_wait_cycles(struct rwdt_priv *priv, unsigned int cycles)
+{
+ unsigned int delay;
+
+ delay = DIV_ROUND_UP(cycles * 1000000, priv->clk_rate);
+
+ usleep_range(delay, 2 * delay);
+}
+
+static int rwdt_start(struct udevice *dev, u64 timeout, ulong flags)
+{
+ struct rwdt_priv *priv = dev_get_priv(dev);
+ u64 max_timeout;
+ u8 val;
+
+ max_timeout = DIV_BY_CLKS_PER_SEC(priv, 65536);
+ timeout = min(max_timeout, timeout / 1000);
+
+ /* Stop the timer before we modify any register */
+ val = readb_relaxed(&priv->wdt->csra) & ~RWTCSRA_TME;
+ writel_relaxed(val | CSR_MASK, &priv->wdt->csra);
+ /* Delay 2 cycles before setting watchdog counter */
+ rwdt_wait_cycles(priv, 2);
+
+ while (readb_relaxed(&priv->wdt->csra) & RWTCSRA_WRFLG)
+ cpu_relax();
+
+ writel_relaxed((65536 - MUL_BY_CLKS_PER_SEC(priv, timeout)) | CNT_MASK,
+ &priv->wdt->cnt);
+
+ writel_relaxed(priv->cks | RWTCSRA_TME | CSR_MASK, &priv->wdt->csra);
+
+ return 0;
+}
+
+static int rwdt_stop(struct udevice *dev)
+{
+ struct rwdt_priv *priv = dev_get_priv(dev);
+
+ writel_relaxed(priv->cks | CSR_MASK, &priv->wdt->csra);
+
+ return 0;
+}
+
+static int rwdt_reset(struct udevice *dev)
+{
+ struct rwdt_priv *priv = dev_get_priv(dev);
+ u8 val;
+
+ /* Stop the timer before we modify any register */
+ val = readb_relaxed(&priv->wdt->csra) & ~RWTCSRA_TME;
+ writel_relaxed(val | CSR_MASK, &priv->wdt->csra);
+ /* Delay 2 cycles before setting watchdog counter */
+ rwdt_wait_cycles(priv, 2);
+
+ writel_relaxed(0xffff | CNT_MASK, &priv->wdt->cnt);
+ /* smallest divider to reboot soon */
+ writel_relaxed(0 | CSR_MASK, &priv->wdt->csra);
+
+ readb_poll_timeout(&priv->wdt->csra, val, !(val & RWTCSRA_WRFLG), 100);
+
+ writel_relaxed(RWTCSRA_TME | CSR_MASK, &priv->wdt->csra);
+
+ /* wait 2 cycles, so watchdog will trigger */
+ rwdt_wait_cycles(priv, 2);
+
+ return 0;
+}
+
+static int rwdt_probe(struct udevice *dev)
+{
+ struct rwdt_priv *priv = dev_get_priv(dev);
+ unsigned long clks_per_sec;
+ int ret, i;
+
+ priv->wdt = dev_remap_addr(dev);
+ if (!priv->wdt)
+ return -EINVAL;
+
+ ret = clk_get_by_index(dev, 0, &priv->clk);
+ if (ret < 0)
+ return ret;
+
+ ret = clk_enable(&priv->clk);
+ if (ret)
+ return ret;
+
+ priv->clk_rate = clk_get_rate(&priv->clk);
+ if (!priv->clk_rate) {
+ ret = -ENOENT;
+ goto err_clk_disable;
+ }
+
+ for (i = ARRAY_SIZE(clk_divs) - 1; i >= 0; i--) {
+ clks_per_sec = priv->clk_rate / clk_divs[i];
+ if (clks_per_sec && clks_per_sec < 65536) {
+ priv->cks = i;
+ break;
+ }
+ }
+
+ /* can't find a suitable clock divider */
+ if (i < 0) {
+ ret = -ERANGE;
+ goto err_clk_disable;
+ }
+
+ return 0;
+
+err_clk_disable:
+ clk_disable(&priv->clk);
+
+ return ret;
+}
+
+static const struct wdt_ops rwdt_ops = {
+ .start = rwdt_start,
+ .reset = rwdt_reset,
+ .stop = rwdt_stop,
+};
+
+static const struct udevice_id rwdt_ids[] = {
+ { .compatible = "renesas,rcar-gen2-wdt" },
+ { .compatible = "renesas,rcar-gen3-wdt" },
+ { .compatible = "renesas,rcar-gen4-wdt" },
+ {}
+};
+
+U_BOOT_DRIVER(wdt_renesas) = {
+ .name = "wdt_renesas",
+ .id = UCLASS_WDT,
+ .of_match = rwdt_ids,
+ .ops = &rwdt_ops,
+ .probe = rwdt_probe,
+ .priv_auto = sizeof(struct rwdt_priv),
+};
--
2.49.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 2/2] renesas: Renesas R-Car Gen4 watchdog driver
2025-06-03 3:06 ` Shmuel Leib Melamud via B4 Relay
(?)
@ 2025-06-04 8:52 ` Mattijs Korpershoek
2025-06-05 3:33 ` Shmuel Melamud
-1 siblings, 1 reply; 12+ messages in thread
From: Mattijs Korpershoek @ 2025-06-04 8:52 UTC (permalink / raw)
To: Shmuel Leib Melamud via B4 Relay, Nobuhiro Iwamatsu, Marek Vasut,
Lukasz Majewski, Sean Anderson, Tom Rini, Stefan Roese,
Mattijs Korpershoek
Cc: u-boot, Shmuel Leib Melamud
Hi Shmuel,
Thank you for the patch.
On Tue, Jun 03, 2025 at 06:06, Shmuel Leib Melamud via B4 Relay <devnull+smelamud.redhat.com@kernel.org> wrote:
> From: Shmuel Leib Melamud <smelamud@redhat.com>
>
> Add support of Renesas R-Car Gen4 watchdog timer. Timeouts up to
> 8184.0s are supported (CKS1 register is not involved). The watchdog
> uses the clock of type CLK_TYPE_GEN4_MDSEL.
>
> The timeout is set in
> dts/upstream/src/arm64/renesas/r8a779f0-spider-cpu.dtsi section &rwdt.
>
> This driver is based on upstream linux commit:
> e70140ba0d2b("Get rid of 'remove_new' relic from platform driver struct")
>
> Signed-off-by: Shmuel Leib Melamud <smelamud@redhat.com>
> ---
> drivers/watchdog/Kconfig | 8 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/renesas_wdt.c | 182 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 191 insertions(+)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 1bb67f5035231df9f6ce01adb08d074855393143..a0f2948335f9c3b74b7823f039e578ff4030e97c 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -335,6 +335,14 @@ config WDT_K3_RTI_FW_FILE
>
> endif
>
> +config WDT_RENESAS
> + bool "Renesas watchdog timer support"
> + depends on WDT && R8A779F0
> + select CLK
> + select CLK_RENESAS
> + help
> + Enables Renesas SoC R8A779F0 watchdog timer support.
> +
> config WDT_SANDBOX
> bool "Enable Watchdog Timer support for Sandbox"
> depends on SANDBOX && WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index e6bd4c587af6133c405dde6dbada8050debc781c..c4467d6e126d0c3e119bf8014b302e5b1432f541 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_WDT_MTK) += mtk_wdt.o
> obj-$(CONFIG_WDT_NPCM) += npcm_wdt.o
> obj-$(CONFIG_WDT_OCTEONTX) += octeontx_wdt.o
> obj-$(CONFIG_WDT_OMAP3) += omap_wdt.o
> +obj-$(CONFIG_WDT_RENESAS) += renesas_wdt.o
> obj-$(CONFIG_WDT_SBSA) += sbsa_gwdt.o
> obj-$(CONFIG_WDT_K3_RTI) += rti_wdt.o
> obj-$(CONFIG_WDT_SIEMENS_PMIC) += siemens_pmic_wdt.o
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..5dc5b0b51e0d19589ea96e7a66d4b790d377e7f8
> --- /dev/null
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright 2025 Red Hat, Inc., Shmuel Leib Melamud <smelamud@redhat.com>
> +
> +#include <clk.h>
> +#include <dm.h>
> +#include <wdt.h>
> +#include <asm/io.h>
> +#include <dm/device_compat.h>
> +#include <linux/delay.h>
> +#include <linux/iopoll.h>
> +
> +#define usleep_range(a, b) udelay((b))
> +
> +struct rwdt {
> + u32 cnt;
> + u32 csra;
> + u32 csrb;
> +};
> +
> +#define RWTCSRA_WOVF BIT(4)
> +#define RWTCSRA_WRFLG BIT(5)
> +#define RWTCSRA_TME BIT(7)
> +
> +#define CSR_MASK 0xA5A5A500
> +#define CNT_MASK 0x5A5A0000
> +
> +/*
> + * In probe, clk_rate is checked to be not more than 16 bit * biggest clock
> + * divider (12 bits). d is only a factor to fully utilize the WDT counter and
> + * will not exceed its 16 bits. Thus, no overflow, we stay below 32 bits.
> + */
> +#define MUL_BY_CLKS_PER_SEC(p, d) \
> + DIV_ROUND_UP((d) * (p)->clk_rate, clk_divs[(p)->cks])
> +
> +/* d is 16 bit, clk_divs 12 bit -> no 32 bit overflow */
> +#define DIV_BY_CLKS_PER_SEC(p, d) ((d) * clk_divs[(p)->cks] / (p)->clk_rate)
> +
> +static const unsigned int clk_divs[] = { 1, 4, 16, 32, 64, 128, 1024, 4096 };
> +
> +struct rwdt_priv {
> + struct rwdt __iomem *wdt;
> + unsigned long clk_rate;
> + u8 cks;
> + struct clk clk;
> +};
> +
> +static void rwdt_wait_cycles(struct rwdt_priv *priv, unsigned int cycles)
> +{
> + unsigned int delay;
> +
> + delay = DIV_ROUND_UP(cycles * 1000000, priv->clk_rate);
> +
> + usleep_range(delay, 2 * delay);
> +}
> +
> +static int rwdt_start(struct udevice *dev, u64 timeout, ulong flags)
> +{
> + struct rwdt_priv *priv = dev_get_priv(dev);
> + u64 max_timeout;
> + u8 val;
> +
> + max_timeout = DIV_BY_CLKS_PER_SEC(priv, 65536);
Why 65536. This number is used here ...
> + timeout = min(max_timeout, timeout / 1000);
> +
> + /* Stop the timer before we modify any register */
> + val = readb_relaxed(&priv->wdt->csra) & ~RWTCSRA_TME;
> + writel_relaxed(val | CSR_MASK, &priv->wdt->csra);
> + /* Delay 2 cycles before setting watchdog counter */
> + rwdt_wait_cycles(priv, 2);
> +
> + while (readb_relaxed(&priv->wdt->csra) & RWTCSRA_WRFLG)
> + cpu_relax();
> +
> + writel_relaxed((65536 - MUL_BY_CLKS_PER_SEC(priv, timeout)) | CNT_MASK,
> + &priv->wdt->cnt);
... and here. Can we move it to a define to document its meaning?
> +
> + writel_relaxed(priv->cks | RWTCSRA_TME | CSR_MASK, &priv->wdt->csra);
I still see some subtle differences between linux driver (where a
rwdt_write() helper function is present) and this U-Boot driver but with
the rename, it's gotten much easier to review.
Thanks for the improvements !
> +
> + return 0;
> +}
> +
> +static int rwdt_stop(struct udevice *dev)
> +{
> + struct rwdt_priv *priv = dev_get_priv(dev);
> +
> + writel_relaxed(priv->cks | CSR_MASK, &priv->wdt->csra);
> +
> + return 0;
> +}
> +
> +static int rwdt_reset(struct udevice *dev)
> +{
> + struct rwdt_priv *priv = dev_get_priv(dev);
> + u8 val;
> +
> + /* Stop the timer before we modify any register */
> + val = readb_relaxed(&priv->wdt->csra) & ~RWTCSRA_TME;
> + writel_relaxed(val | CSR_MASK, &priv->wdt->csra);
> + /* Delay 2 cycles before setting watchdog counter */
> + rwdt_wait_cycles(priv, 2);
> +
> + writel_relaxed(0xffff | CNT_MASK, &priv->wdt->cnt);
> + /* smallest divider to reboot soon */
> + writel_relaxed(0 | CSR_MASK, &priv->wdt->csra);
> +
> + readb_poll_timeout(&priv->wdt->csra, val, !(val & RWTCSRA_WRFLG), 100);
> +
> + writel_relaxed(RWTCSRA_TME | CSR_MASK, &priv->wdt->csra);
> +
> + /* wait 2 cycles, so watchdog will trigger */
> + rwdt_wait_cycles(priv, 2);
> +
> + return 0;
> +}
> +
> +static int rwdt_probe(struct udevice *dev)
> +{
> + struct rwdt_priv *priv = dev_get_priv(dev);
> + unsigned long clks_per_sec;
> + int ret, i;
> +
> + priv->wdt = dev_remap_addr(dev);
> + if (!priv->wdt)
> + return -EINVAL;
> +
> + ret = clk_get_by_index(dev, 0, &priv->clk);
> + if (ret < 0)
> + return ret;
> +
> + ret = clk_enable(&priv->clk);
> + if (ret)
> + return ret;
> +
> + priv->clk_rate = clk_get_rate(&priv->clk);
> + if (!priv->clk_rate) {
> + ret = -ENOENT;
> + goto err_clk_disable;
> + }
> +
> + for (i = ARRAY_SIZE(clk_divs) - 1; i >= 0; i--) {
> + clks_per_sec = priv->clk_rate / clk_divs[i];
> + if (clks_per_sec && clks_per_sec < 65536) {
> + priv->cks = i;
> + break;
> + }
> + }
> +
> + /* can't find a suitable clock divider */
> + if (i < 0) {
> + ret = -ERANGE;
> + goto err_clk_disable;
> + }
> +
> + return 0;
> +
> +err_clk_disable:
> + clk_disable(&priv->clk);
> +
> + return ret;
> +}
> +
> +static const struct wdt_ops rwdt_ops = {
> + .start = rwdt_start,
> + .reset = rwdt_reset,
> + .stop = rwdt_stop,
> +};
> +
> +static const struct udevice_id rwdt_ids[] = {
> + { .compatible = "renesas,rcar-gen2-wdt" },
> + { .compatible = "renesas,rcar-gen3-wdt" },
> + { .compatible = "renesas,rcar-gen4-wdt" },
> + {}
> +};
> +
> +U_BOOT_DRIVER(wdt_renesas) = {
> + .name = "wdt_renesas",
> + .id = UCLASS_WDT,
> + .of_match = rwdt_ids,
> + .ops = &rwdt_ops,
> + .probe = rwdt_probe,
> + .priv_auto = sizeof(struct rwdt_priv),
> +};
>
> --
> 2.49.0
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3 2/2] renesas: Renesas R-Car Gen4 watchdog driver
2025-06-04 8:52 ` Mattijs Korpershoek
@ 2025-06-05 3:33 ` Shmuel Melamud
2025-06-05 9:06 ` Mattijs Korpershoek
0 siblings, 1 reply; 12+ messages in thread
From: Shmuel Melamud @ 2025-06-05 3:33 UTC (permalink / raw)
To: Mattijs Korpershoek
Cc: Shmuel Leib Melamud via B4 Relay, Nobuhiro Iwamatsu, Marek Vasut,
Lukasz Majewski, Sean Anderson, Tom Rini, Stefan Roese, u-boot
On Wed, Jun 4, 2025 at 12:02 PM Mattijs Korpershoek
<mkorpershoek@kernel.org> wrote:
>
> Hi Shmuel,
>
> Thank you for the patch.
>
> On Tue, Jun 03, 2025 at 06:06, Shmuel Leib Melamud via B4 Relay <devnull+smelamud.redhat.com@kernel.org> wrote:
>
> > From: Shmuel Leib Melamud <smelamud@redhat.com>
> >
> > Add support of Renesas R-Car Gen4 watchdog timer. Timeouts up to
> > 8184.0s are supported (CKS1 register is not involved). The watchdog
> > uses the clock of type CLK_TYPE_GEN4_MDSEL.
> >
> > The timeout is set in
> > dts/upstream/src/arm64/renesas/r8a779f0-spider-cpu.dtsi section &rwdt.
> >
> > This driver is based on upstream linux commit:
> > e70140ba0d2b("Get rid of 'remove_new' relic from platform driver struct")
> >
> > Signed-off-by: Shmuel Leib Melamud <smelamud@redhat.com>
> > ---
> > drivers/watchdog/Kconfig | 8 ++
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/renesas_wdt.c | 182 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 191 insertions(+)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 1bb67f5035231df9f6ce01adb08d074855393143..a0f2948335f9c3b74b7823f039e578ff4030e97c 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -335,6 +335,14 @@ config WDT_K3_RTI_FW_FILE
> >
> > endif
> >
> > +config WDT_RENESAS
> > + bool "Renesas watchdog timer support"
> > + depends on WDT && R8A779F0
> > + select CLK
> > + select CLK_RENESAS
> > + help
> > + Enables Renesas SoC R8A779F0 watchdog timer support.
> > +
> > config WDT_SANDBOX
> > bool "Enable Watchdog Timer support for Sandbox"
> > depends on SANDBOX && WDT
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index e6bd4c587af6133c405dde6dbada8050debc781c..c4467d6e126d0c3e119bf8014b302e5b1432f541 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -43,6 +43,7 @@ obj-$(CONFIG_WDT_MTK) += mtk_wdt.o
> > obj-$(CONFIG_WDT_NPCM) += npcm_wdt.o
> > obj-$(CONFIG_WDT_OCTEONTX) += octeontx_wdt.o
> > obj-$(CONFIG_WDT_OMAP3) += omap_wdt.o
> > +obj-$(CONFIG_WDT_RENESAS) += renesas_wdt.o
> > obj-$(CONFIG_WDT_SBSA) += sbsa_gwdt.o
> > obj-$(CONFIG_WDT_K3_RTI) += rti_wdt.o
> > obj-$(CONFIG_WDT_SIEMENS_PMIC) += siemens_pmic_wdt.o
> > diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..5dc5b0b51e0d19589ea96e7a66d4b790d377e7f8
> > --- /dev/null
> > +++ b/drivers/watchdog/renesas_wdt.c
> > @@ -0,0 +1,182 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +// Copyright 2025 Red Hat, Inc., Shmuel Leib Melamud <smelamud@redhat.com>
> > +
> > +#include <clk.h>
> > +#include <dm.h>
> > +#include <wdt.h>
> > +#include <asm/io.h>
> > +#include <dm/device_compat.h>
> > +#include <linux/delay.h>
> > +#include <linux/iopoll.h>
> > +
> > +#define usleep_range(a, b) udelay((b))
> > +
> > +struct rwdt {
> > + u32 cnt;
> > + u32 csra;
> > + u32 csrb;
> > +};
> > +
> > +#define RWTCSRA_WOVF BIT(4)
> > +#define RWTCSRA_WRFLG BIT(5)
> > +#define RWTCSRA_TME BIT(7)
> > +
> > +#define CSR_MASK 0xA5A5A500
> > +#define CNT_MASK 0x5A5A0000
> > +
> > +/*
> > + * In probe, clk_rate is checked to be not more than 16 bit * biggest clock
> > + * divider (12 bits). d is only a factor to fully utilize the WDT counter and
> > + * will not exceed its 16 bits. Thus, no overflow, we stay below 32 bits.
> > + */
> > +#define MUL_BY_CLKS_PER_SEC(p, d) \
> > + DIV_ROUND_UP((d) * (p)->clk_rate, clk_divs[(p)->cks])
> > +
> > +/* d is 16 bit, clk_divs 12 bit -> no 32 bit overflow */
> > +#define DIV_BY_CLKS_PER_SEC(p, d) ((d) * clk_divs[(p)->cks] / (p)->clk_rate)
> > +
> > +static const unsigned int clk_divs[] = { 1, 4, 16, 32, 64, 128, 1024, 4096 };
> > +
> > +struct rwdt_priv {
> > + struct rwdt __iomem *wdt;
> > + unsigned long clk_rate;
> > + u8 cks;
> > + struct clk clk;
> > +};
> > +
> > +static void rwdt_wait_cycles(struct rwdt_priv *priv, unsigned int cycles)
> > +{
> > + unsigned int delay;
> > +
> > + delay = DIV_ROUND_UP(cycles * 1000000, priv->clk_rate);
> > +
> > + usleep_range(delay, 2 * delay);
> > +}
> > +
> > +static int rwdt_start(struct udevice *dev, u64 timeout, ulong flags)
> > +{
> > + struct rwdt_priv *priv = dev_get_priv(dev);
> > + u64 max_timeout;
> > + u8 val;
> > +
> > + max_timeout = DIV_BY_CLKS_PER_SEC(priv, 65536);
>
> Why 65536. This number is used here ...
65536 here is the maximal value of the counter plus 1. If we divide it
by the number of impulses per second, we get the maximal timeout in
seconds.
> > + timeout = min(max_timeout, timeout / 1000);
> > +
> > + /* Stop the timer before we modify any register */
> > + val = readb_relaxed(&priv->wdt->csra) & ~RWTCSRA_TME;
> > + writel_relaxed(val | CSR_MASK, &priv->wdt->csra);
> > + /* Delay 2 cycles before setting watchdog counter */
> > + rwdt_wait_cycles(priv, 2);
> > +
> > + while (readb_relaxed(&priv->wdt->csra) & RWTCSRA_WRFLG)
> > + cpu_relax();
> > +
> > + writel_relaxed((65536 - MUL_BY_CLKS_PER_SEC(priv, timeout)) | CNT_MASK,
> > + &priv->wdt->cnt);
>
> ... and here. Can we move it to a define to document its meaning?
65536 here is for negation. Since the counter is incremented on every
impulse, the timeout is the number of impulses that are left till
overflow of the counter.
In both cases it is the maximal value of the counter, so maybe it
makes sense to add a define for it.
> > +
> > + writel_relaxed(priv->cks | RWTCSRA_TME | CSR_MASK, &priv->wdt->csra);
>
> I still see some subtle differences between linux driver (where a
> rwdt_write() helper function is present) and this U-Boot driver but with
> the rename, it's gotten much easier to review.
>
> Thanks for the improvements !
>
> > +
> > + return 0;
> > +}
> > +
> > +static int rwdt_stop(struct udevice *dev)
> > +{
> > + struct rwdt_priv *priv = dev_get_priv(dev);
> > +
> > + writel_relaxed(priv->cks | CSR_MASK, &priv->wdt->csra);
> > +
> > + return 0;
> > +}
> > +
> > +static int rwdt_reset(struct udevice *dev)
> > +{
> > + struct rwdt_priv *priv = dev_get_priv(dev);
> > + u8 val;
> > +
> > + /* Stop the timer before we modify any register */
> > + val = readb_relaxed(&priv->wdt->csra) & ~RWTCSRA_TME;
> > + writel_relaxed(val | CSR_MASK, &priv->wdt->csra);
> > + /* Delay 2 cycles before setting watchdog counter */
> > + rwdt_wait_cycles(priv, 2);
> > +
> > + writel_relaxed(0xffff | CNT_MASK, &priv->wdt->cnt);
> > + /* smallest divider to reboot soon */
> > + writel_relaxed(0 | CSR_MASK, &priv->wdt->csra);
> > +
> > + readb_poll_timeout(&priv->wdt->csra, val, !(val & RWTCSRA_WRFLG), 100);
> > +
> > + writel_relaxed(RWTCSRA_TME | CSR_MASK, &priv->wdt->csra);
> > +
> > + /* wait 2 cycles, so watchdog will trigger */
> > + rwdt_wait_cycles(priv, 2);
> > +
> > + return 0;
> > +}
> > +
> > +static int rwdt_probe(struct udevice *dev)
> > +{
> > + struct rwdt_priv *priv = dev_get_priv(dev);
> > + unsigned long clks_per_sec;
> > + int ret, i;
> > +
> > + priv->wdt = dev_remap_addr(dev);
> > + if (!priv->wdt)
> > + return -EINVAL;
> > +
> > + ret = clk_get_by_index(dev, 0, &priv->clk);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = clk_enable(&priv->clk);
> > + if (ret)
> > + return ret;
> > +
> > + priv->clk_rate = clk_get_rate(&priv->clk);
> > + if (!priv->clk_rate) {
> > + ret = -ENOENT;
> > + goto err_clk_disable;
> > + }
> > +
> > + for (i = ARRAY_SIZE(clk_divs) - 1; i >= 0; i--) {
> > + clks_per_sec = priv->clk_rate / clk_divs[i];
> > + if (clks_per_sec && clks_per_sec < 65536) {
> > + priv->cks = i;
> > + break;
> > + }
> > + }
> > +
> > + /* can't find a suitable clock divider */
> > + if (i < 0) {
> > + ret = -ERANGE;
> > + goto err_clk_disable;
> > + }
> > +
> > + return 0;
> > +
> > +err_clk_disable:
> > + clk_disable(&priv->clk);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct wdt_ops rwdt_ops = {
> > + .start = rwdt_start,
> > + .reset = rwdt_reset,
> > + .stop = rwdt_stop,
> > +};
> > +
> > +static const struct udevice_id rwdt_ids[] = {
> > + { .compatible = "renesas,rcar-gen2-wdt" },
> > + { .compatible = "renesas,rcar-gen3-wdt" },
> > + { .compatible = "renesas,rcar-gen4-wdt" },
> > + {}
> > +};
> > +
> > +U_BOOT_DRIVER(wdt_renesas) = {
> > + .name = "wdt_renesas",
> > + .id = UCLASS_WDT,
> > + .of_match = rwdt_ids,
> > + .ops = &rwdt_ops,
> > + .probe = rwdt_probe,
> > + .priv_auto = sizeof(struct rwdt_priv),
> > +};
> >
> > --
> > 2.49.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3 2/2] renesas: Renesas R-Car Gen4 watchdog driver
2025-06-05 3:33 ` Shmuel Melamud
@ 2025-06-05 9:06 ` Mattijs Korpershoek
0 siblings, 0 replies; 12+ messages in thread
From: Mattijs Korpershoek @ 2025-06-05 9:06 UTC (permalink / raw)
To: Shmuel Melamud, Mattijs Korpershoek
Cc: Shmuel Leib Melamud via B4 Relay, Nobuhiro Iwamatsu, Marek Vasut,
Lukasz Majewski, Sean Anderson, Tom Rini, Stefan Roese, u-boot
Hi Shmuel,
On jeu., juin 05, 2025 at 06:33, Shmuel Melamud <smelamud@redhat.com> wrote:
> On Wed, Jun 4, 2025 at 12:02 PM Mattijs Korpershoek
> <mkorpershoek@kernel.org> wrote:
>>
>> Hi Shmuel,
>>
>> Thank you for the patch.
>>
>> On Tue, Jun 03, 2025 at 06:06, Shmuel Leib Melamud via B4 Relay <devnull+smelamud.redhat.com@kernel.org> wrote:
>>
>> > From: Shmuel Leib Melamud <smelamud@redhat.com>
[...]
>> >
>> > +
>> > +static int rwdt_start(struct udevice *dev, u64 timeout, ulong flags)
>> > +{
>> > + struct rwdt_priv *priv = dev_get_priv(dev);
>> > + u64 max_timeout;
>> > + u8 val;
>> > +
>> > + max_timeout = DIV_BY_CLKS_PER_SEC(priv, 65536);
>>
>> Why 65536. This number is used here ...
>
> 65536 here is the maximal value of the counter plus 1. If we divide it
> by the number of impulses per second, we get the maximal timeout in
> seconds.
>
>> > + timeout = min(max_timeout, timeout / 1000);
>> > +
>> > + /* Stop the timer before we modify any register */
>> > + val = readb_relaxed(&priv->wdt->csra) & ~RWTCSRA_TME;
>> > + writel_relaxed(val | CSR_MASK, &priv->wdt->csra);
>> > + /* Delay 2 cycles before setting watchdog counter */
>> > + rwdt_wait_cycles(priv, 2);
>> > +
>> > + while (readb_relaxed(&priv->wdt->csra) & RWTCSRA_WRFLG)
>> > + cpu_relax();
>> > +
>> > + writel_relaxed((65536 - MUL_BY_CLKS_PER_SEC(priv, timeout)) | CNT_MASK,
>> > + &priv->wdt->cnt);
>>
>> ... and here. Can we move it to a define to document its meaning?
>
> 65536 here is for negation. Since the counter is incremented on every
> impulse, the timeout is the number of impulses that are left till
> overflow of the counter.
>
> In both cases it is the maximal value of the counter, so maybe it
> makes sense to add a define for it.
Yes, please move this to a #define. With this constant moved to a
#define, you can add the following to v4 patch:
Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
Thanks,
Mattijs
>
>> > +
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] renesas: Renesas R-Car Gen4 watchdog driver
2025-06-03 3:06 ` Shmuel Leib Melamud via B4 Relay
(?)
(?)
@ 2025-06-08 14:48 ` Marek Vasut
-1 siblings, 0 replies; 12+ messages in thread
From: Marek Vasut @ 2025-06-08 14:48 UTC (permalink / raw)
To: smelamud, Nobuhiro Iwamatsu, Marek Vasut, Lukasz Majewski,
Sean Anderson, Tom Rini, Stefan Roese, Mattijs Korpershoek
Cc: u-boot
On 6/3/25 5:06 AM, Shmuel Leib Melamud via B4 Relay wrote:
[...]
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright 2025 Red Hat, Inc., Shmuel Leib Melamud <smelamud@redhat.com>
> +
> +#include <clk.h>
> +#include <dm.h>
> +#include <wdt.h>
> +#include <asm/io.h>
> +#include <dm/device_compat.h>
> +#include <linux/delay.h>
> +#include <linux/iopoll.h>
Nitpick, keep the list sorted.
> +#define usleep_range(a, b) udelay((b))
Shouldn't this be udelay((a)) , the shorter (a) delay should be
sufficient in all cases, shouldn't it ?
[...]
> +static int rwdt_probe(struct udevice *dev)
> +{
> + struct rwdt_priv *priv = dev_get_priv(dev);
> + unsigned long clks_per_sec;
> + int ret, i;
> +
> + priv->wdt = dev_remap_addr(dev);
> + if (!priv->wdt)
> + return -EINVAL;
> +
> + ret = clk_get_by_index(dev, 0, &priv->clk);
> + if (ret < 0)
> + return ret;
> +
> + ret = clk_enable(&priv->clk);
> + if (ret)
> + return ret;
> +
> + priv->clk_rate = clk_get_rate(&priv->clk);
> + if (!priv->clk_rate) {
> + ret = -ENOENT;
> + goto err_clk_disable;
> + }
This loop below could use a code comment clarifying what this does.
> + for (i = ARRAY_SIZE(clk_divs) - 1; i >= 0; i--) {
> + clks_per_sec = priv->clk_rate / clk_divs[i];
> + if (clks_per_sec && clks_per_sec < 65536) {
> + priv->cks = i;
> + break;
> + }
> + }
A few basic nitpicks, very nice otherwise, thanks !
^ permalink raw reply [flat|nested] 12+ messages in thread