[PATCH net-next 0/2] net: phy: aquantia: indicate rate adaptation

[PATCH net-next 0/2] net: phy: aquantia: indicate rate adaptation

[Madalin Bucur]

This patch-set introduces a bit into the phy_device
structure to indicate the PHYs ability to do rate
adaptation between the system and line interfaces and
sets this bit for the Aquantia PHYs that have this feature.

By taking into account the presence of the said bit, address
an issue with the LS1046ARDB board 10G interface no longer
working with an 1G link partner after autonegotiation support
was added for the Aquantia PHY on board in

commit 09c4c57f7bc4 ("net: phy: aquantia: add support for auto-negotiation configuration")

As it only worked in other modes besides 10G because the PHY
was not configured by its driver to remove them, this is not
really a bug fix but more of a feature add.

Reported-by: Mian Yousaf Kaukab <ykaukab@suse.de>

Madalin Bucur (2):
  net: phy: aquantia: add rate_adaptation indication
  dpaa_eth: support all modes with rate adapting PHYs

 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 10 +++++++---
 drivers/net/phy/aquantia_main.c                |  3 +++
 include/linux/phy.h                            |  3 +++
 3 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.1.0

[PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Madalin Bucur]

The AQR PHYs are able to perform rate adaptation between
the system interface and the line interfaces. When such
a PHY is deployed, the ethernet driver should not limit
the modes supported or advertised by the PHY. This patch
introduces the bit that allows checking for this feature
in the phy_device structure and its use for the Aquantia
PHYs.

Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
---
 drivers/net/phy/aquantia_main.c | 3 +++
 include/linux/phy.h             | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 975789d9349d..36fdd523b758 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -209,6 +209,9 @@ static int aqr_config_aneg(struct phy_device *phydev)
 	u16 reg;
 	int ret;
 
+	/* add here as this is called for all devices */
+	phydev->rate_adaptation = 1;
+
 	if (phydev->autoneg == AUTONEG_DISABLE)
 		return genphy_c45_pma_setup_forced(phydev);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index dd4a91f1feaa..2a5c202333fc 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -387,6 +387,9 @@ struct phy_device {
 	/* Interrupts are enabled */
 	unsigned interrupts:1;
 
+	/* Rate adaptation in the PHY */
+	unsigned rate_adaptation:1;
+
 	enum phy_state state;
 
 	u32 dev_flags;
-- 
2.1.0

Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Florian Fainelli]

On 1/22/20 5:59 AM, Madalin Bucur wrote:
> The AQR PHYs are able to perform rate adaptation between
> the system interface and the line interfaces. When such
> a PHY is deployed, the ethernet driver should not limit
> the modes supported or advertised by the PHY. This patch
> introduces the bit that allows checking for this feature
> in the phy_device structure and its use for the Aquantia
> PHYs.
> 
> Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> ---
>  drivers/net/phy/aquantia_main.c | 3 +++
>  include/linux/phy.h             | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
> index 975789d9349d..36fdd523b758 100644
> --- a/drivers/net/phy/aquantia_main.c
> +++ b/drivers/net/phy/aquantia_main.c
> @@ -209,6 +209,9 @@ static int aqr_config_aneg(struct phy_device *phydev)
>  	u16 reg;
>  	int ret;
>  
> +	/* add here as this is called for all devices */
> +	phydev->rate_adaptation = 1;

How about introducing a new PHY_SUPPORTS_RATE_ADAPTATION flag and you
set that directly from the phy_driver entry? using the "flags" bitmask
instead of adding another structure member to phy_device?

