diff mbox

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

Message ID CAFHsovenUB6fPBKRcN4Eh6YnnFu4pKtPtbB6J9Pb_M4NdUC36Q@mail.gmail.com
State Not Applicable, archived
Delegated to: Andreas Bießmann
Headers show

Commit Message

Julius Hemanth P May 9, 2013, 5:07 p.m. UTC 
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@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(-)

--
1.8.2.2

Comments

Bo Shen May 10, 2013, 1:37 a.m. UTC | #1 
Hi Julius,

On 5/10/2013 01: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@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(-)

Sorry for forgetting to mention check your patch before you send it. 
please use ./tools/checkpatch.pl under u-boot to check your patch.

Best Regards,
Bo Shen
Andreas Bießmann May 10, 2013, 6:51 a.m. UTC | #2 
Dear Julius Hemanth P,

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

---8<---
andreas@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@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
Julius Hemanth P May 10, 2013, 11:40 a.m. UTC | #3 
Hi Andreas,

On Fri, May 10, 2013 at 12:21 PM, Andreas Bießmann
<andreas.devel@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@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@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.
Andreas Bießmann May 10, 2013, 1:38 p.m. UTC | #4 
Hi Julius,

On 05/10/2013 01:40 PM, Julius Hemanth P wrote:
> Hi Andreas,
> 
> On Fri, May 10, 2013 at 12:21 PM, Andreas Bießmann
> <andreas.devel@googlemail.com> wrote:
>> Dear Julius Hemanth P,

>> On 09.05.13 19:07, Julius Hemanth P wrote:

<snip>

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

there are a lot of outdated stuff involved ;)
 * ATAGs will be replaced by FDT
 * AT91bootstrap should be replaced with u-boot SPL

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

We have a single definition for all at91sam9x5ek variants. If there are
some which do not provide such an 1wire id chip we need to address this
here (or in at91bootstrap). I think we shouldn't force the user to
change the board config header to enable different variants. We could
introduce a new 'board name' (in boards.cfg) to address this, but as
long as we do not know that we break other boards which do _not_ have
such an 1wire id chip I think it is ok to not introduce a new config.

If we decide to introduce the config parameter we should document it
somewhere.

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

Well, my first idea was to define these places in GPBR to be
'system_rev' and 'system_serial_low', but the provided location does not
fit current definition of GPBR ...

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

board_init() runs in board_init_r(), this is way to late cause a lot of
stuff runs before, board_early_init_f() is a better place, but I dunno
if we can use .bss while in the board_init_f() phase.
Currently GPBR[3] is reserved for bootcount (in some at91 boards), the
aim of bootcount is to read the bootcount store, increment it and write
the new value. The value in the store must not be changed during power
cycle/reset of the system.
So this change breaks the current GPBR layout for this feature.

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

Thats true, but should we really rely on AT91bootstrap to provide this
information on GPBR's? If you come with a patch(set) that reads the
1wire id chip and provide the serial/version information within u-boot I
will accept that patch immediately.
The provided solution to work around problems in a specific kernel by
destroying the (currently) intended use of GPBR's in u-boot doesn't make
me happy.

I think we will need the ds24xx infos in u-boot in any case (I think
about different sama5 variants, but sam9x5 seems to be the same case).
Bo, do you have some code for that in your pipeline?

Best regards

Andreas Bießmann
Julius Hemanth P May 11, 2013, 5:01 a.m. UTC | #5 
Thanks Andreas.
In this case, I too think its better to drop this particular patch and
right a new one to support reading 1 wire Id chip.

If Bo doesn't have any code in pipeline as of now, then I shall start
working on it.

Regards,
Julius Hemanth P.
On May 10, 2013 7:07 PM, "Andreas Bießmann" <andreas.devel@googlemail.com>
wrote:

