[PATCH RFC 0/5] phy: qcom-qmp: add support for pipe clock muxing

[PATCH RFC 0/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold]

Some QMP PHYs need to remux to their pipe clock input to the pipe clock
output generated by the PHY before powering on the PHY and restore the
default source during power down.

         +---------------+
         |   PHY block   |<<---------------------------------------+
         |               |                   +-----+               |
         |   +-------+   |                   | GCC |               |
   I/P---^-->|  PLL  |---^--->pipe_clksrc--->|-\   |		   |
    clk  |   +-------+   |                   |  +--|--->pipe_clk---+
    	 |		 |	       xo--->|-/   |  
         +---------------+	             +-----|

This series moves the pipe clock mux handling from PCIe controller
driver into the PHY driver which already manages the pipe clock as the
PHY is both the producer of the pipe clock as well as its (primary)
consumer.

This is an alternative to the series posted here

	https://lore.kernel.org/r/20220413233144.275926-1-dmitry.baryshkov@linaro.org

which instead moves the mux handling into the clock drivers and ties
remuxing to the gating/ungating of the pipe clock in the GCC. That
approach means that it is still possible to have the pipe clock ungated
without providing a valid source, somethings which can cause hangs when
enabling/disabling the GDSC (presumably as some entity is consuming the
pipe clock).

Implementing this in the clock drivers also means that the
implementation is spread out over multiple files and makes it harder to
add support for new SoCs as it may not be clear that every pipe clock
mux definition needs to be updated with safe and non-safe parent-clock
indexes and an ad-hoc mux ops implementation.

The custom mux implementation currently also hides the actual topology
of the clock tree by always reporting the pipe mux source as being
provided by the PHY (e.g. as reported by debugfs).

This series, by contrast, ties the muxing to when the pipe clock source
is enabled, that is, when the PHY is powered on, so that the GCC pipe
clock always has a valid source.

The implementation is more straight-forward, avoids the one-off clock
mux implementation, and allows for documenting this once and for all in
the PHY driver.

Note that the devicetree bindings remains to be updated but that this
was left out of this RFC.

This series depends on the two qcom-qmp fixlets posted here:

	https://lore.kernel.org/all/20220420152331.5527-1-johan+linaro@kernel.org/

Johan


Dmitry Baryshkov (1):
  PCI: qcom: Remove unnecessary pipe_clk handling

Johan Hovold (4):
  phy: qcom-qmp: add support for pipe clock muxing
  arm64: dts: qcom: sc7280: move pipe mux handling to phy
  PCI: qcom: Drop pipe clock muxing
  PCI: qcom: Drop unused post-init callbacks

 arch/arm64/boot/dts/qcom/sc7280.dtsi   | 18 ++---
 drivers/pci/controller/dwc/pcie-qcom.c | 96 ++------------------------
 drivers/phy/qualcomm/phy-qcom-qmp.c    | 71 +++++++++++++++++--
 3 files changed, 76 insertions(+), 109 deletions(-)

-- 
2.35.1

[PATCH RFC 0/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold]

Some QMP PHYs need to remux to their pipe clock input to the pipe clock
output generated by the PHY before powering on the PHY and restore the
default source during power down.

         +---------------+
         |   PHY block   |<<---------------------------------------+
         |               |                   +-----+               |
         |   +-------+   |                   | GCC |               |
   I/P---^-->|  PLL  |---^--->pipe_clksrc--->|-\   |		   |
    clk  |   +-------+   |                   |  +--|--->pipe_clk---+
    	 |		 |	       xo--->|-/   |  
         +---------------+	             +-----|

This series moves the pipe clock mux handling from PCIe controller
driver into the PHY driver which already manages the pipe clock as the
PHY is both the producer of the pipe clock as well as its (primary)
consumer.

This is an alternative to the series posted here

	https://lore.kernel.org/r/20220413233144.275926-1-dmitry.baryshkov@linaro.org

which instead moves the mux handling into the clock drivers and ties
remuxing to the gating/ungating of the pipe clock in the GCC. That
approach means that it is still possible to have the pipe clock ungated
without providing a valid source, somethings which can cause hangs when
enabling/disabling the GDSC (presumably as some entity is consuming the
pipe clock).

Implementing this in the clock drivers also means that the
implementation is spread out over multiple files and makes it harder to
add support for new SoCs as it may not be clear that every pipe clock
mux definition needs to be updated with safe and non-safe parent-clock
indexes and an ad-hoc mux ops implementation.

The custom mux implementation currently also hides the actual topology
of the clock tree by always reporting the pipe mux source as being
provided by the PHY (e.g. as reported by debugfs).

This series, by contrast, ties the muxing to when the pipe clock source
is enabled, that is, when the PHY is powered on, so that the GCC pipe
clock always has a valid source.

The implementation is more straight-forward, avoids the one-off clock
mux implementation, and allows for documenting this once and for all in
the PHY driver.

Note that the devicetree bindings remains to be updated but that this
was left out of this RFC.

This series depends on the two qcom-qmp fixlets posted here:

	https://lore.kernel.org/all/20220420152331.5527-1-johan+linaro@kernel.org/

Johan


Dmitry Baryshkov (1):
  PCI: qcom: Remove unnecessary pipe_clk handling

Johan Hovold (4):
  phy: qcom-qmp: add support for pipe clock muxing
  arm64: dts: qcom: sc7280: move pipe mux handling to phy
  PCI: qcom: Drop pipe clock muxing
  PCI: qcom: Drop unused post-init callbacks

 arch/arm64/boot/dts/qcom/sc7280.dtsi   | 18 ++---
 drivers/pci/controller/dwc/pcie-qcom.c | 96 ++------------------------
 drivers/phy/qualcomm/phy-qcom-qmp.c    | 71 +++++++++++++++++--
 3 files changed, 76 insertions(+), 109 deletions(-)

-- 
2.35.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold]

Some QMP PHYs need to remux to their pipe clock input to the pipe clock
output generated by the PHY before powering on the PHY and restore the
default source during power down.

Add support for an optional pipe clock mux which will be reparented to
the generated pipe clock before powering on the PHY and restored to the
default reference source on power off.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 6 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 7d2d1ab061f7..bc6db9670291 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
  * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
  * @pcs_misc: iomapped memory space for lane's pcs_misc
  * @pipe_clk: pipe clock
+ * @pipemux_clk: pipe clock source mux
+ * @piperef_clk: pipe clock default reference source
  * @index: lane index
  * @qmp: QMP phy to which this lane belongs
  * @lane_rst: lane's reset controller
@@ -3311,6 +3313,8 @@ struct qmp_phy {
 	void __iomem *rx2;
 	void __iomem *pcs_misc;
 	struct clk *pipe_clk;
+	struct clk *pipemux_clk;
+	struct clk *piperef_clk;
 	unsigned int index;
 	struct qcom_qmp *qmp;
 	struct reset_control *lane_rst;
@@ -3346,6 +3350,7 @@ struct qcom_qmp {
 	void __iomem *dp_com;
 
 	struct clk_bulk_data *clks;
+	struct clk *pipe_clksrc;
 	struct reset_control **resets;
 	struct regulator_bulk_data *vregs;
 
@@ -5355,6 +5360,42 @@ static int qcom_qmp_phy_init(struct phy *phy)
 	return 0;
 }
 
+static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy)
+{
+	struct qcom_qmp *qmp = qphy->qmp;
+	int ret;
+
+	ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc);
+	if (ret)
+		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
+
+
+	ret = clk_prepare_enable(qphy->pipe_clk);
+	if (ret) {
+		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
+		goto err_restore_parent;
+	}
+
+	return 0;
+
+err_restore_parent:
+	clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
+
+	return ret;
+}
+
+static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy)
+{
+	struct qcom_qmp *qmp = qphy->qmp;
+	int ret;
+
+	clk_disable_unprepare(qphy->pipe_clk);
+
+	ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
+	if (ret)
+		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
+}
+
 static int qcom_qmp_phy_power_on(struct phy *phy)
 {
 	struct qmp_phy *qphy = phy_get_drvdata(phy);
@@ -5379,11 +5420,9 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
 		}
 	}
 
-	ret = clk_prepare_enable(qphy->pipe_clk);
-	if (ret) {
-		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
+	ret = qcom_qmp_phy_pipe_clk_enable(qphy);
+	if (ret)
 		goto err_reset_lane;
-	}
 
 	/* Tx, Rx, and PCS configurations */
 	qcom_qmp_phy_configure_lane(tx, cfg->regs,
@@ -5478,7 +5517,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
 	return 0;
 
 err_disable_pipe_clk:
-	clk_disable_unprepare(qphy->pipe_clk);
+	qcom_qmp_phy_pipe_clk_disable(qphy);
 err_reset_lane:
 	if (cfg->has_lane_rst)
 		reset_control_assert(qphy->lane_rst);
@@ -5491,7 +5530,7 @@ static int qcom_qmp_phy_power_off(struct phy *phy)
 	struct qmp_phy *qphy = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qphy->cfg;
 
-	clk_disable_unprepare(qphy->pipe_clk);
+	qcom_qmp_phy_pipe_clk_disable(qphy);
 
 	if (cfg->type == PHY_TYPE_DP) {
 		/* Assert DP PHY power down */
@@ -5777,6 +5816,8 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
 	if (ret)
 		return ret;
 
+	qmp->pipe_clksrc = fixed->hw.clk;
+
 	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
 	if (ret)
 		return ret;
@@ -6091,6 +6132,24 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id,
 		qphy->pipe_clk = NULL;
 	}
 
+	/* Get optional pipe clock mux and default reference source clock. */
+	qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
+	if (IS_ERR(qphy->pipemux_clk)) {
+		ret = PTR_ERR(qphy->pipemux_clk);
+		if (ret == -EPROBE_DEFER)
+			return ret;
+
+		qphy->pipemux_clk = NULL;
+	} else {
+		qphy->piperef_clk = of_clk_get_by_name(np, "ref");
+		if (IS_ERR(qphy->piperef_clk)) {
+			ret = PTR_ERR(qphy->piperef_clk);
+			return dev_err_probe(dev, ret,
+					     "failed to get lane%d piperef_clk\n",
+					     id);
+		}
+	}
+
 	/* Get lane reset, if any */
 	if (cfg->has_lane_rst) {
 		snprintf(prop_name, sizeof(prop_name), "lane%d", id);
-- 
2.35.1

[PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold]

Some QMP PHYs need to remux to their pipe clock input to the pipe clock
output generated by the PHY before powering on the PHY and restore the
default source during power down.

Add support for an optional pipe clock mux which will be reparented to
the generated pipe clock before powering on the PHY and restored to the
default reference source on power off.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 6 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 7d2d1ab061f7..bc6db9670291 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
  * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
  * @pcs_misc: iomapped memory space for lane's pcs_misc
  * @pipe_clk: pipe clock
+ * @pipemux_clk: pipe clock source mux
+ * @piperef_clk: pipe clock default reference source
  * @index: lane index
  * @qmp: QMP phy to which this lane belongs
  * @lane_rst: lane's reset controller
@@ -3311,6 +3313,8 @@ struct qmp_phy {
 	void __iomem *rx2;
 	void __iomem *pcs_misc;
 	struct clk *pipe_clk;
+	struct clk *pipemux_clk;
+	struct clk *piperef_clk;
 	unsigned int index;
 	struct qcom_qmp *qmp;
 	struct reset_control *lane_rst;
@@ -3346,6 +3350,7 @@ struct qcom_qmp {
 	void __iomem *dp_com;
 
 	struct clk_bulk_data *clks;
+	struct clk *pipe_clksrc;
 	struct reset_control **resets;
 	struct regulator_bulk_data *vregs;
 
@@ -5355,6 +5360,42 @@ static int qcom_qmp_phy_init(struct phy *phy)
 	return 0;
 }
 
+static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy)
+{
+	struct qcom_qmp *qmp = qphy->qmp;
+	int ret;
+
+	ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc);
+	if (ret)
+		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
+
+
+	ret = clk_prepare_enable(qphy->pipe_clk);
+	if (ret) {
+		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
+		goto err_restore_parent;
+	}
+
+	return 0;
+
+err_restore_parent:
+	clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
+
+	return ret;
+}
+
+static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy)
+{
+	struct qcom_qmp *qmp = qphy->qmp;
+	int ret;
+
+	clk_disable_unprepare(qphy->pipe_clk);
+
+	ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
+	if (ret)
+		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
+}
+
 static int qcom_qmp_phy_power_on(struct phy *phy)
 {
 	struct qmp_phy *qphy = phy_get_drvdata(phy);
@@ -5379,11 +5420,9 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
 		}
 	}
 
-	ret = clk_prepare_enable(qphy->pipe_clk);
-	if (ret) {
-		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
+	ret = qcom_qmp_phy_pipe_clk_enable(qphy);
+	if (ret)
 		goto err_reset_lane;
-	}
 
 	/* Tx, Rx, and PCS configurations */
 	qcom_qmp_phy_configure_lane(tx, cfg->regs,
@@ -5478,7 +5517,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
 	return 0;
 
 err_disable_pipe_clk:
-	clk_disable_unprepare(qphy->pipe_clk);
+	qcom_qmp_phy_pipe_clk_disable(qphy);
 err_reset_lane:
 	if (cfg->has_lane_rst)
 		reset_control_assert(qphy->lane_rst);
@@ -5491,7 +5530,7 @@ static int qcom_qmp_phy_power_off(struct phy *phy)
 	struct qmp_phy *qphy = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qphy->cfg;
 
-	clk_disable_unprepare(qphy->pipe_clk);
+	qcom_qmp_phy_pipe_clk_disable(qphy);
 
 	if (cfg->type == PHY_TYPE_DP) {
 		/* Assert DP PHY power down */
@@ -5777,6 +5816,8 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
 	if (ret)
 		return ret;
 
+	qmp->pipe_clksrc = fixed->hw.clk;
+
 	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
 	if (ret)
 		return ret;
@@ -6091,6 +6132,24 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id,
 		qphy->pipe_clk = NULL;
 	}
 
+	/* Get optional pipe clock mux and default reference source clock. */
+	qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
+	if (IS_ERR(qphy->pipemux_clk)) {
+		ret = PTR_ERR(qphy->pipemux_clk);
+		if (ret == -EPROBE_DEFER)
+			return ret;
+
+		qphy->pipemux_clk = NULL;
+	} else {
+		qphy->piperef_clk = of_clk_get_by_name(np, "ref");
+		if (IS_ERR(qphy->piperef_clk)) {
+			ret = PTR_ERR(qphy->piperef_clk);
+			return dev_err_probe(dev, ret,
+					     "failed to get lane%d piperef_clk\n",
+					     id);
+		}
+	}
+
 	/* Get lane reset, if any */
 	if (cfg->has_lane_rst) {
 		snprintf(prop_name, sizeof(prop_name), "lane%d", id);
-- 
2.35.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Dmitry Baryshkov]

On 21/04/2022 13:20, Johan Hovold wrote:
> Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> output generated by the PHY before powering on the PHY and restore the
> default source during power down.
> 
> Add support for an optional pipe clock mux which will be reparented to
> the generated pipe clock before powering on the PHY and restored to the
> default reference source on power off.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
>   1 file changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 7d2d1ab061f7..bc6db9670291 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
>    * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
>    * @pcs_misc: iomapped memory space for lane's pcs_misc
>    * @pipe_clk: pipe clock
> + * @pipemux_clk: pipe clock source mux
> + * @piperef_clk: pipe clock default reference source
>    * @index: lane index
>    * @qmp: QMP phy to which this lane belongs
>    * @lane_rst: lane's reset controller
> @@ -3311,6 +3313,8 @@ struct qmp_phy {
>   	void __iomem *rx2;
>   	void __iomem *pcs_misc;
>   	struct clk *pipe_clk;
> +	struct clk *pipemux_clk;
> +	struct clk *piperef_clk;
>   	unsigned int index;
>   	struct qcom_qmp *qmp;
>   	struct reset_control *lane_rst;
> @@ -3346,6 +3350,7 @@ struct qcom_qmp {
>   	void __iomem *dp_com;
>   
>   	struct clk_bulk_data *clks;
> +	struct clk *pipe_clksrc;

Please move this to qmp_phy too.

>   	struct reset_control **resets;
>   	struct regulator_bulk_data *vregs;
>   
> @@ -5355,6 +5360,42 @@ static int qcom_qmp_phy_init(struct phy *phy)
>   	return 0;
>   }
>   
> +static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy)
> +{
> +	struct qcom_qmp *qmp = qphy->qmp;
> +	int ret;
> +
> +	ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc);
> +	if (ret)
> +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +
> +
> +	ret = clk_prepare_enable(qphy->pipe_clk);
> +	if (ret) {
> +		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> +		goto err_restore_parent;
> +	}
> +
> +	return 0;
> +
> +err_restore_parent:
> +	clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> +
> +	return ret;
> +}
> +
> +static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy)
> +{
> +	struct qcom_qmp *qmp = qphy->qmp;
> +	int ret;
> +
> +	clk_disable_unprepare(qphy->pipe_clk);
> +
> +	ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> +	if (ret)
> +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +}
> +
>   static int qcom_qmp_phy_power_on(struct phy *phy)
>   {
>   	struct qmp_phy *qphy = phy_get_drvdata(phy);
> @@ -5379,11 +5420,9 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>   		}
>   	}
>   
> -	ret = clk_prepare_enable(qphy->pipe_clk);
> -	if (ret) {
> -		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> +	ret = qcom_qmp_phy_pipe_clk_enable(qphy);
> +	if (ret)
>   		goto err_reset_lane;
> -	}
>   
>   	/* Tx, Rx, and PCS configurations */
>   	qcom_qmp_phy_configure_lane(tx, cfg->regs,
> @@ -5478,7 +5517,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>   	return 0;
>   
>   err_disable_pipe_clk:
> -	clk_disable_unprepare(qphy->pipe_clk);
> +	qcom_qmp_phy_pipe_clk_disable(qphy);
>   err_reset_lane:
>   	if (cfg->has_lane_rst)
>   		reset_control_assert(qphy->lane_rst);
> @@ -5491,7 +5530,7 @@ static int qcom_qmp_phy_power_off(struct phy *phy)
>   	struct qmp_phy *qphy = phy_get_drvdata(phy);
>   	const struct qmp_phy_cfg *cfg = qphy->cfg;
>   
> -	clk_disable_unprepare(qphy->pipe_clk);
> +	qcom_qmp_phy_pipe_clk_disable(qphy);
>   
>   	if (cfg->type == PHY_TYPE_DP) {
>   		/* Assert DP PHY power down */
> @@ -5777,6 +5816,8 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
>   	if (ret)
>   		return ret;
>   
> +	qmp->pipe_clksrc = fixed->hw.clk;
> +
>   	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
>   	if (ret)
>   		return ret;
> @@ -6091,6 +6132,24 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id,
>   		qphy->pipe_clk = NULL;
>   	}
>   
> +	/* Get optional pipe clock mux and default reference source clock. */
> +	qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> +	if (IS_ERR(qphy->pipemux_clk)) {
> +		ret = PTR_ERR(qphy->pipemux_clk);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		qphy->pipemux_clk = NULL;

This makes the driver ignore every possible erorr except -EPROBE_DEFER. 
However the driver should behave in quite the oppposite way. Please use 
devm_clk_get_optional() instead. It would do that in better way.

Not to mention that this code leaks a refcount on the clock.

> +	} else {
> +		qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> +		if (IS_ERR(qphy->piperef_clk)) {
> +			ret = PTR_ERR(qphy->piperef_clk);
> +			return dev_err_probe(dev, ret,
> +					     "failed to get lane%d piperef_clk\n",
> +					     id);
> +		}
> +	}
> +

As a second thought.
This needs to be more explicit. If the chipset requires the pipe clock 
remuxing, we must fail if the clocks were not provided. So depending on 
the qmp instance/property the driver should either use devm_clk_get() 
(instead of _optional) or skip this block completely.

But this will not work with earlier DTS files.

>   	/* Get lane reset, if any */
>   	if (cfg->has_lane_rst) {
>   		snprintf(prop_name, sizeof(prop_name), "lane%d", id);


-- 
With best wishes
Dmitry

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Dmitry Baryshkov]

On 21/04/2022 13:20, Johan Hovold wrote:
> Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> output generated by the PHY before powering on the PHY and restore the
> default source during power down.
> 
> Add support for an optional pipe clock mux which will be reparented to
> the generated pipe clock before powering on the PHY and restored to the
> default reference source on power off.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
>   1 file changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 7d2d1ab061f7..bc6db9670291 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
>    * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
>    * @pcs_misc: iomapped memory space for lane's pcs_misc
>    * @pipe_clk: pipe clock
> + * @pipemux_clk: pipe clock source mux
> + * @piperef_clk: pipe clock default reference source
>    * @index: lane index
>    * @qmp: QMP phy to which this lane belongs
>    * @lane_rst: lane's reset controller
> @@ -3311,6 +3313,8 @@ struct qmp_phy {
>   	void __iomem *rx2;
>   	void __iomem *pcs_misc;
>   	struct clk *pipe_clk;
> +	struct clk *pipemux_clk;
> +	struct clk *piperef_clk;
>   	unsigned int index;
>   	struct qcom_qmp *qmp;
>   	struct reset_control *lane_rst;
> @@ -3346,6 +3350,7 @@ struct qcom_qmp {
>   	void __iomem *dp_com;
>   
>   	struct clk_bulk_data *clks;
> +	struct clk *pipe_clksrc;

Please move this to qmp_phy too.

>   	struct reset_control **resets;
>   	struct regulator_bulk_data *vregs;
>   
> @@ -5355,6 +5360,42 @@ static int qcom_qmp_phy_init(struct phy *phy)
>   	return 0;
>   }
>   
> +static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy)
> +{
> +	struct qcom_qmp *qmp = qphy->qmp;
> +	int ret;
> +
> +	ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc);
> +	if (ret)
> +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +
> +
> +	ret = clk_prepare_enable(qphy->pipe_clk);
> +	if (ret) {
> +		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> +		goto err_restore_parent;
> +	}
> +
> +	return 0;
> +
> +err_restore_parent:
> +	clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> +
> +	return ret;
> +}
> +
> +static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy)
> +{
> +	struct qcom_qmp *qmp = qphy->qmp;
> +	int ret;
> +
> +	clk_disable_unprepare(qphy->pipe_clk);
> +
> +	ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> +	if (ret)
> +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +}
> +
>   static int qcom_qmp_phy_power_on(struct phy *phy)
>   {
>   	struct qmp_phy *qphy = phy_get_drvdata(phy);
> @@ -5379,11 +5420,9 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>   		}
>   	}
>   
> -	ret = clk_prepare_enable(qphy->pipe_clk);
> -	if (ret) {
> -		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> +	ret = qcom_qmp_phy_pipe_clk_enable(qphy);
> +	if (ret)
>   		goto err_reset_lane;
> -	}
>   
>   	/* Tx, Rx, and PCS configurations */
>   	qcom_qmp_phy_configure_lane(tx, cfg->regs,
> @@ -5478,7 +5517,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>   	return 0;
>   
>   err_disable_pipe_clk:
> -	clk_disable_unprepare(qphy->pipe_clk);
> +	qcom_qmp_phy_pipe_clk_disable(qphy);
>   err_reset_lane:
>   	if (cfg->has_lane_rst)
>   		reset_control_assert(qphy->lane_rst);
> @@ -5491,7 +5530,7 @@ static int qcom_qmp_phy_power_off(struct phy *phy)
>   	struct qmp_phy *qphy = phy_get_drvdata(phy);
>   	const struct qmp_phy_cfg *cfg = qphy->cfg;
>   
> -	clk_disable_unprepare(qphy->pipe_clk);
> +	qcom_qmp_phy_pipe_clk_disable(qphy);
>   
>   	if (cfg->type == PHY_TYPE_DP) {
>   		/* Assert DP PHY power down */
> @@ -5777,6 +5816,8 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
>   	if (ret)
>   		return ret;
>   
> +	qmp->pipe_clksrc = fixed->hw.clk;
> +
>   	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
>   	if (ret)
>   		return ret;
> @@ -6091,6 +6132,24 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id,
>   		qphy->pipe_clk = NULL;
>   	}
>   
> +	/* Get optional pipe clock mux and default reference source clock. */
> +	qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> +	if (IS_ERR(qphy->pipemux_clk)) {
> +		ret = PTR_ERR(qphy->pipemux_clk);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		qphy->pipemux_clk = NULL;

This makes the driver ignore every possible erorr except -EPROBE_DEFER. 
However the driver should behave in quite the oppposite way. Please use 
devm_clk_get_optional() instead. It would do that in better way.

Not to mention that this code leaks a refcount on the clock.

> +	} else {
> +		qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> +		if (IS_ERR(qphy->piperef_clk)) {
> +			ret = PTR_ERR(qphy->piperef_clk);
> +			return dev_err_probe(dev, ret,
> +					     "failed to get lane%d piperef_clk\n",
> +					     id);
> +		}
> +	}
> +

As a second thought.
This needs to be more explicit. If the chipset requires the pipe clock 
remuxing, we must fail if the clocks were not provided. So depending on 
the qmp instance/property the driver should either use devm_clk_get() 
(instead of _optional) or skip this block completely.

But this will not work with earlier DTS files.

>   	/* Get lane reset, if any */
>   	if (cfg->has_lane_rst) {
>   		snprintf(prop_name, sizeof(prop_name), "lane%d", id);


-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold]

On Thu, Apr 21, 2022 at 02:08:27PM +0300, Dmitry Baryshkov wrote:
> On 21/04/2022 13:20, Johan Hovold wrote:
> > Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> > output generated by the PHY before powering on the PHY and restore the
> > default source during power down.
> > 
> > Add support for an optional pipe clock mux which will be reparented to
> > the generated pipe clock before powering on the PHY and restored to the
> > default reference source on power off.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >   drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
> >   1 file changed, 65 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index 7d2d1ab061f7..bc6db9670291 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
> >    * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
> >    * @pcs_misc: iomapped memory space for lane's pcs_misc
> >    * @pipe_clk: pipe clock
> > + * @pipemux_clk: pipe clock source mux
> > + * @piperef_clk: pipe clock default reference source
> >    * @index: lane index
> >    * @qmp: QMP phy to which this lane belongs
> >    * @lane_rst: lane's reset controller
> > @@ -3311,6 +3313,8 @@ struct qmp_phy {
> >   	void __iomem *rx2;
> >   	void __iomem *pcs_misc;
> >   	struct clk *pipe_clk;
> > +	struct clk *pipemux_clk;
> > +	struct clk *piperef_clk;
> >   	unsigned int index;
> >   	struct qcom_qmp *qmp;
> >   	struct reset_control *lane_rst;
> > @@ -3346,6 +3350,7 @@ struct qcom_qmp {
> >   	void __iomem *dp_com;
> >   
> >   	struct clk_bulk_data *clks;
> > +	struct clk *pipe_clksrc;
> 
> Please move this to qmp_phy too.

Ok.
 
> > +	/* Get optional pipe clock mux and default reference source clock. */
> > +	qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> > +	if (IS_ERR(qphy->pipemux_clk)) {
> > +		ret = PTR_ERR(qphy->pipemux_clk);
> > +		if (ret == -EPROBE_DEFER)
> > +			return ret;
> > +
> > +		qphy->pipemux_clk = NULL;
> 
> This makes the driver ignore every possible erorr except -EPROBE_DEFER. 
> However the driver should behave in quite the oppposite way. Please use 
> devm_clk_get_optional() instead. It would do that in better way.

We'd need to add an optional version of devm_get_clk_from_child() for
that due to the questionable "lane" child nodes this driver uses.

The above works for an RFC, but testing for -EINVAL and -ENOENT handles
a few more theoretical errnos until an optional helper is in place.

> Not to mention that this code leaks a refcount on the clock.

True, just like the driver has been doing with the pipe clock and lane
reset since it was merged. I'll fix that up.

> > +	} else {
> > +		qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> > +		if (IS_ERR(qphy->piperef_clk)) {
> > +			ret = PTR_ERR(qphy->piperef_clk);
> > +			return dev_err_probe(dev, ret,
> > +					     "failed to get lane%d piperef_clk\n",
> > +					     id);
> > +		}
> > +	}
> > +
> 
> As a second thought.
> This needs to be more explicit. If the chipset requires the pipe clock 
> remuxing, we must fail if the clocks were not provided. So depending on 
> the qmp instance/property the driver should either use devm_clk_get() 
> (instead of _optional) or skip this block completely.

No, the kernel is not a DT validator (and we have the YAML bindings for
that now).

> But this will not work with earlier DTS files.

So this is not a problem (but if we really wanted to have the driver
validate the DT it can be done by updating the compatible strings).

Johan

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold]

On Thu, Apr 21, 2022 at 02:08:27PM +0300, Dmitry Baryshkov wrote:
> On 21/04/2022 13:20, Johan Hovold wrote:
> > Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> > output generated by the PHY before powering on the PHY and restore the
> > default source during power down.
> > 
> > Add support for an optional pipe clock mux which will be reparented to
> > the generated pipe clock before powering on the PHY and restored to the
> > default reference source on power off.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >   drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
> >   1 file changed, 65 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index 7d2d1ab061f7..bc6db9670291 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
> >    * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
> >    * @pcs_misc: iomapped memory space for lane's pcs_misc
> >    * @pipe_clk: pipe clock
> > + * @pipemux_clk: pipe clock source mux
> > + * @piperef_clk: pipe clock default reference source
> >    * @index: lane index
> >    * @qmp: QMP phy to which this lane belongs
> >    * @lane_rst: lane's reset controller
> > @@ -3311,6 +3313,8 @@ struct qmp_phy {
> >   	void __iomem *rx2;
> >   	void __iomem *pcs_misc;
> >   	struct clk *pipe_clk;
> > +	struct clk *pipemux_clk;
> > +	struct clk *piperef_clk;
> >   	unsigned int index;
> >   	struct qcom_qmp *qmp;
> >   	struct reset_control *lane_rst;
> > @@ -3346,6 +3350,7 @@ struct qcom_qmp {
> >   	void __iomem *dp_com;
> >   
> >   	struct clk_bulk_data *clks;
> > +	struct clk *pipe_clksrc;
> 
> Please move this to qmp_phy too.

Ok.
 
> > +	/* Get optional pipe clock mux and default reference source clock. */
> > +	qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> > +	if (IS_ERR(qphy->pipemux_clk)) {
> > +		ret = PTR_ERR(qphy->pipemux_clk);
> > +		if (ret == -EPROBE_DEFER)
> > +			return ret;
> > +
> > +		qphy->pipemux_clk = NULL;
> 
> This makes the driver ignore every possible erorr except -EPROBE_DEFER. 
> However the driver should behave in quite the oppposite way. Please use 
> devm_clk_get_optional() instead. It would do that in better way.

We'd need to add an optional version of devm_get_clk_from_child() for
that due to the questionable "lane" child nodes this driver uses.

The above works for an RFC, but testing for -EINVAL and -ENOENT handles
a few more theoretical errnos until an optional helper is in place.

> Not to mention that this code leaks a refcount on the clock.

True, just like the driver has been doing with the pipe clock and lane
reset since it was merged. I'll fix that up.

> > +	} else {
> > +		qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> > +		if (IS_ERR(qphy->piperef_clk)) {
> > +			ret = PTR_ERR(qphy->piperef_clk);
> > +			return dev_err_probe(dev, ret,
> > +					     "failed to get lane%d piperef_clk\n",
> > +					     id);
> > +		}
> > +	}
> > +
> 
> As a second thought.
> This needs to be more explicit. If the chipset requires the pipe clock 
> remuxing, we must fail if the clocks were not provided. So depending on 
> the qmp instance/property the driver should either use devm_clk_get() 
> (instead of _optional) or skip this block completely.

No, the kernel is not a DT validator (and we have the YAML bindings for
that now).

> But this will not work with earlier DTS files.

So this is not a problem (but if we really wanted to have the driver
validate the DT it can be done by updating the compatible strings).

Johan

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Dmitry Baryshkov]

On Fri, 22 Apr 2022 at 13:20, Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Apr 21, 2022 at 02:08:27PM +0300, Dmitry Baryshkov wrote:
> > On 21/04/2022 13:20, Johan Hovold wrote:
> > > Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> > > output generated by the PHY before powering on the PHY and restore the
> > > default source during power down.
> > >
> > > Add support for an optional pipe clock mux which will be reparented to
> > > the generated pipe clock before powering on the PHY and restored to the
> > > default reference source on power off.
> > >
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > ---
> > >   drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
> > >   1 file changed, 65 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > index 7d2d1ab061f7..bc6db9670291 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > @@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
> > >    * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
> > >    * @pcs_misc: iomapped memory space for lane's pcs_misc
> > >    * @pipe_clk: pipe clock
> > > + * @pipemux_clk: pipe clock source mux
> > > + * @piperef_clk: pipe clock default reference source
> > >    * @index: lane index
> > >    * @qmp: QMP phy to which this lane belongs
> > >    * @lane_rst: lane's reset controller
> > > @@ -3311,6 +3313,8 @@ struct qmp_phy {
> > >     void __iomem *rx2;
> > >     void __iomem *pcs_misc;
> > >     struct clk *pipe_clk;
> > > +   struct clk *pipemux_clk;
> > > +   struct clk *piperef_clk;
> > >     unsigned int index;
> > >     struct qcom_qmp *qmp;
> > >     struct reset_control *lane_rst;
> > > @@ -3346,6 +3350,7 @@ struct qcom_qmp {
> > >     void __iomem *dp_com;
> > >
> > >     struct clk_bulk_data *clks;
> > > +   struct clk *pipe_clksrc;
> >
> > Please move this to qmp_phy too.
>
> Ok.
>
> > > +   /* Get optional pipe clock mux and default reference source clock. */
> > > +   qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> > > +   if (IS_ERR(qphy->pipemux_clk)) {
> > > +           ret = PTR_ERR(qphy->pipemux_clk);
> > > +           if (ret == -EPROBE_DEFER)
> > > +                   return ret;
> > > +
> > > +           qphy->pipemux_clk = NULL;
> >
> > This makes the driver ignore every possible erorr except -EPROBE_DEFER.
> > However the driver should behave in quite the oppposite way. Please use
> > devm_clk_get_optional() instead. It would do that in better way.
>
> We'd need to add an optional version of devm_get_clk_from_child() for
> that due to the questionable "lane" child nodes this driver uses.
>
> The above works for an RFC, but testing for -EINVAL and -ENOENT handles
> a few more theoretical errnos until an optional helper is in place.
>
> > Not to mention that this code leaks a refcount on the clock.
>
> True, just like the driver has been doing with the pipe clock and lane
> reset since it was merged. I'll fix that up.
>
> > > +   } else {
> > > +           qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> > > +           if (IS_ERR(qphy->piperef_clk)) {
> > > +                   ret = PTR_ERR(qphy->piperef_clk);
> > > +                   return dev_err_probe(dev, ret,
> > > +                                        "failed to get lane%d piperef_clk\n",
> > > +                                        id);
> > > +           }
> > > +   }
> > > +
> >
> > As a second thought.
> > This needs to be more explicit. If the chipset requires the pipe clock
> > remuxing, we must fail if the clocks were not provided. So depending on
> > the qmp instance/property the driver should either use devm_clk_get()
> > (instead of _optional) or skip this block completely.
>
> No, the kernel is not a DT validator (and we have the YAML bindings for
> that now).

It is not about DT validation. It is about passing a correct DT. The
file can come up from the kernel. It can come from the older kernel.
OR it can come from the vendor. Or it even might be being a part of
firmware flashed into the device.
So we can not assume that the DT is correct just because the in-kernel
DT passes YAML validation.

So, as I wrote, the whole patchset needs much more care about compatibility.

> > But this will not work with earlier DTS files.
>
> So this is not a problem (but if we really wanted to have the driver
> validate the DT it can be done by updating the compatible strings).

We should not update compatible strings just because the driver
changes. Compat strings describe the hardware, not the Linux point of
view on it.

-- 
With best wishes
Dmitry

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Dmitry Baryshkov]

On Fri, 22 Apr 2022 at 13:20, Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Apr 21, 2022 at 02:08:27PM +0300, Dmitry Baryshkov wrote:
> > On 21/04/2022 13:20, Johan Hovold wrote:
> > > Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> > > output generated by the PHY before powering on the PHY and restore the
> > > default source during power down.
> > >
> > > Add support for an optional pipe clock mux which will be reparented to
> > > the generated pipe clock before powering on the PHY and restored to the
> > > default reference source on power off.
> > >
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > ---
> > >   drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
> > >   1 file changed, 65 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > index 7d2d1ab061f7..bc6db9670291 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > @@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
> > >    * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
> > >    * @pcs_misc: iomapped memory space for lane's pcs_misc
> > >    * @pipe_clk: pipe clock
> > > + * @pipemux_clk: pipe clock source mux
> > > + * @piperef_clk: pipe clock default reference source
> > >    * @index: lane index
> > >    * @qmp: QMP phy to which this lane belongs
> > >    * @lane_rst: lane's reset controller
> > > @@ -3311,6 +3313,8 @@ struct qmp_phy {
> > >     void __iomem *rx2;
> > >     void __iomem *pcs_misc;
> > >     struct clk *pipe_clk;
> > > +   struct clk *pipemux_clk;
> > > +   struct clk *piperef_clk;
> > >     unsigned int index;
> > >     struct qcom_qmp *qmp;
> > >     struct reset_control *lane_rst;
> > > @@ -3346,6 +3350,7 @@ struct qcom_qmp {
> > >     void __iomem *dp_com;
> > >
> > >     struct clk_bulk_data *clks;
> > > +   struct clk *pipe_clksrc;
> >
> > Please move this to qmp_phy too.
>
> Ok.
>
> > > +   /* Get optional pipe clock mux and default reference source clock. */
> > > +   qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> > > +   if (IS_ERR(qphy->pipemux_clk)) {
> > > +           ret = PTR_ERR(qphy->pipemux_clk);
> > > +           if (ret == -EPROBE_DEFER)
> > > +                   return ret;
> > > +
> > > +           qphy->pipemux_clk = NULL;
> >
> > This makes the driver ignore every possible erorr except -EPROBE_DEFER.
> > However the driver should behave in quite the oppposite way. Please use
> > devm_clk_get_optional() instead. It would do that in better way.
>
> We'd need to add an optional version of devm_get_clk_from_child() for
> that due to the questionable "lane" child nodes this driver uses.
>
> The above works for an RFC, but testing for -EINVAL and -ENOENT handles
> a few more theoretical errnos until an optional helper is in place.
>
> > Not to mention that this code leaks a refcount on the clock.
>
> True, just like the driver has been doing with the pipe clock and lane
> reset since it was merged. I'll fix that up.
>
> > > +   } else {
> > > +           qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> > > +           if (IS_ERR(qphy->piperef_clk)) {
> > > +                   ret = PTR_ERR(qphy->piperef_clk);
> > > +                   return dev_err_probe(dev, ret,
> > > +                                        "failed to get lane%d piperef_clk\n",
> > > +                                        id);
> > > +           }
> > > +   }
> > > +
> >
> > As a second thought.
> > This needs to be more explicit. If the chipset requires the pipe clock
> > remuxing, we must fail if the clocks were not provided. So depending on
> > the qmp instance/property the driver should either use devm_clk_get()
> > (instead of _optional) or skip this block completely.
>
> No, the kernel is not a DT validator (and we have the YAML bindings for
> that now).

It is not about DT validation. It is about passing a correct DT. The
file can come up from the kernel. It can come from the older kernel.
OR it can come from the vendor. Or it even might be being a part of
firmware flashed into the device.
So we can not assume that the DT is correct just because the in-kernel
DT passes YAML validation.

So, as I wrote, the whole patchset needs much more care about compatibility.

> > But this will not work with earlier DTS files.
>
> So this is not a problem (but if we really wanted to have the driver
> validate the DT it can be done by updating the compatible strings).

We should not update compatible strings just because the driver
changes. Compat strings describe the hardware, not the Linux point of
view on it.

-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold]

[ Again, please trim unnecessary context from your replies. ]

On Fri, Apr 22, 2022 at 01:35:01PM +0300, Dmitry Baryshkov wrote:
> On Fri, 22 Apr 2022 at 13:20, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Apr 21, 2022 at 02:08:27PM +0300, Dmitry Baryshkov wrote:
> > > On 21/04/2022 13:20, Johan Hovold wrote:

> > > > +   /* Get optional pipe clock mux and default reference source clock. */
> > > > +   qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> > > > +   if (IS_ERR(qphy->pipemux_clk)) {
> > > > +           ret = PTR_ERR(qphy->pipemux_clk);
> > > > +           if (ret == -EPROBE_DEFER)
> > > > +                   return ret;
> > > > +
> > > > +           qphy->pipemux_clk = NULL;

> > > > +   } else {
> > > > +           qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> > > > +           if (IS_ERR(qphy->piperef_clk)) {
> > > > +                   ret = PTR_ERR(qphy->piperef_clk);
> > > > +                   return dev_err_probe(dev, ret,
> > > > +                                        "failed to get lane%d piperef_clk\n",
> > > > +                                        id);
> > > > +           }
> > > > +   }
> > > > +
> > >
> > > As a second thought.
> > > This needs to be more explicit. If the chipset requires the pipe clock
> > > remuxing, we must fail if the clocks were not provided. So depending on
> > > the qmp instance/property the driver should either use devm_clk_get()
> > > (instead of _optional) or skip this block completely.
> >
> > No, the kernel is not a DT validator (and we have the YAML bindings for
> > that now).
> 
> It is not about DT validation. It is about passing a correct DT.

Heh. That's the same thing.

> The file can come up from the kernel. It can come from the older
> kernel.  OR it can come from the vendor. Or it even might be being a
> part of firmware flashed into the device.  So we can not assume that
> the DT is correct just because the in-kernel DT passes YAML
> validation.

Again, no. The kernel does not need to implement DT validation and can
assume that the DT describes the hardware accurately. If the DT says
there's a mux, the driver can use it. If there's no mux in DT, the
driver can assume it isn't there.

The only thing that complicates things here is the sc7280 dts which has
been released in 5.16. We don't care about Qualcomm's kernels and dts.

> So, as I wrote, the whole patchset needs much more care about compatibility.
> 
> > > But this will not work with earlier DTS files.
> >
> > So this is not a problem (but if we really wanted to have the driver
> > validate the DT it can be done by updating the compatible strings).
> 
> We should not update compatible strings just because the driver
> changes. Compat strings describe the hardware, not the Linux point of
> view on it.

We can, it's a documented practise in case a binding needs to be
updated in an incompatible way:

	https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html

But I don't think it'll be needed here.

Johan

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold]

[ Again, please trim unnecessary context from your replies. ]

On Fri, Apr 22, 2022 at 01:35:01PM +0300, Dmitry Baryshkov wrote:
> On Fri, 22 Apr 2022 at 13:20, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Apr 21, 2022 at 02:08:27PM +0300, Dmitry Baryshkov wrote:
> > > On 21/04/2022 13:20, Johan Hovold wrote:

> > > > +   /* Get optional pipe clock mux and default reference source clock. */
> > > > +   qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> > > > +   if (IS_ERR(qphy->pipemux_clk)) {
> > > > +           ret = PTR_ERR(qphy->pipemux_clk);
> > > > +           if (ret == -EPROBE_DEFER)
> > > > +                   return ret;
> > > > +
> > > > +           qphy->pipemux_clk = NULL;

> > > > +   } else {
> > > > +           qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> > > > +           if (IS_ERR(qphy->piperef_clk)) {
> > > > +                   ret = PTR_ERR(qphy->piperef_clk);
> > > > +                   return dev_err_probe(dev, ret,
> > > > +                                        "failed to get lane%d piperef_clk\n",
> > > > +                                        id);
> > > > +           }
> > > > +   }
> > > > +
> > >
> > > As a second thought.
> > > This needs to be more explicit. If the chipset requires the pipe clock
> > > remuxing, we must fail if the clocks were not provided. So depending on
> > > the qmp instance/property the driver should either use devm_clk_get()
> > > (instead of _optional) or skip this block completely.
> >
> > No, the kernel is not a DT validator (and we have the YAML bindings for
> > that now).
> 
> It is not about DT validation. It is about passing a correct DT.

Heh. That's the same thing.

> The file can come up from the kernel. It can come from the older
> kernel.  OR it can come from the vendor. Or it even might be being a
> part of firmware flashed into the device.  So we can not assume that
> the DT is correct just because the in-kernel DT passes YAML
> validation.

Again, no. The kernel does not need to implement DT validation and can
assume that the DT describes the hardware accurately. If the DT says
there's a mux, the driver can use it. If there's no mux in DT, the
driver can assume it isn't there.

The only thing that complicates things here is the sc7280 dts which has
been released in 5.16. We don't care about Qualcomm's kernels and dts.

> So, as I wrote, the whole patchset needs much more care about compatibility.
> 
> > > But this will not work with earlier DTS files.
> >
> > So this is not a problem (but if we really wanted to have the driver
> > validate the DT it can be done by updating the compatible strings).
> 
> We should not update compatible strings just because the driver
> changes. Compat strings describe the hardware, not the Linux point of
> view on it.

We can, it's a documented practise in case a binding needs to be
updated in an incompatible way:

	https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html

But I don't think it'll be needed here.

Johan

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Rob Herring]

On Fri, Apr 22, 2022 at 5:35 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 22 Apr 2022 at 13:20, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Apr 21, 2022 at 02:08:27PM +0300, Dmitry Baryshkov wrote:
> > > On 21/04/2022 13:20, Johan Hovold wrote:
> > > > Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> > > > output generated by the PHY before powering on the PHY and restore the
> > > > default source during power down.
> > > >
> > > > Add support for an optional pipe clock mux which will be reparented to
> > > > the generated pipe clock before powering on the PHY and restored to the
> > > > default reference source on power off.
> > > >
> > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

[...]

> > > > +   } else {
> > > > +           qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> > > > +           if (IS_ERR(qphy->piperef_clk)) {
> > > > +                   ret = PTR_ERR(qphy->piperef_clk);
> > > > +                   return dev_err_probe(dev, ret,
> > > > +                                        "failed to get lane%d piperef_clk\n",
> > > > +                                        id);
> > > > +           }
> > > > +   }
> > > > +
> > >
> > > As a second thought.
> > > This needs to be more explicit. If the chipset requires the pipe clock
> > > remuxing, we must fail if the clocks were not provided. So depending on
> > > the qmp instance/property the driver should either use devm_clk_get()
> > > (instead of _optional) or skip this block completely.
> >
> > No, the kernel is not a DT validator (and we have the YAML bindings for
> > that now).
>
> It is not about DT validation. It is about passing a correct DT. The
> file can come up from the kernel. It can come from the older kernel.
> OR it can come from the vendor. Or it even might be being a part of
> firmware flashed into the device.

As of dtschema 2022.03, validation of dtb's from firmware (or anywhere
else) is supported. Of course, as the old saying goes, if it's not
upstream, it doesn't exist. We can't control what vendors do in their
DTs.

> So we can not assume that the DT is correct just because the in-kernel
> DT passes YAML validation.

I agree with Johan on this. In terms of ensuring correctness, the
kernel does a horrible job. It never will be as long as it is done in
ad hoc code.

> So, as I wrote, the whole patchset needs much more care about compatibility.
>
> > > But this will not work with earlier DTS files.
> >
> > So this is not a problem (but if we really wanted to have the driver
> > validate the DT it can be done by updating the compatible strings).
>
> We should not update compatible strings just because the driver
> changes. Compat strings describe the hardware, not the Linux point of
> view on it.

Yes and no. It is the OS/client view of the h/w. If a binding is
deemed horribly broken we could do a new compatible string and
binding. That's not something we want to be doing frequently though.

Rob

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Rob Herring]

On Fri, Apr 22, 2022 at 5:35 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 22 Apr 2022 at 13:20, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Apr 21, 2022 at 02:08:27PM +0300, Dmitry Baryshkov wrote:
> > > On 21/04/2022 13:20, Johan Hovold wrote:
> > > > Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> > > > output generated by the PHY before powering on the PHY and restore the
> > > > default source during power down.
> > > >
> > > > Add support for an optional pipe clock mux which will be reparented to
> > > > the generated pipe clock before powering on the PHY and restored to the
> > > > default reference source on power off.
> > > >
> > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

[...]

> > > > +   } else {
> > > > +           qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> > > > +           if (IS_ERR(qphy->piperef_clk)) {
> > > > +                   ret = PTR_ERR(qphy->piperef_clk);
> > > > +                   return dev_err_probe(dev, ret,
> > > > +                                        "failed to get lane%d piperef_clk\n",
> > > > +                                        id);
> > > > +           }
> > > > +   }
> > > > +
> > >
> > > As a second thought.
> > > This needs to be more explicit. If the chipset requires the pipe clock
> > > remuxing, we must fail if the clocks were not provided. So depending on
> > > the qmp instance/property the driver should either use devm_clk_get()
> > > (instead of _optional) or skip this block completely.
> >
> > No, the kernel is not a DT validator (and we have the YAML bindings for
> > that now).
>
> It is not about DT validation. It is about passing a correct DT. The
> file can come up from the kernel. It can come from the older kernel.
> OR it can come from the vendor. Or it even might be being a part of
> firmware flashed into the device.

As of dtschema 2022.03, validation of dtb's from firmware (or anywhere
else) is supported. Of course, as the old saying goes, if it's not
upstream, it doesn't exist. We can't control what vendors do in their
DTs.

> So we can not assume that the DT is correct just because the in-kernel
> DT passes YAML validation.

I agree with Johan on this. In terms of ensuring correctness, the
kernel does a horrible job. It never will be as long as it is done in
ad hoc code.

> So, as I wrote, the whole patchset needs much more care about compatibility.
>
> > > But this will not work with earlier DTS files.
> >
> > So this is not a problem (but if we really wanted to have the driver
> > validate the DT it can be done by updating the compatible strings).
>
> We should not update compatible strings just because the driver
> changes. Compat strings describe the hardware, not the Linux point of
> view on it.

Yes and no. It is the OS/client view of the h/w. If a binding is
deemed horribly broken we could do a new compatible string and
binding. That's not something we want to be doing frequently though.

Rob

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Dmitry Baryshkov]

On 21/04/2022 13:20, Johan Hovold wrote:
> Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> output generated by the PHY before powering on the PHY and restore the
> default source during power down.
> 
> Add support for an optional pipe clock mux which will be reparented to
> the generated pipe clock before powering on the PHY and restored to the
> default reference source on power off.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
>   1 file changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 7d2d1ab061f7..bc6db9670291 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
>    * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
>    * @pcs_misc: iomapped memory space for lane's pcs_misc
>    * @pipe_clk: pipe clock
> + * @pipemux_clk: pipe clock source mux
> + * @piperef_clk: pipe clock default reference source
>    * @index: lane index
>    * @qmp: QMP phy to which this lane belongs
>    * @lane_rst: lane's reset controller
> @@ -3311,6 +3313,8 @@ struct qmp_phy {
>   	void __iomem *rx2;
>   	void __iomem *pcs_misc;
>   	struct clk *pipe_clk;
> +	struct clk *pipemux_clk;
> +	struct clk *piperef_clk;
>   	unsigned int index;
>   	struct qcom_qmp *qmp;
>   	struct reset_control *lane_rst;
> @@ -3346,6 +3350,7 @@ struct qcom_qmp {
>   	void __iomem *dp_com;
>   
>   	struct clk_bulk_data *clks;
> +	struct clk *pipe_clksrc;
>   	struct reset_control **resets;
>   	struct regulator_bulk_data *vregs;
>   
> @@ -5355,6 +5360,42 @@ static int qcom_qmp_phy_init(struct phy *phy)
>   	return 0;
>   }
>   
> +static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy)
> +{
> +	struct qcom_qmp *qmp = qphy->qmp;
> +	int ret;
> +
> +	ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc);
> +	if (ret)
> +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +
> +
> +	ret = clk_prepare_enable(qphy->pipe_clk);
> +	if (ret) {
> +		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> +		goto err_restore_parent;
> +	}

So, what you do here is you manually set the parent of 
GCC_PCIE_1_PIPE_CLK_SRC to PHY pipe clock right before enabling 
GCC_PCIE_1_PIPE_CLK and set it back to XO after disabling 
GCC_PCIE_1_PIPE_CLK.

My proposal is doing exactly the same, but doing that automatically 
through the clock infrastructure. After removing pipe_clock handling 
from pcie driver itself, we can be sure that nobody is playing dirty 
tricks around the pipe_clock.

> +
> +	return 0;
> +
> +err_restore_parent:
> +	clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> +
> +	return ret;
> +}
> +
> +static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy)
> +{
> +	struct qcom_qmp *qmp = qphy->qmp;
> +	int ret;
> +
> +	clk_disable_unprepare(qphy->pipe_clk);
> +
> +	ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> +	if (ret)
> +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +}
> +
>   static int qcom_qmp_phy_power_on(struct phy *phy)
>   {
>   	struct qmp_phy *qphy = phy_get_drvdata(phy);
> @@ -5379,11 +5420,9 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>   		}
>   	}
>   
> -	ret = clk_prepare_enable(qphy->pipe_clk);
> -	if (ret) {
> -		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> +	ret = qcom_qmp_phy_pipe_clk_enable(qphy);
> +	if (ret)
>   		goto err_reset_lane;
> -	}
>   
>   	/* Tx, Rx, and PCS configurations */
>   	qcom_qmp_phy_configure_lane(tx, cfg->regs,
> @@ -5478,7 +5517,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>   	return 0;
>   
>   err_disable_pipe_clk:
> -	clk_disable_unprepare(qphy->pipe_clk);
> +	qcom_qmp_phy_pipe_clk_disable(qphy);
>   err_reset_lane:
>   	if (cfg->has_lane_rst)
>   		reset_control_assert(qphy->lane_rst);
> @@ -5491,7 +5530,7 @@ static int qcom_qmp_phy_power_off(struct phy *phy)
>   	struct qmp_phy *qphy = phy_get_drvdata(phy);
>   	const struct qmp_phy_cfg *cfg = qphy->cfg;
>   
> -	clk_disable_unprepare(qphy->pipe_clk);
> +	qcom_qmp_phy_pipe_clk_disable(qphy);
>   
>   	if (cfg->type == PHY_TYPE_DP) {
>   		/* Assert DP PHY power down */
> @@ -5777,6 +5816,8 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
>   	if (ret)
>   		return ret;
>   
> +	qmp->pipe_clksrc = fixed->hw.clk;
> +
>   	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
>   	if (ret)
>   		return ret;
> @@ -6091,6 +6132,24 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id,
>   		qphy->pipe_clk = NULL;
>   	}
>   
> +	/* Get optional pipe clock mux and default reference source clock. */
> +	qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> +	if (IS_ERR(qphy->pipemux_clk)) {
> +		ret = PTR_ERR(qphy->pipemux_clk);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		qphy->pipemux_clk = NULL;
> +	} else {
> +		qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> +		if (IS_ERR(qphy->piperef_clk)) {
> +			ret = PTR_ERR(qphy->piperef_clk);
> +			return dev_err_probe(dev, ret,
> +					     "failed to get lane%d piperef_clk\n",
> +					     id);
> +		}
> +	}
> +
>   	/* Get lane reset, if any */
>   	if (cfg->has_lane_rst) {
>   		snprintf(prop_name, sizeof(prop_name), "lane%d", id);


-- 
With best wishes
Dmitry

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Dmitry Baryshkov]

On 21/04/2022 13:20, Johan Hovold wrote:
> Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> output generated by the PHY before powering on the PHY and restore the
> default source during power down.
> 
> Add support for an optional pipe clock mux which will be reparented to
> the generated pipe clock before powering on the PHY and restored to the
> default reference source on power off.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
>   1 file changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 7d2d1ab061f7..bc6db9670291 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
>    * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
>    * @pcs_misc: iomapped memory space for lane's pcs_misc
>    * @pipe_clk: pipe clock
> + * @pipemux_clk: pipe clock source mux
> + * @piperef_clk: pipe clock default reference source
>    * @index: lane index
>    * @qmp: QMP phy to which this lane belongs
>    * @lane_rst: lane's reset controller
> @@ -3311,6 +3313,8 @@ struct qmp_phy {
>   	void __iomem *rx2;
>   	void __iomem *pcs_misc;
>   	struct clk *pipe_clk;
> +	struct clk *pipemux_clk;
> +	struct clk *piperef_clk;
>   	unsigned int index;
>   	struct qcom_qmp *qmp;
>   	struct reset_control *lane_rst;
> @@ -3346,6 +3350,7 @@ struct qcom_qmp {
>   	void __iomem *dp_com;
>   
>   	struct clk_bulk_data *clks;
> +	struct clk *pipe_clksrc;
>   	struct reset_control **resets;
>   	struct regulator_bulk_data *vregs;
>   
> @@ -5355,6 +5360,42 @@ static int qcom_qmp_phy_init(struct phy *phy)
>   	return 0;
>   }
>   
> +static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy)
> +{
> +	struct qcom_qmp *qmp = qphy->qmp;
> +	int ret;
> +
> +	ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc);
> +	if (ret)
> +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +
> +
> +	ret = clk_prepare_enable(qphy->pipe_clk);
> +	if (ret) {
> +		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> +		goto err_restore_parent;
> +	}

So, what you do here is you manually set the parent of 
GCC_PCIE_1_PIPE_CLK_SRC to PHY pipe clock right before enabling 
GCC_PCIE_1_PIPE_CLK and set it back to XO after disabling 
GCC_PCIE_1_PIPE_CLK.

My proposal is doing exactly the same, but doing that automatically 
through the clock infrastructure. After removing pipe_clock handling 
from pcie driver itself, we can be sure that nobody is playing dirty 
tricks around the pipe_clock.

> +
> +	return 0;
> +
> +err_restore_parent:
> +	clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> +
> +	return ret;
> +}
> +
> +static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy)
> +{
> +	struct qcom_qmp *qmp = qphy->qmp;
> +	int ret;
> +
> +	clk_disable_unprepare(qphy->pipe_clk);
> +
> +	ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> +	if (ret)
> +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +}
> +
>   static int qcom_qmp_phy_power_on(struct phy *phy)
>   {
>   	struct qmp_phy *qphy = phy_get_drvdata(phy);
> @@ -5379,11 +5420,9 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>   		}
>   	}
>   
> -	ret = clk_prepare_enable(qphy->pipe_clk);
> -	if (ret) {
> -		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> +	ret = qcom_qmp_phy_pipe_clk_enable(qphy);
> +	if (ret)
>   		goto err_reset_lane;
> -	}
>   
>   	/* Tx, Rx, and PCS configurations */
>   	qcom_qmp_phy_configure_lane(tx, cfg->regs,
> @@ -5478,7 +5517,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>   	return 0;
>   
>   err_disable_pipe_clk:
> -	clk_disable_unprepare(qphy->pipe_clk);
> +	qcom_qmp_phy_pipe_clk_disable(qphy);
>   err_reset_lane:
>   	if (cfg->has_lane_rst)
>   		reset_control_assert(qphy->lane_rst);
> @@ -5491,7 +5530,7 @@ static int qcom_qmp_phy_power_off(struct phy *phy)
>   	struct qmp_phy *qphy = phy_get_drvdata(phy);
>   	const struct qmp_phy_cfg *cfg = qphy->cfg;
>   
> -	clk_disable_unprepare(qphy->pipe_clk);
> +	qcom_qmp_phy_pipe_clk_disable(qphy);
>   
>   	if (cfg->type == PHY_TYPE_DP) {
>   		/* Assert DP PHY power down */
> @@ -5777,6 +5816,8 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
>   	if (ret)
>   		return ret;
>   
> +	qmp->pipe_clksrc = fixed->hw.clk;
> +
>   	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
>   	if (ret)
>   		return ret;
> @@ -6091,6 +6132,24 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id,
>   		qphy->pipe_clk = NULL;
>   	}
>   
> +	/* Get optional pipe clock mux and default reference source clock. */
> +	qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> +	if (IS_ERR(qphy->pipemux_clk)) {
> +		ret = PTR_ERR(qphy->pipemux_clk);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		qphy->pipemux_clk = NULL;
> +	} else {
> +		qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> +		if (IS_ERR(qphy->piperef_clk)) {
> +			ret = PTR_ERR(qphy->piperef_clk);
> +			return dev_err_probe(dev, ret,
> +					     "failed to get lane%d piperef_clk\n",
> +					     id);
> +		}
> +	}
> +
>   	/* Get lane reset, if any */
>   	if (cfg->has_lane_rst) {
>   		snprintf(prop_name, sizeof(prop_name), "lane%d", id);


-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold]

On Thu, Apr 21, 2022 at 02:36:05PM +0300, Dmitry Baryshkov wrote:
> On 21/04/2022 13:20, Johan Hovold wrote:
> > Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> > output generated by the PHY before powering on the PHY and restore the
> > default source during power down.
> > 
> > Add support for an optional pipe clock mux which will be reparented to
> > the generated pipe clock before powering on the PHY and restored to the
> > default reference source on power off.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
 
> > +static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy)
> > +{
> > +	struct qcom_qmp *qmp = qphy->qmp;
> > +	int ret;
> > +
> > +	ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc);
> > +	if (ret)
> > +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> > +
> > +
> > +	ret = clk_prepare_enable(qphy->pipe_clk);
> > +	if (ret) {
> > +		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> > +		goto err_restore_parent;
> > +	}
> 
> So, what you do here is you manually set the parent of 
> GCC_PCIE_1_PIPE_CLK_SRC to PHY pipe clock right before enabling 
> GCC_PCIE_1_PIPE_CLK and set it back to XO after disabling 
> GCC_PCIE_1_PIPE_CLK.
> 
> My proposal is doing exactly the same, but doing that automatically 
> through the clock infrastructure. After removing pipe_clock handling 
> from pcie driver itself, we can be sure that nobody is playing dirty 
> tricks around the pipe_clock.

Yes, the end result is similar, but I believe handling it explicitly in
the driver is preferred for a number of reasons that I've already
mentioned. Not least because the mux needs to be updated when the PHY is
powered on, not when the GCC pipe clock is ungated.

In practise, powering on the PHY and ungating the clock happen to
coincide in time because only the PHY driver will use the GCC pipe
clock, but conceptually they are unrelated (and as the GDSC hang shows,
something in the system appears to be ungating the clock while the PHY
is powered off).

The QMP PHY driver implementation is much more straight forward and
easier to reason about than having the mux implementation spread out
over multiple clock drivers where it's not clear at all what is really
going on or why (and even debugfs will give you a false view of the
clock tree state).

> > +
> > +	return 0;
> > +
> > +err_restore_parent:
> > +	clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> > +
> > +	return ret;
> > +}
> > +
> > +static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy)
> > +{
> > +	struct qcom_qmp *qmp = qphy->qmp;
> > +	int ret;
> > +
> > +	clk_disable_unprepare(qphy->pipe_clk);
> > +
> > +	ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> > +	if (ret)
> > +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> > +}
> > +

Johan

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold]

On Thu, Apr 21, 2022 at 02:36:05PM +0300, Dmitry Baryshkov wrote:
> On 21/04/2022 13:20, Johan Hovold wrote:
> > Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> > output generated by the PHY before powering on the PHY and restore the
> > default source during power down.
> > 
> > Add support for an optional pipe clock mux which will be reparented to
> > the generated pipe clock before powering on the PHY and restored to the
> > default reference source on power off.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
 
> > +static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy)
> > +{
> > +	struct qcom_qmp *qmp = qphy->qmp;
> > +	int ret;
> > +
> > +	ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc);
> > +	if (ret)
> > +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> > +
> > +
> > +	ret = clk_prepare_enable(qphy->pipe_clk);
> > +	if (ret) {
> > +		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> > +		goto err_restore_parent;
> > +	}
> 
> So, what you do here is you manually set the parent of 
> GCC_PCIE_1_PIPE_CLK_SRC to PHY pipe clock right before enabling 
> GCC_PCIE_1_PIPE_CLK and set it back to XO after disabling 
> GCC_PCIE_1_PIPE_CLK.
> 
> My proposal is doing exactly the same, but doing that automatically 
> through the clock infrastructure. After removing pipe_clock handling 
> from pcie driver itself, we can be sure that nobody is playing dirty 
> tricks around the pipe_clock.

Yes, the end result is similar, but I believe handling it explicitly in
the driver is preferred for a number of reasons that I've already
mentioned. Not least because the mux needs to be updated when the PHY is
powered on, not when the GCC pipe clock is ungated.

In practise, powering on the PHY and ungating the clock happen to
coincide in time because only the PHY driver will use the GCC pipe
clock, but conceptually they are unrelated (and as the GDSC hang shows,
something in the system appears to be ungating the clock while the PHY
is powered off).

The QMP PHY driver implementation is much more straight forward and
easier to reason about than having the mux implementation spread out
over multiple clock drivers where it's not clear at all what is really
going on or why (and even debugfs will give you a false view of the
clock tree state).

> > +
> > +	return 0;
> > +
> > +err_restore_parent:
> > +	clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> > +
> > +	return ret;
> > +}
> > +
> > +static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy)
> > +{
> > +	struct qcom_qmp *qmp = qphy->qmp;
> > +	int ret;
> > +
> > +	clk_disable_unprepare(qphy->pipe_clk);
> > +
> > +	ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> > +	if (ret)
> > +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> > +}
> > +

Johan

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Bjorn Andersson]

On Thu 21 Apr 03:20 PDT 2022, Johan Hovold wrote:

> Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> output generated by the PHY before powering on the PHY and restore the
> default source during power down.
> 
> Add support for an optional pipe clock mux which will be reparented to
> the generated pipe clock before powering on the PHY and restored to the
> default reference source on power off.
> 

After considering this for a while, I have two objections to doing this
explicitly:

1) The QMP block is fed &gcc_pcie_N_pipe_clk (and on sc8280xp)
gcc_pcie_N_pipediv2_clk. But neither of these clocks are the mux, so
what this patch (and the existing muxing logic in the controller) does
is to poke into gcc "internals".

2) The actual reason for the mux dance is that toggling the associated
GDSC without a valid parent of this clock would cause the clock to lock
up and GDSC transition to time out. This property is shared with a wide
range of other clocks (so far we have 84 users of clk_rcg2_shared_ops on
sc8280xp).



It would be nice if clk_summary would represent the real state of these
clocks, but unfortunately I don't think the state matches reality with
this approach either.

E.g. we prepare/enable the pipe clock before setting
QPHY_POWER_DOWN_CONTROL, during this time there's shouldn't be any pipe
clock coming out of the PHY...

Regards,
Bjorn

> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
>  1 file changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 7d2d1ab061f7..bc6db9670291 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
>   * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
>   * @pcs_misc: iomapped memory space for lane's pcs_misc
>   * @pipe_clk: pipe clock
> + * @pipemux_clk: pipe clock source mux
> + * @piperef_clk: pipe clock default reference source
>   * @index: lane index
>   * @qmp: QMP phy to which this lane belongs
>   * @lane_rst: lane's reset controller
> @@ -3311,6 +3313,8 @@ struct qmp_phy {
>  	void __iomem *rx2;
>  	void __iomem *pcs_misc;
>  	struct clk *pipe_clk;
> +	struct clk *pipemux_clk;
> +	struct clk *piperef_clk;
>  	unsigned int index;
>  	struct qcom_qmp *qmp;
>  	struct reset_control *lane_rst;
> @@ -3346,6 +3350,7 @@ struct qcom_qmp {
>  	void __iomem *dp_com;
>  
>  	struct clk_bulk_data *clks;
> +	struct clk *pipe_clksrc;
>  	struct reset_control **resets;
>  	struct regulator_bulk_data *vregs;
>  
> @@ -5355,6 +5360,42 @@ static int qcom_qmp_phy_init(struct phy *phy)
>  	return 0;
>  }
>  
> +static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy)
> +{
> +	struct qcom_qmp *qmp = qphy->qmp;
> +	int ret;
> +
> +	ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc);
> +	if (ret)
> +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +
> +
> +	ret = clk_prepare_enable(qphy->pipe_clk);
> +	if (ret) {
> +		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> +		goto err_restore_parent;
> +	}
> +
> +	return 0;
> +
> +err_restore_parent:
> +	clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> +
> +	return ret;
> +}
> +
> +static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy)
> +{
> +	struct qcom_qmp *qmp = qphy->qmp;
> +	int ret;
> +
> +	clk_disable_unprepare(qphy->pipe_clk);
> +
> +	ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> +	if (ret)
> +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +}
> +
>  static int qcom_qmp_phy_power_on(struct phy *phy)
>  {
>  	struct qmp_phy *qphy = phy_get_drvdata(phy);
> @@ -5379,11 +5420,9 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>  		}
>  	}
>  
> -	ret = clk_prepare_enable(qphy->pipe_clk);
> -	if (ret) {
> -		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> +	ret = qcom_qmp_phy_pipe_clk_enable(qphy);
> +	if (ret)
>  		goto err_reset_lane;
> -	}
>  
>  	/* Tx, Rx, and PCS configurations */
>  	qcom_qmp_phy_configure_lane(tx, cfg->regs,
> @@ -5478,7 +5517,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>  	return 0;
>  
>  err_disable_pipe_clk:
> -	clk_disable_unprepare(qphy->pipe_clk);
> +	qcom_qmp_phy_pipe_clk_disable(qphy);
>  err_reset_lane:
>  	if (cfg->has_lane_rst)
>  		reset_control_assert(qphy->lane_rst);
> @@ -5491,7 +5530,7 @@ static int qcom_qmp_phy_power_off(struct phy *phy)
>  	struct qmp_phy *qphy = phy_get_drvdata(phy);
>  	const struct qmp_phy_cfg *cfg = qphy->cfg;
>  
> -	clk_disable_unprepare(qphy->pipe_clk);
> +	qcom_qmp_phy_pipe_clk_disable(qphy);
>  
>  	if (cfg->type == PHY_TYPE_DP) {
>  		/* Assert DP PHY power down */
> @@ -5777,6 +5816,8 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
>  	if (ret)
>  		return ret;
>  
> +	qmp->pipe_clksrc = fixed->hw.clk;
> +
>  	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
>  	if (ret)
>  		return ret;
> @@ -6091,6 +6132,24 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id,
>  		qphy->pipe_clk = NULL;
>  	}
>  
> +	/* Get optional pipe clock mux and default reference source clock. */
> +	qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> +	if (IS_ERR(qphy->pipemux_clk)) {
> +		ret = PTR_ERR(qphy->pipemux_clk);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		qphy->pipemux_clk = NULL;
> +	} else {
> +		qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> +		if (IS_ERR(qphy->piperef_clk)) {
> +			ret = PTR_ERR(qphy->piperef_clk);
> +			return dev_err_probe(dev, ret,
> +					     "failed to get lane%d piperef_clk\n",
> +					     id);
> +		}
> +	}
> +
>  	/* Get lane reset, if any */
>  	if (cfg->has_lane_rst) {
>  		snprintf(prop_name, sizeof(prop_name), "lane%d", id);
> -- 
> 2.35.1
> 

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Bjorn Andersson]

On Thu 21 Apr 03:20 PDT 2022, Johan Hovold wrote:

> Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> output generated by the PHY before powering on the PHY and restore the
> default source during power down.
> 
> Add support for an optional pipe clock mux which will be reparented to
> the generated pipe clock before powering on the PHY and restored to the
> default reference source on power off.
> 

After considering this for a while, I have two objections to doing this
explicitly:

1) The QMP block is fed &gcc_pcie_N_pipe_clk (and on sc8280xp)
gcc_pcie_N_pipediv2_clk. But neither of these clocks are the mux, so
what this patch (and the existing muxing logic in the controller) does
is to poke into gcc "internals".

2) The actual reason for the mux dance is that toggling the associated
GDSC without a valid parent of this clock would cause the clock to lock
up and GDSC transition to time out. This property is shared with a wide
range of other clocks (so far we have 84 users of clk_rcg2_shared_ops on
sc8280xp).



It would be nice if clk_summary would represent the real state of these
clocks, but unfortunately I don't think the state matches reality with
this approach either.

E.g. we prepare/enable the pipe clock before setting
QPHY_POWER_DOWN_CONTROL, during this time there's shouldn't be any pipe
clock coming out of the PHY...

Regards,
Bjorn

> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
>  1 file changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 7d2d1ab061f7..bc6db9670291 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
>   * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
>   * @pcs_misc: iomapped memory space for lane's pcs_misc
>   * @pipe_clk: pipe clock
> + * @pipemux_clk: pipe clock source mux
> + * @piperef_clk: pipe clock default reference source
>   * @index: lane index
>   * @qmp: QMP phy to which this lane belongs
>   * @lane_rst: lane's reset controller
> @@ -3311,6 +3313,8 @@ struct qmp_phy {
>  	void __iomem *rx2;
>  	void __iomem *pcs_misc;
>  	struct clk *pipe_clk;
> +	struct clk *pipemux_clk;
> +	struct clk *piperef_clk;
>  	unsigned int index;
>  	struct qcom_qmp *qmp;
>  	struct reset_control *lane_rst;
> @@ -3346,6 +3350,7 @@ struct qcom_qmp {
>  	void __iomem *dp_com;
>  
>  	struct clk_bulk_data *clks;
> +	struct clk *pipe_clksrc;
>  	struct reset_control **resets;
>  	struct regulator_bulk_data *vregs;
>  
> @@ -5355,6 +5360,42 @@ static int qcom_qmp_phy_init(struct phy *phy)
>  	return 0;
>  }
>  
> +static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy)
> +{
> +	struct qcom_qmp *qmp = qphy->qmp;
> +	int ret;
> +
> +	ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc);
> +	if (ret)
> +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +
> +
> +	ret = clk_prepare_enable(qphy->pipe_clk);
> +	if (ret) {
> +		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> +		goto err_restore_parent;
> +	}
> +
> +	return 0;
> +
> +err_restore_parent:
> +	clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> +
> +	return ret;
> +}
> +
> +static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy)
> +{
> +	struct qcom_qmp *qmp = qphy->qmp;
> +	int ret;
> +
> +	clk_disable_unprepare(qphy->pipe_clk);
> +
> +	ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> +	if (ret)
> +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +}
> +
>  static int qcom_qmp_phy_power_on(struct phy *phy)
>  {
>  	struct qmp_phy *qphy = phy_get_drvdata(phy);
> @@ -5379,11 +5420,9 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>  		}
>  	}
>  
> -	ret = clk_prepare_enable(qphy->pipe_clk);
> -	if (ret) {
> -		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> +	ret = qcom_qmp_phy_pipe_clk_enable(qphy);
> +	if (ret)
>  		goto err_reset_lane;
> -	}
>  
>  	/* Tx, Rx, and PCS configurations */
>  	qcom_qmp_phy_configure_lane(tx, cfg->regs,
> @@ -5478,7 +5517,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
>  	return 0;
>  
>  err_disable_pipe_clk:
> -	clk_disable_unprepare(qphy->pipe_clk);
> +	qcom_qmp_phy_pipe_clk_disable(qphy);
>  err_reset_lane:
>  	if (cfg->has_lane_rst)
>  		reset_control_assert(qphy->lane_rst);
> @@ -5491,7 +5530,7 @@ static int qcom_qmp_phy_power_off(struct phy *phy)
>  	struct qmp_phy *qphy = phy_get_drvdata(phy);
>  	const struct qmp_phy_cfg *cfg = qphy->cfg;
>  
> -	clk_disable_unprepare(qphy->pipe_clk);
> +	qcom_qmp_phy_pipe_clk_disable(qphy);
>  
>  	if (cfg->type == PHY_TYPE_DP) {
>  		/* Assert DP PHY power down */
> @@ -5777,6 +5816,8 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
>  	if (ret)
>  		return ret;
>  
> +	qmp->pipe_clksrc = fixed->hw.clk;
> +
>  	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
>  	if (ret)
>  		return ret;
> @@ -6091,6 +6132,24 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id,
>  		qphy->pipe_clk = NULL;
>  	}
>  
> +	/* Get optional pipe clock mux and default reference source clock. */
> +	qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> +	if (IS_ERR(qphy->pipemux_clk)) {
> +		ret = PTR_ERR(qphy->pipemux_clk);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		qphy->pipemux_clk = NULL;
> +	} else {
> +		qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> +		if (IS_ERR(qphy->piperef_clk)) {
> +			ret = PTR_ERR(qphy->piperef_clk);
> +			return dev_err_probe(dev, ret,
> +					     "failed to get lane%d piperef_clk\n",
> +					     id);
> +		}
> +	}
> +
>  	/* Get lane reset, if any */
>  	if (cfg->has_lane_rst) {
>  		snprintf(prop_name, sizeof(prop_name), "lane%d", id);
> -- 
> 2.35.1
> 

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold]

On Thu, Apr 28, 2022 at 06:11:44AM -0700, Bjorn Andersson wrote:
> On Thu 21 Apr 03:20 PDT 2022, Johan Hovold wrote:
> 
> > Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> > output generated by the PHY before powering on the PHY and restore the
> > default source during power down.
> > 
> > Add support for an optional pipe clock mux which will be reparented to
> > the generated pipe clock before powering on the PHY and restored to the
> > default reference source on power off.
> > 
> 
> After considering this for a while, I have two objections to doing this
> explicitly:
> 
> 1) The QMP block is fed &gcc_pcie_N_pipe_clk (and on sc8280xp)
> gcc_pcie_N_pipediv2_clk. But neither of these clocks are the mux, so
> what this patch (and the existing muxing logic in the controller) does
> is to poke into gcc "internals".

I agree that this is perhaps the strongest argument against describing
the mux in DT (as is also currently done for sc7280).

> 2) The actual reason for the mux dance is that toggling the associated
> GDSC without a valid parent of this clock would cause the clock to lock
> up and GDSC transition to time out. This property is shared with a wide
> range of other clocks (so far we have 84 users of clk_rcg2_shared_ops on
> sc8280xp).

Right, but the situation with rcg2 is a little different. From what I
gather the problem there is that some downstream clock could have been
enabled by some other part of the system behind the kernel's back and
then things go wrong when the kernel configures the clock.

Here it is the kernel that controls the source of the pipe clock mux
(the PHY PLL) and knows when the source is valid (and the PHY is both
provider and consumer of the pipe clock).

For rcg2, there's no choice but to work around the hardware in the clock
driver, while for QMP the PHY power sequences could be made explicit in
the driver:

	clk_prepare_enable(pipe);
	clk_set_parent(pipe_src, pll);
	start_pll();
	
	stop_pll();
	clk_set_parent(pipe_src, xo);
	clk_disable_unprepare(pipe);

Note that with the above sequences it would be possible to drop the pipe
clock BRANCH_HALT_SKIP flag, which is only there in the clock driver
because of the how the PHY works (i.e. that the pipe clock must be
ungated before the PLL is started).

(This wouldn't be possible with a pipe-mux implementation in the clock
driver since the parent mux would be enabled before the child pipe
clock.)

But sure, the requirement to restore XO to prevent a later GDSC hang has
little to do with the PHY.

> It would be nice if clk_summary would represent the real state of these
> clocks, but unfortunately I don't think the state matches reality with
> this approach either.
> 
> E.g. we prepare/enable the pipe clock before setting
> QPHY_POWER_DOWN_CONTROL, during this time there's shouldn't be any pipe
> clock coming out of the PHY...

Right, there's a small window there where the source is still off (due
to hardware requirements), but at least the topology is always reported
correctly, which is really useful when dealing with boot handover.

I have an sc8280xp here where the boot firmware leaves the pipe clock
muxed in despite the GDSC being disabled. Fortunately, it doesn't seem
to trigger the lockup when toggling the GDSC as it does on an sa8540p.

My concerns are otherwise mostly related to the implementation of the
safe-mux (e.g. ad-hoc, missing locking) and can probably be addressed.

As an example, with the current implementation it is not possible to use
an assigned clock parent in DT to make sure that the XO source is
selected before toggling the GDSC. The muxing doesn't happen until the
pipe clock is enabled, which is much too late in case the boot firmware
left the pipe clock muxed in, and when enabling the pipe clock you
really want the PHY PLL as source and not the cached XO.

Can you still drop the current safe-mux patches from your tree or would
they need to be fixed up incrementally? I think you merged v2, but
there's already a v3 out (addressing the hardcoded mux configuration
values).

Johan

Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold]

On Thu, Apr 28, 2022 at 06:11:44AM -0700, Bjorn Andersson wrote:
> On Thu 21 Apr 03:20 PDT 2022, Johan Hovold wrote:
> 
> > Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> > output generated by the PHY before powering on the PHY and restore the
> > default source during power down.
> > 
> > Add support for an optional pipe clock mux which will be reparented to
> > the generated pipe clock before powering on the PHY and restored to the
> > default reference source on power off.
> > 
> 
> After considering this for a while, I have two objections to doing this
> explicitly:
> 
> 1) The QMP block is fed &gcc_pcie_N_pipe_clk (and on sc8280xp)
> gcc_pcie_N_pipediv2_clk. But neither of these clocks are the mux, so
> what this patch (and the existing muxing logic in the controller) does
> is to poke into gcc "internals".

I agree that this is perhaps the strongest argument against describing
the mux in DT (as is also currently done for sc7280).

> 2) The actual reason for the mux dance is that toggling the associated
> GDSC without a valid parent of this clock would cause the clock to lock
> up and GDSC transition to time out. This property is shared with a wide
> range of other clocks (so far we have 84 users of clk_rcg2_shared_ops on
> sc8280xp).

Right, but the situation with rcg2 is a little different. From what I
gather the problem there is that some downstream clock could have been
enabled by some other part of the system behind the kernel's back and
then things go wrong when the kernel configures the clock.

Here it is the kernel that controls the source of the pipe clock mux
(the PHY PLL) and knows when the source is valid (and the PHY is both
provider and consumer of the pipe clock).

For rcg2, there's no choice but to work around the hardware in the clock
driver, while for QMP the PHY power sequences could be made explicit in
the driver:

	clk_prepare_enable(pipe);
	clk_set_parent(pipe_src, pll);
	start_pll();
	
	stop_pll();
	clk_set_parent(pipe_src, xo);
	clk_disable_unprepare(pipe);

Note that with the above sequences it would be possible to drop the pipe
clock BRANCH_HALT_SKIP flag, which is only there in the clock driver
because of the how the PHY works (i.e. that the pipe clock must be
ungated before the PLL is started).

(This wouldn't be possible with a pipe-mux implementation in the clock
driver since the parent mux would be enabled before the child pipe
clock.)

But sure, the requirement to restore XO to prevent a later GDSC hang has
little to do with the PHY.

> It would be nice if clk_summary would represent the real state of these
> clocks, but unfortunately I don't think the state matches reality with
> this approach either.
> 
> E.g. we prepare/enable the pipe clock before setting
> QPHY_POWER_DOWN_CONTROL, during this time there's shouldn't be any pipe
> clock coming out of the PHY...

Right, there's a small window there where the source is still off (due
to hardware requirements), but at least the topology is always reported
correctly, which is really useful when dealing with boot handover.

I have an sc8280xp here where the boot firmware leaves the pipe clock
muxed in despite the GDSC being disabled. Fortunately, it doesn't seem
to trigger the lockup when toggling the GDSC as it does on an sa8540p.

My concerns are otherwise mostly related to the implementation of the
safe-mux (e.g. ad-hoc, missing locking) and can probably be addressed.

As an example, with the current implementation it is not possible to use
an assigned clock parent in DT to make sure that the XO source is
selected before toggling the GDSC. The muxing doesn't happen until the
pipe clock is enabled, which is much too late in case the boot firmware
left the pipe clock muxed in, and when enabling the pipe clock you
really want the PHY PLL as source and not the cached XO.

Can you still drop the current safe-mux patches from your tree or would
they need to be fixed up incrementally? I think you merged v2, but
there's already a v3 out (addressing the hardcoded mux configuration
values).

Johan

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH RFC 2/5] arm64: dts: qcom: sc7280: move pipe mux handling to phy

[Johan Hovold]

The QMP PHY pipe clock remuxing is part of the PHY, which is both the
producer and the consumer of the pipe clock.

Update the PCIe controller and PHY node to reflect the new binding.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index c07765df9303..b3a9630262dc 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -1837,11 +1837,7 @@ pcie1: pci@1c08000 {
 					<0 0 0 3 &intc 0 0 0 438 IRQ_TYPE_LEVEL_HIGH>,
 					<0 0 0 4 &intc 0 0 0 439 IRQ_TYPE_LEVEL_HIGH>;
 
-			clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
-				 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
-				 <&pcie1_lane 0>,
-				 <&rpmhcc RPMH_CXO_CLK>,
-				 <&gcc GCC_PCIE_1_AUX_CLK>,
+			clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
 				 <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
 				 <&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
 				 <&gcc GCC_PCIE_1_SLV_AXI_CLK>,
@@ -1849,11 +1845,7 @@ pcie1: pci@1c08000 {
 				 <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,
 				 <&gcc GCC_DDRSS_PCIE_SF_CLK>;
 
-			clock-names = "pipe",
-				      "pipe_mux",
-				      "phy_pipe",
-				      "ref",
-				      "aux",
+			clock-names = "aux",
 				      "cfg",
 				      "bus_master",
 				      "bus_slave",
@@ -1910,8 +1902,10 @@ pcie1_lane: lanes@1c0e200 {
 				      <0 0x01c0e600 0 0x170>,
 				      <0 0x01c0e800 0 0x200>,
 				      <0 0x01c0ee00 0 0xf4>;
-				clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
-				clock-names = "pipe0";
+				clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
+					 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
+					 <&rpmhcc RPMH_CXO_CLK>;
+				clock-names = "pipe0", "mux", "ref";
 
 				#phy-cells = <0>;
 				#clock-cells = <1>;
-- 
2.35.1

[PATCH RFC 2/5] arm64: dts: qcom: sc7280: move pipe mux handling to phy

[Johan Hovold]

The QMP PHY pipe clock remuxing is part of the PHY, which is both the
producer and the consumer of the pipe clock.

Update the PCIe controller and PHY node to reflect the new binding.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index c07765df9303..b3a9630262dc 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -1837,11 +1837,7 @@ pcie1: pci@1c08000 {
 					<0 0 0 3 &intc 0 0 0 438 IRQ_TYPE_LEVEL_HIGH>,
 					<0 0 0 4 &intc 0 0 0 439 IRQ_TYPE_LEVEL_HIGH>;
 
-			clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
-				 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
-				 <&pcie1_lane 0>,
-				 <&rpmhcc RPMH_CXO_CLK>,
-				 <&gcc GCC_PCIE_1_AUX_CLK>,
+			clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
 				 <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
 				 <&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
 				 <&gcc GCC_PCIE_1_SLV_AXI_CLK>,
@@ -1849,11 +1845,7 @@ pcie1: pci@1c08000 {
 				 <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,
 				 <&gcc GCC_DDRSS_PCIE_SF_CLK>;
 
-			clock-names = "pipe",
-				      "pipe_mux",
-				      "phy_pipe",
-				      "ref",
-				      "aux",
+			clock-names = "aux",
 				      "cfg",
 				      "bus_master",
 				      "bus_slave",
@@ -1910,8 +1902,10 @@ pcie1_lane: lanes@1c0e200 {
 				      <0 0x01c0e600 0 0x170>,
 				      <0 0x01c0e800 0 0x200>,
 				      <0 0x01c0ee00 0 0xf4>;
-				clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
-				clock-names = "pipe0";
+				clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
+					 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
+					 <&rpmhcc RPMH_CXO_CLK>;
+				clock-names = "pipe0", "mux", "ref";
 
 				#phy-cells = <0>;
 				#clock-cells = <1>;
-- 
2.35.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 2/5] arm64: dts: qcom: sc7280: move pipe mux handling to phy

[Dmitry Baryshkov]

On 21/04/2022 13:20, Johan Hovold wrote:
> The QMP PHY pipe clock remuxing is part of the PHY, which is both the
> producer and the consumer of the pipe clock.
> 
> Update the PCIe controller and PHY node to reflect the new binding.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index c07765df9303..b3a9630262dc 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -1837,11 +1837,7 @@ pcie1: pci@1c08000 {
>   					<0 0 0 3 &intc 0 0 0 438 IRQ_TYPE_LEVEL_HIGH>,
>   					<0 0 0 4 &intc 0 0 0 439 IRQ_TYPE_LEVEL_HIGH>;
>   
> -			clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> -				 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
> -				 <&pcie1_lane 0>,
> -				 <&rpmhcc RPMH_CXO_CLK>,
> -				 <&gcc GCC_PCIE_1_AUX_CLK>,
> +			clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
>   				 <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
>   				 <&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
>   				 <&gcc GCC_PCIE_1_SLV_AXI_CLK>,
> @@ -1849,11 +1845,7 @@ pcie1: pci@1c08000 {
>   				 <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,
>   				 <&gcc GCC_DDRSS_PCIE_SF_CLK>;
>   
> -			clock-names = "pipe",
> -				      "pipe_mux",
> -				      "phy_pipe",
> -				      "ref",
> -				      "aux",
> +			clock-names = "aux",
>   				      "cfg",
>   				      "bus_master",
>   				      "bus_slave",
> @@ -1910,8 +1902,10 @@ pcie1_lane: lanes@1c0e200 {
>   				      <0 0x01c0e600 0 0x170>,
>   				      <0 0x01c0e800 0 0x200>,
>   				      <0 0x01c0ee00 0 0xf4>;
> -				clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
> -				clock-names = "pipe0";
> +				clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> +					 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
> +					 <&rpmhcc RPMH_CXO_CLK>;
> +				clock-names = "pipe0", "mux", "ref";

This will not be compatible with earlier DTB files, which was a problem 
up to now.

>   
>   				#phy-cells = <0>;
>   				#clock-cells = <1>;


-- 
With best wishes
Dmitry

Re: [PATCH RFC 2/5] arm64: dts: qcom: sc7280: move pipe mux handling to phy

[Dmitry Baryshkov]

On 21/04/2022 13:20, Johan Hovold wrote:
> The QMP PHY pipe clock remuxing is part of the PHY, which is both the
> producer and the consumer of the pipe clock.
> 
> Update the PCIe controller and PHY node to reflect the new binding.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index c07765df9303..b3a9630262dc 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -1837,11 +1837,7 @@ pcie1: pci@1c08000 {
>   					<0 0 0 3 &intc 0 0 0 438 IRQ_TYPE_LEVEL_HIGH>,
>   					<0 0 0 4 &intc 0 0 0 439 IRQ_TYPE_LEVEL_HIGH>;
>   
> -			clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> -				 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
> -				 <&pcie1_lane 0>,
> -				 <&rpmhcc RPMH_CXO_CLK>,
> -				 <&gcc GCC_PCIE_1_AUX_CLK>,
> +			clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
>   				 <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
>   				 <&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
>   				 <&gcc GCC_PCIE_1_SLV_AXI_CLK>,
> @@ -1849,11 +1845,7 @@ pcie1: pci@1c08000 {
>   				 <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,
>   				 <&gcc GCC_DDRSS_PCIE_SF_CLK>;
>   
> -			clock-names = "pipe",
> -				      "pipe_mux",
> -				      "phy_pipe",
> -				      "ref",
> -				      "aux",
> +			clock-names = "aux",
>   				      "cfg",
>   				      "bus_master",
>   				      "bus_slave",
> @@ -1910,8 +1902,10 @@ pcie1_lane: lanes@1c0e200 {
>   				      <0 0x01c0e600 0 0x170>,
>   				      <0 0x01c0e800 0 0x200>,
>   				      <0 0x01c0ee00 0 0xf4>;
> -				clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
> -				clock-names = "pipe0";
> +				clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> +					 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
> +					 <&rpmhcc RPMH_CXO_CLK>;
> +				clock-names = "pipe0", "mux", "ref";

This will not be compatible with earlier DTB files, which was a problem 
up to now.

>   
>   				#phy-cells = <0>;
>   				#clock-cells = <1>;


-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 2/5] arm64: dts: qcom: sc7280: move pipe mux handling to phy

[Johan Hovold]

On Thu, Apr 21, 2022 at 01:59:04PM +0300, Dmitry Baryshkov wrote:
> On 21/04/2022 13:20, Johan Hovold wrote:
> > The QMP PHY pipe clock remuxing is part of the PHY, which is both the
> > producer and the consumer of the pipe clock.
> > 
> > Update the PCIe controller and PHY node to reflect the new binding.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >   arch/arm64/boot/dts/qcom/sc7280.dtsi | 18 ++++++------------
> >   1 file changed, 6 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > index c07765df9303..b3a9630262dc 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > @@ -1837,11 +1837,7 @@ pcie1: pci@1c08000 {
> >   					<0 0 0 3 &intc 0 0 0 438 IRQ_TYPE_LEVEL_HIGH>,
> >   					<0 0 0 4 &intc 0 0 0 439 IRQ_TYPE_LEVEL_HIGH>;
> >   
> > -			clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> > -				 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
> > -				 <&pcie1_lane 0>,
> > -				 <&rpmhcc RPMH_CXO_CLK>,
> > -				 <&gcc GCC_PCIE_1_AUX_CLK>,
> > +			clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
> >   				 <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> >   				 <&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
> >   				 <&gcc GCC_PCIE_1_SLV_AXI_CLK>,
> > @@ -1849,11 +1845,7 @@ pcie1: pci@1c08000 {
> >   				 <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,
> >   				 <&gcc GCC_DDRSS_PCIE_SF_CLK>;
> >   
> > -			clock-names = "pipe",
> > -				      "pipe_mux",
> > -				      "phy_pipe",
> > -				      "ref",
> > -				      "aux",
> > +			clock-names = "aux",
> >   				      "cfg",
> >   				      "bus_master",
> >   				      "bus_slave",
> > @@ -1910,8 +1902,10 @@ pcie1_lane: lanes@1c0e200 {
> >   				      <0 0x01c0e600 0 0x170>,
> >   				      <0 0x01c0e800 0 0x200>,
> >   				      <0 0x01c0ee00 0 0xf4>;
> > -				clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
> > -				clock-names = "pipe0";
> > +				clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> > +					 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
> > +					 <&rpmhcc RPMH_CXO_CLK>;
> > +				clock-names = "pipe0", "mux", "ref";
> 
> This will not be compatible with earlier DTB files, which was a problem 
> up to now.

That depends. The above wasn't added until 5.16 so we may still be able
to fix it.

The NAK you got from Rob earlier was when you removed clocks that have
been in the devicetree for several years:

	https://lore.kernel.org/all/YgQ+tGhLqwUCsTUo@robh.at.kernel.org/

and would still be needed by older kernels.

Worst case, we need to keep both sets for sc7280 (i.e. like we need to
do with the pipe clocks that have been around for years).

Johan

Re: [PATCH RFC 2/5] arm64: dts: qcom: sc7280: move pipe mux handling to phy

[Johan Hovold]

On Thu, Apr 21, 2022 at 01:59:04PM +0300, Dmitry Baryshkov wrote:
> On 21/04/2022 13:20, Johan Hovold wrote:
> > The QMP PHY pipe clock remuxing is part of the PHY, which is both the
> > producer and the consumer of the pipe clock.
> > 
> > Update the PCIe controller and PHY node to reflect the new binding.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >   arch/arm64/boot/dts/qcom/sc7280.dtsi | 18 ++++++------------
> >   1 file changed, 6 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > index c07765df9303..b3a9630262dc 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > @@ -1837,11 +1837,7 @@ pcie1: pci@1c08000 {
> >   					<0 0 0 3 &intc 0 0 0 438 IRQ_TYPE_LEVEL_HIGH>,
> >   					<0 0 0 4 &intc 0 0 0 439 IRQ_TYPE_LEVEL_HIGH>;
> >   
> > -			clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> > -				 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
> > -				 <&pcie1_lane 0>,
> > -				 <&rpmhcc RPMH_CXO_CLK>,
> > -				 <&gcc GCC_PCIE_1_AUX_CLK>,
> > +			clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
> >   				 <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> >   				 <&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
> >   				 <&gcc GCC_PCIE_1_SLV_AXI_CLK>,
> > @@ -1849,11 +1845,7 @@ pcie1: pci@1c08000 {
> >   				 <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,
> >   				 <&gcc GCC_DDRSS_PCIE_SF_CLK>;
> >   
> > -			clock-names = "pipe",
> > -				      "pipe_mux",
> > -				      "phy_pipe",
> > -				      "ref",
> > -				      "aux",
> > +			clock-names = "aux",
> >   				      "cfg",
> >   				      "bus_master",
> >   				      "bus_slave",
> > @@ -1910,8 +1902,10 @@ pcie1_lane: lanes@1c0e200 {
> >   				      <0 0x01c0e600 0 0x170>,
> >   				      <0 0x01c0e800 0 0x200>,
> >   				      <0 0x01c0ee00 0 0xf4>;
> > -				clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
> > -				clock-names = "pipe0";
> > +				clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> > +					 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
> > +					 <&rpmhcc RPMH_CXO_CLK>;
> > +				clock-names = "pipe0", "mux", "ref";
> 
> This will not be compatible with earlier DTB files, which was a problem 
> up to now.

That depends. The above wasn't added until 5.16 so we may still be able
to fix it.

The NAK you got from Rob earlier was when you removed clocks that have
been in the devicetree for several years:

	https://lore.kernel.org/all/YgQ+tGhLqwUCsTUo@robh.at.kernel.org/

and would still be needed by older kernels.

Worst case, we need to keep both sets for sc7280 (i.e. like we need to
do with the pipe clocks that have been around for years).

Johan

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 2/5] arm64: dts: qcom: sc7280: move pipe mux handling to phy

[Dmitry Baryshkov]

On Fri, 22 Apr 2022 at 13:07, Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Apr 21, 2022 at 01:59:04PM +0300, Dmitry Baryshkov wrote:
> > On 21/04/2022 13:20, Johan Hovold wrote:
> > > The QMP PHY pipe clock remuxing is part of the PHY, which is both the
> > > producer and the consumer of the pipe clock.
> > >
> > > Update the PCIe controller and PHY node to reflect the new binding.
> > >
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > ---
> > >   arch/arm64/boot/dts/qcom/sc7280.dtsi | 18 ++++++------------
> > >   1 file changed, 6 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > index c07765df9303..b3a9630262dc 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > @@ -1837,11 +1837,7 @@ pcie1: pci@1c08000 {
> > >                                     <0 0 0 3 &intc 0 0 0 438 IRQ_TYPE_LEVEL_HIGH>,
> > >                                     <0 0 0 4 &intc 0 0 0 439 IRQ_TYPE_LEVEL_HIGH>;
> > >
> > > -                   clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> > > -                            <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
> > > -                            <&pcie1_lane 0>,
> > > -                            <&rpmhcc RPMH_CXO_CLK>,
> > > -                            <&gcc GCC_PCIE_1_AUX_CLK>,
> > > +                   clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
> > >                              <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> > >                              <&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
> > >                              <&gcc GCC_PCIE_1_SLV_AXI_CLK>,
> > > @@ -1849,11 +1845,7 @@ pcie1: pci@1c08000 {
> > >                              <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,
> > >                              <&gcc GCC_DDRSS_PCIE_SF_CLK>;
> > >
> > > -                   clock-names = "pipe",
> > > -                                 "pipe_mux",
> > > -                                 "phy_pipe",
> > > -                                 "ref",
> > > -                                 "aux",
> > > +                   clock-names = "aux",
> > >                                   "cfg",
> > >                                   "bus_master",
> > >                                   "bus_slave",
> > > @@ -1910,8 +1902,10 @@ pcie1_lane: lanes@1c0e200 {
> > >                                   <0 0x01c0e600 0 0x170>,
> > >                                   <0 0x01c0e800 0 0x200>,
> > >                                   <0 0x01c0ee00 0 0xf4>;
> > > -                           clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
> > > -                           clock-names = "pipe0";
> > > +                           clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> > > +                                    <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
> > > +                                    <&rpmhcc RPMH_CXO_CLK>;
> > > +                           clock-names = "pipe0", "mux", "ref";
> >
> > This will not be compatible with earlier DTB files, which was a problem
> > up to now.
>
> That depends. The above wasn't added until 5.16 so we may still be able
> to fix it.

That would depend on Rob/Krzyshtof. But the whole process should be described.
The driver can nod depend on the clocks being there.

>
> The NAK you got from Rob earlier was when you removed clocks that have
> been in the devicetree for several years:
>
>         https://lore.kernel.org/all/YgQ+tGhLqwUCsTUo@robh.at.kernel.org/
>
> and would still be needed by older kernels.
>
> Worst case, we need to keep both sets for sc7280 (i.e. like we need to
> do with the pipe clocks that have been around for years).
>
> Johan



-- 
With best wishes
Dmitry

Re: [PATCH RFC 2/5] arm64: dts: qcom: sc7280: move pipe mux handling to phy

[Dmitry Baryshkov]

On Fri, 22 Apr 2022 at 13:07, Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Apr 21, 2022 at 01:59:04PM +0300, Dmitry Baryshkov wrote:
> > On 21/04/2022 13:20, Johan Hovold wrote:
> > > The QMP PHY pipe clock remuxing is part of the PHY, which is both the
> > > producer and the consumer of the pipe clock.
> > >
> > > Update the PCIe controller and PHY node to reflect the new binding.
> > >
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > ---
> > >   arch/arm64/boot/dts/qcom/sc7280.dtsi | 18 ++++++------------
> > >   1 file changed, 6 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > index c07765df9303..b3a9630262dc 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > @@ -1837,11 +1837,7 @@ pcie1: pci@1c08000 {
> > >                                     <0 0 0 3 &intc 0 0 0 438 IRQ_TYPE_LEVEL_HIGH>,
> > >                                     <0 0 0 4 &intc 0 0 0 439 IRQ_TYPE_LEVEL_HIGH>;
> > >
> > > -                   clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> > > -                            <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
> > > -                            <&pcie1_lane 0>,
> > > -                            <&rpmhcc RPMH_CXO_CLK>,
> > > -                            <&gcc GCC_PCIE_1_AUX_CLK>,
> > > +                   clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
> > >                              <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> > >                              <&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
> > >                              <&gcc GCC_PCIE_1_SLV_AXI_CLK>,
> > > @@ -1849,11 +1845,7 @@ pcie1: pci@1c08000 {
> > >                              <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,
> > >                              <&gcc GCC_DDRSS_PCIE_SF_CLK>;
> > >
> > > -                   clock-names = "pipe",
> > > -                                 "pipe_mux",
> > > -                                 "phy_pipe",
> > > -                                 "ref",
> > > -                                 "aux",
> > > +                   clock-names = "aux",
> > >                                   "cfg",
> > >                                   "bus_master",
> > >                                   "bus_slave",
> > > @@ -1910,8 +1902,10 @@ pcie1_lane: lanes@1c0e200 {
> > >                                   <0 0x01c0e600 0 0x170>,
> > >                                   <0 0x01c0e800 0 0x200>,
> > >                                   <0 0x01c0ee00 0 0xf4>;
> > > -                           clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
> > > -                           clock-names = "pipe0";
> > > +                           clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> > > +                                    <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
> > > +                                    <&rpmhcc RPMH_CXO_CLK>;
> > > +                           clock-names = "pipe0", "mux", "ref";
> >
> > This will not be compatible with earlier DTB files, which was a problem
> > up to now.
>
> That depends. The above wasn't added until 5.16 so we may still be able
> to fix it.

That would depend on Rob/Krzyshtof. But the whole process should be described.
The driver can nod depend on the clocks being there.

>
> The NAK you got from Rob earlier was when you removed clocks that have
> been in the devicetree for several years:
>
>         https://lore.kernel.org/all/YgQ+tGhLqwUCsTUo@robh.at.kernel.org/
>
> and would still be needed by older kernels.
>
> Worst case, we need to keep both sets for sc7280 (i.e. like we need to
> do with the pipe clocks that have been around for years).
>
> Johan



-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH RFC 3/5] PCI: qcom: Remove unnecessary pipe_clk handling

[Johan Hovold]

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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>
Signed-off-by: Johan Hovold <johan+linaro@kernel.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 774d486bf2f7..6e6e40fbfc13 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 RFC 3/5] PCI: qcom: Remove unnecessary pipe_clk handling

[Johan Hovold]

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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>
Signed-off-by: Johan Hovold <johan+linaro@kernel.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 774d486bf2f7..6e6e40fbfc13 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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH RFC 4/5] PCI: qcom: Drop pipe clock muxing

[Johan Hovold]

Some QMP PHYs need to remux to their pipe clock input to the pipe clock
output generated by the PHY before powering on the PHY and restore the
default source during power down.

This is now handled in the QMP PHY driver so remove the broken and
incomplete handling from the PCIe controller driver, which didn't update
the parent clock until after the PHY had been powered on and never
restored the default source after power off.

Signed-off-by: Johan Hovold <johan+linaro@kernel.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 6e6e40fbfc13..d6e33ac9a01a 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

[PATCH RFC 4/5] PCI: qcom: Drop pipe clock muxing

[Johan Hovold]

Some QMP PHYs need to remux to their pipe clock input to the pipe clock
output generated by the PHY before powering on the PHY and restore the
default source during power down.

This is now handled in the QMP PHY driver so remove the broken and
incomplete handling from the PCIe controller driver, which didn't update
the parent clock until after the PHY had been powered on and never
restored the default source after power off.

Signed-off-by: Johan Hovold <johan+linaro@kernel.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 6e6e40fbfc13..d6e33ac9a01a 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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH RFC 5/5] PCI: qcom: Drop unused post-init callbacks

[Johan Hovold]

Drop the unused post_init and post_deinit callbacks that were added for
the now removed pipe clock handling.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index d6e33ac9a01a..7606a9f6eb21 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -180,9 +180,7 @@ struct qcom_pcie;
 struct qcom_pcie_ops {
 	int (*get_resources)(struct qcom_pcie *pcie);
 	int (*init)(struct qcom_pcie *pcie);
-	int (*post_init)(struct qcom_pcie *pcie);
 	void (*deinit)(struct qcom_pcie *pcie);
-	void (*post_deinit)(struct qcom_pcie *pcie);
 	void (*ltssm_enable)(struct qcom_pcie *pcie);
 	int (*config_sid)(struct qcom_pcie *pcie);
 };
@@ -1331,27 +1329,18 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
 	if (ret)
 		goto err_deinit;
 
-	if (pcie->cfg->ops->post_init) {
-		ret = pcie->cfg->ops->post_init(pcie);
-		if (ret)
-			goto err_disable_phy;
-	}
-
 	qcom_ep_reset_deassert(pcie);
 
 	if (pcie->cfg->ops->config_sid) {
 		ret = pcie->cfg->ops->config_sid(pcie);
 		if (ret)
-			goto err;
+			goto err_assert_reset;
 	}
 
 	return 0;
 
-err:
+err_assert_reset:
 	qcom_ep_reset_assert(pcie);
-	if (pcie->cfg->ops->post_deinit)
-		pcie->cfg->ops->post_deinit(pcie);
-err_disable_phy:
 	phy_power_off(pcie->phy);
 err_deinit:
 	pcie->cfg->ops->deinit(pcie);
-- 
2.35.1

[PATCH RFC 5/5] PCI: qcom: Drop unused post-init callbacks

[Johan Hovold]

Drop the unused post_init and post_deinit callbacks that were added for
the now removed pipe clock handling.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index d6e33ac9a01a..7606a9f6eb21 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -180,9 +180,7 @@ struct qcom_pcie;
 struct qcom_pcie_ops {
 	int (*get_resources)(struct qcom_pcie *pcie);
 	int (*init)(struct qcom_pcie *pcie);
-	int (*post_init)(struct qcom_pcie *pcie);
 	void (*deinit)(struct qcom_pcie *pcie);
-	void (*post_deinit)(struct qcom_pcie *pcie);
 	void (*ltssm_enable)(struct qcom_pcie *pcie);
 	int (*config_sid)(struct qcom_pcie *pcie);
 };
@@ -1331,27 +1329,18 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
 	if (ret)
 		goto err_deinit;
 
-	if (pcie->cfg->ops->post_init) {
-		ret = pcie->cfg->ops->post_init(pcie);
-		if (ret)
-			goto err_disable_phy;
-	}
-
 	qcom_ep_reset_deassert(pcie);
 
 	if (pcie->cfg->ops->config_sid) {
 		ret = pcie->cfg->ops->config_sid(pcie);
 		if (ret)
-			goto err;
+			goto err_assert_reset;
 	}
 
 	return 0;
 
-err:
+err_assert_reset:
 	qcom_ep_reset_assert(pcie);
-	if (pcie->cfg->ops->post_deinit)
-		pcie->cfg->ops->post_deinit(pcie);
-err_disable_phy:
 	phy_power_off(pcie->phy);
 err_deinit:
 	pcie->cfg->ops->deinit(pcie);
-- 
2.35.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy