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

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