RE: [balbi-usb:testing/next 48/49] drivers/usb//gadget/function/u_serial.c:540:19: error: invalid operands to binary & (have 'unsigned int' and 'struct tty_struct *')

[Stephen Warren]

Felipe Balbi wrote at Friday, August 18, 2017 3:30 AM:
> Hi,
> 
> kbuild test robot  writes:
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
> > testing/next
> > head:   862b43460309779e5814616a68aa36d29a70d6e0
> > commit: 2b4e4d3c929db0fc7cf99212bfac0570b6902c14 [48/49] usb: gadget: 
> > serial: fix oops when data rx'd after close
> > config: xtensa-allmodconfig (attached as .config)
> > compiler: xtensa-linux-gcc (GCC) 4.9.0
> > reproduce:
> > wget 
> > https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
> > ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > git checkout 2b4e4d3c929db0fc7cf99212bfac0570b6902c14
> > # save the attached .config to linux build tree
> > make.cross ARCH=xtensa
> >
> > All errors (new ones prefixed by >>):
> >
> >drivers/usb//gadget/function/u_serial.c: In function 'gs_rx_push':
> >>> drivers/usb//gadget/function/u_serial.c:540:19: error: invalid operands 
> >>> to binary & (have 'unsigned int' and 'struct tty_struct *')
> >   if (req->actual & tty) {
> 
> fixed locally.

Sorry about that, and thanks for fixing it. When I moved this between kernel
trees I re-implemented it since it was so trivial, rather than copying the
patch via git mechanisms. That's obviously a very bad idea:-(

-- 
nvpublic
--
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: gadget: serial: fix oops when data rx'd after close

[Stephen Warren]

From: Stephen Warren <swar...@nvidia.com>

When the gadget serial device has no associated TTY, do not pass any
received data into the TTY layer for processing; simply drop it instead.
This prevents the TTY layer from calling back into the gadget serial
driver, which will then crash in e.g. gs_write_room() due to lack of
gadget serial device to TTY association (i.e. a NULL pointer dereference).

Signed-off-by: Stephen Warren <swar...@nvidia.com>
---
 drivers/usb/gadget/function/u_serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/u_serial.c 
b/drivers/usb/gadget/function/u_serial.c
index 9b0805f55ad7..16bb24a047d9 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -537,7 +537,7 @@ static void gs_rx_push(unsigned long _port)
}
 
/* push data to (open) tty */
-   if (req->actual) {
+   if (req->actual & tty) {
char*packet = req->buf;
unsignedsize = req->actual;
unsignedn;
-- 
2.14.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

gadget serial oops in gs_write_room when data rx'd after close

[Stephen Warren]

Felipe,

I've found a problem in the gadget serial driver, which I believe is 
triggered in the following scenario:


1) Both host and gadget system have a USB gadget serial port open.

2) Gadget side application closes the serial port.

3) Host side sends data to the serial port.

(Steps 2, 3 may need to happen at almost the same time and/or perhaps in 
the opposite order)


4) Gadget side passes the received data to the TTY layer even though the 
port is closed locally, which soon results in an oops in gs_write_room 
since port is NULL in the following:


struct gs_port  *port = tty->driver_data;
...
spin_lock_irqsave(>port_lock, flags);


The exact oops I see is:


[  132.757406] Unable to handle kernel NULL pointer dereference at virtual 
address 0170
[  132.757409] pgd = ffc001481000
[  132.757414] [0170] *pgd=00026cdd6003, *pud=00026cdd6003, 
*pmd=00026cdd7003, *pte=00e803881707
[  132.757418] Internal error: Oops: 9606 [#1] PREEMPT SMP
[  132.757422] Modules linked in: bcmdhd pci_tegra bluedroid_pm
[  132.757428] CPU: 5 PID: 53 Comm: kworker/u12:1 Not tainted 4.4.38-tegra #1
[  132.757429] Hardware name: quill (DT)
[  132.757439] Workqueue: events_unbound flush_to_ldisc
[  132.757441] task: ffc1eb660c80 ti: ffc1eb6a8000 task.ti: 
ffc1eb6a8000
[  132.757447] PC is at _raw_spin_lock_irqsave+0x20/0x54
[  132.757452] LR is at gs_write_room+0x1c/0x7c
[  132.757453] pc : [] lr : [] pstate: 
80c5
[  132.757454] sp : ffc1eb6abbe0
[  132.757456] x29: ffc1eb6abbe0 x28: ffc07aea9000 
[  132.757458] x27: 0003 x26: ff8015209000 
[  132.757460] x25:  x24: ff8015209000 
[  132.757462] x23:  x22: ffc07aea9000 
[  132.757463] x21: ffc1db1cf820 x20: 0170 
[  132.757465] x19:  x18: 004a4000 
[  132.757466] x17: bc7817d9 x16: ffc000b37a60 
[  132.757468] x15: ffc000b37a60 x14: 07a1 
[  132.757470] x13: ffc1db1cf823 x12: 00012a38 
[  132.757471] x11: ffc000b379d0 x10: 000d 
[  132.757473] x9 : ffc1eb6abd20 x8 : ffc1db1cf823 
[  132.757474] x7 :  x6 :  
[  132.757476] x5 : ffc07aea9238 x4 : 0001 
[  132.757477] x3 : 0001 x2 : 0040 
[  132.757479] x1 : ffc1eb6a8000 x0 : 0170 
[  132.757479] 
[  132.757481] Process kworker/u12:1 (pid: 53, stack limit = 0xffc1eb6a8020)

[  132.757482] Call trace:
[  132.757484] [] _raw_spin_lock_irqsave+0x20/0x54
[  132.757487] [] tty_write_room+0x1c/0x2c
[  132.757489] [] __process_echoes+0x24/0x25c
[  132.757491] [] n_tty_receive_buf_common+0x1ec/0xa24
[  132.757493] [] n_tty_receive_buf2+0x10/0x18
[  132.757495] [] flush_to_ldisc+0xe4/0x154
[  132.757499] [] process_one_work+0x154/0x434
[  132.757501] [] worker_thread+0x134/0x40c
[  132.757503] [] kthread+0xe0/0xf4
[  132.757506] [] ret_from_fork+0x10/0x40
[  132.757508] ---[ end trace 5f1b8572ec9b9a44 ]---
[  132.758707] note: kworker/u12:1[53] exited with preempt_count 1


The patch below fixes this in my testing:


diff --git a/drivers/usb/gadget/function/u_serial.c 
b/drivers/usb/gadget/function/u_serial.c
index 9b0805f55ad7..16bb24a047d9 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -537,7 +537,7 @@ static void gs_rx_push(unsigned long _port)
}
 
 		/* push data to (open) tty */

-   if (req->actual) {
+   if (req->actual & tty) {
char*packet = req->buf;
unsignedsize = req->actual;
unsignedn;


However, I'm not sure if that patch is entirely complete/correct. Can 
you comment? In particular:


1) It causes any data that's received while the port is closed to be 
simply thrown away. I assume this is expected behaviour, as opposed to 
e.g. buffering the data up until the port is opened later. I note that 
gs_rx_push does buffer data in the tty_throttled() case, but that's a 
different case than port closed.


2) gs_rx_push() still calls gs_start_rx() even if there's no TTY 
attached to the gadget serial device. I wonder if it should stop 
submitting USB requests until the port is open again? Perhaps the 
current behaviour is correct since the host will be upset if the gadget 
refuses to read the data it's sending.


3) I'm not convinced that the patch above completely closes the race 
that triggers the oops. It should fully solve the issue if the steps I 
wrote at the very top of this email occur in order, but I think there's 
still a possible race if the set of steps below occur. Should gs_close() 
(or some related/child function) do something to synchronously drain the 
RX queue or RX line discipline, just like it already does to drain 
port_write_buf?


Possible remaining race window:

1) Both host and gadget system 

Re: [PATCH v1 4/9] ARM: tegra: Enable UDC on TrimSlice

[Stephen Warren]

On 07/06/2017 11:16 AM, Dmitry Osipenko wrote:

On 06.07.2017 19:46, Stephen Warren wrote:

On 07/05/2017 07:24 PM, Peter Chen wrote:

On Wed, Jul 05, 2017 at 08:19:53PM +0300, Dmitry Osipenko wrote:

From: Thierry Reding <tred...@nvidia.com>

Override the compatible string of the first USB controller to enable
device mode.

Signed-off-by: Thierry Reding <tred...@nvidia.com>
---
   arch/arm/boot/dts/tegra20-trimslice.dts | 2 ++
   1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/tegra20-trimslice.dts
b/arch/arm/boot/dts/tegra20-trimslice.dts
index b902ab594afa..96b4f3f9827b 100644
--- a/arch/arm/boot/dts/tegra20-trimslice.dts
+++ b/arch/arm/boot/dts/tegra20-trimslice.dts
@@ -336,7 +336,9 @@
   };
 usb@c500 {
+compatible = "nvidia,tegra20-udc";
   status = "okay";
+dr_mode = "otg";


If this board supports peripheral-only, you need to
set dr_mode as "peripheral".


I agree here. Even if the ports can theoretically do otg, and DT is supposed to
represent HW not SW, given we have no SW support for actual OTG, it'd be best
not to change the DT to OTG until it's actually useful to do so. This comment
applies to the patches I ack'd already too.


In case of absence of the "dr_mode" option or without a support of host mode
switching, UDC driver fallbacks to the peripheral mode. Given that DT is
supposed to be stable, I'm not sure what to do... should I specify the
peripheral mode for now (which doesn't represent the HW) or completely remove
the "dr_mode"?


I think specifying dr_mode="peripheral" now is fine; we can change it later.

DT ABI compatibility isn't about never changing the DT content, it's 
about making sure that any schema changes occur in a 
backwards-compatible fashion.


Also in the case of a USB port that does support OTG, the dr_mode 
property represents both (a) the HW's capabilities, and (b) the user's 
desired (initial, if e.g. sysfs modification is possible) mode of 
operation if the HW has multiple capabilities. Given (b), I think it's 
fine to specify peripheral mode for now, and allow the user to change it 
back to host or OTG mode if they want (although OTG mode isn't 
useful/valid until SW support exists).

--
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 v1 8/9] ARM: tegra: Enable UDC on AC100

[Stephen Warren]

On 07/06/2017 10:57 AM, Dmitry Osipenko wrote:

On 06.07.2017 19:38, Stephen Warren wrote:

On 07/05/2017 11:19 AM, Dmitry Osipenko wrote:

Override the compatible string of the first USB controller to enable
device mode.



diff --git a/arch/arm/boot/dts/tegra20-paz00.dts
b/arch/arm/boot/dts/tegra20-paz00.dts



   usb@c500 {
+compatible = "nvidia,tegra20-udc";
   status = "okay";
+dr_mode = "otg";
   };


This port is the only external USB port on the AC100.


Seems that's not true. http://elinux.org/Tegra/Boards/Toshiba_AC100 claims that
there are host and micro USB ports.


You're right; I was getting this board confused with the Springbank 
variety of Seaboard, and when I looked at the mini USB port on the AC100 
I thought it was something else.


Given the physical connector on this USB port on the AC100, it's 
probably fine to convert it to device-mode only since I don't expect 
anyone has a USB OTG cable that has a mini size connector on the host end.


It looks like this port is only enabled because the very first version 
of tegra-paz00.dts didn't explicitly disable the port, and when 
tegra20.dtsi was updated to disable all USB ports by default, 
tegra-paz00.dtsi was updated to enable the port so that the DT change 
didn't affect behaviour, which then left the port enabled explicitly 
albeit without much reason.


So, once the dr_mode is fixed in this change, it's:
Acked-by: Stephen Warren <swar...@nvidia.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 4/9] ARM: tegra: Enable UDC on TrimSlice

[Stephen Warren]

On 07/05/2017 07:24 PM, Peter Chen wrote:

On Wed, Jul 05, 2017 at 08:19:53PM +0300, Dmitry Osipenko wrote:

From: Thierry Reding 

Override the compatible string of the first USB controller to enable
device mode.

Signed-off-by: Thierry Reding 
---
  arch/arm/boot/dts/tegra20-trimslice.dts | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/tegra20-trimslice.dts 
b/arch/arm/boot/dts/tegra20-trimslice.dts
index b902ab594afa..96b4f3f9827b 100644
--- a/arch/arm/boot/dts/tegra20-trimslice.dts
+++ b/arch/arm/boot/dts/tegra20-trimslice.dts
@@ -336,7 +336,9 @@
};
  
  	usb@c500 {

+   compatible = "nvidia,tegra20-udc";
status = "okay";
+   dr_mode = "otg";


If this board supports peripheral-only, you need to
set dr_mode as "peripheral".


I agree here. Even if the ports can theoretically do otg, and DT is 
supposed to represent HW not SW, given we have no SW support for actual 
OTG, it'd be best not to change the DT to OTG until it's actually useful 
to do so. This comment applies to the patches I ack'd already too.

--
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 v1 7/9] ARM: tegra: Enable UDC on Jetson TK1

[Stephen Warren]

On 07/05/2017 11:19 AM, Dmitry Osipenko wrote:

From: Thierry Reding <tred...@nvidia.com>

Override the compatible string of the first USB controller to enable
device mode.


Conceptually I'm fine with this change since the port isn't enabled at 
all yet and so this doesn't change any existing behaviour.


Acked-by: Stephen Warren <swar...@nvidia.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 5/9] ARM: tegra: Enable UDC on Beaver

[Stephen Warren]

On 07/05/2017 11:19 AM, Dmitry Osipenko wrote:

From: Thierry Reding <tred...@nvidia.com>

Override the compatible string of the first USB controller to enable
device mode.


Conceptually I'm fine with this change since the port isn't enabled at 
all yet and so this doesn't change any existing behaviour. I hope 
someone tested this on real HW?


Acked-by: Stephen Warren <swar...@nvidia.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 6/9] ARM: tegra: Enable UDC on Dalmore

[Stephen Warren]

On 07/05/2017 11:19 AM, Dmitry Osipenko wrote:

From: Thierry Reding 

Override the compatible string of the first USB controller to enable
device mode.


I don't have easy access to this HW any more to test this patch; we do 
have one in our automated test system, but it's rather painful to 
manually test new features on the board, and I think doing so is more 
effort than it's worth investing in this particular board.


Do you have this board (I don't think they're widely available at all), 
and did you test it? If not, I'm a bit surprised it was sent without 
mentioning that it was untested...


As such, I'd suggest dropping this patch.
--
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 v1 8/9] ARM: tegra: Enable UDC on AC100

[Stephen Warren]

On 07/05/2017 11:19 AM, Dmitry Osipenko wrote:

Override the compatible string of the first USB controller to enable
device mode.



diff --git a/arch/arm/boot/dts/tegra20-paz00.dts 
b/arch/arm/boot/dts/tegra20-paz00.dts



usb@c500 {
+   compatible = "nvidia,tegra20-udc";
status = "okay";
+   dr_mode = "otg";
};


This port is the only external USB port on the AC100. I'm not sure it's 
a good idea to reconfigure it so it can only work in device mode, 
although given that display, keyboard, and mouse are built-in to this 
device, perhaps this change is merely unexpected rather than 
catastrophic to a user, since they can recover via the keyboard/display. 
Let's drop this patch until OTG mode is actually implemented.

--
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 v1 0/9] Support UDC on Tegra 20/30/114/124

[Stephen Warren]

On 07/05/2017 04:13 PM, Dmitry Osipenko wrote:

On 05.07.2017 23:31, Stephen Warren wrote:

On 07/05/2017 11:19 AM, Dmitry Osipenko wrote:

Some time ago Thierry Reding sent out patches that enabled UDC on NVIDIA
Tegra, unfortunately they haven't got enough traction to get into the
kernel. I've rebased those patches and added a fix for the Ethernet USB
Gadget on Tegra20, Marc Dietrich tested UDC driver on AC100 and Nicolas
Chauvet on TK1. Like an original patchset, this series adds support for
the peripheral mode only.


Does this mean that the relevant ports no longer support host mode? That's going
to be a user-visible regression, which doesn't sound like a good idea. Isn't OTG
possible instead?


We are going to switch only AC100 and TrimSlice to use the UDC driver.


Really? I saw patches in the series for Beaver, Dalmore, and Jetson TK1 too.


Do you
know whether that port is working in a host mode with the tegra-ehci driver on
these devices?


If any USB port is enabled in the DT, it was certainly validated at some 
point. IIRC, we only have host mode enabled at present. So, yes, I'd 
expect this port to work in host mode currently without any issue.



That should be a micro USB port. Since that port is working in a
device mode by default, I presume that switching it into a host mode would
require some extra 'dance', otherwise I may drop the AC100 / TrimSlice patches.

The full-featured OTG should be possible, it could be done later if desired. I'm
leaving it to somebody else to implement.

--
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 v1 9/9] ARM: defconfig: tegra: Enable ChipIdea UDC driver

[Stephen Warren]

On 07/05/2017 11:19 AM, Dmitry Osipenko wrote:

Since NVIDIA Tegra 20/30/40/TK1 are supported now by the ChipIdea driver,
let's enable this driver in tegra_defconfig.


Shouldn't this patch be earlier in the series so that we don't 
completely break USB support between the patch which switches a port to 
the UDC driver and the UDC driver actually being built?

--
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 v1 0/9] Support UDC on Tegra 20/30/114/124

[Stephen Warren]

On 07/05/2017 11:19 AM, Dmitry Osipenko wrote:

Some time ago Thierry Reding sent out patches that enabled UDC on NVIDIA
Tegra, unfortunately they haven't got enough traction to get into the
kernel. I've rebased those patches and added a fix for the Ethernet USB
Gadget on Tegra20, Marc Dietrich tested UDC driver on AC100 and Nicolas
Chauvet on TK1. Like an original patchset, this series adds support for
the peripheral mode only.


Does this mean that the relevant ports no longer support host mode? 
That's going to be a user-visible regression, which doesn't sound like a 
good idea. Isn't OTG possible instead?

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

Crash in gs_write_room() in kernel 4.4, and your gs_start_tx() patch

[Stephen Warren]

Baolin,

I have a question about the following commit:

511a36d2f357 "usb: gadget: Add the gserial port checking in gs_start_tx()"


When usb gadget is set gadget serial function, it will be crash in below
situation.

It will clean the 'port->port_usb' pointer in gserial_disconnect() function
when usb link is inactive, but it will release lock for disabling the endpoints
in this function. Druing the lock release period, it maybe complete one request
to issue gs_write_complete()--->gs_start_tx() function, but the 'port->port_usb'
pointer had been set NULL, thus it will be crash in gs_start_tx() function.

This patch adds the 'port->port_usb' pointer checking in gs_start_tx() function
to avoid this situation.


(also archived at)
https://www.spinics.net/lists/linux-usb/msg143313.html
https://patchwork.kernel.org/patch/9207037/

In kernel 4.4, I'm seeing a rare NULL pointer dereference in 
gs_write_room(). I believe this happens pretty early during system boot, 
likely soon after the gadget configuration has been created, or soon 
after the host PC enumerates the device running the gadget serial 
driver. Full stack trace below, or soon after resume from system 
suspend. I wonder if you think this might be the same issue you fixed? 
Also, your fix was only applied to function gs_start_tx(). Do you think 
the same issue might also apply to other functions in the same driver, 
and hence we might need to add the same check to other functions like 
gs_start_tx() or similar?


I will try applying your patch to see if it fixes my issue. However, the 
issue is very hard to reproduce, so it will be difficult to tell if the 
solution works:-(



[  132.757406] Unable to handle kernel NULL pointer dereference at virtual 
address 0170

...

[  132.757428] CPU: 5 PID: 53 Comm: kworker/u12:1 Not tainted 4.4.38-tegra #1

...

[  132.757439] Workqueue: events_unbound flush_to_ldisc

...

[  132.757447] PC is at _raw_spin_lock_irqsave+0x20/0x54
[  132.757452] LR is at gs_write_room+0x1c/0x7c

...

[  132.757481] Process kworker/u12:1 (pid: 53, stack limit = 0xffc1eb6a8020)
[  132.757482] Call trace:
[  132.757484] [] _raw_spin_lock_irqsave+0x20/0x54
[  132.757487] [] tty_write_room+0x1c/0x2c
[  132.757489] [] __process_echoes+0x24/0x25c
[  132.757491] [] n_tty_receive_buf_common+0x1ec/0xa24
[  132.757493] [] n_tty_receive_buf2+0x10/0x18
[  132.757495] [] flush_to_ldisc+0xe4/0x154
[  132.757499] [] process_one_work+0x154/0x434
[  132.757501] [] worker_thread+0x134/0x40c
[  132.757503] [] kthread+0xe0/0xf4
[  132.757506] [] ret_from_fork+0x10/0x40


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

Re: [PATCH V2 RFC 6/6] ARM: dts: bcm2835: Add Raspberry Pi Zero

[Stephen Warren]

On 08/02/2016 01:29 PM, Stefan Wahren wrote:

Hi Stephen,


Stephen Warren <swar...@wwwdotorg.org> hat am 2. August 2016 um 19:19
geschrieben:


On 07/26/2016 12:53 PM, Stefan Wahren wrote:

The Raspberry Pi Zero is a minified version of model A+. It's
notable there is no PWR LED and the ACT LED is inverted.


Patches 3-6,
Acked-by: Stephen Warren <swar...@nvidia.com>


diff --git a/arch/arm/boot/dts/bcm2835-rpi-zero.dts
b/arch/arm/boot/dts/bcm2835-rpi-zero.dts


The following comment format is a bit strange, but I'm not sure there's
anything objectively better...


the idea behind that was to provide valid values for every dr_mode with minimum
effort. I see 2 alternative solutions:
  a) add different dr_mode examples to the dwc2 => doesn't work for all
platforms
  b) add comments to drivers/usb/dwc2/platform.c => harder to find

At the end the solution here is already obsolete. This patch series doesn't
contain an update for the bcm2835_defconfig which should set the following:

CONFIG_NOP_USB_XCEIV=y
CONFIG_USB_GADGET=y

After enabling these options another issue is revealed. Currently the dr_mode
for all the other bcm283x boards isn't defined, which means "otg" instead of
intended "host". But the dwc2 driver ignores this as long as CONFIG_USB_GADGET
is not defined. So we need to define the dr_mode for all bcm283x boards.


Do note that the existing DTs must work with any new kernel update; 
that's part of DT being an ABI. So, there should be no need to update 
any of the existing DTs. Rather, the driver must cope with missing 
properties and operate as best it can. Perhaps that means that even if 
dr_mode is unspecified and hence defaults to otg, then if properties 
that are mandatory for OTG to operate are missing, the driver falls back 
to host mode. I think that'd be completely backwards-compatible?


> In

order to avoid such massive copy & paste, we better define 3 dtsi files for each
dr_mode.


3 DT files, with an appropriate one of those included in each RPi DT, 
sounds reasonable.



The only catch about this solution is the modes "otg" and "peripheral"
wouldn't be referenced.


That's probably OK; in this patch the properties for those modes are in 
a comment too, so essentially unreferenced. Perhaps on RPi platforms 
where a choice is possible (Zero, A), there could be a comment next to 
the include of the "host" or "otg" version that states the user might 
want to edit the DT and include the other version instead. Then, those 
files would be somewhat referenced.

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

Re: [PATCH V2 RFC 6/6] ARM: dts: bcm2835: Add Raspberry Pi Zero

[Stephen Warren]

On 07/26/2016 12:53 PM, Stefan Wahren wrote:

The Raspberry Pi Zero is a minified version of model A+. It's
notable there is no PWR LED and the ACT LED is inverted.


Patches 3-6,
Acked-by: Stephen Warren <swar...@nvidia.com>


diff --git a/arch/arm/boot/dts/bcm2835-rpi-zero.dts 
b/arch/arm/boot/dts/bcm2835-rpi-zero.dts


