Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status

[Paul Zimmerman]

Hi Felipe,

Felipe Balbi  writes:

< snip >

> thinking about this a little more. This extra list_empty() check is
> not wrong at all :-) I've amended this series with the 3 patches
> below. I'll resend the series once I've given more time for people to
> test. Patches have been updated to the branch on kernel.org as well,
> btw.

< snip >

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 9d4dc8bed644..9cf89f3cf203 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1233,6 +1233,8 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>   return DWC3_DSTS_SOFFN(reg);
>  }
> 
> +#define DWC3_ALIGN_FRAME(d)  (((d)->frame_number + (d)->interval) & 
> ~((d)->interval - 1))
> +
>  static void __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>  {
>   if (list_empty(>pending_list)) {
> @@ -1242,11 +1244,7 @@ static void __dwc3_gadget_start_isoc(struct dwc3_ep 
> *dep)
>   return;
>   }
> 
> - /*
> -  * Schedule the first trb for one interval in the future or at
> -  * least 4 microframes.
> -  */
> - dep->frame_number += max_t(u32, 4, dep->interval);
> + dep->frame_number = DWC3_ALIGN_FRAME(dep);
>   __dwc3_gadget_kick_transfer(dep);
>  }

Pretty sure this could cause problems. Instead of starting at least 4 uframes
in the future, this will now try to start at the next aligned uframe. If
dep->interval is very small (say 1) and we are almost at the end of the
current uframe, there might not be enough time?

-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: howto debug xhci driver?

[Paul Zimmerman]

Forgot to CC linux-usb.


 Forwarded Message 
Subject: Re: howto debug xhci driver?
Date: Tue, 20 Mar 2018 13:56:21 -0700
From: Paul Zimmerman <pauld...@gmail.com>
To: Bin Liu <b-...@ti.com>
CC: Felipe Balbi <felipe.ba...@linux.intel.com>

Hi,

Bin Liu <b-...@ti.com> writes:

Bin Liu <b-...@ti.com> writes:

BTY, the issue I am trying to debug is when reading
bulk IN data from a USB2.0 device, if the device
doesn't have data to transmit and NAKs the IN packet,
after 4 pairs of IN-NAK transactions, xhci stops
sending further IN tokens until the next SOF, which
leaves ~90us gape on the bus.

But when reading data from a USB2.0 thumb drive, this
issue doesn't happen, even if the device NAKs the IN
tokens, xhci still keeps sending IN tokens, which is
way more than 4 pairs of IN-NAK transactions.


Thumb drive has Bulk endpoints, what is the other
device's transfer type?


It is bulk too. I asked for device descriptors. This is a
remote debug effort for me, I don't have that device...




Any one has a clue on what causes xhci to stop sending
IN tokens after the device NAK'd 4 times?


By accident, I reproduced the issue if addng a hub in the
middle... any comments about why a hub changes this xhci
behavior is appreciated.


none off the top of my head. Maybe Mathias can suggest
something.


The issue seems to be not related to how many bulk IN-NAK pairs
before host stops sending IN token, but the host stops IN token
if 1) the device ever NAK'd an bulk IN token, and 2) there is
about 90~100us left to the next SOF. Then all the rest of
bandwidth is wasted.

Is it about xhci bandwidth schduling? /me started reading...


is this AM4 or AM5? Perhaps go after Synopsys' known errata list?


I see the issue on both AM4 & AM5. I don't have access to the errata 
list, I guess I should talk to TI internal for the list?


I have a hazy recollection of something like this being a "feature" of
the Synopsys core, to cut down on the excessive number of IN-NAK
transactions you can sometimes get on the USB bus. So yep, you
should talk to your Synopsys guy about this.

--
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Leak of queue heads in DWC2 driver?

[Paul Zimmerman]

On Tue, 13 Mar 2018 06:22:30 +, Minas Harutyunyan 
<minas.harutyun...@synopsys.com> wrote:

> Hi Paul,
> 
> On 3/13/2018 6:26 AM, Paul Zimmerman wrote:
> > Hi Minas,
> > 
> > While ‌looking at the QH queuing in the DWC2 driver, I think I've found
> > some places where the QH struct may not be freed. Normally, the sequence is:
> > 
> > dwc2_hcd_qh_unlink();
> > < other stuff >
> > dwc2_hcd_qh_free();
> > 
> > or else:
> > 
> > dwc2_hcd_qh_unlink();
> > < other stuff >
> > < link the QH to some other list >
> > 
> > For non-periodic EPs, dwc2_hcd_qh_unlink() does
> > list_del_init(>qh_list_entry), or for periodic EPs it calls
> > dwc2_deschedule_periodic() which in turn does the list_del_init().
> > This means the QH is removed from whatever list it was on.
> > 
> > So after the call to dwc2_hcd_qh_unlink(), the QH either needs to be freed
> > by calling dwc2_hcd_qh_free(), or it needs to be re-linked to another list,
> > otherwise the QH would be "lost" and could never be freed.
> > 
> > The places where I think a problem can happen are in 
> > dwc2_hcd_qh_deactivate(),
> > dwc_hcd_urb_dequeue(), and dwc_hcd_complete_xfer_ddma(). In most if not all
> > of these places, interrupts are disabled, which means that 
> > dwc2_hcd_qh_free()
> > cannot be called, since it can sleep. So maybe the freeing was omitted 
> > because
> > it was hard to do in these places?
> > 
> > What do you think, am I reading the code correctly and this could be a real
> > problem, or am I crazy? :)
> > 
> 
> Thank you for your input. I'm planing to review all host code in 1 or 2 
> months. Yes, in host code there are lot of stuff should be 
> updated/corrected. Currently we want to complete all fixes/improvements 
> on gadget side. We still have lot of prepared patches in our queue which 
> need to be included in mainline Kernel. BTW, besides fixes/improvements 
> there are few new features like Service Interval support on device side 
> which currently under development and debugging.

Looks like this was a false alarm. What I missed is that a pointer to the QH
is saved in usb_host_endpoint->hcpriv, and when dwc2_hcd_endpoint_disable()
is called, the QH is fetched from there and then freed. I also added some
debugging code to count the number of QHs allocated, and did not see the
number rising, except when a new device was connected, and in that case
the count fell back to the previous number once the device was disconnected.

So nevermind, and sorry for the noise.

-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: High CPU load produced by USB (DW2)

[Paul Zimmerman]

On Tue, 13 Mar 2018 06:11:06 +, Minas Harutyunyan 
<minas.harutyun...@synopsys.com> wrote:

> Hi Paul,
> 
> On 3/13/2018 2:39 AM, Paul Zimmerman wrote:
> > On Mon, 12 Mar 2018 12:06:55 +, Minas Harutyunyan 
> > <minas.harutyun...@synopsys.com> wrote:
> > 
> >> Hi Paul,
> >>
> >> Nice to hear you  again! Hope you are doing well.
> > 
> > Thanks, same to you.
> > 
> >> On 3/8/2018 1:51 PM, Paul Zimmerman wrote:
> >>> On 03/06/2018 09:09 AM, Minas Harutyunyan wrote:
> >>>> Driver debug log not required.
> >>>> Based on lsusb output: to dwc2 port connected "Standard Microsystems
> >>>> Corp. USB 2.0 Hub" to which connected your HS device "SanDisk Corp.
> >>>> Ultra". Because of connected HUB, which have periodic endpoint
> >>>> (Interrupt IN EP1) like all HUB's, dwc2 forced in Buffer DMA mode unmask
> >>>> SOF interrupts.
> >>>
> >>> Hmm. It seems to me that for hubs, where the interrupt EP is only used to
> >>> poll for connection changes (so the accuracy of the polling interval is 
> >>> not
> >>> important), an OS timer could be used instead of enabling SOF interrupts.
> >>>
> >>> I have a Raspberry Pi 3 around here somewhere, maybe I'll dig it out and
> >>> try playing around with this idea. No promises though!
> >>>
> >>
> >> As I correctly understand, you suggest to mask SOF's and schedule in
> >> dwc2 timer with expiration time 125us*interval. Based on timer
> >> expiration get first urb from periodic queue and start transfer on
> >> Interrupt IN EPn. In this case interrupt count will be decreased by
> >> 'interval' times. But how you going to recognized inside dwc2 that this
> >> Interrupt IN EPn is HUB endpoint or not?
> > 
> > Yes, you understand correctly. But on second thought, we would need to treat
> > *all* interrupt EPs this way, since things like mice and keyboards also have
> > interrupt EPs. But I'm not sure if any device really needs highly accurate
> > polling intervals on their interrupt EPs, so my idea may still work.
> > 
> > In any case, I have my RPI3 set up and I can build a working kernel for it
> > (sort of, but that's another story), so I will start experimenting with 
> > this.
> > 
> 
> Idea is really good, but I'm not sure it's will work or no. See, if 
> interrupt IN transfers target frame is N. Due to timer can't be very 
> accurate it can will expire in N-1 or N+1 uframe and if we will start 
> transfer in that uframes, then not clear how will behave device if IN 
> token will received in not exact expecting uframe.
> BTW, your suggestion only for Interrupt IN or OUT also?

The USB spec says the device "can depend only on the fact that the host will
ensure that the time duration between two transaction attempts with the
endpoint will be no longer than the desired period. Note that errors on the
bus can prevent an interrupt transaction from being successfully delivered
over the bus and consequently exceed the desired period. Also, the endpoint
is only polled when the software client has an IRP for an interrupt transfer
pending".

So the device can't be that picky about the exact uframe the transfer is
requested.

I will do both Interrupt IN and OUT. And if an Interrupt EP with a very short
interval is submitted, or an Isoch EP, then I will revert to unmasking the
SOF interrupt and handling all periodic EPs the old way.

Yeah, I'm not sure this will work either, but I'll give it a try.

-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leak of queue heads in DWC2 driver?

[Paul Zimmerman]

Hi Minas,

While ‌looking at the QH queuing in the DWC2 driver, I think I've found
some places where the QH struct may not be freed. Normally, the sequence is:

dwc2_hcd_qh_unlink();
< other stuff >
dwc2_hcd_qh_free();

or else:

dwc2_hcd_qh_unlink();
< other stuff >
< link the QH to some other list >

For non-periodic EPs, dwc2_hcd_qh_unlink() does
list_del_init(>qh_list_entry), or for periodic EPs it calls
dwc2_deschedule_periodic() which in turn does the list_del_init().
This means the QH is removed from whatever list it was on.

So after the call to dwc2_hcd_qh_unlink(), the QH either needs to be freed
by calling dwc2_hcd_qh_free(), or it needs to be re-linked to another list,
otherwise the QH would be "lost" and could never be freed.

The places where I think a problem can happen are in dwc2_hcd_qh_deactivate(),
dwc_hcd_urb_dequeue(), and dwc_hcd_complete_xfer_ddma(). In most if not all
of these places, interrupts are disabled, which means that dwc2_hcd_qh_free()
cannot be called, since it can sleep. So maybe the freeing was omitted because
it was hard to do in these places?

What do you think, am I reading the code correctly and this could be a real
problem, or am I crazy? :)

-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: High CPU load produced by USB (DW2)

[Paul Zimmerman]

On Mon, 12 Mar 2018 12:06:55 +, Minas Harutyunyan 
<minas.harutyun...@synopsys.com> wrote:

> Hi Paul,
> 
> Nice to hear you  again! Hope you are doing well.

Thanks, same to you.

> On 3/8/2018 1:51 PM, Paul Zimmerman wrote:
> > On 03/06/2018 09:09 AM, Minas Harutyunyan wrote:
> >> Driver debug log not required.
> >> Based on lsusb output: to dwc2 port connected "Standard Microsystems
> >> Corp. USB 2.0 Hub" to which connected your HS device "SanDisk Corp.
> >> Ultra". Because of connected HUB, which have periodic endpoint
> >> (Interrupt IN EP1) like all HUB's, dwc2 forced in Buffer DMA mode unmask
> >> SOF interrupts.
> > 
> > Hmm. It seems to me that for hubs, where the interrupt EP is only used to
> > poll for connection changes (so the accuracy of the polling interval is not
> > important), an OS timer could be used instead of enabling SOF interrupts.
> > 
> > I have a Raspberry Pi 3 around here somewhere, maybe I'll dig it out and
> > try playing around with this idea. No promises though!
> > 
> 
> As I correctly understand, you suggest to mask SOF's and schedule in 
> dwc2 timer with expiration time 125us*interval. Based on timer 
> expiration get first urb from periodic queue and start transfer on 
> Interrupt IN EPn. In this case interrupt count will be decreased by 
> 'interval' times. But how you going to recognized inside dwc2 that this 
> Interrupt IN EPn is HUB endpoint or not?

Yes, you understand correctly. But on second thought, we would need to treat
*all* interrupt EPs this way, since things like mice and keyboards also have
interrupt EPs. But I'm not sure if any device really needs highly accurate
polling intervals on their interrupt EPs, so my idea may still work.

In any case, I have my RPI3 set up and I can build a working kernel for it
(sort of, but that's another story), so I will start experimenting with this.

-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: High CPU load produced by USB (DW2)

[Paul Zimmerman]

On 03/06/2018 09:09 AM, Minas Harutyunyan wrote:
> Hi,
> 
> On 3/6/2018 10:45 AM, Minas Harutyunyan wrote:
>> Hi,
>>
>> On 3/5/2018 11:14 PM, Marek Vasut wrote:
>>> On 02/20/2018 06:51 AM, Minas Harutyunyan wrote:
>>> [...]
>>> Is there a way to reduce that or is that the absolute minimum in HS 
>>> mode?
>>>
>> We already discussed, in this email thread earlier, why SOF interrupts
>> required and unmasked.
>> Only in case when connected device with CTRL+BLK EP's only (like flash
>> drive) and directly connected to cores root HUB, SOF's will be masked.
>
> That's the setup I have on Altera SoCFPGA, yet the SOFs are still present.
>
 Could you please send verbose lsusb on connected to dwc2 device
>>>
>>> See attached
>>>
 and driver debug log.
>>>
>>> What exactly do you mean by this one ?
>> Enable debugging messages and verbose debugging messages for dwc2 from
>> make menuconfig. Provide dmesg starting from dwc2 load till HS device
>> connected to dwc2 port and enumerated.
>>
>> Thanks,
>> Minas
>>
> Driver debug log not required.
> Based on lsusb output: to dwc2 port connected "Standard Microsystems
> Corp. USB 2.0 Hub" to which connected your HS device "SanDisk Corp.
> Ultra". Because of connected HUB, which have periodic endpoint
> (Interrupt IN EP1) like all HUB's, dwc2 forced in Buffer DMA mode unmask
> SOF interrupts.

Hmm. It seems to me that for hubs, where the interrupt EP is only used to
poll for connection changes (so the accuracy of the polling interval is not
important), an OS timer could be used instead of enabling SOF interrupts.

I have a Raspberry Pi 3 around here somewhere, maybe I'll dig it out and
try playing around with this idea. No promises though!

-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: stopping and starting a functionfs USB-device causes panic

[Paul Zimmerman]

andy_purc...@keysight.com wrote:
>>>
>>> I have a second issue with a functionfs USB-device implementation.
>>>
>>> The scenario is this:
>>> 1) USB-device app starts up, runs fine
>>> 2) ssh to the device, kill the app with CTRL-C
>>> 3) try to start the USB-device app 2nd time
>>>
>>> PANIC
>>>
>>> dmesg output:
>>> [ 2553.87] [ cut here ] [ 2553.87]
>>> kernel BUG at /var/lib/jenkins/workspace/Complete_PXYZ/.../kernel-
>>> source/fs/sysfs/file.c:332!
>>>
>>> [ 2553.87] Internal error: Oops - BUG: 0 [#1] PREEMPT ARM [
>>> 2553.87] Modules linked in: designware_udc usb_f_fs libcomposite
>>> udc_core fuse configfs autofs4
>> 
>> what is this designware_udc? We don't have that in mainline
>
> It is a driver for a very old ST Micro processor. 
> I now believe it is the cause of the problem. 
> The same user-space code for starting and stopping our USB function works
> on another platform. 
> So now I must look at the source code for this old ST Micro processor and
> compare it to our other platform. 

Most likely that is the old Synopsys UDC gadget controller, which is supported
by drivers/usb/gadget/udc/snps_udc_core.c. I suggest you take a look at that,
perhaps you can use that code and get rid of your out-of-tree driver altogether.

-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: g_mass_storage emulation of flash drive - difficulties with passing vendor/product ID

[Paul Zimmerman]

Alan Robertson wrote:

> On Sat, Jul 8, 2017 at 4:52 PM, Alan Stern  wrote:
>> On Sat, 8 Jul 2017, Alan Robertson wrote:
>>
>>> On Sat, Jul 8, 2017 at 2:04 AM, Alan Stern  
>>> wrote:

< snip >

>>> > Did you try enabling verbose debugging in g_mass_storage?  This
>>> > requires setting CONFIG_USB_GADGET_DEBUG and CONFIG_USB_GADGET_VERBOSE
>>> > in the kernel configuration and then rebuilding g_mass_storage.ko.
>>> > And at runtime you may need to enable dynamic debugging by doing
>>> >
>>> > echo 'module g_mass_storage =p' >/sys/kernel/dynamic_debug/control
>>> >
>>> > before plugging the device into anything.  Then compare the contents of
>>> > the dmesg log for the different scenarios.  In particular, look for
>>> > differences between plugging it into System 2 (say) right after boot
>>> > vs. after plugging it into System 1.
>>>
>>> OK interesting - I'll need to do some searching to understand how to
>>> change that and rebuild g_mass_storage.ko but I like the idea of
>>> seeing more detail about what is happening as the systems are closed
>>> so I'm unable to do any troubleshooting at that end.
>>
>> Do you have any experience in building a kernel for the Raspberry Pi?
>> The procedure is explained on several web sites, but you have to know
>> what you're doing.
>
> No I don't - I'll have a read over things to see what I can suss out.

The 'Linux File-Stor Gadget' string is part of the SCSI inquiry data, and
is set by the fsg_common_set_inquiry_string() function in f_mass_storage.c.
This must be what the problematic host systems are reacting to, and not any
of the USB info that you have overridden. It looks like there is a way to
override this string using configfs, perhaps Alan will know how to do this.

Hope this helps,
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: dwc3: gadget: Fix early exit in set/clear ep halt

[Paul Zimmerman]

On Tue, 13 Jun 2017 21:57:42 +, John Youn <john.y...@synopsys.com> wrote:

> On 6/13/2017 2:39 PM, Paul Zimmerman wrote:
> > On Tue, 13 Jun 2017 18:33:09 +, John Youn <john.y...@synopsys.com> 
> > wrote:
> >
> >> On 6/13/2017 12:32 AM, Felipe Balbi wrote:
> >>>
> >>> Hi,
> >>>
> >>> Thinh Nguyen <thinh.ngu...@synopsys.com> writes:
> >>>>>>>>>>>>>> this could be, I don't remember if I checked this or not :-)
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Really, the best way here, IMHO, would be to re-verify what's 
> >>>>>>>>>>>>>> going on
> >>>>>>>>>>>>>> with macOS and revert my orignal patch since it's, rather 
> >>>>>>>>>>>>>> clearly,
> >>>>>>>>>>>>>> wrong.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Sure. Are you going to make a revert patch or I am?
> >>>>>>>>>>>>
> >>>>>>>>>>>> Well, after we really know what's going on with macOS and have a 
> >>>>>>>>>>>> better
> >>>>>>>>>>>> fix, then who makes the revert is less important as long as 
> >>>>>>>>>>>> problems get
> >>>>>>>>>>>> sorted out :-) Either way is fine for me.
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Do you have any update on this issue?
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> The patch ffb80fc672c3 ("usb: dwc3: gadget: skip Set/Clear Halt 
> >>>>>>>>>> when
> >>>>>>>>>> invalid") still causes a regression for us. As there hasn't any 
> >>>>>>>>>> update
> >>>>>>>>>> for the macOS issue, can I submit a revert patch for this?
> >>>>>>>>>
> >>>>>>>>> I just came back from vacations ;-) I'll get back to this. Reverting
> >>>>>>>>> that commit won't do any good as we'd be exchanging one regression 
> >>>>>>>>> for
> >>>>>>>>> another. We really need to understand what's going on.
> >>>>>>>>
> >>>>>>>> Hi Felipe,
> >>>>>>>>
> >>>>>>>> I think we worked around this same issue in the Synopsys vendor 
> >>>>>>>> driver
> >>>>>>>> after a customer reported a problem with 
> >>>>>>>> CLEAR_FEATURE(ENDPOINT_HALT).
> >>>>>>>> I no longer have access to either the databook or the codebase, so I
> >>>>>>>> can't be sure about what the workaround was, but if either John or 
> >>>>>>>> Thinh
> >>>>>>>> can have a look at the Clear Stall code in the vendor driver they 
> >>>>>>>> should
> >>>>>>>> be able to figure it out.
> >>>>>>
> >>>>>> Thanks a lot Paul :-) Good to see you still have a look here every once
> >>>>>> in a while :-)
> >>>>>>
> >>>>>> John, Thinh could either of you check what Paul mentions here?
> >>>>>>
> >>>>>> cheers
> >>>>>>
> >>>>>
> >>>>> Can you provide more detail on the issue you see on MAC OS? and how to
> >>>>> reproduce the issue?
> >>>>>
> >>>>
> >>>> This issue has been a regression for us for a few months now, do you
> >>>> have any update on this?
> >>>
> >>> ordered a mac to test this again. It'll take a few days.
> >>>
> >>
> >> Thanks Felipe.
> >>
> >> Let us know if we can help in any way. If you can give details on your
> >> setup with macOS we can look into running it as well.
> >>
> >> Also curious whether or not you are seeing the same regression? It
> >> should apply to anyone using dwc3 running USB mass-storage CV.
> >
> > Hi John,
> >
> > Did you have a look at the Synopsys driver like I mentioned above?
> > I'm pretty sure I fixed something similar to this there.
> >
> 
> Hi Paul,
> 
> I haven't checked, but Thinh looked and couldn't find anything
> relating to the macOS issue. Which is why we want more information
> about the problem.
> 
> I can take a look at it too later today when I get a chance.
> 
> When you say you fixed the issue in the vendor driver, do you mean the
> macOS problem (multiple clear stalls) or the regression that it caused
> when it was fixed (the MSC data sequence issue)?

Hi John,

The issue was with the host sending a CLEAR_FEATURE(ENDPOINT_HALT) when the
EP was not actually halted. The host does this to reset the EP data toggle /
sequence number to 0, and it is allowed by some USB spec. The problem was
that for DWC3 if you issue a Clear Stall when the EP is not halted, nothing
happens. I *think* the fix was to issue a Set EP Config command instead,
which does reset the sequence number, but I'm not 100% sure about that.

-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: dwc3: gadget: Fix early exit in set/clear ep halt

[Paul Zimmerman]

On Tue, 13 Jun 2017 18:33:09 +, John Youn  wrote:

> On 6/13/2017 12:32 AM, Felipe Balbi wrote:
> >
> > Hi,
> >
> > Thinh Nguyen  writes:
>  this could be, I don't remember if I checked this or not :-)
> 
>  Really, the best way here, IMHO, would be to re-verify what's 
>  going on
>  with macOS and revert my orignal patch since it's, rather 
>  clearly,
>  wrong.
> 
> >>>
> >>> Sure. Are you going to make a revert patch or I am?
> >>
> >> Well, after we really know what's going on with macOS and have a 
> >> better
> >> fix, then who makes the revert is less important as long as 
> >> problems get
> >> sorted out :-) Either way is fine for me.
> >>
> >
> > Do you have any update on this issue?
> >
> 
>  The patch ffb80fc672c3 ("usb: dwc3: gadget: skip Set/Clear Halt when
>  invalid") still causes a regression for us. As there hasn't any 
>  update
>  for the macOS issue, can I submit a revert patch for this?
> >>>
> >>> I just came back from vacations ;-) I'll get back to this. Reverting
> >>> that commit won't do any good as we'd be exchanging one regression for
> >>> another. We really need to understand what's going on.
> >>
> >> Hi Felipe,
> >>
> >> I think we worked around this same issue in the Synopsys vendor driver
> >> after a customer reported a problem with CLEAR_FEATURE(ENDPOINT_HALT).
> >> I no longer have access to either the databook or the codebase, so I
> >> can't be sure about what the workaround was, but if either John or 
> >> Thinh
> >> can have a look at the Clear Stall code in the vendor driver they 
> >> should
> >> be able to figure it out.
> 
>  Thanks a lot Paul :-) Good to see you still have a look here every once
>  in a while :-)
> 
>  John, Thinh could either of you check what Paul mentions here?
> 
>  cheers
> 
> >>>
> >>> Can you provide more detail on the issue you see on MAC OS? and how to
> >>> reproduce the issue?
> >>>
> >>
> >> This issue has been a regression for us for a few months now, do you
> >> have any update on this?
> >
> > ordered a mac to test this again. It'll take a few days.
> >
> 
> Thanks Felipe.
> 
> Let us know if we can help in any way. If you can give details on your
> setup with macOS we can look into running it as well.
> 
> Also curious whether or not you are seeing the same regression? It
> should apply to anyone using dwc3 running USB mass-storage CV.

Hi John,

Did you have a look at the Synopsys driver like I mentioned above?
I'm pretty sure I fixed something similar to this there.

-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: dwc3: gadget: Fix early exit in set/clear ep halt

[Paul Zimmerman]

Forgot to CC linux-usb, doing that now

On Fri, 2 Jun 2017 16:27:56 -0700, Paul Zimmerman <pauld...@gmail.com> wrote:

> Felipe Balbi <ba...@kernel.org> writes:
> > Thinh Nguyen <thinh.ngu...@synopsys.com> writes:
> >>>>>> this could be, I don't remember if I checked this or not :-)
> >>>>>>
> >>>>>> Really, the best way here, IMHO, would be to re-verify what's going on
> >>>>>> with macOS and revert my orignal patch since it's, rather clearly,
> >>>>>> wrong.
> >>>>>>
> >>>>>
> >>>>> Sure. Are you going to make a revert patch or I am?
> >>>>
> >>>> Well, after we really know what's going on with macOS and have a better
> >>>> fix, then who makes the revert is less important as long as problems get
> >>>> sorted out :-) Either way is fine for me.
> >>>>
> >>> 
> >>> Do you have any update on this issue?
> >>> 
> >>
> >> The patch ffb80fc672c3 ("usb: dwc3: gadget: skip Set/Clear Halt when 
> >> invalid") still causes a regression for us. As there hasn't any update 
> >> for the macOS issue, can I submit a revert patch for this?
> >
> > I just came back from vacations ;-) I'll get back to this. Reverting
> > that commit won't do any good as we'd be exchanging one regression for
> > another. We really need to understand what's going on.
> 
> Hi Felipe,
> 
> I think we worked around this same issue in the Synopsys vendor driver
> after a customer reported a problem with CLEAR_FEATURE(ENDPOINT_HALT).
> I no longer have access to either the databook or the codebase, so I
> can't be sure about what the workaround was, but if either John or Thinh
> can have a look at the Clear Stall code in the vendor driver they should
> be able to figure it out.
> 
> Best Regards,
> Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 30/62] usb: dwc3: gadget: loop while (timeout)

[Paul Zimmerman]

Felipe Balbi  writes:

> instead of having infinite loop and always checking
> timeout value as a break condition, we can just
> decrement timeout inside while's condition.
> 
> Signed-off-by: Felipe Balbi 
> ---
>  drivers/usb/dwc3/gadget.c | 18 ++
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index efb758e5c6c7..8a16d8b2da8a 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -327,19 +327,13 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, 
> unsigned cmd,
>  
>   break;
>   }
> + } while (timeout--);

When 'timeout' reaches 0, the post-decrement means the value will be -1 when
the loop exits.

>  
> - /*
> -  * We can't sleep here, because it is also called from
> -  * interrupt context.
> -  */
> - timeout--;
> - if (!timeout) {
> - dwc3_trace(trace_dwc3_gadget,
> - "Command Timed Out");
> - ret = -ETIMEDOUT;
> - break;
> - }
> - } while (1);
> + if (timeout == 0) {

But here you are testing against 0 instead of -1.

> + dwc3_trace(trace_dwc3_gadget,
> + "Command Timed Out");
> + ret = -ETIMEDOUT;
> + }
>  
>   if (unlikely(susphy)) {
>   reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));

-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/22] usb: dwc3: gadget: clear LST from previous TRB on Update Transfer

[Paul Zimmerman]

Felipe Balbi  writes:

> If we're going to issue a Update Transfer command,
> let's clear LST bit from previous TRB. This will let
> us continue processing TRBs and convert previous IRQ
> into XferInProgress, instead of XferComplete.
> 
> Signed-off-by: Felipe Balbi 
> ---
>  drivers/usb/dwc3/gadget.c | 32 +++-
>  1 file changed, 23 insertions(+), 9 deletions(-)

< snip >

> + /*
> +  * When trying to issue Update Transfer, we can remove LST bit from
> +  * previous TRB and avoid a XferComplete
> +  */
> + if (!starting) {
> + trb = >trb_pool[dep->trb_enqueue - 1];
> + if (trb->ctrl & DWC3_TRB_CTRL_HWO)
> + trb->ctrl &= ~DWC3_TRB_CTRL_LST;

Hi Felipe,

This violates the DWC USB3 programming model. Once the HWO bit has been set
and the transfer started, the host is not allowed to modify any of the
fields in the TRB until the controller clears the HWO bit, or the transfer 
completes or is halted.

-- 
Paul


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xhci: handle no ping response error properly

[Paul Zimmerman]

Sorry, resending as plain text.

On Fri, Oct 9, 2015, Mathias Nyman wrote:

> + handling_skipped_tds = ep->skip &&
> + (trb_comp_code != COMP_MISSED_INT ||
   ^^
I think this should be &&, shouldn't it?

> +  trb_comp_code != COMP_PING_ERR);
-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mass storage behaviour

[Paul Zimmerman]

On Tue, Oct 6, 2015 at 10:01 AM, Alan Stern  wrote:
> On Tue, 6 Oct 2015, Felipe Balbi wrote:
>
>> >> In my experience, you need to do at least the following to get max
>> >> performance from the mass storage gadget:
>> >>
>> >> - Use Windows 8 or higher on the host. It's much faster than Linux.
>
> Why is Windows so much faster?  Or to put it another way, why is Linux
> slow?  How can we improve things?

I don't know. We were doing our performance demos using Windows, so we
never looked into why Linux was slower. But I do know the Microsoft
engineers put some effort into tuning their stack for good performance
at USB 3.0 speeds. I don't think anyone has done that for Linux yet.

-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mass storage behaviour

[Paul Zimmerman]

On Mon, 5 Oct 2015, Alan Stern wrote:
> On Mon, 5 Oct 2015, Paul Jones wrote:
>
>>> Increasing the max_sectors_kb value, on the other hand, might remove
>>> overhead by allowing a higher percentage of the transfer to consist of
>>> real data as opposed to CBW and CSW packets.  This depends to some
>>> extent on other factors (such as whether the memory pages are
>>> contiguous, allowing for larger transfers), but it can't hurt.
>>
>> I tried changing the max_sectors_kb to 64 with 64k block size in dd and it’s 
>> transferring at the same \
> speed.
>
> That's a decrease, not an increase.  Try changing it to 1024 or more.
>
>> I verified using usbmon and it then indeed requests 64k in each request.
>> Increasing the dd block size to 240k doesn’t change the transfer speed 
>> either, and it keeps using \
>> alternating 120k/8k requests. Increasing the dd block size to 1M doesn’t 
>> change the transfer speed \
>> either, although I get sequences of 2x 120k followed by 1x 16k requests.
>
> The dd block size makes no difference at all, because the kernel
> aggregates the requests from dd.

In my experience, you need to do at least the following to get max
performance from the mass storage gadget:

- Use Windows 8 or higher on the host. It's much faster than Linux.
- Put the backing file for the mass storage gadget on a tmpfs.
- Increase FSG_BUFLEN (in
drivers/usb/gadget/function/storage_common.h) to at least 128K.

-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Overly conservative xHCI bandwidth estimation

[Paul Zimmerman]

IIRC, at least some of the Intel controllers require the bandwidth
calculations to be done by the xHCI driver, instead of doing it
themselves in hardware. Perhaps you're tripping over a bug in the xHCI
driver? Mathias is probably best the one to comment on this.

-- 
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Overly conservative xHCI bandwidth estimation

[Paul Zimmerman]

Yep, that's the one.

-- 
Paul

On Thu, Sep 24, 2015 at 2:37 PM, Steinar H. Gunderson
<sgunder...@bigfoot.com> wrote:
> On Thu, Sep 24, 2015 at 02:33:06PM -0700, Paul Zimmerman wrote:
>> IIRC, at least some of the Intel controllers require the bandwidth
>> calculations to be done by the xHCI driver, instead of doing it
>> themselves in hardware. Perhaps you're tripping over a bug in the xHCI
>> driver? Mathias is probably best the one to comment on this.
>
> FWIW, I've checked that the XHCI_SW_BW_CHECKING quirk isn't set for my
> device. (I suppose that's the one you're thinking about?)
>
> /* Steinar */
> --
> Homepage: http://www.sesse.net/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Revert usb: dwc2: add bus suspend/resume for dwc2

[Paul Zimmerman]

This reverts commit 0cf884e819e05437287a668b9bfcc198bab6329c.
Even after applying the follow-on patch at
https://patchwork.kernel.org/patch/5325111
there are still problems with device connect on the Altera SOCFPGA
platform at least. One possible fix would be to add a whitelist
to enable suspend/resume on platforms where it does work correctly.

Signed-off-by: Paul Zimmerman pa...@synopsys.com
---
Based on Felipe's testing/next branch.

 drivers/usb/dwc2/hcd.c |   88 ++--
 1 files changed, 11 insertions(+), 77 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index a0cd9db..755e16b 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1473,30 +1473,6 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, 
u16 windex)
}
 }
 
-static void dwc2_port_resume(struct dwc2_hsotg *hsotg)
-{
-   u32 hprt0;
-
-   /* After clear the Stop PHY clock bit, we should wait for a moment
-* for PLL work stable with clock output.
-*/
-   writel(0, hsotg-regs + PCGCTL);
-   usleep_range(2000, 4000);
-
-   hprt0 = dwc2_read_hprt0(hsotg);
-   hprt0 |= HPRT0_RES;
-   writel(hprt0, hsotg-regs + HPRT0);
-   hprt0 = ~HPRT0_SUSP;
-   /* according to USB2.0 Spec 7.1.7.7, the host must send the resume
-* signal for at least 20ms
-*/
-   usleep_range(2, 25000);
-
-   hprt0 = ~HPRT0_RES;
-   writel(hprt0, hsotg-regs + HPRT0);
-   hsotg-lx_state = DWC2_L0;
-}
-
 /* Handles hub class-specific requests */
 static int dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq,
u16 wvalue, u16 windex, char *buf, u16 wlength)
@@ -1542,7 +1518,17 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg 
*hsotg, u16 typereq,
case USB_PORT_FEAT_SUSPEND:
dev_dbg(hsotg-dev,
ClearPortFeature USB_PORT_FEAT_SUSPEND\n);
-   dwc2_port_resume(hsotg);
+   writel(0, hsotg-regs + PCGCTL);
+   usleep_range(2, 4);
+
+   hprt0 = dwc2_read_hprt0(hsotg);
+   hprt0 |= HPRT0_RES;
+   writel(hprt0, hsotg-regs + HPRT0);
+   hprt0 = ~HPRT0_SUSP;
+   usleep_range(10, 15);
+
+   hprt0 = ~HPRT0_RES;
+   writel(hprt0, hsotg-regs + HPRT0);
break;
 
case USB_PORT_FEAT_POWER:
@@ -2315,55 +2301,6 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
usleep_range(1000, 3000);
 }
 
-static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
-{
-   struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
-   u32 hprt0;
-
-   if (!((hsotg-op_state == OTG_STATE_B_HOST) ||
-   (hsotg-op_state == OTG_STATE_A_HOST)))
-   return 0;
-
-   /* TODO: We get into suspend from 'on' state, maybe we need to do
-* something if we get here from DWC2_L1(LPM sleep) state one day.
-*/
-   if (hsotg-lx_state != DWC2_L0)
-   return 0;
-
-   hprt0 = dwc2_read_hprt0(hsotg);
-   if (hprt0  HPRT0_CONNSTS) {
-   dwc2_port_suspend(hsotg, 1);
-   } else {
-   u32 pcgctl = readl(hsotg-regs + PCGCTL);
-
-   pcgctl |= PCGCTL_STOPPCLK;
-   writel(pcgctl, hsotg-regs + PCGCTL);
-   }
-
-   return 0;
-}
-
-static int _dwc2_hcd_resume(struct usb_hcd *hcd)
-{
-   struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
-   u32 hprt0;
-
-   if (!((hsotg-op_state == OTG_STATE_B_HOST) ||
-   (hsotg-op_state == OTG_STATE_A_HOST)))
-   return 0;
-
-   if (hsotg-lx_state != DWC2_L2)
-   return 0;
-
-   hprt0 = dwc2_read_hprt0(hsotg);
-   if ((hprt0  HPRT0_CONNSTS)  (hprt0  HPRT0_SUSP))
-   dwc2_port_resume(hsotg);
-   else
-   writel(0, hsotg-regs + PCGCTL);
-
-   return 0;
-}
-
 /* Returns the current frame number */
 static int _dwc2_hcd_get_frame_number(struct usb_hcd *hcd)
 {
@@ -2734,9 +2671,6 @@ static struct hc_driver dwc2_hc_driver = {
.hub_status_data = _dwc2_hcd_hub_status_data,
.hub_control = _dwc2_hcd_hub_control,
.clear_tt_buffer_complete = _dwc2_hcd_clear_tt_buffer_complete,
-
-   .bus_suspend = _dwc2_hcd_suspend,
-   .bus_resume = _dwc2_hcd_resume,
 };
 
 /*
-- 
1.7.6.5

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] MAINTAINERS: update maintainer entry for dwc2 driver

[Paul Zimmerman]

Adding linux-usb and linux-kernel to CC.

-- 
Paul

 -Original Message-
 From: Paul Zimmerman
 Sent: Thursday, January 15, 2015 11:44 AM
 To: gre...@linuxfoundation.org; ba...@ti.com; John Youn 
 (johny...@synopsys.com)
 Cc: 'r.bald...@samsung.com'; 'dingu...@opensource.altera.com'; 'Kever Yang'; 
 'Mian Yousaf Kaukab';
 'Marek Szyprowski'; 'Yunzhi Li'; 'Jingoo Han'; 'Nick Hudson'
 Subject: [PATCH] MAINTAINERS: update maintainer entry for dwc2 driver
 
 Update the MAINTAINERS entry for the dwc2 driver to show John Youn
 as the new maintainer
 
 Signed-off-by: Paul Zimmerman pa...@synopsys.com
 ---
  MAINTAINERS |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
 
 diff --git a/MAINTAINERS b/MAINTAINERS
 index 3589d67..8096bea 100644
 --- a/MAINTAINERS
 +++ b/MAINTAINERS
 @@ -3028,7 +3028,7 @@ S:  Maintained
  F:   drivers/platform/x86/dell-wmi.c
 
  DESIGNWARE USB2 DRD IP DRIVER
 -M:   Paul Zimmerman pa...@synopsys.com
 +M:   John Youn johny...@synopsys.com
  L:   linux-usb@vger.kernel.org
  T:   git git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
  S:   Maintained
 --
 1.7.6.5

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND] MAINTAINERS: update maintainer entry for dwc2 driver

[Paul Zimmerman]

Update the MAINTAINERS entry for the dwc2 driver to show John Youn
as the new maintainer

Signed-off-by: Paul Zimmerman pa...@synopsys.com
---

Resending with linux-usb and linux-kernel in CC

 MAINTAINERS |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3589d67..8096bea 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3028,7 +3028,7 @@ S:Maintained
 F: drivers/platform/x86/dell-wmi.c
 
 DESIGNWARE USB2 DRD IP DRIVER
-M: Paul Zimmerman pa...@synopsys.com
+M: John Youn johny...@synopsys.com
 L: linux-usb@vger.kernel.org
 T: git git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
 S: Maintained
-- 
1.7.6.5

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2] usb: dwc2: call dwc2_is_controller_alive() under spinlock

[Paul Zimmerman]

 From: Robert Baldyga [mailto:r.bald...@samsung.com]
 Sent: Tuesday, January 13, 2015 10:46 PM
 
 This patch fixes bug described here:
 https://lkml.org/lkml/2014/12/22/185
 
 Signed-off-by: Robert Baldyga r.bald...@samsung.com

Although I don't understand *why* this fixes Robert's issue, it's
certainly a harmless patch, so

Acked-by: Paul Zimmerman pa...@synopsys.com

But I suspect Felipe will want a better changelog, I don't think just
a URL is good enough.

-- 
Paul

 ---
 
 Changelog:
 
 v2:
 - fixed comment from Paul Zimmerman
 
 v1: https://lkml.org/lkml/2015/1/13/186
 
  drivers/usb/dwc2/core_intr.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
 index ad43c5b..02e3e2d 100644
 --- a/drivers/usb/dwc2/core_intr.c
 +++ b/drivers/usb/dwc2/core_intr.c
 @@ -476,13 +476,13 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev)
   u32 gintsts;
   irqreturn_t retval = IRQ_NONE;
 
 + spin_lock(hsotg-lock);
 +
   if (!dwc2_is_controller_alive(hsotg)) {
   dev_warn(hsotg-dev, Controller is dead\n);
   goto out;
   }
 
 - spin_lock(hsotg-lock);
 -
   gintsts = dwc2_read_common_intr(hsotg);
   if (gintsts  ~GINTSTS_PRTINT)
   retval = IRQ_HANDLED;
 @@ -515,8 +515,8 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev)
   }
   }
 
 - spin_unlock(hsotg-lock);
  out:
 + spin_unlock(hsotg-lock);
   return retval;
  }
  EXPORT_SYMBOL_GPL(dwc2_handle_common_intr);
 --
 1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2] usb: dwc2: call dwc2_is_controller_alive() under spinlock

[Paul Zimmerman]

 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Wednesday, January 14, 2015 2:40 PM
 
 On Wed, Jan 14, 2015 at 10:28:54PM +, Paul Zimmerman wrote:
   From: Felipe Balbi [mailto:ba...@ti.com]
   Sent: Wednesday, January 14, 2015 1:46 PM
  
   On Wed, Jan 14, 2015 at 04:41:23PM -0500, Alan Stern wrote:
   This is really, really odd. Register accesses are atomic, so the 
   lock
   isn't really doing anything. Besides, you're calling
   dwc2_is_controller_alive() from within the IRQ handler, so IRQs 
   are
   already disabled.
 
  Spinlocks sometimes do more than you think.  For instance, here the
  lock prevents the register access from happening while some other 
  CPU
  is holding the lock.  If a silicon quirk causes the register access 
  to
  interfere with other activities, this could be important.

 readl() (which is used by dwc2_is_controller_alive()) adds a memory
 barrier to the register accesses, that should force all register
 accesses the be correctly ordered.
   
Memory barriers will order accesses that are all made on the same CPU
with respect to each other.  They do not order these accesses against
accesses made from another CPU -- that's why we have spinlocks.  :-)
  
   a fair point :-) The register is still read-only, so that shouldn't
   matter either :-)
  
  I fail to see how a silicon quirk
 could cause this and if, indeed, it does, I'd be more comfortable 
 with a
 proper STARS tickect number from synopsys :-s
   
Maybe accessing this register somehow resets something else.  I don't
know.  It seems unlikely, but at least it explains how adding a
spinlock could fix the problem.
  
   I would really need Paul (or someone at Synopsys) to confirm this
   somehow. Maybe it has something to do with how the register is
   implemented, dunno.
  
   Paul, do you have any idea what could cause this ? Could the HW into
   some weird state if we read GSNPSID at random locations or when data is
   being transferred, or anything like that ?
 
  Only thing I can think of is that there is some silicon bug in Robert's
  platform. But I am not aware of any STARs that mention accesses to the
  GSNPSID register as being problematic.
 
  Funny thing is, this code has been basically the same since at least
  November 2013. So I think some other recent change must have modified
  the timing of the register accesses, or something like that. But that's
  just handwaving, really.
 
 Alright, I'll apply this patch but for 3.20 with a stable tag as I have
 already sent my last pull request to Greg. Unless someone has a really
 big complaint about doing things as such.

It should go to 3.19-rc shouldn't it? It's a fix, and Robert's platform
is broken without it, IIUC.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2] usb: dwc2: call dwc2_is_controller_alive() under spinlock

[Paul Zimmerman]

 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Wednesday, January 14, 2015 1:46 PM
 
 On Wed, Jan 14, 2015 at 04:41:23PM -0500, Alan Stern wrote:
 This is really, really odd. Register accesses are atomic, so the lock
 isn't really doing anything. Besides, you're calling
 dwc2_is_controller_alive() from within the IRQ handler, so IRQs are
 already disabled.
   
Spinlocks sometimes do more than you think.  For instance, here the
lock prevents the register access from happening while some other CPU
is holding the lock.  If a silicon quirk causes the register access to
interfere with other activities, this could be important.
  
   readl() (which is used by dwc2_is_controller_alive()) adds a memory
   barrier to the register accesses, that should force all register
   accesses the be correctly ordered.
 
  Memory barriers will order accesses that are all made on the same CPU
  with respect to each other.  They do not order these accesses against
  accesses made from another CPU -- that's why we have spinlocks.  :-)
 
 a fair point :-) The register is still read-only, so that shouldn't
 matter either :-)
 
I fail to see how a silicon quirk
   could cause this and if, indeed, it does, I'd be more comfortable with a
   proper STARS tickect number from synopsys :-s
 
  Maybe accessing this register somehow resets something else.  I don't
  know.  It seems unlikely, but at least it explains how adding a
  spinlock could fix the problem.
 
 I would really need Paul (or someone at Synopsys) to confirm this
 somehow. Maybe it has something to do with how the register is
 implemented, dunno.
 
 Paul, do you have any idea what could cause this ? Could the HW into
 some weird state if we read GSNPSID at random locations or when data is
 being transferred, or anything like that ?

Only thing I can think of is that there is some silicon bug in Robert's
platform. But I am not aware of any STARs that mention accesses to the
GSNPSID register as being problematic.

Funny thing is, this code has been basically the same since at least
November 2013. So I think some other recent change must have modified
the timing of the register accesses, or something like that. But that's
just handwaving, really.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2] usb: dwc2: call dwc2_is_controller_alive() under spinlock

[Paul Zimmerman]

 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Wednesday, January 14, 2015 2:50 PM
 
 On Wed, Jan 14, 2015 at 10:45:26PM +, Paul Zimmerman wrote:
   From: Felipe Balbi [mailto:ba...@ti.com]
   Sent: Wednesday, January 14, 2015 2:40 PM
  
   On Wed, Jan 14, 2015 at 10:28:54PM +, Paul Zimmerman wrote:
 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Wednesday, January 14, 2015 1:46 PM

 On Wed, Jan 14, 2015 at 04:41:23PM -0500, Alan Stern wrote:
 This is really, really odd. Register accesses are atomic, so 
 the lock
 isn't really doing anything. Besides, you're calling
 dwc2_is_controller_alive() from within the IRQ handler, so 
 IRQs are
 already disabled.
   
Spinlocks sometimes do more than you think.  For instance, here 
the
lock prevents the register access from happening while some 
other CPU
is holding the lock.  If a silicon quirk causes the register 
access to
interfere with other activities, this could be important.
  
   readl() (which is used by dwc2_is_controller_alive()) adds a 
   memory
   barrier to the register accesses, that should force all register
   accesses the be correctly ordered.
 
  Memory barriers will order accesses that are all made on the same 
  CPU
  with respect to each other.  They do not order these accesses 
  against
  accesses made from another CPU -- that's why we have spinlocks.  :-)

 a fair point :-) The register is still read-only, so that shouldn't
 matter either :-)

I fail to see how a silicon quirk
   could cause this and if, indeed, it does, I'd be more comfortable 
   with a
   proper STARS tickect number from synopsys :-s
 
  Maybe accessing this register somehow resets something else.  I 
  don't
  know.  It seems unlikely, but at least it explains how adding a
  spinlock could fix the problem.

 I would really need Paul (or someone at Synopsys) to confirm this
 somehow. Maybe it has something to do with how the register is
 implemented, dunno.

 Paul, do you have any idea what could cause this ? Could the HW into
 some weird state if we read GSNPSID at random locations or when data 
 is
 being transferred, or anything like that ?
   
Only thing I can think of is that there is some silicon bug in Robert's
platform. But I am not aware of any STARs that mention accesses to the
GSNPSID register as being problematic.
   
Funny thing is, this code has been basically the same since at least
November 2013. So I think some other recent change must have modified
the timing of the register accesses, or something like that. But that's
just handwaving, really.
  
   Alright, I'll apply this patch but for 3.20 with a stable tag as I have
   already sent my last pull request to Greg. Unless someone has a really
   big complaint about doing things as such.
 
  It should go to 3.19-rc shouldn't it? It's a fix, and Robert's platform
  is broken without it, IIUC.
 
 It can also be categorized as has-never-worked-before before the code
 has been like this forever. Since we don't really have a git bisect
 result pointing to a commit that went in v3.19 merge window, I'm not
 sure how I can convince myself that this absolutely needs to be in
 v3.19.
 
 At a minimum, I need a proper bisection with a proper commit being
 blamed (even if it's a commit from months ago). From my point of view,
 debugging of this regression has not been finalized and we're just
 assuming it's caused by GSNPSID because moving that inside the
 spin_lock seems to fix the problem.

On further investigation, I was wrong about this code has been
basically the same since at least November 2013. Prior to commit
db8178c33db usb: dwc2: Update common interrupt handler to call gadget
interrupt handler from November 2014, the gadget interrupt handler
did not read from the GSNPSID register.

So likely the bug in Robert's hardware has been there all along, and
that commit just caused it to manifest itself.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: phy: make GPIOs optional for the generic phy

[Paul Zimmerman]

From 47bd18e210fecf701d493c27884e13c69bc449ea Mon Sep 17 00:00:00 2001
From: Paul Zimmerman pa...@synopsys.com
Date: Wed, 14 Jan 2015 18:15:58 -0800
Subject: [PATCH] usb: phy: make GPIOs optional for the generic phy

The use of GPIOs should be optional for the generic phy, otherwise
the Altera SOCFPGA platform at least is broken.

Fixes breakage caused by a combination of e9f2cefb0cd usb: phy: 
generic: migrate to gpio_desc and 135b3c4304d usb: dwc2: platform: 
add generic PHY framework support.

Signed-off-by: Paul Zimmerman pa...@synopsys.com
---
Based on top of testing/next.

 drivers/usb/phy/phy-generic.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index dd05254..9a826ff 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -218,10 +218,10 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
clk_rate = 0;
 
needs_vcc = of_property_read_bool(node, vcc-supply);
-   nop-gpiod_reset = devm_gpiod_get(dev, reset-gpios);
+   nop-gpiod_reset = devm_gpiod_get_optional(dev, reset-gpios);
err = PTR_ERR(nop-gpiod_reset);
if (!err) {
-   nop-gpiod_vbus = devm_gpiod_get(dev,
+   nop-gpiod_vbus = devm_gpiod_get_optional(dev,
 vbus-detect-gpio);
err = PTR_ERR(nop-gpiod_vbus);
}
-- 
1.7.6.5

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] usb: dwc2: call dwc2_is_controller_alive() under spinlock

[Paul Zimmerman]

 From: Robert Baldyga [mailto:r.bald...@samsung.com]
 Sent: Tuesday, January 13, 2015 2:58 AM
 
 This patch fixes bug described here:
 https://lkml.org/lkml/2014/12/22/185
 
 Signed-off-by: Robert Baldyga r.bald...@samsung.com
 ---
  drivers/usb/dwc2/core_intr.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
 index ad43c5b..668c8dd 100644
 --- a/drivers/usb/dwc2/core_intr.c
 +++ b/drivers/usb/dwc2/core_intr.c
 @@ -476,13 +476,13 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev)
   u32 gintsts;
   irqreturn_t retval = IRQ_NONE;
 
 + spin_lock(hsotg-lock);
 +
   if (!dwc2_is_controller_alive(hsotg)) {
   dev_warn(hsotg-dev, Controller is dead\n);
   goto out;

This isn't right, now the spinlock isn't released if we take this path.

Besides, this patch doesn't really make sense. How could taking the
spinlock affect the value returned from dwc2_is_controller_alive? All
it does is read from the GSNPSID register, and that register is never
written to.

Are you absolutely sure this is the fix?

-- 
Paul

   }
 
 - spin_lock(hsotg-lock);
 -
   gintsts = dwc2_read_common_intr(hsotg);
   if (gintsts  ~GINTSTS_PRTINT)
   retval = IRQ_HANDLED;
 --
 1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 00/30] usb: updates for dwc2 gadget driver

[Paul Zimmerman]

 From: Mian Yousaf Kaukab [mailto:yousaf.kau...@intel.com]
 Sent: Friday, January 09, 2015 4:39 AM
 
 Hi,
 This patchset consists of various bug fixes and feature enhancements for the
 dwc2 gadget driver. All the patches are verified on dwc2 v3.0a with dedicated
 fifos. Main focus of testing was with dma enabled. Although basic testing
 without dma was also done.
 
 It is based on testing/next branch in Felipe's git and
 
 Tested-by: Dinh Nguyen dingu...@opensource.altera.com
 
 Thank you,
 
 Best regards,
 Yousaf
 
 History:
 v3:
  - Fixed comment from Sergei Shtylyov
  - Updated usb: dwc2: gadget: don't process XferCompl on setup packet to
apply the check on endpoint 0 only.
  - Fixed regression in usb: dwc2: gadget: manage ep0 state in software for
dwc2 ip v2.93a, found by Dinh Nguyen.
 
 v2:
  - Rebased to Felipe's testing/next with https://lkml.org/lkml/2014/12/16/135
applied on top.
  - Fixed comments from  Robert Baldyga
  - Some cosmetic changes
  - Replaced usb: dwc2: gadget: process setup packet on transfer complete
with
usb: dwc2: gadget: don't process XferCompl on setup packet
  - Updated usb: dwc2: gadget: provide gadget handle to the phy
so that otg_set_peripheral is called in both udc_start and udc_stop.
 
 v1:
  - Addressed comments from Sergei Shtylyov
 
 Gregory Herrero (13):
   usb: dwc2: gadget: register gadget handle to the phy
   usb: dwc2: gadget: write correct value in ahbcfg register
   usb: dwc2: gadget: don't erase gahbcfg register when enabling dma
   usb: dwc2: gadget: add device tree property to enable dma
   Documentation: dt-bindings: add dt binding info for dwc2 g-use-dma
   usb: dwc2: gadget: configure fifos from device tree
   Documentation: dt-bindings: add dt binding info for dwc2 fifo resizing
   usb: dwc2: gadget: don't block after fifo flush timeout
   usb: dwc2: gadget: add vbus_session support
   usb: dwc2: gadget: reset fifo_map when initializing fifos
   usb: dwc2: gadget: fix pullup handling
   usb: dwc2: gadget: add vbus_draw support
   usb: dwc2: gadget: force gadget initialization in dev mode
 
 Mian Yousaf Kaukab (17):
   usb: dwc2: gadget: mask fifo empty irq with dma
   usb: dwc2: gadget: don't process XferCompl on setup packet
   usb: dwc2: gadget: don't embed ep0 buffers
   usb: dwc2: gadget: fix error path in dwc2_gadget_init
   usb: dwc2: gadget: add bi-directional endpoint support
   usb: dwc2: gadget: check interrupts for all endpoints
   usb: dwc2: gadget: remove unused members from hsotg_req
   usb: dwc2: gadget: fix debug loop limits
   usb: dwc2: gadget: consider all tx fifos
   usb: dwc2: gadget: kill requests after disabling ep
   usb: dwc2: gadget: manage ep0 state in software
   usb: dwc2: gadget: fix zero length packet transfers
   usb: dwc2: gadget: dont warn if endpoint is not enabled
   usb: dwc2: gadget: rename sent_zlp to send_zlp
   usb: dwc2: gadget: pick smallest acceptable fifo
   usb: dwc2: gadget: fix fifo allocation leak
   usb: dwc2: gadget: report disconnection after reset
 
  Documentation/devicetree/bindings/usb/dwc2.txt |   4 +
  drivers/usb/dwc2/core.h|  46 +-
  drivers/usb/dwc2/gadget.c  | 792 
 -
  drivers/usb/dwc2/hw.h  |   1 +
  4 files changed, 556 insertions(+), 287 deletions(-)
 
 --
 1.9.1

For the entire series:

Acked-by: Paul Zimmerman pa...@synopsys.com

Robert Baldyga r.bald...@samsung.com has also given his tested-by.

Felipe, can you please queue this series for 3.20? Thanks.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: dwc2: problems with IN requests completion in linux-next

[Paul Zimmerman]

 From: Robert Baldyga [mailto:r.bald...@samsung.com]
 Sent: Monday, December 22, 2014 6:13 AM
 
 I have recently noticed problem with DWC2 driver in latest linux-next. I
 use it in gadget only mode at Samsung platform (Odroid U3) but I believe
 the bug can be reproduced at another platforms.
 
 While running FFS example (tools/usb/ffs-aio-example/simple/) the
 communication breaks after few seconds. It's because one of IN requests
 enqueued in DWC2 driver do not complete. At USB analyzer I see that USB
 device started to transmit data from this request, but it ended incomplete.
 
 I bisected kernel tree, and I got following patches are reason of break.
 
 941fcce usb: dwc2: Update the gadget driver to use common dwc2_hsotg
 structure
 11b usb: dwc2: Move gadget probe function into platform code
 bcc0607 usb: dwc2: convert to use dev_pm_ops API
 510ffaa usb: dwc2: Initialize the USB core for peripheral mode
 db8178c usb: dwc2: Update common interrupt handler to call gadget
 interrupt handler
 8d736d8 usb: dwc2: gadget: Do not fail probe if there isn't a clock node
 
 Patch 941fcce breaks DWC2 driver at my platform and it starts to work
 from 8d736d8 but it has described bug.
 
 I will try to localize reason of this issue.

Hi Robert,

I think the most likely suspect would be db8178c, since the rest appear
to be init-time things. It seems to revert cleanly, can you try
reverting just that patch and see if it helps?

-- 
Paul

RE: New maintainer for dwc2

[Paul Zimmerman]

 From: gre...@linuxfoundation.org [mailto:gre...@linuxfoundation.org]
 Sent: Monday, January 12, 2015 5:02 PM
 
 On Mon, Jan 12, 2015 at 11:14:54PM +, Paul Zimmerman wrote:
  Hi everyone,
 
  I will be leaving Synopsys on Friday, Jan 16th, so the dwc2 driver
  will need a new maintainer.
 
  I am recommending John Youn johny...@synopsys.com as the new
  maintainer.
 
  On the plus side, John has quite a bit of experience with the dwc2
  controller, and has previous experience submitting kernel patches (for
  the xhci and dwc3 drivers). And being a Synopsys employee, he will have
  access to internal resources for support.
 
  On the minus side, John has not worked directly with the in-kernel dwc2
  driver until very recently, and has not done maintainer work before.
 
  If no one has any objections, I will submit a patch to MAINTAINERS in a
  couple of days making John the new dwc2 maintainer.
 
 I don't have any objections.  John will still be sending patches to
 Felipe through email, and not directly to me, right?

Yes, that is correct.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v7 3/5] usb: dwc2: add generic PHY framework support for dwc2 usb controler platform driver.

[Paul Zimmerman]

 From: Paul Zimmerman
 Sent: Saturday, January 10, 2015 3:52 PM
 
  From: Yunzhi Li [mailto:l...@rock-chips.com]
  Sent: Saturday, January 10, 2015 8:07 AM
 
  在 2015/1/9 10:15, Paul Zimmerman 写道:
   [...]
/*
   - * Attempt to find a generic PHY, then look for an old style
   - * USB PHY, finally fall back to pdata
   + * If platform probe couldn't find a generic PHY or an old style
   + * USB PHY, fall back to pdata
 */
   -phy = devm_phy_get(dev, usb2-phy);
   -if (IS_ERR(phy)) {
   -uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
   -if (IS_ERR(uphy)) {
   -/* Fallback for pdata */
   -plat = dev_get_platdata(dev);
   -if (!plat) {
   -dev_err(dev,
   -no platform data or transceiver 
   defined\n);
   -return -EPROBE_DEFER;
   -}
   -hsotg-plat = plat;
   -} else
   -hsotg-uphy = uphy;
   -} else {
   -hsotg-phy = phy;
   +if (IS_ERR_OR_NULL(hsotg-phy)  IS_ERR_OR_NULL(hsotg-uphy)) {
   +plat = dev_get_platdata(dev);
   +if (!plat) {
   +dev_err(dev,
   +no platform data or transceiver defined\n);
   +return -EPROBE_DEFER;
   Hi Yunzhi,
  
   Testing Felipe's testing/next branch on an Altera SOCFPGA platform,
   the driver never loads because it always returns -EPROBE_DEFER here.
   Apparently the SOCFPGA platform does not have any platform data
   defined, because dev_get_platdata() always returns NULL.
  
   If I remove the -EPROBE_DEFER return and have it continue on, the
   driver works. Reverting the patch also makes it work.
  
  When I debug this problem, I checked socfpga.dtsi, there is a
  usbphy node defined for each
  dwc2 controller, so I think when running dwc2_driver_probe() uphy =
  devm_usb_get_phy()
  should get a valid usbphy pointer and hsotg-uphy will not be NULL or
  ERROR, then in dwc2_gadget_init()
  it will not return -EPROBE_DEFER. I have no idea about why you meet
  -EPROBE_DEFER, could you please tell
  me what's the return value of devm_usb_get_phy() on your socfpga board ?
 
 I'm away from the hardware right now, but I just found this in a saved
 boot log:
 
 [1.097268] usb_phy_generic soc:usbphy@0: Error requesting RESET GPIO
 [1.097285] usb_phy_generic: probe of soc:usbphy@0 failed with error -2
 
 So that probably explains it. I'll dig into this some more on Monday.

The patch below fixes it. And it seems like the right thing to me,
since GPIOs should be optional for a generic phy, I would think. But
my device tree foo is very weak, so I'm not sure.

CCing Robert, who touched the generic phy code last. Robert, what do
you think?

-- 
Paul


diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index dd05254..9a826ff 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -218,10 +218,10 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
clk_rate = 0;
 
needs_vcc = of_property_read_bool(node, vcc-supply);
-   nop-gpiod_reset = devm_gpiod_get(dev, reset-gpios);
+   nop-gpiod_reset = devm_gpiod_get_optional(dev, reset-gpios);
err = PTR_ERR(nop-gpiod_reset);
if (!err) {
-   nop-gpiod_vbus = devm_gpiod_get(dev,
+   nop-gpiod_vbus = devm_gpiod_get_optional(dev,
 vbus-detect-gpio);
err = PTR_ERR(nop-gpiod_vbus);
}
-- 

New maintainer for dwc2

[Paul Zimmerman]

Hi everyone,

I will be leaving Synopsys on Friday, Jan 16th, so the dwc2 driver
will need a new maintainer.

I am recommending John Youn johny...@synopsys.com as the new
maintainer.

On the plus side, John has quite a bit of experience with the dwc2
controller, and has previous experience submitting kernel patches (for
the xhci and dwc3 drivers). And being a Synopsys employee, he will have
access to internal resources for support.

On the minus side, John has not worked directly with the in-kernel dwc2
driver until very recently, and has not done maintainer work before.

If no one has any objections, I will submit a patch to MAINTAINERS in a
couple of days making John the new dwc2 maintainer.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v7 3/5] usb: dwc2: add generic PHY framework support for dwc2 usb controler platform driver.

[Paul Zimmerman]

 From: Yunzhi Li [mailto:l...@rock-chips.com]
 Sent: Saturday, January 10, 2015 8:07 AM
 
 在 2015/1/9 10:15, Paul Zimmerman 写道:
  [...]
 /*
  -   * Attempt to find a generic PHY, then look for an old style
  -   * USB PHY, finally fall back to pdata
  +   * If platform probe couldn't find a generic PHY or an old style
  +   * USB PHY, fall back to pdata
  */
  -  phy = devm_phy_get(dev, usb2-phy);
  -  if (IS_ERR(phy)) {
  -  uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
  -  if (IS_ERR(uphy)) {
  -  /* Fallback for pdata */
  -  plat = dev_get_platdata(dev);
  -  if (!plat) {
  -  dev_err(dev,
  -  no platform data or transceiver defined\n);
  -  return -EPROBE_DEFER;
  -  }
  -  hsotg-plat = plat;
  -  } else
  -  hsotg-uphy = uphy;
  -  } else {
  -  hsotg-phy = phy;
  +  if (IS_ERR_OR_NULL(hsotg-phy)  IS_ERR_OR_NULL(hsotg-uphy)) {
  +  plat = dev_get_platdata(dev);
  +  if (!plat) {
  +  dev_err(dev,
  +  no platform data or transceiver defined\n);
  +  return -EPROBE_DEFER;
  Hi Yunzhi,
 
  Testing Felipe's testing/next branch on an Altera SOCFPGA platform,
  the driver never loads because it always returns -EPROBE_DEFER here.
  Apparently the SOCFPGA platform does not have any platform data
  defined, because dev_get_platdata() always returns NULL.
 
  If I remove the -EPROBE_DEFER return and have it continue on, the
  driver works. Reverting the patch also makes it work.
  When I debug this problem, I checked socfpga.dtsi, there is a
 usbphy node defined for each
 dwc2 controller, so I think when running dwc2_driver_probe() uphy =
 devm_usb_get_phy()
 should get a valid usbphy pointer and hsotg-uphy will not be NULL or
 ERROR, then in dwc2_gadget_init()
 it will not return -EPROBE_DEFER. I have no idea about why you meet
 -EPROBE_DEFER, could you please tell
 me what's the return value of devm_usb_get_phy() on your socfpga board ?

I'm away from the hardware right now, but I just found this in a saved
boot log:

[1.097268] usb_phy_generic soc:usbphy@0: Error requesting RESET GPIO
[1.097285] usb_phy_generic: probe of soc:usbphy@0 failed with error -2

So that probably explains it. I'll dig into this some more on Monday.

-- 
Paul

RE: [PATCH v3 00/30] usb: updates for dwc2 gadget driver

[Paul Zimmerman]

CCing more of the Samsung folks and linux-kernel.

Robert, Marek, can you test this series on your platform, please? If
I don't hear from you soon, I'll just ack it and we can deal with any
breakage later.

-- 
Paul

 From: Mian Yousaf Kaukab [mailto:yousaf.kau...@intel.com]
 Sent: Friday, January 09, 2015 4:39 AM
 
 Hi,
 This patchset consists of various bug fixes and feature enhancements for the
 dwc2 gadget driver. All the patches are verified on dwc2 v3.0a with dedicated
 fifos. Main focus of testing was with dma enabled. Although basic testing
 without dma was also done.
 
 It is based on testing/next branch in Felipe's git and
 
 Tested-by: Dinh Nguyen dingu...@opensource.altera.com
 
 Thank you,
 
 Best regards,
 Yousaf
 
 History:
 v3:
  - Fixed comment from Sergei Shtylyov
  - Updated usb: dwc2: gadget: don't process XferCompl on setup packet to
apply the check on endpoint 0 only.
  - Fixed regression in usb: dwc2: gadget: manage ep0 state in software for
dwc2 ip v2.93a, found by Dinh Nguyen.
 
 v2:
  - Rebased to Felipe's testing/next with https://lkml.org/lkml/2014/12/16/135
applied on top.
  - Fixed comments from  Robert Baldyga
  - Some cosmetic changes
  - Replaced usb: dwc2: gadget: process setup packet on transfer complete
with
usb: dwc2: gadget: don't process XferCompl on setup packet
  - Updated usb: dwc2: gadget: provide gadget handle to the phy
so that otg_set_peripheral is called in both udc_start and udc_stop.
 
 v1:
  - Addressed comments from Sergei Shtylyov
 
 Gregory Herrero (13):
   usb: dwc2: gadget: register gadget handle to the phy
   usb: dwc2: gadget: write correct value in ahbcfg register
   usb: dwc2: gadget: don't erase gahbcfg register when enabling dma
   usb: dwc2: gadget: add device tree property to enable dma
   Documentation: dt-bindings: add dt binding info for dwc2 g-use-dma
   usb: dwc2: gadget: configure fifos from device tree
   Documentation: dt-bindings: add dt binding info for dwc2 fifo resizing
   usb: dwc2: gadget: don't block after fifo flush timeout
   usb: dwc2: gadget: add vbus_session support
   usb: dwc2: gadget: reset fifo_map when initializing fifos
   usb: dwc2: gadget: fix pullup handling
   usb: dwc2: gadget: add vbus_draw support
   usb: dwc2: gadget: force gadget initialization in dev mode
 
 Mian Yousaf Kaukab (17):
   usb: dwc2: gadget: mask fifo empty irq with dma
   usb: dwc2: gadget: don't process XferCompl on setup packet
   usb: dwc2: gadget: don't embed ep0 buffers
   usb: dwc2: gadget: fix error path in dwc2_gadget_init
   usb: dwc2: gadget: add bi-directional endpoint support
   usb: dwc2: gadget: check interrupts for all endpoints
   usb: dwc2: gadget: remove unused members from hsotg_req
   usb: dwc2: gadget: fix debug loop limits
   usb: dwc2: gadget: consider all tx fifos
   usb: dwc2: gadget: kill requests after disabling ep
   usb: dwc2: gadget: manage ep0 state in software
   usb: dwc2: gadget: fix zero length packet transfers
   usb: dwc2: gadget: dont warn if endpoint is not enabled
   usb: dwc2: gadget: rename sent_zlp to send_zlp
   usb: dwc2: gadget: pick smallest acceptable fifo
   usb: dwc2: gadget: fix fifo allocation leak
   usb: dwc2: gadget: report disconnection after reset
 
  Documentation/devicetree/bindings/usb/dwc2.txt |   4 +
  drivers/usb/dwc2/core.h|  46 +-
  drivers/usb/dwc2/gadget.c  | 792 
 -
  drivers/usb/dwc2/hw.h  |   1 +
  4 files changed, 556 insertions(+), 287 deletions(-)
 
 --
 1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v7 3/5] usb: dwc2: add generic PHY framework support for dwc2 usb controler platform driver.

[Paul Zimmerman]

 From: Yunzhi Li [mailto:l...@rock-chips.com]
 Sent: Friday, December 12, 2014 7:10 AM
 
 Get PHY parameters from devicetree and power off usb PHY during
 system suspend.
 
 Signed-off-by: Yunzhi Li l...@rock-chips.com
 Acked-by: Paul Zimmerman pa...@synopsys.com
 
 ---
 
 Changes in v7: None
 Changes in v6: None
 Changes in v5: None
 Changes in v4: None
 Changes in v3:
 - Fix coding style: both branches of the if() which only one
   branch of the conditional statement is a single statement
   should have braces.
 - No need to test dwc2-phy for NULL before calling generic phy
   APIs.
 
  drivers/usb/dwc2/gadget.c   | 33 -
  drivers/usb/dwc2/platform.c | 36 ++--
  2 files changed, 46 insertions(+), 23 deletions(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 200168e..2601c61 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -3410,8 +3410,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
  {
   struct device *dev = hsotg-dev;
   struct s3c_hsotg_plat *plat = dev-platform_data;
 - struct phy *phy;
 - struct usb_phy *uphy;
   struct s3c_hsotg_ep *eps;
   int epnum;
   int ret;
 @@ -3421,30 +3419,23 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int 
 irq)
   hsotg-phyif = GUSBCFG_PHYIF16;
 
   /*
 -  * Attempt to find a generic PHY, then look for an old style
 -  * USB PHY, finally fall back to pdata
 +  * If platform probe couldn't find a generic PHY or an old style
 +  * USB PHY, fall back to pdata
*/
 - phy = devm_phy_get(dev, usb2-phy);
 - if (IS_ERR(phy)) {
 - uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
 - if (IS_ERR(uphy)) {
 - /* Fallback for pdata */
 - plat = dev_get_platdata(dev);
 - if (!plat) {
 - dev_err(dev,
 - no platform data or transceiver defined\n);
 - return -EPROBE_DEFER;
 - }
 - hsotg-plat = plat;
 - } else
 - hsotg-uphy = uphy;
 - } else {
 - hsotg-phy = phy;
 + if (IS_ERR_OR_NULL(hsotg-phy)  IS_ERR_OR_NULL(hsotg-uphy)) {
 + plat = dev_get_platdata(dev);
 + if (!plat) {
 + dev_err(dev,
 + no platform data or transceiver defined\n);
 + return -EPROBE_DEFER;

Hi Yunzhi,

Testing Felipe's testing/next branch on an Altera SOCFPGA platform,
the driver never loads because it always returns -EPROBE_DEFER here.
Apparently the SOCFPGA platform does not have any platform data
defined, because dev_get_platdata() always returns NULL.

If I remove the -EPROBE_DEFER return and have it continue on, the
driver works. Reverting the patch also makes it work.

I am testing with the driver built-in. I haven't tried it as a module
yet.

Any ideas? Is the -EPROBE_DEFER return really needed here?

-- 
Paul

 + }
 + hsotg-plat = plat;
 + } else if (hsotg-phy) {
   /*
* If using the generic PHY framework, check if the PHY bus
* width is 8-bit and set the phyif appropriately.
*/
 - if (phy_get_bus_width(phy) == 8)
 + if (phy_get_bus_width(hsotg-phy) == 8)
   hsotg-phyif = GUSBCFG_PHYIF8;
   }
 
 diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
 index 6a795aa..ae095f0 100644
 --- a/drivers/usb/dwc2/platform.c
 +++ b/drivers/usb/dwc2/platform.c
 @@ -155,6 +155,8 @@ static int dwc2_driver_probe(struct platform_device *dev)
   struct dwc2_core_params defparams;
   struct dwc2_hsotg *hsotg;
   struct resource *res;
 + struct phy *phy;
 + struct usb_phy *uphy;
   int retval;
   int irq;
 
 @@ -212,6 +214,24 @@ static int dwc2_driver_probe(struct platform_device *dev)
 
   hsotg-dr_mode = of_usb_get_dr_mode(dev-dev.of_node);
 
 + /*
 +  * Attempt to find a generic PHY, then look for an old style
 +  * USB PHY
 +  */
 + phy = devm_phy_get(dev-dev, usb2-phy);
 + if (IS_ERR(phy)) {
 + hsotg-phy = NULL;
 + uphy = devm_usb_get_phy(dev-dev, USB_PHY_TYPE_USB2);
 + if (IS_ERR(uphy))
 + hsotg-uphy = NULL;
 + else
 + hsotg-uphy = uphy;
 + } else {
 + hsotg-phy = phy;
 + phy_power_on(hsotg-phy);
 + phy_init(hsotg-phy);
 + }
 +
   spin_lock_init(hsotg-lock);
   mutex_init(hsotg-init_mutex);
   retval = dwc2_gadget_init(hsotg, irq);
 @@ -231,8 +251,15 @@ static int __maybe_unused dwc2_suspend(struct device 
 *dev)
   struct dwc2_hsotg *dwc2 = dev_get_drvdata(dev);
   int ret = 0;
 
 - if (dwc2_is_device_mode(dwc2

RE: [PATCH v3] usb: dwc2: add bus suspend/resume for dwc2

[Paul Zimmerman]

 From: Kever Yang [mailto:kever.y...@rock-chips.com]
 Sent: Wednesday, November 12, 2014 4:42 PM
 
 On 11/13/2014 07:22 AM, Doug Anderson wrote:
  Kever,
 
  On Mon, Nov 10, 2014 at 5:09 AM, Kever Yang kever.y...@rock-chips.com 
  wrote:
  Hcd controller needs bus_suspend/resume, dwc2 controller make
  root hub generate suspend/resume signal with hprt0 register
  when work in host mode.
  After the root hub enter suspend, we can make controller enter
  low power state with PCGCTL register.
 
  We also update the lx_state for hsotg state.
 
  This patch has tested on rk3288 with suspend/resume.
 
  Signed-off-by: Kever Yang kever.y...@rock-chips.com
  Acked-by: Paul Zimmerman pa...@synopsys.com
  ---
 
  Changes in v3:
  - remove CONFIG_PM macro for bus_suspend/resume
  - add PCGCTL operation for no device connect case
 
  Changes in v2:
  - update commit message
  - make dwc2 suspend/resume sourcecode work
 
drivers/usb/dwc2/hcd.c | 88 
  +++---
1 file changed, 77 insertions(+), 11 deletions(-)
  I would certainly appreciate confirmation, but my inclination is to
  NAK this change due to the fact that it regresses functionality.  I
  haven't done any serious review of it, but I've been testing it and it
  appears to break hotplug.
 
  Said another way, I did this:
 
  1. Without this patch, I booted with a USB stick in.  It was detected.
  I unplugged it, waited 5 seconds, and then plugged it back in.  The
  USB stick was redetcted.
 
  2. With this patch, I did the same thing.  The USB not redected after
  plugging it back in.
 With this patch, the dwc2 hcd/root hub will be auto suspend after device
 on port is disconnected, and it can't detect the device connect any more,
 I think that's the problem.
 
 I will figure out how to make dwc2 detect the device connect after auto
 suspend,
 or disable the auto suspend feature for the dwc2 hcd.

Kever,

This patch has made it into Linus' kernel as commit 0cf884e819e0, and
it breaks disconnect/connect on at least the Altera SOCFPGA platform.
I haven't been able to test it on any other platforms.

You need to submit a patch to either fix this, or to only enable this
feature for the Rock-chip platform. Otherwise the patch has to be
reverted.

-- 
Paul

N�r��yb�X��ǧv�^�)޺{.n�+{��^n�r���z���h����G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�

RE: [PATCH v3] usb: dwc2: add bus suspend/resume for dwc2

[Paul Zimmerman]

 From: Kever Yang [mailto:kever.y...@rock-chips.com]
 Sent: Monday, January 05, 2015 5:42 PM
 
 Hi Paul,
 
 I think you need this patch to fix the problem:
 
 usb: dwc2: resume root hub when device detect with suspend state
 https://patchwork.kernel.org/patch/5325111/
 
 Thanks,
 
 - Kever
 On 01/06/2015 09:23 AM, Paul Zimmerman wrote:
  From: Kever Yang [mailto:kever.y...@rock-chips.com]
  Sent: Wednesday, November 12, 2014 4:42 PM
 
  On 11/13/2014 07:22 AM, Doug Anderson wrote:
  Kever,
 
  On Mon, Nov 10, 2014 at 5:09 AM, Kever Yang kever.y...@rock-chips.com 
  wrote:
  Hcd controller needs bus_suspend/resume, dwc2 controller make
  root hub generate suspend/resume signal with hprt0 register
  when work in host mode.
  After the root hub enter suspend, we can make controller enter
  low power state with PCGCTL register.
 
  We also update the lx_state for hsotg state.
 
  This patch has tested on rk3288 with suspend/resume.
 
  Signed-off-by: Kever Yang kever.y...@rock-chips.com
  Acked-by: Paul Zimmerman pa...@synopsys.com
  ---
 
  Changes in v3:
  - remove CONFIG_PM macro for bus_suspend/resume
  - add PCGCTL operation for no device connect case
 
  Changes in v2:
  - update commit message
  - make dwc2 suspend/resume sourcecode work
 
 drivers/usb/dwc2/hcd.c | 88 
  +++---
 1 file changed, 77 insertions(+), 11 deletions(-)
  I would certainly appreciate confirmation, but my inclination is to
  NAK this change due to the fact that it regresses functionality.  I
  haven't done any serious review of it, but I've been testing it and it
  appears to break hotplug.
 
  Said another way, I did this:
 
  1. Without this patch, I booted with a USB stick in.  It was detected.
  I unplugged it, waited 5 seconds, and then plugged it back in.  The
  USB stick was redetcted.
 
  2. With this patch, I did the same thing.  The USB not redected after
  plugging it back in.
  With this patch, the dwc2 hcd/root hub will be auto suspend after device
  on port is disconnected, and it can't detect the device connect any more,
  I think that's the problem.
 
  I will figure out how to make dwc2 detect the device connect after auto
  suspend,
  or disable the auto suspend feature for the dwc2 hcd.
  Kever,
 
  This patch has made it into Linus' kernel as commit 0cf884e819e0, and
  it breaks disconnect/connect on at least the Altera SOCFPGA platform.
  I haven't been able to test it on any other platforms.
 
  You need to submit a patch to either fix this, or to only enable this
  feature for the Rock-chip platform. Otherwise the patch has to be
  reverted.

Unfortunately, after applying that patch there is another problem. If I
connect a device that causes an overcurrent condition to the root port,
then the port is dead after that. No further devices are detected until
after I reboot.

I tried adding another call to usb_hcd_resume_root_hub() in the
if (hprt0  HPRT0_OVRCURRCHG) branch, but then an overcurrent
condition causes a continuous stream of these messages:

[  133.348776] dwc2 ffb4.usb: GetPortStatus wIndex=0x0001 flags=0x0022
[  133.355717] dwc2 ffb4.usb: Overcurrent change detected
[  133.361179] dwc2 ffb4.usb:   HPRT0: 0x0002
[  133.365960] dwc2 ffb4.usb: port_status=0008
[  133.373236] hub 1-0:1.0: state 7 ports 1 chg  evt 
[  133.380038] hub 1-0:1.0: hub_suspend
[  133.384237] usb usb1: bus auto-suspend, wakeup 1
[  133.393631] dwc2 ffb4.usb: DWC OTG HCD HUB STATUS DATA: Root port status 
changed
[  133.401341] dwc2 ffb4.usb:   port_connect_status_change: 0
[  133.407157] dwc2 ffb4.usb:   port_reset_change: 0
[  133.412186] dwc2 ffb4.usb:   port_enable_change: 0
[  133.417310] dwc2 ffb4.usb:   port_suspend_change: 0
[  133.422519] dwc2 ffb4.usb:   port_over_current_change: 1
[  133.428154] usb usb1: suspend raced with wakeup event
[  133.433191] usb usb1: usb auto-resume
[  133.441522] hub 1-0:1.0: hub_resume
[  133.455505] dwc2 ffb4.usb: GetPortStatus wIndex=0x0001 flags=0x0022
[  133.462443] dwc2 ffb4.usb: Overcurrent change detected
[  133.467907] dwc2 ffb4.usb:   HPRT0: 0x0002
[  133.472684] dwc2 ffb4.usb: port_status=0008
[  133.480088] hub 1-0:1.0: state 7 ports 1 chg  evt 
[  133.485578] hub 1-0:1.0: hub_suspend
[  133.489157] usb usb1: bus auto-suspend, wakeup 1
[  133.493768] dwc2 ffb4.usb: _dwc2_hcd_suspend()
[  133.498540] dwc2 ffb4.usb: DWC OTG HCD HUB STATUS DATA: Root port status 
changed
[  133.506257] dwc2 ffb4.usb:   port_connect_status_change: 0
[  133.512065] dwc2 ffb4.usb:   port_reset_change: 0
[  133.517102] dwc2 ffb4.usb:   port_enable_change: 0
[  133.522218] dwc2 ffb4.usb:   port_suspend_change: 0
[  133.527428] dwc2 ffb4.usb:   port_over_current_change: 1
[  133.533069] usb usb1: suspend raced with wakeup event
[  133.538098] usb usb1: usb auto-resume
[  133.546439] hub 1-0:1.0: hub_resume

Any ideas?

-- 
Paul

RE: [PATCH v2 00/30] usb: updates for dwc2 gadget driver

[Paul Zimmerman]

Adding Dinh to CC.

Robert, Dinh, I would like to have your tested-bys for this series, please.

-- 
Paul

 From: Mian Yousaf Kaukab [mailto:yousaf.kau...@intel.com]
 Sent: Friday, January 02, 2015 6:42 AM
 
 Hi,
 This patchset consists of various bug fixes and feature enhancements for the
 dwc2 gadget driver. All the patches are verified on dwc2 v3.0a with dedicated
 fifos. Main focus of testing was with dma enabled. Although basic testing
 without dma was also done.
 
 It is based on testing/next branch in Felipe's git with
 https://lkml.org/lkml/2014/12/16/135 applied on top.
 
 Thank you,
 
 Best regards,
 Yousaf
 
 History:
 v2:
  - Rebased to Felipe's testing/next with https://lkml.org/lkml/2014/12/16/135
applied on top.
  - Fixed comments from  Robert Baldyga
  - Some cosmetic changes
  - Replaced usb: dwc2: gadget: process setup packet on transfer complete
with
usb: dwc2: gadget: don't process XferCompl on setup packet
  - Updated usb: dwc2: gadget: provide gadget handle to the phy
so that otg_set_peripheral is called in both udc_start and udc_stop.
 
 v1:
  - Addressed comments from Sergei Shtylyov
 
 Gregory Herrero (13):
   usb: dwc2: gadget: register gadget handle to the phy
   usb: dwc2: gadget: write correct value in ahbcfg register
   usb: dwc2: gadget: don't erase gahbcfg register when enabling dma
   usb: dwc2: gadget: add device tree property to enable dma
   Documentation: dt-bindings: add dt binding info for dwc2 g-use-dma
   usb: dwc2: gadget: configure fifos from device tree
   Documentation: dt-bindings: add dt binding info for dwc2 fifo resizing
   usb: dwc2: gadget: don't block after fifo flush timeout
   usb: dwc2: gadget: add vbus_session support
   usb: dwc2: gadget: reset fifo_map when initializing fifos
   usb: dwc2: gadget: fix pullup handling
   usb: dwc2: gadget: add vbus_draw support
   usb: dwc2: gadget: force gadget initialization in dev mode
 
 Mian Yousaf Kaukab (17):
   usb: dwc2: gadget: mask fifo empty irq with dma
   usb: dwc2: gadget: don't process XferCompl on setup packet
   usb: dwc2: gadget: don't embed ep0 buffers
   usb: dwc2: gadget: fix error path in dwc2_gadget_init
   usb: dwc2: gadget: add bi-directional endpoint support
   usb: dwc2: gadget: check interrupts for all endpoints
   usb: dwc2: gadget: remove unused members from hsotg_req
   usb: dwc2: gadget: fix debug loop limits
   usb: dwc2: gadget: consider all tx fifos
   usb: dwc2: gadget: kill requests after disabling ep
   usb: dwc2: gadget: manage ep0 state in software
   usb: dwc2: gadget: fix zero length packet transfers
   usb: dwc2: gadget: dont warn if endpoint is not enabled
   usb: dwc2: gadget: rename sent_zlp to send_zlp
   usb: dwc2: gadget: pick smallest acceptable fifo
   usb: dwc2: gadget: fix fifo allocation leak
   usb: dwc2: gadget: report disconnection after reset
 
  Documentation/devicetree/bindings/usb/dwc2.txt |   4 +
  drivers/usb/dwc2/core.h|  46 +-
  drivers/usb/dwc2/gadget.c  | 786 
 -
  drivers/usb/dwc2/hw.h  |   1 +
  4 files changed, 550 insertions(+), 287 deletions(-)
 
 --
 1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] usb: dwc2: add support for initial state for dual-role mode

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Wednesday, December 17, 2014 5:50 AM
 
 DWC2 module on Exynos SoCs is used only as peripheral controller (UDC),
 so add support for selecting default initial state for dual-role mode. The
 default init mode can be overridden by device tree 'dr_mode' property.
 This patch enables to use usb gadget on Exynos SoCs, when DWC2 driver has
 been compiled as 'dual-role driver'.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com

Hi Marek,

Why can't you just set dr_mode=USB_DR_MODE_PERIPHERAL in your platform's
device tree data?

-- 
Paul

 ---
  drivers/usb/dwc2/core.h |  3 +++
  drivers/usb/dwc2/platform.c | 62 
 ++---
  2 files changed, 56 insertions(+), 9 deletions(-)
 
 diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
 index 7a70a13..b0fed4e 100644
 --- a/drivers/usb/dwc2/core.h
 +++ b/drivers/usb/dwc2/core.h
 @@ -354,6 +354,7 @@ struct dwc2_core_params {
   int reload_ctl;
   int ahbcfg;
   int uframe_sched;
 + int default_dr_mode;
  };
 
  /**
 @@ -570,6 +571,8 @@ struct dwc2_hsotg {
   struct dwc2_core_params *core_params;
   enum usb_otg_state op_state;
   enum usb_dr_mode dr_mode;
 + unsigned int hcd_enabled:1;
 + unsigned int gadget_enabled:1;
 
   struct phy *phy;
   struct usb_phy *uphy;
 diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
 index 6a795aa..8abf437 100644
 --- a/drivers/usb/dwc2/platform.c
 +++ b/drivers/usb/dwc2/platform.c
 @@ -76,6 +76,7 @@ static const struct dwc2_core_params params_bcm2835 = {
   .reload_ctl = 0,
   .ahbcfg = 0x10,
   .uframe_sched   = 0,
 + .default_dr_mode= USB_DR_MODE_OTG,
  };
 
  static const struct dwc2_core_params params_rk3066 = {
 @@ -104,6 +105,36 @@ static const struct dwc2_core_params params_rk3066 = {
   .reload_ctl = -1,
   .ahbcfg = 0x7, /* INCR16 */
   .uframe_sched   = -1,
 + .default_dr_mode= USB_DR_MODE_OTG,
 +};
 +
 +static const struct dwc2_core_params params_s3c_hsotg = {
 + .otg_cap= -1,
 + .otg_ver= -1,
 + .dma_enable = 0,
 + .dma_desc_enable= 0,
 + .speed  = -1,
 + .enable_dynamic_fifo= -1,
 + .en_multiple_tx_fifo= -1,
 + .host_rx_fifo_size  = -1,
 + .host_nperio_tx_fifo_size   = -1,
 + .host_perio_tx_fifo_size= -1,
 + .max_transfer_size  = -1,
 + .max_packet_count   = -1,
 + .host_channels  = -1,
 + .phy_type   = -1,
 + .phy_utmi_width = -1,
 + .phy_ulpi_ddr   = -1,
 + .phy_ulpi_ext_vbus  = -1,
 + .i2c_enable = -1,
 + .ulpi_fs_ls = -1,
 + .host_support_fs_ls_low_power   = -1,
 + .host_ls_low_power_phy_clk  = -1,
 + .ts_dline   = -1,
 + .reload_ctl = -1,
 + .ahbcfg = -1,
 + .uframe_sched   = -1,
 + .default_dr_mode= USB_DR_MODE_PERIPHERAL,
  };
 
  /**
 @@ -121,8 +152,10 @@ static int dwc2_driver_remove(struct platform_device 
 *dev)
  {
   struct dwc2_hsotg *hsotg = platform_get_drvdata(dev);
 
 - dwc2_hcd_remove(hsotg);
 - s3c_hsotg_remove(hsotg);
 + if (hsotg-hcd_enabled)
 + dwc2_hcd_remove(hsotg);
 + if (hsotg-gadget_enabled)
 + s3c_hsotg_remove(hsotg);
 
   return 0;
  }
 @@ -131,7 +164,7 @@ static const struct of_device_id dwc2_of_match_table[] = {
   { .compatible = brcm,bcm2835-usb, .data = params_bcm2835 },
   { .compatible = rockchip,rk3066-usb, .data = params_rk3066 },
   { .compatible = snps,dwc2, .data = NULL },
 - { .compatible = samsung,s3c6400-hsotg, .data = NULL},
 + { .compatible = samsung,s3c6400-hsotg, .data = params_s3c_hsotg },
   {},
  };
  MODULE_DEVICE_TABLE(of, dwc2_of_match_table);
 @@ -211,15 +244,26 @@ static int dwc2_driver_probe(struct platform_device 
 *dev)
   (unsigned long)res-start, hsotg-regs);
 
   hsotg-dr_mode = of_usb_get_dr_mode(dev-dev.of_node);
 + if (hsotg-dr_mode == USB_DR_MODE_UNKNOWN 
 + params-default_dr_mode  USB_DR_MODE_UNKNOWN)
 + hsotg-dr_mode = params-default_dr_mode;
 
   spin_lock_init(hsotg-lock);
   mutex_init(hsotg-init_mutex);
 - retval = dwc2_gadget_init(hsotg, irq);
 - if (retval)
 - return retval;
 - retval = dwc2_hcd_init(hsotg, irq, params);
 - if (retval)
 - return retval;
 +
 + if (hsotg-dr_mode != USB_DR_MODE_HOST) {
 + retval = 

RE: [PATCH] drivers: usb: dwc2: remove 'force' parameter from kill_all_requests()

[Paul Zimmerman]

 From: Robert Baldyga [mailto:r.bald...@samsung.com]
 Sent: Tuesday, December 16, 2014 2:52 AM
 
 This patch fixes in simpler way the bug described in [1] and [2]. It
 looks like DWC2 is the only UDC driver that doesn't force usb requests
 to complete in ep_disable() function. This causes described problem,
 because we have no guarantee that all requests will be completed before
 unbind of usb function.
 
 To fix this problem we force all requests of disabled endpoint to complete.
 Also currently running request is not handled. This allowed to simplify
 code of kill_all_requests() function, because 'force' parameter is always
 set to true, so we don't need it anymore.
 
 In s3c_hsotg_rx_data() we change function used to print message when active
 request is NULL from dev_warn() to dev_dbg(), because such situation is
 harmless for driver and now it can take place during normal endpoint
 disabling.
 
 [1] https://lkml.org/lkml/2014/12/9/283
 [2] https://lkml.org/lkml/2014/12/12/360
 
 Signed-off-by: Robert Baldyga r.bald...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 23 ---
  1 file changed, 8 insertions(+), 15 deletions(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 200168e..fee0ad5 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -1305,7 +1305,7 @@ static void s3c_hsotg_rx_data(struct dwc2_hsotg *hsotg, 
 int ep_idx, int size)
   u32 epctl = readl(hsotg-regs + DOEPCTL(ep_idx));
   int ptr;
 
 - dev_warn(hsotg-dev,
 + dev_dbg(hsotg-dev,
%s: FIFO %d bytes on ep%d but no req 
 (DXEPCTl=0x%08x)\n,
__func__, size, ep_idx, epctl);
 
 @@ -1988,30 +1988,23 @@ static void s3c_hsotg_irq_enumdone(struct dwc2_hsotg 
 *hsotg)
   * @hsotg: The device state.
   * @ep: The endpoint the requests may be on.
   * @result: The result code to use.
 - * @force: Force removal of any current requests
   *
   * Go through the requests on the given endpoint and mark them
   * completed with the given result code.
   */
  static void kill_all_requests(struct dwc2_hsotg *hsotg,
 struct s3c_hsotg_ep *ep,
 -   int result, bool force)
 +   int result)
  {
   struct s3c_hsotg_req *req, *treq;
   unsigned size;
 
 - list_for_each_entry_safe(req, treq, ep-queue, queue) {
 - /*
 -  * currently, we can't do much about an already
 -  * running request on an in endpoint
 -  */
 -
 - if (ep-req == req  ep-dir_in  !force)
 - continue;
 + ep-req = NULL;
 
 + list_for_each_entry_safe(req, treq, ep-queue, queue)
   s3c_hsotg_complete_request(hsotg, ep, req,
  result);
 - }
 +
   if (!hsotg-dedicated_fifos)
   return;
   size = (readl(hsotg-regs + DTXFSTS(ep-index))  0x) * 4;
 @@ -2036,7 +2029,7 @@ void s3c_hsotg_disconnect(struct dwc2_hsotg *hsotg)
 
   hsotg-connected = 0;
   for (ep = 0; ep  hsotg-num_of_eps; ep++)
 - kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN, true);
 + kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN);
 
   call_gadget(hsotg, disconnect);
  }
 @@ -2334,7 +2327,7 @@ irq_retry:
  msecs_to_jiffies(200))) {
 
   kill_all_requests(hsotg, hsotg-eps[0],
 -   -ECONNRESET, true);
 +   -ECONNRESET);
 
   s3c_hsotg_core_init_disconnected(hsotg);
   s3c_hsotg_core_connect(hsotg);
 @@ -2588,7 +2581,7 @@ static int s3c_hsotg_ep_disable(struct usb_ep *ep)
 
   spin_lock_irqsave(hsotg-lock, flags);
   /* terminate all requests with shutdown */
 - kill_all_requests(hsotg, hs_ep, -ESHUTDOWN, false);
 + kill_all_requests(hsotg, hs_ep, -ESHUTDOWN);
 
   hsotg-fifo_map = ~(1hs_ep-fifo_index);
   hs_ep-fifo_index = 0;
 --
 1.9.1

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: query on DWC3

[Paul Zimmerman]

 From: linux-usb-ow...@vger.kernel.org 
 [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of sundeep subbaraya
 Sent: Friday, December 12, 2014 9:13 PM
 
 Hi Felipe,
 
 In DWC3 driver, for three stage Control OUT transfer there is a check:
 
else if (!IS_ALIGNED(req-request.length, dep-endpoint.maxpacket)
 
   (dep-number == 0)) {.
 }
 
 I understand that we check for alignment of sizes and if not we
 prepare trb with maxpacket and enable interrupt on short packet. In
 databook I could not find anything related to this, it talks only
 about addresses should be aligned. In Control transfer programming
 model there is no difference between Control OUT or IN transfer, if
 three stage transfer - prepare trb with length wLength. Initially I
 followed this and not able to receive data on EP0 OUT.:( .After adding
 the above check it is working. Please help me to understand why we do
 this?

Hi Sundeep,

Please see section 8.2.3.3 Buffer Size Rules and Zero-Length Packets
in the databook:

For OUT endpoints, the following rules apply:

- The total size of a Buffer Descriptor must be a multiple of
  MaxPacketSize

The wording may be a little confusing, it actually means that the size
of the data buffer for OUT endpoints must be a multiple of MaxPacketSize.
Section 8.2.5 states it more clearly:

- An OUT transfer’s transfer size (Total TRB buffer allocation)
  must be a multiple of MaxPacketSize even if software is expecting
a fixed non-multiple of MaxPacketSize transfer from the Host.

This rule applies to all OUT endpoint types, including Control endpoints.

-- 
Paul

N�r��yb�X��ǧv�^�)޺{.n�+{��^n�r���z���h����G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�

RE: [PATCH] usb: dwc2: gadget: kill requests with 'force' in s3c_hsotg_udc_stop()

[Paul Zimmerman]

 From: Robert Baldyga [mailto:r.bald...@samsung.com]
 Sent: Tuesday, December 09, 2014 5:42 AM
 
 This makes us sure that all requests are completed before we unbind
 gadget. There are assumptions in gadget API that all requests have to
 be completed and leak of complete can break some usb function drivers.
 
 For example unbind of ECM function can cause NULL pointer dereference:
 
 [   26.396595] configfs-gadget gadget: unbind function
 'cdc_ethernet'/e79c4c00
 [   26.414999] Unable to handle kernel NULL pointer dereference at
 virtual address 
 (...)
 [   26.452223] PC is at ecm_unbind+0x6c/0x9c
 [   26.456209] LR is at ecm_unbind+0x68/0x9c
 (...)
 [   26.603696] [c033fdb4] (ecm_unbind) from [c033661c]
 (purge_configs_funcs+0x94/0xd8)
 [   26.611674] [c033661c] (purge_configs_funcs) from [c0336674]
 (configfs_composite_unbind+0x14/0x34)
 [   26.620961] [c0336674] (configfs_composite_unbind) from
 [c0337124] (usb_gadget_remove_driver+0x68/0x9c)
 [   26.630683] [c0337124] (usb_gadget_remove_driver) from [c03376c8]
 (usb_gadget_unregister_driver+0x64/0x94)
 [   26.640664] [c03376c8] (usb_gadget_unregister_driver) from
 [c0336be8] (unregister_gadget+0x20/0x3c)
 [   26.650038] [c0336be8] (unregister_gadget) from [c0336c84]
 (gadget_dev_desc_UDC_store+0x80/0xb8)
 [   26.659152] [c0336c84] (gadget_dev_desc_UDC_store) from
 [c0335120] (gadget_info_attr_store+0x1c/0x28)
 [   26.668703] [c0335120] (gadget_info_attr_store) from [c012135c]
 (configfs_write_file+0xe8/0x148)
 [   26.677818] [c012135c] (configfs_write_file) from [c00c8dd4]
 (vfs_write+0xb0/0x1a0)
 [   26.685801] [c00c8dd4] (vfs_write) from [c00c91b8]
 (SyS_write+0x44/0x84)
 [   26.692834] [c00c91b8] (SyS_write) from [c000e560]
 (ret_fast_syscall+0x0/0x30)
 [   26.700381] Code: e30409f8 e34c0069 eb07b88d e59430a8 (e593)
 [   26.706485] ---[ end trace f62a082b323838a2 ]---
 
 It's because in some cases request is still running on endpoint during
 unbind and kill_all_requests() called from s3c_hsotg_udc_stop() function
 doesn't cause call of complete() of request. Missing complete() call
 causes ecm-notify_req equals NULL in ecm_unbind() function, and this
 is reason of this bug.
 
 Similar breaks can be observed in another usb function drivers.
 
 This patch fixes this bug forcing usb request completion in when
 s3c_hsotg_ep_disable() is called from s3c_hsotg_udc_stop().
 
 Signed-off-by: Robert Baldyga r.bald...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 10 +++---
  1 file changed, 7 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 8b5c079..cb4c925 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2590,7 +2590,7 @@ error:
   * s3c_hsotg_ep_disable - disable given endpoint
   * @ep: The endpoint to disable.
   */
 -static int s3c_hsotg_ep_disable(struct usb_ep *ep)
 +static int s3c_hsotg_ep_disable_force(struct usb_ep *ep, bool force)
  {
   struct s3c_hsotg_ep *hs_ep = our_ep(ep);
   struct s3c_hsotg *hsotg = hs_ep-parent;
 @@ -2611,7 +2611,7 @@ static int s3c_hsotg_ep_disable(struct usb_ep *ep)
 
   spin_lock_irqsave(hsotg-lock, flags);
   /* terminate all requests with shutdown */
 - kill_all_requests(hsotg, hs_ep, -ESHUTDOWN, false);
 + kill_all_requests(hsotg, hs_ep, -ESHUTDOWN, force);
 
   hsotg-fifo_map = ~(1hs_ep-fifo_index);
   hs_ep-fifo_index = 0;
 @@ -2632,6 +2632,10 @@ static int s3c_hsotg_ep_disable(struct usb_ep *ep)
   return 0;
  }
 
 +static int s3c_hsotg_ep_disable(struct usb_ep *ep)
 +{
 + return s3c_hsotg_ep_disable_force(ep, false);
 +}
  /**
   * on_list - check request is on the given endpoint
   * @ep: The endpoint to check.
 @@ -2933,7 +2937,7 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 
   /* all endpoints should be shutdown */
   for (ep = 1; ep  hsotg-num_of_eps; ep++)
 - s3c_hsotg_ep_disable(hsotg-eps[ep].ep);
 + s3c_hsotg_ep_disable_force(hsotg-eps[ep].ep, true);
 
   spin_lock_irqsave(hsotg-lock, flags);
  

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 3/5] usb: dwc2: Add generic PHY framework support for dwc2 usb controler platform driver.

[Paul Zimmerman]

 From: Yunzhi Li [mailto:l...@rock-chips.com]
 Sent: Sunday, December 07, 2014 7:58 PM
 
 On 2014/12/6 3:04, Paul Zimmerman wrote:
  From: Yunzhi Li [mailto:l...@rock-chips.com]
  Sent: Friday, December 05, 2014 4:52 AM
 
  Get PHY parameters from devicetree and power off usb PHY during
  system suspend.
 
  Signed-off-by: Yunzhi Li l...@rock-chips.com
  ---
 
drivers/usb/dwc2/gadget.c   | 33 -
drivers/usb/dwc2/platform.c | 34 ++
2 files changed, 46 insertions(+), 21 deletions(-)
 
  diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
  index 200168e..2601c61 100644
  --- a/drivers/usb/dwc2/gadget.c
  +++ b/drivers/usb/dwc2/gadget.c
  [...]
 
 /*
  -   * Attempt to find a generic PHY, then look for an old style
  -   * USB PHY, finally fall back to pdata
  +   * If platform probe couldn't find a generic PHY or an old style
  +   * USB PHY, fall back to pdata
  */
  -  phy = devm_phy_get(dev, usb2-phy);
  -  if (IS_ERR(phy)) {
  -  uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
  -  if (IS_ERR(uphy)) {
  -  /* Fallback for pdata */
  -  plat = dev_get_platdata(dev);
  -  if (!plat) {
  -  dev_err(dev,
  -  no platform data or transceiver defined\n);
  -  return -EPROBE_DEFER;
  -  }
  -  hsotg-plat = plat;
  -  } else
  -  hsotg-uphy = uphy;
  -  } else {
  -  hsotg-phy = phy;
  +  if (IS_ERR_OR_NULL(hsotg-phy)  IS_ERR_OR_NULL(hsotg-uphy)) {
  +  plat = dev_get_platdata(dev);
  +  if (!plat) {
  +  dev_err(dev,
  +  no platform data or transceiver defined\n);
  +  return -EPROBE_DEFER;
  +  }
  +  hsotg-plat = plat;
  +  } else if (hsotg-phy) {
  You have changed the behavior here. Previously, the driver would work
  even if there were no phys or pdata defined. Now it will return
  -EPROBE_DEFER instead. Are you sure that won't break any existing
  platforms?
 
 I don't really catch your meaning. Could you please point out where is
 the difference? Thanks .

Yeah, sorry, I misread the patch. I think your new version is fine.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 3/5] usb: dwc2: add generic PHY framework support for dwc2 usb controler platform driver.

[Paul Zimmerman]

);
 +
 + }
   return ret;
  }
 

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 3/5] usb: dwc2: Add generic PHY framework support for dwc2 usb controler platform driver.

[Paul Zimmerman]

 From: Yunzhi Li [mailto:l...@rock-chips.com]
 Sent: Friday, December 05, 2014 4:52 AM
 
 Get PHY parameters from devicetree and power off usb PHY during
 system suspend.
 
 Signed-off-by: Yunzhi Li l...@rock-chips.com
 ---
 
  drivers/usb/dwc2/gadget.c   | 33 -
  drivers/usb/dwc2/platform.c | 34 ++
  2 files changed, 46 insertions(+), 21 deletions(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 200168e..2601c61 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -3410,8 +3410,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
  {
   struct device *dev = hsotg-dev;
   struct s3c_hsotg_plat *plat = dev-platform_data;
 - struct phy *phy;
 - struct usb_phy *uphy;
   struct s3c_hsotg_ep *eps;
   int epnum;
   int ret;
 @@ -3421,30 +3419,23 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int 
 irq)
   hsotg-phyif = GUSBCFG_PHYIF16;
 
   /*
 -  * Attempt to find a generic PHY, then look for an old style
 -  * USB PHY, finally fall back to pdata
 +  * If platform probe couldn't find a generic PHY or an old style
 +  * USB PHY, fall back to pdata
*/
 - phy = devm_phy_get(dev, usb2-phy);
 - if (IS_ERR(phy)) {
 - uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
 - if (IS_ERR(uphy)) {
 - /* Fallback for pdata */
 - plat = dev_get_platdata(dev);
 - if (!plat) {
 - dev_err(dev,
 - no platform data or transceiver defined\n);
 - return -EPROBE_DEFER;
 - }
 - hsotg-plat = plat;
 - } else
 - hsotg-uphy = uphy;
 - } else {
 - hsotg-phy = phy;
 + if (IS_ERR_OR_NULL(hsotg-phy)  IS_ERR_OR_NULL(hsotg-uphy)) {
 + plat = dev_get_platdata(dev);
 + if (!plat) {
 + dev_err(dev,
 + no platform data or transceiver defined\n);
 + return -EPROBE_DEFER;
 + }
 + hsotg-plat = plat;
 + } else if (hsotg-phy) {

You have changed the behavior here. Previously, the driver would work
even if there were no phys or pdata defined. Now it will return
-EPROBE_DEFER instead. Are you sure that won't break any existing
platforms?

   /*
* If using the generic PHY framework, check if the PHY bus
* width is 8-bit and set the phyif appropriately.
*/
 - if (phy_get_bus_width(phy) == 8)
 + if (phy_get_bus_width(hsotg-phy) == 8)
   hsotg-phyif = GUSBCFG_PHYIF8;
   }
 
 diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
 index 6a795aa..739d14f 100644
 --- a/drivers/usb/dwc2/platform.c
 +++ b/drivers/usb/dwc2/platform.c
 @@ -155,6 +155,8 @@ static int dwc2_driver_probe(struct platform_device *dev)
   struct dwc2_core_params defparams;
   struct dwc2_hsotg *hsotg;
   struct resource *res;
 + struct phy *phy;
 + struct usb_phy *uphy;
   int retval;
   int irq;
 
 @@ -212,6 +214,24 @@ static int dwc2_driver_probe(struct platform_device *dev)
 
   hsotg-dr_mode = of_usb_get_dr_mode(dev-dev.of_node);
 
 + /*
 +  * Attempt to find a generic PHY, then look for an old style
 +  * USB PHY
 +  */
 + phy = devm_phy_get(dev-dev, usb2-phy);
 + if (IS_ERR(phy)) {
 + hsotg-phy = NULL;
 + uphy = devm_usb_get_phy(dev-dev, USB_PHY_TYPE_USB2);
 + if (IS_ERR(uphy))
 + hsotg-uphy = NULL;
 + else
 + hsotg-uphy = uphy;
 + } else {
 + hsotg-phy = phy;
 + phy_power_on(hsotg-phy);
 + phy_init(hsotg-phy);
 + }
 +
   spin_lock_init(hsotg-lock);
   mutex_init(hsotg-init_mutex);
   retval = dwc2_gadget_init(hsotg, irq);
 @@ -233,6 +253,14 @@ static int __maybe_unused dwc2_suspend(struct device 
 *dev)
 
   if (dwc2_is_device_mode(dwc2))
   ret = s3c_hsotg_suspend(dwc2);
 + else {

Kernel style says that both branches of the if() should have braces
here.

 + if (dwc2-lx_state == DWC2_L0)
 + return 0;
 + if (dwc2-phy) {
 + phy_exit(dwc2-phy);
 + phy_power_off(dwc2-phy);
 + }

Minor nit: no need to test dwc2-phy for NULL here, the phy functions
handle a NULL pointer just fine.

 + }
   return ret;
  }
 
 @@ -243,6 +271,12 @@ static int __maybe_unused dwc2_resume(struct device *dev)
 
   if (dwc2_is_device_mode(dwc2))
   ret = s3c_hsotg_resume(dwc2);
 + else {

Braces.

 + if (dwc2-phy) {
 + phy_power_on(dwc2-phy);
 + 

RE: [PATCH] usb: gadget: zero: fix INT endpoint assignment

[Paul Zimmerman]

 From: Sebastian Andrzej Siewior [mailto:bige...@linutronix.de]
 Sent: Tuesday, November 25, 2014 9:22 AM
 
 The max packet size within the FS descriptor has to be highest possible
 value and _not_ the one that is (or will be) used on FS.
 The way the code works (since day 1) is that usb_ep_autoconfig() is
 invoked _only_ on the FS endpoint and then the endpoint address is
 copies over to HS/SS endpoints. If the any of the critical options are
 different between FS and HS then we have to pass the HS value and
 correct later.
 
 What now happens is that we look for an INT-OUT endpoint of 64bytes. The
 code will return an endpoint matching this category. Further the
 sourcesink will take this endpoint and increase the MPS to 1024. On
 net2280 for instance the code tries to be clever to return an endpoint
 which can only do 64 MPS. The same happens on musb where we mostlike get
 an endpoint which can only do 512. The result is then on the host side:
 
 |~# testusb -a -t 9 -c 2
 |unknown speed   /dev/bus/usb/002/0450
 |usbtest 2-1:3.0: TEST 9:  ch9 (subset) control tests, 2 times
 |usbtest 2-1:3.0: can't set_interface = 2, -32
 |usbtest 2-1:3.0: ch9 subset failed, iterations left 1
 |/dev/bus/usb/002/045 test 9 -- 32 (Broken pipe)
 
 because the on the gadget side we can't enable the endpoint because
 desc-size  ep-max_size.
 
 Fixes: ef11982dd7a6 (usb: gadget: zero: Add support for interrupt EP)
 Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
 ---
  drivers/usb/gadget/function/f_sourcesink.c | 24 
  1 file changed, 12 insertions(+), 12 deletions(-)
 
 diff --git a/drivers/usb/gadget/function/f_sourcesink.c 
 b/drivers/usb/gadget/function/f_sourcesink.c
 index 80be25b32cd7..7d8f0216e1a6 100644
 --- a/drivers/usb/gadget/function/f_sourcesink.c
 +++ b/drivers/usb/gadget/function/f_sourcesink.c
 @@ -161,7 +161,7 @@ static struct usb_endpoint_descriptor fs_int_source_desc 
 = {
 
   .bEndpointAddress = USB_DIR_IN,
   .bmAttributes = USB_ENDPOINT_XFER_INT,
 - .wMaxPacketSize =   cpu_to_le16(64),
 + .wMaxPacketSize =   cpu_to_le16(1024),

This seems strange. You are setting the max packet size in the FS Intr
endpoint descriptor to a value that is illegal for FS. Won't that cause
usb_ep_autoconfig() to fail if the UDC only has a FS Intr endpoint?

Maybe you should set wMaxPacketSize to 0 instead? The ep_matches()
function in epautoconf.c has this code:
/*
 * If the protocol driver hasn't yet decided on wMaxPacketSize
 * and wants to know the maximum possible, provide the info.
 */
if (desc-wMaxPacketSize == 0)
desc-wMaxPacketSize = cpu_to_le16(ep-maxpacket_limit);

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5] usb: dwc2/gadget: rework disconnect event handling

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Monday, November 17, 2014 1:00 AM
 
 This patch adds a call to s3c_hsotg_disconnect() from 'end session'
 interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
 about unplugged usb cable. DISCONNINT interrupt cannot be used for this
 purpose, because it is asserted only in host mode.
 
 To avoid reporting disconnect event more than once, a disconnect call has
 been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
 interrupt. This way driver ensures that disconnect event is reported
 either when usb cable is unplugged or every time the host starts a new
 session. To handle devices which has been synthesized without
 SRP support, connected state is set in ENUMDONE interrupt.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/core.h   |  1 +
  drivers/usb/dwc2/gadget.c | 16 ++--
  2 files changed, 15 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
 index 55c90c53f2d6..e54c3c50cd48 100644
 --- a/drivers/usb/dwc2/core.h
 +++ b/drivers/usb/dwc2/core.h
 @@ -210,6 +210,7 @@ struct s3c_hsotg {
   u8  ctrl_buff[8];
 
   struct usb_gadget   gadget;
 + unsigned intconnected:1;
   unsigned intsetup;
   unsigned long   last_rst;
   struct s3c_hsotg_ep *eps;
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index fcd2bb55ccca..89b1bea50ee3 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -1029,7 +1029,6 @@ static int s3c_hsotg_process_req_feature(struct 
 s3c_hsotg *hsotg,
  }
 
  static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
 -static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg);
 
  /**
   * s3c_hsotg_stall_ep0 - stall ep0
 @@ -1107,7 +1106,6 @@ static void s3c_hsotg_process_control(struct s3c_hsotg 
 *hsotg,
   if ((ctrl-bRequestType  USB_TYPE_MASK) == USB_TYPE_STANDARD) {
   switch (ctrl-bRequest) {
   case USB_REQ_SET_ADDRESS:
 - s3c_hsotg_disconnect(hsotg);
   dcfg = readl(hsotg-regs + DCFG);
   dcfg = ~DCFG_DEVADDR_MASK;
   dcfg |= (le16_to_cpu(ctrl-wValue) 
 @@ -2031,6 +2029,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg 
 *hsotg)
  {
   unsigned ep;
 
 + if (!hsotg-connected)
 + return;
 +
 + hsotg-connected = 0;
   for (ep = 0; ep  hsotg-num_of_eps; ep++)
   kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN, true);
 
 @@ -2290,17 +2292,27 @@ irq_retry:
   dev_info(hsotg-dev, OTGInt: %08x\n, otgint);
 
   writel(otgint, hsotg-regs + GOTGINT);
 +
 + if (otgint  GOTGINT_SES_END_DET) {
 + s3c_hsotg_disconnect(hsotg);
 + hsotg-gadget.speed = USB_SPEED_UNKNOWN;
 + }
   }
 
   if (gintsts  GINTSTS_SESSREQINT) {
   dev_dbg(hsotg-dev, %s: SessReqInt\n, __func__);
   writel(GINTSTS_SESSREQINT, hsotg-regs + GINTSTS);
 + /*
 +  * Report disconnect if there is any previous session 
 established
 +  */
 + s3c_hsotg_disconnect(hsotg);
   }
 
   if (gintsts  GINTSTS_ENUMDONE) {
   writel(GINTSTS_ENUMDONE, hsotg-regs + GINTSTS);
 
   s3c_hsotg_irq_enumdone(hsotg);
 + hsotg-connected = 1;
   }
 
   if (gintsts  GINTSTS_CONIDSTSCHNG) {

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Friday, October 31, 2014 3:13 AM
 
 This patch adds mutex, which protects initialization and
 deinitialization procedures against suspend/resume methods.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/core.h   |  1 +
  drivers/usb/dwc2/gadget.c | 20 
  2 files changed, 21 insertions(+)
 
 diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
 index 9f77b4d1c5ff..58732a9a0019 100644
 --- a/drivers/usb/dwc2/core.h
 +++ b/drivers/usb/dwc2/core.h
 @@ -187,6 +187,7 @@ struct s3c_hsotg {
   struct s3c_hsotg_plat*plat;
 
   spinlock_t  lock;
 + struct mutexinit_mutex;
 
   void __iomem*regs;
   int irq;
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index d8dda39c9e16..a2e4272a904e 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -21,6 +21,7 @@
  #include linux/platform_device.h
  #include linux/dma-mapping.h
  #include linux/debugfs.h
 +#include linux/mutex.h
  #include linux/seq_file.h
  #include linux/delay.h
  #include linux/io.h
 @@ -2908,6 +2909,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
 *gadget,
   return -EINVAL;
   }
 
 + mutex_lock(hsotg-init_mutex);
   WARN_ON(hsotg-driver);
 
   driver-driver.bus = NULL;
 @@ -2933,9 +2935,12 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
 *gadget,
 
   dev_info(hsotg-dev, bound driver %s\n, driver-driver.name);
 
 + mutex_unlock(hsotg-init_mutex);
 +
   return 0;
 
  err:
 + mutex_unlock(hsotg-init_mutex);
   hsotg-driver = NULL;
   return ret;
  }
 @@ -2957,6 +2962,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
   if (!hsotg)
   return -ENODEV;
 
 + mutex_lock(hsotg-init_mutex);
 +
   /* all endpoints should be shutdown */
   for (ep = 1; ep  hsotg-num_of_eps; ep++)
   s3c_hsotg_ep_disable(hsotg-eps[ep].ep);
 @@ -2974,6 +2981,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 
   clk_disable(hsotg-clk);
 
 + mutex_unlock(hsotg-init_mutex);
 +
   return 0;
  }
 
 @@ -3002,6 +3011,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, 
 int is_on)
 
   dev_dbg(hsotg-dev, %s: is_on: %d\n, __func__, is_on);
 
 + mutex_lock(hsotg-init_mutex);
   spin_lock_irqsave(hsotg-lock, flags);
   if (is_on) {
   clk_enable(hsotg-clk);
 @@ -3013,6 +3023,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, 
 int is_on)
 
   hsotg-gadget.speed = USB_SPEED_UNKNOWN;
   spin_unlock_irqrestore(hsotg-lock, flags);
 + mutex_unlock(hsotg-init_mutex);
 
   return 0;
  }
 @@ -3507,6 +3518,7 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
   }
 
   spin_lock_init(hsotg-lock);
 + mutex_init(hsotg-init_mutex);
 
   hsotg-irq = ret;
 
 @@ -3652,6 +3664,8 @@ static int s3c_hsotg_suspend(struct platform_device 
 *pdev, pm_message_t state)
   unsigned long flags;
   int ret = 0;
 
 + mutex_lock(hsotg-init_mutex);
 +
   if (hsotg-driver)
   dev_info(hsotg-dev, suspending usb gadget %s\n,
hsotg-driver-driver.name);
 @@ -3674,6 +3688,8 @@ static int s3c_hsotg_suspend(struct platform_device 
 *pdev, pm_message_t state)
   clk_disable(hsotg-clk);
   }
 
 + mutex_unlock(hsotg-init_mutex);
 +
   return ret;
  }
 
 @@ -3683,7 +3699,9 @@ static int s3c_hsotg_resume(struct platform_device 
 *pdev)
   unsigned long flags;
   int ret = 0;
 
 + mutex_lock(hsotg-init_mutex);
   if (hsotg-driver) {
 +
   dev_info(hsotg-dev, resuming usb gadget %s\n,
hsotg-driver-driver.name);
 
 @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device 
 *pdev)
   s3c_hsotg_core_connect(hsotg);
   spin_unlock_irqrestore(hsotg-lock, flags);
 
 + mutex_unlock(hsotg-init_mutex);
 +
   return ret;
  }
 

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] usb: gadget: USB3 support to the legacy printer driver

[Paul Zimmerman]

 From: linux-usb-ow...@vger.kernel.org 
 [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Felipe Balbi
 Sent: Tuesday, November 18, 2014 12:47 PM
 
 On Tue, Nov 18, 2014 at 03:41:43PM -0500, Jorge Ramirez-Ortiz wrote:
 
  notice that the original PLX driver was still far from the theoretical 5Gbps
  target (I was expecting to measure at least 3Gbps and could only get 1Gbps).
  So 1Gbps should be the target to meet on the kernel.org net2280 - do you 
  agree?
 
 this depends on a whole bunch of things. Mainline is a lot different
 from PLX's kernel tree, I'm sure.
 
 It also depends on how many PCIe lanes you're using. Just because USB3
 guarantees 5Gbps bandwidth, if you use a 1x PCIe connector, you'll never
 get that ;-)

Being pedantic, USB3 runs at 4Gbps, not 5Gbps. The signal transitions
on the bus are at 5GT/s (5 giga-transitions per second), but due to the
8b/10b encoding, that equates to 4Gbps data rate.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4] usb: dwc2/gadget: rework disconnect event handling

[Paul Zimmerman]

 -Original Message-
 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Friday, November 14, 2014 4:20 AM
 
 This patch adds a call to s3c_hsotg_disconnect() from 'end session'
 interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
 about unplugged usb cable. DISCONNINT interrupt cannot be used for this
 purpose, because it is asserted only in host mode.
 
 To avoid reporting disconnect event more than once, a disconnect call has
 been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
 interrupt. This way driver ensures that disconnect event is reported
 either when usb cable is unplugged or every time the host starts a new
 session.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/core.h   |  1 +
  drivers/usb/dwc2/gadget.c | 13 +++--
  2 files changed, 12 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
 index 55c90c53f2d6..78b9090ebf71 100644
 --- a/drivers/usb/dwc2/core.h
 +++ b/drivers/usb/dwc2/core.h
 @@ -210,6 +210,7 @@ struct s3c_hsotg {
   u8  ctrl_buff[8];
 
   struct usb_gadget   gadget;
 + unsigned intsession:1;
   unsigned intsetup;
   unsigned long   last_rst;
   struct s3c_hsotg_ep *eps;
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index fcd2bb55ccca..c7f68dc1cf6b 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -1029,7 +1029,6 @@ static int s3c_hsotg_process_req_feature(struct 
 s3c_hsotg *hsotg,
  }
 
  static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
 -static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg);
 
  /**
   * s3c_hsotg_stall_ep0 - stall ep0
 @@ -1107,7 +1106,6 @@ static void s3c_hsotg_process_control(struct s3c_hsotg 
 *hsotg,
   if ((ctrl-bRequestType  USB_TYPE_MASK) == USB_TYPE_STANDARD) {
   switch (ctrl-bRequest) {
   case USB_REQ_SET_ADDRESS:
 - s3c_hsotg_disconnect(hsotg);
   dcfg = readl(hsotg-regs + DCFG);
   dcfg = ~DCFG_DEVADDR_MASK;
   dcfg |= (le16_to_cpu(ctrl-wValue) 
 @@ -2031,6 +2029,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg 
 *hsotg)
  {
   unsigned ep;
 
 + if (!hsotg-session)
 + return;
 +
 + hsotg-session = 0;
   for (ep = 0; ep  hsotg-num_of_eps; ep++)
   kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN, true);
 
 @@ -2290,11 +2292,18 @@ irq_retry:
   dev_info(hsotg-dev, OTGInt: %08x\n, otgint);
 
   writel(otgint, hsotg-regs + GOTGINT);
 +
 + if (otgint  GOTGINT_SES_END_DET) {
 + s3c_hsotg_disconnect(hsotg);

I think you should clear hsotg-session here, shouldn't you?
Otherwise I think s3c_hsotg_disconnect() will be called twice, once
here and once when the next GINTSTS_SESSREQINT comes.

-- 
Paul

 + hsotg-gadget.speed = USB_SPEED_UNKNOWN;
 + }
   }
 
   if (gintsts  GINTSTS_SESSREQINT) {
   dev_dbg(hsotg-dev, %s: SessReqInt\n, __func__);
   writel(GINTSTS_SESSREQINT, hsotg-regs + GINTSTS);
 + s3c_hsotg_disconnect(hsotg);
 + hsotg-session = 1;
   }
 
   if (gintsts  GINTSTS_ENUMDONE) {
 --
 1.9.2

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Friday, November 14, 2014 1:19 AM
 
 On 2014-11-13 21:55, Paul Zimmerman wrote:
  From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
  Sent: Thursday, November 13, 2014 7:18 AM
 
  On 2014-10-31 19:46, Paul Zimmerman wrote:
  From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
  Sent: Friday, October 31, 2014 3:13 AM
 
  This patch adds mutex, which protects initialization and
  deinitialization procedures against suspend/resume methods.
 
  Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
  ---
 drivers/usb/dwc2/core.h   |  1 +
 drivers/usb/dwc2/gadget.c | 20 
 2 files changed, 21 insertions(+)
 
  diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
  index 9f77b4d1c5ff..58732a9a0019 100644
  --- a/drivers/usb/dwc2/core.h
  +++ b/drivers/usb/dwc2/core.h
  @@ -187,6 +187,7 @@ struct s3c_hsotg {
   struct s3c_hsotg_plat*plat;
 
   spinlock_t  lock;
  +struct mutexinit_mutex;
 
   void __iomem*regs;
   int irq;
  diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
  index d8dda39c9e16..a2e4272a904e 100644
  --- a/drivers/usb/dwc2/gadget.c
  +++ b/drivers/usb/dwc2/gadget.c
  @@ -21,6 +21,7 @@
 #include linux/platform_device.h
 #include linux/dma-mapping.h
 #include linux/debugfs.h
  +#include linux/mutex.h
 #include linux/seq_file.h
 #include linux/delay.h
 #include linux/io.h
  @@ -2908,6 +2909,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
  *gadget,
   return -EINVAL;
   }
 
  +mutex_lock(hsotg-init_mutex);
   WARN_ON(hsotg-driver);
 
   driver-driver.bus = NULL;
  @@ -2933,9 +2935,12 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
  *gadget,
 
   dev_info(hsotg-dev, bound driver %s\n, driver-driver.name);
 
  +mutex_unlock(hsotg-init_mutex);
  +
   return 0;
 
 err:
  +mutex_unlock(hsotg-init_mutex);
   hsotg-driver = NULL;
   return ret;
 }
  @@ -2957,6 +2962,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget 
  *gadget,
   if (!hsotg)
   return -ENODEV;
 
  +mutex_lock(hsotg-init_mutex);
  +
   /* all endpoints should be shutdown */
   for (ep = 1; ep  hsotg-num_of_eps; ep++)
   s3c_hsotg_ep_disable(hsotg-eps[ep].ep);
  @@ -2974,6 +2981,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget 
  *gadget,
 
   clk_disable(hsotg-clk);
 
  +mutex_unlock(hsotg-init_mutex);
  +
   return 0;
 }
 
  @@ -3002,6 +3011,7 @@ static int s3c_hsotg_pullup(struct usb_gadget 
  *gadget, int is_on)
 
   dev_dbg(hsotg-dev, %s: is_on: %d\n, __func__, is_on);
 
  +mutex_lock(hsotg-init_mutex);
   spin_lock_irqsave(hsotg-lock, flags);
   if (is_on) {
   clk_enable(hsotg-clk);
  @@ -3013,6 +3023,7 @@ static int s3c_hsotg_pullup(struct usb_gadget 
  *gadget, int is_on)
 
   hsotg-gadget.speed = USB_SPEED_UNKNOWN;
   spin_unlock_irqrestore(hsotg-lock, flags);
  +mutex_unlock(hsotg-init_mutex);
 
   return 0;
 }
  @@ -3507,6 +3518,7 @@ static int s3c_hsotg_probe(struct platform_device 
  *pdev)
   }
 
   spin_lock_init(hsotg-lock);
  +mutex_init(hsotg-init_mutex);
 
   hsotg-irq = ret;
 
  @@ -3652,6 +3664,8 @@ static int s3c_hsotg_suspend(struct 
  platform_device *pdev, pm_message_t
  state)
   unsigned long flags;
   int ret = 0;
 
  +mutex_lock(hsotg-init_mutex);
  +
   if (hsotg-driver)
   dev_info(hsotg-dev, suspending usb gadget %s\n,
hsotg-driver-driver.name);
  @@ -3674,6 +3688,8 @@ static int s3c_hsotg_suspend(struct 
  platform_device *pdev, pm_message_t
  state)
   clk_disable(hsotg-clk);
   }
 
  +mutex_unlock(hsotg-init_mutex);
  +
   return ret;
 }
 
  @@ -3683,7 +3699,9 @@ static int s3c_hsotg_resume(struct platform_device 
  *pdev)
   unsigned long flags;
   int ret = 0;
 
  +mutex_lock(hsotg-init_mutex);
   if (hsotg-driver) {
  +
   dev_info(hsotg-dev, resuming usb gadget %s\n,
hsotg-driver-driver.name);
 
  @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device 
  *pdev)
   s3c_hsotg_core_connect(hsotg);
   spin_unlock_irqrestore(hsotg-lock, flags);
 
  +mutex_unlock(hsotg-init_mutex);
  +
   return ret;
 }
 
  Hmm. I can't find any other UDC driver that uses a mutex in its
  suspend/resume functions. Can you explain why this is needed only
  for dwc2?
  I've posted this version because I thought you were not convinced that
  the patch
  usb: dwc2/gadget: rework suspend/resume code

RE: [PATCH v3] usb: dwc2: add bus suspend/resume for dwc2

[Paul Zimmerman]

 From: jwer...@google.com [mailto:jwer...@google.com] On Behalf Of Julius 
 Werner
 Sent: Thursday, November 13, 2014 8:50 PM
 
  I will figure out how to make dwc2 detect the device connect after auto
  suspend,
  or disable the auto suspend feature for the dwc2 hcd.
 
 I think auto-suspend of the root hub device (which is what calls
 bus_suspend, but is not the host controller device itself) is expected
 to always happen and not really meant to be disabled. I'm surprised
 that the controller would fail to come back up, though. Does removing
 the PCGCTL part make it work? That's the only thing I can think of
 (but then again the function should immediately return if the port is
 not in L0, so if there is nothing plugged in the suspend shouldn't
 really do anything).

Hi guys,

I don't have any experience with suspend/resume on the dwc2 controller
I'm afraid. One suggestion would be to look at our Synopsys vendor
driver to see how it is handled there.

That driver is part of the Raspberry Pi upstream kernel, which you
can find at https://github.com/raspberrypi/linux. The driver is
under drivers/usb/host/dwc_otg.

In particular, in dwc_otg_cil_intr.c there is a function named
dwc_otg_handle_wakeup_detected_intr() which might be of interest.
Unfortunately, that driver code is rather convoluted, so it may be
difficult to figure out exactly what the driver is doing.

-- 
Paul

N�r��yb�X��ǧv�^�)޺{.n�+{��^n�r���z���h����G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�

RE: [PATCHv7 3/8] usb: dwc2: convert to use dev_pm_ops API

[Paul Zimmerman]

 From: dingu...@opensource.altera.com [mailto:dingu...@opensource.altera.com]
 Sent: Tuesday, November 11, 2014 9:14 AM
 
 From: Dinh Nguyen dingu...@opensource.altera.com
 
 Update suspend/resume to use dev_pm_ops API.
 
 Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com
 ---
  drivers/usb/dwc2/platform.c | 15 +--
  1 file changed, 9 insertions(+), 6 deletions(-)
 
 diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
 index eeba8a4..b94867b 100644
 --- a/drivers/usb/dwc2/platform.c
 +++ b/drivers/usb/dwc2/platform.c
 @@ -219,9 +219,9 @@ static int dwc2_driver_probe(struct platform_device *dev)
   return retval;
  }
 
 -static int dwc2_suspend(struct platform_device *dev, pm_message_t state)
 +static int dwc2_suspend(struct device *dev)
  {
 - struct dwc2_hsotg *dwc2 = platform_get_drvdata(dev);
 + struct dwc2_hsotg *dwc2 = dev_get_drvdata(dev);
   int ret = 0;
 
   if (dwc2_is_device_mode(dwc2))
 @@ -229,9 +229,9 @@ static int dwc2_suspend(struct platform_device *dev, 
 pm_message_t state)
   return ret;
  }
 
 -static int dwc2_resume(struct platform_device *dev)
 +static int dwc2_resume(struct device *dev)
  {
 - struct dwc2_hsotg *dwc2 = platform_get_drvdata(dev);
 + struct dwc2_hsotg *dwc2 = dev_get_drvdata(dev);
   int ret = 0;
 
   if (dwc2_is_device_mode(dwc2))
 @@ -239,15 +239,18 @@ static int dwc2_resume(struct platform_device *dev)
   return ret;
  }
 
 +static const struct dev_pm_ops dwc2_dev_pm_ops = {
 + SET_SYSTEM_SLEEP_PM_OPS(dwc2_suspend, dwc2_resume)
 +};
 +
  static struct platform_driver dwc2_platform_driver = {
   .driver = {
   .name = dwc2_driver_name,
   .of_match_table = dwc2_of_match_table,
 + .pm = dwc2_dev_pm_ops,
   },
   .probe = dwc2_driver_probe,
   .remove = dwc2_driver_remove,
 - .suspend = dwc2_suspend,
 - .resume = dwc2_resume,
  };
 
  module_platform_driver(dwc2_platform_driver);

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCHv7 5/8] usb: dwc2: Update common interrupt handler to call gadget interrupt handler

[Paul Zimmerman]

);
   goto err_clk;
   }
 
 diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c
 index 6d33ecf..a4e724b 100644
 --- a/drivers/usb/dwc2/pci.c
 +++ b/drivers/usb/dwc2/pci.c
 @@ -141,6 +141,12 @@ static int dwc2_driver_probe(struct pci_dev *dev,
 
   pci_set_master(dev);
 
 + retval = devm_request_irq(hsotg-dev, dev-irq,
 +   dwc2_handle_common_intr, IRQF_SHARED,
 +   dev_name(hsotg-dev), hsotg);
 + if (retval)
 + return retval;
 +
   spin_lock_init(hsotg-lock);
   retval = dwc2_hcd_init(hsotg, dev-irq, dwc2_module_params);
   if (retval) {
 diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
 index b94867b..3552602 100644
 --- a/drivers/usb/dwc2/platform.c
 +++ b/drivers/usb/dwc2/platform.c
 @@ -196,6 +196,14 @@ static int dwc2_driver_probe(struct platform_device *dev)
   return irq;
   }
 
 + dev_dbg(hsotg-dev, registering common handler for irq%d\n,
 + irq);
 + retval = devm_request_irq(hsotg-dev, irq,
 +   dwc2_handle_common_intr, IRQF_SHARED,
 +   dev_name(hsotg-dev), hsotg);
 + if (retval)
 + return retval;
 +
   res = platform_get_resource(dev, IORESOURCE_MEM, 0);
   hsotg-regs = devm_ioremap_resource(dev-dev, res);
   if (IS_ERR(hsotg-regs))

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCHv7 7/8] usb: dwc2: move usb_disabled() call to host driver only

[Paul Zimmerman]

 From: dingu...@opensource.altera.com [mailto:dingu...@opensource.altera.com]
 Sent: Tuesday, November 11, 2014 9:14 AM
 
 From: Dinh Nguyen dingu...@opensource.altera.com
 
 Since platform.c will get built for both Host and Gadget, if we leave the
 usb_disabled() call in platform.c, it results in the following build error
 when (!USB  USB_GADGET) condition is met.
 
 ERROR: usb_disabled [drivers/usb/dwc2/dwc2_platform.ko] undefined!
 
 Since usb_disabled() is mostly used to disable USB host functionality, move
 the call the host portion for the DWC2 driver.
 
 Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com
 ---
  drivers/usb/dwc2/hcd.c  | 3 +++
  drivers/usb/dwc2/platform.c | 3 ---
  2 files changed, 3 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
 index fa60f4a..755e16b 100644
 --- a/drivers/usb/dwc2/hcd.c
 +++ b/drivers/usb/dwc2/hcd.c
 @@ -2780,6 +2780,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
   int i, num_channels;
   int retval;
 
 + if (usb_disabled())
 + return -ENODEV;
 +
   dev_dbg(hsotg-dev, DWC OTG HCD INIT\n);
 
   /* Detect config values from hardware */
 diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
 index 3552602..57eb8a3 100644
 --- a/drivers/usb/dwc2/platform.c
 +++ b/drivers/usb/dwc2/platform.c
 @@ -157,9 +157,6 @@ static int dwc2_driver_probe(struct platform_device *dev)
   int retval;
   int irq;
 
 - if (usb_disabled())
 - return -ENODEV;
 -
   match = of_match_device(dwc2_of_match_table, dev-dev);
   if (match  match-data) {
   params = match-data;

This patch also fixes a minor buglet, in that we were missing the
usb_disabled() check in pci.c.

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCHv7 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

[Paul Zimmerman]

 From: dingu...@opensource.altera.com [mailto:dingu...@opensource.altera.com]
 Sent: Tuesday, November 11, 2014 9:14 AM
 
 From: Dinh Nguyen dingu...@opensource.altera.com
 
 Since the dwc2 hcd driver is currently not looking for a clock node during
 init, we should not completely fail if there isn't a clock provided.
 By assigning clk = NULL, this allows the driver, when configured for dual-role
 mode, to be able to continue loading the host portion of the driver when
 a clock node is not specified.
 
 Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com
 ---
 v7: Reworked to use clk=NULL and remove the need to is IS_ERR(clk)
 v6: none
 v5: reworked to not access gadget functions from the hcd.
 ---
  drivers/usb/dwc2/gadget.c | 1 +
  1 file changed, 1 insertion(+)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 37c7916..367689b 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -3431,6 +3431,7 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
 
   hsotg-clk = devm_clk_get(dev, otg);
   if (IS_ERR(hsotg-clk)) {
 + hsotg-clk = NULL;
   dev_err(dev, cannot get otg clock\n);
   return PTR_ERR(hsotg-clk);
   }

Whoops, you just broke the return value, since you NULL out
hsotg-clk and then call PTR_ERR() on it.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4] usb: dwc2/gadget: rework disconnect event handling

[Paul Zimmerman]

 From: linux-usb-ow...@vger.kernel.org 
 [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Felipe Balbi
 Sent: Friday, November 14, 2014 11:05 AM
 
 On Fri, Nov 14, 2014 at 07:01:37PM +, Paul Zimmerman wrote:
   -Original Message-
   From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
   Sent: Friday, November 14, 2014 4:20 AM
  
   This patch adds a call to s3c_hsotg_disconnect() from 'end session'
   interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
   about unplugged usb cable. DISCONNINT interrupt cannot be used for this
   purpose, because it is asserted only in host mode.
  
   To avoid reporting disconnect event more than once, a disconnect call has
   been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
   interrupt. This way driver ensures that disconnect event is reported
   either when usb cable is unplugged or every time the host starts a new
   session.
  
   Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
   ---
drivers/usb/dwc2/core.h   |  1 +
drivers/usb/dwc2/gadget.c | 13 +++--
2 files changed, 12 insertions(+), 2 deletions(-)
  
   diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
   index 55c90c53f2d6..78b9090ebf71 100644
   --- a/drivers/usb/dwc2/core.h
   +++ b/drivers/usb/dwc2/core.h
   @@ -210,6 +210,7 @@ struct s3c_hsotg {
 u8  ctrl_buff[8];
  
 struct usb_gadget   gadget;
   + unsigned intsession:1;
 unsigned intsetup;
 unsigned long   last_rst;
 struct s3c_hsotg_ep *eps;
   diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
   index fcd2bb55ccca..c7f68dc1cf6b 100644
   --- a/drivers/usb/dwc2/gadget.c
   +++ b/drivers/usb/dwc2/gadget.c
   @@ -1029,7 +1029,6 @@ static int s3c_hsotg_process_req_feature(struct 
   s3c_hsotg *hsotg,
}
  
static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
   -static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg);
  
/**
 * s3c_hsotg_stall_ep0 - stall ep0
   @@ -1107,7 +1106,6 @@ static void s3c_hsotg_process_control(struct 
   s3c_hsotg *hsotg,
 if ((ctrl-bRequestType  USB_TYPE_MASK) == USB_TYPE_STANDARD) {
 switch (ctrl-bRequest) {
 case USB_REQ_SET_ADDRESS:
   - s3c_hsotg_disconnect(hsotg);
 dcfg = readl(hsotg-regs + DCFG);
 dcfg = ~DCFG_DEVADDR_MASK;
 dcfg |= (le16_to_cpu(ctrl-wValue) 
   @@ -2031,6 +2029,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg 
   *hsotg)
{
 unsigned ep;
  
   + if (!hsotg-session)
   + return;
   +
   + hsotg-session = 0;
 for (ep = 0; ep  hsotg-num_of_eps; ep++)
 kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN, true);
  
   @@ -2290,11 +2292,18 @@ irq_retry:
 dev_info(hsotg-dev, OTGInt: %08x\n, otgint);
  
 writel(otgint, hsotg-regs + GOTGINT);
   +
   + if (otgint  GOTGINT_SES_END_DET) {
   + s3c_hsotg_disconnect(hsotg);
 
  I think you should clear hsotg-session here, shouldn't you?
  Otherwise I think s3c_hsotg_disconnect() will be called twice, once
  here and once when the next GINTSTS_SESSREQINT comes.
 
 the best way to avoid that would be fiddle with hsotg-session inside
 s3c_hsotg_disconnect() only.

Whoops, I just noticed that hsotg-session *is* cleared inside of
s3c_hsotg_disconnect(). So I think the patch is good as-is.

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4] usb: dwc2/gadget: rework disconnect event handling

[Paul Zimmerman]

 From: Paul Zimmerman
 Sent: Friday, November 14, 2014 1:21 PM
 
  From: linux-usb-ow...@vger.kernel.org 
  [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Felipe Balbi
  Sent: Friday, November 14, 2014 11:05 AM
 
  On Fri, Nov 14, 2014 at 07:01:37PM +, Paul Zimmerman wrote:
-Original Message-
From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
Sent: Friday, November 14, 2014 4:20 AM
   
This patch adds a call to s3c_hsotg_disconnect() from 'end session'
interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
about unplugged usb cable. DISCONNINT interrupt cannot be used for this
purpose, because it is asserted only in host mode.
   
To avoid reporting disconnect event more than once, a disconnect call 
has
been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
interrupt. This way driver ensures that disconnect event is reported
either when usb cable is unplugged or every time the host starts a new
session.
   
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
---
 drivers/usb/dwc2/core.h   |  1 +
 drivers/usb/dwc2/gadget.c | 13 +++--
 2 files changed, 12 insertions(+), 2 deletions(-)
   
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 55c90c53f2d6..78b9090ebf71 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -210,6 +210,7 @@ struct s3c_hsotg {
u8  ctrl_buff[8];
   
struct usb_gadget   gadget;
+   unsigned intsession:1;
unsigned intsetup;
unsigned long   last_rst;
struct s3c_hsotg_ep *eps;
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index fcd2bb55ccca..c7f68dc1cf6b 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1029,7 +1029,6 @@ static int s3c_hsotg_process_req_feature(struct 
s3c_hsotg *hsotg,
 }
   
 static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
-static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg);
   
 /**
  * s3c_hsotg_stall_ep0 - stall ep0
@@ -1107,7 +1106,6 @@ static void s3c_hsotg_process_control(struct 
s3c_hsotg *hsotg,
if ((ctrl-bRequestType  USB_TYPE_MASK) == USB_TYPE_STANDARD) {
switch (ctrl-bRequest) {
case USB_REQ_SET_ADDRESS:
-   s3c_hsotg_disconnect(hsotg);
dcfg = readl(hsotg-regs + DCFG);
dcfg = ~DCFG_DEVADDR_MASK;
dcfg |= (le16_to_cpu(ctrl-wValue) 
@@ -2031,6 +2029,10 @@ static void s3c_hsotg_disconnect(struct 
s3c_hsotg *hsotg)
 {
unsigned ep;
   
+   if (!hsotg-session)
+   return;
+
+   hsotg-session = 0;
for (ep = 0; ep  hsotg-num_of_eps; ep++)
kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN, 
true);
   
@@ -2290,11 +2292,18 @@ irq_retry:
dev_info(hsotg-dev, OTGInt: %08x\n, otgint);
   
writel(otgint, hsotg-regs + GOTGINT);
+
+   if (otgint  GOTGINT_SES_END_DET) {
+   s3c_hsotg_disconnect(hsotg);
  
   I think you should clear hsotg-session here, shouldn't you?
   Otherwise I think s3c_hsotg_disconnect() will be called twice, once
   here and once when the next GINTSTS_SESSREQINT comes.
 
  the best way to avoid that would be fiddle with hsotg-session inside
  s3c_hsotg_disconnect() only.
 
 Whoops, I just noticed that hsotg-session *is* cleared inside of
 s3c_hsotg_disconnect(). So I think the patch is good as-is.
 
 Acked-by: Paul Zimmerman pa...@synopsys.com

I'm having second thoughts about this. Currently, the
GINTSTS_SESSREQINT interrupt in the gadget does nothing except print
a debug message. So I'm not sure that all versions of the controller
actually assert this interrupt when a connection is made. If they
don't, then this patch would break the disconnect handling, since
hsotg-session would never get set.

In particular, I'm thinking that a core which is synthesized without
SRP support may not implement the GINTSTS_SESSREQINT interrupt. The
databook seems to imply that:

SessReqInt: In Device mode, this interrupt is asserted when the
utmisrp_bvalid signal goes high.

And in the section which describes the utmisrp_bvalid signal, there is
a note that says:

This interface is present only when parameter OTG_MODE specifies an
SRP-capable configuration.

So Marek, can you try moving the line which sets hsotg-session = 1
into s3c_hsotg_irq_enumdone() instead? Then we will be sure it gets set
to one after a connect. Probably it should be renamed from 'session'
to 'connect' in that case.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message

RE: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Thursday, November 13, 2014 5:40 AM
 
 On 2014-10-31 19:15, Paul Zimmerman wrote:
  From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
  Sent: Friday, October 31, 2014 1:04 AM
  To: linux-usb@vger.kernel.org; linux-samsung-...@vger.kernel.org
  Cc: Marek Szyprowski; Kyungmin Park; Robert Baldyga; Paul Zimmerman; 
  Krzysztof Kozlowski; Felipe
 Balbi
  Subject: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end 
  session' irq
 
  This patch adds a call to s3c_hsotg_disconnect() from 'end session'
  interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
  about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might
  look a bit more suitable for this event, but it is asserted only in
  host mode, so in device mode we need to use something else.
 
  Additional check has been added in s3c_hsotg_disconnect() function
  to ensure that the event is reported only once after successful device
  enumeration.
 
  Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
  ---
drivers/usb/dwc2/core.h   |  1 +
drivers/usb/dwc2/gadget.c | 10 ++
2 files changed, 11 insertions(+)
 
  diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
  index 55c90c53f2d6..b42df32e7737 100644
  --- a/drivers/usb/dwc2/core.h
  +++ b/drivers/usb/dwc2/core.h
  @@ -212,6 +212,7 @@ struct s3c_hsotg {
 struct usb_gadget   gadget;
 unsigned intsetup;
 unsigned long   last_rst;
  +  unsigned intaddress;
 struct s3c_hsotg_ep *eps;
};
 
  diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
  index fcd2bb55ccca..6304efba11aa 100644
  --- a/drivers/usb/dwc2/gadget.c
  +++ b/drivers/usb/dwc2/gadget.c
  @@ -1114,6 +1114,7 @@ static void s3c_hsotg_process_control(struct 
  s3c_hsotg *hsotg,
  DCFG_DEVADDR_SHIFT)  DCFG_DEVADDR_MASK;
 writel(dcfg, hsotg-regs + DCFG);
 
  +  hsotg-address = ctrl-wValue;
 dev_info(hsotg-dev, new address %d\n, ctrl-wValue);
 
 ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0);
  @@ -2031,6 +2032,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg 
  *hsotg)
{
 unsigned ep;
 
  +  if (!hsotg-address)
  +  return;
  +
  +  hsotg-address = 0;
 for (ep = 0; ep  hsotg-num_of_eps; ep++)
 kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN, true);
 
  @@ -2290,6 +2295,11 @@ irq_retry:
 dev_info(hsotg-dev, OTGInt: %08x\n, otgint);
 
 writel(otgint, hsotg-regs + GOTGINT);
  +
  +  if (otgint  GOTGINT_SES_END_DET) {
  +  s3c_hsotg_disconnect(hsotg);
  +  hsotg-gadget.speed = USB_SPEED_UNKNOWN;
  +  }
 }
 
 if (gintsts  GINTSTS_SESSREQINT) {
  I don't think this is right. The host can send control requests to
  the device before it sends a SetAddress to change from the default
  address of 0. So if a GOTGINT_SES_END_DET happens before the
  SetAddress, it will be ignored.
 
  Or am I missing something?
 
 Well, right. However before finishing enumeration (setting the address)
 host usually
 only retrieves some usb descriptors what doesn't change the state of the
 gadget.
 Right now we always reported 'disconnected' event before setting the new
 address,
 what is a bit overkill (in some cases gadget driver got this even more
 than once).
 The above code handles all cases correctly and reports disconnect event
 only once.

Well, if the disconnect happens before the SetAddress, the disconnect
won't be reported at all. Unless I'm reading the code wrong.

-- 
Paul

RE: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Thursday, November 13, 2014 7:18 AM
 
 On 2014-10-31 19:46, Paul Zimmerman wrote:
  From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
  Sent: Friday, October 31, 2014 3:13 AM
 
  This patch adds mutex, which protects initialization and
  deinitialization procedures against suspend/resume methods.
 
  Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
  ---
drivers/usb/dwc2/core.h   |  1 +
drivers/usb/dwc2/gadget.c | 20 
2 files changed, 21 insertions(+)
 
  diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
  index 9f77b4d1c5ff..58732a9a0019 100644
  --- a/drivers/usb/dwc2/core.h
  +++ b/drivers/usb/dwc2/core.h
  @@ -187,6 +187,7 @@ struct s3c_hsotg {
 struct s3c_hsotg_plat*plat;
 
 spinlock_t  lock;
  +  struct mutexinit_mutex;
 
 void __iomem*regs;
 int irq;
  diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
  index d8dda39c9e16..a2e4272a904e 100644
  --- a/drivers/usb/dwc2/gadget.c
  +++ b/drivers/usb/dwc2/gadget.c
  @@ -21,6 +21,7 @@
#include linux/platform_device.h
#include linux/dma-mapping.h
#include linux/debugfs.h
  +#include linux/mutex.h
#include linux/seq_file.h
#include linux/delay.h
#include linux/io.h
  @@ -2908,6 +2909,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
  *gadget,
 return -EINVAL;
 }
 
  +  mutex_lock(hsotg-init_mutex);
 WARN_ON(hsotg-driver);
 
 driver-driver.bus = NULL;
  @@ -2933,9 +2935,12 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
  *gadget,
 
 dev_info(hsotg-dev, bound driver %s\n, driver-driver.name);
 
  +  mutex_unlock(hsotg-init_mutex);
  +
 return 0;
 
err:
  +  mutex_unlock(hsotg-init_mutex);
 hsotg-driver = NULL;
 return ret;
}
  @@ -2957,6 +2962,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget 
  *gadget,
 if (!hsotg)
 return -ENODEV;
 
  +  mutex_lock(hsotg-init_mutex);
  +
 /* all endpoints should be shutdown */
 for (ep = 1; ep  hsotg-num_of_eps; ep++)
 s3c_hsotg_ep_disable(hsotg-eps[ep].ep);
  @@ -2974,6 +2981,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget 
  *gadget,
 
 clk_disable(hsotg-clk);
 
  +  mutex_unlock(hsotg-init_mutex);
  +
 return 0;
}
 
  @@ -3002,6 +3011,7 @@ static int s3c_hsotg_pullup(struct usb_gadget 
  *gadget, int is_on)
 
 dev_dbg(hsotg-dev, %s: is_on: %d\n, __func__, is_on);
 
  +  mutex_lock(hsotg-init_mutex);
 spin_lock_irqsave(hsotg-lock, flags);
 if (is_on) {
 clk_enable(hsotg-clk);
  @@ -3013,6 +3023,7 @@ static int s3c_hsotg_pullup(struct usb_gadget 
  *gadget, int is_on)
 
 hsotg-gadget.speed = USB_SPEED_UNKNOWN;
 spin_unlock_irqrestore(hsotg-lock, flags);
  +  mutex_unlock(hsotg-init_mutex);
 
 return 0;
}
  @@ -3507,6 +3518,7 @@ static int s3c_hsotg_probe(struct platform_device 
  *pdev)
 }
 
 spin_lock_init(hsotg-lock);
  +  mutex_init(hsotg-init_mutex);
 
 hsotg-irq = ret;
 
  @@ -3652,6 +3664,8 @@ static int s3c_hsotg_suspend(struct platform_device 
  *pdev, pm_message_t
 state)
 unsigned long flags;
 int ret = 0;
 
  +  mutex_lock(hsotg-init_mutex);
  +
 if (hsotg-driver)
 dev_info(hsotg-dev, suspending usb gadget %s\n,
  hsotg-driver-driver.name);
  @@ -3674,6 +3688,8 @@ static int s3c_hsotg_suspend(struct platform_device 
  *pdev, pm_message_t
 state)
 clk_disable(hsotg-clk);
 }
 
  +  mutex_unlock(hsotg-init_mutex);
  +
 return ret;
}
 
  @@ -3683,7 +3699,9 @@ static int s3c_hsotg_resume(struct platform_device 
  *pdev)
 unsigned long flags;
 int ret = 0;
 
  +  mutex_lock(hsotg-init_mutex);
 if (hsotg-driver) {
  +
 dev_info(hsotg-dev, resuming usb gadget %s\n,
  hsotg-driver-driver.name);
 
  @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device 
  *pdev)
 s3c_hsotg_core_connect(hsotg);
 spin_unlock_irqrestore(hsotg-lock, flags);
 
  +  mutex_unlock(hsotg-init_mutex);
  +
 return ret;
}
 
  Hmm. I can't find any other UDC driver that uses a mutex in its
  suspend/resume functions. Can you explain why this is needed only
  for dwc2?
 
 I've posted this version because I thought you were not convinced that
 the patch
 usb: dwc2/gadget: rework suspend/resume code to correctly restore
 gadget state
 can add code for initialization and deinitialization in suspend/resume
 paths.

My problem with that patch was that you were checking the -enabled
flag outside of the spinlock. To address that, you only need to move
the check inside of the spinlock. I don't see why a mutex is needed.

-- 
Paul

RE: Two questions about dwc2 driver

[Paul Zimmerman]

 From: linux-usb-ow...@vger.kernel.org 
 [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Haibo Zhang
 Sent: Tuesday, November 11, 2014 6:37 AM
 
 We know that dwc2 is the USB2.0 driver for DesignWare IP. But it only
 has host’s code without device’s code. So I have two questions:
 1.   Will I develop or port the part of device’s code if I want to
 use dwc2 to support both host and device’s functions?

In the latest versions of the mainline kernel, the dwc2 driver supports
both host and device modes. Maybe you are using an older version of the
kernel where this wasn't true?

 2.   Another question, I know that many chip companies use dwc2.
 How do they solve the question above?

See my previous answer. Also, many companies are using the Synopsys
vendor driver, which has been available for many years, before the dwc2
driver existed. If you are a Synopsys customer, you should have access
to that driver. There are also third parties who will sell you drivers
for the Synopsys controllers.

-- 
Paul

 If anyone has a similar experience or know well about this dwc2
 driver, please reply me.
 Any help will be greatly appreciated.
 
 Thanks,
 
 --
 
 
 *Zhang Haibo*

N�r��yb�X��ǧv�^�)޺{.n�+{��^n�r���z���h����G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�

RE: [PATCH v2] usb: dwc2: add bus suspend/resume for dwc2

[Paul Zimmerman]

 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Thursday, November 06, 2014 9:40 AM
 
 On Thu, Nov 06, 2014 at 06:21:42PM +0100, Romain Perier wrote:
 
  2014-11-06 2:30 GMT+01:00 Kever Yang kever.y...@rock-chips.com:
  
   +static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
   +{
   +   struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
   +   u32 hprt0;
   +
   +   if (!((hsotg-op_state == OTG_STATE_B_HOST) ||
   +   (hsotg-op_state == OTG_STATE_A_HOST)))
   +   return 0;
   +
   +   if (hsotg-lx_state != DWC2_L0)
   +   return 0;
   +
   +   hprt0 = dwc2_read_hprt0(hsotg);
   +   if (hprt0  HPRT0_CONNSTS)
   +   dwc2_port_suspend(hsotg, 1);
   +
   +   return 0;
   +}
   +
   +static int _dwc2_hcd_resume(struct usb_hcd *hcd)
   +{
   +   struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
   +   u32 hprt0;
   +
   +   if (!((hsotg-op_state == OTG_STATE_B_HOST) ||
   +   (hsotg-op_state == OTG_STATE_A_HOST)))
   +   return 0;
   +
   +   if (hsotg-lx_state != DWC2_L2)
   +   return 0;
   +
   +   hprt0 = dwc2_read_hprt0(hsotg);
   +   if ((hprt0  HPRT0_CONNSTS)  (hprt0  HPRT0_SUSP))
   +   dwc2_port_resume(hsotg);
   +
   +   return 0;
   +}
 
  Could you also define these functions under #ifdef CONFIG_PM ?
 
 please don't. I'm actually considering ripping all ifdefs from all these
 drivers and also stop using SIMPLE_DEV_PM_OPS or any of its friends.
 
 There's really nobody today would would build a kernel with CONFIG_PM.

I'm sure Felipe meant *without* CONFIG_PM.

Kever, in that case you should remove the #ifdef CONFIG_PM around the
.bus_suspend and .bus_resume assignments also, otherwise there will be
compiler warnings when built without CONFIG_PM. After that, you can
add my acked-by.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] usb: dwc2: add bus suspend/resume for dwc2

[Paul Zimmerman]

 From: Kever Yang [mailto:kever.y...@gmail.com] On Behalf Of Kever Yang
 Sent: Friday, October 31, 2014 7:03 AM
 
 This patch adds suspend/resume for dwc2 hcd controller.
 
 Signed-off-by: Kever Yang kever.y...@rock-chips.com
 ---
 
  drivers/usb/dwc2/hcd.c | 74 
 ++
  1 file changed, 63 insertions(+), 11 deletions(-)
 
 diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
 index fa49c72..df68449 100644
 --- a/drivers/usb/dwc2/hcd.c
 +++ b/drivers/usb/dwc2/hcd.c
 @@ -1473,6 +1473,29 @@ static void dwc2_port_suspend(struct dwc2_hsotg 
 *hsotg, u16 windex)
   }
  }
 
 +static void dwc2_port_resume(struct dwc2_hsotg *hsotg)
 +{
 + u32 hprt0;
 +
 + /* After clear the Stop PHY clock bit, we should wait for a moment
 +  * for PLL work stable with clock output.
 +  */
 + writel(0, hsotg-regs + PCGCTL);
 + usleep_range(2000, 4000);
 +
 + hprt0 = dwc2_read_hprt0(hsotg);
 + hprt0 |= HPRT0_RES;
 + writel(hprt0, hsotg-regs + HPRT0);
 + hprt0 = ~HPRT0_SUSP;
 + /* according to USB2.0 Spec 7.1.7.7, the host most send the resume
 +  * signal for at least 20ms
 +  */
 + usleep_range(2, 25000);
 +
 + hprt0 = ~HPRT0_RES;
 + writel(hprt0, hsotg-regs + HPRT0);
 +}
 +
  /* Handles hub class-specific requests */
  static int dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq,
   u16 wvalue, u16 windex, char *buf, u16 wlength)
 @@ -1518,17 +1541,7 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg 
 *hsotg, u16 typereq,
   case USB_PORT_FEAT_SUSPEND:
   dev_dbg(hsotg-dev,
   ClearPortFeature USB_PORT_FEAT_SUSPEND\n);
 - writel(0, hsotg-regs + PCGCTL);
 - usleep_range(2, 4);
 -
 - hprt0 = dwc2_read_hprt0(hsotg);
 - hprt0 |= HPRT0_RES;
 - writel(hprt0, hsotg-regs + HPRT0);
 - hprt0 = ~HPRT0_SUSP;
 - usleep_range(10, 15);
 -
 - hprt0 = ~HPRT0_RES;
 - writel(hprt0, hsotg-regs + HPRT0);
 + dwc2_port_resume(hsotg);
   break;
 
   case USB_PORT_FEAT_POWER:
 @@ -2301,6 +2314,42 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
   usleep_range(1000, 3000);
  }
 
 +static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 +{
 + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
 + u32 hprt0;
 +
 + if (hsotg-op_state != OTG_STATE_B_HOST)
 + return 0;
 +
 + if (hsotg-lx_state != DWC2_L0)
 + return 0;
 +
 + hprt0 = dwc2_read_hprt0(hsotg);
 + if (hprt0  HPRT0_CONNSTS)
 + dwc2_port_suspend(hsotg, 1);
 +
 + return 0;
 +}
 +
 +static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 +{
 + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
 + u32 hprt0;
 +
 + if (hsotg-op_state != OTG_STATE_B_HOST)
 + return 0;
 +
 + if (hsotg-lx_state != DWC2_L2)
 + return 0;
 +
 + hprt0 = dwc2_read_hprt0(hsotg);
 + if ((hprt0 | HPRT0_CONNSTS)  (hprt0 | HPRT0_SUSP))

This isn't right, the condition will always be true.

Per your previous email, you are not able to test this because your
platform does not support suspend/resume yet, is that right? I don't
want to apply any untested patches to the driver.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Friday, October 31, 2014 1:04 AM
 To: linux-usb@vger.kernel.org; linux-samsung-...@vger.kernel.org
 Cc: Marek Szyprowski; Kyungmin Park; Robert Baldyga; Paul Zimmerman; 
 Krzysztof Kozlowski; Felipe Balbi
 Subject: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end 
 session' irq
 
 This patch adds a call to s3c_hsotg_disconnect() from 'end session'
 interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
 about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might
 look a bit more suitable for this event, but it is asserted only in
 host mode, so in device mode we need to use something else.
 
 Additional check has been added in s3c_hsotg_disconnect() function
 to ensure that the event is reported only once after successful device
 enumeration.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/core.h   |  1 +
  drivers/usb/dwc2/gadget.c | 10 ++
  2 files changed, 11 insertions(+)
 
 diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
 index 55c90c53f2d6..b42df32e7737 100644
 --- a/drivers/usb/dwc2/core.h
 +++ b/drivers/usb/dwc2/core.h
 @@ -212,6 +212,7 @@ struct s3c_hsotg {
   struct usb_gadget   gadget;
   unsigned intsetup;
   unsigned long   last_rst;
 + unsigned intaddress;
   struct s3c_hsotg_ep *eps;
  };
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index fcd2bb55ccca..6304efba11aa 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -1114,6 +1114,7 @@ static void s3c_hsotg_process_control(struct s3c_hsotg 
 *hsotg,
DCFG_DEVADDR_SHIFT)  DCFG_DEVADDR_MASK;
   writel(dcfg, hsotg-regs + DCFG);
 
 + hsotg-address = ctrl-wValue;
   dev_info(hsotg-dev, new address %d\n, ctrl-wValue);
 
   ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0);
 @@ -2031,6 +2032,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg 
 *hsotg)
  {
   unsigned ep;
 
 + if (!hsotg-address)
 + return;
 +
 + hsotg-address = 0;
   for (ep = 0; ep  hsotg-num_of_eps; ep++)
   kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN, true);
 
 @@ -2290,6 +2295,11 @@ irq_retry:
   dev_info(hsotg-dev, OTGInt: %08x\n, otgint);
 
   writel(otgint, hsotg-regs + GOTGINT);
 +
 + if (otgint  GOTGINT_SES_END_DET) {
 + s3c_hsotg_disconnect(hsotg);
 + hsotg-gadget.speed = USB_SPEED_UNKNOWN;
 + }
   }
 
   if (gintsts  GINTSTS_SESSREQINT) {

I don't think this is right. The host can send control requests to
the device before it sends a SetAddress to change from the default
address of 0. So if a GOTGINT_SES_END_DET happens before the
SetAddress, it will be ignored.

Or am I missing something?

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Friday, October 31, 2014 3:13 AM
 
 This patch adds mutex, which protects initialization and
 deinitialization procedures against suspend/resume methods.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/core.h   |  1 +
  drivers/usb/dwc2/gadget.c | 20 
  2 files changed, 21 insertions(+)
 
 diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
 index 9f77b4d1c5ff..58732a9a0019 100644
 --- a/drivers/usb/dwc2/core.h
 +++ b/drivers/usb/dwc2/core.h
 @@ -187,6 +187,7 @@ struct s3c_hsotg {
   struct s3c_hsotg_plat*plat;
 
   spinlock_t  lock;
 + struct mutexinit_mutex;
 
   void __iomem*regs;
   int irq;
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index d8dda39c9e16..a2e4272a904e 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -21,6 +21,7 @@
  #include linux/platform_device.h
  #include linux/dma-mapping.h
  #include linux/debugfs.h
 +#include linux/mutex.h
  #include linux/seq_file.h
  #include linux/delay.h
  #include linux/io.h
 @@ -2908,6 +2909,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
 *gadget,
   return -EINVAL;
   }
 
 + mutex_lock(hsotg-init_mutex);
   WARN_ON(hsotg-driver);
 
   driver-driver.bus = NULL;
 @@ -2933,9 +2935,12 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
 *gadget,
 
   dev_info(hsotg-dev, bound driver %s\n, driver-driver.name);
 
 + mutex_unlock(hsotg-init_mutex);
 +
   return 0;
 
  err:
 + mutex_unlock(hsotg-init_mutex);
   hsotg-driver = NULL;
   return ret;
  }
 @@ -2957,6 +2962,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
   if (!hsotg)
   return -ENODEV;
 
 + mutex_lock(hsotg-init_mutex);
 +
   /* all endpoints should be shutdown */
   for (ep = 1; ep  hsotg-num_of_eps; ep++)
   s3c_hsotg_ep_disable(hsotg-eps[ep].ep);
 @@ -2974,6 +2981,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 
   clk_disable(hsotg-clk);
 
 + mutex_unlock(hsotg-init_mutex);
 +
   return 0;
  }
 
 @@ -3002,6 +3011,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, 
 int is_on)
 
   dev_dbg(hsotg-dev, %s: is_on: %d\n, __func__, is_on);
 
 + mutex_lock(hsotg-init_mutex);
   spin_lock_irqsave(hsotg-lock, flags);
   if (is_on) {
   clk_enable(hsotg-clk);
 @@ -3013,6 +3023,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, 
 int is_on)
 
   hsotg-gadget.speed = USB_SPEED_UNKNOWN;
   spin_unlock_irqrestore(hsotg-lock, flags);
 + mutex_unlock(hsotg-init_mutex);
 
   return 0;
  }
 @@ -3507,6 +3518,7 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
   }
 
   spin_lock_init(hsotg-lock);
 + mutex_init(hsotg-init_mutex);
 
   hsotg-irq = ret;
 
 @@ -3652,6 +3664,8 @@ static int s3c_hsotg_suspend(struct platform_device 
 *pdev, pm_message_t state)
   unsigned long flags;
   int ret = 0;
 
 + mutex_lock(hsotg-init_mutex);
 +
   if (hsotg-driver)
   dev_info(hsotg-dev, suspending usb gadget %s\n,
hsotg-driver-driver.name);
 @@ -3674,6 +3688,8 @@ static int s3c_hsotg_suspend(struct platform_device 
 *pdev, pm_message_t state)
   clk_disable(hsotg-clk);
   }
 
 + mutex_unlock(hsotg-init_mutex);
 +
   return ret;
  }
 
 @@ -3683,7 +3699,9 @@ static int s3c_hsotg_resume(struct platform_device 
 *pdev)
   unsigned long flags;
   int ret = 0;
 
 + mutex_lock(hsotg-init_mutex);
   if (hsotg-driver) {
 +
   dev_info(hsotg-dev, resuming usb gadget %s\n,
hsotg-driver-driver.name);
 
 @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device 
 *pdev)
   s3c_hsotg_core_connect(hsotg);
   spin_unlock_irqrestore(hsotg-lock, flags);
 
 + mutex_unlock(hsotg-init_mutex);
 +
   return ret;
  }
 

Hmm. I can't find any other UDC driver that uses a mutex in its
suspend/resume functions. Can you explain why this is needed only
for dwc2?

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCHv6 4/8] usb: dwc2: Update common interrupt handler to call gadget interrupt handler

[Paul Zimmerman]

 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Thursday, October 30, 2014 7:01 AM
 
 On Tue, Oct 28, 2014 at 06:25:45PM -0500, dingu...@opensource.altera.com 
 wrote:
  From: Dinh Nguyen dingu...@opensource.altera.com
 
  Make dwc2_handle_common_intr call the gadget interrupt function when 
  operating
  in peripheral mode. Remove the spinlock functions in s3c_hsotg_irq as
  dwc2_handle_common_intr() already has the spinlocks.
 
  Move the registeration of the IRQ to common code for platform and PCI.
 
  Remove duplicate interrupt conditions that was in gadget, as those are 
  handled
  by dwc2 common interrupt handler.
 
  Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com
  Acked-by: Paul Zimmerman pa...@synopsys.com
  ---
  v5: remove individual devm_request_irq from gadget and hcd, and place a
  single devm_request_irq in platform and pci.
  v2: Keep interrupt handler for host and peripheral modes separate
  ---
   drivers/usb/dwc2/core.c  | 10 
   drivers/usb/dwc2/core.h  |  3 +++
   drivers/usb/dwc2/core_intr.c |  3 +++
   drivers/usb/dwc2/gadget.c| 57 
  ++--
   drivers/usb/dwc2/pci.c   |  6 +
   drivers/usb/dwc2/platform.c  |  9 +++
   6 files changed, 23 insertions(+), 65 deletions(-)
 
  diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
  index d926945..7605850b 100644
  --- a/drivers/usb/dwc2/core.c
  +++ b/drivers/usb/dwc2/core.c
  @@ -458,16 +458,6 @@ int dwc2_core_init(struct dwc2_hsotg *hsotg, bool 
  select_phy, int irq)
  /* Clear the SRP success bit for FS-I2c */
  hsotg-srp_success = 0;
 
  -   if (irq = 0) {
  -   dev_dbg(hsotg-dev, registering common handler for irq%d\n,
  -   irq);
  -   retval = devm_request_irq(hsotg-dev, irq,
  - dwc2_handle_common_intr, IRQF_SHARED,
  - dev_name(hsotg-dev), hsotg);
  -   if (retval)
  -   return retval;
  -   }
  -
  /* Enable common interrupts */
  dwc2_enable_common_interrupts(hsotg);
 
  diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
  index 80d29c7..ec70862 100644
  --- a/drivers/usb/dwc2/core.h
  +++ b/drivers/usb/dwc2/core.h
  @@ -967,6 +967,7 @@ extern int s3c_hsotg_suspend(struct dwc2_hsotg *dwc2);
   extern int s3c_hsotg_resume(struct dwc2_hsotg *dwc2);
   extern int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq);
   extern void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2);
  +irqreturn_t s3c_hsotg_irq(int irq, void *pw);
   #else
   static inline int s3c_hsotg_remove(struct dwc2_hsotg *dwc2)
   { return 0; }
  @@ -977,6 +978,8 @@ static inline int s3c_hsotg_resume(struct dwc2_hsotg 
  *dwc2)
   static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
   { return 0; }
   static inline void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2) {}
  +static inline irqreturn_t s3c_hsotg_irq(int irq, void *pw)
  +{ return IRQ_HANDLED; }
   #endif
 
   #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || 
  IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
  diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
  index b176c2f..b0c14e0 100644
  --- a/drivers/usb/dwc2/core_intr.c
  +++ b/drivers/usb/dwc2/core_intr.c
  @@ -474,6 +474,9 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev)
 
  spin_lock(hsotg-lock);
 
  +   if (dwc2_is_device_mode(hsotg))
  +   retval = s3c_hsotg_irq(irq, dev);
  +
  gintsts = dwc2_read_common_intr(hsotg);
  if (gintsts  ~GINTSTS_PRTINT)
  retval = IRQ_HANDLED;
  diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
  index 19d1b03..202f8cc 100644
  --- a/drivers/usb/dwc2/gadget.c
  +++ b/drivers/usb/dwc2/gadget.c
  @@ -2257,14 +2257,13 @@ void s3c_hsotg_core_init(struct dwc2_hsotg *hsotg)
* @irq: The IRQ number triggered
* @pw: The pw value when registered the handler.
*/
  -static irqreturn_t s3c_hsotg_irq(int irq, void *pw)
  +irqreturn_t s3c_hsotg_irq(int irq, void *pw)
 
 why ? It would've been a lot easier to just make the IRQ line shared.

Actually, I would prefer to keep the common interrupt handler. Reason
being, the HSOTG core can spew a *lot* of interrupts when in host mode,
and the driver already struggles to keep up with them on some platforms.
We see this problem especially on the Raspberry Pi.

So I think adding the overhead of an additional interrupt handling
path is a bad idea. Unless the peripheral-mode interrupt handler can
somehow be disabled when the controller is in host mode.

Maybe we could delay registering the gadget interrupt handler until
the controller switches to peripheral mode, and unregister it when it
switches back to host mode? But that seems pretty odd to me.

Felipe, what do you think?

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http

RE: [PATCH v3 01/19] usb: dwc3: enable hibernation if to be supported

[Paul Zimmerman]

 From: Huang Rui [mailto:ray.hu...@amd.com]
 Sent: Tuesday, October 28, 2014 4:54 AM
 
 It enables hibernation if the function is set in coreConsultant.
 
 Suggested-by: Felipe Balbi ba...@ti.com
 Signed-off-by: Huang Rui ray.hu...@amd.com
 ---
  drivers/usb/dwc3/core.c | 1 +
  1 file changed, 1 insertion(+)
 
 diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
 index fa396fc..bf77509 100644
 --- a/drivers/usb/dwc3/core.c
 +++ b/drivers/usb/dwc3/core.c
 @@ -449,6 +449,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
   case DWC3_GHWPARAMS1_EN_PWROPT_HIB:
   /* enable hibernation here */
   dwc-nr_scratch = DWC3_GHWPARAMS4_HIBER_SCRATCHBUFS(hwparams4);
 + reg |= DWC3_GCTL_GBLHIBERNATIONEN;
   break;
   default:
   dev_dbg(dwc-dev, No power optimization available\n);

What effect does this have when the controller is in device mode? I
expect it will start generating DWC3_DEVICE_EVENT_HIBER_REQ interrupt
events when this register bit is set. So the dev_WARN_ONCE in
dwc3_gadget_interrupt() will start firing, I think.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 01/19] usb: dwc3: enable hibernation if to be supported

[Paul Zimmerman]

 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Tuesday, October 28, 2014 11:51 AM
 
 On Tue, Oct 28, 2014 at 06:47:08PM +, Paul Zimmerman wrote:
   From: Huang Rui [mailto:ray.hu...@amd.com]
   Sent: Tuesday, October 28, 2014 4:54 AM
  
   It enables hibernation if the function is set in coreConsultant.
  
   Suggested-by: Felipe Balbi ba...@ti.com
   Signed-off-by: Huang Rui ray.hu...@amd.com
   ---
drivers/usb/dwc3/core.c | 1 +
1 file changed, 1 insertion(+)
  
   diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
   index fa396fc..bf77509 100644
   --- a/drivers/usb/dwc3/core.c
   +++ b/drivers/usb/dwc3/core.c
   @@ -449,6 +449,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
 case DWC3_GHWPARAMS1_EN_PWROPT_HIB:
 /* enable hibernation here */
 dwc-nr_scratch = DWC3_GHWPARAMS4_HIBER_SCRATCHBUFS(hwparams4);
   + reg |= DWC3_GCTL_GBLHIBERNATIONEN;
 break;
 default:
 dev_dbg(dwc-dev, No power optimization available\n);
 
  What effect does this have when the controller is in device mode? I
  expect it will start generating DWC3_DEVICE_EVENT_HIBER_REQ interrupt
  events when this register bit is set. So the dev_WARN_ONCE in
  dwc3_gadget_interrupt() will start firing, I think.
 
 Ok, so this *has* to wait for proper hibernation support ?

Ah, never mind. Since the hibernation event is not enabled in the
DEVTEN register, the controller shouldn't generate that event after
all. So I think it should be OK. Sorry for the noise.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 01/19] usb: dwc3: enable hibernation if to be supported

[Paul Zimmerman]

 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Tuesday, October 28, 2014 12:01 PM
 
 On Tue, Oct 28, 2014 at 06:55:50PM +, Paul Zimmerman wrote:
   From: Felipe Balbi [mailto:ba...@ti.com]
   Sent: Tuesday, October 28, 2014 11:51 AM
  
   On Tue, Oct 28, 2014 at 06:47:08PM +, Paul Zimmerman wrote:
 From: Huang Rui [mailto:ray.hu...@amd.com]
 Sent: Tuesday, October 28, 2014 4:54 AM

 It enables hibernation if the function is set in coreConsultant.

 Suggested-by: Felipe Balbi ba...@ti.com
 Signed-off-by: Huang Rui ray.hu...@amd.com
 ---
  drivers/usb/dwc3/core.c | 1 +
  1 file changed, 1 insertion(+)

 diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
 index fa396fc..bf77509 100644
 --- a/drivers/usb/dwc3/core.c
 +++ b/drivers/usb/dwc3/core.c
 @@ -449,6 +449,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
   case DWC3_GHWPARAMS1_EN_PWROPT_HIB:
   /* enable hibernation here */
   dwc-nr_scratch = 
 DWC3_GHWPARAMS4_HIBER_SCRATCHBUFS(hwparams4);
 + reg |= DWC3_GCTL_GBLHIBERNATIONEN;
   break;
   default:
   dev_dbg(dwc-dev, No power optimization available\n);
   
What effect does this have when the controller is in device mode? I
expect it will start generating DWC3_DEVICE_EVENT_HIBER_REQ interrupt
events when this register bit is set. So the dev_WARN_ONCE in
dwc3_gadget_interrupt() will start firing, I think.
  
   Ok, so this *has* to wait for proper hibernation support ?
 
  Ah, never mind. Since the hibernation event is not enabled in the
  DEVTEN register, the controller shouldn't generate that event after
  all. So I think it should be OK. Sorry for the noise.
 
 do you think it's still nice to have a comment here ?

Maybe something along the lines of enabling this bit so that host-mode
hibernation will work, device-mode hibernation is not implemented yet?

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCHv6 8/8] usb: dwc2: move usb_disabled() call to host driver only

[Paul Zimmerman]

 From: dingu...@opensource.altera.com [mailto:dingu...@opensource.altera.com]
 Sent: Tuesday, October 28, 2014 4:26 PM
 
 Now that platform.c will get built for both Host and Gadget, if we leave the
 usb_disabled() call in platform.c, it results in the following build error
 when (!USB  USB_GADGET) condition is met.
 
 ERROR: usb_disabled [drivers/usb/dwc2/dwc2_platform.ko] undefined!
 
 Since usb_disabled() is mostly used to disable USB host functionality, move
 the call the host portion for the DWC2 driver.
 
 Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com
 ---
  drivers/usb/dwc2/hcd.c  | 3 +++
  drivers/usb/dwc2/platform.c | 3 ---
  2 files changed, 3 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
 index fa49c72..b741997 100644
 --- a/drivers/usb/dwc2/hcd.c
 +++ b/drivers/usb/dwc2/hcd.c
 @@ -2780,6 +2780,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
   int i, num_channels;
   int retval;
 
 + if (usb_disabled())
 + return -ENODEV;
 +
   dev_dbg(hsotg-dev, DWC OTG HCD INIT\n);
 
   /* Detect config values from hardware */
 diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
 index 77c8417..123cf54 100644
 --- a/drivers/usb/dwc2/platform.c
 +++ b/drivers/usb/dwc2/platform.c
 @@ -157,9 +157,6 @@ static int dwc2_driver_probe(struct platform_device *dev)
   int retval;
   int irq;
 
 - if (usb_disabled())
 - return -ENODEV;
 -
   match = of_match_device(dwc2_of_match_table, dev-dev);
   if (match  match-data) {
   params = match-data;

I'm confused. You are saying the build is broken until patch 8/8 is
applied? As always, that is not acceptable. You need to fix the
breakage at the point where it was introduced, not leave it broken
until the last patch in the series.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

[Paul Zimmerman]

 From: dingu...@opensource.altera.com [mailto:dingu...@opensource.altera.com]
 Sent: Tuesday, October 28, 2014 4:26 PM
 
 Since the dwc2 hcd driver is currently not looking for a clock node during
 init, we should not completely fail if there isn't a clock provided.
 For dual-role mode, we will only fail init for a non-clock node error. We
 then update the HCD to only call gadget funtions if there is a proper clock
 node.
 
 Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com
 ---
 v5: reworked to not access gadget functions from the hcd.
 ---
  drivers/usb/dwc2/core.h  |  3 +--
  drivers/usb/dwc2/core_intr.c |  9 ++---
  drivers/usb/dwc2/hcd.c   |  3 ++-
  drivers/usb/dwc2/platform.c  | 19 +++
  4 files changed, 24 insertions(+), 10 deletions(-)
 
 diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
 index ec70862..48120c8 100644
 --- a/drivers/usb/dwc2/core.h
 +++ b/drivers/usb/dwc2/core.h
 @@ -660,6 +660,7 @@ struct dwc2_hsotg {
  #endif
  #endif /* CONFIG_USB_DWC2_HOST || CONFIG_USB_DWC2_DUAL_ROLE */
 
 + struct clk *clk;
  #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || 
 IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
   /* Gadget structures */
   struct usb_gadget_driver *driver;
 @@ -667,8 +668,6 @@ struct dwc2_hsotg {
   struct usb_phy *uphy;
   struct s3c_hsotg_plat *plat;
 
 - struct clk *clk;
 -
   struct regulator_bulk_data supplies[ARRAY_SIZE(s3c_hsotg_supply_names)];
 
   u32 phyif;
 diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
 index 1240875..1608037 100644
 --- a/drivers/usb/dwc2/core_intr.c
 +++ b/drivers/usb/dwc2/core_intr.c
 @@ -339,7 +339,8 @@ static void dwc2_handle_wakeup_detected_intr(struct 
 dwc2_hsotg *hsotg)
   }
   /* Change to L0 state */
   hsotg-lx_state = DWC2_L0;
 - call_gadget(hsotg, resume);
 + if (!IS_ERR(hsotg-clk))
 + call_gadget(hsotg, resume);
   } else {
   if (hsotg-lx_state != DWC2_L1) {
   u32 pcgcctl = readl(hsotg-regs + PCGCTL);
 @@ -400,7 +401,8 @@ static void dwc2_handle_usb_suspend_intr(struct 
 dwc2_hsotg *hsotg)
   DSTS.Suspend Status=%d HWCFG4.Power Optimize=%d\n,
   !!(dsts  DSTS_SUSPSTS),
   hsotg-hw_params.power_optimized);
 - call_gadget(hsotg, suspend);
 + if (!IS_ERR(hsotg-clk))
 + call_gadget(hsotg, suspend);
   } else {
   if (hsotg-op_state == OTG_STATE_A_PERIPHERAL) {
   dev_dbg(hsotg-dev, a_peripheral-a_host\n);
 @@ -477,7 +479,8 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev)
   spin_lock(hsotg-lock);
 
   if (dwc2_is_device_mode(hsotg))
 - retval = s3c_hsotg_irq(irq, dev);
 + if (!IS_ERR(hsotg-clk))
 + retval = s3c_hsotg_irq(irq, dev);
 
   gintsts = dwc2_read_common_intr(hsotg);
   if (gintsts  ~GINTSTS_PRTINT)
 diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
 index 44c609f..fa49c72 100644
 --- a/drivers/usb/dwc2/hcd.c
 +++ b/drivers/usb/dwc2/hcd.c
 @@ -1371,7 +1371,8 @@ static void dwc2_conn_id_status_change(struct 
 work_struct *work)
   hsotg-op_state = OTG_STATE_B_PERIPHERAL;
   dwc2_core_init(hsotg, false, -1);
   dwc2_enable_global_interrupts(hsotg);
 - s3c_hsotg_core_init(hsotg);
 + if (!IS_ERR(hsotg-clk))
 + s3c_hsotg_core_init(hsotg);
   } else {
   /* A-Device connector (Host Mode) */
   dev_dbg(hsotg-dev, connId A\n);
 diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
 index 72f32f7..77c8417 100644
 --- a/drivers/usb/dwc2/platform.c
 +++ b/drivers/usb/dwc2/platform.c
 @@ -217,8 +217,17 @@ static int dwc2_driver_probe(struct platform_device *dev)
 
   spin_lock_init(hsotg-lock);
   retval = dwc2_gadget_init(hsotg, irq);
 - if (retval)
 - return retval;
 + if (retval) {
 + /*
 +  * We will not fail the driver initialization for dual-role
 +  * if no clock node is supplied. However, all gadget
 +  * functionality will be disabled if a clock node is not
 +  * provided. Host functionality will continue.
 +  * TO-DO: make clock node a requirement for the HCD.
 +  */
 + if (!IS_ERR(hsotg-clk))
 + return retval;
 + }
   retval = dwc2_hcd_init(hsotg, irq, params);
   if (retval)
   return retval;
 @@ -234,7 +243,8 @@ static int dwc2_suspend(struct device *dev)
   int ret = 0;
 
   if (dwc2_is_device_mode(dwc2))
 - ret = s3c_hsotg_suspend(dwc2);
 + if (!IS_ERR(dwc2-clk))
 + ret = s3c_hsotg_suspend(dwc2);
   return ret;
  }
 
 @@ -244,7 +254,8 @@ static int 

RE: [PATCH v2 02/10] usb: dwc2/gadget: fix enumeration issues

[Paul Zimmerman]

 -Original Message-
 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Monday, October 20, 2014 3:46 AM
 
 Excessive debug messages might cause timing issues that prevent correct
 usb enumeration. This patch hides information about USB bus reset to let
 driver enumerate fast enough to avoid making host angry. This fixes
 endless enumeration and usb reset loop observed with some Linux hosts.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 Reviewed-by: Felipe Balbi ba...@ti.com
 ---
  drivers/usb/dwc2/gadget.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 119c8a3effc2..8870e38c1d82 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2333,7 +2333,7 @@ irq_retry:
 
   u32 usb_status = readl(hsotg-regs + GOTGCTL);
 
 - dev_info(hsotg-dev, %s: USBRst\n, __func__);
 + dev_dbg(hsotg-dev, %s: USBRst\n, __func__);
   dev_dbg(hsotg-dev, GNPTXSTS=%08x\n,
   readl(hsotg-regs + GNPTXSTS));
 

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 05/10] usb: dwc2/gadget: move setting last reset time to s3c_hsotg_core_init

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Monday, October 20, 2014 3:46 AM
 
 This patch removes duplicated code and sets last_rst variable in the
 function which does the hardware reset.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index fd52a8b23649..c1dad46bbbdd 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2247,6 +2247,8 @@ static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
   /* must be at-least 3ms to allow bus to see disconnect */
   mdelay(3);
 
 + hsotg-last_rst = jiffies;
 +
   /* remove the soft-disconnect and let's go */
   __bic32(hsotg-regs + DCTL, DCTL_SFTDISCON);
  }
 @@ -2347,7 +2349,6 @@ irq_retry:
 -ECONNRESET, true);
 
   s3c_hsotg_core_init(hsotg);
 - hsotg-last_rst = jiffies;
   }
   }
   }
 @@ -2908,7 +2909,6 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
 *gadget,
   goto err;
   }
 
 - hsotg-last_rst = jiffies;
   dev_info(hsotg-dev, bound driver %s\n, driver-driver.name);
   return 0;
 
 @@ -3667,7 +3667,6 @@ static int s3c_hsotg_resume(struct platform_device 
 *pdev)
   }
 
   spin_lock_irqsave(hsotg-lock, flags);
 - hsotg-last_rst = jiffies;
   s3c_hsotg_phy_enable(hsotg);
   s3c_hsotg_core_init(hsotg);
   spin_unlock_irqrestore(hsotg-lock, flags);

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 06/10] usb: dwc2/gadget: decouple setting soft-disconnect from s3c_hsotg_core_init

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Monday, October 20, 2014 3:46 AM
 
 This patch changes s3c_hsotg_core_init function to leave hardware in
 soft disconnect mode, so the moment of coupling the hardware to the usb
 bus can be later controlled by the separate functions for enabling and
 disabling soft disconnect mode. This patch is a preparation to rework
 pullup() method.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 22 +-
  1 file changed, 17 insertions(+), 5 deletions(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index c1dad46bbbdd..5eb2473031c4 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2124,7 +2124,7 @@ static int s3c_hsotg_corereset(struct s3c_hsotg *hsotg)
   *
   * Issue a soft reset to the core, and await the core finishing it.
   */
 -static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
 +static void s3c_hsotg_core_init_disconnected(struct s3c_hsotg *hsotg)
  {
   s3c_hsotg_corereset(hsotg);
 
 @@ -2241,14 +2241,23 @@ static void s3c_hsotg_core_init(struct s3c_hsotg 
 *hsotg)
   readl(hsotg-regs + DOEPCTL0));
 
   /* clear global NAKs */
 - writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK,
 + writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK | DCTL_SFTDISCON,
  hsotg-regs + DCTL);
 
   /* must be at-least 3ms to allow bus to see disconnect */
   mdelay(3);
 
   hsotg-last_rst = jiffies;
 +}
 +
 +static void s3c_hsotg_core_disconnect(struct s3c_hsotg *hsotg)
 +{
 + /* set the soft-disconnect bit */
 + __orr32(hsotg-regs + DCTL, DCTL_SFTDISCON);

I'm not really happy with adding more uses of these __orr32, __bic32
functions. When I first saw those, I went 'wtf?, because with the
assembly-sounding names, they look like some special ARM instruction
or something.

But I guess cleaning that up can wait for a future patch series, so OK.

 +}
 
 +static void s3c_hsotg_core_connect(struct s3c_hsotg *hsotg)
 +{
   /* remove the soft-disconnect and let's go */
   __bic32(hsotg-regs + DCTL, DCTL_SFTDISCON);
  }
 @@ -2348,7 +2357,8 @@ irq_retry:
   kill_all_requests(hsotg, hsotg-eps[0],
 -ECONNRESET, true);
 
 - s3c_hsotg_core_init(hsotg);
 + s3c_hsotg_core_init_disconnected(hsotg);
 + s3c_hsotg_core_connect(hsotg);
   }
   }
   }
 @@ -2981,7 +2991,8 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, 
 int is_on)
   if (is_on) {
   s3c_hsotg_phy_enable(hsotg);
   clk_enable(hsotg-clk);
 - s3c_hsotg_core_init(hsotg);
 + s3c_hsotg_core_init_disconnected(hsotg);
 + s3c_hsotg_core_connect(hsotg);
   } else {
   clk_disable(hsotg-clk);
   s3c_hsotg_phy_disable(hsotg);
 @@ -3668,7 +3679,8 @@ static int s3c_hsotg_resume(struct platform_device 
 *pdev)
 
   spin_lock_irqsave(hsotg-lock, flags);
   s3c_hsotg_phy_enable(hsotg);
 - s3c_hsotg_core_init(hsotg);
 + s3c_hsotg_core_init_disconnected(hsotg);
 + s3c_hsotg_core_connect(hsotg);
   spin_unlock_irqrestore(hsotg-lock, flags);
 
   return ret;

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 07/10] usb: dwc2/gadget: move phy control calls out of pullup() method

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Monday, October 20, 2014 3:46 AM
 
 This patch moves phy enable/disable calls from pullup() method to
 udc_start/stop functions. This solves the issue related to limited caller
 context for PHY functions, because they cannot be called from non-sleeping
 context. This is also a preparation for using soft-disconnect feature of
 udc controller in pullup() method.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 5eb2473031c4..98adf8d17493 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2919,6 +2919,8 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
 *gadget,
   goto err;
   }
 
 + s3c_hsotg_phy_enable(hsotg);
 +
   dev_info(hsotg-dev, bound driver %s\n, driver-driver.name);
   return 0;
 
 @@ -2955,6 +2957,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 
   spin_unlock_irqrestore(hsotg-lock, flags);
 
 + s3c_hsotg_phy_disable(hsotg);
 +
   regulator_bulk_disable(ARRAY_SIZE(hsotg-supplies), hsotg-supplies);
 
   clk_disable(hsotg-clk);
 @@ -2989,13 +2993,11 @@ static int s3c_hsotg_pullup(struct usb_gadget 
 *gadget, int is_on)
 
   spin_lock_irqsave(hsotg-lock, flags);
   if (is_on) {
 - s3c_hsotg_phy_enable(hsotg);
   clk_enable(hsotg-clk);
   s3c_hsotg_core_init_disconnected(hsotg);
   s3c_hsotg_core_connect(hsotg);
   } else {
   clk_disable(hsotg-clk);
 - s3c_hsotg_phy_disable(hsotg);
   }
 
   hsotg-gadget.speed = USB_SPEED_UNKNOWN;

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 08/10] usb: dwc2/gadget: use soft-disconnect udc feature in pullup() method

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Monday, October 20, 2014 3:46 AM
 
 This patch moves udc initialization from pullup() method to
 s3c_hsotg_udc_start(), so that method ends with hardware fully
 initialized and left in soft-disconnected state. After this change, the
 pullup() method simply clears soft-disconnect start() when called with
 is_on=1. For completeness, a call to s3c_hsotg_core_disconnect() has
 been added when pullup() method is called with is_on=0, what puts the
 udc hardware back to soft-disconnected state.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 98adf8d17493..e8ffc080e6c7 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2883,6 +2883,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
 *gadget,
  struct usb_gadget_driver *driver)
  {
   struct s3c_hsotg *hsotg = to_hsotg(gadget);
 + unsigned long flags;
   int ret;
 
   if (!hsotg) {
 @@ -2921,7 +2922,13 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
 *gadget,
 
   s3c_hsotg_phy_enable(hsotg);
 
 + spin_lock_irqsave(hsotg-lock, flags);
 + s3c_hsotg_init(hsotg);
 + s3c_hsotg_core_init_disconnected(hsotg);
 + spin_unlock_irqrestore(hsotg-lock, flags);
 +
   dev_info(hsotg-dev, bound driver %s\n, driver-driver.name);
 +
   return 0;
 
  err:
 @@ -2994,9 +3001,9 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, 
 int is_on)
   spin_lock_irqsave(hsotg-lock, flags);
   if (is_on) {
   clk_enable(hsotg-clk);
 - s3c_hsotg_core_init_disconnected(hsotg);
   s3c_hsotg_core_connect(hsotg);
   } else {
 + s3c_hsotg_core_disconnect(hsotg);
   clk_disable(hsotg-clk);
   }
 

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 09/10] usb: dwc2/gadget: fix calls to phy control functions in suspend/resume code

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Monday, October 20, 2014 3:46 AM
 
 This patch moves calls to phy enable/disable out of spinlock protected
 blocks in device suspend/resume to fix incorrect caller context. Phy
 related functions must not be called from atomic context. To protect
 device internal state from a race during suspend, a call to
 s3c_hsotg_core_disconnect() is added under a spinlock, what prevents any
 further activity on the usb bus.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index e8ffc080e6c7..0d34cfc71bfb 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -3653,11 +3653,13 @@ static int s3c_hsotg_suspend(struct platform_device 
 *pdev, pm_message_t state)
hsotg-driver-driver.name);
 
   spin_lock_irqsave(hsotg-lock, flags);
 + s3c_hsotg_core_disconnect(hsotg);
   s3c_hsotg_disconnect(hsotg);
 - s3c_hsotg_phy_disable(hsotg);
   hsotg-gadget.speed = USB_SPEED_UNKNOWN;
   spin_unlock_irqrestore(hsotg-lock, flags);
 
 + s3c_hsotg_phy_disable(hsotg);
 +
   if (hsotg-driver) {
   int ep;
   for (ep = 0; ep  hsotg-num_of_eps; ep++)
 @@ -3686,8 +3688,9 @@ static int s3c_hsotg_resume(struct platform_device 
 *pdev)
 hsotg-supplies);
   }
 
 - spin_lock_irqsave(hsotg-lock, flags);
   s3c_hsotg_phy_enable(hsotg);
 +
 + spin_lock_irqsave(hsotg-lock, flags);
   s3c_hsotg_core_init_disconnected(hsotg);
   s3c_hsotg_core_connect(hsotg);
   spin_unlock_irqrestore(hsotg-lock, flags);

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 10/10] usb: dwc2/gadget: rework suspend/resume code to correctly restore gadget state

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Monday, October 20, 2014 3:46 AM
 
 Suspend/resume code assumed that the gadget was always enabled and
 connected to usb bus. This means that the actual state of the gadget
 (soft-enabled/disabled or connected/disconnected) was not correctly
 preserved on suspend/resume cycle. This patch fixes this issue.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/core.h   |  4 +++-
  drivers/usb/dwc2/gadget.c | 43 +++
  2 files changed, 30 insertions(+), 17 deletions(-)
 
 diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
 index bf015ab3b44c..3648b76a18b4 100644
 --- a/drivers/usb/dwc2/core.h
 +++ b/drivers/usb/dwc2/core.h
 @@ -210,7 +210,9 @@ struct s3c_hsotg {
   u8  ctrl_buff[8];
 
   struct usb_gadget   gadget;
 - unsigned intsetup;
 + unsigned intsetup:1;
 + unsigned intconnected:1;
 + unsigned intenabled:1;
   unsigned long   last_rst;
   struct s3c_hsotg_ep *eps;
  };
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 0d34cfc71bfb..c6c6cf982c90 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2925,6 +2925,8 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
 *gadget,
   spin_lock_irqsave(hsotg-lock, flags);
   s3c_hsotg_init(hsotg);
   s3c_hsotg_core_init_disconnected(hsotg);
 + hsotg-enabled = 1;
 + hsotg-connected = 0;
   spin_unlock_irqrestore(hsotg-lock, flags);
 
   dev_info(hsotg-dev, bound driver %s\n, driver-driver.name);
 @@ -2961,6 +2963,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 
   hsotg-driver = NULL;
   hsotg-gadget.speed = USB_SPEED_UNKNOWN;
 + hsotg-enabled = 0;
 + hsotg-connected = 0;
 
   spin_unlock_irqrestore(hsotg-lock, flags);
 
 @@ -2999,11 +3003,14 @@ static int s3c_hsotg_pullup(struct usb_gadget 
 *gadget, int is_on)
   dev_dbg(hsotg-dev, %s: is_on: %d\n, __func__, is_on);
 
   spin_lock_irqsave(hsotg-lock, flags);
 +
   if (is_on) {
   clk_enable(hsotg-clk);
 + hsotg-connected = 1;
   s3c_hsotg_core_connect(hsotg);
   } else {
   s3c_hsotg_core_disconnect(hsotg);
 + hsotg-connected = 0;
   clk_disable(hsotg-clk);
   }
 
 @@ -3652,16 +3659,18 @@ static int s3c_hsotg_suspend(struct platform_device 
 *pdev, pm_message_t state)
   dev_info(hsotg-dev, suspending usb gadget %s\n,
hsotg-driver-driver.name);
 
 - spin_lock_irqsave(hsotg-lock, flags);
 - s3c_hsotg_core_disconnect(hsotg);
 - s3c_hsotg_disconnect(hsotg);
 - hsotg-gadget.speed = USB_SPEED_UNKNOWN;
 - spin_unlock_irqrestore(hsotg-lock, flags);
 + if (hsotg-enabled) {

Hmm. Are you sure it's safe to check -enabled outside of the spinlock?
What happens if s3c_hsotg_udc_stop() runs right after this, before the
spinlock is taken, and disables stuff? Sure, it's a tiny window, but
still...

-- 
Paul

 + int ep;
 
 - s3c_hsotg_phy_disable(hsotg);
 + spin_lock_irqsave(hsotg-lock, flags);
 + if (hsotg-connected)
 + s3c_hsotg_core_disconnect(hsotg);
 + s3c_hsotg_disconnect(hsotg);
 + hsotg-gadget.speed = USB_SPEED_UNKNOWN;
 + spin_unlock_irqrestore(hsotg-lock, flags);
 +
 + s3c_hsotg_phy_disable(hsotg);
 
 - if (hsotg-driver) {
 - int ep;
   for (ep = 0; ep  hsotg-num_of_eps; ep++)
   s3c_hsotg_ep_disable(hsotg-eps[ep].ep);
 
 @@ -3679,21 +3688,23 @@ static int s3c_hsotg_resume(struct platform_device 
 *pdev)
   unsigned long flags;
   int ret = 0;
 
 - if (hsotg-driver) {
 + if (hsotg-driver)
   dev_info(hsotg-dev, resuming usb gadget %s\n,
hsotg-driver-driver.name);
 
 + if (hsotg-enabled) {
   clk_enable(hsotg-clk);
   ret = regulator_bulk_enable(ARRAY_SIZE(hsotg-supplies),
 -   hsotg-supplies);
 - }
 + hsotg-supplies);
 
 - s3c_hsotg_phy_enable(hsotg);
 + s3c_hsotg_phy_enable(hsotg);
 
 - spin_lock_irqsave(hsotg-lock, flags);
 - s3c_hsotg_core_init_disconnected(hsotg);
 - s3c_hsotg_core_connect(hsotg);
 - spin_unlock_irqrestore(hsotg-lock, flags);
 + spin_lock_irqsave(hsotg-lock, flags);
 + s3c_hsotg_core_init_disconnected(hsotg);
 + if (hsotg-connected)
 + s3c_hsotg_core_connect(hsotg);
 + spin_unlock_irqrestore(hsotg-lock, flags);
 + }
 
   return ret;
  }

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More 

RE: [PATCH v2 01/10] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Paul Zimmerman]

 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Monday, October 20, 2014 3:46 AM
 
 This patch adds a call to s3c_hsotg_disconnect() from 'end session'
 interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
 about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might
 look a bit more suitable for this event, but it is asserted only in
 host mode, so in device mode we need to use something else.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 6 ++
  1 file changed, 6 insertions(+)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 7b5856fadd93..119c8a3effc2 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2279,6 +2279,12 @@ irq_retry:
   dev_info(hsotg-dev, OTGInt: %08x\n, otgint);
 
   writel(otgint, hsotg-regs + GOTGINT);
 +
 + if (otgint  GOTGINT_SES_END_DET) {
 + if (hsotg-gadget.speed != USB_SPEED_UNKNOWN)
 + s3c_hsotg_disconnect(hsotg);
 + hsotg-gadget.speed = USB_SPEED_UNKNOWN;
 + }
   }
 
   if (gintsts  GINTSTS_SESSREQINT) {

Does this mean we can get rid of the call to s3c_hsotg_disconnect in
s3c_hsotg_process_control after a SET_ADDRESS is received? If not,
then s3c_hsotg_disconnect will be called twice, once here after the
disconnect, and once again after the reconnect and SET_ADDRESS.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 04/10] usb: dwc2/gadget: disable phy before turning off power regulators

[Paul Zimmerman]

 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Thursday, October 23, 2014 8:01 AM
 
 On Mon, Oct 20, 2014 at 12:45:34PM +0200, Marek Szyprowski wrote:
  This patch fixes probe function to match the pattern used elsewhere in
  the driver, where power regulators are turned off as the last element in
  the device shutdown procedure.
 
  Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 
 Paul, can I have your Acked-by here so I can send this to v3.18-rc1 ?

Sorry, I've been working on a really hot issue at $work and didn't have
time yet to review this series.

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 03/10] usb: dwc2/gadget: fix gadget unregistration in udc_stop() function

