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" <[email protected]> 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 > > <[email protected]> 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 > >
_______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
