Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 8901 bytes --]

On Tue, 30 Apr 2024, Marek Behún wrote:

> Add the basic skeleton for a new platform driver for the microcontroller
> found on the Turris Omnia board.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>

> +	struct device *dev = &mcu->client->dev;
> +	bool suggest_fw_upgrade = false;
> +	int status;
> +
> +	/* status word holds MCU type, which we need below */
> +	status = omnia_cmd_read_u16(mcu->client, CMD_GET_STATUS_WORD);
> +	if (status < 0)
> +		return status;
> +
> +	/* check whether MCU firmware supports the CMD_GET_FEAUTRES command */

FEATURES

> +	if (status & STS_FEATURES_SUPPORTED) {
> +		__le32 reply;
> +		int ret;
> +
> +		/* try read 32-bit features */
> +		ret = omnia_cmd_read(mcu->client, CMD_GET_FEATURES, &reply,
> +				     sizeof(reply));
> +		if (ret) {
> +			/* try read 16-bit features */
> +			ret = omnia_cmd_read_u16(mcu->client, CMD_GET_FEATURES);
> +			if (ret < 0)
> +				return ret;
> +
> +			mcu->features = ret;
> +		} else {
> +			mcu->features = le32_to_cpu(reply);
> +
> +			if (mcu->features & FEAT_FROM_BIT_16_INVALID)
> +				mcu->features &= GENMASK(15, 0);
> +		}

I'm not a big fan of the inconsistency here when it comes to who handles 
the endianness, perhaps there is a good reason for that but it looks a bit 
odd to have it done in a different way for 32-bit and 16-bit.

> +	} else {
> +		omnia_info_missing_feature(dev, "feature reading");
> +		suggest_fw_upgrade = true;
> +	}
> +
> +	mcu->type = omnia_status_to_mcu_type(status);
> +	dev_info(dev, "MCU type %s%s\n", mcu->type,
> +		 (mcu->features & FEAT_PERIPH_MCU) ?
> +			", with peripheral resets wired" : "");
> +
> +	omnia_mcu_print_version_hash(mcu, true);
> +
> +	if (mcu->features & FEAT_BOOTLOADER)
> +		dev_warn(dev,
> +			 "MCU is running bootloader firmware. Was firmware upgrade interrupted?\n");
> +	else
> +		omnia_mcu_print_version_hash(mcu, false);
> +
> +	for (unsigned int i = 0; i < ARRAY_SIZE(features); i++) {
> +		if (mcu->features & features[i].mask)
> +			continue;
> +
> +		omnia_info_missing_feature(dev, features[i].name);
> +		suggest_fw_upgrade = true;
> +	}
> +
> +	if (suggest_fw_upgrade)
> +		dev_info(dev,
> +			 "Consider upgrading MCU firmware with the omnia-mcutool utility.\n");
> +
> +	return 0;
> +}

> +int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
> +		   unsigned int len);
> +
> +static inline int omnia_cmd_read_u16(const struct i2c_client *client, u8 cmd)
> +{
> +	__le16 reply;
> +	int err;
> +
> +	err = omnia_cmd_read(client, cmd, &reply, sizeof(reply));
> +
> +	return err ?: le16_to_cpu(reply);
> +}
> +
> +static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
> +{
> +	u8 reply;
> +	int err;
> +
> +	err = omnia_cmd_read(client, cmd, &reply, sizeof(reply));
> +
> +	return err ?: reply;
> +}
> +
> +#endif /* __TURRIS_OMNIA_MCU_H */


