Re: [PATCH v1 net-next 6/9] lan743x: Add power management support

2018-07-06 Thread kbuild test robot 
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

2018-07-05 Thread Bryan.Whitehead 
> > +   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

2018-07-05 Thread Andrew Lunn 
> +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