[PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[John Linn]

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

The patch also updates the driver so that it runs on the MicroBlaze.
The changes were tested on the PowerPC 440, PowerPC 405, and the
MicroBlaze platforms.

Signed-off-by: John Tyner <jtyner@cs.ucr.edu>
Signed-off-by: John Linn <john.linn@xilinx.com>

---

V2 - Incorporated comments from Grant and added more logic to allow the driver
to work on MicroBlaze.

V3 - Only updated it to apply to head, minor change to include slab.h. Also
verified that it now builds for MicroBlaze. Retested on PowerPC and MicroBlaze.
---
 drivers/net/Kconfig         |    1 -
 drivers/net/ll_temac.h      |   17 +++++-
 drivers/net/ll_temac_main.c |  124 ++++++++++++++++++++++++++++++++++---------
 3 files changed, 113 insertions(+), 29 deletions(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 0ba5b8e..17044dc 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -2435,7 +2435,6 @@ config MV643XX_ETH
 config XILINX_LL_TEMAC
 	tristate "Xilinx LL TEMAC (LocalLink Tri-mode Ethernet MAC) driver"
 	select PHYLIB
-	depends on PPC_DCR_NATIVE
 	help
 	  This driver supports the Xilinx 10/100/1000 LocalLink TEMAC
 	  core used in Xilinx Spartan and Virtex FPGAs
diff --git a/drivers/net/ll_temac.h b/drivers/net/ll_temac.h
index 1af66a1..915aa34 100644
--- a/drivers/net/ll_temac.h
+++ b/drivers/net/ll_temac.h
@@ -5,8 +5,11 @@
 #include <linux/netdevice.h>
 #include <linux/of.h>
 #include <linux/spinlock.h>
+
+#ifdef CONFIG_PPC_DCR
 #include <asm/dcr.h>
 #include <asm/dcr-regs.h>
+#endif
 
 /* packet size info */
 #define XTE_HDR_SIZE			14      /* size of Ethernet header */
@@ -290,8 +293,12 @@ This option defaults to enabled (set) */
 
 #define TX_CONTROL_CALC_CSUM_MASK   1
 
+/* Align the IP data in the packet on word boundaries as MicroBlaze
+ * needs it.
+ */
+
 #define XTE_ALIGN       32
-#define BUFFER_ALIGN(adr) ((XTE_ALIGN - ((u32) adr)) % XTE_ALIGN)
+#define BUFFER_ALIGN(adr) ((34 - ((u32) adr)) % XTE_ALIGN)
 
 #define MULTICAST_CAM_TABLE_NUM 4
 
@@ -335,9 +342,15 @@ struct temac_local {
 	struct mii_bus *mii_bus;	/* MII bus reference */
 	int mdio_irqs[PHY_MAX_ADDR];	/* IRQs table for MDIO bus */
 
-	/* IO registers and IRQs */
+	/* IO registers, dma functions and IRQs */
 	void __iomem *regs;
+	void __iomem *sdma_regs;
+#ifdef CONFIG_PPC_DCR
 	dcr_host_t sdma_dcrs;
+#endif
+	u32 (*dma_in)(struct temac_local *, int);
+	void (*dma_out)(struct temac_local *, int, u32);
+
 	int tx_irq;
 	int rx_irq;
 	int emac_num;
diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
index ba617e3..801b076 100644
--- a/drivers/net/ll_temac_main.c
+++ b/drivers/net/ll_temac_main.c
@@ -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.
@@ -116,17 +113,86 @@ void temac_indirect_out32(struct temac_local *lp, int reg, u32 value)
 	temac_iow(lp, XTE_CTL0_OFFSET, CNTLREG_WRITE_ENABLE_MASK | reg);
 }
 
+/**
+ * temac_dma_in32 - Memory mapped DMA read, this function expects a
+ * register input that is based on DCR word addresses which
+ * are then converted to memory mapped byte addresses
+ */
 static u32 temac_dma_in32(struct temac_local *lp, int reg)
 {
-	return dcr_read(lp->sdma_dcrs, reg);
+	return in_be32((u32 *)(lp->sdma_regs + (reg << 2)));
 }
 
+/**
+ * temac_dma_out32 - Memory mapped DMA read, this function expects a
+ * register input that is based on DCR word addresses which
+ * are then converted to memory mapped byte addresses
+ */
 static void temac_dma_out32(struct temac_local *lp, int reg, u32 value)
 {
+	out_be32((u32 *)(lp->sdma_regs + (reg << 2)), value);
+}
+
+/* DMA register access functions can be DCR based or memory mapped.
+ * The PowerPC 440 is DCR based, the PowerPC 405 and MicroBlaze are both
+ * memory mapped.
+ */
+#ifdef CONFIG_PPC_DCR
+
+/**
+ * temac_dma_dcr_in32 - DCR based DMA read
+ */
+static u32 temac_dma_dcr_in(struct temac_local *lp, int reg)
+{
+	return dcr_read(lp->sdma_dcrs, reg);
+}
+
+/**
+ * temac_dma_dcr_out32 - DCR based DMA write
+ */
+static void temac_dma_dcr_out(struct temac_local *lp, int reg, u32 value)
+{
 	dcr_write(lp->sdma_dcrs, reg, value);
 }
 
 /**
+ * temac_dcr_setup - If the DMA is DCR based, then setup the address and
+ * I/O  functions
+ */
+static int temac_dcr_setup(struct temac_local *lp, struct of_device *op,
+				struct device_node *np)
+{
+	unsigned int dcrs;
+
+	/* setup the dcr address mapping if it's in the device tree */
+
+	dcrs = dcr_resource_start(np, 0);
+	if (dcrs != 0) {
+		lp->sdma_dcrs = dcr_map(np, dcrs, dcr_resource_len(np, 0));
+		lp->dma_in = temac_dma_dcr_in;
+		lp->dma_out = temac_dma_dcr_out;
+		dev_dbg(&op->dev, "DCR base: %x\n", dcrs);
+		return 0;
+	}
+	/* no DCR in the device tree, indicate a failure */
+	return -1;
+}
+
+#else
+
+/*
+ * temac_dcr_setup - This is a stub for when DCR is not supported,
+ * such as with MicroBlaze
+ */
+static int temac_dcr_setup(struct temac_local *lp, struct of_device *op,
+				struct device_node *np)
+{
+	return -1;
+}
+
+#endif
+
+/**
  * temac_dma_bd_init - Setup buffer descriptor rings
  */
 static int temac_dma_bd_init(struct net_device *ndev)
@@ -173,23 +239,23 @@ static int temac_dma_bd_init(struct net_device *ndev)
 		lp->rx_bd_v[i].app0 = STS_CTRL_APP0_IRQONEND;
 	}
 
-	temac_dma_out32(lp, TX_CHNL_CTRL, 0x10220400 |
+	lp->dma_out(lp, TX_CHNL_CTRL, 0x10220400 |
 					  CHNL_CTRL_IRQ_EN |
 					  CHNL_CTRL_IRQ_DLY_EN |
 					  CHNL_CTRL_IRQ_COAL_EN);
 	/* 0x10220483 */
 	/* 0x00100483 */
-	temac_dma_out32(lp, RX_CHNL_CTRL, 0xff010000 |
+	lp->dma_out(lp, RX_CHNL_CTRL, 0xff010000 |
 					  CHNL_CTRL_IRQ_EN |
 					  CHNL_CTRL_IRQ_DLY_EN |
 					  CHNL_CTRL_IRQ_COAL_EN |
 					  CHNL_CTRL_IRQ_IOE);
 	/* 0xff010283 */
 
-	temac_dma_out32(lp, RX_CURDESC_PTR,  lp->rx_bd_p);
-	temac_dma_out32(lp, RX_TAILDESC_PTR,
+	lp->dma_out(lp, RX_CURDESC_PTR,  lp->rx_bd_p);
+	lp->dma_out(lp, RX_TAILDESC_PTR,
 		       lp->rx_bd_p + (sizeof(*lp->rx_bd_v) * (RX_BD_NUM - 1)));
-	temac_dma_out32(lp, TX_CURDESC_PTR, lp->tx_bd_p);
+	lp->dma_out(lp, TX_CURDESC_PTR, lp->tx_bd_p);
 
 	return 0;
 }
@@ -427,9 +493,9 @@ static void temac_device_reset(struct net_device *ndev)
 	temac_indirect_out32(lp, XTE_RXC1_OFFSET, val & ~XTE_RXC1_RXEN_MASK);
 
 	/* Reset Local Link (DMA) */
-	temac_dma_out32(lp, DMA_CONTROL_REG, DMA_CONTROL_RST);
+	lp->dma_out(lp, DMA_CONTROL_REG, DMA_CONTROL_RST);
 	timeout = 1000;
-	while (temac_dma_in32(lp, DMA_CONTROL_REG) & DMA_CONTROL_RST) {
+	while (lp->dma_in(lp, DMA_CONTROL_REG) & DMA_CONTROL_RST) {
 		udelay(1);
 		if (--timeout == 0) {
 			dev_err(&ndev->dev,
@@ -437,7 +503,7 @@ static void temac_device_reset(struct net_device *ndev)
 			break;
 		}
 	}
-	temac_dma_out32(lp, DMA_CONTROL_REG, DMA_TAIL_ENABLE);
+	lp->dma_out(lp, DMA_CONTROL_REG, DMA_TAIL_ENABLE);
 
 	temac_dma_bd_init(ndev);
 
@@ -598,7 +664,7 @@ static int temac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		lp->tx_bd_tail = 0;
 
 	/* Kick off the transfer */
-	temac_dma_out32(lp, TX_TAILDESC_PTR, tail_p); /* DMA start */
+	lp->dma_out(lp, TX_TAILDESC_PTR, tail_p); /* DMA start */
 
 	return NETDEV_TX_OK;
 }
@@ -664,7 +730,7 @@ static void ll_temac_recv(struct net_device *ndev)
 		cur_p = &lp->rx_bd_v[lp->rx_bd_ci];
 		bdstat = cur_p->app0;
 	}
-	temac_dma_out32(lp, RX_TAILDESC_PTR, tail_p);
+	lp->dma_out(lp, RX_TAILDESC_PTR, tail_p);
 
 	spin_unlock_irqrestore(&lp->rx_lock, flags);
 }
@@ -675,8 +741,8 @@ static irqreturn_t ll_temac_tx_irq(int irq, void *_ndev)
 	struct temac_local *lp = netdev_priv(ndev);
 	unsigned int status;
 
-	status = temac_dma_in32(lp, TX_IRQ_REG);
-	temac_dma_out32(lp, TX_IRQ_REG, status);
+	status = lp->dma_in(lp, TX_IRQ_REG);
+	lp->dma_out(lp, TX_IRQ_REG, status);
 
 	if (status & (IRQ_COAL | IRQ_DLY))
 		temac_start_xmit_done(lp->ndev);
@@ -693,8 +759,8 @@ static irqreturn_t ll_temac_rx_irq(int irq, void *_ndev)
 	unsigned int status;
 
 	/* Read and clear the status registers */
-	status = temac_dma_in32(lp, RX_IRQ_REG);
-	temac_dma_out32(lp, RX_IRQ_REG, status);
+	status = lp->dma_in(lp, RX_IRQ_REG);
+	lp->dma_out(lp, RX_IRQ_REG, status);
 
 	if (status & (IRQ_COAL | IRQ_DLY))
 		ll_temac_recv(lp->ndev);
@@ -795,7 +861,7 @@ static ssize_t temac_show_llink_regs(struct device *dev,
 	int i, len = 0;
 
 	for (i = 0; i < 0x11; i++)
-		len += sprintf(buf + len, "%.8x%s", temac_dma_in32(lp, i),
+		len += sprintf(buf + len, "%.8x%s", lp->dma_in(lp, i),
 			       (i % 8) == 7 ? "\n" : " ");
 	len += sprintf(buf + len, "\n");
 
@@ -821,7 +887,6 @@ temac_of_probe(struct of_device *op, const struct of_device_id *match)
 	struct net_device *ndev;
 	const void *addr;
 	int size, rc = 0;
-	unsigned int dcrs;
 
 	/* Init network device structure */
 	ndev = alloc_etherdev(sizeof(*lp));
@@ -871,13 +936,20 @@ temac_of_probe(struct of_device *op, const struct of_device_id *match)
 		goto nodev;
 	}
 
-	dcrs = dcr_resource_start(np, 0);
-	if (dcrs == 0) {
-		dev_err(&op->dev, "could not get DMA register address\n");
-		goto nodev;
+	/* Setup the DMA register accesses, could be DCR or memory mapped */
+	if (temac_dcr_setup(lp, op, np)) {
+
+		/* no DCR in the device tree, try non-DCR */
+		lp->sdma_regs = of_iomap(np, 0);
+		if (lp->sdma_regs) {
+			lp->dma_in = temac_dma_in32;
+			lp->dma_out = temac_dma_out32;
+			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);
-- 
1.6.2.1



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[Eric Dumazet]

Le lundi 05 avril 2010 à 15:11 -0600, John Linn a écrit :
> 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).
> 
> The patch also updates the driver so that it runs on the MicroBlaze.
> The changes were tested on the PowerPC 440, PowerPC 405, and the
> MicroBlaze platforms.
> 
> Signed-off-by: John Tyner <jtyner@cs.ucr.edu>
> Signed-off-by: John Linn <john.linn@xilinx.com>
> 
> ---

> +/* Align the IP data in the packet on word boundaries as MicroBlaze
> + * needs it.
> + */
> +
>  #define XTE_ALIGN       32
> -#define BUFFER_ALIGN(adr) ((XTE_ALIGN - ((u32) adr)) % XTE_ALIGN)
> +#define BUFFER_ALIGN(adr) ((34 - ((u32) adr)) % XTE_ALIGN)
>  

Very interesting way of doing this, but why such convoluted thing ?

Because of the % 32, this is equivalent to :

#define BUFFER_ALIGN(adr) ((2 - ((u32) adr)) % XTE_ALIGN)

But wait, dont we recognise the magic constant NET_IP_ALIGN ?

So, I ask, cant you use netdev_alloc_skb_ip_align() in this driver ?

RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[John Linn]

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBFcmljIER1bWF6ZXQgW21haWx0
bzplcmljLmR1bWF6ZXRAZ21haWwuY29tXQ0KPiBTZW50OiBNb25kYXksIEFwcmlsIDA1LCAyMDEw
IDM6MzAgUE0NCj4gVG86IEpvaG4gTGlubg0KPiBDYzogbmV0ZGV2QHZnZXIua2VybmVsLm9yZzsg
bGludXhwcGMtZGV2QG96bGFicy5vcmc7IGdyYW50Lmxpa2VseUBzZWNyZXRsYWIuY2E7DQo+IGp3
Ym95ZXJAbGludXgudm5ldC5pYm0uY29tOyBqb2huLndpbGxpYW1zQHBldGFsb2dpeC5jb207IG1p
Y2hhbC5zaW1la0BwZXRhbG9naXguY29tOyBKb2huIFR5bmVyDQo+IFN1YmplY3Q6IFJlOiBbUEFU
Q0hdIFtWM10gQWRkIG5vbi1WaXJ0ZXg1IHN1cHBvcnQgZm9yIExMIFRFTUFDIGRyaXZlcg0KPiAN
Cj4gTGUgbHVuZGkgMDUgYXZyaWwgMjAxMCDDoCAxNToxMSAtMDYwMCwgSm9obiBMaW5uIGEgw6lj
cml0IDoNCj4gPiBUaGlzIHBhdGNoIGFkZHMgc3VwcG9ydCBmb3IgdXNpbmcgdGhlIExMIFRFTUFD
IEV0aGVybmV0IGRyaXZlciBvbg0KPiA+IG5vbi1WaXJ0ZXggNSBwbGF0Zm9ybXMgYnkgYWRkaW5n
IHN1cHBvcnQgZm9yIGFjY2Vzc2luZyB0aGUgU29mdCBETUENCj4gPiByZWdpc3RlcnMgYXMgaWYg
dGhleSB3ZXJlIG1lbW9yeSBtYXBwZWQgaW5zdGVhZCBvZiBzb2xlbHkgdGhyb3VnaCB0aGUNCj4g
PiBEQ1IncyAoYXZhaWxhYmxlIG9uIHRoZSBWaXJ0ZXggNSkuDQo+ID4NCj4gPiBUaGUgcGF0Y2gg
YWxzbyB1cGRhdGVzIHRoZSBkcml2ZXIgc28gdGhhdCBpdCBydW5zIG9uIHRoZSBNaWNyb0JsYXpl
Lg0KPiA+IFRoZSBjaGFuZ2VzIHdlcmUgdGVzdGVkIG9uIHRoZSBQb3dlclBDIDQ0MCwgUG93ZXJQ
QyA0MDUsIGFuZCB0aGUNCj4gPiBNaWNyb0JsYXplIHBsYXRmb3Jtcy4NCj4gPg0KPiA+IFNpZ25l
ZC1vZmYtYnk6IEpvaG4gVHluZXIgPGp0eW5lckBjcy51Y3IuZWR1Pg0KPiA+IFNpZ25lZC1vZmYt
Ynk6IEpvaG4gTGlubiA8am9obi5saW5uQHhpbGlueC5jb20+DQo+ID4NCj4gPiAtLS0NCj4gDQo+
ID4gKy8qIEFsaWduIHRoZSBJUCBkYXRhIGluIHRoZSBwYWNrZXQgb24gd29yZCBib3VuZGFyaWVz
IGFzIE1pY3JvQmxhemUNCj4gPiArICogbmVlZHMgaXQuDQo+ID4gKyAqLw0KPiA+ICsNCj4gPiAg
I2RlZmluZSBYVEVfQUxJR04gICAgICAgMzINCj4gPiAtI2RlZmluZSBCVUZGRVJfQUxJR04oYWRy
KSAoKFhURV9BTElHTiAtICgodTMyKSBhZHIpKSAlIFhURV9BTElHTikNCj4gPiArI2RlZmluZSBC
VUZGRVJfQUxJR04oYWRyKSAoKDM0IC0gKCh1MzIpIGFkcikpICUgWFRFX0FMSUdOKQ0KPiA+DQo+
IA0KPiBWZXJ5IGludGVyZXN0aW5nIHdheSBvZiBkb2luZyB0aGlzLCBidXQgd2h5IHN1Y2ggY29u
dm9sdXRlZCB0aGluZyA/DQoNCkdyYW50IG1pZ2h0IGhhdmUgaW5zaWdodCBpbnRvIHdoeSB0aGlz
IHN0YXJ0ZWQgdGhpcyB3YXksIEkganVzdCB1cGRhdGVkIHRvIGhlbHAgd2l0aCBNaWNyb0JsYXpl
IGFsaWdubWVudC4NCg0KPiANCj4gQmVjYXVzZSBvZiB0aGUgJSAzMiwgdGhpcyBpcyBlcXVpdmFs
ZW50IHRvIDoNCj4gDQo+ICNkZWZpbmUgQlVGRkVSX0FMSUdOKGFkcikgKCgyIC0gKCh1MzIpIGFk
cikpICUgWFRFX0FMSUdOKQ0KPiANCj4gQnV0IHdhaXQsIGRvbnQgd2UgcmVjb2duaXNlIHRoZSBt
YWdpYyBjb25zdGFudCBORVRfSVBfQUxJR04gPw0KPiANCj4gU28sIEkgYXNrLCBjYW50IHlvdSB1
c2UgbmV0ZGV2X2FsbG9jX3NrYl9pcF9hbGlnbigpIGluIHRoaXMgZHJpdmVyID8NCj4gDQo+IA0K
V2lsbCBoYXZlIHRvIGxvb2sgYXQgaXQgdG8gYmV0dGVyIHVuZGVyc3RhbmQsIG1heWJlLg0KDQpU
aGFua3MsDQpKb2huDQoNCgpUaGlzIGVtYWlsIGFuZCBhbnkgYXR0YWNobWVudHMgYXJlIGludGVu
ZGVkIGZvciB0aGUgc29sZSB1c2Ugb2YgdGhlIG5hbWVkIHJlY2lwaWVudChzKSBhbmQgY29udGFp
bihzKSBjb25maWRlbnRpYWwgaW5mb3JtYXRpb24gdGhhdCBtYXkgYmUgcHJvcHJpZXRhcnksIHBy
aXZpbGVnZWQgb3IgY29weXJpZ2h0ZWQgdW5kZXIgYXBwbGljYWJsZSBsYXcuIElmIHlvdSBhcmUg
bm90IHRoZSBpbnRlbmRlZCByZWNpcGllbnQsIGRvIG5vdCByZWFkLCBjb3B5LCBvciBmb3J3YXJk
IHRoaXMgZW1haWwgbWVzc2FnZSBvciBhbnkgYXR0YWNobWVudHMuIERlbGV0ZSB0aGlzIGVtYWls
IG1lc3NhZ2UgYW5kIGFueSBhdHRhY2htZW50cyBpbW1lZGlhdGVseS4K

RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[Steven J. Magnani]

On Mon, 2010-04-05 at 15:33 -0600, John Linn wrote:
> > > +/* Align the IP data in the packet on word boundaries as MicroBlaze
> > > + * needs it.
> > > + */
> > > +
> > >  #define XTE_ALIGN       32
> > > -#define BUFFER_ALIGN(adr) ((XTE_ALIGN - ((u32) adr)) % XTE_ALIGN)
> > > +#define BUFFER_ALIGN(adr) ((34 - ((u32) adr)) % XTE_ALIGN)
> > >
> > 
> > Very interesting way of doing this, but why such convoluted thing ?
> 
> Grant might have insight into why this started this way, I just updated to help with MicroBlaze alignment.
> 
> > 
> > Because of the % 32, this is equivalent to :
> > 
> > #define BUFFER_ALIGN(adr) ((2 - ((u32) adr)) % XTE_ALIGN)
> > 
> > But wait, dont we recognise the magic constant NET_IP_ALIGN ?
> > 
> > So, I ask, cant you use netdev_alloc_skb_ip_align() in this driver ?
> > 

That should work. I switched to that in the older xilinx_lltemac driver
without any problem. 

------------------------------------------------------------------------
 Steven J. Magnani               "I claim this network for MARS!
 www.digidescorp.com              Earthling, return my space modulator!"

 #include <standard.disclaimer>

Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[Grant Likely]

On Mon, Apr 5, 2010 at 3:33 PM, John Linn <John.Linn@xilinx.com> wrote:
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
>> > +/* Align the IP data in the packet on word boundaries as MicroBlaze
>> > + * needs it.
>> > + */
>> > +
>> > =A0#define XTE_ALIGN =A0 =A0 =A0 32
>> > -#define BUFFER_ALIGN(adr) ((XTE_ALIGN - ((u32) adr)) % XTE_ALIGN)
>> > +#define BUFFER_ALIGN(adr) ((34 - ((u32) adr)) % XTE_ALIGN)
>> >
>>
>> Very interesting way of doing this, but why such convoluted thing ?
>
> Grant might have insight into why this started this way, I just updated t=
o help with MicroBlaze alignment.

It was that way in the code I received from Yoshio Kashiwagi and David
H. Lynch.  I have no problem with it being changed.

Personally I would probably write a followup patch to change this to
netdev_alloc_skb_ip_align(), just to keep changes logically separated.
 But I'm okay with it rolled into this patch also.

g.

RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[John Linn]

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBFcmljIER1bWF6ZXQgW21haWx0
bzplcmljLmR1bWF6ZXRAZ21haWwuY29tXQ0KPiBTZW50OiBNb25kYXksIEFwcmlsIDA1LCAyMDEw
IDM6MzAgUE0NCj4gVG86IEpvaG4gTGlubg0KPiBDYzogbmV0ZGV2QHZnZXIua2VybmVsLm9yZzsg
bGludXhwcGMtZGV2QG96bGFicy5vcmc7IGdyYW50Lmxpa2VseUBzZWNyZXRsYWIuY2E7DQo+IGp3
Ym95ZXJAbGludXgudm5ldC5pYm0uY29tOyBqb2huLndpbGxpYW1zQHBldGFsb2dpeC5jb207IG1p
Y2hhbC5zaW1la0BwZXRhbG9naXguY29tOyBKb2huIFR5bmVyDQo+IFN1YmplY3Q6IFJlOiBbUEFU
Q0hdIFtWM10gQWRkIG5vbi1WaXJ0ZXg1IHN1cHBvcnQgZm9yIExMIFRFTUFDIGRyaXZlcg0KPiAN
Cj4gTGUgbHVuZGkgMDUgYXZyaWwgMjAxMCDDoCAxNToxMSAtMDYwMCwgSm9obiBMaW5uIGEgw6lj
cml0IDoNCj4gPiBUaGlzIHBhdGNoIGFkZHMgc3VwcG9ydCBmb3IgdXNpbmcgdGhlIExMIFRFTUFD
IEV0aGVybmV0IGRyaXZlciBvbg0KPiA+IG5vbi1WaXJ0ZXggNSBwbGF0Zm9ybXMgYnkgYWRkaW5n
IHN1cHBvcnQgZm9yIGFjY2Vzc2luZyB0aGUgU29mdCBETUENCj4gPiByZWdpc3RlcnMgYXMgaWYg
dGhleSB3ZXJlIG1lbW9yeSBtYXBwZWQgaW5zdGVhZCBvZiBzb2xlbHkgdGhyb3VnaCB0aGUNCj4g
PiBEQ1IncyAoYXZhaWxhYmxlIG9uIHRoZSBWaXJ0ZXggNSkuDQo+ID4NCj4gPiBUaGUgcGF0Y2gg
YWxzbyB1cGRhdGVzIHRoZSBkcml2ZXIgc28gdGhhdCBpdCBydW5zIG9uIHRoZSBNaWNyb0JsYXpl
Lg0KPiA+IFRoZSBjaGFuZ2VzIHdlcmUgdGVzdGVkIG9uIHRoZSBQb3dlclBDIDQ0MCwgUG93ZXJQ
QyA0MDUsIGFuZCB0aGUNCj4gPiBNaWNyb0JsYXplIHBsYXRmb3Jtcy4NCj4gPg0KPiA+IFNpZ25l
ZC1vZmYtYnk6IEpvaG4gVHluZXIgPGp0eW5lckBjcy51Y3IuZWR1Pg0KPiA+IFNpZ25lZC1vZmYt
Ynk6IEpvaG4gTGlubiA8am9obi5saW5uQHhpbGlueC5jb20+DQo+ID4NCj4gPiAtLS0NCj4gDQo+
ID4gKy8qIEFsaWduIHRoZSBJUCBkYXRhIGluIHRoZSBwYWNrZXQgb24gd29yZCBib3VuZGFyaWVz
IGFzIE1pY3JvQmxhemUNCj4gPiArICogbmVlZHMgaXQuDQo+ID4gKyAqLw0KPiA+ICsNCj4gPiAg
I2RlZmluZSBYVEVfQUxJR04gICAgICAgMzINCj4gPiAtI2RlZmluZSBCVUZGRVJfQUxJR04oYWRy
KSAoKFhURV9BTElHTiAtICgodTMyKSBhZHIpKSAlIFhURV9BTElHTikNCj4gPiArI2RlZmluZSBC
VUZGRVJfQUxJR04oYWRyKSAoKDM0IC0gKCh1MzIpIGFkcikpICUgWFRFX0FMSUdOKQ0KPiA+DQo+
IA0KPiBWZXJ5IGludGVyZXN0aW5nIHdheSBvZiBkb2luZyB0aGlzLCBidXQgd2h5IHN1Y2ggY29u
dm9sdXRlZCB0aGluZyA/DQoNClRoaXMgaXMgdHJ5aW5nIHRvIGFsaWduIGZvciBhIGNhY2hlIGxp
bmUgKDMyIGJ5dGVzKSBiZWZvcmUgbXkgY2hhbmdlLg0KDQpNeSBjaGFuZ2Ugd2FzIHRoZW4gYWxz
byBtYWtpbmcgaXQgYWxpZ24gdGhlIElQIGRhdGEgb24gYSB3b3JkIGJvdW5kYXJ5LiANCg0KPiAN
Cj4gQmVjYXVzZSBvZiB0aGUgJSAzMiwgdGhpcyBpcyBlcXVpdmFsZW50IHRvIDoNCj4gDQo+ICNk
ZWZpbmUgQlVGRkVSX0FMSUdOKGFkcikgKCgyIC0gKCh1MzIpIGFkcikpICUgWFRFX0FMSUdOKQ0K
PiANCg0KWWVzLCBidXQgSSdtIG5vdCBzdXJlIHRoYXQncyBjbGVhcmVyIElNSE8uDQoNCj4gQnV0
IHdhaXQsIGRvbnQgd2UgcmVjb2duaXNlIHRoZSBtYWdpYyBjb25zdGFudCBORVRfSVBfQUxJR04g
Pw0KDQpZZXMgaXQgY291bGQgYmUgdXNlZC4gIEknbSBzdHJ1Z2dsaW5nIHdpdGggaG93IHRvIG1h
a2UgdGhpcyBhbGwgYmUgY2xlYXJlci4NCg0KSG93IGFib3V0IHRoaXM/DQoNCiNkZWZpbmUgQlVG
RkVSX0FMSUdOKGFkcikgKCgoWFRFX0FMSUdOICsgTkVUX0lQX0FMSUdOKSAtICgodTMyKSBhZHIp
KSAlIFhURV9BTElHTikNCg0KPiANCj4gU28sIEkgYXNrLCBjYW50IHlvdSB1c2UgbmV0ZGV2X2Fs
bG9jX3NrYl9pcF9hbGlnbigpIGluIHRoaXMgZHJpdmVyID8NCg0KRnJvbSB3aGF0IEkgY2FuIHRl
bGwsIHRoaXMgd291bGRuJ3Qgd29yayBhcyBpdCBvbmx5IHJlc2VydmVzIHRoZSAyIGJ5dGVzIHRv
IGFsaWduIHdpdGggDQphIHdvcmQgYm91bmRhcnkuDQoNClRoYW5rcywNCkpvaG4NCg0KPiANCj4g
DQoNCgpUaGlzIGVtYWlsIGFuZCBhbnkgYXR0YWNobWVudHMgYXJlIGludGVuZGVkIGZvciB0aGUg
c29sZSB1c2Ugb2YgdGhlIG5hbWVkIHJlY2lwaWVudChzKSBhbmQgY29udGFpbihzKSBjb25maWRl
bnRpYWwgaW5mb3JtYXRpb24gdGhhdCBtYXkgYmUgcHJvcHJpZXRhcnksIHByaXZpbGVnZWQgb3Ig
Y29weXJpZ2h0ZWQgdW5kZXIgYXBwbGljYWJsZSBsYXcuIElmIHlvdSBhcmUgbm90IHRoZSBpbnRl
bmRlZCByZWNpcGllbnQsIGRvIG5vdCByZWFkLCBjb3B5LCBvciBmb3J3YXJkIHRoaXMgZW1haWwg
bWVzc2FnZSBvciBhbnkgYXR0YWNobWVudHMuIERlbGV0ZSB0aGlzIGVtYWlsIG1lc3NhZ2UgYW5k
IGFueSBhdHRhY2htZW50cyBpbW1lZGlhdGVseS4K

RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[Eric Dumazet]

Le mardi 06 avril 2010 à 10:12 -0600, John Linn a écrit :
> > -----Original Message-----
> > From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> > Sent: Monday, April 05, 2010 3:30 PM
> > To: John Linn
> > Cc: netdev@vger.kernel.org; linuxppc-dev@ozlabs.org; grant.likely@secretlab.ca;
> > jwboyer@linux.vnet.ibm.com; john.williams@petalogix.com; michal.simek@petalogix.com; John Tyner
> > Subject: Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
> > 
> > Le lundi 05 avril 2010 à 15:11 -0600, John Linn a écrit :
> > > 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).
> > >
> > > The patch also updates the driver so that it runs on the MicroBlaze.
> > > The changes were tested on the PowerPC 440, PowerPC 405, and the
> > > MicroBlaze platforms.
> > >
> > > Signed-off-by: John Tyner <jtyner@cs.ucr.edu>
> > > Signed-off-by: John Linn <john.linn@xilinx.com>
> > >
> > > ---
> > 
> > > +/* Align the IP data in the packet on word boundaries as MicroBlaze
> > > + * needs it.
> > > + */
> > > +
> > >  #define XTE_ALIGN       32
> > > -#define BUFFER_ALIGN(adr) ((XTE_ALIGN - ((u32) adr)) % XTE_ALIGN)
> > > +#define BUFFER_ALIGN(adr) ((34 - ((u32) adr)) % XTE_ALIGN)
> > >
> > 
> > Very interesting way of doing this, but why such convoluted thing ?
> 
> This is trying to align for a cache line (32 bytes) before my change.
> 
> My change was then also making it align the IP data on a word boundary. 
> 
> > 
> > Because of the % 32, this is equivalent to :
> > 
> > #define BUFFER_ALIGN(adr) ((2 - ((u32) adr)) % XTE_ALIGN)
> > 
> 
> Yes, but I'm not sure that's clearer IMHO.
> 
> > But wait, dont we recognise the magic constant NET_IP_ALIGN ?
> 
> Yes it could be used.  I'm struggling with how to make this all be clearer.
> 

I am not saying its clearer, I am saying we have a standard way to
handle this exact problem (aligning rcvs buffer so that IP header is
aligned)

There is no need to invent new ones, this makes reviewing of this driver
more difficult.


> How about this?
> #define BUFFER_ALIGN(adr) (((XTE_ALIGN + NET_IP_ALIGN) - ((u32) adr)) % XTE_ALIGN)
> 

Sorry, I still dont understand why you need XTE_ALIGN + ...

((A + B) - C) % A   is equal to (B - C) % A

Which one is more readable ?

Please take a look at existing and clean code, no magic macro, and we
can understand the intention.

find drivers/net | xargs grep -n netdev_alloc_skb_ip_align

RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[John Linn]

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBFcmljIER1bWF6ZXQgW21haWx0
bzplcmljLmR1bWF6ZXRAZ21haWwuY29tXQ0KPiBTZW50OiBUdWVzZGF5LCBBcHJpbCAwNiwgMjAx
MCAxMTowMSBBTQ0KPiBUbzogSm9obiBMaW5uDQo+IENjOiBuZXRkZXZAdmdlci5rZXJuZWwub3Jn
OyBsaW51eHBwYy1kZXZAb3psYWJzLm9yZzsgZ3JhbnQubGlrZWx5QHNlY3JldGxhYi5jYTsNCj4g
andib3llckBsaW51eC52bmV0LmlibS5jb207IGpvaG4ud2lsbGlhbXNAcGV0YWxvZ2l4LmNvbTsg
bWljaGFsLnNpbWVrQHBldGFsb2dpeC5jb207IEpvaG4gVHluZXINCj4gU3ViamVjdDogUkU6IFtQ
QVRDSF0gW1YzXSBBZGQgbm9uLVZpcnRleDUgc3VwcG9ydCBmb3IgTEwgVEVNQUMgZHJpdmVyDQo+
IA0KPiBMZSBtYXJkaSAwNiBhdnJpbCAyMDEwIMOgIDEwOjEyIC0wNjAwLCBKb2huIExpbm4gYSDD
qWNyaXQgOg0KPiA+ID4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gPiA+IEZyb206IEVy
aWMgRHVtYXpldCBbbWFpbHRvOmVyaWMuZHVtYXpldEBnbWFpbC5jb21dDQo+ID4gPiBTZW50OiBN
b25kYXksIEFwcmlsIDA1LCAyMDEwIDM6MzAgUE0NCj4gPiA+IFRvOiBKb2huIExpbm4NCj4gPiA+
IENjOiBuZXRkZXZAdmdlci5rZXJuZWwub3JnOyBsaW51eHBwYy1kZXZAb3psYWJzLm9yZzsgZ3Jh
bnQubGlrZWx5QHNlY3JldGxhYi5jYTsNCj4gPiA+IGp3Ym95ZXJAbGludXgudm5ldC5pYm0uY29t
OyBqb2huLndpbGxpYW1zQHBldGFsb2dpeC5jb207IG1pY2hhbC5zaW1la0BwZXRhbG9naXguY29t
OyBKb2huIFR5bmVyDQo+ID4gPiBTdWJqZWN0OiBSZTogW1BBVENIXSBbVjNdIEFkZCBub24tVmly
dGV4NSBzdXBwb3J0IGZvciBMTCBURU1BQyBkcml2ZXINCj4gPiA+DQo+ID4gPiBMZSBsdW5kaSAw
NSBhdnJpbCAyMDEwIMOgIDE1OjExIC0wNjAwLCBKb2huIExpbm4gYSDDqWNyaXQgOg0KPiA+ID4g
PiBUaGlzIHBhdGNoIGFkZHMgc3VwcG9ydCBmb3IgdXNpbmcgdGhlIExMIFRFTUFDIEV0aGVybmV0
IGRyaXZlciBvbg0KPiA+ID4gPiBub24tVmlydGV4IDUgcGxhdGZvcm1zIGJ5IGFkZGluZyBzdXBw
b3J0IGZvciBhY2Nlc3NpbmcgdGhlIFNvZnQgRE1BDQo+ID4gPiA+IHJlZ2lzdGVycyBhcyBpZiB0
aGV5IHdlcmUgbWVtb3J5IG1hcHBlZCBpbnN0ZWFkIG9mIHNvbGVseSB0aHJvdWdoIHRoZQ0KPiA+
ID4gPiBEQ1IncyAoYXZhaWxhYmxlIG9uIHRoZSBWaXJ0ZXggNSkuDQo+ID4gPiA+DQo+ID4gPiA+
IFRoZSBwYXRjaCBhbHNvIHVwZGF0ZXMgdGhlIGRyaXZlciBzbyB0aGF0IGl0IHJ1bnMgb24gdGhl
IE1pY3JvQmxhemUuDQo+ID4gPiA+IFRoZSBjaGFuZ2VzIHdlcmUgdGVzdGVkIG9uIHRoZSBQb3dl
clBDIDQ0MCwgUG93ZXJQQyA0MDUsIGFuZCB0aGUNCj4gPiA+ID4gTWljcm9CbGF6ZSBwbGF0Zm9y
bXMuDQo+ID4gPiA+DQo+ID4gPiA+IFNpZ25lZC1vZmYtYnk6IEpvaG4gVHluZXIgPGp0eW5lckBj
cy51Y3IuZWR1Pg0KPiA+ID4gPiBTaWduZWQtb2ZmLWJ5OiBKb2huIExpbm4gPGpvaG4ubGlubkB4
aWxpbnguY29tPg0KPiA+ID4gPg0KPiA+ID4gPiAtLS0NCj4gPiA+DQo+ID4gPiA+ICsvKiBBbGln
biB0aGUgSVAgZGF0YSBpbiB0aGUgcGFja2V0IG9uIHdvcmQgYm91bmRhcmllcyBhcyBNaWNyb0Js
YXplDQo+ID4gPiA+ICsgKiBuZWVkcyBpdC4NCj4gPiA+ID4gKyAqLw0KPiA+ID4gPiArDQo+ID4g
PiA+ICAjZGVmaW5lIFhURV9BTElHTiAgICAgICAzMg0KPiA+ID4gPiAtI2RlZmluZSBCVUZGRVJf
QUxJR04oYWRyKSAoKFhURV9BTElHTiAtICgodTMyKSBhZHIpKSAlIFhURV9BTElHTikNCj4gPiA+
ID4gKyNkZWZpbmUgQlVGRkVSX0FMSUdOKGFkcikgKCgzNCAtICgodTMyKSBhZHIpKSAlIFhURV9B
TElHTikNCj4gPiA+ID4NCj4gPiA+DQo+ID4gPiBWZXJ5IGludGVyZXN0aW5nIHdheSBvZiBkb2lu
ZyB0aGlzLCBidXQgd2h5IHN1Y2ggY29udm9sdXRlZCB0aGluZyA/DQo+ID4NCj4gPiBUaGlzIGlz
IHRyeWluZyB0byBhbGlnbiBmb3IgYSBjYWNoZSBsaW5lICgzMiBieXRlcykgYmVmb3JlIG15IGNo
YW5nZS4NCj4gPg0KPiA+IE15IGNoYW5nZSB3YXMgdGhlbiBhbHNvIG1ha2luZyBpdCBhbGlnbiB0
aGUgSVAgZGF0YSBvbiBhIHdvcmQgYm91bmRhcnkuDQo+ID4NCj4gPiA+DQo+ID4gPiBCZWNhdXNl
IG9mIHRoZSAlIDMyLCB0aGlzIGlzIGVxdWl2YWxlbnQgdG8gOg0KPiA+ID4NCj4gPiA+ICNkZWZp
bmUgQlVGRkVSX0FMSUdOKGFkcikgKCgyIC0gKCh1MzIpIGFkcikpICUgWFRFX0FMSUdOKQ0KPiA+
ID4NCj4gPg0KPiA+IFllcywgYnV0IEknbSBub3Qgc3VyZSB0aGF0J3MgY2xlYXJlciBJTUhPLg0K
PiA+DQo+ID4gPiBCdXQgd2FpdCwgZG9udCB3ZSByZWNvZ25pc2UgdGhlIG1hZ2ljIGNvbnN0YW50
IE5FVF9JUF9BTElHTiA/DQo+ID4NCj4gPiBZZXMgaXQgY291bGQgYmUgdXNlZC4gIEknbSBzdHJ1
Z2dsaW5nIHdpdGggaG93IHRvIG1ha2UgdGhpcyBhbGwgYmUgY2xlYXJlci4NCj4gPg0KPiANCj4g
SSBhbSBub3Qgc2F5aW5nIGl0cyBjbGVhcmVyLCBJIGFtIHNheWluZyB3ZSBoYXZlIGEgc3RhbmRh
cmQgd2F5IHRvDQo+IGhhbmRsZSB0aGlzIGV4YWN0IHByb2JsZW0gKGFsaWduaW5nIHJjdnMgYnVm
ZmVyIHNvIHRoYXQgSVAgaGVhZGVyIGlzDQo+IGFsaWduZWQpDQo+IA0KPiBUaGVyZSBpcyBubyBu
ZWVkIHRvIGludmVudCBuZXcgb25lcywgdGhpcyBtYWtlcyByZXZpZXdpbmcgb2YgdGhpcyBkcml2
ZXINCj4gbW9yZSBkaWZmaWN1bHQuDQo+IA0KPiANCj4gPiBIb3cgYWJvdXQgdGhpcz8NCj4gPiAj
ZGVmaW5lIEJVRkZFUl9BTElHTihhZHIpICgoKFhURV9BTElHTiArIE5FVF9JUF9BTElHTikgLSAo
KHUzMikgYWRyKSkgJSBYVEVfQUxJR04pDQo+ID4NCj4gDQo+IFNvcnJ5LCBJIHN0aWxsIGRvbnQg
dW5kZXJzdGFuZCB3aHkgeW91IG5lZWQgWFRFX0FMSUdOICsgLi4uDQo+IA0KPiAoKEEgKyBCKSAt
IEMpICUgQSAgIGlzIGVxdWFsIHRvIChCIC0gQykgJSBBDQo+IA0KPiBXaGljaCBvbmUgaXMgbW9y
ZSByZWFkYWJsZSA/DQoNCkknbSBmaW5lIHdpdGggeW91ciBzdWdnZXN0aW9uLg0KDQojZGVmaW5l
IEJVRkZFUl9BTElHTihhZHIpICgoMiAtICgodTMyKSBhZHIpKSAlIFhURV9BTElHTikNCg0KPiAN
Cj4gUGxlYXNlIHRha2UgYSBsb29rIGF0IGV4aXN0aW5nIGFuZCBjbGVhbiBjb2RlLCBubyBtYWdp
YyBtYWNybywgYW5kIHdlDQo+IGNhbiB1bmRlcnN0YW5kIHRoZSBpbnRlbnRpb24uDQo+IA0KPiBm
aW5kIGRyaXZlcnMvbmV0IHwgeGFyZ3MgZ3JlcCAtbiBuZXRkZXZfYWxsb2Nfc2tiX2lwX2FsaWdu
DQo+IA0KPiANCg0KWWVzIEkgc2VlIGhvdyBpdCdzIHVzZWQsIGJ1dCBpdCBvbmx5IGFsbG93cyB5
b3UgdG8gcmVzZXJ2ZSAyIGJ5dGVzIGluIHRoZSBza2Igd2l0aCBubyBvcHRpb25zLg0KDQoNCg0K
ClRoaXMgZW1haWwgYW5kIGFueSBhdHRhY2htZW50cyBhcmUgaW50ZW5kZWQgZm9yIHRoZSBzb2xl
IHVzZSBvZiB0aGUgbmFtZWQgcmVjaXBpZW50KHMpIGFuZCBjb250YWluKHMpIGNvbmZpZGVudGlh
bCBpbmZvcm1hdGlvbiB0aGF0IG1heSBiZSBwcm9wcmlldGFyeSwgcHJpdmlsZWdlZCBvciBjb3B5
cmlnaHRlZCB1bmRlciBhcHBsaWNhYmxlIGxhdy4gSWYgeW91IGFyZSBub3QgdGhlIGludGVuZGVk
IHJlY2lwaWVudCwgZG8gbm90IHJlYWQsIGNvcHksIG9yIGZvcndhcmQgdGhpcyBlbWFpbCBtZXNz
YWdlIG9yIGFueSBhdHRhY2htZW50cy4gRGVsZXRlIHRoaXMgZW1haWwgbWVzc2FnZSBhbmQgYW55
IGF0dGFjaG1lbnRzIGltbWVkaWF0ZWx5Lgo=

RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[Eric Dumazet]

> 
> Yes I see how it's used, but it only allows you to reserve 2 bytes in the skb with no options.


Really ? This would be a bug !

__alloc_skb() uses kmalloc(XXXX), this gives you the guarantee you want,
or maybe comment you wrote is not what is _really_ necessary ?

/* Align the IP data in the packet on word boundaries as MicroBlaze
 * needs it.
 */

Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[Grant Likely]

On Tue, Apr 6, 2010 at 11:11 AM, John Linn <John.Linn@xilinx.com> wrote:
>> -----Original Message-----
>> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
>> Sent: Tuesday, April 06, 2010 11:01 AM
>> To: John Linn
>> Cc: netdev@vger.kernel.org; linuxppc-dev@ozlabs.org; grant.likely@secret=
lab.ca;
>> jwboyer@linux.vnet.ibm.com; john.williams@petalogix.com; michal.simek@pe=
talogix.com; John Tyner
>> Subject: RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
>>
>> Le mardi 06 avril 2010 =E0 10:12 -0600, John Linn a =E9crit :
>> > > -----Original Message-----
>> > > From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
>> > > Sent: Monday, April 05, 2010 3:30 PM
>> > > To: John Linn
>> > > Cc: netdev@vger.kernel.org; linuxppc-dev@ozlabs.org; grant.likely@se=
cretlab.ca;
>> > > jwboyer@linux.vnet.ibm.com; john.williams@petalogix.com; michal.sime=
k@petalogix.com; John Tyner
>> > > Subject: Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC drive=
r
>> > >
>> > > Le lundi 05 avril 2010 =E0 15:11 -0600, John Linn a =E9crit :
>> > > > This patch adds support for using the LL TEMAC Ethernet driver on
>> > > > non-Virtex 5 platforms by adding support for accessing the Soft DM=
A
>> > > > registers as if they were memory mapped instead of solely through =
the
>> > > > DCR's (available on the Virtex 5).
>> > > >
>> > > > The patch also updates the driver so that it runs on the MicroBlaz=
e.
>> > > > The changes were tested on the PowerPC 440, PowerPC 405, and the
>> > > > MicroBlaze platforms.
>> > > >
>> > > > Signed-off-by: John Tyner <jtyner@cs.ucr.edu>
>> > > > Signed-off-by: John Linn <john.linn@xilinx.com>
>> > > >
>> > > > ---
>> > >
>> > > > +/* Align the IP data in the packet on word boundaries as MicroBla=
ze
>> > > > + * needs it.
>> > > > + */
>> > > > +
>> > > > =A0#define XTE_ALIGN =A0 =A0 =A0 32
>> > > > -#define BUFFER_ALIGN(adr) ((XTE_ALIGN - ((u32) adr)) % XTE_ALIGN)
>> > > > +#define BUFFER_ALIGN(adr) ((34 - ((u32) adr)) % XTE_ALIGN)
>> > > >
>> > >
>> > > Very interesting way of doing this, but why such convoluted thing ?
>> >
>> > This is trying to align for a cache line (32 bytes) before my change.
>> >
>> > My change was then also making it align the IP data on a word boundary=
.
>> >
>> > >
>> > > Because of the % 32, this is equivalent to :
>> > >
>> > > #define BUFFER_ALIGN(adr) ((2 - ((u32) adr)) % XTE_ALIGN)
>> > >
>> >
>> > Yes, but I'm not sure that's clearer IMHO.
>> >
>> > > But wait, dont we recognise the magic constant NET_IP_ALIGN ?
>> >
>> > Yes it could be used. =A0I'm struggling with how to make this all be c=
learer.
>> >
>>
>> I am not saying its clearer, I am saying we have a standard way to
>> handle this exact problem (aligning rcvs buffer so that IP header is
>> aligned)
>>
>> There is no need to invent new ones, this makes reviewing of this driver
>> more difficult.

Hold on.... BUFFER_ALIGN is being used to align the DMA buffer on a
cache line boundary.  I don't think netdev_alloc_skb() makes any
guarantees about how the start of the IP header lines up against cache
line boundaries.  The amount of padding needed is not known until an
skbuff is obtained from netdev_alloc_skb(), and
netdev_alloc_skb_ip_align() can only handle a fixed size padding,

It doesn't look like netdev_alloc_skb_ip_align() is the right thing in
this regard.

>> > How about this?
>> > #define BUFFER_ALIGN(adr) (((XTE_ALIGN + NET_IP_ALIGN) - ((u32) adr)) =
% XTE_ALIGN)
>> >
>>
>> Sorry, I still dont understand why you need XTE_ALIGN + ...
>>
>> ((A + B) - C) % A =A0 is equal to (B - C) % A
>>
>> Which one is more readable ?
>
> I'm fine with your suggestion.
>
> #define BUFFER_ALIGN(adr) ((2 - ((u32) adr)) % XTE_ALIGN)
>
>>
>> Please take a look at existing and clean code, no magic macro, and we
>> can understand the intention.
>>
>> find drivers/net | xargs grep -n netdev_alloc_skb_ip_align
>>
>>
>
> Yes I see how it's used, but it only allows you to reserve 2 bytes in the=
 skb with no options.

Eric is here.  The mod operation means that BUFFER_ALIGN using either
2 or 34 is equivalent.

g.

RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[John Linn]

> -----Original Message-----
> From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Gra=
nt Likely
> Sent: Tuesday, April 06, 2010 12:54 PM
> To: John Linn
> Cc: Eric Dumazet; netdev@vger.kernel.org; linuxppc-dev@ozlabs.org; jwboye=
r@linux.vnet.ibm.com;
> john.williams@petalogix.com; michal.simek@petalogix.com; John Tyner
> Subject: Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
> =

> On Tue, Apr 6, 2010 at 11:11 AM, John Linn <John.Linn@xilinx.com> wrote:
> >> -----Original Message-----
> >> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> >> Sent: Tuesday, April 06, 2010 11:01 AM
> >> To: John Linn
> >> Cc: netdev@vger.kernel.org; linuxppc-dev@ozlabs.org; grant.likely@secr=
etlab.ca;
> >> jwboyer@linux.vnet.ibm.com; john.williams@petalogix.com; michal.simek@=
petalogix.com; John Tyner
> >> Subject: RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
> >>
> >> Le mardi 06 avril 2010 =E0 10:12 -0600, John Linn a =E9crit :
> >> > > -----Original Message-----
> >> > > From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> >> > > Sent: Monday, April 05, 2010 3:30 PM
> >> > > To: John Linn
> >> > > Cc: netdev@vger.kernel.org; linuxppc-dev@ozlabs.org; grant.likely@=
secretlab.ca;
> >> > > jwboyer@linux.vnet.ibm.com; john.williams@petalogix.com; michal.si=
mek@petalogix.com; John Tyner
> >> > > Subject: Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC dri=
ver
> >> > >
> >> > > Le lundi 05 avril 2010 =E0 15:11 -0600, John Linn a =E9crit :
> >> > > > This patch adds support for using the LL TEMAC Ethernet driver o=
n
> >> > > > non-Virtex 5 platforms by adding support for accessing the Soft =
DMA
> >> > > > registers as if they were memory mapped instead of solely throug=
h the
> >> > > > DCR's (available on the Virtex 5).
> >> > > >
> >> > > > The patch also updates the driver so that it runs on the MicroBl=
aze.
> >> > > > The changes were tested on the PowerPC 440, PowerPC 405, and the=

> >> > > > MicroBlaze platforms.
> >> > > >
> >> > > > Signed-off-by: John Tyner <jtyner@cs.ucr.edu>
> >> > > > Signed-off-by: John Linn <john.linn@xilinx.com>
> >> > > >
> >> > > > ---
> >> > >
> >> > > > +/* Align the IP data in the packet on word boundaries as MicroB=
laze
> >> > > > + * needs it.
> >> > > > + */
> >> > > > +
> >> > > > =A0#define XTE_ALIGN =A0 =A0 =A0 32
> >> > > > -#define BUFFER_ALIGN(adr) ((XTE_ALIGN - ((u32) adr)) % XTE_ALIG=
N)
> >> > > > +#define BUFFER_ALIGN(adr) ((34 - ((u32) adr)) % XTE_ALIGN)
> >> > > >
> >> > >
> >> > > Very interesting way of doing this, but why such convoluted thing =
?
> >> >
> >> > This is trying to align for a cache line (32 bytes) before my change=
.
> >> >
> >> > My change was then also making it align the IP data on a word bounda=
ry.
> >> >
> >> > >
> >> > > Because of the % 32, this is equivalent to :
> >> > >
> >> > > #define BUFFER_ALIGN(adr) ((2 - ((u32) adr)) % XTE_ALIGN)
> >> > >
> >> >
> >> > Yes, but I'm not sure that's clearer IMHO.
> >> >
> >> > > But wait, dont we recognise the magic constant NET_IP_ALIGN ?
> >> >
> >> > Yes it could be used. =A0I'm struggling with how to make this all be=
 clearer.
> >> >
> >>
> >> I am not saying its clearer, I am saying we have a standard way to
> >> handle this exact problem (aligning rcvs buffer so that IP header is
> >> aligned)
> >>
> >> There is no need to invent new ones, this makes reviewing of this driv=
er
> >> more difficult.
> =

> Hold on.... BUFFER_ALIGN is being used to align the DMA buffer on a
> cache line boundary.  I don't think netdev_alloc_skb() makes any
> guarantees about how the start of the IP header lines up against cache
> line boundaries.  The amount of padding needed is not known until an
> skbuff is obtained from netdev_alloc_skb(), and
> netdev_alloc_skb_ip_align() can only handle a fixed size padding,
> =

> It doesn't look like netdev_alloc_skb_ip_align() is the right thing in
> this regard.
> =

> >> > How about this?
> >> > #define BUFFER_ALIGN(adr) (((XTE_ALIGN + NET_IP_ALIGN) - ((u32) adr)=
) % XTE_ALIGN)
> >> >
> >>
> >> Sorry, I still dont understand why you need XTE_ALIGN + ...
> >>
> >> ((A + B) - C) % A =A0 is equal to (B - C) % A
> >>
> >> Which one is more readable ?
> >
> > I'm fine with your suggestion.
> >
> > #define BUFFER_ALIGN(adr) ((2 - ((u32) adr)) % XTE_ALIGN)
> >
> >>
> >> Please take a look at existing and clean code, no magic macro, and we
> >> can understand the intention.
> >>
> >> find drivers/net | xargs grep -n netdev_alloc_skb_ip_align
> >>
> >>
> >
> > Yes I see how it's used, but it only allows you to reserve 2 bytes in t=
he skb with no options.
> =

> Eric is here.  The mod operation means that BUFFER_ALIGN using either
> 2 or 34 is equivalent.
> =

> g.

I can spin another patch with the following and with Grant's Kconfig change=
s, just looking for confirmation that's acceptable.

#define BUFFER_ALIGN(adr) ((2 - ((u32) adr)) % XTE_ALIGN)

Thanks,
John




This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.

Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[Steven J. Magnani]

On Tue, 2010-04-06 at 12:53 -0600, Grant Likely wrote:

> Hold on.... BUFFER_ALIGN is being used to align the DMA buffer on a
> cache line boundary.  I don't think netdev_alloc_skb() makes any
> guarantees about how the start of the IP header lines up against cache
> line boundaries.  The amount of padding needed is not known until an
> skbuff is obtained from netdev_alloc_skb(), and
> netdev_alloc_skb_ip_align() can only handle a fixed size padding,
> 
> It doesn't look like netdev_alloc_skb_ip_align() is the right thing in
> this regard.

__netdev_alloc_skb reserves NET_SKB_PAD bytes which gets us cacheline
alignment on Microblaze. From include/linux/skbuff.h:

/*
 * The networking layer reserves some headroom in skb data (via
 * dev_alloc_skb). This is used to avoid having to reallocate skb data
when
 * the header has to grow. In the default case, if the header has to
grow
 * 32 bytes or less we avoid the reallocation.
 *
 * Unfortunately this headroom changes the DMA alignment of the
resulting
 * network packet. As for NET_IP_ALIGN, this unaligned DMA is expensive
 * on some architectures. An architecture can override this value,
 * perhaps setting it to a cacheline in size (since that will maintain
 * cacheline alignment of the DMA). It must be a power of 2.
 *
 * Various parts of the networking layer expect at least 32 bytes of
 * headroom, you should not reduce this.
 */
#ifndef NET_SKB_PAD
#define NET_SKB_PAD     32
#endif

If this doesn't work for some of the PPC variants with larger cache
lines, maybe one of the PPC header files needs to define NET_SKB_PAD?
And if we want to guard against possible future changes to the default
NET_SKB_PAD breaking Microblaze operation, maybe one of its headers
should define NET_SKB_PAD as well?

------------------------------------------------------------------------
 Steven J. Magnani               "I claim this network for MARS!
 www.digidescorp.com              Earthling, return my space modulator!"

 #include <standard.disclaimer>

RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[John Linn]

> -----Original Message-----
> From: Steven J. Magnani [mailto:steve@digidescorp.com]
> Sent: Tuesday, April 06, 2010 2:04 PM
> To: grant.likely@secretlab.ca
> Cc: John Linn; Eric Dumazet; netdev@vger.kernel.org;
linuxppc-dev@ozlabs.org;
> jwboyer@linux.vnet.ibm.com; john.williams@petalogix.com;
michal.simek@petalogix.com; John Tyner
> Subject: Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
> =

> On Tue, 2010-04-06 at 12:53 -0600, Grant Likely wrote:
> =

> > Hold on.... BUFFER_ALIGN is being used to align the DMA buffer on a
> > cache line boundary.  I don't think netdev_alloc_skb() makes any
> > guarantees about how the start of the IP header lines up against
cache
> > line boundaries.  The amount of padding needed is not known until an
> > skbuff is obtained from netdev_alloc_skb(), and
> > netdev_alloc_skb_ip_align() can only handle a fixed size padding,
> >
> > It doesn't look like netdev_alloc_skb_ip_align() is the right thing
in
> > this regard.
> =