> diff --git a/include/linux/turris-omnia-mcu-interface.h b/include/linux/turris-omnia-mcu-interface.h
> new file mode 100644
> index 000000000000..88f8490b5897
> --- /dev/null
> +++ b/include/linux/turris-omnia-mcu-interface.h
> @@ -0,0 +1,249 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * CZ.NIC's Turris Omnia MCU I2C interface commands definitions
> + *
> + * 2024 by Marek Behún <kabel@kernel.org>
> + */
> +
> +#ifndef __TURRIS_OMNIA_MCU_INTERFACE_H
> +#define __TURRIS_OMNIA_MCU_INTERFACE_H
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +
> +enum omnia_commands_e {
> +	CMD_GET_STATUS_WORD		= 0x01, /* slave sends status word back */
> +	CMD_GENERAL_CONTROL		= 0x02,
> +	CMD_LED_MODE			= 0x03, /* default/user */
> +	CMD_LED_STATE			= 0x04, /* LED on/off */
> +	CMD_LED_COLOR			= 0x05, /* LED number + RED + GREEN + BLUE */
> +	CMD_USER_VOLTAGE		= 0x06,
> +	CMD_SET_BRIGHTNESS		= 0x07,
> +	CMD_GET_BRIGHTNESS		= 0x08,
> +	CMD_GET_RESET			= 0x09,
> +	CMD_GET_FW_VERSION_APP		= 0x0A, /* 20B git hash number */
> +	CMD_SET_WATCHDOG_STATE		= 0x0B, /* 0 - disable
> +						 * 1 - enable / ping
> +						 * after boot watchdog is started
> +						 * with 2 minutes timeout
> +						 */
> +
> +	/* CMD_WATCHDOG_STATUS		= 0x0C, not implemented anymore */
> +
> +	CMD_GET_WATCHDOG_STATE		= 0x0D,
> +	CMD_GET_FW_VERSION_BOOT		= 0x0E, /* 20B git hash number */
> +	CMD_GET_FW_CHECKSUM		= 0x0F, /* 4B length, 4B checksum */
> +
> +	/* available if FEATURES_SUPPORTED bit set in status word */
> +	CMD_GET_FEATURES		= 0x10,
> +
> +	/* available if EXT_CMD bit set in features */
> +	CMD_GET_EXT_STATUS_DWORD	= 0x11,
> +	CMD_EXT_CONTROL			= 0x12,
> +	CMD_GET_EXT_CONTROL_STATUS	= 0x13,
> +
> +	/* available if NEW_INT_API bit set in features */
> +	CMD_GET_INT_AND_CLEAR		= 0x14,
> +	CMD_GET_INT_MASK		= 0x15,
> +	CMD_SET_INT_MASK		= 0x16,
> +
> +	/* available if FLASHING bit set in features */
> +	CMD_FLASH			= 0x19,
> +
> +	/* available if WDT_PING bit set in features */
> +	CMD_SET_WDT_TIMEOUT		= 0x20,
> +	CMD_GET_WDT_TIMELEFT		= 0x21,
> +
> +	/* available if POWEROFF_WAKEUP bit set in features */
> +	CMD_SET_WAKEUP			= 0x22,
> +	CMD_GET_UPTIME_AND_WAKEUP	= 0x23,
> +	CMD_POWER_OFF			= 0x24,
> +
> +	/* available if USB_OVC_PROT_SETTING bit set in features */
> +	CMD_SET_USB_OVC_PROT		= 0x25,
> +	CMD_GET_USB_OVC_PROT		= 0x26,
> +
> +	/* available if TRNG bit set in features */
> +	CMD_TRNG_COLLECT_ENTROPY	= 0x28,
> +
> +	/* available if CRYPTO bit set in features */
> +	CMD_CRYPTO_GET_PUBLIC_KEY	= 0x29,
> +	CMD_CRYPTO_SIGN_MESSAGE		= 0x2A,
> +	CMD_CRYPTO_COLLECT_SIGNATURE	= 0x2B,
> +
> +	/* available if BOARD_INFO it set in features */
> +	CMD_BOARD_INFO_GET		= 0x2C,
> +	CMD_BOARD_INFO_BURN		= 0x2D,
> +
> +	/* available only at address 0x2b (led-controller) */
> +	/* available only if LED_GAMMA_CORRECTION bit set in features */
> +	CMD_SET_GAMMA_CORRECTION	= 0x30,
> +	CMD_GET_GAMMA_CORRECTION	= 0x31,
> +
> +	/* available only at address 0x2b (led-controller) */
> +	/* available only if PER_LED_CORRECTION bit set in features */
> +	/* available only if FROM_BIT_16_INVALID bit NOT set in features */
> +	CMD_SET_LED_CORRECTIONS		= 0x32,
> +	CMD_GET_LED_CORRECTIONS		= 0x33,
> +};
> +
> +enum omnia_flashing_commands_e {
> +	FLASH_CMD_UNLOCK		= 0x01,
> +	FLASH_CMD_SIZE_AND_CSUM		= 0x02,
> +	FLASH_CMD_PROGRAM		= 0x03,
> +	FLASH_CMD_RESET			= 0x04,
> +};

