Hi Dominic,
On 14/10/22 7:11 pm, Dominic Rath wrote:
> On Thu, Oct 13, 2022 at 02:12:49PM -0500, Rob Herring wrote:
>> On Thu, Oct 13, 2022 at 08:26:47AM +0200, Dominic Rath wrote:
>>> From: Alexander Bahle <bahle@xxxxxxxxxxxxxxx>
>>>
>>> Add "cdns,tx-phy-latency-ps" and "cdns,rx-phy-latency-ps" DT bindings for
>>> setting the PCIe PHY latencies.
>>> The properties expect a list of uint32 PHY latencies in picoseconds for
>>> every supported speed starting at PCIe Gen1, e.g.:
>>>
>>> max-link-speed = <2>;
>>> tx-phy-latency-ps = <100000 200000>; /* Gen1: 100ns, Gen2: 200ns */
>>> rx-phy-latency-ps = <150000 250000>; /* Gen1: 150ns, Gen2: 250ns */
>>
>> These are a property of the PHY or PCI host? Sounds like PHY to me and
>> that should be in the PHY node. No reason the PCI driver can't go read
>> PHY node properties.
>
> I'm actually not sure if this a property of the PHY, the PCIe host, or
> of the combination of the two.
>
Latency is mostly related to propogation latency through SERDES PCS and
PMA layers.
> We thought about adding this property to the PHY, too, but we didn't
> know how to handle cases where a single PCIe host is linked with
> multiple PHYs for multi-lane configurations (see TI's AM65x for
> example). Which PHYs latency would you use to configure this PCIe RC?
On AM65x, all lanes go through SERDES of same design (but just different
instances) and thus latencies will remain same across lanes as the PCS
and PMA logics are same. So, the delays are not lane specific
>
> Personally I don't have a very strong opinion either way - we just
> didn't know any better than to put this into the PCIe host that needs
> it. If you think this is better put into the PHY node we can of course
> send a new version of this patch.
>
I don't have a preference here... Delays are dependent on PHYs being
used but something that host needs, will leave it to framework
maintainers.
> Is there any binding that specifies "generic" PCIe properties, similar
> to ethernet-phy.yaml? We couldn't find any.
>
> I guess in the AM64x case the "PHY" is serdes0_pcie_link below serdes0:
>
> &serdes0 {
> serdes0_pcie_link: phy@0 {
> ...
>
> This seems to be described by bindings/phy/phy-cadence-torrent.yaml.
>
> Should we add a generic (without cdns) tx/rx-phy-latency-ps property
> there?
>
>> If PTM is a standard PCIe thing, then I don't think these should be
>> Cadence specific. IOW, drop 'cdns'.
>
> Yes, it is a standard PCIe thing, but we haven't seen that many
> implementations yet, so we didn't want to pretend to know what this
> looks like in the generic case. We can of course drop 'cdns'.
PTM is definitely standard and vendor specific prefix don't make sense
to me.
>
> Best Regards,
>
> Dominic & Alexander
