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

[Johan Hovold <johan@kernel.org>]

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.

I noticed that the vendor kernel does not implement this for UFS (yet),
and the PHY PLL is left muxed in for UFS by the boot firmware on the
platforms I have.

But supposedly it is needed, so perhaps this should be reflected in the
naming from the start by using a more generic name than "pipe". Maybe
something like struct clk_regmap_phy_mux?
 
> 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

typo: supplement

> the new regmap-pipe implementation, which hides multiplexer behind
> simple branch-like clock. 

Please rephrase the above. I understand what you mean, but that may not
be case with someone less familiar with the details. Perhaps explain it
along the lines of modelling the multiplexer as a gate.

And you shouldn't take the "hiding" too far and obfuscate the fact that
this is a multiplexer in the implementation.

Renaming some of the structures and fields should make the
implementation more obvious. I already suggested adding a suffix to the
use of "pipe" which really refers to the pipe mux.

Similarly, using another name for the enable/disable value fields may
make it easier to see what it's going on here.

> 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>
> ---
>  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;
> +}
> +
> +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);

So the above would be more obvious as something like

	static int pipe_mux_enable() {
		...

		val = mux->pipe_clk_val << mux->shift;
		...
	}

instead making it look like it is a gate (or maybe phy_mux_enable()
etc).

> +}
> +
> +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;

And similar by using something like xo_clk_val here.

> +
> +	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.
> + * Author: Dmitry Baryshkov
> + */
> +
> +#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;

So this could be
	
	pipe_clk_val
	xo_clk_val

and you wouldn't need to add comments in every mux definition.

> +	struct clk_regmap	clkr;
> +};
> +
> +extern const struct clk_ops clk_regmap_pipe_ops;
> +
> +#endif

Johan