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

Wangseok Lee
2022-06-10T00:03:18Z

On 06/04/2022 01:03, Bjorn Helgaas wrote:
> In the subject, why do you tag this "axis"?  There's an existing
> pcie-artpec6.c that uses the driver name ""artpec6-pcie" and the
> subject line tag "artpec6".
>
> This adds pcie-artpec8.c with driver name "artpec8-pcie", so the
> obvious choice would be "artpec8".
>
> I assume you evaluated the possibility of extending artpec6 to support
> artpec8 in addition to the artpec6 and artpec7 it already supports?
>

"pcie-artpec6. c" supports artpec6 and artpec7 H/W.
artpec8 can not be expanded because H/W configuration is
completely different from artpec6/7.
phy and sub controller are different.

> On Fri, Jun 03, 2022 at 11:34:52AM +0900, 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.
>
> Add blank lines between paragraphs.
>
> Wrap lines to fill 75 columns.
>

I will fix it.

>> 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.
>
> Thanks for the "changes since v1" notes.  You can put them below the
> "---" since there's no need for them in the permanent git commit log:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v5.18#n675

i will fix it.

>> Signed-off-by: Wangseok Lee <wangseok.lee@samsung.com>
>
> Why is there no signoff from Jaeho Cho <jaeho79.cho@samsung.com>?
>

I will add signoff from Jaeho cho.

>> ---
>>  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
>> +        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.
>
> Add blank line between paragraphs or rewrap as a single paragraph.
>
> s/Hardware/hardware/
>
> The last two sentences should be combined since the latter has no
> verb:
>
>   This PCIe controller is based on the DesignWare hardware core and
>   uses DesignWare core functions to implement the driver.
>

I will fix it.

>> +config PCIE_ARTPEC8_EP
>> +        bool "Axis ARTPEC-8 PCIe controller Endpoint Mode"
>> +        depends on ARCH_ARTPEC
>> +        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.
>
> Same.
>

I will fix it.

>>  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
>>
>>  # 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 */
>
> s/equilzation/equalization/
>
> Comment is probably unnecessary, since the name is so descriptive.
>

I will remove this comment.

>> +#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
>
> The previous hex constants used upper-case; this uses lower-case.
> Pick one and stick with it.
>

I will modify the with upper-case.

> This seems like a mix of register offsets and definitions of bits
> within registers.  It's confusing to mentally sort these out.  Is
> there any way to make this more obvious?  Some drivers, e.g.,
> pcie-artpec6.c, use "BIT(x)" and "GENMASK(x,y)" for bits within
> registers.
>

I will clearly devide offset, definitions of bits and redefined.

>> +#define LTSSM_STATE_L0                        0x11
>> +
>> +/* FSYS SYSREG Offsets */
>
> The list below seems to inclue more than just register offsets.
>

Is it clear to change to "FSYS blue logic system registers"
like Jasper Nilsson`s comment?
https://lore.kernel.org/all/20220607070332.GY18902@axis.com/
My opinion is the same.

>> +#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[] = {
>> +        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;
>> +        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 */
>
> Superfluous comment.
>

I will remove this comment.

>> +        struct phy                        *phy;
>> +};
>> +
>> +struct artpec8_pcie_res_ops {
>> +        int (*get_mem_resources)(struct platform_device *pdev,
>> +                                 struct artpec8_pcie *artpec8_ctrl);
>> +        int (*get_clk_resources)(struct platform_device *pdev,
>> +                                 struct artpec8_pcie *artpec8_ctrl);
>> +        int (*init_clk_resources)(struct artpec8_pcie *artpec8_ctrl);
>> +        void (*deinit_clk_resources)(struct artpec8_pcie *artpec8_ctrl);
>> +};
>> +
>> +struct artpec8_pcie_pdata {
>> +        const struct dw_pcie_ops                *dwc_ops;
>> +        const struct dw_pcie_host_ops                        *host_ops;
>
> Fix indentation to match surrounding code.
>

I will fix it.

>> +        const struct artpec8_pcie_res_ops        *res_ops;
>> +        enum dw_pcie_device_mode                mode;
>> +};
>> +
>> +enum artpec8_pcie_isolation {
>> +        PCIE_CLEAR_ISOLATION = 0,
>> +        PCIE_SET_ISOLATION = 1
>> +};
>> +
>> +enum artpec8_pcie_reg_bit {
>> +        PCIE_REG_BIT_LOW = 0,
>> +        PCIE_REG_BIT_HIGH = 1
>> +};
>> +
>> +static void artpec8_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
>> +                                u32 reg, size_t size, u32 val);
>> +static u32 artpec8_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
>> +                                u32 reg, size_t size);
>> +static void artpec8_pcie_writel(void __iomem *base, u32 val, u32 reg);
>
> Can you reorder the function definitions to avoid the need for these
> forward declarations?
>

I will reorder these function to avoid the need for forward declarations.

>> +static int artpec8_pcie_get_subsystem_resources(struct platform_device *pdev,
>> +                                        struct artpec8_pcie *artpec8_ctrl)
>> +{
>> +        struct device *dev = &pdev->dev;
>> +
>> +        /* External Local Bus interface(ELBI) Register */
>> +        artpec8_ctrl->elbi_base = devm_platform_ioremap_resource_byname(pdev, "elbi");
>
> Rewrap to fit in 80 columns.
>

I will fix it.

>> +        if (IS_ERR(artpec8_ctrl->elbi_base)) {
>> +                dev_err(dev, "failed to map elbi_base\n");
>> +                return PTR_ERR(artpec8_ctrl->elbi_base);
>> +        }
>> +
>> +        /* fsys sysreg regmap handle */
>
> All these comments are superfluous since they only repeat the lookup
> arguments.
>

I will remove these comment.

>> +        artpec8_ctrl->sysreg =
>> +                syscon_regmap_lookup_by_phandle(dev->of_node,
>
> The above two lines should fit on one line.
>

I will fit on one line.

>> +                        "samsung,fsys-sysreg");
>> +        if (IS_ERR(artpec8_ctrl->sysreg)) {
>> +                dev_err(dev, "fsys sysreg regmap lookup failed.\n");
>> +                return PTR_ERR(artpec8_ctrl->sysreg);
>> +        }
>> +
>> +        /* pmu syscon regmap handle */
>> +        artpec8_ctrl->pmu_syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
>> +                        "samsung,syscon-phandle");
>> +        if (IS_ERR(artpec8_ctrl->pmu_syscon)) {
>> +                dev_err(dev, "pmu syscon regmap lookup failed.\n");
>> +                return PTR_ERR(artpec8_ctrl->pmu_syscon);
>> +        }
>> +
>> +        /* bus s syscon regmap handle */
>> +        artpec8_ctrl->bus_s_syscon =
>> +                syscon_regmap_lookup_by_phandle(dev->of_node,
>> +                        "samsung,syscon-bus-s-fsys");
>> +        if (IS_ERR(artpec8_ctrl->bus_s_syscon)) {
>> +                dev_err(dev, "bus_s_syscon regmap lookup failed.\n");
>> +                return PTR_ERR(artpec8_ctrl->bus_s_syscon);
>> +        }
>> +
>> +        /* bus p syscon regmap handle */
>> +        artpec8_ctrl->bus_p_syscon =
>> +                syscon_regmap_lookup_by_phandle(dev->of_node,
>> +                        "samsung,syscon-bus-p-fsys");
>> +        if (IS_ERR(artpec8_ctrl->bus_p_syscon)) {
>> +                dev_err(dev, "bus_p_syscon regmap lookup failed.\n");
>> +                return PTR_ERR(artpec8_ctrl->bus_p_syscon);
>> +        }
>> +
>> +        return 0;
>> +}
>> +
>> +static int artpec8_pcie_get_rc_mem_resources(struct platform_device *pdev,
>> +                                             struct artpec8_pcie *artpec8_ctrl)
>> +{
>> +        struct dw_pcie *pci = artpec8_ctrl->pci;
>> +
>> +        /* Data Bus Interface(DBI) Register */
>> +        pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
>> +        if (IS_ERR(pci->dbi_base))
>> +                return PTR_ERR(pci->dbi_base);
>> +
>> +        return 0;
>> +}
>> +
>> +static int artpec8_pcie_get_ep_mem_resources(struct platform_device *pdev,
>> +                                          struct artpec8_pcie *artpec8_ctrl)
>> +{
>> +        struct dw_pcie_ep *ep;
>> +        struct dw_pcie *pci = artpec8_ctrl->pci;
>> +        struct device *dev = &pdev->dev;
>> +        struct resource *res;
>> +
>> +        ep = &pci->ep;
>
> Reorder the locals above:
>
>   struct dw_pcie *pci = artpec8_ctrl->pci;
>   struct device *dev = &pdev->dev;
>   struct dw_pcie_ep *ep = &pci->ep;
>   struct resource *res;
>
> Then they're in the order you use them and you don't need the extra
> "ep = &pci->ep".
>

I will fix it. thank you for your detalied review.

>> +        pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
>> +        if (IS_ERR(pci->dbi_base)) {
>> +                dev_err(dev, "failed to map ep_dbics\n");
>> +                return -ENOMEM;
>> +        }
>> +
>> +        pci->dbi_base2 = devm_platform_ioremap_resource_byname(pdev, "dbi2");
>> +        if (IS_ERR(pci->dbi_base2)) {
>> +                dev_err(dev, "failed to map ep_dbics2\n");
>> +                return -ENOMEM;
>> +        }
>> +
>> +        res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
>> +        if (!res)
>> +                return -EINVAL;
>> +        ep->phys_base = res->start;
>> +        ep->addr_size = resource_size(res);
>> +
>> +        return 0;
>> +}
>> +
>> +static int artpec8_pcie_get_clk_resources(struct platform_device *pdev,
>> +                                       struct artpec8_pcie *artpec8_ctrl)
>> +{
>> +        struct device *dev = &pdev->dev;
>> +
>> +        artpec8_ctrl->pipe_clk = devm_clk_get(dev, "pipe_clk");
>> +        if (IS_ERR(artpec8_ctrl->pipe_clk)) {
>> +                dev_err(dev, "couldn't get pipe clock\n");
>> +                return -EINVAL;
>> +        }
>> +
>> +        artpec8_ctrl->dbi_clk = devm_clk_get(dev, "dbi_clk");
>> +        if (IS_ERR(artpec8_ctrl->dbi_clk)) {
>> +                dev_info(dev, "couldn't get dbi clk\n");
>> +                return -EINVAL;
>> +        }
>> +
>> +        artpec8_ctrl->slv_clk = devm_clk_get(dev, "slv_clk");
>> +        if (IS_ERR(artpec8_ctrl->slv_clk)) {
>> +                dev_err(dev, "couldn't get slave clock\n");
>> +                return -EINVAL;
>> +        }
>> +
>> +        artpec8_ctrl->mstr_clk = devm_clk_get(dev, "mstr_clk");
>> +        if (IS_ERR(artpec8_ctrl->mstr_clk)) {
>> +                dev_info(dev, "couldn't get master clk\n");
>
> It'd be nice if the err/info messages matched the exact DT name:
> "pipe_clk", "dbi_clk", slv_clk", etc.
>

I will fix it.

> Why are some of the above dev_err() and others dev_info() when you
> return -EINVAL in all cases?
>

When property is not found, it just to return error.
I will modify to return PTR_ERR.

>> +                return -EINVAL;
>> +        }
>> +
> +        return 0;
>> +}
>> +
>> +static int artpec8_pcie_init_clk_resources(struct artpec8_pcie *artpec8_ctrl)
>> +{
>> +        clk_prepare_enable(artpec8_ctrl->pipe_clk);
>> +        clk_prepare_enable(artpec8_ctrl->dbi_clk);
>> +        clk_prepare_enable(artpec8_ctrl->mstr_clk);
>> +        clk_prepare_enable(artpec8_ctrl->slv_clk);
>> +
>> +        return 0;
>> +}
>> +
>> +static void artpec8_pcie_deinit_clk_resources(struct artpec8_pcie *artpec8_ctrl)
>> +{
>> +        clk_disable_unprepare(artpec8_ctrl->slv_clk);
>> +        clk_disable_unprepare(artpec8_ctrl->mstr_clk);
>> +        clk_disable_unprepare(artpec8_ctrl->dbi_clk);
>> +        clk_disable_unprepare(artpec8_ctrl->pipe_clk);
>> +}
>> +
>> +static const struct artpec8_pcie_res_ops artpec8_pcie_rc_res_ops = {
>> +        .get_mem_resources        = artpec8_pcie_get_rc_mem_resources,
>> +        .get_clk_resources        = artpec8_pcie_get_clk_resources,
>> +        .init_clk_resources        = artpec8_pcie_init_clk_resources,
>> +        .deinit_clk_resources        = artpec8_pcie_deinit_clk_resources,
>> +};
>> +
>> +static const struct artpec8_pcie_res_ops artpec8_pcie_ep_res_ops = {
>> +        .get_mem_resources        = artpec8_pcie_get_ep_mem_resources,
>> +        .get_clk_resources        = artpec8_pcie_get_clk_resources,
>> +        .init_clk_resources        = artpec8_pcie_init_clk_resources,
>> +        .deinit_clk_resources        = artpec8_pcie_deinit_clk_resources,
>> +};
>> +
>> +static void artpec8_pcie_writel(void __iomem *base, u32 val, u32 reg)
>> +{
>> +        writel(val, base + reg);
>> +}
>> +
>> +static u32 artpec8_pcie_readl(void __iomem *base, u32 reg)
>> +{
>> +        return readl(base + reg);
>> +}
>> +
>> +static int artpec8_pcie_config_phy_power_isolation(struct dw_pcie *pci,
>> +                                                enum artpec8_pcie_reg_bit val)
>> +{
>> +        struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> +        int ret;
>> +
>> +        ret = regmap_write(artpec8_ctrl->pmu_syscon, PMU_SYSCON_PCIE_ISOLATION,
>> +                           val);
>> +
>> +        return ret;
>
>   return regmap_write(artpec8_ctrl->pmu_syscon, ...);
>

I will fix it.

>> +}
>> +
>> +static int artpec8_pcie_config_bus_enable(struct dw_pcie *pci,
>> +                                                enum artpec8_pcie_reg_bit val)
>> +{
>> +        struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> +        int ret;
>> +
>> +        ret = regmap_write(artpec8_ctrl->bus_p_syscon,
>> +                           BUS_SYSCON_BUS_PATH_ENABLE, val);
>> +        if (ret)
>> +                return ret;
>> +
>> +        ret = regmap_write(artpec8_ctrl->bus_s_syscon,
>> +                           BUS_SYSCON_BUS_PATH_ENABLE, val);
>> +        if (ret)
>> +                return ret;
>> +
>> +        return ret;
>
>   return regmap_write(artpec8_ctrl->bus_s_syscon, ...);
>

I will fix it.

>> +}
>> +
>> +static int artpec8_pcie_config_isolation(struct dw_pcie *pci,
>> +                                         enum artpec8_pcie_isolation val)
>> +{
>> +        int ret;
>> +        /* reg_val[0] : for phy power isolation */
>> +        /* reg_val[1] : for bus enable */
>> +        enum artpec8_pcie_reg_bit reg_val[2];
>> +
>> +        switch (val) {
>> +        case PCIE_CLEAR_ISOLATION:
>> +                reg_val[0] = PCIE_REG_BIT_LOW;
>> +                reg_val[1] = PCIE_REG_BIT_HIGH;
>> +                break;
>> +        case PCIE_SET_ISOLATION:
>> +                reg_val[0] = PCIE_REG_BIT_HIGH;
>> +                reg_val[1] = PCIE_REG_BIT_LOW;
>> +                break;
>> +        default:
>> +                return -EINVAL;
>> +        }
>> +
>> +        ret = artpec8_pcie_config_phy_power_isolation(pci, reg_val[0]);
>> +        if (ret)
>> +                return ret;
>> +
>> +        ret = artpec8_pcie_config_bus_enable(pci, reg_val[1]);
>> +        if (ret)
>> +                return ret;
>> +
>> +        return ret;
>
>   return artpec8_pcie_config_bus_enable(pci, ...);
>

I will fix it.

>> +}
>> +
>> +static int artpec8_pcie_config_perstn(struct dw_pcie *pci,
>> +                                      enum artpec8_pcie_reg_bit val)
>> +{
>> +        struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> +        unsigned int bits;
>> +        int ret;
>> +
>> +        if (val == PCIE_REG_BIT_HIGH)
>> +                bits = PCIE_PERSTN;
>> +        else
>> +                bits = 0;
>> +
>> +        ret = regmap_update_bits(artpec8_ctrl->sysreg, FSYS_PCIE_CON,
>> +                                 PCIE_PERSTN, bits);
>> +
>> +        return ret;
>
>   return regmap_update_bits(artpec8_ctrl->sysreg, ...):
>

I will fix it.

>> +}
>> +
>> +static void artpec8_pcie_stop_link(struct dw_pcie *pci)
>> +{
>> +        struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> +        u32 val;
>> +
>> +        val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
>> +                                 PCIE_APP_LTSSM_ENABLE);
>> +
>> +        val &= ~PCIE_ELBI_LTSSM_ENABLE;
>> +        artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
>> +                        PCIE_APP_LTSSM_ENABLE);
>> +}
>> +
>> +static int artpec8_pcie_start_link(struct dw_pcie *pci)
>> +{
>> +        struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> +        u32 val;
>> +
>> +        dw_pcie_dbi_ro_wr_en(pci);
>> +
>> +        /* Equalization disable */
>> +        val = artpec8_pcie_read_dbi(pci, pci->dbi_base, PCIE_GEN3_RELATED_OFF,
>> +                                    4);
>> +        artpec8_pcie_write_dbi(pci, pci->dbi_base, PCIE_GEN3_RELATED_OFF, 4,
>> +                               val | PCIE_GEN3_EQUALIZATION_DISABLE);
>> +
>> +        dw_pcie_dbi_ro_wr_dis(pci);
>> +
>> +        /* assert LTSSM enable */
>> +        val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
>> +                                 PCIE_APP_LTSSM_ENABLE);
>> +
>> +        val |= PCIE_ELBI_LTSSM_ENABLE;
>> +        artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
>> +                        PCIE_APP_LTSSM_ENABLE);
>> +
>> +        return 0;
>> +}
>> +
>> +static irqreturn_t artpec8_pcie_msi_irq_handler(int irq, void *arg)
>> +{
>> +        struct artpec8_pcie *artpec8_ctrl = arg;
>> +        struct dw_pcie *pci = artpec8_ctrl->pci;
>> +        struct pcie_port *pp = &pci->pp;
>> +        u32 val;
>> +
>> +        val = artpec8_pcie_readl(artpec8_ctrl->elbi_base, PCIE_IRQ2_STS);
>> +
>> +        if ((val & IRQ_MSI_ENABLE) == IRQ_MSI_ENABLE) {
>> +                val &= IRQ_MSI_ENABLE;
>> +                artpec8_pcie_writel(artpec8_ctrl->elbi_base, val,
>> +                                    PCIE_IRQ2_STS);
>> +                dw_handle_msi_irq(pp);
>> +        }
>> +
>> +        return IRQ_HANDLED;
>> +}
>> +
>> +static void artpec8_pcie_msi_init(struct artpec8_pcie *artpec8_ctrl)
>> +{
>> +        u32 val;
>> +
>> +        /* enable MSI interrupt */
>> +        val = artpec8_pcie_readl(artpec8_ctrl->elbi_base, PCIE_IRQ2_EN);
>> +        val |= IRQ_MSI_ENABLE;
>> +        artpec8_pcie_writel(artpec8_ctrl->elbi_base, val, PCIE_IRQ2_EN);
>> +}
>> +
>> +static void artpec8_pcie_enable_interrupts(struct artpec8_pcie *artpec8_ctrl)
>> +{
>> +        if (IS_ENABLED(CONFIG_PCI_MSI))
>> +                artpec8_pcie_msi_init(artpec8_ctrl);
>> +}
>> +
>> +static u32 artpec8_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
>> +                                u32 reg, size_t size)
>> +{
>> +        struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> +        u32 val;
>> +        bool is_atu = false;
>> +
>> +        if (base == pci->atu_base) {
>> +                is_atu = true;
>> +                base = pci->dbi_base;
>> +                regmap_write(artpec8_ctrl->sysreg,
>> +                        artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
>> +                                FSYS_PCIE_DBI_ADDR_OVR_ATU);
>> +        }
>> +
>> +        dw_pcie_read(base + reg, size, &val);
>> +
>> +        if (is_atu)
>> +                regmap_write(artpec8_ctrl->sysreg,
>> +                        artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
>> +                                FSYS_PCIE_DBI_ADDR_OVR_CDM);
>> +
>> +        return val;
>> +}
>> +
>> +static void artpec8_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
>> +                                u32 reg, size_t size, u32 val)
>> +{
>> +        struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> +        bool is_atu = false;
>> +
>> +        if (base == pci->atu_base) {
>> +                is_atu = true;
>> +                base = pci->dbi_base;
>> +                regmap_write(artpec8_ctrl->sysreg,
>> +                        artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
>> +                                FSYS_PCIE_DBI_ADDR_OVR_ATU);
>> +        }
>> +
>> +        dw_pcie_write(base + reg, size, val);
>> +
>> +        if (is_atu)
>> +                regmap_write(artpec8_ctrl->sysreg,
>> +                        artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
>> +                                FSYS_PCIE_DBI_ADDR_OVR_CDM);
>> +}
>> +
>> +static void artpec8_pcie_write_dbi2(struct dw_pcie *pci, void __iomem *base,
>> +                                    u32 reg, size_t size, u32 val)
>> +{
>> +        struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> +
>> +        regmap_write(artpec8_ctrl->sysreg,
>> +                artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
>> +                        FSYS_PCIE_DBI_ADDR_OVR_SHADOW);
>> +
>> +        dw_pcie_write(base + reg, size, val);
>> +
>> +        regmap_write(artpec8_ctrl->sysreg,
>> +                artpec8_pcie_dbi_addr_con[artpec8_ctrl->link_id],
>> +                        FSYS_PCIE_DBI_ADDR_OVR_CDM);
>> +}
>> +
>> +static int artpec8_pcie_rd_own_conf(struct pci_bus *bus, unsigned int devfn,
>> +                                    int where, int size, u32 *val)
>> +{
>> +        struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
>> +
>> +        if (PCI_SLOT(devfn)) {
>> +                *val = ~0;
>
>   PCI_SET_ERROR_RESPONSE(val);
>

I will fix it.

>> +                return PCIBIOS_DEVICE_NOT_FOUND;
>> +        }
>>> +
>> +        *val = dw_pcie_read_dbi(pci, where, size);
>> +        return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>> +static int artpec8_pcie_wr_own_conf(struct pci_bus *bus, unsigned int devfn,
>> +                                    int where, int size, u32 val)
>> +{
>> +        struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
>> +
>> +        if (PCI_SLOT(devfn))
>> +                return PCIBIOS_DEVICE_NOT_FOUND;
>> +
>> +        dw_pcie_write_dbi(pci, where, size, val);
>> +        return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>> +static struct pci_ops artpec8_pci_ops = {
>> +        .read = artpec8_pcie_rd_own_conf,
>> +        .write = artpec8_pcie_wr_own_conf,
>> +};
>> +
>> +static int artpec8_pcie_link_up(struct dw_pcie *pci)
>> +{
>> +        struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> +        u32 val;
>> +
>> +        val = artpec8_pcie_readl(artpec8_ctrl->elbi_base,
>> +                        PCIE_ELBI_CXPL_DEBUG_00_31);
>> +
>> +        return (val & LTSSM_STATE_MASK) == LTSSM_STATE_L0;
>> +}
>> +
>> +static int artpec8_pcie_host_init(struct pcie_port *pp)
>> +{
>> +        struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +        struct artpec8_pcie *artpec8_ctrl = to_artpec8_pcie(pci);
>> +
>> +        pp->bridge->ops = &artpec8_pci_ops;
>> +
>> +        dw_pcie_writel_dbi(pci, PCIE_GEN3_RELATED_OFF,
>> +                                (PCIE_GEN3_EQ_PHASE_2_3 |
>> +                                 PCIE_GEN3_RXEQ_PH01_EN |
>> +                                 PCIE_GEN3_RXEQ_RGRDLESS_RXTS));
>> +
>> +        artpec8_pcie_enable_interrupts(artpec8_ctrl);
>> +
>> +        return 0;
>> +}
>> +
>> +static const struct dw_pcie_host_ops artpec8_pcie_host_ops = {
>> +        .host_init = artpec8_pcie_host_init,
>> +};
>> +
>> +static u8 artpec8_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
>> +{
>> +        u32 val;
>> +
>> +        val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
>> +        pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>> +
>> +        if (val == 0xffffffff)
>> +                return 1;
>> +
>> +        return 0;
>> +}
>> +
>> +static void artpec8_pcie_ep_init(struct dw_pcie_ep *ep)
>> +{
>> +        struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +        enum pci_barno bar;
>
> Add blank line here and use usual multi-line comment style:
>
>   /*
>    * Currently PCIe EP core is not ...
>

I will fix it.

>> +        /* Currently PCIe EP core is not setting iatu_unroll_enabled
>> +         * so let's handle it here. We need to find proper place to
>> +         * initialize this so that it can be used as for other EP
>
>   ... can be used for ...
>

Thank you for detailed review.

>> +         * controllers as well.
>> +         */
>> +        pci->iatu_unroll_enabled = artpec8_pcie_iatu_unroll_enabled(pci);
>> +
>> +        for (bar = BAR_0; bar <= BAR_5; bar++)
>> +                dw_pcie_ep_reset_bar(pci, bar);
>> +}
>> +
>> +static int artpec8_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> +                                 enum pci_epc_irq_type type, u16 interrupt_num)
>> +{
>> +        struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +
>> +        switch (type) {
>> +        case PCI_EPC_IRQ_LEGACY:
>> +                return -EINVAL;
>> +        case PCI_EPC_IRQ_MSI:
>> +                return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
>> +        default:
>> +                dev_err(pci->dev, "UNKNOWN IRQ type\n");
>> +        }
>> +
>> +        return 0;
>> +}
>> +
>> +static const struct pci_epc_features artpec8_pcie_epc_features = {
>> +        .linkup_notifier = false,
>> +        .msi_capable = true,
>> +        .msix_capable = false,
>> +};
>> +
>> +static const struct pci_epc_features*
>> +artpec8_pcie_ep_get_features(struct dw_pcie_ep *ep)
>> +{
>> +        return &artpec8_pcie_epc_features;
>> +}
>> +
>> +static const struct dw_pcie_ep_ops artpec8_dw_pcie_ep_ops = {
>> +        .ep_init        = artpec8_pcie_ep_init,
>> +        .raise_irq        = artpec8_pcie_raise_irq,
>> +        .get_features        = artpec8_pcie_ep_get_features,
>> +};
>> +
>> +static int __init artpec8_add_pcie_ep(struct artpec8_pcie *artpec8_ctrl,
>> +                struct platform_device *pdev)
>> +{
>> +        int ret;
>> +        struct dw_pcie_ep *ep;
>> +        struct dw_pcie *pci = artpec8_ctrl->pci;
>> +
>> +        ep = &pci->ep;
>
> Reorder locals and initialize ep as above.
>

I will fix it.

>> +        ep->ops = &artpec8_dw_pcie_ep_ops;
>> +
>> +        dw_pcie_writel_dbi(pci, PCIE_GEN3_RELATED_OFF,
>> +                                (PCIE_GEN3_EQ_PHASE_2_3 |
>> +                                 PCIE_GEN3_RXEQ_PH01_EN |
>> +                                 PCIE_GEN3_RXEQ_RGRDLESS_RXTS));
>> +
>> +        ret = dw_pcie_ep_init(ep);
>> +        if (ret)
>> +                return ret;
>> +
>> +        return 0;
>> +}
>> +
>> +static int __init artpec8_add_pcie_port(struct artpec8_pcie *artpec8_ctrl,
>> +                                        struct platform_device *pdev)
>> +{
>> +        struct dw_pcie *pci = artpec8_ctrl->pci;
>> +        struct pcie_port *pp = &pci->pp;
>> +        struct device *dev = &pdev->dev;
>> +        int ret;
>> +        int irq_flags;
>> +        int irq;
>
> Reorder to be in order of use:
>
>   struct dw_pcie *pci = artpec8_ctrl->pci;
>   struct pcie_port *pp = &pci->pp;
>   struct device *dev = &pdev->dev;
>   int irq;
>   int irq_flags;
>   int ret;
>

I will fix it.

>> +
>> +        if (IS_ENABLED(CONFIG_PCI_MSI)) {
>> +                irq = platform_get_irq_byname(pdev, "intr");
>> +                if (!irq)
>> +                        return -ENODEV;
>> +
>> +                irq_flags = IRQF_SHARED | IRQF_NO_THREAD;
>> +
>> +                ret = devm_request_irq(dev, irq, artpec8_pcie_msi_irq_handler,
>> +                                irq_flags, "artpec8-pcie", artpec8_ctrl);
>> +                if (ret)
>> +                        return ret;
>> +        }
>> +
>> +        /* Prevent core for messing with the IRQ, since it's muxed */
>
>   Prevent core from ...
>

I will fix it.

>> +        pp->msi_irq = -ENODEV;
>> +
>> +        ret = dw_pcie_host_init(pp);
>> +        if (ret)
>> +                return ret;
>> +
>> +        return 0;
>
>   return dw_pcie_host_init(pp);
>

I will fix it.

>> +}
>> +
>> +static const struct dw_pcie_ops artpec8_dw_pcie_ops = {
>> +        .read_dbi        = artpec8_pcie_read_dbi,
>> +        .write_dbi        = artpec8_pcie_write_dbi,
>> +        .write_dbi2        = artpec8_pcie_write_dbi2,
>> +        .start_link        = artpec8_pcie_start_link,
>> +        .stop_link        = artpec8_pcie_stop_link,
>> +        .link_up        = artpec8_pcie_link_up,
>> +};
>> +
>> +static int artpec8_pcie_probe(struct platform_device *pdev)
>> +{
>> +        int ret;
>> +        struct dw_pcie *pci;
>> +        struct pcie_port *pp;
>> +        struct artpec8_pcie *artpec8_ctrl;
>> +        enum dw_pcie_device_mode mode;
>> +        struct device *dev = &pdev->dev;
>> +        const struct artpec8_pcie_pdata *pdata;
>> +        struct device_node *np = dev->of_node;
>
> Reorder in order of use.
>

I will reorder in order of use like below.

struct device *dev = &pdev->dev;
struct artpec8_pcie *artpec8_ctrl;
struct dw_pcie *pci;
const struct artpec8_pcie_pdata *pdata;
enum dw_pcie_device_mode mode;
struct pcie_port *pp;
struct device_node *np = dev->of_node;
int ret;

>> +        artpec8_ctrl = devm_kzalloc(dev, sizeof(*artpec8_ctrl), GFP_KERNEL);
>> +        if (!artpec8_ctrl)
>> +                return -ENOMEM;
>> +
>> +        pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
>> +        if (!pci)
>> +                return -ENOMEM;
>> +
>> +        pdata = (const struct artpec8_pcie_pdata *)
>
> Unnecessary cast.
>

I will remove cast.

>> +                of_device_get_match_data(dev);
>> +        if (!pdata)
>> +                return -ENODEV;
>> +
>> +        mode = (enum dw_pcie_device_mode)pdata->mode;
>> +
>> +        artpec8_ctrl->pci = pci;
>> +        artpec8_ctrl->pdata = pdata;
>> +        artpec8_ctrl->mode = mode;
>> +
>> +        pci->dev = dev;
>> +        pci->ops = pdata->dwc_ops;
>> +        pci->dbi_base2 = NULL;
>> +        pci->dbi_base = NULL;
>> +        pp = &pci->pp;
>> +        pp->ops = artpec8_ctrl->pdata->host_ops;
>> +
>> +        if (mode == DW_PCIE_RC_TYPE)
> +                artpec8_ctrl->link_id = of_alias_get_id(np, "pcierc");
>> +        else
>> +                artpec8_ctrl->link_id = of_alias_get_id(np, "pcieep");
>> +
>> +        ret = artpec8_pcie_get_subsystem_resources(pdev, artpec8_ctrl);
>> +        if (ret)
>> +                return ret;
>> +
>> +        if (pdata->res_ops && pdata->res_ops->get_mem_resources) {
>> +                ret = pdata->res_ops->get_mem_resources(pdev, artpec8_ctrl);
>> +                if (ret)
>> +                        return ret;
>> +        }
>> +
>> +        if (pdata->res_ops && pdata->res_ops->get_clk_resources) {
>> +                ret = pdata->res_ops->get_clk_resources(pdev, artpec8_ctrl);
>> +                if (ret)
>> +                        return ret;
>> +
>> +                ret = pdata->res_ops->init_clk_resources(artpec8_ctrl);
>> +                if (ret)
>> +                        return ret;
>> +        }
>> +
>> +        platform_set_drvdata(pdev, artpec8_ctrl);
>> +
>> +        ret = artpec8_pcie_config_isolation(pci, PCIE_CLEAR_ISOLATION);
>> +        if (ret)
>> +                return ret;
>> +
>> +        ret = artpec8_pcie_config_perstn(pci, PCIE_REG_BIT_HIGH);
>> +        if (ret)
>> +                return ret;
>> +
>> +        artpec8_ctrl->phy = devm_of_phy_get(dev, np, NULL);
>> +        if (IS_ERR(artpec8_ctrl->phy))
>> +                return PTR_ERR(artpec8_ctrl->phy);
>> +
>> +        phy_init(artpec8_ctrl->phy);
>> +        phy_reset(artpec8_ctrl->phy);
>> +
>> +        switch (mode) {
>> +        case DW_PCIE_RC_TYPE:
>> +                artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_RC,
>> +                                PCIE_ARTPEC8_DEVICE_TYPE);
>> +                ret = artpec8_add_pcie_port(artpec8_ctrl, pdev);
>> +                if (ret < 0)
>
> Are there positive return values that indicate success?  Most places
> above you assume "ret != 0" means failure, so just curious why you
> test "ret < 0" instead of just "ret".
>

There is no special reason, but it seems that the format used
in the existing dw driver is applied.

Thank you for kindness reivew.

Best regards,
Wangseok Lee
>> +                        goto fail_probe;
>> +                break;
>> +        case DW_PCIE_EP_TYPE:
>> +                artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_EP,
>> +                                PCIE_ARTPEC8_DEVICE_TYPE);
>> +
>> +                ret = artpec8_add_pcie_ep(artpec8_ctrl, pdev);
>> +                if (ret < 0)
>
> Same question.
>
>> +                        goto fail_probe;
>> +                break;
>> +        default:
>> +                ret = -EINVAL;
>> +                goto fail_probe;
>> +        }
>> +
>> +        return 0;
>> +
>> +fail_probe:
>> +        phy_exit(artpec8_ctrl->phy);
>> +        if (pdata->res_ops && pdata->res_ops->deinit_clk_resources)
>> +                pdata->res_ops->deinit_clk_resources(artpec8_ctrl);
>> +
>> +        return ret;
>> +}
>> +
>> +static int __exit artpec8_pcie_remove(struct platform_device *pdev)
>> +{
>> +        struct artpec8_pcie *artpec8_ctrl = platform_get_drvdata(pdev);
>> +        const struct artpec8_pcie_pdata *pdata = artpec8_ctrl->pdata;
>> +
>> +        if (pdata->res_ops && pdata->res_ops->deinit_clk_resources)
>> +                pdata->res_ops->deinit_clk_resources(artpec8_ctrl);
>> +
>> +        return 0;
>> +}
>> +
>> +static const struct artpec8_pcie_pdata artpec8_pcie_rc_pdata = {
>> +        .dwc_ops        = &artpec8_dw_pcie_ops,
>> +        .host_ops        = &artpec8_pcie_host_ops,
>> +        .res_ops        = &artpec8_pcie_rc_res_ops,
>> +        .mode                = DW_PCIE_RC_TYPE,
>> +};
>> +
>> +static const struct artpec8_pcie_pdata artpec8_pcie_ep_pdata = {
>> +        .dwc_ops        = &artpec8_dw_pcie_ops,
>> +        .host_ops        = &artpec8_pcie_host_ops,
>> +        .res_ops        = &artpec8_pcie_ep_res_ops,
>> +        .mode                = DW_PCIE_EP_TYPE,
>> +};
>> +
>> +static const struct of_device_id artpec8_pcie_of_match[] = {
>> +        {
>> +                .compatible = "axis,artpec8-pcie",
>> +                .data = &artpec8_pcie_rc_pdata,
>> +        },
>> +        {
>> +                .compatible = "axis,artpec8-pcie-ep",
>> +                .data = &artpec8_pcie_ep_pdata,
>> +        },
>> +        {},
>> +};
>> +MODULE_DEVICE_TABLE(of, artpec8_pcie_of_match);
>> +
>> +static struct platform_driver artpec8_pcie_driver = {
>> +        .probe        = artpec8_pcie_probe,
>> +        .remove                = __exit_p(artpec8_pcie_remove),
>> +        .driver = {
>> +                .name        = "artpec8-pcie",
>> +                .of_match_table = artpec8_pcie_of_match,
>> +        },
>> +};
>> +
>> +module_platform_driver(artpec8_pcie_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Jaeho Cho <jaeho79.cho@samsung.com>");
>> --
>> 2.9.5
>>
>> --
>> linux-phy mailing list
>> linux-phy@lists.infradead.org
>> https://protect2.fireeye.com/v1/url?k=8d601177-ec1df90b-8d619a38-74fe485cc33c-571071ed0e3feda8&q=1&e=731ce886-130a-4ad1-bf82-3da4fec844b6&u=https%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-phy