by Abel Vesa

[permalink] [raw]

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

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 <[email protected]>
> > > 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 <[email protected]>
> > > ---
> > >
> > > 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
>

2020-04-30 13:00:28

by Peng Fan

[permalink] [raw]

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

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