Hi Jonas, thank you for the patch. On lun., févr. 26, 2024 at 13:36, Jonas Karlman <jo...@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 <jo...@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