diff mbox

[v10,1/9] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding

Message ID 1457108379-20794-1-git-send-email-thierry.reding@gmail.com
State Accepted, archived
Headers show

Commit Message

Thierry Reding March 4, 2016, 4:19 p.m. UTC
From: Thierry Reding <treding@nvidia.com>

The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a
set of lanes that are used for PCIe, SATA and USB.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v10:
- clarify that the hardware documentation means something different when
  referring to a "port" (intra-SoC connectivity)

Changes in v9:
- rename UTMI -> USB2 to match hardware documentation
- reword according to suggestions by Stephen Warren
- make Tegra132 compatible string list consistent
- remove mailbox support

 .../bindings/phy/nvidia,tegra124-xusb-padctl.txt   | 376 +++++++++++++++++++++
 .../pinctrl/nvidia,tegra124-xusb-padctl.txt        |   5 +
 2 files changed, 381 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt

Comments

Andrew Bresticker March 4, 2016, 9:36 p.m. UTC | #1
Hi Thierry,

On Fri, Mar 4, 2016 at 8:19 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a
> set of lanes that are used for PCIe, SATA and USB.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Thanks, this binding looks much better, IMO.  A couple small comments below...

> +Port nodes:
> +===========
> +
> +A required child node named "ports" contains a list of all the ports exposed
> +by the XUSB pad controller. Per-port configuration is only required for USB.
> +
> +USB2 ports:
> +-----------
> +
> +Required properties:
> +- status: Defines the operation status of the port. Valid values are:
> +  - "disabled": the port is disabled
> +  - "okay": the port is enabled
> +- mode: A string that determines the mode in which to run the port. Valid
> +  values are:
> +  - "host": for USB host mode
> +  - "device": for USB device mode
> +  - "otg": for USB OTG mode
> +
> +Optional properties:
> +- nvidia,internal: A boolean property whose presence determines that a port
> +  is internal. In the absence of this property the port is considered to be
> +  external.
> +- vbus-supply: phandle to a regulator supplying the VBUS voltage.

Both Blaze and Smaug require an offset to be applied to the fused
HS_CURR_LEVEL value, so I think we need another property here for
that.

> +ULPI ports:
> +-----------
> +
> +Optional properties:
> +- status: Defines the operation status of the port. Valid values are:
> +  - "disabled": the port is disabled
> +  - "okay": the port is enabled
> +- nvidia,internal: A boolean property whose presence determines that a port
> +  is internal. In the absence of this property the port is considered to be
> +  external.
> +- vbus-supply: phandle to a regulator supplying the VBUS voltage.
> +
> +HSIC ports:
> +-----------
> +
> +Required properties:
> +- status: Defines the operation status of the port. Valid values are:
> +  - "disabled": the port is disabled
> +  - "okay": the port is enabled
> +
> +Optional properties:
> +- vbus-supply: phandle to a regulator supplying the VBUS voltage.

I believe this pin is named VDDIO_HSIC?

Also there are several other HSIC pad parameters (STROBE_TRIM,
DATA_TRIM, etc.) which probably should be supplied via DT.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Bresticker March 4, 2016, 9:47 p.m. UTC | #2
> +Required properties:
> +--------------------
> +- compatible: Must be:
> +  - Tegra124: "nvidia,tegra124-xusb-padctl"
> +  - Tegra132: "nvidia,tegra132-xusb-padctl", "nvidia,tegra124-xusb-padctl"
> +- reg: Physical base address and length of the controller's registers.
> +- resets: Must contain an entry for each entry in reset-names.
> +- reset-names: Must include the following entries:
> +  - "padctl"

Also... there's a padctl interrupt that'll be necessary for LP0
suspend/resume and runtime power-gating of the xHC.  We should
probably include that here too.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring March 5, 2016, 4:31 a.m. UTC | #3
On Fri, Mar 04, 2016 at 05:19:31PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a
> set of lanes that are used for PCIe, SATA and USB.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v10:
> - clarify that the hardware documentation means something different when
>   referring to a "port" (intra-SoC connectivity)
> 
> Changes in v9:
> - rename UTMI -> USB2 to match hardware documentation
> - reword according to suggestions by Stephen Warren
> - make Tegra132 compatible string list consistent
> - remove mailbox support
> 
>  .../bindings/phy/nvidia,tegra124-xusb-padctl.txt   | 376 +++++++++++++++++++++
>  .../pinctrl/nvidia,tegra124-xusb-padctl.txt        |   5 +
>  2 files changed, 381 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt

Without really understanding the h/w here, looks okay to me.

Acked-by: Rob Herring <robh@kernel.org>

> +SoC include:
> +
> +	padctl@0,7009f000 {

Drop the comma. Commas should only be used if there are distinct fields.

If I get my dtc patch done, these are going to start warning, so you 
might want to go fix dts files (assuming that's where this is coming 
from).

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding March 7, 2016, 11:24 a.m. UTC | #4
On Fri, Mar 04, 2016 at 10:31:45PM -0600, Rob Herring wrote:
> On Fri, Mar 04, 2016 at 05:19:31PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a
> > set of lanes that are used for PCIe, SATA and USB.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v10:
> > - clarify that the hardware documentation means something different when
> >   referring to a "port" (intra-SoC connectivity)
> > 
> > Changes in v9:
> > - rename UTMI -> USB2 to match hardware documentation
> > - reword according to suggestions by Stephen Warren
> > - make Tegra132 compatible string list consistent
> > - remove mailbox support
> > 
> >  .../bindings/phy/nvidia,tegra124-xusb-padctl.txt   | 376 +++++++++++++++++++++
> >  .../pinctrl/nvidia,tegra124-xusb-padctl.txt        |   5 +
> >  2 files changed, 381 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> 
> Without really understanding the h/w here, looks okay to me.
> 
> Acked-by: Rob Herring <robh@kernel.org>
> 
> > +SoC include:
> > +
> > +	padctl@0,7009f000 {
> 
> Drop the comma. Commas should only be used if there are distinct fields.
> 
> If I get my dtc patch done, these are going to start warning, so you 
> might want to go fix dts files (assuming that's where this is coming 
> from).

I noticed that in today's next the updated DTC already complains about a
lot of things in existing DTS files. For Tegra that's primarily the
memory node, because it has a reg property but no unit name. Any hints
as to how to solve that? I think I remember from way back that memory
was supposed to be an exception, perhaps DTC needs to be taught that?

Thierry
Rob Herring March 16, 2016, 6:42 a.m. UTC | #5
On Mon, Mar 07, 2016 at 12:24:18PM +0100, Thierry Reding wrote:
> On Fri, Mar 04, 2016 at 10:31:45PM -0600, Rob Herring wrote:
> > On Fri, Mar 04, 2016 at 05:19:31PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a
> > > set of lanes that are used for PCIe, SATA and USB.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > > Changes in v10:
> > > - clarify that the hardware documentation means something different when
> > >   referring to a "port" (intra-SoC connectivity)
> > > 
> > > Changes in v9:
> > > - rename UTMI -> USB2 to match hardware documentation
> > > - reword according to suggestions by Stephen Warren
> > > - make Tegra132 compatible string list consistent
> > > - remove mailbox support
> > > 
> > >  .../bindings/phy/nvidia,tegra124-xusb-padctl.txt   | 376 +++++++++++++++++++++
> > >  .../pinctrl/nvidia,tegra124-xusb-padctl.txt        |   5 +
> > >  2 files changed, 381 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> > 
> > Without really understanding the h/w here, looks okay to me.
> > 
> > Acked-by: Rob Herring <robh@kernel.org>
> > 
> > > +SoC include:
> > > +
> > > +	padctl@0,7009f000 {
> > 
> > Drop the comma. Commas should only be used if there are distinct fields.
> > 
> > If I get my dtc patch done, these are going to start warning, so you 
> > might want to go fix dts files (assuming that's where this is coming 
> > from).
> 
> I noticed that in today's next the updated DTC already complains about a
> lot of things in existing DTS files. For Tegra that's primarily the
> memory node, because it has a reg property but no unit name. Any hints
> as to how to solve that? I think I remember from way back that memory
> was supposed to be an exception, perhaps DTC needs to be taught that?

I remember something about the skeleton.dtsi files and needing to avoid 
having both memory and memory@x nodes, but we really should fix them and 
have a unit address.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren March 16, 2016, 5:39 p.m. UTC | #6
On 03/04/2016 09:19 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a
> set of lanes that are used for PCIe, SATA and USB.

>   .../bindings/phy/nvidia,tegra124-xusb-padctl.txt   | 376 +++++++++++++++++++++
>   .../pinctrl/nvidia,tegra124-xusb-padctl.txt        |   5 +

It seems odd to add part of the deprecation notice in this patch and one 
more line in the second/next patch. Did an update get squashed into the 
wrong commit? I'd suggest moving the edit to existing file 
nvidia,tegra124-xusb-padctl.txt entirely into patch 2. Perhaps this 
could happen while/when the patch is applied to avoid having to post 
another version.

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

...
> +Pads will be represented as children of the top-level XUSB pad controller
 > +device tree node.

Nit: grand-children, since they're house inside pads{} first. I might 
suggest:

A "pads" node exists to represent all pads contained within the XUSB 
controller. Each pad is represented as a subnode of the pads node.

> Each lane exposed by the pad will be represented by its
> +own subnode and can be referenced by users of the lane using the standard
> +PHY bindings, as described by the phy-bindings.txt file in this directory.

"Each lane exposed by the pad will be represented as a subnode of the 
pad node ..."?

I'd suggest adding a similar paragraph describing the ports node, and 
that ports are child nodes of that. Otherwise, since the documentation 
of the nodes isn't nested in any way, it's not clear from the text 
exactly what nodes are children of what other nodes.

> +The Tegra hardware documentation refers to the connection between the XUSB
> +pad controller and the XUSB controller as "ports".

I think, being pedantic, that "port" in the TRM refers to the set of 
signals at the edge/interface-to the IO controller, not the connection 
between the IO controller and the XUSB controller. Still, the existing 
wording in this patch is fine; no need to change it.

Still, the examples do clear this up, so perhaps it's not worth another 
version of the series to fix this. Or if you do think it's worth fixing, 
I'd be perfectly happy to see that done in follow-on patches. If you 
want I can write that follow-on patch once this series is applied.

...
> +PHY nodes:
> +==========
> +
> +Each pad node has one or more children, each representing one of the lanes
> +controlled by the pad.
> +
> +Required properties:
> +--------------------
> +- status: Defines the operation status of the PHY. Valid values are:
> +  - "disabled": the PHY is disabled
> +  - "okay": the PHY is enabled

Presumably the standard semantics of a missing status property 
implicitly meaning "okay" are also intended? A similar comment applies 
to other places where status is documented. "status" is typically not a 
required property.

> +Port nodes:
> +===========

> +USB2 ports:
> +-----------

Should that say "UTMI ports"? ULPI and HSIC below are (or can be) USB2 
ports too.

> +Required properties:
> +- status: Defines the operation status of the port. Valid values are:
> +  - "disabled": the port is disabled
> +  - "okay": the port is enabled
> +- mode: A string that determines the mode in which to run the port. Valid
> +  values are:
> +  - "host": for USB host mode
> +  - "device": for USB device mode
> +  - "otg": for USB OTG mode

How do these properties tie the DT-based port definition to a particular 
PHY/lane/... in HW? I don't see a property that contains any kind of HW 
ID here.

...
> +Optional properties:
> +- nvidia,internal: A boolean property whose presence determines that a port
> +  is internal. In the absence of this property the port is considered to be
> +  external.

It's not clear what "internal" and "external" mean. Presumably it's 
on-PCB vs. physical-connector-exposed-to-the-user. It may be worth 
explicitly mentioning that.

Is there no vbus-supply for USB2/UTMI ports? I'm also not sure why 
vbus-supply is optional for ULPI and HSIC. Even if there is no SW 
/control/ over VBUS, there still must be some source of power; IIRC Mark 
Brown typically desires that to be explicitly modelled with an always-on 
regulator if there's no SW control.

> +Super-speed USB ports:
> +----------------------
> +
> +Required properties:
> +- status: Defines the operation status of the port. Valid values are:
> +  - "disabled": the port is disabled
> +  - "okay": the port is enabled
> +- nvidia,usb2-companion: A single cell that specifies the physical port number
> +  to map this super-speed USB port to. The range of valid port numbers varies
> +  with the SoC generation:
> +  - 0-2: for Tegra124 and Tegra132

How can this be used 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 <swarren@nvidia.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij March 22, 2016, 11:01 a.m. UTC | #7
On Fri, Mar 4, 2016 at 5:19 PM, Thierry Reding <thierry.reding@gmail.com> wrote:

> From: Thierry Reding <treding@nvidia.com>
>
> The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a
> set of lanes that are used for PCIe, SATA and USB.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v10:
> - clarify that the hardware documentation means something different when
>   referring to a "port" (intra-SoC connectivity)

Thierry I'm a bit out of sync, so can you resend these patches with
collected ACKs after -rc1?

Please send me the patches I can just merge into the pinctrl tree
separately if possible, I encourage any DTS changes to go in
orthogonally through ARM SoC. The DTS business I regard as
kind of its own tree.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcel Ziswiler March 29, 2016, 3:24 p.m. UTC | #8
Hi Thierry

On Fri, 2016-03-04 at 17:19 +0100, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
> >
> 
> The NVIDIA Tegra XUSB pad controller provides a set of pads, each
> with a
> set of lanes that are used for PCIe, SATA and USB.

I finally got around trying this both on Jetson TK1 as well as our own
Toradex Apalis TK1 module we are about to mainline. I actually applied
your patch set on top of 4.6.0-rc1. While USB 3.0 seems to work fine I
noticed PCIe and SATA no more to come up right with the following
message:

[    2.794458] tegra-pcie 1003000.pcie-controller: PLL failed to lock:
-110
[    2.801177] tegra-pcie 1003000.pcie-controller: failed to power on
PHY: -110
[    2.809031] tegra-pcie: probe of 1003000.pcie-controller failed with
error -110

Do you happen to know what could be the issue?

As for USB I do get some message about the endpoint companion but do
not know whether or not this is to be expected:

[ 1021.575301] usb 4-1: new SuperSpeed USB device number 2 using tegra-
xusb
[ 1021.598913] usb 4-1: No SuperSpeed endpoint companion for config
1  interface 0 altsetting 0 ep 2: using minimum values

Otherwise it looks good:

ubuntu@tegra-ubuntu:~$ lsusb
Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 004 Device 002: ID 0951:1666 Kingston Technology DataTraveler G4
Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

And performs satisfactorily (up from around 24 MB/sec with just USB
2.0):

ubuntu@tegra-ubuntu:~$ sudo hdparm -t /dev/sda

/dev/sda:
 Timing buffered disk reads:  94 MB in  3.05 seconds =  30.81 MB/sec

Apalis TK1 actually features two USB 3.0 host ports:

ubuntu@tegra-ubuntu:~$ lsusb
Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 005 Device 002: ID 0951:1666 Kingston Technology DataTraveler G4
Bus 005 Device 003: ID 1f75:0902 Innostor Technology Corporation IS902
UFD controller
Bus 005 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
Bus 004 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

ubuntu@tegra-ubuntu:~$ sudo hdparm -t /dev/sda

/dev/sda:
 Timing buffered disk reads:  96 MB in  3.00 seconds =  31.98 MB/sec

ubuntu@tegra-ubuntu:~$ sudo hdparm -t /dev/sdb

/dev/sdb:
 Timing buffered disk reads: 152 MB in  3.04 seconds =  49.99 MB/sec

Cheers

Marcel

> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.
> gmane.org>
> ---
> Changes in v10:
> - clarify that the hardware documentation means something different
> when
>   referring to a "port" (intra-SoC connectivity)
> 
> Changes in v9:
> - rename UTMI -> USB2 to match hardware documentation
> - reword according to suggestions by Stephen Warren
> - make Tegra132 compatible string list consistent
> - remove mailbox support
> ...
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding March 29, 2016, 5:08 p.m. UTC | #9
On Tue, Mar 29, 2016 at 05:24:59PM +0200, Marcel Ziswiler wrote:
> Hi Thierry
> 
> On Fri, 2016-03-04 at 17:19 +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
> > >
> > 
> > The NVIDIA Tegra XUSB pad controller provides a set of pads, each
> > with a
> > set of lanes that are used for PCIe, SATA and USB.
> 
> I finally got around trying this both on Jetson TK1 as well as our own
> Toradex Apalis TK1 module we are about to mainline.

Looking forward to it.

>                                                     I actually applied
> your patch set on top of 4.6.0-rc1. While USB 3.0 seems to work fine I
> noticed PCIe and SATA no more to come up right with the following
> message:
> 
> [    2.794458] tegra-pcie 1003000.pcie-controller: PLL failed to lock:
> -110
> [    2.801177] tegra-pcie 1003000.pcie-controller: failed to power on
> PHY: -110
> [    2.809031] tegra-pcie: probe of 1003000.pcie-controller failed with
> error -110
> 
> Do you happen to know what could be the issue?

That's to be expected. You'll need this one:

	http://patchwork.ozlabs.org/patch/596548/

which I had hoped would make v4.6-rc1, but didn't. I'll have to respin
and send out again. I didn't know that SATA failed in the same way, but
I'll need to recheck and see if it needs a similar change.

> As for USB I do get some message about the endpoint companion but do
> not know whether or not this is to be expected:
> 
> [ 1021.575301] usb 4-1: new SuperSpeed USB device number 2 using tegra-
> xusb
> [ 1021.598913] usb 4-1: No SuperSpeed endpoint companion for config
> 1  interface 0 altsetting 0 ep 2: using minimum values

I'm not exactly sure why that message appears, but I think it is
harmless.

> Otherwise it looks good:
> 
> ubuntu@tegra-ubuntu:~$ lsusb
> Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 004 Device 002: ID 0951:1666 Kingston Technology DataTraveler G4
> Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> 
> And performs satisfactorily (up from around 24 MB/sec with just USB
> 2.0):
> 
> ubuntu@tegra-ubuntu:~$ sudo hdparm -t /dev/sda
> 
> /dev/sda:
>  Timing buffered disk reads:  94 MB in  3.05 seconds =  30.81 MB/sec
> 
> Apalis TK1 actually features two USB 3.0 host ports:
> 
> ubuntu@tegra-ubuntu:~$ lsusb
> Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 005 Device 002: ID 0951:1666 Kingston Technology DataTraveler G4
> Bus 005 Device 003: ID 1f75:0902 Innostor Technology Corporation IS902
> UFD controller
> Bus 005 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> Bus 004 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> 
> ubuntu@tegra-ubuntu:~$ sudo hdparm -t /dev/sda
> 
> /dev/sda:
>  Timing buffered disk reads:  96 MB in  3.00 seconds =  31.98 MB/sec
> 
> ubuntu@tegra-ubuntu:~$ sudo hdparm -t /dev/sdb
> 
> /dev/sdb:
>  Timing buffered disk reads: 152 MB in  3.04 seconds =  49.99 MB/sec

Looking great. Thanks for testing.

Thierry
Marcel Ziswiler March 30, 2016, 10:44 a.m. UTC | #10
On Tue, 2016-03-29 at 19:08 +0200, Thierry Reding wrote:
> On Tue, Mar 29, 2016 at 05:24:59PM +0200, Marcel Ziswiler wrote:
> > 
> > Hi Thierry
> > 
> > On Fri, 2016-03-04 at 17:19 +0100, Thierry Reding wrote:
> > > 
> > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane
> > > .org
> > > > 
> > > > 
> > > The NVIDIA Tegra XUSB pad controller provides a set of pads, each
> > > with a
> > > set of lanes that are used for PCIe, SATA and USB.
> > I finally got around trying this both on Jetson TK1 as well as our
> > own
> > Toradex Apalis TK1 module we are about to mainline.
> Looking forward to it.

Yes, I'm just running some final tests.

> > 
> >                                                     I actually
> > applied
> > your patch set on top of 4.6.0-rc1. While USB 3.0 seems to work
> > fine I
> > noticed PCIe and SATA no more to come up right with the following
> > message:
> > 
> > [    2.794458] tegra-pcie 1003000.pcie-controller: PLL failed to
> > lock:
> > -110
> > [    2.801177] tegra-pcie 1003000.pcie-controller: failed to power
> > on
> > PHY: -110
> > [    2.809031] tegra-pcie: probe of 1003000.pcie-controller failed
> > with
> > error -110
> > 
> > Do you happen to know what could be the issue?
> That's to be expected. You'll need this one:
> 
> 	http://patchwork.ozlabs.org/patch/596548/
> 
> which I had hoped would make v4.6-rc1, but didn't. I'll have to
> respin
> and send out again. I didn't know that SATA failed in the same way,
> but
> I'll need to recheck and see if it needs a similar change.

Ah, I missed that one which you already sent out a v3 still applying
perfectly:

http://article.gmane.org/gmane.linux.ports.tegra/25396

With that it looks much better again:

ubuntu@tegra-ubuntu:~$ lspci
00:02.0 PCI bridge: NVIDIA Corporation TegraK1 PCIe x1 Bridge (rev a1)
01:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 0c)

I also found a more capable test device:

ubuntu@tegra-ubuntu:~$ sudo hdparm -t /dev/sda

/dev/sda:
 Timing buffered disk reads: 576 MB in  3.01 seconds = 191.55 MB/sec

However on Apalis TK1 this broke the second USB 3.0 host port which now
refuses to work. Still works plugging in USB 2.0 gear though.

Hm, it works if plugged in during boot. Strange. What could be the
issue if hot-plugging USB 3.0 devices on the second port does not quite
work?

I'm also wondering whether the Tegra EHCI stuff is still required as
there are now quite many USB buses showing up:

ubuntu@tegra-ubuntu:~$ lsusb
Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 005 Device 002: ID 174c:07d1 ASMedia Technology Inc.
Bus 005 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
Bus 004 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

I also noticed xhci to register lots of ports:

[    3.603688] tegra-xusb 70090000.usb: Firmware timestamp: 2014-09-16
02:10:07 UTC
[    3.611109] tegra-xusb 70090000.usb: xHCI Host Controller
[    3.616560] tegra-xusb 70090000.usb: new USB bus registered,
assigned bus number 4
[    3.626052] tegra-xusb 70090000.usb: hcc params 0x0184f525 hci
version 0x100 quirks 0x00010010
[    3.634738] tegra-xusb 70090000.usb: irq 346, io mem 0x70090000
[    3.641572] hub 4-0:1.0: USB hub found
[    3.645404] hub 4-0:1.0: 6 ports detected

And:

[    3.650144] tegra-xusb 70090000.usb: xHCI Host Controller
[    3.655599] tegra-xusb 70090000.usb: new USB bus registered,
assigned bus number 5
[    3.663267] usb usb5: We don't know the algorithms for LPM for this
host, disabling LPM.
[    3.696888] usb usb5: No SuperSpeed endpoint companion for config
1  interface 0 altsetting 0 ep 129: using minimum values
[    3.715028] hub 5-0:1.0: USB hub found
[    3.718819] hub 5-0:1.0: 2 ports detected

> > As for USB I do get some message about the endpoint companion but
> > do
> > not know whether or not this is to be expected:
> > 
> > [ 1021.575301] usb 4-1: new SuperSpeed USB device number 2 using
> > tegra-
> > xusb
> > [ 1021.598913] usb 4-1: No SuperSpeed endpoint companion for config
> > 1  interface 0 altsetting 0 ep 2: using minimum values
> I'm not exactly sure why that message appears, but I think it is
> harmless.

OK. Yes, at least it does not seem to have any adverse effects.

> > Otherwise it looks good:
> > 
> > ubuntu@tegra-ubuntu:~$ lsusb
> > Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > Bus 004 Device 002: ID 0951:1666 Kingston Technology DataTraveler
> > G4
> > Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> > Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > 
> > And performs satisfactorily (up from around 24 MB/sec with just USB
> > 2.0):
> > 
> > ubuntu@tegra-ubuntu:~$ sudo hdparm -t /dev/sda
> > 
> > /dev/sda:
> >  Timing buffered disk reads:  94 MB in  3.05 seconds =  30.81
> > MB/sec
> > 
> > Apalis TK1 actually features two USB 3.0 host ports:
> > 
> > ubuntu@tegra-ubuntu:~$ lsusb
> > Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > Bus 005 Device 002: ID 0951:1666 Kingston Technology DataTraveler
> > G4
> > Bus 005 Device 003: ID 1f75:0902 Innostor Technology Corporation
> > IS902
> > UFD controller
> > Bus 005 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> > Bus 004 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > 
> > ubuntu@tegra-ubuntu:~$ sudo hdparm -t /dev/sda
> > 
> > /dev/sda:
> >  Timing buffered disk reads:  96 MB in  3.00 seconds =  31.98
> > MB/sec
> > 
> > ubuntu@tegra-ubuntu:~$ sudo hdparm -t /dev/sdb
> > 
> > /dev/sdb:
> >  Timing buffered disk reads: 152 MB in  3.04 seconds =  49.99
> > MB/sec
> Looking great. Thanks for testing.

Thank you.

BTW: Does any of you make it to GTC this year? I will attend and
Toradex is showcasing its latest Apalis TK1 module.

> Thierry


Cheers

Marcel

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

Patch

diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
new file mode 100644
index 000000000000..8b642d9e3433
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
@@ -0,0 +1,376 @@ 
+Device tree binding for NVIDIA Tegra XUSB pad controller
+========================================================
+
+The Tegra XUSB pad controller manages a set of I/O lanes (with differential
+signals) which connect directly to pins/pads on the SoC package. Each lane
+is controlled by a HW block referred to as a "pad" in the Tegra hardware
+documentation. Each such "pad" may control either one or multiple lanes,
+and thus contains any logic common to all its lanes. Each lane can be
+separately configured and powered up.
+
+Some of the lanes are high-speed lanes, which can be used for PCIe, SATA or
+super-speed USB. Other lanes are for various types of low-speed, full-speed
+or high-speed USB (such as UTMI, ULPI and HSIC). The XUSB pad controller
+contains a software-configurable mux that sits between the I/O controller
+ports (e.g. PCIe) and the lanes.
+
+In addition to per-lane configuration, USB 3.0 ports may require additional
+settings on a per-board basis.
+
+Pads will be represented as children of the top-level XUSB pad controller
+device tree node. Each lane exposed by the pad will be represented by its
+own subnode and can be referenced by users of the lane using the standard
+PHY bindings, as described by the phy-bindings.txt file in this directory.
+
+The Tegra hardware documentation refers to the connection between the XUSB
+pad controller and the XUSB controller as "ports". This is confusing since
+"port" is typically used to denote the physical USB receptacle. The device
+tree binding in this document uses the term "port" to refer to the logical
+abstraction of the signals that are routed to a USB receptacle (i.e. a PHY
+for the USB signal, the VBUS power supply, the USB 2.0 companion port for
+USB 3.0 receptacles, ...).
+
+Required properties:
+--------------------
+- compatible: Must be:
+  - Tegra124: "nvidia,tegra124-xusb-padctl"
+  - Tegra132: "nvidia,tegra132-xusb-padctl", "nvidia,tegra124-xusb-padctl"
+- reg: Physical base address and length of the controller's registers.
+- resets: Must contain an entry for each entry in reset-names.
+- reset-names: Must include the following entries:
+  - "padctl"
+
+
+Pad nodes:
+==========
+
+A required child node named "pads" contains a list of subnodes, one for each
+of the pads exposed by the XUSB pad controller. Each pad may need additional
+resources that can be referenced in its pad node.
+
+The "status" property is used to enable or disable the use of a pad. If set
+to "disabled", the pad will not be used on the given board. In order to use
+the pad and any of its lanes, this property must be set to "okay".
+
+For Tegra124 and Tegra132, the following pads exist: usb2, ulpi, hsic, pcie
+and sata. No extra resources are required for operation of these pads.
+
+
+PHY nodes:
+==========
+
+Each pad node has one or more children, each representing one of the lanes
+controlled by the pad.
+
+Required properties:
+--------------------
+- status: Defines the operation status of the PHY. Valid values are:
+  - "disabled": the PHY is disabled
+  - "okay": the PHY is enabled
+- #phy-cells: Should be 0. Since each lane represents a single PHY, there is
+  no need for an additional specifier.
+- nvidia,function: The output function of the PHY. See below for a list of
+  valid functions per SoC generation.
+
+For Tegra124 and Tegra132, the list of valid PHY nodes is given below:
+- usb2: usb2-0, usb2-1, usb2-2
+  - functions: "snps", "xusb", "uart"
+- ulpi: ulpi-0
+  - functions: "snps", "xusb"
+- hsic: hsic-0, hsic-1
+  - functions: "snps", "xusb"
+- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4
+  - functions: "pcie", "usb3-ss"
+- sata: sata-0
+  - functions: "usb3-ss", "sata"
+
+
+Port nodes:
+===========
+
+A required child node named "ports" contains a list of all the ports exposed
+by the XUSB pad controller. Per-port configuration is only required for USB.
+
+USB2 ports:
+-----------
+
+Required properties:
+- status: Defines the operation status of the port. Valid values are:
+  - "disabled": the port is disabled
+  - "okay": the port is enabled
+- mode: A string that determines the mode in which to run the port. Valid
+  values are:
+  - "host": for USB host mode
+  - "device": for USB device mode
+  - "otg": for USB OTG mode
+
+Optional properties:
+- nvidia,internal: A boolean property whose presence determines that a port
+  is internal. In the absence of this property the port is considered to be
+  external.
+- vbus-supply: phandle to a regulator supplying the VBUS voltage.
+
+ULPI ports:
+-----------
+
+Optional properties:
+- status: Defines the operation status of the port. Valid values are:
+  - "disabled": the port is disabled
+  - "okay": the port is enabled
+- nvidia,internal: A boolean property whose presence determines that a port
+  is internal. In the absence of this property the port is considered to be
+  external.
+- vbus-supply: phandle to a regulator supplying the VBUS voltage.
+
+HSIC ports:
+-----------
+
+Required properties:
+- status: Defines the operation status of the port. Valid values are:
+  - "disabled": the port is disabled
+  - "okay": the port is enabled
+
+Optional properties:
+- vbus-supply: phandle to a regulator supplying the VBUS voltage.
+
+Super-speed USB ports:
+----------------------
+
+Required properties:
+- status: Defines the operation status of the port. Valid values are:
+  - "disabled": the port is disabled
+  - "okay": the port is enabled
+- nvidia,usb2-companion: A single cell that specifies the physical port number
+  to map this super-speed USB port to. The range of valid port numbers varies
+  with the SoC generation:
+  - 0-2: for Tegra124 and Tegra132
+
+Optional properties:
+- nvidia,internal: A boolean property whose presence determines that a port
+  is internal. In the absence of this property the port is considered to be
+  external.
+
+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
+
+
+Examples:
+=========
+
+Tegra124 and Tegra132:
+----------------------
+
+SoC include:
+
+	padctl@0,7009f000 {
+		/* for Tegra124 */
+		compatible = "nvidia,tegra124-xusb-padctl";
+		/* for Tegra132 */
+		compatible = "nvidia,tegra132-xusb-padctl",
+			     "nvidia,tegra124-xusb-padctl";
+		reg = <0x0 0x7009f000 0x0 0x1000>;
+		resets = <&tegra_car 142>;
+		reset-names = "padctl";
+
+		pads {
+			usb2 {
+				status = "disabled";
+
+				usb2-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				usb2-1 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				usb2-2 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+
+			ulpi {
+				status = "disabled";
+
+				ulpi-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+
+			hsic {
+				status = "disabled";
+
+				hsic-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				hsic-1 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+
+			pcie {
+				status = "disabled";
+
+				pcie-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-1 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-2 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-3 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-4 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+
+			sata {
+				status = "disabled";
+
+				sata-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+		};
+
+		ports {
+			usb2-0 {
+				status = "disabled";
+			};
+
+			usb2-1 {
+				status = "disabled";
+			};
+
+			usb2-2 {
+				status = "disabled";
+			};
+
+			ulpi-0 {
+				status = "disabled";
+			};
+
+			hsic-0 {
+				status = "disabled";
+			};
+
+			hsic-1 {
+				status = "disabled";
+			};
+
+			usb3-0 {
+				status = "disabled";
+			};
+
+			usb3-1 {
+				status = "disabled";
+			};
+		};
+	};
+
+Board file:
+
+	padctl@0,7009f000 {
+		status = "okay";
+
+		pads {
+			usb2 {
+				status = "okay";
+
+				usb2-0 {
+					nvidia,function = "xusb";
+					status = "okay";
+				};
+
+				usb2-1 {
+					nvidia,function = "xusb";
+					status = "okay";
+				};
+
+				usb2-2 {
+					nvidia,function = "xusb";
+					status = "okay";
+				};
+			};
+
+			pcie {
+				status = "okay";
+
+				pcie-0 {
+					nvidia,function = "usb3-ss";
+					status = "okay";
+				};
+
+				pcie-2 {
+					nvidia,function = "pcie";
+					status = "okay";
+				};
+
+				pcie-4 {
+					nvidia,function = "pcie";
+					status = "okay";
+				};
+			};
+
+			sata {
+				status = "okay";
+
+				sata-0 {
+					nvidia,function = "sata";
+					status = "okay";
+				};
+			};
+		};
+
+		ports {
+			/* Micro A/B */
+			usb2-0 {
+				status = "okay";
+				mode = "otg";
+			};
+
+			/* Mini PCIe */
+			usb2-1 {
+				status = "okay";
+				mode = "host";
+			};
+
+			/* USB3 */
+			usb2-2 {
+				status = "okay";
+				mode = "host";
+
+				vbus-supply = <&vdd_usb3_vbus>;
+			};
+
+			usb3-0 {
+				nvidia,port = <2>;
+				status = "okay";
+			};
+		};
+	};
diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
index 30676ded85bb..77dfba05ccfd 100644
--- a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
@@ -1,6 +1,11 @@ 
 Device tree binding for NVIDIA Tegra XUSB pad controller
 ========================================================
 
+NOTE: It turns out that this binding isn't an accurate description of the XUSB
+pad controller. While the description is good enough for the functional subset
+required for PCIe and SATA, it lacks the flexibility to represent the features
+needed for USB. For the new binding, see ../phy/nvidia,tegra-xusb-padctl.txt.
+
 The Tegra XUSB pad controller manages a set of lanes, each of which can be
 assigned to one out of a set of different pads. Some of these pads have an
 associated PHY that must be powered up before the pad can be used.