Re: [PATCH V12 6/7] net: sxgbe: add ethtool related functions support Samsung sxgbe

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Sorry I didn't look over this earlier.

On Sat, 2014-03-22 at 21:04 -0700, Byungho An wrote:
[...]
> --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
> +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
> @@ -42,8 +42,12 @@ struct sxgbe_mtl_ops;
>  #define SXGBE_RX_QUEUES   16
>  
>  /* Max/Min RI Watchdog Timer count value */
> -#define SXGBE_MAX_DMA_RIWT	0xff
> -#define SXGBE_MIN_DMA_RIWT	0x20
> +/* Calculated based how much time does it take to fill 256KB Rx memory
> + * at 10Gb speed at 156MHz clock rate and considered little less then
> + * the actual value.
> + */
> +#define SXGBE_MAX_DMA_RIWT	0x70
> +#define SXGBE_MIN_DMA_RIWT	0x01

This change should be folded into patch 2/7.

[...]
> --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
> +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
[...]
> -static int sxgbe_ethtool_get_eee(struct net_device *dev,
> -				 struct ethtool_eee *edata)
> +static int sxgbe_get_eee(struct net_device *dev,
> +			 struct ethtool_eee *edata)
>  {
>  	struct sxgbe_priv_data *priv = netdev_priv(dev);
>  
> @@ -55,8 +150,8 @@ static int sxgbe_ethtool_get_eee(struct net_device *dev,
>  	return phy_ethtool_get_eee(priv->phydev, edata);
>  }
>  
> -static int sxgbe_ethtool_set_eee(struct net_device *dev,
> -				 struct ethtool_eee *edata)
> +static int sxgbe_set_eee(struct net_device *dev,
> +			 struct ethtool_eee *edata)
>  {
>  	struct sxgbe_priv_data *priv = netdev_priv(dev);
>  

These name changes should be folded into patch 2/7.

[...]
> +static int sxgbe_getsettings(struct net_device *dev,
> +			     struct ethtool_cmd *cmd)
> +{
> +	struct sxgbe_priv_data *priv = netdev_priv(dev);
> +
> +	if (priv->phydev)
> +		return phy_ethtool_gset(priv->phydev, cmd);
> +
> +	return -ENODEV;
> +}
> +
> +static int sxgbe_setsettings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> +	struct sxgbe_priv_data *priv = netdev_priv(dev);
> +
> +	if (priv->phydev)
> +		return phy_ethtool_sset(priv->phydev, cmd);
> +
> +	return -ENODEV;
> +}

I think these two operations should return -EOPNOTSUPP if there is no
PHY.

[...]
> +static int sxgbe_get_ts_info(struct net_device *dev,
> +			     struct ethtool_ts_info *info)
> +{
> +	struct sxgbe_priv_data *priv = netdev_priv(dev);
> +
> +	if (!priv->hw_cap.atime_stamp)
> +		return ethtool_op_get_ts_info(dev, info);
> +
> +	info->so_timestamping = (SOF_TIMESTAMPING_TX_SOFTWARE |
> +				 SOF_TIMESTAMPING_RX_SOFTWARE |
> +				 SOF_TIMESTAMPING_SOFTWARE |
> +				 SOF_TIMESTAMPING_TX_HARDWARE |
> +				 SOF_TIMESTAMPING_RX_HARDWARE |
> +				 SOF_TIMESTAMPING_RAW_HARDWARE);
> +
> +	if (priv->ptp_clock)
> +		info->phc_index = ptp_clock_index(priv->ptp_clock);

priv->ptp_clock doesn't appear to be set anywhere; nor does the driver
implement SIOCSHWTSTAMP.  So this function should not advertise any
hardware timestamping features yet.

> +	info->tx_types = ((1 << HWTSTAMP_TX_OFF) |
> +			  (1 << HWTSTAMP_TX_ON) |
> +			  (1 << HWTSTAMP_TX_ONESTEP_SYNC));
> +
> +	info->rx_filters = ((1 << HWTSTAMP_FILTER_NONE) |
> +			    (1 << HWTSTAMP_FILTER_PTP_V1_L4_EVENT) |
> +			    (1 << HWTSTAMP_FILTER_PTP_V1_L4_SYNC) |
> +			    (1 << HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ) |
> +			    (1 << HWTSTAMP_FILTER_PTP_V2_L4_EVENT) |
> +			    (1 << HWTSTAMP_FILTER_PTP_V2_L4_SYNC) |
> +			    (1 << HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ) |
> +			    (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
> +			    (1 << HWTSTAMP_FILTER_PTP_V2_L2_SYNC) |
> +			    (1 << HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ) |
> +			    (1 << HWTSTAMP_FILTER_PTP_V2_EVENT) |
> +			    (1 << HWTSTAMP_FILTER_PTP_V2_SYNC) |
> +			    (1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ) |
> +			    (1 << HWTSTAMP_FILTER_ALL));
> +	return 0;
> +}
> +
> +int sxgbe_set_flow_ctrl(struct sxgbe_priv_data *priv, int rx, int tx)
> +{
> +	return 0;
> +}
> +
> +static void sxgbe_get_pauseparam(struct net_device *netdev,
> +				 struct ethtool_pauseparam *pause)
> +{
> +	struct sxgbe_priv_data *priv = netdev_priv(netdev);
> +
> +	pause->rx_pause = priv->rx_pause;
> +	pause->tx_pause = priv->tx_pause;
> +}
> +
> +static int sxgbe_set_pauseparam(struct net_device *netdev,
> +				struct ethtool_pauseparam *pause)
> +{
> +	struct sxgbe_priv_data *priv = netdev_priv(netdev);
> +
> +	if (pause->autoneg)
> +		return -EINVAL;
> +
> +	return sxgbe_set_flow_ctrl(priv, pause->rx_pause, pause->tx_pause);

But sxgbe_set_flow_ctrl() is empty, so this isn't a proper
implementation.

> +}
[...]
> +static int sxgbe_get_sset_count(struct net_device *netdev, int sset)
> +{
> +	int len;
> +
> +	switch (sset) {
> +	case ETH_SS_STATS:
> +		len = SXGBE_STATS_LEN;
> +		return len;
> +	default:
> +		return -EOPNOTSUPP;

You implement the operation, just not this particular argument value.
So return -EINVAL.

> +	}
> +}
> +
> +static void sxgbe_get_ethtool_stats(struct net_device *dev,
> +				    struct ethtool_stats *dummy, u64 *data)
> +{
> +	struct sxgbe_priv_data *priv = netdev_priv(dev);
> +	int i, j = 0;
> +	char *p;
> +
> +	if (priv->eee_enabled) {
> +		int val = phy_get_eee_err(priv->phydev);
> +
> +		if (val)
> +			priv->xstats.eee_wakeup_error_n = val;
> +	}
> +
> +	for (i = 0; i < SXGBE_STATS_LEN; i++) {
> +		p = (char *)priv + sxgbe_gstrings_stats[i].stat_offset;
> +		data[j++] = (sxgbe_gstrings_stats[i].sizeof_stat == sizeof(u64))
> +			? (*(u64 *)p) : (*(u32 *)p);
> +	}
> +}

It looks like you will always use the same value of i and j in each
pass, so you don't need two separate variables.

[...]
> +static int sxgbe_get_coalesce(struct net_device *dev,
> +                             struct ethtool_coalesce *ec)
> +{
> +       struct sxgbe_priv_data *priv = netdev_priv(dev);
> +
> +       if (priv->use_riwt)
> +               ec->rx_coalesce_usecs = sxgbe_riwt2usec(priv->rx_riwt, priv);

Also set:

	ec->rx_max_coalesced_frames = !priv->use_riwt;
	ec->tx_coalesce_usecs = SXGBE_COAL_TX_TIMER;
	ec->tx_max_coalesced_frames = SXGBE_TX_FRAMES;

> +       return 0;
> +}
> +
> +static int sxgbe_set_coalesce(struct net_device *dev,
> +			      struct ethtool_coalesce *ec)
> +{
> +	struct sxgbe_priv_data *priv = netdev_priv(dev);
> +	unsigned int rx_riwt;
> +
> +	rx_riwt = sxgbe_usec2riwt(ec->rx_coalesce_usecs, priv);
> +
> +	if ((rx_riwt > SXGBE_MAX_DMA_RIWT) || (rx_riwt < SXGBE_MIN_DMA_RIWT))
> +		return -EINVAL;
> +	else if (!priv->use_riwt)
> +		return -EOPNOTSUPP;

Please check for attempts to change unsupported parameters:

	if (ec->rx_max_coalesced_frames != !rx_riwt ||
	    ec->tx_coalesce_usecs != SXGBE_COAL_TX_TIMER ||
	    ec->tx_max_coalesced_frames != SXGBE_TX_FRAMES ||
	    ec->use_adaptive_rx_coalesce ||
	    ec->use_adaptive_tx_coalesce)
		return -EINVAL;

> +	priv->rx_riwt = rx_riwt;
> +	priv->hw->dma->rx_watchdog(priv->ioaddr, priv->rx_riwt);
> +
> +	return 0;
> +}
[...]
> +static int sxgbe_set_rss_hash_opt(struct sxgbe_priv_data *priv,
> +				  struct ethtool_rxnfc *cmd)
> +{
> +	u32 reg_val = 0;
> +
> +	/* RSS does not support anything other than hashing
> +	 * to queues on src and dst IPs and ports
> +	 */
> +	if (cmd->data & ~(RXH_IP_SRC | RXH_IP_DST |
> +			  RXH_L4_B_0_1 | RXH_L4_B_2_3))
> +		return -EINVAL;
> +
> +	switch (cmd->flow_type) {
> +	case TCP_V4_FLOW:
> +	case TCP_V6_FLOW:
> +		if (!(cmd->data & RXH_IP_SRC) ||
> +		    !(cmd->data & RXH_IP_DST) ||
> +		    !(cmd->data & RXH_L4_B_0_1) ||
> +		    !(cmd->data & RXH_L4_B_2_3))
> +			return -EINVAL;
> +		reg_val = SXGBE_CORE_RSS_CTL_TCP4TE;
> +		break;
> +	case UDP_V4_FLOW:
> +	case UDP_V6_FLOW:
> +		if (!(cmd->data & RXH_IP_SRC) ||
> +		    !(cmd->data & RXH_IP_DST) ||
> +		    !(cmd->data & RXH_L4_B_0_1) ||
> +		    !(cmd->data & RXH_L4_B_2_3))
> +			return -EINVAL;
> +		reg_val = SXGBE_CORE_RSS_CTL_UDP4TE;
> +		break;
> +	case SCTP_V4_FLOW:
> +	case AH_ESP_V4_FLOW:
> +	case AH_V4_FLOW:
> +	case ESP_V4_FLOW:
> +	case AH_ESP_V6_FLOW:
> +	case AH_V6_FLOW:
> +	case ESP_V6_FLOW:
> +	case SCTP_V6_FLOW:
> +	case IPV4_FLOW:
> +	case IPV6_FLOW:
> +		if (!(cmd->data & RXH_IP_SRC) ||
> +		    !(cmd->data & RXH_IP_DST) ||
> +		    (cmd->data & RXH_L4_B_0_1) ||
> +		    (cmd->data & RXH_L4_B_2_3))
> +			return -EINVAL;
> +		reg_val = SXGBE_CORE_RSS_CTL_IP2TE;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Unless I'm missing something, this only accepts the default values, so
it's not actually possible to change the configuration.  Why are you
implementing this operation at all?

[...]
> +static void sxgbe_get_regs(struct net_device *dev,
> +			   struct ethtool_regs *regs, void *space)
> +{
> +	struct sxgbe_priv_data *priv = netdev_priv(dev);
> +	u32 *reg_space = (u32 *)space;
> +	int reg_offset;
> +	int reg_ix = 0;
> +	void __iomem *ioaddr = priv->ioaddr;
> +
> +	memset(reg_space, 0x0, REG_SPACE_SIZE);
> +
> +	/* MAC registers */
> +	for (reg_offset = START_MAC_REG_OFFSET;
> +	     reg_offset <= MAX_MAC_REG_OFFSET; reg_offset += 4) {
> +		reg_space[reg_ix] = readl(ioaddr + reg_offset);
> +		reg_ix++;
> +	}
> +
> +	/* MTL registers */
> +	for (reg_offset = START_MTL_REG_OFFSET;
> +	     reg_offset <= MAX_MTL_REG_OFFSET; reg_offset += 4) {
> +		reg_space[reg_ix] = readl(ioaddr + reg_offset);
> +		reg_ix++;
> +	}
> +
> +	/* DMA registers */
> +	for (reg_offset = START_DMA_REG_OFFSET;
> +	     reg_offset <= MAX_DMA_REG_OFFSET; reg_offset += 4) {
> +		reg_space[reg_ix] = readl(ioaddr + reg_offset);
> +		reg_ix++;
> +	}
[...]

Consider adding an assertion at the end:

	BUG_ON(reg_ix * 4 > REG_SPACE_SIZE);

Ben.

-- 
Ben Hutchings
Everything should be made as simple as possible, but not simpler.
                                                           - Albert Einstein

Attachment: signature.asc
Description: This is a digitally signed message part


[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux