[PATCH v2 0/5] PCI: qcom: rework pipe_clk/pipe_clk_src handling

[PATCH v2 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 v1:
 - Rebased on top of [1].
 - Removed erroneous Fixes tag from the patch 4.

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

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

 drivers/clk/qcom/clk-regmap-mux.c      | 78 +++++++++++++++++++++++++
 drivers/clk/qcom/clk-regmap-mux.h      |  3 +
 drivers/clk/qcom/gcc-sc7280.c          |  6 +-
 drivers/clk/qcom/gcc-sm8450.c          |  6 +-
 drivers/pci/controller/dwc/pcie-qcom.c | 81 +-------------------------
 5 files changed, 92 insertions(+), 82 deletions(-)

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

[PATCH v2 1/5] clk: qcom: regmap-mux: 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. As we are changing the parent
behind the back of the clock framework, also implement custom
set_parent() and get_parent() operations behaving accroding to the clock
framework expectations (cache the new parent if the clock is in disabled
state, return cached parent).

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-regmap-mux.c | 78 +++++++++++++++++++++++++++++++
 drivers/clk/qcom/clk-regmap-mux.h |  3 ++
 2 files changed, 81 insertions(+)

diff --git a/drivers/clk/qcom/clk-regmap-mux.c b/drivers/clk/qcom/clk-regmap-mux.c
index 45d9cca28064..c39ee783ee83 100644
--- a/drivers/clk/qcom/clk-regmap-mux.c
+++ b/drivers/clk/qcom/clk-regmap-mux.c
@@ -49,9 +49,87 @@ static int mux_set_parent(struct clk_hw *hw, u8 index)
 	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
 }
 
+static u8 mux_safe_get_parent(struct clk_hw *hw)
+{
+	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
+	unsigned int val;
+
+	if (clk_hw_is_enabled(hw))
+		return mux_get_parent(hw);
+
+	val = mux->stored_parent_cfg;
+
+	if (mux->parent_map)
+		return qcom_find_cfg_index(hw, mux->parent_map, val);
+
+	return val;
+}
+
+static int mux_safe_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
+
+	if (clk_hw_is_enabled(hw))
+		return mux_set_parent(hw, index);
+
+	if (mux->parent_map)
+		index = mux->parent_map[index].cfg;
+
+	mux->stored_parent_cfg = index;
+
+	return 0;
+}
+
+static void mux_safe_disable(struct clk_hw *hw)
+{
+	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
+	struct clk_regmap *clkr = to_clk_regmap(hw);
+	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
+	unsigned int val;
+
+	regmap_read(clkr->regmap, mux->reg, &val);
+
+	mux->stored_parent_cfg = (val & mask) >> mux->shift;
+
+	val = mux->safe_src_parent;
+	if (mux->parent_map) {
+		int index = qcom_find_src_index(hw, mux->parent_map, val);
+
+		if (WARN_ON(index < 0))
+			return;
+
+		val = mux->parent_map[index].cfg;
+	}
+	val <<= mux->shift;
+
+	regmap_update_bits(clkr->regmap, mux->reg, mask, val);
+}
+
+static int mux_safe_enable(struct clk_hw *hw)
+{
+	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
+	struct clk_regmap *clkr = to_clk_regmap(hw);
+	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
+	unsigned int val;
+
+	val = mux->stored_parent_cfg;
+	val <<= mux->shift;
+
+	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
+}
+
 const struct clk_ops clk_regmap_mux_closest_ops = {
 	.get_parent = mux_get_parent,
 	.set_parent = mux_set_parent,
 	.determine_rate = __clk_mux_determine_rate_closest,
 };
 EXPORT_SYMBOL_GPL(clk_regmap_mux_closest_ops);
+
+const struct clk_ops clk_regmap_mux_safe_ops = {
+	.enable = mux_safe_enable,
+	.disable = mux_safe_disable,
+	.get_parent = mux_safe_get_parent,
+	.set_parent = mux_safe_set_parent,
+	.determine_rate = __clk_mux_determine_rate_closest,
+};
+EXPORT_SYMBOL_GPL(clk_regmap_mux_safe_ops);
diff --git a/drivers/clk/qcom/clk-regmap-mux.h b/drivers/clk/qcom/clk-regmap-mux.h
index db6f4cdd9586..f86c674ce139 100644
--- a/drivers/clk/qcom/clk-regmap-mux.h
+++ b/drivers/clk/qcom/clk-regmap-mux.h
@@ -14,10 +14,13 @@ struct clk_regmap_mux {
 	u32			reg;
 	u32			shift;
 	u32			width;
+	u8			safe_src_parent;
+	u8			stored_parent_cfg;
 	const struct parent_map	*parent_map;
 	struct clk_regmap	clkr;
 };
 
 extern const struct clk_ops clk_regmap_mux_closest_ops;
+extern const struct clk_ops clk_regmap_mux_safe_ops;
 
 #endif
-- 
2.35.1

[PATCH v2 2/5] clk: qcom: gcc-sm8450: use new clk_regmap_mux_safe_ops for PCIe pipe clocks

[Dmitry Baryshkov]

Use newly defined clk_regmap_mux_safe_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.

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/gcc-sm8450.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sm8450.c b/drivers/clk/qcom/gcc-sm8450.c
index 593a195467ff..fb6decd3df49 100644
--- a/drivers/clk/qcom/gcc-sm8450.c
+++ b/drivers/clk/qcom/gcc-sm8450.c
@@ -243,13 +243,14 @@ static struct clk_regmap_mux gcc_pcie_0_pipe_clk_src = {
 	.reg = 0x7b060,
 	.shift = 0,
 	.width = 2,
+	.safe_src_parent = P_BI_TCXO,
 	.parent_map = gcc_parent_map_4,
 	.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,
+			.ops = &clk_regmap_mux_safe_ops,
 		},
 	},
 };
@@ -273,13 +274,14 @@ static struct clk_regmap_mux gcc_pcie_1_pipe_clk_src = {
 	.reg = 0x9d064,
 	.shift = 0,
 	.width = 2,
+	.safe_src_parent = P_BI_TCXO,
 	.parent_map = gcc_parent_map_6,
 	.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,
+			.ops = &clk_regmap_mux_safe_ops,
 		},
 	},
 };
-- 
2.35.1

[PATCH v2 3/5] clk: qcom: gcc-sc7280: use new clk_regmap_mux_safe_ops for PCIe pipe clocks

[Dmitry Baryshkov]

Use newly defined clk_regmap_mux_safe_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.

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/gcc-sc7280.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
index 423627d49719..dafbbc8f3bf4 100644
--- a/drivers/clk/qcom/gcc-sc7280.c
+++ b/drivers/clk/qcom/gcc-sc7280.c
@@ -373,13 +373,14 @@ static struct clk_regmap_mux gcc_pcie_0_pipe_clk_src = {
 	.reg = 0x6b054,
 	.shift = 0,
 	.width = 2,
+	.safe_src_parent = P_BI_TCXO,
 	.parent_map = gcc_parent_map_6,
 	.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,
+			.ops = &clk_regmap_mux_safe_ops,
 		},
 	},
 };
@@ -388,13 +389,14 @@ static struct clk_regmap_mux gcc_pcie_1_pipe_clk_src = {
 	.reg = 0x8d054,
 	.shift = 0,
 	.width = 2,
+	.safe_src_parent = P_BI_TCXO,
 	.parent_map = gcc_parent_map_7,
 	.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,
+			.ops = &clk_regmap_mux_safe_ops,
 		},
 	},
 };
-- 
2.35.1

