Message ID | 20210406151059.1187379-1-icenowy@aosc.io |
---|---|
State | Changes Requested |
Delegated to: | Kever Yang |
Headers | show |
Series | phy: rockchip: inno-usb2: fix hang when multiple controllers exit | expand |
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 ><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; >>>> } >>>> >> >>
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; > >>>> } > >>>> > >> > >>
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
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; }
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(+)