[PATCH 00/16] Refactor Exynos PCIe driver to make it generic

[PATCH 00/16] Refactor Exynos PCIe driver to make it generic

[Shradha Todi]

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

-- 
2.17.1

[PATCH 01/16] dt-bindings: PCI: Rename Exynos PCIe binding to Samsung PCIe

[Shradha Todi]

The current DT bindings is being used for Exynos5433 SoC only.
In order to extend this binding for all SoCs manufactured by
Samsung using DWC PCIe controller, renaming this file to a more
generic name.

Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 .../pci/{samsung,exynos-pcie.yaml => samsung,pcie.yaml}     | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
 rename Documentation/devicetree/bindings/pci/{samsung,exynos-pcie.yaml => samsung,pcie.yaml} (93%)

diff --git a/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
similarity index 93%
rename from Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
rename to Documentation/devicetree/bindings/pci/samsung,pcie.yaml
index f20ed7e709f7..6cd36d9ccba0 100644
--- a/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
@@ -1,17 +1,17 @@
 # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/pci/samsung,exynos-pcie.yaml#
+$id: http://devicetree.org/schemas/pci/samsung,pcie.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Samsung SoC series PCIe Host Controller
+title: Samsung SoC series PCIe Controller
 
 maintainers:
   - Marek Szyprowski <m.szyprowski@samsung.com>
   - Jaehoon Chung <jh80.chung@samsung.com>
 
 description: |+
-  Exynos5433 SoC PCIe host controller is based on the Synopsys DesignWare
+  Samsung SoC PCIe controller is based on the Synopsys DesignWare
   PCIe IP and thus inherits all the common properties defined in
   snps,dw-pcie.yaml.
 
-- 
2.17.1

Re: [PATCH 01/16] dt-bindings: PCI: Rename Exynos PCIe binding to Samsung PCIe

[Krzysztof Kozlowski]

On 14/02/2023 13:13, Shradha Todi wrote:
> The current DT bindings is being used for Exynos5433 SoC only.
> In order to extend this binding for all SoCs manufactured by
> Samsung using DWC PCIe controller, renaming this file to a more
> generic name.

Thank you for your patch. There is something to discuss/improve.

> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---
>  .../pci/{samsung,exynos-pcie.yaml => samsung,pcie.yaml}     | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>  rename Documentation/devicetree/bindings/pci/{samsung,exynos-pcie.yaml => samsung,pcie.yaml} (93%)
> 
> diff --git a/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
> similarity index 93%
> rename from Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
> rename to Documentation/devicetree/bindings/pci/samsung,pcie.yaml
> index f20ed7e709f7..6cd36d9ccba0 100644
> --- a/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml

We keep the name rather tied to compatible, not generic. There are no
other compatibles here, so I don't think we should rename it.

Best regards,
Krzysztof

Re: [PATCH 01/16] dt-bindings: PCI: Rename Exynos PCIe binding to Samsung PCIe

[Han Jingoo]

On Thu, Feb 16, 2023 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>
> On 14/02/2023 13:13, Shradha Todi wrote:
> > The current DT bindings is being used for Exynos5433 SoC only.
> > In order to extend this binding for all SoCs manufactured by
> > Samsung using DWC PCIe controller, renaming this file to a more
> > generic name.
>
> Thank you for your patch. There is something to discuss/improve.
>
> >
> > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> > ---
> >  .../pci/{samsung,exynos-pcie.yaml => samsung,pcie.yaml}     | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >  rename Documentation/devicetree/bindings/pci/{samsung,exynos-pcie.yaml => samsung,pcie.yaml} (93%)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
> > similarity index 93%
> > rename from Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
> > rename to Documentation/devicetree/bindings/pci/samsung,pcie.yaml
> > index f20ed7e709f7..6cd36d9ccba0 100644
> > --- a/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
>
> We keep the name rather tied to compatible, not generic. There are no
> other compatibles here, so I don't think we should rename it.

I also agree with Krzysztof Kozlowski's opinion. For renaming, we should have
strong valid reasons.

Best regards,
Jingoo Han

>
> Best regards,
> Krzysztof
>

RE: [PATCH 01/16] dt-bindings: PCI: Rename Exynos PCIe binding to Samsung PCIe

[Shradha Todi]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 16 February 2023 16:24
> 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 01/16] dt-bindings: PCI: Rename Exynos PCIe binding to
> Samsung PCIe
> 
> On 14/02/2023 13:13, Shradha Todi wrote:
> > The current DT bindings is being used for Exynos5433 SoC only.
> > In order to extend this binding for all SoCs manufactured by Samsung
> > using DWC PCIe controller, renaming this file to a more generic name.
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> >
> > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> > ---
> >  .../pci/{samsung,exynos-pcie.yaml => samsung,pcie.yaml}     | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)  rename
> > Documentation/devicetree/bindings/pci/{samsung,exynos-pcie.yaml =>
> > samsung,pcie.yaml} (93%)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
> > b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
> > similarity index 93%
> > rename from
> > Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
> > rename to Documentation/devicetree/bindings/pci/samsung,pcie.yaml
> > index f20ed7e709f7..6cd36d9ccba0 100644
> > --- a/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
> 
> We keep the name rather tied to compatible, not generic. There are no other
> compatibles here, so I don't think we should rename it.
> 

Our intention to rename was to have a common name for Samsung manufactured SoCs having PCIe controller.
Though this change may not be a blocker for us but we feel it will be good to have a common name as this file will
not have bindings only for Exynos series of SoC (Samsung Sys. LSI designed) but also custom ASICs such as FSD / ARTPEC SoC (Samsung Foundry designed). 
We hope we are not breaking any ABI as such in this patch.

Shradha

> Best regards,
> Krzysztof

Re: [PATCH 01/16] dt-bindings: PCI: Rename Exynos PCIe binding to Samsung PCIe

[Krzysztof Kozlowski]

On 02/03/2023 13:54, Shradha Todi wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
>> Sent: 16 February 2023 16:24
>> 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 01/16] dt-bindings: PCI: Rename Exynos PCIe binding to
>> Samsung PCIe
>>
>> On 14/02/2023 13:13, Shradha Todi wrote:
>>> The current DT bindings is being used for Exynos5433 SoC only.
>>> In order to extend this binding for all SoCs manufactured by Samsung
>>> using DWC PCIe controller, renaming this file to a more generic name.
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>>
>>> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
>>> ---
>>>  .../pci/{samsung,exynos-pcie.yaml => samsung,pcie.yaml}     | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)  rename
>>> Documentation/devicetree/bindings/pci/{samsung,exynos-pcie.yaml =>
>>> samsung,pcie.yaml} (93%)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
>>> b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
>>> similarity index 93%
>>> rename from
>>> Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
>>> rename to Documentation/devicetree/bindings/pci/samsung,pcie.yaml
>>> index f20ed7e709f7..6cd36d9ccba0 100644
>>> --- a/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
>>
>> We keep the name rather tied to compatible, not generic. There are no other
>> compatibles here, so I don't think we should rename it.
>>
> 
> Our intention to rename was to have a common name for Samsung manufactured SoCs having PCIe controller.
> Though this change may not be a blocker for us but we feel it will be good to have a common name as this file will
> not have bindings only for Exynos series of SoC (Samsung Sys. LSI designed) but also custom ASICs such as FSD / ARTPEC SoC (Samsung Foundry designed). 
> We hope we are not breaking any ABI as such in this patch.

There is no FSD/Artpec added here, so renaming just for "rename" is a
no. If you add new hardware here, this could have sense, depending on
the hardware. But since we pretty often expect the first compatible to
be the name of the file, why renaming at all?

Best regards,
Krzysztof

[PATCH 02/16] PCI: exynos: Rename Exynos PCIe driver to Samsung PCIe

