In order to extend this driver to all Samsung manufactured SoCs having
DWC PCIe controller, add private data structure which will hold platform
device specific information. It holds function ops like DWC host ops,
DWC generic ops, and PCI read/write ops which will be used as driver
data for different compatibles.
Suggested-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
drivers/pci/controller/dwc/pci-exynos.c | 32 ++++++++++++++++++++-----
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index 286f4987d56f..540612e76f4b 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -49,9 +49,16 @@
#define EXYNOS_PCIE_ELBI_SLV_ARMISC 0x120
#define EXYNOS_PCIE_ELBI_SLV_DBI_ENABLE BIT(21)
+struct samsung_pcie_pdata {
+ struct pci_ops *pci_ops;
+ const struct dw_pcie_ops *dwc_ops;
+ const struct dw_pcie_host_ops *host_ops;
+};
+
struct exynos_pcie {
struct dw_pcie pci;
void __iomem *elbi_base;
+ const struct samsung_pcie_pdata *pdata;
struct clk_bulk_data *clks;
struct phy *phy;
struct regulator_bulk_data supplies[2];
@@ -220,7 +227,7 @@ static int exynos_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);
- pp->bridge->ops = &exynos_pci_ops;
+ pp->bridge->ops = ep->pdata->pci_ops;
exynos_pcie_assert_core_reset(ep);
@@ -268,7 +275,7 @@ static int exynos_add_pcie_port(struct exynos_pcie *ep,
return 0;
}
-static const struct dw_pcie_ops dw_pcie_ops = {
+static const struct dw_pcie_ops exynos_dw_pcie_ops = {
.read_dbi = exynos_pcie_read_dbi,
.write_dbi = exynos_pcie_write_dbi,
.link_up = exynos_pcie_link_up,
@@ -279,6 +286,7 @@ static int exynos_pcie_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct exynos_pcie *ep;
+ const struct samsung_pcie_pdata *pdata;
struct device_node *np = dev->of_node;
int ret;
@@ -286,8 +294,11 @@ static int exynos_pcie_probe(struct platform_device *pdev)
if (!ep)
return -ENOMEM;
+ pdata = of_device_get_match_data(dev);
+
+ ep->pdata = pdata;
ep->pci.dev = dev;
- ep->pci.ops = &dw_pcie_ops;
+ ep->pci.ops = pdata->dwc_ops;
ep->phy = devm_of_phy_get(dev, np, NULL);
if (IS_ERR(ep->phy))
@@ -363,9 +374,9 @@ static int exynos_pcie_resume_noirq(struct device *dev)
return ret;
/* exynos_pcie_host_init controls ep->phy */
- exynos_pcie_host_init(pp);
+ ep->pdata->host_ops->init(pp);
dw_pcie_setup_rc(pp);
- exynos_pcie_start_link(pci);
+ ep->pdata->dwc_ops->start_link(pci);
return dw_pcie_wait_for_link(pci);
}
@@ -374,8 +385,17 @@ static const struct dev_pm_ops exynos_pcie_pm_ops = {
exynos_pcie_resume_noirq)
};
+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,
+};
+
static const struct of_device_id exynos_pcie_of_match[] = {
- { .compatible = "samsung,exynos5433-pcie", },
+ {
+ .compatible = "samsung,exynos5433-pcie",
+ .data = (void *) &exynos_5433_pcie_rc_pdata,
+ },
{ },
};
--
2.49.0On Mon, May 19, 2025 at 01:01:46AM GMT, Shradha Todi wrote:
> -static const struct dw_pcie_ops dw_pcie_ops = {
> +static const struct dw_pcie_ops exynos_dw_pcie_ops = {
> .read_dbi = exynos_pcie_read_dbi,
> .write_dbi = exynos_pcie_write_dbi,
> .link_up = exynos_pcie_link_up,
> @@ -279,6 +286,7 @@ static int exynos_pcie_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct exynos_pcie *ep;
> + const struct samsung_pcie_pdata *pdata;
> struct device_node *np = dev->of_node;
> int ret;
>
> @@ -286,8 +294,11 @@ static int exynos_pcie_probe(struct platform_device *pdev)
> if (!ep)
> return -ENOMEM;
>
> + pdata = of_device_get_match_data(dev);
> +
> + ep->pdata = pdata;
> ep->pci.dev = dev;
> - ep->pci.ops = &dw_pcie_ops;
> + ep->pci.ops = pdata->dwc_ops;
>
> ep->phy = devm_of_phy_get(dev, np, NULL);
> if (IS_ERR(ep->phy))
> @@ -363,9 +374,9 @@ static int exynos_pcie_resume_noirq(struct device *dev)
> return ret;
>
> /* exynos_pcie_host_init controls ep->phy */
> - exynos_pcie_host_init(pp);
> + ep->pdata->host_ops->init(pp);
> dw_pcie_setup_rc(pp);
> - exynos_pcie_start_link(pci);
> + ep->pdata->dwc_ops->start_link(pci);
One more layer of indirection.
Read:
https://lore.kernel.org/all/CAL_JsqJgaeOcnUzw+rUF2yO4hQYCdZYssjxHzrDvvHGJimrASA@mail.gmail.com/
Best regards,
Krzysztof
> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 21 May 2025 15:15
> 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.or;
> linux-kernel@vger.kernel.org; linux-phy@lists.infradead.org; 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 <pankaj.dubey@samsung.com>
> Subject: Re: [PATCH 04/10] PCI: exynos: Add platform device private data
>
> On Mon, May 19, 2025 at 01:01:46AM GMT, Shradha Todi wrote:
> > -static const struct dw_pcie_ops dw_pcie_ops = {
> > +static const struct dw_pcie_ops exynos_dw_pcie_ops = {
> > .read_dbi = exynos_pcie_read_dbi,
> > .write_dbi = exynos_pcie_write_dbi,
> > .link_up = exynos_pcie_link_up,
> > @@ -279,6 +286,7 @@ static int exynos_pcie_probe(struct
> > platform_device *pdev) {
> > struct device *dev = &pdev->dev;
> > struct exynos_pcie *ep;
> > + const struct samsung_pcie_pdata *pdata;
> > struct device_node *np = dev->of_node;
> > int ret;
> >
> > @@ -286,8 +294,11 @@ static int exynos_pcie_probe(struct platform_device *pdev)
> > if (!ep)
> > return -ENOMEM;
> >
> > + pdata = of_device_get_match_data(dev);
> > +
> > + ep->pdata = pdata;
> > ep->pci.dev = dev;
> > - ep->pci.ops = &dw_pcie_ops;
> > + ep->pci.ops = pdata->dwc_ops;
> >
> > ep->phy = devm_of_phy_get(dev, np, NULL);
> > if (IS_ERR(ep->phy))
> > @@ -363,9 +374,9 @@ static int exynos_pcie_resume_noirq(struct device *dev)
> > return ret;
> >
> > /* exynos_pcie_host_init controls ep->phy */
> > - exynos_pcie_host_init(pp);
> > + ep->pdata->host_ops->init(pp);
> > dw_pcie_setup_rc(pp);
> > - exynos_pcie_start_link(pci);
> > + ep->pdata->dwc_ops->start_link(pci);
>
> One more layer of indirection.
>
> Read:
> https://lore.kernel.org/all/CAL_JsqJgaeOcnUzw+rUF2yO4hQYCdZYssjxHzrDvvHGJimrASA@mail.gmail.com/
>
I went through this thread and the solution to avoid redirection seems to be:
1. Make the common parts into a library that each driver can call
2. When there is barely anything in common, make a separate driver
From my understanding of these 2 drivers, there is hardly anything that can go into common library
1. host_init, dbi_read, dbi_write these ops have completely different flow
2. link_up, start_link have similar flow but different register offsets
3. write_dbi2 and stop_link is not implemented for exynos but needed for FSD
4. Resources are different - FSD does not have regulator, Exynos5433 does not have syscon, FSD has msi IRQ vs exynos5433 has legacy
5. Exynos is host only whereas FSD is dual mode controller.
I don’t see any other way except redirection, or using lots of if(variant == FSD) which is also discouraged.
And about making it a different driver altogether, I'm completely okay to do so. In fact we had previously tried to post it as a
different driver which was rejected.
If you still feel there is a way to separate out the common parts into a library, please guide me.
> Best regards,
> Krzysztof
On Tue, May 27, 2025 at 04:13:58PM +0530, Shradha Todi wrote:
>
>
> > -----Original Message-----
> > From: Krzysztof Kozlowski <krzk@kernel.org>
> > Sent: 21 May 2025 15:15
> > 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.or;
> > linux-kernel@vger.kernel.org; linux-phy@lists.infradead.org; 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 <pankaj.dubey@samsung.com>
> > Subject: Re: [PATCH 04/10] PCI: exynos: Add platform device private data
> >
> > On Mon, May 19, 2025 at 01:01:46AM GMT, Shradha Todi wrote:
> > > -static const struct dw_pcie_ops dw_pcie_ops = {
> > > +static const struct dw_pcie_ops exynos_dw_pcie_ops = {
> > > .read_dbi = exynos_pcie_read_dbi,
> > > .write_dbi = exynos_pcie_write_dbi,
> > > .link_up = exynos_pcie_link_up,
> > > @@ -279,6 +286,7 @@ static int exynos_pcie_probe(struct
> > > platform_device *pdev) {
> > > struct device *dev = &pdev->dev;
> > > struct exynos_pcie *ep;
> > > + const struct samsung_pcie_pdata *pdata;
> > > struct device_node *np = dev->of_node;
> > > int ret;
> > >
> > > @@ -286,8 +294,11 @@ static int exynos_pcie_probe(struct platform_device *pdev)
> > > if (!ep)
> > > return -ENOMEM;
> > >
> > > + pdata = of_device_get_match_data(dev);
> > > +
> > > + ep->pdata = pdata;
> > > ep->pci.dev = dev;
> > > - ep->pci.ops = &dw_pcie_ops;
> > > + ep->pci.ops = pdata->dwc_ops;
> > >
> > > ep->phy = devm_of_phy_get(dev, np, NULL);
> > > if (IS_ERR(ep->phy))
> > > @@ -363,9 +374,9 @@ static int exynos_pcie_resume_noirq(struct device *dev)
> > > return ret;
> > >
> > > /* exynos_pcie_host_init controls ep->phy */
> > > - exynos_pcie_host_init(pp);
> > > + ep->pdata->host_ops->init(pp);
> > > dw_pcie_setup_rc(pp);
> > > - exynos_pcie_start_link(pci);
> > > + ep->pdata->dwc_ops->start_link(pci);
> >
> > One more layer of indirection.
> >
> > Read:
> > https://lore.kernel.org/all/CAL_JsqJgaeOcnUzw+rUF2yO4hQYCdZYssjxHzrDvvHGJimrASA@mail.gmail.com/
> >
>
> I went through this thread and the solution to avoid redirection seems to be:
> 1. Make the common parts into a library that each driver can call
> 2. When there is barely anything in common, make a separate driver
>
> From my understanding of these 2 drivers, there is hardly anything that can go into common library
> 1. host_init, dbi_read, dbi_write these ops have completely different flow
> 2. link_up, start_link have similar flow but different register offsets
> 3. write_dbi2 and stop_link is not implemented for exynos but needed for FSD
> 4. Resources are different - FSD does not have regulator, Exynos5433 does not have syscon, FSD has msi IRQ vs exynos5433 has legacy
> 5. Exynos is host only whereas FSD is dual mode controller.
>
> I don’t see any other way except redirection, or using lots of if(variant == FSD) which is also discouraged.
Please wrap your replies to 80 columns.
>
IMO it is OK to have the callbacks for cases like this. This is also similar to
Qcom driver where each SoC requires custom register updates and going by the
common library or a new driver wouldn't be feasible.
> And about making it a different driver altogether, I'm completely okay to do so. In fact we had previously tried to post it as a
> different driver which was rejected.
>
No, please do not create a new driver, that is another maintainer burden.
- Mani
--
மணிவண்ணன் சதாசிவம்
© 2016 - 2025 Red Hat, Inc.