On 12/8/20 5:40 PM, Bjorn Helgaas wrote:
[...]
The R-Car PCIe controller is capable of handling L0s/L1 link states.
Minor wording nit: L0s seems irrelevant to this patch.
Of course.
All PCIe functions are required to support the Power Management
Capability (PCIe r5.0, sec 7.5.2), and that in turn requires D0,
D3hot, and D3cold support, and D3hot requires L1 (sec 5.2).
So saying this device "is capable of handling L1" really doesn't tell
us anything, and it glosses over the fact that it doesn't do it
*correctly* and requires help from the driver to work around this
hardware defect.
I see.
Does this problem occur in both these cases?
1) When ASPM enters L1, and
2) When software writes PCI_PM_CTRL to put the device in D3hot?
IIUC both cases require the link to go to L1. I guess the same
software workaround applies to both cases?
Yes
[...]
+#ifdef CONFIG_ARM
+static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
+ unsigned int fsr, struct pt_regs *regs)
+{
+ u32 pmsr;
+
+ if (!pcie_base || !__clk_is_enabled(pcie_bus_clk))
+ return 1;
+
+ pmsr = readl(pcie_base + PMSR);
+
+ /*
+ * Test if the PCIe controller received PM_ENTER_L1 DLLP and
+ * the PCIe controller is not in L1 link state. If true, apply
+ * fix, which will put the controller into L1 link state, from
+ * which it can return to L0s/L0 on its own.
+ */
+ if ((pmsr & PMEL1RX) && ((pmsr & PMSTATE) != PMSTATE_L1)) {
+ writel(L1IATN, pcie_base + PMCTLR);
+ while (!(readl(pcie_base + PMSR) & L1FAEG))
+ ;
+ writel(L1FAEG | PMEL1RX, pcie_base + PMSR);
+ return 0;
+ }
+
+ return 1;
I have no insight into how these abort handlers work. Looks awfully
kludgy to me, but if it's the only way and the ARM folks are on board
with it, I can't object.
I guess the other alternative would be to have a quirk to stop
advertising ASPM L1 support and D1/D2/D3hot support. Obviously that
may give up some power savings.
If people aren't comfortable with the reliability or maintainability
of this approach in the upstream kernel, there's always the option of
the users who need it carrying this as an out-of-tree patch.
I would highly prefer to be able to use mainline Linux as-is, without
carrying any extra patches, so BSP is not an option.
+}
+
+static const struct of_device_id rcar_pcie_abort_handler_of_match[] __initconst = {
+ { .compatible = "renesas,pcie-r8a7779" },
+ { .compatible = "renesas,pcie-r8a7790" },
+ { .compatible = "renesas,pcie-r8a7791" },
+ { .compatible = "renesas,pcie-rcar-gen2" },
+ {},
+};
Why do we need another copy of these, as opposed to doing something
with of_device_get_match_data(), e.g., like brcm_pcie_probe() does?
This is not a copy, but as subset of SoCs which are affected by this
problem.
+static int __init rcar_pcie_init(void)
+{
+ if (of_find_matching_node(NULL, rcar_pcie_abort_handler_of_match)) {
+#ifdef CONFIG_ARM_LPAE
+ hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
+ "asynchronous external abort");
+#else
+ hook_fault_code(22, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
+ "imprecise external abort");
+#endif
+ }
+
+ return platform_driver_register(&rcar_pcie_driver);
+}
+device_initcall(rcar_pcie_init);
+#else
builtin_platform_driver(rcar_pcie_driver);
+#endif
Is the device_initcall() vs builtin_platform_driver() something
related to the hook_fault_code()? What would break if this were
always builtin_platform_driver()?
rcar_pcie_init() would not be called before probe.
