* [PATCH v2 0/2] Add ASPEED watchdog device tree properties @ 2017-06-27 21:17 Christopher Bostic 2017-06-27 21:17 ` [PATCH v2 1/2] drivers/watchdog: Add optional ASPEED " Christopher Bostic 2017-06-27 21:17 ` [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config Christopher Bostic 0 siblings, 2 replies; 17+ messages in thread From: Christopher Bostic @ 2017-06-27 21:17 UTC (permalink / raw) To: wim, linux, robh+dt, mark.rutland, joel, linux-watchdog, devicetree Cc: Christopher Bostic, linux-kernel Document device tree optional properties for ASPEED watchdog. Reference properties in ASPEED watchdog driver and configure accordingly. Christopher Bostic (2): drivers/watchdog: Add optional ASPEED device tree properties drivers/watchdog: ASPEED reference dev tree properties for config .../devicetree/bindings/watchdog/aspeed-wdt.txt | 25 ++++++++++++++++++++++ drivers/watchdog/aspeed_wdt.c | 13 +++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-) -- 1.8.2.2 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/2] drivers/watchdog: Add optional ASPEED device tree properties 2017-06-27 21:17 [PATCH v2 0/2] Add ASPEED watchdog device tree properties Christopher Bostic @ 2017-06-27 21:17 ` Christopher Bostic 2017-06-27 21:32 ` Guenter Roeck 2017-06-27 21:17 ` [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config Christopher Bostic 1 sibling, 1 reply; 17+ messages in thread From: Christopher Bostic @ 2017-06-27 21:17 UTC (permalink / raw) To: wim, linux, robh+dt, mark.rutland, joel, linux-watchdog, devicetree Cc: Christopher Bostic, linux-kernel Describe device tree optional properties: * aspeed,arm-reet - ARM CPU reset on signal * aspeed,soc-reset - SOC reset on signal * aspeed,sys-reset - System reset on signal Disabling system reset may be required in situations where one of the other watchdog engines in the system is responsible for this. * aspeed,interrupt - Interrupt CPU on signal * aspeed,external-signal - Generate external signal (WDT1 and WDT2 only) * aspeed,alt-boot - Boot from alternate block on signal Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> --- v2 - Add 'aspeed,' prefix to all optional properties - Add arm-reset, soc-reset, interrupt, alt-boot properties --- .../devicetree/bindings/watchdog/aspeed-wdt.txt | 25 ++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt index c5e74d7..555b8b4 100644 --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt @@ -8,9 +8,34 @@ Required properties: - reg: physical base address of the controller and length of memory mapped region +Optional properties: + Signal behavior - Whenever a timeout occurs, the watchdog can be programmed + to generate 6 types of signals: + + - aspeed,arm-reset: If property is present then reset ARM CPU only. + + - aspeed,soc-reset: If property is present then reset SOC. + + - aspeed,sys-reset: If property is present then reset the entire chip. + In cases where one of the other watchdog engines + in the system is responsible for system reset it + may be required to not specify this property. + + - aspeed,interrupt: If property is present then interrupt CPU. + + - aspeed,external-signal: If property is present then signal is sent to + external reset counter (only WDT1 and WDT2). + - aspeed,alt-boot: If property is present then boot from alternate block. + Example: wdt1: watchdog@1e785000 { compatible = "aspeed,ast2400-wdt"; reg = <0x1e785000 0x1c>; + aspeed,arm-reset; + aspeed,soc-reset; + aspeed,sys-reset; + aspeed,interrupt; + aspeed,external-signal; + aspeed,alt-boot; }; -- 1.8.2.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] drivers/watchdog: Add optional ASPEED device tree properties 2017-06-27 21:17 ` [PATCH v2 1/2] drivers/watchdog: Add optional ASPEED " Christopher Bostic @ 2017-06-27 21:32 ` Guenter Roeck 2017-06-27 21:42 ` Christopher Bostic 0 siblings, 1 reply; 17+ messages in thread From: Guenter Roeck @ 2017-06-27 21:32 UTC (permalink / raw) To: Christopher Bostic Cc: wim, robh+dt, mark.rutland, joel, linux-watchdog, devicetree, linux-kernel On Tue, Jun 27, 2017 at 04:17:33PM -0500, Christopher Bostic wrote: > Describe device tree optional properties: > > * aspeed,arm-reet - ARM CPU reset on signal > * aspeed,soc-reset - SOC reset on signal > * aspeed,sys-reset - System reset on signal > Disabling system reset may be required in situations where > one of the other watchdog engines in the system is responsible > for this. > * aspeed,interrupt - Interrupt CPU on signal > * aspeed,external-signal - Generate external signal (WDT1 and WDT2 only) > * aspeed,alt-boot - Boot from alternate block on signal > > Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> > --- > v2 - Add 'aspeed,' prefix to all optional properties > - Add arm-reset, soc-reset, interrupt, alt-boot properties > --- > .../devicetree/bindings/watchdog/aspeed-wdt.txt | 25 ++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt > index c5e74d7..555b8b4 100644 > --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt > +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt > @@ -8,9 +8,34 @@ Required properties: > - reg: physical base address of the controller and length of memory mapped > region > > +Optional properties: > + Signal behavior - Whenever a timeout occurs, the watchdog can be programmed > + to generate 6 types of signals: > + > + - aspeed,arm-reset: If property is present then reset ARM CPU only. > + > + - aspeed,soc-reset: If property is present then reset SOC. > + > + - aspeed,sys-reset: If property is present then reset the entire chip. > + In cases where one of the other watchdog engines > + in the system is responsible for system reset it > + may be required to not specify this property. > + > + - aspeed,interrupt: If property is present then interrupt CPU. > + > + - aspeed,external-signal: If property is present then signal is sent to > + external reset counter (only WDT1 and WDT2). > + - aspeed,alt-boot: If property is present then boot from alternate block. > + > Example: > > wdt1: watchdog@1e785000 { > compatible = "aspeed,ast2400-wdt"; > reg = <0x1e785000 0x1c>; > + aspeed,arm-reset; > + aspeed,soc-reset; > + aspeed,sys-reset; > + aspeed,interrupt; > + aspeed,external-signal; > + aspeed,alt-boot; Is that a bit mask or a value ? I would have thought that, for example, a complete system reset would include the SoC reset, and a SoC reset would include the ARM reset. Generating an interrupt while at the same time resetting the system (or part of it) doesn't seem to make much sense either. Guenter ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] drivers/watchdog: Add optional ASPEED device tree properties 2017-06-27 21:32 ` Guenter Roeck @ 2017-06-27 21:42 ` Christopher Bostic 2017-06-27 22:07 ` Guenter Roeck 0 siblings, 1 reply; 17+ messages in thread From: Christopher Bostic @ 2017-06-27 21:42 UTC (permalink / raw) To: Guenter Roeck Cc: wim, robh+dt, mark.rutland, joel, linux-watchdog, devicetree, linux-kernel On 6/27/17 4:32 PM, Guenter Roeck wrote: > On Tue, Jun 27, 2017 at 04:17:33PM -0500, Christopher Bostic wrote: >> Describe device tree optional properties: >> >> * aspeed,arm-reet - ARM CPU reset on signal >> * aspeed,soc-reset - SOC reset on signal >> * aspeed,sys-reset - System reset on signal >> Disabling system reset may be required in situations where >> one of the other watchdog engines in the system is responsible >> for this. >> * aspeed,interrupt - Interrupt CPU on signal >> * aspeed,external-signal - Generate external signal (WDT1 and WDT2 only) >> * aspeed,alt-boot - Boot from alternate block on signal >> >> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> >> --- >> v2 - Add 'aspeed,' prefix to all optional properties >> - Add arm-reset, soc-reset, interrupt, alt-boot properties >> --- >> .../devicetree/bindings/watchdog/aspeed-wdt.txt | 25 ++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt >> index c5e74d7..555b8b4 100644 >> --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt >> +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt >> @@ -8,9 +8,34 @@ Required properties: >> - reg: physical base address of the controller and length of memory mapped >> region >> >> +Optional properties: >> + Signal behavior - Whenever a timeout occurs, the watchdog can be programmed >> + to generate 6 types of signals: >> + >> + - aspeed,arm-reset: If property is present then reset ARM CPU only. >> + >> + - aspeed,soc-reset: If property is present then reset SOC. >> + >> + - aspeed,sys-reset: If property is present then reset the entire chip. >> + In cases where one of the other watchdog engines >> + in the system is responsible for system reset it >> + may be required to not specify this property. >> + >> + - aspeed,interrupt: If property is present then interrupt CPU. >> + >> + - aspeed,external-signal: If property is present then signal is sent to >> + external reset counter (only WDT1 and WDT2). >> + - aspeed,alt-boot: If property is present then boot from alternate block. >> + >> Example: >> >> wdt1: watchdog@1e785000 { >> compatible = "aspeed,ast2400-wdt"; >> reg = <0x1e785000 0x1c>; >> + aspeed,arm-reset; >> + aspeed,soc-reset; >> + aspeed,sys-reset; >> + aspeed,interrupt; >> + aspeed,external-signal; >> + aspeed,alt-boot; > Is that a bit mask or a value ? I would have thought that, > for example, a complete system reset would include the SoC reset, > and a SoC reset would include the ARM reset. Generating an > interrupt while at the same time resetting the system (or > part of it) doesn't seem to make much sense either. No these aren't bitmasks. The example was intended to indicate what could be used. In practice only a subset of each of these properties would make any sense. How would you suggest the example be formatted to convey that? Multiple examples I suppose. Thanks, Chris > Guenter > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] drivers/watchdog: Add optional ASPEED device tree properties 2017-06-27 21:42 ` Christopher Bostic @ 2017-06-27 22:07 ` Guenter Roeck 2017-06-28 14:55 ` Christopher Bostic 0 siblings, 1 reply; 17+ messages in thread From: Guenter Roeck @ 2017-06-27 22:07 UTC (permalink / raw) To: Christopher Bostic Cc: wim, robh+dt, mark.rutland, joel, linux-watchdog, devicetree, linux-kernel On Tue, Jun 27, 2017 at 04:42:24PM -0500, Christopher Bostic wrote: > > > On 6/27/17 4:32 PM, Guenter Roeck wrote: > >On Tue, Jun 27, 2017 at 04:17:33PM -0500, Christopher Bostic wrote: > >>Describe device tree optional properties: > >> > >> * aspeed,arm-reet - ARM CPU reset on signal > >> * aspeed,soc-reset - SOC reset on signal > >> * aspeed,sys-reset - System reset on signal > >> Disabling system reset may be required in situations where > >> one of the other watchdog engines in the system is responsible > >> for this. > >> * aspeed,interrupt - Interrupt CPU on signal > >> * aspeed,external-signal - Generate external signal (WDT1 and WDT2 only) > >> * aspeed,alt-boot - Boot from alternate block on signal > >> > >>Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> > >>--- > >>v2 - Add 'aspeed,' prefix to all optional properties > >> - Add arm-reset, soc-reset, interrupt, alt-boot properties > >>--- > >> .../devicetree/bindings/watchdog/aspeed-wdt.txt | 25 ++++++++++++++++++++++ > >> 1 file changed, 25 insertions(+) > >> > >>diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt > >>index c5e74d7..555b8b4 100644 > >>--- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt > >>+++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt > >>@@ -8,9 +8,34 @@ Required properties: > >> - reg: physical base address of the controller and length of memory mapped > >> region > >>+Optional properties: > >>+ Signal behavior - Whenever a timeout occurs, the watchdog can be programmed > >>+ to generate 6 types of signals: > >>+ > >>+ - aspeed,arm-reset: If property is present then reset ARM CPU only. > >>+ > >>+ - aspeed,soc-reset: If property is present then reset SOC. > >>+ > >>+ - aspeed,sys-reset: If property is present then reset the entire chip. > >>+ In cases where one of the other watchdog engines > >>+ in the system is responsible for system reset it > >>+ may be required to not specify this property. > >>+ > >>+ - aspeed,interrupt: If property is present then interrupt CPU. > >>+ > >>+ - aspeed,external-signal: If property is present then signal is sent to > >>+ external reset counter (only WDT1 and WDT2). > >>+ - aspeed,alt-boot: If property is present then boot from alternate block. > >>+ > >> Example: > >> wdt1: watchdog@1e785000 { > >> compatible = "aspeed,ast2400-wdt"; > >> reg = <0x1e785000 0x1c>; > >>+ aspeed,arm-reset; > >>+ aspeed,soc-reset; > >>+ aspeed,sys-reset; > >>+ aspeed,interrupt; > >>+ aspeed,external-signal; > >>+ aspeed,alt-boot; > >Is that a bit mask or a value ? I would have thought that, > >for example, a complete system reset would include the SoC reset, > >and a SoC reset would include the ARM reset. Generating an > >interrupt while at the same time resetting the system (or > >part of it) doesn't seem to make much sense either. > > No these aren't bitmasks. The example was intended to indicate what could > be used. > In practice only a subset of each of these properties would make any sense. > How > would you suggest the example be formatted to convey that? Multiple examples > I suppose. > I would just pick one (which is perfectly fine). After all, it is an example. Reminds me: Is there a default (eg the chip's default configuration) ? In other words, what is expected to happen if none of the properties is specified ? Thanks, Guenter ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] drivers/watchdog: Add optional ASPEED device tree properties 2017-06-27 22:07 ` Guenter Roeck @ 2017-06-28 14:55 ` Christopher Bostic 2017-06-28 15:06 ` Guenter Roeck 0 siblings, 1 reply; 17+ messages in thread From: Christopher Bostic @ 2017-06-28 14:55 UTC (permalink / raw) To: Guenter Roeck Cc: wim, robh+dt, mark.rutland, joel, linux-watchdog, devicetree, linux-kernel On 6/27/17 5:07 PM, Guenter Roeck wrote: > On Tue, Jun 27, 2017 at 04:42:24PM -0500, Christopher Bostic wrote: >> >> On 6/27/17 4:32 PM, Guenter Roeck wrote: >>> On Tue, Jun 27, 2017 at 04:17:33PM -0500, Christopher Bostic wrote: >>>> Describe device tree optional properties: >>>> >>>> * aspeed,arm-reet - ARM CPU reset on signal >>>> * aspeed,soc-reset - SOC reset on signal >>>> * aspeed,sys-reset - System reset on signal >>>> Disabling system reset may be required in situations where >>>> one of the other watchdog engines in the system is responsible >>>> for this. >>>> * aspeed,interrupt - Interrupt CPU on signal >>>> * aspeed,external-signal - Generate external signal (WDT1 and WDT2 only) >>>> * aspeed,alt-boot - Boot from alternate block on signal >>>> >>>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> >>>> --- >>>> v2 - Add 'aspeed,' prefix to all optional properties >>>> - Add arm-reset, soc-reset, interrupt, alt-boot properties >>>> --- >>>> .../devicetree/bindings/watchdog/aspeed-wdt.txt | 25 ++++++++++++++++++++++ >>>> 1 file changed, 25 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt >>>> index c5e74d7..555b8b4 100644 >>>> --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt >>>> +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt >>>> @@ -8,9 +8,34 @@ Required properties: >>>> - reg: physical base address of the controller and length of memory mapped >>>> region >>>> +Optional properties: >>>> + Signal behavior - Whenever a timeout occurs, the watchdog can be programmed >>>> + to generate 6 types of signals: >>>> + >>>> + - aspeed,arm-reset: If property is present then reset ARM CPU only. >>>> + >>>> + - aspeed,soc-reset: If property is present then reset SOC. >>>> + >>>> + - aspeed,sys-reset: If property is present then reset the entire chip. >>>> + In cases where one of the other watchdog engines >>>> + in the system is responsible for system reset it >>>> + may be required to not specify this property. >>>> + >>>> + - aspeed,interrupt: If property is present then interrupt CPU. >>>> + >>>> + - aspeed,external-signal: If property is present then signal is sent to >>>> + external reset counter (only WDT1 and WDT2). >>>> + - aspeed,alt-boot: If property is present then boot from alternate block. >>>> + >>>> Example: >>>> wdt1: watchdog@1e785000 { >>>> compatible = "aspeed,ast2400-wdt"; >>>> reg = <0x1e785000 0x1c>; >>>> + aspeed,arm-reset; >>>> + aspeed,soc-reset; >>>> + aspeed,sys-reset; >>>> + aspeed,interrupt; >>>> + aspeed,external-signal; >>>> + aspeed,alt-boot; >>> Is that a bit mask or a value ? I would have thought that, >>> for example, a complete system reset would include the SoC reset, >>> and a SoC reset would include the ARM reset. Generating an >>> interrupt while at the same time resetting the system (or >>> part of it) doesn't seem to make much sense either. >> No these aren't bitmasks. The example was intended to indicate what could >> be used. >> In practice only a subset of each of these properties would make any sense. >> How >> would you suggest the example be formatted to convey that? Multiple examples >> I suppose. >> > I would just pick one (which is perfectly fine). After all, it is an example. > > Reminds me: Is there a default (eg the chip's default configuration) ? > In other words, what is expected to happen if none of the properties > is specified ? Default was to enable SOC reset (parameter aspeed,soc-reset) and system reset (aspeed,sys-reset). Based on your comments in patch v2 2/2 it would be necessary for backwards compatibility to preserve that default behavior. So it seems the optional parameters should be inverted for these: no-soc-reset, no-sys-reset. Thanks, Chris > Thanks, > Guenter > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] drivers/watchdog: Add optional ASPEED device tree properties 2017-06-28 14:55 ` Christopher Bostic @ 2017-06-28 15:06 ` Guenter Roeck 0 siblings, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2017-06-28 15:06 UTC (permalink / raw) To: Christopher Bostic Cc: wim, robh+dt, mark.rutland, joel, linux-watchdog, devicetree, linux-kernel On Wed, Jun 28, 2017 at 09:55:41AM -0500, Christopher Bostic wrote: > > > On 6/27/17 5:07 PM, Guenter Roeck wrote: > >On Tue, Jun 27, 2017 at 04:42:24PM -0500, Christopher Bostic wrote: > >> > >>On 6/27/17 4:32 PM, Guenter Roeck wrote: > >>>On Tue, Jun 27, 2017 at 04:17:33PM -0500, Christopher Bostic wrote: > >>>>Describe device tree optional properties: > >>>> > >>>> * aspeed,arm-reet - ARM CPU reset on signal > >>>> * aspeed,soc-reset - SOC reset on signal > >>>> * aspeed,sys-reset - System reset on signal > >>>> Disabling system reset may be required in situations where > >>>> one of the other watchdog engines in the system is responsible > >>>> for this. > >>>> * aspeed,interrupt - Interrupt CPU on signal > >>>> * aspeed,external-signal - Generate external signal (WDT1 and WDT2 only) > >>>> * aspeed,alt-boot - Boot from alternate block on signal > >>>> > >>>>Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> > >>>>--- > >>>>v2 - Add 'aspeed,' prefix to all optional properties > >>>> - Add arm-reset, soc-reset, interrupt, alt-boot properties > >>>>--- > >>>> .../devicetree/bindings/watchdog/aspeed-wdt.txt | 25 ++++++++++++++++++++++ > >>>> 1 file changed, 25 insertions(+) > >>>> > >>>>diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt > >>>>index c5e74d7..555b8b4 100644 > >>>>--- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt > >>>>+++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt > >>>>@@ -8,9 +8,34 @@ Required properties: > >>>> - reg: physical base address of the controller and length of memory mapped > >>>> region > >>>>+Optional properties: > >>>>+ Signal behavior - Whenever a timeout occurs, the watchdog can be programmed > >>>>+ to generate 6 types of signals: > >>>>+ > >>>>+ - aspeed,arm-reset: If property is present then reset ARM CPU only. > >>>>+ > >>>>+ - aspeed,soc-reset: If property is present then reset SOC. > >>>>+ > >>>>+ - aspeed,sys-reset: If property is present then reset the entire chip. > >>>>+ In cases where one of the other watchdog engines > >>>>+ in the system is responsible for system reset it > >>>>+ may be required to not specify this property. > >>>>+ > >>>>+ - aspeed,interrupt: If property is present then interrupt CPU. > >>>>+ > >>>>+ - aspeed,external-signal: If property is present then signal is sent to > >>>>+ external reset counter (only WDT1 and WDT2). > >>>>+ - aspeed,alt-boot: If property is present then boot from alternate block. > >>>>+ > >>>> Example: > >>>> wdt1: watchdog@1e785000 { > >>>> compatible = "aspeed,ast2400-wdt"; > >>>> reg = <0x1e785000 0x1c>; > >>>>+ aspeed,arm-reset; > >>>>+ aspeed,soc-reset; > >>>>+ aspeed,sys-reset; > >>>>+ aspeed,interrupt; > >>>>+ aspeed,external-signal; > >>>>+ aspeed,alt-boot; > >>>Is that a bit mask or a value ? I would have thought that, > >>>for example, a complete system reset would include the SoC reset, > >>>and a SoC reset would include the ARM reset. Generating an > >>>interrupt while at the same time resetting the system (or > >>>part of it) doesn't seem to make much sense either. > >>No these aren't bitmasks. The example was intended to indicate what could > >>be used. > >>In practice only a subset of each of these properties would make any sense. > >>How > >>would you suggest the example be formatted to convey that? Multiple examples > >>I suppose. > >> > >I would just pick one (which is perfectly fine). After all, it is an example. > > > >Reminds me: Is there a default (eg the chip's default configuration) ? > >In other words, what is expected to happen if none of the properties > >is specified ? > Default was to enable SOC reset (parameter aspeed,soc-reset) and system > reset (aspeed,sys-reset). Based on your comments in patch v2 2/2 it would > be necessary for backwards compatibility to preserve that default behavior. > So it seems the optional parameters should be inverted for these: > no-soc-reset, no-sys-reset. > I don't really understand what you are trying to say, sorry. no-sys-reset would enable everything but sys-reset, and one would have to specify a lot of no-xxx to select the one that is needed ? And not specifying anything would select everything ? That sounds just weird. I still don't see the problem with having a default. Guenter ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config 2017-06-27 21:17 [PATCH v2 0/2] Add ASPEED watchdog device tree properties Christopher Bostic 2017-06-27 21:17 ` [PATCH v2 1/2] drivers/watchdog: Add optional ASPEED " Christopher Bostic @ 2017-06-27 21:17 ` Christopher Bostic 2017-06-28 11:31 ` Guenter Roeck 1 sibling, 1 reply; 17+ messages in thread From: Christopher Bostic @ 2017-06-27 21:17 UTC (permalink / raw) To: wim, linux, robh+dt, mark.rutland, joel, linux-watchdog, devicetree Cc: Christopher Bostic, linux-kernel Reference the system device tree when configuring the watchdog engines. Set external signal mode on timeout if specified. Set system reset on timeout if specified. Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> --- v2 - Change of_get_property() to of_property_read_bool() - Remove redundant check for NULL struct device_node pointer - Optional property names now start with prefix 'aspeed,' --- drivers/watchdog/aspeed_wdt.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c index 1c65258..71ce5f5 100644 --- a/drivers/watchdog/aspeed_wdt.c +++ b/drivers/watchdog/aspeed_wdt.c @@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev) { struct aspeed_wdt *wdt; struct resource *res; + struct device_node *np; int ret; wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); @@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device *pdev) * the SOC and not the full chip */ wdt->ctrl = WDT_CTRL_RESET_MODE_SOC | - WDT_CTRL_1MHZ_CLK | - WDT_CTRL_RESET_SYSTEM; + WDT_CTRL_1MHZ_CLK; + + np = pdev->dev.of_node; + if (of_property_read_bool(np, "aspeed,sys-reset")) + wdt->ctrl |= WDT_CTRL_RESET_SYSTEM; + + if (of_property_read_bool(np, "aspeed,external-signal")) + wdt->ctrl |= WDT_CTRL_WDT_EXT; + + writel(wdt->ctrl, wdt->base + WDT_CTRL); if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE) { aspeed_wdt_start(&wdt->wdd); -- 1.8.2.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config 2017-06-27 21:17 ` [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config Christopher Bostic @ 2017-06-28 11:31 ` Guenter Roeck 2017-06-28 14:29 ` Christopher Bostic 0 siblings, 1 reply; 17+ messages in thread From: Guenter Roeck @ 2017-06-28 11:31 UTC (permalink / raw) To: Christopher Bostic, wim, robh+dt, mark.rutland, joel, linux-watchdog, devicetree Cc: linux-kernel On 06/27/2017 02:17 PM, Christopher Bostic wrote: > Reference the system device tree when configuring the watchdog > engines. Set external signal mode on timeout if specified. > Set system reset on timeout if specified. > > Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> > --- > v2 - Change of_get_property() to of_property_read_bool() > - Remove redundant check for NULL struct device_node pointer > - Optional property names now start with prefix 'aspeed,' > --- > drivers/watchdog/aspeed_wdt.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c > index 1c65258..71ce5f5 100644 > --- a/drivers/watchdog/aspeed_wdt.c > +++ b/drivers/watchdog/aspeed_wdt.c > @@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev) > { > struct aspeed_wdt *wdt; > struct resource *res; > + struct device_node *np; > int ret; > > wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > @@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device *pdev) > * the SOC and not the full chip > */ > wdt->ctrl = WDT_CTRL_RESET_MODE_SOC | > - WDT_CTRL_1MHZ_CLK | > - WDT_CTRL_RESET_SYSTEM; > + WDT_CTRL_1MHZ_CLK; > + > + np = pdev->dev.of_node; > + if (of_property_read_bool(np, "aspeed,sys-reset")) > + wdt->ctrl |= WDT_CTRL_RESET_SYSTEM; > + For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM if no optional property is provided. > + if (of_property_read_bool(np, "aspeed,external-signal")) > + wdt->ctrl |= WDT_CTRL_WDT_EXT; > + > + writel(wdt->ctrl, wdt->base + WDT_CTRL); > > if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE) { > aspeed_wdt_start(&wdt->wdd); > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config 2017-06-28 11:31 ` Guenter Roeck @ 2017-06-28 14:29 ` Christopher Bostic 2017-06-28 14:54 ` Guenter Roeck 0 siblings, 1 reply; 17+ messages in thread From: Christopher Bostic @ 2017-06-28 14:29 UTC (permalink / raw) To: Guenter Roeck, wim, robh+dt, mark.rutland, joel, linux-watchdog, devicetree Cc: linux-kernel On 6/28/17 6:31 AM, Guenter Roeck wrote: > On 06/27/2017 02:17 PM, Christopher Bostic wrote: >> Reference the system device tree when configuring the watchdog >> engines. Set external signal mode on timeout if specified. >> Set system reset on timeout if specified. >> >> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> >> --- >> v2 - Change of_get_property() to of_property_read_bool() >> - Remove redundant check for NULL struct device_node pointer >> - Optional property names now start with prefix 'aspeed,' >> --- >> drivers/watchdog/aspeed_wdt.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/watchdog/aspeed_wdt.c >> b/drivers/watchdog/aspeed_wdt.c >> index 1c65258..71ce5f5 100644 >> --- a/drivers/watchdog/aspeed_wdt.c >> +++ b/drivers/watchdog/aspeed_wdt.c >> @@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct >> platform_device *pdev) >> { >> struct aspeed_wdt *wdt; >> struct resource *res; >> + struct device_node *np; >> int ret; >> wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); >> @@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct >> platform_device *pdev) >> * the SOC and not the full chip >> */ >> wdt->ctrl = WDT_CTRL_RESET_MODE_SOC | >> - WDT_CTRL_1MHZ_CLK | >> - WDT_CTRL_RESET_SYSTEM; >> + WDT_CTRL_1MHZ_CLK; >> + >> + np = pdev->dev.of_node; >> + if (of_property_read_bool(np, "aspeed,sys-reset")) >> + wdt->ctrl |= WDT_CTRL_RESET_SYSTEM; >> + > > For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM > if no optional property is provided. > I had the logic inverted for this property in a previous patch. The property was 'no-system-reset' so that when not present the default was to set WDT_CTRL_RESET_SYSTEM. As it is in this patch, the only way to indicate that no system reset is to be done is to not specify the property. No system reset is desired under circumstances when another wdt engine is to be responsible for this. Given the issue with backward compatibility that's not a solution. Given this, would creating a property 'no-system-reset' be acceptable? Thanks, Chris >> + if (of_property_read_bool(np, "aspeed,external-signal")) >> + wdt->ctrl |= WDT_CTRL_WDT_EXT; >> + >> + writel(wdt->ctrl, wdt->base + WDT_CTRL); >> if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE) { >> aspeed_wdt_start(&wdt->wdd); >> > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config 2017-06-28 14:29 ` Christopher Bostic @ 2017-06-28 14:54 ` Guenter Roeck 2017-06-28 14:59 ` Christopher Bostic 0 siblings, 1 reply; 17+ messages in thread From: Guenter Roeck @ 2017-06-28 14:54 UTC (permalink / raw) To: Christopher Bostic Cc: wim, robh+dt, mark.rutland, joel, linux-watchdog, devicetree, linux-kernel On Wed, Jun 28, 2017 at 09:29:50AM -0500, Christopher Bostic wrote: > > > On 6/28/17 6:31 AM, Guenter Roeck wrote: > >On 06/27/2017 02:17 PM, Christopher Bostic wrote: > >>Reference the system device tree when configuring the watchdog > >>engines. Set external signal mode on timeout if specified. > >>Set system reset on timeout if specified. > >> > >>Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> > >>--- > >>v2 - Change of_get_property() to of_property_read_bool() > >> - Remove redundant check for NULL struct device_node pointer > >> - Optional property names now start with prefix 'aspeed,' > >>--- > >> drivers/watchdog/aspeed_wdt.c | 13 +++++++++++-- > >> 1 file changed, 11 insertions(+), 2 deletions(-) > >> > >>diff --git a/drivers/watchdog/aspeed_wdt.c > >>b/drivers/watchdog/aspeed_wdt.c > >>index 1c65258..71ce5f5 100644 > >>--- a/drivers/watchdog/aspeed_wdt.c > >>+++ b/drivers/watchdog/aspeed_wdt.c > >>@@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device > >>*pdev) > >> { > >> struct aspeed_wdt *wdt; > >> struct resource *res; > >>+ struct device_node *np; > >> int ret; > >> wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > >>@@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device > >>*pdev) > >> * the SOC and not the full chip > >> */ > >> wdt->ctrl = WDT_CTRL_RESET_MODE_SOC | > >>- WDT_CTRL_1MHZ_CLK | > >>- WDT_CTRL_RESET_SYSTEM; > >>+ WDT_CTRL_1MHZ_CLK; > >>+ > >>+ np = pdev->dev.of_node; > >>+ if (of_property_read_bool(np, "aspeed,sys-reset")) > >>+ wdt->ctrl |= WDT_CTRL_RESET_SYSTEM; > >>+ > > > >For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM > >if no optional property is provided. > > > I had the logic inverted for this property in a previous patch. The > property was 'no-system-reset' so that when not present the default was to > set WDT_CTRL_RESET_SYSTEM. As it is in this patch, the only way to > indicate that no system reset is to be done is to not specify the property. > No system reset is desired under circumstances when another wdt engine is to > be responsible for this. Given the issue with backward compatibility > that's not a solution. Given this, would creating a property > 'no-system-reset' be acceptable? > Sorry, I fail to see the problem. There are half a dozen properties. What is the problem with having a default if no property is specified ? Your default is "do nothing", which does not really make any sense to me. If the user wants the watchdog to do nothing, the simple means to accomplish that would be to not instantiate it. Sure, that means specifying "system-reset" is redundant, but I don't see a problem with that either. But I do see a problem with loading a watchdog driver that doesn't do anything. If there is really some use case where it makes sense to load a watchdog driver and have it do nothing, please explain and provide a respective devicetree property. "aspeed,do-nothing" (or whatever similar) doesn't sound good, but at least makes it obvious that the driver isn't doing anything besides creating a false sense of "the system is watchdog protected". Thanks, Guenter > Thanks, > Chris > > >>+ if (of_property_read_bool(np, "aspeed,external-signal")) > >>+ wdt->ctrl |= WDT_CTRL_WDT_EXT; > >>+ > >>+ writel(wdt->ctrl, wdt->base + WDT_CTRL); > >> if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE) { > >> aspeed_wdt_start(&wdt->wdd); > >> > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config 2017-06-28 14:54 ` Guenter Roeck @ 2017-06-28 14:59 ` Christopher Bostic 2017-06-28 15:08 ` Guenter Roeck 0 siblings, 1 reply; 17+ messages in thread From: Christopher Bostic @ 2017-06-28 14:59 UTC (permalink / raw) To: Guenter Roeck Cc: wim, robh+dt, mark.rutland, joel, linux-watchdog, devicetree, linux-kernel On 6/28/17 9:54 AM, Guenter Roeck wrote: > On Wed, Jun 28, 2017 at 09:29:50AM -0500, Christopher Bostic wrote: >> >> On 6/28/17 6:31 AM, Guenter Roeck wrote: >>> On 06/27/2017 02:17 PM, Christopher Bostic wrote: >>>> Reference the system device tree when configuring the watchdog >>>> engines. Set external signal mode on timeout if specified. >>>> Set system reset on timeout if specified. >>>> >>>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> >>>> --- >>>> v2 - Change of_get_property() to of_property_read_bool() >>>> - Remove redundant check for NULL struct device_node pointer >>>> - Optional property names now start with prefix 'aspeed,' >>>> --- >>>> drivers/watchdog/aspeed_wdt.c | 13 +++++++++++-- >>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/watchdog/aspeed_wdt.c >>>> b/drivers/watchdog/aspeed_wdt.c >>>> index 1c65258..71ce5f5 100644 >>>> --- a/drivers/watchdog/aspeed_wdt.c >>>> +++ b/drivers/watchdog/aspeed_wdt.c >>>> @@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device >>>> *pdev) >>>> { >>>> struct aspeed_wdt *wdt; >>>> struct resource *res; >>>> + struct device_node *np; >>>> int ret; >>>> wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); >>>> @@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device >>>> *pdev) >>>> * the SOC and not the full chip >>>> */ >>>> wdt->ctrl = WDT_CTRL_RESET_MODE_SOC | >>>> - WDT_CTRL_1MHZ_CLK | >>>> - WDT_CTRL_RESET_SYSTEM; >>>> + WDT_CTRL_1MHZ_CLK; >>>> + >>>> + np = pdev->dev.of_node; >>>> + if (of_property_read_bool(np, "aspeed,sys-reset")) >>>> + wdt->ctrl |= WDT_CTRL_RESET_SYSTEM; >>>> + >>> For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM >>> if no optional property is provided. >>> >> I had the logic inverted for this property in a previous patch. The >> property was 'no-system-reset' so that when not present the default was to >> set WDT_CTRL_RESET_SYSTEM. As it is in this patch, the only way to >> indicate that no system reset is to be done is to not specify the property. >> No system reset is desired under circumstances when another wdt engine is to >> be responsible for this. Given the issue with backward compatibility >> that's not a solution. Given this, would creating a property >> 'no-system-reset' be acceptable? >> > Sorry, I fail to see the problem. There are half a dozen properties. What is the > problem with having a default if no property is specified ? Your default is "do > nothing", which does not really make any sense to me. If the user wants the > watchdog to do nothing, the simple means to accomplish that would be to not > instantiate it. > > Sure, that means specifying "system-reset" is redundant, but I don't see a > problem with that either. But I do see a problem with loading a watchdog driver > that doesn't do anything. > > If there is really some use case where it makes sense to load a watchdog driver > and have it do nothing, please explain and provide a respective devicetree > property. "aspeed,do-nothing" (or whatever similar) doesn't sound good, but > at least makes it obvious that the driver isn't doing anything besides > creating a false sense of "the system is watchdog protected". If system-reset is not wanted there are other properties that might still be needed, for example ARM reset only. We'd still want to instantiate it. Thanks, Chris > > Thanks, > Guenter > > >> Thanks, >> Chris >> >>>> + if (of_property_read_bool(np, "aspeed,external-signal")) >>>> + wdt->ctrl |= WDT_CTRL_WDT_EXT; >>>> + >>>> + writel(wdt->ctrl, wdt->base + WDT_CTRL); >>>> if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE) { >>>> aspeed_wdt_start(&wdt->wdd); >>>> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config 2017-06-28 14:59 ` Christopher Bostic @ 2017-06-28 15:08 ` Guenter Roeck 2017-06-28 15:55 ` Christopher Bostic 0 siblings, 1 reply; 17+ messages in thread From: Guenter Roeck @ 2017-06-28 15:08 UTC (permalink / raw) To: Christopher Bostic Cc: wim, robh+dt, mark.rutland, joel, linux-watchdog, devicetree, linux-kernel On Wed, Jun 28, 2017 at 09:59:22AM -0500, Christopher Bostic wrote: > > > On 6/28/17 9:54 AM, Guenter Roeck wrote: > >On Wed, Jun 28, 2017 at 09:29:50AM -0500, Christopher Bostic wrote: > >> > >>On 6/28/17 6:31 AM, Guenter Roeck wrote: > >>>On 06/27/2017 02:17 PM, Christopher Bostic wrote: > >>>>Reference the system device tree when configuring the watchdog > >>>>engines. Set external signal mode on timeout if specified. > >>>>Set system reset on timeout if specified. > >>>> > >>>>Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> > >>>>--- > >>>>v2 - Change of_get_property() to of_property_read_bool() > >>>> - Remove redundant check for NULL struct device_node pointer > >>>> - Optional property names now start with prefix 'aspeed,' > >>>>--- > >>>> drivers/watchdog/aspeed_wdt.c | 13 +++++++++++-- > >>>> 1 file changed, 11 insertions(+), 2 deletions(-) > >>>> > >>>>diff --git a/drivers/watchdog/aspeed_wdt.c > >>>>b/drivers/watchdog/aspeed_wdt.c > >>>>index 1c65258..71ce5f5 100644 > >>>>--- a/drivers/watchdog/aspeed_wdt.c > >>>>+++ b/drivers/watchdog/aspeed_wdt.c > >>>>@@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device > >>>>*pdev) > >>>> { > >>>> struct aspeed_wdt *wdt; > >>>> struct resource *res; > >>>>+ struct device_node *np; > >>>> int ret; > >>>> wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > >>>>@@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device > >>>>*pdev) > >>>> * the SOC and not the full chip > >>>> */ > >>>> wdt->ctrl = WDT_CTRL_RESET_MODE_SOC | > >>>>- WDT_CTRL_1MHZ_CLK | > >>>>- WDT_CTRL_RESET_SYSTEM; > >>>>+ WDT_CTRL_1MHZ_CLK; > >>>>+ > >>>>+ np = pdev->dev.of_node; > >>>>+ if (of_property_read_bool(np, "aspeed,sys-reset")) > >>>>+ wdt->ctrl |= WDT_CTRL_RESET_SYSTEM; > >>>>+ > >>>For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM > >>>if no optional property is provided. > >>> > >>I had the logic inverted for this property in a previous patch. The > >>property was 'no-system-reset' so that when not present the default was to > >>set WDT_CTRL_RESET_SYSTEM. As it is in this patch, the only way to > >>indicate that no system reset is to be done is to not specify the property. > >>No system reset is desired under circumstances when another wdt engine is to > >>be responsible for this. Given the issue with backward compatibility > >>that's not a solution. Given this, would creating a property > >>'no-system-reset' be acceptable? > >> > >Sorry, I fail to see the problem. There are half a dozen properties. What is the > >problem with having a default if no property is specified ? Your default is "do > >nothing", which does not really make any sense to me. If the user wants the > >watchdog to do nothing, the simple means to accomplish that would be to not > >instantiate it. > > > >Sure, that means specifying "system-reset" is redundant, but I don't see a > >problem with that either. But I do see a problem with loading a watchdog driver > >that doesn't do anything. > > > >If there is really some use case where it makes sense to load a watchdog driver > >and have it do nothing, please explain and provide a respective devicetree > >property. "aspeed,do-nothing" (or whatever similar) doesn't sound good, but > >at least makes it obvious that the driver isn't doing anything besides > >creating a false sense of "the system is watchdog protected". > If system-reset is not wanted there are other properties that might still be > needed, for example > ARM reset only. We'd still want to instantiate it. > But then you would presumably specify that property, so there would be one. The question here is what to do if _no_ property is provided. Sorry, I don't get your point. Guenter > Thanks, > Chris > > > >Thanks, > >Guenter > > > > > >>Thanks, > >>Chris > >> > >>>>+ if (of_property_read_bool(np, "aspeed,external-signal")) > >>>>+ wdt->ctrl |= WDT_CTRL_WDT_EXT; > >>>>+ > >>>>+ writel(wdt->ctrl, wdt->base + WDT_CTRL); > >>>> if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE) { > >>>> aspeed_wdt_start(&wdt->wdd); > >>>> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config 2017-06-28 15:08 ` Guenter Roeck @ 2017-06-28 15:55 ` Christopher Bostic 2017-06-28 16:06 ` Guenter Roeck 0 siblings, 1 reply; 17+ messages in thread From: Christopher Bostic @ 2017-06-28 15:55 UTC (permalink / raw) To: Guenter Roeck Cc: wim@iguana.be; openbmc, robh+dt, mark.rutland, joel, linux-watchdog, devicetree, linux-kernel On 6/28/17 10:08 AM, Guenter Roeck wrote: > On Wed, Jun 28, 2017 at 09:59:22AM -0500, Christopher Bostic wrote: >> >> On 6/28/17 9:54 AM, Guenter Roeck wrote: >>> On Wed, Jun 28, 2017 at 09:29:50AM -0500, Christopher Bostic wrote: >>>> On 6/28/17 6:31 AM, Guenter Roeck wrote: >>>>> On 06/27/2017 02:17 PM, Christopher Bostic wrote: >>>>>> Reference the system device tree when configuring the watchdog >>>>>> engines. Set external signal mode on timeout if specified. >>>>>> Set system reset on timeout if specified. >>>>>> >>>>>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> >>>>>> --- >>>>>> v2 - Change of_get_property() to of_property_read_bool() >>>>>> - Remove redundant check for NULL struct device_node pointer >>>>>> - Optional property names now start with prefix 'aspeed,' >>>>>> --- >>>>>> drivers/watchdog/aspeed_wdt.c | 13 +++++++++++-- >>>>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/watchdog/aspeed_wdt.c >>>>>> b/drivers/watchdog/aspeed_wdt.c >>>>>> index 1c65258..71ce5f5 100644 >>>>>> --- a/drivers/watchdog/aspeed_wdt.c >>>>>> +++ b/drivers/watchdog/aspeed_wdt.c >>>>>> @@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device >>>>>> *pdev) >>>>>> { >>>>>> struct aspeed_wdt *wdt; >>>>>> struct resource *res; >>>>>> + struct device_node *np; >>>>>> int ret; >>>>>> wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); >>>>>> @@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device >>>>>> *pdev) >>>>>> * the SOC and not the full chip >>>>>> */ >>>>>> wdt->ctrl = WDT_CTRL_RESET_MODE_SOC | >>>>>> - WDT_CTRL_1MHZ_CLK | >>>>>> - WDT_CTRL_RESET_SYSTEM; >>>>>> + WDT_CTRL_1MHZ_CLK; >>>>>> + >>>>>> + np = pdev->dev.of_node; >>>>>> + if (of_property_read_bool(np, "aspeed,sys-reset")) >>>>>> + wdt->ctrl |= WDT_CTRL_RESET_SYSTEM; >>>>>> + >>>>> For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM >>>>> if no optional property is provided. >>>>> >>>> I had the logic inverted for this property in a previous patch. The >>>> property was 'no-system-reset' so that when not present the default was to >>>> set WDT_CTRL_RESET_SYSTEM. As it is in this patch, the only way to >>>> indicate that no system reset is to be done is to not specify the property. >>>> No system reset is desired under circumstances when another wdt engine is to >>>> be responsible for this. Given the issue with backward compatibility >>>> that's not a solution. Given this, would creating a property >>>> 'no-system-reset' be acceptable? >>>> >>> Sorry, I fail to see the problem. There are half a dozen properties. What is the >>> problem with having a default if no property is specified ? Your default is "do >>> nothing", which does not really make any sense to me. If the user wants the >>> watchdog to do nothing, the simple means to accomplish that would be to not >>> instantiate it. >>> >>> Sure, that means specifying "system-reset" is redundant, but I don't see a >>> problem with that either. But I do see a problem with loading a watchdog driver >>> that doesn't do anything. >>> >>> If there is really some use case where it makes sense to load a watchdog driver >>> and have it do nothing, please explain and provide a respective devicetree >>> property. "aspeed,do-nothing" (or whatever similar) doesn't sound good, but >>> at least makes it obvious that the driver isn't doing anything besides >>> creating a false sense of "the system is watchdog protected". >> If system-reset is not wanted there are other properties that might still be >> needed, for example >> ARM reset only. We'd still want to instantiate it. >> > But then you would presumably specify that property, so there would be one. > The question here is what to do if _no_ property is provided. I agree that the default should be to perform a system reset when no property is provided. I will make that change. Additionally for backwards compatibility the SOC reset should be enabled by default when no property is provided. The other parameters are not configured in the watchdog control register if no property is provided. There is still a need to to configure for no system reset. For example, optional parameter 'aspeed,no-sys-reset' that can be specified if system reset is to be disabled. In addition the default 'SOC reset enabled' behavior could be disabled with an optional parameter 'aspeed,no-soc-reset'. Why would there even be a need configure for no system reset? The external mode 'wdt ext: External signal enable after timeout' can be sent off chip even if system reset is disabled. The watchdog is doing something in this case and would require wdt instantiation. Hope this is more clear, sorry for the confusion. Thanks, -Chris AST2500 Software Programming Guide > Sorry, I don't get your point. > > Guenter > >> Thanks, >> Chris >>> Thanks, >>> Guenter >>> >>> >>>> Thanks, >>>> Chris >>>> >>>>>> + if (of_property_read_bool(np, "aspeed,external-signal")) >>>>>> + wdt->ctrl |= WDT_CTRL_WDT_EXT; >>>>>> + >>>>>> + writel(wdt->ctrl, wdt->base + WDT_CTRL); >>>>>> if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE) { >>>>>> aspeed_wdt_start(&wdt->wdd); >>>>>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config 2017-06-28 15:55 ` Christopher Bostic @ 2017-06-28 16:06 ` Guenter Roeck 2017-06-28 16:09 ` Christopher Bostic 0 siblings, 1 reply; 17+ messages in thread From: Guenter Roeck @ 2017-06-28 16:06 UTC (permalink / raw) To: Christopher Bostic Cc: wim, robh+dt, mark.rutland, joel, linux-watchdog, devicetree, linux-kernel On Wed, Jun 28, 2017 at 10:55:08AM -0500, Christopher Bostic wrote: > > > On 6/28/17 10:08 AM, Guenter Roeck wrote: > >On Wed, Jun 28, 2017 at 09:59:22AM -0500, Christopher Bostic wrote: > >> > >>On 6/28/17 9:54 AM, Guenter Roeck wrote: > >>>On Wed, Jun 28, 2017 at 09:29:50AM -0500, Christopher Bostic wrote: > >>>>On 6/28/17 6:31 AM, Guenter Roeck wrote: > >>>>>On 06/27/2017 02:17 PM, Christopher Bostic wrote: > >>>>>>Reference the system device tree when configuring the watchdog > >>>>>>engines. Set external signal mode on timeout if specified. > >>>>>>Set system reset on timeout if specified. > >>>>>> > >>>>>>Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> > >>>>>>--- > >>>>>>v2 - Change of_get_property() to of_property_read_bool() > >>>>>> - Remove redundant check for NULL struct device_node pointer > >>>>>> - Optional property names now start with prefix 'aspeed,' > >>>>>>--- > >>>>>> drivers/watchdog/aspeed_wdt.c | 13 +++++++++++-- > >>>>>> 1 file changed, 11 insertions(+), 2 deletions(-) > >>>>>> > >>>>>>diff --git a/drivers/watchdog/aspeed_wdt.c > >>>>>>b/drivers/watchdog/aspeed_wdt.c > >>>>>>index 1c65258..71ce5f5 100644 > >>>>>>--- a/drivers/watchdog/aspeed_wdt.c > >>>>>>+++ b/drivers/watchdog/aspeed_wdt.c > >>>>>>@@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device > >>>>>>*pdev) > >>>>>> { > >>>>>> struct aspeed_wdt *wdt; > >>>>>> struct resource *res; > >>>>>>+ struct device_node *np; > >>>>>> int ret; > >>>>>> wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > >>>>>>@@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device > >>>>>>*pdev) > >>>>>> * the SOC and not the full chip > >>>>>> */ > >>>>>> wdt->ctrl = WDT_CTRL_RESET_MODE_SOC | > >>>>>>- WDT_CTRL_1MHZ_CLK | > >>>>>>- WDT_CTRL_RESET_SYSTEM; > >>>>>>+ WDT_CTRL_1MHZ_CLK; > >>>>>>+ > >>>>>>+ np = pdev->dev.of_node; > >>>>>>+ if (of_property_read_bool(np, "aspeed,sys-reset")) > >>>>>>+ wdt->ctrl |= WDT_CTRL_RESET_SYSTEM; > >>>>>>+ > >>>>>For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM > >>>>>if no optional property is provided. > >>>>> > >>>>I had the logic inverted for this property in a previous patch. The > >>>>property was 'no-system-reset' so that when not present the default was to > >>>>set WDT_CTRL_RESET_SYSTEM. As it is in this patch, the only way to > >>>>indicate that no system reset is to be done is to not specify the property. > >>>>No system reset is desired under circumstances when another wdt engine is to > >>>>be responsible for this. Given the issue with backward compatibility > >>>>that's not a solution. Given this, would creating a property > >>>>'no-system-reset' be acceptable? > >>>> > >>>Sorry, I fail to see the problem. There are half a dozen properties. What is the > >>>problem with having a default if no property is specified ? Your default is "do > >>>nothing", which does not really make any sense to me. If the user wants the > >>>watchdog to do nothing, the simple means to accomplish that would be to not > >>>instantiate it. > >>> > >>>Sure, that means specifying "system-reset" is redundant, but I don't see a > >>>problem with that either. But I do see a problem with loading a watchdog driver > >>>that doesn't do anything. > >>> > >>>If there is really some use case where it makes sense to load a watchdog driver > >>>and have it do nothing, please explain and provide a respective devicetree > >>>property. "aspeed,do-nothing" (or whatever similar) doesn't sound good, but > >>>at least makes it obvious that the driver isn't doing anything besides > >>>creating a false sense of "the system is watchdog protected". > >>If system-reset is not wanted there are other properties that might still be > >>needed, for example > >>ARM reset only. We'd still want to instantiate it. > >> > >But then you would presumably specify that property, so there would be one. > >The question here is what to do if _no_ property is provided. > I agree that the default should be to perform a system reset when no > property is provided. I will make that change. Additionally for backwards > compatibility the SOC reset should be enabled by default when no property is > provided. The other parameters are not configured in the watchdog control > register if no property is provided. > > There is still a need to to configure for no system reset. For example, > optional parameter 'aspeed,no-sys-reset' that can be specified if system > reset is to be disabled. In addition the default 'SOC reset enabled' > behavior could be disabled with an optional parameter 'aspeed,no-soc-reset'. > > Why would there even be a need configure for no system reset? The external > mode 'wdt ext: External signal enable after timeout' can be sent off chip > even if system reset is disabled. The watchdog is doing something in this > case and would require wdt instantiation. > Wouldn't that be "aspeed,external-signal" ? Guenter > Hope this is more clear, sorry for the confusion. > > Thanks, > -Chris AST2500 Software Programming Guide > >Sorry, I don't get your point. > > > >Guenter > > > >>Thanks, > >>Chris > >>>Thanks, > >>>Guenter > >>> > >>> > >>>>Thanks, > >>>>Chris > >>>> > >>>>>>+ if (of_property_read_bool(np, "aspeed,external-signal")) > >>>>>>+ wdt->ctrl |= WDT_CTRL_WDT_EXT; > >>>>>>+ > >>>>>>+ writel(wdt->ctrl, wdt->base + WDT_CTRL); > >>>>>> if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE) { > >>>>>> aspeed_wdt_start(&wdt->wdd); > >>>>>> > >>-- > >>To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in > >>the body of a message to majordomo@vger.kernel.org > >>More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config 2017-06-28 16:06 ` Guenter Roeck @ 2017-06-28 16:09 ` Christopher Bostic 2017-06-28 17:52 ` Guenter Roeck 0 siblings, 1 reply; 17+ messages in thread From: Christopher Bostic @ 2017-06-28 16:09 UTC (permalink / raw) To: Guenter Roeck Cc: wim, robh+dt, mark.rutland, joel, linux-watchdog, devicetree, linux-kernel On 6/28/17 11:06 AM, Guenter Roeck wrote: > On Wed, Jun 28, 2017 at 10:55:08AM -0500, Christopher Bostic wrote: >> >> On 6/28/17 10:08 AM, Guenter Roeck wrote: >>> On Wed, Jun 28, 2017 at 09:59:22AM -0500, Christopher Bostic wrote: >>>> On 6/28/17 9:54 AM, Guenter Roeck wrote: >>>>> On Wed, Jun 28, 2017 at 09:29:50AM -0500, Christopher Bostic wrote: >>>>>> On 6/28/17 6:31 AM, Guenter Roeck wrote: >>>>>>> On 06/27/2017 02:17 PM, Christopher Bostic wrote: >>>>>>>> Reference the system device tree when configuring the watchdog >>>>>>>> engines. Set external signal mode on timeout if specified. >>>>>>>> Set system reset on timeout if specified. >>>>>>>> >>>>>>>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> >>>>>>>> --- >>>>>>>> v2 - Change of_get_property() to of_property_read_bool() >>>>>>>> - Remove redundant check for NULL struct device_node pointer >>>>>>>> - Optional property names now start with prefix 'aspeed,' >>>>>>>> --- >>>>>>>> drivers/watchdog/aspeed_wdt.c | 13 +++++++++++-- >>>>>>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/watchdog/aspeed_wdt.c >>>>>>>> b/drivers/watchdog/aspeed_wdt.c >>>>>>>> index 1c65258..71ce5f5 100644 >>>>>>>> --- a/drivers/watchdog/aspeed_wdt.c >>>>>>>> +++ b/drivers/watchdog/aspeed_wdt.c >>>>>>>> @@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device >>>>>>>> *pdev) >>>>>>>> { >>>>>>>> struct aspeed_wdt *wdt; >>>>>>>> struct resource *res; >>>>>>>> + struct device_node *np; >>>>>>>> int ret; >>>>>>>> wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); >>>>>>>> @@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device >>>>>>>> *pdev) >>>>>>>> * the SOC and not the full chip >>>>>>>> */ >>>>>>>> wdt->ctrl = WDT_CTRL_RESET_MODE_SOC | >>>>>>>> - WDT_CTRL_1MHZ_CLK | >>>>>>>> - WDT_CTRL_RESET_SYSTEM; >>>>>>>> + WDT_CTRL_1MHZ_CLK; >>>>>>>> + >>>>>>>> + np = pdev->dev.of_node; >>>>>>>> + if (of_property_read_bool(np, "aspeed,sys-reset")) >>>>>>>> + wdt->ctrl |= WDT_CTRL_RESET_SYSTEM; >>>>>>>> + >>>>>>> For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM >>>>>>> if no optional property is provided. >>>>>>> >>>>>> I had the logic inverted for this property in a previous patch. The >>>>>> property was 'no-system-reset' so that when not present the default was to >>>>>> set WDT_CTRL_RESET_SYSTEM. As it is in this patch, the only way to >>>>>> indicate that no system reset is to be done is to not specify the property. >>>>>> No system reset is desired under circumstances when another wdt engine is to >>>>>> be responsible for this. Given the issue with backward compatibility >>>>>> that's not a solution. Given this, would creating a property >>>>>> 'no-system-reset' be acceptable? >>>>>> >>>>> Sorry, I fail to see the problem. There are half a dozen properties. What is the >>>>> problem with having a default if no property is specified ? Your default is "do >>>>> nothing", which does not really make any sense to me. If the user wants the >>>>> watchdog to do nothing, the simple means to accomplish that would be to not >>>>> instantiate it. >>>>> >>>>> Sure, that means specifying "system-reset" is redundant, but I don't see a >>>>> problem with that either. But I do see a problem with loading a watchdog driver >>>>> that doesn't do anything. >>>>> >>>>> If there is really some use case where it makes sense to load a watchdog driver >>>>> and have it do nothing, please explain and provide a respective devicetree >>>>> property. "aspeed,do-nothing" (or whatever similar) doesn't sound good, but >>>>> at least makes it obvious that the driver isn't doing anything besides >>>>> creating a false sense of "the system is watchdog protected". >>>> If system-reset is not wanted there are other properties that might still be >>>> needed, for example >>>> ARM reset only. We'd still want to instantiate it. >>>> >>> But then you would presumably specify that property, so there would be one. >>> The question here is what to do if _no_ property is provided. >> I agree that the default should be to perform a system reset when no >> property is provided. I will make that change. Additionally for backwards >> compatibility the SOC reset should be enabled by default when no property is >> provided. The other parameters are not configured in the watchdog control >> register if no property is provided. >> >> There is still a need to to configure for no system reset. For example, >> optional parameter 'aspeed,no-sys-reset' that can be specified if system >> reset is to be disabled. In addition the default 'SOC reset enabled' >> behavior could be disabled with an optional parameter 'aspeed,no-soc-reset'. >> >> Why would there even be a need configure for no system reset? The external >> mode 'wdt ext: External signal enable after timeout' can be sent off chip >> even if system reset is disabled. The watchdog is doing something in this >> case and would require wdt instantiation. >> > Wouldn't that be "aspeed,external-signal" ? Yes that optional parameter would need to be specified in that situation, as well as 'aspeed,no-system-reset'. Chris > > Guenter > >> Hope this is more clear, sorry for the confusion. >> >> Thanks, >> -Chris AST2500 Software Programming Guide >>> Sorry, I don't get your point. >>> >>> Guenter >>> >>>> Thanks, >>>> Chris >>>>> Thanks, >>>>> Guenter >>>>> >>>>> >>>>>> Thanks, >>>>>> Chris >>>>>> >>>>>>>> + if (of_property_read_bool(np, "aspeed,external-signal")) >>>>>>>> + wdt->ctrl |= WDT_CTRL_WDT_EXT; >>>>>>>> + >>>>>>>> + writel(wdt->ctrl, wdt->base + WDT_CTRL); >>>>>>>> if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE) { >>>>>>>> aspeed_wdt_start(&wdt->wdd); >>>>>>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config 2017-06-28 16:09 ` Christopher Bostic @ 2017-06-28 17:52 ` Guenter Roeck 0 siblings, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2017-06-28 17:52 UTC (permalink / raw) To: Christopher Bostic Cc: wim, robh+dt, mark.rutland, joel, linux-watchdog, devicetree, linux-kernel On Wed, Jun 28, 2017 at 11:09:15AM -0500, Christopher Bostic wrote: > > >> > >>Why would there even be a need configure for no system reset? The external > >>mode 'wdt ext: External signal enable after timeout' can be sent off chip > >>even if system reset is disabled. The watchdog is doing something in this > >>case and would require wdt instantiation. > >> > >Wouldn't that be "aspeed,external-signal" ? > Yes that optional parameter would need to be specified in that situation, as > well as 'aspeed,no-system-reset'. > Ok, makes sense (after looking into the datasheet). The missing detail is that there is a separate configuration bit for "Reset system after timeout", which is set in addition to reset_{sys,soc,arm}. Thanks, Guenter ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-06-28 17:52 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-27 21:17 [PATCH v2 0/2] Add ASPEED watchdog device tree properties Christopher Bostic 2017-06-27 21:17 ` [PATCH v2 1/2] drivers/watchdog: Add optional ASPEED " Christopher Bostic 2017-06-27 21:32 ` Guenter Roeck 2017-06-27 21:42 ` Christopher Bostic 2017-06-27 22:07 ` Guenter Roeck 2017-06-28 14:55 ` Christopher Bostic 2017-06-28 15:06 ` Guenter Roeck 2017-06-27 21:17 ` [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config Christopher Bostic 2017-06-28 11:31 ` Guenter Roeck 2017-06-28 14:29 ` Christopher Bostic 2017-06-28 14:54 ` Guenter Roeck 2017-06-28 14:59 ` Christopher Bostic 2017-06-28 15:08 ` Guenter Roeck 2017-06-28 15:55 ` Christopher Bostic 2017-06-28 16:06 ` Guenter Roeck 2017-06-28 16:09 ` Christopher Bostic 2017-06-28 17:52 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).