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

Dmitry Baryshkov
2022-05-02T15:24:55Z

On 02/05/2022 18:06, 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.

Yes. It is not technically disabled. But as I said, it serves a good
abstraction, as a clock is a good as being 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

--
With best wishes
Dmitry