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

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

[Christian Herber]

On October 16, 2019 10:37:30 AM Lucas Stach <l.stach@pengutronix.de> wrote:

> On Fr, 2019-08-16 at 22:59 +0200, Heiner Kallweit wrote:
>> On 15.08.2019 17:32, Christian Herber wrote:
>> > This patch adds basic support for BASE-T1 PHYs in the framework.
>> > BASE-T1 PHYs main area of application are automotive and industrial.
>> > BASE-T1 is standardized in IEEE 802.3, namely
>> > - IEEE 802.3bw: 100BASE-T1
>> > - IEEE 802.3bp 1000BASE-T1
>> > - IEEE 802.3cg: 10BASE-T1L and 10BASE-T1S
>> >
>> > There are no products which contain BASE-T1 and consumer type PHYs like
>> > 1000BASE-T. However, devices exist which combine 100BASE-T1 and 1000BASE-T1
>> > PHYs with auto-negotiation.
>>
>> Is this meant in a way that *currently* there are no PHY's combining Base-T1
>> with normal Base-T modes? Or are there reasons why this isn't possible in
>> general? I'm asking because we have PHY's combining copper and fiber, and e.g.
>> the mentioned Aquantia PHY that combines NBase-T with 1000Base-T2.
>
> There are PHYs combining both Base-T1 and other Base-T capabilities.
> E.g. the Broadcom BCM54811 support both Base-T1, as well as 1000BASE-T
> and 100BASE-TX.
>
> Regards,
> Lucas

Interesting. To be precise, the device supports BroadR-Reach and not 100BASE-T1 according to their product brief. Therefore I would assume that the registers also do not follow the IEEE defined Clause 45 layout. While we cannot learn much then how to handle such devices in a generic clause 45 driver, this suggests that such devices can appear.

Still, IEEE 802.3 does not really define a way that these devices would coexist, so I would assume it is pretty much two PHYs in one IC and you can chose which flavor you want. Is my assumption on that correct?

In that case, you would have to structure the drivers in a way the you have separate functions for the standalone flavors of Clause 45 managed PHYs, while making as much reuse as possible between them.

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

[Christian Herber]

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.

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

[Christian Herber]

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.

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

[Christian Herber]

On 16.08.2019 22:59, Heiner Kallweit wrote:
> On 15.08.2019 17:32, Christian Herber wrote:
>> This patch adds basic support for BASE-T1 PHYs in the framework.
>> BASE-T1 PHYs main area of application are automotive and industrial.
>> BASE-T1 is standardized in IEEE 802.3, namely
>> - IEEE 802.3bw: 100BASE-T1
>> - IEEE 802.3bp 1000BASE-T1
>> - IEEE 802.3cg: 10BASE-T1L and 10BASE-T1S
>>
>> There are no products which contain BASE-T1 and consumer type PHYs like
>> 1000BASE-T. However, devices exist which combine 100BASE-T1 and 1000BASE-T1
>> PHYs with auto-negotiation.
> 
> Is this meant in a way that *currently* there are no PHY's combining Base-T1
> with normal Base-T modes? Or are there reasons why this isn't possible in
> general? I'm asking because we have PHY's combining copper and fiber, and e.g.
> the mentioned Aquantia PHY that combines NBase-T with 1000Base-T2.
> 
>>
>> The intention of this patch is to make use of the existing Clause 45 functions.
>> BASE-T1 adds some additional registers e.g. for aneg control, which follow a
>> similiar register layout as the existing devices. The bits which are used in
>> BASE-T1 specific registers are the same as in basic registers, thus the
>> existing functions can be resued, with get_aneg_ctrl() selecting the correct
>> register address.
>>
> If Base-T1 can't be combined with other modes then at a first glance I see no
> benefit in defining new registers e.g. for aneg control, and the standard ones
> are unused. Why not using the standard registers? Can you shed some light on that?
> 
> Are the new registers internally shadowed to the standard location?
> That's something I've seen on other PHY's: one register appears in different
> places in different devices.
> 
>> The current version of ethtool has been prepared for 100/1000BASE-T1 and works
>> with this patch. 10BASE-T1 needs to be added to ethtool.
>>
>> Christian Herber (1):
>>    Added BASE-T1 PHY support to PHY Subsystem
>>
>>   drivers/net/phy/phy-c45.c    | 113 +++++++++++++++++++++++++++++++----
>>   drivers/net/phy/phy-core.c   |   4 +-
>>   include/uapi/linux/ethtool.h |   2 +
>>   include/uapi/linux/mdio.h    |  21 +++++++
>>   4 files changed, 129 insertions(+), 11 deletions(-)
>>
> 
> Heiner
> 

Hi Heiner,

I do not think the Aquantia part you are describing is publicly 
documented, so i cannot comment on that part.
There are multiple reasons why e.g. xBASE-T1 plus 1000BASE-T is 
unlikely. First, the is no use-case known to me, where this would be 
required. Second, there is no way that you can do an auto-negotiation 
between the two, as these both have their own auto-neg defined (Clause 
28/73 vs. Clause 98). Thirdly, if you would ever have a product with 
both, I believe it would just include two full PHYs and a way to select 
which flavor you want. Of course, this is the theory until proven 
otherwise, but to me it is sufficient to use a single driver.

The registers are different in the fields they include. It is just that 
the flags which are used by the Linux driver, like restarting auto-neg, 
are at the same position.

Christian