From: Christopher Bostic <cbostic@linux.vnet.ibm.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: mark.rutland@arm.com, Rob Herring <robh@kernel.org>,
linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org,
openbmc@lists.ozlabs.org, linux-kernel@vger.kernel.org,
wim@iguana.be
Subject: Re: [PATCH v3 1/2] drivers/watchdog: Add optional ASPEED device tree properties
Date: Thu, 6 Jul 2017 18:32:26 -0500 [thread overview]
Message-ID: <e90bdd25-4213-70f6-b72f-b3f75d46a4df@linux.vnet.ibm.com> (raw)
In-Reply-To: <20170706204850.GA20079@roeck-us.net>
On 7/6/17 3:48 PM, Guenter Roeck wrote:
> On Thu, Jul 06, 2017 at 02:27:18PM -0500, Christopher Bostic wrote:
>>
>> 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.
>>
> Maybe add 'aspeed,reset-type = "none"' ? The default (no property) would
> then be "system".
That's what I was thinking -will do that.
Thanks,
Chris
>
> Thanks,
> Guenter
>
next prev parent reply other threads:[~2017-07-06 23:32 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-29 0:28 [PATCH v3 0/2] Add ASPEED watchdog device tree properties Christopher Bostic
2017-06-29 0:28 ` [PATCH v3 1/2] drivers/watchdog: Add optional ASPEED " Christopher Bostic
2017-06-29 3:33 ` Guenter Roeck
2017-06-29 13:39 ` Christopher Bostic
2017-06-29 15:04 ` Guenter Roeck
2017-07-06 14:35 ` Rob Herring
2017-07-06 19:27 ` Christopher Bostic
2017-07-06 20:48 ` Guenter Roeck
2017-07-06 23:32 ` Christopher Bostic [this message]
2017-06-29 0:28 ` [PATCH v3 2/2] drivers/watchdog: ASPEED reference dev tree properties for config Christopher Bostic
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e90bdd25-4213-70f6-b72f-b3f75d46a4df@linux.vnet.ibm.com \
--to=cbostic@linux.vnet.ibm.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=mark.rutland@arm.com \
--cc=openbmc@lists.ozlabs.org \
--cc=robh@kernel.org \
--cc=wim@iguana.be \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).