From mboxrd@z Thu Jan 1 00:00:00 1970 From: Icenowy Zheng Date: Wed, 07 Apr 2021 15:31:58 +0800 Subject: [PATCH] phy: rockchip: inno-usb2: fix hang when multiple controllers exit In-Reply-To: <1ddbcb84-9055-cb90-7fbc-c7e43972f8d2@rock-chips.com> References: <20210406151059.1187379-1-icenowy@aosc.io> <4e48a1c9-fd01-48c8-fc5d-ebdfb626ee42@rock-chips.com> <1ddbcb84-9055-cb90-7fbc-c7e43972f8d2@rock-chips.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de ? 2021?4?7? GMT+08:00 ??3:28:53, Frank Wang ??: >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 >>>> 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; >>>> } >>>> >> >>