On Thu, Sep 04, 2025 at 04:10:23PM GMT, zhangsenchuan wrote: > > > > > -----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. > If these are actual interrupts and these names align with the interrupt names in the hardware IP, then you can keep it as it is and should add a comment for inte-inth: - const: inte /* INTA_Deassert */ ... If not, then you should remove inte-inth. > > > + > > > + 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>; > Yes. > > > > > + <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. > Ok, you should also mention this in description or cover letter. - Mani -- மணிவண்ணன் சதாசிவம்
