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

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

On Mon, May 02, 2022 at 08:36:19PM +0530, Manivannan Sadhasivam wrote:
> 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.
> 

After going through the previous iterations and PHY patches, this
implementation makes sense to me now (modelling this pipe_clk_src as gate clk
alone).

But as Johan said, you should change the "pipe_clk" to "pipe_clk_src" or
something similar. As this clk is defined as "pipe_clk_src" in GCC.

Regarding the {enable/disable}_val variables, I'm not fully convinced about the
naming but I don't object it strongly. But irrespective of that, please add a
brief comment in the driver about its purpose and what it does enable/disable
callbacks.

Thanks,
Mani

> 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

-- 
மணிவண்ணன் சதாசிவம்