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

[Peng Fan]

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