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

[PATCH v4 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.

PCIe part depends on [1].

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.

[1]: https://lore.kernel.org/all/20220401133351.10113-1-johan+linaro@kernel.org/

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

 drivers/clk/qcom/Makefile              |  1 +
 drivers/clk/qcom/clk-regmap-pipe.c     | 62 ++++++++++++++++++++
 drivers/clk/qcom/clk-regmap-pipe.h     | 24 ++++++++
 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, 128 insertions(+), 140 deletions(-)
 create mode 100644 drivers/clk/qcom/clk-regmap-pipe.c
 create mode 100644 drivers/clk/qcom/clk-regmap-pipe.h

-- 
2.35.1

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

[Dmitry Baryshkov]

QMP PHY driver already does clk_prepare_enable()/_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>
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 v4 1/5] PCI: qcom: Remove unnecessary pipe_clk handling

[Manivannan Sadhasivam]

On Sun, May 01, 2022 at 10:21:45PM +0300, Dmitry Baryshkov wrote:
> QMP PHY driver already does clk_prepare_enable()/_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>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  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 v4 1/5] PCI: qcom: Remove unnecessary pipe_clk handling

[Johan Hovold]

On Sun, May 01, 2022 at 10:21:45PM +0300, Dmitry Baryshkov wrote:
> QMP PHY driver already does clk_prepare_enable()/_disable() pipe_clk.

The pcie-qcom driver is used also with non-QMP PHYs such as the Qualcomm
PCIe2 PHY so please expand the commit message here explaining why this
is safe to remove.

> Remove extra calls to enable/disable this clock from the PCIe driver, so
> that the PHY driver can manage the clock on its own.

Johan

[PATCH v4 2/5] clk: qcom: regmap: add pipe clk 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. Suppliement the regmap-mux with
the new regmap-pipe implementation, which hides multiplexer behind
simple branch-like clock. 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);
+}
+
+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;
+
+	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;
+	struct clk_regmap	clkr;
+};
+
+extern const struct clk_ops clk_regmap_pipe_ops;
+
+#endif
-- 
2.35.1

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

[Manivannan Sadhasivam]

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.
> 
> 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
> the new regmap-pipe implementation, which hides multiplexer behind
> simple branch-like clock. 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.
> 

Sorry for chiming in late and providing comments that might have been addressed
before. But I have few questions below:

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

Selecting the clk parents in the enable/disable callback seems fine to me but
the way it is implemented doesn't look right.

First this "pipe_clksrc" is a mux clk by design, since we can only select the
parent. But you are converting it to a gate clk now.

Instead of that, my proposal would be to make this clk a composite one i.e,.
gate clk + mux clk. So even though the gate clk here would be a hack, we are
not changing the definition of mux clk.

So you can introduce a new ops like "clk_regmap_mux_gate_ops" and implement the
parent switching logic in the enable/disable callbacks. Additional benefit of
this ops is, in the future we can also support "gate + mux" clks easily.

Also, please don't use the "enable_val/disable_val" members. It should be
something like "mux_sel_pre/mux_sel_post".

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

Linaro

> + * Author: Dmitry Baryshkov

No email?

Thanks,
Mani

> + */
> +
> +#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;
> +	struct clk_regmap	clkr;
> +};
> +
> +extern const struct clk_ops clk_regmap_pipe_ops;
> +
> +#endif
> -- 
> 2.35.1
> 

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

[Dmitry Baryshkov]

On 02/05/2022 13:10, Manivannan Sadhasivam wrote:
> 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.
>>
>> 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
>> the new regmap-pipe implementation, which hides multiplexer behind
>> simple branch-like clock. 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.
>>
> 
> Sorry for chiming in late and providing comments that might have been addressed
> before. But I have few questions below:
> 
>> 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;
> 
> Selecting the clk parents in the enable/disable callback seems fine to me but
> the way it is implemented doesn't look right.
> 
> First this "pipe_clksrc" is a mux clk by design, since we can only select the
> parent. But you are converting it to a gate clk now.
> 
> Instead of that, my proposal would be to make this clk a composite one i.e,.
> gate clk + mux clk. So even though the gate clk here would be a hack, we are
> not changing the definition of mux clk.

This is what I had before, in revisions 1-3. Which proved to work, but 
is problematic a bit.

In the very end, it is not easily possible to make a difference between 
a clock reparented to the bi_tcxo and a disabled clock. E.g. if some 
user reparents the clock to the tcxo, then the driver will consider the 
clock disabled, but the clock framework will think that the clock is 
still enabled.

Thus we have to remove "safe" clock (bi_tcxo) from the list of parents. 
In case of pipe clocks (and ufs symbol clocks) this will leave us with 
just a single possible parent. Then having the mux part just doesn't 
make sense. It is just a gated clock. And this simplified a lot of things.

