| Message ID | 20230214121333.1837-1-shradha.t@samsung.com |
|---|---|
| Headers | show |
| Series | Refactor Exynos PCIe driver to make it generic | expand |
On 14/02/2023 13:13, Shradha Todi wrote: > The current PCIe controller driver is being used for Exynos5433 > SoC only. In order to extend this driver for all SoCs manufactured > by Samsung using DWC PCIe controller, rename this driver and make > it Samsung specific instead of any Samsung SoC name. > > Signed-off-by: Shradha Todi <shradha.t@samsung.com> > --- > MAINTAINERS | 4 +- > drivers/pci/controller/dwc/Kconfig | 6 +- > drivers/pci/controller/dwc/Makefile | 2 +- > drivers/pci/controller/dwc/pci-samsung.c | 443 +++++++++++++++++++++++ Rename missing. I am anyway not sure if this is good. What's wrong with old name? Best regards, Krzysztof
On 14/02/2023 13:13, Shradha Todi wrote: > Currently pci-exynos is being used as a PCIe driver for Exynos5433 > only. This patch set refactors the driver to make it extensible to > other Samsung manufactured SoCs having DWC PCIe controllers. > The major change points are: > - Renaming all common functions/structures to use "samsung" instead > of "exynos". Make common probe/remove/suspend/resume > - Making clock/regulator get/enable/disable generic > - Adding private struct to hold platform specific function ops Thanks for the work. I appreciate it. Some comments in individual patches. Best regards, Krzysztof
On 14/02/2023 13:13, Shradha Todi wrote: > Since the macros being used in samsung file are for exynos > only, renaming it to be more specific. > > Signed-off-by: Shradha Todi <shradha.t@samsung.com> > Suggested-by: Pankaj Dubey <pankaj.dubey@samsung.com> > --- > drivers/pci/controller/dwc/pci-samsung.c | 116 +++++++++++------------ > 1 file changed, 58 insertions(+), 58 deletions(-) Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 16/02/2023 11:55, Krzysztof Kozlowski wrote: > On 14/02/2023 13:13, Shradha Todi wrote: >> The current PCIe controller driver is being used for Exynos5433 >> SoC only. In order to extend this driver for all SoCs manufactured >> by Samsung using DWC PCIe controller, rename this driver and make >> it Samsung specific instead of any Samsung SoC name. >> >> Signed-off-by: Shradha Todi <shradha.t@samsung.com> >> --- >> MAINTAINERS | 4 +- >> drivers/pci/controller/dwc/Kconfig | 6 +- >> drivers/pci/controller/dwc/Makefile | 2 +- >> drivers/pci/controller/dwc/pci-samsung.c | 443 +++++++++++++++++++++++ > > Rename missing. I am anyway not sure if this is good. What's wrong with > old name? OK, looking a bit at your further patches - doesn't it make sense to split a bit the driver? Maybe keep the core as pci-samsung, but some other parts in pci-exynso5433? Best regards, Krzysztof
On 14/02/2023 13:13, Shradha Todi wrote: > Adopt to clock bulk API to handle clocks. > > Signed-off-by: Shradha Todi <shradha.t@samsung.com> > --- > drivers/pci/controller/dwc/pci-samsung.c | 46 ++++++------------------ > 1 file changed, 11 insertions(+), 35 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-samsung.c b/drivers/pci/controller/dwc/pci-samsung.c > index cfe384aee754..6c07d3f151be 100644 > --- a/drivers/pci/controller/dwc/pci-samsung.c > +++ b/drivers/pci/controller/dwc/pci-samsung.c > @@ -54,8 +54,8 @@ > struct exynos_pcie { > struct dw_pcie pci; > void __iomem *elbi_base; > - struct clk *clk; > - struct clk *bus_clk; > + struct clk_bulk_data *clks; > + int clk_cnt; > struct phy *phy; > struct regulator_bulk_data supplies[2]; > }; > @@ -65,30 +65,18 @@ static int exynos_pcie_init_clk_resources(struct exynos_pcie *ep) > struct device *dev = ep->pci.dev; > int ret; > > - ret = clk_prepare_enable(ep->clk); > - if (ret) { > - dev_err(dev, "cannot enable pcie rc clock"); > + ret = devm_clk_bulk_get_all(dev, &ep->clks); > + if (ret < 0) > return ret; > - } > > - ret = clk_prepare_enable(ep->bus_clk); > - if (ret) { > - dev_err(dev, "cannot enable pcie bus clock"); > - goto err_bus_clk; > - } > + ep->clk_cnt = ret; I think this misses check if you got two clocks. Best regards, Krzysztof
On 14/02/2023 13:13, Shradha Todi wrote: > DT uses the name elbi in reg-names for application logic > registers which is a wrong nomenclature. This patch fixes > the same. > > This commit shouldn't be applied without changes > "dt-bindings: PCI: Rename the term elbi to appl" and > "PCI: samsung: Rename the term elbi to appl" Dependencies and patch ordering goes after '---', because there is no point to store it in git history. Anyway, that's an ABI break and Exynos5433 is quite stable, so without clear indication of fixed bug, we should not do this. Best regards, Krzysztof
On 14/02/2023 13:13, Shradha Todi wrote: > The platform specific structure being used is named > exynos_pcie. Changing it to samsung_pcie for making it > generic. > > Suggested-by: Pankaj Dubey <pankaj.dubey@samsung.com> > Signed-off-by: Shradha Todi <shradha.t@samsung.com> > --- > drivers/pci/controller/dwc/pci-samsung.c | 190 +++++++++++------------ > 1 file changed, 95 insertions(+), 95 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-samsung.c b/drivers/pci/controller/dwc/pci-samsung.c > index d5adf1017a05..be0177fcd763 100644 > --- a/drivers/pci/controller/dwc/pci-samsung.c > +++ b/drivers/pci/controller/dwc/pci-samsung.c > @@ -23,7 +23,7 @@ > > #include "pcie-designware.h" > > -#define to_exynos_pcie(x) dev_get_drvdata((x)->dev) > +#define to_samsung_pcie(x) dev_get_drvdata((x)->dev) > > /* PCIe APPL registers */ > #define EXYNOS_PCIE_IRQ_PULSE 0x000 > @@ -51,7 +51,7 @@ > #define EXYNOS_PCIE_APPL_SLV_ARMISC 0x120 > #define EXYNOS_PCIE_APPL_SLV_DBI_ENABLE BIT(21) > > -struct exynos_pcie { > +struct samsung_pcie { No, I don't see benefit of this at all. How we call stuff inside driver is not related whether this is for Tesla or Exynos. We could even call it "pony". :) Thus renamings just to support new variant of Samsung device is not a good reason. Unless all of the old "exynos" names will be soon needed for some exynos-specific variants? Best regards, Krzysztof
On 14/02/2023 13:13, Shradha Todi wrote: > Common application logic register read and write functions > used for better readability. > > Signed-off-by: Shradha Todi <shradha.t@samsung.com> > --- > drivers/pci/controller/dwc/pci-samsung.c | 54 ++++++++++++------------ > 1 file changed, 27 insertions(+), 27 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-samsung.c b/drivers/pci/controller/dwc/pci-samsung.c > index be0177fcd763..e6e2a8ab4403 100644 > --- a/drivers/pci/controller/dwc/pci-samsung.c > +++ b/drivers/pci/controller/dwc/pci-samsung.c > @@ -79,63 +79,63 @@ static void exynos_pcie_deinit_clk_resources(struct samsung_pcie *sp) > clk_bulk_disable_unprepare(sp->clk_cnt, sp->clks); > } > > -static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg) > +static void samsung_pcie_appl_writel(struct samsung_pcie *sp, u32 val, u32 reg) No for renaming - same reason as for previous patch. Best regards, Krzysztof
On 14/02/2023 13:13, Shradha Todi wrote: > Use samsung instead of exynos for all common functions > like probe/remove/suspend/resume. > > Suggested-by: Pankaj Dubey <pankaj.dubey@samsung.com> > Signed-off-by: Shradha Todi <shradha.t@samsung.com> > --- > drivers/pci/controller/dwc/pci-samsung.c | 42 ++++++++++++------------ > 1 file changed, 21 insertions(+), 21 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-samsung.c b/drivers/pci/controller/dwc/pci-samsung.c > index 719d284e1552..dc8ec0b546fd 100644 > --- a/drivers/pci/controller/dwc/pci-samsung.c > +++ b/drivers/pci/controller/dwc/pci-samsung.c > @@ -60,7 +60,7 @@ struct samsung_pcie { > struct regulator_bulk_data supplies[2]; > }; > > -static int exynos_pcie_init_clk_resources(struct samsung_pcie *sp) > +static int samsung_pcie_init_clk_resources(struct samsung_pcie *sp) Same as before - I don't see here benefit. > { > struct device *dev = sp->pci.dev; > int ret; > @@ -74,7 +74,7 @@ static int exynos_pcie_init_clk_resources(struct samsung_pcie *sp) > return clk_bulk_prepare_enable(sp->clk_cnt, sp->clks); > } > (...) > > -static struct platform_driver exynos_pcie_driver = { > - .probe = exynos_pcie_probe, > - .remove = __exit_p(exynos_pcie_remove), > +static struct platform_driver samsung_pcie_driver = { > + .probe = samsung_pcie_probe, > + .remove = __exit_p(samsung_pcie_remove), > .driver = { > - .name = "exynos-pcie", > - .of_match_table = exynos_pcie_of_match, > - .pm = &exynos_pcie_pm_ops, > + .name = "samsung-pcie", This "name" has some point... but I think it would break now all module users. > + .of_match_table = samsung_pcie_of_match, > + .pm = &samsung_pcie_pm_ops, > }, > }; > -module_platform_driver(exynos_pcie_driver); > +module_platform_driver(samsung_pcie_driver); > MODULE_LICENSE("GPL v2"); > -MODULE_DEVICE_TABLE(of, exynos_pcie_of_match); > +MODULE_DEVICE_TABLE(of, samsung_pcie_of_match); Best regards, Krzysztof
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 <krzysztof.kozlowski@linaro.org> > Sent: Thursday, February 16, 2023 4:37 PM > 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 08/16] PCI: samsung: Rename exynos_pcie to > samsung_pcie > > On 14/02/2023 13:13, Shradha Todi wrote: > > The platform specific structure being used is named exynos_pcie. > > Changing it to samsung_pcie for making it generic. > > > > Suggested-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > Signed-off-by: Shradha Todi <shradha.t@samsung.com> > > --- > > drivers/pci/controller/dwc/pci-samsung.c | 190 > > +++++++++++------------ > > 1 file changed, 95 insertions(+), 95 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-samsung.c > > b/drivers/pci/controller/dwc/pci-samsung.c > > index d5adf1017a05..be0177fcd763 100644 > > --- a/drivers/pci/controller/dwc/pci-samsung.c > > +++ b/drivers/pci/controller/dwc/pci-samsung.c > > @@ -23,7 +23,7 @@ > > > > #include "pcie-designware.h" > > > > -#define to_exynos_pcie(x) dev_get_drvdata((x)->dev) > > +#define to_samsung_pcie(x) dev_get_drvdata((x)->dev) > > > > /* PCIe APPL registers */ > > #define EXYNOS_PCIE_IRQ_PULSE 0x000 > > @@ -51,7 +51,7 @@ > > #define EXYNOS_PCIE_APPL_SLV_ARMISC 0x120 > > #define EXYNOS_PCIE_APPL_SLV_DBI_ENABLE BIT(21) > > > > -struct exynos_pcie { > > +struct samsung_pcie { > > No, I don't see benefit of this at all. How we call stuff inside driver is not related > whether this is for Tesla or Exynos. We could even call it "pony". :) Thus > renamings just to support new variant of Samsung device is not a good reason. > Whole intention of this whole series was to make exynos-pcie driver to support for all Samsung manufactured SoCs be it Exynos series or custom ASIC such as fsd, artpect-v8. While doing so, we feel for better readability and conveying better names for files, structs, internal APIs will help developers for understanding and reusing it. For example we know that clock initialization will remain common (thanks for bulk_clk_xxx APIs) so we kept APIs for handling clocks starting with samsung_clk_xxxx, but if we have to implement two variant of APIs, or struct targeting different platforms it would be good if they have platform specific prefixes. This will help in grep or future code maintenance. Though technically all these can be done even without renaming, but if we see no impact as such, so why not use better names? > Unless all of the old "exynos" names will be soon needed for some exynos- > specific variants? > No we don't have any such plans. > Best regards, > Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Thursday, February 16, 2023 4:38 PM > 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 09/16] PCI: samsung: Make common appl readl/writel > functions > > On 14/02/2023 13:13, Shradha Todi wrote: > > Common application logic register read and write functions used for > > better readability. > > > > Signed-off-by: Shradha Todi <shradha.t@samsung.com> > > --- > > drivers/pci/controller/dwc/pci-samsung.c | 54 > > ++++++++++++------------ > > 1 file changed, 27 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-samsung.c > > b/drivers/pci/controller/dwc/pci-samsung.c > > index be0177fcd763..e6e2a8ab4403 100644 > > --- a/drivers/pci/controller/dwc/pci-samsung.c > > +++ b/drivers/pci/controller/dwc/pci-samsung.c > > @@ -79,63 +79,63 @@ static void exynos_pcie_deinit_clk_resources(struct > samsung_pcie *sp) > > clk_bulk_disable_unprepare(sp->clk_cnt, sp->clks); } > > > > -static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg) > > +static void samsung_pcie_appl_writel(struct samsung_pcie *sp, u32 > > +val, u32 reg) > > No for renaming - same reason as for previous patch. > I have tried to justify our rational behind this in previous patch, I hope that makes sense. > > Best regards, > Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org] > Sent: 16 February 2023 16:29 > 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 02/16] PCI: exynos: Rename Exynos PCIe driver to > Samsung PCIe > > On 16/02/2023 11:55, Krzysztof Kozlowski wrote: > > On 14/02/2023 13:13, Shradha Todi wrote: > >> The current PCIe controller driver is being used for Exynos5433 SoC > >> only. In order to extend this driver for all SoCs manufactured by > >> Samsung using DWC PCIe controller, rename this driver and make it > >> Samsung specific instead of any Samsung SoC name. > >> > >> Signed-off-by: Shradha Todi <shradha.t@samsung.com> > >> --- > >> MAINTAINERS | 4 +- > >> drivers/pci/controller/dwc/Kconfig | 6 +- > >> drivers/pci/controller/dwc/Makefile | 2 +- > >> drivers/pci/controller/dwc/pci-samsung.c | 443 > >> +++++++++++++++++++++++ > > > > Rename missing. I am anyway not sure if this is good. What's wrong > > with old name? > > OK, looking a bit at your further patches - doesn't it make sense to split a bit > the driver? Maybe keep the core as pci-samsung, but some other parts in > pci-exynso5433? > Ok agreed. So here is what I am planning, keeping in mind the next set of platform support which I am planning to send out (say FSD, ARTPEC-v8): 1: We will move samsung pci driver inside dwc/samsung/ 2: pci-samsung.c shall contain common APIs, helper functions, etc 3: Platform specific driver will have their own files such as pcie-exynos.c, pcie-fsd.c, pcie-artpec-v8.c Let me know what you think of this. I am not very keen on renaming Exynos SoC file as pcie-exyons5433.c as in future we may end up adding PCIe support for other Exynos which being in same family (Exynos Series) will be very similar in design. Custom ASIC (manufactured by Samsung Foundry) is primarily driven by various vendors and will have separate design in terms of integration of IPs in SoC and we need to have support for all such SoCs manufactured under Samsung umbrella. Shradha > Best regards, > Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org] > Sent: 16 February 2023 16:33 > 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 04/16] PCI: samsung: Use clock bulk API to get clocks > > On 14/02/2023 13:13, Shradha Todi wrote: > > Adopt to clock bulk API to handle clocks. > > > > Signed-off-by: Shradha Todi <shradha.t@samsung.com> > > --- > > drivers/pci/controller/dwc/pci-samsung.c | 46 ++++++------------------ > > 1 file changed, 11 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-samsung.c > b/drivers/pci/controller/dwc/pci-samsung.c > > index cfe384aee754..6c07d3f151be 100644 > > --- a/drivers/pci/controller/dwc/pci-samsung.c > > +++ b/drivers/pci/controller/dwc/pci-samsung.c > > @@ -54,8 +54,8 @@ > > struct exynos_pcie { > > struct dw_pcie pci; > > void __iomem *elbi_base; > > - struct clk *clk; > > - struct clk *bus_clk; > > + struct clk_bulk_data *clks; > > + int clk_cnt; > > struct phy *phy; > > struct regulator_bulk_data supplies[2]; > > }; > > @@ -65,30 +65,18 @@ static int exynos_pcie_init_clk_resources(struct > exynos_pcie *ep) > > struct device *dev = ep->pci.dev; > > int ret; > > > > - ret = clk_prepare_enable(ep->clk); > > - if (ret) { > > - dev_err(dev, "cannot enable pcie rc clock"); > > + ret = devm_clk_bulk_get_all(dev, &ep->clks); > > + if (ret < 0) > > return ret; > > - } > > > > - ret = clk_prepare_enable(ep->bus_clk); > > - if (ret) { > > - dev_err(dev, "cannot enable pcie bus clock"); > > - goto err_bus_clk; > > - } > > + ep->clk_cnt = ret; > > I think this misses check if you got two clocks. > > Got it! Thanks for pointing out. Will add the check in the next version > Best regards, > Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org] > Sent: 16 February 2023 16:34 > 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 06/16] arm64: dts: exynos: Rename the term elbi to appl > > On 14/02/2023 13:13, Shradha Todi wrote: > > DT uses the name elbi in reg-names for application logic registers > > which is a wrong nomenclature. This patch fixes the same. > > > > This commit shouldn't be applied without changes > > "dt-bindings: PCI: Rename the term elbi to appl" and > > "PCI: samsung: Rename the term elbi to appl" > > Dependencies and patch ordering goes after '---', because there is no point > to store it in git history. > Understood will take care in next set of patches. > Anyway, that's an ABI break and Exynos5433 is quite stable, so without clear > indication of fixed bug, we should not do this. > We have strong technical reason to do so. As per DWC PCIe UM, ELBI delivers an inbound register RD/WR received by the controller to external application registers when the controller is expected to generate the PCIe completion of this register RD/WR. In this driver register space which is currently marked as ELBI, is not used for this purpose (Not sure why original author has named this set of registers as ELBI) So to keep this technically correct, it should be marked as application specific wrapper register space. We used name as "appl" taking reference from intel-gw-pcie.yaml's similar register space named as "app", whereas in nvidia,tegra194-pcie.yaml it's named "appl". So our argument is if a future Samsung manufactured SoC having DWC PCIe controller comes with support of real ELBI interface, we need to use the name elbi. We know such SoC exists but they are not yet upstreamed. Ready to adopt the best possible suggested method to make this happen but I really think the name ELBI is misleading. > > 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
On 02/03/2023 13:32, Pankaj Dubey wrote: > > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Thursday, February 16, 2023 4:37 PM >> 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 08/16] PCI: samsung: Rename exynos_pcie to >> samsung_pcie >> >> On 14/02/2023 13:13, Shradha Todi wrote: >>> The platform specific structure being used is named exynos_pcie. >>> Changing it to samsung_pcie for making it generic. >>> >>> Suggested-by: Pankaj Dubey <pankaj.dubey@samsung.com> >>> Signed-off-by: Shradha Todi <shradha.t@samsung.com> >>> --- >>> drivers/pci/controller/dwc/pci-samsung.c | 190 >>> +++++++++++------------ >>> 1 file changed, 95 insertions(+), 95 deletions(-) >>> >>> diff --git a/drivers/pci/controller/dwc/pci-samsung.c >>> b/drivers/pci/controller/dwc/pci-samsung.c >>> index d5adf1017a05..be0177fcd763 100644 >>> --- a/drivers/pci/controller/dwc/pci-samsung.c >>> +++ b/drivers/pci/controller/dwc/pci-samsung.c >>> @@ -23,7 +23,7 @@ >>> >>> #include "pcie-designware.h" >>> >>> -#define to_exynos_pcie(x) dev_get_drvdata((x)->dev) >>> +#define to_samsung_pcie(x) dev_get_drvdata((x)->dev) >>> >>> /* PCIe APPL registers */ >>> #define EXYNOS_PCIE_IRQ_PULSE 0x000 >>> @@ -51,7 +51,7 @@ >>> #define EXYNOS_PCIE_APPL_SLV_ARMISC 0x120 >>> #define EXYNOS_PCIE_APPL_SLV_DBI_ENABLE BIT(21) >>> >>> -struct exynos_pcie { >>> +struct samsung_pcie { >> >> No, I don't see benefit of this at all. How we call stuff inside driver is not related >> whether this is for Tesla or Exynos. We could even call it "pony". :) Thus >> renamings just to support new variant of Samsung device is not a good reason. >> > Whole intention of this whole series was to make exynos-pcie driver to support for all Samsung manufactured SoCs be it Exynos series or custom ASIC such as fsd, artpect-v8. But the patches does not do it, at least mostly. It only renames which does not bring any support... what's more, such renames without actual context - support for the new devices - is a bit pointless. > > While doing so, we feel for better readability and conveying better names for files, structs, internal APIs will help developers for understanding and reusing it. For example we know that clock initialization will remain common (thanks for bulk_clk_xxx APIs) so we kept APIs for handling clocks starting with samsung_clk_xxxx, but if we have to implement two variant of APIs, or struct targeting different platforms it would be good if they have platform specific prefixes. This will help in grep or future code maintenance. Without context it's impossible to judge whether this makes any sense. Best regards, Krzysztof
On 02/03/2023 13:57, Shradha Todi wrote: > > >> -----Original Message----- >> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org] >> Sent: 16 February 2023 16:29 >> 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 02/16] PCI: exynos: Rename Exynos PCIe driver to >> Samsung PCIe >> >> On 16/02/2023 11:55, Krzysztof Kozlowski wrote: >>> On 14/02/2023 13:13, Shradha Todi wrote: >>>> The current PCIe controller driver is being used for Exynos5433 SoC >>>> only. In order to extend this driver for all SoCs manufactured by >>>> Samsung using DWC PCIe controller, rename this driver and make it >>>> Samsung specific instead of any Samsung SoC name. >>>> >>>> Signed-off-by: Shradha Todi <shradha.t@samsung.com> >>>> --- >>>> MAINTAINERS | 4 +- >>>> drivers/pci/controller/dwc/Kconfig | 6 +- >>>> drivers/pci/controller/dwc/Makefile | 2 +- >>>> drivers/pci/controller/dwc/pci-samsung.c | 443 >>>> +++++++++++++++++++++++ >>> >>> Rename missing. I am anyway not sure if this is good. What's wrong >>> with old name? >> >> OK, looking a bit at your further patches - doesn't it make sense to split a bit >> the driver? Maybe keep the core as pci-samsung, but some other parts in >> pci-exynso5433? >> > > Ok agreed. So here is what I am planning, keeping in mind the next set of platform support which I am planning to send out (say FSD, ARTPEC-v8): > 1: We will move samsung pci driver inside dwc/samsung/ I don't think we need one more directory... > 2: pci-samsung.c shall contain common APIs, helper functions, etc > 3: Platform specific driver will have their own files such as pcie-exynos.c, pcie-fsd.c, pcie-artpec-v8.c This sounds reasonable, although depends whether common driver part is more or less common. If it is more common, then you will need only one pci_driver and it should be in common object. > Let me know what you think of this. > I am not very keen on renaming Exynos SoC file as pcie-exyons5433.c as in future we may end up adding PCIe support for other Exynos which being > in same family (Exynos Series) will be very similar in design. Custom ASIC (manufactured by Samsung Foundry) is primarily driven by various > vendors and will have separate design in terms of integration of IPs in SoC and we need to have support for all such SoCs manufactured under Samsung umbrella. Best regards, Krzysztof
On 02/03/2023 14:07, Shradha Todi wrote: > > >> -----Original Message----- >> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org] >> Sent: 16 February 2023 16:34 >> 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 06/16] arm64: dts: exynos: Rename the term elbi to appl >> >> On 14/02/2023 13:13, Shradha Todi wrote: >>> DT uses the name elbi in reg-names for application logic registers >>> which is a wrong nomenclature. This patch fixes the same. >>> >>> This commit shouldn't be applied without changes >>> "dt-bindings: PCI: Rename the term elbi to appl" and >>> "PCI: samsung: Rename the term elbi to appl" >> >> Dependencies and patch ordering goes after '---', because there is no point >> to store it in git history. >> > > Understood will take care in next set of patches. > >> Anyway, that's an ABI break and Exynos5433 is quite stable, so without clear >> indication of fixed bug, we should not do this. >> > > We have strong technical reason to do so. > > As per DWC PCIe UM, ELBI delivers an inbound register RD/WR received by the controller to external application registers when the controller > is expected to generate the PCIe completion of this register RD/WR. > In this driver register space which is currently marked as ELBI, is not used for this purpose (Not sure why original author has named this set of registers as ELBI) > So to keep this technically correct, it should be marked as application specific wrapper register space. > We used name as "appl" taking reference from intel-gw-pcie.yaml's similar register space named as "app", whereas in nvidia,tegra194-pcie.yaml it's named "appl". > > So our argument is if a future Samsung manufactured SoC having DWC PCIe controller comes with support of real ELBI interface, we need to use the name elbi. > We know such SoC exists but they are not yet upstreamed. > > Ready to adopt the best possible suggested method to make this happen but I really think the name ELBI is misleading. All this is rather reason for a future case. What is the problem experienced now? Best regards, Krzysztof
Hi Shradha On Tue, Feb 14, 2023 at 05:43:17PM +0530, Shradha Todi wrote: > Currently pci-exynos is being used as a PCIe driver for Exynos5433 > only. This patch set refactors the driver to make it extensible to > other Samsung manufactured SoCs having DWC PCIe controllers. > The major change points are: > - Renaming all common functions/structures to use "samsung" instead > of "exynos". Make common probe/remove/suspend/resume > - Making clock/regulator get/enable/disable generic > - Adding private struct to hold platform specific function ops Just a general note regarding the DT-bindings. If you're willing to fix some names or most importantly add new ones please follow as much as possible to the generic interface defined in the common part of the DW PCIe bindings schema: Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml for instance the generic "reg-names" are "elbi" or "app" defined for the application-dependent registers map (normally implemented via the ELBI interface in hardware), the "appl" name is marked as vendor-specific and should be avoided. -Serge(y) > > Shradha Todi (16): > dt-bindings: PCI: Rename Exynos PCIe binding to Samsung PCIe > PCI: exynos: Rename Exynos PCIe driver to Samsung PCIe > PCI: samsung: Change macro names to exynos specific > PCI: samsung: Use clock bulk API to get clocks > dt-bindings: PCI: Rename the term elbi to appl > arm64: dts: exynos: Rename the term elbi to appl > PCI: samsung: Rename the term elbi to appl > PCI: samsung: Rename exynos_pcie to samsung_pcie > PCI: samsung: Make common appl readl/writel functions > dt-bindings: PCI: Add phy-names as required property > arm64: dts: exynos: Add phy-names as DT property > PCI: samsung: Get PHY using non-DT version > PCI: samsung: Rename common functions to samsung > PCI: samsung: Add platform device private data > PCI: samsung: Add structure to hold resource operations > PCI: samsung: Make handling of regulators generic > > ...ung,exynos-pcie.yaml => samsung,pcie.yaml} | 15 +- > MAINTAINERS | 4 +- > arch/arm64/boot/dts/exynos/exynos5433.dtsi | 3 +- > drivers/pci/controller/dwc/Kconfig | 6 +- > drivers/pci/controller/dwc/Makefile | 2 +- > drivers/pci/controller/dwc/pci-samsung.c | 508 ++++++++++++++++++ > 6 files changed, 526 insertions(+), 12 deletions(-) > rename Documentation/devicetree/bindings/pci/{samsung,exynos-pcie.yaml => samsung,pcie.yaml} (89%) > create mode 100644 drivers/pci/controller/dwc/pci-samsung.c > > -- > 2.17.1 > >
Currently pci-exynos is being used as a PCIe driver for Exynos5433 only. This patch set refactors the driver to make it extensible to other Samsung manufactured SoCs having DWC PCIe controllers. The major change points are: - Renaming all common functions/structures to use "samsung" instead of "exynos". Make common probe/remove/suspend/resume - Making clock/regulator get/enable/disable generic - Adding private struct to hold platform specific function ops Shradha Todi (16): dt-bindings: PCI: Rename Exynos PCIe binding to Samsung PCIe PCI: exynos: Rename Exynos PCIe driver to Samsung PCIe PCI: samsung: Change macro names to exynos specific PCI: samsung: Use clock bulk API to get clocks dt-bindings: PCI: Rename the term elbi to appl arm64: dts: exynos: Rename the term elbi to appl PCI: samsung: Rename the term elbi to appl PCI: samsung: Rename exynos_pcie to samsung_pcie PCI: samsung: Make common appl readl/writel functions dt-bindings: PCI: Add phy-names as required property arm64: dts: exynos: Add phy-names as DT property PCI: samsung: Get PHY using non-DT version PCI: samsung: Rename common functions to samsung PCI: samsung: Add platform device private data PCI: samsung: Add structure to hold resource operations PCI: samsung: Make handling of regulators generic ...ung,exynos-pcie.yaml => samsung,pcie.yaml} | 15 +- MAINTAINERS | 4 +- arch/arm64/boot/dts/exynos/exynos5433.dtsi | 3 +- drivers/pci/controller/dwc/Kconfig | 6 +- drivers/pci/controller/dwc/Makefile | 2 +- drivers/pci/controller/dwc/pci-samsung.c | 508 ++++++++++++++++++ 6 files changed, 526 insertions(+), 12 deletions(-) rename Documentation/devicetree/bindings/pci/{samsung,exynos-pcie.yaml => samsung,pcie.yaml} (89%) create mode 100644 drivers/pci/controller/dwc/pci-samsung.c