From: <Eugen.Hristev@microchip.com> To: <linux@roeck-us.net> Cc: <wim@linux-watchdog.org>, <robh+dt@kernel.org>, <Nicolas.Ferre@microchip.com>, <alexandre.belloni@bootlin.com>, <linux-watchdog@vger.kernel.org>, <devicetree@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v2 2/2] watchdog: sama5d4_wdt: addition of sam9x60 compatible watchdog Date: Thu, 7 Nov 2019 12:51:15 +0000 [thread overview] Message-ID: <28c6b394-ae88-f913-312e-6b38be1dc5a8@microchip.com> (raw) In-Reply-To: <20191029132831.GA5643@roeck-us.net> 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); > >
WARNING: multiple messages have this Message-ID (diff)
From: <Eugen.Hristev@microchip.com> To: <linux@roeck-us.net> Cc: devicetree@vger.kernel.org, alexandre.belloni@bootlin.com, linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org, robh+dt@kernel.org, wim@linux-watchdog.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 2/2] watchdog: sama5d4_wdt: addition of sam9x60 compatible watchdog Date: Thu, 7 Nov 2019 12:51:15 +0000 [thread overview] Message-ID: <28c6b394-ae88-f913-312e-6b38be1dc5a8@microchip.com> (raw) In-Reply-To: <20191029132831.GA5643@roeck-us.net> 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); > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-11-07 12:51 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-21 9:14 [PATCH v2 1/2] dt-bindings: watchdog: sama5d4_wdt: add microchip,sam9x60-wdt compatible Eugen.Hristev 2019-10-21 9:14 ` Eugen.Hristev 2019-10-21 9:14 ` [PATCH v2 2/2] watchdog: sama5d4_wdt: addition of sam9x60 compatible watchdog Eugen.Hristev 2019-10-21 9:14 ` Eugen.Hristev 2019-10-29 13:28 ` Guenter Roeck 2019-10-29 13:28 ` Guenter Roeck 2019-11-07 12:51 ` Eugen.Hristev [this message] 2019-11-07 12:51 ` Eugen.Hristev 2019-11-07 16:41 ` Guenter Roeck 2019-11-07 16:41 ` Guenter Roeck 2019-11-07 17:10 ` Eugen.Hristev 2019-11-07 17:10 ` Eugen.Hristev 2019-10-25 21:33 ` [PATCH v2 1/2] dt-bindings: watchdog: sama5d4_wdt: add microchip,sam9x60-wdt compatible Rob Herring 2019-10-25 21:33 ` Rob Herring 2019-10-29 13:16 ` Guenter Roeck 2019-10-29 13:16 ` Guenter Roeck
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=28c6b394-ae88-f913-312e-6b38be1dc5a8@microchip.com \ --to=eugen.hristev@microchip.com \ --cc=Nicolas.Ferre@microchip.com \ --cc=alexandre.belloni@bootlin.com \ --cc=devicetree@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-watchdog@vger.kernel.org \ --cc=linux@roeck-us.net \ --cc=robh+dt@kernel.org \ --cc=wim@linux-watchdog.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.