LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Leonard Crestez <leonard.crestez@nxp.com>
To: Peng Fan <peng.fan@nxp.com>, Anson Huang <anson.huang@nxp.com>
Cc: "shawnguo@kernel.org" <shawnguo@kernel.org>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"sboyd@kernel.org" <sboyd@kernel.org>,
	Abel Vesa <abel.vesa@nxp.com>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"festevam@gmail.com" <festevam@gmail.com>,
	dl-linux-imx <linux-imx@nxp.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Daniel Baluta <daniel.baluta@nxp.com>,
	"aford173@gmail.com" <aford173@gmail.com>,
	Jacky Bai <ping.bai@nxp.com>, Jun Li <jun.li@nxp.com>,
	"l.stach@pengutronix.de" <l.stach@pengutronix.de>,
	"andrew.smirnov@gmail.com" <andrew.smirnov@gmail.com>,
	"agx@sigxcpu.org" <agx@sigxcpu.org>,
	"angus@akkea.ca" <angus@akkea.ca>,
	"heiko@sntech.de" <heiko@sntech.de>,
	Andy Duan <fugang.duan@nxp.com>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>
Subject: Re: [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M composite clk
Date: Fri, 24 Apr 2020 19:29:01 +0000	[thread overview]
Message-ID: <VI1PR04MB69418E9348D5765B4AE01D18EED00@VI1PR04MB6941.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <1584008384-11578-8-git-send-email-peng.fan@nxp.com>

On 2020-03-12 12:27 PM, Peng Fan wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The CORE/BUS root slice has following design, simplied graph:
> The difference is core not have pre_div block.
> A composite core/bus clk has 8 inputs for mux to select, saying clk[0-7].
> 
>              SEL_A  GA
>              +--+  +-+
>              |  +->+ +------+
> CLK[0-7]--->+  |  +-+      |
>         |    |  |      +----v---+    +----+
>         |    +--+      |pre_diva+---->    |  +---------+
>         |              +--------+    |mux +--+post_div |
>         |    +--+      |pre_divb+--->+    |  +---------+
>         |    |  |      +----^---+    +----+
>         +--->+  |  +-+      |
>              |  +->+ +------+
>              +--+  +-+
>              SEL_B  GB
> 
> There will be system hang, when doing the following steps:
> 1. switch mux from clk0 to clk1
> 2. gate off clk0
> 3. swtich from clk1 to clk2, or gate off clk1
> 
> Step 3 triggers system hang.
> 
> If we skip step2, keep clk0 on, step 3 will not trigger system hang.
> However we have CLK_OPS_PARENT_ENABLE flag, which will unprepare disable
> the clk0 which will not be used.

As far as I understand when switching from clk1 to clk2 this is done by 
temporarily switching the rightmost SELECT mux to whatever was in the 
spare SEL, which is essentially arbitrary from linux POV.

This is quite unexpected but in theory it might be desirable to use a 
third parent as a fallback.

> 
> To address this issue, we could use following simplied software flow:
> After the first target register set
> wait the target register set finished
> set the target register set again
> wait the target register set finished
> 
> The upper flow will make sure SEL_A and SEL_B both set the new mux,
> but with only one path gate on.
> And there will be no system hang anymore with step3.

Your fix tries to work around this scenario by always setting the mux 
value in SEL_A and SEL_B to the same value after each set_parent operation.

But what if SEL_A and SEL_B are different at linux boot time and the 
first reparenting is done *after* disabling unused clocks? This doesn't 
happen for A53 because it's reparented during clock provider probe but 
maybe this scenario could be contrived if bootloader touches one of the 
other bus slices.

It might be extra safe to assign the parent of the spare mux at the 
start of each set_parent call. This could even be done on probe and this 
way wouldn't have to duplicate mux_ops just to do a double write.

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V2:
>   Drop wait after write, add one line comment for write twice.
> 
>   drivers/clk/imx/clk-composite-8m.c | 62 +++++++++++++++++++++++++++++++++++++-
>   1 file changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> index 99773519b5a5..eae02c151ced 100644
> --- a/drivers/clk/imx/clk-composite-8m.c
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -24,6 +24,12 @@
>   
>   #define PCG_CGC_SHIFT		28
>   
> +#define PRE_REG_OFF		0x30
> +#define PRE_MUXA_SHIFT		24
> +#define PRE_MUXA_MASK		0x7
> +#define PRE_MUXB_SHIFT		8
> +#define PRE_MUXB_MASK		0x7

These are unused.

> +
>   static unsigned long imx8m_clk_composite_divider_recalc_rate(struct clk_hw *hw,
>   						unsigned long parent_rate)
>   {
> @@ -124,6 +130,57 @@ static const struct clk_ops imx8m_clk_composite_divider_ops = {
>   	.set_rate = imx8m_clk_composite_divider_set_rate,
>   };
>   
> +static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
> +{
> +	struct clk_mux *mux = to_clk_mux(hw);
> +	u32 val;
> +
> +	val = readl(mux->reg) >> mux->shift;
> +	val &= mux->mask;
> +
> +	return clk_mux_val_to_index(hw, mux->table, mux->flags, val);
> +}
> +
> +static int imx8m_clk_composite_mux_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct clk_mux *mux = to_clk_mux(hw);
> +	u32 val = clk_mux_index_to_val(mux->table, mux->flags, index);
> +	unsigned long flags = 0;
> +	u32 reg;
> +
> +	if (mux->lock)
> +		spin_lock_irqsave(mux->lock, flags);
> +
> +	reg = readl(mux->reg);
> +	reg &= ~(mux->mask << mux->shift);
> +	val = val << mux->shift;
> +	reg |= val;
> +	/* write twice to make sure SEL_A/B point the same mux */
> +	writel(reg, mux->reg);
> +	writel(reg, mux->reg);
> +
> +	if (mux->lock)
> +		spin_unlock_irqrestore(mux->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int
> +imx8m_clk_composite_mux_determine_rate(struct clk_hw *hw,
> +				       struct clk_rate_request *req)
> +{
> +	struct clk_mux *mux = to_clk_mux(hw);
> +
> +	return clk_mux_determine_rate_flags(hw, req, mux->flags);
> +}
> +
> +
> +const struct clk_ops imx8m_clk_composite_mux_ops = {
> +	.get_parent = imx8m_clk_composite_mux_get_parent,
> +	.set_parent = imx8m_clk_composite_mux_set_parent,
> +	.determine_rate = imx8m_clk_composite_mux_determine_rate,
> +};
> +
>   struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>   					const char * const *parent_names,
>   					int num_parents, void __iomem *reg,
> @@ -136,6 +193,7 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>   	struct clk_gate *gate = NULL;
>   	struct clk_mux *mux = NULL;
>   	const struct clk_ops *divider_ops;
> +	const struct clk_ops *mux_ops;
>   
>   	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
>   	if (!mux)
> @@ -157,10 +215,12 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>   		div->shift = PCG_DIV_SHIFT;
>   		div->width = PCG_CORE_DIV_WIDTH;
>   		divider_ops = &clk_divider_ops;
> +		mux_ops = &imx8m_clk_composite_mux_ops;
>   	} else {
>   		div->shift = PCG_PREDIV_SHIFT;
>   		div->width = PCG_PREDIV_WIDTH;
>   		divider_ops = &imx8m_clk_composite_divider_ops;
> +		mux_ops = &clk_mux_ops;
>   	}
>   
>   	div->lock = &imx_ccm_lock;
> @@ -176,7 +236,7 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>   	gate->lock = &imx_ccm_lock;
>   
>   	hw = clk_hw_register_composite(NULL, name, parent_names, num_parents,
> -			mux_hw, &clk_mux_ops, div_hw,
> +			mux_hw, mux_ops, div_hw,
>   			divider_ops, gate_hw, &clk_gate_ops, flags);
>   	if (IS_ERR(hw))
>   		goto fail;
> 


  reply	other threads:[~2020-04-24 19:29 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-12 10:19 [PATCH V2 00/10] clk: imx: fixes and improve for i.MX8M peng.fan
2020-03-12 10:19 ` [PATCH V2 01/10] arm64: dts: imx8m: assign clocks for A53 peng.fan
2020-04-26  3:51   ` Aisheng Dong
2020-03-12 10:19 ` [PATCH V2 02/10] clk: imx8m: drop clk_hw_set_parent " peng.fan
2020-04-26  3:54   ` Aisheng Dong
2020-03-12 10:19 ` [PATCH V2 03/10] clk: imx: imx8mp: fix pll mux bit peng.fan
2020-04-26  4:23   ` Aisheng Dong
2020-03-12 10:19 ` [PATCH V2 04/10] clk: imx8mp: Define gates for pll1/2 fixed dividers peng.fan
2020-04-26  4:29   ` Aisheng Dong
2020-03-12 10:19 ` [PATCH V2 05/10] clk: imx8mp: use imx8m_clk_hw_composite_core to simplify code peng.fan
2020-04-26  4:38   ` Aisheng Dong
2020-04-27  8:57     ` Peng Fan
2020-03-12 10:19 ` [PATCH V2 06/10] clk: imx8m: migrate A53 clk root to use composite core peng.fan
2020-04-26  4:43   ` Aisheng Dong
2020-04-27  8:58     ` Peng Fan
2020-03-12 10:19 ` [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M composite clk peng.fan
2020-04-24 19:29   ` Leonard Crestez [this message]
2020-04-27  9:15     ` Peng Fan
2020-04-27 19:34       ` Leonard Crestez
2020-04-28  1:08         ` Peng Fan
2020-04-26  5:08   ` Aisheng Dong
2020-04-27  9:11     ` Peng Fan
2020-04-30 10:00       ` Abel Vesa
2020-04-30 12:56         ` Peng Fan
2020-03-12 10:19 ` [PATCH V2 08/10] clk: imx: add imx8m_clk_hw_composite_bus peng.fan
2020-03-12 10:19 ` [PATCH V2 09/10] clk: imx: use imx8m_clk_hw_composite_bus for i.MX8M bus clk slice peng.fan
2020-03-12 10:19 ` [PATCH V2 10/10] clk: imx8mp: mark memrepair clock as critical peng.fan
2020-03-19 10:04 ` [PATCH V2 00/10] clk: imx: fixes and improve for i.MX8M Peng Fan
2020-04-18 13:45 ` Peng Fan
2020-04-24 19:30   ` Leonard Crestez

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=VI1PR04MB69418E9348D5765B4AE01D18EED00@VI1PR04MB6941.eurprd04.prod.outlook.com \
    --to=leonard.crestez@nxp.com \
    --cc=abel.vesa@nxp.com \
    --cc=aford173@gmail.com \
    --cc=agx@sigxcpu.org \
    --cc=andrew.smirnov@gmail.com \
    --cc=angus@akkea.ca \
    --cc=anson.huang@nxp.com \
    --cc=daniel.baluta@nxp.com \
    --cc=festevam@gmail.com \
    --cc=fugang.duan@nxp.com \
    --cc=heiko@sntech.de \
    --cc=jun.li@nxp.com \
    --cc=kernel@pengutronix.de \
    --cc=l.stach@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peng.fan@nxp.com \
    --cc=ping.bai@nxp.com \
    --cc=s.hauer@pengutronix.de \
    --cc=sboyd@kernel.org \
    --cc=shawnguo@kernel.org \
    --subject='Re: [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M composite clk' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).