Re: [U-Boot] u-boot <=> kernel compatibility?

[Chris Moore]

Hi,

Le 08/11/2017 à 09:11, daggs a écrit :

Greetings Tom,


Sent: Saturday, November 04, 2017 at 8:37 PM
From: "Tom Rini" 
To: daggs 
Cc: u-boot@lists.denx.de
Subject: Re: [U-Boot] u-boot <=> kernel compatibility?

On Sat, Nov 04, 2017 at 07:22:06AM +0100, daggs wrote:


Greetings,

is there a minimum kernel version limitation on the latest release of
u-boot?  the reason I'm asking is that I'm unable to boot a arm board
with latest u-boot and the vendor's kernel which is based on kernel
3.14.x


There is generally not, but what is your actual error?  Thanks!


the last print I see is Starting kernel ...
using prints I was able to trace the call to armv8_switch_to_el2, if I'm not 
mistaken, this is a asm code in the kernel I'm trying to boot, am I wrong?

Thanks,

Dagg.

Tom, am I right in the assumption above?

Dagg.


I am neither Tom nor a U-Boot expert but *think* "Starting kernel ..." 
comes from U-Boot, not the kernel.


Cheers,
Chris
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] u-boot <=> kernel compatibility?

[Chris Moore]

Hi,

Le 04/11/2017 à 19:37, Tom Rini a écrit :

On Sat, Nov 04, 2017 at 07:22:06AM +0100, daggs wrote:
Greetings, is there a minimum kernel version limitation on the latest 
release of u-boot? the reason I'm asking is that I'm unable to boot a 
arm board with latest u-boot and the vendor's kernel which is based 
on kernel 3.14.x 

There is generally not, but what is your actual error? Thanks!


I *guess* that the vendor's U-Boot performs some initializations missing 
from mainline U-Boot but necessary for the vendor's kernel to boot.


Cheers,
Chris


___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] Fwd: Debian platform firmware strategy?

[Chris Moore]

Hi Paul,

I hope you don't mind my forwarding your message below to the U-Boot ML.
I think U-Boot ML subscribers may be interested in this discussion.
My apologies in advance if I am wrong.

Cheers,
Chris

 Message original 
Sujet:  Debian platform firmware strategy?
Date de renvoi :Wed, 2 Jul 2014 02:24:53 + (UTC)
De (renvoi) :   debian-...@lists.debian.org
Date :  Wed, 2 Jul 2014 10:24:31 +0800
De :Paul Wise p...@debian.org
Pour :  debian-...@lists.debian.org



Hi all,

Platform firmware is an odd beast in the world of software. It is
mostly different per device. It is mostly proprietary and binary only.
When it isn't proprietary, the code is probably not merged upstream.
Sometimes it is very hard or impossible to update. It is very easy to
brick a device with a firmware update. Some devices are unbrickable
due to read-only secondary platform firmware chosen at boot time with
a button or via USB. Updating/replacing the platform firmware could
have unforeseen consequences (not being able to boot other OSes for
eg).

On x86 we ignore the platform firmware, don't package any libre
firmware (i.e. coreboot) and chainload our own bootloaders before
starting an OS.

On ARM the platform firmware is way less standardised but is often
u-boot. u-boot mainline is packaged and apparently has support for 6
armel devices and 13 armhf devices. Other devices ship with either a
locked proprietary bootloader (Android devices mostly), a forked
u-boot or some other FOSS bootloader. Sometimes the device ships with
older less capable versions of u-boot which complicate installation
(extra boot partitions required etc).

What should Debian's strategy/policy wrt platform firmware be?

Currently it seems to be just leave the platform firmware alone and
leave it up to the user to research if they can install libre
firmware.

I'm thinking we should promote using Free Software where possible and
packaged versions of that Free Software where possible. Due to the
possibility of unforeseeable circumstances, that promotion should
probably only consist of a default-to-no suggestion to replace
existing platform firmware if only intending to use Debian on the
device.

--
bye,
pabs

http://wiki.debian.org/PaulWise


--
To UNSUBSCRIBE, email to debian-arm-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Archive: 
https://lists.debian.org/CAKTje6FuZbmzO1=p5+f4qp9yvnjh6dggobdcvf-ecc5t2zb...@mail.gmail.com



___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] Question about Coding-Style

[Chris Moore]

Hi Albert,
Le 10/02/2014 10:58, Albert ARIBAUD a écrit :

Hi Tom,

On Tue, 4 Feb 2014 10:07:32 -0500, Tom Rinitr...@ti.com  wrote:


On Tue, Feb 04, 2014 at 04:02:56PM +0100, Stefano Babic wrote:

Hi Hannes,

On 04/02/2014 15:50, Hannes Petermaier wrote:

[snip]

Another thing is linewrapping of output strings, to obey to the rules i
have to format the string as following:

if (i2c_probe(TPS65217_CHIP_PM)) {
printf(PMIC chip (0x%02x) not present! skipping \
further configuration.\n, TPS65217_CHIP_PM);
return;
}

But this makes it impossible to grep the code in case of an error.

You must combine a more complicate grep, maybe with the -A (after
context) option or using a regexp. However, this is not a reason to
break the rule.

Strings are the reason to break the rule and we have checkpatch patched
(mostly?) to not complain.  It's even true in the kernel.

Hmm... Last time I checked,

abc
def

Is a valid C string, and does not require a backslash. Do I miss
something?


Yes, you are of course correct: the result is the C string abcdef and 
the backslash in the original is superfluous.

However a grep for abcdef in the source code won't match it :(
This is why splitting strings like this is discouraged in the Linux kernel.

Cheers,
Chris


---
Ce courrier électronique ne contient aucun virus ou logiciel malveillant parce 
que la protection avast! Antivirus est active.
http://www.avast.com

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] Thunderbird bug (was PATCH-add 0X hexadecimal prefix to simple_strtoul)

[Chris Moore]

Hi,

Le 09/02/2011 22:17, Scott Wood a écrit :
 On Wed, 9 Feb 2011 07:40:44 +0100
 Albert ARIBAUDalbert.arib...@free.fr  wrote:

 Le 09/02/2011 07:19, Chris Moore a écrit :
 I also noticed this in some of my (rare) posts.
 I am using TB 3.1.5.
 I am too ashamed to admit my OS ;-)

 A nonsense test case written with correct spacing: a = (b  (c  1))
 ((d  ((e  1)  1)));
 If you receive this correctly then please excuse me for the noise :(
 Received correctly. :)

 In Rob's case I am not sure that the issue is due to TB eating selected
 spaces upon reception; my own TB (3.1.7), using ^U to display the raw
 message from Rob, shows missing spaces are actually in what was sent,
 and I think Wolfgang does not use TB at all.
 It's when quoting a message for a reply that Thunderbird does this mangling.


Right, that would seem to be this bug: 
https://bugzilla.mozilla.org/show_bug.cgi?id=448198
I have certainly encountered this one when quoting :(

But there is also: https://bugzilla.mozilla.org/show_bug.cgi?id=597181
I wonder if I haven't run into this one too :(

Cheers,
Chris


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] Thunderbird bug (was PATCH-add 0X hexadecimal prefix to simple_strtoul)

[Chris Moore]

Hi,

Me again.

