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

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

On 02/05/2022 13:10, Manivannan Sadhasivam wrote:
> On Sun, May 01, 2022 at 10:21:46PM +0300, Dmitry Baryshkov wrote:
>> On recent Qualcomm platforms the QMP PIPE clocks feed into a set of
>> muxes which must be parked to the "safe" source (bi_tcxo) when
>> corresponding GDSC is turned off and on again. Currently this is
>> handcoded in the PCIe driver by reparenting the gcc_pipe_N_clk_src
>> clock. However the same code sequence should be applied in the
>> pcie-qcom endpoint, USB3 and UFS drivers.
>>
>> Rather than copying this sequence over and over again, follow the
>> example of clk_rcg2_shared_ops and implement this parking in the
>> enable() and disable() clock operations. Suppliement the regmap-mux with
>> the new regmap-pipe implementation, which hides multiplexer behind
>> simple branch-like clock. This is possible since each of this
>> multiplexers has just two clock sources: working (pipe) and safe
>> (bi_tcxo) clock sources. If the clock is running off the external pipe
>> source, report it as enable and report it as disabled otherwise.
>>
> 
> Sorry for chiming in late and providing comments that might have been addressed
> before. But I have few questions below:
> 
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/clk/qcom/Makefile          |  1 +
>>   drivers/clk/qcom/clk-regmap-pipe.c | 62 ++++++++++++++++++++++++++++++
>>   drivers/clk/qcom/clk-regmap-pipe.h | 24 ++++++++++++
>>   3 files changed, 87 insertions(+)
>>   create mode 100644 drivers/clk/qcom/clk-regmap-pipe.c
>>   create mode 100644 drivers/clk/qcom/clk-regmap-pipe.h
>>
>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
>> index 671cf5821af1..882c8ecc2e93 100644
>> --- a/drivers/clk/qcom/Makefile
>> +++ b/drivers/clk/qcom/Makefile
>> @@ -11,6 +11,7 @@ clk-qcom-y += clk-branch.o
>>   clk-qcom-y += clk-regmap-divider.o
>>   clk-qcom-y += clk-regmap-mux.o
>>   clk-qcom-y += clk-regmap-mux-div.o
>> +clk-qcom-y += clk-regmap-pipe.o
>>   clk-qcom-$(CONFIG_KRAIT_CLOCKS) += clk-krait.o
>>   clk-qcom-y += clk-hfpll.o
>>   clk-qcom-y += reset.o
>> diff --git a/drivers/clk/qcom/clk-regmap-pipe.c b/drivers/clk/qcom/clk-regmap-pipe.c
>> new file mode 100644
>> index 000000000000..9a7c27cc644b
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-regmap-pipe.c
>> @@ -0,0 +1,62 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022, Linaro Ltd.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/bitops.h>
>> +#include <linux/regmap.h>
>> +#include <linux/export.h>
>> +
>> +#include "clk-regmap-pipe.h"
>> +
>> +static inline struct clk_regmap_pipe *to_clk_regmap_pipe(struct clk_hw *hw)
>> +{
>> +	return container_of(to_clk_regmap(hw), struct clk_regmap_pipe, clkr);
>> +}
>> +
>> +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.

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?

> 
>> +}
>> +
>> +static int pipe_enable(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;
>> +
>> +	val = pipe->enable_val << pipe->shift;
>> +
>> +	return regmap_update_bits(clkr->regmap, pipe->reg, mask, val);
>> +}
>> +
>> +static void pipe_disable(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;
>> +
>> +	val = pipe->disable_val << pipe->shift;
>> +
>> +	regmap_update_bits(clkr->regmap, pipe->reg, mask, val);
>> +}
>> +
>> +const struct clk_ops clk_regmap_pipe_ops = {
>> +	.enable = pipe_enable,
>> +	.disable = pipe_disable,
>> +	.is_enabled = pipe_is_enabled,
>> +};
>> +EXPORT_SYMBOL_GPL(clk_regmap_pipe_ops);
>> diff --git a/drivers/clk/qcom/clk-regmap-pipe.h b/drivers/clk/qcom/clk-regmap-pipe.h
>> new file mode 100644
>> index 000000000000..cfaa792a029b
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-regmap-pipe.h
>> @@ -0,0 +1,24 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2022, Linaru Ltd.
> 
> Linaro

ack

> 
>> + * Author: Dmitry Baryshkov
> 
> No email?
> 
> Thanks,
> Mani
> 
>> + */
>> +
>> +#ifndef __QCOM_CLK_REGMAP_PIPE_H__
>> +#define __QCOM_CLK_REGMAP_PIPE_H__
>> +
>> +#include <linux/clk-provider.h>
>> +#include "clk-regmap.h"
>> +
>> +struct clk_regmap_pipe {
>> +	u32			reg;
>> +	u32			shift;
>> +	u32			width;
>> +	u32			enable_val;
>> +	u32			disable_val;
>> +	struct clk_regmap	clkr;
>> +};
>> +
>> +extern const struct clk_ops clk_regmap_pipe_ops;
>> +
>> +#endif
>> -- 
>> 2.35.1
>>


-- 
With best wishes
Dmitry