| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20241217131002.GA1160167-robh@kernel.org>
Date: Tue, 17 Dec 2024 07:10:02 -0600
From: Rob Herring <robh@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Chen Wang <unicorn_wang@...look.com>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Chen Wang <unicornxw@...il.com>,
kw@...ux.com, u.kleine-koenig@...libre.com, aou@...s.berkeley.edu,
arnd@...db.de, bhelgaas@...gle.com, guoren@...nel.org,
inochiama@...look.com, lee@...nel.org, lpieralisi@...nel.org,
manivannan.sadhasivam@...aro.org, palmer@...belt.com,
paul.walmsley@...ive.com, pbrobinson@...il.com,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pci@...r.kernel.org, linux-riscv@...ts.infradead.org,
chao.wei@...hgo.com, xiaoguang.xing@...hgo.com,
fengchun.li@...hgo.com
Subject: Re: [PATCH v2 1/5] dt-bindings: pci: Add Sophgo SG2042 PCIe host
On Wed, Dec 11, 2024 at 01:20:14PM -0600, Bjorn Helgaas wrote:
> [cc->to: Rob, Krzysztof, Conor because I'm not a DT expert and I'd
> like their thoughts on this idea of describing Root Ports as separate
> children]
>
> On Wed, Dec 11, 2024 at 05:00:44PM +0800, Chen Wang wrote:
> > On 2024/12/11 1:33, Bjorn Helgaas wrote:
> > > On Mon, Dec 09, 2024 at 03:19:38PM +0800, Chen Wang wrote:
>
> > > > + The Cadence IP has two modes of operation, selected by a strap pin.
> > > > +
> > > > + In the single-link mode, the Cadence PCIe core instance associated
> > > > + with Link0 is connected to all the lanes and the Cadence PCIe core
> > > > + instance associated with Link1 is inactive.
> > > > +
> > > > + In the dual-link mode, the Cadence PCIe core instance associated
> > > > + with Link0 is connected to the lower half of the lanes and the
> > > > + Cadence PCIe core instance associated with Link1 is connected to
> > > > + the upper half of the lanes.
>
> > > > + SG2042 contains 2 Cadence IPs and configures the Cores as below:
> > > > +
> > > > + +-- Core(Link0) <---> pcie_rc0 +-----------------+
> > > > + | | |
> > > > + Cadence IP 1 --+ | cdns_pcie0_ctrl |
> > > > + | | |
> > > > + +-- Core(Link1) <---> disabled +-----------------+
> > > > +
> > > > + +-- Core(Link0) <---> pcie_rc1 +-----------------+
> > > > + | | |
> > > > + Cadence IP 2 --+ | cdns_pcie1_ctrl |
> > > > + | | |
> > > > + +-- Core(Link1) <---> pcie_rc2 +-----------------+
> > > > +
> > > > + pcie_rcX is pcie node ("sophgo,sg2042-pcie-host") defined in DTS.
> > > > + cdns_pcie0_ctrl is syscon node ("sophgo,sg2042-pcie-ctrl") defined in DTS
> > > > +
> > > > + cdns_pcieX_ctrl contains some registers shared by pcie_rcX, even two
> > > > + RC(Link)s may share different bits of the same register. For example,
> > > > + cdns_pcie1_ctrl contains registers shared by link0 & link1 for Cadence IP 2.
>
> > > > + "sophgo,pcie-port" is defined to flag which core(link) the rc maps to, with
> > > > + this we can know what registers(bits) we should use.
>
> > > > +required:
> > > > + - compatible
> > > > + - reg
> > > > + - reg-names
> > > > + - vendor-id
> > > > + - device-id
> > > > + - sophgo,syscon-pcie-ctrl
> > > > + - sophgo,pcie-port
> > >
> > > It looks like vendor-id and device-id apply to PCI devices, i.e.,
> > > things that will show up in lspci, I assume Root Ports in this case.
> > > Can we make this explicit in the DT, e.g., something like this?
> > >
> > > pcie@...00000 {
> > > compatible = "sophgo,sg2042-pcie-host";
> > > port0: pci@0,0 {
> > > vendor-id = <0x1f1c>;
> > > device-id = <0x2042>;
> > > };
> >
> > Sorry, I don't understand your meaning very well. Referring to the topology
> > diagram I drew above, is it okay to write DTS as follows?
> >
> > pcie@...0000000 {
> > compatible = "sophgo,sg2042-pcie-host";
> > ...... // other properties
> > pci@0,0 {
> > vendor-id = <0x1f1c>;
> > device-id = <0x2042>;
> > };
> > }
> >
> > pcie@...2000000 {
> > compatible = "sophgo,sg2042-pcie-host";
> > ...... // other properties
> > pci@0,0 {
> > vendor-id = <0x1f1c>;
> > device-id = <0x2042>;
> > };
> > }
> >
> > pcie@...2800000 {
> > compatible = "sophgo,sg2042-pcie-host";
> > ...... // other properties
> > pci@1,0 {
> > vendor-id = <0x1f1c>;
> > device-id = <0x2042>;
> > };
> >
> > }
>
> Generally makes sense to me. I'm suggesting that we should start
> describing Root Ports as children of the host bridge node instead of
> mixing their properties into the host bridge itself.
>
> Some properties apply to the host bridge, e.g., "bus-range" describes
> the bus number aperture, and "ranges" describes the address
> translation between the upstream CPU address space and the PCI address
> space.
>
> Others apply specifically to a Root Port, e.g., "num-lanes",
> "max-link-speed", "phys", "vendor-id", "device-id". I think it will
> help if we can describe these in separate children, especially when
> there are multiple Root Ports.
Agreed.
> Documentation/devicetree/bindings/pci/pci.txt says a Root Port
> should include a reg property that contains the bus/device/function
> number of the RP, e.g.,
> Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt has
> this:
>
> pcie-controller@...0 {
> compatible = "nvidia,tegra30-pcie";
> pci@1,0 {
> reg = <0x000800 0 0 0 0>;
> };
>
> where the "0x000800 0 0 0 0" means the "pci@1,0" Root Port is at
> 00:01.0 (bus 00, device 01, function 0). I don't know what the "@1,0"
> part means.
>
> > And with this change, I can drop the “pcie-port”property and use the port
> > name to figure out the port number, right?
>
> Seems likely to me.
I don't think device 1 would be the correct address. The RP is almost
always device 0.
I think instead the 'syscon-pcie-ctrl' should perhaps be modeled as a
phy with the phy binding. Then the host bridge node can have 1 or 2 phy
entries depending on if the host uses 1 or 2 links. And the 2nd host
should have 'status = "disabled";' when it is not used.
Or perhaps just 'num-lanes' would be enough.
Rob
Powered by blists - more mailing lists