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

["Shradha Todi" <shradha.t@samsung.com>]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 28 June 2025 01:01
> To: Shradha Todi <shradha.t@samsung.com>
> Cc: linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
linux-
> samsung-soc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-phy@lists.infradead.org; linux-
> fsd@tesla.com; manivannan.sadhasivam@linaro.org; lpieralisi@kernel.org; kw@linux.com;
> robh@kernel.org; bhelgaas@google.com; jingoohan1@gmail.com; krzk+dt@kernel.org;
> conor+dt@kernel.org; alim.akhtar@samsung.com; vkoul@kernel.org; kishon@kernel.org;
> arnd@arndb.de; m.szyprowski@samsung.com; jh80.chung@samsung.com; pankaj.dubey@samsung.com
> Subject: Re: [PATCH v2 09/10] PCI: exynos: Add support for Tesla FSD SoC
> 
> 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.
> 

Thanks for the review! Will take care of all mentioned changes in next version

> > @@ -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.
> 

Sure,  I have no issues in splitting the patches further. Though unfortunately,
I or anyone I know does not possess a board which has Exynos 5433 chipset.
Therefore, I'm unable to verify these changes for Exynos chipset. I took care
to not disturb the exynos flow functionally but would be great if someone
could test this and confirm that it works well on Exynos 5433 after the changes.

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