MIME-Version: 1.0
References: <20221013062649.303184-1-dominic.rath@ibv-augsburg.de>
 <20221013062649.303184-2-dominic.rath@ibv-augsburg.de> <20221013191249.GA38183-robh@kernel.org>
 <20221014134114.GA307620@JADEVM-DRA> <5d049f9c-f86e-990d-6e81-168606172d63@ti.com>
In-Reply-To: <5d049f9c-f86e-990d-6e81-168606172d63@ti.com>
From:   Christian Gmeiner <christian.gmeiner@gmail.com>
Date:   Tue, 21 Mar 2023 09:42:36 +0100
Message-ID: <CAH9NwWfLLvVG4=cxCriYjMpXHYw5OhX3rzbSXO0o5Ji3isYtXg@mail.gmail.com>
Subject: Re: [PATCH 1/3] dt-bindings: PCI: cdns: Add PHY latency properties
To:     Vignesh Raghavendra <vigneshr@ti.com>
Cc:     Dominic Rath <dominic.rath@ibv-augsburg.net>,
        Rob Herring <robh@kernel.org>,
        krzysztof.kozlowski+dt@linaro.org, tjoseph@cadence.com,
        bhelgaas@google.com, lpieralisi@kernel.org, nm@ti.com,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-pci@vger.kernel.org, Alexander Bahle <bahle@ibv-augsburg.de>,
        Dominic Rath <rath@ibv-augsburg.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Dominic,

> >>>
> >>> 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.
>

Is there any chance you can send a revisited patch series or is there
anything missing for
you to continue?

-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy