Re: [PATCH net-next v1 2/3] net: phy: Add dts support for Motorcomm yt8521/yt8531s gigabit ethernet phy

From: Frank.Sae
Date: Fri Jan 06 2023 - 02:42:16 EST

Hi Andrew,

On 2023/1/6 01:03, Andrew Lunn wrote:
> On Thu, Jan 05, 2023 at 03:30:23PM +0800, Frank wrote:
>> Add dts support for yt8521 and yt8531s. This patch has
>> been tested on AM335x platform which has one YT8531S interface
>> card and passed all test cases.
>>
>> Signed-off-by: Frank <Frank.Sae@xxxxxxxxxxxxxx>
>> ---
>> drivers/net/phy/motorcomm.c | 517 ++++++++++++++++++++++++++++++------
>> 1 file changed, 434 insertions(+), 83 deletions(-)
>>
>> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
>> index 685190db72de..7ebcca374a67 100644
>> --- a/drivers/net/phy/motorcomm.c
>> +++ b/drivers/net/phy/motorcomm.c
>> @@ -10,10 +10,11 @@
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/phy.h>
>> +#include <linux/of.h>
>>
>> #define PHY_ID_YT8511 0x0000010a
>> -#define PHY_ID_YT8521 0x0000011A
>> -#define PHY_ID_YT8531S 0x4F51E91A
>> +#define PHY_ID_YT8521 0x0000011a
>> +#define PHY_ID_YT8531S 0x4f51e91a
>
> Please do the lower case conversion as a separate patch.
>

I will fix.

>>
>> /* YT8521/YT8531S Register Overview
>> * UTP Register space | FIBER Register space
>> @@ -144,6 +145,16 @@
>> #define YT8521_ESC1R_SLEEP_SW BIT(15)
>> #define YT8521_ESC1R_PLLON_SLP BIT(14)
>>
>> +/* Phy Serdes analog cfg2 Register */
>> +#define YTPHY_SERDES_ANALOG_CFG2_REG 0xA1
>> +#define YTPHY_SAC2R_TX_AMPLITUDE_MASK ((0x7 << 13) | (0x7 << 1))
>> +#define YT8521_SAC2R_TX_AMPLITUDE_LOW ((0x7 << 13) | (0x0 << 1))
>> +#define YT8521_SAC2R_TX_AMPLITUDE_MIDDLE ((0x5 << 13) | (0x5 << 1))
>> +#define YT8521_SAC2R_TX_AMPLITUDE_HIGH ((0x3 << 13) | (0x6 << 1))
>
> So there are two values which control the amplitude? Buts 1-3, and bit
> 7-9? Can they be used independently? Also, 7, 5, 3 is also add. Does
> bit 0 of this value have some special meaning? Please document this
> fully.
>

There are three values which control the amplitude, 13-15,3 and 1-2.
They be used independently. Maybe it is good to expose three type ( 0:
low; 1: middle; 2: high)to dts.

Bit Symbol Access default Description
15:13 Tx_swing_sel RW 0x0 TX driver stage2 amplitude control
12 Tx_driver_stg1 RW 0x1 TX driver stage1 amplitude control
11 Reserved RO 0x0 Reserved
10:8 Tx_ckdiv10_con RW 0x0 tx divide by 10 clock delay control
7:4 Reserved RO 0x0 Reserved
3 Tx_post_stg1 RW 0x0 TX driver post stage1 amplitude control
2:1 Tx_de_sel RW 0x0 TX driver post stage2 amplitude control
0 Tx_pd RW 0x0 power down analog tx


>> +#define YT8531S_SAC2R_TX_AMPLITUDE_LOW ((0x0 << 13) | (0x0 << 1))
>> +#define YT8531S_SAC2R_TX_AMPLITUDE_MIDDLE ((0x0 << 13) | (0x1 << 1))
>> +#define YT8531S_SAC2R_TX_AMPLITUDE_HIGH ((0x0 << 13) | (0x2 << 1))
>
> This more sense, but why the 0 << 13? What do the bits 13-? mean?

0 << 13 means 13-15 bit all zero.
bits 13- mans the start bit is 13.

>
>> +
>> /* Phy fiber Link timer cfg2 Register */
>> #define YT8521_LINK_TIMER_CFG2_REG 0xA5
>> #define YT8521_LTCR_EN_AUTOSEN BIT(15)
>> @@ -161,6 +172,7 @@
>>
>> #define YT8521_CHIP_CONFIG_REG 0xA001
>> #define YT8521_CCR_SW_RST BIT(15)
>> +#define YT8521_CCR_RXC_DLY_EN BIT(8)
>>
>> #define YT8521_CCR_MODE_SEL_MASK (BIT(2) | BIT(1) | BIT(0))
>> #define YT8521_CCR_MODE_UTP_TO_RGMII 0
>> @@ -178,22 +190,27 @@
>> #define YT8521_MODE_POLL 0x3
>>
>> #define YT8521_RGMII_CONFIG1_REG 0xA003
>> -
>> +#define YT8521_RC1R_TX_CLK_SEL_MASK BIT(14)
>> +#define YT8521_RC1R_TX_CLK_SEL_ORIGINAL (0x0 << 14)
>> +#define YT8521_RC1R_TX_CLK_SEL_INVERTED (0x1 << 14)
>
> Please use the BIT macro.
>

ok, change

+#define YT8521_RC1R_TX_CLK_SEL_MASK BIT(14)
+#define YT8521_RC1R_TX_CLK_SEL_ORIGINAL (0x0 << 14)
+#define YT8521_RC1R_TX_CLK_SEL_INVERTED (0x1 << 14)

to

+#define YT8521_RC1R_TX_CLK_SEL_INVERTED BIT(14)

?

>
>> /* TX Gig-E Delay is bits 3:0, default 0x1
>> * TX Fast-E Delay is bits 7:4, default 0xf
>> * RX Delay is bits 13:10, default 0x0
>> * Delay = 150ps * N
>> * On = 2250ps, off = 0ps
>> */
>> -#define YT8521_RC1R_RX_DELAY_MASK (0xF << 10)
>> -#define YT8521_RC1R_RX_DELAY_EN (0xF << 10)
>> -#define YT8521_RC1R_RX_DELAY_DIS (0x0 << 10)
>> -#define YT8521_RC1R_FE_TX_DELAY_MASK (0xF << 4)
>> -#define YT8521_RC1R_FE_TX_DELAY_EN (0xF << 4)
>> -#define YT8521_RC1R_FE_TX_DELAY_DIS (0x0 << 4)
>> -#define YT8521_RC1R_GE_TX_DELAY_MASK (0xF << 0)
>> -#define YT8521_RC1R_GE_TX_DELAY_EN (0xF << 0)
>> -#define YT8521_RC1R_GE_TX_DELAY_DIS (0x0 << 0)
>> +#define YT8521_RC1R_GE_TX_DELAY_BIT (0)
>> +#define YT8521_RC1R_FE_TX_DELAY_BIT (4)
>> +#define YT8521_RC1R_RX_DELAY_BIT (10)
>> +#define YT8521_RC1R_RX_DELAY_MASK (0xF << YT8521_RC1R_RX_DELAY_BIT)
>> +#define YT8521_RC1R_RX_DELAY_EN (0xF << YT8521_RC1R_RX_DELAY_BIT)
>> +#define YT8521_RC1R_RX_DELAY_DIS (0x0 << YT8521_RC1R_RX_DELAY_BIT)
>> +#define YT8521_RC1R_FE_TX_DELAY_MASK (0xF << YT8521_RC1R_FE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_FE_TX_DELAY_EN (0xF << YT8521_RC1R_FE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_FE_TX_DELAY_DIS (0x0 << YT8521_RC1R_FE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_GE_TX_DELAY_MASK (0xF << YT8521_RC1R_GE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_GE_TX_DELAY_EN (0xF << YT8521_RC1R_GE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_GE_TX_DELAY_DIS (0x0 << YT8521_RC1R_GE_TX_DELAY_BIT)
>>
>> #define YTPHY_MISC_CONFIG_REG 0xA006
>> #define YTPHY_MCR_FIBER_SPEED_MASK BIT(0)
>> @@ -222,11 +239,33 @@
>> */
>> #define YTPHY_WCR_TYPE_PULSE BIT(0)
>>
>> -#define YT8531S_SYNCE_CFG_REG 0xA012
>> -#define YT8531S_SCR_SYNCE_ENABLE BIT(6)
>> +#define YTPHY_SYNCE_CFG_REG 0xA012
>> +#define YT8521_SCR_CLK_SRC_MASK (BIT(2) | BIT(1))
>> +#define YT8521_SCR_CLK_SRC_PLL_125M (0x0 << 1)
>> +#define YT8521_SCR_CLK_SRC_REF_25M (0x3 << 1)
>> +#define YT8521_SCR_SYNCE_ENABLE BIT(5)
>> +#define YT8521_SCR_CLK_FRE_SEL_MASK BIT(3)
>> +#define YT8521_SCR_CLK_FRE_SEL_125M (0x1 << 3)
>> +#define YT8521_SCR_CLK_FRE_SEL_25M (0x0 << 3)
>
> Whenever it is a single bit, please use the BIT macro.

I wwill fix.

>
>> +#define YT8531_SCR_CLK_SRC_MASK (BIT(3) | BIT(2) | BIT(1))
>> +#define YT8531_SCR_CLK_SRC_PLL_125M (0x0 << 1)
>> +#define YT8531_SCR_CLK_SRC_REF_25M (0x4 << 1)
>> +#define YT8531_SCR_SYNCE_ENABLE BIT(6)
>> +#define YT8531_SCR_CLK_FRE_SEL_MASK BIT(4)
>> +#define YT8531_SCR_CLK_FRE_SEL_125M (0x1 << 4)
>> +#define YT8531_SCR_CLK_FRE_SEL_25M (0x0 << 4)
>>
>> /* Extended Register end */
>>
>> +#define YTPHY_DTS_MAX_TX_AMPLITUDE 0x2
>> +#define YTPHY_DTS_MAX_DELAY_VAL 2250
>> +#define YTPHY_DTS_STEP_DELAY_VAL 150
>> +#define YTPHY_DTS_INVAL_VAL 0xFF
>> +
>> +#define YTPHY_DTS_OUTPUT_CLK_DIS 0
>> +#define YTPHY_DTS_OUTPUT_CLK_25M 25000000
>> +#define YTPHY_DTS_OUTPUT_CLK_125M 125000000
>> +
>> struct yt8521_priv {
>> /* combo_advertising is used for case of YT8521 in combo mode,
>> * this means that yt8521 may work in utp or fiber mode which depends
>> @@ -243,6 +282,30 @@ struct yt8521_priv {
>> * YT8521_RSSR_TO_BE_ARBITRATED
>> */
>> u8 reg_page;
>> +
>> + /* The following parameters are from dts */
>> + /* rx delay = rx_delay_basic + rx_delay_additional
>> + * basic delay is ~2ns, 0 = off, 1 = on
>> + * rx_delay_additional,delay time = 150ps * val
>> + */
>> + u8 rx_delay_basic;
>> + u8 rx_delay_additional;
>> +
>> + /* tx_delay_ge is tx_delay for 1000Mbps
>> + * tx_delay_fe is tx_delay for 100Mbps or 10Mbps
>> + * delay time = 150ps * val
>> + */
>> + u8 tx_delay_ge;
>> + u8 tx_delay_fe;
>> + u8 sds_tx_amplitude;
>> + bool keep_pll_enabled;
>> + bool auto_sleep_disabled;
>> + bool clock_ouput; /* output clock ctl: 0=off, 1=on */
>> + bool clock_freq_125M; /* output clock freq selcect: 0=25M, 1=125M */
>> + bool tx_clk_adj_enabled;/* tx clk adj ctl: 0=off, 1=on */
>> + bool tx_clk_10_inverted;
>> + bool tx_clk_100_inverted;
>> + bool tx_clk_1000_inverted;
>
> Do you need to store all these values? In general, PHY drivers parse
> DT, and program the hardware directly. If these values are lost on
> reset, and you need to perform a reset for normal operation, then yes,
> it makes sense to store them. But in general, that is not how hardware
> works.
>

I will fix.

>> +static int ytphy_parse_dt(struct phy_device *phydev)
>> +{
>> + struct device_node *node = phydev->mdio.dev.of_node;
>> + struct yt8521_priv *priv = phydev->priv;
>> + u32 freq, val;
>> + int ret;
>> +
>> + priv->rx_delay_additional = YTPHY_DTS_INVAL_VAL;
>> + priv->sds_tx_amplitude = YTPHY_DTS_INVAL_VAL;
>> + priv->rx_delay_basic = YTPHY_DTS_INVAL_VAL;
>> + priv->tx_delay_ge = YTPHY_DTS_INVAL_VAL;
>> + priv->tx_delay_fe = YTPHY_DTS_INVAL_VAL;
>> +
>> + if (!IS_ENABLED(CONFIG_OF_MDIO)) {
>
> No other PHY driver does this. Why is this here?
>
> As a general rule, if you do something which no other driver does, you
> are doing something wrong. All PHY drivers should basically look the
> same, follow the same structure, etc. So it is a good idea to review 5
> other drivers, and make your driver look similar.
>

I will fix.

>> + priv->auto_sleep_disabled = true;
>> + priv->keep_pll_enabled = true;
>> + return 0;
>> + }
>> +
>> + ret = of_property_read_u32(node, "motorcomm,clk-out-frequency", &freq);
>> + if (ret < 0)
>> + freq = YTPHY_DTS_OUTPUT_CLK_DIS;/* default value as dts*/
>> +
>> + switch (freq) {
>> + case YTPHY_DTS_OUTPUT_CLK_DIS:
>> + priv->clock_ouput = false;
>> + break;
>> + case YTPHY_DTS_OUTPUT_CLK_25M:
>> + priv->clock_freq_125M = false;
>> + priv->clock_ouput = true;
>> + break;
>> + case YTPHY_DTS_OUTPUT_CLK_125M:
>> + priv->clock_freq_125M = true;
>> + priv->clock_ouput = true;
>> + break;
>> + default:
>> + phydev_err(phydev, "invalid motorcomm,clk-out-frequency\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!of_property_read_u32(node, "motorcomm,rx-delay-basic", &val)) {
>> + if (val > 1) {
>> + phydev_err(phydev,
>> + "invalid motorcomm,rx-delay-basic\n");
>> + return -EINVAL;
>> + }
>> + priv->rx_delay_basic = val;
>> + }
>> +
>> + if (!of_property_read_u32(node, "motorcomm,rx-delay-additional-ps", &val)) {
>> + if (val > YTPHY_DTS_MAX_DELAY_VAL) {
>> + phydev_err(phydev, "invalid motorcomm,rx-delay-additional-ps\n");
>> + return -EINVAL;
>> + }
>
> Please check the value is also one of the supported values. Please do
> that for all your delays.
>

The following code check the supported values.

if (val)
val /= YTPHY_DTS_STEP_DELAY_VAL;
priv->rx_delay_additional = val;


rx-delay-additional-ps = reg_val(0-15) *150ps. so val >
YTPHY_DTS_MAX_DELAY_VAL 2250 is err.
rx_delay_additional is store the reg_val (0-15).

>> + if (of_property_read_bool(node, "motorcomm,keep-pll-enabled"))
>> + priv->keep_pll_enabled = true;
>
> I think this only makes sense when priv->clock_output is true? Please
> test for that.
>

No,priv->clock_output is used to output 25Mhz or 125Mhz for mac.
priv->keep_pll_enabled is used to enable RXC clock when no wire plug.

>
>> +static int ytphy_rgmii_clk_delay_config(struct phy_device *phydev)
>> +{
>> + struct yt8521_priv *priv = phydev->priv;
>> + u16 mask = 0;
>> + u16 val = 0;
>> + int ret;
>> +
>> + /* rx delay basic controlled by dts.*/
>> + if (priv->rx_delay_basic != YTPHY_DTS_INVAL_VAL) {
>> + if (priv->rx_delay_basic)
>> + val = YT8521_CCR_RXC_DLY_EN;
>> + ret = ytphy_modify_ext(phydev, YT8521_CHIP_CONFIG_REG,
>> + YT8521_CCR_RXC_DLY_EN, val);
>> + if (ret < 0)
>> + return ret;
>> + }
>> +
>> + val = 0;
>> + /* If rx_delay_additional and tx_delay_* are all not be seted in dts,
>> + * then used the fixed *_DELAY_DIS or *_DELAY_EN. Otherwise, use the
>> + * value set by rx_delay_additional, tx_delay_ge and tx_delay_fe.
>> + */
>
> So what you should be doing here is always respecting
> phydev->interface. You can then fine tune the delays using
> rx-internal-delay-ps and tx-internal-delay-ps.
>

I will fix.

>> +static int ytphy_probe_helper(struct phy_device *phydev)
>> +{
>> + struct device *dev = &phydev->mdio.dev;
>> + struct yt8521_priv *priv;
>> + int ret;
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + phydev->priv = priv;
>> +
>> + ret = ytphy_parse_dt(phydev);
>> + if (ret < 0)
>> + return ret;
>> +
>> + phy_lock_mdio_bus(phydev);
>> + ret = ytphy_clk_out_config(phydev);
>> + phy_unlock_mdio_bus(phydev);
>> + return ret;
>> +}
>> +
>> /**
>> * yt8521_probe() - read chip config then set suitable polling_mode
>> * @phydev: a pointer to a &struct phy_device
>> @@ -601,16 +983,15 @@ static int yt8521_write_page(struct phy_device *phydev, int page)
>> */
>> static int yt8521_probe(struct phy_device *phydev)
>> {
>> - struct device *dev = &phydev->mdio.dev;
>> struct yt8521_priv *priv;
>> int chip_config;
>> int ret;
>>
>> - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> - if (!priv)
>> - return -ENOMEM;
>> + ret = ytphy_probe_helper(phydev);
>> + if (ret < 0)
>> + return ret;
>>
>> - phydev->priv = priv;
>> + priv = phydev->priv;
>
> I don't see why you added this probe helper.
>
> Is this to make the driver look more like the vendor driver? Please
> seperate refactoring patches from new functionality. We want to see
> lots of simple, obviously correct patches which are easy to review.
>

I will fix.

> Andrew
>