[PATCH v6 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling

[PATCH v6 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling

[Dmitry Baryshkov]

PCIe pipe clk (and some other clocks) 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.

Instead of doing it manually, follow the approach used by
clk_rcg2_shared_ops and implement this parking in the enable() and
disable() clock operations for respective pipe clocks.

Changes since v5:
 - Rename the clock to clk-regmap-phy-mux and the enable/disable values
   to phy_src_val and ref_src_val respectively (as recommended by
   Johan).

Changes since v4:
 - Renamed the clock to clk-regmap-pipe-src,
 - Added mention of PCIe2 PHY to the commit message,
 - Expanded commit messages to mention additional pipe clock details.

Changes since v3:
 - Replaced the clock multiplexer implementation with branch-like clock.

Changes since v2:
 - Added is_enabled() callback
 - Added default parent to the pipe clock configuration

Changes since v1:
 - Rebased on top of [1].
 - Removed erroneous Fixes tag from the patch 4.

Changes since RFC:
 - Rework clk-regmap-mux fields. Specify safe parent as P_* value rather
   than specifying the register value directly
 - Expand commit message to the first patch to specially mention that
   it is required only on newer generations of Qualcomm chipsets.


Dmitry Baryshkov (5):
  PCI: qcom: Remove unnecessary pipe_clk handling
  clk: qcom: regmap: add PHY clock source implementation
  clk: qcom: gcc-sm8450: use new clk_regmap_pipe_src_ops for PCIe pipe
    clocks
  clk: qcom: gcc-sc7280: use new clk_regmap_pipe_src_ops for PCIe pipe
    clocks
  PCI: qcom: Drop manual pipe_clk_src handling

 drivers/clk/qcom/Makefile              |  1 +
 drivers/clk/qcom/clk-regmap-phy-mux.c  | 62 ++++++++++++++++++++
 drivers/clk/qcom/clk-regmap-phy-mux.h  | 37 ++++++++++++
 drivers/clk/qcom/gcc-sc7280.c          | 49 ++++++----------
 drivers/clk/qcom/gcc-sm8450.c          | 51 ++++++----------
 drivers/pci/controller/dwc/pcie-qcom.c | 81 +-------------------------
 6 files changed, 141 insertions(+), 140 deletions(-)
 create mode 100644 drivers/clk/qcom/clk-regmap-phy-mux.c
 create mode 100644 drivers/clk/qcom/clk-regmap-phy-mux.h


base-commit: 3123109284176b1532874591f7c81f3837bbdc17
prerequisite-patch-id: 71e4b5b7ff5d87f2407735cc6a3074812cde3697
-- 
2.35.1

[PATCH v6 1/5] PCI: qcom: Remove unnecessary pipe_clk handling

[Dmitry Baryshkov]

PCIe PHY drivers (both QMP and PCIe2) already do clk_prepare_enable() /
clk_prepare_disable() pipe_clk. Remove extra calls to enable/disable
this clock from the PCIe driver, so that the PHY driver can manage the
clock on its own.

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 44 ++------------------------
 1 file changed, 3 insertions(+), 41 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 57636246cecc..a6becafb6a77 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -128,7 +128,6 @@ struct qcom_pcie_resources_2_3_2 {
 	struct clk *master_clk;
 	struct clk *slave_clk;
 	struct clk *cfg_clk;
-	struct clk *pipe_clk;
 	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
 };
 
@@ -165,7 +164,6 @@ struct qcom_pcie_resources_2_7_0 {
 	int num_clks;
 	struct regulator_bulk_data supplies[2];
 	struct reset_control *pci_reset;
-	struct clk *pipe_clk;
 	struct clk *pipe_clk_src;
 	struct clk *phy_pipe_clk;
 	struct clk *ref_clk_src;
@@ -597,8 +595,7 @@ static int qcom_pcie_get_resources_2_3_2(struct qcom_pcie *pcie)
 	if (IS_ERR(res->slave_clk))
 		return PTR_ERR(res->slave_clk);
 
-	res->pipe_clk = devm_clk_get(dev, "pipe");
-	return PTR_ERR_OR_ZERO(res->pipe_clk);
+	return 0;
 }
 
 static void qcom_pcie_deinit_2_3_2(struct qcom_pcie *pcie)
@@ -613,13 +610,6 @@ static void qcom_pcie_deinit_2_3_2(struct qcom_pcie *pcie)
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
 }
 
-static void qcom_pcie_post_deinit_2_3_2(struct qcom_pcie *pcie)
-{
-	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
-
-	clk_disable_unprepare(res->pipe_clk);
-}
-
 static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
@@ -694,22 +684,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
 	return ret;
 }
 
-static int qcom_pcie_post_init_2_3_2(struct qcom_pcie *pcie)
-{
-	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
-	struct dw_pcie *pci = pcie->pci;
-	struct device *dev = pci->dev;
-	int ret;
-
-	ret = clk_prepare_enable(res->pipe_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable pipe clock\n");
-		return ret;
-	}
-
-	return 0;
-}
-
 static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
@@ -1198,8 +1172,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 			return PTR_ERR(res->ref_clk_src);
 	}
 
-	res->pipe_clk = devm_clk_get(dev, "pipe");
-	return PTR_ERR_OR_ZERO(res->pipe_clk);
+	return 0;
 }
 
 static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
@@ -1292,14 +1265,7 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
 	if (pcie->cfg->pipe_clk_need_muxing)
 		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
 
-	return clk_prepare_enable(res->pipe_clk);
-}
-
-static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
-{
-	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
-
-	clk_disable_unprepare(res->pipe_clk);
+	return 0;
 }
 
 static int qcom_pcie_link_up(struct dw_pcie *pci)
@@ -1449,9 +1415,7 @@ static const struct qcom_pcie_ops ops_1_0_0 = {
 static const struct qcom_pcie_ops ops_2_3_2 = {
 	.get_resources = qcom_pcie_get_resources_2_3_2,
 	.init = qcom_pcie_init_2_3_2,
-	.post_init = qcom_pcie_post_init_2_3_2,
 	.deinit = qcom_pcie_deinit_2_3_2,
-	.post_deinit = qcom_pcie_post_deinit_2_3_2,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 };
 
@@ -1478,7 +1442,6 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 	.post_init = qcom_pcie_post_init_2_7_0,
-	.post_deinit = qcom_pcie_post_deinit_2_7_0,
 };
 
 /* Qcom IP rev.: 1.9.0 */
@@ -1488,7 +1451,6 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 	.post_init = qcom_pcie_post_init_2_7_0,
-	.post_deinit = qcom_pcie_post_deinit_2_7_0,
 	.config_sid = qcom_pcie_config_sid_sm8250,
 };
 
-- 
2.35.1

Re: [PATCH v6 1/5] PCI: qcom: Remove unnecessary pipe_clk handling

[Johan Hovold]

On Fri, May 13, 2022 at 08:53:35PM +0300, Dmitry Baryshkov wrote:
> PCIe PHY drivers (both QMP and PCIe2) already do clk_prepare_enable() /
> clk_prepare_disable() pipe_clk. Remove extra calls to enable/disable
> this clock from the PCIe driver, so that the PHY driver can manage the
> clock on its own.
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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

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

[Dmitry Baryshkov]

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>
---
 drivers/clk/qcom/Makefile             |  1 +
 drivers/clk/qcom/clk-regmap-phy-mux.c | 62 +++++++++++++++++++++++++++
 drivers/clk/qcom/clk-regmap-phy-mux.h | 37 ++++++++++++++++
 3 files changed, 100 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-regmap-phy-mux.c
 create mode 100644 drivers/clk/qcom/clk-regmap-phy-mux.h

diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 671cf5821af1..e4ceb5819ae6 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..d7a45f7fa1aa
--- /dev/null
+++ b/drivers/clk/qcom/clk-regmap-phy-mux.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-phy-mux.h"
+
+static inline struct clk_regmap_phy_mux *to_clk_regmap_phy_mux(struct clk_hw *hw)
+{
+	return container_of(to_clk_regmap(hw), struct clk_regmap_phy_mux, clkr);
+}
+
+static int phy_mux_is_enabled(struct clk_hw *hw)
+{
+	struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
+	struct clk_regmap *clkr = to_clk_regmap(hw);
+	unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
+	unsigned int val;
+
+	regmap_read(clkr->regmap, phy_mux->reg, &val);
+	val = (val & mask) >> phy_mux->shift;
+
+	WARN_ON(val != phy_mux->phy_src_val && val != phy_mux->ref_src_val);
+
+	return val == phy_mux->phy_src_val;
+}
+
+static int phy_mux_enable(struct clk_hw *hw)
+{
+	struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
+	struct clk_regmap *clkr = to_clk_regmap(hw);
+	unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
+	unsigned int val;
+
+	val = phy_mux->phy_src_val << phy_mux->shift;
+
+	return regmap_update_bits(clkr->regmap, phy_mux->reg, mask, val);
+}
+
+static void phy_mux_disable(struct clk_hw *hw)
+{
+	struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
+	struct clk_regmap *clkr = to_clk_regmap(hw);
+	unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
+	unsigned int val;
+
+	val = phy_mux->ref_src_val << phy_mux->shift;
+
+	regmap_update_bits(clkr->regmap, phy_mux->reg, mask, val);
+}
+
+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-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.
+ *
+ * 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.
+ */
+
+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
-- 
2.35.1

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

[Johan Hovold]

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

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

[Johan Hovold]

On Wed, May 18, 2022 at 09:34:19AM +0200, Johan Hovold wrote:
> On Fri, May 13, 2022 at 08:53:36PM +0300, Dmitry Baryshkov wrote:

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

Just remembered that the PCIe/USB mux is also referred to as
pipe_clk_src and that your not referring to the clock implementation.

I guess the comment works as-is even if the example refers to just
USB/PCIe.

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

Johan

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

[Dmitry Baryshkov]

