| Message ID | 1571648890-15140-2-git-send-email-eugen.hristev@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/2] dt-bindings: watchdog: sama5d4_wdt: add microchip,sam9x60-wdt compatible | expand |
On Mon, Oct 21, 2019 at 09:14:09AM +0000, Eugen.Hristev@microchip.com wrote: > From: Eugen Hristev <eugen.hristev@microchip.com> > > Add support for SAM9X60 WDT into sama5d4_wdt. > This means that this driver will have a platform data that will > hold differences. > Added definitions of different bits. > The ops functions will differentiate between the two hardware blocks. > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> > --- > > Hello, > > This is a rework of the separate sam9x60 watchdog driver into a single driver > that supports both hardware blocks (sam9x60 and sama5d4) > This was done as suggested on the original patches on the mailing list. > > Thanks, > Eugen > > drivers/watchdog/at91sam9_wdt.h | 14 +++++ > drivers/watchdog/sama5d4_wdt.c | 127 +++++++++++++++++++++++++++++++--------- > 2 files changed, 112 insertions(+), 29 deletions(-) > > diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h > index 390941c..7a053fd 100644 > --- a/drivers/watchdog/at91sam9_wdt.h > +++ b/drivers/watchdog/at91sam9_wdt.h > @@ -20,7 +20,10 @@ > #define AT91_WDT_MR 0x04 /* Watchdog Mode Register */ > #define AT91_WDT_WDV (0xfff << 0) /* Counter Value */ > #define AT91_WDT_SET_WDV(x) ((x) & AT91_WDT_WDV) > +#define AT91_SAM9X60_PERIODRST BIT(4) /* Period Reset */ > +#define AT91_SAM9X60_RPTHRST BIT(5) /* Minimum Restart Period */ > #define AT91_WDT_WDFIEN (1 << 12) /* Fault Interrupt Enable */ > +#define AT91_SAM9X60_WDDIS BIT(12) /* Disable */ > #define AT91_WDT_WDRSTEN (1 << 13) /* Reset Processor */ > #define AT91_WDT_WDRPROC (1 << 14) /* Timer Restart */ > #define AT91_WDT_WDDIS (1 << 15) /* Watchdog Disable */ > @@ -33,4 +36,15 @@ > #define AT91_WDT_WDUNF (1 << 0) /* Watchdog Underflow */ > #define AT91_WDT_WDERR (1 << 1) /* Watchdog Error */ > > +#define AT91_SAM9X60_VR 0x08 /* Watchdog Timer Value Register */ > + > +#define AT91_SAM9X60_WLR 0x0c > +#define AT91_SAM9X60_COUNTER (0xfff << 0) /* Watchdog Period Value */ > +#define AT91_SAM9X60_SET_COUNTER(x) ((x) & AT91_SAM9X60_COUNTER) > + > +#define AT91_SAM9X60_IER 0x14 /* Interrupt Enable Register */ > +#define AT91_SAM9X60_PERINT BIT(0) /* Period Interrupt Enable */ > +#define AT91_SAM9X60_IDR 0x18 /* Interrupt Disable Register */ > +#define AT91_SAM9X60_ISR 0x1c /* Interrupt Status Register */ > + Those tabs are getting messy, and the mix of BIT() and shift is messy too. Mind cleaning it up a bit ? Especially, two tabs after #define doesn't really add value (use two spaces), and use BIT() throughout or not at all. > #endif > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c > index d193a60..b92afd7 100644 > --- a/drivers/watchdog/sama5d4_wdt.c > +++ b/drivers/watchdog/sama5d4_wdt.c > @@ -2,7 +2,7 @@ > /* > * Driver for Atmel SAMA5D4 Watchdog Timer > * > - * Copyright (C) 2015 Atmel Corporation > + * Copyright (C) 2015-2019 Microchip Technology Inc. and its subsidiaries > */ > > #include <linux/delay.h> > @@ -11,6 +11,7 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/of_irq.h> > #include <linux/platform_device.h> > #include <linux/reboot.h> > @@ -25,11 +26,18 @@ > > #define WDT_SEC2TICKS(s) ((s) ? (((s) << 8) - 1) : 0) > > +struct sama5d4_wdt_data { > + const unsigned int sam9x60_support; > +}; > + > struct sama5d4_wdt { > - struct watchdog_device wdd; > - void __iomem *reg_base; > - u32 mr; > - unsigned long last_ping; > + struct watchdog_device wdd; > + const struct sama5d4_wdt_data *data; > + void __iomem *reg_base; > + u32 mr; > + u32 ir; > + unsigned long last_ping; > + unsigned int need_irq:1; This can be a bool. Making it a bit just adds complexity to the code. > }; > > static int wdt_timeout; > @@ -78,7 +86,12 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd) > { > struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd); > > - wdt->mr &= ~AT91_WDT_WDDIS; > + if (wdt->data->sam9x60_support) { > + writel_relaxed(wdt->ir, wdt->reg_base + AT91_SAM9X60_IER); > + wdt->mr &= ~AT91_SAM9X60_WDDIS; > + } else { > + wdt->mr &= ~AT91_WDT_WDDIS; > + } > wdt_write(wdt, AT91_WDT_MR, wdt->mr); > > return 0; > @@ -88,7 +101,12 @@ static int sama5d4_wdt_stop(struct watchdog_device *wdd) > { > struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd); > > - wdt->mr |= AT91_WDT_WDDIS; > + if (wdt->data->sam9x60_support) { > + writel_relaxed(wdt->ir, wdt->reg_base + AT91_SAM9X60_IDR); > + wdt->mr |= AT91_SAM9X60_WDDIS; > + } else { > + wdt->mr |= AT91_WDT_WDDIS; > + } > wdt_write(wdt, AT91_WDT_MR, wdt->mr); > > return 0; > @@ -109,6 +127,14 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd, > struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd); > u32 value = WDT_SEC2TICKS(timeout); > > + if (wdt->data->sam9x60_support) { > + wdt_write(wdt, AT91_SAM9X60_WLR, > + AT91_SAM9X60_SET_COUNTER(value)); > + > + wdd->timeout = timeout; > + return 0; > + } > + > wdt->mr &= ~AT91_WDT_WDV; > wdt->mr |= AT91_WDT_SET_WDV(value); > > @@ -143,8 +169,14 @@ static const struct watchdog_ops sama5d4_wdt_ops = { > static irqreturn_t sama5d4_wdt_irq_handler(int irq, void *dev_id) > { > struct sama5d4_wdt *wdt = platform_get_drvdata(dev_id); > + u32 reg; > > - if (wdt_read(wdt, AT91_WDT_SR)) { > + if (wdt->data->sam9x60_support) > + reg = wdt_read(wdt, AT91_SAM9X60_ISR); > + else > + reg = wdt_read(wdt, AT91_WDT_SR); > + > + if (reg) { > pr_crit("Atmel Watchdog Software Reset\n"); > emergency_restart(); > pr_crit("Reboot didn't succeed\n"); > @@ -157,13 +189,14 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt) > { > const char *tmp; > > - wdt->mr = AT91_WDT_WDDIS; > + if (wdt->data->sam9x60_support) > + wdt->mr = AT91_SAM9X60_WDDIS; > + else > + wdt->mr = AT91_WDT_WDDIS; > > if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) && > !strcmp(tmp, "software")) > - wdt->mr |= AT91_WDT_WDFIEN; > - else > - wdt->mr |= AT91_WDT_WDRSTEN; > + wdt->need_irq = 1; > > if (of_property_read_bool(np, "atmel,idle-halt")) > wdt->mr |= AT91_WDT_WDIDLEHLT; > @@ -176,21 +209,46 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt) > > static int sama5d4_wdt_init(struct sama5d4_wdt *wdt) > { > - u32 reg; > + u32 reg, val; > + > + val = WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT); > /* > * When booting and resuming, the bootloader may have changed the > * watchdog configuration. > * If the watchdog is already running, we can safely update it. > * Else, we have to disable it properly. > */ > - if (wdt_enabled) { > - wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr); > - } else { > + if (!wdt_enabled) { > reg = wdt_read(wdt, AT91_WDT_MR); > - if (!(reg & AT91_WDT_WDDIS)) > + if (wdt->data->sam9x60_support && (!(reg & AT91_SAM9X60_WDDIS))) > + wdt_write_nosleep(wdt, AT91_WDT_MR, > + reg | AT91_SAM9X60_WDDIS); > + else if (!wdt->data->sam9x60_support && > + (!(reg & AT91_WDT_WDDIS))) > wdt_write_nosleep(wdt, AT91_WDT_MR, > reg | AT91_WDT_WDDIS); > } > + > + if (wdt->data->sam9x60_support) { > + if (wdt->need_irq) > + wdt->ir = AT91_SAM9X60_PERINT; > + else > + wdt->mr |= AT91_SAM9X60_PERIODRST; > + > + wdt_write(wdt, AT91_SAM9X60_IER, wdt->ir); > + wdt_write(wdt, AT91_SAM9X60_WLR, AT91_SAM9X60_SET_COUNTER(val)); > + } else { > + wdt->mr |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(MAX_WDT_TIMEOUT)); > + wdt->mr |= AT91_WDT_SET_WDV(val); > + > + if (wdt->need_irq) > + wdt->mr |= AT91_WDT_WDFIEN; > + else > + wdt->mr |= AT91_WDT_WDRSTEN; > + } > + > + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr); > + > return 0; > } > > @@ -201,13 +259,14 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) > struct sama5d4_wdt *wdt; > void __iomem *regs; > u32 irq = 0; > - u32 timeout; > int ret; > > wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); > if (!wdt) > return -ENOMEM; > > + wdt->data = of_device_get_match_data(&pdev->dev); > + > wdd = &wdt->wdd; > wdd->timeout = WDT_DEFAULT_TIMEOUT; > wdd->info = &sama5d4_wdt_info; > @@ -224,15 +283,17 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) > > wdt->reg_base = regs; > > - irq = irq_of_parse_and_map(dev->of_node, 0); > - if (!irq) > - dev_warn(dev, "failed to get IRQ from DT\n"); > - > ret = of_sama5d4_wdt_init(dev->of_node, wdt); > if (ret) > return ret; > > - if ((wdt->mr & AT91_WDT_WDFIEN) && irq) { > + irq = irq_of_parse_and_map(dev->of_node, 0); > + if (!irq) { > + dev_warn(dev, "failed to get IRQ from DT\n"); > + wdt->need_irq = 0; Does it make sense to ignore that ? > + } > + > + if (wdt->need_irq) { > ret = devm_request_irq(dev, irq, sama5d4_wdt_irq_handler, > IRQF_SHARED | IRQF_IRQPOLL | > IRQF_NO_SUSPEND, pdev->name, pdev); > @@ -244,11 +305,6 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) > > watchdog_init_timeout(wdd, wdt_timeout, dev); > > - timeout = WDT_SEC2TICKS(wdd->timeout); > - > - wdt->mr |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(MAX_WDT_TIMEOUT)); > - wdt->mr |= AT91_WDT_SET_WDV(timeout); > - > ret = sama5d4_wdt_init(wdt); > if (ret) > return ret; > @@ -268,8 +324,21 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) > return 0; > } > > +static struct sama5d4_wdt_data sama5d4_config; > + > +static struct sama5d4_wdt_data sam9x60_config = { > + .sam9x60_support = 1, > +}; Unless there is reason to believe that there will be other configuration data, please just assign the flag value directly to .data and add a variable to struct sama5d4_wdt to access it. Please make that variable a bool. > + > static const struct of_device_id sama5d4_wdt_of_match[] = { > - { .compatible = "atmel,sama5d4-wdt", }, > + { > + .compatible = "atmel,sama5d4-wdt", > + .data = &sama5d4_config, > + }, > + { > + .compatible = "microchip,sam9x60-wdt", > + .data = &sam9x60_config, > + }, > { } > }; > MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
On 29.10.2019 15:28, Guenter Roeck wrote: > > On Mon, Oct 21, 2019 at 09:14:09AM +0000, Eugen.Hristev@microchip.com wrote: >> From: Eugen Hristev <eugen.hristev@microchip.com> >> >> Add support for SAM9X60 WDT into sama5d4_wdt. >> This means that this driver will have a platform data that will >> hold differences. >> Added definitions of different bits. >> The ops functions will differentiate between the two hardware blocks. >> >> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> >> --- >> >> Hello, >> >> This is a rework of the separate sam9x60 watchdog driver into a single driver >> that supports both hardware blocks (sam9x60 and sama5d4) >> This was done as suggested on the original patches on the mailing list. >> >> Thanks, >> Eugen >> >> drivers/watchdog/at91sam9_wdt.h | 14 +++++ >> drivers/watchdog/sama5d4_wdt.c | 127 +++++++++++++++++++++++++++++++--------- >> 2 files changed, 112 insertions(+), 29 deletions(-) >> >> diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h >> index 390941c..7a053fd 100644 >> --- a/drivers/watchdog/at91sam9_wdt.h >> +++ b/drivers/watchdog/at91sam9_wdt.h >> @@ -20,7 +20,10 @@ >> #define AT91_WDT_MR 0x04 /* Watchdog Mode Register */ >> #define AT91_WDT_WDV (0xfff << 0) /* Counter Value */ >> #define AT91_WDT_SET_WDV(x) ((x) & AT91_WDT_WDV) >> +#define AT91_SAM9X60_PERIODRST BIT(4) /* Period Reset */ >> +#define AT91_SAM9X60_RPTHRST BIT(5) /* Minimum Restart Period */ >> #define AT91_WDT_WDFIEN (1 << 12) /* Fault Interrupt Enable */ >> +#define AT91_SAM9X60_WDDIS BIT(12) /* Disable */ >> #define AT91_WDT_WDRSTEN (1 << 13) /* Reset Processor */ >> #define AT91_WDT_WDRPROC (1 << 14) /* Timer Restart */ >> #define AT91_WDT_WDDIS (1 << 15) /* Watchdog Disable */ >> @@ -33,4 +36,15 @@ >> #define AT91_WDT_WDUNF (1 << 0) /* Watchdog Underflow */ >> #define AT91_WDT_WDERR (1 << 1) /* Watchdog Error */ >> >> +#define AT91_SAM9X60_VR 0x08 /* Watchdog Timer Value Register */ >> + >> +#define AT91_SAM9X60_WLR 0x0c >> +#define AT91_SAM9X60_COUNTER (0xfff << 0) /* Watchdog Period Value */ >> +#define AT91_SAM9X60_SET_COUNTER(x) ((x) & AT91_SAM9X60_COUNTER) >> + >> +#define AT91_SAM9X60_IER 0x14 /* Interrupt Enable Register */ >> +#define AT91_SAM9X60_PERINT BIT(0) /* Period Interrupt Enable */ >> +#define AT91_SAM9X60_IDR 0x18 /* Interrupt Disable Register */ >> +#define AT91_SAM9X60_ISR 0x1c /* Interrupt Status Register */ >> + > > Those tabs are getting messy, and the mix of BIT() and shift is messy too. > Mind cleaning it up a bit ? Especially, two tabs after #define doesn't really > add value (use two spaces), and use BIT() throughout or not at all. > >> #endif >> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c >> index d193a60..b92afd7 100644 >> --- a/drivers/watchdog/sama5d4_wdt.c >> +++ b/drivers/watchdog/sama5d4_wdt.c >> @@ -2,7 +2,7 @@ >> /* >> * Driver for Atmel SAMA5D4 Watchdog Timer >> * >> - * Copyright (C) 2015 Atmel Corporation >> + * Copyright (C) 2015-2019 Microchip Technology Inc. and its subsidiaries >> */ >> >> #include <linux/delay.h> >> @@ -11,6 +11,7 @@ >> #include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/of.h> >> +#include <linux/of_device.h> >> #include <linux/of_irq.h> >> #include <linux/platform_device.h> >> #include <linux/reboot.h> >> @@ -25,11 +26,18 @@ >> >> #define WDT_SEC2TICKS(s) ((s) ? (((s) << 8) - 1) : 0) >> >> +struct sama5d4_wdt_data { >> + const unsigned int sam9x60_support; >> +}; >> + >> struct sama5d4_wdt { >> - struct watchdog_device wdd; >> - void __iomem *reg_base; >> - u32 mr; >> - unsigned long last_ping; >> + struct watchdog_device wdd; >> + const struct sama5d4_wdt_data *data; >> + void __iomem *reg_base; >> + u32 mr; >> + u32 ir; >> + unsigned long last_ping; >> + unsigned int need_irq:1; > > This can be a bool. Making it a bit just adds complexity to the code. > >> }; >> >> static int wdt_timeout; >> @@ -78,7 +86,12 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd) >> { >> struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd); >> >> - wdt->mr &= ~AT91_WDT_WDDIS; >> + if (wdt->data->sam9x60_support) { >> + writel_relaxed(wdt->ir, wdt->reg_base + AT91_SAM9X60_IER); >> + wdt->mr &= ~AT91_SAM9X60_WDDIS; >> + } else { >> + wdt->mr &= ~AT91_WDT_WDDIS; >> + } >> wdt_write(wdt, AT91_WDT_MR, wdt->mr); >> >> return 0; >> @@ -88,7 +101,12 @@ static int sama5d4_wdt_stop(struct watchdog_device *wdd) >> { >> struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd); >> >> - wdt->mr |= AT91_WDT_WDDIS; >> + if (wdt->data->sam9x60_support) { >> + writel_relaxed(wdt->ir, wdt->reg_base + AT91_SAM9X60_IDR); >> + wdt->mr |= AT91_SAM9X60_WDDIS; >> + } else { >> + wdt->mr |= AT91_WDT_WDDIS; >> + } >> wdt_write(wdt, AT91_WDT_MR, wdt->mr); >> >> return 0; >> @@ -109,6 +127,14 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd, >> struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd); >> u32 value = WDT_SEC2TICKS(timeout); >> >> + if (wdt->data->sam9x60_support) { >> + wdt_write(wdt, AT91_SAM9X60_WLR, >> + AT91_SAM9X60_SET_COUNTER(value)); >> + >> + wdd->timeout = timeout; >> + return 0; >> + } >> + >> wdt->mr &= ~AT91_WDT_WDV; >> wdt->mr |= AT91_WDT_SET_WDV(value); >> >> @@ -143,8 +169,14 @@ static const struct watchdog_ops sama5d4_wdt_ops = { >> static irqreturn_t sama5d4_wdt_irq_handler(int irq, void *dev_id) >> { >> struct sama5d4_wdt *wdt = platform_get_drvdata(dev_id); >> + u32 reg; >> >> - if (wdt_read(wdt, AT91_WDT_SR)) { >> + if (wdt->data->sam9x60_support) >> + reg = wdt_read(wdt, AT91_SAM9X60_ISR); >> + else >> + reg = wdt_read(wdt, AT91_WDT_SR); >> + >> + if (reg) { >> pr_crit("Atmel Watchdog Software Reset\n"); >> emergency_restart(); >> pr_crit("Reboot didn't succeed\n"); >> @@ -157,13 +189,14 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt) >> { >> const char *tmp; >> >> - wdt->mr = AT91_WDT_WDDIS; >> + if (wdt->data->sam9x60_support) >> + wdt->mr = AT91_SAM9X60_WDDIS; >> + else >> + wdt->mr = AT91_WDT_WDDIS; >> >> if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) && >> !strcmp(tmp, "software")) >> - wdt->mr |= AT91_WDT_WDFIEN; >> - else >> - wdt->mr |= AT91_WDT_WDRSTEN; >> + wdt->need_irq = 1; >> >> if (of_property_read_bool(np, "atmel,idle-halt")) >> wdt->mr |= AT91_WDT_WDIDLEHLT; >> @@ -176,21 +209,46 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt) >> >> static int sama5d4_wdt_init(struct sama5d4_wdt *wdt) >> { >> - u32 reg; >> + u32 reg, val; >> + >> + val = WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT); >> /* >> * When booting and resuming, the bootloader may have changed the >> * watchdog configuration. >> * If the watchdog is already running, we can safely update it. >> * Else, we have to disable it properly. >> */ >> - if (wdt_enabled) { >> - wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr); >> - } else { >> + if (!wdt_enabled) { >> reg = wdt_read(wdt, AT91_WDT_MR); >> - if (!(reg & AT91_WDT_WDDIS)) >> + if (wdt->data->sam9x60_support && (!(reg & AT91_SAM9X60_WDDIS))) >> + wdt_write_nosleep(wdt, AT91_WDT_MR, >> + reg | AT91_SAM9X60_WDDIS); >> + else if (!wdt->data->sam9x60_support && >> + (!(reg & AT91_WDT_WDDIS))) >> wdt_write_nosleep(wdt, AT91_WDT_MR, >> reg | AT91_WDT_WDDIS); >> } >> + >> + if (wdt->data->sam9x60_support) { >> + if (wdt->need_irq) >> + wdt->ir = AT91_SAM9X60_PERINT; >> + else >> + wdt->mr |= AT91_SAM9X60_PERIODRST; >> + >> + wdt_write(wdt, AT91_SAM9X60_IER, wdt->ir); >> + wdt_write(wdt, AT91_SAM9X60_WLR, AT91_SAM9X60_SET_COUNTER(val)); >> + } else { >> + wdt->mr |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(MAX_WDT_TIMEOUT)); >> + wdt->mr |= AT91_WDT_SET_WDV(val); >> + >> + if (wdt->need_irq) >> + wdt->mr |= AT91_WDT_WDFIEN; >> + else >> + wdt->mr |= AT91_WDT_WDRSTEN; >> + } >> + >> + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr); >> + >> return 0; >> } >> >> @@ -201,13 +259,14 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) >> struct sama5d4_wdt *wdt; >> void __iomem *regs; >> u32 irq = 0; >> - u32 timeout; >> int ret; >> >> wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); >> if (!wdt) >> return -ENOMEM; >> >> + wdt->data = of_device_get_match_data(&pdev->dev); >> + >> wdd = &wdt->wdd; >> wdd->timeout = WDT_DEFAULT_TIMEOUT; >> wdd->info = &sama5d4_wdt_info; >> @@ -224,15 +283,17 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) >> >> wdt->reg_base = regs; >> >> - irq = irq_of_parse_and_map(dev->of_node, 0); >> - if (!irq) >> - dev_warn(dev, "failed to get IRQ from DT\n"); >> - >> ret = of_sama5d4_wdt_init(dev->of_node, wdt); >> if (ret) >> return ret; >> >> - if ((wdt->mr & AT91_WDT_WDFIEN) && irq) { >> + irq = irq_of_parse_and_map(dev->of_node, 0); >> + if (!irq) { >> + dev_warn(dev, "failed to get IRQ from DT\n"); >> + wdt->need_irq = 0; > > Does it make sense to ignore that ? Hi Guenter, Can you detail what exactly is ignored ? > >> + } >> + >> + if (wdt->need_irq) { >> ret = devm_request_irq(dev, irq, sama5d4_wdt_irq_handler, >> IRQF_SHARED | IRQF_IRQPOLL | >> IRQF_NO_SUSPEND, pdev->name, pdev); >> @@ -244,11 +305,6 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) >> >> watchdog_init_timeout(wdd, wdt_timeout, dev); >> >> - timeout = WDT_SEC2TICKS(wdd->timeout); >> - >> - wdt->mr |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(MAX_WDT_TIMEOUT)); >> - wdt->mr |= AT91_WDT_SET_WDV(timeout); >> - >> ret = sama5d4_wdt_init(wdt); >> if (ret) >> return ret; >> @@ -268,8 +324,21 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) >> return 0; >> } >> >> +static struct sama5d4_wdt_data sama5d4_config; >> + >> +static struct sama5d4_wdt_data sam9x60_config = { >> + .sam9x60_support = 1, >> +}; > > Unless there is reason to believe that there will be other > configuration data, please just assign the flag value directly > to .data and add a variable to struct sama5d4_wdt to access it. > Please make that variable a bool. There will be more configuration data for future products, but not at this moment. Do the change or keep it this way ? I will do the other changes as requested. Thanks for reviewing, Eugen > >> + >> static const struct of_device_id sama5d4_wdt_of_match[] = { >> - { .compatible = "atmel,sama5d4-wdt", }, >> + { >> + .compatible = "atmel,sama5d4-wdt", >> + .data = &sama5d4_config, >> + }, >> + { >> + .compatible = "microchip,sam9x60-wdt", >> + .data = &sam9x60_config, >> + }, >> { } >> }; >> MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match); > >
On Thu, Nov 07, 2019 at 12:51:15PM +0000, Eugen.Hristev@microchip.com wrote: > > >> > >> - if ((wdt->mr & AT91_WDT_WDFIEN) && irq) { > >> + irq = irq_of_parse_and_map(dev->of_node, 0); > >> + if (!irq) { > >> + dev_warn(dev, "failed to get IRQ from DT\n"); > >> + wdt->need_irq = 0; > > > > Does it make sense to ignore that ? > > Hi Guenter, > > Can you detail what exactly is ignored ? > The missing interrupt. > >> +static struct sama5d4_wdt_data sama5d4_config; > >> + > >> +static struct sama5d4_wdt_data sam9x60_config = { > >> + .sam9x60_support = 1, > >> +}; > > > > Unless there is reason to believe that there will be other > > configuration data, please just assign the flag value directly > > to .data and add a variable to struct sama5d4_wdt to access it. > > Please make that variable a bool. > > There will be more configuration data for future products, but not at > this moment. Do the change or keep it this way ? > If not as part of this series, it is better to just assign the flag directly. If there are changes coming at a later time which indeed need a structure (with more than one object in it), that structure can be added at that time. Guenter
On 07.11.2019 18:41, Guenter Roeck wrote: > On Thu, Nov 07, 2019 at 12:51:15PM +0000, Eugen.Hristev@microchip.com wrote: >> >>>> >>>> - if ((wdt->mr & AT91_WDT_WDFIEN) && irq) { >>>> + irq = irq_of_parse_and_map(dev->of_node, 0); >>>> + if (!irq) { >>>> + dev_warn(dev, "failed to get IRQ from DT\n"); >>>> + wdt->need_irq = 0; >>> >>> Does it make sense to ignore that ? >> >> Hi Guenter, >> >> Can you detail what exactly is ignored ? >> > The missing interrupt. Hi, The interrupt is not mandatory for the work of the watchdog. So, if it's not available, we must act accordingly: just configure watchdog to reset the CPU. If we have an IRQ, we can then act in the 'software-style' watchdog: trigger an IRQ first and try to allow the system to a safe reboot. So , it's not ignoring, it's acting accordingly (later on, do not enable anything irq-related ) > >>>> +static struct sama5d4_wdt_data sama5d4_config; >>>> + >>>> +static struct sama5d4_wdt_data sam9x60_config = { >>>> + .sam9x60_support = 1, >>>> +}; >>> >>> Unless there is reason to believe that there will be other >>> configuration data, please just assign the flag value directly >>> to .data and add a variable to struct sama5d4_wdt to access it. >>> Please make that variable a bool. >> >> There will be more configuration data for future products, but not at >> this moment. Do the change or keep it this way ? >> > If not as part of this series, it is better to just assign > the flag directly. If there are changes coming at a later time > which indeed need a structure (with more than one object in it), > that structure can be added at that time. Ok, I will change it in next version. Thanks, Eugen > > Guenter > >
diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h index 390941c..7a053fd 100644 --- a/drivers/watchdog/at91sam9_wdt.h +++ b/drivers/watchdog/at91sam9_wdt.h @@ -20,7 +20,10 @@ #define AT91_WDT_MR 0x04 /* Watchdog Mode Register */ #define AT91_WDT_WDV (0xfff << 0) /* Counter Value */ #define AT91_WDT_SET_WDV(x) ((x) & AT91_WDT_WDV) +#define AT91_SAM9X60_PERIODRST BIT(4) /* Period Reset */ +#define AT91_SAM9X60_RPTHRST BIT(5) /* Minimum Restart Period */ #define AT91_WDT_WDFIEN (1 << 12) /* Fault Interrupt Enable */ +#define AT91_SAM9X60_WDDIS BIT(12) /* Disable */ #define AT91_WDT_WDRSTEN (1 << 13) /* Reset Processor */ #define AT91_WDT_WDRPROC (1 << 14) /* Timer Restart */ #define AT91_WDT_WDDIS (1 << 15) /* Watchdog Disable */ @@ -33,4 +36,15 @@ #define AT91_WDT_WDUNF (1 << 0) /* Watchdog Underflow */ #define AT91_WDT_WDERR (1 << 1) /* Watchdog Error */ +#define AT91_SAM9X60_VR 0x08 /* Watchdog Timer Value Register */ + +#define AT91_SAM9X60_WLR 0x0c +#define AT91_SAM9X60_COUNTER (0xfff << 0) /* Watchdog Period Value */ +#define AT91_SAM9X60_SET_COUNTER(x) ((x) & AT91_SAM9X60_COUNTER) + +#define AT91_SAM9X60_IER 0x14 /* Interrupt Enable Register */ +#define AT91_SAM9X60_PERINT BIT(0) /* Period Interrupt Enable */ +#define AT91_SAM9X60_IDR 0x18 /* Interrupt Disable Register */ +#define AT91_SAM9X60_ISR 0x1c /* Interrupt Status Register */ + #endif diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c index d193a60..b92afd7 100644 --- a/drivers/watchdog/sama5d4_wdt.c +++ b/drivers/watchdog/sama5d4_wdt.c @@ -2,7 +2,7 @@ /* * Driver for Atmel SAMA5D4 Watchdog Timer * - * Copyright (C) 2015 Atmel Corporation + * Copyright (C) 2015-2019 Microchip Technology Inc. and its subsidiaries */ #include <linux/delay.h> @@ -11,6 +11,7 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/of_irq.h> #include <linux/platform_device.h> #include <linux/reboot.h> @@ -25,11 +26,18 @@ #define WDT_SEC2TICKS(s) ((s) ? (((s) << 8) - 1) : 0) +struct sama5d4_wdt_data { + const unsigned int sam9x60_support; +}; + struct sama5d4_wdt { - struct watchdog_device wdd; - void __iomem *reg_base; - u32 mr; - unsigned long last_ping; + struct watchdog_device wdd; + const struct sama5d4_wdt_data *data; + void __iomem *reg_base; + u32 mr; + u32 ir; + unsigned long last_ping; + unsigned int need_irq:1; }; static int wdt_timeout; @@ -78,7 +86,12 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd) { struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd); - wdt->mr &= ~AT91_WDT_WDDIS; + if (wdt->data->sam9x60_support) { + writel_relaxed(wdt->ir, wdt->reg_base + AT91_SAM9X60_IER); + wdt->mr &= ~AT91_SAM9X60_WDDIS; + } else { + wdt->mr &= ~AT91_WDT_WDDIS; + } wdt_write(wdt, AT91_WDT_MR, wdt->mr); return 0; @@ -88,7 +101,12 @@ static int sama5d4_wdt_stop(struct watchdog_device *wdd) { struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd); - wdt->mr |= AT91_WDT_WDDIS; + if (wdt->data->sam9x60_support) { + writel_relaxed(wdt->ir, wdt->reg_base + AT91_SAM9X60_IDR); + wdt->mr |= AT91_SAM9X60_WDDIS; + } else { + wdt->mr |= AT91_WDT_WDDIS; + } wdt_write(wdt, AT91_WDT_MR, wdt->mr); return 0; @@ -109,6 +127,14 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd, struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd); u32 value = WDT_SEC2TICKS(timeout); + if (wdt->data->sam9x60_support) { + wdt_write(wdt, AT91_SAM9X60_WLR, + AT91_SAM9X60_SET_COUNTER(value)); + + wdd->timeout = timeout; + return 0; + } + wdt->mr &= ~AT91_WDT_WDV; wdt->mr |= AT91_WDT_SET_WDV(value); @@ -143,8 +169,14 @@ static const struct watchdog_ops sama5d4_wdt_ops = { static irqreturn_t sama5d4_wdt_irq_handler(int irq, void *dev_id) { struct sama5d4_wdt *wdt = platform_get_drvdata(dev_id); + u32 reg; - if (wdt_read(wdt, AT91_WDT_SR)) { + if (wdt->data->sam9x60_support) + reg = wdt_read(wdt, AT91_SAM9X60_ISR); + else + reg = wdt_read(wdt, AT91_WDT_SR); + + if (reg) { pr_crit("Atmel Watchdog Software Reset\n"); emergency_restart(); pr_crit("Reboot didn't succeed\n"); @@ -157,13 +189,14 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt) { const char *tmp; - wdt->mr = AT91_WDT_WDDIS; + if (wdt->data->sam9x60_support) + wdt->mr = AT91_SAM9X60_WDDIS; + else + wdt->mr = AT91_WDT_WDDIS; if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) && !strcmp(tmp, "software")) - wdt->mr |= AT91_WDT_WDFIEN; - else - wdt->mr |= AT91_WDT_WDRSTEN; + wdt->need_irq = 1; if (of_property_read_bool(np, "atmel,idle-halt")) wdt->mr |= AT91_WDT_WDIDLEHLT; @@ -176,21 +209,46 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt) static int sama5d4_wdt_init(struct sama5d4_wdt *wdt) { - u32 reg; + u32 reg, val; + + val = WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT); /* * When booting and resuming, the bootloader may have changed the * watchdog configuration. * If the watchdog is already running, we can safely update it. * Else, we have to disable it properly. */ - if (wdt_enabled) { - wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr); - } else { + if (!wdt_enabled) { reg = wdt_read(wdt, AT91_WDT_MR); - if (!(reg & AT91_WDT_WDDIS)) + if (wdt->data->sam9x60_support && (!(reg & AT91_SAM9X60_WDDIS))) + wdt_write_nosleep(wdt, AT91_WDT_MR, + reg | AT91_SAM9X60_WDDIS); + else if (!wdt->data->sam9x60_support && + (!(reg & AT91_WDT_WDDIS))) wdt_write_nosleep(wdt, AT91_WDT_MR, reg | AT91_WDT_WDDIS); } + + if (wdt->data->sam9x60_support) { + if (wdt->need_irq) + wdt->ir = AT91_SAM9X60_PERINT; + else + wdt->mr |= AT91_SAM9X60_PERIODRST; + + wdt_write(wdt, AT91_SAM9X60_IER, wdt->ir); + wdt_write(wdt, AT91_SAM9X60_WLR, AT91_SAM9X60_SET_COUNTER(val)); + } else { + wdt->mr |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(MAX_WDT_TIMEOUT)); + wdt->mr |= AT91_WDT_SET_WDV(val); + + if (wdt->need_irq) + wdt->mr |= AT91_WDT_WDFIEN; + else + wdt->mr |= AT91_WDT_WDRSTEN; + } + + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr); + return 0; } @@ -201,13 +259,14 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) struct sama5d4_wdt *wdt; void __iomem *regs; u32 irq = 0; - u32 timeout; int ret; wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); if (!wdt) return -ENOMEM; + wdt->data = of_device_get_match_data(&pdev->dev); + wdd = &wdt->wdd; wdd->timeout = WDT_DEFAULT_TIMEOUT; wdd->info = &sama5d4_wdt_info; @@ -224,15 +283,17 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) wdt->reg_base = regs; - irq = irq_of_parse_and_map(dev->of_node, 0); - if (!irq) - dev_warn(dev, "failed to get IRQ from DT\n"); - ret = of_sama5d4_wdt_init(dev->of_node, wdt); if (ret) return ret; - if ((wdt->mr & AT91_WDT_WDFIEN) && irq) { + irq = irq_of_parse_and_map(dev->of_node, 0); + if (!irq) { + dev_warn(dev, "failed to get IRQ from DT\n"); + wdt->need_irq = 0; + } + + if (wdt->need_irq) { ret = devm_request_irq(dev, irq, sama5d4_wdt_irq_handler, IRQF_SHARED | IRQF_IRQPOLL | IRQF_NO_SUSPEND, pdev->name, pdev); @@ -244,11 +305,6 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) watchdog_init_timeout(wdd, wdt_timeout, dev); - timeout = WDT_SEC2TICKS(wdd->timeout); - - wdt->mr |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(MAX_WDT_TIMEOUT)); - wdt->mr |= AT91_WDT_SET_WDV(timeout); - ret = sama5d4_wdt_init(wdt); if (ret) return ret; @@ -268,8 +324,21 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) return 0; } +static struct sama5d4_wdt_data sama5d4_config; + +static struct sama5d4_wdt_data sam9x60_config = { + .sam9x60_support = 1, +}; + static const struct of_device_id sama5d4_wdt_of_match[] = { - { .compatible = "atmel,sama5d4-wdt", }, + { + .compatible = "atmel,sama5d4-wdt", + .data = &sama5d4_config, + }, + { + .compatible = "microchip,sam9x60-wdt", + .data = &sam9x60_config, + }, { } }; MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);