Thread (21 messages) 21 messages, 4 authors, 2022-06-13

Re: [PATCH v2 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver

From: Krzysztof Kozlowski <hidden>
Date: 2022-06-06 10:25:38
Also in: linux-devicetree, linux-pci, lkml

On 03/06/2022 04:34, Wangseok Lee wrote:
quoted hunk ↗ jump to hunk
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 <redacted>
---
 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

quoted hunk ↗ jump to hunk
+	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.
quoted hunk ↗ jump to hunk
 
 # 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 <jaeho79.cho@samsung.com>
+ * 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/ (local) ),
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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help