Re: [PATCH v2 09/10] PCI: exynos: Add support for Tesla FSD SoC

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Jun 25, 2025 at 10:22:28PM +0530, Shradha Todi wrote:
> Add host and endpoint controller driver support for FSD SoC.

> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -20,6 +20,8 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>

The trend is to sort these alphabetically.  The last couple additions
didn't observe this, but maybe these new ones could go a little
farther up and make it more sorted rather than less?

> +#define FSD_PCIE_CXPL_DEBUG_00_31		0x2C8

Existing #defines use lower-case hex; please follow suit.

> +/* to store different SoC variants of Samsung */
> +enum samsung_pcie_variants {
> +	FSD,
> +	EXYNOS_5433,
> +};

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

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

Remove blank lines to match style of fsd_pcie_write_dbi2().

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

Ditto.

> +}
> +
> +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 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 MSIX\n");

s/MSIX/MSI-X/ to match spec usage.

> @@ -373,13 +617,43 @@ static int exynos_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	platform_set_drvdata(pdev, ep);
> -	ret = samsung_irq_init(ep, pdev);
> -	if (ret)
> -		goto fail_regulator;
> -	ep->pci.pp.ops = pdata->host_ops;
> -	ret = dw_pcie_host_init(&ep->pci.pp);
> -	if (ret < 0)
> +
> +	if (pdata->res_ops->set_device_mode)
> +		pdata->res_ops->set_device_mode(ep);
> +
> +	switch (ep->pdata->device_mode) {
> +	case DW_PCIE_RC_TYPE:
> +		ret = samsung_irq_init(ep, pdev);
> +		if (ret)
> +			goto fail_regulator;
> +
> +		ep->pci.pp.ops = pdata->host_ops;
> +
> +		ret = dw_pcie_host_init(&ep->pci.pp);
> +		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");
>  		goto fail_phy_init;
> +	}

This would be a little nicer if you added soc_variant and device_mode
and the code that sets and tests them for exynos_5433 first in a
separate patch.  Then it would be more obvious that the new FSD parts
don't affect exynos_5433 since this patch would only be *adding*
FSD-specific things.

>  static const struct samsung_pcie_pdata exynos_5433_pcie_rc_pdata = {
>  	.dwc_ops		= &exynos_dw_pcie_ops,
>  	.pci_ops		= &exynos_pci_ops,
>  	.host_ops		= &exynos_pcie_host_ops,
>  	.res_ops		= &exynos_res_ops_data,
> +	.soc_variant		= EXYNOS_5433,
> +	.device_mode		= DW_PCIE_RC_TYPE,
>  };

>  static const struct of_device_id exynos_pcie_of_match[] = {
> @@ -449,6 +756,14 @@ static const struct of_device_id exynos_pcie_of_match[] = {
>  		.compatible = "samsung,exynos5433-pcie",
>  		.data = (void *) &exynos_5433_pcie_rc_pdata,
>  	},