> +
>  	if (phydev->autoneg == AUTONEG_DISABLE)
>  		return genphy_c45_pma_setup_forced(phydev);
>  
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index dd4a91f1feaa..2a5c202333fc 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -387,6 +387,9 @@ struct phy_device {
>  	/* Interrupts are enabled */
>  	unsigned interrupts:1;
>  
> +	/* Rate adaptation in the PHY */
> +	unsigned rate_adaptation:1;
> +
>  	enum phy_state state;
>  
>  	u32 dev_flags;
> 


-- 
Florian

RE: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Florian Fainelli <f.fainelli@gmail.com>
> Sent: Wednesday, January 22, 2020 7:58 PM
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>; davem@davemloft.net
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
> ykaukab@suse.de
> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
> indication
> 
> On 1/22/20 5:59 AM, Madalin Bucur wrote:
> > The AQR PHYs are able to perform rate adaptation between
> > the system interface and the line interfaces. When such
> > a PHY is deployed, the ethernet driver should not limit
> > the modes supported or advertised by the PHY. This patch
> > introduces the bit that allows checking for this feature
> > in the phy_device structure and its use for the Aquantia
> > PHYs.
> >
> > Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> > ---
> >  drivers/net/phy/aquantia_main.c | 3 +++
> >  include/linux/phy.h             | 3 +++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/phy/aquantia_main.c
> b/drivers/net/phy/aquantia_main.c
> > index 975789d9349d..36fdd523b758 100644
> > --- a/drivers/net/phy/aquantia_main.c
> > +++ b/drivers/net/phy/aquantia_main.c
> > @@ -209,6 +209,9 @@ static int aqr_config_aneg(struct phy_device
> *phydev)
> >  	u16 reg;
> >  	int ret;
> >
> > +	/* add here as this is called for all devices */
> > +	phydev->rate_adaptation = 1;
> 
> How about introducing a new PHY_SUPPORTS_RATE_ADAPTATION flag and you
> set that directly from the phy_driver entry? using the "flags" bitmask
> instead of adding another structure member to phy_device?

I've looked at the phydev->dev_flags use, it seemed to me that mostly it
is used to convey configuration options towards the PHY. Another problem
is that it's meaning seems to be opaque,  PHY specific. I wanted to avoid
trampling on a certain PHY hardcoded value and I turned my attention to
the bit fields in the phy_device. I noticed that there are already 12 bits
so due to alignment, the added bit is not adding extra size to the struct.
 
> > +
> >  	if (phydev->autoneg == AUTONEG_DISABLE)
> >  		return genphy_c45_pma_setup_forced(phydev);
> >
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index dd4a91f1feaa..2a5c202333fc 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -387,6 +387,9 @@ struct phy_device {
> >  	/* Interrupts are enabled */
> >  	unsigned interrupts:1;
> >
> > +	/* Rate adaptation in the PHY */
> > +	unsigned rate_adaptation:1;
> > +
> >  	enum phy_state state;
> >
> >  	u32 dev_flags;
> >
> 
> 
> --
> Florian

Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Florian Fainelli]

On 1/22/20 11:38 PM, Madalin Bucur (OSS) wrote:
>> -----Original Message-----
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Sent: Wednesday, January 22, 2020 7:58 PM
>> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>; davem@davemloft.net
>> Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
>> ykaukab@suse.de
>> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
>> indication
>>
>> On 1/22/20 5:59 AM, Madalin Bucur wrote:
>>> The AQR PHYs are able to perform rate adaptation between
>>> the system interface and the line interfaces. When such
>>> a PHY is deployed, the ethernet driver should not limit
>>> the modes supported or advertised by the PHY. This patch
>>> introduces the bit that allows checking for this feature
>>> in the phy_device structure and its use for the Aquantia
>>> PHYs.
>>>
>>> Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
>>> ---
>>>  drivers/net/phy/aquantia_main.c | 3 +++
>>>  include/linux/phy.h             | 3 +++
>>>  2 files changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/aquantia_main.c
>> b/drivers/net/phy/aquantia_main.c
>>> index 975789d9349d..36fdd523b758 100644
>>> --- a/drivers/net/phy/aquantia_main.c
>>> +++ b/drivers/net/phy/aquantia_main.c
>>> @@ -209,6 +209,9 @@ static int aqr_config_aneg(struct phy_device
>> *phydev)
>>>  	u16 reg;
>>>  	int ret;
>>>
>>> +	/* add here as this is called for all devices */
>>> +	phydev->rate_adaptation = 1;
>>
>> How about introducing a new PHY_SUPPORTS_RATE_ADAPTATION flag and you
>> set that directly from the phy_driver entry? using the "flags" bitmask
>> instead of adding another structure member to phy_device?
> 
> I've looked at the phydev->dev_flags use, it seemed to me that mostly it
> is used to convey configuration options towards the PHY.

You read me incorrectly, I am suggesting using the phy_driver::flags
member, not the phy_device::dev_flags entry, please re-consider your
position.

> Another problem
> is that it's meaning seems to be opaque,  PHY specific. I wanted to avoid
> trampling on a certain PHY hardcoded value and I turned my attention to
> the bit fields in the phy_device. I noticed that there are already 12 bits
> so due to alignment, the added bit is not adding extra size to the struct.
>  
>>> +
>>>  	if (phydev->autoneg == AUTONEG_DISABLE)
>>>  		return genphy_c45_pma_setup_forced(phydev);
>>>
>>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>>> index dd4a91f1feaa..2a5c202333fc 100644
>>> --- a/include/linux/phy.h
>>> +++ b/include/linux/phy.h
>>> @@ -387,6 +387,9 @@ struct phy_device {
>>>  	/* Interrupts are enabled */
>>>  	unsigned interrupts:1;
>>>
>>> +	/* Rate adaptation in the PHY */
>>> +	unsigned rate_adaptation:1;
>>> +
>>>  	enum phy_state state;
>>>
>>>  	u32 dev_flags;
>>>
>>
>>
>> --
>> Florian


-- 
Florian

RE: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Florian Fainelli <f.fainelli@gmail.com>
> Sent: Monday, January 27, 2020 7:43 PM
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>; davem@davemloft.net
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
> ykaukab@suse.de
> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
> indication
> 
> On 1/22/20 11:38 PM, Madalin Bucur (OSS) wrote:
> >> -----Original Message-----
> >> From: Florian Fainelli <f.fainelli@gmail.com>
> >> Sent: Wednesday, January 22, 2020 7:58 PM
> >> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>;
> davem@davemloft.net
> >> Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
> >> ykaukab@suse.de
> >> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add
> rate_adaptation
> >> indication
> >>
> >> On 1/22/20 5:59 AM, Madalin Bucur wrote:
> >>> The AQR PHYs are able to perform rate adaptation between
> >>> the system interface and the line interfaces. When such
> >>> a PHY is deployed, the ethernet driver should not limit
> >>> the modes supported or advertised by the PHY. This patch
> >>> introduces the bit that allows checking for this feature
> >>> in the phy_device structure and its use for the Aquantia
> >>> PHYs.
> >>>
> >>> Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> >>> ---
> >>>  drivers/net/phy/aquantia_main.c | 3 +++
> >>>  include/linux/phy.h             | 3 +++
> >>>  2 files changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/net/phy/aquantia_main.c
> >> b/drivers/net/phy/aquantia_main.c
> >>> index 975789d9349d..36fdd523b758 100644
> >>> --- a/drivers/net/phy/aquantia_main.c
> >>> +++ b/drivers/net/phy/aquantia_main.c
> >>> @@ -209,6 +209,9 @@ static int aqr_config_aneg(struct phy_device
> >> *phydev)
> >>>  	u16 reg;
> >>>  	int ret;
> >>>
> >>> +	/* add here as this is called for all devices */
> >>> +	phydev->rate_adaptation = 1;
> >>
> >> How about introducing a new PHY_SUPPORTS_RATE_ADAPTATION flag and you
> >> set that directly from the phy_driver entry? using the "flags" bitmask
> >> instead of adding another structure member to phy_device?
> >
> > I've looked at the phydev->dev_flags use, it seemed to me that mostly it
> > is used to convey configuration options towards the PHY.
> 
> You read me incorrectly, I am suggesting using the phy_driver::flags
> member, not the phy_device::dev_flags entry, please re-consider your
> position.

Sorry, I was looking at the wrong flags (thinking this is a PHY device feature,
not a PHY driver feature).

I've looked at PHY_IS_INTERNAL as a reference and found it's used like this:

        if (phydrv->flags & PHY_IS_INTERNAL)
                phydev->is_internal = true;

it just gets mirrored in a phy_device bit so I concluded this bit could fit
there just as well, that's how I came up with the current proposal.

There is ample space unused in the phy_driver flags, so I can add the bit there.
Should I add accessors too?

> > Another problem
> > is that it's meaning seems to be opaque,  PHY specific. I wanted to avoid
> > trampling on a certain PHY hardcoded value and I turned my attention to
> > the bit fields in the phy_device. I noticed that there are already 12
> > bits so due to alignment, the added bit is not adding extra size to the
> > struct.
> >
> >>> +
> >>>  	if (phydev->autoneg == AUTONEG_DISABLE)
> >>>  		return genphy_c45_pma_setup_forced(phydev);
> >>>
> >>> diff --git a/include/linux/phy.h b/include/linux/phy.h
> >>> index dd4a91f1feaa..2a5c202333fc 100644
> >>> --- a/include/linux/phy.h
> >>> +++ b/include/linux/phy.h
> >>> @@ -387,6 +387,9 @@ struct phy_device {
> >>>  	/* Interrupts are enabled */
> >>>  	unsigned interrupts:1;
> >>>
> >>> +	/* Rate adaptation in the PHY */
> >>> +	unsigned rate_adaptation:1;
> >>> +
> >>>  	enum phy_state state;
> >>>
> >>>  	u32 dev_flags;
> >>>
> >>
> >>
> >> --
> >> Florian
> 
> 
> --
> Florian

Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Vladimir Oltean]

Hi Florian,

On Mon, 27 Jan 2020 at 19:44, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 1/22/20 11:38 PM, Madalin Bucur (OSS) wrote:
> >> -----Original Message-----
> >> From: Florian Fainelli <f.fainelli@gmail.com>
> >> Sent: Wednesday, January 22, 2020 7:58 PM
> >> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>; davem@davemloft.net
> >> Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
> >> ykaukab@suse.de
> >> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
> >> indication
> >>
> >> On 1/22/20 5:59 AM, Madalin Bucur wrote:
> >>> The AQR PHYs are able to perform rate adaptation between
> >>> the system interface and the line interfaces. When such
> >>> a PHY is deployed, the ethernet driver should not limit
> >>> the modes supported or advertised by the PHY. This patch
> >>> introduces the bit that allows checking for this feature
> >>> in the phy_device structure and its use for the Aquantia
> >>> PHYs.
> >>>
> >>> Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> >>> ---
> >>>  drivers/net/phy/aquantia_main.c | 3 +++
> >>>  include/linux/phy.h             | 3 +++
> >>>  2 files changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/net/phy/aquantia_main.c
> >> b/drivers/net/phy/aquantia_main.c
> >>> index 975789d9349d..36fdd523b758 100644
> >>> --- a/drivers/net/phy/aquantia_main.c
> >>> +++ b/drivers/net/phy/aquantia_main.c
> >>> @@ -209,6 +209,9 @@ static int aqr_config_aneg(struct phy_device
> >> *phydev)
> >>>     u16 reg;
> >>>     int ret;
> >>>
> >>> +   /* add here as this is called for all devices */
> >>> +   phydev->rate_adaptation = 1;
> >>
> >> How about introducing a new PHY_SUPPORTS_RATE_ADAPTATION flag and you
> >> set that directly from the phy_driver entry? using the "flags" bitmask
> >> instead of adding another structure member to phy_device?
> >
> > I've looked at the phydev->dev_flags use, it seemed to me that mostly it
> > is used to convey configuration options towards the PHY.
>
> You read me incorrectly, I am suggesting using the phy_driver::flags
> member, not the phy_device::dev_flags entry, please re-consider your
> position.
>

Whether the PHY performs rate adaptation is a dynamic property.
It will perform it at wire speeds lower than 2500Mbps (1000/100) when
system side is 2500Base-X, but not at wire speed 2500 & 2500Base-X,
and not at wire speed 1000 & USXGMII.
You can't really encode that in phydev->flags.

I was actually searching for a way to encode some more PHY capabilities:
- Does it work with in-band autoneg enabled?
- Does it work with in-band autoneg bypassed?
- Does it emit pause frames? <- Madalin got ahead of me on this one.

For the first 2, I want a mechanism for the PHY library to figure out
whether the MAC and the PHY should use in-band autoneg or not. If both
support in-band autoneg, that would be preferred. Otherwise,
out-of-band (MDIO) is preferred, and in-band autoneg is explicitly
disabled in both, if that is claimed to be supported. Otherwise,
report error to user. Yes, this deprecates "managed =
'in-band-status'" in the device tree, and that's for (what I think is)
the better. We can make "managed = 'in-band-status'" to just clear the
MAC's ability to support the "in-band autoneg bypassed" mode.

So I thought a function pointer in the phy driver would be better, and
more extensible.
Thoughts?

> --
> Florian

Regards,
-Vladimir

RE: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Tuesday, January 28, 2020 5:55 PM
> To: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>; davem@davemloft.net;
> andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
> ykaukab@suse.de
> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
> indication
> 
> Hi Florian,
> 
> On Mon, 27 Jan 2020 at 19:44, Florian Fainelli <f.fainelli@gmail.com>
> wrote:
> >
> > On 1/22/20 11:38 PM, Madalin Bucur (OSS) wrote:
> > >> -----Original Message-----
> > >> From: Florian Fainelli <f.fainelli@gmail.com>
> > >> Sent: Wednesday, January 22, 2020 7:58 PM
> > >> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>;
> davem@davemloft.net
> > >> Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
> > >> ykaukab@suse.de
> > >> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add
> rate_adaptation
> > >> indication
> > >>
> > >> On 1/22/20 5:59 AM, Madalin Bucur wrote:
> > >>> The AQR PHYs are able to perform rate adaptation between
> > >>> the system interface and the line interfaces. When such
> > >>> a PHY is deployed, the ethernet driver should not limit
> > >>> the modes supported or advertised by the PHY. This patch
> > >>> introduces the bit that allows checking for this feature
> > >>> in the phy_device structure and its use for the Aquantia
> > >>> PHYs.
> > >>>
> > >>> Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> > >>> ---
> > >>>  drivers/net/phy/aquantia_main.c | 3 +++
> > >>>  include/linux/phy.h             | 3 +++
> > >>>  2 files changed, 6 insertions(+)
> > >>>
> > >>> diff --git a/drivers/net/phy/aquantia_main.c
> > >> b/drivers/net/phy/aquantia_main.c
> > >>> index 975789d9349d..36fdd523b758 100644
> > >>> --- a/drivers/net/phy/aquantia_main.c
> > >>> +++ b/drivers/net/phy/aquantia_main.c
> > >>> @@ -209,6 +209,9 @@ static int aqr_config_aneg(struct phy_device
> > >> *phydev)
> > >>>     u16 reg;
> > >>>     int ret;
> > >>>
> > >>> +   /* add here as this is called for all devices */
> > >>> +   phydev->rate_adaptation = 1;
> > >>
> > >> How about introducing a new PHY_SUPPORTS_RATE_ADAPTATION flag and
> you
> > >> set that directly from the phy_driver entry? using the "flags"
> bitmask
> > >> instead of adding another structure member to phy_device?
> > >
> > > I've looked at the phydev->dev_flags use, it seemed to me that mostly
> it
> > > is used to convey configuration options towards the PHY.
> >
> > You read me incorrectly, I am suggesting using the phy_driver::flags
> > member, not the phy_device::dev_flags entry, please re-consider your
> > position.
> >
> 
> Whether the PHY performs rate adaptation is a dynamic property.
> It will perform it at wire speeds lower than 2500Mbps (1000/100) when
> system side is 2500Base-X, but not at wire speed 2500 & 2500Base-X,
> and not at wire speed 1000 & USXGMII.
> You can't really encode that in phydev->flags.

Vladimir, the patch adds a bit that indicates the PHY ability to perform
rate adaptation, not whether it is actually in use in a certain combination
of system interface and line interface modes. Please review the submission
again, I understand you have something slightly different in mind, but this
is just addressing a basic need of knowing whether there is a chance the
line side could work at other speeds than the system interface and allow it
to do so.
 
> I was actually searching for a way to encode some more PHY capabilities:
> - Does it work with in-band autoneg enabled?
> - Does it work with in-band autoneg bypassed?
> - Does it emit pause frames? <- Madalin got ahead of me on this one.
> 
> For the first 2, I want a mechanism for the PHY library to figure out
> whether the MAC and the PHY should use in-band autoneg or not. If both
> support in-band autoneg, that would be preferred. Otherwise,
> out-of-band (MDIO) is preferred, and in-band autoneg is explicitly
> disabled in both, if that is claimed to be supported. Otherwise,
> report error to user. Yes, this deprecates "managed =
> 'in-band-status'" in the device tree, and that's for (what I think is)
> the better. We can make "managed = 'in-band-status'" to just clear the
> MAC's ability to support the "in-band autoneg bypassed" mode.
> 
> So I thought a function pointer in the phy driver would be better, and
> more extensible.
> Thoughts?

This is where you get when you try to implement anti-stupid devices, it gets
complicated fast and, most often, it gets in the way. Should someone change
a setting (pause settings) and experience adverse effects (excessive frame
loss), we should rely on his analytical abilities to connect the dots, imho.

Madalin

 
> > --
> > Florian
> 
> Regards,
> -Vladimir

Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Vladimir Oltean]

Hi Madalin,

On Wed, 29 Jan 2020 at 11:38, Madalin Bucur (OSS)
<madalin.bucur@oss.nxp.com> wrote:
>
> > -----Original Message-----
> > From: Vladimir Oltean <olteanv@gmail.com>
> > Sent: Tuesday, January 28, 2020 5:55 PM
> > To: Florian Fainelli <f.fainelli@gmail.com>
> > Cc: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>; davem@davemloft.net;
> > andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
> > ykaukab@suse.de
> > Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
> > indication
> >
> > Hi Florian,
> >
> > On Mon, 27 Jan 2020 at 19:44, Florian Fainelli <f.fainelli@gmail.com>
> > wrote:
> > >
> > > On 1/22/20 11:38 PM, Madalin Bucur (OSS) wrote:
> > > >> -----Original Message-----
> > > >> From: Florian Fainelli <f.fainelli@gmail.com>
> > > >> Sent: Wednesday, January 22, 2020 7:58 PM
> > > >> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>;
> > davem@davemloft.net
> > > >> Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
> > > >> ykaukab@suse.de
> > > >> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add
> > rate_adaptation
> > > >> indication
> > > >>
> > > >> On 1/22/20 5:59 AM, Madalin Bucur wrote:
> > > >>> The AQR PHYs are able to perform rate adaptation between
> > > >>> the system interface and the line interfaces. When such
> > > >>> a PHY is deployed, the ethernet driver should not limit
> > > >>> the modes supported or advertised by the PHY. This patch
> > > >>> introduces the bit that allows checking for this feature
> > > >>> in the phy_device structure and its use for the Aquantia
> > > >>> PHYs.
> > > >>>
> > > >>> Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> > > >>> ---
> > > >>>  drivers/net/phy/aquantia_main.c | 3 +++
> > > >>>  include/linux/phy.h             | 3 +++
> > > >>>  2 files changed, 6 insertions(+)
> > > >>>
> > > >>> diff --git a/drivers/net/phy/aquantia_main.c
> > > >> b/drivers/net/phy/aquantia_main.c
> > > >>> index 975789d9349d..36fdd523b758 100644
> > > >>> --- a/drivers/net/phy/aquantia_main.c
> > > >>> +++ b/drivers/net/phy/aquantia_main.c
> > > >>> @@ -209,6 +209,9 @@ static int aqr_config_aneg(struct phy_device
> > > >> *phydev)
> > > >>>     u16 reg;
> > > >>>     int ret;
> > > >>>
> > > >>> +   /* add here as this is called for all devices */
> > > >>> +   phydev->rate_adaptation = 1;
> > > >>
> > > >> How about introducing a new PHY_SUPPORTS_RATE_ADAPTATION flag and
> > you
> > > >> set that directly from the phy_driver entry? using the "flags"
> > bitmask
> > > >> instead of adding another structure member to phy_device?
> > > >
> > > > I've looked at the phydev->dev_flags use, it seemed to me that mostly
> > it
> > > > is used to convey configuration options towards the PHY.
> > >
> > > You read me incorrectly, I am suggesting using the phy_driver::flags
> > > member, not the phy_device::dev_flags entry, please re-consider your
> > > position.
> > >
> >
> > Whether the PHY performs rate adaptation is a dynamic property.
> > It will perform it at wire speeds lower than 2500Mbps (1000/100) when
> > system side is 2500Base-X, but not at wire speed 2500 & 2500Base-X,
> > and not at wire speed 1000 & USXGMII.
> > You can't really encode that in phydev->flags.
>
> Vladimir, the patch adds a bit that indicates the PHY ability to perform
> rate adaptation, not whether it is actually in use in a certain combination
> of system interface and line interface modes. Please review the submission

As far as I understand, for Aquantia devices this is a 3-way switch for:
- No rate adaptation
- USX rate adaptation
- Pause rate adaptation.
So what does your "phydev->rate_adaptation = 1" assignment mean?

From context and datasheet I deduced that you mean "the PHY is
configured to send PAUSE frames for 10GBase-R and 2500Base-X modes",
which are probably the modes in which you're using it.

But how do you _know_ that the PHY has this switch set correctly? It
is either set by firmware or by MDIO, but I don't see any of that
being checked for.

> again, I understand you have something slightly different in mind, but this
> is just addressing a basic need of knowing whether there is a chance the
> line side could work at other speeds than the system interface and allow it
> to do so.

I do understand this works, when it works, _for your board_, but is it
generic enough that other systems can work with this simple setting?
What if somebody else reads "phydev->rate_adaptation" to mean "USX
adaptation"?

>
> > I was actually searching for a way to encode some more PHY capabilities:
> > - Does it work with in-band autoneg enabled?
> > - Does it work with in-band autoneg bypassed?
> > - Does it emit pause frames? <- Madalin got ahead of me on this one.
> >
> > For the first 2, I want a mechanism for the PHY library to figure out
> > whether the MAC and the PHY should use in-band autoneg or not. If both
> > support in-band autoneg, that would be preferred. Otherwise,
> > out-of-band (MDIO) is preferred, and in-band autoneg is explicitly
> > disabled in both, if that is claimed to be supported. Otherwise,
> > report error to user. Yes, this deprecates "managed =
> > 'in-band-status'" in the device tree, and that's for (what I think is)
> > the better. We can make "managed = 'in-band-status'" to just clear the
> > MAC's ability to support the "in-band autoneg bypassed" mode.
> >
> > So I thought a function pointer in the phy driver would be better, and
> > more extensible.
> > Thoughts?
>
> This is where you get when you try to implement anti-stupid devices, it gets
> complicated fast and, most often, it gets in the way. Should someone change
> a setting (pause settings) and experience adverse effects (excessive frame
> loss), we should rely on his analytical abilities to connect the dots, imho.
>

So you think that having a PHY firmware with rate adaptation disabled
is "stupid user"?
What if the rate adaptation feature is not enabled in firmware, but is
being enabled by U-Boot, but the user had the generic PHY driver
instead of the Aquantia one, or used a different or old bootloader?
"Stupid user"?
The PHY and the Ethernet driver are strongly decoupled, so they need
to agree on an operating mode that will work for both of them.
Ideally the PHY would really know how it's configured, not just
hardcode "yeah, I can do rate adaptation, try me".
The fact that you can build sanity checks on top (like in this case
not allowing the user to disable pause frames in the Ethernet driver
on RX), that don't stand in the way, is just proof that the system is
well designed. If you can't build sanity checks, or if they stand in
the way, it isn't.

If anything, why haven't you considered the opposite logic here:
- Ethernet driver (dpaa_eth) supports all speeds 10G and below.
- PHY driver (or PHY core) removes the supported and advertised speeds
for anything other than 2.5G and 10G if it can't do this rate
adaptation thing, and its system side isn't USXGMII. It's not like
this is dpaa_eth specific in any way.

> Madalin
>
>
> > > --
> > > Florian
> >
> > Regards,
> > -Vladimir

-Vladimir

RE: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Wednesday, January 29, 2020 12:15 PM
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>; davem@davemloft.net;
> andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
> ykaukab@suse.de; Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
> indication
> 
> Hi Madalin,
> 
> On Wed, 29 Jan 2020 at 11:38, Madalin Bucur (OSS)
> <madalin.bucur@oss.nxp.com> wrote:
> >
> > > -----Original Message-----
> > > From: Vladimir Oltean <olteanv@gmail.com>
> > > Sent: Tuesday, January 28, 2020 5:55 PM
> > > To: Florian Fainelli <f.fainelli@gmail.com>
> > > Cc: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>;
> davem@davemloft.net;
> > > andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
> > > ykaukab@suse.de
> > > Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add
> rate_adaptation
> > > indication
> > >
> > > Hi Florian,
> > >
> > > On Mon, 27 Jan 2020 at 19:44, Florian Fainelli <f.fainelli@gmail.com>
> > > wrote:
> > > >
> > > > On 1/22/20 11:38 PM, Madalin Bucur (OSS) wrote:
> > > > >> -----Original Message-----
> > > > >> From: Florian Fainelli <f.fainelli@gmail.com>
> > > > >> Sent: Wednesday, January 22, 2020 7:58 PM
> > > > >> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>;
> > > davem@davemloft.net
> > > > >> Cc: andrew@lunn.ch; hkallweit1@gmail.com;
> netdev@vger.kernel.org;
> > > > >> ykaukab@suse.de
> > > > >> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add
> > > rate_adaptation
> > > > >> indication
> > > > >>
> > > > >> On 1/22/20 5:59 AM, Madalin Bucur wrote:
> > > > >>> The AQR PHYs are able to perform rate adaptation between
> > > > >>> the system interface and the line interfaces. When such
> > > > >>> a PHY is deployed, the ethernet driver should not limit
> > > > >>> the modes supported or advertised by the PHY. This patch
> > > > >>> introduces the bit that allows checking for this feature
> > > > >>> in the phy_device structure and its use for the Aquantia
> > > > >>> PHYs.
> > > > >>>
> > > > >>> Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> > > > >>> ---
> > > > >>>  drivers/net/phy/aquantia_main.c | 3 +++
> > > > >>>  include/linux/phy.h             | 3 +++
> > > > >>>  2 files changed, 6 insertions(+)
> > > > >>>
> > > > >>> diff --git a/drivers/net/phy/aquantia_main.c
> > > > >> b/drivers/net/phy/aquantia_main.c
> > > > >>> index 975789d9349d..36fdd523b758 100644
> > > > >>> --- a/drivers/net/phy/aquantia_main.c
> > > > >>> +++ b/drivers/net/phy/aquantia_main.c
> > > > >>> @@ -209,6 +209,9 @@ static int aqr_config_aneg(struct
> phy_device
> > > > >> *phydev)
> > > > >>>     u16 reg;
> > > > >>>     int ret;
> > > > >>>
> > > > >>> +   /* add here as this is called for all devices */
> > > > >>> +   phydev->rate_adaptation = 1;
> > > > >>
> > > > >> How about introducing a new PHY_SUPPORTS_RATE_ADAPTATION flag
> and
> > > you
> > > > >> set that directly from the phy_driver entry? using the "flags"
> > > bitmask
> > > > >> instead of adding another structure member to phy_device?
> > > > >
> > > > > I've looked at the phydev->dev_flags use, it seemed to me that
> mostly
> > > it
> > > > > is used to convey configuration options towards the PHY.
> > > >
> > > > You read me incorrectly, I am suggesting using the
> phy_driver::flags
> > > > member, not the phy_device::dev_flags entry, please re-consider
> your
> > > > position.
> > > >
> > >
> > > Whether the PHY performs rate adaptation is a dynamic property.
> > > It will perform it at wire speeds lower than 2500Mbps (1000/100) when
> > > system side is 2500Base-X, but not at wire speed 2500 & 2500Base-X,
> > > and not at wire speed 1000 & USXGMII.
> > > You can't really encode that in phydev->flags.
> >
> > Vladimir, the patch adds a bit that indicates the PHY ability to
> perform
> > rate adaptation, not whether it is actually in use in a certain
> combination
> > of system interface and line interface modes. Please review the
> submission
> 
> As far as I understand, for Aquantia devices this is a 3-way switch for:
> - No rate adaptation
> - USX rate adaptation
> - Pause rate adaptation.
> So what does your "phydev->rate_adaptation = 1" assignment mean?

phydev->rate_adaptation = 1 means the PHY is able to perform rate adaptation

There is not such thing as USX, if you refer to USXGMII, that's something you
can check for in the interface mode.

> From context and datasheet I deduced that you mean "the PHY is
> configured to send PAUSE frames for 10GBase-R and 2500Base-X modes",
> which are probably the modes in which you're using it.
> 
> But how do you _know_ that the PHY has this switch set correctly? It
> is either set by firmware or by MDIO, but I don't see any of that
> being checked for.

I know it is set because someone does put some work in designing a system,
in provisioning a correct firmware image.

> > again, I understand you have something slightly different in mind, but this
> > is just addressing a basic need of knowing whether there is a chance the
> > line side could work at other speeds than the system interface and allow it
> > to do so.
> 
> I do understand this works, when it works, _for your board_, but is it
> generic enough that other systems can work with this simple setting?
> What if somebody else reads "phydev->rate_adaptation" to mean "USX
> adaptation"?

That person would be wrong, as there is a dedicated interface mode for USXGMII
(I assume that's what you're trying to refer to here).

> >
> > > I was actually searching for a way to encode some more PHY
> capabilities:
> > > - Does it work with in-band autoneg enabled?
> > > - Does it work with in-band autoneg bypassed?
> > > - Does it emit pause frames? <- Madalin got ahead of me on this one.
> > >
> > > For the first 2, I want a mechanism for the PHY library to figure out
> > > whether the MAC and the PHY should use in-band autoneg or not. If
> both
> > > support in-band autoneg, that would be preferred. Otherwise,
> > > out-of-band (MDIO) is preferred, and in-band autoneg is explicitly
> > > disabled in both, if that is claimed to be supported. Otherwise,
> > > report error to user. Yes, this deprecates "managed =
> > > 'in-band-status'" in the device tree, and that's for (what I think
> is)
> > > the better. We can make "managed = 'in-band-status'" to just clear
> the
> > > MAC's ability to support the "in-band autoneg bypassed" mode.
> > >
> > > So I thought a function pointer in the phy driver would be better,
> and
> > > more extensible.
> > > Thoughts?
> >
> > This is where you get when you try to implement anti-stupid devices, it
> gets
> > complicated fast and, most often, it gets in the way. Should someone
> change
> > a setting (pause settings) and experience adverse effects (excessive
> frame
> > loss), we should rely on his analytical abilities to connect the dots,
> imho.
> >
> 
> So you think that having a PHY firmware with rate adaptation disabled
> is "stupid user"?
> What if the rate adaptation feature is not enabled in firmware, but is
> being enabled by U-Boot, but the user had the generic PHY driver
> instead of the Aquantia one, or used a different or old bootloader?
> "Stupid user"?

Disabling rate adaptation is one of so many ways one could cripple a
system interface, there are many that require polarity inversion, lane
switching, etc. and still there is no support for that in the kernel.

> The PHY and the Ethernet driver are strongly decoupled, so they need
> to agree on an operating mode that will work for both of them.
> Ideally the PHY would really know how it's configured, not just
> hardcode "yeah, I can do rate adaptation, try me".
> The fact that you can build sanity checks on top (like in this case
> not allowing the user to disable pause frames in the Ethernet driver
> on RX), that don't stand in the way, is just proof that the system is
> well designed. If you can't build sanity checks, or if they stand in
> the way, it isn't.

If you don't need them, it's even better.

> If anything, why haven't you considered the opposite logic here:
> - Ethernet driver (dpaa_eth) supports all speeds 10G and below.
> - PHY driver (or PHY core) removes the supported and advertised speeds
> for anything other than 2.5G and 10G if it can't do this rate
> adaptation thing, and its system side isn't USXGMII. It's not like
> this is dpaa_eth specific in any way.

"what about...", indeed. There are many situations one could imagine
when things would not work, we can do some brain storming on improving
this list. Meanwhile, a real issue is that in the current design, the
ethernet driver needs to remove modes that are invalid from the PHY
advertising, but needs to refrain from doing that when coupled with
a PHY that does rate adaptation. This bit provides just an indication
of that ability.

