Re: [Upstream] [PATCH v2 1/2] board: phytec: common: Add product information to FTD

Hello Daniel,

On Wed, 2025-01-15 at 02:35 -0800, Daniel Schultz wrote:

ft_board_setup inside the board code allows to alter device-tree during the boot process.

Introduce a new function for the PHYTEC SOM detection to read the product name and part number from the EEPROM content and include both into the device-tree as

• phytec,som-part-number
• phytec,som-product-name

This function can be called from the board code when those values should be exposed to Linux.

Signed-off-by: Daniel Schultz d.schultz@phytec.de

Changes in v2:   * Removed 'struct bd_info' as argument in function phytec_ft_board_fixup

board/phytec/common/phytec_som_detection.c | 202 ++++++++++++++++-----  board/phytec/common/phytec_som_detection.h |   7 +  2 files changed, 166 insertions(+), 43 deletions(-)

diff --git a/board/phytec/common/phytec_som_detection.c b/board/phytec/common/phytec_som_detection.c index 166c3eae565..a4a1d20e0ec 100644 --- a/board/phytec/common/phytec_som_detection.c +++ b/board/phytec/common/phytec_som_detection.c @@ -271,11 +271,126 @@ err:   return ret;  }   +static int phytec_get_product_name(struct phytec_eeprom_data *data,

• 		   char *product)
  

phytec_eeprom_data should be const

Please add documentation to this function

+{

• struct phytec_api2_data *api2;
• unsigned int ksp_no, som_type;
• int len;
• if (!data)

• data = &eeprom_data;
  

Why do you have this in here?

• if (!data->valid || data->payload.api_rev < PHYTEC_API_REV2)

• return -EINVAL;
  

• api2 = &data->payload.data.data_api2;
• if (api2->som_type > 1 && api2->som_type <= 3) {

• ksp_no = (api2->ksp_no << 8) | api2->som_no;
  

• len = snprintf(product, PHYTEC_PRODUCT_NAME_LEN, "%s-%04u",
  

• 	       phytec_som_type_str[api2->som_type], ksp_no);
  

• if (len != 8)
  

Please use a variable here. You used snprintf, so you probably the value you expect here should be connected to the value you passed to the function.

• 	return -1;
  

Use error code?

• return 0;
  

• }
• switch (api2->som_type) {

I think some explanation/documentation on how som_type's value is determined would be useful.

• case 0:

• som_type = api2->som_type;
  

• break;
  

• case 4:

• som_type = 0;
  

• break;
  

• case 5:

• som_type = 0;
  

• break;
  

• case 6:

• som_type = 1;
  

• break;
  

• case 7:

• som_type = 1;
  

• break;
  

• default:

• pr_err("%s: Invalid SOM type: %i", __func__, api2->som_type);
  

• return -EINVAL;
  

• };
• len = snprintf(product, PHYTEC_PRODUCT_NAME_LEN, "%s-%03u",

•        phytec_som_type_str[som_type], api2->som_no);
  

• if (len != 7)

• return -1;
  

Same as above when you called snprintf.

• return 0;

+}

+static int phytec_get_part_number(struct phytec_eeprom_data *data,

• 		  char  *part)
  

phytec_eeprom_data should be const

Please add documentation to this function

+{

• char product_name[PHYTEC_PRODUCT_NAME_LEN] = {'\0'};

nitpick: C99 should support {} as initializer.

• struct phytec_api2_data *api2;
• unsigned int ksp_type;
• int res, len;
• if (!data)

• data = &eeprom_data;
  

Why is this here?

• if (!data->valid || data->payload.api_rev < PHYTEC_API_REV2)

• return -EINVAL;
  

• api2 = &data->payload.data.data_api2;
• res = phytec_get_product_name(data, product_name);
• if (res)

• return res;
  

• if (api2->som_type <= 1) {

• len = snprintf(part, PHYTEC_PART_NUMBER_LEN, "%s-%s.%s",
  

• 	       product_name, api2->opt, api2->bom_rev);
  

• if (len < 11)
  

• 	return -1;
  

Please use a variable instead of hardcoded number, see above.

• return 0;
  

• }
• if (api2->som_type <= 3) {

• snprintf(part, PHYTEC_PART_NUMBER_LEN, "%s.%s", product_name,
  

• 	 api2->bom_rev);
  

• if (len != 11)
  

• 	return -1;
  

• return 0;
  

• }
• switch (api2->som_type) {
• case 4:

• ksp_type = 3;
  

• break;
  

• case 5:

• ksp_type = 2;
  

• break;
  

• case 6:

• ksp_type = 3;
  

• break;
  

• case 7:

• ksp_type = 2;
  

• break;
  

Please add some explanation here as well.

• default:

• pr_err("%s: Invalid SOM type: %i", __func__, api2->som_type);
  

• return -EINVAL;
  

• };
• len = snprintf(part, PHYTEC_PART_NUMBER_LEN, "%s-%s%02u.%s",

•        product_name, phytec_som_type_str[ksp_type],
  

•        api2->ksp_no, api2->bom_rev);
  

• if (len < 16)

• return -1;
  

Same as for the other cases

• return 0;

+}

void __maybe_unused phytec_print_som_info(struct phytec_eeprom_data *data)  {

• char part_number[PHYTEC_PART_NUMBER_LEN] = {'\0'};

struct phytec_api2_data *api2;   char pcb_sub_rev;

• unsigned int ksp_no, sub_som_type1, sub_som_type2;

• int res;

if (!data)   data = &eeprom_data; @@ -289,50 +404,14 @@ void __maybe_unused phytec_print_som_info(struct phytec_eeprom_data *data)   pcb_sub_rev = api2->pcb_sub_opt_rev & 0x0f;   pcb_sub_rev = pcb_sub_rev ? ((pcb_sub_rev - 1) + 'a') : ' ';

• /* print standard product string */
• if (api2->som_type <= 1) {

• printf("SoM: %s-%03u-%s.%s PCB rev: %u%c\n",
  

•        phytec_som_type_str[api2->som_type], api2->som_no,
  

•        api2->opt, api2->bom_rev, api2->pcb_rev, pcb_sub_rev);
  

• res = phytec_get_part_number(data, part_number);
• if (res)

return;

• }
• /* print KSP/KSM string */
• if (api2->som_type <= 3) {

• ksp_no = (api2->ksp_no << 8) | api2->som_no;
  

• printf("SoM: %s-%u ",
  

•        phytec_som_type_str[api2->som_type], ksp_no);
  

• /* print standard product based KSP/KSM strings */
• } else {

• switch (api2->som_type) {
  

• case 4:
  

• 	sub_som_type1 = 0;
  

• 	sub_som_type2 = 3;
  

• 	break;
  

• case 5:
  

• 	sub_som_type1 = 0;
  

• 	sub_som_type2 = 2;
  

• 	break;
  

• case 6:
  

• 	sub_som_type1 = 1;
  

• 	sub_som_type2 = 3;
  

• 	break;
  

• case 7:
  

• 	sub_som_type1 = 1;
  

• 	sub_som_type2 = 2;
  

• 	break;
  

• default:
  

• 	pr_err("%s: Invalid SoM type: %i", __func__, api2->som_type);
  

• 	return;
  

• };
  

• printf("SoM: %s-%03u-%s-%03u ",
  

•        phytec_som_type_str[sub_som_type1],
  

•        api2->som_no, phytec_som_type_str[sub_som_type2],
  

•        api2->ksp_no);
  

• }

• printf("Option: %s BOM rev: %s PCB rev: %u%c\n", api2->opt,
• api2->bom_rev, api2->pcb_rev, pcb_sub_rev);

• printf("SOM: %s\n", part_number);
• printf("PCB Rev.: %u%c\n", api2->pcb_rev, pcb_sub_rev);
• if (api2->som_type > 1)

• printf("Options: %s\n", api2->opt);
  

You are changing the way infos are printed. Please put this in the commit description! These are hidden changes. Otherwise, make a separate patch where you change this.

}    char * __maybe_unused phytec_get_opt(struct phytec_eeprom_data *data) @@ -379,6 +458,37 @@ u8 __maybe_unused phytec_get_som_type(struct phytec_eeprom_data *data)   return data->payload.data.data_api2.som_type;  }   +#if IS_ENABLED(CONFIG_OF_LIBFDT) +int phytec_ft_board_fixup(struct phytec_eeprom_data *data, void *blob) +{

• char product_name[PHYTEC_PRODUCT_NAME_LEN] = {'\0'};
• char part_number[PHYTEC_PART_NUMBER_LEN] = {'\0'};
• int res;
• if (!data)

• data = &eeprom_data;
  

• if (!data->valid || data->payload.api_rev < PHYTEC_API_REV2)

• return -EINVAL;
  

• res = phytec_get_product_name(data, product_name);
• if (res)

• return res;
  

• fdt_setprop(blob, 0, "phytec,som-product-name", product_name,

•     strlen(product_name) + 1);
  

• res = phytec_get_part_number(data, part_number);
• if (res)

• return res;
  

• fdt_setprop(blob, 0, "phytec,som-part-number", part_number,

•     strlen(part_number) + 1);
  

• return 0;

+} +#endif /* IS_ENABLED(CONFIG_OF_LIBFDT) */

#if IS_ENABLED(CONFIG_CMD_EXTENSION)  struct extension *phytec_add_extension(const char *name, const char *overlay,          const char *other) @@ -458,6 +568,12 @@ inline struct phytec_api3_element * __maybe_unused   return NULL;  }   +#if IS_ENABLED(CONFIG_OF_LIBFDT) +inline int phytec_ft_board_fixup(struct phytec_eeprom_data *data, void *blob) +{

• return 0;

+} +#endif /* IS_ENABLED(CONFIG_OF_LIBFDT) */  #if IS_ENABLED(CONFIG_CMD_EXTENSION)  inline struct extension *phytec_add_extension(const char *name,         const char *overlay, diff --git a/board/phytec/common/phytec_som_detection.h b/board/phytec/common/phytec_som_detection.h index 5e35a13cb21..5b0ff9b0c89 100644 --- a/board/phytec/common/phytec_som_detection.h +++ b/board/phytec/common/phytec_som_detection.h @@ -8,6 +8,7 @@  #define _PHYTEC_SOM_DETECTION_H    #include "phytec_som_detection_blocks.h" +#include <fdtdec.h>    #define PHYTEC_MAX_OPTIONS 17  #define PHYTEC_EEPROM_INVAL 0xff @@ -17,6 +18,9 @@  #define PHYTEC_GET_OPTION(option) \   (((option) > '9') ? (option) - 'A' + 10 : (option) - '0')   +#define PHYTEC_PRODUCT_NAME_LEN 8 + 1 +#define PHYTEC_PART_NUMBER_LEN PHYTEC_PRODUCT_NAME_LEN + 14 + 1

Do you add 1 here to account for the null byte in the string?  If so, please do not do that here, instead use PHYTEC_PRODUCT_NAME_LEN +1 in the snprintf function calls. Also, defining the PHYTEC_PART_NUMBER_LEN based on PHYTEC_PRODUCT_NAME_LEN now becomes "inconsistent" since you would need to omit the +1 for the null byte you added before.

Yannic

enum {   PHYTEC_API_REV0 = 0,   PHYTEC_API_REV1, @@ -86,6 +90,9 @@ void __maybe_unused phytec_print_som_info(struct phytec_eeprom_data *data);  char * __maybe_unused phytec_get_opt(struct phytec_eeprom_data *data);  u8 __maybe_unused phytec_get_rev(struct phytec_eeprom_data *data);  u8 __maybe_unused phytec_get_som_type(struct phytec_eeprom_data *data); +#if IS_ENABLED(CONFIG_OF_LIBFDT) +int phytec_ft_board_fixup(struct phytec_eeprom_data *data, void *blob); +#endif /* IS_ENABLED(CONFIG_OF_LIBFDT) */    #if IS_ENABLED(CONFIG_CMD_EXTENSION)  struct extension *phytec_add_extension(const char *name, const char *overlay,