| Message ID | 20201011160045.574323-1-marek.vasut@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [V3] PCI: rcar: Add L1 link state fix into data abort hook | expand |
Hi Marek, On Sun, Oct 11, 2020 at 6:00 PM <marek.vasut@gmail.com> wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > The R-Car PCIe controller is capable of handling L0s/L1 link states. > While the controller can enter and exit L0s link state, and exit L1 > link state, without any additional action from the driver, to enter > L1 link state, the driver must complete the link state transition by > issuing additional commands to the controller. > > The problem is, this transition is not atomic. The controller sets > PMEL1RX bit in PMSR register upon reception of PM_ENTER_L1 DLLP from > the PCIe card, but then the controller enters some sort of inbetween > state. The driver must detect this condition and complete the link > state transition, by setting L1IATN bit in PMCTLR and waiting for > the link state transition to complete. > > If a PCIe access happens inside this window, where the controller > is between L0 and L1 link states, the access generates a fault and > the ARM 'imprecise external abort' handler is invoked. > > Just like other PCI controller drivers, here we hook the fault handler, > perform the fixup to help the controller enter L1 link state, and then > restart the instruction which triggered the fault. Since the controller > is in L1 link state now, the link can exit from L1 link state to L0 and > successfully complete the access. > > Note that this fixup is applicable only to Aarch32 R-Car controllers, > the Aarch64 R-Car perform the same fixup in TFA, see TFA commit [1] > 0969397f2 ("rcar_gen3: plat: Prevent PCIe hang during L1X config access") > [1] https://github.com/ARM-software/arm-trusted-firmware/commit/0969397f295621aa26b3d14b76dd397d22be58bf > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > V3: - Fix commit message according to spellchecker > - Use of_find_matching_node() to apply hook only on Gen1 and Gen2 RCar > (in case the kernel is multiplatform) Thanks for the update! > --- a/drivers/pci/controller/pcie-rcar-host.c > +++ b/drivers/pci/controller/pcie-rcar-host.c > @@ -1050,4 +1072,58 @@ static struct platform_driver rcar_pcie_driver = { > }, > .probe = rcar_pcie_probe, > }; > + > +#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; > +} > + > +static const struct of_device_id rcar_pcie_abort_handler_of_match[] = { __initconst (if you intend to keep this, see below). > + { .compatible = "renesas,pcie-r8a7779" }, > + { .compatible = "renesas,pcie-r8a7790" }, > + { .compatible = "renesas,pcie-r8a7791" }, > + { .compatible = "renesas,pcie-rcar-gen2" }, > + {}, > +}; > + > +static int __init rcar_pcie_init(void) > +{ > + if (of_find_matching_node(NULL, rcar_pcie_abort_handler_of_match)) { I guess it doesn't really hurt to use the existing rcar_pcie_of_match[] instead? It just contains two additional entries, which will never match in the CONFIG_ARM=y case. > +#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 With the above fixed: Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> As this fixes the crash seen during s2ram with an Intel E1000E card present and the e1000e driver loaded: Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 10/12/20 9:18 AM, Geert Uytterhoeven wrote: [...] >> --- a/drivers/pci/controller/pcie-rcar-host.c >> +++ b/drivers/pci/controller/pcie-rcar-host.c > >> @@ -1050,4 +1072,58 @@ static struct platform_driver rcar_pcie_driver = { >> }, >> .probe = rcar_pcie_probe, >> }; >> + >> +#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; >> +} >> + >> +static const struct of_device_id rcar_pcie_abort_handler_of_match[] = { > > __initconst (if you intend to keep this, see below). I do, see below. >> + { .compatible = "renesas,pcie-r8a7779" }, >> + { .compatible = "renesas,pcie-r8a7790" }, >> + { .compatible = "renesas,pcie-r8a7791" }, >> + { .compatible = "renesas,pcie-rcar-gen2" }, >> + {}, >> +}; >> + >> +static int __init rcar_pcie_init(void) >> +{ >> + if (of_find_matching_node(NULL, rcar_pcie_abort_handler_of_match)) { > > I guess it doesn't really hurt to use the existing rcar_pcie_of_match[] > instead? It just contains two additional entries, which will never match > in the CONFIG_ARM=y case. Unless you try to build 32bit kernel for the R-Car3, like they do e.g. for RPi3. So I would prefer to keep this to handle that case too. [...]
diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c index cdc0963f154e..6311e2bac04e 100644 --- a/drivers/pci/controller/pcie-rcar-host.c +++ b/drivers/pci/controller/pcie-rcar-host.c @@ -13,6 +13,7 @@ #include <linux/bitops.h> #include <linux/clk.h> +#include <linux/clk-provider.h> #include <linux/delay.h> #include <linux/interrupt.h> #include <linux/irq.h> @@ -42,6 +43,21 @@ struct rcar_msi { int irq2; }; +#ifdef CONFIG_ARM +/* + * Here we keep a static copy of the remapped PCIe controller address. + * This is only used on aarch32 systems, all of which have one single + * PCIe controller, to provide quick access to the PCIe controller in + * the L1 link state fixup function, called from the ARM fault handler. + */ +static void __iomem *pcie_base; +/* + * Static copy of bus clock pointer, so we can check whether the clock + * is enabled or not. + */ +static struct clk *pcie_bus_clk; +#endif + static inline struct rcar_msi *to_rcar_msi(struct msi_controller *chip) { return container_of(chip, struct rcar_msi, chip); @@ -804,6 +820,12 @@ static int rcar_pcie_get_resources(struct rcar_pcie_host *host) } host->msi.irq2 = i; +#ifdef CONFIG_ARM + /* Cache static copy for L1 link state fixup hook on aarch32 */ + pcie_base = pcie->base; + pcie_bus_clk = host->bus_clk; +#endif + return 0; err_irq2: @@ -1050,4 +1072,58 @@ static struct platform_driver rcar_pcie_driver = { }, .probe = rcar_pcie_probe, }; + +#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; +} + +static const struct of_device_id rcar_pcie_abort_handler_of_match[] = { + { .compatible = "renesas,pcie-r8a7779" }, + { .compatible = "renesas,pcie-r8a7790" }, + { .compatible = "renesas,pcie-r8a7791" }, + { .compatible = "renesas,pcie-rcar-gen2" }, + {}, +}; + +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 diff --git a/drivers/pci/controller/pcie-rcar.h b/drivers/pci/controller/pcie-rcar.h index d4c698b5f821..9bb125db85c6 100644 --- a/drivers/pci/controller/pcie-rcar.h +++ b/drivers/pci/controller/pcie-rcar.h @@ -85,6 +85,13 @@ #define LTSMDIS BIT(31) #define MACCTLR_INIT_VAL (LTSMDIS | MACCTLR_NFTS_MASK) #define PMSR 0x01105c +#define L1FAEG BIT(31) +#define PMEL1RX BIT(23) +#define PMSTATE GENMASK(18, 16) +#define PMSTATE_L1 (3 << 16) +#define PMCTLR 0x011060 +#define L1IATN BIT(31) + #define MACS2R 0x011078 #define MACCGSPSETR 0x011084 #define SPCNGRSN BIT(31)