Re: [PATCH RFC 2/2] phy: qcom-snps-femto-v2: Remove AHB2PHY interface clock

[Adrien Thierry <athierry@redhat.com>]

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