| Message ID | 20200204152933.2216615-1-john@metanate.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 1a0808cb9e417170ed6ab97254cf319dc3e3c310 |
| Headers | show |
| Series | usb: dwc2: Implement set_selfpowered() | expand |
Hi John, On 2/4/2020 7:29 PM, John Keeping wrote: > dwc2 always reports as self-powered in response to a device status > request. Implement the set_selfpowered() operations so that the gadget > can report as bus-powered when appropriate. > > This is modelled on the dwc3 implementation. > > Signed-off-by: John Keeping <john@metanate.com> Good catch. Just one concern. Your patch partially fix my patch this is why I think you need to add Fixes tag otherwise it can create merge conflict if your patch will be merged to next earlier than my. Thanks, Minas > --- > drivers/usb/dwc2/gadget.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 2717f4401b97..76c0a5242175 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -1646,7 +1646,8 @@ static int dwc2_hsotg_process_req_status(struct dwc2_hsotg *hsotg, > > switch (ctrl->bRequestType & USB_RECIP_MASK) { > case USB_RECIP_DEVICE: > - status = 1 << USB_DEVICE_SELF_POWERED; > + status = hsotg->gadget.is_selfpowered << > + USB_DEVICE_SELF_POWERED; > status |= hsotg->remote_wakeup_allowed << > USB_DEVICE_REMOTE_WAKEUP; > reply = cpu_to_le16(status); > @@ -4530,6 +4531,26 @@ static int dwc2_hsotg_gadget_getframe(struct usb_gadget *gadget) > return dwc2_hsotg_read_frameno(to_hsotg(gadget)); > } > > +/** > + * dwc2_hsotg_set_selfpowered - set if device is self/bus powered > + * @gadget: The usb gadget state > + * @is_selfpowered: Whether the device is self-powered > + * > + * Set if the device is self or bus powered. > + */ > +static int dwc2_hsotg_set_selfpowered(struct usb_gadget *gadget, > + int is_selfpowered) > +{ > + struct dwc2_hsotg *hsotg = to_hsotg(gadget); > + unsigned long flags; > + > + spin_lock_irqsave(&hsotg->lock, flags); > + gadget->is_selfpowered = !!is_selfpowered; > + spin_unlock_irqrestore(&hsotg->lock, flags); > + > + return 0; > +} > + > /** > * dwc2_hsotg_pullup - connect/disconnect the USB PHY > * @gadget: The usb gadget state > @@ -4621,6 +4642,7 @@ static int dwc2_hsotg_vbus_draw(struct usb_gadget *gadget, unsigned int mA) > > static const struct usb_gadget_ops dwc2_hsotg_gadget_ops = { > .get_frame = dwc2_hsotg_gadget_getframe, > + .set_selfpowered = dwc2_hsotg_set_selfpowered, > .udc_start = dwc2_hsotg_udc_start, > .udc_stop = dwc2_hsotg_udc_stop, > .pullup = dwc2_hsotg_pullup, >
Hi Minas, On Wed, 5 Feb 2020 07:59:48 +0000 Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> wrote: > On 2/4/2020 7:29 PM, John Keeping wrote: > > dwc2 always reports as self-powered in response to a device status > > request. Implement the set_selfpowered() operations so that the gadget > > can report as bus-powered when appropriate. > > > > This is modelled on the dwc3 implementation. > > > > Signed-off-by: John Keeping <john@metanate.com> > > Good catch. Just one concern. Your patch partially fix my patch this is > why I think you need to add Fixes tag otherwise it can create merge > conflict if your patch will be merged to next earlier than my. I don't think this is actually a fix for your patch, it's a separate fix which happens to touch the same code. Since dwc2 has never supported the set_selfpowered() operation, I'm not really sure if this counts as a bugfix or a feature. I'm happy to re-send with a fixes tag if you think it's necessary, but I don't think it's accurate in this case - your patch did not introduce a bug here :-) Regards, John > > --- > > drivers/usb/dwc2/gadget.c | 24 +++++++++++++++++++++++- > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > > index 2717f4401b97..76c0a5242175 100644 > > --- a/drivers/usb/dwc2/gadget.c > > +++ b/drivers/usb/dwc2/gadget.c > > @@ -1646,7 +1646,8 @@ static int dwc2_hsotg_process_req_status(struct dwc2_hsotg *hsotg, > > > > switch (ctrl->bRequestType & USB_RECIP_MASK) { > > case USB_RECIP_DEVICE: > > - status = 1 << USB_DEVICE_SELF_POWERED; > > + status = hsotg->gadget.is_selfpowered << > > + USB_DEVICE_SELF_POWERED; > > status |= hsotg->remote_wakeup_allowed << > > USB_DEVICE_REMOTE_WAKEUP; > > reply = cpu_to_le16(status); > > @@ -4530,6 +4531,26 @@ static int dwc2_hsotg_gadget_getframe(struct usb_gadget *gadget) > > return dwc2_hsotg_read_frameno(to_hsotg(gadget)); > > } > > > > +/** > > + * dwc2_hsotg_set_selfpowered - set if device is self/bus powered > > + * @gadget: The usb gadget state > > + * @is_selfpowered: Whether the device is self-powered > > + * > > + * Set if the device is self or bus powered. > > + */ > > +static int dwc2_hsotg_set_selfpowered(struct usb_gadget *gadget, > > + int is_selfpowered) > > +{ > > + struct dwc2_hsotg *hsotg = to_hsotg(gadget); > > + unsigned long flags; > > + > > + spin_lock_irqsave(&hsotg->lock, flags); > > + gadget->is_selfpowered = !!is_selfpowered; > > + spin_unlock_irqrestore(&hsotg->lock, flags); > > + > > + return 0; > > +} > > + > > /** > > * dwc2_hsotg_pullup - connect/disconnect the USB PHY > > * @gadget: The usb gadget state > > @@ -4621,6 +4642,7 @@ static int dwc2_hsotg_vbus_draw(struct usb_gadget *gadget, unsigned int mA) > > > > static const struct usb_gadget_ops dwc2_hsotg_gadget_ops = { > > .get_frame = dwc2_hsotg_gadget_getframe, > > + .set_selfpowered = dwc2_hsotg_set_selfpowered, > > .udc_start = dwc2_hsotg_udc_start, > > .udc_stop = dwc2_hsotg_udc_stop, > > .pullup = dwc2_hsotg_pullup, > >
Hi, On 2/5/2020 3:56 PM, John Keeping wrote: > Hi Minas, > > On Wed, 5 Feb 2020 07:59:48 +0000 > Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> wrote: > >> On 2/4/2020 7:29 PM, John Keeping wrote: >>> dwc2 always reports as self-powered in response to a device status >>> request. Implement the set_selfpowered() operations so that the gadget >>> can report as bus-powered when appropriate. >>> >>> This is modelled on the dwc3 implementation. >>> >>> Signed-off-by: John Keeping <john@metanate.com> >> Acked-by: Minas Harutyunyan <hminas@synopsys.com> >> Good catch. Just one concern. Your patch partially fix my patch this is >> why I think you need to add Fixes tag otherwise it can create merge >> conflict if your patch will be merged to next earlier than my. > > I don't think this is actually a fix for your patch, it's a separate fix > which happens to touch the same code. > > Since dwc2 has never supported the set_selfpowered() operation, I'm not > really sure if this counts as a bugfix or a feature. > > I'm happy to re-send with a fixes tag if you think it's necessary, but I > don't think it's accurate in this case - your patch did not introduce a > bug here :-) > > > Regards, > John > >>> --- >>> drivers/usb/dwc2/gadget.c | 24 +++++++++++++++++++++++- >>> 1 file changed, 23 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >>> index 2717f4401b97..76c0a5242175 100644 >>> --- a/drivers/usb/dwc2/gadget.c >>> +++ b/drivers/usb/dwc2/gadget.c >>> @@ -1646,7 +1646,8 @@ static int dwc2_hsotg_process_req_status(struct dwc2_hsotg *hsotg, >>> >>> switch (ctrl->bRequestType & USB_RECIP_MASK) { >>> case USB_RECIP_DEVICE: >>> - status = 1 << USB_DEVICE_SELF_POWERED; >>> + status = hsotg->gadget.is_selfpowered << >>> + USB_DEVICE_SELF_POWERED; >>> status |= hsotg->remote_wakeup_allowed << >>> USB_DEVICE_REMOTE_WAKEUP; >>> reply = cpu_to_le16(status); >>> @@ -4530,6 +4531,26 @@ static int dwc2_hsotg_gadget_getframe(struct usb_gadget *gadget) >>> return dwc2_hsotg_read_frameno(to_hsotg(gadget)); >>> } >>> >>> +/** >>> + * dwc2_hsotg_set_selfpowered - set if device is self/bus powered >>> + * @gadget: The usb gadget state >>> + * @is_selfpowered: Whether the device is self-powered >>> + * >>> + * Set if the device is self or bus powered. >>> + */ >>> +static int dwc2_hsotg_set_selfpowered(struct usb_gadget *gadget, >>> + int is_selfpowered) >>> +{ >>> + struct dwc2_hsotg *hsotg = to_hsotg(gadget); >>> + unsigned long flags; >>> + >>> + spin_lock_irqsave(&hsotg->lock, flags); >>> + gadget->is_selfpowered = !!is_selfpowered; >>> + spin_unlock_irqrestore(&hsotg->lock, flags); >>> + >>> + return 0; >>> +} >>> + >>> /** >>> * dwc2_hsotg_pullup - connect/disconnect the USB PHY >>> * @gadget: The usb gadget state >>> @@ -4621,6 +4642,7 @@ static int dwc2_hsotg_vbus_draw(struct usb_gadget *gadget, unsigned int mA) >>> >>> static const struct usb_gadget_ops dwc2_hsotg_gadget_ops = { >>> .get_frame = dwc2_hsotg_gadget_getframe, >>> + .set_selfpowered = dwc2_hsotg_set_selfpowered, >>> .udc_start = dwc2_hsotg_udc_start, >>> .udc_stop = dwc2_hsotg_udc_stop, >>> .pullup = dwc2_hsotg_pullup, >>> >
John Keeping <john@metanate.com> writes: > dwc2 always reports as self-powered in response to a device status > request. Implement the set_selfpowered() operations so that the gadget > can report as bus-powered when appropriate. > > This is modelled on the dwc3 implementation. > > Signed-off-by: John Keeping <john@metanate.com> > --- what's the dependency here? checking file drivers/usb/dwc2/gadget.c Hunk #1 FAILED at 1646. Hunk #2 succeeded at 4521 (offset -9 lines). Hunk #3 succeeded at 4632 (offset -9 lines). 1 out of 3 hunks FAILED
On Wed, 05 Feb 2020 18:36:21 +0200 Felipe Balbi <balbi@kernel.org> wrote: > John Keeping <john@metanate.com> writes: > > > dwc2 always reports as self-powered in response to a device status > > request. Implement the set_selfpowered() operations so that the gadget > > can report as bus-powered when appropriate. > > > > This is modelled on the dwc3 implementation. > > > > Signed-off-by: John Keeping <john@metanate.com> > > --- > > what's the dependency here? It depends on 6de1e301b9cf ("usb: dwc2: Fix SET/CLEAR_FEATURE and GET_STATUS flows") in your testing/fixes tree. Sorry, I should have mentioned that in the original message. John
John Keeping <john@metanate.com> writes: > On Wed, 05 Feb 2020 18:36:21 +0200 > Felipe Balbi <balbi@kernel.org> wrote: > >> John Keeping <john@metanate.com> writes: >> >> > dwc2 always reports as self-powered in response to a device status >> > request. Implement the set_selfpowered() operations so that the gadget >> > can report as bus-powered when appropriate. >> > >> > This is modelled on the dwc3 implementation. >> > >> > Signed-off-by: John Keeping <john@metanate.com> >> > --- >> >> what's the dependency here? > > It depends on 6de1e301b9cf ("usb: dwc2: Fix SET/CLEAR_FEATURE and > GET_STATUS flows") in your testing/fixes tree. > > Sorry, I should have mentioned that in the original message. No worries, I'll wait until those reach mainline before merging $subject, then. cheers
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 2717f4401b97..76c0a5242175 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -1646,7 +1646,8 @@ static int dwc2_hsotg_process_req_status(struct dwc2_hsotg *hsotg, switch (ctrl->bRequestType & USB_RECIP_MASK) { case USB_RECIP_DEVICE: - status = 1 << USB_DEVICE_SELF_POWERED; + status = hsotg->gadget.is_selfpowered << + USB_DEVICE_SELF_POWERED; status |= hsotg->remote_wakeup_allowed << USB_DEVICE_REMOTE_WAKEUP; reply = cpu_to_le16(status); @@ -4530,6 +4531,26 @@ static int dwc2_hsotg_gadget_getframe(struct usb_gadget *gadget) return dwc2_hsotg_read_frameno(to_hsotg(gadget)); } +/** + * dwc2_hsotg_set_selfpowered - set if device is self/bus powered + * @gadget: The usb gadget state + * @is_selfpowered: Whether the device is self-powered + * + * Set if the device is self or bus powered. + */ +static int dwc2_hsotg_set_selfpowered(struct usb_gadget *gadget, + int is_selfpowered) +{ + struct dwc2_hsotg *hsotg = to_hsotg(gadget); + unsigned long flags; + + spin_lock_irqsave(&hsotg->lock, flags); + gadget->is_selfpowered = !!is_selfpowered; + spin_unlock_irqrestore(&hsotg->lock, flags); + + return 0; +} + /** * dwc2_hsotg_pullup - connect/disconnect the USB PHY * @gadget: The usb gadget state @@ -4621,6 +4642,7 @@ static int dwc2_hsotg_vbus_draw(struct usb_gadget *gadget, unsigned int mA) static const struct usb_gadget_ops dwc2_hsotg_gadget_ops = { .get_frame = dwc2_hsotg_gadget_getframe, + .set_selfpowered = dwc2_hsotg_set_selfpowered, .udc_start = dwc2_hsotg_udc_start, .udc_stop = dwc2_hsotg_udc_stop, .pullup = dwc2_hsotg_pullup,
dwc2 always reports as self-powered in response to a device status request. Implement the set_selfpowered() operations so that the gadget can report as bus-powered when appropriate. This is modelled on the dwc3 implementation. Signed-off-by: John Keeping <john@metanate.com> --- drivers/usb/dwc2/gadget.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)