Re: [PATCH v3 4/4] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

Hi Andrew,

> > +static void read_atable(struct switch_enet_private *fep, int index,
> > +			unsigned long *read_lo, unsigned long
> > *read_hi) +{
> > +	unsigned long atable_base = (unsigned long)fep->hwentry;
> > +
> > +	*read_lo = readl((const void *)atable_base + (index << 3));
> > +	*read_hi = readl((const void *)atable_base + (index << 3)
> > + 4); +}
> > +
> > +static void write_atable(struct switch_enet_private *fep, int
> > index,
> > +			 unsigned long write_lo, unsigned long
> > write_hi) +{
> > +	unsigned long atable_base = (unsigned long)fep->hwentry;
> > +
> > +	writel(write_lo, (void *)atable_base + (index << 3));
> > +	writel(write_hi, (void *)atable_base + (index << 3) + 4);
> > +}  
> 
> It would be nice to have the mtip_ prefix on all functions.

Ok.

> 
> > +static int mtip_open(struct net_device *dev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +	int ret, port_idx = priv->portnum - 1;
> > +
> > +	if (fep->usage_count == 0) {
> > +		clk_enable(fep->clk_ipg);
> > +		netif_napi_add(dev, &fep->napi, mtip_rx_napi);
> > +
> > +		ret = mtip_alloc_buffers(dev);
> > +		if (ret)
> > +			return ret;  
> 
> nitpick: You might want to turn the clock off before returning the
> error.

Ok.

> 
> > +	}
> > +
> > +	fep->link[port_idx] = 0;
> > +
> > +	/* Probe and connect to PHY when open the interface, if
> > already
> > +	 * NOT done in the switch driver probe (or when the device
> > is
> > +	 * re-opened).
> > +	 */
> > +	ret = mtip_mii_probe(dev);
> > +	if (ret) {
> > +		mtip_free_buffers(dev);  
> 
> I've not checked. Does this do the opposite of netif_napi_add()?

No, the netif_napi_add() is required here as well.

> 
> > +static void mtip_set_multicast_list(struct net_device *dev)
> > +{
> > +	unsigned int i, bit, data, crc;
> > +
> > +	if (dev->flags & IFF_PROMISC) {
> > +		dev_info(&dev->dev, "%s: IFF_PROMISC\n",
> > __func__);  
> 
> You can save one level of indentation with a return here.

Ok.

> 
> > +	} else {
> > +		if (dev->flags & IFF_ALLMULTI) {
> > +			dev_info(&dev->dev, "%s: IFF_ALLMULTI\n",
> > __func__);  
> 
> and other level here.

Ok.

> 
> > +		} else {
> > +			struct netdev_hw_addr *ha;
> > +			u_char *addrs;
> > +
> > +			netdev_for_each_mc_addr(ha, dev) {
> > +				addrs = ha->addr;
> > +				/* Only support group multicast
> > for now */
> > +				if (!(*addrs & 1))
> > +					continue;  
> 
> You could pull there CRC caluclation out into a helper. You might also
> want to search the tree and see if it exists somewhere else.
> 

The ether_crc_le(ndev->addr_len,·ha->addr); could be the replacement.

However, when I look on the code and compare it with fec_main.c's
set_multicast_list() - it looks like a dead code.

The calculated hash is not used at all (in fec_main.c it is written to
some registers).

I've refactored the code to do similar things, but taking into account
already set switch setup (promisc must be enabled from the outset).

> > +
> > +				/* calculate crc32 value of mac
> > address */
> > +				crc = 0xffffffff;
> > +
> > +				for (i = 0; i < 6; i++) {  
> 
> Is 6 the lengh of a MAC address? There is a #define for that.

This is not needed and can be replaced with already present function.

> 
> > +					data = addrs[i];
> > +					for (bit = 0; bit < 8;
> > +					     bit++, data >>= 1) {
> > +						crc = (crc >> 1) ^
> > +						(((crc ^ data) &
> > 1) ?
> > +						CRC32_POLY : 0);
> > +					}
> > +				}
> > +			}
> > +		}
> > +	}
> > +}
> > +  
> 
> > +struct switch_enet_private *mtip_netdev_get_priv(const struct
> > net_device *ndev) +{
> > +	if (ndev->netdev_ops == &mtip_netdev_ops)
> > +		return netdev_priv(ndev);
> > +
> > +	return NULL;
> > +}
> > +  
> 
> > +static int __init mtip_switch_dma_init(struct switch_enet_private
> > *fep) +{
> > +	struct cbd_t *bdp, *cbd_base;
> > +	int ret, i;
> > +
> > +	/* Check mask of the streaming and coherent API */
> > +	ret = dma_set_mask_and_coherent(&fep->pdev->dev,
> > DMA_BIT_MASK(32));
> > +	if (ret < 0) {
> > +		dev_warn(&fep->pdev->dev, "No suitable DMA
> > available\n");  
> 
> Can you recover from this? Or should it be dev_err()?
> 

It was my mistake - of course there shall be dev_err().

> More later...
> 
> 	Andrew


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@xxxxxxx

Attachment: pgppMGUfQ9Hl4.pgp
Description: OpenPGP digital signature