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>;
+ 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.1On 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 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 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 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 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
© 2016 - 2026 Red Hat, Inc.