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

[Grant Likely]

On Wed, Apr 7, 2021 at 8:32 AM Icenowy Zheng <icenowy at aosc.io> wrote:
>
>
>
> 于 2021年4月7日 GMT+08:00 下午3:28:53, Frank Wang <frank.wang at rock-chips.com> 写到:
> >Hi,
> >
> >
> >On 2021/4/7 14:43, Icenowy Zheng wrote:
> >>
> >> 于 2021年4月7日 GMT+08:00 下午2:42:34, Frank Wang
> ><frank.wang at rock-chips.com> 写到:
> >>> Hi Icenowy Zheng,
> >>>
> >>> In my view, it is better to implement this mechanism in phy-uclass
> >>> which
> >>> resemble Linux Kernel have implemented that can avoid do duplication
> >of
> >>>
> >>> work in each SoC's PHY driver.
> >> I'm afraid of breaking existing drivers when implementing it in
> >uclass,
> >> although I agree this will be more clean.
> >
> >Why would it break existing drivers?
>
> >Refer to clk-uclass, the count mechanism was also introduced later from
> >
> >commit "e6849e2fd clk: introduce enable_count".
> >So fix it in framework codes is much better than in each instance
> >drivers just like clk-uclass.
>
> Okay, I will try.

Any progress on this one? I can also confirm that the patch solves
hanging on ExitBootServices() when I'm using a USB flash drive, but it
still hangs with a USB keyboard plugged in.

g.

>
> >
> >BR.
> >Frank
> >
> >>
> >>>
> >>> BR.
> >>> Frank
> >>>
> >>> On 2021/4/6 23:10, 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 at 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;
> >>>>    }
> >>>>
> >>
> >>