Re: [PATCH 1/5 v2] USB: regroup all depends on USB within an if USB block

[balbi]

Hi,

On Tue, Apr 02, 2013 at 07:10:22PM +0200, Florian Fainelli wrote:
> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> index 05e5143..ab5a3b9 100644
> --- a/drivers/usb/musb/Kconfig
> +++ b/drivers/usb/musb/Kconfig
> @@ -6,7 +6,6 @@
>  # (M)HDRC = (Multipoint) Highspeed Dual-Role Controller
>  config USB_MUSB_HDRC
>   tristate 'Inventra Highspeed Dual Role Controller (TI, ADI, ...)'
> - depends on USB && USB_GADGET

shouldn't:

depends on USB_GADGET

be left here ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] drivers: usb: dwc3 : Configure DMA properties and ops from DT

[Felipe Balbi]

Hi,

Rajesh Bhagat  writes:
> On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set
> to be able to do DMA allocations, so use the of_dma_configure() helper
> to populate the dma properties and assign an appropriate dma_ops.
>
> Signed-off-by: Rajesh Bhagat 
> Reviewed-by: Yang-Leo Li 

Cool, nxp is also using dwc3 :-) C'mon Rajesh, send us a glue layer :)

> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index c679f63..4d5b783 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -17,6 +17,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  #include "core.h"
>  
> @@ -32,6 +33,9 @@ int dwc3_host_init(struct dwc3 *dwc)
>   return -ENOMEM;
>   }
>  
> + if (IS_ENABLED(CONFIG_OF) && dwc->dev->of_node)
> + of_dma_configure(&xhci->dev, dwc->dev->of_node);

okay, so we have a long discussion about this going on. You can catch up
with it starting here:

http://marc.info/?i=1461612094-30939-1-git-send-email-grygorii.stras...@ti.com

At least for now, this patch will be applied. We need to have a better
solution for this, one that helps not only DT platforms.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: xhci-mtk: fixup mouse wakeup failure during system suspend

[Felipe Balbi]

Hi,

chunfeng yun  writes:
> On Tue, 2016-05-03 at 12:33 +0300, Felipe Balbi wrote:
>> Hi,
>> 
>> chunfeng yun  writes:
>> >> chunfeng yun  writes:
>> >> > On Thu, 2016-04-21 at 10:04 +0800, Chunfeng Yun wrote:
>> >> >> Click mouse after xhci suspend completion but before system suspend
>> >> >> completion, system will not be waken up by mouse if the duration of
>> >> >> them is larger than 20ms which is the device UFP's resume signaling
>> >> 
>> >> what is "them" here ? The duration of what is longer than 20ms ?
>> > They are "xhci suspend completion" and "system suspend completion";
>> >
>> > It's time duration
>> 
>> okay. So if xhci suspend takes longer than 20ms your SPM doesn't see a
>> wakeup ?
> It is not the time of xhci suspend consumed, but is the time of duration
> from xhci suspend completion to system suspend completion(after BOOT-CPU
> is turned off, SPM will be enabled to receive wakeup event)

Okay, so SPM will be the entity actually handling wakeups, right ? I'm
assuming something like this happens:

echo mem > /sys/power/state
 /* start suspending devices */
  xhci_suspend()
 /* all devices suspended */
 enable_spm()

so, if a mouse button is pressed after xhci_suspend() returns but before
enable_spm() runs, then we're gonna miss that event, am I right ?

I can't think of any way to sort this out. Let's ask on linux-pm (I've
added linux-pm to Cc list)

>> >> >> lasted. Another reason is that the SPM is not enabled before system
>> >> 
>> >> what's SPM ?
>> > It is System Power Management which is powered off when system is
>> > running in normal mode, and is powered on when system enters suspend
>> > mode. It is used to wakeup system when some wakeup sources, such as
>> > bluetooth or powerkey etc, tigger wakeup event.
>> 
>> okay, thanks
>> 
>> >> >> suspend compeltion, this causes SPM also not notice the resume signal.
>> >>^^
>> >>completion
>> >> 
>> >> >> So in order to reduce the duration less than 20ms, make use of
>> >> >> syscore's suspend/resume interface.
>> >> 
>> >> no, this is the wrong approach
>> > But it seems only one workable approach from software side
>> 
>> I wouldn't say that. It seems to me SPM should be enabled earlier.
> Yes, but normally SPM should be enabled after all CPUS are turned off,
> so it's difficult to do that, I mean enable SPM before turning off CPUS

is it a requirement that SPM should be enabled only after all CPUs are
turned off ? If that's the case, then any device in the system might
have missed wakeups. This doesn't seem like a good design to me; unless
I'm missing something...

>> >> >> Because the syscore runs on irq disabled context, and xhci's
>> >> >> suspend/resume calls some sleeping functions, enable local irq
>> >> >> and then disable it during suspend/resume. This may be not a problem,
>> >> >> since only boot CPU is runing.
>> >> 
>> >> another problem :) calling local_irq_{enable,disable}() is an indication
>> >> that something's wrong.
>> > Oh!
>> >
>> > BTW: There will be warning logs if they are not called.
>> 
>> yeah, I got that :-) But it's still wrong to use
>> local_irq_{enable,disable}() the way you're using them :-)
> Yes
>
> Thank you very much.
>> 
>
>

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: misc: usbtest: fix pattern tests for scatterlists.

[Felipe Balbi]

Hi,

Alan Stern  writes:
>> Alan Stern  writes:
>> > On Mon, 2 May 2016, Mathias Nyman wrote:
>> >
>> >> The current implemenentation restart the sent pattern for each entry in
>> >> the sg list. The receiving end expects a continuous pattern, and test
>> >> will fail unless scatterilst entries happen to be aligned with the
>> >> pattern
>> >
>> > Ah.  We didn't spot this earlier because for non-xHCI controllers, the 
>> > scatterlist entries _must_ be aligned with the maxpacket size and 
>> > therefore with the pattern.
>> 
>> interesting. We actually found a similar issue with XHCI. scatterlist
>> has to be aligned to wMaxPacketSize but only before a link TRB. Mathias
>> has been working on a solution which involves memcpy()ing enough bytes
>> to align to wMaxPacketSize before the link TRB (it's very infrequent as
>> we have 256 TRBs per segment on XHCI), but if you know of a nicer way,
>> we're all ears :-)
>
> You should be able to handle this without memcpy'ing anything.
>
> If the individual scatterlist entries are large enough, you can simply

"if" being the keyword here :-) We check for that, yes :-)

> end the ring segment early.  Either put a link TRB before the physical
> segment end, at the last point where the cumulative transfer size is
> divisible by maxpacket, or else fill out from there to the end of the
> segment with no-op TRBs.  Common case: Each scatterlist entry is a

no-op TRBs was tried before, but it didn't work well and patch was
reverted.

> multiple of 512 bytes and the maxpacket size is 1024.  Then you either

that's not common case for testusb. One of the test cases (see below)
exercises exactly small sg entries:

# try to trigger short OUT processing bugs
echo "test 7a: $COUNT scatterlists, variable size/short entries"
do_test -t 7 -v 579
BUFLEN=4096
echo "test 7b: $COUNT scatterlists, variable size/bigger entries"
do_test -t 7 -v 41
BUFLEN=64
echo "test 7c: $COUNT scatterlists, variable size/micro entries"
do_test -t 7 -v 63

(above is from tools/usb/hcd-tests.sh)

> have to split the last entry in two or move it completely into the next
> ring segment.

right, this is easy enough to do and that we (well, Mathias) already
have working :-)

> This approach doesn't work quite as well if the scatterlist entries are
> very small.  For instance, if they are 8 bytes or smaller then you
> might have to fill out the segment with 128 or more no-op TRBs, which
> is not so good if the segment can hold only 256 TRBs.  I suppose we
> could simply rule this out by requiring SG transfers to have a minimum
> entry size of 128 bytes, or something like that.

that's not a good idea, IMO. HCD drivers should be robust enough in
these situations.

>> /me goes dig EHCI
>
> Not sure that will help.  The same issue could arise there, if the 
> scatterlist entries were smaller than 512 bytes.  The driver doesn't 
> handle this case properly, but it works out okay because the case never 
> comes up.

see testusb :-) You did mention, on another thread, that you ran
testusb. And, btw, years ago I used to run these tests on daily basis on
MUSB. It also seems that Synopsys' XHCI (part of dwc3 when configured as
dual-role) is immune to this link TRB alignment limitation because I was
running hcd-tests.sh also on a daily basis on TI's AM437x SK and IDK
boards (one as host, one as peripheral).

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH net-next 4/5] treewide: replace dev->trans_start update with helper

[Felipe Balbi]

Hi,

Florian Westphal  writes:
> Replace all trans_start updates with netif_trans_update helper.
> change was done via spatch:
>
> struct net_device *d;
> @@
> - d->trans_start = jiffies
> + netif_trans_update(d)
>
> Compile tested only.
>
> Cc: user-mode-linux-de...@lists.sourceforge.net
> Cc: linux-xte...@linux-xtensa.org
> Cc: linux1394-de...@lists.sourceforge.net
> Cc: linux-r...@vger.kernel.org
> Cc: net...@vger.kernel.org
> Cc: mpt-fusionlinux@broadcom.com
> Cc: linux-s...@vger.kernel.org
> Cc: linux-...@vger.kernel.org
> Cc: linux-par...@vger.kernel.org
> Cc: linux-o...@vger.kernel.org
> Cc: linux-h...@vger.kernel.org
> Cc: linux-usb@vger.kernel.org
> Cc: linux-wirel...@vger.kernel.org
> Cc: linux-s...@vger.kernel.org
> Cc: de...@driverdev.osuosl.org
> Cc: b.a.t.m@lists.open-mesh.org
> Cc: linux-blueto...@vger.kernel.org
> Signed-off-by: Florian Westphal 
> ---

for u_ether.c:

Acked-by: Felipe Balbi 

> diff --git a/drivers/usb/gadget/function/u_ether.c 
> b/drivers/usb/gadget/function/u_ether.c
> index 637809e..a3f7e7c 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -597,7 +597,7 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
>   DBG(dev, "tx queue err %d\n", retval);
>   break;
>   case 0:
> -     net->trans_start = jiffies;
> + netif_trans_update(net);
>   atomic_inc(&dev->tx_qlen);
>   }

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed

[Felipe Balbi]

Hi,

Jim Lin  writes:



>>> In f_fs.c
>>> "
>>> static int __ffs_data_do_os_desc(enum ffs_os_desc_type type,
>>>struct usb_os_desc_header *h, void *data,
>>>unsigned len, void *priv)
>>> {
>>>   struct ffs_data *ffs = priv;
>>>   u8 length;
>>>
>>>   ENTER();
>>>
>>>   switch (type) {
>>>   case FFS_OS_DESC_EXT_COMPAT: {
>>>   struct usb_ext_compat_desc *d = data;
>>>   int i;
>>>
>>>   if (len < sizeof(*d) ||
>>>   d->bFirstInterfaceNumber >= ffs->interfaces_count ||
>>>   d->Reserved1)
>>>   return -EINVAL;
>>> "
>> that's fine, but this is only failing because something else is
>> returning the wrong set of descriptors (SS vs HS). That's the bug we
>> want to fix, not work around it.
>>
> Thanks.

you're welcome, but to fix that bug we need more information. Why is
composite.c using the wrong set of descriptors ? What is your setup ?

Are you using an in-kernel gadget ? which one ? Using configfs or legacy
gadgets ? gadgetfs ? f_fs ? How to trigger this ? Can you provide
instructions and (in case of gadgetfs/ffs) code to create a gadget that
hits this problem ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Felipe Balbi]

Hi,

John Youn  writes:
>> John Youn  writes:
>>>> "Du, Changbin"  writes:
>>>>> Hi, Balbi,
>>>>>
>>>>> The step to reproduce this issue is:
>>>>> 1) connect device to a host and wait its enumeration.
>>>>> 2) trigger software disconnect by calling function
>>>>> usb_gadget_disconnect(), which finally call
>>>>>dwc3_gadget_pullup(false). Do not reconnect device
>>>>>   (I mean no enumeration go on, keep bit Run/Stop 0.).
>>>>>
>>>>> At here, gadget driver's disconnect callback should be
>>>>> Called, right? We has been disconnected. But no, as
>>>>> You said " not generating disconnect IRQ after you
>>>>> drop Run/Stop is expected".
>>>>>
>>>>> And I am testing on an Android device, Android only
>>>>> use dwc3_gadget_pullup(false) to issue a soft disconnection.
>>>>> This confused user that the UI still show usb as connected
>>>>> State, caused by missing a disconnect event.
>>>>
>>>> okay, so I know what this is. This is caused by Android gadget itself
>>>> not notifying the gadget that a disconnect has happened. Just look at
>>>> udc-core's soft_connect implementation for the sysfs interface, and
>>>> you'll see what I mean.
>>>>
>>>> This should be fixed at Android gadget itself. The only thing we could
>>>> do is introduce a new usb_gadget_soft_connect()/disconnect() to wrap the
>>>> logic so it's easier for Android gadget to use; but even that I'm a
>>>> little bit reluctant to do because Android should be using our
>>>> soft_connect interface instead of reimplementing it (wrongly) by its
>>>> own.
>>>>
>>>
>>> We've run in to the same issue with our usb_gadget_driver.
>>>
>>> If the usb_gadget_disconnect() API function, which seems like it is
>>> intended to be called by the gadget_driver, does cause the gadget to
>>> disconnect, it seems reasonable to expect the gadget or the UDC core
>>> to notify the gadget_driver via the callback.
>> 
>> Well, the API is supposed to disconnect D+ pullup and that's about it.
>> 
>>> As you mentioned this is handled in the soft_disconnect sysfs. Why
>>> shouldn't usb_gadget_disconnect() do the same thing, if not the gadget
>> 
>> because there might be cases where we don't need/want the gadget to know
>> about this disconnect.
>> 
>
> But what if we do?

well, if the gadget is the one faking a disconnect, then it ought to
cancel requests and do all the other necessary steps, right ? :-)

>>> itself? Exposing the sysfs as an API function would work too. Though
>> 
>> it already _is_ exported. I just don't know why people are re-inventing
>> the same solution :-)
>> 
>>> both functions are "soft" disconnects and both are called
>>> "disconnect".
>>>
>>> In our gadget_driver we do the workaround where we notify ourself that
>>> we called the usb_gadget_disconnect() and that we should now be
>> 
>> you could just rely on the sysfs interface, right ? :-)
>
> Not from the gadget driver, at least I don't think so. The gadget
> driver itself is the one that wants to initiate the soft disconnect.

I need to understand this requirement of yours a little better. Can you
describe exactly what your gadget is doing ? Also, any chance of showing
the code for that gadget ? I don't mind carrying an extra function
driver if it helps you validate your IP :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: xhci-mtk: fixup mouse wakeup failure during system suspend

[Felipe Balbi]

Hi,

chunfeng yun  writes:
>> chunfeng yun  writes:
>> > On Tue, 2016-05-03 at 12:33 +0300, Felipe Balbi wrote:
>> >> Hi,
>> >> 
>> >> chunfeng yun  writes:
>> >> >> chunfeng yun  writes:
>> >> >> > On Thu, 2016-04-21 at 10:04 +0800, Chunfeng Yun wrote:
>> >> >> >> Click mouse after xhci suspend completion but before system suspend
>> >> >> >> completion, system will not be waken up by mouse if the duration of
>> >> >> >> them is larger than 20ms which is the device UFP's resume signaling
>> >> >> 
>> >> >> what is "them" here ? The duration of what is longer than 20ms ?
>> >> > They are "xhci suspend completion" and "system suspend completion";
>> >> >
>> >> > It's time duration
>> >> 
>> >> okay. So if xhci suspend takes longer than 20ms your SPM doesn't see a
>> >> wakeup ?
>> > It is not the time of xhci suspend consumed, but is the time of duration
>> > from xhci suspend completion to system suspend completion(after BOOT-CPU
>> > is turned off, SPM will be enabled to receive wakeup event)
>> 
>> Okay, so SPM will be the entity actually handling wakeups, right ? I'm
>> assuming something like this happens:
>> 
>> echo mem > /sys/power/state
>>  /* start suspending devices */
>>   xhci_suspend()
>>  /* all devices suspended */
>>  enable_spm()
>> 
>> so, if a mouse button is pressed after xhci_suspend() returns but before
>> enable_spm() runs, then we're gonna miss that event, am I right ?
> Yes, you are right.
>> 
>> I can't think of any way to sort this out. Let's ask on linux-pm (I've
>> added linux-pm to Cc list)
> Thanks
>> 
>> >> >> >> lasted. Another reason is that the SPM is not enabled before system
>> >> >> 
>> >> >> what's SPM ?
>> >> > It is System Power Management which is powered off when system is
>> >> > running in normal mode, and is powered on when system enters suspend
>> >> > mode. It is used to wakeup system when some wakeup sources, such as
>> >> > bluetooth or powerkey etc, tigger wakeup event.
>> >> 
>> >> okay, thanks
>> >> 
>> >> >> >> suspend compeltion, this causes SPM also not notice the resume 
>> >> >> >> signal.
>> >> >>^^
>> >> >>completion
>> >> >> 
>> >> >> >> So in order to reduce the duration less than 20ms, make use of
>> >> >> >> syscore's suspend/resume interface.
>> >> >> 
>> >> >> no, this is the wrong approach
>> >> > But it seems only one workable approach from software side
>> >> 
>> >> I wouldn't say that. It seems to me SPM should be enabled earlier.
>> > Yes, but normally SPM should be enabled after all CPUS are turned off,
>> > so it's difficult to do that, I mean enable SPM before turning off CPUS
>> 
>> is it a requirement that SPM should be enabled only after all CPUs are
>> turned off ? If that's the case, then any device in the system might
>> have missed wakeups. This doesn't seem like a good design to me; unless
>> I'm missing something...
> Yes, it's a requirement.
>
> If the wakeup source is level trigger, and will keep it until SPM notice
> it. Although USB wakeup is level trigger, but it just last at most 20ms
> and clear itself automatically.

who's clearing USB wakeup after 20ms ? Sure, it's a requirement by the
USB spec that host should drive reset for *at least* 20ms, but I don't
remember the exact wordings WRT remote wakeup

/me reads

Okay, here it is, section 7.1.7.7 of USB 2.0 Spec states:

"The remote wakeup device must hold the resume signaling for at
least 1 ms but for no more than 15 ms (TDRSMUP). At the end of
this period, the device stops driving the bus (puts its drivers
into the high-impedance state and does not drive the bus to the
J state)."

IOW, the mouse is not at fault here. Remote wakeup can last anywhere
between 1ms and 15ms. If your system can't cope with it, then I'd say
it's a problem with your system and you shouldn't allow XHCI to go to
such deep power states until SPM is enabled.

I really tried finding the correct piece of code which does this for PCI
devices, but basically it's treated as an IRQ and as long as
enable_irq_wake() or the sysfs file is set to true, then PME gets
enabled (hopefully PCI folks can confirm this statement).

Very recently Tony Lindgren added proper handling of wakeup signals as
IRQs for non-PCI systems too (see 4990d4fe327b PM / Wakeirq: Add
automated device wake IRQ handling), wonder if you could treat SPM
similarly.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed

[Felipe Balbi]

Hi Jim,

Jim Lin  writes:
> On 2016年05月04日 18:37, Felipe Balbi wrote:
>> * PGP Signed by an unknown key
>>
>>
>> Hi,
>>
>> Jim Lin  writes:
>>
>> 
>>
>>>>> In f_fs.c
>>>>> "
>>>>> static int __ffs_data_do_os_desc(enum ffs_os_desc_type type,
>>>>> struct usb_os_desc_header *h, void *data,
>>>>> unsigned len, void *priv)
>>>>> {
>>>>>struct ffs_data *ffs = priv;
>>>>>u8 length;
>>>>>
>>>>>ENTER();
>>>>>
>>>>>switch (type) {
>>>>>case FFS_OS_DESC_EXT_COMPAT: {
>>>>>struct usb_ext_compat_desc *d = data;
>>>>>int i;
>>>>>
>>>>>if (len < sizeof(*d) ||
>>>>>d->bFirstInterfaceNumber >= ffs->interfaces_count ||
>>>>>d->Reserved1)
>>>>>return -EINVAL;
>>>>> "
>>>> that's fine, but this is only failing because something else is
>>>> returning the wrong set of descriptors (SS vs HS). That's the bug we
>>>> want to fix, not work around it.
>>>>
>>> Thanks.
>> you're welcome, but to fix that bug we need more information. Why is
>> composite.c using the wrong set of descriptors ? What is your setup ?
>>
>> Are you using an in-kernel gadget ? which one ?
> No, our gadget driver is on the way to submit.
>> Using configfs or legacy
>> gadgets ? gadgetfs ? f_fs ?
>
>>   How to trigger this ? Can you provide
>> instructions and (in case of gadgetfs/ffs) code to create a gadget that
>> hits this problem ?
>>
> Please refer to
> https://android.googlesource.com/platform/system/core/+/master/adb/usb_linux_client.cpp

according to this, there is a set of SuperSpeed descriptors starting on
linux 169:

https://android.googlesource.com/platform/system/core/+/master/adb/usb_linux_client.cpp#169

I don't get what the problem is. You mentioned something about SS vs HS
descriptors at some point, but that shouldn't be a problem seen that ADB
provides SS descriptors.

> Also this is a thought coming from another engineer for your reference.
> "
>
> I think Microsoft and linux are contradicting the requirements. 
> According MSFT's os descriptor definition, one of the reserved fields 
> needs to be set to 1 whereas seems like f_fs.c expects them to be 0. 
> (copy pasting from the spec downloaded from: 
> https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx) 

I see..

> What does upstream think ? Requires some conflict resolution I guess !! 
> Since the OS descriptors are from MSFT, I believe upstream has to drop 
> the check and I think this patch might be valid..

If we difer from the spec, we need to remain compliant. I can see adb
sets this to a 1 as the spec requires:

https://android.googlesource.com/platform/system/core/+/master/adb/usb_linux_client.cpp#206

Now I understand the problem, it's not related to SS vs HS, it's just us
using the wrong check for Reserved1. Here's one thing though, the patch
isn't exactly correct. Instead of removing the check completely, we
*must* force the correct check. IOW:

if (len < sizeof(*d) ||
d->bFirstInterfaceNumber >= ffs->interfaces_count ||
-   d->Reserved1)
+   !d->Reserved1)

Heh, now your commit log makes more sense as well, but it could use some
rewording. It appears, from that commit, that the problem is writing
without SS descriptors, which it isn't. The real problem is the wrong
check of the Reserved1 field in MSFT OS Descriptor.

cheers

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 02/10] usb: musb: Fix idling after host mode by increasing autosuspend delay

[Felipe Balbi]

Hi,

Bin Liu  writes:
>> Does the current 200 ms autosuspend timeout relate to anything real
>> other than what I found out with my measurements?
>
> Not sure, I didn't checkk where the 200ms comes from.

It was an arbitrary number from when runtime PM was first added by
Hema. I don't think it has any esoteric meaning ;-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Felipe Balbi]

Hi Guenter,

Guenter Roeck  writes:
> On Tue, Feb 09, 2016 at 07:01:20PM +0200, Heikki Krogerus wrote:
>> Hi,
>> 
>> The OS, or more precisely the user space, needs to be able to control
>> a few things regarding USB Type-C ports. The first thing that must be
>> allowed to be controlled is the data role. USB Type-C ports will
>> select the data role randomly with DRP ports. When USB PD is
>> supported, also independent (from data role) power role swapping can
>> be supported together with Alternate Mode control.
>> 
>> I'm proposing with this set a Class for the Type-C connectors that
>> gives the user space control over those things on top of getting basic
>> details about the USB Type-C connectors and also partners. The details
>> include the capabilities of the port, the supported data and power
>> roles, supported accessories (audio and debug), supported Alternate
>> Modes, USB PD support and of course the type of the partner (USB, Alt
>> Mode, Accessory or Charger), and more or less the same details about
>> the partner.
>> 
>> I'm not considering cables with this Class, and I have deliberately
>> left out some more technical details, like cable orientation, firstly
>> because I did not see much use for the user space from knowing that
>> an secondly because that kind of details are not always available for
>> example with UCSI.
>> 
>> So the interface to the user space is kept as simple as I dared to
>> make it.
>> 
>> NOTE: In case there is somebody wondering, this is not adding USB PD
>> support to Linux kernel. This is just about USB Type-C.
>> 
>
> Hello Heikki,
>
> we have implemented a prototype TCPM (USB Type-C Protocol Manager)
> software on top of your patch set. It will support TCPCI as well
> as other USB-C controllers such as FUSB302. The plan is to use
> this software in systems where no separate controller is available.
>
> Is there any chance to advance this patch set ? It would be instrumental
> to get a unified interface to user space.

A newer version of $subject is already in Greg's queue [1]

[1] 
https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=0c1849a8c7af652c92ad0265a7ca5934fd773c69

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: misc: usbtest: fix pattern tests for scatterlists.

[Felipe Balbi]

Hi,

Alan Stern  writes:
> On Wed, 4 May 2016, Felipe Balbi wrote:
>
>> > multiple of 512 bytes and the maxpacket size is 1024.  Then you either
>> 
>> that's not common case for testusb. One of the test cases (see below)
>> exercises exactly small sg entries:
>> 
>>  # try to trigger short OUT processing bugs
>>  echo "test 7a: $COUNT scatterlists, variable size/short entries"
>>  do_test -t 7 -v 579
>>  BUFLEN=4096
>>  echo "test 7b: $COUNT scatterlists, variable size/bigger entries"
>>  do_test -t 7 -v 41
>>  BUFLEN=64
>>  echo "test 7c: $COUNT scatterlists, variable size/micro entries"
>>  do_test -t 7 -v 63
>
> These tests will fail if you run them on an EHCI host controller.
>
>> (above is from tools/usb/hcd-tests.sh)
>> 
>> > have to split the last entry in two or move it completely into the next
>> > ring segment.
>> 
>> right, this is easy enough to do and that we (well, Mathias) already
>> have working :-)
>> 
>> > This approach doesn't work quite as well if the scatterlist entries are
>> > very small.  For instance, if they are 8 bytes or smaller then you
>> > might have to fill out the segment with 128 or more no-op TRBs, which
>> > is not so good if the segment can hold only 256 TRBs.  I suppose we
>> > could simply rule this out by requiring SG transfers to have a minimum
>> > entry size of 128 bytes, or something like that.
>> 
>> that's not a good idea, IMO. HCD drivers should be robust enough in
>> these situations.
>
> Why?  Just so that hcd-tests.sh can complete with no errors?  I don't 

and some off-the-shelf usb-network adapters can work ;-)

> know of any actual drivers or user programs that do real work and need 
> such small SG entry sizes.

Well, apparently some ASIX network adapters fail similarly, but perhaps
they provide sg entries big enough to be split without any memcpy() ? Dunno.

>> >> /me goes dig EHCI
>> >
>> > Not sure that will help.  The same issue could arise there, if the 
>> > scatterlist entries were smaller than 512 bytes.  The driver doesn't 
>> > handle this case properly, but it works out okay because the case never 
>> > comes up.
>> 
>> see testusb :-) You did mention, on another thread, that you ran
>> testusb.
>
> Your counterexample isn't really testusb; it's hcd-tests.sh.  I haven't 
> tried running that.

That just calls testusb and is the same test.sh from many, many years
ago which Dave wrote. At [1] you can find Dave's usbtest page on
linux-usb.org and at [2] you can find a direct link to test.sh.

>> And, btw, years ago I used to run these tests on daily basis on
>> MUSB. It also seems that Synopsys' XHCI (part of dwc3 when configured as
>> dual-role) is immune to this link TRB alignment limitation because I was
>> running hcd-tests.sh also on a daily basis on TI's AM437x SK and IDK
>> boards (one as host, one as peripheral).
>
> If creating bounce buffers for small SG transfers is so common (i.e., 
> necessary for more than one host controller driver) then it really 
> belongs in a common library somewhere.

no arguments there ;-)

[1] http://www.linux-usb.org/usbtest/
[2] http://www.linux-usb.org/usbtest/test.sh

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: Fix binding to UDC via configfs interface

[Felipe Balbi]

Hi,

Krzysztof Opasiak  writes:
> By default user could store only valid UDC name in configfs UDC
> attr by doing:
>
> echo $UDC_NAME > UDC
>
> Commit (855ed04 "usb: gadget: udc-core: independent registration of
> gadgets and gadget drivers") broke this behavior and allowed to store
> any arbitrary string in UDC file and udc core was waiting for such
> controller to appear.
>
> echo "any arbitrary string here" > UDC
>
> This commit fix this by adding a flag which prevents configfs
> gadget from being added to list of pending drivers if UDC with
> given name has not been found.
>
> Signed-off-by: Krzysztof Opasiak 

Thank you, this is great. I'll apply it as soon as -rc1 is tagged :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Felipe Balbi]

Hi,

Peter Chen  writes:
>> "Du, Changbin"  writes:
>> > Hi, Balbi,
>> >
>> > The step to reproduce this issue is:
>> > 1) connect device to a host and wait its enumeration.
>> > 2) trigger software disconnect by calling function
>> > usb_gadget_disconnect(), which finally call
>> >dwc3_gadget_pullup(false). Do not reconnect device
>> >   (I mean no enumeration go on, keep bit Run/Stop 0.).
>> >
>> > At here, gadget driver's disconnect callback should be
>> > Called, right? We has been disconnected. But no, as
>> > You said " not generating disconnect IRQ after you
>> > drop Run/Stop is expected".
>> >
>> > And I am testing on an Android device, Android only
>> > use dwc3_gadget_pullup(false) to issue a soft disconnection.
>> > This confused user that the UI still show usb as connected
>> > State, caused by missing a disconnect event.
>> 
>> okay, so I know what this is. This is caused by Android gadget itself
>> not notifying the gadget that a disconnect has happened. Just look at
>> udc-core's soft_connect implementation for the sysfs interface, and
>> you'll see what I mean.
>> 
>> This should be fixed at Android gadget itself. The only thing we could
>> do is introduce a new usb_gadget_soft_connect()/disconnect() to wrap the
>> logic so it's easier for Android gadget to use; but even that I'm a
>> little bit reluctant to do because Android should be using our
>> soft_connect interface instead of reimplementing it (wrongly) by its
>> own.
>> 
>
> If it is a gadget driver, it can call its disconnect explicitly.
> Another thing is the gadget driver should not call usb_gadget_disconnect
> directly, it should call usb_gadget_deactivate or usb_function_deactivate.
>
> Since currently, calling usb_gadget_disconnect may not do real pull down
> dp, Felipe, will you consider adding gadget_driver->disconnect into
> usb_gadget_disconnect after pull down dp?

this is the detail that I'm not yet entirely sure is always valid. Would
there ever be a situation where we want to drop pull-ups but not tell
the gadget about it ? I'm not sure. Anybody wants to investigate ?
Otherwise I'll add it to my TODO list.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Felipe Balbi]

Hi,

Peter Chen  writes:
>> Peter Chen  writes:
>> >> "Du, Changbin"  writes:
>> >> > Hi, Balbi,
>> >> >
>> >> > The step to reproduce this issue is:
>> >> > 1) connect device to a host and wait its enumeration.
>> >> > 2) trigger software disconnect by calling function
>> >> > usb_gadget_disconnect(), which finally call
>> >> >dwc3_gadget_pullup(false). Do not reconnect device
>> >> >   (I mean no enumeration go on, keep bit Run/Stop 0.).
>> >> >
>> >> > At here, gadget driver's disconnect callback should be
>> >> > Called, right? We has been disconnected. But no, as
>> >> > You said " not generating disconnect IRQ after you
>> >> > drop Run/Stop is expected".
>> >> >
>> >> > And I am testing on an Android device, Android only
>> >> > use dwc3_gadget_pullup(false) to issue a soft disconnection.
>> >> > This confused user that the UI still show usb as connected
>> >> > State, caused by missing a disconnect event.
>> >> 
>> >> okay, so I know what this is. This is caused by Android gadget itself
>> >> not notifying the gadget that a disconnect has happened. Just look at
>> >> udc-core's soft_connect implementation for the sysfs interface, and
>> >> you'll see what I mean.
>> >> 
>> >> This should be fixed at Android gadget itself. The only thing we could
>> >> do is introduce a new usb_gadget_soft_connect()/disconnect() to wrap the
>> >> logic so it's easier for Android gadget to use; but even that I'm a
>> >> little bit reluctant to do because Android should be using our
>> >> soft_connect interface instead of reimplementing it (wrongly) by its
>> >> own.
>> >> 
>> >
>> > If it is a gadget driver, it can call its disconnect explicitly.
>> > Another thing is the gadget driver should not call usb_gadget_disconnect
>> > directly, it should call usb_gadget_deactivate or usb_function_deactivate.
>> >
>> > Since currently, calling usb_gadget_disconnect may not do real pull down
>> > dp, Felipe, will you consider adding gadget_driver->disconnect into
>> > usb_gadget_disconnect after pull down dp?
>> 
>> this is the detail that I'm not yet entirely sure is always valid. Would
>> there ever be a situation where we want to drop pull-ups but not tell
>> the gadget about it ?
>
> Yes, we have.
>
> - We have enabled pullup dp default for most of platforms, and for
> gadget obex and uvc will pull down it since it wants app to
> control it
>
> - Some USB charger detection design may need it for the secondary
> detection.
>
> After checking again for usb_gadget_disconnect and
> usb_gadget_disconnect, I think there is no possible that the dp is
> still pulled after we call usb_gadget_disconnect, so we don't need
> to change anything:)

alright, cool. So we'll keep the requirement to call
gadget_driver->disconnect() for anybody who calls
usb_gadget_disconnect(). Caller should know if we need to notify
gadget_driver or not about that disconnect.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3] usb: host: xhci-rcar: Avoid long wait in xhci_reset()

[Felipe Balbi]

Hi,

Yoshihiro Shimoda  writes:
> The firmware of R-Car USB 3.0 host controller will control the reset.
> So, if the xhci driver doesn't do firmware downloading (e.g. kernel
> configuration is CONFIG_USB_XHCI_PLATFORM=y and CONFIG_USB_XHCI_RCAR
> is not set), the reset of USB 3.0 host controller doesn't work
> correctly. Then, the host controller will cause long wait in
> xhci_reset() because the CMD_RESET bit of op_regs->command is not
> cleared for 10 seconds.
>
> So, this patch modifies the Kconfig to enable both CONFIG_USB_XHCI_PLATFORM
> and CONFIG_USB_XHCI_RCAR.
>
> Fixes: 4ac8918f3a7 (usb: host: xhci-plat: add support for the R-Car H2 and M2 
> xHCI controllers)
> Cc:  # v3.17+
>
> Signed-off-by: Yoshihiro Shimoda 

looks good to me, thanks :)

Reviewed-by: Felipe Balbi 

> ---
>  Changes from v2:
>   - Modify the Kconfig instead of xhci-rcar.h
> http://www.spinics.net/lists/linux-usb/msg139681.html
> http://www.spinics.net/lists/linux-usb/msg139722.html
>
>  Changes from v1:
>   - Revise the commit log.
> http://www.spinics.net/lists/stable/msg130007.html
>
>  drivers/usb/host/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 3050b18..e9d4dde 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -35,6 +35,7 @@ config USB_XHCI_PCI
>  
>  config USB_XHCI_PLATFORM
>   tristate "Generic xHCI driver for a platform device"
> + select USB_XHCI_RCAR if ARCH_RENESAS
>   ---help---
> Adds an xHCI host driver for a generic platform device, which
> provides a memory space and an irq.
> @@ -63,7 +64,7 @@ config USB_XHCI_MVEBU
>  
>  config USB_XHCI_RCAR
>   tristate "xHCI support for Renesas R-Car SoCs"
> - select USB_XHCI_PLATFORM
> + depends on USB_XHCI_PLATFORM
>   depends on ARCH_RENESAS || COMPILE_TEST
>   ---help---
> Say 'Y' to enable the support for the xHCI host controller
> -- 
> 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

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Felipe Balbi]

Hi,

John Youn  writes:
>>>>> As you mentioned this is handled in the soft_disconnect sysfs. Why
>>>>> shouldn't usb_gadget_disconnect() do the same thing, if not the gadget
>>>>
>>>> because there might be cases where we don't need/want the gadget to know
>>>> about this disconnect.
>>>>
>>>
>>> But what if we do?
>> 
>> well, if the gadget is the one faking a disconnect, then it ought to
>
> It's not a really "faking" since the dwc3 controller supports soft
> disconnect :)

It's "fake" in the sense that pins are still mated in the connector,
that's what I mean by fake ;)

>> cancel requests and do all the other necessary steps, right ? :-)
>> 
>
> It does take those steps whenever it's notified of disconnect via its
> disconnect callback. But since it doesn't get the callback when the
> disconnect happens, it has to call it explicitly. What I'm saying is

right, and that's the requirement. You're implementing it correctly.

> that the API or UDC is the one that should call it since it knows that
> the usb_gadget_disconnect() API was called and/or the UDC
> disconnected.

but udc-core doesn't know why it was called :) see
usb_function_activate()/deactivate(), we don't want to cancel requests
in that case, just delay the moment when host notices a port change.

>>>>> itself? Exposing the sysfs as an API function would work too. Though
>>>>
>>>> it already _is_ exported. I just don't know why people are re-inventing
>>>> the same solution :-)
>>>>
>>>>> both functions are "soft" disconnects and both are called
>>>>> "disconnect".
>>>>>
>>>>> In our gadget_driver we do the workaround where we notify ourself that
>>>>> we called the usb_gadget_disconnect() and that we should now be
>>>>
>>>> you could just rely on the sysfs interface, right ? :-)
>>>
>>> Not from the gadget driver, at least I don't think so. The gadget
>>> driver itself is the one that wants to initiate the soft disconnect.
>> 
>> I need to understand this requirement of yours a little better. Can you
>> describe exactly what your gadget is doing ? Also, any chance of showing
>> the code for that gadget ? I don't mind carrying an extra function
>> driver if it helps you validate your IP :-)
>> 
>
> This gadget driver does a programmable disconnect during testing. I
> don't think it will be released anytime soon. Which is why I never
> bothered to submit a fix. Also note that this isn't a function but a
> gadget driver (same place in the stack as libcomposite framework). I'm
> not sure if we have those anymore in the kernel.

We still have those, yes. But it seems like your stuff should be
integrated into composite.c itself under a #ifdef USB_GADGET_TESTING or
something like that.

If it helps IP providers using a real OS (linux, of course heh) for
silicon validation, I'd be very happy to integrate these
changes. There's a lot which we can still do to make Linux more
interesting for IP providers and SoC integrators (from validation point
of view), if this minimal change helps, we should do it :)

In fact, if you want to list out some extra requirements for dwc3's
trace functionality, I'd be happy to implement those. Are you missing
any visibility into some internal controller state ? Any sort of tooling
which you'd like to have ?

Let me know ;)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/1] usb: net2272: avoid shifting 0

[Felipe Balbi]

Heinrich Schuchardt  writes:
> Remove redundant code.
> Or'ing with a shifted value of zero is a NOP.
>
> Signed-off-by: Heinrich Schuchardt 

it sure is and the compiler is getting rid of those. They are there as
means of documentation only ;-)

I really have no strong feelings about this patch, so I'll let the other
folks who have been hacking on this to decide.

-- 
balbi
--
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: misc: usbtest: fix pattern tests for scatterlists.

[Felipe Balbi]

Hi,

Alan Stern  writes:
> On Fri, 6 May 2016, Felipe Balbi wrote:
>
>> >> that's not a good idea, IMO. HCD drivers should be robust enough in
>> >> these situations.
>> >
>> > Why?  Just so that hcd-tests.sh can complete with no errors?  I don't 
>> 
>> and some off-the-shelf usb-network adapters can work ;-)
>> 
>> > know of any actual drivers or user programs that do real work and need 
>> > such small SG entry sizes.
>> 
>> Well, apparently some ASIX network adapters fail similarly, but perhaps
>> they provide sg entries big enough to be split without any memcpy() ? Dunno.
>
> Has anybody tried running one of these network adapters under EHCI?

dunno

>> > Your counterexample isn't really testusb; it's hcd-tests.sh.  I haven't 
>> > tried running that.
>> 
>> That just calls testusb and is the same test.sh from many, many years
>> ago which Dave wrote. At [1] you can find Dave's usbtest page on
>> linux-usb.org and at [2] you can find a direct link to test.sh.
>
> Okay.  Still, the fact remains that I haven't tried test.sh.  Only 
> testusb run directly from the command line.

well, then EHCI has been broken for the past 15 years or so :-) Luckily
Mathias has a fix which, potentially, can be made generic.

-- 
balbi
--
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: usb: dwc2: regression on MyBook Live Duo / Canyonlands since 4.3.0-rc4

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Monday 09 May 2016 10:23:22 Benjamin Herrenschmidt wrote:
>> On Sun, 2016-05-08 at 13:44 +0200, Christian Lamparter wrote:
>> > On Sunday, May 08, 2016 08:40:55 PM Benjamin Herrenschmidt wrote:
>> > > 
>> > > On Sun, 2016-05-08 at 00:54 +0200, Christian Lamparter via Linuxppc-dev 
>> > > wrote:
>> > > > 
>> > > > I've been looking in getting the MyBook Live Duo's USB OTG port
>> > > > to function. The SoC is a APM82181. Which has a PowerPC 464 core
>> > > > and related to the supported canyonlands architecture in
>> > > > arch/powerpc/.
>> > > > 
>> > > > Currently in -next the dwc2 module doesn't load: 
>> > > Smells like the APM implementation is little endian. You might need to
>> > > use a flag to indicate what endian to use instead and set it
>> > > appropriately based on some DT properties.
>> > I tried. As per common-properties[0], I added little-endian; but it has no
>> > effect. I looked in dwc2_driver_probe and found no way of specifying the
>> > endian of the device. It all comes down to the dwc2_readl & dwc2_writel
>> > accessors. These - sadly - have been hardwired to use __raw_readl and
>> > __raw_writel. So, it's always "native-endian". While common-properties
>> > says little-endian should be preferred.
>> 
>> Right, I meant, you should produce a patch adding a runtime test inside
>> those functions based on a device-tree property, a bit like we do for
>> some of the HCDs like OHCI, EHCI etc...
>> 
>> 
>
> The patch that caused the problem had multiple issues:
>
> - it broke big-endian ARM kernels: any machine that was working
>   correctly with a little-endian kernel is no longer using byteswaps
>   on big-endian kernels, which clearly breaks them.
> - On PowerPC the same thing must be true: if it was working before,
>   using big-endian kernels is now broken. Unlike ARM, 32-bit PowerPC
>   usually uses big-endian kernels, so they are likely all broken.
> - The barrier for dwc2_writel is on the wrong side of the __raw_writel(),
>   so the MMIO no longer synchronizes with DMA operations.
> - On architectures that require specific CPU instructions for MMIO
>   access, using the __raw_ variant may turn this into a pointer
>   dereference that does not have the same effect as the readl/writel.
>
> I think we can simply make this set of accessors architecture-dependent
> (MIPS vs. the rest of the world) to revert ARM and PowerPC back to
> the working version.

and patch all drivers similarly? Shouldn't arch/mips itself deal with it
and hide it from drivers ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: add DWC3_GUCTL1 reg for debug

[Felipe Balbi]

William Wu  writes:

> Signed-off-by: William Wu 

no changelog = no commit, sorry. Why do you want to dump GUCTL1?

> ---
>  drivers/usb/dwc3/core.h| 1 +
>  drivers/usb/dwc3/debugfs.c | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index e15e307..f268869 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -86,6 +86,7 @@
>  #define DWC3_GCTL0xc110
>  #define DWC3_GEVTEN  0xc114
>  #define DWC3_GSTS0xc118
> +#define DWC3_GUCTL1  0xc11c
>  #define DWC3_GSNPSID 0xc120
>  #define DWC3_GGPIO   0xc124
>  #define DWC3_GUID0xc128
> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> index b1dd3c6..f3c9f44 100644
> --- a/drivers/usb/dwc3/debugfs.c
> +++ b/drivers/usb/dwc3/debugfs.c
> @@ -47,6 +47,7 @@ static const struct debugfs_reg32 dwc3_regs[] = {
>   dump_register(GCTL),
>   dump_register(GEVTEN),
>   dump_register(GSTS),
> + dump_register(GUCTL1),
>   dump_register(GSNPSID),
>   dump_register(GGPIO),
>   dump_register(GUID),
> -- 
> 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

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 3/4] usb: dwc3: make usb2 phy interface configurable in DT

[Felipe Balbi]

Hi,

William Wu  writes:
> Add snps,phyif_utmi_16_bits devicetree property. USB2 phy

this needs a quirk_ prefix...

> interface is hardware property, and it's platform dependent,
> so we need to configure it in devicetree to set the core to
> support a UTMI+ PHY with an 8- or 16-bit interface.
>
> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM
> must set to the corresponding value according to the usb2
> phy interface.

right, that's fine. But also note on section 8.1.1 Table 8-1 where it
states:

|-+|
| GUSB2PHYCFG | Program the following PHY configuration fields: USBTrdTim, |
| | FSIntf, PHYIf, TOUTCal, or leave the default values if |
| | the correct power-on values were selected during   |
| | coreConsultant configuration.  Note: The PHY must not  |
| | be enabled for auto-resume in device mode. Hence the   |
| | field GUSB2PHYCFG[15] (ULPIAutoRes) must be written|
| | with '0' during the power-on initialization in case|
| | the reset value is '1'.|
| ||
|-+|

You only need this because your core was badly configured in
coreConsultant.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 3/4] usb: dwc3: make usb2 phy interface configurable in DT

[Felipe Balbi]

Hi William,

William Wu  writes:
> On 05/09/2016 08:18 PM, Felipe Balbi wrote:
>> Hi,
>>
>> William Wu  writes:
>>> Add snps,phyif_utmi_16_bits devicetree property. USB2 phy
>> this needs a quirk_ prefix...
>   Yes, maybe a quirk is more proper. As you mentioned,
>   the PHYIf can be configured during coreconsultant.
>   But for some specific usb cores(e.g. rk3399 soc dwc3),
>   the default PHYIf configuration is error, so we need to
>   reconfigure it by software.

alright, thanks for confirming. Let's call this one a quirk, then.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: add DWC3_GUCTL1 reg for debug

[Felipe Balbi]

William Wu  writes:
> On 05/09/2016 08:10 PM, Felipe Balbi wrote:
>> William Wu  writes:
>  Thanks Felipe Balbi and Greg KH. I'm really sorry that I forgot to 
> add changelog.
>>> Signed-off-by: William Wu 
>> no changelog = no commit, sorry. Why do you want to dump GUCTL1?
>  Because GUCTL1 can be written by user. For rockchip platform,
>  we set GUCTL1.DEV_FORCE_20_CLK_FOR_30_CLK(bit26) to 1 in 
> bootrom(used for usb2.0 device only),
>  and after kernel boot, I want to check whether this bit can be 
> reset to default 0 after core reset.
>  Dump GUCTL1 reg from debugfs is more convenient for me.

cool, something like this could go to commit log :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/4] usb: dwc3: of-simple: add compatible for rockchip

[Felipe Balbi]

Hi,

Doug Anderson  writes:
> William,
>
> On Mon, May 9, 2016 at 4:46 AM, William Wu  wrote:
>> Signed-off-by: William Wu 
>> ---
>>  drivers/usb/dwc3/dwc3-of-simple.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
>> b/drivers/usb/dwc3/dwc3-of-simple.c
>> index 9743353..1f3665b 100644
>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>> @@ -162,6 +162,7 @@ static const struct dev_pm_ops dwc3_of_simple_dev_pm_ops 
>> = {
>>  static const struct of_device_id of_dwc3_simple_match[] = {
>> { .compatible = "qcom,dwc3" },
>> { .compatible = "xlnx,zynqmp-dwc3" },
>> +   { .compatible = "rockchip,dwc3" },
>
> It is, of course, up to Felipe.  ...but personally I'd prefer that
> things here be sorted alphabetically.  Sorting things in a consistent
> manner tends to reduce merge conflicts as the list gets longer and
> also makes it easier to find things.

I agree, let's keep it sorted :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/4] usb: dwc3: of-simple: add compatible for rockchip

[Felipe Balbi]

Hi,

Brian Norris  writes:
> Hi William,
>
> Did you leave off linux-rockc...@lists.infradead.org intentionally? IMO,
> it's nice to have that list in CC, so interested parties can follow your
> work, even if they aren't as fortunate as me to have been CC'd on your
> patch directly.
>
> On Mon, May 09, 2016 at 07:46:14PM +0800, William Wu wrote:
>> Signed-off-by: William Wu 
>> ---
>>  drivers/usb/dwc3/dwc3-of-simple.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
>> b/drivers/usb/dwc3/dwc3-of-simple.c
>> index 9743353..1f3665b 100644
>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>> @@ -162,6 +162,7 @@ static const struct dev_pm_ops dwc3_of_simple_dev_pm_ops 
>> = {
>>  static const struct of_device_id of_dwc3_simple_match[] = {
>>  { .compatible = "qcom,dwc3" },
>>  { .compatible = "xlnx,zynqmp-dwc3" },
>> +{ .compatible = "rockchip,dwc3" },
>
> Add to Documentation/devicetree/bindings/. Do we need a new
> Documentation/devicetree/bindings/usb/rockchip,dwc3.txt, to match the
> pattern of qcom and xlnx? Or can we just add to dwc3.txt, since so far,
> all bindings are documented in the common file?

dwc3.txt is for dwc3.ko. We need separate files for rockchip, xilinx and
qualcomn :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/4] usb: dwc3: of-simple: add compatible for rockchip

[Felipe Balbi]

Hi William,

William Wu  writes:
> Dear Felipe & Doug,
>  Thanks for your proposal. It's a good idea to sort the list.
>   I'll fix it next patch version.

cool, thanks.

ps: top-posting is frowned upon here. Please avoid it ;-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 03/14] usb: hcd.h: Add OTG to HCD interface

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> On 10/05/16 06:14, Peter Chen wrote:
>> On Mon, May 09, 2016 at 12:45:38PM +0300, Roger Quadros wrote:
>>> On 06/05/16 12:41, Peter Chen wrote:
>>>> On Mon, May 02, 2016 at 03:18:46PM +0300, Roger Quadros wrote:
>>>>> The OTG core will use struct otg_hcd_ops to interface
>>>>> with the HCD controller.
>>>>>
>>>>> The main purpose of this interface is to avoid directly
>>>>> calling HCD APIs from the OTG core as they
>>>>> wouldn't be defined in the built-in symbol table if
>>>>> CONFIG_USB is m.
>>>>>
>>>>> Signed-off-by: Roger Quadros 
>>>>> Acked-by: Peter Chen 
>>>>
>>>> Roger, after thinking more, I still think current dependency between
>>>> OTG, HCD and gadget are too complicated. Since the OTG can't work
>>>> if it is built as module, I suggest letting OTG depends on HCD &&
>>>> USB_GADGET, and it is a boolean, in that case, we don't need to
>>>> export any HCD and gadget ops, things will be much simpler.
>>>> What's your opinion?
>>>
>>> How will it work if HCD and USB_GADGET are modules and OTG is built-in?
>>>
>> 
>> The OTG will not be compiled at this situation, since it is boolean.
>> In fact, like I mentioned at above, OTG or USB function can't work if
>> it is built as module.
>
> Isn't this a limitation?

I agree, it should work built-in or module.

> As per the current implementation dual role works fine even with both
> USB_GADGET and HCD as module.
>
> In the real world it is unlikely that GADGET and HCD will be built-in.

we can't make this assumption, however :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 4/5] usb: dwc3: omap: Pass VBUS and ID events transparently

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> Don't make any decisions regarding VBUS session based on ID
> status. That is best left to the OTG core.
>
> Pass ID and VBUS events independent of each other so that OTG
> core knows exactly what to do.
>
> This makes dual-role with extcon work with OTG irq on OMAP platforms.
>
> Signed-off-by: Roger Quadros 

I have a feeling this will regress OMAP5432 uEVM. Did you test with that
board ?

cheers

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> TRM [1] recommends that POWERPRESENT bit must not be
> set and left at it's default value of 0.
>
> [1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
> Section 23.11.4.5.1 Mailbox VBUS/ID Management
>
> "Because PIPE powerpresent has a different meaning in host and in device mode,
> and because of the redundancy with the UTMI signals, the controller ORes
> together the appropriate PIPE and UTMI inputs to create its internal
> VBUS status. For that reason, it is recommended to leave field
> USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and only 
> to
> fill in the USB2 VBUS status fields in the same register."
>
> Signed-off-by: Roger Quadros 

to make sure we avoid regressions, do you mind sharing on which
platforms you tested this patch ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> We intend to share this interrupt with the OTG driver an to ensure
> that irqflags match for the shared interrupt handlers we use
> request_threaded_irq()
>
> If we don't use request_treaded_irq() then forced threaded irq will
> set IRQF_ONESHOT and this won't match with the OTG IRQ handler's
> IRQ flags.
>
> NOTE: OTG IRQ handler is yet to be added. This is a preparatory step.
>
> Signed-off-by: Roger Quadros 
> ---
>  drivers/usb/dwc3/dwc3-omap.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
> index 59ea35f..6504e94 100644
> --- a/drivers/usb/dwc3/dwc3-omap.c
> +++ b/drivers/usb/dwc3/dwc3-omap.c
> @@ -272,16 +272,22 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void 
> *_omap)
>  {
>   struct dwc3_omap*omap = _omap;
>   u32 reg;
> + int ret = IRQ_NONE;
>  
>   reg = dwc3_omap_read_irqmisc_status(omap);
>  
> + if (reg)
> + ret = IRQ_HANDLED;
> +
>   dwc3_omap_write_irqmisc_status(omap, reg);
>  
>   reg = dwc3_omap_read_irq0_status(omap);
> + if (reg)
> + ret = IRQ_HANDLED;
>  
>   dwc3_omap_write_irq0_status(omap, reg);
>  
> - return IRQ_HANDLED;
> + return ret;
>  }
>  
>  static void dwc3_omap_enable_irqs(struct dwc3_omap *omap)
> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>   /* check the DMA Status */
>   reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>  
> - ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
> - "dwc3-omap", omap);
> + ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
> + NULL, 0, "dwc3-omap", omap);

