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
