[PATCH] usb: cdns3: continue probe even when USB PHY device does not exist

[Roger Quadros]

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 at 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?

-- 
cheers,
-roger