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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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@samsung.com>
> ---
>  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 <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/ ),
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