Le 10/02/2011 07:16, Chris Moore a écrit :
 Hi,

 Le 09/02/2011 22:17, Scott Wood a écrit :
 On Wed, 9 Feb 2011 07:40:44 +0100
 Albert ARIBAUDalbert.arib...@free.fr   wrote:

 Le 09/02/2011 07:19, Chris Moore a écrit :
 I also noticed this in some of my (rare) posts.
 I am using TB 3.1.5.
 I am too ashamed to admit my OS ;-)

 A nonsense test case written with correct spacing: a = (b   (c   1))
 ((d   ((e   1)   1)));
 If you receive this correctly then please excuse me for the noise :(
 Received correctly. :)

 In Rob's case I am not sure that the issue is due to TB eating selected
 spaces upon reception; my own TB (3.1.7), using ^U to display the raw
 message from Rob, shows missing spaces are actually in what was sent,
 and I think Wolfgang does not use TB at all.
 It's when quoting a message for a reply that Thunderbird does this mangling.

 Right, that would seem to be this bug:
 https://bugzilla.mozilla.org/show_bug.cgi?id=448198
 I have certainly encountered this one when quoting :(

 But there is also: https://bugzilla.mozilla.org/show_bug.cgi?id=597181
 I wonder if I haven't run into this one too :(

Note that my reply introduced the quoted text bug in my test case above :(

Cheers,
Chris


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] PATCH-add 0X hexadecimal prefix to simple_strtoul

[Chris Moore]

Hi,

Le 08/02/2011 20:14, Wolfgang Denk a écrit :
 Dear Rob Alexander,

 In message4d51716d.3060...@motorola.com  you wrote:

 +if ((tolower(*cp) == 'x')   isxdigit(cp[1])) {
 ERROR: spaces required around that '' (ctx:VxW)


I suspect that this could be the Thunderbird problem that Albert noticed 
the other day.
Some versions of TB seem to eat the space before an '' character and 
IIRC also before at least one of the '' and '' characters :(

I also noticed this in some of my (rare) posts.
I am using TB 3.1.5.
I am too ashamed to admit my OS ;-)

A nonsense test case written with correct spacing: a = (b  (c  1))  
((d  ((e  1)  1)));
If you receive this correctly then please excuse me for the noise :(

Cheers,
Chris

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v4 2/7] gpio: Add Multi-Function-Pin configuration driver for Marvell SoCs

[Chris Moore]

Hi,

Le 07/12/2010 18:39, Albert ARIBAUD a écrit :
 Le 07/12/2010 18:10, Prafulla Wadaskar a écrit :

 +   val= ~MFP_AF_MASK;
 Do we need to do this   here? For val is only 0 here...
 This can be removed.
 OTOH, with the, this line makes no assumption about val, and thus will
 work regardless of it. If the  is removed, and if later val is set to
 non-zero before reaching this instruction, it will cause a bug.

 IOW, the  makes the statement more resilient.


If val really is zero then the result will always be zero :(
Simply removing the  would give a different result.
It would be better to remove the whole bloody line ;-)

I haven't followed this thread but I suspect the original code was wrong.

Cheers,
Chris
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V2] orion5x: optimize window size computation

[Chris Moore]

  Hi,

Sorry Albert I missed this one last time :(

Le 06/10/2010 16:46, Albert Aribaud a écrit :
 + * 1) A sizeval equal to 0x0 specifies 4 TB

s/TB/GB/ or maybe even s/TB/GiB/

Question: are MB, GB, ... or MiB, GiB, ... preferred in U-Boot?

I generally try to use the i versions where appropriate.
In fact I used a KiB below:

 +  * Calculate the number of 64 KiB blocks needed minus one (rounding up).

IMHO whatever the choice, it would be preferable to be consistent 
throughout.

Cheers,
Chris

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] orion5x: optimize window size computation

[Chris Moore]

  Hi Prafulla,

Le 05/10/2010 07:57, Prafulla Wadaskar a écrit :


 -Original Message-
 From: u-boot-boun...@lists.denx.de
 [mailto:u-boot-boun...@lists.denx.de] On Behalf Of Albert Aribaud
 Sent: Tuesday, October 05, 2010 3:52 AM
 To: u-boot@lists.denx.de
 Subject: [U-Boot] [PATCH] orion5x: optimize window size computation


 Signed-off-by: Chris Mooremo...@free.fr
 ---

 This is a simple optimization of the orion5x window size
 computation. This code was contributed by Chris Moore so
 I put his Signed-off-by rather than mine.
 This is wrong, you should be singed-off since you are posting and Chris can 
 be contributor
 Or let him post the patch.


I asked Albert to post the patch as I am mainly a U-Boot lurker and I 
have no U-Boot git tree.
If there is a problem I don't mind if he signs off either with or 
without me.

 Seehttp://www.mail-archive.com/u-boot@lists.denx.de/msg37075.html

   arch/arm/cpu/arm926ejs/orion5x/cpu.c |   30
 --
   1 files changed, 20 insertions(+), 10 deletions(-)

 diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c
 b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
 index c36d7bf..5c443fe 100644
 --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c
 +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
 @@ -48,24 +48,34 @@ void reset_cpu(unsigned long ignored)
   }

   /*
 - * Window Size
 + * Compute Window Size field value from size expressed in bytes
* Used with the Base register to set the address window
 size and location.
* Must be programmed from LSB to MSB as sequence of ones followed by
* sequence of zeros. The number of ones specifies the size
 of the window in
* 64 KByte granularity (e.g., a value of 0x00FF specifies
 256 = 16 MByte).
 - * NOTE: A value of 0x0 specifies 64-KByte size.
 + * NOTES:
 + * 1) A sizeval equal to 0x0 specifies 4 TB.
 + * 2) A return value of 0x0 specifies 64 KB.
*/
   unsigned int orion5x_winctrl_calcsize(unsigned int sizeval)
   {
 -int i;
 -unsigned int j = 0;
 -u32 val = sizeval  1;
 +/*
 + * Calculate the number of 64 KiB blocks needed minus
 one (rounding up).
 + * For sizeval  0 this is equivalent to:
 + * sizeval = (u32) ceil((double) sizeval / 65536.0) - 1
 + */
 +sizeval = (sizeval - 1)  16;

 -for (i = 0; val= 0x1; i++) {
 -j |= (1  i);
 -val = val  1;
 -}
 -return 0x  j;
 +/*
 + * Propagate 'one' bits to the right by 'oring' them.
 + * We need only treat bits 15-0.
 + */
 +sizeval |= sizeval  1;  /* 'Or' bit 15 onto bit 14 */
 +sizeval |= sizeval  2;  /* 'Or' bits 15-14 onto bits 13-12 */
 +sizeval |= sizeval  4;  /* 'Or' bits 15-12 onto bits 11-8 */
 +sizeval |= sizeval  8;  /* 'Or' bits 15-8 onto bits 7-0*/
 +
 +return sizeval;
 BTW: How much this saves on size?


It is not so much a question of size. I am afraid that the other version 
was just plain *wrong* :(
See my previous post here: 
http://www.mail-archive.com/u-boot@lists.denx.de/msg36996.html
In this post I presented two versions:
a) a corrected version along the lines of the original,
b) a version similar to the above.

The loop version may be slightly shorter in code size, particularly if 
one removes the unnecessary and with 0x at the end.
But aesthetically I find the version above much more pleasing.
(Didn't Donald Knuth write The *Art* of Computer Programming?)
It is also much faster for large window sizes but this probably doesn't 
matter here.

Cheers,
Chris

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation

[Chris Moore]

  Hello Albert,

Le 27/08/2010 07:37, Albert ARIBAUD a écrit :
 Le 27/08/2010 07:00, Chris Moore a écrit :


 I think your proposal to handle size 0 as meaning '4 MB' is fine, 
 since there is no way to express 4MB and a zero size is meaningless as 
 such.


s/MB/GiB/
I agree that it is the most useful.
It also gives the shortest code as it needs no special handling ;-)

 If I have misunderstood, please tell me and I'll rewrite.

 That's fine. Do you want me to resubmit a V2 patch with your change, 
 or will you subit it yourself?


I'd prefer you to submit it if you don't mind as I don't have a U-Boot 
git tree ATM.

Please consider the following version where I have tried to improve the 
comments.
The only other changes are:
a) to keep the return value an unsigned int as in the original.
b) to copy the argument to a local variable.
This enables me to have an argument with a descriptive name and a 
variable with a short name to stay within 78 columns.

/*
  * The spammers are right: size *is* important ;-)
  * This version will not work if sizeval is more than 32 bits.
  * I have therefore made it a u32 to underline this.
  * The return value does not need to be a u32 so I left it as an 
unsigned int.
  */
