Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230418111612.19479-1-ddrokosov@sberdevices.ru> <20230418111612.19479-2-ddrokosov@sberdevices.ru>
In-Reply-To: <20230418111612.19479-2-ddrokosov@sberdevices.ru>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Sun, 23 Apr 2023 19:42:25 +0200
Message-ID: <CAFBinCDyhBQ5Nob38EmXor1PtcO09dRdReDTW+tc5CN4i20HhA@mail.gmail.com>
Subject: Re: [PATCH v2 1/5] phy: amlogic: enable/disable clkin during Amlogic
 USB PHY init/exit
To:     Dmitry Rokosov <ddrokosov@sberdevices.ru>
Cc:     gregkh@linuxfoundation.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, neil.armstrong@linaro.org,
        khilman@baylibre.com, jbrunet@baylibre.com,
        mturquette@baylibre.com, vkoul@kernel.org, kishon@kernel.org,
        hminas@synopsys.com, Thinh.Nguyen@synopsys.com,
        yue.wang@amlogic.com, hanjie.lin@amlogic.com,
        kernel@sberdevices.ru, rockosov@gmail.com,
        linux-usb@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-phy@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Dmitry,

On Tue, Apr 18, 2023 at 1:16=E2=80=AFPM Dmitry Rokosov <ddrokosov@sberdevic=
es.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/amlo=
gic/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 =3D reset_control_reset(priv->reset);
> +       ret =3D clk_prepare_enable(priv->clk);
>         if (ret)
>                 return ret;
>
> +       ret =3D 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 =3D=3D 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 =3D phy_get_drvdata(phy);
> +       int ret =3D 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