On Wed, Feb 12, 2025 at 09:50:11AM +0800, Chen Wang wrote:
> On 2025/2/12 7:34, Bjorn Helgaas wrote:
> > On Sun, Jan 26, 2025 at 10:27:27AM +0800, Chen Wang wrote:
> > > On 2025/1/23 6:21, Bjorn Helgaas wrote:
> > > > On Wed, Jan 15, 2025 at 03:06:37PM +0800, Chen Wang wrote:
> > > > > From: Chen Wang <unicorn_wang@xxxxxxxxxxx>
> > > > >
> > > > > Add binding for Sophgo SG2042 PCIe host controller.
> ...
> > > > "sophgo,link-id" corresponds to Cadence documentation, but I
> > > > think it is somewhat misleading in the binding because a PCIe
> > > > "Link" refers to the downstream side of a Root Port. If we
> > > > use "link-id" to identify either Core0 or Core1 of a Cadence
> > > > IP, it sort of bakes in the idea that there can never be more
> > > > than one Root Port per Core.
> > >
> > > The fact is that for the cadence IP used by sg2042, only one
> > > root port is supported per core.
> >
> > 1) That's true today but may not be true forever.
> >
> > 2) Even if there's only one root port forever, "link" already
> > means something specific in PCIe, and this usage means something
> > different, so it's a little confusing. Maybe a comment to say
> > that this refers to a "Core", not a PCIe link, is the best we can
> > do.
>
> How about using "sophgo,core-id", as I said in the binding
> description, "every IP is composed of 2 cores (called link0 & link1
> as Cadence's term).". This avoids the conflict with the concept
> "link " in the PCIe specification, what do you think?
I think that would be great.
> > > Based on the above analysis, I think the introduction of a
> > > three-layer structure (pcie-core-port) looks a bit too
> > > complicated for candence IP. In fact, the source of the
> > > discussion at the beginning of this issue was whether some
> > > attributes should be placed under the host bridge or the root
> > > port. I suggest that adding the root port layer on the basis of
> > > the existing patch may be enough. What do you think?
> > >
> > > e.g.,
> > >
> > > pcie_rc0: pcie@7060000000 {
> > > compatible = "sophgo,sg2042-pcie-host";
> > > ...... // host bride level properties
> > > sophgo,link-id = <0>;
> > > port {
> > > // port level properties
> > > vendor-id = <0x1f1c>;
> > > device-id = <0x2042>;
> > > num-lanes = <4>;
> > > }
> > > };
> > >
> > > pcie_rc1: pcie@7062000000 {
> > > compatible = "sophgo,sg2042-pcie-host";
> > > ...... // host bride level properties
> > > sophgo,link-id = <0>;
> > > 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
> > > sophgo,link-id = <0>;
> > > port {
> > > // port level properties
> > > vendor-id = <0x1f1c>;
> > > device-id = <0x2042>;
> > > num-lanes = <2>;
> > > }
> > > };
> >
> > Where does linux,pci-domain go?
> >
> > Can you show how link-id 0 and link-id 1 would both be used? I
> > assume they need to be connected somehow, since IIUC there's some
> > register shared between them?
>
> Oh, sorry, I made a typo when I was giving the example. I wrote all
> the link-id values as 0. I rewrote it as follows. I
> changed "sophgo,link-id" to "sophgo,core-id", and added
> "linux,pci-domain".
>
> e.g.,
>
> pcie_rc0: pcie@7060000000 {
>
> compatible = "sophgo,sg2042-pcie-host";
> ...... // host bride level properties
> linux,pci-domain = <0>;
> sophgo,core-id = <0>;
> port {
> // port level properties
> vendor-id = <0x1f1c>;
> device-id = <0x2042>;
> num-lanes = <4>;
> }
> };
>
> pcie_rc1: pcie@7062000000 {
> compatible = "sophgo,sg2042-pcie-host";
> ...... // host bride level properties
> linux,pci-domain = <1>;
> sophgo,core-id = <0>;
> 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>;
> port {
> // port level properties
> vendor-id = <0x1f1c>;
> device-id = <0x2042>;
> num-lanes = <2>;
> }
>
> };
>
> 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?
