On Mon, Oct 16, 2017 at 07:25:27PM +0100, Radu Rendec wrote:
> Support multiple i6300esb devices simultaneously, by removing the single
> device restriction in the original design and leveraging the multiple
> device support of the watchdog subsystem.
>
> This patch replaces the global definitions of watchdog device data with
> a dynamically allocated structure. This structure is allocated during
> device probe, so multiple independent structures can be allocated if
> multiple devices are probed.
>
> Signed-off-by: Radu Rendec <rrendec@xxxxxxxxxx>
Looks good, except it will need a rebase to match changes I asked for
in patch 1.
Guenter
> ---
> drivers/watchdog/i6300esb.c | 178 +++++++++++++++++++++++---------------------
> 1 file changed, 94 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index 1df41a09553c..855b4f80ad09 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -23,6 +23,7 @@
> * Ported driver to kernel 2.6
> * 20171016 Radu Rendec <rrendec@xxxxxxxxxx>
> * Change driver to use the watchdog subsystem
> + * Add support for multiple 6300ESB devices
> */
>
> /*
> @@ -53,10 +54,10 @@
> #define ESB_LOCK_REG 0x68 /* WDT lock register */
>
> /* Memory mapped registers */
> -#define ESB_TIMER1_REG (BASEADDR + 0x00)/* Timer1 value after each reset */
> -#define ESB_TIMER2_REG (BASEADDR + 0x04)/* Timer2 value after each reset */
> -#define ESB_GINTSR_REG (BASEADDR + 0x08)/* General Interrupt Status Register */
> -#define ESB_RELOAD_REG (BASEADDR + 0x0c)/* Reload register */
> +#define ESB_TIMER1_REG(w) ((w)->base + 0x00)/* Timer1 value after each reset */
> +#define ESB_TIMER2_REG(w) ((w)->base + 0x04)/* Timer2 value after each reset */
> +#define ESB_GINTSR_REG(w) ((w)->base + 0x08)/* General Interrupt Status Reg */
> +#define ESB_RELOAD_REG(w) ((w)->base + 0x0c)/* Reload register */
>
> /* Lock register bits */
> #define ESB_WDT_FUNC (0x01 << 2) /* Watchdog functionality */
> @@ -76,12 +77,7 @@
> #define ESB_UNLOCK1 0x80 /* Step 1 to unlock reset registers */
> #define ESB_UNLOCK2 0x86 /* Step 2 to unlock reset registers */
>
> -/* internal variables */
> -static void __iomem *BASEADDR;
> -static DEFINE_SPINLOCK(esb_lock); /* Guards the hardware */
> -static struct pci_dev *esb_pci;
> -
> -/* We can only use 1 card due to the /dev/watchdog restriction */
> +/* Probed devices counter; used only for printing the initial info message */
> static int cards_found;
>
> /* module parameters */
> @@ -99,6 +95,16 @@ MODULE_PARM_DESC(nowayout,
> "Watchdog cannot be stopped once started (default="
> __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>
> +/* internal variables */
> +struct esb_dev {
> + struct watchdog_device wdd;
> + void __iomem *base;
> + spinlock_t lock;
> + struct pci_dev *pdev;
> +};
> +
> +#define to_esb_dev(wptr) container_of(wptr, struct esb_dev, wdd)
> +
> /*
> * Some i6300ESB specific functions
> */
> @@ -109,39 +115,41 @@ MODULE_PARM_DESC(nowayout,
> * reload register. After this the appropriate registers can be written
> * to once before they need to be unlocked again.
> */
> -static inline void esb_unlock_registers(void)
> +static inline void esb_unlock_registers(struct esb_dev *edev)
> {
> - writew(ESB_UNLOCK1, ESB_RELOAD_REG);
> - writew(ESB_UNLOCK2, ESB_RELOAD_REG);
> + writew(ESB_UNLOCK1, ESB_RELOAD_REG(edev));
> + writew(ESB_UNLOCK2, ESB_RELOAD_REG(edev));
> }
>
> static int esb_timer_start(struct watchdog_device *wdd)
> {
> + struct esb_dev *edev = to_esb_dev(wdd);
> int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
> u8 val;
>
> - spin_lock(&esb_lock);
> - esb_unlock_registers();
> - writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> + spin_lock(&edev->lock);
> + esb_unlock_registers(edev);
> + writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
> /* Enable or Enable + Lock? */
> val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
> - pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
> - spin_unlock(&esb_lock);
> + pci_write_config_byte(edev->pdev, ESB_LOCK_REG, val);
> + spin_unlock(&edev->lock);
> return 0;
> }
>
> static int esb_timer_stop(struct watchdog_device *wdd)
> {
> + struct esb_dev *edev = to_esb_dev(wdd);
> u8 val;
>
> - spin_lock(&esb_lock);
> + spin_lock(&edev->lock);
> /* First, reset timers as suggested by the docs */
> - esb_unlock_registers();
> - writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> + esb_unlock_registers(edev);
> + writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
> /* Then disable the WDT */
> - pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x0);
> - pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val);
> - spin_unlock(&esb_lock);
> + pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x0);
> + pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val);
> + spin_unlock(&edev->lock);
>
> /* Returns 0 if the timer was disabled, non-zero otherwise */
> return val & ESB_WDT_ENABLE;
> @@ -149,20 +157,23 @@ static int esb_timer_stop(struct watchdog_device *wdd)
>
> static int esb_timer_keepalive(struct watchdog_device *wdd)
> {
> - spin_lock(&esb_lock);
> - esb_unlock_registers();
> - writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> + struct esb_dev *edev = to_esb_dev(wdd);
> +
> + spin_lock(&edev->lock);
> + esb_unlock_registers(edev);
> + writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
> /* FIXME: Do we need to flush anything here? */
> - spin_unlock(&esb_lock);
> + spin_unlock(&edev->lock);
> return 0;
> }
>
> static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
> unsigned int time)
> {
> + struct esb_dev *edev = to_esb_dev(wdd);
> u32 val;
>
> - spin_lock(&esb_lock);
> + spin_lock(&edev->lock);
>
> /* We shift by 9, so if we are passed a value of 1 sec,
> * val will be 1 << 9 = 512, then write that to two
> @@ -171,21 +182,21 @@ static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
> val = time << 9;
>
> /* Write timer 1 */
> - esb_unlock_registers();
> - writel(val, ESB_TIMER1_REG);
> + esb_unlock_registers(edev);
> + writel(val, ESB_TIMER1_REG(edev));
>
> /* Write timer 2 */
> - esb_unlock_registers();
> - writel(val, ESB_TIMER2_REG);
> + esb_unlock_registers(edev);
> + writel(val, ESB_TIMER2_REG(edev));
>
> /* Reload */
> - esb_unlock_registers();
> - writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> + esb_unlock_registers(edev);
> + writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>
> /* FIXME: Do we need to flush everything out? */
>
> /* Done */
> - spin_unlock(&esb_lock);
> + spin_unlock(&edev->lock);
> return 0;
> }
>
> @@ -206,14 +217,6 @@ static const struct watchdog_ops esb_ops = {
> .ping = esb_timer_keepalive,
> };
>
> -static struct watchdog_device esb_dev = {
> - .info = &esb_info,
> - .ops = &esb_ops,
> - .min_timeout = 1,
> - .max_timeout = 2 * 0x03ff,
> - .timeout = WATCHDOG_HEARTBEAT,
> -};
> -
> /*
> * Data for PCI driver interface
> */
> @@ -227,38 +230,38 @@ MODULE_DEVICE_TABLE(pci, esb_pci_tbl);
> * Init & exit routines
> */
>
> -static unsigned char esb_getdevice(struct pci_dev *pdev)
> +static unsigned char esb_getdevice(struct esb_dev *edev)
> {
> - if (pci_enable_device(pdev)) {
> + if (pci_enable_device(edev->pdev)) {
> pr_err("failed to enable device\n");
> goto err_devput;
> }
>
> - if (pci_request_region(pdev, 0, ESB_MODULE_NAME)) {
> + if (pci_request_region(edev->pdev, 0, ESB_MODULE_NAME)) {
> pr_err("failed to request region\n");
> goto err_disable;
> }
>
> - BASEADDR = pci_ioremap_bar(pdev, 0);
> - if (BASEADDR == NULL) {
> + edev->base = pci_ioremap_bar(edev->pdev, 0);
> + if (edev->base == NULL) {
> /* Something's wrong here, BASEADDR has to be set */
> pr_err("failed to get BASEADDR\n");
> goto err_release;
> }
>
> /* Done */
> - esb_pci = pdev;
> + dev_set_drvdata(&edev->pdev->dev, edev);
> return 1;
>
> err_release:
> - pci_release_region(pdev, 0);
> + pci_release_region(edev->pdev, 0);
> err_disable:
> - pci_disable_device(pdev);
> + pci_disable_device(edev->pdev);
> err_devput:
> return 0;
> }
>
> -static void esb_initdevice(void)
> +static void esb_initdevice(struct esb_dev *edev)
> {
> u8 val1;
> u16 val2;
> @@ -275,33 +278,34 @@ static void esb_initdevice(void)
> * any interrupts as there is not much we can do with it
> * right now.
> */
> - pci_write_config_word(esb_pci, ESB_CONFIG_REG, 0x0003);
> + pci_write_config_word(edev->pdev, ESB_CONFIG_REG, 0x0003);
>
> /* Check that the WDT isn't already locked */
> - pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val1);
> + pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val1);
> if (val1 & ESB_WDT_LOCK)
> pr_warn("nowayout already set\n");
>
> /* Set the timer to watchdog mode and disable it for now */
> - pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x00);
> + pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x00);
>
> /* Check if the watchdog was previously triggered */
> - esb_unlock_registers();
> - val2 = readw(ESB_RELOAD_REG);
> + esb_unlock_registers(edev);
> + val2 = readw(ESB_RELOAD_REG(edev));
> if (val2 & ESB_WDT_TIMEOUT)
> - esb_dev.bootstatus = WDIOF_CARDRESET;
> + edev->wdd.bootstatus = WDIOF_CARDRESET;
>
> /* Reset WDT_TIMEOUT flag and timers */
> - esb_unlock_registers();
> - writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
> + esb_unlock_registers(edev);
> + writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG(edev));
>
> /* And set the correct timeout value */
> - esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
> + esb_timer_set_heartbeat(&edev->wdd, edev->wdd.timeout);
> }
>
> static int esb_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> {
> + struct esb_dev *edev;
> int ret;
>
> cards_found++;
> @@ -309,13 +313,14 @@ static int esb_probe(struct pci_dev *pdev,
> pr_info("Intel 6300ESB WatchDog Timer Driver v%s\n",
> ESB_VERSION);
>
> - if (cards_found > 1) {
> - pr_err("This driver only supports 1 device\n");
> - return -ENODEV;
> - }
> + edev = devm_kzalloc(&pdev->dev, sizeof(*edev), GFP_KERNEL);
> + if (!edev)
> + return -ENOMEM;
>
> /* Check whether or not the hardware watchdog is there */
> - if (!esb_getdevice(pdev) || esb_pci == NULL)
> + spin_lock_init(&edev->lock);
> + edev->pdev = pdev;
> + if (!esb_getdevice(edev))
> return -ENODEV;
>
> /* Check that the heartbeat value is within it's range;
> @@ -327,47 +332,52 @@ static int esb_probe(struct pci_dev *pdev,
> }
>
> /* Initialize the watchdog and make sure it does not run */
> - watchdog_init_timeout(&esb_dev, heartbeat, NULL);
> - watchdog_set_nowayout(&esb_dev, nowayout);
> - esb_initdevice();
> + edev->wdd.info = &esb_info;
> + edev->wdd.ops = &esb_ops;
> + edev->wdd.min_timeout = 1;
> + edev->wdd.max_timeout = 2 * 0x03ff;
> + watchdog_init_timeout(&edev->wdd, heartbeat, NULL);
> + watchdog_set_nowayout(&edev->wdd, nowayout);
> + esb_initdevice(edev);
>
> /* Register the watchdog so that userspace has access to it */
> - ret = watchdog_register_device(&esb_dev);
> + ret = watchdog_register_device(&edev->wdd);
> if (ret != 0) {
> pr_err("cannot register watchdog device (err=%d)\n", ret);
> goto err_unmap;
> }
> pr_info("initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
> - BASEADDR, esb_dev.timeout, nowayout);
> + edev->base, edev->wdd.timeout, nowayout);
> return 0;
>
> err_unmap:
> - iounmap(BASEADDR);
> - pci_release_region(esb_pci, 0);
> - pci_disable_device(esb_pci);
> - esb_pci = NULL;
> + iounmap(edev->base);
> + pci_release_region(edev->pdev, 0);
> + pci_disable_device(edev->pdev);
> return ret;
> }
>
> static void esb_remove(struct pci_dev *pdev)
> {
> - int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &esb_dev.status);
> + struct esb_dev *edev = dev_get_drvdata(&pdev->dev);
> + int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &edev->wdd.status);
>
> /* Stop the timer before we leave */
> if (!_wdd_nowayout)
> - esb_timer_stop(&esb_dev);
> + esb_timer_stop(&edev->wdd);
>
> /* Deregister */
> - watchdog_unregister_device(&esb_dev);
> - iounmap(BASEADDR);
> - pci_release_region(esb_pci, 0);
> - pci_disable_device(esb_pci);
> - esb_pci = NULL;
> + watchdog_unregister_device(&edev->wdd);
> + iounmap(edev->base);
> + pci_release_region(edev->pdev, 0);
> + pci_disable_device(edev->pdev);
> }
>
> static void esb_shutdown(struct pci_dev *pdev)
> {
> - esb_timer_stop(&esb_dev);
> + struct esb_dev *edev = dev_get_drvdata(&pdev->dev);
> +
> + esb_timer_stop(&edev->wdd);
> }
>
> static struct pci_driver esb_driver = {
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
