| Message ID | 20151113161138.GA7219@ulmo |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 13/11/15 16:11, Thierry Reding wrote: > On Wed, Nov 04, 2015 at 01:54:15PM -0700, Stephen Warren wrote: >> On 11/04/2015 10:11 AM, Thierry Reding wrote: >>> From: Thierry Reding <treding@nvidia.com> >>> >>> +- mboxes: Must contain an entry for each entry in mbox-names. >>> +- mbox-names: Must include the following entries: >>> + - "xusb" >> I thought we'd decided not to use any mbox binding or drivers in XUSB now? >> See for example my proposed XUSB controller binding at: >> >> http://www.spinics.net/lists/linux-tegra/msg23922.html >> [PATCH V9] dt: add NVIDIA Tegra XUSB controller binding >> >> The idea is that the mailbox should be entirely internal to the XUSB >> controller driver, and if the receipt of a mailbox message requires any >> change in the XUSB pad controller programming, the XUSB controller driver >> should simply call the XUSB pad controller driver to perform that operation. > Okay, I think that should work, but it'll require a rather large rewrite > of... well, everything. I think Martyn was going to look into that, so I > guess I'll just drop this for now. > Unfortunately I've been shifted onto other things now, Martyn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/13/2015 09:11 AM, Thierry Reding wrote: > On Wed, Nov 04, 2015 at 01:54:15PM -0700, Stephen Warren wrote: >> On 11/04/2015 10:11 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,tegra-xusb-padctl.txt | 359 +++++++++++++++++++++ >> >> For Tegra bindings, we usually use the full compatible value as the >> filename, so I'd expect the chip number in the filename too. > > I specifically didn't do that to avoid confusion. The XUSB pad > controller was introduced on Tegra114, but patches weren't posted until > Tegra124. So nvidia,tegra114-xusb-padctl.txt would be the proper name > for this binding if we were following the conventions, but then we have > never specified that binding (though I think it would look mostly the > same as for Tegra124). > > I can of course still go for nvidia,tegra114-xusb-padctl.txt, the > content would explicitly list valid compatible strings. It's just that > none of them would match the filename. I was asking that the file be named to match the compatible flag, not the SoC version that contains the HW module. The first/oldest compatible value currently documented is Tegra124, so I'd expect that to appear in the filename. Yes, it's theoretically possible for the binding to be enhanced in the future to cover Tegra114, and at that time we'd probably want to rename the file. However, I believe the likelihood of that happening is zero (or even negative, which is admittedly mathematically impossible, but I'm being hyperbolic). >> I'd expect to see a patch in this series that edits the existing >> Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt >> and mentions that the binding is deprecated. > > How about this: > > --- >8 --- > 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. > --- >8 --- That sounds good, although I'd suggest explicitly mentioning that the binding is deprecated. Perhaps add ", and so this binding is deprecated" at the end of the first sentence? >>> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt >>> +- 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" >> >> Are there no clocks or power domains that affect XUSB padctl? I suppose we >> can start off without any, and add them later if we need. > > I don't think there are any. The TRM specifies that it's in the ungated > Vaux_soc power partition, and doesn't mention any clocks. OK. Obviously the module must used some clock, since it contains clocked logic. However, equally that clock is obviously on even without the driver explicitly managing it since the HW works right now. So, I suppose we can defer adding any clock entries to the binding/DT until later, if the need arises. It'd still be nice if we could put the complete details into the binding from day one, but I understand that the documentation isn't exactly forthcoming on these topics. >>> +- mboxes: Must contain an entry for each entry in mbox-names. >>> +- mbox-names: Must include the following entries: >>> + - "xusb" >> >> I thought we'd decided not to use any mbox binding or drivers in XUSB now? >> See for example my proposed XUSB controller binding at: >> >> http://www.spinics.net/lists/linux-tegra/msg23922.html >> [PATCH V9] dt: add NVIDIA Tegra XUSB controller binding >> >> The idea is that the mailbox should be entirely internal to the XUSB >> controller driver, and if the receipt of a mailbox message requires any >> change in the XUSB pad controller programming, the XUSB controller driver >> should simply call the XUSB pad controller driver to perform that operation. > > Okay, I think that should work, but it'll require a rather large rewrite > of... well, everything. I think Martyn was going to look into that, so I > guess I'll just drop this for now. > > Instead I think we'll need a phandle to the XUSB pad controller, so that > we can resolve it to the proper context. Something like this perhaps? > > - nvidia,padctl: phandle to the XUSB pad controller that is used > to configure the USB pads used by the XHCI controller. > > That should of course go into the XHCI controller binding. The nice > thing about this would be that we get rid of the circular dependency > XHCI -> padctl -> mailbox -> XHCI. Yes, that sounds good. >>> +Pad nodes: >>> +========== >> >>> +For Tegra124 and Tegra132, the following pads exist: utmi, ulpi, hsic, pcie >>> +and sata. No extra resources are required for operation of these pads. >> >> Judging by the diagram in the TRM (e.g. figure 41 in the Tegra124 TRM, >> figure 36 in the Tegra210 TRM), there is not a single "utmi" pad, but rather >> a separate pad per USB2 lane. However, the other types of pads are indeed >> multi-lane. The TRM also refers to "USB2" pads rather than "UTMI" pads. I >> don't see a ULPI pad in the diagram either. Assuming the diagram in the TRM >> is consistent with the register layout, I think we should have the following >> set of pad nodes: >> >> utmi-0 >> utmi-1 >> utmi-2 >> hsic >> pcie >> sata > > I think the diagram is incomplete. If you look at the register > definitions there is clearly a ULPI lane that can be muxed to SNPS or > XUSB. Further there are two registers that allow configuration of the > ULPI link. OK, I see ULPI-related fields/registers in the Tegra124 register specs. They've apparently been removed from Tegra210. > Similarly I think the UTMI pads are innacurately represented. First I > think naming them UTMI is more accurate, because USB2 could be either > UTMI or HSIC. Also there is common logic shared between all UTMI pads > which is why I think it makes sense to collect them in a separate top > level utmi pad node. > > With Tegra210 this will become more important because having the top- > level utmi pad node provides for a good location to put shared resources > in the device tree. Can you please provide more details re: which shared resources you're referring to? Looking at the implementation I have of struct tegra_xusb_phy_ops .prepare()/.enable() I'm not seeing any common logic/resources. Note that I'm making the subtle (perhaps arbitrary) distinction between logic that's global to the entire of XUSB PADCTL (but naturally affects everything contained inside it, such as the clamping and reset bits) and logic that actually binds N USB2/UTMI lanes together into some common sub-block (such as registers XUSB_PADCTL_UPHY_PLL_P0_CTL* for PCIe), rather than those lanes being bound together only at the top level of XUSB PADCTL. While I agree that "USB2" isn't good naming, it /is/ the naming used by the TRM and the register documentation. I'm not quite convinced that the DT binding is the correct place to fix this naming mistake. At the very least, can we add a note near the "utmi" names indicating that they're referred to as "USB2" in the HW documentation? ... >>> +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,port: 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 >> >> Wouldn't it be better to name the node after the physical port number, just >> like we name the PHY/lane nodes after the PHY/lane identity? We don't seem >> to have any such mapping information in the UTMI port nodes; how do we >> correlate the USB2 and USB3 ports? > > The mapping of UTMI and HSIC ports seems to be static. ... ... > The correlation of USB2 and USB3 ports is specifically done using this > nvidia,port property. OK, perhaps nvidia,usb2-companion-port-id would be a more descriptive property name then. I got the impression this property indicated which USB3 port this node contained configuration for. > The index in the USB3 port name represents the > hardware index of the port, which is used to index registers etc. Now I > suppose we could turn this around, and have the ports named after the > physical port, but then we'd still need an nvidia,port property to > relate that "physical port" to the pad controller hardware index. That > seems the wrong way around to me, since we're describing the XUSB pad > controller part of the port here. > >> I wonder if we shouldn't represent USB (physical) ports at the lane-side of >> the XUSB pad controller rather than the IO controller side. That way, we >> could pack both USB2- and USB3-related information into a single node. For >> example, vbus-supply really applies equally to the companion USB2 and USB3 >> ports, whereas this binding represents those two parts of the overall USB >> port separately, with the vbus-supply property appearing in only one of the >> two places. > > My understanding is that the USB3 ports work on top of USB2 ports. USB3 > ports don't work standalone, so they must always be associated with one > of the USB2 ports. If they aren't there is a hardware mechanism in place > that disables the USB3 port. Yes, a USB3 port must be coupled with a "companion" USB2 port. > There are further some registers that control the electrical signalling > of each of these ports, so I think it makes sense to represent this from > the perspective of the pad controller rather than the I/O controller. But at the signal interface between the XUSB controller and XUSB PADCTL module, the two are the same thing. By "port" are you referring to something other than the signals at the periphery of the XUSB PADCTL module (either directed internally for connection to the XUSB controller, or externally to the physical balls on the chip package). >> I guess this boils down to what a "port" actually is; the IO >> controller <-> XUSB pad controller interface, or the XUSB pad controller <-> >> SoC pins interface. >> >>> +For Tegra124 and Tegra132, the XUSB pad controller exposes the following >>> +ports: >>> +- 3x UTMI: utmi-0, utmi-1, utmi-2 >>> +- 1x ULPI: ulpi-0 >>> +- 2x HSIC: hsic-0, hsic-1 >>> +- 2x super-speed USB: usb3-0, usb3-1 >> >> I suspect that chunk would be better placed directly inside the "Port >> nodes:" section, since it describes valid values for the nodes that are >> subsequently described. >> >> Do we need port nodes for PCIe or SATA at all? If not, should we >> s/ports/usb-ports/ in the container node name? I suppose it doesn't matter, >> but it feels slightly odd to only represent some of the ports. > > I've seen references to "PCIe ports" and "SATA port" in documentation, > but I don't see any immediate need to include them. An early version of > the binding (and implementation) had them, but they ended up unused and > empty. I don't see any registers that would substantiate that we need > them. Then again, keeping the top-level node named "ports" will give us > the flexibility to add them if necessary with the benefit that the > parent name is still accurate, whereas naming it "usb-ports" would make > things somewhat inconsistent if we ever need PCIe and SATA ports. OK, I was expecting to add top-level "sata-ports" and "pcie-ports" nodes in the future if required, but I supposed we can indeed just add everything under "ports" instead. >>> +Examples: >>> +========= >>> + >>> +Tegra124 and Tegra132: >>> +---------------------- >> >> The example isn't valid for Tegra132 since the compatible values don't >> include any Tegra132 entry. >> >> BTW, I've suggested a lot of phrasing changes. Once we've settled the other >> questions, just let me know if you want me to propose an updated version of >> the patch that contains all those phrasing changes so you don't have to do >> them all yourself. > > Feel free to integrate whatever phrasing changes you think improve the > binding. It might be best for you to do that yourself to avoid churn due > to me misunderstanding what you were suggesting. I think I'd like to see a bit more clarity on the definition of "ports" and the description of the node structure before I suggest any more tweaks to the wording. I'm not 100% sure I understood the proposed structure fully yet. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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. --- >8 ---