Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X
 support for BCM54616S
To:     Tao Ren <taoren@fb.com>, Andrew Lunn <andrew@lunn.ch>,
        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>,
        "openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>
References: <20190806210931.3723590-1-taoren@fb.com>
 <fe0d39ea-91f3-0cac-f13b-3d46ea1748a3@fb.com>
From:   Heiner Kallweit <hkallweit1@gmail.com>
Message-ID: <cfd6e14e-a447-aedb-5bd6-bf65b4b6f98a@gmail.com>
Date:   Wed, 7 Aug 2019 21:18:28 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <fe0d39ea-91f3-0cac-f13b-3d46ea1748a3@fb.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 06.08.2019 23:42, Tao Ren wrote:
> Hi Andrew / Heiner / Vladimir,
> 
> On 8/6/19 2:09 PM, Tao Ren wrote:
>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>> example, on Facebook CMM BMC platform), mainly because genphy functions
>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
>> needs to be handled differently.
>>
>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>> driver callbacks:
>>
>>   - probe: probe callback detects PHY's operation mode based on
>>     INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>>     Control register.
>>
>>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
>>     negotiation in 1000Base-X mode.
>>
>>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
>>     callback which manually set link speed and duplex mode in 1000Base-X
>>     mode.
>>
>> Signed-off-by: Tao Ren <taoren@fb.com>
> 
> I customized config_aneg function for BCM54616S 1000Base-X mode and link-down issue is also fixed: the patch is tested on Facebook CMM and Minipack BMC and everything looks normal. Please kindly review when you have bandwidth and let me know if you have further suggestions.
> 
> BTW, I would be happy to help if we decide to add a set of genphy functions for clause 37, although that may mean I need more help/guidance from you :-)

You want to have standard clause 37 aneg and this should be generic in phylib.
I hacked together a first version that is compile-tested only:
https://patchwork.ozlabs.org/patch/1143631/
It supports fixed mode too.

It doesn't support half duplex mode because phylib doesn't know 1000BaseX HD yet.
Not sure whether half duplex mode is used at all in reality.

You could test the new core functions in your own config_aneg and read_status
callback implementations.

> 
> 
> Cheers,
> 
> Tao
> 
Heiner