[Shradha Todi]

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 +++++++++++++++++++++++
 4 files changed, 449 insertions(+), 6 deletions(-)
 create mode 100644 drivers/pci/controller/dwc/pci-samsung.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a9d38274bd4d..e61374adf193 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16084,13 +16084,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/pci/*rcar*
 F:	drivers/pci/controller/*rcar*
 
-PCI DRIVER FOR SAMSUNG EXYNOS
+PCI DRIVER FOR SAMSUNG
 M:	Jingoo Han <jingoohan1@gmail.com>
 L:	linux-pci@vger.kernel.org
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 L:	linux-samsung-soc@vger.kernel.org
 S:	Maintained
-F:	drivers/pci/controller/dwc/pci-exynos.c
+F:	drivers/pci/controller/dwc/pci-samsung.c
 
 PCI DRIVER FOR SYNOPSYS DESIGNWARE
 M:	Jingoo Han <jingoohan1@gmail.com>
diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 434f6a4f4041..837c7693ac6e 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -80,13 +80,13 @@ config PCIE_DW_PLAT_EP
 	  order to enable device-specific features PCI_DW_PLAT_EP must be
 	  selected.
 
-config PCI_EXYNOS
-	tristate "Samsung Exynos PCIe controller"
+config PCI_SAMSUNG
+	tristate "Samsung PCIe controller"
 	depends on ARCH_EXYNOS || COMPILE_TEST
 	depends on PCI_MSI
 	select PCIE_DW_HOST
 	help
-	  Enables support for the PCIe controller in the Samsung Exynos SoCs
+	  Enables support for the PCIe controller in the Samsung SoCs
 	  to work in host mode. The PCI controller is based on the DesignWare
 	  hardware and therefore the driver re-uses the DesignWare core
 	  functions to implement the driver.
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index bf5c311875a1..9daf9643342d 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -5,7 +5,7 @@ obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
 obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
 obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o
 obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
-obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
+obj-$(CONFIG_PCI_SAMSUNG) += pci-samsung.o
 obj-$(CONFIG_PCIE_FU740) += pcie-fu740.o
 obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
 obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
diff --git a/drivers/pci/controller/dwc/pci-samsung.c b/drivers/pci/controller/dwc/pci-samsung.c
new file mode 100644
index 000000000000..cc562a8694fb
--- /dev/null
+++ b/drivers/pci/controller/dwc/pci-samsung.c
@@ -0,0 +1,443 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe host controller driver for Samsung SoCs
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ *		https://www.samsung.com
+ *
+ * Author: Jingoo Han <jg1.han@samsung.com>
+ *	   Jaehoon Chung <jh80.chung@samsung.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/of_device.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+#include <linux/regulator/consumer.h>
+#include <linux/module.h>
+
+#include "pcie-designware.h"
+
+#define to_exynos_pcie(x)	dev_get_drvdata((x)->dev)
+
+/* PCIe ELBI registers */
+#define PCIE_IRQ_PULSE			0x000
+#define IRQ_INTA_ASSERT			BIT(0)
+#define IRQ_INTB_ASSERT			BIT(2)
+#define IRQ_INTC_ASSERT			BIT(4)
+#define IRQ_INTD_ASSERT			BIT(6)
+#define PCIE_IRQ_LEVEL			0x004
+#define PCIE_IRQ_SPECIAL		0x008
+#define PCIE_IRQ_EN_PULSE		0x00c
+#define PCIE_IRQ_EN_LEVEL		0x010
+#define PCIE_IRQ_EN_SPECIAL		0x014
+#define PCIE_SW_WAKE			0x018
+#define PCIE_BUS_EN			BIT(1)
+#define PCIE_CORE_RESET			0x01c
+#define PCIE_CORE_RESET_ENABLE		BIT(0)
+#define PCIE_STICKY_RESET		0x020
+#define PCIE_NONSTICKY_RESET		0x024
+#define PCIE_APP_INIT_RESET		0x028
+#define PCIE_APP_LTSSM_ENABLE		0x02c
+#define PCIE_ELBI_RDLH_LINKUP		0x074
+#define PCIE_ELBI_XMLH_LINKUP		BIT(4)
+#define PCIE_ELBI_LTSSM_ENABLE		0x1
+#define PCIE_ELBI_SLV_AWMISC		0x11c
+#define PCIE_ELBI_SLV_ARMISC		0x120
+#define PCIE_ELBI_SLV_DBI_ENABLE	BIT(21)
+
+struct exynos_pcie {
+	struct dw_pcie			pci;
+	void __iomem			*elbi_base;
+	struct clk			*clk;
+	struct clk			*bus_clk;
+	struct phy			*phy;
+	struct regulator_bulk_data	supplies[2];
+};
+
+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");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(ep->bus_clk);
+	if (ret) {
+		dev_err(dev, "cannot enable pcie bus clock");
+		goto err_bus_clk;
+	}
+
+	return 0;
+
+err_bus_clk:
+	clk_disable_unprepare(ep->clk);
+
+	return ret;
+}
+
+static void exynos_pcie_deinit_clk_resources(struct exynos_pcie *ep)
+{
+	clk_disable_unprepare(ep->bus_clk);
+	clk_disable_unprepare(ep->clk);
+}
+
+static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
+{
+	writel(val, base + reg);
+}
+
+static u32 exynos_pcie_readl(void __iomem *base, u32 reg)
+{
+	return readl(base + reg);
+}
+
+static void exynos_pcie_sideband_dbi_w_mode(struct exynos_pcie *ep, bool on)
+{
+	u32 val;
+
+	val = exynos_pcie_readl(ep->elbi_base, PCIE_ELBI_SLV_AWMISC);
+	if (on)
+		val |= PCIE_ELBI_SLV_DBI_ENABLE;
+	else
+		val &= ~PCIE_ELBI_SLV_DBI_ENABLE;
+	exynos_pcie_writel(ep->elbi_base, val, PCIE_ELBI_SLV_AWMISC);
+}
+
+static void exynos_pcie_sideband_dbi_r_mode(struct exynos_pcie *ep, bool on)
+{
+	u32 val;
+
+	val = exynos_pcie_readl(ep->elbi_base, PCIE_ELBI_SLV_ARMISC);
+	if (on)
+		val |= PCIE_ELBI_SLV_DBI_ENABLE;
+	else
+		val &= ~PCIE_ELBI_SLV_DBI_ENABLE;
+	exynos_pcie_writel(ep->elbi_base, val, PCIE_ELBI_SLV_ARMISC);
+}
+
+static void exynos_pcie_assert_core_reset(struct exynos_pcie *ep)
+{
+	u32 val;
+
+	val = exynos_pcie_readl(ep->elbi_base, PCIE_CORE_RESET);
+	val &= ~PCIE_CORE_RESET_ENABLE;
+	exynos_pcie_writel(ep->elbi_base, val, PCIE_CORE_RESET);
+	exynos_pcie_writel(ep->elbi_base, 0, PCIE_STICKY_RESET);
+	exynos_pcie_writel(ep->elbi_base, 0, PCIE_NONSTICKY_RESET);
+}
+
+static void exynos_pcie_deassert_core_reset(struct exynos_pcie *ep)
+{
+	u32 val;
+
+	val = exynos_pcie_readl(ep->elbi_base, PCIE_CORE_RESET);
+	val |= PCIE_CORE_RESET_ENABLE;
+
+	exynos_pcie_writel(ep->elbi_base, val, PCIE_CORE_RESET);
+	exynos_pcie_writel(ep->elbi_base, 1, PCIE_STICKY_RESET);
+	exynos_pcie_writel(ep->elbi_base, 1, PCIE_NONSTICKY_RESET);
+	exynos_pcie_writel(ep->elbi_base, 1, PCIE_APP_INIT_RESET);
+	exynos_pcie_writel(ep->elbi_base, 0, PCIE_APP_INIT_RESET);
+}
+
+static int exynos_pcie_start_link(struct dw_pcie *pci)
+{
+	struct exynos_pcie *ep = to_exynos_pcie(pci);
+	u32 val;
+
+	val = exynos_pcie_readl(ep->elbi_base, PCIE_SW_WAKE);
+	val &= ~PCIE_BUS_EN;
+	exynos_pcie_writel(ep->elbi_base, val, PCIE_SW_WAKE);
+
+	/* assert LTSSM enable */
+	exynos_pcie_writel(ep->elbi_base, PCIE_ELBI_LTSSM_ENABLE,
+			  PCIE_APP_LTSSM_ENABLE);
+	return 0;
+}
+
+static void exynos_pcie_clear_irq_pulse(struct exynos_pcie *ep)
+{
+	u32 val = exynos_pcie_readl(ep->elbi_base, PCIE_IRQ_PULSE);
+
+	exynos_pcie_writel(ep->elbi_base, val, PCIE_IRQ_PULSE);
+}
+
+static irqreturn_t exynos_pcie_irq_handler(int irq, void *arg)
+{
+	struct exynos_pcie *ep = arg;
+
+	exynos_pcie_clear_irq_pulse(ep);
+	return IRQ_HANDLED;
+}
+
+static void exynos_pcie_enable_irq_pulse(struct exynos_pcie *ep)
+{
+	u32 val = IRQ_INTA_ASSERT | IRQ_INTB_ASSERT |
+		  IRQ_INTC_ASSERT | IRQ_INTD_ASSERT;
+
+	exynos_pcie_writel(ep->elbi_base, val, PCIE_IRQ_EN_PULSE);
+	exynos_pcie_writel(ep->elbi_base, 0, PCIE_IRQ_EN_LEVEL);
+	exynos_pcie_writel(ep->elbi_base, 0, PCIE_IRQ_EN_SPECIAL);
+}
+
+static u32 exynos_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
+				u32 reg, size_t size)
+{
+	struct exynos_pcie *ep = to_exynos_pcie(pci);
+	u32 val;
+
+	exynos_pcie_sideband_dbi_r_mode(ep, true);
+	dw_pcie_read(base + reg, size, &val);
+	exynos_pcie_sideband_dbi_r_mode(ep, false);
+	return val;
+}
+
+static void exynos_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
+				  u32 reg, size_t size, u32 val)
+{
+	struct exynos_pcie *ep = to_exynos_pcie(pci);
+
+	exynos_pcie_sideband_dbi_w_mode(ep, true);
+	dw_pcie_write(base + reg, size, val);
+	exynos_pcie_sideband_dbi_w_mode(ep, false);
+}
+
+static int exynos_pcie_rd_own_conf(struct pci_bus *bus, unsigned int devfn,
+				   int where, int size, u32 *val)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
+
+	if (PCI_SLOT(devfn))
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	*val = dw_pcie_read_dbi(pci, where, size);
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int exynos_pcie_wr_own_conf(struct pci_bus *bus, unsigned int devfn,
+				   int where, int size, u32 val)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
+
+	if (PCI_SLOT(devfn))
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	dw_pcie_write_dbi(pci, where, size, val);
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops exynos_pci_ops = {
+	.read = exynos_pcie_rd_own_conf,
+	.write = exynos_pcie_wr_own_conf,
+};
+
+static int exynos_pcie_link_up(struct dw_pcie *pci)
+{
+	struct exynos_pcie *ep = to_exynos_pcie(pci);
+	u32 val = exynos_pcie_readl(ep->elbi_base, PCIE_ELBI_RDLH_LINKUP);
+
+	return (val & PCIE_ELBI_XMLH_LINKUP);
+}
+
+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;
+
+	exynos_pcie_assert_core_reset(ep);
+
+	phy_init(ep->phy);
+	phy_power_on(ep->phy);
+
+	exynos_pcie_deassert_core_reset(ep);
+	exynos_pcie_enable_irq_pulse(ep);
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops exynos_pcie_host_ops = {
+	.host_init = exynos_pcie_host_init,
+};
+
+static int exynos_add_pcie_port(struct exynos_pcie *ep,
+				       struct platform_device *pdev)
+{
+	struct dw_pcie *pci = &ep->pci;
+	struct dw_pcie_rp *pp = &pci->pp;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	pp->irq = platform_get_irq(pdev, 0);
+	if (pp->irq < 0)
+		return pp->irq;
+
+	ret = devm_request_irq(dev, pp->irq, exynos_pcie_irq_handler,
+			       IRQF_SHARED, "exynos-pcie", ep);
+	if (ret) {
+		dev_err(dev, "failed to request irq\n");
+		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;
+}
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.read_dbi = exynos_pcie_read_dbi,
+	.write_dbi = exynos_pcie_write_dbi,
+	.link_up = exynos_pcie_link_up,
+	.start_link = exynos_pcie_start_link,
+};
+
+static int exynos_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct exynos_pcie *ep;
+	struct device_node *np = dev->of_node;
+	int ret;
+
+	ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
+	if (!ep)
+		return -ENOMEM;
+
+	ep->pci.dev = dev;
+	ep->pci.ops = &dw_pcie_ops;
+
+	ep->phy = devm_of_phy_get(dev, np, NULL);
+	if (IS_ERR(ep->phy))
+		return PTR_ERR(ep->phy);
+
+	/* External Local Bus interface (ELBI) registers */
+	ep->elbi_base = devm_platform_ioremap_resource_byname(pdev, "elbi");
+	if (IS_ERR(ep->elbi_base))
+		return PTR_ERR(ep->elbi_base);
+
+	ep->clk = devm_clk_get(dev, "pcie");
+	if (IS_ERR(ep->clk)) {
+		dev_err(dev, "Failed to get pcie rc clock\n");
+		return PTR_ERR(ep->clk);
+	}
+
+	ep->bus_clk = devm_clk_get(dev, "pcie_bus");
+	if (IS_ERR(ep->bus_clk)) {
+		dev_err(dev, "Failed to get pcie bus clock\n");
+		return PTR_ERR(ep->bus_clk);
+	}
+
+	ep->supplies[0].supply = "vdd18";
+	ep->supplies[1].supply = "vdd10";
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ep->supplies),
+				      ep->supplies);
+	if (ret)
+		return ret;
+
+	ret = exynos_pcie_init_clk_resources(ep);
+	if (ret)
+		return ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, ep);
+
+	ret = exynos_add_pcie_port(ep, pdev);
+	if (ret < 0)
+		goto fail_probe;
+
+	return 0;
+
+fail_probe:
+	phy_exit(ep->phy);
+	exynos_pcie_deinit_clk_resources(ep);
+	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+
+	return ret;
+}
+
+static int __exit exynos_pcie_remove(struct platform_device *pdev)
+{
+	struct exynos_pcie *ep = platform_get_drvdata(pdev);
+
+	dw_pcie_host_deinit(&ep->pci.pp);
+	exynos_pcie_assert_core_reset(ep);
+	phy_power_off(ep->phy);
+	phy_exit(ep->phy);
+	exynos_pcie_deinit_clk_resources(ep);
+	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+
+	return 0;
+}
+
+static int exynos_pcie_suspend_noirq(struct device *dev)
+{
+	struct exynos_pcie *ep = dev_get_drvdata(dev);
+
+	exynos_pcie_assert_core_reset(ep);
+	phy_power_off(ep->phy);
+	phy_exit(ep->phy);
+	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+
+	return 0;
+}
+
+static int exynos_pcie_resume_noirq(struct device *dev)
+{
+	struct exynos_pcie *ep = dev_get_drvdata(dev);
+	struct dw_pcie *pci = &ep->pci;
+	struct dw_pcie_rp *pp = &pci->pp;
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
+	if (ret)
+		return ret;
+
+	/* exynos_pcie_host_init controls ep->phy */
+	exynos_pcie_host_init(pp);
+	dw_pcie_setup_rc(pp);
+	exynos_pcie_start_link(pci);
+	return dw_pcie_wait_for_link(pci);
+}
+
+static const struct dev_pm_ops exynos_pcie_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(exynos_pcie_suspend_noirq,
+				  exynos_pcie_resume_noirq)
+};
+
+static const struct of_device_id exynos_pcie_of_match[] = {
+	{ .compatible = "samsung,exynos5433-pcie", },
+	{ },
+};
+
+static struct platform_driver exynos_pcie_driver = {
+	.probe		= exynos_pcie_probe,
+	.remove		= __exit_p(exynos_pcie_remove),
+	.driver = {
+		.name	= "exynos-pcie",
+		.of_match_table = exynos_pcie_of_match,
+		.pm		= &exynos_pcie_pm_ops,
+	},
+};
+module_platform_driver(exynos_pcie_driver);
+MODULE_LICENSE("GPL v2");
+MODULE_DEVICE_TABLE(of, exynos_pcie_of_match);
-- 
2.17.1

Re: [PATCH 02/16] PCI: exynos: Rename Exynos PCIe driver to Samsung PCIe

[Krzysztof Kozlowski]

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

Re: [PATCH 02/16] PCI: exynos: Rename Exynos PCIe driver to Samsung PCIe

[Krzysztof Kozlowski]

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

RE: [PATCH 02/16] PCI: exynos: Rename Exynos PCIe driver to Samsung PCIe

[Shradha Todi]

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

Re: [PATCH 02/16] PCI: exynos: Rename Exynos PCIe driver to Samsung PCIe

[Krzysztof Kozlowski]

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

[PATCH 03/16] PCI: samsung: Change macro names to exynos specific

[Shradha Todi]

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(-)

diff --git a/drivers/pci/controller/dwc/pci-samsung.c b/drivers/pci/controller/dwc/pci-samsung.c
index cc562a8694fb..cfe384aee754 100644
--- a/drivers/pci/controller/dwc/pci-samsung.c
+++ b/drivers/pci/controller/dwc/pci-samsung.c
@@ -26,30 +26,30 @@
 #define to_exynos_pcie(x)	dev_get_drvdata((x)->dev)
 
 /* PCIe ELBI registers */
-#define PCIE_IRQ_PULSE			0x000
-#define IRQ_INTA_ASSERT			BIT(0)
-#define IRQ_INTB_ASSERT			BIT(2)
-#define IRQ_INTC_ASSERT			BIT(4)
-#define IRQ_INTD_ASSERT			BIT(6)
-#define PCIE_IRQ_LEVEL			0x004
-#define PCIE_IRQ_SPECIAL		0x008
-#define PCIE_IRQ_EN_PULSE		0x00c
-#define PCIE_IRQ_EN_LEVEL		0x010
-#define PCIE_IRQ_EN_SPECIAL		0x014
-#define PCIE_SW_WAKE			0x018
-#define PCIE_BUS_EN			BIT(1)
-#define PCIE_CORE_RESET			0x01c
-#define PCIE_CORE_RESET_ENABLE		BIT(0)
-#define PCIE_STICKY_RESET		0x020
-#define PCIE_NONSTICKY_RESET		0x024
-#define PCIE_APP_INIT_RESET		0x028
-#define PCIE_APP_LTSSM_ENABLE		0x02c
-#define PCIE_ELBI_RDLH_LINKUP		0x074
-#define PCIE_ELBI_XMLH_LINKUP		BIT(4)
-#define PCIE_ELBI_LTSSM_ENABLE		0x1
-#define PCIE_ELBI_SLV_AWMISC		0x11c
-#define PCIE_ELBI_SLV_ARMISC		0x120
-#define PCIE_ELBI_SLV_DBI_ENABLE	BIT(21)
+#define EXYNOS_PCIE_IRQ_PULSE			0x000
+#define EXYNOS_IRQ_INTA_ASSERT			BIT(0)
+#define EXYNOS_IRQ_INTB_ASSERT			BIT(2)
+#define EXYNOS_IRQ_INTC_ASSERT			BIT(4)
+#define EXYNOS_IRQ_INTD_ASSERT			BIT(6)
+#define EXYNOS_PCIE_IRQ_LEVEL			0x004
+#define EXYNOS_PCIE_IRQ_SPECIAL		0x008
+#define EXYNOS_PCIE_IRQ_EN_PULSE		0x00c
+#define EXYNOS_PCIE_IRQ_EN_LEVEL		0x010
+#define EXYNOS_PCIE_IRQ_EN_SPECIAL		0x014
+#define EXYNOS_PCIE_SW_WAKE			0x018
+#define EXYNOS_PCIE_BUS_EN			BIT(1)
+#define EXYNOS_PCIE_CORE_RESET			0x01c
+#define EXYNOS_PCIE_CORE_RESET_ENABLE		BIT(0)
+#define EXYNOS_PCIE_STICKY_RESET		0x020
+#define EXYNOS_PCIE_NONSTICKY_RESET		0x024
+#define EXYNOS_PCIE_APP_INIT_RESET		0x028
+#define EXYNOS_PCIE_APP_LTSSM_ENABLE		0x02c
+#define EXYNOS_PCIE_ELBI_RDLH_LINKUP		0x074
+#define EXYNOS_PCIE_ELBI_XMLH_LINKUP		BIT(4)
+#define EXYNOS_PCIE_ELBI_LTSSM_ENABLE		0x1
+#define EXYNOS_PCIE_ELBI_SLV_AWMISC		0x11c
+#define EXYNOS_PCIE_ELBI_SLV_ARMISC		0x120
+#define EXYNOS_PCIE_ELBI_SLV_DBI_ENABLE	BIT(21)
 
 struct exynos_pcie {
 	struct dw_pcie			pci;
@@ -105,49 +105,49 @@ static void exynos_pcie_sideband_dbi_w_mode(struct exynos_pcie *ep, bool on)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(ep->elbi_base, PCIE_ELBI_SLV_AWMISC);
+	val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_ELBI_SLV_AWMISC);
 	if (on)
-		val |= PCIE_ELBI_SLV_DBI_ENABLE;
+		val |= EXYNOS_PCIE_ELBI_SLV_DBI_ENABLE;
 	else
-		val &= ~PCIE_ELBI_SLV_DBI_ENABLE;
-	exynos_pcie_writel(ep->elbi_base, val, PCIE_ELBI_SLV_AWMISC);
+		val &= ~EXYNOS_PCIE_ELBI_SLV_DBI_ENABLE;
+	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_ELBI_SLV_AWMISC);
 }
 
 static void exynos_pcie_sideband_dbi_r_mode(struct exynos_pcie *ep, bool on)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(ep->elbi_base, PCIE_ELBI_SLV_ARMISC);
+	val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_ELBI_SLV_ARMISC);
 	if (on)
-		val |= PCIE_ELBI_SLV_DBI_ENABLE;
+		val |= EXYNOS_PCIE_ELBI_SLV_DBI_ENABLE;
 	else
-		val &= ~PCIE_ELBI_SLV_DBI_ENABLE;
-	exynos_pcie_writel(ep->elbi_base, val, PCIE_ELBI_SLV_ARMISC);
+		val &= ~EXYNOS_PCIE_ELBI_SLV_DBI_ENABLE;
+	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_ELBI_SLV_ARMISC);
 }
 
 static void exynos_pcie_assert_core_reset(struct exynos_pcie *ep)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(ep->elbi_base, PCIE_CORE_RESET);
-	val &= ~PCIE_CORE_RESET_ENABLE;
-	exynos_pcie_writel(ep->elbi_base, val, PCIE_CORE_RESET);
-	exynos_pcie_writel(ep->elbi_base, 0, PCIE_STICKY_RESET);
-	exynos_pcie_writel(ep->elbi_base, 0, PCIE_NONSTICKY_RESET);
+	val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_CORE_RESET);
+	val &= ~EXYNOS_PCIE_CORE_RESET_ENABLE;
+	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_CORE_RESET);
+	exynos_pcie_writel(ep->elbi_base, 0, EXYNOS_PCIE_STICKY_RESET);
+	exynos_pcie_writel(ep->elbi_base, 0, EXYNOS_PCIE_NONSTICKY_RESET);
 }
 
 static void exynos_pcie_deassert_core_reset(struct exynos_pcie *ep)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(ep->elbi_base, PCIE_CORE_RESET);
-	val |= PCIE_CORE_RESET_ENABLE;
+	val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_CORE_RESET);
+	val |= EXYNOS_PCIE_CORE_RESET_ENABLE;
 
-	exynos_pcie_writel(ep->elbi_base, val, PCIE_CORE_RESET);
-	exynos_pcie_writel(ep->elbi_base, 1, PCIE_STICKY_RESET);
-	exynos_pcie_writel(ep->elbi_base, 1, PCIE_NONSTICKY_RESET);
-	exynos_pcie_writel(ep->elbi_base, 1, PCIE_APP_INIT_RESET);
-	exynos_pcie_writel(ep->elbi_base, 0, PCIE_APP_INIT_RESET);
+	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_CORE_RESET);
+	exynos_pcie_writel(ep->elbi_base, 1, EXYNOS_PCIE_STICKY_RESET);
+	exynos_pcie_writel(ep->elbi_base, 1, EXYNOS_PCIE_NONSTICKY_RESET);
+	exynos_pcie_writel(ep->elbi_base, 1, EXYNOS_PCIE_APP_INIT_RESET);
+	exynos_pcie_writel(ep->elbi_base, 0, EXYNOS_PCIE_APP_INIT_RESET);
 }
 
 static int exynos_pcie_start_link(struct dw_pcie *pci)
@@ -155,21 +155,21 @@ static int exynos_pcie_start_link(struct dw_pcie *pci)
 	struct exynos_pcie *ep = to_exynos_pcie(pci);
 	u32 val;
 
-	val = exynos_pcie_readl(ep->elbi_base, PCIE_SW_WAKE);
-	val &= ~PCIE_BUS_EN;
-	exynos_pcie_writel(ep->elbi_base, val, PCIE_SW_WAKE);
+	val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_SW_WAKE);
+	val &= ~EXYNOS_PCIE_BUS_EN;
+	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_SW_WAKE);
 
 	/* assert LTSSM enable */
-	exynos_pcie_writel(ep->elbi_base, PCIE_ELBI_LTSSM_ENABLE,
-			  PCIE_APP_LTSSM_ENABLE);
+	exynos_pcie_writel(ep->elbi_base, EXYNOS_PCIE_ELBI_LTSSM_ENABLE,
+			  EXYNOS_PCIE_APP_LTSSM_ENABLE);
 	return 0;
 }
 
 static void exynos_pcie_clear_irq_pulse(struct exynos_pcie *ep)
 {
-	u32 val = exynos_pcie_readl(ep->elbi_base, PCIE_IRQ_PULSE);
+	u32 val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_IRQ_PULSE);
 
-	exynos_pcie_writel(ep->elbi_base, val, PCIE_IRQ_PULSE);
+	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_IRQ_PULSE);
 }
 
 static irqreturn_t exynos_pcie_irq_handler(int irq, void *arg)
@@ -182,12 +182,12 @@ static irqreturn_t exynos_pcie_irq_handler(int irq, void *arg)
 
 static void exynos_pcie_enable_irq_pulse(struct exynos_pcie *ep)
 {
-	u32 val = IRQ_INTA_ASSERT | IRQ_INTB_ASSERT |
-		  IRQ_INTC_ASSERT | IRQ_INTD_ASSERT;
+	u32 val = EXYNOS_IRQ_INTA_ASSERT | EXYNOS_IRQ_INTB_ASSERT |
+		  EXYNOS_IRQ_INTC_ASSERT | EXYNOS_IRQ_INTD_ASSERT;
 
-	exynos_pcie_writel(ep->elbi_base, val, PCIE_IRQ_EN_PULSE);
-	exynos_pcie_writel(ep->elbi_base, 0, PCIE_IRQ_EN_LEVEL);
-	exynos_pcie_writel(ep->elbi_base, 0, PCIE_IRQ_EN_SPECIAL);
+	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_IRQ_EN_PULSE);
+	exynos_pcie_writel(ep->elbi_base, 0, EXYNOS_PCIE_IRQ_EN_LEVEL);
+	exynos_pcie_writel(ep->elbi_base, 0, EXYNOS_PCIE_IRQ_EN_SPECIAL);
 }
 
 static u32 exynos_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
