[PATCH 3/5] usb: dwc3-generic: Relax unsupported dr_mode check

[Jonas Karlman]

On 2023-07-13 12:08, Marek Vasut wrote:
> On 7/13/23 11:56, Jonas Karlman wrote:
>> Hi Marek,
>>
>> Sorry for late reply.
>> On 2023-06-05 12:14, Marek Vasut wrote:
>>> On 5/30/23 12:26, Jonas Karlman wrote:
>>>> When dr_mode is peripheral or otg and U-Boot has not been built with
>>>> DM_USB_GADGET support, booting such device may end up with:
>>>>
>>>>     dwc3_glue_bind_common: subnode name: usb at fcc00000
>>>>     Error binding driver 'dwc3-generic-wrapper': -6
>>>>     Some drivers failed to bind
>>>>     initcall sequence 00000000effbca08 failed at call 0000000000a217c8 (err=-6)
>>>>     ### ERROR ### Please RESET the board ###
>>>>
>>>> Instead fail gracfully with ENODEV to allow board continue booting.
>>>>
>>>>     dwc3_glue_bind_common: subnode name: usb at fcc00000
>>>>     dwc3_glue_bind_common: unsupported dr_mode
>>>>
>>>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>>>> ---
>>>>    drivers/usb/dwc3/dwc3-generic.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
>>>> index c28ad47bddd8..f7859a530280 100644
>>>> --- a/drivers/usb/dwc3/dwc3-generic.c
>>>> +++ b/drivers/usb/dwc3/dwc3-generic.c
>>>> @@ -422,13 +422,13 @@ static int dwc3_glue_bind_common(struct udevice *parent, ofnode node)
>>>>    		dr_mode = usb_get_dr_mode(node);
>>>>    
>>>>    	switch (dr_mode) {
>>>> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
>>>>    	case USB_DR_MODE_PERIPHERAL:
>>>>    	case USB_DR_MODE_OTG:
>>>> -#if CONFIG_IS_ENABLED(DM_USB_GADGET)
>>>
>>> Why not just do
>>>
>>> #else
>>> return -ENODEV
>>> #endif
>>>
>>> here ?
>>
>> The code was changed to closer match how host mode is already handled,
>> and the default switch case already return -ENODEV.
> 
> Let's try the above and see if that makes the code more readable.
> 
> Also, you might want to try
> 
> if (!CONFIG_IS_ENABLED(DM_USB_GADGET))
> 	return -ENODEV;
> 
> To improve build coverage.

In my opinion this (after this patch as-is) is more readable and has the
benefit of the debug message when DM_USB_GADGET is disabled.


	switch (dr_mode) {
#if CONFIG_IS_ENABLED(DM_USB_GADGET)
	case USB_DR_MODE_PERIPHERAL:
	case USB_DR_MODE_OTG:
		debug("%s: dr_mode: OTG or Peripheral\n", __func__);
		driver = "dwc3-generic-peripheral";
		break;
#endif
#if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
	case USB_DR_MODE_HOST:
		debug("%s: dr_mode: HOST\n", __func__);
		driver = "dwc3-generic-host";
		break;
#endif
	default:
		debug("%s: unsupported dr_mode\n", __func__);
		return -ENODEV;
	};


Compared to the following:


	switch (dr_mode) {
	case USB_DR_MODE_PERIPHERAL:
	case USB_DR_MODE_OTG:
		if (!CONFIG_IS_ENABLED(DM_USB_GADGET))
			return -ENODEV;

		debug("%s: dr_mode: OTG or Peripheral\n", __func__);
		driver = "dwc3-generic-peripheral";
		break;
#if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
	case USB_DR_MODE_HOST:
		debug("%s: dr_mode: HOST\n", __func__);
		driver = "dwc3-generic-host";
		break;
#endif
	default:
		debug("%s: unsupported dr_mode\n", __func__);
		return -ENODEV;
	};


Any suggestion on how this could otherwise be refactored?

The original intent with the patch is to fix a non-booting
config to a booting config with minimal code changes :-)

Regards,
Jonas