RE: [PATCH V3 2/8] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver

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

 



Joe Perches <joe@xxxxxxxxxxx> :
> On Mon, 2014-03-17 at 13:43 -0700, Byungho An wrote:
> > From: Siva Reddy <siva.kallam@xxxxxxxxxxx>
> >
> > This patch adds support for Samsung 10Gb ethernet driver(sxgbe).
> 
> Sorry about the brevity, but this driver is very long and I don't like to
read that
> much at a time, so this is very superficial and I didn't read the whole
thing.
> 
> Trivial notes:
> 
> > diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
> > b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
> 
> []
> 
> > +static void sxgbe_clk_csr_set(struct sxgbe_priv_data *priv) {
> > +	u32 clk_rate = clk_get_rate(priv->sxgbe_clk);
> > +
> > +	/* assign the proper divider, this will be used during
> > +	 * mdio communication
> > +	 */
> > +	if (clk_rate < SXGBE_CSR_F_150M)
> > +		priv->clk_csr = SXGBE_CSR_100_150M;
> > +	else if ((clk_rate >= SXGBE_CSR_F_150M) &&
> > +		 (clk_rate <= SXGBE_CSR_F_250M))
> > +		priv->clk_csr = SXGBE_CSR_150_250M;
> > +	else if ((clk_rate >= SXGBE_CSR_F_250M) &&
> > +		 (clk_rate <= SXGBE_CSR_F_300M))
> > +		priv->clk_csr = SXGBE_CSR_250_300M;
> > +	else if ((clk_rate >= SXGBE_CSR_F_300M) &&
> > +		 (clk_rate <= SXGBE_CSR_F_350M))
> > +		priv->clk_csr = SXGBE_CSR_300_350M;
> > +	else if ((clk_rate >= SXGBE_CSR_F_350M) &&
> > +		 (clk_rate <= SXGBE_CSR_F_400M))
> > +		priv->clk_csr = SXGBE_CSR_350_400M;
> > +	else if ((clk_rate >= SXGBE_CSR_F_400M) &&
> > +		 (clk_rate <= SXGBE_CSR_F_500M))
> > +		priv->clk_csr = SXGBE_CSR_400_500M; }
> 
> This block is unnecessarily hard to read and would be better without the
> superfluous tests as:
> 
> 	if (clk_rate < SXGBE_CSR_F_150M)
> 		priv->clk_csr = SXGBE_CSR_100_150M;
> 	else if (clk_rate <= SXGBE_CSR_F_250M)
> 		priv->clk_csr = SXGBE_CSR_150_250M;
> 	else if (clk_rate <= SXGBE_CSR_F_300M)
> 		priv->clk_csr = SXGBE_CSR_250_300M;
> 	else if (clk_rate <= SXGBE_CSR_F_350M)
> 		priv->clk_csr = SXGBE_CSR_300_350M;
> 	else if (clk_rate <= SXGBE_CSR_F_400M)
> 		priv->clk_csr = SXGBE_CSR_350_400M;
> 	else if (clk_rate <= SXGBE_CSR_F_500M)
> 		priv->clk_csr = SXGBE_CSR_400_500M;
> 
OK. 

> I don't understand why the first test is < and the subsequent tests are <=
> though.
Because priv->clk should be set SXGBE_CSR_150_250M when clk_rate is 150M.

