[U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv
Add memory barriers to kwgbe_send/recv
kwgbe_send/recv both have loops waiting for the hardware to set a bit. GCC 4.3.3 cleverly optimizes this to ... a while(1); loop. This patch introduces memory barriers to force re-loading of the transmit descriptor.
mb() wasn't defined for arm, but perhaps it should?
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net --- drivers/net/kirkwood_egiga.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c index 3c5db19..abedf77 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -513,6 +513,8 @@ static int kwgbe_send(struct eth_device *dev, volatile void *dataptr, printf("Err..(%s) in xmit packet\n", __FUNCTION__); return -1; } + /* Memory barrier to see to it that cmd_sts is re-read */ + asm volatile("" : : : "memory"); }; return 0; } @@ -531,6 +533,8 @@ static int kwgbe_recv(struct eth_device *dev) debug("%s time out...\n", __FUNCTION__); return -1; } + /* Memory barrier to see to it that cmd_sts is re-read */ + asm volatile("" : : : "memory"); } while (p_rxdesc_curr->cmd_sts & KWGBE_BUFFER_OWNED_BY_DMA);
if (p_rxdesc_curr->byte_cnt != 0) {
Hi Simon,
Add memory barriers to kwgbe_send/recv
kwgbe_send/recv both have loops waiting for the hardware to set a bit. GCC 4.3.3 cleverly optimizes this to ... a while(1); loop. This patch introduces memory barriers to force re-loading of the transmit descriptor.
mb() wasn't defined for arm, but perhaps it should?
You should rather use read/write accessor macros which "do the right thing". Try using readl() for the loop. Memory barriers do not belong into "upper level" code.
Cheers Detlev
Hi Detlev,
On Wednesday 01 July 2009 18:56:18 Detlev Zundel wrote:
Add memory barriers to kwgbe_send/recv
kwgbe_send/recv both have loops waiting for the hardware to set a bit. GCC 4.3.3 cleverly optimizes this to ... a while(1); loop. This patch introduces memory barriers to force re-loading of the transmit descriptor.
mb() wasn't defined for arm, but perhaps it should?
You should rather use read/write accessor macros which "do the right thing". Try using readl() for the loop. Memory barriers do not belong into "upper level" code.
I know that we advertised the usage of the io accessor functions for quite some time now. But I'm not so sure here anymore. One disadvantage of this usage could be speed penalty by the usage of too many unnecessary barriers (included via the accessor functions on some platforms).
I remember a discussion either on the linuxppc-dev, linux-arm-kernel or the nextdev list about removing the accessor functions from the Linux version of this Marvell ethernet drivers (mv643xx_eth.c) at some time because of this speed penalty. Sorry but I don't have a link for this mlist discussion.
Just my 0.02$.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
Stefan Roese wrote:
I know that we advertised the usage of the io accessor functions for quite some time now. But I'm not so sure here anymore. One disadvantage of this usage could be speed penalty by the usage of too many unnecessary barriers (included via the accessor functions on some platforms).
Hot paths (or tiny bootstraps) can be optimized if needed, but why invite screwups for the majority of code?
-Scott
Hi Stefan & Scott,
Stefan Roese wrote:
I know that we advertised the usage of the io accessor functions for quite some time now. But I'm not so sure here anymore. One disadvantage of this usage could be speed penalty by the usage of too many unnecessary barriers (included via the accessor functions on some platforms).
Hot paths (or tiny bootstraps) can be optimized if needed, but why invite screwups for the majority of code?
I fully agree with Scott here. Experience has shown that robustness pays off much more than premature optimizations. If you happen to have performance problems, then I also seriously advise not to optimize without actual performance data collected by a profiling run.
This will not be easy for U-Boot but still I've seen so many examples of effort put into "optimizations" which in the end turned out not to be relevant for the total performance at all.
Cheers Detlev
-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Simon Kagstrom Sent: Wednesday, July 01, 2009 8:46 PM To: U-Boot ML Subject: [U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv
Add memory barriers to kwgbe_send/recv
kwgbe_send/recv both have loops waiting for the hardware to set a bit. GCC 4.3.3 cleverly optimizes this to ... a while(1); loop.
Struct used is defined as volatile here. Is this not sufficient?
Regards.. Prafulla . .
This patch introduces memory barriers to force re-loading of the transmit descriptor.
mb() wasn't defined for arm, but perhaps it should?
On Wed, 1 Jul 2009 09:57:10 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
kwgbe_send/recv both have loops waiting for the hardware to set a bit. GCC 4.3.3 cleverly optimizes this to ... a while(1); loop.
Struct used is defined as volatile here. Is this not sufficient?
Right, but that was only for recv - send isn't volatile. I'll send a new patch with the required changes. The question is which :-):
1. Make structure volatile in send as well
2. Use readl() where needed
3. Keep the memory barrier approach (and maybe add a mb() macro to arm as well)
I'm happy to get suggestions!
I'll prepare the openrd_base board patches a bit more and send them to the list later today.
// Simon
-----Original Message----- From: Simon Kagstrom [mailto:simon.kagstrom@netinsight.net] Sent: Thursday, July 02, 2009 12:17 PM To: Prafulla Wadaskar Cc: U-Boot ML Subject: Re: [U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv
On Wed, 1 Jul 2009 09:57:10 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
kwgbe_send/recv both have loops waiting for the hardware
to set a
bit. GCC 4.3.3 cleverly optimizes this to ... a while(1); loop.
Struct used is defined as volatile here. Is this not sufficient?
Right, but that was only for recv - send isn't volatile. I'll send a new patch with the required changes. The question is which :-):
- Make structure volatile in send as well
Hi Simon I suggest to implement this change
Regards.. Prafulla . .
Hi Prafulla,
-----Original Message----- From: Simon Kagstrom [mailto:simon.kagstrom@netinsight.net] Sent: Thursday, July 02, 2009 12:17 PM To: Prafulla Wadaskar Cc: U-Boot ML Subject: Re: [U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv
On Wed, 1 Jul 2009 09:57:10 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
kwgbe_send/recv both have loops waiting for the hardware
to set a
bit. GCC 4.3.3 cleverly optimizes this to ... a while(1); loop.
Struct used is defined as volatile here. Is this not sufficient?
Right, but that was only for recv - send isn't volatile. I'll send a new patch with the required changes. The question is which :-):
- Make structure volatile in send as well
Hi Simon I suggest to implement this change
Did you read Documentation/volatile-considered-harmful.txt in a Linux source tree?
Cheers Detlev
Dear Simon Kagstrom,
In message 20090702084702.3e4ecae4@marrow.netinsight.se you wrote:
On Wed, 1 Jul 2009 09:57:10 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
kwgbe_send/recv both have loops waiting for the hardware to set a bit. GCC 4.3.3 cleverly optimizes this to ... a while(1); loop.
Struct used is defined as volatile here. Is this not sufficient?
Right, but that was only for recv - send isn't volatile. I'll send a new patch with the required changes. The question is which :-):
Make structure volatile in send as well
Use readl() where needed
Keep the memory barrier approach (and maybe add a mb() macro to arm as well)
I'm happy to get suggestions!
There is a pretty clear policy in U-Boot now: please always use accessor functions.
Best regards,
Wolfgang Denk
participants (6)
-
Detlev Zundel -
Prafulla Wadaskar -
Scott Wood -
Simon Kagstrom -
Stefan Roese -
Wolfgang Denk