Skip to contentSkip to conversation

usb: gadget: fsl_udc_core: Fix pullup status

PATCH In review Dmitry Osipenko 2 participants 4 messages Mar 25, 2016 – Mar 29, 2016

Subsystems (2)
USB Freescale USB peripheral drivers
lore
Share & tools
lore

Dmitry Osipenko

author
#0 lore raw

PATCH v2 1/1 usb: gadget: fsl_udc_core: Fix pullup status

udc->softconnect should be set regardless of the VBUS state, otherwise the USB peripheral device, connected during suspend, won't be detected since can_pullup() would return false and the UDC won't be enabled.

Fixes 252455c40316 (usb: gadget: fsl driver pullup fix)
Signed-off-by Dmitry Osipenko <[email protected]>

---
Changelog:
V2: "(is_on != 0)" changed to "!!is_on" as per Sergei Shtylyov comment,
cleaned up commit message.

 drivers/usb/gadget/udc/fsl_udc_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/fsl_udc_core.c +2 -1
index aab5221..4309b4f 100644
@@ -1220,10 +1220,11 @@ static int fsl_pullup(struct usb_gadget *gadget, int is_on)
 
 	udc = container_of(gadget, struct fsl_udc, gadget);
 
+	udc->softconnect = !!is_on;
+
 	if (!udc->vbus_active)
 		return -EOPNOTSUPP;
 
-	udc->softconnect = (is_on != 0);
 	if (can_pullup(udc))
 		fsl_writel((fsl_readl(&dr_regs->usbcmd) | USB_CMD_RUN_STOP),
 				&dr_regs->usbcmd);
Signature · 1 line
2.7.4
#1 lore raw
Quoted 19 lines · Dmitry Osipenko

udc->softconnect should be set regardless of the VBUS state, otherwise the USB peripheral device, connected during suspend, won't be detected since can_pullup() would return false and the UDC won't be enabled.

Fixes 252455c40316 (usb: gadget: fsl driver pullup fix)
Signed-off-by Dmitry Osipenko <[email protected]>

---
Changelog:
V2: "(is_on != 0)" changed to "!!is_on" as per Sergei Shtylyov comment,
cleaned up commit message.

 drivers/usb/gadget/udc/fsl_udc_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/fsl_udc_core.c +0 -0
index aab5221..4309b4f 100644

drivers/usb/gadget/udc/fsl_udc_core.c static int fsl_pullup(struct usb_gadget *gadget, int is_on) · line 1226

Quoted 9 lines
drivers/usb/gadget/udc/fsl_udc_core.c +2 -1
@@ -1220,10 +1220,11 @@ static int fsl_pullup(struct usb_gadget *gadget, int is_on)
 
 	udc = container_of(gadget, struct fsl_udc, gadget);
 
+	udc->softconnect = !!is_on;
+
 	if (!udc->vbus_active)
 		return -EOPNOTSUPP;
 
-	udc->softconnect = (is_on != 0);

if we're suspended and VBUS was cut off, why would we keep softconnect set to true ? That would also cause a discrepancy between SW state and HW state.

I don't have this HW to test, but this patch seems wrong to me.

Signature · 1 line
balbi
author
#2 lore raw

29.03.2016 13:31, Felipe Balbi пишет:

Quoted 28 lines
Quoted 27 lines · Dmitry Osipenko

udc->softconnect should be set regardless of the VBUS state, otherwise the USB peripheral device, connected during suspend, won't be detected since can_pullup() would return false and the UDC won't be enabled.

Fixes 252455c40316 (usb: gadget: fsl driver pullup fix)
Signed-off-by Dmitry Osipenko <[email protected]>

---
Changelog:
V2: "(is_on != 0)" changed to "!!is_on" as per Sergei Shtylyov comment,
cleaned up commit message.

  drivers/usb/gadget/udc/fsl_udc_core.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/fsl_udc_core.c +2 -0
index aab5221..4309b4f 100644
@@ -1220,10 +1220,11 @@ static int fsl_pullup(struct usb_gadget *gadget, int is_on)
 
  	udc = container_of(gadget, struct fsl_udc, gadget);
 
+	udc->softconnect = !!is_on;
+
  	if (!udc->vbus_active)
  		return -EOPNOTSUPP;
Quoted 8 lines

drivers/usb/gadget/udc/fsl_udc_core.c static int fsl_pullup(struct usb_gadget *gadget, int is_on) · line 1226

Quoted 1 line
drivers/usb/gadget/udc/fsl_udc_core.c +0 -1
-	udc->softconnect = (is_on != 0);

if we're suspended and VBUS was cut off, why would we keep softconnect set to true ? That would also cause a discrepancy between SW state and HW state.

I don't have this HW to test, but this patch seems wrong to me.

Yeah, you are right. I'm using a fork of this driver with some minor differences to make it work with other hardware and missed that upstream driver enables controller on resume unconditionally. Sorry for the noise and please ignore this patch.

Signature · 1 line
Dmitry
#3 lore raw
Quoted 35 lines · Dmitry Osipenko

29.03.2016 13:31, Felipe Balbi пишет:

Quoted 34 lines
Quoted 19 lines · Dmitry Osipenko

udc->softconnect should be set regardless of the VBUS state, otherwise the USB peripheral device, connected during suspend, won't be detected since can_pullup() would return false and the UDC won't be enabled.

Fixes 252455c40316 (usb: gadget: fsl driver pullup fix)
Signed-off-by Dmitry Osipenko <[email protected]>

---
Changelog:
V2: "(is_on != 0)" changed to "!!is_on" as per Sergei Shtylyov comment,
cleaned up commit message.

  drivers/usb/gadget/udc/fsl_udc_core.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/fsl_udc_core.c +0 -0
index aab5221..4309b4f 100644

drivers/usb/gadget/udc/fsl_udc_core.c static int fsl_pullup(struct usb_gadget *gadget, int is_on) · line 1226

Quoted 9 lines
drivers/usb/gadget/udc/fsl_udc_core.c +2 -1
@@ -1220,10 +1220,11 @@ static int fsl_pullup(struct usb_gadget *gadget, int is_on)
 
  	udc = container_of(gadget, struct fsl_udc, gadget);
 
+	udc->softconnect = !!is_on;
+
  	if (!udc->vbus_active)
  		return -EOPNOTSUPP;
 
-	udc->softconnect = (is_on != 0);

if we're suspended and VBUS was cut off, why would we keep softconnect set to true ? That would also cause a discrepancy between SW state and HW state.

Quoted 7 lines
Quoted 2 lines

I don't have this HW to test, but this patch seems wrong to me.

Yeah, you are right. I'm using a fork of this driver with some minor differences to make it work with other hardware and missed that upstream driver enables controller on resume unconditionally. Sorry for the noise and please ignore this patch.

you shouldn't send patches unless you have tested them. Simply cherry-picking from another tree and sending it is NOT good enough. You MUST test what you're sending with latest tag from Linus (right now, that's v4.6-rc1). If you can't update your kernel, then don't try to patch something you can't validate.

Thanks

Signature · 1 line
balbi