[U-Boot-Users] [PATCH] PCMCIA card support for PXA/WEPEP250

[Wolfgang Denk]

Dear Leon,

sorry it took me so long to have a closer look at your patch.

As mentioned before (in private email) I would like to add it, but  I
would like to ask you for yet another modification / cleanup.

In message <1071053086.15228.80.camel at tt-devel.ultra.si> you wrote:
> 
> > Why do you think such a change (which does waste memory while running
> > U-Boot) is necessary?
> 
> I have changed TEXT_ADDRESS to 0xA1FB0000 now.

Please check again; the situation has changed  with  the  new  memory
layout, I think.

> > * I don't want to see code like "convert_string_in_big_endian_to_little_endian()"
> >   in common/cmd_ide.c
> 
> Function renamed to ident_big2little. Function is still inside cmd_ide.c
> since it is only used in ide_ident. I suggest, since this is a generic
> string manipulation function, that the ident_big2little is renamed to
> string_big2little and moved inside lib_generic/string.c, but let me do
> this as a second stage please. Any better function name suggestions are
> welcome.

I suggest to use the IMHO more  correct  name  string_swab()  [String
swap  bytes].  Please add the function to lib_generic/string.c. Also,
please check the implementation - I am not sure if  the  handling  of
the last byte is correct. If this is an operation on (NUL terminated)
_strings_,  then  the  "len"  argument is not necessary and should be
removed. Otherwise, mem_swab() might be a more appropriate name.


> > * The implementation of "convert_string_in_big_endian_to_little_endian()"
> >   seems broken to me. The use of two_byte_p and complicated shift ops
> >   is not necessary when simple operations of character pointers  will
> >   do,  too.  Also,  using  a  "ushort *" might cause problems on some
> >   boards when the string buffer is not alligned to an even address.
> 
> Reimplemented it using char pointer.

Please use "unsigned char" instead.

> > * You add/modify a number of architecture specific header  files  and
> >   some  source  files.  Can you guarantee that these modifications do
> >   not break support for other ARM boards? Are all these modifications
> >   really needed? To me it seems  only  a  small  subset  is  actually
> >   necessary.  I  don't  want  to  blow  up U-Boot if we can avoid it.
> >   Please clean up to use only the really necessary stuff.
> 
> I have built the image for all ARM boards using "MAKEALL arm" have added
> the missing include/asm-arm/arch-arm720t and dummy io.h and hardware.h
> where they were required. Since I can not verify for all other boards
> except wepep250 I'm asking people to try the patch and see if it works
> for them and respond.

I checked this as far as I could; it seems, all the dependencies come
from the new file include/asm-arm/arch-pxa/io.h - this makes you  add
all the other dummy io.h files etc. -- but none of the definitions in
include/asm-arm/arch-pxa/io.h  are  used anywhere in your code, so it
seems this file is superflous.

> File list breakdown:
...
> include/asm-arm/arch-arm720t/hardware.h
> include/asm-arm/arch-arm720t/io.h
> include/asm-arm/arch-arm920t/hardware.h
> include/asm-arm/arch-arm920t/io.h
> include/asm-arm/arch-arm925t/hardware.h
> include/asm-arm/arch-arm925t/io.h
> include/asm-arm/arch-arm926ejs/hardware.h
> include/asm-arm/arch-arm926ejs/io.h
> include/asm-arm/arch-at91rm9200/hardware.h
> include/asm-arm/arch-at91rm9200/io.h
> include/asm-arm/arch-ixp/hardware.h
> include/asm-arm/arch-ixp/io.h
> include/asm-arm/arch-sa1100/io.h
> include/asm-arm/arch-sa1100/hardware.h
> 
> 			Dummy header files that I have found in Linux 			kernel and not in
> U-Boot. Since I have added
> 			include/asm-arm/io.h which includes 			<asm/arch/io.h> the dummy
> headers also had to be
> 			added in order to avoid hacks.

I see no reason to add these files. Please clean up the patch  so  we
don't have to add such a lot of dummy files.


> include/asm-arm/assembler.h
> 			possible issues with ASM files including 			assembler.h. Currently

None of the definitions seem to be used anywhere. So why do we need this file?

> only
> 			lib_arm/io-readsw-arm4.S and
> 			lib_arm/io-writesw-arm4.S use that header file.

I don't see any actual use of the definitions provided by include/asm-arm/assembler.h


Please try to keep the impact of your patch at a minimum, i. e. don't
add any files that are not really needed, and - if possible  -  don't
add empty / dummy files.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
Quote from a recent meeting:   "We are going to continue having these
meetings everyday until I find out why no work is getting done."