Hi Arun Ramadoss,
On 2023/1/5 17:01, Arun.Ramadoss@xxxxxxxxxxxxx wrote:
> 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.
>
I will fix.
>>
>> 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.
>
I will fix.
>>
>> #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
>
I will fix.
>> +#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.
I will fix.
>
>> + 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.
I will fix.
>
>> + } 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.
I will fix.
>
>> + 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.
I will fix.
>> };
>>
>> MODULE_DEVICE_TABLE(mdio, motorcomm_tbl);
