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

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

[John Tyner]

[-- Attachment #1: Type: text/plain, Size: 289 bytes --]

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>

[-- Attachment #2: temac.patch --]
[-- Type: text/x-diff, Size: 2402 bytes --]

--- /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);
+	}
 }
 
 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);
 
 	/* Add the device attributes */
 	rc = sysfs_create_group(&lp->dev->kobj, &temac_attr_group);

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

[Grant Likely]

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.

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

[John Tyner]

Grant Likely wrote:
> 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.
> 

[snip]

>>   * 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?

As you can see from the comments in the TODO section, someone thinks 
(and I would tend to agree) that it would be a good idea to separate the 
DMA code from the Ethernet driver entirely. I also agree with your 
suggestion of avoiding the ugliness of the if/else block, but it seems 
like a job better left to be done when that refactoring takes place.

This patch is only about 20 lines worth of changes, and to add a bunch 
of complexity doesn't seem worthwhile to me at this time.

>> @@ -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.

It is an unrelated bug fix. If it's really a big deal, I can submit a 
separate patch, but it seems like it would be easier for someone to 
manually make this change than for me to go back and generate a new 
patch (since I'm not working on anything related to this anymore).

I apologize for being kind of a dick about this stuff, but as I said, 
I'm not doing anything related to this stuff anymore. The change is 
really small, but adds support for something that currently doesn't 
work, so I'm hoping somebody else is willing to take up the cause in my 
stead. :)

John

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

[Grant Likely]

On Fri, Feb 5, 2010 at 9:16 AM, John Tyner <jtyner@cs.ucr.edu> wrote:
>
>
> Grant Likely wrote:
>>
>> 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 i=
f
>>> 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. =A0A couple of comments below.
>>
>
> [snip]
>
>>> =A0* TODO:
>>> - * - Fix driver to work on more than just Virtex5. =A0Right now the dr=
iver
>>> - * =A0 assumes that the locallink DMA registers are accessed via DCR
>>> - * =A0 instructions.
>>> =A0* - Factor out locallink DMA code into separate driver
>>> =A0* - Fix multicast assignment.
>>> =A0* - Fix support for hardware checksumming.
>>> @@ -117,12 +114,20 @@
>>>
>>> =A0static u32 temac_dma_in32(struct temac_local *lp, int reg)
>>> =A0{
>>> - =A0 =A0 =A0 return dcr_read(lp->sdma_dcrs, reg);
>>> + =A0 =A0 =A0 if (lp->sdma_regs) {
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return __raw_readl(lp->sdma_regs + reg);
>>> + =A0 =A0 =A0 } else {
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return dcr_read(lp->sdma_dcrs, reg);
>>> + =A0 =A0 =A0 }
>>
>> 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?
>
> As you can see from the comments in the TODO section, someone thinks (and=
 I
> would tend to agree) that it would be a good idea to separate the DMA cod=
e
> from the Ethernet driver entirely. I also agree with your suggestion of
> avoiding the ugliness of the if/else block, but it seems like a job bette=
r
> left to be done when that refactoring takes place.
>
> This patch is only about 20 lines worth of changes, and to add a bunch of
> complexity doesn't seem worthwhile to me at this time.

I wrote that comment, so that someone would happen to be me.  :-)

Breaking the register access functions out into callbacks or an ops
callbacks structure is unrelated to the DMA refactoring task.  Doing
callbacks is pretty trivial to implement and it makes things simpler,
not more complex.

>
>>> @@ -895,7 +904,7 @@
>>>
>>> =A0 =A0 =A0 =A0lp->phy_node =3D of_parse_phandle(op->node, "phy-handle"=
, 0);
>>> =A0 =A0 =A0 =A0if (lp->phy_node)
>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_dbg(lp->dev, "using PHY node %s (%p)\=
n",
>>> np->full_name, np);
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 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. =A0Please put into a separate
>> patch and post separately.
>
> It is an unrelated bug fix. If it's really a big deal, I can submit a
> separate patch, but it seems like it would be easier for someone to manua=
lly
> make this change than for me to go back and generate a new patch (since I=
'm
> not working on anything related to this anymore).
>
> I apologize for being kind of a dick about this stuff, but as I said, I'm
> not doing anything related to this stuff anymore. The change is really
> small, but adds support for something that currently doesn't work, so I'm
> hoping somebody else is willing to take up the cause in my stead. :)

Fair enough.  Thanks for posting what you have.

g.

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