Re: PATCH: Add non-Virtex 5 support for LL TEMAC driver

[Grant Likely <grant.likely@secretlab.ca>]

On Mon, Feb 1, 2010 at 4:25 PM, John Tyner <jtyner@cs.ucr.edu> wrote:
> This patch adds support for using the LL TEMAC Ethernet driver on non-Virtex
> 5 platforms by adding support for accessing the Soft DMA registers as if
> they were memory mapped instead of solely through the DCR's (available on
> the Virtex 5).
>
> Signed-off-by: John Tyner <jtyner@cs.ucr.edu>

Hi John, thanks for doing this work.  A couple of comments below.

>
> --- /tmp/tmp.5198.41	2010-02-01 15:04:45.000000000 -0800
> +++ ./linux-2.6.32.3/drivers/net/ll_temac.h	2010-01-28 15:06:17.000000000 -0800
> @@ -338,6 +338,7 @@
>  	/* IO registers and IRQs */
>  	void __iomem *regs;
>  	dcr_host_t sdma_dcrs;
> +	u32 __iomem *sdma_regs;
>  	int tx_irq;
>  	int rx_irq;
>  	int emac_num;
> --- /tmp/tmp.5198.53	2010-02-01 15:04:45.000000000 -0800
> +++ ./linux-2.6.32.3/drivers/net/ll_temac_main.c	2010-02-01 15:04:01.000000000 -0800
> @@ -20,9 +20,6 @@
>   *   or rx, so this should be okay.
>   *
>   * TODO:
> - * - Fix driver to work on more than just Virtex5.  Right now the driver
> - *   assumes that the locallink DMA registers are accessed via DCR
> - *   instructions.
>   * - Factor out locallink DMA code into separate driver
>   * - Fix multicast assignment.
>   * - Fix support for hardware checksumming.
> @@ -117,12 +114,20 @@
>
>  static u32 temac_dma_in32(struct temac_local *lp, int reg)
>  {
> -	return dcr_read(lp->sdma_dcrs, reg);
> +	if (lp->sdma_regs) {
> +		return __raw_readl(lp->sdma_regs + reg);
> +	} else {
> +		return dcr_read(lp->sdma_dcrs, reg);
> +	}

Rather than taking the ugliness an if/else block on every register
access, why not create an ops structure and populate it with the
correct access routines at runtime?

>  }
>
>  static void temac_dma_out32(struct temac_local *lp, int reg, u32 value)
>  {
> -	dcr_write(lp->sdma_dcrs, reg, value);
> +	if (lp->sdma_regs) {
> +		__raw_writel(value, lp->sdma_regs + reg);
> +	} else {
> +		dcr_write(lp->sdma_dcrs, reg, value);
> +	}
>  }
>
>  /**
> @@ -862,13 +867,17 @@
>  		goto nodev;
>  	}
>
> -	dcrs = dcr_resource_start(np, 0);
> -	if (dcrs == 0) {
> -		dev_err(&op->dev, "could not get DMA register address\n");
> +	lp->sdma_regs = NULL;
> +
> +	if ((dcrs = dcr_resource_start(np, 0)) != 0) {
> +		lp->sdma_dcrs = dcr_map(np, dcrs, dcr_resource_len(np, 0));
> +		dev_dbg(&op->dev, "DCR base: %x\n", dcrs);
> +	} else if ((lp->sdma_regs = of_iomap(np, 0))) {
> +		dev_dbg(&op->dev, "MEM base: %p\n", lp->sdma_regs);
> +	} else {
> +		dev_err(&op->dev, "unable to map DMA registers\n");
>  		goto nodev;
>  	}
> -	lp->sdma_dcrs = dcr_map(np, dcrs, dcr_resource_len(np, 0));
> -	dev_dbg(&op->dev, "DCR base: %x\n", dcrs);
>
>  	lp->rx_irq = irq_of_parse_and_map(np, 0);
>  	lp->tx_irq = irq_of_parse_and_map(np, 1);
> @@ -895,7 +904,7 @@
>
>  	lp->phy_node = of_parse_phandle(op->node, "phy-handle", 0);
>  	if (lp->phy_node)
> -		dev_dbg(lp->dev, "using PHY node %s (%p)\n", np->full_name, np);
> +		dev_dbg(lp->dev, "using PHY node %s (%p)\n", lp->phy_node->full_name, lp->phy_node);

This looks like an unrelated bug fix.  Please put into a separate
patch and post separately.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.