Re: [PATCH 1/2] dt-bindings: pci: tegra: Update for per-lane PHYs

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4634 bytes --]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > > 
> > > > > 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]