Subject: Re: [PATCH v3 2/3] phy/rockchip: add naneng combo phy for RK3568
From:   Johan Jonker <jbx6244@gmail.com>
To:     Yifeng Zhao <yifeng.zhao@rock-chips.com>, heiko@sntech.de,
        robh+dt@kernel.org
Cc:     devicetree@vger.kernel.org, vkoul@kernel.org,
        michael.riesch@wolfvision.net, linux-rockchip@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-phy@lists.infradead.org, kishon@ti.com,
        p.zabel@pengutronix.de, cl@rock-chips.com
References: <20211025080632.32063-1-yifeng.zhao@rock-chips.com>
 <20211025080632.32063-3-yifeng.zhao@rock-chips.com>
 <65898b49-87ee-211e-f5ef-39147107ef83@gmail.com>
Message-ID: <95ba0a5b-8f5a-3351-fc53-33021b2a0111@gmail.com>
Date:   Tue, 7 Dec 2021 14:51:18 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <65898b49-87ee-211e-f5ef-39147107ef83@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

See question below.

On 11/14/21 12:40 PM, Johan Jonker wrote:
> Hi Yifeng,
> 
> Some comments. Have a look if it's useful.
> 
> On 10/25/21 10:06 AM, Yifeng Zhao wrote:
>> This patch implements a combo phy driver for Rockchip SoCs
>> with NaNeng IP block. This phy can be used as pcie-phy, usb3-phy,
>> sata-phy or sgmii-phy.
>>
>> Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com>
>> ---

>> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c

>> +
>> +#define BIT_WRITEABLE_SHIFT		16
>> +#define REF_CLOCK_24MHz			24000000
>> +#define REF_CLOCK_25MHz			25000000
>> +#define REF_CLOCK_100MHz		100000000
>> +/* RK3568 T22 COMBO PHY REG */
>> +#define RK3568_T22_PHYREG5		0x14
>> +#define T22_PHYREG5_PLL_DIV_MASK	GENMASK(7, 6)
>> +#define T22_PHYREG5_PLL_DIV_SHIFT	6
>> +#define T22_PHYREG5_PLL_DIV_2		1
>> +

>> +#define RK3568_T22_PHYREG6		0x18

Multi-PHY_Register007
Address: Operational Base + offset (0x0018)

>> +#define T22_PHYREG6_TX_RTERM_MASK	GENMASK(7, 4)
>> +#define T22_PHYREG6_TX_RTERM_SHIFT	4
>> +#define T22_PHYREG6_TX_RTERM_50OHM	0x8
>> +#define T22_PHYREG6_RX_RTERM_MASK	GENMASK(3, 0)
>> +#define T22_PHYREG6_RX_RTERM_SHIFT	0
>> +#define T22_PHYREG6_RX_RTERM_44OHM	0xF
>> +
>> +#define RK3568_T22_PHYREG7		0x1C

Multi-PHY_Register008
Address: Operational Base + offset (0x001C)

>> +#define T22_PHYREG7_SSC_EN		BIT(4)

See Rockchip RK3568 TRM Part2 V1.0-20210111 page 1957-1958

It looks as if all PHYREG defines add -1 in value? I'm I correct here?

===

SSC_EN
In U3 and SATA mode, this bit= 1, set SSC on
In PCIE mode, this bit=0 set SSC off

      If Spread Spectrum Clocking (SSC) is used it is
      required that a common reference clock is used by the link partners.
      Most commercially available platforms with PCIe backplanes use
      SSC to reduce EMI.

Maybe a dumb question for in a possible YAML description:
Is this above text from the TRM correct?
Is SSC only used in combination with PCIE instead of U3 and SATA?

===

Johan

>> +
>> +#define RK3568_T22_PHYREG10		0x28
>> +#define T22_PHYREG10_SU_TRIM_0_7	0xF0
>> +
>> +#define RK3568_T22_PHYREG11		0x2C
>> +#define T22_PHYREG11_PLL_LPF_ADJ	0x4
>> +

>> +#define RK3568_T22_PHYREG12		0x30

Multi-PHY_Register013
Address: Operational Base + offset (0x0030)

>> +#define T22_PHYREG12_RESISTER_MASK	GENMASK(5, 4)
>> +#define T22_PHYREG12_RESISTER_SHIFT	0x4
>> +#define T22_PHYREG12_RESISTER_HIGH_Z	0x3
>> +#define T22_PHYREG12_CKRCV_AMP0		BIT(7)
>> +
>> +#define RK3568_T22_PHYREG13		0x34
>> +#define T22_PHYREG13_CKRCV_AMP1		BIT(0)
>> +
>> +#define RK3568_T22_PHYREG14		0x38
>> +#define T22_PHYREG14_CTLE_EN		BIT(0)
>> +#define T22_PHYREG14_SSC_CNT_MASK	GENMASK(7, 6)
>> +#define T22_PHYREG14_SSC_CNT_SHIFT	6
>> +#define T22_PHYREG14_SSC_CNT_VALUE	0x1
>> +
>> +#define RK3568_T22_PHYREG15		0x3C
>> +#define T22_PHYREG15_SSC_CNT_VALUE	0x5f
>> +
>> +#define RK3568_T22_PHYREG17		0x44
>> +#define T22_PHYREG17_PLL_LOOP		0x32
>> +
>> +#define RK3568_T22_PHYREG31		0x7C
>> +#define T22_PHYREG31_SSC_MASK		GENMASK(7, 4)
>> +#define T22_PHYREG31_SSC_DIR_SHIFT	4
>> +#define T22_PHYREG31_SSC_UPWARD		0
>> +#define T22_PHYREG31_SSC_DOWNWARD	1
>> +#define T22_PHYREG31_SSC_OFFSET_SHIFT	6
>> +#define T22_PHYREG31_SSC_OFFSET_500PPM	1
>> +
>> +#define RK3568_T22_PHYREG32		0x80
>> +#define T22_PHYREG32_PLL_KVCO_MASK	GENMASK(4, 2)
>> +#define T22_PHYREG32_PLL_KVCO_SHIFT	2
>> +#define T22_PHYREG32_PLL_KVCO_VALUE	2
>> +