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@xxxxxxxxxx>
> > ---
> > 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
> >
