Re: [PATCH net-next 1/2] net: phy: Add Wake-on-LAN driver for Microsemi PHYs.
Hi Florian,
Thank you for code review and valuable comments.
On Wed, Sep 28, 2016 at 10:37:07AM -0700, Florian Fainelli wrote:
> EXTERNAL EMAIL
>
>
> On 09/28/2016 05:01 AM, Raju Lakkaraju wrote:
> > From: Raju Lakkaraju
> >
> > Wake-on-LAN (WoL) is an Ethernet networking standard that allows
> > a computer/device to be turned on or awakened by a network message.
> > VSC8531 PHY can support this feature configure by driver set function.
> > WoL status get by driver get function.
> >
> > Tested on Beaglebone Black with VSC 8531 PHY.
> >
> > Signed-off-by: Raju Lakkaraju
> > ---
> > drivers/net/phy/mscc.c | 132
> > +
> > 1 file changed, 132 insertions(+)
> >
> > diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> > index d350deb..ca6ea23 100644
> > --- a/drivers/net/phy/mscc.c
> > +++ b/drivers/net/phy/mscc.c
> > @@ -11,6 +11,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > enum rgmii_rx_clock_delay {
> > RGMII_RX_CLK_DELAY_0_2_NS = 0,
> > @@ -35,6 +36,7 @@ enum rgmii_rx_clock_delay {
> >
> > #define MII_VSC85XX_INT_MASK 25
> > #define MII_VSC85XX_INT_MASK_MASK 0xa000
> > +#define MII_VSC85XX_INT_MASK_WOL 0x0040
> > #define MII_VSC85XX_INT_STATUS 26
> >
> > #define MSCC_EXT_PAGE_ACCESS 31
> > @@ -46,6 +48,19 @@ enum rgmii_rx_clock_delay {
> > #define RGMII_RX_CLK_DELAY_MASK0x0070
> > #define RGMII_RX_CLK_DELAY_POS 4
> >
> > +#define MSCC_PHY_WOL_LOWER_MAC_ADDR21
> > +#define MSCC_PHY_WOL_MID_MAC_ADDR 22
> > +#define MSCC_PHY_WOL_UPPER_MAC_ADDR23
> > +#define MSCC_PHY_WOL_LOWER_PASSWD 24
> > +#define MSCC_PHY_WOL_MID_PASSWD25
> > +#define MSCC_PHY_WOL_UPPER_PASSWD 26
> > +
> > +#define MSCC_PHY_WOL_MAC_CONTROL 27
> > +#define EDGE_RATE_CNTL_POS 5
> > +#define EDGE_RATE_CNTL_MASK0x00E0
> > +#define SECURE_ON_ENABLE 0x8000
> > +#define SECURE_ON_PASSWD_LEN_4 0x4000
> > +
> > /* Microsemi PHY ID's */
> > #define PHY_ID_VSC8531 0x00070570
> > #define PHY_ID_VSC8541 0x00070770
> > @@ -58,6 +73,119 @@ static int vsc85xx_phy_page_set(struct phy_device
> > *phydev, u8 page)
> > return rc;
> > }
> >
> > +static int vsc85xx_wol_set(struct phy_device *phydev,
> > +struct ethtool_wolinfo *wol)
> > +{
> > + int rc;
> > + u16 reg_val;
> > + struct ethtool_wolinfo *wol_conf = wol;
> > +
> > + mutex_lock(&phydev->lock);
>
> This mutex is used here because you are using an indirect page access,
> right? This is not to protect against multiple calls of wol_set from
> different executing threads?
>
Correct. mutex is used for indirect page access.
I did find any protect against multiple calls of wol_set in other vendors.
Do you have any suggestions?
> > + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
> > + if (rc != 0)
> > + goto out_unlock;
> > +
> > + if (wol->wolopts & WAKE_MAGIC) {
> > + /* Store the device address for the magic packet */
> > + reg_val = phydev->attached_dev->dev_addr[4] << 8;
> > + reg_val |= phydev->attached_dev->dev_addr[5];
> > + phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, reg_val);
> > + reg_val = phydev->attached_dev->dev_addr[2] << 8;
> > + reg_val |= phydev->attached_dev->dev_addr[3];
> > + phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, reg_val);
> > + reg_val = phydev->attached_dev->dev_addr[0] << 8;
> > + reg_val |= phydev->attached_dev->dev_addr[1];
> > + phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, reg_val);
> > + } else {
> > + phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, 0);
> > + phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, 0);
> > + phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, 0);
> > + }
> > +
> > + reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
> > + if (wol_conf->wolopts & WAKE_MAGICSECURE)
> > + reg_val |= SECURE_ON_ENABLE;
> > + else
> > + reg_val &= ~SECURE_ON_ENABLE;
> > + phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
> > +
> > + if (wol_conf->wolopts & WAKE_MAGICSECURE) {
> > + reg_val = wol_conf->sopass[4] << 8;
> > + reg_val |= wol_conf->sopass[5];
> > + phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, reg_val);
> > + reg_val = wol_conf->sopass[2] << 8;
> > + reg_val |= wol_conf->sopass[3];
> > + phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, reg_val);
> > + reg_val = wol_conf->sopass[0] << 8;
> > + reg_val |= wol_conf->sopass[1];
> > + phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, reg_val);
> > + } else {
> > + phy_write(phy
Re: [PATCH net-next 1/2] net: phy: Add Wake-on-LAN driver for Microsemi PHYs.
Hi Andrew,
Thank you for code review and valuable comments.
On Wed, Sep 28, 2016 at 06:27:05PM +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL
>
>
> > +#define MSCC_PHY_WOL_MAC_CONTROL 27
> > +#define EDGE_RATE_CNTL_POS 5
> > +#define EDGE_RATE_CNTL_MASK0x00E0
>
> This patch does not require these two #defines.
>
> Please indicate in the cover note if the patches depends on other
> patches in order to cleanly apply. Or if these patches are going to
> conflict with some other patches.
>
Accepted. I will remove those 2 defines.
> > + reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
> > + if (wol_conf->wolopts & WAKE_MAGICSECURE)
> > + reg_val |= SECURE_ON_ENABLE;
> > + else
> > + reg_val &= ~SECURE_ON_ENABLE;
> > + phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
> > +
> > + if (wol_conf->wolopts & WAKE_MAGICSECURE) {
> > + reg_val = wol_conf->sopass[4] << 8;
> > + reg_val |= wol_conf->sopass[5];
> > + phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, reg_val);
> > + reg_val = wol_conf->sopass[2] << 8;
> > + reg_val |= wol_conf->sopass[3];
> > + phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, reg_val);
> > + reg_val = wol_conf->sopass[0] << 8;
> > + reg_val |= wol_conf->sopass[1];
> > + phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, reg_val);
> > + } else {
> > + phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, 0);
> > + phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, 0);
> > + phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, 0);
> > + }
>
> Wouldn't it be better to set the password, and then enable the
> password feature?
>
Accepted. I will change.
> I don't know much about WOL. Hopefully Florian will add further
> comments.
>
> Andrew
---
Thanks,
Raju.
Re: [PATCH net-next 1/2] net: phy: Add Wake-on-LAN driver for Microsemi PHYs.
On 09/28/2016 05:01 AM, Raju Lakkaraju wrote:
> From: Raju Lakkaraju
>
> Wake-on-LAN (WoL) is an Ethernet networking standard that allows
> a computer/device to be turned on or awakened by a network message.
> VSC8531 PHY can support this feature configure by driver set function.
> WoL status get by driver get function.
>
> Tested on Beaglebone Black with VSC 8531 PHY.
>
> Signed-off-by: Raju Lakkaraju
> ---
> drivers/net/phy/mscc.c | 132
> +
> 1 file changed, 132 insertions(+)
>
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> index d350deb..ca6ea23 100644
> --- a/drivers/net/phy/mscc.c
> +++ b/drivers/net/phy/mscc.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
>
> enum rgmii_rx_clock_delay {
> RGMII_RX_CLK_DELAY_0_2_NS = 0,
> @@ -35,6 +36,7 @@ enum rgmii_rx_clock_delay {
>
> #define MII_VSC85XX_INT_MASK 25
> #define MII_VSC85XX_INT_MASK_MASK 0xa000
> +#define MII_VSC85XX_INT_MASK_WOL 0x0040
> #define MII_VSC85XX_INT_STATUS 26
>
> #define MSCC_EXT_PAGE_ACCESS 31
> @@ -46,6 +48,19 @@ enum rgmii_rx_clock_delay {
> #define RGMII_RX_CLK_DELAY_MASK0x0070
> #define RGMII_RX_CLK_DELAY_POS 4
>
> +#define MSCC_PHY_WOL_LOWER_MAC_ADDR21
> +#define MSCC_PHY_WOL_MID_MAC_ADDR 22
> +#define MSCC_PHY_WOL_UPPER_MAC_ADDR23
> +#define MSCC_PHY_WOL_LOWER_PASSWD 24
> +#define MSCC_PHY_WOL_MID_PASSWD25
> +#define MSCC_PHY_WOL_UPPER_PASSWD 26
> +
> +#define MSCC_PHY_WOL_MAC_CONTROL 27
> +#define EDGE_RATE_CNTL_POS 5
> +#define EDGE_RATE_CNTL_MASK0x00E0
> +#define SECURE_ON_ENABLE 0x8000
> +#define SECURE_ON_PASSWD_LEN_4 0x4000
> +
> /* Microsemi PHY ID's */
> #define PHY_ID_VSC8531 0x00070570
> #define PHY_ID_VSC8541 0x00070770
> @@ -58,6 +73,119 @@ static int vsc85xx_phy_page_set(struct phy_device
> *phydev, u8 page)
> return rc;
> }
>
> +static int vsc85xx_wol_set(struct phy_device *phydev,
> +struct ethtool_wolinfo *wol)
> +{
> + int rc;
> + u16 reg_val;
> + struct ethtool_wolinfo *wol_conf = wol;
> +
> + mutex_lock(&phydev->lock);
This mutex is used here because you are using an indirect page access,
right? This is not to protect against multiple calls of wol_set from
different executing threads?
> + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
> + if (rc != 0)
> + goto out_unlock;
> +
> + if (wol->wolopts & WAKE_MAGIC) {
> + /* Store the device address for the magic packet */
> + reg_val = phydev->attached_dev->dev_addr[4] << 8;
> + reg_val |= phydev->attached_dev->dev_addr[5];
> + phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, reg_val);
> + reg_val = phydev->attached_dev->dev_addr[2] << 8;
> + reg_val |= phydev->attached_dev->dev_addr[3];
> + phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, reg_val);
> + reg_val = phydev->attached_dev->dev_addr[0] << 8;
> + reg_val |= phydev->attached_dev->dev_addr[1];
> + phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, reg_val);
> + } else {
> + phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, 0);
> + phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, 0);
> + phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, 0);
> + }
> +
> + reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
> + if (wol_conf->wolopts & WAKE_MAGICSECURE)
> + reg_val |= SECURE_ON_ENABLE;
> + else
> + reg_val &= ~SECURE_ON_ENABLE;
> + phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
> +
> + if (wol_conf->wolopts & WAKE_MAGICSECURE) {
> + reg_val = wol_conf->sopass[4] << 8;
> + reg_val |= wol_conf->sopass[5];
> + phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, reg_val);
> + reg_val = wol_conf->sopass[2] << 8;
> + reg_val |= wol_conf->sopass[3];
> + phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, reg_val);
> + reg_val = wol_conf->sopass[0] << 8;
> + reg_val |= wol_conf->sopass[1];
> + phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, reg_val);
> + } else {
> + phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, 0);
> + phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, 0);
> + phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, 0);
> + }
How about making the code a little simpler in both cases with something
like this the following:
u16 pwd = { };
unsigned int i;
if (wol_conf->wolopts & WAKE_MAGICECURE)
for (i = 0; i < ARRAY_SIZE(pwd); i++)
pwd[i] = wol_conf->so_pass[5 - (i * 2 + 1)] << 8|
Re: [PATCH net-next 1/2] net: phy: Add Wake-on-LAN driver for Microsemi PHYs.
> +#define MSCC_PHY_WOL_MAC_CONTROL 27
> +#define EDGE_RATE_CNTL_POS 5
> +#define EDGE_RATE_CNTL_MASK0x00E0
This patch does not require these two #defines.
Please indicate in the cover note if the patches depends on other
patches in order to cleanly apply. Or if these patches are going to
conflict with some other patches.
> + reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
> + if (wol_conf->wolopts & WAKE_MAGICSECURE)
> + reg_val |= SECURE_ON_ENABLE;
> + else
> + reg_val &= ~SECURE_ON_ENABLE;
> + phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
> +
> + if (wol_conf->wolopts & WAKE_MAGICSECURE) {
> + reg_val = wol_conf->sopass[4] << 8;
> + reg_val |= wol_conf->sopass[5];
> + phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, reg_val);
> + reg_val = wol_conf->sopass[2] << 8;
> + reg_val |= wol_conf->sopass[3];
> + phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, reg_val);
> + reg_val = wol_conf->sopass[0] << 8;
> + reg_val |= wol_conf->sopass[1];
> + phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, reg_val);
> + } else {
> + phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, 0);
> + phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, 0);
> + phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, 0);
> + }
Wouldn't it be better to set the password, and then enable the
password feature?
I don't know much about WOL. Hopefully Florian will add further
comments.
Andrew
[PATCH net-next 1/2] net: phy: Add Wake-on-LAN driver for Microsemi PHYs.
From: Raju Lakkaraju
Wake-on-LAN (WoL) is an Ethernet networking standard that allows
a computer/device to be turned on or awakened by a network message.
VSC8531 PHY can support this feature configure by driver set function.
WoL status get by driver get function.
Tested on Beaglebone Black with VSC 8531 PHY.
Signed-off-by: Raju Lakkaraju
---
drivers/net/phy/mscc.c | 132 +
1 file changed, 132 insertions(+)
diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index d350deb..ca6ea23 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
enum rgmii_rx_clock_delay {
RGMII_RX_CLK_DELAY_0_2_NS = 0,
@@ -35,6 +36,7 @@ enum rgmii_rx_clock_delay {
#define MII_VSC85XX_INT_MASK 25
#define MII_VSC85XX_INT_MASK_MASK0xa000
+#define MII_VSC85XX_INT_MASK_WOL 0x0040
#define MII_VSC85XX_INT_STATUS 26
#define MSCC_EXT_PAGE_ACCESS 31
@@ -46,6 +48,19 @@ enum rgmii_rx_clock_delay {
#define RGMII_RX_CLK_DELAY_MASK 0x0070
#define RGMII_RX_CLK_DELAY_POS 4
+#define MSCC_PHY_WOL_LOWER_MAC_ADDR 21
+#define MSCC_PHY_WOL_MID_MAC_ADDR22
+#define MSCC_PHY_WOL_UPPER_MAC_ADDR 23
+#define MSCC_PHY_WOL_LOWER_PASSWD24
+#define MSCC_PHY_WOL_MID_PASSWD 25
+#define MSCC_PHY_WOL_UPPER_PASSWD26
+
+#define MSCC_PHY_WOL_MAC_CONTROL 27
+#define EDGE_RATE_CNTL_POS 5
+#define EDGE_RATE_CNTL_MASK 0x00E0
+#define SECURE_ON_ENABLE 0x8000
+#define SECURE_ON_PASSWD_LEN_4 0x4000
+
/* Microsemi PHY ID's */
#define PHY_ID_VSC8531 0x00070570
#define PHY_ID_VSC8541 0x00070770
@@ -58,6 +73,119 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev,
u8 page)
return rc;
}
+static int vsc85xx_wol_set(struct phy_device *phydev,
+ struct ethtool_wolinfo *wol)
+{
+ int rc;
+ u16 reg_val;
+ struct ethtool_wolinfo *wol_conf = wol;
+
+ mutex_lock(&phydev->lock);
+ rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+ if (rc != 0)
+ goto out_unlock;
+
+ if (wol->wolopts & WAKE_MAGIC) {
+ /* Store the device address for the magic packet */
+ reg_val = phydev->attached_dev->dev_addr[4] << 8;
+ reg_val |= phydev->attached_dev->dev_addr[5];
+ phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, reg_val);
+ reg_val = phydev->attached_dev->dev_addr[2] << 8;
+ reg_val |= phydev->attached_dev->dev_addr[3];
+ phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, reg_val);
+ reg_val = phydev->attached_dev->dev_addr[0] << 8;
+ reg_val |= phydev->attached_dev->dev_addr[1];
+ phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, reg_val);
+ } else {
+ phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, 0);
+ phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, 0);
+ phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, 0);
+ }
+
+ reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
+ if (wol_conf->wolopts & WAKE_MAGICSECURE)
+ reg_val |= SECURE_ON_ENABLE;
+ else
+ reg_val &= ~SECURE_ON_ENABLE;
+ phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
+
+ if (wol_conf->wolopts & WAKE_MAGICSECURE) {
+ reg_val = wol_conf->sopass[4] << 8;
+ reg_val |= wol_conf->sopass[5];
+ phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, reg_val);
+ reg_val = wol_conf->sopass[2] << 8;
+ reg_val |= wol_conf->sopass[3];
+ phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, reg_val);
+ reg_val = wol_conf->sopass[0] << 8;
+ reg_val |= wol_conf->sopass[1];
+ phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, reg_val);
+ } else {
+ phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, 0);
+ phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, 0);
+ phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, 0);
+ }
+
+ rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+ if (rc != 0)
+ goto out_unlock;
+
+ if (wol->wolopts & WAKE_MAGIC) {
+ /* Enable the WOL interrupt */
+ reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
+ reg_val |= MII_VSC85XX_INT_MASK_WOL;
+ rc = phy_write(phydev, MII_VSC85XX_INT_MASK, reg_val);
+ if (rc != 0)
+ goto out_unlock;
+ } else {
+ /* Disable the WOL interrupt */
+ reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
+ reg_val &= (~MII_VSC85XX_INT_MASK_WOL);
+ rc = phy_write(phyd
