On Mon, Aug 11, 2025 at 09:16:37PM +0530, Shradha Todi wrote:
> Add host and endpoint controller driver support for FSD SoC.
I think this might be easier if you added host mode first, then added
endpoint mode with a separate patch.
It's kind of unfortunate that the driver uses "ep" everywhere for
struct exynos_pcie pointers. It's going to be confusing because "ep"
is also commonly used for endpoint-related things, e.g., struct
dw_pcie_ep pointers. Maybe it's not worth changing; I dunno.
> Signed-off-by: Shradha Todi <shradha.t@xxxxxxxxxxx>
> ---
> drivers/pci/controller/dwc/pci-exynos.c | 278 ++++++++++++++++++++++++
> 1 file changed, 278 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index ef1f42236575..9aabfecdc147 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -18,6 +18,8 @@
> #include <linux/platform_device.h>
> #include <linux/phy/phy.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> #include <linux/mod_devicetable.h>
> #include <linux/module.h>
>
> @@ -49,15 +51,35 @@
> #define EXYNOS_PCIE_ELBI_SLV_ARMISC 0x120
> #define EXYNOS_PCIE_ELBI_SLV_DBI_ENABLE BIT(21)
>
> +#define FSD_IRQ2_STS 0x008
> +#define FSD_IRQ_MSI_ENABLE BIT(17)
> +#define FSD_IRQ2_EN 0x018
> +#define FSD_PCIE_APP_LTSSM_ENABLE 0x054
> +#define FSD_PCIE_LTSSM_ENABLE 0x1
> +#define FSD_PCIE_DEVICE_TYPE 0x080
> +#define FSD_DEVICE_TYPE_RC 0x4
> +#define FSD_DEVICE_TYPE_EP 0x0
> +#define FSD_PCIE_CXPL_DEBUG_00_31 0x2c8
> +
> /* to store different SoC variants of Samsung */
> enum samsung_pcie_variants {
> + FSD,
> EXYNOS_5433,
> };
>
> +/* Values to be written to SYSREG to view DBI space as CDM/DBI2/IATU/DMA */
> +enum fsd_pcie_addr_type {
> + ADDR_TYPE_DBI = 0x0,
> + ADDR_TYPE_DBI2 = 0x12,
> + ADDR_TYPE_ATU = 0x36,
> + ADDR_TYPE_DMA = 0x3f,
> +};
> +
> struct samsung_pcie_pdata {
> struct pci_ops *pci_ops;
> const struct dw_pcie_ops *dwc_ops;
> const struct dw_pcie_host_ops *host_ops;
> + const struct dw_pcie_ep_ops *ep_ops;
> const struct samsung_res_ops *res_ops;
> unsigned int soc_variant;
> enum dw_pcie_device_mode device_mode;
> @@ -67,6 +89,8 @@ struct exynos_pcie {
> struct dw_pcie pci;
> void __iomem *elbi_base;
> const struct samsung_pcie_pdata *pdata;
> + struct regmap *sysreg;
> + unsigned int sysreg_offset;
> struct clk_bulk_data *clks;
> struct phy *phy;
> struct regulator_bulk_data *supplies;
> @@ -334,11 +358,201 @@ static const struct dw_pcie_ops exynos_dw_pcie_ops = {
> .start_link = exynos_pcie_start_link,
> };
>
> +static void fsd_pcie_stop_link(struct dw_pcie *pci)
> +{
> + u32 val;
> + struct exynos_pcie *ep = to_exynos_pcie(pci);
> +
> + val = readl(ep->elbi_base + FSD_PCIE_APP_LTSSM_ENABLE);
> + val &= ~FSD_PCIE_LTSSM_ENABLE;
> + writel(val, ep->elbi_base + FSD_PCIE_APP_LTSSM_ENABLE);
> +}
> +
> +static int fsd_pcie_start_link(struct dw_pcie *pci)
> +{
> + struct exynos_pcie *ep = to_exynos_pcie(pci);
> + struct dw_pcie_ep *dw_ep = &pci->ep;
> +
> + if (dw_pcie_link_up(pci))
> + return 0;
> +
> + writel(FSD_PCIE_LTSSM_ENABLE, ep->elbi_base + FSD_PCIE_APP_LTSSM_ENABLE);
> +
> + /* no need to wait for link in case of host as core files take care */
> + if (ep->pdata->device_mode == DW_PCIE_RC_TYPE)
> + return 0;
> +
> + /* check if the link is up or not in case of EP */
> + if (!dw_pcie_wait_for_link(pci)) {
> + dw_pcie_ep_linkup(dw_ep);
> + return 0;
> + }
> +
> + return -ETIMEDOUT;
> +}
> +
> +static irqreturn_t fsd_pcie_irq_handler(int irq, void *arg)
> +{
> + u32 val;
> + struct exynos_pcie *ep = arg;
> + struct dw_pcie *pci = &ep->pci;
> + struct dw_pcie_rp *pp = &pci->pp;
> +
> + val = readl(ep->elbi_base + FSD_IRQ2_STS);
> + if ((val & FSD_IRQ_MSI_ENABLE) == FSD_IRQ_MSI_ENABLE) {
> + val &= FSD_IRQ_MSI_ENABLE;
> + writel(val, ep->elbi_base + FSD_IRQ2_STS);
This looks weird because FSD_IRQ_MSI_ENABLE sounds like an *enable*
bit, but here you're treating it as a *status* bit.
As far as I can tell, you set FSD_IRQ_MSI_ENABLE once at probe-time in
fsd_pcie_msi_init(), then you clear it here in an IRQ handler, and it
will never be set again. That seems wrong; am I missing something?
> + dw_handle_msi_irq(pp);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void fsd_pcie_msi_init(struct exynos_pcie *ep)
> +{
> + int val;
> +
> + val = readl(ep->elbi_base + FSD_IRQ2_EN);
> + val |= FSD_IRQ_MSI_ENABLE;
> + writel(val, ep->elbi_base + FSD_IRQ2_EN);
> +}
> +
> +static void __iomem *fsd_atu_setting(struct dw_pcie *pci, void __iomem *base)
The "setting" name suggests that this merely returns an address
without side effects, but in fact it actively *sets* the view.
In this case there's no locking around:
addr = fsd_atu_setting(pci, base);
dw_pcie_read(addr + reg, size, &val);
even though concurrent calls would cause issues, but I think that's OK
because we only get there via the driver, and I assume multiple DBI or
DBI2 accesses never happen because they're not used in asynchronous
paths like interrupt handlers.
But I think a name that hints at the fact that this does have side
effects would be helpful as a reminder in the callers that they must
not be used concurrently.
> +{
> + struct exynos_pcie *ep = to_exynos_pcie(pci);
> +
> + if (base >= pci->atu_base) {
> + regmap_write(ep->sysreg, ep->sysreg_offset, ADDR_TYPE_ATU);
> + return (base - DEFAULT_DBI_ATU_OFFSET);
> + } else if (base == pci->dbi_base) {
> + regmap_write(ep->sysreg, ep->sysreg_offset, ADDR_TYPE_DBI);
> + } else if (base == pci->dbi_base2) {
> + regmap_write(ep->sysreg, ep->sysreg_offset, ADDR_TYPE_DBI2);
> + }
> +
> + return base;
> +}
> +
> +static u32 fsd_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
> + u32 reg, size_t size)
> +{
> + void __iomem *addr;
> + u32 val;
> +
> + addr = fsd_atu_setting(pci, base);
> + dw_pcie_read(addr + reg, size, &val);
> +
> + return val;
> +}
> +
> +static void fsd_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
> + u32 reg, size_t size, u32 val)
> +{
> + void __iomem *addr;
> +
> + addr = fsd_atu_setting(pci, base);
> + dw_pcie_write(addr + reg, size, val);
> +}
> +
> +static void fsd_pcie_write_dbi2(struct dw_pcie *pci, void __iomem *base,
> + u32 reg, size_t size, u32 val)
> +{
> + struct exynos_pcie *ep = to_exynos_pcie(pci);
> +
> + fsd_atu_setting(pci, base);
> + dw_pcie_write(pci->dbi_base + reg, size, val);
> + regmap_write(ep->sysreg, ep->sysreg_offset, ADDR_TYPE_DBI);
> +}
> +
> +static bool fsd_pcie_link_up(struct dw_pcie *pci)
> +{
> + u32 val;
> + struct exynos_pcie *ep = to_exynos_pcie(pci);
> +
> + val = readl(ep->elbi_base + FSD_PCIE_CXPL_DEBUG_00_31);
> + val &= PORT_LOGIC_LTSSM_STATE_MASK;
> +
> + return (val == PORT_LOGIC_LTSSM_STATE_L0);
> +}
> +
> +static int fsd_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct exynos_pcie *ep = to_exynos_pcie(pci);
> +
> + phy_init(ep->phy);
> + fsd_pcie_msi_init(ep);
> +
> + return 0;
> +}
> +
> +static const struct dw_pcie_host_ops fsd_pcie_host_ops = {
> + .init = fsd_pcie_host_init,
> +};
> +
> +static int fsd_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> + unsigned int type, u16 interrupt_num)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +
> + switch (type) {
> + case PCI_IRQ_INTX:
> + return dw_pcie_ep_raise_intx_irq(ep, func_no);
> + case PCI_IRQ_MSIX:
> + dev_err(pci->dev, "EP does not support MSI-X\n");
> + return -EINVAL;
> + case PCI_IRQ_MSI:
> + return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> + default:
> + dev_err(pci->dev, "UNKNOWN IRQ type\n");
> + }
> +
> + return 0;
> +}
> +
> +static const struct pci_epc_features fsd_pcie_epc_features = {
> + .linkup_notifier = false,
> + .msi_capable = true,
> + .msix_capable = false,
I think we should omit features we do *not* support instead of calling
them out explicitly, e.g., we don't need .linkup_notifier or
.msix_capable.
We've added them in the past, but they're unnecessary and they lead to
either pervasive changes (adding ".new_feature = false" to all
existing drivers when adding the feature) or inconsistency (new
drivers include ".new_feature = false" but existing drivers do not).
> +};
> +
> +static const struct pci_epc_features *fsd_pcie_get_features(struct dw_pcie_ep *ep)
> +{
> + return &fsd_pcie_epc_features;
> +}
> +
> +static const struct dw_pcie_ep_ops fsd_ep_ops = {
> + .raise_irq = fsd_pcie_raise_irq,
> + .get_features = fsd_pcie_get_features,
> +};
> +
> +static void fsd_set_device_mode(struct exynos_pcie *ep)
> +{
> + if (ep->pdata->device_mode == DW_PCIE_RC_TYPE)
> + writel(FSD_DEVICE_TYPE_RC, ep->elbi_base + FSD_PCIE_DEVICE_TYPE);
> + else
> + writel(FSD_DEVICE_TYPE_EP, ep->elbi_base + FSD_PCIE_DEVICE_TYPE);
> +}
> +
> +static const struct dw_pcie_ops fsd_dw_pcie_ops = {
> + .read_dbi = fsd_pcie_read_dbi,
> + .write_dbi = fsd_pcie_write_dbi,
> + .write_dbi2 = fsd_pcie_write_dbi2,
> + .start_link = fsd_pcie_start_link,
> + .stop_link = fsd_pcie_stop_link,
> + .link_up = fsd_pcie_link_up,
> +};
> +
> static const struct samsung_res_ops exynos_res_ops_data = {
> .init_regulator = exynos_init_regulator,
> .pcie_irq_handler = exynos_pcie_irq_handler,
> };
>
> +static const struct samsung_res_ops fsd_res_ops_data = {
> + .pcie_irq_handler = fsd_pcie_irq_handler,
> + .set_device_mode = fsd_set_device_mode,
> +};
> +
> static int exynos_pcie_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -361,6 +575,26 @@ static int exynos_pcie_probe(struct platform_device *pdev)
> if (IS_ERR(ep->phy))
> return PTR_ERR(ep->phy);
>
> + if (ep->pdata->soc_variant == FSD) {
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
> + if (ret)
> + return ret;
> +
> + ep->sysreg = syscon_regmap_lookup_by_phandle(dev->of_node,
> + "samsung,syscon-pcie");
> + if (IS_ERR(ep->sysreg)) {
> + dev_err(dev, "sysreg regmap lookup failed.\n");
> + return PTR_ERR(ep->sysreg);
> + }
> +
> + ret = of_property_read_u32_index(dev->of_node, "samsung,syscon-pcie", 1,
> + &ep->sysreg_offset);
> + if (ret) {
> + dev_err(dev, "couldn't get the register offset for syscon!\n");
> + return ret;
> + }
> + }
This is a good example of a complicated set of things where I think
you should either add a SoC-specific function pointer to do this or
test a property, e.g., "DMA width", instead of testing for a specific
SoC.
> /* External Local Bus interface (ELBI) registers */
> ep->elbi_base = devm_platform_ioremap_resource_byname(pdev, "elbi");
> if (IS_ERR(ep->elbi_base))
> @@ -397,6 +631,22 @@ static int exynos_pcie_probe(struct platform_device *pdev)
> if (ret < 0)
> goto fail_phy_init;
>
> + break;
> + case DW_PCIE_EP_TYPE:
> + phy_init(ep->phy);
> +
> + ep->pci.ep.ops = pdata->ep_ops;
> +
> + ret = dw_pcie_ep_init(&ep->pci.ep);
> + if (ret < 0)
> + goto fail_phy_init;
> +
> + ret = dw_pcie_ep_init_registers(&ep->pci.ep);
> + if (ret)
> + goto fail_phy_init;
> +
> + pci_epc_init_notify(ep->pci.ep.epc);
> +
> break;
> default:
> dev_err(dev, "invalid device type\n");
> @@ -436,6 +686,9 @@ static int exynos_pcie_suspend_noirq(struct device *dev)
> if (ep->pdata->device_mode == DW_PCIE_EP_TYPE)
> return 0;
>
> + if (ep->pdata->dwc_ops->stop_link)
> + ep->pdata->dwc_ops->stop_link(pci);
> +
> if (ep->pdata->soc_variant == EXYNOS_5433)
> exynos_pcie_assert_core_reset(ep);
> phy_power_off(ep->phy);
> @@ -471,6 +724,23 @@ static const struct dev_pm_ops exynos_pcie_pm_ops = {
> exynos_pcie_resume_noirq)
> };
>
> +
> +static const struct samsung_pcie_pdata fsd_hw3_pcie_rc_pdata = {
> + .dwc_ops = &fsd_dw_pcie_ops,
> + .host_ops = &fsd_pcie_host_ops,
> + .res_ops = &fsd_res_ops_data,
> + .soc_variant = FSD,
> + .device_mode = DW_PCIE_RC_TYPE,
> +};
> +
> +static const struct samsung_pcie_pdata fsd_hw3_pcie_ep_pdata = {
> + .dwc_ops = &fsd_dw_pcie_ops,
> + .ep_ops = &fsd_ep_ops,
> + .res_ops = &fsd_res_ops_data,
> + .soc_variant = FSD,
> + .device_mode = DW_PCIE_EP_TYPE,
> +};
> +
> static const struct samsung_pcie_pdata exynos_5433_pcie_rc_pdata = {
> .dwc_ops = &exynos_dw_pcie_ops,
> .pci_ops = &exynos_pci_ops,
> @@ -485,6 +755,14 @@ static const struct of_device_id exynos_pcie_of_match[] = {
> .compatible = "samsung,exynos5433-pcie",
> .data = (void *) &exynos_5433_pcie_rc_pdata,
> },
> + {
> + .compatible = "tesla,fsd-pcie",
> + .data = (void *) &fsd_hw3_pcie_rc_pdata,
> + },
> + {
> + .compatible = "tesla,fsd-pcie-ep",
> + .data = (void *) &fsd_hw3_pcie_ep_pdata,
> + },
> { },
> };
>
> --
> 2.49.0
>
