| Message ID | 1579701573-6609-2-git-send-email-madalin.bucur@oss.nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: aquantia: indicate rate adaptation | expand |
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; >
> -----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
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
> -----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
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
> -----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
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
> -----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
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.
> -----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
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
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
> 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
> -----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
> 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
> -----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
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;
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(+)