[V6] PCI: rcar: Add L1 link state fix into data abort hook

Commit Message

Marek Vasut May 14, 2021, 8:05 p.m. UTC

From: Marek Vasut <[e-mail]>

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.

While it was suggested to disable L1 link state support completely on
the controller level, this would not prevent the L1 link state entry
initiated by the link partner. This happens e.g. in case a PCIe card
enters D3Hot state, which could be initiated from pci_set_power_state()
if the card indicates D3Hot support, which in turn means link must enter
L1 state. So instead, fix up the L1 link state after all.

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 <[e-mail]>
Cc: Bjorn Helgaas <[e-mail]>
Cc: Geert Uytterhoeven <[e-mail]>
Cc: Lorenzo Pieralisi <[e-mail]>
Cc: Wolfram Sang <[e-mail]>
Cc: Yoshihiro Shimoda <[e-mail]>
Cc: [e-mail]
---
V2: - Update commit message, add link to TFA repository commit
    - Handle the LPAE case as in ARM fault.c and fsr-{2,3}level.c
    - Cache clock and check whether they are enabled before register
      access
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)
V4: - Mark rcar_pcie_abort_handler_of_match with __initconst
V5: - Add mutex around rcar_pcie_aarch32_abort_handler()
    - Update commit message again to point out issues with L1/D3Hot states
V6: - Return 1 only if condition cannot be fixed
---
 drivers/pci/controller/pcie-rcar-host.c | 84 +++++++++++++++++++++++++
 drivers/pci/controller/pcie-rcar.h      |  7 +++
 2 files changed, 91 insertions(+)

Comments

Geert Uytterhoeven May 17, 2021, 7:39 a.m. UTC | #1

On Fri, May 14, 2021 at 10:06 PM <[e-mail]> wrote:
> From: Marek Vasut <[e-mail]>
>
> 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.
>
> While it was suggested to disable L1 link state support completely on
> the controller level, this would not prevent the L1 link state entry
> initiated by the link partner. This happens e.g. in case a PCIe card
> enters D3Hot state, which could be initiated from pci_set_power_state()
> if the card indicates D3Hot support, which in turn means link must enter
> L1 state. So instead, fix up the L1 link state after all.
>
> 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 <[e-mail]>

Reviewed-by: Geert Uytterhoeven <[e-mail]>

> ---
> V2: - Update commit message, add link to TFA repository commit
>     - Handle the LPAE case as in ARM fault.c and fsr-{2,3}level.c
>     - Cache clock and check whether they are enabled before register
>       access
> 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)
> V4: - Mark rcar_pcie_abort_handler_of_match with __initconst
> V5: - Add mutex around rcar_pcie_aarch32_abort_handler()

[from my v5 comments] spinlock (used as a mutex ;-)

>     - Update commit message again to point out issues with L1/D3Hot states
> V6: - Return 1 only if condition cannot be fixed

Gr{oetje,eeting}s,

                        Geert

Bjorn Helgaas July 17, 2021, 5:33 p.m. UTC | #2

On Fri, May 14, 2021 at 10:05:49PM +0200, [e-mail] wrote:
> From: Marek Vasut <[e-mail]>
> 
> 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.
> 
> While it was suggested to disable L1 link state support completely on
> the controller level, this would not prevent the L1 link state entry
> initiated by the link partner. This happens e.g. in case a PCIe card
> enters D3Hot state, which could be initiated from pci_set_power_state()
> if the card indicates D3Hot support, which in turn means link must enter
> L1 state. So instead, fix up the L1 link state after all.
> 
> 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

This patch is horribly ugly but it's working around a horrible
hardware problem, and I don't have any better suggestions, so I guess
we don't really have much choice.

I do think the commit log is a bit glib:

  - "The R-Car PCIe controller is capable of handling L0s/L1 link
    states."  AFAICT every PCIe device is required to handle L0 and L1
    without software assistance.  So saying R-Car is "capable" puts a
    better face on this than seems warranted.

    L0s doesn't seem relevant at all; at least it doesn't seem to play
    a role in the patch.  There's no such thing as "returning to L0s"
    as mentioned in the comment below; L0s is only reachable from L0.
    Returns from L1 only go to L0 (PCIe r5.0, fig 5-1).

  - "The problem is, this transition is not atomic."  I think the
    *problem* is the hardware is broken in the first place.  This
    transition is supposed to be invisible to software.

  - "Just like other PCI controller drivers ..." suggests that this is
    an ordinary situation that we shouldn't be concerned about.  This
    patch may be the best we can do to work around a bad hardware
    defect, but it's definitely not ordinary.

    I think the other hook_fault_code() uses are for reporting
    legitimate PCIe errors, which most controllers log and turn
    into ~0 data responses without generating an abort or machine
    check, not things caused by hardware defects, so they're not
    really comparable.

Has Renesas documented this as an erratum?  Will future devices
require additions to rcar_pcie_abort_handler_of_match[]?

It'd be nice if the commit log mentioned the user-visible effect of
this problem.  I guess it does mention external aborts -- I assume you
see those when downstream devices go to D3hot or when ASPM puts the
link in L1?  And the abort results in a reboot?

To be clear, I'm not objecting to the patch.  It's a hardware problem
and we should work around it as best we can.

> Signed-off-by: Marek Vasut <[e-mail]>
> Cc: Bjorn Helgaas <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>
> Cc: Lorenzo Pieralisi <[e-mail]>
> Cc: Wolfram Sang <[e-mail]>
> Cc: Yoshihiro Shimoda <[e-mail]>
> Cc: [e-mail]
> ---
> V2: - Update commit message, add link to TFA repository commit
>     - Handle the LPAE case as in ARM fault.c and fsr-{2,3}level.c
>     - Cache clock and check whether they are enabled before register
>       access
> 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)
> V4: - Mark rcar_pcie_abort_handler_of_match with __initconst
> V5: - Add mutex around rcar_pcie_aarch32_abort_handler()
>     - Update commit message again to point out issues with L1/D3Hot states
> V6: - Return 1 only if condition cannot be fixed
> ---
>  drivers/pci/controller/pcie-rcar-host.c | 84 +++++++++++++++++++++++++
>  drivers/pci/controller/pcie-rcar.h      |  7 +++
>  2 files changed, 91 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
> index 765cf2b45e24..0d3f8dc5ff8a 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>
> @@ -41,6 +42,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
> +
>  /* Structure representing the PCIe interface */
>  struct rcar_pcie_host {
>  	struct rcar_pcie	pcie;
> @@ -776,6 +792,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:
> @@ -1031,4 +1053,66 @@ static struct platform_driver rcar_pcie_driver = {
>  	},
>  	.probe = rcar_pcie_probe,
>  };
> +
> +#ifdef CONFIG_ARM
> +static DEFINE_SPINLOCK(pmsr_lock);
> +static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
> +		unsigned int fsr, struct pt_regs *regs)
> +{
> +	unsigned long flags;
> +	int ret = 0;
> +	u32 pmsr;
> +
> +	spin_lock_irqsave(&pmsr_lock, flags);
> +
> +	if (!pcie_base || !__clk_is_enabled(pcie_bus_clk)) {
> +		ret = 1;
> +		goto unlock_exit;
> +	}
> +
> +	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);
> +	}
> +
> +unlock_exit:
> +	spin_unlock_irqrestore(&pmsr_lock, flags);
> +	return ret;
> +}
> +
> +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" },
> +	{},
> +};
> +
> +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)
> -- 
> 2.30.2
>

Marek Vasut July 17, 2021, 6:14 p.m. UTC | #3

On 7/17/21 7:33 PM, Bjorn Helgaas wrote:
[...]

