[PATCH v5 0/5] Add PCIe Root Port support for Agilex family of chips

[PATCH v5 0/5] Add PCIe Root Port support for Agilex family of chips

[Matthew Gerlach]

This patch set adds PCIe Root Port support for the Agilex family of FPGA chips.
Version 3 of this patch set removes patches that have been accepted.

Patch 1:
  Add new compatible strings for the three variants of the Agilex PCIe controller IP.

Patch 2:
  Add a label to the soc@0 device tree node to be used by patch 3.

Patch 3:
  Add base dtsi for PCIe Root Port support of the Agilex family of chips.

Patch 4:
  Add dts enabling PCIe Root Port support on an Agilex F-series Development Kit.

Patch 5:
  Update Altera PCIe controller driver to support the Agilex family of chips.

D M, Sharath Kumar (1):
  PCI: altera: Add Agilex support

Matthew Gerlach (4):
  dt-bindings: PCI: altera: Add binding for Agilex
  arm64: dts: agilex: add soc0 label
  arm64: dts: agilex: add dtsi for PCIe Root Port
  arm64: dts: agilex: add dts enabling PCIe Root Port

 .../bindings/pci/altr,pcie-root-port.yaml     |   9 +
 arch/arm64/boot/dts/intel/Makefile            |   1 +
 arch/arm64/boot/dts/intel/socfpga_agilex.dtsi |   2 +-
 .../socfpga_agilex7f_socdk_pcie_root_port.dts |  16 ++
 .../intel/socfpga_agilex_pcie_root_port.dtsi  |  55 ++++
 drivers/pci/controller/pcie-altera.c          | 253 +++++++++++++++++-
 6 files changed, 326 insertions(+), 10 deletions(-)
 create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
 create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi

-- 
2.34.1

[PATCH v5 1/5] dt-bindings: PCI: altera: Add binding for Agilex

[Matthew Gerlach]

Add the compatible bindings for the three variants of Agilex
PCIe Hard IP.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
v3:
 - Remove accepted patches from patch set.
---
 .../devicetree/bindings/pci/altr,pcie-root-port.yaml     | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
index 52533fccc134..ca9691ec87d2 100644
--- a/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
+++ b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
@@ -12,9 +12,18 @@ maintainers:
 
 properties:
   compatible:
+    description: altr,pcie-root-port-1.0 is used for the Cyclone5
+      family of chips. The Stratix10 family of chips is supported
+      by altr,pcie-root-port-2.0. The Agilex family of chips has
+      three variants of PCIe Hard IP referred to as the f-tile, p-tile,
+      and r-tile.
+
     enum:
       - altr,pcie-root-port-1.0
       - altr,pcie-root-port-2.0
+      - altr,pcie-root-port-3.0-f-tile
+      - altr,pcie-root-port-3.0-p-tile
+      - altr,pcie-root-port-3.0-r-tile
 
   reg:
     items:
-- 
2.34.1

Re: [PATCH v5 1/5] dt-bindings: PCI: altera: Add binding for Agilex

[Krzysztof Kozlowski]

On 27/01/2025 18:35, Matthew Gerlach wrote:
> Add the compatible bindings for the three variants of Agilex
> PCIe Hard IP.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> v3:
>  - Remove accepted patches from patch set.
> ---
>  .../devicetree/bindings/pci/altr,pcie-root-port.yaml     | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
> index 52533fccc134..ca9691ec87d2 100644
> --- a/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
> +++ b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
> @@ -12,9 +12,18 @@ maintainers:
>  
>  properties:
>    compatible:
> +    description: altr,pcie-root-port-1.0 is used for the Cyclone5
> +      family of chips. The Stratix10 family of chips is supported
> +      by altr,pcie-root-port-2.0. The Agilex family of chips has
> +      three variants of PCIe Hard IP referred to as the f-tile, p-tile,
> +      and r-tile.


Has three in the same time? Or one of three? Your board DTS said you
have exactly one, so this comment is confusing.


Best regards,
Krzysztof

Re: [PATCH v5 1/5] dt-bindings: PCI: altera: Add binding for Agilex

[matthew.gerlach]

On Thu, 30 Jan 2025, Krzysztof Kozlowski wrote:

> On 27/01/2025 18:35, Matthew Gerlach wrote:
>> Add the compatible bindings for the three variants of Agilex
>> PCIe Hard IP.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
>> ---
>> v3:
>>  - Remove accepted patches from patch set.
>> ---
>>  .../devicetree/bindings/pci/altr,pcie-root-port.yaml     | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
>> index 52533fccc134..ca9691ec87d2 100644
>> --- a/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
>> +++ b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
>> @@ -12,9 +12,18 @@ maintainers:
>>
>>  properties:
>>    compatible:
>> +    description: altr,pcie-root-port-1.0 is used for the Cyclone5
>> +      family of chips. The Stratix10 family of chips is supported
>> +      by altr,pcie-root-port-2.0. The Agilex family of chips has
>> +      three variants of PCIe Hard IP referred to as the f-tile, p-tile,
>> +      and r-tile.
>
>
> Has three in the same time? Or one of three? Your board DTS said you
> have exactly one, so this comment is confusing.

I will clarify this comment to reflect that a particular instantiantion 
will only have one of the tiles.

>
>
> Best regards,
> Krzysztof
>

Thanks for the feedback,
Matthew Gerlach

[PATCH v5 2/5] arm64: dts: agilex: add soc0 label

[Matthew Gerlach]

Add a label to the soc@0 device tree node.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v3:
 - Remove accepted patches from patch set.
---
 arch/arm64/boot/dts/intel/socfpga_agilex.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
index 1235ba5a9865..144fe74e929e 100644
--- a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
+++ b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
@@ -152,7 +152,7 @@ usbphy0: usbphy {
 		compatible = "usb-nop-xceiv";
 	};
 