@@ -244,9 +244,9 @@ static struct pci_ops exynos_pci_ops = {
 static int exynos_pcie_link_up(struct dw_pcie *pci)
 {
 	struct exynos_pcie *ep = to_exynos_pcie(pci);
-	u32 val = exynos_pcie_readl(ep->elbi_base, PCIE_ELBI_RDLH_LINKUP);
+	u32 val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_ELBI_RDLH_LINKUP);
 
-	return (val & PCIE_ELBI_XMLH_LINKUP);
+	return (val & EXYNOS_PCIE_ELBI_XMLH_LINKUP);
 }
 
 static int exynos_pcie_host_init(struct dw_pcie_rp *pp)
-- 
2.17.1

Re: [PATCH 03/16] PCI: samsung: Change macro names to exynos specific

[Krzysztof Kozlowski]

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

[PATCH 04/16] PCI: samsung: Use clock bulk API to get clocks

[Shradha Todi]

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;
 
-	return 0;
-
-err_bus_clk:
-	clk_disable_unprepare(ep->clk);
-
-	return ret;
+	return clk_bulk_prepare_enable(ep->clk_cnt, ep->clks);
 }
 
 static void exynos_pcie_deinit_clk_resources(struct exynos_pcie *ep)
 {
-	clk_disable_unprepare(ep->bus_clk);
-	clk_disable_unprepare(ep->clk);
+	clk_bulk_disable_unprepare(ep->clk_cnt, ep->clks);
 }
 
 static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
@@ -332,17 +320,9 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(ep->elbi_base))
 		return PTR_ERR(ep->elbi_base);
 
-	ep->clk = devm_clk_get(dev, "pcie");
-	if (IS_ERR(ep->clk)) {
-		dev_err(dev, "Failed to get pcie rc clock\n");
-		return PTR_ERR(ep->clk);
-	}
-
-	ep->bus_clk = devm_clk_get(dev, "pcie_bus");
-	if (IS_ERR(ep->bus_clk)) {
-		dev_err(dev, "Failed to get pcie bus clock\n");
-		return PTR_ERR(ep->bus_clk);
-	}
+	ret = exynos_pcie_init_clk_resources(ep);
+	if (ret < 0)
+		return ret;
 
 	ep->supplies[0].supply = "vdd18";
 	ep->supplies[1].supply = "vdd10";
@@ -351,10 +331,6 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = exynos_pcie_init_clk_resources(ep);
-	if (ret)
-		return ret;
-
 	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
 	if (ret)
 		return ret;
@@ -369,8 +345,8 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 
 fail_probe:
 	phy_exit(ep->phy);
-	exynos_pcie_deinit_clk_resources(ep);
 	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+	exynos_pcie_deinit_clk_resources(ep);
 
 	return ret;
 }
-- 
2.17.1

Re: [PATCH 04/16] PCI: samsung: Use clock bulk API to get clocks

[Krzysztof Kozlowski]

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

RE: [PATCH 04/16] PCI: samsung: Use clock bulk API to get clocks

[Shradha Todi]

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

[PATCH 05/16] dt-bindings: PCI: Rename the term elbi to appl

[Shradha Todi]

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
"arm64: dts: Rename the term elbi to appl" and
"PCI: samsung: Rename the term elbi to appl"

Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 Documentation/devicetree/bindings/pci/samsung,pcie.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/samsung,pcie.yaml b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
index 6cd36d9ccba0..9c58c4d1f6a7 100644
--- a/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
@@ -25,13 +25,13 @@ properties:
   reg:
     items:
       - description: Data Bus Interface (DBI) registers.
-      - description: External Local Bus interface (ELBI) registers.
+      - description: Controller's application logic registers.
       - description: PCIe configuration space region.
 
   reg-names:
     items:
       - const: dbi
-      - const: elbi
+      - const: appl
       - const: config
 
   interrupts:
-- 
2.17.1

Re: [PATCH 05/16] dt-bindings: PCI: Rename the term elbi to appl

[Rob Herring]

On Tue, 14 Feb 2023 17:43:22 +0530, 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
> "arm64: dts: Rename the term elbi to appl" and
> "PCI: samsung: Rename the term elbi to appl"
> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---
>  Documentation/devicetree/bindings/pci/samsung,pcie.yaml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/samsung,pcie.example.dtb: pcie@15700000: reg-names:1: 'appl' was expected
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/samsung,pcie.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230214121333.1837-6-shradha.t@samsung.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

RE: [PATCH 05/16] dt-bindings: PCI: Rename the term elbi to appl

[Shradha Todi]

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: 14 February 2023 21:43
> To: Shradha Todi <shradha.t@samsung.com>
> Cc: alim.akhtar@samsung.com; Sergey.Semin@baikalelectronics.ru;
> pankaj.dubey@samsung.com; jh80.chung@samsung.co;
> krzysztof.kozlowski+dt@linaro.org; devicetree@vger.kernel.org;
> lukas.bulwahn@gmail.com; hongxing.zhu@nxp.com; bhelgaas@google.com;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> m.szyprowski@samsung.com; linux-kernel@vger.kernel.org;
> lpieralisi@kernel.org; tglx@linutronix.de; linux-samsung-
> soc@vger.kernel.org; kw@linux.com; jingoohan1@gmail.com
> Subject: Re: [PATCH 05/16] dt-bindings: PCI: Rename the term elbi to appl
> 
> 
> On Tue, 14 Feb 2023 17:43:22 +0530, 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
> > "arm64: dts: Rename the term elbi to appl" and
> > "PCI: samsung: Rename the term elbi to appl"
> >
> > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> > ---
> >  Documentation/devicetree/bindings/pci/samsung,pcie.yaml | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m
> dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/pci/samsung,pcie.example.dtb
> : pcie@15700000: reg-names:1: 'appl' was expected
> 	From schema: /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
> 
> doc reference errors (make refcheckdocs):
> 
> See https://protect2.fireeye.com/v1/url?k=b0812da3-d10a3895-b080a6ec-
> 74fe485fffe0-cf137b875436d997&q=1&e=09e1c000-01c6-48c4-8fff-
> 94e474b2862c&u=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fdev
> icetree-bindings%2Fpatch%2F20230214121333.1837-6-
> shradha.t%40samsung.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above error(s),
> then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your
> schema.

Thanks for review. Sorry, I missed to update the example. Will take care in next patch set.

Re: [PATCH 05/16] dt-bindings: PCI: Rename the term elbi to appl

[Rob Herring]

On Tue, Feb 14, 2023 at 05:43:22PM +0530, 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
> "arm64: dts: Rename the term elbi to appl" and
> "PCI: samsung: Rename the term elbi to appl"

Which is your clue that this is an ABI break. You're stuck with the old 
name unless the ABI break is fine for all Samsung platforms using this.

Rob

Re: [PATCH 05/16] dt-bindings: PCI: Rename the term elbi to appl

[Krzysztof Kozlowski]

On 14/02/2023 20:15, Rob Herring wrote:
> On Tue, Feb 14, 2023 at 05:43:22PM +0530, 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
>> "arm64: dts: Rename the term elbi to appl" and
>> "PCI: samsung: Rename the term elbi to appl"
> 
> Which is your clue that this is an ABI break. You're stuck with the old 
> name unless the ABI break is fine for all Samsung platforms using this.

This piece is quite stable so even without many out-of-tree users, we
shouldn't break it without valid reason. Wrong nomenclature is not
necessarily valid enough.

Best regards,
Krzysztof

[PATCH 06/16] arm64: dts: exynos: Rename the term elbi to appl

[Shradha Todi]

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"

Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 arch/arm64/boot/dts/exynos/exynos5433.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
index 5519a80576c5..96b216099594 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
@@ -1944,7 +1944,7 @@
 			compatible = "samsung,exynos5433-pcie";
 			reg = <0x15700000 0x1000>, <0x156b0000 0x1000>,
 			      <0x0c000000 0x1000>;
-			reg-names = "dbi", "elbi", "config";
+			reg-names = "dbi", "appl", "config";
 			#address-cells = <3>;
 			#size-cells = <2>;
 			#interrupt-cells = <1>;
-- 
2.17.1

Re: [PATCH 06/16] arm64: dts: exynos: Rename the term elbi to appl

[Krzysztof Kozlowski]

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

RE: [PATCH 06/16] arm64: dts: exynos: Rename the term elbi to appl

[Shradha Todi]

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

Re: [PATCH 06/16] arm64: dts: exynos: Rename the term elbi to appl

[Krzysztof Kozlowski]

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

[PATCH 07/16] PCI: samsung: Rename the term elbi to appl

[Shradha Todi]

The driver uses the name elbi for application logic
registers which is a wrong nomenclature. This patch fixes
the same.

This commit shouldn't be applied without changes
"arm64: dts: exynos: Rename the term elbi to appl" and
"dt-bindings: PCI: Rename the term elbi to appl"

Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 drivers/pci/controller/dwc/pci-samsung.c | 80 ++++++++++++------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-samsung.c b/drivers/pci/controller/dwc/pci-samsung.c
index 6c07d3f151be..d5adf1017a05 100644
--- a/drivers/pci/controller/dwc/pci-samsung.c
+++ b/drivers/pci/controller/dwc/pci-samsung.c
@@ -25,7 +25,7 @@
 
 #define to_exynos_pcie(x)	dev_get_drvdata((x)->dev)
 
-/* PCIe ELBI registers */
+/* PCIe APPL registers */
 #define EXYNOS_PCIE_IRQ_PULSE			0x000
 #define EXYNOS_IRQ_INTA_ASSERT			BIT(0)
 #define EXYNOS_IRQ_INTB_ASSERT			BIT(2)
@@ -44,16 +44,16 @@
 #define EXYNOS_PCIE_NONSTICKY_RESET		0x024
 #define EXYNOS_PCIE_APP_INIT_RESET		0x028
 #define EXYNOS_PCIE_APP_LTSSM_ENABLE		0x02c
-#define EXYNOS_PCIE_ELBI_RDLH_LINKUP		0x074
-#define EXYNOS_PCIE_ELBI_XMLH_LINKUP		BIT(4)
-#define EXYNOS_PCIE_ELBI_LTSSM_ENABLE		0x1
-#define EXYNOS_PCIE_ELBI_SLV_AWMISC		0x11c
-#define EXYNOS_PCIE_ELBI_SLV_ARMISC		0x120
-#define EXYNOS_PCIE_ELBI_SLV_DBI_ENABLE	BIT(21)
+#define EXYNOS_PCIE_APPL_RDLH_LINKUP		0x074
+#define EXYNOS_PCIE_APPL_XMLH_LINKUP		BIT(4)
+#define EXYNOS_PCIE_APPL_LTSSM_ENABLE		0x1
+#define EXYNOS_PCIE_APPL_SLV_AWMISC		0x11c
+#define EXYNOS_PCIE_APPL_SLV_ARMISC		0x120
+#define EXYNOS_PCIE_APPL_SLV_DBI_ENABLE	BIT(21)
 
 struct exynos_pcie {
 	struct dw_pcie			pci;
-	void __iomem			*elbi_base;
+	void __iomem			*appl_base;
 	struct clk_bulk_data		*clks;
 	int				clk_cnt;
 	struct phy			*phy;
@@ -93,49 +93,49 @@ static void exynos_pcie_sideband_dbi_w_mode(struct exynos_pcie *ep, bool on)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_ELBI_SLV_AWMISC);
+	val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_APPL_SLV_AWMISC);
 	if (on)
-		val |= EXYNOS_PCIE_ELBI_SLV_DBI_ENABLE;
+		val |= EXYNOS_PCIE_APPL_SLV_DBI_ENABLE;
 	else
-		val &= ~EXYNOS_PCIE_ELBI_SLV_DBI_ENABLE;
-	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_ELBI_SLV_AWMISC);
+		val &= ~EXYNOS_PCIE_APPL_SLV_DBI_ENABLE;
+	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_APPL_SLV_AWMISC);
 }
 
 static void exynos_pcie_sideband_dbi_r_mode(struct exynos_pcie *ep, bool on)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_ELBI_SLV_ARMISC);
+	val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_APPL_SLV_ARMISC);
 	if (on)
-		val |= EXYNOS_PCIE_ELBI_SLV_DBI_ENABLE;
+		val |= EXYNOS_PCIE_APPL_SLV_DBI_ENABLE;
 	else
-		val &= ~EXYNOS_PCIE_ELBI_SLV_DBI_ENABLE;
-	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_ELBI_SLV_ARMISC);
+		val &= ~EXYNOS_PCIE_APPL_SLV_DBI_ENABLE;
+	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_APPL_SLV_ARMISC);
 }
 
 static void exynos_pcie_assert_core_reset(struct exynos_pcie *ep)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_CORE_RESET);
+	val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_CORE_RESET);
 	val &= ~EXYNOS_PCIE_CORE_RESET_ENABLE;
-	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_CORE_RESET);
-	exynos_pcie_writel(ep->elbi_base, 0, EXYNOS_PCIE_STICKY_RESET);
-	exynos_pcie_writel(ep->elbi_base, 0, EXYNOS_PCIE_NONSTICKY_RESET);
+	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_CORE_RESET);
+	exynos_pcie_writel(ep->appl_base, 0, EXYNOS_PCIE_STICKY_RESET);
+	exynos_pcie_writel(ep->appl_base, 0, EXYNOS_PCIE_NONSTICKY_RESET);
 }
 
 static void exynos_pcie_deassert_core_reset(struct exynos_pcie *ep)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_CORE_RESET);
+	val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_CORE_RESET);
 	val |= EXYNOS_PCIE_CORE_RESET_ENABLE;
 
-	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_CORE_RESET);
-	exynos_pcie_writel(ep->elbi_base, 1, EXYNOS_PCIE_STICKY_RESET);
-	exynos_pcie_writel(ep->elbi_base, 1, EXYNOS_PCIE_NONSTICKY_RESET);
-	exynos_pcie_writel(ep->elbi_base, 1, EXYNOS_PCIE_APP_INIT_RESET);
-	exynos_pcie_writel(ep->elbi_base, 0, EXYNOS_PCIE_APP_INIT_RESET);
+	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_CORE_RESET);
+	exynos_pcie_writel(ep->appl_base, 1, EXYNOS_PCIE_STICKY_RESET);
+	exynos_pcie_writel(ep->appl_base, 1, EXYNOS_PCIE_NONSTICKY_RESET);
+	exynos_pcie_writel(ep->appl_base, 1, EXYNOS_PCIE_APP_INIT_RESET);
+	exynos_pcie_writel(ep->appl_base, 0, EXYNOS_PCIE_APP_INIT_RESET);
 }
 
 static int exynos_pcie_start_link(struct dw_pcie *pci)
@@ -143,21 +143,21 @@ static int exynos_pcie_start_link(struct dw_pcie *pci)
 	struct exynos_pcie *ep = to_exynos_pcie(pci);
 	u32 val;
 
-	val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_SW_WAKE);
+	val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_SW_WAKE);
 	val &= ~EXYNOS_PCIE_BUS_EN;
-	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_SW_WAKE);
+	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_SW_WAKE);
 
 	/* assert LTSSM enable */
-	exynos_pcie_writel(ep->elbi_base, EXYNOS_PCIE_ELBI_LTSSM_ENABLE,
+	exynos_pcie_writel(ep->appl_base, EXYNOS_PCIE_APPL_LTSSM_ENABLE,
 			  EXYNOS_PCIE_APP_LTSSM_ENABLE);
 	return 0;
 }
 
 static void exynos_pcie_clear_irq_pulse(struct exynos_pcie *ep)
 {
-	u32 val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_IRQ_PULSE);
+	u32 val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_IRQ_PULSE);
 
-	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_IRQ_PULSE);
+	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_IRQ_PULSE);
 }
 
 static irqreturn_t exynos_pcie_irq_handler(int irq, void *arg)
@@ -173,9 +173,9 @@ static void exynos_pcie_enable_irq_pulse(struct exynos_pcie *ep)
 	u32 val = EXYNOS_IRQ_INTA_ASSERT | EXYNOS_IRQ_INTB_ASSERT |
 		  EXYNOS_IRQ_INTC_ASSERT | EXYNOS_IRQ_INTD_ASSERT;
 
-	exynos_pcie_writel(ep->elbi_base, val, EXYNOS_PCIE_IRQ_EN_PULSE);
-	exynos_pcie_writel(ep->elbi_base, 0, EXYNOS_PCIE_IRQ_EN_LEVEL);
-	exynos_pcie_writel(ep->elbi_base, 0, EXYNOS_PCIE_IRQ_EN_SPECIAL);
+	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_IRQ_EN_PULSE);
+	exynos_pcie_writel(ep->appl_base, 0, EXYNOS_PCIE_IRQ_EN_LEVEL);
+	exynos_pcie_writel(ep->appl_base, 0, EXYNOS_PCIE_IRQ_EN_SPECIAL);
 }
 
 static u32 exynos_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
@@ -232,9 +232,9 @@ static struct pci_ops exynos_pci_ops = {
 static int exynos_pcie_link_up(struct dw_pcie *pci)
 {
 	struct exynos_pcie *ep = to_exynos_pcie(pci);
-	u32 val = exynos_pcie_readl(ep->elbi_base, EXYNOS_PCIE_ELBI_RDLH_LINKUP);
+	u32 val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_APPL_RDLH_LINKUP);
 
-	return (val & EXYNOS_PCIE_ELBI_XMLH_LINKUP);
+	return (val & EXYNOS_PCIE_APPL_XMLH_LINKUP);
 }
 
 static int exynos_pcie_host_init(struct dw_pcie_rp *pp)
@@ -315,10 +315,10 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(ep->phy))
 		return PTR_ERR(ep->phy);
 
-	/* External Local Bus interface (ELBI) registers */
-	ep->elbi_base = devm_platform_ioremap_resource_byname(pdev, "elbi");
-	if (IS_ERR(ep->elbi_base))
-		return PTR_ERR(ep->elbi_base);
+	/* Application logic registers */
+	ep->appl_base = devm_platform_ioremap_resource_byname(pdev, "appl");
+	if (IS_ERR(ep->appl_base))
+		return PTR_ERR(ep->appl_base);
 
 	ret = exynos_pcie_init_clk_resources(ep);
 	if (ret < 0)
-- 
2.17.1

[PATCH 08/16] PCI: samsung: Rename exynos_pcie to samsung_pcie

[Shradha Todi]

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 {
 	struct dw_pcie			pci;
 	void __iomem			*appl_base;
 	struct clk_bulk_data		*clks;
@@ -60,23 +60,23 @@ struct exynos_pcie {
 	struct regulator_bulk_data	supplies[2];
 };
 
-static int exynos_pcie_init_clk_resources(struct exynos_pcie *ep)
+static int exynos_pcie_init_clk_resources(struct samsung_pcie *sp)
 {
-	struct device *dev = ep->pci.dev;
+	struct device *dev = sp->pci.dev;
 	int ret;
 
-	ret = devm_clk_bulk_get_all(dev, &ep->clks);
+	ret = devm_clk_bulk_get_all(dev, &sp->clks);
 	if (ret < 0)
 		return ret;
 
-	ep->clk_cnt = ret;
+	sp->clk_cnt = ret;
 
-	return clk_bulk_prepare_enable(ep->clk_cnt, ep->clks);
+	return clk_bulk_prepare_enable(sp->clk_cnt, sp->clks);
 }
 
-static void exynos_pcie_deinit_clk_resources(struct exynos_pcie *ep)
+static void exynos_pcie_deinit_clk_resources(struct samsung_pcie *sp)
 {
-	clk_bulk_disable_unprepare(ep->clk_cnt, ep->clks);
+	clk_bulk_disable_unprepare(sp->clk_cnt, sp->clks);
 }
 
 static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
@@ -89,115 +89,115 @@ static u32 exynos_pcie_readl(void __iomem *base, u32 reg)
 	return readl(base + reg);
 }
 
-static void exynos_pcie_sideband_dbi_w_mode(struct exynos_pcie *ep, bool on)
+static void exynos_pcie_sideband_dbi_w_mode(struct samsung_pcie *sp, bool on)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_APPL_SLV_AWMISC);
+	val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_APPL_SLV_AWMISC);
 	if (on)
 		val |= EXYNOS_PCIE_APPL_SLV_DBI_ENABLE;
 	else
 		val &= ~EXYNOS_PCIE_APPL_SLV_DBI_ENABLE;
-	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_APPL_SLV_AWMISC);
+	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_APPL_SLV_AWMISC);
 }
 
-static void exynos_pcie_sideband_dbi_r_mode(struct exynos_pcie *ep, bool on)
+static void exynos_pcie_sideband_dbi_r_mode(struct samsung_pcie *sp, bool on)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_APPL_SLV_ARMISC);
+	val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_APPL_SLV_ARMISC);
 	if (on)
 		val |= EXYNOS_PCIE_APPL_SLV_DBI_ENABLE;
 	else
 		val &= ~EXYNOS_PCIE_APPL_SLV_DBI_ENABLE;
-	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_APPL_SLV_ARMISC);
+	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_APPL_SLV_ARMISC);
 }
 
-static void exynos_pcie_assert_core_reset(struct exynos_pcie *ep)
+static void exynos_pcie_assert_core_reset(struct samsung_pcie *sp)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_CORE_RESET);
+	val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_CORE_RESET);
 	val &= ~EXYNOS_PCIE_CORE_RESET_ENABLE;
-	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_CORE_RESET);
-	exynos_pcie_writel(ep->appl_base, 0, EXYNOS_PCIE_STICKY_RESET);
-	exynos_pcie_writel(ep->appl_base, 0, EXYNOS_PCIE_NONSTICKY_RESET);
+	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_CORE_RESET);
+	exynos_pcie_writel(sp->appl_base, 0, EXYNOS_PCIE_STICKY_RESET);
+	exynos_pcie_writel(sp->appl_base, 0, EXYNOS_PCIE_NONSTICKY_RESET);
 }
 
-static void exynos_pcie_deassert_core_reset(struct exynos_pcie *ep)
+static void exynos_pcie_deassert_core_reset(struct samsung_pcie *sp)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_CORE_RESET);
+	val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_CORE_RESET);
 	val |= EXYNOS_PCIE_CORE_RESET_ENABLE;
 
-	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_CORE_RESET);
-	exynos_pcie_writel(ep->appl_base, 1, EXYNOS_PCIE_STICKY_RESET);
-	exynos_pcie_writel(ep->appl_base, 1, EXYNOS_PCIE_NONSTICKY_RESET);
-	exynos_pcie_writel(ep->appl_base, 1, EXYNOS_PCIE_APP_INIT_RESET);
-	exynos_pcie_writel(ep->appl_base, 0, EXYNOS_PCIE_APP_INIT_RESET);
+	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_CORE_RESET);
+	exynos_pcie_writel(sp->appl_base, 1, EXYNOS_PCIE_STICKY_RESET);
+	exynos_pcie_writel(sp->appl_base, 1, EXYNOS_PCIE_NONSTICKY_RESET);
+	exynos_pcie_writel(sp->appl_base, 1, EXYNOS_PCIE_APP_INIT_RESET);
+	exynos_pcie_writel(sp->appl_base, 0, EXYNOS_PCIE_APP_INIT_RESET);
 }
 
 static int exynos_pcie_start_link(struct dw_pcie *pci)
 {
-	struct exynos_pcie *ep = to_exynos_pcie(pci);
+	struct samsung_pcie *sp = to_samsung_pcie(pci);
 	u32 val;
 
-	val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_SW_WAKE);
+	val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_SW_WAKE);
 	val &= ~EXYNOS_PCIE_BUS_EN;
-	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_SW_WAKE);
+	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_SW_WAKE);
 
 	/* assert LTSSM enable */
