| Message ID | 20231103164323.14294-1-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 16b7e0cccb243033de4406ffb4d892365041a1e7 |
| Headers | show |
| Series | USB: xhci-plat: fix legacy PHY double init | expand |
On Fri, Nov 03, 2023 at 05:43:23PM +0100, Johan Hovold wrote: > Commits 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") and > 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") added support > for looking up legacy PHYs from the sysdev devicetree node and > initialising them. > > This broke drivers such as dwc3 which manages PHYs themself as the PHYs > would now be initialised twice, something which specifically can lead to > resources being left enabled during suspend (e.g. with the > usb_phy_generic PHY driver). > > As the dwc3 driver uses driver-name matching for the xhci platform > device, fix this by only looking up and initialising PHYs for devices > that have been matched using OF. > > Note that checking that the platform device has a devicetree node would > currently be sufficient, but that could lead to subtle breakages in case > anyone ever tries to reuse an ancestor's node. > > Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") > Fixes: 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") > Cc: stable@vger.kernel.org # 4.1 > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Stanley Chang <stanley_chang@realtek.com> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> Tested-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> On Fri, Nov 03, 2023 at 05:43:23PM +0100, Johan Hovold wrote: > > Commits 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") and > > 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") added > > support for looking up legacy PHYs from the sysdev devicetree node and > > initialising them. > > > > This broke drivers such as dwc3 which manages PHYs themself as the > > PHYs would now be initialised twice, something which specifically can > > lead to resources being left enabled during suspend (e.g. with the > > usb_phy_generic PHY driver). > > > > As the dwc3 driver uses driver-name matching for the xhci platform > > device, fix this by only looking up and initialising PHYs for devices > > that have been matched using OF. > > > > Note that checking that the platform device has a devicetree node > > would currently be sufficient, but that could lead to subtle breakages > > in case anyone ever tries to reuse an ancestor's node. > > > > Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") > > Fixes: 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") > > Cc: stable@vger.kernel.org # 4.1 > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Stanley Chang <stanley_chang@realtek.com> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > Tested-by: Stefan Eichenberger <stefan.eichenberger@toradex.com> Tested-by: Stanley Chang <stanley_chang@realtek.com>
Hi Johan, > > On Fri, Nov 03, 2023 at 05:43:23PM +0100, Johan Hovold wrote: > > > Commits 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") and > > > 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") added > > > support for looking up legacy PHYs from the sysdev devicetree node > > > and initialising them. > > > > > > This broke drivers such as dwc3 which manages PHYs themself as the > > > PHYs would now be initialised twice, something which specifically > > > can lead to resources being left enabled during suspend (e.g. with > > > the usb_phy_generic PHY driver). > > > > > > As the dwc3 driver uses driver-name matching for the xhci platform > > > device, fix this by only looking up and initialising PHYs for > > > devices that have been matched using OF. > > > > > > Note that checking that the platform device has a devicetree node > > > would currently be sufficient, but that could lead to subtle > > > breakages in case anyone ever tries to reuse an ancestor's node. > > > > > > Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") > > > Fixes: 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") > > > Cc: stable@vger.kernel.org # 4.1 > > > Cc: Maxime Ripard <mripard@kernel.org> > > > Cc: Stanley Chang <stanley_chang@realtek.com> > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > > > Tested-by: Stefan Eichenberger <stefan.eichenberger@toradex.com> > > Tested-by: Stanley Chang <stanley_chang@realtek.com> > I am sorry to notify you this patch is tested fail. I test the Realtek phy driver at drivers/phy/Realtek/phy-rtk-usb2.c again. But I can't get the phy in xhci. It is a dwc3 generic phy driver, and it is also a usb phy driver. Base on you modified, I can't run on callback rtk_phy->phy.notify_port_status = rtk_phy_notify_port_status; Thanks, Stanley.
On Mon, Nov 06, 2023 at 06:53:23AM +0000, Stanley Chang[昌育德] wrote: > > > On Fri, Nov 03, 2023 at 05:43:23PM +0100, Johan Hovold wrote: > > > > Commits 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") and > > > > 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") added > > > > support for looking up legacy PHYs from the sysdev devicetree node > > > > and initialising them. > > > > > > > > This broke drivers such as dwc3 which manages PHYs themself as the > > > > PHYs would now be initialised twice, something which specifically > > > > can lead to resources being left enabled during suspend (e.g. with > > > > the usb_phy_generic PHY driver). > > > > > > > > As the dwc3 driver uses driver-name matching for the xhci platform > > > > device, fix this by only looking up and initialising PHYs for > > > > devices that have been matched using OF. > > Tested-by: Stanley Chang <stanley_chang@realtek.com> > I am sorry to notify you this patch is tested fail. Hmm. Thanks for testing. > I test the Realtek phy driver at drivers/phy/Realtek/phy-rtk-usb2.c again. > But I can't get the phy in xhci. > It is a dwc3 generic phy driver, and it is also a usb phy driver. That sounds broken (i.e. to be relying on both frameworks), but indeed that seems to be the current state of the generic and legacy USB PHY implementations. What a mess. > Base on you modified, I can't run on callback > rtk_phy->phy.notify_port_status = rtk_phy_notify_port_status; Which dwc3 driver are you using? Unless I'm missing something this would not be an issue unless you are doing something crazy like describing the same PHY twice in the devicetree (i.e. both as a generic and legacy PHY). Apparently, there are no in-tree users of this particular realtek PHY so I can't check the devicetree, but we do have other instances of such abuse since at least a decade: 6747caa76cab ("usb: phy: twl4030: use the new generic PHY framework") And, yes, then this is sort of expected. The dwc3 driver has always managed its own PHYs, but functionality has now been bolted on top so that people may have started relying on it being managed *also* by xhci, well at least for notifications like the one you just added: a08799cf17c2 ("usb: phy: add usb phy notify port status API") Johan
Hi Johan, > > On Mon, Nov 06, 2023 at 06:53:23AM +0000, Stanley Chang[昌育德] wrote: > > > > On Fri, Nov 03, 2023 at 05:43:23PM +0100, Johan Hovold wrote: > > > > > Commits 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") > > > > > and > > > > > 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") added > > > > > support for looking up legacy PHYs from the sysdev devicetree > > > > > node and initialising them. > > > > > > > > > > This broke drivers such as dwc3 which manages PHYs themself as > > > > > the PHYs would now be initialised twice, something which > > > > > specifically can lead to resources being left enabled during > > > > > suspend (e.g. with the usb_phy_generic PHY driver). > > > > > > > > > > As the dwc3 driver uses driver-name matching for the xhci > > > > > platform device, fix this by only looking up and initialising > > > > > PHYs for devices that have been matched using OF. > > > > Tested-by: Stanley Chang <stanley_chang@realtek.com> > > > I am sorry to notify you this patch is tested fail. > > Hmm. Thanks for testing. > > > I test the Realtek phy driver at drivers/phy/Realtek/phy-rtk-usb2.c again. > > But I can't get the phy in xhci. > > > It is a dwc3 generic phy driver, and it is also a usb phy driver. > > That sounds broken (i.e. to be relying on both frameworks), but indeed that > seems to be the current state of the generic and legacy USB PHY > implementations. > > What a mess. > > Base on you modified, I can't run on callback > > rtk_phy->phy.notify_port_status = rtk_phy_notify_port_status; > > Which dwc3 driver are you using? Unless I'm missing something this would not > be an issue unless you are doing something crazy like describing the same PHY > twice in the devicetree (i.e. both as a generic and legacy PHY). I use drivers/usb/dwc3/core.c and drivers/usb/dwc3/dwc3-rtk.c I describe the PHY as generic and legacy PHY in device tree. Our driver needs the API base on a08799cf17c2 ("usb: phy: add usb phy notify port status API"). But generic PHY driver is not support this. Thanks, Stanley > Apparently, there are no in-tree users of this particular realtek PHY so I can't > check the devicetree, but we do have other instances of such abuse since at > least a decade: > > 6747caa76cab ("usb: phy: twl4030: use the new generic PHY > framework") > > And, yes, then this is sort of expected. The dwc3 driver has always managed its > own PHYs, but functionality has now been bolted on top so that people may > have started relying on it being managed *also* by xhci, well at least for > notifications like the one you just added: > > a08799cf17c2 ("usb: phy: add usb phy notify port status API") > > Johan
On Mon, Nov 06, 2023 at 10:08:24AM +0000, Stanley Chang[昌育德] wrote: > > On Mon, Nov 06, 2023 at 06:53:23AM +0000, Stanley Chang[昌育德] wrote: > > > I test the Realtek phy driver at drivers/phy/Realtek/phy-rtk-usb2.c again. > > > But I can't get the phy in xhci. > > > > > It is a dwc3 generic phy driver, and it is also a usb phy driver. > > > > That sounds broken (i.e. to be relying on both frameworks), but indeed that > > seems to be the current state of the generic and legacy USB PHY > > implementations. > > > > What a mess. > > > > Base on you modified, I can't run on callback > > > rtk_phy->phy.notify_port_status = rtk_phy_notify_port_status; > > > > Which dwc3 driver are you using? Unless I'm missing something this would not > > be an issue unless you are doing something crazy like describing the same PHY > > twice in the devicetree (i.e. both as a generic and legacy PHY). > > I use drivers/usb/dwc3/core.c and drivers/usb/dwc3/dwc3-rtk.c > I describe the PHY as generic and legacy PHY in device tree. That's not right. You should just use the generic PHY binding for new platforms. > Our driver needs the API base on a08799cf17c2 ("usb: phy: add usb phy notify port status API"). > But generic PHY driver is not support this. Yes, but you added that interface yourself, and that I think merging that was a mistake. We should not be building functionality on top of the legacy USB PHY implementation which is stuck in some transitional limbo. Apparently, your PHY drivers which were merged for 6.6 are the only users of this interface, and there are no upstream devicetrees that use these PHYs. I think we should revert this mess before we dig ourselves into an even deeper hole. Johan
Hi Johan, > > On Mon, Nov 06, 2023 at 10:08:24AM +0000, Stanley Chang[昌育德] wrote: > > > On Mon, Nov 06, 2023 at 06:53:23AM +0000, Stanley Chang[昌育德] > wrote: > > > > > I test the Realtek phy driver at drivers/phy/Realtek/phy-rtk-usb2.c again. > > > > But I can't get the phy in xhci. > > > > > > > It is a dwc3 generic phy driver, and it is also a usb phy driver. > > > > > > That sounds broken (i.e. to be relying on both frameworks), but > > > indeed that seems to be the current state of the generic and legacy > > > USB PHY implementations. > > > > > > What a mess. > > > > > > Base on you modified, I can't run on callback > > > > rtk_phy->phy.notify_port_status = rtk_phy_notify_port_status; > > > > > > Which dwc3 driver are you using? Unless I'm missing something this > > > would not be an issue unless you are doing something crazy like > > > describing the same PHY twice in the devicetree (i.e. both as a generic and > legacy PHY). > > > > I use drivers/usb/dwc3/core.c and drivers/usb/dwc3/dwc3-rtk.c I > > describe the PHY as generic and legacy PHY in device tree. > > That's not right. You should just use the generic PHY binding for new platforms. > > > Our driver needs the API base on a08799cf17c2 ("usb: phy: add usb phy > notify port status API"). > > But generic PHY driver is not support this. > > Yes, but you added that interface yourself, and that I think merging that was a > mistake. > > We should not be building functionality on top of the legacy USB PHY > implementation which is stuck in some transitional limbo. > > Apparently, your PHY drivers which were merged for 6.6 are the only users of > this interface, and there are no upstream devicetrees that use these PHYs. > > I think we should revert this mess before we dig ourselves into an even deeper > hole. > This is an interim method, as the current generic PHY framework does not support special operations on USB PHY. Now the generic PHY can't instead USB PHY in this stage. For example, drivers/phy/ti/phy-twl4030-usb.c drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c drivers/phy/ti/phy-omap-usb2.c Thanks, Stanley
On Mon, Nov 06, 2023 at 10:37:06AM +0000, Stanley Chang[昌育德] wrote: > > > I use drivers/usb/dwc3/core.c and drivers/usb/dwc3/dwc3-rtk.c I > > > describe the PHY as generic and legacy PHY in device tree. > > > > That's not right. You should just use the generic PHY binding for new platforms. > > > > > Our driver needs the API base on a08799cf17c2 ("usb: phy: add usb phy > > notify port status API"). > > > But generic PHY driver is not support this. > > > > Yes, but you added that interface yourself, and that I think merging that was a > > mistake. > > > > We should not be building functionality on top of the legacy USB PHY > > implementation which is stuck in some transitional limbo. > > > > Apparently, your PHY drivers which were merged for 6.6 are the only users of > > this interface, and there are no upstream devicetrees that use these PHYs. > > > > I think we should revert this mess before we dig ourselves into an even deeper > > hole. > > This is an interim method, as the current generic PHY framework does > not support special operations on USB PHY. Then you need to add that. You can't add a new interface which is broken by design and can't be used unless you abuse the devicetree and describe your PHYs using *both* the generic 'phy' property and the *deprecated* 'usb-phy' property. That's just broken. > Now the generic PHY can't instead USB PHY in this stage. > For example, > drivers/phy/ti/phy-twl4030-usb.c > drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c > drivers/phy/ti/phy-omap-usb2.c These should be fixed as well eventually. Johan
Hi Johan, > > On Mon, Nov 06, 2023 at 10:37:06AM +0000, Stanley Chang[昌育德] wrote: > > > > > I use drivers/usb/dwc3/core.c and drivers/usb/dwc3/dwc3-rtk.c I > > > > describe the PHY as generic and legacy PHY in device tree. > > > > > > That's not right. You should just use the generic PHY binding for new > platforms. > > > > > > > Our driver needs the API base on a08799cf17c2 ("usb: phy: add usb > > > > phy > > > notify port status API"). > > > > But generic PHY driver is not support this. > > > > > > Yes, but you added that interface yourself, and that I think merging > > > that was a mistake. > > > > > > We should not be building functionality on top of the legacy USB PHY > > > implementation which is stuck in some transitional limbo. > > > > > > Apparently, your PHY drivers which were merged for 6.6 are the only > > > users of this interface, and there are no upstream devicetrees that use > these PHYs. > > > > > > I think we should revert this mess before we dig ourselves into an > > > even deeper hole. > > > > This is an interim method, as the current generic PHY framework does > > not support special operations on USB PHY. > > Then you need to add that. > > You can't add a new interface which is broken by design and can't be used > unless you abuse the devicetree and describe your PHYs using *both* the > generic 'phy' property and the *deprecated* 'usb-phy' property. > > That's just broken. I will modify the Realtek phy to solve this problem and just use the generic PHY. I don't think this patch on a08799cf17c2 ("usb:phy: New usb phy notification port status API") needs to be reverted. I will submit fixes based on these patches. Thanks, Stanley > > Now the generic PHY can't instead USB PHY in this stage. > > For example, > > drivers/phy/ti/phy-twl4030-usb.c > > drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c > > drivers/phy/ti/phy-omap-usb2.c > > These should be fixed as well eventually. > > Johan
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 28218c8f1837..01d19d17153b 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -13,6 +13,7 @@ #include <linux/module.h> #include <linux/pci.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/usb/phy.h> #include <linux/slab.h> @@ -148,7 +149,7 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s int ret; int irq; struct xhci_plat_priv *priv = NULL; - + bool of_match; if (usb_disabled()) return -ENODEV; @@ -253,16 +254,23 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s &xhci->imod_interval); } - hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0); - if (IS_ERR(hcd->usb_phy)) { - ret = PTR_ERR(hcd->usb_phy); - if (ret == -EPROBE_DEFER) - goto disable_clk; - hcd->usb_phy = NULL; - } else { - ret = usb_phy_init(hcd->usb_phy); - if (ret) - goto disable_clk; + /* + * Drivers such as dwc3 manages PHYs themself (and rely on driver name + * matching for the xhci platform device). + */ + of_match = of_match_device(pdev->dev.driver->of_match_table, &pdev->dev); + if (of_match) { + hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0); + if (IS_ERR(hcd->usb_phy)) { + ret = PTR_ERR(hcd->usb_phy); + if (ret == -EPROBE_DEFER) + goto disable_clk; + hcd->usb_phy = NULL; + } else { + ret = usb_phy_init(hcd->usb_phy); + if (ret) + goto disable_clk; + } } hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node); @@ -285,15 +293,17 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s goto dealloc_usb2_hcd; } - xhci->shared_hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, - "usb-phy", 1); - if (IS_ERR(xhci->shared_hcd->usb_phy)) { - xhci->shared_hcd->usb_phy = NULL; - } else { - ret = usb_phy_init(xhci->shared_hcd->usb_phy); - if (ret) - dev_err(sysdev, "%s init usb3phy fail (ret=%d)\n", - __func__, ret); + if (of_match) { + xhci->shared_hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, + "usb-phy", 1); + if (IS_ERR(xhci->shared_hcd->usb_phy)) { + xhci->shared_hcd->usb_phy = NULL; + } else { + ret = usb_phy_init(xhci->shared_hcd->usb_phy); + if (ret) + dev_err(sysdev, "%s init usb3phy fail (ret=%d)\n", + __func__, ret); + } } xhci->shared_hcd->tpl_support = hcd->tpl_support;
Commits 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") and 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") added support for looking up legacy PHYs from the sysdev devicetree node and initialising them. This broke drivers such as dwc3 which manages PHYs themself as the PHYs would now be initialised twice, something which specifically can lead to resources being left enabled during suspend (e.g. with the usb_phy_generic PHY driver). As the dwc3 driver uses driver-name matching for the xhci platform device, fix this by only looking up and initialising PHYs for devices that have been matched using OF. Note that checking that the platform device has a devicetree node would currently be sufficient, but that could lead to subtle breakages in case anyone ever tries to reuse an ancestor's node. Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") Fixes: 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") Cc: stable@vger.kernel.org # 4.1 Cc: Maxime Ripard <mripard@kernel.org> Cc: Stanley Chang <stanley_chang@realtek.com> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/usb/host/xhci-plat.c | 50 +++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 20 deletions(-)