[PATCH 0/6] Add support for Andes Qilai SoC PCIe controller

[PATCH 0/6] Add support for Andes Qilai SoC PCIe controller

[Randolph Lin]

Add support for Andes Qilai SoC PCIe controller

These patches introduce driver support for the PCIe controller on the
Andes Qilai SoC.

Randolph Lin (6):
  riscv: dts: andes: Define dma-ranges for coherent port
  PCI: dwc: Add outbound ATU range check callback
  dt-bindings: Add Andes QiLai PCIe support
  riscv: dts: andes: Add PCIe node into the QiLai SoC
  PCI: andes: Add Andes QiLai SoC PCIe host driver support
  MAINTAINERS: Add maintainers for Andes QiLai PCIe driver

 .../bindings/pci/andestech,qilai-pcie.yaml    | 146 +++++++++++
 MAINTAINERS                                   |   7 +
 arch/riscv/boot/dts/andes/qilai.dtsi          |  77 ++++++
 drivers/pci/controller/dwc/Kconfig            |  13 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-andes-qilai.c | 227 ++++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.c  |  18 +-
 drivers/pci/controller/dwc/pcie-designware.h  |   3 +
 8 files changed, 487 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/andestech,qilai-pcie.yaml
 create mode 100644 drivers/pci/controller/dwc/pcie-andes-qilai.c

-- 
2.34.1

[PATCH 1/6] riscv: dts: andes: Define dma-ranges for coherent port

[Randolph Lin]

Specify the dma-ranges property to map the coherent port
address space from 0x400000000 to 0x4400000000, ensuring
proper DMA address translation for devices under this port.

Signed-off-by: Randolph Lin <randolph@andestech.com>
---
 arch/riscv/boot/dts/andes/qilai.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/boot/dts/andes/qilai.dtsi b/arch/riscv/boot/dts/andes/qilai.dtsi
