Re: [RFC PATCH v1 3/6] phy: rockchip: add driver for rk3328 usb3 phy

Diederik de Haas
2025-01-16T15:27:18Z

[-- Attachment #1.1: Type: text/plain, Size: 11596 bytes --]

Hi Peter,

Thanks for working on this :-)

On Wed Jan 15, 2025 at 2:26 AM CET, Peter Geis wrote:
> The rk3328 has a combined usb2 and usb3 phy block for the usb3 dwc
> interface. The implementation up until now has been bugged, with
> multiple issues ranging from disconnect detection failures to low
> performance. This driver fixes the majority of the original issues and
> allows better performance for the rk3328 usb3 port.
>
> This driver sources data from multiple sources, including the rk3328
> trm, the rk3228h trm, emails from Rockchip, and both the 4.4 and 5.10
> downstream drivers. The current implementation allows for basic bring up
> of the phy block and fixes the detection issues. Interrupts are enabled,
> but currently only used for debugging. Features missing currently are
> power management, OTG handling, board specific tuning, regulator control,
>
> Currently the only known bugs are a AX88179 usb3 gigabit adapter crashes
> when operating at usb3 speeds and transmitting large amounts of data and
> full disconnection detections may be slower than expected (~5-10 seconds).
>
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
>
> drivers/phy/rockchip/Kconfig | 10 +
> drivers/phy/rockchip/Makefile | 1 +
> drivers/phy/rockchip/phy-rockchip-inno-usb3.c | 869 ++++++++++++++++++
> 3 files changed, 880 insertions(+)
> create mode 100644 drivers/phy/rockchip/phy-rockchip-inno-usb3.c
>
> diff --git a/drivers/phy/rockchip/Kconfig b/drivers/phy/rockchip/Kconfig
> index 2f7a05f21dc5..aac623e84f96 100644
> --- a/drivers/phy/rockchip/Kconfig
> +++ b/drivers/phy/rockchip/Kconfig
> @@ -48,6 +48,16 @@ config PHY_ROCKCHIP_INNO_USB2
> help
> Support for Rockchip USB2.0 PHY with Innosilicon IP block.
>
> +config PHY_ROCKCHIP_INNO_USB3
> + tristate "Rockchip INNO USB3PHY Driver"
> + depends on (ARCH_ROCKCHIP || COMPILE_TEST) && OF
> + depends on COMMON_CLK
> + depends on USB_SUPPORT
> + select GENERIC_PHY
> + select USB_COMMON
> + help
> + Support for Rockchip USB3.0 PHY with Innosilicon IP block.
> +
> config PHY_ROCKCHIP_INNO_CSIDPHY
> tristate "Rockchip Innosilicon MIPI CSI PHY driver"
> depends on (ARCH_ROCKCHIP || COMPILE_TEST) && OF
> diff --git a/drivers/phy/rockchip/Makefile b/drivers/phy/rockchip/Makefile
> index 010a824e32ce..ec15dcf37fba 100644
> --- a/drivers/phy/rockchip/Makefile
> +++ b/drivers/phy/rockchip/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_PHY_ROCKCHIP_INNO_CSIDPHY) += phy-rockchip-inno-csidphy.o
> obj-$(CONFIG_PHY_ROCKCHIP_INNO_DSIDPHY) += phy-rockchip-inno-dsidphy.o
> obj-$(CONFIG_PHY_ROCKCHIP_INNO_HDMI) += phy-rockchip-inno-hdmi.o
> obj-$(CONFIG_PHY_ROCKCHIP_INNO_USB2) += phy-rockchip-inno-usb2.o
> +obj-$(CONFIG_PHY_ROCKCHIP_INNO_USB3) += phy-rockchip-inno-usb3.o
> obj-$(CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY) += phy-rockchip-naneng-combphy.o
> obj-$(CONFIG_PHY_ROCKCHIP_PCIE) += phy-rockchip-pcie.o
> obj-$(CONFIG_PHY_ROCKCHIP_SAMSUNG_HDPTX) += phy-rockchip-samsung-hdptx.o
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb3.c b/drivers/phy/rockchip/phy-rockchip-inno-usb3.c
> new file mode 100644
> index 000000000000..51b9f3b7fbfa
> --- /dev/null
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb3.c
> @@ -0,0 +1,869 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/*
> + * phy-rockchip-inno-usb3.c - USB3 PHY based on Innosilicon IP as
> + * implemented on Rockchip rk3328. Tuning data magic bits are taken as is
> + * from the downstream driver. Downstream driver is located at:
> + * https://github.com/rockchip-linux/kernel/blob/240a5660d7c23841ccf7b7cc489078bf521b9802/drivers/phy/rockchip/phy-rockchip-inno-usb3.c
> + *
> + * Author: Peter Geis <pgwipeout@gmail.com>
> + * TODO:
> + * - Find the rest of the register names / definitions.
> + * - Implement pm functions.
> + * - Implement board specific tuning from dts.
> + * - Implement regulator control.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/usb/of.h>
> +
> +#define REG_WRITE_MASK GENMASK(31, 16)
> +#define REG_WRITE_SHIFT 16
> +#define DISABLE_BITS 0x0
> +
> +/* USB3PHY GRF Registers */
> +#define USB3PHY_WAKEUP_CON_REG 0x40
> +#define USB3PHY_WAKEUP_STAT_REG 0x44
> +#define USB3_LINESTATE_IRQ_EN BIT(0)
> +#define USB3_RXDET_IRQ_EN BIT(1)
> +#define USB3_BVALID_RISE_IRQ_EN BIT(2)
> +#define USB3_BVALID_FALL_IRQ_EN BIT(3)
> +#define USB3_BVALID_CLEAR_MASK GENMASK(3, 2)
> +#define USB3_ID_RISE_IRQ_EN BIT(4)
> +#define USB3_ID_FALL_IRQ_EN BIT(5)
> +#define USB3_ID_CLEAR_MASK GENMASK(5, 4)
> +#define USB3_RXDET_EN BIT(6)
> +
> +/* PIPE registers */
> +/* 0x08 for SSC, default 0x0e */
> +#define UNKNOWN_PIPE_REG_000 0x000
> +#define UNKNOWN_SSC_000_MASK GENMASK(2, 1)
> +#define UNKNOWN_SSC_000_ENABLE (0x00 << 1)
> +
> +/* 0x83 for 24m, 0x01 for 25m, default 0x86 */
> +#define PIPE_REG_020 0x020
> +/* RX CDR multiplier high bits [7:6], as P, default 0x2, RX data rate = (2*refclk*P)/Q */
> +#define PIPE_RX_CDR_MULT_HIGH_MASK GENMASK(7, 6)
> +/* TX PLL divider bits [4:0], as N, default 0x6, TX data rate = (2*refclk*M)/N */
> +#define PIPE_TX_PLL_DIV_MASK GENMASK(4, 0)
> +
> +/* 0x71 for 24m, 0x64 for 25m, default 0x71 */
> +#define PIPE_REG_028 0x028
> +/* RX CDR multiplier low bits [7:0], as P, default 0x71, RX data rate = (2*refclk*P)/Q */
> +#define PIPE_RX_CDR_MULT_LOW_MASK GENMASK(7, 0)
> +
> +/* 0x26 for 24m?, 0x21 for 25m, default 0x26 */
> +#define PIPE_REG_030 0x030
> +/* RX CDR divider bits [4:0], as Q, default 0x6, RX data rate = (2*refclk*P)/Q */
> +#define PIPE_RX_CDR_DIV_MASK GENMASK(4, 0)
> +
> +/* 1'b1 Disable bandgap power, default 0x00 */
> +#define PIPE_REG_044 0x044
> +#define BANDGAP_POWER_DISABLE BIT(4)
> +
> +/* 0xe0 for rx tune?, default 0xe1 */
> +#define PIPE_REG_060 0x060
> +#define PIPE_TX_DETECT_BYPASS_DEBUG BIT(4) /* enable to always force detection */
> +/* RX CTLE frequency bandwidth response tuning bits [1:0], default 0x1 */
> +#define PIPE_RX_CTLE_FREQ_BW_MASK GENMASK(1, 0)
> +#define PIPE_RX_CTLE_FREQ_BW_TUNE 0x0
> +
> +/* default 0x49 */
> +#define PIPE_REG_064 0x064
> +/* RX equalizer tail current control bits [6:4], default 0x4 */
> +#define PIPE_RX_EQ_TAIL_CURR_MASK GENMASK(6, 4)
> +
> +/* 0x08 for rx tune?, default 0x07 */
> +#define PIPE_REG_068 0x068
> +/* RX equalizer low frequency gain control bits [7:4], default 0x0 */
> +#define PIPE_RX_EQ_LOW_GAIN_MASK GENMASK(7, 4)
> +#define PIPE_RX_EQ_LOW_GAIN_TUNE (0x1 << 4)
> +/* RX CTLE gain tuning bits [3:0], higher = more gain default 0x7 */
> +#define PIPE_RX_CTLE_GAIN_MASK GENMASK(3, 0)
> +#define PIPE_RX_CTLE_GAIN_TUNE 0x7 /* 0x5 lowest functional value, 0xf highest */
> +
> +/* RX ODT manual resistance config, higher = less resistance, depends on REG_1C4 BIT(5) set */
> +#define PIPE_REG_06C 0x06c
> +/* RX ODT manual resistance high bits [3:0], default 0x0 */
> +#define PIPE_RX_ODT_RES_HIGH_MASK GENMASK(3, 0)
> +#define PIPE_RX_ODT_RES_HIGH_TUNE 0xf
> +
> +#define PIPE_REG_070 0x070
> +/* RX ODT manual resistance mid bits [7:0], default 0x03 */
> +#define PIPE_RX_ODT_RES_MID_MASK GENMASK(7, 0)
> +#define PIPE_RX_ODT_RES_MID_TUNE 0xff
> +
> +#define PIPE_REG_074 0x074
> +/* RX ODT manual resistance low bits [7:0], default 0xff */
> +#define PIPE_RX_ODT_RES_LOW_MASK GENMASK(7, 0)
> +#define PIPE_RX_ODT_RES_LOW_TUNE 0xff
> +
> +/* default 0x08 */
> +#define PIPE_REG_080 0x080
> +#define PIPE_TX_COMMON_MODE_DIS BIT(2) /* 1'b1 disable TX common type */
> +
> +/* default 0x33 */
> +#define PIPE_REG_088 0x088
> +#define PIPE_TX_DRIVER_PREEMP_EN BIT(4) /* 1'b1 enable pre-emphasis */
> +
> +/* default 0x18 */
> +#define PIPE_REG_0C0 0x0c0
> +#define PIPE_RX_CM_EN BIT(3) /* 1'b1 enable RX CM */
> +#define PIPE_TX_OBS_EN BIT(4) /* 1'b1 enable TX OBS */
> +
> +/* 0x12 for rx tune?, default 0x14 */
> +#define PIPE_REG_0C8 0x0c8
> +/* RX CDR charge pump current bits [3:1], default 0x2 */
> +#define PIPE_RX_CDR_CHG_PUMP_MASK GENMASK(3, 1)
> +#define PIPE_RX_CDR_CHG_PUMP_TUNE (0x2 << 1)
> +
> +/* 0x02 for 24m, 0x06 for 25m, default 0x06 */
> +#define UNKNOWN_PIPE_REG_108 0x108
> +#define UNKNOWN_REFCLK_108_24M 0x02
> +
> +/* 0x80 for 24m, default 0x00 */
> +#define UNKNOWN_PIPE_REG_10C 0x10c
> +#define UNKNOWN_REFCLK_10C_24M BIT(7)
> +
> +/* 0x01 for 24m, 0x00 for 25m, default 0x02 */
> +#define PIPE_REG_118 0x118
> +/* TX PLL multiplier high bits [3:0], as M, default 0x2, TX data rate = (2*refclk*M)/N */
> +#define PIPE_TX_PLL_MUL_HIGH_MASK GENMASK(3, 0)
> +
> +/* 0x38 for 24m, 0x64 for 25m, default 0x71 */
> +#define PIPE_REG_11C 0x11c
> +/* TX PLL multiplier low bits [7:0], as M, default 0x71, TX data rate = (2*refclk*M)/N */
> +#define PIPE_TX_PLL_MUL_LOW_MASK GENMASK(7, 0)
> +
> +/* 0x0c for SSC, default 0x1c */
> +#define UNKNOWN_PIPE_REG_120 0x120
> +#define UNKNOWN_SSC_120_MASK BIT(4)
> +#define UNKNOWN_SSC_120_ENABLE (0x0 << 4)
> +
> +/* default 0x40 */
> +#define PIPE_REG_12C 0x12c
> +#define PIPE_TX_PLL_ALWAYS_ON BIT(0) /* disable for PLL control by pipe_pd */
> +
> +/* 0x05 for rx tune, default 0x01 */
> +#define PIPE_REG_148 0x148
> +#define PIPE_RX_CHG_PUMP_DIV_2 BIT(2) /* RX CDR charge pump div/2, default 0 */
> +
> +/* 0x70 for rx tune, default 0x72 */
> +#define PIPE_REG_150 0x150
> +#define PIPE_TX_BIAS_EN BIT(6) /* 1'b1 Enable TX Bias */
> +/* RX CDR phase tracking speed bits [3:0], default 0x2 */
> +#define PIPE_RX_CDR_SPEED_MASK GENMASK(3, 0)
> +#define PIPE_RX_CDR_SPEED_TUNE 0x00
> +
> +/* default 0xd4 */
> +#define PIPE_REG_160
> +/* RX common mode voltage strength bits [5:4], default 0x1 */
> +#define PIPE_RX_CDR_COM_VOLT_MASK GENMASK(5, 4)
> +#define PIPE_RX_CDR_COM_VOLT_TUNE (0x1 << 4)
> +
> +/* default 0x00 */
> +#define PIPE_REG_180 0x180
> +/* TX driver bias reference voltage bits [3:2], in mv */
> +#define PIPE_TX_BIAS_REF_VOLT_MASK GENMASK(3, 2)
> +#define PIPE_TX_BIAS_REF_VOLT_200 (0x0 << 2)
> +#define PIPE_TX_BIAS_REF_VOLT_175 (0x1 << 2)
> +#define PIPE_TX_BIAS_REF_VOLT_225 (0x2 << 2) /* downstream 5.10 driver setting */
> +#define PIPE_TX_BIAS_REF_VOLT_250 (0x3 << 2)
> +
> +/* default 0x01 */
> +#define PIPE_REG_1A8 0x1a8
> +#define PIPE_LDO_POWER_DIS BIT(4) /* 1'b1 Disable LDO Power */
> +
> +/* default 0x07 */
> +#define PIPE_REG_1AC 0x1ac
> +/* TX driver output common voltage bits [5:4], in mv */
> +#define PIPE_TX_COMMON_VOLT_MASK GENMASK(5, 4)
> +#define PIPE_TX_COMMON_VOLT_800 (0x0 << 4)
> +#define PIPE_TX_COMMON_VOLT_750 (0x1 << 4)
> +#define PIPE_TX_COMMON_VOLT_950 (0x2 << 4)
> +#define PIPE_TX_COMMON_VOLT_1100 (0x3 << 4)

Just a question:
In this list as well as the one above it (PIPE_TX_BIAS_xyz) I noticed
the values are not sequential (800, 750, 950, 1100) and (200, 175, 225,
250). Is that correct?

Cheers,
Diederik

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]