[PATCH v2 1/2] board: phytec: common: Add product information to FTD
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) +{ + struct phytec_api2_data *api2; + unsigned int ksp_no, som_type; + int len; + + if (!data) + data = &eeprom_data; + + 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) + return -1; + return 0; + } + + switch (api2->som_type) { + 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; + return 0; +} + +static int phytec_get_part_number(struct phytec_eeprom_data *data, + char *part) +{ + char product_name[PHYTEC_PRODUCT_NAME_LEN] = {'\0'}; + struct phytec_api2_data *api2; + unsigned int ksp_type; + int res, len; + + if (!data) + data = &eeprom_data; + + 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; + 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; + 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; + + 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); }
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 + 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,
Call 'phytec_ft_board_fixup' in the common K3 board code to expose the product name and part number to Linux.
Signed-off-by: Daniel Schultz d.schultz@phytec.de Reviewed-by: Wadim Egorov w.egorov@phytec.de ---
Changes in v2: * Removed 'return 0' right before leaving with the same return code anyways. * Added Wadim's Reviewed-by.
board/phytec/common/k3/board.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/board/phytec/common/k3/board.c b/board/phytec/common/k3/board.c index 9ff861cd3f4..3f70dcddf45 100644 --- a/board/phytec/common/k3/board.c +++ b/board/phytec/common/k3/board.c @@ -252,9 +252,21 @@ fixup_error:
int ft_board_setup(void *blob, struct bd_info *bd) { + struct phytec_eeprom_data data; + int ret; + fdt_apply_som_overlays(blob); fdt_copy_fixed_partitions(blob);
+ ret = phytec_eeprom_data_setup(&data, 0, EEPROM_ADDR); + if (ret || !data.valid) + return 0; + + ret = phytec_ft_board_fixup(&data, blob); + if (ret) + pr_err("%s: Failed to add PHYTEC information to fdt.\n", + __func__); + return 0; } #endif
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,
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@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,
Hi Daniel,
On Thu, 2025-01-23 at 14:41 +0000, Daniel Schultz wrote:
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.
I apologise for not reviewing earlier. I think improvements are worth adding and they have no expiration date.
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.
I am not asking you to change existing code/module. I am requesting you to change the signature of your newly introduced functions. The function takes pointer to data where neither the pointer nor the data must be modified. Therefore they should be const. Same for the documentation. I am fine with you moving the existing code and not adding documentation . I would like brief documentation for the newly created functions.
Yannic
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@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,
participants (3)
-
Daniel Schultz -
Daniel Schultz
-
Yannic Moog