Re: [PATCH v7 2/6] clk: qcom: regmap: add PHY clock source implementation

[Johan Hovold <johan@kernel.org>]

On Fri, May 20, 2022 at 04:58:40AM +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. Supplement the regmap-mux with
> the new clk_regmap_phy_mux type, which implements such multiplexers
> as a simple gate clocks.
> 
> This is possible since each of these multiplexers has just two clock
> sources: one coming from the PHY and a reference (XO) one.  If the clock
> is running off the from-PHY source, report it as enabled. Report it as
> disabled otherwise (if it uses reference source).
> 
> This way the PHY will disable the pipe clock before turning off the
> GDSC, which in turn would lead to disabling corresponding pipe_clk_src
> (and thus it being parked to a safe, reference clock source). And vice
> versa, after enabling the GDSC the PHY will enable the pipe clock, which
> would cause pipe_clk_src to be switched from a safe source to the
> working one.
> 
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> Tested-by: Johan Hovold <johan+linaro@kernel.org>

I haven't reviewed or tested this version yet...

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/Makefile             |  1 +
>  drivers/clk/qcom/clk-regmap-phy-mux.c | 53 +++++++++++++++++++++++++++
>  drivers/clk/qcom/clk-regmap.h         | 17 +++++++++
>  3 files changed, 71 insertions(+)
>  create mode 100644 drivers/clk/qcom/clk-regmap-phy-mux.c
> 
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index dff6aeb980e6..6d242f46bd1d 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-phy-mux.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-phy-mux.c b/drivers/clk/qcom/clk-regmap-phy-mux.c
> new file mode 100644
> index 000000000000..dc96714a6175
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-regmap-phy-mux.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022, Linaro Ltd.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/bitops.h>
> +#include <linux/regmap.h>
> +#include <linux/export.h>
> +
> +#include "clk-regmap.h"
> +
> +#define PHY_MUX_MASK		GENMASK(1, 0)
> +#define PHY_MUX_PHY_SRC		0
> +#define PHY_MUX_REF_SRC		2
> +
> +static int phy_mux_is_enabled(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clkr = to_clk_regmap(hw);
> +	unsigned int val;
> +
> +	regmap_read(clkr->regmap, clkr->enable_reg, &val);
> +	val = FIELD_GET(PHY_MUX_MASK, val);
> +
> +	WARN_ON(val != PHY_MUX_PHY_SRC && val != PHY_MUX_REF_SRC);
> +
> +	return val == PHY_MUX_PHY_SRC;
> +}
> +
> +static int phy_mux_enable(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clkr = to_clk_regmap(hw);
> +
> +	return regmap_update_bits(clkr->regmap, clkr->enable_reg,
> +				  PHY_MUX_MASK,
> +				  FIELD_PREP(PHY_MUX_MASK, PHY_MUX_PHY_SRC));
> +}
> +
> +static void phy_mux_disable(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clkr = to_clk_regmap(hw);
> +
> +	regmap_update_bits(clkr->regmap, clkr->enable_reg,
> +			   PHY_MUX_MASK,
> +			   FIELD_PREP(PHY_MUX_MASK, PHY_MUX_REF_SRC));
> +}

I prefer the implementation where you had a dedicated struct
clk_regmap_phy_mux to match the ops rather than repurpose the clk_regmap
and its enable_reg.

This is a mux and that should be reflected in the implementation (even if
it's modelled as a gate).

This will also make it easier to add further fields which there are
indications that we may need to do pretty soon.

> +
> +const struct clk_ops clk_regmap_phy_mux_ops = {
> +	.enable = phy_mux_enable,
> +	.disable = phy_mux_disable,
> +	.is_enabled = phy_mux_is_enabled,
> +};
> +EXPORT_SYMBOL_GPL(clk_regmap_phy_mux_ops);
> diff --git a/drivers/clk/qcom/clk-regmap.h b/drivers/clk/qcom/clk-regmap.h
> index 14ec659a3a77..a58cd1d790fe 100644
> --- a/drivers/clk/qcom/clk-regmap.h
> +++ b/drivers/clk/qcom/clk-regmap.h
> @@ -35,4 +35,21 @@ int clk_enable_regmap(struct clk_hw *hw);
>  void clk_disable_regmap(struct clk_hw *hw);
>  int devm_clk_register_regmap(struct device *dev, struct clk_regmap *rclk);
>  
> +/*
> + * A clock implementation for PHY pipe and symbols clock muxes.
> + *
> + * If the clock is running off the from-PHY source, report it as enabled.
> + * Report it as disabled otherwise (if it uses reference source).
> + *
> + * This way the PHY will disable the pipe clock before turning off the GDSC,
> + * which in turn would lead to disabling corresponding pipe_clk_src (and thus
> + * it being parked to a safe, reference clock source). And vice versa, after
> + * enabling the GDSC the PHY will enable the pipe clock, which would cause
> + * pipe_clk_src to be switched from a safe source to the working one.
> + *
> + * For some platforms this should be used for the UFS symbol_clk_src clocks
> + * too.
> + */
> +extern const struct clk_ops clk_regmap_phy_mux_ops;
> +
>  #endif

Johan