diff mbox series

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

Message ID 20220116022549.456486-1-marek.vasut@gmail.com (mailing list archive)
State Superseded
Headers show
Series [1/2] PCI: rcar: Finish transition to L1 state in rcar_pcie_config_access() | expand

Commit Message

Marek Vasut Jan. 16, 2022, 2:25 a.m. UTC 
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>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Krzysztof Wilczyński <kw@linux.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/pci/controller/pcie-rcar-host.c | 61 ++++++++++++++++---------
 1 file changed, 40 insertions(+), 21 deletions(-)

Comments

Geert Uytterhoeven Jan. 16, 2022, 10:39 a.m. UTC | #1 
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
kernel test robot Jan. 16, 2022, 6:41 p.m. UTC | #2 
Hi,

I love your patch! Yet something to improve:

[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on next-20220116]
[cannot apply to v5.16]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/marek-vasut-gmail-com/PCI-rcar-Finish-transition-to-L1-state-in-rcar_pcie_config_access/20220116-102631
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: powerpc-randconfig-r004-20220116 (https://download.01.org/0day-ci/archive/20220117/202201170208.glCZ5BJW-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/dff324f279a78c9ca7f04c3fcf603b40d5a893fa
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review marek-vasut-gmail-com/PCI-rcar-Finish-transition-to-L1-state-in-rcar_pcie_config_access/20220116-102631
        git checkout dff324f279a78c9ca7f04c3fcf603b40d5a893fa
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/pci/controller/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/rwsem.h:15,
                    from include/linux/notifier.h:15,
                    from include/linux/clk.h:14,
                    from drivers/pci/controller/pcie-rcar-host.c:15:
   drivers/pci/controller/pcie-rcar-host.c: In function 'rcar_pcie_config_access':
>> drivers/pci/controller/pcie-rcar-host.c:120:28: error: 'pmsr_lock' undeclared (first use in this function); did you mean 'pmd_lock'?
     120 |         spin_lock_irqsave(&pmsr_lock, flags);
         |                            ^~~~~~~~~
   include/linux/spinlock.h:242:48: note: in definition of macro 'raw_spin_lock_irqsave'
     242 |                 flags = _raw_spin_lock_irqsave(lock);   \
         |                                                ^~~~
   drivers/pci/controller/pcie-rcar-host.c:120:9: note: in expansion of macro 'spin_lock_irqsave'
     120 |         spin_lock_irqsave(&pmsr_lock, flags);
         |         ^~~~~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rcar-host.c:120:28: note: each undeclared identifier is reported only once for each function it appears in
     120 |         spin_lock_irqsave(&pmsr_lock, flags);
         |                            ^~~~~~~~~
   include/linux/spinlock.h:242:48: note: in definition of macro 'raw_spin_lock_irqsave'
     242 |                 flags = _raw_spin_lock_irqsave(lock);   \
         |                                                ^~~~
   drivers/pci/controller/pcie-rcar-host.c:120:9: note: in expansion of macro 'spin_lock_irqsave'
     120 |         spin_lock_irqsave(&pmsr_lock, flags);
         |         ^~~~~~~~~~~~~~~~~
>> drivers/pci/controller/pcie-rcar-host.c:121:15: error: implicit declaration of function 'rcar_pcie_wakeup' [-Werror=implicit-function-declaration]
     121 |         ret = rcar_pcie_wakeup(pcie->dev, pcie->base);
         |               ^~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +120 drivers/pci/controller/pcie-rcar-host.c

  > 15	#include <linux/clk.h>
    16	#include <linux/clk-provider.h>
    17	#include <linux/delay.h>
    18	#include <linux/interrupt.h>
    19	#include <linux/irq.h>
    20	#include <linux/irqdomain.h>
    21	#include <linux/kernel.h>
    22	#include <linux/init.h>
    23	#include <linux/iopoll.h>
    24	#include <linux/msi.h>
    25	#include <linux/of_address.h>
    26	#include <linux/of_irq.h>
    27	#include <linux/of_platform.h>
    28	#include <linux/pci.h>
    29	#include <linux/phy/phy.h>
    30	#include <linux/platform_device.h>
    31	#include <linux/pm_runtime.h>
    32	
    33	#include "pcie-rcar.h"
    34	
    35	struct rcar_msi {
    36		DECLARE_BITMAP(used, INT_PCI_MSI_NR);
    37		struct irq_domain *domain;
    38		struct mutex map_lock;
    39		spinlock_t mask_lock;
    40		int irq1;
    41		int irq2;
    42	};
    43	
    44	#ifdef CONFIG_ARM
    45	/*
    46	 * Here we keep a static copy of the remapped PCIe controller address.
    47	 * This is only used on aarch32 systems, all of which have one single
    48	 * PCIe controller, to provide quick access to the PCIe controller in
    49	 * the L1 link state fixup function, called from the ARM fault handler.
    50	 */
    51	static void __iomem *pcie_base;
    52	/*
    53	 * Static copy of PCIe device pointer, so we can check whether the
    54	 * device is runtime suspended or not.
    55	 */
    56	static struct device *pcie_dev;
    57	
    58	static DEFINE_SPINLOCK(pmsr_lock);
    59	static int rcar_pcie_wakeup(struct device *pcie_dev, void __iomem *pcie_base)
    60	{
    61		u32 pmsr, val;
    62		int ret = 0;
    63	
    64		if (!pcie_base || pm_runtime_suspended(pcie_dev))
    65			return 1;
    66	
    67		pmsr = readl(pcie_base + PMSR);
    68	
    69		/*
    70		 * Test if the PCIe controller received PM_ENTER_L1 DLLP and
    71		 * the PCIe controller is not in L1 link state. If true, apply
    72		 * fix, which will put the controller into L1 link state, from
    73		 * which it can return to L0s/L0 on its own.
    74		 */
    75		if ((pmsr & PMEL1RX) && ((pmsr & PMSTATE) != PMSTATE_L1)) {
    76			writel(L1IATN, pcie_base + PMCTLR);
    77			ret = readl_poll_timeout_atomic(pcie_base + PMSR, val,
    78							val & L1FAEG, 10, 1000);
    79			WARN(ret, "Timeout waiting for L1 link state, ret=%d\n", ret);
    80			writel(L1FAEG | PMEL1RX, pcie_base + PMSR);
    81		}
    82	
    83		return ret;
    84	}
    85	#endif
    86	
    87	/* Structure representing the PCIe interface */
    88	struct rcar_pcie_host {
    89		struct rcar_pcie	pcie;
    90		struct phy		*phy;
    91		struct clk		*bus_clk;
    92		struct			rcar_msi msi;
    93		int			(*phy_init_fn)(struct rcar_pcie_host *host);
    94	};
    95	
    96	static struct rcar_pcie_host *msi_to_host(struct rcar_msi *msi)
    97	{
    98		return container_of(msi, struct rcar_pcie_host, msi);
    99	}
   100	
   101	static u32 rcar_read_conf(struct rcar_pcie *pcie, int where)
   102	{
   103		unsigned int shift = BITS_PER_BYTE * (where & 3);
   104		u32 val = rcar_pci_read_reg(pcie, where & ~3);
   105	
   106		return val >> shift;
   107	}
   108	
   109	/* Serialization is provided by 'pci_lock' in drivers/pci/access.c */
   110	static int rcar_pcie_config_access(struct rcar_pcie_host *host,
   111			unsigned char access_type, struct pci_bus *bus,
   112			unsigned int devfn, int where, u32 *data)
   113	{
   114		struct rcar_pcie *pcie = &host->pcie;
   115		unsigned int dev, func, reg, index;
   116		unsigned long flags;
   117		int ret;
   118	
   119		/* Wake the bus up in case it is in L1 state. */
 > 120		spin_lock_irqsave(&pmsr_lock, flags);
 > 121		ret = rcar_pcie_wakeup(pcie->dev, pcie->base);
   122		spin_unlock_irqrestore(&pmsr_lock, flags);
   123		if (ret)
   124			return ret;
   125	
   126		dev = PCI_SLOT(devfn);
   127		func = PCI_FUNC(devfn);
   128		reg = where & ~3;
   129		index = reg / 4;
   130	
   131		/*
   132		 * While each channel has its own memory-mapped extended config
   133		 * space, it's generally only accessible when in endpoint mode.
   134		 * When in root complex mode, the controller is unable to target
   135		 * itself with either type 0 or type 1 accesses, and indeed, any
   136		 * controller initiated target transfer to its own config space
   137		 * result in a completer abort.
   138		 *
   139		 * Each channel effectively only supports a single device, but as
   140		 * the same channel <-> device access works for any PCI_SLOT()
   141		 * value, we cheat a bit here and bind the controller's config
   142		 * space to devfn 0 in order to enable self-enumeration. In this
   143		 * case the regular ECAR/ECDR path is sidelined and the mangled
   144		 * config access itself is initiated as an internal bus transaction.
   145		 */
   146		if (pci_is_root_bus(bus)) {
   147			if (dev != 0)
   148				return PCIBIOS_DEVICE_NOT_FOUND;
   149	
   150			if (access_type == RCAR_PCI_ACCESS_READ)
   151				*data = rcar_pci_read_reg(pcie, PCICONF(index));
   152			else
   153				rcar_pci_write_reg(pcie, *data, PCICONF(index));
   154	
   155			return PCIBIOS_SUCCESSFUL;
   156		}
   157	
   158		/* Clear errors */
   159		rcar_pci_write_reg(pcie, rcar_pci_read_reg(pcie, PCIEERRFR), PCIEERRFR);
   160	
   161		/* Set the PIO address */
   162		rcar_pci_write_reg(pcie, PCIE_CONF_BUS(bus->number) |
   163			PCIE_CONF_DEV(dev) | PCIE_CONF_FUNC(func) | reg, PCIECAR);
   164	
   165		/* Enable the configuration access */
   166		if (pci_is_root_bus(bus->parent))
   167			rcar_pci_write_reg(pcie, CONFIG_SEND_ENABLE | TYPE0, PCIECCTLR);
   168		else
   169			rcar_pci_write_reg(pcie, CONFIG_SEND_ENABLE | TYPE1, PCIECCTLR);
   170	
   171		/* Check for errors */
   172		if (rcar_pci_read_reg(pcie, PCIEERRFR) & UNSUPPORTED_REQUEST)
   173			return PCIBIOS_DEVICE_NOT_FOUND;
   174	
   175		/* Check for master and target aborts */
   176		if (rcar_read_conf(pcie, RCONF(PCI_STATUS)) &
   177			(PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_REC_TARGET_ABORT))
   178			return PCIBIOS_DEVICE_NOT_FOUND;
   179	
   180		if (access_type == RCAR_PCI_ACCESS_READ)
   181			*data = rcar_pci_read_reg(pcie, PCIECDR);
   182		else
   183			rcar_pci_write_reg(pcie, *data, PCIECDR);
   184	
   185		/* Disable the configuration access */
   186		rcar_pci_write_reg(pcie, 0, PCIECCTLR);
   187	
   188		return PCIBIOS_SUCCESSFUL;
   189	}
   190	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
index 38b6e02edfa9..0be58a91ddea 100644
--- 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;
+
+	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);
+	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);
+	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);