From: Adrien Thierry <athierry@redhat.com> To: Bjorn Andersson <andersson@kernel.org> Cc: Andy Gross <agross@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Vinod Koul <vkoul@kernel.org>, Kishon Vijay Abraham I <kishon@kernel.org>, linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org Subject: Re: [PATCH RFC 2/2] phy: qcom-snps-femto-v2: Remove AHB2PHY interface clock Date: Thu, 1 Jun 2023 13:12:02 -0400 [thread overview] Message-ID: <ZHjRYpjwHI3bP4yC@fedora> (raw) In-Reply-To: <20230529211921.imf6tttlrkza4lc3@ripper> Hi Bjorn, On Mon, May 29, 2023 at 02:19:21PM -0700, Bjorn Andersson wrote: > On Mon, May 29, 2023 at 02:56:37PM -0400, Adrien Thierry wrote: > > The AHB2PHY interface clock cfg_ahb_clk is not assigned anywhere in the > > driver. Moreover, it's not used by any device tree, nor is present in > > the qcom,usb-snps-femto-v2 bindings. Remove it. > > > > The downstream driver deals with cfg_ahb as well, so I think it would be > more appropriate to ensure that it's actually wired up. > > And in that case, I would find it preferable to switch to use the > clk_bulk API for the introduction of the ref clk - to clean up the error > paths if nothing else. > I won't be able to properly wire the cfg_ahb clock in the device trees because I've got no way of knowing which clock to use for cfg_ahb in the various SoCs, but I can at least try to clean the code by using the clk_bulk API. > Regards, > Bjorn > > > Signed-off-by: Adrien Thierry <athierry@redhat.com> > > --- > > I'm not 100% sure if the clock should be removed, or if it should be added > > to bindings and device trees that use this PHY. Better informed opinions > > on the topic are highly welcome. > > > > drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 20 +------------------ > > 1 file changed, 1 insertion(+), 19 deletions(-) > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > index 8abf482e81a8..2d9c1105e28c 100644 > > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > @@ -113,7 +113,6 @@ struct phy_override_seq { > > * @phy: generic phy > > * @base: iomapped memory space for snps hs phy > > * > > - * @cfg_ahb_clk: AHB2PHY interface clock > > * @ref_clk: phy reference clock > > * @phy_reset: phy reset control > > * @vregs: regulator supplies bulk data > > @@ -125,7 +124,6 @@ struct qcom_snps_hsphy { > > struct phy *phy; > > void __iomem *base; > > > > - struct clk *cfg_ahb_clk; > > struct clk *ref_clk; > > struct reset_control *phy_reset; > > struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; > > @@ -165,7 +163,6 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) > > 0, USB2_AUTO_RESUME); > > } > > > > - clk_disable_unprepare(hsphy->cfg_ahb_clk); > > clk_disable_unprepare(hsphy->ref_clk); > > return 0; > > } > > @@ -176,12 +173,6 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) > > > > dev_dbg(&hsphy->phy->dev, "Resume QCOM SNPS PHY, mode\n"); > > > > - ret = clk_prepare_enable(hsphy->cfg_ahb_clk); > > - if (ret) { > > - dev_err(&hsphy->phy->dev, "failed to enable cfg ahb clock\n"); > > - return ret; > > - } > > - > > ret = clk_prepare_enable(hsphy->ref_clk); > > if (ret) { > > dev_err(&hsphy->phy->dev, "failed to enable ref clock\n"); > > @@ -381,16 +372,10 @@ static int qcom_snps_hsphy_init(struct phy *phy) > > if (ret) > > return ret; > > > > - ret = clk_prepare_enable(hsphy->cfg_ahb_clk); > > - if (ret) { > > - dev_err(&phy->dev, "failed to enable cfg ahb clock, %d\n", ret); > > - goto poweroff_phy; > > - } > > - > > ret = clk_prepare_enable(hsphy->ref_clk); > > if (ret) { > > dev_err(&phy->dev, "failed to enable ref clock, %d\n", ret); > > - goto disable_ahb_clk; > > + goto poweroff_phy; > > } > > > > ret = reset_control_assert(hsphy->phy_reset); > > @@ -463,8 +448,6 @@ static int qcom_snps_hsphy_init(struct phy *phy) > > > > disable_ref_clk: > > clk_disable_unprepare(hsphy->ref_clk); > > -disable_ahb_clk: > > - clk_disable_unprepare(hsphy->cfg_ahb_clk); > > poweroff_phy: > > regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs); > > > > @@ -476,7 +459,6 @@ static int qcom_snps_hsphy_exit(struct phy *phy) > > struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); > > > > reset_control_assert(hsphy->phy_reset); > > - clk_disable_unprepare(hsphy->cfg_ahb_clk); > > clk_disable_unprepare(hsphy->ref_clk); > > regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs); > > hsphy->phy_initialized = false; > > -- > > 2.40.1 > > -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy
WARNING: multiple messages have this Message-ID (diff)
From: Adrien Thierry <athierry@redhat.com> To: Bjorn Andersson <andersson@kernel.org> Cc: Andy Gross <agross@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Vinod Koul <vkoul@kernel.org>, Kishon Vijay Abraham I <kishon@kernel.org>, linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org Subject: Re: [PATCH RFC 2/2] phy: qcom-snps-femto-v2: Remove AHB2PHY interface clock Date: Thu, 1 Jun 2023 13:12:02 -0400 [thread overview] Message-ID: <ZHjRYpjwHI3bP4yC@fedora> (raw) In-Reply-To: <20230529211921.imf6tttlrkza4lc3@ripper> Hi Bjorn, On Mon, May 29, 2023 at 02:19:21PM -0700, Bjorn Andersson wrote: > On Mon, May 29, 2023 at 02:56:37PM -0400, Adrien Thierry wrote: > > The AHB2PHY interface clock cfg_ahb_clk is not assigned anywhere in the > > driver. Moreover, it's not used by any device tree, nor is present in > > the qcom,usb-snps-femto-v2 bindings. Remove it. > > > > The downstream driver deals with cfg_ahb as well, so I think it would be > more appropriate to ensure that it's actually wired up. > > And in that case, I would find it preferable to switch to use the > clk_bulk API for the introduction of the ref clk - to clean up the error > paths if nothing else. > I won't be able to properly wire the cfg_ahb clock in the device trees because I've got no way of knowing which clock to use for cfg_ahb in the various SoCs, but I can at least try to clean the code by using the clk_bulk API. > Regards, > Bjorn > > > Signed-off-by: Adrien Thierry <athierry@redhat.com> > > --- > > I'm not 100% sure if the clock should be removed, or if it should be added > > to bindings and device trees that use this PHY. Better informed opinions > > on the topic are highly welcome. > > > > drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 20 +------------------ > > 1 file changed, 1 insertion(+), 19 deletions(-) > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > index 8abf482e81a8..2d9c1105e28c 100644 > > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > @@ -113,7 +113,6 @@ struct phy_override_seq { > > * @phy: generic phy > > * @base: iomapped memory space for snps hs phy > > * > > - * @cfg_ahb_clk: AHB2PHY interface clock > > * @ref_clk: phy reference clock > > * @phy_reset: phy reset control > > * @vregs: regulator supplies bulk data > > @@ -125,7 +124,6 @@ struct qcom_snps_hsphy { > > struct phy *phy; > > void __iomem *base; > > > > - struct clk *cfg_ahb_clk; > > struct clk *ref_clk; > > struct reset_control *phy_reset; > > struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; > > @@ -165,7 +163,6 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) > > 0, USB2_AUTO_RESUME); > > } > > > > - clk_disable_unprepare(hsphy->cfg_ahb_clk); > > clk_disable_unprepare(hsphy->ref_clk); > > return 0; > > } > > @@ -176,12 +173,6 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) > > > > dev_dbg(&hsphy->phy->dev, "Resume QCOM SNPS PHY, mode\n"); > > > > - ret = clk_prepare_enable(hsphy->cfg_ahb_clk); > > - if (ret) { > > - dev_err(&hsphy->phy->dev, "failed to enable cfg ahb clock\n"); > > - return ret; > > - } > > - > > ret = clk_prepare_enable(hsphy->ref_clk); > > if (ret) { > > dev_err(&hsphy->phy->dev, "failed to enable ref clock\n"); > > @@ -381,16 +372,10 @@ static int qcom_snps_hsphy_init(struct phy *phy) > > if (ret) > > return ret; > > > > - ret = clk_prepare_enable(hsphy->cfg_ahb_clk); > > - if (ret) { > > - dev_err(&phy->dev, "failed to enable cfg ahb clock, %d\n", ret); > > - goto poweroff_phy; > > - } > > - > > ret = clk_prepare_enable(hsphy->ref_clk); > > if (ret) { > > dev_err(&phy->dev, "failed to enable ref clock, %d\n", ret); > > - goto disable_ahb_clk; > > + goto poweroff_phy; > > } > > > > ret = reset_control_assert(hsphy->phy_reset); > > @@ -463,8 +448,6 @@ static int qcom_snps_hsphy_init(struct phy *phy) > > > > disable_ref_clk: > > clk_disable_unprepare(hsphy->ref_clk); > > -disable_ahb_clk: > > - clk_disable_unprepare(hsphy->cfg_ahb_clk); > > poweroff_phy: > > regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs); > > > > @@ -476,7 +459,6 @@ static int qcom_snps_hsphy_exit(struct phy *phy) > > struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); > > > > reset_control_assert(hsphy->phy_reset); > > - clk_disable_unprepare(hsphy->cfg_ahb_clk); > > clk_disable_unprepare(hsphy->ref_clk); > > regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs); > > hsphy->phy_initialized = false; > > -- > > 2.40.1 > >
next prev parent reply other threads:[~2023-06-01 17:12 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-05-29 18:56 [PATCH RFC 0/2] Clock fixes for qcom-snps-femto-v2 PHY driver Adrien Thierry 2023-05-29 18:56 ` Adrien Thierry 2023-05-29 18:56 ` [PATCH RFC 1/2] phy: qcom-snps-femto-v2: properly enable ref clock Adrien Thierry 2023-05-29 18:56 ` Adrien Thierry 2023-05-29 21:16 ` Bjorn Andersson 2023-05-29 21:16 ` Bjorn Andersson 2023-05-30 18:46 ` Adrien Thierry 2023-05-30 18:46 ` Adrien Thierry 2023-05-31 2:34 ` Bjorn Andersson 2023-05-31 2:34 ` Bjorn Andersson 2023-06-01 17:09 ` Adrien Thierry 2023-06-01 17:09 ` Adrien Thierry 2023-05-30 13:22 ` Andrew Halaney 2023-05-30 13:22 ` Andrew Halaney 2023-05-29 18:56 ` [PATCH RFC 2/2] phy: qcom-snps-femto-v2: Remove AHB2PHY interface clock Adrien Thierry 2023-05-29 18:56 ` Adrien Thierry 2023-05-29 21:19 ` Bjorn Andersson 2023-05-29 21:19 ` Bjorn Andersson 2023-06-01 17:12 ` Adrien Thierry [this message] 2023-06-01 17:12 ` Adrien Thierry
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=ZHjRYpjwHI3bP4yC@fedora \ --to=athierry@redhat.com \ --cc=agross@kernel.org \ --cc=andersson@kernel.org \ --cc=kishon@kernel.org \ --cc=konrad.dybcio@linaro.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-phy@lists.infradead.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.