Re: [PATCH] phy: rockchip: inno-usb2: fix hang when multiple controllers exit

[Simon Glass <sjg@chromium.org>]

Hi,

On Sat, 23 Oct 2021 at 12:19, Icenowy Zheng <icenowy@aosc.io> wrote:
>
> 在 2021-10-23星期六的 13:23 -0400，Siva Mahadevan写道：
> > Icenowy Zheng wrote:
> > > The OHCI and EHCI controllers are both bound to the same PHY. They
> > > will
> > > both do init and power_on operations when the controller is brought
> > > up
> > > and both do power_off and exit when the controller is stopped.
> > > However,
> > > the PHY uclass of U-Boot is not as sane as we thought -- they won't
> > > maintain a status mark for PHYs, and thus the functions of the PHYs
> > > could be called for multiple times. Calling init/power_on for
> > > multiple
> > > times have no severe problems, however calling power_off/exit for
> > > multiple times have a problem -- the first exit call will stop the
> > > PHY
> > > clock, and power_off/exit calls after it still trying to write to
> > > PHY
> > > registers. The write operation to PHY registers will fail because
> > > clock
> > > is already stopped.
> > >
> > > Adapt the count mechanism from phy-sun4i-usb to both init/exit and
> > > power_on/power_off functions to phy-rockchip-inno-usb2 to fix this
> > > problem. With this stopping USB controllers (manually or before
> > > booting
> > > a kernel) will work.
> > >
> > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > > Fixes: ac97a9ece14e ("phy: rockchip: Add Rockchip USB2PHY driver")
> > > ---
> > >  drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 21
> > > +++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > >
> > > diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > > b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > > index 62b8ba3a4a..be9cc99d90 100644
> > > --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > > @@ -62,6 +62,8 @@ struct rockchip_usb2phy {
> > >         void *reg_base;
> > >         struct clk phyclk;
> > >         const struct rockchip_usb2phy_cfg *phy_cfg;
> > > +       int init_count;
> > > +       int power_on_count;
> > >  };
> > >
> > >  static inline int property_enable(void *reg_base,
> > > @@ -92,6 +94,10 @@ static int rockchip_usb2phy_power_on(struct phy
> > > *phy)
> > >         struct rockchip_usb2phy *priv = dev_get_priv(parent);
> > >         const struct rockchip_usb2phy_port_cfg *port_cfg =
> > > us2phy_get_port(phy);
> > >
> > > +       priv->power_on_count++;
> > > +       if (priv->power_on_count != 1)
> > > +               return 0;
> > > +
> > >         property_enable(priv->reg_base, &port_cfg->phy_sus, false);
> > >
> > >         /* waiting for the utmi_clk to become stable */
> > > @@ -106,6 +112,10 @@ static int rockchip_usb2phy_power_off(struct
> > > phy *phy)
> > >         struct rockchip_usb2phy *priv = dev_get_priv(parent);
> > >         const struct rockchip_usb2phy_port_cfg *port_cfg =
> > > us2phy_get_port(phy);
> > >
> > > +       priv->power_on_count--;
> > > +       if (priv->power_on_count != 0)
> > > +               return 0;
> > > +
> > >         property_enable(priv->reg_base, &port_cfg->phy_sus, true);
> > >
> > >         return 0;
> > > @@ -118,6 +128,10 @@ static int rockchip_usb2phy_init(struct phy
> > > *phy)
> > >         const struct rockchip_usb2phy_port_cfg *port_cfg =
> > > us2phy_get_port(phy);
> > >         int ret;
> > >
> > > +       priv->init_count++;
> > > +       if (priv->init_count != 1)
> > > +               return 0;
> > > +
> > >         ret = clk_enable(&priv->phyclk);
> > >         if (ret) {
> > >                 dev_err(phy->dev, "failed to enable phyclk
> > > (ret=%d)\n", ret);
> > > @@ -140,6 +154,10 @@ static int rockchip_usb2phy_exit(struct phy
> > > *phy)
> > >         struct udevice *parent = dev_get_parent(phy->dev);
> > >         struct rockchip_usb2phy *priv = dev_get_priv(parent);
> > >
> > > +       priv->init_count--;
> > > +       if (priv->init_count != 0)
> > > +               return 0;
> > > +
> > >         clk_disable(&priv->phyclk);
> > >
> > >         return 0;
> > > @@ -212,6 +230,9 @@ static int rockchip_usb2phy_probe(struct
> > > udevice *dev)
> > >                 return ret;
> > >         }
> > >
> > > +       priv->power_on_count = 0;
> > > +       priv->init_count = 0;
> > > +
> > >         return 0;
> > >  }
> > >
> > > --
> > > 2.30.2
> >
> > Are there any plans of submitting this patch to u-boot mainline? I
> > recently got a pinebook pro and got u-boot mainline working as-is
> > plus
> > this patch. I can confirm that this fixes the issue for me.
>
> The current maintainer wants a fix in PHY framework instead of the
> specific driver, but I am recently not interested in fixing it (because
> PBP is my daily driver now, and I don't dare to do dangerous BL
> development that will lead to regression on it).
>

From what I can tell the maintainer is correct - the PHY uclass should
be updated as clk was. There is a sandbox driver for the PHY uclass so
you can add a test for it the new behaviour. I don't think there is
too much risk of a regression, but in any case it seems like the
correct fix to me.

Regards,
Simon