> 
> So you can introduce a new ops like "clk_regmap_mux_gate_ops" and implement the
> parent switching logic in the enable/disable callbacks. Additional benefit of
> this ops is, in the future we can also support "gate + mux" clks easily.

If the need arises, we can easily resurrect the regmap_mux_safe 
patchset, fix the race pointed out by Johan, remove extra src-val 
mapping for safe value and use it for such clocks. I can post it 
separately, if you wish. But I'm not sure that it makes sense to use it 
for single-parent clocks.

> 
> Also, please don't use the "enable_val/disable_val" members. It should be
> something like "mux_sel_pre/mux_sel_post".

Why? Could you please elaborate?

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

ack

> 
>> + * Author: Dmitry Baryshkov
> 
> No email?
> 
> Thanks,
> Mani
> 
>> + */
>> +
>> +#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;
>> +	struct clk_regmap	clkr;
>> +};
>> +
>> +extern const struct clk_ops clk_regmap_pipe_ops;
>> +
>> +#endif
>> -- 
>> 2.35.1
>>


-- 
With best wishes
Dmitry

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

[Manivannan Sadhasivam]

On Mon, May 02, 2022 at 01:35:34PM +0300, Dmitry Baryshkov wrote:

[...]

> > > +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;
> > 
> > Selecting the clk parents in the enable/disable callback seems fine to me but
> > the way it is implemented doesn't look right.
> > 
> > First this "pipe_clksrc" is a mux clk by design, since we can only select the
> > parent. But you are converting it to a gate clk now.
> > 
> > Instead of that, my proposal would be to make this clk a composite one i.e,.
> > gate clk + mux clk. So even though the gate clk here would be a hack, we are
> > not changing the definition of mux clk.
> 
> This is what I had before, in revisions 1-3. Which proved to work, but is
> problematic a bit.
> 
> In the very end, it is not easily possible to make a difference between a
> clock reparented to the bi_tcxo and a disabled clock. E.g. if some user
> reparents the clock to the tcxo, then the driver will consider the clock
> disabled, but the clock framework will think that the clock is still
> enabled.

I don't understand this. How can you make this clock disabled? It just has 4
parents, right?

> 
> Thus we have to remove "safe" clock (bi_tcxo) from the list of parents. In
> case of pipe clocks (and ufs symbol clocks) this will leave us with just a
> single possible parent. Then having the mux part just doesn't make sense. It
> is just a gated clock. And this simplified a lot of things.
> 
> > 
> > So you can introduce a new ops like "clk_regmap_mux_gate_ops" and implement the
> > parent switching logic in the enable/disable callbacks. Additional benefit of
> > this ops is, in the future we can also support "gate + mux" clks easily.
> 
> If the need arises, we can easily resurrect the regmap_mux_safe patchset,
> fix the race pointed out by Johan, remove extra src-val mapping for safe
> value and use it for such clocks. I can post it separately, if you wish. But
> I'm not sure that it makes sense to use it for single-parent clocks.
> 
> > 
> > Also, please don't use the "enable_val/disable_val" members. It should be
> > something like "mux_sel_pre/mux_sel_post".
> 
> Why? Could you please elaborate?
> 

It aligns with my question above. I don't see how this clk can be
enabled/disabled.

Thanks,
Mani

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

[Dmitry Baryshkov]

On 02/05/2022 14:10, Manivannan Sadhasivam wrote:
> On Mon, May 02, 2022 at 01:35:34PM +0300, Dmitry Baryshkov wrote:
> 
> [...]
> 
>>>> +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;
>>>
>>> Selecting the clk parents in the enable/disable callback seems fine to me but
>>> the way it is implemented doesn't look right.
>>>
>>> First this "pipe_clksrc" is a mux clk by design, since we can only select the
>>> parent. But you are converting it to a gate clk now.
>>>
>>> Instead of that, my proposal would be to make this clk a composite one i.e,.
>>> gate clk + mux clk. So even though the gate clk here would be a hack, we are
>>> not changing the definition of mux clk.
>>
>> This is what I had before, in revisions 1-3. Which proved to work, but is
>> problematic a bit.
>>
>> In the very end, it is not easily possible to make a difference between a
>> clock reparented to the bi_tcxo and a disabled clock. E.g. if some user
>> reparents the clock to the tcxo, then the driver will consider the clock
>> disabled, but the clock framework will think that the clock is still
>> enabled.
> 
> I don't understand this. How can you make this clock disabled? It just has 4
> parents, right?

It has 4 parents. It uses just two of them (pipe and tcxo).

And like the clk_rcg2_safe clock we'd like to say that these clocks are 
disabled by reparenting ("parking") them to the tcxo source. Yes, this 
makes a lot of code simpler. The clock framework will switch the clock 
to the "safe" state instead of disabling it during the unused clocks 
evaporation. The PHY can just disable the gcc_pcie_N_pipe_clock, which 
will end up in parking this clock to a safe state too, etc.

> 
>>
>> Thus we have to remove "safe" clock (bi_tcxo) from the list of parents. In
>> case of pipe clocks (and ufs symbol clocks) this will leave us with just a
>> single possible parent. Then having the mux part just doesn't make sense. It
>> is just a gated clock. And this simplified a lot of things.
>>
>>>
>>> So you can introduce a new ops like "clk_regmap_mux_gate_ops" and implement the
>>> parent switching logic in the enable/disable callbacks. Additional benefit of
>>> this ops is, in the future we can also support "gate + mux" clks easily.
>>
>> If the need arises, we can easily resurrect the regmap_mux_safe patchset,
>> fix the race pointed out by Johan, remove extra src-val mapping for safe
>> value and use it for such clocks. I can post it separately, if you wish. But
>> I'm not sure that it makes sense to use it for single-parent clocks.
>>
>>>
>>> Also, please don't use the "enable_val/disable_val" members. It should be
>>> something like "mux_sel_pre/mux_sel_post".
>>
>> Why? Could you please elaborate?
>>
> 
> It aligns with my question above. I don't see how this clk can be
> enabled/disabled.

I see. Let's settle on the first question then.

-- 
With best wishes
Dmitry

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

[Manivannan Sadhasivam]

On Mon, May 02, 2022 at 02:18:26PM +0300, Dmitry Baryshkov wrote:
> On 02/05/2022 14:10, Manivannan Sadhasivam wrote:
> > On Mon, May 02, 2022 at 01:35:34PM +0300, Dmitry Baryshkov wrote:
> > 
> > [...]
> > 
> > > > > +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;
> > > > 
> > > > Selecting the clk parents in the enable/disable callback seems fine to me but
> > > > the way it is implemented doesn't look right.
> > > > 
> > > > First this "pipe_clksrc" is a mux clk by design, since we can only select the
> > > > parent. But you are converting it to a gate clk now.
> > > > 
> > > > Instead of that, my proposal would be to make this clk a composite one i.e,.
> > > > gate clk + mux clk. So even though the gate clk here would be a hack, we are
> > > > not changing the definition of mux clk.
> > > 
> > > This is what I had before, in revisions 1-3. Which proved to work, but is
> > > problematic a bit.
> > > 
> > > In the very end, it is not easily possible to make a difference between a
> > > clock reparented to the bi_tcxo and a disabled clock. E.g. if some user
> > > reparents the clock to the tcxo, then the driver will consider the clock
> > > disabled, but the clock framework will think that the clock is still
> > > enabled.
> > 
> > I don't understand this. How can you make this clock disabled? It just has 4
> > parents, right?
> 
> It has 4 parents. It uses just two of them (pipe and tcxo).
> 
> And like the clk_rcg2_safe clock we'd like to say that these clocks are
> disabled by reparenting ("parking") them to the tcxo source. Yes, this makes
> a lot of code simpler. The clock framework will switch the clock to the
> "safe" state instead of disabling it during the unused clocks evaporation.
> The PHY can just disable the gcc_pcie_N_pipe_clock, which will end up in
> parking this clock to a safe state too, etc.

If I get the logic behind this "parking" thing right, then it is required
for producing a stable pipe_clk from GCC when the PHY is about to initialize.
Also to make sure that there is no glitch observed on pipe_clk while
initializing the PHY. And once it is powered ON properly, the pipe_clksrc
should be used as the parent for pipe_clk.

So with that logic, we cannot say that this clk is disabled.

Please correct me if my understanding is wrong.

Thanks,
Mani

> 
> > 
> > > 
> > > Thus we have to remove "safe" clock (bi_tcxo) from the list of parents. In
> > > case of pipe clocks (and ufs symbol clocks) this will leave us with just a
> > > single possible parent. Then having the mux part just doesn't make sense. It
> > > is just a gated clock. And this simplified a lot of things.
> > > 
> > > > 
> > > > So you can introduce a new ops like "clk_regmap_mux_gate_ops" and implement the
> > > > parent switching logic in the enable/disable callbacks. Additional benefit of
> > > > this ops is, in the future we can also support "gate + mux" clks easily.
> > > 
> > > If the need arises, we can easily resurrect the regmap_mux_safe patchset,
> > > fix the race pointed out by Johan, remove extra src-val mapping for safe
> > > value and use it for such clocks. I can post it separately, if you wish. But
> > > I'm not sure that it makes sense to use it for single-parent clocks.
> > > 
> > > > 
> > > > Also, please don't use the "enable_val/disable_val" members. It should be
> > > > something like "mux_sel_pre/mux_sel_post".
> > > 
> > > Why? Could you please elaborate?
> > > 
> > 
> > It aligns with my question above. I don't see how this clk can be
> > enabled/disabled.
> 
> I see. Let's settle on the first question then.
> 
> -- 
> With best wishes
> Dmitry

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

[Dmitry Baryshkov]

On 02/05/2022 18:06, Manivannan Sadhasivam wrote:
> On Mon, May 02, 2022 at 02:18:26PM +0300, Dmitry Baryshkov wrote:
>> On 02/05/2022 14:10, Manivannan Sadhasivam wrote:
>>> On Mon, May 02, 2022 at 01:35:34PM +0300, Dmitry Baryshkov wrote:
>>>
>>> [...]
>>>
>>>>>> +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;
>>>>>
>>>>> Selecting the clk parents in the enable/disable callback seems fine to me but
>>>>> the way it is implemented doesn't look right.
>>>>>
>>>>> First this "pipe_clksrc" is a mux clk by design, since we can only select the
>>>>> parent. But you are converting it to a gate clk now.
>>>>>
>>>>> Instead of that, my proposal would be to make this clk a composite one i.e,.
>>>>> gate clk + mux clk. So even though the gate clk here would be a hack, we are
>>>>> not changing the definition of mux clk.
>>>>
>>>> This is what I had before, in revisions 1-3. Which proved to work, but is
>>>> problematic a bit.
>>>>
>>>> In the very end, it is not easily possible to make a difference between a
>>>> clock reparented to the bi_tcxo and a disabled clock. E.g. if some user
>>>> reparents the clock to the tcxo, then the driver will consider the clock
>>>> disabled, but the clock framework will think that the clock is still
>>>> enabled.
>>>
>>> I don't understand this. How can you make this clock disabled? It just has 4
>>> parents, right?
>>
>> It has 4 parents. It uses just two of them (pipe and tcxo).
>>
>> And like the clk_rcg2_safe clock we'd like to say that these clocks are
>> disabled by reparenting ("parking") them to the tcxo source. Yes, this makes
>> a lot of code simpler. The clock framework will switch the clock to the
>> "safe" state instead of disabling it during the unused clocks evaporation.
>> The PHY can just disable the gcc_pcie_N_pipe_clock, which will end up in
>> parking this clock to a safe state too, etc.
> 
> If I get the logic behind this "parking" thing right, then it is required
> for producing a stable pipe_clk from GCC when the PHY is about to initialize.
> Also to make sure that there is no glitch observed on pipe_clk while
> initializing the PHY. And once it is powered ON properly, the pipe_clksrc
> should be used as the parent for pipe_clk.
> 
> So with that logic, we cannot say that this clk is disabled.

Yes. It is not technically disabled. But as I said, it serves a good 
abstraction, as a clock is a good as being disabled.

> 
> Please correct me if my understanding is wrong.
> 
> Thanks,
> Mani
> 
>>
>>>
>>>>
>>>> Thus we have to remove "safe" clock (bi_tcxo) from the list of parents. In
>>>> case of pipe clocks (and ufs symbol clocks) this will leave us with just a
>>>> single possible parent. Then having the mux part just doesn't make sense. It
>>>> is just a gated clock. And this simplified a lot of things.
>>>>
>>>>>
>>>>> So you can introduce a new ops like "clk_regmap_mux_gate_ops" and implement the
>>>>> parent switching logic in the enable/disable callbacks. Additional benefit of
>>>>> this ops is, in the future we can also support "gate + mux" clks easily.
>>>>
>>>> If the need arises, we can easily resurrect the regmap_mux_safe patchset,
>>>> fix the race pointed out by Johan, remove extra src-val mapping for safe
>>>> value and use it for such clocks. I can post it separately, if you wish. But
>>>> I'm not sure that it makes sense to use it for single-parent clocks.
>>>>
>>>>>
>>>>> Also, please don't use the "enable_val/disable_val" members. It should be
>>>>> something like "mux_sel_pre/mux_sel_post".
>>>>
>>>> Why? Could you please elaborate?
>>>>
>>>
>>> It aligns with my question above. I don't see how this clk can be
>>> enabled/disabled.
>>
>> I see. Let's settle on the first question then.
>>
>> -- 
>> With best wishes
>> Dmitry


-- 
With best wishes
Dmitry

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

[Johan Hovold]

On Mon, May 02, 2022 at 08:36:11PM +0530, Manivannan Sadhasivam wrote:

> If I get the logic behind this "parking" thing right, then it is required
> for producing a stable pipe_clk from GCC when the PHY is about to initialize.
> Also to make sure that there is no glitch observed on pipe_clk while
> initializing the PHY. And once it is powered ON properly, the pipe_clksrc
> should be used as the parent for pipe_clk.

