Re: [PATCH v4 2/5] clk: qcom: regmap: add pipe clk implementation

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Mon, May 02, 2022 at 01:35:34PM +0300, Dmitry Baryshkov wrote:

[...]

> > > +static int pipe_is_enabled(struct clk_hw *hw)
> > > +{
> > > +	struct clk_regmap_pipe *pipe = to_clk_regmap_pipe(hw);
> > > +	struct clk_regmap *clkr = to_clk_regmap(hw);
> > > +	unsigned int mask = GENMASK(pipe->width + pipe->shift - 1, pipe->shift);
> > > +	unsigned int val;
> > > +
> > > +	regmap_read(clkr->regmap, pipe->reg, &val);
> > > +	val = (val & mask) >> pipe->shift;
> > > +
> > > +	WARN_ON(unlikely(val != pipe->enable_val && val != pipe->disable_val));
> > > +
> > > +	return val == pipe->enable_val;
> > 
> > Selecting the clk parents in the enable/disable callback seems fine to me but
> > the way it is implemented doesn't look right.
> > 
> > First this "pipe_clksrc" is a mux clk by design, since we can only select the
> > parent. But you are converting it to a gate clk now.
> > 
> > Instead of that, my proposal would be to make this clk a composite one i.e,.
> > gate clk + mux clk. So even though the gate clk here would be a hack, we are
> > not changing the definition of mux clk.
> 
> This is what I had before, in revisions 1-3. Which proved to work, but is
> problematic a bit.
> 
> In the very end, it is not easily possible to make a difference between a
> clock reparented to the bi_tcxo and a disabled clock. E.g. if some user
> reparents the clock to the tcxo, then the driver will consider the clock
> disabled, but the clock framework will think that the clock is still
> enabled.

I don't understand this. How can you make this clock disabled? It just has 4
parents, right?

> 
> Thus we have to remove "safe" clock (bi_tcxo) from the list of parents. In
> case of pipe clocks (and ufs symbol clocks) this will leave us with just a
> single possible parent. Then having the mux part just doesn't make sense. It
> is just a gated clock. And this simplified a lot of things.
> 
> > 
> > So you can introduce a new ops like "clk_regmap_mux_gate_ops" and implement the
> > parent switching logic in the enable/disable callbacks. Additional benefit of
> > this ops is, in the future we can also support "gate + mux" clks easily.
> 
> If the need arises, we can easily resurrect the regmap_mux_safe patchset,
> fix the race pointed out by Johan, remove extra src-val mapping for safe
> value and use it for such clocks. I can post it separately, if you wish. But
> I'm not sure that it makes sense to use it for single-parent clocks.
> 
> > 
> > Also, please don't use the "enable_val/disable_val" members. It should be
> > something like "mux_sel_pre/mux_sel_post".
> 
> Why? Could you please elaborate?
> 

It aligns with my question above. I don't see how this clk can be
enabled/disabled.

Thanks,
Mani