Message-ID: <CAMdYzYpTU2YfNJTa3+sRe5hWgnR3pGOb_kmK_P85Ea_w2P9cRQ@mail.gmail.com>
Date: Thu, 16 Jan 2025 10:57:54 -0500
From: Peter Geis <pgwipeout@...il.com>
To: Diederik de Haas <didi.debian@...ow.org>
Cc: Heiko Stuebner <heiko@...ech.de>, Kishon Vijay Abraham I <kishon@...nel.org>, 
	Algea Cao <algea.cao@...k-chips.com>, linux-kernel@...r.kernel.org, 
	Philipp Zabel <p.zabel@...gutronix.de>, Arnd Bergmann <arnd@...db.de>, frank.wang@...k-chips.com, 
	william.wu@...k-chips.com, kever.yang@...k-chips.com, 
	linux-phy@...ts.infradead.org, 
	Sebastian Reichel <sebastian.reichel@...labora.com>, linux-rockchip@...ts.infradead.org, 
	Vinod Koul <vkoul@...nel.org>, Zhang Yubing <yubing.zhang@...k-chips.com>, zyw@...k-chips.com, 
	wulf@...k-chips.com, linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH v1 3/6] phy: rockchip: add driver for rk3328 usb3 phy

On Thu, Jan 16, 2025 at 10:27 AM Diederik de Haas <didi.debian@...ow.org> wrote:
>
> 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@...il.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@...il.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?

According to the rk3228h trm, that's accurate. I'm hoping someone with
access to Innosilicon's IP can confirm the registers I've found and
provide insight to the rest.

>
> Cheers,
>   Diederik

Powered by blists - more mailing lists