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

[Wangseok Lee <wangseok.lee@samsung.com>]

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@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
> 
 
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@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.
> 
 
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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy