| Message ID | 05998df400a64734308e986069ca0b337618e464.1733726572.git.unicorn_wang@outlook.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add PCIe support to Sophgo SG2042 SoC | expand |
On Mon, Dec 09, 2024 at 03:19:38PM +0800, Chen Wang wrote: > Add binding for Sophgo SG2042 PCIe host controller. > + sophgo,pcie-port: This is just an index, isn't it? I don't see why it should include "sophgo" unless it encodes something sophgo-specific. > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: | > + SG2042 uses Cadence IP, every IP is composed of 2 cores(called link0 Add space before "(". More instances below. > + & link1 as Cadence's term). "sophgo,pcie-port" is used to identify which > + core/link the pcie host controller node corresponds to. s/pcie/PCIe/ for consistency in the text. More instances below. > + 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. I assume this means there are two separate Root Ports, one for Link0 and a second for Link1? > + 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. An RC doesn't have a Link. A Root Port does. > + "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. > + > + sophgo,syscon-pcie-ctrl: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + Phandle to the PCIe System Controller DT node. It's required to > + access some MSI operation registers shared by PCIe RCs. I think this probably means "shared by PCIe Root Ports", not RCs. It's unlikely that this hardware has multiple Root Complexes. > +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@62000000 { compatible = "sophgo,sg2042-pcie-host"; port0: pci@0,0 { vendor-id = <0x1f1c>; device-id = <0x2042>; }; > +additionalProperties: true > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + > + pcie@62000000 { > + compatible = "sophgo,sg2042-pcie-host"; > + device_type = "pci"; > + reg = <0x62000000 0x00800000>, > + <0x48000000 0x00001000>; > + reg-names = "reg", "cfg"; > + #address-cells = <3>; > + #size-cells = <2>; > + ranges = <0x81000000 0 0x00000000 0xde000000 0 0x00010000>, > + <0x82000000 0 0xd0400000 0xd0400000 0 0x0d000000>; > + bus-range = <0x80 0xbf>; > + vendor-id = <0x1f1c>; > + device-id = <0x2042>; > + cdns,no-bar-match-nbits = <48>; > + sophgo,pcie-port = <0>; > + sophgo,syscon-pcie-ctrl = <&cdns_pcie1_ctrl>; > + msi-parent = <&msi_pcie>; > + msi_pcie: msi { > + compatible = "sophgo,sg2042-pcie-msi"; > + msi-controller; > + interrupt-parent = <&intc>; > + interrupts = <123 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "msi"; > + }; > + }; > -- > 2.34.1 >
On 2024/12/11 1:33, Bjorn Helgaas wrote: > On Mon, Dec 09, 2024 at 03:19:38PM +0800, Chen Wang wrote: >> Add binding for Sophgo SG2042 PCIe host controller. >> + sophgo,pcie-port: > This is just an index, isn't it? I don't see why it should include > "sophgo" unless it encodes something sophgo-specific. I previously understood that if it is not a standard attribute defined by the dts schema, such as pcie-port, which is defined by me, it must be prefixed with vendor. Is that right? > >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: | >> + SG2042 uses Cadence IP, every IP is composed of 2 cores(called link0 > Add space before "(". More instances below. ok > >> + & link1 as Cadence's term). "sophgo,pcie-port" is used to identify which >> + core/link the pcie host controller node corresponds to. > s/pcie/PCIe/ for consistency in the text. More instances below. ok > >> + 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. > I assume this means there are two separate Root Ports, one for Link0 > and a second for Link1? Yes. So the naming of pcie_rcX is wrong, I will correct it, thanks. > >> + 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. > An RC doesn't have a Link. A Root Port does. > >> + "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. >> + >> + sophgo,syscon-pcie-ctrl: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + Phandle to the PCIe System Controller DT node. It's required to >> + access some MSI operation registers shared by PCIe RCs. > I think this probably means "shared by PCIe Root Ports", not RCs. > It's unlikely that this hardware has multiple Root Complexes. Ok, I will fix this. > >> +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@62000000 { > 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@7060000000 { compatible = "sophgo,sg2042-pcie-host"; ...... // other properties pci@0,0 { vendor-id = <0x1f1c>; device-id = <0x2042>; }; } pcie@7062000000 { compatible = "sophgo,sg2042-pcie-host"; ...... // other properties pci@0,0 { vendor-id = <0x1f1c>; device-id = <0x2042>; }; } pcie@7062800000 { compatible = "sophgo,sg2042-pcie-host"; ...... // other properties pci@1,0 { vendor-id = <0x1f1c>; device-id = <0x2042>; }; } And with this change, I can drop the “pcie-port”property and use the port name to figure out the port number, right? >> +additionalProperties: true >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/irq.h> >> + >> + pcie@62000000 { >> + compatible = "sophgo,sg2042-pcie-host"; >> + device_type = "pci"; >> + reg = <0x62000000 0x00800000>, >> + <0x48000000 0x00001000>; >> + reg-names = "reg", "cfg"; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + ranges = <0x81000000 0 0x00000000 0xde000000 0 0x00010000>, >> + <0x82000000 0 0xd0400000 0xd0400000 0 0x0d000000>; >> + bus-range = <0x80 0xbf>; >> + vendor-id = <0x1f1c>; >> + device-id = <0x2042>; >> + cdns,no-bar-match-nbits = <48>; >> + sophgo,pcie-port = <0>; >> + sophgo,syscon-pcie-ctrl = <&cdns_pcie1_ctrl>; >> + msi-parent = <&msi_pcie>; >> + msi_pcie: msi { >> + compatible = "sophgo,sg2042-pcie-msi"; >> + msi-controller; >> + interrupt-parent = <&intc>; >> + interrupts = <123 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-names = "msi"; >> + }; >> + }; >> -- >> 2.34.1 >>
[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@62000000 { > > 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@7060000000 { > compatible = "sophgo,sg2042-pcie-host"; > ...... // other properties > pci@0,0 { > vendor-id = <0x1f1c>; > device-id = <0x2042>; > }; > } > > pcie@7062000000 { > compatible = "sophgo,sg2042-pcie-host"; > ...... // other properties > pci@0,0 { > vendor-id = <0x1f1c>; > device-id = <0x2042>; > }; > } > > pcie@7062800000 { > 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. 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@3000 { 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. > > > +additionalProperties: true > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/interrupt-controller/irq.h> > > > + > > > + pcie@62000000 { > > > + compatible = "sophgo,sg2042-pcie-host"; > > > + device_type = "pci"; > > > + reg = <0x62000000 0x00800000>, > > > + <0x48000000 0x00001000>; > > > + reg-names = "reg", "cfg"; > > > + #address-cells = <3>; > > > + #size-cells = <2>; > > > + ranges = <0x81000000 0 0x00000000 0xde000000 0 0x00010000>, > > > + <0x82000000 0 0xd0400000 0xd0400000 0 0x0d000000>; > > > + bus-range = <0x80 0xbf>; > > > + vendor-id = <0x1f1c>; > > > + device-id = <0x2042>; > > > + cdns,no-bar-match-nbits = <48>; > > > + sophgo,pcie-port = <0>; > > > + sophgo,syscon-pcie-ctrl = <&cdns_pcie1_ctrl>; > > > + msi-parent = <&msi_pcie>; > > > + msi_pcie: msi { > > > + compatible = "sophgo,sg2042-pcie-msi"; > > > + msi-controller; > > > + interrupt-parent = <&intc>; > > > + interrupts = <123 IRQ_TYPE_LEVEL_HIGH>; > > > + interrupt-names = "msi"; > > > + }; > > > + };
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@62000000 { > > > 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@7060000000 { > > compatible = "sophgo,sg2042-pcie-host"; > > ...... // other properties > > pci@0,0 { > > vendor-id = <0x1f1c>; > > device-id = <0x2042>; > > }; > > } > > > > pcie@7062000000 { > > compatible = "sophgo,sg2042-pcie-host"; > > ...... // other properties > > pci@0,0 { > > vendor-id = <0x1f1c>; > > device-id = <0x2042>; > > }; > > } > > > > pcie@7062800000 { > > 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@3000 { > 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
hello ~ On 2024/12/11 1:33, Bjorn Helgaas wrote: > On Mon, Dec 09, 2024 at 03:19:38PM +0800, Chen Wang wrote: >> Add binding for Sophgo SG2042 PCIe host controller. >> + sophgo,pcie-port: [......] >> + 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. > I assume this means there are two separate Root Ports, one for Link0 > and a second for Link1? > >> + 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. > An RC doesn't have a Link. A Root Port does. > >> + "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. >> + >> + sophgo,syscon-pcie-ctrl: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + Phandle to the PCIe System Controller DT node. It's required to >> + access some MSI operation registers shared by PCIe RCs. > I think this probably means "shared by PCIe Root Ports", not RCs. > It's unlikely that this hardware has multiple Root Complexes. hi, Bjorn, I just double confirmed with sophgo engineers, they told me that the actual PCIe design is that there is only one root port under a host bridge. I am sorry that my original description and diagram may not make this clear, so please allow me to introduce this historical background in detail again. Please read it patiently :): The IP provided by Cadence contains two independent cores (called "links" according to the terminology of their manual, the first one is called link0 and the second one is called link1). Each core corresponds to a host bridge, and each host bridge has only one root port, and their configuration registers are completely independent. That is to say，one cadence IP encapsulates two independent host bridges. SG2042 integrates two Cadence IPs, so there can actually be up to four host bridges. Taking a Cadence IP as an example, the two host bridges can be connected to different lanes through configuration, which has been described in the original message. At present, the configuration of SG2042 is to let core0 (link0) in the first ip occupy all lanes in the ip, and let core0 (link0) and core1 (link1) in the second ip each use half of the lanes in the ip. So in the end we only use 3 cores, that's why 3 host bridge nodes are configured in dts. Because the configurations of these links are independent, the story ends here, but unfortunately, sophgo engineers defined some new register files to add support for their msi controller inside pcie. The problem is they did not separate these register files according to link0 and link1. These new register files are "cdns_pcie0_ctrl" / "cdns_pcie1_ctrl" in the original picture and dts, where the register of "cdns_pcie0_ctrl" is shared by link0 and link1 of the first ip, and "cdns_pcie1_ctrl" is shared by link0 and link1 of the second ip. According to my new description, "cdns_pcieX_ctrl" is not shared by root ports, they are shared by host bridge/rc. Because the register design of "cdns_pcieX_ctrl" is not strictly segmented according to link0 and link1, in pcie host bridge driver coding we must know whether the host bridge corresponds to link0 or link1 in the ip, so the "sophgo,link-id" attribute is introduced. Now I think it is not appropriate to change it to "sophgo,pcie-port". The reason is that as mentioned above, there is only one root port under each host bridge in the cadence ip. Link0 and link1 are actually used to distinguish the two host bridges in one ip. So I suggest to keep the original "sophgo,link-id" and with the prefix because the introduction of this attribute is indeed caused by the private design of sophgo. Any other good idea please feel free let me know. Thansk, Chen >> +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@62000000 { > compatible = "sophgo,sg2042-pcie-host"; > port0: pci@0,0 { > vendor-id = <0x1f1c>; > device-id = <0x2042>; > }; As I mentioned above, there is actually only one root port under a host bridge, so I think it is unnecessary to introduce the port subnode. In addition, I found that it is also allowed to directly add the vendor-id and device-id properties directly under the host bridge, see https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-host-bridge.yaml And refer to the dts for those products using cadence ip: arch/arm64/boot/dts/ti/k3-j721e-main.dtsi In this way, when executing lspci, the vendor id and device id will appear in the line corresponding to the pci brdge device. [......]
On Thu, Dec 19, 2024 at 10:34:50AM +0800, Chen Wang wrote: > hello ~ > > On 2024/12/11 1:33, Bjorn Helgaas wrote: > > On Mon, Dec 09, 2024 at 03:19:38PM +0800, Chen Wang wrote: > > > Add binding for Sophgo SG2042 PCIe host controller. > > > + sophgo,pcie-port: > [......] > > > + 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. > > I assume this means there are two separate Root Ports, one for Link0 > > and a second for Link1? > > > > > + 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. > > An RC doesn't have a Link. A Root Port does. > > > > > + "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. > > > + > > > + sophgo,syscon-pcie-ctrl: > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > + description: > > > + Phandle to the PCIe System Controller DT node. It's required to > > > + access some MSI operation registers shared by PCIe RCs. > > I think this probably means "shared by PCIe Root Ports", not RCs. > > It's unlikely that this hardware has multiple Root Complexes. > > hi, Bjorn, > > I just double confirmed with sophgo engineers, they told me that the actual > PCIe design is that there is only one root port under a host bridge. I am > sorry that my original description and diagram may not make this clear, so > please allow me to introduce this historical background in detail again. > Please read it patiently :): > > The IP provided by Cadence contains two independent cores (called "links" > according to the terminology of their manual, the first one is called link0 > and the second one is called link1). Each core corresponds to a host bridge, > and each host bridge has only one root port, and their configuration > registers are completely independent. That is to say，one cadence IP > encapsulates two independent host bridges. SG2042 integrates two Cadence > IPs, so there can actually be up to four host bridges. > > > Taking a Cadence IP as an example, the two host bridges can be connected to > different lanes through configuration, which has been described in the > original message. At present, the configuration of SG2042 is to let core0 > (link0) in the first ip occupy all lanes in the ip, and let core0 (link0) > and core1 (link1) in the second ip each use half of the lanes in the ip. So > in the end we only use 3 cores, that's why 3 host bridge nodes are > configured in dts. > > > Because the configurations of these links are independent, the story ends > here, but unfortunately, sophgo engineers defined some new register files to > add support for their msi controller inside pcie. The problem is they did > not separate these register files according to link0 and link1. These new > register files are "cdns_pcie0_ctrl" / "cdns_pcie1_ctrl" in the original > picture and dts, where the register of "cdns_pcie0_ctrl" is shared by link0 > and link1 of the first ip, and "cdns_pcie1_ctrl" is shared by link0 and > link1 of the second ip. According to my new description, "cdns_pcieX_ctrl" > is not shared by root ports, they are shared by host bridge/rc. > > > Because the register design of "cdns_pcieX_ctrl" is not strictly segmented > according to link0 and link1, in pcie host bridge driver coding we must know > whether the host bridge corresponds to link0 or link1 in the ip, so the > "sophgo,link-id" attribute is introduced. > > > Now I think it is not appropriate to change it to "sophgo,pcie-port". The > reason is that as mentioned above, there is only one root port under each > host bridge in the cadence ip. Link0 and link1 are actually used to > distinguish the two host bridges in one ip. > > So I suggest to keep the original "sophgo,link-id" and with the prefix > because the introduction of this attribute is indeed caused by the private > design of sophgo. > > Any other good idea please feel free let me know. > > Thansk, > > Chen > > > > +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@62000000 { > > compatible = "sophgo,sg2042-pcie-host"; > > port0: pci@0,0 { > > vendor-id = <0x1f1c>; > > device-id = <0x2042>; > > }; > As I mentioned above, there is actually only one root port under a host > bridge, so I think it is unnecessary to introduce the port subnode. It doesn't matter how many RPs there are. What matters is what are the properties associated with. > In addition, I found that it is also allowed to directly add the vendor-id > and device-id properties directly under the host bridge, see https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-host-bridge.yaml > And refer to the dts for those products using cadence ip: > arch/arm64/boot/dts/ti/k3-j721e-main.dtsi It's allowed because we are stuck things with the wrong way. That doesn't mean we should continue to do so. > In this way, when executing lspci, the vendor id and device id will appear > in the line corresponding to the pci brdge device. That's the RP though, isn't it? There is one difference in location though. If the properties are in the RP, then they should be handled by the PCI core and override what's read from the RP registers. If the properties are in the host bridge node, then the host bridge driver sets the values in some host bridge specific registers (or has a way to make read-only regs writeable) which get reflected in the RP registers. So perhaps in the host bridge is the correct place. Rob
On 2024/12/19 20:16, Rob Herring wrote: > On Thu, Dec 19, 2024 at 10:34:50AM +0800, Chen Wang wrote: >> hello ~ >> >> On 2024/12/11 1:33, Bjorn Helgaas wrote: >>> On Mon, Dec 09, 2024 at 03:19:38PM +0800, Chen Wang wrote: >>>> Add binding for Sophgo SG2042 PCIe host controller. >>>> + sophgo,pcie-port: >> [......] >>>> + 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. >>> I assume this means there are two separate Root Ports, one for Link0 >>> and a second for Link1? >>> >>>> + 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. >>> An RC doesn't have a Link. A Root Port does. >>> >>>> + "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. >>>> + >>>> + sophgo,syscon-pcie-ctrl: >>>> + $ref: /schemas/types.yaml#/definitions/phandle >>>> + description: >>>> + Phandle to the PCIe System Controller DT node. It's required to >>>> + access some MSI operation registers shared by PCIe RCs. >>> I think this probably means "shared by PCIe Root Ports", not RCs. >>> It's unlikely that this hardware has multiple Root Complexes. >> hi, Bjorn, >> >> I just double confirmed with sophgo engineers, they told me that the actual >> PCIe design is that there is only one root port under a host bridge. I am >> sorry that my original description and diagram may not make this clear, so >> please allow me to introduce this historical background in detail again. >> Please read it patiently :): >> >> The IP provided by Cadence contains two independent cores (called "links" >> according to the terminology of their manual, the first one is called link0 >> and the second one is called link1). Each core corresponds to a host bridge, >> and each host bridge has only one root port, and their configuration >> registers are completely independent. That is to say，one cadence IP >> encapsulates two independent host bridges. SG2042 integrates two Cadence >> IPs, so there can actually be up to four host bridges. >> >> >> Taking a Cadence IP as an example, the two host bridges can be connected to >> different lanes through configuration, which has been described in the >> original message. At present, the configuration of SG2042 is to let core0 >> (link0) in the first ip occupy all lanes in the ip, and let core0 (link0) >> and core1 (link1) in the second ip each use half of the lanes in the ip. So >> in the end we only use 3 cores, that's why 3 host bridge nodes are >> configured in dts. >> >> >> Because the configurations of these links are independent, the story ends >> here, but unfortunately, sophgo engineers defined some new register files to >> add support for their msi controller inside pcie. The problem is they did >> not separate these register files according to link0 and link1. These new >> register files are "cdns_pcie0_ctrl" / "cdns_pcie1_ctrl" in the original >> picture and dts, where the register of "cdns_pcie0_ctrl" is shared by link0 >> and link1 of the first ip, and "cdns_pcie1_ctrl" is shared by link0 and >> link1 of the second ip. According to my new description, "cdns_pcieX_ctrl" >> is not shared by root ports, they are shared by host bridge/rc. >> >> >> Because the register design of "cdns_pcieX_ctrl" is not strictly segmented >> according to link0 and link1, in pcie host bridge driver coding we must know >> whether the host bridge corresponds to link0 or link1 in the ip, so the >> "sophgo,link-id" attribute is introduced. >> >> >> Now I think it is not appropriate to change it to "sophgo,pcie-port". The >> reason is that as mentioned above, there is only one root port under each >> host bridge in the cadence ip. Link0 and link1 are actually used to >> distinguish the two host bridges in one ip. >> >> So I suggest to keep the original "sophgo,link-id" and with the prefix >> because the introduction of this attribute is indeed caused by the private >> design of sophgo. >> >> Any other good idea please feel free let me know. >> >> Thansk, >> >> Chen >> >>>> +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@62000000 { >>> compatible = "sophgo,sg2042-pcie-host"; >>> port0: pci@0,0 { >>> vendor-id = <0x1f1c>; >>> device-id = <0x2042>; >>> }; >> As I mentioned above, there is actually only one root port under a host >> bridge, so I think it is unnecessary to introduce the port subnode. > It doesn't matter how many RPs there are. What matters is what are the > properties associated with. > >> In addition, I found that it is also allowed to directly add the vendor-id >> and device-id properties directly under the host bridge, see https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-host-bridge.yaml >> And refer to the dts for those products using cadence ip: >> arch/arm64/boot/dts/ti/k3-j721e-main.dtsi > It's allowed because we are stuck things with the wrong way. That > doesn't mean we should continue to do so. > >> In this way, when executing lspci, the vendor id and device id will appear >> in the line corresponding to the pci brdge device. > That's the RP though, isn't it? > > There is one difference in location though. If the properties are in the > RP, then they should be handled by the PCI core and override what's read > from the RP registers. If the properties are in the host bridge node, > then the host bridge driver sets the values in some host bridge specific > registers (or has a way to make read-only regs writeable) which get > reflected in the RP registers. So perhaps in the host bridge is the > correct place. Yes, cadence host brdige will handle the regiser setting for these ids, see function cdns_pcie_host_setup() in drivers/pci/controller/cadence/pcie-cadence-host.c. So for this case, in the host bridge is the correct place. Thanks, Chen > > Rob
diff --git a/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml new file mode 100644 index 000000000000..aec31ec97092 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml @@ -0,0 +1,141 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/sophgo,sg2042-pcie-host.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Sophgo SG2042 PCIe Host (Cadence PCIe Wrapper) + +description: + Sophgo SG2042 PCIe host controller is based on the Cadence PCIe core. + +maintainers: + - Chen Wang <unicorn_wang@outlook.com> + +properties: + compatible: + const: sophgo,sg2042-pcie-host + + reg: + maxItems: 2 + + reg-names: + items: + - const: reg + - const: cfg + + vendor-id: + const: 0x1f1c + + device-id: + const: 0x2042 + + msi: + type: object + $ref: /schemas/interrupt-controller/msi-controller.yaml# + unevaluatedProperties: false + + properties: + compatible: + items: + - const: sophgo,sg2042-pcie-msi + + interrupts: + maxItems: 1 + + interrupt-names: + const: msi + + msi-parent: true + + sophgo,pcie-port: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + SG2042 uses Cadence IP, every IP is composed of 2 cores(called link0 + & link1 as Cadence's term). "sophgo,pcie-port" is used to identify which + core/link the pcie host controller node corresponds to. + + 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. + + sophgo,syscon-pcie-ctrl: + $ref: /schemas/types.yaml#/definitions/phandle + description: + Phandle to the PCIe System Controller DT node. It's required to + access some MSI operation registers shared by PCIe RCs. + +allOf: + - $ref: cdns-pcie-host.yaml# + +required: + - compatible + - reg + - reg-names + - vendor-id + - device-id + - sophgo,syscon-pcie-ctrl + - sophgo,pcie-port + +additionalProperties: true + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + + pcie@62000000 { + compatible = "sophgo,sg2042-pcie-host"; + device_type = "pci"; + reg = <0x62000000 0x00800000>, + <0x48000000 0x00001000>; + reg-names = "reg", "cfg"; + #address-cells = <3>; + #size-cells = <2>; + ranges = <0x81000000 0 0x00000000 0xde000000 0 0x00010000>, + <0x82000000 0 0xd0400000 0xd0400000 0 0x0d000000>; + bus-range = <0x80 0xbf>; + vendor-id = <0x1f1c>; + device-id = <0x2042>; + cdns,no-bar-match-nbits = <48>; + sophgo,pcie-port = <0>; + sophgo,syscon-pcie-ctrl = <&cdns_pcie1_ctrl>; + msi-parent = <&msi_pcie>; + msi_pcie: msi { + compatible = "sophgo,sg2042-pcie-msi"; + msi-controller; + interrupt-parent = <&intc>; + interrupts = <123 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "msi"; + }; + };