RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
-Original Message- From: David Miller [mailto:da...@davemloft.net] Sent: Tuesday, April 06, 2010 8:52 PM To: eric.duma...@gmail.com Cc: John Linn; net...@vger.kernel.org; linuxppc-...@ozlabs.org; grant.lik...@secretlab.ca; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@petalogix.com; jty...@cs.ucr.edu Subject: Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver From: Eric Dumazet eric.duma...@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 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
Le mercredi 07 avril 2010 à 07:25 -0600, John Linn a écrit : -Original Message- From: David Miller [mailto:da...@davemloft.net] Sent: Tuesday, April 06, 2010 8:52 PM To: eric.duma...@gmail.com Cc: John Linn; net...@vger.kernel.org; linuxppc-...@ozlabs.org; grant.lik...@secretlab.ca; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@petalogix.com; jty...@cs.ucr.edu Subject: Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver From: Eric Dumazet eric.duma...@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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
-Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] Sent: Monday, April 05, 2010 3:30 PM To: John Linn Cc: net...@vger.kernel.org; linuxppc-...@ozlabs.org; grant.lik...@secretlab.ca; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@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 jty...@cs.ucr.edu Signed-off-by: John Linn john.l...@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. How about this? #define BUFFER_ALIGN(adr) (((XTE_ALIGN + NET_IP_ALIGN) - ((u32) adr)) % XTE_ALIGN) So, I ask, cant you use netdev_alloc_skb_ip_align() in this driver ? From what I can tell, this wouldn't work as it only reserves the 2 bytes to align with a word boundary. Thanks, John 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
Le mardi 06 avril 2010 à 10:12 -0600, John Linn a écrit : -Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] Sent: Monday, April 05, 2010 3:30 PM To: John Linn Cc: net...@vger.kernel.org; linuxppc-...@ozlabs.org; grant.lik...@secretlab.ca; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@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 jty...@cs.ucr.edu Signed-off-by: John Linn john.l...@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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
-Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] Sent: Tuesday, April 06, 2010 11:01 AM To: John Linn Cc: net...@vger.kernel.org; linuxppc-...@ozlabs.org; grant.lik...@secretlab.ca; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@petalogix.com; John Tyner Subject: RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver Le mardi 06 avril 2010 à 10:12 -0600, John Linn a écrit : -Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] Sent: Monday, April 05, 2010 3:30 PM To: John Linn Cc: net...@vger.kernel.org; linuxppc-...@ozlabs.org; grant.lik...@secretlab.ca; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@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 jty...@cs.ucr.edu Signed-off-by: John Linn john.l...@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 ? 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. 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
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(), 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. */ ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
On Mon, Apr 5, 2010 at 3:33 PM, John Linn john.l...@xilinx.com wrote: From: Eric Dumazet [mailto:eric.duma...@gmail.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 ? Grant might have insight into why this started this way, I just updated to 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
On Tue, Apr 6, 2010 at 11:11 AM, John Linn john.l...@xilinx.com wrote: -Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] Sent: Tuesday, April 06, 2010 11:01 AM To: John Linn Cc: net...@vger.kernel.org; linuxppc-...@ozlabs.org; grant.lik...@secretlab.ca; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@petalogix.com; John Tyner Subject: RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver Le mardi 06 avril 2010 à 10:12 -0600, John Linn a écrit : -Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] Sent: Monday, April 05, 2010 3:30 PM To: John Linn Cc: net...@vger.kernel.org; linuxppc-...@ozlabs.org; grant.lik...@secretlab.ca; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@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 jty...@cs.ucr.edu Signed-off-by: John Linn john.l...@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. 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 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
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: /* * 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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
-Original Message- From: glik...@secretlab.ca [mailto:glik...@secretlab.ca] On Behalf Of Grant Likely Sent: Tuesday, April 06, 2010 12:54 PM To: John Linn Cc: Eric Dumazet; net...@vger.kernel.org; linuxppc-...@ozlabs.org; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@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.l...@xilinx.com wrote: -Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] Sent: Tuesday, April 06, 2010 11:01 AM To: John Linn Cc: net...@vger.kernel.org; linuxppc-...@ozlabs.org; grant.lik...@secretlab.ca; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@petalogix.com; John Tyner Subject: RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver Le mardi 06 avril 2010 à 10:12 -0600, John Linn a écrit : -Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] Sent: Monday, April 05, 2010 3:30 PM To: John Linn Cc: net...@vger.kernel.org; linuxppc-...@ozlabs.org; grant.lik...@secretlab.ca; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@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 jty...@cs.ucr.edu Signed-off-by: John Linn john.l...@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. 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 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. I can spin another patch with the following and with Grant's Kconfig changes, 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 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https
RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
-Original Message- From: Steven J. Magnani [mailto:st...@digidescorp.com] Sent: Tuesday, April 06, 2010 2:04 PM To: grant.lik...@secretlab.ca Cc: John Linn; Eric Dumazet; net...@vger.kernel.org; linuxppc-...@ozlabs.org; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@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 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
-Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] Sent: Tuesday, April 06, 2010 2:24 PM To: grant.lik...@secretlab.ca Cc: John Linn; net...@vger.kernel.org; linuxppc-...@ozlabs.org; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@petalogix.com; John Tyner Subject: Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver 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) Yes. 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. Trying it now, thanks for the help and patience :) -- John Thanks 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
From: Eric Dumazet eric.duma...@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! ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
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 jty...@cs.ucr.edu Signed-off-by: John Linn john.l...@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 ? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
-Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] Sent: Monday, April 05, 2010 3:30 PM To: John Linn Cc: net...@vger.kernel.org; linuxppc-...@ozlabs.org; grant.lik...@secretlab.ca; jwbo...@linux.vnet.ibm.com; john.willi...@petalogix.com; michal.si...@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 jty...@cs.ucr.edu Signed-off-by: John Linn john.l...@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 ? 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 ? Will have to look at it to better understand, maybe. Thanks, John 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] [V3] Add non-Virtex5 support for LL TEMAC driver
On Mon, Apr 5, 2010 at 3:11 PM, John Linn john.l...@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 jty...@cs.ucr.edu Signed-off-by: John Linn john.l...@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 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. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
