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:
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 (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 | 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 | 112 +++++++++++++++++-
4 files changed, 197 insertions(+), 13 deletions(-)
--
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 | 112 +++++++++++++++++-
1 file changed, 110 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..863374604fb1 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,94 @@ 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, ®, 8)
+ generic_handle_domain_irq(rockchip->irq_domain, 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 = {
+ .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;
+
+ 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 +207,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;
+
+ 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
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]>
---
.../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
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 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
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 ca20d7b91fe5..bb08633332c7 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -722,6 +722,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
On Mon, 18 Apr 2022 12:37:00 +0100,
Peter Geis <[email protected]> wrote:
>
> On Sun, Apr 17, 2022 at 5:53 AM Marc Zyngier <[email protected]> wrote:
> >
> > On Sat, 16 Apr 2022 14:24:26 +0100,
> > Peter Geis <[email protected]> wrote:
> > >
> > > Okay, that makes sense. I'm hitting the entire block when it should be
> > > the individual IRQ.
> > > I also notice some drivers protect this with a spinlock while others
> > > do not, how should this be handled?
> >
> > It obviously depends on how the HW. works. If this is a shared
> > register using a RMW sequence, then you need some form of mutual
> > exclusion in order to preserve the atomicity of the update.
> >
> > If the HW supports updating the masks using a set of hot bits (with
> > separate clear/set registers), than there is no need for locking. In
> > your case PCIE_CLIENT_INTR_MASK_LEGACY seems to support this odd
> > "write-enable" feature which can probably be used to implement a
> > lockless access, something like:
> >
> > void mask(struct irq_data *d)
> > {
> > u32 val = BIT(d->hwirq + 16) | BIT(d->hwirq);
>
> This is what HIWORD_UPDATE_BIT does, it's rather common in Rockchip code.
> I believe I can safely drop the spinlock when enabling/disabling
> individual interrupts.
Yes.
>
> > writel_relaxed(val, ...);
> > }
> >
> > void mask(struct irq_data *d)
> > {
> > u32 val = BIT(d->hwirq + 16);
> > writel_relaxed(val, ...);
> > }
> >
> > Another thing is that it is completely unclear to me what initialises
> > these interrupts the first place (INTR_MASK_LEGACY, INTR_EN_LEGACY).
> > Are you relying on the firmware to do that for you?
>
> There is no dedicated mask or enable/disable for the legacy interrupt
> line (unless it's undocumented).
I'm talking about the INTR_MASK_LEGACY and INTR_EN_LEGACY registers,
which control the INTx (although the latter seems to default to some
reserved values). I don't see where you initialise them to a state
where they are enabled and masked, which should be the initial state
once this driver has probed. The output interrupt itself is obviously
controlled by the GIC driver.
> It appears to be enabled via an "or" function with the emulated interrupts.
> As far as I can tell this is common for dw-pcie, looking at the other drivers.
I think we're talking past each other. I'm solely concerned with the
initialisation of the input control registers, for which I see no code
in this patch.
M.
--
Without deviation from the norm, progress is not possible.
On Mon, Apr 18, 2022 at 6:53 PM Marc Zyngier <[email protected]> wrote:
>
> On Mon, 18 Apr 2022 16:13:39 +0100,
> Peter Geis <[email protected]> wrote:
> >
> > On Mon, Apr 18, 2022 at 8:34 AM Marc Zyngier <[email protected]> wrote:
> > >
> > > On Mon, 18 Apr 2022 12:37:00 +0100,
> > > Peter Geis <[email protected]> wrote:
> > > >
> > > > On Sun, Apr 17, 2022 at 5:53 AM Marc Zyngier <[email protected]> wrote:
> > > > >
> > > > > On Sat, 16 Apr 2022 14:24:26 +0100,
> > > > > Peter Geis <[email protected]> wrote:
> > > > > >
> > > > > > Okay, that makes sense. I'm hitting the entire block when it should be
> > > > > > the individual IRQ.
> > > > > > I also notice some drivers protect this with a spinlock while others
> > > > > > do not, how should this be handled?
> > > > >
> > > > > It obviously depends on how the HW. works. If this is a shared
> > > > > register using a RMW sequence, then you need some form of mutual
> > > > > exclusion in order to preserve the atomicity of the update.
> > > > >
> > > > > If the HW supports updating the masks using a set of hot bits (with
> > > > > separate clear/set registers), than there is no need for locking. In
> > > > > your case PCIE_CLIENT_INTR_MASK_LEGACY seems to support this odd
> > > > > "write-enable" feature which can probably be used to implement a
> > > > > lockless access, something like:
> > > > >
> > > > > void mask(struct irq_data *d)
> > > > > {
> > > > > u32 val = BIT(d->hwirq + 16) | BIT(d->hwirq);
> > > >
> > > > This is what HIWORD_UPDATE_BIT does, it's rather common in Rockchip code.
> > > > I believe I can safely drop the spinlock when enabling/disabling
> > > > individual interrupts.
> > >
> > > Yes.
> > >
> > > >
> > > > > writel_relaxed(val, ...);
> > > > > }
> > > > >
> > > > > void mask(struct irq_data *d)
> > > > > {
> > > > > u32 val = BIT(d->hwirq + 16);
> > > > > writel_relaxed(val, ...);
> > > > > }
> > > > >
> > > > > Another thing is that it is completely unclear to me what initialises
> > > > > these interrupts the first place (INTR_MASK_LEGACY, INTR_EN_LEGACY).
> > > > > Are you relying on the firmware to do that for you?
> > > >
> > > > There is no dedicated mask or enable/disable for the legacy interrupt
> > > > line (unless it's undocumented).
> > >
> > > I'm talking about the INTR_MASK_LEGACY and INTR_EN_LEGACY registers,
> > > which control the INTx (although the latter seems to default to some
> > > reserved values). I don't see where you initialise them to a state
> > > where they are enabled and masked, which should be the initial state
> > > once this driver has probed. The output interrupt itself is obviously
> > > controlled by the GIC driver.
> >
> > PCIE_CLIENT_INTR_MASK_LEGACY is the register I use here to mask/unmask
> > the interrupts.
> > It defaults to all masked on reset.
>
> And? Are your really expecting that the firmware that runs before the
> kernel will preserve this register and not write anything silly to it
> because, oh wait, it wants to use interrupts? Or that nobody will
> kexec a secondary kernel from the first one after having used these
> interrupts?
>
> Rule #1: Initialise the HW to sensible values
> Rule #2: See Rule #1
I don't disagree here, there are plenty of examples of bugs that stem
from this in Rockchip's code.
Working on this series has given me ideas for improvements to the
rk3399 controller as well.
>
> > The current rk3568 trm v1.1 does not reference an INTR_EN_LEGACY register.
>
> The TRM for RK3588 mentions it, and is the same IP.
Unfortunately this assumption doesn't hold up to testing.
On rk356x this entire register block is 0x0, which if it was
implemented means legacy interrupts would not work, among other
issues.
Even in the rk3588 it's marked as "reserved" which means there's a
good possibility it isn't fully implemented there either.
A number of other blocks in the rk3588 trm are labeled as being
available only after a specific hardware revision.
We are seeing other bugs in the hardware implementation Rockchip has
here, so making assumptions based on other implementations of the DW
IP is unsafe.
>
> > >
> > > > It appears to be enabled via an "or" function with the emulated interrupts.
> > > > As far as I can tell this is common for dw-pcie, looking at the other drivers.
> > >
> > > I think we're talking past each other. I'm solely concerned with the
> > > initialisation of the input control registers, for which I see no code
> > > in this patch.
> >
> > Downstream points to the mask/unmask functions for the enable/disable
> > functions, which would be superfluous here as mainline defaults to
> > that anyways if they are null.
>
> Yeah, that's completely dumb. But there is no shortage of dumb stuff
> in the RK downstream code...
You'll find no argument from me here, I'm merely using it as an
example of the vendor's implementation.
The only resources I have available are the publically released
documentation and the publically released downstream code.
>
> >
> > I've double checked and downstream only uses the mask register, enable
> > and routing config appears to be left as is from reset.
>
> And that's a bug.
>
> > I'm rather concerned about the lack of any obvious way to control
> > routing, nor an ack mechanism for the irq.
>
> Which routing? Do you mean the affinity? You can't change it, as this
> would change the affinity of all interrupts at once.
>
> > I see other implementations reference the core registers or vendor
> > defined registers for these functions.
> > Unfortunately the rk3568 trm does not include the core register
> > definitions, and the designware documentation appears to be behind a
> > paywall/nda.
>
> If you use a search engine, you'll find *CONFIDENTIAL* copies of the
> DW stuff. The whole thing is a laugh anyway.
>
> >
> > I suspect most of the confusion here boils down to a lack of
> > documentation, but it's entirely possible I am simply not
> > understanding the question.
>
> My only ask is that you properly initialise the HW. This will save
> countless amount of head-scratching once you have a decent firmware or
> kexec.
The only way to ensure that in a sane way is to trigger the resets at
driver probe.
Can that be safely done without causing other issues with an already
configured card or should I power cycle it as well?
This is starting to feature creep from the original intention of this
series, since a pre-configured controller would affect more than just
interrupts.
If you wish, as a compromise I can ensure all INTx interrupts are
masked at probe (which would hilariously be the opposite of
downstream).
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
On Mon, 18 Apr 2022 16:13:39 +0100,
Peter Geis <[email protected]> wrote:
>
> On Mon, Apr 18, 2022 at 8:34 AM Marc Zyngier <[email protected]> wrote:
> >
> > On Mon, 18 Apr 2022 12:37:00 +0100,
> > Peter Geis <[email protected]> wrote:
> > >
> > > On Sun, Apr 17, 2022 at 5:53 AM Marc Zyngier <[email protected]> wrote:
> > > >
> > > > On Sat, 16 Apr 2022 14:24:26 +0100,
> > > > Peter Geis <[email protected]> wrote:
> > > > >
> > > > > Okay, that makes sense. I'm hitting the entire block when it should be
> > > > > the individual IRQ.
> > > > > I also notice some drivers protect this with a spinlock while others
> > > > > do not, how should this be handled?
> > > >
> > > > It obviously depends on how the HW. works. If this is a shared
> > > > register using a RMW sequence, then you need some form of mutual
> > > > exclusion in order to preserve the atomicity of the update.
> > > >
> > > > If the HW supports updating the masks using a set of hot bits (with
> > > > separate clear/set registers), than there is no need for locking. In
> > > > your case PCIE_CLIENT_INTR_MASK_LEGACY seems to support this odd
> > > > "write-enable" feature which can probably be used to implement a
> > > > lockless access, something like:
> > > >
> > > > void mask(struct irq_data *d)
> > > > {
> > > > u32 val = BIT(d->hwirq + 16) | BIT(d->hwirq);
> > >
> > > This is what HIWORD_UPDATE_BIT does, it's rather common in Rockchip code.
> > > I believe I can safely drop the spinlock when enabling/disabling
> > > individual interrupts.
> >
> > Yes.
> >
> > >
> > > > writel_relaxed(val, ...);
> > > > }
> > > >
> > > > void mask(struct irq_data *d)
> > > > {
> > > > u32 val = BIT(d->hwirq + 16);
> > > > writel_relaxed(val, ...);
> > > > }
> > > >
> > > > Another thing is that it is completely unclear to me what initialises
> > > > these interrupts the first place (INTR_MASK_LEGACY, INTR_EN_LEGACY).
> > > > Are you relying on the firmware to do that for you?
> > >
> > > There is no dedicated mask or enable/disable for the legacy interrupt
> > > line (unless it's undocumented).
> >
> > I'm talking about the INTR_MASK_LEGACY and INTR_EN_LEGACY registers,
> > which control the INTx (although the latter seems to default to some
> > reserved values). I don't see where you initialise them to a state
> > where they are enabled and masked, which should be the initial state
> > once this driver has probed. The output interrupt itself is obviously
> > controlled by the GIC driver.
>
> PCIE_CLIENT_INTR_MASK_LEGACY is the register I use here to mask/unmask
> the interrupts.
> It defaults to all masked on reset.
And? Are your really expecting that the firmware that runs before the
kernel will preserve this register and not write anything silly to it
because, oh wait, it wants to use interrupts? Or that nobody will
kexec a secondary kernel from the first one after having used these
interrupts?
Rule #1: Initialise the HW to sensible values
Rule #2: See Rule #1
> The current rk3568 trm v1.1 does not reference an INTR_EN_LEGACY register.
The TRM for RK3588 mentions it, and is the same IP.
> >
> > > It appears to be enabled via an "or" function with the emulated interrupts.
> > > As far as I can tell this is common for dw-pcie, looking at the other drivers.
> >
> > I think we're talking past each other. I'm solely concerned with the
> > initialisation of the input control registers, for which I see no code
> > in this patch.
>
> Downstream points to the mask/unmask functions for the enable/disable
> functions, which would be superfluous here as mainline defaults to
> that anyways if they are null.
Yeah, that's completely dumb. But there is no shortage of dumb stuff
in the RK downstream code...
>
> I've double checked and downstream only uses the mask register, enable
> and routing config appears to be left as is from reset.
And that's a bug.
> I'm rather concerned about the lack of any obvious way to control
> routing, nor an ack mechanism for the irq.
Which routing? Do you mean the affinity? You can't change it, as this
would change the affinity of all interrupts at once.
> I see other implementations reference the core registers or vendor
> defined registers for these functions.
> Unfortunately the rk3568 trm does not include the core register
> definitions, and the designware documentation appears to be behind a
> paywall/nda.
If you use a search engine, you'll find *CONFIDENTIAL* copies of the
DW stuff. The whole thing is a laugh anyway.
>
> I suspect most of the confusion here boils down to a lack of
> documentation, but it's entirely possible I am simply not
> understanding the question.
My only ask is that you properly initialise the HW. This will save
countless amount of head-scratching once you have a decent firmware or
kexec.
M.
--
Without deviation from the norm, progress is not possible.
On Tue, 19 Apr 2022 01:23:23 +0100,
Peter Geis <[email protected]> wrote:
>
> > My only ask is that you properly initialise the HW. This will save
> > countless amount of head-scratching once you have a decent firmware or
> > kexec.
>
> The only way to ensure that in a sane way is to trigger the resets at
> driver probe.
If that can be done, that'd be great.
> Can that be safely done without causing other issues with an already
> configured card or should I power cycle it as well?
Well, you are already renegotiating the link anyway, so that's a very
moot point.
> This is starting to feature creep from the original intention of this
> series, since a pre-configured controller would affect more than just
> interrupts.
Configuring the HW is not exactly a feature creep. If your intention
is to keep everything as it was left, then you don't have much of a
driver, but instead a time bomb. And we can do without another one in
the tree.
> If you wish, as a compromise I can ensure all INTx interrupts are
> masked at probe (which would hilariously be the opposite of
> downstream).
As far as I'm concerned, downstream doesn't exist. If someone wants
the downstream code, they can use it directly and we don't need to
merge this code.
If, on the other hand, you want this driver to be useful and to be
maintained upstream, initialising the interrupt mask is the absolute
bare minimum.
M.
--
Without deviation from the norm, progress is not possible.
On Mon, Apr 18, 2022 at 8:34 AM Marc Zyngier <[email protected]> wrote:
>
> On Mon, 18 Apr 2022 12:37:00 +0100,
> Peter Geis <[email protected]> wrote:
> >
> > On Sun, Apr 17, 2022 at 5:53 AM Marc Zyngier <[email protected]> wrote:
> > >
> > > On Sat, 16 Apr 2022 14:24:26 +0100,
> > > Peter Geis <[email protected]> wrote:
> > > >
> > > > Okay, that makes sense. I'm hitting the entire block when it should be
> > > > the individual IRQ.
> > > > I also notice some drivers protect this with a spinlock while others
> > > > do not, how should this be handled?
> > >
> > > It obviously depends on how the HW. works. If this is a shared
> > > register using a RMW sequence, then you need some form of mutual
> > > exclusion in order to preserve the atomicity of the update.
> > >
> > > If the HW supports updating the masks using a set of hot bits (with
> > > separate clear/set registers), than there is no need for locking. In
> > > your case PCIE_CLIENT_INTR_MASK_LEGACY seems to support this odd
> > > "write-enable" feature which can probably be used to implement a
> > > lockless access, something like:
> > >
> > > void mask(struct irq_data *d)
> > > {
> > > u32 val = BIT(d->hwirq + 16) | BIT(d->hwirq);
> >
> > This is what HIWORD_UPDATE_BIT does, it's rather common in Rockchip code.
> > I believe I can safely drop the spinlock when enabling/disabling
> > individual interrupts.
>
> Yes.
>
> >
> > > writel_relaxed(val, ...);
> > > }
> > >
> > > void mask(struct irq_data *d)
> > > {
> > > u32 val = BIT(d->hwirq + 16);
> > > writel_relaxed(val, ...);
> > > }
> > >
> > > Another thing is that it is completely unclear to me what initialises
> > > these interrupts the first place (INTR_MASK_LEGACY, INTR_EN_LEGACY).
> > > Are you relying on the firmware to do that for you?
> >
> > There is no dedicated mask or enable/disable for the legacy interrupt
> > line (unless it's undocumented).
>
> I'm talking about the INTR_MASK_LEGACY and INTR_EN_LEGACY registers,
> which control the INTx (although the latter seems to default to some
> reserved values). I don't see where you initialise them to a state
> where they are enabled and masked, which should be the initial state
> once this driver has probed. The output interrupt itself is obviously
> controlled by the GIC driver.
PCIE_CLIENT_INTR_MASK_LEGACY is the register I use here to mask/unmask
the interrupts.
It defaults to all masked on reset.
The current rk3568 trm v1.1 does not reference an INTR_EN_LEGACY register.
>
> > It appears to be enabled via an "or" function with the emulated interrupts.
> > As far as I can tell this is common for dw-pcie, looking at the other drivers.
>
> I think we're talking past each other. I'm solely concerned with the
> initialisation of the input control registers, for which I see no code
> in this patch.
Downstream points to the mask/unmask functions for the enable/disable
functions, which would be superfluous here as mainline defaults to
that anyways if they are null.
I've double checked and downstream only uses the mask register, enable
and routing config appears to be left as is from reset.
I'm rather concerned about the lack of any obvious way to control
routing, nor an ack mechanism for the irq.
I see other implementations reference the core registers or vendor
defined registers for these functions.
Unfortunately the rk3568 trm does not include the core register
definitions, and the designware documentation appears to be behind a
paywall/nda.
I suspect most of the confusion here boils down to a lack of
documentation, but it's entirely possible I am simply not
understanding the question.
I'm already aware of other functions that I need documentation for
that is currently unavailable.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
Thank you for your time,
Peter
On Tue, Apr 19, 2022 at 4:05 AM Marc Zyngier <[email protected]> wrote:
>
> On Tue, 19 Apr 2022 01:23:23 +0100,
> Peter Geis <[email protected]> wrote:
> >
> > > My only ask is that you properly initialise the HW. This will save
> > > countless amount of head-scratching once you have a decent firmware or
> > > kexec.
> >
> > The only way to ensure that in a sane way is to trigger the resets at
> > driver probe.
>
> If that can be done, that'd be great.
Okay, I'll work on implementing this then.
>
> > Can that be safely done without causing other issues with an already
> > configured card or should I power cycle it as well?
>
> Well, you are already renegotiating the link anyway, so that's a very
> moot point.
Understood, thank you.
>
> > This is starting to feature creep from the original intention of this
> > series, since a pre-configured controller would affect more than just
> > interrupts.
>
> Configuring the HW is not exactly a feature creep. If your intention
> is to keep everything as it was left, then you don't have much of a
> driver, but instead a time bomb. And we can do without another one in
> the tree.
Understood, I apologize if I'm being difficult here, I just want to
make sure I completely understand the assignment.
Your comment about kexec made it clear for me, thank you.
>
> > If you wish, as a compromise I can ensure all INTx interrupts are
> > masked at probe (which would hilariously be the opposite of
> > downstream).
>
> As far as I'm concerned, downstream doesn't exist. If someone wants
> the downstream code, they can use it directly and we don't need to
> merge this code.
Once again, you'll have no argument from me in this regard.
I've had several blocks of hardware enablement sitting out of tree
waiting for the phy code to land.
As much testing as my branch has seen, it's still only a drop in the
bucket compared to finally being mainlined.
I appreciate all of your effort and review here and I absolutely want
this done correctly.
>
> If, on the other hand, you want this driver to be useful and to be
> maintained upstream, initialising the interrupt mask is the absolute
> bare minimum.
I think resetting the whole core is the best move, since it's the only
way we can guarantee a sane configuration with the documentation we
have.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
On Sat, 16 Apr 2022 07:05:03 -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 <[email protected]>
> ---
> .../devicetree/bindings/pci/rockchip-dw-pcie.yaml | 12 +-----------
> 1 file changed, 1 insertion(+), 11 deletions(-)
>
Reviewed-by: Rob Herring <[email protected]>