RE: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support

From: Yang, Wenyou
Date: Tue Jul 28 2015 - 20:38:38 EST

Next message: Rafael J. Wysocki: "[PATCH] cpufreq: Replace recover_policy with new_policy in cpufreq_online()"
Previous message: Rafael J. Wysocki: "[Update 2x][PATCH 7/7] cpufreq: Separate CPU device registration from CPU online"
In reply to: Guenter Roeck: "Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support"
Next in thread: Guenter Roeck: "Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Guenter,

Thank you very much for your review.

> -----Original Message-----
> From: Guenter Roeck [mailto:linux@xxxxxxxxxxxx]
> Sent: 2015年7月28日 15:14
> To: Yang, Wenyou; wim@xxxxxxxxx; robh+dt@xxxxxxxxxx; pawel.moll@xxxxxxx;
> mark.rutland@xxxxxxx; ijc+devicetree@xxxxxxxxxxxxxx; galak@xxxxxxxxxxxxxx
> Cc: sylvain.rochet@xxxxxxxxxxxx; Ferre, Nicolas; boris.brezillon@free-
> electrons.com; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> watchdog@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature
> support
>
> On 07/28/2015 12:00 AM, Wenyou Yang wrote:
> > In the datasheet, the new feature is describled as "WDT_MR can be
> > written until a LOCKMR command is issued in WDT_CR".
> > That is to say, as long as the bootstrap and u-boot don't issue a
> > LOCKMR command, WDT_MR can be written in kernel.
> >
> > So the driver can enable/disable the watchdog timer hardware, set
> > WDV(Watchdog Counter Value) and WDD(Watchdog Delta Value) fields of
> > WDT_MR register to set the watchdog timer timeout.
> >
> > The timer is not necessary that regularly sends a keepalive ping to
> > the watchdog timer hardware.
> >
> > It is introduced from sama5d4 SoCs.
> >
> Since there are so many changes, I wonder is a separate driver would make more
> sense.
Yes, a bit many changes.
I thought reuse the driver code.
If a separate driver, I am afraid it includes much duplicated code.
After all, it is for the same device with different feature.

I don't think it is necessary to have multiple drivers for the same peripheral with different feature.

>
> Thoughts ?
>
> Guenter
>
> > Signed-off-by: Wenyou Yang <wenyou.yang@xxxxxxxxx>
> > ---
> > drivers/watchdog/at91sam9_wdt.c | 255
> ++++++++++++++++++++++++++++-----------
> > drivers/watchdog/at91sam9_wdt.h | 4 +
> > 2 files changed, 190 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/watchdog/at91sam9_wdt.c
> > b/drivers/watchdog/at91sam9_wdt.c index 1443b3c..6b61084 100644
> > --- a/drivers/watchdog/at91sam9_wdt.c
> > +++ b/drivers/watchdog/at91sam9_wdt.c
> > @@ -10,9 +10,12 @@
> > */
> >
> > /*
> > + * For AT91SAM9x SoCs, the Watchdog Timer has the following constraint.
> > * The Watchdog Timer Mode Register can be only written to once. If the
> > * timeout need to be set from Linux, be sure that the bootstrap or the
> > * bootloader doesn't write to this register.
> > + * From SAMA5D4, the Watchdog Timer Mode Register can be written
> > + * until a LOCKMR command is issued in WDT_CR.
> > */
> >
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt @@ -80,6 +83,11 @@
> > MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started
> "
> > "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> >
> > #define to_wdt(wdd) container_of(wdd, struct at91wdt, wdd)
> > +
> > +struct at91wdt_variant {
> > + bool mr_writable;
> > +};
> > +
> > struct at91wdt {
> > struct watchdog_device wdd;
> > void __iomem *base;
> > @@ -90,6 +98,9 @@ struct at91wdt {
> > unsigned long heartbeat; /* WDT heartbeat in jiffies */
> > bool nowayout;
> > unsigned int irq;
> > + bool use_timer;
> > + bool enabled;
> > + struct at91wdt_variant *drv_data;
> > };
> >
> > /*
> > ......................................................................... */ @@ -133,21 +144,67 @@
> static void at91_ping(unsigned long data)
> > static int at91_wdt_start(struct watchdog_device *wdd)
> > {
> > struct at91wdt *wdt = to_wdt(wdd);
> > - /* calculate when the next userspace timeout will be */
> > - wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
> > + u32 reg;
> > +
> > + if (wdt->drv_data->mr_writable) {
> > + reg = wdt_read(wdt, AT91_WDT_MR);
> > + reg &= ~AT91_WDT_WDDIS;
> > + wdt_write(wdt, AT91_WDT_MR, reg);
> > + } else {
> > + /* calculate when the next userspace timeout will be */
> > + wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
> > + }
> > +
> > return 0;
> > }
> >
> > static int at91_wdt_stop(struct watchdog_device *wdd)
> > {
> > - /* The watchdog timer hardware can not be stopped... */
> > + struct at91wdt *wdt = to_wdt(wdd);
> > + u32 reg;
> > +
> > + if (wdt->drv_data->mr_writable) {
> > + reg = wdt_read(wdt, AT91_WDT_MR);
> > + reg |= AT91_WDT_WDDIS;
> > + wdt_write(wdt, AT91_WDT_MR, reg);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int at91_wdt_ping(struct watchdog_device *wdd) {
> > + struct at91wdt *wdt = to_wdt(wdd);
> > +
> > + wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY |
> AT91_WDT_WDRSTT);
> > +
> > return 0;
> > }
> >
> > static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int
> new_timeout)
> > {
> > - wdd->timeout = new_timeout;
> > - return at91_wdt_start(wdd);
> > + struct at91wdt *wdt = to_wdt(wdd);
> > + u32 reg, timeout;
> > +
> > + if (wdt->drv_data->mr_writable) {
> > + timeout = secs_to_ticks(new_timeout);
> > + if (timeout > WDT_COUNTER_MAX_TICKS)
> > + return -EINVAL;
> > +
> > + reg = wdt_read(wdt, AT91_WDT_MR);
> > + reg &= ~AT91_WDT_WDV;
> > + reg |= AT91_WDT_WDV_(timeout);
> > + reg &= ~AT91_WDT_WDD;
> > + reg |= AT91_WDT_WDD_(timeout);
> > + wdt_write(wdt, AT91_WDT_MR, reg);
> > +
> > + wdd->timeout = new_timeout;
> > +
> > + return 0;
> > + } else {
> > + wdd->timeout = new_timeout;
> > + return at91_wdt_start(wdd);
> > + }
> > }
> >
> > static int at91_wdt_init(struct platform_device *pdev, struct
> > at91wdt *wdt) @@ -161,50 +218,65 @@ static int at91_wdt_init(struct
> platform_device *pdev, struct at91wdt *wdt)
> > unsigned long max_heartbeat;
> > struct device *dev = &pdev->dev;
> >
> > - tmp = wdt_read(wdt, AT91_WDT_MR);
> > - if ((tmp & mask) != (wdt->mr & mask)) {
> > - if (tmp == WDT_MR_RESET) {
> > - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> > - tmp = wdt_read(wdt, AT91_WDT_MR);
> > + if (wdt->drv_data->mr_writable) {
> > + wdt->use_timer = false;
> > +
> > + wdt_write(wdt, AT91_WDT_MR, wdt->mr | AT91_WDT_WDDIS);
> > + } else {
> > + wdt->use_timer = true;
> > +
> > + tmp = wdt_read(wdt, AT91_WDT_MR);
> > + if ((tmp & mask) != (wdt->mr & mask)) {
> > + if (tmp == WDT_MR_RESET) {
> > + wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> > + tmp = wdt_read(wdt, AT91_WDT_MR);
> > + }
> > }
> > - }
> >
> > - if (tmp & AT91_WDT_WDDIS) {
> > - if (wdt->mr & AT91_WDT_WDDIS)
> > - return 0;
> > - dev_err(dev, "watchdog is disabled\n");
> > - return -EINVAL;
> > + if (tmp & AT91_WDT_WDDIS) {
> > + if (wdt->mr & AT91_WDT_WDDIS)
> > + return 0;
> > +
> > + dev_err(dev, "watchdog is disabled\n");
> > + return -EINVAL;
> > + }
> > }
> >
> > - value = tmp & AT91_WDT_WDV;
> > - delta = (tmp & AT91_WDT_WDD) >> 16;
> > + tmp = wdt_read(wdt, AT91_WDT_MR);
> > + wdt->enabled = (tmp & AT91_WDT_WDDIS) ? false : true;
> >
> > - if (delta < value)
> > - min_heartbeat = ticks_to_hz_roundup(value - delta);
> > + if (wdt->use_timer) {
> > + value = tmp & AT91_WDT_WDV;
> > + delta = (tmp & AT91_WDT_WDD) >> 16;
> >
> > - max_heartbeat = ticks_to_hz_rounddown(value);
> > - if (!max_heartbeat) {
> > - dev_err(dev,
> > - "heartbeat is too small for the system to handle it
> correctly\n");
> > - return -EINVAL;
> > - }
> > + if (delta < value)
> > + min_heartbeat = ticks_to_hz_roundup(value - delta);
> >
> > - /*
> > - * Try to reset the watchdog counter 4 or 2 times more often than
> > - * actually requested, to avoid spurious watchdog reset.
> > - * If this is not possible because of the min_heartbeat value, reset
> > - * it at the min_heartbeat period.
> > - */
> > - if ((max_heartbeat / 4) >= min_heartbeat)
> > - wdt->heartbeat = max_heartbeat / 4;
> > - else if ((max_heartbeat / 2) >= min_heartbeat)
> > - wdt->heartbeat = max_heartbeat / 2;
> > - else
> > - wdt->heartbeat = min_heartbeat;
> > -
> > - if (max_heartbeat < min_heartbeat + 4)
> > - dev_warn(dev,
> > - "min heartbeat and max heartbeat might be too close for
> the system to handle it correctly\n");
> > + max_heartbeat = ticks_to_hz_rounddown(value);
> > + if (!max_heartbeat) {
> > + dev_err(dev,
> > + "heartbeat is too small for the system to handle it
> correctly\n");
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * Try to reset the watchdog counter 4 or 2 times more often than
> > + * actually requested, to avoid spurious watchdog reset.
> > + * If this is not possible because of the min_heartbeat value, reset
> > + * it at the min_heartbeat period.
> > + */
> > + if ((max_heartbeat / 4) >= min_heartbeat)
> > + wdt->heartbeat = max_heartbeat / 4;
> > + else if ((max_heartbeat / 2) >= min_heartbeat)
> > + wdt->heartbeat = max_heartbeat / 2;
> > + else
> > + wdt->heartbeat = min_heartbeat;
> > +
> > + if (max_heartbeat < min_heartbeat + 4) {
> > + dev_warn(dev,
> > + "min heartbeat and max heartbeat might be too
> close for the system to handle it correctly\n");
> > + }
> > + }
> >
> > if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
> > err = request_irq(wdt->irq, wdt_interrupt, @@ -215,21 +287,24
> @@
> > static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> > return err;
> > }
> >
> > - if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask))
> > - dev_warn(dev,
> > - "watchdog already configured differently (mr = %x
> expecting %x)\n",
> > - tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> > + if (wdt->use_timer) {
> > + if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask)) {
> > + dev_warn(dev,
> > + "watchdog already configured differently (mr
> = %x expecting %x)\n",
> > + tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> > + }
> >
> > - setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> > + setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> >
> > - /*
> > - * Use min_heartbeat the first time to avoid spurious watchdog reset:
> > - * we don't know for how long the watchdog counter is running, and
> > - * - resetting it right now might trigger a watchdog fault reset
> > - * - waiting for heartbeat time might lead to a watchdog timeout
> > - * reset
> > - */
> > - mod_timer(&wdt->timer, jiffies + min_heartbeat);
> > + /*
> > + * Use min_heartbeat the first time to avoid spurious watchdog
> reset:
> > + * we don't know for how long the watchdog counter is running,
> and
> > + * - resetting it right now might trigger a watchdog fault reset
> > + * - waiting for heartbeat time might lead to a watchdog timeout
> > + * reset
> > + */
> > + mod_timer(&wdt->timer, jiffies + min_heartbeat);
> > + }
> >
> > /* Try to set timeout from device tree first */
> > if (watchdog_init_timeout(&wdt->wdd, 0, dev)) @@ -239,12 +314,14 @@
> > static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> > if (err)
> > goto out_stop_timer;
> >
> > - wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
> > + if (wdt->use_timer)
> > + wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
> >
> > return 0;
> >
> > out_stop_timer:
> > - del_timer(&wdt->timer);
> > + if (wdt->use_timer)
> > + del_timer(&wdt->timer);
> > return err;
> > }
> >
> > @@ -256,13 +333,54 @@ static const struct watchdog_info at91_wdt_info = {
> > WDIOF_MAGICCLOSE,
> > };
> >
> > -static const struct watchdog_ops at91_wdt_ops = {
> > +static struct watchdog_ops at91_wdt_ops = {
> > .owner = THIS_MODULE,
> > .start = at91_wdt_start,
> > .stop = at91_wdt_stop,
> > .set_timeout = at91_wdt_set_timeout,
> > };
> >
> > +static const struct at91wdt_variant drv_data_at91sam9260 = {
> > + .mr_writable = false,
> > +};
> > +
> > +#if defined(CONFIG_OF)
> > +static const struct at91wdt_variant drv_data_sama5d4 = {
> > + .mr_writable = true,
> > +};
> > +
> > +static const struct of_device_id at91_wdt_dt_ids[] = {
> > + { .compatible = "atmel,at91sam9260-wdt",
> > + .data = &drv_data_at91sam9260 },
> > + { .compatible = "atmel,sama5d4-wdt",
> > + .data = &drv_data_sama5d4 },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, at91_wdt_dt_ids); #endif
> > +
> > +static const struct platform_device_id at91wdt_ids[] = {
> > + {
> > + .name = "at91_wdt",
> > + .driver_data = (unsigned long)&drv_data_at91sam9260,
> > + },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(platform, at91wdt_ids);
> > +
> > +static struct at91wdt_variant *at91wdt_get_drv_data(struct
> > +platform_device *pdev) {
> > + const struct of_device_id *match;
> > +
> > + if (pdev->dev.of_node) {
> > + match = of_match_node(at91_wdt_dt_ids, pdev->dev.of_node);
> > + return (struct at91wdt_variant *)match->data;
> > + } else {
> > + return (struct at91wdt_variant *)
> > + platform_get_device_id(pdev)->driver_data;
> > + }
> > +}
> > +
> > #if defined(CONFIG_OF)
> > static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
> > {
> > @@ -336,6 +454,10 @@ static int __init at91wdt_probe(struct platform_device
> *pdev)
> > if (!wdt)
> > return -ENOMEM;
> >
> > + wdt->drv_data = at91wdt_get_drv_data(pdev);
> > + if (wdt->drv_data->mr_writable)
> > + at91_wdt_ops.ping = at91_wdt_ping;
> > +
> > wdt->mr = (WDT_HW_TIMEOUT * 256) | AT91_WDT_WDRSTEN |
> AT91_WDT_WDD |
> > AT91_WDT_WDDBGHLT | AT91_WDT_WDIDLEHLT;
> > wdt->mr_mask = 0x3FFFFFFF;
> > @@ -364,8 +486,12 @@ static int __init at91wdt_probe(struct
> > platform_device *pdev)
> >
> > platform_set_drvdata(pdev, wdt);
> >
> > - pr_info("enabled (heartbeat=%d sec, nowayout=%d)\n",
> > - wdt->wdd.timeout, wdt->nowayout);
> > + if (wdt->enabled) {
> > + dev_info(&pdev->dev, "enabled (heartbeat=%d sec,
> nowayout=%d)\n",
> > + wdt->wdd.timeout, wdt->nowayout);
> > + } else {
> > + dev_info(&pdev->dev, "not enabled\n");
> > + }
> >
> > return 0;
> > }
> > @@ -381,15 +507,6 @@ static int __exit at91wdt_remove(struct
> platform_device *pdev)
> > return 0;
> > }
> >
> > -#if defined(CONFIG_OF)
> > -static const struct of_device_id at91_wdt_dt_ids[] = {
> > - { .compatible = "atmel,at91sam9260-wdt" },
> > - { /* sentinel */ }
> > -};
> > -
> > -MODULE_DEVICE_TABLE(of, at91_wdt_dt_ids); -#endif
> > -
> > static struct platform_driver at91wdt_driver = {
> > .remove = __exit_p(at91wdt_remove),
> > .driver = {
> > diff --git a/drivers/watchdog/at91sam9_wdt.h
> > b/drivers/watchdog/at91sam9_wdt.h index c6fbb2e6..79add4f 100644
> > --- a/drivers/watchdog/at91sam9_wdt.h
> > +++ b/drivers/watchdog/at91sam9_wdt.h
> > @@ -22,11 +22,15 @@
> >
> > #define AT91_WDT_MR 0x04 /* Watchdog
> Mode Register */
> > #define AT91_WDT_WDV (0xfff << 0) /*
> Counter Value */
> > +#define AT91_WDT_WDV_MSK (0xfff)
> > +#define AT91_WDT_WDV_(x) (((x) &
> AT91_WDT_WDV_MSK) << 0)
> > #define AT91_WDT_WDFIEN (1 << 12) /*
> Fault Interrupt Enable */
> > #define AT91_WDT_WDRSTEN (1 << 13) /*
> Reset Processor */
> > #define AT91_WDT_WDRPROC (1 << 14) /*
> Timer Restart */
> > #define AT91_WDT_WDDIS (1 << 15) /*
> Watchdog Disable */
> > #define AT91_WDT_WDD (0xfff << 16) /*
> Delta Value */
> > +#define AT91_WDT_WDD_MSK (0xfff)
> > +#define AT91_WDT_WDD_(x) (((x) &
> AT91_WDT_WDD_MSK) << 16)
> > #define AT91_WDT_WDDBGHLT (1 << 28) /*
> Debug Halt */
> > #define AT91_WDT_WDIDLEHLT (1 << 29) /*
> Idle Halt */
> >
> >

Best Regards,
Wenyou Yang

N?叉??y??b??千v??藓{.n???{?赙zXФ?塄}?财??j:+v???赙zZ+?zf＂?????i????ア??璀??撷f?^j谦y??@A?囤?0鹅h??i

Next message: Rafael J. Wysocki: "[PATCH] cpufreq: Replace recover_policy with new_policy in cpufreq_online()"
Previous message: Rafael J. Wysocki: "[Update 2x][PATCH 7/7] cpufreq: Separate CPU device registration from CPU online"
In reply to: Guenter Roeck: "Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support"
Next in thread: Guenter Roeck: "Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]