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

[Frank Wang]

Hi,

On 2021/4/7 14:43, Icenowy Zheng wrote:

于 2021年4月7日 GMT+08:00 下午2:42:34, Frank Wang <frank.w...@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.

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 <icen...@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;
   }