From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> To: Johan Hovold <johan+linaro@kernel.org>, Andy Gross <agross@kernel.org>, Bjorn Andersson <bjorn.andersson@linaro.org>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Kishon Vijay Abraham I <kishon@ti.com>, Vinod Koul <vkoul@kernel.org>, Stephen Boyd <swboyd@chromium.org> Cc: "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzk+dt@kernel.org>, "Stanimir Varbanov" <svarbanov@mm-sol.com>, "Krzysztof Wilczyński" <kw@linux.com>, "Bjorn Helgaas" <bhelgaas@google.com>, "Prasad Malisetty" <quic_pmaliset@quicinc.com>, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, linux-phy@lists.infradead.org Subject: Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing Date: Thu, 21 Apr 2022 14:08:27 +0300 [thread overview] Message-ID: <de4f9514-5132-f208-d43f-4c50afcda203@linaro.org> (raw) In-Reply-To: <20220421102041.17345-2-johan+linaro@kernel.org> 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
WARNING: multiple messages have this Message-ID (diff)
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> To: Johan Hovold <johan+linaro@kernel.org>, Andy Gross <agross@kernel.org>, Bjorn Andersson <bjorn.andersson@linaro.org>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Kishon Vijay Abraham I <kishon@ti.com>, Vinod Koul <vkoul@kernel.org>, Stephen Boyd <swboyd@chromium.org> Cc: "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzk+dt@kernel.org>, "Stanimir Varbanov" <svarbanov@mm-sol.com>, "Krzysztof Wilczyński" <kw@linux.com>, "Bjorn Helgaas" <bhelgaas@google.com>, "Prasad Malisetty" <quic_pmaliset@quicinc.com>, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, linux-phy@lists.infradead.org Subject: Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing Date: Thu, 21 Apr 2022 14:08:27 +0300 [thread overview] Message-ID: <de4f9514-5132-f208-d43f-4c50afcda203@linaro.org> (raw) In-Reply-To: <20220421102041.17345-2-johan+linaro@kernel.org> 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
next prev parent reply other threads:[~2022-04-21 11:08 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-04-21 10:20 [PATCH RFC 0/5] phy: qcom-qmp: add support for pipe clock muxing Johan Hovold 2022-04-21 10:20 ` Johan Hovold 2022-04-21 10:20 ` [PATCH RFC 1/5] " Johan Hovold 2022-04-21 10:20 ` Johan Hovold 2022-04-21 11:08 ` Dmitry Baryshkov [this message] 2022-04-21 11:08 ` Dmitry Baryshkov 2022-04-22 10:20 ` Johan Hovold 2022-04-22 10:20 ` Johan Hovold 2022-04-22 10:35 ` Dmitry Baryshkov 2022-04-22 10:35 ` Dmitry Baryshkov 2022-04-22 11:22 ` Johan Hovold 2022-04-22 11:22 ` Johan Hovold 2022-04-28 16:15 ` Rob Herring 2022-04-28 16:15 ` Rob Herring 2022-04-21 11:36 ` Dmitry Baryshkov 2022-04-21 11:36 ` Dmitry Baryshkov 2022-04-22 10:41 ` Johan Hovold 2022-04-22 10:41 ` Johan Hovold 2022-04-28 13:11 ` Bjorn Andersson 2022-04-28 13:11 ` Bjorn Andersson 2022-04-29 6:53 ` Johan Hovold 2022-04-29 6:53 ` Johan Hovold 2022-04-21 10:20 ` [PATCH RFC 2/5] arm64: dts: qcom: sc7280: move pipe mux handling to phy Johan Hovold 2022-04-21 10:20 ` Johan Hovold 2022-04-21 10:59 ` Dmitry Baryshkov 2022-04-21 10:59 ` Dmitry Baryshkov 2022-04-22 10:07 ` Johan Hovold 2022-04-22 10:07 ` Johan Hovold 2022-04-22 10:36 ` Dmitry Baryshkov 2022-04-22 10:36 ` Dmitry Baryshkov 2022-04-21 10:20 ` [PATCH RFC 3/5] PCI: qcom: Remove unnecessary pipe_clk handling Johan Hovold 2022-04-21 10:20 ` Johan Hovold 2022-04-21 10:20 ` [PATCH RFC 4/5] PCI: qcom: Drop pipe clock muxing Johan Hovold 2022-04-21 10:20 ` Johan Hovold 2022-04-21 10:20 ` [PATCH RFC 5/5] PCI: qcom: Drop unused post-init callbacks Johan Hovold 2022-04-21 10:20 ` Johan Hovold
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=de4f9514-5132-f208-d43f-4c50afcda203@linaro.org \ --to=dmitry.baryshkov@linaro.org \ --cc=agross@kernel.org \ --cc=bhelgaas@google.com \ --cc=bjorn.andersson@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=johan+linaro@kernel.org \ --cc=kishon@ti.com \ --cc=krzk+dt@kernel.org \ --cc=kw@linux.com \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux-phy@lists.infradead.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=quic_pmaliset@quicinc.com \ --cc=robh+dt@kernel.org \ --cc=svarbanov@mm-sol.com \ --cc=swboyd@chromium.org \ --cc=vkoul@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.