Re: [OpenWrt-Devel] [PATCH] bgmac - add support for unaligned addressing
On Thursday 28 February 2013 20:06:49 Hauke Mehrtens wrote: Hi, I haven't tested your patch I have just some comments. In general it looks good to me. Would be nice if it could be tested for both types.. I'm pretty sure by now it's working okay for unaligned addressing, but have only one device to test on. On 02/25/2013 10:18 PM, Tijs Van Buggenhout wrote: Signed-off-by: Tijs Van Buggenhout (t...@able.be) Your whole patch is malformed, there are extra line breaks and tabs were eaten by your mailer. It's a cpp from a cat'ed diff on console. Email client does a line wrap after 80 characters in plain text. Will try as attachment next time... Read this http://kerneltrap.org/Linux/Email_Clients_and_Patches to configure your mail client to not mangle the patches. You could also try it for you own, just send he patch to your self and try to apply it. Sorry again.. What do you think about the following patch? Only one change in bgmac_ring struct, a boolean named dma_unaligned. An u32 variable index_base is locally defined in functions where needed, assigned to addr lo of dma_base in case of unaligned addr, otherwise 0. --- a/drivers/net/ethernet/broadcom/bgmac.h 2013-02-20 12:41:03.138480108 +0100 +++ b/drivers/net/ethernet/broadcom/bgmac.h 2013-02-25 21:03:29.110125298 +0100 @@ -384,6 +384,7 @@ u16 mmio_base; struct bgmac_dma_desc *cpu_base; dma_addr_t dma_base; + bool dma_unaligned; struct bgmac_slot_info slots[BGMAC_RX_RING_SLOTS]; }; --- a/drivers/net/ethernet/broadcom/bgmac.c 2013-02-25 22:10:14.744943929 +0100 +++ b/drivers/net/ethernet/broadcom/bgmac.c 2013-02-25 20:59:39.697996832 +0100 @@ -108,6 +108,7 @@ struct net_device *net_dev = bgmac-net_dev; struct bgmac_dma_desc *dma_desc; struct bgmac_slot_info *slot; + u32 index_base = ( ring-dma_unaligned ? lower_32_bits(ring-dma_base) : 0 ); u32 ctl0, ctl1; int free_slots; @@ -156,7 +157,7 @@ if (++ring-end = BGMAC_TX_RING_SLOTS) ring-end = 0; bgmac_write(bgmac, ring-mmio_base + BGMAC_DMA_TX_INDEX, - ring-end * sizeof(struct bgmac_dma_desc)); + ( index_base + ( ring-end * sizeof(struct bgmac_dma_desc) ) )); /* Always keep one slot free to allow detecting bugged calls. */ if (--free_slots == 1) @@ -174,14 +175,25 @@ static void bgmac_dma_tx_free(struct bgmac *bgmac, struct bgmac_dma_ring *ring) { struct device *dma_dev = bgmac-core-dma_dev; - int empty_slot; + u16 empty_slot; + u32 index_base = ( ring-dma_unaligned ? lower_32_bits(ring-dma_base) : 0 ); bool freed = false; + if (ring-start == ring-end) + return; + Is this an unrelated change? Not entirely related.. would be an optimisation, but when I come to think of it, it's probably more like a rare case. I'll have a look whether the hardware can deal with this case.. /* The last slot that hardware didn't consume yet */ - empty_slot = bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS); - empty_slot = BGMAC_DMA_TX_STATDPTR; + empty_slot = (u16) ( ( bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS) - + index_base ) BGMAC_DMA_TX_STATDPTR ); I like the following more: empty_slot = (u16)(bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS) - index_base); empty_slot = BGMAC_DMA_TX_STATDPTR; empty_slot /= sizeof(struct bgmac_dma_desc); + if (((ring-start == 0) (empty_slot ring-end)) || + (empty_slot = ring-num_slots)) { + bgmac_err(bgmac, Bogus current TX slot index %u (start index: %u, end index: %u)\n, + empty_slot, ring-start, ring-end); + return; + } + Is this an unrelated change? This is related if you consider the bgmac bcma0:1: Hardware reported transmission for empty TX ring slot 0! End of ring: 0 ams messages. They only appeared because the while loop (see underneath) would never end, because the condition (ring-start != empty_slot) would always be true when empty_slot = ring-num_slots. The body of the while loop doesn't allow ring- start to become any higher than ring-num_slots, resetting it to zero when it does, turning it into an infinite loop. You can see this as a protection for the software driver in case the hardware fails (and returns a faulty slot id (== not within range)). In that regard I've been thinking to set a BUG_ON instead of the error report.. in case of unaligned addressing, the hardware could not recover from this (without being reset/reinitialised). BUG_ON(empty_slot = ring-num_slots); while (ring-start !=
Re: [OpenWrt-Devel] [PATCH] bgmac - add support for unaligned addressing
On Friday 01 March 2013 10:56:33 Tijs Van Buggenhout wrote: On Thursday 28 February 2013 20:06:49 Hauke Mehrtens wrote: Hi, I haven't tested your patch I have just some comments. In general it looks good to me. Would be nice if it could be tested for both types.. I'm pretty sure by now it's working okay for unaligned addressing, but have only one device to test on. On 02/25/2013 10:18 PM, Tijs Van Buggenhout wrote: Signed-off-by: Tijs Van Buggenhout (t...@able.be) Your whole patch is malformed, there are extra line breaks and tabs were eaten by your mailer. It's a cpp from a cat'ed diff on console. Email client does a line wrap after 80 characters in plain text. Will try as attachment next time... Read this http://kerneltrap.org/Linux/Email_Clients_and_Patches to configure your mail client to not mangle the patches. You could also try it for you own, just send he patch to your self and try to apply it. Sorry again.. What do you think about the following patch? Only one change in bgmac_ring struct, a boolean named dma_unaligned. An u32 variable index_base is locally defined in functions where needed, assigned to addr lo of dma_base in case of unaligned addr, otherwise 0. --- a/drivers/net/ethernet/broadcom/bgmac.h 2013-02-20 12:41:03.138480108 +0100 +++ b/drivers/net/ethernet/broadcom/bgmac.h 2013-02-25 21:03:29.110125298 +0100 @@ -384,6 +384,7 @@ u16 mmio_base; struct bgmac_dma_desc *cpu_base; dma_addr_t dma_base; + bool dma_unaligned; struct bgmac_slot_info slots[BGMAC_RX_RING_SLOTS]; }; --- a/drivers/net/ethernet/broadcom/bgmac.c 2013-02-25 22:10:14.744943929 +0100 +++ b/drivers/net/ethernet/broadcom/bgmac.c 2013-02-25 20:59:39.697996832 +0100 @@ -108,6 +108,7 @@ struct net_device *net_dev = bgmac-net_dev; struct bgmac_dma_desc *dma_desc; struct bgmac_slot_info *slot; + u32 index_base = ( ring-dma_unaligned ? lower_32_bits(ring-dma_base) : 0 ); u32 ctl0, ctl1; int free_slots; @@ -156,7 +157,7 @@ if (++ring-end = BGMAC_TX_RING_SLOTS) ring-end = 0; bgmac_write(bgmac, ring-mmio_base + BGMAC_DMA_TX_INDEX, - ring-end * sizeof(struct bgmac_dma_desc)); + ( index_base + ( ring-end * sizeof(struct bgmac_dma_desc) ) )); /* Always keep one slot free to allow detecting bugged calls. */ if (--free_slots == 1) @@ -174,14 +175,25 @@ static void bgmac_dma_tx_free(struct bgmac *bgmac, struct bgmac_dma_ring *ring) { struct device *dma_dev = bgmac-core-dma_dev; - int empty_slot; + u16 empty_slot; + u32 index_base = ( ring-dma_unaligned ? lower_32_bits(ring-dma_base) : 0 ); bool freed = false; + if (ring-start == ring-end) + return; + Is this an unrelated change? Not entirely related.. would be an optimisation, but when I come to think of it, it's probably more like a rare case. I'll have a look whether the hardware can deal with this case.. This is not needed .. hardware can deal with it. /* The last slot that hardware didn't consume yet */ - empty_slot = bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS); - empty_slot = BGMAC_DMA_TX_STATDPTR; + empty_slot = (u16) ( ( bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS) - + index_base ) BGMAC_DMA_TX_STATDPTR ); I like the following more: empty_slot = (u16)(bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS) - index_base); empty_slot = BGMAC_DMA_TX_STATDPTR; Used this one. empty_slot /= sizeof(struct bgmac_dma_desc); + if (((ring-start == 0) (empty_slot ring-end)) || + (empty_slot = ring-num_slots)) { + bgmac_err(bgmac, Bogus current TX slot index %u (start index: %u, end index: %u)\n, + empty_slot, ring-start, ring-end); + return; + } + Is this an unrelated change? This is related if you consider the bgmac bcma0:1: Hardware reported transmission for empty TX ring slot 0! End of ring: 0 ams messages. They only appeared because the while loop (see underneath) would never end, because the condition (ring-start != empty_slot) would always be true when empty_slot = ring-num_slots. The body of the while loop doesn't allow ring- start to become any higher than ring-num_slots, resetting it to zero when it does, turning it into an infinite loop. You can see this as a protection for the software driver in case the hardware fails (and returns a faulty slot id (== not within
Re: [OpenWrt-Devel] [PATCH] bgmac - add support for unaligned addressing
Hi, I haven't tested your patch I have just some comments. In general it looks good to me. On 02/25/2013 10:18 PM, Tijs Van Buggenhout wrote: Signed-off-by: Tijs Van Buggenhout (t...@able.be) Your whole patch is malformed, there are extra line breaks and tabs were eaten by your mailer. It's a cpp from a cat'ed diff on console. Email client does a line wrap after 80 characters in plain text. Will try as attachment next time... Read this http://kerneltrap.org/Linux/Email_Clients_and_Patches to configure your mail client to not mangle the patches. You could also try it for you own, just send he patch to your self and try to apply it. What do you think about the following patch? Only one change in bgmac_ring struct, a boolean named dma_unaligned. An u32 variable index_base is locally defined in functions where needed, assigned to addr lo of dma_base in case of unaligned addr, otherwise 0. --- a/drivers/net/ethernet/broadcom/bgmac.h 2013-02-20 12:41:03.138480108 +0100 +++ b/drivers/net/ethernet/broadcom/bgmac.h 2013-02-25 21:03:29.110125298 +0100 @@ -384,6 +384,7 @@ u16 mmio_base; struct bgmac_dma_desc *cpu_base; dma_addr_t dma_base; + bool dma_unaligned; struct bgmac_slot_info slots[BGMAC_RX_RING_SLOTS]; }; --- a/drivers/net/ethernet/broadcom/bgmac.c 2013-02-25 22:10:14.744943929 +0100 +++ b/drivers/net/ethernet/broadcom/bgmac.c 2013-02-25 20:59:39.697996832 +0100 @@ -108,6 +108,7 @@ struct net_device *net_dev = bgmac-net_dev; struct bgmac_dma_desc *dma_desc; struct bgmac_slot_info *slot; + u32 index_base = ( ring-dma_unaligned ? lower_32_bits(ring-dma_base) : 0 ); u32 ctl0, ctl1; int free_slots; @@ -156,7 +157,7 @@ if (++ring-end = BGMAC_TX_RING_SLOTS) ring-end = 0; bgmac_write(bgmac, ring-mmio_base + BGMAC_DMA_TX_INDEX, - ring-end * sizeof(struct bgmac_dma_desc)); + ( index_base + ( ring-end * sizeof(struct bgmac_dma_desc) ) )); /* Always keep one slot free to allow detecting bugged calls. */ if (--free_slots == 1) @@ -174,14 +175,25 @@ static void bgmac_dma_tx_free(struct bgmac *bgmac, struct bgmac_dma_ring *ring) { struct device *dma_dev = bgmac-core-dma_dev; - int empty_slot; + u16 empty_slot; + u32 index_base = ( ring-dma_unaligned ? lower_32_bits(ring-dma_base) : 0 ); bool freed = false; + if (ring-start == ring-end) + return; + Is this an unrelated change? /* The last slot that hardware didn't consume yet */ - empty_slot = bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS); - empty_slot = BGMAC_DMA_TX_STATDPTR; + empty_slot = (u16) ( ( bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS) - + index_base ) BGMAC_DMA_TX_STATDPTR ); I like the following more: empty_slot = (u16)(bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS) - index_base); empty_slot = BGMAC_DMA_TX_STATDPTR; empty_slot /= sizeof(struct bgmac_dma_desc); + if (((ring-start == 0) (empty_slot ring-end)) || + (empty_slot = ring-num_slots)) { + bgmac_err(bgmac, Bogus current TX slot index %u (start index: %u, end index: %u)\n, + empty_slot, ring-start, ring-end); + return; + } + Is this an unrelated change? while (ring-start != empty_slot) { struct bgmac_slot_info *slot = ring-slots[ring-start]; @@ -195,7 +207,7 @@ dev_kfree_skb(slot-skb); slot-skb = NULL; } else { - bgmac_err(bgmac, Hardware reported transmission for empty TX ring slot %d! End of ring: %d\n, + bgmac_err(bgmac, Hardware reported transmission for empty TX ring slot %u! End of ring: %u\n, ring-start, ring-end); } @@ -270,11 +282,12 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, int weight) { - u32 end_slot; + u16 end_slot; + u32 index_base = ( ring-dma_unaligned ? lower_32_bits(ring-dma_base) : 0 ); int handled = 0; - end_slot = bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_RX_STATUS); - end_slot = BGMAC_DMA_RX_STATDPTR; + end_slot = (u16) ( ( bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_RX_STATUS) - + index_base ) BGMAC_DMA_RX_STATDPTR ); end_slot /= sizeof(struct bgmac_dma_desc); see above ring-end = end_slot; @@ -298,7 +311,7 @@ /* Check for poison and drop or pass the packet */ if (len == 0xdead flags == 0xbeef) { -
Re: [OpenWrt-Devel] [PATCH] bgmac - add support for unaligned addressing
2013/2/25 Tijs Van Buggenhout t...@able.be: I opted to introduce a CONFIG_BGMAC_UNALIGNED_ADDRESSING macro to introduce the changes to support unaligned addressing for the bgmac driver (it does add some extra bytes in bgmac_dma_ring struct). This could be an option in Kconfig (not yet in patch), but for now it is defined fix in bgmac.h. The implementation should work for both aligned and unaligned addressing when CONFIG_BGMAC_UNALIGNED_ADDRESSING is defined. I don't think we should need CONFIG_BGMAC_UNALIGNED_ADDRESSING, it doesn't save a lot of size (when disabled) and makes configuration more confusing. If there is no need for en extra Kconfig option, let me know, I'll adjust the patch. In the other case, I'll create an extra entry in Kconfig and add it to the patch aswell. I created the patch from 3.6 kernel sources, let me know if there are differences for the 3.8 kernel. Kernel 3.6 doesn't include bgmac at all ;) I hope this can be tested on different hardware supporting dma aligned/unaligned addressing, it should work on both. Any comments/suggestions are welcome.. I'll give it a try on my BCM4706s, just give me some time. Note: Besides the changes for unaligned addressing, I corrected some types for variables and formats. I also introduced an extra int j variable in the second loop within dma initialisation of rx ring(s).. from my understanding reusing the int i variable in the inner loop breaks the outer loop if there would be more than one rx ring. That definitely should go in separated patches. Signed-off-by: Tijs Van Buggenhout (t...@able.be) Your whole patch is malformed, there are extra line breaks and tabs were eaten by your mailer. @@ -384,6 +388,10 @@ u16 mmio_base; struct bgmac_dma_desc *cpu_base; dma_addr_t dma_base; +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING + bool is_unaligned; + u32 index_base; I'm pretty sure it's some duplication, we already store address somewhere else. @@ -156,6 +156,9 @@ if (++ring-end = BGMAC_TX_RING_SLOTS) ring-end = 0; bgmac_write(bgmac, ring-mmio_base + BGMAC_DMA_TX_INDEX, +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING + ring-index_base + +#endif Note: If we de-duplicate index_base we have to add that conditionally. + if (ring-start == ring-end) { + bgmac_warn(bgmac, Ignore DMA TX free on empty ring 0x%X\n, ring-mmio_base); + return; + } + Is that supposed to happen? I've to analyze that part :) /* The last slot that hardware didn't consume yet */ empty_slot = bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS); empty_slot = BGMAC_DMA_TX_STATDPTR; +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING + empty_slot -= ring-index_base; + empty_slot = BGMAC_DMA_TX_STATDPTR; +#endif empty_slot /= sizeof(struct bgmac_dma_desc); Do we need to mask that twice? + if (((ring-start == 0) (empty_slot ring-end)) || + (empty_slot = ring-num_slots)) { + bgmac_err(bgmac, Bogus current TX slot index %u (start index: %u, end index: %u)\n, + empty_slot, ring-start, ring-end); + return; + } Looks OK to add the check, I just wonder: did you actually hit that? Maybe we setup something incorrectly? @@ -416,9 +439,15 @@ ring = bgmac-tx_ring[i]; ring-num_slots = BGMAC_TX_RING_SLOTS; ring-mmio_base = ring_base[i]; +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING + if ((ring-is_unaligned = bgmac_dma_unaligned(bgmac, ring, BGMAC_DMA_RING_TX))) + bgmac_warn(bgmac, TX on ring 0x%X supports unaligned addressing\n, + ring-mmio_base); Warn? ;) +#else if (bgmac_dma_unaligned(bgmac, ring, BGMAC_DMA_RING_TX)) bgmac_warn(bgmac, TX on ring 0x%X supports unaligned addressing but this feature is not implemented\n, ring-mmio_base); There should be info like not enabled or sth, but it'll go anyway when removing CONFIG_BGMAC_UNALIGNED_ADDRESSING -- Rafał ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH] bgmac - add support for unaligned addressing
On Monday 25 February 2013 18:51:53 Rafał Miłecki wrote: 2013/2/25 Tijs Van Buggenhout t...@able.be: I opted to introduce a CONFIG_BGMAC_UNALIGNED_ADDRESSING macro to introduce the changes to support unaligned addressing for the bgmac driver (it does add some extra bytes in bgmac_dma_ring struct). This could be an option in Kconfig (not yet in patch), but for now it is defined fix in bgmac.h. The implementation should work for both aligned and unaligned addressing when CONFIG_BGMAC_UNALIGNED_ADDRESSING is defined. I don't think we should need CONFIG_BGMAC_UNALIGNED_ADDRESSING, it doesn't save a lot of size (when disabled) and makes configuration more confusing. I'll get rid of the defines, and create another patch ;-) If there is no need for en extra Kconfig option, let me know, I'll adjust the patch. In the other case, I'll create an extra entry in Kconfig and add it to the patch aswell. I created the patch from 3.6 kernel sources, let me know if there are differences for the 3.8 kernel. Kernel 3.6 doesn't include bgmac at all ;) Building from openwrt trunk, which configures a 3.6.11 kernel by default, where bgmac is patched in? I hope this can be tested on different hardware supporting dma aligned/unaligned addressing, it should work on both. Any comments/suggestions are welcome.. I'll give it a try on my BCM4706s, just give me some time. No problem, take your time ;-) Note: Besides the changes for unaligned addressing, I corrected some types for variables and formats. I also introduced an extra int j variable in the second loop within dma initialisation of rx ring(s).. from my understanding reusing the int i variable in the inner loop breaks the outer loop if there would be more than one rx ring. That definitely should go in separated patches. I'll separate them into different patches. The loop variable fix is already settled Signed-off-by: Tijs Van Buggenhout (t...@able.be) Your whole patch is malformed, there are extra line breaks and tabs were eaten by your mailer. It's a cpp from a cat'ed diff on console. Email client does a line wrap after 80 characters in plain text. Will try as attachment next time... @@ -384,6 +388,10 @@ u16 mmio_base; struct bgmac_dma_desc *cpu_base; dma_addr_t dma_base; +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING + bool is_unaligned; + u32 index_base; I'm pretty sure it's some duplication, we already store address somewhere else. Correct, but has implicit 0 value, when it is not assigned. Avoiding if conditions in the rest of the code. @@ -156,6 +156,9 @@ if (++ring-end = BGMAC_TX_RING_SLOTS) ring-end = 0; bgmac_write(bgmac, ring-mmio_base + BGMAC_DMA_TX_INDEX, +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING + ring-index_base + +#endif Note: If we de-duplicate index_base we have to add that conditionally. Indeed .. we could go for the bool is_unaligned, and write something like '((is_unaligned ? lower_32_bits( ring-dma_base ) : 0 ) + ' or would you prefer a normal if statement. We could also go for a local variable index_base in the corresponding functions, which is initialised to 0 or set to the correct value in case of unalignment. + if (ring-start == ring-end) { + bgmac_warn(bgmac, Ignore DMA TX free on empty ring 0x%X\n, ring-mmio_base); + return; + } + Is that supposed to happen? I've to analyze that part :) We can leave it out.. it's an optimisation (BGMAC_DMA_TX_STATUS doesn't need to be read). I saw the warning appear a few times on my screen today, so the message print is better left out anyways. /* The last slot that hardware didn't consume yet */ empty_slot = bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS); empty_slot = BGMAC_DMA_TX_STATDPTR; +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING + empty_slot -= ring-index_base; + empty_slot = BGMAC_DMA_TX_STATDPTR; +#endif empty_slot /= sizeof(struct bgmac_dma_desc); Do we need to mask that twice? I kept as close to the broadcom sources as possible. Keeping in mind that empty_slot is an u16, bgmac_read returns an u32 as is index_base, it's probably best (for clarity) to write: empty_slot = ( bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS) BGMAC_DMA_TX_STATDPTR ); empty_slot -= ( ring-index_base BGMAC_DMA_TX_STATDPTR ) but empty_slot = (( bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS) - ring-index_base ) BGMAC_DMA_TX_STATDPTR ); can avoid the extra mask. Should I include an extra type cast (u16)? + if (((ring-start == 0) (empty_slot ring-end)) || + (empty_slot = ring-num_slots)) { + bgmac_err(bgmac, Bogus current TX slot index %u (start index: %u, end index: %u)\n, +
Re: [OpenWrt-Devel] [PATCH] bgmac - add support for unaligned addressing
On Monday 25 February 2013 20:35:23 Tijs Van Buggenhout wrote: On Monday 25 February 2013 18:51:53 Rafał Miłecki wrote: 2013/2/25 Tijs Van Buggenhout t...@able.be: I opted to introduce a CONFIG_BGMAC_UNALIGNED_ADDRESSING macro to introduce the changes to support unaligned addressing for the bgmac driver (it does add some extra bytes in bgmac_dma_ring struct). This could be an option in Kconfig (not yet in patch), but for now it is defined fix in bgmac.h. The implementation should work for both aligned and unaligned addressing when CONFIG_BGMAC_UNALIGNED_ADDRESSING is defined. I don't think we should need CONFIG_BGMAC_UNALIGNED_ADDRESSING, it doesn't save a lot of size (when disabled) and makes configuration more confusing. I'll get rid of the defines, and create another patch ;-) If there is no need for en extra Kconfig option, let me know, I'll adjust the patch. In the other case, I'll create an extra entry in Kconfig and add it to the patch aswell. I created the patch from 3.6 kernel sources, let me know if there are differences for the 3.8 kernel. Kernel 3.6 doesn't include bgmac at all ;) Building from openwrt trunk, which configures a 3.6.11 kernel by default, where bgmac is patched in? I hope this can be tested on different hardware supporting dma aligned/unaligned addressing, it should work on both. Any comments/suggestions are welcome.. I'll give it a try on my BCM4706s, just give me some time. No problem, take your time ;-) Note: Besides the changes for unaligned addressing, I corrected some types for variables and formats. I also introduced an extra int j variable in the second loop within dma initialisation of rx ring(s).. from my understanding reusing the int i variable in the inner loop breaks the outer loop if there would be more than one rx ring. That definitely should go in separated patches. I'll separate them into different patches. The loop variable fix is already settled Signed-off-by: Tijs Van Buggenhout (t...@able.be) Your whole patch is malformed, there are extra line breaks and tabs were eaten by your mailer. It's a cpp from a cat'ed diff on console. Email client does a line wrap after 80 characters in plain text. Will try as attachment next time... @@ -384,6 +388,10 @@ u16 mmio_base; struct bgmac_dma_desc *cpu_base; dma_addr_t dma_base; +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING + bool is_unaligned; + u32 index_base; I'm pretty sure it's some duplication, we already store address somewhere else. Correct, but has implicit 0 value, when it is not assigned. Avoiding if conditions in the rest of the code. @@ -156,6 +156,9 @@ if (++ring-end = BGMAC_TX_RING_SLOTS) ring-end = 0; bgmac_write(bgmac, ring-mmio_base + BGMAC_DMA_TX_INDEX, +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING + ring-index_base + +#endif Note: If we de-duplicate index_base we have to add that conditionally. Indeed .. we could go for the bool is_unaligned, and write something like '((is_unaligned ? lower_32_bits( ring-dma_base ) : 0 ) + ' or would you prefer a normal if statement. We could also go for a local variable index_base in the corresponding functions, which is initialised to 0 or set to the correct value in case of unalignment. + if (ring-start == ring-end) { + bgmac_warn(bgmac, Ignore DMA TX free on empty ring 0x%X\n, ring-mmio_base); + return; + } + Is that supposed to happen? I've to analyze that part :) We can leave it out.. it's an optimisation (BGMAC_DMA_TX_STATUS doesn't need to be read). I saw the warning appear a few times on my screen today, so the message print is better left out anyways. /* The last slot that hardware didn't consume yet */ empty_slot = bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS); empty_slot = BGMAC_DMA_TX_STATDPTR; +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING + empty_slot -= ring-index_base; + empty_slot = BGMAC_DMA_TX_STATDPTR; +#endif empty_slot /= sizeof(struct bgmac_dma_desc); Do we need to mask that twice? I kept as close to the broadcom sources as possible. Keeping in mind that empty_slot is an u16, bgmac_read returns an u32 as is index_base, it's probably best (for clarity) to write: empty_slot = ( bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS) BGMAC_DMA_TX_STATDPTR ); empty_slot -= ( ring-index_base BGMAC_DMA_TX_STATDPTR ) but empty_slot = (( bgmac_read(bgmac, ring-mmio_base + BGMAC_DMA_TX_STATUS) - ring-index_base ) BGMAC_DMA_TX_STATDPTR ); can avoid the extra mask. Should I include an extra type cast (u16)? + if (((ring-start == 0)