-	soc@0 {
+	soc0: soc@0 {
 		#address-cells = <1>;
 		#size-cells = <1>;
 		compatible = "simple-bus";
-- 
2.34.1

Re: [PATCH v5 2/5] arm64: dts: agilex: add soc0 label

[Krzysztof Kozlowski]

On 27/01/2025 18:35, Matthew Gerlach wrote:
> Add a label to the soc@0 device tree node.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v3:
>  - Remove accepted patches from patch set.
> ---
>  arch/arm64/boot/dts/intel/socfpga_agilex.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
> index 1235ba5a9865..144fe74e929e 100644
> --- a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
> @@ -152,7 +152,7 @@ usbphy0: usbphy {
>  		compatible = "usb-nop-xceiv";
>  	};
>  
> -	soc@0 {
> +	soc0: soc@0 {
This shouldn't be a separate commit, really. It serves no purpose to
just add the label. Just like you do not add just a define in a driver
without its user. Label like this itself is pointless. It's useful for
something, so this should be squashed.

Best regards,
Krzysztof

Re: [PATCH v5 2/5] arm64: dts: agilex: add soc0 label

[matthew.gerlach]

On Wed, 29 Jan 2025, Krzysztof Kozlowski wrote:

> On 27/01/2025 18:35, Matthew Gerlach wrote:
>> Add a label to the soc@0 device tree node.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v3:
>>  - Remove accepted patches from patch set.
>> ---
>>  arch/arm64/boot/dts/intel/socfpga_agilex.dtsi | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
>> index 1235ba5a9865..144fe74e929e 100644
>> --- a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
>> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
>> @@ -152,7 +152,7 @@ usbphy0: usbphy {
>>  		compatible = "usb-nop-xceiv";
>>  	};
>>
>> -	soc@0 {
>> +	soc0: soc@0 {
> This shouldn't be a separate commit, really. It serves no purpose to
> just add the label. Just like you do not add just a define in a driver
> without its user. Label like this itself is pointless. It's useful for
> something, so this should be squashed.

I will squash this label change with the user of the lable.

Thanks for the review,
Matthew Gerlach

>
> Best regards,
> Krzysztof
>

[PATCH v5 3/5] arm64: dts: agilex: add dtsi for PCIe Root Port

[Matthew Gerlach]

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.1

Re: [PATCH v5 3/5] arm64: dts: agilex: add dtsi for PCIe Root Port

[Krzysztof Kozlowski]

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

Re: [PATCH v5 3/5] arm64: dts: agilex: add dtsi for PCIe Root Port

[matthew.gerlach]

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
>

Re: [PATCH v5 3/5] arm64: dts: agilex: add dtsi for PCIe Root Port

[Krzysztof Kozlowski]

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

Re: [PATCH v5 3/5] arm64: dts: agilex: add dtsi for PCIe Root Port

[matthew.gerlach]

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

Re: [PATCH v5 3/5] arm64: dts: agilex: add dtsi for PCIe Root Port

[Krzysztof Kozlowski]

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

Re: [PATCH v5 3/5] arm64: dts: agilex: add dtsi for PCIe Root Port

[matthew.gerlach]

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

Re: [PATCH v5 3/5] arm64: dts: agilex: add dtsi for PCIe Root Port

[Krzysztof Kozlowski]

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

Re: [PATCH v5 3/5] arm64: dts: agilex: add dtsi for PCIe Root Port

[matthew.gerlach]

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

Re: [PATCH v5 3/5] arm64: dts: agilex: add dtsi for PCIe Root Port

[Frank Li]

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
>

Re: [PATCH v5 3/5] arm64: dts: agilex: add dtsi for PCIe Root Port

[matthew.gerlach]

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
>>
>

[PATCH v5 4/5] arm64: dts: agilex: add dts enabling PCIe Root Port

[Matthew Gerlach]

Add a device tree enabling PCIe Root Port support on
an Agilex F-series Development Kit which has the
P-tile variant PCIe IP.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v3:
 - Remove accepted patches from patch set.
---
 arch/arm64/boot/dts/intel/Makefile               |  1 +
 .../socfpga_agilex7f_socdk_pcie_root_port.dts    | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)
 create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts

diff --git a/arch/arm64/boot/dts/intel/Makefile b/arch/arm64/boot/dts/intel/Makefile
index d39cfb723f5b..737e81c3c3f7 100644
--- a/arch/arm64/boot/dts/intel/Makefile
+++ b/arch/arm64/boot/dts/intel/Makefile
@@ -2,6 +2,7 @@
 dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += socfpga_agilex_n6000.dtb \
 				socfpga_agilex_socdk.dtb \
 				socfpga_agilex_socdk_nand.dtb \
+				socfpga_agilex7f_socdk_pcie_root_port.dtb \
 				socfpga_agilex5_socdk.dtb \
 				socfpga_n5x_socdk.dtb
 dtb-$(CONFIG_ARCH_KEEMBAY) += keembay-evm.dtb
diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts b/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
new file mode 100644
index 000000000000..76a989ba6a44
--- /dev/null
+++ b/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier:     GPL-2.0
+/*
+ * Copyright (C) 2024, Intel Corporation
+ */
+
+#include "socfpga_agilex_socdk.dts"
+#include "socfpga_agilex_pcie_root_port.dtsi"
+
+&pcie_0_pcie_aglx {
+	status = "okay";
+	compatible = "altr,pcie-root-port-3.0-p-tile";
+};
+
+&pcie_0_msi_irq {
+	status = "okay";
+};
-- 
2.34.1

Re: [PATCH v5 4/5] arm64: dts: agilex: add dts enabling PCIe Root Port

[Krzysztof Kozlowski]

On 27/01/2025 18:35, Matthew Gerlach wrote:
> Add a device tree enabling PCIe Root Port support on
> an Agilex F-series Development Kit which has the
> P-tile variant PCIe IP.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v3:
>  - Remove accepted patches from patch set.
> ---
>  arch/arm64/boot/dts/intel/Makefile               |  1 +
>  .../socfpga_agilex7f_socdk_pcie_root_port.dts    | 16 ++++++++++++++++
>  2 files changed, 17 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
> 
> diff --git a/arch/arm64/boot/dts/intel/Makefile b/arch/arm64/boot/dts/intel/Makefile
> index d39cfb723f5b..737e81c3c3f7 100644
> --- a/arch/arm64/boot/dts/intel/Makefile
> +++ b/arch/arm64/boot/dts/intel/Makefile
> @@ -2,6 +2,7 @@
>  dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += socfpga_agilex_n6000.dtb \
>  				socfpga_agilex_socdk.dtb \
>  				socfpga_agilex_socdk_nand.dtb \
> +				socfpga_agilex7f_socdk_pcie_root_port.dtb \
>  				socfpga_agilex5_socdk.dtb \
>  				socfpga_n5x_socdk.dtb
>  dtb-$(CONFIG_ARCH_KEEMBAY) += keembay-evm.dtb
> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts b/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
> new file mode 100644
> index 000000000000..76a989ba6a44
> --- /dev/null
> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
> @@ -0,0 +1,16 @@
> +// SPDX-License-Identifier:     GPL-2.0
> +/*
> + * Copyright (C) 2024, Intel Corporation
> + */
> +
> +#include "socfpga_agilex_socdk.dts"
> +#include "socfpga_agilex_pcie_root_port.dtsi"
> +

Missing board compatible, missing bindings.

> +&pcie_0_pcie_aglx {
> +	status = "okay";
> +	compatible = "altr,pcie-root-port-3.0-p-tile";

Why do you define the compatible here, not in DTSI? This is highly
unusual and confusing. Also, compatible is never the last property, but
opposite.

Plus:

Please run scripts/checkpatch.pl and fix reported warnings. After that,
run also `scripts/checkpatch.pl --strict` and (probably) fix more
warnings. Some warnings can be ignored, especially from --strict run,
but the code here looks like it needs a fix. Feel free to get in touch
if the warning is not clear.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
Maybe you need to update your dtschema and yamllint. Don't rely on
distro packages for dtschema and be sure you are using the latest
released dtschema.



Best regards,
Krzysztof

Re: [PATCH v5 4/5] arm64: dts: agilex: add dts enabling PCIe Root Port

[matthew.gerlach]

On Wed, 29 Jan 2025, Krzysztof Kozlowski wrote:

> On 27/01/2025 18:35, Matthew Gerlach wrote:
>> Add a device tree enabling PCIe Root Port support on
>> an Agilex F-series Development Kit which has the
>> P-tile variant PCIe IP.
>
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>
Thank you for the pointer. I will fix the commit message accordingly.

>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v3:
>>  - Remove accepted patches from patch set.
>> ---
>>  arch/arm64/boot/dts/intel/Makefile               |  1 +
>>  .../socfpga_agilex7f_socdk_pcie_root_port.dts    | 16 ++++++++++++++++
>>  2 files changed, 17 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
>>
>> diff --git a/arch/arm64/boot/dts/intel/Makefile b/arch/arm64/boot/dts/intel/Makefile
>> index d39cfb723f5b..737e81c3c3f7 100644
>> --- a/arch/arm64/boot/dts/intel/Makefile
>> +++ b/arch/arm64/boot/dts/intel/Makefile
>> @@ -2,6 +2,7 @@
>>  dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += socfpga_agilex_n6000.dtb \
>>  				socfpga_agilex_socdk.dtb \
>>  				socfpga_agilex_socdk_nand.dtb \
>> +				socfpga_agilex7f_socdk_pcie_root_port.dtb \
>>  				socfpga_agilex5_socdk.dtb \
>>  				socfpga_n5x_socdk.dtb
>>  dtb-$(CONFIG_ARCH_KEEMBAY) += keembay-evm.dtb
>> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts b/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
>> new file mode 100644
>> index 000000000000..76a989ba6a44
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
>> @@ -0,0 +1,16 @@
>> +// SPDX-License-Identifier:     GPL-2.0
>> +/*
>> + * Copyright (C) 2024, Intel Corporation
>> + */
>> +
>> +#include "socfpga_agilex_socdk.dts"
>> +#include "socfpga_agilex_pcie_root_port.dtsi"
>> +
>
> Missing board compatible, missing bindings.

The model and compatible bindings are inherited from socfpga_agilex_socdk.dts.

>
>> +&pcie_0_pcie_aglx {
>> +	status = "okay";
>> +	compatible = "altr,pcie-root-port-3.0-p-tile";
>
> Why do you define the compatible here, not in DTSI? This is highly
> unusual and confusing. Also, compatible is never the last property, but
> opposite.

The current DTSI supports all three variants of the PCI hardware in the 
Agilex family, referred to as P-Tile, F-Tile, and R-Tile. This particular 
board has an Agilex chip with the P-Tile variant of the PCI hardware.

I will move the compatible property to be the first property.

>
> Plus:
>
> Please run scripts/checkpatch.pl and fix reported warnings. After that,
> run also `scripts/checkpatch.pl --strict` and (probably) fix more
> warnings. Some warnings can be ignored, especially from --strict run,
> but the code here looks like it needs a fix. Feel free to get in touch
> if the warning is not clear.

The only warning I see from scripts/checkpatch.pl --strict is "added, 
moved or deleted file(s), does MAINTAINERS need updating?". The directory, 
arch/arm64/boot/dts/intel/, is already mentioned in the MAINTAINERS file. 
Do I need to do anything to resolve this?

>
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
> Maybe you need to update your dtschema and yamllint. Don't rely on
> distro packages for dtschema and be sure you are using the latest
> released dtschema.

The dtschema check failures are inherited from socfpga_agilex_socdk.dts.
Rob Herring's bot indicates that "Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not." Is this 
applicable here? Is the correct way to fix the existing dtschema check 
warnings is with their own patches, rather than adding to this PCIe Root 
Port patchset?

Thanks for the review,
Matthew Gerlach

>
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH v5 4/5] arm64: dts: agilex: add dts enabling PCIe Root Port

[Krzysztof Kozlowski]

On 29/01/2025 23:54, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Wed, 29 Jan 2025, Krzysztof Kozlowski wrote:
> 
>> On 27/01/2025 18:35, Matthew Gerlach wrote:
>>> Add a device tree enabling PCIe Root Port support on
>>> an Agilex F-series Development Kit which has the
>>> P-tile variant PCIe IP.
>>
>> Please wrap commit message according to Linux coding style / submission
>> process (neither too early nor over the limit):
>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>
> Thank you for the pointer. I will fix the commit message accordingly.
> 
>>>
>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>> ---
>>> v3:
>>>  - Remove accepted patches from patch set.
>>> ---
>>>  arch/arm64/boot/dts/intel/Makefile               |  1 +
>>>  .../socfpga_agilex7f_socdk_pcie_root_port.dts    | 16 ++++++++++++++++
>>>  2 files changed, 17 insertions(+)
>>>  create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
>>>
>>> diff --git a/arch/arm64/boot/dts/intel/Makefile b/arch/arm64/boot/dts/intel/Makefile
>>> index d39cfb723f5b..737e81c3c3f7 100644
>>> --- a/arch/arm64/boot/dts/intel/Makefile
>>> +++ b/arch/arm64/boot/dts/intel/Makefile
>>> @@ -2,6 +2,7 @@
>>>  dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += socfpga_agilex_n6000.dtb \
>>>  				socfpga_agilex_socdk.dtb \
>>>  				socfpga_agilex_socdk_nand.dtb \
>>> +				socfpga_agilex7f_socdk_pcie_root_port.dtb \
>>>  				socfpga_agilex5_socdk.dtb \
>>>  				socfpga_n5x_socdk.dtb
>>>  dtb-$(CONFIG_ARCH_KEEMBAY) += keembay-evm.dtb
>>> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts b/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
>>> new file mode 100644
>>> index 000000000000..76a989ba6a44
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
>>> @@ -0,0 +1,16 @@
>>> +// SPDX-License-Identifier:     GPL-2.0
>>> +/*
>>> + * Copyright (C) 2024, Intel Corporation
>>> + */
>>> +
>>> +#include "socfpga_agilex_socdk.dts"


Nope, you cannot include a board in other board.

>>> +#include "socfpga_agilex_pcie_root_port.dtsi"
>>> +
>>
>> Missing board compatible, missing bindings.
> 
> The model and compatible bindings are inherited from socfpga_agilex_socdk.dts.

Then this is the same board, so entire DTS should be removed and instead
merged into parent DTS. There is no such thing as "inherit" of an
compatible.

> 
>>
>>> +&pcie_0_pcie_aglx {
>>> +	status = "okay";
>>> +	compatible = "altr,pcie-root-port-3.0-p-tile";
>>
>> Why do you define the compatible here, not in DTSI? This is highly
>> unusual and confusing. Also, compatible is never the last property, but
>> opposite.
> 
> The current DTSI supports all three variants of the PCI hardware in the 
> Agilex family, referred to as P-Tile, F-Tile, and R-Tile. This particular 
> board has an Agilex chip with the P-Tile variant of the PCI hardware.

And devices are not compatible? If they have common part in the DTSI, I
would expect that. This is really unusual stuff and needs proper
justifications, not just "DTSI support something". DTSI represents SoC
and SoC either has p-tile or something else. It does not have a "wildcard".

It's the same with that earlier simple-bus. You wrote DTS which does not
represent real hardware.

> 
> I will move the compatible property to be the first property.
> 
>>
>> Plus:
>>
>> Please run scripts/checkpatch.pl and fix reported warnings. After that,
>> run also `scripts/checkpatch.pl --strict` and (probably) fix more
>> warnings. Some warnings can be ignored, especially from --strict run,
>> but the code here looks like it needs a fix. Feel free to get in touch
>> if the warning is not clear.
> 
> The only warning I see from scripts/checkpatch.pl --strict is "added, 
> moved or deleted file(s), does MAINTAINERS need updating?". The directory, 
> arch/arm64/boot/dts/intel/, is already mentioned in the MAINTAINERS file. 
> Do I need to do anything to resolve this?

My mistake, I missed the first patch and assumed checkpatch will
complain about this compatible.

> 
>>
>> It does not look like you tested the DTS against bindings. Please run
>> `make dtbs_check W=1` (see
>> Documentation/devicetree/bindings/writing-schema.rst or
>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
>> for instructions).
>> Maybe you need to update your dtschema and yamllint. Don't rely on
>> distro packages for dtschema and be sure you are using the latest
>> released dtschema.
> 
> The dtschema check failures are inherited from socfpga_agilex_socdk.dts.

New errors are not inherited from DTS/DTSI. Anyway, you cannot include
other DTS. DTS is final board. You do not include C files in C code in
Linux kernel.

> Rob Herring's bot indicates that "Ultimately, it is up to the platform
> maintainer whether these warnings are acceptable or not." Is this 
> applicable here? Is the correct way to fix the existing dtschema check 
> warnings is with their own patches, rather than adding to this PCIe Root 
> Port patchset?

Any new warning is on you and for example with that simple-bus, you
brought new ones.

Best regards,
Krzysztof

Re: [PATCH v5 4/5] arm64: dts: agilex: add dts enabling PCIe Root Port

[matthew.gerlach]

On Thu, 30 Jan 2025, Krzysztof Kozlowski wrote:

> On 29/01/2025 23:54, matthew.gerlach@linux.intel.com wrote:
>>
>>
>> On Wed, 29 Jan 2025, Krzysztof Kozlowski wrote:
>>
>>> On 27/01/2025 18:35, Matthew Gerlach wrote:
>>>> Add a device tree enabling PCIe Root Port support on
>>>> an Agilex F-series Development Kit which has the
>>>> P-tile variant PCIe IP.
>>>
>>> Please wrap commit message according to Linux coding style / submission
>>> process (neither too early nor over the limit):
>>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>>
>> Thank you for the pointer. I will fix the commit message accordingly.
>>
>>>>
>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>> ---
>>>> v3:
>>>>  - Remove accepted patches from patch set.
>>>> ---
>>>>  arch/arm64/boot/dts/intel/Makefile               |  1 +
>>>>  .../socfpga_agilex7f_socdk_pcie_root_port.dts    | 16 ++++++++++++++++
>>>>  2 files changed, 17 insertions(+)
>>>>  create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
>>>>
>>>> diff --git a/arch/arm64/boot/dts/intel/Makefile b/arch/arm64/boot/dts/intel/Makefile
>>>> index d39cfb723f5b..737e81c3c3f7 100644
>>>> --- a/arch/arm64/boot/dts/intel/Makefile
>>>> +++ b/arch/arm64/boot/dts/intel/Makefile
>>>> @@ -2,6 +2,7 @@
>>>>  dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += socfpga_agilex_n6000.dtb \
>>>>  				socfpga_agilex_socdk.dtb \
>>>>  				socfpga_agilex_socdk_nand.dtb \
>>>> +				socfpga_agilex7f_socdk_pcie_root_port.dtb \
>>>>  				socfpga_agilex5_socdk.dtb \
>>>>  				socfpga_n5x_socdk.dtb
>>>>  dtb-$(CONFIG_ARCH_KEEMBAY) += keembay-evm.dtb
>>>> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts b/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
>>>> new file mode 100644
>>>> index 000000000000..76a989ba6a44
>>>> --- /dev/null
>>>> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dts
>>>> @@ -0,0 +1,16 @@
>>>> +// SPDX-License-Identifier:     GPL-2.0
>>>> +/*
>>>> + * Copyright (C) 2024, Intel Corporation
>>>> + */
>>>> +
>>>> +#include "socfpga_agilex_socdk.dts"
>
>
> Nope, you cannot include a board in other board.

Ok, I understand.

>
>>>> +#include "socfpga_agilex_pcie_root_port.dtsi"
>>>> +
>>>
>>> Missing board compatible, missing bindings.
>>
>> The model and compatible bindings are inherited from socfpga_agilex_socdk.dts.
>
> Then this is the same board, so entire DTS should be removed and instead
> merged into parent DTS. There is no such thing as "inherit" of an
> compatible.

It is the same physical board, but the image programmed into the FPGA is 
different in so far as the PCIe IP is connected and enabled. This 
different FPGA image allows for a PCIe End Point to be plugged in. Is this 
difference enough for it be considered and different board?

>
>>
>>>
>>>> +&pcie_0_pcie_aglx {
>>>> +	status = "okay";
>>>> +	compatible = "altr,pcie-root-port-3.0-p-tile";
>>>
>>> Why do you define the compatible here, not in DTSI? This is highly
>>> unusual and confusing. Also, compatible is never the last property, but
>>> opposite.
>>
>> The current DTSI supports all three variants of the PCI hardware in the
>> Agilex family, referred to as P-Tile, F-Tile, and R-Tile. This particular
>> board has an Agilex chip with the P-Tile variant of the PCI hardware.
>
> And devices are not compatible? If they have common part in the DTSI, I
> would expect that. This is really unusual stuff and needs proper
> justifications, not just "DTSI support something". DTSI represents SoC
> and SoC either has p-tile or something else. It does not have a "wildcard".

Unfortunately, the P-Tile, F-Tile, and R-Tile are not compatible. A small 
number of registers have different offsets.

>
> It's the same with that earlier simple-bus. You wrote DTS which does not
> represent real hardware.
>
>>
>> I will move the compatible property to be the first property.
>>
>>>
>>> Plus:
>>>
>>> Please run scripts/checkpatch.pl and fix reported warnings. After that,
>>> run also `scripts/checkpatch.pl --strict` and (probably) fix more
>>> warnings. Some warnings can be ignored, especially from --strict run,
>>> but the code here looks like it needs a fix. Feel free to get in touch
>>> if the warning is not clear.
>>
>> The only warning I see from scripts/checkpatch.pl --strict is "added,
>> moved or deleted file(s), does MAINTAINERS need updating?". The directory,
>> arch/arm64/boot/dts/intel/, is already mentioned in the MAINTAINERS file.
>> Do I need to do anything to resolve this?
>
> My mistake, I missed the first patch and assumed checkpatch will
> complain about this compatible.
>
>>
>>>
>>> It does not look like you tested the DTS against bindings. Please run
>>> `make dtbs_check W=1` (see
>>> Documentation/devicetree/bindings/writing-schema.rst or
>>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
>>> for instructions).
>>> Maybe you need to update your dtschema and yamllint. Don't rely on
>>> distro packages for dtschema and be sure you are using the latest
>>> released dtschema.
>>
>> The dtschema check failures are inherited from socfpga_agilex_socdk.dts.
>
> New errors are not inherited from DTS/DTSI. Anyway, you cannot include
> other DTS. DTS is final board. You do not include C files in C code in
> Linux kernel.
>
>> Rob Herring's bot indicates that "Ultimately, it is up to the platform
>> maintainer whether these warnings are acceptable or not." Is this
>> applicable here? Is the correct way to fix the existing dtschema check
>> warnings is with their own patches, rather than adding to this PCIe Root
>> Port patchset?
>
> Any new warning is on you and for example with that simple-bus, you
> brought new ones.

I now see the new warnings that I introduced, and they will be fixed.

>
> Best regards,
> Krzysztof
>

Thanks for the feedback,
Matthew Gerlach

Re: [PATCH v5 4/5] arm64: dts: agilex: add dts enabling PCIe Root Port

[Krzysztof Kozlowski]

On 04/02/2025 17:57, matthew.gerlach@linux.intel.com wrote:
> 
>>
>>>>> +#include "socfpga_agilex_pcie_root_port.dtsi"
>>>>> +
>>>>
>>>> Missing board compatible, missing bindings.
>>>
>>> The model and compatible bindings are inherited from socfpga_agilex_socdk.dts.
>>
>> Then this is the same board, so entire DTS should be removed and instead
>> merged into parent DTS. There is no such thing as "inherit" of an
>> compatible.
> 
> It is the same physical board, but the image programmed into the FPGA is 
> different in so far as the PCIe IP is connected and enabled. This 
> different FPGA image allows for a PCIe End Point to be plugged in. Is this 
> difference enough for it be considered and different board?

Yes, it can be different board DTS. Look at other vendors how shared
designs are being actually shared between DTS - DTSI.

Best regards,
Krzysztof

[PATCH v5 5/5] PCI: altera: Add Agilex support

[Matthew Gerlach]

From: "D M, Sharath Kumar" <sharath.kumar.d.m@intel.com>

Add PCIe root port controller support for the Agilex family of chips.
The Agilex PCIe IP has three variants that are mostly sw compatible,
except for a couple register offsets. The P-Tile variant supports
Gen3/Gen4 1x16. The F-Tile variant supports Gen3/Gen4 4x4, 4x8, and 4x16.
The R-Tile variant improves on the F-Tile variant by adding Gen5 support.

To simplify the implementation of pci_ops read/write functions,
ep_{read/write}_cfg() callbacks were added to struct altera_pci_ops
to easily distinguish between hardware variants.

Signed-off-by: D M, Sharath Kumar <sharath.kumar.d.m@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v5:
 - remove unnecessary !!
 - Improve macro usage to make comment unnecessary.

v4:
 - Add info to commit message.
 - Use {read/write}?_relaxed where appropriate.
 - Use BIT(12) instead of (1 << 12).
 - Clear IRQ before handling it.
 - add interrupt number to unexpected IRQ messge.

v3:
 - Remove accepted patches from patch set.

v2:
 - Match historical style of subject.
 - Remove unrelated changes.
 - Fix indentation.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 drivers/pci/controller/pcie-altera.c | 253 ++++++++++++++++++++++++++-
 1 file changed, 244 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
index eb55a7f8573a..42ea9960b9da 100644
--- a/drivers/pci/controller/pcie-altera.c
+++ b/drivers/pci/controller/pcie-altera.c
@@ -6,6 +6,7 @@
  * Description: Altera PCIe host controller driver
  */
 
+#include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/irqchip/chained_irq.h>
@@ -77,9 +78,25 @@
 #define S10_TLP_FMTTYPE_CFGWR0		0x45
 #define S10_TLP_FMTTYPE_CFGWR1		0x44
 
+#define AGLX_RP_CFG_ADDR(pcie, reg)	(((pcie)->hip_base) + (reg))
+#define AGLX_RP_SECONDARY(pcie)		\
+	readb(AGLX_RP_CFG_ADDR(pcie, PCI_SECONDARY_BUS))
+
+#define AGLX_BDF_REG			0x00002004
+#define AGLX_ROOT_PORT_IRQ_STATUS	0x14c
+#define AGLX_ROOT_PORT_IRQ_ENABLE	0x150
+#define CFG_AER				BIT(4)
+
+#define AGLX_CFG_TARGET			GENMASK(13, 12)
+#define AGLX_CFG_TARGET_TYPE0		0
+#define AGLX_CFG_TARGET_TYPE1		1
+#define AGLX_CFG_TARGET_LOCAL_2000	2
+#define AGLX_CFG_TARGET_LOCAL_3000	3
+
 enum altera_pcie_version {
 	ALTERA_PCIE_V1 = 0,
 	ALTERA_PCIE_V2,
+	ALTERA_PCIE_V3,
 };
 
 struct altera_pcie {
@@ -102,6 +119,11 @@ struct altera_pcie_ops {
 			   int size, u32 *value);
 	int (*rp_write_cfg)(struct altera_pcie *pcie, u8 busno,
 			    int where, int size, u32 value);
+	int (*ep_read_cfg)(struct altera_pcie *pcie, u8 busno,
+			   unsigned int devfn, int where, int size, u32 *value);
+	int (*ep_write_cfg)(struct altera_pcie *pcie, u8 busno,
+			    unsigned int devfn, int where, int size, u32 value);
+	void (*rp_isr)(struct irq_desc *desc);
 };
 
 struct altera_pcie_data {
@@ -112,6 +134,9 @@ struct altera_pcie_data {
 	u32 cfgrd1;
 	u32 cfgwr0;
 	u32 cfgwr1;
+	u32 port_conf_offset;
+	u32 port_irq_status_offset;
+	u32 port_irq_enable_offset;
 };
 
 struct tlp_rp_regpair_t {
@@ -131,6 +156,28 @@ static inline u32 cra_readl(struct altera_pcie *pcie, const u32 reg)
 	return readl_relaxed(pcie->cra_base + reg);
 }
 
+static inline void cra_writew(struct altera_pcie *pcie, const u32 value,
+			      const u32 reg)
+{
+	writew_relaxed(value, pcie->cra_base + reg);
+}
+
+static inline u32 cra_readw(struct altera_pcie *pcie, const u32 reg)
+{
+	return readw_relaxed(pcie->cra_base + reg);
+}
+
+static inline void cra_writeb(struct altera_pcie *pcie, const u32 value,
+			      const u32 reg)
+{
+	writeb_relaxed(value, pcie->cra_base + reg);
+}
+
+static inline u32 cra_readb(struct altera_pcie *pcie, const u32 reg)
+{
+	return readb_relaxed(pcie->cra_base + reg);
+}
+
 static bool altera_pcie_link_up(struct altera_pcie *pcie)
 {
 	return !!((cra_readl(pcie, RP_LTSSM) & RP_LTSSM_MASK) == LTSSM_L0);
@@ -145,6 +192,15 @@ static bool s10_altera_pcie_link_up(struct altera_pcie *pcie)
 	return !!(readw(addr) & PCI_EXP_LNKSTA_DLLLA);
 }
 
+static bool aglx_altera_pcie_link_up(struct altera_pcie *pcie)
+{
+	void __iomem *addr = AGLX_RP_CFG_ADDR(pcie,
+				   pcie->pcie_data->cap_offset +
+				   PCI_EXP_LNKSTA);
+
+	return (readw_relaxed(addr) & PCI_EXP_LNKSTA_DLLLA);
+}
+
 /*
  * Altera PCIe port uses BAR0 of RC's configuration space as the translation
  * from PCI bus to native BUS.  Entire DDR region is mapped into PCIe space
@@ -425,6 +481,103 @@ static int s10_rp_write_cfg(struct altera_pcie *pcie, u8 busno,
 	return PCIBIOS_SUCCESSFUL;
 }
 
+static int aglx_rp_read_cfg(struct altera_pcie *pcie, int where,
+			    int size, u32 *value)
+{
+	void __iomem *addr = AGLX_RP_CFG_ADDR(pcie, where);
+
+	switch (size) {
+	case 1:
+		*value = readb_relaxed(addr);
+		break;
+	case 2:
+		*value = readw_relaxed(addr);
+		break;
+	default:
+		*value = readl_relaxed(addr);
+		break;
+	}
+
+	/* interrupt pin not programmed in hardware, set to INTA */
+	if (where == PCI_INTERRUPT_PIN && size == 1 && !(*value))
+		*value = 0x01;
+	else if (where == PCI_INTERRUPT_LINE && !(*value & 0xff00))
+		*value |= 0x0100;
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int aglx_rp_write_cfg(struct altera_pcie *pcie, u8 busno,
+			     int where, int size, u32 value)
+{
+	void __iomem *addr = AGLX_RP_CFG_ADDR(pcie, where);
+
+	switch (size) {
+	case 1:
+		writeb_relaxed(value, addr);
+		break;
+	case 2:
+		writew_relaxed(value, addr);
+		break;
+	default:
+		writel_relaxed(value, addr);
+		break;
+	}
+
+	/*
+	 * Monitor changes to PCI_PRIMARY_BUS register on root port
+	 * and update local copy of root bus number accordingly.
+	 */
+	if (busno == pcie->root_bus_nr && where == PCI_PRIMARY_BUS)
+		pcie->root_bus_nr = value & 0xff;
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int aglx_ep_write_cfg(struct altera_pcie *pcie, u8 busno,
+			     unsigned int devfn, int where, int size, u32 value)
+{
+	cra_writel(pcie, ((busno << 8) | devfn), AGLX_BDF_REG);
+	if (busno > AGLX_RP_SECONDARY(pcie))
+		where |= FIELD_PREP(AGLX_CFG_TARGET, AGLX_CFG_TARGET_TYPE1);
+
+	switch (size) {
+	case 1:
+		cra_writeb(pcie, value, where);
+		break;
+	case 2:
+		cra_writew(pcie, value, where);
+		break;
+	default:
+		cra_writel(pcie, value, where);
+			break;
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int aglx_ep_read_cfg(struct altera_pcie *pcie, u8 busno,
+			    unsigned int devfn, int where, int size, u32 *value)
+{
+	cra_writel(pcie, ((busno << 8) | devfn), AGLX_BDF_REG);
+	if (busno > AGLX_RP_SECONDARY(pcie))
+		where |= FIELD_PREP(AGLX_CFG_TARGET, AGLX_CFG_TARGET_TYPE1);
+
+	switch (size) {
+	case 1:
+		*value = cra_readb(pcie, where);
+		break;
+	case 2:
+		*value = cra_readw(pcie, where);
+		break;
+	default:
+		*value = cra_readl(pcie, where);
+		break;
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 static int _altera_pcie_cfg_read(struct altera_pcie *pcie, u8 busno,
 				 unsigned int devfn, int where, int size,
 				 u32 *value)
@@ -437,6 +590,10 @@ static int _altera_pcie_cfg_read(struct altera_pcie *pcie, u8 busno,
 		return pcie->pcie_data->ops->rp_read_cfg(pcie, where,
 							 size, value);
 
+	if (pcie->pcie_data->ops->ep_read_cfg)
+		return pcie->pcie_data->ops->ep_read_cfg(pcie, busno, devfn,
+							where, size, value);
+
 	switch (size) {
 	case 1:
 		byte_en = 1 << (where & 3);
@@ -481,6 +638,10 @@ static int _altera_pcie_cfg_write(struct altera_pcie *pcie, u8 busno,
 		return pcie->pcie_data->ops->rp_write_cfg(pcie, busno,
 						     where, size, value);
 
+	if (pcie->pcie_data->ops->ep_write_cfg)
+		return pcie->pcie_data->ops->ep_write_cfg(pcie, busno, devfn,
+						     where, size, value);
+
 	switch (size) {
 	case 1:
 		data32 = (value & 0xff) << shift;
@@ -659,7 +820,30 @@ static void altera_pcie_isr(struct irq_desc *desc)
 				dev_err_ratelimited(dev, "unexpected IRQ, INT%d\n", bit);
 		}
 	}
+	chained_irq_exit(chip, desc);
+}
+
+static void aglx_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct altera_pcie *pcie;
+	struct device *dev;
+	u32 status;
+	int ret;
+
+	chained_irq_enter(chip, desc);
+	pcie = irq_desc_get_handler_data(desc);
+	dev = &pcie->pdev->dev;
 
+	status = readl(pcie->hip_base + pcie->pcie_data->port_conf_offset +
+		       pcie->pcie_data->port_irq_status_offset);
+	if (status & CFG_AER) {
+		writel(CFG_AER, (pcie->hip_base + pcie->pcie_data->port_conf_offset +
+				 pcie->pcie_data->port_irq_status_offset));
+		ret = generic_handle_domain_irq(pcie->irq_domain, 0);
+		if (ret)
+			dev_err_ratelimited(dev, "unexpected IRQ %d\n", pcie->irq);
+	}
 	chained_irq_exit(chip, desc);
 }
 
@@ -694,9 +878,9 @@ static int altera_pcie_parse_dt(struct altera_pcie *pcie)
 	if (IS_ERR(pcie->cra_base))
 		return PTR_ERR(pcie->cra_base);
 
-	if (pcie->pcie_data->version == ALTERA_PCIE_V2) {
-		pcie->hip_base =
-			devm_platform_ioremap_resource_byname(pdev, "Hip");
+	if (pcie->pcie_data->version == ALTERA_PCIE_V2 ||
+	    pcie->pcie_data->version == ALTERA_PCIE_V3) {
+		pcie->hip_base = devm_platform_ioremap_resource_byname(pdev, "Hip");
 		if (IS_ERR(pcie->hip_base))
 			return PTR_ERR(pcie->hip_base);
 	}
@@ -706,7 +890,7 @@ static int altera_pcie_parse_dt(struct altera_pcie *pcie)
 	if (pcie->irq < 0)
 		return pcie->irq;
 
-	irq_set_chained_handler_and_data(pcie->irq, altera_pcie_isr, pcie);
+	irq_set_chained_handler_and_data(pcie->irq, pcie->pcie_data->ops->rp_isr, pcie);
 	return 0;
 }
 
@@ -719,6 +903,7 @@ static const struct altera_pcie_ops altera_pcie_ops_1_0 = {
 	.tlp_read_pkt = tlp_read_packet,
 	.tlp_write_pkt = tlp_write_packet,
 	.get_link_status = altera_pcie_link_up,
+	.rp_isr = altera_pcie_isr,
 };
 
 static const struct altera_pcie_ops altera_pcie_ops_2_0 = {
@@ -727,6 +912,16 @@ static const struct altera_pcie_ops altera_pcie_ops_2_0 = {
 	.get_link_status = s10_altera_pcie_link_up,
 	.rp_read_cfg = s10_rp_read_cfg,
 	.rp_write_cfg = s10_rp_write_cfg,
+	.rp_isr = altera_pcie_isr,
+};
+
+static const struct altera_pcie_ops altera_pcie_ops_3_0 = {
+	.rp_read_cfg = aglx_rp_read_cfg,
+	.rp_write_cfg = aglx_rp_write_cfg,
+	.get_link_status = aglx_altera_pcie_link_up,
+	.ep_read_cfg = aglx_ep_read_cfg,
+	.ep_write_cfg = aglx_ep_write_cfg,
+	.rp_isr = aglx_isr,
 };
 
 static const struct altera_pcie_data altera_pcie_1_0_data = {
@@ -749,11 +944,44 @@ static const struct altera_pcie_data altera_pcie_2_0_data = {
 	.cfgwr1 = S10_TLP_FMTTYPE_CFGWR1,
 };
 
+static const struct altera_pcie_data altera_pcie_3_0_f_tile_data = {
+	.ops = &altera_pcie_ops_3_0,
+	.version = ALTERA_PCIE_V3,
+	.cap_offset = 0x70,
+	.port_conf_offset = 0x14000,
+	.port_irq_status_offset = AGLX_ROOT_PORT_IRQ_STATUS,
+	.port_irq_enable_offset = AGLX_ROOT_PORT_IRQ_ENABLE,
+};
+
+static const struct altera_pcie_data altera_pcie_3_0_p_tile_data = {
+	.ops = &altera_pcie_ops_3_0,
+	.version = ALTERA_PCIE_V3,
+	.cap_offset = 0x70,
+	.port_conf_offset = 0x104000,
+	.port_irq_status_offset = AGLX_ROOT_PORT_IRQ_STATUS,
+	.port_irq_enable_offset = AGLX_ROOT_PORT_IRQ_ENABLE,
+};
+
+static const struct altera_pcie_data altera_pcie_3_0_r_tile_data = {
+	.ops = &altera_pcie_ops_3_0,
+	.version = ALTERA_PCIE_V3,
+	.cap_offset = 0x70,
+	.port_conf_offset = 0x1300,
+	.port_irq_status_offset = 0x0,
+	.port_irq_enable_offset = 0x4,
+};
+
 static const struct of_device_id altera_pcie_of_match[] = {
 	{.compatible = "altr,pcie-root-port-1.0",
 	 .data = &altera_pcie_1_0_data },
 	{.compatible = "altr,pcie-root-port-2.0",
 	 .data = &altera_pcie_2_0_data },
+	{.compatible = "altr,pcie-root-port-3.0-f-tile",
+	 .data = &altera_pcie_3_0_f_tile_data },
+	{.compatible = "altr,pcie-root-port-3.0-p-tile",
+	 .data = &altera_pcie_3_0_p_tile_data },
+	{.compatible = "altr,pcie-root-port-3.0-r-tile",
+	 .data = &altera_pcie_3_0_r_tile_data },
 	{},
 };
 
@@ -791,11 +1019,18 @@ static int altera_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	/* clear all interrupts */
-	cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
-	/* enable all interrupts */
-	cra_writel(pcie, P2A_INT_ENA_ALL, P2A_INT_ENABLE);
-	altera_pcie_host_init(pcie);
+	if (pcie->pcie_data->version == ALTERA_PCIE_V1 ||
+	    pcie->pcie_data->version == ALTERA_PCIE_V2) {
+		/* clear all interrupts */
+		cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
+		/* enable all interrupts */
+		cra_writel(pcie, P2A_INT_ENA_ALL, P2A_INT_ENABLE);
+		altera_pcie_host_init(pcie);
+	} else if (pcie->pcie_data->version == ALTERA_PCIE_V3) {
+		writel(CFG_AER,
+		       pcie->hip_base + pcie->pcie_data->port_conf_offset +
+		       pcie->pcie_data->port_irq_enable_offset);
+	}
 
 	bridge->sysdata = pcie;
 	bridge->busnr = pcie->root_bus_nr;
-- 
2.34.1

Re: [PATCH v5 5/5] PCI: altera: Add Agilex support

[Krzysztof Kozlowski]

On 27/01/2025 18:35, Matthew Gerlach wrote:
> +
>  static const struct of_device_id altera_pcie_of_match[] = {
>  	{.compatible = "altr,pcie-root-port-1.0",
>  	 .data = &altera_pcie_1_0_data },
>  	{.compatible = "altr,pcie-root-port-2.0",
>  	 .data = &altera_pcie_2_0_data },
> +	{.compatible = "altr,pcie-root-port-3.0-f-tile",
> +	 .data = &altera_pcie_3_0_f_tile_data },
> +	{.compatible = "altr,pcie-root-port-3.0-p-tile",
> +	 .data = &altera_pcie_3_0_p_tile_data },
> +	{.compatible = "altr,pcie-root-port-3.0-r-tile",
> +	 .data = &altera_pcie_3_0_r_tile_data },

Please run scripts/checkpatch.pl and fix reported warnings. After that,
run also `scripts/checkpatch.pl --strict` and (probably) fix more
warnings. Some warnings can be ignored, especially from --strict run,
but the code here looks like it needs a fix. Feel free to get in touch
if the warning is not clear.

Best regards,
Krzysztof

Re: [PATCH v5 5/5] PCI: altera: Add Agilex support

[matthew.gerlach]

On Wed, 29 Jan 2025, Krzysztof Kozlowski wrote:

> On 27/01/2025 18:35, Matthew Gerlach wrote:
>> +
>>  static const struct of_device_id altera_pcie_of_match[] = {
>>  	{.compatible = "altr,pcie-root-port-1.0",
>>  	 .data = &altera_pcie_1_0_data },
>>  	{.compatible = "altr,pcie-root-port-2.0",
>>  	 .data = &altera_pcie_2_0_data },
>> +	{.compatible = "altr,pcie-root-port-3.0-f-tile",
>> +	 .data = &altera_pcie_3_0_f_tile_data },
>> +	{.compatible = "altr,pcie-root-port-3.0-p-tile",
>> +	 .data = &altera_pcie_3_0_p_tile_data },
>> +	{.compatible = "altr,pcie-root-port-3.0-r-tile",
>> +	 .data = &altera_pcie_3_0_r_tile_data },
>
> Please run scripts/checkpatch.pl and fix reported warnings. After that,
> run also `scripts/checkpatch.pl --strict` and (probably) fix more
> warnings. Some warnings can be ignored, especially from --strict run,
> but the code here looks like it needs a fix. Feel free to get in touch
> if the warning is not clear.
I will fix the duplicate signature warning.

Thanks,
Matthew Gerlach
>
> Best regards,
> Krzysztof
>

Re: [PATCH v5 5/5] PCI: altera: Add Agilex support

[Manivannan Sadhasivam]

On Mon, Jan 27, 2025 at 11:35:50AM -0600, Matthew Gerlach wrote:
> From: "D M, Sharath Kumar" <sharath.kumar.d.m@intel.com>
> 
> Add PCIe root port controller support for the Agilex family of chips.
> The Agilex PCIe IP has three variants that are mostly sw compatible,
> except for a couple register offsets. The P-Tile variant supports
> Gen3/Gen4 1x16. The F-Tile variant supports Gen3/Gen4 4x4, 4x8, and 4x16.
> The R-Tile variant improves on the F-Tile variant by adding Gen5 support.
> 
> To simplify the implementation of pci_ops read/write functions,
> ep_{read/write}_cfg() callbacks were added to struct altera_pci_ops
> to easily distinguish between hardware variants.
> 
> Signed-off-by: D M, Sharath Kumar <sharath.kumar.d.m@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Driver changes LGTM! You just need to fix the checkpatch warnings as reported
by krzk. With that,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
> v5:
>  - remove unnecessary !!
>  - Improve macro usage to make comment unnecessary.
> 
> v4:
>  - Add info to commit message.
>  - Use {read/write}?_relaxed where appropriate.
>  - Use BIT(12) instead of (1 << 12).
>  - Clear IRQ before handling it.
>  - add interrupt number to unexpected IRQ messge.
> 
> v3:
>  - Remove accepted patches from patch set.
> 
> v2:
>  - Match historical style of subject.
>  - Remove unrelated changes.
>  - Fix indentation.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  drivers/pci/controller/pcie-altera.c | 253 ++++++++++++++++++++++++++-
>  1 file changed, 244 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
> index eb55a7f8573a..42ea9960b9da 100644
> --- a/drivers/pci/controller/pcie-altera.c
> +++ b/drivers/pci/controller/pcie-altera.c
> @@ -6,6 +6,7 @@
>   * Description: Altera PCIe host controller driver
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
>  #include <linux/irqchip/chained_irq.h>
> @@ -77,9 +78,25 @@
>  #define S10_TLP_FMTTYPE_CFGWR0		0x45
>  #define S10_TLP_FMTTYPE_CFGWR1		0x44
>  
> +#define AGLX_RP_CFG_ADDR(pcie, reg)	(((pcie)->hip_base) + (reg))
> +#define AGLX_RP_SECONDARY(pcie)		\
> +	readb(AGLX_RP_CFG_ADDR(pcie, PCI_SECONDARY_BUS))
> +
> +#define AGLX_BDF_REG			0x00002004
> +#define AGLX_ROOT_PORT_IRQ_STATUS	0x14c
> +#define AGLX_ROOT_PORT_IRQ_ENABLE	0x150
> +#define CFG_AER				BIT(4)
> +
> +#define AGLX_CFG_TARGET			GENMASK(13, 12)
> +#define AGLX_CFG_TARGET_TYPE0		0
> +#define AGLX_CFG_TARGET_TYPE1		1
> +#define AGLX_CFG_TARGET_LOCAL_2000	2
> +#define AGLX_CFG_TARGET_LOCAL_3000	3
> +
>  enum altera_pcie_version {
>  	ALTERA_PCIE_V1 = 0,
>  	ALTERA_PCIE_V2,
> +	ALTERA_PCIE_V3,
>  };
>  
>  struct altera_pcie {
> @@ -102,6 +119,11 @@ struct altera_pcie_ops {
>  			   int size, u32 *value);
>  	int (*rp_write_cfg)(struct altera_pcie *pcie, u8 busno,
>  			    int where, int size, u32 value);
> +	int (*ep_read_cfg)(struct altera_pcie *pcie, u8 busno,
> +			   unsigned int devfn, int where, int size, u32 *value);
> +	int (*ep_write_cfg)(struct altera_pcie *pcie, u8 busno,
> +			    unsigned int devfn, int where, int size, u32 value);
> +	void (*rp_isr)(struct irq_desc *desc);
>  };
>  
>  struct altera_pcie_data {
> @@ -112,6 +134,9 @@ struct altera_pcie_data {
>  	u32 cfgrd1;
>  	u32 cfgwr0;
>  	u32 cfgwr1;
> +	u32 port_conf_offset;
> +	u32 port_irq_status_offset;
> +	u32 port_irq_enable_offset;
>  };
>  
>  struct tlp_rp_regpair_t {
> @@ -131,6 +156,28 @@ static inline u32 cra_readl(struct altera_pcie *pcie, const u32 reg)
>  	return readl_relaxed(pcie->cra_base + reg);
>  }
>  
> +static inline void cra_writew(struct altera_pcie *pcie, const u32 value,
> +			      const u32 reg)
> +{
> +	writew_relaxed(value, pcie->cra_base + reg);
> +}
> +
> +static inline u32 cra_readw(struct altera_pcie *pcie, const u32 reg)
> +{
> +	return readw_relaxed(pcie->cra_base + reg);
> +}
> +
> +static inline void cra_writeb(struct altera_pcie *pcie, const u32 value,
> +			      const u32 reg)
> +{
> +	writeb_relaxed(value, pcie->cra_base + reg);
> +}
> +
> +static inline u32 cra_readb(struct altera_pcie *pcie, const u32 reg)
> +{
> +	return readb_relaxed(pcie->cra_base + reg);
> +}
> +
>  static bool altera_pcie_link_up(struct altera_pcie *pcie)
>  {
>  	return !!((cra_readl(pcie, RP_LTSSM) & RP_LTSSM_MASK) == LTSSM_L0);
> @@ -145,6 +192,15 @@ static bool s10_altera_pcie_link_up(struct altera_pcie *pcie)
>  	return !!(readw(addr) & PCI_EXP_LNKSTA_DLLLA);
>  }
>  
> +static bool aglx_altera_pcie_link_up(struct altera_pcie *pcie)
> +{
> +	void __iomem *addr = AGLX_RP_CFG_ADDR(pcie,
> +				   pcie->pcie_data->cap_offset +
> +				   PCI_EXP_LNKSTA);
> +
> +	return (readw_relaxed(addr) & PCI_EXP_LNKSTA_DLLLA);
> +}
> +
>  /*
>   * Altera PCIe port uses BAR0 of RC's configuration space as the translation
>   * from PCI bus to native BUS.  Entire DDR region is mapped into PCIe space
> @@ -425,6 +481,103 @@ static int s10_rp_write_cfg(struct altera_pcie *pcie, u8 busno,
>  	return PCIBIOS_SUCCESSFUL;
>  }
>  
> +static int aglx_rp_read_cfg(struct altera_pcie *pcie, int where,
> +			    int size, u32 *value)
> +{
> +	void __iomem *addr = AGLX_RP_CFG_ADDR(pcie, where);
> +
> +	switch (size) {
> +	case 1:
> +		*value = readb_relaxed(addr);
> +		break;
> +	case 2:
> +		*value = readw_relaxed(addr);
> +		break;
> +	default:
> +		*value = readl_relaxed(addr);
> +		break;
> +	}
> +
> +	/* interrupt pin not programmed in hardware, set to INTA */
> +	if (where == PCI_INTERRUPT_PIN && size == 1 && !(*value))
> +		*value = 0x01;
> +	else if (where == PCI_INTERRUPT_LINE && !(*value & 0xff00))
> +		*value |= 0x0100;
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int aglx_rp_write_cfg(struct altera_pcie *pcie, u8 busno,
> +			     int where, int size, u32 value)
> +{
> +	void __iomem *addr = AGLX_RP_CFG_ADDR(pcie, where);
> +
> +	switch (size) {
> +	case 1:
> +		writeb_relaxed(value, addr);
> +		break;
> +	case 2:
> +		writew_relaxed(value, addr);
> +		break;
> +	default:
> +		writel_relaxed(value, addr);
> +		break;
> +	}
> +
> +	/*
> +	 * Monitor changes to PCI_PRIMARY_BUS register on root port
> +	 * and update local copy of root bus number accordingly.
> +	 */
> +	if (busno == pcie->root_bus_nr && where == PCI_PRIMARY_BUS)
> +		pcie->root_bus_nr = value & 0xff;
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int aglx_ep_write_cfg(struct altera_pcie *pcie, u8 busno,
> +			     unsigned int devfn, int where, int size, u32 value)
> +{
> +	cra_writel(pcie, ((busno << 8) | devfn), AGLX_BDF_REG);
> +	if (busno > AGLX_RP_SECONDARY(pcie))
> +		where |= FIELD_PREP(AGLX_CFG_TARGET, AGLX_CFG_TARGET_TYPE1);
> +
> +	switch (size) {
> +	case 1:
> +		cra_writeb(pcie, value, where);
> +		break;
> +	case 2:
> +		cra_writew(pcie, value, where);
> +		break;
> +	default:
> +		cra_writel(pcie, value, where);
> +			break;
> +	}
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int aglx_ep_read_cfg(struct altera_pcie *pcie, u8 busno,
> +			    unsigned int devfn, int where, int size, u32 *value)
> +{
> +	cra_writel(pcie, ((busno << 8) | devfn), AGLX_BDF_REG);
> +	if (busno > AGLX_RP_SECONDARY(pcie))
> +		where |= FIELD_PREP(AGLX_CFG_TARGET, AGLX_CFG_TARGET_TYPE1);
> +
> +	switch (size) {
> +	case 1:
> +		*value = cra_readb(pcie, where);
> +		break;
> +	case 2:
> +		*value = cra_readw(pcie, where);
> +		break;
> +	default:
> +		*value = cra_readl(pcie, where);
> +		break;
> +	}
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
>  static int _altera_pcie_cfg_read(struct altera_pcie *pcie, u8 busno,
>  				 unsigned int devfn, int where, int size,
>  				 u32 *value)
> @@ -437,6 +590,10 @@ static int _altera_pcie_cfg_read(struct altera_pcie *pcie, u8 busno,
>  		return pcie->pcie_data->ops->rp_read_cfg(pcie, where,
>  							 size, value);
>  
> +	if (pcie->pcie_data->ops->ep_read_cfg)
> +		return pcie->pcie_data->ops->ep_read_cfg(pcie, busno, devfn,
> +							where, size, value);
> +
>  	switch (size) {
>  	case 1:
>  		byte_en = 1 << (where & 3);
> @@ -481,6 +638,10 @@ static int _altera_pcie_cfg_write(struct altera_pcie *pcie, u8 busno,
>  		return pcie->pcie_data->ops->rp_write_cfg(pcie, busno,
>  						     where, size, value);
>  
> +	if (pcie->pcie_data->ops->ep_write_cfg)
> +		return pcie->pcie_data->ops->ep_write_cfg(pcie, busno, devfn,
> +						     where, size, value);
> +
>  	switch (size) {
>  	case 1:
>  		data32 = (value & 0xff) << shift;
> @@ -659,7 +820,30 @@ static void altera_pcie_isr(struct irq_desc *desc)
>  				dev_err_ratelimited(dev, "unexpected IRQ, INT%d\n", bit);
>  		}
>  	}
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void aglx_isr(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct altera_pcie *pcie;
> +	struct device *dev;
> +	u32 status;
> +	int ret;
> +
> +	chained_irq_enter(chip, desc);
> +	pcie = irq_desc_get_handler_data(desc);
> +	dev = &pcie->pdev->dev;
>  
> +	status = readl(pcie->hip_base + pcie->pcie_data->port_conf_offset +
> +		       pcie->pcie_data->port_irq_status_offset);
> +	if (status & CFG_AER) {
> +		writel(CFG_AER, (pcie->hip_base + pcie->pcie_data->port_conf_offset +
> +				 pcie->pcie_data->port_irq_status_offset));
> +		ret = generic_handle_domain_irq(pcie->irq_domain, 0);
> +		if (ret)
> +			dev_err_ratelimited(dev, "unexpected IRQ %d\n", pcie->irq);
> +	}
>  	chained_irq_exit(chip, desc);
>  }
>  
> @@ -694,9 +878,9 @@ static int altera_pcie_parse_dt(struct altera_pcie *pcie)
>  	if (IS_ERR(pcie->cra_base))
>  		return PTR_ERR(pcie->cra_base);
>  
> -	if (pcie->pcie_data->version == ALTERA_PCIE_V2) {
> -		pcie->hip_base =
> -			devm_platform_ioremap_resource_byname(pdev, "Hip");
> +	if (pcie->pcie_data->version == ALTERA_PCIE_V2 ||
> +	    pcie->pcie_data->version == ALTERA_PCIE_V3) {
> +		pcie->hip_base = devm_platform_ioremap_resource_byname(pdev, "Hip");
>  		if (IS_ERR(pcie->hip_base))
>  			return PTR_ERR(pcie->hip_base);
>  	}
> @@ -706,7 +890,7 @@ static int altera_pcie_parse_dt(struct altera_pcie *pcie)
>  	if (pcie->irq < 0)
>  		return pcie->irq;
>  
> -	irq_set_chained_handler_and_data(pcie->irq, altera_pcie_isr, pcie);
> +	irq_set_chained_handler_and_data(pcie->irq, pcie->pcie_data->ops->rp_isr, pcie);
>  	return 0;
>  }
>  
> @@ -719,6 +903,7 @@ static const struct altera_pcie_ops altera_pcie_ops_1_0 = {
>  	.tlp_read_pkt = tlp_read_packet,
>  	.tlp_write_pkt = tlp_write_packet,
>  	.get_link_status = altera_pcie_link_up,
> +	.rp_isr = altera_pcie_isr,
>  };
>  
>  static const struct altera_pcie_ops altera_pcie_ops_2_0 = {
> @@ -727,6 +912,16 @@ static const struct altera_pcie_ops altera_pcie_ops_2_0 = {
>  	.get_link_status = s10_altera_pcie_link_up,
>  	.rp_read_cfg = s10_rp_read_cfg,
>  	.rp_write_cfg = s10_rp_write_cfg,
> +	.rp_isr = altera_pcie_isr,
> +};
> +
> +static const struct altera_pcie_ops altera_pcie_ops_3_0 = {
> +	.rp_read_cfg = aglx_rp_read_cfg,
> +	.rp_write_cfg = aglx_rp_write_cfg,
> +	.get_link_status = aglx_altera_pcie_link_up,
> +	.ep_read_cfg = aglx_ep_read_cfg,
> +	.ep_write_cfg = aglx_ep_write_cfg,
> +	.rp_isr = aglx_isr,
>  };
>  
>  static const struct altera_pcie_data altera_pcie_1_0_data = {
> @@ -749,11 +944,44 @@ static const struct altera_pcie_data altera_pcie_2_0_data = {
>  	.cfgwr1 = S10_TLP_FMTTYPE_CFGWR1,
>  };
>  
> +static const struct altera_pcie_data altera_pcie_3_0_f_tile_data = {
> +	.ops = &altera_pcie_ops_3_0,
> +	.version = ALTERA_PCIE_V3,
> +	.cap_offset = 0x70,
> +	.port_conf_offset = 0x14000,
> +	.port_irq_status_offset = AGLX_ROOT_PORT_IRQ_STATUS,
> +	.port_irq_enable_offset = AGLX_ROOT_PORT_IRQ_ENABLE,
> +};
> +
> +static const struct altera_pcie_data altera_pcie_3_0_p_tile_data = {
> +	.ops = &altera_pcie_ops_3_0,
> +	.version = ALTERA_PCIE_V3,
> +	.cap_offset = 0x70,
> +	.port_conf_offset = 0x104000,
> +	.port_irq_status_offset = AGLX_ROOT_PORT_IRQ_STATUS,
> +	.port_irq_enable_offset = AGLX_ROOT_PORT_IRQ_ENABLE,
> +};
> +
> +static const struct altera_pcie_data altera_pcie_3_0_r_tile_data = {
> +	.ops = &altera_pcie_ops_3_0,
> +	.version = ALTERA_PCIE_V3,
> +	.cap_offset = 0x70,
> +	.port_conf_offset = 0x1300,
> +	.port_irq_status_offset = 0x0,
> +	.port_irq_enable_offset = 0x4,
> +};
> +
>  static const struct of_device_id altera_pcie_of_match[] = {
>  	{.compatible = "altr,pcie-root-port-1.0",
>  	 .data = &altera_pcie_1_0_data },
>  	{.compatible = "altr,pcie-root-port-2.0",
>  	 .data = &altera_pcie_2_0_data },
> +	{.compatible = "altr,pcie-root-port-3.0-f-tile",
> +	 .data = &altera_pcie_3_0_f_tile_data },
> +	{.compatible = "altr,pcie-root-port-3.0-p-tile",
> +	 .data = &altera_pcie_3_0_p_tile_data },
> +	{.compatible = "altr,pcie-root-port-3.0-r-tile",
> +	 .data = &altera_pcie_3_0_r_tile_data },
>  	{},
>  };
>  
> @@ -791,11 +1019,18 @@ static int altera_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	/* clear all interrupts */
> -	cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
> -	/* enable all interrupts */
> -	cra_writel(pcie, P2A_INT_ENA_ALL, P2A_INT_ENABLE);
> -	altera_pcie_host_init(pcie);
> +	if (pcie->pcie_data->version == ALTERA_PCIE_V1 ||
> +	    pcie->pcie_data->version == ALTERA_PCIE_V2) {
> +		/* clear all interrupts */
> +		cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
> +		/* enable all interrupts */
> +		cra_writel(pcie, P2A_INT_ENA_ALL, P2A_INT_ENABLE);
> +		altera_pcie_host_init(pcie);
> +	} else if (pcie->pcie_data->version == ALTERA_PCIE_V3) {
> +		writel(CFG_AER,
> +		       pcie->hip_base + pcie->pcie_data->port_conf_offset +
> +		       pcie->pcie_data->port_irq_enable_offset);
> +	}
>  
>  	bridge->sysdata = pcie;
>  	bridge->busnr = pcie->root_bus_nr;
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 5/5] PCI: altera: Add Agilex support

[Krzysztof Kozlowski]

On 03/02/2025 15:18, Manivannan Sadhasivam wrote:
> On Mon, Jan 27, 2025 at 11:35:50AM -0600, Matthew Gerlach wrote:
>> From: "D M, Sharath Kumar" <sharath.kumar.d.m@intel.com>
>>
>> Add PCIe root port controller support for the Agilex family of chips.
>> The Agilex PCIe IP has three variants that are mostly sw compatible,
>> except for a couple register offsets. The P-Tile variant supports
>> Gen3/Gen4 1x16. The F-Tile variant supports Gen3/Gen4 4x4, 4x8, and 4x16.
>> The R-Tile variant improves on the F-Tile variant by adding Gen5 support.
>>
>> To simplify the implementation of pci_ops read/write functions,
>> ep_{read/write}_cfg() callbacks were added to struct altera_pci_ops
>> to easily distinguish between hardware variants.
>>
>> Signed-off-by: D M, Sharath Kumar <sharath.kumar.d.m@intel.com>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Driver changes LGTM! You just need to fix the checkpatch warnings as reported
> by krzk. With that,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

I expected warnings, because of missing bindings, but there actually are
bindings, just some unusual order of patches, so maybe nothing to fix.

Anywy just in case: never mix DTS into the middle of patchset because it
just raises the question about dependency and you cannot have one. These
are different subsystems - DTS *always* goes to SoC.

Best regards,
Krzysztof