if you're using threaded_irq, it's better to have a NULL top half and
valid bottom half.

In fact, since this will be shared, you could do a proper preparation
and on top half check if $this device generated the IRQ and
conditionally schedule the bottom half. Don't forget to mask device's
interrupts from top half so you can run without IRQF_ONESHOT.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 5/5] usb: dwc3: core: cleanup IRQ resources

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> Implementations might use different IRQs for
> host, gadget and OTG so use named interrupt resources
> to allow Device tree to specify the 3 interrupts.
>
> Following are the interrupt names
>
> Peripheral Interrupt - peripheral
> HOST Interrupt - host
> OTG Interrupt - otg
>
> We still maintain backward compatibility for a single named
> interrupt for all 3 interrupts (e.g. for dwc3-pci) and
> single unnamed interrupt for all 3 interrupts (e.g. old DT).

cool :-)

> @@ -748,6 +750,20 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>   }
>   break;
>   case USB_DR_MODE_OTG:
> + dwc->otg_irq = platform_get_irq_byname(dwc3_pdev, "otg");
> + if (dwc->otg_irq <= 0) {
> + dwc->otg_irq = platform_get_irq_byname(dwc3_pdev,
> +"dwc_usb3");
> + if (dwc->otg_irq <= 0) {
> + res = platform_get_resource(dwc3_pdev,
> + IORESOURCE_IRQ, 0);
> + if (!res) {
> + dev_err(dwc->dev, "missing otg IRQ\n");
> + return -ENODEV;
> + }
> + dwc->otg_irq = res->start;
> + }
> + }

I suppose this part can be removed and added only when OTG is
supported. Then dwc3_otg_init() (or whatever) can do this.

> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 186a886..2e20892 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -716,6 +716,8 @@ struct dwc3_scratchpad_array {
>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
>   * @revision: revision register contents
>   * @dr_mode: requested mode of operation
> + * @gadget_irq: IRQ number for Peripheral IRQs
> + * @otg_irq: IRQ number for OTG IRQs
>   * @usb2_phy: pointer to USB2 PHY
>   * @usb3_phy: pointer to USB3 PHY
>   * @usb2_generic_phy: pointer to USB2 PHY
> @@ -817,6 +819,9 @@ struct dwc3 {
>  
>   enum usb_dr_modedr_mode;
>  
> + int gadget_irq;
> + int otg_irq;

while at that, let's add host_irq too and do proper changes to dwc3/host.c

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index c3b0d01..8db8d13 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1605,7 +1605,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>   int irq;
>   u32 reg;
>  
> - irq = platform_get_irq(to_platform_device(dwc->dev), 0);
> + irq = dwc->gadget_irq;
>   ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
>   IRQF_SHARED, "dwc3", dwc->ev_buf);
>   if (ret) {
> @@ -2781,6 +2781,23 @@ static irqreturn_t dwc3_interrupt(int irq, void *_evt)
>  int dwc3_gadget_init(struct dwc3 *dwc)
>  {
>   int ret;
> + struct resource *res;
> + struct platform_device *dwc3_pdev = to_platform_device(dwc->dev);
> +
> + dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev, "peripheral");
> + if (dwc->gadget_irq <= 0) {
> + dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev,
> +   "dwc_usb3");
> + if (dwc->gadget_irq <= 0) {
> + res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ,
> + 0);
> + if (!res) {
> + dev_err(dwc->dev, "missing peripheral IRQ\n");
> + return -ENODEV;
> + }
> + dwc->gadget_irq = res->start;
> + }
> + }
>  
>   dwc->ctrl_req = dma_alloc_coherent(dwc->dev, sizeof(*dwc->ctrl_req),
>   &dwc->ctrl_req_addr, GFP_KERNEL);

you're regressing dwc3_gadget_stop().

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> On 10/05/16 12:54, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Roger Quadros  writes:
>>> TRM [1] recommends that POWERPRESENT bit must not be
>>> set and left at it's default value of 0.
>>>
>>> [1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
>>> Section 23.11.4.5.1 Mailbox VBUS/ID Management
>>>
>>> "Because PIPE powerpresent has a different meaning in host and in device 
>>> mode,
>>> and because of the redundancy with the UTMI signals, the controller ORes
>>> together the appropriate PIPE and UTMI inputs to create its internal
>>> VBUS status. For that reason, it is recommended to leave field
>>> USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and 
>>> only to
>>> fill in the USB2 VBUS status fields in the same register."
>>>
>>> Signed-off-by: Roger Quadros 
>> 
>> to make sure we avoid regressions, do you mind sharing on which
>> platforms you tested this patch ?
>> 
> I tested this on omap5-uevm and dra7-evm.
> My am437x board stopped working so couldn't test on that one.

would you have a colleague or perhaps an automated test-farm which could
run the test for you ? :-)

I can take the patch, no problem, but if there are any regressions don't
blame me :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 4/5] usb: dwc3: omap: Pass VBUS and ID events transparently

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> On 10/05/16 12:55, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Roger Quadros  writes:
>>> Don't make any decisions regarding VBUS session based on ID
>>> status. That is best left to the OTG core.
>>>
>>> Pass ID and VBUS events independent of each other so that OTG
>>> core knows exactly what to do.
>>>
>>> This makes dual-role with extcon work with OTG irq on OMAP platforms.
>>>
>>> Signed-off-by: Roger Quadros 
>> 
>> I have a feeling this will regress OMAP5432 uEVM. Did you test with that
>> board ?
>> 
>
> Yes. Any specific test case you would like me to test?
> For now I'm just doing enumeration with g_zero.

IIRC OMAP5 uEVM didn't have separate VBUS and ID GPIOs. How are you
handling that case ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>>> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>>> /* check the DMA Status */
>>> reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>>>  
>>> -   ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
>>> -   "dwc3-omap", omap);
>>> +   ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
>>> +   NULL, 0, "dwc3-omap", omap);
>> 
>> if you're using threaded_irq, it's better to have a NULL top half and
>> valid bottom half.
>
> But in this case we don't need a bottom half as there is nothing to do :).
>
>> 
>> In fact, since this will be shared, you could do a proper preparation
>> and on top half check if $this device generated the IRQ and
>> conditionally schedule the bottom half. Don't forget to mask device's
>> interrupts from top half so you can run without IRQF_ONESHOT.
>> 
>
> Why do this at all if there is nothing to do in the bottom half?

oh, but there is :-)

The whole idea of threaded IRQs is that you spend as little time as
possible on top half and the (strong) recommendation is that you *only*
check if $this device generated the interrupt. Note that "checking if
$this device generated the interrupt" will be mandatory as soon as you
mark the IRQ line as shared ;-)

So here's how this should look like:

static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
{
struct dwc3_omap *omap = _omap;
u32 reg;

reg = readl(IRQSTATUS)
if (reg) {
mask_interrupts(omap);
return IRQ_WAKE_THREAD;
}

return IRQ_HANDLED;
}

static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
{
struct dwc3_omap *omap = _omap;
u32 reg;

spin_lock(&omap->lock);
reg = readl(IRQSTATUS);

if (reg & BIT0)
handle_bit_0(omap);

if (reg & BIT1)
handle_bit_1(omap);

unmask_interrupts(omap);
    spin_unlock(&omap->lock);

return IRQ_HANDLED;
}

this will *always* behave well with RT and non-RT kernels. It also
allows for the user to change priorities on these interrupt handlers if
necessary.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 5/5] usb: dwc3: core: cleanup IRQ resources

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 186a886..2e20892 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -716,6 +716,8 @@ struct dwc3_scratchpad_array {
>>>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
>>>   * @revision: revision register contents
>>>   * @dr_mode: requested mode of operation
>>> + * @gadget_irq: IRQ number for Peripheral IRQs
>>> + * @otg_irq: IRQ number for OTG IRQs
>>>   * @usb2_phy: pointer to USB2 PHY
>>>   * @usb3_phy: pointer to USB3 PHY
>>>   * @usb2_generic_phy: pointer to USB2 PHY
>>> @@ -817,6 +819,9 @@ struct dwc3 {
>>>  
>>> enum usb_dr_modedr_mode;
>>>  
>>> +   int gadget_irq;
>>> +   int otg_irq;
>> 
>> while at that, let's add host_irq too and do proper changes to dwc3/host.c
>
> Sure. So we add host_irq here, and manually create an irq resource
> in dwc3_host_init?

right :-) Then the code looks similar for otg, peripheral and host parts ;-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 4/5] usb: dwc3: omap: Pass VBUS and ID events transparently

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>>>> Roger Quadros  writes:
>>>>> Don't make any decisions regarding VBUS session based on ID
>>>>> status. That is best left to the OTG core.
>>>>>
>>>>> Pass ID and VBUS events independent of each other so that OTG
>>>>> core knows exactly what to do.
>>>>>
>>>>> This makes dual-role with extcon work with OTG irq on OMAP platforms.
>>>>>
>>>>> Signed-off-by: Roger Quadros 
>>>>
>>>> I have a feeling this will regress OMAP5432 uEVM. Did you test with that
>>>> board ?
>>>>
>>>
>>> Yes. Any specific test case you would like me to test?
>>> For now I'm just doing enumeration with g_zero.
>> 
>> IIRC OMAP5 uEVM didn't have separate VBUS and ID GPIOs. How are you
>> handling that case ?
>> 
> It comes from the PMIC, extcon-palmas.c.

alright, then :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> On 10/05/16 13:04, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Roger Quadros  writes:
>>> On 10/05/16 12:54, Felipe Balbi wrote:
>>>>
>>>> Hi,
>>>>
>>>> Roger Quadros  writes:
>>>>> TRM [1] recommends that POWERPRESENT bit must not be
>>>>> set and left at it's default value of 0.
>>>>>
>>>>> [1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
>>>>> Section 23.11.4.5.1 Mailbox VBUS/ID Management
>>>>>
>>>>> "Because PIPE powerpresent has a different meaning in host and in device 
>>>>> mode,
>>>>> and because of the redundancy with the UTMI signals, the controller ORes
>>>>> together the appropriate PIPE and UTMI inputs to create its internal
>>>>> VBUS status. For that reason, it is recommended to leave field
>>>>> USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and 
>>>>> only to
>>>>> fill in the USB2 VBUS status fields in the same register."
>>>>>
>>>>> Signed-off-by: Roger Quadros 
>>>>
>>>> to make sure we avoid regressions, do you mind sharing on which
>>>> platforms you tested this patch ?
>>>>
>>> I tested this on omap5-uevm and dra7-evm.
>>> My am437x board stopped working so couldn't test on that one.
>> 
>> would you have a colleague or perhaps an automated test-farm which could
>> run the test for you ? :-)
>> 
>> I can take the patch, no problem, but if there are any regressions don't
>> blame me :-)
>> 
> Don't worry, blame is on the TRM then :).

alright then, so seems like v8 will be queued. Just remember we're
pretty close to opening the merge window and I have already sent a pull
request to Greg. On the bright side, your patches will sit in linux-next
for quite some time :-) (well, until -rc1 is tagged)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 5/5] usb: dwc3: core: cleanup IRQ resources

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>> Roger Quadros  writes:
>>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>>> index 186a886..2e20892 100644
>>>>> --- a/drivers/usb/dwc3/core.h
>>>>> +++ b/drivers/usb/dwc3/core.h
>>>>> @@ -716,6 +716,8 @@ struct dwc3_scratchpad_array {
>>>>>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
>>>>>   * @revision: revision register contents
>>>>>   * @dr_mode: requested mode of operation
>>>>> + * @gadget_irq: IRQ number for Peripheral IRQs
>>>>> + * @otg_irq: IRQ number for OTG IRQs
>>>>>   * @usb2_phy: pointer to USB2 PHY
>>>>>   * @usb3_phy: pointer to USB3 PHY
>>>>>   * @usb2_generic_phy: pointer to USB2 PHY
>>>>> @@ -817,6 +819,9 @@ struct dwc3 {
>>>>>  
>>>>>   enum usb_dr_modedr_mode;
>>>>>  
>>>>> + int gadget_irq;
>>>>> + int otg_irq;
>>>>
>>>> while at that, let's add host_irq too and do proper changes to dwc3/host.c
>>>
>>> Sure. So we add host_irq here, and manually create an irq resource
>>> in dwc3_host_init?
>> 
>> right :-) Then the code looks similar for otg, peripheral and host parts ;-)
>> 
> Just saw that host_irq is not used anywhere other than creating the XHCI 
> platform
> device. So I don't see why we need host_irq in struct dwc3.
> It is obtained in dwc3_host_init() and consumed there itself.

fair enough.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>> Roger Quadros  writes:
>>>>> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device 
>>>>> *pdev)
>>>>>   /* check the DMA Status */
>>>>>   reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>>>>>  
>>>>> - ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
>>>>> - "dwc3-omap", omap);
>>>>> + ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
>>>>> + NULL, 0, "dwc3-omap", omap);
>>>>
>>>> if you're using threaded_irq, it's better to have a NULL top half and
>>>> valid bottom half.
>>>
>>> But in this case we don't need a bottom half as there is nothing to do :).
>>>
>>>>
>>>> In fact, since this will be shared, you could do a proper preparation
>>>> and on top half check if $this device generated the IRQ and
>>>> conditionally schedule the bottom half. Don't forget to mask device's
>>>> interrupts from top half so you can run without IRQF_ONESHOT.
>>>>
>>>
>>> Why do this at all if there is nothing to do in the bottom half?
>> 
>> oh, but there is :-)
>> 
>> The whole idea of threaded IRQs is that you spend as little time as
>> possible on top half and the (strong) recommendation is that you *only*
>> check if $this device generated the interrupt. Note that "checking if
>> $this device generated the interrupt" will be mandatory as soon as you
>> mark the IRQ line as shared ;-)
>> 
>> So here's how this should look like:
>> 
>> static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
>> {
>> struct dwc3_omap *omap = _omap;
>> u32 reg;
>> 
>> reg = readl(IRQSTATUS)
>> if (reg) {
>> mask_interrupts(omap);
>>  return IRQ_WAKE_THREAD;
>> }
>> 
>>  return IRQ_HANDLED;
>
> This should be IRQ_NONE right?

possibly, testing will say ;-)

>> static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
>> {
>> struct dwc3_omap *omap = _omap;
>> u32 reg;
>> 
>> spin_lock(&omap->lock);
>
> Do we really need a spin_lock for the dwc3-omap driver?
> Currently we won't be doing anything other than just
> clearing the irqstatus and re-enabling the interrupts.

well, if there's no possibility of races, then no. But only testing will
say for sure, I guess. I didn't really go through the entire thing just
to a write a quick little template :-p

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: f_fs: report error if excess data received

[Felipe Balbi]

Hi,

changbin...@intel.com writes:
> From: "Du, Changbin" 
>
> Since the buffer size for req is rounded up to maxpacketsize,
> then we may end up with more data then user space has space
> for.

only for OUT direction with the controller you're using ;-)

> If it happen, we can keep the excess data for next i/o, or
> report an error. But we cannot silently drop data, because
> USB layer should ensure the data integrality it has transferred,
> otherwise applications may get corrupt data if it doesn't
> detect this case.

and when has this actually happened ? Host should not send more data in
this case, if it does, it's an error on the host side. Also, returning
-EOVERFLOW is not exactly correct here, because you'd violate POSIX
specification of read(), right ?

> Here, we simply report an error to userspace to let userspace
> proccess. Actually, userspace applications should negotiate

no, this violates POSIX. Care to explain what problem are you actually
facing ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>>>> static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
>>>> {
>>>> struct dwc3_omap *omap = _omap;
>>>> u32 reg;
>>>>
>>>> spin_lock(&omap->lock);
>>>
>>> Do we really need a spin_lock for the dwc3-omap driver?
>>> Currently we won't be doing anything other than just
>>> clearing the irqstatus and re-enabling the interrupts.
>> 
>> well, if there's no possibility of races, then no. But only testing will
>> say for sure, I guess. I didn't really go through the entire thing just
>> to a write a quick little template :-p
>> 
> OK. Another hurdle I have is that how do I mask/unmask the interrupts?
> I do not see any masking bits, only enable/disable bits.
>
> I don't think we can use the enable/disable bits to mask as we'd loose
> events while the event is disabled.

I'm pretty sure your TRM discusses the usage of IRQSTATUS_RAW*
registers, doesn't it ? :-)

Those registers should be modified by HW even when interrupts are
disabled/masked. Note that, the IRQSTATUS_SET* and IRQSTATUS_CLR*
registers act more like mask/unmask than enable/disable considering we
can still read IRQ status using *RAW* registers.

See if below works fine for OMAP5, AM4 and AM5 SoCs:

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index af264493bbae..ece2f25ad2c3 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -165,20 +165,20 @@ static void dwc3_omap_write_utmi_ctrl(struct dwc3_omap 
*omap, u32 value)
 
 static u32 dwc3_omap_read_irq0_status(struct dwc3_omap *omap)
 {
-   return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_0 -
+   return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_RAW_0 -
omap->irq0_offset);
 }
 
 static void dwc3_omap_write_irq0_status(struct dwc3_omap *omap, u32 value)
 {
-   dwc3_omap_writel(omap->base, USBOTGSS_IRQSTATUS_0 -
+   dwc3_omap_writel(omap->base, USBOTGSS_IRQSTATUS_RAW_0 -
omap->irq0_offset, value);
 
 }
 
 static u32 dwc3_omap_read_irqmisc_status(struct dwc3_omap *omap)
 {
-   return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_MISC +
+   return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_RAW_MISC +
omap->irqmisc_offset);
 }
 
-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] usb: gadget: f_fs: report error if excess data received

[Felipe Balbi]

Hi,

"Du, Changbin"  writes:
>> > If it happen, we can keep the excess data for next i/o, or
>> > report an error. But we cannot silently drop data, because
>> > USB layer should ensure the data integrality it has transferred,
>> > otherwise applications may get corrupt data if it doesn't
>> > detect this case.
>> 
>> and when has this actually happened ? Host should not send more data in
>> this case, if it does, it's an error on the host side. Also, returning
>> -EOVERFLOW is not exactly correct here, because you'd violate POSIX
>> specification of read(), right ?
>> 
> This can happen if the host side app force kill-restart, not taking care of 
> this
> special condition(and we are not documented), or even it is a bug. Usually 
> APPs
> may has  a protocol to control the packet size, but protocol mismatch can 
> happen
> if either side encounter an error.
>
> Anyway, this is real. If kernel return success and drop data, the error may 
> explosion later, or its totally hided (but why some data lost in kernel?
> Kernel cannot tell userspace we cannot be trusted sometimes, right?). 
> so IMO, if this is an error, we need report an error or fix it, not hide it.
>
> The POSIX didn't say read cannot return "-EOVERFLOW", it says:
> " Other errors may occur, depending on the object connected to fd."
>
> If "-EOVERFLOW" is not suitable, EFAULT, or any suggestions?
>
>> > Here, we simply report an error to userspace to let userspace
>> > proccess. Actually, userspace applications should negotiate
>> 
>> no, this violates POSIX. Care to explain what problem are you actually
>> facing ?
>> 
> Why this violates POSIX? Could you give more details?

read(5) should return at mode 5 bytes. If there are more, than 5 bytes,
we don't error out, we just return the requested 5 bytes and wait for a
further read.

What I'm more concerned, however, is why we received more than expected
data. What's on the extra bytes ? Can you capture dwc3 traces ? Perhaps
add a few traces doing a hexdump (using __print_hex()) of the data in
req->buf.

> The problem is device side app sometimes received incorrect data caused
> by the dropping. Most times the error can be detected by APP itself, but

why ? app did e.g. read(5), that caused driver to queue a usb_request
with length set to 512. Host sent more data than the expected 5 bytes,
why did host do that ? And if that data was needed, why didn't userspace
read() more than 5 ?

-- 
balbi
--
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: f_fs: report error if excess data received

[Felipe Balbi]

Hi,

"Du, Changbin"  writes:
> Hi,
>
>> >> and when has this actually happened ? Host should not send more data in
>> >> this case, if it does, it's an error on the host side. Also, returning
>> >> -EOVERFLOW is not exactly correct here, because you'd violate POSIX
>> >> specification of read(), right ?
>> >>
>> > This can happen if the host side app force kill-restart, not taking care 
>> > of this
>> > special condition(and we are not documented), or even it is a bug. Usually
>> APPs
>> > may has  a protocol to control the packet size, but protocol mismatch can
>> happen
>> > if either side encounter an error.
>> >
>> > Anyway, this is real. If kernel return success and drop data, the error may
>> > explosion later, or its totally hided (but why some data lost in kernel?
>> > Kernel cannot tell userspace we cannot be trusted sometimes, right?).
>> > so IMO, if this is an error, we need report an error or fix it, not hide 
>> > it.
>> >
>> > The POSIX didn't say read cannot return "-EOVERFLOW", it says:
>> > " Other errors may occur, depending on the object connected to fd."
>> >
>> > If "-EOVERFLOW" is not suitable, EFAULT, or any suggestions?
>> >
>> >> > Here, we simply report an error to userspace to let userspace
>> >> > proccess. Actually, userspace applications should negotiate
>> >>
>> >> no, this violates POSIX. Care to explain what problem are you actually
>> >> facing ?
>> >>
>> > Why this violates POSIX? Could you give more details?
>> 
>> read(5) should return at mode 5 bytes. If there are more, than 5 bytes,
>> we don't error out, we just return the requested 5 bytes and wait for a
>> further read.
>> 
> Yes, it is true. As I mentioned before, we also can keep the extra data for
> next read. This need more work to maintain a buffer. Here I just simply 
> report an error(let userspace know something goes wrong.) before the
> logic is implemented by someone.

no, this is not how we do things here. If we find a bug we actually fix
it, we don't just work around it ;-)

> (Maybe ioctl approach may be more appropriate for usb transfer, like usbfs.)

heh, no :-)

>> What I'm more concerned, however, is why we received more than
>> expected
>> data. What's on the extra bytes ? Can you capture dwc3 traces ? Perhaps
>> add a few traces doing a hexdump (using __print_hex()) of the data in
>> req->buf.
>> 
> The extra bytes can be anything(random), they just data from APP layer.
> It doesn't make sense for you to check. So I will not dump them, sorry.

interesting, so you claim to have found a bug, but when asked to provide
more information your answer is "no" ? Thanks :-)

>> > The problem is device side app sometimes received incorrect data caused
>> > by the dropping. Most times the error can be detected by APP itself, but
>> 
>> why ? app did e.g. read(5), that caused driver to queue a usb_request
>> with length set to 512. Host sent more data than the expected 5 bytes,
>> why did host do that ? And if that data was needed, why didn't userspace
>> read() more than 5 ?
>> 
>
> Well, first, there must be a protocol upon usb between host side and
> device side.

sorry, I don't know what mean here. USB does not *require* a protocol
running on top of USB. There usually is one, but that's not a
requirement.

> Second device side didn't know how many bytes to receive, it need host
> side tell it.

well, many protocols work like this. See Mass Storage, for example.

> But host could be buggy,

if host is buggy, why should we fix host on the peripheral side ?

> or the application is killed and restart.

If application is killed (why was the application killed? Which
application was killed?), then why are we still connected to host at
all? It's clear that this gadget can't work without its userspace
counterpart. If that userspace isn't available, we should drop data
pullup and disconnect from host.

> These all can lead host send more than device wanted bytes. For sure
> it wrong at host side, but device side don't know.

but none of this means we have a bug at device side. In fact, by
allowing these extra bytes to reach userspace, we could be creating a
possible attack vector.

Your explanation is unsatisfactory, so I won't apply your patch, sorry.

-- 
balbi
--
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: f_fs: report error if excess data received

[Felipe Balbi]

Hi,

"Du, Changbin"  writes:
>> >> > The problem is device side app sometimes received incorrect data
>> caused
>> >> > by the dropping. Most times the error can be detected by APP itself, but
>> >>
>> >> why ? app did e.g. read(5), that caused driver to queue a usb_request
>> >> with length set to 512. Host sent more data than the expected 5 bytes,
>> >> why did host do that ? And if that data was needed, why didn't userspace
>> >> read() more than 5 ?
>> >>
>> >
>> > Well, first, there must be a protocol upon usb between host side and
>> > device side.
>> 
>> sorry, I don't know what mean here. USB does not *require* a protocol
>> running on top of USB. There usually is one, but that's not a
>> requirement.
>> 
> Communication between two endpoints must has a protocol, even it may
> very simple. Without protocol, they cannot exchange information.

that protocol is USB :-)

>> > Second device side didn't know how many bytes to receive, it need host
>> > side tell it.
>> 
>> well, many protocols work like this. See Mass Storage, for example.
>> 
>> > But host could be buggy,
>> 
>> if host is buggy, why should we fix host on the peripheral side ?
>> 
> True it is bug of host, but is it a reason kernel can drop data then? 

sure is :-) For all we know, that data means nothing to us

>> > or the application is killed and restart.
>> 
>> If application is killed (why was the application killed? Which
>> application was killed?), then why are we still connected to host at
>> all? It's clear that this gadget can't work without its userspace
>> counterpart. If that userspace isn't available, we should drop data
>> pullup and disconnect from host.
>> 
> Usb no need disconnect if the application exit (host side). Seems you
> only care about device side.

oh, application was killed on host side. I thought it was on device
side. Yeah, then we shouldn't disconnect.

>> > These all can lead host send more than device wanted bytes. For sure
>> > it wrong at host side, but device side don't know.
>> 
>> but none of this means we have a bug at device side. In fact, by
>> allowing these extra bytes to reach userspace, we could be creating a
>> possible attack vector.
>> 
>> Your explanation is unsatisfactory, so I won't apply your patch, sorry.
>> 
>> --
>> balbi
> It is fine. Then need userspace take care of all the data it received. Because
> Kernel may drop some data for it. Kernel ffs driver is unauthentic sometimes.

I really cannot understand what you mean sometimes. You're saying that
userspace needs to take care of all the data it received because kernel
can drop data. If kernel is dropping data, there's no extra data
reaching userspace, right?

Is the problem that we *are* giving more data than expected to
userspace? Are we overflowing some userspace buffer? If that's the case,
then below should be enough for the time being:

diff --git a/drivers/usb/gadget/function/f_fs.c 
b/drivers/usb/gadget/function/f_fs.c
index 73515d54e1cc..d1bd53c895ca 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -156,6 +156,8 @@ struct ffs_io_data {
struct usb_request *req;
 
struct ffs_data *ffs;
+
+   ssize_t expected_len;
 };
 
 struct ffs_desc_helper {
@@ -730,8 +732,10 @@ static ssize_t ffs_epfile_io(struct file *file, struct 
ffs_io_data *io_data)
 * Controller may require buffer size to be aligned to
 * maxpacketsize of an out endpoint.
 */
-   if (io_data->read)
+   if (io_data->read) {
+   io_data->expected_len = data_len;
data_len = usb_ep_align_maybe(gadget, ep->ep, data_len);
+   }
spin_unlock_irq(&epfile->ffs->eps_lock);
 
data = kmalloc(data_len, GFP_KERNEL);
@@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct 
ffs_io_data *io_data)
 */
ret = interrupted ? -EINTR : ep->status;
if (io_data->read && ret > 0) {
-   ret = copy_to_iter(data, ret, &io_data->data);
+   if (ret > io_data->expected_len)
+   pr_debug("FFS: size mismatch: %zd for %zd",
+   ret, io_data->expected_len);
+
+   ret = copy_to_iter(data, io_data->expected_len,
+   &io_data->data);
if (!ret)
ret = -EFAULT;
}

that we can get merged during v4.7-rc and Cc stable and backport this to
anything containing Al's commit c993c39b8639 ("gadget/function/f_fs.c:
use put iov_iter into io_data").

-- 
balbi
--
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: f_fs: report error if excess data received

[Felipe Balbi]

Hi again,

Felipe Balbi  writes:
> @@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct 
> ffs_io_data *io_data)
>*/
>   ret = interrupted ? -EINTR : ep->status;
>   if (io_data->read && ret > 0) {
> - ret = copy_to_iter(data, ret, &io_data->data);
> + if (ret > io_data->expected_len)
> + pr_debug("FFS: size mismatch: %zd for %zd",
> + ret, io_data->expected_len);
> +
> + ret = copy_to_iter(data, io_data->expected_len,
> + &io_data->data);

we need a min() here. Better version below:

diff --git a/drivers/usb/gadget/function/f_fs.c 
b/drivers/usb/gadget/function/f_fs.c
index 73515d54e1cc..6c49b152f46e 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -156,6 +156,8 @@ struct ffs_io_data {
struct usb_request *req;
 
struct ffs_data *ffs;
+
+   ssize_t expected_len;
 };
 
 struct ffs_desc_helper {
@@ -730,8 +732,10 @@ static ssize_t ffs_epfile_io(struct file *file, struct 
ffs_io_data *io_data)
 * Controller may require buffer size to be aligned to
 * maxpacketsize of an out endpoint.
 */
-   if (io_data->read)
+   if (io_data->read) {
+   io_data->expected_len = data_len;
data_len = usb_ep_align_maybe(gadget, ep->ep, data_len);
+   }
spin_unlock_irq(&epfile->ffs->eps_lock);
 
data = kmalloc(data_len, GFP_KERNEL);
@@ -811,7 +815,15 @@ static ssize_t ffs_epfile_io(struct file *file, struct 
ffs_io_data *io_data)
 */
ret = interrupted ? -EINTR : ep->status;
if (io_data->read && ret > 0) {
-   ret = copy_to_iter(data, ret, &io_data->data);
+   ssize_t bytes;
+
+   if (ret > io_data->expected_len)
+   pr_debug("FFS: size mismatch: %zd for %zd",
+   ret, io_data->expected_len);
+
+   bytes = min(ret, io_data->expected_len);
+
+   ret = copy_to_iter(data, bytes, &io_data->data);
if (!ret)
ret = -EFAULT;
}


-- 
balbi
--
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: f_fs: report error if excess data received

[Felipe Balbi]

Hi,

"Du, Changbin"  writes:
>> >> > These all can lead host send more than device wanted bytes. For sure
>> >> > it wrong at host side, but device side don't know.
>> >>
>> >> but none of this means we have a bug at device side. In fact, by
>> >> allowing these extra bytes to reach userspace, we could be creating a
>> >> possible attack vector.
>> >>
>> >> Your explanation is unsatisfactory, so I won't apply your patch, sorry.
>> >>
>> >> --
>> >> balbi
>> > It is fine. Then need userspace take care of all the data it received. 
>> > Because
>> > Kernel may drop some data for it. Kernel ffs driver is unauthentic
>> sometimes.
>> 
>> I really cannot understand what you mean sometimes. You're saying that
>> userspace needs to take care of all the data it received because kernel
>> can drop data. If kernel is dropping data, there's no extra data
>> reaching userspace, right?
>> 
> For sure, maybe I didn't describe it well so let you confused. :)

okay

>> Is the problem that we *are* giving more data than expected to
>> userspace? Are we overflowing some userspace buffer? If that's the case,
>> then below should be enough for the time being:
>> 
> No, the problem is we drop data but silently. We cannot give more data to

okay, but does that create any problems for device side userspace? What
problem is that?

> userspace since buffer is limited.

right, and that was my point: if we copy more to userspace, then we have
a real big problem.

>> @@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct
>> ffs_io_data *io_data)
>>   */
>>  ret = interrupted ? -EINTR : ep->status;
>>  if (io_data->read && ret > 0) {
>> -ret = copy_to_iter(data, ret, &io_data->data);
>> +if (ret > io_data->expected_len)
>> +pr_debug("FFS: size mismatch: %zd for %zd",
>> +ret, io_data->expected_len);
>> +
>> +ret = copy_to_iter(data, io_data->expected_len,
>> +&io_data->data);
>>  if (!ret)
>>  ret = -EFAULT;
>>  }
>> 
>> that we can get merged during v4.7-rc and Cc stable and backport this to
>> anything containing Al's commit c993c39b8639 ("gadget/function/f_fs.c:
>> use put iov_iter into io_data").
>> 
>
> The different for this code is just give warning but not return
> error. It is also fine for me that at least this let development can
> find some key message to find What happed under kernel. But the
> message should be *error* I think.

I'm fine with pr_error()

> And this missed AIO path. This is identify to my patch after remove the

right, it's more of a debug patch since I don't have the setup to
trigger this (I'm assuming you're using adb?)

> "return -EOVERFLOW;" line.

there's one key difference, see below

> Byw, we not need add the field "expected_len", we can get it from the
> struct ffs_io_data.

without expected_len we can copy more data to userspace, right ? If
req->actual > data_len_before_aligning_to_maxpacket, then we will copy
more data then we should to userspace and this was a regression caused
by Al's commit, AFAICT.

> If this is fine for you, I can publish a new patch.
>
>> --
>> Balbi
>
> Best Regards,
> Du, Changbin

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] usb: gadget: f_fs: report error if excess data received

[Felipe Balbi]

Hi,

"Du, Changbin"  writes:
>> >> > These all can lead host send more than device wanted bytes. For sure
>> >> > it wrong at host side, but device side don't know.
>> >>
>> >> but none of this means we have a bug at device side. In fact, by
>> >> allowing these extra bytes to reach userspace, we could be creating a
>> >> possible attack vector.
>> >>
>> >> Your explanation is unsatisfactory, so I won't apply your patch, sorry.
>> >>
>> >> --
>> >> balbi
>> > It is fine. Then need userspace take care of all the data it received. 
>> > Because
>> > Kernel may drop some data for it. Kernel ffs driver is unauthentic
>> sometimes.
>> 
>> I really cannot understand what you mean sometimes. You're saying that
>> userspace needs to take care of all the data it received because kernel
>> can drop data. If kernel is dropping data, there's no extra data
>> reaching userspace, right?
>> 
> For sure, maybe I didn't describe it well so let you confused. :)

okay

>> Is the problem that we *are* giving more data than expected to
>> userspace? Are we overflowing some userspace buffer? If that's the case,
>> then below should be enough for the time being:
>> 
> No, the problem is we drop data but silently. We cannot give more data to

okay, but does that create any problems for device side userspace? What
problem is that?

> userspace since buffer is limited.

right, and that was my point: if we copy more to userspace, then we have
a real big problem.

>> @@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct
>> ffs_io_data *io_data)
>>   */
>>  ret = interrupted ? -EINTR : ep->status;
>>  if (io_data->read && ret > 0) {
>> -ret = copy_to_iter(data, ret, &io_data->data);
>> +if (ret > io_data->expected_len)
>> +pr_debug("FFS: size mismatch: %zd for %zd",
>> +ret, io_data->expected_len);
>> +
>> +ret = copy_to_iter(data, io_data->expected_len,
>> +&io_data->data);
>>  if (!ret)
>>  ret = -EFAULT;
>>  }
>> 
>> that we can get merged during v4.7-rc and Cc stable and backport this to
>> anything containing Al's commit c993c39b8639 ("gadget/function/f_fs.c:
>> use put iov_iter into io_data").
>> 
>
> The different for this code is just give warning but not return
> error. It is also fine for me that at least this let development can
> find some key message to find What happed under kernel. But the
> message should be *error* I think.

I'm fine with pr_error()

> And this missed AIO path. This is identify to my patch after remove the

right, it's more of a debug patch since I don't have the setup to
trigger this (I'm assuming you're using adb?)

> "return -EOVERFLOW;" line.

there's one key difference, see below

> Byw, we not need add the field "expected_len", we can get it from the
> struct ffs_io_data.

without expected_len we can copy more data to userspace, right ? If
req->actual > data_len_before_aligning_to_maxpacket, then we will copy
more data then we should to userspace and this was a regression caused
by Al's commit, AFAICT.

> If this is fine for you, I can publish a new patch.
>
>> --
>> Balbi
>
> Best Regards,
> Du, Changbin

-- 
balbi
--
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: f_fs: report error if excess data received

[Felipe Balbi]

Hi,

"Du, Changbin"  writes:
>> right, and that was my point: if we copy more to userspace, then we have
>> a real big problem.
>> 
> Yes, we drop the data because we userspace buffer is not enough this time.
> The problem here is that really can we just drop it silently? Maybe not.

Yeah, it probably deserves a pr_err() or pr_debug(), but host sending
more data than it should, is another problem altogether which needs to
be addressed at the host.

Adding a print to aid debugging is a good idea, but bailing out on the
peripheral side is not :-s

>> > And this missed AIO path. This is identify to my patch after remove the
>> 
>> right, it's more of a debug patch since I don't have the setup to
>> trigger this (I'm assuming you're using adb?)
>> 
> Right. And adb can detect this unexpected behavior(data mismatch) quickly
> because it has some selfcheck for the data content.

cool

>> > Byw, we not need add the field "expected_len", we can get it from the
>> > struct ffs_io_data.
>> 
>> without expected_len we can copy more data to userspace, right ? If
>> req->actual > data_len_before_aligning_to_maxpacket, then we will copy
>> more data then we should to userspace and this was a regression caused
>> by Al's commit, AFAICT.
>> 
> No, expected_len equals to iov_iter_count(&io_data->data), right? So we
> do not need a new field.

/me goes read iov_iter_count()

you're right, we don't need expected len at all ;-)

in any case, did you figure out if the extra data host sends is
important data at all or just garbage ?

-- 
balbi
--
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: fix regression on big-endian PowerPC/ARM systems

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> A patch that went into Linux-4.4 to fix big-endian mode on a Lantiq
> MIPS system unfortunately broke big-endian operation on PowerPC
> APM82181 as reported by Christian Lamparter, and likely other
> systems.
>
> It actually introduced multiple issues:
>
> - it broke big-endian ARM kernels: any machine that was working
>   correctly with a little-endian kernel is no longer using byteswaps
>   on big-endian kernels, which clearly breaks them.
> - On PowerPC the same thing must be true: if it was working before,
>   using big-endian kernels is now broken. Unlike ARM, 32-bit PowerPC
>   usually uses big-endian kernels, so they are likely all broken.
> - The barrier for dwc2_writel is on the wrong side of the __raw_writel(),
>   so the MMIO no longer synchronizes with DMA operations.
> - On architectures that require specific CPU instructions for MMIO
>   access, using the __raw_ variant may turn this into a pointer
>   dereference that does not have the same effect as the readl/writel.
>
> This patch is a simple revert for all architectures other than MIPS,
> in the hope that we can more easily backport it to fix the regression
> on PowerPC and ARM systems without breaking the Lantiq system again.
>
> We should follow this up with a more elaborate change to add runtime
> detection of endianess, to make sure it also works on all other
> combinations of architectures and implementations of the usb-dwc2
> device. That patch however will be fairly large and not appropriate
> for backports to stable kernels.
>
> Signed-off-by: Arnd Bergmann 
> Fixes: 95c8bc360944 ("usb: dwc2: Use platform endianness when accessing 
> registers")
> ---
>  drivers/usb/dwc2/core.h | 41 +++--
>  1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 3c58d633ce80..1f8ed149a40f 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -64,12 +64,24 @@
>   DWC2_TRACE_SCHEDULER_VB(pr_fmt("%s: SCH: " fmt),\
>   dev_name(hsotg->dev), ##__VA_ARGS__)
>  
> +
> +#ifdef CONFIG_MIPS
> +/*
> + * There are some MIPS machines that can run in either big-endian
> + * or little-endian mode and that use the dwc2 register without
> + * a byteswap in both ways.
> + * Unlike other architectures, MIPS does not require a barrier
> + * before the __raw_writel() to synchronize with DMA but does
> + * require the barrier after the writel() to serialize a series
> + * of writes. This set of operations was added specifically for
> + * MIPS and should only be used there.
> + */
>  static inline u32 dwc2_readl(const void __iomem *addr)
>  {
>   u32 value = __raw_readl(addr);
>  
> - /* In order to preserve endianness __raw_* operation is used. Therefore
> -  * a barrier is needed to ensure IO access is not re-ordered across
> + /* in order to preserve endianness __raw_* operation is used. therefore
> +  * a barrier is needed to ensure io access is not re-ordered across
>* reads or writes
>*/
>   mb();
> @@ -81,15 +93,32 @@ static inline void dwc2_writel(u32 value, void __iomem 
> *addr)
>   __raw_writel(value, addr);
>  
>   /*
> -  * In order to preserve endianness __raw_* operation is used. Therefore
> -  * a barrier is needed to ensure IO access is not re-ordered across
> +  * in order to preserve endianness __raw_* operation is used. therefore
> +  * a barrier is needed to ensure io access is not re-ordered across
>* reads or writes
>*/
>   mb();
> -#ifdef DWC2_LOG_WRITES
> - pr_info("INFO:: wrote %08x to %p\n", value, addr);
> +#ifdef dwc2_log_writes
> + pr_info("info:: wrote %08x to %p\n", value, addr);
>  #endif
>  }
> +#else

I still think this is something that should be handled at MIPS side, no ?

How many more drivers will we have to 'fix' like this ?

-- 
balbi
--
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: fix regression on big-endian PowerPC/ARM systems

[Felipe Balbi]

Hi,

(Arnd, you didn't Cc dwc2's maintainer. I'm also not part of TI anymore)

Arnd Bergmann  writes:
> On Thursday 12 May 2016 14:25:49 Felipe Balbi wrote:
>> >  {
>> >   u32 value = __raw_readl(addr);
>> >  
>> > - /* In order to preserve endianness __raw_* operation is used. 
>> > Therefore
>> > -  * a barrier is needed to ensure IO access is not re-ordered across
>> > + /* in order to preserve endianness __raw_* operation is used. 
>> > therefore
>> > +  * a barrier is needed to ensure io access is not re-ordered across
>> >* reads or writes
>> >*/
>> >   mb();
>> > @@ -81,15 +93,32 @@ static inline void dwc2_writel(u32 value, void __iomem 
>> > *addr)
>> >   __raw_writel(value, addr);
>> >  
>> >   /*
>> > -  * In order to preserve endianness __raw_* operation is used. 
>> > Therefore
>> > -  * a barrier is needed to ensure IO access is not re-ordered across
>> > +  * in order to preserve endianness __raw_* operation is used. 
>> > therefore
>> > +  * a barrier is needed to ensure io access is not re-ordered across
>> >* reads or writes
>> >*/
>> >   mb();
>> > -#ifdef DWC2_LOG_WRITES
>> > - pr_info("INFO:: wrote %08x to %p\n", value, addr);
>> > +#ifdef dwc2_log_writes
>> > + pr_info("info:: wrote %08x to %p\n", value, addr);
>> >  #endif
>> >  }
>> > +#else
>
> Oops, the accidental lowercase conversion is still in here, I'll fix it
> up once we agree on the approach.
>
>> I still think this is something that should be handled at MIPS side, no ?
>
> As I explained, there isn't really anything we can do in MIPS code
> because of the way they have to handle PCI.
>
>> How many more drivers will we have to 'fix' like this ?
>
> Endianess problems will keep coming up, and we have hundreds or thousands
> of drivers that are written with a particular design in mind that could
> be wrong as soon as someone chooses to build an SoC that does things
> differently. Once that happens, we'll fix them.
>
> Also, Christian has already posted a better version of the patch
> that fixes this driver in an architecture independent way, but we still
> need a workaround for the stable backports.

hmmm, at least dwc3 (also from SNPS) has a couple bits where we can
choose endianess for registers and DMA descriptors. John, do we have the
same for dwc2 ? Wouldn't that be a better way to solve the problem ?

-- 
balbi
--
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: f_fs: report error if excess data received

[Felipe Balbi]

Hi,

"Du, Changbin"  writes:
>> "Du, Changbin"  writes:
>> >> right, and that was my point: if we copy more to userspace, then we have
>> >> a real big problem.
>> >>
>> > Yes, we drop the data because we userspace buffer is not enough this time.
>> > The problem here is that really can we just drop it silently? Maybe not.
>> 
>> Yeah, it probably deserves a pr_err() or pr_debug(), but host sending
>> more data than it should, is another problem altogether which needs to
>> be addressed at the host.
>> 
>> Adding a print to aid debugging is a good idea, but bailing out on the
>> peripheral side is not :-s
>> 
> Ok, if we think this is a problem at host side that the transfer is not device
> expected, then device side should not accept the data or deliver the
> transferred data to userspace. But now we take part of the data to userspace
> and says it is ok.
> Do you agree with this point?

We deliver to userspace the part userspace requested, right? So that's
okay. The USB details WRT e.g. babble or host trying to send more data
than expected, needs to be handled within the kernel.

> IMO, we expose usb transfer as a file on device side. But file read() doesn't
> have a requirement that "sorry, you cannot read so little! you need read all
> once, else we may drop data for you. :) ".

but that's not how read() semantics work. When userspace asks to read(x)
bytes, we have three possible outcomes:

i. We have x bytes to return, so we copy_to_user(x)

ii. We have y < x bytes to return, so we copy_to_user(y)

iii. We have y > x bytes to return, so we copy_to_user(x)

This is exactly how the kernel is behaving. The only "detail" we have is
that, for some reason, host is sending too much data. what I still don't
know is if this extra data is garbage or something userspace genuinely
cares about. Do you know the answer to this?

> And some library that may retry read() until get enough data (which is
> normal For a general read). Then sometimes the buffer size for
> sys_read may not as expected. This is why I think ioctl approach is
> more appropriate for usb transfer.

no, this won't change anything. Besides, it's a pointless discussion as
cannot break userspace ABI. GadgetFS and FunctionFS have been shipping
in kernel for many years.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2 0/5] support rockchip dwc3 driver

[Felipe Balbi]

Hi,

William Wu  writes:
> This series add support for rockchip dwc3 driver,
> and add additional optional properties for specific
> platforms (e.g., rockchip platform).
>
> William Wu (5):
>   usb: dwc3: of-simple: add compatible for rockchip
>   usb: dwc3: add dis_u2_freeclk_exists_quirk
>   usb: dwc3: add phyif_utmi_quirk
>   usb: dwc3: add dis_del_phy_power_chg_quirk
>   usb: dwc3: rockchip: add devicetree bindings documentation
>
>  Documentation/devicetree/bindings/usb/dwc3.txt |  9 +
>  .../devicetree/bindings/usb/rockchip,dwc3.txt  | 45 
> ++
>  drivers/usb/dwc3/core.c| 39 ++-
>  drivers/usb/dwc3/core.h| 20 ++
>  drivers/usb/dwc3/dwc3-of-simple.c  |  1 +
>  drivers/usb/dwc3/platform_data.h   |  4 ++
>  6 files changed, 117 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/rockchip,dwc3.txt

I didn't get patch 5/5 :-s

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on

[Felipe Balbi]

 0;
>  }
>  
> +void dwc3_gadget_connect(struct dwc3 *dwc)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dwc->lock, flags);
> + dwc->cable_connected = true;
> + spin_unlock_irqrestore(&dwc->lock, flags);
> +}
> +
> +void dwc3_gadget_disconnect(struct dwc3 *dwc)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dwc->lock, flags);
> + dwc->cable_connected = false;
> + spin_unlock_irqrestore(&dwc->lock, flags);
> +}
> +

nope

> +static bool dwc3_gadget_is_connected(struct dwc3 *dwc)
> +{
> + /*
> +  * If the gadget is always power on, then no need to check if the
> +  * cable is plugin or not.
> +  */
> + if (!dwc->can_save_power)
> + return true;

this is wrong! Really, really wrong! If cable is detached, you're saying
it's always attached. No. Don't do it. Don't lie to the driver.

> +static int __dwc3_gadget_start(struct dwc3 *dwc);
> +
>  static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>  {
>   u32 reg;
>   u32 timeout = 500;
> + int ret;
> +
> + if (!dwc3_gadget_is_connected(dwc) || !dwc->gadget_driver)
> + return 0;
>  
>   reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>   if (is_on) {
> + if (dwc->need_restart) {
> + /*
> +  * We need to reset the device firstly when the device
> +  * is power on.
> +  */
> + ret = dwc3_soft_reset(dwc);
> + if (ret)
> + return ret;
> +
> + /*
> +  * After resetting the device, it need to re-setup the
> +  * event buffer.
> +  */
> + ret = dwc3_event_buffers_setup(dwc);
> + if (ret) {
> + dev_err(dwc->dev,
> + "failed to setup event buffers\n");
> + return ret;
> + }
> +
> + /* Start the gadget */
> + ret = __dwc3_gadget_start(dwc);
> + if (ret)
> + return ret;
> + }

no way

> +static int dwc3_gadget_start(struct usb_gadget *g,
> + struct usb_gadget_driver *driver)
> +{
> + struct dwc3 *dwc = gadget_to_dwc(g);
> + unsigned long   flags;
> + int ret = 0;
> + int irq;
> +
> + irq = platform_get_irq(to_platform_device(dwc->dev), 0);
> + ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
> + IRQF_SHARED, "dwc3", dwc);
> + if (ret) {
> + dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
> + irq, ret);
> + goto err0;
> + }
> +
> + spin_lock_irqsave(&dwc->lock, flags);
> +
> + if (dwc->gadget_driver) {
> + dev_err(dwc->dev, "%s is already bound to %s\n",
> + dwc->gadget.name,
> + dwc->gadget_driver->driver.name);
> +     ret = -EBUSY;
> + goto err1;
> + }
> +
> + dwc->gadget_driver  = driver;
> +
> + /*
> +  * If the gadget can be power off when there is no cable plug in, we
> +  * need to check if the device power is on or not. If not, we should
> +  * not access the device registers.
> +  */
> + if (!dwc3_gadget_is_connected(dwc)) {
> + dwc->need_restart = 1;
> + spin_unlock_irqrestore(&dwc->lock, flags);
> + return 0;
> + }
> +
> + ret = __dwc3_gadget_start(dwc);
> + if (ret)
> + goto err2;
> +

this is similar to a patch I wrote to improve system suspend/resume (not
runtime). See:

https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=dwc3-fix-suspend&id=d1c2d86ef61b8f7ac793036aee9d501fa44d9b8a

https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=dwc3-fix-suspend&id=b6dc23f16389ee8803aba6a4c0265aac547f9c57

https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=dwc3-fix-suspend&id=7c38518f5ea748ee651d899cf7714dfb653f

I haven't sent because I didn't finish testing yet

Anyway, which platform are you dealing with? Why is dwc3 off while VBUS
is off? How do you handle host mode?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 6254b2f..dada5c6 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -734,6 +734,9 @@ struct dwc3_scratchpad_array {
>>>   *   1   - -3.5dB de-emphasis
>>>   *   2   - No de-emphasis
>>>   *   3   - Reserved
>>> + * @can_save_power: set if the gadget will power off when no cable plug in.
>>
>> nope
>>
>>> + * @need_restart: set if we need to restart the gadget.
>>
>> why does it need restart? Why is dwc3 powered off? Who powers it off?
>
> Because when the dwc3 Vbus is off (no cable pluging in now),
> especially for some mobile device, the system need to power off the
> dwc3 to save power in this situation.

but dwc3 doesn't do this by itself, so who's doing it?

>> This looks like a *really* bad power management implementation. Do you
>> have hibernation enabled? Do you have Clock gating enabled? Which dwc3
>> version are you using? How was it configured?
>
> This is not hibernation, we want to power off the dwc3 to save power
> when no cable plugging in. Yes, we have clock gating, at this
> situation we will disable the clock and shutdown the phy to save
> power. For mobile device, most time no cable plugging in, so we need
> to think about the power consuming. How do you think this requirement?

Well, seems like you're missing *proper* runtime PM. I've been meaning
to work on it for weeks, but I still have a few other things to do
before I get to that. In any case, we don't need to do what you did
here. There are better ways.

>> Anyway, which platform are you dealing with? Why is dwc3 off while VBUS
>> is off? How do you handle host mode?
>
> On Spreadtrum platform, for thinking about some mobile devices with

I meant the SoC ;-) It's their own SoC? Are we getting glue-layer
patches any time soon?

> strict power management. This is just for gadget mode, we don't power
> off the dwc3 when it is host mode. Thanks.

okay, thanks

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>>>> why does it need restart? Why is dwc3 powered off? Who powers it off?
>>>
>>> Because when the dwc3 Vbus is off (no cable pluging in now),
>>> especially for some mobile device, the system need to power off the
>>> dwc3 to save power in this situation.
>>
>> but dwc3 doesn't do this by itself, so who's doing it?
>
> Yes, the dwc3 clock is controlled by the Soc system, so the Soc system
> can disable the dwc3 clock when there is no cable plugging in.

understood.

>>>> This looks like a *really* bad power management implementation. Do you
>>>> have hibernation enabled? Do you have Clock gating enabled? Which dwc3
>>>> version are you using? How was it configured?
>>>
>>> This is not hibernation, we want to power off the dwc3 to save power
>>> when no cable plugging in. Yes, we have clock gating, at this
>>> situation we will disable the clock and shutdown the phy to save
>>> power. For mobile device, most time no cable plugging in, so we need
>>> to think about the power consuming. How do you think this requirement?
>>
>> Well, seems like you're missing *proper* runtime PM. I've been meaning
>> to work on it for weeks, but I still have a few other things to do
>> before I get to that. In any case, we don't need to do what you did
>> here. There are better ways.
>
> Make sense.

cool, if you wanna work on it, let me know and I can give some details
of what I have in mind.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] usb: gadget: f_fs: report error if excess data received

[Felipe Balbi]

Hi,

Alan Stern  writes:
> On Fri, 13 May 2016, Felipe Balbi wrote:
>
>> We deliver to userspace the part userspace requested, right? So that's
>> okay. The USB details WRT e.g. babble or host trying to send more data
>> than expected, needs to be handled within the kernel.
>
> The point is that you don't know whether the host sent more data than
> expected.  All you know is that the host sent more data than the user
> asked the kernel for -- but maybe the user didn't ask for all the data
> that he expected.  Maybe the user wanted to retrieve the full set of
> data using two read() system calls.

right, but that just means we need to buffer the data instead of bailing
out of the first read() completely.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: f_fs: report error if excess data received

[Felipe Balbi]

Michal Nazarewicz  writes:

>> Alan Stern  writes:
>>> The point is that you don't know whether the host sent more data than
>>> expected.  All you know is that the host sent more data than the user
>>> asked the kernel for -- but maybe the user didn't ask for all the
>>> data that he expected.  Maybe the user wanted to retrieve the full
>>> set of data using two read() system calls.
>
> On Mon, May 16 2016, Felipe Balbi wrote:
>> right, but that just means we need to buffer the data instead of bailing
>> out of the first read() completely.
>
> Correct.
>
> I have a ~4h bus ride ahead of me so I’ll try to implement it.  If you
> don’t hear from me by the end of the day, there probably wasn’t enough
> space/comfort in the bus to use a laptop.

Cool, Michal. Thanks

seems like a kfifo would do well here(?)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on

[Felipe Balbi]

Hi

Baolin Wang  writes:
> Hi Felipe,
>
> On 13 May 2016 at 20:46, Felipe Balbi  wrote:
>>
>> Hi,
>>
>> Baolin Wang  writes:
>>>>>> why does it need restart? Why is dwc3 powered off? Who powers it off?
>>>>>
>>>>> Because when the dwc3 Vbus is off (no cable pluging in now),
>>>>> especially for some mobile device, the system need to power off the
>>>>> dwc3 to save power in this situation.
>>>>
>>>> but dwc3 doesn't do this by itself, so who's doing it?
>>>
>>> Yes, the dwc3 clock is controlled by the Soc system, so the Soc system
>>> can disable the dwc3 clock when there is no cable plugging in.
>>
>> understood.
>>
>>>>>> This looks like a *really* bad power management implementation. Do you
>>>>>> have hibernation enabled? Do you have Clock gating enabled? Which dwc3
>>>>>> version are you using? How was it configured?
>>>>>
>>>>> This is not hibernation, we want to power off the dwc3 to save power
>>>>> when no cable plugging in. Yes, we have clock gating, at this
>>>>> situation we will disable the clock and shutdown the phy to save
>>>>> power. For mobile device, most time no cable plugging in, so we need
>>>>> to think about the power consuming. How do you think this requirement?
>>>>
>>>> Well, seems like you're missing *proper* runtime PM. I've been meaning
>>>> to work on it for weeks, but I still have a few other things to do
>>>> before I get to that. In any case, we don't need to do what you did
>>>> here. There are better ways.
>>>
>>> Make sense.
>>
>> cool, if you wanna work on it, let me know and I can give some details
>> of what I have in mind.
>
> Could you explain details to me, and I wanna continue to optimize the
> power management things. Thanks.

I have it half-way done. Have a look at my dwc3-fix-suspend branch on
k.org. I haven't sent because I'm not getting a PME event. Can you test
on your end and let me know what happens?

Note that if cable is disconnected, we will drop RUN/STOP bit. On
runtime_resume, we will restart the controller from scratch (skipping
memory allocations, of course)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>>>>> Make sense.
>>>>
>>>> cool, if you wanna work on it, let me know and I can give some details
>>>> of what I have in mind.
>>>
>>> Could you explain details to me, and I wanna continue to optimize the
>>> power management things. Thanks.
>>
>> I have it half-way done. Have a look at my dwc3-fix-suspend branch on
>> k.org. I haven't sent because I'm not getting a PME event. Can you test
>> on your end and let me know what happens?
>
> OK. I try to test on my platform to see what will happen.

great, thanks. You might need to patch your glue layer with proper
runtime_* callbacks, btw.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 00/13] usb: dwc2: Fix up gadget isochronous support