unsigned int orion5x_winctrl_calcsize_CM_fast(u32 sizeval)
{
   u32 x = sizeval;

   /*
* Requesting a window with a size of 0 bytes makes no sense.
* A sizeval of 0 therefore needs special handling such as:
* a) treat 0 as 4 GiB.
* b) treat 0 as an error.
* c) treat 0 as requiring one 64 KiB block.
*
* The most useful is probably to treat 0 as 4 GiB as we do here.
* This occurs naturally and needs no special handling.
*/

   /*
* Step 1: Get the most significant one (MSO) in the correct bit 
position.
*
* Calculate the number of 64 KiB blocks needed minus one (rounding up).
* For x  0 this is equivalent to x = (u32) ceil((double) x / 
65536.0) - 1
*/
   x = (x - 1)  16;

   /*
* Step 2: Force all bits to the right of the MSO to one.
*
* The right shift above ensures that the 16 MSB of x are zero.
* So, for a u32, there are never more than 15 bits to the right of 
the MSO.
* We 'or' the MSO into them which forces them to one.
*/
   x |= x  1;  /* Force the first bit to the right of the MSO to one */
   x |= x  2;  /* Force the next 2 bits to the right of the MSO to one */
   x |= x  4;  /* Force the next 4 bits to the right of the MSO to one */
   x |= x  8;  /* Force the next 8 bits to the right of the MS0 to one */

   return x;
}

Without comments I think the code would be difficult to understand.
However it may now be overcommented.
Please feel free to modify the comments.
If necessary you can add my SOB.

Cheers,
Chris
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation

[Chris Moore]

  Hi list,

Le 24/08/2010 15:27, Albert Aribaud a écrit :
 Fix orion5x_winctrl_calcsize() off-by-1 bug which caused mapping
 windows to be cut by half. This afected all windows including NOR
 flash (causing half the flash to be unaccessible) but DRAM was and
 still is fine as its size is determined otherwise.

 Signed-off-by: Albert Aribaudalbert.arib...@free.fr
 ---
   arch/arm/cpu/arm926ejs/orion5x/cpu.c |2 +-
   1 files changed, 1 insertions(+), 1 deletions(-)

 diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c 
 b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
 index 3740e33..260f88b 100644
 --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c
 +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
 @@ -61,7 +61,7 @@ unsigned int orion5x_winctrl_calcsize(unsigned int sizeval)
   unsigned int j = 0;
   u32 val = sizeval  1;

 - for (i = 0; val  0x1; i++) {
 + for (i = 0; val= 0x1; i++) {
   j |= (1  i);
   val = val  1;
   }

Sorry for this late reply.
Both versions looked wrong to me but I couldn't test them at the time.

Albert's version gives the correct result for exact powers of 2.
This is admittedly the usual case.
However AFAICS it gives a wrong result for *all* other values above 64 KiB.

The following version gives the results I would expect:

unsigned int orion5x_winctrl_calcsize(unsigned int sizeval)
{
int i;
unsigned int j = 0;
u32 val = sizeval - 1;

for (i = 0; val = 0x1; i++) {
j |= (1  i);
val = val  1;
}
return 0x  j;
}

Notes:
a) Masking with 0x is not necessary.
I kept it to keep the code as close as possible to the original.
b) A size of 0 on entry is stupid and must be considered as a special case.
The most useful is probably to treat 0 as 4 GiB.
This occurs naturally and needs no special handling.
Otherwise we could treat it as one 64 KiB page.
Otherwise we could BUG_ON.

However I have completely rewritten the function using a more efficient 
algorithm.
It gives identical results to the previous one above.
For large window sizes this version is much faster.
It avoids the loop which repeatedly divides the size by 2.

u32 orion5x_winctrl_calcsize(u32 x)
{
   /*
* Step 1: Get the most significant one in the right bit position.
*
* Calculate the number of 64 KiB blocks needed minus one (rounding up).
* It is equivalent to x = (u32) ceil((double) x / 65536.0) - 1
*/
   x = (x - 1)  16;

   /*
* Step 2: Copy the most significant one (MSO) into all bits to its 
right.
*
* At this point the right shift above ensures that the 16 MSB of x 
are zero.
* Propagate the MSO to all bits to its right (up to 15 bits).
*/
   x |= x  1;  /* Set the bit to the right of the MSO */
   x |= x  2;  /* Set the next 2 bits to the right of the MSO */
   x |= x  4;  /* Set the next 4 bits to the right of the MSO */
   x |= x  8;  /* Set the next 8 bits to the right of the MS0 */

   return x;
}

