On Mon, Oct 10, 2016 at 12:20 PM, Brian Norris <briannorris@xxxxxxxxxxxx> wrote: > Hi Rob, > > On Mon, Oct 10, 2016 at 09:16:39AM -0500, Rob Herring wrote: >> On Thu, Oct 06, 2016 at 04:50:00PM +0800, Shawn Lin wrote: >> > From: Brian Norris <briannorris@xxxxxxxxxxxx> >> > >> > rk3399 supports PCIe 2.x link speeds marginally at best, and on some >> > boards, the link won't train at 5 GT/s at all. Rather than sacrifice 500 >> > ms waiting for training that will never happen, let's add a property >> > from devicetree to specify link capability. >> > >> > Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx> >> > Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> >> > >> > --- >> > >> > Changes in v4: >> > - define link_gen as u32 >> > - elaborate more for rockchip,max-link-speed on doc >> > >> > Changes in v3: >> > - Cast a warning for invalid max link speed and use gen1 for it. >> > That looks better than v2. (Suggested by Brian) >> > >> > Changes in v2: >> > - rename the property to rockchip,max-link-speed according to >> > Bjorn's recommendation and take some bits from imx6q-pcie to >> > make this requirement more consisent. >> > >> > .../devicetree/bindings/pci/rockchip-pcie.txt | 4 ++ >> > drivers/pci/host/pcie-rockchip.c | 63 ++++++++++++++-------- >> > 2 files changed, 44 insertions(+), 23 deletions(-) >> > >> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt >> > index ba67b39..9bb29de 100644 >> > --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt >> > +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt >> > @@ -42,6 +42,10 @@ Required properties: >> > Optional Property: >> > - ep-gpios: contain the entry for pre-reset gpio >> > - num-lanes: number of lanes to use >> > +- rockchip,max-link-speed: Specify PCI gen for link capability. Must >> > + be '2' for gen2, and '1' for gen1, otherwise will default to gen1. >> > + For backward compatibility, if this property isn't assigned, we >> > + use gen2 by default. >> >> Defaults to gen1 or gen2? > > The wording is a bit confusing, but I think Shawn intended for: > (a) if no property is provided, default to the maximum supported (i.e., > gen2), to keep backward compatibility > (b) if a property is provided, but it doesn't contain 1 or 2, just > default to 1 > > I already disagreed with (b) (and suggested we make this an error > instead; we have no business accepting malformed device trees here IMO). > Alternatives to clear up the confusion Rob pointed out might include: > (1) explaining this better (i.e., what does "otherwise" mean in the > original sentence?) > (2) changing this to uniformly default to 2 (if someone used an > unsupported value, treat it as if the property wasn't there at all) > (3) reject incorrect values outright (so we don't have this fuzzy middle > ground at all) Sounds good. >> Let's drop rockchip and make this a common property. > > As in, handle this in some generic/common way? AFAICT, there's really > no way to do that given the current structure of PCIe handling (and the > nature of these kinds of limitations). > > Or, do you just mean make the name common? (i.e,. "max-link-speed" > instead of "rockchip,max-link-speed") Right, just the name and document in a common spot. Could be a simple helper function that drivers can call as well. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