-	exynos_pcie_writel(ep->appl_base, EXYNOS_PCIE_APPL_LTSSM_ENABLE,
+	exynos_pcie_writel(sp->appl_base, EXYNOS_PCIE_APPL_LTSSM_ENABLE,
 			  EXYNOS_PCIE_APP_LTSSM_ENABLE);
 	return 0;
 }
 
-static void exynos_pcie_clear_irq_pulse(struct exynos_pcie *ep)
+static void exynos_pcie_clear_irq_pulse(struct samsung_pcie *sp)
 {
-	u32 val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_IRQ_PULSE);
+	u32 val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_IRQ_PULSE);
 
-	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_IRQ_PULSE);
+	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_IRQ_PULSE);
 }
 
 static irqreturn_t exynos_pcie_irq_handler(int irq, void *arg)
 {
-	struct exynos_pcie *ep = arg;
+	struct samsung_pcie *sp = arg;
 
-	exynos_pcie_clear_irq_pulse(ep);
+	exynos_pcie_clear_irq_pulse(sp);
 	return IRQ_HANDLED;
 }
 
-static void exynos_pcie_enable_irq_pulse(struct exynos_pcie *ep)
+static void exynos_pcie_enable_irq_pulse(struct samsung_pcie *sp)
 {
 	u32 val = EXYNOS_IRQ_INTA_ASSERT | EXYNOS_IRQ_INTB_ASSERT |
 		  EXYNOS_IRQ_INTC_ASSERT | EXYNOS_IRQ_INTD_ASSERT;
 
-	exynos_pcie_writel(ep->appl_base, val, EXYNOS_PCIE_IRQ_EN_PULSE);
-	exynos_pcie_writel(ep->appl_base, 0, EXYNOS_PCIE_IRQ_EN_LEVEL);
-	exynos_pcie_writel(ep->appl_base, 0, EXYNOS_PCIE_IRQ_EN_SPECIAL);
+	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_IRQ_EN_PULSE);
+	exynos_pcie_writel(sp->appl_base, 0, EXYNOS_PCIE_IRQ_EN_LEVEL);
+	exynos_pcie_writel(sp->appl_base, 0, EXYNOS_PCIE_IRQ_EN_SPECIAL);
 }
 
 static u32 exynos_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
 				u32 reg, size_t size)
 {
-	struct exynos_pcie *ep = to_exynos_pcie(pci);
+	struct samsung_pcie *sp = to_samsung_pcie(pci);
 	u32 val;
 
-	exynos_pcie_sideband_dbi_r_mode(ep, true);
+	exynos_pcie_sideband_dbi_r_mode(sp, true);
 	dw_pcie_read(base + reg, size, &val);
-	exynos_pcie_sideband_dbi_r_mode(ep, false);
+	exynos_pcie_sideband_dbi_r_mode(sp, false);
 	return val;
 }
 
 static void exynos_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
 				  u32 reg, size_t size, u32 val)
 {
-	struct exynos_pcie *ep = to_exynos_pcie(pci);
+	struct samsung_pcie *sp = to_samsung_pcie(pci);
 
-	exynos_pcie_sideband_dbi_w_mode(ep, true);
+	exynos_pcie_sideband_dbi_w_mode(sp, true);
 	dw_pcie_write(base + reg, size, val);
-	exynos_pcie_sideband_dbi_w_mode(ep, false);
+	exynos_pcie_sideband_dbi_w_mode(sp, false);
 }
 
 static int exynos_pcie_rd_own_conf(struct pci_bus *bus, unsigned int devfn,
@@ -231,8 +231,8 @@ static struct pci_ops exynos_pci_ops = {
 
 static int exynos_pcie_link_up(struct dw_pcie *pci)
 {
-	struct exynos_pcie *ep = to_exynos_pcie(pci);
-	u32 val = exynos_pcie_readl(ep->appl_base, EXYNOS_PCIE_APPL_RDLH_LINKUP);
+	struct samsung_pcie *sp = to_samsung_pcie(pci);
+	u32 val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_APPL_RDLH_LINKUP);
 
 	return (val & EXYNOS_PCIE_APPL_XMLH_LINKUP);
 }
@@ -240,17 +240,17 @@ static int exynos_pcie_link_up(struct dw_pcie *pci)
 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);
+	struct samsung_pcie *sp = to_samsung_pcie(pci);
 
 	pp->bridge->ops = &exynos_pci_ops;
 
-	exynos_pcie_assert_core_reset(ep);
+	exynos_pcie_assert_core_reset(sp);
 
-	phy_init(ep->phy);
-	phy_power_on(ep->phy);
+	phy_init(sp->phy);
+	phy_power_on(sp->phy);
 
-	exynos_pcie_deassert_core_reset(ep);
-	exynos_pcie_enable_irq_pulse(ep);
+	exynos_pcie_deassert_core_reset(sp);
+	exynos_pcie_enable_irq_pulse(sp);
 
 	return 0;
 }
@@ -259,10 +259,10 @@ static const struct dw_pcie_host_ops exynos_pcie_host_ops = {
 	.host_init = exynos_pcie_host_init,
 };
 
-static int exynos_add_pcie_port(struct exynos_pcie *ep,
+static int exynos_add_pcie_port(struct samsung_pcie *sp,
 				       struct platform_device *pdev)
 {
-	struct dw_pcie *pci = &ep->pci;
+	struct dw_pcie *pci = &sp->pci;
 	struct dw_pcie_rp *pp = &pci->pp;
 	struct device *dev = &pdev->dev;
 	int ret;
@@ -272,7 +272,7 @@ static int exynos_add_pcie_port(struct exynos_pcie *ep,
 		return pp->irq;
 
 	ret = devm_request_irq(dev, pp->irq, exynos_pcie_irq_handler,
-			       IRQF_SHARED, "exynos-pcie", ep);
+			       IRQF_SHARED, "exynos-pcie", sp);
 	if (ret) {
 		dev_err(dev, "failed to request irq\n");
 		return ret;
@@ -300,95 +300,95 @@ static const struct dw_pcie_ops dw_pcie_ops = {
 static int exynos_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct exynos_pcie *ep;
+	struct samsung_pcie *sp;
 	struct device_node *np = dev->of_node;
 	int ret;
 
-	ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
-	if (!ep)
+	sp = devm_kzalloc(dev, sizeof(*sp), GFP_KERNEL);
+	if (!sp)
 		return -ENOMEM;
 
-	ep->pci.dev = dev;
-	ep->pci.ops = &dw_pcie_ops;
+	sp->pci.dev = dev;
+	sp->pci.ops = &dw_pcie_ops;
 
-	ep->phy = devm_of_phy_get(dev, np, NULL);
-	if (IS_ERR(ep->phy))
-		return PTR_ERR(ep->phy);
+	sp->phy = devm_of_phy_get(dev, np, NULL);
+	if (IS_ERR(sp->phy))
+		return PTR_ERR(sp->phy);
 
 	/* Application logic registers */
-	ep->appl_base = devm_platform_ioremap_resource_byname(pdev, "appl");
-	if (IS_ERR(ep->appl_base))
-		return PTR_ERR(ep->appl_base);
+	sp->appl_base = devm_platform_ioremap_resource_byname(pdev, "appl");
+	if (IS_ERR(sp->appl_base))
+		return PTR_ERR(sp->appl_base);
 
-	ret = exynos_pcie_init_clk_resources(ep);
+	ret = exynos_pcie_init_clk_resources(sp);
 	if (ret < 0)
 		return ret;
 
-	ep->supplies[0].supply = "vdd18";
-	ep->supplies[1].supply = "vdd10";
-	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ep->supplies),
-				      ep->supplies);
+	sp->supplies[0].supply = "vdd18";
+	sp->supplies[1].supply = "vdd10";
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sp->supplies),
+				      sp->supplies);
 	if (ret)
 		return ret;
 
-	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
+	ret = regulator_bulk_enable(ARRAY_SIZE(sp->supplies), sp->supplies);
 	if (ret)
 		return ret;
 
-	platform_set_drvdata(pdev, ep);
+	platform_set_drvdata(pdev, sp);
 
-	ret = exynos_add_pcie_port(ep, pdev);
+	ret = exynos_add_pcie_port(sp, pdev);
 	if (ret < 0)
 		goto fail_probe;
 
 	return 0;
 
 fail_probe:
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
-	exynos_pcie_deinit_clk_resources(ep);
+	phy_exit(sp->phy);
+	regulator_bulk_disable(ARRAY_SIZE(sp->supplies), sp->supplies);
+	exynos_pcie_deinit_clk_resources(sp);
 
 	return ret;
 }
 
 static int __exit exynos_pcie_remove(struct platform_device *pdev)
 {
-	struct exynos_pcie *ep = platform_get_drvdata(pdev);
+	struct samsung_pcie *sp = platform_get_drvdata(pdev);
 
-	dw_pcie_host_deinit(&ep->pci.pp);
-	exynos_pcie_assert_core_reset(ep);
-	phy_power_off(ep->phy);
-	phy_exit(ep->phy);
-	exynos_pcie_deinit_clk_resources(ep);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+	dw_pcie_host_deinit(&sp->pci.pp);
+	exynos_pcie_assert_core_reset(sp);
+	phy_power_off(sp->phy);
+	phy_exit(sp->phy);
+	exynos_pcie_deinit_clk_resources(sp);
+	regulator_bulk_disable(ARRAY_SIZE(sp->supplies), sp->supplies);
 
 	return 0;
 }
 
 static int exynos_pcie_suspend_noirq(struct device *dev)
 {
-	struct exynos_pcie *ep = dev_get_drvdata(dev);
+	struct samsung_pcie *sp = dev_get_drvdata(dev);
 
-	exynos_pcie_assert_core_reset(ep);
-	phy_power_off(ep->phy);
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+	exynos_pcie_assert_core_reset(sp);
+	phy_power_off(sp->phy);
+	phy_exit(sp->phy);
+	regulator_bulk_disable(ARRAY_SIZE(sp->supplies), sp->supplies);
 
 	return 0;
 }
 
 static int exynos_pcie_resume_noirq(struct device *dev)
 {
-	struct exynos_pcie *ep = dev_get_drvdata(dev);
-	struct dw_pcie *pci = &ep->pci;
+	struct samsung_pcie *sp = dev_get_drvdata(dev);
+	struct dw_pcie *pci = &sp->pci;
 	struct dw_pcie_rp *pp = &pci->pp;
 	int ret;
 
-	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
+	ret = regulator_bulk_enable(ARRAY_SIZE(sp->supplies), sp->supplies);
 	if (ret)
 		return ret;
 
-	/* exynos_pcie_host_init controls ep->phy */
+	/* exynos_pcie_host_init controls sp->phy */
 	exynos_pcie_host_init(pp);
 	dw_pcie_setup_rc(pp);
 	exynos_pcie_start_link(pci);
-- 
2.17.1

Re: [PATCH 08/16] PCI: samsung: Rename exynos_pcie to samsung_pcie

[Krzysztof Kozlowski]

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

RE: [PATCH 08/16] PCI: samsung: Rename exynos_pcie to samsung_pcie

[Pankaj Dubey]

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

Re: [PATCH 08/16] PCI: samsung: Rename exynos_pcie to samsung_pcie

[Krzysztof Kozlowski]

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

[PATCH 09/16] PCI: samsung: Make common appl readl/writel functions

[Shradha Todi]

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)
 {
-	writel(val, base + reg);
+	writel(val, sp->appl_base + reg);
 }
 
-static u32 exynos_pcie_readl(void __iomem *base, u32 reg)
+static unsigned int samsung_pcie_appl_readl(struct samsung_pcie *sp, u32 reg)
 {
-	return readl(base + reg);
+	return readl(sp->appl_base + reg);
 }
 
 static void exynos_pcie_sideband_dbi_w_mode(struct samsung_pcie *sp, bool on)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_APPL_SLV_AWMISC);
+	val = samsung_pcie_appl_readl(sp, EXYNOS_PCIE_APPL_SLV_AWMISC);
 	if (on)
 		val |= EXYNOS_PCIE_APPL_SLV_DBI_ENABLE;
 	else
 		val &= ~EXYNOS_PCIE_APPL_SLV_DBI_ENABLE;
-	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_APPL_SLV_AWMISC);
+	samsung_pcie_appl_writel(sp, val, EXYNOS_PCIE_APPL_SLV_AWMISC);
 }
 
 static void exynos_pcie_sideband_dbi_r_mode(struct samsung_pcie *sp, bool on)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_APPL_SLV_ARMISC);
+	val = samsung_pcie_appl_readl(sp, EXYNOS_PCIE_APPL_SLV_ARMISC);
 	if (on)
 		val |= EXYNOS_PCIE_APPL_SLV_DBI_ENABLE;
 	else
 		val &= ~EXYNOS_PCIE_APPL_SLV_DBI_ENABLE;
-	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_APPL_SLV_ARMISC);
+	samsung_pcie_appl_writel(sp, val, EXYNOS_PCIE_APPL_SLV_ARMISC);
 }
 
 static void exynos_pcie_assert_core_reset(struct samsung_pcie *sp)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_CORE_RESET);
+	val = samsung_pcie_appl_readl(sp, EXYNOS_PCIE_CORE_RESET);
 	val &= ~EXYNOS_PCIE_CORE_RESET_ENABLE;
-	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_CORE_RESET);
-	exynos_pcie_writel(sp->appl_base, 0, EXYNOS_PCIE_STICKY_RESET);
-	exynos_pcie_writel(sp->appl_base, 0, EXYNOS_PCIE_NONSTICKY_RESET);
+	samsung_pcie_appl_writel(sp, val, EXYNOS_PCIE_CORE_RESET);
+	samsung_pcie_appl_writel(sp, 0, EXYNOS_PCIE_STICKY_RESET);
+	samsung_pcie_appl_writel(sp, 0, EXYNOS_PCIE_NONSTICKY_RESET);
 }
 
 static void exynos_pcie_deassert_core_reset(struct samsung_pcie *sp)
 {
 	u32 val;
 
-	val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_CORE_RESET);
+	val = samsung_pcie_appl_readl(sp, EXYNOS_PCIE_CORE_RESET);
 	val |= EXYNOS_PCIE_CORE_RESET_ENABLE;
 
-	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_CORE_RESET);
-	exynos_pcie_writel(sp->appl_base, 1, EXYNOS_PCIE_STICKY_RESET);
-	exynos_pcie_writel(sp->appl_base, 1, EXYNOS_PCIE_NONSTICKY_RESET);
-	exynos_pcie_writel(sp->appl_base, 1, EXYNOS_PCIE_APP_INIT_RESET);
-	exynos_pcie_writel(sp->appl_base, 0, EXYNOS_PCIE_APP_INIT_RESET);
+	samsung_pcie_appl_writel(sp, val, EXYNOS_PCIE_CORE_RESET);
+	samsung_pcie_appl_writel(sp, 1, EXYNOS_PCIE_STICKY_RESET);
+	samsung_pcie_appl_writel(sp, 1, EXYNOS_PCIE_NONSTICKY_RESET);
+	samsung_pcie_appl_writel(sp, 1, EXYNOS_PCIE_APP_INIT_RESET);
+	samsung_pcie_appl_writel(sp, 0, EXYNOS_PCIE_APP_INIT_RESET);
 }
 
 static int exynos_pcie_start_link(struct dw_pcie *pci)
@@ -143,21 +143,21 @@ static int exynos_pcie_start_link(struct dw_pcie *pci)
 	struct samsung_pcie *sp = to_samsung_pcie(pci);
 	u32 val;
 
-	val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_SW_WAKE);
+	val = samsung_pcie_appl_readl(sp, EXYNOS_PCIE_SW_WAKE);
 	val &= ~EXYNOS_PCIE_BUS_EN;
-	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_SW_WAKE);
+	samsung_pcie_appl_writel(sp, val, EXYNOS_PCIE_SW_WAKE);
 
 	/* assert LTSSM enable */
-	exynos_pcie_writel(sp->appl_base, EXYNOS_PCIE_APPL_LTSSM_ENABLE,
+	samsung_pcie_appl_writel(sp, EXYNOS_PCIE_APPL_LTSSM_ENABLE,
 			  EXYNOS_PCIE_APP_LTSSM_ENABLE);
 	return 0;
 }
 
 static void exynos_pcie_clear_irq_pulse(struct samsung_pcie *sp)
 {
-	u32 val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_IRQ_PULSE);
+	u32 val = samsung_pcie_appl_readl(sp, EXYNOS_PCIE_IRQ_PULSE);
 
-	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_IRQ_PULSE);
+	samsung_pcie_appl_writel(sp, val, EXYNOS_PCIE_IRQ_PULSE);
 }
 
 static irqreturn_t exynos_pcie_irq_handler(int irq, void *arg)
@@ -173,9 +173,9 @@ static void exynos_pcie_enable_irq_pulse(struct samsung_pcie *sp)
 	u32 val = EXYNOS_IRQ_INTA_ASSERT | EXYNOS_IRQ_INTB_ASSERT |
 		  EXYNOS_IRQ_INTC_ASSERT | EXYNOS_IRQ_INTD_ASSERT;
 
-	exynos_pcie_writel(sp->appl_base, val, EXYNOS_PCIE_IRQ_EN_PULSE);
-	exynos_pcie_writel(sp->appl_base, 0, EXYNOS_PCIE_IRQ_EN_LEVEL);
-	exynos_pcie_writel(sp->appl_base, 0, EXYNOS_PCIE_IRQ_EN_SPECIAL);
+	samsung_pcie_appl_writel(sp, val, EXYNOS_PCIE_IRQ_EN_PULSE);
+	samsung_pcie_appl_writel(sp, 0, EXYNOS_PCIE_IRQ_EN_LEVEL);
+	samsung_pcie_appl_writel(sp, 0, EXYNOS_PCIE_IRQ_EN_SPECIAL);
 }
 
 static u32 exynos_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
@@ -232,7 +232,7 @@ static struct pci_ops exynos_pci_ops = {
 static int exynos_pcie_link_up(struct dw_pcie *pci)
 {
 	struct samsung_pcie *sp = to_samsung_pcie(pci);
-	u32 val = exynos_pcie_readl(sp->appl_base, EXYNOS_PCIE_APPL_RDLH_LINKUP);
+	u32 val = samsung_pcie_appl_readl(sp, EXYNOS_PCIE_APPL_RDLH_LINKUP);
 
 	return (val & EXYNOS_PCIE_APPL_XMLH_LINKUP);
 }
-- 
2.17.1

Re: [PATCH 09/16] PCI: samsung: Make common appl readl/writel functions

[Krzysztof Kozlowski]

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

RE: [PATCH 09/16] PCI: samsung: Make common appl readl/writel functions

[Pankaj Dubey]

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

[PATCH 10/16] dt-bindings: PCI: Add phy-names as required property

[Shradha Todi]

To replace devm_of_phy_get with devm_phy_get to get the
PHY pointer using non DT version, we need to add phy-names
property in DT and make it a required property with const
value.

Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 Documentation/devicetree/bindings/pci/samsung,pcie.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/samsung,pcie.yaml b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
index 9c58c4d1f6a7..11feff9d6526 100644
--- a/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/samsung,pcie.yaml
@@ -50,6 +50,10 @@ properties:
   phys:
     maxItems: 1
 
+  phy-names:
+    items:
+      - const: pcie_phy
+
   vdd10-supply:
     description:
       Phandle to a regulator that provides 1.0V power to the PCIe block.
@@ -81,6 +85,7 @@ required:
   - clocks
   - clock-names
   - phys
+  - phy-names
   - vdd10-supply
   - vdd18-supply
 
-- 
2.17.1

Re: [PATCH 10/16] dt-bindings: PCI: Add phy-names as required property

[Rob Herring]

On Tue, 14 Feb 2023 17:43:27 +0530, Shradha Todi wrote:
> To replace devm_of_phy_get with devm_phy_get to get the
> PHY pointer using non DT version, we need to add phy-names
> property in DT and make it a required property with const
> value.
> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---
>  Documentation/devicetree/bindings/pci/samsung,pcie.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.example.dtb: pcie@15700000: 'phy-names' is a required property
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230214121333.1837-11-shradha.t@samsung.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

RE: [PATCH 10/16] dt-bindings: PCI: Add phy-names as required property

[Shradha Todi]

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: 14 February 2023 21:43
> To: Shradha Todi <shradha.t@samsung.com>
> Cc: pankaj.dubey@samsung.com; linux-pci@vger.kernel.org;
> jingoohan1@gmail.com; jh80.chung@samsung.co; linux-arm-
> kernel@lists.infradead.org; tglx@linutronix.de; linux-
> kernel@vger.kernel.org; hongxing.zhu@nxp.com;
> alim.akhtar@samsung.com; bhelgaas@google.com;
> krzysztof.kozlowski+dt@linaro.org; m.szyprowski@samsung.com;
> kw@linux.com; devicetree@vger.kernel.org;
> Sergey.Semin@baikalelectronics.ru; linux-samsung-soc@vger.kernel.org;
> lukas.bulwahn@gmail.com; lpieralisi@kernel.org
> Subject: Re: [PATCH 10/16] dt-bindings: PCI: Add phy-names as required
> property
> 
> 
> On Tue, 14 Feb 2023 17:43:27 +0530, Shradha Todi wrote:
> > To replace devm_of_phy_get with devm_phy_get to get the PHY pointer
> > using non DT version, we need to add phy-names property in DT and make
> > it a required property with const value.
> >
> > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> > ---
> >  Documentation/devicetree/bindings/pci/samsung,pcie.yaml | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m
> dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/pci/samsung,exynos-
> pcie.example.dtb: pcie@15700000: 'phy-names' is a required property
> 	From schema: /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
> 
> doc reference errors (make refcheckdocs):
> 
> See https://protect2.fireeye.com/v1/url?k=788bbab7-19f652f0-788a31f8-
> 74fe485fff30-c0e59906b0eee0e1&q=1&e=33c2cefb-9526-43d7-9446-
> b159854d2369&u=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fdev
> icetree-bindings%2Fpatch%2F20230214121333.1837-11-
> shradha.t%40samsung.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above error(s),
> then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your
> schema.

Thanks for review. Sorry, I missed to update the example. Anyway I will get rid of this patch all together!

Re: [PATCH 10/16] dt-bindings: PCI: Add phy-names as required property

[Rob Herring]

On Tue, Feb 14, 2023 at 05:43:27PM +0530, Shradha Todi wrote:
> To replace devm_of_phy_get with devm_phy_get to get the
> PHY pointer using non DT version, we need to add phy-names
> property in DT and make it a required property with const
> value.

Also an ABI break.

And unnecessary. You don't need a name with a single entry. Pretty sure 
I fixed that for phy_get at some point.

Rob

RE: [PATCH 10/16] dt-bindings: PCI: Add phy-names as required property

[Shradha Todi]

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: 15 February 2023 00:48
> To: Shradha Todi <shradha.t@samsung.com>
> Cc: lpieralisi@kernel.org; kw@linux.com; 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; 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 10/16] dt-bindings: PCI: Add phy-names as required
> property
> 
> On Tue, Feb 14, 2023 at 05:43:27PM +0530, Shradha Todi wrote:
> > To replace devm_of_phy_get with devm_phy_get to get the PHY pointer
> > using non DT version, we need to add phy-names property in DT and make
> > it a required property with const value.
> 
> Also an ABI break.
> 
> And unnecessary. You don't need a name with a single entry. Pretty sure I
> fixed that for phy_get at some point.
> 

Thanks, I got your point. I will drop this. As of now we have only single
PHYs for Samsung and it can be handled
via phy_get API without a name as you highlighted.

Shradha

> Rob

[PATCH 11/16] arm64: dts: exynos: Add phy-names as DT property

[Shradha Todi]

To replace devm_of_phy_get with devm_phy_get to get the
PHY pointer using non DT version, we need to add phy-names
property in DT.

Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 arch/arm64/boot/dts/exynos/exynos5433.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
index 96b216099594..f50167923fd8 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
@@ -1957,6 +1957,7 @@
 			num-viewport = <3>;
 			bus-range = <0x00 0xff>;
 			phys = <&pcie_phy>;
+			phy-names = "pcie_phy";
 			ranges = <0x81000000 0 0	  0x0c001000 0 0x00010000>,
 				 <0x82000000 0 0x0c011000 0x0c011000 0 0x03feefff>;
 			status = "disabled";
-- 
2.17.1

[PATCH 12/16] PCI: samsung: Get PHY using non-DT version

[Shradha Todi]

Replace devm_of_phy_get with devm_phy_get to get the
PHY pointer.

Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 drivers/pci/controller/dwc/pci-samsung.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-samsung.c b/drivers/pci/controller/dwc/pci-samsung.c
index e6e2a8ab4403..719d284e1552 100644
--- a/drivers/pci/controller/dwc/pci-samsung.c
+++ b/drivers/pci/controller/dwc/pci-samsung.c
@@ -301,7 +301,6 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct samsung_pcie *sp;
-	struct device_node *np = dev->of_node;
 	int ret;
 
 	sp = devm_kzalloc(dev, sizeof(*sp), GFP_KERNEL);
@@ -311,7 +310,7 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	sp->pci.dev = dev;
 	sp->pci.ops = &dw_pcie_ops;
 
-	sp->phy = devm_of_phy_get(dev, np, NULL);
+	sp->phy = devm_phy_get(dev, "pcie_phy");
 	if (IS_ERR(sp->phy))
 		return PTR_ERR(sp->phy);
 
-- 
2.17.1

[PATCH 13/16] PCI: samsung: Rename common functions to samsung

[Shradha Todi]

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)
 {
 	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 void exynos_pcie_deinit_clk_resources(struct samsung_pcie *sp)
+static void samsung_pcie_deinit_clk_resources(struct samsung_pcie *sp)
 {
 	clk_bulk_disable_unprepare(sp->clk_cnt, sp->clks);
 }
@@ -297,7 +297,7 @@ static const struct dw_pcie_ops dw_pcie_ops = {
 	.start_link = exynos_pcie_start_link,
 };
 
-static int exynos_pcie_probe(struct platform_device *pdev)
+static int samsung_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct samsung_pcie *sp;
@@ -319,7 +319,7 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(sp->appl_base))
 		return PTR_ERR(sp->appl_base);
 
-	ret = exynos_pcie_init_clk_resources(sp);
+	ret = samsung_pcie_init_clk_resources(sp);
 	if (ret < 0)
 		return ret;
 
@@ -345,12 +345,12 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 fail_probe:
 	phy_exit(sp->phy);
 	regulator_bulk_disable(ARRAY_SIZE(sp->supplies), sp->supplies);
-	exynos_pcie_deinit_clk_resources(sp);
+	samsung_pcie_deinit_clk_resources(sp);
 
 	return ret;
 }
 
-static int __exit exynos_pcie_remove(struct platform_device *pdev)
+static int __exit samsung_pcie_remove(struct platform_device *pdev)
 {
 	struct samsung_pcie *sp = platform_get_drvdata(pdev);
 
@@ -358,13 +358,13 @@ static int __exit exynos_pcie_remove(struct platform_device *pdev)
 	exynos_pcie_assert_core_reset(sp);
 	phy_power_off(sp->phy);
 	phy_exit(sp->phy);
-	exynos_pcie_deinit_clk_resources(sp);
+	samsung_pcie_deinit_clk_resources(sp);
 	regulator_bulk_disable(ARRAY_SIZE(sp->supplies), sp->supplies);
 
 	return 0;
 }
 
-static int exynos_pcie_suspend_noirq(struct device *dev)
+static int samsung_pcie_suspend_noirq(struct device *dev)
 {
 	struct samsung_pcie *sp = dev_get_drvdata(dev);
 
@@ -376,7 +376,7 @@ static int exynos_pcie_suspend_noirq(struct device *dev)
 	return 0;
 }
 
-static int exynos_pcie_resume_noirq(struct device *dev)
+static int samsung_pcie_resume_noirq(struct device *dev)
 {
 	struct samsung_pcie *sp = dev_get_drvdata(dev);
 	struct dw_pcie *pci = &sp->pci;
@@ -394,25 +394,25 @@ static int exynos_pcie_resume_noirq(struct device *dev)
 	return dw_pcie_wait_for_link(pci);
 }
 
-static const struct dev_pm_ops exynos_pcie_pm_ops = {
-	NOIRQ_SYSTEM_SLEEP_PM_OPS(exynos_pcie_suspend_noirq,
-				  exynos_pcie_resume_noirq)
+static const struct dev_pm_ops samsung_pcie_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(samsung_pcie_suspend_noirq,
+				  samsung_pcie_resume_noirq)
 };
 
-static const struct of_device_id exynos_pcie_of_match[] = {
+static const struct of_device_id samsung_pcie_of_match[] = {
 	{ .compatible = "samsung,exynos5433-pcie", },
 	{ },
 };
 
-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",
+		.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);
-- 
2.17.1

Re: [PATCH 13/16] PCI: samsung: Rename common functions to samsung

[Krzysztof Kozlowski]

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

[PATCH 15/16] PCI: samsung: Add structure to hold resource operations

[Shradha Todi]

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(-)

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[] = {
-- 
2.17.1

Re: [PATCH 15/16] PCI: samsung: Add structure to hold resource operations

[Krzysztof Kozlowski]

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

RE: [PATCH 15/16] PCI: samsung: Add structure to hold resource operations

[Shradha Todi]

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

[PATCH 16/16] PCI: samsung: Make handling of regulators generic

[Shradha Todi]

Use pointers instead of fixed size array to store
regulator related information. Add common regulator
initialization and de-initialization functions.
For platform specific init, add a res_ops function
pointer.

Suggested-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 drivers/pci/controller/dwc/pci-samsung.c | 79 +++++++++++++++++++-----
 1 file changed, 64 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-samsung.c b/drivers/pci/controller/dwc/pci-samsung.c
index 01882f2d06c7..bf18020c14da 100644
--- a/drivers/pci/controller/dwc/pci-samsung.c
+++ b/drivers/pci/controller/dwc/pci-samsung.c
@@ -66,7 +66,8 @@ struct samsung_pcie_pdata {
  * @clks: list of clocks required for the controller
  * @clk_cnt: count of clocks required for the controller
  * @phy: PHY device associated with the controller
- * @supplies: array of regulators required for the controller
+ * @supplies: list of regulators required for the controller
+ * @supplies_cnt: count of regulators required for the controller
  */
 struct samsung_pcie {
 	struct dw_pcie			pci;
@@ -75,10 +76,12 @@ struct samsung_pcie {
 	struct clk_bulk_data		*clks;
 	int				clk_cnt;
 	struct phy			*phy;
-	struct regulator_bulk_data	supplies[2];
+	struct regulator_bulk_data	*supplies;
+	int				supplies_cnt;
 };
 
 struct samsung_res_ops {
+	int (*init_regulator)(struct samsung_pcie *sp);
 	int (*irq_init)(struct samsung_pcie *sp, struct platform_device *pdev);
 };
 
@@ -111,6 +114,34 @@ static unsigned int samsung_pcie_appl_readl(struct samsung_pcie *sp, u32 reg)
 	return readl(sp->appl_base + reg);
 }
 
+static int samsung_regulator_enable(struct samsung_pcie *sp)
+{
+	struct device *dev = sp->pci.dev;
+	int ret;
+
+	if (sp->supplies_cnt == 0)
+		return 0;
+
+	ret = devm_regulator_bulk_get(dev, sp->supplies_cnt,
+				      sp->supplies);
+	if (ret)
+		return ret;
+
+	ret = regulator_bulk_enable(sp->supplies_cnt, sp->supplies);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void samsung_regulator_disable(struct samsung_pcie *sp)
+{
+	if (sp->supplies_cnt == 0)
+		return;
+
+	regulator_bulk_disable(sp->supplies_cnt, sp->supplies);
+}
+
 static void exynos_pcie_sideband_dbi_w_mode(struct samsung_pcie *sp, bool on)
 {
 	u32 val;
@@ -281,6 +312,24 @@ static const struct dw_pcie_host_ops exynos_pcie_host_ops = {
 	.host_init = exynos_pcie_host_init,
 };
 
+static int exynos_init_regulator(struct samsung_pcie *sp)
+{
+	struct device *dev = sp->pci.dev;
+
+	sp->supplies_cnt = 2;
+
+	sp->supplies = devm_kcalloc(dev, sp->supplies_cnt,
+			sizeof(*sp->supplies),
+			GFP_KERNEL);
+	if (!sp->supplies)
+		return -ENOMEM;
+
+	sp->supplies[0].supply = "vdd18";
+	sp->supplies[1].supply = "vdd10";
+
+	return 0;
+}
+
 static int exynos_irq_init(struct samsung_pcie *sp,
 				       struct platform_device *pdev)
 {
@@ -313,6 +362,7 @@ static const struct dw_pcie_ops exynos_dw_pcie_ops = {
 };
 
 static const struct samsung_res_ops exynos_res_ops_data = {
+	.init_regulator		= exynos_init_regulator,
 	.irq_init		= exynos_irq_init,
 };
 
@@ -346,16 +396,15 @@ static int samsung_pcie_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	sp->supplies[0].supply = "vdd18";
-	sp->supplies[1].supply = "vdd10";
-	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sp->supplies),
-				      sp->supplies);
-	if (ret)
-		return ret;
+	if (pdata->res_ops && pdata->res_ops->init_regulator) {
+		ret = sp->pdata->res_ops->init_regulator(sp);
+		if (ret)
+			goto fail_regulator;
+	}
 
-	ret = regulator_bulk_enable(ARRAY_SIZE(sp->supplies), sp->supplies);
+	ret = samsung_regulator_enable(sp);
 	if (ret)
-		return ret;
+		goto fail_regulator;
 
 	platform_set_drvdata(pdev, sp);
 
@@ -372,7 +421,8 @@ static int samsung_pcie_probe(struct platform_device *pdev)
 
 fail_probe:
 	phy_exit(sp->phy);
-	regulator_bulk_disable(ARRAY_SIZE(sp->supplies), sp->supplies);
+	samsung_regulator_disable(sp);
+fail_regulator:
 	samsung_pcie_deinit_clk_resources(sp);
 
 	return ret;
@@ -387,8 +437,7 @@ static int __exit samsung_pcie_remove(struct platform_device *pdev)
 	phy_power_off(sp->phy);
 	phy_exit(sp->phy);
 	samsung_pcie_deinit_clk_resources(sp);
-	regulator_bulk_disable(ARRAY_SIZE(sp->supplies), sp->supplies);
-
+	samsung_regulator_disable(sp);
 	return 0;
 }
 
@@ -399,7 +448,7 @@ static int samsung_pcie_suspend_noirq(struct device *dev)
 	exynos_pcie_assert_core_reset(sp);
 	phy_power_off(sp->phy);
 	phy_exit(sp->phy);
-	regulator_bulk_disable(ARRAY_SIZE(sp->supplies), sp->supplies);
+	samsung_regulator_disable(sp);
 
 	return 0;
 }
@@ -411,7 +460,7 @@ static int samsung_pcie_resume_noirq(struct device *dev)
 	struct dw_pcie_rp *pp = &pci->pp;
 	int ret;
 
-	ret = regulator_bulk_enable(ARRAY_SIZE(sp->supplies), sp->supplies);
+	ret = samsung_regulator_enable(sp);
 	if (ret)
 		return ret;
 
-- 
2.17.1

Re: [PATCH 00/16] Refactor Exynos PCIe driver to make it generic

[Krzysztof Kozlowski]

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

Re: [PATCH 00/16] Refactor Exynos PCIe driver to make it generic

[Serge Semin]

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