Re: [PATCH] usb: dwc3-generic: Fix build errors when USB_DWC3_GADGET is disabled

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Jonas, thank you for the patch.

On lun., févr. 26, 2024 at 13:36, Jonas Karlman <jonas@kwiboo.se> wrote:

> On 2024-02-26 11:18, Marek Vasut wrote:
>> On 2/26/24 10:50 AM, Jonas Karlman wrote:
>>> On 2024-02-26 09:22, Marek Vasut wrote:
>>>> On 2/26/24 8:54 AM, Jonas Karlman wrote:
>>>>> On 2024-02-26 02:47, Tom Rini wrote:
>>>>>> On Mon, Feb 26, 2024 at 01:02:04AM +0100, Jonas Karlman wrote:
>>>>>>> On 2024-02-25 23:01, Marek Vasut wrote:
>>>>>>>> On 2/25/24 4:27 PM, Jonas Karlman wrote:
>>>>>>>>> Build fail with the following error when DM_USB_GADGET is enabled and
>>>>>>>>> USB_DWC3_GADGET is disabled:
>>>>>>>>>
>>>>>>>>>      dwc3/dwc3-generic.o: in function `dm_usb_gadget_handle_interrupts':
>>>>>>>>>      dwc3/dwc3-generic.c:201:(.text.dm_usb_gadget_handle_interrupts+0x10):
>>>>>>>>>        undefined reference to `dwc3_gadget_uboot_handle_interrupt'
>>>>>>>>>
>>>>>>>>> Build also fail with the following error when USB_GADGET_DWC2_OTG +
>>>>>>>>> DM_USB_GADGET is enabled and USB_DWC3_GADGET is disabled:
>>>>>>>>>
>>>>>>>>>      gadget/dwc2_udc_otg.o: in function `dm_usb_gadget_handle_interrupts':
>>>>>>>>>      gadget/dwc2_udc_otg.c:947: multiple definition of `dm_usb_gadget_handle_interrupts';
>>>>>>>>>        dwc3/dwc3-generic.o:dwc3/dwc3-generic.c:197: first defined here
>>>>>>>>>
>>>>>>>>> Fix this by checking for USB_DWC3_GADGET in addition to DM_USB_GADGET.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jonas Karlman <jonas@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 6fb2de8a5ace..891d01957619 100644
>>>>>>>>> --- a/drivers/usb/dwc3/dwc3-generic.c
>>>>>>>>> +++ b/drivers/usb/dwc3/dwc3-generic.c
>>>>>>>>> @@ -192,7 +192,7 @@ static int dwc3_generic_of_to_plat(struct udevice *dev)
>>>>>>>>>     	return 0;
>>>>>>>>>     }
>>>>>>>>>     
>>>>>>>>> -#if CONFIG_IS_ENABLED(DM_USB_GADGET)
>>>>>>>>> +#if IS_ENABLED(CONFIG_USB_DWC3_GADGET) && CONFIG_IS_ENABLED(DM_USB_GADGET)
>>>>>>>>
>>>>>>>> Maybe just make USB_DWC3_GADGET depend on (or select?) DM_USB_GADGET ,
>>>>>>>> since I think the DWC3 code doesn't work without DM anyway .
>>>>>>>
>>>>>>> Do you mean in addition to this? I do not think that alone is something
>>>>>>> that will address my intention to be able to disable the gadget part of
>>>>>>> the dwc3-generic driver.
>>>>>>>
>>>>>>> Before this patch it was possible to enable disable host or gadget by
>>>>>>> using USB_HOST/SPL_USB_HOST and DM_USB_GADGET/SPL_DM_USB_GADGET options.
>>>>>>>
>>>>>>> However, Rockchip RK3328 boards have dwc2 otg and dwc3 host, trying to
>>>>>>> use DM_USB_GADGET for dwc2 and USB_HOST for dwc3 is currently not
>>>>>>> possible and result in the build errors reported, i.e. multiple
>>>>>>> definition of dm_usb_gadget_handle_interrupts() and undefined reference
>>>>>>> to dwc3_gadget_uboot_handle_interrupt().
>>>>>>>
>>>>>>> After this patch I can use dm dwc2 gadget and dwc3 host without issue.
>>>>>>
>>>>>> Note that *DM_* symbols can be confusing. At this point, in SPL
>>>>>> SPL_DM_USB and SPL_DM_USB_GADGET are encouraged but not required for
>>>>>> host/gadget drivers. In full U-Boot, DM_USB is required for all host
>>>>>> drivers and really should be enabled in all cases for DM_USB_GADGET. We
>>>>>> can't enforce that at the Kconfig level because of, iirc, some of the
>>>>>> armv7 part gadget drivers (more than one). But for newer and still very
>>>>>> active chips, we should be using DM_USB_GADGET unconditionally in full
>>>>>> U-Boot.
>>>>>
>>>>> The issue is that with DM_USB_GADGET we can only have one driver
>>>>> providing the dm_usb_gadget_handle_interrupts() function.
>>>>
>>>> DM was always intended to permit multiple drivers, so this is a bug and
>>>> should be fixed, e.g. by turning the interrupt handling function into a
>>>> driver-specific callback.
>>>
>>> I fully agree, and I hope that someone can work on that separate issue
>>> and that it does not block this build error fix patch.
>> 
>> Please, let's fix this properly instead of piling up another ifdef 
>> workaround.

I agree with Marek.

>
> Reworking the core usb gadget interrupt handling is unfortunately not
> something I can spend any of my already limited hobbyist free time on.

I understand you don't have time for this.

I will try to tackle it, but I have limited time in the upcoming weeks.

I will keep you posted.

>
> If this workaround is not acceptable I can just drop the RK3328 cleanup
> and leave usb in current semi-broken state.
>
>> 
>> Since CI does pass, where is there a build error ?
>
> Currently there is no board with a configuration that cause build errors,
> that happens when I add DM_USB_GADGET=y to RK3328 boards in a separate
> series.
>
> Previously you have requested that I do not send usb patches as part of
> a larger series targeted to rockchip maintainers. So this time I sent
> the usb part as a standalone patch ahead of such series.
>
> Will send out that series later, and if no one wants to pick this up or
> have time to rework gadget interrupt handling that is fine with me :-)
>
> Regards,
> Jonas