| Message ID | 1444158109-1590-1-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
(Dropping a lot of CCs since it looks like this needs some more work) On 10/06/2015 01:01 PM, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > Add device-tree binding documentation for the XUSB (xHCI) controller > present on Tegra124 and later SoCs. > > Signed-off-by: Andrew Bresticker <abrestic@chromium.org> > [swarren, combined separate MFD, mailbox, XHCI bindings into one node] > Signed-off-by: Stephen Warren <swarren@nvidia.com> > diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt > +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 "<type>-<number>" where <type> is one of "utmi", > + "hsic", or "usb3" and <number> is a 0-based index. On Tegra124, there may > + be up to 3 UTMI, 2 HSIC, and 2 USB3 PHYs. Now that I've looked at this binding in more detail, I think this phy-related text is wrong. First off, I don't think the phys are optional at all. Whenever a phy is used, we must include it in the phys/phy-names list. I guess what we need is a more precise definition of which entries are required, and then to make these properties mandatory. I think the naming of the phys above may be wrong too. The text above sounds like it's using the names of the actual phys, rather than of the ports that are using them. Since there's a big mux between the ports and the phys, I suspect it would make more sense to identify the ports in phy-names, and have the values in phys reflect the configuration of the mux. That way no matter how the mux is configured, the names used in phy-names always map 1:1 to the XUSB controller ports. Does that make sense? For some of the ports, this makes no difference since all the mux does is allow XUSB vs. some other module access to the PHY. But I think for at least the USB3 ports, each port could be mux'd to various different PHYs/lanes, so this distinction does have an effect. Question: Should the XUSB controller driver determine which of its ports to enable/use based solely on which phy-names are present, or should we have some explicit property to define which ports are in use? Downstream does have a property for this purpose. Question: Each physical USB connector contains signals from both a USB2 port and a USB3 port. The HW can be configured to associate different USB2 and USB3 controllers with each-other for this purpose. The configuration for this happens via a register in the XUSB padctl module in register SS_PORT_MAP. However, the XUSB padctl binding doesn't allow specification of that mapping yet, and the driver doesn't program that register. I'm not sure if we should have a similar mapping property in the XUSB controller's DT node, a custom API between the two drivers to retrieve that information, or perhaps the XUSB controller driver doesn't actually care, since HW implements all required interactions. Does anyone know? Downstream also has a property that describes this mapping. (Although note that the downstream properties for those two purposes, or at least the code that handles them, seems like a mess to me so far). Either way, I think we need to update the XUSB padctl binding to contain this information before we consider the XUSB controller binding to be fully fleshed out, so we can make sure the two bindings interact well. > +Example: > + 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"; We're also missing an update to the XUSB padctl binding to define all those TEGRA_XUSB_PADCTL_UTMI_P* values. I think we need to complete that in order to make sure that both bindings are fully thought out. Thierry, I found your WIP patches perhaps related to this at: > https://github.com/thierryreding/linux/commits/staging/xhci However, those top two patches there don't contain any DT binding or .dts file updates so it's difficult to tell where the changes are going. Do you have binding/.dts patches elsewhere to show the intended result? I notice that series may also have support for configuring the per-board tuning parameters too. That'd be useful to get in place too. Do you have a source for the actual tuning values for any of the T210 boards that mainline U-Boot now supports? -- 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
On 14/10/15 20:20, Stephen Warren wrote: > (Dropping a lot of CCs since it looks like this needs some more work) > > On 10/06/2015 01:01 PM, Stephen Warren wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> Add device-tree binding documentation for the XUSB (xHCI) controller >> present on Tegra124 and later SoCs. >> >> Signed-off-by: Andrew Bresticker <abrestic@chromium.org> >> [swarren, combined separate MFD, mailbox, XHCI bindings into one node] >> Signed-off-by: Stephen Warren <swarren@nvidia.com> > >> diff --git >> a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt >> b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt > >> +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 "<type>-<number>" where <type> is one of >> "utmi", >> + "hsic", or "usb3" and <number> is a 0-based index. On Tegra124, >> there may >> + be up to 3 UTMI, 2 HSIC, and 2 USB3 PHYs. > > Now that I've looked at this binding in more detail, I think this > phy-related text is wrong. > > First off, I don't think the phys are optional at all. Whenever a phy is > used, we must include it in the phys/phy-names list. I guess what we > need is a more precise definition of which entries are required, and > then to make these properties mandatory. > > I think the naming of the phys above may be wrong too. The text above > sounds like it's using the names of the actual phys, rather than of the > ports that are using them. Since there's a big mux between the ports and > the phys, I suspect it would make more sense to identify the ports in > phy-names, and have the values in phys reflect the configuration of the > mux. That way no matter how the mux is configured, the names used in > phy-names always map 1:1 to the XUSB controller ports. Does that make > sense? For some of the ports, this makes no difference since all the mux > does is allow XUSB vs. some other module access to the PHY. But I think > for at least the USB3 ports, each port could be mux'd to various > different PHYs/lanes, so this distinction does have an effect. Yes but please note that the documentation is a bit misleading on the number of actual options you have. For example, usb3-port0 is only available on pcie lane 0. I have just posted a patch to correct this [0]. > Question: Should the XUSB controller driver determine which of its ports > to enable/use based solely on which phy-names are present, or should we > have some explicit property to define which ports are in use? Downstream > does have a property for this purpose. Given that the number of port-to-phy mappings are finite and there are not too many, could be good to define them all and have the board dts enabled the ones it is using. Right now it is not very clear exactly what ports are available on which lanes. > Question: Each physical USB connector contains signals from both a USB2 > port and a USB3 port. The HW can be configured to associate different > USB2 and USB3 controllers with each-other for this purpose. The > configuration for this happens via a register in the XUSB padctl module > in register SS_PORT_MAP. However, the XUSB padctl binding doesn't allow > specification of that mapping yet, and the driver doesn't program that > register. I'm not sure if we should have a similar mapping property in > the XUSB controller's DT node, a custom API between the two drivers to > retrieve that information, or perhaps the XUSB controller driver doesn't > actually care, since HW implements all required interactions. Does > anyone know? Downstream also has a property that describes this mapping. My understanding is the the XUSB controller has 3 usb2 ports and 2 usb3 ports. The SS_PORT_MAP allows you to map any of the 3 usb2 ports from the XUSB module to any of the 2 usb3 ports. Yes this mapping is not supported currently by the padctl binding but this was in Andrew's series for XHCI [1] and the patch you are probably interested in is here [2]. Jon > (Although note that the downstream properties for those two purposes, or > at least the code that handles them, seems like a mess to me so far). > > Either way, I think we need to update the XUSB padctl binding to contain > this information before we consider the XUSB controller binding to be > fully fleshed out, so we can make sure the two bindings interact well. > >> +Example: > >> + 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"; > > We're also missing an update to the XUSB padctl binding to define all > those TEGRA_XUSB_PADCTL_UTMI_P* values. I think we need to complete that > in order to make sure that both bindings are fully thought out. > > Thierry, I found your WIP patches perhaps related to this at: >> https://github.com/thierryreding/linux/commits/staging/xhci > > However, those top two patches there don't contain any DT binding or > .dts file updates so it's difficult to tell where the changes are going. > Do you have binding/.dts patches elsewhere to show the intended result? > > I notice that series may also have support for configuring the per-board > tuning parameters too. That'd be useful to get in place too. Do you have > a source for the actual tuning values for any of the T210 boards that > mainline U-Boot now supports? [0] http://marc.info/?l=linux-tegra&m=144498745102948&w=2 [1] https://github.com/abrestic/linux/tree/tegra-xhci-v8-test [2] https://github.com/abrestic/linux/commit/82281999409b6c5a6a51df449a1c45204a249df8 -- 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
On 10/16/2015 03:37 AM, Jon Hunter wrote: > > On 14/10/15 20:20, Stephen Warren wrote: >> (Dropping a lot of CCs since it looks like this needs some more work) >> >> On 10/06/2015 01:01 PM, Stephen Warren wrote: >>> From: Stephen Warren <swarren@nvidia.com> >>> >>> Add device-tree binding documentation for the XUSB (xHCI) controller >>> present on Tegra124 and later SoCs. >>> >>> Signed-off-by: Andrew Bresticker <abrestic@chromium.org> >>> [swarren, combined separate MFD, mailbox, XHCI bindings into one node] >>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >> >>> diff --git >>> a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt >>> b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt >> >>> +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 "<type>-<number>" where <type> is one of >>> "utmi", >>> + "hsic", or "usb3" and <number> is a 0-based index. On Tegra124, >>> there may >>> + be up to 3 UTMI, 2 HSIC, and 2 USB3 PHYs. >> >> Now that I've looked at this binding in more detail, I think this >> phy-related text is wrong. >> >> First off, I don't think the phys are optional at all. Whenever a phy is >> used, we must include it in the phys/phy-names list. I guess what we >> need is a more precise definition of which entries are required, and >> then to make these properties mandatory. >> >> I think the naming of the phys above may be wrong too. The text above >> sounds like it's using the names of the actual phys, rather than of the >> ports that are using them. Since there's a big mux between the ports and >> the phys, I suspect it would make more sense to identify the ports in >> phy-names, and have the values in phys reflect the configuration of the >> mux. That way no matter how the mux is configured, the names used in >> phy-names always map 1:1 to the XUSB controller ports. Does that make >> sense? For some of the ports, this makes no difference since all the mux >> does is allow XUSB vs. some other module access to the PHY. But I think >> for at least the USB3 ports, each port could be mux'd to various >> different PHYs/lanes, so this distinction does have an effect. > > Yes but please note that the documentation is a bit misleading on the > number of actual options you have. For example, usb3-port0 is only > available on pcie lane 0. I have just posted a patch to correct this [0]. > >> Question: Should the XUSB controller driver determine which of its ports >> to enable/use based solely on which phy-names are present, or should we >> have some explicit property to define which ports are in use? Downstream >> does have a property for this purpose. > > Given that the number of port-to-phy mappings are finite and there are > not too many, could be good to define them all and have the board dts > enabled the ones it is using. Right now it is not very clear exactly > what ports are available on which lanes. I think I'd prefer a binding with something more like a node per port or an enable bit/flag per port. This keeps things simple and maps directly to the HW. If we start enumerating all the valid combinations of which ports are active and mapped where, then we will need (a) to maintain that table in the binding, (b) a table in the driver to map from the list of options the binding allows to the set of HW ports/registers/... that are enabled. That sounds like extra complexity that's not necessary. >> Question: Each physical USB connector contains signals from both a USB2 >> port and a USB3 port. The HW can be configured to associate different >> USB2 and USB3 controllers with each-other for this purpose. The >> configuration for this happens via a register in the XUSB padctl module >> in register SS_PORT_MAP. However, the XUSB padctl binding doesn't allow >> specification of that mapping yet, and the driver doesn't program that >> register. I'm not sure if we should have a similar mapping property in >> the XUSB controller's DT node, a custom API between the two drivers to >> retrieve that information, or perhaps the XUSB controller driver doesn't >> actually care, since HW implements all required interactions. Does >> anyone know? Downstream also has a property that describes this mapping. > > My understanding is the the XUSB controller has 3 usb2 ports and 2 usb3 > ports. The SS_PORT_MAP allows you to map any of the 3 usb2 ports from > the XUSB module to any of the 2 usb3 ports. Yes this mapping is not > supported currently by the padctl binding but this was in Andrew's > series for XHCI [1] and the patch you are probably interested in is > here [2]. > [2] > https://github.com/abrestic/linux/commit/82281999409b6c5a6a51df449a1c45204a249df8 Oh, I hadn't noticed that one since it wasn't part of the posted XHCI v8 series, but in a different branch that built on it. The binding proposed there (i.e. the usb2-port and usb3-port properties) don't map correctly to the way HW represents the data. I think we need to tie the binding to how the HW works much more directly. In HW there's a single "which USB2 port is associated with this USB3 port" register. I think we need DT to have a single property (or a property per USB3 port since there's a register field per USB3 port) that represents that, rather than properties that represent which USB2/3 controller ports will be using which lanes thus requiring the driver to derive the register data by comparing those values. Also, I'm not sure the set of entries in include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h represents HW correctly; after the patch, some of TEGRA_XUSB_PADCTL_* (e.g. TEGRA_XUSB_PADCTL_PCIE) represent actual physical PHYs (which often have multiple lanes, each potentially used by various different PCIe/SATA/USB controllers), whereas some of them (e.g. TEGRA_XUSB_PADCTL_USB3_P*) represent the USB3 controllers that make use of PHYs rather than the PHYs or lanes themselves. That feels inconsistent to me. Thierry, you mentioned you had a proposal for XUSB padctl which represented things using sub-nodes. Can you please post a DT binding (or at least example DT content) for that ASAP so that all the proposed options are out on the table, and we can get to work on a final binding proposal? Thanks. -- 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 --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt new file mode 100644 index 000000000000..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 "<type>-<number>" where <type> is one of "utmi", + "hsic", or "usb3" and <number> is a 0-based index. On Tegra124, there may + be up to 3 UTMI, 2 HSIC, and 2 USB3 PHYs. + +Example: +-------- + usb-host@0,70090000 { + compatible = "nvidia,tegra124-xusb"; + reg = <0x0 0x70090000 0x0 0x8000>, + <0x0 0x70099000 0x0 0x1000>, + <0x0 0x70098000 0x0 0x1000>; + interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&tegra_car TEGRA124_CLK_XUSB_HOST>, + <&tegra_car TEGRA124_CLK_XUSB_HOST_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_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 156>, <&tegra_car 143>; + reset-names = "xusb_host", "xusb_ss", "xusb_src"; + 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-usb-ss-pll-supply = <&vdd_1v05_run>; + hvdd-usb-ss-supply = <&vdd_3v3_lp0>; + hvdd-usb-ss-pll-e-supply = <&vdd_3v3_lp0>; + };