Re: [PATCH net-next v2 3/3] net: phy: broadcom: Allow BCM54210E to configure APD

From: Florian Fainelli <f.fainelli@gmail.com>
To: Vladimir Oltean <olteanv@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>
Cc: netdev@vger.kernel.org, Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Michael Chan <mchan@broadcom.com>,
	"open list:BROADCOM ETHERNET PHY DRIVERS" 
	<bcm-kernel-feedback-list@broadcom.com>,
	open list <linux-kernel@vger.kernel.org>,
	michael@walle.cc
Subject: Re: [PATCH net-next v2 3/3] net: phy: broadcom: Allow BCM54210E to configure APD
Date: Sun, 14 Feb 2021 20:29:15 -0800	[thread overview]
Message-ID: <4e1c1a4c-d276-c850-8e97-16ef1f08dcff@gmail.com> (raw)
In-Reply-To: <20210213104245.uti4qb2u2r5nblef@skbuf>


On 2/13/2021 2:42 AM, Vladimir Oltean wrote:
> On Fri, Feb 12, 2021 at 07:46:32PM -0800, Florian Fainelli wrote:
>> BCM54210E/BCM50212E has been verified to work correctly with the
>> auto-power down configuration done by bcm54xx_adjust_rxrefclk(), add it
>> to the list of PHYs working.
>>
>> While we are at it, provide an appropriate name for the bit we are
>> changing which disables the RXC and TXC during auto-power down when
>> there is no energy on the cable.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
> 
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> 
>>  drivers/net/phy/broadcom.c | 8 +++++---
>>  include/linux/brcmphy.h    | 2 +-
>>  2 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>> index 3ce266ab521b..91fbd26c809e 100644
>> --- a/drivers/net/phy/broadcom.c
>> +++ b/drivers/net/phy/broadcom.c
>> @@ -193,6 +193,7 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev)
>>  	if (BRCM_PHY_MODEL(phydev) != PHY_ID_BCM57780 &&
>>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610 &&
>>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610M &&
>> +	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54210E &&
>>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54810 &&
>>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54811)
>>  		return;
>> @@ -227,9 +228,10 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev)
>>  		val |= BCM54XX_SHD_SCR3_DLLAPD_DIS;
>>  
>>  	if (phydev->dev_flags & PHY_BRCM_DIS_TXCRXC_NOENRGY) {
>> -		if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 ||
>> -		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54811)
>> -			val |= BCM54810_SHD_SCR3_TRDDAPD;
>> +		if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E ||
>> +		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 ||
>> +		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E)
>> +			val |= BCM54XX_SHD_SCR3_RXCTXC_DIS;
>>  		else
>>  			val |= BCM54XX_SHD_SCR3_TRDDAPD;
>>  	}
>> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
>> index 844dcfe789a2..16597d3fa011 100644
>> --- a/include/linux/brcmphy.h
>> +++ b/include/linux/brcmphy.h
>> @@ -193,6 +193,7 @@
>>  #define  BCM54XX_SHD_SCR3_DEF_CLK125	0x0001
>>  #define  BCM54XX_SHD_SCR3_DLLAPD_DIS	0x0002
>>  #define  BCM54XX_SHD_SCR3_TRDDAPD	0x0004
>> +#define  BCM54XX_SHD_SCR3_RXCTXC_DIS	0x0100
> 
> Curiously enough, my BCM5464R datasheet does say:
> 
> The TXC and RXC outputs can be disabled during auto-power down by setting the “1000BASE-T/100BASE-TX/10BASE-T
> Spare Control 3 Register (Address 1Ch, Shadow Value 00101),” bit 8 =1.
> 
> but when I go to the definition of the register, bit 8 is hidden. Odd.
> 
> How can I ensure that the auto power down feature is doing something?

I am trying to confirm what the expected power levels should be from the
54210E product engineer so I can give you an estimate of what you should
see with and without while measure the PHY's regulators.
-- 
Florian