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.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,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 9295EC43603 for ; Wed, 11 Dec 2019 16:45:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6ED8524656 for ; Wed, 11 Dec 2019 16:45:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730779AbfLKQpF (ORCPT ); Wed, 11 Dec 2019 11:45:05 -0500 Received: from metis.ext.pengutronix.de ([85.220.165.71]:55755 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730315AbfLKQpF (ORCPT ); Wed, 11 Dec 2019 11:45:05 -0500 Received: from lupine.hi.pengutronix.de ([2001:67c:670:100:3ad5:47ff:feaf:1a17] helo=lupine) by metis.ext.pengutronix.de with esmtp (Exim 4.92) (envelope-from ) id 1if56e-0003fN-Rr; Wed, 11 Dec 2019 17:44:56 +0100 Message-ID: Subject: Re: [PATCH v6 2/2] watchdog: mtk_wdt: mt8183: Add reset controller From: Philipp Zabel To: Jiaxin Yu , yong.liang@mediatek.com, wim@linux-watchdog.org, linux@roeck-us.net, matthias.bgg@gmail.com, linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org Cc: yingjoe.chen@mediatek.com, sboyd@kernel.org Date: Wed, 11 Dec 2019 17:44:55 +0100 In-Reply-To: <1576081356-18298-3-git-send-email-jiaxin.yu@mediatek.com> References: <1576081356-18298-1-git-send-email-jiaxin.yu@mediatek.com> <1576081356-18298-3-git-send-email-jiaxin.yu@mediatek.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.30.5-1.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-SA-Exim-Connect-IP: 2001:67c:670:100:3ad5:47ff:feaf:1a17 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-watchdog@vger.kernel.org Sender: linux-watchdog-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-watchdog@vger.kernel.org Hi, On Thu, 2019-12-12 at 00:22 +0800, Jiaxin Yu wrote: > From: "yong.liang" > > Add reset controller API in watchdog driver. > Besides watchdog, MTK toprgu module alsa provide sub-system (eg, audio, > camera, codec and connectivity) software reset functionality. Do any of the listed sub-systems use the reset_control_reset() functionality? Is there no delay requirement between assert and deassert? Otherwise it would be safer not to implement the .reset() operation at all. > > Signed-off-by: yong.liang > --- > drivers/watchdog/Kconfig | 1 + > drivers/watchdog/mtk_wdt.c | 109 ++++++++++++++++++++++++++++++++++++- > 2 files changed, 109 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 2e07caab9db2..629249fe5305 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG > tristate "Mediatek SoCs watchdog support" > depends on ARCH_MEDIATEK || COMPILE_TEST > select WATCHDOG_CORE > + select RESET_CONTROLLER > help > Say Y here to include support for the watchdog timer > in Mediatek SoCs. > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c > index 9c3d0033260d..667380031dfd 100644 > --- a/drivers/watchdog/mtk_wdt.c > +++ b/drivers/watchdog/mtk_wdt.c > @@ -9,6 +9,9 @@ > * Based on sunxi_wdt.c > */ > > +#include > +#include > +#include > #include > #include > #include > @@ -16,10 +19,12 @@ > #include > #include > #include > +#include > #include > +#include > +#include What is this required for? > #include > #include > -#include > > #define WDT_MAX_TIMEOUT 31 > #define WDT_MIN_TIMEOUT 1 > @@ -44,6 +49,9 @@ > #define WDT_SWRST 0x14 > #define WDT_SWRST_KEY 0x1209 > > +#define WDT_SWSYSRST 0x18U > +#define WDT_SWSYS_RST_KEY 0x88000000 > + > #define DRV_NAME "mtk-wdt" > #define DRV_VERSION "1.0" > > @@ -53,8 +61,97 @@ static unsigned int timeout; > struct mtk_wdt_dev { > struct watchdog_device wdt_dev; > void __iomem *wdt_base; > + spinlock_t lock; /* protects WDT_SWSYSRST reg */ > + struct reset_controller_dev rcdev; > +}; > + > +struct mtk_wdt_data { > + int infracfg_sw_rst_num; This is not used at all, better remove it. > + int toprgu_sw_rst_num; > +}; > + > +static const struct mtk_wdt_data mt2712_data = { > + .toprgu_sw_rst_num = MT2712_TOPRGU_SW_RST_NUM, > +}; > + > +static const struct mtk_wdt_data mt8183_data = { > + .infracfg_sw_rst_num = MT8183_INFRACFG_SW_RST_NUM, Same as above. > + .toprgu_sw_rst_num = MT8183_TOPRGU_SW_RST_NUM, > +}; > + > +static int toprgu_reset_update(struct reset_controller_dev *rcdev, > + unsigned long id, bool assert) > +{ > + unsigned int tmp; > + unsigned long flags; > + This empty line can be removed. > + struct mtk_wdt_dev *data = > + container_of(rcdev, struct mtk_wdt_dev, rcdev); > + > + spin_lock_irqsave(&data->lock, flags); > + > + tmp = readl(data->wdt_base + WDT_SWSYSRST); > + if (assert) > + tmp |= BIT(id); > + else > + tmp &= ~BIT(id); > + tmp |= WDT_SWSYS_RST_KEY; > + writel(tmp, data->wdt_base + WDT_SWSYSRST); > + > + spin_unlock_irqrestore(&data->lock, flags); > + > + return 0; > +} > + > +static int toprgu_reset_assert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return toprgu_reset_update(rcdev, id, true); > +} > + > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return toprgu_reset_update(rcdev, id, false); > +} > + > +static int toprgu_reset(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + int ret; > + > + ret = toprgu_reset_assert(rcdev, id); > + if (ret) > + return ret; > + > + return toprgu_reset_deassert(rcdev, id); > +} As mentioned above, is this needed? Does this work for all modules? Only implement this if you are sure both are true. regards Philipp