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

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