Re: [PATCH net-next 1/2] net: phy: broadcom: set features explicitly for BCM54616S

From: Heiner Kallweit <hkallweit1@gmail.com>
To: Tao Ren <taoren@fb.com>, Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
	"David S . Miller" <davem@davemloft.net>,
	Arun Parameswaran <arun.parameswaran@broadcom.com>,
	Justin Chen <justinpopo6@gmail.com>,
	Vladimir Oltean <olteanv@gmail.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Andrew Jeffery <andrew@aj.id.au>,
	"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>
Subject: Re: [PATCH net-next 1/2] net: phy: broadcom: set features explicitly for BCM54616S
Date: Wed, 31 Jul 2019 07:53:44 +0200	[thread overview]
Message-ID: <41c1f898-aee8-d73a-386d-c3ce280c5a1b@gmail.com> (raw)
In-Reply-To: <f59c2ae9-ef44-1e1b-4ae2-216eb911e92e@fb.com>

On 31.07.2019 02:12, Tao Ren wrote:
> On 7/29/19 11:00 PM, Heiner Kallweit wrote:
>> On 30.07.2019 07:05, Tao Ren wrote:
>>> On 7/29/19 8:35 PM, Andrew Lunn wrote:
>>>> On Mon, Jul 29, 2019 at 05:25:32PM -0700, Tao Ren wrote:
>>>>> BCM54616S feature "PHY_GBIT_FEATURES" was removed by commit dcdecdcfe1fc
>>>>> ("net: phy: switch drivers to use dynamic feature detection"). As dynamic
>>>>> feature detection doesn't work when BCM54616S is working in RGMII-Fiber
>>>>> mode (different sets of MII Control/Status registers being used), let's
>>>>> set "PHY_GBIT_FEATURES" for BCM54616S explicitly.
>>>>
>>>> Hi Tao
>>>>
>>>> What exactly does it get wrong?
>>>>
>>>>      Thanks
>>>> 	Andrew
>>>
>>> Hi Andrew,
>>>
>>> BCM54616S is set to RGMII-Fiber (1000Base-X) mode on my platform, and none of the features (1000BaseT/100BaseT/10BaseT) can be detected by genphy_read_abilities(), because the PHY only reports 1000BaseX_Full|Half ability in this mode.
>>>
>> Are you going to use the PHY in copper or fibre mode?
>> In case you use fibre mode, why do you need the copper modes set as supported?
>> Or does the PHY just start in fibre mode and you want to switch it to copper mode?
> 
> Hi Heiner,
> 
> The phy starts in fiber mode and that's the mode I want.
> My observation is: phydev->link is always 0 (Link status bit is never set in MII_BMSR) by using dynamic ability detection on my machine. I checked phydev->supported and it's set to "AutoNeg | TP | MII | Pause | Asym_Pause" by dynamic ability detection. Is it normal/expected? Or maybe the fix should go to different places? Thank you for your help.
> 

Not sure whether you stated already which kernel version you're using.
There's a brand-new extension to auto-detect 1000BaseX:
f30e33bcdab9 ("net: phy: Add more 1000BaseX support detection")
It's included in the 5.3-rc series.

If a feature can be read from a vendor-specific register only,
then the preferred way is: Implement callback get_features in
the PHY driver, call genphy_read_abilities for the basic features
and complement it with reading the vendor-specific register(s).

> 
> Thanks,
> 
> Tao
> 
Heiner