No, the "parking" is only needed when toggling the corresponding GDSC
which needs a ticking source for some handshake or else it hangs.

The PHY PLL could be muxed in whenever the GDSC power domain is enabled.

That's the thing I don't like about tying the muxing to gating the pipe
clock in the PHY driver. It just happens to work as long as we remember
to gate before disabling the power domain (for a separate device, the
PCIe controller).

But that doesn't solve the case were the boot firmware has left things
in a weird state.

Johan

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

[Manivannan Sadhasivam]

On Mon, May 02, 2022 at 08:36:19PM +0530, Manivannan Sadhasivam wrote:
> On Mon, May 02, 2022 at 02:18:26PM +0300, Dmitry Baryshkov wrote:
> > On 02/05/2022 14:10, Manivannan Sadhasivam wrote:
> > > On Mon, May 02, 2022 at 01:35:34PM +0300, Dmitry Baryshkov wrote:
> > > 
> > > [...]
> > > 
> > > > > > +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;
> > > > > 
> > > > > Selecting the clk parents in the enable/disable callback seems fine to me but
> > > > > the way it is implemented doesn't look right.
> > > > > 
> > > > > First this "pipe_clksrc" is a mux clk by design, since we can only select the
> > > > > parent. But you are converting it to a gate clk now.
> > > > > 
> > > > > Instead of that, my proposal would be to make this clk a composite one i.e,.
> > > > > gate clk + mux clk. So even though the gate clk here would be a hack, we are
> > > > > not changing the definition of mux clk.
> > > > 
> > > > This is what I had before, in revisions 1-3. Which proved to work, but is
> > > > problematic a bit.
> > > > 
> > > > In the very end, it is not easily possible to make a difference between a
> > > > clock reparented to the bi_tcxo and a disabled clock. E.g. if some user
> > > > reparents the clock to the tcxo, then the driver will consider the clock
> > > > disabled, but the clock framework will think that the clock is still
> > > > enabled.
> > > 
> > > I don't understand this. How can you make this clock disabled? It just has 4
> > > parents, right?
> > 
> > It has 4 parents. It uses just two of them (pipe and tcxo).
> > 
> > And like the clk_rcg2_safe clock we'd like to say that these clocks are
> > disabled by reparenting ("parking") them to the tcxo source. Yes, this makes
> > a lot of code simpler. The clock framework will switch the clock to the
> > "safe" state instead of disabling it during the unused clocks evaporation.
> > The PHY can just disable the gcc_pcie_N_pipe_clock, which will end up in
> > parking this clock to a safe state too, etc.
> 
> If I get the logic behind this "parking" thing right, then it is required
> for producing a stable pipe_clk from GCC when the PHY is about to initialize.
> Also to make sure that there is no glitch observed on pipe_clk while
> initializing the PHY. And once it is powered ON properly, the pipe_clksrc
> should be used as the parent for pipe_clk.
> 
> So with that logic, we cannot say that this clk is disabled.
> 
> Please correct me if my understanding is wrong.
> 

After going through the previous iterations and PHY patches, this
implementation makes sense to me now (modelling this pipe_clk_src as gate clk
alone).

But as Johan said, you should change the "pipe_clk" to "pipe_clk_src" or
something similar. As this clk is defined as "pipe_clk_src" in GCC.

Regarding the {enable/disable}_val variables, I'm not fully convinced about the
naming but I don't object it strongly. But irrespective of that, please add a
brief comment in the driver about its purpose and what it does enable/disable
callbacks.

Thanks,
Mani

> Thanks,
> Mani
> 
> > 
> > > 
> > > > 
> > > > Thus we have to remove "safe" clock (bi_tcxo) from the list of parents. In
> > > > case of pipe clocks (and ufs symbol clocks) this will leave us with just a
> > > > single possible parent. Then having the mux part just doesn't make sense. It
> > > > is just a gated clock. And this simplified a lot of things.
> > > > 
> > > > > 
> > > > > So you can introduce a new ops like "clk_regmap_mux_gate_ops" and implement the
> > > > > parent switching logic in the enable/disable callbacks. Additional benefit of
> > > > > this ops is, in the future we can also support "gate + mux" clks easily.
> > > > 
> > > > If the need arises, we can easily resurrect the regmap_mux_safe patchset,
> > > > fix the race pointed out by Johan, remove extra src-val mapping for safe
> > > > value and use it for such clocks. I can post it separately, if you wish. But
> > > > I'm not sure that it makes sense to use it for single-parent clocks.
> > > > 
> > > > > 
> > > > > Also, please don't use the "enable_val/disable_val" members. It should be
> > > > > something like "mux_sel_pre/mux_sel_post".
> > > > 
> > > > Why? Could you please elaborate?
> > > > 
> > > 
> > > It aligns with my question above. I don't see how this clk can be
> > > enabled/disabled.
> > 
> > I see. Let's settle on the first question then.
> > 
> > -- 
> > With best wishes
> > Dmitry

-- 
மணிவண்ணன் சதாசிவம்

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

[Johan Hovold]

On Mon, May 02, 2022 at 02:18:26PM +0300, Dmitry Baryshkov wrote:
> On 02/05/2022 14:10, Manivannan Sadhasivam wrote:

> > I don't understand this. How can you make this clock disabled? It just has 4
> > parents, right?
> 
> It has 4 parents. It uses just two of them (pipe and tcxo).

Really? I did not know that. Which are the other two parents and what
would they be used for?

Johan

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

[Dmitry Baryshkov]

On 06/05/2022 15:40, Johan Hovold wrote:
> On Mon, May 02, 2022 at 02:18:26PM +0300, Dmitry Baryshkov wrote:
>> On 02/05/2022 14:10, Manivannan Sadhasivam wrote:
> 
>>> I don't understand this. How can you make this clock disabled? It just has 4
>>> parents, right?
>>
>> It has 4 parents. It uses just two of them (pipe and tcxo).
> 
> Really? I did not know that. Which are the other two parents and what
> would they be used for?

This is described neither in the downstream tree nor in any sources I 
have at possession.


-- 
With best wishes
Dmitry

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

[Johan Hovold]

On Fri, May 06, 2022 at 04:00:38PM +0300, Dmitry Baryshkov wrote:
> On 06/05/2022 15:40, Johan Hovold wrote:
> > On Mon, May 02, 2022 at 02:18:26PM +0300, Dmitry Baryshkov wrote:
> >> On 02/05/2022 14:10, Manivannan Sadhasivam wrote:
> > 
> >>> I don't understand this. How can you make this clock disabled? It just has 4
> >>> parents, right?
> >>
> >> It has 4 parents. It uses just two of them (pipe and tcxo).
> > 
> > Really? I did not know that. Which are the other two parents and what
> > would they be used for?
> 
> This is described neither in the downstream tree nor in any sources I 
> have at possession.

Yeah, I don't see anything downstream either, but how do you know that
it has four parents then?

Johan

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

[Dmitry Baryshkov]

On 09/05/2022 13:29, Johan Hovold wrote:
> On Fri, May 06, 2022 at 04:00:38PM +0300, Dmitry Baryshkov wrote:
>> On 06/05/2022 15:40, Johan Hovold wrote:
>>> On Mon, May 02, 2022 at 02:18:26PM +0300, Dmitry Baryshkov wrote:
>>>> On 02/05/2022 14:10, Manivannan Sadhasivam wrote:
>>>
>>>>> I don't understand this. How can you make this clock disabled? It just has 4
>>>>> parents, right?
>>>>
>>>> It has 4 parents. It uses just two of them (pipe and tcxo).
>>>
>>> Really? I did not know that. Which are the other two parents and what
>>> would they be used for?
>>
>> This is described neither in the downstream tree nor in any sources I
>> have at possession.
> 
> Yeah, I don't see anything downstream either, but how do you know that
> it has four parents then?

4 possible parents (judging from bitfield).

-- 
With best wishes
Dmitry

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

[Johan Hovold]

On Wed, May 11, 2022 at 05:17:48PM +0300, Dmitry Baryshkov wrote:
> On 09/05/2022 13:29, Johan Hovold wrote:
> > On Fri, May 06, 2022 at 04:00:38PM +0300, Dmitry Baryshkov wrote:
> >> On 06/05/2022 15:40, Johan Hovold wrote:
> >>> On Mon, May 02, 2022 at 02:18:26PM +0300, Dmitry Baryshkov wrote:

> >>>> It has 4 parents. It uses just two of them (pipe and tcxo).
> >>>
> >>> Really? I did not know that. Which are the other two parents and what
> >>> would they be used for?
> >>
> >> This is described neither in the downstream tree nor in any sources I
> >> have at possession.
> > 
> > Yeah, I don't see anything downstream either, but how do you know that
> > it has four parents then?
> 
> 4 possible parents (judging from bitfield).

Ah, ok, so just an assumption based on the configuration field width.

Johan

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

[Manivannan Sadhasivam]

