On 10/09/2017 11:59 AM, Dan Murphy wrote:
> Add support for the TI  DP83822 10/100Mbit ethernet phy.
> 
> The DP83822 provides flexibility to connect to a MAC through a
> standard MII, RMII or RGMII interface.
> 
> In addition the DP83822 needs to be removed from the DP83848 driver
> as the WoL support is added here for this device.
> 
> Datasheet:
> http://www.ti.com/product/DP83822I/datasheet
> 
> Signed-off-by: Dan Murphy <dmur...@ti.com>> ---
> 
> v4 - Squash DP83822 removal patch into this patch -
> https://www.mail-archive.com/netdev@vger.kernel.org/msg192424.html
> 
> v3 - Fixed WoL indication bit and removed mutex for suspend/resume - 
> https://www.mail-archive.com/netdev@vger.kernel.org/msg191891.html and
> https://www.mail-archive.com/netdev@vger.kernel.org/msg191665.html
> 
> v2 - Updated per comments.  Removed unnessary parantheis, called 
> genphy_suspend/
> resume routines and then performing WoL changes, reworked sopass storage and 
> reduced
> the number of phy reads, and moved WOL_SECURE_ON - 
> https://www.mail-archive.com/netdev@vger.kernel.org/msg191392.html
> 
>  drivers/net/phy/Kconfig   |   5 +
>  drivers/net/phy/Makefile  |   1 +
>  drivers/net/phy/dp83822.c | 302 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/phy/dp83848.c |   3 -
>  4 files changed, 308 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/net/phy/dp83822.c
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index cd931cf9dcc2..8e78a482e09e 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -277,6 +277,11 @@ config DAVICOM_PHY
>       ---help---
>         Currently supports dm9161e and dm9131
>  
> +config DP83822_PHY
> +     tristate "Texas Instruments DP83822 PHY"
> +     ---help---
> +       Supports the DP83822 PHY.
> +
>  config DP83848_PHY
>       tristate "Texas Instruments DP83848 PHY"
>       ---help---
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 416df92fbf4f..df3b82ba8550 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_CICADA_PHY)    += cicada.o
>  obj-$(CONFIG_CORTINA_PHY)    += cortina.o
>  obj-$(CONFIG_DAVICOM_PHY)    += davicom.o
>  obj-$(CONFIG_DP83640_PHY)    += dp83640.o
> +obj-$(CONFIG_DP83822_PHY)    += dp83822.o
>  obj-$(CONFIG_DP83848_PHY)    += dp83848.o
>  obj-$(CONFIG_DP83867_PHY)    += dp83867.o
>  obj-$(CONFIG_FIXED_PHY)              += fixed_phy.o
> diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
> new file mode 100644
> index 000000000000..de196dbc46cd
> --- /dev/null
> +++ b/drivers/net/phy/dp83822.c
> @@ -0,0 +1,302 @@
> +/*
> + * Driver for the Texas Instruments DP83822 PHY
> + *
> + * Copyright (C) 2017 Texas Instruments Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/ethtool.h>
> +#include <linux/etherdevice.h>
> +#include <linux/kernel.h>
> +#include <linux/mii.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/phy.h>
> +#include <linux/netdevice.h>
> +
> +#define DP83822_PHY_ID               0x2000a240
> +#define DP83822_DEVADDR              0x1f
> +
> +#define MII_DP83822_MISR1    0x12
> +#define MII_DP83822_MISR2    0x13
> +#define MII_DP83822_RESET_CTRL       0x1f
> +
> +#define DP83822_HW_RESET     BIT(15)
> +#define DP83822_SW_RESET     BIT(14)
> +
> +/* MISR1 bits */
> +#define DP83822_RX_ERR_HF_INT_EN     BIT(0)
> +#define DP83822_FALSE_CARRIER_HF_INT_EN      BIT(1)
> +#define DP83822_ANEG_COMPLETE_INT_EN BIT(2)
> +#define DP83822_DUP_MODE_CHANGE_INT_EN       BIT(3)
> +#define DP83822_SPEED_CHANGED_INT_EN BIT(4)
> +#define DP83822_LINK_STAT_INT_EN     BIT(5)
> +#define DP83822_ENERGY_DET_INT_EN    BIT(6)
> +#define DP83822_LINK_QUAL_INT_EN     BIT(7)
> +
> +/* MISR2 bits */
> +#define DP83822_JABBER_DET_INT_EN    BIT(0)
> +#define DP83822_WOL_PKT_INT_EN               BIT(1)
> +#define DP83822_SLEEP_MODE_INT_EN    BIT(2)
> +#define DP83822_MDI_XOVER_INT_EN     BIT(3)
> +#define DP83822_LB_FIFO_INT_EN               BIT(4)
> +#define DP83822_PAGE_RX_INT_EN               BIT(5)
> +#define DP83822_ANEG_ERR_INT_EN              BIT(6)
> +#define DP83822_EEE_ERROR_CHANGE_INT_EN      BIT(7)
> +
> +/* INT_STAT1 bits */
> +#define DP83822_WOL_INT_EN   BIT(4)
> +#define DP83822_WOL_INT_STAT BIT(12)
> +
> +#define MII_DP83822_RXSOP1   0x04a5
> +#define      MII_DP83822_RXSOP2      0x04a6
> +#define      MII_DP83822_RXSOP3      0x04a7
> +
> +/* WoL Registers */
> +#define      MII_DP83822_WOL_CFG     0x04a0
> +#define      MII_DP83822_WOL_STAT    0x04a1
> +#define      MII_DP83822_WOL_DA1     0x04a2
> +#define      MII_DP83822_WOL_DA2     0x04a3
> +#define      MII_DP83822_WOL_DA3     0x04a4
> +
> +/* WoL bits */
> +#define DP83822_WOL_MAGIC_EN BIT(1)

Datasheet seems to indicate MAGIC_EN is bit 0, not 1.

> +#define DP83822_WOL_SECURE_ON        BIT(5)
> +#define DP83822_WOL_EN               BIT(7)
> +#define DP83822_WOL_INDICATION_SEL BIT(8)
> +#define DP83822_WOL_CLR_INDICATION BIT(11)
> +
> +static int dp83822_ack_interrupt(struct phy_device *phydev)
> +{
> +     int err = phy_read(phydev, MII_DP83822_MISR1);
> +
> +     if (err < 0)
> +             return err;
> +

The above could also be written:

int err;

err = phy_read(phydev, MII_DP83822_MISR1);
if (err < 0)
        return err;

This matches the below better and is more clear to me.

> +     err = phy_read(phydev, MII_DP83822_MISR2);
> +     if (err < 0)
> +             return err;
> +
> +     return 0;
> +}
> +
> +static int dp83822_set_wol(struct phy_device *phydev,
> +                        struct ethtool_wolinfo *wol)
> +{
> +     struct net_device *ndev = phydev->attached_dev;
> +     u16 value;
> +     const u8 *mac;
> +
> +     if (wol->wolopts & (WAKE_MAGIC | WAKE_MAGICSECURE)) {
> +             mac = (const u8 *)ndev->dev_addr;
> +
> +             if (!is_valid_ether_addr(mac))
> +                     return -EINVAL;
> +
> +             /* MAC addresses start with byte 5, but stored in mac[0].
> +              * 822 PHYs store bytes 4|5, 2|3, 0|1
> +              */
> +             phy_write_mmd(phydev, DP83822_DEVADDR,
> +                           MII_DP83822_WOL_DA1, (mac[1] << 8) | mac[0]);
> +             phy_write_mmd(phydev, DP83822_DEVADDR,
> +                           MII_DP83822_WOL_DA2, (mac[3] << 8) | mac[2]);
> +             phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_DA3,
> +                           (mac[5] << 8) | mac[4]);

This 'phy_write_mmd' doesn't match the others, 'MII_DP83822_WOL_DAx'
should be on the next line, or all others should be on same.

> +
> +             value = phy_read_mmd(phydev, DP83822_DEVADDR,
> +                                  MII_DP83822_WOL_CFG);
> +             if (wol->wolopts & WAKE_MAGIC)
> +                     value |= DP83822_WOL_MAGIC_EN;
> +             else
> +                     value &= ~DP83822_WOL_MAGIC_EN;
> +
> +             if (wol->wolopts & WAKE_MAGICSECURE) {
> +                     phy_write_mmd(phydev, DP83822_DEVADDR,
> +                                   MII_DP83822_RXSOP1,
> +                                   (wol->sopass[1] << 8) | wol->sopass[0]);
> +                     phy_write_mmd(phydev, DP83822_DEVADDR,
> +                                   MII_DP83822_RXSOP2,
> +                                   (wol->sopass[3] << 8) | wol->sopass[2]);
> +                     phy_write_mmd(phydev, DP83822_DEVADDR,
> +                                   MII_DP83822_RXSOP3,
> +                                   (wol->sopass[5] << 8) | wol->sopass[4]);
> +                     value |= DP83822_WOL_SECURE_ON;
> +             } else {
> +                     value &= ~DP83822_WOL_SECURE_ON;
> +             }
> +
> +             value |= (DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL |
> +                       DP83822_WOL_CLR_INDICATION);
> +             phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,
> +                           value);
> +     } else {
> +             value = phy_read_mmd(phydev, DP83822_DEVADDR,
> +                                  MII_DP83822_WOL_CFG);
> +             value &= ~DP83822_WOL_EN;
> +             phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,
> +                           value);
> +     }
> +
> +     return 0;
> +}
> +
> +static void dp83822_get_wol(struct phy_device *phydev,
> +                         struct ethtool_wolinfo *wol)
> +{
> +     int value;
> +     u16 sopass_val;
> +
> +     wol->supported = (WAKE_MAGIC | WAKE_MAGICSECURE);
> +     wol->wolopts = 0;
> +
> +     value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
> +     if (value & DP83822_WOL_MAGIC_EN)
> +             wol->wolopts |= WAKE_MAGIC;
> +
> +     if (~value & DP83822_WOL_CLR_INDICATION)
> +             wol->wolopts = 0;

I'm not sure I understand the logic here, why do we clear all other
wolopts if this is not set?

> +
> +     if (value & DP83822_WOL_SECURE_ON) {
> +             wol->wolopts |= WAKE_MAGICSECURE;
> +     } else {
> +             wol->wolopts &= ~WAKE_MAGICSECURE;

wol->wolopts is set to 0 at the start, and nothing else sets it, why
clear it here?

> +             return;
> +     }
> +
> +     sopass_val = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP1);
> +     wol->sopass[0] = (sopass_val & 0xff);
> +     wol->sopass[1] = (sopass_val >> 8);
> +
> +     sopass_val = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP2);
> +     wol->sopass[2] = (sopass_val & 0xff);
> +     wol->sopass[3] = (sopass_val >> 8);
> +
> +     sopass_val = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP3);
> +     wol->sopass[4] = (sopass_val & 0xff);
> +     wol->sopass[5] = (sopass_val >> 8);

Why not encase the above password lines in the 'if (value &
DP83822_WOL_SECURE_ON)' block above, then you can drop the whole else block.

> +}
> +
> +static int dp83822_config_intr(struct phy_device *phydev)
> +{
> +     int misr_status;
> +     int err;
> +
> +     if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +             misr_status = phy_read(phydev, MII_DP83822_MISR1);
> +             if (misr_status < 0)
> +                     return misr_status;
> +
> +             misr_status |= (DP83822_RX_ERR_HF_INT_EN |
> +                             DP83822_FALSE_CARRIER_HF_INT_EN |
> +                             DP83822_ANEG_COMPLETE_INT_EN |
> +                             DP83822_DUP_MODE_CHANGE_INT_EN |
> +                             DP83822_SPEED_CHANGED_INT_EN |
> +                             DP83822_LINK_STAT_INT_EN |
> +                             DP83822_ENERGY_DET_INT_EN |
> +                             DP83822_LINK_QUAL_INT_EN);
> +
> +             err = phy_write(phydev, MII_DP83822_MISR1, misr_status);
> +             if (err < 0)
> +                     return err;
> +
> +             misr_status = phy_read(phydev, MII_DP83822_MISR2);
> +             if (misr_status < 0)
> +                     return misr_status;
> +
> +             misr_status |= (DP83822_JABBER_DET_INT_EN |
> +                             DP83822_WOL_PKT_INT_EN |
> +                             DP83822_SLEEP_MODE_INT_EN |
> +                             DP83822_MDI_XOVER_INT_EN |
> +                             DP83822_LB_FIFO_INT_EN |
> +                             DP83822_PAGE_RX_INT_EN |
> +                             DP83822_ANEG_ERR_INT_EN |
> +                             DP83822_EEE_ERROR_CHANGE_INT_EN);
> +
> +             err = phy_write(phydev, MII_DP83822_MISR2, misr_status);
> +     } else {
> +             err = phy_write(phydev, MII_DP83822_MISR1, 0);

You should only clear the ones you set, I know it is all of them plus
the other registers are read-only, but for clarity you could have a
define with the mask you are using for each register and then ~MASK when
clearing, like the dp83848.c driver.

> +             if (err < 0)
> +                     return err;
> +
> +             err = phy_write(phydev, MII_DP83822_MISR1, 0);
> +     }
> +
> +     return err;
> +}
> +
> +static int dp83822_phy_reset(struct phy_device *phydev)
> +{
> +     int err;
> +
> +     err = phy_write(phydev, MII_DP83822_RESET_CTRL, DP83822_HW_RESET);
> +     if (err < 0)
> +             return err;
> +
> +     return 0;
> +}
> +
> +static int dp83822_suspend(struct phy_device *phydev)
> +{
> +     int value;
> +
> +     value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
> +
> +     if (!(value & DP83822_WOL_EN))
> +             genphy_suspend(phydev);
> +
> +     return 0;
> +}
> +
> +static int dp83822_resume(struct phy_device *phydev)
> +{
> +     int value;
> +
> +     genphy_resume(phydev);
> +
> +     value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
> +
> +     phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, value |
> +                   DP83822_WOL_CLR_INDICATION);
> +
> +

Extra newline.

> +     return 0;
> +}
> +
> +static struct phy_driver dp83822_driver[] = {
> +     {
> +      .phy_id = DP83822_PHY_ID,
> +      .phy_id_mask = 0xfffffff0,
> +      .name = "TI DP83822",
> +      .features = PHY_BASIC_FEATURES,
> +      .flags = PHY_HAS_INTERRUPT,
> +      .config_init = genphy_config_init,
> +      .soft_reset = dp83822_phy_reset,
> +      .get_wol = dp83822_get_wol,
> +      .set_wol = dp83822_set_wol,
> +      .ack_interrupt = dp83822_ack_interrupt,
> +      .config_intr = dp83822_config_intr,
> +      .config_aneg = genphy_config_aneg,
> +      .read_status = genphy_read_status,
> +      .suspend = dp83822_suspend,
> +      .resume = dp83822_resume,
> +      },

Something is not right about the indenting here, tab then space?

> +};
> +module_phy_driver(dp83822_driver);
> +
> +static struct mdio_device_id __maybe_unused dp83822_tbl[] = {
> +     { DP83822_PHY_ID, 0xfffffff0 },
> +     { },
> +};
> +MODULE_DEVICE_TABLE(mdio, dp83822_tbl);
> +
> +MODULE_DESCRIPTION("Texas Instruments DP83822 PHY driver");
> +MODULE_AUTHOR("Dan Murphy <dmur...@ti.com");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
> index 3de4fe4dda77..3966d43c5146 100644
> --- a/drivers/net/phy/dp83848.c
> +++ b/drivers/net/phy/dp83848.c
> @@ -20,7 +20,6 @@
>  #define TI_DP83620_PHY_ID            0x20005ce0
>  #define NS_DP83848C_PHY_ID           0x20005c90
>  #define TLK10X_PHY_ID                        0x2000a210
> -#define TI_DP83822_PHY_ID            0x2000a240
>  
>  /* Registers */
>  #define DP83848_MICR                 0x11 /* MII Interrupt Control Register 
> */
> @@ -80,7 +79,6 @@ static struct mdio_device_id __maybe_unused dp83848_tbl[] = 
> {
>       { NS_DP83848C_PHY_ID, 0xfffffff0 },
>       { TI_DP83620_PHY_ID, 0xfffffff0 },
>       { TLK10X_PHY_ID, 0xfffffff0 },
> -     { TI_DP83822_PHY_ID, 0xfffffff0 },
>       { }
>  };
>  MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
> @@ -110,7 +108,6 @@ static struct phy_driver dp83848_driver[] = {
>       DP83848_PHY_DRIVER(NS_DP83848C_PHY_ID, "NS DP83848C 10/100 Mbps PHY"),
>       DP83848_PHY_DRIVER(TI_DP83620_PHY_ID, "TI DP83620 10/100 Mbps PHY"),
>       DP83848_PHY_DRIVER(TLK10X_PHY_ID, "TI TLK10X 10/100 Mbps PHY"),
> -     DP83848_PHY_DRIVER(TI_DP83822_PHY_ID, "TI DP83822 10/100 Mbps PHY"),
>  };
>  module_phy_driver(dp83848_driver);
>  
> 

Reply via email to