[PATCH] phy: rockchip: inno-usb2: fix hang when multiple controllers exit
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; }
On Tue, Apr 6, 2021 at 4:11 PM Icenowy Zheng icenowy@aosc.io 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")
Tested-by: Peter Robinson pbrobinson@gmail.com
Tested on Rock960 and Pinebook Pro with both USB1 and USB2 devices connected and it fixes issues I have seen. Thanks!
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
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.
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@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; }
于 2021年4月7日 GMT+08:00 下午2:42:34, Frank Wang frank.wang@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.
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@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; }
Hi,
On 2021/4/7 14:43, Icenowy Zheng wrote:
于 2021年4月7日 GMT+08:00 下午2:42:34, Frank Wang frank.wang@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 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; }
于 2021年4月7日 GMT+08:00 下午3:28:53, Frank Wang frank.wang@rock-chips.com 写到:
Hi,
On 2021/4/7 14:43, Icenowy Zheng wrote:
于 2021年4月7日 GMT+08:00 下午2:42:34, Frank Wang
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.
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@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; }
On Wed, Apr 7, 2021 at 8:32 AM Icenowy Zheng icenowy@aosc.io wrote:
于 2021年4月7日 GMT+08:00 下午3:28:53, Frank Wang frank.wang@rock-chips.com 写到:
Hi,
On 2021/4/7 14:43, Icenowy Zheng wrote:
于 2021年4月7日 GMT+08:00 下午2:42:34, Frank Wang
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@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;
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.
在 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).
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
participants (6)
-
Frank Wang -
Grant Likely -
Icenowy Zheng -
Peter Robinson -
Simon Glass -
Siva Mahadevan