Re: [PATCH u-boot-mvebu v3 05/18] arm: mvebu: turris_omnia: Implement getting board information from MCU

[Stefan Roese]

On 3/28/24 12:17, Marek Behún wrote:

On Thu, 28 Mar 2024 10:56:01 +0100
Stefan Roese <s...@denx.de> wrote:

On 3/27/24 17:23, Marek Behún wrote:

Implement reading board serial number, first MAC address and board
version from MCU. MCU supports board information if the FEAT_BOARD_INFO
feature bit is set in MCU features.

Prefer getting board information from MCU if supported, fallback to
Atmel SHA chip.

Signed-off-by: Marek Behún <ka...@kernel.org>

Minor comment below. Other than this:

Reviewed-by: Stefan Roese <s...@denx.de>

Thanks,
Stefan

---
   board/CZ.NIC/turris_atsha_otp.c          | 27 +------
   board/CZ.NIC/turris_omnia/Makefile       |  2 +-
   board/CZ.NIC/turris_omnia/turris_omnia.c | 94 +++++++++++++++++++++++-
   3 files changed, 93 insertions(+), 30 deletions(-)

diff --git a/board/CZ.NIC/turris_atsha_otp.c b/board/CZ.NIC/turris_atsha_otp.c
index a29fe36231..85eebcdf18 100644
--- a/board/CZ.NIC/turris_atsha_otp.c
+++ b/board/CZ.NIC/turris_atsha_otp.c
@@ -11,6 +11,7 @@
   #include <atsha204a-i2c.h>

#include "turris_atsha_otp.h"

+#include "turris_common.h"

#define TURRIS_ATSHA_OTP_VERSION 0

   #define TURRIS_ATSHA_OTP_SERIAL              1
@@ -32,26 +33,6 @@ static struct udevice *get_atsha204a_dev(void)
        return dev;
   }

-static void increment_mac(u8 *mac)

-{
-       int i;
-
-       for (i = 5; i >= 3; i--) {
-               mac[i] += 1;
-               if (mac[i])
-                       break;
-       }
-}
-
-static void set_mac_if_invalid(int i, u8 *mac)
-{
-       u8 oldmac[6];
-
-       if (is_valid_ethaddr(mac) &&
-           !eth_env_get_enetaddr_by_index("eth", i, oldmac))
-               eth_env_set_enetaddr_by_index("eth", i, mac);
-}
-
   int turris_atsha_otp_init_mac_addresses(int first_idx)
   {
        struct udevice *dev = get_atsha204a_dev();
@@ -84,11 +65,7 @@ int turris_atsha_otp_init_mac_addresses(int first_idx)
        mac[4] = mac1[2];
        mac[5] = mac1[3];

- set_mac_if_invalid((first_idx + 0) % 3, mac);

-       increment_mac(mac);
-       set_mac_if_invalid((first_idx + 1) % 3, mac);
-       increment_mac(mac);
-       set_mac_if_invalid((first_idx + 2) % 3, mac);
+       turris_init_mac_addresses(first_idx, mac);

return 0;

   }
diff --git a/board/CZ.NIC/turris_omnia/Makefile 
b/board/CZ.NIC/turris_omnia/Makefile
index dc39b44ae1..341378b4e5 100644
--- a/board/CZ.NIC/turris_omnia/Makefile
+++ b/board/CZ.NIC/turris_omnia/Makefile
@@ -2,4 +2,4 @@
   #
   # Copyright (C) 2017 Marek Behún <ka...@kernel.org>

-obj-y := turris_omnia.o ../turris_atsha_otp.o

+obj-y  := turris_omnia.o ../turris_atsha_otp.o ../turris_common.o
diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c 
b/board/CZ.NIC/turris_omnia/turris_omnia.c
index 6dfde5ee7a..f63640ad64 100644
--- a/board/CZ.NIC/turris_omnia/turris_omnia.c
+++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
@@ -18,6 +18,7 @@
   #include <asm/io.h>
   #include <asm/arch/cpu.h>
   #include <asm/arch/soc.h>
+#include <asm/unaligned.h>
   #include <dm/uclass.h>
   #include <dt-bindings/gpio/gpio.h>
   #include <fdt_support.h>
@@ -25,12 +26,14 @@
   #include <time.h>
   #include <turris-omnia-mcu-interface.h>
   #include <linux/bitops.h>
+#include <linux/bitrev.h>
   #include <linux/delay.h>
   #include <u-boot/crc.h>

#include "../drivers/ddr/marvell/a38x/ddr3_init.h"

   #include <../serdes/a38x/high_speed_env_spec.h>
   #include "../turris_atsha_otp.h"
+#include "../turris_common.h"

DECLARE_GLOBAL_DATA_PTR; @@ -186,6 +189,70 @@ static bool omnia_mcu_has_feature(u32 feature)

        return feature & features;
   }

+static u32 omnia_mcu_crc32(const void *p, size_t len)

+{
+       u32 val, crc = 0;
+
+       compiletime_assert(!(len % 4), "length has to be a multiple of 4");
+
+       while (len) {
+               val = bitrev32(get_unaligned_le32(p));
+               crc = crc32(crc, (void *)&val, 4);
+               p += 4;
+               len -= 4;
+       }
+
+       return ~bitrev32(crc);
+}
+
+/* Can only be called after relocation, since it needs cleared BSS */
+static int omnia_mcu_board_info(char *serial, u8 *mac, char *version)
+{
+       static u8 reply[17];
+       static bool cached;
+
+       if (!cached) {
+               u8 csum;
+               int ret;
+
+               ret = omnia_mcu_read(CMD_BOARD_INFO_GET, reply, sizeof(reply));
+               if (ret)
+                       return ret;
+
+               if (reply[0] != 16)
+                       return -EBADMSG;
+
+               csum = reply[16];
+               reply[16] = 0;
+
+               if ((omnia_mcu_crc32(&reply[1], 16) & 0xff) != csum)
+                       return -EBADMSG;
+
+               cached = true;
+       }
+
+       if (serial) {
+               const char *serial_env;
+
+               serial_env = env_get("serial#");
+               if (serial_env && strlen(serial_env) == 16) {
+                       strcpy(serial, serial_env);

Usage of strcpy() is discouraged AFAIK.

Yeah, it would make sense to use strncpy, but the potential leak is
prevented by strlen() check.

strncpy is also not optimal. Please see here if you are interested:

https://www.geeksforgeeks.org/why-strcpy-and-strncpy-are-not-safe-to-use/

AFAIU, there is no pressing need to change this implementation here
though right now. I just wanted to make this comment.

Thanks,
Stefan