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 v3] net: phy: broadcom: add 1000Base-X support
 for BCM54616S
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     Andrew Lunn <andrew@lunn.ch>, Tao Ren <taoren@fb.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S . Miller" <davem@davemloft.net>,
        Arun Parameswaran <arun.parameswaran@broadcom.com>,
        Justin Chen <justinpopo6@gmail.com>,
        netdev <netdev@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        "openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>
References: <20190802215419.313512-1-taoren@fb.com>
 <CA+h21hrOEape89MTqCUyGFt=f6ba7Q-2KcOsN_Vw2Qv8iq86jw@mail.gmail.com>
 <53e18a01-3d08-3023-374f-2c712c4ee9ea@fb.com> <20190804145152.GA6800@lunn.ch>
 <CA+h21hrUDaSxKpsy9TuWqwgaxKYaoXHyhgS=xSoAcPwxXzvrHg@mail.gmail.com>
 <f8de2514-081a-0e6e-fbe2-bcafcd459646@gmail.com>
 <CA+h21hov3WzqYSUcxOnH0DOMO2dYdh_Q30Q_GQJpxa4nFM7MsQ@mail.gmail.com>
From:   Heiner Kallweit <hkallweit1@gmail.com>
Message-ID: <291a3c6e-ca8f-a9b8-a0b8-735a68dc04ea@gmail.com>
Date:   Mon, 5 Aug 2019 22:45:01 +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: <CA+h21hov3WzqYSUcxOnH0DOMO2dYdh_Q30Q_GQJpxa4nFM7MsQ@mail.gmail.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 04.08.2019 21:22, Vladimir Oltean wrote:
> On Sun, 4 Aug 2019 at 19:07, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 04.08.2019 17:59, Vladimir Oltean wrote:
>>> On Sun, 4 Aug 2019 at 17:52, Andrew Lunn <andrew@lunn.ch> wrote:
>>>>
>>>>>> The patchset looks better now. But is it ok, I wonder, to keep
>>>>>> PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that
>>>>>> phy_attach_direct is overwriting it?
>>>>>
>>>>
>>>>> I checked ftgmac100 driver (used on my machine) and it calls
>>>>> phy_connect_direct which passes phydev->dev_flags when calling
>>>>> phy_attach_direct: that explains why the flag is not cleared in my
>>>>> case.
>>>>
>>>> Yes, that is the way it is intended to be used. The MAC driver can
>>>> pass flags to the PHY. It is a fragile API, since the MAC needs to
>>>> know what PHY is being used, since the flags are driver specific.
>>>>
>>>> One option would be to modify the assignment in phy_attach_direct() to
>>>> OR in the flags passed to it with flags which are already in
>>>> phydev->dev_flags.
>>>>
>>>>         Andrew
>>>
>>> Even if that were the case (patching phy_attach_direct to apply a
>>> logical-or to dev_flags), it sounds fishy to me that the genphy code
>>> is unable to determine that this PHY is running in 1000Base-X mode.
>>>
>>> In my opinion it all boils down to this warning:
>>>
>>> "PHY advertising (0,00000200,000062c0) more modes than genphy
>>> supports, some modes not advertised".
>>>
>> The genphy code deals with Clause 22 + Gigabit BaseT only.
>> Question is whether you want aneg at all in 1000Base-X mode and
>> what you want the config_aneg callback to do.
>> There may be some inspiration in the Marvel PHY drivers.
>>
> 
> AN for 1000Base-X still gives you duplex and pause frame settings. I
> thought the base page format for exchanging that info is standardized
> in clause 37.
> Does genphy cover only copper media by design, or is it desirable to
> augment genphy_read_status?
> 
So far we care about copper only in phylib. Some constants needed for
Clause 37 support are defined, but used by few drivers only.

ADVERTISE_1000XHALF
ADVERTISE_1000XFULL
ADVERTISE_1000XPAUSE
ADVERTISE_1000XPSE_ASYM

I think it would make sense to have something like genphy_c37_config_aneg.
Similar for read_status.

>>> You see, the 0x200 in the above advertising mask corresponds exactly
>>> to this definition from ethtool.h:
>>>     ETHTOOL_LINK_MODE_1000baseX_Full_BIT    = 41,
>>>
>>> But it gets truncated and hence lost.
>>>
>>> Regards,
>>> -Vladimir
>>>
>> Heiner
> 
Heiner