* [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY @ 2023-06-28 12:43 Revanth Kumar Uppala 2023-06-28 12:43 ` [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE Revanth Kumar Uppala ` (4 more replies) 0 siblings, 5 replies; 21+ messages in thread From: Revanth Kumar Uppala @ 2023-06-28 12:43 UTC (permalink / raw) To: linux, andrew, hkallweit1, netdev Cc: linux-tegra, Narayan Reddy, Revanth Kumar Uppala From: Narayan Reddy <narayanr@nvidia.com> Enable flow control support using pause frames in aquantia phy driver. Signed-off-by: Narayan Reddy <narayanr@nvidia.com> Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com> --- drivers/net/phy/aquantia_main.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c index 334a6904ca5a..c99b9d066463 100644 --- a/drivers/net/phy/aquantia_main.c +++ b/drivers/net/phy/aquantia_main.c @@ -26,6 +26,8 @@ #define PHY_ID_AQR412 0x03a1b712 #define PHY_ID_AQR113C 0x31c31c12 +#define MDIO_AN_ADVT 0x0010 + #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK GENMASK(7, 3) #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR 0 @@ -583,6 +585,17 @@ static int aqr107_config_init(struct phy_device *phydev) if (!ret) aqr107_chip_info(phydev); + /* Advertize flow control */ + linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported); + linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported); + linkmode_copy(phydev->advertising, phydev->supported); + + /* Configure flow control */ + ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_ADVT, ADVERTISE_PAUSE_CAP | + ADVERTISE_PAUSE_ASYM); + if (ret < 0) + return ret; + return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE 2023-06-28 12:43 [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY Revanth Kumar Uppala @ 2023-06-28 12:43 ` Revanth Kumar Uppala 2023-06-28 13:54 ` Andrew Lunn 2023-06-28 12:43 ` [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side Revanth Kumar Uppala ` (3 subsequent siblings) 4 siblings, 1 reply; 21+ messages in thread From: Revanth Kumar Uppala @ 2023-06-28 12:43 UTC (permalink / raw) To: linux, andrew, hkallweit1, netdev Cc: linux-tegra, Revanth Kumar Uppala, Narayan Reddy Enable MAC controlled energy efficient ethernet (EEE) so that MAC can keep the PHY in EEE sleep mode when link utilization is low to reduce energy consumption. Signed-off-by: Narayan Reddy <narayanr@nvidia.com> Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com> --- drivers/net/phy/aquantia_main.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c index c99b9d066463..faca2a0b1d49 100644 --- a/drivers/net/phy/aquantia_main.c +++ b/drivers/net/phy/aquantia_main.c @@ -28,6 +28,9 @@ #define MDIO_AN_ADVT 0x0010 +#define VEND1_SEC_INGRESS_CNTRL_REG1 0x7001 +#define MAC_CNTRL_EEE (BIT(8) | BIT(12)) + #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK GENMASK(7, 3) #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR 0 @@ -596,6 +599,12 @@ static int aqr107_config_init(struct phy_device *phydev) if (ret < 0) return ret; + /* Enable MAC Controlled EEE */ + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, + VEND1_SEC_INGRESS_CNTRL_REG1, MAC_CNTRL_EEE); + if (ret < 0) + return ret; + return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE 2023-06-28 12:43 ` [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE Revanth Kumar Uppala @ 2023-06-28 13:54 ` Andrew Lunn 2023-07-24 11:29 ` Revanth Kumar Uppala 0 siblings, 1 reply; 21+ messages in thread From: Andrew Lunn @ 2023-06-28 13:54 UTC (permalink / raw) To: Revanth Kumar Uppala Cc: linux, hkallweit1, netdev, linux-tegra, Narayan Reddy On Wed, Jun 28, 2023 at 06:13:24PM +0530, Revanth Kumar Uppala wrote: > Enable MAC controlled energy efficient ethernet (EEE) so that MAC can > keep the PHY in EEE sleep mode when link utilization is low to reduce > energy consumption. This needs more explanation. Is this 'SmartEEE', in that the PHY is doing EEE without the SoC MAC being involved? Ideally, you should only do SmartEEE, if the SoC MAC is dumb and does not have EEE itself. I guess if you are doing rate adaptation, or MACSEC in the PHY, then you might be forced to use SmartEEE since the SoC MAC is somewhat decoupled from the PHY. At the moment, we don't have a good story for SmartEEE. It should be configured in the same way as normal EEE, ethtool --set-eee etc. I've got a rewrite of normal EEE in the works. Once that is merged i hope SmartEEE will be next. Andrew ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE 2023-06-28 13:54 ` Andrew Lunn @ 2023-07-24 11:29 ` Revanth Kumar Uppala 2023-07-24 11:52 ` Russell King (Oracle) 0 siblings, 1 reply; 21+ messages in thread From: Revanth Kumar Uppala @ 2023-07-24 11:29 UTC (permalink / raw) To: Andrew Lunn; +Cc: linux, hkallweit1, netdev, linux-tegra, Narayan Reddy > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Wednesday, June 28, 2023 7:24 PM > To: Revanth Kumar Uppala <ruppala@nvidia.com> > Cc: linux@armlinux.org.uk; hkallweit1@gmail.com; netdev@vger.kernel.org; > linux-tegra@vger.kernel.org; Narayan Reddy <narayanr@nvidia.com> > Subject: Re: [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE > > External email: Use caution opening links or attachments > > > On Wed, Jun 28, 2023 at 06:13:24PM +0530, Revanth Kumar Uppala wrote: > > Enable MAC controlled energy efficient ethernet (EEE) so that MAC can > > keep the PHY in EEE sleep mode when link utilization is low to reduce > > energy consumption. > > This needs more explanation. Is this 'SmartEEE', in that the PHY is doing EEE > without the SoC MAC being involved? No, this is not Smart EEE. > > Ideally, you should only do SmartEEE, if the SoC MAC is dumb and does not have > EEE itself. I guess if you are doing rate adaptation, or MACSEC in the PHY, then > you might be forced to use SmartEEE since the SoC MAC is somewhat decoupled > from the PHY. > > At the moment, we don't have a good story for SmartEEE. It should be > configured in the same way as normal EEE, ethtool --set-eee etc. I've got a > rewrite of normal EEE in the works. Once that is merged i hope SmartEEE will be > next. "ethtool --set-eee" is a dynamic way of enabling normal EEE and here we are doing the same normal EEE but configuring it by default in aqr107_config_init() instead of doing it dynamically. So, is there any concern for this? Thanks, Revanth Uppala > > Andrew ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE 2023-07-24 11:29 ` Revanth Kumar Uppala @ 2023-07-24 11:52 ` Russell King (Oracle) 0 siblings, 0 replies; 21+ messages in thread From: Russell King (Oracle) @ 2023-07-24 11:52 UTC (permalink / raw) To: Revanth Kumar Uppala Cc: Andrew Lunn, hkallweit1, netdev, linux-tegra, Narayan Reddy On Mon, Jul 24, 2023 at 11:29:28AM +0000, Revanth Kumar Uppala wrote: > > Ideally, you should only do SmartEEE, if the SoC MAC is dumb and does not have > > EEE itself. I guess if you are doing rate adaptation, or MACSEC in the PHY, then > > you might be forced to use SmartEEE since the SoC MAC is somewhat decoupled > > from the PHY. > > > > At the moment, we don't have a good story for SmartEEE. It should be > > configured in the same way as normal EEE, ethtool --set-eee etc. I've got a > > rewrite of normal EEE in the works. Once that is merged i hope SmartEEE will be > > next. > "ethtool --set-eee" is a dynamic way of enabling normal EEE and here we are doing the same normal EEE but configuring it by default in aqr107_config_init() instead of doing it dynamically. So, setting the MAC_CNTRL_EEE bits is just enabling the standard IEEE paths in the PHY to allow the IEEE defined EEE architecture to work? If that's all its doing, I wonder why they aren't set by default... seems rather strange. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side 2023-06-28 12:43 [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY Revanth Kumar Uppala 2023-06-28 12:43 ` [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE Revanth Kumar Uppala @ 2023-06-28 12:43 ` Revanth Kumar Uppala 2023-06-28 13:33 ` Russell King (Oracle) 2023-06-28 12:43 ` [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) Revanth Kumar Uppala ` (2 subsequent siblings) 4 siblings, 1 reply; 21+ messages in thread From: Revanth Kumar Uppala @ 2023-06-28 12:43 UTC (permalink / raw) To: linux, andrew, hkallweit1, netdev; +Cc: linux-tegra, Revanth Kumar Uppala Lane bring-up failures are seen during interface up, as interface speed settings are configured while the PHY is still initializing. So, poll until PHY system side interface gets ready and the interface speed settings are configured. Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com> --- drivers/net/phy/aquantia_main.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c index faca2a0b1d49..a27ff4733050 100644 --- a/drivers/net/phy/aquantia_main.c +++ b/drivers/net/phy/aquantia_main.c @@ -41,6 +41,7 @@ #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_SGMII 6 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_RXAUI 7 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_OCSGMII 10 +#define MDIO_PHYXS_VEND_IF_STATUS_TX_READY BIT(12) #define MDIO_AN_VEND_PROV 0xc400 #define MDIO_AN_VEND_PROV_1000BASET_FULL BIT(15) @@ -467,6 +468,19 @@ static int aqr107_read_status(struct phy_device *phydev) break; } + /* Lane bring-up failures are seen during interface up, as interface + * speed settings are configured while the PHY is still initializing. + * To resolve this, poll until PHY system side interface gets ready + * and the interface speed settings are configured. + */ + ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS, MDIO_PHYXS_VEND_IF_STATUS, + val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY), + 20000, 2000000, false); + if (ret) { + phydev_err(phydev, "PHY system interface is not yet ready\n"); + return ret; + } + /* Read possibly downshifted rate from vendor register */ return aqr107_read_rate(phydev); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side 2023-06-28 12:43 ` [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side Revanth Kumar Uppala @ 2023-06-28 13:33 ` Russell King (Oracle) 2023-07-24 11:29 ` Revanth Kumar Uppala 0 siblings, 1 reply; 21+ messages in thread From: Russell King (Oracle) @ 2023-06-28 13:33 UTC (permalink / raw) To: Revanth Kumar Uppala; +Cc: andrew, hkallweit1, netdev, linux-tegra On Wed, Jun 28, 2023 at 06:13:25PM +0530, Revanth Kumar Uppala wrote: > + /* Lane bring-up failures are seen during interface up, as interface > + * speed settings are configured while the PHY is still initializing. > + * To resolve this, poll until PHY system side interface gets ready > + * and the interface speed settings are configured. > + */ > + ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS, MDIO_PHYXS_VEND_IF_STATUS, > + val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY), > + 20000, 2000000, false); What does this actually mean when the condition succeeds? Does it mean that the system interface is now fully configured (but may or may not have link)? If that's correct, then that's fine. If it doesn't succeed because the system interface doesn't have link, then that would be very bad, because _this_ function needs to return so the MAC side can then be configured to gain link with the PHY with the appropriate link parameters. The comment doesn't make it clear which it is. Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side 2023-06-28 13:33 ` Russell King (Oracle) @ 2023-07-24 11:29 ` Revanth Kumar Uppala 2023-07-24 11:57 ` Russell King (Oracle) 0 siblings, 1 reply; 21+ messages in thread From: Revanth Kumar Uppala @ 2023-07-24 11:29 UTC (permalink / raw) To: Russell King; +Cc: andrew, hkallweit1, netdev, linux-tegra > -----Original Message----- > From: Russell King <linux@armlinux.org.uk> > Sent: Wednesday, June 28, 2023 7:04 PM > To: Revanth Kumar Uppala <ruppala@nvidia.com> > Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux- > tegra@vger.kernel.org > Subject: Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side > > External email: Use caution opening links or attachments > > > On Wed, Jun 28, 2023 at 06:13:25PM +0530, Revanth Kumar Uppala wrote: > > + /* Lane bring-up failures are seen during interface up, as interface > > + * speed settings are configured while the PHY is still initializing. > > + * To resolve this, poll until PHY system side interface gets ready > > + * and the interface speed settings are configured. > > + */ > > + ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS, > MDIO_PHYXS_VEND_IF_STATUS, > > + val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY), > > + 20000, 2000000, false); > > What does this actually mean when the condition succeeds? Does it mean that > the system interface is now fully configured (but may or may not have link)? Yes, your understanding is correct. It means that the system interface is now fully configured and has the link. > > If that's correct, then that's fine. If it doesn't succeed because the system > interface doesn't have link, then that would be very bad, because _this_ function > needs to return so the MAC side can then be configured to gain link with the PHY > with the appropriate link parameters. > > The comment doesn't make it clear which it is. I will add the comment more clearly in V2 series. > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side 2023-07-24 11:29 ` Revanth Kumar Uppala @ 2023-07-24 11:57 ` Russell King (Oracle) 0 siblings, 0 replies; 21+ messages in thread From: Russell King (Oracle) @ 2023-07-24 11:57 UTC (permalink / raw) To: Revanth Kumar Uppala; +Cc: andrew, hkallweit1, netdev, linux-tegra On Mon, Jul 24, 2023 at 11:29:33AM +0000, Revanth Kumar Uppala wrote: > > -----Original Message----- > > From: Russell King <linux@armlinux.org.uk> > > Sent: Wednesday, June 28, 2023 7:04 PM > > To: Revanth Kumar Uppala <ruppala@nvidia.com> > > Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux- > > tegra@vger.kernel.org > > Subject: Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side > > > > External email: Use caution opening links or attachments > > > > > > On Wed, Jun 28, 2023 at 06:13:25PM +0530, Revanth Kumar Uppala wrote: > > > + /* Lane bring-up failures are seen during interface up, as interface > > > + * speed settings are configured while the PHY is still initializing. > > > + * To resolve this, poll until PHY system side interface gets ready > > > + * and the interface speed settings are configured. > > > + */ > > > + ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS, > > MDIO_PHYXS_VEND_IF_STATUS, > > > + val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY), > > > + 20000, 2000000, false); > > > > What does this actually mean when the condition succeeds? Does it mean that > > the system interface is now fully configured (but may or may not have link)? > Yes, your understanding is correct. > It means that the system interface is now fully configured and has the link. As you indicate that it also indicates that the system interface has link, then you leave me no option but to NAK this patch, sorry. The reason is: > > ... If it doesn't succeed because the system > > interface doesn't have link, then that would be very bad, because _this_ function > > needs to return so the MAC side can then be configured to gain link with the PHY > > with the appropriate link parameters. Essentially, if the PHY changes its host interface because the media side has changed, we *need* the read_status() function to succeed, tell us that the link is up, and what the parameters are for the media side link _and_ the host side interface. At this point, if the PHY has changed its host-side interface, then the link with the host MAC will be _down_ because the MAC driver is not yet aware of the new parameters for the link. read_status() has to succeed and report the new parameters to the MAC so that the MAC (or phylink) can reconfigure the MAC and PCS for the PHY's new operating mode. Sorry, but NAK. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) 2023-06-28 12:43 [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY Revanth Kumar Uppala 2023-06-28 12:43 ` [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE Revanth Kumar Uppala 2023-06-28 12:43 ` [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side Revanth Kumar Uppala @ 2023-06-28 12:43 ` Revanth Kumar Uppala 2023-06-28 13:43 ` Russell King (Oracle) ` (2 more replies) 2023-06-28 13:30 ` [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY Russell King (Oracle) 2023-06-28 13:46 ` Russell King (Oracle) 4 siblings, 3 replies; 21+ messages in thread From: Revanth Kumar Uppala @ 2023-06-28 12:43 UTC (permalink / raw) To: linux, andrew, hkallweit1, netdev Cc: linux-tegra, Revanth Kumar Uppala, Narayan Reddy Configure the WOL settings in the PHY to allow the PHY to detect magic packets and generate an interrupt to wake the device from suspend. The PHY configuration is restored back to XFI once the PHY received the WOL magic packet. Note that it is not necessary to poll the PHY status when WOL is enabled because the interface speed is not being re-configured Signed-off-by: Narayan Reddy <narayanr@nvidia.com> Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com> --- drivers/net/phy/aquantia_main.c | 235 +++++++++++++++++++++++++++++++- include/uapi/linux/mdio.h | 1 + 2 files changed, 229 insertions(+), 7 deletions(-) diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c index a27ff4733050..5c69ecd2cf9f 100644 --- a/drivers/net/phy/aquantia_main.c +++ b/drivers/net/phy/aquantia_main.c @@ -12,6 +12,7 @@ #include <linux/delay.h> #include <linux/bitfield.h> #include <linux/phy.h> +#include <linux/netdevice.h> #include "aquantia.h" @@ -48,10 +49,13 @@ #define MDIO_AN_VEND_PROV_1000BASET_HALF BIT(14) #define MDIO_AN_VEND_PROV_5000BASET_FULL BIT(11) #define MDIO_AN_VEND_PROV_2500BASET_FULL BIT(10) +#define MDIO_AN_VEND_PROV_AQRATE_DWN_SHFT_CAP BIT(12) #define MDIO_AN_VEND_PROV_DOWNSHIFT_EN BIT(4) #define MDIO_AN_VEND_PROV_DOWNSHIFT_MASK GENMASK(3, 0) #define MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT 4 +#define MDIO_AN_VEND_MASK 0xF0FF +#define MDIO_AN_RSVD_VEND_PROV1 0xc410 #define MDIO_AN_TX_VEND_STATUS1 0xc800 #define MDIO_AN_TX_VEND_STATUS1_RATE_MASK GENMASK(3, 1) #define MDIO_AN_TX_VEND_STATUS1_10BASET 0 @@ -61,6 +65,9 @@ #define MDIO_AN_TX_VEND_STATUS1_2500BASET 4 #define MDIO_AN_TX_VEND_STATUS1_5000BASET 5 #define MDIO_AN_TX_VEND_STATUS1_FULL_DUPLEX BIT(0) +#define MDIO_MMD_AN_WOL_ENABLE BIT(6) + +#define MDIO_AN_RSVD_VEND_STATUS3 0xc812 #define MDIO_AN_TX_VEND_INT_STATUS1 0xcc00 #define MDIO_AN_TX_VEND_INT_STATUS1_DOWNSHIFT BIT(1) @@ -86,6 +93,19 @@ #define MDIO_AN_RX_VEND_STAT3_AFR BIT(0) /* MDIO_MMD_C22EXT */ +#define MDIO_C22EXT_MAGIC_PKT_PATTERN_0_2_15 0xc339 +#define MDIO_C22EXT_MAGIC_PKT_PATTERN_16_2_31 0xc33a +#define MDIO_C22EXT_MAGIC_PKT_PATTERN_32_2_47 0xc33b + +#define MDIO_C22EXT_GBE_PHY_RSI1_CTRL6 0xc355 +#define MDIO_C22EXT_RSI_WAKE_UP_FRAME_DETECTION BIT(0) + +#define MDIO_C22EXT_GBE_PHY_RSI1_CTRL7 0xc356 +#define MDIO_C22EXT_RSI_MAGIC_PKT_FRAME_DETECTION BIT(0) + +#define MDIO_C22EXT_GBE_PHY_RSI1_CTRL8 0xc357 +#define MDIO_C22EXT_RSI_WOL_FCS_MONITOR_MODE BIT(15) + #define MDIO_C22EXT_STAT_SGMII_RX_GOOD_FRAMES 0xd292 #define MDIO_C22EXT_STAT_SGMII_RX_BAD_FRAMES 0xd294 #define MDIO_C22EXT_STAT_SGMII_RX_FALSE_CARRIER 0xd297 @@ -96,6 +116,11 @@ #define MDIO_C22EXT_STAT_SGMII_TX_LINE_COLLISIONS 0xd319 #define MDIO_C22EXT_STAT_SGMII_TX_FRAME_ALIGN_ERR 0xd31a #define MDIO_C22EXT_STAT_SGMII_TX_RUNT_FRAMES 0xd31b +#define MDIO_C22EXT_GBE_PHY_SGMII_TX_ALARM1 0xec20 + +#define MDIO_C22EXT_GBE_PHY_SGMII_TX_INT_MASK1 0xf420 +#define MDIO_C22EXT_SGMII0_WAKE_UP_FRAME_MASK BIT(4) +#define MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK BIT(5) /* Vendor specific 1, MDIO_MMD_VEND1 */ #define VEND1_GLOBAL_FW_ID 0x0020 @@ -109,6 +134,10 @@ #define VEND1_GLOBAL_CFG_10M 0x0310 #define VEND1_GLOBAL_CFG_100M 0x031b #define VEND1_GLOBAL_CFG_1G 0x031c +#define VEND1_GLOBAL_SYS_CONFIG_SGMII (BIT(0) | BIT(1)) +#define VEND1_GLOBAL_SYS_CONFIG_AN BIT(3) +#define VEND1_GLOBAL_SYS_CONFIG_XFI BIT(8) + #define VEND1_GLOBAL_CFG_2_5G 0x031d #define VEND1_GLOBAL_CFG_5G 0x031e #define VEND1_GLOBAL_CFG_10G 0x031f @@ -181,6 +210,7 @@ static const struct aqr107_hw_stat aqr107_hw_stats[] = { struct aqr107_priv { u64 sgmii_stats[AQR107_SGMII_STAT_SZ]; + int wol_status; }; static int aqr107_get_sset_count(struct phy_device *phydev) @@ -327,9 +357,139 @@ static int aqr_config_intr(struct phy_device *phydev) return 0; } +static int aqr113c_wol_enable(struct phy_device *phydev) +{ + struct aqr107_priv *priv = phydev->priv; + u16 val; + int ret; + + /* Disables all advertised speeds except for the WoL + * speed (100BASE-TX FD or 1000BASE-T) + * This is set as per the APP note from Marvel + */ + ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL, + MDIO_AN_LD_LOOP_TIMING_ABILITY); + if (ret < 0) + return ret; + + ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV); + if (ret < 0) + return ret; + + val = (ret & MDIO_AN_VEND_MASK) | + (MDIO_AN_VEND_PROV_AQRATE_DWN_SHFT_CAP | MDIO_AN_VEND_PROV_1000BASET_FULL); + ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV, val); + if (ret < 0) + return ret; + + /* Enable the magic frame and wake up frame detection for the PHY */ + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL6, + MDIO_C22EXT_RSI_WAKE_UP_FRAME_DETECTION); + if (ret < 0) + return ret; + + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL7, + MDIO_C22EXT_RSI_MAGIC_PKT_FRAME_DETECTION); + if (ret < 0) + return ret; + + /* Set the WoL enable bit */ + ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_PROV1, + MDIO_MMD_AN_WOL_ENABLE); + if (ret < 0) + return ret; + + /* Set the WoL INT_N trigger bit */ + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL8, + MDIO_C22EXT_RSI_WOL_FCS_MONITOR_MODE); + if (ret < 0) + return ret; + + /* Enable Interrupt INT_N Generation at pin level */ + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_SGMII_TX_INT_MASK1, + MDIO_C22EXT_SGMII0_WAKE_UP_FRAME_MASK | + MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK); + if (ret < 0) + return ret; + + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_INT_STD_MASK, + VEND1_GLOBAL_INT_STD_MASK_ALL); + if (ret < 0) + return ret; + + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_INT_VEND_MASK, + VEND1_GLOBAL_INT_VEND_MASK_GBE); + if (ret < 0) + return ret; + + /* Set the system interface to SGMII */ + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, + VEND1_GLOBAL_CFG_100M, VEND1_GLOBAL_SYS_CONFIG_SGMII | + VEND1_GLOBAL_SYS_CONFIG_AN); + if (ret < 0) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, + VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_SGMII | + VEND1_GLOBAL_SYS_CONFIG_AN); + if (ret < 0) + return ret; + + /* restart auto-negotiation */ + genphy_c45_restart_aneg(phydev); + priv->wol_status = 1; + + return 0; +} + +static int aqr113c_wol_disable(struct phy_device *phydev) +{ + struct aqr107_priv *priv = phydev->priv; + int ret; + + /* Disable the WoL enable bit */ + ret = phy_clear_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_PROV1, + MDIO_MMD_AN_WOL_ENABLE); + if (ret < 0) + return ret; + + /* Restore the SERDES/System Interface back to the XFI mode */ + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, + VEND1_GLOBAL_CFG_100M, VEND1_GLOBAL_SYS_CONFIG_XFI); + if (ret < 0) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, + VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_XFI); + if (ret < 0) + return ret; + + /* restart auto-negotiation */ + genphy_c45_restart_aneg(phydev); + priv->wol_status = 0; + + return 0; +} + static irqreturn_t aqr_handle_interrupt(struct phy_device *phydev) { + struct aqr107_priv *priv = phydev->priv; int irq_status; + int ret; + + ret = phy_read_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_SGMII_TX_ALARM1); + if (ret < 0) { + phy_error(phydev); + return IRQ_NONE; + } + + if ((ret & MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK) == + MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK) { + /* Disable the WoL */ + ret = aqr113c_wol_disable(phydev); + if (ret < 0) + return IRQ_NONE; + } irq_status = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_INT_STATUS2); @@ -425,6 +585,7 @@ static int aqr107_read_rate(struct phy_device *phydev) static int aqr107_read_status(struct phy_device *phydev) { + struct aqr107_priv *priv = phydev->priv; int val, ret; ret = aqr_read_status(phydev); @@ -471,14 +632,18 @@ static int aqr107_read_status(struct phy_device *phydev) /* Lane bring-up failures are seen during interface up, as interface * speed settings are configured while the PHY is still initializing. * To resolve this, poll until PHY system side interface gets ready - * and the interface speed settings are configured. + * and the interface speed settings are configured.Polling is skipped + * when WoL is enabled because interface speed settings are not + * configured at that time. */ - ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS, MDIO_PHYXS_VEND_IF_STATUS, - val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY), - 20000, 2000000, false); - if (ret) { - phydev_err(phydev, "PHY system interface is not yet ready\n"); - return ret; + if (!priv->wol_status) { + ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS, MDIO_PHYXS_VEND_IF_STATUS, + val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY), + 20000, 2000000, false); + if (ret) { + phydev_err(phydev, "PHY system interface is not yet ready\n"); + return ret; + } } /* Read possibly downshifted rate from vendor register */ @@ -619,6 +784,31 @@ static int aqr107_config_init(struct phy_device *phydev) if (ret < 0) return ret; + /* Configure Magic packet frame pattern (MAC address) */ + ret = phy_write_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_MAGIC_PKT_PATTERN_0_2_15, + phydev->attached_dev->dev_addr[0] | + (phydev->attached_dev->dev_addr[1] << 8)); + if (ret < 0) { + phydev_err(phydev, "Error setting magic packet frame of 0/1st byte\n"); + return ret; + } + + ret = phy_write_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_MAGIC_PKT_PATTERN_16_2_31, + phydev->attached_dev->dev_addr[2] | + (phydev->attached_dev->dev_addr[3] << 8)); + if (ret < 0) { + phydev_err(phydev, "Error setting magic packet frame of 2/3rd byte\n"); + return ret; + } + + ret = phy_write_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_MAGIC_PKT_PATTERN_32_2_47, + phydev->attached_dev->dev_addr[4] | + (phydev->attached_dev->dev_addr[5] << 8)); + if (ret < 0) { + phydev_err(phydev, "Error setting magic packet frame of 4/5th byte\n"); + return ret; + } + return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT); } @@ -757,6 +947,35 @@ static int aqr107_probe(struct phy_device *phydev) return aqr_hwmon_probe(phydev); } +static void aqr113c_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) +{ + int val; + + val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_STATUS3); + if (val < 0) + return; + + wol->supported = WAKE_MAGIC; + if (val & 0x1) + wol->wolopts = WAKE_MAGIC; +} + +static int aqr113c_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) +{ + struct aqr107_priv *priv = phydev->priv; + + /* Return success if WOL is already set. Don't entertain duplicate setting of WOL */ + if (!(priv->wol_status ^ wol->wolopts)) + return 0; + + if (wol->wolopts & WAKE_MAGIC) + return aqr113c_wol_enable(phydev); + else + return aqr113c_wol_disable(phydev); + + return 0; +} + static struct phy_driver aqr_driver[] = { { PHY_ID_MATCH_MODEL(PHY_ID_AQ1202), @@ -892,6 +1111,8 @@ static struct phy_driver aqr_driver[] = { .get_strings = aqr107_get_strings, .get_stats = aqr107_get_stats, .link_change_notify = aqr107_link_change_notify, + .get_wol = &aqr113c_get_wol, + .set_wol = &aqr113c_set_wol, }, }; diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h index b826598d1e94..07ca44891378 100644 --- a/include/uapi/linux/mdio.h +++ b/include/uapi/linux/mdio.h @@ -298,6 +298,7 @@ #define MDIO_AN_10GBT_CTRL_ADV5G 0x0100 /* Advertise 5GBASE-T */ #define MDIO_AN_10GBT_CTRL_ADV10G 0x1000 /* Advertise 10GBASE-T */ +#define MDIO_AN_LD_LOOP_TIMING_ABILITY 0x0001 /* AN 10GBASE-T status register. */ #define MDIO_AN_10GBT_STAT_LP2_5G 0x0020 /* LP is 2.5GBT capable */ #define MDIO_AN_10GBT_STAT_LP5G 0x0040 /* LP is 5GBT capable */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) 2023-06-28 12:43 ` [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) Revanth Kumar Uppala @ 2023-06-28 13:43 ` Russell King (Oracle) 2023-07-24 11:29 ` Revanth Kumar Uppala 2023-06-28 14:17 ` Andrew Lunn 2023-06-28 18:57 ` kernel test robot 2 siblings, 1 reply; 21+ messages in thread From: Russell King (Oracle) @ 2023-06-28 13:43 UTC (permalink / raw) To: Revanth Kumar Uppala Cc: andrew, hkallweit1, netdev, linux-tegra, Narayan Reddy On Wed, Jun 28, 2023 at 06:13:26PM +0530, Revanth Kumar Uppala wrote: > @@ -109,6 +134,10 @@ > #define VEND1_GLOBAL_CFG_10M 0x0310 > #define VEND1_GLOBAL_CFG_100M 0x031b > #define VEND1_GLOBAL_CFG_1G 0x031c > +#define VEND1_GLOBAL_SYS_CONFIG_SGMII (BIT(0) | BIT(1)) > +#define VEND1_GLOBAL_SYS_CONFIG_AN BIT(3) > +#define VEND1_GLOBAL_SYS_CONFIG_XFI BIT(8) My understanding is that bits 2:0 are a _bitfield_ and not individual bits, which contain the following values: 0 - 10GBASE-R (XFI if you really want to call it that) 3 - SGMII 4 - OCSGMII (2.5G) 6 - 5GBASE-R (XFI5G if you really want to call it that) Bit 3 controls whether the SGMII control word is used, and this is the only applicable mode. Bit 8 is already defined - it's part of the rate adaption mode field, see VEND1_GLOBAL_CFG_RATE_ADAPT and VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE. These bits apply to all the VEND1_GLOBAL_CFG_* registers, so these should be defined after the last register (0x031f). > +static int aqr113c_wol_enable(struct phy_device *phydev) > +{ > + struct aqr107_priv *priv = phydev->priv; > + u16 val; > + int ret; > + > + /* Disables all advertised speeds except for the WoL > + * speed (100BASE-TX FD or 1000BASE-T) > + * This is set as per the APP note from Marvel > + */ > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL, > + MDIO_AN_LD_LOOP_TIMING_ABILITY); > + if (ret < 0) > + return ret; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV); > + if (ret < 0) > + return ret; > + > + val = (ret & MDIO_AN_VEND_MASK) | > + (MDIO_AN_VEND_PROV_AQRATE_DWN_SHFT_CAP | MDIO_AN_VEND_PROV_1000BASET_FULL); > + ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV, val); > + if (ret < 0) > + return ret; > + > + /* Enable the magic frame and wake up frame detection for the PHY */ > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL6, > + MDIO_C22EXT_RSI_WAKE_UP_FRAME_DETECTION); > + if (ret < 0) > + return ret; > + > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL7, > + MDIO_C22EXT_RSI_MAGIC_PKT_FRAME_DETECTION); > + if (ret < 0) > + return ret; > + > + /* Set the WoL enable bit */ > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_PROV1, > + MDIO_MMD_AN_WOL_ENABLE); > + if (ret < 0) > + return ret; > + > + /* Set the WoL INT_N trigger bit */ > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL8, > + MDIO_C22EXT_RSI_WOL_FCS_MONITOR_MODE); > + if (ret < 0) > + return ret; > + > + /* Enable Interrupt INT_N Generation at pin level */ > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_SGMII_TX_INT_MASK1, > + MDIO_C22EXT_SGMII0_WAKE_UP_FRAME_MASK | > + MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK); > + if (ret < 0) > + return ret; > + > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_INT_STD_MASK, > + VEND1_GLOBAL_INT_STD_MASK_ALL); > + if (ret < 0) > + return ret; > + > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_INT_VEND_MASK, > + VEND1_GLOBAL_INT_VEND_MASK_GBE); > + if (ret < 0) > + return ret; > + > + /* Set the system interface to SGMII */ > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, > + VEND1_GLOBAL_CFG_100M, VEND1_GLOBAL_SYS_CONFIG_SGMII | > + VEND1_GLOBAL_SYS_CONFIG_AN); How do you know that SGMII should be used for 100M? > + if (ret < 0) > + return ret; > + > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, > + VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_SGMII | > + VEND1_GLOBAL_SYS_CONFIG_AN); How do you know that SGMII should be used for 1G? Doesn't this depend on the configuration of the host MAC and the capabilities of it? If the host MAC only supports 10G, doesn't this break stuff? > + if (ret < 0) > + return ret; > + > + /* restart auto-negotiation */ > + genphy_c45_restart_aneg(phydev); > + priv->wol_status = 1; > + > + return 0; > +} > + > +static int aqr113c_wol_disable(struct phy_device *phydev) > +{ > + struct aqr107_priv *priv = phydev->priv; > + int ret; > + > + /* Disable the WoL enable bit */ > + ret = phy_clear_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_PROV1, > + MDIO_MMD_AN_WOL_ENABLE); > + if (ret < 0) > + return ret; > + > + /* Restore the SERDES/System Interface back to the XFI mode */ > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, > + VEND1_GLOBAL_CFG_100M, VEND1_GLOBAL_SYS_CONFIG_XFI); > + if (ret < 0) > + return ret; > + > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, > + VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_XFI); > + if (ret < 0) > + return ret; Conversely, how do you know that configuring 100M/1G to use 10GBASE-R on the host interface is how the PHY was provisioned in firmware? I think at the very least, you should be leaving these settings alone until you know that the system is entering a low power mode, saving the settings, and restoring them when you wake up. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) 2023-06-28 13:43 ` Russell King (Oracle) @ 2023-07-24 11:29 ` Revanth Kumar Uppala 2023-07-24 12:29 ` Russell King (Oracle) 0 siblings, 1 reply; 21+ messages in thread From: Revanth Kumar Uppala @ 2023-07-24 11:29 UTC (permalink / raw) To: Russell King; +Cc: andrew, hkallweit1, netdev, linux-tegra, Narayan Reddy > -----Original Message----- > From: Russell King <linux@armlinux.org.uk> > Sent: Wednesday, June 28, 2023 7:13 PM > To: Revanth Kumar Uppala <ruppala@nvidia.com> > Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux- > tegra@vger.kernel.org; Narayan Reddy <narayanr@nvidia.com> > Subject: Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) > > External email: Use caution opening links or attachments > > > On Wed, Jun 28, 2023 at 06:13:26PM +0530, Revanth Kumar Uppala wrote: > > @@ -109,6 +134,10 @@ > > #define VEND1_GLOBAL_CFG_10M 0x0310 > > #define VEND1_GLOBAL_CFG_100M 0x031b > > #define VEND1_GLOBAL_CFG_1G 0x031c > > +#define VEND1_GLOBAL_SYS_CONFIG_SGMII (BIT(0) | BIT(1)) > > +#define VEND1_GLOBAL_SYS_CONFIG_AN BIT(3) > > +#define VEND1_GLOBAL_SYS_CONFIG_XFI BIT(8) > > My understanding is that bits 2:0 are a _bitfield_ and not individual bits, which > contain the following values: I will define bitfield instead of defining individual bits in V2 series > > 0 - 10GBASE-R (XFI if you really want to call it that) > 3 - SGMII > 4 - OCSGMII (2.5G) > 6 - 5GBASE-R (XFI5G if you really want to call it that) > > Bit 3 controls whether the SGMII control word is used, and this is the only > applicable mode. > > Bit 8 is already defined - it's part of the rate adaption mode field, see > VEND1_GLOBAL_CFG_RATE_ADAPT and > VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE. Sure, I will use above mentioned macros and will set the register values with help of FIELD_PREP in V2 series > > These bits apply to all the VEND1_GLOBAL_CFG_* registers, so these should be > defined after the last register (0x031f). Will take care of this. > > > +static int aqr113c_wol_enable(struct phy_device *phydev) { > > + struct aqr107_priv *priv = phydev->priv; > > + u16 val; > > + int ret; > > + > > + /* Disables all advertised speeds except for the WoL > > + * speed (100BASE-TX FD or 1000BASE-T) > > + * This is set as per the APP note from Marvel > > + */ > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, > MDIO_AN_10GBT_CTRL, > > + MDIO_AN_LD_LOOP_TIMING_ABILITY); > > + if (ret < 0) > > + return ret; > > + > > + ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV); > > + if (ret < 0) > > + return ret; > > + > > + val = (ret & MDIO_AN_VEND_MASK) | > > + (MDIO_AN_VEND_PROV_AQRATE_DWN_SHFT_CAP | > MDIO_AN_VEND_PROV_1000BASET_FULL); > > + ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV, > val); > > + if (ret < 0) > > + return ret; > > + > > + /* Enable the magic frame and wake up frame detection for the PHY */ > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, > MDIO_C22EXT_GBE_PHY_RSI1_CTRL6, > > + MDIO_C22EXT_RSI_WAKE_UP_FRAME_DETECTION); > > + if (ret < 0) > > + return ret; > > + > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, > MDIO_C22EXT_GBE_PHY_RSI1_CTRL7, > > + MDIO_C22EXT_RSI_MAGIC_PKT_FRAME_DETECTION); > > + if (ret < 0) > > + return ret; > > + > > + /* Set the WoL enable bit */ > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, > MDIO_AN_RSVD_VEND_PROV1, > > + MDIO_MMD_AN_WOL_ENABLE); > > + if (ret < 0) > > + return ret; > > + > > + /* Set the WoL INT_N trigger bit */ > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, > MDIO_C22EXT_GBE_PHY_RSI1_CTRL8, > > + MDIO_C22EXT_RSI_WOL_FCS_MONITOR_MODE); > > + if (ret < 0) > > + return ret; > > + > > + /* Enable Interrupt INT_N Generation at pin level */ > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, > MDIO_C22EXT_GBE_PHY_SGMII_TX_INT_MASK1, > > + MDIO_C22EXT_SGMII0_WAKE_UP_FRAME_MASK | > > + MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK); > > + if (ret < 0) > > + return ret; > > + > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, > VEND1_GLOBAL_INT_STD_MASK, > > + VEND1_GLOBAL_INT_STD_MASK_ALL); > > + if (ret < 0) > > + return ret; > > + > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, > VEND1_GLOBAL_INT_VEND_MASK, > > + VEND1_GLOBAL_INT_VEND_MASK_GBE); > > + if (ret < 0) > > + return ret; > > + > > + /* Set the system interface to SGMII */ > > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, > > + VEND1_GLOBAL_CFG_100M, > VEND1_GLOBAL_SYS_CONFIG_SGMII | > > + VEND1_GLOBAL_SYS_CONFIG_AN); > > How do you know that SGMII should be used for 100M? > > > + if (ret < 0) > > + return ret; > > + > > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, > > + VEND1_GLOBAL_CFG_1G, > VEND1_GLOBAL_SYS_CONFIG_SGMII | > > + VEND1_GLOBAL_SYS_CONFIG_AN); > > How do you know that SGMII should be used for 1G? > > Doesn't this depend on the configuration of the host MAC and the capabilities of > it? If the host MAC only supports 10G, doesn't this break stuff? > > > + if (ret < 0) > > + return ret; > > + > > + /* restart auto-negotiation */ > > + genphy_c45_restart_aneg(phydev); > > + priv->wol_status = 1; > > + > > + return 0; > > +} > > + > > +static int aqr113c_wol_disable(struct phy_device *phydev) { > > + struct aqr107_priv *priv = phydev->priv; > > + int ret; > > + > > + /* Disable the WoL enable bit */ > > + ret = phy_clear_bits_mmd(phydev, MDIO_MMD_AN, > MDIO_AN_RSVD_VEND_PROV1, > > + MDIO_MMD_AN_WOL_ENABLE); > > + if (ret < 0) > > + return ret; > > + > > + /* Restore the SERDES/System Interface back to the XFI mode */ > > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, > > + VEND1_GLOBAL_CFG_100M, > VEND1_GLOBAL_SYS_CONFIG_XFI); > > + if (ret < 0) > > + return ret; > > + > > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, > > + VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_XFI); > > + if (ret < 0) > > + return ret; > > Conversely, how do you know that configuring 100M/1G to use 10GBASE-R on > the host interface is how the PHY was provisioned in firmware? I think at the > very least, you should be leaving these settings alone until you know that the > system is entering a low power mode, saving the settings, and restoring them > when you wake up. Regarding all the above comments , We are following the app note AN-N4209 by Marvell semiconductors for enabling and disabling of WOL. Below are the steps in brief as mentioned in app note For enabling the WOL, 1. Set the MAC address for the PHY. Make sure the MAC address is a legal address 2. Disables all advertised speeds except for the WoL speed (100BASE-TX FD or 1000BASE-T) 3. Enable the magic frame and wake up frame detection for the PHY 4. Set the WoL enable bit 5. Set the WoL INT_N trigger bit 6. Optional: Enable Interrupt INT_N Generation at pin level 7. Set the system interface to SGMII by writing into below registers MDIO Write 1e.31b = 0x0b (Sets SGMII for 100M) MDIO Write 1e.31c = 0x0b (Sets SGMII for 1G) 8. Perform a link re-negotiation/auto-negotiation 9. The WoL status bit should be 1 which indicates that the WoL is active. The PHY now is in sleep mode For remote WAKEUP via magic packet, 1.MAC detects INT from PHY and confirm Wake request. 2. Disable the WoL mode by unsetting the WoL enable bit. 3. Restore the SERDES/System Interface back to the original mode before WoL was initialized using SGMII mode i.e; back to XFI mode. MDIO write 1e.31b = 0x100 (Reverts the 100M setup to original mode) MDIO write 1e.31c = 0x100 (Reverts the 1G setup to original mode 4. Perform an auto-negotiation for the register values to take effect and establish a proper link Thanks, Revanth Uppala > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) 2023-07-24 11:29 ` Revanth Kumar Uppala @ 2023-07-24 12:29 ` Russell King (Oracle) 0 siblings, 0 replies; 21+ messages in thread From: Russell King (Oracle) @ 2023-07-24 12:29 UTC (permalink / raw) To: Revanth Kumar Uppala Cc: andrew, hkallweit1, netdev, linux-tegra, Narayan Reddy On Mon, Jul 24, 2023 at 11:29:39AM +0000, Revanth Kumar Uppala wrote: > > > > -----Original Message----- > > From: Russell King <linux@armlinux.org.uk> > > Sent: Wednesday, June 28, 2023 7:13 PM > > To: Revanth Kumar Uppala <ruppala@nvidia.com> > > Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux- > > tegra@vger.kernel.org; Narayan Reddy <narayanr@nvidia.com> > > Subject: Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) > > > > External email: Use caution opening links or attachments > > > > > > On Wed, Jun 28, 2023 at 06:13:26PM +0530, Revanth Kumar Uppala wrote: > > > @@ -109,6 +134,10 @@ > > > #define VEND1_GLOBAL_CFG_10M 0x0310 > > > #define VEND1_GLOBAL_CFG_100M 0x031b > > > #define VEND1_GLOBAL_CFG_1G 0x031c > > > +#define VEND1_GLOBAL_SYS_CONFIG_SGMII (BIT(0) | BIT(1)) > > > +#define VEND1_GLOBAL_SYS_CONFIG_AN BIT(3) > > > +#define VEND1_GLOBAL_SYS_CONFIG_XFI BIT(8) > > > > My understanding is that bits 2:0 are a _bitfield_ and not individual bits, which > > contain the following values: > I will define bitfield instead of defining individual bits in V2 series > > > > 0 - 10GBASE-R (XFI if you really want to call it that) > > 3 - SGMII > > 4 - OCSGMII (2.5G) > > 6 - 5GBASE-R (XFI5G if you really want to call it that) > > > > Bit 3 controls whether the SGMII control word is used, and this is the only > > applicable mode. > > > > Bit 8 is already defined - it's part of the rate adaption mode field, see > > VEND1_GLOBAL_CFG_RATE_ADAPT and > > VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE. > Sure, I will use above mentioned macros and will set the register values with help of FIELD_PREP in V2 series > > > > These bits apply to all the VEND1_GLOBAL_CFG_* registers, so these should be > > defined after the last register (0x031f). > Will take care of this. > > > > > +static int aqr113c_wol_enable(struct phy_device *phydev) { > > > + struct aqr107_priv *priv = phydev->priv; > > > + u16 val; > > > + int ret; > > > + > > > + /* Disables all advertised speeds except for the WoL > > > + * speed (100BASE-TX FD or 1000BASE-T) > > > + * This is set as per the APP note from Marvel > > > + */ > > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, > > MDIO_AN_10GBT_CTRL, > > > + MDIO_AN_LD_LOOP_TIMING_ABILITY); > > > + if (ret < 0) > > > + return ret; > > > + > > > + ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV); > > > + if (ret < 0) > > > + return ret; > > > + > > > + val = (ret & MDIO_AN_VEND_MASK) | > > > + (MDIO_AN_VEND_PROV_AQRATE_DWN_SHFT_CAP | > > MDIO_AN_VEND_PROV_1000BASET_FULL); > > > + ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV, > > val); > > > + if (ret < 0) > > > + return ret; > > > + > > > + /* Enable the magic frame and wake up frame detection for the PHY */ > > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, > > MDIO_C22EXT_GBE_PHY_RSI1_CTRL6, > > > + MDIO_C22EXT_RSI_WAKE_UP_FRAME_DETECTION); > > > + if (ret < 0) > > > + return ret; > > > + > > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, > > MDIO_C22EXT_GBE_PHY_RSI1_CTRL7, > > > + MDIO_C22EXT_RSI_MAGIC_PKT_FRAME_DETECTION); > > > + if (ret < 0) > > > + return ret; > > > + > > > + /* Set the WoL enable bit */ > > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, > > MDIO_AN_RSVD_VEND_PROV1, > > > + MDIO_MMD_AN_WOL_ENABLE); > > > + if (ret < 0) > > > + return ret; > > > + > > > + /* Set the WoL INT_N trigger bit */ > > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, > > MDIO_C22EXT_GBE_PHY_RSI1_CTRL8, > > > + MDIO_C22EXT_RSI_WOL_FCS_MONITOR_MODE); > > > + if (ret < 0) > > > + return ret; > > > + > > > + /* Enable Interrupt INT_N Generation at pin level */ > > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, > > MDIO_C22EXT_GBE_PHY_SGMII_TX_INT_MASK1, > > > + MDIO_C22EXT_SGMII0_WAKE_UP_FRAME_MASK | > > > + MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK); > > > + if (ret < 0) > > > + return ret; > > > + > > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, > > VEND1_GLOBAL_INT_STD_MASK, > > > + VEND1_GLOBAL_INT_STD_MASK_ALL); > > > + if (ret < 0) > > > + return ret; > > > + > > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, > > VEND1_GLOBAL_INT_VEND_MASK, > > > + VEND1_GLOBAL_INT_VEND_MASK_GBE); > > > + if (ret < 0) > > > + return ret; > > > + > > > + /* Set the system interface to SGMII */ > > > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, > > > + VEND1_GLOBAL_CFG_100M, > > VEND1_GLOBAL_SYS_CONFIG_SGMII | > > > + VEND1_GLOBAL_SYS_CONFIG_AN); > > > > How do you know that SGMII should be used for 100M? > > > > > + if (ret < 0) > > > + return ret; > > > + > > > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, > > > + VEND1_GLOBAL_CFG_1G, > > VEND1_GLOBAL_SYS_CONFIG_SGMII | > > > + VEND1_GLOBAL_SYS_CONFIG_AN); > > > > How do you know that SGMII should be used for 1G? > > > > Doesn't this depend on the configuration of the host MAC and the capabilities of > > it? If the host MAC only supports 10G, doesn't this break stuff? > > > > > + if (ret < 0) > > > + return ret; > > > + > > > + /* restart auto-negotiation */ > > > + genphy_c45_restart_aneg(phydev); > > > + priv->wol_status = 1; > > > + > > > + return 0; > > > +} > > > + > > > +static int aqr113c_wol_disable(struct phy_device *phydev) { > > > + struct aqr107_priv *priv = phydev->priv; > > > + int ret; > > > + > > > + /* Disable the WoL enable bit */ > > > + ret = phy_clear_bits_mmd(phydev, MDIO_MMD_AN, > > MDIO_AN_RSVD_VEND_PROV1, > > > + MDIO_MMD_AN_WOL_ENABLE); > > > + if (ret < 0) > > > + return ret; > > > + > > > + /* Restore the SERDES/System Interface back to the XFI mode */ > > > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, > > > + VEND1_GLOBAL_CFG_100M, > > VEND1_GLOBAL_SYS_CONFIG_XFI); > > > + if (ret < 0) > > > + return ret; > > > + > > > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, > > > + VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_XFI); > > > + if (ret < 0) > > > + return ret; > > > > Conversely, how do you know that configuring 100M/1G to use 10GBASE-R on > > the host interface is how the PHY was provisioned in firmware? I think at the > > very least, you should be leaving these settings alone until you know that the > > system is entering a low power mode, saving the settings, and restoring them > > when you wake up. > > Regarding all the above comments , > We are following the app note AN-N4209 by Marvell semiconductors for enabling and disabling of WOL. > Below are the steps in brief as mentioned in app note So basically what I gather is that the answer to "how do you know that configuring 100M/1G to use 10GBASE-R the host interface is how the PHY was provisioned in firmware?" is that you don't know, and you're just blindly following what someone's thrown into an application note but haven't thought enough about it. > For remote WAKEUP via magic packet, > 1.MAC detects INT from PHY and confirm Wake request. > 2. Disable the WoL mode by unsetting the WoL enable bit. > 3. Restore the SERDES/System Interface back to the original mode before WoL was initialized using SGMII mode i.e; back to XFI mode. > MDIO write 1e.31b = 0x100 (Reverts the 100M setup to original mode) > MDIO write 1e.31c = 0x100 (Reverts the 1G setup to original mode I think you have misunderstood step 3. "Restore ... back to the original mode" when interpreting the application note. Since these registers are set during the provisioning on the PHY, there is *no* guarantee that they were originally in XFI mode before WoL was enabled. Hence, in order to "restore" their state, you need to "save" their state at some point, and it would probably be a good idea to do that when: 1) the PHY is probed to get the power-up status. 2) update the saved registers whenever the driver reconfigures the PHY for a different interface mode (I don't think it does this.) 3) use this saved information to restore these registers when WoL is disabled, _or_ when the PHY device is detached from the PHY driver i.o.w. when the ->remove method is called, so that if the driver re-probes, it can get at the _original_ information. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) 2023-06-28 12:43 ` [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) Revanth Kumar Uppala 2023-06-28 13:43 ` Russell King (Oracle) @ 2023-06-28 14:17 ` Andrew Lunn 2023-07-24 11:30 ` Revanth Kumar Uppala 2023-06-28 18:57 ` kernel test robot 2 siblings, 1 reply; 21+ messages in thread From: Andrew Lunn @ 2023-06-28 14:17 UTC (permalink / raw) To: Revanth Kumar Uppala Cc: linux, hkallweit1, netdev, linux-tegra, Narayan Reddy > +static int aqr113c_wol_enable(struct phy_device *phydev) > +{ > + struct aqr107_priv *priv = phydev->priv; > + u16 val; > + int ret; > + > + /* Disables all advertised speeds except for the WoL > + * speed (100BASE-TX FD or 1000BASE-T) > + * This is set as per the APP note from Marvel > + */ > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL, > + MDIO_AN_LD_LOOP_TIMING_ABILITY); > + if (ret < 0) > + return ret; Please take a look at phylink_speed_down() and phylink_speed_up(). Assuming the PHY is not reporting it can do 10Full and 10Half, it should end up in 100BaseFull. Assuming the link partner can do 100BaseFull.... Russell points out you are making a lot of assumptions about the system side link. Ideally, you want to leave that to the PHY. Once the auto-neg at the lower speed has completed, it might change the system side link, e.g. to SGMII and the normal machinery should pass that onto the MAC, so it can follow. I would not force anything. > @@ -619,6 +784,31 @@ static int aqr107_config_init(struct phy_device *phydev) > if (ret < 0) > return ret; > > + /* Configure Magic packet frame pattern (MAC address) */ > + ret = phy_write_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_MAGIC_PKT_PATTERN_0_2_15, > + phydev->attached_dev->dev_addr[0] | > + (phydev->attached_dev->dev_addr[1] << 8)); I think most PHY drivers do this as part of enabling WOL. Doing it in aqr107_config_init() is early, is the MAC address stable yet? The user could change it. It could still be changed after wol is enabled, but at least the user has a clear point in time when WoL configuration happens. > +static void aqr113c_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) > +{ > + int val; > + > + val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_STATUS3); > + if (val < 0) > + return; > + > + wol->supported = WAKE_MAGIC; > + if (val & 0x1) > + wol->wolopts = WAKE_MAGIC; WoL seems to be tried to interrupts. So maybe you should actually check an interrupt is available? This is not going to work if the PHY is being polled. It does however get a bit messy, some boards might connect the 'interrupt' pin to PMIC. So there is not a true interrupt, but the PMIC can turn the power back on. Andrew ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) 2023-06-28 14:17 ` Andrew Lunn @ 2023-07-24 11:30 ` Revanth Kumar Uppala 0 siblings, 0 replies; 21+ messages in thread From: Revanth Kumar Uppala @ 2023-07-24 11:30 UTC (permalink / raw) To: Andrew Lunn; +Cc: linux, hkallweit1, netdev, linux-tegra, Narayan Reddy > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Wednesday, June 28, 2023 7:48 PM > To: Revanth Kumar Uppala <ruppala@nvidia.com> > Cc: linux@armlinux.org.uk; hkallweit1@gmail.com; netdev@vger.kernel.org; > linux-tegra@vger.kernel.org; Narayan Reddy <narayanr@nvidia.com> > Subject: Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) > > External email: Use caution opening links or attachments > > > > +static int aqr113c_wol_enable(struct phy_device *phydev) { > > + struct aqr107_priv *priv = phydev->priv; > > + u16 val; > > + int ret; > > + > > + /* Disables all advertised speeds except for the WoL > > + * speed (100BASE-TX FD or 1000BASE-T) > > + * This is set as per the APP note from Marvel > > + */ > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, > MDIO_AN_10GBT_CTRL, > > + MDIO_AN_LD_LOOP_TIMING_ABILITY); > > + if (ret < 0) > > + return ret; > > Please take a look at phylink_speed_down() and phylink_speed_up(). Assuming > the PHY is not reporting it can do 10Full and 10Half, it should end up in > 100BaseFull. Assuming the link partner can do 100BaseFull.... > > Russell points out you are making a lot of assumptions about the system side > link. Ideally, you want to leave that to the PHY. Once the auto-neg at the lower > speed has completed, it might change the system side link, e.g. to SGMII and the > normal machinery should pass that onto the MAC, so it can follow. I would not > force anything. As per my reply to Russel's comment, we are following the app note AN-N4209 by Marvell semiconductors for enabling and disabling of WOL and above logic is part of the same app note. > > > @@ -619,6 +784,31 @@ static int aqr107_config_init(struct phy_device > *phydev) > > if (ret < 0) > > return ret; > > > > + /* Configure Magic packet frame pattern (MAC address) */ > > + ret = phy_write_mmd(phydev, MDIO_MMD_C22EXT, > MDIO_C22EXT_MAGIC_PKT_PATTERN_0_2_15, > > + phydev->attached_dev->dev_addr[0] | > > + (phydev->attached_dev->dev_addr[1] << 8)); > > I think most PHY drivers do this as part of enabling WOL. Doing it in > aqr107_config_init() is early, is the MAC address stable yet? The user could > change it. It could still be changed after wol is enabled, but at least the user has > a clear point in time when WoL configuration happens. Yes, your assumption is correct. Will move this logic to aqr113c_wol_enable() function to take care of above scenario. Thanks, Revanth Uppala > > > +static void aqr113c_get_wol(struct phy_device *phydev, struct > > +ethtool_wolinfo *wol) { > > + int val; > > + > > + val = phy_read_mmd(phydev, MDIO_MMD_AN, > MDIO_AN_RSVD_VEND_STATUS3); > > + if (val < 0) > > + return; > > + > > + wol->supported = WAKE_MAGIC; > > + if (val & 0x1) > > + wol->wolopts = WAKE_MAGIC; > > WoL seems to be tried to interrupts. So maybe you should actually check an > interrupt is available? This is not going to work if the PHY is being polled. It does > however get a bit messy, some boards might connect the 'interrupt' pin to PMIC. > So there is not a true interrupt, but the PMIC can turn the power back on. > > Andrew ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) 2023-06-28 12:43 ` [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) Revanth Kumar Uppala 2023-06-28 13:43 ` Russell King (Oracle) 2023-06-28 14:17 ` Andrew Lunn @ 2023-06-28 18:57 ` kernel test robot 2 siblings, 0 replies; 21+ messages in thread From: kernel test robot @ 2023-06-28 18:57 UTC (permalink / raw) To: Revanth Kumar Uppala, linux, andrew, hkallweit1, netdev Cc: oe-kbuild-all, linux-tegra, Revanth Kumar Uppala, Narayan Reddy Hi Revanth, kernel test robot noticed the following build warnings: [auto build test WARNING on net/main] [also build test WARNING on net-next/main linus/master horms-ipvs/master v6.4 next-20230628] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Revanth-Kumar-Uppala/net-phy-aquantia-Enable-MAC-Controlled-EEE/20230628-204746 base: net/main patch link: https://lore.kernel.org/r/20230628124326.55732-4-ruppala%40nvidia.com patch subject: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) config: i386-randconfig-i013-20230628 (https://download.01.org/0day-ci/archive/20230629/202306290253.b8D3gQf8-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce: (https://download.01.org/0day-ci/archive/20230629/202306290253.b8D3gQf8-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202306290253.b8D3gQf8-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/net/phy/aquantia_main.c: In function 'aqr_handle_interrupt': >> drivers/net/phy/aquantia_main.c:476:29: warning: unused variable 'priv' [-Wunused-variable] 476 | struct aqr107_priv *priv = phydev->priv; | ^~~~ vim +/priv +476 drivers/net/phy/aquantia_main.c 473 474 static irqreturn_t aqr_handle_interrupt(struct phy_device *phydev) 475 { > 476 struct aqr107_priv *priv = phydev->priv; 477 int irq_status; 478 int ret; 479 480 ret = phy_read_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_SGMII_TX_ALARM1); 481 if (ret < 0) { 482 phy_error(phydev); 483 return IRQ_NONE; 484 } 485 486 if ((ret & MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK) == 487 MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK) { 488 /* Disable the WoL */ 489 ret = aqr113c_wol_disable(phydev); 490 if (ret < 0) 491 return IRQ_NONE; 492 } 493 494 irq_status = phy_read_mmd(phydev, MDIO_MMD_AN, 495 MDIO_AN_TX_VEND_INT_STATUS2); 496 if (irq_status < 0) { 497 phy_error(phydev); 498 return IRQ_NONE; 499 } 500 501 if (!(irq_status & MDIO_AN_TX_VEND_INT_STATUS2_MASK)) 502 return IRQ_NONE; 503 504 phy_trigger_machine(phydev); 505 506 return IRQ_HANDLED; 507 } 508 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY 2023-06-28 12:43 [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY Revanth Kumar Uppala ` (2 preceding siblings ...) 2023-06-28 12:43 ` [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) Revanth Kumar Uppala @ 2023-06-28 13:30 ` Russell King (Oracle) 2023-06-28 13:46 ` Andrew Lunn 2023-06-28 13:46 ` Russell King (Oracle) 4 siblings, 1 reply; 21+ messages in thread From: Russell King (Oracle) @ 2023-06-28 13:30 UTC (permalink / raw) To: Revanth Kumar Uppala Cc: andrew, hkallweit1, netdev, linux-tegra, Narayan Reddy On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote: > From: Narayan Reddy <narayanr@nvidia.com> > > Enable flow control support using pause frames in aquantia phy driver. > > Signed-off-by: Narayan Reddy <narayanr@nvidia.com> > Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com> I think this is over-complex. > #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK GENMASK(7, 3) > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR 0 > @@ -583,6 +585,17 @@ static int aqr107_config_init(struct phy_device *phydev) > if (!ret) > aqr107_chip_info(phydev); > > + /* Advertize flow control */ > + linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported); > + linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported); > + linkmode_copy(phydev->advertising, phydev->supported); This is the wrong place to be doing this, since pause support depends not only on the PHY but also on the MAC. There are phylib interfaces that MACs should call so that phylib knows that the MAC supports pause frames. Secondly, the PHY driver needs to tell phylib that the PHY supports pause frames, and that's done through either setting the .features member in the PHY driver, or by providing a .get_features implementation. Configuration of the pause advertisement should already be happening through the core phylib code. Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY 2023-06-28 13:30 ` [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY Russell King (Oracle) @ 2023-06-28 13:46 ` Andrew Lunn 2023-07-24 11:29 ` Revanth Kumar Uppala 0 siblings, 1 reply; 21+ messages in thread From: Andrew Lunn @ 2023-06-28 13:46 UTC (permalink / raw) To: Russell King (Oracle) Cc: Revanth Kumar Uppala, hkallweit1, netdev, linux-tegra, Narayan Reddy On Wed, Jun 28, 2023 at 02:30:48PM +0100, Russell King (Oracle) wrote: > On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote: > > From: Narayan Reddy <narayanr@nvidia.com> > > > > Enable flow control support using pause frames in aquantia phy driver. > > > > Signed-off-by: Narayan Reddy <narayanr@nvidia.com> > > Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com> > > I think this is over-complex. > > > #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 > > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK GENMASK(7, 3) > > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR 0 > > @@ -583,6 +585,17 @@ static int aqr107_config_init(struct phy_device *phydev) > > if (!ret) > > aqr107_chip_info(phydev); > > > > + /* Advertize flow control */ > > + linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported); > > + linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported); > > + linkmode_copy(phydev->advertising, phydev->supported); > > This is the wrong place to be doing this, since pause support depends > not only on the PHY but also on the MAC. There are phylib interfaces > that MACs should call so that phylib knows that the MAC supports pause > frames. > > Secondly, the PHY driver needs to tell phylib that the PHY supports > pause frames, and that's done through either setting the .features > member in the PHY driver, or by providing a .get_features > implementation. > > Configuration of the pause advertisement should already be happening > through the core phylib code. I really should do a LPC netdev talk "Everybody gets pause wrong..." genphy_c45_an_config_aneg() will configure pause advertisement. The PHY driver does not need to configure it, if the PHY follows the standard and has the configuration in the correct place. As Russell said, please check the PHYs ability to advertise pause is being reported correctly, by .get_features, of the default implementation of .get_features if that is being used. And then check your MAC driver is also indicating it supports pause. Andrew ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY 2023-06-28 13:46 ` Andrew Lunn @ 2023-07-24 11:29 ` Revanth Kumar Uppala 2023-07-24 11:47 ` Russell King (Oracle) 0 siblings, 1 reply; 21+ messages in thread From: Revanth Kumar Uppala @ 2023-07-24 11:29 UTC (permalink / raw) To: Andrew Lunn, Russell King (Oracle) Cc: hkallweit1, netdev, linux-tegra, Narayan Reddy > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Wednesday, June 28, 2023 7:17 PM > To: Russell King (Oracle) <linux@armlinux.org.uk> > Cc: Revanth Kumar Uppala <ruppala@nvidia.com>; hkallweit1@gmail.com; > netdev@vger.kernel.org; linux-tegra@vger.kernel.org; Narayan Reddy > <narayanr@nvidia.com> > Subject: Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support > in aquantia PHY > > External email: Use caution opening links or attachments > > > On Wed, Jun 28, 2023 at 02:30:48PM +0100, Russell King (Oracle) wrote: > > On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote: > > > From: Narayan Reddy <narayanr@nvidia.com> > > > > > > Enable flow control support using pause frames in aquantia phy driver. > > > > > > Signed-off-by: Narayan Reddy <narayanr@nvidia.com> > > > Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com> > > > > I think this is over-complex. > > > > > #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 > > > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK GENMASK(7, 3) > > > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR 0 @@ -583,6 +585,17 > @@ > > > static int aqr107_config_init(struct phy_device *phydev) > > > if (!ret) > > > aqr107_chip_info(phydev); > > > > > > + /* Advertize flow control */ > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev- > >supported); > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev- > >supported); > > > + linkmode_copy(phydev->advertising, phydev->supported); > > > > This is the wrong place to be doing this, since pause support depends > > not only on the PHY but also on the MAC. There are phylib interfaces > > that MACs should call so that phylib knows that the MAC supports pause > > frames. > > > > Secondly, the PHY driver needs to tell phylib that the PHY supports > > pause frames, and that's done through either setting the .features > > member in the PHY driver, or by providing a .get_features > > implementation. > > > > Configuration of the pause advertisement should already be happening > > through the core phylib code. > > I really should do a LPC netdev talk "Everybody gets pause wrong..." > > genphy_c45_an_config_aneg() will configure pause advertisement. The PHY > driver does not need to configure it, if the PHY follows the standard and has the > configuration in the correct place. As Russell said, please check the PHYs ability > to advertise pause is being reported correctly, by .get_features, of the default > implementation of .get_features if that is being used. And then check your MAC > driver is also indicating it supports pause. From .get_features, it is not possible to check PHY's ability to advertise pause is being reported as there is no such register present for AQR PHY to check capabilities in its datasheet. Hence, we are directly configuring the pause frames from aqr107_config_init(). Thanks, Revanth Uppala > > Andrew ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY 2023-07-24 11:29 ` Revanth Kumar Uppala @ 2023-07-24 11:47 ` Russell King (Oracle) 0 siblings, 0 replies; 21+ messages in thread From: Russell King (Oracle) @ 2023-07-24 11:47 UTC (permalink / raw) To: Revanth Kumar Uppala Cc: Andrew Lunn, hkallweit1, netdev, linux-tegra, Narayan Reddy On Mon, Jul 24, 2023 at 11:29:18AM +0000, Revanth Kumar Uppala wrote: > > > > -----Original Message----- > > From: Andrew Lunn <andrew@lunn.ch> > > Sent: Wednesday, June 28, 2023 7:17 PM > > To: Russell King (Oracle) <linux@armlinux.org.uk> > > Cc: Revanth Kumar Uppala <ruppala@nvidia.com>; hkallweit1@gmail.com; > > netdev@vger.kernel.org; linux-tegra@vger.kernel.org; Narayan Reddy > > <narayanr@nvidia.com> > > Subject: Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support > > in aquantia PHY > > > > External email: Use caution opening links or attachments > > > > > > On Wed, Jun 28, 2023 at 02:30:48PM +0100, Russell King (Oracle) wrote: > > > On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote: > > > > From: Narayan Reddy <narayanr@nvidia.com> > > > > > > > > Enable flow control support using pause frames in aquantia phy driver. > > > > > > > > Signed-off-by: Narayan Reddy <narayanr@nvidia.com> > > > > Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com> > > > > > > I think this is over-complex. > > > > > > > #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 > > > > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK GENMASK(7, 3) > > > > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR 0 @@ -583,6 +585,17 > > @@ > > > > static int aqr107_config_init(struct phy_device *phydev) > > > > if (!ret) > > > > aqr107_chip_info(phydev); > > > > > > > > + /* Advertize flow control */ > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev- > > >supported); > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev- > > >supported); > > > > + linkmode_copy(phydev->advertising, phydev->supported); > > > > > > This is the wrong place to be doing this, since pause support depends > > > not only on the PHY but also on the MAC. There are phylib interfaces > > > that MACs should call so that phylib knows that the MAC supports pause > > > frames. > > > > > > Secondly, the PHY driver needs to tell phylib that the PHY supports > > > pause frames, and that's done through either setting the .features > > > member in the PHY driver, or by providing a .get_features > > > implementation. > > > > > > Configuration of the pause advertisement should already be happening > > > through the core phylib code. > > > > I really should do a LPC netdev talk "Everybody gets pause wrong..." > > > > genphy_c45_an_config_aneg() will configure pause advertisement. The PHY > > driver does not need to configure it, if the PHY follows the standard and has the > > configuration in the correct place. As Russell said, please check the PHYs ability > > to advertise pause is being reported correctly, by .get_features, of the default > > implementation of .get_features if that is being used. And then check your MAC > > driver is also indicating it supports pause. > From .get_features, it is not possible to check PHY's ability to advertise pause is being reported as there is no such register present for AQR PHY to check capabilities in its datasheet. > Hence, we are directly configuring the pause frames from aqr107_config_init(). ... and thus creating a trashy implementation... so NAK. The first thing to get straight is that in "normal" non-rate adapting setups, pause frames are no different from normal Ethernet frames as far as a PHY is concerned. The PHY should be doing absolutely nothing special with pause frames - it should merely forward them to the MAC, and it's the MAC that deals with pause frames. The only thing that a non-rate adapting baseT PHY has to deal with is the media advertisement and nothing else. So, whether pause frames are supported has more to do with the MAC than the PHY. The way phylib works is that when a PHY is probed, phy_probe() will set the Pause and Asym_Pause bits in the ->supported bitmap. It is then up to the MAC driver (or phylink) to call phy_support_asym_pause() or phy_support_sym_pause() to tell phylib what the MAC supports. PHY drivers must *not* override this in their config_init() function, and most certainly must not copy the supported bitmap to the advertising bitmap there either. If you need pause-mode rate adaption, this has nothing to do with the media side, and ->supported and ->advertising are not relevant for that. Non-phylink based MAC drivers have to use phy_get_rate_matching() to find out whether the PHY will be using rate adaption and act accordingly. phylink based MAC drivers have this dealt with for them and as long as they do what phylink tells them to do, everything should be fine. So, basically, do not mess with setting bits in the ->supported bitmap nor the ->advertising bitmap in config_init. It is wrong, and we will NAK it. Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY 2023-06-28 12:43 [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY Revanth Kumar Uppala ` (3 preceding siblings ...) 2023-06-28 13:30 ` [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY Russell King (Oracle) @ 2023-06-28 13:46 ` Russell King (Oracle) 4 siblings, 0 replies; 21+ messages in thread From: Russell King (Oracle) @ 2023-06-28 13:46 UTC (permalink / raw) To: Revanth Kumar Uppala Cc: andrew, hkallweit1, netdev, linux-tegra, Narayan Reddy Final comments on the series, posted here because there's no cover message. 1. The series should include a cover message summarising the overall purpose of the series is, giving an overview of the changes, and a diffstat for the whole series. 2. The subject line should contain the tree that the patches are targetting, e.g. "[PATCH net-next 0/4] Add support for WoL to Aquantia PHY driver". 3. The tree should be identified in each patch mailed out. 4. The patches should be threaded to the cover message. Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2023-07-24 12:29 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-28 12:43 [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY Revanth Kumar Uppala 2023-06-28 12:43 ` [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE Revanth Kumar Uppala 2023-06-28 13:54 ` Andrew Lunn 2023-07-24 11:29 ` Revanth Kumar Uppala 2023-07-24 11:52 ` Russell King (Oracle) 2023-06-28 12:43 ` [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side Revanth Kumar Uppala 2023-06-28 13:33 ` Russell King (Oracle) 2023-07-24 11:29 ` Revanth Kumar Uppala 2023-07-24 11:57 ` Russell King (Oracle) 2023-06-28 12:43 ` [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL) Revanth Kumar Uppala 2023-06-28 13:43 ` Russell King (Oracle) 2023-07-24 11:29 ` Revanth Kumar Uppala 2023-07-24 12:29 ` Russell King (Oracle) 2023-06-28 14:17 ` Andrew Lunn 2023-07-24 11:30 ` Revanth Kumar Uppala 2023-06-28 18:57 ` kernel test robot 2023-06-28 13:30 ` [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY Russell King (Oracle) 2023-06-28 13:46 ` Andrew Lunn 2023-07-24 11:29 ` Revanth Kumar Uppala 2023-07-24 11:47 ` Russell King (Oracle) 2023-06-28 13:46 ` Russell King (Oracle)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).