On 18/05/2022 10:48, Johan Hovold wrote:
> On Wed, May 18, 2022 at 09:34:19AM +0200, Johan Hovold wrote:
>> On Fri, May 13, 2022 at 08:53:36PM +0300, Dmitry Baryshkov wrote:
> 
>>> +/*
>>> + * 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.
> 
> Just remembered that the PCIe/USB mux is also referred to as
> pipe_clk_src and that your not referring to the clock implementation.
> 
> I guess the comment works as-is even if the example refers to just
> USB/PCIe.

I will add a phrase mentioning UFS symbol clocks.

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


-- 
With best wishes
Dmitry

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

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-05-13 10:53:36)
> 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..d7a45f7fa1aa
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-regmap-phy-mux.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>

clk-provider.h for clk_hw/clk_ops usage. It helps with grep to identify
clk providers.

> +
> +#include "clk-regmap-phy-mux.h"

Same for clk-regmap.h, avoid include hell.

> +
> +static inline struct clk_regmap_phy_mux *to_clk_regmap_phy_mux(struct clk_hw *hw)
> +{
> +       return container_of(to_clk_regmap(hw), struct clk_regmap_phy_mux, clkr);
> +}
> +
> +static int phy_mux_is_enabled(struct clk_hw *hw)
> +{
> +       struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
> +       struct clk_regmap *clkr = to_clk_regmap(hw);
> +       unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
> +       unsigned int val;
> +
> +       regmap_read(clkr->regmap, phy_mux->reg, &val);
> +       val = (val & mask) >> phy_mux->shift;

Can this use FIELD_GET?

> +
> +       WARN_ON(val != phy_mux->phy_src_val && val != phy_mux->ref_src_val);
> +
> +       return val == phy_mux->phy_src_val;
> +}
> +
> +static int phy_mux_enable(struct clk_hw *hw)
> +{
> +       struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
> +       struct clk_regmap *clkr = to_clk_regmap(hw);
> +       unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
> +       unsigned int val;
> +
> +       val = phy_mux->phy_src_val << phy_mux->shift;

Can this use FIELD_PREP?

> +
> +       return regmap_update_bits(clkr->regmap, phy_mux->reg, mask, val);
> +}
> +
> +static void phy_mux_disable(struct clk_hw *hw)
> +{
> +       struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
> +       struct clk_regmap *clkr = to_clk_regmap(hw);
> +       unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
> +       unsigned int val;
> +
> +       val = phy_mux->ref_src_val << phy_mux->shift;
> +
> +       regmap_update_bits(clkr->regmap, phy_mux->reg, mask, val);
> +}
> +
> +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-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.

Remove "special" please. Everything is special :)

> + *
> + * If the clock is running off the from-PHY source, report it as enabled.

from-PHY is @phy_src_val? Maybe add that information like "from-PHY
source (@phy_src_val)"

> + * Report it as disabled otherwise (if it uses reference source).

Same for @ref_src_val

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

Might as well make it into real kernel-doc at the same time.

> + */
> +
> +struct clk_regmap_phy_mux {
> +       u32                     reg;
> +       u32                     shift;
> +       u32                     width;

Technically neither of these need to be u32 and could be u8 to save a
byte or two. The other thing is that possibly the width and shift never
changes? The RCG layout is pretty well fixed. Does hardcoding it work?

> +       u32                     phy_src_val;
> +       u32                     ref_src_val;

I feel like "_val" is redundant. Just "ref_src" and "phy_src"? Shorter
is nice.

> +       struct clk_regmap       clkr;
> +};
> +
> +extern const struct clk_ops clk_regmap_phy_mux_ops;

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

[Dmitry Baryshkov]

On 18/05/2022 20:58, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-05-13 10:53:36)
>> 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..d7a45f7fa1aa
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-regmap-phy-mux.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>
> 
> clk-provider.h for clk_hw/clk_ops usage. It helps with grep to identify
> clk providers.
> 
>> +
>> +#include "clk-regmap-phy-mux.h"
> 
> Same for clk-regmap.h, avoid include hell.
> 
>> +
>> +static inline struct clk_regmap_phy_mux *to_clk_regmap_phy_mux(struct clk_hw *hw)
>> +{
>> +       return container_of(to_clk_regmap(hw), struct clk_regmap_phy_mux, clkr);
>> +}
>> +
>> +static int phy_mux_is_enabled(struct clk_hw *hw)
>> +{
>> +       struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
>> +       struct clk_regmap *clkr = to_clk_regmap(hw);
>> +       unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
>> +       unsigned int val;
>> +
>> +       regmap_read(clkr->regmap, phy_mux->reg, &val);
>> +       val = (val & mask) >> phy_mux->shift;
> 
> Can this use FIELD_GET?
> 
>> +
>> +       WARN_ON(val != phy_mux->phy_src_val && val != phy_mux->ref_src_val);
>> +
>> +       return val == phy_mux->phy_src_val;
>> +}
>> +
>> +static int phy_mux_enable(struct clk_hw *hw)
>> +{
>> +       struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
>> +       struct clk_regmap *clkr = to_clk_regmap(hw);
>> +       unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
>> +       unsigned int val;
>> +
>> +       val = phy_mux->phy_src_val << phy_mux->shift;
> 
> Can this use FIELD_PREP?
> 
>> +
>> +       return regmap_update_bits(clkr->regmap, phy_mux->reg, mask, val);
>> +}
>> +
>> +static void phy_mux_disable(struct clk_hw *hw)
>> +{
>> +       struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
>> +       struct clk_regmap *clkr = to_clk_regmap(hw);
>> +       unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
>> +       unsigned int val;
>> +
>> +       val = phy_mux->ref_src_val << phy_mux->shift;
>> +
>> +       regmap_update_bits(clkr->regmap, phy_mux->reg, mask, val);
>> +}
>> +
>> +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-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.
> 
> Remove "special" please. Everything is special :)

ack for the docs changes, will send shortly.

> 
>> + *
>> + * If the clock is running off the from-PHY source, report it as enabled.
> 
> from-PHY is @phy_src_val? Maybe add that information like "from-PHY
> source (@phy_src_val)"
> 
>> + * Report it as disabled otherwise (if it uses reference source).
> 
> Same for @ref_src_val
> 
>> + *
>> + * 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.
> 
> Might as well make it into real kernel-doc at the same time.
> 
>> + */
>> +
>> +struct clk_regmap_phy_mux {
>> +       u32                     reg;
>> +       u32                     shift;
>> +       u32                     width;
> 
> Technically neither of these need to be u32 and could be u8 to save a
> byte or two. The other thing is that possibly the width and shift never
> changes? The RCG layout is pretty well fixed. Does hardcoding it work?

It seems, I can hardcode shift=0 and width=2.

> 
>> +       u32                     phy_src_val;
>> +       u32                     ref_src_val;
> 
> I feel like "_val" is redundant. Just "ref_src" and "phy_src"? Shorter
> is nice.

I had this since I wanted to point that these are 'values', not the 
enum-ed sources. But I can drop this now.

> 
>> +       struct clk_regmap       clkr;
>> +};
>> +
>> +extern const struct clk_ops clk_regmap_phy_mux_ops;


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 18/05/2022 20:58, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-05-13 10:53:36)
>> 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..d7a45f7fa1aa
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-regmap-phy-mux.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>
> 
> clk-provider.h for clk_hw/clk_ops usage. It helps with grep to identify
> clk providers.
> 
>> +
>> +#include "clk-regmap-phy-mux.h"
> 
> Same for clk-regmap.h, avoid include hell.

