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

[PATCH v3 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 5.

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          | 246 +++++++++++++++++-
 6 files changed, 319 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 v3 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

[PATCH v3 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 2a5eeb21da47..98e14b9b4228 100644
--- a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
+++ b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
@@ -149,7 +149,7 @@ usbphy0: usbphy {
 		compatible = "usb-nop-xceiv";
 	};
 
-	soc@0 {
+	soc0: soc@0 {
 		#address-cells = <1>;
 		#size-cells = <1>;
 		compatible = "simple-bus";
-- 
2.34.1

[PATCH v3 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 v3 3/5] arm64: dts: agilex: add dtsi for PCIe Root Port

[Bjorn Helgaas]

On Wed, Jan 08, 2025 at 10:59:07AM -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>;

I don't think this bus-range is needed since
pci_parse_request_of_pci_ranges() defaults to 00-ff when bus-range is
absent.

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

[matthew.gerlach]

On Wed, 8 Jan 2025, Bjorn Helgaas wrote:

> On Wed, Jan 08, 2025 at 10:59:07AM -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>;
>
> I don't think this bus-range is needed since
> pci_parse_request_of_pci_ranges() defaults to 00-ff when bus-range is
> absent.
>

Yes, pci_parse_request_of_pci_ranges() does default to using 00-ff when 
the bus-range property is absent. Removing the bus-range property does 
result in an extra kernel message at startup:
     No bus range found for ...,using [bus 00-ff].

If the extra kernel message is not a problem, then removing the bus-range 
property does result in a smaller device tree.

Matthew Gerlach

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

[Bjorn Helgaas]

On Wed, Jan 08, 2025 at 02:53:50PM -0800, matthew.gerlach@linux.intel.com wrote:
> On Wed, 8 Jan 2025, Bjorn Helgaas wrote:
> > On Wed, Jan 08, 2025 at 10:59:07AM -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>;
> > 
> > I don't think this bus-range is needed since
> > pci_parse_request_of_pci_ranges() defaults to 00-ff when bus-range is
> > absent.
> 
> Yes, pci_parse_request_of_pci_ranges() does default to using 00-ff when the
> bus-range property is absent. Removing the bus-range property does result in
> an extra kernel message at startup:
>     No bus range found for ...,using [bus 00-ff].
> 
> If the extra kernel message is not a problem, then removing the bus-range
> property does result in a smaller device tree.

Interesting, I think we should remove that message.

[PATCH v3 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

[PATCH v3 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 Agilex family of chips.

Signed-off-by: D M, Sharath Kumar <sharath.kumar.d.m@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v3:
 - Remove accepted patches from patch set.

v2:
 - Match historical style of subject.
 - Remove unrelated changes.
 - Fix indentation.
---
 drivers/pci/controller/pcie-altera.c | 246 ++++++++++++++++++++++++++-
 1 file changed, 237 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
index eb55a7f8573a..da4ae21d661d 100644
--- a/drivers/pci/controller/pcie-altera.c
+++ b/drivers/pci/controller/pcie-altera.c
@@ -77,9 +77,19 @@
 #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)
+
 enum altera_pcie_version {
 	ALTERA_PCIE_V1 = 0,
 	ALTERA_PCIE_V2,
+	ALTERA_PCIE_V3,
 };
 
 struct altera_pcie {
@@ -102,6 +112,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 +127,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 +149,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 +185,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(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 +474,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(addr);
+		break;
+	case 2:
+		*value = readw(addr);
+		break;
+	default:
+		*value = readl(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(value, addr);
+		break;
+	case 2:
+		writew(value, addr);
+		break;
+	default:
+		writel(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 |= (1 << 12); /* type 1 */
+
+	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 |= (1 << 12); /* type 1 */
+
+	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 +583,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 +631,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 +813,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) {
+		ret = generic_handle_domain_irq(pcie->irq_domain, 0);
+		if (ret)
+			dev_err_ratelimited(dev, "unexpected IRQ\n");
+	}
+	writel(CFG_AER, (pcie->hip_base + pcie->pcie_data->port_conf_offset +
+			 pcie->pcie_data->port_irq_status_offset));
 	chained_irq_exit(chip, desc);
 }
 
@@ -694,9 +871,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 +883,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 +896,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 +905,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 +937,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 +1012,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 v3 5/5] PCI: altera: Add Agilex support

[Manivannan Sadhasivam]

On Wed, Jan 08, 2025 at 10:59:09AM -0600, Matthew Gerlach wrote:
> From: "D M, Sharath Kumar" <sharath.kumar.d.m@intel.com>
> 
> Add PCIe root port controller support Agilex family of chips.
> 

Please add more info about the PCIe controller in description. Like speed,
lanes, IP revision etc... Also, you are introducing ep_{read/write}_cfg()
callbacks, so they should also be described here.

> Signed-off-by: D M, Sharath Kumar <sharath.kumar.d.m@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v3:
>  - Remove accepted patches from patch set.
> 
> v2:
>  - Match historical style of subject.
>  - Remove unrelated changes.
>  - Fix indentation.
> ---
>  drivers/pci/controller/pcie-altera.c | 246 ++++++++++++++++++++++++++-
>  1 file changed, 237 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
> index eb55a7f8573a..da4ae21d661d 100644
> --- a/drivers/pci/controller/pcie-altera.c
> +++ b/drivers/pci/controller/pcie-altera.c
> @@ -77,9 +77,19 @@
>  #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)
> +
>  enum altera_pcie_version {
>  	ALTERA_PCIE_V1 = 0,
>  	ALTERA_PCIE_V2,
> +	ALTERA_PCIE_V3,
>  };
>  
>  struct altera_pcie {
> @@ -102,6 +112,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 +127,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 +149,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)

No need to add inline keyword to .c files. Compiler will inline the function
anyway if needed.

> +{
> +	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 +185,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(addr) & PCI_EXP_LNKSTA_DLLLA);

Why this can't be readw_relaxed()?

> +}
> +
>  /*
>   * 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 +474,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(addr);

Same question as above. Why the relaxed variant is not used here?

> +		break;
> +	case 2:
> +		*value = readw(addr);
> +		break;
> +	default:
> +		*value = readl(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(value, addr);
> +		break;
> +	case 2:
> +		writew(value, addr);
> +		break;
> +	default:
> +		writel(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 |= (1 << 12); /* type 1 */

Can you use macro definition for this?

> +
> +	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 |= (1 << 12); /* type 1 */

Same here.

> +
> +	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 +583,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);

Why do you need to call both rp_read_cfg() and ep_read_cfg()? This looks wrong.

> +
>  	switch (size) {
>  	case 1:
>  		byte_en = 1 << (where & 3);
> @@ -481,6 +631,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 +813,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) {
> +		ret = generic_handle_domain_irq(pcie->irq_domain, 0);
> +		if (ret)
> +			dev_err_ratelimited(dev, "unexpected IRQ\n");

It'd be good to print the IRQ number in error log.

> +	}
> +	writel(CFG_AER, (pcie->hip_base + pcie->pcie_data->port_conf_offset +
> +			 pcie->pcie_data->port_irq_status_offset));

You should clear the IRQ before handling it.

>  	chained_irq_exit(chip, desc);
>  }
>  

[...]

> -	/* 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);

Why altera_pcie_host_init() is not called for ALTERA_PCIE_V3?

- Mani

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

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

[matthew.gerlach]

[-- Attachment #1: Type: text/plain, Size: 10367 bytes --]



On Thu, 16 Jan 2025, Manivannan Sadhasivam wrote:

> On Wed, Jan 08, 2025 at 10:59:09AM -0600, Matthew Gerlach wrote:
>> From: "D M, Sharath Kumar" <sharath.kumar.d.m@intel.com>
>>
>> Add PCIe root port controller support Agilex family of chips.
>>
>
> Please add more info about the PCIe controller in description. Like speed,
> lanes, IP revision etc... Also, you are introducing ep_{read/write}_cfg()
> callbacks, so they should also be described here.

I will add more info about the Agilex PCIe controller and describe 
ep_{read/write}_cfg() callbacks.

>
>> Signed-off-by: D M, Sharath Kumar <sharath.kumar.d.m@intel.com>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v3:
>>  - Remove accepted patches from patch set.
>>
>> v2:
>>  - Match historical style of subject.
>>  - Remove unrelated changes.
>>  - Fix indentation.
>> ---
>>  drivers/pci/controller/pcie-altera.c | 246 ++++++++++++++++++++++++++-
>>  1 file changed, 237 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
>> index eb55a7f8573a..da4ae21d661d 100644
>> --- a/drivers/pci/controller/pcie-altera.c
>> +++ b/drivers/pci/controller/pcie-altera.c
>> @@ -77,9 +77,19 @@
>>  #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)
>> +
>>  enum altera_pcie_version {
>>  	ALTERA_PCIE_V1 = 0,
>>  	ALTERA_PCIE_V2,
>> +	ALTERA_PCIE_V3,
>>  };
>>
>>  struct altera_pcie {
>> @@ -102,6 +112,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 +127,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 +149,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)
>
> No need to add inline keyword to .c files. Compiler will inline the function
> anyway if needed.

Using inline is consistent with existing cra_writel and cra_readl.

>
>> +{
>> +	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 +185,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(addr) & PCI_EXP_LNKSTA_DLLLA);
>
> Why this can't be readw_relaxed()?

This can be changed to readw_relaxed().

>
>> +}
>> +
>>  /*
>>   * 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 +474,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(addr);
>
> Same question as above. Why the relaxed variant is not used here?

Yes, the relaxed variant can be used here too.

>
>> +		break;
>> +	case 2:
>> +		*value = readw(addr);
>> +		break;
>> +	default:
>> +		*value = readl(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(value, addr);
>> +		break;
>> +	case 2:
>> +		writew(value, addr);
>> +		break;
>> +	default:
>> +		writel(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 |= (1 << 12); /* type 1 */
>
> Can you use macro definition for this?

Use a macro like BIT(12)?

>
>> +
>> +	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 |= (1 << 12); /* type 1 */
>
> Same here.
>
>> +
>> +	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 +583,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);
>
> Why do you need to call both rp_read_cfg() and ep_read_cfg()? This looks wrong.
>
>> +
>>  	switch (size) {
>>  	case 1:
>>  		byte_en = 1 << (where & 3);
>> @@ -481,6 +631,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 +813,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) {
>> +		ret = generic_handle_domain_irq(pcie->irq_domain, 0);
>> +		if (ret)
>> +			dev_err_ratelimited(dev, "unexpected IRQ\n");
>
> It'd be good to print the IRQ number in error log.

Adding the IRQ number to the error log would be helpful.

>
>> +	}
>> +	writel(CFG_AER, (pcie->hip_base + pcie->pcie_data->port_conf_offset +
>> +			 pcie->pcie_data->port_irq_status_offset));
>
> You should clear the IRQ before handling it.

Yes, the IRQ should be cleared before handling it.

>
>>  	chained_irq_exit(chip, desc);
>>  }
>>
>
> [...]
>
>> -	/* 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);
>
> Why altera_pcie_host_init() is not called for ALTERA_PCIE_V3?

The V3 hardware does not need to perform a link retraining in order to 
establish link at higher speeds.

>
> - Mani

Thank you for your feedback,
Matthew Gerlach

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

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

[matthew.gerlach]

[-- Attachment #1: Type: text/plain, Size: 11163 bytes --]



On Fri, 17 Jan 2025, matthew.gerlach@linux.intel.com wrote:

>
>
> On Thu, 16 Jan 2025, Manivannan Sadhasivam wrote:
>
>> On Wed, Jan 08, 2025 at 10:59:09AM -0600, Matthew Gerlach wrote:
>>> From: "D M, Sharath Kumar" <sharath.kumar.d.m@intel.com>
>>> 
>>> Add PCIe root port controller support Agilex family of chips.
>>> 
>> 
>> Please add more info about the PCIe controller in description. Like speed,
>> lanes, IP revision etc... Also, you are introducing ep_{read/write}_cfg()
>> callbacks, so they should also be described here.
>
> I will add more info about the Agilex PCIe controller and describe 
> ep_{read/write}_cfg() callbacks.
>
>> 
>>> Signed-off-by: D M, Sharath Kumar <sharath.kumar.d.m@intel.com>
>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>> ---
>>> v3:
>>>  - Remove accepted patches from patch set.
>>> 
>>> v2:
>>>  - Match historical style of subject.
>>>  - Remove unrelated changes.
>>>  - Fix indentation.
>>> ---
>>>  drivers/pci/controller/pcie-altera.c | 246 ++++++++++++++++++++++++++-
>>>  1 file changed, 237 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/drivers/pci/controller/pcie-altera.c 
>>> b/drivers/pci/controller/pcie-altera.c
>>> index eb55a7f8573a..da4ae21d661d 100644
>>> --- a/drivers/pci/controller/pcie-altera.c
>>> +++ b/drivers/pci/controller/pcie-altera.c
>>> @@ -77,9 +77,19 @@
>>>  #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)
>>> +
>>>  enum altera_pcie_version {
>>>  	ALTERA_PCIE_V1 = 0,
>>>  	ALTERA_PCIE_V2,
>>> +	ALTERA_PCIE_V3,
>>>  };
>>>
>>>  struct altera_pcie {
>>> @@ -102,6 +112,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 +127,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 +149,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)
>> 
>> No need to add inline keyword to .c files. Compiler will inline the 
>> function
>> anyway if needed.
>
> Using inline is consistent with existing cra_writel and cra_readl.
>
>> 
>>> +{
>>> +	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 +185,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(addr) & PCI_EXP_LNKSTA_DLLLA);
>> 
>> Why this can't be readw_relaxed()?
>
> This can be changed to readw_relaxed().
>
>> 
>>> +}
>>> +
>>>  /*
>>>   * 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 +474,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(addr);
>> 
>> Same question as above. Why the relaxed variant is not used here?
>
> Yes, the relaxed variant can be used here too.
>
>> 
>>> +		break;
>>> +	case 2:
>>> +		*value = readw(addr);
>>> +		break;
>>> +	default:
>>> +		*value = readl(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(value, addr);
>>> +		break;
>>> +	case 2:
>>> +		writew(value, addr);
>>> +		break;
>>> +	default:
>>> +		writel(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 |= (1 << 12); /* type 1 */
>> 
>> Can you use macro definition for this?
>
> Use a macro like BIT(12)?
>
>> 
>>> +
>>> +	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 |= (1 << 12); /* type 1 */
>> 
>> Same here.
>> 
>>> +
>>> +	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 +583,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);
>> 
>> Why do you need to call both rp_read_cfg() and ep_read_cfg()? This looks 
>> wrong.

I missed this comment in my first reply. The functions, rp_read_cfg() and 
ep_read_cfg(), are not both being called. The flow is if, else if, else.

If root port and rp_read_cfg exits
else if eq_read_cfg() exists
else perform original code path.

Matthew
>> 
>>> +
>>>  	switch (size) {
>>>  	case 1:
>>>  		byte_en = 1 << (where & 3);
>>> @@ -481,6 +631,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 +813,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) {
>>> +		ret = generic_handle_domain_irq(pcie->irq_domain, 0);
>>> +		if (ret)
>>> +			dev_err_ratelimited(dev, "unexpected IRQ\n");
>> 
>> It'd be good to print the IRQ number in error log.
>
> Adding the IRQ number to the error log would be helpful.
>
>> 
>>> +	}
>>> +	writel(CFG_AER, (pcie->hip_base + pcie->pcie_data->port_conf_offset +
>>> +			 pcie->pcie_data->port_irq_status_offset));
>> 
>> You should clear the IRQ before handling it.
>
> Yes, the IRQ should be cleared before handling it.
>
>>
>>>  	chained_irq_exit(chip, desc);
>>>  }
>>> 
>> 
>> [...]
>> 
>>> -	/* 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);
>> 
>> Why altera_pcie_host_init() is not called for ALTERA_PCIE_V3?
>
> The V3 hardware does not need to perform a link retraining in order to 
> establish link at higher speeds.
>
>> 
>> - Mani
>
> Thank you for your feedback,
> Matthew Gerlach
>
>> 
>> -- 
>> மணிவண்ணன் சதாசிவம்
>

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

[Rob Herring (Arm)]

On Wed, 08 Jan 2025 10:59:04 -0600, Matthew Gerlach wrote:
> 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 5.
> 
> 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          | 246 +++++++++++++++++-
>  6 files changed, 319 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
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y intel/socfpga_agilex7f_socdk_pcie_root_port.dtb' for 20250108165909.3344354-1-matthew.gerlach@linux.intel.com:

arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /firmware/svc: failed to match any schema with compatible: ['intel,agilex-svc']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /firmware/svc/fpga-mgr: failed to match any schema with compatible: ['intel,agilex-soc-fpga-mgr']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: cb-intosc-hs-div2-clk: 'clock-frequency' is a required property
	from schema $id: http://devicetree.org/schemas/clock/fixed-clock.yaml#
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: cb-intosc-ls-clk: 'clock-frequency' is a required property
	from schema $id: http://devicetree.org/schemas/clock/fixed-clock.yaml#
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: f2s-free-clk: 'clock-frequency' is a required property
	from schema $id: http://devicetree.org/schemas/clock/fixed-clock.yaml#
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: clock-controller@ffd10000: 'clocks' is a required property
	from schema $id: http://devicetree.org/schemas/clock/intel,agilex.yaml#
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/ethernet@ff800000: failed to match any schema with compatible: ['altr,socfpga-stmmac-a10-s10', 'snps,dwmac-3.74a', 'snps,dwmac']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/ethernet@ff800000: failed to match any schema with compatible: ['altr,socfpga-stmmac-a10-s10', 'snps,dwmac-3.74a', 'snps,dwmac']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/ethernet@ff802000: failed to match any schema with compatible: ['altr,socfpga-stmmac-a10-s10', 'snps,dwmac-3.74a', 'snps,dwmac']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/ethernet@ff802000: failed to match any schema with compatible: ['altr,socfpga-stmmac-a10-s10', 'snps,dwmac-3.74a', 'snps,dwmac']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/ethernet@ff804000: failed to match any schema with compatible: ['altr,socfpga-stmmac-a10-s10', 'snps,dwmac-3.74a', 'snps,dwmac']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/ethernet@ff804000: failed to match any schema with compatible: ['altr,socfpga-stmmac-a10-s10', 'snps,dwmac-3.74a', 'snps,dwmac']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/eccmgr: failed to match any schema with compatible: ['altr,socfpga-s10-ecc-manager', 'altr,socfpga-a10-ecc-manager']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/eccmgr: failed to match any schema with compatible: ['altr,socfpga-s10-ecc-manager', 'altr,socfpga-a10-ecc-manager']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/eccmgr/sdramedac: failed to match any schema with compatible: ['altr,sdram-edac-s10']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/eccmgr/ocram-ecc@ff8cc000: failed to match any schema with compatible: ['altr,socfpga-s10-ocram-ecc', 'altr,socfpga-a10-ocram-ecc']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/eccmgr/ocram-ecc@ff8cc000: failed to match any schema with compatible: ['altr,socfpga-s10-ocram-ecc', 'altr,socfpga-a10-ocram-ecc']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/eccmgr/usb0-ecc@ff8c4000: failed to match any schema with compatible: ['altr,socfpga-s10-usb-ecc', 'altr,socfpga-usb-ecc']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/eccmgr/usb0-ecc@ff8c4000: failed to match any schema with compatible: ['altr,socfpga-s10-usb-ecc', 'altr,socfpga-usb-ecc']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/eccmgr/emac0-rx-ecc@ff8c0000: failed to match any schema with compatible: ['altr,socfpga-s10-eth-mac-ecc', 'altr,socfpga-eth-mac-ecc']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/eccmgr/emac0-rx-ecc@ff8c0000: failed to match any schema with compatible: ['altr,socfpga-s10-eth-mac-ecc', 'altr,socfpga-eth-mac-ecc']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/eccmgr/emac0-tx-ecc@ff8c0400: failed to match any schema with compatible: ['altr,socfpga-s10-eth-mac-ecc', 'altr,socfpga-eth-mac-ecc']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/eccmgr/emac0-tx-ecc@ff8c0400: failed to match any schema with compatible: ['altr,socfpga-s10-eth-mac-ecc', 'altr,socfpga-eth-mac-ecc']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/eccmgr/sdmmca-ecc@ff8c8c00: failed to match any schema with compatible: ['altr,socfpga-s10-sdmmc-ecc', 'altr,socfpga-sdmmc-ecc']
arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: /soc@0/eccmgr/sdmmca-ecc@ff8c8c00: failed to match any schema with compatible: ['altr,socfpga-s10-sdmmc-ecc', 'altr,socfpga-sdmmc-ecc']