[Paul Zimmerman]

 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Thursday, October 23, 2014 8:02 AM
 
 On Mon, Oct 20, 2014 at 12:45:33PM +0200, Marek Szyprowski wrote:
  udc_stop() should clear -driver pointer unconditionally to let the UDC
  framework to work correctly with both registering/unregistering gadgets
  and enabling/disabling gadgets by writing to
  /sys/class/udc/*hsotg/soft_connect interface.
 
  Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 
 Also here. In fact, this is much more important :-)

Acked-by: Paul Zimmerman pa...@synopsys.com

Are there any more patches in this series that need immediate attention?
Otherwise I plan to finish reviewing the series on Friday or so.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCHv5 7/7] usb: dwc2: Update Kconfig to support dual-role

[Paul Zimmerman]

 From: Bartlomiej Zolnierkiewicz [mailto:b.zolnier...@samsung.com]
 Sent: Wednesday, October 22, 2014 5:26 AM
 
 On Monday, October 20, 2014 01:52:06 PM dingu...@opensource.altera.com wrote:
  From: Dinh Nguyen dingu...@opensource.altera.com
 
   config USB_DWC2_PLATFORM
  bool DWC2 Platform
  -   depends on USB_DWC2_HOST
  default USB_DWC2_HOST
 
 It should be default USB_DWC2_HOST || USB_DWC2_PERIPHERAL because
 USB_DWC2_PLATFORM replaces current USB_DWC2_PERIPHERAL functionality.
 
 Additionaly USB_DWC2_PLATFORM should be changed to tristate
 (USB_DWC2_PERIPHERAL was a tristeate before your changes).

Dinh, I think this is a good point. Is there any reason why
USB_DWC2_PLATFORM (and USB_DWC2_PCI for that matter) can't be
tristate? That's what DWC3 does.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] usb: dwc2: allow dwc2 to get built when USB_GADGET=m

[Paul Zimmerman]

 From: dingu...@opensource.altera.com [mailto:dingu...@opensource.altera.com]
 Sent: Tuesday, October 21, 2014 1:32 PM
 
 From: Dinh Nguyen dingu...@opensource.altera.com
 
 This patch allows the gadget portion of the DWC2 driver to get built when
 (!USB  USB_GADGET) condition is encountered.
 
 Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com
 ---
  drivers/usb/dwc2/Kconfig | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig
 index f93807b..4d02718 100644
 --- a/drivers/usb/dwc2/Kconfig
 +++ b/drivers/usb/dwc2/Kconfig
 @@ -1,6 +1,6 @@
  config USB_DWC2
   bool DesignWare USB2 DRD Core Support
 - depends on USB
 + depends on USB || USB_GADGET
   help
 Say Y here if your system has a Dual Role Hi-Speed USB
 controller based on the DesignWare HSOTG IP Core.

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCHv5 7/7] usb: dwc2: Update Kconfig to support dual-role

[Paul Zimmerman]

 From: Dinh Nguyen [mailto:dingu...@opensource.altera.com]
 Sent: Tuesday, October 21, 2014 1:48 PM

 diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig
 index f93807b..1ea702e 100644
 --- a/drivers/usb/dwc2/Kconfig
 +++ b/drivers/usb/dwc2/Kconfig
 @@ -1,5 +1,5 @@
  config USB_DWC2
 - bool DesignWare USB2 DRD Core Support
 + tristate DesignWare USB2 DRD Core Support
   depends on USB
   help
 Say Y here if your system has a Dual Role Hi-Speed USB
 @@ -10,31 +10,53 @@ config USB_DWC2
 bus interface module (if you have a PCI bus system) will be
 called dwc2_pci.ko, and the platform interface module (for
 controllers directly connected to the CPU) will be called
 -   dwc2_platform.ko. For gadget mode, there will be a single
 -   module called dwc2_gadget.ko.
 -
 -   NOTE: The s3c-hsotg driver is now renamed to dwc2_gadget. The
 -   host and gadget drivers are still currently separate drivers.
 -   There are plans to merge the dwc2_gadget driver with the dwc2
 -   host driver in the near future to create a dual-role driver.
 +   dwc2_platform.ko. For all modes(host, gadget and dual-role), there
 +   will be a single module called dwc2.ko.

Maybe For all modes (host, gadget and dual-role), there will be an
additional module named dwc2.ko. That would be clearer.

  if USB_DWC2
  
 +choice
 + bool DWC2 Mode Selection
 + default USB_DWC2_DUAL_ROLE if (USB  USB_GADGET)
 + default USB_DWC2_HOST if (USB  !USB_GADGET)
 + default USB_DWC2_PERIPHERAL if (!USB  USB_GADGET)
 +
  config USB_DWC2_HOST
 - tristate Host only mode
 + bool Host only mode
   depends on USB
   help
 The Designware USB2.0 high-speed host controller
 -   integrated into many SoCs.
 +   integrated into many SoCs. Select this option if you want the
 +   driver to operate in Host-only mode.
 +
 +comment Gadget/Dual-role mode requires USB Gadget support to be enabled
 +
 +config USB_DWC2_PERIPHERAL
 + bool Gadget only mode
 + depends on USB_GADGET=y || USB_GADGET=USB_DWC2
 + help
 +   The Designware USB2.0 high-speed gadget controller
 +   integrated into many SoCs. Select this option if you want the
 +   driver to operate in Peripheral-only mode. This option requires
 +   USB_GADGET=y.

Shouldn't this be This option requires USB_GADGET to be enabled? It
doesn't have to be built-in.

 +config USB_DWC2_DUAL_ROLE
 + bool Dual Role mode
 + depends on (USB=y || USB=USB_DWC2)  (USB_GADGET=y || 
 USB_GADGET=USB_DWC2)
 + help
 +   Select this option if you want the driver to work in a dual-role
 +   mode. In this mode both host and gadget features are enabled, and
 +   the role will be determined by the cable that gets plugged-in. This
 +   option requires USB_GADGET=y.

Ditto.

Once you fix these, plus the extraneous default y that Paul Bolle
pointed out, you can add my acked-by.

-- 
Paul

N�r��yb�X��ǧv�^�)޺{.n�+{��^n�r���z���h����G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�

RE: [PATCHv5 2/7] usb: dwc2: Move gadget probe function into platform code

[Paul Zimmerman]

 From: Bartlomiej Zolnierkiewicz [mailto:b.zolnier...@samsung.com]
 Sent: Wednesday, October 22, 2014 4:16 AM
 
 On Monday, October 20, 2014 01:52:01 PM dingu...@opensource.altera.com wrote:
  From: Dinh Nguyen dingu...@opensource.altera.com
 
  This patch will aggregate the probing of gadget/hcd driver into platform.c.
  The gadget probe funtion is converted into gadget_init that is now only
  responsible for gadget only initialization. All the gadget resources is now
  handled by platform.c
 
  Since the host workqueue will not get initialized if the driver is 
  configured
  for peripheral mode only. Thus we need to check for wq_otg before calling
  queue_work().
 
  Also, we move spin_lock_init to common location for both host and gadget 
  that
  is either in platform.c or pci.c.
 
  We also ove suspend/resume code to common platform code, and update it to 
  use
  the new PM API (struct dev_pm_ops).
 
  Lastly, move the samsung,s3c6400-hsotg binding into dwc2_of_match_table.
 
 This patch seems to break bisectability.  It moves all the gadget probing
 to platform.c but Kconfig/Makefile are not updated (platform.c will be
 compiled only for CONFIG_USB_DWC2_PLATFORM=y which in turn depends on
 CONFIG_USB_DWC2_HOST).  IMO patch #7 should be merged into this one (#2).

It doesn't break the compile, I already tested it. It does break the
operation of the driver until patch #7 is applied, but I think that's
OK in the middle of a patch series. I think it's a bit much to expect
the driver to keep working at each step of a patch series like this.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] usb: serial: Fix indentation style issue

[Paul Zimmerman]

 From: linux-usb-ow...@vger.kernel.org 
 [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Greg KH
 Sent: Wednesday, October 22, 2014 7:19 AM
 
 On Wed, Oct 22, 2014 at 11:51:12AM +0200, Johan Hovold wrote:
  On Sat, Oct 11, 2014 at 07:20:49AM -0700, Greg Kroah-Hartman wrote:
   On Sat, Oct 11, 2014 at 03:49:43PM +0200, Philip Munksgaard wrote:
Fix a style issue
   
Signed-off-by: Philip Munksgaard pmunksga...@gmail.com
---
 drivers/usb/serial/option.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
   
diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
index d1a3f60..d88998d 100644
--- a/drivers/usb/serial/option.c
+++ b/drivers/usb/serial/option.c
@@ -1616,7 +1616,7 @@ static const struct usb_device_id option_ids[] = {
{ USB_DEVICE(TLAYTECH_VENDOR_ID, TLAYTECH_PRODUCT_TEU800) },
{ USB_DEVICE(LONGCHEER_VENDOR_ID, FOUR_G_SYSTEMS_PRODUCT_W14),
  .driver_info = (kernel_ulong_t)four_g_w14_blacklist
-   },
+   },
  
   Why not fix the same 'space' issue on the line before this at the same
   time?
 
  And what about the remaining white-space issues in this file? Do we
  really want to go down this path?
 
 No, we don't, if you want to have patches be able to apply properly to
 older kernels, as you point out.

git-apply --ignore-whitespace ?

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 16/16] usb: dwc3: enable usb suspend phy

[Paul Zimmerman]

 From: Huang Rui [mailto:ray.hu...@amd.com]
 Sent: Monday, October 20, 2014 2:01 AM
 
 On Mon, Oct 20, 2014 at 04:41:54PM +0800, Huang Rui wrote:
  On Fri, Oct 17, 2014 at 01:48:19PM -0500, Felipe Balbi wrote:
   Hi,
  
   On Fri, Oct 17, 2014 at 06:41:04PM +, Paul Zimmerman wrote:
 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Friday, October 17, 2014 8:00 AM

 On Fri, Oct 17, 2014 at 04:53:41PM +0800, Huang Rui wrote:
  AMD NL needs to suspend usb3 ss phy, but this doesn't enable on 
  simulation
  board.
 
  Signed-off-by: Huang Rui ray.hu...@amd.com
  ---
   drivers/usb/dwc3/core.c  | 7 ++-
   drivers/usb/dwc3/dwc3-pci.c  | 3 ++-
   drivers/usb/dwc3/platform_data.h | 1 +
   3 files changed, 9 insertions(+), 2 deletions(-)
 
  diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
  index 3ccfe41..4a98696 100644
  --- a/drivers/usb/dwc3/core.c
  +++ b/drivers/usb/dwc3/core.c
  @@ -395,6 +395,9 @@ static void dwc3_phy_setup(struct dwc3 *dwc)
  if (dwc-quirks  DWC3_QUIRK_TX_DEEPH)
  reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(1);
 
  +   if (dwc-quirks  DWC3_QUIRK_SUSPHY)

 should be:

   if (!dwc-suspend_usb3_phy_quirk)

  +   reg |= DWC3_GUSB3PIPECTL_SUSPHY;

 IIRC, databook asks us to set that bit anyway, so the quirk is 
 disabling
 that bit. Am I missing something ? Paul ?
   
It looks to me that Huang's patch is enabling that bit, not disabling
it.
  
   right, but that's what's actually quirky right ? IIRC, Databook asks us
   to set usb2 and usb3 suspend phy bits and we're just not doing it.
  
Currently the driver does not set either DWC3_GUSB3PIPECTL_SUSPHY or
DWC3_GUSB2PHYCFG_SUSPHY (unless it has been added by that big patch
series you just posted). According to the databook, both of those
bits should be set to 1 after the core initialization has completed.
  
   there you go. So unless that quirk bit flag is set, we're safe of
   setting SUSPHY bit for everybody.
  
 
 
 I read the databook again, below words (DWC3_GUSB3PIPECTL_SUSPHY) is
 copied from databook:
 
 For DRD/OTG configurations, it is recommended that this bit is set to‘
 0’ during coreConsultant configuration. If it is set to ’1’, then the
 application should clear this bit after power-on reset. Application
 needs to set it to ’1’ after the core initialization is completed.
 For all other configurations, this bit can be set to ’1’ during core
 configuration.
 
 I see it's recommended to set '0' if on DRD/OTG configuration.

No, it's recommended to set it to '0' during coreConsultant
configuration. This is a part of the synthesis process, i.e. when
building the RTL for the SoC. This determines what the default value
of the bit will be when the core is reset. At runtime it is still
recommended to set it to '1' when in device mode, after the core
initialization is completed.

-- 
Paul

N�r��yb�X��ǧv�^�)޺{.n�+{��^n�r���z���h����G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�

RE: [PATCH 1/4] usb: dwc2: gadget: do not call usb_gadget_unregister_driver()

[Paul Zimmerman]

 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Friday, October 17, 2014 6:23 PM
 
 that call is completely unnecessary because
 usb_del_gadget_udc() already makes sure the
 gadget driver is properly unregistered from
 the UDC.
 
 Signed-off-by: Felipe Balbi ba...@ti.com
 ---
 
 found while reading code
 
  drivers/usb/dwc2/gadget.c | 7 ---
  1 file changed, 7 deletions(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index e55ba85..8f1502f 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -3608,14 +3608,7 @@ static int s3c_hsotg_remove(struct platform_device 
 *pdev)
   struct s3c_hsotg *hsotg = platform_get_drvdata(pdev);
 
   usb_del_gadget_udc(hsotg-gadget);
 -
   s3c_hsotg_delete_debug(hsotg);
 -
 - if (hsotg-driver) {
 - /* should have been done already by driver model core */
 - usb_gadget_unregister_driver(hsotg-driver);
 - }
 -
   clk_disable_unprepare(hsotg-clk);
 
   return 0;

Looks good to me.

Acked-by: Paul Zimmerman pa...@synopsys.com

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize

[Paul Zimmerman]

 From: linux-usb-ow...@vger.kernel.org 
 [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of David Laight
 Sent: Monday, October 20, 2014 2:48 AM
 
 From: Felipe Balbi
  According to Section 8.5.3.2 of the USB 2.0 specification,
  a USB device must terminate a Data Phase with either a
  short packet or a ZLP (if the previous transfer was
  a multiple of wMaxPacketSize).
 
  For reference, here's what the USB 2.0 specification, section
  8.5.3.2 says:
 
  
  8.5.3.2 Variable-length Data Stage
 
  A control pipe may have a variable-length data phase
  in which the host requests more data than is contained
  in the specified data structure. When all of the data
  structure is returned to the host, the function should
  indicate that the Data stage is ended by returning a
  packet that is shorter than the MaxPacketSize for the
  pipe. If the data structure is an exact multiple of
  wMaxPacketSize for the pipe, the function will return
  a zero-length packet to indicate the end of the Data
  stage.
  
 
 If that is the same as my understanding of the USB3 spec then the
 requirement for a ZLP isn't unconditional.
 
 In particular if the data phase isn't variable length then a ZLP
 must not be added.

Also, in the USB 3.0 spec, the corresponding section has been modified
a bit. The last sentence has been changed to this:

Note that if the amount of data in the data structure that is returned
to the host *is less than the amount requested by the host* and is an
exact multiple of maximum packet size then a control endpoint shall send
a zero length DP to terminate the data stage. (emphasis mine)

So I think you also need to take into account the wLength field of the
request, and only send the ZLP if the amount of data returned is less
than wLength.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 2/2] usb: dwc2: gadget: modify return statement

[Paul Zimmerman]

 From: Sudip Mukherjee [mailto:sudipm.mukher...@gmail.com]
 Sent: Friday, October 17, 2014 3:03 AM
 
 On Fri, Oct 17, 2014 at 09:02:00AM +, David Laight wrote:
  From: Of Sudip Mukherjee
   modified the function to have a single return statement at the end
   instead of multiple return statement in the middle of the function
   to improve the readability of the code.
 
  Many of us would disagree with you there.
  Early returns actually make the code easier to read, certainly
  better than a goto 'end of function'.
 
 actually , frankly speaking, this first return statement was also easier for 
 me to understand. But in
 my v1 patch , Paul mentioned :
 For a long function like this, I'd rather keep a single return point at
 the end.
 so I thought he meant all the return statements in the function.

What I didn't like about your first patch was that there were two
places where the spinlock was released. I think that is error-prone,
as can be seen by the original bug. But I am OK with leaving the
first return statement as-is, since the spinlock is not held there.

So I think we should apply patch 1, and drop patch 2.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 16/16] usb: dwc3: enable usb suspend phy

[Paul Zimmerman]

 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Friday, October 17, 2014 8:00 AM
 
 On Fri, Oct 17, 2014 at 04:53:41PM +0800, Huang Rui wrote:
  AMD NL needs to suspend usb3 ss phy, but this doesn't enable on simulation
  board.
 
  Signed-off-by: Huang Rui ray.hu...@amd.com
  ---
   drivers/usb/dwc3/core.c  | 7 ++-
   drivers/usb/dwc3/dwc3-pci.c  | 3 ++-
   drivers/usb/dwc3/platform_data.h | 1 +
   3 files changed, 9 insertions(+), 2 deletions(-)
 
  diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
  index 3ccfe41..4a98696 100644
  --- a/drivers/usb/dwc3/core.c
  +++ b/drivers/usb/dwc3/core.c
  @@ -395,6 +395,9 @@ static void dwc3_phy_setup(struct dwc3 *dwc)
  if (dwc-quirks  DWC3_QUIRK_TX_DEEPH)
  reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(1);
 
  +   if (dwc-quirks  DWC3_QUIRK_SUSPHY)
 
 should be:
 
   if (!dwc-suspend_usb3_phy_quirk)
 
  +   reg |= DWC3_GUSB3PIPECTL_SUSPHY;
 
 IIRC, databook asks us to set that bit anyway, so the quirk is disabling
 that bit. Am I missing something ? Paul ?

It looks to me that Huang's patch is enabling that bit, not disabling
it.

Currently the driver does not set either DWC3_GUSB3PIPECTL_SUSPHY or
DWC3_GUSB2PHYCFG_SUSPHY (unless it has been added by that big patch
series you just posted). According to the databook, both of those
bits should be set to 1 after the core initialization has completed.

So I think the driver should be changed to enable both of those by
default, and then maybe you want quirks to disable them if there is
some platform out there which needs that.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 1/2] usb: dwc2: gadget: sparse warning of context imbalance

[Paul Zimmerman]

 From: Sudip Mukherjee [mailto:sudipm.mukher...@gmail.com]
 Sent: Thursday, October 16, 2014 9:44 PM
 
 sparse was giving the following warning:
 warning: context imbalance in 's3c_hsotg_ep_enable'
   - different lock contexts for basic block
 
 we were returning ENOMEM while still holding the spinlock.
 The sparse warning was fixed by releasing the spinlock before return.
 
 Signed-off-by: Sudip Mukherjee su...@vectorindia.org
 ---
  drivers/usb/dwc2/gadget.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 7b5856f..7f25527 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2561,8 +2561,10 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
   hs_ep-fifo_size = val;
   break;
   }
 - if (i == 8)
 - return -ENOMEM;
 + if (i == 8) {
 + ret = -ENOMEM;
 + goto error;
 + }
   }
 
   /* for non control endpoints, set PID to D0 */
 @@ -2579,6 +2581,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
   /* enable the endpoint interrupt */
   s3c_hsotg_ctrl_epint(hsotg, index, dir_in, 1);
 
 +error:
   spin_unlock_irqrestore(hsotg-lock, flags);
   return ret;
  }

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 2/2] usb: dwc2: gadget: modify return statement

[Paul Zimmerman]

 From: Sudip Mukherjee [mailto:sudipm.mukher...@gmail.com]
 Sent: Thursday, October 16, 2014 9:44 PM
 
 modified the function to have a single return statement at the end
 instead of multiple return statement in the middle of the function
 to improve the readability of the code.
 
 This patch depends on the previous patch of the series.
 
 Signed-off-by: Sudip Mukherjee su...@vectorindia.org
 ---
  drivers/usb/dwc2/gadget.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 7f25527..e8a8fc7 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2471,7 +2471,8 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
   dir_in = (desc-bEndpointAddress  USB_ENDPOINT_DIR_MASK) ? 1 : 0;
   if (dir_in != hs_ep-dir_in) {
   dev_err(hsotg-dev, %s: direction mismatch!\n, __func__);
 - return -EINVAL;
 + ret = -EINVAL;
 + goto error1;
   }
 
   mps = usb_endpoint_maxp(desc);
 @@ -2583,6 +2584,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
 
  error:
   spin_unlock_irqrestore(hsotg-lock, flags);
 +error1:
   return ret;
  }

According to the discussion in another thread, let's drop this patch.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 1/2] usb: dwc2: gadget: sparse warning of context imbalance

[Paul Zimmerman]

 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Friday, October 17, 2014 11:52 AM
 
 On Fri, Oct 17, 2014 at 06:50:19PM +, Paul Zimmerman wrote:
   From: Sudip Mukherjee [mailto:sudipm.mukher...@gmail.com]
   Sent: Thursday, October 16, 2014 9:44 PM
  
   sparse was giving the following warning:
   warning: context imbalance in 's3c_hsotg_ep_enable'
 - different lock contexts for basic block
  
   we were returning ENOMEM while still holding the spinlock.
   The sparse warning was fixed by releasing the spinlock before return.
  
   Signed-off-by: Sudip Mukherjee su...@vectorindia.org
   ---
drivers/usb/dwc2/gadget.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
  
   diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
   index 7b5856f..7f25527 100644
   --- a/drivers/usb/dwc2/gadget.c
   +++ b/drivers/usb/dwc2/gadget.c
   @@ -2561,8 +2561,10 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
 hs_ep-fifo_size = val;
 break;
 }
   - if (i == 8)
   - return -ENOMEM;
   + if (i == 8) {
   + ret = -ENOMEM;
   + goto error;
   + }
 }
  
 /* for non control endpoints, set PID to D0 */
   @@ -2579,6 +2581,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
 /* enable the endpoint interrupt */
 s3c_hsotg_ctrl_epint(hsotg, index, dir_in, 1);
  
   +error:
 spin_unlock_irqrestore(hsotg-lock, flags);
 return ret;
}
 
  Acked-by: Paul Zimmerman pa...@synopsys.com
 
 v3.18-rc or v3.19 ?

v3.18-rc, since it's a bugfix. And I forgot, this should be marked for
3.17 stable too.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 2/2] usb: dwc2: gadget: sparse warning of context imbalance

[Paul Zimmerman]

 From: Sudip Mukherjee [mailto:sudipm.mukher...@gmail.com]
 Sent: Thursday, October 16, 2014 7:52 AM
 
 On Thu, Oct 16, 2014 at 08:21:55AM -0500, Felipe Balbi wrote:
  HI,
 
  On Thu, Oct 16, 2014 at 01:11:10PM +0530, Sudip Mukherjee wrote:
   sparse was giving the following warning:
   warning: context imbalance in 's3c_hsotg_ep_enable'
 - different lock contexts for basic block
  
   we were returning ENOMEM while still holding the spinlock.
   The sparse warning was fixed by releasing the spinlock before return.
  
   This patch depends on the previous patch of the series.
  
   Signed-off-by: Sudip Mukherjee su...@vectorindia.org
 
  this should be patch one so it can be backported to stable kernels.
 
 my v1 patch fixed only this , while reviewing that one Paul Zimmerman 
 suggested to rewrite the return
 statements.
 so this v2 series had the rewrite and the spinlock error fix.
 now if this is to be made the patch one then it will be a duplicate of my v1 
 followed by another patch
 for return statements.
 should i do that ?

Hi Sudip,

Please make the first patch like I showed in my previous reply. Then we
can mark that one for stable to fix the bug. Then make a second patch to
change the other error path.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V2] USB: dwc2: add support for the reset_controller api

[Paul Zimmerman]

 From: linux-usb-ow...@vger.kernel.org 
 [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of John Crispin
 Sent: Thursday, October 16, 2014 1:34 PM
 
 We need this for dwc2 to work on older ralink SoC like the rt3052. Without, we
 see the following when loading the driver:
 
 [0.76] dwc2 101c.usb: Bad value for GSNPSID: 0x
 
 Signed-off-by: John Crispin blo...@openwrt.org
 ---
 Changes since V1
 * move the OF lookup call into the platform code
 * add code to the cleanup path that puts the core back into reset
 
  drivers/usb/dwc2/core.h |3 +++
  drivers/usb/dwc2/hcd.c  |   11 +++
  drivers/usb/dwc2/platform.c |5 +
  3 files changed, 19 insertions(+)
 
 diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
 index 1efd10c..8dfd16a 100644
 --- a/drivers/usb/dwc2/core.h
 +++ b/drivers/usb/dwc2/core.h
 @@ -42,6 +42,7 @@
  #include linux/usb/gadget.h
  #include linux/usb/otg.h
  #include linux/usb/phy.h
 +#include linux/reset.h
  #include hw.h
 
  #ifdef DWC2_LOG_WRITES
 @@ -596,6 +597,8 @@ struct dwc2_hsotg {
   unsigned int queuing_high_bandwidth:1;
   unsigned int srp_success:1;
 
 + struct reset_control *reset_control;
 +

Please also add a kerneldoc header for this to the comment block.

   struct workqueue_struct *wq_otg;
   struct work_struct wf_otg;
   struct timer_list wkp_timer;
 diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
 index 4d918ed..ff2ca4b 100644
 --- a/drivers/usb/dwc2/hcd.c
 +++ b/drivers/usb/dwc2/hcd.c
 @@ -2764,6 +2764,14 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
 
   dev_dbg(hsotg-dev, DWC OTG HCD INIT\n);
 
 + /* bring the device out of reset */
 + if (hsotg-reset_control) {
 + int retval = reset_control_deassert(hsotg-reset_control);
 +
 + if (retval)
 + return retval;
 + }
 +
   /* Detect config values from hardware */
   retval = dwc2_get_hwparams(hsotg);
 
 @@ -2973,6 +2981,9 @@ void dwc2_hcd_remove(struct dwc2_hsotg *hsotg)
   dwc2_hcd_release(hsotg);
   usb_put_hcd(hcd);
 
 + if (hsotg-reset_control)
 + reset_control_assert(hsotg-reset_control);
 +
  #ifdef CONFIG_USB_DWC2_TRACK_MISSED_SOFS
   kfree(hsotg-last_frame_num_array);
   kfree(hsotg-frame_num_array);
 diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
 index a10e7a3..6d74583 100644
 --- a/drivers/usb/dwc2/platform.c
 +++ b/drivers/usb/dwc2/platform.c
 @@ -120,6 +120,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
   const struct dwc2_core_params *params;
   struct dwc2_core_params defparams;
   struct dwc2_hsotg *hsotg;
 + struct reset_control *reset_control;
   struct resource *res;
   int retval;
   int irq;
 @@ -171,6 +172,10 @@ static int dwc2_driver_probe(struct platform_device *dev)
   dev_dbg(dev-dev, mapped PA %08lx to VA %p\n,
   (unsigned long)res-start, hsotg-regs);
 
 + reset_control = devm_reset_control_get(dev-dev, NULL);
 + if (!IS_ERR(reset_control))
 + hsotg-reset_control = reset_control;
 +

I guess you also need a patch to the platform code / dt bindings, to
enable this? Or is it already there?

Also, please CC Felipe (ba...@ti.com) on all dwc2 patches, since they
are now going through his tree.

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: XHCI max scratchpad buffers seem to differ from specification

[Paul Zimmerman]

 From: linux-usb-ow...@vger.kernel.org 
 [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Daniel Karling
 Sent: Wednesday, October 15, 2014 2:08 PM
 
 Hi,
 
 I noticed that the macro for extracting the number of scratchpad
 buffers to allocate from the HCSPARAMS2 register differs from what is
 defined in the specification.
 
 Current macro is in xhci.h:
 
 /* bits 27:31 number of Scratchpad buffers SW must allocate for the HW */
 #define HCS_MAX_SCRATCHPAD(p)   (((p)  27)  0x1f)
 
 This macro is used in scratchpad_alloc and scratchpad_free in xhci-mem.c.
 
 According to eXtensible Host Controller Interface for Universal
 Serial Bus (xHCI) revision 1.1, section 5.3.4, bits 27:31 are only
 the five low order bits and there are five high order bits to get from
 bits 21:25.
 
 Is there any particular reason for why the macro is as it is, or is this a 
 bug?
 
 Note: I haven't seen any host controllers actually requesting so many
 scratchpad buffers that any of the high bits have to be used, but I
 thought it might cause issues in the future.

That must be a relatively recent addition to the spec. In the original
v1.0 spec, only bits 31:27 are specified for this.

-- 
Paul

RE: [PATCH] USB: dwc2: add a call to device_reset_optional()

[Paul Zimmerman]

 From: linux-usb-ow...@vger.kernel.org 
 [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of John Crispin
 Sent: Friday, October 10, 2014 10:27 AM
 
 We need this for dwc2 to work on older ralink SoC like the rt3052.
 
 Signed-off-by: John Crispin blo...@openwrt.org
 ---
  drivers/usb/dwc2/hcd.c |6 ++
  1 file changed, 6 insertions(+)
 
 diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
 index 4d918ed..8b5f966 100644
 --- a/drivers/usb/dwc2/hcd.c
 +++ b/drivers/usb/dwc2/hcd.c
 @@ -47,6 +47,7 @@
  #include linux/io.h
  #include linux/slab.h
  #include linux/usb.h
 +#include linux/reset.h
 
  #include linux/usb/hcd.h
  #include linux/usb/ch11.h
 @@ -2764,6 +2765,11 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
 
   dev_dbg(hsotg-dev, DWC OTG HCD INIT\n);
 
 + /* reset the device if a reset controller is present */
 + retval = device_reset_optional(hsotg-dev);
 + if (retval  retval != -ENOSYS)
 + return retval;
 +
   /* Detect config values from hardware */
   retval = dwc2_get_hwparams(hsotg);
 

Hmm. Can you explain this a bit more? I'm not familiar with the reset
control API.

But I see that there is currently no user of device_reset_optional() in
the kernel. Looks like drivers typically call devm_reset_control_get()
in their probe functions, then save the returned pointer and call it
later to do the reset. I wonder why dwc2 would be the only driver not
to do it that way?

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] usb: dwc2: gadget: sparse warning of context imbalance

[Paul Zimmerman]

 From: Sudip Mukherjee [mailto:sudipm.mukher...@gmail.com]
 Sent: Friday, October 10, 2014 6:10 AM
 
 sparse was giving the following warning:
   warning: context imbalance in 's3c_hsotg_ep_enable'
   - different lock contexts for basic block
 
 we were returning ENOMEM while still holding the spinlock.
 The sparse warning was fixed by releasing the spinlock before return.
 
 Signed-off-by: Sudip Mukherjee su...@vectorindia.org
 ---
  drivers/usb/dwc2/gadget.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 7b5856f..046e90d 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2561,8 +2561,10 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
   hs_ep-fifo_size = val;
   break;
   }
 - if (i == 8)
 + if (i == 8) {
 + spin_unlock_irqrestore(hsotg-lock, flags);
   return -ENOMEM;
 + }
   }
 
   /* for non control endpoints, set PID to D0 */

For a long function like this, I'd rather keep a single return point at
the end. Something like this:

}
-   if (i == 8)
-   return -ENOMEM;
+   if (i == 8) {
+   ret = -ENOMEM;
+   goto out;
+   }
}
...
 
+out:
spin_unlock_irqrestore(hsotg-lock, flags);
return ret;
 }

Care to respin the patch like that?

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html