Message ID | 20221114180843.1125308-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp2290517wru; Mon, 14 Nov 2022 10:16:21 -0800 (PST) X-Google-Smtp-Source: AA0mqf4q+2bTMEQrA3KAZ5FqGNIy9FRfBFvW6qdLAkFn7voqS9XN+DQW4Fgg+AYJXUFLyyupGofL X-Received: by 2002:a63:fb0e:0:b0:46f:13b0:8dab with SMTP id o14-20020a63fb0e000000b0046f13b08dabmr12680339pgh.497.1668449781662; Mon, 14 Nov 2022 10:16:21 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668449781; cv=none; d=google.com; s=arc-20160816; b=KMNzN3M++r01p7zc3lUNiafwQ3ptJfXFKKeaecV0JKl3JaFHG2/ZN2GJpz8aXBuBDj S6zvecUOjE2sKf3sc/atvpKfKYqbe7i60rDjTRdYNl3WVVOHAT9KIaTGKGpKBEFWlV/9 L+FncmLPAv0Ztz1Z8bfLFItCf9ZF6Hs3qaCvP9HTOFl3pyBgakBFGgZIxLkJ+9W6DFxd FuxrYPcWRGFCMsdZzbSVUEJFPN6DoWfICIXzdVyLpDrEF9CHWuiceXpRK8mdDU+FOFwM tWPhyRG82WNnMuD/3URkD3hEX0GyK6V7OnXt/cfsyoGYdv0Ib7CTuPCBYPsLcYYYUtp7 Kh7w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=izOCkbsm9IKKzQ3UFw4KGD8+VGBrn84t2S84ESpJtYY=; b=WYrN5ypxJKn8GPqTwpw6SEAm8i5UAc1xel7WiiCSIMWK3gvsURy2/veieIBgz5wETY 2yvLpxnoVY458DyhkksebHoCEjXX1T8seKajlxu2gNWp+rrAwmkItZWZUR36BMAqhDhN y/gmdoHFDqrb8nRVMKeQ1mASIwFjY6X+6HzRlZN0rHfhBFmpnc7yIRwTSlB+7+optMxN juYCWGmis82wGv+p/cIbCxjyKDArwYSs7w/MQqw5Dt6u0q8Uak7O9fAZKPeVcd1sSq2M 9d8BKrkGtI8a3vnf28bhP4VMad7fV2wV8O81WLbZRYhuQQYM+j5syxKG/p0szstGWl8w Rahw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=IO6lxxcT; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id p18-20020a170902e35200b00186881688f9si8726760plc.276.2022.11.14.10.16.08; Mon, 14 Nov 2022 10:16:21 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=IO6lxxcT; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238092AbiKNSJZ (ORCPT <rfc822;zwp10758@gmail.com> + 99 others); Mon, 14 Nov 2022 13:09:25 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39180 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238212AbiKNSJT (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 14 Nov 2022 13:09:19 -0500 Received: from mail-wm1-x336.google.com (mail-wm1-x336.google.com [IPv6:2a00:1450:4864:20::336]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BD9A8DF55; Mon, 14 Nov 2022 10:09:18 -0800 (PST) Received: by mail-wm1-x336.google.com with SMTP id 5so8054993wmo.1; Mon, 14 Nov 2022 10:09:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=izOCkbsm9IKKzQ3UFw4KGD8+VGBrn84t2S84ESpJtYY=; b=IO6lxxcT/1ZMxCQLSut5/xGerOoJDNos6w+RcB8b9iIJ1px/CAUH1YTtayE/0KKviD cZqKoJ+9B8XFMIfjbU2So3dJim8ABf2WWxgZBVcRJJuEDcjt9onD6+8ieQxKOiySG974 p03+dppDU7wmUTJ07ZzoyUTMhFyF7udjYYYD1eGJjZcm3PYFKleaHx5yTtduGIcwLrXg wDgeI9WKoM5lEkT/zrz/jOCZ8bbx7m2Aoyjytp+Zn/RLQ2sqalB07zkAx5q/7XE2gYvQ wrhzGXPwQfN0K4mAeccsrkZ9UM3p6978xUU/UgexKfE0jQXAG2UBfG0I7vXq5uNYo+1W 5H0w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=izOCkbsm9IKKzQ3UFw4KGD8+VGBrn84t2S84ESpJtYY=; b=kU76GsKgQIveVG4pL8PfbgAVcRjORBkdyxg9M0ssPSGkAZ5I+vTwgKe/t+eAOYPNV8 vHpNyIl0bsqiJlXYi4WeemzmEMug4tGcLx3EQHWkH7vNxAWggQAovaxXJ0IA0HkZoH7m JfNFt3Jc17Y5Y5ayKGEnwwOuKYx/ouESYRna0UtP8Ng2kes8bBxyNmDme82tYvyj5qXi kCJzqonqSqJhFBwNvhSwVU56mUminKLv1ABJrEQ21cZbduc0dnPgDVpj/XKRbopPnGPV vXyruJ9++BFLskYtG4ei9Z3e83TLhSEF9TNtGvpavUP5jQUWVWS/it9qG4b3H7oETxCN VFhw== X-Gm-Message-State: ANoB5pkbBs+5VoNRPe8qm3VymaJZG3cIa2U8h7xYZggRF/OU50A/c6IF nWpE2/eLDWDZyqExJ33BH+s= X-Received: by 2002:a05:600c:348e:b0:3cf:e0ef:4508 with SMTP id a14-20020a05600c348e00b003cfe0ef4508mr2865356wmq.84.1668449357162; Mon, 14 Nov 2022 10:09:17 -0800 (PST) Received: from prasmi.home ([2a00:23c8:2501:c701:ed66:fe24:8268:500]) by smtp.gmail.com with ESMTPSA id p33-20020a05600c1da100b003c71358a42dsm24380771wms.18.2022.11.14.10.09.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 14 Nov 2022 10:09:15 -0800 (PST) From: Prabhakar <prabhakar.csengg@gmail.com> X-Google-Original-From: Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> To: Geert Uytterhoeven <geert+renesas@glider.be>, Wim Van Sebroeck <wim@linux-watchdog.org>, Guenter Roeck <linux@roeck-us.net>, Philipp Zabel <p.zabel@pengutronix.de>, linux-watchdog@vger.kernel.org Cc: linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org, Prabhakar <prabhakar.csengg@gmail.com>, Biju Das <biju.das.jz@bp.renesas.com>, Fabrizio Castro <fabrizio.castro.jz@renesas.com>, Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Subject: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put the PM clocks Date: Mon, 14 Nov 2022 18:08:43 +0000 Message-Id: <20221114180843.1125308-1-prabhakar.mahadev-lad.rj@bp.renesas.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749496398162040271?= X-GMAIL-MSGID: =?utf-8?q?1749496398162040271?= |
Series |
watchdog: rzg2l_wdt: Issue a reset before we put the PM clocks
|
|
Commit Message
Lad, Prabhakar
Nov. 14, 2022, 6:08 p.m. UTC
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> On RZ/Five SoC it was observed that setting timeout (to say 1 sec) wouldn't reset the system. To fix this we make sure we issue a reset before putting the PM clocks to make sure the registers have been cleared. While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as we were calling the same functions here. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- Note, - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. - My initial investigation showed adding the delay after pm_runtime_get_sync() also fixed this issue. - Do I need add the fixes tag ? what should be the operation PUT->RESET/RESET->PUT? in case we need the tag is: Fixes: 4055ee81009e6 ("watchdog: rzg2l_wdt: Add set_timeout callback") --- drivers/watchdog/rzg2l_wdt.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
Comments
> -----Original Message----- > From: Prabhakar <prabhakar.csengg@gmail.com> > Sent: 14 November 2022 18:09 > To: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; Philipp Zabel > <p.zabel@pengutronix.de>; linux-watchdog@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; > Prabhakar <prabhakar.csengg@gmail.com>; Biju Das > <biju.das.jz@bp.renesas.com>; Fabrizio Castro > <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev- > lad.rj@bp.renesas.com> > Subject: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put the PM > clocks > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > On RZ/Five SoC it was observed that setting timeout (to say 1 sec) wouldn't > reset the system. To fix this we make sure we issue a reset before putting > the PM clocks to make sure the registers have been cleared. > > While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as we were > calling the same functions here. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > Note, > - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. > - My initial investigation showed adding the delay after > pm_runtime_get_sync() > also fixed this issue. > - Do I need add the fixes tag ? what should be the operation PUT- > >RESET/RESET->PUT? It looks like timing issue, None of the previous devices are affected by this. > in case we need the tag is: > Fixes: 4055ee81009e6 ("watchdog: rzg2l_wdt: Add set_timeout callback") > --- > drivers/watchdog/rzg2l_wdt.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c > index 00438ceed17a..d1271cc7750f 100644 > --- a/drivers/watchdog/rzg2l_wdt.c > +++ b/drivers/watchdog/rzg2l_wdt.c > @@ -115,16 +115,14 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev) > { > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > - pm_runtime_put(wdev->parent); > reset_control_reset(priv->rstc); > + pm_runtime_put(wdev->parent); > > return 0; > } > > static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int > timeout) { > - struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > - > wdev->timeout = timeout; > > /* > @@ -132,8 +130,7 @@ static int rzg2l_wdt_set_timeout(struct watchdog_device > *wdev, unsigned int time > * register so that it is updated with new timeout values. > */ Maybe update the comment above with new code change. Cheers, Biju > if (watchdog_active(wdev)) { > - pm_runtime_put(wdev->parent); > - reset_control_reset(priv->rstc); > + rzg2l_wdt_stop(wdev); > rzg2l_wdt_start(wdev); > } > > -- > 2.25.1
Hi Biju, On Mon, Nov 14, 2022 at 7:42 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > -----Original Message----- > > From: Prabhakar <prabhakar.csengg@gmail.com> > > Sent: 14 November 2022 18:09 > > To: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; Philipp Zabel > > <p.zabel@pengutronix.de>; linux-watchdog@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; > > Prabhakar <prabhakar.csengg@gmail.com>; Biju Das > > <biju.das.jz@bp.renesas.com>; Fabrizio Castro > > <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev- > > lad.rj@bp.renesas.com> > > Subject: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put the PM > > clocks > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > On RZ/Five SoC it was observed that setting timeout (to say 1 sec) wouldn't > > reset the system. To fix this we make sure we issue a reset before putting > > the PM clocks to make sure the registers have been cleared. > > > > While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as we were > > calling the same functions here. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > Note, > > - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. > > - My initial investigation showed adding the delay after > > pm_runtime_get_sync() > > also fixed this issue. > > - Do I need add the fixes tag ? what should be the operation PUT- > > >RESET/RESET->PUT? > > It looks like timing issue, None of the previous devices are affected by this. To me it looks like the device must be clocked for the reset signal to be propagated? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
> -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: 14 November 2022 19:04 > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Prabhakar <prabhakar.csengg@gmail.com>; Geert Uytterhoeven > <geert+renesas@glider.be>; Wim Van Sebroeck <wim@linux-watchdog.org>; Guenter > Roeck <linux@roeck-us.net>; Philipp Zabel <p.zabel@pengutronix.de>; linux- > watchdog@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas- > soc@vger.kernel.org; Fabrizio Castro <fabrizio.castro.jz@renesas.com>; > Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > Subject: Re: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put the PM > clocks > > Hi Biju, > > On Mon, Nov 14, 2022 at 7:42 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > -----Original Message----- > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > Sent: 14 November 2022 18:09 > > > To: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > > > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; > > > Philipp Zabel <p.zabel@pengutronix.de>; > > > linux-watchdog@vger.kernel.org > > > Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; > > > Prabhakar <prabhakar.csengg@gmail.com>; Biju Das > > > <biju.das.jz@bp.renesas.com>; Fabrizio Castro > > > <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad > > > <prabhakar.mahadev- lad.rj@bp.renesas.com> > > > Subject: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put > > > the PM clocks > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > On RZ/Five SoC it was observed that setting timeout (to say 1 sec) > > > wouldn't reset the system. To fix this we make sure we issue a reset > > > before putting the PM clocks to make sure the registers have been > cleared. > > > > > > While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as > > > we were calling the same functions here. > > > > > > Signed-off-by: Lad Prabhakar > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > Note, > > > - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. > > > - My initial investigation showed adding the delay after > > > pm_runtime_get_sync() > > > also fixed this issue. > > > - Do I need add the fixes tag ? what should be the operation PUT- > > > >RESET/RESET->PUT? > > > > It looks like timing issue, None of the previous devices are affected by > this. > > To me it looks like the device must be clocked for the reset signal to be > propagated? Yep, provide clk supply for a device, then apply reset. Cheers, Biju
HI Biju, Thank you for the review. On Mon, Nov 14, 2022 at 6:42 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > -----Original Message----- > > From: Prabhakar <prabhakar.csengg@gmail.com> > > Sent: 14 November 2022 18:09 > > To: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; Philipp Zabel > > <p.zabel@pengutronix.de>; linux-watchdog@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; > > Prabhakar <prabhakar.csengg@gmail.com>; Biju Das > > <biju.das.jz@bp.renesas.com>; Fabrizio Castro > > <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev- > > lad.rj@bp.renesas.com> > > Subject: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put the PM > > clocks > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > On RZ/Five SoC it was observed that setting timeout (to say 1 sec) wouldn't > > reset the system. To fix this we make sure we issue a reset before putting > > the PM clocks to make sure the registers have been cleared. > > > > While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as we were > > calling the same functions here. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > Note, > > - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. > > - My initial investigation showed adding the delay after > > pm_runtime_get_sync() > > also fixed this issue. > > - Do I need add the fixes tag ? what should be the operation PUT- > > >RESET/RESET->PUT? > > It looks like timing issue, None of the previous devices are affected by this. > yep. > > in case we need the tag is: > > Fixes: 4055ee81009e6 ("watchdog: rzg2l_wdt: Add set_timeout callback") > > --- > > drivers/watchdog/rzg2l_wdt.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c > > index 00438ceed17a..d1271cc7750f 100644 > > --- a/drivers/watchdog/rzg2l_wdt.c > > +++ b/drivers/watchdog/rzg2l_wdt.c > > @@ -115,16 +115,14 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev) > > { > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > - pm_runtime_put(wdev->parent); > > reset_control_reset(priv->rstc); > > + pm_runtime_put(wdev->parent); > > > > return 0; > > } > > > > static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int > > timeout) { > > - struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > - > > wdev->timeout = timeout; > > > > /* > > @@ -132,8 +130,7 @@ static int rzg2l_wdt_set_timeout(struct watchdog_device > > *wdev, unsigned int time > > * register so that it is updated with new timeout values. > > */ > > > Maybe update the comment above with new code change. > /* * If the watchdog is active, reset the module for updating the WDTSET * register so that it is updated with new timeout values. */ The above existing comment holds good with this code change. If you prefer something else please let me know I'll update it accordingly. Cheers, Prabhakar
> -----Original Message----- > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Sent: 14 November 2022 19:46 > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; Philipp > Zabel <p.zabel@pengutronix.de>; linux-watchdog@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Fabrizio > Castro <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad > <prabhakar.mahadev-lad.rj@bp.renesas.com> > Subject: Re: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put > the PM clocks > > HI Biju, > > Thank you for the review. > > On Mon, Nov 14, 2022 at 6:42 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > > > > > > > -----Original Message----- > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > Sent: 14 November 2022 18:09 > > > To: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > > > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; > > > Philipp Zabel <p.zabel@pengutronix.de>; > > > linux-watchdog@vger.kernel.org > > > Cc: linux-kernel@vger.kernel.org; linux-renesas- > soc@vger.kernel.org; > > > Prabhakar <prabhakar.csengg@gmail.com>; Biju Das > > > <biju.das.jz@bp.renesas.com>; Fabrizio Castro > > > <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad > > > <prabhakar.mahadev- lad.rj@bp.renesas.com> > > > Subject: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put > > > the PM clocks > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > On RZ/Five SoC it was observed that setting timeout (to say 1 sec) > > > wouldn't reset the system. To fix this we make sure we issue a > reset > > > before putting the PM clocks to make sure the registers have been > cleared. > > > > > > While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as > > > we were calling the same functions here. > > > > > > Signed-off-by: Lad Prabhakar > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > Note, > > > - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. > > > - My initial investigation showed adding the delay after > > > pm_runtime_get_sync() > > > also fixed this issue. > > > - Do I need add the fixes tag ? what should be the operation PUT- > > > >RESET/RESET->PUT? > > > > It looks like timing issue, None of the previous devices are > affected by this. > > > yep. > > > > in case we need the tag is: > > > Fixes: 4055ee81009e6 ("watchdog: rzg2l_wdt: Add set_timeout > > > callback") > > > --- > > > drivers/watchdog/rzg2l_wdt.c | 7 ++----- > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/watchdog/rzg2l_wdt.c > > > b/drivers/watchdog/rzg2l_wdt.c index 00438ceed17a..d1271cc7750f > > > 100644 > > > --- a/drivers/watchdog/rzg2l_wdt.c > > > +++ b/drivers/watchdog/rzg2l_wdt.c > > > @@ -115,16 +115,14 @@ static int rzg2l_wdt_stop(struct > > > watchdog_device *wdev) { > > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > > > - pm_runtime_put(wdev->parent); > > > reset_control_reset(priv->rstc); > > > + pm_runtime_put(wdev->parent); > > > > > > return 0; > > > } > > > > > > static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, > > > unsigned int > > > timeout) { > > > - struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > - > > > wdev->timeout = timeout; > > > > > > /* > > > @@ -132,8 +130,7 @@ static int rzg2l_wdt_set_timeout(struct > > > watchdog_device *wdev, unsigned int time > > > * register so that it is updated with new timeout values. > > > */ > > > > > > Maybe update the comment above with new code change. > > > /* > * If the watchdog is active, reset the module for updating the > WDTSET > * register so that it is updated with new timeout values. > */ > > The above existing comment holds good with this code change. If you > prefer something else please let me know I'll update it accordingly. Maybe mention, The resetting of the module is done in wdt_stop function. Cheers, Biju
On Mon, Nov 14, 2022 at 7:53 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > -----Original Message----- > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > Sent: 14 November 2022 19:46 > > To: Biju Das <biju.das.jz@bp.renesas.com> > > Cc: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; Philipp > > Zabel <p.zabel@pengutronix.de>; linux-watchdog@vger.kernel.org; linux- > > kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Fabrizio > > Castro <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Subject: Re: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put > > the PM clocks > > > > HI Biju, > > > > Thank you for the review. > > > > On Mon, Nov 14, 2022 at 6:42 PM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > > > > > > > > > > > -----Original Message----- > > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > > Sent: 14 November 2022 18:09 > > > > To: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > > > > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; > > > > Philipp Zabel <p.zabel@pengutronix.de>; > > > > linux-watchdog@vger.kernel.org > > > > Cc: linux-kernel@vger.kernel.org; linux-renesas- > > soc@vger.kernel.org; > > > > Prabhakar <prabhakar.csengg@gmail.com>; Biju Das > > > > <biju.das.jz@bp.renesas.com>; Fabrizio Castro > > > > <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad > > > > <prabhakar.mahadev- lad.rj@bp.renesas.com> > > > > Subject: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put > > > > the PM clocks > > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > On RZ/Five SoC it was observed that setting timeout (to say 1 sec) > > > > wouldn't reset the system. To fix this we make sure we issue a > > reset > > > > before putting the PM clocks to make sure the registers have been > > cleared. > > > > > > > > While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as > > > > we were calling the same functions here. > > > > > > > > Signed-off-by: Lad Prabhakar > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > --- > > > > Note, > > > > - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. > > > > - My initial investigation showed adding the delay after > > > > pm_runtime_get_sync() > > > > also fixed this issue. > > > > - Do I need add the fixes tag ? what should be the operation PUT- > > > > >RESET/RESET->PUT? > > > > > > It looks like timing issue, None of the previous devices are > > affected by this. > > > > > yep. > > > > > > in case we need the tag is: > > > > Fixes: 4055ee81009e6 ("watchdog: rzg2l_wdt: Add set_timeout > > > > callback") > > > > --- > > > > drivers/watchdog/rzg2l_wdt.c | 7 ++----- > > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/watchdog/rzg2l_wdt.c > > > > b/drivers/watchdog/rzg2l_wdt.c index 00438ceed17a..d1271cc7750f > > > > 100644 > > > > --- a/drivers/watchdog/rzg2l_wdt.c > > > > +++ b/drivers/watchdog/rzg2l_wdt.c > > > > @@ -115,16 +115,14 @@ static int rzg2l_wdt_stop(struct > > > > watchdog_device *wdev) { > > > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > > > > > - pm_runtime_put(wdev->parent); > > > > reset_control_reset(priv->rstc); > > > > + pm_runtime_put(wdev->parent); > > > > > > > > return 0; > > > > } > > > > > > > > static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, > > > > unsigned int > > > > timeout) { > > > > - struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > - > > > > wdev->timeout = timeout; > > > > > > > > /* > > > > @@ -132,8 +130,7 @@ static int rzg2l_wdt_set_timeout(struct > > > > watchdog_device *wdev, unsigned int time > > > > * register so that it is updated with new timeout values. > > > > */ > > > > > > > > > Maybe update the comment above with new code change. > > > > > /* > > * If the watchdog is active, reset the module for updating the > > WDTSET > > * register so that it is updated with new timeout values. > > */ > > > > The above existing comment holds good with this code change. If you > > prefer something else please let me know I'll update it accordingly. > > Maybe mention, The resetting of the module is done in wdt_stop function. > /* * If the watchdog is active, reset the module for updating the WDTSET * register by calling rzg2l_wdt_stop() (which internally calls reset_control_reset() and pm_runtime_put() * so that it is updated with new timeout values. */ Does the above look good? Cheers, Prabhakar
> -----Original Message----- > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Sent: 14 November 2022 19:56 > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; Philipp > Zabel <p.zabel@pengutronix.de>; linux-watchdog@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Fabrizio > Castro <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad > <prabhakar.mahadev-lad.rj@bp.renesas.com> > Subject: Re: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put > the PM clocks > > On Mon, Nov 14, 2022 at 7:53 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > > > > > > > -----Original Message----- > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > > Sent: 14 November 2022 19:46 > > > To: Biju Das <biju.das.jz@bp.renesas.com> > > > Cc: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > > > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; > > > Philipp Zabel <p.zabel@pengutronix.de>; > > > linux-watchdog@vger.kernel.org; linux- kernel@vger.kernel.org; > > > linux-renesas-soc@vger.kernel.org; Fabrizio Castro > > > <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > Subject: Re: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we > > > put the PM clocks > > > > > > HI Biju, > > > > > > Thank you for the review. > > > > > > On Mon, Nov 14, 2022 at 6:42 PM Biju Das > > > <biju.das.jz@bp.renesas.com> > > > wrote: > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > > > Sent: 14 November 2022 18:09 > > > > > To: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van > > > > > Sebroeck <wim@linux-watchdog.org>; Guenter Roeck > > > > > <linux@roeck-us.net>; Philipp Zabel <p.zabel@pengutronix.de>; > > > > > linux-watchdog@vger.kernel.org > > > > > Cc: linux-kernel@vger.kernel.org; linux-renesas- > > > soc@vger.kernel.org; > > > > > Prabhakar <prabhakar.csengg@gmail.com>; Biju Das > > > > > <biju.das.jz@bp.renesas.com>; Fabrizio Castro > > > > > <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad > > > > > <prabhakar.mahadev- lad.rj@bp.renesas.com> > > > > > Subject: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we > > > > > put the PM clocks > > > > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > On RZ/Five SoC it was observed that setting timeout (to say 1 > > > > > sec) wouldn't reset the system. To fix this we make sure we > > > > > issue a > > > reset > > > > > before putting the PM clocks to make sure the registers have > > > > > been > > > cleared. > > > > > > > > > > While at it re-used rzg2l_wdt_stop() in > rzg2l_wdt_set_timeout() > > > > > as we were calling the same functions here. > > > > > > > > > > Signed-off-by: Lad Prabhakar > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > --- > > > > > Note, > > > > > - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. > > > > > - My initial investigation showed adding the delay after > > > > > pm_runtime_get_sync() > > > > > also fixed this issue. > > > > > - Do I need add the fixes tag ? what should be the operation > > > > > PUT- > > > > > >RESET/RESET->PUT? > > > > > > > > It looks like timing issue, None of the previous devices are > > > affected by this. > > > > > > > yep. > > > > > > > > in case we need the tag is: > > > > > Fixes: 4055ee81009e6 ("watchdog: rzg2l_wdt: Add set_timeout > > > > > callback") > > > > > --- > > > > > drivers/watchdog/rzg2l_wdt.c | 7 ++----- > > > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/watchdog/rzg2l_wdt.c > > > > > b/drivers/watchdog/rzg2l_wdt.c index > 00438ceed17a..d1271cc7750f > > > > > 100644 > > > > > --- a/drivers/watchdog/rzg2l_wdt.c > > > > > +++ b/drivers/watchdog/rzg2l_wdt.c > > > > > @@ -115,16 +115,14 @@ static int rzg2l_wdt_stop(struct > > > > > watchdog_device *wdev) { > > > > > struct rzg2l_wdt_priv *priv = > watchdog_get_drvdata(wdev); > > > > > > > > > > - pm_runtime_put(wdev->parent); > > > > > reset_control_reset(priv->rstc); > > > > > + pm_runtime_put(wdev->parent); > > > > > > > > > > return 0; > > > > > } > > > > > > > > > > static int rzg2l_wdt_set_timeout(struct watchdog_device > *wdev, > > > > > unsigned int > > > > > timeout) { > > > > > - struct rzg2l_wdt_priv *priv = > watchdog_get_drvdata(wdev); > > > > > - > > > > > wdev->timeout = timeout; > > > > > > > > > > /* > > > > > @@ -132,8 +130,7 @@ static int rzg2l_wdt_set_timeout(struct > > > > > watchdog_device *wdev, unsigned int time > > > > > * register so that it is updated with new timeout > values. > > > > > */ > > > > > > > > > > > > Maybe update the comment above with new code change. > > > > > > > /* > > > * If the watchdog is active, reset the module for updating > the > > > WDTSET > > > * register so that it is updated with new timeout values. > > > */ > > > > > > The above existing comment holds good with this code change. If > you > > > prefer something else please let me know I'll update it > accordingly. > > > > Maybe mention, The resetting of the module is done in wdt_stop > function. > > > /* > * If the watchdog is active, reset the module for updating the > WDTSET > * register by calling rzg2l_wdt_stop() (which internally calls > reset_control_reset() and pm_runtime_put() (which internally calls reset_control_reset() to reset the module) > * so that it is updated with new timeout values. > */
On Mon, Nov 14, 2022 at 7:59 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > -----Original Message----- > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > Sent: 14 November 2022 19:56 > > To: Biju Das <biju.das.jz@bp.renesas.com> > > Cc: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; Philipp > > Zabel <p.zabel@pengutronix.de>; linux-watchdog@vger.kernel.org; linux- > > kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Fabrizio > > Castro <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Subject: Re: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put > > the PM clocks > > > > On Mon, Nov 14, 2022 at 7:53 PM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > > > > > > > > > > > -----Original Message----- > > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > > > Sent: 14 November 2022 19:46 > > > > To: Biju Das <biju.das.jz@bp.renesas.com> > > > > Cc: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > > > > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; > > > > Philipp Zabel <p.zabel@pengutronix.de>; > > > > linux-watchdog@vger.kernel.org; linux- kernel@vger.kernel.org; > > > > linux-renesas-soc@vger.kernel.org; Fabrizio Castro > > > > <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Subject: Re: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we > > > > put the PM clocks > > > > > > > > HI Biju, > > > > > > > > Thank you for the review. > > > > > > > > On Mon, Nov 14, 2022 at 6:42 PM Biju Das > > > > <biju.das.jz@bp.renesas.com> > > > > wrote: > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > > > > Sent: 14 November 2022 18:09 > > > > > > To: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van > > > > > > Sebroeck <wim@linux-watchdog.org>; Guenter Roeck > > > > > > <linux@roeck-us.net>; Philipp Zabel <p.zabel@pengutronix.de>; > > > > > > linux-watchdog@vger.kernel.org > > > > > > Cc: linux-kernel@vger.kernel.org; linux-renesas- > > > > soc@vger.kernel.org; > > > > > > Prabhakar <prabhakar.csengg@gmail.com>; Biju Das > > > > > > <biju.das.jz@bp.renesas.com>; Fabrizio Castro > > > > > > <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad > > > > > > <prabhakar.mahadev- lad.rj@bp.renesas.com> > > > > > > Subject: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we > > > > > > put the PM clocks > > > > > > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > > > On RZ/Five SoC it was observed that setting timeout (to say 1 > > > > > > sec) wouldn't reset the system. To fix this we make sure we > > > > > > issue a > > > > reset > > > > > > before putting the PM clocks to make sure the registers have > > > > > > been > > > > cleared. > > > > > > > > > > > > While at it re-used rzg2l_wdt_stop() in > > rzg2l_wdt_set_timeout() > > > > > > as we were calling the same functions here. > > > > > > > > > > > > Signed-off-by: Lad Prabhakar > > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > --- > > > > > > Note, > > > > > > - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. > > > > > > - My initial investigation showed adding the delay after > > > > > > pm_runtime_get_sync() > > > > > > also fixed this issue. > > > > > > - Do I need add the fixes tag ? what should be the operation > > > > > > PUT- > > > > > > >RESET/RESET->PUT? > > > > > > > > > > It looks like timing issue, None of the previous devices are > > > > affected by this. > > > > > > > > > yep. > > > > > > > > > > in case we need the tag is: > > > > > > Fixes: 4055ee81009e6 ("watchdog: rzg2l_wdt: Add set_timeout > > > > > > callback") > > > > > > --- > > > > > > drivers/watchdog/rzg2l_wdt.c | 7 ++----- > > > > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/drivers/watchdog/rzg2l_wdt.c > > > > > > b/drivers/watchdog/rzg2l_wdt.c index > > 00438ceed17a..d1271cc7750f > > > > > > 100644 > > > > > > --- a/drivers/watchdog/rzg2l_wdt.c > > > > > > +++ b/drivers/watchdog/rzg2l_wdt.c > > > > > > @@ -115,16 +115,14 @@ static int rzg2l_wdt_stop(struct > > > > > > watchdog_device *wdev) { > > > > > > struct rzg2l_wdt_priv *priv = > > watchdog_get_drvdata(wdev); > > > > > > > > > > > > - pm_runtime_put(wdev->parent); > > > > > > reset_control_reset(priv->rstc); > > > > > > + pm_runtime_put(wdev->parent); > > > > > > > > > > > > return 0; > > > > > > } > > > > > > > > > > > > static int rzg2l_wdt_set_timeout(struct watchdog_device > > *wdev, > > > > > > unsigned int > > > > > > timeout) { > > > > > > - struct rzg2l_wdt_priv *priv = > > watchdog_get_drvdata(wdev); > > > > > > - > > > > > > wdev->timeout = timeout; > > > > > > > > > > > > /* > > > > > > @@ -132,8 +130,7 @@ static int rzg2l_wdt_set_timeout(struct > > > > > > watchdog_device *wdev, unsigned int time > > > > > > * register so that it is updated with new timeout > > values. > > > > > > */ > > > > > > > > > > > > > > > Maybe update the comment above with new code change. > > > > > > > > > /* > > > > * If the watchdog is active, reset the module for updating > > the > > > > WDTSET > > > > * register so that it is updated with new timeout values. > > > > */ > > > > > > > > The above existing comment holds good with this code change. If > > you > > > > prefer something else please let me know I'll update it > > accordingly. > > > > > > Maybe mention, The resetting of the module is done in wdt_stop > > function. > > > > > /* > > * If the watchdog is active, reset the module for updating the > > WDTSET > > * register by calling rzg2l_wdt_stop() (which internally calls > > reset_control_reset() and pm_runtime_put() > > (which internally calls reset_control_reset() to reset the module) > Sure will update that in v2. Cheers, Prabhakar
Hi Geert, On Mon, Nov 14, 2022 at 7:03 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Biju, > > On Mon, Nov 14, 2022 at 7:42 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > -----Original Message----- > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > Sent: 14 November 2022 18:09 > > > To: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > > > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; Philipp Zabel > > > <p.zabel@pengutronix.de>; linux-watchdog@vger.kernel.org > > > Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; > > > Prabhakar <prabhakar.csengg@gmail.com>; Biju Das > > > <biju.das.jz@bp.renesas.com>; Fabrizio Castro > > > <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev- > > > lad.rj@bp.renesas.com> > > > Subject: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put the PM > > > clocks > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > On RZ/Five SoC it was observed that setting timeout (to say 1 sec) wouldn't > > > reset the system. To fix this we make sure we issue a reset before putting > > > the PM clocks to make sure the registers have been cleared. > > > > > > While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as we were > > > calling the same functions here. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > Note, > > > - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. > > > - My initial investigation showed adding the delay after > > > pm_runtime_get_sync() > > > also fixed this issue. > > > - Do I need add the fixes tag ? what should be the operation PUT- > > > >RESET/RESET->PUT? > > > > It looks like timing issue, None of the previous devices are affected by this. > > To me it looks like the device must be clocked for the reset signal > to be propagated? > In the HW manual (7.4.3 Procedure for Activating Modules) it does state the below before applying the reset signal, Set up the clock control register for the clock signal connected to the target module to start the supply of the clock. Note that the PLL for the clock should be started before the clock if the PLL is stopped. So maybe I can add the fixes tag in v2. Cheers, Prabhakar
> Subject: RE: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put > the PM clocks > > > > > -----Original Message----- > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > Sent: 14 November 2022 19:04 > > To: Biju Das <biju.das.jz@bp.renesas.com> > > Cc: Prabhakar <prabhakar.csengg@gmail.com>; Geert Uytterhoeven > > <geert+renesas@glider.be>; Wim Van Sebroeck <wim@linux- > watchdog.org>; > > Guenter Roeck <linux@roeck-us.net>; Philipp Zabel > > <p.zabel@pengutronix.de>; linux- watchdog@vger.kernel.org; > > linux-kernel@vger.kernel.org; linux-renesas- soc@vger.kernel.org; > > Fabrizio Castro <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev > > Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Subject: Re: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we > put > > the PM clocks > > > > Hi Biju, > > > > On Mon, Nov 14, 2022 at 7:42 PM Biju Das > <biju.das.jz@bp.renesas.com> wrote: > > > > -----Original Message----- > > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > > Sent: 14 November 2022 18:09 > > > > To: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van > Sebroeck > > > > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; > > > > Philipp Zabel <p.zabel@pengutronix.de>; > > > > linux-watchdog@vger.kernel.org > > > > Cc: linux-kernel@vger.kernel.org; > > > > linux-renesas-soc@vger.kernel.org; > > > > Prabhakar <prabhakar.csengg@gmail.com>; Biju Das > > > > <biju.das.jz@bp.renesas.com>; Fabrizio Castro > > > > <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad > > > > <prabhakar.mahadev- lad.rj@bp.renesas.com> > > > > Subject: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we > put > > > > the PM clocks > > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > On RZ/Five SoC it was observed that setting timeout (to say 1 > sec) > > > > wouldn't reset the system. To fix this we make sure we issue a > > > > reset before putting the PM clocks to make sure the registers > have > > > > been > > cleared. > > > > > > > > While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() > as > > > > we were calling the same functions here. > > > > > > > > Signed-off-by: Lad Prabhakar > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > --- > > > > Note, > > > > - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. > > > > - My initial investigation showed adding the delay after > > > > pm_runtime_get_sync() > > > > also fixed this issue. > > > > - Do I need add the fixes tag ? what should be the operation > PUT- > > > > >RESET/RESET->PUT? > > > > > > It looks like timing issue, None of the previous devices are > > > affected by > > this. > > > > To me it looks like the device must be clocked for the reset signal > to > > be propagated? > > Yep, provide clk supply for a device, then apply reset. Maybe we need to make it consistent by taking care of [1] Current patch: CLK ON -> apply Reset for V2M. [1]: Apply Reset -> CLK ON for V2M. [1] https://lore.kernel.org/linux-renesas-soc/CAMuHMdUWbT6VArm9B56VE0yUYWCTm=3vMGrrONSv9cdsQQnhpg@mail.gmail.com/T/#mdb78503524a8f4207f59a40f7ff573e210656988 Cheers, Biju
Hi Biju, On Tue, Nov 15, 2022 at 8:48 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > -----Original Message----- > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > On Mon, Nov 14, 2022 at 7:42 PM Biju Das > > <biju.das.jz@bp.renesas.com> wrote: > > > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > > > On RZ/Five SoC it was observed that setting timeout (to say 1 > > sec) > > > > > wouldn't reset the system. To fix this we make sure we issue a > > > > > reset before putting the PM clocks to make sure the registers > > have > > > > > been > > > cleared. > > > > > > > > > > While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() > > as > > > > > we were calling the same functions here. > > > > > > > > > > Signed-off-by: Lad Prabhakar > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > --- > > > > > Note, > > > > > - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. > > > > > - My initial investigation showed adding the delay after > > > > > pm_runtime_get_sync() > > > > > also fixed this issue. > > > > > - Do I need add the fixes tag ? what should be the operation > > PUT- > > > > > >RESET/RESET->PUT? > > > > > > > > It looks like timing issue, None of the previous devices are > > > > affected by > > > this. > > > > > > To me it looks like the device must be clocked for the reset signal > > to > > > be propagated? > > > > Yep, provide clk supply for a device, then apply reset. > > Maybe we need to make it consistent by taking care of [1] > > Current patch: CLK ON -> apply Reset for V2M. > [1]: Apply Reset -> CLK ON for V2M. Yes, that would also simplify that patch: just add the call to reset? > [1] https://lore.kernel.org/linux-renesas-soc/CAMuHMdUWbT6VArm9B56VE0yUYWCTm=3vMGrrONSv9cdsQQnhpg@mail.gmail.com/T/#mdb78503524a8f4207f59a40f7ff573e210656988 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
> -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: 15 November 2022 08:11 > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Prabhakar <prabhakar.csengg@gmail.com>; Wim Van Sebroeck > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; Philipp > Zabel <p.zabel@pengutronix.de>; linux-watchdog@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Fabrizio > Castro <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad > <prabhakar.mahadev-lad.rj@bp.renesas.com> > Subject: Re: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put > the PM clocks > > Hi Biju, > > On Tue, Nov 15, 2022 at 8:48 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > > -----Original Message----- > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, Nov 14, > > > > 2022 at 7:42 PM Biju Das > > > <biju.das.jz@bp.renesas.com> wrote: > > > > > > From: Prabhakar <prabhakar.csengg@gmail.com> On RZ/Five SoC > it > > > > > > was observed that setting timeout (to say 1 > > > sec) > > > > > > wouldn't reset the system. To fix this we make sure we issue > a > > > > > > reset before putting the PM clocks to make sure the > registers > > > have > > > > > > been > > > > cleared. > > > > > > > > > > > > While at it re-used rzg2l_wdt_stop() in > > > > > > rzg2l_wdt_set_timeout() > > > as > > > > > > we were calling the same functions here. > > > > > > > > > > > > Signed-off-by: Lad Prabhakar > > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > --- > > > > > > Note, > > > > > > - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. > > > > > > - My initial investigation showed adding the delay after > > > > > > pm_runtime_get_sync() > > > > > > also fixed this issue. > > > > > > - Do I need add the fixes tag ? what should be the operation > > > PUT- > > > > > > >RESET/RESET->PUT? > > > > > > > > > > It looks like timing issue, None of the previous devices are > > > > > affected by > > > > this. > > > > > > > > To me it looks like the device must be clocked for the reset > > > > signal > > > to > > > > be propagated? > > > > > > Yep, provide clk supply for a device, then apply reset. > > > > Maybe we need to make it consistent by taking care of [1] > > > > Current patch: CLK ON -> apply Reset for V2M. > > [1]: Apply Reset -> CLK ON for V2M. > > Yes, that would also simplify that patch: just add the call to reset? Fabrizio previously told me, CLK ON -> apply Reset does not work for RZ/V2M reboot use case. Regards, Biju
diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index 00438ceed17a..d1271cc7750f 100644 --- a/drivers/watchdog/rzg2l_wdt.c +++ b/drivers/watchdog/rzg2l_wdt.c @@ -115,16 +115,14 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev) { struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); - pm_runtime_put(wdev->parent); reset_control_reset(priv->rstc); + pm_runtime_put(wdev->parent); return 0; } static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout) { - struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); - wdev->timeout = timeout; /* @@ -132,8 +130,7 @@ static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int time * register so that it is updated with new timeout values. */ if (watchdog_active(wdev)) { - pm_runtime_put(wdev->parent); - reset_control_reset(priv->rstc); + rzg2l_wdt_stop(wdev); rzg2l_wdt_start(wdev); }