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

[Andreas Bießmann]

Dear Julius Hemanth P,

first of all, please address Bo's comment about checkpatch:

---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?

> +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.

> +
> +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.

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?

Best regards

Andreas Bießmann