Here are my test results for all four versions:

For range 0x to 0x00020001 the original orion5x_winctrl_calcsize 
returns 0x0
For range 0x00020002 to 0x00040003 the original orion5x_winctrl_calcsize 
returns 0x1
For range 0x00040004 to 0x00080007 the original orion5x_winctrl_calcsize 
returns 0x3
For range 0x00080008 to 0x001f the original orion5x_winctrl_calcsize 
returns 0x7
For range 0x00100010 to 0x0020001f the original orion5x_winctrl_calcsize 
returns 0xf
For range 0x00200020 to 0x0040003f the original orion5x_winctrl_calcsize 
returns 0x1f
For range 0x00400040 to 0x0080007f the original orion5x_winctrl_calcsize 
returns 0x3f
For range 0x00800080 to 0x01ff the original orion5x_winctrl_calcsize 
returns 0x7f
For range 0x01000100 to 0x020001ff the original orion5x_winctrl_calcsize 
returns 0xff
For range 0x02000200 to 0x040003ff the original orion5x_winctrl_calcsize 
returns 0x1ff
For range 0x04000400 to 0x080007ff the original orion5x_winctrl_calcsize 
returns 0x3ff
For range 0x08000800 to 0x1fff the original orion5x_winctrl_calcsize 
returns 0x7ff
For range 0x10001000 to 0x20001fff the original orion5x_winctrl_calcsize 
returns 0xfff
For range 0x20002000 to 0x40003fff the original orion5x_winctrl_calcsize 
returns 0x1fff
For range 0x40004000 to 0x80007fff the original orion5x_winctrl_calcsize 
returns 0x3fff
For range 0x80008000 to 0x the original orion5x_winctrl_calcsize 
returns 0x7fff

For range 0x to 0x0001 Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x0
For range 0x0002 to 0x0003 Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x1
For range 0x0004 to 0x0007 Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x3
For range 0x0008 to 0x000f Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x7
For range 0x0010 to 0x001f Albert Aribaud's 
orion5x_winctrl_calcsize returns 0xf
For range 0x0020 to 0x003f Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x1f

Re: [U-Boot] [PATCH V2 1/3] memcpy: copy one word at a time if possible

[Chris Moore]

Mike Frysinger a écrit :
 On Friday 09 October 2009 06:11:16 Mark Jackson wrote:
   
 Chris Moore wrote:
 
 I agree wholeheartedly with the idea but shouldn't it be more like this
 (untested) code :

 void * memcpy(void *dest, const void *src, size_t count)

 {
 char *d8, *s8;
 unsigned long *dl = dest, *sl = src;
   
 In here, would it be overkill to add byte copying until data is aligned,
  and then fall into the aligned copy code.
 

 both addresses have to be unaligned the same ...

 if ((ulong)dl  (sizeof(*dl) - 1) == (ulong)sl  (sizeof(*sl) - 1))

   
 In that case, you'd still gain a speed increase if you're starting at an
  unaligned address ?
 

 now it's a question of how often does this come up and is it worth the code 
 size increase ?
   

Like Mike I'm not sure if it is worth it for memcpy.
However it may be for memset where only the destination has to be aligned.

Chris


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V3 1/3] lib_generic memcpy: copy one word at a time if possible

[Chris Moore]

