Re: [PATCH v3 1/5] dt-bindings: pci: Add Sophgo SG2042 PCIe host

From: Bjorn Helgaas <helgaas@kernel.org>
To: Chen Wang <unicorn_wang@outlook.com>, Rob Herring <robh+dt@kernel.org>
Cc: Chen Wang <unicornxw@gmail.com>,
	kw@linux.com, u.kleine-koenig@baylibre.com,
	aou@eecs.berkeley.edu, arnd@arndb.de, bhelgaas@google.com,
	conor+dt@kernel.org, guoren@kernel.org, inochiama@outlook.com,
	krzk+dt@kernel.org, lee@kernel.org, lpieralisi@kernel.org,
	manivannan.sadhasivam@linaro.org, palmer@dabbelt.com,
	paul.walmsley@sifive.com, pbrobinson@gmail.com,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-pci@vger.kernel.org, linux-riscv@lists.infradead.org,
	chao.wei@sophgo.com, xiaoguang.xing@sophgo.com,
	fengchun.li@sophgo.com
Subject: Re: [PATCH v3 1/5] dt-bindings: pci: Add Sophgo SG2042 PCIe host
Date: Fri, 21 Feb 2025 16:13:30 -0600	[thread overview]
Message-ID: <20250221221330.GA367172@bhelgaas> (raw)
In-Reply-To: <PN0PR01MB5662DF3C3D71A274A2E5B9C2FEC72@PN0PR01MB5662.INDPRD01.PROD.OUTLOOK.COM>

[cc->to: Rob]

On Fri, Feb 21, 2025 at 11:29:20AM +0800, Chen Wang wrote:
> On 2025/2/20 2:22, Bjorn Helgaas wrote:
> > On Wed, Feb 12, 2025 at 01:54:11PM +0800, Chen Wang wrote:
> > > On 2025/2/12 12:25, Bjorn Helgaas wrote:
> > > [......]
> > > > > pcie_rc1 and pcie_rc2 share registers in cdns_pcie1_ctrl. By using
> > > > > different "sophgo,core-id" values, they can distinguish and access
> > > > > the registers they need in cdns_pcie1_ctrl.
> > > > Where does cdns_pcie1_ctrl fit in this example?  Does that enclose
> > > > both pcie_rc1 and pcie_rc2?
> > > cdns_pcie1_ctrl is defined as a syscon node,  which contains registers
> > > shared by pcie_rc1 and pcie_rc2. In the binding yaml file, I drew a diagram
> > > to describe the relationship between them, copy here for your quick
> > > reference:
> > > 
> > > +                     +-- Core (Link0) <---> pcie_rc1  +-----------------+
> > > +                     |                                |                 |
> > > +      Cadence IP 2 --+                                | cdns_pcie1_ctrl |
> > > +                     |                                |                 |
> > > +                     +-- Core (Link1) <---> pcie_rc2  +-----------------+
> > > 
> > > The following is an example with cdns_pcie1_ctrl added. For simplicity, I
> > > deleted pcie_rc0.
> >
> > Looks good.  It would be nice if there were some naming similarity or
> > comment or other hint to connect sophgo,core-id with the syscon node.
> > 
> > > pcie_rc1: pcie@7062000000 {
> > >      compatible = "sophgo,sg2042-pcie-host";
> > >      ...... // host bride level properties
> > >      linux,pci-domain = <1>;
> > >      sophgo,core-id = <0>;
> > >      sophgo,syscon-pcie-ctrl = <&cdns_pcie1_ctrl>;
> > >      port {
> > >          // port level properties
> > >          vendor-id = <0x1f1c>;
> > >          device-id = <0x2042>;
> > >          num-lanes = <2>;
> > >      };
> > > };
> > > 
> > > pcie_rc2: pcie@7062800000 {
> > >      compatible = "sophgo,sg2042-pcie-host";
> > >      ...... // host bride level properties
> > >      linux,pci-domain = <2>;
> > >      sophgo,core-id = <1>;
> > >      sophgo,syscon-pcie-ctrl = <&cdns_pcie1_ctrl>;
> > >      port {
> > >          // port level properties
> > >          vendor-id = <0x1f1c>;
> > >          device-id = <0x2042>;
> > >          num-lanes = <2>;
> > >      }
> > > 
> > > };
> > > 
> > > cdns_pcie1_ctrl: syscon@7063800000 {
> > >      compatible = "sophgo,sg2042-pcie-ctrl", "syscon";
> > >      reg = <0x70 0x63800000 0x0 0x800000>;
> > > };
> 
> I find dtb check will report error due to "port" is not a evaulated property
> for pcie host. Should we add a vendror specific property for this?
> 
> Or do you have any example for reference?

Sorry, I don't know enough about dtb to answer this.  Maybe Rob?