[PATCH v2 4/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

[PATCH v2 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 v2 1/5] clk: qcom: regmap-mux: add pipe clk implementation

[Johan Hovold]

On Tue, Apr 12, 2022 at 10:38:35PM +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'm starting to think this really belongs in the PHY driver which is the
provider of the pipe clock. Moving it there would also allow the code to
be shared between PCIe, USB, and UFS.

The PHY driver enables the pipe clock by starting the PHY and before
doing so there's no point in updating the mux. Similarly, the PHY driver
can restore the "safe" source after disabling the pipe clock.

That way there's no magic happening behind scenes, the clock framework
always reports the actual state of the tree, and the reason for all of
this can be documented in the QMP PHY driver once and for all.

The only change to the bindings compared to what this series proposes is
that the PHY driver also needs a reference to bi_tcxo.

Also note that updating the mux separately from starting the PHY as this
series allows for, doesn't really make the pipe clock any safer to use.

Either way, there are also some problems with this safe-mux
implementation that I point out below.

> 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. As we are changing the parent
> behind the back of the clock framework, also implement custom
> set_parent() and get_parent() operations behaving accroding to the clock
> framework expectations (cache the new parent if the clock is in disabled
> state, return cached parent).
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/clk-regmap-mux.c | 78 +++++++++++++++++++++++++++++++
>  drivers/clk/qcom/clk-regmap-mux.h |  3 ++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-regmap-mux.c b/drivers/clk/qcom/clk-regmap-mux.c
> index 45d9cca28064..c39ee783ee83 100644
> --- a/drivers/clk/qcom/clk-regmap-mux.c
> +++ b/drivers/clk/qcom/clk-regmap-mux.c
> @@ -49,9 +49,87 @@ static int mux_set_parent(struct clk_hw *hw, u8 index)
>  	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
>  }
>  
> +static u8 mux_safe_get_parent(struct clk_hw *hw)
> +{
> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> +	unsigned int val;
> +
> +	if (clk_hw_is_enabled(hw))
> +		return mux_get_parent(hw);
> +
> +	val = mux->stored_parent_cfg;
> +
> +	if (mux->parent_map)
> +		return qcom_find_cfg_index(hw, mux->parent_map, val);
> +
> +	return val;
> +}
> +
> +static int mux_safe_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> +
> +	if (clk_hw_is_enabled(hw))
> +		return mux_set_parent(hw, index);
> +
> +	if (mux->parent_map)
> +		index = mux->parent_map[index].cfg;
> +
> +	mux->stored_parent_cfg = index;
> +
> +	return 0;
> +}
> +
> +static void mux_safe_disable(struct clk_hw *hw)
> +{
> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> +	struct clk_regmap *clkr = to_clk_regmap(hw);
> +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
> +	unsigned int val;
> +
> +	regmap_read(clkr->regmap, mux->reg, &val);
> +
> +	mux->stored_parent_cfg = (val & mask) >> mux->shift;
> +
> +	val = mux->safe_src_parent;
> +	if (mux->parent_map) {
> +		int index = qcom_find_src_index(hw, mux->parent_map, val);
> +
> +		if (WARN_ON(index < 0))
> +			return;
> +
> +		val = mux->parent_map[index].cfg;
> +	}
> +	val <<= mux->shift;
> +
> +	regmap_update_bits(clkr->regmap, mux->reg, mask, val);
> +}
> +
> +static int mux_safe_enable(struct clk_hw *hw)
> +{
> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> +	struct clk_regmap *clkr = to_clk_regmap(hw);
> +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
> +	unsigned int val;
> +
> +	val = mux->stored_parent_cfg;
> +	val <<= mux->shift;
> +
> +	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
> +}

The caching of the parent is broken since set_parent() is typically not
called before enabling the clock.

This means that the above code will set the mux to its zero-initialised
value, which currently only works by chance as the pipe clock config
value happens to be zero.

For this to work generally, you'd also need to define also the
(default/initial) non-safe parent for each mux. Handling handover from
the bootloader might also be tricky.

Furthermore, the current implementation appears to ignore locking and
doesn't handle the case where set_parent() races with enable(). The
former is protected by the prepare mutex and the latter by the enable
spinlock and a driver that needs to serialise the two needs to handle
that itself.

> +
>  const struct clk_ops clk_regmap_mux_closest_ops = {
>  	.get_parent = mux_get_parent,
>  	.set_parent = mux_set_parent,
>  	.determine_rate = __clk_mux_determine_rate_closest,
>  };
>  EXPORT_SYMBOL_GPL(clk_regmap_mux_closest_ops);
> +
> +const struct clk_ops clk_regmap_mux_safe_ops = {
> +	.enable = mux_safe_enable,
> +	.disable = mux_safe_disable,
> +	.get_parent = mux_safe_get_parent,
> +	.set_parent = mux_safe_set_parent,
> +	.determine_rate = __clk_mux_determine_rate_closest,
> +};
> +EXPORT_SYMBOL_GPL(clk_regmap_mux_safe_ops);
> diff --git a/drivers/clk/qcom/clk-regmap-mux.h b/drivers/clk/qcom/clk-regmap-mux.h
> index db6f4cdd9586..f86c674ce139 100644
> --- a/drivers/clk/qcom/clk-regmap-mux.h
> +++ b/drivers/clk/qcom/clk-regmap-mux.h
> @@ -14,10 +14,13 @@ struct clk_regmap_mux {
>  	u32			reg;
>  	u32			shift;
>  	u32			width;
> +	u8			safe_src_parent;
> +	u8			stored_parent_cfg;
>  	const struct parent_map	*parent_map;
>  	struct clk_regmap	clkr;
>  };
>  
>  extern const struct clk_ops clk_regmap_mux_closest_ops;
> +extern const struct clk_ops clk_regmap_mux_safe_ops;
>  
>  #endif

Johan

Re: [PATCH v2 0/5] PCI: qcom: rework pipe_clk/pipe_clk_src handling

[Johan Hovold]

[ Dropping tdas@codeaurora.org, which bounces. ]

On Tue, Apr 12, 2022 at 10:38:34PM +0300, Dmitry Baryshkov wrote:
> PCIe pipe clk (and some other clocks) must be parked to the "safe"
> source (bi_tcxo) when corresponding GDSC is turned off and on again.
> Currently this is handcoded in the PCIe driver by reparenting the
> gcc_pipe_N_clk_src clock.
> 
> Instead of doing it manually, follow the approach used by
> clk_rcg2_shared_ops and implement this parking in the enable() and
> disable() clock operations for respective pipe clocks.
> 
> PCIe part depends on [1].

Looks like you forgot to add the link to the prerequisite patch:

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

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

Johan

Re: [PATCH v2 0/5] PCI: qcom: rework pipe_clk/pipe_clk_src handling

[patchwork-bot+linux-arm-msm]

Hello:

This series was applied to qcom/linux.git (for-next)
by Bjorn Andersson <bjorn.andersson@linaro.org>:

On Tue, 12 Apr 2022 22:38:34 +0300 you 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.
> 
> [...]

Here is the summary with links:
  - [v2,1/5] clk: qcom: regmap-mux: add pipe clk implementation
    https://git.kernel.org/qcom/c/e9a4c7f667ed
  - [v2,2/5] clk: qcom: gcc-sm8450: use new clk_regmap_mux_safe_ops for PCIe pipe clocks
    https://git.kernel.org/qcom/c/fa5ad5c51706
  - [v2,3/5] clk: qcom: gcc-sc7280: use new clk_regmap_mux_safe_ops for PCIe pipe clocks
    https://git.kernel.org/qcom/c/a9ed9e2bf794
  - [v2,4/5] PCI: qcom: Remove unnecessary pipe_clk handling
    (no matching commit)
  - [v2,5/5] PCI: qcom: Drop manual pipe_clk_src handling
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 1/5] clk: qcom: regmap-mux: add pipe clk implementation

[Dmitry Baryshkov]

On 13/04/2022 12:15, Johan Hovold wrote:
> On Tue, Apr 12, 2022 at 10:38:35PM +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'm starting to think this really belongs in the PHY driver which is the
> provider of the pipe clock. Moving it there would also allow the code to
> be shared between PCIe, USB, and UFS.
> 
> The PHY driver enables the pipe clock by starting the PHY and before
> doing so there's no point in updating the mux. Similarly, the PHY driver
> can restore the "safe" source after disabling the pipe clock.


I thought about this at some point. However it would still mean that the 
driver does the dance manually: disable pipe_clock, switch parent, 
sleep, switch the parent back, enable pipe clock. Switching parents is 
tied to disabling pipe_clock, so enforcing this link seems like a better 
option to me.

No to mention that it would complicate already overcomplicated QMP driver.

> That way there's no magic happening behind scenes, the clock framework
> always reports the actual state of the tree, and the reason for all of
> this can be documented in the QMP PHY driver once and for all.

We already have such 'magic' for the RCG2 (clk_rcg2_shared_ops), with 
the very practical reason. If the clock is running from the tcxo, it is 
as good as disabled from the practical purpose.

> The only change to the bindings compared to what this series proposes is
> that the PHY driver also needs a reference to bi_tcxo.

And this looks as bad, as providing bi_tcxo to the PCI device. From the 
schematics/silicon point of view neither of them actually uses these 
parents. Neither of them uses the pipe_clock_src. What do they need is 
just the pipe_clock. The rest should be in the programming API.

> 
> Also note that updating the mux separately from starting the PHY as this
> series allows for, doesn't really make the pipe clock any safer to use.
> 
> Either way, there are also some problems with this safe-mux
> implementation that I point out below.
> 
>> 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. As we are changing the parent
>> behind the back of the clock framework, also implement custom
>> set_parent() and get_parent() operations behaving accroding to the clock
>> framework expectations (cache the new parent if the clock is in disabled
>> state, return cached parent).
>>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/clk/qcom/clk-regmap-mux.c | 78 +++++++++++++++++++++++++++++++
>>   drivers/clk/qcom/clk-regmap-mux.h |  3 ++
>>   2 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-regmap-mux.c b/drivers/clk/qcom/clk-regmap-mux.c
>> index 45d9cca28064..c39ee783ee83 100644
>> --- a/drivers/clk/qcom/clk-regmap-mux.c
>> +++ b/drivers/clk/qcom/clk-regmap-mux.c
>> @@ -49,9 +49,87 @@ static int mux_set_parent(struct clk_hw *hw, u8 index)
>>   	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
>>   }
>>   
>> +static u8 mux_safe_get_parent(struct clk_hw *hw)
>> +{
>> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
>> +	unsigned int val;
>> +
>> +	if (clk_hw_is_enabled(hw))
>> +		return mux_get_parent(hw);
>> +
>> +	val = mux->stored_parent_cfg;
>> +
>> +	if (mux->parent_map)
>> +		return qcom_find_cfg_index(hw, mux->parent_map, val);
>> +
>> +	return val;
>> +}
>> +
>> +static int mux_safe_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
>> +
>> +	if (clk_hw_is_enabled(hw))
>> +		return mux_set_parent(hw, index);
>> +
>> +	if (mux->parent_map)
>> +		index = mux->parent_map[index].cfg;
>> +
>> +	mux->stored_parent_cfg = index;
>> +
>> +	return 0;
>> +}
>> +
>> +static void mux_safe_disable(struct clk_hw *hw)
>> +{
>> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
>> +	struct clk_regmap *clkr = to_clk_regmap(hw);
>> +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
>> +	unsigned int val;
>> +
>> +	regmap_read(clkr->regmap, mux->reg, &val);
>> +
>> +	mux->stored_parent_cfg = (val & mask) >> mux->shift;
>> +
>> +	val = mux->safe_src_parent;
>> +	if (mux->parent_map) {
>> +		int index = qcom_find_src_index(hw, mux->parent_map, val);
>> +
>> +		if (WARN_ON(index < 0))
>> +			return;
>> +
>> +		val = mux->parent_map[index].cfg;
>> +	}
>> +	val <<= mux->shift;
>> +
>> +	regmap_update_bits(clkr->regmap, mux->reg, mask, val);
>> +}
>> +
>> +static int mux_safe_enable(struct clk_hw *hw)
>> +{
>> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
>> +	struct clk_regmap *clkr = to_clk_regmap(hw);
>> +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
>> +	unsigned int val;
>> +
>> +	val = mux->stored_parent_cfg;
>> +	val <<= mux->shift;
>> +
>> +	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
>> +}
> 
> The caching of the parent is broken since set_parent() is typically not
> called before enabling the clock.
> 
> This means that the above code will set the mux to its zero-initialised
> value, which currently only works by chance as the pipe clock config
> value happens to be zero.
> 
> For this to work generally, you'd also need to define also the
> (default/initial) non-safe parent for each mux. Handling handover from
> the bootloader might also be tricky.
> 
> Furthermore, the current implementation appears to ignore locking and
> doesn't handle the case where set_parent() races with enable(). The
> former is protected by the prepare mutex and the latter by the enable
> spinlock and a driver that needs to serialise the two needs to handle
> that itself.
> 
>> +
>>   const struct clk_ops clk_regmap_mux_closest_ops = {
>>   	.get_parent = mux_get_parent,
>>   	.set_parent = mux_set_parent,
>>   	.determine_rate = __clk_mux_determine_rate_closest,
>>   };
>>   EXPORT_SYMBOL_GPL(clk_regmap_mux_closest_ops);
>> +
>> +const struct clk_ops clk_regmap_mux_safe_ops = {
>> +	.enable = mux_safe_enable,
>> +	.disable = mux_safe_disable,
>> +	.get_parent = mux_safe_get_parent,
>> +	.set_parent = mux_safe_set_parent,
>> +	.determine_rate = __clk_mux_determine_rate_closest,
>> +};
>> +EXPORT_SYMBOL_GPL(clk_regmap_mux_safe_ops);
>> diff --git a/drivers/clk/qcom/clk-regmap-mux.h b/drivers/clk/qcom/clk-regmap-mux.h
>> index db6f4cdd9586..f86c674ce139 100644
>> --- a/drivers/clk/qcom/clk-regmap-mux.h
>> +++ b/drivers/clk/qcom/clk-regmap-mux.h
>> @@ -14,10 +14,13 @@ struct clk_regmap_mux {
>>   	u32			reg;
>>   	u32			shift;
>>   	u32			width;
>> +	u8			safe_src_parent;
>> +	u8			stored_parent_cfg;
>>   	const struct parent_map	*parent_map;
>>   	struct clk_regmap	clkr;
>>   };
>>   
>>   extern const struct clk_ops clk_regmap_mux_closest_ops;
>> +extern const struct clk_ops clk_regmap_mux_safe_ops;
>>   
>>   #endif
> 
> Johan


