2021-10-28 17:24:41

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 0/7] Removal of bcm63xx-wdt

This patch series prepares the bcm7038_wdt driver to support its bcm63xx
counter part, updates the MIPS BCM63xx platform code to provide the
necessary information about the "periph" clock, and finally proceeds
with removing the bcm63xx_wdt altogether.

This was only compiled tested as I did not have a readily available
BCM63xx system to test with.

This should also help with adding support for BCM4908 which Rafal is
working on.

Florian Fainelli (6):
dt-bindings: watchdog: Add BCM6345 compatible to BCM7038 binding
watchdog: bcm7038_wdt: Support platform data configuration
watchdog: Allow building BCM7038_WDT for BCM63XX
watchdog: bcm7038_wdt: Add platform device id for bcm63xx-wdt
MIPS: BCM63XX: Provide platform data to watchdog device
watchdog: Remove BCM63XX_WDT

Rafał Miłecki (1):
dt-bindings: watchdog: convert Broadcom's WDT to the json-schema

.../bindings/watchdog/brcm,bcm7038-wdt.txt | 19 --
.../bindings/watchdog/brcm,bcm7038-wdt.yaml | 40 +++
arch/mips/bcm63xx/dev-wdt.c | 8 +
drivers/watchdog/Kconfig | 15 +-
drivers/watchdog/Makefile | 1 -
drivers/watchdog/bcm63xx_wdt.c | 315 ------------------
drivers/watchdog/bcm7038_wdt.c | 15 +-
include/linux/platform_data/bcm7038_wdt.h | 8 +
8 files changed, 73 insertions(+), 348 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
create mode 100644 Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml
delete mode 100644 drivers/watchdog/bcm63xx_wdt.c
create mode 100644 include/linux/platform_data/bcm7038_wdt.h

--
2.25.1


2021-10-28 17:24:42

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 1/7] dt-bindings: watchdog: convert Broadcom's WDT to the json-schema

From: Rafał Miłecki <[email protected]>

This helps validating DTS files.

Signed-off-by: Rafał Miłecki <[email protected]>
Signed-off-by: Florian Fainelli <[email protected]>
---
.../bindings/watchdog/brcm,bcm7038-wdt.txt | 19 ---------
.../bindings/watchdog/brcm,bcm7038-wdt.yaml | 39 +++++++++++++++++++
2 files changed, 39 insertions(+), 19 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
create mode 100644 Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml

diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
deleted file mode 100644
index 84122270be8f..000000000000
--- a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
+++ /dev/null
@@ -1,19 +0,0 @@
-BCM7038 Watchdog timer
-
-Required properties:
-
-- compatible : should be "brcm,bcm7038-wdt"
-- reg : Specifies base physical address and size of the registers.
-
-Optional properties:
-
-- clocks: The clock running the watchdog. If no clock is found the
- driver will default to 27000000 Hz.
-
-Example:
-
-watchdog@f040a7e8 {
- compatible = "brcm,bcm7038-wdt";
- clocks = <&upg_fixed>;
- reg = <0xf040a7e8 0x16>;
-};
diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml
new file mode 100644
index 000000000000..bbf2e91f9db7
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/brcm,bcm7038-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: BCM7038 watchdog timer
+
+allOf:
+ - $ref: "watchdog.yaml#"
+
+maintainers:
+ - Justin Chen <[email protected]>
+ - Florian Fainelli <[email protected]>
+
+properties:
+ compatible:
+ const: brcm,bcm7038-wdt
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ description: >
+ The clock running the watchdog. If no clock is found the driver will
+ default to 27000000 Hz.
+
+unevaluatedProperties: false
+
+required:
+ - reg
+
+examples:
+ - |
+ watchdog@f040a7e8 {
+ compatible = "brcm,bcm7038-wdt";
+ reg = <0xf040a7e8 0x16>;
+ clocks = <&upg_fixed>;
+ };
--
2.25.1

2021-10-28 17:25:07

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 2/7] dt-bindings: watchdog: Add BCM6345 compatible to BCM7038 binding

The BCM7038 watchdog binding is updated to include a "brcm,bcm6345-wdt"
compatible string which is the first instance of a DSL (BCM63xx) SoC
seeing the integration of such a watchdog timer block.

Signed-off-by: Florian Fainelli <[email protected]>
---
.../devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml
index bbf2e91f9db7..cab3a8ff7bab 100644
--- a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml
@@ -4,7 +4,7 @@
$id: http://devicetree.org/schemas/watchdog/brcm,bcm7038-wdt.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#

-title: BCM7038 watchdog timer
+title: BCM63xx and BCM7038 watchdog timer

allOf:
- $ref: "watchdog.yaml#"
@@ -15,6 +15,7 @@ maintainers:

properties:
compatible:
+ const: brcm,bcm6345-wdt
const: brcm,bcm7038-wdt

reg:
--
2.25.1

2021-10-28 17:25:13

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 4/7] watchdog: Allow building BCM7038_WDT for BCM63XX

CONFIG_BCM63XX denotes the legacy MIPS-based DSL SoCs which utilize the
same piece of hardware as a watchdog, make it possible to select that
driver for those platforms.

Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/watchdog/Kconfig | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index bf59faeb3de1..24a775dd2bf1 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1756,12 +1756,13 @@ config BCM7038_WDT
tristate "BCM7038 Watchdog"
select WATCHDOG_CORE
depends on HAS_IOMEM
- depends on ARCH_BRCMSTB || BMIPS_GENERIC || COMPILE_TEST
+ depends on ARCH_BRCMSTB || BMIPS_GENERIC || BCM63XX || COMPILE_TEST
help
Watchdog driver for the built-in hardware in Broadcom 7038 and
later SoCs used in set-top boxes. BCM7038 was made public
during the 2004 CES, and since then, many Broadcom chips use this
- watchdog block, including some cable modem chips.
+ watchdog block, including some cable modem chips and DSL (63xx)
+ chips.

config IMGPDC_WDT
tristate "Imagination Technologies PDC Watchdog Timer"
--
2.25.1

2021-10-28 17:25:15

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 7/7] watchdog: Remove BCM63XX_WDT

Now that we can utilize the BCM7038_WDT driver, remove that one which
was not converted to the watchdog APIs. There are a couple of notable
differences with how the bcm7038_wdt driver proceeds:

- bcm63xx_wdt would register with the ad-hoc BCM63xx hardware timer API,
but this would only be used in order to catch the interrupt *before* a
SoC reset and make the kernel "die"

