| Message ID | 20200301094040.123189-5-tmaimon77@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | watchdog: npcm: support new capabilities | expand |
On 3/1/20 1:40 AM, Tomer Maimon wrote: > During probe NPCM watchdog sets the following bootstatus flags: > - WDIOF_CARDRESET represent power and core reset. > - WDIOF_EXTERN1 represent watchdog 0-2 reset. > - WDIOF_EXTERN2 represent software 1-4 reset. > > Each flag is representing a group of bootstatus. > The user can configure through the device treethe exact reset > to each flag group. > Sorry, this doesn't make sense to me. I could understand reporting the above, but it looks to me like devicetree is used to associate a reset bit from the controller with one of the above. Devicetree only seems to be used to associate reset status bits from the controller with WDIOF_CARDRESET, WDIOF_EXTERN1, or WDIOF_EXTERN2. That adds a lot of complexity for little if any gain. It would make sense to set the bootstatus bits as suggested above, but that doesn't require devicetree properties. More comments inline. Guenter > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > drivers/watchdog/npcm_wdt.c | 132 ++++++++++++++++++++++++++++++++---- > 1 file changed, 119 insertions(+), 13 deletions(-) > > diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c > index 8609c7acf17d..dba9a73249c9 100644 > --- a/drivers/watchdog/npcm_wdt.c > +++ b/drivers/watchdog/npcm_wdt.c > @@ -11,7 +11,24 @@ > #include <linux/platform_device.h> > #include <linux/slab.h> > #include <linux/watchdog.h> > - > +#include <linux/regmap.h> > +#include <linux/mfd/syscon.h> > + New include files in alphabetic order merged with existing ones, please. > +/* NPCM7xx GCR module */ > +#define NPCM7XX_RESSR_OFFSET 0x6C > +#define NPCM7XX_INTCR2_OFFSET 0x60 > + > +#define NPCM7XX_PORST BIT(31) > +#define NPCM7XX_CORST BIT(30) > +#define NPCM7XX_WD0RST BIT(29) > +#define NPCM7XX_WD1RST BIT(24) > +#define NPCM7XX_WD2RST BIT(23) > +#define NPCM7XX_SWR1RST BIT(28) > +#define NPCM7XX_SWR2RST BIT(27) > +#define NPCM7XX_SWR3RST BIT(26) > +#define NPCM7XX_SWR4RST BIT(25) > + > + /* WD register */ > #define NPCM_WTCR 0x1C > > #define NPCM_WTCLK (BIT(10) | BIT(11)) /* Clock divider */ > @@ -43,6 +60,9 @@ > struct npcm_wdt { > struct watchdog_device wdd; > void __iomem *reg; > + u32 card_reset; > + u32 ext1_reset; > + u32 ext2_reset; > }; > > static inline struct npcm_wdt *to_npcm_wdt(struct watchdog_device *wdd) > @@ -103,30 +123,29 @@ static int npcm_wdt_stop(struct watchdog_device *wdd) > return 0; > } > > - > static int npcm_wdt_set_timeout(struct watchdog_device *wdd, > unsigned int timeout) > { > if (timeout < 2) > wdd->timeout = 1; > else if (timeout < 3) > - wdd->timeout = 2; > + wdd->timeout = 2; > else if (timeout < 6) > - wdd->timeout = 5; > + wdd->timeout = 5; > else if (timeout < 11) > - wdd->timeout = 10; > + wdd->timeout = 10; > else if (timeout < 22) > - wdd->timeout = 21; > + wdd->timeout = 21; > else if (timeout < 44) > - wdd->timeout = 43; > + wdd->timeout = 43; > else if (timeout < 87) > - wdd->timeout = 86; > + wdd->timeout = 86; > else if (timeout < 173) > - wdd->timeout = 172; > + wdd->timeout = 172; > else if (timeout < 688) > - wdd->timeout = 687; > + wdd->timeout = 687; > else > - wdd->timeout = 2750; > + wdd->timeout = 2750; > Whitespace changes in a separate patch, please. > if (watchdog_active(wdd)) > npcm_wdt_start(wdd); > @@ -177,9 +196,61 @@ static const struct watchdog_ops npcm_wdt_ops = { > .restart = npcm_wdt_restart, > }; > > +static void npcm_get_reset_status(struct npcm_wdt *wdt, struct device *dev) > +{ > + struct regmap *gcr_regmap; > + u32 rstval; > + > + if (of_device_is_compatible(dev->of_node, "nuvoton,npcm750-wdt")) { > + gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr"); > + if (IS_ERR(gcr_regmap)) > + dev_warn(dev, "Failed to find nuvoton,npcm750-gcr WD reset status not supported\n"); > + > + regmap_read(gcr_regmap, NPCM7XX_RESSR_OFFSET, &rstval); > + if (!rstval) { > + regmap_read(gcr_regmap, NPCM7XX_INTCR2_OFFSET, &rstval); > + rstval = ~rstval; > + } The second register reports the same as the first only negated if bits in the first register are not set ? That seems unlikely. Please point to the datasheet, or at least provide a reference to the two registers. > + > + if (rstval & wdt->card_reset) > + wdt->wdd.bootstatus |= WDIOF_CARDRESET; > + if (rstval & wdt->ext1_reset) > + wdt->wdd.bootstatus |= WDIOF_EXTERN1; > + if (rstval & wdt->ext2_reset) > + wdt->wdd.bootstatus |= WDIOF_EXTERN2; > + } > +} > + > +static u32 npcm_wdt_reset_type(const char *reset_type) > +{ > + if (!strcmp(reset_type, "porst")) > + return NPCM7XX_PORST; > + else if (!strcmp(reset_type, "corst")) > + return NPCM7XX_CORST; > + else if (!strcmp(reset_type, "wd0")) > + return NPCM7XX_WD0RST; > + else if (!strcmp(reset_type, "wd1")) > + return NPCM7XX_WD1RST; > + else if (!strcmp(reset_type, "wd2")) > + return NPCM7XX_WD2RST; > + else if (!strcmp(reset_type, "sw1")) > + return NPCM7XX_SWR1RST; > + else if (!strcmp(reset_type, "sw2")) > + return NPCM7XX_SWR2RST; > + else if (!strcmp(reset_type, "sw3")) > + return NPCM7XX_SWR3RST; > + else if (!strcmp(reset_type, "sw4")) > + return NPCM7XX_SWR4RST; > + > + return 0; > +} > + > static int npcm_wdt_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + const char *card_reset_type; > + const char *ext1_reset_type; > + const char *ext2_reset_type; > struct npcm_wdt *wdt; > u32 priority; > int irq; > @@ -202,6 +273,39 @@ static int npcm_wdt_probe(struct platform_device *pdev) > else > watchdog_set_restart_priority(&wdt->wdd, priority); > > + ret = of_property_read_string(pdev->dev.of_node, > + "nuvoton,card-reset-type", > + &card_reset_type); > + if (ret) { > + wdt->card_reset = NPCM7XX_PORST; > + } else { > + wdt->card_reset = npcm_wdt_reset_type(card_reset_type); > + if (!wdt->card_reset) > + wdt->card_reset = NPCM7XX_PORST; > + } > + > + ret = of_property_read_string(pdev->dev.of_node, > + "nuvoton,ext1-reset-type", > + &ext1_reset_type); > + if (ret) { > + wdt->ext1_reset = NPCM7XX_WD0RST; > + } else { > + wdt->ext1_reset = npcm_wdt_reset_type(ext1_reset_type); > + if (!wdt->ext1_reset) > + wdt->ext1_reset = NPCM7XX_WD0RST; > + } > + > + ret = of_property_read_string(pdev->dev.of_node, > + "nuvoton,ext2-reset-type", > + &ext2_reset_type); > + if (ret) { > + wdt->ext2_reset = NPCM7XX_SWR1RST; > + } else { > + wdt->ext2_reset = npcm_wdt_reset_type(ext2_reset_type); > + if (!wdt->ext2_reset) > + wdt->ext2_reset = NPCM7XX_SWR1RST; > + } > + > wdt->wdd.info = &npcm_wdt_info; > wdt->wdd.ops = &npcm_wdt_ops; > wdt->wdd.min_timeout = 1; > @@ -220,8 +324,10 @@ static int npcm_wdt_probe(struct platform_device *pdev) > set_bit(WDOG_HW_RUNNING, &wdt->wdd.status); > } > > - ret = devm_request_irq(dev, irq, npcm_wdt_interrupt, 0, "watchdog", > - wdt); > + npcm_get_reset_status(wdt, dev); > + > + ret = devm_request_irq(dev, irq, npcm_wdt_interrupt, 0, > + "watchdog", wdt); > if (ret) > return ret; > >
On 3/1/20 8:08 AM, Tomer Maimon wrote: > Sorry Guenter probebly I didnt explain it well. > > > On Sun, 1 Mar 2020 at 12:48, Guenter Roeck <linux@roeck-us.net <mailto:linux@roeck-us.net>> wrote: > > On 3/1/20 1:40 AM, Tomer Maimon wrote: > > During probe NPCM watchdog sets the following bootstatus flags: > > - WDIOF_CARDRESET represent power and core reset. > > - WDIOF_EXTERN1 represent watchdog 0-2 reset. > > - WDIOF_EXTERN2 represent software 1-4 reset. > > > > Each flag is representing a group of bootstatus. > > The user can configure through the device treethe exact reset > > to each flag group. > > > > Sorry, this doesn't make sense to me. I could understand reporting > the above, but it looks to me like devicetree is used to associate > a reset bit from the controller with one of the above. > Devicetree only seems to be used to associate reset status bits > from the controller with WDIOF_CARDRESET, WDIOF_EXTERN1, or > WDIOF_EXTERN2. That adds a lot of complexity for little if any > gain. > > > > It would make sense to set the bootstatus bits as suggested above, > but that doesn't require devicetree properties. > > More comments inline. > > Guenter > > > > In the NPCM750 we have the following reset types: > > 1. board reset (Power on reset, Core reset) > 2. WD reset (0-2 WD reset). > 3. SW reset (1-4 SW reset). > > > Each board can use different reset types, because in the WD status bit there is not enough bits to represent the entire NPCM750 resets. > > The NPCM750 reset groups are represent as follow: > > - WDIOF_CARDRESET represent power and core reset. > - WDIOF_EXTERN1 represent watchdog 0-2 reset. > - WDIOF_EXTERN2 represent software 1-4 reset. > Exactly, and I don't see a need to be more specific than that. This can be implemented without all the DT complexity. Guenter
diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c index 8609c7acf17d..dba9a73249c9 100644 --- a/drivers/watchdog/npcm_wdt.c +++ b/drivers/watchdog/npcm_wdt.c @@ -11,7 +11,24 @@ #include <linux/platform_device.h> #include <linux/slab.h> #include <linux/watchdog.h> - +#include <linux/regmap.h> +#include <linux/mfd/syscon.h> + +/* NPCM7xx GCR module */ +#define NPCM7XX_RESSR_OFFSET 0x6C +#define NPCM7XX_INTCR2_OFFSET 0x60 + +#define NPCM7XX_PORST BIT(31) +#define NPCM7XX_CORST BIT(30) +#define NPCM7XX_WD0RST BIT(29) +#define NPCM7XX_WD1RST BIT(24) +#define NPCM7XX_WD2RST BIT(23) +#define NPCM7XX_SWR1RST BIT(28) +#define NPCM7XX_SWR2RST BIT(27) +#define NPCM7XX_SWR3RST BIT(26) +#define NPCM7XX_SWR4RST BIT(25) + + /* WD register */ #define NPCM_WTCR 0x1C #define NPCM_WTCLK (BIT(10) | BIT(11)) /* Clock divider */ @@ -43,6 +60,9 @@ struct npcm_wdt { struct watchdog_device wdd; void __iomem *reg; + u32 card_reset; + u32 ext1_reset; + u32 ext2_reset; }; static inline struct npcm_wdt *to_npcm_wdt(struct watchdog_device *wdd) @@ -103,30 +123,29 @@ static int npcm_wdt_stop(struct watchdog_device *wdd) return 0; } - static int npcm_wdt_set_timeout(struct watchdog_device *wdd, unsigned int timeout) { if (timeout < 2) wdd->timeout = 1; else if (timeout < 3) - wdd->timeout = 2; + wdd->timeout = 2; else if (timeout < 6) - wdd->timeout = 5; + wdd->timeout = 5; else if (timeout < 11) - wdd->timeout = 10; + wdd->timeout = 10; else if (timeout < 22) - wdd->timeout = 21; + wdd->timeout = 21; else if (timeout < 44) - wdd->timeout = 43; + wdd->timeout = 43; else if (timeout < 87) - wdd->timeout = 86; + wdd->timeout = 86; else if (timeout < 173) - wdd->timeout = 172; + wdd->timeout = 172; else if (timeout < 688) - wdd->timeout = 687; + wdd->timeout = 687; else - wdd->timeout = 2750; + wdd->timeout = 2750; if (watchdog_active(wdd)) npcm_wdt_start(wdd); @@ -177,9 +196,61 @@ static const struct watchdog_ops npcm_wdt_ops = { .restart = npcm_wdt_restart, }; +static void npcm_get_reset_status(struct npcm_wdt *wdt, struct device *dev) +{ + struct regmap *gcr_regmap; + u32 rstval; + + if (of_device_is_compatible(dev->of_node, "nuvoton,npcm750-wdt")) { + gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr"); + if (IS_ERR(gcr_regmap)) + dev_warn(dev, "Failed to find nuvoton,npcm750-gcr WD reset status not supported\n"); + + regmap_read(gcr_regmap, NPCM7XX_RESSR_OFFSET, &rstval); + if (!rstval) { + regmap_read(gcr_regmap, NPCM7XX_INTCR2_OFFSET, &rstval); + rstval = ~rstval; + } + + if (rstval & wdt->card_reset) + wdt->wdd.bootstatus |= WDIOF_CARDRESET; + if (rstval & wdt->ext1_reset) + wdt->wdd.bootstatus |= WDIOF_EXTERN1; + if (rstval & wdt->ext2_reset) + wdt->wdd.bootstatus |= WDIOF_EXTERN2; + } +} + +static u32 npcm_wdt_reset_type(const char *reset_type) +{ + if (!strcmp(reset_type, "porst")) + return NPCM7XX_PORST; + else if (!strcmp(reset_type, "corst")) + return NPCM7XX_CORST; + else if (!strcmp(reset_type, "wd0")) + return NPCM7XX_WD0RST; + else if (!strcmp(reset_type, "wd1")) + return NPCM7XX_WD1RST; + else if (!strcmp(reset_type, "wd2")) + return NPCM7XX_WD2RST; + else if (!strcmp(reset_type, "sw1")) + return NPCM7XX_SWR1RST; + else if (!strcmp(reset_type, "sw2")) + return NPCM7XX_SWR2RST; + else if (!strcmp(reset_type, "sw3")) + return NPCM7XX_SWR3RST; + else if (!strcmp(reset_type, "sw4")) + return NPCM7XX_SWR4RST; + + return 0; +} + static int npcm_wdt_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; + const char *card_reset_type; + const char *ext1_reset_type; + const char *ext2_reset_type; struct npcm_wdt *wdt; u32 priority; int irq; @@ -202,6 +273,39 @@ static int npcm_wdt_probe(struct platform_device *pdev) else watchdog_set_restart_priority(&wdt->wdd, priority); + ret = of_property_read_string(pdev->dev.of_node, + "nuvoton,card-reset-type", + &card_reset_type); + if (ret) { + wdt->card_reset = NPCM7XX_PORST; + } else { + wdt->card_reset = npcm_wdt_reset_type(card_reset_type); + if (!wdt->card_reset) + wdt->card_reset = NPCM7XX_PORST; + } + + ret = of_property_read_string(pdev->dev.of_node, + "nuvoton,ext1-reset-type", + &ext1_reset_type); + if (ret) { + wdt->ext1_reset = NPCM7XX_WD0RST; + } else { + wdt->ext1_reset = npcm_wdt_reset_type(ext1_reset_type); + if (!wdt->ext1_reset) + wdt->ext1_reset = NPCM7XX_WD0RST; + } + + ret = of_property_read_string(pdev->dev.of_node, + "nuvoton,ext2-reset-type", + &ext2_reset_type); + if (ret) { + wdt->ext2_reset = NPCM7XX_SWR1RST; + } else { + wdt->ext2_reset = npcm_wdt_reset_type(ext2_reset_type); + if (!wdt->ext2_reset) + wdt->ext2_reset = NPCM7XX_SWR1RST; + } + wdt->wdd.info = &npcm_wdt_info; wdt->wdd.ops = &npcm_wdt_ops; wdt->wdd.min_timeout = 1; @@ -220,8 +324,10 @@ static int npcm_wdt_probe(struct platform_device *pdev) set_bit(WDOG_HW_RUNNING, &wdt->wdd.status); } - ret = devm_request_irq(dev, irq, npcm_wdt_interrupt, 0, "watchdog", - wdt); + npcm_get_reset_status(wdt, dev); + + ret = devm_request_irq(dev, irq, npcm_wdt_interrupt, 0, + "watchdog", wdt); if (ret) return ret;
During probe NPCM watchdog sets the following bootstatus flags: - WDIOF_CARDRESET represent power and core reset. - WDIOF_EXTERN1 represent watchdog 0-2 reset. - WDIOF_EXTERN2 represent software 1-4 reset. Each flag is representing a group of bootstatus. The user can configure through the device treethe exact reset to each flag group. Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> --- drivers/watchdog/npcm_wdt.c | 132 ++++++++++++++++++++++++++++++++---- 1 file changed, 119 insertions(+), 13 deletions(-)