Message ID | 20230801162731.3278396-1-a.fatoum@pengutronix.de |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:918b:0:b0:3e4:2afc:c1 with SMTP id s11csp2815665vqg; Tue, 1 Aug 2023 10:16:58 -0700 (PDT) X-Google-Smtp-Source: APBJJlEHLABkmHoDqcaf5zNWPPN3PNGrgbdScunKykd+L/8n6DayCSXSjOVYjexZ7v7Qni4TqVrj X-Received: by 2002:a17:90a:9281:b0:268:5b19:c35e with SMTP id n1-20020a17090a928100b002685b19c35emr16487511pjo.6.1690910218253; Tue, 01 Aug 2023 10:16:58 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690910218; cv=none; d=google.com; s=arc-20160816; b=OzR0Iid60P0STaNoZGAcOxF6qKGNsUYrB+qgQT8Sp4gjm+Q/onrmEe+PWAFEx3qpEH miaUlatraln8FeDriOdzAKapowB5jtf5+FAd6td42HI5UZDLqfIszIUD4c3bb4/Su9jN UiF8gLCGu3ktMIREbB6PUM+aKzpYpaUESM3KkeuE21RdmbymDxH+qckjby1Nd9vYoViA qSxORBjnGd4QAGPrcZV69v6dZwmbHrZ6hlS98eNWNCW/q26Z0AK3LqCzADZsCKdD2yHR qYAOf2nbMXfjyRiCthPdU5IYTqyEwHmGauxLqlrsyTMzCqucB8e7xD7tX4s+9jWqiruV ElWw== 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; bh=qgcIY3ZGUA3SQSIGRAfbML0uq/9wC//36ldOQaa7HvM=; fh=SWEHQcBRCa6tI6IeOBY89k68XTqAL8eiUakPFKEYqdo=; b=hynlF9PJR2YNbcSpUHWU8AHTnD2WhFES5QA8CsREpvssJ20/U00NpVQLDsuRfOFjdM LiUzFnstoHQZEyTVC8c1E+NnJSLcPZO9o39zjjS+sV5fl/WeLSCQcsZOzXdf6qWKwz17 iN77Tf0IQYbqFtpl36dy8d9zdKnjzdITpvixbSPZD+pLtBuA1OUA/uf3SdmFc9EfEibX JlHbX+FLpbsbnq8zeovOTvwN91mtKFYxn0CnkrDgPsJQo8uvpGbwZC5i5cdS8uNDc0+i 1MSeMf/viNARIBvzWmMNhokbqoFMsEHvOzvsVN9sErZDuOBrZWxsBmXNdbUz2xTy1WA8 sqIg== ARC-Authentication-Results: i=1; mx.google.com; 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 Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id s20-20020a632c14000000b0055c96379d00si7488882pgs.710.2023.08.01.10.16.43; Tue, 01 Aug 2023 10:16:58 -0700 (PDT) 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; 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 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229558AbjHAQ2K (ORCPT <rfc822;maxi.paulin@gmail.com> + 99 others); Tue, 1 Aug 2023 12:28:10 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54304 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229457AbjHAQ2I (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 1 Aug 2023 12:28:08 -0400 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 79CD9133 for <linux-kernel@vger.kernel.org>; Tue, 1 Aug 2023 09:28:07 -0700 (PDT) Received: from drehscheibe.grey.stw.pengutronix.de ([2a0a:edc0:0:c01:1d::a2]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from <afa@pengutronix.de>) id 1qQsDv-00018q-Q3; Tue, 01 Aug 2023 18:27:51 +0200 Received: from [2a0a:edc0:0:1101:1d::54] (helo=dude05.red.stw.pengutronix.de) by drehscheibe.grey.stw.pengutronix.de with esmtp (Exim 4.94.2) (envelope-from <afa@pengutronix.de>) id 1qQsDu-000QYa-SP; Tue, 01 Aug 2023 18:27:50 +0200 Received: from afa by dude05.red.stw.pengutronix.de with local (Exim 4.96) (envelope-from <afa@pengutronix.de>) id 1qQsDt-00Dkrt-2z; Tue, 01 Aug 2023 18:27:49 +0200 From: Ahmad Fatoum <a.fatoum@pengutronix.de> To: Abel Vesa <abelvesa@kernel.org>, Peng Fan <peng.fan@nxp.com>, Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Shawn Guo <shawnguo@kernel.org>, Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix Kernel Team <kernel@pengutronix.de>, Fabio Estevam <festevam@gmail.com>, NXP Linux Team <linux-imx@nxp.com> Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: [PATCH] clk: imx: composite-8m: avoid glitches when set_rate would be a no-op Date: Tue, 1 Aug 2023 18:27:31 +0200 Message-Id: <20230801162731.3278396-1-a.fatoum@pengutronix.de> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SA-Exim-Connect-IP: 2a0a:edc0:0:c01:1d::a2 X-SA-Exim-Mail-From: afa@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable 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: INBOX X-GMAIL-THRID: 1773047872647495019 X-GMAIL-MSGID: 1773047872647495019 |
Series |
clk: imx: composite-8m: avoid glitches when set_rate would be a no-op
|
|
Commit Message
Ahmad Fatoum
Aug. 1, 2023, 4:27 p.m. UTC
Reconfiguring the clock divider to the exact same value is observed
on an i.MX8MN to often cause a short clock pause, probably because
the divider restarts counting from the time the register is written.
This issue doesn't show up normally, because the clock framework will
take care to not call set_rate when the clock rate is the same.
However, when we configure an upstream clock (e.g. an audio_pll), the
common code will call set_rate with the newly calculated rate on all
children. As the new rate is different, we enter set_rate and compute
the same divider values, write them back and cause the glitch (e.g.
on a SAI's MCLK).
To avoid the glitch, we skip writing the same value back again.
Fixes: d3ff9728134e ("clk: imx: Add imx composite clock")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/clk/imx/clk-composite-8m.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
Comments
On 23-08-01, Ahmad Fatoum wrote: > Reconfiguring the clock divider to the exact same value is observed > on an i.MX8MN to often cause a short clock pause, probably because > the divider restarts counting from the time the register is written. > > This issue doesn't show up normally, because the clock framework will > take care to not call set_rate when the clock rate is the same. > However, when we configure an upstream clock (e.g. an audio_pll), the > common code will call set_rate with the newly calculated rate on all > children. As the new rate is different, we enter set_rate and compute > the same divider values, write them back and cause the glitch (e.g. > on a SAI's MCLK). > > To avoid the glitch, we skip writing the same value back again. > > Fixes: d3ff9728134e ("clk: imx: Add imx composite clock") > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
On 8/2/2023 12:27 AM, Ahmad Fatoum wrote: > Reconfiguring the clock divider to the exact same value is observed > on an i.MX8MN to often cause a short clock pause, probably because > the divider restarts counting from the time the register is written. > > This issue doesn't show up normally, because the clock framework will > take care to not call set_rate when the clock rate is the same. > However, when we configure an upstream clock (e.g. an audio_pll), the > common code will call set_rate with the newly calculated rate on all > children. As the new rate is different, we enter set_rate and compute > the same divider values, write them back and cause the glitch (e.g. > on a SAI's MCLK). The CCM root has glitch-free mux. When upstream pll freq change, the child set rate will also touch the mux bit, since div and mux in one register, so the mux logic will also function. Per design, it is glitch free, so I not understand well why glitch. When you configure pll, the downstream sai clk should still not be enabled, right? Thanks, Peng. > > To avoid the glitch, we skip writing the same value back again. > > Fixes: d3ff9728134e ("clk: imx: Add imx composite clock") > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > --- > drivers/clk/imx/clk-composite-8m.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c > index cbf0d7955a00..3e9a092e136c 100644 > --- a/drivers/clk/imx/clk-composite-8m.c > +++ b/drivers/clk/imx/clk-composite-8m.c > @@ -97,7 +97,7 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, > int prediv_value; > int div_value; > int ret; > - u32 val; > + u32 orig, val; > > ret = imx8m_clk_composite_compute_dividers(rate, parent_rate, > &prediv_value, &div_value); > @@ -106,13 +106,15 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, > > spin_lock_irqsave(divider->lock, flags); > > - val = readl(divider->reg); > - val &= ~((clk_div_mask(divider->width) << divider->shift) | > - (clk_div_mask(PCG_DIV_WIDTH) << PCG_DIV_SHIFT)); > + orig = readl(divider->reg); > + val = orig & ~((clk_div_mask(divider->width) << divider->shift) | > + (clk_div_mask(PCG_DIV_WIDTH) << PCG_DIV_SHIFT)); > > val |= (u32)(prediv_value - 1) << divider->shift; > val |= (u32)(div_value - 1) << PCG_DIV_SHIFT; > - writel(val, divider->reg); > + > + if (val != orig) > + writel(val, divider->reg); > > spin_unlock_irqrestore(divider->lock, flags); >
Hello Peng, On 02.08.23 03:25, Peng Fan wrote: > > > On 8/2/2023 12:27 AM, Ahmad Fatoum wrote: >> Reconfiguring the clock divider to the exact same value is observed >> on an i.MX8MN to often cause a short clock pause, probably because >> the divider restarts counting from the time the register is written. >> >> This issue doesn't show up normally, because the clock framework will >> take care to not call set_rate when the clock rate is the same. >> However, when we configure an upstream clock (e.g. an audio_pll), the >> common code will call set_rate with the newly calculated rate on all >> children. As the new rate is different, we enter set_rate and compute >> the same divider values, write them back and cause the glitch (e.g. >> on a SAI's MCLK). > > > The CCM root has glitch-free mux. When upstream pll freq change, > the child set rate will also touch the mux bit, since div and mux > in one register, so the mux logic will also function. > > Per design, it is glitch free, so I not understand well why glitch. > > When you configure pll, the downstream sai clk should still not be enabled, right? - sai5 is running normally and divides Audio PLL out by 16. - audio_pll1 is increased by 32 Hz -> only kdiv changes, so no glitch - imx8m_clk_composite_divider_set_rate(sai5) is called with 32 / 16 = 2 Hz more - imx8m_clk_composite_divider_set_rate computes same divider as before and writes register - divider starts counting from zero, so we have a longer clock pause than usual, e.g. 40ns -> 125ns, external MCLK consumer doesn't like that at all. So it's not a glitch in the transient high frequency sense, but rather a transient low frequency period. I can reword the commit message to s/glitch/clock pause/ if you prefer. And yes, if mux is switched, we will probably get the same clock pause, but that is not a problem for me currently, because we don't switch parents except at boot up. Afterwards, only PLL is tuned. Cheers, Ahmad > > Thanks, > Peng. > >> >> To avoid the glitch, we skip writing the same value back again. >> >> Fixes: d3ff9728134e ("clk: imx: Add imx composite clock") >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >> --- >> drivers/clk/imx/clk-composite-8m.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c >> index cbf0d7955a00..3e9a092e136c 100644 >> --- a/drivers/clk/imx/clk-composite-8m.c >> +++ b/drivers/clk/imx/clk-composite-8m.c >> @@ -97,7 +97,7 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, >> int prediv_value; >> int div_value; >> int ret; >> - u32 val; >> + u32 orig, val; >> ret = imx8m_clk_composite_compute_dividers(rate, parent_rate, >> &prediv_value, &div_value); >> @@ -106,13 +106,15 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, >> spin_lock_irqsave(divider->lock, flags); >> - val = readl(divider->reg); >> - val &= ~((clk_div_mask(divider->width) << divider->shift) | >> - (clk_div_mask(PCG_DIV_WIDTH) << PCG_DIV_SHIFT)); >> + orig = readl(divider->reg); >> + val = orig & ~((clk_div_mask(divider->width) << divider->shift) | >> + (clk_div_mask(PCG_DIV_WIDTH) << PCG_DIV_SHIFT)); >> val |= (u32)(prediv_value - 1) << divider->shift; >> val |= (u32)(div_value - 1) << PCG_DIV_SHIFT; >> - writel(val, divider->reg); >> + >> + if (val != orig) >> + writel(val, divider->reg); >> spin_unlock_irqrestore(divider->lock, flags); >> >
On 8/2/2023 2:30 PM, Ahmad Fatoum wrote: > Hello Peng, > > On 02.08.23 03:25, Peng Fan wrote: >> >> >> On 8/2/2023 12:27 AM, Ahmad Fatoum wrote: >>> Reconfiguring the clock divider to the exact same value is observed >>> on an i.MX8MN to often cause a short clock pause, probably because >>> the divider restarts counting from the time the register is written. >>> >>> This issue doesn't show up normally, because the clock framework will >>> take care to not call set_rate when the clock rate is the same. >>> However, when we configure an upstream clock (e.g. an audio_pll), the >>> common code will call set_rate with the newly calculated rate on all >>> children. As the new rate is different, we enter set_rate and compute >>> the same divider values, write them back and cause the glitch (e.g. >>> on a SAI's MCLK). >> >> >> The CCM root has glitch-free mux. When upstream pll freq change, >> the child set rate will also touch the mux bit, since div and mux >> in one register, so the mux logic will also function. >> >> Per design, it is glitch free, so I not understand well why glitch. >> >> When you configure pll, the downstream sai clk should still not be enabled, right? > > - sai5 is running normally and divides Audio PLL out by 16. > - audio_pll1 is increased by 32 Hz -> only kdiv changes, so no glitch > - imx8m_clk_composite_divider_set_rate(sai5) is called with > 32 / 16 = 2 Hz more > - imx8m_clk_composite_divider_set_rate computes same divider as before > and writes register > - divider starts counting from zero, so we have a longer clock pause > than usual, e.g. 40ns -> 125ns, external MCLK consumer doesn't like that at all. Thanks for detailed explaination, I would expect write this down in commit message. > > So it's not a glitch in the transient high frequency sense, but rather a transient > low frequency period. I can reword the commit message to s/glitch/clock pause/ > if you prefer. clock pause would be better, since the hardware design could avoid real glitch. Thanks, Peng. > > And yes, if mux is switched, we will probably get the same clock pause, but > that is not a problem for me currently, because we don't switch parents except > at boot up. Afterwards, only PLL is tuned. > > Cheers, > Ahmad > >> >> Thanks, >> Peng. >> >>> >>> To avoid the glitch, we skip writing the same value back again. >>> >>> Fixes: d3ff9728134e ("clk: imx: Add imx composite clock") >>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >>> --- >>> drivers/clk/imx/clk-composite-8m.c | 12 +++++++----- >>> 1 file changed, 7 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c >>> index cbf0d7955a00..3e9a092e136c 100644 >>> --- a/drivers/clk/imx/clk-composite-8m.c >>> +++ b/drivers/clk/imx/clk-composite-8m.c >>> @@ -97,7 +97,7 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, >>> int prediv_value; >>> int div_value; >>> int ret; >>> - u32 val; >>> + u32 orig, val; >>> ret = imx8m_clk_composite_compute_dividers(rate, parent_rate, >>> &prediv_value, &div_value); >>> @@ -106,13 +106,15 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, >>> spin_lock_irqsave(divider->lock, flags); >>> - val = readl(divider->reg); >>> - val &= ~((clk_div_mask(divider->width) << divider->shift) | >>> - (clk_div_mask(PCG_DIV_WIDTH) << PCG_DIV_SHIFT)); >>> + orig = readl(divider->reg); >>> + val = orig & ~((clk_div_mask(divider->width) << divider->shift) | >>> + (clk_div_mask(PCG_DIV_WIDTH) << PCG_DIV_SHIFT)); >>> val |= (u32)(prediv_value - 1) << divider->shift; >>> val |= (u32)(div_value - 1) << PCG_DIV_SHIFT; >>> - writel(val, divider->reg); >>> + >>> + if (val != orig) >>> + writel(val, divider->reg); >>> spin_unlock_irqrestore(divider->lock, flags); >>> >> >
diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c index cbf0d7955a00..3e9a092e136c 100644 --- a/drivers/clk/imx/clk-composite-8m.c +++ b/drivers/clk/imx/clk-composite-8m.c @@ -97,7 +97,7 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, int prediv_value; int div_value; int ret; - u32 val; + u32 orig, val; ret = imx8m_clk_composite_compute_dividers(rate, parent_rate, &prediv_value, &div_value); @@ -106,13 +106,15 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, spin_lock_irqsave(divider->lock, flags); - val = readl(divider->reg); - val &= ~((clk_div_mask(divider->width) << divider->shift) | - (clk_div_mask(PCG_DIV_WIDTH) << PCG_DIV_SHIFT)); + orig = readl(divider->reg); + val = orig & ~((clk_div_mask(divider->width) << divider->shift) | + (clk_div_mask(PCG_DIV_WIDTH) << PCG_DIV_SHIFT)); val |= (u32)(prediv_value - 1) << divider->shift; val |= (u32)(div_value - 1) << PCG_DIV_SHIFT; - writel(val, divider->reg); + + if (val != orig) + writel(val, divider->reg); spin_unlock_irqrestore(divider->lock, flags);