[PATCH] usb: dwc2: Implement set_selfpowered()

[PATCH] usb: dwc2: Implement set_selfpowered()

[John Keeping]

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(-)

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

Re: [PATCH] usb: dwc2: Implement set_selfpowered()

[Minas Harutyunyan]

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,
> 

Re: [PATCH] usb: dwc2: Implement set_selfpowered()

[John Keeping]

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,
> >   

Re: [PATCH] usb: dwc2: Implement set_selfpowered()

[Minas Harutyunyan]

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,
>>>    
> 

Re: [PATCH] usb: dwc2: Implement set_selfpowered()

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

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

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] usb: dwc2: Implement set_selfpowered()

[John Keeping]

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

Re: [PATCH] usb: dwc2: Implement set_selfpowered()

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

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

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]