Re: [RFT 0/3] usb: usb3503: Fix probing on Arndale board (missing phy)

2015-10-08 Thread Marek Szyprowski

Hello,

On 2015-10-08 08:02, Krzysztof Kozlowski wrote:

On 07.10.2015 23:26, Marek Szyprowski wrote:

Hello,

On 2015-10-07 02:30, Krzysztof Kozlowski wrote:

Introduction

This patchset tries to fix probing of usb3503 on Arndale board
if the Samsung PHY driver is probed later (or built as a module).

*The patchset was not tested on Arndale board.*
I don't have that board. Please test it and say if the usb3503
deferred probe
works fine and the issue is solved.

The patchset was tested on Odroid U3 board (which is different!)
in a simulated environment. It is not sufficient testing.


Difference
==
The usb3503 device driver can be used as a I2C device (on Odroid U3)
or as a platform device connected through phy (on Arndale). In the second
case the necessary phy reference has to be obtained and enabled.

For some details please look also at thread [0][1].

[0]
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348524.html

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348875.html



I'm not sure that this is the correct approach. usb3503 chip is simply
connected
to Exynos USB2 phy, so it visible on the USB bus. The real driver that
controls USB2
PHY is Exynos EHCI driver and USB3503 should not mess around it.

The ehci node (usb@1211) has one port configured and it takes one
PHY reference (phy of id 1 - USB host). I can't see driver taking
reference to HSIC0 or HSIC1 phys... Since I cannot diagnose the error I
don't know what is really expected here.


It looks that EHCI in Exynos 5250 and 5420 still use old phy bindings. For
the reference, see Exynos4 dts and exynos4412-odroidu3.dts to check how 
to enable

more than one USB port (Odroid U3 has both HSIC ports enabled).




In my opinion all that is needed in case of Arndale board is forcing
reset of
usb3503 chip after successful EHCI and USB2 PHY initialization (for some
reason
initialization of usb3503 chip must be done after usb host initialization).
However I have no idea which driver should trigger this reset. Right now
I didn't
find any good solution for additional control for devices which are on
autoprobed
bus like usb.

The reset is done at the end of usb3503's probe. The question "why
usb3503 has to be initialized after EHCI and USB PHY" is still valid...


I remember that I saw some code to reset HSIC device after phy power on 
in case

of HSIC-connected modem chip, so maybe this is somehow common for HSIC chips
(which are some special case of 'embedded usb').

Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland

--
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: iperf UDP packet loss with Chipidea HDRC

2015-10-08 Thread Peter Chen
On Mon, Oct 05, 2015 at 07:27:23PM +0530, Jayan John wrote:
> We are developing a custom USB device on a iMX6q platform with a Chipidea
> HDRC. The device uses a single NCM interface for communication with another
> OTG device i.e. Chipidea HDRC again. I see very poor iperf UDP performance
> after role reversal with iperf server running on gadget.
> 
> Kernel: 3.10.17 using Wandboard config
> 
> Test sequence:
> 1. Connect both boards (board A and board B) using OTG cable
> 2. Board A is assigned as A device and board B is assigned as B device
> 3. B device initiates role reversal
> echo -n host > /sys/kernel/debug/ci_hdrc.0/role
> 4. A device switches to gadget mode
> echo -n gadget > /sys/kernel/debug/ci_hdrc.0/role
> 5. Assign IPV6 address on board A (now gadget).. ifconfig usb0 2012::1/64 up
> 6. Assign IPV6 address on board B (now host).. ifconfig usb0 2012::2/64 up
> 7. Run iperf server on board A.. iperf -V -s -u
> 8. Run iperf client on board B.. iperf -u -t 10 -V -c 2012::1 -b 150M
> 
> iperf server logs:
> [  4] local fe80::6cc9:b6ff:fec5:be28 port 5001 connected with
> fe80::54e0:86ff:fea6:8987 port 35629
> [  4]  0.0-10.0 sec  63.7 MBytes  53.4 Mbits/sec 0.171 ms 61701/107109 (58%)
> [  4]  0.0-10.0 sec  1 datagrams received out-of-order
> [  3] local fe80::6cc9:b6ff:fec5:be28 port 5001 connected with
> fe80::54e0:86ff:fea6:8987 port 33609
> [  3]  0.0-10.0 sec  62.1 MBytes  52.1 Mbits/sec 0.215 ms 62551/106825 (59%)
> [  3]  0.0-10.0 sec  1 datagrams received out-of-order
> 
> Query:
> 1. The UDP packet loss is seen only in the case of iperf server on gadget
> after role reversal. I see there are patches (12) from Peter for performance
> improvements on the newer kernels. Will they help?

If you only see this problem after role switch, compare the controller
registers may help it.

> 2. The issue is not seen if UDP socket buffer size/ datagram size on iperf is
> increased i.e. iperf -u -t 10 -l 32k -V -c 2012::1 -b 150M. Is this only
> hiding an issue with Chipidea driver e.g. TXFIFO or burst size?
> 3. Should ncm be tweaked to have different buffer size or NTB handling?

NCM stack may be updated from v3.10, I just tried (without role switch), it
can get 110Mbps, and about 3% loss rate using your command at v4.3-rc1.

-- 

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


Re: [PATCH v3 08/10] Doc: usb: ci-hdrc-usb2: add tx(rx)-burst-config-dword for binding doc

2015-10-08 Thread Peter Chen
On Fri, Aug 14, 2015 at 12:02:37PM +0200, Lucas Stach wrote:
> Am Freitag, den 14.08.2015, 16:40 +0800 schrieb Peter Chen:
> > On Fri, Aug 14, 2015 at 10:37:30AM +0200, Lucas Stach wrote:
> > > Am Freitag, den 07.08.2015, 15:15 +0800 schrieb Peter Chen:
> > > > It is used to override the default setting for burst size, changing
> > > > burst size takes effect only when the SBUSCFG.AHBBRST = 0.
> > > > 
> > > > Signed-off-by: Peter Chen 
> > > > ---
> > > >  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 10 ++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt 
> > > > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > index d52a747..d71ef07 100644
> > > > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > @@ -37,6 +37,14 @@ Optional properties:
> > > >property is used to change AHB burst configuration, check the 
> > > > chipidea
> > > >spec for meaning of each value. If this property is not existed, it
> > > >will use the reset value.
> > > > +- tx-burst-size-dword: it is vendor dependent, the tx burst size in 
> > > > dword
> > > > +  (4 bytes), This register represents the maximum length of a the burst
> > > > +  in 32-bit words while moving data from system memory to the USB
> > > > +  bus, changing this value takes effect only the SBUSCFG.AHBBRST is 0.
> > > 
> > > The last bits about SBUSCFG.AHBBRST don't make any sense in the DT
> > > binding. The DT writer doesn't know about the SBUSCFG.AHBBRST value and
> > > should not care either.
> > > If someone sets the burst size to something
> > > non-standard in the DT, the driver should make sure to enable to
> > > necessary bits to make this setting take effect.
> > 
> > The SBUSCFG.AHBBRST property description is just above this one,
> > and the user should read the spec for changing these system
> > configuration parameters.
> > 
> This is not clear from the description. This should read something like
> "The value of this property will only take effect if property
> ahb-burst-config is set to 0."

Thanks, will change.

> 
> The DT binding should be coherent and understandable without reading the
> DW USB spec and/or the code. Please take some care about this.
> 
> > This requirement is from the spec, and the code logic makes sure
> > the burst size changes only when SBUSCFG.AHBBRST is 0, since the
> > hardware will only change burst size if SBUSCFG.AHBBRST is 0.
> > 
> > > 
> > > Also both those descriptions are missing a description to what value the
> > > burst sizes will be set if the DT properties are not found. If it's
> > > implementation defined spell this out in the doc.
> > 
> > It is optional property, if without this property, it will use the
> > default value.
> > 
> Then specify this in the binding. Again, the binding is a spec that one
> should be able to understand without reading the code.
> 
> "If this property is missing the reset defaults of the hardware
> implementation will be used."
> 

Thanks, will change.

> > > 
> > > Are there really cases where it makes sense to set RX and TX burst sizes
> > > to different values?
> > 
> > Yes, the default burst size is the same for all SoCs, but the bus
> > burst size may be larger for some SoCs.
> > 
> This question wasn't meant to dispute the RX/TX burst size
> configuration, but I'm questioning if it really makes sense to set them
> to different values. Do we really need 2 properties for this, or is 1
> enough?
> 
> Do you see any use case where someone would like to do something like:
> tx-burst-size-dword = <0x8>;
> rx-burst-size-dword = <0x10>;
> 

For Freelscae i.mx current design, the tx/rx burst size are always the same,
but not sure if it is always true for others or for future design.

-- 

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


Re: [PATCH] USB: serial: cp210x: Workaround for cp2108 failure due to GET_LINE_CTL bug

2015-10-08 Thread Bjørn Mork
Konstantin Shkolnyy  writes:

> @@ -343,6 +344,28 @@ static int cp210x_get_config(struct usb_serial_port 
> *port, u8 request,
>   return result;
>   }
>  
> + /* Workaround for swapped bytes in 16-bit value from 
> CP210X_GET_LINE_CTL */
> + if (spriv->swap_get_line_ctl && request == CP210X_GET_LINE_CTL && size 
> == 2) {
> + union {
> + struct {
> + u8 byte0;
> + u8 byte1;
> + u8 byte2;
> + u8 byte3;
> + };
> + u32 as_u32;
> + } tmp_data;
> + u8 old_byte0;
> + u8 old_byte1;
> +
> + tmp_data.as_u32  = data[0]; /* from caller's buffer */
> + old_byte0= tmp_data.byte0;
> + old_byte1= tmp_data.byte1;
> + tmp_data.byte0   = old_byte1;
> + tmp_data.byte1   = old_byte0;
> + data[0]  = tmp_data.as_u32; /* to caller's buffer */
> + }

That looks unnecessarily complicated...  How about:

if (spriv->swap_get_line_ctl && request == CP210X_GET_LINE_CTL && size 
== 2)
swab16s((u16 *)data);


Completely untested...



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


Re: [PATCH v3 0/4] USB MIDI Gadget bug fixes and improvements

