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;
Date:   Tue, 25 Apr 2023 13:29:12 +0300
From:   Dmitry Rokosov <ddrokosov@sberdevices.ru>
To:     Martin Blumenstingl <martin.blumenstingl@googlemail.com>
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>
Subject: Re: [PATCH v2 1/5] phy: amlogic: enable/disable clkin during Amlogic
 USB PHY init/exit
Message-ID: <20230425102912.qhey7fpbuqvwg44j@CAB-WSD-L081021>
References: <20230418111612.19479-1-ddrokosov@sberdevices.ru>
 <20230418111612.19479-2-ddrokosov@sberdevices.ru>
 <CAFBinCDyhBQ5Nob38EmXor1PtcO09dRdReDTW+tc5CN4i20HhA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAFBinCDyhBQ5Nob38EmXor1PtcO09dRdReDTW+tc5CN4i20HhA@mail.gmail.com>
User-Agent: NeoMutt/20220415
Precedence: bulk

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.

-- 
Thank you,
Dmitry