From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: "Marek Behún" <kabel@kernel.org>
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>,
Arnd Bergmann <arnd@arndb.de>,
soc@kernel.org, arm@kernel.org,
Andy Shevchenko <andy@kernel.org>,
Hans de Goede <hdegoede@redhat.com>,
Linus Walleij <linus.walleij@linaro.org>,
Bartosz Golaszewski <brgl@bgdev.pl>,
linux-gpio@vger.kernel.org,
Alessandro Zummo <a.zummo@towertech.it>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
linux-rtc@vger.kernel.org,
Wim Van Sebroeck <wim@linux-watchdog.org>,
Guenter Roeck <linux@roeck-us.net>,
linux-watchdog@vger.kernel.org
Subject: Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
Date: Tue, 30 Apr 2024 18:31:46 +0300 (EEST) [thread overview]
Message-ID: <b90daba1-3333-0ca5-fb09-c2157f12d594@linux.intel.com> (raw)
In-Reply-To: <20240430115111.3453-3-kabel@kernel.org>
[-- 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.
next prev parent reply other threads:[~2024-04-30 15:31 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 11:51 [PATCH v8 0/9] Turris Omnia MCU driver Marek Behún
2024-04-30 11:51 ` [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU Marek Behún
2024-04-30 12:53 ` Andy Shevchenko
2024-04-30 14:05 ` Marek Behún
2024-04-30 15:10 ` Dan Carpenter
2024-04-30 15:17 ` Andy Shevchenko
2024-05-02 18:40 ` Marek Behún
2024-05-02 18:47 ` Andy Shevchenko
2024-05-02 19:17 ` Marek Behún
2024-05-03 3:59 ` Andy Shevchenko
2024-05-03 6:51 ` Marek Behún
2024-04-30 15:31 ` Ilpo Järvinen [this message]
2024-05-02 19:19 ` Marek Behún
2024-04-30 11:51 ` [PATCH v8 3/9] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs Marek Behún
2024-05-03 4:05 ` Andy Shevchenko
2024-05-03 8:28 ` Marek Behún
2024-05-03 18:51 ` Andy Shevchenko
2024-05-05 8:18 ` Marek Behún
2024-05-05 13:34 ` Andy Shevchenko
2024-05-07 17:44 ` Marek Behún
2024-05-03 8:43 ` Marek Behún
2024-05-03 17:33 ` Andy Shevchenko
2024-05-05 8:12 ` Marek Behún
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b90daba1-3333-0ca5-fb09-c2157f12d594@linux.intel.com \
--to=ilpo.jarvinen@linux.intel.com \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@bootlin.com \
--cc=andy@kernel.org \
--cc=arm@kernel.org \
--cc=arnd@arndb.de \
--cc=brgl@bgdev.pl \
--cc=gregory.clement@bootlin.com \
--cc=hdegoede@redhat.com \
--cc=kabel@kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=soc@kernel.org \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).