netdev - Re: [PATCH net 2/3] amd-xgbe: enable PLL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8a7c989d-1541-bb69-89b6-e18398ecaed4@amd.com>
Date:   Thu, 6 Oct 2022 22:53:53 +0530
From:   Raju Rangoju <Raju.Rangoju@....com>
To:     Tom Lendacky <thomas.lendacky@....com>, Shyam-sundar.S-k@....com,
        davem@...emloft.net, kuba@...nel.org
Cc:     netdev@...r.kernel.org, rrangoju@....com
Subject: Re: [PATCH net 2/3] amd-xgbe: enable PLL_CTL for fixed PHY modes only

Hi Tom,

On 10/6/2022 8:17 PM, Tom Lendacky wrote:
> On 10/6/22 08:54, Raju Rangoju wrote:
>> PLL control setting(HW RRCM) is needed only in fixed PHY configuration
>> to fix the peer-peer issues. Without the PLL control setting, the link
>> up takes longer time in a fixed phy configuration.
>>
>> Driver implements SW RRCM for Autoneg On configuration, hence PLL
>> control setting (HW RRCM) is not needed for AN On configuration, and
>> can be skipped.
>>
>> Also, PLL re-initialization is not needed for PHY Power Off and RRCM
>> commands. Otherwise, they lead to mailbox errors. Added the changes
>> accordingly.
>>
>> Fixes: daf182d360e5 ("net: amd-xgbe: Toggle PLL settings during rate 
>> change")
>> Signed-off-by: Raju Rangoju <Raju.Rangoju@....com>
>> ---
>>   drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 21 +++++++++++++--------
>>   drivers/net/ethernet/amd/xgbe/xgbe.h        | 10 ++++++++++
>>   2 files changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c 
>> b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>> index 19b943eba560..23fbd89a29df 100644
>> --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>> @@ -1979,13 +1979,16 @@ static void xgbe_phy_rx_reset(struct 
>> xgbe_prv_data *pdata)
>>   static void xgbe_phy_pll_ctrl(struct xgbe_prv_data *pdata, bool enable)
>>   {
>> -    XMDIO_WRITE_BITS(pdata, MDIO_MMD_PMAPMD, MDIO_VEND2_PMA_MISC_CTRL0,
>> -             XGBE_PMA_PLL_CTRL_MASK,
>> -             enable ? XGBE_PMA_PLL_CTRL_ENABLE
>> -                : XGBE_PMA_PLL_CTRL_DISABLE);
>> +    /* PLL_CTRL feature needs to be enabled for fixed PHY modes 
>> (Non-Autoneg) only */
>> +    if (pdata->phy.autoneg == AUTONEG_DISABLE) {
>> +        XMDIO_WRITE_BITS(pdata, MDIO_MMD_PMAPMD, 
>> MDIO_VEND2_PMA_MISC_CTRL0,
>> +                 XGBE_PMA_PLL_CTRL_MASK,
>> +                 enable ? XGBE_PMA_PLL_CTRL_ENABLE
>> +                    : XGBE_PMA_PLL_CTRL_DISABLE);
>> -    /* Wait for command to complete */
>> -    usleep_range(100, 200);
>> +        /* Wait for command to complete */
>> +        usleep_range(100, 200);
>> +    }
> 
> Rather than indent all this, just add an if that returns at the beginning
> of the function, e.g.:
> 
>      /* PLL_CTRL feature needs to be enabled for fixed PHY modes 
> (Non-Autoneg) only */
>      if (pdata->phy.autoneg != AUTONEG_DISABLE)
>          return;

Sure, I'll add the above changes.

> 
> Now a general question... is this going to force Autoneg ON to end up
> always going through the RRC path now where it may not have before? In
> other words, there's now an auto-negotiation delay?

As per the databook, Receiver power state change (HW RRCM) is not 
allowed during the CL73 / CL72. FW is already implementing SW RRCM for 
Autoneg ON, and no additional changes are needed in driver for AN ON 
path. However, HW RRCM(PLL_CTRL) is needed in Fixed PHY configs.

> 
>>   }
>>   static void xgbe_phy_perform_ratechange(struct xgbe_prv_data *pdata,
>> @@ -2029,8 +2032,10 @@ static void xgbe_phy_perform_ratechange(struct 
>> xgbe_prv_data *pdata,
>>       xgbe_phy_rx_reset(pdata);
>>   reenable_pll:
>> -    /* Enable PLL re-initialization */
>> -    xgbe_phy_pll_ctrl(pdata, true);
>> +    /* Enable PLL re-initialization, not needed for PHY Power Off cmd */
> 
> Comment should also include the RRC command...
> 
>> +    if (cmd != XGBE_MAILBOX_CMD_POWER_OFF &&
>> +        cmd != XGBE_MAILBOX_CMD_RRCM)
>> +        xgbe_phy_pll_ctrl(pdata, true);
>>   }
>>   static void xgbe_phy_rrc(struct xgbe_prv_data *pdata)
>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h 
>> b/drivers/net/ethernet/amd/xgbe/xgbe.h
>> index 49d23abce73d..c7865681790c 100644
>> --- a/drivers/net/ethernet/amd/xgbe/xgbe.h
>> +++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
>> @@ -611,6 +611,16 @@ enum xgbe_mdio_mode {
>>       XGBE_MDIO_MODE_CL45,
>>   };
>> +enum XGBE_MAILBOX_CMD {
>> +    XGBE_MAILBOX_CMD_POWER_OFF    = 0,
>> +    XGBE_MAILBOX_CMD_SET_1G        = 1,
>> +    XGBE_MAILBOX_CMD_SET_2_5G    = 2,
>> +    XGBE_MAILBOX_CMD_SET_10G_SFI    = 3,
>> +    XGBE_MAILBOX_CMD_SET_10G_KR    = 4,
>> +    XGBE_MAILBOX_CMD_RRCM        = 5,
>> +    XGBE_MAILBOX_CMD_UNKNOWN    = 6
>> +};
> 
> If you're going to add an enum for the commands, then you should apply
> them everywhere. Creating this enum and updating all the command locations
> should be a pre-patch to this patch.
> 
> Thanks,
> Tom
> 
>> +
>>   struct xgbe_phy {
>>       struct ethtool_link_ksettings lks;