[V2,07/10] clk: imx: add mux ops for i.MX8M composite clk

Commit Message

Peng Fan March 12, 2020, 10:19 a.m. UTC

From: Peng Fan <[e-mail]>

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.

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.

Signed-off-by: Peng Fan <[e-mail]>
---

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(-)

Comments

Leonard Crestez April 24, 2020, 7:29 p.m. UTC | #1

On 2020-03-12 12:27 PM, Peng Fan wrote:
> From: Peng Fan <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 
> 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;
>

Aisheng Dong April 26, 2020, 5:08 a.m. UTC | #2

> From: Peng Fan <[e-mail]>
> Sent: Thursday, March 12, 2020 6:20 PM
> 
> 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.

Why Step 3 triggers system hang? Is this a HW limitation?

> 
> 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.
> 
> 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.

Is this IC proposed solution?

> 
> Signed-off-by: Peng Fan <[e-mail]>
> ---
> 
> 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

Are those macros used somewhere?

> +
>  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); }

You don't have to redefinition them if they're the same as clk_mux_ops.
You have the access to clk_mux_ops.

> +
> +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);

Why this affects both SEL_A/B?
Very tricky and may worth more comments.

Besides that, I'd like to see Abel's comments on this patch.

Regards
Aisheng

> +
> +	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); }

Same as bove.

> +
> +
> +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;
> --
> 2.16.4

Peng Fan April 27, 2020, 9:11 a.m. UTC | #3

> Subject: RE: [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M composite
> clk
> 
> > From: Peng Fan <[e-mail]>
> > Sent: Thursday, March 12, 2020 6:20 PM
> >
> > 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.
> 
> Why Step 3 triggers system hang? Is this a HW limitation?

It is what hardware designed.

There is a counter inside the clk root module to choose
SEL_A or SEL_B. If choose SEL_B, the parent of SEL_B must
be active, otherwise the change from SEL_A to SEL_B
will cause hang.

SEL_A and SEL_B is inside the clock root module,
It is not clk's software parentA/B. misunderstand
this will misunderstand the whole fix.

> 
> >
> > 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.
> >
> > 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.
> 
> Is this IC proposed solution?

This is what I proposed and IC team confirmed.

> 
> >
> > Signed-off-by: Peng Fan <[e-mail]>
> > ---
> >
> > 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
> 
> Are those macros used somewhere?

Remove in v3.

> 
> > +
> >  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); }
> 
> You don't have to redefinition them if they're the same as clk_mux_ops.
> You have the access to clk_mux_ops.

This will require export_symbol of clk_mux_ops callbacks.

> 
> > +
> > +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);
> 
> Why this affects both SEL_A/B?

The internal counter will make sure both SEL_A/B point
to the same mux.

> Very tricky and may worth more comments.

Ah, I think RM should be clear about the target interface
and non-target interface.

When you write once, saying use SEL_A, when
you write the 2nd, the hardware will use SEL_B,
when you write 3rd, the hardware will use SEL_A.
and ...

> 
> Besides that, I'd like to see Abel's comments on this patch.


Abel,

 Any comments?

Thanks,
Peng.

> 
> Regards
> Aisheng
> 
> > +
> > +	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); }
> 
> Same as bove.
> 
> > +
> > +
> > +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;
> > --
> > 2.16.4

Peng Fan April 27, 2020, 9:15 a.m. UTC | #4

