[PATCH] usb: cdns3: continue probe even when USB PHY device does not exist
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) { 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 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?
participants (2)
-
Roger Quadros -
Siddharth Vadapalli