> > Madalin
> >
> >
> > > > --
> > > > Florian
> > >
> > > Regards,
> > > -Vladimir
> 
> -Vladimir

Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Russell King - ARM Linux admin]

On Wed, Jan 29, 2020 at 10:53:03AM +0000, Madalin Bucur (OSS) wrote:
> Meanwhile, a real issue is that in the current design, the
> ethernet driver needs to remove modes that are invalid from the PHY
> advertising, but needs to refrain from doing that when coupled with
> a PHY that does rate adaptation. This bit provides just an indication
> of that ability.

That should be phylink's job, not the ethernet driver.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

RE: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Sent: Wednesday, January 29, 2020 12:57 PM
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>; Florian Fainelli
> <f.fainelli@gmail.com>; davem@davemloft.net; andrew@lunn.ch;
> hkallweit1@gmail.com; netdev@vger.kernel.org; ykaukab@suse.de
> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
> indication
> 
> On Wed, Jan 29, 2020 at 10:53:03AM +0000, Madalin Bucur (OSS) wrote:
> > Meanwhile, a real issue is that in the current design, the
> > ethernet driver needs to remove modes that are invalid from the PHY
> > advertising, but needs to refrain from doing that when coupled with
> > a PHY that does rate adaptation. This bit provides just an indication
> > of that ability.
> 
> That should be phylink's job, not the ethernet driver.

