| Message ID | 20201216165935.9149-1-jmaselbas@kalray.eu |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] usb: dwc2: Try usb_get_phy_by_phandle instead of usb_get_phy | expand |
Hi Jules, On 12/16/2020 20:59, Jules Maselbas wrote: > On probe the dwc2 driver tries two path to get an usb phy, first calling > devm_phy_get() and secondly with devm_usb_get_phy(). > > However the current implementation of devm_phy_get() never return a valid > phy for usb-nop-xceiv. And the current implementation of devm_usb_get_phy > only check for phy that's has already been registered. > > During boot, I see the dwc2 driver being probed before the usb-nop-xceiv > probe, this means that during the dwc2 probe the function devm_usb_get_phy > never finds the a phy (because it hasn't been registered yet) but never > cause the dwc2 probe to defer. > > I tried to follow what is done by dwc3_core_get_phy(): if the current > device has an of_node then try to get the usb_phy by phandle instead of > using devm_usb_get_phy(). This way when the probe order is not good the > devm_usb_get_phy_by_phandle() function will return -EPROBE_DEFER. > > Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu> > --- 8< --- > > A snippet of the device-tree source I am using: > &usb0 { > phys = <&usb_phy0>; > phy-names = "usb2-phy"; > }; > &usb_phy0 { > #phy-cells = <0>; > compatible = "usb-nop-xceiv"; > reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>; > }; > --- > drivers/usb/dwc2/platform.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c > index db9fd4bd1a38..b58ce996add7 100644 > --- a/drivers/usb/dwc2/platform.c > +++ b/drivers/usb/dwc2/platform.c > @@ -251,7 +251,12 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg) > } > > if (!hsotg->phy) { > - hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2); > + if (hsotg->dev->of_node) > + i = of_property_match_string(hsotg->dev->of_node, "phy-names", "usb2-phy"); According the device tree you have provided the value of "i" will always be "0". > + if (hsotg->dev->of_node && i >= 0) > + hsotg->uphy = devm_usb_get_phy_by_phandle(hsotg->dev, "phys", i); Why do you use the value of "i" while in "<&usb_phy0>" you have only one phy. If you had several phy-names and the value of "i" gets more than 0, then based on your usb_phy0 "devm_usb_get_phy_by_phandle" function will return error. So, maybe it would be more correct (based on your device tree), to use below command hsotg->uphy = devm_usb_get_phy_by_phandle(hsotg->dev, "phys", 0); > + else > + hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2); > if (IS_ERR(hsotg->uphy)) { > ret = PTR_ERR(hsotg->uphy); > switch (ret) { > Regards, Artur
Hi Artur, On Fri, Dec 25, 2020 at 11:41:04AM +0000, Artur Petrosyan wrote: > > @@ -251,7 +251,12 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg) > > } > > > > if (!hsotg->phy) { > > - hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2); > > + if (hsotg->dev->of_node) > > + i = of_property_match_string(hsotg->dev->of_node, "phy-names", "usb2-phy"); > > According the device tree you have provided the value of "i" will always > be "0". Yes > > + if (hsotg->dev->of_node && i >= 0) > > + hsotg->uphy = devm_usb_get_phy_by_phandle(hsotg->dev, "phys", i); > > Why do you use the value of "i" while in "<&usb_phy0>" you have only one > phy. If you had several phy-names and the value of "i" gets more than 0, > then based on your usb_phy0 "devm_usb_get_phy_by_phandle" function will > return error. So, maybe it would be more correct (based on your device > tree), to use below command > hsotg->uphy = devm_usb_get_phy_by_phandle(hsotg->dev, "phys", 0); Yes I could use 0 instead of i, but I would like this to work not only for my case where the "usb2-phy" phandle comes first. I've tried to follow what's done in phy-core.c, as done by the function phy_get. Where it first call "of_property_match_string" and then get the phy with the matched index. I don't see how, in my case, the function "devm_usb_get_phy_by_phandle" can be called with i greater than 0, and returning an error. Best, Jules
Hi Jules, On 12/26/2020 17:45, Jules Maselbas wrote: > Hi Artur, > > On Fri, Dec 25, 2020 at 11:41:04AM +0000, Artur Petrosyan wrote: >>> @@ -251,7 +251,12 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg) >>> } >>> >>> if (!hsotg->phy) { >>> - hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2); >>> + if (hsotg->dev->of_node) >>> + i = of_property_match_string(hsotg->dev->of_node, "phy-names", "usb2-phy"); >> >> According the device tree you have provided the value of "i" will always >> be "0". > Yes > >>> + if (hsotg->dev->of_node && i >= 0) >>> + hsotg->uphy = devm_usb_get_phy_by_phandle(hsotg->dev, "phys", i); >> >> Why do you use the value of "i" while in "<&usb_phy0>" you have only one >> phy. If you had several phy-names and the value of "i" gets more than 0, >> then based on your usb_phy0 "devm_usb_get_phy_by_phandle" function will >> return error. So, maybe it would be more correct (based on your device >> tree), to use below command >> hsotg->uphy = devm_usb_get_phy_by_phandle(hsotg->dev, "phys", 0); > Yes I could use 0 instead of i, but I would like this to work not only > for my case where the "usb2-phy" phandle comes first. > > I've tried to follow what's done in phy-core.c, as done by the function > phy_get. Where it first call "of_property_match_string" and then get the > phy with the matched index. > > I don't see how, in my case, the function "devm_usb_get_phy_by_phandle" can > be called with i greater than 0, and returning an error. I have noticed that there might be no need to check for "i >= 0" at all because "of_parse_phandle()" function has a check for index to not be less then 0. > > Best, > Jules > Regards, Artur
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index db9fd4bd1a38..b58ce996add7 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -251,7 +251,12 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg) } if (!hsotg->phy) { - hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2); + if (hsotg->dev->of_node) + i = of_property_match_string(hsotg->dev->of_node, "phy-names", "usb2-phy"); + if (hsotg->dev->of_node && i >= 0) + hsotg->uphy = devm_usb_get_phy_by_phandle(hsotg->dev, "phys", i); + else + hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2); if (IS_ERR(hsotg->uphy)) { ret = PTR_ERR(hsotg->uphy); switch (ret) {
On probe the dwc2 driver tries two path to get an usb phy, first calling devm_phy_get() and secondly with devm_usb_get_phy(). However the current implementation of devm_phy_get() never return a valid phy for usb-nop-xceiv. And the current implementation of devm_usb_get_phy only check for phy that's has already been registered. During boot, I see the dwc2 driver being probed before the usb-nop-xceiv probe, this means that during the dwc2 probe the function devm_usb_get_phy never finds the a phy (because it hasn't been registered yet) but never cause the dwc2 probe to defer. I tried to follow what is done by dwc3_core_get_phy(): if the current device has an of_node then try to get the usb_phy by phandle instead of using devm_usb_get_phy(). This way when the probe order is not good the devm_usb_get_phy_by_phandle() function will return -EPROBE_DEFER. Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu> --- 8< --- A snippet of the device-tree source I am using: &usb0 { phys = <&usb_phy0>; phy-names = "usb2-phy"; }; &usb_phy0 { #phy-cells = <0>; compatible = "usb-nop-xceiv"; reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>; }; --- drivers/usb/dwc2/platform.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)