[Felipe Balbi]

Hi John,

John Youn  writes:
> The following patch series fixes up isochronous support for the dwc2
> gadget. The existing isochronous support lacked a few features. Most
> notably it did not properly sync up with the first packet and it
> didn't handle the Incomplete ISO IN/OUT interrupts.
>
> These patches have been sitting in our internal tree for a while and
> tested mostly on a HAPS-based FPGA IP validation platform.

I see we have fixes and non-fixes together in this series. If you want
the fixes here to be merged during v4.7-rc, would you mind splitting the
series as to avoid dependencies between fixes and cleanups?

Thanks

ps: if you don't mind waiting for v4.8 it's fine. I'll start queueing
again once v4.7-rc1 is tagged.

-- 
balbi


signature.asc
Description: PGP signature

[PATCH 15/22] usb: dwc3: gadget: switch over to spin_lock()

[Felipe Balbi]

we *know* that our dwc3 interrupt is masked because
we masked it in the hardirq handler. This means we
don't need to disable all of current cpu's
interrupts.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 441d23e0dbf7..9bc0e3080950 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2762,12 +2762,11 @@ static irqreturn_t dwc3_thread_interrupt(int irq, void 
*_evt)
 {
struct dwc3_event_buffer *evt = _evt;
struct dwc3 *dwc = evt->dwc;
-   unsigned long flags;
irqreturn_t ret = IRQ_NONE;
 
-   spin_lock_irqsave(&dwc->lock, flags);
+   spin_lock(&dwc->lock);
ret = dwc3_process_event_buf(evt);
-   spin_unlock_irqrestore(&dwc->lock, flags);
+   spin_unlock(&dwc->lock);
 
return ret;
 }
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 19/22] usb: dwc3: gadget: hold gadget IRQ in dwc->irq_gadget

[Felipe Balbi]

by holding gadget's IRQ number in dwc->irq_gadget,
it'll be simpler to free_irq() and disable the IRQ
in case an IRQ fires while we are runtime suspended.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/core.h   | 2 ++
 drivers/usb/dwc3/gadget.c | 5 ++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 58509cedb038..5f67c4bc1d37 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -729,6 +729,7 @@ struct dwc3_scratchpad_array {
  * @regs: base address for our registers
  * @regs_size: address space size
  * @fladj: frame length adjustment
+ * @irq_gadget: peripheral controller's IRQ number
  * @nr_scratch: number of scratch buffers
  * @u1u2: only used on revisions <1.83a for workaround
  * @maximum_speed: maximum speed requested (mainly for testing purposes)
@@ -836,6 +837,7 @@ struct dwc3 {
enum usb_dr_modedr_mode;
 
u32 fladj;
+   u32 irq_gadget;
u32 nr_scratch;
u32 u1u2;
u32 maximum_speed;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9bc0e3080950..2a53d318f581 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1725,6 +1725,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
irq, ret);
goto err0;
}
+   dwc->irq_gadget = irq;
 
spin_lock_irqsave(&dwc->lock, flags);
if (dwc->gadget_driver) {
@@ -1761,15 +1762,13 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
 {
struct dwc3 *dwc = gadget_to_dwc(g);
unsigned long   flags;
-   int irq;
 
spin_lock_irqsave(&dwc->lock, flags);
__dwc3_gadget_stop(dwc);
dwc->gadget_driver  = NULL;
spin_unlock_irqrestore(&dwc->lock, flags);
 
-   irq = platform_get_irq(to_platform_device(dwc->dev), 0);
-   free_irq(irq, dwc->ev_buf);
+   free_irq(dwc->irq_gadget, dwc->ev_buf);
 
return 0;
 }
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 11/22] usb: dwc3: gadget: clear LST from previous TRB on Update Transfer

[Felipe Balbi]

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

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 4a3ed4cbe39b..809445ec6a87 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -770,7 +770,8 @@ static void dwc3_gadget_ep_free_request(struct usb_ep *ep,
  */
 static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
struct dwc3_request *req, dma_addr_t dma,
-   unsigned length, unsigned last, unsigned chain, unsigned node)
+   unsigned length, unsigned last, unsigned chain,
+   unsigned node, int starting)
 {
struct dwc3_trb *trb;
 
@@ -779,6 +780,15 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
length, last ? " last" : "",
chain ? " chain" : "");
 
+   /*
+* When trying to issue Update Transfer, we can remove LST bit from
+* previous TRB and avoid a XferComplete
+*/
+   if (!starting) {
+   trb = &dep->trb_pool[dep->trb_enqueue - 1];
+   if (trb->ctrl & DWC3_TRB_CTRL_HWO)
+   trb->ctrl &= ~DWC3_TRB_CTRL_LST;
+   }
 
trb = &dep->trb_pool[dep->trb_enqueue];
 
@@ -871,7 +881,8 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
 }
 
 static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
-   struct dwc3_request *req, unsigned int trbs_left)
+   struct dwc3_request *req, unsigned int trbs_left,
+   int starting)
 {
struct usb_request *request = &req->request;
struct scatterlist *sg = request->sg;
@@ -901,7 +912,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
chain = false;
 
dwc3_prepare_one_trb(dep, req, dma, length,
-   last, chain, i);
+   last, chain, i, starting);
 
if (last)
break;
@@ -909,7 +920,8 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 }
 
 static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
-   struct dwc3_request *req, unsigned int trbs_left)
+   struct dwc3_request *req, unsigned int trbs_left,
+   int starting)
 {
unsigned intlast = false;
unsigned intlength;
@@ -926,7 +938,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
last = true;
 
dwc3_prepare_one_trb(dep, req, dma, length,
-   last, false, 0);
+   last, false, 0, starting);
 }
 
 /*
@@ -937,7 +949,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
  * transfers. The function returns once there are no more TRBs available or
  * it runs out of requests.
  */
-static void dwc3_prepare_trbs(struct dwc3_ep *dep)
+static void dwc3_prepare_trbs(struct dwc3_ep *dep, int starting)
 {
struct dwc3_request *req, *n;
u32 trbs_left;
@@ -948,9 +960,11 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
 
list_for_each_entry_safe(req, n, &dep->pending_list, list) {
if (req->request.num_mapped_sgs > 0)
-   dwc3_prepare_one_trb_sg(dep, req, trbs_left--);
+   dwc3_prepare_one_trb_sg(dep, req, trbs_left--,
+   starting);
else
-   dwc3_prepare_one_trb_linear(dep, req, trbs_left--);
+   dwc3_prepare_one_trb_linear(dep, req, trbs_left--,
+   starting);
 
if (!trbs_left)
return;
@@ -968,7 +982,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, 
u16 cmd_param)
 
starting = !(dep->flags & DWC3_EP_BUSY);
 
-   dwc3_prepare_trbs(dep);
+   dwc3_prepare_trbs(dep, starting);
req = next_request(&dep->started_list);
if (!req) {
dep->flags |= DWC3_EP_PENDING_REQUEST;
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
To unsubscribe from this list: send the line "unsu

[PATCH 04/22] usb: dwc3: gadget: prepare TRBs on update transfers too

[Felipe Balbi]

If we're updating transfers, we can also prepare as
many TRBs as we can fit in the ring. Let's start
doing that.

This patch 'solves' a limitation of how many TRBs we
can prepare when we're getting close the end of the
ring. Instead driver to prepare only up to end of
the ring, we check if we have space to wrap around
the ring properly.

Note that this only happens when our enqueue and
dequeue pointers are equal (which is the case for
bulk endpoints after an XferComplete event).

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 50 +++
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 309cdb4d950b..fa38ab3086e7 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -858,16 +858,40 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
trace_dwc3_prepare_trb(dep, trb);
 }
 
+static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
+{
+   struct dwc3_trb *tmp;
+
+   /*
+* If enqueue & dequeue are equal than it is either full or empty.
+*
+* One way to know for sure is if the TRB right before us has HWO bit
+* set or not. If it has, then we're definitely full and can't fit any
+* more transfers in our ring.
+*/
+   if (dep->trb_enqueue == dep->trb_dequeue) {
+   /* If we're full, enqueue/dequeue are > 0 */
+   if (dep->trb_enqueue) {
+   tmp = &dep->trb_pool[dep->trb_enqueue - 1];
+   if (tmp->ctrl & DWC3_TRB_CTRL_HWO)
+   return 0;
+   }
+
+   return DWC3_TRB_NUM - 1;
+   }
+
+   return dep->trb_dequeue - dep->trb_enqueue;
+}
+
 /*
  * dwc3_prepare_trbs - setup TRBs from requests
  * @dep: endpoint for which requests are being prepared
- * @starting: true if the endpoint is idle and no requests are queued.
  *
  * The function goes through the requests list and sets up TRBs for the
  * transfers. The function returns once there are no more TRBs available or
  * it runs out of requests.
  */
-static void dwc3_prepare_trbs(struct dwc3_ep *dep, bool starting)
+static void dwc3_prepare_trbs(struct dwc3_ep *dep)
 {
struct dwc3_request *req, *n;
u32 trbs_left;
@@ -875,23 +899,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep, bool 
starting)
 
BUILD_BUG_ON_NOT_POWER_OF_2(DWC3_TRB_NUM);
 
-   trbs_left = dep->trb_dequeue - dep->trb_enqueue;
-
-   /*
-* If enqueue & dequeue are equal than it is either full or empty. If we
-* are starting to process requests then we are empty. Otherwise we are
-* full and don't do anything
-*/
-   if (!trbs_left) {
-   if (!starting)
-   return;
-
-   trbs_left = DWC3_TRB_NUM;
-   }
-
-   /* The last TRB is a link TRB, not used for xfer */
-   if (trbs_left <= 1)
-   return;
+   trbs_left = dwc3_calc_trbs_left(dep);
 
list_for_each_entry_safe(req, n, &dep->pending_list, list) {
unsignedlength;
@@ -974,12 +982,12 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep 
*dep, u16 cmd_param,
 */
if (start_new) {
if (list_empty(&dep->started_list))
-   dwc3_prepare_trbs(dep, start_new);
+   dwc3_prepare_trbs(dep);
 
/* req points to the first request which will be sent */
req = next_request(&dep->started_list);
} else {
-   dwc3_prepare_trbs(dep, start_new);
+   dwc3_prepare_trbs(dep);
 
/*
 * req points to the first request where HWO changed from 0 to 1
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 09/22] usb: dwc3: gadget: split __dwc3_gadget_kick_transfer()

[Felipe Balbi]

To aid code readability, we're gonna split
__dwc3_gadget_kick_transfer() into its constituent
parts: scatter gather and linear buffers.

That way, it's easier to follow the code and focus
debug effort when one or the other fails.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 122 --
 1 file changed, 65 insertions(+), 57 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index eca131f0be59..adf743bca36f 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -881,6 +881,65 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
return dep->trb_dequeue - dep->trb_enqueue;
 }
 
+static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
+   struct dwc3_request *req, unsigned int trbs_left)
+{
+   struct usb_request *request = &req->request;
+   struct scatterlist *sg = request->sg;
+   struct scatterlist *s;
+   unsigned intlast = false;
+   unsigned intlength;
+   dma_addr_t  dma;
+   int i;
+
+   for_each_sg(sg, s, request->num_mapped_sgs, i) {
+   unsigned chain = true;
+
+   length = sg_dma_len(s);
+   dma = sg_dma_address(s);
+
+   if (sg_is_last(s)) {
+   if (list_is_last(&req->list, &dep->pending_list))
+   last = true;
+
+   chain = false;
+   }
+
+   if (!trbs_left)
+   last = true;
+
+   if (last)
+   chain = false;
+
+   dwc3_prepare_one_trb(dep, req, dma, length,
+   last, chain, i);
+
+   if (last)
+   break;
+   }
+}
+
+static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
+   struct dwc3_request *req, unsigned int trbs_left)
+{
+   unsigned intlast = false;
+   unsigned intlength;
+   dma_addr_t  dma;
+
+   dma = req->request.dma;
+   length = req->request.length;
+
+   if (!trbs_left)
+   last = true;
+
+   /* Is this the last request? */
+   if (list_is_last(&req->list, &dep->pending_list))
+   last = true;
+
+   dwc3_prepare_one_trb(dep, req, dma, length,
+   last, false, 0);
+}
+
 /*
  * dwc3_prepare_trbs - setup TRBs from requests
  * @dep: endpoint for which requests are being prepared
@@ -893,70 +952,19 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
 {
struct dwc3_request *req, *n;
u32 trbs_left;
-   unsigned intlast_one = 0;
 
BUILD_BUG_ON_NOT_POWER_OF_2(DWC3_TRB_NUM);
 
trbs_left = dwc3_calc_trbs_left(dep);
 
list_for_each_entry_safe(req, n, &dep->pending_list, list) {
-   unsignedlength;
-   dma_addr_t  dma;
-   last_one = false;
-
-   if (req->request.num_mapped_sgs > 0) {
-   struct usb_request *request = &req->request;
-   struct scatterlist *sg = request->sg;
-   struct scatterlist *s;
-   int i;
-
-   for_each_sg(sg, s, request->num_mapped_sgs, i) {
-   unsigned chain = true;
-
-   length = sg_dma_len(s);
-   dma = sg_dma_address(s);
-
-   if (sg_is_last(s)) {
-   if (list_is_last(&req->list, 
&dep->pending_list))
-   last_one = true;
-
-   chain = false;
-   }
-
-   trbs_left--;
-   if (!trbs_left)
-   last_one = true;
-
-   if (last_one)
-   chain = false;
-
-   dwc3_prepare_one_trb(dep, req, dma, length,
-   last_one, chain, i);
-
-   if (last_one)
-   break;
-   }
-
-   if (last_one)
-   break;
-   } else {
-   dma = req->request.dma;
-   length = req->request.length;
-   trbs_left--;
-
-   if (!trbs_left)
-   last_one = true;
-
-   /* Is this the last request? */
-   if (list_is_last(&req->list, &dep->pending_list))
-   last_one = true;
-
-  

[PATCH 02/22] usb: dwc3: gadget: fix gadget suspend/resume

[Felipe Balbi]

Instead of trying hard to stay connected to the
host, it's best (and far easier) to disconnect from
the host already.

Anything relying on KEEP_CONNECT will just have that
ignored, but we don't have proper hibernation
implementation yet, so there are no regressions.

In any case, hibernation is only useful for runtime
PM, not system sleep.

While at that, also remove dwc3.dcfg which has been
rendered unnecessary.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/core.h   |  1 -
 drivers/usb/dwc3/gadget.c | 44 
 2 files changed, 12 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 7ddf9449a063..9366351ac136 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -818,7 +818,6 @@ struct dwc3 {
enum usb_dr_modedr_mode;
 
/* used for suspend/resume */
-   u32 dcfg;
u32 gctl;
 
u32 nr_scratch;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index db5f81b4ffdc..309cdb4d950b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2925,60 +2925,40 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
 
 int dwc3_gadget_suspend(struct dwc3 *dwc)
 {
+   int ret;
+
if (!dwc->gadget_driver)
return 0;
 
-   if (dwc->pullups_connected) {
-   dwc3_gadget_disable_irq(dwc);
-   dwc3_gadget_run_stop(dwc, true, true);
-   }
-
-   __dwc3_gadget_ep_disable(dwc->eps[0]);
-   __dwc3_gadget_ep_disable(dwc->eps[1]);
+   ret = dwc3_gadget_run_stop(dwc, false, false);
+   if (ret < 0)
+   return ret;
 
-   dwc->dcfg = dwc3_readl(dwc->regs, DWC3_DCFG);
+   dwc3_disconnect_gadget(dwc);
+   __dwc3_gadget_stop(dwc);
 
return 0;
 }
 
 int dwc3_gadget_resume(struct dwc3 *dwc)
 {
-   struct dwc3_ep  *dep;
int ret;
 
if (!dwc->gadget_driver)
return 0;
 
-   /* Start with SuperSpeed Default */
-   dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
-
-   dep = dwc->eps[0];
-   ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL, false,
-   false);
-   if (ret)
+   ret = __dwc3_gadget_start(dwc);
+   if (ret < 0)
goto err0;
 
-   dep = dwc->eps[1];
-   ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL, false,
-   false);
-   if (ret)
+   ret = dwc3_gadget_run_stop(dwc, true, false);
+   if (ret < 0)
goto err1;
 
-   /* begin to receive SETUP packets */
-   dwc->ep0state = EP0_SETUP_PHASE;
-   dwc3_ep0_out_start(dwc);
-
-   dwc3_writel(dwc->regs, DWC3_DCFG, dwc->dcfg);
-
-   if (dwc->pullups_connected) {
-   dwc3_gadget_enable_irq(dwc);
-   dwc3_gadget_run_stop(dwc, true, false);
-   }
-
return 0;
 
 err1:
-   __dwc3_gadget_ep_disable(dwc->eps[0]);
+   __dwc3_gadget_stop(dwc);
 
 err0:
return ret;
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 17/22] usb: dwc3: core: re-factor init and exit paths

[Felipe Balbi]

The idea of this patch is for dwc3_core_init() to
abstract all the details about how to initialize
dwc3 and dwc3_core_exit() to do the same for
teardown.

With this, we can simplify suspend/resume operations
by a large margin and always know that we're going
to start dwc3 from a known starting point.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/core.c | 118 
 1 file changed, 60 insertions(+), 58 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 1f4ac355f384..cbdefbb3d302 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -506,6 +506,21 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
return 0;
 }
 
+static void dwc3_core_exit(struct dwc3 *dwc)
+{
+   dwc3_event_buffers_cleanup(dwc);
+
+   usb_phy_shutdown(dwc->usb2_phy);
+   usb_phy_shutdown(dwc->usb3_phy);
+   phy_exit(dwc->usb2_generic_phy);
+   phy_exit(dwc->usb3_generic_phy);
+
+   usb_phy_set_suspend(dwc->usb2_phy, 1);
+   usb_phy_set_suspend(dwc->usb3_phy, 1);
+   phy_power_off(dwc->usb2_generic_phy);
+   phy_power_off(dwc->usb3_generic_phy);
+}
+
 /**
  * dwc3_core_init - Low-level initialization of DWC3 Core
  * @dwc: Pointer to our controller context structure
@@ -555,6 +570,10 @@ static int dwc3_core_init(struct dwc3 *dwc)
if (ret)
goto err0;
 
+   ret = dwc3_phy_setup(dwc);
+   if (ret)
+   goto err0;
+
reg = dwc3_readl(dwc->regs, DWC3_GCTL);
reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
 
@@ -621,22 +640,45 @@ static int dwc3_core_init(struct dwc3 *dwc)
if (dwc->revision < DWC3_REVISION_190A)
reg |= DWC3_GCTL_U2RSTECN;
 
-   dwc3_core_num_eps(dwc);
-
dwc3_writel(dwc->regs, DWC3_GCTL, reg);
 
-   ret = dwc3_alloc_scratch_buffers(dwc);
-   if (ret)
-   goto err1;
+   dwc3_core_num_eps(dwc);
 
ret = dwc3_setup_scratch_buffers(dwc);
if (ret)
+   goto err1;
+
+   /* Adjust Frame Length */
+   dwc3_frame_length_adjustment(dwc);
+
+   usb_phy_set_suspend(dwc->usb2_phy, 0);
+   usb_phy_set_suspend(dwc->usb3_phy, 0);
+   ret = phy_power_on(dwc->usb2_generic_phy);
+   if (ret < 0)
goto err2;
 
+   ret = phy_power_on(dwc->usb3_generic_phy);
+   if (ret < 0)
+   goto err3;
+
+   ret = dwc3_event_buffers_setup(dwc);
+   if (ret) {
+   dev_err(dwc->dev, "failed to setup event buffers\n");
+   goto err4;
+   }
+
return 0;
 
+err4:
+   phy_power_off(dwc->usb2_generic_phy);
+
+err3:
+   phy_power_off(dwc->usb3_generic_phy);
+
 err2:
-   dwc3_free_scratch_buffers(dwc);
+   usb_phy_set_suspend(dwc->usb2_phy, 1);
+   usb_phy_set_suspend(dwc->usb3_phy, 1);
+   dwc3_core_exit(dwc);
 
 err1:
usb_phy_shutdown(dwc->usb2_phy);
@@ -648,15 +690,6 @@ err0:
return ret;
 }
 
-static void dwc3_core_exit(struct dwc3 *dwc)
-{
-   dwc3_free_scratch_buffers(dwc);
-   usb_phy_shutdown(dwc->usb2_phy);
-   usb_phy_shutdown(dwc->usb3_phy);
-   phy_exit(dwc->usb2_generic_phy);
-   phy_exit(dwc->usb3_generic_phy);
-}
-
 static int dwc3_core_get_phy(struct dwc3 *dwc)
 {
struct device   *dev = dwc->dev;
@@ -951,10 +984,6 @@ static int dwc3_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, dwc);
dwc3_cache_hwparams(dwc);
 
-   ret = dwc3_phy_setup(dwc);
-   if (ret)
-   goto err0;
-
ret = dwc3_core_get_phy(dwc);
if (ret)
goto err0;
@@ -975,7 +1004,7 @@ static int dwc3_probe(struct platform_device *pdev)
if (ret) {
dev_err(dwc->dev, "failed to allocate event buffers\n");
ret = -ENOMEM;
-   goto err1;
+   goto err0;
}
 
if (IS_ENABLED(CONFIG_USB_DWC3_HOST))
@@ -986,10 +1015,14 @@ static int dwc3_probe(struct platform_device *pdev)
if (dwc->dr_mode == USB_DR_MODE_UNKNOWN)
dwc->dr_mode = USB_DR_MODE_OTG;
 
+   ret = dwc3_alloc_scratch_buffers(dwc);
+   if (ret)
+   goto err1;
+
ret = dwc3_core_init(dwc);
if (ret) {
dev_err(dev, "failed to initialize core\n");
-   goto err1;
+   goto err2;
}
 
/* Check the maximum_speed parameter */
@@ -1019,47 +1052,20 @@ static int dwc3_probe(struct platform_device *pdev)
break;
}
 
-   /* Adjust Frame Length */
-   dwc3_frame_length_adjustment(dwc);
-
-   usb_phy_set_suspend(dwc->usb2_phy, 0);
-   usb_phy_set_suspend(dwc->usb3_phy, 0);
-   ret = phy_power_on(dwc->usb2_generic_phy);
-   if (ret < 0)
-   goto err2;
-
-   r

[PATCH 05/22] usb: dwc3: gadget: simplify __dwc3_gadget_kick_transfer()

[Felipe Balbi]

as it turns out, we don't need the extra 'start_new'
argument as that can be inferred from DWC3_EP_BUSY
flag.

Because of that, we can simplify
__dwc3_gadget_kick_transfer() by quite a bit, even
allowing us to prepare more TRBs unconditionally.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 48 ++-
 1 file changed, 14 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index fa38ab3086e7..3640808e326d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -962,38 +962,19 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
}
 }
 
-static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, u16 cmd_param,
-   int start_new)
+static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, u16 cmd_param)
 {
struct dwc3_gadget_ep_cmd_params params;
struct dwc3_request *req;
struct dwc3 *dwc = dep->dwc;
+   int starting;
int ret;
u32 cmd;
 
-   if (start_new && (dep->flags & DWC3_EP_BUSY)) {
-   dwc3_trace(trace_dwc3_gadget, "%s: endpoint busy", dep->name);
-   return -EBUSY;
-   }
-
-   /*
-* If we are getting here after a short-out-packet we don't enqueue any
-* new requests as we try to set the IOC bit only on the last request.
-*/
-   if (start_new) {
-   if (list_empty(&dep->started_list))
-   dwc3_prepare_trbs(dep);
+   starting = !(dep->flags & DWC3_EP_BUSY);
 
-   /* req points to the first request which will be sent */
-   req = next_request(&dep->started_list);
-   } else {
-   dwc3_prepare_trbs(dep);
-
-   /*
-* req points to the first request where HWO changed from 0 to 1
-*/
-   req = next_request(&dep->started_list);
-   }
+   dwc3_prepare_trbs(dep);
+   req = next_request(&dep->started_list);
if (!req) {
dep->flags |= DWC3_EP_PENDING_REQUEST;
return 0;
@@ -1001,7 +982,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep 
*dep, u16 cmd_param,
 
memset(¶ms, 0, sizeof(params));
 
-   if (start_new) {
+   if (starting) {
params.param0 = upper_32_bits(req->trb_dma);
params.param1 = lower_32_bits(req->trb_dma);
cmd = DWC3_DEPCMD_STARTTRANSFER;
@@ -1025,7 +1006,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep 
*dep, u16 cmd_param,
 
dep->flags |= DWC3_EP_BUSY;
 
-   if (start_new) {
+   if (starting) {
dep->resource_index = dwc3_gadget_ep_get_transfer_index(dwc,
dep->number);
WARN_ON_ONCE(!dep->resource_index);
@@ -1050,7 +1031,7 @@ static void __dwc3_gadget_start_isoc(struct dwc3 *dwc,
/* 4 micro frames in the future */
uf = cur_uf + dep->interval * 4;
 
-   __dwc3_gadget_kick_transfer(dep, uf, 1);
+   __dwc3_gadget_kick_transfer(dep, uf);
 }
 
 static void dwc3_gadget_start_isoc(struct dwc3 *dwc,
@@ -1119,7 +1100,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, 
struct dwc3_request *req)
if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
!usb_endpoint_xfer_int(dep->endpoint.desc) &&
!(dep->flags & DWC3_EP_BUSY)) {
-   ret = __dwc3_gadget_kick_transfer(dep, 0, true);
+   ret = __dwc3_gadget_kick_transfer(dep, 0);
goto out;
}
 
@@ -1149,7 +1130,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, 
struct dwc3_request *req)
return 0;
}
 
-   ret = __dwc3_gadget_kick_transfer(dep, 0, true);
+   ret = __dwc3_gadget_kick_transfer(dep, 0);
if (!ret)
dep->flags &= ~DWC3_EP_PENDING_REQUEST;
 
@@ -1165,8 +1146,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, 
struct dwc3_request *req)
(dep->flags & DWC3_EP_BUSY) &&
!(dep->flags & DWC3_EP_MISSED_ISOC)) {
WARN_ON_ONCE(!dep->resource_index);
-   ret = __dwc3_gadget_kick_transfer(dep, dep->resource_index,
-   false);
+   ret = __dwc3_gadget_kick_transfer(dep, dep->resource_index);
goto out;
}
 
@@ -1176,7 +1156,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, 
struct dwc3_request *req)
 * handled.
 */
if (dep->stream_capable)
-   ret

[PATCH 01/22] usb: dwc3: gadget: re-factor ->udc_start and ->udc_stop

[Felipe Balbi]

we will be re-using it for suspend/resume, so
instead of duplicating code, let's just re-factor
the functions so they can be re-used.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 90 ++-
 1 file changed, 49 insertions(+), 41 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9a7d0bd15dc3..db5f81b4ffdc 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1595,37 +1595,12 @@ static void dwc3_gadget_disable_irq(struct dwc3 *dwc)
 static irqreturn_t dwc3_interrupt(int irq, void *_dwc);
 static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc);
 
-static int dwc3_gadget_start(struct usb_gadget *g,
-   struct usb_gadget_driver *driver)
+static int __dwc3_gadget_start(struct dwc3 *dwc)
 {
-   struct dwc3 *dwc = gadget_to_dwc(g);
struct dwc3_ep  *dep;
-   unsigned long   flags;
int ret = 0;
-   int irq;
u32 reg;
 
-   irq = platform_get_irq(to_platform_device(dwc->dev), 0);
-   ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
-   IRQF_SHARED, "dwc3", dwc->ev_buf);
-   if (ret) {
-   dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
-   irq, ret);
-   goto err0;
-   }
-
-   spin_lock_irqsave(&dwc->lock, flags);
-
-   if (dwc->gadget_driver) {
-   dev_err(dwc->dev, "%s is already bound to %s\n",
-   dwc->gadget.name,
-   dwc->gadget_driver->driver.name);
-   ret = -EBUSY;
-   goto err1;
-   }
-
-   dwc->gadget_driver  = driver;
-
reg = dwc3_readl(dwc->regs, DWC3_DCFG);
reg &= ~(DWC3_DCFG_SPEED_MASK);
 
@@ -1688,7 +1663,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
false);
if (ret) {
dev_err(dwc->dev, "failed to enable %s\n", dep->name);
-   goto err2;
+   goto err0;
}
 
dep = dwc->eps[1];
@@ -1696,7 +1671,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
false);
if (ret) {
dev_err(dwc->dev, "failed to enable %s\n", dep->name);
-   goto err3;
+   goto err1;
}
 
/* begin to receive SETUP packets */
@@ -1705,39 +1680,72 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 
dwc3_gadget_enable_irq(dwc);
 
-   spin_unlock_irqrestore(&dwc->lock, flags);
-
return 0;
 
-err3:
-   __dwc3_gadget_ep_disable(dwc->eps[0]);
-
-err2:
-   dwc->gadget_driver = NULL;
-
 err1:
-   spin_unlock_irqrestore(&dwc->lock, flags);
-
-   free_irq(irq, dwc->ev_buf);
+   __dwc3_gadget_ep_disable(dwc->eps[0]);
 
 err0:
return ret;
 }
 
-static int dwc3_gadget_stop(struct usb_gadget *g)
+static int dwc3_gadget_start(struct usb_gadget *g,
+   struct usb_gadget_driver *driver)
 {
struct dwc3 *dwc = gadget_to_dwc(g);
unsigned long   flags;
+   int ret = 0;
int irq;
 
+   irq = platform_get_irq(to_platform_device(dwc->dev), 0);
+   ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
+   IRQF_SHARED, "dwc3", dwc->ev_buf);
+   if (ret) {
+   dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
+   irq, ret);
+   goto err0;
+   }
+
spin_lock_irqsave(&dwc->lock, flags);
+   if (dwc->gadget_driver) {
+   dev_err(dwc->dev, "%s is already bound to %s\n",
+   dwc->gadget.name,
+   dwc->gadget_driver->driver.name);
+   ret = -EBUSY;
+   goto err1;
+   }
+
+   dwc->gadget_driver  = driver;
 
+   __dwc3_gadget_start(dwc);
+   spin_unlock_irqrestore(&dwc->lock, flags);
+
+   return 0;
+
+err1:
+   spin_unlock_irqrestore(&dwc->lock, flags);
+   free_irq(irq, dwc);
+
+err0:
+   return ret;
+}
+
+static void __dwc3_gadget_stop(struct dwc3 *dwc)
+{
dwc3_gadget_disable_irq(dwc);
__dwc3_gadget_ep_disable(dwc->eps[0]);
__dwc3_gadget_ep_disable(dwc->eps[1]);
+}
 
-   dwc->gadget_driver  = NULL;
+static int dwc3_gadget_stop(struct usb_gadget *g)
+{
+   struct dwc3 *dwc = gadget_to_dwc(g);
+   unsigned long   flags;
+   int irq;
 
+   spin_loc

[PATCH 16/22] usb: dwc3: core: move fladj to dwc3 structure

[Felipe Balbi]

this patch is in preparation for some further
re-factoring in dwc3 initialization. No functional
changes.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/core.c | 16 +++-
 drivers/usb/dwc3/core.h |  2 ++
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 245f4ff6ae16..1f4ac355f384 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -149,9 +149,8 @@ static int dwc3_soft_reset(struct dwc3 *dwc)
 /*
  * dwc3_frame_length_adjustment - Adjusts frame length if required
  * @dwc3: Pointer to our controller context structure
- * @fladj: Value of GFLADJ_30MHZ to adjust frame length
  */
-static void dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj)
+static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
 {
u32 reg;
u32 dft;
@@ -159,15 +158,15 @@ static void dwc3_frame_length_adjustment(struct dwc3 
*dwc, u32 fladj)
if (dwc->revision < DWC3_REVISION_250A)
return;
 
-   if (fladj == 0)
+   if (dwc->fladj == 0)
return;
 
reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
dft = reg & DWC3_GFLADJ_30MHZ_MASK;
-   if (!dev_WARN_ONCE(dwc->dev, dft == fladj,
+   if (!dev_WARN_ONCE(dwc->dev, dft == dwc->fladj,
"request value same as default, ignoring\n")) {
reg &= ~DWC3_GFLADJ_30MHZ_MASK;
-   reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL | fladj;
+   reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL | dwc->fladj;
dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
}
 }
@@ -799,7 +798,6 @@ static int dwc3_probe(struct platform_device *pdev)
u8  lpm_nyet_threshold;
u8  tx_de_emphasis;
u8  hird_threshold;
-   u32 fladj = 0;
 
int ret;
 
@@ -909,7 +907,7 @@ static int dwc3_probe(struct platform_device *pdev)
device_property_read_string(dev, "snps,hsphy_interface",
&dwc->hsphy_interface);
device_property_read_u32(dev, "snps,quirk-frame-length-adjustment",
-&fladj);
+&dwc->fladj);
 
if (pdata) {
dwc->maximum_speed = pdata->maximum_speed;
@@ -941,7 +939,7 @@ static int dwc3_probe(struct platform_device *pdev)
tx_de_emphasis = pdata->tx_de_emphasis;
 
dwc->hsphy_interface = pdata->hsphy_interface;
-   fladj = pdata->fladj_value;
+   dwc->fladj = pdata->fladj_value;
}
 
dwc->lpm_nyet_threshold = lpm_nyet_threshold;
@@ -1022,7 +1020,7 @@ static int dwc3_probe(struct platform_device *pdev)
}
 
/* Adjust Frame Length */
-   dwc3_frame_length_adjustment(dwc, fladj);
+   dwc3_frame_length_adjustment(dwc);
 
usb_phy_set_suspend(dwc->usb2_phy, 0);
usb_phy_set_suspend(dwc->usb3_phy, 0);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index fb0f9d94f1b2..ce9596b1b1f4 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -728,6 +728,7 @@ struct dwc3_scratchpad_array {
  * @gadget_driver: pointer to the gadget driver
  * @regs: base address for our registers
  * @regs_size: address space size
+ * @fladj: frame length adjustment
  * @nr_scratch: number of scratch buffers
  * @u1u2: only used on revisions <1.83a for workaround
  * @maximum_speed: maximum speed requested (mainly for testing purposes)
@@ -837,6 +838,7 @@ struct dwc3 {
/* used for suspend/resume */
u32 gctl;
 
+   u32 fladj;
u32 nr_scratch;
u32 u1u2;
u32 maximum_speed;
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 22/22] usb: gadget: storage: increase maximum storage num buffers

[Felipe Balbi]

With a default size of 16kiB and with maximum of 32
buffers, we can transfer up to 512kiB, however Linux
can transfer up to 1MiB in a single mass storage
block transfer to USB3 storage devices.

Because of this, 1MiB block transfers end up being
slower than 512kiB block transfers. Let's increase
maximum number of storage buffers to a ridiculous
amount (256) so that anybody wanting to test maximum
achievable throughput can do so.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/gadget/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 2057add439f0..3c3f31ceece7 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -114,7 +114,7 @@ config USB_GADGET_VBUS_DRAW
 
 config USB_GADGET_STORAGE_NUM_BUFFERS
int "Number of storage pipeline buffers"
-   range 2 32
+   range 2 256
default 2
help
   Usually 2 buffers are enough to establish a good buffering
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 06/22] usb: dwc3: gadget: rely on sg_is_last() and list_is_last()

[Felipe Balbi]

sg_is_last() and list_is_last() will encode the
required information for the driver to make
decisions WRT CHN and LST bits.

While at that, also replace '1' with 'true' for
consistency.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3640808e326d..fab2d166d844 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -918,10 +918,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
length = sg_dma_len(s);
dma = sg_dma_address(s);
 
-   if (i == (request->num_mapped_sgs - 1) ||
-   sg_is_last(s)) {
-   if (list_empty(&dep->pending_list))
+   if (sg_is_last(s)) {
+   if (list_is_last(&req->list, 
&dep->pending_list))
last_one = true;
+
chain = false;
}
 
@@ -947,11 +947,11 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
trbs_left--;
 
if (!trbs_left)
-   last_one = 1;
+   last_one = true;
 
/* Is this the last request? */
if (list_is_last(&req->list, &dep->pending_list))
-   last_one = 1;
+   last_one = true;
 
dwc3_prepare_one_trb(dep, req, dma, length,
last_one, false, 0);
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 18/22] usb: dwc3: core: simplify suspend/resume operations

[Felipe Balbi]

now that we have re-factored dwc3_core_init() and
dwc3_core_exit() we can use them for suspend/resume
operations.

This will help us avoid some common mistakes when
patching code when we have duplicated pieces of code
doing the same thing.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/core.c | 59 +
 drivers/usb/dwc3/core.h |  3 ---
 2 files changed, 5 insertions(+), 57 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index cbdefbb3d302..80e9affd3d77 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1113,33 +1113,19 @@ static int dwc3_remove(struct platform_device *pdev)
 static int dwc3_suspend(struct device *dev)
 {
struct dwc3 *dwc = dev_get_drvdata(dev);
-   unsigned long   flags;
-
-   spin_lock_irqsave(&dwc->lock, flags);
 
switch (dwc->dr_mode) {
case USB_DR_MODE_PERIPHERAL:
case USB_DR_MODE_OTG:
dwc3_gadget_suspend(dwc);
-   /* FALLTHROUGH */
+   break;
case USB_DR_MODE_HOST:
default:
-   dwc3_event_buffers_cleanup(dwc);
+   /* do nothing */
break;
}
 
-   dwc->gctl = dwc3_readl(dwc->regs, DWC3_GCTL);
-   spin_unlock_irqrestore(&dwc->lock, flags);
-
-   usb_phy_shutdown(dwc->usb3_phy);
-   usb_phy_shutdown(dwc->usb2_phy);
-   phy_exit(dwc->usb2_generic_phy);
-   phy_exit(dwc->usb3_generic_phy);
-
-   usb_phy_set_suspend(dwc->usb2_phy, 1);
-   usb_phy_set_suspend(dwc->usb3_phy, 1);
-   WARN_ON(phy_power_off(dwc->usb2_generic_phy) < 0);
-   WARN_ON(phy_power_off(dwc->usb3_generic_phy) < 0);
+   dwc3_core_exit(dwc);
 
pinctrl_pm_select_sleep_state(dev);
 
@@ -1149,36 +1135,14 @@ static int dwc3_suspend(struct device *dev)
 static int dwc3_resume(struct device *dev)
 {
struct dwc3 *dwc = dev_get_drvdata(dev);
-   unsigned long   flags;
int ret;
 
pinctrl_pm_select_default_state(dev);
 
-   usb_phy_set_suspend(dwc->usb2_phy, 0);
-   usb_phy_set_suspend(dwc->usb3_phy, 0);
-   ret = phy_power_on(dwc->usb2_generic_phy);
-   if (ret < 0)
+   ret = dwc3_core_init(dwc);
+   if (ret)
return ret;
 
-   ret = phy_power_on(dwc->usb3_generic_phy);
-   if (ret < 0)
-   goto err_usb2phy_power;
-
-   usb_phy_init(dwc->usb3_phy);
-   usb_phy_init(dwc->usb2_phy);
-   ret = phy_init(dwc->usb2_generic_phy);
-   if (ret < 0)
-   goto err_usb3phy_power;
-
-   ret = phy_init(dwc->usb3_generic_phy);
-   if (ret < 0)
-   goto err_usb2phy_init;
-
-   spin_lock_irqsave(&dwc->lock, flags);
-
-   dwc3_event_buffers_setup(dwc);
-   dwc3_writel(dwc->regs, DWC3_GCTL, dwc->gctl);
-
switch (dwc->dr_mode) {
case USB_DR_MODE_PERIPHERAL:
case USB_DR_MODE_OTG:
@@ -1190,24 +1154,11 @@ static int dwc3_resume(struct device *dev)
break;
}
 
-   spin_unlock_irqrestore(&dwc->lock, flags);
-
pm_runtime_disable(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
 
return 0;
-
-err_usb2phy_init:
-   phy_exit(dwc->usb2_generic_phy);
-
-err_usb3phy_power:
-   phy_power_off(dwc->usb3_generic_phy);
-
-err_usb2phy_power:
-   phy_power_off(dwc->usb2_generic_phy);
-
-   return ret;
 }
 #endif /* CONFIG_PM_SLEEP */
 
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index ce9596b1b1f4..58509cedb038 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -835,9 +835,6 @@ struct dwc3 {
 
enum usb_dr_modedr_mode;
 
-   /* used for suspend/resume */
-   u32 gctl;
-
u32 fladj;
u32 nr_scratch;
u32 u1u2;
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 21/22] usb: gadget: storage: get rid of fsg_num_buffers_validate()

[Felipe Balbi]

valid range for storage buffers is encoded in
Kconfig already. Instead of checking again, let's
drop fsg_num_buffers_validate() altogether.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/gadget/function/f_mass_storage.c | 18 +-
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index 5c6d4d7ca605..e9c0bc208d7e 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2655,18 +2655,6 @@ void fsg_common_put(struct fsg_common *common)
 }
 EXPORT_SYMBOL_GPL(fsg_common_put);
 
-/* check if fsg_num_buffers is within a valid range */
-static inline int fsg_num_buffers_validate(unsigned int fsg_num_buffers)
-{
-#define FSG_MAX_NUM_BUFFERS32
-
-   if (fsg_num_buffers >= 2 && fsg_num_buffers <= FSG_MAX_NUM_BUFFERS)
-   return 0;
-   pr_err("fsg_num_buffers %u is out of range (%d to %d)\n",
-  fsg_num_buffers, 2, FSG_MAX_NUM_BUFFERS);
-   return -EINVAL;
-}
-
 static struct fsg_common *fsg_common_setup(struct fsg_common *common)
 {
if (!common) {
@@ -2709,11 +2697,7 @@ static void _fsg_common_free_buffers(struct fsg_buffhd 
*buffhds, unsigned n)
 int fsg_common_set_num_buffers(struct fsg_common *common, unsigned int n)
 {
struct fsg_buffhd *bh, *buffhds;
-   int i, rc;
-
-   rc = fsg_num_buffers_validate(n);
-   if (rc != 0)
-   return rc;
+   int i;
 
buffhds = kcalloc(n, sizeof(*buffhds), GFP_KERNEL);
if (!buffhds)
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 12/22] usb: dwc3: gadget: start bulk endpoints more frequently

[Felipe Balbi]

Instead of waiting for !BUSY, we can kick bulk
endpoints more frequently and rely on the fact that
__dwc3_gadget_kick_transfer() will use Update
Transfer if BUSY flag is set.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 47 +++
 1 file changed, 15 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 809445ec6a87..f9f7fe2c1e14 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1080,18 +1080,6 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, 
struct dwc3_request *req)
 
trace_dwc3_ep_queue(req);
 
-   /*
-* We only add to our list of requests now and
-* start consuming the list once we get XferNotReady
-* IRQ.
-*
-* That way, we avoid doing anything that we don't need
-* to do now and defer it until the point we receive a
-* particular token from the Host side.
-*
-* This will also avoid Host cancelling URBs due to too
-* many NAKs.
-*/
ret = usb_gadget_map_request(&dwc->gadget, &req->request,
dep->direction);
if (ret)
@@ -1100,28 +1088,24 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, 
struct dwc3_request *req)
list_add_tail(&req->list, &dep->pending_list);
 
/*
-* If there are no pending requests and the endpoint isn't already
-* busy, we will just start the request straight away.
+* If this is a bulk endpoint, then just issue Start Transfer or Update
+* Transfer straight away. This will possibly avoid a few XferNotReady
+* events and let us recycle a struct usb_request earlier.
 *
-* This will save one IRQ (XFER_NOT_READY) and possibly make it a
-* little bit faster.
 */
if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
-   !usb_endpoint_xfer_int(dep->endpoint.desc) &&
-   !(dep->flags & DWC3_EP_BUSY)) {
+   !usb_endpoint_xfer_int(dep->endpoint.desc)) {
ret = __dwc3_gadget_kick_transfer(dep, 0);
goto out;
}
 
/*
-* There are a few special cases:
-*
-* 1. XferNotReady with empty list of requests. We need to kick the
-*transfer here in that situation, otherwise we will be NAKing
-*forever. If we get XferNotReady before gadget driver has a
-*chance to queue a request, we will ACK the IRQ but won't be
-*able to receive the data until the next request is queued.
-*The following code is handling exactly that.
+* XferNotReady with empty list of requests. We need to kick the
+* transfer here in that situation, otherwise we will be NAKing
+* forever. If we get XferNotReady before gadget driver has a chance to
+* queue a request, we will ACK the IRQ but won't be able to receive the
+* data until the next request is queued.  The following code is
+* handling exactly that.
 *
 */
if (dep->flags & DWC3_EP_PENDING_REQUEST) {
@@ -1147,9 +1131,9 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, 
struct dwc3_request *req)
}
 
/*
-* 2. XferInProgress on Isoc EP with an active transfer. We need to
-*kick the transfer here after queuing a request, otherwise the
-*core may not see the modified TRB(s).
+* XferInProgress on Isoc EP with an active transfer. We need to kick
+* the transfer here after queuing a request, otherwise the core may not
+* see the modified TRB(s).
 */
if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
(dep->flags & DWC3_EP_BUSY) &&
@@ -1160,9 +1144,8 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, 
struct dwc3_request *req)
}
 
/*
-* 4. Stream Capable Bulk Endpoints. We need to start the transfer
-* right away, otherwise host will not know we have streams to be
-* handled.
+* Stream Capable Bulk Endpoints. We need to start the transfer right
+* away, otherwise host will not know we have streams to be handled.
 */
if (dep->stream_capable)
ret = __dwc3_gadget_kick_transfer(dep, 0);
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete 

[PATCH 14/22] usb: dwc3: gadget: add a pointer to endpoint registers

[Felipe Balbi]

By adding a pointer to endpoint registers' base
address, we can avoid using our controller-wide
struct dwc3 pointer for everything. At some point
this will allow us to have per-endpoint locks which
will, in turn, let us queue requests to separate
endpoints in parallel.

Because of this change our debugfs interface and io
accessors need to be changed accordingly.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/core.h|  13 +++-
 drivers/usb/dwc3/debugfs.c | 190 ++---
 drivers/usb/dwc3/ep0.c |   4 +-
 drivers/usb/dwc3/gadget.c  |  16 ++--
 drivers/usb/dwc3/gadget.h  |   4 +-
 drivers/usb/dwc3/io.h  |   7 +-
 6 files changed, 78 insertions(+), 156 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 32e8262f5b2e..fb0f9d94f1b2 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -138,10 +138,12 @@
 #define DWC3_DGCMDPAR  0xc710
 #define DWC3_DGCMD 0xc714
 #define DWC3_DALEPENA  0xc720
-#define DWC3_DEPCMDPAR2(n) (0xc800 + (n * 0x10))
-#define DWC3_DEPCMDPAR1(n) (0xc804 + (n * 0x10))
-#define DWC3_DEPCMDPAR0(n) (0xc808 + (n * 0x10))
-#define DWC3_DEPCMD(n) (0xc80c + (n * 0x10))
+
+#define DWC3_DEP_BASE(n)   (0xc800 + (n * 0x10))
+#define DWC3_DEPCMDPAR20x00
+#define DWC3_DEPCMDPAR10x04
+#define DWC3_DEPCMDPAR00x08
+#define DWC3_DEPCMD0x0c
 
 /* OTG Registers */
 #define DWC3_OCFG  0xcc00
@@ -479,6 +481,7 @@ struct dwc3_event_buffer {
  * @endpoint: usb endpoint
  * @pending_list: list of pending requests for this endpoint
  * @started_list: list of started requests on this endpoint
+ * @regs: pointer to first endpoint register
  * @trb_pool: array of transaction buffers
  * @trb_pool_dma: dma address of @trb_pool
  * @trb_enqueue: enqueue 'pointer' into TRB array
@@ -500,6 +503,8 @@ struct dwc3_ep {
struct list_headpending_list;
struct list_headstarted_list;
 
+   void __iomem*regs;
+
struct dwc3_trb *trb_pool;
dma_addr_t  trb_pool_dma;
const struct usb_ss_ep_comp_descriptor *comp_desc;
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index b1dd3c6d7ef7..89c26e09870c 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -36,9 +36,32 @@
 #define dump_register(nm)  \
 {  \
.name   = __stringify(nm),  \
-   .offset = DWC3_ ##nm - DWC3_GLOBALS_REGS_START, \
+   .offset = DWC3_ ##nm,   \
 }
 
+#define dump_ep_register_set(n)\
+   {   \
+   .name = "DEPCMDPAR2("__stringify(n)")", \
+   .offset = DWC3_DEP_BASE(n) +\
+   DWC3_DEPCMDPAR2,\
+   },  \
+   {   \
+   .name = "DEPCMDPAR1("__stringify(n)")", \
+   .offset = DWC3_DEP_BASE(n) +\
+   DWC3_DEPCMDPAR1,\
+   },  \
+   {   \
+   .name = "DEPCMDPAR0("__stringify(n)")", \
+   .offset = DWC3_DEP_BASE(n) +\
+   DWC3_DEPCMDPAR0,\
+   },  \
+   {   \
+   .name = "DEPCMD("__stringify(n)")", \
+   .offset = DWC3_DEP_BASE(n) +\
+   DWC3_DEPCMD,\
+   }
+
+
 static const struct debugfs_reg32 dwc3_regs[] = {
dump_register(GSBUSCFG0),
dump_register(GSBUSCFG1),
@@ -218,137 +241,38 @@ static const struct debugfs_reg32 dwc3_regs[] = {
dump_register(DGCMD),
dump_register(DALEPENA),
 
-   dump_register(DEPCMDPAR2(0)),
-   dump_register(DEPCMDPAR2(1)),
-   dump_register(DEPCMDPAR2(2)),
-   dump_register(DEPCMDPAR2(3)),
-   dump_register(DEPCMDPAR2(4)),
-   dump_register(DEPCMDPAR2(5)),
-   dump_register(DEPCMDPAR2(6)),
-   dump_register(DEPCMDPAR2(7)),
-   dump_register(DEPCMDPAR2(8)),
-   dump_register(DEPCMDPAR2(9)),
-   dump_register(DEPCMDPAR2(10)),
-   dump_register(DEPCMDPAR2(11)),
-   dump_register(DEPCMDPAR2(12)),
-   dump_register(DEPCMDPAR2(13)),
-   dump_register(DEPCMDPAR2(14)),
-   dump_register(DEPCMDPAR2(15)),
-   dump_register(DEPCMDPAR2(16)),
-   dump_register(DEPCMDPAR2(17)),
-   dump_register(DEPCMDPAR2(18)),
-   dump_register(DEPCMDPAR2(19)),
-   dump_register(DEPCMDPAR2(20)),
-   dump_register(DEPCMDPAR2(21)),
-   dump_regis

[PATCH 00/22] usb: dwc3 / f_mass_storage: misc changes

[Felipe Balbi]

Hi folks,

these last couple weeks I've been working on further
improving dwc3 throughput. Also worked a little on
suspend/resume.

The result of this work is here. Note that dwc3's
transfer handling is a lot simpler and easier to
follow. I can reach up to 45MiB/sec in HS with Mass
Storage gadget (see details below) and up to
370MiB/sec in SS. I measured on single setup here,
so numbers should be taken with a grain of salt.

For the test, I had CPU doing "nothing". Only the
mass storage gadget was running. I had 96 Mass
Storage requests, each with 16kiB buffer size. Also
set max_sectors to 2048 on HS (via sysfs). With
default 240 max_sectors, I couldn't go past
37MiB/sec.

Anyway, there's a lot of refactoring and I really
think people should test on their dwc3-based
platforms to make sure I'm not regressing
anybody. These patches will be queue for v4.8 and
I'll keep them soaking on linux-next for as long as
I can (I want these patches soaking for at least 5
weeks there).

cheers

Felipe Balbi (22):
  usb: dwc3: gadget: re-factor ->udc_start and ->udc_stop
  usb: dwc3: gadget: fix gadget suspend/resume
  usb: dwc3: core: get rid of DWC3_PM_OPS macro
  usb: dwc3: gadget: prepare TRBs on update transfers too
  usb: dwc3: gadget: simplify __dwc3_gadget_kick_transfer()
  usb: dwc3: gadget: rely on sg_is_last() and list_is_last()
  usb: dwc3: gadget: remove udelay(1) when sending ep cmds
  usb: dwc3: gadget: return 0 if we try to Wakeup in superspeed
  usb: dwc3: gadget: split __dwc3_gadget_kick_transfer()
  usb: dwc3: gadget: initialize NUMP based on RxFIFO Size
  usb: dwc3: gadget: clear LST from previous TRB on Update Transfer
  usb: dwc3: gadget: start bulk endpoints more frequently
  usb: dwc3: gadget: pass dep as argument to endpoint command
  usb: dwc3: gadget: add a pointer to endpoint registers
  usb: dwc3: gadget: switch over to spin_lock()
  usb: dwc3: core: move fladj to dwc3 structure
  usb: dwc3: core: re-factor init and exit paths
  usb: dwc3: core: simplify suspend/resume operations
  usb: dwc3: gadget: hold gadget IRQ in dwc->irq_gadget
  usb: dwc3: gadget: only resume USB2 PHY in <=HIGHSPEED
  usb: gadget: storage: get rid of fsg_num_buffers_validate()
  usb: gadget: storage: increase maximum storage num buffers

 drivers/usb/dwc3/core.c  | 199 --
 drivers/usb/dwc3/core.h  |  41 ++-
 drivers/usb/dwc3/debugfs.c   | 190 +++---
 drivers/usb/dwc3/ep0.c   |   9 +-
 drivers/usb/dwc3/gadget.c| 525 ++-
 drivers/usb/dwc3/gadget.h|   4 +-
 drivers/usb/dwc3/io.h|   7 +-
 drivers/usb/gadget/Kconfig   |   2 +-
 drivers/usb/gadget/function/f_mass_storage.c |  18 +-
 9 files changed, 435 insertions(+), 560 deletions(-)

-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 08/22] usb: dwc3: gadget: return 0 if we try to Wakeup in superspeed

[Felipe Balbi]

Instead of returning -EINVAL when someone calls
__dwc3_gadget_wakeup() in speeds > highspeed, let's
return 0. There are no problems for the driver for
calling it in superspeed as we cleanly just return.

This avoids an annoying WARN_ONCE() always
triggering during superspeed enumeration with LPM
enabled.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index db7bbbd0cecd..eca131f0be59 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1410,7 +1410,7 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
if ((speed == DWC3_DSTS_SUPERSPEED) ||
(speed == DWC3_DSTS_SUPERSPEED_PLUS)) {
dwc3_trace(trace_dwc3_gadget, "no wakeup on SuperSpeed\n");
-   return -EINVAL;
+   return 0;
}
 
link_state = DWC3_DSTS_USBLNKST(reg);
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 13/22] usb: dwc3: gadget: pass dep as argument to endpoint command

[Felipe Balbi]

In all call sites of dwc3_send_gadget_ep_cmd() we
already had a valid dep pointer, so instead of
passing dwc and dep->number, which would be used to
fetch the same pointer we already had, just pass dep
directly.

In other words, we're changing:

struct dwc3_ep *dep = dwc[dep->number];

to just passing struct dwc3_ep *dep as argument.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/core.h   |  8 
 drivers/usb/dwc3/ep0.c|  5 ++---
 drivers/usb/dwc3/gadget.c | 33 +
 3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 761dc34885be..32e8262f5b2e 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1104,8 +1104,8 @@ void dwc3_gadget_exit(struct dwc3 *dwc);
 int dwc3_gadget_set_test_mode(struct dwc3 *dwc, int mode);
 int dwc3_gadget_get_link_state(struct dwc3 *dwc);
 int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum dwc3_link_state state);
-int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
-   unsigned cmd, struct dwc3_gadget_ep_cmd_params *params);
+int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
+   struct dwc3_gadget_ep_cmd_params *params);
 int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned cmd, u32 
param);
 #else
 static inline int dwc3_gadget_init(struct dwc3 *dwc)
@@ -1120,8 +1120,8 @@ static inline int dwc3_gadget_set_link_state(struct dwc3 
*dwc,
enum dwc3_link_state state)
 { return 0; }
 
-static inline int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
-   unsigned cmd, struct dwc3_gadget_ep_cmd_params *params)
+static inline int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
+   struct dwc3_gadget_ep_cmd_params *params)
 { return 0; }
 static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
int cmd, u32 param)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 51b52a79dfec..e08150a7cf8f 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -98,8 +98,7 @@ static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, 
dma_addr_t buf_dma,
 
trace_dwc3_prepare_trb(dep, trb);
 
-   ret = dwc3_send_gadget_ep_cmd(dwc, dep->number,
-   DWC3_DEPCMD_STARTTRANSFER, ¶ms);
+   ret = dwc3_send_gadget_ep_cmd(dep, DWC3_DEPCMD_STARTTRANSFER, ¶ms);
if (ret < 0) {
dwc3_trace(trace_dwc3_ep0, "%s STARTTRANSFER failed",
dep->name);
@@ -1058,7 +1057,7 @@ static void dwc3_ep0_end_control_data(struct dwc3 *dwc, 
struct dwc3_ep *dep)
cmd |= DWC3_DEPCMD_CMDIOC;
cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
memset(¶ms, 0, sizeof(params));
-   ret = dwc3_send_gadget_ep_cmd(dwc, dep->number, cmd, ¶ms);
+   ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
WARN_ON_ONCE(ret);
dep->resource_index = 0;
 }
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index f9f7fe2c1e14..97e3ecc716de 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -238,16 +238,18 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, 
unsigned cmd, u32 param)
 
 static int __dwc3_gadget_wakeup(struct dwc3 *dwc);
 
-int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
-   unsigned cmd, struct dwc3_gadget_ep_cmd_params *params)
+int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
+   struct dwc3_gadget_ep_cmd_params *params)
 {
-   struct dwc3_ep  *dep = dwc->eps[ep];
+   struct dwc3 *dwc = dep->dwc;
u32 timeout = 500;
u32 reg;
 
int susphy = false;
int ret = -EINVAL;
 
+   unsignedep = dep->number;
+
trace_dwc3_gadget_ep_cmd(dep, cmd, params);
 
/*
@@ -430,7 +432,7 @@ static int dwc3_gadget_start_config(struct dwc3 *dwc, 
struct dwc3_ep *dep)
memset(¶ms, 0x00, sizeof(params));
cmd = DWC3_DEPCMD_DEPSTARTCFG;
 
-   ret = dwc3_send_gadget_ep_cmd(dwc, 0, cmd, ¶ms);
+   ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
if (ret)
return ret;
 
@@ -506,8 +508,7 @@ static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, 
struct dwc3_ep *dep,
dep->interval = 1 << (desc->bInterval - 1);
}
 
-   return dwc3_send_gadget_ep_cmd(dwc, dep->number,
-   DWC3_DEPCMD_SETEPCONFIG, ¶ms);
+   return dwc3_send_gadget_ep_cmd(dep, DWC3_DEPCMD_SETEPCONFIG, ¶ms);
 }
 
 static int dwc3_gadget_set_xfer_resource(struct dwc3 *dwc, struct dwc3_ep *dep)
@@ -518,8 +519,8 @@ static int dwc3_gadget_set_xfer_resource(struct dwc3 *dwc, 
struct dwc3_ep *dep)
 
params.param0 = DWC3_DEPXFERCFG_NUM_XFER_RE

[PATCH 10/22] usb: dwc3: gadget: initialize NUMP based on RxFIFO Size

[Felipe Balbi]

Instead of using burst size to configure NUMP, we
should be using RxFIFO Size instead. DWC3 is smart
enough to know that it shouldn't burst in case burst
size is 0.

Reported-by: John Youn 
Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/core.h   | 12 +++
 drivers/usb/dwc3/gadget.c | 53 +--
 2 files changed, 54 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 9366351ac136..761dc34885be 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -231,6 +231,14 @@
 #define DWC3_GEVNTSIZ_INTMASK  (1 << 31)
 #define DWC3_GEVNTSIZ_SIZE(n)  ((n) & 0x)
 
+/* Global HWPARAMS0 Register */
+#define DWC3_GHWPARAMS0_USB3_MODE(n)   ((n) & 0x3)
+#define DWC3_GHWPARAMS0_MBUS_TYPE(n)   (((n) >> 3) & 0x7)
+#define DWC3_GHWPARAMS0_SBUS_TYPE(n)   (((n) >> 6) & 0x3)
+#define DWC3_GHWPARAMS0_MDWIDTH(n) (((n) >> 8) & 0xff)
+#define DWC3_GHWPARAMS0_SDWIDTH(n) (((n) >> 16) & 0xff)
+#define DWC3_GHWPARAMS0_AWIDTH(n)  (((n) >> 24) & 0xff)
+
 /* Global HWPARAMS1 Register */
 #define DWC3_GHWPARAMS1_EN_PWROPT(n)   (((n) & (3 << 24)) >> 24)
 #define DWC3_GHWPARAMS1_EN_PWROPT_NO   0
@@ -260,6 +268,10 @@
 /* Global HWPARAMS6 Register */
 #define DWC3_GHWPARAMS6_EN_FPGA(1 << 7)
 
+/* Global HWPARAMS7 Register */
+#define DWC3_GHWPARAMS7_RAM1_DEPTH(n)  ((n) & 0x)
+#define DWC3_GHWPARAMS7_RAM2_DEPTH(n)  (((n) >> 16) & 0x)
+
 /* Global Frame Length Adjustment Register */
 #define DWC3_GFLADJ_30MHZ_SDBND_SEL(1 << 7)
 #define DWC3_GFLADJ_30MHZ_MASK 0x3f
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index adf743bca36f..4a3ed4cbe39b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -463,17 +463,6 @@ static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, 
struct dwc3_ep *dep,
/* Burst size is only needed in SuperSpeed mode */
if (dwc->gadget.speed >= USB_SPEED_SUPER) {
u32 burst = dep->endpoint.maxburst;
-   u32 nump;
-   u32 reg;
-
-   /* update NumP */
-   reg = dwc3_readl(dwc->regs, DWC3_DCFG);
-   nump = DWC3_DCFG_NUMP(reg);
-   nump = max(nump, burst);
-   reg &= ~DWC3_DCFG_NUMP_MASK;
-   reg |= nump << DWC3_DCFG_NUMP_SHIFT;
-   dwc3_writel(dwc->regs, DWC3_DCFG, reg);
-
params.param0 |= DWC3_DEPCFG_BURST_SIZE(burst - 1);
}
 
@@ -1589,6 +1578,46 @@ static void dwc3_gadget_disable_irq(struct dwc3 *dwc)
 static irqreturn_t dwc3_interrupt(int irq, void *_dwc);
 static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc);
 
+/**
+ * dwc3_gadget_setup_nump - Calculate and initialize NUMP field of DCFG
+ * dwc: pointer to our context structure
+ *
+ * The following looks like complex but it's actually very simple. In order to
+ * calculate the number of packets we can burst at once on OUT transfers, we're
+ * gonna use RxFIFO size.
+ *
+ * To calculate RxFIFO size we need two numbers:
+ * MDWIDTH = size, in bits, of the internal memory bus
+ * RAM2_DEPTH = depth, in MDWIDTH, of internal RAM2 (where RxFIFO sits)
+ *
+ * Given these two numbers, the formula is simple:
+ *
+ * RxFIFO Size = (RAM2_DEPTH * MDWIDTH / 8) - 24 - 16;
+ *
+ * 24 bytes is for 3x SETUP packets
+ * 16 bytes is a clock domain crossing tolerance
+ *
+ * Given RxFIFO Size, NUMP = RxFIFOSize / 1024;
+ */
+static void dwc3_gadget_setup_nump(struct dwc3 *dwc)
+{
+   u32 ram2_depth;
+   u32 mdwidth;
+   u32 nump;
+   u32 reg;
+
+   ram2_depth = DWC3_GHWPARAMS7_RAM2_DEPTH(dwc->hwparams.hwparams7);
+   mdwidth = DWC3_GHWPARAMS0_MDWIDTH(dwc->hwparams.hwparams0);
+
+   nump = ((ram2_depth * mdwidth / 8) - 24 - 16) / 1024;
+
+   /* update NumP */
+   reg = dwc3_readl(dwc->regs, DWC3_DCFG);
+   reg &= ~DWC3_DCFG_NUMP_MASK;
+   reg |= nump << DWC3_DCFG_NUMP_SHIFT;
+   dwc3_writel(dwc->regs, DWC3_DCFG, reg);
+}
+
 static int __dwc3_gadget_start(struct dwc3 *dwc)
 {
struct dwc3_ep  *dep;
@@ -1649,6 +1678,8 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
reg &= ~DWC3_GRXTHRCFG_PKTCNTSEL;
dwc3_writel(dwc->regs, DWC3_GRXTHRCFG, reg);
 
+   dwc3_gadget_setup_nump(dwc);
+
/* Start with SuperSpeed Default */
dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
 
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by other

[PATCH 07/22] usb: dwc3: gadget: remove udelay(1) when sending ep cmds

[Felipe Balbi]

When we send an endpoint command, we want that to
complete as soon as possible, so let's remove the
unnecessary udelay(1) call.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index fab2d166d844..db7bbbd0cecd 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -334,8 +334,6 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
ret = -ETIMEDOUT;
break;
}
-
-   udelay(1);
} while (1);
 
if (unlikely(susphy)) {
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 03/22] usb: dwc3: core: get rid of DWC3_PM_OPS macro

[Felipe Balbi]

that macro is unnecessary and just adds pointless
obfuscation. Let's remove it.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/core.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a590cd225bb7..245f4ff6ae16 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1209,16 +1209,12 @@ err_usb2phy_power:
 
return ret;
 }
+#endif /* CONFIG_PM_SLEEP */
 
 static const struct dev_pm_ops dwc3_dev_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
 };
 
-#define DWC3_PM_OPS&(dwc3_dev_pm_ops)
-#else
-#define DWC3_PM_OPSNULL
-#endif
-
 #ifdef CONFIG_OF
 static const struct of_device_id of_dwc3_match[] = {
{
@@ -1250,7 +1246,7 @@ static struct platform_driver dwc3_driver = {
.name   = "dwc3",
.of_match_table = of_match_ptr(of_dwc3_match),
.acpi_match_table = ACPI_PTR(dwc3_acpi_match),
-   .pm = DWC3_PM_OPS,
+   .pm = &dwc3_dev_pm_ops,
},
 };
 
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 20/22] usb: dwc3: gadget: only resume USB2 PHY in <=HIGHSPEED

[Felipe Balbi]

As a micro-power optimization, let's only resume the
USB2 PHY if we're working on <=HIGHSPEED. If we're
gonna work on SUPERSPEED or SUPERSPEED+, there's no
point in resuming the USB2 PHY.

Fixes: 2b0f11df84bb ("usb: dwc3: gadget: clear SUSPHY bit before ep cmds")
Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 2a53d318f581..68d13f107b2f 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -258,11 +258,13 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned 
cmd,
 * We will also set SUSPHY bit to what it was before returning as stated
 * by the same section on Synopsys databook.
 */
-   reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-   if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
-   susphy = true;
-   reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
-   dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+   if (dwc->gadget.speed <= USB_SPEED_HIGH) {
+   reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+   if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
+   susphy = true;
+   reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
+   dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+   }
}
 
if (cmd == DWC3_DEPCMD_STARTTRANSFER) {
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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] dwc3: gadget: Defer starting the gadget device until gadget is power on

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>>> Baolin Wang  writes:
>>>>>>>> Make sense.
>>>>>>>
>>>>>>> cool, if you wanna work on it, let me know and I can give some details
>>>>>>> of what I have in mind.
>>>>>>
>>>>>> Could you explain details to me, and I wanna continue to optimize the
>>>>>> power management things. Thanks.
>>>>>
>>>>> I have it half-way done. Have a look at my dwc3-fix-suspend branch on
>>>>> k.org. I haven't sent because I'm not getting a PME event. Can you test
>>>>> on your end and let me know what happens?
>
> I applied some patches (showing below) about suspend/resume at your
> dwc3-fix-suspend branch with my glue layer runtime_* callbacks on my
> testing platform.
> usb: dwc3: implement runtime PM
> usb: dwc3: core: simplify suspend/resume operations
> usb: dwc3: core: re-factor init and exit paths
> usb: dwc3: core: get rid of DWC3_PM_OPS macro
> usb: dwc3: gadget: fix gadget suspend/resume
> usb: dwc3: gadget: re-factor ->udc_start and ->udc_stop
>
> Then I tested the cable connect/disconnect action to see the dwc core
> resume/suspend. It looks work well on my platform, except that I did
> some extra 2 modifications like below:
> (1)
> @@ -1485,16 +1490,11 @@ static int dwc3_gadget_run_stop(struct dwc3
> *dwc, int is_on, int suspend)
>  {
> u32 reg;
> u32 timeout = 500, i;
>
> +   if (pm_runtime_suspended(dwc->dev))
> +   return 0;
>
> (2)
> @@ -1748,15 +1754,25 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>  * even though host mode might be active. Don't actually perform
>  * device-specific initialization until device mode is activated.
>  */
>
> +   if (pm_runtime_suspended(dwc->dev)) {
> +   spin_unlock_irqrestore(&dwc->lock, flags);
> +   return 0;
> +   }
>
> +   ret = __dwc3_gadget_start(dwc);
> +   if (ret)
> +   goto err1;
>
> So I think the dwc3 core can enter suspend mode before gadget function
> is ready to call the 'usb_gadget_udc_start()' and
> 'usb_udc_connect_control()', then if the dwc3 core has entered
> suspended mode, we need to return success when starting the gadget,
> and leave the gadget starting action from gadget resume. What do you
> think about that? Thanks.

Well, if this makes it work properly. Then, yeah; looks okay to me. I'll
add this to the patch introducing runtime PM.

Thanks a lot for testing on your side :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: f_fs: report error if excess data received

[Felipe Balbi]

Hi,

Michal Nazarewicz  writes:
> On Tue, May 17 2016, Changbin Du wrote:
>>> There appears to be no kfifo support for iov_iter though, so I just went
>>> with a simple buffer.
>>> 
>>> I haven’t looked at the patch too carefully so this is an RFC rather
>>> than an actual patch at this point.  It does compile at least.
>>> 
>>> Regardless, the more I thin about it, the more I’m under the impression
>>> that the whole rounding up in f_fs was a mistake.  And the more I’m
>>> leaning towards ignoring the excess data set by the host.
>>> 
>>> -- >8 --
>>> Subject: usb: gadget: f_fs: buffer data from ‘oversized’ OUT requests
>>> 
>>> f_fs rounds up read(2) requests to a multiple of a max packet size
>>> which means that host may provide more data than user has space for.
>>> So far, the excess data has been silently ignored.
>>> 
>>> This introduces a buffer for a tail of such requests so that they are
>>> returned on next read instead of being ignored.
>>> 
>>
>> Congratulations finally reach an agreement,
>
> To be honest, if it was up to me, I would rip request size rounding up
> out of the code.

we've been through this before. This needs to be done at the gadget
layer. Gadget driver can over-allocate ahead of time if
gadget->quirk_ep_out_aligned_size is true, then we avoid memcpy() at the
UDC driver level.

>> thanks Alan Stern and Michal.
>> Here just have a comment - the buffered data need be dropped when the
>> epfile is closed, because it means the session is terminated.
>
> I blame that on sleep deprivation.  Another issue is what to do when
> endpoint is disabled.  Should the buffer be cleared as soon as the
> endpoint is disabled?  Or maybe when the endpoint is enabled again?  Or
> maybe it should never be cleared?
>
> If the buffer is cleared when endpoint is disabled, we again silently
> drop data.  On the other hand, if we don’t do that, read() on the
> endpoint will may succeed even if the configuration is disabled which
> may be surprising for users.

tough decision... but seems like clearing the buffer as soon as ep is
disabled is the way to go.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>>> @@ -1485,16 +1490,11 @@ static int dwc3_gadget_run_stop(struct dwc3
>>> *dwc, int is_on, int suspend)
>>>  {
>>> u32 reg;
>>> u32 timeout = 500, i;
>>>
>>> +   if (pm_runtime_suspended(dwc->dev))
>>> +   return 0;
>>>
>>> (2)
>>> @@ -1748,15 +1754,25 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>>>  * even though host mode might be active. Don't actually perform
>>>  * device-specific initialization until device mode is activated.
>>>  */
>>>
>>> +   if (pm_runtime_suspended(dwc->dev)) {
>>> +   spin_unlock_irqrestore(&dwc->lock, flags);
>>> +   return 0;
>>> +   }
>>>
>>> +   ret = __dwc3_gadget_start(dwc);
>>> +   if (ret)
>>> +   goto err1;
>>>
>>> So I think the dwc3 core can enter suspend mode before gadget function
>>> is ready to call the 'usb_gadget_udc_start()' and
>>> 'usb_udc_connect_control()', then if the dwc3 core has entered
>>> suspended mode, we need to return success when starting the gadget,
>>> and leave the gadget starting action from gadget resume. What do you
>>> think about that? Thanks.
>>
>> Well, if this makes it work properly. Then, yeah; looks okay to me. I'll
>> add this to the patch introducing runtime PM.
>
> OK.

I've updated the branch with slightly modified version of your
changes. Can you test again just to make sure it still works ?

Basically, here's what I did:

on dwc3_gadget_start:

-   __dwc3_gadget_start(dwc);
+   if (pm_runtime_active(dwc->dev))
+   __dwc3_gadget_start(dwc);
+

on run_stop, I kept the same thing.

you just need to replace "usb: dwc3: implement runtime PM" with the new
version from my branch.

cheers

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>>>>> @@ -1748,15 +1754,25 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>>>>>  * even though host mode might be active. Don't actually perform
>>>>>  * device-specific initialization until device mode is activated.
>>>>>  */
>>>>>
>>>>> +   if (pm_runtime_suspended(dwc->dev)) {
>>>>> +   spin_unlock_irqrestore(&dwc->lock, flags);
>>>>> +   return 0;
>>>>> +   }
>>>>>
>>>>> +   ret = __dwc3_gadget_start(dwc);
>>>>> +   if (ret)
>>>>> +   goto err1;
>>>>>
>>>>> So I think the dwc3 core can enter suspend mode before gadget function
>>>>> is ready to call the 'usb_gadget_udc_start()' and
>>>>> 'usb_udc_connect_control()', then if the dwc3 core has entered
>>>>> suspended mode, we need to return success when starting the gadget,
>>>>> and leave the gadget starting action from gadget resume. What do you
>>>>> think about that? Thanks.
>>>>
>>>> Well, if this makes it work properly. Then, yeah; looks okay to me. I'll
>>>> add this to the patch introducing runtime PM.
>>>
>>> OK.
>>
>> I've updated the branch with slightly modified version of your
>> changes. Can you test again just to make sure it still works ?
>>
>> Basically, here's what I did:
>>
>> on dwc3_gadget_start:
>>
>> -   __dwc3_gadget_start(dwc);
>> +   if (pm_runtime_active(dwc->dev))
>> +   __dwc3_gadget_start(dwc);
>> +
>
> Great.
>
>>
>> on run_stop, I kept the same thing.
>>
>> you just need to replace "usb: dwc3: implement runtime PM" with the new
>> version from my branch.
>
> Yeah, it can work well on my platform with your new patch.

cool, thanks again :-) I'll drop my "not for merging note" and add your
"Tested-by" (assuming it's okay for you that I do it).

cheers

-- 
balbi


signature.asc
Description: PGP signature

[PATCH 1/2] usb: dwc3: fix for the isoc transfer EP_BUSY flag

[Felipe Balbi]

From: Konrad Leszczynski 

commit f3af36511e60 ("usb: dwc3: gadget: always
enable IOC on bulk/interrupt transfers") ended up
regressing Isochronous endpoints by clearing
DWC3_EP_BUSY flag too early, which resulted in
choppy audio playback over USB.

Fix that by partially reverting original commit and
making sure that we check for isochronous endpoints.

Fixes: f3af36511e60 ("usb: dwc3: gadget: always enable IOC
on bulk/interrupt transfers")
Cc: 
Signed-off-by: Konrad Leszczynski 
Signed-off-by: Rafal Redzimski 
Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 68d13f107b2f..8c06b8d64144 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2032,6 +2032,10 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, 
struct dwc3_ep *dep,
return 1;
}
 
+   if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
+   if ((event->status & DEPEVT_STATUS_IOC) &&
+   (trb->ctrl & DWC3_TRB_CTRL_IOC))
+   return 0;
return 1;
 }
 
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
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 2/2] usb: dwc3: gadget: fix for possible endpoint disable race

[Felipe Balbi]

when we call dwc3_gadget_giveback(), we end up
releasing our controller's lock. Another thread
could get scheduled and disable the endpoint,
subsequently setting dep->endpoint.desc to NULL.

In that case, we would end up dereferencing a NULL
pointer which would result in a Kernel Oops. Let's
avoid the problem by simply returning early if we
have a NULL descriptor.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8c06b8d64144..fb053a307a8b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2015,6 +2015,14 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, 
struct dwc3_ep *dep,
break;
} while (1);
 
+   /*
+* Our endpoint might get disabled by another thread during
+* dwc3_gadget_giveback(). If that happens, we're just gonna return 1
+* early on so DWC3_EP_BUSY flag gets cleared
+*/
+   if (!dep->endpoint.desc)
+   return 1;
+
if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
list_empty(&dep->started_list)) {
if (list_empty(&dep->pending_list)) {
@@ -2081,6 +2089,14 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 
*dwc,
dwc->u1u2 = 0;
}
 
+   /*
+* Our endpoint might get disabled by another thread during
+* dwc3_gadget_giveback(). If that happens, we're just gonna return 1
+* early on so DWC3_EP_BUSY flag gets cleared
+*/
+   if (!dep->endpoint.desc)
+   return;
+
if (!usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
int ret;
 
-- 
2.8.2

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

[Felipe Balbi]

Hi,

Paul Zimmerman  writes:
> 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 = &dep->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.

oh that's right. Just found it on section 8.2.3.2. I'll drop this patch
from the queue. Thanks for the note :-)

Hope you're doing ok, Paul

cheers

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 10/22] usb: dwc3: gadget: initialize NUMP based on RxFIFO Size

[Felipe Balbi]

Hi,

John Youn  writes:
>> @@ -1589,6 +1578,46 @@ static void dwc3_gadget_disable_irq(struct dwc3 *dwc)
>>  static irqreturn_t dwc3_interrupt(int irq, void *_dwc);
>>  static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc);
>>  
>> +/**
>> + * dwc3_gadget_setup_nump - Calculate and initialize NUMP field of DCFG
>> + * dwc: pointer to our context structure
>> + *
>> + * The following looks like complex but it's actually very simple. In order 
>> to
>> + * calculate the number of packets we can burst at once on OUT transfers, 
>> we're
>> + * gonna use RxFIFO size.
>> + *
>> + * To calculate RxFIFO size we need two numbers:
>> + * MDWIDTH = size, in bits, of the internal memory bus
>> + * RAM2_DEPTH = depth, in MDWIDTH, of internal RAM2 (where RxFIFO sits)
>> + *
>> + * Given these two numbers, the formula is simple:
>> + *
>> + * RxFIFO Size = (RAM2_DEPTH * MDWIDTH / 8) - 24 - 16;
>> + *
>> + * 24 bytes is for 3x SETUP packets
>> + * 16 bytes is a clock domain crossing tolerance
>> + *
>> + * Given RxFIFO Size, NUMP = RxFIFOSize / 1024;
>> + */
>> +static void dwc3_gadget_setup_nump(struct dwc3 *dwc)
>> +{
>> +u32 ram2_depth;
>> +u32 mdwidth;
>> +u32 nump;
>> +u32 reg;
>> +
>> +ram2_depth = DWC3_GHWPARAMS7_RAM2_DEPTH(dwc->hwparams.hwparams7);
>> +mdwidth = DWC3_GHWPARAMS0_MDWIDTH(dwc->hwparams.hwparams0);
>> +
>> +nump = ((ram2_depth * mdwidth / 8) - 24 - 16) / 1024;
>
> This value should be capped to 16.

I'll add a min() here.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/2] usb: dwc3: gadget: Simplify skipping of link TRBs

[Felipe Balbi]

Hi,

John Youn  writes:
> Make the skipping of the link TRBS build-in to the increment and
> decrement operations. This simplifies the code wherever we increment and
> decrement and ensures that we never end up pointing to a link trb.
>
> Signed-off-by: John Youn 

I had this in my TODO for today :-) Glad I don't have to do it :-p

> ---
>  drivers/usb/dwc3/gadget.c | 26 +-
>  1 file changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 8629b60..3eaef22 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -145,21 +145,23 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum 
> dwc3_link_state state)
>   return -ETIMEDOUT;
>  }
>  
> +static bool dwc3_ep_is_last_trb(unsigned int index)
> +{
> + return index == (DWC3_TRB_NUM - 1);
> +}
> +
>  static void dwc3_ep_inc_enq(struct dwc3_ep *dep)
>  {
>   dep->trb_enqueue++;
> - dep->trb_enqueue %= DWC3_TRB_NUM;

I would really like to keep the comment about skipping link TRB here.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/2] usb: dwc3: gadget: Various fixes to trbs_left calculation

[Felipe Balbi]

Hi,

John Youn  writes:
> This patch fixes up some issues related to the trb_left calculation.
>
> This calculation sometimes included the link trb slot in the trbs_left
> and sometimes didn't.

good catch. But this patch seems like it can be broken into smaller
pieces. See below

> In the case where the dequeue < enqueue, this number does not include
> the link trb and should be used as-is. Otherwise it will include the
> link_trb and should be decremented to reflect the actual amount of free
> trb slots. The fixed calculation will be the proper amount of free TRB
> slots left, taking into account whether a link TRB takes up one of the
> slots.

this is one fix :-)

> When checking for full or empty where enqueue == dequeue, the pointer
> may be 0. In that case previous TRB is the one before the link TRB. Use
> that to check the HWO bit to see if we are full or empty.

if enqueue == dequeue, we could be anywhere in the ring. So previous TRB
might not be the one before the link. Care to further explain this case?

It's also a separate fix, btw.

> In case DWC3_TRB_NUM is ever set lower than 256, mod trbs_left result by
> DWC3_TRB_NUM.

I don't get this. Where did we have % 256? I can only see % DWC3_TRB_NUM.

> In dwc3_prepare_trbs, if trbs_left is 0, do nothing and return early.

another fix.

> When an EP is enabled, initialized the TRB pool to clean up any stale
> data. When the previous TRB was not properly cleaned up on disconnect,
> the empty/full checking logic sometimes failed causing the EP to stop
> processing requests.

another fix.

> Signed-off-by: John Youn 
> ---
>  drivers/usb/dwc3/gadget.c | 37 ++---
>  1 file changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3eaef22..a76634b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -561,6 +561,12 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
>   if (usb_endpoint_xfer_control(desc))
>   goto out;
>  
> + /* Initialize the TRB pool */
> + dep->trb_dequeue = 0;
> + dep->trb_enqueue = 0;
> + memset(dep->trb_pool, 0,
> +sizeof(struct dwc3_trb) * DWC3_TRB_NUM);

this is a separate fix. A very good one, actually ;-)

>   /* Link TRB. The HWO bit is never reset */
>   trb_st_hw = &dep->trb_pool[0];
>  
> @@ -849,6 +855,8 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
>  static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
>  {
>   struct dwc3_trb *tmp;
> + u8  tmp_index;
> + u8  trbs_left;
>  
>   /*
>* If enqueue & dequeue are equal than it is either full or empty.
> @@ -858,17 +866,29 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
>* more transfers in our ring.
>*/
>   if (dep->trb_enqueue == dep->trb_dequeue) {
> - /* If we're full, enqueue/dequeue are > 0 */
> - if (dep->trb_enqueue) {
> - tmp = &dep->trb_pool[dep->trb_enqueue - 1];
> - if (tmp->ctrl & DWC3_TRB_CTRL_HWO)
> - return 0;
> - }
> + if (!dep->trb_enqueue)
> + /*
> +  * The TRB just before the zeroth one is the
> +  * one just before the LINK TRB.
> +  */
> + tmp_index = DWC3_TRB_NUM - 2;

this seems wrong. If both enqueue and dequeue are 0, it means our entire
ring is empty, and we can use (by default) 255 TRBs, with one space left
for the link. This DWC3_TRB_NUM - 2 will give me 254, instead of 255.

> + else
> + tmp_index = dep->trb_enqueue - 1;

If enqueue and dequeue are equal, but not 0, then we're anywhere in the
middle of the ring, and this is where we're either full or empty. I'm
using HWO of the previous TRB to figure out if we're full or not.

> + tmp = &dep->trb_pool[tmp_index];
> + if (tmp->ctrl & DWC3_TRB_CTRL_HWO)
> + return 0;
>  
>   return DWC3_TRB_NUM - 1;
>   }
>  
> - return dep->trb_dequeue - dep->trb_enqueue;
> + trbs_left = dep->trb_dequeue - dep->trb_enqueue;
> + trbs_left %= DWC3_TRB_NUM;
> +
> + if (dep->trb_dequeue < dep->trb_enqueue)
> + trbs_left--;

this branch seems correct, but part of a separate patch.

> +
> + return trbs_left;
>  }
>  
>  static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
> @@ -949,6 +969,9 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep, int 
> starting)
>  
>   trbs_left = dwc3_calc_trbs_left(dep);
>  
> + if (!trbs_left)
> + return;

also a separate patch.

-- 
balbi


signature.asc
Description: PGP signature

Re: xhci DWC3 flavor problem

[Felipe Balbi]

Hi João,

Adding Mathias, who's xHCI's maintainer

Joao Pinto  writes:
> Hi Felipe,
>
> I am trying to bring up a DWC USB 3.0 Host with linux (v4.6-rc5)
> running in a ARM64 development board.

Just to be clear, is this Juno with dwc3 in FPGA or do you have dwc3 in ASIC?

> I have implemented the following suggested fix to overcome the DMA
> problem I was having:
>
> https://lkml.org/lkml/2016/3/31/609
>
> I received one interrupt but I am getting cyclic crashes like this:

if they happen all the time, it probably means your command ring is
completely messed up and no command ever completes. Can you share a more
complete snippet of logs?

> [] __switch_to+0xc8/0xd4
> [] __schedule+0x18c/0x5c8
> [] schedule+0x38/0x98
> [] schedule_timeout+0x160/0x1ac
> [] wait_for_common+0xac/0x150
> [] wait_for_completion+0x14/0x1c
> [] xhci_alloc_dev+0xf4/0x2a0
> [] usb_alloc_dev+0x68/0x2cc
> [] hub_event+0x784/0x11f4
> [] process_one_work+0x130/0x2f4
> [] worker_thread+0x54/0x434
> [] kthread+0xd4/0xe8
> [] ret_from_fork+0x10/0x40
>
> Any thoughts about what might be happening?

pasting xhci_alloc_dev here:

> int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
> {
>   struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>   unsigned long flags;
>   int ret, slot_id;
>   struct xhci_command *command;
>
>   command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
>   if (!command)
>   return 0;

we allocate a command fine

>   /* xhci->slot_id and xhci->addr_dev are not thread-safe */
>   mutex_lock(&xhci->mutex);
>   spin_lock_irqsave(&xhci->lock, flags);
>   command->completion = &xhci->addr_dev;
>   ret = xhci_queue_slot_control(xhci, command, TRB_ENABLE_SLOT, 0);

queue it to the command ring

>   if (ret) {
>   spin_unlock_irqrestore(&xhci->lock, flags);
>   mutex_unlock(&xhci->mutex);
>   xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
>   kfree(command);
>   return 0;
>   }
>   xhci_ring_cmd_db(xhci);
>   spin_unlock_irqrestore(&xhci->lock, flags);
>
>   wait_for_completion(command->completion);

but the command never completes. I wonder if your command doorbell
completed before wait_for_completion() was called, or if it didn't
complete at all.

Can you enable XHCI debugging logs and try again? (Mathias, what was the
easy trick to enable all XHCI debugging logs?)

-- 
balbi


signature.asc
Description: PGP signature

Re: xhci DWC3 flavor problem

[Felipe Balbi]

Hi,

Joao Pinto  writes:
> Hi Felipe and Mathias,
>
> Sending kernel log with extra xhci messages in attachment!
> Thanks you for the help!

yeah, no problems. So here's the interesting part:

   *INSERTING PEN DRIVE

   # xhci-hcd xhci-hcd.0.auto: Port Status Change Event for port 1
   xhci-hcd xhci-hcd.0.auto: resume root hub
   xhci-hcd xhci-hcd.0.auto: handle_port_status: starting port polling.
   xhci-hcd xhci-hcd.0.auto: get port status, actual port 0 status  = 0x206e1
   xhci-hcd xhci-hcd.0.auto: Get port status returned 0x10101
   xhci-hcd xhci-hcd.0.auto: clear port connect change, actual port 0 status  = 
0x6e1
   xhci-hcd xhci-hcd.0.auto: xhci_hub_status_data: stopping port polling.
   xhci-hcd xhci-hcd.0.auto: get port status, actual port 0 status  = 0x6e1
   xhci-hcd xhci-hcd.0.auto: Get port status returned 0x101
   xhci-hcd xhci-hcd.0.auto: xhci_hub_status_data: stopping port polling.
   xhci-hcd xhci-hcd.0.auto: // Ding dong!
   xhci-hcd xhci-hcd.0.auto: Command timeout
   xhci-hcd xhci-hcd.0.auto: Abort command ring

Note that we really did get a command timeout. Can you add a little
extra debugging to try and figure out why that command failed?

-- 
balbi


signature.asc
Description: PGP signature

Re: xhci DWC3 flavor problem

[Felipe Balbi]

Hi,

Joao Pinto  writes:
> Hi Felipe,
>
> On 5/19/2016 11:32 AM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Joao Pinto  writes:
>>> Hi Felipe and Mathias,
>>>
>>> Sending kernel log with extra xhci messages in attachment!
>>> Thanks you for the help!
>> 
>> yeah, no problems. So here's the interesting part:
>> 
>>*INSERTING PEN DRIVE
>> 
>># xhci-hcd xhci-hcd.0.auto: Port Status Change Event for port 1
>>xhci-hcd xhci-hcd.0.auto: resume root hub
>>xhci-hcd xhci-hcd.0.auto: handle_port_status: starting port polling.
>>xhci-hcd xhci-hcd.0.auto: get port status, actual port 0 status  = 0x206e1
>>xhci-hcd xhci-hcd.0.auto: Get port status returned 0x10101
>>xhci-hcd xhci-hcd.0.auto: clear port connect change, actual port 0 status 
>>  = 0x6e1
>>xhci-hcd xhci-hcd.0.auto: xhci_hub_status_data: stopping port polling.
>>xhci-hcd xhci-hcd.0.auto: get port status, actual port 0 status  = 0x6e1
>>xhci-hcd xhci-hcd.0.auto: Get port status returned 0x101
>>xhci-hcd xhci-hcd.0.auto: xhci_hub_status_data: stopping port polling.
>>xhci-hcd xhci-hcd.0.auto: // Ding dong!
>>xhci-hcd xhci-hcd.0.auto: Command timeout
>>xhci-hcd xhci-hcd.0.auto: Abort command ring
>> 
>> Note that we really did get a command timeout. Can you add a little
>> extra debugging to try and figure out why that command failed?
>> 
>
> After instrumenting and capturing FPGA signals, the driver could go a bit
> further... Could this be a FPGA timming issue?

possible. But before we blame FPGA, can you test Mathias' suggestion?

Thanks

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/2] usb: dwc3: gadget: Various fixes to trbs_left calculation

[Felipe Balbi]

Hi,

John Youn  writes:
> On 5/19/2016 12:51 AM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> John Youn  writes:
>>> This patch fixes up some issues related to the trb_left calculation.
>>>
>>> This calculation sometimes included the link trb slot in the trbs_left
>>> and sometimes didn't.
>> 
>> good catch. But this patch seems like it can be broken into smaller
>> pieces. See below
>
> Ok, I can split it up.

thanks :-)

>>> In the case where the dequeue < enqueue, this number does not include
>>> the link trb and should be used as-is. Otherwise it will include the
>>> link_trb and should be decremented to reflect the actual amount of free
>>> trb slots. The fixed calculation will be the proper amount of free TRB
>>> slots left, taking into account whether a link TRB takes up one of the
>>> slots.
>> 
>> this is one fix :-)
>> 
>>> When checking for full or empty where enqueue == dequeue, the pointer
>>> may be 0. In that case previous TRB is the one before the link TRB. Use
>>> that to check the HWO bit to see if we are full or empty.
>> 
>> if enqueue == dequeue, we could be anywhere in the ring. So previous TRB
>> might not be the one before the link. Care to further explain this case?
>
> The enqueue and dequeue can both be 0 and full and index 0 is treated
> just like any other index in the ring. That has to be the case as
> whenever we increment we don't ever point to a link TRB. We pretend it
> doesn't exist and that it is just a 255-TRB circular queue. How we end
> up full at index 0 is that they are both initially 0, then we enqueue
> 255 TRBs without the hardware getting a chance to process anything
> yet. The enqueue will wrap and point to 0 again, and then it will be
> full.

oh, you're right. I completely missed the case were gadget driver fills
up the ring in one go.

> We check for empty/full in the same way, except that at index 0, when
> we look at the previous TRB, we must wrap around backwards, skip the
> link trb, and look at the TRB prior to the link TRB.

right

>> It's also a separate fix, btw.
>> 
>>> In case DWC3_TRB_NUM is ever set lower than 256, mod trbs_left result by
>>> DWC3_TRB_NUM.
>> 
>> I don't get this. Where did we have % 256? I can only see % DWC3_TRB_NUM.
>
> Right. That was taken care of in the trb_enqueue/deqeuue increment
> functions. But it also needs to be accounted for in the trbs_left
> calculation which was just returning (trb->dequeue - trb->enqueue).
>
> For example if you queue one trb the calculation would
> 0x00 - 0x01 = 0xFF (255), minus 1 for link trb = 254 TRB slots
> free. But if DWC3_TRB_NUM = 8, then I have 254 % 8 = 6 slots free.

okay.

>>> In dwc3_prepare_trbs, if trbs_left is 0, do nothing and return early.
>> 
>> another fix.
>> 
>>> When an EP is enabled, initialized the TRB pool to clean up any stale
>>> data. When the previous TRB was not properly cleaned up on disconnect,
>>> the empty/full checking logic sometimes failed causing the EP to stop
>>> processing requests.
>> 
>> another fix.
>> 
>>> Signed-off-by: John Youn 
>>> ---
>>>  drivers/usb/dwc3/gadget.c | 37 ++---
>>>  1 file changed, 30 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 3eaef22..a76634b 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -561,6 +561,12 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
>>> if (usb_endpoint_xfer_control(desc))
>>> goto out;
>>>  
>>> +   /* Initialize the TRB pool */
>>> +   dep->trb_dequeue = 0;
>>> +   dep->trb_enqueue = 0;
>>> +   memset(dep->trb_pool, 0,
>>> +  sizeof(struct dwc3_trb) * DWC3_TRB_NUM);
>> 
>> this is a separate fix. A very good one, actually ;-)
>> 
>>> /* Link TRB. The HWO bit is never reset */
>>> trb_st_hw = &dep->trb_pool[0];
>>>  
>>> @@ -849,6 +855,8 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
>>>  static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
>>>  {
>>> struct dwc3_trb *tmp;
>>> +   u8  tmp_index;
>>> +   u8  trbs_left;
>>>  
>>> /*
>>> 

Re: composite gadget with _real_ USB device

[Felipe Balbi]

Hi,

Shea Ako  writes:
> I’ve been learning about and playing with configfs and functionfs to
> create composite user space USB gadgets. My objective is to create a
> composite USB gadget that incorporates a custom functionfs function of
> my own creation along with some _real_ USB devices connected to my
> linux platform.
>
> Is there an easy existing way to bundle _real_ USB devices into a
> composite gadget like this or am I looking at writing my own user
> space functionfs functions which handle wrapping and forwarding the
> interfaces/endpoints/data of the connected _real_ USB devices?

heh, you're really on your own. Sounds pretty interesting but you're
gonna spend a lot of time with this :p

> I haven’t found any documented existing way to do this, but I thought
> I should ask before I go off an implement it myself as it seems that
> this might be a use case which isn’t entirely off the wall and there
> could already be support for this somewhere that I haven’t yet
> encountered.

I don't think anybody has done anything like this yet. The closest I got
was to attach some USB sticks to host port via HUB, setup RAID and use
that RAID as backing store for g_mass_storage, then connect
g_mass_storage back to same host which has the RAID of several USB
sticks (same machine has host and peripheral controllers).

-- 
balbi


signature.asc
Description: PGP signature

[PATCH 1/2] usb: xhci: switch to running avg trb length

[Felipe Balbi]

It's unlikely that we will ever know the avg so
instead of assuming it'll be something really large,
we will calculate the avg as we go as mentioned in
XHCI specification section 4.14.1.1.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-mem.c  | 34 --
 drivers/usb/host/xhci-ring.c | 20 
 drivers/usb/host/xhci.h  |  3 ++-
 3 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index bad0d1f9a41d..0f2ca6e85bc0 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1451,18 +1451,34 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
virt_dev->eps[ep_index].skip = false;
ep_ring = virt_dev->eps[ep_index].new_ring;
 
-   /*
-* Get values to fill the endpoint context, mostly from ep descriptor.
-* The average TRB buffer lengt for bulk endpoints is unclear as we
-* have no clue on scatter gather list entry size. For Isoc and Int,
-* set it to max available. See xHCI 1.1 spec 4.14.1.1 for details.
-*/
+   /* Get values to fill the endpoint context, mostly from ep descriptor. 
*/
max_esit_payload = xhci_get_max_esit_payload(udev, ep);
interval = xhci_get_endpoint_interval(udev, ep);
mult = xhci_get_endpoint_mult(udev, ep);
max_packet = GET_MAX_PACKET(usb_endpoint_maxp(&ep->desc));
max_burst = xhci_get_endpoint_max_burst(udev, ep);
-   avg_trb_len = max_esit_payload;
+
+   /*
+* We are using a running avg for our endpoint's avg_trb_len. The reason
+* is that we have no clue about average transfer sizes for any
+* endpoints because the HCD does not know which USB Class is running on
+* the other end.
+*
+* See xHCI 1.1 spec 4.14.1.1 for details about initial avg_trb_len
+* setting.
+*/
+   switch (usb_endpoint_type(&ep->desc)) {
+   case USB_ENDPOINT_XFER_CONTROL:
+   avg_trb_len = 8;
+   break;
+   case USB_ENDPOINT_XFER_INT:
+   avg_trb_len = 1024;
+   break;
+   case USB_ENDPOINT_XFER_ISOC:
+   case USB_ENDPOINT_XFER_BULK:
+   avg_trb_len = 3072;
+   break;
+   }
 
/* FIXME dig Mult and streams info out of ep companion desc */
 
@@ -1472,9 +1488,7 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
/* Some devices get this wrong */
if (usb_endpoint_xfer_bulk(&ep->desc) && udev->speed == USB_SPEED_HIGH)
max_packet = 512;
-   /* xHCI 1.0 and 1.1 indicates that ctrl ep avg TRB Length should be 8 */
-   if (usb_endpoint_xfer_control(&ep->desc) && xhci->hci_version >= 0x100)
-   avg_trb_len = 8;
+
/* xhci 1.1 with LEC support doesn't use mult field, use RsvdZ */
if ((xhci->hci_version > 0x100) && HCC2_LEC(xhci->hcc_params2))
mult = 0;
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 99b4ff42f7a0..6c41dbbf9f2f 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2901,6 +2901,8 @@ static int prepare_transfer(struct xhci_hcd *xhci,
struct xhci_td  *td;
struct xhci_ring *ep_ring;
struct xhci_ep_ctx *ep_ctx = xhci_get_ep_ctx(xhci, xdev->out_ctx, 
ep_index);
+   unsigned int avg_trb_len;
+   unsigned int tx_info;
 
ep_ring = xhci_stream_id_to_ring(xdev, ep_index, stream_id);
if (!ep_ring) {
@@ -2909,6 +2911,24 @@ static int prepare_transfer(struct xhci_hcd *xhci,
return -EINVAL;
}
 
+   /*
+* Here we update avg_trb_len so that, over time, we get a better
+* representation of what the actual average length for this endpoint's
+* TRBs are going to be.
+*/
+   if (urb->transfer_buffer_length > 0) {
+   tx_info = le32_to_cpu(ep_ctx->tx_info);
+
+   avg_trb_len = EP_AVG_TRB_LENGTH(tx_info);
+   avg_trb_len += urb->transfer_buffer_length;
+   avg_trb_len /= 2;
+
+   tx_info &= ~EP_AVG_TRB_LENGTH_MASK;
+   tx_info |= EP_AVG_TRB_LENGTH(avg_trb_len);
+
+   ep_ctx->tx_info = cpu_to_le32(tx_info);
+   }
+
ret = prepare_ring(xhci, ep_ring,
   le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK,
   num_trbs, mem_flags);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 6c629c97f8ad..9f1e9be0afcc 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -751,7 +751,8 @@ struct xhci_ep_ctx {
 #define GET_MAX_PACKET(p)  ((p) & 0x7ff)
 
 /* tx_info bitmasks */
-#define EP_AVG_TRB_LENGTH(p)   ((p) & 0x)
+#define EP_AVG_TRB_LENGTH_MASK 0x
+#de