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 <wangseok.lee@xxxxxxxxxxx> >> --- >> 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 <jaeho79.cho@xxxxxxxxxxx> >> + * 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
