Re: [PATCH v7 2/3] phy: zynqmp: Add PHY driver for the Xilinx ZynqMP Gigabit Transceiver
From: Kishon Vijay Abraham I
Date: Tue May 12 2020 - 22:46:29 EST
Hi Laurent,
On 5/8/2020 6:23 AM, Laurent Pinchart wrote:
> Hi Kishon,
>
> On Thu, May 07, 2020 at 10:14:45AM +0530, Kishon Vijay Abraham I wrote:
>> On 4/2/2020 3:40 AM, Laurent Pinchart wrote:
>>> From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx>
>>>
>>> Xilinx ZynqMP SoCs have a Gigabit Transceiver with four lanes. All the
>>> high speed peripherals such as USB, SATA, PCIE, Display Port and
>>> Ethernet SGMII can rely on any of the four GT lanes for PHY layer. This
>>> patch adds driver for that ZynqMP GT core.
>>>
>>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>> ---
>>> Changes since v5:
>>>
>>> - Cleanup headers
>>> - Organize the code in sections
>>> - Constify data tables and structures
>>> - Allocate all PHY instances in one go
>>> - Add I/O accessors
>>> - Move DP-specific init to a separate function
>>> - Use devm_platform_ioremap_resource_byname()
>>> - Simplify acquisition of reset controllers
>>> - Implement .configure() PHY operation for DP
>>> - Implement .power_on() and .power_off() operations
>>> - Wait for PLL lock for DP PHY too
>>> - Remove USB core reset operations
>>> - Fix SGMII bus width settings
>>> - Update copyright notice and authors list
>>> - Disable error messages on probe deferral
>>> - Update reset names to new DT bindings
>>> - Update to removal of subnodes in new DT bindings
>>> - Handle reference clocks through CCF
>>> - Add MAINTAINERS entry
>>> - Drop reset handling
>>> - Split TX term fix to separate function
>>> - Remove unused registers
>>> ---
>>> MAINTAINERS | 9 +
>>> drivers/phy/Kconfig | 8 +
>>> drivers/phy/Makefile | 1 +
>>> drivers/phy/phy-zynqmp.c | 995 +++++++++++++++++++++++++++++++++++++++
>>
>> Better to add a xilinx directory for this driver.
>
> OK.
>
>>> 4 files changed, 1013 insertions(+)
>>> create mode 100644 drivers/phy/phy-zynqmp.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 07293073c4f6..19e630fcaf62 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -18406,6 +18406,15 @@ F: Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dpdma.yaml
>>> F: drivers/dma/xilinx/xilinx_dpdma.c
>>> F: include/dt-bindings/dma/xlnx-zynqmp-dpdma.h
>>>
>>> +XILINX ZYNQMP GSPTR PHY DRIVER
>>
>> Looks like a typo here, rest of the place seems to use PSGTR
>
> Good catch. Will fix it.
>
>>> +M: Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx>
>>> +M: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>> +L: linux-kernel@xxxxxxxxxxxxxxx
>>> +T: git https://github.com/Xilinx/linux-xlnx.git
>>> +S: Supported
>>> +F: Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
>>> +F: drivers/phy/phy-zynqmp.c
>>> +
>>> XILLYBUS DRIVER
>>> M: Eli Billauer <eli.billauer@xxxxxxxxx>
>>> L: linux-kernel@xxxxxxxxxxxxxxx
>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>>> index b3ed94b98d9b..b8251b9f3d87 100644
>>> --- a/drivers/phy/Kconfig
>>> +++ b/drivers/phy/Kconfig
>>> @@ -49,6 +49,14 @@ config PHY_XGENE
>>> help
>>> This option enables support for APM X-Gene SoC multi-purpose PHY.
>>>
>>> +config PHY_XILINX_ZYNQMP
>>> + tristate "Xilinx ZynqMP PHY driver"
>>> + depends on ARCH_ZYNQMP
>>> + select GENERIC_PHY
>>> + help
>>> + Enable this to support ZynqMP High Speed Gigabit Transceiver
>>> + that is part of ZynqMP SoC.
>>> +
>>> source "drivers/phy/allwinner/Kconfig"
>>> source "drivers/phy/amlogic/Kconfig"
>>> source "drivers/phy/broadcom/Kconfig"
>>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>>> index 310c149a9df5..5dc7469f078b 100644
>>> --- a/drivers/phy/Makefile
>>> +++ b/drivers/phy/Makefile
>>> @@ -8,6 +8,7 @@ obj-$(CONFIG_GENERIC_PHY_MIPI_DPHY) += phy-core-mipi-dphy.o
>>> obj-$(CONFIG_PHY_LPC18XX_USB_OTG) += phy-lpc18xx-usb-otg.o
>>> obj-$(CONFIG_PHY_XGENE) += phy-xgene.o
>>> obj-$(CONFIG_PHY_PISTACHIO_USB) += phy-pistachio-usb.o
>>> +obj-$(CONFIG_PHY_XILINX_ZYNQMP) += phy-zynqmp.o
>>> obj-$(CONFIG_ARCH_SUNXI) += allwinner/
>>> obj-$(CONFIG_ARCH_MESON) += amlogic/
>>> obj-$(CONFIG_ARCH_MEDIATEK) += mediatek/
>>> diff --git a/drivers/phy/phy-zynqmp.c b/drivers/phy/phy-zynqmp.c
>>> new file mode 100644
>>> index 000000000000..8ab99d6b9220
>>> --- /dev/null
>>> +++ b/drivers/phy/phy-zynqmp.c
>>> @@ -0,0 +1,995 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * phy-zynqmp.c - PHY driver for Xilinx ZynqMP GT.
>>> + *
>>> + * Copyright (C) 2018-20 Xilinx Inc.
>>> + *
>>> + * Author: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>
>>> + * Author: Subbaraya Sundeep <sundeep.lkml@xxxxxxxxx>
>>> + * Author: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>> + *
>>> + * This driver is tested for USB, SATA and Display Port currently.
>>> + * Other controllers PCIe and SGMII should also work but that is
>>> + * experimental as of now.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/phy/phy.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/slab.h>
>>> +
>>> +#include <dt-bindings/phy/phy.h>
>>> +
>>> +/*
>>> + * Lane Registers
>>> + */
>>> +
>>> +/* TX De-emphasis parameters */
>>> +#define L0_TX_ANA_TM_18 0x0048
>>> +#define L0_TX_ANA_TM_118 0x01d8
>>> +#define L0_TX_ANA_TM_118_FORCE_17_0 BIT(0)
>>> +
>>> +/* DN Resistor calibration code parameters */
>>> +#define L0_TXPMA_ST_3 0x0b0c
>>> +#define L0_DN_CALIB_CODE 0x3f
>>> +
>>> +/* PMA control parameters */
>>> +#define L0_TXPMD_TM_45 0x0cb4
>>> +#define L0_TXPMD_TM_48 0x0cc0
>>> +#define L0_TXPMD_TM_45_OVER_DP_MAIN BIT(0)
>>> +#define L0_TXPMD_TM_45_ENABLE_DP_MAIN BIT(1)
>>> +#define L0_TXPMD_TM_45_OVER_DP_POST1 BIT(2)
>>> +#define L0_TXPMD_TM_45_ENABLE_DP_POST1 BIT(3)
>>> +#define L0_TXPMD_TM_45_OVER_DP_POST2 BIT(4)
>>> +#define L0_TXPMD_TM_45_ENABLE_DP_POST2 BIT(5)
>>> +
>>> +/* PCS control parameters */
>>> +#define L0_TM_DIG_6 0x106c
>>> +#define L0_TM_DIS_DESCRAMBLE_DECODER 0x0f
>>> +#define L0_TX_DIG_61 0x00f4
>>> +#define L0_TM_DISABLE_SCRAMBLE_ENCODER 0x0f
>>> +
>>> +/* PLL Test Mode register parameters */
>>> +#define L0_TM_PLL_DIG_37 0x2094
>>> +#define L0_TM_COARSE_CODE_LIMIT 0x10
>>> +
>>> +/* PLL SSC step size offsets */
>>> +#define L0_PLL_SS_STEPS_0_LSB 0x2368
>>> +#define L0_PLL_SS_STEPS_1_MSB 0x236c
>>> +#define L0_PLL_SS_STEP_SIZE_0_LSB 0x2370
>>> +#define L0_PLL_SS_STEP_SIZE_1 0x2374
>>> +#define L0_PLL_SS_STEP_SIZE_2 0x2378
>>> +#define L0_PLL_SS_STEP_SIZE_3_MSB 0x237c
>>> +#define L0_PLL_STATUS_READ_1 0x23e4
>>> +
>>> +/* SSC step size parameters */
>>> +#define STEP_SIZE_0_MASK 0xff
>>> +#define STEP_SIZE_1_MASK 0xff
>>> +#define STEP_SIZE_2_MASK 0xff
>>> +#define STEP_SIZE_3_MASK 0x3
>>> +#define STEP_SIZE_SHIFT 8
>>> +#define FORCE_STEP_SIZE 0x10
>>> +#define FORCE_STEPS 0x20
>>> +#define STEPS_0_MASK 0xff
>>> +#define STEPS_1_MASK 0x07
>>> +
>>> +/* Reference clock selection parameters */
>>> +#define L0_Ln_REF_CLK_SEL(n) (0x2860 + (n) * 4)
>>> +#define L0_REF_CLK_SEL_MASK 0x8f
>>> +
>>> +/* Calibration digital logic parameters */
>>> +#define L3_TM_CALIB_DIG19 0xec4c
>>> +#define L3_CALIB_DONE_STATUS 0xef14
>>> +#define L3_TM_CALIB_DIG18 0xec48
>>> +#define L3_TM_CALIB_DIG19_NSW 0x07
>>> +#define L3_TM_CALIB_DIG18_NSW 0xe0
>>> +#define L3_TM_OVERRIDE_NSW_CODE 0x20
>>> +#define L3_CALIB_DONE 0x02
>>> +#define L3_NSW_SHIFT 5
>>> +#define L3_NSW_PIPE_SHIFT 4
>>> +#define L3_NSW_CALIB_SHIFT 3
>>> +
>>> +#define PHY_REG_OFFSET 0x4000
>>> +
>>> +/*
>>> + * Global Registers
>>> + */
>>> +
>>> +/* Refclk selection parameters */
>>> +#define PLL_REF_SEL(n) (0x10000 + (n) * 4)
>>> +#define PLL_FREQ_MASK 0x1f
>>> +#define PLL_STATUS_LOCKED 0x10
>>> +
>>> +/* Inter Connect Matrix parameters */
>>> +#define ICM_CFG0 0x10010
>>> +#define ICM_CFG1 0x10014
>>> +#define ICM_CFG0_L0_MASK 0x07
>>> +#define ICM_CFG0_L1_MASK 0x70
>>> +#define ICM_CFG1_L2_MASK 0x07
>>> +#define ICM_CFG2_L3_MASK 0x70
>>> +#define ICM_CFG_SHIFT 4
>>> +
>>> +/* Inter Connect Matrix allowed protocols */
>>> +#define ICM_PROTOCOL_PD 0x0
>>> +#define ICM_PROTOCOL_PCIE 0x1
>>> +#define ICM_PROTOCOL_SATA 0x2
>>> +#define ICM_PROTOCOL_USB 0x3
>>> +#define ICM_PROTOCOL_DP 0x4
>>> +#define ICM_PROTOCOL_SGMII 0x5
>>> +
>>> +/* Test Mode common reset control parameters */
>>> +#define TM_CMN_RST 0x10018
>>> +#define TM_CMN_RST_EN 0x1
>>> +#define TM_CMN_RST_SET 0x2
>>> +#define TM_CMN_RST_MASK 0x3
>>> +
>>> +/* Bus width parameters */
>>> +#define TX_PROT_BUS_WIDTH 0x10040
>>> +#define RX_PROT_BUS_WIDTH 0x10044
>>> +#define PROT_BUS_WIDTH_10 0x0
>>> +#define PROT_BUS_WIDTH_20 0x1
>>> +#define PROT_BUS_WIDTH_40 0x2
>>> +#define PROT_BUS_WIDTH_SHIFT 2
>>> +
>>> +/* Number of GT lanes */
>>> +#define NUM_LANES 4
>>> +
>>> +/* SIOU SATA control register */
>>> +#define SATA_CONTROL_OFFSET 0x0100
>>> +
>>> +/* Total number of controllers */
>>> +#define CONTROLLERS_PER_LANE 5
>>> +
>>> +/* Protocol Type parameters */
>>> +#define XPSGTR_TYPE_USB0 0 /* USB controller 0 */
>>> +#define XPSGTR_TYPE_USB1 1 /* USB controller 1 */
>>> +#define XPSGTR_TYPE_SATA_0 2 /* SATA controller lane 0 */
>>> +#define XPSGTR_TYPE_SATA_1 3 /* SATA controller lane 1 */
>>> +#define XPSGTR_TYPE_PCIE_0 4 /* PCIe controller lane 0 */
>>> +#define XPSGTR_TYPE_PCIE_1 5 /* PCIe controller lane 1 */
>>> +#define XPSGTR_TYPE_PCIE_2 6 /* PCIe controller lane 2 */
>>> +#define XPSGTR_TYPE_PCIE_3 7 /* PCIe controller lane 3 */
>>> +#define XPSGTR_TYPE_DP_0 8 /* Display Port controller lane 0 */
>>> +#define XPSGTR_TYPE_DP_1 9 /* Display Port controller lane 1 */
>>> +#define XPSGTR_TYPE_SGMII0 10 /* Ethernet SGMII controller 0 */
>>> +#define XPSGTR_TYPE_SGMII1 11 /* Ethernet SGMII controller 1 */
>>> +#define XPSGTR_TYPE_SGMII2 12 /* Ethernet SGMII controller 2 */
>>> +#define XPSGTR_TYPE_SGMII3 13 /* Ethernet SGMII controller 3 */
>>> +
>>> +/* Timeout values */
>>> +#define TIMEOUT_US 1000
>>> +
>>> +struct xpsgtr_dev;
>>> +
>>> +/**
>>> + * struct xpsgtr_ssc - structure to hold SSC settings for a lane
>>> + * @refclk_rate: PLL reference clock frequency
>>> + * @pll_ref_clk: value to be written to register for corresponding ref clk rate
>>> + * @steps: number of steps of SSC (Spread Spectrum Clock)
>>> + * @step_size: step size of each step
>>> + */
>>> +struct xpsgtr_ssc {
>>> + u32 refclk_rate;
>>> + u8 pll_ref_clk;
>>> + u32 steps;
>>> + u32 step_size;
>>> +};
>>> +
>>> +/**
>>> + * struct xpsgtr_phy - representation of a lane
>>> + * @phy: pointer to the kernel PHY device
>>> + * @type: controller which uses this lane
>>> + * @lane: lane number
>>> + * @protocol: protocol in which the lane operates
>>> + * @skip_phy_init: skip phy_init() if true
>>> + * @dev: pointer to the xpsgtr_dev instance
>>> + * @refclk: reference clock index
>>> + */
>>> +struct xpsgtr_phy {
>>> + struct phy *phy;
>>> + u8 type;
>>> + u8 lane;
>>> + u8 protocol;
>>> + bool skip_phy_init;
>>> + struct xpsgtr_dev *dev;
>>> + unsigned int refclk;
>>> +};
>>> +
>>> +/**
>>> + * struct xpsgtr_dev - representation of a ZynMP GT device
>>> + * @dev: pointer to device
>>> + * @serdes: serdes base address
>>> + * @siou: siou base address
>>> + * @gtr_mutex: mutex for locking
>>> + * @phys: PHY lanes
>>> + * @refclk_sscs: spread spectrum settings for the reference clocks
>>> + * @tx_term_fix: fix for GT issue
>>> + * @saved_icm_cfg0: stored value of ICM CFG0 register
>>> + * @saved_icm_cfg1: stored value of ICM CFG1 register
>>> + */
>>> +struct xpsgtr_dev {
>>> + struct device *dev;
>>> + void __iomem *serdes;
>>> + void __iomem *siou;
>>> + struct mutex gtr_mutex; /* mutex for locking */
>>> + struct xpsgtr_phy phys[NUM_LANES];
>>> + const struct xpsgtr_ssc *refclk_sscs[NUM_LANES];
>>> + bool tx_term_fix;
>>> + unsigned int saved_icm_cfg0;
>>> + unsigned int saved_icm_cfg1;
>>> +};
>>> +
>>> +/* -----------------------------------------------------------------------------
>>
>> The dash here is not commonly used. Please stick to default multi-line comment
>> style.
>
> It's more common in some subsystems than others :-) No big deal, I'll
> drop it (even if I think it increases readability by clearly marking
> sections visually).
>
>>> + * Configuration Data
>>> + */
>>> +
>>> +/* lookup table to hold all settings needed for a ref clock frequency */
>>> +static const struct xpsgtr_ssc ssc_lookup[] = {
>>> + { 19200000, 0x05, 608, 264020 },
>>> + { 20000000, 0x06, 634, 243454 },
>>> + { 24000000, 0x07, 760, 168973 },
>>> + { 26000000, 0x08, 824, 143860 },
>>> + { 27000000, 0x09, 856, 86551 },
>>> + { 38400000, 0x0a, 1218, 65896 },
>>> + { 40000000, 0x0b, 634, 243454 },
>>> + { 52000000, 0x0c, 824, 143860 },
>>> + { 100000000, 0x0d, 1058, 87533 },
>>> + { 108000000, 0x0e, 856, 86551 },
>>> + { 125000000, 0x0f, 992, 119497 },
>>> + { 135000000, 0x10, 1070, 55393 },
>>> + { 150000000, 0x11, 792, 187091 }
>>> +};
>>> +
>>> +/* -----------------------------------------------------------------------------
>>> + * I/O Accessors
>>> + */
>>> +
>>> +static inline u32 xpsgtr_read(struct xpsgtr_dev *gtr_dev, u32 reg)
>>> +{
>>> + return readl(gtr_dev->serdes + reg);
>>> +}
>>> +
>>> +static inline void xpsgtr_write(struct xpsgtr_dev *gtr_dev, u32 reg, u32 value)
>>> +{
>>> + writel(value, gtr_dev->serdes + reg);
>>> +}
>>> +
>>> +static inline void xpsgtr_clr_set(struct xpsgtr_dev *gtr_dev, u32 reg,
>>> + u32 clr, u32 set)
>>> +{
>>> + u32 value = xpsgtr_read(gtr_dev, reg);
>>> +
>>> + value &= ~clr;
>>> + value |= set;
>>> + xpsgtr_write(gtr_dev, reg, value);
>>> +}
>>> +
>>> +static inline u32 xpsgtr_read_phy(struct xpsgtr_phy *gtr_phy, u32 reg)
>>> +{
>>> + void __iomem *addr = gtr_phy->dev->serdes
>>> + + gtr_phy->lane * PHY_REG_OFFSET + reg;
>>> +
>>> + return readl(addr);
>>> +}
>>> +
>>> +static inline void xpsgtr_write_phy(struct xpsgtr_phy *gtr_phy,
>>> + u32 reg, u32 value)
>>> +{
>>> + void __iomem *addr = gtr_phy->dev->serdes
>>> + + gtr_phy->lane * PHY_REG_OFFSET + reg;
>>> +
>>> + writel(value, addr);
>>> +}
>>> +
>>> +static inline void xpsgtr_clr_set_phy(struct xpsgtr_phy *gtr_phy,
>>> + u32 reg, u32 clr, u32 set)
>>> +{
>>> + void __iomem *addr = gtr_phy->dev->serdes
>>> + + gtr_phy->lane * PHY_REG_OFFSET + reg;
>>> +
>>> + writel((readl(addr) & ~clr) | set, addr);
>>> +}
>>> +
>>> +/* -----------------------------------------------------------------------------
>>> + * Hardware Configuration
>>> + */
>>> +
>>> +/* Wait for the PLL to lock (with a timeout). */
>>> +static int xpsgtr_wait_pll_lock(struct phy *phy)
>>> +{
>>> + struct xpsgtr_phy *gtr_phy = phy_get_drvdata(phy);
>>> + struct xpsgtr_dev *gtr_dev = gtr_phy->dev;
>>> + unsigned int timeout = TIMEOUT_US;
>>> + int ret;
>>> +
>>> + dev_dbg(gtr_dev->dev, "Waiting for PLL lock\n");
>>> +
>>> + while (1) {
>>> + u32 reg = xpsgtr_read_phy(gtr_phy, L0_PLL_STATUS_READ_1);
>>> +
>>> + if ((reg & PLL_STATUS_LOCKED) == PLL_STATUS_LOCKED) {
>>> + ret = 0;
>>> + break;
>>> + }
>>> +
>>> + if (--timeout == 0) {
>>> + ret = -ETIMEDOUT;
>>> + break;
>>> + }
>>> +
>>> + udelay(1);
>>> + }
>>> +
>>> + if (ret == -ETIMEDOUT)
>>> + dev_err(gtr_dev->dev,
>>> + "lane %u (type %u, protocol %u): PLL lock timeout\n",
>>> + gtr_phy->lane, gtr_phy->type, gtr_phy->protocol);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +/* Configure PLL and spread-sprectrum clock. */
>>> +static void xpsgtr_configure_pll(struct xpsgtr_phy *gtr_phy)
>>> +{
>>> + const struct xpsgtr_ssc *ssc;
>>> + u32 step_size;
>>> +
>>> + ssc = gtr_phy->dev->refclk_sscs[gtr_phy->refclk];
>>> + step_size = ssc->step_size;
>>> +
>>> + xpsgtr_clr_set(gtr_phy->dev, PLL_REF_SEL(gtr_phy->lane),
>>> + PLL_FREQ_MASK, ssc->pll_ref_clk);
>>> +
>>> + /* Enable lane clock sharing, if required */
>>> + if (gtr_phy->refclk != gtr_phy->lane) {
>>> + /* Lane3 Ref Clock Selection Register */
>>> + xpsgtr_clr_set(gtr_phy->dev, L0_Ln_REF_CLK_SEL(gtr_phy->lane),
>>> + L0_REF_CLK_SEL_MASK, 1 << gtr_phy->refclk);
>>> + }
>>> +
>>> + /* SSC step size [7:0] */
>>> + xpsgtr_clr_set_phy(gtr_phy, L0_PLL_SS_STEP_SIZE_0_LSB,
>>> + STEP_SIZE_0_MASK, step_size & STEP_SIZE_0_MASK);
>>> +
>>> + /* SSC step size [15:8] */
>>> + step_size >>= STEP_SIZE_SHIFT;
>>> + xpsgtr_clr_set_phy(gtr_phy, L0_PLL_SS_STEP_SIZE_1,
>>> + STEP_SIZE_1_MASK, step_size & STEP_SIZE_1_MASK);
>>> +
>>> + /* SSC step size [23:16] */
>>> + step_size >>= STEP_SIZE_SHIFT;
>>> + xpsgtr_clr_set_phy(gtr_phy, L0_PLL_SS_STEP_SIZE_2,
>>> + STEP_SIZE_2_MASK, step_size & STEP_SIZE_2_MASK);
>>> +
>>> + /* SSC steps [7:0] */
>>> + xpsgtr_clr_set_phy(gtr_phy, L0_PLL_SS_STEPS_0_LSB,
>>> + STEPS_0_MASK, ssc->steps & STEPS_0_MASK);
>>> +
>>> + /* SSC steps [10:8] */
>>> + xpsgtr_clr_set_phy(gtr_phy, L0_PLL_SS_STEPS_1_MSB,
>>> + STEPS_1_MASK,
>>> + (ssc->steps >> STEP_SIZE_SHIFT) & STEPS_1_MASK);
>>> +
>>> + /* SSC step size [24:25] */
>>> + step_size >>= STEP_SIZE_SHIFT;
>>> + xpsgtr_clr_set_phy(gtr_phy, L0_PLL_SS_STEP_SIZE_3_MSB,
>>> + STEP_SIZE_3_MASK, (step_size & STEP_SIZE_3_MASK) |
>>> + FORCE_STEP_SIZE | FORCE_STEPS);
>>> +}
>>> +
>>> +/* Configure the lane protocol. */
>>> +static void xpsgtr_lane_set_protocol(struct xpsgtr_phy *gtr_phy)
>>> +{
>>> + struct xpsgtr_dev *gtr_dev = gtr_phy->dev;
>>> + u8 protocol = gtr_phy->protocol;
>>> +
>>> + switch (gtr_phy->lane) {
>>> + case 0:
>>> + xpsgtr_clr_set(gtr_dev, ICM_CFG0, ICM_CFG0_L0_MASK, protocol);
>>> + break;
>>> + case 1:
>>> + xpsgtr_clr_set(gtr_dev, ICM_CFG0, ICM_CFG0_L1_MASK,
>>> + protocol << ICM_CFG_SHIFT);
>>> + break;
>>> + case 2:
>>> + xpsgtr_clr_set(gtr_dev, ICM_CFG1, ICM_CFG0_L0_MASK, protocol);
>>> + break;
>>> + case 3:
>>> + xpsgtr_clr_set(gtr_dev, ICM_CFG1, ICM_CFG0_L1_MASK,
>>> + protocol << ICM_CFG_SHIFT);
>>> + break;
>>> + default:
>>> + /* We already checked 0 <= lane <= 3 */
>>> + break;
>>> + }
>>> +}
>>> +
>>> +/* Bypass (de)scrambler and 8b/10b decoder and encoder. */
>>> +static void xpsgtr_bypass_scrambler_8b10b(struct xpsgtr_phy *gtr_phy)
>>> +{
>>> + xpsgtr_write_phy(gtr_phy, L0_TM_DIG_6, L0_TM_DIS_DESCRAMBLE_DECODER);
>>> + xpsgtr_write_phy(gtr_phy, L0_TX_DIG_61, L0_TM_DISABLE_SCRAMBLE_ENCODER);
>>> +}
>>> +
>>> +/* DP-specific initialization. */
>>> +static void xpsgtr_phy_init_dp(struct xpsgtr_phy *gtr_phy)
>>> +{
>>> + xpsgtr_write_phy(gtr_phy, L0_TXPMD_TM_45,
>>> + L0_TXPMD_TM_45_OVER_DP_MAIN |
>>> + L0_TXPMD_TM_45_ENABLE_DP_MAIN |
>>> + L0_TXPMD_TM_45_OVER_DP_POST1 |
>>> + L0_TXPMD_TM_45_OVER_DP_POST2 |
>>> + L0_TXPMD_TM_45_ENABLE_DP_POST2);
>>> + xpsgtr_write_phy(gtr_phy, L0_TX_ANA_TM_118,
>>> + L0_TX_ANA_TM_118_FORCE_17_0);
>>> +}
>>> +
>>> +/* SATA-specific initialization. */
>>> +static void xpsgtr_phy_init_sata(struct xpsgtr_phy *gtr_phy)
>>> +{
>>> + struct xpsgtr_dev *gtr_dev = gtr_phy->dev;
>>> +
>>> + xpsgtr_bypass_scrambler_8b10b(gtr_phy);
>>> +
>>> + writel(gtr_phy->lane, gtr_dev->siou + SATA_CONTROL_OFFSET);
>>> +}
>>
>> The cover letter mentions this has been tested with DP. Was it tested with SATA
>> and SGMII as well?
>
> On the Xilinx downstream kernel only, as SATA and SGMII isn't available
> upstream yet.
Would prefer to merge only what is tested upstream. Are there plans of getting
SATA and SGMII upstream? Are the patches posted?
>
>>> +
>>> +/* SGMII-specific initialization. */
>>> +static void xpsgtr_phy_init_sgmii(struct xpsgtr_phy *gtr_phy)
>>> +{
>>> + struct xpsgtr_dev *gtr_dev = gtr_phy->dev;
>>> +
>>> + /* Set SGMII protocol TX and RX bus width to 10 bits. */
>>> + xpsgtr_write(gtr_dev, TX_PROT_BUS_WIDTH,
>>> + PROT_BUS_WIDTH_10 << (gtr_phy->lane * PROT_BUS_WIDTH_SHIFT));
>>> + xpsgtr_write(gtr_dev, RX_PROT_BUS_WIDTH,
>>> + PROT_BUS_WIDTH_10 << (gtr_phy->lane * PROT_BUS_WIDTH_SHIFT));
>>> +
>>> + xpsgtr_bypass_scrambler_8b10b(gtr_phy);
>>> +}
>>> +
>>> +/* Configure TX de-emphasis and margining for DP. */
>>> +static void xpsgtr_phy_configure_dp(struct xpsgtr_phy *gtr_phy, unsigned int pre,
>>> + unsigned int voltage)
>>> +{
>>> + static const u8 voltage_swing[4][4] = {
>>> + { 0x2a, 0x27, 0x24, 0x20 },
>>> + { 0x27, 0x23, 0x20, 0xff },
>>> + { 0x24, 0x20, 0xff, 0xff },
>>> + { 0xff, 0xff, 0xff, 0xff }
>>> + };
>>> + static const u8 pre_emphasis[4][4] = {
>>> + { 0x02, 0x02, 0x02, 0x02 },
>>> + { 0x01, 0x01, 0x01, 0xff },
>>> + { 0x00, 0x00, 0xff, 0xff },
>>> + { 0xff, 0xff, 0xff, 0xff }
>>> + };
>>> +
>>> + xpsgtr_write_phy(gtr_phy, L0_TXPMD_TM_48, voltage_swing[pre][voltage]);
>>> + xpsgtr_write_phy(gtr_phy, L0_TX_ANA_TM_18, pre_emphasis[pre][voltage]);
>>> +}
>>> +
>>> +/* -----------------------------------------------------------------------------
>>> + * PHY Operations
>>> + */
>>> +
>>> +static bool xpsgtr_phy_init_required(struct xpsgtr_phy *gtr_phy)
>>> +{
>>> + /*
>>> + * As USB may save the snapshot of the states during hibernation, doing
>>> + * phy_init() will put the USB controller into reset, resulting in the
>>> + * losing of the saved snapshot. So try to avoid phy_init() for USB
>>> + * except when gtr_phy->skip_phy_init is false (this happens when FPD is
>>> + * shutdown during suspend or when gt lane is changed from current one)
>>> + */
>>> + if (gtr_phy->protocol == ICM_PROTOCOL_USB && gtr_phy->skip_phy_init)
>>> + return false;
>>> + else
>>
>> 'else' here is un-necessary.
>
> Does it hurt though ? I find it more readable.
Fine then.
>
>>> + return true;
>>> +}
>>> +
>>> +/*
>>> + * There is a functional issue in the GT. The TX termination resistance can be
>>> + * out of spec due to a issue in the calibration logic. This is the workaround
>>> + * to fix it, required for XCZU9EG silicon.
>>> + */
>>> +static int xpsgtr_phy_tx_term_fix(struct xpsgtr_phy *gtr_phy)
>>> +{
>>> + struct xpsgtr_dev *gtr_dev = gtr_phy->dev;
>>> + u32 timeout = TIMEOUT_US;
>>> + u32 nsw;
>>> +
>>> + /* Enabling Test Mode control for CMN Rest */
>>> + xpsgtr_clr_set(gtr_dev, TM_CMN_RST, TM_CMN_RST_MASK, TM_CMN_RST_SET);
>>
>> This function writes to the global registers. Wouldn't this affect a lane that
>> is already configured?
>
> This function is only executed once, for the first PHY that is
> initialized (see the caller, this is guarded with a mutex, so there's no
> race condition).
Indeed.
Thanks
Kishon