Re: [PATCH v2 4/5] phy: Add ARTPEC-8 PCIe PHY driver

Wangseok Lee
2022-06-08T07:18:46Z

On 06/06/2022 19:34, Krzysztof Kozlowski wrote:
> On 03/06/2022 04:38, 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 driver provides PHY interface for ARTPEC-8 SoC PCIe controller,
>> based on Samsung PCIe PHY IP.
>>
>> Main changes since v1 [1]:
>> -change folder name of phy driver to axis from artpec
>>
>> Signed-off-by: Wangseok Lee <wangseok.lee@samsung.com>
>> ---
>>  drivers/phy/Kconfig                 |   1 +
>>  drivers/phy/Makefile                |   1 +
>>  drivers/phy/axis/Kconfig            |   9 +
>>  drivers/phy/axis/Makefile           |   2 +
>>  drivers/phy/axis/phy-artpec8-pcie.c | 806 ++++++++++++++++++++++++++++++++++++
>>  5 files changed, 819 insertions(+)
>>  create mode 100644 drivers/phy/axis/Kconfig
>>  create mode 100644 drivers/phy/axis/Makefile
>>  create mode 100644 drivers/phy/axis/phy-artpec8-pcie.c
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 300b0f2..92b8232 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -73,6 +73,7 @@ config PHY_CAN_TRANSCEIVER
>>
>>  source "drivers/phy/allwinner/Kconfig"
>>  source "drivers/phy/amlogic/Kconfig"
>> +source "drivers/phy/axis/Kconfig"
>>  source "drivers/phy/broadcom/Kconfig"
>>  source "drivers/phy/cadence/Kconfig"
>>  source "drivers/phy/freescale/Kconfig"
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index 01e9eff..808c055e 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -12,6 +12,7 @@ obj-$(CONFIG_PHY_PISTACHIO_USB)                += phy-pistachio-usb.o
>>  obj-$(CONFIG_USB_LGM_PHY)                += phy-lgm-usb.o
>>  obj-y                                        += allwinner/        \
>>                                             amlogic/        \
>> +                                           axis/                \
>>                                             broadcom/        \
>>                                             cadence/        \
>>                                             freescale/        \
>> diff --git a/drivers/phy/axis/Kconfig b/drivers/phy/axis/Kconfig
>> new file mode 100644
>> index 0000000..7198b93
>> --- /dev/null
>> +++ b/drivers/phy/axis/Kconfig
>> @@ -0,0 +1,9 @@
>> +config PHY_ARTPEC8_PCIE
>> +        bool "ARTPEC-8 PCIe PHY driver"
>> +        depends on OF && (ARCH_ARTPEC8 || COMPILE_TEST)
>> +        select GENERIC_PHY
>> +        help
>> +          Enable PCIe PHY support for ARTPEC-8 SoC.
>> +          This driver provides PHY interface for ARTPEC-8 SoC
>> +          PCIe controller.
>> +          This is based on Samsung PCIe PHY IP.
>> diff --git a/drivers/phy/axis/Makefile b/drivers/phy/axis/Makefile
>> new file mode 100644
>> index 0000000..45d853c
>> --- /dev/null
>> +++ b/drivers/phy/axis/Makefile
>> @@ -0,0 +1,2 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +obj-$(CONFIG_PHY_ARTPEC8_PCIE)                += phy-artpec8-pcie.o
>> diff --git a/drivers/phy/axis/phy-artpec8-pcie.c b/drivers/phy/axis/phy-artpec8-pcie.c
>> new file mode 100644
>> index 0000000..2742301
>> --- /dev/null
>> +++ b/drivers/phy/axis/phy-artpec8-pcie.c
>> @@ -0,0 +1,806 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * PHY provider for ARTPEC-8 PCIe controller
>> + *
>> + * Copyright (C) 2019 Samsung Electronics Co., Ltd.
>> + *                http://www.samsung.com
>> + *
>> + * Author: Jaeho Cho <jaeho79.cho@samsung.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/init.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/regmap.h>
>> +#include <linux/debugfs.h>
>> +
>> +/* ARTPEC-8 PCIe PHY registers */
>> +/* CMN registers */
>> +#define PCIE_PHY_CMN_REG004                0x10
>> +#define PCIE_PHY_CMN_REG00B                0x2C
>> +#define PCIE_PHY_CMN_REG016                0x58
>> +#define PCIE_PHY_CMN_REG01C                0x70
>> +#define PCIE_PHY_CMN_REG021                0x84
>> +#define PCIE_PHY_CMN_REG024                0x90
>> +#define PCIE_PHY_CMN_REG025                0x94
>> +#define PCIE_PHY_CMN_REG0E6                0x398
>> +#define PCIE_PHY_CMN_REG0E7                0x39C
>> +#define PCIE_PHY_CMN_REG0E8                0x3A0
>> +#define PCIE_PHY_CMN_REG0E9                0x3A4
>> +#define PCIE_PHY_CMN_REG0EA                0x3A8
>> +#define PCIE_PHY_CMN_REG0EB                0x3AC
>> +#define PCIE_PHY_CMN_REG0EC                0x3B0
>> +#define PCIE_PHY_CMN_REG0EE                0x3B8
>> +#define PCIE_PHY_CMN_REG0EF                0x3BC
>> +#define PCIE_PHY_CMN_REG0F1                0x3C4
>> +#define PCIE_PHY_CMN_REG0F3                0x3CC
>> +#define PCIE_PHY_CMN_REG0F4                0x3D0
>> +
>> +#define PCIE_PHY_CMN_REG101                0x404
>> +#define OV_I_CMN_RSTN                        BIT(4)
>> +#define OV_I_INIT_RSTN                        BIT(6)
>> +
>> +#define PCIE_PHY_CMN_REG131                0x4C4
>> +#define PCIE_PHY_CMN_REG17B                0x5EC
>> +#define PCIE_PHY_CMN_REG17D                0x5F4
>> +#define PCIE_PHY_CMN_REG190                0x640
>> +#define PCIE_PHY_CMN_REG191                0x644
>> +#define PCIE_PHY_CMN_REG192                0x648
>> +#define PCIE_PHY_CMN_REG1C7                0x71C
>> +#define PCIE_PHY_CMN_REG1DF                0x77C
>> +#define PCIE_PHY_CMN_REG1E0                0x780
>> +
>> +#define PCIE_PHY_CMN_REG0B1                0x2C4
>> +#define ANA_ROPLL_REF_DIG_CLK_SEL        BIT(2)
>> +
>> +/* External clock */
>> +#define PCIE_PHY_CMN_REG14D                0x534
>> +#define PCIE_AUX_RX_MODE_EXTEND                BIT(7)
>> +
>> +#define PCIE_PHY_CMN_REG0D9                0x364
>> +#define ANA_AUX_EXT_REF_CLK_SEL                BIT(4)
>> +
>> +#define PCIE_PHY_CMN_REG10F                0x43C
>> +#define AUX_PLL_EN_EXTEND                BIT(4)
>> +
>> +#define PCIE_PHY_CMN_REG11E                0x478
>> +#define AUX2_PLL_EN_EXTEND                BIT(3)
>> +
>> +#define PCIE_PHY_CMN_REG0D4                0x350
>> +#define OV_S_ANA_AUX_EN                        BIT(3)
>> +#define OV_I_ANA_AUX_EN                        BIT(2)
>> +
>> +/* LANE registers */
>> +#define PCIE_PHY_TRSV_REG22D                0x8B4
>> +#define PCIE_PHY_TRSV_REG23E                0x8F8
>> +#define PCIE_PHY_TRSV_REG2A5                0xA94
>> +#define PCIE_PHY_TRSV_REG3E3                0xF8C
>> +#define PCIE_PHY_TRSV_REG3ED                0xFB4
>> +#define PCIE_PHY_TRSV_REG20B                0x82C
>> +#define PCIE_PHY_TRSV_REG20C                0x830
>> +#define PCIE_PHY_TRSV_REG234                0x8D0
>> +#define PCIE_PHY_TRSV_REG235                0x8D4
>> +#define PCIE_PHY_TRSV_REG237                0x8DC
>> +#define PCIE_PHY_TRSV_REG239                0x8E4
>> +#define PCIE_PHY_TRSV_REG23A                0x8E8
>> +#define PCIE_PHY_TRSV_REG23B                0x8EC
>> +#define PCIE_PHY_TRSV_REG24B                0x92C
>> +#define PCIE_PHY_TRSV_REG25D                0x974
>> +#define PCIE_PHY_TRSV_REG262                0x988
>> +#define PCIE_PHY_TRSV_REG271                0x9C4
>> +#define PCIE_PHY_TRSV_REG272                0x9C8
>> +#define PCIE_PHY_TRSV_REG27C                0x9F0
>> +#define PCIE_PHY_TRSV_REG27D                0x9F4
>> +#define PCIE_PHY_TRSV_REG27E                0x9F8
>> +#define PCIE_PHY_TRSV_REG284                0xA10
>> +#define PCIE_PHY_TRSV_REG289                0xA24
>> +#define PCIE_PHY_TRSV_REG28A                0xA28
>> +#define PCIE_PHY_TRSV_REG28B                0xA2C
>> +#define PCIE_PHY_TRSV_REG28C                0xA30
>> +#define PCIE_PHY_TRSV_REG28E                0xA38
>> +#define PCIE_PHY_TRSV_REG28F                0xA3C
>> +#define PCIE_PHY_TRSV_REG290                0xA40
>> +#define PCIE_PHY_TRSV_REG291                0xA44
>> +#define PCIE_PHY_TRSV_REG292                0xA48
>> +#define PCIE_PHY_TRSV_REG294                0xA50
>> +#define PCIE_PHY_TRSV_REG295                0xA54
>> +#define PCIE_PHY_TRSV_REG296                0xA58
>> +#define PCIE_PHY_TRSV_REG297                0xA5C
>> +#define PCIE_PHY_TRSV_REG298                0xA60
>> +#define PCIE_PHY_TRSV_REG29B                0xA6C
>> +#define PCIE_PHY_TRSV_REG29C                0xA70
>> +#define PCIE_PHY_TRSV_REG29D                0xA74
>> +#define PCIE_PHY_TRSV_REG29E                0xA78
>> +#define PCIE_PHY_TRSV_REG2AA                0xAA8
>> +#define PCIE_PHY_TRSV_REG2AE                0xAB8
>> +#define PCIE_PHY_TRSV_REG2C2                0xB08
>> +#define PCIE_PHY_TRSV_REG2C6                0xB18
>> +#define PCIE_PHY_TRSV_REG2C7                0xB1C
>> +#define PCIE_PHY_TRSV_REG2CB                0xB2C
>> +#define PCIE_PHY_TRSV_REG2CC                0xB30
>> +#define PCIE_PHY_TRSV_REG2CD                0xB34
>> +#define PCIE_PHY_TRSV_REG2CE                0xB38
>> +#define PCIE_PHY_TRSV_REG2D0                0xB40
>> +#define PCIE_PHY_TRSV_REG2CF                0xB3C
>> +#define PCIE_PHY_TRSV_REG2E0                0xB80
>> +#define PCIE_PHY_TRSV_REG2E9                0xBA4
>> +#define PCIE_PHY_TRSV_REG2EA                0xBA8
>> +#define PCIE_PHY_TRSV_REG2EB                0xBAC
>> +#define PCIE_PHY_TRSV_REG315                0xC54
>> +#define PCIE_PHY_TRSV_REG317                0xC5C
>> +#define PCIE_PHY_TRSV_REG319                0xC64
>> +#define PCIE_PHY_TRSV_REG364                0xD90
>> +#define PCIE_PHY_TRSV_REG36C                0xDB0
>> +#define PCIE_PHY_TRSV_REG36D                0xDB4
>> +#define PCIE_PHY_TRSV_REG37E                0xDF8
>> +#define PCIE_PHY_TRSV_REG37F                0xDFC
>> +#define PCIE_PHY_TRSV_REG38F                0xE3C
>> +#define PCIE_PHY_TRSV_REG391                0xE44
>> +#define PCIE_PHY_TRSV_REG39C                0xE70
>> +#define PCIE_PHY_TRSV_REG3A8                0xEA0
>> +#define PCIE_PHY_TRSV_REG3E0                0xF80
>> +#define PCIE_PHY_TRSV_REG3E1                0xF84
>> +#define PCIE_PHY_TRSV_REG3E7                0xF9C
>> +#define PCIE_PHY_TRSV_REG3E9                0xFA4
>> +#define PCIE_PHY_TRSV_REG3EA                0xFA8
>> +#define PCIE_PHY_TRSV_REG3EE                0xFB8
>> +#define PCIE_PHY_TRSV_REG3EF                0xFBC
>> +#define PCIE_PHY_TRSV_REG3F0                0xFC0
>> +
>> +#define PCIE_PHY_TRSV_REG2C0                0xB00
>> +#define LN_EQ_CTRL_RX_DATA_HOLD                BIT(5)
>> +
>> +/* RX Preset registers */
>> +#define PCIE_PHY_CMN_REG17E                0x5F8
>> +#define PCIE_PHY_CMN_REG180                0x600
>> +#define PCIE_PHY_CMN_REG181                0x604
>> +#define PCIE_PHY_CMN_REG182                0x608
>> +#define PCIE_PHY_CMN_REG183                0x60C
>> +#define PCIE_PHY_CMN_REG184                0x610
>> +#define PCIE_PHY_CMN_REG185                0x614
>> +#define PCIE_PHY_CMN_REG186                0x618
>> +#define PCIE_PHY_CMN_REG187                0x61C
>> +
>> +
>> +/* ARTPEC-8 PCIe PCS registers */
>> +#define PCIE_PCS_OUT_VEC_4                0x154
>> +#define B1_DYNAMIC                        BIT(3)
>> +
>> +/* ARTPEC-8 SYS REG registers */
>> +#define FSYS_PCIE_CON                        0x424
>> +#define PCIE_PHY_LCPLL_REFCLK_SEL        0x3
>> +#define PCIE_PHY_ROPLL_REFCLK_SEL        (0x3UL << 2)
>> +#define ROPLL_REFCLK_NOT_AVAILABLE        (0x2UL << 2)
>> +#define PCIE_PHY_LN0_REFCLK_PAD_EN        BIT(10)
>> +#define PCIE_PHY_LN1_REFCLK_PAD_EN        BIT(11)
>> +#define PCIE_PHY_PWR_OFF                BIT(7)
>> +
>> +/* ARTPEC-8 Sub Controller registers */
>> +#define SFR_INIT_RSTN                        0x1404
>> +#define SFR_CMN_RSTN                        0x1408
>> +
>> +#define PCIE_PHY_LN0_REG_START                0x800
>> +#define PCIE_PHY_LN0_REG_END                0xFCC
>> +#define OFFSET_PER_LANE                        0x800
>> +
>> +enum artpec8_pcie_phy_num_lanes {
>> +        LANE0 = 0,
>> +        LANE1,
>> +        LANE_MAX
>> +};
>> +
>> +struct artpec8_pcie_phy_data {
>> +        const struct phy_ops        *ops;
>
> No need for indentation before "*ops". Other places do not use it.
>

Ok. remove unnecessary indentation.

>> +};
>> +
> +struct artpec8_pcie_phy {
>> +        const struct artpec8_pcie_phy_data *drv_data;
>> +        void __iomem *phy_base;
>> +        void __iomem *pcs_base;
>> +        void __iomem *elbi_base;
>> +        struct clk *soc_pll_clk;
>> +        struct regmap *sysreg;
>> +        u32 lcpll_ref_clk;
>> +        const char *mode;
>> +        u32 num_lanes;
>> +};
>> +
>> +enum artpec8_pcie_ref_clk {
>> +        REF_CLK_FROM_XO = 0,
>> +        REF_CLK_FROM_IO,
>> +        REF_CLK_RESERVED,
>> +        REF_CLK_FROM_SOC_PLL,
>> +        REF_CLK_MAX
>> +};
>> +
>> +struct artpec8_pcie_phy_tune_reg {
>> +        u32 offset;
>> +        u32 val;
>> +};
>> +
>> +/* ARTPEC-8 PCIe Gen4 x2 PHY CMN register settings */
>> +struct artpec8_pcie_phy_tune_reg cmn_regs[] = {
>
> static const
>

Ok.

>> +        {PCIE_PHY_CMN_REG004, 0x65},
>> +        {PCIE_PHY_CMN_REG00B, 0x18},
>> +        {PCIE_PHY_CMN_REG016, 0x0E},
>> +        {PCIE_PHY_CMN_REG01C, 0x4F},
>> +        {PCIE_PHY_CMN_REG021, 0x01},
>> +        {PCIE_PHY_CMN_REG024, 0x58},
>> +        {PCIE_PHY_CMN_REG025, 0x98},
>> +        {PCIE_PHY_CMN_REG0E6, 0x00},
>> +        {PCIE_PHY_CMN_REG0E7, 0x00},
>> +        {PCIE_PHY_CMN_REG0E8, 0x3F},
>> +        {PCIE_PHY_CMN_REG0E9, 0x3F},
>> +        {PCIE_PHY_CMN_REG0EA, 0xFF},
>> +        {PCIE_PHY_CMN_REG0EB, 0xFF},
>> +        {PCIE_PHY_CMN_REG0EC, 0x42},
>> +        {PCIE_PHY_CMN_REG0EE, 0x3F},
>> +        {PCIE_PHY_CMN_REG0EF, 0x7F},
>> +        {PCIE_PHY_CMN_REG0F1, 0x02},
>> +        {PCIE_PHY_CMN_REG0F3, 0xFF},
>> +        {PCIE_PHY_CMN_REG0F4, 0xFF},
>> +        {PCIE_PHY_CMN_REG131, 0x01},
>> +        {PCIE_PHY_CMN_REG17B, 0xC0},
>> +        {PCIE_PHY_CMN_REG17D, 0xAF},
>> +        {PCIE_PHY_CMN_REG190, 0x27},
>> +        {PCIE_PHY_CMN_REG191, 0x0F},
>> +        {PCIE_PHY_CMN_REG192, 0x3F},
>> +        {PCIE_PHY_CMN_REG1C7, 0x05},
>> +        {PCIE_PHY_CMN_REG1DF, 0x28},
>> +        {PCIE_PHY_CMN_REG1E0, 0x28},
>> +};
>> +
>> +/* ARTPEC-8 PCIe Gen4 x2 PHY lane register settings */
>> +struct artpec8_pcie_phy_tune_reg lane_regs[] = {
>
> Same.
>

Ok.

>> +        {PCIE_PHY_TRSV_REG22D, 0x00},
>> +        {PCIE_PHY_TRSV_REG23E, 0x00},
>> +        {PCIE_PHY_TRSV_REG2A5, 0x73},
>> +        {PCIE_PHY_TRSV_REG3E3, 0x7B},
>> +        {PCIE_PHY_TRSV_REG3ED, 0x4B},
>> +        {PCIE_PHY_TRSV_REG20B, 0x02},
>> +        {PCIE_PHY_TRSV_REG20C, 0xEA},
>> +        {PCIE_PHY_TRSV_REG234, 0x7A},
>> +        {PCIE_PHY_TRSV_REG235, 0x1C},
>> +        {PCIE_PHY_TRSV_REG237, 0x10},
>> +        {PCIE_PHY_TRSV_REG239, 0x68},
>> +        {PCIE_PHY_TRSV_REG23A, 0xC0},
>> +        {PCIE_PHY_TRSV_REG23B, 0x0B},
>> +        {PCIE_PHY_TRSV_REG24B, 0x00},
>> +        {PCIE_PHY_TRSV_REG25D, 0x07},
>> +        {PCIE_PHY_TRSV_REG262, 0x07},
>> +        {PCIE_PHY_TRSV_REG271, 0x23},
>> +        {PCIE_PHY_TRSV_REG272, 0x5E},
>> +        {PCIE_PHY_TRSV_REG27C, 0x8C},
>> +        {PCIE_PHY_TRSV_REG27D, 0x5B},
>> +        {PCIE_PHY_TRSV_REG27E, 0x2C},
>> +        {PCIE_PHY_TRSV_REG284, 0x33},
>> +        {PCIE_PHY_TRSV_REG289, 0xD4},
>> +        {PCIE_PHY_TRSV_REG28A, 0xCC},
>> +        {PCIE_PHY_TRSV_REG28B, 0xD9},
>> +        {PCIE_PHY_TRSV_REG28C, 0xDC},
>> +        {PCIE_PHY_TRSV_REG28E, 0xC6},
>> +        {PCIE_PHY_TRSV_REG28F, 0x90},
>> +        {PCIE_PHY_TRSV_REG290, 0x4D},
>> +        {PCIE_PHY_TRSV_REG291, 0x19},
>> +        {PCIE_PHY_TRSV_REG292, 0x1C},
>> +        {PCIE_PHY_TRSV_REG294, 0x05},
>> +        {PCIE_PHY_TRSV_REG295, 0x10},
>> +        {PCIE_PHY_TRSV_REG296, 0x0C},
>> +        {PCIE_PHY_TRSV_REG297, 0x19},
>> +        {PCIE_PHY_TRSV_REG298, 0x04},
>> +        {PCIE_PHY_TRSV_REG29B, 0x03},
>> +        {PCIE_PHY_TRSV_REG29C, 0x1B},
>> +        {PCIE_PHY_TRSV_REG29D, 0x1B},
>> +        {PCIE_PHY_TRSV_REG29E, 0x1F},
>> +        {PCIE_PHY_TRSV_REG2AA, 0x00},
>> +        {PCIE_PHY_TRSV_REG2AE, 0x1F},
>> +        {PCIE_PHY_TRSV_REG2C2, 0x25},
>> +        {PCIE_PHY_TRSV_REG2C6, 0x10},
>> +        {PCIE_PHY_TRSV_REG2C7, 0x06},
>> +        {PCIE_PHY_TRSV_REG2CB, 0x10},
>> +        {PCIE_PHY_TRSV_REG2CC, 0x06},
>> +        {PCIE_PHY_TRSV_REG2CD, 0x20},
>> +        {PCIE_PHY_TRSV_REG2CE, 0x27},
>> +        {PCIE_PHY_TRSV_REG2D0, 0x10},
>> +        {PCIE_PHY_TRSV_REG2CF, 0x0A},
>> +        {PCIE_PHY_TRSV_REG2E0, 0x01},
>> +        {PCIE_PHY_TRSV_REG2E9, 0x11},
>> +        {PCIE_PHY_TRSV_REG2EA, 0x05},
>> +        {PCIE_PHY_TRSV_REG2EB, 0x4C},
>> +        {PCIE_PHY_TRSV_REG315, 0x18},
>> +        {PCIE_PHY_TRSV_REG317, 0x86},
>> +        {PCIE_PHY_TRSV_REG319, 0x8E},
>> +        {PCIE_PHY_TRSV_REG364, 0x00},
>> +        {PCIE_PHY_TRSV_REG36C, 0x03},
>> +        {PCIE_PHY_TRSV_REG36D, 0x04},
>> +        {PCIE_PHY_TRSV_REG37E, 0x06},
>> +        {PCIE_PHY_TRSV_REG37F, 0x04},
>> +        {PCIE_PHY_TRSV_REG38F, 0x40},
>> +        {PCIE_PHY_TRSV_REG391, 0x8B},
>> +        {PCIE_PHY_TRSV_REG39C, 0xFF},
>> +        {PCIE_PHY_TRSV_REG3A8, 0x02},
>> +        {PCIE_PHY_TRSV_REG3E0, 0x93},
>> +        {PCIE_PHY_TRSV_REG3E1, 0x79},
>> +        {PCIE_PHY_TRSV_REG3E7, 0xF5},
>> +        {PCIE_PHY_TRSV_REG3E9, 0x75},
>> +        {PCIE_PHY_TRSV_REG3EA, 0x0D},
>> +        {PCIE_PHY_TRSV_REG3EE, 0xE2},
>> +        {PCIE_PHY_TRSV_REG3EF, 0x6F},
>> +        {PCIE_PHY_TRSV_REG3F0, 0x3D}
>> +};
>> +#define PCIE_PHY_CMN_REG17E                0x5F8
>> +#define PCIE_PHY_CMN_REG180                0x600
>> +#define PCIE_PHY_CMN_REG181                0x604
>> +#define PCIE_PHY_CMN_REG182                0x608
>> +#define PCIE_PHY_CMN_REG183                0x60C
>> +#define PCIE_PHY_CMN_REG184                0x610
>> +#define PCIE_PHY_CMN_REG185                0x614
>> +#define PCIE_PHY_CMN_REG186                0x618
>> +#define PCIE_PHY_CMN_REG187                0x61C
>
> Defines go to the top, before any type declarations.
>

Ok.

>> +
>> +struct artpec8_pcie_phy_tune_reg rx_preset_regs[] = {
>
> Same.
>

Ok, static const.

>> +        /* 0 */
>> +        {PCIE_PHY_CMN_REG17E, 0x00},
>> +        {PCIE_PHY_CMN_REG180, 0x23},
>> +        {PCIE_PHY_CMN_REG181, 0x44},
>> +        {PCIE_PHY_CMN_REG182, 0x61},
>> +        {PCIE_PHY_CMN_REG183, 0x55},
>> +        {PCIE_PHY_CMN_REG184, 0x14},
>> +        {PCIE_PHY_CMN_REG185, 0x23},
>> +        {PCIE_PHY_CMN_REG186, 0x1A},
>> +        {PCIE_PHY_CMN_REG187, 0x04},
>> +        {PCIE_PHY_CMN_REG17E, 0x04},
>> +        {PCIE_PHY_CMN_REG17E, 0x00},
>> +        /* 1 */
>> +        {PCIE_PHY_CMN_REG17E, 0x08},
>> +        {PCIE_PHY_CMN_REG181, 0x42},
>> +        {PCIE_PHY_CMN_REG17E, 0x0C},
>> +        {PCIE_PHY_CMN_REG17E, 0x08},
>> +        /* 2 */
>> +        {PCIE_PHY_CMN_REG17E, 0x10},
>> +        {PCIE_PHY_CMN_REG181, 0x40},
>> +        {PCIE_PHY_CMN_REG17E, 0x14},
>> +        {PCIE_PHY_CMN_REG17E, 0x10},
>> +        /* 3 */
>> +        {PCIE_PHY_CMN_REG17E, 0x18},
>> +        {PCIE_PHY_CMN_REG181, 0x45},
>> +        {PCIE_PHY_CMN_REG17E, 0x1C},
>> +        {PCIE_PHY_CMN_REG17E, 0x18},
>> +        /* 4 */
>> +        {PCIE_PHY_CMN_REG17E, 0x20},
>> +        {PCIE_PHY_CMN_REG181, 0x46},
>> +        {PCIE_PHY_CMN_REG17E, 0x24},
>> +        {PCIE_PHY_CMN_REG17E, 0x20},
>> +        /* 5 */
>> +        {PCIE_PHY_CMN_REG17E, 0x28},
>> +        {PCIE_PHY_CMN_REG181, 0x48},
>> +        {PCIE_PHY_CMN_REG17E, 0x2C},
>> +        {PCIE_PHY_CMN_REG17E, 0x28},
>> +        /* 6 */
>> +        {PCIE_PHY_CMN_REG17E, 0x30},
>> +        {PCIE_PHY_CMN_REG181, 0x4A},
>> +        {PCIE_PHY_CMN_REG17E, 0x34},
>> +        {PCIE_PHY_CMN_REG17E, 0x30},
>> +        /* 7 */
>> +        {PCIE_PHY_CMN_REG17E, 0x38},
>> +        {PCIE_PHY_CMN_REG181, 0x4C},
>> +        {PCIE_PHY_CMN_REG17E, 0x3C},
>> +        {PCIE_PHY_CMN_REG17E, 0x38},
>> +        /* 8 */
>> +        {PCIE_PHY_CMN_REG17E, 0x40},
>> +        {PCIE_PHY_CMN_REG180, 0x20},
>> +        {PCIE_PHY_CMN_REG181, 0x20},
>> +        {PCIE_PHY_CMN_REG182, 0x01},
>> +        {PCIE_PHY_CMN_REG17E, 0x44},
>> +        {PCIE_PHY_CMN_REG17E, 0x40},
>> +        /* 9 */
>> +        {PCIE_PHY_CMN_REG17E, 0x48},
>> +        {PCIE_PHY_CMN_REG180, 0x20},
>> +        {PCIE_PHY_CMN_REG181, 0x21},
>> +        {PCIE_PHY_CMN_REG182, 0x01},
>> +        {PCIE_PHY_CMN_REG17E, 0x4C},
>> +        {PCIE_PHY_CMN_REG17E, 0x48},
>> +        /* 10 */
>> +        {PCIE_PHY_CMN_REG17E, 0x50},
>> +        {PCIE_PHY_CMN_REG180, 0x24},
>> +        {PCIE_PHY_CMN_REG181, 0x80},
>> +        {PCIE_PHY_CMN_REG182, 0x41},
>> +        {PCIE_PHY_CMN_REG183, 0xAF},
>> +        {PCIE_PHY_CMN_REG184, 0x26},
>> +        {PCIE_PHY_CMN_REG185, 0x34},
>> +        {PCIE_PHY_CMN_REG186, 0x24},
>> +        {PCIE_PHY_CMN_REG187, 0x06},
>> +        {PCIE_PHY_CMN_REG17E, 0x54},
>> +        {PCIE_PHY_CMN_REG17E, 0x50},
>> +        /* 11 */
>> +        {PCIE_PHY_CMN_REG17E, 0x58},
>> +        {PCIE_PHY_CMN_REG181, 0x81},
>> +        {PCIE_PHY_CMN_REG17E, 0x5C},
>> +        {PCIE_PHY_CMN_REG17E, 0x58},
>> +        /* 12 */
>> +        {PCIE_PHY_CMN_REG17E, 0x60},
>> +        {PCIE_PHY_CMN_REG181, 0x82},
>> +        {PCIE_PHY_CMN_REG17E, 0x64},
>> +        {PCIE_PHY_CMN_REG17E, 0x60},
>> +        /* 13 */
>> +        {PCIE_PHY_CMN_REG17E, 0x68},
>> +        {PCIE_PHY_CMN_REG181, 0x83},
>> +        {PCIE_PHY_CMN_REG17E, 0x6C},
>> +        {PCIE_PHY_CMN_REG17E, 0x68},
>> +        /* 14 */
>> +        {PCIE_PHY_CMN_REG17E, 0x70},
>> +        {PCIE_PHY_CMN_REG181, 0x84},
>> +        {PCIE_PHY_CMN_REG17E, 0x74},
>> +        {PCIE_PHY_CMN_REG17E, 0x70},
>> +        /* 15 */
>> +        {PCIE_PHY_CMN_REG17E, 0x78},
>> +        {PCIE_PHY_CMN_REG180, 0x24},
>> +        {PCIE_PHY_CMN_REG181, 0x85},
>> +        {PCIE_PHY_CMN_REG182, 0x80},
>> +        {PCIE_PHY_CMN_REG183, 0x7F},
>> +        {PCIE_PHY_CMN_REG184, 0x2D},
>> +        {PCIE_PHY_CMN_REG185, 0x34},
>> +        {PCIE_PHY_CMN_REG186, 0x24},
>> +        {PCIE_PHY_CMN_REG187, 0x05},
>> +        {PCIE_PHY_CMN_REG17E, 0x7C},
>> +        {PCIE_PHY_CMN_REG17E, 0x78},
>> +        /* 16 */
>> +        {PCIE_PHY_CMN_REG17E, 0x80},
>> +        {PCIE_PHY_CMN_REG181, 0x86},
>> +        {PCIE_PHY_CMN_REG17E, 0x84},
>> +        {PCIE_PHY_CMN_REG17E, 0x80},
>> +        /* 17 */
>> +        {PCIE_PHY_CMN_REG17E, 0x88},
>> +        {PCIE_PHY_CMN_REG181, 0x87},
>> +        {PCIE_PHY_CMN_REG17E, 0x8C},
>> +        {PCIE_PHY_CMN_REG17E, 0x88},
>> +        /* 18 */
>> +        {PCIE_PHY_CMN_REG17E, 0x90},
>> +        {PCIE_PHY_CMN_REG181, 0x88},
>> +        {PCIE_PHY_CMN_REG17E, 0x94},
>> +        {PCIE_PHY_CMN_REG17E, 0x90},
>> +        /* 19 */
>> +        {PCIE_PHY_CMN_REG17E, 0x98},
>> +        {PCIE_PHY_CMN_REG181, 0x89},
>> +        {PCIE_PHY_CMN_REG17E, 0x9C},
>> +        {PCIE_PHY_CMN_REG17E, 0x98},
>> +};
>> +
>> +
>> +static void artpec8_pcie_phy_reg_writel(void __iomem *base, u32 val, u32 reg)
>> +{
>> +        writel(val, base + reg);
>
> No, do not create wrappers on writel. Remove entire function.
>

Ok, i will remove wrappers function and direct call writel.

>> +};
>> +
>> +static u32 artpec8_pcie_phy_reg_readl(void __iomem *base, u32 reg)
>> +{
>> +        return readl(base + reg);
>
> Ditto
>

Ok, i will remove wrappers function and direct call readl.

>> +};
>> +
>> +static void artpec8_pcie_phy_reg_update(void __iomem *base, u32 mask,
>> +                                        u32 update, u32 reg)
>> +{
>> +        u32 val;
>> +
>> +        val = artpec8_pcie_phy_reg_readl(base, reg);
>> +        val &= ~(mask);
>> +        val |= update;
>> +        artpec8_pcie_phy_reg_writel(base, val, reg);
>> +};
>> +
>
> (...)
>
>> +static int artpec8_pcie_phy_probe(struct platform_device *pdev)
>> +{
>> +        struct device *dev = &pdev->dev;
>> +        struct artpec8_pcie_phy *artpec8_phy;
>> +        struct phy *generic_phy;
>> +        struct phy_provider *phy_provider;
>> +        const struct artpec8_pcie_phy_data *drv_data;
>> +
>> +        drv_data = of_device_get_match_data(dev);
>> +        if (!drv_data)
>> +                return -ENODEV;
>> +
>> +        artpec8_phy = devm_kzalloc(dev, sizeof(*artpec8_phy), GFP_KERNEL);
>> +        if (!artpec8_phy)
>> +                return -ENOMEM;
>> +
>> +        /* reference clock */
>> +        if (of_property_read_u32(dev->of_node, "lcpll-ref-clk",
>
> No, really, no...
>
> All properties *must* be documented in the bindings. You cannot sneak in
> some stuff here...
>

Ok, sure.
I will should be documented the all property.

>> +                                &artpec8_phy->lcpll_ref_clk)) {
>> +                return -EINVAL;
>> +        }
>> +        /* PLL SOC reference clock */
>> +        if (artpec8_phy->lcpll_ref_clk == REF_CLK_FROM_SOC_PLL) {
>> +                artpec8_phy->soc_pll_clk = devm_clk_get(dev, "ref_clk");
>> +                if (IS_ERR(artpec8_phy->soc_pll_clk))
>> +                        return -EINVAL;
>> +                clk_prepare_enable(artpec8_phy->soc_pll_clk);
>> +        }
>> +
>> +        /* link mode */
>> +        if (of_property_read_string(dev->of_node, "mode", &artpec8_phy->mode))
>> +                return -EINVAL;
>> +
>> +        /* number of lanes */
>> +        if (of_property_read_u32(dev->of_node, "num-lanes",
>> +                                 &artpec8_phy->num_lanes))
>> +                return -EINVAL;
>> +
>> +        if (artpec8_phy->num_lanes > LANE_MAX)
>> +                return -EINVAL;
>> +
>> +        /* PHY base register */
>> +        artpec8_phy->phy_base = devm_platform_ioremap_resource_byname(pdev, "phy");
>> +        if (IS_ERR(artpec8_phy->phy_base))
>> +                return PTR_ERR(artpec8_phy->phy_base);
>> +
>> +        /* PCS base register */
>> +        artpec8_phy->pcs_base = devm_platform_ioremap_resource_byname(pdev, "pcs");
>> +        if (IS_ERR(artpec8_phy->pcs_base))
>> +                return PTR_ERR(artpec8_phy->pcs_base);
>> +
>> +        /* sysreg regmap handle, need to change using smc */
>> +        artpec8_phy->sysreg =
>> +                syscon_regmap_lookup_by_phandle(dev->of_node,
>> +                        "samsung,fsys-sysreg");
>
> Nope. Usage of undocumented properties. Please post your DTS changes, so
> we can validate the user of this driver.
>

I will should be documented the all property.

> Best regards,
> Krzysztof
>

Thank you for kindness reivew.
I will return with v3 patchset which is applied review comments.

Best regards,
Wangseok Lee