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 reset the PCIe controller at driver probe to
prevent issues in the future when firmware / kexec leaves the controller
in an unknown state. 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 resets the PCIe controller to prevent configuration bugs
Patch 3 adds legacy interrupt support to the driver
Patch 4 adds the device tree binding to the rk356x.dtsi
Patch 5 enables the PCIe controller on the Quartz64-A
Changelog:
v9:
- move reset_control_assert out of rockchip_pcie_resource_get
- fix various formatting mistakes
- fix a checkpatch warning
v8:
- add core reset patch
- simplify IRQ enable/disable functions
- drop spinlock
- only enable/disable IRQ requested
- only pass the IRQ register bits used to irq functions
v7:
- drop assigned-clocks
v6:
- fix a ranges issue
- point to GIC instead of ITS
v5:
- fix incorrect series (apologies for the v4 spam)
v4:
- drop the ITS modification, poor compatibility is better than
completely broken
v3:
- drop select node from dt-binding
- convert to for_each_set_bit
- convert to generic_handle_domain_irq
- drop unncessary dev_err
- reorder irq_chip items
- change to level_irq
- install the handler after initializing the domain
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 (5):
dt-bindings: PCI: Remove fallback from Rockchip DesignWare binding
PCI: rockchip-dwc: Reset core at driver probe
PCI: rockchip-dwc: 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 | 12 +-
.../boot/dts/rockchip/rk3566-quartz64-a.dts | 34 +++++
arch/arm64/boot/dts/rockchip/rk356x.dtsi | 52 ++++++++
drivers/pci/controller/dwc/pcie-dw-rockchip.c | 119 +++++++++++++++---
4 files changed, 191 insertions(+), 26 deletions(-)
--
2.25.1
The PCIe2x1 controller is common between the rk3568 and rk3566. It is a
single lane PCIe2 compliant controller.
Signed-off-by: Peter Geis <[email protected]>
---
arch/arm64/boot/dts/rockchip/rk356x.dtsi | 52 ++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index 7cdef800cb3c..aea5d9255235 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -689,6 +689,58 @@ 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>;
+ 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 &gic 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 0x01100000 0x0 0x3ef00000>;
+ 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
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 <[email protected]>
---
drivers/pci/controller/dwc/pcie-dw-rockchip.c | 96 ++++++++++++++++++-
1 file changed, 94 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index faedbd6ebc20..8c5bb9d7cc36 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>
@@ -26,6 +29,7 @@
*/
#define HIWORD_UPDATE(mask, val) (((mask) << 16) | (val))
#define HIWORD_UPDATE_BIT(val) HIWORD_UPDATE(val, val)
+#define HIWORD_DISABLE_BIT(val) HIWORD_UPDATE(val, ~val)
#define to_rockchip_pcie(x) dev_get_drvdata((x)->dev)
@@ -36,10 +40,12 @@
#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_LTSSM_ENABLE_ENHANCE BIT(4)
#define PCIE_LTSSM_STATUS_MASK GENMASK(5, 0)
struct rockchip_pcie {
@@ -51,6 +57,7 @@ struct rockchip_pcie {
struct reset_control *rst;
struct gpio_desc *rst_gpio;
struct regulator *vpcie3v3;
+ struct irq_domain *irq_domain;
};
static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip,
@@ -65,6 +72,78 @@ 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);
+ unsigned long reg, hwirq;
+
+ chained_irq_enter(chip, desc);
+
+ reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_LEGACY);
+
+ for_each_set_bit(hwirq, ®, 4)
+ generic_handle_domain_irq(rockchip->irq_domain, hwirq);
+
+ chained_irq_exit(chip, desc);
+}
+
+static void rockchip_intx_mask(struct irq_data *data)
+{
+ rockchip_pcie_writel_apb(irq_data_get_irq_chip_data(data),
+ HIWORD_UPDATE_BIT(BIT(data->hwirq)),
+ PCIE_CLIENT_INTR_MASK_LEGACY);
+};
+
+static void rockchip_intx_unmask(struct irq_data *data)
+{
+ rockchip_pcie_writel_apb(irq_data_get_irq_chip_data(data),
+ HIWORD_DISABLE_BIT(BIT(data->hwirq)),
+ PCIE_CLIENT_INTR_MASK_LEGACY);
+};
+
+static struct irq_chip rockchip_intx_irq_chip = {
+ .name = "INTx",
+ .irq_mask = rockchip_intx_mask,
+ .irq_unmask = rockchip_intx_unmask,
+ .flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND,
+};
+
+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_level_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;
+
+ 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 +190,20 @@ 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;
+
+ ret = rockchip_pcie_init_irq_domain(rockchip);
+ if (ret < 0)
+ dev_err(dev, "failed to init irq domain\n");
+
+ irq_set_chained_handler_and_data(irq, rockchip_pcie_legacy_int_handler,
+ rockchip);
/* LTSSM enable control mode */
rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
--
2.25.1
Add the nodes to enable the PCIe controller on the Quartz64 Model A
board.
Signed-off-by: Peter Geis <[email protected]>
---
.../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 dd7f4b9b686b..8b0537744a60 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";
@@ -187,6 +199,10 @@ vcc_wl: vcc_wl {
};
};
+&combphy2 {
+ status = "okay";
+};
+
&cpu0 {
cpu-supply = <&vdd_cpu>;
};
@@ -495,6 +511,14 @@ rgmii_phy1: ethernet-phy@0 {
};
};
+&pcie2x1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pcie_reset_h>;
+ reset-gpios = <&gpio1 RK_PB2 GPIO_ACTIVE_HIGH>;
+ vpcie3v3-supply = <&vcc3v3_pcie_p>;
+ status = "okay";
+};
+
&pinctrl {
bt {
bt_enable_h: bt-enable-h {
@@ -520,6 +544,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
The PCIe controller is in an unknown state at driver probe. This can
lead to undesireable effects when the driver attempts to configure the
controller.
Prevent issues in the future by resetting the core during probe.
Signed-off-by: Peter Geis <[email protected]>
Tested-by: Nicolas Frattaroli <[email protected]>
---
drivers/pci/controller/dwc/pcie-dw-rockchip.c | 23 ++++++++-----------
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index c9b341e55cbb..faedbd6ebc20 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -152,6 +152,11 @@ static int rockchip_pcie_resource_get(struct platform_device *pdev,
if (IS_ERR(rockchip->rst_gpio))
return PTR_ERR(rockchip->rst_gpio);
+ rockchip->rst = devm_reset_control_array_get_exclusive(&pdev->dev);
+ if (IS_ERR(rockchip->rst))
+ return dev_err_probe(&pdev->dev, PTR_ERR(rockchip->rst),
+ "failed to get reset lines\n");
+
return 0;
}
@@ -182,18 +187,6 @@ static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip)
phy_power_off(rockchip->phy);
}
-static int rockchip_pcie_reset_control_release(struct rockchip_pcie *rockchip)
-{
- struct device *dev = rockchip->pci.dev;
-
- rockchip->rst = devm_reset_control_array_get_exclusive(dev);
- if (IS_ERR(rockchip->rst))
- return dev_err_probe(dev, PTR_ERR(rockchip->rst),
- "failed to get reset lines\n");
-
- return reset_control_deassert(rockchip->rst);
-}
-
static const struct dw_pcie_ops dw_pcie_ops = {
.link_up = rockchip_pcie_link_up,
.start_link = rockchip_pcie_start_link,
@@ -222,6 +215,10 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
if (ret)
return ret;
+ ret = reset_control_assert(rockchip->rst);
+ if (ret)
+ return ret;
+
/* DON'T MOVE ME: must be enable before PHY init */
rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
if (IS_ERR(rockchip->vpcie3v3)) {
@@ -241,7 +238,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
if (ret)
goto disable_regulator;
- ret = rockchip_pcie_reset_control_release(rockchip);
+ ret = reset_control_deassert(rockchip->rst);
if (ret)
goto deinit_phy;
--
2.25.1
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 <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../devicetree/bindings/pci/rockchip-dw-pcie.yaml | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
index 142bbe577763..bc0a9d1db750 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
@@ -19,20 +19,10 @@ description: |+
allOf:
- $ref: /schemas/pci/pci-bus.yaml#
-# We need a select here so we don't match all nodes with 'snps,dw-pcie'
-select:
- properties:
- compatible:
- contains:
- const: rockchip,rk3568-pcie
- required:
- - compatible
-
properties:
compatible:
items:
- const: rockchip,rk3568-pcie
- - const: snps,dw-pcie
reg:
items:
@@ -110,7 +100,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
On Fri, Apr 29, 2022 at 8:38 AM Peter Geis <[email protected]> wrote:
>
> 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 reset the PCIe controller at driver probe to
> prevent issues in the future when firmware / kexec leaves the controller
> in an unknown state. 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.
Good Evening,
Just a gentle ping to see if there's anything outstanding here.
Very Respectfully,
Peter Geis
>
> Patch 1 drops the snps,dw,pcie fallback from the dt-binding
> Patch 2 resets the PCIe controller to prevent configuration bugs
> Patch 3 adds legacy interrupt support to the driver
> Patch 4 adds the device tree binding to the rk356x.dtsi
> Patch 5 enables the PCIe controller on the Quartz64-A
>
> Changelog:
> v9:
> - move reset_control_assert out of rockchip_pcie_resource_get
> - fix various formatting mistakes
> - fix a checkpatch warning
>
> v8:
> - add core reset patch
> - simplify IRQ enable/disable functions
> - drop spinlock
> - only enable/disable IRQ requested
> - only pass the IRQ register bits used to irq functions
>
> v7:
> - drop assigned-clocks
>
> v6:
> - fix a ranges issue
> - point to GIC instead of ITS
>
> v5:
> - fix incorrect series (apologies for the v4 spam)
>
> v4:
> - drop the ITS modification, poor compatibility is better than
> completely broken
>
> v3:
> - drop select node from dt-binding
> - convert to for_each_set_bit
> - convert to generic_handle_domain_irq
> - drop unncessary dev_err
> - reorder irq_chip items
> - change to level_irq
> - install the handler after initializing the domain
>
> 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 (5):
> dt-bindings: PCI: Remove fallback from Rockchip DesignWare binding
> PCI: rockchip-dwc: Reset core at driver probe
> PCI: rockchip-dwc: 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 | 12 +-
> .../boot/dts/rockchip/rk3566-quartz64-a.dts | 34 +++++
> arch/arm64/boot/dts/rockchip/rk356x.dtsi | 52 ++++++++
> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 119 +++++++++++++++---
> 4 files changed, 191 insertions(+), 26 deletions(-)
>
> --
> 2.25.1
>
Am Dienstag, 10. Mai 2022, 23:11:18 CEST schrieb Peter Geis:
> On Fri, Apr 29, 2022 at 8:38 AM Peter Geis <[email protected]> wrote:
> >
> > 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 reset the PCIe controller at driver probe to
> > prevent issues in the future when firmware / kexec leaves the controller
> > in an unknown state. 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.
>
> Good Evening,
>
> Just a gentle ping to see if there's anything outstanding here.
From my side it looks good. I'll take patches 4+5 once the binding-change
from patch1 has been applied to some tree.
Heiko
> >
> > Patch 1 drops the snps,dw,pcie fallback from the dt-binding
> > Patch 2 resets the PCIe controller to prevent configuration bugs
> > Patch 3 adds legacy interrupt support to the driver
> > Patch 4 adds the device tree binding to the rk356x.dtsi
> > Patch 5 enables the PCIe controller on the Quartz64-A
> >
> > Changelog:
> > v9:
> > - move reset_control_assert out of rockchip_pcie_resource_get
> > - fix various formatting mistakes
> > - fix a checkpatch warning
> >
> > v8:
> > - add core reset patch
> > - simplify IRQ enable/disable functions
> > - drop spinlock
> > - only enable/disable IRQ requested
> > - only pass the IRQ register bits used to irq functions
> >
> > v7:
> > - drop assigned-clocks
> >
> > v6:
> > - fix a ranges issue
> > - point to GIC instead of ITS
> >
> > v5:
> > - fix incorrect series (apologies for the v4 spam)
> >
> > v4:
> > - drop the ITS modification, poor compatibility is better than
> > completely broken
> >
> > v3:
> > - drop select node from dt-binding
> > - convert to for_each_set_bit
> > - convert to generic_handle_domain_irq
> > - drop unncessary dev_err
> > - reorder irq_chip items
> > - change to level_irq
> > - install the handler after initializing the domain
> >
> > 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 (5):
> > dt-bindings: PCI: Remove fallback from Rockchip DesignWare binding
> > PCI: rockchip-dwc: Reset core at driver probe
> > PCI: rockchip-dwc: 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 | 12 +-
> > .../boot/dts/rockchip/rk3566-quartz64-a.dts | 34 +++++
> > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 52 ++++++++
> > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 119 +++++++++++++++---
> > 4 files changed, 191 insertions(+), 26 deletions(-)
> >
> > --
> > 2.25.1
> >
>
Am Mittwoch, 11. Mai 2022, 17:00:05 CEST schrieb Lorenzo Pieralisi:
> On Wed, May 11, 2022 at 10:26:20AM -0400, Peter Geis wrote:
> > On Wed, May 11, 2022 at 9:50 AM Lorenzo Pieralisi
> > <[email protected]> wrote:
> > >
> > > On Fri, Apr 29, 2022 at 08:38:28AM -0400, Peter Geis wrote:
> > > > The PCIe controller is in an unknown state at driver probe. This can
> > > > lead to undesireable effects when the driver attempts to configure the
> > > > controller.
> > > >
> > > > Prevent issues in the future by resetting the core during probe.
> > > >
> > > > Signed-off-by: Peter Geis <[email protected]>
> > > > Tested-by: Nicolas Frattaroli <[email protected]>
> > > > ---
> > > > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 23 ++++++++-----------
> > > > 1 file changed, 10 insertions(+), 13 deletions(-)
> > >
> > > I fear that the controller reset behaviour is bootloader/firmware
> > > dependent.
> > >
> > > Are we sure we are not triggering any regressions by resetting the
> > > controller in the middle of probe (aka is the driver implicitly
> > > relying on existing behaviour on systems that are not the ones
> > > you are testing on) ?
> > >
> > > Just asking, the rockchip maintainers should be able to answer this
> > > question.
> >
> > This is a new driver with no current users, this series enables the
> > first user. It does not support ACPI nor any sort of handoff at this
> > time.
>
> Ok. I will take patches [1-3], dts changes will have to go via
> platform trees, I hope that's fine.
yep, that sounds great and I'll pick the dts patches then :-)
Thanks
Heiko
> > > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > > index c9b341e55cbb..faedbd6ebc20 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > > @@ -152,6 +152,11 @@ static int rockchip_pcie_resource_get(struct platform_device *pdev,
> > > > if (IS_ERR(rockchip->rst_gpio))
> > > > return PTR_ERR(rockchip->rst_gpio);
> > > >
> > > > + rockchip->rst = devm_reset_control_array_get_exclusive(&pdev->dev);
> > > > + if (IS_ERR(rockchip->rst))
> > > > + return dev_err_probe(&pdev->dev, PTR_ERR(rockchip->rst),
> > > > + "failed to get reset lines\n");
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > @@ -182,18 +187,6 @@ static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip)
> > > > phy_power_off(rockchip->phy);
> > > > }
> > > >
> > > > -static int rockchip_pcie_reset_control_release(struct rockchip_pcie *rockchip)
> > > > -{
> > > > - struct device *dev = rockchip->pci.dev;
> > > > -
> > > > - rockchip->rst = devm_reset_control_array_get_exclusive(dev);
> > > > - if (IS_ERR(rockchip->rst))
> > > > - return dev_err_probe(dev, PTR_ERR(rockchip->rst),
> > > > - "failed to get reset lines\n");
> > > > -
> > > > - return reset_control_deassert(rockchip->rst);
> > > > -}
> > > > -
> > > > static const struct dw_pcie_ops dw_pcie_ops = {
> > > > .link_up = rockchip_pcie_link_up,
> > > > .start_link = rockchip_pcie_start_link,
> > > > @@ -222,6 +215,10 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> > > > if (ret)
> > > > return ret;
> > > >
> > > > + ret = reset_control_assert(rockchip->rst);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > /* DON'T MOVE ME: must be enable before PHY init */
> > > > rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> > > > if (IS_ERR(rockchip->vpcie3v3)) {
> > > > @@ -241,7 +238,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> > > > if (ret)
> > > > goto disable_regulator;
> > > >
> > > > - ret = rockchip_pcie_reset_control_release(rockchip);
> > > > + ret = reset_control_deassert(rockchip->rst);
> > > > if (ret)
> > > > goto deinit_phy;
> > > >
> > > > --
> > > > 2.25.1
> > > >
>
On Fri, 29 Apr 2022 08:38:26 -0400, Peter Geis wrote:
> 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 reset the PCIe controller at driver probe to
> prevent issues in the future when firmware / kexec leaves the controller
> in an unknown state. 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.
>
> [...]
Applied to pci/dwc, thanks!
[1/5] dt-bindings: PCI: Remove fallback from Rockchip DesignWare binding
https://git.kernel.org/lpieralisi/pci/c/931262e646
[2/5] PCI: rockchip-dwc: Reset core at driver probe
https://git.kernel.org/lpieralisi/pci/c/431e7d2eec
[3/5] PCI: rockchip-dwc: Add legacy interrupt support
https://git.kernel.org/lpieralisi/pci/c/e8aae154df
Thanks,
Lorenzo
On Fri, Apr 29, 2022 at 08:38:28AM -0400, Peter Geis wrote:
> The PCIe controller is in an unknown state at driver probe. This can
> lead to undesireable effects when the driver attempts to configure the
> controller.
>
> Prevent issues in the future by resetting the core during probe.
>
> Signed-off-by: Peter Geis <[email protected]>
> Tested-by: Nicolas Frattaroli <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 23 ++++++++-----------
> 1 file changed, 10 insertions(+), 13 deletions(-)
I fear that the controller reset behaviour is bootloader/firmware
dependent.
Are we sure we are not triggering any regressions by resetting the
controller in the middle of probe (aka is the driver implicitly
relying on existing behaviour on systems that are not the ones
you are testing on) ?
Just asking, the rockchip maintainers should be able to answer this
question.
Thanks,
Lorenzo
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index c9b341e55cbb..faedbd6ebc20 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -152,6 +152,11 @@ static int rockchip_pcie_resource_get(struct platform_device *pdev,
> if (IS_ERR(rockchip->rst_gpio))
> return PTR_ERR(rockchip->rst_gpio);
>
> + rockchip->rst = devm_reset_control_array_get_exclusive(&pdev->dev);
> + if (IS_ERR(rockchip->rst))
> + return dev_err_probe(&pdev->dev, PTR_ERR(rockchip->rst),
> + "failed to get reset lines\n");
> +
> return 0;
> }
>
> @@ -182,18 +187,6 @@ static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip)
> phy_power_off(rockchip->phy);
> }
>
> -static int rockchip_pcie_reset_control_release(struct rockchip_pcie *rockchip)
> -{
> - struct device *dev = rockchip->pci.dev;
> -
> - rockchip->rst = devm_reset_control_array_get_exclusive(dev);
> - if (IS_ERR(rockchip->rst))
> - return dev_err_probe(dev, PTR_ERR(rockchip->rst),
> - "failed to get reset lines\n");
> -
> - return reset_control_deassert(rockchip->rst);
> -}
> -
> static const struct dw_pcie_ops dw_pcie_ops = {
> .link_up = rockchip_pcie_link_up,
> .start_link = rockchip_pcie_start_link,
> @@ -222,6 +215,10 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + ret = reset_control_assert(rockchip->rst);
> + if (ret)
> + return ret;
> +
> /* DON'T MOVE ME: must be enable before PHY init */
> rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> if (IS_ERR(rockchip->vpcie3v3)) {
> @@ -241,7 +238,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> if (ret)
> goto disable_regulator;
>
> - ret = rockchip_pcie_reset_control_release(rockchip);
> + ret = reset_control_deassert(rockchip->rst);
> if (ret)
> goto deinit_phy;
>
> --
> 2.25.1
>
On Wed, May 11, 2022 at 9:50 AM Lorenzo Pieralisi
<[email protected]> wrote:
>
> On Fri, Apr 29, 2022 at 08:38:28AM -0400, Peter Geis wrote:
> > The PCIe controller is in an unknown state at driver probe. This can
> > lead to undesireable effects when the driver attempts to configure the
> > controller.
> >
> > Prevent issues in the future by resetting the core during probe.
> >
> > Signed-off-by: Peter Geis <[email protected]>
> > Tested-by: Nicolas Frattaroli <[email protected]>
> > ---
> > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 23 ++++++++-----------
> > 1 file changed, 10 insertions(+), 13 deletions(-)
>
> I fear that the controller reset behaviour is bootloader/firmware
> dependent.
>
> Are we sure we are not triggering any regressions by resetting the
> controller in the middle of probe (aka is the driver implicitly
> relying on existing behaviour on systems that are not the ones
> you are testing on) ?
>
> Just asking, the rockchip maintainers should be able to answer this
> question.
This is a new driver with no current users, this series enables the
first user. It does not support ACPI nor any sort of handoff at this
time.
>
> Thanks,
> Lorenzo
>
> > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > index c9b341e55cbb..faedbd6ebc20 100644
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -152,6 +152,11 @@ static int rockchip_pcie_resource_get(struct platform_device *pdev,
> > if (IS_ERR(rockchip->rst_gpio))
> > return PTR_ERR(rockchip->rst_gpio);
> >
> > + rockchip->rst = devm_reset_control_array_get_exclusive(&pdev->dev);
> > + if (IS_ERR(rockchip->rst))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(rockchip->rst),
> > + "failed to get reset lines\n");
> > +
> > return 0;
> > }
> >
> > @@ -182,18 +187,6 @@ static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip)
> > phy_power_off(rockchip->phy);
> > }
> >
> > -static int rockchip_pcie_reset_control_release(struct rockchip_pcie *rockchip)
> > -{
> > - struct device *dev = rockchip->pci.dev;
> > -
> > - rockchip->rst = devm_reset_control_array_get_exclusive(dev);
> > - if (IS_ERR(rockchip->rst))
> > - return dev_err_probe(dev, PTR_ERR(rockchip->rst),
> > - "failed to get reset lines\n");
> > -
> > - return reset_control_deassert(rockchip->rst);
> > -}
> > -
> > static const struct dw_pcie_ops dw_pcie_ops = {
> > .link_up = rockchip_pcie_link_up,
> > .start_link = rockchip_pcie_start_link,
> > @@ -222,6 +215,10 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> > if (ret)
> > return ret;
> >
> > + ret = reset_control_assert(rockchip->rst);
> > + if (ret)
> > + return ret;
> > +
> > /* DON'T MOVE ME: must be enable before PHY init */
> > rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> > if (IS_ERR(rockchip->vpcie3v3)) {
> > @@ -241,7 +238,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> > if (ret)
> > goto disable_regulator;
> >
> > - ret = rockchip_pcie_reset_control_release(rockchip);
> > + ret = reset_control_deassert(rockchip->rst);
> > if (ret)
> > goto deinit_phy;
> >
> > --
> > 2.25.1
> >
On Wed, May 11, 2022 at 10:26:20AM -0400, Peter Geis wrote:
> On Wed, May 11, 2022 at 9:50 AM Lorenzo Pieralisi
> <[email protected]> wrote:
> >
> > On Fri, Apr 29, 2022 at 08:38:28AM -0400, Peter Geis wrote:
> > > The PCIe controller is in an unknown state at driver probe. This can
> > > lead to undesireable effects when the driver attempts to configure the
> > > controller.
> > >
> > > Prevent issues in the future by resetting the core during probe.
> > >
> > > Signed-off-by: Peter Geis <[email protected]>
> > > Tested-by: Nicolas Frattaroli <[email protected]>
> > > ---
> > > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 23 ++++++++-----------
> > > 1 file changed, 10 insertions(+), 13 deletions(-)
> >
> > I fear that the controller reset behaviour is bootloader/firmware
> > dependent.
> >
> > Are we sure we are not triggering any regressions by resetting the
> > controller in the middle of probe (aka is the driver implicitly
> > relying on existing behaviour on systems that are not the ones
> > you are testing on) ?
> >
> > Just asking, the rockchip maintainers should be able to answer this
> > question.
>
> This is a new driver with no current users, this series enables the
> first user. It does not support ACPI nor any sort of handoff at this
> time.
Ok. I will take patches [1-3], dts changes will have to go via
platform trees, I hope that's fine.
Thanks,
Lorenzo
> > Thanks,
> > Lorenzo
> >
> > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > index c9b341e55cbb..faedbd6ebc20 100644
> > > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > @@ -152,6 +152,11 @@ static int rockchip_pcie_resource_get(struct platform_device *pdev,
> > > if (IS_ERR(rockchip->rst_gpio))
> > > return PTR_ERR(rockchip->rst_gpio);
> > >
> > > + rockchip->rst = devm_reset_control_array_get_exclusive(&pdev->dev);
> > > + if (IS_ERR(rockchip->rst))
> > > + return dev_err_probe(&pdev->dev, PTR_ERR(rockchip->rst),
> > > + "failed to get reset lines\n");
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -182,18 +187,6 @@ static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip)
> > > phy_power_off(rockchip->phy);
> > > }
> > >
> > > -static int rockchip_pcie_reset_control_release(struct rockchip_pcie *rockchip)
> > > -{
> > > - struct device *dev = rockchip->pci.dev;
> > > -
> > > - rockchip->rst = devm_reset_control_array_get_exclusive(dev);
> > > - if (IS_ERR(rockchip->rst))
> > > - return dev_err_probe(dev, PTR_ERR(rockchip->rst),
> > > - "failed to get reset lines\n");
> > > -
> > > - return reset_control_deassert(rockchip->rst);
> > > -}
> > > -
> > > static const struct dw_pcie_ops dw_pcie_ops = {
> > > .link_up = rockchip_pcie_link_up,
> > > .start_link = rockchip_pcie_start_link,
> > > @@ -222,6 +215,10 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> > > if (ret)
> > > return ret;
> > >
> > > + ret = reset_control_assert(rockchip->rst);
> > > + if (ret)
> > > + return ret;
> > > +
> > > /* DON'T MOVE ME: must be enable before PHY init */
> > > rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> > > if (IS_ERR(rockchip->vpcie3v3)) {
> > > @@ -241,7 +238,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> > > if (ret)
> > > goto disable_regulator;
> > >
> > > - ret = rockchip_pcie_reset_control_release(rockchip);
> > > + ret = reset_control_deassert(rockchip->rst);
> > > if (ret)
> > > goto deinit_phy;
> > >
> > > --
> > > 2.25.1
> > >
On Fri, 29 Apr 2022 08:38:26 -0400, Peter Geis wrote:
> 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 reset the PCIe controller at driver probe to
> prevent issues in the future when firmware / kexec leaves the controller
> in an unknown state. 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.
>
> [...]
Applied, thanks!
[4/5] arm64: dts: rockchip: Add rk3568 PCIe2x1 controller
commit: c9168492af55bdbc811e05bfc55ae70880bf8ff3
[5/5] arm64: dts: rockchip: Enable PCIe controller on quartz64-a
commit: 4f4cbbb147b988daaa036dcf34628d93b2e22cd9
Best regards,
--
Heiko Stuebner <[email protected]>
Am Freitag, 29. April 2022, 14:38:30 CEST schrieb Peter Geis:
> The PCIe2x1 controller is common between the rk3568 and rk3566. It is a
> single lane PCIe2 compliant controller.
>
> Signed-off-by: Peter Geis <[email protected]>
> ---
> arch/arm64/boot/dts/rockchip/rk356x.dtsi | 52 ++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> index 7cdef800cb3c..aea5d9255235 100644
> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> @@ -689,6 +689,58 @@ 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>;
> + 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>;
I guess #interrupt-cells shouldn't be necessary here, as that property
is meant for interrupt-controller nodes - like the subnode here
which already has its own #interrupt-cells, right?
> + 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 &gic 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 0x01100000 0x0 0x3ef00000>;
> + 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>;
>