Re: [PATCH v6 2/5] clk: qcom: regmap: add PHY clock source implementation

[Johan Hovold <johan@kernel.org>]

On Fri, May 13, 2022 at 08:53:36PM +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.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

This looks really good now. Thanks for sticking with it.

Just one nit below.

> diff --git a/drivers/clk/qcom/clk-regmap-phy-mux.h b/drivers/clk/qcom/clk-regmap-phy-mux.h
> new file mode 100644
> index 000000000000..6260912191c5
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-regmap-phy-mux.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2022, Linaro Ltd.
> + * Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> + */
> +
> +#ifndef __QCOM_CLK_REGMAP_PHY_MUX_H__
> +#define __QCOM_CLK_REGMAP_PHY_MUX_H__
> +
> +#include <linux/clk-provider.h>
> +#include "clk-regmap.h"
> +
> +/*
> + * A special clock implementation for PHY pipe and symbols clock sources.

s/sources/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,

s|pipe|pipe/symbol|

> + * 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

s|pipe|pipe/symbol|

> + * pipe_clk_src to be switched from a safe source to the working one.
> + */

You're still referring to the old pipe_clk_src name in two places in
this comment.

Should this be reflected in Subject as well (e.g. "PHY mux
implementation")?

> +
> +struct clk_regmap_phy_mux {
> +	u32			reg;
> +	u32			shift;
> +	u32			width;
> +	u32			phy_src_val;
> +	u32			ref_src_val;
> +	struct clk_regmap	clkr;
> +};
> +
> +extern const struct clk_ops clk_regmap_phy_mux_ops;
> +
> +#endif

With the above fixed:

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

I've also tested the series on sc8280xp-crd and sa8295p-adp:

Tested-by: Johan Hovold <johan+linaro@kernel.org>

Johan