On Mon, May 09, 2022 at 12:29:58PM +0200, Johan Hovold wrote:
> On Fri, May 06, 2022 at 04:00:38PM +0300, Dmitry Baryshkov wrote:
> > On 06/05/2022 15:40, Johan Hovold wrote:
> > > On Mon, May 02, 2022 at 02:18:26PM +0300, Dmitry Baryshkov wrote:
> > >> On 02/05/2022 14:10, Manivannan Sadhasivam wrote:
> > > 
> > >>> I don't understand this. How can you make this clock disabled? It just has 4
> > >>> parents, right?
> > >>
> > >> It has 4 parents. It uses just two of them (pipe and tcxo).
> > > 
> > > Really? I did not know that. Which are the other two parents and what
> > > would they be used for?
> > 
> > This is described neither in the downstream tree nor in any sources I 
> > have at possession.
> 
> Yeah, I don't see anything downstream either, but how do you know that
> it has four parents then?
> 

This information is available in Qcom's internal GCC documentation.

Thanks,
Mani

> Johan

-- 
மணிவண்ணன் சதாசிவம்

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

[Johan Hovold]

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.
> 
> 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
> the new regmap-pipe implementation, which hides multiplexer behind
> simple branch-like clock. 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>

I think this is much better and it addresses most of my concerns with
the previous approach by keeping things simple and using a dedicated
implementation (i.e. separate from regmap-mux).

The purpose of the clock implementation can be documented in the source
and is reflected in the naming. It avoids the issues related to the
caching (locking and deferred muxing) which wasn't really needed in the
first place as these muxes are binary.

By implementing is_enabled() you also allow for inspecting the state
that the boot firmware left the mux in.

The only thing that comes to mind that wouldn't be possible is to
set the mux state using an assigned clock parent in devicetree to make
sure that XO is always selected before toggling the GDSC at probe.

But since that doesn't seem to work anyway when the boot firmware has
set things up (e.g. causes a modem here to reset) that would probably
need to be handled in the GDSC driver anyway (i.e. make sure the source
is XO before enabling the GDSC but only when it was actually disabled).

Taking that one step further would be to implement all this in the GDSC
driver from the start so that the PHY PLL is always muxed in while the
power domain is enabled (and only then)...

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

Since pipe is so overloaded already can we call this "pipe_mux" or
"pipe_src" instead of just "pipe"?

And similarly for

	pipe_mux_is_enabled()
	struct clk_regmap_pipe_mux
	struct clk_regmap_pipe_mux_ops

etc.

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

This is not a hot path and there's rarely a need for unlikely().

> +
> +	return val == pipe->enable_val;
> +}

Johan

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

[Dmitry Baryshkov]

On 06/05/2022 15:31, Johan Hovold wrote:
> 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.
>>
>> 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
>> the new regmap-pipe implementation, which hides multiplexer behind
>> simple branch-like clock. 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>
> 
> I think this is much better and it addresses most of my concerns with
> the previous approach by keeping things simple and using a dedicated
> implementation (i.e. separate from regmap-mux).
> 
> The purpose of the clock implementation can be documented in the source
> and is reflected in the naming. It avoids the issues related to the
> caching (locking and deferred muxing) which wasn't really needed in the
> first place as these muxes are binary.
> 
> By implementing is_enabled() you also allow for inspecting the state
> that the boot firmware left the mux in.
> 
> The only thing that comes to mind that wouldn't be possible is to
> set the mux state using an assigned clock parent in devicetree to make
> sure that XO is always selected before toggling the GDSC at probe.
> 
> But since that doesn't seem to work anyway when the boot firmware has
> set things up (e.g. causes a modem here to reset) that would probably
> need to be handled in the GDSC driver anyway (i.e. make sure the source
> is XO before enabling the GDSC but only when it was actually disabled).
> 
> Taking that one step further would be to implement all this in the GDSC
> driver from the start so that the PHY PLL is always muxed in while the
> power domain is enabled (and only then)...

I think, if we move this to the gdsc driver, we'd loose the part of the 
clock tree.

> 
>> ---
>>   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);
> 
> Since pipe is so overloaded already can we call this "pipe_mux" or
> "pipe_src" instead of just "pipe"?

I'd settle on the pipe_src then.
If you don't mind, I'll wait for your Tested-by and will post the rename 
patchset afterwards.

> 
> And similarly for
> 
> 	pipe_mux_is_enabled()
> 	struct clk_regmap_pipe_mux
> 	struct clk_regmap_pipe_mux_ops
> 
> etc.
> 
>> +	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));
> 
> This is not a hot path and there's rarely a need for unlikely().

Ack.

> 
>> +
>> +	return val == pipe->enable_val;
>> +}
> 
> Johan


-- 
With best wishes
Dmitry

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

[Johan Hovold]

On Fri, May 06, 2022 at 03:40:18PM +0300, Dmitry Baryshkov wrote:
> On 06/05/2022 15:31, Johan Hovold wrote:

> > The only thing that comes to mind that wouldn't be possible is to
> > set the mux state using an assigned clock parent in devicetree to make
> > sure that XO is always selected before toggling the GDSC at probe.
> > 
> > But since that doesn't seem to work anyway when the boot firmware has
> > set things up (e.g. causes a modem here to reset) that would probably
> > need to be handled in the GDSC driver anyway (i.e. make sure the source
> > is XO before enabling the GDSC but only when it was actually disabled).
> > 
> > Taking that one step further would be to implement all this in the GDSC
> > driver from the start so that the PHY PLL is always muxed in while the
> > power domain is enabled (and only then)...
> 
> I think, if we move this to the gdsc driver, we'd loose the part of the 
> clock tree.

Not necessarily, if the GDSC is modeled as a consumer of the mux. 

> If you don't mind, I'll wait for your Tested-by and will post the rename 
> patchset afterwards.

I've tested the series and it works as expected. I'll retest the final
version before giving my Tested-by.

Johan

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

[Johan Hovold]

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

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

[Dmitry Baryshkov]

Use newly defined clk_regmap_pipe_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..4dd48d62c2ff 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-pipe.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_pipe gcc_pcie_0_pipe_clk_src = {
 	.reg = 0x7b060,
 	.shift = 0,
 	.width = 2,
-	.parent_map = gcc_parent_map_4,
+	.enable_val = 0, /* pipe_clk */
+	.disable_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_pipe_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_pipe gcc_pcie_1_pipe_clk_src = {
 	.reg = 0x9d064,
 	.shift = 0,
 	.width = 2,
-	.parent_map = gcc_parent_map_6,
+	.enable_val = 0, /* pipe_clk */
+	.disable_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_pipe_ops,
 		},
 	},
 };
-- 
2.35.1

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

[Dmitry Baryshkov]

Use newly defined clk_regmap_pipe_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..458e7bd4a3cb 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-pipe.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_pipe gcc_pcie_0_pipe_clk_src = {
 	.reg = 0x6b054,
 	.shift = 0,
 	.width = 2,
-	.parent_map = gcc_parent_map_6,
+	.enable_val = 0, /* pipe_clk */
+	.disable_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_pipe_ops,
 		},
 	},
 };
 
-static struct clk_regmap_mux gcc_pcie_1_pipe_clk_src = {
+static struct clk_regmap_pipe gcc_pcie_1_pipe_clk_src = {
 	.reg = 0x8d054,
 	.shift = 0,
 	.width = 2,
-	.parent_map = gcc_parent_map_7,
+	.enable_val = 0, /* pipe_clk */
+	.disable_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_pipe_ops,
 		},
 	},
 };
-- 
2.35.1

[PATCH v4 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(). Drop redundant code letting
the pipe clock driver park the clock to the safe bi_tcxo parent
automatically.

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 v4 5/5] PCI: qcom: Drop manual pipe_clk_src handling

[Johan Hovold]

On Sun, May 01, 2022 at 10:21:49PM +0300, Dmitry Baryshkov wrote:
> Manual reparenting of pipe_clk_src is being replaced with the parking of
> the clock with clk_disable()/clk_enable(). Drop redundant code letting
> the pipe clock driver park the clock to the safe bi_tcxo parent
> automatically.

This isn't just about restoring XO before disabling, you also need to
make sure the PHY PLL is muxed in (and the current implementation never
even bothered to restore XO).

Perhaps rephrase the above.

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

This unconditional reparenting would even cause problems on some
platforms where everything has been set up by the boot firmware.

> -
>  	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 = {

Change itself looks good otherwise.

Johan

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

[Lorenzo Pieralisi]

On Sun, May 01, 2022 at 10:21:44PM +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.
> 
> PCIe part depends on [1].
> 
> 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.
> 
> [1]: https://lore.kernel.org/all/20220401133351.10113-1-johan+linaro@kernel.org/
> 
> Dmitry Baryshkov (5):
>   PCI: qcom: Remove unnecessary pipe_clk handling
>   clk: qcom: regmap: add pipe clk implementation
>   clk: qcom: gcc-sm8450: use new clk_regmap_pipe_ops for PCIe pipe
>     clocks
>   clk: qcom: gcc-sc7280: use new clk_regmap_pipe_ops for PCIe pipe
>     clocks
>   PCI: qcom: Drop manual pipe_clk_src handling
> 
>  drivers/clk/qcom/Makefile              |  1 +
>  drivers/clk/qcom/clk-regmap-pipe.c     | 62 ++++++++++++++++++++
>  drivers/clk/qcom/clk-regmap-pipe.h     | 24 ++++++++
>  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, 128 insertions(+), 140 deletions(-)
>  create mode 100644 drivers/clk/qcom/clk-regmap-pipe.c
>  create mode 100644 drivers/clk/qcom/clk-regmap-pipe.h

Hi guys,

where are we with this series ? I have noticed there is an ongoing
review so if you don't disagree I'd mark it as "Changes Requested" and
wait for a v5.

Thanks,
Lorenzo