linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/5]  PCI: qcom: Rework pipe_clk/pipe_clk_src handling
@ 2022-05-12 17:29 Dmitry Baryshkov
  2022-05-12 17:29 ` [PATCH v5 1/5] PCI: qcom: Remove unnecessary pipe_clk handling Dmitry Baryshkov
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2022-05-12 17:29 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Stanimir Varbanov,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Turquette, Stephen Boyd, Johan Hovold,
	Manivannan Sadhasivam
  Cc: Prasad Malisetty, Vinod Koul, linux-arm-msm, linux-pci, linux-clk

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

[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 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-pipe-src.c | 62 ++++++++++++++++++++
 drivers/clk/qcom/clk-regmap-pipe-src.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-src.c
 create mode 100644 drivers/clk/qcom/clk-regmap-pipe-src.h


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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v5 1/5] PCI: qcom: Remove unnecessary pipe_clk handling
  2022-05-12 17:29 [PATCH v5 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling Dmitry Baryshkov
@ 2022-05-12 17:29 ` Dmitry Baryshkov
  2022-05-13  7:45   ` Johan Hovold
  2022-05-12 17:29 ` [PATCH v5 2/5] clk: qcom: regmap: add PHY clock source implementation Dmitry Baryshkov
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Dmitry Baryshkov @ 2022-05-12 17:29 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Stanimir Varbanov,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Turquette, Stephen Boyd, Johan Hovold,
	Manivannan Sadhasivam
  Cc: Prasad Malisetty, Vinod Koul, linux-arm-msm, linux-pci, linux-clk

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v5 2/5] clk: qcom: regmap: add PHY clock source implementation
  2022-05-12 17:29 [PATCH v5 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling Dmitry Baryshkov
  2022-05-12 17:29 ` [PATCH v5 1/5] PCI: qcom: Remove unnecessary pipe_clk handling Dmitry Baryshkov
@ 2022-05-12 17:29 ` Dmitry Baryshkov
  2022-05-13  8:16   ` Johan Hovold
  2022-05-12 17:29 ` [PATCH v5 3/5] clk: qcom: gcc-sm8450: use new clk_regmap_pipe_src_ops for PCIe pipe clocks Dmitry Baryshkov
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Dmitry Baryshkov @ 2022-05-12 17:29 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Stanimir Varbanov,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Turquette, Stephen Boyd, Johan Hovold,
	Manivannan Sadhasivam
  Cc: Prasad Malisetty, Vinod Koul, linux-arm-msm, linux-pci, linux-clk

On recent Qualcomm platforms the QMP PIPE clocks feed into a set of
muxes which must be parked to the "safe" source (bi_tcxo) when
corresponding GDSC is turned off and on again. Currently this is
handcoded in the PCIe driver by reparenting the gcc_pipe_N_clk_src
clock. However the same code sequence should be applied in the
pcie-qcom endpoint, USB3 and UFS drivers.

Rather than copying this sequence over and over again, follow the
example of clk_rcg2_shared_ops and implement this parking in the
enable() and disable() clock operations. Supplement the regmap-mux with
the new clk_regmap_pipe_src type, which implements such multiplexers
as a simple gate clocks.

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

This way the PHY will disable the pipe clock before turning off the
GDSC, which in turn would lead to disabling corresponding pipe_clk_src
(and thus parked to a safe clock srouce). And vice versa, after enabling
the GDSC the PHY will enable the pipe clock, which would cause
pipe_clk_src to be switched from a safe source to the working one.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/Makefile              |  1 +
 drivers/clk/qcom/clk-regmap-pipe-src.c | 62 ++++++++++++++++++++++++++
 drivers/clk/qcom/clk-regmap-pipe-src.h | 24 ++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-regmap-pipe-src.c
 create mode 100644 drivers/clk/qcom/clk-regmap-pipe-src.h

diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 671cf5821af1..03b945535e49 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -11,6 +11,7 @@ clk-qcom-y += clk-branch.o
 clk-qcom-y += clk-regmap-divider.o
 clk-qcom-y += clk-regmap-mux.o
 clk-qcom-y += clk-regmap-mux-div.o
+clk-qcom-y += clk-regmap-pipe-src.o
 clk-qcom-$(CONFIG_KRAIT_CLOCKS) += clk-krait.o
 clk-qcom-y += clk-hfpll.o
 clk-qcom-y += reset.o
diff --git a/drivers/clk/qcom/clk-regmap-pipe-src.c b/drivers/clk/qcom/clk-regmap-pipe-src.c
new file mode 100644
index 000000000000..02047987ab5f
--- /dev/null
+++ b/drivers/clk/qcom/clk-regmap-pipe-src.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022, Linaro Ltd.
+ */
+
+#include <linux/kernel.h>
+#include <linux/bitops.h>
+#include <linux/regmap.h>
+#include <linux/export.h>
+
+#include "clk-regmap-pipe-src.h"
+
+static inline struct clk_regmap_pipe_src *to_clk_regmap_pipe_src(struct clk_hw *hw)
+{
+	return container_of(to_clk_regmap(hw), struct clk_regmap_pipe_src, clkr);
+}
+
+static int pipe_src_is_enabled(struct clk_hw *hw)
+{
+	struct clk_regmap_pipe_src *pipe = to_clk_regmap_pipe_src(hw);
+	struct clk_regmap *clkr = to_clk_regmap(hw);
+	unsigned int mask = GENMASK(pipe->width + pipe->shift - 1, pipe->shift);
+	unsigned int val;
+
+	regmap_read(clkr->regmap, pipe->reg, &val);
+	val = (val & mask) >> pipe->shift;
+
+	WARN_ON(unlikely(val != pipe->working_val && val != pipe->park_val));
+
+	return val == pipe->working_val;
+}
+
+static int pipe_src_enable(struct clk_hw *hw)
+{
+	struct clk_regmap_pipe_src *pipe = to_clk_regmap_pipe_src(hw);
+	struct clk_regmap *clkr = to_clk_regmap(hw);
+	unsigned int mask = GENMASK(pipe->width + pipe->shift - 1, pipe->shift);
+	unsigned int val;
+
+	val = pipe->working_val << pipe->shift;
+
+	return regmap_update_bits(clkr->regmap, pipe->reg, mask, val);
+}
+
+static void pipe_src_disable(struct clk_hw *hw)
+{
+	struct clk_regmap_pipe_src *pipe = to_clk_regmap_pipe_src(hw);
+	struct clk_regmap *clkr = to_clk_regmap(hw);
+	unsigned int mask = GENMASK(pipe->width + pipe->shift - 1, pipe->shift);
+	unsigned int val;
+
+	val = pipe->park_val << pipe->shift;
+
+	regmap_update_bits(clkr->regmap, pipe->reg, mask, val);
+}
+
+const struct clk_ops clk_regmap_pipe_src_ops = {
+	.enable = pipe_src_enable,
+	.disable = pipe_src_disable,
+	.is_enabled = pipe_src_is_enabled,
+};
+EXPORT_SYMBOL_GPL(clk_regmap_pipe_src_ops);
diff --git a/drivers/clk/qcom/clk-regmap-pipe-src.h b/drivers/clk/qcom/clk-regmap-pipe-src.h
new file mode 100644
index 000000000000..3aa4a9f402cd
--- /dev/null
+++ b/drivers/clk/qcom/clk-regmap-pipe-src.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2022, Linaro Ltd.
+ * Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
+ */
+
+#ifndef __QCOM_CLK_REGMAP_PIPE_SRC_H__
+#define __QCOM_CLK_REGMAP_PIPE_SRC_H__
+
+#include <linux/clk-provider.h>
+#include "clk-regmap.h"
+
+struct clk_regmap_pipe_src {
+	u32			reg;
+	u32			shift;
+	u32			width;
+	u32			working_val;
+	u32			park_val;
+	struct clk_regmap	clkr;
+};
+
+extern const struct clk_ops clk_regmap_pipe_src_ops;
+
+#endif
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v5 3/5] clk: qcom: gcc-sm8450: use new clk_regmap_pipe_src_ops for PCIe pipe clocks
  2022-05-12 17:29 [PATCH v5 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling Dmitry Baryshkov
  2022-05-12 17:29 ` [PATCH v5 1/5] PCI: qcom: Remove unnecessary pipe_clk handling Dmitry Baryshkov
  2022-05-12 17:29 ` [PATCH v5 2/5] clk: qcom: regmap: add PHY clock source implementation Dmitry Baryshkov
@ 2022-05-12 17:29 ` Dmitry Baryshkov
  2022-05-12 17:29 ` [PATCH v5 4/5] clk: qcom: gcc-sc7280: " Dmitry Baryshkov
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2022-05-12 17:29 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Stanimir Varbanov,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Turquette, Stephen Boyd, Johan Hovold,
	Manivannan Sadhasivam
  Cc: Prasad Malisetty, Vinod Koul, linux-arm-msm, linux-pci, linux-clk

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..bbe4cc1957da 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-src.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_src gcc_pcie_0_pipe_clk_src = {
 	.reg = 0x7b060,
 	.shift = 0,
 	.width = 2,
-	.parent_map = gcc_parent_map_4,
+	.working_val = 0, /* pipe_clk */
+	.park_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_src_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_src gcc_pcie_1_pipe_clk_src = {
 	.reg = 0x9d064,
 	.shift = 0,
 	.width = 2,
-	.parent_map = gcc_parent_map_6,
+	.working_val = 0, /* pipe_clk */
+	.park_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_src_ops,
 		},
 	},
 };
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v5 4/5] clk: qcom: gcc-sc7280: use new clk_regmap_pipe_src_ops for PCIe pipe clocks
  2022-05-12 17:29 [PATCH v5 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling Dmitry Baryshkov
                   ` (2 preceding siblings ...)
  2022-05-12 17:29 ` [PATCH v5 3/5] clk: qcom: gcc-sm8450: use new clk_regmap_pipe_src_ops for PCIe pipe clocks Dmitry Baryshkov
@ 2022-05-12 17:29 ` Dmitry Baryshkov
  2022-05-12 17:29 ` [PATCH v5 5/5] PCI: qcom: Drop manual pipe_clk_src handling Dmitry Baryshkov
  2022-05-13  7:41 ` [PATCH v5 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling Johan Hovold
  5 siblings, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2022-05-12 17:29 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Stanimir Varbanov,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Turquette, Stephen Boyd, Johan Hovold,
	Manivannan Sadhasivam
  Cc: Prasad Malisetty, Vinod Koul, linux-arm-msm, linux-pci, linux-clk

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..dbe12580ca70 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-src.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_src gcc_pcie_0_pipe_clk_src = {
 	.reg = 0x6b054,
 	.shift = 0,
 	.width = 2,
-	.parent_map = gcc_parent_map_6,
+	.working_val = 0, /* pipe_clk */
+	.park_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_src_ops,
 		},
 	},
 };
 
-static struct clk_regmap_mux gcc_pcie_1_pipe_clk_src = {
+static struct clk_regmap_pipe_src gcc_pcie_1_pipe_clk_src = {
 	.reg = 0x8d054,
 	.shift = 0,
 	.width = 2,
-	.parent_map = gcc_parent_map_7,
+	.working_val = 0, /* pipe_clk */
+	.park_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_src_ops,
 		},
 	},
 };
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v5 5/5] PCI: qcom: Drop manual pipe_clk_src handling
  2022-05-12 17:29 [PATCH v5 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling Dmitry Baryshkov
                   ` (3 preceding siblings ...)
  2022-05-12 17:29 ` [PATCH v5 4/5] clk: qcom: gcc-sc7280: " Dmitry Baryshkov
@ 2022-05-12 17:29 ` Dmitry Baryshkov
  2022-05-13  8:19   ` Johan Hovold
  2022-05-13  7:41 ` [PATCH v5 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling Johan Hovold
  5 siblings, 1 reply; 13+ messages in thread
From: Dmitry Baryshkov @ 2022-05-12 17:29 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Stanimir Varbanov,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Turquette, Stephen Boyd, Johan Hovold,
	Manivannan Sadhasivam
  Cc: Prasad Malisetty, Vinod Koul, linux-arm-msm, linux-pci,
	linux-clk, Prasad Malisetty

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 0/5]  PCI: qcom: Rework pipe_clk/pipe_clk_src handling
  2022-05-12 17:29 [PATCH v5 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling Dmitry Baryshkov
                   ` (4 preceding siblings ...)
  2022-05-12 17:29 ` [PATCH v5 5/5] PCI: qcom: Drop manual pipe_clk_src handling Dmitry Baryshkov
@ 2022-05-13  7:41 ` Johan Hovold
  2022-05-13  9:31   ` Dmitry Baryshkov
  5 siblings, 1 reply; 13+ messages in thread
From: Johan Hovold @ 2022-05-13  7:41 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andy Gross, Bjorn Andersson, Stanimir Varbanov,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Turquette, Stephen Boyd, Johan Hovold,
	Manivannan Sadhasivam, Prasad Malisetty, Vinod Koul,
	linux-arm-msm, linux-pci, linux-clk

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

This one was merged a month ago.

> 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.
> 
> [1]: https://lore.kernel.org/all/20220401133351.10113-1-johan+linaro@kernel.org/

Johan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/5] PCI: qcom: Remove unnecessary pipe_clk handling
  2022-05-12 17:29 ` [PATCH v5 1/5] PCI: qcom: Remove unnecessary pipe_clk handling Dmitry Baryshkov
@ 2022-05-13  7:45   ` Johan Hovold
  0 siblings, 0 replies; 13+ messages in thread
From: Johan Hovold @ 2022-05-13  7:45 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andy Gross, Bjorn Andersson, Stanimir Varbanov,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Turquette, Stephen Boyd, Johan Hovold,
	Manivannan Sadhasivam, Prasad Malisetty, Vinod Koul,
	linux-arm-msm, linux-pci, linux-clk

On Thu, May 12, 2022 at 08:29:05PM +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>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I assume you checked that there are no further PHY drivers used with
pcie-qcom by any in-kernel dts?

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

Johan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 2/5] clk: qcom: regmap: add PHY clock source implementation
  2022-05-12 17:29 ` [PATCH v5 2/5] clk: qcom: regmap: add PHY clock source implementation Dmitry Baryshkov
@ 2022-05-13  8:16   ` Johan Hovold
  2022-05-13  9:50     ` Dmitry Baryshkov
  0 siblings, 1 reply; 13+ messages in thread
From: Johan Hovold @ 2022-05-13  8:16 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andy Gross, Bjorn Andersson, Stanimir Varbanov,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Turquette, Stephen Boyd, Johan Hovold,
	Manivannan Sadhasivam, Prasad Malisetty, Vinod Koul,
	linux-arm-msm, linux-pci, linux-clk

On Thu, May 12, 2022 at 08:29:06PM +0300, Dmitry Baryshkov wrote:
> On recent Qualcomm platforms the QMP PIPE clocks feed into a set of
> muxes which must be parked to the "safe" source (bi_tcxo) when
> corresponding GDSC is turned off and on again. Currently this is
> handcoded in the PCIe driver by reparenting the gcc_pipe_N_clk_src
> clock. However the same code sequence should be applied in the
> pcie-qcom endpoint, USB3 and UFS drivers.

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

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

> Rather than copying this sequence over and over again, follow the
> example of clk_rcg2_shared_ops and implement this parking in the
> enable() and disable() clock operations. Supplement the regmap-mux with
> the new clk_regmap_pipe_src type, which implements such multiplexers
> as a simple gate clocks.
> 
> This is possible since each of these multiplexers has just two clock
> sources: working (pipe) and safe/park (bi_tcxo) clock sources. If the
> clock is running off the external pipe source, report it as enabled and
> report it as disabled otherwise.
> 
> This way the PHY will disable the pipe clock before turning off the
> GDSC, which in turn would lead to disabling corresponding pipe_clk_src
> (and thus parked to a safe clock srouce). And vice versa, after enabling

typo: source

> the GDSC the PHY will enable the pipe clock, which would cause
> pipe_clk_src to be switched from a safe source to the working one.

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

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/Makefile              |  1 +
>  drivers/clk/qcom/clk-regmap-pipe-src.c | 62 ++++++++++++++++++++++++++
>  drivers/clk/qcom/clk-regmap-pipe-src.h | 24 ++++++++++
>  3 files changed, 87 insertions(+)
>  create mode 100644 drivers/clk/qcom/clk-regmap-pipe-src.c
>  create mode 100644 drivers/clk/qcom/clk-regmap-pipe-src.h
> 
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index 671cf5821af1..03b945535e49 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -11,6 +11,7 @@ clk-qcom-y += clk-branch.o
>  clk-qcom-y += clk-regmap-divider.o
>  clk-qcom-y += clk-regmap-mux.o
>  clk-qcom-y += clk-regmap-mux-div.o
> +clk-qcom-y += clk-regmap-pipe-src.o
>  clk-qcom-$(CONFIG_KRAIT_CLOCKS) += clk-krait.o
>  clk-qcom-y += clk-hfpll.o
>  clk-qcom-y += reset.o
> diff --git a/drivers/clk/qcom/clk-regmap-pipe-src.c b/drivers/clk/qcom/clk-regmap-pipe-src.c
> new file mode 100644
> index 000000000000..02047987ab5f
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-regmap-pipe-src.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022, Linaro Ltd.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/bitops.h>
> +#include <linux/regmap.h>
> +#include <linux/export.h>
> +
> +#include "clk-regmap-pipe-src.h"
> +
> +static inline struct clk_regmap_pipe_src *to_clk_regmap_pipe_src(struct clk_hw *hw)
> +{
> +	return container_of(to_clk_regmap(hw), struct clk_regmap_pipe_src, clkr);
> +}
> +
> +static int pipe_src_is_enabled(struct clk_hw *hw)
> +{
> +	struct clk_regmap_pipe_src *pipe = to_clk_regmap_pipe_src(hw);

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

> +	struct clk_regmap *clkr = to_clk_regmap(hw);
> +	unsigned int mask = GENMASK(pipe->width + pipe->shift - 1, pipe->shift);
> +	unsigned int val;
> +
> +	regmap_read(clkr->regmap, pipe->reg, &val);
> +	val = (val & mask) >> pipe->shift;
> +
> +	WARN_ON(unlikely(val != pipe->working_val && val != pipe->park_val));

Again, please drop unlikely() here.

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

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

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

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

Johan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 5/5] PCI: qcom: Drop manual pipe_clk_src handling
  2022-05-12 17:29 ` [PATCH v5 5/5] PCI: qcom: Drop manual pipe_clk_src handling Dmitry Baryshkov
@ 2022-05-13  8:19   ` Johan Hovold
  0 siblings, 0 replies; 13+ messages in thread
From: Johan Hovold @ 2022-05-13  8:19 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andy Gross, Bjorn Andersson, Stanimir Varbanov,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Turquette, Stephen Boyd, Johan Hovold,
	Manivannan Sadhasivam, Prasad Malisetty, Vinod Koul,
	linux-arm-msm, linux-pci, linux-clk, Prasad Malisetty

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

You ignored my comments on this one too.

Johan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling
  2022-05-13  7:41 ` [PATCH v5 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling Johan Hovold
@ 2022-05-13  9:31   ` Dmitry Baryshkov
  2022-05-13  9:43     ` Johan Hovold
  0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Baryshkov @ 2022-05-13  9:31 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Andy Gross, Bjorn Andersson, Stanimir Varbanov,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Turquette, Stephen Boyd, Johan Hovold,
	Manivannan Sadhasivam, Prasad Malisetty, Vinod Koul,
	linux-arm-msm, linux-pci, linux-clk

On Fri, 13 May 2022 at 10:41, Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, May 12, 2022 at 08:29:04PM +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].
>
> This one was merged a month ago.

It is not in Linus's tree (only in Lorenzo's one). So anybody wishing
to test the series would still have to pick it up manually.