> Hi Julius,
>
> On 05/10/2013 01:40 PM, Julius Hemanth P wrote:
> > Hi Andreas,
> >
> > On Fri, May 10, 2013 at 12:21 PM, Andreas Bießmann
> > <andreas.devel@googlemail.com> wrote:
> >> Dear Julius Hemanth P,
>
> >> On 09.05.13 19:07, Julius Hemanth P wrote:
>
> <snip>
>
> >>> 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.
>
> there are a lot of outdated stuff involved ;)
>  * ATAGs will be replaced by FDT
>  * AT91bootstrap should be replaced with u-boot SPL
>
> > 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).
>
> We have a single definition for all at91sam9x5ek variants. If there are
> some which do not provide such an 1wire id chip we need to address this
> here (or in at91bootstrap). I think we shouldn't force the user to
> change the board config header to enable different variants. We could
> introduce a new 'board name' (in boards.cfg) to address this, but as
> long as we do not know that we break other boards which do _not_ have
> such an 1wire id chip I think it is ok to not introduce a new config.
>
> If we decide to introduce the config parameter we should document it
> somewhere.
>
> >>> +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.
>
> Well, my first idea was to define these places in GPBR to be
> 'system_rev' and 'system_serial_low', but the provided location does not
> fit current definition of GPBR ...
>
> >>> +
> >>> +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.
>
> board_init() runs in board_init_r(), this is way to late cause a lot of
> stuff runs before, board_early_init_f() is a better place, but I dunno
> if we can use .bss while in the board_init_f() phase.
> Currently GPBR[3] is reserved for bootcount (in some at91 boards), the
> aim of bootcount is to read the bootcount store, increment it and write
> the new value. The value in the store must not be changed during power
> cycle/reset of the system.
> So this change breaks the current GPBR layout for this feature.
>
> >> 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.
>
> Thats true, but should we really rely on AT91bootstrap to provide this
> information on GPBR's? If you come with a patch(set) that reads the
> 1wire id chip and provide the serial/version information within u-boot I
> will accept that patch immediately.
> The provided solution to work around problems in a specific kernel by
> destroying the (currently) intended use of GPBR's in u-boot doesn't make
> me happy.
>
> I think we will need the ds24xx infos in u-boot in any case (I think
> about different sama5 variants, but sam9x5 seems to be the same case).
> Bo, do you have some code for that in your pipeline?
>
> Best regards
>
> Andreas Bießmann
>
>
Bo Shen May 13, 2013, 2:39 a.m. UTC | #6 
Hi Julius,

On 5/11/2013 13:01, Julius Hemanth P wrote:
> If Bo doesn't have any code in pipeline as of now, then I shall start
> working on it.

Now, I don't have code in pipeline, you can work this on it.
Thanks.

BTW, you can take the reference code on github: 
https://github.com/linux4sam/at91bootstrap/blob/master/driver/ds24xx.c. 
This also have Atmel new SoC sama5d3 series support.

Best Regards,
Bo Shen
Julius Hemanth P May 13, 2013, 2:30 p.m. UTC | #7 
Hi Bo,

Thanks for the reference.

On Mon, May 13, 2013 at 8:09 AM, Bo Shen <voice.shen@atmel.com> wrote:
> Hi Julius,
>
>
> On 5/11/2013 13:01, Julius Hemanth P wrote:
>>
>> If Bo doesn't have any code in pipeline as of now, then I shall start
>> working on it.
>
>
> Now, I don't have code in pipeline, you can work this on it.
> Thanks.
>
> BTW, you can take the reference code on github:
> https://github.com/linux4sam/at91bootstrap/blob/master/driver/ds24xx.c. This
> also have Atmel new SoC sama5d3 series support.
>
> Best Regards,
> Bo Shen
diff mbox

Patch

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
+static u32 system_rev;
+static u32 system_serial_low;
+
+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 */
+       system_serial_low = readl(&gpbr->reg[2]);
+       system_rev = readl(&gpbr->reg[3]);
+}
+#endif
+
 #ifdef CONFIG_CMD_NAND
 static void at91sam9x5ek_nand_hw_init(void)
 {
@@ -282,6 +309,10 @@  int board_init(void)
        /* adress of boot parameters */
        gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;

+#ifdef CONFIG_LOAD_ONE_WIRE_INFO
+       load_1wire_info();
+#endif
+
 #ifdef CONFIG_CMD_NAND
        at91sam9x5ek_nand_hw_init();
 #endif
diff --git a/include/configs/at91sam9x5ek.h b/include/configs/at91sam9x5ek.h
index ee6e3fc..995e43b 100644
--- a/include/configs/at91sam9x5ek.h
+++ b/include/configs/at91sam9x5ek.h
@@ -38,6 +38,11 @@ 
 #define CONFIG_CMDLINE_TAG             /* enable passing of ATAGs */
 #define CONFIG_SETUP_MEMORY_TAGS
 #define CONFIG_INITRD_TAG
+
+#define CONFIG_LOAD_ONE_WIRE_INFO
+#define CONFIG_REVISION_TAG
+#define CONFIG_SERIAL_TAG
+
 #define CONFIG_SKIP_LOWLEVEL_INIT
 #define CONFIG_BOARD_EARLY_INIT_F
 #define CONFIG_DISPLAY_CPUINFO