-- 
With best wishes
Dmitry

Re: [PATCH v2 0/5] PCI: qcom: rework pipe_clk/pipe_clk_src handling

[Dmitry Baryshkov]

On 13/04/2022 12:20, Johan Hovold wrote:
> [ Dropping tdas@codeaurora.org, which bounces. ]
> 
> On Tue, Apr 12, 2022 at 10:38:34PM +0300, Dmitry Baryshkov wrote:
>> PCIe pipe clk (and some other clocks) must be parked to the "safe"
>> source (bi_tcxo) when corresponding GDSC is turned off and on again.
>> Currently this is handcoded in the PCIe driver by reparenting the
>> gcc_pipe_N_clk_src clock.
>>
>> Instead of doing it manually, follow the approach used by
>> clk_rcg2_shared_ops and implement this parking in the enable() and
>> disable() clock operations for respective pipe clocks.
>>
>> PCIe part depends on [1].
> 
> Looks like you forgot to add the link to the prerequisite patch:
> 
> 	[1] https://lore.kernel.org/all/20220401133351.10113-1-johan+linaro@kernel.org/

yes, thank you for the correction

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


-- 
With best wishes
Dmitry

Re: [PATCH v2 1/5] clk: qcom: regmap-mux: add pipe clk implementation

[Dmitry Baryshkov]

On 13/04/2022 12:15, Johan Hovold wrote:
> On Tue, Apr 12, 2022 at 10:38:35PM +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'm starting to think this really belongs in the PHY driver which is the
> provider of the pipe clock. Moving it there would also allow the code to
> be shared between PCIe, USB, and UFS.
> 
> The PHY driver enables the pipe clock by starting the PHY and before
> doing so there's no point in updating the mux. Similarly, the PHY driver
> can restore the "safe" source after disabling the pipe clock.
> 
> That way there's no magic happening behind scenes, the clock framework
> always reports the actual state of the tree, and the reason for all of
> this can be documented in the QMP PHY driver once and for all.
> 
> The only change to the bindings compared to what this series proposes is
> that the PHY driver also needs a reference to bi_tcxo.
> 
> Also note that updating the mux separately from starting the PHY as this
> series allows for, doesn't really make the pipe clock any safer to use.
> 
> Either way, there are also some problems with this safe-mux
> implementation that I point out below.
> 
>> 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. As we are changing the parent
>> behind the back of the clock framework, also implement custom
>> set_parent() and get_parent() operations behaving accroding to the clock
>> framework expectations (cache the new parent if the clock is in disabled
>> state, return cached parent).
>>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/clk/qcom/clk-regmap-mux.c | 78 +++++++++++++++++++++++++++++++
>>   drivers/clk/qcom/clk-regmap-mux.h |  3 ++
>>   2 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-regmap-mux.c b/drivers/clk/qcom/clk-regmap-mux.c
>> index 45d9cca28064..c39ee783ee83 100644
>> --- a/drivers/clk/qcom/clk-regmap-mux.c
>> +++ b/drivers/clk/qcom/clk-regmap-mux.c
>> @@ -49,9 +49,87 @@ static int mux_set_parent(struct clk_hw *hw, u8 index)
>>   	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
>>   }
>>   
>> +static u8 mux_safe_get_parent(struct clk_hw *hw)
>> +{
>> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
>> +	unsigned int val;
>> +
>> +	if (clk_hw_is_enabled(hw))
>> +		return mux_get_parent(hw);
>> +
>> +	val = mux->stored_parent_cfg;
>> +
>> +	if (mux->parent_map)
>> +		return qcom_find_cfg_index(hw, mux->parent_map, val);
>> +
>> +	return val;
>> +}
>> +
>> +static int mux_safe_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
>> +
>> +	if (clk_hw_is_enabled(hw))
>> +		return mux_set_parent(hw, index);
>> +
>> +	if (mux->parent_map)
>> +		index = mux->parent_map[index].cfg;
>> +
>> +	mux->stored_parent_cfg = index;
>> +
>> +	return 0;
>> +}
>> +
>> +static void mux_safe_disable(struct clk_hw *hw)
>> +{
>> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
>> +	struct clk_regmap *clkr = to_clk_regmap(hw);
>> +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
>> +	unsigned int val;
>> +
>> +	regmap_read(clkr->regmap, mux->reg, &val);
>> +
>> +	mux->stored_parent_cfg = (val & mask) >> mux->shift;
>> +
>> +	val = mux->safe_src_parent;
>> +	if (mux->parent_map) {
>> +		int index = qcom_find_src_index(hw, mux->parent_map, val);
>> +
>> +		if (WARN_ON(index < 0))
>> +			return;
>> +
>> +		val = mux->parent_map[index].cfg;
>> +	}
>> +	val <<= mux->shift;
>> +
>> +	regmap_update_bits(clkr->regmap, mux->reg, mask, val);
>> +}
>> +
>> +static int mux_safe_enable(struct clk_hw *hw)
>> +{
>> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
>> +	struct clk_regmap *clkr = to_clk_regmap(hw);
>> +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
>> +	unsigned int val;
>> +
>> +	val = mux->stored_parent_cfg;
>> +	val <<= mux->shift;
>> +
>> +	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
>> +}
> 
> The caching of the parent is broken since set_parent() is typically not
> called before enabling the clock.
> 
> This means that the above code will set the mux to its zero-initialised
> value, which currently only works by chance as the pipe clock config
> value happens to be zero.
> 
> For this to work generally, you'd also need to define also the
> (default/initial) non-safe parent for each mux. Handling handover from
> the bootloader might also be tricky.

