| 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 |
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
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 --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);