RE: [PATCH v4 2/2] phy: zynqmp: Add phy driver for xilinx zynqmp phy core

[Anurag Kumar Vulisha <anuragku@xilinx.com>]

Hi Kishon,

>-----Original Message-----
>From: Kishon Vijay Abraham I [mailto:kishon@ti.com]
>Sent: Tuesday, October 09, 2018 1:18 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; robh+dt@kernel.org;
>mark.rutland@arm.com; Michal Simek <michals@xilinx.com>;
>vivek.gautam@codeaurora.org
>Cc: v.anuragkumar@gmail.com; Ajay Yugalkishore Pandey <APANDEY@xilinx.com>;
>devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>kernel@vger.kernel.org
>Subject: Re: [PATCH v4 2/2] phy: zynqmp: Add phy driver for xilinx zynqmp phy core
>
>Hi Anurag,
>
>On Wednesday 12 September 2018 09:52 PM, Anurag Kumar Vulisha wrote:
>> ZynqMP SoC has 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@xilinx.com>
>> ---
>> Changes in v4:
>> 	1. Moved include/dt-bindings/phy/phy.h into patch 1 as suggested by
>> 	   "Rob Herring"
>>
>> Changes in v3:
>> 	1. Corrected the Documentation as suggested by "Vivek Gautam"
>>
>> Changes in v2:
>> 	1. Fixed the compilation error when compiled phy-zynqmp.c as a module
>> 	2. Added CONFIG_PM macro in phy-zynqmp.c driver
>> ---
>>  drivers/phy/Kconfig            |    8 +
>>  drivers/phy/Makefile           |    1 +
>>  drivers/phy/phy-zynqmp.c       | 1581
>++++++++++++++++++++++++++++++++++++++++
>>  include/linux/phy/phy-zynqmp.h |   52 ++
>>  4 files changed, 1642 insertions(+)
>>  create mode 100644 drivers/phy/phy-zynqmp.c
>>  create mode 100644 include/linux/phy/phy-zynqmp.h
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 5c8d452..14cf3330 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -40,6 +40,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 84e3bd9..f2a8d27 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_GENERIC_PHY)		+= phy-core.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_LANTIQ)			+= lantiq/
>> diff --git a/drivers/phy/phy-zynqmp.c b/drivers/phy/phy-zynqmp.c
>> new file mode 100644
>> index 0000000..497862d
>> --- /dev/null
>> +++ b/drivers/phy/phy-zynqmp.c
>> @@ -0,0 +1,1581 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * phy-zynqmp.c - PHY driver for Xilinx ZynqMP GT.
>> + *
>> + * Copyright (C) 2018 Xilinx Inc.
>> + *
>> + * Author: Anurag Kumar Vulisha <anuragku@xilinx.com>
>> + *
>> + * 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/of_platform.h>
>> +#include <linux/of_address.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/phy/phy-zynqmp.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/delay.h>
>> +#include <dt-bindings/phy/phy.h>
>> +#include <linux/reset.h>
>> +#include <linux/list.h>
>> +#include <linux/slab.h>
>> +
>> +/* 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
>> +
>> +/* Refclk selection parameters */
>> +#define PLL_REF_SEL0			0x10000
>> +#define PLL_REF_OFFSET			0x4
>> +#define PLL_FREQ_MASK			0x1F
>> +#define PLL_STATUS_READ_OFFSET		0x4000
>> +#define PLL_STATUS_LOCKED		0x10
>> +
>> +/* PLL SSC step size offsets */
>> +#define L0_L0_REF_CLK_SEL		0x2860
>> +#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_OFFSET		0x4000
>> +#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_OFFSET			0x4000
>> +#define STEPS_0_MASK			0xFF
>> +#define STEPS_1_MASK			0x07
>> +
>> +/* BG calibration parameters */
>> +#define BGCAL_REF_SEL			0x10028
>> +#define BGCAL_REF_VALUE			0x0C
>> +
>> +/* 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
>> +
>> +/* DN Resistor calibration code parameters */
>> +#define L0_TXPMA_ST_3			0x0B0C
>> +#define L0_DN_CALIB_CODE		0x3F
>> +
>> +/* PLL Test Mode register parameters */
>> +#define L0_TM_PLL_DIG_37		0x2094
>> +#define L0_TM_PLL_DIG_37_OFFSET		0x4000
>> +#define L0_TM_COARSE_CODE_LIMIT		0x10
>> +
>> +/* PCS control parameters */
>> +#define L0_TM_DIG_6			0x106C
>> +#define L0_TX_DIG_61			0x00F4
>> +#define L0_TM_DIG_6_OFFSET		0x4000
>> +#define L0_TX_DIG_61_OFFSET		0x4000
>> +#define L0_TM_DIS_DESCRAMBLE_DECODER	0x0F
>> +#define L0_TM_DISABLE_SCRAMBLE_ENCODER	0x0F
>> +
>> +/* TX De-emphasis parameters */
>> +#define L0_TX_ANA_TM_18			0x0048
>> +#define L0_TX_ANA_TM_118		0x01D8
>> +#define L0_TX_ANA_TM_18_OFFSET		0x4000
>> +#define L0_TX_ANA_TM_118_OFFSET		0x4000
>> +#define L0_TX_ANA_TM_118_FORCE_17_0	BIT(0)
>> +
>> +/* PMA control parameters */
>> +#define L0_TXPMD_TM_45			0x0CB4
>> +#define L0_TXPMD_TM_48			0x0CC0
>> +#define L0_TXPMD_TM_45_OFFSET		0x4000
>> +#define L0_TXPMD_TM_48_OFFSET		0x4000
>> +#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)
>> +
>> +/* 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
>> +
>> +/* Max number of GT lanes */
>> +#define MAX_LANES			4
>> +
>> +/* Max Allowed refclk frequencies */
>> +#define MAX_REFCLK			13
>> +
>> +/* Lane CLK sharing mask */
>> +#define LANE_CLK_SHARE_MASK		0x8F
>> +
>> +/* SIOU SATA control register */
>> +#define SATA_CONTROL_OFFSET		0x0100
>> +
>> +/* Total number of controllers */
>> +#define CONTROLLERS_PER_LANE		5
>> +
>> +/* USB pipe control parameters */
>> +#define PIPE_CLK_OFFSET			0x7c
>> +#define PIPE_POWER_OFFSET		0x80
>> +#define PIPE_CLK_ON			1
>> +#define PIPE_CLK_OFF			0
>> +#define PIPE_POWER_ON			1
>> +#define PIPE_POWER_OFF			0
>> +
>> +/* Protocol Type Pparameters */
>> +#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 RST_TIMEOUT_MS			1000
>> +#define TIMEOUT_US			1000
>> +
>> +/*
>> + * This table holds the valid combinations of controllers and
>> + * lanes(Interconnect Matrix).
>> + */
>> +static unsigned int icm_matrix[MAX_LANES][CONTROLLERS_PER_LANE] = {
>> +	{ XPSGTR_TYPE_PCIE_0, XPSGTR_TYPE_SATA_0, XPSGTR_TYPE_USB0,
>> +		XPSGTR_TYPE_DP_1, XPSGTR_TYPE_SGMII0 },
>> +	{ XPSGTR_TYPE_PCIE_1, XPSGTR_TYPE_SATA_1, XPSGTR_TYPE_USB0,
>> +		XPSGTR_TYPE_DP_0, XPSGTR_TYPE_SGMII1 },
>> +	{ XPSGTR_TYPE_PCIE_2, XPSGTR_TYPE_SATA_0, XPSGTR_TYPE_USB0,
>> +		XPSGTR_TYPE_DP_1, XPSGTR_TYPE_SGMII2 },
>> +	{ XPSGTR_TYPE_PCIE_3, XPSGTR_TYPE_SATA_1, XPSGTR_TYPE_USB1,
>> +		XPSGTR_TYPE_DP_0, XPSGTR_TYPE_SGMII3 }
>> +};
>> +
>> +/* Allowed PLL reference clock frequencies */
>> +enum pll_frequencies {
>> +	REF_19_2M = 0,
>> +	REF_20M,
>> +	REF_24M,
>> +	REF_26M,
>> +	REF_27M,
>> +	REF_38_4M,
>> +	REF_40M,
>> +	REF_52M,
>> +	REF_100M,
>> +	REF_108M,
>> +	REF_125M,
>> +	REF_135M,
>> +	REF_150M,
>> +};
>> +
>> +/**
>> + * 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
>> + * @ref_clk: enum of allowed ref clock rates for this lane PLL
>> + * @pll_lock: PLL status
>> + * @skip_phy_init: skip phy_init() if true
>> + * @data: pointer to hold private data
>> + * @refclk_rate: PLL reference clock frequency
>> + * @share_laneclk: lane number of the clock to be shared
>> + */
>> +struct xpsgtr_phy {
>> +	struct phy *phy;
>> +	u8 type;
>> +	u8 lane;
>> +	u8 protocol;
>> +	enum pll_frequencies ref_clk;
>> +	bool pll_lock;
>> +	bool skip_phy_init;
>> +	void *data;
>> +	u32 refclk_rate;
>> +	u32 share_laneclk;
>> +};
>> +
>> +/**
>> + * 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;
>> +};
>> +
>> +/* lookup table to hold all settings needed for a ref clock frequency */
>> +static struct xpsgtr_ssc ssc_lookup[MAX_REFCLK] = {
>> +	{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}
>> +};
>> +
>> +/**
>> + * 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: pointer to all the lanes
>> + * @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
>> + * @sata_rst: a reset control for SATA
>> + * @dp_rst: a reset control for DP
>> + * @usb0_crst: a reset control for usb0 core
>> + * @usb1_crst: a reset control for usb1 core
>> + * @usb0_hibrst: a reset control for usb0 hibernation module
>> + * @usb1_hibrst: a reset control for usb1 hibernation module
>> + * @usb0_apbrst: a reset control for usb0 apb bus
>> + * @usb1_apbrst: a reset control for usb1 apb bus
>> + * @gem0_rst: a reset control for gem0
>> + * @gem1_rst: a reset control for gem1
>> + * @gem2_rst: a reset control for gem2
>> + * @gem3_rst: a reset control for gem3
>> + */
>> +struct xpsgtr_dev {
>> +	struct device *dev;
>> +	void __iomem *serdes;
>> +	void __iomem *siou;
>> +	struct mutex gtr_mutex; /* mutex for locking */
>> +	struct xpsgtr_phy **phys;
>> +	bool tx_term_fix;
>> +	unsigned int saved_icm_cfg0;
>> +	unsigned int saved_icm_cfg1;
>> +	struct reset_control *sata_rst;
>> +	struct reset_control *dp_rst;
>> +	struct reset_control *usb0_crst;
>> +	struct reset_control *usb1_crst;
>> +	struct reset_control *usb0_hibrst;
>> +	struct reset_control *usb1_hibrst;
>> +	struct reset_control *usb0_apbrst;
>> +	struct reset_control *usb1_apbrst;
>> +	struct reset_control *gem0_rst;
>> +	struct reset_control *gem1_rst;
>> +	struct reset_control *gem2_rst;
>> +	struct reset_control *gem3_rst;
>> +};
>> +
>> +/**
>> + * xpsgtr_override_deemph - override PIPE TX de-emphasis
>> + * @phy: pointer to phy
>> + * @plvl: pre-emphasis level
>> + * @vlvl: voltage swing level
>> + *
>> + * Return: None
>> + */
>> +void xpsgtr_override_deemph(struct phy *phy, u8 plvl, u8 vlvl)
>> +{
>> +	struct xpsgtr_phy *gtr_phy = phy_get_drvdata(phy);
>> +	struct xpsgtr_dev *gtr_dev = gtr_phy->data;
>> +	static u8 pe[4][4] = { { 0x2, 0x2, 0x2, 0x2 },
>> +			       { 0x1, 0x1, 0x1, 0xFF },
>> +			       { 0x0, 0x0, 0xFF, 0xFF },
>> +			       { 0xFF, 0xFF, 0xFF, 0xFF } };
>
>Here the consumer driver directly passes the index of a local array as
>argument. How is the consumer driver supposed to this index. What happens if
>the same consumer is connected to a different PHY?
>

Thanks a lot for spending your time in reviewing this patch. The consumer of this
particular function  is a Display Port and it is Xilinx specific IP. I am not very sure of
the information if any other Display Port drivers need to handle this similar kind of
runtime adjustment of the de-emphasis or voltage swing values, but the Xilinx DP
has to do it at runtime as a part of training sequence. You may consider this as an
implementation requirement. The Xilinx DP is expected to work only with this zynqmp
phy and these plvl & vlvl values are predefined and there is an one to one mapping of
these values between Xilinx DP driver and phy-zynqmp driver. These values are calculated
by the DP driver based on the DPCD data read from sink and passed as an arguments
to xpsgtr_override_deemph() function. Hope this answers your query.
Please correct me, if I am missing something or my understanding is wrong 

Thanks,
Anurag Kumar Vulisha