mbox series

[0/7] Removal of bcm63xx-wdt

Message ID 20211028172322.4021440-1-f.fainelli@gmail.com
Headers show
Series Removal of bcm63xx-wdt | expand

Message

Florian Fainelli Oct. 28, 2021, 5:23 p.m. UTC
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

Comments

Guenter Roeck Oct. 28, 2021, 6:18 p.m. UTC | #1
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 <f.fainelli@gmail.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  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
>
Guenter Roeck Oct. 28, 2021, 6:18 p.m. UTC | #2
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 <f.fainelli@gmail.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  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
>
Guenter Roeck Oct. 28, 2021, 6:19 p.m. UTC | #3
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 <f.fainelli@gmail.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  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
>
Guenter Roeck Oct. 28, 2021, 6:19 p.m. UTC | #4
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 <f.fainelli@gmail.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

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
>
Guenter Roeck Oct. 28, 2021, 6:20 p.m. UTC | #5
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 <f.fainelli@gmail.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  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 <miguel.gaio@efixo.com>
> - *  Copyright (C) 2008, Florian Fainelli <florian@openwrt.org>
> - *
> - */
> -
> -#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 <miguel.gaio@efixo.com>");
> -MODULE_AUTHOR("Florian Fainelli <florian@openwrt.org>");
> -MODULE_DESCRIPTION("Driver for the Broadcom BCM63xx SoC watchdog");
> -MODULE_LICENSE("GPL");
> -MODULE_ALIAS("platform:bcm63xx-wdt");
> -- 
> 2.25.1
>
Rafał Miłecki Oct. 29, 2021, 12:37 p.m. UTC | #6
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 <f.fainelli@gmail.com>
> ---
>  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).
Florian Fainelli Oct. 29, 2021, 6:34 p.m. UTC | #7
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 <f.fainelli@gmail.com>
>> ---
>>  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.
Thomas Bogendoerfer Nov. 2, 2021, 10:19 a.m. UTC | #8
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 <f.fainelli@gmail.com>
> ---
>  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 <tsbogend@alpha.franken.de>