[PATCH] watchdog: Add a watchdog driver for Raspberry Pi boards

Etienne Dublé etienne.duble at imag.fr
Tue Jan 24 15:42:48 CET 2023


Hi Stefan,
Thanks for the feedback.
I will send a v2 to fix those issues.
Regards,
Etienne


Le 24/01/2023 à 14:38, Stefan Roese a écrit :
> Hi Etienne,
>
> On 1/24/23 14:20, Etienne Dublé wrote:
>> This driver supports the bcm2835 watchdog found on
>> Raspberry Pi boards.
>> It is derived from the Linux driver and was tested
>> on two Raspberry Pi board versions (B+ and 3B+).
>>
>> Signed-off-by: Etienne Dublé <etienne.duble at imag.fr>
>> ---
>>
>>   drivers/watchdog/Kconfig       |   9 +++
>>   drivers/watchdog/Makefile      |   1 +
>>   drivers/watchdog/bcm2835_wdt.c | 135 +++++++++++++++++++++++++++++++++
>>   3 files changed, 145 insertions(+)
>>   create mode 100644 drivers/watchdog/bcm2835_wdt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index f1b1cf63ca..06c0d630c8 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -30,6 +30,7 @@ config WATCHDOG_TIMEOUT_MSECS
>>       default 128000 if ARCH_MX7 || ARCH_VF610
>>       default 30000 if ARCH_SOCFPGA
>>       default 16000 if ARCH_SUNXI
>> +    default 15000 if ARCH_BCM283X
>>       default 60000
>>       help
>>         Watchdog timeout in msec
>> @@ -326,6 +327,14 @@ config WDT_SUNXI
>>       help
>>         Enable support for the watchdog timer in Allwinner sunxi SoCs.
>>   +config WDT_BCM2835
>> +    bool "Broadcom 2835 watchdog timer support"
>
> Your patch seems to have whitespace problems. Please make sure to send
> it e.g. via git send-email.
>
> More comments below...
>
>> +    depends on WDT && ARCH_BCM283X
>> +    default y
>> +    help
>> +      Enable support for the watchdog timer in Broadcom 283X SoCs such
>> +      as Raspberry Pi boards.
>> +
>>   config XILINX_TB_WATCHDOG
>>       bool "Xilinx Axi watchdog timer support"
>>       depends on WDT
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 446d961d7d..f99915960c 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -21,6 +21,7 @@ obj-$(CONFIG_WDT_APPLE) += apple_wdt.o
>>   obj-$(CONFIG_WDT_ARMADA_37XX) += armada-37xx-wdt.o
>>   obj-$(CONFIG_WDT_ASPEED) += ast_wdt.o
>>   obj-$(CONFIG_WDT_AST2600) += ast2600_wdt.o
>> +obj-$(CONFIG_WDT_BCM2835) += bcm2835_wdt.o
>>   obj-$(CONFIG_WDT_BCM6345) += bcm6345_wdt.o
>>   obj-$(CONFIG_WDT_BOOKE) += booke_wdt.o
>>   obj-$(CONFIG_WDT_CORTINA) += cortina_wdt.o
>> diff --git a/drivers/watchdog/bcm2835_wdt.c 
>> b/drivers/watchdog/bcm2835_wdt.c
>> new file mode 100644
>> index 0000000000..48535c003f
>> --- /dev/null
>> +++ b/drivers/watchdog/bcm2835_wdt.c
>> @@ -0,0 +1,135 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2013 Lubomir Rintel <lkundrak at v3.sk>
>> + * Copyright (C) 2023 Etienne Dublé (CNRS) <etienne.duble at imag.fr>
>> + *
>> + * This code is mostly derived from the linux driver.
>> + */
>> +
>> +#include <dm.h>
>> +#include <wdt.h>
>> +#include <asm/io.h>
>> +#include <linux/delay.h>
>> +
>> +#define PM_RSTC                    0x1c
>> +#define PM_WDOG                    0x24
>> +
>> +#define PM_PASSWORD                0x5a000000
>> +
>> +/* The hardware supports a maximum timeout value of 0xfffff ticks
>> + * (just below 16 seconds).
>> + * U-boot users specify the timeout as a number of milliseconds
>> + * by using variable CONFIG_WATCHDOG_TIMEOUT_MSECS.
>> + * The maximum value should be 15999 ms in our case.
>> + * However, u-boot internally converts this config value to seconds,
>> + * thus specifying 15999 actually means 15000 ms (0xf0000 ticks).
>> + */
>> +#define PM_WDOG_MAX_TICKS            0x000f0000
>> +#define PM_RSTC_WRCFG_CLR            0xffffffcf
>> +#define PM_RSTC_WRCFG_FULL_RESET        0x00000020
>> +#define PM_RSTC_RESET                0x00000102
>> +
>> +#define MS_TO_WDOG_TICKS(x) (((x) << 16) / 1000)
>> +
>> +struct bcm2835_wdt_priv {
>> +    void __iomem *base;
>> +};
>> +
>> +static u64 timeout_ticks = PM_WDOG_MAX_TICKS;
>
> Is this static variable really needed? Why not add this variable (if
> needed) into the priv struct above instead?
>
>> +
>> +static int bcm2835_wdt_start_ticks(struct udevice *dev,
>> +                   u64 timeout_ticks, ulong flags)
>> +{
>> +    struct bcm2835_wdt_priv *priv = dev_get_priv(dev);
>> +    void __iomem *base = priv->base;
>> +    u32 cur;
>> +
>> +    writel(PM_PASSWORD | timeout_ticks, base + PM_WDOG);
>> +    cur = readl(base + PM_RSTC);
>> +    writel(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
>> +          PM_RSTC_WRCFG_FULL_RESET, base + PM_RSTC);
>> +
>> +    return 0;
>> +}
>> +
>> +static int bcm2835_wdt_start(struct udevice *dev, u64 timeout_ms, 
>> ulong flags)
>> +{
>> +    timeout_ticks = MS_TO_WDOG_TICKS(timeout_ms);
>
> Again, there should be no need for this static variable. Please move it
> to the priv struct instead.
>
> Thanks,
> Stefan
>
>> +
>> +    if (timeout_ticks > PM_WDOG_MAX_TICKS) {
>> +        printf("WARNING: bcm2835_wdt cannot handle large timeout 
>> values.\n");
>> +        printf("     Setting the max value of 15000 ms instead.\n");
>> +        printf("     Set CONFIG_WATCHDOG_TIMEOUT_MSECS=15000 at most "
>> +                "to avoid this warning.\n");
>> +        timeout_ticks = PM_WDOG_MAX_TICKS;
>> +    }
>> +
>> +    return bcm2835_wdt_start_ticks(dev, timeout_ticks, flags);
>> +}
>> +
>> +static int bcm2835_wdt_reset(struct udevice *dev)
>> +{
>> +    /* restart the timer with the static variable 'timeout_ticks'
>> +     * saved from the last bcm2835_wdt_start() call.
>> +     */
>> +    return bcm2835_wdt_start_ticks(dev, timeout_ticks, 0);
>> +}
>> +
>> +static int bcm2835_wdt_stop(struct udevice *dev)
>> +{
>> +    struct bcm2835_wdt_priv *priv = dev_get_priv(dev);
>> +    void __iomem *base = priv->base;
>> +
>> +    writel(PM_PASSWORD | PM_RSTC_RESET, base + PM_RSTC);
>> +
>> +    return 0;
>> +}
>> +
>> +static int bcm2835_wdt_expire_now(struct udevice *dev, ulong flags)
>> +{
>> +    int ret;
>> +
>> +    /* use a timeout of 10 ticks (~150us) */
>> +    ret = bcm2835_wdt_start_ticks(dev, 10, flags);
>> +    if (ret)
>> +        return ret;
>> +
>> +    mdelay(500);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct wdt_ops bcm2835_wdt_ops = {
>> +    .reset        = bcm2835_wdt_reset,
>> +    .start        = bcm2835_wdt_start,
>> +    .stop        = bcm2835_wdt_stop,
>> +    .expire_now    = bcm2835_wdt_expire_now,
>> +};
>> +
>> +static const struct udevice_id bcm2835_wdt_ids[] = {
>> +    { .compatible = "brcm,bcm2835-pm" },
>> +    { .compatible = "brcm,bcm2835-pm-wdt" },
>> +    { /* sentinel */ }
>> +};
>> +
>> +static int bcm2835_wdt_probe(struct udevice *dev)
>> +{
>> +    struct bcm2835_wdt_priv *priv = dev_get_priv(dev);
>> +
>> +    priv->base = dev_remap_addr(dev);
>> +    if (!priv->base)
>> +        return -EINVAL;
>> +
>> +    bcm2835_wdt_stop(dev);
>> +
>> +    return 0;
>> +}
>> +
>> +U_BOOT_DRIVER(bcm2835_wdt) = {
>> +    .name        = "bcm2835_wdt",
>> +    .id        = UCLASS_WDT,
>> +    .of_match    = bcm2835_wdt_ids,
>> +    .probe        = bcm2835_wdt_probe,
>> +    .priv_auto    = sizeof(struct bcm2835_wdt_priv),
>> +    .ops        = &bcm2835_wdt_ops,
>> +};
>
> Viele Grüße,
> Stefan Roese
>

-- 
Etienne Dublé
CNRS / LIG - Bâtiment IMAG
700 avenue Centrale - 38401 St Martin d'Hères
Bureau 426 - Tel 0457421431



More information about the U-Boot mailing list