[U-Boot] [PATCH v3] at91sam9x5ek: Pass serial and revision tags to Linux

[Julius Hemanth P]

Hi Andreas,

On Fri, May 10, 2013 at 12:21 PM, Andreas Bießmann
<andreas.devel at googlemail.com> wrote:
> Dear Julius Hemanth P,
>
> first of all, please address Bo's comment about checkpatch:
I am sorry, Its my fault, will address them in next patch.

>
> ---8<---
> andreas at andreas-mbp % ./tools/checkpatch.pl ~/Downloads/\[PATCH\ v3\]\
> at91sam9x5ek_\ Pass\ serial\ and\ revision\ tags\ to\ Linux.eml
> ERROR: DOS line endings
> #70: FILE: board/atmel/at91sam9x5ek/at91sam9x5ek.c:30:
> +#include <asm/arch/at91_gpbr.h>^M$
>
> ERROR: patch seems to be corrupt (line wrapped?)
> #75: FILE: board/atmel/at91sam9x5ek/at91sam9x5ek.c:48:
>
> ....
>
> total: 38 errors, 7 warnings, 1 checks, 66 lines checked
>
> NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
>       scripts/cleanfile
>
> NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> MULTISTATEMENT_MACRO_USE_DO_WHILE USLEEP_RANGE
>
> /Users/andreas/Downloads/[PATCH v3] at91sam9x5ek_ Pass serial and
> revision tags to Linux.eml has style problems, please review.
>
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
> --->8---
>
> On 09.05.13 19:07, Julius Hemanth P wrote:
>> This code is small snippet from patch
>> ftp://ftp.linux4sam.org/pub/uboot/u-boot-v2010.06/u-boot-5series_1.0.patch
>>
>> Linux 2.6.39 (released on www.at91.com/linux4sam) requires serial and
>> revision ATAGs to detect NAND device.
>>
>> This patch provides backward compatibility for old Linux 2.6.39 by
>> passing serial and revision ATAGs to Linux kernel.
>>
>> Signed-off-by: Julius Hemanth <juliushemanth at gmail.com>
>> ---
>> Changes for v3:
>>         - corrected GPBR register access
>>         - updated commit message
>>
>> Changes for v2:
>>         - access GPBR using c structure
>>         - removed tailing 1 for #define
>>         - s/Miscelaneous/Miscellaneous
>>         - s/initialisations/initializations
>>
>>  board/atmel/at91sam9x5ek/at91sam9x5ek.c | 33 ++++++++++++++++++++++++++++++++-
>>  include/configs/at91sam9x5ek.h          |  5 +++++
>>  2 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/board/atmel/at91sam9x5ek/at91sam9x5ek.c
>> b/board/atmel/at91sam9x5ek/at91sam9x5ek.c
>> index 8773e6f..116bd83 100644
>> --- a/board/atmel/at91sam9x5ek/at91sam9x5ek.c
>> +++ b/board/atmel/at91sam9x5ek/at91sam9x5ek.c
>> @@ -27,6 +27,7 @@
>>  #include <asm/arch/at91_common.h>
>>  #include <asm/arch/at91_pmc.h>
>>  #include <asm/arch/at91_rstc.h>
>> +#include <asm/arch/at91_gpbr.h>
>>  #include <asm/arch/gpio.h>
>>  #include <asm/arch/clk.h>
>>  #include <lcd.h>
>> @@ -48,8 +49,34 @@ DECLARE_GLOBAL_DATA_PTR;
>>
>>  /* ------------------------------------------------------------------------- */
>>  /*
>> - * Miscelaneous platform dependent initialisations
>> + * Miscellaneous platform dependent initializations
>>   */
>> +
>> +#ifdef CONFIG_LOAD_ONE_WIRE_INFO
>
> What the hell has this one wire thing to do with reading internal GPBR's?
> This is a new config parameter (which lacks documentation in this patch)
> is not required at all cause it is enabled in any case, or do I miss
> something?
AT91Bootstrap reads board revision and serial info from 1 wire chip
and writes it to GPBR registers. U-boot, in board_init(), reads that
info from GPBR and places it in global variables in order to pass it
to Linux. At the time of boot_prep_linux(), these info will be read
from global variables and passed to linux kernel as ATAGs.

The board at91sam9x5ek I came across has this 1 wire chip connected,
But I am really not sure if this is true with all at91sam9x5ek boards,
hence I made a config parameter so that others can just enable or
disable reading rev and serial info from 1 wire chip(in this case from
GPBR registers).

>
>> +static u32 system_rev;
>> +static u32 system_serial_low;
>
> Can we please omit these global variables? We could just read the GPBR's
> in respective get-functions. This will reduce the .bss and .text size
> and is therefore reasonable.
May be yes, if we have some place to preserve these info for
processing at later stage, As of now I am really not aware of any such
struct. If you have any suggestion of one such place, please suggest
me so that I can omit these global variables. I too dislike the idea
of using global variables.

>
>> +
>> +u32 get_board_rev(void)
>> +{
>> +       return system_rev;
>> +}
>> +
>> +void get_board_serial(struct tag_serialnr *serialnr)
>> +{
>> +       serialnr->high = 0; /* Not used */
>> +       serialnr->low = system_serial_low;
>> +}
>> +
>> +void load_1wire_info(void)
>> +{
>> +       at91_gpbr_t *gpbr = (at91_gpbr_t *) ATMEL_BASE_GPBR;
>> +
>> +       /* serial is in GPBR #2 and revision is in GPBR #3 */
>
> Why is that so? Which code places the serial and revision version into
> the GPBR's?
> Please add documentation and mark the usage in at91_gpbr.h.
> As at91_gpbr.h states GPBR[3] is already used for bootcount ... so I
> tend to completely NAK this patch.
In early stage of U-boot, at the time of board_init(), we read serial
and revision info from GPBR registers, which making GPBR registers
free and available for other purpose (like bootcount). Hence, this
patch will not conflict with any part of code which plays with GPBR.

>
> As I understand Bo's comment in a prior mail this patch is only required
> for one specific kernel which is outdated. Can't you just patch the
> kernel to get the whole thing working?
Typically Linux reads serial and revision info from ATAGs, and since
U-boot was not passing serial and revision info to Linux, I thought
patch should go to u-boot code.

If you think, OK this patch make some sense, please let me know, I
shall submit patch with all corrections,

>
> Best regards
>
> Andreas Bießmann

Regards,
Julius Hemanth P.