>> 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
> 
> This patch is horribly ugly but it's working around a horrible
> hardware problem, and I don't have any better suggestions, so I guess
> we don't really have much choice.
> 
> I do think the commit log is a bit glib:

Should I reword the commit message one more time and send V7 ?

>    - "The R-Car PCIe controller is capable of handling L0s/L1 link
>      states."  AFAICT every PCIe device is required to handle L0 and L1
>      without software assistance.  So saying R-Car is "capable" puts a
>      better face on this than seems warranted.
> 
>      L0s doesn't seem relevant at all; at least it doesn't seem to play
>      a role in the patch.  There's no such thing as "returning to L0s"
>      as mentioned in the comment below; L0s is only reachable from L0.
>      Returns from L1 only go to L0 (PCIe r5.0, fig 5-1).
> 
>    - "The problem is, this transition is not atomic."  I think the
>      *problem* is the hardware is broken in the first place.  This
>      transition is supposed to be invisible to software.
> 
>    - "Just like other PCI controller drivers ..." suggests that this is
>      an ordinary situation that we shouldn't be concerned about.  This
>      patch may be the best we can do to work around a bad hardware
>      defect, but it's definitely not ordinary.
> 
>      I think the other hook_fault_code() uses are for reporting
>      legitimate PCIe errors, which most controllers log and turn
>      into ~0 data responses without generating an abort or machine
>      check, not things caused by hardware defects, so they're not
>      really comparable.
> 
> Has Renesas documented this as an erratum?

They are aware of this.

> Will future devices
> require additions to rcar_pcie_abort_handler_of_match[]?

No, this change is for legacy arm32 SoCs only.

> It'd be nice if the commit log mentioned the user-visible effect of
> this problem.  I guess it does mention external aborts -- I assume you
> see those when downstream devices go to D3hot or when ASPM puts the
> link in L1?  And the abort results in a reboot?

It results in a hang.

> To be clear, I'm not objecting to the patch.  It's a hardware problem
> and we should work around it as best we can.

[...]

Lorenzo Pieralisi July 19, 2021, 8:59 a.m. UTC | #4

[+Pali]

On Sat, Jul 17, 2021 at 12:33:34PM -0500, Bjorn Helgaas wrote:
> On Fri, May 14, 2021 at 10:05:49PM +0200, [e-mail] wrote:
> > From: Marek Vasut <[e-mail]>
> > 
> > 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.
> > 
> > While it was suggested to disable L1 link state support completely on
> > the controller level, this would not prevent the L1 link state entry
> > initiated by the link partner. This happens e.g. in case a PCIe card
> > enters D3Hot state, which could be initiated from pci_set_power_state()
> > if the card indicates D3Hot support, which in turn means link must enter
> > L1 state. So instead, fix up the L1 link state after all.
> > 
> > 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
> 
> This patch is horribly ugly but it's working around a horrible
> hardware problem, and I don't have any better suggestions, so I guess
> we don't really have much choice.

Pali is doing some work on the matter (in particular [1] above) and I
was following that up to see if there was any outcome before merging
this code, I could not follow up myself for lack of time.

Lorenzo

> I do think the commit log is a bit glib:
> 
>   - "The R-Car PCIe controller is capable of handling L0s/L1 link
>     states."  AFAICT every PCIe device is required to handle L0 and L1
>     without software assistance.  So saying R-Car is "capable" puts a
>     better face on this than seems warranted.
> 
>     L0s doesn't seem relevant at all; at least it doesn't seem to play
>     a role in the patch.  There's no such thing as "returning to L0s"
>     as mentioned in the comment below; L0s is only reachable from L0.
>     Returns from L1 only go to L0 (PCIe r5.0, fig 5-1).
> 
>   - "The problem is, this transition is not atomic."  I think the
>     *problem* is the hardware is broken in the first place.  This
>     transition is supposed to be invisible to software.
> 
>   - "Just like other PCI controller drivers ..." suggests that this is
>     an ordinary situation that we shouldn't be concerned about.  This
>     patch may be the best we can do to work around a bad hardware
>     defect, but it's definitely not ordinary.
> 
>     I think the other hook_fault_code() uses are for reporting
>     legitimate PCIe errors, which most controllers log and turn
>     into ~0 data responses without generating an abort or machine
>     check, not things caused by hardware defects, so they're not
>     really comparable.
> 
> Has Renesas documented this as an erratum?  Will future devices
> require additions to rcar_pcie_abort_handler_of_match[]?
> 
> It'd be nice if the commit log mentioned the user-visible effect of
> this problem.  I guess it does mention external aborts -- I assume you
> see those when downstream devices go to D3hot or when ASPM puts the
> link in L1?  And the abort results in a reboot?
> 
> To be clear, I'm not objecting to the patch.  It's a hardware problem
> and we should work around it as best we can.
> 
> > Signed-off-by: Marek Vasut <[e-mail]>
> > Cc: Bjorn Helgaas <[e-mail]>
> > Cc: Geert Uytterhoeven <[e-mail]>
> > Cc: Lorenzo Pieralisi <[e-mail]>
> > Cc: Wolfram Sang <[e-mail]>
> > Cc: Yoshihiro Shimoda <[e-mail]>
> > Cc: [e-mail]
> > ---
> > V2: - Update commit message, add link to TFA repository commit
> >     - Handle the LPAE case as in ARM fault.c and fsr-{2,3}level.c
> >     - Cache clock and check whether they are enabled before register
> >       access
> > 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)
> > V4: - Mark rcar_pcie_abort_handler_of_match with __initconst
> > V5: - Add mutex around rcar_pcie_aarch32_abort_handler()
> >     - Update commit message again to point out issues with L1/D3Hot states
> > V6: - Return 1 only if condition cannot be fixed
> > ---
> >  drivers/pci/controller/pcie-rcar-host.c | 84 +++++++++++++++++++++++++
> >  drivers/pci/controller/pcie-rcar.h      |  7 +++
> >  2 files changed, 91 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
> > index 765cf2b45e24..0d3f8dc5ff8a 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>
> > @@ -41,6 +42,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
> > +
> >  /* Structure representing the PCIe interface */
> >  struct rcar_pcie_host {
> >  	struct rcar_pcie	pcie;
> > @@ -776,6 +792,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:
> > @@ -1031,4 +1053,66 @@ static struct platform_driver rcar_pcie_driver = {
> >  	},
> >  	.probe = rcar_pcie_probe,
> >  };
> > +
> > +#ifdef CONFIG_ARM
> > +static DEFINE_SPINLOCK(pmsr_lock);
> > +static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
> > +		unsigned int fsr, struct pt_regs *regs)
> > +{
> > +	unsigned long flags;
> > +	int ret = 0;
> > +	u32 pmsr;
> > +
> > +	spin_lock_irqsave(&pmsr_lock, flags);
> > +
> > +	if (!pcie_base || !__clk_is_enabled(pcie_bus_clk)) {
> > +		ret = 1;
> > +		goto unlock_exit;
> > +	}
> > +
> > +	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);
> > +	}
> > +
> > +unlock_exit:
> > +	spin_unlock_irqrestore(&pmsr_lock, flags);
> > +	return ret;
> > +}
> > +
> > +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" },
> > +	{},
> > +};
> > +
> > +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)
> > -- 
> > 2.30.2
> >

Marek Vasut July 19, 2021, 3:38 p.m. UTC | #5

On 7/19/21 10:59 AM, Lorenzo Pieralisi wrote:
> [+Pali]
> 
> On Sat, Jul 17, 2021 at 12:33:34PM -0500, Bjorn Helgaas wrote:
>> On Fri, May 14, 2021 at 10:05:49PM +0200, [e-mail] wrote:
>>> From: Marek Vasut <[e-mail]>
>>>
>>> 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.
>>>
>>> While it was suggested to disable L1 link state support completely on
>>> the controller level, this would not prevent the L1 link state entry
>>> initiated by the link partner. This happens e.g. in case a PCIe card
>>> enters D3Hot state, which could be initiated from pci_set_power_state()
>>> if the card indicates D3Hot support, which in turn means link must enter
>>> L1 state. So instead, fix up the L1 link state after all.
>>>
>>> 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
>>
>> This patch is horribly ugly but it's working around a horrible
>> hardware problem, and I don't have any better suggestions, so I guess
>> we don't really have much choice.
> 
> Pali is doing some work on the matter (in particular [1] above) and I
> was following that up to see if there was any outcome before merging
> this code, I could not follow up myself for lack of time.

arm32 r-car does NOT use atf, so this does not apply here.

Pali Rohár July 19, 2021, 5:23 p.m. UTC | #6

On Monday 19 July 2021 09:59:53 Lorenzo Pieralisi wrote:
> [+Pali]
> 
> On Sat, Jul 17, 2021 at 12:33:34PM -0500, Bjorn Helgaas wrote:
> > On Fri, May 14, 2021 at 10:05:49PM +0200, [e-mail] wrote:
> > > From: Marek Vasut <[e-mail]>
> > > 
> > > 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.

And if PCIe MMIO access does not happen, what fixes this issue? In this
patch is implemented only arm32 external abort hook handler (which is
called only when PCIe MMIO access happens and aborts).

> > > 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.

Link cannot directly goes to L0 from L1. It first goes to Recovery state
and in this state card can "disconnect" or reset...

What would happen if PCIe MMIO access is issued when link is not in some
L* state? (This can be manually triggered by PCIe Hot Reset - toggling
Secondary Bus Reset bit in Bridge Control register on parent PCIe Bridge
device) Is R-Car working in this case and does not crash?

> > > While it was suggested to disable L1 link state support completely on
> > > the controller level, this would not prevent the L1 link state entry
> > > initiated by the link partner. This happens e.g. in case a PCIe card
> > > enters D3Hot state, which could be initiated from pci_set_power_state()
> > > if the card indicates D3Hot support, which in turn means link must enter
> > > L1 state. So instead, fix up the L1 link state after all.
> > > 
> > > 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
> > 
> > This patch is horribly ugly but it's working around a horrible
> > hardware problem, and I don't have any better suggestions, so I guess
> > we don't really have much choice.
> 
> Pali is doing some work on the matter (in particular [1] above) and I
> was following that up to see if there was any outcome before merging
> this code, I could not follow up myself for lack of time.

Yes, I'm in process to remove similar hack / hook for pci aardvark
controller space. Kernel now has better fix for aardvark issue
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f18139966d072dab8e4398c95ce955a9742e04f7
so TF-A part of this hack could be disabled. But this is nothing for
R-Car. The only change which is touching TF-A code outside of pci
aardvark space is de-duplication of external abort handler code.

> Lorenzo
> 
> > I do think the commit log is a bit glib:
> > 
> >   - "The R-Car PCIe controller is capable of handling L0s/L1 link
> >     states."  AFAICT every PCIe device is required to handle L0 and L1
> >     without software assistance.  So saying R-Car is "capable" puts a
> >     better face on this than seems warranted.
> > 
> >     L0s doesn't seem relevant at all; at least it doesn't seem to play
> >     a role in the patch.  There's no such thing as "returning to L0s"
> >     as mentioned in the comment below; L0s is only reachable from L0.
> >     Returns from L1 only go to L0 (PCIe r5.0, fig 5-1).

IIRC from L1 you can only go to Recovery. And from L0s you go to L0 or
Recovery. But I do not know what is or was changed in PCIe r5.0.

> > 
> >   - "The problem is, this transition is not atomic."  I think the
> >     *problem* is the hardware is broken in the first place.  This
> >     transition is supposed to be invisible to software.
> > 
> >   - "Just like other PCI controller drivers ..." suggests that this is
> >     an ordinary situation that we shouldn't be concerned about.  This
> >     patch may be the best we can do to work around a bad hardware
> >     defect, but it's definitely not ordinary.
> > 
> >     I think the other hook_fault_code() uses are for reporting
> >     legitimate PCIe errors, which most controllers log and turn
> >     into ~0 data responses without generating an abort or machine
> >     check, not things caused by hardware defects, so they're not
> >     really comparable.

Yes, other hooks translate read aborts to fabricated 0xFFFFFFFF
response. But this one is totally different "hack" for buggy hw.

> > Has Renesas documented this as an erratum?  Will future devices
> > require additions to rcar_pcie_abort_handler_of_match[]?
> > 
> > It'd be nice if the commit log mentioned the user-visible effect of
> > this problem.  I guess it does mention external aborts -- I assume you
> > see those when downstream devices go to D3hot or when ASPM puts the
> > link in L1?  And the abort results in a reboot?

Normally these abort handlers reset cpu at the end, right? But maybe it
can be configured...

> > To be clear, I'm not objecting to the patch.  It's a hardware problem
> > and we should work around it as best we can.

I'm not sure if current API of hook_fault_code or rather whole usage of
it is prepared to expand into more and more drivers. Last time I looked
at this arm32 part, it was possible to register only one callback from
driver. So extending usage of this hook API can result that two drivers
start fighting who register it earlier...

> > > Signed-off-by: Marek Vasut <[e-mail]>
> > > Cc: Bjorn Helgaas <[e-mail]>
> > > Cc: Geert Uytterhoeven <[e-mail]>
> > > Cc: Lorenzo Pieralisi <[e-mail]>
> > > Cc: Wolfram Sang <[e-mail]>
> > > Cc: Yoshihiro Shimoda <[e-mail]>
> > > Cc: [e-mail]
> > > ---
> > > V2: - Update commit message, add link to TFA repository commit
> > >     - Handle the LPAE case as in ARM fault.c and fsr-{2,3}level.c
> > >     - Cache clock and check whether they are enabled before register
> > >       access
> > > 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)
> > > V4: - Mark rcar_pcie_abort_handler_of_match with __initconst
> > > V5: - Add mutex around rcar_pcie_aarch32_abort_handler()
> > >     - Update commit message again to point out issues with L1/D3Hot states
> > > V6: - Return 1 only if condition cannot be fixed
> > > ---
> > >  drivers/pci/controller/pcie-rcar-host.c | 84 +++++++++++++++++++++++++
> > >  drivers/pci/controller/pcie-rcar.h      |  7 +++
> > >  2 files changed, 91 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
> > > index 765cf2b45e24..0d3f8dc5ff8a 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>
> > > @@ -41,6 +42,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
> > > +
> > >  /* Structure representing the PCIe interface */
> > >  struct rcar_pcie_host {
> > >  	struct rcar_pcie	pcie;
> > > @@ -776,6 +792,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

Usage of global variables with address space is ugly and horrible too,
but current hook_fault_code() does not provide nothing better.

> > > +
> > >  	return 0;
> > >  
> > >  err_irq2:
> > > @@ -1031,4 +1053,66 @@ static struct platform_driver rcar_pcie_driver = {
> > >  	},
> > >  	.probe = rcar_pcie_probe,
> > >  };
> > > +
> > > +#ifdef CONFIG_ARM
> > > +static DEFINE_SPINLOCK(pmsr_lock);
> > > +static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
> > > +		unsigned int fsr, struct pt_regs *regs)
> > > +{
> > > +	unsigned long flags;
> > > +	int ret = 0;
> > > +	u32 pmsr;
> > > +
> > > +	spin_lock_irqsave(&pmsr_lock, flags);
> > > +
> > > +	if (!pcie_base || !__clk_is_enabled(pcie_bus_clk)) {
> > > +		ret = 1;
> > > +		goto unlock_exit;
> > > +	}
> > > +
> > > +	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))
> > > +			;

Infinite loop in abort handler is not a good idea. If this software
workaround is not able to fix HW in broken state then it is better to
let kernel finish abort handler and reboot machine (or whatever is
default action for particular abort handler).

> > > +		writel(L1FAEG | PMEL1RX, pcie_base + PMSR);
> > > +	}
> > > +
> > > +unlock_exit:
> > > +	spin_unlock_irqrestore(&pmsr_lock, flags);
> > > +	return ret;
> > > +}
> > > +
> > > +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" },
> > > +	{},
> > > +};
> > > +
> > > +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)
> > > -- 
> > > 2.30.2
> > >

Marek Vasut July 19, 2021, 6:39 p.m. UTC | #7

On 7/19/21 7:23 PM, Pali Rohár wrote:

[...]

>>>> 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.
> 
> And if PCIe MMIO access does not happen, what fixes this issue?

Then you have no problem because you don't hit this fault.

> In this
> patch is implemented only arm32 external abort hook handler (which is
> called only when PCIe MMIO access happens and aborts).

Yes, for the aarch64 rcar the same fix is implemented in atf (see below).

>>>> 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.
> 
> Link cannot directly goes to L0 from L1. It first goes to Recovery state
> and in this state card can "disconnect" or reset...
> 
> What would happen if PCIe MMIO access is issued when link is not in some
> L* state? (This can be manually triggered by PCIe Hot Reset - toggling
> Secondary Bus Reset bit in Bridge Control register on parent PCIe Bridge
> device) Is R-Car working in this case and does not crash?

This seems to be exactly the situation the commit message describes -- 
the controller is stuck between L states and needs manual register write 
to proceed.

[...]

>>> To be clear, I'm not objecting to the patch.  It's a hardware problem
>>> and we should work around it as best we can.
> 
> I'm not sure if current API of hook_fault_code or rather whole usage of
> it is prepared to expand into more and more drivers. Last time I looked
> at this arm32 part, it was possible to register only one callback from
> driver. So extending usage of this hook API can result that two drivers
> start fighting who register it earlier...

There doesn't seem to be much ongoing HW development on the arm32 r-car, 
so I don't expect this list of hooks to grow much on this platform.

[...]

Bjorn Helgaas July 19, 2021, 10:06 p.m. UTC | #8

On Mon, Jul 19, 2021 at 07:23:40PM +0200, Pali Rohár wrote:
> > On Sat, Jul 17, 2021 at 12:33:34PM -0500, Bjorn Helgaas wrote:

> > >   - "The R-Car PCIe controller is capable of handling L0s/L1 link
> > >     states."  AFAICT every PCIe device is required to handle L0 and L1
> > >     without software assistance.  So saying R-Car is "capable" puts a
> > >     better face on this than seems warranted.
> > > 
> > >     L0s doesn't seem relevant at all; at least it doesn't seem to play
> > >     a role in the patch.  There's no such thing as "returning to L0s"
> > >     as mentioned in the comment below; L0s is only reachable from L0.
> > >     Returns from L1 only go to L0 (PCIe r5.0, fig 5-1).
> 
> IIRC from L1 you can only go to Recovery. And from L0s you go to L0 or
> Recovery. But I do not know what is or was changed in PCIe r5.0.

Yes.  Per PCIe r5.0, fig 4-26, the LTSSM state diagram, that's still
the same.  The overview in Fig 5-1 doesn't include Recovery, Detect,
and othef LTSSM states.

Pali Rohár July 22, 2021, 8:31 p.m. UTC | #9

On Monday 19 July 2021 20:39:13 Marek Vasut wrote:
> On 7/19/21 7:23 PM, Pali Rohár wrote:
> 
> [...]
> 
> > > > > 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.
> > 
> > And if PCIe MMIO access does not happen, what fixes this issue?
> 
> Then you have no problem because you don't hit this fault.

When controller stucks in some "unknown" state you have a problem. And
it does not matter if you are doing MMIO or not. If controller is in
Recovery or Configuration state then endpoint card cannot send neither
interrupt nor memory read / write messages to system.

Driver for endpoint card does not have to do active polling to check if
something in endpoint card happened. It can just wait for interrupt and
then do some stuff (which is IIRC preferred design if events are not too
frequent). And in this case card is in dead state and you have this
problem, right?

> > In this
> > patch is implemented only arm32 external abort hook handler (which is
> > called only when PCIe MMIO access happens and aborts).
> 
> Yes, for the aarch64 rcar the same fix is implemented in atf (see below).
> 
> > > > > 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.
> > 
> > Link cannot directly goes to L0 from L1. It first goes to Recovery state
> > and in this state card can "disconnect" or reset...
> > 
> > What would happen if PCIe MMIO access is issued when link is not in some
> > L* state? (This can be manually triggered by PCIe Hot Reset - toggling
> > Secondary Bus Reset bit in Bridge Control register on parent PCIe Bridge
> > device) Is R-Car working in this case and does not crash?
> 
> This seems to be exactly the situation the commit message describes -- the
> controller is stuck between L states and needs manual register write to
> proceed.

No, I asked what happen when is *not* in L state. Commit message does
not describe it.

So what happen if you try to do MMIO e.g. during Hot Reset state? (This
state can be easily "forced", so easy to test) Does it crash too (and
therefore needs some other "hack")? Or it is working fine without any
crash? Read operation in most cases returns all-ones and write just do
nothing (as write has no response).

> [...]
> 
> > > > To be clear, I'm not objecting to the patch.  It's a hardware problem
> > > > and we should work around it as best we can.
> > 
> > I'm not sure if current API of hook_fault_code or rather whole usage of
> > it is prepared to expand into more and more drivers. Last time I looked
> > at this arm32 part, it was possible to register only one callback from
> > driver. So extending usage of this hook API can result that two drivers
> > start fighting who register it earlier...
> 
> There doesn't seem to be much ongoing HW development on the arm32 r-car, so
> I don't expect this list of hooks to grow much on this platform.

For R-Car it then fine. This was just general comment as arm32 is still
actively developed platform.

Geert Uytterhoeven July 26, 2021, 2:47 p.m. UTC | #10

Hi Bjorn,

On Sat, Jul 17, 2021 at 7:33 PM Bjorn Helgaas <[e-mail]> wrote:
> On Fri, May 14, 2021 at 10:05:49PM +0200, [e-mail] wrote:
> > From: Marek Vasut <[e-mail]>
> >
> > 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.
> >
> > While it was suggested to disable L1 link state support completely on
> > the controller level, this would not prevent the L1 link state entry
> > initiated by the link partner. This happens e.g. in case a PCIe card
> > enters D3Hot state, which could be initiated from pci_set_power_state()
> > if the card indicates D3Hot support, which in turn means link must enter
> > L1 state. So instead, fix up the L1 link state after all.
> >
> > 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
>
> This patch is horribly ugly but it's working around a horrible
> hardware problem, and I don't have any better suggestions, so I guess
> we don't really have much choice.
>
> I do think the commit log is a bit glib:
>
>   - "The R-Car PCIe controller is capable of handling L0s/L1 link
>     states."  AFAICT every PCIe device is required to handle L0 and L1
>     without software assistance.  So saying R-Car is "capable" puts a
>     better face on this than seems warranted.
>
>     L0s doesn't seem relevant at all; at least it doesn't seem to play
>     a role in the patch.  There's no such thing as "returning to L0s"
>     as mentioned in the comment below; L0s is only reachable from L0.
>     Returns from L1 only go to L0 (PCIe r5.0, fig 5-1).
>
>   - "The problem is, this transition is not atomic."  I think the
>     *problem* is the hardware is broken in the first place.  This
>     transition is supposed to be invisible to software.
>
>   - "Just like other PCI controller drivers ..." suggests that this is
>     an ordinary situation that we shouldn't be concerned about.  This
>     patch may be the best we can do to work around a bad hardware
>     defect, but it's definitely not ordinary.
>
>     I think the other hook_fault_code() uses are for reporting
>     legitimate PCIe errors, which most controllers log and turn
>     into ~0 data responses without generating an abort or machine
>     check, not things caused by hardware defects, so they're not
>     really comparable.
>
> Has Renesas documented this as an erratum?  Will future devices
> require additions to rcar_pcie_abort_handler_of_match[]?
>
> It'd be nice if the commit log mentioned the user-visible effect of
> this problem.  I guess it does mention external aborts -- I assume you
> see those when downstream devices go to D3hot or when ASPM puts the
> link in L1?  And the abort results in a reboot?
>
> To be clear, I'm not objecting to the patch.  It's a hardware problem
> and we should work around it as best we can.

Cool! So what's missing for this patch, which we have been polishing
for almost one year, to be applied, so innocent people can no longer
lock up an R-Car system just by inserting an ubiquitous Intel Ethernet
card, and suspending the system?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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

Bjorn Helgaas July 26, 2021, 5:49 p.m. UTC | #11

On Mon, Jul 26, 2021 at 04:47:54PM +0200, Geert Uytterhoeven wrote:
> Hi Bjorn,
> 
> On Sat, Jul 17, 2021 at 7:33 PM Bjorn Helgaas <[e-mail]> wrote:
> > On Fri, May 14, 2021 at 10:05:49PM +0200, [e-mail] wrote:
> > > From: Marek Vasut <[e-mail]>
> > >
> > > 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.
> > >
> > > While it was suggested to disable L1 link state support completely on
> > > the controller level, this would not prevent the L1 link state entry
> > > initiated by the link partner. This happens e.g. in case a PCIe card
> > > enters D3Hot state, which could be initiated from pci_set_power_state()
> > > if the card indicates D3Hot support, which in turn means link must enter
> > > L1 state. So instead, fix up the L1 link state after all.
> > >
> > > 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
> >
> > This patch is horribly ugly but it's working around a horrible
> > hardware problem, and I don't have any better suggestions, so I guess
> > we don't really have much choice.
> >
> > I do think the commit log is a bit glib:
> >
> >   - "The R-Car PCIe controller is capable of handling L0s/L1 link
> >     states."  AFAICT every PCIe device is required to handle L0 and L1
> >     without software assistance.  So saying R-Car is "capable" puts a
> >     better face on this than seems warranted.
> >
> >     L0s doesn't seem relevant at all; at least it doesn't seem to play
> >     a role in the patch.  There's no such thing as "returning to L0s"
> >     as mentioned in the comment below; L0s is only reachable from L0.
> >     Returns from L1 only go to L0 (PCIe r5.0, fig 5-1).
> >
> >   - "The problem is, this transition is not atomic."  I think the
> >     *problem* is the hardware is broken in the first place.  This
> >     transition is supposed to be invisible to software.
> >
> >   - "Just like other PCI controller drivers ..." suggests that this is
> >     an ordinary situation that we shouldn't be concerned about.  This
> >     patch may be the best we can do to work around a bad hardware
> >     defect, but it's definitely not ordinary.
> >
> >     I think the other hook_fault_code() uses are for reporting
> >     legitimate PCIe errors, which most controllers log and turn
> >     into ~0 data responses without generating an abort or machine
> >     check, not things caused by hardware defects, so they're not
> >     really comparable.
> >
> > Has Renesas documented this as an erratum?  Will future devices
> > require additions to rcar_pcie_abort_handler_of_match[]?
> >
> > It'd be nice if the commit log mentioned the user-visible effect of
> > this problem.  I guess it does mention external aborts -- I assume you
> > see those when downstream devices go to D3hot or when ASPM puts the
> > link in L1?  And the abort results in a reboot?
> >
> > To be clear, I'm not objecting to the patch.  It's a hardware problem
> > and we should work around it as best we can.
> 
> Cool! So what's missing for this patch, which we have been polishing
> for almost one year, to be applied, so innocent people can no longer
> lock up an R-Car system just by inserting an ubiquitous Intel Ethernet
> card, and suspending the system?

Nothing missing from my point of view, so if Lorenzo is OK with it,
he'll apply it.  If I were applying it, I would make the commit log
something like this:

  When the link is in L1, hardware should return it to L0
  automatically whenever a transaction targets a component on the
  other end of the link (PCIe r5.0, sec 5.2).

  The R-Car PCIe controller doesn't handle this transition correctly.
  If the link is not in L0, an MMIO transaction targeting a downstream
  device fails, and the controller reports an ARM imprecise external
  abort.

  Work around this by hooking the abort handler so the driver can
  detect this situation and help the hardware complete the link state
  transition.

  When the R-Car controller receives a PM_ENTER_L1 DLLP from the
  downstream component, it sets PMEL1RX bit in PMSR register, but then
  the controller enters some sort of in-between state.  A subsequent
  MMIO transaction will fail, resulting in the external abort.  The
  abort handler detects this condition and completes the link state
  transition by setting the L1IATN bit in PMCTLR and waiting for the
  link state transition to complete.

I assume that on the PCIe side, there must be an error like
Unsupported Request or Malformed TLP, and the R-Car controller is
logging that and turning it into the ARM external abort?

I didn't see a clear response to Pali's question about what happens if
there's no MMIO access, e.g., what if the downstream device initiates
a DMA or MSI transaction?

Bjorn

Lorenzo Pieralisi July 27, 2021, 4:11 p.m. UTC | #12

On Mon, Jul 19, 2021 at 07:23:40PM +0200, Pali Rohár wrote:

[...]

> > > > +#ifdef CONFIG_ARM
> > > > +static DEFINE_SPINLOCK(pmsr_lock);
> > > > +static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
> > > > +		unsigned int fsr, struct pt_regs *regs)
> > > > +{
> > > > +	unsigned long flags;
> > > > +	int ret = 0;
> > > > +	u32 pmsr;
> > > > +
> > > > +	spin_lock_irqsave(&pmsr_lock, flags);
> > > > +
> > > > +	if (!pcie_base || !__clk_is_enabled(pcie_bus_clk)) {
> > > > +		ret = 1;
> > > > +		goto unlock_exit;
> > > > +	}
> > > > +
> > > > +	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))
> > > > +			;
> 
> Infinite loop in abort handler is not a good idea. If this software
> workaround is not able to fix HW in broken state then it is better to
> let kernel finish abort handler and reboot machine (or whatever is
> default action for particular abort handler).

Probably worth adding a timeout, I can do it before merging it.

Lorenzo

> > > > +		writel(L1FAEG | PMEL1RX, pcie_base + PMSR);
> > > > +	}
> > > > +
> > > > +unlock_exit:
> > > > +	spin_unlock_irqrestore(&pmsr_lock, flags);
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +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" },
> > > > +	{},
> > > > +};
> > > > +
> > > > +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)
> > > > -- 
> > > > 2.30.2
> > > >

Geert Uytterhoeven July 27, 2021, 4:16 p.m. UTC | #13

Hi Lorenzo,

On Tue, Jul 27, 2021 at 6:11 PM Lorenzo Pieralisi
<[e-mail]> wrote:
> On Mon, Jul 19, 2021 at 07:23:40PM +0200, Pali Rohár wrote:
>
> [...]
>
> > > > > +#ifdef CONFIG_ARM
> > > > > +static DEFINE_SPINLOCK(pmsr_lock);
> > > > > +static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
> > > > > +               unsigned int fsr, struct pt_regs *regs)
> > > > > +{
> > > > > +       unsigned long flags;
> > > > > +       int ret = 0;
> > > > > +       u32 pmsr;
> > > > > +
> > > > > +       spin_lock_irqsave(&pmsr_lock, flags);
> > > > > +
> > > > > +       if (!pcie_base || !__clk_is_enabled(pcie_bus_clk)) {
> > > > > +               ret = 1;
> > > > > +               goto unlock_exit;
> > > > > +       }
> > > > > +
> > > > > +       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))
> > > > > +                       ;
> >
> > Infinite loop in abort handler is not a good idea. If this software
> > workaround is not able to fix HW in broken state then it is better to
> > let kernel finish abort handler and reboot machine (or whatever is
> > default action for particular abort handler).

The default action is to crash with an imprecise external abort.

> Probably worth adding a timeout, I can do it before merging it.

Indeed, better to lock up with a message than without ;-)

Gr{oetje,eeting}s,

                        Geert

Lorenzo Pieralisi July 27, 2021, 4:32 p.m. UTC | #14

On Mon, Jul 26, 2021 at 12:49:25PM -0500, Bjorn Helgaas wrote:
> On Mon, Jul 26, 2021 at 04:47:54PM +0200, Geert Uytterhoeven wrote:
> > Hi Bjorn,
> > 
> > On Sat, Jul 17, 2021 at 7:33 PM Bjorn Helgaas <[e-mail]> wrote:
> > > On Fri, May 14, 2021 at 10:05:49PM +0200, [e-mail] wrote:
> > > > From: Marek Vasut <[e-mail]>
> > > >
> > > > 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.
> > > >
> > > > While it was suggested to disable L1 link state support completely on
> > > > the controller level, this would not prevent the L1 link state entry
> > > > initiated by the link partner. This happens e.g. in case a PCIe card
> > > > enters D3Hot state, which could be initiated from pci_set_power_state()
> > > > if the card indicates D3Hot support, which in turn means link must enter
> > > > L1 state. So instead, fix up the L1 link state after all.
> > > >
> > > > 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
> > >
> > > This patch is horribly ugly but it's working around a horrible
> > > hardware problem, and I don't have any better suggestions, so I guess
> > > we don't really have much choice.
> > >
> > > I do think the commit log is a bit glib:
> > >
> > >   - "The R-Car PCIe controller is capable of handling L0s/L1 link
> > >     states."  AFAICT every PCIe device is required to handle L0 and L1
> > >     without software assistance.  So saying R-Car is "capable" puts a
> > >     better face on this than seems warranted.
> > >
> > >     L0s doesn't seem relevant at all; at least it doesn't seem to play
> > >     a role in the patch.  There's no such thing as "returning to L0s"
> > >     as mentioned in the comment below; L0s is only reachable from L0.
> > >     Returns from L1 only go to L0 (PCIe r5.0, fig 5-1).
> > >
> > >   - "The problem is, this transition is not atomic."  I think the
> > >     *problem* is the hardware is broken in the first place.  This
> > >     transition is supposed to be invisible to software.
> > >
> > >   - "Just like other PCI controller drivers ..." suggests that this is
> > >     an ordinary situation that we shouldn't be concerned about.  This
> > >     patch may be the best we can do to work around a bad hardware
> > >     defect, but it's definitely not ordinary.
> > >
> > >     I think the other hook_fault_code() uses are for reporting
> > >     legitimate PCIe errors, which most controllers log and turn
> > >     into ~0 data responses without generating an abort or machine
> > >     check, not things caused by hardware defects, so they're not
> > >     really comparable.
> > >
> > > Has Renesas documented this as an erratum?  Will future devices
> > > require additions to rcar_pcie_abort_handler_of_match[]?
> > >
> > > It'd be nice if the commit log mentioned the user-visible effect of
> > > this problem.  I guess it does mention external aborts -- I assume you
> > > see those when downstream devices go to D3hot or when ASPM puts the
> > > link in L1?  And the abort results in a reboot?
> > >
> > > To be clear, I'm not objecting to the patch.  It's a hardware problem
> > > and we should work around it as best we can.
> > 
> > Cool! So what's missing for this patch, which we have been polishing
> > for almost one year, to be applied, so innocent people can no longer
> > lock up an R-Car system just by inserting an ubiquitous Intel Ethernet
> > card, and suspending the system?
> 
> Nothing missing from my point of view, so if Lorenzo is OK with it,
> he'll apply it.

