Messages in this thread | | | Date | Thu, 21 Apr 2022 14:36:05 +0300 | Subject | Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing | From | 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
| |