From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752234AbdF1OaM (ORCPT ); Wed, 28 Jun 2017 10:30:12 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:32991 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751745AbdF1O36 (ORCPT ); Wed, 28 Jun 2017 10:29:58 -0400 Subject: Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config To: Guenter Roeck , wim@iguana.be, robh+dt@kernel.org, mark.rutland@arm.com, joel@jms.id.au, linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org References: <20170627211734.60477-1-cbostic@linux.vnet.ibm.com> <20170627211734.60477-3-cbostic@linux.vnet.ibm.com> <606a2e11-2d43-5204-0bb6-9c4415ee03df@roeck-us.net> Cc: linux-kernel@vger.kernel.org From: Christopher Bostic Date: Wed, 28 Jun 2017 09:29:50 -0500 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <606a2e11-2d43-5204-0bb6-9c4415ee03df@roeck-us.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17062814-0004-0000-0000-00001277994D X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007290; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000214; SDB=6.00879938; UDB=6.00438611; IPR=6.00660092; BA=6.00005445; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00015991; XFM=3.00000015; UTC=2017-06-28 14:29:55 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17062814-0005-0000-0000-00007FF7C2D4 Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-06-28_09:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1706280236 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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); >> > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christopher Bostic Subject: Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config Date: Wed, 28 Jun 2017 09:29:50 -0500 Message-ID: References: <20170627211734.60477-1-cbostic@linux.vnet.ibm.com> <20170627211734.60477-3-cbostic@linux.vnet.ibm.com> <606a2e11-2d43-5204-0bb6-9c4415ee03df@roeck-us.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <606a2e11-2d43-5204-0bb6-9c4415ee03df-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> Sender: linux-watchdog-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Guenter Roeck , wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org, linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: devicetree@vger.kernel.org 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 >> --- >> 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); >> > -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html