| Message ID | 20240702120724.368102-1-s-vadapalli@ti.com |
|---|---|
| State | New |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | usb: cdns3: continue probe even when USB PHY device does not exist | expand |
On 02/07/2024 15:07, Siddharth Vadapalli wrote: > Prior to commit cd295286c786 ("usb: cdns3: avoid error messages if phys > don't exist"), cdns3_probe() errors out only on failing to initialize the > USB2/USB3 PHY. However, since commit cd295286c786, absence of the PHY > device is also treated as an error, resulting in a regression. > > Extend commit cd295286c786 to treat -ENODEV as an acceptable return value > of generic_phy_get_by_name() and continue device probe as was the case > prior to the commit. > > Fixes: cd295286c786 ("usb: cdns3: avoid error messages if phys don't exist") > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > > Hello, > > This patch is based on commit > b4cbd1a257 Merge tag 'u-boot-amlogic-20240701' of https://source.denx.de/u-boot/custodians/u-boot-amlogic into next > of the next branch of U-Boot. > > Regards, > Siddharth. > > drivers/usb/cdns3/core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c > index b4e931646b..5b3e32953e 100644 > --- a/drivers/usb/cdns3/core.c > +++ b/drivers/usb/cdns3/core.c > @@ -338,7 +338,7 @@ static int cdns3_probe(struct cdns3 *cdns) > dev_err(dev, "USB2 PHY init failed: %d\n", ret); > return ret; > } > - } else if (ret != -ENOENT && ret != -ENODATA) { > + } else if (ret != -ENOENT && ret != -ENODATA && ret != -ENODEV) { With this change we will not error out on a genuine error condition that produces ENODEV. If PHY phandle is not present the API should return ENOENT right? static int __of_parse_phandle_with_args(const struct device_node *np, ... { ... /* Retrieve the phandle list property */ list = of_get_property(np, list_name, &size); if (!list) return -ENOENT; Can you please check and point where the -ENODEV error is coming from? > dev_err(dev, "Couldn't get USB2 PHY: %d\n", ret); > return ret; > } > @@ -350,7 +350,7 @@ static int cdns3_probe(struct cdns3 *cdns) > dev_err(dev, "USB3 PHY init failed: %d\n", ret); > return ret; > } > - } else if (ret != -ENOENT && ret != -ENODATA) { > + } else if (ret != -ENOENT && ret != -ENODATA && ret != -ENODEV) { > dev_err(dev, "Couldn't get USB3 PHY: %d\n", ret); > return ret; > }
On Tue, Jul 02, 2024 at 04:20:43PM +0300, Roger Quadros wrote: > > > On 02/07/2024 15:07, Siddharth Vadapalli wrote: > > Prior to commit cd295286c786 ("usb: cdns3: avoid error messages if phys > > don't exist"), cdns3_probe() errors out only on failing to initialize the > > USB2/USB3 PHY. However, since commit cd295286c786, absence of the PHY > > device is also treated as an error, resulting in a regression. > > > > Extend commit cd295286c786 to treat -ENODEV as an acceptable return value > > of generic_phy_get_by_name() and continue device probe as was the case > > prior to the commit. > > > > Fixes: cd295286c786 ("usb: cdns3: avoid error messages if phys don't exist") > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > --- > > > > Hello, > > > > This patch is based on commit > > b4cbd1a257 Merge tag 'u-boot-amlogic-20240701' of https://source.denx.de/u-boot/custodians/u-boot-amlogic into next > > of the next branch of U-Boot. > > > > Regards, > > Siddharth. > > > > drivers/usb/cdns3/core.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c > > index b4e931646b..5b3e32953e 100644 > > --- a/drivers/usb/cdns3/core.c > > +++ b/drivers/usb/cdns3/core.c > > @@ -338,7 +338,7 @@ static int cdns3_probe(struct cdns3 *cdns) > > dev_err(dev, "USB2 PHY init failed: %d\n", ret); > > return ret; > > } > > - } else if (ret != -ENOENT && ret != -ENODATA) { > > + } else if (ret != -ENOENT && ret != -ENODATA && ret != -ENODEV) { > > With this change we will not error out on a genuine error condition > that produces ENODEV. It isn't necessarily a genuine error condition which is why it was a "dev_warn" earlier for any error. If the previous stage has already configured the PHY, or if the PHY present in the device-tree in Linux is not the same as the PHY being used at U-Boot (USB 2 PHY at U-Boot vs SERDES in Linux), then it isn't an error. > > If PHY phandle is not present the API should return ENOENT right? > > static int __of_parse_phandle_with_args(const struct device_node *np, > > /* Retrieve the phandle list property */ > list = of_get_property(np, list_name, &size); > if (!list) > return -ENOENT; The PHY phandle is present, but it isn't the one being used by U-Boot. The device-tree could be pointing to SERDES as the PHY, since Linux uses USB with SERDES. So the entry exists, but the error is -ENODEV rather than -ENOENT. > > Can you please check and point where the -ENODEV error is coming from? The sequence of function calls is as follows: generic_phy_get_by_name generic_phy_get_by_index generic_phy_get_by_index_nodev uclass_get_device_by_ofnode uclass_find_device_by_ofnode -ENODEV In the above sequence, the device-tree contains SERDES PHY as the USB PHY since Linux uses the same and U-Boot's device-tree is in sync with Linux's. However, USB at U-Boot will use the USB 2 PHY. So one option is to remove the SERDES PHY from USB node to have it fallback to USB 2 PHY. At the same time, if the previous stage has configured SERDES for example, it might not be necessary to reconfigure SERDES. -ENODEV might be an acceptable error in such a situation as well. Please let me know. [...] Regards, Siddharth.
On 02/07/2024 16:36, Siddharth Vadapalli wrote: > On Tue, Jul 02, 2024 at 04:20:43PM +0300, Roger Quadros wrote: >> >> >> On 02/07/2024 15:07, Siddharth Vadapalli wrote: >>> Prior to commit cd295286c786 ("usb: cdns3: avoid error messages if phys >>> don't exist"), cdns3_probe() errors out only on failing to initialize the >>> USB2/USB3 PHY. However, since commit cd295286c786, absence of the PHY >>> device is also treated as an error, resulting in a regression. >>> >>> Extend commit cd295286c786 to treat -ENODEV as an acceptable return value >>> of generic_phy_get_by_name() and continue device probe as was the case >>> prior to the commit. >>> >>> Fixes: cd295286c786 ("usb: cdns3: avoid error messages if phys don't exist") >>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>> --- >>> >>> Hello, >>> >>> This patch is based on commit >>> b4cbd1a257 Merge tag 'u-boot-amlogic-20240701' of https://source.denx.de/u-boot/custodians/u-boot-amlogic into next >>> of the next branch of U-Boot. >>> >>> Regards, >>> Siddharth. >>> >>> drivers/usb/cdns3/core.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c >>> index b4e931646b..5b3e32953e 100644 >>> --- a/drivers/usb/cdns3/core.c >>> +++ b/drivers/usb/cdns3/core.c >>> @@ -338,7 +338,7 @@ static int cdns3_probe(struct cdns3 *cdns) >>> dev_err(dev, "USB2 PHY init failed: %d\n", ret); >>> return ret; >>> } >>> - } else if (ret != -ENOENT && ret != -ENODATA) { >>> + } else if (ret != -ENOENT && ret != -ENODATA && ret != -ENODEV) { >> >> With this change we will not error out on a genuine error condition >> that produces ENODEV. > > It isn't necessarily a genuine error condition which is why it was a > "dev_warn" earlier for any error. If the previous stage has already Earlier it was clearly wrong to warn for everything. > configured the PHY, or if the PHY present in the device-tree in Linux is > not the same as the PHY being used at U-Boot (USB 2 PHY at U-Boot vs > SERDES in Linux), then it isn't an error. > >> >> If PHY phandle is not present the API should return ENOENT right? >> >> static int __of_parse_phandle_with_args(const struct device_node *np, >> >> /* Retrieve the phandle list property */ >> list = of_get_property(np, list_name, &size); >> if (!list) >> return -ENOENT; > > The PHY phandle is present, but it isn't the one being used by U-Boot. OK. commit cd295286c786 was only addressing the case if USB PHY node is not present (-ENOENT case). So there is no regression there right? > The device-tree could be pointing to SERDES as the PHY, since Linux uses > USB with SERDES. So the entry exists, but the error is -ENODEV rather > than -ENOENT. If the device tree contains the PHY then it should be initialized and any error initializing it is an error condition we cannot ignore. > >> >> Can you please check and point where the -ENODEV error is coming from? > > The sequence of function calls is as follows: > generic_phy_get_by_name > generic_phy_get_by_index > generic_phy_get_by_index_nodev > uclass_get_device_by_ofnode > uclass_find_device_by_ofnode > -ENODEV uclass_find_device_by_ofnode() ... ret = uclass_get(id, &uc); if (ret) return ret; uclass_foreach_dev(dev, uc) { log(LOGC_DM, LOGL_DEBUG_CONTENT, " - checking %s\n", dev->name); if (ofnode_equal(dev_ofnode(dev), node)) { *devp = dev; goto done; } } ret = -ENODEV; This means the class driver was not registered yet? Do you know why that might be the case? Was the SERDES PHY driver enabled? Are there any error there? > > In the above sequence, the device-tree contains SERDES PHY as the USB > PHY since Linux uses the same and U-Boot's device-tree is in sync with > Linux's. However, USB at U-Boot will use the USB 2 PHY. So one option is > to remove the SERDES PHY from USB node to have it fallback to USB 2 PHY. Ideally we would want u-boot to behave like Linux. If USB3 can be supported it should be made to work on u-boot as well. Any reason why USB3 cannot work on u-boot? > At the same time, if the previous stage has configured SERDES for example, > it might not be necessary to reconfigure SERDES. -ENODEV might be an > acceptable error in such a situation as well. Please let me know. Let's not assume error codes can be acceptable. There is patch on Linux to not re-initialize SERDES if it was already configured by previous stage. Maybe we could use something similar on u-boot?
diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c index b4e931646b..5b3e32953e 100644 --- a/drivers/usb/cdns3/core.c +++ b/drivers/usb/cdns3/core.c @@ -338,7 +338,7 @@ static int cdns3_probe(struct cdns3 *cdns) dev_err(dev, "USB2 PHY init failed: %d\n", ret); return ret; } - } else if (ret != -ENOENT && ret != -ENODATA) { + } else if (ret != -ENOENT && ret != -ENODATA && ret != -ENODEV) { dev_err(dev, "Couldn't get USB2 PHY: %d\n", ret); return ret; } @@ -350,7 +350,7 @@ static int cdns3_probe(struct cdns3 *cdns) dev_err(dev, "USB3 PHY init failed: %d\n", ret); return ret; } - } else if (ret != -ENOENT && ret != -ENODATA) { + } else if (ret != -ENOENT && ret != -ENODATA && ret != -ENODEV) { dev_err(dev, "Couldn't get USB3 PHY: %d\n", ret); return ret; }
Prior to commit cd295286c786 ("usb: cdns3: avoid error messages if phys don't exist"), cdns3_probe() errors out only on failing to initialize the USB2/USB3 PHY. However, since commit cd295286c786, absence of the PHY device is also treated as an error, resulting in a regression. Extend commit cd295286c786 to treat -ENODEV as an acceptable return value of generic_phy_get_by_name() and continue device probe as was the case prior to the commit. Fixes: cd295286c786 ("usb: cdns3: avoid error messages if phys don't exist") Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> --- Hello, This patch is based on commit b4cbd1a257 Merge tag 'u-boot-amlogic-20240701' of https://source.denx.de/u-boot/custodians/u-boot-amlogic into next of the next branch of U-Boot. Regards, Siddharth. drivers/usb/cdns3/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)