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

[Heiner Kallweit <hkallweit1@gmail.com>]

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