The following comment format is a bit strange, but I'm not sure there's 
anything objectively better...



+ {
+   dr_mode = "host";
+   h-rx-fifo-size = <774>;
+   h-np-tx-fifo-size = <256>;
+   h-tx-fifo-size = <512>;
+/*
+ * Settings for otg
+ *
+   dr_mode = "otg";
+   h-rx-fifo-size = <774>;
+   h-np-tx-fifo-size = <32>;
+   h-tx-fifo-size = <0>;
+   g-np-tx-fifo-size = <16>;
+   g-rx-fifo-size = <256>;
+   g-tx-fifo-size = <256 128 128 64 64 64 32>;
+ *
+ * Settings for peripheral
+ *
+   dr_mode = "peripheral";
+   h-rx-fifo-size = <774>;
+   h-np-tx-fifo-size = <0>;
+   h-tx-fifo-size = <0>;
+   g-np-tx-fifo-size = <16>;
+   g-rx-fifo-size = <256>;
+   g-tx-fifo-size = <256 128 128 64 64 64 32>;
+ */
+};


--
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: [RFC 1/5] usb: chipidea: Add support for Tegra20/30/114/124

[Stephen Warren]

On 05/26/2016 03:17 PM, Stephen Warren wrote:

On 05/26/2016 09:40 AM, Thierry Reding wrote:

From: Thierry Reding <tred...@nvidia.com>

All of these Tegra SoC generations have a ChipIdea UDC IP block that can
be used for device mode communication with a host. Implement rudimentary
support that doesn't allow switching between host and device modes.


Are you sure this is correct for Tegra20? I ask because for the /host/
mode driver, there's a "has_hostpc" flag which is set to false for
Tegra20 and true for all other SoCs. In the U-Boot device mode driver
(if not in the kernel driver; I didn't check), there's a concept of "has
hostpc" too. I might expect that flag to be set the same way for both
drivers. That said, I /think/ the host and device HW are unrelated, so
it's possible has_hostpc might be set differently for them.
Unfortunately, we haven't enabled the device mode driver for any Tegra20
system in U-Boot so I can't tell whether we should enable has_hostpc for
Tegra20's device mode driver.

Still, if this code works then I guess it's likely correct...


On the other hand, it looks like the kernel device mode driver might 
auto-detect this; in core.c:hw_device_init(), I see:


reg = hw_read(ci, CAP_HCCPARAMS, HCCPARAMS_LEN) >>
__ffs(HCCPARAMS_LEN);
ci->hw_bank.lpm  = reg;

... and in host.c:host_start(), I see:

ehci->has_hostpc = ci->hw_bank.lpm;

--
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: [RFC 1/5] usb: chipidea: Add support for Tegra20/30/114/124

[Stephen Warren]

On 05/26/2016 09:40 AM, Thierry Reding wrote:

From: Thierry Reding 

All of these Tegra SoC generations have a ChipIdea UDC IP block that can
be used for device mode communication with a host. Implement rudimentary
support that doesn't allow switching between host and device modes.


Are you sure this is correct for Tegra20? I ask because for the /host/ 
mode driver, there's a "has_hostpc" flag which is set to false for 
Tegra20 and true for all other SoCs. In the U-Boot device mode driver 
(if not in the kernel driver; I didn't check), there's a concept of "has 
hostpc" too. I might expect that flag to be set the same way for both 
drivers. That said, I /think/ the host and device HW are unrelated, so 
it's possible has_hostpc might be set differently for them. 
Unfortunately, we haven't enabled the device mode driver for any Tegra20 
system in U-Boot so I can't tell whether we should enable has_hostpc for 
Tegra20's device mode driver.


Still, if this code works then I guess it's likely correct...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Stephen Warren]

On 05/05/2016 02:05 AM, Hans de Goede wrote:

Hi,

On 04-05-16 22:25, Thierry Reding wrote:

On Wed, May 04, 2016 at 11:23:20AM -0600, Stephen Warren wrote:

On 05/04/2016 08:40 AM, Thierry Reding wrote:

From: Thierry Reding <tred...@nvidia.com>

Starting with commit 0b52297f2288 ("reset: Add support for shared reset
controls") there is a reference count for reset control assertions. The
goal is to allow resets to be shared by multiple devices and an assert
will take effect only when all instances have asserted the reset.

In order to preserve backwards-compatibility, all reset controls become
exclusive by default. This is to ensure that reset_control_assert() can
immediately assert in hardware.

However, this new behaviour triggers the following warning in the EHCI
driver for Tegra:

...

The reason is that Tegra SoCs have three EHCI controllers, each with a
separate reset line. However the first controller contains UTMI pads
configuration registers that are shared with its siblings and that are
reset as part of the first controller's reset. There is special code in
the driver to assert and deassert this shared reset at probe time, and
it does so irrespective of which controller is probed first to ensure
that these shared registers are reset before any of the controllers are
initialized. Unfortunately this means that if the first controller gets
probed first, it will request its own reset line and will subsequently
request the same reset line again (temporarily) to perform the reset.
This used to work fine before the above-mentioned commit, but now
triggers the new WARN.

Work around this by making sure we reuse the controller's reset if the
controller happens to be the first controller.



diff --git a/drivers/usb/host/ehci-tegra.c
b/drivers/usb/host/ehci-tegra.c



@@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct
platform_device *pdev)



+bool has_utmi_pad_registers = false;

  phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
  if (!phy_np)
  return -ENOENT;

+if (of_property_read_bool(phy_np,
"nvidia,has-utmi-pad-registers"))
+has_utmi_pad_registers = true;


Isn't that just:

has_utmi_pad_registers = of_property_read_bool(phy_np,
"nvidia,has-utmi-pad-registers");

... and then you can remove " = false" from the declaration too?


Yes. This is really only for aesthetics. The direct assignment doesn't
fit within 80 columns, and wrapping it looks ugly whichever way you do
it.


  if (!usb1_reset_attempted) {
  struct reset_control *usb1_reset;

-usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
+if (!has_utmi_pad_registers)
+usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
+else
+usb1_reset = tegra->rst;

...

  usb1_reset_attempted = true;
  }


This is a pre-existing issue, but what happens if the probes for two USB
controllers run in parallel; there seems to be missing locking
related to
testing/setting usb1_reset_attempted, which could cause multiple
parallel
attempts to get the "utmi-pads" reset object, which would presumably
cause
essentially the same issue this patch is solving in other cases?


Hah! Interestingly my initial attempt at fixing this was to introduce a
lock to serialize these, because I thought that was what was going on. I
don't think this function can ever run concurrently for different
devices because the driver core already serializes probes (unless a
driver specifically requests asynchronous probing, which this one
doesn't).


Why not just use the new shared reset functionality ? It is easy to use,
that way you can drop some of the special handling in the driver and
you're code actually reflects the hardware (which IMHO has a shared reset).


Judging purely by the descriptions of the shared reset functionality in 
this thread, I doubt that will work. A varying number of USB controllers 
will be enabled in DT on a board-by-board basis, so anything that 
attempts to wait for "all devices to assert reset" can't be implemented, 
since it won't be known ahead of time how many reset assertions to wait 
for. Equally, if device probes are serialized, the reset will not happen 
at the right time since it can't happen until the nth probe (when each 
device has asserted reset) but we want it to happen during the 1st probe.


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

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Stephen Warren]

On 05/04/2016 08:40 AM, Thierry Reding wrote:

From: Thierry Reding 

Starting with commit 0b52297f2288 ("reset: Add support for shared reset
controls") there is a reference count for reset control assertions. The
goal is to allow resets to be shared by multiple devices and an assert
will take effect only when all instances have asserted the reset.

In order to preserve backwards-compatibility, all reset controls become
exclusive by default. This is to ensure that reset_control_assert() can
immediately assert in hardware.

However, this new behaviour triggers the following warning in the EHCI
driver for Tegra:

...

The reason is that Tegra SoCs have three EHCI controllers, each with a
separate reset line. However the first controller contains UTMI pads
configuration registers that are shared with its siblings and that are
reset as part of the first controller's reset. There is special code in
the driver to assert and deassert this shared reset at probe time, and
it does so irrespective of which controller is probed first to ensure
that these shared registers are reset before any of the controllers are
initialized. Unfortunately this means that if the first controller gets
probed first, it will request its own reset line and will subsequently
request the same reset line again (temporarily) to perform the reset.
This used to work fine before the above-mentioned commit, but now
triggers the new WARN.

Work around this by making sure we reuse the controller's reset if the
controller happens to be the first controller.



diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c



@@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct 
platform_device *pdev)



+   bool has_utmi_pad_registers = false;

phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
if (!phy_np)
return -ENOENT;

+   if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
+   has_utmi_pad_registers = true;


Isn't that just:

has_utmi_pad_registers = of_property_read_bool(phy_np,
"nvidia,has-utmi-pad-registers");

... and then you can remove " = false" from the declaration too?


if (!usb1_reset_attempted) {
struct reset_control *usb1_reset;

-   usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
+   if (!has_utmi_pad_registers)
+   usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
+   else
+   usb1_reset = tegra->rst;

...

usb1_reset_attempted = true;
}


This is a pre-existing issue, but what happens if the probes for two USB 
controllers run in parallel; there seems to be missing locking related 
to testing/setting usb1_reset_attempted, which could cause multiple 
parallel attempts to get the "utmi-pads" reset object, which would 
presumably cause essentially the same issue this patch is solving in 
other cases?

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

Re: [PATCH v3 1/2] usb: host: ehci-tegra: Grab the correct UTMI pads reset

[Stephen Warren]

On 05/04/2016 08:39 AM, Thierry Reding wrote:

From: Thierry Reding 

There are three EHCI controllers on Tegra SoCs, each with its own reset
line. However, the first controller contains a set of UTMI configuration
registers that are shared with its siblings. These registers will only
be reset as part of the first controller's reset. For proper operation
it must be ensured that the UTMI configuration registers are reset
before any of the EHCI controllers are enabled, irrespective of the
probe order.

Commit a47cc24cd1e5 ("USB: EHCI: tegra: Fix probe order issue leading to
broken USB") introduced code that ensures the first controller is always
reset before setting up any of the controllers, and is never again reset
afterwards.

This code, however, grabs the wrong reset. Each EHCI controller has two
reset controls attached: 1) the USB controller reset and 2) the UTMI
pads reset (really the first controller's reset). In order to reset the
UTMI pads registers the code must grab the second reset, but instead it
grabbing the first.

Signed-off-by: Thierry Reding 
---
Stephen, Alex, Jon, have you ever encountered cases where UTMI might not
have worked correctly? It seems that this code was pulsing the wrong
reset line and therefore the UTMI pads would never be reset unless the
first USB controller was probed before all others. I've never seen any
such problems myself, so I'm unsure about whether it's worth Cc'ing the
patch to sta...@vger.kernel.org.


I don't think I recall seeing USB issues like that, although I don't use 
USB a huge amount. Perhaps the issue just never happens because we 
always have USB1 enabled, and it's physically present in the DTB first, 
so it always happens to get probed first?

--
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 v10 3/9] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support

[Stephen Warren]

On 04/06/2016 11:08 AM, Thierry Reding wrote:

On Tue, Apr 05, 2016 at 03:10:16PM -0600, Stephen Warren wrote:

On 04/05/2016 08:44 AM, Thierry Reding wrote:

On Wed, Mar 16, 2016 at 11:59:44AM -0600, Stephen Warren wrote:

On 03/04/2016 09:19 AM, Thierry Reding wrote:

From: Thierry Reding <tred...@nvidia.com>

Extend the binding to cover the set of feature found in Tegra210.


Acked-by: Stephen Warren <swar...@nvidia.com>


diff --git 
a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt 
b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt



+   padctl@0,7009f000 {

...

+   pads {

...

+   };
+
+   ports {

...

+   };


As a comment not affecting my ack in any way: At the top-level, we place all
the child nodes into "array container" nodes named "pads" and "ports". This
is nice since it separates different types of child nodes and allows easily
adding more types of child nodes in the future without interference, and in
a way that allows us to explicitly know what each node is without having to
interpret its name or compatible value to do so. However, we haven't done
this with the per-lane child nodes inside each pad. If we were to rev the
design, I'd be tempted to suggest:

padctl@0,7009f000 {
pads {
usb2 {
lanes { // This level is new
usb2-0 {


I tried to make this work, but it's unfortunately not possible with the
current code. The reason is that the PHY subsystem assumes that either
the provider DT node corresponds to that of the device (the usb2 pad in
the above example) or one of its children. Hence, putting everything
into one more level further down would require some mechanism to tell
the subsystem about it so that it can be found.


When the padctl driver registers the PHY objects with the PHY subsystem, can
it pass the lanes node as the DT node? That woulud mean each lane /was/ a
child of the node registered with the PHY subsystem.

Perhaps the PHY subsystem requires a struct device rather than a DT node
registered with it?


Yes, that's how the PHY subsystem works. You pass it a struct device *
that it will wrap a struct phy_provider around. It will then use that
struct device's ->of_node as the parent for the child lookup.


If so, does it make sense to create a separate struct device with the
of_node pointing at lanes{}?


I suspect that that would work, but I'm already somewhat uncomfortable
about how many devices the driver creates. Adding more dummy devices
seems like a workaround.


Arguably the current support code isn't a good argument for designing a
binding, so perhaps it'd be useful to add this mechanism in order to get
a better binding. On the other hand, I'm not sure it's really worth it,
since we already have generic bindings that specify the layout of child
devices, and those have been agreed upon broadly (presumably).

In light of the recent discussion on DPAUX vs. I2C, I see how having the
extra level would be useful to provide additional context. If you think
it's worth it I can spend the extra time to get this implemented in the
core.


Naively, it sounds like it'd be a good idea to fix the PHY core. It really
shouldn't care about the parent of any object registered with it; it should
only interact with the specific object it was given, and any other data such
as "ops" callbacks. Do you have any inkling how much work that would be?


I attached what I came up with. It extends the OF PHY provider registry
by allowing an additional node to be specified that if specified will
serve as the parent for the child lookup (and hence overrides the
default node that's taken from the struct device).

It is a fairly trivial patch, and you'll notice the bulk of the changes
is adding the additional parameter in a number of different places. The
only thing I'm not quite happy about is that we start needing to pass a
fairly large number of arguments. But perhaps it's still okay.

An alternative would be to make struct phy_provider embeddable into
driver private structures. That way they could be completely initialized
by a driver before being passed to the __of_phy_provider_register()
function (much like struct gpio_chip and others). That would be a fairly
intrusive change, one that I'd be willing to take on, though I'd like to
have Kishon's opinion on this before going ahead.


Either of those options look OK to me, and don't seem too invasive. I 
imagine the amount of work is not too great given you've already 
implemented the first option:-)



For reference, here's what I'm imagining:

struct foo_phy_provider {
struct phy_provider base;
...
};

int foo_probe(struct platform_device *pdev)
{
struct foo_phy_provider *priv;
...

Re: [PATCH v10 3/9] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support

[Stephen Warren]

On 04/05/2016 08:44 AM, Thierry Reding wrote:

On Wed, Mar 16, 2016 at 11:59:44AM -0600, Stephen Warren wrote:

On 03/04/2016 09:19 AM, Thierry Reding wrote:

From: Thierry Reding <tred...@nvidia.com>

Extend the binding to cover the set of feature found in Tegra210.


Acked-by: Stephen Warren <swar...@nvidia.com>


diff --git 
a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt 
b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt



+   padctl@0,7009f000 {

...

+   pads {

...

+   };
+
+   ports {

...

+   };


As a comment not affecting my ack in any way: At the top-level, we place all
the child nodes into "array container" nodes named "pads" and "ports". This
is nice since it separates different types of child nodes and allows easily
adding more types of child nodes in the future without interference, and in
a way that allows us to explicitly know what each node is without having to
interpret its name or compatible value to do so. However, we haven't done
this with the per-lane child nodes inside each pad. If we were to rev the
design, I'd be tempted to suggest:

padctl@0,7009f000 {
pads {
usb2 {
lanes { // This level is new
usb2-0 {


I tried to make this work, but it's unfortunately not possible with the
current code. The reason is that the PHY subsystem assumes that either
the provider DT node corresponds to that of the device (the usb2 pad in
the above example) or one of its children. Hence, putting everything
into one more level further down would require some mechanism to tell
the subsystem about it so that it can be found.


When the padctl driver registers the PHY objects with the PHY subsystem, 
can it pass the lanes node as the DT node? That woulud mean each lane 
/was/ a child of the node registered with the PHY subsystem.


Perhaps the PHY subsystem requires a struct device rather than a DT node 
registered with it? If so, does it make sense to create a separate 
struct device with the of_node pointing at lanes{}?



Arguably the current support code isn't a good argument for designing a
binding, so perhaps it'd be useful to add this mechanism in order to get
a better binding. On the other hand, I'm not sure it's really worth it,
since we already have generic bindings that specify the layout of child
devices, and those have been agreed upon broadly (presumably).

In light of the recent discussion on DPAUX vs. I2C, I see how having the
extra level would be useful to provide additional context. If you think
it's worth it I can spend the extra time to get this implemented in the
core.


Naively, it sounds like it'd be a good idea to fix the PHY core. It 
really shouldn't care about the parent of any object registered with it; 
it should only interact with the specific object it was given, and any 
other data such as "ops" callbacks. Do you have any inkling how much 
work that would be?


--
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 v10 3/9] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support

[Stephen Warren]

On 03/04/2016 09:19 AM, Thierry Reding wrote:

From: Thierry Reding <tred...@nvidia.com>

Extend the binding to cover the set of feature found in Tegra210.


Acked-by: Stephen Warren <swar...@nvidia.com>


diff --git 
a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt 
b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt



+   padctl@0,7009f000 {

...

+   pads {

...

+   };
+
+   ports {

...

+   };


As a comment not affecting my ack in any way: At the top-level, we place 
all the child nodes into "array container" nodes named "pads" and 
"ports". This is nice since it separates different types of child nodes 
and allows easily adding more types of child nodes in the future without 
interference, and in a way that allows us to explicitly know what each 
node is without having to interpret its name or compatible value to do 
so. However, we haven't done this with the per-lane child nodes inside 
each pad. If we were to rev the design, I'd be tempted to suggest:


padctl@0,7009f000 {
pads {
usb2 {
lanes { // This level is new
usb2-0 {

--
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 v10 1/9] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding

[Stephen Warren]

sed to look up the corresponding USB2 node in DT? I 
would have expected a phandle here, with the physical (HW) port ID being 
represented in the referenced node. Otherwise, I don't see how to tie 
together the USB2 and USB3 DT nodes.



+For Tegra124 and Tegra132, the XUSB pad controller exposes the following
+ports:
+- 3x USB2: usb2-0, usb2-1, usb2-2
+- 1x ULPI: ulpi-0
+- 2x HSIC: hsic-0, hsic-1
+- 2x super-speed USB: usb3-0, usb3-1


Oh, is the physical port ID implicit in the DT node name? It may be 
worth mentioning that when describing the properties for each type of node.


I'll assume that's how the USB2<->USB3 mapping works. All of my comments 
are mainly re: the description/documentation of the binding itself. That 
can all be enhanced later. The underlying binding itself, and the 
example, look reasonable. As such, this patch,


Acked-by: Stephen Warren <swar...@nvidia.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 6/9] dt-bindings: usb: Add NVIDIA Tegra XUSB controller binding

[Stephen Warren]

On 03/04/2016 09:19 AM, Thierry Reding wrote:

From: Thierry Reding <tred...@nvidia.com>

Add device-tree binding documentation for the XUSB controller present
on Tegra124 and later SoCs. This controller supports USB 3.0 via an xHCI
compliant interface.

Based on work by Andrew Bresticker <abres...@chromium.org>.



diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt 
b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt



+Required properties:



+- nvidia,xusb-padctl: phandle to the XUSB pad controller that is used to
+  configure the USB pads used by the XHCI controller


Is "global" access to the PADCTL DT node/driver required? I rather would 
have expected this binding to reference the port objects in the PADCTL 
node. Perhaps the intent is that drivers can use this property to go and 
read the port information directly from the PADCTL node and interpret it 
themselves without requiring the PADCTL driver to provide an interface 
for ports? I guess that would also explain why this binding references 
only PHYs and not ports:



+Optional properties:
+
+- phys: Must contain an entry for each entry in phy-names.
+  See ../phy/phy-bindings.txt for details.
+- phy-names: Should include an entry for each PHY used by the controller. The
+  following PHYs are available:
+  - Tegra124: usb2-0, usb2-1, usb2-2, hsic-0, hsic-1, usb3-0, usb3-1
+  - Tegra132: usb2-0, usb2-1, usb2-2, hsic-0, hsic-1, usb3-0, usb3-1


If that's how this works (which might be worth mentioning in the binding 
doc), then:

Acked-by: Stephen Warren <swar...@nvidia.com>

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

Re: [PATCH v10 2/9] dt-bindings: pinctrl: Deprecate Tegra XUSB pad controller binding

[Stephen Warren]

On 03/04/2016 09:19 AM, Thierry Reding wrote:

From: Thierry Reding <tred...@nvidia.com>

This is an old version of the binding that isn't flexible enough to
describe all aspects of the XUSB pad controller. Specifically with the
addition of XUSB support (for SuperSpeed USB) the existing binding is
no longer suitable.


Assuming the rest of the edits to this file in patch 1/9 get pulled into 
this patch, or perhaps this patch simply gets squashed into path 1/9, 
then this patch,


Acked-by: Stephen Warren <swar...@nvidia.com>

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

Re: [PATCH v10 7/9] dt-bindings: usb: xhci-tegra: Add Tegra210 XUSB controller support

[Stephen Warren]

On 03/04/2016 09:19 AM, Thierry Reding wrote:

From: Thierry Reding <tred...@nvidia.com>

Extend the Tegra XUSB controller device tree binding with Tegra210
support.


Acked-by: Stephen Warren <swar...@nvidia.com>

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

Re: usb: dwc2: regression during boot on Raspberry Pi

[Stephen Warren]

On 11/07/2015 05:16 PM, Stefan Wahren wrote:
> Hi,
> 
> i try to run linux-next-20151106 (U-Boot + USB patches from Stephen
> Warren) on my Raspberry Pi B rev2 and get a kernel oops (see bootlog at
> the end). I bisect the issue to this commit:
> 
> 09a75e8577901489f77a14a3b305a9a1f67bf25b ("usb: dwc2: refactor common
> low-level hw code to platform.c")
> 
> Looking at the messages before the oops
> 
> [ 1.658393] dwc2 2098.usb: Configuration mismatch. Forcing host mode
> [ 1.665605] dwc2 2098.usb: no platform data or transceiver defined
> 
> seems to come from missing DT properties according to
> Documentation/devicetree/bindings/usb/dwc2.txt:
> 
>   * dr_mode (optional)
>   * clocks (required)
>   * clock-names (required)

When I first added the USB node into bcm2835.dtsi, the binding did not
state that "clocks" and "clock-names" were required. The later addition
of that requirement is a non-backwards-compatible change, and should not
have been made. The new properties should have been optional.

The following commit added the clock-related properties as required:

65b4eb9e3513 staging: dwc2: update DT binding to add generic clock/phy
properties

The following commit, at almost exactly the same time, added DWC2 into
bcm2835.dtsi:

5631e7f4e29b ARM: bcm2835: add USB controller to device tree

>   * phys (optional)
>   * phy-names (optional)
> 
> So here are my questions:
> 
> How to fix the kernel oops in dwc2 driver?

Do you know exactly what causes the crash; you've bisected to the
specific commit, but I don't think mentioned why that commit causes an
issue.

> Should we specify dr_mode = "host" for all Raspberry Pi variants in DT?

It's optional so the driver must work without it. However, that value
seems appropriate, so you could certainly add it.

> Which clock should be referenced by USB host in DT?
> 
> Do we need a USB PHY DT node and driver for bcm2835?

I don't think there's a separate PHY module in bcm2835 is there?
Certainly there is no documentation, binding, or driver for it that I'm
aware of. I haven't checked the RPi Foundation downstream kernel though.
--
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: [PATCHv2] usb: dwc2: use devm_phy_optional_get

[Stephen Warren]

On 10/29/2015 01:41 AM, Uwe Kleine-König wrote:
> Hello Stephen,
> 
> On Wed, Oct 28, 2015 at 09:55:05PM -0600, Stephen Warren wrote:
>> On 10/27/2015 12:18 PM, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Tue, Oct 27, 2015 at 09:08:27AM -0600, Stephen Warren wrote:
>>>> On 10/27/2015 01:41 AM, Uwe Kleine-König wrote:
>>>>> Hello,
>>>>>
>>>>> On Mon, Oct 26, 2015 at 08:08:58PM -0600, Stephen Warren wrote:
>>>>>> On 10/26/2015 03:28 AM, Alexander Aring wrote:
>>>>>>> This patch changes devm_phy_get to devm_phy_optional_get. Which fixes 
>>>>>>> the
>>>>>>> error handling when trying to get the phy. Currently on all errors we
>>>>>>> try to look for an old style USB PHY. With this patch we try to look for
>>>>>>> an old USB PHY when devm_phy_get returns -ENOENT only. Other values like
>>>>>>> -EPROBE_DEFER need to be returned from the probe function, because it
>>>>>>> could be that a phy is specified but not probed before.
>>>>>>
>>>>>>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>>>>>>
>>>>>>> @@ -222,12 +222,15 @@ static int dwc2_driver_probe(struct 
>>>>>>> platform_device *dev)
>>>>>>>
>>>>>>> hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node);
>>>>>>>
>>>>>>> +   phy = devm_phy_optional_get(>dev, "usb2-phy");
>>>>>>> +   if (IS_ERR(phy))
>>>>>>> +   return PTR_ERR(phy);
>>>>>>
>>>>>> Here, phy is checked using IS_ERR(), ...
>>>>>
>>>>> IS_ERR(phy) is true, if something went wrong getting a phy (probably
>>>> >from dt).
>>>>>
>>>>>>> /*
>>>>>>>  * Attempt to find a generic PHY, then look for an old style
>>>>>>>  * USB PHY
>>>>>>>  */
>>>>>>> -   phy = devm_phy_get(>dev, "usb2-phy");
>>>>>>> -   if (IS_ERR(phy)) {
>>>>>>> +   if (!phy) {
>>>>>>
>>>>>> ... and here the same phy value is checked against NULL. Hopefully,
>>>>>
>>>>> and phy is NULL if dt didn't specify a phy. That's the semantic of
>>>>> devm_phy_optional_get (which is identically to what
>>>>> devm_gpiod_get_optional) does.
>>>>>
>>>>>> devm_phy_get() doesn't actually return either an ERR value or a NULL
>>>>>> pointer as errors, but restricts itself to either an ERR value or
>>>>>
>>>>> The relevant part is that for a function with "optinal" in it's name not
>>>>> finding the respective resource isn't an error. So checking for NULL
>>>>> isn't error handling but handling the 2nd valid case.
>>>>
>>>> If devm_phy_optional_get() uses IS_ERR to check for errors, then ANY other
>>>> (non-IS_ERR) value is an opaque handle to a PHY. The values should be
>>>> meaningless outside of the PHY subsystem. Any other set of return values is
>>>> broken.
>>>>
>>>> The whole point about optional APIs is that they return one of:
>>>>
>>>> a) An error value that can be checked for. Any error is a hard error.
>>>>
>>>> b) A valid PHY value that can be passed to all other PHY-related API.
>>>>
>>>> (b) can be further divided into two cases:
>>>>
>>>> b1) An actual PHY handle if the PHY exists
>>>>
>>>> b2) A dummy PHY value that the PHY APIs internally map to no-ops.
>>>>
>>>> However, the distinction between (b1) and (b2) is something that clients
>>>> should not know about, and hence assuming "NULL" is that dummy value is
>>>> wrong.
>>>
>>> So for the case where there is an "optional" phy handle in the dt (in
>>> the sense that the driver can handle both the presence and the absence
>>> of such a handle) what do you suggest? If not using *_optional the only
>>> alternative is to use plain devm_phy_get and ignore -ESOMETHING
>>> (whatever signals the absence of the handle). Not pretty. I like using
>>> devm_phy_optional_get better here.
>>
>> The

Re: [PATCHv2] usb: dwc2: use devm_phy_optional_get

[Stephen Warren]

On 10/27/2015 12:18 PM, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Oct 27, 2015 at 09:08:27AM -0600, Stephen Warren wrote:
>> On 10/27/2015 01:41 AM, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Mon, Oct 26, 2015 at 08:08:58PM -0600, Stephen Warren wrote:
>>>> On 10/26/2015 03:28 AM, Alexander Aring wrote:
>>>>> This patch changes devm_phy_get to devm_phy_optional_get. Which fixes the
>>>>> error handling when trying to get the phy. Currently on all errors we
>>>>> try to look for an old style USB PHY. With this patch we try to look for
>>>>> an old USB PHY when devm_phy_get returns -ENOENT only. Other values like
>>>>> -EPROBE_DEFER need to be returned from the probe function, because it
>>>>> could be that a phy is specified but not probed before.
>>>>
>>>>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>>>>
>>>>> @@ -222,12 +222,15 @@ static int dwc2_driver_probe(struct platform_device 
>>>>> *dev)
>>>>>
>>>>>   hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node);
>>>>>
>>>>> + phy = devm_phy_optional_get(>dev, "usb2-phy");
>>>>> + if (IS_ERR(phy))
>>>>> + return PTR_ERR(phy);
>>>>
>>>> Here, phy is checked using IS_ERR(), ...
>>>
>>> IS_ERR(phy) is true, if something went wrong getting a phy (probably
>> >from dt).
>>>
>>>>>   /*
>>>>>* Attempt to find a generic PHY, then look for an old style
>>>>>* USB PHY
>>>>>*/
>>>>> - phy = devm_phy_get(>dev, "usb2-phy");
>>>>> - if (IS_ERR(phy)) {
>>>>> + if (!phy) {
>>>>
>>>> ... and here the same phy value is checked against NULL. Hopefully,
>>>
>>> and phy is NULL if dt didn't specify a phy. That's the semantic of
>>> devm_phy_optional_get (which is identically to what
>>> devm_gpiod_get_optional) does.
>>>
>>>> devm_phy_get() doesn't actually return either an ERR value or a NULL
>>>> pointer as errors, but restricts itself to either an ERR value or
>>>
>>> The relevant part is that for a function with "optinal" in it's name not
>>> finding the respective resource isn't an error. So checking for NULL
>>> isn't error handling but handling the 2nd valid case.
>>
>> If devm_phy_optional_get() uses IS_ERR to check for errors, then ANY other
>> (non-IS_ERR) value is an opaque handle to a PHY. The values should be
>> meaningless outside of the PHY subsystem. Any other set of return values is
>> broken.
>>
>> The whole point about optional APIs is that they return one of:
>>
>> a) An error value that can be checked for. Any error is a hard error.
>>
>> b) A valid PHY value that can be passed to all other PHY-related API.
>>
>> (b) can be further divided into two cases:
>>
>> b1) An actual PHY handle if the PHY exists
>>
>> b2) A dummy PHY value that the PHY APIs internally map to no-ops.
>>
>> However, the distinction between (b1) and (b2) is something that clients
>> should not know about, and hence assuming "NULL" is that dummy value is
>> wrong.
> 
> So for the case where there is an "optional" phy handle in the dt (in
> the sense that the driver can handle both the presence and the absence
> of such a handle) what do you suggest? If not using *_optional the only
> alternative is to use plain devm_phy_get and ignore -ESOMETHING
> (whatever signals the absence of the handle). Not pretty. I like using
> devm_phy_optional_get better here.

Then it's not actually optional. For the optional case, the driver
doesn't care whether it's there, but rather simply wants to blindly call
the phy APIs irrespective. The only error condition is if some problem
occurs actually looking up the PHY.

In this case, the driver cares whether the PHY exists, so it can take
alternate action if it does not. The code should do something like:

phy = devm_phy_get(); // not optional
if (error) {
phy = some_compatibility_api();
if (error)
return error;
}

(ignoring things like deferred probe, and real function names)
--
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: [PATCHv2] usb: dwc2: use devm_phy_optional_get

[Stephen Warren]

On 10/27/2015 01:41 AM, Uwe Kleine-König wrote:

Hello,

On Mon, Oct 26, 2015 at 08:08:58PM -0600, Stephen Warren wrote:

On 10/26/2015 03:28 AM, Alexander Aring wrote:

This patch changes devm_phy_get to devm_phy_optional_get. Which fixes the
error handling when trying to get the phy. Currently on all errors we
try to look for an old style USB PHY. With this patch we try to look for
an old USB PHY when devm_phy_get returns -ENOENT only. Other values like
-EPROBE_DEFER need to be returned from the probe function, because it
could be that a phy is specified but not probed before.



diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c



@@ -222,12 +222,15 @@ static int dwc2_driver_probe(struct platform_device *dev)

hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node);

+   phy = devm_phy_optional_get(>dev, "usb2-phy");
+   if (IS_ERR(phy))
+   return PTR_ERR(phy);


Here, phy is checked using IS_ERR(), ...


IS_ERR(phy) is true, if something went wrong getting a phy (probably
from dt).


/*
 * Attempt to find a generic PHY, then look for an old style
 * USB PHY
 */
-   phy = devm_phy_get(>dev, "usb2-phy");
-   if (IS_ERR(phy)) {
+   if (!phy) {


... and here the same phy value is checked against NULL. Hopefully,


and phy is NULL if dt didn't specify a phy. That's the semantic of
devm_phy_optional_get (which is identically to what
devm_gpiod_get_optional) does.


devm_phy_get() doesn't actually return either an ERR value or a NULL
pointer as errors, but restricts itself to either an ERR value or


The relevant part is that for a function with "optinal" in it's name not
finding the respective resource isn't an error. So checking for NULL
isn't error handling but handling the 2nd valid case.


If devm_phy_optional_get() uses IS_ERR to check for errors, then ANY 
other (non-IS_ERR) value is an opaque handle to a PHY. The values should 
be meaningless outside of the PHY subsystem. Any other set of return 
values is broken.


The whole point about optional APIs is that they return one of:

a) An error value that can be checked for. Any error is a hard error.

b) A valid PHY value that can be passed to all other PHY-related API.

(b) can be further divided into two cases:

b1) An actual PHY handle if the PHY exists

b2) A dummy PHY value that the PHY APIs internally map to no-ops.

However, the distinction between (b1) and (b2) is something that clients 
should not know about, and hence assuming "NULL" is that dummy value is 
wrong.

--
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: [PATCHv2] usb: dwc2: use devm_phy_optional_get

[Stephen Warren]

On 10/26/2015 03:28 AM, Alexander Aring wrote:
> This patch changes devm_phy_get to devm_phy_optional_get. Which fixes the
> error handling when trying to get the phy. Currently on all errors we
> try to look for an old style USB PHY. With this patch we try to look for
> an old USB PHY when devm_phy_get returns -ENOENT only. Other values like
> -EPROBE_DEFER need to be returned from the probe function, because it
> could be that a phy is specified but not probed before.

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c

> @@ -222,12 +222,15 @@ static int dwc2_driver_probe(struct platform_device 
> *dev)
>  
>   hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node);
>  
> + phy = devm_phy_optional_get(>dev, "usb2-phy");
> + if (IS_ERR(phy))
> + return PTR_ERR(phy);

Here, phy is checked using IS_ERR(), ...

>   /*
>* Attempt to find a generic PHY, then look for an old style
>* USB PHY
>*/
> - phy = devm_phy_get(>dev, "usb2-phy");
> - if (IS_ERR(phy)) {
> + if (!phy) {

... and here the same phy value is checked against NULL. Hopefully,
devm_phy_get() doesn't actually return either an ERR value or a NULL
pointer as errors, but restricts itself to either an ERR value or
returning NULL. In other words, it's bad form to check a value using
both IS_ERR and comparison against NULL.
--
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 V9] dt: add NVIDIA Tegra XUSB controller binding

[Stephen Warren]

From: Stephen Warren <swar...@nvidia.com>

Add device-tree binding documentation for the XUSB (xHCI) controller
present on Tegra124 and later SoCs.

Signed-off-by: Andrew Bresticker <abres...@chromium.org>
[swarren, combined separate MFD, mailbox, XHCI bindings into one node]
Signed-off-by: Stephen Warren <swar...@nvidia.com>
Cc: Rob Herring <robh...@kernel.org>
Cc: Pawel Moll <pawel.m...@arm.com>
Cc: Mark Rutland <mark.rutl...@arm.com>
Cc: Ian Campbell <ijc+devicet...@hellion.org.uk>
Cc: Kumar Gala <ga...@codeaurora.org>
Cc: Mathias Nyman <mathias.ny...@intel.com>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---
Changes from v8:
 - Combined the separate MFD, mailbox, and XHCI bindings into one; there
   is a single HW module, and so there should be a single DT node to
   represent it. Any Linux-internal driver structure should not influence
   the binding structure. This included squashing the various reg and
   interrupt resources that were previously in the separate modules into
   one list.
 - Used lists to document the compatible, reg, and interrupts properties.
 - Renamed the primary binding from xhci to xusb to reflect the name of
   the overall module.
Changes from v7:
 - Added back non-shared reg/interrupts properties.
Changes from v6:
 - Removed XUSB_DEV related clocks/resets.  They will be consumed by
   a separate driver and binding.
 - Removed reg/interrupts properties.
No changes from v5.
Changes from v4:
 - Updated regulator names, as suggested by Thierry.
No changes from v3.
Changes from v2:
 - Added mbox-names property.
Changes from v1:
 - Updated to use common mailbox bindings.
 - Added remaining XUSB-related clocks and resets.
 - Updated list of power supplies to be more accurate wrt to the hardware.
---
 .../bindings/usb/nvidia,tegra124-xusb.txt  | 96 ++
 1 file changed, 96 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt

diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt 
b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt
new file mode 100644
index ..f8de8d49602e
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt
@@ -0,0 +1,96 @@
+NVIDIA Tegra XUSB (XHCI) controller
+===
+
+The Tegra XUSB controller supports both USB2 and USB3 interfaces exposed
+by the Tegra XUSB pad controller.
+
+Required properties:
+
+ - compatible: Valid options are:
+- Tegra124: "nvidia,tegra124-xusb".
+- Tegra132: "nvidia,tegra132-xusb", "nvidia,tegra124-xusb".
+ - reg: Must contain the following entries, in the following order:
+- The xHCI host registers.
+- The IPFS registers.
+- The FPCI registers.
+ - interrupts: Must contain the following entries, in the following order:
+- The xHCI host interrupt
+- The XUSB mailbox interrupt.
+ - clocks: Must contain an entry for each entry in clock-names.
+   See ../clock/clock-bindings.txt for details.
+ - clock-names: Must include the following entries:
+- xusb_host
+- xusb_host_src
+- xusb_falcon_src
+- xusb_ss
+- xusb_ss_src
+- xusb_ss_div2
+- xusb_hs_src
+- xusb_fs_src
+- pll_u_480m
+- clk_m
+- pll_e
+ - resets: Must contain an entry for each entry in reset-names.
+   See ../reset/reset.txt for details.
+ - reset-names: Must include the following entries:
+   - xusb_host
+   - xusb_ss
+   - xusb_src
+   Note that xusb_src is the shared reset for xusb_{ss,hs,fs,falcon,host}_src.
+ - avddio-pex-supply: PCIe/USB3 analog logic power supply.  Must supply 1.05V.
+ - dvddio-pex-supply: PCIe/USB3 digital logic power supply.  Must supply 1.05V.
+ - avdd-usb-supply: USB controller power supply.  Must supply 3.3V.
+ - avdd-pll-utmip-supply: UTMI PLL power supply.  Must supply 1.8V.
+ - avdd-pll-erefe-supply: PLLE reference PLL power supply.  Must supply 1.05V.
+ - avdd-usb-ss-pll-supply: PCIe/USB3 PLL power supply.  Must supply 1.05V.
+ - hvdd-usb-ss-supply: High-voltage PCIe/USB3 power supply.  Must supply 3.3V.
+ - hvdd-usb-ss-pll-e-supply: High-voltage PLLE power supply.  Must supply 3.3V.
+
+Optional properties:
+
+ - phys: Must contain an entry for each entry in phy-names.
+   See ../phy/phy-bindings.txt for details.
+ - phy-names: Should include an entry for each PHY used by the controller.
+   Names must be of the form "-" where  is one of "utmi",
+   "hsic", or "usb3" and  is a 0-based index.  On Tegra124, there may
+   be up to 3 UTMI, 2 HSIC, and 2 USB3 PHYs.
+
+Example:
+
+   usb-host@0,7009 {
+   compatible = "nvidia,tegra124-xusb";
+   reg = <0x0 0x7009 0x0 0x8000>,
+ <0x0 0x70099000 0x0 0x1000>,
+ <0x0 0x70098000 0x0 0x

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Stephen Warren]

On 06/23/2015 05:56 AM, Roger Quadros wrote:

+ Kukjin, Stephen,

for board specific USB question.

On Tue, 23 Jun 2015 16:35:49 +0800
Li Jun b47...@freescale.com wrote:


On Tue, Jun 23, 2015 at 10:43:28AM +0300, Roger Quadros wrote:


If the dr_mode was otg for such case and we want OTG disabled then it is 
really the DT fault.


It's ID pin detect for dual role switch as many current OTG controllers have.
not DT fault, its dt only has a dr_mode = otg.


We don't need to tackle this case. Just fix up the DT to dr_mode = peripheral 
if
OTG behaviour is not needed.


OTG behaviour is not needed, so we need disable HNP/SRP/ADP. but dr_mode =
otg as it already works fine with ID pin detect.


Do you know which platform has plain non-otg dual-role working as of today
with dr_mode set to otg?


I don't know.
The dt property dr_mode is already there, and currently there are some platforms
as its user, so I assume those platforms either are non-otg dual-role, or real 
OTG
with HNP supported, I guess most are the former cases; chipidea is the later 
case.


For TI platforms none of them have it working currently.


So for Ti platforms, some enables non-otg dual-role function but do not use
dr_mode = otg?


for TI we have only musb based and dwc3 based platforms. MUSB based are OTG.
dwc3 doesn't support dual-role or OTG yet so we use either peripheral or 
Host



For those only have non-otg dual-role platforms, no matter using dr_mode or not,
we need keep it's real OTG disabled for legacy users after it's controller
driver adds real OTG later.


I wouldn't even bother about platforms expecting dual-role behaviour
with dr_mode not set to otg. That is just plain wrong and can't be supported.

These are the dts files having dr_mode == otg.
I've sorted them as per Soc vendor and USB controller

...

Nvidia
--
arm/boot/dts/tegra20-seaboard.dts:  dr_mode = otg;
compatible = nvidia,tegra20-ehci, usb-ehci;
arm/boot/dts/tegra30-colibri-eval-v3.dts:   dr_mode = otg;
compatible = nvidia,tegra30-ehci, usb-ehci;

I couldn't find gadget side implementation for this.
Stephen, any comments on whether this board supports true OTG operation or just 
dual-role operation?


Sorry for the slow reply; I was on vacation.

I'm not sure what the difference is?

I won't be able to answer for the Colibri board since I'm not familiar 
with the HW. I have the schematics for Seaboard, so I should be able to 
answer once I understand the question:-)

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

Re: [PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers

[Stephen Warren]

On 04/23/2015 08:06 AM, Johan Hovold wrote:

Make sure only to copy any actual data rather than the whole buffer,
when releasing the temporary buffer used for unaligned non-isochronous
transfers.

Compile-tested only.


Tested-by: Stephen Warren swar...@nvidia.com

(Tested a USB network device attached to Jetson TK1, with a large 
download, and ping packets of various sizes from another host)

--
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: phy: Restore deferred probing path

[Stephen Warren]

On 12/23/2014 11:36 AM, Felipe Balbi wrote:

On Thu, Dec 04, 2014 at 01:06:07PM +0100, Thierry Reding wrote:

From: Thierry Reding tred...@nvidia.com

Commit 1290a958d48e (usb: phy: propagate __of_usb_find_phy()'s error on
failure) broke platforms that rely on deferred probing to order probing
of PHY and host controller drivers. The reason is that the commit simply
propagates errors from __of_usb_find_phy(), which returns -ENODEV if no
PHY has been registered yet for a given device tree node. The only case
in which -EPROBE_DEFER would now be returned is if try_module_get() did
fail, which does not make sense.

The correct thing to do is to return -EPROBE_DEFER if a PHY hasn't been
registered yet. The only condition under which it makes sense to return
-ENODEV is if the device tree node representing the PHY has been
disabled (via the status property) because in that case the PHY will
never be registered.

This patch addresses the problem by making __of_usb_find_phy() return an
appropriate error code while keeping in line with the above-mentioned
commit to propagate error codes rather than overwriting them. At the
same time the check for a valid PHY is decoupled from the check for the
try_module_get() call and a separate error code is returned if the
latter fails.

Signed-off-by: Thierry Reding tred...@nvidia.com


you forgot to add the magic Fixes: foo bar here, I'll add it this
time, but I've already sent my -rc2 pull request, so this will only show
up on -rc3.


Presumably also add the following, since the patch fixes a regression in 
a previous kernel release:


Cc: stable # v3.18

?

--
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/9] Tegra xHCI support

[Stephen Warren]

On 09/16/2014 05:51 PM, Andrew Bresticker wrote:

On Tue, Sep 16, 2014 at 4:15 PM, Stephen Warren swar...@wwwdotorg.org wrote:

On 09/16/2014 04:46 PM, Andrew Bresticker wrote:


On Tue, Sep 16, 2014 at 9:57 AM, Andrew Bresticker
abres...@chromium.org wrote:


On Tue, Sep 16, 2014 at 8:26 AM, Stephen Warren swar...@wwwdotorg.org
wrote:


On 09/15/2014 01:30 PM, Andrew Bresticker wrote:



On Mon, Sep 15, 2014 at 11:09 AM, Stephen Warren
swar...@wwwdotorg.org
wrote:



On 09/15/2014 11:06 AM, Andrew Bresticker wrote:



On Mon, Sep 15, 2014 at 12:00 AM, Tomeu Vizoso
to...@tomeuvizoso.net
wrote:



On 12 September 2014 18:37, Andrew Bresticker
abres...@chromium.org
wrote:



On Tue, Sep 9, 2014 at 1:21 AM, Tomeu Vizoso
to...@tomeuvizoso.net
wrote:



On 8 September 2014 18:22, Andrew Bresticker
abres...@chromium.org
wrote:



On Mon, Sep 8, 2014 at 8:34 AM, Tomeu Vizoso
to...@tomeuvizoso.net
wrote:



On 2 September 2014 23:34, Andrew Bresticker
abres...@chromium.org wrote:



Tested on Venice2, Jetson TK1, and Big with a variety of USB2.0
and
USB3.0 memory sticks and ethernet dongles using controller
firmware
recently posted by Andrew Chew [2].



...



Stephen, Thierry, have either of you had a chance to test this
series?




I haven't had a chance to yet. I just went to try it out, and found
that
it
depends on a whole slew of other patches that I don't have. Is there a
git
branch somewhere to save me having to track down all the dependencies?




Yes, Tomeu has the branch he used for testing here:
http://cgit.collabora.com/git/user/tomeu/linux.git/log/?h=3.17rc4-xhci




Hmm. That git server was quite reluctant to cough up its bits, but it
did
eventually.


You'll also need the firmware Andrew Chew posted:
https://patchwork.ozlabs.org/patch/384013/




The XHCI driver can't load its firmware unless it's a module; if I make
it
built-in, it fails immediately with error -2 during Direct firmware
loading. The driver needs to work with either immediate or deferred
firmware loading.



If you want the driver to be built-in, you'll either need to build the
firmware in as well (i.e. EXTRA_FIRMWARE) or enable
FW_LOADER_USER_HELPER_FALLBACK to load with userspace/uevent (though
apparently this is deprecated).


The following USB2 devices had problems:

0b95:7720 ASIX Electronics Corp. AX88772


[  489.140536] usb 1-3: new high-speed USB device number 81 using
xhci-tegra
[  489.260860] usb 1-3: device descriptor read/64, error -71
[  489.370804] xhci-tegra 7009.usb: Setup ERROR: setup context
command
for slot 1.
[  489.378463] usb 1-3: hub failed to enable device, error -22
[  489.500531] usb 1-3: new high-speed USB device number 82 using
xhci-tegra
[  489.655708] usb 1-3: can't set config #1, error -71
[  489.661231] usb 1-3: USB disconnect, device number 82
[  489.940531] usb 1-3: new high-speed USB device number 83 using
xhci-tegra
[  490.060860] usb 1-3: device descriptor read/64, error -71
[  490.170805] xhci-tegra 7009.usb: Setup ERROR: setup context
command
for slot 1.
[  490.178462] usb 1-3: hub failed to enable device, error -22



(repeats over and over)

15a4:1336 Afatech Technologies, Inc. SDHC/MicroSD/MMC/MS/M2/CF/XD Flash
Card
Reader

The power light comes on, and the activity light just keeps flashing
fast.
Usually the activity light flashes a couple times and then turns off.
There
is nothing in dmesg at all for this device.

05e3:0608 Genesys Logic, Inc. USB-2.0 4-Port HUB

Not detected. There is nothing in dmesg at all for this device.

1bcf:0c31 Sunplus Innovation Technology Inc. SPIF30x Serial-ATA bridge

Not detected. There is nothing in dmesg at all for this device.



Thanks, I'll try to figure out what's going on here.



Grr... this is due to the unfortunate UTMI pad controller design on Tegra.

Here's the issue:  When UTMI pad 0 is assigned to the EHCI controller
(as is currently the case on Jetson-TK1), the UTMI parameters from
UTMIP_BIAS_CFG0 in the global UTMI pad register space are used
regardless of the owner of the UTMI pads.  If pad 0 is assigned to the
XUSB controller, then parameters in USB2_BIAS_PAD_CTL0 in the XUSB
register space are used for all UTMI pads (again, regardless of
ownership).

I wasn't able to reproduce before because I always TFTP booted and
U-Boot programs the UTMI parameters correctly when starting the EHCI
controllers.  I suspect you and Tomeu were booting without starting
the EHCI controllers and thus were stuck with the POR values.



I am loading the kernel over the PCIe Ethernet on Jetson TK1. However, the
U-Boot boot scripts do usb start first, so I would assume all the USB set
is already done either way, before U-Boot attempts to probe USB devices.


Interesting - just doing usb start made the difference for me.


The easiest way to fix this is to just assign UTMI port 0 (i.e. the
OTG port) to the XUSB controller.  AFAIK, device mode on Tegra isn't
supported yet in the kernel, so this shouldn't break any existing use
cases.  If we wanted

Re: [PATCHv4 04/12] usb: dwc2: Add the appropriate init calls in platform code

[Stephen Warren]

On 09/17/2014 02:47 PM, Dinh Nguyen wrote:

Hi Robert,

On 9/12/14, 7:13 AM, Robert Baldyga wrote:

Hi Dinh,

On 08/26/2014 06:19 PM, dingu...@opensource.altera.com wrote:

From: Dinh Nguyen dingu...@opensource.altera.com

Add the proper init calls for either host, gadget or both in platform.c

Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com
Acked-by: Paul Zimmerman pa...@synopsys.com
---
  drivers/usb/dwc2/core.h | 13 +
  drivers/usb/dwc2/gadget.c   |  2 +-
  drivers/usb/dwc2/platform.c | 28 
  3 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index f55e62d..3a49a00 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -960,6 +960,19 @@ extern void dwc2_dump_global_registers(struct dwc2_hsotg 
*hsotg);
   */
  extern u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg);

+/* Gadget defines */
+#if defined(CONFIG_USB_DWC2_PERIPHERAL) || defined(CONFIG_USB_DWC2_DUAL_ROLE)
+extern int s3c_hsotg_remove(struct dwc2_hsotg *hsotg);
+extern void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2);


Function s3c_hsotg_core_init() is used only inside file gadget.c so
exporting it makes no sense. By the way it should be static.


Yes, I agree here. Fixed up in v5.




+extern int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq);
+#else
+static inline void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2) {}
+static inline int s3c_hsotg_remove(struct dwc2_hsotg *dwc2)
+{ return 0; }
+static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
+{ return 0; }


It also makes no sense to have this functions declared if you don't have
to use them. They are called in one place in code, inside
dwc2_driver_probe() function, so you can rather use if defined() there.


I'm not sure I agree here. This is necessary for the current runtime
implementation of the role initialization. This is probably relevant
with your next 2 comments.




+#endif
+
  #if defined(CONFIG_USB_DWC2_HOST) || defined(CONFIG_USB_DWC2_DUAL_ROLE)
  /**
   * dwc2_hcd_get_frame_number() - Returns current frame number
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 96f868f..efa68a0 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3572,7 +3572,7 @@ err_clk:
   * s3c_hsotg_remove - remove function for hsotg driver
   * @pdev: The platform information for the driver
   */
-static int s3c_hsotg_remove(struct dwc2_hsotg *hsotg)
+int s3c_hsotg_remove(struct dwc2_hsotg *hsotg)
  {
usb_del_gadget_udc(hsotg-gadget);

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index dd2f8f5..2871f351 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -92,7 +92,14 @@ static int dwc2_driver_remove(struct platform_device *dev)
  {
struct dwc2_hsotg *hsotg = platform_get_drvdata(dev);

-   dwc2_hcd_remove(hsotg);
+   if (IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL))
+   s3c_hsotg_remove(hsotg);
+   else if (IS_ENABLED(CONFIG_USB_DWC2_HOST))
+   dwc2_hcd_remove(hsotg);
+   else { /* dual role */
+   dwc2_hcd_remove(hsotg);
+   s3c_hsotg_remove(hsotg);
+   }


Why don't make this checks compile-time?



Do you have a reason for a compile-time versus runtime here? It just
seems that from a few discussion threads on lkml that there is a general
biased towards using IS_ENABLED() as it looks a bit cleaner than
littering the code with a bunch of #ifdefs.


With typical compiler optimization, if (IS_ENABLED(...)) *is* a 
compile-time check. Yet, it allows the code within the if block body to 
be parsed, so that even if the code doesn't make it into the binary, it 
still gets syntax checking etc.

--
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/9] Tegra xHCI support

[Stephen Warren]

On 09/15/2014 01:30 PM, Andrew Bresticker wrote:

On Mon, Sep 15, 2014 at 11:09 AM, Stephen Warren swar...@wwwdotorg.org wrote:

On 09/15/2014 11:06 AM, Andrew Bresticker wrote:

On Mon, Sep 15, 2014 at 12:00 AM, Tomeu Vizoso to...@tomeuvizoso.net wrote:

On 12 September 2014 18:37, Andrew Bresticker abres...@chromium.org wrote:

On Tue, Sep 9, 2014 at 1:21 AM, Tomeu Vizoso to...@tomeuvizoso.net wrote:

On 8 September 2014 18:22, Andrew Bresticker abres...@chromium.org wrote:

On Mon, Sep 8, 2014 at 8:34 AM, Tomeu Vizoso to...@tomeuvizoso.net wrote:

On 2 September 2014 23:34, Andrew Bresticker abres...@chromium.org wrote:

Tested on Venice2, Jetson TK1, and Big with a variety of USB2.0 and
USB3.0 memory sticks and ethernet dongles using controller firmware
recently posted by Andrew Chew [2].

...

Stephen, Thierry, have either of you had a chance to test this series?


I haven't had a chance to yet. I just went to try it out, and found that it
depends on a whole slew of other patches that I don't have. Is there a git
branch somewhere to save me having to track down all the dependencies?


Yes, Tomeu has the branch he used for testing here:
http://cgit.collabora.com/git/user/tomeu/linux.git/log/?h=3.17rc4-xhci


Hmm. That git server was quite reluctant to cough up its bits, but it 
did eventually.



You'll also need the firmware Andrew Chew posted:
https://patchwork.ozlabs.org/patch/384013/


The XHCI driver can't load its firmware unless it's a module; if I make 
it built-in, it fails immediately with error -2 during Direct firmware 
loading. The driver needs to work with either immediate or deferred 
firmware loading.


I tried three USB3 mass storage devices, and they all appeared to work 
without issue.


0781:5581 SanDisk Corp.
154b:fa0a PNY
0781:5580 SanDisk Corp.

I tried the following USB2 devices without issue:

0781:5575 SanDisk Corp.
0b95:7720 ASIX Electronics Corp. AX88772
045e:0095 Microsoft Corp. IntelliMouse Explorer 4.0 (IntelliPoint)
046d:c313 Logitech, Inc. Internet 350 Keyboard

The following USB2 devices had problems:

0b95:7720 ASIX Electronics Corp. AX88772


[  489.140536] usb 1-3: new high-speed USB device number 81 using xhci-tegra
[  489.260860] usb 1-3: device descriptor read/64, error -71
[  489.370804] xhci-tegra 7009.usb: Setup ERROR: setup context command for 
slot 1.
[  489.378463] usb 1-3: hub failed to enable device, error -22
[  489.500531] usb 1-3: new high-speed USB device number 82 using xhci-tegra
[  489.655708] usb 1-3: can't set config #1, error -71
[  489.661231] usb 1-3: USB disconnect, device number 82
[  489.940531] usb 1-3: new high-speed USB device number 83 using xhci-tegra
[  490.060860] usb 1-3: device descriptor read/64, error -71
[  490.170805] xhci-tegra 7009.usb: Setup ERROR: setup context command for 
slot 1.
[  490.178462] usb 1-3: hub failed to enable device, error -22

(repeats over and over)

15a4:1336 Afatech Technologies, Inc. SDHC/MicroSD/MMC/MS/M2/CF/XD Flash 
Card Reader


The power light comes on, and the activity light just keeps flashing 
fast. Usually the activity light flashes a couple times and then turns 
off. There is nothing in dmesg at all for this device.


05e3:0608 Genesys Logic, Inc. USB-2.0 4-Port HUB

Not detected. There is nothing in dmesg at all for this device.

1bcf:0c31 Sunplus Innovation Technology Inc. SPIF30x Serial-ATA bridge

Not detected. There is nothing in dmesg at all for this device.
--
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/9] Tegra xHCI support

[Stephen Warren]

On 09/16/2014 10:57 AM, Andrew Bresticker wrote:

On Tue, Sep 16, 2014 at 8:26 AM, Stephen Warren swar...@wwwdotorg.org wrote:

On 09/15/2014 01:30 PM, Andrew Bresticker wrote:


On Mon, Sep 15, 2014 at 11:09 AM, Stephen Warren swar...@wwwdotorg.org
wrote:


On 09/15/2014 11:06 AM, Andrew Bresticker wrote:


On Mon, Sep 15, 2014 at 12:00 AM, Tomeu Vizoso to...@tomeuvizoso.net
wrote:


On 12 September 2014 18:37, Andrew Bresticker abres...@chromium.org
wrote:


On Tue, Sep 9, 2014 at 1:21 AM, Tomeu Vizoso to...@tomeuvizoso.net
wrote:


On 8 September 2014 18:22, Andrew Bresticker abres...@chromium.org
wrote:


On Mon, Sep 8, 2014 at 8:34 AM, Tomeu Vizoso to...@tomeuvizoso.net
wrote:


On 2 September 2014 23:34, Andrew Bresticker
abres...@chromium.org wrote:


Tested on Venice2, Jetson TK1, and Big with a variety of USB2.0
and
USB3.0 memory sticks and ethernet dongles using controller
firmware
recently posted by Andrew Chew [2].


...


Stephen, Thierry, have either of you had a chance to test this series?



I haven't had a chance to yet. I just went to try it out, and found that it
depends on a whole slew of other patches that I don't have. Is there a git
branch somewhere to save me having to track down all the dependencies?


Yes, Tomeu has the branch he used for testing here:
http://cgit.collabora.com/git/user/tomeu/linux.git/log/?h=3.17rc4-xhci


Hmm. That git server was quite reluctant to cough up its bits, but it did
eventually.


You'll also need the firmware Andrew Chew posted:
https://patchwork.ozlabs.org/patch/384013/


The XHCI driver can't load its firmware unless it's a module; if I make it
built-in, it fails immediately with error -2 during Direct firmware
loading. The driver needs to work with either immediate or deferred
firmware loading.


If you want the driver to be built-in, you'll either need to build the
firmware in as well (i.e. EXTRA_FIRMWARE) or enable
FW_LOADER_USER_HELPER_FALLBACK to load with userspace/uevent (though
apparently this is deprecated).


Hmm. I didn't have to do that for the Atmel touchpad driver to work, but 
perhaps it's just ignoring a firmware loading error, and continuing with 
whatever is in the device's flash already.


It seems odd that such a fundamental feature would require a deprecated 
Kconfig option. Is there some replacement that does the same thing that 
isn't deprecated? The Kconfig help for the option doesn't say anything 
useful...


Oh, this option doesn't actually seem to work. I see the following in dmesg:


[root@swarren-dt ~]# dmesg|grep -i -e xhci -e firmware
[1.461773] xhci-tegra 7009.usb: Failed to get supply 'avddio-pex': -517
[1.468930] platform 7009.usb: Driver xhci-tegra requests probe deferral
[2.567966] xhci-tegra 7009.usb: Direct firmware load for 
nvidia/tegra124/xusb.bin failed with error -2
[2.577786] xhci-tegra 7009.usb: Falling back to user helper


... but:

[root@swarren-dt ~]# lsusb
unable to initialize libusb: -99

Perhaps systemd-udevd doesn't implement firmware loading; is it 
user-space udev that's deprecated implementing user-space firmware 
loading, rather than the kernel deprecating support for calling out to 
user space?


This sucks, because now I can't just TFTP boot kernels but somehow have 
to get updated kernel modules onto my device every time before testing a 
new kernel build. That's a huge time-sink, unless I work out NFS root, 
which probably isn't properly or easily supported by any distro.

--
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/9] Tegra xHCI support

