Hi Marek,
On Sun, Jan 16, 2022 at 3:26 AM <marek.vasut@xxxxxxxxx> wrote:
> From: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
>
> In case the controller is transitioning to L1 in rcar_pcie_config_access(),
> any read/write access to PCIECDR triggers asynchronous external abort. This
> is because the transition to L1 link state must be manually finished by the
> driver. The PCIe IP can transition back from L1 state to L0 on its own.
>
> Avoid triggering the abort in rcar_pcie_config_access() by checking whether
> the controller is in the transition state, and if so, finish the transition
> right away. This prevents a lot of unnecessary exceptions, although not all
> of them.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
Thanks for your patch!
> --- a/drivers/pci/controller/pcie-rcar-host.c
> +++ b/drivers/pci/controller/pcie-rcar-host.c
> @@ -54,6 +54,34 @@ static void __iomem *pcie_base;
> * device is runtime suspended or not.
> */
> static struct device *pcie_dev;
> +
> +static DEFINE_SPINLOCK(pmsr_lock);
> +static int rcar_pcie_wakeup(struct device *pcie_dev, void __iomem *pcie_base)
> +{
> + u32 pmsr, val;
> + int ret = 0;
> +
> + if (!pcie_base || pm_runtime_suspended(pcie_dev))
> + return 1;
This is not a suitable return code to be propagated in
rcar_pcie_config_access(). But probably this cannot happen anyway
when called from rcar_pcie_config_access()?
> +
> + 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);
> + ret = readl_poll_timeout_atomic(pcie_base + PMSR, val,
> + val & L1FAEG, 10, 1000);
> + WARN(ret, "Timeout waiting for L1 link state, ret=%d\n", ret);
> + writel(L1FAEG | PMEL1RX, pcie_base + PMSR);
> + }
> +
> + return ret;
> +}
> #endif
>
> /* Structure representing the PCIe interface */
> @@ -85,6 +113,15 @@ static int rcar_pcie_config_access(struct rcar_pcie_host *host,
> {
> struct rcar_pcie *pcie = &host->pcie;
> unsigned int dev, func, reg, index;
> + unsigned long flags;
> + int ret;
> +
> + /* Wake the bus up in case it is in L1 state. */
> + spin_lock_irqsave(&pmsr_lock, flags);
> + ret = rcar_pcie_wakeup(pcie->dev, pcie->base);
> + spin_unlock_irqrestore(&pmsr_lock, flags);
Move the spinlock handling in the caller?
> + if (ret)
> + return ret;
>
> dev = PCI_SLOT(devfn);
> func = PCI_FUNC(devfn);
> @@ -1050,36 +1087,18 @@ static struct platform_driver rcar_pcie_driver = {
> };
>
> #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;
> - u32 pmsr, val;
> int ret = 0;
>
> spin_lock_irqsave(&pmsr_lock, flags);
>
> - if (!pcie_base || pm_runtime_suspended(pcie_dev)) {
> - ret = 1;
> + ret = rcar_pcie_wakeup(pcie_dev, pcie_base);
> + spin_unlock_irqrestore(&pmsr_lock, flags);
Move the spinlock handling in the caller?
> + if (ret)
> 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);
> - ret = readl_poll_timeout_atomic(pcie_base + PMSR, val,
> - val & L1FAEG, 10, 1000);
> - WARN(ret, "Timeout waiting for L1 link state, ret=%d\n", ret);
> - writel(L1FAEG | PMEL1RX, pcie_base + PMSR);
> - }
>
> unlock_exit:
> spin_unlock_irqrestore(&pmsr_lock, flags);
Whoops, double unlock.
As there's nothing to be done below, the goto and label can be removed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
