[v5,2/5] clk: qcom: regmap: add PHY clock source implementation

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_pipe_src type, which implements such multiplexers
as a simple gate clocks.

This is possible since each of these multiplexers has just two clock
sources: working (pipe) and safe/park (bi_tcxo) clock sources. If the
clock is running off the external pipe source, report it as enabled and
report it as disabled otherwise.

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 parked to a safe clock srouce). 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>
---
 drivers/clk/qcom/Makefile              |  1 +
 drivers/clk/qcom/clk-regmap-pipe-src.c | 62 ++++++++++++++++++++++++++
 drivers/clk/qcom/clk-regmap-pipe-src.h | 24 ++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-regmap-pipe-src.c
 create mode 100644 drivers/clk/qcom/clk-regmap-pipe-src.h

On Thu, May 12, 2022 at 08:29:06PM +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.

You seem to have ignored my comment regarding UFS and naming except for
the updated Subject.

For UFS the corresponding clocks would be named symbol clocks, which
seems to suggest that a more generic name is warranted. I mentioned
phy_mux as a possible alternative name for for pipe_mux (or pipe_src).

> 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_pipe_src type, which implements such multiplexers
> as a simple gate clocks.
> 
> This is possible since each of these multiplexers has just two clock
> sources: working (pipe) and safe/park (bi_tcxo) clock sources. If the
> clock is running off the external pipe source, report it as enabled and
> report it as disabled otherwise.
> 
> 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 parked to a safe clock srouce). And vice versa, after enabling

typo: source

> 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.

Explaining how this fits together with the PHY power sequencing is good
but it needs to be reflected in the implementation too. Preferably using
good naming, but possibly also with a comment.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/Makefile              |  1 +
>  drivers/clk/qcom/clk-regmap-pipe-src.c | 62 ++++++++++++++++++++++++++
>  drivers/clk/qcom/clk-regmap-pipe-src.h | 24 ++++++++++
>  3 files changed, 87 insertions(+)
>  create mode 100644 drivers/clk/qcom/clk-regmap-pipe-src.c
>  create mode 100644 drivers/clk/qcom/clk-regmap-pipe-src.h
> 
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index 671cf5821af1..03b945535e49 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-src.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-src.c b/drivers/clk/qcom/clk-regmap-pipe-src.c
> new file mode 100644
> index 000000000000..02047987ab5f
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-regmap-pipe-src.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-src.h"
> +
> +static inline struct clk_regmap_pipe_src *to_clk_regmap_pipe_src(struct clk_hw *hw)
> +{
> +	return container_of(to_clk_regmap(hw), struct clk_regmap_pipe_src, clkr);
> +}
> +
> +static int pipe_src_is_enabled(struct clk_hw *hw)
> +{
> +	struct clk_regmap_pipe_src *pipe = to_clk_regmap_pipe_src(hw);

Again, "pipe" is so overloaded and using "mux" (or possibly "src")
throughout would make the code easier to understand.

> +	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->working_val && val != pipe->park_val));

Again, please drop unlikely() here.

> +
> +	return val == pipe->working_val;
> +}
> +
> +static int pipe_src_enable(struct clk_hw *hw)
> +{
> +	struct clk_regmap_pipe_src *pipe = to_clk_regmap_pipe_src(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->working_val << pipe->shift;
> +
> +	return regmap_update_bits(clkr->regmap, pipe->reg, mask, val);
> +}
> +
> +static void pipe_src_disable(struct clk_hw *hw)
> +{
> +	struct clk_regmap_pipe_src *pipe = to_clk_regmap_pipe_src(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->park_val << pipe->shift;
> +
> +	regmap_update_bits(clkr->regmap, pipe->reg, mask, val);
> +}
> +
> +const struct clk_ops clk_regmap_pipe_src_ops = {
> +	.enable = pipe_src_enable,
> +	.disable = pipe_src_disable,
> +	.is_enabled = pipe_src_is_enabled,
> +};
> +EXPORT_SYMBOL_GPL(clk_regmap_pipe_src_ops);
> diff --git a/drivers/clk/qcom/clk-regmap-pipe-src.h b/drivers/clk/qcom/clk-regmap-pipe-src.h
> new file mode 100644
> index 000000000000..3aa4a9f402cd
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-regmap-pipe-src.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2022, Linaro Ltd.
> + * Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> + */
> +
> +#ifndef __QCOM_CLK_REGMAP_PIPE_SRC_H__
> +#define __QCOM_CLK_REGMAP_PIPE_SRC_H__
> +
> +#include <linux/clk-provider.h>
> +#include "clk-regmap.h"
> +
> +struct clk_regmap_pipe_src {
> +	u32			reg;
> +	u32			shift;
> +	u32			width;
> +	u32			working_val;
> +	u32			park_val;

Naming is hard, but I believe something like ext_src_val (or
phy_src_val) and ref_src_val (or xo_src_val) would allow the code to be
more self-explanatory and not rely on looking up the commit message to
understand where "working" and "park" came from.

You probably need to add a comment in the implementation either way.

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

Johan

On Fri, 13 May 2022 at 11:16, Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, May 12, 2022 at 08:29:06PM +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.
>
> You seem to have ignored my comment regarding UFS and naming except for
> the updated Subject.
>
> For UFS the corresponding clocks would be named symbol clocks, which
> seems to suggest that a more generic name is warranted. I mentioned
> phy_mux as a possible alternative name for for pipe_mux (or pipe_src).

No, I did not. For some time I had named it clk_regmap_phy_src. Then I
decided against it on the grounds of being not descriptive enough too.
There are many PHY clocks (and sources), while this mechanism is used
only for pipe and symbol clocks. So I preferred to have a descriptive
name that is further extended to be used for symbol clocks rather than
a too broad name that is used only for pipe and symbol clocks. But if
you insist, I can change this.

>
> > 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_pipe_src type, which implements such multiplexers
> > as a simple gate clocks.
> >
> > This is possible since each of these multiplexers has just two clock
> > sources: working (pipe) and safe/park (bi_tcxo) clock sources. If the
> > clock is running off the external pipe source, report it as enabled and
> > report it as disabled otherwise.
> >
> > 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 parked to a safe clock srouce). And vice versa, after enabling
>
> typo: source
>
> > 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.
>
> Explaining how this fits together with the PHY power sequencing is good
> but it needs to be reflected in the implementation too. Preferably using
> good naming, but possibly also with a comment.

Ack

>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/clk/qcom/Makefile              |  1 +
> >  drivers/clk/qcom/clk-regmap-pipe-src.c | 62 ++++++++++++++++++++++++++
> >  drivers/clk/qcom/clk-regmap-pipe-src.h | 24 ++++++++++
> >  3 files changed, 87 insertions(+)
> >  create mode 100644 drivers/clk/qcom/clk-regmap-pipe-src.c
> >  create mode 100644 drivers/clk/qcom/clk-regmap-pipe-src.h
> >
> > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> > index 671cf5821af1..03b945535e49 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-src.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-src.c b/drivers/clk/qcom/clk-regmap-pipe-src.c
> > new file mode 100644
> > index 000000000000..02047987ab5f
> > --- /dev/null
> > +++ b/drivers/clk/qcom/clk-regmap-pipe-src.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-src.h"
> > +
> > +static inline struct clk_regmap_pipe_src *to_clk_regmap_pipe_src(struct clk_hw *hw)
> > +{
> > +     return container_of(to_clk_regmap(hw), struct clk_regmap_pipe_src, clkr);
> > +}
> > +
> > +static int pipe_src_is_enabled(struct clk_hw *hw)
> > +{
> > +     struct clk_regmap_pipe_src *pipe = to_clk_regmap_pipe_src(hw);
>
> Again, "pipe" is so overloaded and using "mux" (or possibly "src")
> throughout would make the code easier to understand.

Argh. I missed that the code still uses 'pipe' here. Will fix it in
the next version.

>
> > +     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->working_val && val != pipe->park_val));
>
> Again, please drop unlikely() here.

Ack

>
> > +
> > +     return val == pipe->working_val;
> > +}
> > +
> > +static int pipe_src_enable(struct clk_hw *hw)
> > +{
> > +     struct clk_regmap_pipe_src *pipe = to_clk_regmap_pipe_src(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->working_val << pipe->shift;
> > +
> > +     return regmap_update_bits(clkr->regmap, pipe->reg, mask, val);
> > +}
> > +
> > +static void pipe_src_disable(struct clk_hw *hw)
> > +{
> > +     struct clk_regmap_pipe_src *pipe = to_clk_regmap_pipe_src(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->park_val << pipe->shift;
> > +
> > +     regmap_update_bits(clkr->regmap, pipe->reg, mask, val);
> > +}
> > +
> > +const struct clk_ops clk_regmap_pipe_src_ops = {
> > +     .enable = pipe_src_enable,
> > +     .disable = pipe_src_disable,
> > +     .is_enabled = pipe_src_is_enabled,
> > +};
> > +EXPORT_SYMBOL_GPL(clk_regmap_pipe_src_ops);
> > diff --git a/drivers/clk/qcom/clk-regmap-pipe-src.h b/drivers/clk/qcom/clk-regmap-pipe-src.h
> > new file mode 100644
> > index 000000000000..3aa4a9f402cd
> > --- /dev/null
> > +++ b/drivers/clk/qcom/clk-regmap-pipe-src.h
> > @@ -0,0 +1,24 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2022, Linaro Ltd.
> > + * Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > + */
> > +
> > +#ifndef __QCOM_CLK_REGMAP_PIPE_SRC_H__
> > +#define __QCOM_CLK_REGMAP_PIPE_SRC_H__
> > +
> > +#include <linux/clk-provider.h>
> > +#include "clk-regmap.h"
> > +
> > +struct clk_regmap_pipe_src {
> > +     u32                     reg;
> > +     u32                     shift;
> > +     u32                     width;
> > +     u32                     working_val;
> > +     u32                     park_val;
>
> Naming is hard, but I believe something like ext_src_val (or
> phy_src_val) and ref_src_val (or xo_src_val) would allow the code to be
> more self-explanatory and not rely on looking up the commit message to
> understand where "working" and "park" came from.

Thanks. Yes, this looks more descriptive.

>
> You probably need to add a comment in the implementation either way.
>
> > +     struct clk_regmap       clkr;
> > +};
> > +
> > +extern const struct clk_ops clk_regmap_pipe_src_ops;
> > +
> > +#endif
>
> Johan