PCI: rcar: Finish transition to L1 state in rcar_pcie_config_access()
Subsystems (4)
PATCH v2 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.
---
V2: Pull DEFINE_SPINLOCK(pmsr_lock) and rcar_pcie_wakeup() out of ifdef(CONFIG_ARM),
since this change is applicable even on arm64
--- drivers/pci/controller/pcie-rcar-host.c | 78 ++++++++++++++++--------- 1 file changed, 49 insertions(+), 29 deletions(-)
drivers/pci/controller/pcie-rcar-host.c +49 -29
@@ -41,6 +41,15 @@ struct rcar_msi {int irq2; }; +/* Structure representing the PCIe interface */ +struct rcar_pcie_host { +struct rcar_pcie pcie; +struct phy *phy; +struct clk *bus_clk; +struct rcar_msi msi; +int (*phy_init_fn)(struct rcar_pcie_host *host); +}; +#ifdef CONFIG_ARM /* * Here we keep a static copy of the remapped PCIe controller address.
@@ -56,14 +65,34 @@ static void __iomem *pcie_base;static struct device *pcie_dev;#endif -/* Structure representing the PCIe interface */ -struct rcar_pcie_host { -struct rcar_pcie pcie; -struct phy *phy; -struct clk *bus_clk; -struct rcar_msi msi; -int (*phy_init_fn)(struct rcar_pcie_host *host); -}; +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; +}static struct rcar_pcie_host *msi_to_host (struct rcar_msi *msi) {
@@ -85,6 +114,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 +1088,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
PATCH v2 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.
---
V2: Rebase on 1/2
--- drivers/pci/controller/pcie-rcar-host.c | 67 ++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 2 deletions(-)
drivers/pci/controller/pcie-rcar-host.c +65 -2
@@ -107,6 +107,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 + asmvolatile ( +" 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; + + asmvolatile ( +"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,
@@ -179,9 +208,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 );
@@ -1091,7 +1120,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);
@@ -1101,6 +1134,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
Re: [PATCH v2 2/2] PCI: rcar: Return all Fs from read which triggered an exception
Quoted 9 lines · On Mon, Jan 17, 2022 at 11:03 PM <[email protected]> wrote:
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.
Hi Marek,
As mentioned on IRC, I think this can be done a lot simpler, using a .text.fixup section hack:
drivers/pci/controller/pcie-rcar-host.c static u32 rcar_read_conf(struct rcar_pcie *pcie, int where) · line 116
Quoted 7 lines
drivers/pci/controller/pcie-rcar-host.c +7
-0+void rcar_pci_write_reg_workaround (struct rcar_pcie *pcie,u32 val,unsigned int reg) +{ +#ifdef CONFIG_ARM + asmvolatile ( +" str %0, [%1]\n" +" isb\n" + ::"r" (val),"r" (pcie->base + reg):"memory" );
I think this would looks something like
int error = 0;
asm volatile(
" str %1, [%2]\n"
"1: isb\n"
"2:\n"
" pushsection .text.fixup,\"ax\"\n"
" .align 2\n" \
"3: mov %0, %3\n" \
" b 2b\n" \
" .popsection\n" \
" .pushsection __ex_table,\"a\"\n" \
" .align 3\n" \
" .long 1b, 3b\n" \
" .popsection" \
: "+r" (error) :"r"(val), "r"(pcie->base + reg), "i" (-ENXIO):"memory");This saves you from hand-parsing the instruction sequence, which tends to be even more fragile. After this, you just need to check the 'error' variable,
which remains at 0 normally but contains -ENXIO if an exception hits.
I'm not entirely sure this works for the particular exception you are getting, and it probably requires not registering the rcar_pcie_aarch32_abort_handler function, but it seems likely to work.
Arnd
Re: [PATCH v2 2/2] PCI: rcar: Return all Fs from read which triggered an exception
In reply to [email protected]Quoted 119 lines · On Mon, Jan 17, 2022 at 11:03:55PM +0100, [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.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]>---
V2: Rebase on 1/2--- drivers/pci/controller/pcie-rcar-host.c | 67 ++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 2 deletions(-)drivers/pci/controller/pcie-rcar-host.c +46
-2@@ -107,6 +107,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 + asmvolatile ( +" 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; + + asmvolatile ( +"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,@@ -179,9 +208,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 );@@ -1091,7 +1120,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);@@ -1101,6 +1134,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,
drivers/pci/controller/pcie-rcar-host.c static int rcar_pcie_aarch32_abort_handler(unsigned long addr, · line 1157
Quoted 10 lines
drivers/pci/controller/pcie-rcar-host.c +10
-0+* 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 ;
Can you please use PCI_SET_ERROR_RESPONSE() or something similar here to make this greppable?
drivers/pci/controller/pcie-rcar-host.c static int rcar_pcie_aarch32_abort_handler(unsigned long addr, · line 1162
Quoted 4 lines
drivers/pci/controller/pcie-rcar-host.c +4
-0+ regs->uregs[reg] = val; + regs->ARM_pc +=4 ; + }else if ((instr &0x0e100090 ) ==0x00100090 ) { + regs->uregs[reg] =-1 ;
Also here, I guess?
Quoted 10 lines
drivers/pci/controller/pcie-rcar-host.c +4
-0+ regs->ARM_pc +=4 ; + } + } + unlock_exit:spin_unlock_irqrestore (&pmsr_lock, flags);return ret;Signature · 1 line
2.34.1
Re: [PATCH v2 2/2] PCI: rcar: Return all Fs from read which triggered an exception
Quoted 12 lines · Bjorn Helgaas
Quoted 10 lines · On Mon, Jan 17, 2022 at 11:03:55PM +0100, [email protected] wrote:
drivers/pci/controller/pcie-rcar-host.c +10
-0+ 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 ; +
Quoted 8 lines
drivers/pci/controller/pcie-rcar-host.c static int rcar_pcie_aarch32_abort_handler(unsigned long addr, · line 1157
Quoted 5 lines
drivers/pci/controller/pcie-rcar-host.c +5
-0+if ((instr &0x0c100000 ) ==0x04100000 ) { +if (instr &0x00400000 ) + val =255 ; +else + val =-1 ;Can you please use
PCI_SET_ERROR_RESPONSE()or something similar here to make this greppable?
I should have mentioned that PCI_SET_ERROR_RESPONSE() was added in the current merge window, so it will appear in v5.17-rc1.
Bjorn
Hi Marek,
Quoted 7 lines · On Mon, Jan 17, 2022 at 11:04 PM <[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!
I believe all my comments on v1[1] are still valid.
[1] https://lore.kernel.org/r/CAMuHMdXUteqOinkCNH8L-dC=W82DChQSupAXv_Uhjq5M=T5uxQ@mail.gmail.com/
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