| 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:
<BM1PR01MB4515ECD36D8FC6ECB7D161C5FE3E2@BM1PR01MB4515.INDPRD01.PROD.OUTLOOK.COM>
Date: Wed, 11 Dec 2024 17:00:44 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Bjorn Helgaas <helgaas@...nel.org>, Chen Wang <unicornxw@...il.com>
Cc: kw@...ux.com, u.kleine-koenig@...libre.com, aou@...s.berkeley.edu,
arnd@...db.de, bhelgaas@...gle.com, conor+dt@...nel.org, guoren@...nel.org,
inochiama@...look.com, krzk+dt@...nel.org, lee@...nel.org,
lpieralisi@...nel.org, manivannan.sadhasivam@...aro.org, palmer@...belt.com,
paul.walmsley@...ive.com, pbrobinson@...il.com, robh@...nel.org,
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 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@...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>;
};
}
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@...00000 {
>> + 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
>>
Powered by blists - more mailing lists