Re: [PATCH v1 net-next 6/9] lan743x: Add power management support
Hi Bryan,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url:
https://github.com/0day-ci/linux/commits/Bryan-Whitehead/lan743x-Add-features-to-lan743x-driver/20180706-051812
config: alpha-allmodconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=alpha
All warnings (new ones prefixed by >>):
>> drivers/net/ethernet/microchip/lan743x_main.c:2787:5: warning: "CONFIG_PM"
>> is not defined, evaluates to 0 [-Wundef]
#if CONFIG_PM
^
drivers/net/ethernet/microchip/lan743x_main.c:2894:5: warning: "CONFIG_PM"
is not defined, evaluates to 0 [-Wundef]
#if CONFIG_PM
^
drivers/net/ethernet/microchip/lan743x_main.c:2926:5: warning: "CONFIG_PM"
is not defined, evaluates to 0 [-Wundef]
#if CONFIG_PM
^
drivers/net/ethernet/microchip/lan743x_main.c:2957:5: warning: "CONFIG_PM"
is not defined, evaluates to 0 [-Wundef]
#if CONFIG_PM
^
--
>> drivers/net/ethernet/microchip/lan743x_ethtool.c:436:5: warning: "CONFIG_PM"
>> is not defined, evaluates to 0 [-Wundef]
#if CONFIG_PM
^
vim +/CONFIG_PM +2787 drivers/net/ethernet/microchip/lan743x_main.c
2786
> 2787 #if CONFIG_PM
2788 static void lan743x_pm_set_wol(struct lan743x_adapter *adapter)
2789 {
2790 const u8 ipv4_multicast[3] = { 0x01, 0x00, 0x5E };
2791 const u8 ipv6_multicast[3] = { 0x33, 0x33 };
2792 const u8 arp_type[2] = { 0x08, 0x06 };
2793 int mask_index;
2794 u32 pmtctl;
2795 u32 wucsr;
2796 u32 macrx;
2797 u16 crc;
2798
2799 for (mask_index = 0; mask_index < MAC_NUM_OF_WUF_CFG;
mask_index++)
2800 lan743x_csr_write(adapter, MAC_WUF_CFG(mask_index), 0);
2801
2802 /* clear wake settings */
2803 pmtctl = lan743x_csr_read(adapter, PMT_CTL);
2804 pmtctl |= PMT_CTL_WUPS_MASK_;
2805 pmtctl &= ~(PMT_CTL_GPIO_WAKEUP_EN_ | PMT_CTL_EEE_WAKEUP_EN_ |
2806 PMT_CTL_WOL_EN_ | PMT_CTL_MAC_D3_RX_CLK_OVR_ |
2807 PMT_CTL_RX_FCT_RFE_D3_CLK_OVR_ |
PMT_CTL_ETH_PHY_WAKE_EN_);
2808
2809 macrx = lan743x_csr_read(adapter, MAC_RX);
2810
2811 wucsr = 0;
2812 mask_index = 0;
2813
2814 pmtctl |= PMT_CTL_ETH_PHY_D3_COLD_OVR_ |
PMT_CTL_ETH_PHY_D3_OVR_;
2815
2816 if (adapter->wolopts & WAKE_PHY) {
2817 pmtctl |= PMT_CTL_ETH_PHY_EDPD_PLL_CTL_;
2818 pmtctl |= PMT_CTL_ETH_PHY_WAKE_EN_;
2819 }
2820 if (adapter->wolopts & WAKE_MAGIC) {
2821 wucsr |= MAC_WUCSR_MPEN_;
2822 macrx |= MAC_RX_RXEN_;
2823 pmtctl |= PMT_CTL_WOL_EN_ | PMT_CTL_MAC_D3_RX_CLK_OVR_;
2824 }
2825 if (adapter->wolopts & WAKE_UCAST) {
2826 wucsr |= MAC_WUCSR_RFE_WAKE_EN_ | MAC_WUCSR_PFDA_EN_;
2827 macrx |= MAC_RX_RXEN_;
2828 pmtctl |= PMT_CTL_WOL_EN_ | PMT_CTL_MAC_D3_RX_CLK_OVR_;
2829 pmtctl |= PMT_CTL_RX_FCT_RFE_D3_CLK_OVR_;
2830 }
2831 if (adapter->wolopts & WAKE_BCAST) {
2832 wucsr |= MAC_WUCSR_RFE_WAKE_EN_ | MAC_WUCSR_BCST_EN_;
2833 macrx |= MAC_RX_RXEN_;
2834 pmtctl |= PMT_CTL_WOL_EN_ | PMT_CTL_MAC_D3_RX_CLK_OVR_;
2835 pmtctl |= PMT_CTL_RX_FCT_RFE_D3_CLK_OVR_;
2836 }
2837 if (adapter->wolopts & WAKE_MCAST) {
2838 /* IPv4 multicast */
2839 crc = lan743x_pm_wakeframe_crc16(ipv4_multicast, 3);
2840 lan743x_csr_write(adapter, MAC_WUF_CFG(mask_index),
2841MAC_WUF_CFG_EN_ |
MAC_WUF_CFG_TYPE_MCAST_ |
2842(0 << MAC_WUF_CFG_OFFSET_SHIFT_) |
2843(crc & MAC_WUF_CFG_CRC16_MASK_));
2844 lan743x_csr_write(adapter, MAC_WUF_MASK0(mask_index),
7);
2845 lan743x_csr_write(adapter, MAC_WUF_MASK1(mask_index),
0);
2846 lan743x_csr_write(adapter, MAC_WUF_MASK2(mask_index),
0);
2847 lan743x_csr_write(adapter, MAC_WUF_MASK3(mask_index),
0);
2848 mask_index++;
2849
2850 /* IPv6 multicast */
2851 crc = lan743x_pm_wakeframe_crc16(ipv6_multicast, 2);
2852 lan743x_csr_write(adapter, MAC_WUF_CFG(mask_index),
2853MAC_WUF_CFG_EN_ |
RE: [PATCH v1 net-next 6/9] lan743x: Add power management support
> > + data = lan743x_csr_read(adapter, PMT_CTL);
>
> Hi Bryan
>
> Why do you do this read? You do not use the result.
>
Good catch, I'll remove it.
> > +
> > + wol->supported = WAKE_BCAST | WAKE_UCAST | WAKE_MCAST |
> > + WAKE_MAGIC | WAKE_PHY | WAKE_ARP;
> > +
> > + wol->wolopts = adapter->wolopts;
> > +}
> > +#endif /* CONFIG_PM */
> > +
> > +static int lan743x_pm_wakeframe_crc16(const u8 *buf, int len) {
> > + const u16 crc16poly = 0x8005;
> > + u16 bit, crc, msb;
> > + u8 data;
> > + int i;
> > +
> > + crc = 0x;
> > + for (i = 0; i < len; i++) {
> > + data = *buf++;
> > + for (bit = 0; bit < 8; bit++) {
> > + msb = crc >> 15;
> > + crc <<= 1;
> > +
> > + if (msb ^ (u16)(data & 1)) {
> > + crc ^= crc16poly;
> > + crc |= (u16)0x0001U;
> > + }
> > + data >>= 1;
> > + }
> > + }
> > +
>
> There are a few different crc algorithms in lib. Can you use one of them,
> rather than implementing it yourself?
OK I'll check.
>
> > +#if CONFIG_PM
> > +static int lan743x_pm_suspend(struct device *dev) {
> > + struct pci_dev *pdev = to_pci_dev(dev);
> > + struct net_device *netdev = pci_get_drvdata(pdev);
> > + struct lan743x_adapter *adapter = netdev_priv(netdev);
> > + u16 phydata;
> > + int ret;
> > +
> > + if (adapter->wolopts & WAKE_PHY) {
> > + phydata = phy_read(netdev->phydev, 27);
> > + phydata |= 0x0500;
> > + phy_write(netdev->phydev, 27, phydata);
> > + }
>
> Shouldn't the PHY driver do this?
Perhaps so. I'll check with the PM writer.
Thanks Andrew
Re: [PATCH v1 net-next 6/9] lan743x: Add power management support
> +static void lan743x_ethtool_get_wol(struct net_device *netdev,
> + struct ethtool_wolinfo *wol)
> +{
> + struct lan743x_adapter *adapter = netdev_priv(netdev);
> + u32 data;
> +
> + data = lan743x_csr_read(adapter, PMT_CTL);
Hi Bryan
Why do you do this read? You do not use the result.
> +
> + wol->supported = WAKE_BCAST | WAKE_UCAST | WAKE_MCAST |
> + WAKE_MAGIC | WAKE_PHY | WAKE_ARP;
> +
> + wol->wolopts = adapter->wolopts;
> +}
> +#endif /* CONFIG_PM */
> +
> +static int lan743x_pm_wakeframe_crc16(const u8 *buf, int len)
> +{
> + const u16 crc16poly = 0x8005;
> + u16 bit, crc, msb;
> + u8 data;
> + int i;
> +
> + crc = 0x;
> + for (i = 0; i < len; i++) {
> + data = *buf++;
> + for (bit = 0; bit < 8; bit++) {
> + msb = crc >> 15;
> + crc <<= 1;
> +
> + if (msb ^ (u16)(data & 1)) {
> + crc ^= crc16poly;
> + crc |= (u16)0x0001U;
> + }
> + data >>= 1;
> + }
> + }
> +
There are a few different crc algorithms in lib. Can you use one of
them, rather than implementing it yourself?
> +#if CONFIG_PM
> +static int lan743x_pm_suspend(struct device *dev)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct net_device *netdev = pci_get_drvdata(pdev);
> + struct lan743x_adapter *adapter = netdev_priv(netdev);
> + u16 phydata;
> + int ret;
> +
> + if (adapter->wolopts & WAKE_PHY) {
> + phydata = phy_read(netdev->phydev, 27);
> + phydata |= 0x0500;
> + phy_write(netdev->phydev, 27, phydata);
> + }
Shouldn't the PHY driver do this?
Andrew
[PATCH v1 net-next 6/9] lan743x: Add power management support
Signed-off-by: Bryan Whitehead
---
drivers/net/ethernet/microchip/lan743x_ethtool.c | 51 ++
drivers/net/ethernet/microchip/lan743x_main.c| 210 +++
drivers/net/ethernet/microchip/lan743x_main.h| 47 +
3 files changed, 308 insertions(+)
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c
b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index 0d0c997..0709a8d 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -417,6 +417,53 @@ static int lan743x_ethtool_get_sset_count(struct
net_device *netdev, int sset)
}
}
+#ifdef CONFIG_PM
+static void lan743x_ethtool_get_wol(struct net_device *netdev,
+ struct ethtool_wolinfo *wol)
+{
+ struct lan743x_adapter *adapter = netdev_priv(netdev);
+ u32 data;
+
+ data = lan743x_csr_read(adapter, PMT_CTL);
+
+ wol->supported = WAKE_BCAST | WAKE_UCAST | WAKE_MCAST |
+ WAKE_MAGIC | WAKE_PHY | WAKE_ARP;
+
+ wol->wolopts = adapter->wolopts;
+}
+#endif /* CONFIG_PM */
+
+#if CONFIG_PM
+static int lan743x_ethtool_set_wol(struct net_device *netdev,
+ struct ethtool_wolinfo *wol)
+{
+ struct lan743x_adapter *adapter = netdev_priv(netdev);
+
+ if (wol->wolopts & WAKE_MAGICSECURE)
+ return -EOPNOTSUPP;
+
+ adapter->wolopts = 0;
+ if (wol->wolopts & WAKE_UCAST)
+ adapter->wolopts |= WAKE_UCAST;
+ if (wol->wolopts & WAKE_MCAST)
+ adapter->wolopts |= WAKE_MCAST;
+ if (wol->wolopts & WAKE_BCAST)
+ adapter->wolopts |= WAKE_BCAST;
+ if (wol->wolopts & WAKE_MAGIC)
+ adapter->wolopts |= WAKE_MAGIC;
+ if (wol->wolopts & WAKE_PHY)
+ adapter->wolopts |= WAKE_PHY;
+ if (wol->wolopts & WAKE_ARP)
+ adapter->wolopts |= WAKE_ARP;
+
+ device_set_wakeup_enable(>pdev->dev, (bool)wol->wolopts);
+
+ phy_ethtool_set_wol(netdev->phydev, wol);
+
+ return 0;
+}
+#endif /* CONFIG_PM */
+
const struct ethtool_ops lan743x_ethtool_ops = {
.get_drvinfo = lan743x_ethtool_get_drvinfo,
.get_msglevel = lan743x_ethtool_get_msglevel,
@@ -431,4 +478,8 @@ const struct ethtool_ops lan743x_ethtool_ops = {
.get_sset_count = lan743x_ethtool_get_sset_count,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
+#ifdef CONFIG_PM
+ .get_wol = lan743x_ethtool_get_wol,
+ .set_wol = lan743x_ethtool_set_wol,
+#endif
};
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c
b/drivers/net/ethernet/microchip/lan743x_main.c
index 1e2f8c6..52ca8b9 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -2749,10 +2749,217 @@ static void lan743x_pcidev_shutdown(struct pci_dev
*pdev)
lan743x_netdev_close(netdev);
rtnl_unlock();
+#ifdef CONFIG_PM
+ pci_save_state(pdev);
+#endif
+
/* clean up lan743x portion */
lan743x_hardware_cleanup(adapter);
}
+#ifdef CONFIG_PM
+static int lan743x_pm_wakeframe_crc16(const u8 *buf, int len)
+{
+ const u16 crc16poly = 0x8005;
+ u16 bit, crc, msb;
+ u8 data;
+ int i;
+
+ crc = 0x;
+ for (i = 0; i < len; i++) {
+ data = *buf++;
+ for (bit = 0; bit < 8; bit++) {
+ msb = crc >> 15;
+ crc <<= 1;
+
+ if (msb ^ (u16)(data & 1)) {
+ crc ^= crc16poly;
+ crc |= (u16)0x0001U;
+ }
+ data >>= 1;
+ }
+ }
+
+ return crc;
+}
+#endif /* CONFIG_PM */
+
+#if CONFIG_PM
+static void lan743x_pm_set_wol(struct lan743x_adapter *adapter)
+{
+ const u8 ipv4_multicast[3] = { 0x01, 0x00, 0x5E };
+ const u8 ipv6_multicast[3] = { 0x33, 0x33 };
+ const u8 arp_type[2] = { 0x08, 0x06 };
+ int mask_index;
+ u32 pmtctl;
+ u32 wucsr;
+ u32 macrx;
+ u16 crc;
+
+ for (mask_index = 0; mask_index < MAC_NUM_OF_WUF_CFG; mask_index++)
+ lan743x_csr_write(adapter, MAC_WUF_CFG(mask_index), 0);
+
+ /* clear wake settings */
+ pmtctl = lan743x_csr_read(adapter, PMT_CTL);
+ pmtctl |= PMT_CTL_WUPS_MASK_;
+ pmtctl &= ~(PMT_CTL_GPIO_WAKEUP_EN_ | PMT_CTL_EEE_WAKEUP_EN_ |
+ PMT_CTL_WOL_EN_ | PMT_CTL_MAC_D3_RX_CLK_OVR_ |
+ PMT_CTL_RX_FCT_RFE_D3_CLK_OVR_ | PMT_CTL_ETH_PHY_WAKE_EN_);
+
+ macrx = lan743x_csr_read(adapter, MAC_RX);
+
+ wucsr = 0;
+ mask_index = 0;
+
+ pmtctl |= PMT_CTL_ETH_PHY_D3_COLD_OVR_ | PMT_CTL_ETH_PHY_D3_OVR_;
+
+ if (adapter->wolopts & WAKE_PHY) {
+ pmtctl |= PMT_CTL_ETH_PHY_EDPD_PLL_CTL_;
+
