Skip to contentSkip to conversation

PCI: rcar: Finish transition to L1 state in rcar_pcie_config_access()

PATCH Superseded [email protected] 3 participants 7 messages Jan 16, 2022

Subsystems (4)
PCI Arm/RISC-V/Renesas architecture PCI native host bridge and endpoint drivers PCI driver for Renesas R-Car

v1 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

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

From: Marek Vasut <[email protected]>

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 <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Geert Uytterhoeven <[email protected]>
Cc Krzysztof Wilczyński <[email protected]>
Cc Lorenzo Pieralisi <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Yoshihiro Shimoda <[email protected]>
Cc [email protected]
---
 drivers/pci/controller/pcie-rcar-host.c | 61 ++++++++++++++++---------
 1 file changed, 40 insertions(+), 21 deletions(-)
drivers/pci/controller/pcie-rcar-host.c +40 -21
index 38b6e02edfa9..0be58a91ddea 100644
@@ -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);
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH 2/2 PCI: rcar: Return all Fs from read which triggered an exception

From: Marek Vasut <[email protected]>

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.

The current asynchronous external abort hook implementation restarts the instruction which finally triggered the fault, which can be a different instruction than the read/write instruction which started the faulting access. Usually the instruction which finally triggers the fault is one which has some data dependency on the result of the read/write. In case of read, the read value after fixup is undefined, while a read value of faulting read should be all Fs.

It is possible to enforce the fault using 'isb' instruction placed right after the read/write instruction which started the faulting access. Add custom register accessors which perform the read/write followed immediately by 'isb'.

This way, the fault always happens on the 'isb' and in case of read, which is located one instruction before the 'isb', it is now possible to fix up the return value of the read in the asynchronous external abort hook and make that read return all Fs.

Signed-off-by Marek Vasut <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Geert Uytterhoeven <[email protected]>
Cc Krzysztof Wilczyński <[email protected]>
Cc Lorenzo Pieralisi <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Yoshihiro Shimoda <[email protected]>
Cc [email protected]
---
 drivers/pci/controller/pcie-rcar-host.c | 67 ++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 2 deletions(-)
drivers/pci/controller/pcie-rcar-host.c +65 -2
index 0be58a91ddea..6da5af92568d 100644
@@ -106,6 +106,35 @@ static u32 rcar_read_conf(struct rcar_pcie *pcie, int where)
 	return val >> shift;
 }
 
+void rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
+{
+#ifdef CONFIG_ARM
+	asm volatile(
+		"	str %0, [%1]\n"
+		"	isb\n"
+	::"r"(val), "r"(pcie->base + reg):"memory");
+#else
+	rcar_pci_write_reg(pcie, val, reg);
+#endif
+}
+
+u32 rcar_pci_read_reg_workaround(struct rcar_pcie *pcie, unsigned int reg)
+{
+#ifdef CONFIG_ARM
+	u32 val;
+
+	asm volatile(
+		"rcar_pci_read_reg_workaround_start:\n"
+		"1:	ldr %0, [%1]\n"
+		"	isb\n"
+	: "=r"(val):"r"(pcie->base + reg):"memory");
+
+	return val;
+#else
+	return rcar_pci_read_reg(pcie, reg);
+#endif
+}
+
 /* Serialization is provided by 'pci_lock' in drivers/pci/access.c */
 static int rcar_pcie_config_access(struct rcar_pcie_host *host,
 		unsigned char access_type, struct pci_bus *bus,

@@ -178,9 +207,9 @@ static int rcar_pcie_config_access(struct rcar_pcie_host *host,
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	if (access_type == RCAR_PCI_ACCESS_READ)
-		*data = rcar_pci_read_reg(pcie, PCIECDR);
+		*data = rcar_pci_read_reg_workaround(pcie, PCIECDR);
 	else
-		rcar_pci_write_reg(pcie, *data, PCIECDR);
+		rcar_pci_write_reg_workaround(pcie, *data, PCIECDR);
 
 	/* Disable the configuration access */
 	rcar_pci_write_reg(pcie, 0, PCIECCTLR);

@@ -1090,7 +1119,11 @@ static struct platform_driver rcar_pcie_driver = {
 static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
 		unsigned int fsr, struct pt_regs *regs)
 {
+	extern u32 *rcar_pci_read_reg_workaround_start;
+	unsigned long pc = instruction_pointer(regs);
+	unsigned long instr = *(unsigned long *)pc;
 	unsigned long flags;
+	u32 reg, val;
 	int ret = 0;
 
 	spin_lock_irqsave(&pmsr_lock, flags);

@@ -1100,6 +1133,36 @@ static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
 	if (ret)
 		goto unlock_exit;
 
+	/*
+	 * Test whether the faulting instruction is 'isb' and if
+	 * so, test whether it is the 'isb' instruction within
+	 * rcar_pci_read_reg_workaround() asm volatile()
+	 * implementation of read access. If it is, fix it up.
+	 */
+	instr &= ~0xf;
+	if ((instr == 0xf57ff060 || instr == 0xf3bf8f60) &&
+	    (pc == (u32)&rcar_pci_read_reg_workaround_start + 4)) {
+		/*
+		 * If the instruction being executed was a read,
+		 * make it look like it read all-ones.
+		 */
+		instr = *(unsigned long *)(pc - 4);
+		reg = (instr >> 12) & 15;
+
+		if ((instr & 0x0c100000) == 0x04100000) {
+			if (instr & 0x00400000)
+				val = 255;
+			else
+				val = -1;
+
+			regs->uregs[reg] = val;
+			regs->ARM_pc += 4;
+		} else if ((instr & 0x0e100090) == 0x00100090) {
+			regs->uregs[reg] = -1;
+			regs->ARM_pc += 4;
+		}
+	}
+
 unlock_exit:
 	spin_unlock_irqrestore(&pmsr_lock, flags);
 	return ret;
Signature · 1 line
2.34.1
build bot
#2 lore raw

Re: [PATCH 2/2] PCI: rcar: Return all Fs from read which triggered an exception

I love your patch! Perhaps something to improve:

Hi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on helgaas-pci/next]
[also build test WARNING 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: arm-randconfig-c002-20220116 (https://download.01.org/0day-ci/archive/20220117/202201170248.g8zqyL3O-lkp(a)intel.com/config)
compiler: arm-linux-gnueabi-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/f784bebf4a058d633fc77579892309b12dd33edb
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 f784bebf4a058d633fc77579892309b12dd33edb
# 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=arm SHELL=/bin/bash drivers/pci/controller/

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/pci/controller/pcie-rcar-host.c:109:6: warning: no previous prototype for 'rcar_pci_write_reg_workaround' [-Wmissing-prototypes]
     109 | void rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/pci/controller/pcie-rcar-host.c:121:5: warning: no previous prototype for 'rcar_pci_read_reg_workaround' [-Wmissing-prototypes]

121 | u32 rcar_pci_read_reg_workaround(struct rcar_pcie *pcie, unsigned int reg)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/cc0JJaba.s: Assembler messages:
/tmp/cc0JJaba.s:1425: Error: selected processor does not support isb' in ARM mode /tmp/cc0JJaba.s:1486: Error: selected processor does not support isb' in ARM mode /tmp/cc0JJaba.s:2594: Error: selected processor does not support isb' in ARM mode /tmp/cc0JJaba.s:2622: Error: symbol rcar_pci_read_reg_workaround_start' is already defined /tmp/cc0JJaba.s:2624: Error: selected processor does not support `isb' in ARM mode

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

108

 > 109	void rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
   110	{
   111	#ifdef CONFIG_ARM
   112		asm volatile(
   113			"	str %0, [%1]\n"
   114			"	isb\n"
   115		::"r"(val), "r"(pcie->base + reg):"memory");
   116	#else
   117		rcar_pci_write_reg(pcie, val, reg);
   118	#endif
   119	}
   120	
 > 121	u32 rcar_pci_read_reg_workaround(struct rcar_pcie *pcie, unsigned int reg)

122 {
123 #ifdef CONFIG_ARM
124 u32 val;
125
126 asm volatile(
127 "rcar_pci_read_reg_workaround_start:\n"
128 "1: ldr %0, [%1]\n"
129 " isb\n"
130 : "=r"(val):"r"(pcie->base + reg):"memory");
131
132 return val;
133 #else
134 return rcar_pci_read_reg(pcie, reg);
135 #endif
136 }
137

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

build bot
#3 lore raw

Re: [PATCH 2/2] PCI: rcar: Return all Fs from read which triggered an exception

In reply to [email protected]
I love your patch! Perhaps something to improve:

Hi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on helgaas-pci/next]
[also build test WARNING 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: arm-randconfig-c002-20220116 (https://download.01.org/0day-ci/archive/20220117/202201170230.U9LeY58M-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c63a3175c2947e8c1a2d3bbe16a8586600705c54)

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
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/0day-ci/linux/commit/f784bebf4a058d633fc77579892309b12dd33edb
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 f784bebf4a058d633fc77579892309b12dd33edb
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/pci/controller/

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/pci/controller/pcie-rcar-host.c:109:6: warning: no previous prototype for function 'rcar_pci_write_reg_workaround' [-Wmissing-prototypes]
   void rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
        ^
   drivers/pci/controller/pcie-rcar-host.c:109:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
   ^
   static 
>> drivers/pci/controller/pcie-rcar-host.c:121:5: warning: no previous prototype for function 'rcar_pci_read_reg_workaround' [-Wmissing-prototypes]
   u32 rcar_pci_read_reg_workaround(struct rcar_pcie *pcie, unsigned int reg)
       ^
   drivers/pci/controller/pcie-rcar-host.c:121:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   u32 rcar_pci_read_reg_workaround(struct rcar_pcie *pcie, unsigned int reg)
   ^
   static 
   drivers/pci/controller/pcie-rcar-host.c:114:4: error: instruction requires: data-barriers
                   "       isb\n"
                    ^
   <inline asm>:2:2: note: instantiated into assembly here
           isb
           ^
   drivers/pci/controller/pcie-rcar-host.c:129:4: error: instruction requires: data-barriers
                   "       isb\n"
                    ^
   <inline asm>:3:2: note: instantiated into assembly here
           isb
           ^
   drivers/pci/controller/pcie-rcar-host.c:114:4: error: instruction requires: data-barriers
                   "       isb\n"
                    ^
   <inline asm>:2:2: note: instantiated into assembly here
           isb
           ^
   drivers/pci/controller/pcie-rcar-host.c:127:3: error: symbol 'rcar_pci_read_reg_workaround_start' is already defined
                   "rcar_pci_read_reg_workaround_start:\n"
                   ^
   <inline asm>:1:2: note: instantiated into assembly here
           rcar_pci_read_reg_workaround_start:
           ^
   drivers/pci/controller/pcie-rcar-host.c:129:4: error: instruction requires: data-barriers
                   "       isb\n"
                    ^
   <inline asm>:3:2: note: instantiated into assembly here
           isb
           ^

2 warnings and 5 errors generated.

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

108

 > 109	void rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
   110	{
   111	#ifdef CONFIG_ARM
   112		asm volatile(
   113			"	str %0, [%1]\n"
   114			"	isb\n"
   115		::"r"(val), "r"(pcie->base + reg):"memory");
   116	#else
   117		rcar_pci_write_reg(pcie, val, reg);
   118	#endif
   119	}
   120	
 > 121	u32 rcar_pci_read_reg_workaround(struct rcar_pcie *pcie, unsigned int reg)

122 {
123 #ifdef CONFIG_ARM
124 u32 val;
125
126 asm volatile(
127 "rcar_pci_read_reg_workaround_start:\n"
128 "1: ldr %0, [%1]\n"
129 " isb\n"
130 : "=r"(val):"r"(pcie->base + reg):"memory");
131
132 return val;
133 #else
134 return rcar_pci_read_reg(pcie, reg);
135 #endif
136 }
137

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

build bot
#4 lore raw

Re: [PATCH 2/2] PCI: rcar: Return all Fs from read which triggered an exception

In reply to [email protected]
I love your patch! Yet something to improve:

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: arm-randconfig-c002-20220116 (https://download.01.org/0day-ci/archive/20220117/202201170402.sPT8Gtpa-lkp(a)intel.com/config)
compiler: arm-linux-gnueabi-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/f784bebf4a058d633fc77579892309b12dd33edb
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 f784bebf4a058d633fc77579892309b12dd33edb
# 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=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

drivers/pci/controller/pcie-rcar-host.c:109:6: warning: no previous prototype for 'rcar_pci_write_reg_workaround' [-Wmissing-prototypes]
109 | void rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/pci/controller/pcie-rcar-host.c:121:5: warning: no previous prototype for 'rcar_pci_read_reg_workaround' [-Wmissing-prototypes]
121 | u32 rcar_pci_read_reg_workaround(struct rcar_pcie *pcie, unsigned int reg)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/ccquRjDl.s: Assembler messages:
/tmp/ccquRjDl.s:1425: Error: selected processor does not support `isb' in ARM mode
/tmp/ccquRjDl.s:1486: Error: selected processor does not support `isb' in ARM mode
/tmp/ccquRjDl.s:2594: Error: selected processor does not support `isb' in ARM mode

>> /tmp/ccquRjDl.s:2622: Error: symbol `rcar_pci_read_reg_workaround_start' is already defined

/tmp/ccquRjDl.s:2624: Error: selected processor does not support `isb' in ARM mode

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

#5 lore raw
In reply to [email protected]

Hi Marek,

Quoted 7 lines · On Sun, Jan 16, 2022 at 3:26 AM <[email protected]> wrote:

From: Marek Vasut <[email protected]>

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.

Quoted 6 lines

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 <[email protected]>

Thanks for your patch!

Quoted 5 lines
drivers/pci/controller/pcie-rcar-host.c +0 -0
@@ -54,6 +54,34 @@ static void __iomem *pcie_base;
  * device is runtime suspended or not.
  */

drivers/pci/controller/pcie-rcar-host.c static void __iomem *pcie_base; · line 65

Quoted 10 lines
drivers/pci/controller/pcie-rcar-host.c +9 -0
 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()?

Quoted 23 lines
drivers/pci/controller/pcie-rcar-host.c +19 -0
+
+       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 */

drivers/pci/controller/pcie-rcar-host.c static int rcar_pcie_config_access(struct rcar_pcie_host *host, · line 122

Quoted 10 lines
drivers/pci/controller/pcie-rcar-host.c +7 -0
@@ -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?

Quoted 13 lines
drivers/pci/controller/pcie-rcar-host.c +2 -1
+       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)
 {

drivers/pci/controller/pcie-rcar-host.c static struct platform_driver rcar_pcie_driver = { · line 1099

Quoted 10 lines
drivers/pci/controller/pcie-rcar-host.c +2 -3
        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?

Quoted 12 lines
drivers/pci/controller/pcie-rcar-host.c +1 -10
+       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.
-        */

drivers/pci/controller/pcie-rcar-host.c static struct platform_driver rcar_pcie_driver = { · line 1104

Quoted 10 lines
drivers/pci/controller/pcie-rcar-host.c +0 -7
-       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 -- [email protected]

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

build bot
#6 lore raw
In reply to [email protected]
I love your patch! Yet something to improve:

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(a)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 <[email protected]>

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(a)lists.01.org