lkml.org 
[lkml]   [2017]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 1/2] drivers/watchdog: Add optional ASPEED device tree properties
From
Date


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
>

\
 
 \ /
  Last update: 2017-07-06 21:27    [W:0.060 / U:0.032 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site