From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrice CHOTARD Date: Wed, 17 Feb 2021 09:57:16 +0100 Subject: [PATCH] usb: gadget: dwc2_udc_otg: Fix dwc2_gadget_start() In-Reply-To: References: <20210210141759.5641-1-patrice.chotard@foss.st.com> <7b665909-b1d6-323c-f6a4-4221fca616c7@foss.st.com> <7b1e8706-8fa3-44e3-4bed-ff418e9d803d@denx.de> <457ca91c-8fd3-929c-be24-90b7d0d87670@foss.st.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Marek On 2/16/21 10:15 PM, Marek Vasut wrote: > On 2/16/21 6:32 PM, Patrice CHOTARD wrote: >> Hi Marek > > Hi, > >> On 2/11/21 12:26 PM, Marek Vasut wrote: >>> On 2/11/21 10:58 AM, Patrice CHOTARD wrote: >>>> Hi Marek >>>> >>>> On 2/10/21 3:26 PM, Marek Vasut wrote: >>>>> On 2/10/21 3:17 PM, Patrice Chotard wrote: >>>>>> Since commit 8745b9ebccae ("usb: gadget: add super speed support") >>>>>> ums was no more functional on platform which use dwc2_udc_otg driver. >>>>>> >>>>>> Remove the speed test in dwc2_gadget_start() to fix it. >>>>>> Tested on stm32mp157c-ev1 board. >>>>> >>>>> Isn't the speed check correct though ? >>>> >>>> I am not sure this speed test is needed. >>>> >>>>> >>>>> What is really going on when this fails ? >>>> >>>> >>>> Since 8745b9ebccae ("usb: gadget: add super speed support"), >>>> driver->speed is now set to USB_SPEED_SUPER in drivers/usb/gadget/composite.c >>>> >>>> and this forbids dwc2_udc_otg.c to be registered. >>> >>> That sounds like a bug in the USB gadget/otg core , no ? >> >> After analysis, if i correctly understood, the speed test done in both usb_gadget_register_driver() >> and in dwc2_gadget_start() in dwc2_udc_otg.c is too restrictive and accepts only gadget driver with >> max speed equal to USB_SPEED_FULL or USB_SPEED_HIGH. > > That should be fine for the DWC2 IP, which is LS/FS/HS. > >> So all gadget driver with max speed set to higher speed than USB_SPEED_HIGH (USB_SPEED_SUPER in case of >> composite gadget driver) are not allowed, which is wrong. >> >> This test should check that the gadget driver max speed is higher or equal to the min speed supported by >> dwc2_udc_otg driver, ie USB_SPEED_FULL. >> >> So the test should be : >> >> @@ -247,12 +247,11 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver) >> ????? unsigned long flags = 0; >> ? ????? debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name"); >> ? ????? if (!driver >> -??????? || (driver->speed != USB_SPEED_FULL >> -??????? && driver->speed != USB_SPEED_HIGH) >> +??????? || driver->speed < USB_SPEED_FULL >> ????????? || !driver->bind || !driver->disconnect || !driver->setup) >> ????????? return -EINVAL; >> ????? if (!dev) >> ????????? return -ENODEV; >> ????? if (dev->driver) >> @@ -319,12 +318,11 @@ static int dwc2_gadget_start(struct usb_gadget *g, >> ????? struct dwc2_udc *dev = the_controller; >> ? ????? debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name"); >> ? ????? if (!driver || >> -??????? (driver->speed != USB_SPEED_FULL && >> -???????? driver->speed != USB_SPEED_HIGH) || >> +??????? driver->speed < USB_SPEED_FULL || > > The DWC2 can't operate in LS gadget mode , i.e. emulate some old keyboard ? Maybe what you want is driver->speed > USB_SPEED_HIGH instead ? The test is correct, in case driver->speed is lower than FS, we return -EINVAL. All others speed FS/HS/SS and higher are allowed. > >> ????????? !driver->bind || !driver->disconnect || !driver->setup) >> ????????? return -EINVAL; >> ? ????? if (!dev) >> >> I you are agree, i will send a v2 with this. > > Yes please. But you really want to get AB/RB from Lukasz, since he does the gadget stuff. Ok, i will add Lukasz for the V2 review. Thanks Patrice