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-pci, linux-phy, 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.cdiff --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_DWdiff --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