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

[Christian Herber <christian.herber@nxp.com>]

On 24.08.2019 17:03, Heiner Kallweit wrote:
> 
> 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.

Clause 45 defines e.g. bit 7.1.2 just like it defines the BASE-T1 
auto-neg registers. Any bit that i have used in my patch is 100% 
standardized in IEEE 802.3-2018, Clause 45. By definition, BASE-T1 PHYs 
have to use the Clause 45 BASE-T1 registers, otherwise they are not IEEE 
compliant.

> 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.

For now, there is no such public data sheet. However, IEEE 802.3-2018 is 
public. This should be the basis for a generic driver. Datasheets are 
needed for the device specific drivers. If Linux cares to support 
BASE-T1, it should implement a driver that works with a standard 
compliant PHY and that can be done on the basis of IEEE.

> 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
> 

There is not requirement in IEEE to shadow the BASE-T1 registers into 
the "standard" ones. Thus, such an assumption should not be done for a 
generic driver, as it will quite certainly not work with all devices. 
Fyi, both registers are standard, just that the historically first ones 
are for classic 10/100/1000/10000 PHYs and BASE-T1 registers are for the 
single twisted pair PHYs.