2015-10-08 Thread Felipe Tonello
On Tue, Sep 29, 2015 at 1:01 PM, Felipe F. Tonello  
wrote:
> Here is the third version of this patch set. It includes memory leakage bug
> fix, improvements and code cleanups.
>
> Felipe F. Tonello (4):
>   usb: gadget: f_midi: free usb request when done
>   usb: gadget: f_midi: free request when usb_ep_queue fails
>   usb: gadget: f_midi: Transmit data only when IN ep is enabled
>   usb: gadget: f_midi: remove duplicated code
>
>  drivers/usb/gadget/function/f_midi.c | 36 
> +++-
>  1 file changed, 11 insertions(+), 25 deletions(-)
>
> --
> 2.1.4
>

ping?
--
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: [RFT 0/3] usb: usb3503: Fix probing on Arndale board (missing phy)

2015-10-08 Thread Javier Martinez Canillas
Hello,

On 10/08/2015 08:23 AM, Marek Szyprowski wrote:
> Hello,
> 
> On 2015-10-08 08:02, Krzysztof Kozlowski wrote:
>> On 07.10.2015 23:26, Marek Szyprowski wrote:
>>> Hello,
>>>
>>> On 2015-10-07 02:30, Krzysztof Kozlowski wrote:
 Introduction
 
 This patchset tries to fix probing of usb3503 on Arndale board
 if the Samsung PHY driver is probed later (or built as a module).

 *The patchset was not tested on Arndale board.*
 I don't have that board. Please test it and say if the usb3503
 deferred probe
 works fine and the issue is solved.

 The patchset was tested on Odroid U3 board (which is different!)
 in a simulated environment. It is not sufficient testing.


 Difference
 ==
 The usb3503 device driver can be used as a I2C device (on Odroid U3)
 or as a platform device connected through phy (on Arndale). In the second
 case the necessary phy reference has to be obtained and enabled.

 For some details please look also at thread [0][1].

 [0]
 http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348524.html

 [1]
 http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348875.html


>>> I'm not sure that this is the correct approach. usb3503 chip is simply
>>> connected
>>> to Exynos USB2 phy, so it visible on the USB bus. The real driver that
>>> controls USB2
>>> PHY is Exynos EHCI driver and USB3503 should not mess around it.
>> The ehci node (usb@1211) has one port configured and it takes one
>> PHY reference (phy of id 1 - USB host). I can't see driver taking
>> reference to HSIC0 or HSIC1 phys... Since I cannot diagnose the error I
>> don't know what is really expected here.
> 
> It looks that EHCI in Exynos 5250 and 5420 still use old phy bindings. For
> the reference, see Exynos4 dts and exynos4412-odroidu3.dts to check how to 
> enable
> more than one USB port (Odroid U3 has both HSIC ports enabled).
> 
>>
>>> In my opinion all that is needed in case of Arndale board is forcing
>>> reset of
>>> usb3503 chip after successful EHCI and USB2 PHY initialization (for some
>>> reason
>>> initialization of usb3503 chip must be done after usb host initialization).
>>> However I have no idea which driver should trigger this reset. Right now
>>> I didn't
>>> find any good solution for additional control for devices which are on
>>> autoprobed
>>> bus like usb.
>> The reset is done at the end of usb3503's probe. The question "why
>> usb3503 has to be initialized after EHCI and USB PHY" is still valid...
> 
> I remember that I saw some code to reset HSIC device after phy power on in 
> case
> of HSIC-connected modem chip, so maybe this is somehow common for HSIC chips
> (which are some special case of 'embedded usb').
>

I also don't have an Arndale board and haven't followed the thread to closely
but I just wanted to mention that the ChromiumOS 3.8 tree has a workaround to
reset the HSIC phys:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/81685c447954a29d1098268776582457258dd98f%5E%21/

and later a "supports-hsicphy-reset" DT property was added to force the reset
per board instead of unconditionally:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/a4d1c1a223ffa1ed38a4257d0378ca70c6667be0%5E%21/

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
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: [RFT 0/3] usb: usb3503: Fix probing on Arndale board (missing phy)

2015-10-08 Thread Krzysztof Kozlowski
On 07.10.2015 23:26, Marek Szyprowski wrote:
> Hello,
> 
> On 2015-10-07 02:30, Krzysztof Kozlowski wrote:
>> Introduction
>> 
>> This patchset tries to fix probing of usb3503 on Arndale board
>> if the Samsung PHY driver is probed later (or built as a module).
>>
>> *The patchset was not tested on Arndale board.*
>> I don't have that board. Please test it and say if the usb3503
>> deferred probe
>> works fine and the issue is solved.
>>
>> The patchset was tested on Odroid U3 board (which is different!)
>> in a simulated environment. It is not sufficient testing.
>>
>>
>> Difference
>> ==
>> The usb3503 device driver can be used as a I2C device (on Odroid U3)
>> or as a platform device connected through phy (on Arndale). In the second
>> case the necessary phy reference has to be obtained and enabled.
>>
>> For some details please look also at thread [0][1].
>>
>> [0]
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348524.html
>>
>> [1]
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348875.html
>>
>>

> 
> I'm not sure that this is the correct approach. usb3503 chip is simply
> connected
> to Exynos USB2 phy, so it visible on the USB bus. The real driver that
> controls USB2
> PHY is Exynos EHCI driver and USB3503 should not mess around it.

The ehci node (usb@1211) has one port configured and it takes one
PHY reference (phy of id 1 - USB host). I can't see driver taking
reference to HSIC0 or HSIC1 phys... Since I cannot diagnose the error I
don't know what is really expected here.

> 
> In my opinion all that is needed in case of Arndale board is forcing
> reset of
> usb3503 chip after successful EHCI and USB2 PHY initialization (for some
> reason
> initialization of usb3503 chip must be done after usb host initialization).
> However I have no idea which driver should trigger this reset. Right now
> I didn't
> find any good solution for additional control for devices which are on
> autoprobed
> bus like usb.

The reset is done at the end of usb3503's probe. The question "why
usb3503 has to be initialized after EHCI and USB PHY" is still valid...

Anyway thanks for feedback! I really appreciate it.

Best regards,
Krzysztof

--
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 1/3][v4] Documentation: dt: dwc3: Add snps,quirk-frame-length-adjustment property

2015-10-08 Thread Shawn Guo
On Mon, Oct 05, 2015 at 05:37:02AM +, RAJESH BHAGAT wrote:
> Hi Shawn,
> 
> Regarding below patch, Felipe has suggested to talk to you:
> 
> > [PATCH 3/3][v4] arm: dts: ls1021a: Add quirk for Erratum A009116
> 
> talk to you ARM-SoC maintainer.
> 
> https://lkml.org/lkml/2015/9/4/7
> 
> Please provide your comments. Inform me in case, if a resend is required.

If it can go independently through arm-soc tree, please resend it to me.

Shawn
--
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 1/1] usb: misc: usbtest: add bulk queue test

2015-10-08 Thread Peter Chen
On Sun, Oct 04, 2015 at 10:47:58AM +0100, Greg KH wrote:
> On Mon, Sep 07, 2015 at 02:13:57PM +0800, Peter Chen wrote:
> > The bulk queue tests are used to show 'best performance' for bulk
> > transfer, we are often asked this question by users. The implementation
> > is the same with iso test, that is queue request at interrupt completion,
> > so we reuse the iso structures, and rename them as common one.
> > 
> > It's result should be very close to IC simulation, in order
> > to get that, the device side should also need to prepare enough
> > queue.
> > 
> > We have got the 'best performance' (IN: 41MB, OUT: 39MB) at i.mx platform
> > (USB2, ARM Cortex A9, stream mode need to enable) with below command:
> > 
> > Host side:
> > modprobe usbtest
> > ./testusb -a -t 27 -g 64 -s 16384
> > ./testusb -a -t 28 -g 64 -s 16384
> > Gadget side:
> > modprobe g_zero loopdefault=1 qlen=64 buflen=16384
> > 
> > Signed-off-by: Peter Chen 
> > ---
> >  drivers/usb/misc/usbtest.c | 104 
> > +++--
> >  1 file changed, 73 insertions(+), 31 deletions(-)
> 
> Doesn't apply to the usb-next branch :(

The other usb-test changes are applied at Felipe's tree, this one
is based on those change.

Felipe, would you please help to queue it?

-- 

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


Re: [PATCH v3 1/1] USB:option:add ZTE PIDs

2015-10-08 Thread Johan Hovold
On Tue, Oct 06, 2015 at 07:10:44PM -0700, Liu.Zhao wrote:
> This is intended to add ZTE device PIDs on kernel.
> 
> Signed-off-by: Liu.Zhao 

This one is already in Linus' tree and should be backported to the
stable trees shortly. No need to resend.

Johan
--
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 v9 4/5] xhci: mediatek: support MTK xHCI host controller

2015-10-08 Thread Daniel Thompson

On 08/10/15 13:05, chunfeng yun wrote:

Hi,
On Thu, 2015-10-01 at 12:44 +0100, Daniel Thompson wrote:

On 29/09/15 04:01, Chunfeng Yun wrote:

There some vendor quirks for MTK xhci host controller:
1. It defines some extra SW scheduling parameters for HW
to minimize the scheduling effort for synchronous and
interrupt endpoints. The parameters are put into reseved
DWs of slot context and endpoint context.
2. Its IMODI unit for Interrupter Moderation register is
8 times as much as that defined in xHCI spec.
3. Its TDS in  Normal TRB defines a number of packets that
remains to be transferred for a TD after processing all
Max packets in all previous TRBs.

Signed-off-by: Chunfeng Yun 


I've done some basic soak tests, both with a directly attached USB3 HDD
and, given the extra code to manage isochronous xfer, also with a hub,
disc and two audio interfaces.

Tested-by: Daniel Thompson 



diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 57f40a1..243f696 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -68,6 +68,7 @@
   #include 
   #include "xhci.h"
   #include "xhci-trace.h"
+#include "xhci-mtk.h"

   /*
* Returns zero if the TRB isn't in this segment, otherwise it returns the 
DMA
@@ -3044,18 +3045,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
  struct urb *urb, unsigned int num_trbs_left)
   {
u32 maxp, total_packet_count;
+   u32 skip_current_trb = 0;


A bit of a nitpick but why do we need skip_current_trb? Testing
(xhci->quirks & XHCI_MTK_HOST) twice would make what the code does, and
why, more obvious.


I am not sure which way it is better, so add variable of
skip_current_trb to reduce test times.


I can't imagine either approach impacts performance. However introducing 
skip_current_trb makes the flow of the branches harder to follow.


The first branch was a version check and can remain so; we only need to 
do something special with the quirks because the MTK controller is 0.97 
but with a few bits added:


/* MTK xHCI is mostly 0.97 but contains some features from 1.0 */
if (xhci->hci_version < 0x100 && !(xhci->quirks & XHCI_MTK_HOST))
return ...

Setting trb_buff_len to zero is accommodating a quirk and this is 
clearer when we have:


/* for MTK xHCI, TD size doesn't include this TRB */
if (xhci->quirks & XHCI_MTK_HOST)
trb_buff_len = 0;



Anyhow with that looked at, and the caveat that I'm not much of USB
expert, you're welcome to add my Reviewed-by: to v10.



Thanks a lot


No worries.

--
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 v9 3/5] phy: add usb3.0 phy driver for mt65xx SoCs

2015-10-08 Thread chunfeng yun
Hi,
On Tue, 2015-10-06 at 19:50 +0530, Kishon Vijay Abraham I wrote:
> Hi Chunfeng,
> 
> On Tuesday 29 September 2015 08:31 AM, Chunfeng Yun wrote:
> > support usb3.0 phy of mt65xx SoCs
> 
> I have merged this driver. Can you also send a patch adding yourself as
> the maintainer of this driver?
> 
Ok, I will send it later

Thank you very much.

> Thanks
> Kishon
> > 
> > Signed-off-by: Chunfeng Yun 
> > ---
> >  drivers/phy/Kconfig   |   9 +
> >  drivers/phy/Makefile  |   1 +
> >  drivers/phy/phy-mt65xx-usb3.c | 506 
> > ++
> >  3 files changed, 516 insertions(+)
> >  create mode 100644 drivers/phy/phy-mt65xx-usb3.c
> > 
> > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> > index 47da573..ec436c1 100644
> > --- a/drivers/phy/Kconfig
> > +++ b/drivers/phy/Kconfig
> > @@ -206,6 +206,15 @@ config PHY_HIX5HD2_SATA
> > help
> >   Support for SATA PHY on Hisilicon hix5hd2 Soc.
> >  
> > +config PHY_MT65XX_USB3
> > +   tristate "Mediatek USB3.0 PHY Driver"
> > +   depends on ARCH_MEDIATEK && OF
> > +   select GENERIC_PHY
> > +   help
> > + Say 'Y' here to add support for Mediatek USB3.0 PHY driver
> > + for mt65xx SoCs. it supports two usb2.0 ports and
> > + one usb3.0 port.
> > +
> >  config PHY_SUN4I_USB
> > tristate "Allwinner sunxi SoC USB PHY driver"
> > depends on ARCH_SUNXI && HAS_IOMEM && OF
> > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> > index a5b18c1..a7cc629 100644
> > --- a/drivers/phy/Makefile
> > +++ b/drivers/phy/Makefile
> > @@ -23,6 +23,7 @@ obj-$(CONFIG_TI_PIPE3)+= 
> > phy-ti-pipe3.o
> >  obj-$(CONFIG_TWL4030_USB)  += phy-twl4030-usb.o
> >  obj-$(CONFIG_PHY_EXYNOS5250_SATA)  += phy-exynos5250-sata.o
> >  obj-$(CONFIG_PHY_HIX5HD2_SATA) += phy-hix5hd2-sata.o
> > +obj-$(CONFIG_PHY_MT65XX_USB3)  += phy-mt65xx-usb3.o
> >  obj-$(CONFIG_PHY_SUN4I_USB)+= phy-sun4i-usb.o
> >  obj-$(CONFIG_PHY_SUN9I_USB)+= phy-sun9i-usb.o
> >  obj-$(CONFIG_PHY_SAMSUNG_USB2) += phy-exynos-usb2.o
> > diff --git a/drivers/phy/phy-mt65xx-usb3.c b/drivers/phy/phy-mt65xx-usb3.c
> > new file mode 100644
> > index 000..f30b28b
> > --- /dev/null
> > +++ b/drivers/phy/phy-mt65xx-usb3.c
> > @@ -0,0 +1,506 @@
> > +/*
> > + * Copyright (c) 2015 MediaTek Inc.
> > + * Author: Chunfeng Yun 
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/*
> > + * for sifslv2 register, but exclude port's;
> > + * relative to USB3_SIF2_BASE base address
> > + */
> > +#define SSUSB_SIFSLV_SPLLC 0x
> > +
> > +/* offsets of sub-segment in each port registers */
> > +#define SSUSB_SIFSLV_U2PHY_COM_BASE0x
> > +#define SSUSB_SIFSLV_U3PHYD_BASE   0x0100
> > +#define SSUSB_USB30_PHYA_SIV_B_BASE0x0300
> > +#define SSUSB_SIFSLV_U3PHYA_DA_BASE0x0400
> > +
> > +#define U3P_USBPHYACR0 (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x)
> > +#define PA0_RG_U2PLL_FORCE_ON  BIT(15)
> > +
> > +#define U3P_USBPHYACR2 (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0008)
> > +#define PA2_RG_SIF_U2PLL_FORCE_EN  BIT(18)
> > +
> > +#define U3P_USBPHYACR5 (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0014)
> > +#define PA5_RG_U2_HSTX_SRCTRL  GENMASK(14, 12)
> > +#define PA5_RG_U2_HSTX_SRCTRL_VAL(x)   ((0x7 & (x)) << 12)
> > +#define PA5_RG_U2_HS_100U_U3_ENBIT(11)
> > +
> > +#define U3P_USBPHYACR6 (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0018)
> > +#define PA6_RG_U2_ISO_EN   BIT(31)
> > +#define PA6_RG_U2_BC11_SW_EN   BIT(23)
> > +#define PA6_RG_U2_OTG_VBUSCMP_EN   BIT(20)
> > +
> > +#define U3P_U2PHYACR4  (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0020)
> > +#define P2C_RG_USB20_GPIO_CTL  BIT(9)
> > +#define P2C_USB20_GPIO_MODEBIT(8)
> > +#define P2C_U2_GPIO_CTR_MSK(P2C_RG_USB20_GPIO_CTL | 
> > P2C_USB20_GPIO_MODE)
> > +
> > +#define U3D_U2PHYDCR0  (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0060)
> > +#define P2C_RG_SIF_U2PLL_FORCE_ON  BIT(24)
> > +
> > +#define U3P_U2PHYDTM0  (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0068)
> > +#define P2C_FORCE_UART_EN  BIT(26)
> > +#define P2C_FORCE_DATAIN   BIT(23)
> > +#define P2C_FORCE_DM_PULLDOWN  BIT(21)
> > +#define 

Re: [RFT 0/3] usb: usb3503: Fix probing on Arndale board (missing phy)

2015-10-08 Thread Marek Szyprowski

Hello,

On 2015-10-08 11:35, Javier Martinez Canillas wrote:

Hello,

On 10/08/2015 08:23 AM, Marek Szyprowski wrote:

Hello,

On 2015-10-08 08:02, Krzysztof Kozlowski wrote:

On 07.10.2015 23:26, Marek Szyprowski wrote:

Hello,

On 2015-10-07 02:30, Krzysztof Kozlowski wrote:

Introduction

This patchset tries to fix probing of usb3503 on Arndale board
if the Samsung PHY driver is probed later (or built as a module).

*The patchset was not tested on Arndale board.*
I don't have that board. Please test it and say if the usb3503
deferred probe
works fine and the issue is solved.

The patchset was tested on Odroid U3 board (which is different!)
in a simulated environment. It is not sufficient testing.


Difference
==
The usb3503 device driver can be used as a I2C device (on Odroid U3)
or as a platform device connected through phy (on Arndale). In the second
case the necessary phy reference has to be obtained and enabled.

For some details please look also at thread [0][1].

[0]
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348524.html

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348875.html



I'm not sure that this is the correct approach. usb3503 chip is simply
connected
to Exynos USB2 phy, so it visible on the USB bus. The real driver that
controls USB2
PHY is Exynos EHCI driver and USB3503 should not mess around it.

The ehci node (usb@1211) has one port configured and it takes one
PHY reference (phy of id 1 - USB host). I can't see driver taking
reference to HSIC0 or HSIC1 phys... Since I cannot diagnose the error I
don't know what is really expected here.

It looks that EHCI in Exynos 5250 and 5420 still use old phy bindings. For
the reference, see Exynos4 dts and exynos4412-odroidu3.dts to check how to 
enable
more than one USB port (Odroid U3 has both HSIC ports enabled).


In my opinion all that is needed in case of Arndale board is forcing
reset of
usb3503 chip after successful EHCI and USB2 PHY initialization (for some
reason
initialization of usb3503 chip must be done after usb host initialization).
However I have no idea which driver should trigger this reset. Right now
I didn't
find any good solution for additional control for devices which are on
autoprobed
bus like usb.

The reset is done at the end of usb3503's probe. The question "why
usb3503 has to be initialized after EHCI and USB PHY" is still valid...

I remember that I saw some code to reset HSIC device after phy power on in case
of HSIC-connected modem chip, so maybe this is somehow common for HSIC chips
(which are some special case of 'embedded usb').


I also don't have an Arndale board and haven't followed the thread to closely
but I just wanted to mention that the ChromiumOS 3.8 tree has a workaround to
reset the HSIC phys:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/81685c447954a29d1098268776582457258dd98f%5E%21/

and later a "supports-hsicphy-reset" DT property was added to force the reset
per board instead of unconditionally:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/a4d1c1a223ffa1ed38a4257d0378ca70c6667be0%5E%21/


I didn't check this approach, but for me it looks that the problem is caused
by the lack of resetting the chip connected to hsic phy not the lack of
resetting the phy itself. However this is pure speculation and one should
check it with the real hardware.

Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland

--
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 v9 1/5] dt-bindings: Add usb3.0 phy binding for MT65xx SoCs

2015-10-08 Thread chunfeng yun
Hi,
On Tue, 2015-09-29 at 17:43 +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 09/29/2015 06:01 AM, Chunfeng Yun wrote:
> 
> > add a DT binding documentation of usb3.0 phy for MT65xx
> > SoCs from Mediatek.
> >
> > Acked-by: Rob Herring 
> > Signed-off-by: Chunfeng Yun 
> > ---
> >   .../devicetree/bindings/phy/phy-mt65xx-usb.txt | 68 
> > ++
> >   1 file changed, 68 insertions(+)
> >   create mode 100644 
> > Documentation/devicetree/bindings/phy/phy-mt65xx-usb.txt
> >
> > diff --git a/Documentation/devicetree/bindings/phy/phy-mt65xx-usb.txt 
> > b/Documentation/devicetree/bindings/phy/phy-mt65xx-usb.txt
> > new file mode 100644
> > index 000..00100cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/phy-mt65xx-usb.txt
> > @@ -0,0 +1,68 @@
> > +mt65xx USB3.0 PHY binding
> > +--
> > +
> > +This binding describes a usb3.0 phy for mt65xx platforms of Medaitek SoC.
> > +
> > +Required properties (controller (parent) node):
> > + - compatible  : should be "mediatek,mt8173-u3phy"
> > + - reg : offset and length of register for phy, exclude port's
> > + register.
> > + - clocks  : a list of phandle + clock-specifier pairs, one for each
> > + entry in clock-names
> > + - clock-names : must contain
> > + "u3phya_ref": for reference clock of usb3.0 analog phy.
> > +
> > +Required nodes : a sub-node is required for each port the controller
> > + provides. Address range information including the usual
> > + 'reg' property is used inside these nodes to describe
> > + the controller's topology.
> > +
> > +Required properties (port (child) node):
> > +- reg  : address and length of the register set for the port.
> > +- #phy-cells   : should be 1 (See second example)
> > + cell after port phandle is phy type from:
> > +   - PHY_TYPE_USB2
> > +   - PHY_TYPE_USB3
> > +
> > +Example:
> > +
> > +u3phy: usb-phy@1129 {
> > +   compatible = "mediatek,mt8173-u3phy";
> > +   reg = <0 0x1129 0 0x800>;
> > +   clocks = < CLK_APMIXED_REF2USB_TX>;
> > +   clock-names = "u3phya_ref";
> > +   #address-cells = <2>;
> > +   #size-cells = <2>;
> > +   ranges;
> 
> You don't describe the above 3 props neither as mandatory nor optional.
> 
I will describe them as mandatory properties later.

> [...]
> > +Specifying phy control of devices
> > +-
> > +
> > +Device nodes should specify the configuration required in their "phys"
> > +property, containing a phandle to the phy port node and a device type;
> > +phy-names for each port are optional.
> > +
> > +Example:
> > +
> > +#include 
> > +
> > +usb30: usb@1127 {
> > +   ...
> > +   phys = <_port0 PHY_TYPE_USB3>;
> > +   phy-names = "usb3-0";
> > +   ...
> > +};
> 
> Isn't the above already described in 
> Documentation/devicetree/bindings/phy/phy-bindings.txt?
> 
The usage is already described in phy-bindings.txt.
But many phy binding documents include a simple example, so I add it
too.

Thanks a lot

> MBR, Sergei
> 


--
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 v9 2/5] dt-bindings: Add a binding for Mediatek xHCI host controller

2015-10-08 Thread chunfeng yun
Hi,
On Tue, 2015-09-29 at 17:49 +0300, Sergei Shtylyov wrote:
> On 09/29/2015 06:01 AM, Chunfeng Yun wrote:
> 
> > add a DT binding documentation of xHCI host controller for the
> > MT8173 SoC from Mediatek.
> >
> > Signed-off-by: Chunfeng Yun 
> > ---
> >   .../devicetree/bindings/usb/mt8173-xhci.txt| 52 
> > ++
> >   1 file changed, 52 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/usb/mt8173-xhci.txt
> >
> > diff --git a/Documentation/devicetree/bindings/usb/mt8173-xhci.txt 
> > b/Documentation/devicetree/bindings/usb/mt8173-xhci.txt
> > new file mode 100644
> > index 000..0c96273
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/mt8173-xhci.txt
> > @@ -0,0 +1,52 @@
> [...]
> > +
> > +Example:
> > +usb30: usb@1127 {
> [...]
> > +   status = "okay";
> 
> This line is not needed.
> 
I will remove it.

Thanks

> [...]
> 
> MBR, Sergei
> 


--
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: Regression: USB OTG port breaks after a few hours in host mode on iMX6

2015-10-08 Thread Peter Chen
 
> 
> On Wed, Oct 07, 2015 at 09:22:22AM +0100, Russell King - ARM Linux wrote:
> > This bug is soo obscure, I'm not even sure how to start debugging it.
> > This never used to be a problem, but I'm not sure when it started as
> > USB (in general) is not something I use regularly.
> >
> > In this setup, the USB OTG port is wired in host mode via pinmix
> > configuration of the USB OTG ID pin:
> >
> > MX6QDL_PAD_GPIO_1__USB_OTG_ID 0x13059
> >
> 
> If the port has only host function, you can set dr_mode = "host" at dts, it 
> will
> configure this port for host only, and ignore id interrupt.
> 
> > The problem characterised so far: after booting the kernel, USB seems
> > to work fine.  You can plug and unplug devices from both USB host and
> > USB OTG ports, and it's detected.
> >
> > If you boot a kernel with nothing plugged in, leave it for maybe four
> > hours, then plug in a device to either port, the device will be seen
> > in the USB host port, but completely ignored in the USB OTG port.
> > Both USB OTG ports have power, confirmed last night when using a USB
> > microscope with built-in LED lighting: the LEDs are functional, the
> > device is otherwise completely ignored.
> 
> Does interrupt occur after plugging in? If not, it means wake-on-connect does
> not wake up otg port. Else, the USB PHY may not be recovered well after
> suspend.
> 
> >
> > The above is basically what I know so far: I don't know when the port
> > fails, whether in the case of "boot, wait four hours, and it's dead"
> > whether it's dead from boot: when I've tried testing it immediately
> > after boot, it's worked.
> >
> > As it takes around four hours to reproduce, and I have a massive patch
> > stack on top of the kernel for this platform, I'm unwilling to attempt
> > a git bisection to try and find when this occurred, but I know it's
> > been going on for a while now as I've noticed the same behaviour when
> > I transfer my logitech USB receiver to that port.
> >
> > I had thought it was a power issue, but the USB camera last night
> > confirmed that it's a port driver issue.
> 
> I am testing it with two boards to see if it can be reproduced.
 
I can't reproduce it for 5 hours, and will change pinmux (do the same thing 
with your platform),
and do the overnight test.

Peter
--
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 v9 4/5] xhci: mediatek: support MTK xHCI host controller

2015-10-08 Thread chunfeng yun
Hi,
On Thu, 2015-10-01 at 12:44 +0100, Daniel Thompson wrote:
> On 29/09/15 04:01, Chunfeng Yun wrote:
> > There some vendor quirks for MTK xhci host controller:
> > 1. It defines some extra SW scheduling parameters for HW
> >to minimize the scheduling effort for synchronous and
> >interrupt endpoints. The parameters are put into reseved
> >DWs of slot context and endpoint context.
> > 2. Its IMODI unit for Interrupter Moderation register is
> >8 times as much as that defined in xHCI spec.
> > 3. Its TDS in  Normal TRB defines a number of packets that
> >remains to be transferred for a TD after processing all
> >Max packets in all previous TRBs.
> >
> > Signed-off-by: Chunfeng Yun 
> 
> I've done some basic soak tests, both with a directly attached USB3 HDD 
> and, given the extra code to manage isochronous xfer, also with a hub, 
> disc and two audio interfaces.
> 
> Tested-by: Daniel Thompson 
> 
> 
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 57f40a1..243f696 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -68,6 +68,7 @@
> >   #include 
> >   #include "xhci.h"
> >   #include "xhci-trace.h"
> > +#include "xhci-mtk.h"
> >
> >   /*
> >* Returns zero if the TRB isn't in this segment, otherwise it returns 
> > the DMA
> > @@ -3044,18 +3045,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, 
> > int transferred,
> >   struct urb *urb, unsigned int num_trbs_left)
> >   {
> > u32 maxp, total_packet_count;
> > +   u32 skip_current_trb = 0;
> 
> A bit of a nitpick but why do we need skip_current_trb? Testing 
> (xhci->quirks & XHCI_MTK_HOST) twice would make what the code does, and 
> why, more obvious.
> 
I am not sure which way it is better, so add variable of
skip_current_trb to reduce test times.

> Anyhow with that looked at, and the caveat that I'm not much of USB 
> expert, you're welcome to add my Reviewed-by: to v10.
> 

Thanks a lot
> >
> > -   if (xhci->hci_version < 0x100)
> > -   return ((td_total_len - transferred) >> 10);
> > -
> > -   maxp = GET_MAX_PACKET(usb_endpoint_maxp(>ep->desc));
> > -   total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
> > +   if (xhci->hci_version < 0x100) {
> > +   /* for MTK xHCI, TD size doesn't include this TRB */
> > +   if (xhci->quirks & XHCI_MTK_HOST)
> > +   skip_current_trb = 1;
> > +   else
> > +   return ((td_total_len - transferred) >> 10);
> > +   }
> >
> > /* One TRB with a zero-length data packet. */
> > if (num_trbs_left == 0 || (transferred == 0 && trb_buff_len == 0) ||
> > trb_buff_len == td_total_len)
> > return 0;
> >
> > +   if (skip_current_trb)
> > +   trb_buff_len = 0;
> > +
> > +   maxp = GET_MAX_PACKET(usb_endpoint_maxp(>ep->desc));
> > +   total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
> > +
> > /* Queueing functions don't count the current TRB into transferred */
> > return (total_packet_count - ((transferred + trb_buff_len) / maxp));
> >   }
> 
> 
> Daniel.
> 


--
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: Configfs composite gadget with CCID and mass storage

2015-10-08 Thread Frans-Pieter van Wyk
I managed to get the implementation working for us using the following dirty 
patch for kernel 3.14,
-
diff -u -r a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
--- a/drivers/usb/gadget/f_fs.c 2014-06-11 21:02:49.0 +0200
+++ b/drivers/usb/gadget/f_fs.c 2015-10-08 11:34:29.177420065 +0200
@@ -1449,8 +1449,9 @@
break;
 
case HID_DT_HID:
-   pr_vdebug("hid descriptor\n");
-   if (length != sizeof(struct hid_descriptor))
+   pr_vdebug("hid or sc device class descriptor\n");
+   if (length != sizeof(struct hid_descriptor) &&
+   length != 0x36)
goto inv_length;
break;
 
-

It turns out that our main problem was passing the smart card device class 
descriptor to FunctionFS as the code in f_fs.c incorrectly identified our 
descriptor as a HID descriptor since HID_DT_HID == 0x21 which corresponds to 
the bDescriptorType value for a smart card device class descriptor. It looks 
like FunctionFS does not make provision for class descriptors specific to 
certain interfaces.

I also had to remove "USB wakeup signalling" from dwFeatures in our smart card 
class descriptor to get the implementation working on Windows.

This might help someone someday :-)

Cheers,
Pieter



--
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 v9 4/5] xhci: mediatek: support MTK xHCI host controller

2015-10-08 Thread Mathias Nyman

On 08.10.2015 15:28, Daniel Thompson wrote:

On 08/10/15 13:05, chunfeng yun wrote:

Hi,
On Thu, 2015-10-01 at 12:44 +0100, Daniel Thompson wrote:

On 29/09/15 04:01, Chunfeng Yun wrote:

There some vendor quirks for MTK xhci host controller:
1. It defines some extra SW scheduling parameters for HW
to minimize the scheduling effort for synchronous and
interrupt endpoints. The parameters are put into reseved
DWs of slot context and endpoint context.
2. Its IMODI unit for Interrupter Moderation register is
8 times as much as that defined in xHCI spec.
3. Its TDS in  Normal TRB defines a number of packets that
remains to be transferred for a TD after processing all
Max packets in all previous TRBs.

Signed-off-by: Chunfeng Yun 


I've done some basic soak tests, both with a directly attached USB3 HDD
and, given the extra code to manage isochronous xfer, also with a hub,
disc and two audio interfaces.

Tested-by: Daniel Thompson 



diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 57f40a1..243f696 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -68,6 +68,7 @@
   #include 
   #include "xhci.h"
   #include "xhci-trace.h"
+#include "xhci-mtk.h"

   /*
* Returns zero if the TRB isn't in this segment, otherwise it returns the 
DMA
@@ -3044,18 +3045,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
 struct urb *urb, unsigned int num_trbs_left)
   {
   u32 maxp, total_packet_count;
+u32 skip_current_trb = 0;


A bit of a nitpick but why do we need skip_current_trb? Testing
(xhci->quirks & XHCI_MTK_HOST) twice would make what the code does, and
why, more obvious.


I am not sure which way it is better, so add variable of
skip_current_trb to reduce test times.


I can't imagine either approach impacts performance. However introducing 
skip_current_trb makes the flow of the branches harder to follow.

The first branch was a version check and can remain so; we only need to do 
something special with the quirks because the MTK controller is 0.97 but with a 
few bits added:

 /* MTK xHCI is mostly 0.97 but contains some features from 1.0 */
 if (xhci->hci_version < 0x100 && !(xhci->quirks & XHCI_MTK_HOST))
 return ...

Setting trb_buff_len to zero is accommodating a quirk and this is clearer when 
we have:

 /* for MTK xHCI, TD size doesn't include this TRB */
 if (xhci->quirks & XHCI_MTK_HOST)
 trb_buff_len = 0;


Both will work, but I'd prefer this solution as well.

-Mathias

--
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: musb: dsps: implement vbus_status method

2015-10-08 Thread Sergei Shtylyov

On 10/8/2015 4:50 PM, Sergei Shtylyov wrote:


Implement vbus_status  method of musb_platform_ops that allows
musb_core to properly represent the VBUS status of musb_dsps devices in
corresponding sysfs entry

Signed-off-by: Roman Alyautdin 
---
  drivers/usb/musb/musb_dsps.c |   13 +
  1 file changed, 13 insertions(+)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 84512d1..9c00edf 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -632,6 +632,18 @@ static void dsps_read_fifo32(struct musb_hw_ep *hw_ep,
u16 len, u8 *dst)
  }
  }

+static int dsps_musb_vbus_status(struct musb *musb)
+{
+void __iomem *mregs = musb->mregs;
+u8 devctl;
+
+devctl = dsps_readb(mregs, MUSB_DEVCTL);
+if ((devctl & MUSB_DEVCTL_VBUS) == (3 << MUSB_DEVCTL_VBUS_SHIFT))


I don't see why this has to be is implemented in the DSPS glue layer. The
DevCtl register is a standard MUSB one.


   Looking at the musb_platform_get_vbus_status(), it's unclear why it 
returns 0 if the vbus_status() method id undefined. I think we should read the 
DevCtl registre here, removing the FIXME at the call site.


MBR, Sergei

--
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: musb: dsps: implement vbus_status method

2015-10-08 Thread Roman Alyautdin

On 08/10/15 17:07, Sergei Shtylyov wrote:

On 10/8/2015 4:50 PM, Sergei Shtylyov wrote:


Implement vbus_status  method of musb_platform_ops that allows
musb_core to properly represent the VBUS status of musb_dsps devices in
corresponding sysfs entry

Signed-off-by: Roman Alyautdin 
---
  drivers/usb/musb/musb_dsps.c |   13 +
  1 file changed, 13 insertions(+)

diff --git a/drivers/usb/musb/musb_dsps.c 
b/drivers/usb/musb/musb_dsps.c

index 84512d1..9c00edf 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -632,6 +632,18 @@ static void dsps_read_fifo32(struct musb_hw_ep 
*hw_ep,

u16 len, u8 *dst)
  }
  }

+static int dsps_musb_vbus_status(struct musb *musb)
+{
+void __iomem *mregs = musb->mregs;
+u8 devctl;
+
+devctl = dsps_readb(mregs, MUSB_DEVCTL);
+if ((devctl & MUSB_DEVCTL_VBUS) == (3 << MUSB_DEVCTL_VBUS_SHIFT))


I don't see why this has to be is implemented in the DSPS glue 
layer. The

DevCtl register is a standard MUSB one.


   Looking at the musb_platform_get_vbus_status(), it's unclear why it 
returns 0 if the vbus_status() method id undefined. I think we should 
read the DevCtl registre here, removing the FIXME at the call site.


Maybe it is worth to return -EINVAL from musb_platform_get_vbus_status 
in case of method is undefined and keep current logic of interface 
implementation in platform-specific driver.


Hence print "Vbus unknown" in musb_vbus_show() in case of -EINVAL from 
musb_platform_get_vbus_status


Regards,
Roman




MBR, Sergei



--
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: musb: dsps: implement vbus_status method

2015-10-08 Thread Sergei Shtylyov

Hello.

On 10/7/2015 5:40 PM, Roman Alyautdin wrote:


Implement vbus_status  method of musb_platform_ops that allows
musb_core to properly represent the VBUS status of musb_dsps devices in
corresponding sysfs entry

Signed-off-by: Roman Alyautdin 
---
  drivers/usb/musb/musb_dsps.c |   13 +
  1 file changed, 13 insertions(+)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 84512d1..9c00edf 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -632,6 +632,18 @@ static void dsps_read_fifo32(struct musb_hw_ep *hw_ep, u16 
len, u8 *dst)
}
  }

+static int dsps_musb_vbus_status(struct musb *musb)
+{
+   void __iomem *mregs = musb->mregs;
+   u8 devctl;
+
+   devctl = dsps_readb(mregs, MUSB_DEVCTL);
+   if ((devctl & MUSB_DEVCTL_VBUS) == (3 << MUSB_DEVCTL_VBUS_SHIFT))


   I don't see why this has to be is implemented in the DSPS glue layer. The 
DevCtl register is a standard MUSB one.


[...]

MBR, Sergei

--
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 v9 4/5] xhci: mediatek: support MTK xHCI host controller

2015-10-08 Thread chunfeng yun
On Thu, 2015-10-08 at 13:28 +0100, Daniel Thompson wrote:
> On 08/10/15 13:05, chunfeng yun wrote:
> > Hi,
> > On Thu, 2015-10-01 at 12:44 +0100, Daniel Thompson wrote:
> >> On 29/09/15 04:01, Chunfeng Yun wrote:
> >>> There some vendor quirks for MTK xhci host controller:
> >>> 1. It defines some extra SW scheduling parameters for HW
> >>> to minimize the scheduling effort for synchronous and
> >>> interrupt endpoints. The parameters are put into reseved
> >>> DWs of slot context and endpoint context.
> >>> 2. Its IMODI unit for Interrupter Moderation register is
> >>> 8 times as much as that defined in xHCI spec.
> >>> 3. Its TDS in  Normal TRB defines a number of packets that
> >>> remains to be transferred for a TD after processing all
> >>> Max packets in all previous TRBs.
> >>>
> >>> Signed-off-by: Chunfeng Yun 
> >>
> >> I've done some basic soak tests, both with a directly attached USB3 HDD
> >> and, given the extra code to manage isochronous xfer, also with a hub,
> >> disc and two audio interfaces.
> >>
> >> Tested-by: Daniel Thompson 
> >>
> >>
> >>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> >>> index 57f40a1..243f696 100644
> >>> --- a/drivers/usb/host/xhci-ring.c
> >>> +++ b/drivers/usb/host/xhci-ring.c
> >>> @@ -68,6 +68,7 @@
> >>>#include 
> >>>#include "xhci.h"
> >>>#include "xhci-trace.h"
> >>> +#include "xhci-mtk.h"
> >>>
> >>>/*
> >>> * Returns zero if the TRB isn't in this segment, otherwise it returns 
> >>> the DMA
> >>> @@ -3044,18 +3045,27 @@ static u32 xhci_td_remainder(struct xhci_hcd 
> >>> *xhci, int transferred,
> >>> struct urb *urb, unsigned int 
> >>> num_trbs_left)
> >>>{
> >>>   u32 maxp, total_packet_count;
> >>> + u32 skip_current_trb = 0;
> >>
> >> A bit of a nitpick but why do we need skip_current_trb? Testing
> >> (xhci->quirks & XHCI_MTK_HOST) twice would make what the code does, and
> >> why, more obvious.
> >>
> > I am not sure which way it is better, so add variable of
> > skip_current_trb to reduce test times.
> 
> I can't imagine either approach impacts performance. However introducing 
> skip_current_trb makes the flow of the branches harder to follow.
> 
> The first branch was a version check and can remain so; we only need to 
> do something special with the quirks because the MTK controller is 0.97 
> but with a few bits added:
> 
>  /* MTK xHCI is mostly 0.97 but contains some features from 1.0 */
>  if (xhci->hci_version < 0x100 && !(xhci->quirks & XHCI_MTK_HOST))
>   return ...
> 
> Setting trb_buff_len to zero is accommodating a quirk and this is 
> clearer when we have:
> 
>  /* for MTK xHCI, TD size doesn't include this TRB */
>  if (xhci->quirks & XHCI_MTK_HOST)
>   trb_buff_len = 0;
> 
I will revise the code according to your suggestion, thanks

> 
> >> Anyhow with that looked at, and the caveat that I'm not much of USB
> >> expert, you're welcome to add my Reviewed-by: to v10.
> >>
> >
> > Thanks a lot
> 
> No worries.
> 


--
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 v9 4/5] xhci: mediatek: support MTK xHCI host controller

2015-10-08 Thread chunfeng yun
Hi,
On Thu, 2015-10-08 at 16:05 +0300, Mathias Nyman wrote:
> On 08.10.2015 15:28, Daniel Thompson wrote:
> > On 08/10/15 13:05, chunfeng yun wrote:
> >> Hi,
> >> On Thu, 2015-10-01 at 12:44 +0100, Daniel Thompson wrote:
> >>> On 29/09/15 04:01, Chunfeng Yun wrote:
>  There some vendor quirks for MTK xhci host controller:
>  1. It defines some extra SW scheduling parameters for HW
>  to minimize the scheduling effort for synchronous and
>  interrupt endpoints. The parameters are put into reseved
>  DWs of slot context and endpoint context.
>  2. Its IMODI unit for Interrupter Moderation register is
>  8 times as much as that defined in xHCI spec.
>  3. Its TDS in  Normal TRB defines a number of packets that
>  remains to be transferred for a TD after processing all
>  Max packets in all previous TRBs.
> 
>  Signed-off-by: Chunfeng Yun 
> >>>
> >>> I've done some basic soak tests, both with a directly attached USB3 HDD
> >>> and, given the extra code to manage isochronous xfer, also with a hub,
> >>> disc and two audio interfaces.
> >>>
> >>> Tested-by: Daniel Thompson 
> >>>
> >>>
>  diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>  index 57f40a1..243f696 100644
>  --- a/drivers/usb/host/xhci-ring.c
>  +++ b/drivers/usb/host/xhci-ring.c
>  @@ -68,6 +68,7 @@
> #include 
> #include "xhci.h"
> #include "xhci-trace.h"
>  +#include "xhci-mtk.h"
> 
> /*
>  * Returns zero if the TRB isn't in this segment, otherwise it 
>  returns the DMA
>  @@ -3044,18 +3045,27 @@ static u32 xhci_td_remainder(struct xhci_hcd 
>  *xhci, int transferred,
>   struct urb *urb, unsigned int num_trbs_left)
> {
> u32 maxp, total_packet_count;
>  +u32 skip_current_trb = 0;
> >>>
> >>> A bit of a nitpick but why do we need skip_current_trb? Testing
> >>> (xhci->quirks & XHCI_MTK_HOST) twice would make what the code does, and
> >>> why, more obvious.
> >>>
> >> I am not sure which way it is better, so add variable of
> >> skip_current_trb to reduce test times.
> >
> > I can't imagine either approach impacts performance. However introducing 
> > skip_current_trb makes the flow of the branches harder to follow.
> >
> > The first branch was a version check and can remain so; we only need to do 
> > something special with the quirks because the MTK controller is 0.97 but 
> > with a few bits added:
> >
> >  /* MTK xHCI is mostly 0.97 but contains some features from 1.0 */
> >  if (xhci->hci_version < 0x100 && !(xhci->quirks & XHCI_MTK_HOST))
> >  return ...
> >
> > Setting trb_buff_len to zero is accommodating a quirk and this is clearer 
> > when we have:
> >
> >  /* for MTK xHCI, TD size doesn't include this TRB */
> >  if (xhci->quirks & XHCI_MTK_HOST)
> >  trb_buff_len = 0;
> 
> Both will work, but I'd prefer this solution as well.
> 
Thanks for your suggestion as well

> -Mathias
> 


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


4758dcd19a7d9ba9610b38fecb93f65f56f86346 - usb: xhci: Add support for URB_ZERO_PACKET to bulk/sg transfers; into stable?

2015-10-08 Thread Oliver Neukum
Hi,

it looks to me like

commit 4758dcd19a7d9ba9610b38fecb93f65f56f86346
Author: Reyad Attiyat 
Date:   Thu Aug 6 19:23:58 2015 +0300

usb: xhci: Add support for URB_ZERO_PACKET to bulk/sg transfers

should go into stable. Is there a special reason it has no CC?

Regards
Oliver


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


Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

2015-10-08 Thread Pavel Machek
HI!

> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = raw_notifier_chain_unregister(>nh, nb);
> 
> Greg, this is the kind of thing I wanted to avoid adding more of.
> 
> I was wondering if you would accept subsystems using kdbus for
> this sort of notification. I'm okay waiting for kdbus for another
> couple merge windows (if we have to) before that's merged, but
> if we take this raw notifier approach now, we will end up having
> to support it forever.
> 
> Also, because soon enough we will have to support USB Power Delivery
> with Type C connector, this is bound to change in the coming months.
> 
> Frankly, I wanted all of this to be decided in userland with the
> kernel just providing notification and basic safety checks (we don't
> want to allow a bogus userspace daemon frying anybody's devices).
> 
> How would you feel about that ?

So init=/bin/bash boot no longer provides machine that charges itself?

That would be bad. Traditionally, hardware controls battery charging,
and if hardware needs some help, we should do it in kernel, to mask
the difference from userspace.
Pavel
--
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: [Device-mainlining] [PATCH v2 2/3] gadget: Introduce the usb charger framework

2015-10-08 Thread Pavel Machek
Hi!

> On 08/16/2015 08:03 PM, Baolin Wang via device-mainlining wrote:
> > On 14 August 2015 at 23:27, Greg KH  wrote:
> >> On Fri, Aug 14, 2015 at 05:47:45PM +0800, Baolin Wang wrote:
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or modify
> >>> + * it under the terms of the GNU General Public License as published by
> >>> + * the Free Software Foundation; either version 2 of the License, or
> >>> + * (at your option) any later version.
> >>
> >> I have to ask, do you really mean "any later version"?
> >>
> > 
> > I think I did not get your point, could you explain it detailedly?
> 
> The full kernel is licensed under v2 of the GPL only, not "any later version".
> See the second paragraph at the top of the COPYING file in the root directory
> of the kernel source tree. There are differences on individual files, but
> having this file allow "any later version" makes it different from much of
> rest of the kernel.
> 
> Unless you have a specific reason to allow greater-than-V2 GPL licensing on 
> this
> file, you should change the licensing clause.  The following wording appears
> to be pretty popular:
> 
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> * published by the Free Software Foundation.

Please, keep it V2 or later, if you can. It makes sharing code with U-Boot (for
example) easier.

There's long tradition of "V2 or later" code in the kernel.

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


Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

2015-10-08 Thread Pavel Machek
Hi!

> > What's the advantage of pushing this to userspace?  By the time we
> > provide enough discoverability to enable userspace to configure itself
> > it seems like we'd have enough information to do the job anyway.
> 
> you're going to be dealing with a setup where each vendor does one thing
> differently. Some will have it all in the SoC part of a single IP (dwc3 can be
> configured that way), some will push it out to companion IC, some might even 
> use
> some mostly discrete setup and so on...
> 
> We're gonna be dealing with a decision that involves information from multiple
> subsystems (USB, regulator, hwmon, power supply to name a few).
> 
> We tried doing it all in the kernel back in N800, N810 and N900/N9 days and 
> it's
> just plain difficult. To make matters worse, N900 had two USB PHYs, one for
> actual runtime use and another just for detecting the charger type on the 
> other
> end.

N900 does charging from kernel these days.

Not being able to boot generic distribution would be regression there...


Pavel
--
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: [Device-mainlining] [PATCH v2 2/3] gadget: Introduce the usb charger framework

2015-10-08 Thread Greg KH
On Thu, Oct 08, 2015 at 05:50:31PM +0200, Pavel Machek wrote:
> Hi!
> 
> > On 08/16/2015 08:03 PM, Baolin Wang via device-mainlining wrote:
> > > On 14 August 2015 at 23:27, Greg KH  wrote:
> > >> On Fri, Aug 14, 2015 at 05:47:45PM +0800, Baolin Wang wrote:
> > >>> + *
> > >>> + * This program is free software; you can redistribute it and/or modify
> > >>> + * it under the terms of the GNU General Public License as published by
> > >>> + * the Free Software Foundation; either version 2 of the License, or
> > >>> + * (at your option) any later version.
> > >>
> > >> I have to ask, do you really mean "any later version"?
> > >>
> > > 
> > > I think I did not get your point, could you explain it detailedly?
> > 
> > The full kernel is licensed under v2 of the GPL only, not "any later 
> > version".
> > See the second paragraph at the top of the COPYING file in the root 
> > directory
> > of the kernel source tree. There are differences on individual files, but
> > having this file allow "any later version" makes it different from much of
> > rest of the kernel.
> > 
> > Unless you have a specific reason to allow greater-than-V2 GPL licensing on 
> > this
> > file, you should change the licensing clause.  The following wording appears
> > to be pretty popular:
> > 
> > * This program is free software; you can redistribute it and/or modify
> > * it under the terms of the GNU General Public License version 2 as
> > * published by the Free Software Foundation.
> 
> Please, keep it V2 or later, if you can. It makes sharing code with U-Boot 
> (for
> example) easier.
> 
> There's long tradition of "V2 or later" code in the kernel.

And there's even longer "v2 only" tradition as well.  It's up to the
person / company submitting the code to pick the license for it, let
them make the decision please.

thanks,

greg k-h
--
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: Regression: USB OTG port breaks after a few hours in host mode on iMX6

2015-10-08 Thread Russell King - ARM Linux
On Thu, Oct 08, 2015 at 09:52:52AM +, Peter Chen wrote:
> I can't reproduce it for 5 hours, and will change pinmux (do the same
> thing with your platform), and do the overnight test.

There's definitely something weird going on.  Over night last night,
I left the Logitech universal receiver in the port, and this morning
it was indicating in /proc/interrupts:

283: 50  0  0  0   GPC  43 Edge  
2184000.usb

I removed it, and now I have:

283: 50  0   1716  0   GPC  43 Edge  
2184000.usb

which is increasing at a rate of 90 per minute.

Nothing in the kernel message log indicating why this may be.  It looks
like runtime PM doesn't work on this port:

/sys/bus/platform/devices/2184000.usb/power/runtime_active_time:109850496
/sys/bus/platform/devices/2184000.usb/power/runtime_status:active
/sys/bus/platform/devices/2184000.usb/power/runtime_suspended_time:0

whereas the other port (which has zero interrupts) it does:

/sys/bus/platform/devices/2184200.usb/power/runtime_active_time:16924
/sys/bus/platform/devices/2184200.usb/power/runtime_status:suspended
/sys/bus/platform/devices/2184200.usb/power/runtime_suspended_time:109861760

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Device-mainlining] [PATCH v2 2/3] gadget: Introduce the usb charger framework

2015-10-08 Thread Pavel Machek
Hi!

> > > * This program is free software; you can redistribute it and/or modify
> > > * it under the terms of the GNU General Public License version 2 as
> > > * published by the Free Software Foundation.
> > 
> > Please, keep it V2 or later, if you can. It makes sharing code with U-Boot 
> > (for
> > example) easier.
> > 
> > There's long tradition of "V2 or later" code in the kernel.
> 
> And there's even longer "v2 only" tradition as well.  It's up to the
> person / company submitting the code to pick the license for it, let
> them make the decision please.

You wrote:

>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>
>I have to ask, do you really mean "any later version"?

Given that you are quite high-level maintainer, that does not sound exactly
like "let them make the decision". It sounds like a pressure to change the
license to the one you liked. WHy did you have to ask?

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
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 3/5] CHROMIUM: usb: dwc2: Avoid double-reset at boot time

2015-10-08 Thread Doug Anderson
Hi,

On Wed, Oct 7, 2015 at 5:48 PM, Douglas Anderson  wrote:
> In (usb: dwc2: reset dwc2 core before dwc2_get_hwparams()) we added an
> extra reset to the probe path for the dwc2 USB controllers.  This
> allowed proper detection of parameters even if the firmware had already
> used the USB part.
>
> Unfortunately, this extra reset is quite slow and is affecting boot
> speed.  We can avoid the double-reset by skipping the extra reset that
> would happen just after the one we added.  Logic that explains why this
> is safe:
>
> * As of the CL mentioned above, we now always call dwc2_core_reset() in
>   dwc2_driver_probe() before dwc2_hcd_init().
>
> * The only caller of dwc2_hcd_init() is dwc2_driver_probe(), so we're
>   guaranteed that dwc2_core_reset() was called before dwc2_hdc_init().
>
> * dwc2_hdc_init() is the only caller that passes an irq other than -1 to
>   dwc2_core_init().  Thus if dwc2_core_init() is called with an irq
>   other than -1 we're guaranteed that dwc2_core_reset was called before
>   dwc2_core_init().
>
> ...this allows us to remove the dwc2_core_reset() in dwc2_core_init() if
> irq is not < 0.
>
> Note that since "irq" wasn't used in the function dwc2_core_init()
> anyway and since select_phy was always set at exactly the same times we
> could avoid the reset, we remove "irq" and rename "select_phy" to
> "initial_setup" and adjust the callers accordingly.
>
> Signed-off-by: Douglas Anderson 
> ---
>  drivers/usb/dwc2/core.c | 29 ++---
>  drivers/usb/dwc2/core.h |  2 +-
>  drivers/usb/dwc2/hcd.c  |  6 +++---
>  3 files changed, 22 insertions(+), 15 deletions(-)

Obviously I stupidly forgot to remove the "CHROMIUM" prefix on this
patch.  :(  Sorry about that.  I can repost if necessary...

-Doug
--
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: [Device-mainlining] [PATCH v2 2/3] gadget: Introduce the usb charger framework

2015-10-08 Thread Greg KH
On Thu, Oct 08, 2015 at 07:04:05PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > * This program is free software; you can redistribute it and/or modify
> > > > * it under the terms of the GNU General Public License version 2 as
> > > > * published by the Free Software Foundation.
> > > 
> > > Please, keep it V2 or later, if you can. It makes sharing code with 
> > > U-Boot (for
> > > example) easier.
> > > 
> > > There's long tradition of "V2 or later" code in the kernel.
> > 
> > And there's even longer "v2 only" tradition as well.  It's up to the
> > person / company submitting the code to pick the license for it, let
> > them make the decision please.
> 
> You wrote:
> 
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; either version 2 of the License, or
> >> + * (at your option) any later version.
> >
> >I have to ask, do you really mean "any later version"?
> 
> Given that you are quite high-level maintainer, that does not sound exactly
> like "let them make the decision". It sounds like a pressure to change the
> license to the one you liked. WHy did you have to ask?

I have to "ask" because I need to ensure that people put thought into
the license they chose instead of blindly cut-and-pasting it from a
random web site.  Companies all have policies on what the license of the
kernel code they produce are, and by nicely asking new developers this
question, it ensures that they are picking the license in a thoughtful
manner.

If they pick v2 vs. v2+ is of no opinion of mine, either is just
wonderful.

thanks,

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


[PATCH] usb: dwc2: Avoid more calls to dwc2_core_reset()

2015-10-08 Thread Douglas Anderson
Calls to dwc2_core_reset() are currently very slow, taking at least
150ms (possibly more).  It behooves us to take as many of these calls
out as possible.

It turns out that the calls in dwc2_fs_phy_init() and dwc2_hs_phy_init()
should (as documented in the code) only be needed if we need to do a PHY
SELECT.  That means that if we see that we can avoid the PHY SELECT then
we can avoid the reset.

This patch appears to successfully bypass two resets (one per USB
device) on rk3288-based ARM Chromebooks.

Signed-off-by: Douglas Anderson 
---
This patch should really be appended to the end of the 5-part series
ending at  (making it a
6-part series).  I realized this additional speedup last night upon
thinking more about the series and tested it today.

 drivers/usb/dwc2/core.c | 40 +++-
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index bf5e951..ec459e5 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -553,16 +553,20 @@ static int dwc2_fs_phy_init(struct dwc2_hsotg *hsotg, 
bool select_phy)
 */
if (select_phy) {
dev_dbg(hsotg->dev, "FS PHY selected\n");
+
usbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
-   usbcfg |= GUSBCFG_PHYSEL;
-   dwc2_writel(usbcfg, hsotg->regs + GUSBCFG);
+   if (!(usbcfg & GUSBCFG_PHYSEL)) {
+   usbcfg |= GUSBCFG_PHYSEL;
+   dwc2_writel(usbcfg, hsotg->regs + GUSBCFG);
 
-   /* Reset after a PHY select */
-   retval = dwc2_core_reset(hsotg);
-   if (retval) {
-   dev_err(hsotg->dev, "%s() Reset failed, aborting",
-   __func__);
-   return retval;
+   /* Reset after a PHY select */
+   retval = dwc2_core_reset(hsotg);
+
+   if (retval) {
+   dev_err(hsotg->dev,
+   "%s: Reset failed, aborting", __func__);
+   return retval;
+   }
}
}
 
@@ -597,13 +601,13 @@ static int dwc2_fs_phy_init(struct dwc2_hsotg *hsotg, 
bool select_phy)
 
 static int dwc2_hs_phy_init(struct dwc2_hsotg *hsotg, bool select_phy)
 {
-   u32 usbcfg;
+   u32 usbcfg, usbcfg_old;
int retval = 0;
 
if (!select_phy)
return 0;
 
-   usbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
+   usbcfg = usbcfg_old = dwc2_readl(hsotg->regs + GUSBCFG);
 
/*
 * HS PHY parameters. These parameters are preserved during soft reset
@@ -631,14 +635,16 @@ static int dwc2_hs_phy_init(struct dwc2_hsotg *hsotg, 
bool select_phy)
break;
}
 
-   dwc2_writel(usbcfg, hsotg->regs + GUSBCFG);
+   if (usbcfg != usbcfg_old) {
+   dwc2_writel(usbcfg, hsotg->regs + GUSBCFG);
 
-   /* Reset after setting the PHY parameters */
-   retval = dwc2_core_reset(hsotg);
-   if (retval) {
-   dev_err(hsotg->dev, "%s() Reset failed, aborting",
-   __func__);
-   return retval;
+   /* Reset after setting the PHY parameters */
+   retval = dwc2_core_reset(hsotg);
+   if (retval) {
+   dev_err(hsotg->dev,
+   "%s: Reset failed, aborting", __func__);
+   return retval;
+   }
}
 
return retval;
-- 
2.6.0.rc2.230.g3dd15c0

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


[PATCH] USB: serial: cp210x: Workaround for cp2108 failure due to GET_LINE_CTL bug

2015-10-08 Thread Konstantin Shkolnyy
cp2108 GET_LINE_CTL returns the 16-bit value with the 2 bytes swapped.
However, SET_LINE_CTL functions properly. When the driver tries to modify
the register, it reads it, modifies some bits and writes back. Because the
read bytes were swapped, this often results in an invalid value to be written.
In turn, this causes cp2108 respond with a stall. The stall sometimes doesn't
clear properly and cp2108 starts responding to following valid commands also
with stalls, effectively failing.

Signed-off-by: Konstantin Shkolnyy 
---
 drivers/usb/serial/cp210x.c | 58 +
 1 file changed, 58 insertions(+)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index eac7cca..060a1e8 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -199,6 +199,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 
 struct cp210x_serial_private {
__u8bInterfaceNumber;
+   boolswap_get_line_ctl; /* set if CP2108 swaps bytes 
due to a bug */
 };
 
 static struct usb_serial_driver cp210x_device = {
@@ -343,6 +344,28 @@ static int cp210x_get_config(struct usb_serial_port *port, 
u8 request,
return result;
}
 
+   /* Workaround for swapped bytes in 16-bit value from 
CP210X_GET_LINE_CTL */
+   if (spriv->swap_get_line_ctl && request == CP210X_GET_LINE_CTL && size 
== 2)
+   swab16s((u16 *)data);

return 0;
 }
 
@@ -865,18 +888,53 @@ static void cp210x_break_ctl(struct tty_struct *tty, int 
break_state)
 
 static int cp210x_startup(struct usb_serial *serial)
 {
+   struct usb_serial_port *port;
struct usb_host_interface *cur_altsetting;
struct cp210x_serial_private *spriv;
+   unsigned int  line_ctl;
+   int   err;
+
+   /* We always expect a single port only */
+   if (serial->num_ports != 1) {
+   dev_err(>dev->dev, "%s - expected 1 port, found %d\n",
+   __func__, serial->num_ports);
+   return -EINVAL;
+   }
+   port = serial->port[0];
 
spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
if (!spriv)
return -ENOMEM;
 
+   /* get_config and set_config rely on this spriv field */
cur_altsetting = serial->interface->cur_altsetting;
spriv->bInterfaceNumber = cur_altsetting->desc.bInterfaceNumber;
 
+   /* Detect CP2108 bug and activate workaround.
+* Write a known good value 0x800, read it back.
+* If it comes back swapped the bug is detected.
+*/
+
+   /* The following get_config won't swap the bytes */
+   spriv->swap_get_line_ctl = false;
+
+   /* must be set before calling get_config and set_config */
usb_set_serial_data(serial, spriv);
 
+   line_ctl = 0x800;
+
+   err = cp210x_set_config(port, CP210X_SET_LINE_CTL, _ctl, 2);
+   if (err)
+   return err;
+
+   err = cp210x_get_config(port, CP210X_GET_LINE_CTL, _ctl, 2);
+   if (err)
+   return err;
+
+   if ((line_ctl & 0x) == 8)
+   /* Future get_config calls will swap the bytes */
+   spriv->swap_get_line_ctl = true;
+
return 0;
 }
 
-- 
1.9.1

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


Re: Regression: USB OTG port breaks after a few hours in host mode on iMX6

2015-10-08 Thread Peter Chen
On Thu, Oct 08, 2015 at 05:16:06PM +0100, Russell King - ARM Linux wrote:
> On Thu, Oct 08, 2015 at 09:52:52AM +, Peter Chen wrote:
> > I can't reproduce it for 5 hours, and will change pinmux (do the same
> > thing with your platform), and do the overnight test.
> 

I still not reproduce it.

> There's definitely something weird going on.  Over night last night,
> I left the Logitech universal receiver in the port, and this morning
> it was indicating in /proc/interrupts:
> 
> 283: 50  0  0  0   GPC  43 Edge  
> 2184000.usb
> 
> I removed it, and now I have:
> 
> 283: 50  0   1716  0   GPC  43 Edge  
> 2184000.usb
> 
> which is increasing at a rate of 90 per minute.

It is strange your interrupt occurs at cpu2 at that time, and
so many interrupts during the removal. In order to avoid other
interrupts occurring during the removal, would you please change
your dts like below:

 {
vbus-supply = <_usb_otg_vbus>;
dr_mode = "host";
disable-over-current;
status = "okay";
};

> 
> Nothing in the kernel message log indicating why this may be.  It looks
> like runtime PM doesn't work on this port:
> 
> /sys/bus/platform/devices/2184000.usb/power/runtime_active_time:109850496
> /sys/bus/platform/devices/2184000.usb/power/runtime_status:active
> /sys/bus/platform/devices/2184000.usb/power/runtime_suspended_time:0
> 

This may be correct, assume your usb receiver does not support runtime
pm, so the controller is active when it is on the port, when the
removal, the interrupt occurs so frequently, it has no chance to enter
suspend status.

> whereas the other port (which has zero interrupts) it does:
> 
> /sys/bus/platform/devices/2184200.usb/power/runtime_active_time:16924
> /sys/bus/platform/devices/2184200.usb/power/runtime_status:suspended
> /sys/bus/platform/devices/2184200.usb/power/runtime_suspended_time:109861760
> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

-- 

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


Re: Mass Storage Gadget Kthread

2015-10-08 Thread Peter Chen
On Fri, Oct 02, 2015 at 11:05:28AM -0500, Felipe Balbi wrote:
> Hi Alan,
> 
> here's a question which I hope you can help me understand :-)
> 
> Why do we have that kthread for the mass storage gadgets ? I noticed a very
> interesting behavior.
> 
> Basically, the MSC class works in a loop like so:
> 
> CBW
> Data Transfer
> CSW
> 
> In our implemention, what we do is:
> 
> CBW
> wake_up_process()
> Data Transfer
> wake_up_process()
> CSW
> wake_up_process()
> 
> Now here's the interesting bit. Every time we wake_up_process(), we basically
> don't do anything until MSC's kthread gets finally scheduled and has a chance 
> of
> doing its job. This means that the host keeps sending us tokens but the UDC
> doesn't have any request queued to start a transfer. This happens specially 
> with
> IN endpoints, not so much on OUT directions. See figure one [1] we can see 
> that
> host issues over 7 POLLs before UDC has finally started a usb_request, 
> sometimes
> this goes for even longer (see image [3]).
> 
> On figure two we can see that on this particular session, I had as much as 15%
> of the bandwidth wasted on POLLs. With this current setup I'm 34MB/sec and 
> with
> the added 15% that would get really close to 40MB/sec.
> 
> So the question is, why do we have to wait for that kthread to get scheduled ?
> Why couldn't we skip it completely ? Is there really anything left in there 
> that
> couldn't be done from within usb_request->complete() itself ?
> 
> I'll spend some time on that today and really dig that thing up, but if you 
> know
> the answer off the top of your head, I'd be happy to hear.
> 

To get the best performance, you may try to see as least IN-NAKs
or NYET-PINGs as possible within uFrame, the software should
always queue the enough requests, and hardware FIFO should always
be ready before host sends the token.

>From my test (usbtest & g_loopback), with the best situations,
the chipidea hardware can get 10 transactions for OUT and 11
transactions for IN within uFrame, and chipidea leaves a little more
space before end of SoF for last transaction, so the other hardware
may get 11 transactions for OUT and 12 transactions for IN within
uFrame (I see it at Intel platform), so from what I see, the best
performance at Linux for bulk is 44MB for OUT and 48MB for IN.

-- 

Best Regards,
Peter Chen
--
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: iperf UDP packet loss with Chipidea HDRC

2015-10-08 Thread Jayan John
On Thu, Oct 8, 2015 at 1:37 PM, Peter Chen  wrote:
> On Mon, Oct 05, 2015 at 07:27:23PM +0530, Jayan John wrote:
>> We are developing a custom USB device on a iMX6q platform with a Chipidea
>> HDRC. The device uses a single NCM interface for communication with another
>> OTG device i.e. Chipidea HDRC again. I see very poor iperf UDP performance
>> after role reversal with iperf server running on gadget.
>>
>> Kernel: 3.10.17 using Wandboard config
>>
>> Test sequence:
>> 1. Connect both boards (board A and board B) using OTG cable
>> 2. Board A is assigned as A device and board B is assigned as B device
>> 3. B device initiates role reversal
>> echo -n host > /sys/kernel/debug/ci_hdrc.0/role
>> 4. A device switches to gadget mode
>> echo -n gadget > /sys/kernel/debug/ci_hdrc.0/role
>> 5. Assign IPV6 address on board A (now gadget).. ifconfig usb0 2012::1/64 up
>> 6. Assign IPV6 address on board B (now host).. ifconfig usb0 2012::2/64 up
>> 7. Run iperf server on board A.. iperf -V -s -u
>> 8. Run iperf client on board B.. iperf -u -t 10 -V -c 2012::1 -b 150M
>>
>> iperf server logs:
>> [  4] local fe80::6cc9:b6ff:fec5:be28 port 5001 connected with
>> fe80::54e0:86ff:fea6:8987 port 35629
>> [  4]  0.0-10.0 sec  63.7 MBytes  53.4 Mbits/sec 0.171 ms 61701/107109 (58%)
>> [  4]  0.0-10.0 sec  1 datagrams received out-of-order
>> [  3] local fe80::6cc9:b6ff:fec5:be28 port 5001 connected with
>> fe80::54e0:86ff:fea6:8987 port 33609
>> [  3]  0.0-10.0 sec  62.1 MBytes  52.1 Mbits/sec 0.215 ms 62551/106825 (59%)
>> [  3]  0.0-10.0 sec  1 datagrams received out-of-order
>>
>> Query:
>> 1. The UDP packet loss is seen only in the case of iperf server on gadget
>> after role reversal. I see there are patches (12) from Peter for performance
>> improvements on the newer kernels. Will they help?
>
> If you only see this problem after role switch, compare the controller
> registers may help it.
>
>> 2. The issue is not seen if UDP socket buffer size/ datagram size on iperf is
>> increased i.e. iperf -u -t 10 -l 32k -V -c 2012::1 -b 150M. Is this only
>> hiding an issue with Chipidea driver e.g. TXFIFO or burst size?
>> 3. Should ncm be tweaked to have different buffer size or NTB handling?
>
> NCM stack may be updated from v3.10, I just tried (without role switch), it
> can get 110Mbps, and about 3% loss rate using your command at v4.3-rc1.
>
> --
>
> Best Regards,
> Peter Chen

Thanks Peter, I will try that. I saw the packet loss only after role
switch.

I will close this thread for now, as I intend to make a kernel upgrade
before I continue investigation on this issue.

I will update the community/ push a patch when I root cause/ fix this.

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