> 
> > +static int init_tx_ring(struct device *dev, u8 queue_no,
> > +			struct sxgbe_tx_queue *tx_ring,	int tx_rsize)
> > +{
> []
> > +	/* allocate memory for TX descriptors */
> > +	tx_ring->dma_tx = dma_zalloc_coherent(dev,
> > +					      tx_rsize * sizeof(struct
> sxgbe_tx_norm_desc),
> > +					      &tx_ring->dma_tx_phy,
> GFP_KERNEL);
> > +	if (!tx_ring->dma_tx) {
> > +		dev_err(dev, "No memory for TX desc of SXGBE\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	/* allocate memory for TX skbuff array */
> > +	tx_ring->tx_skbuff_dma = devm_kcalloc(dev, tx_rsize,
> > +					      sizeof(dma_addr_t), GFP_KERNEL);
> > +	if (!tx_ring->tx_skbuff_dma) {
> > +		dev_err(dev, "No memory for TX skbuffs DMA of SXGBE\n");
> > +		goto dmamem_err;
> > +	}
> > +
> > +	tx_ring->tx_skbuff = devm_kcalloc(dev, tx_rsize,
> > +					  sizeof(struct sk_buff *),
GFP_KERNEL);
> > +
> > +	if (!tx_ring->tx_skbuff) {
> > +		dev_err(dev, "No memory for TX skbuffs of SXGBE\n");
> > +		goto dmamem_err;
> > +	}
> 
> All the OOM messages like these are unnecessary as there is an existing
> generic OOM on allocation failures and a dump_stack()
OK. These will be removed.

> 
> []
> 
> > +static netdev_tx_t sxgbe_xmit(struct sk_buff *skb, struct net_device
> > +*dev) {
> []
> > +	/* display current ring */
> > +	if (netif_msg_pktdata(priv)) {
> > +		netdev_dbg(dev, "%s: curr %d dirty=%d entry=%d, first=%p,
> nfrags=%d\n",
> > +			   __func__, (tqueue->cur_tx % tx_rsize),
> > +			   (tqueue->dirty_tx % tx_rsize), entry,
> > +			   first_desc, nr_frags);
> 
> These 2 lines can be combined (and unnecessary parentheses removed)
> into:
> 
> 	netif_dbg(priv, pktdata, dev, "%s: curr %d dirty=%d entry=%d,
first=%p,
> nfrags=%d\n",
> 		  __func__, tqueue->cur_tx % tx_rsize,
> 		  tqueue->dirty_tx % tx_rsize, entry,
> 		  first_desc, nr_frags);
> 
OK. Thanks.
> []
> 
> > +/*  sxgbe_get_stats64 - entry point to see statistical information of
> > +device
> > + *  @dev : device pointer.
> > + *  @stats : pointer to hold all the statistical information of device.
> > + *  Description:
> > + *  This function is a driver entry point whenever ifconfig command
> > +gets
> > + *  executed to see device statistics. Statistics are number of
> > + *  bytes sent or received, errors occured etc.
> > + *  Return value:
> > + *  This function returns various statistical information of device.
> > + */
> > +static struct rtnl_link_stats64 *sxgbe_get_stats64(struct net_device
*dev,
> > +						   struct rtnl_link_stats64
*stats)
> {
> > +	struct sxgbe_priv_data *priv = netdev_priv(dev);
> > +	void __iomem *ioaddr = priv->ioaddr;
> > +	u64 count;
> > +
> > +	spin_lock(&priv->stats_lock);
> > +	/* Freeze the counter registers before reading value otherwise it may
> > +	 * get updated by hardware while we are reading them
> > +	 */
> > +	writel(SXGBE_MMC_CTRL_CNT_FRZ, ioaddr + SXGBE_MMC_CTL_REG);
> > +
> > +	stats->rx_bytes = readl(ioaddr +
> SXGBE_MMC_RXOCTETLO_GCNT_REG);
> > +	stats->rx_bytes |= (u64)(readl(ioaddr +
> SXGBE_MMC_RXOCTETHI_GCNT_REG))
> > +		<< 32;
> 
> Perhaps it'd be better to use a macro or a function to return a u64 like:
> 
> static inline u64 sxgbe_get_stat64(void __iomem *ioaddr, int reg_lo, int
reg_hi)
> {
> 	u64 val = readl(ioaddr + reg_lo);
> 	val |= ((u64)readl(ioaddr + reg_hi)) << 32;
> 	return val;
> }
> 
> > +	stats->rx_packets = readl(ioaddr +
> SXGBE_MMC_RXFRAMELO_GBCNT_REG);
> > +	stats->rx_packets |=
> > +		(u64)(readl(ioaddr + SXGBE_MMC_RXFRAMEHI_GBCNT_REG))
> << 32;
> 
> So this would be:
> 
> 	stats->rx_packets = sxgbe_get_stat64(ioaddr,
> SXGBE_MMC_RXFRAMELO_GBCNT_REG,
> SXGBE_MMC_RXFRAMEHI_GBCNT_REG);
Good suggestion.

> 
> It's a pity all of these registers don't have the same form as there could
then be
> a macro to simplify that long line and maybe remove the
> SXGBE_MMC_<foo>_REG uses.
> 
> > +	stats->multicast = readl(ioaddr +
> SXGBE_MMC_RXMULTILO_GCNT_REG);
> > +	stats->multicast |= (u64)(readl(ioaddr +
> SXGBE_MMC_RXMULTIHI_GCNT_REG))
> > +		<< 32;
> > +	stats->rx_crc_errors = readl(ioaddr + SXGBE_MMC_RXCRCERRLO_REG);
> > +	stats->rx_crc_errors |= (u64)(readl(ioaddr +
> SXGBE_MMC_RXCRCERRHI_REG))
> > +		<< 32;
> > +	stats->rx_length_errors = readl(ioaddr +
> SXGBE_MMC_RXLENERRLO_REG);
> > +	stats->rx_length_errors |=
> > +		(u64)(readl(ioaddr + SXGBE_MMC_RXLENERRHI_REG))	<< 32;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in the
body of
> a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at
> http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux