[U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

Steven Kipisz
2015-11-03T15:13:12Z

On 11/03/2015 07:16 AM, Igor Grinberg wrote:
> Hi Steve,
>
> On 11/03/15 14:22, Steve Kipisz wrote:
>> From: Lokesh Vutla <lokeshvutla@ti.com>
>
> [...]
>
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
>> ---
>> v2 Based on
>> master a6104737 ARM: at91: sama5: change the environment address to 0x6000
>>
>> Changes in v2 (since v1)
>> - make the EEPROM code mor generic for TI EVMs
>> - rename structures/subroutines to ti_am_xxxxx
>> - add routines to access the EEPROM data
>> - redo commit message to be more clear
>>
>> v1: http://marc.info/?t=144608007900001&r=1&w=2
>> (mailing list squashed original submission)
>>
>> arch/arm/cpu/armv7/omap-common/Makefile | 1 +
>> arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c | 148 +++++++++++++++++++++++++
>> arch/arm/include/asm/omap_common.h | 130 +++++++++++++++++++++-
>> 3 files changed, 278 insertions(+), 1 deletion(-)
>> create mode 100644 arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
>>
>> diff --git a/arch/arm/cpu/armv7/omap-common/Makefile b/arch/arm/cpu/armv7/omap-common/Makefile
>> index 464a5d1d732a..53a9fdb81100 100644
>> --- a/arch/arm/cpu/armv7/omap-common/Makefile
>> +++ b/arch/arm/cpu/armv7/omap-common/Makefile
>> @@ -15,6 +15,7 @@ obj-y += clocks-common.o
>> obj-y += emif-common.o
>> obj-y += vc.o
>> obj-y += abb.o
>> +obj-$(CONFIG_I2C) += ti-i2c-eeprom.o
>
> This makes this module compile on all TI SoC based boards enabling I2C.
> AFAIU, this is a separate chip (not inside the SoC), so this module will
> also compile on non-TI boards that do not have this EEPROM.
> I think, it should be more fine grained (e.g. have its own symbol).
>
Can you give a suggestion?
>> endif
>>
>> ifneq ($(CONFIG_OMAP54XX),)
>> diff --git a/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c b/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
>> new file mode 100644
>> index 000000000000..f59ebbdb4ee8
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
>
> [...]
>
>> +void __maybe_unused set_board_info_env(char *name, char *default_name,
>> + char *revision, char *serial)
>
> That looks really weird API to me...
> You have name and default_name? Why would you need both...
>
Default to beagle_x15 if the EEPROM isn't programmed. Looking at your
other post, I think I see how to remove default_name.
>> +{
>> + char *unknown = "unknown";
>> +
>> + if (name)
>> + setenv("board_name", name);
>> + else
>> + setenv("board_name", default_name);
>> +
>> + if (revision)
>> + setenv("board_revision", revision);
>> + else
>> + setenv("board_revision", unknown);
>> +
>> + if (serial)
>> + setenv("board_serial", serial);
>> + else
>> + setenv("board_serial", unknown);
>> +}
>
> [...]
>
>> diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h
>> index d773b0430ad4..a76c67a85d37 100644
>> --- a/arch/arm/include/asm/omap_common.h
>> +++ b/arch/arm/include/asm/omap_common.h
>
> [...]
>
>> +/**
>> + * set_board_info_env() - Setup commonly used board information environment vars
>> + * @name: Name of the board
>> + * @default_name: In case of empty string, what name to use?
>
> That seems redundant.
> The caller knows the board name, and if it does not, then it can place
> an arbitrary name (like unknown) into name parameter.
>
>> + * @revision: Revision of the board
>> + * @serial: Serial Number of the board
>> + *
>> + * In case of NULL revision or serial information "unknown" is setup.
>> + * If name is NULL, default_name is used.
>> + */
>> +void set_board_info_env(char *name, char *default_name,
>> + char *revision, char *serial);
>> +
>> +/**
>> + * board_am_is() - Generic Board detection logic
>> + * @name_tag: Tag used in eeprom for the board
>> + *
>> + * Return: false if board information does not match OR eeprom was'nt read.
>> + * true otherwise
>> + */
>> +static inline bool board_am_is(char *name_tag)
>> +{
>> + struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>> +
>> + if (ep->header != TI_EEPROM_HEADER_MAGIC)
>> + return false;
>> + return !strncmp(ep->name, name_tag, TI_EEPROM_HDR_NAME_LEN);
>> +}
>
> Why do you need to place non trivial function implementation inside the
> header file?
>
>> +
>> +/**
>> + * board_am_rev_is() - Compare board revision
>> + * @rev_tag: Revision tag to check in eeprom
>> + * @cmp_len: How many chars to compare?
>> + *
>> + * NOTE: revision information is often messed up (hence the str len match) :(
>> + *
>> + * Return: false if board information does not match OR eeprom was'nt read.
>> + * true otherwise
>> + */
>> +static inline bool board_am_rev_is(char *rev_tag, int cmp_len)
>> +{
>> + struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>> + int l;
>> +
>> + if (ep->header != TI_EEPROM_HEADER_MAGIC)
>> + return false;
>> +
>> + l = cmp_len > TI_EEPROM_HDR_REV_LEN ? TI_EEPROM_HDR_NAME_LEN : cmp_len;
>> + return !strncmp(ep->version, rev_tag, l);
>> +}
>
> Same here.
>
I thought by making them static inline would save space.
>> +#endif /* __ASSEMBLY__ */
>> +
>> #endif /* _OMAP_COMMON_H_ */
>>
>
Steve K.