Re: [PATCH] usb: gadget: dwc2_udc_otg: implement pullup()
On Tue, Jan 10, 2023 at 14:45, Marek Vasut wrote: > On 1/10/23 14:39, Mattijs Korpershoek wrote: >> Hi Harald, >> >> Thank you for your review. >> >> On Tue, Jan 10, 2023 at 14:30, Harald Seiler wrote: >> >>> Hi, >>> >>> On Tue, 2023-01-10 at 10:11 +0100, Mattijs Korpershoek wrote: Pullup is used by the usb framework in order to do software-controlled usb_gadget_connect() and usb_gadget_disconnect(). Implement pullup() for dwc2 using the SOFT_DISCONNECT bit in the dctl register. This is especially useful when a gadget disconnection is initiated but no board_usb_cleanup() is called. Signed-off-by: Mattijs Korpershoek --- On some boards using the dwc2 controller, like the Khadas VIM3L, whenever usb_gadget_release() is called, the D+ and D- lines are in an unknown state. Because of that, the host can't detect usb disconnection. It was attempted to be be fixed with [1] but ended up doing the gadget disconnection too early, creating issues on NXP-based boards which use uuu [2]. By implementing pullup() in the controller driver, we ensure that the disconnection will only be done when the framework calls usb_gadget_disconnect(). [1] https://lore.kernel.org/all/20220728-reset-usb-controller-v2-1-ef7657ce7...@baylibre.com/ [2] https://lore.kernel.org/all/20230107164807.3597020-1-dario.binac...@amarulasolutions.com/ --- drivers/usb/gadget/dwc2_udc_otg.c | 16 1 file changed, 16 insertions(+) diff --git a/drivers/usb/gadget/dwc2_udc_otg.c b/drivers/usb/gadget/dwc2_udc_otg.c index 77988f78ab30..93ed9712d18a 100644 --- a/drivers/usb/gadget/dwc2_udc_otg.c +++ b/drivers/usb/gadget/dwc2_udc_otg.c @@ -236,6 +236,21 @@ static int udc_enable(struct dwc2_udc *dev) return 0; } +static int dwc2_gadget_pullup(struct usb_gadget *g, int is_on) +{ + unsigned int uTemp; >>> >>> Just some minor points about style... >>> >>> I think the `u32` type should be used for 32-bit registers instead. >>> More explicit and no room for accidentally choosing the wrong size. >>> >>> Also, U-Boot and Linux don't use hungarian notation for variable names, >>> just call it `tmp` or even better `dctl` to be explicit. >> >> I agree with this. I actually picked up some of the code from >> reconfig_usbd() where hungarian nototation is used. That's also where >> the unsigned int comes from. >> >> Sorry I did not convert it to some more Linux/U-Boot oriented style >> before submitting. >> >>> + + is_on = !!is_on; >>> >>> This is superfluous, the if condition works either way. >> >> Ack. >> >> The maintainer (Marek) already picked this up in his usb tree: >> https://lore.kernel.org/all/1f2c5d74-faae-42f8-5d4d-dfc08cb09...@denx.de/ >> >> Should I send a clean-up/follow-up patch for this? > > The maintainer already dropped it and waits for V2 :) Thank you, will send a V2 shortly.
Re: [PATCH] usb: gadget: dwc2_udc_otg: implement pullup()
On 1/10/23 14:39, Mattijs Korpershoek wrote: Hi Harald, Thank you for your review. On Tue, Jan 10, 2023 at 14:30, Harald Seiler wrote: Hi, On Tue, 2023-01-10 at 10:11 +0100, Mattijs Korpershoek wrote: Pullup is used by the usb framework in order to do software-controlled usb_gadget_connect() and usb_gadget_disconnect(). Implement pullup() for dwc2 using the SOFT_DISCONNECT bit in the dctl register. This is especially useful when a gadget disconnection is initiated but no board_usb_cleanup() is called. Signed-off-by: Mattijs Korpershoek --- On some boards using the dwc2 controller, like the Khadas VIM3L, whenever usb_gadget_release() is called, the D+ and D- lines are in an unknown state. Because of that, the host can't detect usb disconnection. It was attempted to be be fixed with [1] but ended up doing the gadget disconnection too early, creating issues on NXP-based boards which use uuu [2]. By implementing pullup() in the controller driver, we ensure that the disconnection will only be done when the framework calls usb_gadget_disconnect(). [1] https://lore.kernel.org/all/20220728-reset-usb-controller-v2-1-ef7657ce7...@baylibre.com/ [2] https://lore.kernel.org/all/20230107164807.3597020-1-dario.binac...@amarulasolutions.com/ --- drivers/usb/gadget/dwc2_udc_otg.c | 16 1 file changed, 16 insertions(+) diff --git a/drivers/usb/gadget/dwc2_udc_otg.c b/drivers/usb/gadget/dwc2_udc_otg.c index 77988f78ab30..93ed9712d18a 100644 --- a/drivers/usb/gadget/dwc2_udc_otg.c +++ b/drivers/usb/gadget/dwc2_udc_otg.c @@ -236,6 +236,21 @@ static int udc_enable(struct dwc2_udc *dev) return 0; } +static int dwc2_gadget_pullup(struct usb_gadget *g, int is_on) +{ + unsigned int uTemp; Just some minor points about style... I think the `u32` type should be used for 32-bit registers instead. More explicit and no room for accidentally choosing the wrong size. Also, U-Boot and Linux don't use hungarian notation for variable names, just call it `tmp` or even better `dctl` to be explicit. I agree with this. I actually picked up some of the code from reconfig_usbd() where hungarian nototation is used. That's also where the unsigned int comes from. Sorry I did not convert it to some more Linux/U-Boot oriented style before submitting. + + is_on = !!is_on; This is superfluous, the if condition works either way. Ack. The maintainer (Marek) already picked this up in his usb tree: https://lore.kernel.org/all/1f2c5d74-faae-42f8-5d4d-dfc08cb09...@denx.de/ Should I send a clean-up/follow-up patch for this? The maintainer already dropped it and waits for V2 :)
Re: [PATCH] usb: gadget: dwc2_udc_otg: implement pullup()
Hi Harald, Thank you for your review. On Tue, Jan 10, 2023 at 14:30, Harald Seiler wrote: > Hi, > > On Tue, 2023-01-10 at 10:11 +0100, Mattijs Korpershoek wrote: >> Pullup is used by the usb framework in order to do software-controlled >> usb_gadget_connect() and usb_gadget_disconnect(). >> >> Implement pullup() for dwc2 using the SOFT_DISCONNECT bit in the dctl >> register. >> >> This is especially useful when a gadget disconnection is initiated but >> no board_usb_cleanup() is called. >> >> Signed-off-by: Mattijs Korpershoek >> --- >> On some boards using the dwc2 controller, like the Khadas VIM3L, whenever >> usb_gadget_release() is called, the D+ and D- lines are in an unknown state. >> >> Because of that, the host can't detect usb disconnection. >> >> It was attempted to be be fixed with [1] but ended up doing the gadget >> disconnection >> too early, creating issues on NXP-based boards which use uuu [2]. >> >> By implementing pullup() in the controller driver, we ensure that the >> disconnection will >> only be done when the framework calls usb_gadget_disconnect(). >> >> [1] >> https://lore.kernel.org/all/20220728-reset-usb-controller-v2-1-ef7657ce7...@baylibre.com/ >> [2] >> https://lore.kernel.org/all/20230107164807.3597020-1-dario.binac...@amarulasolutions.com/ >> --- >> drivers/usb/gadget/dwc2_udc_otg.c | 16 >> 1 file changed, 16 insertions(+) >> >> diff --git a/drivers/usb/gadget/dwc2_udc_otg.c >> b/drivers/usb/gadget/dwc2_udc_otg.c >> index 77988f78ab30..93ed9712d18a 100644 >> --- a/drivers/usb/gadget/dwc2_udc_otg.c >> +++ b/drivers/usb/gadget/dwc2_udc_otg.c >> @@ -236,6 +236,21 @@ static int udc_enable(struct dwc2_udc *dev) >> return 0; >> } >> >> +static int dwc2_gadget_pullup(struct usb_gadget *g, int is_on) >> +{ >> +unsigned int uTemp; > > Just some minor points about style... > > I think the `u32` type should be used for 32-bit registers instead. > More explicit and no room for accidentally choosing the wrong size. > > Also, U-Boot and Linux don't use hungarian notation for variable names, > just call it `tmp` or even better `dctl` to be explicit. I agree with this. I actually picked up some of the code from reconfig_usbd() where hungarian nototation is used. That's also where the unsigned int comes from. Sorry I did not convert it to some more Linux/U-Boot oriented style before submitting. > >> + >> +is_on = !!is_on; > > This is superfluous, the if condition works either way. Ack. The maintainer (Marek) already picked this up in his usb tree: https://lore.kernel.org/all/1f2c5d74-faae-42f8-5d4d-dfc08cb09...@denx.de/ Should I send a clean-up/follow-up patch for this? > >> +uTemp = readl(>dctl); >> +if (is_on) >> +uTemp = uTemp & ~SOFT_DISCONNECT; >> +else >> +uTemp |= SOFT_DISCONNECT; >> +writel(uTemp, >dctl); >> + >> +return 0; >> +} >> + >> #if !CONFIG_IS_ENABLED(DM_USB_GADGET) >> /* >>Register entry point for the peripheral controller driver. >> @@ -805,6 +820,7 @@ static void dwc2_fifo_flush(struct usb_ep *_ep) >> } >> >> static const struct usb_gadget_ops dwc2_udc_ops = { >> +.pullup = dwc2_gadget_pullup, >> /* current versions must always be self-powered */ >> #if CONFIG_IS_ENABLED(DM_USB_GADGET) >> .udc_start = dwc2_gadget_start, >> >> --- >> base-commit: 7b84c973b96775576dcff228d865e8570be26c82 >> change-id: 20230110-dwc2-pullup-5b0f5a073d6b >> >> Best regards, > > Regards, > > Harald
Re: [PATCH] usb: gadget: dwc2_udc_otg: implement pullup()
On 1/10/23 14:30, Harald Seiler wrote: Hi, On Tue, 2023-01-10 at 10:11 +0100, Mattijs Korpershoek wrote: Pullup is used by the usb framework in order to do software-controlled usb_gadget_connect() and usb_gadget_disconnect(). Implement pullup() for dwc2 using the SOFT_DISCONNECT bit in the dctl register. This is especially useful when a gadget disconnection is initiated but no board_usb_cleanup() is called. Signed-off-by: Mattijs Korpershoek --- On some boards using the dwc2 controller, like the Khadas VIM3L, whenever usb_gadget_release() is called, the D+ and D- lines are in an unknown state. Because of that, the host can't detect usb disconnection. It was attempted to be be fixed with [1] but ended up doing the gadget disconnection too early, creating issues on NXP-based boards which use uuu [2]. By implementing pullup() in the controller driver, we ensure that the disconnection will only be done when the framework calls usb_gadget_disconnect(). [1] https://lore.kernel.org/all/20220728-reset-usb-controller-v2-1-ef7657ce7...@baylibre.com/ [2] https://lore.kernel.org/all/20230107164807.3597020-1-dario.binac...@amarulasolutions.com/ --- drivers/usb/gadget/dwc2_udc_otg.c | 16 1 file changed, 16 insertions(+) diff --git a/drivers/usb/gadget/dwc2_udc_otg.c b/drivers/usb/gadget/dwc2_udc_otg.c index 77988f78ab30..93ed9712d18a 100644 --- a/drivers/usb/gadget/dwc2_udc_otg.c +++ b/drivers/usb/gadget/dwc2_udc_otg.c @@ -236,6 +236,21 @@ static int udc_enable(struct dwc2_udc *dev) return 0; } +static int dwc2_gadget_pullup(struct usb_gadget *g, int is_on) +{ + unsigned int uTemp; Just some minor points about style... I think the `u32` type should be used for 32-bit registers instead. More explicit and no room for accidentally choosing the wrong size. Also, U-Boot and Linux don't use hungarian notation for variable names, just call it `tmp` or even better `dctl` to be explicit. + + is_on = !!is_on; This is superfluous, the if condition works either way. clrsetbits_le32() would turn the above into a oneliner.
Re: [PATCH] usb: gadget: dwc2_udc_otg: implement pullup()
Hi, On Tue, 2023-01-10 at 10:11 +0100, Mattijs Korpershoek wrote: > Pullup is used by the usb framework in order to do software-controlled > usb_gadget_connect() and usb_gadget_disconnect(). > > Implement pullup() for dwc2 using the SOFT_DISCONNECT bit in the dctl > register. > > This is especially useful when a gadget disconnection is initiated but > no board_usb_cleanup() is called. > > Signed-off-by: Mattijs Korpershoek > --- > On some boards using the dwc2 controller, like the Khadas VIM3L, whenever > usb_gadget_release() is called, the D+ and D- lines are in an unknown state. > > Because of that, the host can't detect usb disconnection. > > It was attempted to be be fixed with [1] but ended up doing the gadget > disconnection > too early, creating issues on NXP-based boards which use uuu [2]. > > By implementing pullup() in the controller driver, we ensure that the > disconnection will > only be done when the framework calls usb_gadget_disconnect(). > > [1] > https://lore.kernel.org/all/20220728-reset-usb-controller-v2-1-ef7657ce7...@baylibre.com/ > [2] > https://lore.kernel.org/all/20230107164807.3597020-1-dario.binac...@amarulasolutions.com/ > --- > drivers/usb/gadget/dwc2_udc_otg.c | 16 > 1 file changed, 16 insertions(+) > > diff --git a/drivers/usb/gadget/dwc2_udc_otg.c > b/drivers/usb/gadget/dwc2_udc_otg.c > index 77988f78ab30..93ed9712d18a 100644 > --- a/drivers/usb/gadget/dwc2_udc_otg.c > +++ b/drivers/usb/gadget/dwc2_udc_otg.c > @@ -236,6 +236,21 @@ static int udc_enable(struct dwc2_udc *dev) > return 0; > } > > +static int dwc2_gadget_pullup(struct usb_gadget *g, int is_on) > +{ > + unsigned int uTemp; Just some minor points about style... I think the `u32` type should be used for 32-bit registers instead. More explicit and no room for accidentally choosing the wrong size. Also, U-Boot and Linux don't use hungarian notation for variable names, just call it `tmp` or even better `dctl` to be explicit. > + > + is_on = !!is_on; This is superfluous, the if condition works either way. > + uTemp = readl(>dctl); > + if (is_on) > + uTemp = uTemp & ~SOFT_DISCONNECT; > + else > + uTemp |= SOFT_DISCONNECT; > + writel(uTemp, >dctl); > + > + return 0; > +} > + > #if !CONFIG_IS_ENABLED(DM_USB_GADGET) > /* >Register entry point for the peripheral controller driver. > @@ -805,6 +820,7 @@ static void dwc2_fifo_flush(struct usb_ep *_ep) > } > > static const struct usb_gadget_ops dwc2_udc_ops = { > + .pullup = dwc2_gadget_pullup, > /* current versions must always be self-powered */ > #if CONFIG_IS_ENABLED(DM_USB_GADGET) > .udc_start = dwc2_gadget_start, > > --- > base-commit: 7b84c973b96775576dcff228d865e8570be26c82 > change-id: 20230110-dwc2-pullup-5b0f5a073d6b > > Best regards, Regards, Harald
[PATCH] usb: gadget: dwc2_udc_otg: implement pullup()
Pullup is used by the usb framework in order to do software-controlled usb_gadget_connect() and usb_gadget_disconnect(). Implement pullup() for dwc2 using the SOFT_DISCONNECT bit in the dctl register. This is especially useful when a gadget disconnection is initiated but no board_usb_cleanup() is called. Signed-off-by: Mattijs Korpershoek --- On some boards using the dwc2 controller, like the Khadas VIM3L, whenever usb_gadget_release() is called, the D+ and D- lines are in an unknown state. Because of that, the host can't detect usb disconnection. It was attempted to be be fixed with [1] but ended up doing the gadget disconnection too early, creating issues on NXP-based boards which use uuu [2]. By implementing pullup() in the controller driver, we ensure that the disconnection will only be done when the framework calls usb_gadget_disconnect(). [1] https://lore.kernel.org/all/20220728-reset-usb-controller-v2-1-ef7657ce7...@baylibre.com/ [2] https://lore.kernel.org/all/20230107164807.3597020-1-dario.binac...@amarulasolutions.com/ --- drivers/usb/gadget/dwc2_udc_otg.c | 16 1 file changed, 16 insertions(+) diff --git a/drivers/usb/gadget/dwc2_udc_otg.c b/drivers/usb/gadget/dwc2_udc_otg.c index 77988f78ab30..93ed9712d18a 100644 --- a/drivers/usb/gadget/dwc2_udc_otg.c +++ b/drivers/usb/gadget/dwc2_udc_otg.c @@ -236,6 +236,21 @@ static int udc_enable(struct dwc2_udc *dev) return 0; } +static int dwc2_gadget_pullup(struct usb_gadget *g, int is_on) +{ + unsigned int uTemp; + + is_on = !!is_on; + uTemp = readl(>dctl); + if (is_on) + uTemp = uTemp & ~SOFT_DISCONNECT; + else + uTemp |= SOFT_DISCONNECT; + writel(uTemp, >dctl); + + return 0; +} + #if !CONFIG_IS_ENABLED(DM_USB_GADGET) /* Register entry point for the peripheral controller driver. @@ -805,6 +820,7 @@ static void dwc2_fifo_flush(struct usb_ep *_ep) } static const struct usb_gadget_ops dwc2_udc_ops = { + .pullup = dwc2_gadget_pullup, /* current versions must always be self-powered */ #if CONFIG_IS_ENABLED(DM_USB_GADGET) .udc_start = dwc2_gadget_start, --- base-commit: 7b84c973b96775576dcff228d865e8570be26c82 change-id: 20230110-dwc2-pullup-5b0f5a073d6b Best regards, -- Mattijs Korpershoek