Re: [U-Boot] [PATCH] ARMV7: OMAP3: Add support for Comelit CPS board
Dear Luca Ceresoli, In message 1300707767-15682-2-git-send-email-luca.ceres...@comelit.it you wrote: Board support for the Comelit Group SpA CPS board, which is a custom board based on the BeagleBoard http://beagleboard.org/ by Texas Instruments. The board support is based on the BeagleBoard implementation. It appears you base this code on an old version of U-Boot. Please update your code base first. ALso make sure to run your patch to checkpatch before submitting: [PATCH] ARMV7: OMAP3: Add support for Comelit CPS board total: 4 errors, 325 warnings, 976 lines checked Please fix these! --- /dev/null +++ b/board/comelit/cps/config.mk ... +CONFIG_SYS_TEXT_BASE = 0x80008000 We don't accept such config.mk files any more. Please move definition into your board config file. + /* GPIO list +- 159 OUT (GPIO5+31): reset for remote camera interface connector. +- 19 OUT (GPIO1+19): integrated speaker amplifier (1=on, 0=shdn). +- 20 OUT (GPIO1+20): handset amplifier (1=on, 0=shdn). + */ Incorrect multiline comment style. ... --- a/boards.cfg +++ b/boards.cfg @@ -117,6 +117,7 @@ omap3_pandoraarm armv7 pandora - igep0020 arm armv7 igep0020 isee omap3 igep0030 arm armv7 igep0030 isee omap3 am3517_evm arm armv7 am3517evm logicpdomap3 +omap3_cpsarm armv7 cps comelitomap3 omap3_zoom1 arm armv7 zoom1 logicpdomap3 omap3_zoom2 arm armv7 zoom2 logicpdomap3 omap3_beagle arm armv7 beagle ti omap3 Please name your board just cps (or chose a better name). There shall be no SoC-prefix in the board names. --- /dev/null +++ b/include/configs/omap3_cps.h @@ -0,0 +1,315 @@ ... +#define CONFIG_ARMV7 1 /* This is an ARM V7 CPU core */ +#define CONFIG_OMAP 1 /* in a TI OMAP core */ +#define CONFIG_OMAP34XX 1 /* which is a 34XX */ +#define CONFIG_OMAP3430 1 /* which is in a 3430 */ +#define CONFIG_OMAP3_CPS 1 /* working with CPS board */ #defines like these which select a feature shall not have values assigned. Remove all these '1' here. Please fix globally. 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: w...@denx.de At the source of every error which is blamed on the computer you will find at least two human errors, including the error of blaming it on the computer. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] ARMV7: OMAP3: Add support for Comelit CPS board
Wolfgang, thanks for your patient and prompt review, and sorry for having overlooked checkpatch. Il 21/03/2011 12:55, Wolfgang Denk ha scritto: Dear Luca Ceresoli, In message1300707767-15682-2-git-send-email-luca.ceres...@comelit.it you wrote: Board support for the Comelit Group SpA CPS board, which is a custom board based on the BeagleBoardhttp://beagleboard.org/ by Texas Instruments. The board support is based on the BeagleBoard implementation. It appears you base this code on an old version of U-Boot. Please update your code base first. Strange. My patch is based on current master, on the latest commit that I see today on git://git.denx.de/u-boot.git: $ git show HEAD^ commit cc1dd33f273f8c96cbd7539b4a2d1d7aa12773cd Author: John Schmoller jschmol...@xes-inc.com Date: Thu Mar 10 16:09:26 2011 -0600 mpc8[5/6]xx: Ensure POST word does not get reset ... The next branch is much older, and I haven't found any more recent head in either u-boot-arm nor u-boot-ti. Where should I rebase my code on? ALso make sure to run your patch to checkpatch before submitting: [PATCH] ARMV7: OMAP3: Add support for Comelit CPS board total: 4 errors, 325 warnings, 976 lines checked Please fix these! Fixed most of them for v2. There are still a few warnings, though, that I think are false positives: WARNING: Use #include linux/io.h instead of asm/io.h #169: FILE: board/comelit/cps/cps.c:39: +#include asm/io.h I guess I can safely ignore this one. WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt #1011: FILE: include/configs/omap3_cps.h:305: +extern volatile unsigned int boot_flash_env_addr; I don't know why it is volatile, but since it's defined this way in arch/arm/cpu/armv7/omap3/mem.c I think there's a reason. --- /dev/null +++ b/board/comelit/cps/config.mk ... +CONFIG_SYS_TEXT_BASE = 0x80008000 We don't accept such config.mk files any more. Please move definition into your board config file. Meaning I should remove config.mk entirely, right? +/* GPIO list + - 159 OUT (GPIO5+31): reset for remote camera interface connector. + - 19 OUT (GPIO1+19): integrated speaker amplifier (1=on, 0=shdn). + - 20 OUT (GPIO1+20): handset amplifier (1=on, 0=shdn). +*/ Incorrect multiline comment style. Will fix in v2. ... --- a/boards.cfg +++ b/boards.cfg @@ -117,6 +117,7 @@ omap3_pandoraarm armv7 pandora - igep0020 arm armv7 igep0020 isee omap3 igep0030 arm armv7 igep0030 isee omap3 am3517_evm arm armv7 am3517evm logicpdomap3 +omap3_cpsarm armv7 cps comelitomap3 omap3_zoom1 arm armv7 zoom1 logicpdomap3 omap3_zoom2 arm armv7 zoom2 logicpdomap3 omap3_beagle arm armv7 beagle ti omap3 Please name your board just cps (or chose a better name). There shall be no SoC-prefix in the board names. Will be called dig297 in v2. --- /dev/null +++ b/include/configs/omap3_cps.h @@ -0,0 +1,315 @@ ... +#define CONFIG_ARMV71 /* This is an ARM V7 CPU core */ +#define CONFIG_OMAP 1 /* in a TI OMAP core */ +#define CONFIG_OMAP34XX 1 /* which is a 34XX */ +#define CONFIG_OMAP3430 1 /* which is in a 3430 */ +#define CONFIG_OMAP3_CPS1 /* working with CPS board */ #defines like these which select a feature shall not have values assigned. Remove all these '1' here. Please fix globally. Ok except for OMAP34XX, since arch/arm/cpu/armv7/start.S at line 114 does: #if (CONFIG_OMAP34XX) Is it intended? If that's a mistake, I can submit a trivial patch to fix it. Best regards, Wolfgang Denk Luca ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] ARMV7: OMAP3: Add support for Comelit CPS board
Il 21/03/2011 17:32, Luca Ceresoli ha scritto: ... --- a/boards.cfg +++ b/boards.cfg @@ -117,6 +117,7 @@ omap3_pandoraarm armv7 pandora - igep0020 arm armv7 igep0020 isee omap3 igep0030 arm armv7 igep0030 isee omap3 am3517_evm arm armv7 am3517evm logicpdomap3 +omap3_cpsarm armv7 cps comelitomap3 omap3_zoom1 arm armv7 zoom1 logicpdomap3 omap3_zoom2 arm armv7 zoom2 logicpdomap3 omap3_beagle arm armv7 beagle ti omap3 Please name your board just cps (or chose a better name). There shall be no SoC-prefix in the board names. Will be called dig297 in v2. Well, not so fast. The board is named OMAP3_CPS in the ARM Linux Machine Registry (http://www.arm.linux.org.uk/developer/machines/list.php?id=2751), which is reflected in mach-types.h. I don't know how exactly the registry info is used throughout U-Boot, and I didn't find documentation about it. Am I supposed to rename the board in the registry, then wait for mach-types to be updated in U-Boot before submitting a new patch? Or can I live with the current name in mach-types and rename elsewhere? Thanks, Luca ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] ARMV7: OMAP3: Add support for Comelit CPS board
Dear Luca Ceresoli, In message 4d877d80.6060...@comelit.it you wrote: It appears you base this code on an old version of U-Boot. Please update your code base first. Strange. My patch is based on current master, on the latest commit that I see today on git://git.denx.de/u-boot.git: Sorry. I thought we had cleaned up the config.mk file use for these boards long ago. Seems I was wrong. WARNING: Use #include linux/io.h instead of asm/io.h #169: FILE: board/comelit/cps/cps.c:39: +#include asm/io.h I guess I can safely ignore this one. Yes. WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt #1011: FILE: include/configs/omap3_cps.h:305: +extern volatile unsigned int boot_flash_env_addr; I don't know why it is volatile, but since it's defined this way in arch/arm/cpu/armv7/omap3/mem.c I think there's a reason. No, this should be removed. See Documentation/volatile-considered-harmful.txt We don't accept such config.mk files any more. Please move definition into your board config file. Meaning I should remove config.mk entirely, right? Right. Please name your board just cps (or chose a better name). There shall be no SoC-prefix in the board names. Will be called dig297 in v2. Thanks. #defines like these which select a feature shall not have values assigned. Remove all these '1' here. Please fix globally. Ok except for OMAP34XX, since arch/arm/cpu/armv7/start.S at line 114 does: #if (CONFIG_OMAP34XX) Is it intended? If that's a mistake, I can submit a trivial patch to fix it. Ouch. This is wrong and should be fixed. Yes, please send a (separate) patch for that. 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: w...@denx.de My play was a complete success. The audience was a failure. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] ARMV7: OMAP3: Add support for Comelit CPS board
Dear Luca Ceresoli, In message 4d878b2c.5050...@comelit.it you wrote: Will be called dig297 in v2. Well, not so fast. The board is named OMAP3_CPS in the ARM Linux Machine Registry (http://www.arm.linux.org.uk/developer/machines/list.php?id=2751), which is reflected in mach-types.h. If you want, you can ask for a name change in the Machine Registry. On the other hand, I see no inherent issues when the board config name 0n U-Boot (and Linux?) don't match the MACH_ID's name. But I forgot one thing: it seems also a good idear that the board config names in U-Boot and Linux match. But you don't have any Linux kernel code pushed upstream yet, or have you? Am I supposed to rename the board in the registry, then wait for mach-types to be updated in U-Boot before submitting a new patch? No, this is not really necessary. Or can I live with the current name in mach-types and rename elsewhere? Yes. You may want to clean that up in the near future, but it's not blocking anything here. 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: w...@denx.de 1 1 was a race-horse, 2 2 was 1 2. When 1 1 1 1 race, 2 2 1 1 2. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
