| Message ID | 20231002124217.68220-2-artur@conclusive.pl |
|---|---|
| State | Superseded |
| Delegated to: | Eugen Hristev |
| Headers | show |
| Series | Conclusive KSTR-SAMA5D27 support | expand |
Hi Artur, On Mon, 2 Oct 2023 at 06:42, Artur Rojek <artur@conclusive.pl> wrote: > > Provide a generic way for boards to read their serial number from EEPROM > at init. > > If CONFIG_ID_EEPROM is set, the new serial_read_from_eeprom() function > will now be called during the post-relocation part of the board init. > > Provided is the tlv eeprom implementation of the above function, making > use of the existing, yet never utilized, populate_serial_number(). > Boards which use custom logic for interaction with their EEPROMs need to > supply their own implementation. > > Signed-off-by: Artur Rojek <artur@conclusive.pl> > --- > > v3: - restore original function name and make it static > - provide a generic function for reading EEPROM serial number and > wrap it around the existing tlv logic > - move the env var check out of populate_serial_number() and into > the new serial_read_from_eeprom() in order to stay consistent with > the documentation > > v2: - rename the function > - move function documentation from .c file to the prototype location > > cmd/tlv_eeprom.c | 25 +++++++++---------------- > common/board_r.c | 8 ++++++++ > include/init.h | 14 ++++++++++++++ > 3 files changed, 31 insertions(+), 16 deletions(-) Can you please use events for this? Something like EVT_SETTINGS_R ? See the one recently added for how to do this: INITCALL_EVENT(EVT_LAST_STAGE_INIT), Regards, Simon
>Hi Artur, > >On Mon, 2 Oct 2023 at 06:42, Artur Rojek <artur@conclusive.pl> wrote: >> >> Provide a generic way for boards to read their serial number from EEPROM >> at init. >> >> If CONFIG_ID_EEPROM is set, the new serial_read_from_eeprom() function >> will now be called during the post-relocation part of the board init. >> >> Provided is the tlv eeprom implementation of the above function, making >> use of the existing, yet never utilized, populate_serial_number(). >> Boards which use custom logic for interaction with their EEPROMs need to >> supply their own implementation. >> >> Signed-off-by: Artur Rojek <artur@conclusive.pl> >> --- >> >> v3: - restore original function name and make it static >> - provide a generic function for reading EEPROM serial number and >> wrap it around the existing tlv logic >> - move the env var check out of populate_serial_number() and into >> the new serial_read_from_eeprom() in order to stay consistent with >> the documentation >> >> v2: - rename the function >> - move function documentation from .c file to the prototype location >> >> cmd/tlv_eeprom.c | 25 +++++++++---------------- >> common/board_r.c | 8 ++++++++ >> include/init.h | 14 ++++++++++++++ >> 3 files changed, 31 insertions(+), 16 deletions(-) > >Can you please use events for this? Something like EVT_SETTINGS_R ? > >See the one recently added for how to do this: > >INITCALL_EVENT(EVT_LAST_STAGE_INIT), I like this approach, but just to be clear with your intention - you want me to move both serial_read_from_eeprom AND mac_read_from_eeprom into a separate function, defined for each affected board? To do this for mac_read_from_eeprom becomes slightly cumbersome, because there are this many of them, depending on the current scheme: $ grep -r "ID_EEPROM" ./configs/ | wc -l 55 If each of them needs to contain something like this: >static int settings_r(void) >{ >#if defined(CONFIG_ID_EEPROM) > mac_read_from_eeprom(); >#endif > return 0; >} >EVENT_SPY_SIMPLE(EVT_SETTINGS_R, settings_r); then this strays very far from the original intention of this series, which is to add support for a single board :) Unless you only care about serial_read_from_eeprom, then I don't need to modify any of the existing boards. Cheers, Artur > >Regards, >Simon
Hi Artur, On Wed, 4 Oct 2023 at 06:47, Artur Rojek <artur@conclusive.pl> wrote: > > >Hi Artur, > > > >On Mon, 2 Oct 2023 at 06:42, Artur Rojek <artur@conclusive.pl> wrote: > >> > >> Provide a generic way for boards to read their serial number from EEPROM > >> at init. > >> > >> If CONFIG_ID_EEPROM is set, the new serial_read_from_eeprom() function > >> will now be called during the post-relocation part of the board init. > >> > >> Provided is the tlv eeprom implementation of the above function, making > >> use of the existing, yet never utilized, populate_serial_number(). > >> Boards which use custom logic for interaction with their EEPROMs need to > >> supply their own implementation. > >> > >> Signed-off-by: Artur Rojek <artur@conclusive.pl> > >> --- > >> > >> v3: - restore original function name and make it static > >> - provide a generic function for reading EEPROM serial number and > >> wrap it around the existing tlv logic > >> - move the env var check out of populate_serial_number() and into > >> the new serial_read_from_eeprom() in order to stay consistent with > >> the documentation > >> > >> v2: - rename the function > >> - move function documentation from .c file to the prototype location > >> > >> cmd/tlv_eeprom.c | 25 +++++++++---------------- > >> common/board_r.c | 8 ++++++++ > >> include/init.h | 14 ++++++++++++++ > >> 3 files changed, 31 insertions(+), 16 deletions(-) > > > >Can you please use events for this? Something like EVT_SETTINGS_R ? > > > >See the one recently added for how to do this: > > > >INITCALL_EVENT(EVT_LAST_STAGE_INIT), > > I like this approach, but just to be clear with your intention - you > want me to move both serial_read_from_eeprom AND mac_read_from_eeprom > into a separate function, defined for each affected board? To do this > for mac_read_from_eeprom becomes slightly cumbersome, because there are > this many of them, depending on the current scheme: > > $ grep -r "ID_EEPROM" ./configs/ | wc -l > 55 > > If each of them needs to contain something like this: > > >static int settings_r(void) > >{ > >#if defined(CONFIG_ID_EEPROM) > > mac_read_from_eeprom(); > >#endif > > return 0; > >} > >EVENT_SPY_SIMPLE(EVT_SETTINGS_R, settings_r); > > then this strays very far from the original intention of this series, > which is to add support for a single board :) > Unless you only care about serial_read_from_eeprom, then I don't need to > modify any of the existing boards. As per irc, you don't need to adjust the mac_read_from_eeprom(). Just start your own thing (read_settings() or whatever) - i.e. don't make it any worse. People do appreciate cleanup, but sometimes it can be a big task Regards, Simon
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 79796394c5c8..9aa9b070473e 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -1088,27 +1088,12 @@ int mac_read_from_eeprom(void) return 0; } -/** - * populate_serial_number - read the serial number from EEPROM - * - * This function reads the serial number from the EEPROM and sets the - * appropriate environment variable. - * - * The environment variable is only set if it has not been set - * already. This ensures that any user-saved variables are never - * overwritten. - * - * This function must be called after relocation. - */ -int populate_serial_number(int devnum) +static int populate_serial_number(int devnum) { char serialstr[257]; int eeprom_index; struct tlvinfo_tlv *eeprom_tlv; - if (env_get("serial#")) - return 0; - if (read_eeprom(devnum, eeprom)) { printf("Read failed.\n"); return -1; @@ -1123,3 +1108,11 @@ int populate_serial_number(int devnum) return 0; } + +int serial_read_from_eeprom(void) +{ + if (env_get("serial#")) + return 0; + + return populate_serial_number(0); +} diff --git a/common/board_r.c b/common/board_r.c index 4aaa89403117..0f2cbf7bd741 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -121,6 +121,13 @@ __weak int fixup_cpu(void) return 0; } +#if defined(CONFIG_ID_EEPROM) +__weak int serial_read_from_eeprom(void) +{ + return 0; +} +#endif + static int initr_reloc_global_data(void) { #ifdef __ARM__ @@ -714,6 +721,7 @@ static init_fnc_t init_sequence_r[] = { cpu_secondary_init_r, #if defined(CONFIG_ID_EEPROM) mac_read_from_eeprom, + serial_read_from_eeprom, #endif INIT_FUNC_WATCHDOG_RESET #if defined(CONFIG_PCI_INIT_R) && !defined(CONFIG_SYS_EARLY_PCI_INIT) diff --git a/include/init.h b/include/init.h index 3bf30476a2e0..df218c95de42 100644 --- a/include/init.h +++ b/include/init.h @@ -283,6 +283,20 @@ void board_init_r(struct global_data *id, ulong dest_addr) int cpu_init_r(void); int last_stage_init(void); int mac_read_from_eeprom(void); + +/** + * serial_read_from_eeprom - read the serial number from EEPROM + * + * This function reads the serial number from the EEPROM and sets the + * appropriate environment variable. + * + * The environment variable is only set if it has not been set + * already. This ensures that any user-saved variables are never + * overwritten. + * + * This function must be called after relocation. + */ +int serial_read_from_eeprom(void); int set_cpu_clk_info(void); int update_flash_size(int flash_size); int arch_early_init_r(void);
Provide a generic way for boards to read their serial number from EEPROM at init. If CONFIG_ID_EEPROM is set, the new serial_read_from_eeprom() function will now be called during the post-relocation part of the board init. Provided is the tlv eeprom implementation of the above function, making use of the existing, yet never utilized, populate_serial_number(). Boards which use custom logic for interaction with their EEPROMs need to supply their own implementation. Signed-off-by: Artur Rojek <artur@conclusive.pl> --- v3: - restore original function name and make it static - provide a generic function for reading EEPROM serial number and wrap it around the existing tlv logic - move the env var check out of populate_serial_number() and into the new serial_read_from_eeprom() in order to stay consistent with the documentation v2: - rename the function - move function documentation from .c file to the prototype location cmd/tlv_eeprom.c | 25 +++++++++---------------- common/board_r.c | 8 ++++++++ include/init.h | 14 ++++++++++++++ 3 files changed, 31 insertions(+), 16 deletions(-)