Re: [PATCH 1/2] PCI: rcar: Finish transition to L1 state in rcar_pcie_config_access()

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Marek,

On Sun, Jan 16, 2022 at 3:26 AM <marek.vasut@gmail.com> wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>
> 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@gmail.com>

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@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds