Re: [PATCH net-next 2/3] net: phy: broadcom: Fix RXC/TXC auto disabling

From: Florian Fainelli <f.fainelli@gmail.com>
To: Florian Fainelli <f.fainelli@gmail.com>,
	Vladimir Oltean <olteanv@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 2/3] net: phy: broadcom: Fix RXC/TXC auto disabling
Date: Fri, 12 Feb 2021 19:43:24 -0800	[thread overview]
Message-ID: <6482a9a1-964f-2f7b-8688-62f585d13206@gmail.com> (raw)
In-Reply-To: <01e62046-7674-bb1d-115f-9044726c0ce7@gmail.com>

On 2/12/2021 5:14 PM, Florian Fainelli wrote:
> 
> 
> On 2/12/2021 5:11 PM, Vladimir Oltean wrote:
>> On Fri, Feb 12, 2021 at 12:57:20PM -0800, Florian Fainelli wrote:
>>> When support for optionally disabling the TXC was introduced, bit 2 was
>>> used to do that operation but the datasheet for 50610M from 2009 does
>>> not show bit 2 as being defined. Bit 8 is the one that allows automatic
>>> disabling of the RXC/TXC auto disabling during auto power down.
>>>
>>> Fixes: 52fae0837153 ("tg3 / broadcom: Optionally disable TXC if no link")
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>>  include/linux/brcmphy.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
>>> index da7bf9dfef5b..3dd8203cf780 100644
>>> --- a/include/linux/brcmphy.h
>>> +++ b/include/linux/brcmphy.h
>>> @@ -193,7 +193,7 @@
>>>  #define BCM54XX_SHD_SCR3		0x05
>>>  #define  BCM54XX_SHD_SCR3_DEF_CLK125	0x0001
>>>  #define  BCM54XX_SHD_SCR3_DLLAPD_DIS	0x0002
>>> -#define  BCM54XX_SHD_SCR3_TRDDAPD	0x0004
>>> +#define  BCM54XX_SHD_SCR3_TRDDAPD	0x0100
>>>  
>>>  /* 01010: Auto Power-Down */
>>>  #define BCM54XX_SHD_APD			0x0a
>>> -- 
>>> 2.25.1
>>>
>>
>> We may have a problem here, with the layout of the Spare Control 3
>> register not being as universal as we think.
>>
>> Your finding may have been the same as Kevin Lo's from commit
>> b0ed0bbfb304 ("net: phy: broadcom: add support for BCM54811 PHY"),
>> therefore your change is making BCM54XX_SHD_SCR3_TRDDAPD ==
>> BCM54810_SHD_SCR3_TRDDAPD, so currently this if condition is redundant
>> and probably something else is wrong too:
>>
>> 	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;
>> 		else
>> 			val |= BCM54XX_SHD_SCR3_TRDDAPD;
>> 	}
>>
>> I'm not sure what "TRDD" stands for, but my copy of the BCM5464R
>> datasheet shows both bits 2 as well as 8 as being reserved. I have
>> "CLK125 Output" in bit 0, "DLL Auto Power-Down" in bit 1, "SD/Energy
>> Detect Change" in bit 5, "TXC Disable" in bit 6, and that's about it.
> 
> Let me go back to the datasheet of all of the PHYs supported by
> bcm54xx_adjust_rxrefclk() and make sure we set the right bit.

I really don't know what the situation is with the 50610 and 50610M and
the datasheet appears to reflect that the latest PHYs should be revision
3 or newer (which we explicitly check for earlier), and that bit 8 is
supposed to control the disabling of RXC/TXC during auto-power down. I
can only trust that Matt checked with the design team back then and that
the datasheet must be wrong (would not be an isolated incident), let's
try not to fix something that we do not know for sure is broken.

I will respin without this patch and with another clean up added.
-- 
Florian