On Thu, Apr 14, 2016 at 05:29:05PM +0200, Thierry Reding wrote: > On Wed, Apr 13, 2016 at 11:04:56AM -0600, Stephen Warren wrote: > > On 04/13/2016 10:22 AM, Thierry Reding wrote: > > > On Wed, Mar 16, 2016 at 10:51:58AM -0600, Stephen Warren wrote: > > > > On 03/08/2016 08:48 AM, Thierry Reding wrote: > > > > > From: Thierry Reding <treding@xxxxxxxxxx> > > > > > > > > > > Changes to the pad controller device tree binding have required that > > > > > each lane be associated with a separate PHY. > > > > > > > > I still don't think this has anything to do with DT bindings. Rather, the > > > > definition of a PHY (in HW and the Linux PHY subsystem) is a single lane. > > > > That fact then requires drivers to support a PHY per lane rather than a > > > > single multi-lane PHY, and equally means the DT bindings must be written > > > > according to the correct definition of a PHY. > > > > > > > > Still, I suppose the commit description is fine as is. > > > > > > I've reworded the commit message to give a more accurate rationale for > > > the change. I'll be posting a v5 soon. > > > > > > > > Update the PCI host bridge > > > > > device tree binding to allow each root port to define the list of PHYs > > > > > required to drive the lanes associated with it. > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt > > > > > > > > > +Required properties for Tegra124 and later: > > > > > +- phys: Must contain an phandle to a PHY for each entry in phy-names. > > > > > +- phy-names: Must include an entry for each active lane. Note that the number > > > > > + of entries does not have to (though usually will) be equal to the specified > > > > > + number of lanes in the nvidia,num-lanes property. Entries are of the form > > > > > + "pcie-N": where N ranges from 0 to the value specified in nvidia,num-lanes. > > > > > > > > When would the number of PHYs not equal the number of lanes? I thought the > > > > whole point of this patch was to switch to per-lane PHYs? Perhaps I'm just > > > > misremembering some exception, so there may be no need to change this. > > > > > > This is useful to support the case where we want to connect a x1 or x2 > > > device to a root port that is configured to drive more lanes. It's a > > > rather unusual configuration, but it would be possible for example to > > > have an onboard x1 ethernet card, but the board layout is such that it > > > runs in x1/x2 mode, with the ethernet card connected to the x2 port. > > > > Does the controller HW actually work correctly in such a mode? > > I think it does, and up until a few minutes ago I was even sure that I > had tested it once. But looking at the various boards that I have I > don't think I actually have test equipment that's wired the proper way > to test this. > > > Obviously a fully initialized x4 controller has to correctly handle being > > attached solely to a x1 device. However, that's a different case to simply > > not initializing 3 of the 4 PHYs. It's plausible the controller handles this > > just fine, or that it hangs up or otherwise misbehaves if some of the PHYs > > aren't enabled and hence it can't even detect whether something is attached > > to them or not. Either way, adding your explanation into the binding would > > be useful to highlight the reason for the special case. > > Perhaps for now it would be better to make the binding stricter. The > wording could be relaxed if we ever determine that it still works > correctly with a number of PHYs smaller than the number of lanes. Going over the patches again I realized that Jetson TK1 is actually one such case. The PCI host bridge controller is configured to run root port 0 using two lanes, and root port 1 using one lane. However, only one lane is connected to each port. Root port 0 goes to the miniPCIe slot, which takes a single lane (PEX4), while root port 1 goes to the onboard NIC, which takes a single lane (PEX2) as well. x1 + x1 is an unsupported configuration for the root complex, though, hence why it is configured to be x2 + x1. I've tested that both the onboard NIC as well as a miniPCIe card work correctly with the setup. So I think the wording in the binding, as well as the example, are correct. So I left the original wording in place, but instead added a couple more examples so that we have one per SoC generation, which will hopefully clarify what properties should and shouldn't be used. Thierry
Attachment:
signature.asc
Description: PGP signature