[Stephen Warren]

On 09/16/2014 04:46 PM, Andrew Bresticker wrote:

On Tue, Sep 16, 2014 at 9:57 AM, Andrew Bresticker
abres...@chromium.org wrote:

On Tue, Sep 16, 2014 at 8:26 AM, Stephen Warren swar...@wwwdotorg.org wrote:

On 09/15/2014 01:30 PM, Andrew Bresticker wrote:


On Mon, Sep 15, 2014 at 11:09 AM, Stephen Warren swar...@wwwdotorg.org
wrote:


On 09/15/2014 11:06 AM, Andrew Bresticker wrote:


On Mon, Sep 15, 2014 at 12:00 AM, Tomeu Vizoso to...@tomeuvizoso.net
wrote:


On 12 September 2014 18:37, Andrew Bresticker abres...@chromium.org
wrote:


On Tue, Sep 9, 2014 at 1:21 AM, Tomeu Vizoso to...@tomeuvizoso.net
wrote:


On 8 September 2014 18:22, Andrew Bresticker abres...@chromium.org
wrote:


On Mon, Sep 8, 2014 at 8:34 AM, Tomeu Vizoso to...@tomeuvizoso.net
wrote:


On 2 September 2014 23:34, Andrew Bresticker
abres...@chromium.org wrote:


Tested on Venice2, Jetson TK1, and Big with a variety of USB2.0
and
USB3.0 memory sticks and ethernet dongles using controller
firmware
recently posted by Andrew Chew [2].


...


Stephen, Thierry, have either of you had a chance to test this series?



I haven't had a chance to yet. I just went to try it out, and found that
it
depends on a whole slew of other patches that I don't have. Is there a
git
branch somewhere to save me having to track down all the dependencies?



Yes, Tomeu has the branch he used for testing here:
http://cgit.collabora.com/git/user/tomeu/linux.git/log/?h=3.17rc4-xhci



Hmm. That git server was quite reluctant to cough up its bits, but it did
eventually.


You'll also need the firmware Andrew Chew posted:
https://patchwork.ozlabs.org/patch/384013/



The XHCI driver can't load its firmware unless it's a module; if I make it
built-in, it fails immediately with error -2 during Direct firmware
loading. The driver needs to work with either immediate or deferred
firmware loading.


If you want the driver to be built-in, you'll either need to build the
firmware in as well (i.e. EXTRA_FIRMWARE) or enable
FW_LOADER_USER_HELPER_FALLBACK to load with userspace/uevent (though
apparently this is deprecated).


The following USB2 devices had problems:

0b95:7720 ASIX Electronics Corp. AX88772


[  489.140536] usb 1-3: new high-speed USB device number 81 using
xhci-tegra
[  489.260860] usb 1-3: device descriptor read/64, error -71
[  489.370804] xhci-tegra 7009.usb: Setup ERROR: setup context command
for slot 1.
[  489.378463] usb 1-3: hub failed to enable device, error -22
[  489.500531] usb 1-3: new high-speed USB device number 82 using
xhci-tegra
[  489.655708] usb 1-3: can't set config #1, error -71
[  489.661231] usb 1-3: USB disconnect, device number 82
[  489.940531] usb 1-3: new high-speed USB device number 83 using
xhci-tegra
[  490.060860] usb 1-3: device descriptor read/64, error -71
[  490.170805] xhci-tegra 7009.usb: Setup ERROR: setup context command
for slot 1.
[  490.178462] usb 1-3: hub failed to enable device, error -22


(repeats over and over)

15a4:1336 Afatech Technologies, Inc. SDHC/MicroSD/MMC/MS/M2/CF/XD Flash Card
Reader

The power light comes on, and the activity light just keeps flashing fast.
Usually the activity light flashes a couple times and then turns off. There
is nothing in dmesg at all for this device.

05e3:0608 Genesys Logic, Inc. USB-2.0 4-Port HUB

Not detected. There is nothing in dmesg at all for this device.

1bcf:0c31 Sunplus Innovation Technology Inc. SPIF30x Serial-ATA bridge

Not detected. There is nothing in dmesg at all for this device.


Thanks, I'll try to figure out what's going on here.


Grr... this is due to the unfortunate UTMI pad controller design on Tegra.

Here's the issue:  When UTMI pad 0 is assigned to the EHCI controller
(as is currently the case on Jetson-TK1), the UTMI parameters from
UTMIP_BIAS_CFG0 in the global UTMI pad register space are used
regardless of the owner of the UTMI pads.  If pad 0 is assigned to the
XUSB controller, then parameters in USB2_BIAS_PAD_CTL0 in the XUSB
register space are used for all UTMI pads (again, regardless of
ownership).

I wasn't able to reproduce before because I always TFTP booted and
U-Boot programs the UTMI parameters correctly when starting the EHCI
controllers.  I suspect you and Tomeu were booting without starting
the EHCI controllers and thus were stuck with the POR values.


I am loading the kernel over the PCIe Ethernet on Jetson TK1. However, 
the U-Boot boot scripts do usb start first, so I would assume all the 
USB set is already done either way, before U-Boot attempts to probe USB 
devices.



The easiest way to fix this is to just assign UTMI port 0 (i.e. the
OTG port) to the XUSB controller.  AFAIK, device mode on Tegra isn't
supported yet in the kernel, so this shouldn't break any existing use
cases.  If we wanted to support device mode in the future, it would
just have to be done with the XUSB controller instead.  The
alternative is some ugly driver that programs the correct register set
depending on which controller UTMI port 0

Re: [PATCH v3 0/9] Tegra xHCI support

[Stephen Warren]

On 09/15/2014 11:06 AM, Andrew Bresticker wrote:

On Mon, Sep 15, 2014 at 12:00 AM, Tomeu Vizoso to...@tomeuvizoso.net wrote:

On 12 September 2014 18:37, Andrew Bresticker abres...@chromium.org wrote:

On Tue, Sep 9, 2014 at 1:21 AM, Tomeu Vizoso to...@tomeuvizoso.net wrote:

On 8 September 2014 18:22, Andrew Bresticker abres...@chromium.org wrote:

On Mon, Sep 8, 2014 at 8:34 AM, Tomeu Vizoso to...@tomeuvizoso.net wrote:

On 2 September 2014 23:34, Andrew Bresticker abres...@chromium.org wrote:


Tested on Venice2, Jetson TK1, and Big with a variety of USB2.0 and
USB3.0 memory sticks and ethernet dongles using controller firmware
recently posted by Andrew Chew [2].


I have had mixed results when testing this on a Jetson TK1 board. Of 8
USB devices I tested with, about half where probed correctly, but the
other half repeatedly fail in hub_port_init because in the GET_STATUS
right after the reset, the C_PORT_CONNECTION bit is set.

I don't see any correlation between the failure and the kind of usb
device, but I don't have much experience with USB implementations
either.


Hmm... I haven't seen that before.  Which particular devices were you using?


Here they are:

work:

Genius keyboard: 05d5:6782
Logitech mouse: 046d:c06a
Denver MP3 player: 10d6:1100
Atheros Bluetooth dongle: 0cf3:3005

don't work:

MCS7830 ethernet dongle: 9710:7830
Genesys Logic hub: 05e3:0608


I tried the hub and it works just fine for me... Have you tried this
on any other boards like Big or Blaze?


I have a Blaze here, but haven't been able to get the external ports
to power up. I don't have schematics nor a strong interest to get that
fixed myself, as I can test my code just fine with the Blaze's
internal camera and the devices that do work on the Jetson.


Ok, since Blaze has been announced, hopefully we can post a
device-tree for it soon.


I'm using the firmware file that was posted recently:

xhci-tegra 7009.usb: Firmware timestamp: 2014-05-02 02:22:50 UTC,
Falcon state 0x20


Yup, I'm using the same firmware.


If things work for you with the same sources, .config and firmware, I
can only think of the hw being different (other hw revision?).


That, or perhaps bootloader differences?  I'm using mainline U-Boot.

Stephen, Thierry, have either of you had a chance to test this series?


I haven't had a chance to yet. I just went to try it out, and found that 
it depends on a whole slew of other patches that I don't have. Is there 
a git branch somewhere to save me having to track down all the dependencies?


Thierry is on vacation right now. I think he was expecting to return 
sometime this week, but the start of his vacation was delayed, so 
perhaps he'll be back a bit later too.


--
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/9] of: Add NVIDIA Tegra XUSB mailbox binding

[Stephen Warren]

On 09/02/2014 03:34 PM, Andrew Bresticker wrote:

Add device-tree bindings for the Tegra XUSB mailbox which will be used
for communication between the Tegra xHCI controller's firmware and the
host processor.


The series,
Reviewed-by: Stephen Warren swar...@nvidia.com

I'd like Thierry to review at least the XUSB pinctrl and all DT binding 
changes too, if possible.

--
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 6/9] usb: xhci: Add NVIDIA Tegra xHCI host-controller driver

[Stephen Warren]

On 09/02/2014 03:34 PM, Andrew Bresticker wrote:

Add support for the on-chip xHCI host controller present on Tegra SoCs.

The driver is currently very basic: it loads the controller with its
firmware, starts the controller, and is able to service messages sent
by the controller's firmware.  The hardware also supports device mode
as well as powergating of the SuperSpeed and host-controller logic
when not in use, but support for these is not yet implemented.



diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c



+static bool is_host_mbox_message(u32 cmd)
+{
+   switch (cmd) {
+   case MBOX_CMD_INC_SSPI_CLOCK:
+   case MBOX_CMD_DEC_SSPI_CLOCK:
+   case MBOX_CMD_INC_FALC_CLOCK:
+   case MBOX_CMD_DEC_FALC_CLOCK:
+   return true;
+   case MBOX_CMD_SET_BW:
+   /*
+* TODO: Request bandwidth once EMC scaling is supported.
+* Ignore for now since ACK/NAK is not required for SET_BW
+* messages.
+*/


I think that TODO belongs inside tegra_xhci_mbox_work() where the 
message would actually be handled, if implemented.

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

Re: [PATCH v2 3/9] of: Update Tegra XUSB pad controller binding for USB

[Stephen Warren]

On 08/27/2014 10:36 AM, Andrew Bresticker wrote:

On Mon, Aug 25, 2014 at 12:12 PM, Stephen Warren swar...@wwwdotorg.org wrote:

On 08/18/2014 11:08 AM, Andrew Bresticker wrote:

   - #phy-cells: Should be 1. The specifier is the index of the PHY to
reference.
 See dt-bindings/pinctrl/pinctrl-tegra-xusb.h for the list of valid
values.
+- mboxes: Must contain an entry for the XUSB PHY mailbox channel.
+  See ../mailbox/mailbox.txt for details.



Can we require the mbox-names property here, so that everything is looked up
by names. I know that the proposed mbox binding states that using indexes is
preferred over names, but that's just silly considering that names are
widely used in most other similar bindings, and are much easier to extend in
a backwards compatible fashion in the face of optional entries. As such, I'd
prefer that all Tegra bindings use foo-names properties where they exist.


Sure, will do.


+Optional properties:
+---
+- vbus-otg-{0,1,2}-supply: VBUS regulator for the corresponding UTMI pad.



Why -otg? It's quite possible to have a regulator for VBUS even on systems
that don't support OTG, but rather simply have the ability to turn VBUS off.


Because they're the VBUS supplies for the OTG 'lanes'.  It doesn't
really add anything, so I'll omit the -otg.


Ah right. In that case, if the lanes are named OTG lanes in the HW 
docs, I'm happy either way. If you did decide to keep the -otg, 
rewording as VBUS regulator for the corresponding OTG UTMI pad would 
make the meaning clearer.

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

Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver

[Stephen Warren]

On 08/27/2014 11:38 AM, Andrew Bresticker wrote:

On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren swar...@wwwdotorg.org wrote:

On 08/18/2014 11:08 AM, Andrew Bresticker wrote:


The Tegra xHCI controller's firmware communicates requests to the host
processor through a mailbox interface.  While there is only a single
communication channel, messages sent by the controller can be divided
into two groups: those intended for the PHY driver and those intended
for the host-controller driver.  This mailbox driver exposes the two
channels and routes incoming messages to the appropriate channel based
on the command encoded in the message.




diff --git a/drivers/mailbox/tegra-xusb-mailbox.c
b/drivers/mailbox/tegra-xusb-mailbox.c



+static struct mbox_chan *mbox_cmd_to_chan(struct tegra_xusb_mbox *mbox,
u32 cmd)
+{
+   switch (cmd) {
+   case MBOX_CMD_INC_FALC_CLOCK:
+   case MBOX_CMD_DEC_FALC_CLOCK:
+   case MBOX_CMD_INC_SSPI_CLOCK:
+   case MBOX_CMD_DEC_SSPI_CLOCK:
+   case MBOX_CMD_SET_BW:
+   return mbox-mbox.chans[TEGRA_XUSB_MBOX_CHAN_HOST];
+   case MBOX_CMD_SAVE_DFE_CTLE_CTX:
+   case MBOX_CMD_START_HSIC_IDLE:
+   case MBOX_CMD_STOP_HSIC_IDLE:
+   return mbox-mbox.chans[TEGRA_XUSB_MBOX_CHAN_PHY];
+   default:
+   return NULL;
+   }
+}



This makes me think that the CHAN_HOST/CHAN_PHY values are purely a facet of
the Linux driver's message de-multiplexing, rather than anything to do with
the HW.


Yup, they are...


I'm not even sure if it's appropriate for the low-level mailbox driver to
know about the semantics of the message, rather than simply sending them on
to the client driver? Perhaps when drivers register(?) for callbacks(?) for
messages, they should state which types of messages they want to listen to?


So there's not really a way for the client driver to tell the mailbox
driver which types of messages it wants to listen to on a particular
channel with the mailbox framework - it simply provides a way for
clients to bind with channels.  I think there are a couple of options
here, either: a) have a channel per message (as you mentioned in the
previous patch), which allows the client to only register for messages
(channels) it wants to handle, or b) extend the mailbox framework to
allow shared channels so that both clients can receive messages on the
single channel and handle messages appropriately.   The disadvantage
of (a) is that the commands are firmware defined and could
theoretically change between releases of the firmware, though I'm not
sure how common that is in practice.  So that leaves (b) - Jassi, what
do you think about having shared (non-exclusive) channels?


Another alternative might be for each client driver to hard-code a 
unique dummy channel ID so that each client still gets a separate 
exclusive channel, but then have the mbox driver broadcast each message 
to each of those channels. I'm not sure that would be any better though; 
adding (b) as an explicit option to the mbox subsystem would almost 
certainly be cleaner.



+static int tegra_xusb_mbox_probe(struct platform_device *pdev)




+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

+   if (!res)
+   return -ENODEV;



Should devm_request_mem_region() be called here to claim the region?


No, the xHCI host driver also needs to map these registers, so they
cannot be mapped exclusively here.


That's unfortunate. Having multiple drivers with overlapping register 
regions is not a good idea. Can we instead have a top-level driver map 
all the IO regions, then instantiate the various different 
sub-components internally, and divide up the address space. Probably via 
MFD or similar. That would prevent multiple drivers from touching the 
same register region.

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

Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver

[Stephen Warren]

On 08/27/2014 12:13 PM, Andrew Bresticker wrote:

On Wed, Aug 27, 2014 at 10:50 AM, Stephen Warren swar...@wwwdotorg.org wrote:

On 08/27/2014 11:38 AM, Andrew Bresticker wrote:


On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren swar...@wwwdotorg.org
wrote:


On 08/18/2014 11:08 AM, Andrew Bresticker wrote:


+static int tegra_xusb_mbox_probe(struct platform_device *pdev)





+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

+   if (!res)
+   return -ENODEV;




Should devm_request_mem_region() be called here to claim the region?



No, the xHCI host driver also needs to map these registers, so they
cannot be mapped exclusively here.



That's unfortunate. Having multiple drivers with overlapping register
regions is not a good idea. Can we instead have a top-level driver map all
the IO regions, then instantiate the various different sub-components
internally, and divide up the address space. Probably via MFD or similar.
That would prevent multiple drivers from touching the same register region.


Perhaps I'm misunderstanding, but I don't see how MFD would prevent us
from having to map this register space in two different locations -
the XUSB FPCI address space cannot be divided cleanly between host and
mailbox registers.  Or are you saying that there should be a separate
device driver that exposes an API for accessing this register space,
like the Tegra fuse or PMC drivers?


With MFD, there's typically a top-level driver for the HW module (or 
register space) that gets instantiated by the DT node. This driver then 
instantiates all the different sub-drivers that use that register space, 
and provides APIs for the sub-drivers to access the registers (either 
custom APIs or more recently by passing a regmap object down to the 
sub-drivers).


This top-level driver is the only driver that maps the space, and can 
manage sharing the space between the various sub-drivers.


That said, I haven't noticed many MFD drivers for MMIO devices. I 
certainly have seen multiple different drivers just re-mapping shared 
registers for themselves. It is simpler and does work. However, people 
usually mutter about it when it happens, since it's not clear which 
drivers are using what from the IO mapping registry. Using MFD or 
similar would allow the sharing to work in a clean fashion.

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

Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver

[Stephen Warren]

On 08/27/2014 03:56 PM, Andrew Bresticker wrote:

On Wed, Aug 27, 2014 at 11:19 AM, Stephen Warren swar...@wwwdotorg.org wrote:

On 08/27/2014 12:13 PM, Andrew Bresticker wrote:


On Wed, Aug 27, 2014 at 10:50 AM, Stephen Warren swar...@wwwdotorg.org
wrote:


On 08/27/2014 11:38 AM, Andrew Bresticker wrote:



On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren swar...@wwwdotorg.org
wrote:



On 08/18/2014 11:08 AM, Andrew Bresticker wrote:



+static int tegra_xusb_mbox_probe(struct platform_device *pdev)






+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

+   if (!res)
+   return -ENODEV;





Should devm_request_mem_region() be called here to claim the region?




No, the xHCI host driver also needs to map these registers, so they
cannot be mapped exclusively here.




That's unfortunate. Having multiple drivers with overlapping register
regions is not a good idea. Can we instead have a top-level driver map
all
the IO regions, then instantiate the various different sub-components
internally, and divide up the address space. Probably via MFD or similar.
That would prevent multiple drivers from touching the same register
region.



Perhaps I'm misunderstanding, but I don't see how MFD would prevent us
from having to map this register space in two different locations -
the XUSB FPCI address space cannot be divided cleanly between host and
mailbox registers.  Or are you saying that there should be a separate
device driver that exposes an API for accessing this register space,
like the Tegra fuse or PMC drivers?



With MFD, there's typically a top-level driver for the HW module (or
register space) that gets instantiated by the DT node. This driver then
instantiates all the different sub-drivers that use that register space, and
provides APIs for the sub-drivers to access the registers (either custom
APIs or more recently by passing a regmap object down to the sub-drivers).

This top-level driver is the only driver that maps the space, and can manage
sharing the space between the various sub-drivers.


So if I'm understanding correctly, we end up with something like this:

usb@7009 {
 compatible = nvidia,tegra124-xusb;
 reg = 0x0 0x7009 0x0 0x8000, // xHCI host registers
   0x0 0x70098000 0x0 0x1000, // FPCI registers
   0x0 0x70099000 0x0 0x1000; // IPFS registers
 interrupts = GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH, // host interrupt
  GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH; // mailbox interrupt

 host-controller {
 compatible = nvidia,tegra124-xhci;
 ...
 };

 mailbox {
 compatible = nvidia,tegra124-xusb-mailbox;
...
 };
};

To be honest though, this seems like overkill to share a couple of
registers when no other resources are shared between the two.


Something like that, yes.

Given that the xusb driver knows that its HW module contains both an 
XHCI and XUSB mailbox chunk, those might not need to appear inside the 
main XUSB module, but could be hard-coded into the driver. They might 
serve as convenient containers for sub-device-specific properties though.


Other alternatives might be:

a) If the registers that are shared between drivers are distinct, then 
divide the reg values into non-overlapping lists. We have taken this 
approach for the MC/SMMU drivers on Tegra, although it's a horrible mess 
and Thierry is actively thinking about reverting that and doing it 
through MFD or something MFD-like.


b) Allow the same IO region to be claimed by multiple devices, perhaps 
with a new API so that it doesn't accidentally happen when not desired.


c) Ignore the issue and deal with the fact that not all driver usage 
shows up in /proc/iomem. This is what we have for the Tegra USB2 and 
USB2 PHY drivers, and this is (I think) what your current patch does.


To be honest, none of the options are that good; some end up with the 
correct result but are a pain to implement, but others are nice and 
simple yet /proc/iomem isn't complete. Given that, I'm personally not 
going to try and mandate one option or the other, so the current patch 
is fine. Food for thought though:-)


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

Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver

[Stephen Warren]

On 08/18/2014 11:08 AM, Andrew Bresticker wrote:

The Tegra xHCI controller's firmware communicates requests to the host
processor through a mailbox interface.  While there is only a single
communication channel, messages sent by the controller can be divided
into two groups: those intended for the PHY driver and those intended
for the host-controller driver.  This mailbox driver exposes the two
channels and routes incoming messages to the appropriate channel based
on the command encoded in the message.



diff --git a/drivers/mailbox/tegra-xusb-mailbox.c 
b/drivers/mailbox/tegra-xusb-mailbox.c



+#define XUSB_CFG_ARU_MBOX_CMD  0xe4
+#define  MBOX_FALC_INT_EN  BIT(27)
+#define  MBOX_PME_INT_EN   BIT(28)
+#define  MBOX_SMI_INT_EN   BIT(29)
+#define  MBOX_XHCI_INT_EN  BIT(30)
+#define  MBOX_INT_EN   BIT(31)


Those field names don't match the documentation in the TRM; they're 
called DEST_xxx rather than xxx_INT_EN. I'm not sure what that 
disconnect means (i.e. whether it's just a different naming choice, or 
there's some practical disconnect that will cause issues.)



+static struct mbox_chan *mbox_cmd_to_chan(struct tegra_xusb_mbox *mbox, u32 
cmd)
+{
+   switch (cmd) {
+   case MBOX_CMD_INC_FALC_CLOCK:
+   case MBOX_CMD_DEC_FALC_CLOCK:
+   case MBOX_CMD_INC_SSPI_CLOCK:
+   case MBOX_CMD_DEC_SSPI_CLOCK:
+   case MBOX_CMD_SET_BW:
+   return mbox-mbox.chans[TEGRA_XUSB_MBOX_CHAN_HOST];
+   case MBOX_CMD_SAVE_DFE_CTLE_CTX:
+   case MBOX_CMD_START_HSIC_IDLE:
+   case MBOX_CMD_STOP_HSIC_IDLE:
+   return mbox-mbox.chans[TEGRA_XUSB_MBOX_CHAN_PHY];
+   default:
+   return NULL;
+   }
+}


This makes me think that the CHAN_HOST/CHAN_PHY values are purely a 
facet of the Linux driver's message de-multiplexing, rather than 
anything to do with the HW.


I'm not even sure if it's appropriate for the low-level mailbox driver 
to know about the semantics of the message, rather than simply sending 
them on to the client driver? Perhaps when drivers register(?) for 
callbacks(?) for messages, they should state which types of messages 
they want to listen to?



+static irqreturn_t tegra_xusb_mbox_irq(int irq, void *p)



+   /* Clear mbox interrupts */
+   reg = mbox_readl(mbox, XUSB_CFG_ARU_SMI_INTR);
+   if (reg  MBOX_SMI_INTR_FW_HANG)
+   dev_err(mbox-mbox.dev, Controller firmware hang\n);
+   mbox_writel(mbox, reg, XUSB_CFG_ARU_SMI_INTR);



+   /*
+* Set the mailbox back to idle.  The recipient of the message is
+* responsible for sending an ACK/NAK, if necessary.
+*/
+   reg = mbox_readl(mbox, XUSB_CFG_ARU_MBOX_CMD);
+   reg = ~MBOX_SMI_INT_EN;
+   mbox_writel(mbox, reg, XUSB_CFG_ARU_MBOX_CMD);


Does the protocol not allow the remote firmware to send another message 
until the host has ack'd/nak'd the message; the code above turns off the 
IRQ that indicated to the host that a message was sent to it...



+static int tegra_xusb_mbox_probe(struct platform_device *pdev)



+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   if (!res)
+   return -ENODEV;


Should devm_request_mem_region() be called here to claim the region?


+   mbox-regs = devm_ioremap_nocache(pdev-dev, res-start,
+ resource_size(res));
+   if (!mbox-regs)
+   return -ENOMEM;


Is _nocache required? I don't see other drivers using it. I assume 
there's nothing special about the mbox registers.



+   mbox-irq = platform_get_irq(pdev, 0);
+   if (mbox-irq  0)
+   return mbox-irq;
+   ret = devm_request_irq(pdev-dev, mbox-irq, tegra_xusb_mbox_irq, 0,
+  dev_name(pdev-dev), mbox);


Is it possible for an IRQ to occur after tegra_xusb_mbox_remove() has 
returned, but before the cleanup for the devm IRQ allocation occurs? If 
that happens, will the code handle it gracefully, or crash?



+MODULE_ALIAS(platform:tegra-xusb-mailbox);


I don't think that's required; it should auto-load based on the 
of_device_id/MODULE_DEVICE_TABLE(of,...) table.



diff --git a/include/soc/tegra/xusb.h b/include/soc/tegra/xusb.h



+#define TEGRA_XUSB_MBOX_NUM_CHANS 2 /* host + phy */


I'd rather see that definition in the same place as the 
TEGRA_XUSB_MBOX_CHAN_* values it refers to. Otherwise, it'd be quite 
easy to add values without updating this constant.

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

Re: [PATCH v2 1/9] of: Add NVIDIA Tegra XUSB mailbox binding

[Stephen Warren]

On 08/25/2014 12:48 PM, Stephen Warren wrote:

On 08/18/2014 11:08 AM, Andrew Bresticker wrote:

Add device-tree bindings for the Tegra XUSB mailbox which will be used
for communication between the Tegra xHCI controller's firmware and the
host processor.



diff --git
a/Documentation/devicetree/bindings/mailbox/nvidia,tegra124-xusb-mbox.txt
b/Documentation/devicetree/bindings/mailbox/nvidia,tegra124-xusb-mbox.txt



+NVIDIA Tegra XUSB mailbox
+=
+
+The Tegra XUSB mailbox is used by the Tegra xHCI controller's
firmware to
+communicate requests to the host and PHY drivers.
+
+Required properties:
+
+ - compatible: Should be nvidia,tegra124-xusb-mbox.
+ - reg: Address and length of the XUSB FPCI registers.
+ - interrupts: XUSB mailbox interrupt.
+ - #mbox-cells: Should be 1.  The specifier is the index of the
mailbox to
+   reference.  See dt-bindings/mailbox/tegra-xusb-mailbox.h for the
list
+   of valid values.


Is there a common mailbox binding somewhere? I couldn't find one. While
the text above specifies the value for #mbox-cells, it doesn't specify
the details of what the property is used for (i.e. there's no
documentation of the consumer-side of this property, for parsing the
mboxes property). Typically, that would be part of a subsystem's common
binding document, and that document would be referenced here.


Ah, I see it's still being developed. I found it at:
http://lkml.iu.edu/hypermail/linux/kernel/1408.0/00201.html

It would be good to mention that the semantics of this property are 
defined by ../mailbox/mailbox.txt.

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

Re: [PATCH v2 3/9] of: Update Tegra XUSB pad controller binding for USB

[Stephen Warren]

On 08/18/2014 11:08 AM, Andrew Bresticker wrote:

Add new bindings used for USB support by the Tegra XUSB pad controller.
This includes additional PHY types, USB-specific pinconfig properties, etc.


I'll mainly defer to Thierry for this patch, since he's the expert on 
this HW module.



diff --git 
a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt 
b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt



  - #phy-cells: Should be 1. The specifier is the index of the PHY to reference.
See dt-bindings/pinctrl/pinctrl-tegra-xusb.h for the list of valid values.
+- mboxes: Must contain an entry for the XUSB PHY mailbox channel.
+  See ../mailbox/mailbox.txt for details.


Can we require the mbox-names property here, so that everything is 
looked up by names. I know that the proposed mbox binding states that 
using indexes is preferred over names, but that's just silly considering 
that names are widely used in most other similar bindings, and are much 
easier to extend in a backwards compatible fashion in the face of 
optional entries. As such, I'd prefer that all Tegra bindings use 
foo-names properties where they exist.



+Optional properties:
+---
+- vbus-otg-{0,1,2}-supply: VBUS regulator for the corresponding UTMI pad.


Why -otg? It's quite possible to have a regulator for VBUS even on 
systems that don't support OTG, but rather simply have the ability to 
turn VBUS off.




- pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, sata-0:

  Valid functions for this group are: pcie, usb3, sata, rsvd.

+The nvidia,usb2-port-num property only applies and is required when
+the function is usb3.
+



There are 2 blank lines there.

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

Re: [PATCH v2 4/9] pinctrl: tegra-xusb: Add USB PHY support

[Stephen Warren]

On 08/18/2014 11:08 AM, Andrew Bresticker wrote:

In addition to the PCIe and SATA PHYs, the XUSB pad controller also
supports 3 UTMI, 2 HSIC, and 2 USB3 PHYs.  Each USB3 PHY uses a single
PCIe or SATA lane and is mapped to one of the three UTMI ports.

The xHCI controller will also send messages intended for the PHY driver,
so request and listen for messages on the mailbox's PHY channel.


I'd like a review from Thierry here as the HW expert.

I need an ack from LinusW in order to take this pinctrl patch through 
the Tegra tree.



diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c 
b/drivers/pinctrl/pinctrl-tegra-xusb.c



+static int usb3_phy_power_on(struct phy *phy)
+{
+   struct tegra_xusb_padctl *padctl = phy_get_drvdata(phy);
+   int port = usb3_phy_to_port(phy);
+   int lane = padctl-usb3_ports[port].lane;
+   u32 value, offset;
+
+   if (!is_pcie_or_sata_lane(lane)) {
+   dev_err(padctl-dev, USB3 PHY %d mapped to invalid lane: %d\n,
+   port, lane);
+   return -EINVAL;
+   }


An aside: This implies that the SATA driver should be talking to this 
pinctrl driver and explicitly powering on the XUSB pins. However, the 
SATA driver doesn't depend on this series. I'm a bit confused how that 
works. Perhaps it's just by accident? Mikko, can you comment?



+static int utmi_phy_to_port(struct phy *phy)
+{
+   struct tegra_xusb_padctl *padctl = phy_get_drvdata(phy);
+   int i;
+
+   for (i = 0; i  TEGRA_XUSB_UTMI_PHYS; i++) {
+   if (phy == padctl-phys[TEGRA_XUSB_PADCTL_UTMI_P0 + i])
+   break;
+   }
+   BUG_ON(i == TEGRA_XUSB_UTMI_PHYS);


Can this be triggered by e.g. bad DT content? If so, returning an error 
would be nicer. The comment applies to other xxx_to_port() functions.



@@ -896,6 +1933,22 @@ static int tegra_xusb_padctl_probe(struct platform_device 
*pdev)



+   for (i = 0; i  TEGRA_XUSB_USB3_PHYS; i++) {
+   char prop[sizeof(nvidia,usb3-port-N-lane)];
+   u32 lane;
+
+   sprintf(prop, nvidia,usb3-port-%d-lane, i);
+   if (!of_property_read_u32(pdev-dev.of_node, prop, lane)) {
+   if (!is_pcie_or_sata_lane(lane)) {
+   err = -EINVAL;
+   goto unregister;


It'd be nice to print a message so that the user/developer knows what's 
wrong with the DT.

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

Re: [PATCH v2 5/9] of: Add NVIDIA Tegra xHCI controller binding

[Stephen Warren]

On 08/18/2014 11:08 AM, Andrew Bresticker wrote:

Add device-tree binding documentation for the xHCI controller present
on Tegra124 and later SoCs.



diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt 
b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt



+Required properties:
+



+ - mboxes: Must contain an entry for the XUSB host mailbox channel.
+   See ../mailbox/mailbox.txt for details.


I'd like to use mbox-names here too.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/9] usb: xhci: Add NVIDIA Tegra xHCI host-controller driver

[Stephen Warren]

On 08/18/2014 11:08 AM, Andrew Bresticker wrote:

Add support for the on-chip xHCI host controller present on Tegra SoCs.

The driver is currently very basic: it loads the controller with its
firmware, starts the controller, and is able to service messages sent
by the controller's firmware.  The hardware supports device mode as
well as lower-power operating modes, but support for these is not yet
implemented here.


Just one minor comment below.

I'd like an ack from a USB maintainer so that this patch can be taken 
through the Tegra tree along with the rest of the series, which has 
various dependencies.



+MODULE_ALIAS(platform:xhci-tegra);


I don't think that's needed; MODULE_DEVICE_TABLE(of, 
tegra_xhci_of_match) should be enough upstream.

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

Re: [PATCH v2 0/9] Tegra xHCI support

[Stephen Warren]

On 08/18/2014 11:08 AM, Andrew Bresticker wrote:

This series adds support for xHCI on NVIDIA Tegra SoCs.  This includes:
  - adding a driver for the mailbox used to communicate with the xHCI
controller's firmware,
  - extending the XUSB pad controller driver to support the USB PHY
types (UTMI, HSIC, and USB3), and
  - adding a xHCI host-controller driver.


Can you please remind us what dependencies exist between the patches? In 
other words, is it possible to apply parts of the series to their usual 
subsystem branches, or does the whole thing need to go into a single 
topic branch in order to avoid compile-time problems, or run-time 
problems in features unrelated to the new functionality? Thanks.

--
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 0/3] Tegra USB probe order issue fix

[Stephen Warren]

On 07/02/2014 08:02 AM, Alan Stern wrote:
 On Wed, 2 Jul 2014, Tuomas Tynkkynen wrote:
 
 Hi all,

 This series fixes a probe order issue with the Tegra EHCI driver.
 Basically, the register area of the 1st USB controller contains some
 registers that are global to all of the controllers, but that are also
 cleared when reset is asserted to the 1st controller. So if (say) the
 3rd controller would be the first one to finish probing successfully,
 then the reset that happens during the 1st controller's probe would
 result in broken USB. So the before doing anything with the USB HW,
 we should reset the 1st controller once, and then never ever reset
 it again.
 
 This sounds very much like the sort of thing that ought to be described 
 in DT.  It is a hardware dependence, and DT exists for the purpose of 
 describing the hardware.

DT is more about describing the HW, not how SW has to use the HW.
probe() ordering is a SW issue, not a HW description. It's driver
knowledge that the HW resets have to run in a certain order, and if the
driver didn't actually reset the HW ever (but rather, re-programmed all
registers so reset was never needed), then order wouldn't be relevant

DT certainly doesn't have any mechanism for describing probe order or
anything like that, although you can fake it out by adding phandles
between nodes, and having SW wait for the driver for the referenced node
to probe first. That won't work here though, since there's no guarantee
that the USB1 node will actually be enabled (that USB port might not be
hooked up on the board, hence the DT node will be disabled), so we can't
rely on a driver for it ever appearing.
--
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 0/3] Tegra USB probe order issue fix

[Stephen Warren]

On 07/02/2014 10:09 AM, Alan Stern wrote:
 On Wed, 2 Jul 2014, Stephen Warren wrote:
 
 On 07/02/2014 08:02 AM, Alan Stern wrote:
 On Wed, 2 Jul 2014, Tuomas Tynkkynen wrote:

 Hi all,

 This series fixes a probe order issue with the Tegra EHCI driver.
 Basically, the register area of the 1st USB controller contains some
 registers that are global to all of the controllers, but that are also
 cleared when reset is asserted to the 1st controller. So if (say) the
 3rd controller would be the first one to finish probing successfully,
 then the reset that happens during the 1st controller's probe would
 result in broken USB. So the before doing anything with the USB HW,
 we should reset the 1st controller once, and then never ever reset
 it again.

 This sounds very much like the sort of thing that ought to be described 
 in DT.  It is a hardware dependence, and DT exists for the purpose of 
 describing the hardware.

 DT is more about describing the HW, not how SW has to use the HW.
 
 Tuomas wrote: the register area of the 1st USB controller contains
 some registers that are global to all of the controllers, but that are
 also cleared when reset is asserted to the 1st controller.  That is
 very much an attribute of the hardware and so DT should describe it.

So you want to add a Boolean DT property to the first USB controller
node indicating that it has the shared reset? That would be fine, but
would only replace the content of tegra_find_usb1_node(); much of the
rest of the patch would remain the same.

I guess in this case, it's fine to require DT changes to enable the new
feature of fixing this issue, so long as the code continues to work the
same as it currently does with old DTs. Due to the backwards
compatibility requirement, the patch will end up slightly more complex,
but hopefully not too bad.

 probe() ordering is a SW issue, not a HW description. It's driver
 knowledge that the HW resets have to run in a certain order, and if the
 driver didn't actually reset the HW ever (but rather, re-programmed all
 registers so reset was never needed), then order wouldn't be relevant

 DT certainly doesn't have any mechanism for describing probe order or
 anything like that, although you can fake it out by adding phandles
 between nodes, and having SW wait for the driver for the referenced node
 to probe first. That won't work here though, since there's no guarantee
 that the USB1 node will actually be enabled (that USB port might not be
 hooked up on the board, hence the DT node will be disabled), so we can't
 rely on a driver for it ever appearing.
 
 I wasn't talking about probe order; I was talking about the fact that 
 registers pertinent to the later controllers are in the reset domain of 
 the first controller.
 
 Alan Stern

--
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/3] USB: PHY: tegra: Call tegra_usb_phy_close only on device removal

[Stephen Warren]

On 07/01/2014 03:08 PM, Tuomas Tynkkynen wrote:
 tegra_usb_phy_close() is supposed to undo the effects of
 tegra_usb_phy_init(). It is also currently added as the USB PHY shutdown
 callback, which is wrong, since tegra_usb_phy_init() is only called
 during probing wheras the shutdown callback can get called multiple
 times. This then leads to warnings about unbalanced regulator_disable if
 the EHCI driver is unbound and bound again at runtime.

The series,
Reviewed-by: Stephen Warren swar...@nvidia.com

 diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c

 -static void tegra_usb_phy_close(struct usb_phy *x)
 +static void tegra_usb_phy_close(struct tegra_usb_phy *phy)

If this function undoes what _init does, it seems it should be called
_fini not _close. But that's bike-shedding perhaps.
--
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 v1 4/9] pinctrl: tegra-xusb: Add USB PHY support

[Stephen Warren]

On 06/27/2014 09:00 AM, Felipe Balbi wrote:
 On Wed, Jun 25, 2014 at 04:30:48PM -0700, Andrew Bresticker wrote:
 +static int usb3_phy_power_on(struct phy *phy)
 +{
 + struct tegra_xusb_padctl *padctl = phy_get_drvdata(phy);
 + int port = usb3_phy_to_port(phy);
 + int lane = padctl-usb3_ports[port].lane;
 + u32 value, offset;
 +
 + value = padctl_readl(padctl, XUSB_PADCTL_IOPHY_USB3_PADX_CTL2(port));
 + value = ~((XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_WANDER_MASK 
 + XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_WANDER_SHIFT) |
 +(XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_EQ_MASK 
 + XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_EQ_SHIFT) |
 +(XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_CDR_CNTL_MASK 
 + XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_CDR_CNTL_SHIFT));

 Hmm. So there is a lot of PHY stuff here after all.

 However, the PHYs implemented here appear to implement very low-level
 I/O pad code, whereas the PHYs we have for our USB 2.0 controller are
 somewhat higher-level; they're more USB-oriented than just IO pad
 oriented. Do you know which level of abstraction a Linux PHY object is
 supposed to be? I could never get an answer when I asked before.

 The only other PHY driver I've worked with (Exynos USB2/3 PHYs) also
 mainly only did low-level pad control stuff, but looking at a couple
 of other USB PHYs (MSM, MV), there appear to be others that have
 higher-level USB stuff in the PHY driver.  Perhaps Kishon or Felipe
 could offer us some guidance?
 
 well, if you're adding a new driver, I'd rather see folks moving over to
 the generic phy framework (drivers/phy) because we're trying really hard
 to get rid of drivers/usb/phy/. And I think, in your case, it's actually
 ok to use pinctrl because you actually are muxing pads to the USB3 PHY,
 you just do it lazyly.

What I'm looking for is a good definition of exactly what a PHY is
supposed to be in Linux.

Is it purely something that turns some IO pads/drivers off/on, and
nothing more?

Or, does the PHY concept encompass protocol-specific concepts such as
USB VBUS enable, USB VBUS detection, USB OTG switching,
UTMI-vs-ULPI-vs-HSIC selection... all of which are irrelevant of the PHY
(or at least IO pads) are used for SATA or PCIe instead.

This obviously affects which code goes in the PHY driver, and which in
the EHCI/XHCI controller driver.



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v1 6/9] usb: xhci: Add NVIDIA Tegra XHCI host-controller driver

[Stephen Warren]

On 06/27/2014 03:19 PM, Andrew Bresticker wrote:
 On Thu, Jun 26, 2014 at 11:07 AM, Stephen Warren swar...@wwwdotorg.org 
 wrote:
 On 06/25/2014 06:06 PM, Andrew Bresticker wrote:
 On Wed, Jun 25, 2014 at 3:37 PM, Stephen Warren swar...@wwwdotorg.org 
 wrote:
 On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 Add support for the on-chip XHCI host controller present on Tegra SoCs.

 The driver is currently very basic: it loads the controller with its
 firmware, starts the controller, and is able to service messages sent
 by the controller's firmware.  The hardware supports device mode as
 well as runtime power-gating, but support for these is not yet
 implemented here.
 
 diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
 
 +static int tegra_xhci_set_ss_clk(struct tegra_xhci_hcd *tegra,
 +  unsigned long rate)

 + switch (rate) {
 + case TEGRA_XHCI_SS_CLK_HIGH_SPEED:
 + /* Reparent to PLLU_480M. Set div first to avoid 
 overclocking */
 + old_parent_rate = clk_get_rate(clk_get_parent(clk));
 + new_parent_rate = clk_get_rate(tegra-pll_u_480m);
 + div = new_parent_rate / rate;
 + ret = clk_set_rate(clk, old_parent_rate / div);
 + if (ret)
 + return ret;
 + ret = clk_set_parent(clk, tegra-pll_u_480m);
 + if (ret)
 + return ret;

 Don't you need to call clk_set_rate() again after reparenting, since the
 divisor will be different, and the rounding too.

 Nope, the divider we set before remains in-tact after clk_set_parent().

 Oh I see, the clk_set_rate() call is setting up div so it's appropriate
 after the new parent is selected.

 Wouldn't it be better to just stop the clock, assert reset, reparent the
 clock, and then set the desired rate directly?
 
 I'm not sure how that would be better than making it more obvious as
 to how we arrive at the final rate.  Keep in mind that the XHCI host
 is running at this point (we usually get the scale-up message as a
 USB3 device is being enumerated) and that disabling the clock and/or
 asserting reset to the SS partition clock may not be the best idea...

Oh, this happens while the device is running rather than when
initializing it? Applying reset is probably a bad idea then. Still,
perhaps stopping the clock for a short time is fine? What about:

clk_disable_unprepare(clk);
clk_set_parent(clk, tegra-pll_u_480m);
clk_set_rate(clk, rate);
clk_prepare_enable(clk);

That seems much more direct to me. The code above feels over-complex to me.

If the clock really can't be stopped, then I suppose the existing code
in the patch is fine. I'd like to see a final clk_get_rate(clk) call
added, and the value compared against the expected value, to make sure
no rounding/truncation of the divider happened though.
--
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 v1 6/9] usb: xhci: Add NVIDIA Tegra XHCI host-controller driver

[Stephen Warren]

On 06/25/2014 06:06 PM, Andrew Bresticker wrote:
 On Wed, Jun 25, 2014 at 3:37 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 Add support for the on-chip XHCI host controller present on Tegra SoCs.

 The driver is currently very basic: it loads the controller with its
 firmware, starts the controller, and is able to service messages sent
 by the controller's firmware.  The hardware supports device mode as
 well as runtime power-gating, but support for these is not yet
 implemented here.

 diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c

 +static int tegra_xhci_set_ss_clk(struct tegra_xhci_hcd *tegra,
 +  unsigned long rate)

 + switch (rate) {
 + case TEGRA_XHCI_SS_CLK_HIGH_SPEED:
 + /* Reparent to PLLU_480M. Set div first to avoid overclocking 
 */
 + old_parent_rate = clk_get_rate(clk_get_parent(clk));
 + new_parent_rate = clk_get_rate(tegra-pll_u_480m);
 + div = new_parent_rate / rate;
 + ret = clk_set_rate(clk, old_parent_rate / div);
 + if (ret)
 + return ret;
 + ret = clk_set_parent(clk, tegra-pll_u_480m);
 + if (ret)
 + return ret;

 Don't you need to call clk_set_rate() again after reparenting, since the
 divisor will be different, and the rounding too.
 
 Nope, the divider we set before remains in-tact after clk_set_parent().

Oh I see, the clk_set_rate() call is setting up div so it's appropriate
after the new parent is selected.

Wouldn't it be better to just stop the clock, assert reset, reparent the
clock, and then set the desired rate directly?
--
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 v1 4/9] pinctrl: tegra-xusb: Add USB PHY support

[Stephen Warren]

On 06/25/2014 05:30 PM, Andrew Bresticker wrote:
 On Wed, Jun 25, 2014 at 3:12 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 In addition to the PCIe and SATA PHYs, the XUSB pad controller also
 supports 3 UTMI, 2 HSIC, and 2 USB3 PHYs.  Each USB3 PHY uses a single
 PCIe or SATA lane and is mapped to one of the three UTMI ports.


 diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c 
 b/drivers/pinctrl/pinctrl-tegra-xusb.c

 @@ -372,6 +720,193 @@ static int tegra_xusb_padctl_pinconf_group_set(struct 
 pinctrl_dev *pinctrl,
   padctl_writel(padctl, regval, lane-offset);
   break;

 + case TEGRA_XUSB_PADCTL_USB3_PORT_NUM:
 + if (value = TEGRA_XUSB_PADCTL_USB3_PORTS) {
 + dev_err(padctl-dev, Invalid USB3 port: 
 %lu\n,
 + value);
 + return -EINVAL;
 + }
 + if (!is_pcie_sata_lane(group)) {
 + dev_err(padctl-dev,
 + USB3 port not applicable for pin 
 %d\n,
 + group);
 + return -EINVAL;
 + }
 + padctl-usb3_ports[value].lane = group;
 + break;

 It feels odd to use pinctrl for a SW-only purpose. In other words, that
 chunk of code isn't writing the pinconf data to HW, but rather some
 internal variable.
 
 Well the mapping of lanes to USB3 ports is a hardware property and we
 do use it when programming the hardware later to choose which set of
 lane registers to program given a USB3 port, but it's true that it's
 not some value we program into HW directly.
 
 Perhaps it would make more sense for the DT binding to represent this
 data directly in a custom property that's parsed at probe() time. That
 way, pinctrl only touches real HW stuff.
 
 I'm on the fence about this.  If you or others feel strongly about
 this then I can make it a separate DT property and move it out of the
 pinctrl properties.

I'd certainly prefer to use pinctrl bindings only for things that get
directly written into HW. Other configuration data should be easy to
retrieve directly from properties.
--
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 v1 3/9] of: Update Tegra XUSB pad controller binding for USB

[Stephen Warren]

On 06/25/2014 04:25 PM, Andrew Bresticker wrote:
 Thanks for the review Stephen!
 
 On Wed, Jun 25, 2014 at 2:46 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 Add new bindings used for USB support by the Tegra XUSB pad controller.
 This includes additional PHY types, USB-specific pinconfig properties, etc.

 diff --git 
 a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt 
 b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt

 @@ -21,6 +21,12 @@ Required properties:
- padctl
  - #phy-cells: Should be 1. The specifier is the index of the PHY to 
 reference.
See dt-bindings/pinctrl/pinctrl-tegra-xusb.h for the list of valid 
 values.
 +- nvidia,xusb-mbox: Handle to the Tegra XUSB mailbox node.

 Why does the padctrl code need access to the XUSB mailbox?
 
 The XUSB firmware sends messages which make requests of the PHY (XUSB
 pad controller), such as idling/un-idling the HSIC PHYs or saving USB3
 PHY context.
 
 Isn't the padctrl HW module something that provides services to the XUSB
 code. I would have expected the XUSB node to reference the padctrl node.
 
 The XUSB padctrl HW does provide services to the XUSB host in the form
 of PHYs and it is through the PHY bindings that the host references
 the padctrl node.
 
 If notifications need to be sent back from XUSB padctrl to XUSB, then that
 seems like an internal SW detail that doesn't need to be represented in DT.
 
 I think you mean notifications need to be sent back from the XUSB host
 to the XUSB padctrl?  This is what the mailbox is for and I chose to
 have the padctrl refer to the mailbox since messages are sent from the
 mailbox which make requests to the PHY specifically and not the host
 (see above).

I've looked at the details of the mailbox messages a bit more now. It
seems that the firmware running on the XUSB controller sends a variety
of different messages, some of which are relevant to the XHCI controller
driver and some relevant to the PHY/PAD driver. It's a pity these
different message streams are intermixed, but I guess that's not changing.

As such, I think at this stage it does make sense for the mailbox to be
represented as a separate node, with each of the XHCI controller and USB
PADCTL nodes referring to the mailbox node by phandle.

I'm still not 100% sure about whether the PHY driver is the same level
of abstraction intended by the Linux kernel's PHY layer. Pending that
discussion's results, the PHY message from the firmware may not go to
a Linux kernel PHY but some layer above which might get subsumed into
the overall XHCI controller driver, which would change my argument above
a bit.
--
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 v1 3/9] of: Update Tegra XUSB pad controller binding for USB

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 Add new bindings used for USB support by the Tegra XUSB pad controller.
 This includes additional PHY types, USB-specific pinconfig properties, etc.

 diff --git 
 a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt 
 b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt

 @@ -21,6 +21,12 @@ Required properties:
- padctl
  - #phy-cells: Should be 1. The specifier is the index of the PHY to 
 reference.
See dt-bindings/pinctrl/pinctrl-tegra-xusb.h for the list of valid 
 values.
 +- nvidia,xusb-mbox: Handle to the Tegra XUSB mailbox node.

Why does the padctrl code need access to the XUSB mailbox? Isn't the
padctrl HW module something that provides services to the XUSB code. I
would have expected the XUSB node to reference the padctrl node. If
notifications need to be sent back from XUSB padctrl to XUSB, then that
seems like an internal SW detail that doesn't need to be represented in DT.

 +Optional properties:
 +---
 +- vbus-otg-{0,1,2}-supply: VBUS regulator for the corresponding UTMI pad.

Isn't VBUS something that's controlled by the USB PHY? I think the PHYs
are part of the XUSB controller, whereas the XUSB pad control is just
the low level IO pad HW.

 +- nvidia,usb3-port-num: USB3 port (0 or 1) to which the lane is mapped.
 +- nvidia,usb2-port-num: USB2 port (0, 1, or 2) to which the lane is mapped.
 +- nvidia,hsic-strobe-trim: HSIC strobe trimmer value.
 +- nvidia,hsic-rx-strobe-trim: HSIC RX strobe trimmer value.
 +- nvidia,hsic-rx-data-trim: HSIC RX data trimmer value.
 +- nvidia,hsic-tx-rtune-n: HSIC TX RTUNEN value.
 +- nvidia,hsic-tx-rtune-p: HSIC TX RTUNEP value.
 +- nvidia,hsic-tx-slew-n: HSIC TX SLEWN value.
 +- nvidia,hsic-tx-slew-p: HSIC TX SLEWP value.
 +- nvidia,hsic-auto-term: Enables HSIC AUTO_TERM. (0: no, 1: yes)

I wonder if some of that isn't part of the PHY not the pads?

  Valid functions for this group are: snps, xusb, uart, rsvd.
  
 -The nvidia,iddq property does not apply to this group.
 +The nvidia,iddq, nvidia,usb3-port-num, nvidia,usb2-port-num, and
 +nvidia,hsic-* properties do not apply to this group.

Given the increased number of properties we now have, it seems better to
list the properties that *do* apply to each group, rather than those
that don't.
--
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 v1 1/9] of: Add NVIDIA Tegra XUSB mailbox binding

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 Add device-tree bindings for the Tegra XUSB mailbox which will be used
 for communication between the Tegra XHCI controller and the host.

Sorry for the slow review.

 diff --git 
 a/Documentation/devicetree/bindings/mailbox/nvidia,tegra124-xusb-mbox.txt 
 b/Documentation/devicetree/bindings/mailbox/nvidia,tegra124-xusb-mbox.txt

 +NVIDIA Tegra XUSB mailbox
 +=
 +
 +The Tegra XUSB mailbox is used by the Tegra XHCI controller's firmware to
 +communicate with the host.

Isn't the mailbox an internal implementation detail of the XUSB controller.

In other words, I'd naively think that there isn't a standalone generic
mailbox that can be used by anything, but we just happen to want to use
for XUSB. Rather, there's an XUSB controller, and part of the interface
to that controller is a mailbox.

As such, I don't think we want a standalone mailbox node in DT. Rather,
we should add the required reg and interrupt values into the XUSB DT node.

The driver for that XUSB HW module can either:

a) Register as both a mailbox driver and an EHCI driver.

b) Spawn a child device to instantiate the mailbox driver.

Perhaps (b) could be assisted by using the MFD framework?
--
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 v1 5/9] of: Add NVIDIA Tegra XHCI controller binding

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 Add device-tree binding documentation for the XHCI controller present
 on Tegra124 and later SoCs.

 diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt 
 b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt

 +Required properties:
 +

 + - clock-names: Must include the following entries:
 +- xusb_host
 +- xusb_falcon_src
 +- xusb_ss
 +- xusb_ss_src
 +- xusb_hs_src
 +- xusb_fs_src
 +- pll_u_480m
 +- clk_m
 +- pll_e

 + - reset-names: Must include the following entries:
 +   - xusb_host
 +   - xusb_ss

Usually the CAR has a reset control for each clock. So, I would expect
as many entries in reset-names as in clock-names. Even if the SW doesn't
currently touch all the reset lines, we should make sure the binding
requires them to be present so that any DT will contain the entries if
they're ever needed in the future.

In the CAR documentation, I see XUSB_DEV as a clock/reset bit. Is that
missing from the list above?

 + - nvidia,xusb-mbox: Handle to the Tegra XUSB mailbox node.

As mentioned earlier, I think that's an internal implementation detail.
Shouldn't the two nodes be squashed together?

 +Optional properties:

 + - s1p05v-supply: 1.05V supply regulator.
 + - s1p8v-supply: 1.8V supply regulator.
 + - s3p3v-supply: 3.3V supply regulator.

What are those supplies for? I would have expected any input to the SoC
to have a name that described its purpose, and the pins and DT
properties would be named to match.
--
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 v1 5/9] of: Add NVIDIA Tegra XHCI controller binding

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 Add device-tree binding documentation for the XHCI controller present
 on Tegra124 and later SoCs.

 diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt 
 b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt

 + - clock-names: Must include the following entries:
 +- xusb_host
 +- xusb_falcon_src
 +- xusb_ss
 +- xusb_ss_src
 +- xusb_hs_src
 +- xusb_fs_src

Looking at include/dt-bindings/clock/tegra124-car.h I see a few entries
potentially missing here:

#define TEGRA124_CLK_XUSB_HOST_SRC 252
#define TEGRA124_CLK_XUSB_DEV_SRC 256
#define TEGRA124_CLK_XUSB_DEV 257
#define TEGRA124_CLK_XUSB_SS_DIV2 312

 +- pll_u_480m

Not just pll_u?
--
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 v1 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 The Tegra XHCI controller communicates requests to the host through
 a mailbox interface.  Host drivers which can handle these requests,
 such as the Tegra XUSB pad controller driver and upcoming Tegra XHCI
 host controller driver, can send messages and register to be notified
 of incoming messages.

 diff --git a/include/linux/tegra-xusb-mbox.h b/include/linux/tegra-xusb-mbox.h

 +extern int tegra_xusb_mbox_register_notifier(struct tegra_xusb_mbox *mbox,
 +  struct notifier_block *nb);
 +extern void tegra_xusb_mbox_unregister_notifier(struct tegra_xusb_mbox *mbox,
 + struct notifier_block *nb);
 +extern int tegra_xusb_mbox_send(struct tegra_xusb_mbox *mbox,
 + enum tegra_xusb_mbox_cmd cmd, u32 data);
 +extern struct tegra_xusb_mbox *
 +tegra_xusb_mbox_lookup_by_phandle(struct device_node *np, const char *prop);

This seems to use a custom API. I've seen mention of a mailbox
subsystem, and I assume that has a standardized API. Should this driver
implement that instead?
--
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 v1 4/9] pinctrl: tegra-xusb: Add USB PHY support

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 In addition to the PCIe and SATA PHYs, the XUSB pad controller also
 supports 3 UTMI, 2 HSIC, and 2 USB3 PHYs.  Each USB3 PHY uses a single
 PCIe or SATA lane and is mapped to one of the three UTMI ports.
 

 diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c 
 b/drivers/pinctrl/pinctrl-tegra-xusb.c

 @@ -372,6 +720,193 @@ static int tegra_xusb_padctl_pinconf_group_set(struct 
 pinctrl_dev *pinctrl,
   padctl_writel(padctl, regval, lane-offset);
   break;
  
 + case TEGRA_XUSB_PADCTL_USB3_PORT_NUM:
 + if (value = TEGRA_XUSB_PADCTL_USB3_PORTS) {
 + dev_err(padctl-dev, Invalid USB3 port: %lu\n,
 + value);
 + return -EINVAL;
 + }
 + if (!is_pcie_sata_lane(group)) {
 + dev_err(padctl-dev,
 + USB3 port not applicable for pin %d\n,
 + group);
 + return -EINVAL;
 + }
 + padctl-usb3_ports[value].lane = group;
 + break;

It feels odd to use pinctrl for a SW-only purpose. In other words, that
chunk of code isn't writing the pinconf data to HW, but rather some
internal variable.

Perhaps it would make more sense for the DT binding to represent this
data directly in a custom property that's parsed at probe() time. That
way, pinctrl only touches real HW stuff.

 +static int usb3_phy_power_on(struct phy *phy)
 +{
 + struct tegra_xusb_padctl *padctl = phy_get_drvdata(phy);
 + int port = usb3_phy_to_port(phy);
 + int lane = padctl-usb3_ports[port].lane;
 + u32 value, offset;
 +
 + value = padctl_readl(padctl, XUSB_PADCTL_IOPHY_USB3_PADX_CTL2(port));
 + value = ~((XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_WANDER_MASK 
 + XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_WANDER_SHIFT) |
 +(XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_EQ_MASK 
 + XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_EQ_SHIFT) |
 +(XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_CDR_CNTL_MASK 
 + XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_CDR_CNTL_SHIFT));

Hmm. So there is a lot of PHY stuff here after all.

However, the PHYs implemented here appear to implement very low-level
I/O pad code, whereas the PHYs we have for our USB 2.0 controller are
somewhat higher-level; they're more USB-oriented than just IO pad
oriented. Do you know which level of abstraction a Linux PHY object is
supposed to be? I could never get an answer when I asked before.
--
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 v1 6/9] usb: xhci: Add NVIDIA Tegra XHCI host-controller driver

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 Add support for the on-chip XHCI host controller present on Tegra SoCs.
 
 The driver is currently very basic: it loads the controller with its
 firmware, starts the controller, and is able to service messages sent
 by the controller's firmware.  The hardware supports device mode as
 well as runtime power-gating, but support for these is not yet
 implemented here.
 
 Based on work by:
   Ajay Gupta aj...@nvidia.com
   Bharath Yadav bya...@nvidia.com

 diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig

 +config USB_XHCI_TEGRA
 + tristate NVIDIA Tegra XHCI support
 + depends on ARCH_TEGRA
 + select PINCTRL_TEGRA_XUSB
 + select TEGRA_XUSB_MBOX
 + select FW_LOADER

I think at least some of those should be depends. In particular, the
mbox driver patch said:

+config TEGRA_XUSB_MBOX
+   bool NVIDIA Tegra XUSB mailbox support

which means the option is user-selectable. Either MBOX should be
invisible and selected here, or it should be visible with USB_XHCI_TEGRA
depending on it.

 diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c

 +#define TEGRA_XHCI_UTMI_PHYS 3
 +#define TEGRA_XHCI_HSIC_PHYS 2
 +#define TEGRA_XHCI_USB3_PHYS 2
 +#define TEGRA_XHCI_MAX_PHYS (TEGRA_XHCI_UTMI_PHYS + TEGRA_XHCI_HSIC_PHYS + \
 +  TEGRA_XHCI_USB3_PHYS)

Do those numbers need to be synchronized with the XUSB padctrl driver at
all?

 +static u32 csb_readl(struct tegra_xhci_hcd *tegra, u32 addr)
 +{
 + u32 page, offset;
 +
 + page = CSB_PAGE_SELECT(addr);
 + offset = CSB_PAGE_OFFSET(addr);
 + fpci_writel(tegra, page, XUSB_CFG_ARU_C11_CSBRANGE);
 + return fpci_readl(tegra, XUSB_CFG_CSB_BASE_ADDR + offset);
 +}

I assume some higher level has the required locking or single-threading
so that the keyhole register accesses don't get interleaved?

 +static void tegra_xhci_cfg(struct tegra_xhci_hcd *tegra)
 +{
 + u32 reg;
 +
 + reg = ipfs_readl(tegra, IPFS_XUSB_HOST_CONFIGURATION_0);
 + reg |= IPFS_EN_FPCI;
 + ipfs_writel(tegra, reg, IPFS_XUSB_HOST_CONFIGURATION_0);
 + udelay(10);
 +
 + /* Program Bar0 Space */
 + reg = fpci_readl(tegra, XUSB_CFG_4);
 + reg |= tegra-hcd-rsrc_start;

Don't you need to mask out the original value here? I guess whatever is
being written is probably always the same, but it seems scary to assume
that a bootloader, or previous version of a module during development,
didn't write something unexpected there. Perhaps if the HW module's
reset is pulsed we don't need to worry though.

 +static int tegra_xhci_load_firmware(struct tegra_xhci_hcd *tegra)
 +{
 + struct device *dev = tegra-dev;
 + struct tegra_xhci_fw_cfgtbl *cfg_tbl;
 + u64 fw_base;
 + u32 val;
 + time_t fw_time;
 + struct tm fw_tm;
 +
 + if (csb_readl(tegra, XUSB_CSB_MP_ILOAD_BASE_LO) != 0) {
 + dev_info(dev, Firmware already loaded, Falcon state 0x%x\n,
 +  csb_readl(tegra, XUSB_FALC_CPUCTL));
 + return 0;
 + }
 +
 + cfg_tbl = (struct tegra_xhci_fw_cfgtbl *)tegra-fw_data;

Are there endianness or CPU word size (e.g. ARMv8) issues here; this is
casting the content of a data file to a CPU data structure.

 +static int tegra_xhci_set_ss_clk(struct tegra_xhci_hcd *tegra,
 +  unsigned long rate)

 + switch (rate) {
 + case TEGRA_XHCI_SS_CLK_HIGH_SPEED:
 + /* Reparent to PLLU_480M. Set div first to avoid overclocking */
 + old_parent_rate = clk_get_rate(clk_get_parent(clk));
 + new_parent_rate = clk_get_rate(tegra-pll_u_480m);
 + div = new_parent_rate / rate;
 + ret = clk_set_rate(clk, old_parent_rate / div);
 + if (ret)
 + return ret;
 + ret = clk_set_parent(clk, tegra-pll_u_480m);
 + if (ret)
 + return ret;

Don't you need to call clk_set_rate() again after reparenting, since the
divisor will be different, and the rounding too.

 +static int tegra_xhci_regulator_enable(struct tegra_xhci_hcd *tegra)
 +{
 + int ret;
 +
 + ret = regulator_enable(tegra-s3p3v_reg);
 + if (ret  0)
 + return ret;
 + ret = regulator_enable(tegra-s1p8v_reg);
 + if (ret  0)
 + goto disable_s3p3v;
 + ret = regulator_enable(tegra-s1p05v_reg);
 + if (ret  0)
 + goto disable_s1p8v;

Would regulator_bulk_enable() save any code here? Similar in _disable().

 +static const struct tegra_xhci_soc_config tegra124_soc_config = {
 + .firmware_file = tegra12x/tegra_xusb_firmware,
 +};

I would prefer an nvidia/ prefix so everything gets namespaced by vendor.

tegra12x isn't the name of the chip, but rather Tegra124.

tegra_ and _firmware seem redundant, since they're implied by parent
directories.

So, how about nvidia/tegra124/xusb? (perhaps with .img or .bin file
extension)

 +static int 

Re: [PATCH v1 1/9] of: Add NVIDIA Tegra XUSB mailbox binding

[Stephen Warren]

On 06/25/2014 04:37 PM, Andrew Bresticker wrote:
 On Wed, Jun 25, 2014 at 2:42 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 Add device-tree bindings for the Tegra XUSB mailbox which will be used
 for communication between the Tegra XHCI controller and the host.

 Sorry for the slow review.

 diff --git 
 a/Documentation/devicetree/bindings/mailbox/nvidia,tegra124-xusb-mbox.txt 
 b/Documentation/devicetree/bindings/mailbox/nvidia,tegra124-xusb-mbox.txt

 +NVIDIA Tegra XUSB mailbox
 +=
 +
 +The Tegra XUSB mailbox is used by the Tegra XHCI controller's firmware to
 +communicate with the host.

 Isn't the mailbox an internal implementation detail of the XUSB controller.

 In other words, I'd naively think that there isn't a standalone generic
 mailbox that can be used by anything, but we just happen to want to use
 for XUSB. Rather, there's an XUSB controller, and part of the interface
 to that controller is a mailbox.
 
 Yes, the mailbox isn't an actual piece of hardware but rather the
 interface through which the XUSB host and AP communicate.
 
 As such, I don't think we want a standalone mailbox node in DT. Rather,
 we should add the required reg and interrupt values into the XUSB DT node.

 The driver for that XUSB HW module can either:

 a) Register as both a mailbox driver and an EHCI driver.

 b) Spawn a child device to instantiate the mailbox driver.

 Perhaps (b) could be assisted by using the MFD framework?
 
 So in the RFC series I did something like (a) where the XUSB host
 handled the mailbox interrupt with both the PHY and host could
 registering notifiers to handle the messages.  It was suggested by
 Arnd though that I make a separate mailbox driver.  Instead of having
 a both a host and mailbox node, I could have a single XUSB host node
 and have the mailbox driver bind to that - thoughts?

Yes, that sounds like what I meant by (b) above. I don't think you can
actually have 2 drivers bind to the same DT node though, so it'd have to
work something like:

* XUSB host node causes a platform device to be instantiated
* XUSB host driver probe()s against that
* XUSB host driver's probe() creates a platform device for the mailbox
* XUSB mailbox driver probe()s against that.

Or, perhaps go completely MFD, and have 2 child devices (XUSB host and
XUSB mailbox) instantiated by the MFD parent, which is what is in the DT.
--
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 v1 5/9] of: Add NVIDIA Tegra XHCI controller binding

[Stephen Warren]

On 06/25/2014 05:01 PM, Andrew Bresticker wrote:
 On Wed, Jun 25, 2014 at 2:52 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 Add device-tree binding documentation for the XHCI controller present
 on Tegra124 and later SoCs.

 diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt 
 b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt

 +Required properties:
 +

 + - clock-names: Must include the following entries:
 +- xusb_host
 +- xusb_falcon_src
 +- xusb_ss
 +- xusb_ss_src
 +- xusb_hs_src
 +- xusb_fs_src
 +- pll_u_480m
 +- clk_m
 +- pll_e

 + - reset-names: Must include the following entries:
 +   - xusb_host
 +   - xusb_ss

 Usually the CAR has a reset control for each clock. So, I would expect
 as many entries in reset-names as in clock-names. Even if the SW doesn't
 currently touch all the reset lines, we should make sure the binding
 requires them to be present so that any DT will contain the entries if
 they're ever needed in the future.
 
 The xusb_{falcon,host,hs,fs,ss}_src clocks all share the same reset
 bit (143), so I can add a single entry for those.
 
 In the CAR documentation, I see XUSB_DEV as a clock/reset bit. Is that
 missing from the list above?
 
 This is used when XUSB is in device mode, which the driver does not
 support.  I can add those clocks here though if you want.

That'd be a good idea. That way, the DT doesn't have to change later.

 +Optional properties:

 + - s1p05v-supply: 1.05V supply regulator.
 + - s1p8v-supply: 1.8V supply regulator.
 + - s3p3v-supply: 3.3V supply regulator.

 What are those supplies for? I would have expected any input to the SoC
 to have a name that described its purpose, and the pins and DT
 properties would be named to match.
 
 I *think* this what they are from looking at the schematic, but I'll
 have to ask around:
  - s1p05v: avddio_pex, dvddio_pex, and maybe avdd_pll_erefe
  - s1p8v: avdd_pll_utmip
  - s3p3v: avdd_usb, hvdd_pex, hvdd_pex_pll_e
 Should these be separated out as they are for PCIe?

Yes, I think they should be separated. I wonder if the supplies for PCIe
shouldn't have been added to the XUSB padctrl rather than PCIe node
though? It probably doesn't matter much either way.
--
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 v1 5/9] of: Add NVIDIA Tegra XHCI controller binding

[Stephen Warren]

On 06/25/2014 05:02 PM, Andrew Bresticker wrote:
 On Wed, Jun 25, 2014 at 2:54 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
 Add device-tree binding documentation for the XHCI controller present
 on Tegra124 and later SoCs.

 diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt 
 b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt

 + - clock-names: Must include the following entries:
 +- xusb_host
 +- xusb_falcon_src
 +- xusb_ss
 +- xusb_ss_src
 +- xusb_hs_src
 +- xusb_fs_src

 Looking at include/dt-bindings/clock/tegra124-car.h I see a few entries
 potentially missing here:

 #define TEGRA124_CLK_XUSB_HOST_SRC 252
 #define TEGRA124_CLK_XUSB_DEV_SRC 256
 #define TEGRA124_CLK_XUSB_DEV 257
 #define TEGRA124_CLK_XUSB_SS_DIV2 312
 
 The driver doesn't use them, so I didn't put them in the binding.

I think we should add them in case we need them later. Best to fully
describe the HW rather than the parts of the HW that SW currently uses.

 +- pll_u_480m

 Not just pll_u?
 
 We specifically want pll_u_480M as that's what we use as the parent of
 xusb_ss_src when scaling it to 120Mhz.

OK. I recall text in the TRM implying that SW should just leave PLL_U
alone and not fiddle with the separate output clocks. Still, if we have
a clock ID for each output, and it's the correct parent for the clock,
then it does make sense to use that ID.
--
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: EHCI: tegra: Fix use-after-free in .remove()

[Stephen Warren]

On 06/17/2014 08:17 AM, Tuomas Tynkkynen wrote:
 The tegra_ehci_hcd structure is located in the private space allocated
 by the core USB code so it must not be accessed after the HCD is
 freed.

 diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c

This seems to be a persistent problem. Witness commit ecc8a0cdca18 usb:
host: tegra: fix warning messages in ehci_remove. So, I wonder if ...

 @@ -479,10 +479,11 @@ static int tegra_ehci_remove(struct platform_device 
 *pdev)
  
   usb_phy_shutdown(hcd-phy);
   usb_remove_hcd(hcd);
 - usb_put_hcd(hcd);
  
   clk_disable_unprepare(tegra-clk);
  

It's worth putting a comment here:

/* This must be last, since *tegra is part of *hcd */
 + usb_put_hcd(hcd);
 +
   return 0;
  }

Of course, that could be a separate commit if you want.
--
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: [RFC PATCH 04/10] clk: tegra: Initialize xusb clocks

[Stephen Warren]

On 05/14/2014 06:33 PM, Andrew Bresticker wrote:
 Initialize the XUSB-related clocks with appropriate parents and rates
 for both Tegra114 and Tegra124.

These first 4 clock driver patches look plausible to me, although I
didn't look that hard!

Peter or Mike, if they look OK to you, can you please pick them up for
3.16 so that by the time we apply the rest of the patches in this series
for 3.17, the clock dependencies are already there? Thanks.
--
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: [RFC PATCH 00/10] Tegra XHCI support

[Stephen Warren]

On 05/14/2014 06:32 PM, Andrew Bresticker wrote:
 This is a first pass at the host and PHY drivers necessary for USB3.0
 support on Tegra114 and Tegra124.  The Tegra XHCI host controller requires
 external firmware [1] which must be loaded before using any USB ports owned
 by the controller.  The XUSB PHY driver handles mapping and enabling of
 the UTMI, HSIC, and SuperSpeed pads to the XHCI controller.
 
 Tested on a Venice2 with a variety of USB2.0 and USB3.0 memory sticks
 and ethernet dongles.  
 
 Notes:
  - I've included support for Tegra114, but since I don't have Tegra114-based
hardware, it is completely untested.
  - The PCIe and SATA PHYs also are programmed using the XUSB_PADCTL space
as well.  At least some of the code can be re-used, specifically with
respect to lane programming.  I believe Thierry is working on the PCIe
parts of this.

If I understand the HW correctly, there's a separate pad control HW
block that provides routing/sharing of signals from USB2(?), USB3, SATA,
and PCIe to the pads.

I believe Thierry is working on exposing this block as a pinctrl driver,
or at least something that the other drivers can call into in order to
configure that block. It'd be good if you can co-ordinate with him to
rebase this driver on top of that, rather than (I assume; haven't read
the code yet...) directly manipulating the padctrl registers inside each
of the different drivers. Co-ordinating that could turn out to be
problematic, and presumably if each driver does its own thing, we end up
duplicating defines, code, and DT bindings for configuring the padctrl HW.
--
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: [RFC PATCH 05/10] ARM: tegra: Export function to read USB calibration data

[Stephen Warren]

On 05/14/2014 06:33 PM, Andrew Bresticker wrote:
 Board-specific USB configuration data is stored in FUSE_SKU_CALIB_0.
 Export a function to read it so the PHY can be properly configured.

This patch seems conceptually fine to me. Presumably once Peter's fuse
driver is fleshed out, it can expose the exact same semantic interface
to drivers.

I suppose one could argue that we should create/use an explicit fuse
subsystem with a standard API that drivers can call into, rather than a
custom API. We can get the appropriate handle to use for that API from a
phandle (plus fuse IDs?) in a DT property (in the XHCI device node,
pointing at the fuse device node) much like any other cross-device
resource reference. I'm not too bothered whether the code actually does
that in the first instance, although we should make sure we add that DT
property (containing all required fuse ID information) so that the code
/can/ work that way if we want it to in the future.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/6] usb: host: ehci-tegra: Use devm_ioremap_resource instead of devm_ioremap

[Stephen Warren]

On 05/10/2014 06:00 AM, Vivek Gautam wrote:
 Using devm_ioremap_resource() API should actually be preferred over
 devm_ioremap(), since the former request the mem region first and then
 gives back the ioremap'ed memory pointer.
 devm_ioremap_resource() calls request_mem_region(), therby preventing
 other drivers to make any overlapping call to the same region.

This patch on its own works OK on Tegra. However, if a similar patch
were to be applied to the Tegra PHY driver, then I expect that would
break USB on Tegra. The reason is that the Tegra USB controller and PHY
registers are interleaved a bit randomly within the same address range,
and rather than call out which individual addresses are relevant to the
controller and the PHY, the DT for both devices just specifies the same
whole range, and the drivers only touch the appropriate registers within
the range. Perhaps we should have described that as an MFD rather than
separate DT nodes and devices, but that's not what we ended up with.
--
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 2/4] USB: EHCI: Export the ehci_hub_control function

[Stephen Warren]

On 04/16/2014 10:00 AM, Laurent Pinchart wrote:
 Platform drivers sometimes need to perform specific handling of hub
 control requests. Make this possible by exporting the ehci_hub_control()
 function which can then be called from a custom hub control handler in
 the default case.

The Tegra parts,
Acked-by: Stephen Warren swar...@nvidia.com
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] USB: EHCI: Export the ehci_hub_control function

[Stephen Warren]

On 04/15/2014 10:06 AM, Laurent Pinchart wrote:
 Platform drivers sometimes need to perform specific handling of hub
 control requests. Make this possible by exporting the ehci_hub_control()
 function which can then be called from a custom hub control handler in
 the default case.

I recall trying to do something like this in the past, but IIRC Alan
Stern didn't want to make this easy for odd drivers needing this unusual
case. Witness the comment right above the context of the modified code
in ehci-tegra.c:

 diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c

/*
 * The Tegra HW has some unusual quirks, which require Tegra-specific
 * workarounds. We override certain hc_driver functions here to
 * achieve that. We explicitly do not enhance ehci_driver_overrides to
 * allow this more easily, since this is an unusual case, and we don't
 * want to encourage others to override these functions by making it
 * too easy.
 */

orig_hub_control = tegra_ehci_hc_driver.hub_control;

 @@ -531,8 +527,6 @@ static int __init ehci_tegra_init(void)
* too easy.
*/
  
 - orig_hub_control = tegra_ehci_hc_driver.hub_control;
 -

--
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: EHCI: tegra: set txfill_tuning

[Stephen Warren]

From: Stephen Warren swar...@nvidia.com

To avoid memory fetch underflows with larger USB transfers, Tegra SoCs
need txfill_tuning's txfifothresh register field set to a non-default
value. Add a custom reset override in order to set this up.

These values are recommended practice for all Tegra chips. However,
I've only noticed practical problems when not setting them this way on
systems using Tegra124. Hence, CC: stable only for recent kernels which
actually support Tegra124.

Cc: sta...@vger.kernel.org # 3.14+
Signed-off-by: Stephen Warren swar...@nvidia.com
---
 drivers/usb/host/ehci-tegra.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 27ac6ad53c3d..7ef00ecb0da1 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -509,8 +509,31 @@ static struct platform_driver tegra_ehci_driver = {
}
 };
 
+static int tegra_ehci_reset(struct usb_hcd *hcd)
+{
+   struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+   int retval;
+   int txfifothresh;
+
+   retval = ehci_setup(hcd);
+   if (retval)
+   return retval;
+
+   /*
+* We should really pull this value out of tegra_ehci_soc_config, but
+* to avoid needing access to it, make use of the fact that Tegra20 is
+* the only one so far that needs a value of 10, and Tegra20 is the
+* only one which doesn't set has_hostpc.
+*/
+   txfifothresh = ehci-has_hostpc ? 0x10 : 10;
+   ehci_writel(ehci, txfifothresh  16, ehci-regs-txfill_tuning);
+
+   return 0;
+}
+
 static const struct ehci_driver_overrides tegra_overrides __initconst = {
.extra_priv_size= sizeof(struct tegra_ehci_hcd),
+   .reset  = tegra_ehci_reset,
 };
 
 static int __init ehci_tegra_init(void)
-- 
1.8.1.5

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

Re: [PATCH 1/2] usb: rename 'phy' field of 'struct usb_hcd' to 'transceiver'

[Stephen Warren]

On 04/09/2014 07:57 AM, Sergei Shtylyov wrote:
 Return to the 'phy' field of 'struct usb_hcd' its historic name 
 'transceiver'. 
 This is in preparation to adding the generic PHY support.

Surely if the correct term is transceiver, we should be adding generic
transceiver support not generic PHY support? To be honest, this rename
feels like churn, especially since the APIs and DT bindings all still
include the work phy so now everything will be inconsistent.
--
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/2] usb: rename 'phy' field of 'struct usb_hcd' to 'transceiver'

[Stephen Warren]

