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

[Marek Vasut]

On 7/13/23 12:25, Jonas Karlman wrote:
> 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;

Maybe drop the switch-case and try:

if (CONFIG_IS_ENABLED(DM_USB_GADGET) &&
     (dr_mode == USB_DR_MODE_PERIPHERAL || dr_mode == USB_DR_MODE_OTG))
{

> 		debug("%s: dr_mode: OTG or Peripheral\n", __func__);
> 		driver = "dwc3-generic-peripheral";
> 		break;
> #if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)


else if ((CONFIG_IS_ENABLED(USB_HOST) || !IS_ENABLED(SPL_BUILD)) && 
dr_mode == host) {

> 	case USB_DR_MODE_HOST:
> 		debug("%s: dr_mode: HOST\n", __func__);
> 		driver = "dwc3-generic-host";
> 		break;
> #endif

} else {

> 	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 :-)

It's awful either way, but at least with the CONFIG_IS_ENABLED() stuff, 
we avoid bitrot of ifdef'd out code, since all the code is compiled in 
any case.