> __netdev_alloc_skb reserves NET_SKB_PAD bytes which gets us cacheline
> alignment on Microblaze. From include/linux/skbuff.h:
> =


Good find. I'll give it a test on MicroBlaze and PowerPC.

> /*
>  * The networking layer reserves some headroom in skb data (via
>  * dev_alloc_skb). This is used to avoid having to reallocate skb data
> when
>  * the header has to grow. In the default case, if the header has to
> grow
>  * 32 bytes or less we avoid the reallocation.
>  *
>  * Unfortunately this headroom changes the DMA alignment of the
> resulting
>  * network packet. As for NET_IP_ALIGN, this unaligned DMA is
expensive
>  * on some architectures. An architecture can override this value,
>  * perhaps setting it to a cacheline in size (since that will maintain
>  * cacheline alignment of the DMA). It must be a power of 2.
>  *
>  * Various parts of the networking layer expect at least 32 bytes of
>  * headroom, you should not reduce this.
>  */
> #ifndef NET_SKB_PAD
> #define NET_SKB_PAD     32
> #endif
> =

> If this doesn't work for some of the PPC variants with larger cache
> lines, maybe one of the PPC header files needs to define NET_SKB_PAD?

Looks like it is defined in system.h in powerpc so that it works.

> And if we want to guard against possible future changes to the default
> NET_SKB_PAD breaking Microblaze operation, maybe one of its headers
> should define NET_SKB_PAD as well?

Good idea, we can add that to system.h for MicroBlaze also.

Thanks,
John

> =

>
------------------------------------------------------------------------
>  Steven J. Magnani               "I claim this network for MARS!
>  www.digidescorp.com              Earthling, return my space
modulator!"
> =

>  #include <standard.disclaimer>
> =

> =

> =



This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.

Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[Eric Dumazet]

Le mardi 06 avril 2010 à 12:53 -0600, Grant Likely a écrit :

> Hold on.... BUFFER_ALIGN is being used to align the DMA buffer on a
> cache line boundary.  I don't think netdev_alloc_skb() makes any
> guarantees about how the start of the IP header lines up against cache
> line boundaries.  The amount of padding needed is not known until an
> skbuff is obtained from netdev_alloc_skb(), and
> netdev_alloc_skb_ip_align() can only handle a fixed size padding,
> 
> It doesn't look like netdev_alloc_skb_ip_align() is the right thing in
> this regard.
> 

OK, time to have a long explanation :

netdev_alloc_skb_ip_align() is doing the right thing, but it seems you
guys insist to invent a new private stuff.

I am only wondering if you really know why you do this. 

Many drivers do have same requirements, so every driver should reinvent
the wheel ? Really this is beyond me.

Original code was aligning the buffer on a 32 bytes boundary (because of
a hardware requirement on NIC, I only trust original code on this).

Then you want to change this to align buffer on this 32 bytes boundary
PLUS 2. What is this kind of new requirement ? 

1) Hardware requirement really changed that much. (firmware changed on
NIC). If not using this new alignement, NIC doesnt work at all.

2) or Microblaze arch requires that IP header is aligned on a word
boundary to avoid unaligned traps in IP stack ? (like many arches)

If this is the latest requirement, then use standard mechanism.
skb data is naturally aligned on L1_CACHE_SIZE + SKB_PAD boundaries.
(32 bytes alignment)

netdev_alloc_skb_ip_align()() then skips 2 bytes to make skb data
aligned so that 2 + 14 (sizeof eth header) = 16 : IP header is aligned
(modulo 16)

It just works. If not, we should correct it, please fill a bug report.

Thanks

RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[John Linn]

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBFcmljIER1bWF6ZXQgW21haWx0
bzplcmljLmR1bWF6ZXRAZ21haWwuY29tXQ0KPiBTZW50OiBUdWVzZGF5LCBBcHJpbCAwNiwgMjAx
MCAyOjI0IFBNDQo+IFRvOiBncmFudC5saWtlbHlAc2VjcmV0bGFiLmNhDQo+IENjOiBKb2huIExp
bm47IG5ldGRldkB2Z2VyLmtlcm5lbC5vcmc7IGxpbnV4cHBjLWRldkBvemxhYnMub3JnOyBqd2Jv
eWVyQGxpbnV4LnZuZXQuaWJtLmNvbTsNCj4gam9obi53aWxsaWFtc0BwZXRhbG9naXguY29tOyBt
aWNoYWwuc2ltZWtAcGV0YWxvZ2l4LmNvbTsgSm9obiBUeW5lcg0KPiBTdWJqZWN0OiBSZTogW1BB
VENIXSBbVjNdIEFkZCBub24tVmlydGV4NSBzdXBwb3J0IGZvciBMTCBURU1BQyBkcml2ZXINCj4g
DQo+IExlIG1hcmRpIDA2IGF2cmlsIDIwMTAgw6AgMTI6NTMgLTA2MDAsIEdyYW50IExpa2VseSBh
IMOpY3JpdCA6DQo+IA0KPiA+IEhvbGQgb24uLi4uIEJVRkZFUl9BTElHTiBpcyBiZWluZyB1c2Vk
IHRvIGFsaWduIHRoZSBETUEgYnVmZmVyIG9uIGENCj4gPiBjYWNoZSBsaW5lIGJvdW5kYXJ5LiAg
SSBkb24ndCB0aGluayBuZXRkZXZfYWxsb2Nfc2tiKCkgbWFrZXMgYW55DQo+ID4gZ3VhcmFudGVl
cyBhYm91dCBob3cgdGhlIHN0YXJ0IG9mIHRoZSBJUCBoZWFkZXIgbGluZXMgdXAgYWdhaW5zdCBj
YWNoZQ0KPiA+IGxpbmUgYm91bmRhcmllcy4gIFRoZSBhbW91bnQgb2YgcGFkZGluZyBuZWVkZWQg
aXMgbm90IGtub3duIHVudGlsIGFuDQo+ID4gc2tidWZmIGlzIG9idGFpbmVkIGZyb20gbmV0ZGV2
X2FsbG9jX3NrYigpLCBhbmQNCj4gPiBuZXRkZXZfYWxsb2Nfc2tiX2lwX2FsaWduKCkgY2FuIG9u
bHkgaGFuZGxlIGEgZml4ZWQgc2l6ZSBwYWRkaW5nLA0KPiA+DQo+ID4gSXQgZG9lc24ndCBsb29r
IGxpa2UgbmV0ZGV2X2FsbG9jX3NrYl9pcF9hbGlnbigpIGlzIHRoZSByaWdodCB0aGluZyBpbg0K
PiA+IHRoaXMgcmVnYXJkLg0KPiA+DQo+IA0KPiBPSywgdGltZSB0byBoYXZlIGEgbG9uZyBleHBs
YW5hdGlvbiA6DQo+IA0KPiBuZXRkZXZfYWxsb2Nfc2tiX2lwX2FsaWduKCkgaXMgZG9pbmcgdGhl
IHJpZ2h0IHRoaW5nLCBidXQgaXQgc2VlbXMgeW91DQo+IGd1eXMgaW5zaXN0IHRvIGludmVudCBh
IG5ldyBwcml2YXRlIHN0dWZmLg0KPiANCj4gSSBhbSBvbmx5IHdvbmRlcmluZyBpZiB5b3UgcmVh
bGx5IGtub3cgd2h5IHlvdSBkbyB0aGlzLg0KPiANCj4gTWFueSBkcml2ZXJzIGRvIGhhdmUgc2Ft
ZSByZXF1aXJlbWVudHMsIHNvIGV2ZXJ5IGRyaXZlciBzaG91bGQgcmVpbnZlbnQNCj4gdGhlIHdo
ZWVsID8gUmVhbGx5IHRoaXMgaXMgYmV5b25kIG1lLg0KPiANCj4gT3JpZ2luYWwgY29kZSB3YXMg
YWxpZ25pbmcgdGhlIGJ1ZmZlciBvbiBhIDMyIGJ5dGVzIGJvdW5kYXJ5IChiZWNhdXNlIG9mDQo+
IGEgaGFyZHdhcmUgcmVxdWlyZW1lbnQgb24gTklDLCBJIG9ubHkgdHJ1c3Qgb3JpZ2luYWwgY29k
ZSBvbiB0aGlzKS4NCj4gDQo+IFRoZW4geW91IHdhbnQgdG8gY2hhbmdlIHRoaXMgdG8gYWxpZ24g
YnVmZmVyIG9uIHRoaXMgMzIgYnl0ZXMgYm91bmRhcnkNCj4gUExVUyAyLiBXaGF0IGlzIHRoaXMg
a2luZCBvZiBuZXcgcmVxdWlyZW1lbnQgPw0KPiANCj4gMSkgSGFyZHdhcmUgcmVxdWlyZW1lbnQg
cmVhbGx5IGNoYW5nZWQgdGhhdCBtdWNoLiAoZmlybXdhcmUgY2hhbmdlZCBvbg0KPiBOSUMpLiBJ
ZiBub3QgdXNpbmcgdGhpcyBuZXcgYWxpZ25lbWVudCwgTklDIGRvZXNudCB3b3JrIGF0IGFsbC4N
Cj4gDQo+IDIpIG9yIE1pY3JvYmxhemUgYXJjaCByZXF1aXJlcyB0aGF0IElQIGhlYWRlciBpcyBh
bGlnbmVkIG9uIGEgd29yZA0KPiBib3VuZGFyeSB0byBhdm9pZCB1bmFsaWduZWQgdHJhcHMgaW4g
SVAgc3RhY2sgPyAobGlrZSBtYW55IGFyY2hlcykNCg0KWWVzLg0KDQo+IA0KPiBJZiB0aGlzIGlz
IHRoZSBsYXRlc3QgcmVxdWlyZW1lbnQsIHRoZW4gdXNlIHN0YW5kYXJkIG1lY2hhbmlzbS4NCj4g
c2tiIGRhdGEgaXMgbmF0dXJhbGx5IGFsaWduZWQgb24gTDFfQ0FDSEVfU0laRSArIFNLQl9QQUQg
Ym91bmRhcmllcy4NCj4gKDMyIGJ5dGVzIGFsaWdubWVudCkNCj4gDQo+IG5ldGRldl9hbGxvY19z
a2JfaXBfYWxpZ24oKSgpIHRoZW4gc2tpcHMgMiBieXRlcyB0byBtYWtlIHNrYiBkYXRhDQo+IGFs
aWduZWQgc28gdGhhdCAyICsgMTQgKHNpemVvZiBldGggaGVhZGVyKSA9IDE2IDogSVAgaGVhZGVy
IGlzIGFsaWduZWQNCj4gKG1vZHVsbyAxNikNCj4gDQo+IEl0IGp1c3Qgd29ya3MuIElmIG5vdCwg
d2Ugc2hvdWxkIGNvcnJlY3QgaXQsIHBsZWFzZSBmaWxsIGEgYnVnIHJlcG9ydC4NCj4gDQoNClRy
eWluZyBpdCBub3csIHRoYW5rcyBmb3IgdGhlIGhlbHAgYW5kIHBhdGllbmNlIDopDQoNCi0tIEpv
aG4NCg0KPiBUaGFua3MNCj4gDQo+IA0KDQoKVGhpcyBlbWFpbCBhbmQgYW55IGF0dGFjaG1lbnRz
IGFyZSBpbnRlbmRlZCBmb3IgdGhlIHNvbGUgdXNlIG9mIHRoZSBuYW1lZCByZWNpcGllbnQocykg
YW5kIGNvbnRhaW4ocykgY29uZmlkZW50aWFsIGluZm9ybWF0aW9uIHRoYXQgbWF5IGJlIHByb3By
aWV0YXJ5LCBwcml2aWxlZ2VkIG9yIGNvcHlyaWdodGVkIHVuZGVyIGFwcGxpY2FibGUgbGF3LiBJ
ZiB5b3UgYXJlIG5vdCB0aGUgaW50ZW5kZWQgcmVjaXBpZW50LCBkbyBub3QgcmVhZCwgY29weSwg
b3IgZm9yd2FyZCB0aGlzIGVtYWlsIG1lc3NhZ2Ugb3IgYW55IGF0dGFjaG1lbnRzLiBEZWxldGUg
dGhpcyBlbWFpbCBtZXNzYWdlIGFuZCBhbnkgYXR0YWNobWVudHMgaW1tZWRpYXRlbHkuCg==

Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 05 Apr 2010 23:29:53 +0200

> So, I ask, cant you use netdev_alloc_skb_ip_align() in this driver ?

Thanks to everyone for getting this patch into shape.

I applied version 4 of the patch to net-next-2.6, thanks!

RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[John Linn]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Tuesday, April 06, 2010 8:52 PM
> To: eric.dumazet@gmail.com
> Cc: John Linn; netdev@vger.kernel.org; linuxppc-dev@ozlabs.org;
grant.likely@secretlab.ca;
> jwboyer@linux.vnet.ibm.com; john.williams@petalogix.com;
michal.simek@petalogix.com; jtyner@cs.ucr.edu
> Subject: Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
> =

> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 05 Apr 2010 23:29:53 +0200
> 
> > So, I ask, cant you use netdev_alloc_skb_ip_align() in this driver ?
> 
> Thanks to everyone for getting this patch into shape.
> =

> I applied version 4 of the patch to net-next-2.6, thanks!

Thanks David, appreciate everyone's help and patience.

This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.

RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[Eric Dumazet]

Le mercredi 07 avril 2010 à 07:25 -0600, John Linn a écrit :
> > -----Original Message-----
> > From: David Miller [mailto:davem@davemloft.net]
> > Sent: Tuesday, April 06, 2010 8:52 PM
> > To: eric.dumazet@gmail.com
> > Cc: John Linn; netdev@vger.kernel.org; linuxppc-dev@ozlabs.org;
> grant.likely@secretlab.ca;
> > jwboyer@linux.vnet.ibm.com; john.williams@petalogix.com;
> michal.simek@petalogix.com; jtyner@cs.ucr.edu
> > Subject: Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
> > 
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Mon, 05 Apr 2010 23:29:53 +0200
> > 
> > > So, I ask, cant you use netdev_alloc_skb_ip_align() in this driver ?
> > 
> > Thanks to everyone for getting this patch into shape.
> > 
> > I applied version 4 of the patch to net-next-2.6, thanks!
> 
> Thanks David, appreciate everyone's help and patience.
> 

You're welcome ;)

Thanks

Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver

[Grant Likely]

On Mon, Apr 5, 2010 at 3:11 PM, John Linn <john.linn@xilinx.com> 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).
>
> The patch also updates the driver so that it runs on the MicroBlaze.
> The changes were tested on the PowerPC 440, PowerPC 405, and the
> MicroBlaze platforms.
>
> Signed-off-by: John Tyner <jtyner@cs.ucr.edu>
> Signed-off-by: John Linn <john.linn@xilinx.com>
>
> ---
>
> V2 - Incorporated comments from Grant and added more logic to allow the d=
river
> to work on MicroBlaze.
>
> V3 - Only updated it to apply to head, minor change to include slab.h. Al=
so
> verified that it now builds for MicroBlaze. Retested on PowerPC and Micro=
Blaze.
> ---
> =A0drivers/net/Kconfig =A0 =A0 =A0 =A0 | =A0 =A01 -
> =A0drivers/net/ll_temac.h =A0 =A0 =A0| =A0 17 +++++-
> =A0drivers/net/ll_temac_main.c | =A0124 +++++++++++++++++++++++++++++++++=
+---------
> =A03 files changed, 113 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 0ba5b8e..17044dc 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -2435,7 +2435,6 @@ config MV643XX_ETH
> =A0config XILINX_LL_TEMAC
> =A0 =A0 =A0 =A0tristate "Xilinx LL TEMAC (LocalLink Tri-mode Ethernet MAC=
) driver"
> =A0 =A0 =A0 =A0select PHYLIB
> - =A0 =A0 =A0 depends on PPC_DCR_NATIVE
> =A0 =A0 =A0 =A0help
> =A0 =A0 =A0 =A0 =A0This driver supports the Xilinx 10/100/1000 LocalLink =
TEMAC
> =A0 =A0 =A0 =A0 =A0core used in Xilinx Spartan and Virtex FPGAs


This still at the very least needs to depend on CONFIG_OF.  Otherwise
allmodconfig and allyesconfig on x86 and others will break.  The
driver also doesn't build on sparc, so you'll need to either exclude
CONFIG_SPARC or depend on (PPC || MICROBLAZE) too.

g.

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