From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751790AbbKCFMp (ORCPT ); Tue, 3 Nov 2015 00:12:45 -0500 Received: from mail-bl2on0143.outbound.protection.outlook.com ([65.55.169.143]:51424 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750742AbbKCFMl (ORCPT ); Tue, 3 Nov 2015 00:12:41 -0500 Authentication-Results: spf=fail (sender IP is 192.88.168.50) smtp.mailfrom=freescale.com; lists.infradead.org; dkim=none (message not signed) header.d=none;lists.infradead.org; dmarc=none action=none header.from=freescale.com; Date: Tue, 3 Nov 2015 12:55:02 +0800 From: Robin Gong To: Guenter Roeck CC: , , , Subject: Re: [PATCH v1 2/2] watchdog: imx2_wdt: add set_pretimeout interface Message-ID: <20151103045501.GB26305@shlinux2> References: <1446521367-25748-1-git-send-email-b38343@freescale.com> <1446521367-25748-2-git-send-email-b38343@freescale.com> <563835C0.5010204@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <563835C0.5010204@roeck-us.net> User-Agent: Mutt/1.5.21 (2010-09-15) X-EOPAttributedMessage: 0 X-Microsoft-Exchange-Diagnostics: 1;BN1AFFO11FD025;1:7ppHOVK0ICrTOTTllYxcyGoY+aAs4WR9DcFfw0R+NTQn0fiBwC4oyqeStyEQtCs7zZkMeZDCaZXkz2a2vv9qxTFZpq+I3Nv+bcqpxmXBb9f7orgDQS9fS6scrpj1EMpCUNG9hzj41RD2+wGus4AU1zE9n8IM+t3ojMdpNc6RrsGFlVB6R+6NwY6852fsA9CxKm18A0/Fud3Isj5UMn5bizT5DcnMTyWFY3RESSOwfhMmntHqo56JTBL59zPloaIb7OnHOk4KLkJwSBlZaQP9ifugpHhNbpJ+RnNfMQWdqD6IRvjbpMZitmTvtIRY1PlC4xtUKD4ei0PlbRFscuDiSjzkg4Nfwh0lN6yMxjEfpGHvIb8IG6JGrtkHktYT5mKF X-Forefront-Antispam-Report: CIP:192.88.168.50;CTRY:US;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10019020)(6009001)(2980300002)(1109001)(1110001)(339900001)(199003)(479174004)(377454003)(189002)(24454002)(81156007)(189998001)(87936001)(92566002)(50466002)(46406003)(33716001)(83506001)(2950100001)(33656002)(23726002)(5008740100001)(97756001)(5001960100002)(110136002)(76176999)(54356999)(77096005)(50986999)(104016004)(6806005)(11100500001)(97736004)(4001350100001)(19580395003)(85426001)(105606002)(47776003)(106466001)(19580405001)(5007970100001)(42262002);DIR:OUT;SFP:1102;SCL:1;SRVR:DM2PR0301MB0734;H:tx30smr01.am.freescale.net;FPR:;SPF:Fail;PTR:InfoDomainNonexistent;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: 1;DM2PR0301MB0734;2:XhlYpj+z2O5DYFFkcG0rsYtrSXxwvO0ftdQPRDPes1OUsSGXHYGPDA2rc1Hb6YBY6siTRl1WPwgz8Re4EFO1ot0QrAe7xPBIHcnPV1dNvvKPTjM67uP2pJgVkjFbU5+sWuUtzjQkhfVcoRwJosWUw2WDDRW6ST5tPfwTBUm4EjU=;3:Dn7fw13rrO82ywuO2VMMTOyKQ1I0/DM9EaiAwXsSjsk3ehJdOkE4vkx2mWuQfBcVZiSf9GTDNmg+a/+uccGSCSOJMDsSfzgQfs+egB0jcmUdFxAsiWAd2rVRTwpwXT9qVAHPLn3DwXUtCVZxWUrcaR0eBY4WKyPQinAiV2BKwkCiBcn+ng+AZEblsWRS8uOaPCEMpjy/OZ9GoHK+nK6CezvsKes///L3G+NEkqq3y8E=;25:J7VTq+Nm3KDQOmHu1g+/ClVUtQF+s5lxOU2evsi47lFaB5Ns/gP7VaVl0BduYNhq/M8HUfu4ba0RbVBsO/F9bjBhHYoB7xRLuB64AGvnllpcCFV/UuFc6A0NW8VBjgZeElsjv+T4jSHhWj9udvTffwosKsV5zAfkSsXPGgKKt+XsZGbjWYiGcauxBuH6YfrbSB4hcR9Uq4CPZOuUlY09UFv6LJJA+wCqZOBbSo02pxZmRV51ecraXJfEtl6+0ydSi43SvhQvJ9F9GLxif54cPA== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:DM2PR0301MB0734; X-Microsoft-Exchange-Diagnostics: 1;DM2PR0301MB0734;20:1vH7+KQx7SYTdh4A8HRkeAF6ir02DH8NU40HBpH0424XeDx6g+wTfOYMJzKqKXUwi0Sf6BksbwuouKXO2gTrRzrOGaq7F2cFv69s97NEF3TlUcHn6oPTD2FrcBn9uUUXewBa3n8DoPV2rAHE6f+YeWhgvxIp5dDAp2FfqHP/fAX37uriTYQ6XV4slgh8S4MrMOFLdvnGE/ZnwRrfEmCIZBUQrWZjqnC9VDzsIIlP1sVIld2UXIPVmv7p+8GfE2kJ/pM1Et8x83mG7g2Bi62QJR0ywtka5WJrd7Vvx/aG+FQE0sYqaPwc7//NrbP+19FHtIio5wiPSvP+7+MTiXxT0bGrkkvkC9c+sX6LIsaPBZs=;4:kj53IvB4nKVPSo020+M6G22dNKaQeGhh3aNBYrqTeoUcReyWcA1ToypjiA7sYdaTrPSXAg/DFgMsUmlbLBGS/sq0nECXWYY1W+atcxKoqrUBz5grgU1TcxaXDBtYoiElbEsq7n35mO1o0dcoeyToYQtYchx8w1UM56eGWLdRwAMxn+49NPhtJ5+mnxQD+M36+tLHoWff7lcoXkatb1h30g1jf8KTqrKo5uBHqlEy4FoEKeeXw9hEsr/DD2UL9usRltHWFo3/YHeaeOaavpjW73YOuSCTkjj8TkgzdaolsyrJF8IVi+ZVvCU+rHYF8A922s/VNZ8u40koyLJsTdhal7dD+cbACa8tjsL/NoWkOhKNbDP26Zbl4Sc36FsYDzYR X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(101931422205132); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(520078)(5005006)(8121501046)(3002001)(10201501046);SRVR:DM2PR0301MB0734;BCL:0;PCL:0;RULEID:;SRVR:DM2PR0301MB0734; X-Forefront-PRVS: 0749DC2CE6 X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;DM2PR0301MB0734;23:X+cuP1mUNlO7hzTTNmV7ui5Fg/xNLrzXcRm+eDv?= =?us-ascii?Q?0epIPHYjHQjXyQ6qNGQP7fj2RmvJQUHxBIN5x9ZuYSxzzkxqX3dHOczoWyXM?= =?us-ascii?Q?65IxoQvu8LgQgraMHcgTOjbqmDC4TQfqAVpOMthtOl6J8x4muEGIHtaD93qC?= =?us-ascii?Q?IjjTv42U2oDlnnsLJpuTpzRO5GY8ef0IQZocBhjnlUfhKG3FC5lzMGfuHUgT?= =?us-ascii?Q?6M59EudL0uktsFexq+QVaytorO/NY8cPnWq0YHccmkEOAY2OlRDel3l37kE0?= =?us-ascii?Q?ZG9R2cluoWFkenz37XJy+prhR7U4AtnLNX0PqbN/lpPeMBqPxHU5pf6JJYIe?= =?us-ascii?Q?Q9MA2SgiqfgCcDRWRXrvsxa83e3azvC4Ca/eh1GBGO90D4Mq30YMFIDcgZi3?= =?us-ascii?Q?/TTfJbBA2gerB/oSXkMz7P8TFxLG1Sse1tRg3w/wBbt/ZGbof0CT4vBwn3og?= =?us-ascii?Q?fkxS1C6VTlCLuA7Udbf4tltcmCjTZ9oHDDLphKupDE/TnPt8FO0Uly2p6iZ+?= =?us-ascii?Q?KkL1BUmP315qeY4GeDFVTZ9mHuc0NMeQXWqWJz8Z0eoaCPVLG4WUaxFWIYjp?= =?us-ascii?Q?W7tRLbRi9wBQPUPSkKlokCXpikeVtOEW1M+G7jKZe4a1RXuxnWHdL7+guF6z?= =?us-ascii?Q?XQmOOO4qveS9+yZbGo6Al0YvqH6RxbhnpbIci5ETA0SHKwLSM48Ewz1KbYUB?= =?us-ascii?Q?bG+GlDSY/TvhcqCzRYUjmIls924buWCbnq7ty3Yvc87jKasSS4wSsJ0FRtc2?= =?us-ascii?Q?fUhwfMaoPmFrHWa1Bzp2ItVJjgZHdb6PpHPZnle4h4Mu1D2VgiDQgxQllqgS?= =?us-ascii?Q?SbaJ4jJvI3OnNLk6zCm7fP0AG0THJc6lP73zYPYoXrAYc2yN8xLr0oOluTVB?= =?us-ascii?Q?Ga5if6CpMWpcreiHWXdmwb6YaaeuiZ9LUOrvNVCgOut9J3gRzWuWNElsVwcG?= =?us-ascii?Q?7H3yRPyccrN1jdIKefT6usPst8jpvfBoikK4Rn8oOkq80svaf4TbuKwAPsXp?= =?us-ascii?Q?pYd4Mi4h69jFVsN8shQoCdkAkfnnJ4V0tqondJjlqh7HKgPpBC21gp5f5JBs?= =?us-ascii?Q?HYzzz7xSl1sw97DSUQKjiErgyRqkZzptkW17PlYPoRtxbqmOaV59fcvn+fhs?= =?us-ascii?Q?rCMs2Mj8dTmLk3cxRXFvqXisor0JeypnT?= X-Microsoft-Exchange-Diagnostics: 1;DM2PR0301MB0734;5:q/nifdaM2ZUDHUcZ+O30HNjjDeWL1emeOr2/q+w6k5PR8aDRpj9J2vyG+u+YhuCZvQBIYvLySvNfrVoXlEW1d0V13k5AvQ8VZObAEgWG2boPkwx6EMp1CMqWx/WX7Hpshho9BOfw4k2lFzXhOF3aag==;24:4bXYkMRRIllr4nXRPVqCoe6neXkmFDuraxZ5xspfU3V5cOzStCr2aRcAVpvH5SJjnyGT/sgshBKKNsxPYePYqT5ZRKZZFhuNmjEPcEED5ZU=;20:fZGT0r50zewi2ewP4fbihWKlNDZQ5HJyHFSvq98x2huR6i0GVNh3LBoOG5gvp7NiIYy9OpZxeFSaPhvPDKyLGw== SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Nov 2015 04:56:54.8271 (UTC) X-MS-Exchange-CrossTenant-Id: 710a03f5-10f6-4d38-9ff4-a80b81da590d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=710a03f5-10f6-4d38-9ff4-a80b81da590d;Ip=[192.88.168.50];Helo=[tx30smr01.am.freescale.net] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM2PR0301MB0734 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 02, 2015 at 08:19:12PM -0800, Guenter Roeck wrote: > On 11/02/2015 07:29 PM, Robin Gong wrote: > >Enable set_pretimeout interface and trigger the pretimeout interrupt before > >watchdog timeout event happen. > > > >Signed-off-by: Robin Gong > >--- > > drivers/watchdog/imx2_wdt.c | 67 ++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 66 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c > >index 0bb1a1d..d3c6b07 100644 > >--- a/drivers/watchdog/imx2_wdt.c > >+++ b/drivers/watchdog/imx2_wdt.c > >@@ -24,7 +24,9 @@ > > #include > > #include > > #include > >+#include > > #include > >+#include > > Are those two new includes both needed ? > Yes, irq.h is not needed. > > #include > > #include > > #include > >@@ -52,12 +54,18 @@ > > #define IMX2_WDT_WRSR 0x04 /* Reset Status Register */ > > #define IMX2_WDT_WRSR_TOUT (1 << 1) /* -> Reset due to Timeout */ > > > >+#define IMX2_WDT_WICR 0x06 /*Interrupt Control Register*/ > >+#define IMX2_WDT_WICR_WIE (1 << 15) /* -> Interrupt Enable */ > >+#define IMX2_WDT_WICR_WTIS (1 << 14) /* -> Interrupt Status */ > >+#define IMX2_WDT_WICR_WICT (0xFF << 0) /* Watchdog Interrupt Timeout */ > >+ > > "<< 0" doesn't really add any value here. > Accept. > > #define IMX2_WDT_WMCR 0x08 /* Misc Register */ > > > > #define IMX2_WDT_MAX_TIME 128 > > #define IMX2_WDT_DEFAULT_TIME 60 /* in seconds */ > > > > #define WDOG_SEC_TO_COUNT(s) ((s * 2 - 1) << 8) > >+#define WDOG_SEC_TO_PRECOUNT(s) (s * 2) /* set WDOG pre timeout count*/ > > > ((s) * 2) > > Ah yes, WDOG_SEC_TO_COUNT should also use (s). > > > struct imx2_wdt_device { > > struct clk *clk; > >@@ -80,7 +88,8 @@ MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default=" > > > > static const struct watchdog_info imx2_wdt_info = { > > .identity = "imx2+ watchdog", > >- .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE, > >+ .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE > >+ | WDIOF_PRETIMEOUT, > > }; > > > > static int imx2_restart_handler(struct notifier_block *this, unsigned long mode, > >@@ -207,12 +216,59 @@ static inline void imx2_wdt_ping_if_active(struct watchdog_device *wdog) > > } > > } > > > >+static int imx2_wdt_check_pretimeout_set(struct imx2_wdt_device *wdev) > >+{ > >+ u32 val; > >+ > >+ regmap_read(wdev->regmap, IMX2_WDT_WICR, &val); > >+ return (val & IMX2_WDT_WICR_WIE) ? 1 : 0; > > I don't understand the point of this function. > You check if IMX2_WDT_WICR_WIE is set, > Yes, looks no need check,just directly set this bit. > >+} > >+ > >+static int imx2_wdt_set_pretimeout(struct watchdog_device *wdog, > >+ unsigned int new_timeout) > >+{ > >+ struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog); > >+ u32 val; > >+ > >+ regmap_read(wdev->regmap, IMX2_WDT_WICR, &val); > >+ /* set the new pre-timeout value in the WSR */ > >+ val &= ~IMX2_WDT_WICR_WICT; > >+ val |= WDOG_SEC_TO_PRECOUNT(new_timeout); > >+ > > What is the time here ? Is pretimeout the number of seconds > until the interrupt occurs, or the number of seconds before the actual > timeout (as per API) ? > The latter is. > >+ if (!imx2_wdt_check_pretimeout_set(wdev)) > >+ val |= IMX2_WDT_WICR_WIE; /*enable*/ > > if IMX2_WDT_WICR_WIE is not set, you set it, > > >+ > >+ regmap_write(wdev->regmap, IMX2_WDT_WICR, val); > >+ > > and write the result unconditionally. Unless I am missing something, > > regmap_write, wdev->regmap, IMX2_WDT_WICR, val | IMX2_WDT_WICR_WIE); > > would accomplish exactly the same. > > >+ wdog->pretimeout = new_timeout; > >+ > >+ return 0; > >+} > >+ > >+static irqreturn_t imx2_wdt_isr(int irq, void *dev_id) > >+{ > >+ struct platform_device *pdev = dev_id; > >+ struct watchdog_device *wdog = platform_get_drvdata(pdev); > >+ struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog); > >+ u32 val; > >+ > >+ regmap_read(wdev->regmap, IMX2_WDT_WICR, &val); > >+ if (val & IMX2_WDT_WICR_WTIS) { > >+ /*clear interrupt status bit*/ > >+ regmap_write(wdev->regmap, IMX2_WDT_WICR, val); > >+ dev_warn(&pdev->dev, "pre-timeout:%d, %d Seconds remained\n", > >+ wdog->pretimeout, wdog->timeout - wdog->pretimeout); > > The idea here is that this should trigger a panic. > Just add a print message, our customer will add what they want here. > >+ } > >+ return IRQ_HANDLED; > >+} > >+ > > static const struct watchdog_ops imx2_wdt_ops = { > > .owner = THIS_MODULE, > > .start = imx2_wdt_start, > > .stop = imx2_wdt_stop, > > .ping = imx2_wdt_ping, > > .set_timeout = imx2_wdt_set_timeout, > >+ .set_pretimeout = imx2_wdt_set_pretimeout, > > }; > > > > static const struct regmap_config imx2_wdt_regmap_config = { > >@@ -229,6 +285,7 @@ static int __init imx2_wdt_probe(struct platform_device *pdev) > > struct resource *res; > > void __iomem *base; > > int ret; > >+ int irq; > > u32 val; > > > > wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL); > >@@ -253,6 +310,14 @@ static int __init imx2_wdt_probe(struct platform_device *pdev) > > return PTR_ERR(wdev->clk); > > } > > > >+ irq = platform_get_irq(pdev, 0); > > This makes the irq mandatory. What if a platform doesn't have one configured ? > > >+ ret = devm_request_irq(&pdev->dev, irq, imx2_wdt_isr, 0, > >+ dev_name(&pdev->dev), pdev); > >+ if (ret) { > >+ dev_err(&pdev->dev, "can't get irq %d\n", irq); > > You got it, but you could not request it. > > This is also a bit early, as the interrupt handler uses variables which are not yet > initialized. > Accept. > >+ return ret; > >+ } > >+ > > wdog = &wdev->wdog; > > wdog->info = &imx2_wdt_info; > > wdog->ops = &imx2_wdt_ops; > > >