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) {
-
[OpenWrt-Devel] [PATCH] bgmac - add support for unaligned addressing
This is a proposal for target/linux/brcm47xx/patches-3.6/770-bgmac-unaligned-
addressing.patch, which adds support for DMA unaligned addressing in bgmac
driver (needed for e.g. linksys e3200 devices).
From analysing the 'Generic Broadcom Home Networking Division (HND) DMA
module' (hnddma.{c,h}), the DMA index register (BGMAC_DMA_TX_INDEX or
BGMAC_DMA_RX_INDEX) needs an extra base or offset from address register. This
offset needs to be accounted for when reading/analysing status
(BGMAC_DMA_TX_STATUS or BGMAC_DMA_RX_STATUS) to retrieve the current (empty)
slot id (BGMAC_DMA_TX_STATDPTR or BGMAC_DMA_RX_STATDPTR) in the ring.
Also for retrieving the active slot id (BGMAC_DMA_TX_ERRDPTR or
BGMAC_DMA_RX_STATDPTR) from BGMAC_DMA_TX_ERROR or BGMAC_DMA_RX_ERROR register
respectively, this would be needed, but currently not used in driver.
The patch adds two extra fields to the bgmac_dma_ring structure, namely a
boolean 'is_unaligned' which is set in bgmac_dma_alloc (and later used in
bgmac_dma_init), and an unsigned int of 32 bits, holding the base for the
index register for that ring.
For unaligned addressing (in bgmac_dma_init), the DMA table is first
initialised, before enabling any dma tx/rx control, which is different from
aligned addressing where it is the other way around.
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.
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.
I hope this can be tested on different hardware supporting dma
aligned/unaligned addressing, it should work on both. Any comments/suggestions
are welcome..
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.
Signed-off-by: Tijs Van Buggenhout (t...@able.be)
---
--- 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 13:22:06.187475387
+0100
@@ -15,6 +15,10 @@
#include linux/bcma/bcma.h
#include linux/netdevice.h
+#ifndef CONFIG_BGMAC_UNALIGNED_ADDRESSING
+#define CONFIG_BGMAC_UNALIGNED_ADDRESSING
+#endif
+
#define BGMAC_DEV_CTL 0x000
#define BGMAC_DC_TSM 0x0002
#define BGMAC_DC_CFCO 0x0004
@@ -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;
+#endif
struct bgmac_slot_info slots[BGMAC_RX_RING_SLOTS];
};
--- a/drivers/net/ethernet/broadcom/bgmac.c 2013-02-20 12:41:03.122481212
+0100
+++ b/drivers/net/ethernet/broadcom/bgmac.c 2013-02-25 13:31:34.880174836
+0100
@@ -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
ring-end * sizeof(struct bgmac_dma_desc));
/* Always keep one slot free to allow detecting bugged calls. */
@@ -174,14 +177,30 @@
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;
bool freed = false;
+ if (ring-start == ring-end) {
+ bgmac_warn(bgmac, Ignore DMA TX free on empty ring 0x%X\n,
ring-mmio_base);
+ return;
+ }
+
/* 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);
+ 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;
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)
