From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver
Date: Thu, 13 Dec 2018 15:33:52 +0100
Message-ID: <20181213143352.GA12808@lunn.ch>
References: <20181213020150.17099-1-marex@denx.de>
 <20181213083812.GA1605@lunn.ch>
 <26001882-7e29-1e73-3259-656772d37ad9@denx.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: netdev@vger.kernel.org, f.fainelli@gmail.com
To: Marek Vasut <marex@denx.de>
Return-path: <netdev-owner@vger.kernel.org>
Received: from vps0.lunn.ch ([185.16.172.187]:46527 "EHLO vps0.lunn.ch"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1727947AbeLMOdz (ORCPT <rfc822;netdev@vger.kernel.org>);
        Thu, 13 Dec 2018 09:33:55 -0500
Content-Disposition: inline
In-Reply-To: <26001882-7e29-1e73-3259-656772d37ad9@denx.de>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Thu, Dec 13, 2018 at 02:27:51PM +0100, Marek Vasut wrote:
> On 12/13/2018 09:38 AM, Andrew Lunn wrote:
> >> +static int tja11xx_config_aneg(struct phy_device *phydev)
> >> +{
> >> +	phydev->autoneg = 0;
> >> +	phydev->speed = SPEED_100;
> >> +	phydev->duplex = DUPLEX_FULL;
> >> +	phydev->pause = 0;
> >> +	phydev->asym_pause = 0;
> >> +
> >> +	return 0;
> >> +}
> > 
> > Hi Marek
> 
> Hi,
> 
> > That is, err, interesting. Are you saying the PHY cannot do auto-neg?
> 
> There isn't much to auto-negotiate and some of the bits in the standard
> registers behave weirdly.

Hi Marek

Hopefully the config_aneg callback is not called if you don't list
autoneg to the .features. The microchip_t1 driver just uses
genphy_config_aneg, but if a NULL works, i would prefer that.

> > Does it happen to be a 100T1 device for automotive?
> 
> Yes, that's what the commit message says, why ?

Sorry, i missed it in the commit message.  We now have enough bits in
the link_mode that we should explicitly say this is 100BaseT2_Full,
not 100BaseT_Full. So please add a new member to
ethtool_link_mode_bit_indices ...
 
> >> +
> >> +static int tja11xx_config_init(struct phy_device *phydev)
> >> +{
> >> +	u32 features = SUPPORTED_TP | SUPPORTED_MII | SUPPORTED_100baseT_Full;

And make it part of the .features. Well, you need to add a new
PHY_BASIC_T2_FEATURES macro, which uses it, and use that in the
phy_driver structure. There should not be any need to modify it in the
config_init call.

We also need to think about what we do with the PHY_BASIC_T1_FEATURES
macro. Ideally we want to swap that to also make use of a new
ethtool_link_mode_bit_indices, but i've no idea at the moment if that
will break something.

  Andrew