Add support Axis, ARTPEC-8 SoC.
ARTPEC-8 is the SoC platform of Axis Communications.
This is based on arm64 and support GEN4 & 2lane.
This PCIe controller is based on DesignWare Hardware core
and uses DesignWare core functions to implement the driver.
changes since v1 :
improvement review comment of Krzysztof on driver code.
-debug messages for probe or other functions.
-Inconsistent coding style (different indentation in structure members).
-Inconsistent code (artpec8_pcie_get_subsystem_resources() gets device
from pdev and from pci so you have two same pointers;
or artpec8_pcie_get_ep_mem_resources() stores dev
as local variable but uses instead pdev->dev).
-Not using devm_platform_ioremap_resource().
-Printing messages in interrupt handlers.
-Several local/static structures or array are not const.
Signed-off-by: Wangseok Lee <[email protected]>
---
drivers/pci/controller/dwc/Kconfig | 31 ++
drivers/pci/controller/dwc/Makefile | 1 +
drivers/pci/controller/dwc/pcie-artpec8.c | 864 ++++++++++++++++++++++++++++++
3 files changed, 896 insertions(+)
create mode 100644 drivers/pci/controller/dwc/pcie-artpec8.c
diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 62ce3ab..4aa6da8 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -222,6 +222,37 @@ config PCIE_ARTPEC6_EP
Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
endpoint mode. This uses the DesignWare core.
+config PCIE_ARTPEC8
+ bool "Axis ARTPEC-8 PCIe controller"
+
+config PCIE_ARTPEC8_HOST
+ bool "Axis ARTPEC-8 PCIe controller Host Mode"
+ depends on ARCH_ARTPEC
+ depends on PCI_MSI_IRQ_DOMAIN
+ depends on PCI_ENDPOINT
+ select PCI_EPF_TEST
+ select PCIE_DW_HOST
+ select PCIE_ARTPEC8
+ help
+ Say 'Y' here to enable support for the PCIe controller in the
+ ARTPEC-8 SoC to work in host mode.
+ This PCIe controller is based on DesignWare Hardware core.
+ And uses DesignWare core functions to implement the driver.
+
+config PCIE_ARTPEC8_EP
+ bool "Axis ARTPEC-8 PCIe controller Endpoint Mode"
+ depends on ARCH_ARTPEC
+ depends on PCI_ENDPOINT
+ depends on PCI_ENDPOINT_CONFIGFS
+ select PCI_EPF_TEST
+ select PCIE_DW_EP
+ select PCIE_ARTPEC8
+ help
+ Say 'Y' here to enable support for the PCIe controller in the
+ ARTPEC-8 SoC to work in endpoint mode.
+ This PCIe controller is based on DesignWare Hardware core.
+ And uses DesignWare core functions to implement the driver.
+
config PCIE_ROCKCHIP_DW_HOST
bool "Rockchip DesignWare PCIe controller"
select PCIE_DW
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 8ba7b67..b361022 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
+obj-$(CONFIG_PCIE_ARTPEC8) += pcie-artpec8.o
# The following drivers are for devices that use the generic ACPI
# pci_root.c driver but don't support standard ECAM config access.
diff --git a/drivers/pci/controller/dwc/pcie-artpec8.c b/drivers/pci/controller/dwc/pcie-artpec8.c
new file mode 100644
index 0000000..d9ae9bf
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-artpec8.c
@@ -0,0 +1,864 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * PCIe controller driver for Axis ARTPEC-8 SoC
+ *
+ * Copyright (C) 2019 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * Author: Jaeho Cho <[email protected]>
+ * This file is based on driver/pci/controller/dwc/pci-exynos.c
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/resource.h>
+#include <linux/types.h>
+#include <linux/phy/phy.h>
+
+#include "pcie-designware.h"
+
+#define to_artpec8_pcie(x) dev_get_drvdata((x)->dev)
+
+/* Gen3 Control Register */
+#define PCIE_GEN3_RELATED_OFF 0x890
+/* Disables equilzation feature */
+#define PCIE_GEN3_EQUALIZATION_DISABLE (0x1 << 16)
+#define PCIE_GEN3_EQ_PHASE_2_3 (0x1 << 9)
+#define PCIE_GEN3_RXEQ_PH01_EN (0x1 << 12)
+#define PCIE_GEN3_RXEQ_RGRDLESS_RXTS (0x1 << 13)
+
+#define FAST_LINK_MODE (7)
+
+/* PCIe ELBI registers */
+#define PCIE_IRQ0_STS 0x000
+#define PCIE_IRQ1_STS 0x004
+#define PCIE_IRQ2_STS 0x008
+#define PCIE_IRQ5_STS 0x00C
+#define PCIE_IRQ0_EN 0x010
+#define PCIE_IRQ1_EN 0x014
+#define PCIE_IRQ2_EN 0x018
+#define PCIE_IRQ5_EN 0x01C
+#define IRQ_MSI_ENABLE BIT(20)
+#define PCIE_APP_LTSSM_ENABLE 0x054
+#define PCIE_ELBI_LTSSM_ENABLE 0x1
+#define PCIE_ELBI_CXPL_DEBUG_00_31 0x2C8
+#define PCIE_ELBI_CXPL_DEBUG_32_63 0x2CC
+#define PCIE_ELBI_SMLH_LINK_UP BIT(4)
+#define PCIE_ARTPEC8_DEVICE_TYPE 0x080
+#define DEVICE_TYPE_EP 0x0
+#define DEVICE_TYPE_LEG_EP 0x1
+#define DEVICE_TYPE_RC 0x4
+#define PCIE_ELBI_SLV_AWMISC 0x828
+#define PCIE_ELBI_SLV_ARMISC 0x820
+#define PCIE_ELBI_SLV_DBI_ENABLE BIT(21)
+#define LTSSM_STATE_MASK 0x3f
+#define LTSSM_STATE_L0 0x11
+
+/* FSYS SYSREG Offsets */
+#define FSYS_PCIE_CON 0x424
+#define PCIE_PERSTN BIT(5)
+#define FSYS_PCIE_DBI_ADDR_CON 0x428
+#define FSYS_PCIE_DBI_ADDR_OVR_CDM 0x00
+#define FSYS_PCIE_DBI_ADDR_OVR_SHADOW 0x12
+#define FSYS_PCIE_DBI_ADDR_OVR_ATU 0x36
+
+/* PMU SYSCON Offsets */
+#define PMU_SYSCON_PCIE_ISOLATION 0x3200
+
+/* BUS P/S SYSCON Offsets */
+#define BUS_SYSCON_BUS_PATH_ENABLE 0x0
+
+int artpec8_pcie_dbi_addr_con[] = {
+ FSYS_PCIE_DBI_ADDR_CON
+};
+
+struct artpec8_pcie {
+ struct dw_pcie *pci;
+ struct clk *pipe_clk;
+ struct clk *dbi_clk;
+ struct clk *mstr_clk;
+ struct clk *slv_clk;
+ const struct artpec8_pcie_pdata *pdata;
+ void __iomem *elbi_base;
+ struct regmap *sysreg;
+ struct regmap *pmu_syscon;
+ struct regmap *bus_s_syscon;
+ struct regmap *bus_p_syscon;
+ enum dw_pcie_device_mode mode;
+ int link_id;
+ /* For Generic PHY Framework */
+ struct phy *phy;
+};
+
+struct artpec8_pcie_res_ops {
+ int (*get_mem_resources)(struct platform_device *pdev,
+ struct artpec8_pcie *artpec8_ctrl);
+ int (*get_clk_resources)(struct platform_device *pdev,
+ struct artpec8_pcie *artpec8_ctrl);
+ int (*init_clk_resources)(struct artpec8_pcie *artpec8_ctrl);
+ void (*deinit_clk_resources)(struct artpec8_pcie *artpec8_ctrl);
+};
+
+struct artpec8_pcie_pdata {
+ const struct dw_pcie_ops *dwc_ops;
+ const struct dw_pcie_host_ops *host_ops;
+ const struct artpec8_pcie_res_ops *res_ops;
+ enum dw_pcie_device_mode mode;
+};
+
+enum artpec8_pcie_isolation {
+ PCIE_CLEAR_ISOLATION = 0,
+ PCIE_SET_ISOLATION = 1
+};
+
+enum artpec8_pcie_reg_bit {
+ PCIE_REG_BIT_LOW = 0,
+ PCIE_REG_BIT_HIGH = 1
+};
+
+static void artpec8_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
+ u32 reg, size_t size, u32 val);
+static u32 artpec8_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
+ u32 reg, size_t size);
+static void artpec8_pcie_writel(void __iomem *base, u32 val, u32 reg);
+
+static int artpec8_pcie_get_subsystem_resources(struct platform_device *pdev,
+ struct artpec8_pcie *artpec8_ctrl)
+{
+ struct device *dev = &pdev->dev;
+
+ /* External Local Bus interface(ELBI) Register */
+ artpec8_ctrl->elbi_base = devm_platform_ioremap_resource_byname(pdev, "elbi");
+ if (IS_ERR(artpec8_ctrl->elbi_base)) {
+ dev_err(dev, "failed to map elbi_base\n");
+ return PTR_ERR(artpec8_ctrl->elbi_base);
+ }
+
+ /* fsys sysreg regmap handle */
+ artpec8_ctrl->sysreg =
+ syscon_regmap_lookup_by_phandle(dev->of_node,
+ "samsung,fsys-sysreg");
+ if (IS_ERR(artpec8_ctrl->sysreg)) {
+ dev_err(dev, "fsys sysreg regmap lookup failed.\n");
+ return PTR_ERR(artpec8_ctrl->sysreg);
+ }
+
+ /* pmu syscon regmap handle */
+ artpec8_ctrl->pmu_syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
+ "samsung,syscon-phandle");
+ if (IS_ERR(artpec8_ctrl->pmu_syscon)) {
+ dev_err(dev, "pmu syscon regmap lookup failed.\n");
+ return PTR_ERR(artpec8_ctrl->pmu_syscon);
+ }
+
+ /* bus s syscon regmap handle */
+ artpec8_ctrl->bus_s_syscon =
+ syscon_regmap_lookup_by_phandle(dev->of_node,
+ "samsung,syscon-bus-s-fsys");
+ if (IS_ERR(artpec8_ctrl->bus_s_syscon)) {
+ dev_err(dev, "bus_s_syscon regmap lookup failed.\n");
+ return PTR_ERR(artpec8_ctrl->bus_s_syscon);
+ }
+
+ /* bus p syscon regmap handle */
+ artpec8_ctrl->bus_p_syscon =
+ syscon_regmap_lookup_by_phandle(dev->of_node,
+ "samsung,syscon-bus-p-fsys");
+ if (IS_ERR(artpec8_ctrl->bus_p_syscon)) {
+ dev_err(dev, "bus_p_syscon regmap lookup failed.\n");
+ return PTR_ERR(artpec8_ctrl->bus_p_syscon);
+ }
+
+ return 0;
+}
+
+static int artpec8_pcie_get_rc_mem_resources(struct platform_device *pdev,
+ struct artpec8_pcie *artpec8_ctrl)
+{
+ struct dw_pcie *pci = artpec8_ctrl->pci;
+
+ /* Data Bus Interface(DBI) Register */
+ pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
+ if (IS_ERR(pci->dbi_base))
+ return PTR_ERR(pci->dbi_base);
+
+ return 0;
+}
+
+static int artpec8_pcie_get_ep_mem_resources(struct platform_device *pdev,
+ struct artpec8_pcie *artpec8_ctrl)
+{
+ struct dw_pcie_ep *ep;
+ struct dw_pcie *pci = artpec8_ctrl->pci;
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+
+ ep = &pci->ep;
+
+ pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
+ if (IS_ERR(pci->dbi_base)) {
+ dev_err(dev, "failed to map ep_dbics\n");
+ return -ENOMEM;
+ }
+
+ pci->dbi_base2 = devm_platform_ioremap_resource_byname(pdev, "dbi2");
+ if (IS_ERR(pci->dbi_base2)) {
+ dev_err(dev, "failed to map ep_dbics2\n");
+ return -ENOMEM;
+ }
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
+ if (!res)
+ return -EINVAL;
+ ep->phys_base = res->start;
+ ep->addr_size = resource_size(res);
+
+ return 0;
+}
+
+static int artpec8_pcie_get_clk_resources(struct platform_device *pdev,
+ struct artpec8_pcie *artpec8_ctrl)
+{
+ struct device *dev = &pdev->dev;
+
+ artpec8_ctrl->pipe_clk = devm_clk_get(dev, "pipe_clk");
+ if (IS_ERR(artpec8_ctrl->pipe_clk)) {
+ dev_err(dev, "couldn't get pipe clock\n");
+ return -EINVAL;
+ }
+
+ artpec8_ctrl->dbi_clk = devm_clk_get(dev, "dbi_clk");
+ if (IS_ERR(artpec8_ctrl->dbi_clk)) {
+ dev_info(dev, "couldn't get dbi clk\n");
+ return -EINVAL;
+ }
+
+ artpec8_ctrl->slv_clk = devm_clk_get(dev, "slv_clk");
+ if (IS_ERR(artpec8_ctrl->slv_clk)) {
+ dev_err(dev, "couldn't get slave clock\n");
+ return -EINVAL;
+ }
+
+ artpec8_ctrl->mstr_clk = devm_clk_get(dev, "mstr_clk");
+ if (IS_ERR(artpec8_ctrl->mstr_clk)) {
+ dev_info(dev, "couldn't get master clk\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int artpec8_pcie_init_clk_resources(struct artpec8_pcie *artpec8_ctrl)
+{
+ clk_prepare_enable(artpec8_ctrl->pipe_clk);
+ clk_prepare_enable(artpec8_ctrl->dbi_clk);
+ clk_prepare_enable(artpec8_ctrl->mstr_clk);
+ clk_prepare_enable(artpec8_ctrl->slv_clk);
+
+ return 0;
+}
+
+static void artpec8_pcie_deinit_clk_resources(struct artpec8_pcie *artpec8_ctrl)
+{
+ clk_disable_unprepare(artpec8_ctrl->slv_clk);
+ clk_disable_unprepare(artpec8_ctrl->mstr_clk);
+ clk_disable_unprepare(artpec8_ctrl->dbi_clk);
+ clk_disable_unprepare(artpec8_ctrl->pipe_clk);
+}
+
+static const struct artpec8_pcie_res_ops artpec8_pcie_rc_res_ops = {
+ .get_mem_resources = artpec8_pcie_get_rc_mem_resources,
+ .get_clk_resources = artpec8_pcie_get_clk_resources,
+ .init_clk_resources = artpec8_pcie_init_clk_resources,
+ .deinit_clk_resources = artpec8_pcie_deinit_clk_resources,
+};
+
+static const struct artpec8_pcie_res_ops artpec8_pcie_ep_res_ops = {
+ .get_mem_resources = artpec8_pcie_get_ep_mem_resources,
+ .get_clk_resources = artpec8_pcie_get_clk_resources,
+ .init_clk_resources = artpec8_pcie_init_clk_resources,
+ .deinit_clk_resources = artpec8_pcie_deinit_clk_resources,
+};
+
+static void artpec8_pcie_writel(void __iomem *base, u32 val, u32 reg)
+{
+ writel(val, base + reg);
+}
+
+static u32 artpec8_pcie_readl(void __iomem *base, u32 reg)
+{
+ return readl(base + reg);
+}
+
+static int artpec8_pcie_config_phy_power_isolation(struct dw_pcie *pci,
+ enum artpec8_pcie_reg_bit val)
+{
+ struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
+ int ret;
+
+ ret = regmap_write(artpec8_ctrl->pmu_syscon, PMU_SYSCON_PCIE_ISOLATION,
+ val);
+
+ return ret;
+}
+
+static int artpec8_pcie_config_bus_enable(struct dw_pcie *pci,
+ enum artpec8_pcie_reg_bit val)
+{
+ struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
+ int ret;
+
+ ret = regmap_write(artpec8_ctrl->bus_p_syscon,
+ BUS_SYSCON_BUS_PATH_ENABLE, val);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(artpec8_ctrl->bus_s_syscon,
+ BUS_SYSCON_BUS_PATH_ENABLE, val);
+ if (ret)
+ return ret;
+
+ return ret;
+}
+
+static int artpec8_pcie_config_isolation(struct dw_pcie *pci,
+ enum artpec8_pcie_isolation val)
+{
+ int ret;
+ /* reg_val[0] : for phy power isolation */
+ /* reg_val[1] : for bus enable */
+ enum artpec8_pcie_reg_bit reg_val[2];
+
+ switch (val) {
+ case PCIE_CLEAR_ISOLATION:
+ reg_val[0] = PCIE_REG_BIT_LOW;
+ reg_val[1] = PCIE_REG_BIT_HIGH;
+ break;
+ case PCIE_SET_ISOLATION:
+ reg_val[0] = PCIE_REG_BIT_HIGH;
+ reg_val[1] = PCIE_REG_BIT_LOW;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ ret = artpec8_pcie_config_phy_power_isolation(pci, reg_val[0]);
+ if (ret)
+ return ret;
+
+ ret = artpec8_pcie_config_bus_enable(pci, reg_val[1]);
+ if (ret)
+ return ret;
+
+ return ret;
+}
+
+static int artpec8_pcie_config_perstn(struct dw_pcie *pci,
+ enum artpec8_pcie_reg_bit val)
+{
+ struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
+ unsigned int bits;
+ int ret;
+
+ if (val == PCIE_REG_BIT_HIGH)
+ bits = PCIE_PERSTN;
+ else
+ bits = 0;
+
+ ret = regmap_update_bits(artpec8_ctrl->sysreg, FSYS_PCIE_CON,
+ PCIE_PERSTN, bits);
+
+ return ret;
+}
+
+static void artpec8_pcie_stop_link(struct dw_pcie *pci)
+{
+ struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
+ u32 val;
+
+ val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
+ PCIE_APP_LTSSM_ENABLE);
+
+ val &= ~PCIE_ELBI_LTSSM_ENABLE;
+ artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
+ PCIE_APP_LTSSM_ENABLE);
+}
+
+static int artpec8_pcie_start_link(struct dw_pcie *pci)
+{
+ struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
+ u32 val;
+
+ dw_pcie_dbi_ro_wr_en(pci);
+
+ /* Equalization disable */
+ val = artpec8_pcie_read_dbi(pci, pci->dbi_base, PCIE_GEN3_RELATED_OFF,
+ 4);
+ artpec8_pcie_write_dbi(pci, pci->dbi_base, PCIE_GEN3_RELATED_OFF, 4,
+ val | PCIE_GEN3_EQUALIZATION_DISABLE);
+
+ dw_pcie_dbi_ro_wr_dis(pci);
+
+ /* assert LTSSM enable */
+ val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
+ PCIE_APP_LTSSM_ENABLE);
+
+ val |= PCIE_ELBI_LTSSM_ENABLE;
+ artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
+ PCIE_APP_LTSSM_ENABLE);
+
+ return 0;
+}
+
+static irqreturn_t artpec8_pcie_msi_irq_handler(int irq, void *arg)
+{
+ struct artpec8_pcie *artpec8_ctrl = arg;
+ struct dw_pcie *pci = artpec8_ctrl->pci;
+ struct pcie_port *pp = &pci->pp;
+ u32 val;
+
+ val = artpec8_pcie_readl(artpec8_ctrl->elbi_base, PCIE_IRQ2_STS);
+
+ if ((val & IRQ_MSI_ENABLE) == IRQ_MSI_ENABLE) {
+ val &= IRQ_MSI_ENABLE;
+ artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
+ PCIE_IRQ2_STS);
+ dw_handle_msi_irq(pp);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static void artpec8_pcie_msi_init(struct artpec8_pcie *artpec8_ctrl)
+{
+ u32 val;
+
+ /* enable MSI interrupt */
+ val = artpec8_pcie_readl(artpec8_ctrl->elbi_base, PCIE_IRQ2_EN);
+ val |= IRQ_MSI_ENABLE;
+ artpec8_pcie_writel(artpec8_ctrl->elbi_base, val, PCIE_IRQ2_EN);
+}
+
+static void artpec8_pcie_enable_interrupts(struct artpec8_pcie *artpec8_ctrl)
+{
+ if (IS_ENABLED(CONFIG_PCI_MSI))
+ artpec8_pcie_msi_init(artpec8_ctrl);
+}
+
+static u32 artpec8_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
+ u32 reg, size_t size)
+{
+ struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
+ u32 val;
+ bool is_atu = false;
+
+ if (base == pci->atu_base) {
+ is_atu = true;
+ base = pci->dbi_base;
+ regmap_write(artpec8_ctrl->sysreg,
+ artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
+ FSYS_PCIE_DBI_ADDR_OVR_ATU);
+ }
+
+ dw_pcie_read(base + reg, size, &val);
+
+ if (is_atu)
+ regmap_write(artpec8_ctrl->sysreg,
+ artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
+ FSYS_PCIE_DBI_ADDR_OVR_CDM);
+
+ return val;
+}
+
+static void artpec8_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
+ u32 reg, size_t size, u32 val)
+{
+ struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
+ bool is_atu = false;
+
+ if (base == pci->atu_base) {
+ is_atu = true;
+ base = pci->dbi_base;
+ regmap_write(artpec8_ctrl->sysreg,
+ artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
+ FSYS_PCIE_DBI_ADDR_OVR_ATU);
+ }
+
+ dw_pcie_write(base + reg, size, val);
+
+ if (is_atu)
+ regmap_write(artpec8_ctrl->sysreg,
+ artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
+ FSYS_PCIE_DBI_ADDR_OVR_CDM);
+}
+
+static void artpec8_pcie_write_dbi2(struct dw_pcie *pci, void __iomem *base,
+ u32 reg, size_t size, u32 val)
+{
+ struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
+
+ regmap_write(artpec8_ctrl->sysreg,
+ artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
+ FSYS_PCIE_DBI_ADDR_OVR_SHADOW);
+
+ dw_pcie_write(base + reg, size, val);
+
+ regmap_write(artpec8_ctrl->sysreg,
+ artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
+ FSYS_PCIE_DBI_ADDR_OVR_CDM);
+}
+
+static int artpec8_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)) {
+ *val = ~0;
+ return PCIBIOS_DEVICE_NOT_FOUND;
+ }
+
+ *val = dw_pcie_read_dbi(pci, where, size);
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static int artpec8_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 artpec8_pci_ops = {
+ .read = artpec8_pcie_rd_own_conf,
+ .write = artpec8_pcie_wr_own_conf,
+};
+
+static int artpec8_pcie_link_up(struct dw_pcie *pci)
+{
+ struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
+ u32 val;
+
+ val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
+ PCIE_ELBI_CXPL_DEBUG_00_31);
+
+ return (val & LTSSM_STATE_MASK) == LTSSM_STATE_L0;
+}
+
+static int artpec8_pcie_host_init(struct pcie_port *pp)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
+
+ pp->bridge->ops = &artpec8_pci_ops;
+
+ dw_pcie_writel_dbi(pci, PCIE_GEN3_RELATED_OFF,
+ (PCIE_GEN3_EQ_PHASE_2_3 |
+ PCIE_GEN3_RXEQ_PH01_EN |
+ PCIE_GEN3_RXEQ_RGRDLESS_RXTS));
+
+ artpec8_pcie_enable_interrupts(artpec8_ctrl);
+
+ return 0;
+}
+
+static const struct dw_pcie_host_ops artpec8_pcie_host_ops = {
+ .host_init = artpec8_pcie_host_init,
+};
+
+static u8 artpec8_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
+{
+ u32 val;
+
+ val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
+ pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
+
+ if (val == 0xffffffff)
+ return 1;
+
+ return 0;
+}
+
+static void artpec8_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+ enum pci_barno bar;
+ /* Currently PCIe EP core is not setting iatu_unroll_enabled
+ * so let's handle it here. We need to find proper place to
+ * initialize this so that it can be used as for other EP
+ * controllers as well.
+ */
+ pci->iatu_unroll_enabled = artpec8_pcie_iatu_unroll_enabled(pci);
+
+ for (bar = BAR_0; bar <= BAR_5; bar++)
+ dw_pcie_ep_reset_bar(pci, bar);
+}
+
+static int artpec8_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
+ enum pci_epc_irq_type type, u16 interrupt_num)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+
+ switch (type) {
+ case PCI_EPC_IRQ_LEGACY:
+ return -EINVAL;
+ case PCI_EPC_IRQ_MSI:
+ return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
+ default:
+ dev_err(pci->dev, "UNKNOWN IRQ type\n");
+ }
+
+ return 0;
+}
+
+static const struct pci_epc_features artpec8_pcie_epc_features = {
+ .linkup_notifier = false,
+ .msi_capable = true,
+ .msix_capable = false,
+};
+
+static const struct pci_epc_features*
+artpec8_pcie_ep_get_features(struct dw_pcie_ep *ep)
+{
+ return &artpec8_pcie_epc_features;
+}
+
+static const struct dw_pcie_ep_ops artpec8_dw_pcie_ep_ops = {
+ .ep_init = artpec8_pcie_ep_init,
+ .raise_irq = artpec8_pcie_raise_irq,
+ .get_features = artpec8_pcie_ep_get_features,
+};
+
+static int __init artpec8_add_pcie_ep(struct artpec8_pcie *artpec8_ctrl,
+ struct platform_device *pdev)
+{
+ int ret;
+ struct dw_pcie_ep *ep;
+ struct dw_pcie *pci = artpec8_ctrl->pci;
+
+ ep = &pci->ep;
+ ep->ops = &artpec8_dw_pcie_ep_ops;
+
+ dw_pcie_writel_dbi(pci, PCIE_GEN3_RELATED_OFF,
+ (PCIE_GEN3_EQ_PHASE_2_3 |
+ PCIE_GEN3_RXEQ_PH01_EN |
+ PCIE_GEN3_RXEQ_RGRDLESS_RXTS));
+
+ ret = dw_pcie_ep_init(ep);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int __init artpec8_add_pcie_port(struct artpec8_pcie *artpec8_ctrl,
+ struct platform_device *pdev)
+{
+ struct dw_pcie *pci = artpec8_ctrl->pci;
+ struct pcie_port *pp = &pci->pp;
+ struct device *dev = &pdev->dev;
+ int ret;
+ int irq_flags;
+ int irq;
+
+ if (IS_ENABLED(CONFIG_PCI_MSI)) {
+ irq = platform_get_irq_byname(pdev, "intr");
+ if (!irq)
+ return -ENODEV;
+
+ irq_flags = IRQF_SHARED | IRQF_NO_THREAD;
+
+ ret = devm_request_irq(dev, irq, artpec8_pcie_msi_irq_handler,
+ irq_flags, "artpec8-pcie", artpec8_ctrl);
+ if (ret)
+ return ret;
+ }
+
+ /* Prevent core for messing with the IRQ, since it's muxed */
+ pp->msi_irq = -ENODEV;
+
+ ret = dw_pcie_host_init(pp);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static const struct dw_pcie_ops artpec8_dw_pcie_ops = {
+ .read_dbi = artpec8_pcie_read_dbi,
+ .write_dbi = artpec8_pcie_write_dbi,
+ .write_dbi2 = artpec8_pcie_write_dbi2,
+ .start_link = artpec8_pcie_start_link,
+ .stop_link = artpec8_pcie_stop_link,
+ .link_up = artpec8_pcie_link_up,
+};
+
+static int artpec8_pcie_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct dw_pcie *pci;
+ struct pcie_port *pp;
+ struct artpec8_pcie *artpec8_ctrl;
+ enum dw_pcie_device_mode mode;
+ struct device *dev = &pdev->dev;
+ const struct artpec8_pcie_pdata *pdata;
+ struct device_node *np = dev->of_node;
+
+ artpec8_ctrl = devm_kzalloc(dev, sizeof(*artpec8_ctrl), GFP_KERNEL);
+ if (!artpec8_ctrl)
+ return -ENOMEM;
+
+ pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+ if (!pci)
+ return -ENOMEM;
+
+ pdata = (const struct artpec8_pcie_pdata *)
+ of_device_get_match_data(dev);
+ if (!pdata)
+ return -ENODEV;
+
+ mode = (enum dw_pcie_device_mode)pdata->mode;
+
+ artpec8_ctrl->pci = pci;
+ artpec8_ctrl->pdata = pdata;
+ artpec8_ctrl->mode = mode;
+
+ pci->dev = dev;
+ pci->ops = pdata->dwc_ops;
+ pci->dbi_base2 = NULL;
+ pci->dbi_base = NULL;
+ pp = &pci->pp;
+ pp->ops = artpec8_ctrl->pdata->host_ops;
+
+ if (mode == DW_PCIE_RC_TYPE)
+ artpec8_ctrl->link_id = of_alias_get_id(np, "pcierc");
+ else
+ artpec8_ctrl->link_id = of_alias_get_id(np, "pcieep");
+
+ ret = artpec8_pcie_get_subsystem_resources(pdev, artpec8_ctrl);
+ if (ret)
+ return ret;
+
+ if (pdata->res_ops && pdata->res_ops->get_mem_resources) {
+ ret = pdata->res_ops->get_mem_resources(pdev, artpec8_ctrl);
+ if (ret)
+ return ret;
+ }
+
+ if (pdata->res_ops && pdata->res_ops->get_clk_resources) {
+ ret = pdata->res_ops->get_clk_resources(pdev, artpec8_ctrl);
+ if (ret)
+ return ret;
+
+ ret = pdata->res_ops->init_clk_resources(artpec8_ctrl);
+ if (ret)
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, artpec8_ctrl);
+
+ ret = artpec8_pcie_config_isolation(pci, PCIE_CLEAR_ISOLATION);
+ if (ret)
+ return ret;
+
+ ret = artpec8_pcie_config_perstn(pci, PCIE_REG_BIT_HIGH);
+ if (ret)
+ return ret;
+
+ artpec8_ctrl->phy = devm_of_phy_get(dev, np, NULL);
+ if (IS_ERR(artpec8_ctrl->phy))
+ return PTR_ERR(artpec8_ctrl->phy);
+
+ phy_init(artpec8_ctrl->phy);
+ phy_reset(artpec8_ctrl->phy);
+
+ switch (mode) {
+ case DW_PCIE_RC_TYPE:
+ artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_RC,
+ PCIE_ARTPEC8_DEVICE_TYPE);
+ ret = artpec8_add_pcie_port(artpec8_ctrl, pdev);
+ if (ret < 0)
+ goto fail_probe;
+ break;
+ case DW_PCIE_EP_TYPE:
+ artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_EP,
+ PCIE_ARTPEC8_DEVICE_TYPE);
+
+ ret = artpec8_add_pcie_ep(artpec8_ctrl, pdev);
+ if (ret < 0)
+ goto fail_probe;
+ break;
+ default:
+ ret = -EINVAL;
+ goto fail_probe;
+ }
+
+ return 0;
+
+fail_probe:
+ phy_exit(artpec8_ctrl->phy);
+ if (pdata->res_ops && pdata->res_ops->deinit_clk_resources)
+ pdata->res_ops->deinit_clk_resources(artpec8_ctrl);
+
+ return ret;
+}
+
+static int __exit artpec8_pcie_remove(struct platform_device *pdev)
+{
+ struct artpec8_pcie *artpec8_ctrl = platform_get_drvdata(pdev);
+ const struct artpec8_pcie_pdata *pdata = artpec8_ctrl->pdata;
+
+ if (pdata->res_ops && pdata->res_ops->deinit_clk_resources)
+ pdata->res_ops->deinit_clk_resources(artpec8_ctrl);
+
+ return 0;
+}
+
+static const struct artpec8_pcie_pdata artpec8_pcie_rc_pdata = {
+ .dwc_ops = &artpec8_dw_pcie_ops,
+ .host_ops = &artpec8_pcie_host_ops,
+ .res_ops = &artpec8_pcie_rc_res_ops,
+ .mode = DW_PCIE_RC_TYPE,
+};
+
+static const struct artpec8_pcie_pdata artpec8_pcie_ep_pdata = {
+ .dwc_ops = &artpec8_dw_pcie_ops,
+ .host_ops = &artpec8_pcie_host_ops,
+ .res_ops = &artpec8_pcie_ep_res_ops,
+ .mode = DW_PCIE_EP_TYPE,
+};
+
+static const struct of_device_id artpec8_pcie_of_match[] = {
+ {
+ .compatible = "axis,artpec8-pcie",
+ .data = &artpec8_pcie_rc_pdata,
+ },
+ {
+ .compatible = "axis,artpec8-pcie-ep",
+ .data = &artpec8_pcie_ep_pdata,
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, artpec8_pcie_of_match);
+
+static struct platform_driver artpec8_pcie_driver = {
+ .probe = artpec8_pcie_probe,
+ .remove = __exit_p(artpec8_pcie_remove),
+ .driver = {
+ .name = "artpec8-pcie",
+ .of_match_table = artpec8_pcie_of_match,
+ },
+};
+
+module_platform_driver(artpec8_pcie_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jaeho Cho <[email protected]>");
--
2.9.5
In the subject, why do you tag this "axis"? There's an existing
pcie-artpec6.c that uses the driver name ""artpec6-pcie" and the
subject line tag "artpec6".
This adds pcie-artpec8.c with driver name "artpec8-pcie", so the
obvious choice would be "artpec8".
I assume you evaluated the possibility of extending artpec6 to support
artpec8 in addition to the artpec6 and artpec7 it already supports?
On Fri, Jun 03, 2022 at 11:34:52AM +0900, Wangseok Lee wrote:
> Add support Axis, ARTPEC-8 SoC.
> ARTPEC-8 is the SoC platform of Axis Communications.
> This is based on arm64 and support GEN4 & 2lane.
> This PCIe controller is based on DesignWare Hardware core
> and uses DesignWare core functions to implement the driver.
Add blank lines between paragraphs.
Wrap lines to fill 75 columns.
> changes since v1 :
> improvement review comment of Krzysztof on driver code.
> -debug messages for probe or other functions.
> -Inconsistent coding style (different indentation in structure members).
> -Inconsistent code (artpec8_pcie_get_subsystem_resources() gets device
> from pdev and from pci so you have two same pointers;
> or artpec8_pcie_get_ep_mem_resources() stores dev
> as local variable but uses instead pdev->dev).
> -Not using devm_platform_ioremap_resource().
> -Printing messages in interrupt handlers.
> -Several local/static structures or array are not const.
Thanks for the "changes since v1" notes. You can put them below the
"---" since there's no need for them in the permanent git commit log:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v5.18#n675
> Signed-off-by: Wangseok Lee <[email protected]>
Why is there no signoff from Jaeho Cho <[email protected]>?
> ---
> drivers/pci/controller/dwc/Kconfig | 31 ++
> drivers/pci/controller/dwc/Makefile | 1 +
> drivers/pci/controller/dwc/pcie-artpec8.c | 864 ++++++++++++++++++++++++++++++
> 3 files changed, 896 insertions(+)
> create mode 100644 drivers/pci/controller/dwc/pcie-artpec8.c
>
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 62ce3ab..4aa6da8 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -222,6 +222,37 @@ config PCIE_ARTPEC6_EP
> Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
> endpoint mode. This uses the DesignWare core.
>
> +config PCIE_ARTPEC8
> + bool "Axis ARTPEC-8 PCIe controller"
> +
> +config PCIE_ARTPEC8_HOST
> + bool "Axis ARTPEC-8 PCIe controller Host Mode"
> + depends on ARCH_ARTPEC
> + depends on PCI_MSI_IRQ_DOMAIN
> + depends on PCI_ENDPOINT
> + select PCI_EPF_TEST
> + select PCIE_DW_HOST
> + select PCIE_ARTPEC8
> + help
> + Say 'Y' here to enable support for the PCIe controller in the
> + ARTPEC-8 SoC to work in host mode.
> + This PCIe controller is based on DesignWare Hardware core.
> + And uses DesignWare core functions to implement the driver.
Add blank line between paragraphs or rewrap as a single paragraph.
s/Hardware/hardware/
The last two sentences should be combined since the latter has no
verb:
This PCIe controller is based on the DesignWare hardware core and
uses DesignWare core functions to implement the driver.
> +config PCIE_ARTPEC8_EP
> + bool "Axis ARTPEC-8 PCIe controller Endpoint Mode"
> + depends on ARCH_ARTPEC
> + depends on PCI_ENDPOINT
> + depends on PCI_ENDPOINT_CONFIGFS
> + select PCI_EPF_TEST
> + select PCIE_DW_EP
> + select PCIE_ARTPEC8
> + help
> + Say 'Y' here to enable support for the PCIe controller in the
> + ARTPEC-8 SoC to work in endpoint mode.
> + This PCIe controller is based on DesignWare Hardware core.
> + And uses DesignWare core functions to implement the driver.
Same.
> config PCIE_ROCKCHIP_DW_HOST
> bool "Rockchip DesignWare PCIe controller"
> select PCIE_DW
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index 8ba7b67..b361022 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
> obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
> obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
> obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
> +obj-$(CONFIG_PCIE_ARTPEC8) += pcie-artpec8.o
>
> # The following drivers are for devices that use the generic ACPI
> # pci_root.c driver but don't support standard ECAM config access.
> diff --git a/drivers/pci/controller/dwc/pcie-artpec8.c b/drivers/pci/controller/dwc/pcie-artpec8.c
> new file mode 100644
> index 0000000..d9ae9bf
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-artpec8.c
> @@ -0,0 +1,864 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * PCIe controller driver for Axis ARTPEC-8 SoC
> + *
> + * Copyright (C) 2019 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * Author: Jaeho Cho <[email protected]>
> + * This file is based on driver/pci/controller/dwc/pci-exynos.c
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/resource.h>
> +#include <linux/types.h>
> +#include <linux/phy/phy.h>
> +
> +#include "pcie-designware.h"
> +
> +#define to_artpec8_pcie(x) dev_get_drvdata((x)->dev)
> +
> +/* Gen3 Control Register */
> +#define PCIE_GEN3_RELATED_OFF 0x890
> +/* Disables equilzation feature */
s/equilzation/equalization/
Comment is probably unnecessary, since the name is so descriptive.
> +#define PCIE_GEN3_EQUALIZATION_DISABLE (0x1 << 16)
> +#define PCIE_GEN3_EQ_PHASE_2_3 (0x1 << 9)
> +#define PCIE_GEN3_RXEQ_PH01_EN (0x1 << 12)
> +#define PCIE_GEN3_RXEQ_RGRDLESS_RXTS (0x1 << 13)
> +
> +#define FAST_LINK_MODE (7)
> +
> +/* PCIe ELBI registers */
> +#define PCIE_IRQ0_STS 0x000
> +#define PCIE_IRQ1_STS 0x004
> +#define PCIE_IRQ2_STS 0x008
> +#define PCIE_IRQ5_STS 0x00C
> +#define PCIE_IRQ0_EN 0x010
> +#define PCIE_IRQ1_EN 0x014
> +#define PCIE_IRQ2_EN 0x018
> +#define PCIE_IRQ5_EN 0x01C
> +#define IRQ_MSI_ENABLE BIT(20)
> +#define PCIE_APP_LTSSM_ENABLE 0x054
> +#define PCIE_ELBI_LTSSM_ENABLE 0x1
> +#define PCIE_ELBI_CXPL_DEBUG_00_31 0x2C8
> +#define PCIE_ELBI_CXPL_DEBUG_32_63 0x2CC
> +#define PCIE_ELBI_SMLH_LINK_UP BIT(4)
> +#define PCIE_ARTPEC8_DEVICE_TYPE 0x080
> +#define DEVICE_TYPE_EP 0x0
> +#define DEVICE_TYPE_LEG_EP 0x1
> +#define DEVICE_TYPE_RC 0x4
> +#define PCIE_ELBI_SLV_AWMISC 0x828
> +#define PCIE_ELBI_SLV_ARMISC 0x820
> +#define PCIE_ELBI_SLV_DBI_ENABLE BIT(21)
> +#define LTSSM_STATE_MASK 0x3f
The previous hex constants used upper-case; this uses lower-case.
Pick one and stick with it.
This seems like a mix of register offsets and definitions of bits
within registers. It's confusing to mentally sort these out. Is
there any way to make this more obvious? Some drivers, e.g.,
pcie-artpec6.c, use "BIT(x)" and "GENMASK(x,y)" for bits within
registers.
> +#define LTSSM_STATE_L0 0x11
> +
> +/* FSYS SYSREG Offsets */
The list below seems to inclue more than just register offsets.
> +#define FSYS_PCIE_CON 0x424
> +#define PCIE_PERSTN BIT(5)
> +#define FSYS_PCIE_DBI_ADDR_CON 0x428
> +#define FSYS_PCIE_DBI_ADDR_OVR_CDM 0x00
> +#define FSYS_PCIE_DBI_ADDR_OVR_SHADOW 0x12
> +#define FSYS_PCIE_DBI_ADDR_OVR_ATU 0x36
> +
> +/* PMU SYSCON Offsets */
> +#define PMU_SYSCON_PCIE_ISOLATION 0x3200
> +
> +/* BUS P/S SYSCON Offsets */
> +#define BUS_SYSCON_BUS_PATH_ENABLE 0x0
> +
> +int artpec8_pcie_dbi_addr_con[] = {
> + FSYS_PCIE_DBI_ADDR_CON
> +};
> +
> +struct artpec8_pcie {
> + struct dw_pcie *pci;
> + struct clk *pipe_clk;
> + struct clk *dbi_clk;
> + struct clk *mstr_clk;
> + struct clk *slv_clk;
> + const struct artpec8_pcie_pdata *pdata;
> + void __iomem *elbi_base;
> + struct regmap *sysreg;
> + struct regmap *pmu_syscon;
> + struct regmap *bus_s_syscon;
> + struct regmap *bus_p_syscon;
> + enum dw_pcie_device_mode mode;
> + int link_id;
> + /* For Generic PHY Framework */
Superfluous comment.
> + struct phy *phy;
> +};
> +
> +struct artpec8_pcie_res_ops {
> + int (*get_mem_resources)(struct platform_device *pdev,
> + struct artpec8_pcie *artpec8_ctrl);
> + int (*get_clk_resources)(struct platform_device *pdev,
> + struct artpec8_pcie *artpec8_ctrl);
> + int (*init_clk_resources)(struct artpec8_pcie *artpec8_ctrl);
> + void (*deinit_clk_resources)(struct artpec8_pcie *artpec8_ctrl);
> +};
> +
> +struct artpec8_pcie_pdata {
> + const struct dw_pcie_ops *dwc_ops;
> + const struct dw_pcie_host_ops *host_ops;
Fix indentation to match surrounding code.
> + const struct artpec8_pcie_res_ops *res_ops;
> + enum dw_pcie_device_mode mode;
> +};
> +
> +enum artpec8_pcie_isolation {
> + PCIE_CLEAR_ISOLATION = 0,
> + PCIE_SET_ISOLATION = 1
> +};
> +
> +enum artpec8_pcie_reg_bit {
> + PCIE_REG_BIT_LOW = 0,
> + PCIE_REG_BIT_HIGH = 1
> +};
> +
> +static void artpec8_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
> + u32 reg, size_t size, u32 val);
> +static u32 artpec8_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
> + u32 reg, size_t size);
> +static void artpec8_pcie_writel(void __iomem *base, u32 val, u32 reg);
Can you reorder the function definitions to avoid the need for these
forward declarations?
> +static int artpec8_pcie_get_subsystem_resources(struct platform_device *pdev,
> + struct artpec8_pcie *artpec8_ctrl)
> +{
> + struct device *dev = &pdev->dev;
> +
> + /* External Local Bus interface(ELBI) Register */
> + artpec8_ctrl->elbi_base = devm_platform_ioremap_resource_byname(pdev, "elbi");
Rewrap to fit in 80 columns.
> + if (IS_ERR(artpec8_ctrl->elbi_base)) {
> + dev_err(dev, "failed to map elbi_base\n");
> + return PTR_ERR(artpec8_ctrl->elbi_base);
> + }
> +
> + /* fsys sysreg regmap handle */
All these comments are superfluous since they only repeat the lookup
arguments.
> + artpec8_ctrl->sysreg =
> + syscon_regmap_lookup_by_phandle(dev->of_node,
The above two lines should fit on one line.
> + "samsung,fsys-sysreg");
> + if (IS_ERR(artpec8_ctrl->sysreg)) {
> + dev_err(dev, "fsys sysreg regmap lookup failed.\n");
> + return PTR_ERR(artpec8_ctrl->sysreg);
> + }
> +
> + /* pmu syscon regmap handle */
> + artpec8_ctrl->pmu_syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
> + "samsung,syscon-phandle");
> + if (IS_ERR(artpec8_ctrl->pmu_syscon)) {
> + dev_err(dev, "pmu syscon regmap lookup failed.\n");
> + return PTR_ERR(artpec8_ctrl->pmu_syscon);
> + }
> +
> + /* bus s syscon regmap handle */
> + artpec8_ctrl->bus_s_syscon =
> + syscon_regmap_lookup_by_phandle(dev->of_node,
> + "samsung,syscon-bus-s-fsys");
> + if (IS_ERR(artpec8_ctrl->bus_s_syscon)) {
> + dev_err(dev, "bus_s_syscon regmap lookup failed.\n");
> + return PTR_ERR(artpec8_ctrl->bus_s_syscon);
> + }
> +
> + /* bus p syscon regmap handle */
> + artpec8_ctrl->bus_p_syscon =
> + syscon_regmap_lookup_by_phandle(dev->of_node,
> + "samsung,syscon-bus-p-fsys");
> + if (IS_ERR(artpec8_ctrl->bus_p_syscon)) {
> + dev_err(dev, "bus_p_syscon regmap lookup failed.\n");
> + return PTR_ERR(artpec8_ctrl->bus_p_syscon);
> + }
> +
> + return 0;
> +}
> +
> +static int artpec8_pcie_get_rc_mem_resources(struct platform_device *pdev,
> + struct artpec8_pcie *artpec8_ctrl)
> +{
> + struct dw_pcie *pci = artpec8_ctrl->pci;
> +
> + /* Data Bus Interface(DBI) Register */
> + pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
> + if (IS_ERR(pci->dbi_base))
> + return PTR_ERR(pci->dbi_base);
> +
> + return 0;
> +}
> +
> +static int artpec8_pcie_get_ep_mem_resources(struct platform_device *pdev,
> + struct artpec8_pcie *artpec8_ctrl)
> +{
> + struct dw_pcie_ep *ep;
> + struct dw_pcie *pci = artpec8_ctrl->pci;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> +
> + ep = &pci->ep;
Reorder the locals above:
struct dw_pcie *pci = artpec8_ctrl->pci;
struct device *dev = &pdev->dev;
struct dw_pcie_ep *ep = &pci->ep;
struct resource *res;
Then they're in the order you use them and you don't need the extra
"ep = &pci->ep".
> + pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
> + if (IS_ERR(pci->dbi_base)) {
> + dev_err(dev, "failed to map ep_dbics\n");
> + return -ENOMEM;
> + }
> +
> + pci->dbi_base2 = devm_platform_ioremap_resource_byname(pdev, "dbi2");
> + if (IS_ERR(pci->dbi_base2)) {
> + dev_err(dev, "failed to map ep_dbics2\n");
> + return -ENOMEM;
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> + if (!res)
> + return -EINVAL;
> + ep->phys_base = res->start;
> + ep->addr_size = resource_size(res);
> +
> + return 0;
> +}
> +
> +static int artpec8_pcie_get_clk_resources(struct platform_device *pdev,
> + struct artpec8_pcie *artpec8_ctrl)
> +{
> + struct device *dev = &pdev->dev;
> +
> + artpec8_ctrl->pipe_clk = devm_clk_get(dev, "pipe_clk");
> + if (IS_ERR(artpec8_ctrl->pipe_clk)) {
> + dev_err(dev, "couldn't get pipe clock\n");
> + return -EINVAL;
> + }
> +
> + artpec8_ctrl->dbi_clk = devm_clk_get(dev, "dbi_clk");
> + if (IS_ERR(artpec8_ctrl->dbi_clk)) {
> + dev_info(dev, "couldn't get dbi clk\n");
> + return -EINVAL;
> + }
> +
> + artpec8_ctrl->slv_clk = devm_clk_get(dev, "slv_clk");
> + if (IS_ERR(artpec8_ctrl->slv_clk)) {
> + dev_err(dev, "couldn't get slave clock\n");
> + return -EINVAL;
> + }
> +
> + artpec8_ctrl->mstr_clk = devm_clk_get(dev, "mstr_clk");
> + if (IS_ERR(artpec8_ctrl->mstr_clk)) {
> + dev_info(dev, "couldn't get master clk\n");
It'd be nice if the err/info messages matched the exact DT name:
"pipe_clk", "dbi_clk", slv_clk", etc.
Why are some of the above dev_err() and others dev_info() when you
return -EINVAL in all cases?
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int artpec8_pcie_init_clk_resources(struct artpec8_pcie *artpec8_ctrl)
> +{
> + clk_prepare_enable(artpec8_ctrl->pipe_clk);
> + clk_prepare_enable(artpec8_ctrl->dbi_clk);
> + clk_prepare_enable(artpec8_ctrl->mstr_clk);
> + clk_prepare_enable(artpec8_ctrl->slv_clk);
> +
> + return 0;
> +}
> +
> +static void artpec8_pcie_deinit_clk_resources(struct artpec8_pcie *artpec8_ctrl)
> +{
> + clk_disable_unprepare(artpec8_ctrl->slv_clk);
> + clk_disable_unprepare(artpec8_ctrl->mstr_clk);
> + clk_disable_unprepare(artpec8_ctrl->dbi_clk);
> + clk_disable_unprepare(artpec8_ctrl->pipe_clk);
> +}
> +
> +static const struct artpec8_pcie_res_ops artpec8_pcie_rc_res_ops = {
> + .get_mem_resources = artpec8_pcie_get_rc_mem_resources,
> + .get_clk_resources = artpec8_pcie_get_clk_resources,
> + .init_clk_resources = artpec8_pcie_init_clk_resources,
> + .deinit_clk_resources = artpec8_pcie_deinit_clk_resources,
> +};
> +
> +static const struct artpec8_pcie_res_ops artpec8_pcie_ep_res_ops = {
> + .get_mem_resources = artpec8_pcie_get_ep_mem_resources,
> + .get_clk_resources = artpec8_pcie_get_clk_resources,
> + .init_clk_resources = artpec8_pcie_init_clk_resources,
> + .deinit_clk_resources = artpec8_pcie_deinit_clk_resources,
> +};
> +
> +static void artpec8_pcie_writel(void __iomem *base, u32 val, u32 reg)
> +{
> + writel(val, base + reg);
> +}
> +
> +static u32 artpec8_pcie_readl(void __iomem *base, u32 reg)
> +{
> + return readl(base + reg);
> +}
> +
> +static int artpec8_pcie_config_phy_power_isolation(struct dw_pcie *pci,
> + enum artpec8_pcie_reg_bit val)
> +{
> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> + int ret;
> +
> + ret = regmap_write(artpec8_ctrl->pmu_syscon, PMU_SYSCON_PCIE_ISOLATION,
> + val);
> +
> + return ret;
return regmap_write(artpec8_ctrl->pmu_syscon, ...);
> +}
> +
> +static int artpec8_pcie_config_bus_enable(struct dw_pcie *pci,
> + enum artpec8_pcie_reg_bit val)
> +{
> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> + int ret;
> +
> + ret = regmap_write(artpec8_ctrl->bus_p_syscon,
> + BUS_SYSCON_BUS_PATH_ENABLE, val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(artpec8_ctrl->bus_s_syscon,
> + BUS_SYSCON_BUS_PATH_ENABLE, val);
> + if (ret)
> + return ret;
> +
> + return ret;
return regmap_write(artpec8_ctrl->bus_s_syscon, ...);
> +}
> +
> +static int artpec8_pcie_config_isolation(struct dw_pcie *pci,
> + enum artpec8_pcie_isolation val)
> +{
> + int ret;
> + /* reg_val[0] : for phy power isolation */
> + /* reg_val[1] : for bus enable */
> + enum artpec8_pcie_reg_bit reg_val[2];
> +
> + switch (val) {
> + case PCIE_CLEAR_ISOLATION:
> + reg_val[0] = PCIE_REG_BIT_LOW;
> + reg_val[1] = PCIE_REG_BIT_HIGH;
> + break;
> + case PCIE_SET_ISOLATION:
> + reg_val[0] = PCIE_REG_BIT_HIGH;
> + reg_val[1] = PCIE_REG_BIT_LOW;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = artpec8_pcie_config_phy_power_isolation(pci, reg_val[0]);
> + if (ret)
> + return ret;
> +
> + ret = artpec8_pcie_config_bus_enable(pci, reg_val[1]);
> + if (ret)
> + return ret;
> +
> + return ret;
return artpec8_pcie_config_bus_enable(pci, ...);
> +}
> +
> +static int artpec8_pcie_config_perstn(struct dw_pcie *pci,
> + enum artpec8_pcie_reg_bit val)
> +{
> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> + unsigned int bits;
> + int ret;
> +
> + if (val == PCIE_REG_BIT_HIGH)
> + bits = PCIE_PERSTN;
> + else
> + bits = 0;
> +
> + ret = regmap_update_bits(artpec8_ctrl->sysreg, FSYS_PCIE_CON,
> + PCIE_PERSTN, bits);
> +
> + return ret;
return regmap_update_bits(artpec8_ctrl->sysreg, ...):
> +}
> +
> +static void artpec8_pcie_stop_link(struct dw_pcie *pci)
> +{
> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> + u32 val;
> +
> + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
> + PCIE_APP_LTSSM_ENABLE);
> +
> + val &= ~PCIE_ELBI_LTSSM_ENABLE;
> + artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
> + PCIE_APP_LTSSM_ENABLE);
> +}
> +
> +static int artpec8_pcie_start_link(struct dw_pcie *pci)
> +{
> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> + u32 val;
> +
> + dw_pcie_dbi_ro_wr_en(pci);
> +
> + /* Equalization disable */
> + val = artpec8_pcie_read_dbi(pci, pci->dbi_base, PCIE_GEN3_RELATED_OFF,
> + 4);
> + artpec8_pcie_write_dbi(pci, pci->dbi_base, PCIE_GEN3_RELATED_OFF, 4,
> + val | PCIE_GEN3_EQUALIZATION_DISABLE);
> +
> + dw_pcie_dbi_ro_wr_dis(pci);
> +
> + /* assert LTSSM enable */
> + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
> + PCIE_APP_LTSSM_ENABLE);
> +
> + val |= PCIE_ELBI_LTSSM_ENABLE;
> + artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
> + PCIE_APP_LTSSM_ENABLE);
> +
> + return 0;
> +}
> +
> +static irqreturn_t artpec8_pcie_msi_irq_handler(int irq, void *arg)
> +{
> + struct artpec8_pcie *artpec8_ctrl = arg;
> + struct dw_pcie *pci = artpec8_ctrl->pci;
> + struct pcie_port *pp = &pci->pp;
> + u32 val;
> +
> + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base, PCIE_IRQ2_STS);
> +
> + if ((val & IRQ_MSI_ENABLE) == IRQ_MSI_ENABLE) {
> + val &= IRQ_MSI_ENABLE;
> + artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
> + PCIE_IRQ2_STS);
> + dw_handle_msi_irq(pp);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void artpec8_pcie_msi_init(struct artpec8_pcie *artpec8_ctrl)
> +{
> + u32 val;
> +
> + /* enable MSI interrupt */
> + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base, PCIE_IRQ2_EN);
> + val |= IRQ_MSI_ENABLE;
> + artpec8_pcie_writel(artpec8_ctrl->elbi_base, val, PCIE_IRQ2_EN);
> +}
> +
> +static void artpec8_pcie_enable_interrupts(struct artpec8_pcie *artpec8_ctrl)
> +{
> + if (IS_ENABLED(CONFIG_PCI_MSI))
> + artpec8_pcie_msi_init(artpec8_ctrl);
> +}
> +
> +static u32 artpec8_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
> + u32 reg, size_t size)
> +{
> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> + u32 val;
> + bool is_atu = false;
> +
> + if (base == pci->atu_base) {
> + is_atu = true;
> + base = pci->dbi_base;
> + regmap_write(artpec8_ctrl->sysreg,
> + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
> + FSYS_PCIE_DBI_ADDR_OVR_ATU);
> + }
> +
> + dw_pcie_read(base + reg, size, &val);
> +
> + if (is_atu)
> + regmap_write(artpec8_ctrl->sysreg,
> + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
> + FSYS_PCIE_DBI_ADDR_OVR_CDM);
> +
> + return val;
> +}
> +
> +static void artpec8_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
> + u32 reg, size_t size, u32 val)
> +{
> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> + bool is_atu = false;
> +
> + if (base == pci->atu_base) {
> + is_atu = true;
> + base = pci->dbi_base;
> + regmap_write(artpec8_ctrl->sysreg,
> + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
> + FSYS_PCIE_DBI_ADDR_OVR_ATU);
> + }
> +
> + dw_pcie_write(base + reg, size, val);
> +
> + if (is_atu)
> + regmap_write(artpec8_ctrl->sysreg,
> + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
> + FSYS_PCIE_DBI_ADDR_OVR_CDM);
> +}
> +
> +static void artpec8_pcie_write_dbi2(struct dw_pcie *pci, void __iomem *base,
> + u32 reg, size_t size, u32 val)
> +{
> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> +
> + regmap_write(artpec8_ctrl->sysreg,
> + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
> + FSYS_PCIE_DBI_ADDR_OVR_SHADOW);
> +
> + dw_pcie_write(base + reg, size, val);
> +
> + regmap_write(artpec8_ctrl->sysreg,
> + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
> + FSYS_PCIE_DBI_ADDR_OVR_CDM);
> +}
> +
> +static int artpec8_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)) {
> + *val = ~0;
PCI_SET_ERROR_RESPONSE(val);
> + return PCIBIOS_DEVICE_NOT_FOUND;
> + }
> +
> + *val = dw_pcie_read_dbi(pci, where, size);
> + return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int artpec8_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 artpec8_pci_ops = {
> + .read = artpec8_pcie_rd_own_conf,
> + .write = artpec8_pcie_wr_own_conf,
> +};
> +
> +static int artpec8_pcie_link_up(struct dw_pcie *pci)
> +{
> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> + u32 val;
> +
> + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
> + PCIE_ELBI_CXPL_DEBUG_00_31);
> +
> + return (val & LTSSM_STATE_MASK) == LTSSM_STATE_L0;
> +}
> +
> +static int artpec8_pcie_host_init(struct pcie_port *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> +
> + pp->bridge->ops = &artpec8_pci_ops;
> +
> + dw_pcie_writel_dbi(pci, PCIE_GEN3_RELATED_OFF,
> + (PCIE_GEN3_EQ_PHASE_2_3 |
> + PCIE_GEN3_RXEQ_PH01_EN |
> + PCIE_GEN3_RXEQ_RGRDLESS_RXTS));
> +
> + artpec8_pcie_enable_interrupts(artpec8_ctrl);
> +
> + return 0;
> +}
> +
> +static const struct dw_pcie_host_ops artpec8_pcie_host_ops = {
> + .host_init = artpec8_pcie_host_init,
> +};
> +
> +static u8 artpec8_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
> +{
> + u32 val;
> +
> + val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
> + pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
> +
> + if (val == 0xffffffff)
> + return 1;
> +
> + return 0;
> +}
> +
> +static void artpec8_pcie_ep_init(struct dw_pcie_ep *ep)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> + enum pci_barno bar;
Add blank line here and use usual multi-line comment style:
/*
* Currently PCIe EP core is not ...
> + /* Currently PCIe EP core is not setting iatu_unroll_enabled
> + * so let's handle it here. We need to find proper place to
> + * initialize this so that it can be used as for other EP
... can be used for ...
> + * controllers as well.
> + */
> + pci->iatu_unroll_enabled = artpec8_pcie_iatu_unroll_enabled(pci);
> +
> + for (bar = BAR_0; bar <= BAR_5; bar++)
> + dw_pcie_ep_reset_bar(pci, bar);
> +}
> +
> +static int artpec8_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> + enum pci_epc_irq_type type, u16 interrupt_num)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +
> + switch (type) {
> + case PCI_EPC_IRQ_LEGACY:
> + return -EINVAL;
> + case PCI_EPC_IRQ_MSI:
> + return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> + default:
> + dev_err(pci->dev, "UNKNOWN IRQ type\n");
> + }
> +
> + return 0;
> +}
> +
> +static const struct pci_epc_features artpec8_pcie_epc_features = {
> + .linkup_notifier = false,
> + .msi_capable = true,
> + .msix_capable = false,
> +};
> +
> +static const struct pci_epc_features*
> +artpec8_pcie_ep_get_features(struct dw_pcie_ep *ep)
> +{
> + return &artpec8_pcie_epc_features;
> +}
> +
> +static const struct dw_pcie_ep_ops artpec8_dw_pcie_ep_ops = {
> + .ep_init = artpec8_pcie_ep_init,
> + .raise_irq = artpec8_pcie_raise_irq,
> + .get_features = artpec8_pcie_ep_get_features,
> +};
> +
> +static int __init artpec8_add_pcie_ep(struct artpec8_pcie *artpec8_ctrl,
> + struct platform_device *pdev)
> +{
> + int ret;
> + struct dw_pcie_ep *ep;
> + struct dw_pcie *pci = artpec8_ctrl->pci;
> +
> + ep = &pci->ep;
Reorder locals and initialize ep as above.
> + ep->ops = &artpec8_dw_pcie_ep_ops;
> +
> + dw_pcie_writel_dbi(pci, PCIE_GEN3_RELATED_OFF,
> + (PCIE_GEN3_EQ_PHASE_2_3 |
> + PCIE_GEN3_RXEQ_PH01_EN |
> + PCIE_GEN3_RXEQ_RGRDLESS_RXTS));
> +
> + ret = dw_pcie_ep_init(ep);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int __init artpec8_add_pcie_port(struct artpec8_pcie *artpec8_ctrl,
> + struct platform_device *pdev)
> +{
> + struct dw_pcie *pci = artpec8_ctrl->pci;
> + struct pcie_port *pp = &pci->pp;
> + struct device *dev = &pdev->dev;
> + int ret;
> + int irq_flags;
> + int irq;
Reorder to be in order of use:
struct dw_pcie *pci = artpec8_ctrl->pci;
struct pcie_port *pp = &pci->pp;
struct device *dev = &pdev->dev;
int irq;
int irq_flags;
int ret;
> +
> + if (IS_ENABLED(CONFIG_PCI_MSI)) {
> + irq = platform_get_irq_byname(pdev, "intr");
> + if (!irq)
> + return -ENODEV;
> +
> + irq_flags = IRQF_SHARED | IRQF_NO_THREAD;
> +
> + ret = devm_request_irq(dev, irq, artpec8_pcie_msi_irq_handler,
> + irq_flags, "artpec8-pcie", artpec8_ctrl);
> + if (ret)
> + return ret;
> + }
> +
> + /* Prevent core for messing with the IRQ, since it's muxed */
Prevent core from ...
> + pp->msi_irq = -ENODEV;
> +
> + ret = dw_pcie_host_init(pp);
> + if (ret)
> + return ret;
> +
> + return 0;
return dw_pcie_host_init(pp);
> +}
> +
> +static const struct dw_pcie_ops artpec8_dw_pcie_ops = {
> + .read_dbi = artpec8_pcie_read_dbi,
> + .write_dbi = artpec8_pcie_write_dbi,
> + .write_dbi2 = artpec8_pcie_write_dbi2,
> + .start_link = artpec8_pcie_start_link,
> + .stop_link = artpec8_pcie_stop_link,
> + .link_up = artpec8_pcie_link_up,
> +};
> +
> +static int artpec8_pcie_probe(struct platform_device *pdev)
> +{
> + int ret;
> + struct dw_pcie *pci;
> + struct pcie_port *pp;
> + struct artpec8_pcie *artpec8_ctrl;
> + enum dw_pcie_device_mode mode;
> + struct device *dev = &pdev->dev;
> + const struct artpec8_pcie_pdata *pdata;
> + struct device_node *np = dev->of_node;
Reorder in order of use.
> + artpec8_ctrl = devm_kzalloc(dev, sizeof(*artpec8_ctrl), GFP_KERNEL);
> + if (!artpec8_ctrl)
> + return -ENOMEM;
> +
> + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> + if (!pci)
> + return -ENOMEM;
> +
> + pdata = (const struct artpec8_pcie_pdata *)
Unnecessary cast.
> + of_device_get_match_data(dev);
> + if (!pdata)
> + return -ENODEV;
> +
> + mode = (enum dw_pcie_device_mode)pdata->mode;
> +
> + artpec8_ctrl->pci = pci;
> + artpec8_ctrl->pdata = pdata;
> + artpec8_ctrl->mode = mode;
> +
> + pci->dev = dev;
> + pci->ops = pdata->dwc_ops;
> + pci->dbi_base2 = NULL;
> + pci->dbi_base = NULL;
> + pp = &pci->pp;
> + pp->ops = artpec8_ctrl->pdata->host_ops;
> +
> + if (mode == DW_PCIE_RC_TYPE)
> + artpec8_ctrl->link_id = of_alias_get_id(np, "pcierc");
> + else
> + artpec8_ctrl->link_id = of_alias_get_id(np, "pcieep");
> +
> + ret = artpec8_pcie_get_subsystem_resources(pdev, artpec8_ctrl);
> + if (ret)
> + return ret;
> +
> + if (pdata->res_ops && pdata->res_ops->get_mem_resources) {
> + ret = pdata->res_ops->get_mem_resources(pdev, artpec8_ctrl);
> + if (ret)
> + return ret;
> + }
> +
> + if (pdata->res_ops && pdata->res_ops->get_clk_resources) {
> + ret = pdata->res_ops->get_clk_resources(pdev, artpec8_ctrl);
> + if (ret)
> + return ret;
> +
> + ret = pdata->res_ops->init_clk_resources(artpec8_ctrl);
> + if (ret)
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, artpec8_ctrl);
> +
> + ret = artpec8_pcie_config_isolation(pci, PCIE_CLEAR_ISOLATION);
> + if (ret)
> + return ret;
> +
> + ret = artpec8_pcie_config_perstn(pci, PCIE_REG_BIT_HIGH);
> + if (ret)
> + return ret;
> +
> + artpec8_ctrl->phy = devm_of_phy_get(dev, np, NULL);
> + if (IS_ERR(artpec8_ctrl->phy))
> + return PTR_ERR(artpec8_ctrl->phy);
> +
> + phy_init(artpec8_ctrl->phy);
> + phy_reset(artpec8_ctrl->phy);
> +
> + switch (mode) {
> + case DW_PCIE_RC_TYPE:
> + artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_RC,
> + PCIE_ARTPEC8_DEVICE_TYPE);
> + ret = artpec8_add_pcie_port(artpec8_ctrl, pdev);
> + if (ret < 0)
Are there positive return values that indicate success? Most places
above you assume "ret != 0" means failure, so just curious why you
test "ret < 0" instead of just "ret".
> + goto fail_probe;
> + break;
> + case DW_PCIE_EP_TYPE:
> + artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_EP,
> + PCIE_ARTPEC8_DEVICE_TYPE);
> +
> + ret = artpec8_add_pcie_ep(artpec8_ctrl, pdev);
> + if (ret < 0)
Same question.
> + goto fail_probe;
> + break;
> + default:
> + ret = -EINVAL;
> + goto fail_probe;
> + }
> +
> + return 0;
> +
> +fail_probe:
> + phy_exit(artpec8_ctrl->phy);
> + if (pdata->res_ops && pdata->res_ops->deinit_clk_resources)
> + pdata->res_ops->deinit_clk_resources(artpec8_ctrl);
> +
> + return ret;
> +}
> +
> +static int __exit artpec8_pcie_remove(struct platform_device *pdev)
> +{
> + struct artpec8_pcie *artpec8_ctrl = platform_get_drvdata(pdev);
> + const struct artpec8_pcie_pdata *pdata = artpec8_ctrl->pdata;
> +
> + if (pdata->res_ops && pdata->res_ops->deinit_clk_resources)
> + pdata->res_ops->deinit_clk_resources(artpec8_ctrl);
> +
> + return 0;
> +}
> +
> +static const struct artpec8_pcie_pdata artpec8_pcie_rc_pdata = {
> + .dwc_ops = &artpec8_dw_pcie_ops,
> + .host_ops = &artpec8_pcie_host_ops,
> + .res_ops = &artpec8_pcie_rc_res_ops,
> + .mode = DW_PCIE_RC_TYPE,
> +};
> +
> +static const struct artpec8_pcie_pdata artpec8_pcie_ep_pdata = {
> + .dwc_ops = &artpec8_dw_pcie_ops,
> + .host_ops = &artpec8_pcie_host_ops,
> + .res_ops = &artpec8_pcie_ep_res_ops,
> + .mode = DW_PCIE_EP_TYPE,
> +};
> +
> +static const struct of_device_id artpec8_pcie_of_match[] = {
> + {
> + .compatible = "axis,artpec8-pcie",
> + .data = &artpec8_pcie_rc_pdata,
> + },
> + {
> + .compatible = "axis,artpec8-pcie-ep",
> + .data = &artpec8_pcie_ep_pdata,
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, artpec8_pcie_of_match);
> +
> +static struct platform_driver artpec8_pcie_driver = {
> + .probe = artpec8_pcie_probe,
> + .remove = __exit_p(artpec8_pcie_remove),
> + .driver = {
> + .name = "artpec8-pcie",
> + .of_match_table = artpec8_pcie_of_match,
> + },
> +};
> +
> +module_platform_driver(artpec8_pcie_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Jaeho Cho <[email protected]>");
> --
> 2.9.5
>
> --
> linux-phy mailing list
> [email protected]
> https://lists.infradead.org/mailman/listinfo/linux-phy
On 03/06/2022 04:34, Wangseok Lee wrote:
> Add support Axis, ARTPEC-8 SoC.
> ARTPEC-8 is the SoC platform of Axis Communications.
> This is based on arm64 and support GEN4 & 2lane.
> This PCIe controller is based on DesignWare Hardware core
> and uses DesignWare core functions to implement the driver.
>
> changes since v1 :
> improvement review comment of Krzysztof on driver code.
> -debug messages for probe or other functions.
> -Inconsistent coding style (different indentation in structure members).
> -Inconsistent code (artpec8_pcie_get_subsystem_resources() gets device
> from pdev and from pci so you have two same pointers;
> or artpec8_pcie_get_ep_mem_resources() stores dev
> as local variable but uses instead pdev->dev).
> -Not using devm_platform_ioremap_resource().
> -Printing messages in interrupt handlers.
> -Several local/static structures or array are not const.
>
> Signed-off-by: Wangseok Lee <[email protected]>
> ---
> drivers/pci/controller/dwc/Kconfig | 31 ++
> drivers/pci/controller/dwc/Makefile | 1 +
> drivers/pci/controller/dwc/pcie-artpec8.c | 864 ++++++++++++++++++++++++++++++
> 3 files changed, 896 insertions(+)
> create mode 100644 drivers/pci/controller/dwc/pcie-artpec8.c
>
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 62ce3ab..4aa6da8 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -222,6 +222,37 @@ config PCIE_ARTPEC6_EP
> Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
> endpoint mode. This uses the DesignWare core.
>
> +config PCIE_ARTPEC8
> + bool "Axis ARTPEC-8 PCIe controller"
> +
> +config PCIE_ARTPEC8_HOST
> + bool "Axis ARTPEC-8 PCIe controller Host Mode"
> + depends on ARCH_ARTPEC
|| COMPILE_TEST
and test it
> + depends on PCI_MSI_IRQ_DOMAIN
> + depends on PCI_ENDPOINT
> + select PCI_EPF_TEST
> + select PCIE_DW_HOST
> + select PCIE_ARTPEC8
> + help
> + Say 'Y' here to enable support for the PCIe controller in the
> + ARTPEC-8 SoC to work in host mode.
> + This PCIe controller is based on DesignWare Hardware core.
> + And uses DesignWare core functions to implement the driver.
> +
> +config PCIE_ARTPEC8_EP
> + bool "Axis ARTPEC-8 PCIe controller Endpoint Mode"
> + depends on ARCH_ARTPEC
|| COMPILE_TEST
and test it
> + depends on PCI_ENDPOINT
> + depends on PCI_ENDPOINT_CONFIGFS
> + select PCI_EPF_TEST
> + select PCIE_DW_EP
> + select PCIE_ARTPEC8
> + help
> + Say 'Y' here to enable support for the PCIe controller in the
> + ARTPEC-8 SoC to work in endpoint mode.
> + This PCIe controller is based on DesignWare Hardware core.
> + And uses DesignWare core functions to implement the driver.
> +
> config PCIE_ROCKCHIP_DW_HOST
> bool "Rockchip DesignWare PCIe controller"
> select PCIE_DW
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index 8ba7b67..b361022 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
> obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
> obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
> obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
> +obj-$(CONFIG_PCIE_ARTPEC8) += pcie-artpec8.o
This does not look properly ordered. Usually entries should not be added
at the end.
>
> # The following drivers are for devices that use the generic ACPI
> # pci_root.c driver but don't support standard ECAM config access.
> diff --git a/drivers/pci/controller/dwc/pcie-artpec8.c b/drivers/pci/controller/dwc/pcie-artpec8.c
> new file mode 100644
> index 0000000..d9ae9bf
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-artpec8.c
> @@ -0,0 +1,864 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * PCIe controller driver for Axis ARTPEC-8 SoC
> + *
> + * Copyright (C) 2019 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * Author: Jaeho Cho <[email protected]>
> + * This file is based on driver/pci/controller/dwc/pci-exynos.c
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/resource.h>
> +#include <linux/types.h>
> +#include <linux/phy/phy.h>
> +
> +#include "pcie-designware.h"
> +
> +#define to_artpec8_pcie(x) dev_get_drvdata((x)->dev)
> +
> +/* Gen3 Control Register */
> +#define PCIE_GEN3_RELATED_OFF 0x890
> +/* Disables equilzation feature */
> +#define PCIE_GEN3_EQUALIZATION_DISABLE (0x1 << 16)
> +#define PCIE_GEN3_EQ_PHASE_2_3 (0x1 << 9)
> +#define PCIE_GEN3_RXEQ_PH01_EN (0x1 << 12)
> +#define PCIE_GEN3_RXEQ_RGRDLESS_RXTS (0x1 << 13)
> +
> +#define FAST_LINK_MODE (7)
> +
> +/* PCIe ELBI registers */
> +#define PCIE_IRQ0_STS 0x000
> +#define PCIE_IRQ1_STS 0x004
> +#define PCIE_IRQ2_STS 0x008
> +#define PCIE_IRQ5_STS 0x00C
> +#define PCIE_IRQ0_EN 0x010
> +#define PCIE_IRQ1_EN 0x014
> +#define PCIE_IRQ2_EN 0x018
> +#define PCIE_IRQ5_EN 0x01C
> +#define IRQ_MSI_ENABLE BIT(20)
> +#define PCIE_APP_LTSSM_ENABLE 0x054
> +#define PCIE_ELBI_LTSSM_ENABLE 0x1
> +#define PCIE_ELBI_CXPL_DEBUG_00_31 0x2C8
> +#define PCIE_ELBI_CXPL_DEBUG_32_63 0x2CC
> +#define PCIE_ELBI_SMLH_LINK_UP BIT(4)
> +#define PCIE_ARTPEC8_DEVICE_TYPE 0x080
> +#define DEVICE_TYPE_EP 0x0
> +#define DEVICE_TYPE_LEG_EP 0x1
> +#define DEVICE_TYPE_RC 0x4
> +#define PCIE_ELBI_SLV_AWMISC 0x828
> +#define PCIE_ELBI_SLV_ARMISC 0x820
> +#define PCIE_ELBI_SLV_DBI_ENABLE BIT(21)
> +#define LTSSM_STATE_MASK 0x3f
> +#define LTSSM_STATE_L0 0x11
> +
> +/* FSYS SYSREG Offsets */
> +#define FSYS_PCIE_CON 0x424
> +#define PCIE_PERSTN BIT(5)
> +#define FSYS_PCIE_DBI_ADDR_CON 0x428
> +#define FSYS_PCIE_DBI_ADDR_OVR_CDM 0x00
> +#define FSYS_PCIE_DBI_ADDR_OVR_SHADOW 0x12
> +#define FSYS_PCIE_DBI_ADDR_OVR_ATU 0x36
> +
> +/* PMU SYSCON Offsets */
> +#define PMU_SYSCON_PCIE_ISOLATION 0x3200
> +
> +/* BUS P/S SYSCON Offsets */
> +#define BUS_SYSCON_BUS_PATH_ENABLE 0x0
> +
> +int artpec8_pcie_dbi_addr_con[] = {
1. I think I pointed before the need to constify everything which is const.
2. Missing static
3. definitions of static variables go after type declarations.
> + FSYS_PCIE_DBI_ADDR_CON
> +};
> +
> +struct artpec8_pcie {
> + struct dw_pcie *pci;
> + struct clk *pipe_clk;
> + struct clk *dbi_clk;
> + struct clk *mstr_clk;
> + struct clk *slv_clk;
Not really... Just use clk_bulk_api.
> + const struct artpec8_pcie_pdata *pdata;
> + void __iomem *elbi_base;
> + struct regmap *sysreg;
> + struct regmap *pmu_syscon;
> + struct regmap *bus_s_syscon;
> + struct regmap *bus_p_syscon;
> + enum dw_pcie_device_mode mode;
> + int link_id;
> + /* For Generic PHY Framework */
Skip comment, it's obvious.
> + struct phy *phy;
> +};
> +
> + /* fsys sysreg regmap handle */
> + artpec8_ctrl->sysreg =
> + syscon_regmap_lookup_by_phandle(dev->of_node,
> + "samsung,fsys-sysreg");
NAK.
Usage of undocumented properties. Every property must be documented.
Since you do not want to merge it with existing drivers (and more people
insist on that: https://lore.kernel.org/all/Ym+u9yYrV9mxkyWX@matsya/ ),
I am actually considering to NAK entire set if you do not post a user of
this - DTS. Mainly because we cannot verify how does that user look like
and such changes are sneaked in.
Best regards,
Krzysztof
On Fri, Jun 03, 2022 at 06:03:14PM +0200, Bjorn Helgaas wrote:
> In the subject, why do you tag this "axis"? There's an existing
> pcie-artpec6.c that uses the driver name ""artpec6-pcie" and the
> subject line tag "artpec6".
>
> This adds pcie-artpec8.c with driver name "artpec8-pcie", so the
> obvious choice would be "artpec8".
>
> I assume you evaluated the possibility of extending artpec6 to support
> artpec8 in addition to the artpec6 and artpec7 it already supports?
I think that the artpec6 & 7 version of the IP is quite different
in IP version, IP configuration and the glue-logic around the DWC IP.
Obviously integration of the IP is Samsung based, which artpec6 and 7 wasn't.
Even the IP supported by the old Exynos version of the driver seems
quite different in it's intergration. :-(
> On Fri, Jun 03, 2022 at 11:34:52AM +0900, Wangseok Lee wrote:
> > +#define LTSSM_STATE_L0 0x11
> > +
> > +/* FSYS SYSREG Offsets */
>
> The list below seems to inclue more than just register offsets.
... as I have some internal knowledge I can confirm that these are
glue logic registers and fields specific for ARTPEC-8, which
is what the comment "FSYS SYSREG Offsets" above very cryptically states.
Would this be clearer? "FSYS glue logic system registers"
> > +#define FSYS_PCIE_CON 0x424
> > +#define PCIE_PERSTN BIT(5)
> > +#define FSYS_PCIE_DBI_ADDR_CON 0x428
> > +#define FSYS_PCIE_DBI_ADDR_OVR_CDM 0x00
> > +#define FSYS_PCIE_DBI_ADDR_OVR_SHADOW 0x12
> > +#define FSYS_PCIE_DBI_ADDR_OVR_ATU 0x36
> > +
> > +/* PMU SYSCON Offsets */
And similarly:
PMU glue logic system registers
> > +#define PMU_SYSCON_PCIE_ISOLATION 0x3200
> > +
> > +/* BUS P/S SYSCON Offsets */
> > +#define BUS_SYSCON_BUS_PATH_ENABLE 0x0
> > +
> > +int artpec8_pcie_dbi_addr_con[] = {
> > + FSYS_PCIE_DBI_ADDR_CON
> > +};
> > +
> > +struct artpec8_pcie {
> > + struct dw_pcie *pci;
> > + struct clk *pipe_clk;
> > + struct clk *dbi_clk;
> > + struct clk *mstr_clk;
> > + struct clk *slv_clk;
> > + const struct artpec8_pcie_pdata *pdata;
> > + void __iomem *elbi_base;
> > + struct regmap *sysreg;
> > + struct regmap *pmu_syscon;
> > + struct regmap *bus_s_syscon;
> > + struct regmap *bus_p_syscon;
> > + enum dw_pcie_device_mode mode;
> > + int link_id;
> > + /* For Generic PHY Framework */
>
> Superfluous comment.
>
> > + struct phy *phy;
> > +};
> > +
> > +struct artpec8_pcie_res_ops {
> > + int (*get_mem_resources)(struct platform_device *pdev,
> > + struct artpec8_pcie *artpec8_ctrl);
> > + int (*get_clk_resources)(struct platform_device *pdev,
> > + struct artpec8_pcie *artpec8_ctrl);
> > + int (*init_clk_resources)(struct artpec8_pcie *artpec8_ctrl);
> > + void (*deinit_clk_resources)(struct artpec8_pcie *artpec8_ctrl);
> > +};
> > +
> > +struct artpec8_pcie_pdata {
> > + const struct dw_pcie_ops *dwc_ops;
> > + const struct dw_pcie_host_ops *host_ops;
>
> Fix indentation to match surrounding code.
>
> > + const struct artpec8_pcie_res_ops *res_ops;
> > + enum dw_pcie_device_mode mode;
> > +};
> > +
> > +enum artpec8_pcie_isolation {
> > + PCIE_CLEAR_ISOLATION = 0,
> > + PCIE_SET_ISOLATION = 1
> > +};
> > +
> > +enum artpec8_pcie_reg_bit {
> > + PCIE_REG_BIT_LOW = 0,
> > + PCIE_REG_BIT_HIGH = 1
> > +};
> > +
> > +static void artpec8_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
> > + u32 reg, size_t size, u32 val);
> > +static u32 artpec8_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
> > + u32 reg, size_t size);
> > +static void artpec8_pcie_writel(void __iomem *base, u32 val, u32 reg);
>
> Can you reorder the function definitions to avoid the need for these
> forward declarations?
>
> > +static int artpec8_pcie_get_subsystem_resources(struct platform_device *pdev,
> > + struct artpec8_pcie *artpec8_ctrl)
> > +{
> > + struct device *dev = &pdev->dev;
> > +
> > + /* External Local Bus interface(ELBI) Register */
> > + artpec8_ctrl->elbi_base = devm_platform_ioremap_resource_byname(pdev, "elbi");
>
> Rewrap to fit in 80 columns.
>
> > + if (IS_ERR(artpec8_ctrl->elbi_base)) {
> > + dev_err(dev, "failed to map elbi_base\n");
> > + return PTR_ERR(artpec8_ctrl->elbi_base);
> > + }
> > +
> > + /* fsys sysreg regmap handle */
>
> All these comments are superfluous since they only repeat the lookup
> arguments.
>
> > + artpec8_ctrl->sysreg =
> > + syscon_regmap_lookup_by_phandle(dev->of_node,
>
> The above two lines should fit on one line.
>
> > + "samsung,fsys-sysreg");
> > + if (IS_ERR(artpec8_ctrl->sysreg)) {
> > + dev_err(dev, "fsys sysreg regmap lookup failed.\n");
> > + return PTR_ERR(artpec8_ctrl->sysreg);
> > + }
> > +
> > + /* pmu syscon regmap handle */
> > + artpec8_ctrl->pmu_syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
> > + "samsung,syscon-phandle");
> > + if (IS_ERR(artpec8_ctrl->pmu_syscon)) {
> > + dev_err(dev, "pmu syscon regmap lookup failed.\n");
> > + return PTR_ERR(artpec8_ctrl->pmu_syscon);
> > + }
> > +
> > + /* bus s syscon regmap handle */
> > + artpec8_ctrl->bus_s_syscon =
> > + syscon_regmap_lookup_by_phandle(dev->of_node,
> > + "samsung,syscon-bus-s-fsys");
> > + if (IS_ERR(artpec8_ctrl->bus_s_syscon)) {
> > + dev_err(dev, "bus_s_syscon regmap lookup failed.\n");
> > + return PTR_ERR(artpec8_ctrl->bus_s_syscon);
> > + }
> > +
> > + /* bus p syscon regmap handle */
> > + artpec8_ctrl->bus_p_syscon =
> > + syscon_regmap_lookup_by_phandle(dev->of_node,
> > + "samsung,syscon-bus-p-fsys");
> > + if (IS_ERR(artpec8_ctrl->bus_p_syscon)) {
> > + dev_err(dev, "bus_p_syscon regmap lookup failed.\n");
> > + return PTR_ERR(artpec8_ctrl->bus_p_syscon);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int artpec8_pcie_get_rc_mem_resources(struct platform_device *pdev,
> > + struct artpec8_pcie *artpec8_ctrl)
> > +{
> > + struct dw_pcie *pci = artpec8_ctrl->pci;
> > +
> > + /* Data Bus Interface(DBI) Register */
> > + pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
> > + if (IS_ERR(pci->dbi_base))
> > + return PTR_ERR(pci->dbi_base);
> > +
> > + return 0;
> > +}
> > +
> > +static int artpec8_pcie_get_ep_mem_resources(struct platform_device *pdev,
> > + struct artpec8_pcie *artpec8_ctrl)
> > +{
> > + struct dw_pcie_ep *ep;
> > + struct dw_pcie *pci = artpec8_ctrl->pci;
> > + struct device *dev = &pdev->dev;
> > + struct resource *res;
> > +
> > + ep = &pci->ep;
>
> Reorder the locals above:
>
> struct dw_pcie *pci = artpec8_ctrl->pci;
> struct device *dev = &pdev->dev;
> struct dw_pcie_ep *ep = &pci->ep;
> struct resource *res;
>
> Then they're in the order you use them and you don't need the extra
> "ep = &pci->ep".
>
> > + pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
> > + if (IS_ERR(pci->dbi_base)) {
> > + dev_err(dev, "failed to map ep_dbics\n");
> > + return -ENOMEM;
> > + }
> > +
> > + pci->dbi_base2 = devm_platform_ioremap_resource_byname(pdev, "dbi2");
> > + if (IS_ERR(pci->dbi_base2)) {
> > + dev_err(dev, "failed to map ep_dbics2\n");
> > + return -ENOMEM;
> > + }
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> > + if (!res)
> > + return -EINVAL;
> > + ep->phys_base = res->start;
> > + ep->addr_size = resource_size(res);
> > +
> > + return 0;
> > +}
> > +
> > +static int artpec8_pcie_get_clk_resources(struct platform_device *pdev,
> > + struct artpec8_pcie *artpec8_ctrl)
> > +{
> > + struct device *dev = &pdev->dev;
> > +
> > + artpec8_ctrl->pipe_clk = devm_clk_get(dev, "pipe_clk");
> > + if (IS_ERR(artpec8_ctrl->pipe_clk)) {
> > + dev_err(dev, "couldn't get pipe clock\n");
> > + return -EINVAL;
> > + }
> > +
> > + artpec8_ctrl->dbi_clk = devm_clk_get(dev, "dbi_clk");
> > + if (IS_ERR(artpec8_ctrl->dbi_clk)) {
> > + dev_info(dev, "couldn't get dbi clk\n");
> > + return -EINVAL;
> > + }
> > +
> > + artpec8_ctrl->slv_clk = devm_clk_get(dev, "slv_clk");
> > + if (IS_ERR(artpec8_ctrl->slv_clk)) {
> > + dev_err(dev, "couldn't get slave clock\n");
> > + return -EINVAL;
> > + }
> > +
> > + artpec8_ctrl->mstr_clk = devm_clk_get(dev, "mstr_clk");
> > + if (IS_ERR(artpec8_ctrl->mstr_clk)) {
> > + dev_info(dev, "couldn't get master clk\n");
>
> It'd be nice if the err/info messages matched the exact DT name:
> "pipe_clk", "dbi_clk", slv_clk", etc.
>
> Why are some of the above dev_err() and others dev_info() when you
> return -EINVAL in all cases?
>
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int artpec8_pcie_init_clk_resources(struct artpec8_pcie *artpec8_ctrl)
> > +{
> > + clk_prepare_enable(artpec8_ctrl->pipe_clk);
> > + clk_prepare_enable(artpec8_ctrl->dbi_clk);
> > + clk_prepare_enable(artpec8_ctrl->mstr_clk);
> > + clk_prepare_enable(artpec8_ctrl->slv_clk);
> > +
> > + return 0;
> > +}
> > +
> > +static void artpec8_pcie_deinit_clk_resources(struct artpec8_pcie *artpec8_ctrl)
> > +{
> > + clk_disable_unprepare(artpec8_ctrl->slv_clk);
> > + clk_disable_unprepare(artpec8_ctrl->mstr_clk);
> > + clk_disable_unprepare(artpec8_ctrl->dbi_clk);
> > + clk_disable_unprepare(artpec8_ctrl->pipe_clk);
> > +}
> > +
> > +static const struct artpec8_pcie_res_ops artpec8_pcie_rc_res_ops = {
> > + .get_mem_resources = artpec8_pcie_get_rc_mem_resources,
> > + .get_clk_resources = artpec8_pcie_get_clk_resources,
> > + .init_clk_resources = artpec8_pcie_init_clk_resources,
> > + .deinit_clk_resources = artpec8_pcie_deinit_clk_resources,
> > +};
> > +
> > +static const struct artpec8_pcie_res_ops artpec8_pcie_ep_res_ops = {
> > + .get_mem_resources = artpec8_pcie_get_ep_mem_resources,
> > + .get_clk_resources = artpec8_pcie_get_clk_resources,
> > + .init_clk_resources = artpec8_pcie_init_clk_resources,
> > + .deinit_clk_resources = artpec8_pcie_deinit_clk_resources,
> > +};
> > +
> > +static void artpec8_pcie_writel(void __iomem *base, u32 val, u32 reg)
> > +{
> > + writel(val, base + reg);
> > +}
> > +
> > +static u32 artpec8_pcie_readl(void __iomem *base, u32 reg)
> > +{
> > + return readl(base + reg);
> > +}
> > +
> > +static int artpec8_pcie_config_phy_power_isolation(struct dw_pcie *pci,
> > + enum artpec8_pcie_reg_bit val)
> > +{
> > + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> > + int ret;
> > +
> > + ret = regmap_write(artpec8_ctrl->pmu_syscon, PMU_SYSCON_PCIE_ISOLATION,
> > + val);
> > +
> > + return ret;
>
> return regmap_write(artpec8_ctrl->pmu_syscon, ...);
>
> > +}
> > +
> > +static int artpec8_pcie_config_bus_enable(struct dw_pcie *pci,
> > + enum artpec8_pcie_reg_bit val)
> > +{
> > + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> > + int ret;
> > +
> > + ret = regmap_write(artpec8_ctrl->bus_p_syscon,
> > + BUS_SYSCON_BUS_PATH_ENABLE, val);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(artpec8_ctrl->bus_s_syscon,
> > + BUS_SYSCON_BUS_PATH_ENABLE, val);
> > + if (ret)
> > + return ret;
> > +
> > + return ret;
>
> return regmap_write(artpec8_ctrl->bus_s_syscon, ...);
>
> > +}
> > +
> > +static int artpec8_pcie_config_isolation(struct dw_pcie *pci,
> > + enum artpec8_pcie_isolation val)
> > +{
> > + int ret;
> > + /* reg_val[0] : for phy power isolation */
> > + /* reg_val[1] : for bus enable */
> > + enum artpec8_pcie_reg_bit reg_val[2];
> > +
> > + switch (val) {
> > + case PCIE_CLEAR_ISOLATION:
> > + reg_val[0] = PCIE_REG_BIT_LOW;
> > + reg_val[1] = PCIE_REG_BIT_HIGH;
> > + break;
> > + case PCIE_SET_ISOLATION:
> > + reg_val[0] = PCIE_REG_BIT_HIGH;
> > + reg_val[1] = PCIE_REG_BIT_LOW;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + ret = artpec8_pcie_config_phy_power_isolation(pci, reg_val[0]);
> > + if (ret)
> > + return ret;
> > +
> > + ret = artpec8_pcie_config_bus_enable(pci, reg_val[1]);
> > + if (ret)
> > + return ret;
> > +
> > + return ret;
>
> return artpec8_pcie_config_bus_enable(pci, ...);
>
> > +}
> > +
> > +static int artpec8_pcie_config_perstn(struct dw_pcie *pci,
> > + enum artpec8_pcie_reg_bit val)
> > +{
> > + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> > + unsigned int bits;
> > + int ret;
> > +
> > + if (val == PCIE_REG_BIT_HIGH)
> > + bits = PCIE_PERSTN;
> > + else
> > + bits = 0;
> > +
> > + ret = regmap_update_bits(artpec8_ctrl->sysreg, FSYS_PCIE_CON,
> > + PCIE_PERSTN, bits);
> > +
> > + return ret;
>
> return regmap_update_bits(artpec8_ctrl->sysreg, ...):
>
> > +}
> > +
> > +static void artpec8_pcie_stop_link(struct dw_pcie *pci)
> > +{
> > + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> > + u32 val;
> > +
> > + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
> > + PCIE_APP_LTSSM_ENABLE);
> > +
> > + val &= ~PCIE_ELBI_LTSSM_ENABLE;
> > + artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
> > + PCIE_APP_LTSSM_ENABLE);
> > +}
> > +
> > +static int artpec8_pcie_start_link(struct dw_pcie *pci)
> > +{
> > + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> > + u32 val;
> > +
> > + dw_pcie_dbi_ro_wr_en(pci);
> > +
> > + /* Equalization disable */
> > + val = artpec8_pcie_read_dbi(pci, pci->dbi_base, PCIE_GEN3_RELATED_OFF,
> > + 4);
> > + artpec8_pcie_write_dbi(pci, pci->dbi_base, PCIE_GEN3_RELATED_OFF, 4,
> > + val | PCIE_GEN3_EQUALIZATION_DISABLE);
> > +
> > + dw_pcie_dbi_ro_wr_dis(pci);
> > +
> > + /* assert LTSSM enable */
> > + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
> > + PCIE_APP_LTSSM_ENABLE);
> > +
> > + val |= PCIE_ELBI_LTSSM_ENABLE;
> > + artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
> > + PCIE_APP_LTSSM_ENABLE);
> > +
> > + return 0;
> > +}
> > +
> > +static irqreturn_t artpec8_pcie_msi_irq_handler(int irq, void *arg)
> > +{
> > + struct artpec8_pcie *artpec8_ctrl = arg;
> > + struct dw_pcie *pci = artpec8_ctrl->pci;
> > + struct pcie_port *pp = &pci->pp;
> > + u32 val;
> > +
> > + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base, PCIE_IRQ2_STS);
> > +
> > + if ((val & IRQ_MSI_ENABLE) == IRQ_MSI_ENABLE) {
> > + val &= IRQ_MSI_ENABLE;
> > + artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
> > + PCIE_IRQ2_STS);
> > + dw_handle_msi_irq(pp);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static void artpec8_pcie_msi_init(struct artpec8_pcie *artpec8_ctrl)
> > +{
> > + u32 val;
> > +
> > + /* enable MSI interrupt */
> > + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base, PCIE_IRQ2_EN);
> > + val |= IRQ_MSI_ENABLE;
> > + artpec8_pcie_writel(artpec8_ctrl->elbi_base, val, PCIE_IRQ2_EN);
> > +}
> > +
> > +static void artpec8_pcie_enable_interrupts(struct artpec8_pcie *artpec8_ctrl)
> > +{
> > + if (IS_ENABLED(CONFIG_PCI_MSI))
> > + artpec8_pcie_msi_init(artpec8_ctrl);
> > +}
> > +
> > +static u32 artpec8_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
> > + u32 reg, size_t size)
> > +{
> > + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> > + u32 val;
> > + bool is_atu = false;
> > +
> > + if (base == pci->atu_base) {
> > + is_atu = true;
> > + base = pci->dbi_base;
> > + regmap_write(artpec8_ctrl->sysreg,
> > + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
> > + FSYS_PCIE_DBI_ADDR_OVR_ATU);
> > + }
> > +
> > + dw_pcie_read(base + reg, size, &val);
> > +
> > + if (is_atu)
> > + regmap_write(artpec8_ctrl->sysreg,
> > + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
> > + FSYS_PCIE_DBI_ADDR_OVR_CDM);
> > +
> > + return val;
> > +}
> > +
> > +static void artpec8_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
> > + u32 reg, size_t size, u32 val)
> > +{
> > + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> > + bool is_atu = false;
> > +
> > + if (base == pci->atu_base) {
> > + is_atu = true;
> > + base = pci->dbi_base;
> > + regmap_write(artpec8_ctrl->sysreg,
> > + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
> > + FSYS_PCIE_DBI_ADDR_OVR_ATU);
> > + }
> > +
> > + dw_pcie_write(base + reg, size, val);
> > +
> > + if (is_atu)
> > + regmap_write(artpec8_ctrl->sysreg,
> > + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
> > + FSYS_PCIE_DBI_ADDR_OVR_CDM);
> > +}
> > +
> > +static void artpec8_pcie_write_dbi2(struct dw_pcie *pci, void __iomem *base,
> > + u32 reg, size_t size, u32 val)
> > +{
> > + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> > +
> > + regmap_write(artpec8_ctrl->sysreg,
> > + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
> > + FSYS_PCIE_DBI_ADDR_OVR_SHADOW);
> > +
> > + dw_pcie_write(base + reg, size, val);
> > +
> > + regmap_write(artpec8_ctrl->sysreg,
> > + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
> > + FSYS_PCIE_DBI_ADDR_OVR_CDM);
> > +}
> > +
> > +static int artpec8_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)) {
> > + *val = ~0;
>
> PCI_SET_ERROR_RESPONSE(val);
>
> > + return PCIBIOS_DEVICE_NOT_FOUND;
> > + }
> > +
> > + *val = dw_pcie_read_dbi(pci, where, size);
> > + return PCIBIOS_SUCCESSFUL;
> > +}
> > +
> > +static int artpec8_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 artpec8_pci_ops = {
> > + .read = artpec8_pcie_rd_own_conf,
> > + .write = artpec8_pcie_wr_own_conf,
> > +};
> > +
> > +static int artpec8_pcie_link_up(struct dw_pcie *pci)
> > +{
> > + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> > + u32 val;
> > +
> > + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
> > + PCIE_ELBI_CXPL_DEBUG_00_31);
> > +
> > + return (val & LTSSM_STATE_MASK) == LTSSM_STATE_L0;
> > +}
> > +
> > +static int artpec8_pcie_host_init(struct pcie_port *pp)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
> > +
> > + pp->bridge->ops = &artpec8_pci_ops;
> > +
> > + dw_pcie_writel_dbi(pci, PCIE_GEN3_RELATED_OFF,
> > + (PCIE_GEN3_EQ_PHASE_2_3 |
> > + PCIE_GEN3_RXEQ_PH01_EN |
> > + PCIE_GEN3_RXEQ_RGRDLESS_RXTS));
> > +
> > + artpec8_pcie_enable_interrupts(artpec8_ctrl);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dw_pcie_host_ops artpec8_pcie_host_ops = {
> > + .host_init = artpec8_pcie_host_init,
> > +};
> > +
> > +static u8 artpec8_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
> > +{
> > + u32 val;
> > +
> > + val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
> > + pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
> > +
> > + if (val == 0xffffffff)
> > + return 1;
> > +
> > + return 0;
> > +}
> > +
> > +static void artpec8_pcie_ep_init(struct dw_pcie_ep *ep)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > + enum pci_barno bar;
>
> Add blank line here and use usual multi-line comment style:
>
> /*
> * Currently PCIe EP core is not ...
>
> > + /* Currently PCIe EP core is not setting iatu_unroll_enabled
> > + * so let's handle it here. We need to find proper place to
> > + * initialize this so that it can be used as for other EP
>
> ... can be used for ...
>
> > + * controllers as well.
> > + */
> > + pci->iatu_unroll_enabled = artpec8_pcie_iatu_unroll_enabled(pci);
> > +
> > + for (bar = BAR_0; bar <= BAR_5; bar++)
> > + dw_pcie_ep_reset_bar(pci, bar);
> > +}
> > +
> > +static int artpec8_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> > + enum pci_epc_irq_type type, u16 interrupt_num)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > + switch (type) {
> > + case PCI_EPC_IRQ_LEGACY:
> > + return -EINVAL;
> > + case PCI_EPC_IRQ_MSI:
> > + return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> > + default:
> > + dev_err(pci->dev, "UNKNOWN IRQ type\n");
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct pci_epc_features artpec8_pcie_epc_features = {
> > + .linkup_notifier = false,
> > + .msi_capable = true,
> > + .msix_capable = false,
> > +};
> > +
> > +static const struct pci_epc_features*
> > +artpec8_pcie_ep_get_features(struct dw_pcie_ep *ep)
> > +{
> > + return &artpec8_pcie_epc_features;
> > +}
> > +
> > +static const struct dw_pcie_ep_ops artpec8_dw_pcie_ep_ops = {
> > + .ep_init = artpec8_pcie_ep_init,
> > + .raise_irq = artpec8_pcie_raise_irq,
> > + .get_features = artpec8_pcie_ep_get_features,
> > +};
> > +
> > +static int __init artpec8_add_pcie_ep(struct artpec8_pcie *artpec8_ctrl,
> > + struct platform_device *pdev)
> > +{
> > + int ret;
> > + struct dw_pcie_ep *ep;
> > + struct dw_pcie *pci = artpec8_ctrl->pci;
> > +
> > + ep = &pci->ep;
>
> Reorder locals and initialize ep as above.
>
> > + ep->ops = &artpec8_dw_pcie_ep_ops;
> > +
> > + dw_pcie_writel_dbi(pci, PCIE_GEN3_RELATED_OFF,
> > + (PCIE_GEN3_EQ_PHASE_2_3 |
> > + PCIE_GEN3_RXEQ_PH01_EN |
> > + PCIE_GEN3_RXEQ_RGRDLESS_RXTS));
> > +
> > + ret = dw_pcie_ep_init(ep);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int __init artpec8_add_pcie_port(struct artpec8_pcie *artpec8_ctrl,
> > + struct platform_device *pdev)
> > +{
> > + struct dw_pcie *pci = artpec8_ctrl->pci;
> > + struct pcie_port *pp = &pci->pp;
> > + struct device *dev = &pdev->dev;
> > + int ret;
> > + int irq_flags;
> > + int irq;
>
> Reorder to be in order of use:
>
> struct dw_pcie *pci = artpec8_ctrl->pci;
> struct pcie_port *pp = &pci->pp;
> struct device *dev = &pdev->dev;
> int irq;
> int irq_flags;
> int ret;
>
> > +
> > + if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > + irq = platform_get_irq_byname(pdev, "intr");
> > + if (!irq)
> > + return -ENODEV;
> > +
> > + irq_flags = IRQF_SHARED | IRQF_NO_THREAD;
> > +
> > + ret = devm_request_irq(dev, irq, artpec8_pcie_msi_irq_handler,
> > + irq_flags, "artpec8-pcie", artpec8_ctrl);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + /* Prevent core for messing with the IRQ, since it's muxed */
>
> Prevent core from ...
>
> > + pp->msi_irq = -ENODEV;
> > +
> > + ret = dw_pcie_host_init(pp);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> return dw_pcie_host_init(pp);
>
> > +}
> > +
> > +static const struct dw_pcie_ops artpec8_dw_pcie_ops = {
> > + .read_dbi = artpec8_pcie_read_dbi,
> > + .write_dbi = artpec8_pcie_write_dbi,
> > + .write_dbi2 = artpec8_pcie_write_dbi2,
> > + .start_link = artpec8_pcie_start_link,
> > + .stop_link = artpec8_pcie_stop_link,
> > + .link_up = artpec8_pcie_link_up,
> > +};
> > +
> > +static int artpec8_pcie_probe(struct platform_device *pdev)
> > +{
> > + int ret;
> > + struct dw_pcie *pci;
> > + struct pcie_port *pp;
> > + struct artpec8_pcie *artpec8_ctrl;
> > + enum dw_pcie_device_mode mode;
> > + struct device *dev = &pdev->dev;
> > + const struct artpec8_pcie_pdata *pdata;
> > + struct device_node *np = dev->of_node;
>
> Reorder in order of use.
>
> > + artpec8_ctrl = devm_kzalloc(dev, sizeof(*artpec8_ctrl), GFP_KERNEL);
> > + if (!artpec8_ctrl)
> > + return -ENOMEM;
> > +
> > + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > + if (!pci)
> > + return -ENOMEM;
> > +
> > + pdata = (const struct artpec8_pcie_pdata *)
>
> Unnecessary cast.
>
> > + of_device_get_match_data(dev);
> > + if (!pdata)
> > + return -ENODEV;
> > +
> > + mode = (enum dw_pcie_device_mode)pdata->mode;
> > +
> > + artpec8_ctrl->pci = pci;
> > + artpec8_ctrl->pdata = pdata;
> > + artpec8_ctrl->mode = mode;
> > +
> > + pci->dev = dev;
> > + pci->ops = pdata->dwc_ops;
> > + pci->dbi_base2 = NULL;
> > + pci->dbi_base = NULL;
> > + pp = &pci->pp;
> > + pp->ops = artpec8_ctrl->pdata->host_ops;
> > +
> > + if (mode == DW_PCIE_RC_TYPE)
> > + artpec8_ctrl->link_id = of_alias_get_id(np, "pcierc");
> > + else
> > + artpec8_ctrl->link_id = of_alias_get_id(np, "pcieep");
> > +
> > + ret = artpec8_pcie_get_subsystem_resources(pdev, artpec8_ctrl);
> > + if (ret)
> > + return ret;
> > +
> > + if (pdata->res_ops && pdata->res_ops->get_mem_resources) {
> > + ret = pdata->res_ops->get_mem_resources(pdev, artpec8_ctrl);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (pdata->res_ops && pdata->res_ops->get_clk_resources) {
> > + ret = pdata->res_ops->get_clk_resources(pdev, artpec8_ctrl);
> > + if (ret)
> > + return ret;
> > +
> > + ret = pdata->res_ops->init_clk_resources(artpec8_ctrl);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + platform_set_drvdata(pdev, artpec8_ctrl);
> > +
> > + ret = artpec8_pcie_config_isolation(pci, PCIE_CLEAR_ISOLATION);
> > + if (ret)
> > + return ret;
> > +
> > + ret = artpec8_pcie_config_perstn(pci, PCIE_REG_BIT_HIGH);
> > + if (ret)
> > + return ret;
> > +
> > + artpec8_ctrl->phy = devm_of_phy_get(dev, np, NULL);
> > + if (IS_ERR(artpec8_ctrl->phy))
> > + return PTR_ERR(artpec8_ctrl->phy);
> > +
> > + phy_init(artpec8_ctrl->phy);
> > + phy_reset(artpec8_ctrl->phy);
> > +
> > + switch (mode) {
> > + case DW_PCIE_RC_TYPE:
> > + artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_RC,
> > + PCIE_ARTPEC8_DEVICE_TYPE);
> > + ret = artpec8_add_pcie_port(artpec8_ctrl, pdev);
> > + if (ret < 0)
>
> Are there positive return values that indicate success? Most places
> above you assume "ret != 0" means failure, so just curious why you
> test "ret < 0" instead of just "ret".
>
> > + goto fail_probe;
> > + break;
> > + case DW_PCIE_EP_TYPE:
> > + artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_EP,
> > + PCIE_ARTPEC8_DEVICE_TYPE);
> > +
> > + ret = artpec8_add_pcie_ep(artpec8_ctrl, pdev);
> > + if (ret < 0)
>
> Same question.
>
> > + goto fail_probe;
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + goto fail_probe;
> > + }
> > +
> > + return 0;
> > +
> > +fail_probe:
> > + phy_exit(artpec8_ctrl->phy);
> > + if (pdata->res_ops && pdata->res_ops->deinit_clk_resources)
> > + pdata->res_ops->deinit_clk_resources(artpec8_ctrl);
> > +
> > + return ret;
> > +}
> > +
> > +static int __exit artpec8_pcie_remove(struct platform_device *pdev)
> > +{
> > + struct artpec8_pcie *artpec8_ctrl = platform_get_drvdata(pdev);
> > + const struct artpec8_pcie_pdata *pdata = artpec8_ctrl->pdata;
> > +
> > + if (pdata->res_ops && pdata->res_ops->deinit_clk_resources)
> > + pdata->res_ops->deinit_clk_resources(artpec8_ctrl);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct artpec8_pcie_pdata artpec8_pcie_rc_pdata = {
> > + .dwc_ops = &artpec8_dw_pcie_ops,
> > + .host_ops = &artpec8_pcie_host_ops,
> > + .res_ops = &artpec8_pcie_rc_res_ops,
> > + .mode = DW_PCIE_RC_TYPE,
> > +};
> > +
> > +static const struct artpec8_pcie_pdata artpec8_pcie_ep_pdata = {
> > + .dwc_ops = &artpec8_dw_pcie_ops,
> > + .host_ops = &artpec8_pcie_host_ops,
> > + .res_ops = &artpec8_pcie_ep_res_ops,
> > + .mode = DW_PCIE_EP_TYPE,
> > +};
> > +
> > +static const struct of_device_id artpec8_pcie_of_match[] = {
> > + {
> > + .compatible = "axis,artpec8-pcie",
> > + .data = &artpec8_pcie_rc_pdata,
> > + },
> > + {
> > + .compatible = "axis,artpec8-pcie-ep",
> > + .data = &artpec8_pcie_ep_pdata,
> > + },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, artpec8_pcie_of_match);
> > +
> > +static struct platform_driver artpec8_pcie_driver = {
> > + .probe = artpec8_pcie_probe,
> > + .remove = __exit_p(artpec8_pcie_remove),
> > + .driver = {
> > + .name = "artpec8-pcie",
> > + .of_match_table = artpec8_pcie_of_match,
> > + },
> > +};
> > +
> > +module_platform_driver(artpec8_pcie_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Jaeho Cho <[email protected]>");
/^JN - Jesper Nilsson
--
Jesper Nilsson -- [email protected]
On 06/06/2022 19:25, Krzysztof Kozlowski wrote:
> On 03/06/2022 04:34, Wangseok Lee wrote:
>> Add support Axis, ARTPEC-8 SoC.
>> ARTPEC-8 is the SoC platform of Axis Communications.
>> This is based on arm64 and support GEN4 & 2lane.
>> This PCIe controller is based on DesignWare Hardware core
>> and uses DesignWare core functions to implement the driver.
>>
>> changes since v1 :
>> improvement review comment of Krzysztof on driver code.
>> -debug messages for probe or other functions.
>> -Inconsistent coding style (different indentation in structure members).
>> -Inconsistent code (artpec8_pcie_get_subsystem_resources() gets device
>> from pdev and from pci so you have two same pointers;
>> or artpec8_pcie_get_ep_mem_resources() stores dev
>> as local variable but uses instead pdev->dev).
>> -Not using devm_platform_ioremap_resource().
>> -Printing messages in interrupt handlers.
>> -Several local/static structures or array are not const.
>>
>> Signed-off-by: Wangseok Lee <[email protected]>
>> ---
>> drivers/pci/controller/dwc/Kconfig | 31 ++
>> drivers/pci/controller/dwc/Makefile | 1 +
>> drivers/pci/controller/dwc/pcie-artpec8.c | 864 ++++++++++++++++++++++++++++++
>> 3 files changed, 896 insertions(+)
>> create mode 100644 drivers/pci/controller/dwc/pcie-artpec8.c
>>
>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
>> index 62ce3ab..4aa6da8 100644
>> --- a/drivers/pci/controller/dwc/Kconfig
>> +++ b/drivers/pci/controller/dwc/Kconfig
>> @@ -222,6 +222,37 @@ config PCIE_ARTPEC6_EP
>> Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>> endpoint mode. This uses the DesignWare core.
>>
>> +config PCIE_ARTPEC8
>> + bool "Axis ARTPEC-8 PCIe controller"
>> +
>> +config PCIE_ARTPEC8_HOST
>> + bool "Axis ARTPEC-8 PCIe controller Host Mode"
>> + depends on ARCH_ARTPEC
>
> || COMPILE_TEST
> and test it
>
Ok, I will add 'COMPILE_TEST'
And then test.
>> + depends on PCI_MSI_IRQ_DOMAIN
>> + depends on PCI_ENDPOINT
>> + select PCI_EPF_TEST
>> + select PCIE_DW_HOST
>> + select PCIE_ARTPEC8
>> + help
>> + Say 'Y' here to enable support for the PCIe controller in the
>> + ARTPEC-8 SoC to work in host mode.
>> + This PCIe controller is based on DesignWare Hardware core.
>> + And uses DesignWare core functions to implement the driver.
>> +
>> +config PCIE_ARTPEC8_EP
>> + bool "Axis ARTPEC-8 PCIe controller Endpoint Mode"
>> + depends on ARCH_ARTPEC
>
> || COMPILE_TEST
> and test it
>
>
Ok, I will add 'COMPILE_TEST'
And then test.
>> + depends on PCI_ENDPOINT
>> + depends on PCI_ENDPOINT_CONFIGFS
>> + select PCI_EPF_TEST
>> + select PCIE_DW_EP
>> + select PCIE_ARTPEC8
>> + help
>> + Say 'Y' here to enable support for the PCIe controller in the
>> + ARTPEC-8 SoC to work in endpoint mode.
>> + This PCIe controller is based on DesignWare Hardware core.
>> + And uses DesignWare core functions to implement the driver.
>> +
>> config PCIE_ROCKCHIP_DW_HOST
>> bool "Rockchip DesignWare PCIe controller"
>> select PCIE_DW
>> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
>> index 8ba7b67..b361022 100644
>> --- a/drivers/pci/controller/dwc/Makefile
>> +++ b/drivers/pci/controller/dwc/Makefile
>> @@ -25,6 +25,7 @@ obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
>> obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>> obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
>> obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
>> +obj-$(CONFIG_PCIE_ARTPEC8) += pcie-artpec8.o
>
> This does not look properly ordered. Usually entries should not be added
> at the end.
>
I'll move to the 'CONFIG_PCIE_Axxx'.
>>
>> # The following drivers are for devices that use the generic ACPI
>> # pci_root.c driver but don't support standard ECAM config access.
>> diff --git a/drivers/pci/controller/dwc/pcie-artpec8.c b/drivers/pci/controller/dwc/pcie-artpec8.c
>> new file mode 100644
>> index 0000000..d9ae9bf
>> --- /dev/null
>> +++ b/drivers/pci/controller/dwc/pcie-artpec8.c
>> @@ -0,0 +1,864 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * PCIe controller driver for Axis ARTPEC-8 SoC
>> + *
>> + * Copyright (C) 2019 Samsung Electronics Co., Ltd.
>> + * http://www.samsung.com
>> + *
>> + * Author: Jaeho Cho <[email protected]>
>> + * This file is based on driver/pci/controller/dwc/pci-exynos.c
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/resource.h>
>> +#include <linux/types.h>
>> +#include <linux/phy/phy.h>
>> +
>> +#include "pcie-designware.h"
>> +
>> +#define to_artpec8_pcie(x) dev_get_drvdata((x)->dev)
>> +
>> +/* Gen3 Control Register */
>> +#define PCIE_GEN3_RELATED_OFF 0x890
>> +/* Disables equilzation feature */
>> +#define PCIE_GEN3_EQUALIZATION_DISABLE (0x1 << 16)
>> +#define PCIE_GEN3_EQ_PHASE_2_3 (0x1 << 9)
>> +#define PCIE_GEN3_RXEQ_PH01_EN (0x1 << 12)
>> +#define PCIE_GEN3_RXEQ_RGRDLESS_RXTS (0x1 << 13)
>> +
>> +#define FAST_LINK_MODE (7)
>> +
>> +/* PCIe ELBI registers */
>> +#define PCIE_IRQ0_STS 0x000
>> +#define PCIE_IRQ1_STS 0x004
>> +#define PCIE_IRQ2_STS 0x008
>> +#define PCIE_IRQ5_STS 0x00C
>> +#define PCIE_IRQ0_EN 0x010
>> +#define PCIE_IRQ1_EN 0x014
>> +#define PCIE_IRQ2_EN 0x018
>> +#define PCIE_IRQ5_EN 0x01C
>> +#define IRQ_MSI_ENABLE BIT(20)
>> +#define PCIE_APP_LTSSM_ENABLE 0x054
>> +#define PCIE_ELBI_LTSSM_ENABLE 0x1
>> +#define PCIE_ELBI_CXPL_DEBUG_00_31 0x2C8
>> +#define PCIE_ELBI_CXPL_DEBUG_32_63 0x2CC
>> +#define PCIE_ELBI_SMLH_LINK_UP BIT(4)
>> +#define PCIE_ARTPEC8_DEVICE_TYPE 0x080
>> +#define DEVICE_TYPE_EP 0x0
>> +#define DEVICE_TYPE_LEG_EP 0x1
>> +#define DEVICE_TYPE_RC 0x4
>> +#define PCIE_ELBI_SLV_AWMISC 0x828
>> +#define PCIE_ELBI_SLV_ARMISC 0x820
>> +#define PCIE_ELBI_SLV_DBI_ENABLE BIT(21)
>> +#define LTSSM_STATE_MASK 0x3f
>> +#define LTSSM_STATE_L0 0x11
>> +
>> +/* FSYS SYSREG Offsets */
>> +#define FSYS_PCIE_CON 0x424
>> +#define PCIE_PERSTN BIT(5)
>> +#define FSYS_PCIE_DBI_ADDR_CON 0x428
>> +#define FSYS_PCIE_DBI_ADDR_OVR_CDM 0x00
>> +#define FSYS_PCIE_DBI_ADDR_OVR_SHADOW 0x12
>> +#define FSYS_PCIE_DBI_ADDR_OVR_ATU 0x36
>> +
>> +/* PMU SYSCON Offsets */
>> +#define PMU_SYSCON_PCIE_ISOLATION 0x3200
>> +
>> +/* BUS P/S SYSCON Offsets */
>> +#define BUS_SYSCON_BUS_PATH_ENABLE 0x0
>> +
>> +int artpec8_pcie_dbi_addr_con[] = {
>
> 1. I think I pointed before the need to constify everything which is const.
> 2. Missing static
> 3. definitions of static variables go after type declarations.
>
Ok, i will modify to static const type.
>> + FSYS_PCIE_DBI_ADDR_CON
>> +};
>> +
>> +struct artpec8_pcie {
>> + struct dw_pcie *pci;
>> + struct clk *pipe_clk;
>> + struct clk *dbi_clk;
>> + struct clk *mstr_clk;
>> + struct clk *slv_clk;
>
> Not really... Just use clk_bulk_api.
>
Ok, i will modify to use clk_bilk_api.
>> + const struct artpec8_pcie_pdata *pdata;
>> + void __iomem *elbi_base;
>> + struct regmap *sysreg;
>> + struct regmap *pmu_syscon;
>> + struct regmap *bus_s_syscon;
>> + struct regmap *bus_p_syscon;
>> + enum dw_pcie_device_mode mode;
>> + int link_id;
>> + /* For Generic PHY Framework */
>
> Skip comment, it's obvious.
>
Ok.
>> + struct phy *phy;
> +};
>> +
>
>> + /* fsys sysreg regmap handle */
>> + artpec8_ctrl->sysreg =
>> + syscon_regmap_lookup_by_phandle(dev->of_node,
>> + "samsung,fsys-sysreg");
>
> NAK.
>
> Usage of undocumented properties. Every property must be documented.
>
> Since you do not want to merge it with existing drivers (and more people
> insist on that: https://lore.kernel.org/all/Ym+u9yYrV9mxkyWX@matsya/ ),
> I am actually considering to NAK entire set if you do not post a user of
> this - DTS. Mainly because we cannot verify how does that user look like
> and such changes are sneaked in.
>
Ok, sure .
I will should be documented the all property include subsystem resource.
> Best regards,
> Krzysztof
Thank you for kindness reivew.
Best regards,
Wangseok Lee
On 06/04/2022 01:03, Bjorn Helgaas wrote:
> In the subject, why do you tag this "axis"? There's an existing
> pcie-artpec6.c that uses the driver name ""artpec6-pcie" and the
> subject line tag "artpec6".
>
> This adds pcie-artpec8.c with driver name "artpec8-pcie", so the
> obvious choice would be "artpec8".
>
> I assume you evaluated the possibility of extending artpec6 to support
> artpec8 in addition to the artpec6 and artpec7 it already supports?
>
"pcie-artpec6. c" supports artpec6 and artpec7 H/W.
artpec8 can not be expanded because H/W configuration is
completely different from artpec6/7.
phy and sub controller are different.
> On Fri, Jun 03, 2022 at 11:34:52AM +0900, Wangseok Lee wrote:
>> Add support Axis, ARTPEC-8 SoC.
>> ARTPEC-8 is the SoC platform of Axis Communications.
>> This is based on arm64 and support GEN4 & 2lane.
>> This PCIe controller is based on DesignWare Hardware core
>> and uses DesignWare core functions to implement the driver.
>
> Add blank lines between paragraphs.
>
> Wrap lines to fill 75 columns.
>
I will fix it.
>> changes since v1 :
>> improvement review comment of Krzysztof on driver code.
>> -debug messages for probe or other functions.
>> -Inconsistent coding style (different indentation in structure members).
>> -Inconsistent code (artpec8_pcie_get_subsystem_resources() gets device
>> from pdev and from pci so you have two same pointers;
>> or artpec8_pcie_get_ep_mem_resources() stores dev
>> as local variable but uses instead pdev->dev).
>> -Not using devm_platform_ioremap_resource().
>> -Printing messages in interrupt handlers.
>> -Several local/static structures or array are not const.
>
> Thanks for the "changes since v1" notes. You can put them below the
> "---" since there's no need for them in the permanent git commit log:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v5.18#n675
i will fix it.
>> Signed-off-by: Wangseok Lee <[email protected]>
>
> Why is there no signoff from Jaeho Cho <[email protected]>?
>
I will add signoff from Jaeho cho.
>> ---
>> drivers/pci/controller/dwc/Kconfig | 31 ++
>> drivers/pci/controller/dwc/Makefile | 1 +
>> drivers/pci/controller/dwc/pcie-artpec8.c | 864 ++++++++++++++++++++++++++++++
>> 3 files changed, 896 insertions(+)
>> create mode 100644 drivers/pci/controller/dwc/pcie-artpec8.c
>>
>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
>> index 62ce3ab..4aa6da8 100644
>> --- a/drivers/pci/controller/dwc/Kconfig
>> +++ b/drivers/pci/controller/dwc/Kconfig
>> @@ -222,6 +222,37 @@ config PCIE_ARTPEC6_EP
>> Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>> endpoint mode. This uses the DesignWare core.
>>
>> +config PCIE_ARTPEC8
>> + bool "Axis ARTPEC-8 PCIe controller"
>> +
>> +config PCIE_ARTPEC8_HOST
>> + bool "Axis ARTPEC-8 PCIe controller Host Mode"
>> + depends on ARCH_ARTPEC
>> + depends on PCI_MSI_IRQ_DOMAIN
>> + depends on PCI_ENDPOINT
>> + select PCI_EPF_TEST
>> + select PCIE_DW_HOST
>> + select PCIE_ARTPEC8
>> + help
>> + Say 'Y' here to enable support for the PCIe controller in the
>> + ARTPEC-8 SoC to work in host mode.
>> + This PCIe controller is based on DesignWare Hardware core.
>> + And uses DesignWare core functions to implement the driver.
>
> Add blank line between paragraphs or rewrap as a single paragraph.
>
> s/Hardware/hardware/
>
> The last two sentences should be combined since the latter has no
> verb:
>
> This PCIe controller is based on the DesignWare hardware core and
> uses DesignWare core functions to implement the driver.
>
I will fix it.
>> +config PCIE_ARTPEC8_EP
>> + bool "Axis ARTPEC-8 PCIe controller Endpoint Mode"
>> + depends on ARCH_ARTPEC
>> + depends on PCI_ENDPOINT
>> + depends on PCI_ENDPOINT_CONFIGFS
>> + select PCI_EPF_TEST
>> + select PCIE_DW_EP
>> + select PCIE_ARTPEC8
>> + help
>> + Say 'Y' here to enable support for the PCIe controller in the
>> + ARTPEC-8 SoC to work in endpoint mode.
>> + This PCIe controller is based on DesignWare Hardware core.
>> + And uses DesignWare core functions to implement the driver.
>
> Same.
>
I will fix it.
>> config PCIE_ROCKCHIP_DW_HOST
>> bool "Rockchip DesignWare PCIe controller"
>> select PCIE_DW
>> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
>> index 8ba7b67..b361022 100644
>> --- a/drivers/pci/controller/dwc/Makefile
>> +++ b/drivers/pci/controller/dwc/Makefile
>> @@ -25,6 +25,7 @@ obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
>> obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>> obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
>> obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
>> +obj-$(CONFIG_PCIE_ARTPEC8) += pcie-artpec8.o
>>
>> # The following drivers are for devices that use the generic ACPI
>> # pci_root.c driver but don't support standard ECAM config access.
>> diff --git a/drivers/pci/controller/dwc/pcie-artpec8.c b/drivers/pci/controller/dwc/pcie-artpec8.c
>> new file mode 100644
>> index 0000000..d9ae9bf
>> --- /dev/null
>> +++ b/drivers/pci/controller/dwc/pcie-artpec8.c
>> @@ -0,0 +1,864 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * PCIe controller driver for Axis ARTPEC-8 SoC
>> + *
>> + * Copyright (C) 2019 Samsung Electronics Co., Ltd.
>> + * http://www.samsung.com
>> + *
>> + * Author: Jaeho Cho <[email protected]>
>> + * This file is based on driver/pci/controller/dwc/pci-exynos.c
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/resource.h>
>> +#include <linux/types.h>
>> +#include <linux/phy/phy.h>
>> +
>> +#include "pcie-designware.h"
>> +
>> +#define to_artpec8_pcie(x) dev_get_drvdata((x)->dev)
>> +
>> +/* Gen3 Control Register */
>> +#define PCIE_GEN3_RELATED_OFF 0x890
>> +/* Disables equilzation feature */
>
> s/equilzation/equalization/
>
> Comment is probably unnecessary, since the name is so descriptive.
>
I will remove this comment.
>> +#define PCIE_GEN3_EQUALIZATION_DISABLE (0x1 << 16)
>> +#define PCIE_GEN3_EQ_PHASE_2_3 (0x1 << 9)
>> +#define PCIE_GEN3_RXEQ_PH01_EN (0x1 << 12)
>> +#define PCIE_GEN3_RXEQ_RGRDLESS_RXTS (0x1 << 13)
>> +
>> +#define FAST_LINK_MODE (7)
>> +
>> +/* PCIe ELBI registers */
>> +#define PCIE_IRQ0_STS 0x000
>> +#define PCIE_IRQ1_STS 0x004
>> +#define PCIE_IRQ2_STS 0x008
>> +#define PCIE_IRQ5_STS 0x00C
>> +#define PCIE_IRQ0_EN 0x010
>> +#define PCIE_IRQ1_EN 0x014
>> +#define PCIE_IRQ2_EN 0x018
>> +#define PCIE_IRQ5_EN 0x01C
>> +#define IRQ_MSI_ENABLE BIT(20)
>> +#define PCIE_APP_LTSSM_ENABLE 0x054
>> +#define PCIE_ELBI_LTSSM_ENABLE 0x1
>> +#define PCIE_ELBI_CXPL_DEBUG_00_31 0x2C8
>> +#define PCIE_ELBI_CXPL_DEBUG_32_63 0x2CC
>> +#define PCIE_ELBI_SMLH_LINK_UP BIT(4)
>> +#define PCIE_ARTPEC8_DEVICE_TYPE 0x080
>> +#define DEVICE_TYPE_EP 0x0
>> +#define DEVICE_TYPE_LEG_EP 0x1
>> +#define DEVICE_TYPE_RC 0x4
>> +#define PCIE_ELBI_SLV_AWMISC 0x828
>> +#define PCIE_ELBI_SLV_ARMISC 0x820
>> +#define PCIE_ELBI_SLV_DBI_ENABLE BIT(21)
>> +#define LTSSM_STATE_MASK 0x3f
>
> The previous hex constants used upper-case; this uses lower-case.
> Pick one and stick with it.
>
I will modify the with upper-case.
> This seems like a mix of register offsets and definitions of bits
> within registers. It's confusing to mentally sort these out. Is
> there any way to make this more obvious? Some drivers, e.g.,
> pcie-artpec6.c, use "BIT(x)" and "GENMASK(x,y)" for bits within
> registers.
>
I will clearly devide offset, definitions of bits and redefined.
>> +#define LTSSM_STATE_L0 0x11
>> +
>> +/* FSYS SYSREG Offsets */
>
> The list below seems to inclue more than just register offsets.
>
Is it clear to change to "FSYS blue logic system registers"
like Jasper Nilsson`s comment?
https://lore.kernel.org/all/[email protected]/
My opinion is the same.
>> +#define FSYS_PCIE_CON 0x424
>> +#define PCIE_PERSTN BIT(5)
>> +#define FSYS_PCIE_DBI_ADDR_CON 0x428
>> +#define FSYS_PCIE_DBI_ADDR_OVR_CDM 0x00
>> +#define FSYS_PCIE_DBI_ADDR_OVR_SHADOW 0x12
>> +#define FSYS_PCIE_DBI_ADDR_OVR_ATU 0x36
>> +
>> +/* PMU SYSCON Offsets */
>> +#define PMU_SYSCON_PCIE_ISOLATION 0x3200
>> +
>> +/* BUS P/S SYSCON Offsets */
>> +#define BUS_SYSCON_BUS_PATH_ENABLE 0x0
>> +
>> +int artpec8_pcie_dbi_addr_con[] = {
>> + FSYS_PCIE_DBI_ADDR_CON
>> +};
>> +
>> +struct artpec8_pcie {
>> + struct dw_pcie *pci;
>> + struct clk *pipe_clk;
>> + struct clk *dbi_clk;
>> + struct clk *mstr_clk;
>> + struct clk *slv_clk;
>> + const struct artpec8_pcie_pdata *pdata;
>> + void __iomem *elbi_base;
>> + struct regmap *sysreg;
>> + struct regmap *pmu_syscon;
>> + struct regmap *bus_s_syscon;
>> + struct regmap *bus_p_syscon;
>> + enum dw_pcie_device_mode mode;
>> + int link_id;
>> + /* For Generic PHY Framework */
>
> Superfluous comment.
>
I will remove this comment.
>> + struct phy *phy;
>> +};
>> +
>> +struct artpec8_pcie_res_ops {
>> + int (*get_mem_resources)(struct platform_device *pdev,
>> + struct artpec8_pcie *artpec8_ctrl);
>> + int (*get_clk_resources)(struct platform_device *pdev,
>> + struct artpec8_pcie *artpec8_ctrl);
>> + int (*init_clk_resources)(struct artpec8_pcie *artpec8_ctrl);
>> + void (*deinit_clk_resources)(struct artpec8_pcie *artpec8_ctrl);
>> +};
>> +
>> +struct artpec8_pcie_pdata {
>> + const struct dw_pcie_ops *dwc_ops;
>> + const struct dw_pcie_host_ops *host_ops;
>
> Fix indentation to match surrounding code.
>
I will fix it.
>> + const struct artpec8_pcie_res_ops *res_ops;
>> + enum dw_pcie_device_mode mode;
>> +};
>> +
>> +enum artpec8_pcie_isolation {
>> + PCIE_CLEAR_ISOLATION = 0,
>> + PCIE_SET_ISOLATION = 1
>> +};
>> +
>> +enum artpec8_pcie_reg_bit {
>> + PCIE_REG_BIT_LOW = 0,
>> + PCIE_REG_BIT_HIGH = 1
>> +};
>> +
>> +static void artpec8_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
>> + u32 reg, size_t size, u32 val);
>> +static u32 artpec8_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
>> + u32 reg, size_t size);
>> +static void artpec8_pcie_writel(void __iomem *base, u32 val, u32 reg);
>
> Can you reorder the function definitions to avoid the need for these
> forward declarations?
>
I will reorder these function to avoid the need for forward declarations.
>> +static int artpec8_pcie_get_subsystem_resources(struct platform_device *pdev,
>> + struct artpec8_pcie *artpec8_ctrl)
>> +{
>> + struct device *dev = &pdev->dev;
>> +
>> + /* External Local Bus interface(ELBI) Register */
>> + artpec8_ctrl->elbi_base = devm_platform_ioremap_resource_byname(pdev, "elbi");
>
> Rewrap to fit in 80 columns.
>
I will fix it.
>> + if (IS_ERR(artpec8_ctrl->elbi_base)) {
>> + dev_err(dev, "failed to map elbi_base\n");
>> + return PTR_ERR(artpec8_ctrl->elbi_base);
>> + }
>> +
>> + /* fsys sysreg regmap handle */
>
> All these comments are superfluous since they only repeat the lookup
> arguments.
>
I will remove these comment.
>> + artpec8_ctrl->sysreg =
>> + syscon_regmap_lookup_by_phandle(dev->of_node,
>
> The above two lines should fit on one line.
>
I will fit on one line.
>> + "samsung,fsys-sysreg");
>> + if (IS_ERR(artpec8_ctrl->sysreg)) {
>> + dev_err(dev, "fsys sysreg regmap lookup failed.\n");
>> + return PTR_ERR(artpec8_ctrl->sysreg);
>> + }
>> +
>> + /* pmu syscon regmap handle */
>> + artpec8_ctrl->pmu_syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
>> + "samsung,syscon-phandle");
>> + if (IS_ERR(artpec8_ctrl->pmu_syscon)) {
>> + dev_err(dev, "pmu syscon regmap lookup failed.\n");
>> + return PTR_ERR(artpec8_ctrl->pmu_syscon);
>> + }
>> +
>> + /* bus s syscon regmap handle */
>> + artpec8_ctrl->bus_s_syscon =
>> + syscon_regmap_lookup_by_phandle(dev->of_node,
>> + "samsung,syscon-bus-s-fsys");
>> + if (IS_ERR(artpec8_ctrl->bus_s_syscon)) {
>> + dev_err(dev, "bus_s_syscon regmap lookup failed.\n");
>> + return PTR_ERR(artpec8_ctrl->bus_s_syscon);
>> + }
>> +
>> + /* bus p syscon regmap handle */
>> + artpec8_ctrl->bus_p_syscon =
>> + syscon_regmap_lookup_by_phandle(dev->of_node,
>> + "samsung,syscon-bus-p-fsys");
>> + if (IS_ERR(artpec8_ctrl->bus_p_syscon)) {
>> + dev_err(dev, "bus_p_syscon regmap lookup failed.\n");
>> + return PTR_ERR(artpec8_ctrl->bus_p_syscon);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int artpec8_pcie_get_rc_mem_resources(struct platform_device *pdev,
>> + struct artpec8_pcie *artpec8_ctrl)
>> +{
>> + struct dw_pcie *pci = artpec8_ctrl->pci;
>> +
>> + /* Data Bus Interface(DBI) Register */
>> + pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
>> + if (IS_ERR(pci->dbi_base))
>> + return PTR_ERR(pci->dbi_base);
>> +
>> + return 0;
>> +}
>> +
>> +static int artpec8_pcie_get_ep_mem_resources(struct platform_device *pdev,
>> + struct artpec8_pcie *artpec8_ctrl)
>> +{
>> + struct dw_pcie_ep *ep;
>> + struct dw_pcie *pci = artpec8_ctrl->pci;
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> +
>> + ep = &pci->ep;
>
> Reorder the locals above:
>
> struct dw_pcie *pci = artpec8_ctrl->pci;
> struct device *dev = &pdev->dev;
> struct dw_pcie_ep *ep = &pci->ep;
> struct resource *res;
>
> Then they're in the order you use them and you don't need the extra
> "ep = &pci->ep".
>
I will fix it. thank you for your detalied review.
>> + pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
>> + if (IS_ERR(pci->dbi_base)) {
>> + dev_err(dev, "failed to map ep_dbics\n");
>> + return -ENOMEM;
>> + }
>> +
>> + pci->dbi_base2 = devm_platform_ioremap_resource_byname(pdev, "dbi2");
>> + if (IS_ERR(pci->dbi_base2)) {
>> + dev_err(dev, "failed to map ep_dbics2\n");
>> + return -ENOMEM;
>> + }
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
>> + if (!res)
>> + return -EINVAL;
>> + ep->phys_base = res->start;
>> + ep->addr_size = resource_size(res);
>> +
>> + return 0;
>> +}
>> +
>> +static int artpec8_pcie_get_clk_resources(struct platform_device *pdev,
>> + struct artpec8_pcie *artpec8_ctrl)
>> +{
>> + struct device *dev = &pdev->dev;
>> +
>> + artpec8_ctrl->pipe_clk = devm_clk_get(dev, "pipe_clk");
>> + if (IS_ERR(artpec8_ctrl->pipe_clk)) {
>> + dev_err(dev, "couldn't get pipe clock\n");
>> + return -EINVAL;
>> + }
>> +
>> + artpec8_ctrl->dbi_clk = devm_clk_get(dev, "dbi_clk");
>> + if (IS_ERR(artpec8_ctrl->dbi_clk)) {
>> + dev_info(dev, "couldn't get dbi clk\n");
>> + return -EINVAL;
>> + }
>> +
>> + artpec8_ctrl->slv_clk = devm_clk_get(dev, "slv_clk");
>> + if (IS_ERR(artpec8_ctrl->slv_clk)) {
>> + dev_err(dev, "couldn't get slave clock\n");
>> + return -EINVAL;
>> + }
>> +
>> + artpec8_ctrl->mstr_clk = devm_clk_get(dev, "mstr_clk");
>> + if (IS_ERR(artpec8_ctrl->mstr_clk)) {
>> + dev_info(dev, "couldn't get master clk\n");
>
> It'd be nice if the err/info messages matched the exact DT name:
> "pipe_clk", "dbi_clk", slv_clk", etc.
>
I will fix it.
> Why are some of the above dev_err() and others dev_info() when you
> return -EINVAL in all cases?
>
When property is not found, it just to return error.
I will modify to return PTR_ERR.
>> + return -EINVAL;
>> + }
>> +
> + return 0;
>> +}
>> +
>> +static int artpec8_pcie_init_clk_resources(struct artpec8_pcie *artpec8_ctrl)
>> +{
>> + clk_prepare_enable(artpec8_ctrl->pipe_clk);
>> + clk_prepare_enable(artpec8_ctrl->dbi_clk);
>> + clk_prepare_enable(artpec8_ctrl->mstr_clk);
>> + clk_prepare_enable(artpec8_ctrl->slv_clk);
>> +
>> + return 0;
>> +}
>> +
>> +static void artpec8_pcie_deinit_clk_resources(struct artpec8_pcie *artpec8_ctrl)
>> +{
>> + clk_disable_unprepare(artpec8_ctrl->slv_clk);
>> + clk_disable_unprepare(artpec8_ctrl->mstr_clk);
>> + clk_disable_unprepare(artpec8_ctrl->dbi_clk);
>> + clk_disable_unprepare(artpec8_ctrl->pipe_clk);
>> +}
>> +
>> +static const struct artpec8_pcie_res_ops artpec8_pcie_rc_res_ops = {
>> + .get_mem_resources = artpec8_pcie_get_rc_mem_resources,
>> + .get_clk_resources = artpec8_pcie_get_clk_resources,
>> + .init_clk_resources = artpec8_pcie_init_clk_resources,
>> + .deinit_clk_resources = artpec8_pcie_deinit_clk_resources,
>> +};
>> +
>> +static const struct artpec8_pcie_res_ops artpec8_pcie_ep_res_ops = {
>> + .get_mem_resources = artpec8_pcie_get_ep_mem_resources,
>> + .get_clk_resources = artpec8_pcie_get_clk_resources,
>> + .init_clk_resources = artpec8_pcie_init_clk_resources,
>> + .deinit_clk_resources = artpec8_pcie_deinit_clk_resources,
>> +};
>> +
>> +static void artpec8_pcie_writel(void __iomem *base, u32 val, u32 reg)
>> +{
>> + writel(val, base + reg);
>> +}
>> +
>> +static u32 artpec8_pcie_readl(void __iomem *base, u32 reg)
>> +{
>> + return readl(base + reg);
>> +}
>> +
>> +static int artpec8_pcie_config_phy_power_isolation(struct dw_pcie *pci,
>> + enum artpec8_pcie_reg_bit val)
>> +{
>> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> + int ret;
>> +
>> + ret = regmap_write(artpec8_ctrl->pmu_syscon, PMU_SYSCON_PCIE_ISOLATION,
>> + val);
>> +
>> + return ret;
>
> return regmap_write(artpec8_ctrl->pmu_syscon, ...);
>
I will fix it.
>> +}
>> +
>> +static int artpec8_pcie_config_bus_enable(struct dw_pcie *pci,
>> + enum artpec8_pcie_reg_bit val)
>> +{
>> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> + int ret;
>> +
>> + ret = regmap_write(artpec8_ctrl->bus_p_syscon,
>> + BUS_SYSCON_BUS_PATH_ENABLE, val);
>> + if (ret)
>> + return ret;
>> +
>> + ret = regmap_write(artpec8_ctrl->bus_s_syscon,
>> + BUS_SYSCON_BUS_PATH_ENABLE, val);
>> + if (ret)
>> + return ret;
>> +
>> + return ret;
>
> return regmap_write(artpec8_ctrl->bus_s_syscon, ...);
>
I will fix it.
>> +}
>> +
>> +static int artpec8_pcie_config_isolation(struct dw_pcie *pci,
>> + enum artpec8_pcie_isolation val)
>> +{
>> + int ret;
>> + /* reg_val[0] : for phy power isolation */
>> + /* reg_val[1] : for bus enable */
>> + enum artpec8_pcie_reg_bit reg_val[2];
>> +
>> + switch (val) {
>> + case PCIE_CLEAR_ISOLATION:
>> + reg_val[0] = PCIE_REG_BIT_LOW;
>> + reg_val[1] = PCIE_REG_BIT_HIGH;
>> + break;
>> + case PCIE_SET_ISOLATION:
>> + reg_val[0] = PCIE_REG_BIT_HIGH;
>> + reg_val[1] = PCIE_REG_BIT_LOW;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + ret = artpec8_pcie_config_phy_power_isolation(pci, reg_val[0]);
>> + if (ret)
>> + return ret;
>> +
>> + ret = artpec8_pcie_config_bus_enable(pci, reg_val[1]);
>> + if (ret)
>> + return ret;
>> +
>> + return ret;
>
> return artpec8_pcie_config_bus_enable(pci, ...);
>
I will fix it.
>> +}
>> +
>> +static int artpec8_pcie_config_perstn(struct dw_pcie *pci,
>> + enum artpec8_pcie_reg_bit val)
>> +{
>> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> + unsigned int bits;
>> + int ret;
>> +
>> + if (val == PCIE_REG_BIT_HIGH)
>> + bits = PCIE_PERSTN;
>> + else
>> + bits = 0;
>> +
>> + ret = regmap_update_bits(artpec8_ctrl->sysreg, FSYS_PCIE_CON,
>> + PCIE_PERSTN, bits);
>> +
>> + return ret;
>
> return regmap_update_bits(artpec8_ctrl->sysreg, ...):
>
I will fix it.
>> +}
>> +
>> +static void artpec8_pcie_stop_link(struct dw_pcie *pci)
>> +{
>> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> + u32 val;
>> +
>> + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
>> + PCIE_APP_LTSSM_ENABLE);
>> +
>> + val &= ~PCIE_ELBI_LTSSM_ENABLE;
>> + artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
>> + PCIE_APP_LTSSM_ENABLE);
>> +}
>> +
>> +static int artpec8_pcie_start_link(struct dw_pcie *pci)
>> +{
>> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> + u32 val;
>> +
>> + dw_pcie_dbi_ro_wr_en(pci);
>> +
>> + /* Equalization disable */
>> + val = artpec8_pcie_read_dbi(pci, pci->dbi_base, PCIE_GEN3_RELATED_OFF,
>> + 4);
>> + artpec8_pcie_write_dbi(pci, pci->dbi_base, PCIE_GEN3_RELATED_OFF, 4,
>> + val | PCIE_GEN3_EQUALIZATION_DISABLE);
>> +
>> + dw_pcie_dbi_ro_wr_dis(pci);
>> +
>> + /* assert LTSSM enable */
>> + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
>> + PCIE_APP_LTSSM_ENABLE);
>> +
>> + val |= PCIE_ELBI_LTSSM_ENABLE;
>> + artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
>> + PCIE_APP_LTSSM_ENABLE);
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t artpec8_pcie_msi_irq_handler(int irq, void *arg)
>> +{
>> + struct artpec8_pcie *artpec8_ctrl = arg;
>> + struct dw_pcie *pci = artpec8_ctrl->pci;
>> + struct pcie_port *pp = &pci->pp;
>> + u32 val;
>> +
>> + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base, PCIE_IRQ2_STS);
>> +
>> + if ((val & IRQ_MSI_ENABLE) == IRQ_MSI_ENABLE) {
>> + val &= IRQ_MSI_ENABLE;
>> + artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
>> + PCIE_IRQ2_STS);
>> + dw_handle_msi_irq(pp);
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static void artpec8_pcie_msi_init(struct artpec8_pcie *artpec8_ctrl)
>> +{
>> + u32 val;
>> +
>> + /* enable MSI interrupt */
>> + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base, PCIE_IRQ2_EN);
>> + val |= IRQ_MSI_ENABLE;
>> + artpec8_pcie_writel(artpec8_ctrl->elbi_base, val, PCIE_IRQ2_EN);
>> +}
>> +
>> +static void artpec8_pcie_enable_interrupts(struct artpec8_pcie *artpec8_ctrl)
>> +{
>> + if (IS_ENABLED(CONFIG_PCI_MSI))
>> + artpec8_pcie_msi_init(artpec8_ctrl);
>> +}
>> +
>> +static u32 artpec8_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
>> + u32 reg, size_t size)
>> +{
>> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> + u32 val;
>> + bool is_atu = false;
>> +
>> + if (base == pci->atu_base) {
>> + is_atu = true;
>> + base = pci->dbi_base;
>> + regmap_write(artpec8_ctrl->sysreg,
>> + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
>> + FSYS_PCIE_DBI_ADDR_OVR_ATU);
>> + }
>> +
>> + dw_pcie_read(base + reg, size, &val);
>> +
>> + if (is_atu)
>> + regmap_write(artpec8_ctrl->sysreg,
>> + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
>> + FSYS_PCIE_DBI_ADDR_OVR_CDM);
>> +
>> + return val;
>> +}
>> +
>> +static void artpec8_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
>> + u32 reg, size_t size, u32 val)
>> +{
>> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> + bool is_atu = false;
>> +
>> + if (base == pci->atu_base) {
>> + is_atu = true;
>> + base = pci->dbi_base;
>> + regmap_write(artpec8_ctrl->sysreg,
>> + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
>> + FSYS_PCIE_DBI_ADDR_OVR_ATU);
>> + }
>> +
>> + dw_pcie_write(base + reg, size, val);
>> +
>> + if (is_atu)
>> + regmap_write(artpec8_ctrl->sysreg,
>> + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
>> + FSYS_PCIE_DBI_ADDR_OVR_CDM);
>> +}
>> +
>> +static void artpec8_pcie_write_dbi2(struct dw_pcie *pci, void __iomem *base,
>> + u32 reg, size_t size, u32 val)
>> +{
>> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> +
>> + regmap_write(artpec8_ctrl->sysreg,
>> + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
>> + FSYS_PCIE_DBI_ADDR_OVR_SHADOW);
>> +
>> + dw_pcie_write(base + reg, size, val);
>> +
>> + regmap_write(artpec8_ctrl->sysreg,
>> + artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
>> + FSYS_PCIE_DBI_ADDR_OVR_CDM);
>> +}
>> +
>> +static int artpec8_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)) {
>> + *val = ~0;
>
> PCI_SET_ERROR_RESPONSE(val);
>
I will fix it.
>> + return PCIBIOS_DEVICE_NOT_FOUND;
>> + }
>>> +
>> + *val = dw_pcie_read_dbi(pci, where, size);
>> + return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>> +static int artpec8_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 artpec8_pci_ops = {
>> + .read = artpec8_pcie_rd_own_conf,
>> + .write = artpec8_pcie_wr_own_conf,
>> +};
>> +
>> +static int artpec8_pcie_link_up(struct dw_pcie *pci)
>> +{
>> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> + u32 val;
>> +
>> + val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
>> + PCIE_ELBI_CXPL_DEBUG_00_31);
>> +
>> + return (val & LTSSM_STATE_MASK) == LTSSM_STATE_L0;
>> +}
>> +
>> +static int artpec8_pcie_host_init(struct pcie_port *pp)
>> +{
>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> + struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> +
>> + pp->bridge->ops = &artpec8_pci_ops;
>> +
>> + dw_pcie_writel_dbi(pci, PCIE_GEN3_RELATED_OFF,
>> + (PCIE_GEN3_EQ_PHASE_2_3 |
>> + PCIE_GEN3_RXEQ_PH01_EN |
>> + PCIE_GEN3_RXEQ_RGRDLESS_RXTS));
>> +
>> + artpec8_pcie_enable_interrupts(artpec8_ctrl);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct dw_pcie_host_ops artpec8_pcie_host_ops = {
>> + .host_init = artpec8_pcie_host_init,
>> +};
>> +
>> +static u8 artpec8_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
>> +{
>> + u32 val;
>> +
>> + val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
>> + pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>> +
>> + if (val == 0xffffffff)
>> + return 1;
>> +
>> + return 0;
>> +}
>> +
>> +static void artpec8_pcie_ep_init(struct dw_pcie_ep *ep)
>> +{
>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> + enum pci_barno bar;
>
> Add blank line here and use usual multi-line comment style:
>
> /*
> * Currently PCIe EP core is not ...
>
I will fix it.
>> + /* Currently PCIe EP core is not setting iatu_unroll_enabled
>> + * so let's handle it here. We need to find proper place to
>> + * initialize this so that it can be used as for other EP
>
> ... can be used for ...
>
Thank you for detailed review.
>> + * controllers as well.
>> + */
>> + pci->iatu_unroll_enabled = artpec8_pcie_iatu_unroll_enabled(pci);
>> +
>> + for (bar = BAR_0; bar <= BAR_5; bar++)
>> + dw_pcie_ep_reset_bar(pci, bar);
>> +}
>> +
>> +static int artpec8_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> + enum pci_epc_irq_type type, u16 interrupt_num)
>> +{
>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +
>> + switch (type) {
>> + case PCI_EPC_IRQ_LEGACY:
>> + return -EINVAL;
>> + case PCI_EPC_IRQ_MSI:
>> + return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
>> + default:
>> + dev_err(pci->dev, "UNKNOWN IRQ type\n");
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct pci_epc_features artpec8_pcie_epc_features = {
>> + .linkup_notifier = false,
>> + .msi_capable = true,
>> + .msix_capable = false,
>> +};
>> +
>> +static const struct pci_epc_features*
>> +artpec8_pcie_ep_get_features(struct dw_pcie_ep *ep)
>> +{
>> + return &artpec8_pcie_epc_features;
>> +}
>> +
>> +static const struct dw_pcie_ep_ops artpec8_dw_pcie_ep_ops = {
>> + .ep_init = artpec8_pcie_ep_init,
>> + .raise_irq = artpec8_pcie_raise_irq,
>> + .get_features = artpec8_pcie_ep_get_features,
>> +};
>> +
>> +static int __init artpec8_add_pcie_ep(struct artpec8_pcie *artpec8_ctrl,
>> + struct platform_device *pdev)
>> +{
>> + int ret;
>> + struct dw_pcie_ep *ep;
>> + struct dw_pcie *pci = artpec8_ctrl->pci;
>> +
>> + ep = &pci->ep;
>
> Reorder locals and initialize ep as above.
>
I will fix it.
>> + ep->ops = &artpec8_dw_pcie_ep_ops;
>> +
>> + dw_pcie_writel_dbi(pci, PCIE_GEN3_RELATED_OFF,
>> + (PCIE_GEN3_EQ_PHASE_2_3 |
>> + PCIE_GEN3_RXEQ_PH01_EN |
>> + PCIE_GEN3_RXEQ_RGRDLESS_RXTS));
>> +
>> + ret = dw_pcie_ep_init(ep);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int __init artpec8_add_pcie_port(struct artpec8_pcie *artpec8_ctrl,
>> + struct platform_device *pdev)
>> +{
>> + struct dw_pcie *pci = artpec8_ctrl->pci;
>> + struct pcie_port *pp = &pci->pp;
>> + struct device *dev = &pdev->dev;
>> + int ret;
>> + int irq_flags;
>> + int irq;
>
> Reorder to be in order of use:
>
> struct dw_pcie *pci = artpec8_ctrl->pci;
> struct pcie_port *pp = &pci->pp;
> struct device *dev = &pdev->dev;
> int irq;
> int irq_flags;
> int ret;
>
I will fix it.
>> +
>> + if (IS_ENABLED(CONFIG_PCI_MSI)) {
>> + irq = platform_get_irq_byname(pdev, "intr");
>> + if (!irq)
>> + return -ENODEV;
>> +
>> + irq_flags = IRQF_SHARED | IRQF_NO_THREAD;
>> +
>> + ret = devm_request_irq(dev, irq, artpec8_pcie_msi_irq_handler,
>> + irq_flags, "artpec8-pcie", artpec8_ctrl);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + /* Prevent core for messing with the IRQ, since it's muxed */
>
> Prevent core from ...
>
I will fix it.
>> + pp->msi_irq = -ENODEV;
>> +
>> + ret = dw_pcie_host_init(pp);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>
> return dw_pcie_host_init(pp);
>
I will fix it.
>> +}
>> +
>> +static const struct dw_pcie_ops artpec8_dw_pcie_ops = {
>> + .read_dbi = artpec8_pcie_read_dbi,
>> + .write_dbi = artpec8_pcie_write_dbi,
>> + .write_dbi2 = artpec8_pcie_write_dbi2,
>> + .start_link = artpec8_pcie_start_link,
>> + .stop_link = artpec8_pcie_stop_link,
>> + .link_up = artpec8_pcie_link_up,
>> +};
>> +
>> +static int artpec8_pcie_probe(struct platform_device *pdev)
>> +{
>> + int ret;
>> + struct dw_pcie *pci;
>> + struct pcie_port *pp;
>> + struct artpec8_pcie *artpec8_ctrl;
>> + enum dw_pcie_device_mode mode;
>> + struct device *dev = &pdev->dev;
>> + const struct artpec8_pcie_pdata *pdata;
>> + struct device_node *np = dev->of_node;
>
> Reorder in order of use.
>
I will reorder in order of use like below.
struct device *dev = &pdev->dev;
struct artpec8_pcie *artpec8_ctrl;
struct dw_pcie *pci;
const struct artpec8_pcie_pdata *pdata;
enum dw_pcie_device_mode mode;
struct pcie_port *pp;
struct device_node *np = dev->of_node;
int ret;
>> + artpec8_ctrl = devm_kzalloc(dev, sizeof(*artpec8_ctrl), GFP_KERNEL);
>> + if (!artpec8_ctrl)
>> + return -ENOMEM;
>> +
>> + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
>> + if (!pci)
>> + return -ENOMEM;
>> +
>> + pdata = (const struct artpec8_pcie_pdata *)
>
> Unnecessary cast.
>
I will remove cast.
>> + of_device_get_match_data(dev);
>> + if (!pdata)
>> + return -ENODEV;
>> +
>> + mode = (enum dw_pcie_device_mode)pdata->mode;
>> +
>> + artpec8_ctrl->pci = pci;
>> + artpec8_ctrl->pdata = pdata;
>> + artpec8_ctrl->mode = mode;
>> +
>> + pci->dev = dev;
>> + pci->ops = pdata->dwc_ops;
>> + pci->dbi_base2 = NULL;
>> + pci->dbi_base = NULL;
>> + pp = &pci->pp;
>> + pp->ops = artpec8_ctrl->pdata->host_ops;
>> +
>> + if (mode == DW_PCIE_RC_TYPE)
> + artpec8_ctrl->link_id = of_alias_get_id(np, "pcierc");
>> + else
>> + artpec8_ctrl->link_id = of_alias_get_id(np, "pcieep");
>> +
>> + ret = artpec8_pcie_get_subsystem_resources(pdev, artpec8_ctrl);
>> + if (ret)
>> + return ret;
>> +
>> + if (pdata->res_ops && pdata->res_ops->get_mem_resources) {
>> + ret = pdata->res_ops->get_mem_resources(pdev, artpec8_ctrl);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + if (pdata->res_ops && pdata->res_ops->get_clk_resources) {
>> + ret = pdata->res_ops->get_clk_resources(pdev, artpec8_ctrl);
>> + if (ret)
>> + return ret;
>> +
>> + ret = pdata->res_ops->init_clk_resources(artpec8_ctrl);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + platform_set_drvdata(pdev, artpec8_ctrl);
>> +
>> + ret = artpec8_pcie_config_isolation(pci, PCIE_CLEAR_ISOLATION);
>> + if (ret)
>> + return ret;
>> +
>> + ret = artpec8_pcie_config_perstn(pci, PCIE_REG_BIT_HIGH);
>> + if (ret)
>> + return ret;
>> +
>> + artpec8_ctrl->phy = devm_of_phy_get(dev, np, NULL);
>> + if (IS_ERR(artpec8_ctrl->phy))
>> + return PTR_ERR(artpec8_ctrl->phy);
>> +
>> + phy_init(artpec8_ctrl->phy);
>> + phy_reset(artpec8_ctrl->phy);
>> +
>> + switch (mode) {
>> + case DW_PCIE_RC_TYPE:
>> + artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_RC,
>> + PCIE_ARTPEC8_DEVICE_TYPE);
>> + ret = artpec8_add_pcie_port(artpec8_ctrl, pdev);
>> + if (ret < 0)
>
> Are there positive return values that indicate success? Most places
> above you assume "ret != 0" means failure, so just curious why you
> test "ret < 0" instead of just "ret".
>
There is no special reason, but it seems that the format used
in the existing dw driver is applied.
Thank you for kindness reivew.
Best regards,
Wangseok Lee
>> + goto fail_probe;
>> + break;
>> + case DW_PCIE_EP_TYPE:
>> + artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_EP,
>> + PCIE_ARTPEC8_DEVICE_TYPE);
>> +
>> + ret = artpec8_add_pcie_ep(artpec8_ctrl, pdev);
>> + if (ret < 0)
>
> Same question.
>
>> + goto fail_probe;
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + goto fail_probe;
>> + }
>> +
>> + return 0;
>> +
>> +fail_probe:
>> + phy_exit(artpec8_ctrl->phy);
>> + if (pdata->res_ops && pdata->res_ops->deinit_clk_resources)
>> + pdata->res_ops->deinit_clk_resources(artpec8_ctrl);
>> +
>> + return ret;
>> +}
>> +
>> +static int __exit artpec8_pcie_remove(struct platform_device *pdev)
>> +{
>> + struct artpec8_pcie *artpec8_ctrl = platform_get_drvdata(pdev);
>> + const struct artpec8_pcie_pdata *pdata = artpec8_ctrl->pdata;
>> +
>> + if (pdata->res_ops && pdata->res_ops->deinit_clk_resources)
>> + pdata->res_ops->deinit_clk_resources(artpec8_ctrl);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct artpec8_pcie_pdata artpec8_pcie_rc_pdata = {
>> + .dwc_ops = &artpec8_dw_pcie_ops,
>> + .host_ops = &artpec8_pcie_host_ops,
>> + .res_ops = &artpec8_pcie_rc_res_ops,
>> + .mode = DW_PCIE_RC_TYPE,
>> +};
>> +
>> +static const struct artpec8_pcie_pdata artpec8_pcie_ep_pdata = {
>> + .dwc_ops = &artpec8_dw_pcie_ops,
>> + .host_ops = &artpec8_pcie_host_ops,
>> + .res_ops = &artpec8_pcie_ep_res_ops,
>> + .mode = DW_PCIE_EP_TYPE,
>> +};
>> +
>> +static const struct of_device_id artpec8_pcie_of_match[] = {
>> + {
>> + .compatible = "axis,artpec8-pcie",
>> + .data = &artpec8_pcie_rc_pdata,
>> + },
>> + {
>> + .compatible = "axis,artpec8-pcie-ep",
>> + .data = &artpec8_pcie_ep_pdata,
>> + },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, artpec8_pcie_of_match);
>> +
>> +static struct platform_driver artpec8_pcie_driver = {
>> + .probe = artpec8_pcie_probe,
>> + .remove = __exit_p(artpec8_pcie_remove),
>> + .driver = {
>> + .name = "artpec8-pcie",
>> + .of_match_table = artpec8_pcie_of_match,
>> + },
>> +};
>> +
>> +module_platform_driver(artpec8_pcie_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Jaeho Cho <[email protected]>");
>> --
>> 2.9.5
>>
>> --
>> linux-phy mailing list
>> [email protected]
>> https://protect2.fireeye.com/v1/url?k=8d601177-ec1df90b-8d619a38-74fe485cc33c-571071ed0e3feda8&q=1&e=731ce886-130a-4ad1-bf82-3da4fec844b6&u=https%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-phy
On Fri, Jun 10, 2022 at 09:03:03AM +0900, Wangseok Lee wrote:
> On?06/04/2022?01:03,?Bjorn?Helgaas?wrote:
> >?In?the?subject,?why?do?you?tag?this?"axis"???There's?an?existing
> >?pcie-artpec6.c?that?uses?the?driver?name?""artpec6-pcie"?and?the
> >?subject?line?tag?"artpec6".
> >?
> >?This?adds?pcie-artpec8.c?with?driver?name?"artpec8-pcie",?so?the
> >?obvious?choice?would?be?"artpec8".
> >?
> >?I?assume?you?evaluated?the?possibility?of?extending?artpec6?to?support
> >?artpec8?in?addition?to?the?artpec6?and?artpec7?it?already?supports?
> ?
> "pcie-artpec6.?c"?supports?artpec6?and?artpec7?H/W.
> artpec8?can?not?be?expanded?because?H/W?configuration?is
> completely?different?from?artpec6/7.
> phy?and?sub?controller?are?different.
Thanks for this detail. Can you include this in the commit log next
time around in case anybody else has a similar question?
> >>?+/*?FSYS?SYSREG?Offsets?*/
> >?
> >?The?list?below?seems?to?inclue?more?than?just?register?offsets.
> >?
> ?
> Is?it?clear?to?change?to?"FSYS?blue?logic?system?registers"?
> like?Jasper?Nilsson`s?comment?
> https://lore.kernel.org/all/[email protected]/
> My?opinion?is?the?same.
Yep, that's fine. But spell it "glue logic", not "blue logic" :)
> >>?+static?int?artpec8_pcie_get_clk_resources(struct?platform_device?*pdev,
> >>?+???????????????????????????????????????struct?artpec8_pcie?*artpec8_ctrl)
> >>?+{
> >>?+????????struct?device?*dev?=?&pdev->dev;
> >>?+
> >>?+????????artpec8_ctrl->pipe_clk?=?devm_clk_get(dev,?"pipe_clk");
> >>?+????????if?(IS_ERR(artpec8_ctrl->pipe_clk))?{
> >>?+????????????????dev_err(dev,?"couldn't?get?pipe?clock\n");
> >>?+????????????????return?-EINVAL;
> >>?+????????}
> >>?+
> >>?+????????artpec8_ctrl->dbi_clk?=?devm_clk_get(dev,?"dbi_clk");
> >>?+????????if?(IS_ERR(artpec8_ctrl->dbi_clk))?{
> >>?+????????????????dev_info(dev,?"couldn't?get?dbi?clk\n");
> >>?+????????????????return?-EINVAL;
> >>?+????????}
> >>?+
> >>?+????????artpec8_ctrl->slv_clk?=?devm_clk_get(dev,?"slv_clk");
> >>?+????????if?(IS_ERR(artpec8_ctrl->slv_clk))?{
> >>?+????????????????dev_err(dev,?"couldn't?get?slave?clock\n");
> >>?+????????????????return?-EINVAL;
> >>?+????????}
> >>?+
> >>?+????????artpec8_ctrl->mstr_clk?=?devm_clk_get(dev,?"mstr_clk");
> >>?+????????if?(IS_ERR(artpec8_ctrl->mstr_clk))?{
> >>?+????????????????dev_info(dev,?"couldn't?get?master?clk\n");
> >?
> >?It'd?be?nice?if?the?err/info?messages?matched?the?exact?DT?name:
> >?"pipe_clk",?"dbi_clk",?slv_clk",?etc.
> >?
> ?
> I?will?fix?it.
> ?
> >?Why?are?some?of?the?above?dev_err()?and?others?dev_info()?when?you
> >?return?-EINVAL?in?all?cases?
> ?
> When?property?is?not?found,?it?just?to?return?error.
> I?will?modify?to?return?PTR_ERR.
Using PTR_ERR() looks like a good idea, since then you return the
actual error from devm_clk_get() instead of always returning -EINVAL.
But that wasn't my comment. My comment was that it looks like these
should be all dev_err() (or all dev_info()).
> >>?+????????switch?(mode)?{
> >>?+????????case?DW_PCIE_RC_TYPE:
> >>?+????????????????artpec8_pcie_writel(artpec8_ctrl->elbi_base,?DEVICE_TYPE_RC,
> >>?+????????????????????????????????PCIE_ARTPEC8_DEVICE_TYPE);
> >>?+????????????????ret?=?artpec8_add_pcie_port(artpec8_ctrl,?pdev);
> >>?+????????????????if?(ret?<?0)
> >?
> >?Are?there?positive?return?values?that?indicate?success???Most?places
> >?above?you?assume?"ret?!=?0"?means?failure,?so?just?curious?why?you
> >?test?"ret?<?0"?instead?of?just?"ret".
> ?
> There?is?no?special?reason,?but?it?seems?that?the?format?used?
> in?the?existing?dw?driver?is?applied.
Fair enough. "git grep -A2 add_pcie_port drivers/pci/controller/"
says all *_add_pcie_port() calls use the same pattern, so thanks for
following that.
Bjorn
On 06/11/2022 00:30, Bjorn Helgaas wrote:
> On Fri, Jun 10, 2022 at 09:03:03AM +0900, Wangseok Lee wrote:
>> On 06/04/2022 01:03, Bjorn Helgaas wrote:
>> > In the subject, why do you tag this "axis"? There's an existing
>> > pcie-artpec6.c that uses the driver name ""artpec6-pcie" and the
>> > subject line tag "artpec6".
>> >
>> > This adds pcie-artpec8.c with driver name "artpec8-pcie", so the
>> > obvious choice would be "artpec8".
>> >
>> > I assume you evaluated the possibility of extending artpec6 to support
>> > artpec8 in addition to the artpec6 and artpec7 it already supports?
>>
>> "pcie-artpec6. c" supports artpec6 and artpec7 H/W.
>> artpec8 can not be expanded because H/W configuration is
>> completely different from artpec6/7.
>> phy and sub controller are different.
>
> Thanks for this detail. Can you include this in the commit log next
> time around in case anybody else has a similar question?
>
Ok, sure.
>> >> +/* FSYS SYSREG Offsets */
>> >
>> > The list below seems to inclue more than just register offsets.
>> >
>>
>> Is it clear to change to "FSYS blue logic system registers"
>> like Jasper Nilsson`s comment?
>> https://lore.kernel.org/all/[email protected]/
>> My opinion is the same.
>
> Yep, that's fine. But spell it "glue logic", not "blue logic" :)
>
Thanks, it was just a typo.
>> >> +static int artpec8_pcie_get_clk_resources(struct platform_device *pdev,
>> >> + struct artpec8_pcie *artpec8_ctrl)
>> >> +{
>> >> + struct device *dev = &pdev->dev;
>> >> +
>> >> + artpec8_ctrl->pipe_clk = devm_clk_get(dev, "pipe_clk");
>> >> + if (IS_ERR(artpec8_ctrl->pipe_clk)) {
>> >> + dev_err(dev, "couldn't get pipe clock\n");
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >> + artpec8_ctrl->dbi_clk = devm_clk_get(dev, "dbi_clk");
>> >> + if (IS_ERR(artpec8_ctrl->dbi_clk)) {
>> >> + dev_info(dev, "couldn't get dbi clk\n");
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >> + artpec8_ctrl->slv_clk = devm_clk_get(dev, "slv_clk");
>> >> + if (IS_ERR(artpec8_ctrl->slv_clk)) {
>> >> + dev_err(dev, "couldn't get slave clock\n");
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >> + artpec8_ctrl->mstr_clk = devm_clk_get(dev, "mstr_clk");
>> >> + if (IS_ERR(artpec8_ctrl->mstr_clk)) {
>> >> + dev_info(dev, "couldn't get master clk\n");
>> >
>> > It'd be nice if the err/info messages matched the exact DT name:
>> > "pipe_clk", "dbi_clk", slv_clk", etc.
>> >
>>
>> I will fix it.
>>
>> > Why are some of the above dev_err() and others dev_info() when you
>> > return -EINVAL in all cases?
>>
>> When property is not found, it just to return error.
>> I will modify to return PTR_ERR.
>
>
> Using PTR_ERR() looks like a good idea, since then you return the
> actual error from devm_clk_get() instead of always returning -EINVAL.
>
> But that wasn't my comment. My comment was that it looks like these
> should be all dev_err() (or all dev_info()).
>
I understood your question. I think it was simply a way to generate log
msg.
In this case, is there a more proper print function that use to generate
log msg?
In addition, error return in artpec8_pcie_get_clk_resources() will be
modified to devm_clk_bulk_get().
(according to Krzysztof's review comment.. )
>> >> + switch (mode) {
>> >> + case DW_PCIE_RC_TYPE:
>> >> + artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_RC,
>> >> + PCIE_ARTPEC8_DEVICE_TYPE);
>> >> + ret = artpec8_add_pcie_port(artpec8_ctrl, pdev);
>> >> + if (ret < 0)
>> >
>> > Are there positive return values that indicate success? Most places
>> > above you assume "ret != 0" means failure, so just curious why you
>> > test "ret < 0" instead of just "ret".
>>
>> There is no special reason, but it seems that the format used
>> in the existing dw driver is applied.
>
> Fair enough. "git grep -A2 add_pcie_port drivers/pci/controller/"
> says all *_add_pcie_port() calls use the same pattern, so thanks for
> following that.
>
> Bjorn
Thank you for kindness reivew.
Best regards,
Wangseok Lee