| Message ID | 20250127173550.1222427-4-matthew.gerlach@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add PCIe Root Port support for Agilex family of chips | expand |
On 27/01/2025 18:35, Matthew Gerlach wrote: > Add the base device tree for support of the PCIe Root Port > for the Agilex family of chips. > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > --- > v3: > - Remove accepted patches from patch set. > > v2: > - Rename node to fix schema check error. > --- > .../intel/socfpga_agilex_pcie_root_port.dtsi | 55 +++++++++++++++++++ > 1 file changed, 55 insertions(+) > create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi > > diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi > new file mode 100644 > index 000000000000..50f131f5791b > --- /dev/null > +++ b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi > @@ -0,0 +1,55 @@ > +// SPDX-License-Identifier: GPL-2.0 Odd spaces in SPDX tag. > +/* > + * Copyright (C) 2024, Intel Corporation > + */ > +&soc0 { > + aglx_hps_bridges: fpga-bus@80000000 { > + compatible = "simple-bus"; > + reg = <0x80000000 0x20200000>, > + <0xf9000000 0x00100000>; > + reg-names = "axi_h2f", "axi_h2f_lw"; Where is this binding defined? > + #address-cells = <0x2>; > + #size-cells = <0x1>; These two are not hex. > + ranges = <0x00000000 0x00000000 0x80000000 0x00040000>, > + <0x00000000 0x10000000 0x90100000 0x0ff00000>, > + <0x00000000 0x20000000 0xa0000000 0x00200000>, > + <0x00000001 0x00010000 0xf9010000 0x00008000>, > + <0x00000001 0x00018000 0xf9018000 0x00000080>, > + <0x00000001 0x00018080 0xf9018080 0x00000010>; > + > + pcie_0_pcie_aglx: pcie@200000000 { > + reg = <0x00000000 0x10000000 0x10000000>, > + <0x00000001 0x00010000 0x00008000>, > + <0x00000000 0x20000000 0x00200000>; > + reg-names = "Txs", "Cra", "Hip"; Where is this binding defined? > + interrupt-parent = <&intc>; > + interrupts = <GIC_SPI 0x14 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-controller; > + #interrupt-cells = <0x1>; > + device_type = "pci"; > + bus-range = <0x0000000 0x000000ff>; > + ranges = <0x82000000 0x00000000 0x00100000 0x00000000 0x10000000 0x00000000 0x0ff00000>; > + msi-parent = <&pcie_0_msi_irq>; > + #address-cells = <0x3>; > + #size-cells = <0x2>; Same problem for all cells. > + interrupt-map-mask = <0x0 0x0 0x0 0x7>; > + interrupt-map = <0x0 0x0 0x0 0x1 &pcie_0_pcie_aglx 0 0 0 0x1>, > + <0x0 0x0 0x0 0x2 &pcie_0_pcie_aglx 0 0 0 0x2>, > + <0x0 0x0 0x0 0x3 &pcie_0_pcie_aglx 0 0 0 0x3>, > + <0x0 0x0 0x0 0x4 &pcie_0_pcie_aglx 0 0 0 0x4>; > + status = "disabled"; > + }; > + > + pcie_0_msi_irq: msi@10008080 { > + compatible = "altr,msi-1.0"; > + reg = <0x00000001 0x00018080 0x00000010>, > + <0x00000001 0x00018000 0x00000080>; > + reg-names = "csr", "vector_slave"; > + interrupt-parent = <&intc>; > + interrupts = <GIC_SPI 0x13 IRQ_TYPE_LEVEL_HIGH>; > + msi-controller; > + num-vectors = <0x20>; That's decimal. Value is for humans and we count numbers in decimal. Best regards, Krzysztof
On Wed, 29 Jan 2025, Krzysztof Kozlowski wrote: > On 27/01/2025 18:35, Matthew Gerlach wrote: >> Add the base device tree for support of the PCIe Root Port >> for the Agilex family of chips. >> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> --- >> v3: >> - Remove accepted patches from patch set. >> >> v2: >> - Rename node to fix schema check error. >> --- >> .../intel/socfpga_agilex_pcie_root_port.dtsi | 55 +++++++++++++++++++ >> 1 file changed, 55 insertions(+) >> create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi >> >> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi >> new file mode 100644 >> index 000000000000..50f131f5791b >> --- /dev/null >> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi >> @@ -0,0 +1,55 @@ >> +// SPDX-License-Identifier: GPL-2.0 > > Odd spaces in SPDX tag. Yes, there should only be one space. > >> +/* >> + * Copyright (C) 2024, Intel Corporation >> + */ >> +&soc0 { >> + aglx_hps_bridges: fpga-bus@80000000 { >> + compatible = "simple-bus"; >> + reg = <0x80000000 0x20200000>, >> + <0xf9000000 0x00100000>; >> + reg-names = "axi_h2f", "axi_h2f_lw"; > > Where is this binding defined? The bindings for these reg-names are not currently defined anywhere, but they are also referenced in the following: Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts I am not exactly sure where the right place is to define them, maybe Documentation/devicetree/bindings/arm/intel,socfpga.yaml. On the other hand, no code references these names; so it might make sense to just remove them. > >> + #address-cells = <0x2>; >> + #size-cells = <0x1>; > > These two are not hex. I will change all #address-cells and #size-cell to decimal. > >> + ranges = <0x00000000 0x00000000 0x80000000 0x00040000>, >> + <0x00000000 0x10000000 0x90100000 0x0ff00000>, >> + <0x00000000 0x20000000 0xa0000000 0x00200000>, >> + <0x00000001 0x00010000 0xf9010000 0x00008000>, >> + <0x00000001 0x00018000 0xf9018000 0x00000080>, >> + <0x00000001 0x00018080 0xf9018080 0x00000010>; >> + >> + pcie_0_pcie_aglx: pcie@200000000 { >> + reg = <0x00000000 0x10000000 0x10000000>, >> + <0x00000001 0x00010000 0x00008000>, >> + <0x00000000 0x20000000 0x00200000>; >> + reg-names = "Txs", "Cra", "Hip"; > > Where is this binding defined? Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml > > >> + interrupt-parent = <&intc>; >> + interrupts = <GIC_SPI 0x14 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-controller; >> + #interrupt-cells = <0x1>; >> + device_type = "pci"; >> + bus-range = <0x0000000 0x000000ff>; >> + ranges = <0x82000000 0x00000000 0x00100000 0x00000000 0x10000000 0x00000000 0x0ff00000>; >> + msi-parent = <&pcie_0_msi_irq>; >> + #address-cells = <0x3>; >> + #size-cells = <0x2>; > > Same problem for all cells. > >> + interrupt-map-mask = <0x0 0x0 0x0 0x7>; >> + interrupt-map = <0x0 0x0 0x0 0x1 &pcie_0_pcie_aglx 0 0 0 0x1>, >> + <0x0 0x0 0x0 0x2 &pcie_0_pcie_aglx 0 0 0 0x2>, >> + <0x0 0x0 0x0 0x3 &pcie_0_pcie_aglx 0 0 0 0x3>, >> + <0x0 0x0 0x0 0x4 &pcie_0_pcie_aglx 0 0 0 0x4>; >> + status = "disabled"; >> + }; >> + >> + pcie_0_msi_irq: msi@10008080 { >> + compatible = "altr,msi-1.0"; >> + reg = <0x00000001 0x00018080 0x00000010>, >> + <0x00000001 0x00018000 0x00000080>; >> + reg-names = "csr", "vector_slave"; >> + interrupt-parent = <&intc>; >> + interrupts = <GIC_SPI 0x13 IRQ_TYPE_LEVEL_HIGH>; >> + msi-controller; >> + num-vectors = <0x20>; > > That's decimal. Value is for humans and we count numbers in decimal. I will change num-vectors to decimal. Thanks for the review, Matthew Gerlach > > > > Best regards, > Krzysztof >
On Mon, Jan 27, 2025 at 11:35:48AM -0600, Matthew Gerlach wrote: > Add the base device tree for support of the PCIe Root Port > for the Agilex family of chips. > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > --- > v3: > - Remove accepted patches from patch set. > > v2: > - Rename node to fix schema check error. > --- > .../intel/socfpga_agilex_pcie_root_port.dtsi | 55 +++++++++++++++++++ > 1 file changed, 55 insertions(+) > create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi > > diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi > new file mode 100644 > index 000000000000..50f131f5791b > --- /dev/null > +++ b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi > @@ -0,0 +1,55 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2024, Intel Corporation > + */ > +&soc0 { > + aglx_hps_bridges: fpga-bus@80000000 { > + compatible = "simple-bus"; > + reg = <0x80000000 0x20200000>, > + <0xf9000000 0x00100000>; > + reg-names = "axi_h2f", "axi_h2f_lw"; > + #address-cells = <0x2>; > + #size-cells = <0x1>; > + ranges = <0x00000000 0x00000000 0x80000000 0x00040000>, > + <0x00000000 0x10000000 0x90100000 0x0ff00000>, > + <0x00000000 0x20000000 0xa0000000 0x00200000>, > + <0x00000001 0x00010000 0xf9010000 0x00008000>, > + <0x00000001 0x00018000 0xf9018000 0x00000080>, > + <0x00000001 0x00018080 0xf9018080 0x00000010>; > + > + pcie_0_pcie_aglx: pcie@200000000 { > + reg = <0x00000000 0x10000000 0x10000000>, > + <0x00000001 0x00010000 0x00008000>, > + <0x00000000 0x20000000 0x00200000>; > + reg-names = "Txs", "Cra", "Hip"; > + interrupt-parent = <&intc>; > + interrupts = <GIC_SPI 0x14 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-controller; > + #interrupt-cells = <0x1>; > + device_type = "pci"; > + bus-range = <0x0000000 0x000000ff>; > + ranges = <0x82000000 0x00000000 0x00100000 0x00000000 0x10000000 0x00000000 0x0ff00000>; This convert to pci address 0x0010_0000..0x1000_0000 from parent bus address 0x1000_0000..0x1ff0_0000 aglx_hps_bridges bridge convert 0x90100000..0xa000_0000 (cpu address) to 0x1000_0000..0x1ff0_0000 according to ranges[1](second entry). Just want to confirm that "0x1000_0000..0x1ff0_0000" is actually reflect hardware behavior. On going a thread https://lore.kernel.org/linux-pci/Z5pfiJyXB3NtGSfe@lizhi-Precision-Tower-5810/T/#t Try to clean up all cpu_addr_fixup() or similar fixup() in pci root complex drivers, but which require dtsi reflect the real hardware behavior. In most current pci driver, even "0x1000_0000..0x1ff0_0000" is wrong, it still work by drivers' fixup. If dts correct descript hardware, these fixup can be removed. best regards Frank > + msi-parent = <&pcie_0_msi_irq>; > + #address-cells = <0x3>; > + #size-cells = <0x2>; > + interrupt-map-mask = <0x0 0x0 0x0 0x7>; > + interrupt-map = <0x0 0x0 0x0 0x1 &pcie_0_pcie_aglx 0 0 0 0x1>, > + <0x0 0x0 0x0 0x2 &pcie_0_pcie_aglx 0 0 0 0x2>, > + <0x0 0x0 0x0 0x3 &pcie_0_pcie_aglx 0 0 0 0x3>, > + <0x0 0x0 0x0 0x4 &pcie_0_pcie_aglx 0 0 0 0x4>; > + status = "disabled"; > + }; > + > + pcie_0_msi_irq: msi@10008080 { > + compatible = "altr,msi-1.0"; > + reg = <0x00000001 0x00018080 0x00000010>, > + <0x00000001 0x00018000 0x00000080>; > + reg-names = "csr", "vector_slave"; > + interrupt-parent = <&intc>; > + interrupts = <GIC_SPI 0x13 IRQ_TYPE_LEVEL_HIGH>; > + msi-controller; > + num-vectors = <0x20>; > + status = "disabled"; > + }; > + }; > +}; > -- > 2.34.1 >
On 29/01/2025 20:42, matthew.gerlach@linux.intel.com wrote: > > > On Wed, 29 Jan 2025, Krzysztof Kozlowski wrote: > >> On 27/01/2025 18:35, Matthew Gerlach wrote: >>> Add the base device tree for support of the PCIe Root Port >>> for the Agilex family of chips. >>> >>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>> --- >>> v3: >>> - Remove accepted patches from patch set. >>> >>> v2: >>> - Rename node to fix schema check error. >>> --- >>> .../intel/socfpga_agilex_pcie_root_port.dtsi | 55 +++++++++++++++++++ >>> 1 file changed, 55 insertions(+) >>> create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi >>> >>> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi >>> new file mode 100644 >>> index 000000000000..50f131f5791b >>> --- /dev/null >>> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi >>> @@ -0,0 +1,55 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >> >> Odd spaces in SPDX tag. > > Yes, there should only be one space. > >> >>> +/* >>> + * Copyright (C) 2024, Intel Corporation >>> + */ >>> +&soc0 { >>> + aglx_hps_bridges: fpga-bus@80000000 { >>> + compatible = "simple-bus"; >>> + reg = <0x80000000 0x20200000>, >>> + <0xf9000000 0x00100000>; >>> + reg-names = "axi_h2f", "axi_h2f_lw"; >> >> Where is this binding defined? > > The bindings for these reg-names are not currently defined anywhere, but Then you cannot use them. > they are also referenced in the following: > Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml > arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts > I am not exactly sure where the right place is to define them, maybe > Documentation/devicetree/bindings/arm/intel,socfpga.yaml. On the other > hand, no code references these names; so it might make sense to just > remove them. In general: nowhere, because simple bus does not have such properties. It's not about reg-names only - you cannot have reg. You just did not define here simple-bus. Best regards, Krzysztof
On Wed, 29 Jan 2025, Frank Li wrote: > On Mon, Jan 27, 2025 at 11:35:48AM -0600, Matthew Gerlach wrote: >> Add the base device tree for support of the PCIe Root Port >> for the Agilex family of chips. >> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> --- >> v3: >> - Remove accepted patches from patch set. >> >> v2: >> - Rename node to fix schema check error. >> --- >> .../intel/socfpga_agilex_pcie_root_port.dtsi | 55 +++++++++++++++++++ >> 1 file changed, 55 insertions(+) >> create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi >> >> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi >> new file mode 100644 >> index 000000000000..50f131f5791b >> --- /dev/null >> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi >> @@ -0,0 +1,55 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2024, Intel Corporation >> + */ >> +&soc0 { >> + aglx_hps_bridges: fpga-bus@80000000 { >> + compatible = "simple-bus"; >> + reg = <0x80000000 0x20200000>, >> + <0xf9000000 0x00100000>; >> + reg-names = "axi_h2f", "axi_h2f_lw"; >> + #address-cells = <0x2>; >> + #size-cells = <0x1>; >> + ranges = <0x00000000 0x00000000 0x80000000 0x00040000>, >> + <0x00000000 0x10000000 0x90100000 0x0ff00000>, >> + <0x00000000 0x20000000 0xa0000000 0x00200000>, >> + <0x00000001 0x00010000 0xf9010000 0x00008000>, >> + <0x00000001 0x00018000 0xf9018000 0x00000080>, >> + <0x00000001 0x00018080 0xf9018080 0x00000010>; >> + >> + pcie_0_pcie_aglx: pcie@200000000 { >> + reg = <0x00000000 0x10000000 0x10000000>, >> + <0x00000001 0x00010000 0x00008000>, >> + <0x00000000 0x20000000 0x00200000>; >> + reg-names = "Txs", "Cra", "Hip"; >> + interrupt-parent = <&intc>; >> + interrupts = <GIC_SPI 0x14 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-controller; >> + #interrupt-cells = <0x1>; >> + device_type = "pci"; >> + bus-range = <0x0000000 0x000000ff>; >> + ranges = <0x82000000 0x00000000 0x00100000 0x00000000 0x10000000 0x00000000 0x0ff00000>; > > This convert to pci address 0x0010_0000..0x1000_0000 from parent bus address > 0x1000_0000..0x1ff0_0000 > > aglx_hps_bridges bridge convert 0x90100000..0xa000_0000 (cpu address) to > 0x1000_0000..0x1ff0_0000 according to ranges[1](second entry). > > Just want to confirm that "0x1000_0000..0x1ff0_0000" is actually reflect > hardware behavior. As far as I know, these conversions reflect the actual hardware behavior, but I will investigate further to confirm. > > On going a thread > https://lore.kernel.org/linux-pci/Z5pfiJyXB3NtGSfe@lizhi-Precision-Tower-5810/T/#t > > Try to clean up all cpu_addr_fixup() or similar fixup() in pci root complex > drivers, but which require dtsi reflect the real hardware behavior. > > In most current pci driver, even "0x1000_0000..0x1ff0_0000" is wrong, it > still work by drivers' fixup. If dts correct descript hardware, these > fixup can be removed. The current driver, drivers/pci/controller/pcie-altera.c, does not have any cpu_addr_fix(); so I think the dts is properly describing the hardware, but I will continue to investigate and follow the thread to clean the fixups. > > best regards > Frank Thanks for the feedback, Matthew Gerlach > >> + msi-parent = <&pcie_0_msi_irq>; >> + #address-cells = <0x3>; >> + #size-cells = <0x2>; >> + interrupt-map-mask = <0x0 0x0 0x0 0x7>; >> + interrupt-map = <0x0 0x0 0x0 0x1 &pcie_0_pcie_aglx 0 0 0 0x1>, >> + <0x0 0x0 0x0 0x2 &pcie_0_pcie_aglx 0 0 0 0x2>, >> + <0x0 0x0 0x0 0x3 &pcie_0_pcie_aglx 0 0 0 0x3>, >> + <0x0 0x0 0x0 0x4 &pcie_0_pcie_aglx 0 0 0 0x4>; >> + status = "disabled"; >> + }; >> + >> + pcie_0_msi_irq: msi@10008080 { >> + compatible = "altr,msi-1.0"; >> + reg = <0x00000001 0x00018080 0x00000010>, >> + <0x00000001 0x00018000 0x00000080>; >> + reg-names = "csr", "vector_slave"; >> + interrupt-parent = <&intc>; >> + interrupts = <GIC_SPI 0x13 IRQ_TYPE_LEVEL_HIGH>; >> + msi-controller; >> + num-vectors = <0x20>; >> + status = "disabled"; >> + }; >> + }; >> +}; >> -- >> 2.34.1 >> >
On Thu, 30 Jan 2025, Krzysztof Kozlowski wrote: > On 29/01/2025 20:42, matthew.gerlach@linux.intel.com wrote: >> >> >> On Wed, 29 Jan 2025, Krzysztof Kozlowski wrote: >> >>> On 27/01/2025 18:35, Matthew Gerlach wrote: >>>> Add the base device tree for support of the PCIe Root Port >>>> for the Agilex family of chips. >>>> >>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>>> --- >>>> v3: >>>> - Remove accepted patches from patch set. >>>> >>>> v2: >>>> - Rename node to fix schema check error. >>>> --- >>>> .../intel/socfpga_agilex_pcie_root_port.dtsi | 55 +++++++++++++++++++ >>>> 1 file changed, 55 insertions(+) >>>> create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi >>>> >>>> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi >>>> new file mode 100644 >>>> index 000000000000..50f131f5791b >>>> --- /dev/null >>>> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi >>>> @@ -0,0 +1,55 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>> >>> Odd spaces in SPDX tag. >> >> Yes, there should only be one space. >> >>> >>>> +/* >>>> + * Copyright (C) 2024, Intel Corporation >>>> + */ >>>> +&soc0 { >>>> + aglx_hps_bridges: fpga-bus@80000000 { >>>> + compatible = "simple-bus"; >>>> + reg = <0x80000000 0x20200000>, >>>> + <0xf9000000 0x00100000>; >>>> + reg-names = "axi_h2f", "axi_h2f_lw"; >>> >>> Where is this binding defined? >> >> The bindings for these reg-names are not currently defined anywhere, but > > Then you cannot use them. > >> they are also referenced in the following: >> Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml >> arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts >> I am not exactly sure where the right place is to define them, maybe >> Documentation/devicetree/bindings/arm/intel,socfpga.yaml. On the other >> hand, no code references these names; so it might make sense to just >> remove them. > > In general: nowhere, because simple bus does not have such properties. > It's not about reg-names only - you cannot have reg. You just did not > define here simple-bus. I understand. I will remove reg and reg-names. > > Best regards, > Krzysztof > Thanks for the review, Matthew Gerlach
On 01/02/2025 20:12, matthew.gerlach@linux.intel.com wrote: >> >>> they are also referenced in the following: >>> Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml >>> arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts >>> I am not exactly sure where the right place is to define them, maybe >>> Documentation/devicetree/bindings/arm/intel,socfpga.yaml. On the other >>> hand, no code references these names; so it might make sense to just >>> remove them. >> >> In general: nowhere, because simple bus does not have such properties. >> It's not about reg-names only - you cannot have reg. You just did not >> define here simple-bus. > > I understand. I will remove reg and reg-names. If you have there IO address space, then removal does not sound right, either. You just need to come with the bindings for this dedicated device, whatever this is. There is no description here, not much in commit msg, so I don't know what is the device you are adding. PCI has several bindings, so is this just host bridge? Best regards, Krzysztof
On Sun, 2 Feb 2025, Krzysztof Kozlowski wrote: > On 01/02/2025 20:12, matthew.gerlach@linux.intel.com wrote: >>> >>>> they are also referenced in the following: >>>> Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml >>>> arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts >>>> I am not exactly sure where the right place is to define them, maybe >>>> Documentation/devicetree/bindings/arm/intel,socfpga.yaml. On the other >>>> hand, no code references these names; so it might make sense to just >>>> remove them. >>> >>> In general: nowhere, because simple bus does not have such properties. >>> It's not about reg-names only - you cannot have reg. You just did not >>> define here simple-bus. >> >> I understand. I will remove reg and reg-names. > > If you have there IO address space, then removal does not sound right, > either. You just need to come with the bindings for this dedicated > device, whatever this is. There is no description here, not much in > commit msg, so I don't know what is the device you are adding. PCI has > several bindings, so is this just host bridge? The device associated with two address ranges may be best described as a simple-bus. It is a bus between the CPU and the directly connected FPGA in the same package as the SOC. The design programmed into the FPGA determines the device(s) connected to the bus. The hardware implementing this bus does have reset lines which allow for safely reprogramming the FPGA while the SOC is running, which implies appropriate bindings as you suggest. Something like the following might make sense: aglx_hps_bridges: fpga-bus@80000000 { compatible = "altr,agilex-hps-fpga-bridge", "simple-bus"; reg = <0x80000000 0x20200000>, <0xf9000000 0x00100000>; reg-names = "axi_h2f", "axi_h2f_lw"; #address-cells = <0x2>; #size-cells = <0x1>; ranges = <0x00000000 0x00000000 0x80000000 0x00040000>, <0x00000000 0x10000000 0x90100000 0x0ff00000>, <0x00000000 0x20000000 0xa0000000 0x00200000>, <0x00000001 0x00010000 0xf9010000 0x00008000>, <0x00000001 0x00018000 0xf9018000 0x00000080>, <0x00000001 0x00018080 0xf9018080 0x00000010>; reset = <&rst SOC2FPGA_RESET>, <&rst LWHPS2FPGA_RESET>; reset-names = "soc2fpga", "lwhps2fpga"; ... }; > > Best regards, > Krzysztof > Thank you for the feedback, Matthew Gerlach
On 02/02/2025 19:49, matthew.gerlach@linux.intel.com wrote: > > > On Sun, 2 Feb 2025, Krzysztof Kozlowski wrote: > >> On 01/02/2025 20:12, matthew.gerlach@linux.intel.com wrote: >>>> >>>>> they are also referenced in the following: >>>>> Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml >>>>> arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts >>>>> I am not exactly sure where the right place is to define them, maybe >>>>> Documentation/devicetree/bindings/arm/intel,socfpga.yaml. On the other >>>>> hand, no code references these names; so it might make sense to just >>>>> remove them. >>>> >>>> In general: nowhere, because simple bus does not have such properties. >>>> It's not about reg-names only - you cannot have reg. You just did not >>>> define here simple-bus. >>> >>> I understand. I will remove reg and reg-names. >> >> If you have there IO address space, then removal does not sound right, >> either. You just need to come with the bindings for this dedicated >> device, whatever this is. There is no description here, not much in >> commit msg, so I don't know what is the device you are adding. PCI has >> several bindings, so is this just host bridge? > > The device associated with two address ranges may be best described as a > simple-bus. It is a bus between the CPU and the directly connected FPGA in > the same package as the SOC. The design programmed into the FPGA > determines the device(s) connected to the bus. The hardware implementing So it is part of FPGA? > this bus does have reset lines which allow for safely reprogramming the Then it is not simple-bus. Simple-bus is our construct for simple devices. You cannot have something a bit more complex and call it simple-bus. > FPGA while the SOC is running, which implies appropriate bindings as you > suggest. Something like the following might make sense: > > aglx_hps_bridges: fpga-bus@80000000 { > compatible = "altr,agilex-hps-fpga-bridge", "simple-bus"; FPGA bridge maybe, but not simple-bus. It does not look like a simple bus if you have here some resources. > reg = <0x80000000 0x20200000>, > <0xf9000000 0x00100000>; > reg-names = "axi_h2f", "axi_h2f_lw"; > #address-cells = <0x2>; > #size-cells = <0x1>; > ranges = <0x00000000 0x00000000 0x80000000 0x00040000>, > <0x00000000 0x10000000 0x90100000 0x0ff00000>, > <0x00000000 0x20000000 0xa0000000 0x00200000>, > <0x00000001 0x00010000 0xf9010000 0x00008000>, > <0x00000001 0x00018000 0xf9018000 0x00000080>, > <0x00000001 0x00018080 0xf9018080 0x00000010>; > reset = <&rst SOC2FPGA_RESET>, <&rst LWHPS2FPGA_RESET>; Best regards, Krzysztof
On Sun, 2 Feb 2025, Krzysztof Kozlowski wrote: > On 02/02/2025 19:49, matthew.gerlach@linux.intel.com wrote: >> >> >> On Sun, 2 Feb 2025, Krzysztof Kozlowski wrote: >> >>> On 01/02/2025 20:12, matthew.gerlach@linux.intel.com wrote: >>>>> >>>>>> they are also referenced in the following: >>>>>> Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml >>>>>> arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts >>>>>> I am not exactly sure where the right place is to define them, maybe >>>>>> Documentation/devicetree/bindings/arm/intel,socfpga.yaml. On the other >>>>>> hand, no code references these names; so it might make sense to just >>>>>> remove them. >>>>> >>>>> In general: nowhere, because simple bus does not have such properties. >>>>> It's not about reg-names only - you cannot have reg. You just did not >>>>> define here simple-bus. >>>> >>>> I understand. I will remove reg and reg-names. >>> >>> If you have there IO address space, then removal does not sound right, >>> either. You just need to come with the bindings for this dedicated >>> device, whatever this is. There is no description here, not much in >>> commit msg, so I don't know what is the device you are adding. PCI has >>> several bindings, so is this just host bridge? >> >> The device associated with two address ranges may be best described as a >> simple-bus. It is a bus between the CPU and the directly connected FPGA in >> the same package as the SOC. The design programmed into the FPGA >> determines the device(s) connected to the bus. The hardware implementing > > So it is part of FPGA? Technically, the PCIe Tiles are hard IP, but they are connected to the processor through the FPGA. > >> this bus does have reset lines which allow for safely reprogramming the > > Then it is not simple-bus. Simple-bus is our construct for simple > devices. You cannot have something a bit more complex and call it > simple-bus. > >> FPGA while the SOC is running, which implies appropriate bindings as you >> suggest. Something like the following might make sense: >> >> aglx_hps_bridges: fpga-bus@80000000 { >> compatible = "altr,agilex-hps-fpga-bridge", "simple-bus"; > > FPGA bridge maybe, but not simple-bus. It does not look like a simple > bus if you have here some resources. FPGA bridge is a more accurate description of the actual hardware than simple-bus. This bridge does have two address ranges to access the FPGA. One address range is considered "light-weight" intended for register accesses in the FPGA, while the other a full featured AXI interface suitable DMA. > >> reg = <0x80000000 0x20200000>, >> <0xf9000000 0x00100000>; >> reg-names = "axi_h2f", "axi_h2f_lw"; >> #address-cells = <0x2>; >> #size-cells = <0x1>; >> ranges = <0x00000000 0x00000000 0x80000000 0x00040000>, >> <0x00000000 0x10000000 0x90100000 0x0ff00000>, >> <0x00000000 0x20000000 0xa0000000 0x00200000>, >> <0x00000001 0x00010000 0xf9010000 0x00008000>, >> <0x00000001 0x00018000 0xf9018000 0x00000080>, >> <0x00000001 0x00018080 0xf9018080 0x00000010>; >> reset = <&rst SOC2FPGA_RESET>, <&rst LWHPS2FPGA_RESET>; > Best regards, > Krzysztof > Thanks for the feedback, Matthew Gerlach
diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi new file mode 100644 index 000000000000..50f131f5791b --- /dev/null +++ b/arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2024, Intel Corporation + */ +&soc0 { + aglx_hps_bridges: fpga-bus@80000000 { + compatible = "simple-bus"; + reg = <0x80000000 0x20200000>, + <0xf9000000 0x00100000>; + reg-names = "axi_h2f", "axi_h2f_lw"; + #address-cells = <0x2>; + #size-cells = <0x1>; + ranges = <0x00000000 0x00000000 0x80000000 0x00040000>, + <0x00000000 0x10000000 0x90100000 0x0ff00000>, + <0x00000000 0x20000000 0xa0000000 0x00200000>, + <0x00000001 0x00010000 0xf9010000 0x00008000>, + <0x00000001 0x00018000 0xf9018000 0x00000080>, + <0x00000001 0x00018080 0xf9018080 0x00000010>; + + pcie_0_pcie_aglx: pcie@200000000 { + reg = <0x00000000 0x10000000 0x10000000>, + <0x00000001 0x00010000 0x00008000>, + <0x00000000 0x20000000 0x00200000>; + reg-names = "Txs", "Cra", "Hip"; + interrupt-parent = <&intc>; + interrupts = <GIC_SPI 0x14 IRQ_TYPE_LEVEL_HIGH>; + interrupt-controller; + #interrupt-cells = <0x1>; + device_type = "pci"; + bus-range = <0x0000000 0x000000ff>; + ranges = <0x82000000 0x00000000 0x00100000 0x00000000 0x10000000 0x00000000 0x0ff00000>; + msi-parent = <&pcie_0_msi_irq>; + #address-cells = <0x3>; + #size-cells = <0x2>; + interrupt-map-mask = <0x0 0x0 0x0 0x7>; + interrupt-map = <0x0 0x0 0x0 0x1 &pcie_0_pcie_aglx 0 0 0 0x1>, + <0x0 0x0 0x0 0x2 &pcie_0_pcie_aglx 0 0 0 0x2>, + <0x0 0x0 0x0 0x3 &pcie_0_pcie_aglx 0 0 0 0x3>, + <0x0 0x0 0x0 0x4 &pcie_0_pcie_aglx 0 0 0 0x4>; + status = "disabled"; + }; + + pcie_0_msi_irq: msi@10008080 { + compatible = "altr,msi-1.0"; + reg = <0x00000001 0x00018080 0x00000010>, + <0x00000001 0x00018000 0x00000080>; + reg-names = "csr", "vector_slave"; + interrupt-parent = <&intc>; + interrupts = <GIC_SPI 0x13 IRQ_TYPE_LEVEL_HIGH>; + msi-controller; + num-vectors = <0x20>; + status = "disabled"; + }; + }; +};
Add the base device tree for support of the PCIe Root Port for the Agilex family of chips. Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> --- v3: - Remove accepted patches from patch set. v2: - Rename node to fix schema check error. --- .../intel/socfpga_agilex_pcie_root_port.dtsi | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi