> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: 28 June 2025 01:01
> To: Shradha Todi <shradha.t@xxxxxxxxxxx>
> Cc: linux-pci@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
linux-
> samsung-soc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-phy@xxxxxxxxxxxxxxxxxxx; linux-
> fsd@xxxxxxxxx; manivannan.sadhasivam@xxxxxxxxxx; lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx;
> robh@xxxxxxxxxx; bhelgaas@xxxxxxxxxx; jingoohan1@xxxxxxxxx; krzk+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; alim.akhtar@xxxxxxxxxxx; vkoul@xxxxxxxxxx; kishon@xxxxxxxxxx;
> arnd@xxxxxxxx; m.szyprowski@xxxxxxxxxxx; jh80.chung@xxxxxxxxxxx; pankaj.dubey@xxxxxxxxxxx
> 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,
> > },
