On Sat, Jan 22, 2022 at 11:15:54PM +0100, marek.vasut@xxxxxxxxx wrote:
> From: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
>
> 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 <marek.vasut+renesas@xxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> Cc: Krzysztof Wilczyński <kw@xxxxxxxxx>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> Cc: Wolfram Sang <wsa@xxxxxxxxxxxxx>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
> ---
> V2: Rebase on 1/2
> V3: - Add .text.fixup on all three ldr/str/isb instructions and call
> fixup_exception() in the abort handler to trigger the fixup.
> - Propagate return value from read/write accessors, in case the
> access fails, return PCIBIOS_SET_FAILED, else PCIBIOS_SUCCESSFUL.
> ---
> drivers/pci/controller/pcie-rcar-host.c | 53 +++++++++++++++++++++++--
> 1 file changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
> index 7d38a9c50093..b2e521ee95eb 100644
> --- a/drivers/pci/controller/pcie-rcar-host.c
> +++ b/drivers/pci/controller/pcie-rcar-host.c
> @@ -114,6 +114,51 @@ static u32 rcar_read_conf(struct rcar_pcie *pcie, int where)
> return val >> shift;
> }
>
> +#ifdef CONFIG_ARM
> +#define __rcar_pci_rw_reg_workaround(instr) \
> + "1: " instr " %1, [%2]\n" \
> + "2: isb\n" \
> + "3: .pushsection .text.fixup,\"ax\"\n" \
> + " .align 2\n" \
> + "4: mov %0, #" __stringify(PCIBIOS_SET_FAILED) "\n" \
> + " b 3b\n" \
> + " .popsection\n" \
> + " .pushsection __ex_table,\"a\"\n" \
> + " .align 3\n" \
> + " .long 1b, 4b\n" \
> + " .long 1b, 4b\n" \
> + " .popsection\n"
> +#endif
> +
> +int rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
> +{
> + int error = PCIBIOS_SUCCESSFUL;
> +#ifdef CONFIG_ARM
> + asm volatile(
> + __rcar_pci_rw_reg_workaround("str")
> + : "+r"(error):"r"(val), "r"(pcie->base + reg) : "memory");
> +#else
> + rcar_pci_write_reg(pcie, val, reg);
> +#endif
> + return error;
> +}
> +
> +int rcar_pci_read_reg_workaround(struct rcar_pcie *pcie, u32 *val, unsigned int reg)
> +{
> + int error = PCIBIOS_SUCCESSFUL;
> +#ifdef CONFIG_ARM
> + asm volatile(
> + __rcar_pci_rw_reg_workaround("ldr")
> + : "+r"(error), "=r"(*val) : "r"(pcie->base + reg) : "memory");
> +
> + if (error != PCIBIOS_SUCCESSFUL)
> + *val = 0xffffffff;
PCI_SET_ERROR_RESPONSE()?
Maybe also use PCI_ERROR_RESPONSE in the subject and commit log
instead of "all Fs" to make it more greppable.
> +#else
> + *val = rcar_pci_read_reg(pcie, reg);
> +#endif
> + return error;
> +}
> +
> /* 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,
> @@ -185,14 +230,14 @@ 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);
> + ret = rcar_pci_read_reg_workaround(pcie, data, PCIECDR);
> else
> - rcar_pci_write_reg(pcie, *data, PCIECDR);
> + ret = rcar_pci_write_reg_workaround(pcie, *data, PCIECDR);
>
> /* Disable the configuration access */
> rcar_pci_write_reg(pcie, 0, PCIECCTLR);
>
> - return PCIBIOS_SUCCESSFUL;
> + return ret;
> }
>
> static int rcar_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
> @@ -1097,7 +1142,7 @@ static struct platform_driver rcar_pcie_driver = {
> static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
> unsigned int fsr, struct pt_regs *regs)
> {
> - return !!rcar_pcie_wakeup(pcie_dev, pcie_base);
> + return !fixup_exception(regs);
> }
>
> static const struct of_device_id rcar_pcie_abort_handler_of_match[] __initconst = {
> --
> 2.34.1
>
