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

[Arun.Ramadoss]

Hi Frank,

On Thu, 2023-01-05 at 15:30 +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.

As per the commit message and description, it mentions adding dts
support. But this patch does lot of things. Add elaborate description
or split the patch logically. 

> 
> 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
>  
>  /* 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))
> +#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))
> +
>  /* 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)
>  /* 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)
> 

This can be splitted as preparatory patch like using BIT macro instead
of magic number.

>  
>  #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))

For the mask, you can consider using GENMASK macro

> +#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)
> +#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)
>  
>   
> +
> +static int ytphy_clk_out_config(struct phy_device *phydev)
> +{
> +	struct yt8521_priv *priv = phydev->priv;
> +	u16 set = 0;
> +	u16 mask;
> +
> +	switch (phydev->drv->phy_id) {
> +	case PHY_ID_YT8511:
> +		/* YT8511 will be supported later */
> +		return -EOPNOTSUPP;
> +	case PHY_ID_YT8521:
> +		mask = YT8521_SCR_SYNCE_ENABLE;
> +		if (priv->clock_ouput) {
> +			mask |= YT8521_SCR_CLK_SRC_MASK;
> +			mask |= YT8521_SCR_CLK_FRE_SEL_MASK;

You can consider assigning mask in single statement.

> +			set |= YT8521_SCR_SYNCE_ENABLE;
> +			if (priv->clock_freq_125M) {
> +				set |= YT8521_SCR_CLK_FRE_SEL_125M;
> +				set |= YT8521_SCR_CLK_SRC_PLL_125M;

Similarly here.

> +			} else {
> +				set |= YT8521_SCR_CLK_FRE_SEL_25M;
> +				set |= YT8521_SCR_CLK_SRC_REF_25M;
> +			}
> +		}
> +		break;
> +	case PHY_ID_YT8531:
> +	case PHY_ID_YT8531S:
> +		mask = YT8531_SCR_SYNCE_ENABLE;
> +		if (priv->clock_ouput) {
> +			mask |= YT8531_SCR_CLK_SRC_MASK;
> +			mask |= YT8531_SCR_CLK_FRE_SEL_MASK;
> +			set |= YT8531_SCR_SYNCE_ENABLE;
> +			if (priv->clock_freq_125M) {
> +				set |= YT8531_SCR_CLK_FRE_SEL_125M;
> +				set |= YT8531_SCR_CLK_SRC_PLL_125M;
> +			} else {
> +				set |= YT8531_SCR_CLK_FRE_SEL_25M;
> +				set |= YT8531_SCR_CLK_SRC_REF_25M;
> +			}
> +		}
> +		break;
> +	default:
> +		phydev_err(phydev, "invalid phy id\n");
> +		return -EINVAL;
> +	}
> +
> +	return ytphy_modify_ext(phydev, YTPHY_SYNCE_CFG_REG, mask,
> set);
> +}
> +
> ++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.
> +	 */
> +	if ((priv->rx_delay_additional & priv->tx_delay_ge & priv-
> >tx_delay_fe)
> +	   == YTPHY_DTS_INVAL_VAL) {
> +		switch (phydev->interface) {
> +		case PHY_INTERFACE_MODE_RGMII:
> +			val |= YT8521_RC1R_GE_TX_DELAY_DIS;
> +			val |= YT8521_RC1R_FE_TX_DELAY_DIS;
> +			val |= YT8521_RC1R_RX_DELAY_DIS;

Single statement would be suffice.

> +			break;
> +		case PHY_INTERFACE_MODE_RGMII_RXID:
> +			val |= YT8521_RC1R_GE_TX_DELAY_DIS;
> +			val |= YT8521_RC1R_FE_TX_DELAY_DIS;
> +			val |= YT8521_RC1R_RX_DELAY_EN;
> +			break;
> +		case PHY_INTERFACE_MODE_RGMII_TXID:
> +			val |= YT8521_RC1R_GE_TX_DELAY_EN;
> +			val |= YT8521_RC1R_FE_TX_DELAY_EN;
> +			val |= YT8521_RC1R_RX_DELAY_DIS;
> +			break;
> +		case PHY_INTERFACE_MODE_RGMII_ID:
> +			val |= YT8521_RC1R_GE_TX_DELAY_EN;
> +			val |= YT8521_RC1R_FE_TX_DELAY_EN;
> +			val |= YT8521_RC1R_RX_DELAY_EN;
> +			break;
> +		default: /* do not support other modes */
> +			return -EOPNOTSUPP;
> +		}
> +		mask = YT8521_RC1R_RX_DELAY_MASK |
> YT8521_RC1R_FE_TX_DELAY_MASK
> +		       | YT8521_RC1R_GE_TX_DELAY_MASK;
> +	} 
> +
>  
>  /**
>   * ytphy_utp_read_lpa() - read LPA then setup lp_advertising for utp
>   * @phydev: a pointer to a &struct phy_device
> @@ -1125,6 +1486,34 @@ static int yt8521_resume(struct phy_device
> *phydev)
>  	return yt8521_modify_utp_fiber_bmcr(phydev, BMCR_PDOWN, 0);
>  }
>  
> 
> @@ -1778,7 +2129,7 @@ static struct phy_driver motorcomm_phy_drvs[] =
> {
>  		PHY_ID_MATCH_EXACT(PHY_ID_YT8531S),
>  		.name		= "YT8531S Gigabit Ethernet",
>  		.get_features	= yt8521_get_features,
> -		.probe		= yt8531s_probe,
> +		.probe		= yt8521_probe,
>  		.read_page	= yt8521_read_page,
>  		.write_page	= yt8521_write_page,
>  		.get_wol	= ytphy_get_wol,
> @@ -1804,7 +2155,7 @@ static const struct mdio_device_id
> __maybe_unused motorcomm_tbl[] = {
>  	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8511) },
>  	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8521) },
>  	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8531S) },
> -	{ /* sentinal */ }
> +	{ /* sentinel */ }

It should go as separate patch.
>  };
>  
>  MODULE_DEVICE_TABLE(mdio, motorcomm_tbl);