I couldn't catch this comment. I think we need clk-regmap.h in 
clk-regmap-phy-mux.h as clk_regmap is a part of defined structure.

>> +
>> +static inline struct clk_regmap_phy_mux *to_clk_regmap_phy_mux(struct clk_hw *hw)
>> +{
>> +       return container_of(to_clk_regmap(hw), struct clk_regmap_phy_mux, clkr);
>> +}
>> +
>> +static int phy_mux_is_enabled(struct clk_hw *hw)
>> +{
>> +       struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
>> +       struct clk_regmap *clkr = to_clk_regmap(hw);
>> +       unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
>> +       unsigned int val;
>> +
>> +       regmap_read(clkr->regmap, phy_mux->reg, &val);
>> +       val = (val & mask) >> phy_mux->shift;
> 
> Can this use FIELD_GET?
> 
>> +
>> +       WARN_ON(val != phy_mux->phy_src_val && val != phy_mux->ref_src_val);
>> +
>> +       return val == phy_mux->phy_src_val;
>> +}
>> +
>> +static int phy_mux_enable(struct clk_hw *hw)
>> +{
>> +       struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
>> +       struct clk_regmap *clkr = to_clk_regmap(hw);
>> +       unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
>> +       unsigned int val;
>> +
>> +       val = phy_mux->phy_src_val << phy_mux->shift;
> 
> Can this use FIELD_PREP?
> 
>> +
>> +       return regmap_update_bits(clkr->regmap, phy_mux->reg, mask, val);
>> +}
>> +
>> +static void phy_mux_disable(struct clk_hw *hw)
>> +{
>> +       struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
>> +       struct clk_regmap *clkr = to_clk_regmap(hw);
>> +       unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
>> +       unsigned int val;
>> +
>> +       val = phy_mux->ref_src_val << phy_mux->shift;
>> +
>> +       regmap_update_bits(clkr->regmap, phy_mux->reg, mask, val);
>> +}
>> +
>> +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-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.
> 
> Remove "special" please. Everything is special :)
> 
>> + *
>> + * If the clock is running off the from-PHY source, report it as enabled.
> 
> from-PHY is @phy_src_val? Maybe add that information like "from-PHY
> source (@phy_src_val)"
> 
>> + * Report it as disabled otherwise (if it uses reference source).
> 
> Same for @ref_src_val
> 
>> + *
>> + * 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.
> 
> Might as well make it into real kernel-doc at the same time.
> 
>> + */
>> +
>> +struct clk_regmap_phy_mux {
>> +       u32                     reg;
>> +       u32                     shift;
>> +       u32                     width;
> 
> Technically neither of these need to be u32 and could be u8 to save a
> byte or two. The other thing is that possibly the width and shift never
> changes? The RCG layout is pretty well fixed. Does hardcoding it work?
> 
>> +       u32                     phy_src_val;
>> +       u32                     ref_src_val;
> 
> I feel like "_val" is redundant. Just "ref_src" and "phy_src"? Shorter
> is nice.
> 
>> +       struct clk_regmap       clkr;
>> +};
>> +
>> +extern const struct clk_ops clk_regmap_phy_mux_ops;


-- 
With best wishes
Dmitry

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

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-05-19 04:16:19)
> On 18/05/2022 20:58, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2022-05-13 10:53:36)
> >> 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..d7a45f7fa1aa
> >> --- /dev/null
> >> +++ b/drivers/clk/qcom/clk-regmap-phy-mux.c
[...]
> >> +
> >> +#include "clk-regmap-phy-mux.h"
> > 
> > Same for clk-regmap.h, avoid include hell.
> 
> I couldn't catch this comment. I think we need clk-regmap.h in 
> clk-regmap-phy-mux.h as clk_regmap is a part of defined structure.
> 

Don't rely on implicit includes. It makes changing header files error
prone. Also, please trim replies.

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

[Dmitry Baryshkov]

On 21/05/2022 01:49, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-05-19 04:16:19)
>> On 18/05/2022 20:58, Stephen Boyd wrote:
>>> Quoting Dmitry Baryshkov (2022-05-13 10:53:36)
>>>> 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..d7a45f7fa1aa
>>>> --- /dev/null
>>>> +++ b/drivers/clk/qcom/clk-regmap-phy-mux.c
> [...]
>>>> +
>>>> +#include "clk-regmap-phy-mux.h"
>>>
>>> Same for clk-regmap.h, avoid include hell.
>>
>> I couldn't catch this comment. I think we need clk-regmap.h in
>> clk-regmap-phy-mux.h as clk_regmap is a part of defined structure.
>>
> 
> Don't rely on implicit includes. It makes changing header files error
> prone. Also, please trim replies.

Ack. Will change this in v8.


-- 
With best wishes
Dmitry

[PATCH v6 3/5] clk: qcom: gcc-sm8450: use new clk_regmap_pipe_src_ops for PCIe pipe clocks

[Dmitry Baryshkov]

Use newly defined clk_regmap_pipe_src_ops for PCIe pipe clocks to let
the clock framework automatically park the clock when the clock is
switched off and restore the parent when the clock is switched on.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/gcc-sm8450.c | 51 +++++++++++++----------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sm8450.c b/drivers/clk/qcom/gcc-sm8450.c
index 593a195467ff..a140a89b73b4 100644
--- a/drivers/clk/qcom/gcc-sm8450.c
+++ b/drivers/clk/qcom/gcc-sm8450.c
@@ -17,6 +17,7 @@
 #include "clk-regmap.h"
 #include "clk-regmap-divider.h"
 #include "clk-regmap-mux.h"
+#include "clk-regmap-phy-mux.h"
 #include "gdsc.h"
 #include "reset.h"
 
