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

[Julius Hemanth P]

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