I'm not using phylink. This pattern is present in most drivers that use PHYlib.

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down
> 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Vladimir Oltean]

Hi Madalin,

On Wed, 29 Jan 2020 at 12:53, Madalin Bucur (OSS)
<madalin.bucur@oss.nxp.com> wrote:
>
> > As far as I understand, for Aquantia devices this is a 3-way switch for:
> > - No rate adaptation
> > - USX rate adaptation
> > - Pause rate adaptation.
> > So what does your "phydev->rate_adaptation = 1" assignment mean?
>
> phydev->rate_adaptation = 1 means the PHY is able to perform rate adaptation
>
> There is not such thing as USX, if you refer to USXGMII, that's something you
> can check for in the interface mode.
>

I did nothing more than just read aloud the AQR412 description for
register 1E.31F.

> > From context and datasheet I deduced that you mean "the PHY is
> > configured to send PAUSE frames for 10GBase-R and 2500Base-X modes",
> > which are probably the modes in which you're using it.
> >
> > But how do you _know_ that the PHY has this switch set correctly? It
> > is either set by firmware or by MDIO, but I don't see any of that
> > being checked for.
>
> I know it is set because someone does put some work in designing a system,
> in provisioning a correct firmware image.
>

So you don't consider "firmware without this flag set, but software
(bootloader, kernel) enables it" to be a valid way of designing a
system? And your position is "well, I don't care if the person
integrating the system didn't take care of all the hardware/firmware
prerequisites, because software isn't going to attempt to do anything
helpful here even if it can"?

So the generic Linux kernel will just ask the person who put the work
in designing some system, right?

What is it that you are trying to prove?

If anything, this reminds me of the xkcd:
int getRandomNumber()
{
    return 4; // chosen by fair dice roll. guaranteed to be random.
}

> >
> > So you think that having a PHY firmware with rate adaptation disabled
> > is "stupid user"?
> > What if the rate adaptation feature is not enabled in firmware, but is
> > being enabled by U-Boot, but the user had the generic PHY driver
> > instead of the Aquantia one, or used a different or old bootloader?
> > "Stupid user"?
>
> Disabling rate adaptation is one of so many ways one could cripple a
> system interface, there are many that require polarity inversion, lane
> switching, etc. and still there is no support for that in the kernel.
>
> > The PHY and the Ethernet driver are strongly decoupled, so they need
> > to agree on an operating mode that will work for both of them.
> > Ideally the PHY would really know how it's configured, not just
> > hardcode "yeah, I can do rate adaptation, try me".
> > The fact that you can build sanity checks on top (like in this case
> > not allowing the user to disable pause frames in the Ethernet driver
> > on RX), that don't stand in the way, is just proof that the system is
> > well designed. If you can't build sanity checks, or if they stand in
> > the way, it isn't.
>
> If you don't need them, it's even better.
>

I am not really sure where you're heading with this one.

