* [PATCH v8 0/9] Turris Omnia MCU driver
@ 2024-04-30 11:51 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 11:51 ` [PATCH v8 5/9] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog Marek Behún
0 siblings, 2 replies; 14+ messages in thread
From: Marek Behún @ 2024-04-30 11:51 UTC (permalink / raw)
To: Gregory CLEMENT, Arnd Bergmann, soc, arm, Andy Shevchenko,
Hans de Goede, Ilpo Järvinen, Alessandro Zummo,
Alexandre Belloni, Bartosz Golaszewski, Christophe JAILLET,
Dan Carpenter, devicetree, Greg Kroah-Hartman, Guenter Roeck,
Herbert Xu, Krzysztof Kozlowski, Linus Walleij, linux-crypto,
linux-gpio, linux-rtc, linux-watchdog, Olivia Mackall,
Rob Herring, Wim Van Sebroeck
Cc: Marek Behún, Andrew Lunn, Conor Dooley, Krzysztof Kozlowski,
Rob Herring, Sebastian Hesselbarth, Uwe Kleine-König
Hello Andy, Hans, Ilpo, Arnd, Gregory, and others,
this is v8 of the series adding Turris Omnia MCU driver.
This series depends on the immutable branch between LEDs and locking,
introducing devm_mutex_init(), see the PR
https://lore.kernel.org/linux-leds/20240412084616.GR2399047@google.com/
See also cover letters for v1, v2, v3, v4, v5, v6 and v7:
https://patchwork.kernel.org/project/linux-soc/cover/20230823161012.6986-1-kabel@kernel.org/
https://patchwork.kernel.org/project/linux-soc/cover/20230919103815.16818-1-kabel@kernel.org/
https://patchwork.kernel.org/project/linux-soc/cover/20231023143130.11602-1-kabel@kernel.org/
https://patchwork.kernel.org/project/linux-soc/cover/20231026161803.16750-1-kabel@kernel.org/
https://patchwork.kernel.org/project/linux-soc/cover/20240323164359.21642-1-kabel@kernel.org/
https://patchwork.kernel.org/project/linux-soc/cover/20240418121116.22184-1-kabel@kernel.org/
https://patchwork.kernel.org/project/linux-soc/cover/20240424173809.7214-1-kabel@kernel.org/
Changes since v7:
- fixed wrong $id path in DT binding (patch 1)
- removed resource managed IRQ mapping disposal, which is not needed, as
pointed out by Andy (patches 6, 7)
- added some more #includes (for linux/device.h, linux/interrupt.h,
linux/hw_random.h) (patches 3, 6, 7)
- dropped the Fixes tags from the DT changes (patches 8, 9), with an
explanation of this added into the commit message of patch 8, as
suggested by Andrew
Marek Behún (9):
dt-bindings: firmware: add cznic,turris-omnia-mcu binding
platform: cznic: Add preliminary support for Turris Omnia MCU
platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup
platform: cznic: turris-omnia-mcu: Add support for MCU watchdog
platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG
platform: cznic: turris-omnia-mcu: Add support for digital message
signing via debugfs
ARM: dts: turris-omnia: Add MCU system-controller node
ARM: dts: turris-omnia: Add GPIO key node for front button
.../ABI/testing/debugfs-turris-omnia-mcu | 13 +
.../sysfs-bus-i2c-devices-turris-omnia-mcu | 126 ++
.../firmware/cznic,turris-omnia-mcu.yaml | 86 ++
MAINTAINERS | 5 +
.../dts/marvell/armada-385-turris-omnia.dts | 35 +-
drivers/platform/Kconfig | 2 +
drivers/platform/Makefile | 1 +
drivers/platform/cznic/Kconfig | 51 +
drivers/platform/cznic/Makefile | 9 +
.../platform/cznic/turris-omnia-mcu-base.c | 439 +++++++
.../platform/cznic/turris-omnia-mcu-debugfs.c | 207 ++++
.../platform/cznic/turris-omnia-mcu-gpio.c | 1048 +++++++++++++++++
.../cznic/turris-omnia-mcu-sys-off-wakeup.c | 258 ++++
.../platform/cznic/turris-omnia-mcu-trng.c | 101 ++
.../cznic/turris-omnia-mcu-watchdog.c | 123 ++
drivers/platform/cznic/turris-omnia-mcu.h | 188 +++
include/linux/turris-omnia-mcu-interface.h | 249 ++++
17 files changed, 2940 insertions(+), 1 deletion(-)
create mode 100644 Documentation/ABI/testing/debugfs-turris-omnia-mcu
create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
create mode 100644 Documentation/devicetree/bindings/firmware/cznic,turris-omnia-mcu.yaml
create mode 100644 drivers/platform/cznic/Kconfig
create mode 100644 drivers/platform/cznic/Makefile
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-base.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-debugfs.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-gpio.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-trng.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-watchdog.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu.h
create mode 100644 include/linux/turris-omnia-mcu-interface.h
--
2.43.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
2024-04-30 11:51 [PATCH v8 0/9] Turris Omnia MCU driver Marek Behún
@ 2024-04-30 11:51 ` Marek Behún
2024-04-30 12:53 ` Andy Shevchenko
2024-04-30 15:31 ` Ilpo Järvinen
2024-04-30 11:51 ` [PATCH v8 5/9] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog Marek Behún
1 sibling, 2 replies; 14+ messages in thread
From: Marek Behún @ 2024-04-30 11:51 UTC (permalink / raw)
To: Gregory CLEMENT, Arnd Bergmann, soc, arm, Andy Shevchenko,
Hans de Goede, Ilpo Järvinen, Linus Walleij,
Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
linux-watchdog
Cc: Marek Behún
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>
---
.../sysfs-bus-i2c-devices-turris-omnia-mcu | 81 ++++
MAINTAINERS | 3 +
drivers/platform/Kconfig | 2 +
drivers/platform/Makefile | 1 +
drivers/platform/cznic/Kconfig | 26 ++
drivers/platform/cznic/Makefile | 4 +
.../platform/cznic/turris-omnia-mcu-base.c | 376 ++++++++++++++++++
drivers/platform/cznic/turris-omnia-mcu.h | 50 +++
include/linux/turris-omnia-mcu-interface.h | 249 ++++++++++++
9 files changed, 792 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
create mode 100644 drivers/platform/cznic/Kconfig
create mode 100644 drivers/platform/cznic/Makefile
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-base.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu.h
create mode 100644 include/linux/turris-omnia-mcu-interface.h
diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
new file mode 100644
index 000000000000..ff5e7cb00206
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
@@ -0,0 +1,81 @@
+What: /sys/bus/i2c/devices/<mcu_device>/board_revision
+Date: July 2024
+KernelVersion: 6.10
+Contact: Marek Behún <kabel@kernel.org>
+Description: (RO) Contains board revision number.
+
+ Only available if board information is burned in the MCU (older
+ revisions have board information burned in the ATSHA204-A chip).
+
+ Format: %u.
+
+What: /sys/bus/i2c/devices/<mcu_device>/first_mac_address
+Date: July 2024
+KernelVersion: 6.10
+Contact: Marek Behún <kabel@kernel.org>
+Description: (RO) Contains device first MAC address. Each Turris Omnia is
+ allocated 3 MAC addresses. The two additional addresses are
+ computed from the first one by incrementing it.
+
+ Only available if board information is burned in the MCU (older
+ revisions have board information burned in the ATSHA204-A chip).
+
+ Format: %pM.
+
+What: /sys/bus/i2c/devices/<mcu_device>/fw_features
+Date: July 2024
+KernelVersion: 6.10
+Contact: Marek Behún <kabel@kernel.org>
+Description: (RO) Newer versions of the microcontroller firmware report the
+ features they support. These can be read from this file. If the
+ MCU firmware is too old, this file reads 0x0.
+
+ Format: 0x%x.
+
+What: /sys/bus/i2c/devices/<mcu_device>/fw_version_hash_application
+Date: July 2024
+KernelVersion: 6.10
+Contact: Marek Behún <kabel@kernel.org>
+Description: (RO) Contains the version hash (commit hash) of the application
+ part of the microcontroller firmware.
+
+ Format: %s.
+
+What: /sys/bus/i2c/devices/<mcu_device>/fw_version_hash_bootloader
+Date: July 2024
+KernelVersion: 6.10
+Contact: Marek Behún <kabel@kernel.org>
+Description: (RO) Contains the version hash (commit hash) of the bootloader
+ part of the microcontroller firmware.
+
+ Format: %s.
+
+What: /sys/bus/i2c/devices/<mcu_device>/mcu_type
+Date: July 2024
+KernelVersion: 6.10
+Contact: Marek Behún <kabel@kernel.org>
+Description: (RO) Contains the microcontroller type (STM32, GD32, MKL).
+
+ Format: %s.
+
+What: /sys/bus/i2c/devices/<mcu_device>/reset_selector
+Date: July 2024
+KernelVersion: 6.10
+Contact: Marek Behún <kabel@kernel.org>
+Description: (RO) Contains the selected factory reset level, determined by
+ how long the rear reset button was held by the user during board
+ reset.
+
+ Format: %i.
+
+What: /sys/bus/i2c/devices/<mcu_device>/serial_number
+Date: July 2024
+KernelVersion: 6.10
+Contact: Marek Behún <kabel@kernel.org>
+Description: (RO) Contains the 64 bit long board serial number in hexadecimal
+ format.
+
+ Only available if board information is burned in the MCU (older
+ revisions have board information burned in the ATSHA204-A chip).
+
+ Format: %016X.
diff --git a/MAINTAINERS b/MAINTAINERS
index 359ed2cadfc4..2aa178a9d53e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2139,6 +2139,7 @@ M: Marek Behún <kabel@kernel.org>
S: Maintained
W: https://www.turris.cz/
F: Documentation/ABI/testing/debugfs-moxtet
+F: Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
F: Documentation/ABI/testing/sysfs-bus-moxtet-devices
F: Documentation/ABI/testing/sysfs-firmware-turris-mox-rwtm
F: Documentation/devicetree/bindings/bus/moxtet.txt
@@ -2152,10 +2153,12 @@ F: drivers/firmware/turris-mox-rwtm.c
F: drivers/gpio/gpio-moxtet.c
F: drivers/leds/leds-turris-omnia.c
F: drivers/mailbox/armada-37xx-rwtm-mailbox.c
+F: drivers/platform/cznic/
F: drivers/watchdog/armada_37xx_wdt.c
F: include/dt-bindings/bus/moxtet.h
F: include/linux/armada-37xx-rwtm-mailbox.h
F: include/linux/moxtet.h
+F: include/linux/turris-omnia-mcu-interface.h
ARM/FARADAY FA526 PORT
M: Hans Ulli Kroll <ulli.kroll@googlemail.com>
diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
index 868b20361769..fef907a94001 100644
--- a/drivers/platform/Kconfig
+++ b/drivers/platform/Kconfig
@@ -7,6 +7,8 @@ source "drivers/platform/goldfish/Kconfig"
source "drivers/platform/chrome/Kconfig"
+source "drivers/platform/cznic/Kconfig"
+
source "drivers/platform/mellanox/Kconfig"
source "drivers/platform/olpc/Kconfig"
diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile
index 41640172975a..8bf189264374 100644
--- a/drivers/platform/Makefile
+++ b/drivers/platform/Makefile
@@ -10,4 +10,5 @@ obj-$(CONFIG_MIPS) += mips/
obj-$(CONFIG_OLPC_EC) += olpc/
obj-$(CONFIG_GOLDFISH) += goldfish/
obj-$(CONFIG_CHROME_PLATFORMS) += chrome/
+obj-$(CONFIG_CZNIC_PLATFORMS) += cznic/
obj-$(CONFIG_SURFACE_PLATFORMS) += surface/
diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig
new file mode 100644
index 000000000000..f8560ff9c1af
--- /dev/null
+++ b/drivers/platform/cznic/Kconfig
@@ -0,0 +1,26 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# For a description of the syntax of this configuration file,
+# see Documentation/kbuild/kconfig-language.rst.
+#
+
+menuconfig CZNIC_PLATFORMS
+ bool "Platform support for CZ.NIC's Turris hardware"
+ depends on MACH_ARMADA_38X || COMPILE_TEST
+ help
+ Say Y here to be able to choose driver support for CZ.NIC's Turris
+ devices. This option alone does not add any kernel code.
+
+if CZNIC_PLATFORMS
+
+config TURRIS_OMNIA_MCU
+ tristate "Turris Omnia MCU driver"
+ depends on MACH_ARMADA_38X || COMPILE_TEST
+ depends on I2C
+ help
+ Say Y here to add support for the features implemented by the
+ microcontroller on the CZ.NIC's Turris Omnia SOHO router.
+ To compile this driver as a module, choose M here; the module will be
+ called turris-omnia-mcu.
+
+endif # CZNIC_PLATFORMS
diff --git a/drivers/platform/cznic/Makefile b/drivers/platform/cznic/Makefile
new file mode 100644
index 000000000000..31adca73bb94
--- /dev/null
+++ b/drivers/platform/cznic/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_TURRIS_OMNIA_MCU) += turris-omnia-mcu.o
+turris-omnia-mcu-y := turris-omnia-mcu-base.o
diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c
new file mode 100644
index 000000000000..8893c990853a
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
@@ -0,0 +1,376 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CZ.NIC's Turris Omnia MCU driver
+ *
+ * 2024 by Marek Behún <kabel@kernel.org>
+ */
+
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/hex.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/turris-omnia-mcu-interface.h>
+#include <linux/types.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+
+#include "turris-omnia-mcu.h"
+
+#define OMNIA_FW_VERSION_LEN 20
+#define OMNIA_FW_VERSION_HEX_LEN (2 * OMNIA_FW_VERSION_LEN + 1)
+#define OMNIA_BOARD_INFO_LEN 16
+
+int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
+ unsigned int len)
+{
+ struct i2c_msg msgs[2];
+ int ret;
+
+ msgs[0].addr = client->addr;
+ msgs[0].flags = 0;
+ msgs[0].len = 1;
+ msgs[0].buf = &cmd;
+ msgs[1].addr = client->addr;
+ msgs[1].flags = I2C_M_RD;
+ msgs[1].len = len;
+ msgs[1].buf = reply;
+
+ ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (ret < 0)
+ return ret;
+ if (ret != ARRAY_SIZE(msgs))
+ return -EIO;
+
+ return 0;
+}
+
+static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
+ u8 version[static OMNIA_FW_VERSION_HEX_LEN])
+{
+ u8 reply[OMNIA_FW_VERSION_LEN];
+ int err;
+
+ err = omnia_cmd_read(mcu->client,
+ bootloader ? CMD_GET_FW_VERSION_BOOT
+ : CMD_GET_FW_VERSION_APP,
+ reply, sizeof(reply));
+ if (err)
+ return err;
+
+ version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
+ bin2hex(version, reply, OMNIA_FW_VERSION_LEN);
+
+ return 0;
+}
+
+static ssize_t fw_version_hash_show(struct device *dev, char *buf,
+ bool bootloader)
+{
+ struct omnia_mcu *mcu = dev_get_drvdata(dev);
+ u8 version[OMNIA_FW_VERSION_HEX_LEN];
+ int err;
+
+ err = omnia_get_version_hash(mcu, bootloader, version);
+ if (err)
+ return err;
+
+ return sysfs_emit(buf, "%s\n", version);
+}
+
+static ssize_t fw_version_hash_application_show(struct device *dev,
+ struct device_attribute *a,
+ char *buf)
+{
+ return fw_version_hash_show(dev, buf, false);
+}
+static DEVICE_ATTR_RO(fw_version_hash_application);
+
+static ssize_t fw_version_hash_bootloader_show(struct device *dev,
+ struct device_attribute *a,
+ char *buf)
+{
+ return fw_version_hash_show(dev, buf, true);
+}
+static DEVICE_ATTR_RO(fw_version_hash_bootloader);
+
+static ssize_t fw_features_show(struct device *dev, struct device_attribute *a,
+ char *buf)
+{
+ struct omnia_mcu *mcu = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "0x%x\n", mcu->features);
+}
+static DEVICE_ATTR_RO(fw_features);
+
+static ssize_t mcu_type_show(struct device *dev, struct device_attribute *a,
+ char *buf)
+{
+ struct omnia_mcu *mcu = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%s\n", mcu->type);
+}
+static DEVICE_ATTR_RO(mcu_type);
+
+static ssize_t reset_selector_show(struct device *dev,
+ struct device_attribute *a, char *buf)
+{
+ int ret;
+
+ ret = omnia_cmd_read_u8(to_i2c_client(dev), CMD_GET_RESET);
+ if (ret < 0)
+ return ret;
+
+ return sysfs_emit(buf, "%d\n", ret);
+}
+static DEVICE_ATTR_RO(reset_selector);
+
+static ssize_t serial_number_show(struct device *dev,
+ struct device_attribute *a, char *buf)
+{
+ struct omnia_mcu *mcu = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%016llX\n", mcu->board_serial_number);
+}
+static DEVICE_ATTR_RO(serial_number);
+
+static ssize_t first_mac_address_show(struct device *dev,
+ struct device_attribute *a, char *buf)
+{
+ struct omnia_mcu *mcu = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%pM\n", mcu->board_first_mac);
+}
+static DEVICE_ATTR_RO(first_mac_address);
+
+static ssize_t board_revision_show(struct device *dev,
+ struct device_attribute *a, char *buf)
+{
+ struct omnia_mcu *mcu = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%u\n", mcu->board_revision);
+}
+static DEVICE_ATTR_RO(board_revision);
+
+static struct attribute *omnia_mcu_base_attrs[] = {
+ &dev_attr_fw_version_hash_application.attr,
+ &dev_attr_fw_version_hash_bootloader.attr,
+ &dev_attr_fw_features.attr,
+ &dev_attr_mcu_type.attr,
+ &dev_attr_reset_selector.attr,
+ &dev_attr_serial_number.attr,
+ &dev_attr_first_mac_address.attr,
+ &dev_attr_board_revision.attr,
+ NULL
+};
+
+static umode_t omnia_mcu_base_attrs_visible(struct kobject *kobj,
+ struct attribute *a, int n)
+{
+ struct device *dev = kobj_to_dev(kobj);
+ struct omnia_mcu *mcu = dev_get_drvdata(dev);
+ umode_t mode = a->mode;
+
+ if ((a == &dev_attr_serial_number.attr ||
+ a == &dev_attr_first_mac_address.attr ||
+ a == &dev_attr_board_revision.attr) &&
+ !(mcu->features & FEAT_BOARD_INFO))
+ mode = 0;
+
+ return mode;
+}
+
+static const struct attribute_group omnia_mcu_base_group = {
+ .attrs = omnia_mcu_base_attrs,
+ .is_visible = omnia_mcu_base_attrs_visible,
+};
+
+static const struct attribute_group *omnia_mcu_groups[] = {
+ &omnia_mcu_base_group,
+ NULL
+};
+
+static void omnia_mcu_print_version_hash(struct omnia_mcu *mcu, bool bootloader)
+{
+ const char *type = bootloader ? "bootloader" : "application";
+ struct device *dev = &mcu->client->dev;
+ u8 version[OMNIA_FW_VERSION_HEX_LEN];
+ int err;
+
+ err = omnia_get_version_hash(mcu, bootloader, version);
+ if (err) {
+ dev_err(dev, "Cannot read MCU %s firmware version: %d\n", type,
+ err);
+ return;
+ }
+
+ dev_info(dev, "MCU %s firmware version hash: %s\n", type, version);
+}
+
+static const char *omnia_status_to_mcu_type(uint16_t status)
+{
+ switch (status & STS_MCU_TYPE_MASK) {
+ case STS_MCU_TYPE_STM32:
+ return "STM32";
+ case STS_MCU_TYPE_GD32:
+ return "GD32";
+ case STS_MCU_TYPE_MKL:
+ return "MKL";
+ default:
+ return "unknown";
+ }
+}
+
+static void omnia_info_missing_feature(struct device *dev, const char *feature)
+{
+ dev_info(dev,
+ "Your board's MCU firmware does not support the %s feature.\n",
+ feature);
+}
+
+static int omnia_mcu_read_features(struct omnia_mcu *mcu)
+{
+ static const struct {
+ uint16_t mask;
+ const char *name;
+ } features[] = {
+ { FEAT_EXT_CMDS, "extended control and status" },
+ { FEAT_WDT_PING, "watchdog pinging" },
+ { FEAT_LED_STATE_EXT_MASK, "peripheral LED pins reading" },
+ { FEAT_NEW_INT_API, "new interrupt API" },
+ { FEAT_POWEROFF_WAKEUP, "poweroff and wakeup" },
+ { FEAT_TRNG, "true random number generator" },
+ };
+ 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 */
+ 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);
+ }
+ } 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;
+}
+
+static int omnia_mcu_read_board_info(struct omnia_mcu *mcu)
+{
+ u8 reply[1 + OMNIA_BOARD_INFO_LEN];
+ int err;
+
+ err = omnia_cmd_read(mcu->client, CMD_BOARD_INFO_GET, reply,
+ sizeof(reply));
+ if (err)
+ return err;
+
+ if (reply[0] != OMNIA_BOARD_INFO_LEN)
+ return -EIO;
+
+ mcu->board_serial_number = get_unaligned_le64(&reply[1]);
+ memcpy(mcu->board_first_mac, &reply[9], ETH_ALEN);
+ mcu->board_revision = reply[15];
+
+ return 0;
+}
+
+static int omnia_mcu_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct omnia_mcu *mcu;
+ int err;
+
+ if (!client->irq)
+ return dev_err_probe(dev, -EINVAL, "IRQ resource not found\n");
+
+ mcu = devm_kzalloc(dev, sizeof(*mcu), GFP_KERNEL);
+ if (!mcu)
+ return -ENOMEM;
+
+ mcu->client = client;
+ i2c_set_clientdata(client, mcu);
+
+ err = omnia_mcu_read_features(mcu);
+ if (err)
+ return dev_err_probe(dev, err,
+ "Cannot determine MCU supported features\n");
+
+ if (mcu->features & FEAT_BOARD_INFO) {
+ err = omnia_mcu_read_board_info(mcu);
+ if (err)
+ return dev_err_probe(dev, err,
+ "Cannot read board info\n");
+ }
+
+ return 0;
+}
+
+static const struct of_device_id of_omnia_mcu_match[] = {
+ { .compatible = "cznic,turris-omnia-mcu" },
+ {}
+};
+
+static struct i2c_driver omnia_mcu_driver = {
+ .probe = omnia_mcu_probe,
+ .driver = {
+ .name = "turris-omnia-mcu",
+ .of_match_table = of_omnia_mcu_match,
+ .dev_groups = omnia_mcu_groups,
+ },
+};
+module_i2c_driver(omnia_mcu_driver);
+
+MODULE_AUTHOR("Marek Behun <kabel@kernel.org>");
+MODULE_DESCRIPTION("CZ.NIC's Turris Omnia MCU");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h
new file mode 100644
index 000000000000..5b21fbca8fe1
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * CZ.NIC's Turris Omnia MCU driver
+ *
+ * 2024 by Marek Behún <kabel@kernel.org>
+ */
+
+#ifndef __TURRIS_OMNIA_MCU_H
+#define __TURRIS_OMNIA_MCU_H
+
+#include <linux/i2c.h>
+#include <linux/if_ether.h>
+#include <linux/types.h>
+#include <asm/byteorder.h>
+
+struct omnia_mcu {
+ struct i2c_client *client;
+ const char *type;
+ u32 features;
+
+ /* board information */
+ u64 board_serial_number;
+ u8 board_first_mac[ETH_ALEN];
+ u8 board_revision;
+};
+
+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,
+};
+
+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,
+ 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,
+ 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,
+ FEAT_FEATURES_SUPPORTED = BIT(18),
+ FEAT_USER_REGULATOR_NOT_SUPPORTED = BIT(19),
+
+ /* must not be set */
+ FEAT_FROM_BIT_16_INVALID = BIT(20),
+
+ FEAT_PER_LED_CORRECTION = BIT(21),
+ FEAT_USB_OVC_PROT_SETTING = BIT(22),
+};
+
+enum omnia_ext_sts_dword_e {
+ EXT_STS_SFP_nDET = BIT(0),
+ EXT_STS_LED_STATES_MASK = GENMASK(31, 12),
+ EXT_STS_WLAN0_MSATA_LED = BIT(12),
+ EXT_STS_WLAN1_LED = BIT(13),
+ EXT_STS_WLAN2_LED = BIT(14),
+ EXT_STS_WPAN0_LED = BIT(15),
+ EXT_STS_WPAN1_LED = BIT(16),
+ EXT_STS_WPAN2_LED = BIT(17),
+ EXT_STS_WAN_LED0 = BIT(18),
+ EXT_STS_WAN_LED1 = BIT(19),
+ EXT_STS_LAN0_LED0 = BIT(20),
+ EXT_STS_LAN0_LED1 = BIT(21),
+ EXT_STS_LAN1_LED0 = BIT(22),
+ EXT_STS_LAN1_LED1 = BIT(23),
+ EXT_STS_LAN2_LED0 = BIT(24),
+ EXT_STS_LAN2_LED1 = BIT(25),
+ EXT_STS_LAN3_LED0 = BIT(26),
+ EXT_STS_LAN3_LED1 = BIT(27),
+ EXT_STS_LAN4_LED0 = BIT(28),
+ EXT_STS_LAN4_LED1 = BIT(29),
+ EXT_STS_LAN5_LED0 = BIT(30),
+ EXT_STS_LAN5_LED1 = BIT(31),
+};
+
+enum omnia_ext_ctl_e {
+ EXT_CTL_nRES_MMC = BIT(0),
+ EXT_CTL_nRES_LAN = BIT(1),
+ EXT_CTL_nRES_PHY = BIT(2),
+ EXT_CTL_nPERST0 = BIT(3),
+ EXT_CTL_nPERST1 = BIT(4),
+ EXT_CTL_nPERST2 = BIT(5),
+ EXT_CTL_PHY_SFP = BIT(6),
+ EXT_CTL_PHY_SFP_AUTO = BIT(7),
+ EXT_CTL_nVHV_CTRL = BIT(8),
+};
+
+enum omnia_int_e {
+ INT_CARD_DET = BIT(0),
+ INT_MSATA_IND = BIT(1),
+ INT_USB30_OVC = BIT(2),
+ INT_USB31_OVC = BIT(3),
+ INT_BUTTON_PRESSED = BIT(4),
+ INT_SFP_nDET = BIT(5),
+ INT_BRIGHTNESS_CHANGED = BIT(6),
+ INT_TRNG = BIT(7),
+ INT_MESSAGE_SIGNED = BIT(8),
+
+ INT_LED_STATES_MASK = GENMASK(31, 12),
+ INT_WLAN0_MSATA_LED = BIT(12),
+ INT_WLAN1_LED = BIT(13),
+ INT_WLAN2_LED = BIT(14),
+ INT_WPAN0_LED = BIT(15),
+ INT_WPAN1_LED = BIT(16),
+ INT_WPAN2_LED = BIT(17),
+ INT_WAN_LED0 = BIT(18),
+ INT_WAN_LED1 = BIT(19),
+ INT_LAN0_LED0 = BIT(20),
+ INT_LAN0_LED1 = BIT(21),
+ INT_LAN1_LED0 = BIT(22),
+ INT_LAN1_LED1 = BIT(23),
+ INT_LAN2_LED0 = BIT(24),
+ INT_LAN2_LED1 = BIT(25),
+ INT_LAN3_LED0 = BIT(26),
+ INT_LAN3_LED1 = BIT(27),
+ INT_LAN4_LED0 = BIT(28),
+ INT_LAN4_LED1 = BIT(29),
+ INT_LAN5_LED0 = BIT(30),
+ INT_LAN5_LED1 = BIT(31),
+};
+
+enum omnia_cmd_poweroff_e {
+ CMD_POWER_OFF_POWERON_BUTTON = BIT(0),
+ CMD_POWER_OFF_MAGIC = 0xdead,
+};
+
+enum cmd_usb_ovc_prot_e {
+ CMD_xET_USB_OVC_PROT_PORT_MASK = GENMASK(3, 0),
+ CMD_xET_USB_OVC_PROT_ENABLE = BIT(4),
+};
+
+#endif /* __TURRIS_OMNIA_MCU_INTERFACE_H */
--
2.43.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v8 5/9] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog
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 11:51 ` Marek Behún
1 sibling, 0 replies; 14+ messages in thread
From: Marek Behún @ 2024-04-30 11:51 UTC (permalink / raw)
To: Gregory CLEMENT, Arnd Bergmann, soc, arm, Andy Shevchenko,
Hans de Goede, Ilpo Järvinen, Bartosz Golaszewski,
Guenter Roeck, Linus Walleij, linux-watchdog, Wim Van Sebroeck
Cc: Marek Behún
Add support for the watchdog mechanism provided by the MCU.
Signed-off-by: Marek Behún <kabel@kernel.org>
---
drivers/platform/cznic/Kconfig | 2 +
drivers/platform/cznic/Makefile | 1 +
.../platform/cznic/turris-omnia-mcu-base.c | 4 +
.../cznic/turris-omnia-mcu-watchdog.c | 123 ++++++++++++++++++
drivers/platform/cznic/turris-omnia-mcu.h | 24 ++++
5 files changed, 154 insertions(+)
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-watchdog.c
diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig
index 0a752aa654fa..e2649cdecc38 100644
--- a/drivers/platform/cznic/Kconfig
+++ b/drivers/platform/cznic/Kconfig
@@ -20,6 +20,7 @@ config TURRIS_OMNIA_MCU
select GPIOLIB
select GPIOLIB_IRQCHIP
select RTC_CLASS
+ select WATCHDOG_CORE
help
Say Y here to add support for the features implemented by the
microcontroller on the CZ.NIC's Turris Omnia SOHO router.
@@ -27,6 +28,7 @@ config TURRIS_OMNIA_MCU
- board poweroff into true low power mode (with voltage regulators
disabled) and the ability to configure wake up from this mode (via
rtcwake)
+ - MCU watchdog
- GPIO pins
- to get front button press events (the front button can be
configured either to generate press events to the CPU or to change
diff --git a/drivers/platform/cznic/Makefile b/drivers/platform/cznic/Makefile
index a185ef882e44..687f7718c0a1 100644
--- a/drivers/platform/cznic/Makefile
+++ b/drivers/platform/cznic/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_TURRIS_OMNIA_MCU) += turris-omnia-mcu.o
turris-omnia-mcu-y := turris-omnia-mcu-base.o
turris-omnia-mcu-y += turris-omnia-mcu-gpio.o
turris-omnia-mcu-y += turris-omnia-mcu-sys-off-wakeup.o
+turris-omnia-mcu-y += turris-omnia-mcu-watchdog.o
diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c
index 697ace090bda..5f88119d825c 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-base.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
@@ -365,6 +365,10 @@ static int omnia_mcu_probe(struct i2c_client *client)
if (err)
return err;
+ err = omnia_mcu_register_watchdog(mcu);
+ if (err)
+ return err;
+
return omnia_mcu_register_gpiochip(mcu);
}
diff --git a/drivers/platform/cznic/turris-omnia-mcu-watchdog.c b/drivers/platform/cznic/turris-omnia-mcu-watchdog.c
new file mode 100644
index 000000000000..b0de9585da84
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu-watchdog.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CZ.NIC's Turris Omnia MCU watchdog driver
+ *
+ * 2024 by Marek Behún <kabel@kernel.org>
+ */
+
+#include <linux/i2c.h>
+#include <linux/moduleparam.h>
+#include <linux/turris-omnia-mcu-interface.h>
+#include <linux/types.h>
+#include <linux/watchdog.h>
+
+#include "turris-omnia-mcu.h"
+
+#define WATCHDOG_TIMEOUT 120
+
+static unsigned int timeout;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static int omnia_wdt_start(struct watchdog_device *wdt)
+{
+ struct omnia_mcu *mcu = watchdog_get_drvdata(wdt);
+
+ return omnia_cmd_write_u8(mcu->client, CMD_SET_WATCHDOG_STATE, 1);
+}
+
+static int omnia_wdt_stop(struct watchdog_device *wdt)
+{
+ struct omnia_mcu *mcu = watchdog_get_drvdata(wdt);
+
+ return omnia_cmd_write_u8(mcu->client, CMD_SET_WATCHDOG_STATE, 0);
+}
+
+static int omnia_wdt_ping(struct watchdog_device *wdt)
+{
+ struct omnia_mcu *mcu = watchdog_get_drvdata(wdt);
+
+ return omnia_cmd_write_u8(mcu->client, CMD_SET_WATCHDOG_STATE, 1);
+}
+
+static int omnia_wdt_set_timeout(struct watchdog_device *wdt,
+ unsigned int timeout)
+{
+ struct omnia_mcu *mcu = watchdog_get_drvdata(wdt);
+
+ return omnia_cmd_write_u16(mcu->client, CMD_SET_WDT_TIMEOUT,
+ timeout * 10);
+}
+
+static unsigned int omnia_wdt_get_timeleft(struct watchdog_device *wdt)
+{
+ struct omnia_mcu *mcu = watchdog_get_drvdata(wdt);
+ int ret;
+
+ ret = omnia_cmd_read_u16(mcu->client, CMD_GET_WDT_TIMELEFT);
+ if (ret < 0) {
+ dev_err(&mcu->client->dev, "Cannot get watchdog timeleft: %d\n",
+ ret);
+ return 0;
+ }
+
+ return ret / 10;
+}
+
+static const struct watchdog_info omnia_wdt_info = {
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+ .identity = "Turris Omnia MCU Watchdog",
+};
+
+static const struct watchdog_ops omnia_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = omnia_wdt_start,
+ .stop = omnia_wdt_stop,
+ .ping = omnia_wdt_ping,
+ .set_timeout = omnia_wdt_set_timeout,
+ .get_timeleft = omnia_wdt_get_timeleft,
+};
+
+int omnia_mcu_register_watchdog(struct omnia_mcu *mcu)
+{
+ struct device *dev = &mcu->client->dev;
+ int ret;
+
+ if (!(mcu->features & FEAT_WDT_PING))
+ return 0;
+
+ mcu->wdt.info = &omnia_wdt_info;
+ mcu->wdt.ops = &omnia_wdt_ops;
+ mcu->wdt.parent = dev;
+ mcu->wdt.min_timeout = 1;
+ mcu->wdt.max_timeout = 6553; /* 65535 deciseconds */
+
+ mcu->wdt.timeout = WATCHDOG_TIMEOUT;
+ watchdog_init_timeout(&mcu->wdt, timeout, dev);
+
+ watchdog_set_drvdata(&mcu->wdt, mcu);
+
+ omnia_wdt_set_timeout(&mcu->wdt, mcu->wdt.timeout);
+
+ ret = omnia_cmd_read_u8(mcu->client, CMD_GET_WATCHDOG_STATE);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Cannot get MCU watchdog state\n");
+
+ if (ret)
+ set_bit(WDOG_HW_RUNNING, &mcu->wdt.status);
+
+ watchdog_set_nowayout(&mcu->wdt, nowayout);
+ watchdog_stop_on_reboot(&mcu->wdt);
+ ret = devm_watchdog_register_device(dev, &mcu->wdt);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Cannot register MCU watchdog\n");
+
+ return 0;
+}
diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h
index 6179a4f2069e..1838cb3d636e 100644
--- a/drivers/platform/cznic/turris-omnia-mcu.h
+++ b/drivers/platform/cznic/turris-omnia-mcu.h
@@ -14,6 +14,7 @@
#include <linux/mutex.h>
#include <linux/rtc.h>
#include <linux/types.h>
+#include <linux/watchdog.h>
#include <linux/workqueue.h>
#include <asm/byteorder.h>
#include <asm/unaligned.h>
@@ -41,6 +42,9 @@ struct omnia_mcu {
struct rtc_device *rtcdev;
u32 rtc_alarm;
bool front_button_poweron;
+
+ /* MCU watchdog */
+ struct watchdog_device wdt;
};
int omnia_cmd_write_read(const struct i2c_client *client,
@@ -53,6 +57,25 @@ static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
return omnia_cmd_write_read(client, cmd, len, NULL, 0);
}
+static inline int omnia_cmd_write_u8(const struct i2c_client *client, u8 cmd,
+ u8 val)
+{
+ u8 buf[2] = { cmd, val };
+
+ return omnia_cmd_write(client, buf, sizeof(buf));
+}
+
+static inline int omnia_cmd_write_u16(const struct i2c_client *client, u8 cmd,
+ u16 val)
+{
+ u8 buf[3];
+
+ buf[0] = cmd;
+ put_unaligned_le16(val, &buf[1]);
+
+ return omnia_cmd_write(client, buf, sizeof(buf));
+}
+
static inline int omnia_cmd_write_u32(const struct i2c_client *client, u8 cmd,
u32 val)
{
@@ -129,5 +152,6 @@ extern const struct attribute_group omnia_mcu_poweroff_group;
int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu);
int omnia_mcu_register_sys_off_and_wakeup(struct omnia_mcu *mcu);
+int omnia_mcu_register_watchdog(struct omnia_mcu *mcu);
#endif /* __TURRIS_OMNIA_MCU_H */
--
2.43.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
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:31 ` Ilpo Järvinen
1 sibling, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2024-04-30 12:53 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Andy Shevchenko,
Hans de Goede, Ilpo Järvinen, Linus Walleij,
Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
linux-watchdog
On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
>
> Add the basic skeleton for a new platform driver for the microcontroller
> found on the Turris Omnia board.
...
> +What: /sys/bus/i2c/devices/<mcu_device>/serial_number
> +Date: July 2024
> +KernelVersion: 6.10
> +Contact: Marek Behún <kabel@kernel.org>
> +Description: (RO) Contains the 64 bit long board serial number in hexadecimal
64 bit long --> 64-bit
> + format.
> +
> + Only available if board information is burned in the MCU (older
> + revisions have board information burned in the ATSHA204-A chip).
> +
> + Format: %016X.
It's strange to use capitalized hexadecimal here and not in other
files, but maybe it's something special about "serial number"? Dunno.
...
> +menuconfig CZNIC_PLATFORMS
> + bool "Platform support for CZ.NIC's Turris hardware"
> + depends on MACH_ARMADA_38X || COMPILE_TEST
This...
> + help
> + Say Y here to be able to choose driver support for CZ.NIC's Turris
> + devices. This option alone does not add any kernel code.
> +
> +if CZNIC_PLATFORMS
> +
> +config TURRIS_OMNIA_MCU
> + tristate "Turris Omnia MCU driver"
> + depends on MACH_ARMADA_38X || COMPILE_TEST
...or this dependency is redundant. I think one would expect that
these platforms will not be always based on the same platform, hence I
would drop the former and leave the latter. But you should know better
than me.
> + depends on I2C
> + help
> + Say Y here to add support for the features implemented by the
> + microcontroller on the CZ.NIC's Turris Omnia SOHO router.
> + To compile this driver as a module, choose M here; the module will be
> + called turris-omnia-mcu.
...
> +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> + u8 version[static OMNIA_FW_VERSION_HEX_LEN])
Interesting format of the last parameter. Does it make any difference
to the compiler if you use u8 *version?
> +{
> + u8 reply[OMNIA_FW_VERSION_LEN];
> + int err;
> +
> + err = omnia_cmd_read(mcu->client,
> + bootloader ? CMD_GET_FW_VERSION_BOOT
> + : CMD_GET_FW_VERSION_APP,
> + reply, sizeof(reply));
> + if (err)
> + return err;
> + version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
> + bin2hex(version, reply, OMNIA_FW_VERSION_LEN);
Hmm... I would rather use returned value
char *p;
p = bin2hex(...);
*p = '\0';
return 0;
> + return 0;
> +}
...
> +static umode_t omnia_mcu_base_attrs_visible(struct kobject *kobj,
> + struct attribute *a, int n)
> +{
> + struct device *dev = kobj_to_dev(kobj);
> + struct omnia_mcu *mcu = dev_get_drvdata(dev);
> + umode_t mode = a->mode;
Do you need this?
> + if ((a == &dev_attr_serial_number.attr ||
> + a == &dev_attr_first_mac_address.attr ||
> + a == &dev_attr_board_revision.attr) &&
> + !(mcu->features & FEAT_BOARD_INFO))
> + mode = 0;
return 0; ?
> + return mode;
return a->mode; ?
> +}
...
> +static void omnia_mcu_print_version_hash(struct omnia_mcu *mcu, bool bootloader)
> +{
> + const char *type = bootloader ? "bootloader" : "application";
> + struct device *dev = &mcu->client->dev;
> + u8 version[OMNIA_FW_VERSION_HEX_LEN];
> + int err;
> +
> + err = omnia_get_version_hash(mcu, bootloader, version);
> + if (err) {
> + dev_err(dev, "Cannot read MCU %s firmware version: %d\n", type,
> + err);
One line?
> + return;
> + }
> +
> + dev_info(dev, "MCU %s firmware version hash: %s\n", type, version);
> +}
...
> +static const char *omnia_status_to_mcu_type(uint16_t status)
Why out of a sudden uint16_t instead of u16?
> +{
> + switch (status & STS_MCU_TYPE_MASK) {
> + case STS_MCU_TYPE_STM32:
> + return "STM32";
> + case STS_MCU_TYPE_GD32:
> + return "GD32";
> + case STS_MCU_TYPE_MKL:
> + return "MKL";
> + default:
> + return "unknown";
> + }
> +}
...
> + static const struct {
> + uint16_t mask;
Ditto.
> + const char *name;
> + } features[] = {
> + { FEAT_EXT_CMDS, "extended control and status" },
> + { FEAT_WDT_PING, "watchdog pinging" },
> + { FEAT_LED_STATE_EXT_MASK, "peripheral LED pins reading" },
> + { FEAT_NEW_INT_API, "new interrupt API" },
> + { FEAT_POWEROFF_WAKEUP, "poweroff and wakeup" },
> + { FEAT_TRNG, "true random number generator" },
> + };
...
> + omnia_info_missing_feature(dev, "feature reading");
Tautology. Read the final message. I believe you wanted to pass just
"reading" here.
...
> + memcpy(mcu->board_first_mac, &reply[9], ETH_ALEN);
There is an API ether_copy_addr() or so, don't remember by heart.
You also need an include for ETH_ALEN definition.
...
> +#include <linux/i2c.h>
No users of this, you may replace with
struct i2c_client;
Am I right?
...
> + CMD_GET_FW_VERSION_BOOT = 0x0E, /* 20B git hash number */
Git
...
> + /* available only at address 0x2b (led-controller) */
LED-controller
...
> +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)
Keep trailing comma as it might be extended (theoretically). And you
do the similar in other enums anyway.
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
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
0 siblings, 2 replies; 14+ messages in thread
From: Marek Behún @ 2024-04-30 14:05 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Andy Shevchenko,
Hans de Goede, Ilpo Järvinen, Linus Walleij,
Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
linux-watchdog
On Tue, 30 Apr 2024 15:53:51 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
> >
> > Add the basic skeleton for a new platform driver for the microcontroller
> > found on the Turris Omnia board.
>
> ...
>
> > +What: /sys/bus/i2c/devices/<mcu_device>/serial_number
> > +Date: July 2024
> > +KernelVersion: 6.10
> > +Contact: Marek Behún <kabel@kernel.org>
> > +Description: (RO) Contains the 64 bit long board serial number in hexadecimal
>
> 64 bit long --> 64-bit
ok
>
> > + format.
> > +
> > + Only available if board information is burned in the MCU (older
> > + revisions have board information burned in the ATSHA204-A chip).
> > +
> > + Format: %016X.
>
> It's strange to use capitalized hexadecimal here and not in other
> files, but maybe it's something special about "serial number"? Dunno.
Yes, the serial number is printed with captial hex letters.
> > +menuconfig CZNIC_PLATFORMS
> > + bool "Platform support for CZ.NIC's Turris hardware"
>
> > + depends on MACH_ARMADA_38X || COMPILE_TEST
>
> This...
>
> > + help
> > + Say Y here to be able to choose driver support for CZ.NIC's Turris
> > + devices. This option alone does not add any kernel code.
> > +
> > +if CZNIC_PLATFORMS
> > +
> > +config TURRIS_OMNIA_MCU
> > + tristate "Turris Omnia MCU driver"
>
> > + depends on MACH_ARMADA_38X || COMPILE_TEST
>
> ...or this dependency is redundant. I think one would expect that
> these platforms will not be always based on the same platform, hence I
> would drop the former and leave the latter. But you should know better
> than me.
ok
>
> > + depends on I2C
> > + help
> > + Say Y here to add support for the features implemented by the
> > + microcontroller on the CZ.NIC's Turris Omnia SOHO router.
> > + To compile this driver as a module, choose M here; the module will be
> > + called turris-omnia-mcu.
>
> ...
>
> > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > + u8 version[static OMNIA_FW_VERSION_HEX_LEN])
>
> Interesting format of the last parameter. Does it make any difference
> to the compiler if you use u8 *version?
The compiler will warn if an array with not enough space is passed as
argument.
>
> > +{
> > + u8 reply[OMNIA_FW_VERSION_LEN];
> > + int err;
> > +
> > + err = omnia_cmd_read(mcu->client,
> > + bootloader ? CMD_GET_FW_VERSION_BOOT
> > + : CMD_GET_FW_VERSION_APP,
> > + reply, sizeof(reply));
> > + if (err)
> > + return err;
>
> > + version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
> > + bin2hex(version, reply, OMNIA_FW_VERSION_LEN);
>
> Hmm... I would rather use returned value
>
> char *p;
>
> p = bin2hex(...);
> *p = '\0';
>
> return 0;
OK. I guess
*bin2hex(version, reply, OMNIA_FW_VERSION_LEN) = '\0';
would be too crazy, right?
>
> > + return 0;
> > +}
>
> ...
>
> > +static umode_t omnia_mcu_base_attrs_visible(struct kobject *kobj,
> > + struct attribute *a, int n)
> > +{
> > + struct device *dev = kobj_to_dev(kobj);
> > + struct omnia_mcu *mcu = dev_get_drvdata(dev);
>
> > + umode_t mode = a->mode;
>
> Do you need this?
>
> > + if ((a == &dev_attr_serial_number.attr ||
> > + a == &dev_attr_first_mac_address.attr ||
> > + a == &dev_attr_board_revision.attr) &&
> > + !(mcu->features & FEAT_BOARD_INFO))
>
> > + mode = 0;
>
> return 0; ?
>
> > + return mode;
>
> return a->mode; ?
ok
>
> > +}
>
> ...
>
> > +static void omnia_mcu_print_version_hash(struct omnia_mcu *mcu, bool bootloader)
> > +{
> > + const char *type = bootloader ? "bootloader" : "application";
> > + struct device *dev = &mcu->client->dev;
> > + u8 version[OMNIA_FW_VERSION_HEX_LEN];
> > + int err;
> > +
> > + err = omnia_get_version_hash(mcu, bootloader, version);
> > + if (err) {
> > + dev_err(dev, "Cannot read MCU %s firmware version: %d\n", type,
> > + err);
>
> One line?
I'd like to keep this driver to 80 columns.
>
> > + return;
> > + }
> > +
> > + dev_info(dev, "MCU %s firmware version hash: %s\n", type, version);
> > +}
>
> ...
>
> > +static const char *omnia_status_to_mcu_type(uint16_t status)
>
> Why out of a sudden uint16_t instead of u16?
This was a mistake, thanks.
> > +{
> > + switch (status & STS_MCU_TYPE_MASK) {
> > + case STS_MCU_TYPE_STM32:
> > + return "STM32";
> > + case STS_MCU_TYPE_GD32:
> > + return "GD32";
> > + case STS_MCU_TYPE_MKL:
> > + return "MKL";
> > + default:
> > + return "unknown";
> > + }
> > +}
>
> ...
>
> > + static const struct {
> > + uint16_t mask;
>
> Ditto.
>
> > + const char *name;
> > + } features[] = {
> > + { FEAT_EXT_CMDS, "extended control and status" },
> > + { FEAT_WDT_PING, "watchdog pinging" },
> > + { FEAT_LED_STATE_EXT_MASK, "peripheral LED pins reading" },
> > + { FEAT_NEW_INT_API, "new interrupt API" },
> > + { FEAT_POWEROFF_WAKEUP, "poweroff and wakeup" },
> > + { FEAT_TRNG, "true random number generator" },
> > + };
>
> ...
>
> > + omnia_info_missing_feature(dev, "feature reading");
>
> Tautology. Read the final message. I believe you wanted to pass just
> "reading" here.
No, I actually wanted it to print
Your board's MCU firmware does not support the feature reading
feature.
as in the feature "feature reading" is not supported.
I guess I could change it to
Your board's MCU firmware does not support the feature reading.
but that would complicate the code: either I would need to add
" feature" suffix to all the features[].name, or duplicate the
info string from the omnia_info_missing_feature() function.
> ...
>
> > + memcpy(mcu->board_first_mac, &reply[9], ETH_ALEN);
>
> There is an API ether_copy_addr() or so, don't remember by heart.
> You also need an include for ETH_ALEN definition.
Doc for ether_addr_copy says:
Please note: dst & src must both be aligned to u16.
since the code does:
u16 *a = (u16 *)dst;
const u16 *b = (const u16 *)src;
a[0] = b[0];
a[1] = b[1];
a[2] = b[2]
Since I am copying from &reply[9], which is not u16-aligned, this won't
work.
> ...
>
> > +#include <linux/i2c.h>
>
> No users of this, you may replace with
>
> struct i2c_client;
>
> Am I right?
OK.
>
> ...
>
> > + CMD_GET_FW_VERSION_BOOT = 0x0E, /* 20B git hash
> > number */
>
> Git
OK.
> ...
>
> > + /* available only at address 0x2b (led-controller) */
>
> LED-controller
OK
>
> ...
>
> > +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)
>
> Keep trailing comma as it might be extended (theoretically). And you
> do the similar in other enums anyway.
ctl_byt is 8-bit, so this enum really can't be extended. In fact I need
to drop the last comma from omnia_ext_sts_dword_e and omnia_int_e.
Thanks, Andy.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
2024-04-30 14:05 ` Marek Behún
@ 2024-04-30 15:10 ` Dan Carpenter
2024-04-30 15:17 ` Andy Shevchenko
1 sibling, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2024-04-30 15:10 UTC (permalink / raw)
To: Marek Behún
Cc: Andy Shevchenko, Gregory CLEMENT, Arnd Bergmann, soc, arm,
Andy Shevchenko, Hans de Goede, Ilpo Järvinen,
Linus Walleij, Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
linux-watchdog
On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> > > +{
> > > + u8 reply[OMNIA_FW_VERSION_LEN];
> > > + int err;
> > > +
> > > + err = omnia_cmd_read(mcu->client,
> > > + bootloader ? CMD_GET_FW_VERSION_BOOT
> > > + : CMD_GET_FW_VERSION_APP,
> > > + reply, sizeof(reply));
> > > + if (err)
> > > + return err;
> >
> > > + version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
> > > + bin2hex(version, reply, OMNIA_FW_VERSION_LEN);
> >
> > Hmm... I would rather use returned value
> >
> > char *p;
> >
> > p = bin2hex(...);
> > *p = '\0';
> >
> > return 0;
>
> OK. I guess
>
> *bin2hex(version, reply, OMNIA_FW_VERSION_LEN) = '\0';
>
> would be too crazy, right?
Indeed. Please don't do that. 0_0
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
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
1 sibling, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2024-04-30 15:17 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Hans de Goede,
Ilpo Järvinen, Linus Walleij, Bartosz Golaszewski,
linux-gpio, Alessandro Zummo, Alexandre Belloni, linux-rtc,
Wim Van Sebroeck, Guenter Roeck, linux-watchdog
On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> On Tue, 30 Apr 2024 15:53:51 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
...
> > > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > > + u8 version[static OMNIA_FW_VERSION_HEX_LEN])
> >
> > Interesting format of the last parameter. Does it make any difference
> > to the compiler if you use u8 *version?
>
> The compiler will warn if an array with not enough space is passed as
> argument.
Really?
> > > +{
> > > + u8 reply[OMNIA_FW_VERSION_LEN];
> > > + int err;
> > > +
> > > + err = omnia_cmd_read(mcu->client,
> > > + bootloader ? CMD_GET_FW_VERSION_BOOT
> > > + : CMD_GET_FW_VERSION_APP,
> > > + reply, sizeof(reply));
> > > + if (err)
> > > + return err;
> >
> > > + version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
> > > + bin2hex(version, reply, OMNIA_FW_VERSION_LEN);
> >
> > Hmm... I would rather use returned value
> >
> > char *p;
> >
> > p = bin2hex(...);
> > *p = '\0';
> >
> > return 0;
>
> OK. I guess
>
> *bin2hex(version, reply, OMNIA_FW_VERSION_LEN) = '\0';
>
> would be too crazy, right?
Yes, it's not a Python :-)
> > > + return 0;
> > > +}
...
> > > + dev_err(dev, "Cannot read MCU %s firmware version: %d\n", type,
> > > + err);
> >
> > One line?
>
> I'd like to keep this driver to 80 columns.
Then better to have a logical split then?
dev_err(dev, "Cannot read MCU %s firmware version: %d\n",
type, err);
...
> > > + omnia_info_missing_feature(dev, "feature reading");
> >
> > Tautology. Read the final message. I believe you wanted to pass just
> > "reading" here.
>
> No, I actually wanted it to print
> Your board's MCU firmware does not support the feature reading
> feature.
> as in the feature "feature reading" is not supported.
> I guess I could change it to
> Your board's MCU firmware does not support the feature reading.
> but that would complicate the code: either I would need to add
> " feature" suffix to all the features[].name, or duplicate the
> info string from the omnia_info_missing_feature() function.
From point of a mere user (as I am towards this driver) I consider
the tautology confusing.
...the 'reading' feature
may be a good compromise.
...
> > > + memcpy(mcu->board_first_mac, &reply[9], ETH_ALEN);
> >
> > There is an API ether_copy_addr() or so, don't remember by heart.
> > You also need an include for ETH_ALEN definition.
>
> Doc for ether_addr_copy says:
> Please note: dst & src must both be aligned to u16.
> since the code does:
> u16 *a = (u16 *)dst;
> const u16 *b = (const u16 *)src;
>
> a[0] = b[0];
> a[1] = b[1];
> a[2] = b[2]
>
> Since I am copying from &reply[9], which is not u16-aligned, this won't
> work.
It would work on architectures that support misaligned accesses, but in general
you are right. Perhaps a comment on top to avoid "cleanup" patches like this?
...
> > > +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)
> >
> > Keep trailing comma as it might be extended (theoretically). And you
> > do the similar in other enums anyway.
>
> ctl_byt is 8-bit, so this enum really can't be extended.
I understand that (even before writing the previous reply).
> In fact I need
> to drop the last comma from omnia_ext_sts_dword_e and omnia_int_e.
Who prevents from having in a new firmware let's say
BIT(31) | BIT(1)
as a new value?
From Linux perspective these are not terminating lines.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
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 15:31 ` Ilpo Järvinen
2024-05-02 19:19 ` Marek Behún
1 sibling, 1 reply; 14+ messages in thread
From: Ilpo Järvinen @ 2024-04-30 15:31 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Andy Shevchenko,
Hans de Goede, Linus Walleij, Bartosz Golaszewski, linux-gpio,
Alessandro Zummo, Alexandre Belloni, linux-rtc, Wim Van Sebroeck,
Guenter Roeck, linux-watchdog
[-- 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
2024-04-30 15:17 ` Andy Shevchenko
@ 2024-05-02 18:40 ` Marek Behún
2024-05-02 18:47 ` Andy Shevchenko
0 siblings, 1 reply; 14+ messages in thread
From: Marek Behún @ 2024-05-02 18:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Hans de Goede,
Ilpo Järvinen, Linus Walleij, Bartosz Golaszewski,
linux-gpio, Alessandro Zummo, Alexandre Belloni, linux-rtc,
Wim Van Sebroeck, Guenter Roeck, linux-watchdog
On Tue, Apr 30, 2024 at 06:17:45PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> > On Tue, 30 Apr 2024 15:53:51 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
>
> ...
>
> > > > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > > > + u8 version[static OMNIA_FW_VERSION_HEX_LEN])
> > >
> > > Interesting format of the last parameter. Does it make any difference
> > > to the compiler if you use u8 *version?
> >
> > The compiler will warn if an array with not enough space is passed as
> > argument.
>
> Really?
Indeed:
extern void a(char *x);
static void b(char x[static 10]) {
a(x);
}
void c(void) {
char x[5] = "abcd";
b(x);
}
says:
test.c: In function ‘c’:
test.c:9:9: warning: ‘b’ accessing 10 bytes in a region of size 5 [-Wstringop-overflow=]
9 | b(x);
| ^~~~
test.c:9:9: note: referencing argument 1 of type ‘char[10]’
test.c:3:13: note: in a call to function ‘b’
3 | static void b(char x[static 10]) {
|
...
> > > > + dev_err(dev, "Cannot read MCU %s firmware version: %d\n", type,
> > > > + err);
> > >
> > > One line?
> >
> > I'd like to keep this driver to 80 columns.
>
> Then better to have a logical split then?
>
> dev_err(dev, "Cannot read MCU %s firmware version: %d\n",
> type, err);
OK
> > > > + omnia_info_missing_feature(dev, "feature reading");
> > >
> > > Tautology. Read the final message. I believe you wanted to pass just
> > > "reading" here.
> >
> > No, I actually wanted it to print
> > Your board's MCU firmware does not support the feature reading
> > feature.
> > as in the feature "feature reading" is not supported.
> > I guess I could change it to
> > Your board's MCU firmware does not support the feature reading.
> > but that would complicate the code: either I would need to add
> > " feature" suffix to all the features[].name, or duplicate the
> > info string from the omnia_info_missing_feature() function.
>
> From point of a mere user (as I am towards this driver) I consider
> the tautology confusing.
>
> ...the 'reading' feature
>
> may be a good compromise.
I have rewritten it to use another dev_info:
dev_info(dev,
"Your board's MCU firmware does not support feature reading.\n");
>
> ...
>
> > > > + memcpy(mcu->board_first_mac, &reply[9], ETH_ALEN);
> > >
> > > There is an API ether_copy_addr() or so, don't remember by heart.
> > > You also need an include for ETH_ALEN definition.
> >
> > Doc for ether_addr_copy says:
> > Please note: dst & src must both be aligned to u16.
> > since the code does:
> > u16 *a = (u16 *)dst;
> > const u16 *b = (const u16 *)src;
> >
> > a[0] = b[0];
> > a[1] = b[1];
> > a[2] = b[2]
> >
> > Since I am copying from &reply[9], which is not u16-aligned, this won't
> > work.
>
> It would work on architectures that support misaligned accesses, but in general
> you are right. Perhaps a comment on top to avoid "cleanup" patches like this?
OK
> > > > +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)
> > >
> > > Keep trailing comma as it might be extended (theoretically). And you
> > > do the similar in other enums anyway.
> >
> > ctl_byt is 8-bit, so this enum really can't be extended.
>
> I understand that (even before writing the previous reply).
>
> > In fact I need
> > to drop the last comma from omnia_ext_sts_dword_e and omnia_int_e.
>
> Who prevents from having in a new firmware let's say
>
> BIT(31) | BIT(1)
>
> as a new value?
>
> From Linux perspective these are not terminating lines.
OK
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
2024-05-02 18:40 ` Marek Behún
@ 2024-05-02 18:47 ` Andy Shevchenko
2024-05-02 19:17 ` Marek Behún
0 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2024-05-02 18:47 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Hans de Goede,
Ilpo Järvinen, Linus Walleij, Bartosz Golaszewski,
linux-gpio, Alessandro Zummo, Alexandre Belloni, linux-rtc,
Wim Van Sebroeck, Guenter Roeck, linux-watchdog
On Thu, May 2, 2024 at 9:40 PM Marek Behún <kabel@kernel.org> wrote:
> On Tue, Apr 30, 2024 at 06:17:45PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> > > On Tue, 30 Apr 2024 15:53:51 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
...
> > > > > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > > > > + u8 version[static OMNIA_FW_VERSION_HEX_LEN])
> > > >
> > > > Interesting format of the last parameter. Does it make any difference
> > > > to the compiler if you use u8 *version?
> > >
> > > The compiler will warn if an array with not enough space is passed as
> > > argument.
> >
> > Really?
>
> Indeed:
>
> extern void a(char *x);
>
> static void b(char x[static 10]) {
> a(x);
> }
>
> void c(void) {
> char x[5] = "abcd";
> b(x);
It's not the example I was talking about. Here should be a(x).
> }
>
> says:
> test.c: In function ‘c’:
> test.c:9:9: warning: ‘b’ accessing 10 bytes in a region of size 5 [-Wstringop-overflow=]
> 9 | b(x);
> | ^~~~
> test.c:9:9: note: referencing argument 1 of type ‘char[10]’
> test.c:3:13: note: in a call to function ‘b’
> 3 | static void b(char x[static 10]) {
> |
...
> > > > > + omnia_info_missing_feature(dev, "feature reading");
> > > >
> > > > Tautology. Read the final message. I believe you wanted to pass just
> > > > "reading" here.
> > >
> > > No, I actually wanted it to print
> > > Your board's MCU firmware does not support the feature reading
> > > feature.
> > > as in the feature "feature reading" is not supported.
> > > I guess I could change it to
> > > Your board's MCU firmware does not support the feature reading.
> > > but that would complicate the code: either I would need to add
> > > " feature" suffix to all the features[].name, or duplicate the
> > > info string from the omnia_info_missing_feature() function.
> >
> > From point of a mere user (as I am towards this driver) I consider
> > the tautology confusing.
> >
> > ...the 'reading' feature
> >
> > may be a good compromise.
>
> I have rewritten it to use another dev_info:
> dev_info(dev,
> "Your board's MCU firmware does not support feature reading.\n");
OK! As long as there are no two messages in a row for the same error.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
2024-05-02 18:47 ` Andy Shevchenko
@ 2024-05-02 19:17 ` Marek Behún
2024-05-03 3:59 ` Andy Shevchenko
0 siblings, 1 reply; 14+ messages in thread
From: Marek Behún @ 2024-05-02 19:17 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Hans de Goede,
Ilpo Järvinen, Linus Walleij, Bartosz Golaszewski,
linux-gpio, Alessandro Zummo, Alexandre Belloni, linux-rtc,
Wim Van Sebroeck, Guenter Roeck, linux-watchdog
On Thu, May 02, 2024 at 09:47:25PM +0300, Andy Shevchenko wrote:
> On Thu, May 2, 2024 at 9:40 PM Marek Behún <kabel@kernel.org> wrote:
> > On Tue, Apr 30, 2024 at 06:17:45PM +0300, Andy Shevchenko wrote:
> > > On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> > > > On Tue, 30 Apr 2024 15:53:51 +0300
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
>
> ...
>
> > > > > > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > > > > > + u8 version[static OMNIA_FW_VERSION_HEX_LEN])
> > > > >
> > > > > Interesting format of the last parameter. Does it make any difference
> > > > > to the compiler if you use u8 *version?
> > > >
> > > > The compiler will warn if an array with not enough space is passed as
> > > > argument.
> > >
> > > Really?
> >
> > Indeed:
> >
> > extern void a(char *x);
> >
> > static void b(char x[static 10]) {
> > a(x);
> > }
> >
> > void c(void) {
> > char x[5] = "abcd";
>
> > b(x);
>
> It's not the example I was talking about. Here should be a(x).
Somehow I got lost. Let's return to my function, where I have
int omnia_get_version_hash(..., u8 version[static 40]);
and then I use this function:
u8 version[40];
omnia_get_version_hash(..., version);
If somehow I made a mistake and declared the version array shorter:
u8 version[20];
omnia_get_version_hash(..., version);
I would get a warning.
So the purpose is to get warned if the compiler can prove that there is
not enough space in the destination buffer.
Marek
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
2024-04-30 15:31 ` Ilpo Järvinen
@ 2024-05-02 19:19 ` Marek Behún
0 siblings, 0 replies; 14+ messages in thread
From: Marek Behún @ 2024-05-02 19:19 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Andy Shevchenko,
Hans de Goede, Linus Walleij, Bartosz Golaszewski, linux-gpio,
Alessandro Zummo, Alexandre Belloni, linux-rtc, Wim Van Sebroeck,
Guenter Roeck, linux-watchdog
Thanks, Ilpo, I've applied all your suggestions.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
2024-05-02 19:17 ` Marek Behún
@ 2024-05-03 3:59 ` Andy Shevchenko
2024-05-03 6:51 ` Marek Behún
0 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2024-05-03 3:59 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Hans de Goede,
Ilpo Järvinen, Linus Walleij, Bartosz Golaszewski,
linux-gpio, Alessandro Zummo, Alexandre Belloni, linux-rtc,
Wim Van Sebroeck, Guenter Roeck, linux-watchdog
On Thu, May 2, 2024 at 10:18 PM Marek Behún <kabel@kernel.org> wrote:
> On Thu, May 02, 2024 at 09:47:25PM +0300, Andy Shevchenko wrote:
> > On Thu, May 2, 2024 at 9:40 PM Marek Behún <kabel@kernel.org> wrote:
> > > On Tue, Apr 30, 2024 at 06:17:45PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> > > > > On Tue, 30 Apr 2024 15:53:51 +0300
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
...
> > > > > > > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > > > > > > + u8 version[static OMNIA_FW_VERSION_HEX_LEN])
> > > > > >
> > > > > > Interesting format of the last parameter. Does it make any difference
> > > > > > to the compiler if you use u8 *version?
> > > > >
> > > > > The compiler will warn if an array with not enough space is passed as
> > > > > argument.
> > > >
> > > > Really?
> > >
> > > Indeed:
> > >
> > > extern void a(char *x);
> > >
> > > static void b(char x[static 10]) {
> > > a(x);
> > > }
> > >
> > > void c(void) {
> > > char x[5] = "abcd";
> >
> > > b(x);
> >
> > It's not the example I was talking about. Here should be a(x).
>
> Somehow I got lost. Let's return to my function, where I have
> int omnia_get_version_hash(..., u8 version[static 40]);
>
> and then I use this function:
>
> u8 version[40];
> omnia_get_version_hash(..., version);
>
> If somehow I made a mistake and declared the version array shorter:
> u8 version[20];
> omnia_get_version_hash(..., version);
> I would get a warning.
Yes. Would you get the same warning if you replace the parameter to a pointer?
> So the purpose is to get warned if the compiler can prove that there is
> not enough space in the destination buffer.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
2024-05-03 3:59 ` Andy Shevchenko
@ 2024-05-03 6:51 ` Marek Behún
0 siblings, 0 replies; 14+ messages in thread
From: Marek Behún @ 2024-05-03 6:51 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Hans de Goede,
Ilpo Järvinen, Linus Walleij, Bartosz Golaszewski,
linux-gpio, Alessandro Zummo, Alexandre Belloni, linux-rtc,
Wim Van Sebroeck, Guenter Roeck, linux-watchdog
On Fri, May 03, 2024 at 06:59:20AM +0300, Andy Shevchenko wrote:
> On Thu, May 2, 2024 at 10:18 PM Marek Behún <kabel@kernel.org> wrote:
> > On Thu, May 02, 2024 at 09:47:25PM +0300, Andy Shevchenko wrote:
> > > On Thu, May 2, 2024 at 9:40 PM Marek Behún <kabel@kernel.org> wrote:
> > > > On Tue, Apr 30, 2024 at 06:17:45PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> > > > > > On Tue, 30 Apr 2024 15:53:51 +0300
> > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
>
> ...
>
> > > > > > > > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > > > > > > > + u8 version[static OMNIA_FW_VERSION_HEX_LEN])
> > > > > > >
> > > > > > > Interesting format of the last parameter. Does it make any difference
> > > > > > > to the compiler if you use u8 *version?
> > > > > >
> > > > > > The compiler will warn if an array with not enough space is passed as
> > > > > > argument.
> > > > >
> > > > > Really?
> > > >
> > > > Indeed:
> > > >
> > > > extern void a(char *x);
> > > >
> > > > static void b(char x[static 10]) {
> > > > a(x);
> > > > }
> > > >
> > > > void c(void) {
> > > > char x[5] = "abcd";
> > >
> > > > b(x);
> > >
> > > It's not the example I was talking about. Here should be a(x).
> >
> > Somehow I got lost. Let's return to my function, where I have
> > int omnia_get_version_hash(..., u8 version[static 40]);
> >
> > and then I use this function:
> >
> > u8 version[40];
> > omnia_get_version_hash(..., version);
> >
> > If somehow I made a mistake and declared the version array shorter:
> > u8 version[20];
> > omnia_get_version_hash(..., version);
> > I would get a warning.
>
> Yes. Would you get the same warning if you replace the parameter to a pointer?
If you mean instead of declaring
u8 version[20];
if I did
u8 *version;
omnia_get_version_hash(..., version);
then I will get a different warning:
version is used uninitialized
And if you mean that instead of the
char version[static 40]
in the argument list of the function I used
char *version,
then no, GCC spits no warning.
Marek
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-05-03 6:51 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2024-05-02 19:19 ` Marek Behún
2024-04-30 11:51 ` [PATCH v8 5/9] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog Marek Behún
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 NNTP newsgroup(s).