> -----Original Messages----- > From: "Manivannan Sadhasivam" <mani@xxxxxxxxxx> > Send time:Monday, 01/09/2025 14:04:50 > To: zhangsenchuan@xxxxxxxxxxxxxxxxxx > Cc: bhelgaas@xxxxxxxxxx, lpieralisi@xxxxxxxxxx, kwilczynski@xxxxxxxxxx, robh@xxxxxxxxxx, krzk+dt@xxxxxxxxxx, conor+dt@xxxxxxxxxx, linux-pci@xxxxxxxxxxxxxxx, devicetree@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, p.zabel@xxxxxxxxxxxxxx, johan+linaro@xxxxxxxxxx, quic_schintav@xxxxxxxxxxx, shradha.t@xxxxxxxxxxx, cassel@xxxxxxxxxx, thippeswamy.havalige@xxxxxxx, mayank.rana@xxxxxxxxxxxxxxxx, inochiama@xxxxxxxxx, ningyu@xxxxxxxxxxxxxxxxxx, linmin@xxxxxxxxxxxxxxxxxx, pinkesh.vaghela@xxxxxxxxxxxxxx > Subject: Re: [PATCH v2 1/2] dt-bindings: PCI: eic7700: Add Eswin eic7700 PCIe host controller > > On Fri, Aug 29, 2025 at 04:22:37PM GMT, zhangsenchuan@xxxxxxxxxxxxxxxxxx wrote: > > From: Senchuan Zhang <zhangsenchuan@xxxxxxxxxxxxxxxxxx> > > > > Add Device Tree binding documentation for the ESWIN EIC7700 > > PCIe controller module,the PCIe controller enables the core > > to correctly initialize and manage the PCIe bus and connected > > devices. > > > > Signed-off-by: Yu Ning <ningyu@xxxxxxxxxxxxxxxxxx> > > Signed-off-by: Senchuan Zhang <zhangsenchuan@xxxxxxxxxxxxxxxxxx> > > --- > > .../bindings/pci/eswin,eic7700-pcie.yaml | 142 ++++++++++++++++++ > > 1 file changed, 142 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pci/eswin,eic7700-pcie.yaml > > > > diff --git a/Documentation/devicetree/bindings/pci/eswin,eic7700-pcie.yaml b/Documentation/devicetree/bindings/pci/eswin,eic7700-pcie.yaml > > new file mode 100644 > > index 000000000000..65f640902b11 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pci/eswin,eic7700-pcie.yaml > > @@ -0,0 +1,142 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pci/eswin,eic7700-pcie.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Eswin EIC7700 PCIe host controller > > + > > +maintainers: > > + - Yu Ning <ningyu@xxxxxxxxxxxxxxxxxx> > > + - Senchuan Zhang <zhangsenchuan@xxxxxxxxxxxxxxxxxx> > > + > > +description: > > + The PCIe controller on EIC7700 SoC. > > + > > +allOf: > > + - $ref: /schemas/pci/pci-host-bridge.yaml# > > + > > +properties: > > + compatible: > > + const: eswin,eic7700-pcie > > + > > + reg: > > + maxItems: 3 > > + > > + reg-names: > > + items: > > + - const: dbi > > + - const: config > > + - const: mgmt > > + > > + ranges: > > + maxItems: 3 > > + > > + num-lanes: > > + const: 4 > > + > > + '#interrupt-cells': > > + const: 1 > > + > > + interrupts: > > + maxItems: 9 > > + > > + interrupt-names: > > + items: > > + - const: msi > > + - const: inta > > + - const: intb > > + - const: intc > > + - const: intd > > + - const: inte > > + - const: intf > > + - const: intg > > + - const: inth > > What? Are these standard INTx or something elese? PCI(e) spec defines only > INT{A-D}. > Dear Manivannan Thank you for your thorough review . You are right, the PCI(e) spec defines only four legacy INTx interrupts (INTA#, INTB#, INTC#, INTD#). PCI(e) spec defines also mentions that INTX interrupts have two control states (Assert_INTx/Deassert_INTx Message). In our yaml, inta~intd corresponds to Assert_INTA~Assert_INTD, and inte~inth corresponds to Deassert_INTA~Deassert_INTD. May I ask if inte~inth needs to be removed or if the naming needs to be standardized? I saw that in "sifive,fu740-pcie.yaml", interrupt-names only retain inta to intd. > > + > > + interrupt-map: > > + maxItems: 4 > > + > > + interrupt-map-mask: > > + items: > > + - const: 0 > > + - const: 0 > > + - const: 0 > > + - const: 7 > > + > > + clocks: > > + maxItems: 4 > > + > > + clock-names: > > + items: > > + - const: mstr > > + - const: dbi > > + - const: pclk > > + - const: aux > > + > > + resets: > > + maxItems: 3 > > + > > + reset-names: > > + items: > > + - const: cfg > > + - const: powerup > > + - const: pwren > > + > > +required: > > + - compatible > > + - reg > > + - ranges > > + - num-lanes > > + - interrupts > > + - interrupt-names > > + - interrupt-map-mask > > + - interrupt-map > > + - '#interrupt-cells' > > + - clocks > > + - clock-names > > + - resets > > + - reset-names > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + soc { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + > > + pcie@54000000 { > > + compatible = "eswin,eic7700-pcie"; > > + reg = <0x0 0x54000000 0x0 0x4000000>, > > + <0x0 0x40000000 0x0 0x800000>, > > + <0x0 0x50000000 0x0 0x100000>; > > + reg-names = "dbi", "config", "mgmt"; > > + #address-cells = <3>; > > + #size-cells = <2>; > > + #interrupt-cells = <1>; > > + ranges = <0x81000000 0x0 0x40800000 0x0 0x40800000 0x0 0x800000>, > > I/O CPU range starts from 0x0 > > Also, I don't think you need to set the relocatable flag (bit 31) for any > regions. if cannot set the relocatable flag (bit 31) for any regions.Is it appropriate to write it this way： ranges = <0x01000000 0x0 0x40800000 0x0 0x40800000 0x0 0x800000>, <0x02000000 0x0 0x41000000 0x0 0x41000000 0x0 0xf000000>, <0x43000000 0x80 0x00000000 0x80 0x00000000 0x2 0x00000000>; > > > + <0x82000000 0x0 0x41000000 0x0 0x41000000 0x0 0xf000000>, > > + <0xc3000000 0x80 0x00000000 0x80 0x00000000 0x2 0x00000000>; > > + bus-range = <0x0 0xff>; > > + clocks = <&clock 562>, > > + <&clock 563>, > > + <&clock 564>, > > + <&clock 565>; > > Don't you have clock definitions for these values? > Our clock and reset drivers have the definitions of these values, but the clock and reset drivers are under review. Currently, these values can only be replaced by constants. > > + clock-names = "mstr", "dbi", "pclk", "aux"; > > + resets = <&reset 8 (1 << 0)>, > > + <&reset 8 (1 << 1)>, > > + <&reset 8 (1 << 2)>; > > Same here. > > > + reset-names = "cfg", "powerup", "pwren"; > > + interrupts = <220>, <179>, <180>, <181>, <182>, <183>, <184>, <185>, <186>; > > + interrupt-names = "msi", "inta", "intb", "intc", "intd", > > + "inte", "intf", "intg", "inth"; > > + interrupt-parent = <&plic>; > > + interrupt-map-mask = <0x0 0x0 0x0 0x7>; > > + interrupt-map = <0x0 0x0 0x0 0x1 &plic 179>, > > + <0x0 0x0 0x0 0x2 &plic 180>, > > + <0x0 0x0 0x0 0x3 &plic 181>, > > + <0x0 0x0 0x0 0x4 &plic 182>; > > + device_type = "pci"; > > + num-lanes = <0x4>; > > nit: Most of the bindings define num-lanes as decimal. > Best Regards, Senchuan Zhang
