[PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM
Christoph Niedermaier
cniedermaier at dh-electronics.com
Wed Oct 16 14:31:06 CEST 2024
From: Marek Vasut <marex at denx.de>
Sent: Saturday, October 12, 2024 10:56 PM
> On 10/10/24 3:23 PM, Christoph Niedermaier wrote:
>> The new i.MX8M Plus DHCOM rev.200 is populated with M24C32-D EEPROM
>> which contains additional write-lockable page called ID page, which
>> is populated with a structure containing the item and serial number.
>>
>> Extend the support for parsing the item and serial number of the
>> EEPROM ID page. Write the item and serial number to the U-Boot
>> environment if the aren't there. If the environment is already
>> there compare it with the one from the EEPROM ID page and output
>> a warning if it differs.
>>
>> Signed-off-by: Christoph Niedermaier <cniedermaier at dh-electronics.com>
>> ---
>> Cc: "NXP i.MX U-Boot Team" <uboot-imx at nxp.com>
>> Cc: Marek Vasut <marex at denx.de>
>> Cc: Fabio Estevam <festevam at gmail.com>
>> Cc: Stefano Babic <sbabic at denx.de>
>> Cc: Tom Rini <trini at konsulko.com>
>> Cc: u-boot at dh-electronics.com
>> ---
>> board/dhelectronics/common/dh_common.c | 51 ++++++++++++++++++
>> board/dhelectronics/common/dh_common.h | 2 +
>> .../dh_imx8mp/imx8mp_dhcom_pdk2.c | 53 +++++++++++++++++++
>> 3 files changed, 106 insertions(+)
>>
>> diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
>> index 8ea70fc984..4c31b32e0c 100644
>> --- a/board/dhelectronics/common/dh_common.c
>> +++ b/board/dhelectronics/common/dh_common.c
>> @@ -11,6 +11,23 @@
>>
>> #include "dh_common.h"
>>
>> +/* DH item: Vendor coding */
>> +#define ITEM_PREFIX_NXP 0x01
>> +#define ITEM_PREFIX_NXP_CHR 'I'
>> +#define ITEM_PREFIX_ST 0x02
>> +#define ITEM_PREFIX_ST_CHR 'S'
>
> #define DH_ITEM_... to namespace the macros, please fix globally.
Will be fixed in v2.
>> +/*
>> + * DH item: Finished state coding
>> + * Bit = 0 means half finished
>> + * Prefix is 'H'
>> + * Bit = 1 means finished with a customer image flashed
>> + * Prefix is 'F'
>> + */
>> +#define ITEM_PREFIX_FIN_BIT BIT(7)
>> +#define ITEM_PREFIX_FIN_HALF_CHR 'H'
>> +#define ITEM_PREFIX_FIN_FLASHED_CHR 'F'
>> +
>> struct eeprom_id_page {
>> u8 id[3]; /* Identifier 'D', 'H', 'E' - 'D' is at index 0 */
>> u8 version; /* 0x10 -- Version 1.0 */
>> @@ -54,6 +71,7 @@ int dh_get_value_from_eeprom_id_page(enum eip_request_values request, u8 *data,
>> ofnode node;
>> u16 c16;
>> u8 c8;
>> + char soc;
>
> Reverse xmas tree ordering here please.
Will be fixed in v2.
>> eipp = (struct eeprom_id_page *)eipa;
>>
>> @@ -136,6 +154,39 @@ int dh_get_value_from_eeprom_id_page(enum eip_request_values request, u8 *data,
>> else
>> return -EINVAL;
>> break;
>> + case ITEM_NUMBER:
>> + if (data_len >= 8) { /* String with 7 characters + string termination */
>
> Invert this condition and reduce indent:
>
> if (data < 8)
> return -EINVAL;
>
> switch ...
> ...
Will be fixed in v2.
>> + switch (eipp->item_prefix & 0xf) {
>> + case ITEM_PREFIX_NXP:
>> + soc = ITEM_PREFIX_NXP_CHR;
>> + break;
>> + case ITEM_PREFIX_ST:
>> + soc = ITEM_PREFIX_ST_CHR;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + snprintf(data, data_len, "%c%c%05d",
>> + (eipp->item_prefix & ITEM_PREFIX_FIN_BIT) ?
>> + ITEM_PREFIX_FIN_FLASHED_CHR : ITEM_PREFIX_FIN_HALF_CHR,
>> + soc, (eipp->item_num[0] << 16) | (eipp->item_num[1] << 8)
>> + | eipp->item_num[2]);
>> + } else {
>> + return -EINVAL;
>> + }
>> + break;
>> + case SN:
>
> Use namespaced "DH_SERIAL_NUMBER":
Will be fixed in v2.
>> + /*
>> + * data_len must be greater than the size of eipp->serial,
>> + * because there is a string termination needed.
>> + */
>
> Invert this condition and reduce indent:
>
> if (data_len <= sizeof(eipp->serial))
> return -EINVAL;
>
> ...
Will be fixed in v2.
>> + if (data_len > sizeof(eipp->serial)) {
>> + data[sizeof(eipp->serial)] = 0;
>> + memcpy(data, eipp->serial, sizeof(eipp->serial));
>> + } else {
>> + return -EINVAL;
>> + }
>> + break;
>> default:
>> return -EINVAL;
>> }
>> diff --git a/board/dhelectronics/common/dh_common.h b/board/dhelectronics/common/dh_common.h
>> index 4c22ece435..1baa45e340 100644
>> --- a/board/dhelectronics/common/dh_common.h
>> +++ b/board/dhelectronics/common/dh_common.h
>> @@ -6,6 +6,8 @@
>> enum eip_request_values {
>> MAC0,
>> MAC1,
>> + ITEM_NUMBER,
>> + SN,
>
> Why is this patch not squashed into 1/2 ? It seems to be changing the
> same code.
The first patch add the reading for MAC address from the EEPROM ID
page and add the use of that addresses. The second extends the reading
to the serial number and the item number. So that the patch doesn't
get too big I found it useful to split it into two. Do you want me to
make one patch out of it?
>> };
>>
>> /*
>> diff --git a/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c b/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
>> index 9a8f09fcd4..8970c8fc2d 100644
>> --- a/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
>> +++ b/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
>> @@ -116,6 +116,56 @@ int dh_setup_mac_address(void)
>> return ret;
>> }
>>
>> +void dh_add_item_number_and_serial_to_env(void)
>> +{
>> + char item_number[8]; /* String with 7 characters + string termination */
>> + char *item_number_env;
>> + char serial[10]; /* String with 9 characters + string termination */
>> + char *serial_env;
>> + int ret;
>
> Reverse xmas tree please -- swap the first pair and second pair of
> variables.
Will be fixed in v2.
>> +
>> + ret = dh_get_value_from_eeprom_id_page(ITEM_NUMBER, item_number, sizeof(item_number),
>> + "eeprom0");
>> + if (ret) {
>> + /*
>> + * The function only returns the value -ENOENT for SoM rev.100, because
>> + * the EEPROM ID page isn't available there. Therefore the output makes
>> + * no sense and will be suppressed here.
>> + */
>> + if (ret != -ENOENT)
>> + printf("%s: Unable to get item number form EEPROM ID page! ret = %d\n",
>> + __func__, ret);
>
> This will be printed on every device, even the ones without ID EEPROM,
> correct ? This should not be printed on devices without ID EEPROM. Also,
This is suppressed by the -ENOENT check.
> if (ret && ret != -ENOENT) {}
>
> works equally well without the extra indent.
I have an else to (ret) here not to (ret && ret != -ENOENT).
This would change the logic.
>> + } else {
>> + item_number_env = env_get("vendor_item_number");
>> + if (!item_number_env)
>> + env_set("vendor_item_number", item_number);
>> + else
>> + if (strcmp(item_number_env, item_number) != 0)
>
> else if (strcmp(..., ...))
> log_warning(...)
Will be fixed in v2.
>> + printf("Warning: Environment vendor_item_number differs from EEPROM ID page value (%s != %s)\n",
>> + item_number_env, item_number);
>> + }
>> +
>> + ret = dh_get_value_from_eeprom_id_page(SN, serial, sizeof(serial), "eeprom0");
>> + if (ret) {
>> + /*
>> + * The function only returns the value -ENOENT for SoM rev.100, because
>> + * the EEPROM ID page isn't available there. Therefore the output makes
>> + * no sense and will be suppressed here.
>> + */
>> + if (ret != -ENOENT)
>> + printf("%s: Unable to get serial form EEPROM ID page! ret = %d\n",
>> + __func__, ret);
>
> See above.
Will be fixed in v2.
> Also, this shouldn't be repeatedly reading the EEPROM, the EEPROM read
> operation is the slow part, the EEPROM is 32 bytes, so the EEPROM should
> be read once and cached, and once the cache is populated all read
> accesses to the EEPROM should use the cache.
This is already covered in function dh_get_value_from_eeprom_id_page().
>> + } else {
>> + serial_env = env_get("SN");
>> + if (!serial_env)
>> + env_set("SN", serial);
>> + else
>> + if (strcmp(serial_env, serial) != 0)
>> + printf("Warning: Environment SN differs from EEPROM ID page value (%s != %s)\n",
>> + serial_env, serial);
>> + }
>> +}
>> +
>> int board_init(void)
>> {
>> return 0;
> [...]
Regards
Christoph
More information about the U-Boot
mailing list