[U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv

[U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv

[Simon Kagstrom]

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) {
-- 
1.6.0.4

[U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv

[Detlev Zundel]

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

-- 
X-Windows has to be the most expensive way ever of popping up an Emacs
window.
                                          -- The UNIX Haters Handbook
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv

[Scott Wood]

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

[U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv

[Detlev Zundel]

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

-- 
Programming X-Windows is like trying to find the square root of pi
using roman numerals.
                                          -- The UNIX Haters Handbook
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de 
> [mailto:u-boot-bounces at 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?

[U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv

[Simon Kagstrom]

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

[U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: Simon Kagstrom [mailto:simon.kagstrom at 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 :-):
> 
> 1. Make structure volatile in send as well
Hi Simon
I suggest to implement this change

Regards..
Prafulla . .

[U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv

[Detlev Zundel]

Hi Prafulla,

>> -----Original Message-----
>> From: Simon Kagstrom [mailto:simon.kagstrom at 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 :-):
>> 
>> 1. 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

-- 
Practice random senselessness and act kind of beautiful.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH] arm: Kirkwood: Add memory barriers to kwgbe_send/recv

[Wolfgang Denk]

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 :-):
> 
> 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!

There is a pretty clear policy in U-Boot now: please always use
accessor functions.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The shortest unit of time in the multiverse is the News York  Second,
defined  as  the  period  of  time between the traffic lights turning
green and the cab behind you honking.
                                - Terry Pratchett, _Lords and Ladies_