On Wed, Apr 7, 2021 at 8:32 AM Icenowy Zheng <icen...@aosc.io> wrote: > > > > 于 2021年4月7日 GMT+08:00 下午3:28:53, Frank Wang <frank.w...@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.w...@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 <icen...@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; > >>>> } > >>>> > >> > >>