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

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

On Mon, May 02, 2022 at 02:18:26PM +0300, Dmitry Baryshkov wrote:
> On 02/05/2022 14:10, Manivannan Sadhasivam wrote:
> > 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?
> 
> It has 4 parents. It uses just two of them (pipe and tcxo).
> 
> And like the clk_rcg2_safe clock we'd like to say that these clocks are
> disabled by reparenting ("parking") them to the tcxo source. Yes, this makes
> a lot of code simpler. The clock framework will switch the clock to the
> "safe" state instead of disabling it during the unused clocks evaporation.
> The PHY can just disable the gcc_pcie_N_pipe_clock, which will end up in
> parking this clock to a safe state too, etc.

If I get the logic behind this "parking" thing right, then it is required
for producing a stable pipe_clk from GCC when the PHY is about to initialize.
Also to make sure that there is no glitch observed on pipe_clk while
initializing the PHY. And once it is powered ON properly, the pipe_clksrc
should be used as the parent for pipe_clk.

So with that logic, we cannot say that this clk is disabled.

Please correct me if my understanding is wrong.

Thanks,
Mani

> 
> > 
> > > 
> > > 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.
> 
> I see. Let's settle on the first question then.
> 
> -- 
> With best wishes
> Dmitry