@@ -26,9 +27,7 @@ enum {
 	P_GCC_GPLL0_OUT_MAIN,
 	P_GCC_GPLL4_OUT_MAIN,
 	P_GCC_GPLL9_OUT_MAIN,
-	P_PCIE_0_PIPE_CLK,
 	P_PCIE_1_PHY_AUX_CLK,
-	P_PCIE_1_PIPE_CLK,
 	P_SLEEP_CLK,
 	P_UFS_PHY_RX_SYMBOL_0_CLK,
 	P_UFS_PHY_RX_SYMBOL_1_CLK,
@@ -153,16 +152,6 @@ static const struct clk_parent_data gcc_parent_data_3[] = {
 	{ .fw_name = "bi_tcxo" },
 };
 
-static const struct parent_map gcc_parent_map_4[] = {
-	{ P_PCIE_0_PIPE_CLK, 0 },
-	{ P_BI_TCXO, 2 },
-};
-
-static const struct clk_parent_data gcc_parent_data_4[] = {
-	{ .fw_name = "pcie_0_pipe_clk", },
-	{ .fw_name = "bi_tcxo", },
-};
-
 static const struct parent_map gcc_parent_map_5[] = {
 	{ P_PCIE_1_PHY_AUX_CLK, 0 },
 	{ P_BI_TCXO, 2 },
@@ -173,16 +162,6 @@ static const struct clk_parent_data gcc_parent_data_5[] = {
 	{ .fw_name = "bi_tcxo" },
 };
 
-static const struct parent_map gcc_parent_map_6[] = {
-	{ P_PCIE_1_PIPE_CLK, 0 },
-	{ P_BI_TCXO, 2 },
-};
-
-static const struct clk_parent_data gcc_parent_data_6[] = {
-	{ .fw_name = "pcie_1_pipe_clk" },
-	{ .fw_name = "bi_tcxo" },
-};
-
 static const struct parent_map gcc_parent_map_7[] = {
 	{ P_BI_TCXO, 0 },
 	{ P_GCC_GPLL0_OUT_MAIN, 1 },
@@ -239,17 +218,21 @@ static const struct clk_parent_data gcc_parent_data_11[] = {
 	{ .fw_name = "bi_tcxo" },
 };
 
-static struct clk_regmap_mux gcc_pcie_0_pipe_clk_src = {
+static struct clk_regmap_phy_mux gcc_pcie_0_pipe_clk_src = {
 	.reg = 0x7b060,
 	.shift = 0,
 	.width = 2,
-	.parent_map = gcc_parent_map_4,
+	.phy_src_val = 0, /* pipe_clk */
+	.ref_src_val = 2, /* bi_tcxo */
 	.clkr = {
 		.hw.init = &(struct clk_init_data){
 			.name = "gcc_pcie_0_pipe_clk_src",
-			.parent_data = gcc_parent_data_4,
-			.num_parents = ARRAY_SIZE(gcc_parent_data_4),
-			.ops = &clk_regmap_mux_closest_ops,
+			.parent_data = &(const struct clk_parent_data){
+				.fw_name = "pcie_0_pipe_clk",
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_regmap_phy_mux_ops,
 		},
 	},
 };
@@ -269,17 +252,21 @@ static struct clk_regmap_mux gcc_pcie_1_phy_aux_clk_src = {
 	},
 };
 
-static struct clk_regmap_mux gcc_pcie_1_pipe_clk_src = {
+static struct clk_regmap_phy_mux gcc_pcie_1_pipe_clk_src = {
 	.reg = 0x9d064,
 	.shift = 0,
 	.width = 2,
-	.parent_map = gcc_parent_map_6,
+	.phy_src_val = 0, /* pipe_clk */
+	.ref_src_val = 2, /* bi_tcxo */
 	.clkr = {
 		.hw.init = &(struct clk_init_data){
 			.name = "gcc_pcie_1_pipe_clk_src",
-			.parent_data = gcc_parent_data_6,
-			.num_parents = ARRAY_SIZE(gcc_parent_data_6),
-			.ops = &clk_regmap_mux_closest_ops,
+			.parent_data = &(const struct clk_parent_data){
+				.fw_name = "pcie_1_pipe_clk",
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_regmap_phy_mux_ops,
 		},
 	},
 };
-- 
2.35.1

Re: [PATCH v6 3/5] clk: qcom: gcc-sm8450: use new clk_regmap_pipe_src_ops for PCIe pipe clocks

[Johan Hovold]

On Fri, May 13, 2022 at 08:53:37PM +0300, Dmitry Baryshkov wrote:
> Use newly defined clk_regmap_pipe_src_ops for PCIe pipe clocks to let

clk_regmap_phy_mux

> the clock framework automatically park the clock when the clock is
> switched off and restore the parent when the clock is switched on.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Looks good otherwise:

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

Re: [PATCH v6 3/5] clk: qcom: gcc-sm8450: use new clk_regmap_pipe_src_ops for PCIe pipe clocks

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-05-13 10:53:37)
> diff --git a/drivers/clk/qcom/gcc-sm8450.c b/drivers/clk/qcom/gcc-sm8450.c
> index 593a195467ff..a140a89b73b4 100644
> --- a/drivers/clk/qcom/gcc-sm8450.c
> +++ b/drivers/clk/qcom/gcc-sm8450.c
> @@ -239,17 +218,21 @@ static const struct clk_parent_data gcc_parent_data_11[] = {
>         { .fw_name = "bi_tcxo" },
>  };
>
> -static struct clk_regmap_mux gcc_pcie_0_pipe_clk_src = {
> +static struct clk_regmap_phy_mux gcc_pcie_0_pipe_clk_src = {
>         .reg = 0x7b060,
>         .shift = 0,
>         .width = 2,
> -       .parent_map = gcc_parent_map_4,
> +       .phy_src_val = 0, /* pipe_clk */

Make a define? PCIE0_PIPE_CLK_SRC_VAL and drop the comment?

> +       .ref_src_val = 2, /* bi_tcxo */
>         .clkr = {
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_pcie_0_pipe_clk_src",
> -                       .parent_data = gcc_parent_data_4,
> -                       .num_parents = ARRAY_SIZE(gcc_parent_data_4),
> -                       .ops = &clk_regmap_mux_closest_ops,
> +                       .parent_data = &(const struct clk_parent_data){
> +                               .fw_name = "pcie_0_pipe_clk",
> +                       },
> +                       .num_parents = 1,
> +                       .flags = CLK_SET_RATE_PARENT,
> +                       .ops = &clk_regmap_phy_mux_ops,
>                 },
>         },
>  };

Re: [PATCH v6 3/5] clk: qcom: gcc-sm8450: use new clk_regmap_pipe_src_ops for PCIe pipe clocks

[Dmitry Baryshkov]

On 18/05/2022 20:59, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-05-13 10:53:37)
>> diff --git a/drivers/clk/qcom/gcc-sm8450.c b/drivers/clk/qcom/gcc-sm8450.c
>> index 593a195467ff..a140a89b73b4 100644
>> --- a/drivers/clk/qcom/gcc-sm8450.c
>> +++ b/drivers/clk/qcom/gcc-sm8450.c
>> @@ -239,17 +218,21 @@ static const struct clk_parent_data gcc_parent_data_11[] = {
>>          { .fw_name = "bi_tcxo" },
>>   };
>>
>> -static struct clk_regmap_mux gcc_pcie_0_pipe_clk_src = {
>> +static struct clk_regmap_phy_mux gcc_pcie_0_pipe_clk_src = {
>>          .reg = 0x7b060,
>>          .shift = 0,
>>          .width = 2,
>> -       .parent_map = gcc_parent_map_4,
>> +       .phy_src_val = 0, /* pipe_clk */
> 
> Make a define? PCIE0_PIPE_CLK_SRC_VAL and drop the comment?

This value can change between the muxes. Thus I'd prefer not to do this.
Compare it with the parent_maps, where we do not use defines for the 
'val' part.

> 
>> +       .ref_src_val = 2, /* bi_tcxo */
>>          .clkr = {
>>                  .hw.init = &(struct clk_init_data){
>>                          .name = "gcc_pcie_0_pipe_clk_src",
>> -                       .parent_data = gcc_parent_data_4,
>> -                       .num_parents = ARRAY_SIZE(gcc_parent_data_4),
>> -                       .ops = &clk_regmap_mux_closest_ops,
>> +                       .parent_data = &(const struct clk_parent_data){
>> +                               .fw_name = "pcie_0_pipe_clk",
>> +                       },
>> +                       .num_parents = 1,
>> +                       .flags = CLK_SET_RATE_PARENT,
>> +                       .ops = &clk_regmap_phy_mux_ops,
>>                  },
>>          },
>>   };


-- 
With best wishes
Dmitry

Re: [PATCH v6 3/5] clk: qcom: gcc-sm8450: use new clk_regmap_pipe_src_ops for PCIe pipe clocks

[Stephen Boyd]

- bouncing Prasad

Quoting Dmitry Baryshkov (2022-05-18 11:26:16)
> On 18/05/2022 20:59, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2022-05-13 10:53:37)
> >> diff --git a/drivers/clk/qcom/gcc-sm8450.c b/drivers/clk/qcom/gcc-sm8450.c
> >> index 593a195467ff..a140a89b73b4 100644
> >> --- a/drivers/clk/qcom/gcc-sm8450.c
> >> +++ b/drivers/clk/qcom/gcc-sm8450.c
> >> @@ -239,17 +218,21 @@ static const struct clk_parent_data gcc_parent_data_11[] = {
> >>          { .fw_name = "bi_tcxo" },
> >>   };
> >>
> >> -static struct clk_regmap_mux gcc_pcie_0_pipe_clk_src = {
> >> +static struct clk_regmap_phy_mux gcc_pcie_0_pipe_clk_src = {
> >>          .reg = 0x7b060,
> >>          .shift = 0,
> >>          .width = 2,
> >> -       .parent_map = gcc_parent_map_4,
> >> +       .phy_src_val = 0, /* pipe_clk */
> > 
> > Make a define? PCIE0_PIPE_CLK_SRC_VAL and drop the comment?
> 
> This value can change between the muxes. Thus I'd prefer not to do this.
> Compare it with the parent_maps, where we do not use defines for the 
> 'val' part.
> 

We don't have defines for the parent maps because they have defines for
the other side.

[PATCH v6 4/5] clk: qcom: gcc-sc7280: use new clk_regmap_pipe_src_ops for PCIe pipe clocks

[Dmitry Baryshkov]

Use newly defined clk_regmap_pipe_src_ops for PCIe pipe clocks to let
the clock framework automatically park the clock when the clock is
switched off and restore the parent when the clock is switched on.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/gcc-sc7280.c | 49 ++++++++++++++---------------------
 1 file changed, 19 insertions(+), 30 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
index 423627d49719..05589ddefcde 100644
--- a/drivers/clk/qcom/gcc-sc7280.c
+++ b/drivers/clk/qcom/gcc-sc7280.c
@@ -17,6 +17,7 @@
 #include "clk-rcg.h"
 #include "clk-regmap-divider.h"
 #include "clk-regmap-mux.h"
+#include "clk-regmap-phy-mux.h"
 #include "common.h"
 #include "gdsc.h"
 #include "reset.h"
@@ -255,26 +256,6 @@ static const struct clk_parent_data gcc_parent_data_5[] = {
 	{ .hw = &gcc_gpll0_out_even.clkr.hw },
 };
 
-static const struct parent_map gcc_parent_map_6[] = {
-	{ P_PCIE_0_PIPE_CLK, 0 },
-	{ P_BI_TCXO, 2 },
-};
-
-static const struct clk_parent_data gcc_parent_data_6[] = {
-	{ .fw_name = "pcie_0_pipe_clk", .name = "pcie_0_pipe_clk" },
-	{ .fw_name = "bi_tcxo" },
-};
-
-static const struct parent_map gcc_parent_map_7[] = {
-	{ P_PCIE_1_PIPE_CLK, 0 },
-	{ P_BI_TCXO, 2 },
-};
-
-static const struct clk_parent_data gcc_parent_data_7[] = {
-	{ .fw_name = "pcie_1_pipe_clk", .name = "pcie_1_pipe_clk" },
-	{ .fw_name = "bi_tcxo" },
-};
-
 static const struct parent_map gcc_parent_map_8[] = {
 	{ P_BI_TCXO, 0 },
 	{ P_GCC_GPLL0_OUT_MAIN, 1 },
@@ -369,32 +350,40 @@ static const struct clk_parent_data gcc_parent_data_15[] = {
 	{ .hw = &gcc_mss_gpll0_main_div_clk_src.clkr.hw },
 };
 
-static struct clk_regmap_mux gcc_pcie_0_pipe_clk_src = {
+static struct clk_regmap_phy_mux gcc_pcie_0_pipe_clk_src = {
 	.reg = 0x6b054,
 	.shift = 0,
 	.width = 2,
-	.parent_map = gcc_parent_map_6,
+	.phy_src_val = 0, /* pipe_clk */
+	.ref_src_val = 2, /* bi_tcxo */
 	.clkr = {
 		.hw.init = &(struct clk_init_data){
 			.name = "gcc_pcie_0_pipe_clk_src",
-			.parent_data = gcc_parent_data_6,
-			.num_parents = ARRAY_SIZE(gcc_parent_data_6),
-			.ops = &clk_regmap_mux_closest_ops,
+			.parent_data = &(const struct clk_parent_data){
+				.fw_name = "pcie_0_pipe_clk",
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_regmap_phy_mux_ops,
 		},
 	},
 };
 
-static struct clk_regmap_mux gcc_pcie_1_pipe_clk_src = {
+static struct clk_regmap_phy_mux gcc_pcie_1_pipe_clk_src = {
 	.reg = 0x8d054,
 	.shift = 0,
 	.width = 2,
-	.parent_map = gcc_parent_map_7,
+	.phy_src_val = 0, /* pipe_clk */
+	.ref_src_val = 2, /* bi_tcxo */
 	.clkr = {
 		.hw.init = &(struct clk_init_data){
 			.name = "gcc_pcie_1_pipe_clk_src",
-			.parent_data = gcc_parent_data_7,
-			.num_parents = ARRAY_SIZE(gcc_parent_data_7),
-			.ops = &clk_regmap_mux_closest_ops,
+			.parent_data = &(const struct clk_parent_data){
+				.fw_name = "pcie_1_pipe_clk",
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_regmap_phy_mux_ops,
 		},
 	},
 };
-- 
2.35.1

Re: [PATCH v6 4/5] clk: qcom: gcc-sc7280: use new clk_regmap_pipe_src_ops for PCIe pipe clocks

[Johan Hovold]

On Fri, May 13, 2022 at 08:53:38PM +0300, Dmitry Baryshkov wrote:
> Use newly defined clk_regmap_pipe_src_ops for PCIe pipe clocks to let

clk_regmap_phy_mux

> the clock framework automatically park the clock when the clock is
> switched off and restore the parent when the clock is switched on.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Looks good otherwise:

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

[PATCH v6 5/5] PCI: qcom: Drop manual pipe_clk_src handling

[Dmitry Baryshkov]

Manual reparenting of pipe_clk_src is being replaced with the parking of
the clock with clk_disable()/clk_enable() in the phy driver. Drop
redundant code switching of the pipe clock between the PHY clock source
and the safe bi_tcxo.

Cc: Prasad Malisetty <quic_pmaliset@quicinc.com>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 39 +-------------------------
 1 file changed, 1 insertion(+), 38 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index a6becafb6a77..b48c899bcc97 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -164,9 +164,6 @@ struct qcom_pcie_resources_2_7_0 {
 	int num_clks;
 	struct regulator_bulk_data supplies[2];
 	struct reset_control *pci_reset;
-	struct clk *pipe_clk_src;
-	struct clk *phy_pipe_clk;
-	struct clk *ref_clk_src;
 };
 
 union qcom_pcie_resources {
@@ -192,7 +189,6 @@ struct qcom_pcie_ops {
 
 struct qcom_pcie_cfg {
 	const struct qcom_pcie_ops *ops;
-	unsigned int pipe_clk_need_muxing:1;
 	unsigned int has_tbu_clk:1;
 	unsigned int has_ddrss_sf_tbu_clk:1;
 	unsigned int has_aggre0_clk:1;
@@ -1158,20 +1154,6 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 	if (ret < 0)
 		return ret;
 
-	if (pcie->cfg->pipe_clk_need_muxing) {
-		res->pipe_clk_src = devm_clk_get(dev, "pipe_mux");
-		if (IS_ERR(res->pipe_clk_src))
-			return PTR_ERR(res->pipe_clk_src);
-
-		res->phy_pipe_clk = devm_clk_get(dev, "phy_pipe");
-		if (IS_ERR(res->phy_pipe_clk))
-			return PTR_ERR(res->phy_pipe_clk);
-
-		res->ref_clk_src = devm_clk_get(dev, "ref");
-		if (IS_ERR(res->ref_clk_src))
-			return PTR_ERR(res->ref_clk_src);
-	}
-
 	return 0;
 }
 
@@ -1189,10 +1171,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 		return ret;
 	}
 
-	/* Set TCXO as clock source for pcie_pipe_clk_src */
-	if (pcie->cfg->pipe_clk_need_muxing)
-		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
-
 	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
 	if (ret < 0)
 		goto err_disable_regulators;
@@ -1254,18 +1232,8 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
 
 	clk_bulk_disable_unprepare(res->num_clks, res->clks);
-	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
-}
 
-static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
-{
-	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
-
-	/* Set pipe clock as clock source for pcie_pipe_clk_src */
-	if (pcie->cfg->pipe_clk_need_muxing)
-		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
-
-	return 0;
+	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
 }
 
 static int qcom_pcie_link_up(struct dw_pcie *pci)
@@ -1441,7 +1409,6 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
 	.init = qcom_pcie_init_2_7_0,
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
-	.post_init = qcom_pcie_post_init_2_7_0,
 };
 
 /* Qcom IP rev.: 1.9.0 */
@@ -1450,7 +1417,6 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.init = qcom_pcie_init_2_7_0,
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
-	.post_init = qcom_pcie_post_init_2_7_0,
 	.config_sid = qcom_pcie_config_sid_sm8250,
 };
 
@@ -1488,7 +1454,6 @@ static const struct qcom_pcie_cfg sm8250_cfg = {
 static const struct qcom_pcie_cfg sm8450_pcie0_cfg = {
 	.ops = &ops_1_9_0,
 	.has_ddrss_sf_tbu_clk = true,
-	.pipe_clk_need_muxing = true,
 	.has_aggre0_clk = true,
 	.has_aggre1_clk = true,
 };
@@ -1496,14 +1461,12 @@ static const struct qcom_pcie_cfg sm8450_pcie0_cfg = {
 static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
 	.ops = &ops_1_9_0,
 	.has_ddrss_sf_tbu_clk = true,
-	.pipe_clk_need_muxing = true,
 	.has_aggre1_clk = true,
 };
 
 static const struct qcom_pcie_cfg sc7280_cfg = {
 	.ops = &ops_1_9_0,
 	.has_tbu_clk = true,
-	.pipe_clk_need_muxing = true,
 };
 
 static const struct dw_pcie_ops dw_pcie_ops = {
-- 
2.35.1

Re: [PATCH v6 5/5] PCI: qcom: Drop manual pipe_clk_src handling

[Johan Hovold]

On Fri, May 13, 2022 at 08:53:39PM +0300, Dmitry Baryshkov wrote:
> Manual reparenting of pipe_clk_src is being replaced with the parking of
> the clock with clk_disable()/clk_enable() in the phy driver. Drop
> redundant code switching of the pipe clock between the PHY clock source
> and the safe bi_tcxo.

Thanks for updating the commit message.

> Cc: Prasad Malisetty <quic_pmaliset@quicinc.com>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 39 +-------------------------
>  1 file changed, 1 insertion(+), 38 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index a6becafb6a77..b48c899bcc97 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c

> @@ -1496,14 +1461,12 @@ static const struct qcom_pcie_cfg sm8450_pcie0_cfg = {
>  static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
>  	.ops = &ops_1_9_0,
>  	.has_ddrss_sf_tbu_clk = true,
> -	.pipe_clk_need_muxing = true,
>  	.has_aggre1_clk = true,
>  };
>  
>  static const struct qcom_pcie_cfg sc7280_cfg = {
>  	.ops = &ops_1_9_0,
>  	.has_tbu_clk = true,
> -	.pipe_clk_need_muxing = true,
>  };
>  
>  static const struct dw_pcie_ops dw_pcie_ops = {

Note that this hunk fails to apply due to commit 134b5ce3ed33 ("PCI:
qcom: Remove ddrss_sf_tbu clock from SC8180X").

Fixing it up is trivial but still.

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

Johan

Re: [PATCH v6 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling

[Johan Hovold]

On Fri, May 13, 2022 at 08:53:34PM +0300, Dmitry Baryshkov wrote:
> PCIe pipe clk (and some other clocks) 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.
> 
> Instead of doing it manually, follow the approach used by
> clk_rcg2_shared_ops and implement this parking in the enable() and
> disable() clock operations for respective pipe clocks.
> 
> Changes since v5:
>  - Rename the clock to clk-regmap-phy-mux and the enable/disable values
>    to phy_src_val and ref_src_val respectively (as recommended by
>    Johan).
> 
> Changes since v4:
>  - Renamed the clock to clk-regmap-pipe-src,
>  - Added mention of PCIe2 PHY to the commit message,
>  - Expanded commit messages to mention additional pipe clock details.
> 
> Changes since v3:
>  - Replaced the clock multiplexer implementation with branch-like clock.
> 
> Changes since v2:
>  - Added is_enabled() callback
>  - Added default parent to the pipe clock configuration
> 
> Changes since v1:
>  - Rebased on top of [1].
>  - Removed erroneous Fixes tag from the patch 4.
> 
> Changes since RFC:
>  - Rework clk-regmap-mux fields. Specify safe parent as P_* value rather
>    than specifying the register value directly
>  - Expand commit message to the first patch to specially mention that
>    it is required only on newer generations of Qualcomm chipsets.
> 
> 
> Dmitry Baryshkov (5):
>   PCI: qcom: Remove unnecessary pipe_clk handling
>   clk: qcom: regmap: add PHY clock source implementation
>   clk: qcom: gcc-sm8450: use new clk_regmap_pipe_src_ops for PCIe pipe
>     clocks
>   clk: qcom: gcc-sc7280: use new clk_regmap_pipe_src_ops for PCIe pipe
>     clocks
>   PCI: qcom: Drop manual pipe_clk_src handling

So Bjorn A has already applied v2 of the three clock patches this series
to his tree. I guess dropping or reverting does is the best way to
handle this since trying to fix things up incrementally would just be
messy.

Johan