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

["Marek Behún" <kabel@kernel.org>]

On Thu, 28 Mar 2024 10:56:01 +0100
Stefan Roese <sr@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 <kabel@kernel.org>  
> 
> Minor comment below. Other than this:
> 
> Reviewed-by: Stefan Roese <sr@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 <kabel@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.

Marek