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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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.

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