- bcm6xx_wdt would register a software timer and kick it every second in
order to pet the watchdog, thus offering a two step watchdog process.
This is not something that is brought over to the bcm7038_wdt as it is
deemed unnecessary. If user-space cannot pet the watchdog, but a
kernel timer can, the system is still in a bad shape anyway.

bcm7038_wdt is simpler in its behavior and behaves as a standard
watchdog driver and is not making use of any specific platform APIs,
therefore making it more maintainable and extensible.

Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/watchdog/Kconfig | 10 --
drivers/watchdog/Makefile | 1 -
drivers/watchdog/bcm63xx_wdt.c | 315 ---------------------------------
3 files changed, 326 deletions(-)
delete mode 100644 drivers/watchdog/bcm63xx_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 24a775dd2bf1..acebf9caf6d1 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1709,16 +1709,6 @@ config OCTEON_WDT
from the first interrupt, it is then only poked when the
device is written.

-config BCM63XX_WDT
- tristate "Broadcom BCM63xx hardware watchdog"
- depends on BCM63XX
- help
- Watchdog driver for the built in watchdog hardware in Broadcom
- BCM63xx SoC.
-
- To compile this driver as a loadable module, choose M here.
- The module will be called bcm63xx_wdt.
-
config BCM2835_WDT
tristate "Broadcom BCM2835 hardware watchdog"
depends on ARCH_BCM2835 || (OF && COMPILE_TEST)
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 1bd2d6f37c53..9811c4b1cd16 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -154,7 +154,6 @@ obj-$(CONFIG_XILINX_WATCHDOG) += of_xilinx_wdt.o
# MIPS Architecture
obj-$(CONFIG_ATH79_WDT) += ath79_wdt.o
obj-$(CONFIG_BCM47XX_WDT) += bcm47xx_wdt.o
-obj-$(CONFIG_BCM63XX_WDT) += bcm63xx_wdt.o
obj-$(CONFIG_RC32434_WDT) += rc32434_wdt.o
obj-$(CONFIG_INDYDOG) += indydog.o
obj-$(CONFIG_JZ4740_WDT) += jz4740_wdt.o
diff --git a/drivers/watchdog/bcm63xx_wdt.c b/drivers/watchdog/bcm63xx_wdt.c
deleted file mode 100644
index 7cdb25363ea0..000000000000
--- a/drivers/watchdog/bcm63xx_wdt.c
+++ /dev/null
@@ -1,315 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * Broadcom BCM63xx SoC watchdog driver
- *
- * Copyright (C) 2007, Miguel Gaio <[email protected]>
- * Copyright (C) 2008, Florian Fainelli <[email protected]>
- *
- */
-
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-#include <linux/bitops.h>
-#include <linux/errno.h>
-#include <linux/fs.h>
-#include <linux/io.h>
-#include <linux/kernel.h>
-#include <linux/miscdevice.h>
-#include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/types.h>
-#include <linux/uaccess.h>
-#include <linux/watchdog.h>
-#include <linux/timer.h>
-#include <linux/jiffies.h>
-#include <linux/interrupt.h>
-#include <linux/ptrace.h>
-#include <linux/resource.h>
-#include <linux/platform_device.h>
-
-#include <bcm63xx_cpu.h>
-#include <bcm63xx_io.h>
-#include <bcm63xx_regs.h>
-#include <bcm63xx_timer.h>
-
-#define PFX KBUILD_MODNAME
-
-#define WDT_HZ 50000000 /* Fclk */
-#define WDT_DEFAULT_TIME 30 /* seconds */
-#define WDT_MAX_TIME 256 /* seconds */
-
-static struct {
- void __iomem *regs;
- struct timer_list timer;
- unsigned long inuse;
- atomic_t ticks;
-} bcm63xx_wdt_device;
-
-static int expect_close;
-
-static int wdt_time = WDT_DEFAULT_TIME;
-static bool nowayout = WATCHDOG_NOWAYOUT;
-module_param(nowayout, bool, 0);
-MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
- __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
-
-/* HW functions */
-static void bcm63xx_wdt_hw_start(void)
-{
- bcm_writel(0xfffffffe, bcm63xx_wdt_device.regs + WDT_DEFVAL_REG);
- bcm_writel(WDT_START_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
- bcm_writel(WDT_START_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
-}
-
-static void bcm63xx_wdt_hw_stop(void)
-{
- bcm_writel(WDT_STOP_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
- bcm_writel(WDT_STOP_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
-}
-
-static void bcm63xx_wdt_isr(void *data)
-{
- struct pt_regs *regs = get_irq_regs();
-
- die(PFX " fire", regs);
-}
-
-static void bcm63xx_timer_tick(struct timer_list *unused)
-{
- if (!atomic_dec_and_test(&bcm63xx_wdt_device.ticks)) {
- bcm63xx_wdt_hw_start();
- mod_timer(&bcm63xx_wdt_device.timer, jiffies + HZ);
- } else
- pr_crit("watchdog will restart system\n");
-}
-
-static void bcm63xx_wdt_pet(void)
-{
- atomic_set(&bcm63xx_wdt_device.ticks, wdt_time);
-}
-
-static void bcm63xx_wdt_start(void)
-{
- bcm63xx_wdt_pet();
- bcm63xx_timer_tick(0);
-}
-
-static void bcm63xx_wdt_pause(void)
-{
- del_timer_sync(&bcm63xx_wdt_device.timer);
- bcm63xx_wdt_hw_stop();
-}
-
-static int bcm63xx_wdt_settimeout(int new_time)
-{
- if ((new_time <= 0) || (new_time > WDT_MAX_TIME))
- return -EINVAL;
-
- wdt_time = new_time;
-
- return 0;
-}
-
-static int bcm63xx_wdt_open(struct inode *inode, struct file *file)
-{
- if (test_and_set_bit(0, &bcm63xx_wdt_device.inuse))
- return -EBUSY;
-
- bcm63xx_wdt_start();
- return stream_open(inode, file);
-}
-
-static int bcm63xx_wdt_release(struct inode *inode, struct file *file)
-{
- if (expect_close == 42)
- bcm63xx_wdt_pause();
- else {
- pr_crit("Unexpected close, not stopping watchdog!\n");
- bcm63xx_wdt_start();
- }
- clear_bit(0, &bcm63xx_wdt_device.inuse);
- expect_close = 0;
- return 0;
-}
-
-static ssize_t bcm63xx_wdt_write(struct file *file, const char *data,
- size_t len, loff_t *ppos)
-{
- if (len) {
- if (!nowayout) {
- size_t i;
-
- /* In case it was set long ago */
- expect_close = 0;
-
- for (i = 0; i != len; i++) {
- char c;
- if (get_user(c, data + i))
- return -EFAULT;
- if (c == 'V')
- expect_close = 42;
- }
- }
- bcm63xx_wdt_pet();
- }
- return len;
-}
-
-static struct watchdog_info bcm63xx_wdt_info = {
- .identity = PFX,
- .options = WDIOF_SETTIMEOUT |
- WDIOF_KEEPALIVEPING |
- WDIOF_MAGICCLOSE,
-};
-
-
-static long bcm63xx_wdt_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
-{
- void __user *argp = (void __user *)arg;
- int __user *p = argp;
- int new_value, retval = -EINVAL;
-
- switch (cmd) {
- case WDIOC_GETSUPPORT:
- return copy_to_user(argp, &bcm63xx_wdt_info,
- sizeof(bcm63xx_wdt_info)) ? -EFAULT : 0;
-
- case WDIOC_GETSTATUS:
- case WDIOC_GETBOOTSTATUS:
- return put_user(0, p);
-
- case WDIOC_SETOPTIONS:
- if (get_user(new_value, p))
- return -EFAULT;
-
- if (new_value & WDIOS_DISABLECARD) {
- bcm63xx_wdt_pause();
- retval = 0;
- }
- if (new_value & WDIOS_ENABLECARD) {
- bcm63xx_wdt_start();
- retval = 0;
- }
-
- return retval;
-
- case WDIOC_KEEPALIVE:
- bcm63xx_wdt_pet();
- return 0;
-
- case WDIOC_SETTIMEOUT:
- if (get_user(new_value, p))
- return -EFAULT;
-
- if (bcm63xx_wdt_settimeout(new_value))
- return -EINVAL;
-
- bcm63xx_wdt_pet();
-
- case WDIOC_GETTIMEOUT:
- return put_user(wdt_time, p);
-
- default:
- return -ENOTTY;
-
- }
-}
-
-static const struct file_operations bcm63xx_wdt_fops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .write = bcm63xx_wdt_write,
- .unlocked_ioctl = bcm63xx_wdt_ioctl,
- .compat_ioctl = compat_ptr_ioctl,
- .open = bcm63xx_wdt_open,
- .release = bcm63xx_wdt_release,
-};
-
-static struct miscdevice bcm63xx_wdt_miscdev = {
- .minor = WATCHDOG_MINOR,
- .name = "watchdog",
- .fops = &bcm63xx_wdt_fops,
-};
-
-
-static int bcm63xx_wdt_probe(struct platform_device *pdev)
-{
- int ret;
- struct resource *r;
-
- timer_setup(&bcm63xx_wdt_device.timer, bcm63xx_timer_tick, 0);
-
- r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!r) {
- dev_err(&pdev->dev, "failed to get resources\n");
- return -ENODEV;
- }
-
- bcm63xx_wdt_device.regs = devm_ioremap(&pdev->dev, r->start,
- resource_size(r));
- if (!bcm63xx_wdt_device.regs) {
- dev_err(&pdev->dev, "failed to remap I/O resources\n");
- return -ENXIO;
- }
-
- ret = bcm63xx_timer_register(TIMER_WDT_ID, bcm63xx_wdt_isr, NULL);
- if (ret < 0) {
- dev_err(&pdev->dev, "failed to register wdt timer isr\n");
- return ret;
- }
-
- if (bcm63xx_wdt_settimeout(wdt_time)) {
- bcm63xx_wdt_settimeout(WDT_DEFAULT_TIME);
- dev_info(&pdev->dev,
- ": wdt_time value must be 1 <= wdt_time <= 256, using %d\n",
- wdt_time);
- }
-
- ret = misc_register(&bcm63xx_wdt_miscdev);
- if (ret < 0) {
- dev_err(&pdev->dev, "failed to register watchdog device\n");
- goto unregister_timer;
- }
-
- dev_info(&pdev->dev, " started, timer margin: %d sec\n",
- WDT_DEFAULT_TIME);
-
- return 0;
-
-unregister_timer:
- bcm63xx_timer_unregister(TIMER_WDT_ID);
- return ret;
-}
-
-static int bcm63xx_wdt_remove(struct platform_device *pdev)
-{
- if (!nowayout)
- bcm63xx_wdt_pause();
-
- misc_deregister(&bcm63xx_wdt_miscdev);
- bcm63xx_timer_unregister(TIMER_WDT_ID);
- return 0;
-}
-
-static void bcm63xx_wdt_shutdown(struct platform_device *pdev)
-{
- bcm63xx_wdt_pause();
-}
-
-static struct platform_driver bcm63xx_wdt_driver = {
- .probe = bcm63xx_wdt_probe,
- .remove = bcm63xx_wdt_remove,
- .shutdown = bcm63xx_wdt_shutdown,
- .driver = {
- .name = "bcm63xx-wdt",
- }
-};
-
-module_platform_driver(bcm63xx_wdt_driver);
-
-MODULE_AUTHOR("Miguel Gaio <[email protected]>");
-MODULE_AUTHOR("Florian Fainelli <[email protected]>");
-MODULE_DESCRIPTION("Driver for the Broadcom BCM63xx SoC watchdog");
-MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:bcm63xx-wdt");
--
2.25.1

2021-10-28 17:25:21

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 3/7] watchdog: bcm7038_wdt: Support platform data configuration

The BCM7038 watchdog driver needs to be able to obtain a specific clock
name on BCM63xx platforms which is the "periph" clock ticking at 50MHz.
make it possible to specify the clock name to obtain via platform data.

Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/watchdog/bcm7038_wdt.c | 8 +++++++-
include/linux/platform_data/bcm7038_wdt.h | 8 ++++++++
2 files changed, 15 insertions(+), 1 deletion(-)
create mode 100644 include/linux/platform_data/bcm7038_wdt.h

diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
index acaaa0005d5b..506cd7ef9c77 100644
--- a/drivers/watchdog/bcm7038_wdt.c
+++ b/drivers/watchdog/bcm7038_wdt.c
@@ -10,6 +10,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/platform_data/bcm7038_wdt.h>
#include <linux/pm.h>
#include <linux/watchdog.h>

@@ -133,8 +134,10 @@ static void bcm7038_clk_disable_unprepare(void *data)

static int bcm7038_wdt_probe(struct platform_device *pdev)
{
+ struct bcm7038_wdt_platform_data *pdata = pdev->dev.platform_data;
struct device *dev = &pdev->dev;
struct bcm7038_watchdog *wdt;
+ const char *clk_name = NULL;
int err;

wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
@@ -147,7 +150,10 @@ static int bcm7038_wdt_probe(struct platform_device *pdev)
if (IS_ERR(wdt->base))
return PTR_ERR(wdt->base);

- wdt->clk = devm_clk_get(dev, NULL);
+ if (pdata && pdata->clk_name)
+ clk_name = pdata->clk_name;
+
+ wdt->clk = devm_clk_get(dev, clk_name);
/* If unable to get clock, use default frequency */
if (!IS_ERR(wdt->clk)) {
err = clk_prepare_enable(wdt->clk);
diff --git a/include/linux/platform_data/bcm7038_wdt.h b/include/linux/platform_data/bcm7038_wdt.h
new file mode 100644
index 000000000000..e18cfd9ec8f9
--- /dev/null
+++ b/include/linux/platform_data/bcm7038_wdt.h
@@ -0,0 +1,8 @@
+#ifndef __BCM7038_WDT_PDATA_H
+#define __BCM7038_WDT_PDATA_H
+
+struct bcm7038_wdt_platform_data {
+ const char *clk_name;
+};
+
+#endif /* __BCM7038_WDT_PDATA_H */
--
2.25.1

2021-10-28 17:25:22

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 6/7] MIPS: BCM63XX: Provide platform data to watchdog device

In order to utilize the bcm7038_wdt.c driver which needs to know the
clock name to obtain, pass it via platform data using the
bcm7038_wdt_platform_data structure.

Signed-off-by: Florian Fainelli <[email protected]>
---
arch/mips/bcm63xx/dev-wdt.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/arch/mips/bcm63xx/dev-wdt.c b/arch/mips/bcm63xx/dev-wdt.c
index 2a2346a99bcb..42130914a3c2 100644
--- a/arch/mips/bcm63xx/dev-wdt.c
+++ b/arch/mips/bcm63xx/dev-wdt.c
@@ -9,6 +9,7 @@
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/platform_device.h>
+#include <linux/platform_data/bcm7038_wdt.h>
#include <bcm63xx_cpu.h>

static struct resource wdt_resources[] = {
@@ -19,11 +20,18 @@ static struct resource wdt_resources[] = {
},
};

+static struct bcm7038_wdt_platform_data bcm63xx_wdt_pdata = {
+ .clk_name = "periph",
+};
+
static struct platform_device bcm63xx_wdt_device = {
.name = "bcm63xx-wdt",
.id = -1,
.num_resources = ARRAY_SIZE(wdt_resources),
.resource = wdt_resources,
+ .dev = {
+ .platform_data = &bcm63xx_wdt_pdata,
+ },
};

int __init bcm63xx_wdt_register(void)
--
2.25.1

2021-10-28 17:25:38

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 5/7] watchdog: bcm7038_wdt: Add platform device id for bcm63xx-wdt

In order to phase out bcm63xx_wdt and use bcm7038_wdt instead, introduce
a platform_device_id table that allows both names to be matched.

Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/watchdog/bcm7038_wdt.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
index 506cd7ef9c77..2535f450e8a1 100644
--- a/drivers/watchdog/bcm7038_wdt.c
+++ b/drivers/watchdog/bcm7038_wdt.c
@@ -223,6 +223,13 @@ static const struct of_device_id bcm7038_wdt_match[] = {
};
MODULE_DEVICE_TABLE(of, bcm7038_wdt_match);

+static const struct platform_device_id bcm7038_wdt_devtype[] = {
+ { .name = "bcm7038-wdt" },
+ { .name = "bcm63xx-wdt" },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(platform, bcm7038_wdt_devtype);
+
static struct platform_driver bcm7038_wdt_driver = {
.probe = bcm7038_wdt_probe,
.driver = {
--
2.25.1

2021-10-28 18:21:10

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 3/7] watchdog: bcm7038_wdt: Support platform data configuration

On Thu, Oct 28, 2021 at 10:23:18AM -0700, Florian Fainelli wrote:
> The BCM7038 watchdog driver needs to be able to obtain a specific clock
> name on BCM63xx platforms which is the "periph" clock ticking at 50MHz.
> make it possible to specify the clock name to obtain via platform data.
>
> Signed-off-by: Florian Fainelli <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
> drivers/watchdog/bcm7038_wdt.c | 8 +++++++-
> include/linux/platform_data/bcm7038_wdt.h | 8 ++++++++
> 2 files changed, 15 insertions(+), 1 deletion(-)
> create mode 100644 include/linux/platform_data/bcm7038_wdt.h
>
> diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
> index acaaa0005d5b..506cd7ef9c77 100644
> --- a/drivers/watchdog/bcm7038_wdt.c
> +++ b/drivers/watchdog/bcm7038_wdt.c
> @@ -10,6 +10,7 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> +#include <linux/platform_data/bcm7038_wdt.h>
> #include <linux/pm.h>
> #include <linux/watchdog.h>
>
> @@ -133,8 +134,10 @@ static void bcm7038_clk_disable_unprepare(void *data)
>
> static int bcm7038_wdt_probe(struct platform_device *pdev)
> {
> + struct bcm7038_wdt_platform_data *pdata = pdev->dev.platform_data;
> struct device *dev = &pdev->dev;
> struct bcm7038_watchdog *wdt;
> + const char *clk_name = NULL;
> int err;
>
> wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> @@ -147,7 +150,10 @@ static int bcm7038_wdt_probe(struct platform_device *pdev)
> if (IS_ERR(wdt->base))
> return PTR_ERR(wdt->base);
>
> - wdt->clk = devm_clk_get(dev, NULL);
> + if (pdata && pdata->clk_name)
> + clk_name = pdata->clk_name;
> +
> + wdt->clk = devm_clk_get(dev, clk_name);
> /* If unable to get clock, use default frequency */
> if (!IS_ERR(wdt->clk)) {
> err = clk_prepare_enable(wdt->clk);
> diff --git a/include/linux/platform_data/bcm7038_wdt.h b/include/linux/platform_data/bcm7038_wdt.h
> new file mode 100644
> index 000000000000..e18cfd9ec8f9
> --- /dev/null
> +++ b/include/linux/platform_data/bcm7038_wdt.h
> @@ -0,0 +1,8 @@
> +#ifndef __BCM7038_WDT_PDATA_H
> +#define __BCM7038_WDT_PDATA_H
> +
> +struct bcm7038_wdt_platform_data {
> + const char *clk_name;
> +};
> +
> +#endif /* __BCM7038_WDT_PDATA_H */
> --
> 2.25.1
>

2021-10-28 18:21:35

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 4/7] watchdog: Allow building BCM7038_WDT for BCM63XX

On Thu, Oct 28, 2021 at 10:23:19AM -0700, Florian Fainelli wrote:
> CONFIG_BCM63XX denotes the legacy MIPS-based DSL SoCs which utilize the
> same piece of hardware as a watchdog, make it possible to select that
> driver for those platforms.
>
> Signed-off-by: Florian Fainelli <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
> drivers/watchdog/Kconfig | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index bf59faeb3de1..24a775dd2bf1 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1756,12 +1756,13 @@ config BCM7038_WDT
> tristate "BCM7038 Watchdog"
> select WATCHDOG_CORE
> depends on HAS_IOMEM
> - depends on ARCH_BRCMSTB || BMIPS_GENERIC || COMPILE_TEST
> + depends on ARCH_BRCMSTB || BMIPS_GENERIC || BCM63XX || COMPILE_TEST
> help
> Watchdog driver for the built-in hardware in Broadcom 7038 and
> later SoCs used in set-top boxes. BCM7038 was made public
> during the 2004 CES, and since then, many Broadcom chips use this
> - watchdog block, including some cable modem chips.
> + watchdog block, including some cable modem chips and DSL (63xx)
> + chips.
>
> config IMGPDC_WDT
> tristate "Imagination Technologies PDC Watchdog Timer"
> --
> 2.25.1
>

2021-10-28 18:22:33

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 6/7] MIPS: BCM63XX: Provide platform data to watchdog device

On Thu, Oct 28, 2021 at 10:23:21AM -0700, Florian Fainelli wrote:
> In order to utilize the bcm7038_wdt.c driver which needs to know the
> clock name to obtain, pass it via platform data using the
> bcm7038_wdt_platform_data structure.
>
> Signed-off-by: Florian Fainelli <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

We'd need an Ack from a mips maintainer to apply this patch through
the watchdog tree.

Guenter

> ---
> arch/mips/bcm63xx/dev-wdt.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/mips/bcm63xx/dev-wdt.c b/arch/mips/bcm63xx/dev-wdt.c
> index 2a2346a99bcb..42130914a3c2 100644
> --- a/arch/mips/bcm63xx/dev-wdt.c
> +++ b/arch/mips/bcm63xx/dev-wdt.c
> @@ -9,6 +9,7 @@
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/platform_device.h>
> +#include <linux/platform_data/bcm7038_wdt.h>
> #include <bcm63xx_cpu.h>
>
> static struct resource wdt_resources[] = {
> @@ -19,11 +20,18 @@ static struct resource wdt_resources[] = {
> },
> };
>
> +static struct bcm7038_wdt_platform_data bcm63xx_wdt_pdata = {
> + .clk_name = "periph",
> +};
> +
> static struct platform_device bcm63xx_wdt_device = {
> .name = "bcm63xx-wdt",
> .id = -1,
> .num_resources = ARRAY_SIZE(wdt_resources),
> .resource = wdt_resources,
> + .dev = {
> + .platform_data = &bcm63xx_wdt_pdata,
> + },
> };
>
> int __init bcm63xx_wdt_register(void)
> --
> 2.25.1
>

2021-10-28 18:24:20

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 5/7] watchdog: bcm7038_wdt: Add platform device id for bcm63xx-wdt

On Thu, Oct 28, 2021 at 10:23:20AM -0700, Florian Fainelli wrote:
> In order to phase out bcm63xx_wdt and use bcm7038_wdt instead, introduce
> a platform_device_id table that allows both names to be matched.
>
> Signed-off-by: Florian Fainelli <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
> drivers/watchdog/bcm7038_wdt.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
> index 506cd7ef9c77..2535f450e8a1 100644
> --- a/drivers/watchdog/bcm7038_wdt.c
> +++ b/drivers/watchdog/bcm7038_wdt.c
> @@ -223,6 +223,13 @@ static const struct of_device_id bcm7038_wdt_match[] = {
> };
> MODULE_DEVICE_TABLE(of, bcm7038_wdt_match);
>
> +static const struct platform_device_id bcm7038_wdt_devtype[] = {
> + { .name = "bcm7038-wdt" },
> + { .name = "bcm63xx-wdt" },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(platform, bcm7038_wdt_devtype);
> +
> static struct platform_driver bcm7038_wdt_driver = {
> .probe = bcm7038_wdt_probe,
> .driver = {
> --
> 2.25.1
>

2021-10-28 18:25:03

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 7/7] watchdog: Remove BCM63XX_WDT

On Thu, Oct 28, 2021 at 10:23:22AM -0700, Florian Fainelli wrote:
> Now that we can utilize the BCM7038_WDT driver, remove that one which
> was not converted to the watchdog APIs. There are a couple of notable
> differences with how the bcm7038_wdt driver proceeds:
>
> - bcm63xx_wdt would register with the ad-hoc BCM63xx hardware timer API,
> but this would only be used in order to catch the interrupt *before* a
> SoC reset and make the kernel "die"
>
> - bcm6xx_wdt would register a software timer and kick it every second in
> order to pet the watchdog, thus offering a two step watchdog process.
> This is not something that is brought over to the bcm7038_wdt as it is
> deemed unnecessary. If user-space cannot pet the watchdog, but a
> kernel timer can, the system is still in a bad shape anyway.
>
> bcm7038_wdt is simpler in its behavior and behaves as a standard
> watchdog driver and is not making use of any specific platform APIs,
> therefore making it more maintainable and extensible.
>
> Signed-off-by: Florian Fainelli <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
> drivers/watchdog/Kconfig | 10 --
> drivers/watchdog/Makefile | 1 -
> drivers/watchdog/bcm63xx_wdt.c | 315 ---------------------------------
> 3 files changed, 326 deletions(-)
> delete mode 100644 drivers/watchdog/bcm63xx_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 24a775dd2bf1..acebf9caf6d1 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1709,16 +1709,6 @@ config OCTEON_WDT
> from the first interrupt, it is then only poked when the
> device is written.
>
> -config BCM63XX_WDT
> - tristate "Broadcom BCM63xx hardware watchdog"
> - depends on BCM63XX
> - help
> - Watchdog driver for the built in watchdog hardware in Broadcom
> - BCM63xx SoC.
> -
> - To compile this driver as a loadable module, choose M here.
> - The module will be called bcm63xx_wdt.
> -
> config BCM2835_WDT
> tristate "Broadcom BCM2835 hardware watchdog"
> depends on ARCH_BCM2835 || (OF && COMPILE_TEST)
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 1bd2d6f37c53..9811c4b1cd16 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -154,7 +154,6 @@ obj-$(CONFIG_XILINX_WATCHDOG) += of_xilinx_wdt.o
> # MIPS Architecture
> obj-$(CONFIG_ATH79_WDT) += ath79_wdt.o
> obj-$(CONFIG_BCM47XX_WDT) += bcm47xx_wdt.o
> -obj-$(CONFIG_BCM63XX_WDT) += bcm63xx_wdt.o
> obj-$(CONFIG_RC32434_WDT) += rc32434_wdt.o
> obj-$(CONFIG_INDYDOG) += indydog.o
> obj-$(CONFIG_JZ4740_WDT) += jz4740_wdt.o
> diff --git a/drivers/watchdog/bcm63xx_wdt.c b/drivers/watchdog/bcm63xx_wdt.c
> deleted file mode 100644
> index 7cdb25363ea0..000000000000
> --- a/drivers/watchdog/bcm63xx_wdt.c
> +++ /dev/null
> @@ -1,315 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0+
> -/*
> - * Broadcom BCM63xx SoC watchdog driver
> - *
> - * Copyright (C) 2007, Miguel Gaio <[email protected]>
> - * Copyright (C) 2008, Florian Fainelli <[email protected]>
> - *
> - */
> -
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
> -#include <linux/bitops.h>
> -#include <linux/errno.h>
> -#include <linux/fs.h>
> -#include <linux/io.h>
> -#include <linux/kernel.h>
> -#include <linux/miscdevice.h>
> -#include <linux/module.h>
> -#include <linux/moduleparam.h>
> -#include <linux/types.h>
> -#include <linux/uaccess.h>
> -#include <linux/watchdog.h>
> -#include <linux/timer.h>
> -#include <linux/jiffies.h>
> -#include <linux/interrupt.h>
> -#include <linux/ptrace.h>
> -#include <linux/resource.h>
> -#include <linux/platform_device.h>
> -
> -#include <bcm63xx_cpu.h>
> -#include <bcm63xx_io.h>
> -#include <bcm63xx_regs.h>
> -#include <bcm63xx_timer.h>
> -
> -#define PFX KBUILD_MODNAME
> -
> -#define WDT_HZ 50000000 /* Fclk */
> -#define WDT_DEFAULT_TIME 30 /* seconds */
> -#define WDT_MAX_TIME 256 /* seconds */
> -
> -static struct {
> - void __iomem *regs;
> - struct timer_list timer;
> - unsigned long inuse;
> - atomic_t ticks;
> -} bcm63xx_wdt_device;
> -
> -static int expect_close;
> -
> -static int wdt_time = WDT_DEFAULT_TIME;
> -static bool nowayout = WATCHDOG_NOWAYOUT;
> -module_param(nowayout, bool, 0);
> -MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> - __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> -
> -/* HW functions */
> -static void bcm63xx_wdt_hw_start(void)
> -{
> - bcm_writel(0xfffffffe, bcm63xx_wdt_device.regs + WDT_DEFVAL_REG);
> - bcm_writel(WDT_START_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
> - bcm_writel(WDT_START_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
> -}
> -
> -static void bcm63xx_wdt_hw_stop(void)
> -{
> - bcm_writel(WDT_STOP_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
> - bcm_writel(WDT_STOP_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
> -}
> -
> -static void bcm63xx_wdt_isr(void *data)
> -{
> - struct pt_regs *regs = get_irq_regs();
> -
> - die(PFX " fire", regs);
> -}
> -
> -static void bcm63xx_timer_tick(struct timer_list *unused)
> -{
> - if (!atomic_dec_and_test(&bcm63xx_wdt_device.ticks)) {
> - bcm63xx_wdt_hw_start();
> - mod_timer(&bcm63xx_wdt_device.timer, jiffies + HZ);
> - } else
> - pr_crit("watchdog will restart system\n");
> -}
> -
> -static void bcm63xx_wdt_pet(void)
> -{
> - atomic_set(&bcm63xx_wdt_device.ticks, wdt_time);
> -}
> -
> -static void bcm63xx_wdt_start(void)
> -{
> - bcm63xx_wdt_pet();
> - bcm63xx_timer_tick(0);
> -}
> -
> -static void bcm63xx_wdt_pause(void)
> -{
> - del_timer_sync(&bcm63xx_wdt_device.timer);
> - bcm63xx_wdt_hw_stop();
> -}
> -
> -static int bcm63xx_wdt_settimeout(int new_time)
> -{
> - if ((new_time <= 0) || (new_time > WDT_MAX_TIME))
> - return -EINVAL;
> -
> - wdt_time = new_time;
> -
> - return 0;
> -}
> -
> -static int bcm63xx_wdt_open(struct inode *inode, struct file *file)
> -{
> - if (test_and_set_bit(0, &bcm63xx_wdt_device.inuse))
> - return -EBUSY;
> -
> - bcm63xx_wdt_start();
> - return stream_open(inode, file);
> -}
> -
> -static int bcm63xx_wdt_release(struct inode *inode, struct file *file)
> -{
> - if (expect_close == 42)
> - bcm63xx_wdt_pause();
> - else {
> - pr_crit("Unexpected close, not stopping watchdog!\n");
> - bcm63xx_wdt_start();
> - }
> - clear_bit(0, &bcm63xx_wdt_device.inuse);
> - expect_close = 0;
> - return 0;
> -}
> -
> -static ssize_t bcm63xx_wdt_write(struct file *file, const char *data,
> - size_t len, loff_t *ppos)
> -{
> - if (len) {
> - if (!nowayout) {
> - size_t i;
> -
> - /* In case it was set long ago */
> - expect_close = 0;
> -
> - for (i = 0; i != len; i++) {
> - char c;
> - if (get_user(c, data + i))
> - return -EFAULT;
> - if (c == 'V')
> - expect_close = 42;
> - }
> - }
> - bcm63xx_wdt_pet();
> - }
> - return len;
> -}
> -
> -static struct watchdog_info bcm63xx_wdt_info = {
> - .identity = PFX,
> - .options = WDIOF_SETTIMEOUT |
> - WDIOF_KEEPALIVEPING |
> - WDIOF_MAGICCLOSE,
> -};
> -
> -
> -static long bcm63xx_wdt_ioctl(struct file *file, unsigned int cmd,
> - unsigned long arg)
> -{
> - void __user *argp = (void __user *)arg;
> - int __user *p = argp;
> - int new_value, retval = -EINVAL;
> -
> - switch (cmd) {
> - case WDIOC_GETSUPPORT:
> - return copy_to_user(argp, &bcm63xx_wdt_info,
> - sizeof(bcm63xx_wdt_info)) ? -EFAULT : 0;
> -
> - case WDIOC_GETSTATUS:
> - case WDIOC_GETBOOTSTATUS:
> - return put_user(0, p);
> -
> - case WDIOC_SETOPTIONS:
> - if (get_user(new_value, p))
> - return -EFAULT;
> -
> - if (new_value & WDIOS_DISABLECARD) {
> - bcm63xx_wdt_pause();
> - retval = 0;
> - }
> - if (new_value & WDIOS_ENABLECARD) {
> - bcm63xx_wdt_start();
> - retval = 0;
> - }
> -
> - return retval;
> -
> - case WDIOC_KEEPALIVE:
> - bcm63xx_wdt_pet();
> - return 0;
> -
> - case WDIOC_SETTIMEOUT:
> - if (get_user(new_value, p))
> - return -EFAULT;
> -
> - if (bcm63xx_wdt_settimeout(new_value))
> - return -EINVAL;
> -
> - bcm63xx_wdt_pet();
> -
> - case WDIOC_GETTIMEOUT:
> - return put_user(wdt_time, p);
> -
> - default:
> - return -ENOTTY;
> -
> - }
> -}
> -
> -static const struct file_operations bcm63xx_wdt_fops = {
> - .owner = THIS_MODULE,
> - .llseek = no_llseek,
> - .write = bcm63xx_wdt_write,
> - .unlocked_ioctl = bcm63xx_wdt_ioctl,
> - .compat_ioctl = compat_ptr_ioctl,
> - .open = bcm63xx_wdt_open,
> - .release = bcm63xx_wdt_release,
> -};
> -
> -static struct miscdevice bcm63xx_wdt_miscdev = {
> - .minor = WATCHDOG_MINOR,
> - .name = "watchdog",
> - .fops = &bcm63xx_wdt_fops,
> -};
> -
> -
> -static int bcm63xx_wdt_probe(struct platform_device *pdev)
> -{
> - int ret;
> - struct resource *r;
> -
> - timer_setup(&bcm63xx_wdt_device.timer, bcm63xx_timer_tick, 0);
> -
> - r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!r) {
> - dev_err(&pdev->dev, "failed to get resources\n");
> - return -ENODEV;
> - }
> -
> - bcm63xx_wdt_device.regs = devm_ioremap(&pdev->dev, r->start,
> - resource_size(r));
> - if (!bcm63xx_wdt_device.regs) {
> - dev_err(&pdev->dev, "failed to remap I/O resources\n");
> - return -ENXIO;
> - }
> -
> - ret = bcm63xx_timer_register(TIMER_WDT_ID, bcm63xx_wdt_isr, NULL);
> - if (ret < 0) {
> - dev_err(&pdev->dev, "failed to register wdt timer isr\n");
> - return ret;
> - }
> -
> - if (bcm63xx_wdt_settimeout(wdt_time)) {
> - bcm63xx_wdt_settimeout(WDT_DEFAULT_TIME);
> - dev_info(&pdev->dev,
> - ": wdt_time value must be 1 <= wdt_time <= 256, using %d\n",
> - wdt_time);
> - }
> -
> - ret = misc_register(&bcm63xx_wdt_miscdev);
> - if (ret < 0) {
> - dev_err(&pdev->dev, "failed to register watchdog device\n");
> - goto unregister_timer;
> - }
> -
> - dev_info(&pdev->dev, " started, timer margin: %d sec\n",
> - WDT_DEFAULT_TIME);
> -
> - return 0;
> -
> -unregister_timer:
> - bcm63xx_timer_unregister(TIMER_WDT_ID);
> - return ret;
> -}
> -
> -static int bcm63xx_wdt_remove(struct platform_device *pdev)
> -{
> - if (!nowayout)
> - bcm63xx_wdt_pause();
> -
> - misc_deregister(&bcm63xx_wdt_miscdev);
> - bcm63xx_timer_unregister(TIMER_WDT_ID);
> - return 0;
> -}
> -
> -static void bcm63xx_wdt_shutdown(struct platform_device *pdev)
> -{
> - bcm63xx_wdt_pause();
> -}
> -
> -static struct platform_driver bcm63xx_wdt_driver = {
> - .probe = bcm63xx_wdt_probe,
> - .remove = bcm63xx_wdt_remove,
> - .shutdown = bcm63xx_wdt_shutdown,
> - .driver = {
> - .name = "bcm63xx-wdt",
> - }
> -};
> -
> -module_platform_driver(bcm63xx_wdt_driver);
> -
> -MODULE_AUTHOR("Miguel Gaio <[email protected]>");
> -MODULE_AUTHOR("Florian Fainelli <[email protected]>");
> -MODULE_DESCRIPTION("Driver for the Broadcom BCM63xx SoC watchdog");
> -MODULE_LICENSE("GPL");
> -MODULE_ALIAS("platform:bcm63xx-wdt");
> --
> 2.25.1
>

2021-10-29 02:18:39

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH 2/7] dt-bindings: watchdog: Add BCM6345 compatible to BCM7038 binding

On Thu, 28 Oct 2021 10:23:17 -0700, Florian Fainelli wrote:
> The BCM7038 watchdog binding is updated to include a "brcm,bcm6345-wdt"
> compatible string which is the first instance of a DSL (BCM63xx) SoC
> seeing the integration of such a watchdog timer block.
>
> Signed-off-by: Florian Fainelli <[email protected]>
> ---
> .../devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml:19:5: [error] duplication of key "const" in mapping (key-duplicates)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.example.dts'
Traceback (most recent call last):
File "/usr/local/bin/dt-extract-example", line 45, in <module>
binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 434, in load
return constructor.get_single_data()
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 122, in get_single_data
return self.construct_document(node)
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 132, in construct_document
for _dummy in generator:
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 722, in construct_yaml_map
value = self.construct_mapping(node)
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 446, in construct_mapping
return BaseConstructor.construct_mapping(self, node, deep=deep)
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 264, in construct_mapping
if self.check_mapping_key(node, key_node, mapping, key, value):
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 295, in check_mapping_key
raise DuplicateKeyError(*args)
ruamel.yaml.constructor.DuplicateKeyError: while constructing a mapping
in "<unicode string>", line 18, column 5
found duplicate key "const" with value "brcm,bcm7038-wdt" (original value: "brcm,bcm6345-wdt")
in "<unicode string>", line 19, column 5

To suppress this check see:
http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys

make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "/usr/local/bin/dt-doc-validate", line 25, in check_doc
testtree = dtschema.load(filename, line_number=line_number)
File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 612, in load
return yaml.load(f.read())
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 434, in load
return constructor.get_single_data()
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 122, in get_single_data
return self.construct_document(node)
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 132, in construct_document
for _dummy in generator:
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 722, in construct_yaml_map
value = self.construct_mapping(node)
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 446, in construct_mapping
return BaseConstructor.construct_mapping(self, node, deep=deep)
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 264, in construct_mapping
if self.check_mapping_key(node, key_node, mapping, key, value):
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 295, in check_mapping_key
raise DuplicateKeyError(*args)
ruamel.yaml.constructor.DuplicateKeyError: while constructing a mapping
in "<unicode string>", line 18, column 5
found duplicate key "const" with value "brcm,bcm7038-wdt" (original value: "brcm,bcm6345-wdt")
in "<unicode string>", line 19, column 5

To suppress this check see:
http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys


During handling of the above exception, another exception occurred:

Traceback (most recent call last):
File "/usr/local/bin/dt-doc-validate", line 67, in <module>
ret = check_doc(f)
File "/usr/local/bin/dt-doc-validate", line 30, in check_doc
print(filename + ":", exc.path[-1], exc.message, file=sys.stderr)
AttributeError: 'DuplicateKeyError' object has no attribute 'path'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml
make: *** [Makefile:1441: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1547626

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

2021-10-29 13:58:14

by Rafał Miłecki

[permalink] [raw]
Subject: Re: [PATCH 5/7] watchdog: bcm7038_wdt: Add platform device id for bcm63xx-wdt

On 2021-10-28 19:23, Florian Fainelli wrote:
> In order to phase out bcm63xx_wdt and use bcm7038_wdt instead,
> introduce
> a platform_device_id table that allows both names to be matched.
>
> Signed-off-by: Florian Fainelli <[email protected]>
> ---
> drivers/watchdog/bcm7038_wdt.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/watchdog/bcm7038_wdt.c
> b/drivers/watchdog/bcm7038_wdt.c
> index 506cd7ef9c77..2535f450e8a1 100644
> --- a/drivers/watchdog/bcm7038_wdt.c
> +++ b/drivers/watchdog/bcm7038_wdt.c
> @@ -223,6 +223,13 @@ static const struct of_device_id
> bcm7038_wdt_match[] = {
> };
> MODULE_DEVICE_TABLE(of, bcm7038_wdt_match);
>
> +static const struct platform_device_id bcm7038_wdt_devtype[] = {
> + { .name = "bcm7038-wdt" },
> + { .name = "bcm63xx-wdt" },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(platform, bcm7038_wdt_devtype);

Do we really want "bcm7038-wdt" here? I don't think it will ever be used
as apparently BCM7038 uses DT.

I'd also prefer to have Rob's comment on mapping blocks vs. mapping
registers.

If we were to map whole hardware blocks, we should have per-SoC
bindings and handling registers layouts in a driver. Right now
bcm63xx arch code maps selected part of hardware block that is
meant to match driver's logic (offsets 0x00 and 0x04).

2021-10-29 18:36:55

by Florian Fainelli

[permalink] [raw]
Subject: Re: [PATCH 5/7] watchdog: bcm7038_wdt: Add platform device id for bcm63xx-wdt

On 10/29/21 5:37 AM, Rafał Miłecki wrote:
> On 2021-10-28 19:23, Florian Fainelli wrote:
>> In order to phase out bcm63xx_wdt and use bcm7038_wdt instead, introduce
>> a platform_device_id table that allows both names to be matched.
>>
>> Signed-off-by: Florian Fainelli <[email protected]>
>> ---
>>  drivers/watchdog/bcm7038_wdt.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/watchdog/bcm7038_wdt.c
>> b/drivers/watchdog/bcm7038_wdt.c
>> index 506cd7ef9c77..2535f450e8a1 100644
>> --- a/drivers/watchdog/bcm7038_wdt.c
>> +++ b/drivers/watchdog/bcm7038_wdt.c
>> @@ -223,6 +223,13 @@ static const struct of_device_id
>> bcm7038_wdt_match[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, bcm7038_wdt_match);
>>
>> +static const struct platform_device_id bcm7038_wdt_devtype[] = {
>> +    { .name = "bcm7038-wdt" },
>> +    { .name = "bcm63xx-wdt" },
>> +    { /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(platform, bcm7038_wdt_devtype);
>
> Do we really want "bcm7038-wdt" here? I don't think it will ever be used
> as apparently BCM7038 uses DT.

Let me dig through the platform_device_id code, but I believe we somehow do.
--
Florian

2021-11-02 10:26:26

by Thomas Bogendoerfer

[permalink] [raw]
Subject: Re: [PATCH 6/7] MIPS: BCM63XX: Provide platform data to watchdog device

On Thu, Oct 28, 2021 at 10:23:21AM -0700, Florian Fainelli wrote:
> In order to utilize the bcm7038_wdt.c driver which needs to know the
> clock name to obtain, pass it via platform data using the
> bcm7038_wdt_platform_data structure.
>
> Signed-off-by: Florian Fainelli <[email protected]>
> ---
> arch/mips/bcm63xx/dev-wdt.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/mips/bcm63xx/dev-wdt.c b/arch/mips/bcm63xx/dev-wdt.c
> index 2a2346a99bcb..42130914a3c2 100644
> --- a/arch/mips/bcm63xx/dev-wdt.c
> +++ b/arch/mips/bcm63xx/dev-wdt.c
> @@ -9,6 +9,7 @@
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/platform_device.h>
> +#include <linux/platform_data/bcm7038_wdt.h>
> #include <bcm63xx_cpu.h>
>
> static struct resource wdt_resources[] = {
> @@ -19,11 +20,18 @@ static struct resource wdt_resources[] = {
> },
> };
>
> +static struct bcm7038_wdt_platform_data bcm63xx_wdt_pdata = {
> + .clk_name = "periph",
> +};
> +
> static struct platform_device bcm63xx_wdt_device = {
> .name = "bcm63xx-wdt",
> .id = -1,
> .num_resources = ARRAY_SIZE(wdt_resources),
> .resource = wdt_resources,
> + .dev = {
> + .platform_data = &bcm63xx_wdt_pdata,
> + },
> };
>
> int __init bcm63xx_wdt_register(void)
> --
> 2.25.1

Acked-by: Thomas Bogendoerfer <[email protected]>

--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]