Messages in this thread | | | Subject | Re: [PATCH v3 1/2] drivers/watchdog: Add optional ASPEED device tree properties | From | Christopher Bostic <> | Date | Thu, 6 Jul 2017 14:27:18 -0500 |
| |
On 7/6/17 9:35 AM, Rob Herring wrote: > On Thu, Jun 29, 2017 at 08:04:17AM -0700, Guenter Roeck wrote: >> On Thu, Jun 29, 2017 at 08:39:59AM -0500, Christopher Bostic wrote: >>> >>> On 6/28/17 10:33 PM, Guenter Roeck wrote: >>>> On 06/28/2017 05:28 PM, Christopher Bostic wrote: >>>>> Describe device tree optional properties: >>>>> >>>>> * aspeed,arm-reet - ARM CPU reset on signal >>>>> * aspeed,no-soc-reset - SOC reset on signal >>>>> * aspeed,no-sys-reset - System reset on signal >>>>> * 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> >>>>> --- >>>>> v3 - Invert soc and sys reset to 'no' to preserve backwards >>>>> compatibility. SOC and SYS reset will be set by default >>>>> without any optional parameters set >>>>> v2 - Add 'aspeed,' prefix to all optional properties >>>>> - Add arm-reset, soc-reset, interrupt, alt-boot properties >>>>> --- >>>>> .../devicetree/bindings/watchdog/aspeed-wdt.txt | 24 >>>>> ++++++++++++++++++++++ >>>>> 1 file changed, 24 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt >>>>> b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt >>>>> index c5e74d7..6f18005 100644 >>>>> --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt >>>>> +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt >>>>> @@ -8,9 +8,33 @@ 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/not generate 6 types of signals: >>>>> + >>>>> + - aspeed,arm-reset: If property is present then reset ARM CPU only. >>>>> + If not specified no ARM CPU reset is done. >>>>> + >>>>> + - aspeed,no-soc-reset: If property is present then do not reset SOC. >>>>> + If not specified then SOC reset is done. >>>>> + >>>>> + - aspeed,no-sys-reset: If property is present then do not reset >>>>> system. >>>>> + Typcally used in tandem with 'aspeed-external-signal' >>>> Is this correct ? As I understand the datasheet, it could also used in >>>> tandem with >>>> aspeed,interrupt. >>> True, that should be documented. Will add that. >>>>> + If not specified then system reset is done. >>>>> + >>>> I'll leave it up to Rob to decide, but for my part I don't understand >>>> no-soc-reset. >>> As the aspeed watchdog driver exists prior to this change an SOC reset is >>> done by >>> default. In order to preserve backwards compatibility a missing optional >>> property >>> should result in default behavior. I however need to be able to specify >>> that SOC >>> reset be disabled in some way. This goes back to our discussion about why >>> we'd >>> ever want to disable SYSTEM reset in the first place. Same reasoning >>> applies for >>> SOC reset. >>> >>>> I would instead use four properties. >>>> >>>> aspeed,arm-reset >>>> aspeed,soc-reset >>> Per my response above I think it should remain as aspeed,no-soc-reset due to >>> backwards compatibility requirements. >> The same can be accomplished with "aspeed,no-reset", which would avoid the, in >> my opinion, awkward "no-{sys,soc}-reset" poperties. >> >>>> aspeed,sys-reset (which is the default) >>> Again as per our discussion yesterday I need some way to specify how system >>> reset is to be done. For backwards compatibility, a lack of parameter here >>> would >>> result in a system reset being configured. Only way to indicate to the >>> driver >>> that no system reset is to be done is to indicate 'no' system reset in the >>> optional >>> parameter. >> Or "aspeed,no-reset". >> >>>> aspeed,no-reset >>> This parameter seems ambiguous as we could be doing a 'no system reset' >>> or a 'no SOC reset' in theory. >> {arm,soc,sys}-reset are mutually exclusive per datasheet, and there is a >> separate configuration bit which enables the reset in the first place. >> I don't see how that is ambiguous. >> >> Anyway, I don't think we are making any progress here. Let's wait for >> guidance from Rob. > I tend to agree with Guenter. > > Maybe using the form 'aspeed,reset-type = "cpu|soc|system"' would be > more aligned to the type of reset being mutually exclusive.
OK I'll update to this method.
Thanks -Chris
> > Rob >
| |