[PATCH v2 0/4] Enable rk356x PCIe controller

[PATCH v2 0/4] Enable rk356x PCIe controller

[Peter Geis]

This series enables the DesignWare based PCIe controller on the rk356x
series of chips.
We drop the fallback to the core driver due to compatibility issues.
We add support for legacy interrupts for cards that lack MSI support
(which is partially broken currently).
We then add the device tree nodes to enable PCIe on the Quartz64 Model
A.

Patch 1 drops the snps,dw,pcie fallback from the dt-binding
Patch 2 adds legacy interrupt support to the driver
Patch 3 adds the device tree binding to the rk356x.dtsi
Patch 4 enables the PCIe controller on the Quartz64-A

Changelog:
v2:
- Define PCIE_CLIENT_INTR_STATUS_LEGACY
- Fix PCIE_LEGACY_INT_ENABLE to only enable the RC interrupts
- Add legacy interrupt enable/disable support

Peter Geis (4):
  dt-bindings: pci: remove fallback from Rockchip DesignWare binding
  PCI: dwc: rockchip: add legacy interrupt support
  arm64: dts: rockchip: add rk3568 pcie2x1 controller
  arm64: dts: rockchip: enable pcie controller on quartz64-a

 .../bindings/pci/rockchip-dw-pcie.yaml        |   3 +-
 .../boot/dts/rockchip/rk3566-quartz64-a.dts   |  34 +++++
 arch/arm64/boot/dts/rockchip/rk356x.dtsi      |  68 +++++++++-
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 123 +++++++++++++++++-
 4 files changed, 221 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] dt-bindings: pci: remove fallback from Rockchip DesignWare binding

[Peter Geis]

The snps,dw-pcie binds to a standalone driver.
It is not fully compatible with the Rockchip implementation and causes a
hang if it binds to the device.

Remove this binding as a valid fallback.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
index 142bbe577763..8dc11fed8a3c 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
@@ -32,7 +32,6 @@ properties:
   compatible:
     items:
       - const: rockchip,rk3568-pcie
-      - const: snps,dw-pcie
 
   reg:
     items:
@@ -110,7 +109,7 @@ examples:
         #size-cells = <2>;
 
         pcie3x2: pcie@fe280000 {
-            compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
+            compatible = "rockchip,rk3568-pcie";
             reg = <0x3 0xc0800000 0x0 0x390000>,
                   <0x0 0xfe280000 0x0 0x10000>,
                   <0x3 0x80000000 0x0 0x100000>;
-- 
2.25.1

[PATCH v2 2/4] PCI: dwc: rockchip: add legacy interrupt support

[Peter Geis]

The legacy interrupts on the rk356x pcie controller are handled by a
single muxed interrupt. Add irq domain support to the pcie-dw-rockchip
driver to support the virtual domain.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 123 +++++++++++++++++-
 1 file changed, 121 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index c9b341e55cbb..a8b1dc03d3cc 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -10,9 +10,12 @@
 
 #include <linux/clk.h>
 #include <linux/gpio/consumer.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/of_irq.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -36,10 +39,13 @@
 #define PCIE_LINKUP			(PCIE_SMLH_LINKUP | PCIE_RDLH_LINKUP)
 #define PCIE_L0S_ENTRY			0x11
 #define PCIE_CLIENT_GENERAL_CONTROL	0x0
+#define PCIE_CLIENT_INTR_STATUS_LEGACY	0x8
+#define PCIE_CLIENT_INTR_MASK_LEGACY	0x1c
 #define PCIE_CLIENT_GENERAL_DEBUG	0x104
-#define PCIE_CLIENT_HOT_RESET_CTRL      0x180
+#define PCIE_CLIENT_HOT_RESET_CTRL	0x180
 #define PCIE_CLIENT_LTSSM_STATUS	0x300
-#define PCIE_LTSSM_ENABLE_ENHANCE       BIT(4)
+#define PCIE_LEGACY_INT_ENABLE		GENMASK(3, 0)
+#define PCIE_LTSSM_ENABLE_ENHANCE	BIT(4)
 #define PCIE_LTSSM_STATUS_MASK		GENMASK(5, 0)
 
 struct rockchip_pcie {
@@ -51,6 +57,8 @@ struct rockchip_pcie {
 	struct reset_control		*rst;
 	struct gpio_desc		*rst_gpio;
 	struct regulator                *vpcie3v3;
+	struct irq_domain		*irq_domain;
+	raw_spinlock_t			irq_lock;
 };
 
 static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip,
@@ -65,6 +73,105 @@ static void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip,
 	writel_relaxed(val, rockchip->apb_base + reg);
 }
 
+static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct rockchip_pcie *rockchip = irq_desc_get_handler_data(desc);
+	struct device *dev = rockchip->pci.dev;
+	u32 reg;
+	u32 hwirq;
+	u32 virq;
+
+	chained_irq_enter(chip, desc);
+
+	reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_LEGACY);
+
+	while (reg) {
+		hwirq = ffs(reg) - 1;
+		reg &= ~BIT(hwirq);
+
+		virq = irq_find_mapping(rockchip->irq_domain, hwirq);
+		if (virq)
+			generic_handle_irq(virq);
+		else
+			dev_err(dev, "unexpected IRQ, INT%d\n", hwirq);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void rockchip_intx_mask(struct irq_data *data)
+{
+	struct rockchip_pcie *rockchip = irq_data_get_irq_chip_data(data);
+	unsigned long flags;
+	u32 val;
+
+	/* disable legacy interrupts */
+	raw_spin_lock_irqsave(&rockchip->irq_lock, flags);
+	val = HIWORD_UPDATE_BIT(PCIE_LEGACY_INT_ENABLE);
+	val |= PCIE_LEGACY_INT_ENABLE;
+	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_LEGACY);
+	raw_spin_unlock_irqrestore(&rockchip->irq_lock, flags);
+};
+
+static void rockchip_intx_unmask(struct irq_data *data)
+{
+	struct rockchip_pcie *rockchip = irq_data_get_irq_chip_data(data);
+	unsigned long flags;
+	u32 val;
+
+	/* enable legacy interrupts */
+	raw_spin_lock_irqsave(&rockchip->irq_lock, flags);
+	val = HIWORD_UPDATE_BIT(PCIE_LEGACY_INT_ENABLE);
+	val &= ~PCIE_LEGACY_INT_ENABLE;
+	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_LEGACY);
+	raw_spin_unlock_irqrestore(&rockchip->irq_lock, flags);
+};
+
+static struct irq_chip rockchip_intx_irq_chip = {
+	.flags			= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND,
+	.irq_mask		= rockchip_intx_mask,
+	.irq_unmask		= rockchip_intx_unmask,
+	.name			= "INTx",
+};
+
+static int rockchip_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
+				  irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &rockchip_intx_irq_chip, handle_simple_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops intx_domain_ops = {
+	.map = rockchip_pcie_intx_map,
+};
+
+static int rockchip_pcie_init_irq_domain(struct rockchip_pcie *rockchip)
+{
+	struct device *dev = rockchip->pci.dev;
+	struct device_node *intc;
+
+	raw_spin_lock_init(&rockchip->irq_lock);
+
+	intc = of_get_child_by_name(dev->of_node, "legacy-interrupt-controller");
+	if (!intc) {
+		dev_err(dev, "missing child interrupt-controller node\n");
+		return -EINVAL;
+	}
+
+	rockchip->irq_domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
+						    &intx_domain_ops, rockchip);
+	of_node_put(intc);
+	if (!rockchip->irq_domain) {
+		dev_err(dev, "failed to get a INTx IRQ domain\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip)
 {
 	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_ENABLE_LTSSM,
@@ -111,7 +218,19 @@ static int rockchip_pcie_host_init(struct pcie_port *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
+	struct device *dev = rockchip->pci.dev;
 	u32 val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
+	int irq, ret;
+
+	irq = of_irq_get_byname(dev->of_node, "legacy");
+	if (irq < 0)
+		return irq;
+
+	irq_set_chained_handler_and_data(irq, rockchip_pcie_legacy_int_handler, rockchip);
+
+	ret = rockchip_pcie_init_irq_domain(rockchip);
+	if (ret < 0)
+		dev_err(dev, "failed to init irq domain\n");
 
 	/* LTSSM enable control mode */
 	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
-- 
2.25.1

[PATCH v2 3/4] arm64: dts: rockchip: add rk3568 pcie2x1 controller

[Peter Geis]

The pcie2x1 controller is commong between the rk3568 and rk3566. It is a
single lane pcie2 compliant controller.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk356x.dtsi | 68 ++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index ca20d7b91fe5..d5131f5aaf73 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -270,10 +270,17 @@ gic: interrupt-controller@fd400000 {
 		      <0x0 0xfd460000 0 0x80000>; /* GICR */
 		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
 		interrupt-controller;
+		ranges;
 		#interrupt-cells = <3>;
-		mbi-alias = <0x0 0xfd410000>;
-		mbi-ranges = <296 24>;
-		msi-controller;
+		#address-cells = <2>;
+		#size-cells = <2>;
+
+		its: interrupt-controller@fd440000 {
+			compatible = "arm,gic-v3-its";
+			reg = <0x0 0xfd440000 0x0 0x20000>;
+			msi-controller;
+			#msi-cells = <1>;
+		};
 	};
 
 	usb_host0_ehci: usb@fd800000 {
@@ -722,6 +729,61 @@ qos_vop_m1: qos@fe1a8100 {
 		reg = <0x0 0xfe1a8100 0x0 0x20>;
 	};
 
+	pcie2x1: pcie@fe260000 {
+		compatible = "rockchip,rk3568-pcie";
+		#address-cells = <3>;
+		#size-cells = <2>;
+		bus-range = <0x0 0xf>;
+		assigned-clocks = <&cru ACLK_PCIE20_MST>, <&cru ACLK_PCIE20_SLV>,
+			 <&cru ACLK_PCIE20_DBI>, <&cru PCLK_PCIE20>,
+			 <&cru CLK_PCIE20_AUX_NDFT>;
+		clocks = <&cru ACLK_PCIE20_MST>, <&cru ACLK_PCIE20_SLV>,
+			 <&cru ACLK_PCIE20_DBI>, <&cru PCLK_PCIE20>,
+			 <&cru CLK_PCIE20_AUX_NDFT>;
+		clock-names = "aclk_mst", "aclk_slv",
+			      "aclk_dbi", "pclk", "aux";
+		device_type = "pci";
+		interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "sys", "pmc", "msi", "legacy", "err";
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie_intc 0>,
+				<0 0 0 2 &pcie_intc 1>,
+				<0 0 0 3 &pcie_intc 2>,
+				<0 0 0 4 &pcie_intc 3>;
+		linux,pci-domain = <0>;
+		num-ib-windows = <6>;
+		num-ob-windows = <2>;
+		max-link-speed = <2>;
+		msi-map = <0x0 &its 0x0 0x1000>;
+		num-lanes = <1>;
+		phys = <&combphy2 PHY_TYPE_PCIE>;
+		phy-names = "pcie-phy";
+		power-domains = <&power RK3568_PD_PIPE>;
+		reg = <0x3 0xc0000000 0x0 0x00400000>,
+		      <0x0 0xfe260000 0x0 0x00010000>,
+		      <0x3 0x00000000 0x0 0x01000000>;
+		ranges = <0x01000000 0x0 0x01000000 0x3 0x01000000 0x0 0x00100000
+			  0x02000000 0x0 0x02000000 0x3 0x02000000 0x0 0x3e000000>;
+		reg-names = "dbi", "apb", "config";
+		resets = <&cru SRST_PCIE20_POWERUP>;
+		reset-names = "pipe";
+		status = "disabled";
+
+		pcie_intc: legacy-interrupt-controller {
+			#address-cells = <0>;
+			#interrupt-cells = <1>;
+			interrupt-controller;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SPI 72 IRQ_TYPE_EDGE_RISING>;
+		};
+
+	};
+
 	sdmmc0: mmc@fe2b0000 {
 		compatible = "rockchip,rk3568-dw-mshc", "rockchip,rk3288-dw-mshc";
 		reg = <0x0 0xfe2b0000 0x0 0x4000>;
-- 
2.25.1

[PATCH v2 4/4] arm64: dts: rockchip: enable pcie controller on quartz64-a

[Peter Geis]

Add the nodes to enable the pcie controller on the quartz64 model a
board.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
 .../boot/dts/rockchip/rk3566-quartz64-a.dts   | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
index 141a433429b5..85926d46337d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
@@ -125,6 +125,18 @@ vbus: vbus {
 		vin-supply = <&vcc12v_dcin>;
 	};
 
+	vcc3v3_pcie_p: vcc3v3_pcie_p {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpio = <&gpio0 RK_PC6 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pcie_enable_h>;
+		regulator-name = "vcc3v3_pcie_p";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		vin-supply = <&vcc_3v3>;
+	};
+
 	vcc5v0_usb: vcc5v0_usb {
 		compatible = "regulator-fixed";
 		regulator-name = "vcc5v0_usb";
@@ -201,6 +213,10 @@ &combphy1 {
 	status = "okay";
 };
 
+&combphy2 {
+	status = "okay";
+};
+
 &cpu0 {
 	cpu-supply = <&vdd_cpu>;
 };
@@ -509,6 +525,14 @@ rgmii_phy1: ethernet-phy@0 {
 	};
 };
 
+&pcie2x1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie_reset_h>;
+	reset-gpios = <&gpio1 RK_PB2 GPIO_ACTIVE_HIGH>;
+	status = "okay";
+	vpcie3v3-supply = <&vcc3v3_pcie_p>;
+};
+
 &pinctrl {
 	bt {
 		bt_enable_h: bt-enable-h {
@@ -534,6 +558,16 @@ diy_led_enable_h: diy-led-enable-h {
 		};
 	};
 
+	pcie {
+		pcie_enable_h: pcie-enable-h {
+			rockchip,pins = <0 RK_PC6 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+
+		pcie_reset_h: pcie-reset-h {
+			rockchip,pins = <1 RK_PB2 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
 	pmic {
 		pmic_int_l: pmic-int-l {
 			rockchip,pins = <0 RK_PA3 RK_FUNC_GPIO &pcfg_pull_up>;
-- 
2.25.1

Re: [PATCH v2 1/4] dt-bindings: pci: remove fallback from Rockchip DesignWare binding

[Rob Herring]

On Wed, Apr 13, 2022 at 09:37:28AM -0400, Peter Geis wrote:
> The snps,dw-pcie binds to a standalone driver.
> It is not fully compatible with the Rockchip implementation and causes a
> hang if it binds to the device.
> 
> Remove this binding as a valid fallback.
> 
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
>  Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> index 142bbe577763..8dc11fed8a3c 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> @@ -32,7 +32,6 @@ properties:
>    compatible:
>      items:
>        - const: rockchip,rk3568-pcie
> -      - const: snps,dw-pcie

With this, you no longer need 'select' so please drop it too.

>  
>    reg:
>      items:
> @@ -110,7 +109,7 @@ examples:
>          #size-cells = <2>;
>  
>          pcie3x2: pcie@fe280000 {
> -            compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
> +            compatible = "rockchip,rk3568-pcie";
>              reg = <0x3 0xc0800000 0x0 0x390000>,
>                    <0x0 0xfe280000 0x0 0x10000>,
>                    <0x3 0x80000000 0x0 0x100000>;
> -- 
> 2.25.1
> 
> 

Re: [PATCH v2 2/4] PCI: dwc: rockchip: add legacy interrupt support

[Marc Zyngier]

On 2022-04-13 14:37, Peter Geis wrote:
> The legacy interrupts on the rk356x pcie controller are handled by a
> single muxed interrupt. Add irq domain support to the pcie-dw-rockchip
> driver to support the virtual domain.
> 
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 123 +++++++++++++++++-
>  1 file changed, 121 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index c9b341e55cbb..a8b1dc03d3cc 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -10,9 +10,12 @@
> 
>  #include <linux/clk.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/of_irq.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> @@ -36,10 +39,13 @@
>  #define PCIE_LINKUP			(PCIE_SMLH_LINKUP | PCIE_RDLH_LINKUP)
>  #define PCIE_L0S_ENTRY			0x11
>  #define PCIE_CLIENT_GENERAL_CONTROL	0x0
> +#define PCIE_CLIENT_INTR_STATUS_LEGACY	0x8
> +#define PCIE_CLIENT_INTR_MASK_LEGACY	0x1c
>  #define PCIE_CLIENT_GENERAL_DEBUG	0x104
> -#define PCIE_CLIENT_HOT_RESET_CTRL      0x180
> +#define PCIE_CLIENT_HOT_RESET_CTRL	0x180
>  #define PCIE_CLIENT_LTSSM_STATUS	0x300
> -#define PCIE_LTSSM_ENABLE_ENHANCE       BIT(4)
> +#define PCIE_LEGACY_INT_ENABLE		GENMASK(3, 0)
> +#define PCIE_LTSSM_ENABLE_ENHANCE	BIT(4)
>  #define PCIE_LTSSM_STATUS_MASK		GENMASK(5, 0)
> 
>  struct rockchip_pcie {
> @@ -51,6 +57,8 @@ struct rockchip_pcie {
>  	struct reset_control		*rst;
>  	struct gpio_desc		*rst_gpio;
>  	struct regulator                *vpcie3v3;
> +	struct irq_domain		*irq_domain;
> +	raw_spinlock_t			irq_lock;
>  };
> 
>  static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip,
> @@ -65,6 +73,105 @@ static void rockchip_pcie_writel_apb(struct
> rockchip_pcie *rockchip,
>  	writel_relaxed(val, rockchip->apb_base + reg);
>  }
> 
> +static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct rockchip_pcie *rockchip = irq_desc_get_handler_data(desc);
> +	struct device *dev = rockchip->pci.dev;
> +	u32 reg;
> +	u32 hwirq;
> +	u32 virq;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	reg = rockchip_pcie_readl_apb(rockchip, 
> PCIE_CLIENT_INTR_STATUS_LEGACY);
> +
> +	while (reg) {
> +		hwirq = ffs(reg) - 1;
> +		reg &= ~BIT(hwirq);

The whole construct would be better served by for_each_set_bit().

> +
> +		virq = irq_find_mapping(rockchip->irq_domain, hwirq);
> +		if (virq)
> +			generic_handle_irq(virq);

Please replace this with generic_handle_domain_irq().

> +		else
> +			dev_err(dev, "unexpected IRQ, INT%d\n", hwirq);

This hardly serves any purpose. At best, this is a debug statement.
At worse, this is a DoS. In any case, please remove it.

> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void rockchip_intx_mask(struct irq_data *data)
> +{
> +	struct rockchip_pcie *rockchip = irq_data_get_irq_chip_data(data);
> +	unsigned long flags;
> +	u32 val;
> +
> +	/* disable legacy interrupts */
> +	raw_spin_lock_irqsave(&rockchip->irq_lock, flags);
> +	val = HIWORD_UPDATE_BIT(PCIE_LEGACY_INT_ENABLE);
> +	val |= PCIE_LEGACY_INT_ENABLE;
> +	rockchip_pcie_writel_apb(rockchip, val, 
> PCIE_CLIENT_INTR_MASK_LEGACY);
> +	raw_spin_unlock_irqrestore(&rockchip->irq_lock, flags);
> +};
> +
> +static void rockchip_intx_unmask(struct irq_data *data)
> +{
> +	struct rockchip_pcie *rockchip = irq_data_get_irq_chip_data(data);
> +	unsigned long flags;
> +	u32 val;
> +
> +	/* enable legacy interrupts */
> +	raw_spin_lock_irqsave(&rockchip->irq_lock, flags);
> +	val = HIWORD_UPDATE_BIT(PCIE_LEGACY_INT_ENABLE);
> +	val &= ~PCIE_LEGACY_INT_ENABLE;
> +	rockchip_pcie_writel_apb(rockchip, val, 
> PCIE_CLIENT_INTR_MASK_LEGACY);
> +	raw_spin_unlock_irqrestore(&rockchip->irq_lock, flags);
> +};
> +
> +static struct irq_chip rockchip_intx_irq_chip = {
> +	.flags			= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND,
> +	.irq_mask		= rockchip_intx_mask,
> +	.irq_unmask		= rockchip_intx_unmask,
> +	.name			= "INTx",

For consistency, please place 'name' at the top, and 'flags' at the end.

> +};
> +
> +static int rockchip_pcie_intx_map(struct irq_domain *domain, unsigned 
> int irq,
> +				  irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &rockchip_intx_irq_chip, 
> handle_simple_irq);

Why isn't this a *level* handler, as per the PCI spec?

> +	irq_set_chip_data(irq, domain->host_data);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops intx_domain_ops = {
> +	.map = rockchip_pcie_intx_map,
> +};
> +
> +static int rockchip_pcie_init_irq_domain(struct rockchip_pcie 
> *rockchip)
> +{
> +	struct device *dev = rockchip->pci.dev;
> +	struct device_node *intc;
> +
> +	raw_spin_lock_init(&rockchip->irq_lock);
> +
> +	intc = of_get_child_by_name(dev->of_node, 
> "legacy-interrupt-controller");
> +	if (!intc) {
> +		dev_err(dev, "missing child interrupt-controller node\n");
> +		return -EINVAL;
> +	}
> +
> +	rockchip->irq_domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
> +						    &intx_domain_ops, rockchip);
> +	of_node_put(intc);
> +	if (!rockchip->irq_domain) {
> +		dev_err(dev, "failed to get a INTx IRQ domain\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip)
>  {
>  	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_ENABLE_LTSSM,
> @@ -111,7 +218,19 @@ static int rockchip_pcie_host_init(struct 
> pcie_port *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
> +	struct device *dev = rockchip->pci.dev;
>  	u32 val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
> +	int irq, ret;
> +
> +	irq = of_irq_get_byname(dev->of_node, "legacy");
> +	if (irq < 0)
> +		return irq;
> +
> +	irq_set_chained_handler_and_data(irq,
> rockchip_pcie_legacy_int_handler, rockchip);
> +

Installing the handler before the domain is instantiated is
unlikely to end well if you have a pending interrupt...

> +	ret = rockchip_pcie_init_irq_domain(rockchip);
> +	if (ret < 0)
> +		dev_err(dev, "failed to init irq domain\n");
> 
>  	/* LTSSM enable control mode */
>  	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 2/4] PCI: dwc: rockchip: add legacy interrupt support

[Peter Geis]

On Thu, Apr 14, 2022 at 4:36 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On 2022-04-13 14:37, Peter Geis wrote:
> > The legacy interrupts on the rk356x pcie controller are handled by a
> > single muxed interrupt. Add irq domain support to the pcie-dw-rockchip
> > driver to support the virtual domain.
> >
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 123 +++++++++++++++++-
> >  1 file changed, 121 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > index c9b341e55cbb..a8b1dc03d3cc 100644
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -10,9 +10,12 @@
> >
> >  #include <linux/clk.h>
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +#include <linux/irqdomain.h>
> >  #include <linux/mfd/syscon.h>
> >  #include <linux/module.h>
> >  #include <linux/of_device.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/phy/phy.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regmap.h>
> > @@ -36,10 +39,13 @@
> >  #define PCIE_LINKUP                  (PCIE_SMLH_LINKUP | PCIE_RDLH_LINKUP)
> >  #define PCIE_L0S_ENTRY                       0x11
> >  #define PCIE_CLIENT_GENERAL_CONTROL  0x0
> > +#define PCIE_CLIENT_INTR_STATUS_LEGACY       0x8
> > +#define PCIE_CLIENT_INTR_MASK_LEGACY 0x1c
> >  #define PCIE_CLIENT_GENERAL_DEBUG    0x104
> > -#define PCIE_CLIENT_HOT_RESET_CTRL      0x180
> > +#define PCIE_CLIENT_HOT_RESET_CTRL   0x180
> >  #define PCIE_CLIENT_LTSSM_STATUS     0x300
> > -#define PCIE_LTSSM_ENABLE_ENHANCE       BIT(4)
> > +#define PCIE_LEGACY_INT_ENABLE               GENMASK(3, 0)
> > +#define PCIE_LTSSM_ENABLE_ENHANCE    BIT(4)
> >  #define PCIE_LTSSM_STATUS_MASK               GENMASK(5, 0)
> >
> >  struct rockchip_pcie {
> > @@ -51,6 +57,8 @@ struct rockchip_pcie {
> >       struct reset_control            *rst;
> >       struct gpio_desc                *rst_gpio;
> >       struct regulator                *vpcie3v3;
> > +     struct irq_domain               *irq_domain;
> > +     raw_spinlock_t                  irq_lock;
> >  };
> >
> >  static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip,
> > @@ -65,6 +73,105 @@ static void rockchip_pcie_writel_apb(struct
> > rockchip_pcie *rockchip,
> >       writel_relaxed(val, rockchip->apb_base + reg);
> >  }
> >
> > +static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
> > +{
> > +     struct irq_chip *chip = irq_desc_get_chip(desc);
> > +     struct rockchip_pcie *rockchip = irq_desc_get_handler_data(desc);
> > +     struct device *dev = rockchip->pci.dev;
> > +     u32 reg;
> > +     u32 hwirq;
> > +     u32 virq;
> > +
> > +     chained_irq_enter(chip, desc);
> > +
> > +     reg = rockchip_pcie_readl_apb(rockchip,
> > PCIE_CLIENT_INTR_STATUS_LEGACY);
> > +
> > +     while (reg) {
> > +             hwirq = ffs(reg) - 1;
> > +             reg &= ~BIT(hwirq);
>
> The whole construct would be better served by for_each_set_bit().

Ah, that's much nicer, thanks!

>
> > +
> > +             virq = irq_find_mapping(rockchip->irq_domain, hwirq);
> > +             if (virq)
> > +                     generic_handle_irq(virq);
>
> Please replace this with generic_handle_domain_irq().

I see the bulk conversion was done after I created this patch.
I will make this change.

>
> > +             else
> > +                     dev_err(dev, "unexpected IRQ, INT%d\n", hwirq);
>
> This hardly serves any purpose. At best, this is a debug statement.
> At worse, this is a DoS. In any case, please remove it.

Done!

>
> > +     }
> > +
> > +     chained_irq_exit(chip, desc);
> > +}
> > +
> > +static void rockchip_intx_mask(struct irq_data *data)
> > +{
> > +     struct rockchip_pcie *rockchip = irq_data_get_irq_chip_data(data);
> > +     unsigned long flags;
> > +     u32 val;
> > +
> > +     /* disable legacy interrupts */
> > +     raw_spin_lock_irqsave(&rockchip->irq_lock, flags);
> > +     val = HIWORD_UPDATE_BIT(PCIE_LEGACY_INT_ENABLE);
> > +     val |= PCIE_LEGACY_INT_ENABLE;
> > +     rockchip_pcie_writel_apb(rockchip, val,
> > PCIE_CLIENT_INTR_MASK_LEGACY);
> > +     raw_spin_unlock_irqrestore(&rockchip->irq_lock, flags);
> > +};
> > +
> > +static void rockchip_intx_unmask(struct irq_data *data)
> > +{
> > +     struct rockchip_pcie *rockchip = irq_data_get_irq_chip_data(data);
> > +     unsigned long flags;
> > +     u32 val;
> > +
> > +     /* enable legacy interrupts */
> > +     raw_spin_lock_irqsave(&rockchip->irq_lock, flags);
> > +     val = HIWORD_UPDATE_BIT(PCIE_LEGACY_INT_ENABLE);
> > +     val &= ~PCIE_LEGACY_INT_ENABLE;
> > +     rockchip_pcie_writel_apb(rockchip, val,
> > PCIE_CLIENT_INTR_MASK_LEGACY);
> > +     raw_spin_unlock_irqrestore(&rockchip->irq_lock, flags);
> > +};
> > +
> > +static struct irq_chip rockchip_intx_irq_chip = {
> > +     .flags                  = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND,
> > +     .irq_mask               = rockchip_intx_mask,
> > +     .irq_unmask             = rockchip_intx_unmask,
> > +     .name                   = "INTx",
>
> For consistency, please place 'name' at the top, and 'flags' at the end.

Will do.

>
> > +};
> > +
> > +static int rockchip_pcie_intx_map(struct irq_domain *domain, unsigned
> > int irq,
> > +                               irq_hw_number_t hwirq)
> > +{
> > +     irq_set_chip_and_handler(irq, &rockchip_intx_irq_chip,
> > handle_simple_irq);
>
> Why isn't this a *level* handler, as per the PCI spec?

Fixed.

>
> > +     irq_set_chip_data(irq, domain->host_data);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct irq_domain_ops intx_domain_ops = {
> > +     .map = rockchip_pcie_intx_map,
> > +};
> > +
> > +static int rockchip_pcie_init_irq_domain(struct rockchip_pcie
> > *rockchip)
> > +{
> > +     struct device *dev = rockchip->pci.dev;
> > +     struct device_node *intc;
> > +
> > +     raw_spin_lock_init(&rockchip->irq_lock);
> > +
> > +     intc = of_get_child_by_name(dev->of_node,
> > "legacy-interrupt-controller");
> > +     if (!intc) {
> > +             dev_err(dev, "missing child interrupt-controller node\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     rockchip->irq_domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
> > +                                                 &intx_domain_ops, rockchip);
> > +     of_node_put(intc);
> > +     if (!rockchip->irq_domain) {
> > +             dev_err(dev, "failed to get a INTx IRQ domain\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip)
> >  {
> >       rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_ENABLE_LTSSM,
> > @@ -111,7 +218,19 @@ static int rockchip_pcie_host_init(struct
> > pcie_port *pp)
> >  {
> >       struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >       struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
> > +     struct device *dev = rockchip->pci.dev;
> >       u32 val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
> > +     int irq, ret;
> > +
> > +     irq = of_irq_get_byname(dev->of_node, "legacy");
> > +     if (irq < 0)
> > +             return irq;
> > +
> > +     irq_set_chained_handler_and_data(irq,
> > rockchip_pcie_legacy_int_handler, rockchip);
> > +
>
> Installing the handler before the domain is instantiated is
> unlikely to end well if you have a pending interrupt...

While the interrupts are masked at this stage, you make a valid point,
I'll move this a little south.

>
> > +     ret = rockchip_pcie_init_irq_domain(rockchip);
> > +     if (ret < 0)
> > +             dev_err(dev, "failed to init irq domain\n");
> >
> >       /* LTSSM enable control mode */
> >       rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
>
> Thanks,
>
>          M.
> --
> Jazz is not dead. It just smells funny...

Thank you for your review.

Re: [PATCH v2 1/4] dt-bindings: pci: remove fallback from Rockchip DesignWare binding

[Peter Geis]

On Thu, Apr 14, 2022 at 1:53 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Apr 13, 2022 at 09:37:28AM -0400, Peter Geis wrote:
> > The snps,dw-pcie binds to a standalone driver.
> > It is not fully compatible with the Rockchip implementation and causes a
> > hang if it binds to the device.
> >
> > Remove this binding as a valid fallback.
> >
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > index 142bbe577763..8dc11fed8a3c 100644
> > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > @@ -32,7 +32,6 @@ properties:
> >    compatible:
> >      items:
> >        - const: rockchip,rk3568-pcie
> > -      - const: snps,dw-pcie
>
> With this, you no longer need 'select' so please drop it too.

Thanks! Will do.

>
> >
> >    reg:
> >      items:
> > @@ -110,7 +109,7 @@ examples:
> >          #size-cells = <2>;
> >
> >          pcie3x2: pcie@fe280000 {
> > -            compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
> > +            compatible = "rockchip,rk3568-pcie";
> >              reg = <0x3 0xc0800000 0x0 0x390000>,
> >                    <0x0 0xfe280000 0x0 0x10000>,
> >                    <0x3 0x80000000 0x0 0x100000>;
> > --
> > 2.25.1
> >
> >