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
