Date:	Tue, 20 Mar 2012 11:00:28 +0100
From:	Florian Fainelli <florian@...nwrt.org>
To:	Andreas Fenkart <afenkart@...il.com>
CC:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH 1/1] ARC VMAC driver.

Hi Andreas,

Le 03/20/12 09:42, Andreas Fenkart a écrit :
> This is a driver for the MAC IP block from ARC International. It
> is based on an existing driver found in ARC Linux distribution,
> but essentially, a full rewrite.
>
> Signed-off-by: Andreas Fenkart<afenkart@...il.com>

I have some phylib-related comments inline.

> ---
>   drivers/net/ethernet/Kconfig   |    9 +
>   drivers/net/ethernet/Makefile  |    1 +
>   drivers/net/ethernet/arcvmac.c | 1472 ++++++++++++++++++++++++++++++++++++++++
>   drivers/net/ethernet/arcvmac.h |  269 ++++++++
>   4 files changed, 1751 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
> index 597f4d4..4b6baf8 100644
> --- a/drivers/net/ethernet/Kconfig
> +++ b/drivers/net/ethernet/Kconfig
> @@ -23,6 +23,15 @@ source "drivers/net/ethernet/aeroflex/Kconfig"
>   source "drivers/net/ethernet/alteon/Kconfig"
>   source "drivers/net/ethernet/amd/Kconfig"
>   source "drivers/net/ethernet/apple/Kconfig"
> +
> +config ARCVMAC
> +	tristate "ARC VMAC ethernet driver"
> +	select MII
> +	select PHYLIB
> +	select CRC32
> +	help
> +	  MAC present Zoran43xx, IP from ARC international
> +
>   source "drivers/net/ethernet/atheros/Kconfig"
>   source "drivers/net/ethernet/cadence/Kconfig"
>   source "drivers/net/ethernet/adi/Kconfig"
> diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
> index be5dde0..cb61799 100644
> --- a/drivers/net/ethernet/Makefile
> +++ b/drivers/net/ethernet/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_GRETH) += aeroflex/
>   obj-$(CONFIG_NET_VENDOR_ALTEON) += alteon/
>   obj-$(CONFIG_NET_VENDOR_AMD) += amd/
>   obj-$(CONFIG_NET_VENDOR_APPLE) += apple/
> +obj-$(CONFIG_ARCVMAC) += arcvmac.o
>   obj-$(CONFIG_NET_VENDOR_ATHEROS) += atheros/
>   obj-$(CONFIG_NET_ATMEL) += cadence/
>   obj-$(CONFIG_NET_BFIN) += adi/
> diff --git a/drivers/net/ethernet/arcvmac.c b/drivers/net/ethernet/arcvmac.c
> new file mode 100644
> index 0000000..d512806
> --- /dev/null
> +++ b/drivers/net/ethernet/arcvmac.c
> @@ -0,0 +1,1472 @@
> +/*
> + * ARC VMAC Driver
> + *
> + * Copyright (C) 2009-2012 Andreas Fenkart
> + * All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + * Initial work taken from arc linux distribution, any bugs are mine
> + *
> + *	-----<snip>-----
> + * Copyright (C) 2003-2006 Codito Technologies, for linux-2.4 port
> + * Copyright (C) 2006-2007 Celunite Inc, for linux-2.6 port
> + * Authors: amit.bhor@...unite.com, sameer.dhavale@...unite.com
> + *	-----<snip>-----
> + */
> +
> +#include<linux/clk.h>
> +#include<linux/crc32.h>
> +#include<linux/delay.h>
> +#include<linux/dma-mapping.h>
> +#include<linux/etherdevice.h>
> +#include<linux/init.h>
> +#include<linux/interrupt.h>
> +#include<linux/io.h>
> +#include<linux/kernel.h>
> +#include<linux/module.h>
> +#include<linux/moduleparam.h>
> +#include<linux/netdevice.h>
> +#include<linux/phy.h>
> +#include<linux/platform_device.h>
> +#include<linux/slab.h>
> +#include<linux/sched.h>
> +#include<linux/types.h>
> +
> +#include "arcvmac.h"
> +
> +/* Register access macros */
> +#define vmac_writel(port, value, reg)	\
> +	writel(cpu_to_le32(value), (port)->regs + VMAC_##reg)
> +#define vmac_readl(port, reg)	le32_to_cpu(readl((port)->regs + VMAC_##reg))
> +
> +static int get_register_map(struct vmac_priv *ap);
> +static int put_register_map(struct vmac_priv *ap);
> +static void update_vmac_stats_unlocked(struct net_device *dev);
> +static int vmac_tx_reclaim_unlocked(struct net_device *dev, bool force);
> +
> +static unsigned char *read_mac_reg(struct net_device *dev,
> +				   unsigned char hwaddr[ETH_ALEN])
> +{
> +	struct vmac_priv *ap = netdev_priv(dev);
> +	u32 mac_lo, mac_hi;
> +
> +	WARN_ON(!hwaddr);
> +	mac_lo = vmac_readl(ap, ADDRL);
> +	mac_hi = vmac_readl(ap, ADDRH);
> +
> +	hwaddr[0] = (mac_lo>>  0)&  0xff;
> +	hwaddr[1] = (mac_lo>>  8)&  0xff;
> +	hwaddr[2] = (mac_lo>>  16)&  0xff;
> +	hwaddr[3] = (mac_lo>>  24)&  0xff;
> +	hwaddr[4] = (mac_hi>>  0)&  0xff;
> +	hwaddr[5] = (mac_hi>>  8)&  0xff;
> +	return hwaddr;
> +}
> +
> +static void write_mac_reg(struct net_device *dev, unsigned char* hwaddr)
> +{
> +	struct vmac_priv *ap = netdev_priv(dev);
> +	u32 mac_lo, mac_hi;
> +
> +	mac_lo = hwaddr[3]<<  24 | hwaddr[2]<<  16 | hwaddr[1]<<  8 |
> +		hwaddr[0];
> +	mac_hi = hwaddr[5]<<  8 | hwaddr[4];
> +
> +	vmac_writel(ap, mac_lo, ADDRL);
> +	vmac_writel(ap, mac_hi, ADDRH);
> +}
> +
> +static void vmac_mdio_xmit(struct vmac_priv *ap, u32 val)
> +{
> +	init_completion(&ap->mdio_complete);
> +	vmac_writel(ap, val, MDIO_DATA);
> +	wait_for_completion(&ap->mdio_complete);

I would rather switch to a variant which allows a timeout, just in case 
the MDIO transaction never completes, so you are not stuck here, and you 
can guarantee the maximum waiting time. You might also want to test if 
you need the _interruptible variant too in case you have a process like 
ethtool calling into your driver's ethtool ops callbacks. Then you also 
need to propagate the error to the callers.

> +}
> +
> +static int vmac_mdio_read(struct mii_bus *bus, int phy_id, int phy_reg)
> +{
> +	struct vmac_priv *vmac = bus->priv;
> +	u32 val;
> +
> +	/* only 5 bits allowed for phy-addr and reg_offset */
> +	WARN_ON(phy_id&  ~0x1f || phy_reg&  ~0x1f);

A WARN_ON() here is a little too hard, just return -EINVAL.

> +
> +	val = MDIO_BASE | MDIO_OP_READ;
> +	val |= phy_id<<  23 | phy_reg<<  18;
> +	vmac_mdio_xmit(vmac, val);
> +
> +	val = vmac_readl(vmac, MDIO_DATA);
> +	return val&  MDIO_DATA_MASK;
> +}
> +
> +static int vmac_mdio_write(struct mii_bus *bus, int phy_id, int phy_reg,
> +			   u16 value)
> +{
> +	struct vmac_priv *vmac = bus->priv;
> +	u32 val;
> +
> +	/* only 5 bits allowed for phy-addr and reg_offset */
> +	WARN_ON(phy_id&  ~0x1f || phy_reg&  ~0x1f);

Same here.

> +
> +	val = MDIO_BASE | MDIO_OP_WRITE;
> +	val |= phy_id<<  23 | phy_reg<<  18;
> +	val |= (value&  MDIO_DATA_MASK);
> +	vmac_mdio_xmit(vmac, val);
> +
> +	return 0;
> +}
> +
> +static void vmac_handle_link_change(struct net_device *dev)
> +{
> +	struct vmac_priv *ap = netdev_priv(dev);
> +	struct phy_device *phydev = ap->phy_dev;
> +	unsigned long flags;
> +	bool report_change = false;
> +
> +	spin_lock_irqsave(&ap->lock, flags);
> +
> +	if (phydev->duplex != ap->duplex) {
> +		u32 tmp;
> +
> +		tmp = vmac_readl(ap, ENABLE);
> +
> +		if (phydev->duplex)
> +			tmp |= ENFL_MASK;
> +		else
> +			tmp&= ~ENFL_MASK;
> +
> +		vmac_writel(ap, tmp, ENABLE);
> +
> +		ap->duplex = phydev->duplex;
> +		report_change = true;
> +	}
> +
> +	if (phydev->speed != ap->speed) {
> +		ap->speed = phydev->speed;
> +		report_change = true;
> +	}
> +
> +	if (phydev->link != ap->link) {
> +		ap->link = phydev->link;
> +		report_change = true;
> +	}
> +
> +	spin_unlock_irqrestore(&ap->lock, flags);
> +
> +	if (report_change)
> +		phy_print_status(ap->phy_dev);
> +}
> +
> +static int __devinit vmac_mii_probe(struct net_device *dev)
> +{
> +	struct vmac_priv *ap = netdev_priv(dev);
> +	struct phy_device *phydev = NULL;
> +	struct clk *vmac_clk;
> +	unsigned long clock_rate;
> +	int phy_addr, err;
> +
> +	/* find the first phy */
> +	for (phy_addr = 0; phy_addr<  PHY_MAX_ADDR; phy_addr++) {
> +		if (ap->mii_bus->phy_map[phy_addr]) {
> +			phydev = ap->mii_bus->phy_map[phy_addr];
> +			break;
> +		}
> +	}

Use phy_find_first() instead of open-coding the iteration.

> +
> +	if (!phydev) {
> +		dev_err(&ap->pdev->dev, "no PHY found\n");
> +		return -ENODEV;
> +	}
> +
> +	/* FIXME: add pin_irq, if avail */
> +
> +	phydev = phy_connect(dev, dev_name(&phydev->dev),
> +			&vmac_handle_link_change, 0,
> +			     PHY_INTERFACE_MODE_MII);
> +
> +	if (IS_ERR(phydev)) {
> +		err = PTR_ERR(phydev);
> +		dev_err(&ap->pdev->dev, "could not attach to PHY %d\n", err);
> +		goto err_out;
> +	}
> +
> +	phydev->supported&= PHY_BASIC_FEATURES;
> +	phydev->supported |= SUPPORTED_Asym_Pause | SUPPORTED_Pause;
> +
> +	vmac_clk = clk_get(&ap->pdev->dev, "arcvmac");
> +	if (IS_ERR(vmac_clk)) {
> +		err = PTR_ERR(vmac_clk);
> +		goto err_disconnect;
> +	}
> +
> +	clock_rate = clk_get_rate(vmac_clk);
> +	clk_put(vmac_clk);
> +
> +	dev_dbg(&ap->pdev->dev, "vmac_clk: %lu Hz\n", clock_rate);
> +
> +	if (clock_rate<  25000000)
> +		phydev->supported&= ~(SUPPORTED_100baseT_Half |
> +				       SUPPORTED_100baseT_Full);
> +
> +	phydev->advertising = phydev->supported;
> +
> +	ap->link = 0;
> +	ap->speed = 0;
> +	ap->duplex = -1;
> +	ap->phy_dev = phydev;
> +
> +	return 0;
> +
> +err_disconnect:
> +	phy_disconnect(phydev);
> +err_out:
> +	return err;
> +}
> +
> +static int __devinit vmac_mii_init(struct vmac_priv *ap)
> +{
> +	unsigned long flags;
> +	int err, i;
> +
> +	spin_lock_irqsave(&ap->lock, flags);
> +
> +	ap->mii_bus = mdiobus_alloc();
> +	if (!ap->mii_bus)
> +		return -ENOMEM;
> +
> +	ap->mii_bus->name = "vmac_mii_bus";
> +	ap->mii_bus->read =&vmac_mdio_read;
> +	ap->mii_bus->write =&vmac_mdio_write;
> +
> +	snprintf(ap->mii_bus->id, MII_BUS_ID_SIZE, "%x", 0);

Please use an unique name such as:

snprintf(ap->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x", ap->pdev->name, 
ap->pdev->id);

> +
> +	ap->mii_bus->priv = ap;
> +
> +	err = -ENOMEM;
> +	ap->mii_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
> +	if (!ap->mii_bus->irq)
> +		goto err_out;

This is a little unusual, rather do this:
if (!ap->mii_bus->irq) {
	err = -ENOMEM;
	goto err_out;
}

> +
> +	for (i = 0; i<  PHY_MAX_ADDR; i++)
> +		ap->mii_bus->irq[i] = PHY_POLL;
> +
> +	spin_unlock_irqrestore(&ap->lock, flags);
> +
> +	/* locking: mdio concurrency */
> +
> +	err = mdiobus_register(ap->mii_bus);
> +	if (err)
> +		goto err_out_free_mdio_irq;
> +
> +	err = vmac_mii_probe(ap->dev);
> +	if (err)
> +		goto err_out_unregister_bus;
> +
> +	return 0;
> +
> +err_out_unregister_bus:
> +	mdiobus_unregister(ap->mii_bus);
> +err_out_free_mdio_irq:
> +	kfree(ap->mii_bus->irq);
> +err_out:
> +	mdiobus_free(ap->mii_bus);
> +	return err;
> +}
> +
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists