Re: Re: [PATCH net-next 0/1] Add BASE-T1 PHY support

From: Christian Herber <christian.herber@nxp.com>
To: Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>
Cc: "davem@davemloft.net" <davem@davemloft.net>,
	Florian Fainelli <f.fainelli@gmail.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
Date: Thu, 22 Aug 2019 07:18:18 +0000	[thread overview]
Message-ID: <AM6PR0402MB3798C702793071E34A5659ED86A50@AM6PR0402MB3798.eurprd04.prod.outlook.com> (raw)
In-Reply-To: 20190821185715.GA16401@lunn.ch

On 21.08.2019 20:57, Andrew Lunn wrote:
> 
>> The current patch set IMO is a little bit hacky. I'm not 100% happy
>> with the implicit assumption that there can't be devices supporting
>> T1 and classic BaseT modes or fiber modes.
>
>> Andrew: Do you have an opinion on that?
> 
> Hi Heiner
> 
> I would also like cleaner integration. I doubt here is anything in the
> standard which says you cannot combine these modes. It is more a
> marketing question if anybody would build such a device. Maybe not
> directly into a vehicle, but you could imaging a mobile test device
> which uses T1 to talk to the car and T4 to connect to the garage
> network?
> 
> So i don't think we should limit ourselves. phylib should provide a
> clean, simple set of helpers to perform standard operations for
> various modes. Drivers can make use of those helpers. That much should
> be clear. If we try to make genphy support them all simultaneously, is
> less clear.
> 
>       Andrew
> 

If you want to go down this path, then i think we have to ask some more 
questions. Clause 45 is a very scalable register scheme, it is not a 
specific class of devices and will be extended and extended.

Currently, the phy-c45.c supports 10/100/1000/2500/5000/10000 Mbps 
consumer/enterprise PHYs. This is also an implicit assumption. The 
register set (e.g. on auto-neg) used for this will also only support 
these modes and nothing more, as it is done scaling.

Currently not supported, but already present in IEEE 802.3:
- MultiGBASE-T (25/40 Gbps) (see e.g. MultiGBASE-T AN control 1 register)
- BASE-T1
- 10BASE-T1
- NGBASE-T1

And surely there are some on the way or already there that I am not 
aware of.

To me, one architectural decision point is if you want to have generic 
support for all C45 PHYs in one file, or if you want to split it by 
device class. I went down the first path with my patch, as this is the 
road gone also with the existing code.

If you want to split BASE-T1, i think you will need one basic C45 
library (genphy_c45_pma_read_abilities() is a good example of a function 
that is not specific to a device class). On the other hand, 
genphy_c45_pma_setup_forced() is not a generic function at this point as 
it supports only a subset of devices managed in C45.

I tend to agree with you that splitting is the best way to go in the 
long run, but that also requires a split of the existing phy-c45.c into 
two IMHO.