Alessandro Rubini a écrit :
 From: Alessandro Rubini rub...@unipv.it

 If source and destination are aligned, this copies ulong values
 until possible, trailing part is copied by byte. Thanks for the details
 to Wolfgang Denk, Mike Frysinger, Peter Tyser, Chris Moore.

 Signed-off-by: Alessandro Rubini rub...@unipv.it
 Acked-by: Andrea Gallo andrea.ga...@stericsson.com
 ---
  lib_generic/string.c |   19 +++
  1 files changed, 15 insertions(+), 4 deletions(-)

 diff --git a/lib_generic/string.c b/lib_generic/string.c
 index 181eda6..9bee79b 100644
 --- a/lib_generic/string.c
 +++ b/lib_generic/string.c
 @@ -446,12 +446,23 @@ char * bcopy(const char * src, char * dest, int count)
   * You should not use this function to access IO space, use memcpy_toio()
   * or memcpy_fromio() instead.
   */
 -void * memcpy(void * dest,const void *src,size_t count)
 +void * memcpy(void *dest, const void *src, size_t count)
  {
 - char *tmp = (char *) dest, *s = (char *) src;
 -
 + unsigned long *dl = (unsigned long *)dest, *sl = (unsigned long *)src;
   

Nitpick: Are you sure the casts are necessary here ?
I am not sure about U-Boot but Linux kernel maintainers frown on 
unnecessary casts.

 + char *d8, *s8;
 +
 + /* while all data is aligned (common case), copy a word at a time */
 + if ( (((ulong)dest | (ulong)src | count)  (sizeof(*dl) - 1)) == 0) {
   

+   if ( (((ulong)dest | (ulong)src)  (sizeof(*dl) - 1)) == 0) {

The or should not include count: the remaining count % sizeof(unsigned 
long) bytes are copied below.

 + while (count = sizeof(*dl)) {
 + *dl++ = *sl++;
 + count -= sizeof(*dl);
 + }
 + }
 + /* copy the reset one byte at a time */
   

Nitpick: s/reset/rest/

 + d8 = (char *)dl;
 + s8 = (char *)sl;
   while (count--)
 - *tmp++ = *s++;
 + *d8++ = *s8++;
  
   return dest;
  }
   

Cheers,
Chris

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V3 2/3] lib_generic memset: fill one word at a time if possible

[Chris Moore]

Similar remarks to those for memcpy :

Alessandro Rubini a écrit :
 From: Alessandro Rubini rub...@unipv.it

 If the destination is aligned, fill ulong values until possible.
 Then fill remaining part by bytes.

 Signed-off-by: Alessandro Rubini rub...@unipv.it
 Acked-by: Andrea Gallo andrea.ga...@stericsson.com
 ---
  lib_generic/string.c |   22 +++---
  1 files changed, 19 insertions(+), 3 deletions(-)

 diff --git a/lib_generic/string.c b/lib_generic/string.c
 index 9bee79b..1575c63 100644
 --- a/lib_generic/string.c
 +++ b/lib_generic/string.c
 @@ -403,10 +403,26 @@ char *strswab(const char *s)
   */
  void * memset(void * s,int c,size_t count)
  {
 - char *xs = (char *) s;
 -
 + unsigned long *sl = (unsigned long *) s;
   

Nitpick: Cast ?

+   unsigned long *sl = s;


 + unsigned long cl = 0;
 + char *s8;
 + int i;
 +
 + /* do it one word at a time (32 bits or 64 bits) while possible */
 + if ( ((count | (ulong)s)  (sizeof(*sl) - 1)) == 0) {
   

Remove count from or.

+   if (((ulong)s  (sizeof(*sl) - 1)) == 0) {


 + for (i=0; isizeof(*sl); ++i) {
   

Nitpick: for LK spaces around operators are preferred but I am not sure 
for U-Boot.

+   for (i = 0; i  sizeof(*sl); ++i) {


 + cl = 8;
 + cl |= c  0xff;
 + }
 + while (count = sizeof(*sl)) {
 + *sl++ = cl;
 + count -= sizeof(*sl);
 + }
 + }
 + /* fill 8 bits at a time */
 + s8 = (char *)sl;
   while (count--)
 - *xs++ = c;
 + *s8++ = c;
  
   return s;
  }
   

Cheers,
Chris

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V2 1/3] memcpy: copy one word at a time if possible

[Chris Moore]

Wolfgang Denk a écrit :
 I think we should change this if-else into a plain if, something like
 that:

 void * memcpy(void *dest, const void *src, size_t count)
 {
   char *tmp = (char *) dest, *s = (char *) src;
   char *d8 = (char *)dest, *s8 = (char *)src;
   unsigned long *dl = (unsigned long *)dest, *sl = (unsigned long *)src;

   /* while all data is aligned (common case), copy a word at a time */
   if ( (((int)dest | (int)src | count)  (sizeof(long) - 1)) == 0) {
   while (count) {
   *dl++ = *sl++;
   count -= sizeof(unsigned long);
   }
   }
   while (count--)
   *d8++ = *s8++;

   return dest;
 }

 This way we can have both - the long copy of a potential aligne
 dfirst part, and the byte copy of any trailing (or unaligned) part.

   

I agree wholeheartedly with the idea but shouldn't it be more like this 
(untested) code :

void * memcpy(void *dest, const void *src, size_t count)

{
char *d8, *s8;
unsigned long *dl = dest, *sl = src;

/* while all data is aligned (common case), copy multiple bytes at a 
time */
if ( (((int)(long)dest | (int)(long)src)  (sizeof(*dl) - 1)) == 0) {
while (count = sizeof(*dl)) {
*dl++ = *sl++;
count -= sizeof(*dl);
}
}

d8 = (char *)dl;
s8 = (char *)sl;

/* copy any remaining data byte by byte */
while (count--)
*d8++ = *s8++;

return dest;
}


Remarks :
1) My curious (int) (long) pointer casts are intended to avoid compiler 
warnings while avoiding unnecessary calculations in long.
On some architectures long calculations are less efficient than int ones.
In fact I wonder whether, on such architectures, it might not also be 
better to perform the copy with int size chunks.
2) Personally I prefer sizeof(*dl) to sizeof(unsigned long) as there is 
less risk of error if the type of the chunks is changed.
3) In C (but not in C++) I think the casts from void * to unsigned long 
* are unnecessary.

