[U-Boot] [PATCH] ARMV7: OMAP3: Add support for Comelit CPS board

[Luca Ceresoli <luca.ceresoli@comelit.it>]

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 message<1300707767-15682-2-git-send-email-luca.ceresoli@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.

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 <jschmoller@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_pandora                arm         armv7       pandora             -
>>   igep0020                     arm         armv7       igep0020            isee           omap3
>>   igep0030                     arm         armv7       igep0030            isee           omap3
>>   am3517_evm                   arm         armv7       am3517evm           logicpd        omap3
>> +omap3_cps                    arm         armv7       cps                 comelit        omap3
>>   omap3_zoom1                  arm         armv7       zoom1               logicpd        omap3
>>   omap3_zoom2                  arm         armv7       zoom2               logicpd        omap3
>>   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_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.
>

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