Messages in this thread | | | Subject | Re: [PATCH 05/10] dt-bindings: PCI: tegra: Add device tree support for T194 | From | Vidya Sagar <> | Date | Wed, 3 Apr 2019 10:59:13 +0530 |
| |
On 4/2/2019 7:50 PM, Thierry Reding wrote: > On Tue, Apr 02, 2019 at 02:46:27PM +0530, Vidya Sagar wrote: >> On 4/1/2019 8:01 PM, Thierry Reding wrote: >>> On Mon, Apr 01, 2019 at 04:48:42PM +0530, Vidya Sagar wrote: >>>> On 3/31/2019 12:12 PM, Rob Herring wrote: >>>>> On Tue, Mar 26, 2019 at 08:43:22PM +0530, Vidya Sagar wrote: > [...] >>>>>> +Optional properties: >>>>>> +- nvidia,max-speed: limits controllers max speed to this value. >>>>>> + 1 - Gen-1 (2.5 GT/s) >>>>>> + 2 - Gen-2 (5 GT/s) >>>>>> + 3 - Gen-3 (8 GT/s) >>>>>> + 4 - Gen-4 (16 GT/s) >>>>>> +- nvidia,init-speed: limits controllers init speed to this value. >>>>>> + 1 - Gen-1 (2. 5 GT/s) >>>>>> + 2 - Gen-2 (5 GT/s) >>>>>> + 3 - Gen-3 (8 GT/s) >>>>>> + 4 - Gen-4 (16 GT/s) >>>>> >>>>> Don't we have standard speed properties? >>>> Not that I'm aware of. If you come across any, please do let me know and >>>> I'll change these. >>> >>> See Documentation/devicetree/bindings/pci/pci.txt, max-link-speed. >>> There's a standard of_pci_get_max_link_speed() property that reads it >>> from device tree. >> Thanks for the pointer. I'll switch to this in the next patch. >> >>> >>>>> Why do we need 2 values? >>>> max-speed configures the controller to advertise the speed mentioned through >>>> this flag, whereas, init-speed gets the link up at this speed and software >>>> can further take the link speed to a different speed by retraining the link. >>>> This is to give flexibility to developers depending on the platform. >>> >>> This seems to me like overcomplicating things. Couldn't we do something >>> like start in the slowest mode by default and then upgrade if endpoints >>> support higher speeds? >>> >>> I'm assuming that the maximum speed is already fixed by the IP hardware >>> instantiation, so why would we want to limit it additionally? Similarly, >>> what's the use-case for setting the initial link speed to something >>> other than the lowest speed? >> You are right that maximum speed supported by hardware is fixed and through >> max-link-speed DT option, flexibility is given to limit it to the desired speed >> for a controller on a given platform. As mentioned in the documentation for max-link-speed, >> this is a strategy to avoid unnecessary operation for unsupported link speed. >> There is no real use-case as such even for setting the initial link speed, but it is >> there to give flexibility (for any debugging) to get the link up at a certain speed >> and then take it to a higher speed at a later point of time. Please note that, hardware >> as such already has the capability to take the link to maximum speed agreed by both >> upstream and downstream ports. 'nvidia,init-speed' is only to give more flexibility >> while debugging. I'm OK to remove it if this is not adding much value here. > > If this is primarily used for debugging or troubleshooting, maybe making > it a module parameter is a better choice? > > I can see how max-link-speed might be good in certain situations where a > board layout may mandate that a link speed slower than the one supported > by the hardware is used, but I can't imagine a case where the initial > link speed would have to be limited based on the hardware designe. > > Rob, do you know of any other cases where something like this is being > used? Fair enough. I'll make max-link-speed as an optional DT parameter and leave 'nvidia,init-speed' to Rob to decided whether it is OK to have it or it is not acceptable.
> >>>>>> +- nvidia,disable-aspm-states : controls advertisement of ASPM states >>>>>> + bit-0 to '1' : disables advertisement of ASPM-L0s >>>>>> + bit-1 to '1' : disables advertisement of ASPM-L1. This also disables >>>>>> + advertisement of ASPM-L1.1 and ASPM-L1.2 >>>>>> + bit-2 to '1' : disables advertisement of ASPM-L1.1 >>>>>> + bit-3 to '1' : disables advertisement of ASPM-L1.2 >>>>> >>>>> Seems like these too should be common. >>>> This flag controls the advertisement of different ASPM states by root port. >>>> Again, I'm not aware of any common method for this. >>> >>> rockchip-pcie-host.txt documents an "aspm-no-l0s" property that prevents >>> the root complex from advertising L0s. Sounds like maybe following a >>> similar scheme would be best for consistency. I think we'll also want >>> these to be non-NVIDIA specific, so drop the "nvidia," prefix and maybe >>> document them in pci.txt so that they can be more broadly used. >> Since we have ASPM-L0s, L1, L1.1 and L1.2 states, I prefer to have just one entry >> with different bit positions indicating which particular state should not be >> advertised by root port. Do you see any particular advantage to have 4 different options? >> If having one options is fine, I'll remove "nvidia," and document it in pci.txt. > > I don't care strongly either way. It's really up to Rob to decide. Rob, please let us know your comments on this also.
> > Thierry >
| |