RE: [PATCH v5 6/7] PCI: rcar: Add support for rcar PCIe controller in endpoint mode
From: Yoshihiro Shimoda
Date: Tue Mar 17 2020 - 05:59:27 EST
Hi Prabhakar-san,
Thank you for the patch!
> From: Lad Prabhakar, Sent: Saturday, February 29, 2020 12:41 AM
>
> This patch adds support for rcar PCIe controller to work in endpoint mode.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
> drivers/pci/controller/Kconfig | 8 +
> drivers/pci/controller/Makefile | 1 +
> drivers/pci/controller/pcie-rcar-ep.c | 490 ++++++++++++++++++++++++++++++++++
> drivers/pci/controller/pcie-rcar.h | 4 +
> 4 files changed, 503 insertions(+)
> create mode 100644 drivers/pci/controller/pcie-rcar-ep.c
>
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 37e0ea7..9bf4b02 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -62,6 +62,14 @@ config PCIE_RCAR_HOST
> Say Y here if you want PCIe controller support on R-Car SoCs in host
> mode.
>
> +config PCIE_RCAR_EP
> + bool "Renesas R-Car PCIe endpoint controller"
> + depends on ARCH_RENESAS || COMPILE_TEST
> + depends on PCI_ENDPOINT
> + help
> + Say Y here if you want PCIe controller support on R-Car SoCs in
> + endpoint mode.
> +
> config PCI_HOST_COMMON
> bool
> select PCI_ECAM
> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
> index b4ada32..067bd33 100644
> --- a/drivers/pci/controller/Makefile
> +++ b/drivers/pci/controller/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_PCI_AARDVARK) += pci-aardvark.o
> obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o
> obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
> obj-$(CONFIG_PCIE_RCAR_HOST) += pcie-rcar.o pcie-rcar-host.o
> +obj-$(CONFIG_PCIE_RCAR_EP) += pcie-rcar.o pcie-rcar-ep.o
> obj-$(CONFIG_PCI_HOST_COMMON) += pci-host-common.o
> obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
> obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
> diff --git a/drivers/pci/controller/pcie-rcar-ep.c b/drivers/pci/controller/pcie-rcar-ep.c
> new file mode 100644
> index 0000000..db89bbe
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-rcar-ep.c
> @@ -0,0 +1,490 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCIe endpoint driver for Renesas R-Car SoCs
> + * Copyright (c) 2020 Renesas Electronics Europe GmbH
> + *
> + * Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/pci.h>
> +#include <linux/pci-epc.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +
> +#include "pcie-rcar.h"
> +
> +/* Structure representing the PCIe interface */
> +struct rcar_pcie {
> + phys_addr_t *ob_addr;
I think "ob_mapped_addr" is better.
> + struct pci_epc_mem_window *ob_window;
I think we can get these windows from "array of address space of
the endpoint controller" in struct pci_epc. If so, we can remove
this member.
> + struct pci_epc *epc;
This member can be removed like pcie-cadence-ep.c because
this is not used except saving the epc value from devm_pci_epc_create().
<snip>
> +static int rcar_pcie_ep_start(struct pci_epc *epc)
> +{
> + struct rcar_pcie *ep = epc_get_drvdata(epc);
> +
> + rcar_pci_write_reg(ep->base, CFINIT, PCIETCTLR);
The following setting is needed before CFINIT like host.
rcar_pci_write_reg(pcie->base, MACCTLR_INIT_VAL, MACCTLR);
> +
> + return 0;
> +}
> +
> +static void rcar_pcie_ep_stop(struct pci_epc *epc)
> +{
> + struct rcar_pcie *ep = epc_get_drvdata(epc);
> +
> + rcar_pci_write_reg(ep->base, 0, PCIETCTLR);
> +}
> +
> +static const struct pci_epc_features rcar_pcie_epc_features = {
> + .linkup_notifier = false,
> + .msi_capable = false,
> + .msix_capable = false,
> + /* use 64-bit bars so mark bar1/3/5 as reserved */
> + .reserved_bar = 1 << BAR_1 | 1 << BAR_3 | 1 << BAR_5,
> + .bar_fixed_64bit = (1 << BAR_0) | (1 << BAR_2) | (1 << BAR_4),
These parentheses are not needed like .reserved_bar.
<snip>
> + err = pci_epc_mem_init(epc, pcie->ob_window, pcie->num_ob_windows);
> + if (err < 0) {
> + dev_err(dev, "failed to initialize the epc memory space\n");
> + goto err_pm_put;
> + }
> +
> + rcar_pcie_ep_hw_init(pcie);
I'm not sure, but I wonder if we should call this hw init before pci_epc_mem_init().
<snip>
> +builtin_platform_driver(rcar_pcie_ep_driver);
> diff --git a/drivers/pci/controller/pcie-rcar.h b/drivers/pci/controller/pcie-rcar.h
> index b529d806..5564ca8 100644
> --- a/drivers/pci/controller/pcie-rcar.h
> +++ b/drivers/pci/controller/pcie-rcar.h
> @@ -17,6 +17,7 @@
> #define PCIECDR 0x000020
> #define PCIEMSR 0x000028
> #define PCIEINTXR 0x000400
> +#define ASTINTX_SHIFT BIT(16)
Just "ASTINTX" is better.
> #define PCIEPHYSR 0x0007f0
> #define PHYRDY BIT(0)
> #define PCIEMSITXR 0x000840
> @@ -55,12 +56,15 @@
>
> /* Configuration */
> #define PCICONF(x) (0x010000 + ((x) * 0x4))
> +#define INTDIS_SHIFT BIT(10)
Same here (we can remove "_SHIFT").
Best regards,
Yoshihiro Shimoda
> #define PMCAP(x) (0x010040 + ((x) * 0x4))
> #define EXPCAP(x) (0x010070 + ((x) * 0x4))
> #define VCCAP(x) (0x010100 + ((x) * 0x4))
>
> /* link layer */
> +#define IDSETR0 0x011000
> #define IDSETR1 0x011004
> +#define SUBIDSETR 0x011024
> #define TLCTLR 0x011048
> #define MACSR 0x011054
> #define SPCHGFIN BIT(4)
> --
> 2.7.4