| Message ID | 20230214121333.1837-16-shradha.t@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | Refactor Exynos PCIe driver to make it generic | expand |
On 14/02/2023 13:13, Shradha Todi wrote: > Some resources might differ based on platforms and we Please wrap commit message according to Linux coding style / submission process (neither too early nor over the limit): https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586 Wrapping looks a bit short... > need platform specific functions to initialize or alter > them. For better code reusibility, making a separate typo, I think it is: re-usability > res_ops which will hold all such function pointers or > other resource specific data. Are you saying that interrupts differ in different devices? > > This patch includes adding function pointer for IRQ Do not use "This commit/patch". https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > initialization which will help to move common operations for > host init into the probe sequence. > > Suggested-by: Pankaj Dubey <pankaj.dubey@samsung.com> > Signed-off-by: Shradha Todi <shradha.t@samsung.com> > --- > drivers/pci/controller/dwc/pci-samsung.c | 26 ++++++++++++++++-------- > 1 file changed, 17 insertions(+), 9 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-samsung.c b/drivers/pci/controller/dwc/pci-samsung.c > index 47ca2a6a545d..01882f2d06c7 100644 > --- a/drivers/pci/controller/dwc/pci-samsung.c > +++ b/drivers/pci/controller/dwc/pci-samsung.c > @@ -55,6 +55,7 @@ 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 samsung_res_ops *res_ops; > }; > > /* > @@ -77,6 +78,10 @@ struct samsung_pcie { > struct regulator_bulk_data supplies[2]; > }; > > +struct samsung_res_ops { > + int (*irq_init)(struct samsung_pcie *sp, struct platform_device *pdev); > +}; > + > static int samsung_pcie_init_clk_resources(struct samsung_pcie *sp) > { > struct device *dev = sp->pci.dev; > @@ -276,7 +281,7 @@ static const struct dw_pcie_host_ops exynos_pcie_host_ops = { > .host_init = exynos_pcie_host_init, > }; > > -static int exynos_add_pcie_port(struct samsung_pcie *sp, > +static int exynos_irq_init(struct samsung_pcie *sp, > struct platform_device *pdev) > { > struct dw_pcie *pci = &sp->pci; > @@ -295,15 +300,8 @@ static int exynos_add_pcie_port(struct samsung_pcie *sp, > return ret; > } > > - pp->ops = &exynos_pcie_host_ops; > pp->msi_irq[0] = -ENODEV; > > - ret = dw_pcie_host_init(pp); > - if (ret) { > - dev_err(dev, "failed to initialize host\n"); > - return ret; > - } > - > return 0; > } > > @@ -314,6 +312,10 @@ static const struct dw_pcie_ops exynos_dw_pcie_ops = { > .start_link = exynos_pcie_start_link, > }; > > +static const struct samsung_res_ops exynos_res_ops_data = { > + .irq_init = exynos_irq_init, > +}; > + > static int samsung_pcie_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -357,7 +359,12 @@ static int samsung_pcie_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, sp); > > - ret = exynos_add_pcie_port(sp, pdev); > + if (pdata->res_ops->irq_init) > + pdata->res_ops->irq_init(sp, pdev); Check return value and handle errors. > + > + sp->pci.pp.ops = pdata->host_ops; > + > + ret = dw_pcie_host_init(&sp->pci.pp); > if (ret < 0) > goto fail_probe; > > @@ -428,6 +435,7 @@ 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, > }; > > static const struct of_device_id samsung_pcie_of_match[] = { Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org] > Sent: 16 February 2023 16:42 > To: Shradha Todi <shradha.t@samsung.com>; lpieralisi@kernel.org; > kw@linux.com; robh@kernel.org; bhelgaas@google.com; > krzysztof.kozlowski+dt@linaro.org; alim.akhtar@samsung.com; > jingoohan1@gmail.com; Sergey.Semin@baikalelectronics.ru; > lukas.bulwahn@gmail.com; hongxing.zhu@nxp.com; tglx@linutronix.de; > m.szyprowski@samsung.com; jh80.chung@samsung.co; > pankaj.dubey@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 > Subject: Re: [PATCH 15/16] PCI: samsung: Add structure to hold resource > operations > > On 14/02/2023 13:13, Shradha Todi wrote: > > Some resources might differ based on platforms and we > > Please wrap commit message according to Linux coding style / submission > process (neither too early nor over the limit): > https://protect2.fireeye.com/v1/url?k=66656d8a-07ee78a5-6664e6c5- > 74fe485cbfe7-a61191c61bcf38f7&q=1&e=80994c2d-d0ca-4b83-a7ca- > 5242c4bb701f&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.18- > rc4%2Fsource%2FDocumentation%2Fprocess%2Fsubmitting- > patches.rst%23L586 > > Wrapping looks a bit short... Ack > > > need platform specific functions to initialize or alter them. For > > better code reusibility, making a separate > > typo, I think it is: re-usability Ack > > > res_ops which will hold all such function pointers or other resource > > specific data. > > Are you saying that interrupts differ in different devices? > Yes, the interrupts are routed and integrated differently for the different platforms > > > > This patch includes adding function pointer for IRQ > > Do not use "This commit/patch". > https://protect2.fireeye.com/v1/url?k=ffdc2502-9e57302d-ffddae4d- > 74fe485cbfe7-49aeaacd1141660f&q=1&e=80994c2d-d0ca-4b83-a7ca- > 5242c4bb701f&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.17.1%2 > Fsource%2FDocumentation%2Fprocess%2Fsubmitting-patches.rst%23L95 > Ack > > initialization which will help to move common operations for host init > > into the probe sequence. > > > > Suggested-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > Signed-off-by: Shradha Todi <shradha.t@samsung.com> > > --- > > drivers/pci/controller/dwc/pci-samsung.c | 26 > > ++++++++++++++++-------- > > 1 file changed, 17 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-samsung.c > > b/drivers/pci/controller/dwc/pci-samsung.c > > index 47ca2a6a545d..01882f2d06c7 100644 > > --- a/drivers/pci/controller/dwc/pci-samsung.c > > +++ b/drivers/pci/controller/dwc/pci-samsung.c > > @@ -55,6 +55,7 @@ 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 samsung_res_ops *res_ops; > > }; > > > > /* > > @@ -77,6 +78,10 @@ struct samsung_pcie { > > struct regulator_bulk_data supplies[2]; > > }; > > > > +struct samsung_res_ops { > > + int (*irq_init)(struct samsung_pcie *sp, struct platform_device > > +*pdev); }; > > + > > static int samsung_pcie_init_clk_resources(struct samsung_pcie *sp) > > { > > struct device *dev = sp->pci.dev; > > @@ -276,7 +281,7 @@ static const struct dw_pcie_host_ops > exynos_pcie_host_ops = { > > .host_init = exynos_pcie_host_init, > > }; > > > > -static int exynos_add_pcie_port(struct samsung_pcie *sp, > > +static int exynos_irq_init(struct samsung_pcie *sp, > > struct platform_device *pdev) { > > struct dw_pcie *pci = &sp->pci; > > @@ -295,15 +300,8 @@ static int exynos_add_pcie_port(struct > samsung_pcie *sp, > > return ret; > > } > > > > - pp->ops = &exynos_pcie_host_ops; > > pp->msi_irq[0] = -ENODEV; > > > > - ret = dw_pcie_host_init(pp); > > - if (ret) { > > - dev_err(dev, "failed to initialize host\n"); > > - return ret; > > - } > > - > > return 0; > > } > > > > @@ -314,6 +312,10 @@ static const struct dw_pcie_ops > exynos_dw_pcie_ops = { > > .start_link = exynos_pcie_start_link, }; > > > > +static const struct samsung_res_ops exynos_res_ops_data = { > > + .irq_init = exynos_irq_init, > > +}; > > + > > static int samsung_pcie_probe(struct platform_device *pdev) { > > struct device *dev = &pdev->dev; > > @@ -357,7 +359,12 @@ static int samsung_pcie_probe(struct > > platform_device *pdev) > > > > platform_set_drvdata(pdev, sp); > > > > - ret = exynos_add_pcie_port(sp, pdev); > > + if (pdata->res_ops->irq_init) > > + pdata->res_ops->irq_init(sp, pdev); > > Check return value and handle errors. > Ack > > + > > + sp->pci.pp.ops = pdata->host_ops; > > + > > + ret = dw_pcie_host_init(&sp->pci.pp); > > if (ret < 0) > > goto fail_probe; > > > > @@ -428,6 +435,7 @@ 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, > > }; > > > > static const struct of_device_id samsung_pcie_of_match[] = { > > Best regards, > Krzysztof
diff --git a/drivers/pci/controller/dwc/pci-samsung.c b/drivers/pci/controller/dwc/pci-samsung.c index 47ca2a6a545d..01882f2d06c7 100644 --- a/drivers/pci/controller/dwc/pci-samsung.c +++ b/drivers/pci/controller/dwc/pci-samsung.c @@ -55,6 +55,7 @@ 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 samsung_res_ops *res_ops; }; /* @@ -77,6 +78,10 @@ struct samsung_pcie { struct regulator_bulk_data supplies[2]; }; +struct samsung_res_ops { + int (*irq_init)(struct samsung_pcie *sp, struct platform_device *pdev); +}; + static int samsung_pcie_init_clk_resources(struct samsung_pcie *sp) { struct device *dev = sp->pci.dev; @@ -276,7 +281,7 @@ static const struct dw_pcie_host_ops exynos_pcie_host_ops = { .host_init = exynos_pcie_host_init, }; -static int exynos_add_pcie_port(struct samsung_pcie *sp, +static int exynos_irq_init(struct samsung_pcie *sp, struct platform_device *pdev) { struct dw_pcie *pci = &sp->pci; @@ -295,15 +300,8 @@ static int exynos_add_pcie_port(struct samsung_pcie *sp, return ret; } - pp->ops = &exynos_pcie_host_ops; pp->msi_irq[0] = -ENODEV; - ret = dw_pcie_host_init(pp); - if (ret) { - dev_err(dev, "failed to initialize host\n"); - return ret; - } - return 0; } @@ -314,6 +312,10 @@ static const struct dw_pcie_ops exynos_dw_pcie_ops = { .start_link = exynos_pcie_start_link, }; +static const struct samsung_res_ops exynos_res_ops_data = { + .irq_init = exynos_irq_init, +}; + static int samsung_pcie_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -357,7 +359,12 @@ static int samsung_pcie_probe(struct platform_device *pdev) platform_set_drvdata(pdev, sp); - ret = exynos_add_pcie_port(sp, pdev); + if (pdata->res_ops->irq_init) + pdata->res_ops->irq_init(sp, pdev); + + sp->pci.pp.ops = pdata->host_ops; + + ret = dw_pcie_host_init(&sp->pci.pp); if (ret < 0) goto fail_probe; @@ -428,6 +435,7 @@ 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, }; static const struct of_device_id samsung_pcie_of_match[] = {
Some resources might differ based on platforms and we need platform specific functions to initialize or alter them. For better code reusibility, making a separate res_ops which will hold all such function pointers or other resource specific data. This patch includes adding function pointer for IRQ initialization which will help to move common operations for host init into the probe sequence. Suggested-by: Pankaj Dubey <pankaj.dubey@samsung.com> Signed-off-by: Shradha Todi <shradha.t@samsung.com> --- drivers/pci/controller/dwc/pci-samsung.c | 26 ++++++++++++++++-------- 1 file changed, 17 insertions(+), 9 deletions(-)