> Subject: Re: [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M composite
> clk
> 
> On 2020-03-12 12:27 PM, Peng Fan wrote:
> > From: Peng Fan <[e-mail]>
> >
> > 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.

No. The fixes in this patches has been confirmed by IC design owner

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

No. this will make things complicated. To CCM SEL_A and SEL_B,
it is controlled by a hardware counter. Saying you write n times to
the target interface.

The mux will use n % 2 to choose SEL_A or SEL_B. write twice
to make sure SEL_A and SEL_B has the same value.

Thanks,
Peng.

> 
> >
> > 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 <[e-mail]>
> > ---
> >
> > 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;
> >

Leonard Crestez April 27, 2020, 7:34 p.m. UTC | #5

On 2020-04-27 12:15 PM, Peng Fan wrote:
>> Subject: Re: [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M composite
>> clk
>>
>> On 2020-03-12 12:27 PM, Peng Fan wrote:
>>> From: Peng Fan <[e-mail]>
>>>
>>> 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.
> 
> No. The fixes in this patches has been confirmed by IC design owner
> 
>>
>> This is quite unexpected but in theory it might be desirable to use a third
>> parent as a fallback.
> 
> No. this will make things complicated. To CCM SEL_A and SEL_B,
> it is controlled by a hardware counter. Saying you write n times to
> the target interface.
> 
> The mux will use n % 2 to choose SEL_A or SEL_B. write twice
> to make sure SEL_A and SEL_B has the same value.

What if SEL_A and SEL_B have different values on boot? The first time 
linux does set_parent it will switch to the other SEL_X which might be off.

Peng Fan April 28, 2020, 1:08 a.m. UTC | #6

> Subject: Re: [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M composite
> clk
> 
> On 2020-04-27 12:15 PM, Peng Fan wrote:
> >> Subject: Re: [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M
> >> composite clk
> >>
> >> On 2020-03-12 12:27 PM, Peng Fan wrote:
> >>> From: Peng Fan <[e-mail]>
> >>>
> >>> 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.
> >
> > No. The fixes in this patches has been confirmed by IC design owner
> >
> >>
> >> This is quite unexpected but in theory it might be desirable to use a
> >> third parent as a fallback.
> >
> > No. this will make things complicated. To CCM SEL_A and SEL_B, it is
> > controlled by a hardware counter. Saying you write n times to the
> > target interface.
> >
> > The mux will use n % 2 to choose SEL_A or SEL_B. write twice to make
> > sure SEL_A and SEL_B has the same value.
> 
> What if SEL_A and SEL_B have different values on boot? The first time linux
> does set_parent it will switch to the other SEL_X which might be off.

If SEL_A and SEL_B has different values on boot, SEL_A or SEL_B will
be effective according internal counter. There must be one
chosen for mux usage whether SEL_A or SEL_B.

The CCM ROOT slice only has SEL_A and SEL_B, no SEL_X. SEL_A and SEL_B
both have 7 mux inputs.

Thanks,
Peng.

Abel Vesa April 30, 2020, 10 a.m. UTC | #7

On 20-04-27 09:11:56, Peng Fan wrote:
> > Subject: RE: [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M composite
> > clk
> > 
> > > From: Peng Fan <[e-mail]>
> > > Sent: Thursday, March 12, 2020 6:20 PM
> > >
> > > 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.
> > 
> > Why Step 3 triggers system hang? Is this a HW limitation?
> 
> It is what hardware designed.
> 
> There is a counter inside the clk root module to choose
> SEL_A or SEL_B. If choose SEL_B, the parent of SEL_B must
> be active, otherwise the change from SEL_A to SEL_B
> will cause hang.
> 
> SEL_A and SEL_B is inside the clock root module,
> It is not clk's software parentA/B. misunderstand
> this will misunderstand the whole fix.
> 
> > 
> > >
> > > 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.
> > >
> > > 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.
> > 
> > Is this IC proposed solution?
> 
> This is what I proposed and IC team confirmed.
> 
> > 
> > >
> > > Signed-off-by: Peng Fan <[e-mail]>
> > > ---
> > >
> > > 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
> > 
> > Are those macros used somewhere?
> 
> Remove in v3.
> 
> > 
> > > +
> > >  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); }
> > 
> > You don't have to redefinition them if they're the same as clk_mux_ops.
> > You have the access to clk_mux_ops.
> 
> This will require export_symbol of clk_mux_ops callbacks.
> 

Maybe you can do here:

return clk_mux_ops.get_parent(hw);

> > 
> > > +
> > > +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);
> > 
> > Why this affects both SEL_A/B?
> 
> The internal counter will make sure both SEL_A/B point
> to the same mux.
> 
> > Very tricky and may worth more comments.
> 
> Ah, I think RM should be clear about the target interface
> and non-target interface.
> 
> When you write once, saying use SEL_A, when
> you write the 2nd, the hardware will use SEL_B,
> when you write 3rd, the hardware will use SEL_A.
> and ...
> 

This is a very interesting behavior from HW point of view.
So every write changes the mux ? 

Unless there is an ERRATA for this, we'll get a lot of pushback from upstream.

> > 
> > Besides that, I'd like to see Abel's comments on this patch.
> 
> 
> Abel,
> 
>  Any comments?
> 
> Thanks,
> Peng.
> 
> > 
> > Regards
> > Aisheng
> > 
> > > +
> > > +	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); }
> > 
> > Same as bove.
> > 
> > > +
> > > +
> > > +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;
> > > --
> > > 2.16.4
>

Peng Fan April 30, 2020, 12:56 p.m. UTC | #8

Hi Abel, Aisheng, Leonard and all

> Subject: Re: [PATCH V2 07/10] clk: imx: add mux ops for i.MX8M composite
> clk
> 
....
> > > > +
> > > > +	return clk_mux_val_to_index(hw, mux->table, mux->flags, val); }
> > >
> > > You don't have to redefinition them if they're the same as clk_mux_ops.
> > > You have the access to clk_mux_ops.
> >
> > This will require export_symbol of clk_mux_ops callbacks.
> >
> 
> Maybe you can do here:
> 
> return clk_mux_ops.get_parent(hw);

Ok, will try this.

> 
> > >
> > > > +
> > > > +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);
> > >
> > > Why this affects both SEL_A/B?
> >
> > The internal counter will make sure both SEL_A/B point to the same
> > mux.
> >
> > > Very tricky and may worth more comments.
> >
> > Ah, I think RM should be clear about the target interface and
> > non-target interface.
> >
> > When you write once, saying use SEL_A, when you write the 2nd, the
> > hardware will use SEL_B, when you write 3rd, the hardware will use
> > SEL_A.
> > and ...
> >
> 
> This is a very interesting behavior from HW point of view.
> So every write changes the mux ?
> 
> Unless there is an ERRATA for this, we'll get a lot of pushback from upstream.

Let me share more details about this. Then if ok, I'll put in commit log and post
V2.

There is no errata, this is the hardware designed as is and it exist since i.MX7D.
i.MX8M and i.MX7D using same CCM root design.

It support target(smart) interface and normal interface. Target interface is exported
for programmer easy to configure ccm root. Normal interface is also
exported, but we not use it in our driver, because it will introduce more
complexity compared with target interface.

From RM:
"
The Target Interface is optimized to simplify software operation. Using this interface, all
clock roots are in the same program model with the same register bit field mapping. The
software does not handle the details of the clock slice and clock slice types. Software
writes the desired settings to the register, and the internal hardware logic generates a
required sequence to achieve the desired settings.
"

From i.MX8MN RM:
"
A requirement of the Target Interface's software is that the
target clock source is active.
"

We touch target interface, but hardware logic actually also need configure normal interface.

I draw a simple normal interface for core clock slice pic:

CLK[0-7] --------->SEL_A ----->-----CG---->|
         |                        \
         |                          mux-->post_podf-->
         V                        /                   
         |------>SEL_B------>---CG---->| /


The mux in the upper pic is not the target interface MUX, target interface MUX is
hiding SEL_A and SEL_B. When you choose clk[0-7], you are actually writing
SEL_A or SEL_B depends on the internal counter which will also control the
internal "mux".

Whether the hardware touch SEL_A or SEL_B, it requires the clock input to
SEL_A or SEL_B must be active. However SEL_A and SEL_B could have
different value. Saying SEL_A is clk1, SEL_B is clk4, the internal counter
controlled automatically by hardware logic choose SEL_A, then Linux will
disable clk4 because of no user. Now we write target MUX to choose clk5,
the internal counter will configure SEL_B to clk5 and switch to SEL_B,
however the previous SEL_B input clk4 is off, system hang, the hardware
requires SEL_B input clk4 is on, then hardware could configure SEL_B to
clk5.

That's why write twice to make sure the internal counter could select
SEL_A and SEL_B to same active input clk.

Please see whether this clarify the issue or not. I could post the upper
into commit log in V3. The fixes needs to be into 5.7 to avoid system
boot hang.

Thanks,
Peng.

> 
> > >
> > > Besides that, I'd like to see Abel's comments on this patch.
> >
> >
> > Abel,
> >
> >  Any comments?
> >
> > Thanks,
> > Peng.
> >
> > >
> > > Regards
> > > Aisheng
> > >
> > > > +
> > > > +	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); }
> > >
> > > Same as bove.
> > >
> > > > +
> > > > +
> > > > +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;
> > > > --
> > > > 2.16.4
> >

Patch

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
+
 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;