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

[Andreas Bießmann]

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