I will apply it at some point for v5.15 - there is still some details I
would like to investigate (disclaimer: I am not picking on this
particular patch - it is just a really thorny issue and I want to
understand what's the best way forward); I will update the patch and log
accordingly, no need for a v7 (which I can post myself publicly so that
you can have a look before I merge it).

> If I were applying it, I would make the commit log
> something like this:

I will do it myself, see above.

>   When the link is in L1, hardware should return it to L0
>   automatically whenever a transaction targets a component on the
>   other end of the link (PCIe r5.0, sec 5.2).
> 
>   The R-Car PCIe controller doesn't handle this transition correctly.
>   If the link is not in L0, an MMIO transaction targeting a downstream
>   device fails, and the controller reports an ARM imprecise external
>   abort.
> 
>   Work around this by hooking the abort handler so the driver can
>   detect this situation and help the hardware complete the link state
>   transition.
> 
>   When the R-Car controller receives a PM_ENTER_L1 DLLP from the
>   downstream component, it sets PMEL1RX bit in PMSR register, but then
>   the controller enters some sort of in-between state.  A subsequent
>   MMIO transaction will fail, resulting in the external abort.  The
>   abort handler detects this condition and completes the link state
>   transition by setting the L1IATN bit in PMCTLR and waiting for the
>   link state transition to complete.
> 
> I assume that on the PCIe side, there must be an error like
> Unsupported Request or Malformed TLP, and the R-Car controller is
> logging that and turning it into the ARM external abort?
> 
> I didn't see a clear response to Pali's question about what happens if
> there's no MMIO access, e.g., what if the downstream device initiates
> a DMA or MSI transaction?

It'd be great if I could update the log with these questions answered -
along with others Pali asked [1] and that are very relevant.

Thanks,
Lorenzo

[1] https://lore.kernel.org/linux-pci/20210719172340.vvtnddbli2vgxndi@pali

Marek Vasut July 27, 2021, 5:08 p.m. UTC | #15

On 7/26/21 7:49 PM, Bjorn Helgaas wrote:
> On Mon, Jul 26, 2021 at 04:47:54PM +0200, Geert Uytterhoeven wrote:
[...]
>>>> 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.
>>>>
>>>> While it was suggested to disable L1 link state support completely on
>>>> the controller level, this would not prevent the L1 link state entry
>>>> initiated by the link partner. This happens e.g. in case a PCIe card
>>>> enters D3Hot state, which could be initiated from pci_set_power_state()
>>>> if the card indicates D3Hot support, which in turn means link must enter
>>>> L1 state. So instead, fix up the L1 link state after all.
>>>>
>>>> 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
>>>
>>> This patch is horribly ugly but it's working around a horrible
>>> hardware problem, and I don't have any better suggestions, so I guess
>>> we don't really have much choice.
>>>
>>> I do think the commit log is a bit glib:
>>>
>>>    - "The R-Car PCIe controller is capable of handling L0s/L1 link
>>>      states."  AFAICT every PCIe device is required to handle L0 and L1
>>>      without software assistance.  So saying R-Car is "capable" puts a
>>>      better face on this than seems warranted.
>>>
>>>      L0s doesn't seem relevant at all; at least it doesn't seem to play
>>>      a role in the patch.  There's no such thing as "returning to L0s"
>>>      as mentioned in the comment below; L0s is only reachable from L0.
>>>      Returns from L1 only go to L0 (PCIe r5.0, fig 5-1).
>>>
>>>    - "The problem is, this transition is not atomic."  I think the
>>>      *problem* is the hardware is broken in the first place.  This
>>>      transition is supposed to be invisible to software.
>>>
>>>    - "Just like other PCI controller drivers ..." suggests that this is
>>>      an ordinary situation that we shouldn't be concerned about.  This
>>>      patch may be the best we can do to work around a bad hardware
>>>      defect, but it's definitely not ordinary.
>>>
>>>      I think the other hook_fault_code() uses are for reporting
>>>      legitimate PCIe errors, which most controllers log and turn
>>>      into ~0 data responses without generating an abort or machine
>>>      check, not things caused by hardware defects, so they're not
>>>      really comparable.
>>>
>>> Has Renesas documented this as an erratum?  Will future devices
>>> require additions to rcar_pcie_abort_handler_of_match[]?
>>>
>>> It'd be nice if the commit log mentioned the user-visible effect of
>>> this problem.  I guess it does mention external aborts -- I assume you
>>> see those when downstream devices go to D3hot or when ASPM puts the
>>> link in L1?  And the abort results in a reboot?
>>>
>>> To be clear, I'm not objecting to the patch.  It's a hardware problem
>>> and we should work around it as best we can.
>>
>> Cool! So what's missing for this patch, which we have been polishing
>> for almost one year, to be applied, so innocent people can no longer
>> lock up an R-Car system just by inserting an ubiquitous Intel Ethernet
>> card, and suspending the system?
> 
> Nothing missing from my point of view, so if Lorenzo is OK with it,
> he'll apply it.  If I were applying it, I would make the commit log
> something like this:
> 
>    When the link is in L1, hardware should return it to L0
>    automatically whenever a transaction targets a component on the
>    other end of the link (PCIe r5.0, sec 5.2).
> 
>    The R-Car PCIe controller doesn't handle this transition correctly.
>    If the link is not in L0, an MMIO transaction targeting a downstream
>    device fails, and the controller reports an ARM imprecise external
>    abort.
> 
>    Work around this by hooking the abort handler so the driver can
>    detect this situation and help the hardware complete the link state
>    transition.
> 
>    When the R-Car controller receives a PM_ENTER_L1 DLLP from the
>    downstream component, it sets PMEL1RX bit in PMSR register, but then
>    the controller enters some sort of in-between state.  A subsequent
>    MMIO transaction will fail, resulting in the external abort.  The
>    abort handler detects this condition and completes the link state
>    transition by setting the L1IATN bit in PMCTLR and waiting for the
>    link state transition to complete.

OK, should I submit V7 and just copy-paste this commit message in, or 
wait for Lorenzo to provide clear direction ?

> I assume that on the PCIe side, there must be an error like
> Unsupported Request or Malformed TLP, and the R-Car controller is
> logging that and turning it into the ARM external abort?
> 
> I didn't see a clear response to Pali's question about what happens if
> there's no MMIO access, e.g., what if the downstream device initiates
> a DMA or MSI transaction?

If the link is in this state, the packet won't reach the root complex, 
so nothing happens. And I don't see a good way to fix that option.

Lorenzo Pieralisi Aug. 4, 2021, 11:06 a.m. UTC | #16

On Tue, Jul 27, 2021 at 07:08:17PM +0200, Marek Vasut wrote:
> On 7/26/21 7:49 PM, Bjorn Helgaas wrote:
> > On Mon, Jul 26, 2021 at 04:47:54PM +0200, Geert Uytterhoeven wrote:
> [...]
> > > > > 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.
> > > > > 
> > > > > While it was suggested to disable L1 link state support completely on
> > > > > the controller level, this would not prevent the L1 link state entry
> > > > > initiated by the link partner. This happens e.g. in case a PCIe card
> > > > > enters D3Hot state, which could be initiated from pci_set_power_state()
> > > > > if the card indicates D3Hot support, which in turn means link must enter
> > > > > L1 state. So instead, fix up the L1 link state after all.
> > > > > 
> > > > > 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
> > > > 
> > > > This patch is horribly ugly but it's working around a horrible
> > > > hardware problem, and I don't have any better suggestions, so I guess
> > > > we don't really have much choice.
> > > > 
> > > > I do think the commit log is a bit glib:
> > > > 
> > > >    - "The R-Car PCIe controller is capable of handling L0s/L1 link
> > > >      states."  AFAICT every PCIe device is required to handle L0 and L1
> > > >      without software assistance.  So saying R-Car is "capable" puts a
> > > >      better face on this than seems warranted.
> > > > 
> > > >      L0s doesn't seem relevant at all; at least it doesn't seem to play
> > > >      a role in the patch.  There's no such thing as "returning to L0s"
> > > >      as mentioned in the comment below; L0s is only reachable from L0.
> > > >      Returns from L1 only go to L0 (PCIe r5.0, fig 5-1).
> > > > 
> > > >    - "The problem is, this transition is not atomic."  I think the
> > > >      *problem* is the hardware is broken in the first place.  This
> > > >      transition is supposed to be invisible to software.
> > > > 
> > > >    - "Just like other PCI controller drivers ..." suggests that this is
> > > >      an ordinary situation that we shouldn't be concerned about.  This
> > > >      patch may be the best we can do to work around a bad hardware
> > > >      defect, but it's definitely not ordinary.
> > > > 
> > > >      I think the other hook_fault_code() uses are for reporting
> > > >      legitimate PCIe errors, which most controllers log and turn
> > > >      into ~0 data responses without generating an abort or machine
> > > >      check, not things caused by hardware defects, so they're not
> > > >      really comparable.
> > > > 
> > > > Has Renesas documented this as an erratum?  Will future devices
> > > > require additions to rcar_pcie_abort_handler_of_match[]?
> > > > 
> > > > It'd be nice if the commit log mentioned the user-visible effect of
> > > > this problem.  I guess it does mention external aborts -- I assume you
> > > > see those when downstream devices go to D3hot or when ASPM puts the
> > > > link in L1?  And the abort results in a reboot?
> > > > 
> > > > To be clear, I'm not objecting to the patch.  It's a hardware problem
> > > > and we should work around it as best we can.
> > > 
> > > Cool! So what's missing for this patch, which we have been polishing
> > > for almost one year, to be applied, so innocent people can no longer
> > > lock up an R-Car system just by inserting an ubiquitous Intel Ethernet
> > > card, and suspending the system?
> > 
> > Nothing missing from my point of view, so if Lorenzo is OK with it,
> > he'll apply it.  If I were applying it, I would make the commit log
> > something like this:
> > 
> >    When the link is in L1, hardware should return it to L0
> >    automatically whenever a transaction targets a component on the
> >    other end of the link (PCIe r5.0, sec 5.2).
> > 
> >    The R-Car PCIe controller doesn't handle this transition correctly.
> >    If the link is not in L0, an MMIO transaction targeting a downstream
> >    device fails, and the controller reports an ARM imprecise external
> >    abort.
> > 
> >    Work around this by hooking the abort handler so the driver can
> >    detect this situation and help the hardware complete the link state
> >    transition.
> > 
> >    When the R-Car controller receives a PM_ENTER_L1 DLLP from the
> >    downstream component, it sets PMEL1RX bit in PMSR register, but then
> >    the controller enters some sort of in-between state.  A subsequent
> >    MMIO transaction will fail, resulting in the external abort.  The
> >    abort handler detects this condition and completes the link state
> >    transition by setting the L1IATN bit in PMCTLR and waiting for the
> >    link state transition to complete.
> 
> OK, should I submit V7 and just copy-paste this commit message in, or wait
> for Lorenzo to provide clear direction ?

Hi Marek,

may I ask you please to update the log as Bjorn requested, add a timeout
in the trap handler to avoid an infinite loop on failure and repost a v7
?

I will merge it straight away sorry for all this back and forth.

Thanks,
Lorenzo

> > I assume that on the PCIe side, there must be an error like
> > Unsupported Request or Malformed TLP, and the R-Car controller is
> > logging that and turning it into the ARM external abort?
> > 
> > I didn't see a clear response to Pali's question about what happens if
> > there's no MMIO access, e.g., what if the downstream device initiates
> > a DMA or MSI transaction?
> 
> If the link is in this state, the packet won't reach the root complex, so
> nothing happens. And I don't see a good way to fix that option.

Pali Rohár Aug. 5, 2021, 6:30 p.m. UTC | #17

On Tuesday 27 July 2021 17:32:12 Lorenzo Pieralisi wrote:
> On Mon, Jul 26, 2021 at 12:49:25PM -0500, Bjorn Helgaas wrote:
> > On Mon, Jul 26, 2021 at 04:47:54PM +0200, Geert Uytterhoeven wrote:
> > > Hi Bjorn,
> > > 
> > > On Sat, Jul 17, 2021 at 7:33 PM Bjorn Helgaas <[e-mail]> wrote:
> > > > On Fri, May 14, 2021 at 10:05:49PM +0200, [e-mail] wrote:
> > > > > From: Marek Vasut <[e-mail]>
> > > > >
> > > > > 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.
> > > > >
> > > > > While it was suggested to disable L1 link state support completely on
> > > > > the controller level, this would not prevent the L1 link state entry
> > > > > initiated by the link partner. This happens e.g. in case a PCIe card
> > > > > enters D3Hot state, which could be initiated from pci_set_power_state()
> > > > > if the card indicates D3Hot support, which in turn means link must enter
> > > > > L1 state. So instead, fix up the L1 link state after all.
> > > > >
> > > > > 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
> > > >
> > > > This patch is horribly ugly but it's working around a horrible
> > > > hardware problem, and I don't have any better suggestions, so I guess
> > > > we don't really have much choice.
> > > >
> > > > I do think the commit log is a bit glib:
> > > >
> > > >   - "The R-Car PCIe controller is capable of handling L0s/L1 link
> > > >     states."  AFAICT every PCIe device is required to handle L0 and L1
> > > >     without software assistance.  So saying R-Car is "capable" puts a
> > > >     better face on this than seems warranted.
> > > >
> > > >     L0s doesn't seem relevant at all; at least it doesn't seem to play
> > > >     a role in the patch.  There's no such thing as "returning to L0s"
> > > >     as mentioned in the comment below; L0s is only reachable from L0.
> > > >     Returns from L1 only go to L0 (PCIe r5.0, fig 5-1).
> > > >
> > > >   - "The problem is, this transition is not atomic."  I think the
> > > >     *problem* is the hardware is broken in the first place.  This
> > > >     transition is supposed to be invisible to software.
> > > >
> > > >   - "Just like other PCI controller drivers ..." suggests that this is
> > > >     an ordinary situation that we shouldn't be concerned about.  This
> > > >     patch may be the best we can do to work around a bad hardware
> > > >     defect, but it's definitely not ordinary.
> > > >
> > > >     I think the other hook_fault_code() uses are for reporting
> > > >     legitimate PCIe errors, which most controllers log and turn
> > > >     into ~0 data responses without generating an abort or machine
> > > >     check, not things caused by hardware defects, so they're not
> > > >     really comparable.
> > > >
> > > > Has Renesas documented this as an erratum?  Will future devices
> > > > require additions to rcar_pcie_abort_handler_of_match[]?
> > > >
> > > > It'd be nice if the commit log mentioned the user-visible effect of
> > > > this problem.  I guess it does mention external aborts -- I assume you
> > > > see those when downstream devices go to D3hot or when ASPM puts the
> > > > link in L1?  And the abort results in a reboot?
> > > >
> > > > To be clear, I'm not objecting to the patch.  It's a hardware problem
> > > > and we should work around it as best we can.
> > > 
> > > Cool! So what's missing for this patch, which we have been polishing
> > > for almost one year, to be applied, so innocent people can no longer
> > > lock up an R-Car system just by inserting an ubiquitous Intel Ethernet
> > > card, and suspending the system?
> > 
> > Nothing missing from my point of view, so if Lorenzo is OK with it,
> > he'll apply it.
> 
> I will apply it at some point for v5.15 - there is still some details I
> would like to investigate (disclaimer: I am not picking on this
> particular patch - it is just a really thorny issue and I want to
> understand what's the best way forward); I will update the patch and log
> accordingly, no need for a v7 (which I can post myself publicly so that
> you can have a look before I merge it).
> 
> > If I were applying it, I would make the commit log
> > something like this:
> 
> I will do it myself, see above.
> 
> >   When the link is in L1, hardware should return it to L0
> >   automatically whenever a transaction targets a component on the
> >   other end of the link (PCIe r5.0, sec 5.2).
> > 
> >   The R-Car PCIe controller doesn't handle this transition correctly.
> >   If the link is not in L0, an MMIO transaction targeting a downstream
> >   device fails, and the controller reports an ARM imprecise external
> >   abort.
> > 
> >   Work around this by hooking the abort handler so the driver can
> >   detect this situation and help the hardware complete the link state
> >   transition.
> > 
> >   When the R-Car controller receives a PM_ENTER_L1 DLLP from the
> >   downstream component, it sets PMEL1RX bit in PMSR register, but then
> >   the controller enters some sort of in-between state.  A subsequent
> >   MMIO transaction will fail, resulting in the external abort.  The
> >   abort handler detects this condition and completes the link state
> >   transition by setting the L1IATN bit in PMCTLR and waiting for the
> >   link state transition to complete.
> > 
> > I assume that on the PCIe side, there must be an error like
> > Unsupported Request or Malformed TLP, and the R-Car controller is
> > logging that and turning it into the ARM external abort?
> > 
> > I didn't see a clear response to Pali's question about what happens if
> > there's no MMIO access, e.g., what if the downstream device initiates
> > a DMA or MSI transaction?
> 
> It'd be great if I could update the log with these questions answered -
> along with others Pali asked [1] and that are very relevant.
> 
> Thanks,
> Lorenzo
> 
> [1] https://lore.kernel.org/linux-pci/20210719172340.vvtnddbli2vgxndi@pali

And also there is still unanswered question what happens with MMIO
during non-L* states (e.g. Hot Reset, Detect, Polling)?
https://lore.kernel.org/linux-pci/20210722203151.heytxzup2uti4noi@pali/

Patch

diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
index 765cf2b45e24..0d3f8dc5ff8a 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>
@@ -41,6 +42,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
+
 /* Structure representing the PCIe interface */
 struct rcar_pcie_host {
 	struct rcar_pcie	pcie;
@@ -776,6 +792,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:
@@ -1031,4 +1053,66 @@  static struct platform_driver rcar_pcie_driver = {
 	},
 	.probe = rcar_pcie_probe,
 };
+
+#ifdef CONFIG_ARM
+static DEFINE_SPINLOCK(pmsr_lock);
+static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
+		unsigned int fsr, struct pt_regs *regs)
+{
+	unsigned long flags;
+	int ret = 0;
+	u32 pmsr;
+
+	spin_lock_irqsave(&pmsr_lock, flags);
+
+	if (!pcie_base || !__clk_is_enabled(pcie_bus_clk)) {
+		ret = 1;
+		goto unlock_exit;
+	}
+
+	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);
+	}
+
+unlock_exit:
+	spin_unlock_irqrestore(&pmsr_lock, flags);
+	return ret;
+}
+
+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" },
+	{},
+};
+
+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)