I'm bit worried about many items above, they seem generic enough they 
could trigger name conflict at some point. Could these all defines be 
prefixed so there's no risk of collisions.

> +enum omnia_sts_word_e {
> +	STS_MCU_TYPE_MASK			= GENMASK(1, 0),
> +	STS_MCU_TYPE_STM32			= 0 << 0,
> +	STS_MCU_TYPE_GD32			= 1 << 0,
> +	STS_MCU_TYPE_MKL			= 2 << 0,

These are FIELD_PREP(STS_MCU_TYPE_MASK, x), although I suspect you need to 
use FIELD_PREP_CONST() in this context. If neither ends up working in this 
context, then leave it as is.

> +	STS_FEATURES_SUPPORTED			= BIT(2),
> +	STS_USER_REGULATOR_NOT_SUPPORTED	= BIT(3),
> +	STS_CARD_DET				= BIT(4),
> +	STS_MSATA_IND				= BIT(5),
> +	STS_USB30_OVC				= BIT(6),
> +	STS_USB31_OVC				= BIT(7),
> +	STS_USB30_PWRON				= BIT(8),
> +	STS_USB31_PWRON				= BIT(9),
> +	STS_ENABLE_4V5				= BIT(10),
> +	STS_BUTTON_MODE				= BIT(11),
> +	STS_BUTTON_PRESSED			= BIT(12),
> +	STS_BUTTON_COUNTER_MASK			= GENMASK(15, 13)
> +};
> +
> +enum omnia_ctl_byte_e {
> +	CTL_LIGHT_RST		= BIT(0),
> +	CTL_HARD_RST		= BIT(1),
> +	/* BIT(2) is currently reserved */
> +	CTL_USB30_PWRON		= BIT(3),
> +	CTL_USB31_PWRON		= BIT(4),
> +	CTL_ENABLE_4V5		= BIT(5),
> +	CTL_BUTTON_MODE		= BIT(6),
> +	CTL_BOOTLOADER		= BIT(7)
> +};
> +
> +enum omnia_features_e {
> +	FEAT_PERIPH_MCU			= BIT(0),
> +	FEAT_EXT_CMDS			= BIT(1),
> +	FEAT_WDT_PING			= BIT(2),
> +	FEAT_LED_STATE_EXT_MASK		= GENMASK(4, 3),
> +	FEAT_LED_STATE_EXT		= 1 << 3,
> +	FEAT_LED_STATE_EXT_V32		= 2 << 3,

Ditto.

> +	FEAT_LED_GAMMA_CORRECTION	= BIT(5),
> +	FEAT_NEW_INT_API		= BIT(6),
> +	FEAT_BOOTLOADER			= BIT(7),
> +	FEAT_FLASHING			= BIT(8),
> +	FEAT_NEW_MESSAGE_API		= BIT(9),
> +	FEAT_BRIGHTNESS_INT		= BIT(10),
> +	FEAT_POWEROFF_WAKEUP		= BIT(11),
> +	FEAT_CAN_OLD_MESSAGE_API	= BIT(12),
> +	FEAT_TRNG			= BIT(13),
> +	FEAT_CRYPTO			= BIT(14),
> +	FEAT_BOARD_INFO			= BIT(15),
> +
> +	/*
> +	 * Orginally the features command replied only 16 bits. If more were
> +	 * read, either the I2C transaction failed or 0xff bytes were sent.
> +	 * Therefore to consider bits 16 - 31 valid, one bit (20) was reserved
> +	 * to be zero.
> +	 */
> +
> +	/* Bits 16 - 19 correspond to bits 0 - 3 of status word */
> +	FEAT_MCU_TYPE_MASK		= GENMASK(17, 16),
> +	FEAT_MCU_TYPE_STM32		= 0 << 16,
> +	FEAT_MCU_TYPE_GD32		= 1 << 16,
> +	FEAT_MCU_TYPE_MKL		= 2 << 16,

Ditto.


-- 
 i.