Re: [PATCH v2 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver
From: Wangseok Lee
Date: Wed Jun 08 2022 - 02:54:41 EST
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