From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8E042C43381 for ; Sun, 24 Feb 2019 03:29:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3A12D2054F for ; Sun, 24 Feb 2019 03:29:05 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="j/GCzRu2" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727915AbfBXD3E (ORCPT ); Sat, 23 Feb 2019 22:29:04 -0500 Received: from mail-pl1-f196.google.com ([209.85.214.196]:41326 "EHLO mail-pl1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727752AbfBXD3E (ORCPT ); Sat, 23 Feb 2019 22:29:04 -0500 Received: by mail-pl1-f196.google.com with SMTP id y5so2890481plk.8; Sat, 23 Feb 2019 19:29:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=v5R5LCGjAlFCGhFSnspcqiVvj2dc3luKtcjDkiR+PU0=; b=j/GCzRu2vT4DNYcV1gq9//xX1rrZz6vW+mwKXGLfS4MC3i8oCdIKHDpQHtkdKnGmGD Iod5ptSz6+/wE41F7vaQsMVSUZtmNHztdc2Ja3GiIrZ5TQos5pCxmWAytGcM3mYSVwVK bRcIVJCLdXeK9tACHp318Pt+pjCVWLdyApSC1AqI7TWQHz7nv9c9J9NMYTBo0sFkKiad xnylVj6/M3GF8YUNgHB3HR+cUcSlCTITTHVZmnt6ULVvrgvBASOWioGSYr/P7WFlYH3q ltTeKMvJkcKlCiQh0wpk4Glar2pcoxm+L4Jt+rUl+usi2HDaBSbfPZksA9jHf59Bi1+4 6Nsg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:subject:to:cc:references:from:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=v5R5LCGjAlFCGhFSnspcqiVvj2dc3luKtcjDkiR+PU0=; b=IWpOb4pfqdDICYCd4jdz/be/E3sW0I7kqC3kQ+WhMyZO8sExZl41ktFwgHh8KIAyxH 0mISF2nR3ChBF6zqcprpzlxduS75yDBYPM5Oy+otzc7f8CTv6xFzRg8tO81Rk98VEu+k aGgeyX+s5pNicYTVzG4pfobkp3ooak/R73Q7wEsp3R4riVvxZA5pxzo1DMdEUeLi/HgB tQC1otEcpNop4BBU+MN6cGI5eVMJQzhRCpc0BTFp5H0mqsi/YBJ//y2dvDUtLqlDyErn tlIYKmC3JTb1eXySJSqDn68U/fKhe2vGFKAIVboSHFt+dq3MZVRrpQPd+f4ug8EFszjl ndOw== X-Gm-Message-State: AHQUAubANELVK7YYmMbGywoNGCTRxSA4Ub/HFJo8ryeaysFVOfWmqhng lUChOuMYKA2HOee/UVdcw70= X-Google-Smtp-Source: AHgI3Ia9R2c1ss7zYNae4m0mgNhPhwVQ+A4+cPZvVGiTV00jvwlajfrkhlfXrKdqtSJPvro/jjO1fg== X-Received: by 2002:a17:902:298a:: with SMTP id h10mr12225949plb.312.1550978943359; Sat, 23 Feb 2019 19:29:03 -0800 (PST) Received: from server.roeck-us.net ([2600:1700:e321:62f0:329c:23ff:fee3:9d7c]) by smtp.gmail.com with ESMTPSA id e63sm14631470pfc.47.2019.02.23.19.29.01 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 23 Feb 2019 19:29:02 -0800 (PST) Subject: Re: [PATCH RESEND V2 2/4] watchdog: imx_sc: Add i.MX system controller watchdog support To: Anson Huang , "robh+dt@kernel.org" , "mark.rutland@arm.com" , "shawnguo@kernel.org" , "s.hauer@pengutronix.de" , "kernel@pengutronix.de" , "festevam@gmail.com" , "catalin.marinas@arm.com" , "will.deacon@arm.com" , "wim@linux-watchdog.org" , Aisheng Dong , "ulf.hansson@linaro.org" , Daniel Baluta , Andy Gross , "horms+renesas@verge.net.au" , "heiko@sntech.de" , "arnd@arndb.de" , "bjorn.andersson@linaro.org" , "jagan@amarulasolutions.com" , "enric.balletbo@collabora.com" , "marc.w.gonzalez@free.fr" , "olof@lixom.net" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-watchdog@vger.kernel.org" Cc: dl-linux-imx References: <1550472539-16590-1-git-send-email-Anson.Huang@nxp.com> <1550472539-16590-3-git-send-email-Anson.Huang@nxp.com> From: Guenter Roeck Message-ID: Date: Sat, 23 Feb 2019 19:29:00 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <1550472539-16590-3-git-send-email-Anson.Huang@nxp.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-watchdog-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-watchdog@vger.kernel.org On 2/17/19 10:53 PM, Anson Huang wrote: > i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller > inside, the system controller is in charge of controlling power, > clock and watchdog etc.. > > This patch adds i.MX system controller watchdog driver support, > watchdog operation needs to be done in secure EL3 mode via > ARM-Trusted-Firmware, using SMC call, CPU will trap into > ARM-Trusted-Firmware and then it will request system controller > to do watchdog operation via IPC. > > Signed-off-by: Anson Huang Sorry, but I have to take back my Reviewed-by: Comments below. Sorry, I completely missed that earlier. > --- > Changes since V1: > - use watchdog_active() instead of comparing watchdog status directly; > - use devm_kzalloc instead of using static variable; > - remove unnecessary wdog set driver data in .probe; > - use watchdog_stop_on_unregister() instead of .remove callback; > - use watchdog_stop_on_reboot() instead of .shutdown callback; > - use SIMPLE_DEV_PM_OPS() to simply the PM code. > --- > drivers/watchdog/Kconfig | 13 +++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/imx_sc_wdt.c | 185 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 199 insertions(+) > create mode 100644 drivers/watchdog/imx_sc_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 65c3c42..5c5b8ba 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -625,6 +625,19 @@ config IMX2_WDT > To compile this driver as a module, choose M here: the > module will be called imx2_wdt. > > +config IMX_SC_WDT > + tristate "IMX SC Watchdog" > + depends on ARCH_MXC || COMPILE_TEST > + select WATCHDOG_CORE > + help > + This is the driver for the system controller watchdog > + on the NXP i.MX SoCs with system controller inside. > + If you have one of these processors and wish to have > + watchdog support enabled, say Y, otherwise say N. > + > + To compile this driver as a module, choose M here: the > + module will be called imx_sc_wdt. > + > config UX500_WATCHDOG > tristate "ST-Ericsson Ux500 watchdog" > depends on MFD_DB8500_PRCMU > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 4e78a8c..0c9da63 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -68,6 +68,7 @@ obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o > obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o > obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o > obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o > +obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o > obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o > obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o > obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o > diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c > new file mode 100644 > index 0000000..a234a0c > --- /dev/null > +++ b/drivers/watchdog/imx_sc_wdt.c > @@ -0,0 +1,185 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright 2018-2019 NXP. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DEFAULT_TIMEOUT 60 > +/* > + * Software timer tick implemented in scfw side, support 10ms to 0xffffffff ms > + * in theory, but for normal case, 1s~128s is enough, you can change this max > + * value in case it's not enough. > + */ > +#define MAX_TIMEOUT 128 > + > +#define IMX_SIP_TIMER 0xC2000002 > +#define IMX_SIP_TIMER_START_WDOG 0x01 > +#define IMX_SIP_TIMER_STOP_WDOG 0x02 > +#define IMX_SIP_TIMER_SET_WDOG_ACT 0x03 > +#define IMX_SIP_TIMER_PING_WDOG 0x04 > +#define IMX_SIP_TIMER_SET_TIMEOUT_WDOG 0x05 > +#define IMX_SIP_TIMER_GET_WDOG_STAT 0x06 > +#define IMX_SIP_TIMER_SET_PRETIME_WDOG 0x07 > + > +#define SC_TIMER_WDOG_ACTION_PARTITION 0 > + > +static bool nowayout = WATCHDOG_NOWAYOUT; > +module_param(nowayout, bool, 0000); > +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); > + > +static unsigned int timeout = DEFAULT_TIMEOUT; Should not be initialized. > +module_param(timeout, uint, 0000); > +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default=" > + __MODULE_STRING(DEFAULT_TIMEOUT) ")"); > + This suggests that you want to use that timeout. But that isn't the case. > +static int imx_sc_wdt_ping(struct watchdog_device *wdog) > +{ > + struct arm_smccc_res res; > + > + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_PING_WDOG, > + 0, 0, 0, 0, 0, 0, &res); > + > + return res.a0; > +} > + > +static int imx_sc_wdt_start(struct watchdog_device *wdog) > +{ > + struct arm_smccc_res res; > + > + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_START_WDOG, > + 0, 0, 0, 0, 0, 0, &res); > + if (res.a0) > + return res.a0; > + > + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_SET_WDOG_ACT, > + SC_TIMER_WDOG_ACTION_PARTITION, > + 0, 0, 0, 0, 0, &res); > + return res.a0; > +} > + > +static int imx_sc_wdt_stop(struct watchdog_device *wdog) > +{ > + struct arm_smccc_res res; > + > + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_STOP_WDOG, > + 0, 0, 0, 0, 0, 0, &res); > + > + return res.a0; > +} > + > +static int imx_sc_wdt_set_timeout(struct watchdog_device *wdog, > + unsigned int timeout) > +{ > + struct arm_smccc_res res; > + > + wdog->timeout = timeout; > + arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_SET_TIMEOUT_WDOG, > + timeout * 1000, 0, 0, 0, 0, 0, &res); > + > + return res.a0; > +} > + > +static const struct watchdog_ops imx_sc_wdt_ops = { > + .owner = THIS_MODULE, > + .start = imx_sc_wdt_start, > + .stop = imx_sc_wdt_stop, > + .ping = imx_sc_wdt_ping, > + .set_timeout = imx_sc_wdt_set_timeout, > +}; > + > +static const struct watchdog_info imx_sc_wdt_info = { > + .identity = "i.MX SC watchdog timer", > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | > + WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT, > +}; > + > +static int imx_sc_wdt_probe(struct platform_device *pdev) > +{ > + struct watchdog_device *imx_sc_wdd; > + int ret; > + > + imx_sc_wdd = devm_kzalloc(&pdev->dev, sizeof(*imx_sc_wdd), GFP_KERNEL); > + if (!imx_sc_wdd) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, imx_sc_wdd); > + > + imx_sc_wdd->info = &imx_sc_wdt_info; > + imx_sc_wdd->ops = &imx_sc_wdt_ops; > + imx_sc_wdd->min_timeout = 1; > + imx_sc_wdd->max_timeout = MAX_TIMEOUT; > + imx_sc_wdd->parent = &pdev->dev; imx_sc_wdd->timeout = DEFAULT_TIMEOUT; > + > + ret = watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT, &pdev->dev); As written this doesn't make sense: It will set the timeout to the default, which is always valid, making the error check unnecessary. I assume you meant the above plus ret = watchdog_init_timeout(imx_sc_wdd, timeout, &pdev->dev); By not pre=initializing 'timeout', the watchdog core will try to read the configured timeout from devicetree. If it is not specified, it will keep the default timeout which should be set explicitly as suggested above. With that, the error check makes some sense, though it is unusual to bail out if an out-of-range timeout was specified. But that is your call. Again, sorry for not noticing this earlier. Guenter > + if (ret) { > + dev_err(&pdev->dev, "Failed to init the wdog timeout:%d\n", > + ret); > + return ret; > + } > + > + watchdog_stop_on_reboot(imx_sc_wdd); > + watchdog_stop_on_unregister(imx_sc_wdd); > + > + ret = devm_watchdog_register_device(&pdev->dev, imx_sc_wdd); > + if (ret) { > + dev_err(&pdev->dev, "Failed to register watchdog device\n"); > + return ret; > + } > + > + return 0; > +} > + > +static const struct of_device_id imx_sc_wdt_dt_ids[] = { > + { .compatible = "fsl,imx8qxp-sc-wdt", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, imx_sc_wdt_dt_ids); > + > +static int __maybe_unused imx_sc_wdt_suspend(struct device *dev) > +{ > + struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev); > + > + if (watchdog_active(imx_sc_wdd)) > + imx_sc_wdt_stop(imx_sc_wdd); > + > + return 0; > +} > + > +static int __maybe_unused imx_sc_wdt_resume(struct device *dev) > +{ > + struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev); > + > + if (watchdog_active(imx_sc_wdd)) > + imx_sc_wdt_start(imx_sc_wdd); > + > + return 0; > +} > + > +static SIMPLE_DEV_PM_OPS(imx_sc_wdt_pm_ops, > + imx_sc_wdt_suspend, imx_sc_wdt_resume); > + > +static struct platform_driver imx_sc_wdt_driver = { > + .probe = imx_sc_wdt_probe, > + .driver = { > + .name = "imx-sc-wdt", > + .of_match_table = imx_sc_wdt_dt_ids, > + .pm = &imx_sc_wdt_pm_ops, > + }, > +}; > + > +module_platform_driver(imx_sc_wdt_driver); > + > +MODULE_AUTHOR("Robin Gong "); > +MODULE_DESCRIPTION("NXP i.MX system controller watchdog driver"); > +MODULE_LICENSE("GPL v2"); >