It's not tricky at all. We can set stored_parent_cfg from gcc probe from 
function. Or set statically from the config. I'll probably do the latter.

> Furthermore, the current implementation appears to ignore locking and
> doesn't handle the case where set_parent() races with enable(). The
> former is protected by the prepare mutex and the latter by the enable
> spinlock and a driver that needs to serialise the two needs to handle
> that itself.

Since I'm trying to remove pipe_clk usage from pcie driver itself, there 
is just one user left - qmp phy. And while you are correct that there is 
a race, I think we can neglect that for now. Or shift enable/disable ops 
to prepare/unprepare, thus using the same mutex everywhere.


> 
>> +
>>   const struct clk_ops clk_regmap_mux_closest_ops = {
>>   	.get_parent = mux_get_parent,
>>   	.set_parent = mux_set_parent,
>>   	.determine_rate = __clk_mux_determine_rate_closest,
>>   };
>>   EXPORT_SYMBOL_GPL(clk_regmap_mux_closest_ops);
>> +
>> +const struct clk_ops clk_regmap_mux_safe_ops = {
>> +	.enable = mux_safe_enable,
>> +	.disable = mux_safe_disable,
>> +	.get_parent = mux_safe_get_parent,
>> +	.set_parent = mux_safe_set_parent,
>> +	.determine_rate = __clk_mux_determine_rate_closest,
>> +};
>> +EXPORT_SYMBOL_GPL(clk_regmap_mux_safe_ops);
>> diff --git a/drivers/clk/qcom/clk-regmap-mux.h b/drivers/clk/qcom/clk-regmap-mux.h
>> index db6f4cdd9586..f86c674ce139 100644
>> --- a/drivers/clk/qcom/clk-regmap-mux.h
>> +++ b/drivers/clk/qcom/clk-regmap-mux.h
>> @@ -14,10 +14,13 @@ struct clk_regmap_mux {
>>   	u32			reg;
>>   	u32			shift;
>>   	u32			width;
>> +	u8			safe_src_parent;
>> +	u8			stored_parent_cfg;
>>   	const struct parent_map	*parent_map;
>>   	struct clk_regmap	clkr;
>>   };
>>   
>>   extern const struct clk_ops clk_regmap_mux_closest_ops;
>> +extern const struct clk_ops clk_regmap_mux_safe_ops;
>>   
>>   #endif
> 
> Johan


-- 
With best wishes
Dmitry

Re: [PATCH v2 1/5] clk: qcom: regmap-mux: add pipe clk implementation

[Johan Hovold]

[ Please trim your replies. ]

On Wed, Apr 13, 2022 at 08:57:47PM +0300, Dmitry Baryshkov wrote:
> On 13/04/2022 12:15, Johan Hovold wrote:
> > On Tue, Apr 12, 2022 at 10:38:35PM +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'm starting to think this really belongs in the PHY driver which is the
> > provider of the pipe clock. Moving it there would also allow the code to
> > be shared between PCIe, USB, and UFS.
> > 
> > The PHY driver enables the pipe clock by starting the PHY and before
> > doing so there's no point in updating the mux. Similarly, the PHY driver
> > can restore the "safe" source after disabling the pipe clock.
> 
> I thought about this at some point. However it would still mean that the 
> driver does the dance manually: disable pipe_clock, switch parent, 
> sleep, switch the parent back, enable pipe clock. Switching parents is 
> tied to disabling pipe_clock, so enforcing this link seems like a better 
> option to me.

No, that's precisely my point. It is not tied to disabling (gating) the
pipe clock, it is tied to powering down the PHY (i.e. disabling the pipe
clock source). And that is under the control of the PHY driver.

In practise, once we've cleaned up the other users of the pipe clock,
tying it to pipe clock disabling will work, but it doesn't prevent
anyone from shooting themselves in the foot as the "safe-mux" name
suggests (i.e. it is still possibly to enable the pipe clock while its
source is disabled).

> No to mention that it would complicate already overcomplicated QMP driver.

That driver sure could use some love, but that's not a valid argument
against adding things were they belong.

And regarding complexity, I have a working prototype implementation here
which is smaller than what you're proposing and very straight forward.

> > That way there's no magic happening behind scenes, the clock framework
> > always reports the actual state of the tree, and the reason for all of
> > this can be documented in the QMP PHY driver once and for all.
> 
> We already have such 'magic' for the RCG2 (clk_rcg2_shared_ops), with 
> the very practical reason. If the clock is running from the tcxo, it is 
> as good as disabled from the practical purpose.

That implementation doesn't try to implement the caching you're
proposing and hence doesn't suffer from the associated implementation
issues.
 
> > The only change to the bindings compared to what this series proposes is
> > that the PHY driver also needs a reference to bi_tcxo.
> 
> And this looks as bad, as providing bi_tcxo to the PCI device. From the 
> schematics/silicon point of view neither of them actually uses these 
> parents. Neither of them uses the pipe_clock_src. What do they need is 
> just the pipe_clock. The rest should be in the programming API.

No, the PHY driver is both the provider of the source clock for the
pipe clock and the consumer of the latter.

That it may need to handle any muxes in between the two only makes
sense.

Hiding this away and spreading the implementation out over multiple
clock drivers (i.e. every mux definition for each platform + the
regmap-mux hack) only obscures things.

Johan

Re: [PATCH v2 1/5] clk: qcom: regmap-mux: add pipe clk implementation

[Johan Hovold]

On Thu, Apr 14, 2022 at 01:21:29AM +0300, Dmitry Baryshkov wrote:
> On 13/04/2022 12:15, Johan Hovold wrote:
> > On Tue, Apr 12, 2022 at 10:38:35PM +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.

> > The caching of the parent is broken since set_parent() is typically not
> > called before enabling the clock.
> > 
> > This means that the above code will set the mux to its zero-initialised
> > value, which currently only works by chance as the pipe clock config
> > value happens to be zero.
> > 
> > For this to work generally, you'd also need to define also the
> > (default/initial) non-safe parent for each mux. Handling handover from
> > the bootloader might also be tricky.
> 
> It's not tricky at all. We can set stored_parent_cfg from gcc probe from 
> function. Or set statically from the config. I'll probably do the latter.

That means you're just ignoring the problem. How is hard coding the
initial mux configuration in any way dealing with bootloader handover?
 
> > Furthermore, the current implementation appears to ignore locking and
> > doesn't handle the case where set_parent() races with enable(). The
> > former is protected by the prepare mutex and the latter by the enable
> > spinlock and a driver that needs to serialise the two needs to handle
> > that itself.
> 
> Since I'm trying to remove pipe_clk usage from pcie driver itself, there 
> is just one user left - qmp phy. And while you are correct that there is 
> a race, I think we can neglect that for now. Or shift enable/disable ops 
> to prepare/unprepare, thus using the same mutex everywhere.

Let's not add known broken code. Correctness first.

Handling the muxing the prepare/unprepare might work. I even considered
using those callbacks to let CCF know about the reparenting so that for
example debugfs continues to reflect the actual state of things.

But I'm still leaning towards this being a misguided endeavour as I've
explained elsewhere.

Johan