* [U-Boot-Users] [PATCH] Support for csb535fs / i.MX21 LiteKit.
@ 2007-02-27 13:02 Txema Lopez
2007-02-27 17:20 ` Grant Likely
0 siblings, 1 reply; 8+ messages in thread
From: Txema Lopez @ 2007-02-27 13:02 UTC (permalink / raw)
To: u-boot
Hi all,
This patch, for the 1.2.0 version, adds support for the csb535fs board
embedded in a csb935fs breakout board. The set is known as i.MX21 Litekit.
The main features of the board are:
csb535fs.
- MC9328MX21 processor with an ARM926EJS core.
- 8 Mbytes of NOR FLASH.
- 64 MBytes of SDRAM.
csb935fs
- CS8900 10 MBit Ethernet controller.
- RS232.
- USB.
- SD/MMC.
- Compact Flash.
- 3,5'' 240*320 QVGA TFT LCD.
This patch only adds support for:
- MC9328MX21 processor with an ARM926EJS core.
- 8 Mbytes of NOR FLASH.
- 64 MBytes of SDRAM.
- CS8900 10 MBit Ethernet controller.
- RS232.
Signed-off-by: Jose Maria Lopez <tlopez@fagorautomation.es>
CHANGELOG
* Add support for the csb535fs board:
Patch by Jose Maria Lopez, 27 February 2007
-------------- next part --------------
A non-text attachment was scrubbed...
Name: csb535fs.patch.tgz
Type: application/x-compressed-tar
Size: 13652 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070227/0cb3d930/attachment.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tlopez.vcf
Type: text/x-vcard
Size: 324 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070227/0cb3d930/attachment.vcf
^ permalink raw reply [flat|nested] 8+ messages in thread* [U-Boot-Users] [PATCH] Support for csb535fs / i.MX21 LiteKit. 2007-02-27 13:02 [U-Boot-Users] [PATCH] Support for csb535fs / i.MX21 LiteKit Txema Lopez @ 2007-02-27 17:20 ` Grant Likely 2007-02-28 7:43 ` Txema Lopez 0 siblings, 1 reply; 8+ messages in thread From: Grant Likely @ 2007-02-27 17:20 UTC (permalink / raw) To: u-boot On 2/27/07, Txema Lopez <tlopez@aotek.es> wrote: > Hi all, > This patch, for the 1.2.0 version, adds support for the csb535fs board > embedded in a csb935fs breakout board. The set is known as i.MX21 Litekit. With a quick perusal, the patch looks mostly okay. Unfortunately, it's inconvenient to review because it was sent as a gzipped attachment instead of inline (I can't just hit 'reply' and start typing comments). Here are some general comments: - You should add your copyright to all new files that you've added. At the moment, your copyright only appears on a few of the new files. - Where did include/asm-arm/arch-imx21/imx21-regs.h come from? There is no copyright notice on it at all. - There's a fair bit of inconsistent whitespace (intermixed space and tab characters). - I do wonder at the amount of boilerplate required for each new board port (but that's a longer ranged questions directed at the whole of u-boot). Cheers, g. -- Grant Likely, B.Sc. P.Eng. Secret Lab Technologies Ltd. grant.likely at secretlab.ca (403) 399-0195 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot-Users] [PATCH] Support for csb535fs / i.MX21 LiteKit. 2007-02-27 17:20 ` Grant Likely @ 2007-02-28 7:43 ` Txema Lopez 2007-02-28 16:55 ` Grant Likely 0 siblings, 1 reply; 8+ messages in thread From: Txema Lopez @ 2007-02-28 7:43 UTC (permalink / raw) To: u-boot Grant Likely wrote: > On 2/27/07, Txema Lopez <tlopez@aotek.es> wrote: > >> Hi all, >> This patch, for the 1.2.0 version, adds support for the csb535fs board >> embedded in a csb935fs breakout board. The set is known as i.MX21 >> Litekit. > > > With a quick perusal, the patch looks mostly okay. Unfortunately, > it's inconvenient to review because it was sent as a gzipped > attachment instead of inline (I can't just hit 'reply' and start > typing comments). > I'm sorry, the patch size is more than the 40k U-Boot's limit, so ... > Here are some general comments: > - You should add your copyright to all new files that you've added. > At the moment, your copyright only appears on a few of the new files. The cpu/arm926ejs/imx21 files have been copied from the cpu/arm920t/imx with a few modifications, so in the files with minor changes I've left the old copyright. > - Where did include/asm-arm/arch-imx21/imx21-regs.h come from? There > is no copyright notice on it at all. It is a modification of include/asm-arm/arch-imx/imx-regs.h > - There's a fair bit of inconsistent whitespace (intermixed space and > tab characters). Please, could you be more explicit and tell me where these mistakes are . > - I do wonder at the amount of boilerplate required for each new board > port (but that's a longer ranged questions directed at the whole of > u-boot). > Me too. Best regards, -------------- next part -------------- A non-text attachment was scrubbed... Name: tlopez.vcf Type: text/x-vcard Size: 324 bytes Desc: not available Url : http://lists.denx.de/pipermail/u-boot/attachments/20070228/d5bedb08/attachment.vcf ^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot-Users] [PATCH] Support for csb535fs / i.MX21 LiteKit. 2007-02-28 7:43 ` Txema Lopez @ 2007-02-28 16:55 ` Grant Likely 2007-03-01 8:08 ` Txema Lopez 0 siblings, 1 reply; 8+ messages in thread From: Grant Likely @ 2007-02-28 16:55 UTC (permalink / raw) To: u-boot On 2/28/07, Txema Lopez <tlopez@aotek.es> wrote: > Grant Likely wrote: > > > On 2/27/07, Txema Lopez <tlopez@aotek.es> wrote: > > > >> Hi all, > >> This patch, for the 1.2.0 version, adds support for the csb535fs board > >> embedded in a csb935fs breakout board. The set is known as i.MX21 > >> Litekit. > > > > > > With a quick perusal, the patch looks mostly okay. Unfortunately, > > it's inconvenient to review because it was sent as a gzipped > > attachment instead of inline (I can't just hit 'reply' and start > > typing comments). > > > I'm sorry, the patch size is more than the 40k U-Boot's limit, so ... ... it then makes sense to break it up into a couple of patches. :-) shorter patches are easier to review anyway. Plus with smaller patches, changes that get 'acked' aren't necessarily held up by changes that need some more work. > > > Here are some general comments: > > - You should add your copyright to all new files that you've added. > > At the moment, your copyright only appears on a few of the new files. > > The cpu/arm926ejs/imx21 files have been copied from the cpu/arm920t/imx with > a few modifications, so in the files with minor changes I've left the > old copyright. Hmmm, indeed. There are large sections of identical, or near identical code between the two. If I replace all the IMX21_* macros with IMX_* in imx-regs.h, it results in a file that is somewhere around 75% identical. I know that slightly modified duplicate code is common in u-boot, so this is not an critique on your work, but I'd really like to move away from this mode of operation. Duplicating the original file and modifying it is certainly the easiest way to add support for a new board/cpu, but I think it just leads to maintenance problems down the roads. For example, if one file has been duplicated with minor mods 10 times and a bug is found in it at a later date; the bug fix needs to be applied to 10 files, not one. Unfortunately, this situation is messy because the imx is in cpu/arm920t and imx32 is in cpu/arm926ejs. It probably requires the creation of a new directory for the common imx soc bits, but where? Perhaps under lib_arm/imx? > > > - Where did include/asm-arm/arch-imx21/imx21-regs.h come from? There > > is no copyright notice on it at all. > > It is a modification of include/asm-arm/arch-imx/imx-regs.h ok > > > - There's a fair bit of inconsistent whitespace (intermixed space and > > tab characters). > > Please, could you be more explicit and tell me where these mistakes are . Configure your editor to make tab characters and trailing whitespace visually distinct and you'll see them. - lowlevel_init.S has a dozen or so lines indented with 4 spaces instead of a tab character. The coding standard is for indentation with tab characters, and tab stops are at 8 character columns. - imx21-regs.h: inconsistent throughout; but you probably inherited this whitespace - serial.c has a few lines where spaces precede tabs; pretty minor stuff. > > > - I do wonder at the amount of boilerplate required for each new board > > port (but that's a longer ranged questions directed at the whole of > > u-boot). > > > Me too. :-) -- Grant Likely, B.Sc. P.Eng. Secret Lab Technologies Ltd. grant.likely at secretlab.ca (403) 399-0195 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot-Users] [PATCH] Support for csb535fs / i.MX21 LiteKit. 2007-02-28 16:55 ` Grant Likely @ 2007-03-01 8:08 ` Txema Lopez 2007-03-01 8:17 ` Wolfgang Denk ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Txema Lopez @ 2007-03-01 8:08 UTC (permalink / raw) To: u-boot Grant Likely wrote: > > > I know that slightly modified duplicate code is common in u-boot, so > this is not an critique on your work, but I'd really like to move away > from this mode of operation. Duplicating the original file and > modifying it is certainly the easiest way to add support for a new And It makes the code more readable. It's the pro. > 10 times and a bug is found in it at a later date; the bug fix needs > to be applied to 10 files, not one. Yes, It's the con. But, how many times does it happen?. > > Unfortunately, this situation is messy because the imx is in > cpu/arm920t and imx32 is in cpu/arm926ejs. It probably requires the > creation of a new directory for the common imx soc bits, but where? > Perhaps under lib_arm/imx? > Ummm, lib_arm/imx?. I think this mixes up concepts. Why not to decouple the arm cores and the SoC code?. For example: cpu/arm/cores/arm926ejs cpu/arm/soc/imx Perhaps, there was a discussion about this in U-Boot and I'm talking nonsense. Anyway, you are the mainteiner so you have the last say. ;-) Best regards, Txema. -------------- next part -------------- A non-text attachment was scrubbed... Name: tlopez.vcf Type: text/x-vcard Size: 324 bytes Desc: not available Url : http://lists.denx.de/pipermail/u-boot/attachments/20070301/4c8fb6c2/attachment.vcf ^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot-Users] [PATCH] Support for csb535fs / i.MX21 LiteKit. 2007-03-01 8:08 ` Txema Lopez @ 2007-03-01 8:17 ` Wolfgang Denk 2007-03-01 10:30 ` Txema Lopez 2007-03-01 14:23 ` Grant Likely 2 siblings, 0 replies; 8+ messages in thread From: Wolfgang Denk @ 2007-03-01 8:17 UTC (permalink / raw) To: u-boot In message <45E68A01.3060607@aotek.es> you wrote: > > I know that slightly modified duplicate code is common in u-boot, so > > this is not an critique on your work, but I'd really like to move away > > from this mode of operation. Duplicating the original file and > > modifying it is certainly the easiest way to add support for a new > > And It makes the code more readable. It's the pro. No, it doesn't make anything more redable. Distributing the code over too many polaces just pollutes the source tree. > > 10 times and a bug is found in it at a later date; the bug fix needs > > to be applied to 10 files, not one. > > Yes, It's the con. But, how many times does it happen?. All too many times. > Ummm, lib_arm/imx?. I think this mixes up concepts. > Why not to decouple the arm cores and the SoC code?. For example: > cpu/arm/cores/arm926ejs > cpu/arm/soc/imx No, but eventually cpu/arm/imx_common plus cpu/arm926ejs Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de I used to be indecisive, now I'm not sure. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot-Users] [PATCH] Support for csb535fs / i.MX21 LiteKit. 2007-03-01 8:08 ` Txema Lopez 2007-03-01 8:17 ` Wolfgang Denk @ 2007-03-01 10:30 ` Txema Lopez 2007-03-01 14:23 ` Grant Likely 2 siblings, 0 replies; 8+ messages in thread From: Txema Lopez @ 2007-03-01 10:30 UTC (permalink / raw) To: u-boot Hi Wolfgang, Please, let me know if i'm wrong. Is this your proposal? ifdef SOC ifeq ($(SOC),imx) LIBS += cpu/$(SOC)_common/lib$(SOC).a else LIBS += cpu/$(CPU)/$(SOC)/lib$(SOC).a endif endif Hi Grant, If you are agree, I'll start up with the changes. Best Regards, Txema. -------------- next part -------------- A non-text attachment was scrubbed... Name: tlopez.vcf Type: text/x-vcard Size: 324 bytes Desc: not available Url : http://lists.denx.de/pipermail/u-boot/attachments/20070301/409373d8/attachment.vcf ^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot-Users] [PATCH] Support for csb535fs / i.MX21 LiteKit. 2007-03-01 8:08 ` Txema Lopez 2007-03-01 8:17 ` Wolfgang Denk 2007-03-01 10:30 ` Txema Lopez @ 2007-03-01 14:23 ` Grant Likely 2 siblings, 0 replies; 8+ messages in thread From: Grant Likely @ 2007-03-01 14:23 UTC (permalink / raw) To: u-boot On 3/1/07, Txema Lopez <tlopez@aotek.es> wrote: > Grant Likely wrote: > > I know that slightly modified duplicate code is common in u-boot, so > > this is not an critique on your work, but I'd really like to move away > > from this mode of operation. Duplicating the original file and > > modifying it is certainly the easiest way to add support for a new > > And It makes the code more readable. It's the pro. I disagree; adding more code volume makes it harder to find stuff. > > 10 times and a bug is found in it at a later date; the bug fix needs > > to be applied to 10 files, not one. > > Yes, It's the con. But, how many times does it happen?. Very frequently > > Unfortunately, this situation is messy because the imx is in > > cpu/arm920t and imx32 is in cpu/arm926ejs. It probably requires the > > creation of a new directory for the common imx soc bits, but where? > > Perhaps under lib_arm/imx? > > > Ummm, lib_arm/imx?. I think this mixes up concepts. > Why not to decouple the arm cores and the SoC code?. For example: > cpu/arm/cores/arm926ejs > cpu/arm/soc/imx That's an idea too. I don't really know where it should go and I'm throwing out ideas. To put it in cpu/arm/soc/* I think will require changes to the config system. If I understand correctly, the Makefiles assume the CPU support code is one level below the 'cpu' directory. I don't know what the impact is of moving it deeper. > Perhaps, there was a discussion about this in U-Boot and I'm talking > nonsense. Not that I know of; this is a new discussion. > Anyway, you are the mainteiner so you have the last say. ;-) Nope, I'm just a reviewer. :-) g. -- Grant Likely, B.Sc. P.Eng. Secret Lab Technologies Ltd. grant.likely at secretlab.ca (403) 399-0195 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2007-03-01 14:23 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-02-27 13:02 [U-Boot-Users] [PATCH] Support for csb535fs / i.MX21 LiteKit Txema Lopez 2007-02-27 17:20 ` Grant Likely 2007-02-28 7:43 ` Txema Lopez 2007-02-28 16:55 ` Grant Likely 2007-03-01 8:08 ` Txema Lopez 2007-03-01 8:17 ` Wolfgang Denk 2007-03-01 10:30 ` Txema Lopez 2007-03-01 14:23 ` Grant Likely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox