| Message ID | 1414535277-15645-6-git-send-email-abrestic@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 28, 2014 at 03:27:50PM -0700, Andrew Bresticker wrote: [...] > diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt [...] > +Optional properties: > +------------------- > +- vbus-{0,1,2}-supply: VBUS regulator for the corresponding UTMI pad. > +- vddio-hsic-supply: VDDIO regulator for the HSIC pads. > +- nvidia,usb3-port-{0,1}-lane: PCIe/SATA lane to which the corresponding USB3 > + port is mapped. See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list > + of valid values. I dislike how we now need to provide a list of all pins in the header file, where previously we used strings for this. This could become very ugly if the set of pins changes in future generations of this IP block. Could we instead derive this from the pinmux nodes? For example you have this in the example below: usb3p0 { nvidia,lanes = "pcie-0"; ... }; Perhaps what we need is to either key off the node name or add another property, such as: nvidia,usb3-port = <0>; This would match the nvidia,usb2-port property that you've added below. > Lane muxing: > ------------ > @@ -50,6 +62,17 @@ Optional properties: > pin or group should be assigned to. Valid values for function names are > listed below. > - nvidia,iddq: Enables IDDQ mode of the lane. (0: no, 1: yes) > +- nvidia,usb2-port-num: USB2 port (0, 1, or 2) to which the lane is mapped. I'd leave away the -num suffix since it is implied that it's a number. > +- 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. It would be useful for these to provide a range of valid values. I also see that there are other registers that contain values for tuning. I take it that the ones you've included here are the only ones that need to be overridden on hardware you've tested this on? That should be fine, we can always add new ones if necessary. > +- nvidia,hsic-auto-term: Enables HSIC AUTO_TERM. (0: no, 1: yes) > +- nvidia,otg-hs-curr-level-offset: Offset to be applied to the pad's fused > + HS_CURR_LEVEL value. > > Note that not all of these properties are valid for all lanes. Lanes can be > divided into three groups: > @@ -58,18 +81,21 @@ divided into three groups: > > Valid functions for this group are: "snps", "xusb", "uart", "rsvd". > > - The nvidia,iddq property does not apply to this group. > + The nvidia,otg-hs-curr-level-offset property only applies. The wording is confusing here in my opinion. Maybe better: "Only the ... property applies."? > - ulpi-0, hsic-0, hsic-1: > > Valid functions for this group are: "snps", "xusb". > > - The nvidia,iddq property does not apply to this group. > + The nvidia,hsic-* properties apply only to the pins hsic-{0,1} when > + the function is xusb. I assume that hsic-* properties also only apply to the hsic-* pins? That's not clear from the above sentence. Thierry
On Tue, Oct 28, 2014 at 03:27:52PM -0700, Andrew Bresticker wrote: [...] > diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt [...] > + - pll_u_480m > + - clk_m > + - pll_e What are these used for? I guess I'll see when I get to the driver patch. > +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. > + May be a subset of the following: > + - utmi-{0,1,2} > + - hsic-{0,1} > + - usb3-{0,1} > + - 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-pex-pll-supply: PCIe/USB3 PLL power supply. Must supply 1.05V. > + - hvdd-pex-supply: High-voltage PCIe/USB3 power supply. Must supply 3.3V. > + - hvdd-pex-plle-supply: High-voltage PLLE power supply. Must supply 3.3V. I think the name for this in the documentation is HVDD_PEX_PLL_E, which would translate to hvdd-pex-pll-e. At least that's how we named this supply for PCIe. Alternatively it seems like there are aliases for the USB 3.0 related supplies: avdd-pex-pll -> avdd-usb-ss-pll hvdd-pex -> hvdd-usb-ss hvdd-pex-pll-e -> hvdd-usb-ss-pll-e So perhaps these could be used for the XHCI driver instead? Also, should these supplies not be mandatory? Thierry
On Wed, Oct 29, 2014 at 2:43 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Oct 28, 2014 at 03:27:50PM -0700, Andrew Bresticker wrote: > [...] >> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt > [...] >> +Optional properties: >> +------------------- >> +- vbus-{0,1,2}-supply: VBUS regulator for the corresponding UTMI pad. >> +- vddio-hsic-supply: VDDIO regulator for the HSIC pads. >> +- nvidia,usb3-port-{0,1}-lane: PCIe/SATA lane to which the corresponding USB3 >> + port is mapped. See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list >> + of valid values. > > I dislike how we now need to provide a list of all pins in the header > file, where previously we used strings for this. This could become very > ugly if the set of pins changes in future generations of this IP block. > > Could we instead derive this from the pinmux nodes? For example you have > this in the example below: > > usb3p0 { > nvidia,lanes = "pcie-0"; > ... > }; > > Perhaps what we need is to either key off the node name or add another > property, such as: > > nvidia,usb3-port = <0>; > > This would match the nvidia,usb2-port property that you've added below. That is actually how I described the USB3 port to SS lane mapping originally, but in review of an earlier version of this series, Stephen suggested that I make it a separate, not pinconfig property since it wasn't a value written directly to the hardware. I'm fine with changing it back as the pinconfig property makes more sense to me as well.
On Wed, Oct 29, 2014 at 09:37:14AM -0700, Andrew Bresticker wrote: > On Wed, Oct 29, 2014 at 2:43 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Tue, Oct 28, 2014 at 03:27:50PM -0700, Andrew Bresticker wrote: > > [...] > >> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt > > [...] > >> +Optional properties: > >> +------------------- > >> +- vbus-{0,1,2}-supply: VBUS regulator for the corresponding UTMI pad. > >> +- vddio-hsic-supply: VDDIO regulator for the HSIC pads. > >> +- nvidia,usb3-port-{0,1}-lane: PCIe/SATA lane to which the corresponding USB3 > >> + port is mapped. See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list > >> + of valid values. > > > > I dislike how we now need to provide a list of all pins in the header > > file, where previously we used strings for this. This could become very > > ugly if the set of pins changes in future generations of this IP block. > > > > Could we instead derive this from the pinmux nodes? For example you have > > this in the example below: > > > > usb3p0 { > > nvidia,lanes = "pcie-0"; > > ... > > }; > > > > Perhaps what we need is to either key off the node name or add another > > property, such as: > > > > nvidia,usb3-port = <0>; > > > > This would match the nvidia,usb2-port property that you've added below. > > That is actually how I described the USB3 port to SS lane mapping > originally, but in review of an earlier version of this series, > Stephen suggested that I make it a separate, not pinconfig property > since it wasn't a value written directly to the hardware. I'm fine > with changing it back as the pinconfig property makes more sense to me > as well. Hmm... I had considered it a mux option of the specific lane. If the function is usb3, it'd still need to be muxed to one of the ports. So it's additional information associated with the usb3 function. I did look through the driver changes and can't really make out which part of the code actually performs this assignment. Can you point me to it? Thierry
On Thu, Oct 30, 2014 at 6:55 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Oct 29, 2014 at 09:37:14AM -0700, Andrew Bresticker wrote: >> On Wed, Oct 29, 2014 at 2:43 AM, Thierry Reding >> <thierry.reding@gmail.com> wrote: >> > On Tue, Oct 28, 2014 at 03:27:50PM -0700, Andrew Bresticker wrote: >> > [...] >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt >> > [...] >> >> +Optional properties: >> >> +------------------- >> >> +- vbus-{0,1,2}-supply: VBUS regulator for the corresponding UTMI pad. >> >> +- vddio-hsic-supply: VDDIO regulator for the HSIC pads. >> >> +- nvidia,usb3-port-{0,1}-lane: PCIe/SATA lane to which the corresponding USB3 >> >> + port is mapped. See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list >> >> + of valid values. >> > >> > I dislike how we now need to provide a list of all pins in the header >> > file, where previously we used strings for this. This could become very >> > ugly if the set of pins changes in future generations of this IP block. >> > >> > Could we instead derive this from the pinmux nodes? For example you have >> > this in the example below: >> > >> > usb3p0 { >> > nvidia,lanes = "pcie-0"; >> > ... >> > }; >> > >> > Perhaps what we need is to either key off the node name or add another >> > property, such as: >> > >> > nvidia,usb3-port = <0>; >> > >> > This would match the nvidia,usb2-port property that you've added below. >> >> That is actually how I described the USB3 port to SS lane mapping >> originally, but in review of an earlier version of this series, >> Stephen suggested that I make it a separate, not pinconfig property >> since it wasn't a value written directly to the hardware. I'm fine >> with changing it back as the pinconfig property makes more sense to me >> as well. > > Hmm... I had considered it a mux option of the specific lane. If the > function is usb3, it'd still need to be muxed to one of the ports. So > it's additional information associated with the usb3 function. > > I did look through the driver changes and can't really make out which > part of the code actually performs this assignment. Can you point me to > it? There's not really an assignment. The property is used to map between a lane (e.g. PCIe-0 or SATA) and the USB3.0 port it's mapped to. For an example of where it's used, take a look at usb3_phy_power_on(). There are certain per-lane registers which need to be programmed in addition to the per-USB3.0 port pad registers. This mapping is used to determine which lane needs to be programmed.
On Thu, Oct 30, 2014 at 10:19:21AM -0700, Andrew Bresticker wrote: > On Thu, Oct 30, 2014 at 6:55 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Wed, Oct 29, 2014 at 09:37:14AM -0700, Andrew Bresticker wrote: > >> On Wed, Oct 29, 2014 at 2:43 AM, Thierry Reding > >> <thierry.reding@gmail.com> wrote: > >> > On Tue, Oct 28, 2014 at 03:27:50PM -0700, Andrew Bresticker wrote: > >> > [...] > >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt > >> > [...] > >> >> +Optional properties: > >> >> +------------------- > >> >> +- vbus-{0,1,2}-supply: VBUS regulator for the corresponding UTMI pad. > >> >> +- vddio-hsic-supply: VDDIO regulator for the HSIC pads. > >> >> +- nvidia,usb3-port-{0,1}-lane: PCIe/SATA lane to which the corresponding USB3 > >> >> + port is mapped. See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list > >> >> + of valid values. > >> > > >> > I dislike how we now need to provide a list of all pins in the header > >> > file, where previously we used strings for this. This could become very > >> > ugly if the set of pins changes in future generations of this IP block. > >> > > >> > Could we instead derive this from the pinmux nodes? For example you have > >> > this in the example below: > >> > > >> > usb3p0 { > >> > nvidia,lanes = "pcie-0"; > >> > ... > >> > }; > >> > > >> > Perhaps what we need is to either key off the node name or add another > >> > property, such as: > >> > > >> > nvidia,usb3-port = <0>; > >> > > >> > This would match the nvidia,usb2-port property that you've added below. > >> > >> That is actually how I described the USB3 port to SS lane mapping > >> originally, but in review of an earlier version of this series, > >> Stephen suggested that I make it a separate, not pinconfig property > >> since it wasn't a value written directly to the hardware. I'm fine > >> with changing it back as the pinconfig property makes more sense to me > >> as well. > > > > Hmm... I had considered it a mux option of the specific lane. If the > > function is usb3, it'd still need to be muxed to one of the ports. So > > it's additional information associated with the usb3 function. > > > > I did look through the driver changes and can't really make out which > > part of the code actually performs this assignment. Can you point me to > > it? > > There's not really an assignment. The property is used to map between > a lane (e.g. PCIe-0 or SATA) and the USB3.0 port it's mapped to. For > an example of where it's used, take a look at usb3_phy_power_on(). > There are certain per-lane registers which need to be programmed in > addition to the per-USB3.0 port pad registers. This mapping is used > to determine which lane needs to be programmed. Are you saying the mapping of lane to USB port is fixed? That is, PCIe-0 lane is always used for USB port X and SATA always for USB port Y? If so I'd argue that we don't need this property in DT at all. Thierry
On Thu, Oct 30, 2014 at 10:24 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, Oct 30, 2014 at 10:19:21AM -0700, Andrew Bresticker wrote: >> On Thu, Oct 30, 2014 at 6:55 AM, Thierry Reding >> <thierry.reding@gmail.com> wrote: >> > On Wed, Oct 29, 2014 at 09:37:14AM -0700, Andrew Bresticker wrote: >> >> On Wed, Oct 29, 2014 at 2:43 AM, Thierry Reding >> >> <thierry.reding@gmail.com> wrote: >> >> > On Tue, Oct 28, 2014 at 03:27:50PM -0700, Andrew Bresticker wrote: >> >> > [...] >> >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt >> >> > [...] >> >> >> +Optional properties: >> >> >> +------------------- >> >> >> +- vbus-{0,1,2}-supply: VBUS regulator for the corresponding UTMI pad. >> >> >> +- vddio-hsic-supply: VDDIO regulator for the HSIC pads. >> >> >> +- nvidia,usb3-port-{0,1}-lane: PCIe/SATA lane to which the corresponding USB3 >> >> >> + port is mapped. See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list >> >> >> + of valid values. >> >> > >> >> > I dislike how we now need to provide a list of all pins in the header >> >> > file, where previously we used strings for this. This could become very >> >> > ugly if the set of pins changes in future generations of this IP block. >> >> > >> >> > Could we instead derive this from the pinmux nodes? For example you have >> >> > this in the example below: >> >> > >> >> > usb3p0 { >> >> > nvidia,lanes = "pcie-0"; >> >> > ... >> >> > }; >> >> > >> >> > Perhaps what we need is to either key off the node name or add another >> >> > property, such as: >> >> > >> >> > nvidia,usb3-port = <0>; >> >> > >> >> > This would match the nvidia,usb2-port property that you've added below. >> >> >> >> That is actually how I described the USB3 port to SS lane mapping >> >> originally, but in review of an earlier version of this series, >> >> Stephen suggested that I make it a separate, not pinconfig property >> >> since it wasn't a value written directly to the hardware. I'm fine >> >> with changing it back as the pinconfig property makes more sense to me >> >> as well. >> > >> > Hmm... I had considered it a mux option of the specific lane. If the >> > function is usb3, it'd still need to be muxed to one of the ports. So >> > it's additional information associated with the usb3 function. >> > >> > I did look through the driver changes and can't really make out which >> > part of the code actually performs this assignment. Can you point me to >> > it? >> >> There's not really an assignment. The property is used to map between >> a lane (e.g. PCIe-0 or SATA) and the USB3.0 port it's mapped to. For >> an example of where it's used, take a look at usb3_phy_power_on(). >> There are certain per-lane registers which need to be programmed in >> addition to the per-USB3.0 port pad registers. This mapping is used >> to determine which lane needs to be programmed. > > Are you saying the mapping of lane to USB port is fixed? That is, PCIe-0 > lane is always used for USB port X and SATA always for USB port Y? No, sorry if that was unclear, it's not fixed - it's a board specific property.
On Thu, Oct 30, 2014 at 10:26:47AM -0700, Andrew Bresticker wrote: > On Thu, Oct 30, 2014 at 10:24 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Thu, Oct 30, 2014 at 10:19:21AM -0700, Andrew Bresticker wrote: > >> On Thu, Oct 30, 2014 at 6:55 AM, Thierry Reding > >> <thierry.reding@gmail.com> wrote: > >> > On Wed, Oct 29, 2014 at 09:37:14AM -0700, Andrew Bresticker wrote: > >> >> On Wed, Oct 29, 2014 at 2:43 AM, Thierry Reding > >> >> <thierry.reding@gmail.com> wrote: > >> >> > On Tue, Oct 28, 2014 at 03:27:50PM -0700, Andrew Bresticker wrote: > >> >> > [...] > >> >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt > >> >> > [...] > >> >> >> +Optional properties: > >> >> >> +------------------- > >> >> >> +- vbus-{0,1,2}-supply: VBUS regulator for the corresponding UTMI pad. > >> >> >> +- vddio-hsic-supply: VDDIO regulator for the HSIC pads. > >> >> >> +- nvidia,usb3-port-{0,1}-lane: PCIe/SATA lane to which the corresponding USB3 > >> >> >> + port is mapped. See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list > >> >> >> + of valid values. > >> >> > > >> >> > I dislike how we now need to provide a list of all pins in the header > >> >> > file, where previously we used strings for this. This could become very > >> >> > ugly if the set of pins changes in future generations of this IP block. > >> >> > > >> >> > Could we instead derive this from the pinmux nodes? For example you have > >> >> > this in the example below: > >> >> > > >> >> > usb3p0 { > >> >> > nvidia,lanes = "pcie-0"; > >> >> > ... > >> >> > }; > >> >> > > >> >> > Perhaps what we need is to either key off the node name or add another > >> >> > property, such as: > >> >> > > >> >> > nvidia,usb3-port = <0>; > >> >> > > >> >> > This would match the nvidia,usb2-port property that you've added below. > >> >> > >> >> That is actually how I described the USB3 port to SS lane mapping > >> >> originally, but in review of an earlier version of this series, > >> >> Stephen suggested that I make it a separate, not pinconfig property > >> >> since it wasn't a value written directly to the hardware. I'm fine > >> >> with changing it back as the pinconfig property makes more sense to me > >> >> as well. > >> > > >> > Hmm... I had considered it a mux option of the specific lane. If the > >> > function is usb3, it'd still need to be muxed to one of the ports. So > >> > it's additional information associated with the usb3 function. > >> > > >> > I did look through the driver changes and can't really make out which > >> > part of the code actually performs this assignment. Can you point me to > >> > it? > >> > >> There's not really an assignment. The property is used to map between > >> a lane (e.g. PCIe-0 or SATA) and the USB3.0 port it's mapped to. For > >> an example of where it's used, take a look at usb3_phy_power_on(). > >> There are certain per-lane registers which need to be programmed in > >> addition to the per-USB3.0 port pad registers. This mapping is used > >> to determine which lane needs to be programmed. > > > > Are you saying the mapping of lane to USB port is fixed? That is, PCIe-0 > > lane is always used for USB port X and SATA always for USB port Y? > > No, sorry if that was unclear, it's not fixed - it's a board specific > property. Okay, but there's no register that contains the mapping of the port to a lane, similar to what's done for the functions, right? I mean the driver only uses the lane to find out which register to write. Doesn't that imply that two lanes (or more) could be mapped to the same USB 3.0 port? I'm not sure I'm being clear here, so let me try another way. In order to establish a mapping between USB port and lane, I would've expected one of the following to happen: - A value derived from the lane number is written to a register belonging to a given port. - A value derived from the port number is written to a register belonging to a given lane. I can't see the code do either of the above, which to me implies that there's a fixed mapping between lanes and ports. What am I missing? Thierry
On Fri, Oct 31, 2014 at 4:32 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, Oct 30, 2014 at 10:26:47AM -0700, Andrew Bresticker wrote: >> On Thu, Oct 30, 2014 at 10:24 AM, Thierry Reding >> <thierry.reding@gmail.com> wrote: >> > On Thu, Oct 30, 2014 at 10:19:21AM -0700, Andrew Bresticker wrote: >> >> On Thu, Oct 30, 2014 at 6:55 AM, Thierry Reding >> >> <thierry.reding@gmail.com> wrote: >> >> > On Wed, Oct 29, 2014 at 09:37:14AM -0700, Andrew Bresticker wrote: >> >> >> On Wed, Oct 29, 2014 at 2:43 AM, Thierry Reding >> >> >> <thierry.reding@gmail.com> wrote: >> >> >> > On Tue, Oct 28, 2014 at 03:27:50PM -0700, Andrew Bresticker wrote: >> >> >> > [...] >> >> >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt >> >> >> > [...] >> >> >> >> +Optional properties: >> >> >> >> +------------------- >> >> >> >> +- vbus-{0,1,2}-supply: VBUS regulator for the corresponding UTMI pad. >> >> >> >> +- vddio-hsic-supply: VDDIO regulator for the HSIC pads. >> >> >> >> +- nvidia,usb3-port-{0,1}-lane: PCIe/SATA lane to which the corresponding USB3 >> >> >> >> + port is mapped. See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list >> >> >> >> + of valid values. >> >> >> > >> >> >> > I dislike how we now need to provide a list of all pins in the header >> >> >> > file, where previously we used strings for this. This could become very >> >> >> > ugly if the set of pins changes in future generations of this IP block. >> >> >> > >> >> >> > Could we instead derive this from the pinmux nodes? For example you have >> >> >> > this in the example below: >> >> >> > >> >> >> > usb3p0 { >> >> >> > nvidia,lanes = "pcie-0"; >> >> >> > ... >> >> >> > }; >> >> >> > >> >> >> > Perhaps what we need is to either key off the node name or add another >> >> >> > property, such as: >> >> >> > >> >> >> > nvidia,usb3-port = <0>; >> >> >> > >> >> >> > This would match the nvidia,usb2-port property that you've added below. >> >> >> >> >> >> That is actually how I described the USB3 port to SS lane mapping >> >> >> originally, but in review of an earlier version of this series, >> >> >> Stephen suggested that I make it a separate, not pinconfig property >> >> >> since it wasn't a value written directly to the hardware. I'm fine >> >> >> with changing it back as the pinconfig property makes more sense to me >> >> >> as well. >> >> > >> >> > Hmm... I had considered it a mux option of the specific lane. If the >> >> > function is usb3, it'd still need to be muxed to one of the ports. So >> >> > it's additional information associated with the usb3 function. >> >> > >> >> > I did look through the driver changes and can't really make out which >> >> > part of the code actually performs this assignment. Can you point me to >> >> > it? >> >> >> >> There's not really an assignment. The property is used to map between >> >> a lane (e.g. PCIe-0 or SATA) and the USB3.0 port it's mapped to. For >> >> an example of where it's used, take a look at usb3_phy_power_on(). >> >> There are certain per-lane registers which need to be programmed in >> >> addition to the per-USB3.0 port pad registers. This mapping is used >> >> to determine which lane needs to be programmed. >> > >> > Are you saying the mapping of lane to USB port is fixed? That is, PCIe-0 >> > lane is always used for USB port X and SATA always for USB port Y? >> >> No, sorry if that was unclear, it's not fixed - it's a board specific >> property. > > Okay, but there's no register that contains the mapping of the port to a > lane, similar to what's done for the functions, right? Correct. > I mean the driver only uses the lane to find out which register to write. > Doesn't that imply that two lanes (or more) could be mapped to the same > USB 3.0 port? I guess? Not sure how that would work in hardware to have two SuperSpeed lanes wired up to a single port. > I'm not sure I'm being clear here, so let me try another way. In order > to establish a mapping between USB port and lane, I would've expected > one of the following to happen: > > - A value derived from the lane number is written to a register > belonging to a given port. > > - A value derived from the port number is written to a register > belonging to a given lane. > > I can't see the code do either of the above, which to me implies that > there's a fixed mapping between lanes and ports. What am I missing? It's fixed in that it's not a software-modifiable property. It's describing an electrical connection between lane and port. For example, it's possible that one board has PCIe lane 1 wired up to SuperSpeed port 1 and on another board for the SATA lane to be wired up to SuperSpeed port 1. Does that make sense?
On Fri, Oct 31, 2014 at 9:41 AM, Andrew Bresticker <abrestic@chromium.org> wrote: > On Fri, Oct 31, 2014 at 4:32 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: >> On Thu, Oct 30, 2014 at 10:26:47AM -0700, Andrew Bresticker wrote: >>> On Thu, Oct 30, 2014 at 10:24 AM, Thierry Reding >>> <thierry.reding@gmail.com> wrote: >>> > On Thu, Oct 30, 2014 at 10:19:21AM -0700, Andrew Bresticker wrote: >>> >> On Thu, Oct 30, 2014 at 6:55 AM, Thierry Reding >>> >> <thierry.reding@gmail.com> wrote: >>> >> > On Wed, Oct 29, 2014 at 09:37:14AM -0700, Andrew Bresticker wrote: >>> >> >> On Wed, Oct 29, 2014 at 2:43 AM, Thierry Reding >>> >> >> <thierry.reding@gmail.com> wrote: >>> >> >> > On Tue, Oct 28, 2014 at 03:27:50PM -0700, Andrew Bresticker wrote: >>> >> >> > [...] >>> >> >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt >>> >> >> > [...] >>> >> >> >> +Optional properties: >>> >> >> >> +------------------- >>> >> >> >> +- vbus-{0,1,2}-supply: VBUS regulator for the corresponding UTMI pad. >>> >> >> >> +- vddio-hsic-supply: VDDIO regulator for the HSIC pads. >>> >> >> >> +- nvidia,usb3-port-{0,1}-lane: PCIe/SATA lane to which the corresponding USB3 >>> >> >> >> + port is mapped. See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list >>> >> >> >> + of valid values. >>> >> >> > >>> >> >> > I dislike how we now need to provide a list of all pins in the header >>> >> >> > file, where previously we used strings for this. This could become very >>> >> >> > ugly if the set of pins changes in future generations of this IP block. >>> >> >> > >>> >> >> > Could we instead derive this from the pinmux nodes? For example you have >>> >> >> > this in the example below: >>> >> >> > >>> >> >> > usb3p0 { >>> >> >> > nvidia,lanes = "pcie-0"; >>> >> >> > ... >>> >> >> > }; >>> >> >> > >>> >> >> > Perhaps what we need is to either key off the node name or add another >>> >> >> > property, such as: >>> >> >> > >>> >> >> > nvidia,usb3-port = <0>; >>> >> >> > >>> >> >> > This would match the nvidia,usb2-port property that you've added below. >>> >> >> >>> >> >> That is actually how I described the USB3 port to SS lane mapping >>> >> >> originally, but in review of an earlier version of this series, >>> >> >> Stephen suggested that I make it a separate, not pinconfig property >>> >> >> since it wasn't a value written directly to the hardware. I'm fine >>> >> >> with changing it back as the pinconfig property makes more sense to me >>> >> >> as well. >>> >> > >>> >> > Hmm... I had considered it a mux option of the specific lane. If the >>> >> > function is usb3, it'd still need to be muxed to one of the ports. So >>> >> > it's additional information associated with the usb3 function. >>> >> > >>> >> > I did look through the driver changes and can't really make out which >>> >> > part of the code actually performs this assignment. Can you point me to >>> >> > it? >>> >> >>> >> There's not really an assignment. The property is used to map between >>> >> a lane (e.g. PCIe-0 or SATA) and the USB3.0 port it's mapped to. For >>> >> an example of where it's used, take a look at usb3_phy_power_on(). >>> >> There are certain per-lane registers which need to be programmed in >>> >> addition to the per-USB3.0 port pad registers. This mapping is used >>> >> to determine which lane needs to be programmed. >>> > >>> > Are you saying the mapping of lane to USB port is fixed? That is, PCIe-0 >>> > lane is always used for USB port X and SATA always for USB port Y? >>> >>> No, sorry if that was unclear, it's not fixed - it's a board specific >>> property. >> >> Okay, but there's no register that contains the mapping of the port to a >> lane, similar to what's done for the functions, right? > > Correct. > >> I mean the driver only uses the lane to find out which register to write. >> Doesn't that imply that two lanes (or more) could be mapped to the same >> USB 3.0 port? > > I guess? Not sure how that would work in hardware to have two > SuperSpeed lanes wired up to a single port. > >> I'm not sure I'm being clear here, so let me try another way. In order >> to establish a mapping between USB port and lane, I would've expected >> one of the following to happen: >> >> - A value derived from the lane number is written to a register >> belonging to a given port. >> >> - A value derived from the port number is written to a register >> belonging to a given lane. >> >> I can't see the code do either of the above, which to me implies that >> there's a fixed mapping between lanes and ports. What am I missing? > > It's fixed in that it's not a software-modifiable property. It's > describing an electrical connection between lane and port. For > example, it's possible that one board has PCIe lane 1 wired up to > SuperSpeed port 1 and on another board for the SATA lane to be wired > up to SuperSpeed port 1. Does that make sense? So what's the verdict here?
diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt new file mode 100644 index 0000000..51a7751 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt @@ -0,0 +1,104 @@ +NVIDIA Tegra xHCI controller +============================ + +The Tegra xHCI controller supports both USB2 and USB3 interfaces exposed +by the Tegra XUSB pad controller. + +Required properties: +-------------------- + - compatible: Should be "nvidia,tegra124-xhci". + - reg: Address and length of the register sets. There should be three + entries in the following order: xHCI host registers, FPCI registers, and + IPFS registers. + - interrupts: xHCI host 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_dev + - xusb_dev_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_dev + - xusb_ss + - xusb + Note that xusb_dev is the shared reset for xusb_dev and xusb_dev_src and + that xusb is the shared reset for xusb_{ss,hs,fs,falcon,host}_src. + - mboxes: Must contain an entry for the XUSB mailbox channel. + See ../mailbox/mailbox.txt for details. + - mbox-names: Must include the following entries: + - xusb + +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. + May be a subset of the following: + - utmi-{0,1,2} + - hsic-{0,1} + - usb3-{0,1} + - 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-pex-pll-supply: PCIe/USB3 PLL power supply. Must supply 1.05V. + - hvdd-pex-supply: High-voltage PCIe/USB3 power supply. Must supply 3.3V. + - hvdd-pex-plle-supply: High-voltage PLLE power supply. Must supply 3.3V. + +Example: +-------- + usb@0,70090000 { + compatible = "nvidia,tegra124-xhci"; + reg = <0x0 0x70090000 0x0 0x8000>, + <0x0 0x70098000 0x0 0x1000>, + <0x0 0x70099000 0x0 0x1000>; + interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&tegra_car TEGRA124_CLK_XUSB_HOST>, + <&tegra_car TEGRA124_CLK_XUSB_HOST_SRC>, + <&tegra_car TEGRA124_CLK_XUSB_DEV>, + <&tegra_car TEGRA124_CLK_XUSB_DEV_SRC>, + <&tegra_car TEGRA124_CLK_XUSB_FALCON_SRC>, + <&tegra_car TEGRA124_CLK_XUSB_SS>, + <&tegra_car TEGRA124_CLK_XUSB_SS_DIV2>, + <&tegra_car TEGRA124_CLK_XUSB_SS_SRC>, + <&tegra_car TEGRA124_CLK_XUSB_HS_SRC>, + <&tegra_car TEGRA124_CLK_XUSB_FS_SRC>, + <&tegra_car TEGRA124_CLK_PLL_U_480M>, + <&tegra_car TEGRA124_CLK_CLK_M>, + <&tegra_car TEGRA124_CLK_PLL_E>; + clock-names = "xusb_host", "xusb_host_src", "xusb_dev", + "xusb_dev_src", "xusb_falcon_src", "xusb_ss", + "xusb_ss_div2", "xusb_ss_src", "xusb_hs_src", + "xusb_fs_src", "pll_u_480m", "clk_m", "pll_e"; + resets = <&tegra_car 89>, <&tegra_car 95>, <&tegra_car 156>, + <&tegra_car 143>; + reset-names = "xusb_host", "xusb_dev", "xusb_ss", "xusb"; + mboxes = <&xusb_mbox>; + mbox-names = "xusb"; + phys = <&padctl TEGRA_XUSB_PADCTL_UTMI_P1>, /* mini-PCIe USB */ + <&padctl TEGRA_XUSB_PADCTL_UTMI_P2>, /* USB A */ + <&padctl TEGRA_XUSB_PADCTL_USB3_P0>; /* USB A */ + phy-names = "utmi-1", "utmi-2", "usb3-0"; + avddio-pex-supply = <&vdd_1v05_run>; + dvddio-pex-supply = <&vdd_1v05_run>; + avdd-usb-supply = <&vdd_3v3_lp0>; + avdd-pll-utmip-supply = <&vddio_1v8>; + avdd-pll-erefe-supply = <&avdd_1v05_run>; + avdd-pex-pll-supply = <&vdd_1v05_run>; + hvdd-pex-supply = <&vdd_3v3_lp0>; + hvdd-pex-plle-supply = <&vdd_3v3_lp0>; + };