On Wed, Dec 18, 2024 at 3:28 AM Havalige, Thippeswamy <thippeswamy.havalige@xxxxxxx> wrote: > > Hi Rob, > > -----Original Message----- > > From: Rob Herring <robh@xxxxxxxxxx> > > Sent: Tuesday, December 17, 2024 8:41 PM > > To: Havalige, Thippeswamy <thippeswamy.havalige@xxxxxxx> > > Cc: bhelgaas@xxxxxxxxxx; lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx; > > manivannan.sadhasivam@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; > > linux-pci@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- > > kernel@xxxxxxxxxxxxxxx; jingoohan1@xxxxxxxxx; Simek, Michal > > <michal.simek@xxxxxxx>; Gogada, Bharat Kumar > > <bharat.kumar.gogada@xxxxxxx> > > Subject: Re: [RESEND PATCH v5 2/3] dt-bindings: PCI: amd-mdb: Add AMD > > Versal2 MDB PCIe Root Port Bridge > > > > On Fri, Dec 13, 2024 at 12:10:34PM +0530, Thippeswamy Havalige wrote: > > > Add AMD Versal2 MDB (Multimedia DMA Bridge) PCIe Root Port Bridge. > > > > > > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@xxxxxxx> > > > --- > > > Changes in v2: > > > ------------- > > > - Modify patch subject. > > > - Add pcie host bridge reference. > > > - Modify filename as per compatible string. > > > - Remove standard PCI properties. > > > - Modify interrupt controller description. > > > - Indentation > > > > > > Changes in v3: > > > ------------- > > > - Modified SLCR to lower case. > > > - Add dwc schemas. > > > - Remove common properties. > > > - Move additionalProperties below properties. > > > - Remove ranges property from required properties. > > > - Drop blank line. > > > - Modify pci@ to pcie@ > > > > > > Changes in v4: > > > ------------- > > > - None. > > > > > > Changes in v5: > > > ------------- > > > - None. > > > --- > > > .../bindings/pci/amd,versal2-mdb-host.yaml | 121 ++++++++++++++++++ > > > --- > > > .../bindings/pci/amd,versal2-mdb-host.yaml | 121 ++++++++++++++++++ > > > 1 file changed, 121 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/pci/amd,versal2-mdb- > > host.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml > > b/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml > > > new file mode 100644 > > > index 000000000000..c319adeeee66 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml > > > @@ -0,0 +1,121 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/pci/amd,versal2-mdb-host.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: AMD Versal2 MDB(Multimedia DMA Bridge) Host Controller > > > + > > > +maintainers: > > > + - Thippeswamy Havalige <thippeswamy.havalige@xxxxxxx> > > > + > > > +allOf: > > > + - $ref: /schemas/pci/pci-host-bridge.yaml# > > > + - $ref: /schemas/pci/snps,dw-pcie.yaml# > > > + > > > +properties: > > > + compatible: > > > + const: amd,versal2-mdb-host > > > + > > > + reg: > > > + items: > > > + - description: MDB PCIe controller 0 SLCR > > > + - description: configuration region > > > + - description: data bus interface > > > + - description: address translation unit register > > > + > > > + reg-names: > > > + items: > > > + - const: mdb_pcie_slcr > > > + - const: config > > > + - const: dbi > > > + - const: atu > > > + > > > + ranges: > > > + maxItems: 2 > > > + > > > + msi-map: > > > + maxItems: 1 > > > + > > > + interrupts: > > > + maxItems: 1 > > > + > > > + interrupt-map-mask: > > > + items: > > > + - const: 0 > > > + - const: 0 > > > + - const: 0 > > > + - const: 7 > > > + > > > + interrupt-map: > > > + maxItems: 4 > > > + > > > + "#interrupt-cells": > > > + const: 1 > > > + > > > + interrupt-controller: > > > + description: identifies the node as an interrupt controller > > > + type: object > > > + additionalProperties: false > > > + properties: > > > + interrupt-controller: true > > > + > > > + "#address-cells": > > > + const: 0 > > > + > > > + "#interrupt-cells": > > > + const: 1 > > > + > > > + required: > > > + - interrupt-controller > > > + - "#address-cells" > > > + - "#interrupt-cells" > > > + > > > +required: > > > + - reg > > > + - reg-names > > > + - interrupts > > > + - interrupt-map > > > + - interrupt-map-mask > > > + - msi-map > > > + - "#interrupt-cells" > > > + - interrupt-controller > > > + > > > +unevaluatedProperties: false > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > + #include <dt-bindings/interrupt-controller/irq.h> > > > + > > > + soc { > > > + #address-cells = <2>; > > > + #size-cells = <2>; > > > + pcie@ed931000 { > > > + compatible = "amd,versal2-mdb-host"; > > > + reg = <0x0 0xed931000 0x0 0x2000>, > > > + <0x1000 0x100000 0x0 0xff00000>, > > > + <0x1000 0x0 0x0 0x100000>, > > > > DBI space is 1MB? Last I checked, there's less than 4KB worth of > > registers. > Thank you for your feedback. I will reduce the size to 4KB, as the DBI space primarily > uses less than 4KB for its registers. Beyond this, the space includes port logic registers, > which can be safely ignored in this context. > > The address looks odd. The config space is purely iATU configuration. > > Really, we should have described the entire address space (like the > > endpoint) available to the ATU. So the 1MB offset in the base > > address seems like just that. Most h/w design to cut down signal > > routing would put the base address for the ATU input at something > > aligned greater than the size of the address space. > In AMD (Xilinx) PCIe IPs, the configuration space for the endpoint typically starts after 1MB. > To align with this, I initially set the DBI size to 1MB. However, considering the actual utilization of less > than 4KB for DBI registers, this allocation I'll update in the next patch. > > > > > + <0x0 0xed860000 0x0 0x2000>; > > > > And then the DBI and ATU registers are nowhere near each other? > > Possible, but seems odd. > > Thank you for your feedback. The DBI address provided in the device tree serves as > one way to access the local ECAM space, which corresponds to a relative address > of 0xED840000. > > The internal IP uses the 0xED840000 address to configure all PCIe attributes. > When accessing the address 0x1000_0000_0000, PCIe internally translates and > implicitly accesses the 0xED840000 address. Using the 0xED840000 address would be more aligned with how everyone else accesses the DBI. If you support ECAM, then doesn't 0x1000_0010_0000 also correspond to the same registers in the DBI space? Though I thought only the generic PCI config space registers would be exposed there. Speaking of ECAM, if that's supported, it would be better if the driver used that (there's not any support in the driver for that). The advantage would be you could skip reconfiguring the iATU on every access. Ideally, the result would be utilizing what's in drivers/pci/ecam.c. Not something that needs to be done in this series, but you need to make sure the DT correctly describes the ECAM region (seems like it should be large enough). The hisi driver does this though it suffers from not handling devfn>0 on the root bus correctly (also a quirk in the generic ECAM based driver for DWC based implementations) and also needs 32-bit accesses to the root bus. Rob
