[v2,1/5] phy: amlogic: enable/disable clkin during Amlogic USB PHY init/exit

  Previously, all Amlogic boards used the XTAL clock as the default board
clock for the USB PHY input, so there was no need to enable it.
However, with the introduction of new Amlogic SoCs like the A1 family,
the USB PHY now uses a gated clock. Hence, it is necessary to enable
this gated clock during the PHY initialization sequence, or disable it
during the PHY exit, as appropriate.

Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
---
 drivers/phy/amlogic/phy-meson-g12a-usb2.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
  

On 18/04/2023 13:16, Dmitry Rokosov wrote:
> Previously, all Amlogic boards used the XTAL clock as the default board
> clock for the USB PHY input, so there was no need to enable it.
> However, with the introduction of new Amlogic SoCs like the A1 family,
> the USB PHY now uses a gated clock. Hence, it is necessary to enable
> this gated clock during the PHY initialization sequence, or disable it
> during the PHY exit, as appropriate.
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> ---
>   drivers/phy/amlogic/phy-meson-g12a-usb2.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/amlogic/phy-meson-g12a-usb2.c b/drivers/phy/amlogic/phy-meson-g12a-usb2.c
> index 9d1efa0d9394..80938751da4f 100644
> --- a/drivers/phy/amlogic/phy-meson-g12a-usb2.c
> +++ b/drivers/phy/amlogic/phy-meson-g12a-usb2.c
> @@ -172,10 +172,16 @@ static int phy_meson_g12a_usb2_init(struct phy *phy)
>   	int ret;
>   	unsigned int value;
>   
> -	ret = reset_control_reset(priv->reset);
> +	ret = clk_prepare_enable(priv->clk);
>   	if (ret)
>   		return ret;
>   
> +	ret = reset_control_reset(priv->reset);
> +	if (ret) {
> +		clk_disable_unprepare(priv->clk);
> +		return ret;
> +	}
> +
>   	udelay(RESET_COMPLETE_TIME);
>   
>   	/* usb2_otg_aca_en == 0 */
> @@ -277,8 +283,11 @@ static int phy_meson_g12a_usb2_init(struct phy *phy)
>   static int phy_meson_g12a_usb2_exit(struct phy *phy)
>   {
>   	struct phy_meson_g12a_usb2_priv *priv = phy_get_drvdata(phy);
> +	int ret = reset_control_reset(priv->reset);
> +
> +	clk_disable_unprepare(priv->clk);
>   
> -	return reset_control_reset(priv->reset);
> +	return ret;
>   }
>   
>   /* set_mode is not needed, mode setting is handled via the UTMI bus */

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
  

Hi Dmitry,

On Tue, Apr 18, 2023 at 1:16 PM Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote:
>
> Previously, all Amlogic boards used the XTAL clock as the default board
> clock for the USB PHY input, so there was no need to enable it.
> However, with the introduction of new Amlogic SoCs like the A1 family,
> the USB PHY now uses a gated clock. Hence, it is necessary to enable
> this gated clock during the PHY initialization sequence, or disable it
> during the PHY exit, as appropriate.
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> ---
>  drivers/phy/amlogic/phy-meson-g12a-usb2.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/phy/amlogic/phy-meson-g12a-usb2.c b/drivers/phy/amlogic/phy-meson-g12a-usb2.c
> index 9d1efa0d9394..80938751da4f 100644
> --- a/drivers/phy/amlogic/phy-meson-g12a-usb2.c
> +++ b/drivers/phy/amlogic/phy-meson-g12a-usb2.c
> @@ -172,10 +172,16 @@ static int phy_meson_g12a_usb2_init(struct phy *phy)
>         int ret;
>         unsigned int value;
>
> -       ret = reset_control_reset(priv->reset);
> +       ret = clk_prepare_enable(priv->clk);
>         if (ret)
>                 return ret;
>
> +       ret = reset_control_reset(priv->reset);
> +       if (ret) {
> +               clk_disable_unprepare(priv->clk);
> +               return ret;
> +       }
> +
This part looks good. You asked why I suggested this approach instead
of enabling the clock at probe time and only now I have time to reply
to it.
Consider the following scenario:
- modprobe phy-meson-g12a-usb2
- modprobe dwc3-meson-g12a (this will call phy_init)
- rmmod dwc3-meson-g12a (this will call phy_exit)

If the clock was enabled at probe time then it would only be disabled
when using rmmod phy-meson-g12a-usb2.
By manually calling clk_prepare_enable/clk_disable_unprepare we ensure
that the clock gets disabled when we don't need the PHY anymore.
Whether this makes any difference in terms of power draw: I can't say.

>         udelay(RESET_COMPLETE_TIME);
>
>         /* usb2_otg_aca_en == 0 */
> @@ -277,8 +283,11 @@ static int phy_meson_g12a_usb2_init(struct phy *phy)
>  static int phy_meson_g12a_usb2_exit(struct phy *phy)
>  {
>         struct phy_meson_g12a_usb2_priv *priv = phy_get_drvdata(phy);
> +       int ret = reset_control_reset(priv->reset);
> +
> +       clk_disable_unprepare(priv->clk);
>
> -       return reset_control_reset(priv->reset);
> +       return ret;
I think this can cause issues in case when reset_control_reset returns
an error: If I understand the code in phy-core.c correctly it will
only decrease the init ref-count if exit returns 0.
Whenever phy_exit is called for the second time
clk_disable_unprepare() will be called with a clock ref-count of 0, so
it'll likely print some warning.

My suggestion is to return early if reset_control_reset() fails and
not call clk_disable_unprepare() in that case.
What do you think?


Best regards,
Martin
  

Hello Martin,

Thanks a lot for the review! Appreciate it!
Please find my comments below.

On Sun, Apr 23, 2023 at 07:42:25PM +0200, Martin Blumenstingl wrote:
> Hi Dmitry,
> 
> On Tue, Apr 18, 2023 at 1:16 PM Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote:
> >
> > Previously, all Amlogic boards used the XTAL clock as the default board
> > clock for the USB PHY input, so there was no need to enable it.
> > However, with the introduction of new Amlogic SoCs like the A1 family,
> > the USB PHY now uses a gated clock. Hence, it is necessary to enable
> > this gated clock during the PHY initialization sequence, or disable it
> > during the PHY exit, as appropriate.
> >
> > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> > ---
> >  drivers/phy/amlogic/phy-meson-g12a-usb2.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/phy/amlogic/phy-meson-g12a-usb2.c b/drivers/phy/amlogic/phy-meson-g12a-usb2.c
> > index 9d1efa0d9394..80938751da4f 100644
> > --- a/drivers/phy/amlogic/phy-meson-g12a-usb2.c
> > +++ b/drivers/phy/amlogic/phy-meson-g12a-usb2.c
> > @@ -172,10 +172,16 @@ static int phy_meson_g12a_usb2_init(struct phy *phy)
> >         int ret;
> >         unsigned int value;
> >
> > -       ret = reset_control_reset(priv->reset);
> > +       ret = clk_prepare_enable(priv->clk);
> >         if (ret)
> >                 return ret;
> >
> > +       ret = reset_control_reset(priv->reset);
> > +       if (ret) {
> > +               clk_disable_unprepare(priv->clk);
> > +               return ret;
> > +       }
> > +
> This part looks good. You asked why I suggested this approach instead
> of enabling the clock at probe time and only now I have time to reply
> to it.
> Consider the following scenario:
> - modprobe phy-meson-g12a-usb2
> - modprobe dwc3-meson-g12a (this will call phy_init)
> - rmmod dwc3-meson-g12a (this will call phy_exit)
> 
> If the clock was enabled at probe time then it would only be disabled
> when using rmmod phy-meson-g12a-usb2.
> By manually calling clk_prepare_enable/clk_disable_unprepare we ensure
> that the clock gets disabled when we don't need the PHY anymore.
> Whether this makes any difference in terms of power draw: I can't say.
> 

It makes sense. I fully agree with your approach, which looks better
compared to using the automatic devm_clk API.

> >         udelay(RESET_COMPLETE_TIME);
> >
> >         /* usb2_otg_aca_en == 0 */
> > @@ -277,8 +283,11 @@ static int phy_meson_g12a_usb2_init(struct phy *phy)
> >  static int phy_meson_g12a_usb2_exit(struct phy *phy)
> >  {
> >         struct phy_meson_g12a_usb2_priv *priv = phy_get_drvdata(phy);
> > +       int ret = reset_control_reset(priv->reset);
> > +
> > +       clk_disable_unprepare(priv->clk);
> >
> > -       return reset_control_reset(priv->reset);
> > +       return ret;
> I think this can cause issues in case when reset_control_reset returns
> an error: If I understand the code in phy-core.c correctly it will
> only decrease the init ref-count if exit returns 0.
> Whenever phy_exit is called for the second time
> clk_disable_unprepare() will be called with a clock ref-count of 0, so
> it'll likely print some warning.
> 
> My suggestion is to return early if reset_control_reset() fails and
> not call clk_disable_unprepare() in that case.
> What do you think?

After taking a closer look at the phy_exit core code, it appears that
your idea is spot on. Exiting immediately when reset fails seems like
the better choice.
I will work on a new version of the code accordingly.