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

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

[Icenowy Zheng]

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

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

[Peter Robinson]

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

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

[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.


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;
>   }
>   

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

[Icenowy Zheng]

? 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;
>>   }
>>   

[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.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;
>>>    }
>>>    
>
>

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

[Icenowy Zheng]

? 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
><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.

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;
>>>>    }
>>>>    
>>
>>

Re: [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@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
> ><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.
>
> 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;
> >>>>    }
> >>>>
> >>
> >>

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

[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.

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

[Icenowy Zheng]

在 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).

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

[Simon Glass]

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