index de3de32f8c39..d78d57b3bc52 100644
--- a/arch/riscv/boot/dts/andes/qilai.dtsi
+++ b/arch/riscv/boot/dts/andes/qilai.dtsi
@@ -126,6 +126,7 @@ soc {
 		interrupt-parent = <&plic>;
 		#address-cells = <2>;
 		#size-cells = <2>;
+		dma-ranges = <0x44 0x00000000 0x4 0x00000000 0x4 0x00000000>;
 
 		plmt: timer@100000 {
 			compatible = "andestech,qilai-plmt", "andestech,plmt0";
-- 
2.34.1

[PATCH 2/6] PCI: dwc: Add outbound ATU range check callback

[Randolph Lin]

Introduce a callback for outbound ATU range checking to support
range validations specific to cases that deviate from the generic
check.

Signed-off-by: Randolph Lin <randolph@andestech.com>
---
 drivers/pci/controller/dwc/pcie-designware.c | 18 +++++++++++++-----
 drivers/pci/controller/dwc/pcie-designware.h |  3 +++
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 89aad5a08928..f410aefaeb5e 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -535,12 +535,20 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
 	u32 retries, val;
 	u64 limit_addr;
 
-	limit_addr = parent_bus_addr + atu->size - 1;
+	if (pci->ops && pci->ops->outbound_atu_check) {
+		val = pci->ops->outbound_atu_check(pci, atu, &limit_addr);
+		if (val)
+			return val;
+	} else {
+		limit_addr = parent_bus_addr + atu->size - 1;
 
-	if ((limit_addr & ~pci->region_limit) != (parent_bus_addr & ~pci->region_limit) ||
-	    !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
-	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size) {
-		return -EINVAL;
+		if ((limit_addr & ~pci->region_limit) !=
+		    (parent_bus_addr & ~pci->region_limit) ||
+		    !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
+		    !IS_ALIGNED(atu->pci_addr, pci->region_align) ||
+		    !atu->size) {
+			return -EINVAL;
+		}
 	}
 
 	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_BASE,
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 00f52d472dcd..40dd2c83b1c7 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -469,6 +469,9 @@ struct dw_pcie_ep {
 
 struct dw_pcie_ops {
 	u64	(*cpu_addr_fixup)(struct dw_pcie *pcie, u64 cpu_addr);
+	u32	(*outbound_atu_check)(struct dw_pcie *pcie,
+				      const struct dw_pcie_ob_atu_cfg *atu,
+				      u64 *limit_addr);
 	u32	(*read_dbi)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
 			    size_t size);
 	void	(*write_dbi)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
-- 
2.34.1

Re: [PATCH 2/6] PCI: dwc: Add outbound ATU range check callback

[Bjorn Helgaas]

On Wed, Aug 20, 2025 at 07:18:39PM +0800, Randolph Lin wrote:
> Introduce a callback for outbound ATU range checking to support
> range validations specific to cases that deviate from the generic
> check.
> 
> Signed-off-by: Randolph Lin <randolph@andestech.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 18 +++++++++++++-----
>  drivers/pci/controller/dwc/pcie-designware.h |  3 +++
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 89aad5a08928..f410aefaeb5e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -535,12 +535,20 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
>  	u32 retries, val;
>  	u64 limit_addr;
>  
> -	limit_addr = parent_bus_addr + atu->size - 1;
> +	if (pci->ops && pci->ops->outbound_atu_check) {
> +		val = pci->ops->outbound_atu_check(pci, atu, &limit_addr);

The return is not a "val" and not a "u32".  It should be named "ret"
or similar.  Should be "int" since the callback and
dw_pcie_prog_outbound_atu() return "int".  But see below for possible
signature change.

Also not 100% convinced this is needed, see other patch where this is
implemented.

> +		if (val)
> +			return val;
> +	} else {
> +		limit_addr = parent_bus_addr + atu->size - 1;
>  
> -	if ((limit_addr & ~pci->region_limit) != (parent_bus_addr & ~pci->region_limit) ||
> -	    !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
> -	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size) {
> -		return -EINVAL;
> +		if ((limit_addr & ~pci->region_limit) !=
> +		    (parent_bus_addr & ~pci->region_limit) ||
> +		    !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
> +		    !IS_ALIGNED(atu->pci_addr, pci->region_align) ||
> +		    !atu->size) {
> +			return -EINVAL;
> +		}
>  	}
>  
>  	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_BASE,
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 00f52d472dcd..40dd2c83b1c7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -469,6 +469,9 @@ struct dw_pcie_ep {
>  
>  struct dw_pcie_ops {
>  	u64	(*cpu_addr_fixup)(struct dw_pcie *pcie, u64 cpu_addr);
> +	u32	(*outbound_atu_check)(struct dw_pcie *pcie,
> +				      const struct dw_pcie_ob_atu_cfg *atu,
> +				      u64 *limit_addr);

I have kind of an allergic reaction to things named "check" because
the name doesn't suggest anything about what the function does or what
it will return.  For bool functions, I prefer a name that's a
predicate that can be either true or false, e.g., "valid".

This isn't a bool, but possibly *could* be, e.g.,
"outbound_atu_addr_valid()".  Then the caller would be something like:

  if (!pci->ops->outbound_atu_addr_valid(...))
    return -EINVAL;

[PATCH 3/6] dt-bindings: Add Andes QiLai PCIe support

[Randolph Lin]

Add the Andes QiLai PCIe node, which includes 3 Root Complexes.

Signed-off-by: Randolph Lin <randolph@andestech.com>
---
 .../bindings/pci/andestech,qilai-pcie.yaml    | 146 ++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/andestech,qilai-pcie.yaml

diff --git a/Documentation/devicetree/bindings/pci/andestech,qilai-pcie.yaml b/Documentation/devicetree/bindings/pci/andestech,qilai-pcie.yaml
new file mode 100644
index 000000000000..2c67b3a6828e
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/andestech,qilai-pcie.yaml
@@ -0,0 +1,146 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/andestech,qilai-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Andes QiLai PCIe host controller
+
+description: |+
+  Andes QiLai PCIe host controller is based on the Synopsys DesignWare
+  PCI core. It shares common features with the PCIe DesignWare core and
+  inherits common properties defined in
+  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml.
+
+maintainers:
+  - Randolph Lin <randolph@andestech.com>
+
+allOf:
+  - $ref: /schemas/pci/pci-host-bridge.yaml#
+  - $ref: /schemas/pci/snps,dw-pcie.yaml#
+
+properties:
+  compatible:
+    const: andestech,qilai-pcie
+
+  reg:
+    items:
+      - description: Data Bus Interface (DBI) registers.
+      - description: PCIe configuration space region.
+      - description: APB registers.
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: config
+      - const: apb
+
+  ranges:
+    maxItems: 2
+
+  interrupts:
+    maxItems: 1
+
+  "#interrupt-cells":
+    const: 1
+
+  interrupt-controller: true
+
+required:
+  - reg
+  - reg-names
+  - interrupts
+  - "#interrupt-cells"
+  - interrupt-controller
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      pci@80000000 {
+        compatible = "andestech,qilai-pcie";
+        device_type = "pci";
+        reg = <0x00 0x80000000 0x00 0x20000000>,
+              <0x20 0x00000000 0x00 0x00010000>,
+              <0x00 0x04000000 0x00 0x00001000>;
+        reg-names = "dbi", "config", "apb";
+        bus-range = <0x0 0xff>;
+        num-viewport = <4>;
+
+        #address-cells = <3>;
+        #size-cells = <2>;
+        ranges = <0x02000000 0x00 0x10000000 0x20 0x10000000 0x0 0xF0000000>,
+                 <0x43000000 0x01 0x00000000 0x21 0x0000000 0x1F 0x00000000>;
+
+        #interrupt-cells = <1>;
+        interrupts = <0xF>;
+        interrupt-names = "msi";
+        interrupt-parent = <&plic0>;
+        interrupt-controller;
+        interrupt-map-mask = <0 0 0 7>;
+        interrupt-map = <0 0 0 1 &plic0 0xF IRQ_TYPE_LEVEL_HIGH>,
+                        <0 0 0 2 &plic0 0xF IRQ_TYPE_LEVEL_HIGH>,
+                        <0 0 0 3 &plic0 0xF IRQ_TYPE_LEVEL_HIGH>,
+                        <0 0 0 4 &plic0 0xF IRQ_TYPE_LEVEL_HIGH>;
+      };
+
+      pci@a0000000 {
+        compatible = "andestech,qilai-pcie";
+        device_type = "pci";
+        reg = <0x00 0xA0000000 0x00 0x20000000>,
+              <0x10 0x00000000 0x00 0x00010000>,
+              <0x00 0x04001000 0x00 0x00001000>;
+        reg-names = "dbi", "config", "apb";
+        bus-range = <0x0 0xff>;
+        num-viewport = <4>;
+
+        #address-cells = <3>;
+        #size-cells = <2>;
+        ranges = <0x02000000 0x00 0x10000000 0x10 0x10000000 0x0 0xF0000000>,
+                 <0x43000000 0x01 0x00000000 0x11 0x00000000 0x7 0x00000000>;
+
+        #interrupt-cells = <1>;
+        interrupts = <0xE>;
+        interrupt-names = "msi";
+        interrupt-parent = <&plic0>;
+        interrupt-controller;
+        interrupt-map-mask = <0 0 0 7>;
+        interrupt-map = <0 0 0 1 &plic0 0xE IRQ_TYPE_LEVEL_HIGH>,
+                        <0 0 0 2 &plic0 0xE IRQ_TYPE_LEVEL_HIGH>,
+                        <0 0 0 3 &plic0 0xE IRQ_TYPE_LEVEL_HIGH>,
+                        <0 0 0 4 &plic0 0xE IRQ_TYPE_LEVEL_HIGH>;
+      };
+
+      pci@c0000000 {
+        compatible = "andestech,qilai-pcie";
+        device_type = "pci";
+        reg = <0x00 0xC0000000 0x00 0x20000000>,
+              <0x18 0x00000000 0x00 0x00010000>,
+              <0x00 0x04002000 0x00 0x00001000>;
+        reg-names = "dbi", "config", "apb";
+        bus-range = <0x0 0xff>;
+        num-viewport = <4>;
+
+        #address-cells = <3>;
+        #size-cells = <2>;
+        ranges = <0x02000000 0x00 0x10000000 0x18 0x10000000 0x0 0xF0000000>,
+                 <0x43000000 0x01 0x00000000 0x19 0x00000000 0x7 0x00000000>;
+
+        #interrupt-cells = <1>;
+        interrupts = <0xD>;
+        interrupt-names = "msi";
+        interrupt-parent = <&plic0>;
+        interrupt-controller;
+        interrupt-map-mask = <0 0 0 7>;
+        interrupt-map = <0 0 0 1 &plic0 0xD IRQ_TYPE_LEVEL_HIGH>,
+                        <0 0 0 2 &plic0 0xD IRQ_TYPE_LEVEL_HIGH>,
+                        <0 0 0 3 &plic0 0xD IRQ_TYPE_LEVEL_HIGH>,
+                        <0 0 0 4 &plic0 0xD IRQ_TYPE_LEVEL_HIGH>;
+      };
+    };
-- 
2.34.1

Re: [PATCH 3/6] dt-bindings: Add Andes QiLai PCIe support

[Krzysztof Kozlowski]

On 20/08/2025 13:18, Randolph Lin wrote:
> Add the Andes QiLai PCIe node, which includes 3 Root Complexes.
> 
> Signed-off-by: Randolph Lin <randolph@andestech.com>

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

...

> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      pci@80000000 {
> +        compatible = "andestech,qilai-pcie";
> +        device_type = "pci";
> +        reg = <0x00 0x80000000 0x00 0x20000000>,
> +              <0x20 0x00000000 0x00 0x00010000>,
> +              <0x00 0x04000000 0x00 0x00001000>;
> +        reg-names = "dbi", "config", "apb";
> +        bus-range = <0x0 0xff>;
> +        num-viewport = <4>;
> +
> +        #address-cells = <3>;
> +        #size-cells = <2>;
> +        ranges = <0x02000000 0x00 0x10000000 0x20 0x10000000 0x0 0xF0000000>,
> +                 <0x43000000 0x01 0x00000000 0x21 0x0000000 0x1F 0x00000000>;
> +
> +        #interrupt-cells = <1>;
> +        interrupts = <0xF>;
> +        interrupt-names = "msi";
> +        interrupt-parent = <&plic0>;
> +        interrupt-controller;
> +        interrupt-map-mask = <0 0 0 7>;
> +        interrupt-map = <0 0 0 1 &plic0 0xF IRQ_TYPE_LEVEL_HIGH>,
> +                        <0 0 0 2 &plic0 0xF IRQ_TYPE_LEVEL_HIGH>,
> +                        <0 0 0 3 &plic0 0xF IRQ_TYPE_LEVEL_HIGH>,
> +                        <0 0 0 4 &plic0 0xF IRQ_TYPE_LEVEL_HIGH>;
> +      };
> +
> +      pci@a0000000 {
> +        compatible = "andestech,qilai-pcie";
> +        device_type = "pci";
> +        reg = <0x00 0xA0000000 0x00 0x20000000>,
> +              <0x10 0x00000000 0x00 0x00010000>,
> +              <0x00 0x04001000 0x00 0x00001000>;
> +        reg-names = "dbi", "config", "apb";
> +        bus-range = <0x0 0xff>;
> +        num-viewport = <4>;
> +
> +        #address-cells = <3>;
> +        #size-cells = <2>;
> +        ranges = <0x02000000 0x00 0x10000000 0x10 0x10000000 0x0 0xF0000000>,
> +                 <0x43000000 0x01 0x00000000 0x11 0x00000000 0x7 0x00000000>;
> +
> +        #interrupt-cells = <1>;
> +        interrupts = <0xE>;
> +        interrupt-names = "msi";
> +        interrupt-parent = <&plic0>;
> +        interrupt-controller;
> +        interrupt-map-mask = <0 0 0 7>;
> +        interrupt-map = <0 0 0 1 &plic0 0xE IRQ_TYPE_LEVEL_HIGH>,
> +                        <0 0 0 2 &plic0 0xE IRQ_TYPE_LEVEL_HIGH>,
> +                        <0 0 0 3 &plic0 0xE IRQ_TYPE_LEVEL_HIGH>,
> +                        <0 0 0 4 &plic0 0xE IRQ_TYPE_LEVEL_HIGH>;
> +      };
> +
> +      pci@c0000000 {

One example is enough.

> +        compatible = "andestech,qilai-pcie";
> +        device_type = "pci";
> +        reg = <0x00 0xC0000000 0x00 0x20000000>,
> +              <0x18 0x00000000 0x00 0x00010000>,
> +              <0x00 0x04002000 0x00 0x00001000>;
> +        reg-names = "dbi", "config", "apb";
> +        bus-range = <0x0 0xff>;
> +        num-viewport = <4>;
Best regards,
Krzysztof

[PATCH 4/6] riscv: dts: andes: Add PCIe node into the QiLai SoC

[Randolph Lin]

Add the Andes QiLai PCIe node, which includes 3 Root Complexes.

Signed-off-by: Randolph Lin <randolph@andestech.com>
---
 arch/riscv/boot/dts/andes/qilai.dtsi | 76 ++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/arch/riscv/boot/dts/andes/qilai.dtsi b/arch/riscv/boot/dts/andes/qilai.dtsi
index d78d57b3bc52..9a0e32499621 100644
--- a/arch/riscv/boot/dts/andes/qilai.dtsi
+++ b/arch/riscv/boot/dts/andes/qilai.dtsi
@@ -183,5 +183,81 @@ uart0: serial@30300000 {
 			reg-io-width = <4>;
 			no-loopback-test;
 		};
+
+		/* DM0 */
+		pci@80000000 {
+			compatible = "andestech,qilai-pcie";
+			device_type = "pci";
+			reg = <0x00 0x80000000 0x00 0x20000000>, /* DBI registers */
+			      <0x20 0x00000000 0x00 0x00010000>, /* Configuration registers */
+			      <0x00 0x04000000 0x00 0x00001000>; /* APB registers */
+			reg-names = "dbi", "config", "apb";
+			bus-range = <0x0 0xff>;
+			num-viewport = <4>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			ranges = <0x02000000 0x00 0x10000000 0x20 0x10000000 0x0 0xF0000000>,
+				 <0x43000000 0x01 0x00000000 0x21 0x0000000 0x1F 0x00000000>;
+			#interrupt-cells = <1>;
+			interrupts = <0xF IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi";
+			interrupt-parent = <&plic>;
+			interrupt-map-mask = <0 0 0 0>;
+			interrupt-map = <0 0 0 1 &plic 0xF IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 2 &plic 0xF IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 3 &plic 0xF IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 4 &plic 0xF IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		/* DM1 */
+		pci@a0000000 {
+			compatible = "andestech,qilai-pcie";
+			device_type = "pci";
+			reg = <0x00 0xA0000000 0x00 0x20000000>, /* DBI registers */
+			      <0x10 0x00000000 0x00 0x00010000>, /* Configuration registers */
+			      <0x00 0x04001000 0x00 0x00001000>; /* APB registers */
+			reg-names = "dbi", "config", "apb";
+			bus-range = <0x0 0xff>;
+			num-viewport = <4>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			ranges = <0x02000000 0x00 0x10000000 0x10 0x10000000 0x0 0xF0000000>,
+				 <0x43000000 0x01 0x00000000 0x11 0x00000000 0x7 0x00000000>;
+			#interrupt-cells = <1>;
+			interrupts = <0xE IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi";
+			interrupt-parent = <&plic>;
+			interrupt-controller;
+			interrupt-map-mask = <0 0 0 0>;
+			interrupt-map = <0 0 0 1 &plic 0xE IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 2 &plic 0xE IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 3 &plic 0xE IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 4 &plic 0xE IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		/* DM2 */
+		pci@c0000000 {
+			compatible = "andestech,qilai-pcie";
+			device_type = "pci";
+			reg = <0x00 0xC0000000 0x00 0x20000000>, /* DBI registers */
+			      <0x18 0x00000000 0x00 0x00010000>, /* Configuration registers */
+			      <0x00 0x04002000 0x00 0x00001000>; /* APB registers */
+			reg-names = "dbi", "config", "apb";
+			bus-range = <0x0 0xff>;
+			num-viewport = <4>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			ranges = <0x02000000 0x00 0x10000000 0x18 0x10000000 0x0 0xF0000000>,
+				 <0x43000000 0x01 0x00000000 0x19 0x00000000 0x7 0x00000000>;
+			#interrupt-cells = <1>;
+			interrupts = <0xD IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi";
+			interrupt-parent = <&plic>;
+			interrupt-map-mask = <0 0 0 0>;
+			interrupt-map = <0 0 0 1 &plic 0xD IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 2 &plic 0xD IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 3 &plic 0xD IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 4 &plic 0xD IRQ_TYPE_LEVEL_HIGH>;
+		};
 	};
 };
-- 
2.34.1

[PATCH 5/6] PCI: andes: Add Andes QiLai SoC PCIe host driver support

[Randolph Lin]

Add driver support for DesignWare based PCIe controller in Andes
QiLai SoC. The driver only supports the Root Complex mode.

Signed-off-by: Randolph Lin <randolph@andestech.com>
---
 drivers/pci/controller/dwc/Kconfig            |  13 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-andes-qilai.c | 227 ++++++++++++++++++
 3 files changed, 241 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-andes-qilai.c

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index ff6b6d9e18ec..a9c5a43f648b 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -49,6 +49,19 @@ config PCIE_AMD_MDB
 	  DesignWare IP and therefore the driver re-uses the DesignWare
 	  core functions to implement the driver.
 
+config PCIE_ANDES_QILAI
+	bool "ANDES QiLai PCIe controller"
+	depends on OF && (RISCV || COMPILE_TEST)
+	depends on PCI_MSI
+	depends on ARCH_ANDES
+	select PCIE_DW_HOST
+	default y if ARCH_ANDES
+	help
+	  Say Y here if you want to enable PCIe controller support on Andes
+	  QiLai SoCs. The Andes QiLai SoCs PCIe controller is based on
+	  DesignWare IP and therefore the driver re-uses the DesignWare
+	  core functions to implement the driver.
+
 config PCI_MESON
 	tristate "Amlogic Meson PCIe controller"
 	default m if ARCH_MESON
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 6919d27798d1..de9583cbd675 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
 obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
 obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
 obj-$(CONFIG_PCIE_AMD_MDB) += pcie-amd-mdb.o
+obj-$(CONFIG_PCIE_ANDES_QILAI) += pcie-andes-qilai.o
 obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o
 obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
 obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
diff --git a/drivers/pci/controller/dwc/pcie-andes-qilai.c b/drivers/pci/controller/dwc/pcie-andes-qilai.c
new file mode 100644
index 000000000000..dd06eee82cac
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-andes-qilai.c
@@ -0,0 +1,227 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for the PCIe Controller in QiLai from Andes
+ *
+ * Copyright (C) 2025 Andes Technology Corporation
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#include "pcie-designware.h"
+
+#define PCIE_INTR_CONTROL1			0x15c
+#define PCIE_MSI_CTRL_INT_EN			BIT(28)
+
+#define PCIE_LOGIC_COHERENCY_CONTROL3		0x8e8
+/* Write-Back, Read and Write Allocate */
+#define IOCP_ARCACHE				0xf
+/* Write-Back, Read and Write Allocate */
+#define IOCP_AWCACHE				0xf
+#define PCIE_CFG_MSTR_ARCACHE_MODE		GENMASK(6, 3)
+#define PCIE_CFG_MSTR_AWCACHE_MODE		GENMASK(14, 11)
+#define PCIE_CFG_MSTR_ARCACHE_VALUE		GENMASK(22, 19)
+#define PCIE_CFG_MSTR_AWCACHE_VALUE		GENMASK(30, 27)
+
+#define PCIE_GEN_CONTROL2			0x54
+#define PCIE_CFG_LTSSM_EN			BIT(0)
+
+#define PCIE_REGS_PCIE_SII_PM_STATE		0xc0
+#define SMLH_LINK_UP				BIT(6)
+#define RDLH_LINK_UP				BIT(7)
+#define PCIE_REGS_PCIE_SII_LINK_UP		(SMLH_LINK_UP | RDLH_LINK_UP)
+
+struct qilai_pcie {
+	struct dw_pcie dw;
+	struct platform_device *pdev;
+	void __iomem *apb_base;
+};
+
+#define to_qilai_pcie(_dw) container_of(_dw, struct qilai_pcie, dw)
+
+static u64 qilai_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
+{
+	struct dw_pcie_rp *pp = &pci->pp;
+
+	return cpu_addr - pp->cfg0_base;
+}
+
+static u32 qilai_pcie_outbound_atu_check(struct dw_pcie *pci,
+					 const struct dw_pcie_ob_atu_cfg *atu,
+					 u64 *limit_addr)
+{
+	u64 parent_bus_addr = atu->parent_bus_addr;
+
+	*limit_addr = parent_bus_addr + atu->size - 1;
+
+	/*
+	 * Addresses below 4 GB are not 1:1 mapped; therefore, range checks
+	 * only need to ensure addresses below 4 GB match pci->region_limit.
+	 */
+	if (lower_32_bits(*limit_addr & ~pci->region_limit) !=
+	    lower_32_bits(parent_bus_addr & ~pci->region_limit) ||
+	    !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
+	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size)
+		return -EINVAL;
+	else
+		return 0;
+}
+
+static bool qilai_pcie_link_up(struct dw_pcie *pci)
+{
+	struct qilai_pcie *qlpci = to_qilai_pcie(pci);
+	u32 val;
+
+	/* Read smlh & rdlh link up by checking debug port */
+	dw_pcie_read(qlpci->apb_base + PCIE_REGS_PCIE_SII_PM_STATE, 0x4, &val);
+
+	return (val & PCIE_REGS_PCIE_SII_LINK_UP) == PCIE_REGS_PCIE_SII_LINK_UP;
+}
+
+static int qilai_pcie_start_link(struct dw_pcie *pci)
+{
+	struct qilai_pcie *qlpci = to_qilai_pcie(pci);
+	u32 val;
+
+	/* Do phy link up */
+	dw_pcie_read(qlpci->apb_base + PCIE_GEN_CONTROL2, 0x4, &val);
+	val |= PCIE_CFG_LTSSM_EN;
+	dw_pcie_write(qlpci->apb_base + PCIE_GEN_CONTROL2, 0x4, val);
+
+	return 0;
+}
+
+static const struct dw_pcie_ops qilai_pcie_ops = {
+	.cpu_addr_fixup = qilai_pcie_cpu_addr_fixup,
+	.outbound_atu_check = qilai_pcie_outbound_atu_check,
+	.link_up = qilai_pcie_link_up,
+	.start_link = qilai_pcie_start_link,
+};
+
+static struct qilai_pcie *qilai_pcie_create_data(struct platform_device *pdev)
+{
+	struct qilai_pcie *qlpci;
+
+	qlpci = devm_kzalloc(&pdev->dev, sizeof(*qlpci), GFP_KERNEL);
+	if (!qlpci)
+		return ERR_PTR(-ENOMEM);
+
+	qlpci->pdev = pdev;
+	platform_set_drvdata(pdev, qlpci);
+
+	return qlpci;
+}
+
+/*
+ * Setup the Qilai PCIe IOCP (IO Coherence Port) Read/Write Behaviors to the
+ * Write-Back, Read and Write Allocate mode.
+ */
+static void qilai_pcie_iocp_cache_setup(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	u32 val;
+
+	dw_pcie_dbi_ro_wr_en(pci);
+
+	dw_pcie_read(pci->dbi_base + PCIE_LOGIC_COHERENCY_CONTROL3,
+		     sizeof(val), &val);
+	val |= FIELD_PREP(PCIE_CFG_MSTR_ARCACHE_MODE, IOCP_ARCACHE);
+	val |= FIELD_PREP(PCIE_CFG_MSTR_AWCACHE_MODE, IOCP_AWCACHE);
+	val |= FIELD_PREP(PCIE_CFG_MSTR_ARCACHE_VALUE, IOCP_ARCACHE);
+	val |= FIELD_PREP(PCIE_CFG_MSTR_AWCACHE_VALUE, IOCP_AWCACHE);
+	dw_pcie_write(pci->dbi_base + PCIE_LOGIC_COHERENCY_CONTROL3,
+		      sizeof(val), val);
+
+	dw_pcie_dbi_ro_wr_dis(pci);
+}
+
+static void qilai_pcie_enable_msi(struct qilai_pcie *qlpci)
+{
+	u32 val;
+
+	dw_pcie_read(qlpci->apb_base + PCIE_INTR_CONTROL1,
+		     sizeof(val), &val);
+	val |= PCIE_MSI_CTRL_INT_EN;
+	dw_pcie_write(qlpci->apb_base + PCIE_INTR_CONTROL1,
+		      sizeof(val), val);
+}
+
+static int qilai_pcie_host_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct qilai_pcie *qlpci = to_qilai_pcie(pci);
+
+	qilai_pcie_enable_msi(qlpci);
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops qilai_pcie_host_ops = {
+	.init = qilai_pcie_host_init,
+};
+
+static int qilai_pcie_add_port(struct qilai_pcie *qlpci)
+{
+	struct device *dev = &qlpci->pdev->dev;
+	struct platform_device *pdev = qlpci->pdev;
+	int ret;
+
+	qlpci->dw.dev = dev;
+	qlpci->dw.ops = &qilai_pcie_ops;
+	qlpci->dw.pp.num_vectors = MAX_MSI_IRQS;
+	qlpci->dw.pp.ops = &qilai_pcie_host_ops;
+
+	dw_pcie_cap_set(&qlpci->dw, REQ_RES);
+
+	qlpci->apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
+	if (IS_ERR(qlpci->apb_base))
+		return PTR_ERR(qlpci->apb_base);
+
+	ret = dw_pcie_host_init(&qlpci->dw.pp);
+	if (ret) {
+		dev_err_probe(dev, ret, "Failed to initialize PCIe host\n");
+		return ret;
+	}
+
+	qilai_pcie_iocp_cache_setup(&qlpci->dw.pp);
+
+	return 0;
+}
+
+static int qilai_pcie_probe(struct platform_device *pdev)
+{
+	struct qilai_pcie *qlpci;
+
+	qlpci = qilai_pcie_create_data(pdev);
+	if (IS_ERR(qlpci))
+		return PTR_ERR(qlpci);
+
+	platform_set_drvdata(pdev, qlpci);
+
+	return qilai_pcie_add_port(qlpci);
+}
+
+static const struct of_device_id qilai_pcie_of_match[] = {
+	{ .compatible = "andestech,qilai-pcie" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, qilai_pcie_of_match);
+
+static struct platform_driver qilai_pcie_driver = {
+	.probe = qilai_pcie_probe,
+	.driver = {
+		.name	= "qilai-pcie",
+		.of_match_table = qilai_pcie_of_match,
+	},
+};
+
+module_platform_driver(qilai_pcie_driver);
+
+MODULE_AUTHOR("Randolph Lin <randolph@andestech.com>");
+MODULE_DESCRIPTION("Andes Qilai PCIe driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH 5/6] PCI: andes: Add Andes QiLai SoC PCIe host driver support

[Bjorn Helgaas]

On Wed, Aug 20, 2025 at 07:18:42PM +0800, Randolph Lin wrote:
> Add driver support for DesignWare based PCIe controller in Andes
> QiLai SoC. The driver only supports the Root Complex mode.
> 
> Signed-off-by: Randolph Lin <randolph@andestech.com>
> ---
>  drivers/pci/controller/dwc/Kconfig            |  13 +
>  drivers/pci/controller/dwc/Makefile           |   1 +
>  drivers/pci/controller/dwc/pcie-andes-qilai.c | 227 ++++++++++++++++++
>  3 files changed, 241 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-andes-qilai.c
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index ff6b6d9e18ec..a9c5a43f648b 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -49,6 +49,19 @@ config PCIE_AMD_MDB
>  	  DesignWare IP and therefore the driver re-uses the DesignWare
>  	  core functions to implement the driver.
>  
> +config PCIE_ANDES_QILAI
> +	bool "ANDES QiLai PCIe controller"
> +	depends on OF && (RISCV || COMPILE_TEST)
> +	depends on PCI_MSI
> +	depends on ARCH_ANDES
> +	select PCIE_DW_HOST
> +	default y if ARCH_ANDES
> +	help
> +	  Say Y here if you want to enable PCIe controller support on Andes
> +	  QiLai SoCs. The Andes QiLai SoCs PCIe controller is based on
> +	  DesignWare IP and therefore the driver re-uses the DesignWare
> +	  core functions to implement the driver.
> +
>  config PCI_MESON
>  	tristate "Amlogic Meson PCIe controller"
>  	default m if ARCH_MESON
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index 6919d27798d1..de9583cbd675 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
>  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
>  obj-$(CONFIG_PCIE_AMD_MDB) += pcie-amd-mdb.o
> +obj-$(CONFIG_PCIE_ANDES_QILAI) += pcie-andes-qilai.o
>  obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o
>  obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>  obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
> diff --git a/drivers/pci/controller/dwc/pcie-andes-qilai.c b/drivers/pci/controller/dwc/pcie-andes-qilai.c
> new file mode 100644
> index 000000000000..dd06eee82cac
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-andes-qilai.c
> @@ -0,0 +1,227 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the PCIe Controller in QiLai from Andes
> + *
> + * Copyright (C) 2025 Andes Technology Corporation
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +#include "pcie-designware.h"
> +
> +#define PCIE_INTR_CONTROL1			0x15c
> +#define PCIE_MSI_CTRL_INT_EN			BIT(28)
> +
> +#define PCIE_LOGIC_COHERENCY_CONTROL3		0x8e8
> +/* Write-Back, Read and Write Allocate */
> +#define IOCP_ARCACHE				0xf
> +/* Write-Back, Read and Write Allocate */
> +#define IOCP_AWCACHE				0xf

Are IOCP_ARCACHE and IOCP_AWCACHE supposed to be identical values with
identical comments?

> +#define PCIE_CFG_MSTR_ARCACHE_MODE		GENMASK(6, 3)
> +#define PCIE_CFG_MSTR_AWCACHE_MODE		GENMASK(14, 11)
> +#define PCIE_CFG_MSTR_ARCACHE_VALUE		GENMASK(22, 19)
> +#define PCIE_CFG_MSTR_AWCACHE_VALUE		GENMASK(30, 27)
> +
> +#define PCIE_GEN_CONTROL2			0x54
> +#define PCIE_CFG_LTSSM_EN			BIT(0)
> +
> +#define PCIE_REGS_PCIE_SII_PM_STATE		0xc0
> +#define SMLH_LINK_UP				BIT(6)
> +#define RDLH_LINK_UP				BIT(7)
> +#define PCIE_REGS_PCIE_SII_LINK_UP		(SMLH_LINK_UP | RDLH_LINK_UP)
> +
> +struct qilai_pcie {
> +	struct dw_pcie dw;
> +	struct platform_device *pdev;
> +	void __iomem *apb_base;
> +};
> +
> +#define to_qilai_pcie(_dw) container_of(_dw, struct qilai_pcie, dw)
> +
> +static u64 qilai_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
> +{
> +	struct dw_pcie_rp *pp = &pci->pp;
> +
> +	return cpu_addr - pp->cfg0_base;

Sorry, we can't do this.  We're removing .cpu_addr_fixup() because
it's a workaround for defects in the DT description.  See these
commits, for example:

    befc86a0b354 ("PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()")
    b9812179f601 ("PCI: imx6: Remove imx_pcie_cpu_addr_fixup()")
    07ae413e169d ("PCI: intel-gw: Remove intel_pcie_cpu_addr()")

> +}
> +
> +static u32 qilai_pcie_outbound_atu_check(struct dw_pcie *pci,
> +					 const struct dw_pcie_ob_atu_cfg *atu,
> +					 u64 *limit_addr)
> +{
> +	u64 parent_bus_addr = atu->parent_bus_addr;
> +
> +	*limit_addr = parent_bus_addr + atu->size - 1;
> +
> +	/*
> +	 * Addresses below 4 GB are not 1:1 mapped; therefore, range checks
> +	 * only need to ensure addresses below 4 GB match pci->region_limit.
> +	 */
> +	if (lower_32_bits(*limit_addr & ~pci->region_limit) !=
> +	    lower_32_bits(parent_bus_addr & ~pci->region_limit) ||
> +	    !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
> +	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size)
> +		return -EINVAL;
> +	else
> +		return 0;

I'm a little dubious about this because the knowledge about 1:1
mapping feels like something we should know from devicetree.  But I'm
not an expert in this area.

If you do need this, write it like this, which I think is slightly
simpler than the if/else:

  if (...)
    return -EINVAL;

  return 0;

> +}
> +
> +static bool qilai_pcie_link_up(struct dw_pcie *pci)
> +{
> +	struct qilai_pcie *qlpci = to_qilai_pcie(pci);

Suggest using "pcie" as the name for "struct qilai_pcie *" pointers
because that's a common pattern in other drivers.

> +	u32 val;
> +
> +	/* Read smlh & rdlh link up by checking debug port */
> +	dw_pcie_read(qlpci->apb_base + PCIE_REGS_PCIE_SII_PM_STATE, 0x4, &val);
> +
> +	return (val & PCIE_REGS_PCIE_SII_LINK_UP) == PCIE_REGS_PCIE_SII_LINK_UP;
> +}
> +
> +static int qilai_pcie_start_link(struct dw_pcie *pci)
> +{
> +	struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> +	u32 val;
> +
> +	/* Do phy link up */
> +	dw_pcie_read(qlpci->apb_base + PCIE_GEN_CONTROL2, 0x4, &val);
> +	val |= PCIE_CFG_LTSSM_EN;
> +	dw_pcie_write(qlpci->apb_base + PCIE_GEN_CONTROL2, 0x4, val);
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_ops qilai_pcie_ops = {
> +	.cpu_addr_fixup = qilai_pcie_cpu_addr_fixup,
> +	.outbound_atu_check = qilai_pcie_outbound_atu_check,
> +	.link_up = qilai_pcie_link_up,
> +	.start_link = qilai_pcie_start_link,
> +};
> +
> +static struct qilai_pcie *qilai_pcie_create_data(struct platform_device *pdev)
> +{
> +	struct qilai_pcie *qlpci;
> +
> +	qlpci = devm_kzalloc(&pdev->dev, sizeof(*qlpci), GFP_KERNEL);
> +	if (!qlpci)
> +		return ERR_PTR(-ENOMEM);
> +
> +	qlpci->pdev = pdev;
> +	platform_set_drvdata(pdev, qlpci);
> +
> +	return qlpci;

Doesn't seem worth a separate function to me.  I see this is copied
from bt1_pcie_create_data(), and kudos for paying attention to how
other drivers do things.  But I don't think it's really worth it
there, either, and it's the only driver that does that.

Most drivers do it directly in *_pcie_probe(), which also makes it
possible to delay the platform_set_drvdata() until after qlpci is
initialized (and also makes it obvious that you do the
platform_set_drvdata() *twice* :)).

> +}
> +
> +/*
> + * Setup the Qilai PCIe IOCP (IO Coherence Port) Read/Write Behaviors to the
> + * Write-Back, Read and Write Allocate mode.
> + */
> +static void qilai_pcie_iocp_cache_setup(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	u32 val;
> +
> +	dw_pcie_dbi_ro_wr_en(pci);
> +
> +	dw_pcie_read(pci->dbi_base + PCIE_LOGIC_COHERENCY_CONTROL3,
> +		     sizeof(val), &val);
> +	val |= FIELD_PREP(PCIE_CFG_MSTR_ARCACHE_MODE, IOCP_ARCACHE);
> +	val |= FIELD_PREP(PCIE_CFG_MSTR_AWCACHE_MODE, IOCP_AWCACHE);
> +	val |= FIELD_PREP(PCIE_CFG_MSTR_ARCACHE_VALUE, IOCP_ARCACHE);
> +	val |= FIELD_PREP(PCIE_CFG_MSTR_AWCACHE_VALUE, IOCP_AWCACHE);

Since you never clear anything in "val", it looks like this assumes
the value you read from PCIE_LOGIC_COHERENCY_CONTROL3 contains zeroes
where you insert the new values.  Possibly use FIELD_MODIFY() instead?

> +	dw_pcie_write(pci->dbi_base + PCIE_LOGIC_COHERENCY_CONTROL3,
> +		      sizeof(val), val);
> +
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +}
> +
> +static void qilai_pcie_enable_msi(struct qilai_pcie *qlpci)
> +{
> +	u32 val;
> +
> +	dw_pcie_read(qlpci->apb_base + PCIE_INTR_CONTROL1,
> +		     sizeof(val), &val);
> +	val |= PCIE_MSI_CTRL_INT_EN;
> +	dw_pcie_write(qlpci->apb_base + PCIE_INTR_CONTROL1,
> +		      sizeof(val), val);

Apparently you don't need dw_pcie_dbi_ro_wr_en() and
dw_pcie_dbi_ro_wr_dis() around this?  Or maybe this relies on the DWC
core taking care of this, I dunno.

> +}
> +
> +static int qilai_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> +
> +	qilai_pcie_enable_msi(qlpci);
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_host_ops qilai_pcie_host_ops = {
> +	.init = qilai_pcie_host_init,
> +};
> +
> +static int qilai_pcie_add_port(struct qilai_pcie *qlpci)
> +{
> +	struct device *dev = &qlpci->pdev->dev;
> +	struct platform_device *pdev = qlpci->pdev;
> +	int ret;
> +
> +	qlpci->dw.dev = dev;
> +	qlpci->dw.ops = &qilai_pcie_ops;
> +	qlpci->dw.pp.num_vectors = MAX_MSI_IRQS;
> +	qlpci->dw.pp.ops = &qilai_pcie_host_ops;
> +
> +	dw_pcie_cap_set(&qlpci->dw, REQ_RES);
> +
> +	qlpci->apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
> +	if (IS_ERR(qlpci->apb_base))
> +		return PTR_ERR(qlpci->apb_base);
> +
> +	ret = dw_pcie_host_init(&qlpci->dw.pp);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to initialize PCIe host\n");
> +		return ret;
> +	}
> +
> +	qilai_pcie_iocp_cache_setup(&qlpci->dw.pp);
> +
> +	return 0;
> +}
> +
> +static int qilai_pcie_probe(struct platform_device *pdev)
> +{
> +	struct qilai_pcie *qlpci;
> +
> +	qlpci = qilai_pcie_create_data(pdev);
> +	if (IS_ERR(qlpci))
> +		return PTR_ERR(qlpci);
> +
> +	platform_set_drvdata(pdev, qlpci);
> +
> +	return qilai_pcie_add_port(qlpci);
> +}
> +
> +static const struct of_device_id qilai_pcie_of_match[] = {
> +	{ .compatible = "andestech,qilai-pcie" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, qilai_pcie_of_match);
> +
> +static struct platform_driver qilai_pcie_driver = {
> +	.probe = qilai_pcie_probe,
> +	.driver = {
> +		.name	= "qilai-pcie",
> +		.of_match_table = qilai_pcie_of_match,
> +	},
> +};
> +
> +module_platform_driver(qilai_pcie_driver);
> +
> +MODULE_AUTHOR("Randolph Lin <randolph@andestech.com>");
> +MODULE_DESCRIPTION("Andes Qilai PCIe driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 

Re: [PATCH 5/6] PCI: andes: Add Andes QiLai SoC PCIe host driver support

[Randolph Lin]

Hi Bjorn Helgaas,

Thank you for the feedback. I will update the changes accordingly.

Rn Wed, Aug 20, 2025 at 10:54:44AM -0500, Bjorn Helgaas wrote:
> [EXTERNAL MAIL]
> 
> On Wed, Aug 20, 2025 at 07:18:42PM +0800, Randolph Lin wrote:
> > Add driver support for DesignWare based PCIe controller in Andes
> > QiLai SoC. The driver only supports the Root Complex mode.
> >
> > Signed-off-by: Randolph Lin <randolph@andestech.com>
> > ---
> >  drivers/pci/controller/dwc/Kconfig            |  13 +
> >  drivers/pci/controller/dwc/Makefile           |   1 +
> >  drivers/pci/controller/dwc/pcie-andes-qilai.c | 227 ++++++++++++++++++
> >  3 files changed, 241 insertions(+)
> >  create mode 100644 drivers/pci/controller/dwc/pcie-andes-qilai.c
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > index ff6b6d9e18ec..a9c5a43f648b 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -49,6 +49,19 @@ config PCIE_AMD_MDB
> >         DesignWare IP and therefore the driver re-uses the DesignWare
> >         core functions to implement the driver.
> >
> > +config PCIE_ANDES_QILAI
> > +     bool "ANDES QiLai PCIe controller"
> > +     depends on OF && (RISCV || COMPILE_TEST)
> > +     depends on PCI_MSI
> > +     depends on ARCH_ANDES
> > +     select PCIE_DW_HOST
> > +     default y if ARCH_ANDES
> > +     help
> > +       Say Y here if you want to enable PCIe controller support on Andes
> > +       QiLai SoCs. The Andes QiLai SoCs PCIe controller is based on
> > +       DesignWare IP and therefore the driver re-uses the DesignWare
> > +       core functions to implement the driver.
> > +
> >  config PCI_MESON
> >       tristate "Amlogic Meson PCIe controller"
> >       default m if ARCH_MESON
> > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > index 6919d27798d1..de9583cbd675 100644
> > --- a/drivers/pci/controller/dwc/Makefile
> > +++ b/drivers/pci/controller/dwc/Makefile
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
> >  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
> >  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
> >  obj-$(CONFIG_PCIE_AMD_MDB) += pcie-amd-mdb.o
> > +obj-$(CONFIG_PCIE_ANDES_QILAI) += pcie-andes-qilai.o
> >  obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o
> >  obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
> >  obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
> > diff --git a/drivers/pci/controller/dwc/pcie-andes-qilai.c b/drivers/pci/controller/dwc/pcie-andes-qilai.c
> > new file mode 100644
> > index 000000000000..dd06eee82cac
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-andes-qilai.c
> > @@ -0,0 +1,227 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for the PCIe Controller in QiLai from Andes
> > + *
> > + * Copyright (C) 2025 Andes Technology Corporation
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/types.h>
> > +
> > +#include "pcie-designware.h"
> > +
> > +#define PCIE_INTR_CONTROL1                   0x15c
> > +#define PCIE_MSI_CTRL_INT_EN                 BIT(28)
> > +
> > +#define PCIE_LOGIC_COHERENCY_CONTROL3                0x8e8
> > +/* Write-Back, Read and Write Allocate */
> > +#define IOCP_ARCACHE                         0xf
> > +/* Write-Back, Read and Write Allocate */
> > +#define IOCP_AWCACHE                         0xf
> 
> Are IOCP_ARCACHE and IOCP_AWCACHE supposed to be identical values with
> identical comments?
> 
> > +#define PCIE_CFG_MSTR_ARCACHE_MODE           GENMASK(6, 3)
> > +#define PCIE_CFG_MSTR_AWCACHE_MODE           GENMASK(14, 11)
> > +#define PCIE_CFG_MSTR_ARCACHE_VALUE          GENMASK(22, 19)
> > +#define PCIE_CFG_MSTR_AWCACHE_VALUE          GENMASK(30, 27)
> > +
> > +#define PCIE_GEN_CONTROL2                    0x54
> > +#define PCIE_CFG_LTSSM_EN                    BIT(0)
> > +
> > +#define PCIE_REGS_PCIE_SII_PM_STATE          0xc0
> > +#define SMLH_LINK_UP                         BIT(6)
> > +#define RDLH_LINK_UP                         BIT(7)
> > +#define PCIE_REGS_PCIE_SII_LINK_UP           (SMLH_LINK_UP | RDLH_LINK_UP)
> > +
> > +struct qilai_pcie {
> > +     struct dw_pcie dw;
> > +     struct platform_device *pdev;
> > +     void __iomem *apb_base;
> > +};
> > +
> > +#define to_qilai_pcie(_dw) container_of(_dw, struct qilai_pcie, dw)
> > +
> > +static u64 qilai_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
> > +{
> > +     struct dw_pcie_rp *pp = &pci->pp;
> > +
> > +     return cpu_addr - pp->cfg0_base;
> 
> Sorry, we can't do this.  We're removing .cpu_addr_fixup() because
> it's a workaround for defects in the DT description.  See these
> commits, for example:
> 
>     befc86a0b354 ("PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()")
>     b9812179f601 ("PCI: imx6: Remove imx_pcie_cpu_addr_fixup()")
>     07ae413e169d ("PCI: intel-gw: Remove intel_pcie_cpu_addr()")
> 

I’m a bit confused about the following question:
After removing cpu_addr_fixup, should we use pci->parent_bus_offset to store the
offset value, or should pci->parent_bus_offset remain 0?

In the commit message:
befc86a0b354 ("PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()")
    We know the parent_bus_offset, either computed from a DT reg property (the
    offset is the CPU physical addr - the 'config'/'addr_space' address on the
    parent bus) or from a .cpu_addr_fixup() (which may have used a host bridge
    window offset).

We know that "the offset is the CPU physical addr - the 'config'/'addr_space'
address on the parent bus".

However, in dw_pcie_host_get_resources(), it passes pp->cfg0_base, which is
parsed from the device tree using "config", as the cpu_phys_addr parameter to
dw_pcie_parent_bus_offset(). It also passes "config" as the 2nd parameter to
dw_pcie_parent_bus_offset().

In dw_pcie_parent_bus_offset(), the 2nd parameter is used to get the index
from the devicetree "reg-names" field, and the result is used as the
'config'/'addr_space' address. 

It seems that the same value is being obtained through a different method,
and the return value appears to be 0.
Could I be misunderstanding something?

> > +}
> > +
> > +static u32 qilai_pcie_outbound_atu_check(struct dw_pcie *pci,
> > +                                      const struct dw_pcie_ob_atu_cfg *atu,
> > +                                      u64 *limit_addr)
> > +{
> > +     u64 parent_bus_addr = atu->parent_bus_addr;
> > +
> > +     *limit_addr = parent_bus_addr + atu->size - 1;
> > +
> > +     /*
> > +      * Addresses below 4 GB are not 1:1 mapped; therefore, range checks
> > +      * only need to ensure addresses below 4 GB match pci->region_limit.
> > +      */
> > +     if (lower_32_bits(*limit_addr & ~pci->region_limit) !=
> > +         lower_32_bits(parent_bus_addr & ~pci->region_limit) ||
> > +         !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
> > +         !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size)
> > +             return -EINVAL;
> > +     else
> > +             return 0;
> 
> I'm a little dubious about this because the knowledge about 1:1
> mapping feels like something we should know from devicetree.  But I'm
> not an expert in this area.
> 
> If you do need this, write it like this, which I think is slightly
> simpler than the if/else:
> 
>   if (...)
>     return -EINVAL;
> 
>   return 0;
> 
> > +}
> > +
> > +static bool qilai_pcie_link_up(struct dw_pcie *pci)
> > +{
> > +     struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> 
> Suggest using "pcie" as the name for "struct qilai_pcie *" pointers
> because that's a common pattern in other drivers.
> 
> > +     u32 val;
> > +
> > +     /* Read smlh & rdlh link up by checking debug port */
> > +     dw_pcie_read(qlpci->apb_base + PCIE_REGS_PCIE_SII_PM_STATE, 0x4, &val);
> > +
> > +     return (val & PCIE_REGS_PCIE_SII_LINK_UP) == PCIE_REGS_PCIE_SII_LINK_UP;
> > +}
> > +
> > +static int qilai_pcie_start_link(struct dw_pcie *pci)
> > +{
> > +     struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> > +     u32 val;
> > +
> > +     /* Do phy link up */
> > +     dw_pcie_read(qlpci->apb_base + PCIE_GEN_CONTROL2, 0x4, &val);
> > +     val |= PCIE_CFG_LTSSM_EN;
> > +     dw_pcie_write(qlpci->apb_base + PCIE_GEN_CONTROL2, 0x4, val);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct dw_pcie_ops qilai_pcie_ops = {
> > +     .cpu_addr_fixup = qilai_pcie_cpu_addr_fixup,
> > +     .outbound_atu_check = qilai_pcie_outbound_atu_check,
> > +     .link_up = qilai_pcie_link_up,
> > +     .start_link = qilai_pcie_start_link,
> > +};
> > +
> > +static struct qilai_pcie *qilai_pcie_create_data(struct platform_device *pdev)
> > +{
> > +     struct qilai_pcie *qlpci;
> > +
> > +     qlpci = devm_kzalloc(&pdev->dev, sizeof(*qlpci), GFP_KERNEL);
> > +     if (!qlpci)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     qlpci->pdev = pdev;
> > +     platform_set_drvdata(pdev, qlpci);
> > +
> > +     return qlpci;
> 
> Doesn't seem worth a separate function to me.  I see this is copied
> from bt1_pcie_create_data(), and kudos for paying attention to how
> other drivers do things.  But I don't think it's really worth it
> there, either, and it's the only driver that does that.
> 
> Most drivers do it directly in *_pcie_probe(), which also makes it
> possible to delay the platform_set_drvdata() until after qlpci is
> initialized (and also makes it obvious that you do the
> platform_set_drvdata() *twice* :)).
> 
> > +}
> > +
> > +/*
> > + * Setup the Qilai PCIe IOCP (IO Coherence Port) Read/Write Behaviors to the
> > + * Write-Back, Read and Write Allocate mode.
> > + */
> > +static void qilai_pcie_iocp_cache_setup(struct dw_pcie_rp *pp)
> > +{
> > +     struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +     u32 val;
> > +
> > +     dw_pcie_dbi_ro_wr_en(pci);
> > +
> > +     dw_pcie_read(pci->dbi_base + PCIE_LOGIC_COHERENCY_CONTROL3,
> > +                  sizeof(val), &val);
> > +     val |= FIELD_PREP(PCIE_CFG_MSTR_ARCACHE_MODE, IOCP_ARCACHE);
> > +     val |= FIELD_PREP(PCIE_CFG_MSTR_AWCACHE_MODE, IOCP_AWCACHE);
> > +     val |= FIELD_PREP(PCIE_CFG_MSTR_ARCACHE_VALUE, IOCP_ARCACHE);
> > +     val |= FIELD_PREP(PCIE_CFG_MSTR_AWCACHE_VALUE, IOCP_AWCACHE);
> 
> Since you never clear anything in "val", it looks like this assumes
> the value you read from PCIE_LOGIC_COHERENCY_CONTROL3 contains zeroes
> where you insert the new values.  Possibly use FIELD_MODIFY() instead?
> 
> > +     dw_pcie_write(pci->dbi_base + PCIE_LOGIC_COHERENCY_CONTROL3,
> > +                   sizeof(val), val);
> > +
> > +     dw_pcie_dbi_ro_wr_dis(pci);
> > +}
> > +
> > +static void qilai_pcie_enable_msi(struct qilai_pcie *qlpci)
> > +{
> > +     u32 val;
> > +
> > +     dw_pcie_read(qlpci->apb_base + PCIE_INTR_CONTROL1,
> > +                  sizeof(val), &val);
> > +     val |= PCIE_MSI_CTRL_INT_EN;
> > +     dw_pcie_write(qlpci->apb_base + PCIE_INTR_CONTROL1,
> > +                   sizeof(val), val);
> 
> Apparently you don't need dw_pcie_dbi_ro_wr_en() and
> dw_pcie_dbi_ro_wr_dis() around this?  Or maybe this relies on the DWC
> core taking care of this, I dunno.
> 

Maybe these are not the DBI registers. 
I think the API is wrong, that's why you confused.
I will use readl() and writel() to instead of it. 

> > +}
> > +
> > +static int qilai_pcie_host_init(struct dw_pcie_rp *pp)
> > +{
> > +     struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +     struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> > +
> > +     qilai_pcie_enable_msi(qlpci);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct dw_pcie_host_ops qilai_pcie_host_ops = {
> > +     .init = qilai_pcie_host_init,
> > +};
> > +
> > +static int qilai_pcie_add_port(struct qilai_pcie *qlpci)
> > +{
> > +     struct device *dev = &qlpci->pdev->dev;
> > +     struct platform_device *pdev = qlpci->pdev;
> > +     int ret;
> > +
> > +     qlpci->dw.dev = dev;
> > +     qlpci->dw.ops = &qilai_pcie_ops;
> > +     qlpci->dw.pp.num_vectors = MAX_MSI_IRQS;
> > +     qlpci->dw.pp.ops = &qilai_pcie_host_ops;
> > +
> > +     dw_pcie_cap_set(&qlpci->dw, REQ_RES);
> > +
> > +     qlpci->apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
> > +     if (IS_ERR(qlpci->apb_base))
> > +             return PTR_ERR(qlpci->apb_base);
> > +
> > +     ret = dw_pcie_host_init(&qlpci->dw.pp);
> > +     if (ret) {
> > +             dev_err_probe(dev, ret, "Failed to initialize PCIe host\n");
> > +             return ret;
> > +     }
> > +
> > +     qilai_pcie_iocp_cache_setup(&qlpci->dw.pp);
> > +
> > +     return 0;
> > +}
> > +
> > +static int qilai_pcie_probe(struct platform_device *pdev)
> > +{
> > +     struct qilai_pcie *qlpci;
> > +
> > +     qlpci = qilai_pcie_create_data(pdev);
> > +     if (IS_ERR(qlpci))
> > +             return PTR_ERR(qlpci);
> > +
> > +     platform_set_drvdata(pdev, qlpci);
> > +
> > +     return qilai_pcie_add_port(qlpci);
> > +}
> > +
> > +static const struct of_device_id qilai_pcie_of_match[] = {
> > +     { .compatible = "andestech,qilai-pcie" },
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, qilai_pcie_of_match);
> > +
> > +static struct platform_driver qilai_pcie_driver = {
> > +     .probe = qilai_pcie_probe,
> > +     .driver = {
> > +             .name   = "qilai-pcie",
> > +             .of_match_table = qilai_pcie_of_match,
> > +     },
> > +};
> > +
> > +module_platform_driver(qilai_pcie_driver);
> > +
> > +MODULE_AUTHOR("Randolph Lin <randolph@andestech.com>");
> > +MODULE_DESCRIPTION("Andes Qilai PCIe driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> >

Sincerely,
Randolph Lin

Re: [PATCH 5/6] PCI: andes: Add Andes QiLai SoC PCIe host driver support

[Bjorn Helgaas]

[+cc Frank, who can probably answer faster than I can]

On Fri, Aug 29, 2025 at 05:41:17PM +0800, Randolph Lin wrote:
> Rn Wed, Aug 20, 2025 at 10:54:44AM -0500, Bjorn Helgaas wrote:
> > On Wed, Aug 20, 2025 at 07:18:42PM +0800, Randolph Lin wrote:
> > > Add driver support for DesignWare based PCIe controller in Andes
> > > QiLai SoC. The driver only supports the Root Complex mode.

> > > +#define PCIE_LOGIC_COHERENCY_CONTROL3                0x8e8
> > > +/* Write-Back, Read and Write Allocate */
> > > +#define IOCP_ARCACHE                         0xf
> > > +/* Write-Back, Read and Write Allocate */
> > > +#define IOCP_AWCACHE                         0xf
> > 
> > Are IOCP_ARCACHE and IOCP_AWCACHE supposed to be identical values with
> > identical comments?

Just pointing this out since you didn't respond to it.

> > > +static u64 qilai_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
> > > +{
> > > +     struct dw_pcie_rp *pp = &pci->pp;
> > > +
> > > +     return cpu_addr - pp->cfg0_base;
> > 
> > Sorry, we can't do this.  We're removing .cpu_addr_fixup() because
> > it's a workaround for defects in the DT description.  See these
> > commits, for example:
> > 
> >     befc86a0b354 ("PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()")
> >     b9812179f601 ("PCI: imx6: Remove imx_pcie_cpu_addr_fixup()")
> >     07ae413e169d ("PCI: intel-gw: Remove intel_pcie_cpu_addr()")
> 
> I’m a bit confused about the following question:
> After removing cpu_addr_fixup, should we use pci->parent_bus_offset
> to store the offset value, or should pci->parent_bus_offset remain
> 0?

If you needed qilai_pcie_cpu_addr_fixup(), I would expect your
dw_pcie.parent_bus_offset to be non-zero because parent_bus_offset is
used instead of ->cpu_addr_fixup().

> In the commit message:
> befc86a0b354 ("PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()")
>     We know the parent_bus_offset, either computed from a DT reg
>     property (the offset is the CPU physical addr - the
>     'config'/'addr_space' address on the parent bus) or from a
>     .cpu_addr_fixup() (which may have used a host bridge window
>     offset).
> 
> We know that "the offset is the CPU physical addr - the
> 'config'/'addr_space' address on the parent bus".
> 
> However, in dw_pcie_host_get_resources(), it passes pp->cfg0_base,
> which is parsed from the device tree using "config", as the
> cpu_phys_addr parameter to dw_pcie_parent_bus_offset(). It also
> passes "config" as the 2nd parameter to dw_pcie_parent_bus_offset().
> 
> In dw_pcie_parent_bus_offset(), the 2nd parameter is used to get the
> index from the devicetree "reg-names" field, and the result is used
> as the 'config'/'addr_space' address. 
> 
> It seems that the same value is being obtained through a different
> method, and the return value appears to be 0.  Could I be
> misunderstanding something?

Re: [PATCH 5/6] PCI: andes: Add Andes QiLai SoC PCIe host driver support

[Randolph Lin]

Hi Bjorn and Frank,

On Mon, Sep 08, 2025 at 05:07:37PM -0500, Bjorn Helgaas wrote:
> [EXTERNAL MAIL]
> 
> [+cc Frank, who can probably answer faster than I can]
> 
> On Fri, Aug 29, 2025 at 05:41:17PM +0800, Randolph Lin wrote:
> > Rn Wed, Aug 20, 2025 at 10:54:44AM -0500, Bjorn Helgaas wrote:
> > > On Wed, Aug 20, 2025 at 07:18:42PM +0800, Randolph Lin wrote:
> > > > Add driver support for DesignWare based PCIe controller in Andes
> > > > QiLai SoC. The driver only supports the Root Complex mode.
> 
> > > > +#define PCIE_LOGIC_COHERENCY_CONTROL3                0x8e8
> > > > +/* Write-Back, Read and Write Allocate */
> > > > +#define IOCP_ARCACHE                         0xf
> > > > +/* Write-Back, Read and Write Allocate */
> > > > +#define IOCP_AWCACHE                         0xf
> > >
> > > Are IOCP_ARCACHE and IOCP_AWCACHE supposed to be identical values with
> > > identical comments?
> 
> Just pointing this out since you didn't respond to it.
> 

Sorry, I originally intended to add a comment in the V2 patch.

The following shows the modified version:
/*
 * Refer to Table A4-5 (Memory type encoding) in the
 * AMBA AXI and ACE Protocol Specification.
 * The selected value corresponds to the Memory type field:
 * "Write-back, Read and Write-allocate".
 */
#define IOCP_ARCACHE				0b1111
#define IOCP_AWCACHE				0b1111


Refer to the AMBA AXI and ACE Protocol Specification.
The last three rows in the table A4-5:
ARCACHE         AWCACHE         Memory type
1111 (0111)     0111            Write-back Read-allocate
1011            1111 (1011)     Write-back Write-allocate
1111            1111            Write-back Read and Write-allocate

I know that the following are meaningless.
1. Read-allocate in AWCACHE 
2. Write-allocate in ARCACHE
However, I want to set it up as "Write-back, Read and Write-allocate";
therefore, it seems they have the same comment.

> > > > +static u64 qilai_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
> > > > +{
> > > > +     struct dw_pcie_rp *pp = &pci->pp;
> > > > +
> > > > +     return cpu_addr - pp->cfg0_base;
> > >
> > > Sorry, we can't do this.  We're removing .cpu_addr_fixup() because
> > > it's a workaround for defects in the DT description.  See these
> > > commits, for example:
> > >
> > >     befc86a0b354 ("PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()")
> > >     b9812179f601 ("PCI: imx6: Remove imx_pcie_cpu_addr_fixup()")
> > >     07ae413e169d ("PCI: intel-gw: Remove intel_pcie_cpu_addr()")
> >
> > I’m a bit confused about the following question:
> > After removing cpu_addr_fixup, should we use pci->parent_bus_offset
> > to store the offset value, or should pci->parent_bus_offset remain
> > 0?
> 
> If you needed qilai_pcie_cpu_addr_fixup(), I would expect your
> dw_pcie.parent_bus_offset to be non-zero because parent_bus_offset is
> used instead of ->cpu_addr_fixup().
> 

In this SoC, the dw_pcie.parent_bus_offset should be set to the value of the
config register base address. Setting the dw_pcie.parent_bus_offset to a
non-zero value seems to occur only in the path that uses ->cpu_addr_fixup().

The parent_bus_offset is generally used instead of ->cpu_addr_fixup() throughout
most of the code, but its assignment still seems to rely on ->cpu_addr_fixup()
when it needs to be set to a non-zero value.
If no other method exists to set up dw_pcie.parent_bus_offset, can we keep using
->cpu_addr_fixup() for the assignment?

> > In the commit message:
> > befc86a0b354 ("PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()")
> >     We know the parent_bus_offset, either computed from a DT reg
> >     property (the offset is the CPU physical addr - the
> >     'config'/'addr_space' address on the parent bus) or from a
> >     .cpu_addr_fixup() (which may have used a host bridge window
> >     offset).
> >
> > We know that "the offset is the CPU physical addr - the
> > 'config'/'addr_space' address on the parent bus".
> >
> > However, in dw_pcie_host_get_resources(), it passes pp->cfg0_base,
> > which is parsed from the device tree using "config", as the
> > cpu_phys_addr parameter to dw_pcie_parent_bus_offset(). It also
> > passes "config" as the 2nd parameter to dw_pcie_parent_bus_offset().
> >
> > In dw_pcie_parent_bus_offset(), the 2nd parameter is used to get the
> > index from the devicetree "reg-names" field, and the result is used
> > as the 'config'/'addr_space' address.
> >
> > It seems that the same value is being obtained through a different
> > method, and the return value appears to be 0.  Could I be
> > misunderstanding something?
> 

Sincerely,
Randolph

Re: [PATCH 5/6] PCI: andes: Add Andes QiLai SoC PCIe host driver support

[Frank Li]

On Tue, Sep 09, 2025 at 04:14:48PM +0800, Randolph Lin wrote:
> Hi Bjorn and Frank,
>
> On Mon, Sep 08, 2025 at 05:07:37PM -0500, Bjorn Helgaas wrote:
> > [EXTERNAL MAIL]
> >
> > [+cc Frank, who can probably answer faster than I can]
> >
...
> > > >
> > > > Sorry, we can't do this.  We're removing .cpu_addr_fixup() because
> > > > it's a workaround for defects in the DT description.  See these
> > > > commits, for example:
> > > >
> > > >     befc86a0b354 ("PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()")
> > > >     b9812179f601 ("PCI: imx6: Remove imx_pcie_cpu_addr_fixup()")
> > > >     07ae413e169d ("PCI: intel-gw: Remove intel_pcie_cpu_addr()")
> > >
> > > I’m a bit confused about the following question:
> > > After removing cpu_addr_fixup, should we use pci->parent_bus_offset
> > > to store the offset value, or should pci->parent_bus_offset remain
> > > 0?
> >
> > If you needed qilai_pcie_cpu_addr_fixup(), I would expect your
> > dw_pcie.parent_bus_offset to be non-zero because parent_bus_offset is
> > used instead of ->cpu_addr_fixup().
> >
>
> In this SoC, the dw_pcie.parent_bus_offset should be set to the value of the
> config register base address. Setting the dw_pcie.parent_bus_offset to a
> non-zero value seems to occur only in the path that uses ->cpu_addr_fixup().
>
> The parent_bus_offset is generally used instead of ->cpu_addr_fixup() throughout
> most of the code, but its assignment still seems to rely on ->cpu_addr_fixup()
> when it needs to be set to a non-zero value.
> If no other method exists to set up dw_pcie.parent_bus_offset, can we keep using
> ->cpu_addr_fixup() for the assignment?

Correct your dts file. See below

>
> > > In the commit message:
> > > befc86a0b354 ("PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()")
> > >     We know the parent_bus_offset, either computed from a DT reg
> > >     property (the offset is the CPU physical addr - the
> > >     'config'/'addr_space' address on the parent bus) or from a
> > >     .cpu_addr_fixup() (which may have used a host bridge window
> > >     offset).
> > >
> > > We know that "the offset is the CPU physical addr - the
> > > 'config'/'addr_space' address on the parent bus".
> > >
> > > However, in dw_pcie_host_get_resources(), it passes pp->cfg0_base,
> > > which is parsed from the device tree using "config", as the
> > > cpu_phys_addr parameter to dw_pcie_parent_bus_offset(). It also
> > > passes "config" as the 2nd parameter to dw_pcie_parent_bus_offset().
> > >
> > > In dw_pcie_parent_bus_offset(), the 2nd parameter is used to get the
> > > index from the devicetree "reg-names" field, and the result is used
> > > as the 'config'/'addr_space' address.
> > >
> > > It seems that the same value is being obtained through a different
> > > method, and the return value appears to be 0.  Could I be
> > > misunderstanding something?

I missed prevous discussion. Actually address convert happen at PCIe's parent
bus fabric. If your DTS correct reflect this address convert, you needn't
cpu_addr_fixup() at all.

bus {
	ranges = (0x2000_00000, 0x1000_0000, 0x1000_0000)
	^^^^^

	pcie@xxxx {
		reg = <0x2000_00000, 0x1000>;
		reg-names = <config>;
	}
}

0x2000_0000 is actaully address input to your PCI controller although
CPU use 0x1000_0000.  "bus" fabric convert 0x1000_0000 to 0x2000_0000

So dwc common code already can handle this to auto calucate
parent_bus_offset base on ranges of bus.

Frank

> >
>
> Sincerely,
> Randolph

Re: [PATCH 5/6] PCI: andes: Add Andes QiLai SoC PCIe host driver support

[Manivannan Sadhasivam]

On Wed, Aug 20, 2025 at 07:18:42PM GMT, Randolph Lin wrote:

On top of Bjorn's review...

> Add driver support for DesignWare based PCIe controller in Andes
> QiLai SoC. The driver only supports the Root Complex mode.
> 

Add more info about the controller. Like data rate supported, lanes, DWC IP
version, any quirks etc...

> Signed-off-by: Randolph Lin <randolph@andestech.com>
> ---
>  drivers/pci/controller/dwc/Kconfig            |  13 +
>  drivers/pci/controller/dwc/Makefile           |   1 +
>  drivers/pci/controller/dwc/pcie-andes-qilai.c | 227 ++++++++++++++++++
>  3 files changed, 241 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-andes-qilai.c
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index ff6b6d9e18ec..a9c5a43f648b 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -49,6 +49,19 @@ config PCIE_AMD_MDB
>  	  DesignWare IP and therefore the driver re-uses the DesignWare
>  	  core functions to implement the driver.
>  
> +config PCIE_ANDES_QILAI
> +	bool "ANDES QiLai PCIe controller"

No need to make it always built-in. You can make it as a loadable module, but
prevent unloading during runtime (by using builtin_platform_driver) as it
implements MSI controller. We do not allows irqchip controllers to be unloaded
during runtime.

> +	depends on OF && (RISCV || COMPILE_TEST)
> +	depends on PCI_MSI
> +	depends on ARCH_ANDES
> +	select PCIE_DW_HOST
> +	default y if ARCH_ANDES
> +	help
> +	  Say Y here if you want to enable PCIe controller support on Andes

Mention 'Root Complex' mode.

> +	  QiLai SoCs. The Andes QiLai SoCs PCIe controller is based on
> +	  DesignWare IP and therefore the driver re-uses the DesignWare
> +	  core functions to implement the driver.
> +
>  config PCI_MESON
>  	tristate "Amlogic Meson PCIe controller"
>  	default m if ARCH_MESON
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index 6919d27798d1..de9583cbd675 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
>  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
>  obj-$(CONFIG_PCIE_AMD_MDB) += pcie-amd-mdb.o
> +obj-$(CONFIG_PCIE_ANDES_QILAI) += pcie-andes-qilai.o
>  obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o
>  obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>  obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
> diff --git a/drivers/pci/controller/dwc/pcie-andes-qilai.c b/drivers/pci/controller/dwc/pcie-andes-qilai.c
> new file mode 100644
> index 000000000000..dd06eee82cac
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-andes-qilai.c
> @@ -0,0 +1,227 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the PCIe Controller in QiLai from Andes
> + *
> + * Copyright (C) 2025 Andes Technology Corporation
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +#include "pcie-designware.h"
> +
> +#define PCIE_INTR_CONTROL1			0x15c
> +#define PCIE_MSI_CTRL_INT_EN			BIT(28)
> +
> +#define PCIE_LOGIC_COHERENCY_CONTROL3		0x8e8
> +/* Write-Back, Read and Write Allocate */
> +#define IOCP_ARCACHE				0xf
> +/* Write-Back, Read and Write Allocate */
> +#define IOCP_AWCACHE				0xf
> +#define PCIE_CFG_MSTR_ARCACHE_MODE		GENMASK(6, 3)
> +#define PCIE_CFG_MSTR_AWCACHE_MODE		GENMASK(14, 11)
> +#define PCIE_CFG_MSTR_ARCACHE_VALUE		GENMASK(22, 19)
> +#define PCIE_CFG_MSTR_AWCACHE_VALUE		GENMASK(30, 27)
> +
> +#define PCIE_GEN_CONTROL2			0x54
> +#define PCIE_CFG_LTSSM_EN			BIT(0)
> +
> +#define PCIE_REGS_PCIE_SII_PM_STATE		0xc0
> +#define SMLH_LINK_UP				BIT(6)
> +#define RDLH_LINK_UP				BIT(7)
> +#define PCIE_REGS_PCIE_SII_LINK_UP		(SMLH_LINK_UP | RDLH_LINK_UP)
> +
> +struct qilai_pcie {
> +	struct dw_pcie dw;
> +	struct platform_device *pdev;
> +	void __iomem *apb_base;
> +};
> +
> +#define to_qilai_pcie(_dw) container_of(_dw, struct qilai_pcie, dw)
> +
> +static u64 qilai_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
> +{
> +	struct dw_pcie_rp *pp = &pci->pp;
> +
> +	return cpu_addr - pp->cfg0_base;
> +}
> +
> +static u32 qilai_pcie_outbound_atu_check(struct dw_pcie *pci,
> +					 const struct dw_pcie_ob_atu_cfg *atu,
> +					 u64 *limit_addr)
> +{
> +	u64 parent_bus_addr = atu->parent_bus_addr;
> +
> +	*limit_addr = parent_bus_addr + atu->size - 1;
> +
> +	/*
> +	 * Addresses below 4 GB are not 1:1 mapped; therefore, range checks
> +	 * only need to ensure addresses below 4 GB match pci->region_limit.
> +	 */
> +	if (lower_32_bits(*limit_addr & ~pci->region_limit) !=
> +	    lower_32_bits(parent_bus_addr & ~pci->region_limit) ||
> +	    !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
> +	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size)
> +		return -EINVAL;
> +	else
> +		return 0;
> +}
> +
> +static bool qilai_pcie_link_up(struct dw_pcie *pci)
> +{
> +	struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> +	u32 val;
> +
> +	/* Read smlh & rdlh link up by checking debug port */
> +	dw_pcie_read(qlpci->apb_base + PCIE_REGS_PCIE_SII_PM_STATE, 0x4, &val);
> +
> +	return (val & PCIE_REGS_PCIE_SII_LINK_UP) == PCIE_REGS_PCIE_SII_LINK_UP;
> +}
> +
> +static int qilai_pcie_start_link(struct dw_pcie *pci)
> +{
> +	struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> +	u32 val;
> +
> +	/* Do phy link up */

Drop the comment.

> +	dw_pcie_read(qlpci->apb_base + PCIE_GEN_CONTROL2, 0x4, &val);
> +	val |= PCIE_CFG_LTSSM_EN;
> +	dw_pcie_write(qlpci->apb_base + PCIE_GEN_CONTROL2, 0x4, val);
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_ops qilai_pcie_ops = {
> +	.cpu_addr_fixup = qilai_pcie_cpu_addr_fixup,
> +	.outbound_atu_check = qilai_pcie_outbound_atu_check,
> +	.link_up = qilai_pcie_link_up,
> +	.start_link = qilai_pcie_start_link,
> +};
> +
> +static struct qilai_pcie *qilai_pcie_create_data(struct platform_device *pdev)
> +{
> +	struct qilai_pcie *qlpci;
> +
> +	qlpci = devm_kzalloc(&pdev->dev, sizeof(*qlpci), GFP_KERNEL);
> +	if (!qlpci)
> +		return ERR_PTR(-ENOMEM);
> +
> +	qlpci->pdev = pdev;
> +	platform_set_drvdata(pdev, qlpci);
> +
> +	return qlpci;
> +}
> +
> +/*
> + * Setup the Qilai PCIe IOCP (IO Coherence Port) Read/Write Behaviors to the
> + * Write-Back, Read and Write Allocate mode.
> + */

What caches does the IOCP target? Is it the shared system cache? Does it allow
the PCIe transactions to snoop the cache?

> +static void qilai_pcie_iocp_cache_setup(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	u32 val;
> +
> +	dw_pcie_dbi_ro_wr_en(pci);
> +
> +	dw_pcie_read(pci->dbi_base + PCIE_LOGIC_COHERENCY_CONTROL3,
> +		     sizeof(val), &val);
> +	val |= FIELD_PREP(PCIE_CFG_MSTR_ARCACHE_MODE, IOCP_ARCACHE);
> +	val |= FIELD_PREP(PCIE_CFG_MSTR_AWCACHE_MODE, IOCP_AWCACHE);
> +	val |= FIELD_PREP(PCIE_CFG_MSTR_ARCACHE_VALUE, IOCP_ARCACHE);
> +	val |= FIELD_PREP(PCIE_CFG_MSTR_AWCACHE_VALUE, IOCP_AWCACHE);
> +	dw_pcie_write(pci->dbi_base + PCIE_LOGIC_COHERENCY_CONTROL3,
> +		      sizeof(val), val);
> +
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +}
> +
> +static void qilai_pcie_enable_msi(struct qilai_pcie *qlpci)
> +{
> +	u32 val;
> +
> +	dw_pcie_read(qlpci->apb_base + PCIE_INTR_CONTROL1,
> +		     sizeof(val), &val);
> +	val |= PCIE_MSI_CTRL_INT_EN;
> +	dw_pcie_write(qlpci->apb_base + PCIE_INTR_CONTROL1,
> +		      sizeof(val), val);
> +}
> +
> +static int qilai_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> +
> +	qilai_pcie_enable_msi(qlpci);
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_host_ops qilai_pcie_host_ops = {
> +	.init = qilai_pcie_host_init,
> +};
> +
> +static int qilai_pcie_add_port(struct qilai_pcie *qlpci)
> +{
> +	struct device *dev = &qlpci->pdev->dev;
> +	struct platform_device *pdev = qlpci->pdev;
> +	int ret;
> +
> +	qlpci->dw.dev = dev;
> +	qlpci->dw.ops = &qilai_pcie_ops;
> +	qlpci->dw.pp.num_vectors = MAX_MSI_IRQS;

Does your platform really supports 256 MSIs? You have defined only a single
'msi' SPI interrupt in binding. So that means, your platform can only support 32
MSIs.

So if that's the case, you can drop this assignment since DWC uses 32 MSIs by
default.

> +	qlpci->dw.pp.ops = &qilai_pcie_host_ops;
> +
> +	dw_pcie_cap_set(&qlpci->dw, REQ_RES);
> +
> +	qlpci->apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
> +	if (IS_ERR(qlpci->apb_base))
> +		return PTR_ERR(qlpci->apb_base);
> +
> +	ret = dw_pcie_host_init(&qlpci->dw.pp);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to initialize PCIe host\n");
> +		return ret;
> +	}
> +
> +	qilai_pcie_iocp_cache_setup(&qlpci->dw.pp);

Are you sure that you need to set it up after enumerating devices?

> +
> +	return 0;
> +}
> +
> +static int qilai_pcie_probe(struct platform_device *pdev)
> +{
> +	struct qilai_pcie *qlpci;
> +
> +	qlpci = qilai_pcie_create_data(pdev);
> +	if (IS_ERR(qlpci))
> +		return PTR_ERR(qlpci);
> +
> +	platform_set_drvdata(pdev, qlpci);
> +
> +	return qilai_pcie_add_port(qlpci);
> +}
> +
> +static const struct of_device_id qilai_pcie_of_match[] = {
> +	{ .compatible = "andestech,qilai-pcie" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, qilai_pcie_of_match);
> +
> +static struct platform_driver qilai_pcie_driver = {
> +	.probe = qilai_pcie_probe,
> +	.driver = {
> +		.name	= "qilai-pcie",
> +		.of_match_table = qilai_pcie_of_match,
> +	},

You should start using PROBE_PREFER_ASYNCHRONOUS.

> +};
> +
> +module_platform_driver(qilai_pcie_driver);

builtin_platform_driver() since this driver is not going to be removed.

- Mani

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

Re: [PATCH 5/6] PCI: andes: Add Andes QiLai SoC PCIe host driver support

[Randolph Lin]

Hi Manivannan,

On Mon, Sep 08, 2025 at 12:32:28PM +0530, Manivannan Sadhasivam wrote:
> [EXTERNAL MAIL]
> 
> On Wed, Aug 20, 2025 at 07:18:42PM GMT, Randolph Lin wrote:
> 
> On top of Bjorn's review...
> 
> > Add driver support for DesignWare based PCIe controller in Andes
> > QiLai SoC. The driver only supports the Root Complex mode.
> >
> 
> Add more info about the controller. Like data rate supported, lanes, DWC IP
> version, any quirks etc...
>

It's PCIe 4.0,
One Port is 4 lanes (at 0x80000000),
Two port is 2 lanes (at 0xA0000000 and 0xC0000000).

The result of dw_pcie_readl_dbi(pci, PCIE_VERSION_NUMBER) is 0x3539372a.
Should I write it as 5.97a (version from documenmt) or
597A (version macro in pcie-designware.h)?

> > Signed-off-by: Randolph Lin <randolph@andestech.com>
> > ---
> >  drivers/pci/controller/dwc/Kconfig            |  13 +
> >  drivers/pci/controller/dwc/Makefile           |   1 +
> >  drivers/pci/controller/dwc/pcie-andes-qilai.c | 227 ++++++++++++++++++
> >  3 files changed, 241 insertions(+)
> >  create mode 100644 drivers/pci/controller/dwc/pcie-andes-qilai.c
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > index ff6b6d9e18ec..a9c5a43f648b 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -49,6 +49,19 @@ config PCIE_AMD_MDB
> >         DesignWare IP and therefore the driver re-uses the DesignWare
> >         core functions to implement the driver.
> >
> > +config PCIE_ANDES_QILAI
> > +     bool "ANDES QiLai PCIe controller"
> 
> No need to make it always built-in. You can make it as a loadable module, but
> prevent unloading during runtime (by using builtin_platform_driver) as it
> implements MSI controller. We do not allows irqchip controllers to be unloaded
> during runtime.
> 

Ok.

> > +     depends on OF && (RISCV || COMPILE_TEST)
> > +     depends on PCI_MSI
> > +     depends on ARCH_ANDES
> > +     select PCIE_DW_HOST
> > +     default y if ARCH_ANDES
> > +     help
> > +       Say Y here if you want to enable PCIe controller support on Andes
> 
> Mention 'Root Complex' mode.
>

Ok. 

> > +       QiLai SoCs. The Andes QiLai SoCs PCIe controller is based on
> > +       DesignWare IP and therefore the driver re-uses the DesignWare
> > +       core functions to implement the driver.
> > +
> >  config PCI_MESON
> >       tristate "Amlogic Meson PCIe controller"
> >       default m if ARCH_MESON
> > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > index 6919d27798d1..de9583cbd675 100644
> > --- a/drivers/pci/controller/dwc/Makefile
> > +++ b/drivers/pci/controller/dwc/Makefile
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
> >  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
> >  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
> >  obj-$(CONFIG_PCIE_AMD_MDB) += pcie-amd-mdb.o
> > +obj-$(CONFIG_PCIE_ANDES_QILAI) += pcie-andes-qilai.o
> >  obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o
> >  obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
> >  obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
> > diff --git a/drivers/pci/controller/dwc/pcie-andes-qilai.c b/drivers/pci/controller/dwc/pcie-andes-qilai.c
> > new file mode 100644
> > index 000000000000..dd06eee82cac
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-andes-qilai.c
> > @@ -0,0 +1,227 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for the PCIe Controller in QiLai from Andes
> > + *
> > + * Copyright (C) 2025 Andes Technology Corporation
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/types.h>
> > +
> > +#include "pcie-designware.h"
> > +
> > +#define PCIE_INTR_CONTROL1                   0x15c
> > +#define PCIE_MSI_CTRL_INT_EN                 BIT(28)
> > +
> > +#define PCIE_LOGIC_COHERENCY_CONTROL3                0x8e8
> > +/* Write-Back, Read and Write Allocate */
> > +#define IOCP_ARCACHE                         0xf
> > +/* Write-Back, Read and Write Allocate */
> > +#define IOCP_AWCACHE                         0xf
> > +#define PCIE_CFG_MSTR_ARCACHE_MODE           GENMASK(6, 3)
> > +#define PCIE_CFG_MSTR_AWCACHE_MODE           GENMASK(14, 11)
> > +#define PCIE_CFG_MSTR_ARCACHE_VALUE          GENMASK(22, 19)
> > +#define PCIE_CFG_MSTR_AWCACHE_VALUE          GENMASK(30, 27)
> > +
> > +#define PCIE_GEN_CONTROL2                    0x54
> > +#define PCIE_CFG_LTSSM_EN                    BIT(0)
> > +
> > +#define PCIE_REGS_PCIE_SII_PM_STATE          0xc0
> > +#define SMLH_LINK_UP                         BIT(6)
> > +#define RDLH_LINK_UP                         BIT(7)
> > +#define PCIE_REGS_PCIE_SII_LINK_UP           (SMLH_LINK_UP | RDLH_LINK_UP)
> > +
> > +struct qilai_pcie {
> > +     struct dw_pcie dw;
> > +     struct platform_device *pdev;
> > +     void __iomem *apb_base;
> > +};
> > +
> > +#define to_qilai_pcie(_dw) container_of(_dw, struct qilai_pcie, dw)
> > +
> > +static u64 qilai_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
> > +{
> > +     struct dw_pcie_rp *pp = &pci->pp;
> > +
> > +     return cpu_addr - pp->cfg0_base;
> > +}
> > +
> > +static u32 qilai_pcie_outbound_atu_check(struct dw_pcie *pci,
> > +                                      const struct dw_pcie_ob_atu_cfg *atu,
> > +                                      u64 *limit_addr)
> > +{
> > +     u64 parent_bus_addr = atu->parent_bus_addr;
> > +
> > +     *limit_addr = parent_bus_addr + atu->size - 1;
> > +
> > +     /*
> > +      * Addresses below 4 GB are not 1:1 mapped; therefore, range checks
> > +      * only need to ensure addresses below 4 GB match pci->region_limit.
> > +      */
> > +     if (lower_32_bits(*limit_addr & ~pci->region_limit) !=
> > +         lower_32_bits(parent_bus_addr & ~pci->region_limit) ||
> > +         !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
> > +         !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size)
> > +             return -EINVAL;
> > +     else
> > +             return 0;
> > +}
> > +
> > +static bool qilai_pcie_link_up(struct dw_pcie *pci)
> > +{
> > +     struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> > +     u32 val;
> > +
> > +     /* Read smlh & rdlh link up by checking debug port */
> > +     dw_pcie_read(qlpci->apb_base + PCIE_REGS_PCIE_SII_PM_STATE, 0x4, &val);
> > +
> > +     return (val & PCIE_REGS_PCIE_SII_LINK_UP) == PCIE_REGS_PCIE_SII_LINK_UP;
> > +}
> > +
> > +static int qilai_pcie_start_link(struct dw_pcie *pci)
> > +{
> > +     struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> > +     u32 val;
> > +
> > +     /* Do phy link up */
> 
> Drop the comment.
> 

Ok.

> > +     dw_pcie_read(qlpci->apb_base + PCIE_GEN_CONTROL2, 0x4, &val);
> > +     val |= PCIE_CFG_LTSSM_EN;
> > +     dw_pcie_write(qlpci->apb_base + PCIE_GEN_CONTROL2, 0x4, val);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct dw_pcie_ops qilai_pcie_ops = {
> > +     .cpu_addr_fixup = qilai_pcie_cpu_addr_fixup,
> > +     .outbound_atu_check = qilai_pcie_outbound_atu_check,
> > +     .link_up = qilai_pcie_link_up,
> > +     .start_link = qilai_pcie_start_link,
> > +};
> > +
> > +static struct qilai_pcie *qilai_pcie_create_data(struct platform_device *pdev)
> > +{
> > +     struct qilai_pcie *qlpci;
> > +
> > +     qlpci = devm_kzalloc(&pdev->dev, sizeof(*qlpci), GFP_KERNEL);
> > +     if (!qlpci)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     qlpci->pdev = pdev;
> > +     platform_set_drvdata(pdev, qlpci);
> > +
> > +     return qlpci;
> > +}
> > +
> > +/*
> > + * Setup the Qilai PCIe IOCP (IO Coherence Port) Read/Write Behaviors to the
> > + * Write-Back, Read and Write Allocate mode.
> > + */
> 
> What caches does the IOCP target? Is it the shared system cache? Does it allow
> the PCIe transactions to snoop the cache?
> 

The target is SoC last-level cache (L2 Cache), which serves as the system cache.
The IOCP hardware helps maintain cache monitoring, ensuring that the device can
snoop data from/to the cache. The PCIe interface is connected behind the IOCP.
In this case, the PCIe does not directly snoop the cache; it achieves the same
purpose through the IOCP hardware.

> > +static void qilai_pcie_iocp_cache_setup(struct dw_pcie_rp *pp)
> > +{
> > +     struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +     u32 val;
> > +
> > +     dw_pcie_dbi_ro_wr_en(pci);
> > +
> > +     dw_pcie_read(pci->dbi_base + PCIE_LOGIC_COHERENCY_CONTROL3,
> > +                  sizeof(val), &val);
> > +     val |= FIELD_PREP(PCIE_CFG_MSTR_ARCACHE_MODE, IOCP_ARCACHE);
> > +     val |= FIELD_PREP(PCIE_CFG_MSTR_AWCACHE_MODE, IOCP_AWCACHE);
> > +     val |= FIELD_PREP(PCIE_CFG_MSTR_ARCACHE_VALUE, IOCP_ARCACHE);
> > +     val |= FIELD_PREP(PCIE_CFG_MSTR_AWCACHE_VALUE, IOCP_AWCACHE);
> > +     dw_pcie_write(pci->dbi_base + PCIE_LOGIC_COHERENCY_CONTROL3,
> > +                   sizeof(val), val);
> > +
> > +     dw_pcie_dbi_ro_wr_dis(pci);
> > +}
> > +
> > +static void qilai_pcie_enable_msi(struct qilai_pcie *qlpci)
> > +{
> > +     u32 val;
> > +
> > +     dw_pcie_read(qlpci->apb_base + PCIE_INTR_CONTROL1,
> > +                  sizeof(val), &val);
> > +     val |= PCIE_MSI_CTRL_INT_EN;
> > +     dw_pcie_write(qlpci->apb_base + PCIE_INTR_CONTROL1,
> > +                   sizeof(val), val);
> > +}
> > +
> > +static int qilai_pcie_host_init(struct dw_pcie_rp *pp)
> > +{
> > +     struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +     struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> > +
> > +     qilai_pcie_enable_msi(qlpci);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct dw_pcie_host_ops qilai_pcie_host_ops = {
> > +     .init = qilai_pcie_host_init,
> > +};
> > +
> > +static int qilai_pcie_add_port(struct qilai_pcie *qlpci)
> > +{
> > +     struct device *dev = &qlpci->pdev->dev;
> > +     struct platform_device *pdev = qlpci->pdev;
> > +     int ret;
> > +
> > +     qlpci->dw.dev = dev;
> > +     qlpci->dw.ops = &qilai_pcie_ops;
> > +     qlpci->dw.pp.num_vectors = MAX_MSI_IRQS;
> 
> Does your platform really supports 256 MSIs? You have defined only a single
> 'msi' SPI interrupt in binding. So that means, your platform can only support 32
> MSIs.
> 
> So if that's the case, you can drop this assignment since DWC uses 32 MSIs by
> default.
>

It should use the default value, I will remove this assignment.

> > +     qlpci->dw.pp.ops = &qilai_pcie_host_ops;
> > +
> > +     dw_pcie_cap_set(&qlpci->dw, REQ_RES);
> > +
> > +     qlpci->apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
> > +     if (IS_ERR(qlpci->apb_base))
> > +             return PTR_ERR(qlpci->apb_base);
> > +
> > +     ret = dw_pcie_host_init(&qlpci->dw.pp);
> > +     if (ret) {
> > +             dev_err_probe(dev, ret, "Failed to initialize PCIe host\n");
> > +             return ret;
> > +     }
> > +
> > +     qilai_pcie_iocp_cache_setup(&qlpci->dw.pp);
> 
> Are you sure that you need to set it up after enumerating devices?
>

Yes, it must be set up.
If the IOCP cache attribute is not configured, the EP device will be treated as
a non-coherent device. For better performance and compatibility, it is therefore
treated as a coherent device. Of course, it also provides flexibility for users
to configure the attribute themselves. Here, we set it up with the commonly used
settings.

> > +
> > +     return 0;
> > +}
> > +
> > +static int qilai_pcie_probe(struct platform_device *pdev)
> > +{
> > +     struct qilai_pcie *qlpci;
> > +
> > +     qlpci = qilai_pcie_create_data(pdev);
> > +     if (IS_ERR(qlpci))
> > +             return PTR_ERR(qlpci);
> > +
> > +     platform_set_drvdata(pdev, qlpci);
> > +
> > +     return qilai_pcie_add_port(qlpci);
> > +}
> > +
> > +static const struct of_device_id qilai_pcie_of_match[] = {
> > +     { .compatible = "andestech,qilai-pcie" },
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, qilai_pcie_of_match);
> > +
> > +static struct platform_driver qilai_pcie_driver = {
> > +     .probe = qilai_pcie_probe,
> > +     .driver = {
> > +             .name   = "qilai-pcie",
> > +             .of_match_table = qilai_pcie_of_match,
> > +     },
> 
> You should start using PROBE_PREFER_ASYNCHRONOUS.
> 

Can I keep using the PROBE_DEFAULT_STRATEGY?
I will leave a comment noting that the tests only passed with
PROBE_DEFAULT_STRATEGY.

When using PROBE_PREFER_ASYNCHRONOUS, numerous errors were encountered in
the EP device probe flow. 

> > +};
> > +
> > +module_platform_driver(qilai_pcie_driver);
> 
> builtin_platform_driver() since this driver is not going to be removed.
> 

ok.

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

Sincerely,
Randolph Lin

[PATCH 6/6] MAINTAINERS: Add maintainers for Andes QiLai PCIe driver

[Randolph Lin]

Here add maintainer information for Andes QiLai PCIe driver.

Signed-off-by: Randolph Lin <randolph@andestech.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index daf520a13bdf..7caede190f73 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19221,6 +19221,13 @@ S:	Supported
 F:	Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
 F:	drivers/pci/controller/pcie-altera.c
 
+PCI DRIVER FOR ANDES QILAI PCIE
+M:	Randolph Lin <randolph@andestech.com>
+L:	linux-pci@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/pci/andestech,qilai-pcie.yaml
+F:	drivers/pci/controller/dwc/pcie-andes-qilai.c
+
 PCI DRIVER FOR APPLIEDMICRO XGENE
 M:	Toan Le <toan@os.amperecomputing.com>
 L:	linux-pci@vger.kernel.org
-- 
2.34.1