> > If anything, why haven't you considered the opposite logic here:
> > - Ethernet driver (dpaa_eth) supports all speeds 10G and below.
> > - PHY driver (or PHY core) removes the supported and advertised speeds
> > for anything other than 2.5G and 10G if it can't do this rate
> > adaptation thing, and its system side isn't USXGMII. It's not like
> > this is dpaa_eth specific in any way.
>
> "what about...", indeed. There are many situations one could imagine
> when things would not work, we can do some brain storming on improving
> this list. Meanwhile, a real issue is that in the current design, the
> ethernet driver needs to remove modes that are invalid from the PHY
> advertising, but needs to refrain from doing that when coupled with
> a PHY that does rate adaptation. This bit provides just an indication
> of that ability.

An incomplete one, at that.
Here's a list of things/potential design improvements that were
suggested to you but you only gave evasive answers on unrelated
topics:
- PHY says "I can do rate adaptation" [ via pause frames ]. Ethernet
driver knows it supports RX flow control. Good for them. But there's a
difference between "can" and "will", and somebody should bridge that
gap. The PHY should either (a) really check if rate adaptation is
enabled, before saying it is (b) say it is, for the interface modes
where that makes sense, but then actually enable it when necessary.
- The system that will be devised would work as an extensible way for
PHY to report capabilities depending on interface mode. Another
capability has been expressed (in-band autoneg) that is similar to
what you are trying to introduce, in that it requires PHY-MAC
coordination and that it is dependent on the system interface that is
being used.
- Why would the Ethernet driver be concerned about what media-side
link speed gets negotiated and used, as long as there's a PHY that can
deal with that. The approach you're taking doesn't really scale. It
scales better to have this sort of logic in the PHY driver only (if
possible), or in the PHY library (either one) too if necessary. _Yes_,
this is a larger problem and is present outside of dpaa_eth too, at
the moment.

> "what about...", indeed.

I didn't say anything about "anti-stupid devices", you did. I prefer
fail-fast systems because I don't enjoy debugging issues that can be
caught automatically. If you enjoy spending you and your users' time
like that, good for you.

>
> > > Madalin
> > >
> > >
> > > > > --
> > > > > Florian
> > > >
> > > > Regards,
> > > > -Vladimir
> >
> > -Vladimir

-Vladimir

RE: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Madalin Bucur]

Vladimir,

you had a thread opened about some of the points you are trying to make here
and as far as I read it, you were told it's a bad place for whatever you were
trying to accomplish there. You can use this thread if you feel like it, but
please try to stick to the actual proposal and reserve initiatives for further
submissions (you're planning to make?). Sorry, my proposal has not intent to
fix yours.

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Wednesday, January 29, 2020 1:58 PM
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>; davem@davemloft.net;
> andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
> ykaukab@suse.de; Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
> indication
> 
> Hi Madalin,
> 
> On Wed, 29 Jan 2020 at 12:53, Madalin Bucur (OSS)
> <madalin.bucur@oss.nxp.com> wrote:
> >
> > > As far as I understand, for Aquantia devices this is a 3-way switch
> for:
> > > - No rate adaptation
> > > - USX rate adaptation
> > > - Pause rate adaptation.
> > > So what does your "phydev->rate_adaptation = 1" assignment mean?
> >
> > phydev->rate_adaptation = 1 means the PHY is able to perform rate
> adaptation
> >
> > There is not such thing as USX, if you refer to USXGMII, that's
> something you
> > can check for in the interface mode.
> >
> 
> I did nothing more than just read aloud the AQR412 description for
> register 1E.31F.

Can you please share a link to that manual on the list?

There are hundreds of features and config knobs in the PHYs and very few
are supported in the upstream drivers yet, somehow, people use those PHYs
without issues.
 
> > > From context and datasheet I deduced that you mean "the PHY is
> > > configured to send PAUSE frames for 10GBase-R and 2500Base-X modes",
> > > which are probably the modes in which you're using it.
> > >
> > > But how do you _know_ that the PHY has this switch set correctly? It
> > > is either set by firmware or by MDIO, but I don't see any of that
> > > being checked for.
> >
> > I know it is set because someone does put some work in designing a
> system,
> > in provisioning a correct firmware image.
> >
> 
> So you don't consider "firmware without this flag set, but software
> (bootloader, kernel) enables it" to be a valid way of designing a
> system? And your position is "well, I don't care if the person
> integrating the system didn't take care of all the hardware/firmware
> prerequisites, because software isn't going to attempt to do anything
> helpful here even if it can"?
> 
> So the generic Linux kernel will just ask the person who put the work
> in designing some system, right?
> 
> What is it that you are trying to prove?

I'm just trying to fix a particular problem, if you are concerned by other
issues please submit a patch, I can try to review it.

> If anything, this reminds me of the xkcd:
> int getRandomNumber()
> {
>     return 4; // chosen by fair dice roll. guaranteed to be random.
> }

I appreciate the humor attempt, but it does not help this conversation.

https://xkcd.com/386/

> > >
> > > So you think that having a PHY firmware with rate adaptation disabled
> > > is "stupid user"?
> > > What if the rate adaptation feature is not enabled in firmware, but
> is
> > > being enabled by U-Boot, but the user had the generic PHY driver
> > > instead of the Aquantia one, or used a different or old bootloader?
> > > "Stupid user"?
> >
> > Disabling rate adaptation is one of so many ways one could cripple a
> > system interface, there are many that require polarity inversion, lane
> > switching, etc. and still there is no support for that in the kernel.
> >
> > > The PHY and the Ethernet driver are strongly decoupled, so they need
> > > to agree on an operating mode that will work for both of them.
> > > Ideally the PHY would really know how it's configured, not just
> > > hardcode "yeah, I can do rate adaptation, try me".
> > > The fact that you can build sanity checks on top (like in this case
> > > not allowing the user to disable pause frames in the Ethernet driver
> > > on RX), that don't stand in the way, is just proof that the system is
> > > well designed. If you can't build sanity checks, or if they stand in
> > > the way, it isn't.
> >
> > If you don't need them, it's even better.
> >
> 
> I am not really sure where you're heading with this one.

Lookup "kiss principle"
 
> > > If anything, why haven't you considered the opposite logic here:
> > > - Ethernet driver (dpaa_eth) supports all speeds 10G and below.
> > > - PHY driver (or PHY core) removes the supported and advertised
> speeds
> > > for anything other than 2.5G and 10G if it can't do this rate
> > > adaptation thing, and its system side isn't USXGMII. It's not like
> > > this is dpaa_eth specific in any way.
> >
> > "what about...", indeed. There are many situations one could imagine
> > when things would not work, we can do some brain storming on improving
> > this list. Meanwhile, a real issue is that in the current design, the
> > ethernet driver needs to remove modes that are invalid from the PHY
> > advertising, but needs to refrain from doing that when coupled with
> > a PHY that does rate adaptation. This bit provides just an indication
> > of that ability.
> 
> An incomplete one, at that.
> Here's a list of things/potential design improvements that were
> suggested to you but you only gave evasive answers on unrelated
> topics:
> - PHY says "I can do rate adaptation" [ via pause frames ]. Ethernet
> driver knows it supports RX flow control. Good for them. But there's a
> difference between "can" and "will", and somebody should bridge that
> gap. The PHY should either (a) really check if rate adaptation is
> enabled, before saying it is (b) say it is, for the interface modes
> where that makes sense, but then actually enable it when necessary.

Sorry, I do not have a need for all this overhead. Let's not overengineer
a solution for hypothetical issues.

> - The system that will be devised would work as an extensible way for
> PHY to report capabilities depending on interface mode. Another
> capability has been expressed (in-band autoneg) that is similar to
> what you are trying to introduce, in that it requires PHY-MAC
> coordination and that it is dependent on the system interface that is
> being used.

Which system? I've added a bit for a certain capability, to fix a particular
problem, I'm not proposing a "system" to cure all issues there. Do you have
such a proposal?

> - Why would the Ethernet driver be concerned about what media-side
> link speed gets negotiated and used, as long as there's a PHY that can
> deal with that. The approach you're taking doesn't really scale. It
> scales better to have this sort of logic in the PHY driver only (if
> possible), or in the PHY library (either one) too if necessary. _Yes_,
> this is a larger problem and is present outside of dpaa_eth too, at
> the moment.

I'm glad you understood this, there have been messages on the mailing list
about it. You can always try to send some patches to make things better.

> 
> > "what about...", indeed.
> 
> I didn't say anything about "anti-stupid devices", you did. I prefer
> fail-fast systems because I don't enjoy debugging issues that can be
> caught automatically. If you enjoy spending you and your users' time
> like that, good for you.

Please also lookup "straw man", maybe it will help keep the discussion
focused.

Madalin

Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Andrew Lunn]

> I know it is set because someone does put some work in designing a system,
> in provisioning a correct firmware image.

Hi Madalin

That is one of the things i don't like about the aquantia PHY. It can
have all sorts of magic in its firmware, but the firmware is specific
to the board. Is this phydev->rate_adaptation going to be correct for
any other board using an Aquantia PHY?

I think at least you need to poke around in the Aquantia PHY registers
and ensure this feature is actually enabled before setting this bit to
true.

	Andrew

RE: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, January 29, 2020 3:45 PM
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>; Florian Fainelli
> <f.fainelli@gmail.com>; davem@davemloft.net; hkallweit1@gmail.com;
> netdev@vger.kernel.org; ykaukab@suse.de; Russell King - ARM Linux admin
> <linux@armlinux.org.uk>
> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
> indication
> 
> > I know it is set because someone does put some work in designing a
> system,
> > in provisioning a correct firmware image.
> 
> Hi Madalin
> 
> That is one of the things i don't like about the aquantia PHY. It can
> have all sorts of magic in its firmware, but the firmware is specific
> to the board. Is this phydev->rate_adaptation going to be correct for
> any other board using an Aquantia PHY?

It's a vendor policy I cannot comment upon. Probably it gives them more
visibility / control over the use-cases of their products and also
leaves less room for configuration issues on a device that's not that
simple. As far as I can tell from the public information available, the
Aquantia PHYs we have support for in the kernel have this capability.
Whether it can be disabled through various means, I cannot provide any
guarantees. I'm not aware of any way to determine if the rate adaptation
is functional or not, Vladimir seems to have some information on that.

Nor do I need to know, my approach here is "non nocere", if there is a
chance for the PHY to link with the partner at a different speed than the
one supported on the system interface, do not prevent it by deleting those
modes, as the dpaa_eth driver does now. Whether that link will be successful
or not depends upon many variables and only some are related to rate adaptation.

While it would be perfect to have total control over those variables, to check
each and every bit that could have a value conflicting with the intended use
case, this would require open documentation from the PHY vendor, a large effort 
and would have a limited benefit in the end. Somehow, these devices do work
today, without exposing all the internal knobs to the user. If we invest the
above said large effort, we'll have them still working, but we'll have users
puzzled at a myriad of options at their fingertips. Then we'd need them to spend
some time with the board schematic in hand, to make sure they feed in the correct
values for everything. We're at a certain point on the curve between good
usability and full features, I'm not saying it's the best position, that it
cannot be improved upon, just that we need to have a balanced approach and some
priorities.

> I think at least you need to poke around in the Aquantia PHY registers
> and ensure this feature is actually enabled before setting this bit to
> true.
> 
> 	Andrew

That would change this bit from "rate adaptation capable" (my intent) to
"rate adaptation functional" (outside my scope). In anyone cares to add the
latter I'm not against it but I can settle with the former.

Regards,
Madalin

Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Andrew Lunn]

> Nor do I need to know, my approach here is "non nocere", if there is a
> chance for the PHY to link with the partner at a different speed than the
> one supported on the system interface, do not prevent it by deleting those
> modes, as the dpaa_eth driver does now. Whether that link will be successful
> or not depends upon many variables and only some are related to rate adaptation.

We need to make it clear then that this bit being true just indicates
that the device might perform rate adaptation, no guarantees, it might
depend on firmware your board does not have, phase of the moon, etc.

I don't like this. Your board not working is your problem, you know
the risks. But when somebody else starts using this bit, and it does
not work, that is not so nice.

Either:

We have documentary evidence that all Aquantia PHYs support this all
the time.

We add code to read registers to determine if the feature is enabled.

We add code to enable the feature.

You limit the scope of this to just your MAC driver. It can try to
detect if an Aquantia phy is being used, phdev->drv can be used to
detect this, and then you enable the extra link modes. Or add a device
tree property, etc.

Thanks
	Andrew

RE: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, January 29, 2020 5:34 PM
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>; Florian Fainelli
> <f.fainelli@gmail.com>; davem@davemloft.net; hkallweit1@gmail.com;
> netdev@vger.kernel.org; ykaukab@suse.de; Russell King - ARM Linux admin
> <linux@armlinux.org.uk>
> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
> indication
> 
> > Nor do I need to know, my approach here is "non nocere", if there is a
> > chance for the PHY to link with the partner at a different speed than
> the
> > one supported on the system interface, do not prevent it by deleting
> those
> > modes, as the dpaa_eth driver does now. Whether that link will be
> successful
> > or not depends upon many variables and only some are related to rate
> adaptation.
> 
> We need to make it clear then that this bit being true just indicates
> that the device might perform rate adaptation, no guarantees, it might
> depend on firmware your board does not have, phase of the moon, etc.
> 
> I don't like this. Your board not working is your problem, you know
> the risks. But when somebody else starts using this bit, and it does
> not work, that is not so nice.

Indeed, it's of little use if you do not control your environment, this
hint (it's not more than that) can only go that far.

> Either:
> 
> We have documentary evidence that all Aquantia PHYs support this all
> the time.

There are reports that it can be disabled, so no.
 
> We add code to read registers to determine if the feature is enabled.
> 
> We add code to enable the feature.

I don't have a public document for this. If someone has it and can contribute
I'll be happy to make use of the new phy_rate_adaptation_active() api.

> You limit the scope of this to just your MAC driver. It can try to
> detect if an Aquantia phy is being used, phdev->drv can be used to
> detect this, and then you enable the extra link modes. Or add a device
> tree property, etc.
> 
> Thanks
> 	Andrew

Until someone gives me that API, I will run a check on the vendor bits in
phy_driver->phy_id and get over this.

Thanks,
Madalin

[PATCH net-next 2/2] dpaa_eth: support all modes with rate adapting PHYs

[Madalin Bucur]

Stop removing modes that are not supported on the system interface
when the connected PHY is capable of rate adaptation. This addresses
an issue with the LS1046ARDB board 10G interface no longer working
with an 1G link partner after autonegotiation support was added
for the Aquantia PHY on board in

commit 09c4c57f7bc4 ("net: phy: aquantia: add support for auto-negotiation configuration")

As it only worked in other modes besides 10G because the PHY
was not configured by its driver to remove them, this is not
really a bug fix but more of a feature add.

Reported-by: Mian Yousaf Kaukab <ykaukab@suse.de>
Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index a301f0095223..d3eb235450e5 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2471,9 +2471,13 @@ static int dpaa_phy_init(struct net_device *net_dev)
 		return -ENODEV;
 	}
 
-	/* Remove any features not supported by the controller */
-	ethtool_convert_legacy_u32_to_link_mode(mask, mac_dev->if_support);
-	linkmode_and(phy_dev->supported, phy_dev->supported, mask);
+	if (mac_dev->phy_if != PHY_INTERFACE_MODE_XGMII ||
+	    !phy_dev->rate_adaptation) {
+		/* Remove any features not supported by the controller */
+		ethtool_convert_legacy_u32_to_link_mode(mask,
+							mac_dev->if_support);
+		linkmode_and(phy_dev->supported, phy_dev->supported, mask);
+	}
 
 	phy_support_asym_pause(phy_dev);
 
-- 
2.1.0

Re: [PATCH net-next 2/2] dpaa_eth: support all modes with rate adapting PHYs

[Heiner Kallweit]

On 22.01.2020 14:59, Madalin Bucur wrote:
> Stop removing modes that are not supported on the system interface
> when the connected PHY is capable of rate adaptation. This addresses
> an issue with the LS1046ARDB board 10G interface no longer working
> with an 1G link partner after autonegotiation support was added
> for the Aquantia PHY on board in
> 
> commit 09c4c57f7bc4 ("net: phy: aquantia: add support for auto-negotiation configuration")
> 
> As it only worked in other modes besides 10G because the PHY
> was not configured by its driver to remove them, this is not
> really a bug fix but more of a feature add.
> 

I understand the issue, however the description may be a little misleading.
mac_dev->if_support doesn't include 1Gbps mode, therefore this mode is
removed from phydev->supported. What happens:
- before referenced commit: aqr_config_aneg() basically is a
  no-op and doesn't touch the advertised modes in the chip.
  Therefore 1Gbps is advertised and aneg succeeds.
- after referenced commit: 1Gbps is removed from modes advertised by the
  PHY, therefore aneg doesn't succeed.

Maybe in the context of this change the interface mode should be fixed.
These Aquantia PHY's don't support XGMII, they support USXGMII.
USXGMII support was added to phylib not too long ago, therefore older
drivers use value PHY_INTERFACE_MODE_XGMII. For the same compatibility
reason the Aquantia PHY driver still accepts PHY_INTERFACE_MODE_XGMII.

Heiner

> Reported-by: Mian Yousaf Kaukab <ykaukab@suse.de>
> Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index a301f0095223..d3eb235450e5 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2471,9 +2471,13 @@ static int dpaa_phy_init(struct net_device *net_dev)
>  		return -ENODEV;
>  	}
>  
> -	/* Remove any features not supported by the controller */
> -	ethtool_convert_legacy_u32_to_link_mode(mask, mac_dev->if_support);
> -	linkmode_and(phy_dev->supported, phy_dev->supported, mask);
> +	if (mac_dev->phy_if != PHY_INTERFACE_MODE_XGMII ||
> +	    !phy_dev->rate_adaptation) {
> +		/* Remove any features not supported by the controller */
> +		ethtool_convert_legacy_u32_to_link_mode(mask,
> +							mac_dev->if_support);
> +		linkmode_and(phy_dev->supported, phy_dev->supported, mask);
> +	}
>  
>  	phy_support_asym_pause(phy_dev);
>  
> 

RE: [PATCH net-next 2/2] dpaa_eth: support all modes with rate adapting PHYs

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Sent: Wednesday, January 22, 2020 10:06 PM
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>; davem@davemloft.net
> Cc: andrew@lunn.ch; f.fainelli@gmail.com; netdev@vger.kernel.org;
> ykaukab@suse.de
> Subject: Re: [PATCH net-next 2/2] dpaa_eth: support all modes with rate
> adapting PHYs
> 
> On 22.01.2020 14:59, Madalin Bucur wrote:
> > Stop removing modes that are not supported on the system interface
> > when the connected PHY is capable of rate adaptation. This addresses
> > an issue with the LS1046ARDB board 10G interface no longer working
> > with an 1G link partner after autonegotiation support was added
> > for the Aquantia PHY on board in
> >
> > commit 09c4c57f7bc4 ("net: phy: aquantia: add support for auto-
> negotiation configuration")
> >
> > As it only worked in other modes besides 10G because the PHY
> > was not configured by its driver to remove them, this is not
> > really a bug fix but more of a feature add.
> >
> 
> I understand the issue, however the description may be a little
> misleading.
> mac_dev->if_support doesn't include 1Gbps mode, therefore this mode is
> removed from phydev->supported. What happens:
> - before referenced commit: aqr_config_aneg() basically is a
>   no-op and doesn't touch the advertised modes in the chip.
>   Therefore 1Gbps is advertised and aneg succeeds.
> - after referenced commit: 1Gbps is removed from modes advertised by the
>   PHY, therefore aneg doesn't succeed.

Hi, I can include this description in the commit log, it's a more detailed
explanation of the actual mechanism behind the above issue. 
 
> Maybe in the context of this change the interface mode should be fixed.
> These Aquantia PHY's don't support XGMII, they support USXGMII.
> USXGMII support was added to phylib not too long ago, therefore older
> drivers use value PHY_INTERFACE_MODE_XGMII. For the same compatibility
> reason the Aquantia PHY driver still accepts PHY_INTERFACE_MODE_XGMII.
> 
> Heiner

There has been a long email thread [1] related to this particular issue.
Please note USXGMII is one of the supported modes of the AQR PHYs but
unfortunately not a mode our SoC is capable to use. The DPAA 1 platforms
do not support USXGMII, they use a serial interface to the PHY, called
anywhere in the DPAA SoC and PHY datasheets XFI. We're left with "xgmii"
compatible in the device tree because that's what was available at the
time in the kernel for 10G and because in the SoC XGMII is the actual
MII that connects the MAC to the SoC internal blocks that are part of
the physical layer. Because we have an internal PCS, the DPAA SoC to
external PHY connection is a PHY-layer internal connection, that has
no official denomination derived from a standard. In the industry, this
is called XFI. We are using the XFI electrical specification to connect
to the external Aquantia PHY and I intended to describe this in the device
tree, and add it to the known PHY connection types. The issue is, the
phy-connection-type device tree parameter was aliased at a certain
moment with phy-mode, which is something else, a PHY operation mode
configuration parameter (not sure this should belong in a device tree)
and now it is considered that only PHY modes should be allowed there.

I see a way forward by splitting the two device tree elements, as they
should have been in the first place. Now we're having issues with things
like RGMII internal delay, making the code overcomplicated when one
just wants to check for RGMII. Maybe we should have a parameter for the
system interface electrical connection (phy-connection-type), another
one for the operation mode (phy-mode) and a tolerant parsing function
that adapts for backward compatibility.

The existing device trees must be supported going forward, even if we
change them for the better. In this regard, the rejection of XGMII in
the PHY driver is forcing this backward compatibility to be moved
somewhere else (in the MAC driver), which is probably a better place
for it. I'm assuming this is the reason for the change in the PHY
driver, as otherwise I do not see any real issue with it treating
XGMII as XFI in the AQR code going further.

[1] https://patchwork.ozlabs.org/patch/1213515/

Regards,
Madalin

> > Reported-by: Mian Yousaf Kaukab <ykaukab@suse.de>
> > Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index a301f0095223..d3eb235450e5 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -2471,9 +2471,13 @@ static int dpaa_phy_init(struct net_device
> *net_dev)
> >  		return -ENODEV;
> >  	}
> >
> > -	/* Remove any features not supported by the controller */
> > -	ethtool_convert_legacy_u32_to_link_mode(mask, mac_dev->if_support);
> > -	linkmode_and(phy_dev->supported, phy_dev->supported, mask);
> > +	if (mac_dev->phy_if != PHY_INTERFACE_MODE_XGMII ||
> > +	    !phy_dev->rate_adaptation) {
> > +		/* Remove any features not supported by the controller */
> > +		ethtool_convert_legacy_u32_to_link_mode(mask,
> > +							mac_dev->if_support);
> > +		linkmode_and(phy_dev->supported, phy_dev->supported, mask);
> > +	}
> >
> >  	phy_support_asym_pause(phy_dev);
> >
> >

Re: [PATCH net-next 2/2] dpaa_eth: support all modes with rate adapting PHYs

[Russell King - ARM Linux admin]

On Thu, Jan 23, 2020 at 07:20:58AM +0000, Madalin Bucur (OSS) wrote:
> There has been a long email thread [1] related to this particular issue.
> Please note USXGMII is one of the supported modes of the AQR PHYs but
> unfortunately not a mode our SoC is capable to use. The DPAA 1 platforms
> do not support USXGMII, they use a serial interface to the PHY, called
> anywhere in the DPAA SoC and PHY datasheets XFI. We're left with "xgmii"
> compatible in the device tree because that's what was available at the
> time in the kernel for 10G and because in the SoC XGMII is the actual
> MII that connects the MAC to the SoC internal blocks that are part of
> the physical layer. Because we have an internal PCS, the DPAA SoC to
> external PHY connection is a PHY-layer internal connection, that has
> no official denomination derived from a standard. In the industry, this
> is called XFI.

People in industry call 1000BASE-X "SGMII" as well, despite SGMII being
a Cisco modification of 1000BASE-X.  They are compatible up to a point
but they do not inter-operate.  What terms industry uses does not make
those terms correct, and does not mean we should continue their cockups.
The same is true for the XFI.  Get over it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up