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 <[email protected]>
---
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,
--
2.25.0
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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]>
>>
Acked-by: Minas Harutyunyan <[email protected]>
>> 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 <[email protected]> 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 <[email protected]>
> ---
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
--
balbi
On Wed, 05 Feb 2020 18:36:21 +0200
Felipe Balbi <[email protected]> wrote:
> John Keeping <[email protected]> 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 <[email protected]>
> > ---
>
> 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 <[email protected]> writes:
> On Wed, 05 Feb 2020 18:36:21 +0200
> Felipe Balbi <[email protected]> wrote:
>
>> John Keeping <[email protected]> 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 <[email protected]>
>> > ---
>>
>> 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
--
balbi