But as I said all this is completely untested :(

Cheers,
Chris

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] memcpy: use 32-bit copies if possible

[Chris Moore]

Alessandro Rubini a écrit :
 From: Alessandro Rubini rub...@unipv.it

 Signed-off-by: Alessandro Rubini rub...@unipv.it
 Acked-by: Andrea Gallo andrea.ga...@stericsson.com
 ---
  lib_generic/string.c |   11 ++-
  1 files changed, 10 insertions(+), 1 deletions(-)

 diff --git a/lib_generic/string.c b/lib_generic/string.c
 index 181eda6..fdccab6 100644
 --- a/lib_generic/string.c
 +++ b/lib_generic/string.c
 @@ -449,7 +449,16 @@ char * bcopy(const char * src, char * dest, int count)
  void * memcpy(void * dest,const void *src,size_t count)
  {
   char *tmp = (char *) dest, *s = (char *) src;
 + u32 *d32 = (u32 *)dest, *s32 = (u32 *) src;
  
 + /* if both are aligned, use 32-bit copy */
 + if ( (((int)dest  3) | ((int)src  3) | (count  3)) == 0 ) {
   

This can be factorized as (a  3) | (b  3) | (c  3) is equivalent to 
(a | b | c)  3.
GCC is pretty smart but I doubt that it will pick this up :(

 + count /= 4;
 + while (count--)
 + *d32++ = *s32++;
 + return dest;
 + }
 + /* else, use 1-byte copy */
   while (count--)
   *tmp++ = *s++;
  
   

Cheers,
Chris

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] ppc/85xx: Fix LCRR_CLKDIV defines

[Chris Moore]

Peter Tyser a écrit :
 On Tue, 2009-09-15 at 22:26 -0500, Kumar Gala wrote:
   
 For some reason the CLKDIV field varies between SoC in how it interprets
 the bit values.

 

[snip]


 Ensure that the MPC86xx and MPC85xx still get the same behavior and make
 the the defines reflect their logical view (not the value of the field).

 

Nit picking:
I just noticed that there is a duplicate the here.
Also I prefer behaviour but this is a US/UK issue :-(
trollWhy can't Americans write English ;-) ?/troll

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot