[U-Boot] [PATCH] p2020ds: add alternate boot bank support using the ngPIXIS FPGA

[Timur Tabi]

On Mon, Mar 22, 2010 at 5:46 PM, Wolfgang Denk <wd at denx.de> wrote:

>> + * This file is licensed under the terms of the GNU General Public License
>> + * version 2.  This program is licensed "as is" without any warranty of any
>> + * kind, whether express or implied.
>
> Please make this "either version 2 of the License, or (at your
> option) any later version'.

I'd rather not, actually.  I don't like the "at your option any later
version" clause, and I intentionally don't include it in any new code.
 This is Freescale's (unofficial) policy.

>> + * PIXIS_BASE - The virtual address of the base of the PIXIS register map
>
> Please use a C struct to desctibe the register map.

The PIXIS is not something that fits into a register map cleanly.
It's been mostly stable over time, but the hardware team reserves the
right to change any and all registers whenever they want.  The current
PIXIS code uses hard-coded offsets, and you've been accepting that.  I
don't see why the ngPIXIS should be different.

> NAK. We don't accept base + offset. Please use a C struct. Please fix
> globally.

Sure you do.  Just look at all the existing PIXIS code in board/freescale/.

>> +     /* Tell the ngPIXIS to use this the bits in the physical switch for the
>> +      * boot bank value, instead of the SWx register.  We need to be careful
>> +      * only to set the bits in SWx that correspond to the boot bank.
>> +      */
>
> Incorrect multiline comment style. Please fix globally.

What's wrong with it?  It matches the Kernel multi-line comment style.

>> +     /*
>> +      * No args is a simple reset request.
>> +      */
>> +     if (argc <= 1)
>> +             pixis_reset();
>> +             /* not reached */
>
> Braces needed here.

Really?  I'm conforming the kernel guideline here as well.

>> +U_BOOT_CMD(
>> +     pixis_reset, CONFIG_SYS_MAXARGS, 1, pixis_reset_cmd,
>> +     "Reset the board using the FPGA sequencer",
>> +     "    pixis_reset\n"
>> +     "    pixis_reset [altbank]\n"
>
> Please check the message format. I think there is one too many newline
> here.

I used the exact same message format of the original PIXIS code.

>> -#define CONFIG_FSL_PIXIS     1       /* use common PIXIS code */
>> +#define CONFIG_FSL_NGPIXIS             /* use common ngPIXIS code */
>
> Please use TABs for vertical alignment. Please fix globally.

Ok.

-- 
Timur Tabi
Linux kernel developer at Freescale