>
> > 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.
> >
> > [1]: https://lore.kernel.org/all/20220401133351.10113-1-johan+linaro@kernel.org/
>
> Johan



-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling
  2022-05-13  9:31   ` Dmitry Baryshkov
@ 2022-05-13  9:43     ` Johan Hovold
  0 siblings, 0 replies; 13+ messages in thread
From: Johan Hovold @ 2022-05-13  9:43 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andy Gross, Bjorn Andersson, Stanimir Varbanov,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Turquette, Stephen Boyd, Johan Hovold,
	Manivannan Sadhasivam, Prasad Malisetty, Vinod Koul,
	linux-arm-msm, linux-pci, linux-clk

On Fri, May 13, 2022 at 12:31:27PM +0300, Dmitry Baryshkov wrote:
> On Fri, 13 May 2022 at 10:41, Johan Hovold <johan@kernel.org> wrote:
> > On Thu, May 12, 2022 at 08:29:04PM +0300, Dmitry Baryshkov wrote:

> > > PCIe part depends on [1].
> >
> > This one was merged a month ago.
> 
> It is not in Linus's tree (only in Lorenzo's one). So anybody wishing
> to test the series would still have to pick it up manually.

Fair enough, but you generally don't need to describe dependencies that
have already been picked up by the maintainer.

Johan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 2/5] clk: qcom: regmap: add PHY clock source implementation
  2022-05-13  8:16   ` Johan Hovold
@ 2022-05-13  9:50     ` Dmitry Baryshkov
  0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2022-05-13  9:50 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Andy Gross, Bjorn Andersson, Stanimir Varbanov,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Turquette, Stephen Boyd, Johan Hovold,
	Manivannan Sadhasivam, Prasad Malisetty, Vinod Koul,
	linux-arm-msm, linux-pci, linux-clk

On Fri, 13 May 2022 at 11:16, Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, May 12, 2022 at 08:29:06PM +0300, Dmitry Baryshkov wrote:
> > On recent Qualcomm platforms the QMP PIPE clocks feed into a set of
> > muxes which must be parked to the "safe" source (bi_tcxo) when
> > corresponding GDSC is turned off and on again. Currently this is
> > handcoded in the PCIe driver by reparenting the gcc_pipe_N_clk_src
> > clock. However the same code sequence should be applied in the
> > pcie-qcom endpoint, USB3 and UFS drivers.
>
> You seem to have ignored my comment regarding UFS and naming except for
> the updated Subject.
>
> For UFS the corresponding clocks would be named symbol clocks, which
> seems to suggest that a more generic name is warranted. I mentioned
> phy_mux as a possible alternative name for for pipe_mux (or pipe_src).

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

>
> > Rather than copying this sequence over and over again, follow the
> > example of clk_rcg2_shared_ops and implement this parking in the
> > enable() and disable() clock operations. Supplement the regmap-mux with
> > the new clk_regmap_pipe_src type, which implements such multiplexers
> > as a simple gate clocks.
> >
> > This is possible since each of these multiplexers has just two clock
> > sources: working (pipe) and safe/park (bi_tcxo) clock sources. If the
> > clock is running off the external pipe source, report it as enabled and
> > report it as disabled otherwise.
> >
> > This way the PHY will disable the pipe clock before turning off the
> > GDSC, which in turn would lead to disabling corresponding pipe_clk_src
> > (and thus parked to a safe clock srouce). And vice versa, after enabling
>
> typo: source
>
> > the GDSC the PHY will enable the pipe clock, which would cause
> > pipe_clk_src to be switched from a safe source to the working one.
>
> Explaining how this fits together with the PHY power sequencing is good
> but it needs to be reflected in the implementation too. Preferably using
> good naming, but possibly also with a comment.

Ack

>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/clk/qcom/Makefile              |  1 +
> >  drivers/clk/qcom/clk-regmap-pipe-src.c | 62 ++++++++++++++++++++++++++
> >  drivers/clk/qcom/clk-regmap-pipe-src.h | 24 ++++++++++
> >  3 files changed, 87 insertions(+)
> >  create mode 100644 drivers/clk/qcom/clk-regmap-pipe-src.c
> >  create mode 100644 drivers/clk/qcom/clk-regmap-pipe-src.h
> >
> > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> > index 671cf5821af1..03b945535e49 100644
> > --- a/drivers/clk/qcom/Makefile
> > +++ b/drivers/clk/qcom/Makefile
> > @@ -11,6 +11,7 @@ clk-qcom-y += clk-branch.o
> >  clk-qcom-y += clk-regmap-divider.o
> >  clk-qcom-y += clk-regmap-mux.o
> >  clk-qcom-y += clk-regmap-mux-div.o
> > +clk-qcom-y += clk-regmap-pipe-src.o
> >  clk-qcom-$(CONFIG_KRAIT_CLOCKS) += clk-krait.o
> >  clk-qcom-y += clk-hfpll.o
> >  clk-qcom-y += reset.o
> > diff --git a/drivers/clk/qcom/clk-regmap-pipe-src.c b/drivers/clk/qcom/clk-regmap-pipe-src.c
> > new file mode 100644
> > index 000000000000..02047987ab5f
> > --- /dev/null
> > +++ b/drivers/clk/qcom/clk-regmap-pipe-src.c
> > @@ -0,0 +1,62 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022, Linaro Ltd.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/bitops.h>
> > +#include <linux/regmap.h>
> > +#include <linux/export.h>
> > +
> > +#include "clk-regmap-pipe-src.h"
> > +
> > +static inline struct clk_regmap_pipe_src *to_clk_regmap_pipe_src(struct clk_hw *hw)
> > +{
> > +     return container_of(to_clk_regmap(hw), struct clk_regmap_pipe_src, clkr);
> > +}
> > +
> > +static int pipe_src_is_enabled(struct clk_hw *hw)
> > +{
> > +     struct clk_regmap_pipe_src *pipe = to_clk_regmap_pipe_src(hw);
>
> Again, "pipe" is so overloaded and using "mux" (or possibly "src")
> throughout would make the code easier to understand.

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

>
> > +     struct clk_regmap *clkr = to_clk_regmap(hw);
> > +     unsigned int mask = GENMASK(pipe->width + pipe->shift - 1, pipe->shift);
> > +     unsigned int val;
> > +
> > +     regmap_read(clkr->regmap, pipe->reg, &val);
> > +     val = (val & mask) >> pipe->shift;
> > +
> > +     WARN_ON(unlikely(val != pipe->working_val && val != pipe->park_val));
>
> Again, please drop unlikely() here.

Ack

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

Thanks. Yes, this looks more descriptive.

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



-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-05-13  9:50 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-12 17:29 [PATCH v5 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling Dmitry Baryshkov
2022-05-12 17:29 ` [PATCH v5 1/5] PCI: qcom: Remove unnecessary pipe_clk handling Dmitry Baryshkov
2022-05-13  7:45   ` Johan Hovold
2022-05-12 17:29 ` [PATCH v5 2/5] clk: qcom: regmap: add PHY clock source implementation Dmitry Baryshkov
2022-05-13  8:16   ` Johan Hovold
2022-05-13  9:50     ` Dmitry Baryshkov
2022-05-12 17:29 ` [PATCH v5 3/5] clk: qcom: gcc-sm8450: use new clk_regmap_pipe_src_ops for PCIe pipe clocks Dmitry Baryshkov
2022-05-12 17:29 ` [PATCH v5 4/5] clk: qcom: gcc-sc7280: " Dmitry Baryshkov
2022-05-12 17:29 ` [PATCH v5 5/5] PCI: qcom: Drop manual pipe_clk_src handling Dmitry Baryshkov
2022-05-13  8:19   ` Johan Hovold
2022-05-13  7:41 ` [PATCH v5 0/5] PCI: qcom: Rework pipe_clk/pipe_clk_src handling Johan Hovold
2022-05-13  9:31   ` Dmitry Baryshkov
2022-05-13  9:43     ` Johan Hovold

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).