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 0/1] Add BASE-T1 PHY support
To:     Christian Herber <christian.herber@nxp.com>,
        Andrew Lunn <andrew@lunn.ch>
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>
References: <20190815153209.21529-1-christian.herber@nxp.com>
 <8c15b855-6947-9930-c3df-71a64fbff33b@gmail.com>
 <AM6PR0402MB379864B810F08D3698618B5F86A80@AM6PR0402MB3798.eurprd04.prod.outlook.com>
 <13e65051-fe4f-5964-30b3-75285e6d2eee@gmail.com>
 <AM6PR0402MB3798FCBF1EE592687B13A3C386AB0@AM6PR0402MB3798.eurprd04.prod.outlook.com>
 <5c920846-b8f5-d087-cea4-a8ca3f816127@gmail.com>
 <20190821185715.GA16401@lunn.ch>
 <AM6PR0402MB3798C702793071E34A5659ED86A50@AM6PR0402MB3798.eurprd04.prod.outlook.com>
From:   Heiner Kallweit <hkallweit1@gmail.com>
Message-ID: <1f50cdcf-200d-7c25-35ae-aee011a6a520@gmail.com>
Date:   Sat, 24 Aug 2019 17:03:33 +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: <AM6PR0402MB3798C702793071E34A5659ED86A50@AM6PR0402MB3798.eurprd04.prod.outlook.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 22.08.2019 09:18, Christian Herber wrote:
> 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.
> 
BASE-T1 seems to be based on Clause 45 (at least Clause 45 MDIO),
but it's not fully compliant with Clause 45. Taking AN link status
as an example: 45.2.7.2.7 states that link-up is signaled in bit 7.1.2.
If BASE-T1 uses a different register, then it's not fully Clause 45
compatible.
Therefore also my question for the datasheet of an actual BASE-T1 PHY,
as I would be curious whether it shadows the link-up bit from 7.513.2
to 7.1.2 to be Clause 45 compliant. Definitely reading bit 7.513.2
is nothing that belongs into a genphy_c45_ function.

The extension to genphy_c45_pma_read_abilities() looks good to me,
for the other parts I'd like to see first how real world BASE-T1 PHYs
handle it. If they shadow the T1-specific bits to the Clause 45
standard ones, we should be fine. Otherwise IMO we have to add
separate T1 functions to phylib.

Heiner