On 04/09/2014 10:27 AM, Sergei Shtylyov wrote:
 Hello.
 
 On 04/09/2014 07:31 PM, Stephen Warren wrote:
 
 Return to the 'phy' field of 'struct usb_hcd' its historic name
 'transceiver'.
 This is in preparation to adding the generic PHY support.
 
 Surely if the correct term is transceiver, we should be adding generic
 transceiver support not generic PHY support? To be honest, this rename
 feels like churn, especially since the APIs and DT bindings all still
 include the work phy so now everything will be inconsistent.
 
How about 'usb_phy'?

That certainly would make things more consistent, but I wonder why
usb_phy is better than phy when the code/struct in question is
something USB-specific; the usb_ prefix seems implicit to me due to
context.
--
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/2] usb: rename 'phy' field of 'struct usb_hcd' to 'transceiver'

[Stephen Warren]

On 04/09/2014 10:53 AM, Sergei Shtylyov wrote:
 On 04/09/2014 08:48 PM, Stephen Warren wrote:
 
 Return to the 'phy' field of 'struct usb_hcd' its historic name
 'transceiver'.
 This is in preparation to adding the generic PHY support.
 
 Surely if the correct term is transceiver, we should be adding generic
 transceiver support not generic PHY support? To be honest, this rename
 feels like churn, especially since the APIs and DT bindings all still
 include the work phy so now everything will be inconsistent.
 
 How about 'usb_phy'?
 
 That certainly would make things more consistent, but I wonder why
 usb_phy is better than phy when the code/struct in question is
 something USB-specific; the usb_ prefix seems implicit to me due to
 context.
 
I tend to agree. However, I need to name the new field of stype
 'struct phy *' somehow... perhaps something like 'gen_phy' for it would do?

Ok, the existing field is being replaced by something? I didn't get that
from the patch description; I thought the new name in this patch was
going to be it. In that case, a temporary name of usb_phy for the
existing field, or adding the new field as gen_phy sound reasonable.

--
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/2] usb: rename 'phy' field of 'struct usb_hcd' to 'transceiver'

[Stephen Warren]

On 04/09/2014 12:16 PM, Sergei Shtylyov wrote:
 Hello.
 
 On 04/09/2014 09:56 PM, Alan Stern wrote:
 
 Ok, the existing field is being replaced by something? I didn't get
 that
 
  No, not replaced. I'm adding the support for generic PHY to the
 existing
 USB PHY support. I thought that was clear from the changelog.
 
 from the patch description; I thought the new name in this patch was
 going to be it. In that case, a temporary name of usb_phy for the
 existing field, or adding the new field as gen_phy sound reasonable.
 
  OK, I'll respin the patch #2 with 'gen_phy' and remove the patch
 #1.
 
 What is the reason for all of this?  That is, can you explain the
 difference between USB PHY support and general PHY support, and why we
 need both?
 
The generic PHY framework (drivers/phy/phy-core.c) supports
 multifunction complex PHYs (some functions of which may be related to
 USB). My case is a Renesas R-Car generation 2 PHY that can switch two
 USB ports between different USB controllers (one PCI and one non-PCI on
 each port); I just haven't CCed linux-usb on my driver submission.
 Though there's already drivers/phy/usb/ driver for that hardware, it
 failed to meet the expectations (dynamic setting of the port
 multiplexing depending on what USB host/gadget drivers are loaded), so I
 had to write a new driver. I guess I don't need to describe
 drivers/phy/usb/ framework in detail, do I? It only provides for
 single-function simple USB PHYs...

Naively, it sounds like the complex PHY driver should also be a pinctrl
driver, since it sounds like the main feature it has beyond a simple PHY
is the ability to do pin muxing.

--
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: EHCI: tegra: Drop unused defines

[Stephen Warren]

On 02/25/2014 09:09 AM, Thierry Reding wrote:
 From: Thierry Reding tred...@nvidia.com
 
 Since commit 2d22b42db02f usb: phy: registering Tegra USB PHY as
 platform driver the driver no longer relies on the hard-coded physical
 addresses to determine the association between PHY and EHCI port, so
 these defines can be dropped.

Reviewed-by: Stephen Warren swar...@nvidia.com

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

Re: [PATCH 0/7] usb: dwc2/s3c-hsotg: Combine drivers into a single DRD

[Stephen Warren]

On 02/16/2014 05:37 PM, Jingoo Han wrote:
 On Friday, February 14, 2014 6:44 PM, Robert Baldyga wrote:
 On 02/13/2014 10:10 PM, dingu...@altera.com wrote:
 From: Dinh Nguyen dingu...@altera.com

 Hello,

 This patch series combines the dwc2 host driver and the s3c-hsotg peripheral
 driver into a single dual-roler driver similar to the dwc3.

 Hi.
 It looks like it doesn't work on Samsung platforms without device-tree
 support.
 
 Hi Robert Baldyga,
 
 In order to test this patch, the current mainline kernel requires
 additional patches to support device-tree. The following patches
 are necessary.

DT is supposed to be an ABI (old DTs must work with newer SW), so you
shouldn't/can't change the code in a way that requires changes to the
DT, at least for features that are working and stable and already have
sensible stable DT bindings.
--
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: rpi/dwc2 kernel panics

[Stephen Warren]

On 02/13/2014 11:47 AM, Andre Heider wrote:
 On Wed, Feb 12, 2014 at 08:00:18PM -0700, Stephen Warren wrote:
 On 02/12/2014 11:52 AM, Andre Heider wrote:
 Hi guys,

 I just tried today's Linus' master (45f7fdc2ff) with usb-linus (3635c7e2d5)
 merged on top to give the latest dwc2 fixes another try.
 Unfortunately I'm getting various crashes on system startup. Kernel boots
 fine, dwc2 and the integrated smsc95xx are detected, but somewhere in the
 init sequence the system panics.
...
 Huh, switching compilers made a difference...

Interesting. I almost didn't mention all the compiler/... versions I
use, since I figured they shouldn't make a difference!

...
 Which one are you using?

[swarren@dart u-boot.git]$ echo $CROSS_COMPILE
arm-linux-gnueabi-

[swarren@dart u-boot.git]$ dpkg -l | grep gcc-arm-linux-gnueabi
ii  gcc-arm-linux-gnueabi
4:4.7.2-1 amd64The GNU C
compiler for armel architecture

(I do have the hf version installed too, but use the non-hf version to
build the kernel/U-Boot)

[swarren@dart u-boot.git]$ ${CROSS_COMPILE}gcc --version
arm-linux-gnueabi-gcc (Ubuntu/Linaro 4.7.3-1ubuntu1) 4.7.3
--
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 0/7] usb: dwc2/s3c-hsotg: Combine drivers into a single DRD

[Stephen Warren]

On 02/13/2014 02:10 PM, dingu...@altera.com wrote:
 From: Dinh Nguyen dingu...@altera.com
 
 Hello,
 
 This patch series combines the dwc2 host driver and the s3c-hsotg peripheral
 driver into a single dual-roler driver similar to the dwc3.
 
 The patch series moves the s3c-hsotg files into the /dwc2 folder, so this is
 the final location of the driver. When the driver is built as a kernel module,
 it will be dwc2.ko and dwc2_platform.ko.

This series causes the DWC2 driver to fail to probe on the Raspberry Pi:

 [0.953749] dwc2/s3c-hsotg 2098.usb: cannot get otg clock
 [0.962250] dwc2/s3c-hsotg: probe of 2098.usb failed with error -2

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

Re: rpi/dwc2 kernel panics

[Stephen Warren]

On 02/12/2014 11:52 AM, Andre Heider wrote:
 Hi guys,
 
 I just tried today's Linus' master (45f7fdc2ff) with usb-linus (3635c7e2d5)
 merged on top to give the latest dwc2 fixes another try.
 Unfortunately I'm getting various crashes on system startup. Kernel boots
 fine, dwc2 and the integrated smsc95xx are detected, but somewhere in the
 init sequence the system panics.

I haven't ever seen crashes like that, when running linux-next. I also
merged those two exact commits together and didn't see any issue in 10
boot cycles.

FWIW, I am running:
* A rev2 Model B
* Firmware commit 9c3d7b6 Add latest linaro gcc toolchain:
gcc-linaro-arm-linux-gnueabihf-raspbian-2012.08-20120724_linux
* Upstream U-Boot (very recent) and kernel
* Using Ubuntu 13.10's packaged ARM cross-compiler

--
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: [RFC PATCHv1] usb: dwc2: Combine the dwc2 and s3c_hsotg into a single USB DRD driver.

[Stephen Warren]

On 02/04/2014 02:45 PM, dingu...@altera.com wrote:
 From: Dinh Nguyen dingu...@altera.com
 
 This means that the driver can be in host or peripheral mode when the 
 appropriate
 connector is used. When an A-cable is plugged in, the driver behaves in host
 mode, and when a B-cable is used, the driver will be in peripheral mode.

Sorry for the slow response. When building ARCH=arm bcm2835_defconfig, I
get build errors:

 drivers/built-in.o: In function `dwc2_gadget_init':
 drivers/usb/dwc2/s3c-hsotg.c:3335: undefined reference to `usb_add_gadget_udc'
 drivers/built-in.o: In function `s3c_hsotg_remove':
 drivers/usb/dwc2/s3c-hsotg.c:3358: undefined reference to `usb_del_gadget_udc'
 drivers/usb/dwc2/s3c-hsotg.c:3364: undefined reference to 
 `usb_gadget_unregister_driver'
--
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] Move DWC2 driver out of staging

[Stephen Warren]

On 02/03/2014 01:51 PM, Paul Zimmerman wrote:
...
 Stephen, Andre,
 
 Can you test the attached patch, please? It works for my on the Synopsys
 PCIe-based FPGA board. Unfortunately my RPI board is currently broken,
 so I am unable to test it there to verify it actually fixes the problem
 you are seeing.
 
 The dwc2 driver doesn't use the usb_device toggle bits anywhere else,
 so the quickest fix is to just remove the problematic code from
 _dwc2_hcd_endpoint_reset().
 
 If you give me your tested-bys, I will submit this as a proper patch
 to Greg.

I've tested a basically equivalent patch (link below), so I feel
comfortable saying:

Tested-by: Stephen Warren swar...@wwwdotorg.org

https://github.com/swarren/linux-rpi/commit/f7b9c896153cc0501acecb58053db978ec00a5bf

 @@ -2579,9 +2579,11 @@ static void _dwc2_hcd_endpoint_reset(struct usb_hcd 
 *hcd,
 
   spin_lock_irqsave(hsotg-lock, flags);
 
 +#if 0
   usb_settoggle(udev, epnum, is_out, 0);
   if (is_control)
   usb_settoggle(udev, epnum, !is_out, 0);
 +#endif
   dwc2_hcd_endpoint_reset(hsotg, ep);
 
   spin_unlock_irqrestore(hsotg-lock, flags);

--
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] Move DWC2 driver out of staging

[Stephen Warren]

On 02/01/2014 03:00 AM, Andre Heider wrote:
 On Fri, Jan 31, 2014 at 11:48:37PM -0700, Stephen Warren wrote:
 On 01/31/2014 11:12 AM, Andre Heider wrote:
 On Mon, Jan 13, 2014 at 01:50:09PM -0800, Paul Zimmerman wrote:
 The DWC2 driver should now be in good enough shape to move out of
 staging. I have stress tested it overnight on RPI running mass
 storage and Ethernet transfers in parallel, and for several days
 on our proprietary PCI-based platform.
 ...
 this looks just fine, but for whatever reason it breaks sdhci on my rpi.
 With today's Linus' master the dwc2 controller seems to initialize fine,
 but I get this upon boot:

 [1.783316] sdhci-bcm2835 2030.sdhci: sdhci_pltfm_init failed -12
 [1.794820] sdhci-bcm2835: probe of 2030.sdhci failed with error -12
...
 This is due to the following code:
...
 What ends up happening, simply due to memory allocation order, is that
 the memory writes inside usb_settoggle() end up setting the SDHCI struct
 platform_device's num_resources to 0, so that it's call to
 platform_get_resource() fails.

 With the DWC2 move patch reverted, some other random piece of memory is
 being corrupted, which just happens not to cause any visible problem.
 Likely it's some other struct platform_device that's already had its
 resources read by the time DWC2 probes and corrupts them.

 (Yes, this was hard to find!)
 
 Nice work, but how did you pinpoint this? Am I missing some option/tool
 or did I just not stare for long enough?

Well, there was a clear place where an issue was present; the resource
lookup in sdhci_pltfm_init() was failing, so I put a bunch of printfs
into that function to dump out the data platform_get_resource() used.
This clearly pointed at num_resources==0 being the problem. Next, I
dumped the same data from the code in drivers/of that sets it up, and it
was OK there, so I knew it was getting over-written somewhere. I then
basically added hundreds of calls to the same data dumping function
throughout kernel functions like really_probe() to track down the
location of the problem. Luckily, the behaviour was stable, so I wasn't
chasing a race/timing condition. Eventually I narrowed the window to the
few lines of code I mentioned in _dwc2_hcd_endpoint_reset(). It would
have been much harder if it was e.g. the USB HW DMAing to memory that
caused the corruption, so I was lucky:-)
--
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] Move DWC2 driver out of staging

[Stephen Warren]

On 01/31/2014 12:37 PM, Paul Zimmerman wrote:
 From: Andre Heider [mailto:a.hei...@gmail.com]
 Sent: Friday, January 31, 2014 11:04 AM
 On Fri, Jan 31, 2014 at 12:15:26PM -0600, Felipe Balbi wrote:
 On Fri, Jan 31, 2014 at 07:12:36PM +0100, Andre Heider wrote:
 On Mon, Jan 13, 2014 at 01:50:09PM -0800, Paul Zimmerman wrote:
 The DWC2 driver should now be in good enough shape to move out of
 staging. I have stress tested it overnight on RPI running mass
 storage and Ethernet transfers in parallel, and for several days
 on our proprietary PCI-based platform.
...
 this looks just fine, but for whatever reason it breaks sdhci on my rpi.
 With today's Linus' master the dwc2 controller seems to initialize fine,
 but I get this upon boot:

 [1.783316] sdhci-bcm2835 2030.sdhci: sdhci_pltfm_init failed -12
 [1.794820] sdhci-bcm2835: probe of 2030.sdhci failed with error -12
...
 This silly move doesn't trigger the sdhci ENOMEM:

 diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
 index b021c96..f739b80 100644
 --- a/arch/arm/boot/dts/bcm2835.dtsi
 +++ b/arch/arm/boot/dts/bcm2835.dtsi
 @@ -100,6 +100,12 @@
  status = disabled;
  };

 +usb {
 +compatible = brcm,bcm2835-usb;
 +reg = 0x7e98 0x1;
 +interrupts = 1 9;
 +};
 +
  sdhci: sdhci {
  compatible = brcm,bcm2835-sdhci;
  reg = 0x7e30 0x100;
 @@ -107,12 +113,6 @@
  clocks = clk_mmc;
  status = disabled;
  };
 -
 -usb {
 -compatible = brcm,bcm2835-usb;
 -reg = 0x7e98 0x1;
 -interrupts = 1 9;
 -};
  };

  clocks {

 Maybe there's some kind of race, or something even messing at the .dtb
 at runtime?
 
 Hi Greg, Steve,
 
 Would moving a USB driver from drivers/staging/ to drivers/usb/ perhaps
 cause the initialization order to change?

Yes, certainly.

 If so, maybe that has exposed
 some pre-existing interference between sdhci and dwc2?
 
 Could moving the resource in the .dts file as Andre has done also cause
 the initialization order to change?

Yes, certainly.

I'll try to look into this, this weekend.
--
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] Move DWC2 driver out of staging

[Stephen Warren]

On 01/31/2014 11:12 AM, Andre Heider wrote:
 On Mon, Jan 13, 2014 at 01:50:09PM -0800, Paul Zimmerman wrote:
 The DWC2 driver should now be in good enough shape to move out of
 staging. I have stress tested it overnight on RPI running mass
 storage and Ethernet transfers in parallel, and for several days
 on our proprietary PCI-based platform.
...
 this looks just fine, but for whatever reason it breaks sdhci on my rpi.
 With today's Linus' master the dwc2 controller seems to initialize fine,
 but I get this upon boot:
 
 [1.783316] sdhci-bcm2835 2030.sdhci: sdhci_pltfm_init failed -12
 [1.794820] sdhci-bcm2835: probe of 2030.sdhci failed with error -12

 That is:
 
   struct sdhci_host *sdhci_pltfm_init(struct platform_device 
 *pdev,
   const struct 
 sdhci_pltfm_data *pdata,
   size_t priv_size)
   {
   ...
 
   iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   if (!iomem) {
   ret = -ENOMEM;
   goto err;
   }

This is due to the following code:

static void _dwc2_hcd_endpoint_reset(struct usb_hcd *hcd,
 struct usb_host_endpoint *ep)
{
struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
...
struct usb_device *udev;
...
udev = to_usb_device(hsotg-dev);
...
usb_settoggle(udev, epnum, is_out, 0);
if (is_control)
usb_settoggle(udev, epnum, !is_out, 0);

The problem is that hsotg-dev is assigned as follows:

static int dwc2_driver_probe(struct platform_device *dev)
...
hsotg = devm_kzalloc(dev-dev, sizeof(*hsotg), GFP_KERNEL);
...
hsotg-dev = dev-dev;

As such, it's not legal to call to_usb_device() on it.

What ends up happening, simply due to memory allocation order, is that
the memory writes inside usb_settoggle() end up setting the SDHCI struct
platform_device's num_resources to 0, so that it's call to
platform_get_resource() fails.

With the DWC2 move patch reverted, some other random piece of memory is
being corrupted, which just happens not to cause any visible problem.
Likely it's some other struct platform_device that's already had its
resources read by the time DWC2 probes and corrupts them.

(Yes, this was hard to find!)

I honestly can't see how to solve this myself, since the whole DWC2
driver doesn't seem to have a struct usb_device * hanging around that we
can stash somewhere for it to look up later. Perhaps someone more
familiar with the USB stack can help with that.
--
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] staging: dwc2: don't issue traffic to LS devices in FS mode

[Stephen Warren]

On 12/06/2013 03:01 PM, Paul Zimmerman wrote:
 From: Nick Hudson sk...@netbsd.org
 
 I fell over the problem reported in
 https://github.com/raspberrypi/linux/pull/390:
 
 Issuing low-speed packets when the root port is in full-speed mode
  causes the root port to stop responding. Explicitly fail when
  enqueuing URBs to a LS endpoint on a FS bus.
 
 with my dwc2 testing in NetBSD, so I adapted the change to dwc2.

Tested-by: Stephen Warren swar...@wwwdotorg.org

The built-in USB Ethernet on my model B still works fine with this
change. A USB keyboard/mouse combo isn't reliable (it keeps dropping
out, and I need to unplug/replug to get it working again), but it never
was before. I'm also not sure if that's because of power issues or not
(I'm not using powered hub on this system).
--
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: phy-tegra-usb.c: wrong pointer check for remap UTMI

[Stephen Warren]

On 12/03/2013 07:02 PM, Chris Ruehl wrote:
 usb: phy-tegra-usb.c: wrong pointer check for remap UTMI
 
 A wrong pointer was used to test the result of devm_ioremap()
 
 Signed-off-by: Chris Ruehl chris.ru...@gtsys.com.hk
 Acked-by: Venu Byravarasu vbyravar...@nvidia.com

Out of curiosity, when did that ack happen? I didn't see it. But anyway,

Acked-by: Stephen Warren swar...@nvidia.com

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

[PATCH 27/31] USB: EHCI: tegra: use reset framework

[Stephen Warren]

From: Stephen Warren swar...@nvidia.com

Tegra's clock driver now provides an implementation of the common
reset API (include/linux/reset.h). Use this instead of the old Tegra-
specific API; that will soon be removed.

Cc: tred...@nvidia.com
Cc: pdeschrij...@nvidia.com
Cc: linux-te...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: Alan Stern st...@rowland.harvard.edu
Cc: linux-usb@vger.kernel.org
Signed-off-by: Stephen Warren swar...@nvidia.com
---
This patch is part of a series with strong internal depdendencies. I'm
looking for an ack so that I can take the entire series through the Tegra
and arm-soc trees. The series will be part of a stable branch that can be
merged into other subsystems if needed to avoid/resolve dependencies.
---
 drivers/usb/host/ehci-tegra.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index b9fd0396011e..6f7e23dd1417 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -17,7 +17,6 @@
  */
 
 #include linux/clk.h
-#include linux/clk/tegra.h
 #include linux/dma-mapping.h
 #include linux/err.h
 #include linux/gpio.h
@@ -29,6 +28,7 @@
 #include linux/of_gpio.h
 #include linux/platform_device.h
 #include linux/pm_runtime.h
+#include linux/reset.h
 #include linux/slab.h
 #include linux/usb/ehci_def.h
 #include linux/usb/tegra_usb_phy.h
@@ -62,6 +62,7 @@ static int (*orig_hub_control)(struct usb_hcd *hcd,
 struct tegra_ehci_hcd {
struct tegra_usb_phy *phy;
struct clk *clk;
+   struct reset_control *rst;
int port_resuming;
bool needs_double_reset;
enum tegra_usb_phy_port_speed port_speed;
@@ -385,13 +386,20 @@ static int tegra_ehci_probe(struct platform_device *pdev)
goto cleanup_hcd_create;
}
 
+   tegra-rst = devm_reset_control_get(pdev-dev, usb);
+   if (IS_ERR(tegra-rst)) {
+   dev_err(pdev-dev, Can't get ehci reset\n);
+   err = PTR_ERR(tegra-rst);
+   goto cleanup_hcd_create;
+   }
+
err = clk_prepare_enable(tegra-clk);
if (err)
goto cleanup_hcd_create;
 
-   tegra_periph_reset_assert(tegra-clk);
+   reset_control_assert(tegra-rst);
udelay(1);
-   tegra_periph_reset_deassert(tegra-clk);
+   reset_control_deassert(tegra-rst);
 
u_phy = devm_usb_get_phy_by_phandle(pdev-dev, nvidia,phy, 0);
if (IS_ERR(u_phy)) {
-- 
1.8.1.5

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

Re: [PATCH] USB: host: tegra: CONFIG_USB_EHCI_TEGRA requires ULPI and ULPI viewport support

[Stephen Warren]

On 10/04/2013 12:02 AM, Paul Walmsley wrote:
 
 Selecting CONFIG_USB_EHCI_TEGRA requires CONFIG_USB_ULPI_VIEWPORT.
 Otherwise the build can break with:
 
 drivers/usb/phy/phy-tegra-usb.c: In function 'ulpi_open':
 drivers/usb/phy/phy-tegra-usb.c:689:31: error:
 'ulpi_viewport_access_ops' undeclared (first use in this function)
 drivers/usb/phy/phy-tegra-usb.c:689:31: note: each undeclared identifier
 is reported only once for each function it appears in
 
 if CONFIG_USB_ULPI_VIEWPORT is not manually selected.
 
 Fix by forcing CONFIG_USB_ULPI_VIEWPORT to be selected when
 CONFIG_USB_EHCI_TEGRA is selected.  Then, since CONFIG_USB_ULPI_VIEWPORT
 requires CONFIG_USB_ULPI to be selected, add that too.
 
 N.B.: ULPI is deprecated on this controller for T114, so it might make
 sense to split the ULPI support code into a separate file, compiled only
 if a ULPI PHY is selected.

This was fixed at least in 3.13 (perhaps 3.12 too?) by:

config ARCH_TEGRA
...
select USB_ARCH_HAS_EHCI if USB_SUPPORT
select USB_ULPI if USB_PHY
select USB_ULPI_VIEWPORT if USB_PHY

I think it'd be better to back-port that patch to stable, for consistency.
--
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