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

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

On 06/05/2022 15:31, Johan Hovold 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.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> I think this is much better and it addresses most of my concerns with
> the previous approach by keeping things simple and using a dedicated
> implementation (i.e. separate from regmap-mux).
> 
> The purpose of the clock implementation can be documented in the source
> and is reflected in the naming. It avoids the issues related to the
> caching (locking and deferred muxing) which wasn't really needed in the
> first place as these muxes are binary.
> 
> By implementing is_enabled() you also allow for inspecting the state
> that the boot firmware left the mux in.
> 
> The only thing that comes to mind that wouldn't be possible is to
> set the mux state using an assigned clock parent in devicetree to make
> sure that XO is always selected before toggling the GDSC at probe.
> 
> But since that doesn't seem to work anyway when the boot firmware has
> set things up (e.g. causes a modem here to reset) that would probably
> need to be handled in the GDSC driver anyway (i.e. make sure the source
> is XO before enabling the GDSC but only when it was actually disabled).
> 
> Taking that one step further would be to implement all this in the GDSC
> driver from the start so that the PHY PLL is always muxed in while the
> power domain is enabled (and only then)...

I think, if we move this to the gdsc driver, we'd loose the part of the 
clock tree.

> 
>> ---
>>   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);
> 
> Since pipe is so overloaded already can we call this "pipe_mux" or
> "pipe_src" instead of just "pipe"?

I'd settle on the pipe_src then.
If you don't mind, I'll wait for your Tested-by and will post the rename 
patchset afterwards.

> 
> And similarly for
> 
> 	pipe_mux_is_enabled()
> 	struct clk_regmap_pipe_mux
> 	struct clk_regmap_pipe_mux_ops
> 
> etc.
> 
>> +	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));
> 
> This is not a hot path and there's rarely a need for unlikely().

Ack.

> 
>> +
>> +	return val == pipe->enable_val;
>> +}
> 
> Johan


-- 
With best wishes
Dmitry