* [PATCH v1 0/4] watchdog: npcm: support new capabilities
@ 2020-03-01 9:40 Tomer Maimon
2020-03-01 9:40 ` [PATCH v1 1/4] dt-binding: watchdog: add restart priority documentation Tomer Maimon
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Tomer Maimon @ 2020-03-01 9:40 UTC (permalink / raw)
To: wim, linux, robh+dt, mark.rutland, joel, avifishman70,
tali.perry1, yuenn, benjaminfair
Cc: linux-watchdog, devicetree, linux-kernel, openbmc, Tomer Maimon
This patch set adds last reset bootstatus and restart priority
support in watchdog the Nuvoton NPCM Baseboard Management
Controller (BMC).
The NPCM watchdog driver tested on NPCM750 evaluation board.
Tomer Maimon (4):
dt-binding: watchdog: add restart priority documentation
watchdog: npcm: add restart priority support
dt-binding: watchdog: add bootstatus reset type documentation
watchdog: npcm: sets card ext1 and ext2 bootstatus during probe
.../bindings/watchdog/nuvoton,npcm-wdt.txt | 32 ++++
drivers/watchdog/npcm_wdt.c | 138 ++++++++++++++++--
2 files changed, 157 insertions(+), 13 deletions(-)
--
2.22.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 1/4] dt-binding: watchdog: add restart priority documentation
2020-03-01 9:40 [PATCH v1 0/4] watchdog: npcm: support new capabilities Tomer Maimon
@ 2020-03-01 9:40 ` Tomer Maimon
2020-03-01 10:05 ` Guenter Roeck
2020-03-01 9:40 ` [PATCH v1 2/4] watchdog: npcm: add restart priority support Tomer Maimon
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Tomer Maimon @ 2020-03-01 9:40 UTC (permalink / raw)
To: wim, linux, robh+dt, mark.rutland, joel, avifishman70,
tali.perry1, yuenn, benjaminfair
Cc: linux-watchdog, devicetree, linux-kernel, openbmc, Tomer Maimon
Add device tree restart priority documentation.
Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt b/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt
index 6d593003c933..0a0f86a25eb0 100644
--- a/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt
@@ -17,6 +17,7 @@ Required clocking property, have to be one of:
Optional properties:
- timeout-sec : Contains the watchdog timeout in seconds
+- nuvoton,restart-priority : Contains the card restart priority.
Example:
@@ -25,4 +26,5 @@ timer@f000801c {
interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
reg = <0xf000801c 0x4>;
clocks = <&clk NPCM7XX_CLK_TIMER>;
+ nuvoton,restart-priority = <155>;
};
--
2.22.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v1 2/4] watchdog: npcm: add restart priority support
2020-03-01 9:40 [PATCH v1 0/4] watchdog: npcm: support new capabilities Tomer Maimon
2020-03-01 9:40 ` [PATCH v1 1/4] dt-binding: watchdog: add restart priority documentation Tomer Maimon
@ 2020-03-01 9:40 ` Tomer Maimon
2020-03-01 9:40 ` [PATCH v1 3/4] dt-binding: watchdog: add bootstatus reset type documentation Tomer Maimon
2020-03-01 9:40 ` [PATCH v1 4/4] watchdog: npcm: sets card ext1 and ext2 bootstatus during probe Tomer Maimon
3 siblings, 0 replies; 13+ messages in thread
From: Tomer Maimon @ 2020-03-01 9:40 UTC (permalink / raw)
To: wim, linux, robh+dt, mark.rutland, joel, avifishman70,
tali.perry1, yuenn, benjaminfair
Cc: linux-watchdog, devicetree, linux-kernel, openbmc, Tomer Maimon
Add NPCM watchdog restart priority support.
The default restart priority is 128.
Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
drivers/watchdog/npcm_wdt.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c
index 9c773c3d6d5d..8609c7acf17d 100644
--- a/drivers/watchdog/npcm_wdt.c
+++ b/drivers/watchdog/npcm_wdt.c
@@ -181,6 +181,7 @@ static int npcm_wdt_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct npcm_wdt *wdt;
+ u32 priority;
int irq;
int ret;
@@ -196,6 +197,11 @@ static int npcm_wdt_probe(struct platform_device *pdev)
if (irq < 0)
return irq;
+ if (of_property_read_u32(pdev->dev.of_node, "nuvoton,restart-priority", &priority))
+ watchdog_set_restart_priority(&wdt->wdd, 128);
+ else
+ watchdog_set_restart_priority(&wdt->wdd, priority);
+
wdt->wdd.info = &npcm_wdt_info;
wdt->wdd.ops = &npcm_wdt_ops;
wdt->wdd.min_timeout = 1;
--
2.22.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v1 3/4] dt-binding: watchdog: add bootstatus reset type documentation
2020-03-01 9:40 [PATCH v1 0/4] watchdog: npcm: support new capabilities Tomer Maimon
2020-03-01 9:40 ` [PATCH v1 1/4] dt-binding: watchdog: add restart priority documentation Tomer Maimon
2020-03-01 9:40 ` [PATCH v1 2/4] watchdog: npcm: add restart priority support Tomer Maimon
@ 2020-03-01 9:40 ` Tomer Maimon
2020-03-01 9:40 ` [PATCH v1 4/4] watchdog: npcm: sets card ext1 and ext2 bootstatus during probe Tomer Maimon
3 siblings, 0 replies; 13+ messages in thread
From: Tomer Maimon @ 2020-03-01 9:40 UTC (permalink / raw)
To: wim, linux, robh+dt, mark.rutland, joel, avifishman70,
tali.perry1, yuenn, benjaminfair
Cc: linux-watchdog, devicetree, linux-kernel, openbmc, Tomer Maimon
Add device tree three bootstatus reset types documentation.
Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
.../bindings/watchdog/nuvoton,npcm-wdt.txt | 30 +++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt b/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt
index 0a0f86a25eb0..836e0ac29196 100644
--- a/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt
@@ -18,6 +18,33 @@ Required clocking property, have to be one of:
Optional properties:
- timeout-sec : Contains the watchdog timeout in seconds
- nuvoton,restart-priority : Contains the card restart priority.
+- nuvoton,card-reset-type : "porst|corst|wd0|wd1|wd2|sw1|sw2|sw3|sw4"
+ Contains the card reset type for checking and indicating
+ the last card reset status (WDIOF_CARDRESET)
+
+ If 'nuvoton,card-reset-type' is not specified the default is porst
+
+ Reset types:
+ - porst: Power reset
+ - corst: Core reset
+ - wdX : Watchdog reset X (X represante 0-2)
+ - swX : Software reset X (X represante 1-4)
+
+- nuvoton,ext1-reset-type : "porst|corst|wd0|wd1|wd2|sw1|sw2|sw3|sw4"
+ Contains the external 2 reset type for checking and indicating
+ the last external 2 reset status (WDIOF_EXTERN1)
+
+ If 'nuvoton,card-reset-type' is not specified the default is wd0.
+
+ Reset types are the same as in nuvoton,card-reset-type property.
+
+- nuvoton,ext2-reset-type : "porst|corst|wd0|wd1|wd2|sw1|sw2|sw3|sw4"
+ Contains the external 2 reset type for checking and indicating
+ the last external 2 reset status (WDIOF_EXTERN2)
+
+ If 'nuvoton,card-reset-type' is not specified the default is sw1.
+
+ Reset types are the same as in nuvoton,card-reset-type property.
Example:
@@ -27,4 +54,7 @@ timer@f000801c {
reg = <0xf000801c 0x4>;
clocks = <&clk NPCM7XX_CLK_TIMER>;
nuvoton,restart-priority = <155>;
+ nuvoton,card-reset-type = "porst";
+ nuvoton,ext1-reset-type = "wd1";
+ nuvoton,ext2-reset-type = "sw2";
};
--
2.22.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v1 4/4] watchdog: npcm: sets card ext1 and ext2 bootstatus during probe
2020-03-01 9:40 [PATCH v1 0/4] watchdog: npcm: support new capabilities Tomer Maimon
` (2 preceding siblings ...)
2020-03-01 9:40 ` [PATCH v1 3/4] dt-binding: watchdog: add bootstatus reset type documentation Tomer Maimon
@ 2020-03-01 9:40 ` Tomer Maimon
2020-03-01 10:48 ` Guenter Roeck
3 siblings, 1 reply; 13+ messages in thread
From: Tomer Maimon @ 2020-03-01 9:40 UTC (permalink / raw)
To: wim, linux, robh+dt, mark.rutland, joel, avifishman70,
tali.perry1, yuenn, benjaminfair
Cc: linux-watchdog, devicetree, linux-kernel, openbmc, Tomer Maimon
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(-)
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;
--
2.22.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/4] dt-binding: watchdog: add restart priority documentation
2020-03-01 9:40 ` [PATCH v1 1/4] dt-binding: watchdog: add restart priority documentation Tomer Maimon
@ 2020-03-01 10:05 ` Guenter Roeck
2020-03-01 15:36 ` Tomer Maimon
0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2020-03-01 10:05 UTC (permalink / raw)
To: Tomer Maimon, wim, robh+dt, mark.rutland, joel, avifishman70,
tali.perry1, yuenn, benjaminfair
Cc: linux-watchdog, devicetree, linux-kernel, openbmc
On 3/1/20 1:40 AM, Tomer Maimon wrote:
> Add device tree restart priority documentation.
>
I think this warrants an explanation _why_ this is needed.
What is the use case ? Not just theory, please.
Guenter
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
> Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt b/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt
> index 6d593003c933..0a0f86a25eb0 100644
> --- a/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt
> @@ -17,6 +17,7 @@ Required clocking property, have to be one of:
>
> Optional properties:
> - timeout-sec : Contains the watchdog timeout in seconds
> +- nuvoton,restart-priority : Contains the card restart priority.
>
> Example:
>
> @@ -25,4 +26,5 @@ timer@f000801c {
> interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
> reg = <0xf000801c 0x4>;
> clocks = <&clk NPCM7XX_CLK_TIMER>;
> + nuvoton,restart-priority = <155>;
> };
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 4/4] watchdog: npcm: sets card ext1 and ext2 bootstatus during probe
2020-03-01 9:40 ` [PATCH v1 4/4] watchdog: npcm: sets card ext1 and ext2 bootstatus during probe Tomer Maimon
@ 2020-03-01 10:48 ` Guenter Roeck
2020-03-01 16:08 ` Tomer Maimon
0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2020-03-01 10:48 UTC (permalink / raw)
To: Tomer Maimon, wim, robh+dt, mark.rutland, joel, avifishman70,
tali.perry1, yuenn, benjaminfair
Cc: linux-watchdog, devicetree, linux-kernel, openbmc
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;
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/4] dt-binding: watchdog: add restart priority documentation
2020-03-01 10:05 ` Guenter Roeck
@ 2020-03-01 15:36 ` Tomer Maimon
2020-03-01 15:46 ` Guenter Roeck
0 siblings, 1 reply; 13+ messages in thread
From: Tomer Maimon @ 2020-03-01 15:36 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, Joel Stanley,
Avi Fishman, Tali Perry, Nancy Yuen, Benjamin Fair,
linux-watchdog, devicetree, Linux Kernel Mailing List,
OpenBMC Maillist
[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]
On Sun, 1 Mar 2020 at 12:06, Guenter Roeck <linux@roeck-us.net> wrote:
> On 3/1/20 1:40 AM, Tomer Maimon wrote:
> > Add device tree restart priority documentation.
> >
>
> I think this warrants an explanation _why_ this is needed.
> What is the use case ? Not just theory, please.
>
In the NPCM750 there is two initiated restarts:
- Software reset
- WD reset
the Software restart found at NPCM reset driver
https://github.com/torvalds/linux/blob/master/drivers/reset/reset-npcm.c
In NPCM WD driver the restart is configure as well, I will like to add the
priority so the user will have maximum flexibility if he using both restarts
Thanks,
Tomer
> Guenter
>
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > ---
> > Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git
> a/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt
> b/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt
> > index 6d593003c933..0a0f86a25eb0 100644
> > --- a/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt
> > +++ b/Documentation/devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt
> > @@ -17,6 +17,7 @@ Required clocking property, have to be one of:
> >
> > Optional properties:
> > - timeout-sec : Contains the watchdog timeout in seconds
> > +- nuvoton,restart-priority : Contains the card restart priority.
> >
> > Example:
> >
> > @@ -25,4 +26,5 @@ timer@f000801c {
> > interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
> > reg = <0xf000801c 0x4>;
> > clocks = <&clk NPCM7XX_CLK_TIMER>;
> > + nuvoton,restart-priority = <155>;
> > };
> >
>
>
[-- Attachment #2: Type: text/html, Size: 2774 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/4] dt-binding: watchdog: add restart priority documentation
2020-03-01 15:36 ` Tomer Maimon
@ 2020-03-01 15:46 ` Guenter Roeck
2020-03-01 16:19 ` Tomer Maimon
0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2020-03-01 15:46 UTC (permalink / raw)
To: Tomer Maimon
Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, Joel Stanley,
Avi Fishman, Tali Perry, Nancy Yuen, Benjamin Fair,
linux-watchdog, devicetree, Linux Kernel Mailing List,
OpenBMC Maillist
On 3/1/20 7:36 AM, Tomer Maimon wrote:
>
>
> On Sun, 1 Mar 2020 at 12:06, Guenter Roeck <linux@roeck-us.net <mailto:linux@roeck-us.net>> wrote:
>
> On 3/1/20 1:40 AM, Tomer Maimon wrote:
> > Add device tree restart priority documentation.
> >
>
> I think this warrants an explanation _why_ this is needed.
> What is the use case ? Not just theory, please.
>
>
> In the NPCM750 there is two initiated restarts:
>
> * Software reset
> * WD reset
>
> the Software restart found at NPCM reset driver
> https://github.com/torvalds/linux/blob/master/drivers/reset/reset-npcm.c
>
> In NPCM WD driver the restart is configure as well, I will like to add the priority so the user will have maximum flexibility if he using both restarts
>
This is not the intended use case for restart priority. It is not
intended to be user configurable. The idea is that the more thorough
restart gets higher priority. This is implied by the restart method,
not by user preferences.
Also, the idea behind supporting multiple means to reset the system
is to be able to support multiple means to restart, some of which
may not always be available. In that situation, the priority means,
and is supposed to mean, "pick the best restart method available".
Again, that is determined by system design, and not supposed to
be configurable by the user.
On top of that, a watchdog driver based reset is almost always
a reset of last resort, to be chosen only if nothing else is available
in a given system. The existence of the reset driver confirms that
this is not different for this driver/chip.
Guenter
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 4/4] watchdog: npcm: sets card ext1 and ext2 bootstatus during probe
2020-03-01 10:48 ` Guenter Roeck
@ 2020-03-01 16:08 ` Tomer Maimon
2020-03-01 16:10 ` Guenter Roeck
0 siblings, 1 reply; 13+ messages in thread
From: Tomer Maimon @ 2020-03-01 16:08 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, Joel Stanley,
Avi Fishman, Tali Perry, Nancy Yuen, Benjamin Fair,
linux-watchdog, devicetree, Linux Kernel Mailing List,
OpenBMC Maillist
[-- Attachment #1: Type: text/plain, Size: 10036 bytes --]
Sorry Guenter probebly I didnt explain it well.
On Sun, 1 Mar 2020 at 12:48, Guenter Roeck <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.
To have maximum flexibility (with three boot status bits), In the device
tree the user can configure each reset group type according the one the
board is using.
Hope I explian it better :-)
> 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 NPCM750 is power on the NPCM7XX_RESSR_OFFSET register is cleaned and the
reset copied to the NPCM7XX_INTCR2_OFFSET.
I will add a note to the code
> > +
> > + 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;
> >
> >
>
>
Thanks a lot,
Tomer
[-- Attachment #2: Type: text/html, Size: 15303 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 4/4] watchdog: npcm: sets card ext1 and ext2 bootstatus during probe
2020-03-01 16:08 ` Tomer Maimon
@ 2020-03-01 16:10 ` Guenter Roeck
2020-03-01 17:03 ` Tomer Maimon
0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2020-03-01 16:10 UTC (permalink / raw)
To: Tomer Maimon
Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, Joel Stanley,
Avi Fishman, Tali Perry, Nancy Yuen, Benjamin Fair,
linux-watchdog, devicetree, Linux Kernel Mailing List,
OpenBMC Maillist
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/4] dt-binding: watchdog: add restart priority documentation
2020-03-01 15:46 ` Guenter Roeck
@ 2020-03-01 16:19 ` Tomer Maimon
0 siblings, 0 replies; 13+ messages in thread
From: Tomer Maimon @ 2020-03-01 16:19 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, Joel Stanley,
Avi Fishman, Tali Perry, Nancy Yuen, Benjamin Fair,
linux-watchdog, devicetree, Linux Kernel Mailing List,
OpenBMC Maillist
[-- Attachment #1: Type: text/plain, Size: 1915 bytes --]
Thank you for the clarification.
I will remove the priority patch, and send a new patch set after receiving
comments for the boot status patch.
Tomer
On Sun, 1 Mar 2020 at 17:46, Guenter Roeck <linux@roeck-us.net> wrote:
> On 3/1/20 7:36 AM, Tomer Maimon wrote:
> >
> >
> > On Sun, 1 Mar 2020 at 12:06, Guenter Roeck <linux@roeck-us.net <mailto:
> linux@roeck-us.net>> wrote:
> >
> > On 3/1/20 1:40 AM, Tomer Maimon wrote:
> > > Add device tree restart priority documentation.
> > >
> >
> > I think this warrants an explanation _why_ this is needed.
> > What is the use case ? Not just theory, please.
> >
> >
> > In the NPCM750 there is two initiated restarts:
> >
> > * Software reset
> > * WD reset
> >
> > the Software restart found at NPCM reset driver
> > https://github.com/torvalds/linux/blob/master/drivers/reset/reset-npcm.c
> >
> > In NPCM WD driver the restart is configure as well, I will like to add
> the priority so the user will have maximum flexibility if he using both
> restarts
> >
>
> This is not the intended use case for restart priority. It is not
> intended to be user configurable. The idea is that the more thorough
> restart gets higher priority. This is implied by the restart method,
> not by user preferences.
>
> Also, the idea behind supporting multiple means to reset the system
> is to be able to support multiple means to restart, some of which
> may not always be available. In that situation, the priority means,
> and is supposed to mean, "pick the best restart method available".
> Again, that is determined by system design, and not supposed to
> be configurable by the user.
>
> On top of that, a watchdog driver based reset is almost always
> a reset of last resort, to be chosen only if nothing else is available
> in a given system. The existence of the reset driver confirms that
> this is not different for this driver/chip.
>
> Guenter
>
[-- Attachment #2: Type: text/html, Size: 2726 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 4/4] watchdog: npcm: sets card ext1 and ext2 bootstatus during probe
2020-03-01 16:10 ` Guenter Roeck
@ 2020-03-01 17:03 ` Tomer Maimon
0 siblings, 0 replies; 13+ messages in thread
From: Tomer Maimon @ 2020-03-01 17:03 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, Joel Stanley,
Avi Fishman, Tali Perry, Nancy Yuen, Benjamin Fair,
linux-watchdog, devicetree, Linux Kernel Mailing List,
OpenBMC Maillist
[-- Attachment #1: Type: text/plain, Size: 2598 bytes --]
I deeply sorry Guenter,
Each of the bit boot status have the maximum flexibility to represent the
entire resets (I will make sure to modify the commit message as in the
device tree document)
So if the user using WD0 to restart the board and WD1 for a board health
monitor it can be represented at EXT1 and EXT2.
If I will use a group for each reset type in a case above the user will
need to read the RESSR register (prefer not to do it) to know which reset
occur.
Sorry again.
Tomer
On Sun, 1 Mar 2020 at 18:10, Guenter Roeck <linux@roeck-us.net> wrote:
> 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
>
[-- Attachment #2: Type: text/html, Size: 3571 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-03-01 16:53 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-01 9:40 [PATCH v1 0/4] watchdog: npcm: support new capabilities Tomer Maimon
2020-03-01 9:40 ` [PATCH v1 1/4] dt-binding: watchdog: add restart priority documentation Tomer Maimon
2020-03-01 10:05 ` Guenter Roeck
2020-03-01 15:36 ` Tomer Maimon
2020-03-01 15:46 ` Guenter Roeck
2020-03-01 16:19 ` Tomer Maimon
2020-03-01 9:40 ` [PATCH v1 2/4] watchdog: npcm: add restart priority support Tomer Maimon
2020-03-01 9:40 ` [PATCH v1 3/4] dt-binding: watchdog: add bootstatus reset type documentation Tomer Maimon
2020-03-01 9:40 ` [PATCH v1 4/4] watchdog: npcm: sets card ext1 and ext2 bootstatus during probe Tomer Maimon
2020-03-01 10:48 ` Guenter Roeck
2020-03-01 16:08 ` Tomer Maimon
2020-03-01 16:10 ` Guenter Roeck
2020-03-01 17:03 ` Tomer Maimon
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.