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

[Daniel Schultz]

Hi Yannic,

I sent these patches on our internal mailing list as RFC. Please 
understand that I won't make bigger now, since we're colleges and you 
haven't reviewed in the (for you) three iterations before. So, there 
might be feedback I won't comment.

Documentation should be added as follow-up patch because it's missing 
for all function right now. Same for 'const'. I'm not a fan of adding 
that now and making this module inconsistent about how we use it.

On 16.01.25 09:06, Yannic Moog wrote:
> 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 at 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?
Removed that.
>
>
>> +
>> +	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.
Added defines for them.
>
>> +			return -1;
> Use error code?
ack.
>
>> +		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.

Moved that to the snprintf calls.

Thanks,
Daniel

>
> 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,