* [PATCH v2 0/7] Turris Omnia MCU driver
@ 2023-09-19 10:38 Marek Behún
2023-09-19 10:38 ` [PATCH v2 1/7] dt-bindings: arm: add cznic,turris-omnia-mcu binding Marek Behún
` (6 more replies)
0 siblings, 7 replies; 30+ messages in thread
From: Marek Behún @ 2023-09-19 10:38 UTC (permalink / raw)
To: Gregory CLEMENT, Arnd Bergmann, soc, arm, Alessandro Zummo,
Alexandre Belloni, Andy Shevchenko, Bartosz Golaszewski,
devicetree, Guenter Roeck, Krzysztof Kozlowski, Linus Walleij,
linux-gpio, linux-rtc, linux-watchdog, Rob Herring,
Wim Van Sebroeck
Cc: Marek Behún
Hello Arnd, Gregorgy and others,
this is v2 of series adding support for Turris Omnia MCU. See the cover
letter from v1:
https://patchwork.kernel.org/project/linux-soc/cover/20230823161012.6986-1-kabel@kernel.org/
I am also sending this to specific subsystem maintainers as requested by
Krzysztof.
Changes since v1:
- incorporated small changes requested by Krzysztof Kozlowski
- in order to make reviewing simpler, I split the second patch, which
implements the MCU driver, into four incremental patches:
- patch 2/7 adds basic driver skeleton
- patch 3/7 adds GPIO support
- patch 4/7 adds board poweroff and wakeup support
- patch 5/7 adds MCU watchdog support
Marek
Marek Behún (7):
dt-bindings: arm: 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
ARM: dts: turris-omnia: Add MCU system-controller node
ARM: dts: turris-omnia: Add GPIO key node for front button
.../sysfs-bus-i2c-devices-turris-omnia-mcu | 77 ++
.../bindings/arm/cznic,turris-omnia-mcu.yaml | 67 ++
MAINTAINERS | 4 +
.../dts/marvell/armada-385-turris-omnia.dts | 35 +-
drivers/platform/Kconfig | 2 +
drivers/platform/Makefile | 1 +
drivers/platform/cznic/Kconfig | 47 +
drivers/platform/cznic/Makefile | 7 +
.../platform/cznic/turris-omnia-mcu-base.c | 273 +++++
.../platform/cznic/turris-omnia-mcu-gpio.c | 971 ++++++++++++++++++
.../cznic/turris-omnia-mcu-sys-off-wakeup.c | 245 +++++
.../cznic/turris-omnia-mcu-watchdog.c | 126 +++
drivers/platform/cznic/turris-omnia-mcu.h | 164 +++
include/linux/turris-omnia-mcu-interface.h | 194 ++++
14 files changed, 2212 insertions(+), 1 deletion(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
create mode 100644 Documentation/devicetree/bindings/arm/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-gpio.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.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.41.0
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/7] dt-bindings: arm: add cznic,turris-omnia-mcu binding
2023-09-19 10:38 [PATCH v2 0/7] Turris Omnia MCU driver Marek Behún
@ 2023-09-19 10:38 ` Marek Behún
2023-09-20 12:37 ` Krzysztof Kozlowski
2023-09-19 10:38 ` [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU Marek Behún
` (5 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Marek Behún @ 2023-09-19 10:38 UTC (permalink / raw)
To: Gregory CLEMENT, Arnd Bergmann, soc, arm, Rob Herring,
Krzysztof Kozlowski, devicetree
Cc: Marek Behún
Add binding for cznic,turris-omnia-mcu, the device-tree node
representing the system-controller features provided by the MCU on the
Turris Omnia router.
Signed-off-by: Marek Behún <kabel@kernel.org>
---
.../bindings/arm/cznic,turris-omnia-mcu.yaml | 67 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 68 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml
diff --git a/Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml b/Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml
new file mode 100644
index 000000000000..ea1fd0117bb8
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/cznic,turris-omnia-mcu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CZ.NIC's Turris Omnia MCU
+
+maintainers:
+ - Marek Behún <kabel@kernel.org>
+
+description:
+ The MCU on Turris Omnia acts as a system controller providing additional
+ GPIOs, interrupts, watchdog, system power off and wakeup configuration.
+
+properties:
+ compatible:
+ const: cznic,turris-omnia-mcu
+
+ reg:
+ description: MCU I2C slave address
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ interrupt-controller: true
+
+ '#interrupt-cells':
+ const: 2
+
+ gpio-controller: true
+
+ '#gpio-cells':
+ const: 2
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - interrupt-controller
+ - gpio-controller
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ system-controller@2a {
+ compatible = "cznic,turris-omnia-mcu";
+ reg = <0x2a>;
+
+ interrupt-parent = <&gpio1>;
+ interrupts = <11 IRQ_TYPE_NONE>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index bf0f54c24f81..0a4d7d6ed63a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2091,6 +2091,7 @@ W: https://www.turris.cz/
F: Documentation/ABI/testing/debugfs-moxtet
F: Documentation/ABI/testing/sysfs-bus-moxtet-devices
F: Documentation/ABI/testing/sysfs-firmware-turris-mox-rwtm
+F: Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml
F: Documentation/devicetree/bindings/bus/moxtet.txt
F: Documentation/devicetree/bindings/firmware/cznic,turris-mox-rwtm.txt
F: Documentation/devicetree/bindings/gpio/gpio-moxtet.txt
--
2.41.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU
2023-09-19 10:38 [PATCH v2 0/7] Turris Omnia MCU driver Marek Behún
2023-09-19 10:38 ` [PATCH v2 1/7] dt-bindings: arm: add cznic,turris-omnia-mcu binding Marek Behún
@ 2023-09-19 10:38 ` Marek Behún
2023-09-19 12:29 ` Andy Shevchenko
2023-09-19 10:38 ` [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs Marek Behún
` (4 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Marek Behún @ 2023-09-19 10:38 UTC (permalink / raw)
To: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, Andy Shevchenko, linux-gpio,
Alessandro Zummo, Alexandre Belloni, linux-rtc, Wim Van Sebroeck,
Guenter Roeck, linux-watchdog
Cc: Marek Behún
Add a new platform driver that exposes the features provided by the
microcontroller on the Turris Omnia board.
This commit adds the basic skeleton for the driver.
Signed-off-by: Marek Behún <kabel@kernel.org>
---
.../sysfs-bus-i2c-devices-turris-omnia-mcu | 45 ++++
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 | 253 ++++++++++++++++++
drivers/platform/cznic/turris-omnia-mcu.h | 65 +++++
include/linux/turris-omnia-mcu-interface.h | 194 ++++++++++++++
9 files changed, 593 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..69a4db7e20c0
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
@@ -0,0 +1,45 @@
+What: /sys/bus/i2c/devices/<mcu_device>/fw_features
+Date: August 2023
+KernelVersion: 6.6
+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: August 2023
+KernelVersion: 6.6
+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: August 2023
+KernelVersion: 6.6
+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: August 2023
+KernelVersion: 6.6
+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: August 2023
+KernelVersion: 6.6
+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.
diff --git a/MAINTAINERS b/MAINTAINERS
index 0a4d7d6ed63a..1396743d5f9f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2089,6 +2089,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/arm/cznic,turris-omnia-mcu.yaml
@@ -2102,10 +2103,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..cc66b5020a34 100644
--- a/drivers/platform/Makefile
+++ b/drivers/platform/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OLPC_EC) += olpc/
obj-$(CONFIG_GOLDFISH) += goldfish/
obj-$(CONFIG_CHROME_PLATFORMS) += chrome/
obj-$(CONFIG_SURFACE_PLATFORMS) += surface/
+obj-$(CONFIG_CZNIC_PLATFORMS) += cznic/
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..4d0a9586538c
--- /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-objs := 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..2d3c3f9c68fc
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CZ.NIC's Turris Omnia MCU driver
+ *
+ * 2023 by Marek Behún <kabel@kernel.org>
+ */
+
+#include <linux/hex.h>
+#include <linux/module.h>
+
+#include "turris-omnia-mcu.h"
+
+static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
+ u8 *version)
+{
+ u8 reply[20];
+ int ret;
+
+ ret = omnia_cmd_read(mcu->client, bootloader ? CMD_GET_FW_VERSION_BOOT :
+ CMD_GET_FW_VERSION_APP,
+ reply, sizeof(reply));
+ if (ret < 0)
+ return ret;
+
+ version[40] = '\0';
+ bin2hex(version, reply, 20);
+
+ return 0;
+}
+
+static ssize_t fw_version_hash_show(struct device *dev, char *buf,
+ bool bootloader)
+{
+ struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
+ u8 version[41];
+ 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 = i2c_get_clientdata(to_i2c_client(dev));
+
+ return sysfs_emit(buf, "%#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 = i2c_get_clientdata(to_i2c_client(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 = 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 struct attribute *omnia_mcu_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,
+ NULL,
+};
+
+ATTRIBUTE_GROUPS(omnia_mcu);
+
+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[41];
+ 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" },
+ };
+ struct device *dev = &mcu->client->dev;
+ bool suggest_fw_upgrade = false;
+ int status;
+
+ 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) {
+ int val = omnia_cmd_read_u16(mcu->client, CMD_GET_FEATURES);
+
+ if (val < 0)
+ return val;
+
+ mcu->features = val;
+ } 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 (int i = 0; i < ARRAY_SIZE(features); ++i) {
+ if (!(mcu->features & features[i].mask)) {
+ 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_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct omnia_mcu *mcu;
+ int ret;
+
+ mcu = devm_kzalloc(dev, sizeof(*mcu), GFP_KERNEL);
+ if (!mcu)
+ return -ENOMEM;
+
+ mcu->client = client;
+ i2c_set_clientdata(client, mcu);
+
+ ret = omnia_mcu_read_features(mcu);
+ if (ret) {
+ dev_err(dev, "Cannot determine MCU supported features: %d\n",
+ ret);
+ return ret;
+ }
+
+ if (!client->irq) {
+ dev_err(dev, "IRQ resource not found\n");
+ return -ENODATA;
+ }
+
+ return 0;
+}
+
+static const struct of_device_id of_omnia_mcu_match[] = {
+ { .compatible = "cznic,turris-omnia-mcu", },
+ {},
+};
+
+static const struct i2c_device_id omnia_mcu_id[] = {
+ { "turris-omnia-mcu", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, omnia_mcu_id);
+
+static struct i2c_driver omnia_mcu_driver = {
+ .probe = omnia_mcu_probe,
+ .id_table = omnia_mcu_id,
+ .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..a045b114e293
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * CZ.NIC's Turris Omnia MCU driver
+ *
+ * 2023 by Marek Behún <kabel@kernel.org>
+ */
+
+#ifndef __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
+#define __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
+
+#include <asm/byteorder.h>
+#include <linux/i2c.h>
+#include <linux/turris-omnia-mcu-interface.h>
+
+struct omnia_mcu {
+ struct i2c_client *client;
+ const char *type;
+ u16 features;
+};
+
+static inline 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 (likely(ret == ARRAY_SIZE(msgs)))
+ return len;
+ else if (ret < 0)
+ return ret;
+ else
+ return -EIO;
+}
+
+static inline int omnia_cmd_read_u16(const struct i2c_client *client, u8 cmd)
+{
+ u16 reply;
+ int ret;
+
+ ret = omnia_cmd_read(client, cmd, &reply, sizeof(reply));
+
+ return ret < 0 ? ret : le16_to_cpu(reply);
+}
+
+static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
+{
+ u8 reply;
+ int ret;
+
+ ret = omnia_cmd_read(client, cmd, &reply, sizeof(reply));
+
+ return ret < 0 ? ret : reply;
+}
+
+#endif /* __DRIVERS_PLATFORM_CZNIC_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..888dc74c5ec0
--- /dev/null
+++ b/include/linux/turris-omnia-mcu-interface.h
@@ -0,0 +1,194 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * CZ.NIC's Turris Omnia MCU I2C interface commands definitions
+ *
+ * 2023 by Marek Behún <kabel@kernel.org>
+ */
+
+#ifndef __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H
+#define __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H
+
+#include <linux/bits.h>
+
+enum 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 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,
+};
+
+enum flashing_commands_e {
+ FLASH_CMD_UNLOCK = 0x01,
+ FLASH_CMD_SIZE_AND_CSUM = 0x02,
+ FLASH_CMD_PROGRAM = 0x03,
+ FLASH_CMD_RESET = 0x04,
+};
+
+enum 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 ctl_byte_e {
+ CTL_LIGHT_RST = BIT(0),
+ CTL_HARD_RST = BIT(1),
+ /*CTL_RESERVED = BIT(2),*/
+ 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 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),
+};
+
+enum 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 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 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_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 cmd_poweroff_e {
+ CMD_POWER_OFF_POWERON_BUTTON = BIT(0),
+ CMD_POWER_OFF_MAGIC = 0xdead,
+};
+
+#endif /* __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H */
--
2.41.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-19 10:38 [PATCH v2 0/7] Turris Omnia MCU driver Marek Behún
2023-09-19 10:38 ` [PATCH v2 1/7] dt-bindings: arm: add cznic,turris-omnia-mcu binding Marek Behún
2023-09-19 10:38 ` [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU Marek Behún
@ 2023-09-19 10:38 ` Marek Behún
2023-09-19 13:00 ` Andy Shevchenko
2023-09-20 11:58 ` Linus Walleij
2023-09-19 10:38 ` [PATCH v2 4/7] platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup Marek Behún
` (3 subsequent siblings)
6 siblings, 2 replies; 30+ messages in thread
From: Marek Behún @ 2023-09-19 10:38 UTC (permalink / raw)
To: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, Andy Shevchenko, linux-gpio
Cc: Marek Behún
Add support for GPIOs connected to the MCU on the Turris Omnia board.
This include:
- front button pin
- enable pins for USB regulators
- MiniPCIe / mSATA card presence pins in MiniPCIe port 0
- LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports
- on board revisions 32+ also various peripheral resets and another
voltage regulator enable pin
Signed-off-by: Marek Behún <kabel@kernel.org>
---
.../sysfs-bus-i2c-devices-turris-omnia-mcu | 16 +
drivers/platform/cznic/Kconfig | 15 +
drivers/platform/cznic/Makefile | 1 +
.../platform/cznic/turris-omnia-mcu-base.c | 12 +
.../platform/cznic/turris-omnia-mcu-gpio.c | 973 ++++++++++++++++++
drivers/platform/cznic/turris-omnia-mcu.h | 56 +
6 files changed, 1073 insertions(+)
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-gpio.c
diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
index 69a4db7e20c0..c0efdf1b3803 100644
--- a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
@@ -1,3 +1,19 @@
+What: /sys/bus/i2c/devices/<mcu_device>/front_button_mode
+Date: August 2023
+KernelVersion: 6.6
+Contact: Marek Behún <kabel@kernel.org>
+Description: (RW) The front button on the Turris Omnia router can be
+ configured either to change the intensity of all the LEDs on the
+ front panel, or to send the press event to the CPU as an
+ interrupt.
+
+ This file switches between these two modes:
+ - "mcu" makes the button press event be handled by the MCU to
+ change the LEDs panel intensity.
+ - "cpu" makes the button press event be handled by the CPU.
+
+ Format: %s.
+
What: /sys/bus/i2c/devices/<mcu_device>/fw_features
Date: August 2023
KernelVersion: 6.6
diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig
index f8560ff9c1af..3a8c3edcd7e6 100644
--- a/drivers/platform/cznic/Kconfig
+++ b/drivers/platform/cznic/Kconfig
@@ -17,9 +17,24 @@ config TURRIS_OMNIA_MCU
tristate "Turris Omnia MCU driver"
depends on MACH_ARMADA_38X || COMPILE_TEST
depends on I2C
+ select GPIOLIB
+ select GPIOLIB_IRQCHIP
help
Say Y here to add support for the features implemented by the
microcontroller on the CZ.NIC's Turris Omnia SOHO router.
+ The features include:
+ - 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
+ front LEDs panel brightness)
+ - to enable / disable USB port voltage regulators and to detect
+ USB overcurrent
+ - to detect MiniPCIe / mSATA card presence in MiniPCIe port 0
+ - to configure resets of various peripherals on board revisions 32+
+ - to enable / disable the VHV voltage regulator to the SOC in order
+ to be able to program SOC's OTP on board revisions 32+
+ - to get input from the LED output pins of the WAN ethernet PHY, LAN
+ switch and MiniPCIe ports
To compile this driver as a module, choose M here; the module will be
called turris-omnia-mcu.
diff --git a/drivers/platform/cznic/Makefile b/drivers/platform/cznic/Makefile
index 4d0a9586538c..a6177f5b4fff 100644
--- a/drivers/platform/cznic/Makefile
+++ b/drivers/platform/cznic/Makefile
@@ -2,3 +2,4 @@
obj-$(CONFIG_TURRIS_OMNIA_MCU) += turris-omnia-mcu.o
turris-omnia-mcu-objs := turris-omnia-mcu-base.o
+turris-omnia-mcu-objs += turris-omnia-mcu-gpio.o
diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c
index 2d3c3f9c68fc..2954151468e5 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-base.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
@@ -222,9 +222,20 @@ static int omnia_mcu_probe(struct i2c_client *client)
return -ENODATA;
}
+ ret = omnia_mcu_register_gpiochip(mcu);
+ if (ret)
+ return ret;
+
return 0;
}
+static void omnia_mcu_remove(struct i2c_client *client)
+{
+ struct omnia_mcu *mcu = i2c_get_clientdata(client);
+
+ omnia_mcu_unregister_gpiochip(mcu);
+}
+
static const struct of_device_id of_omnia_mcu_match[] = {
{ .compatible = "cznic,turris-omnia-mcu", },
{},
@@ -238,6 +249,7 @@ MODULE_DEVICE_TABLE(i2c, omnia_mcu_id);
static struct i2c_driver omnia_mcu_driver = {
.probe = omnia_mcu_probe,
+ .remove = omnia_mcu_remove,
.id_table = omnia_mcu_id,
.driver = {
.name = "turris-omnia-mcu",
diff --git a/drivers/platform/cznic/turris-omnia-mcu-gpio.c b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
new file mode 100644
index 000000000000..e8d5eeb4eb31
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
@@ -0,0 +1,973 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CZ.NIC's Turris Omnia MCU GPIO and IRQ driver
+ *
+ * 2023 by Marek Behún <kabel@kernel.org>
+ */
+
+#include <asm/unaligned.h>
+
+#include "turris-omnia-mcu.h"
+
+static const char * const omnia_mcu_gpio_names[64] = {
+ /* GPIOs with value read from the 16-bit wide status */
+ [4] = "gpio%u.MiniPCIe0 Card Detect",
+ [5] = "gpio%u.MiniPCIe0 mSATA Indicator",
+ [6] = "gpio%u.Front USB3 port over-current",
+ [7] = "gpio%u.Rear USB3 port over-current",
+ [8] = "gpio%u.Front USB3 port power",
+ [9] = "gpio%u.Rear USB3 port power",
+ [12] = "gpio%u.Front Button",
+
+ /* GPIOs with value read from the 32-bit wide extended status */
+ [16] = "gpio%u.SFP nDET",
+ [28] = "gpio%u.MiniPCIe0 LED",
+ [29] = "gpio%u.MiniPCIe1 LED",
+ [30] = "gpio%u.MiniPCIe2 LED",
+ [31] = "gpio%u.MiniPCIe0 PAN LED",
+ [32] = "gpio%u.MiniPCIe1 PAN LED",
+ [33] = "gpio%u.MiniPCIe2 PAN LED",
+ [34] = "gpio%u.WAN PHY LED0",
+ [35] = "gpio%u.WAN PHY LED1",
+ [36] = "gpio%u.LAN switch p0 LED0",
+ [37] = "gpio%u.LAN switch p0 LED1",
+ [38] = "gpio%u.LAN switch p1 LED0",
+ [39] = "gpio%u.LAN switch p1 LED1",
+ [40] = "gpio%u.LAN switch p2 LED0",
+ [41] = "gpio%u.LAN switch p2 LED1",
+ [42] = "gpio%u.LAN switch p3 LED0",
+ [43] = "gpio%u.LAN switch p3 LED1",
+ [44] = "gpio%u.LAN switch p4 LED0",
+ [45] = "gpio%u.LAN switch p4 LED1",
+ [46] = "gpio%u.LAN switch p5 LED0",
+ [47] = "gpio%u.LAN switch p5 LED1",
+
+ /* GPIOs with value read from the 16-bit wide extended control status */
+ [48] = "gpio%u.eMMC nRESET",
+ [49] = "gpio%u.LAN switch nRESET",
+ [50] = "gpio%u.WAN PHY nRESET",
+ [51] = "gpio%u.MiniPCIe0 nPERST",
+ [52] = "gpio%u.MiniPCIe1 nPERST",
+ [53] = "gpio%u.MiniPCIe2 nPERST",
+ [54] = "gpio%u.WAN PHY SFP mux",
+};
+
+#define _DEF_GPIO(_cmd, _ctl_cmd, _bit, _ctl_bit, _int_bit, _feat, _feat_mask) \
+ { \
+ .cmd = _cmd, \
+ .ctl_cmd = _ctl_cmd, \
+ .bit = _bit, \
+ .ctl_bit = _ctl_bit, \
+ .int_bit = _int_bit, \
+ .feat = _feat, \
+ .feat_mask = _feat_mask, \
+ }
+#define _DEF_GPIO_STS(_name) \
+ _DEF_GPIO(CMD_GET_STATUS_WORD, 0, STS_ ## _name, 0, INT_ ## _name, 0, 0)
+#define _DEF_GPIO_CTL(_name) \
+ _DEF_GPIO(CMD_GET_STATUS_WORD, CMD_GENERAL_CONTROL, STS_ ## _name, \
+ CTL_ ## _name, 0, 0, 0)
+#define _DEF_GPIO_EXT_STS(_name, _feat) \
+ _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \
+ INT_ ## _name, FEAT_ ## _feat | FEAT_EXT_CMDS, \
+ FEAT_ ## _feat | FEAT_EXT_CMDS)
+#define _DEF_GPIO_EXT_STS_LED(_name, _ledext) \
+ _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \
+ INT_ ## _name, FEAT_LED_STATE_ ## _ledext, \
+ FEAT_LED_STATE_EXT_MASK)
+#define _DEF_GPIO_EXT_STS_LEDALL(_name) \
+ _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \
+ INT_ ## _name, FEAT_LED_STATE_EXT_MASK, 0)
+#define _DEF_GPIO_EXT_CTL(_name, _feat) \
+ _DEF_GPIO(CMD_GET_EXT_CONTROL_STATUS, CMD_EXT_CONTROL, \
+ EXT_CTL_ ## _name, EXT_CTL_ ## _name, 0, \
+ FEAT_ ## _feat | FEAT_EXT_CMDS, \
+ FEAT_ ## _feat | FEAT_EXT_CMDS)
+#define _DEF_INT(_name) \
+ _DEF_GPIO(0, 0, 0, 0, INT_ ## _name, 0, 0)
+
+static const struct omnia_gpio {
+ u8 cmd, ctl_cmd;
+ u32 bit, ctl_bit;
+ u32 int_bit;
+ u16 feat, feat_mask;
+} omnia_gpios[64] = {
+ /* GPIOs with value read from the 16-bit wide status */
+ [4] = _DEF_GPIO_STS(CARD_DET),
+ [5] = _DEF_GPIO_STS(MSATA_IND),
+ [6] = _DEF_GPIO_STS(USB30_OVC),
+ [7] = _DEF_GPIO_STS(USB31_OVC),
+ [8] = _DEF_GPIO_CTL(USB30_PWRON),
+ [9] = _DEF_GPIO_CTL(USB31_PWRON),
+
+ /* brightness changed interrupt, no GPIO */
+ [11] = _DEF_INT(BRIGHTNESS_CHANGED),
+
+ [12] = _DEF_GPIO_STS(BUTTON_PRESSED),
+
+ /* GPIOs with value read from the 32-bit wide extended status */
+ [16] = _DEF_GPIO_EXT_STS(SFP_nDET, PERIPH_MCU),
+ [28] = _DEF_GPIO_EXT_STS_LEDALL(WLAN0_MSATA_LED),
+ [29] = _DEF_GPIO_EXT_STS_LEDALL(WLAN1_LED),
+ [30] = _DEF_GPIO_EXT_STS_LEDALL(WLAN2_LED),
+ [31] = _DEF_GPIO_EXT_STS_LED(WPAN0_LED, EXT),
+ [32] = _DEF_GPIO_EXT_STS_LED(WPAN1_LED, EXT),
+ [33] = _DEF_GPIO_EXT_STS_LED(WPAN2_LED, EXT),
+ [34] = _DEF_GPIO_EXT_STS_LEDALL(WAN_LED0),
+ [35] = _DEF_GPIO_EXT_STS_LED(WAN_LED1, EXT_V32),
+ [36] = _DEF_GPIO_EXT_STS_LEDALL(LAN0_LED0),
+ [37] = _DEF_GPIO_EXT_STS_LEDALL(LAN0_LED1),
+ [38] = _DEF_GPIO_EXT_STS_LEDALL(LAN1_LED0),
+ [39] = _DEF_GPIO_EXT_STS_LEDALL(LAN1_LED1),
+ [40] = _DEF_GPIO_EXT_STS_LEDALL(LAN2_LED0),
+ [41] = _DEF_GPIO_EXT_STS_LEDALL(LAN2_LED1),
+ [42] = _DEF_GPIO_EXT_STS_LEDALL(LAN3_LED0),
+ [43] = _DEF_GPIO_EXT_STS_LEDALL(LAN3_LED1),
+ [44] = _DEF_GPIO_EXT_STS_LEDALL(LAN4_LED0),
+ [45] = _DEF_GPIO_EXT_STS_LEDALL(LAN4_LED1),
+ [46] = _DEF_GPIO_EXT_STS_LEDALL(LAN5_LED0),
+ [47] = _DEF_GPIO_EXT_STS_LEDALL(LAN5_LED1),
+
+ /* GPIOs with value read from the 16-bit wide extended control status */
+ [48] = _DEF_GPIO_EXT_CTL(nRES_MMC, PERIPH_MCU),
+ [49] = _DEF_GPIO_EXT_CTL(nRES_LAN, PERIPH_MCU),
+ [50] = _DEF_GPIO_EXT_CTL(nRES_PHY, PERIPH_MCU),
+ [51] = _DEF_GPIO_EXT_CTL(nPERST0, PERIPH_MCU),
+ [52] = _DEF_GPIO_EXT_CTL(nPERST1, PERIPH_MCU),
+ [53] = _DEF_GPIO_EXT_CTL(nPERST2, PERIPH_MCU),
+ [54] = _DEF_GPIO_EXT_CTL(PHY_SFP, PERIPH_MCU),
+};
+
+/* mapping from interrupts to indexes of GPIOs in the omnia_gpios array */
+static const unsigned int omnia_int_to_gpio_idx[32] = {
+ [ilog2(INT_CARD_DET)] = 4,
+ [ilog2(INT_MSATA_IND)] = 5,
+ [ilog2(INT_USB30_OVC)] = 6,
+ [ilog2(INT_USB31_OVC)] = 7,
+ [ilog2(INT_BUTTON_PRESSED)] = 12,
+ [ilog2(INT_SFP_nDET)] = 16,
+ [ilog2(INT_BRIGHTNESS_CHANGED)] = 11,
+ [ilog2(INT_WLAN0_MSATA_LED)] = 28,
+ [ilog2(INT_WLAN1_LED)] = 29,
+ [ilog2(INT_WLAN2_LED)] = 30,
+ [ilog2(INT_WPAN0_LED)] = 31,
+ [ilog2(INT_WPAN1_LED)] = 32,
+ [ilog2(INT_WPAN2_LED)] = 33,
+ [ilog2(INT_WAN_LED0)] = 34,
+ [ilog2(INT_WAN_LED1)] = 35,
+ [ilog2(INT_LAN0_LED0)] = 36,
+ [ilog2(INT_LAN0_LED1)] = 37,
+ [ilog2(INT_LAN1_LED0)] = 38,
+ [ilog2(INT_LAN1_LED1)] = 39,
+ [ilog2(INT_LAN2_LED0)] = 40,
+ [ilog2(INT_LAN2_LED1)] = 41,
+ [ilog2(INT_LAN3_LED0)] = 42,
+ [ilog2(INT_LAN3_LED1)] = 43,
+ [ilog2(INT_LAN4_LED0)] = 44,
+ [ilog2(INT_LAN4_LED1)] = 45,
+ [ilog2(INT_LAN5_LED0)] = 46,
+ [ilog2(INT_LAN5_LED1)] = 47,
+};
+
+/* index of PHY_SFP GPIO in the omnia_gpios array */
+enum {
+ OMNIA_GPIO_PHY_SFP_OFFSET = 54,
+};
+
+static int omnia_ctl_cmd_unlocked(struct omnia_mcu *mcu, u8 cmd, u16 val,
+ u16 mask)
+{
+ size_t len;
+ u8 buf[5];
+
+ buf[0] = cmd;
+
+ switch (cmd) {
+ case CMD_GENERAL_CONTROL:
+ buf[1] = val;
+ buf[2] = mask;
+ len = 3;
+ break;
+
+ case CMD_EXT_CONTROL:
+ put_unaligned_le16(val, &buf[1]);
+ put_unaligned_le16(mask, &buf[3]);
+ len = 5;
+ break;
+
+ default:
+ unreachable();
+ }
+
+ return omnia_cmd_write(mcu->client, buf, len);
+}
+
+static int omnia_ctl_cmd(struct omnia_mcu *mcu, u8 cmd, u16 val, u16 mask)
+{
+ int err;
+
+ mutex_lock(&mcu->lock);
+ err = omnia_ctl_cmd_unlocked(mcu, cmd, val, mask);
+ mutex_unlock(&mcu->lock);
+
+ return err;
+}
+
+static int omnia_gpio_request(struct gpio_chip *gc, unsigned int offset)
+{
+ if (!omnia_gpios[offset].cmd)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int omnia_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+
+ if (offset == OMNIA_GPIO_PHY_SFP_OFFSET) {
+ int val;
+
+ mutex_lock(&mcu->lock);
+ val = omnia_cmd_read_bit(mcu->client,
+ CMD_GET_EXT_CONTROL_STATUS,
+ EXT_CTL_PHY_SFP_AUTO);
+ mutex_unlock(&mcu->lock);
+
+ if (val < 0)
+ return val;
+
+ if (val)
+ return GPIO_LINE_DIRECTION_IN;
+ else
+ return GPIO_LINE_DIRECTION_OUT;
+ }
+
+ if (omnia_gpios[offset].ctl_cmd)
+ return GPIO_LINE_DIRECTION_OUT;
+ else
+ return GPIO_LINE_DIRECTION_IN;
+}
+
+static int omnia_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+ const struct omnia_gpio *gpio = &omnia_gpios[offset];
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+
+ if (offset == OMNIA_GPIO_PHY_SFP_OFFSET)
+ return omnia_ctl_cmd(mcu, CMD_EXT_CONTROL, EXT_CTL_PHY_SFP_AUTO,
+ EXT_CTL_PHY_SFP_AUTO);
+
+ if (gpio->ctl_cmd)
+ return -EOPNOTSUPP;
+
+ return 0;
+}
+
+static int omnia_gpio_direction_output(struct gpio_chip *gc,
+ unsigned int offset, int value)
+{
+ const struct omnia_gpio *gpio = &omnia_gpios[offset];
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+ uint16_t val, mask;
+
+ mask = gpio->ctl_bit;
+ val = value ? mask : 0;
+
+ if (offset == OMNIA_GPIO_PHY_SFP_OFFSET)
+ mask |= EXT_CTL_PHY_SFP_AUTO;
+
+ if (gpio->ctl_cmd)
+ return omnia_ctl_cmd(mcu, gpio->ctl_cmd, val, mask);
+ else
+ return -EOPNOTSUPP;
+}
+
+static int omnia_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+ const struct omnia_gpio *gpio = &omnia_gpios[offset];
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+ int ret;
+
+ /*
+ * If firmware does not support the new interrupt API, we are informed
+ * of every change of the status word by an interrupt from MCU and save
+ * its value in the interrupt service routine. Simply return the saved
+ * value.
+ */
+ if (gpio->cmd == CMD_GET_STATUS_WORD &&
+ !(mcu->features & FEAT_NEW_INT_API))
+ return !!(mcu->last_status & gpio->bit);
+
+ mutex_lock(&mcu->lock);
+
+ /*
+ * If firmware does support the new interrupt API, we may have cached
+ * the value of a GPIO in the interrupt service routine. If not, read
+ * the relevant bit now.
+ */
+ if (gpio->int_bit && (mcu->is_cached & gpio->int_bit))
+ ret = !!(mcu->cached & gpio->int_bit);
+ else
+ ret = omnia_cmd_read_bit(mcu->client, gpio->cmd, gpio->bit);
+
+ mutex_unlock(&mcu->lock);
+
+ return ret;
+}
+
+static int omnia_gpio_get_multiple(struct gpio_chip *gc, unsigned long *mask,
+ unsigned long *bits)
+{
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+ u32 sts_bits, ext_sts_bits, ext_ctl_bits;
+ int err, i;
+
+ sts_bits = 0;
+ ext_sts_bits = 0;
+ ext_ctl_bits = 0;
+
+ /* determine which bits to read from the 3 possible commands */
+ for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
+ if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD)
+ sts_bits |= omnia_gpios[i].bit;
+ else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD)
+ ext_sts_bits |= omnia_gpios[i].bit;
+ else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS)
+ ext_ctl_bits |= omnia_gpios[i].bit;
+ }
+
+ mutex_lock(&mcu->lock);
+
+ if (mcu->features & FEAT_NEW_INT_API) {
+ /* read relevant bits from status */
+ err = omnia_cmd_read_bits(mcu->client, CMD_GET_STATUS_WORD,
+ sts_bits, &sts_bits);
+ if (err)
+ goto err_unlock;
+ } else {
+ /*
+ * Use status word value cached in the interrupt service routine
+ * if firmware does not support the new interrupt API.
+ */
+ sts_bits = mcu->last_status;
+ }
+
+ /* read relevant bits from extended status */
+ err = omnia_cmd_read_bits(mcu->client, CMD_GET_EXT_STATUS_DWORD,
+ ext_sts_bits, &ext_sts_bits);
+ if (err)
+ goto err_unlock;
+
+ /* read relevant bits from extended control */
+ err = omnia_cmd_read_bits(mcu->client, CMD_GET_EXT_CONTROL_STATUS,
+ ext_ctl_bits, &ext_ctl_bits);
+ if (err)
+ goto err_unlock;
+
+ mutex_unlock(&mcu->lock);
+
+ /* assign relevant bits in result */
+ for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
+ if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD)
+ assign_bit(i, bits, sts_bits & omnia_gpios[i].bit);
+ else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD)
+ assign_bit(i, bits, ext_sts_bits & omnia_gpios[i].bit);
+ else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS)
+ assign_bit(i, bits, ext_ctl_bits & omnia_gpios[i].bit);
+ }
+
+ return 0;
+
+err_unlock:
+ mutex_unlock(&mcu->lock);
+ return err;
+}
+
+static void omnia_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+ const struct omnia_gpio *gpio = &omnia_gpios[offset];
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+ u16 val, mask;
+ int err = 0;
+
+ if (!gpio->ctl_cmd)
+ return;
+
+ mask = gpio->ctl_bit;
+ val = value ? mask : 0;
+
+ err = omnia_ctl_cmd(mcu, gpio->ctl_cmd, val, mask);
+ if (err)
+ dev_err(&mcu->client->dev, "Cannot set GPIO %u: %d\n",
+ offset, err);
+}
+
+static void omnia_gpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+ unsigned long *bits)
+{
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+ u16 ext_ctl, ext_ctl_mask;
+ u8 ctl, ctl_mask;
+ int err = 0, i;
+
+ ctl = 0;
+ ctl_mask = 0;
+ ext_ctl = 0;
+ ext_ctl_mask = 0;
+
+ for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
+ if (omnia_gpios[i].ctl_cmd == CMD_GENERAL_CONTROL) {
+ ctl_mask |= omnia_gpios[i].ctl_bit;
+ if (test_bit(i, bits))
+ ctl |= omnia_gpios[i].ctl_bit;
+ } else if (omnia_gpios[i].ctl_cmd == CMD_EXT_CONTROL) {
+ ext_ctl_mask |= omnia_gpios[i].ctl_bit;
+ if (test_bit(i, bits))
+ ext_ctl |= omnia_gpios[i].ctl_bit;
+ }
+ }
+
+ mutex_lock(&mcu->lock);
+
+ if (ctl_mask)
+ err = omnia_ctl_cmd_unlocked(mcu, CMD_GENERAL_CONTROL, ctl,
+ ctl_mask);
+
+ if (!err && ext_ctl_mask)
+ err = omnia_ctl_cmd_unlocked(mcu, CMD_EXT_CONTROL, ext_ctl,
+ ext_ctl_mask);
+
+ mutex_unlock(&mcu->lock);
+
+ if (err)
+ dev_err(&mcu->client->dev, "Cannot set GPIOs: %d\n", err);
+}
+
+static bool omnia_gpio_available(struct omnia_mcu *mcu,
+ const struct omnia_gpio *gpio)
+{
+ if (gpio->feat_mask)
+ return (mcu->features & gpio->feat_mask) == gpio->feat;
+ else if (gpio->feat)
+ return mcu->features & gpio->feat;
+ else
+ return true;
+}
+
+static int omnia_gpio_init_valid_mask(struct gpio_chip *gc,
+ unsigned long *valid_mask,
+ unsigned int ngpios)
+{
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+
+ bitmap_zero(valid_mask, ngpios);
+
+ for (int i = 0; i < ngpios; ++i) {
+ const struct omnia_gpio *gpio = &omnia_gpios[i];
+
+ if (!gpio->cmd && !gpio->int_bit)
+ continue;
+
+ if (omnia_gpio_available(mcu, gpio))
+ set_bit(i, valid_mask);
+ }
+
+ return 0;
+}
+
+static void omnia_irq_shutdown(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+ irq_hw_number_t hwirq = irqd_to_hwirq(d);
+ u32 bit = omnia_gpios[hwirq].int_bit;
+
+ mcu->rising &= ~bit;
+ mcu->falling &= ~bit;
+}
+
+static void omnia_irq_mask(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+ irq_hw_number_t hwirq = irqd_to_hwirq(d);
+ u32 bit = omnia_gpios[hwirq].int_bit;
+
+ if (!omnia_gpios[hwirq].cmd)
+ mcu->rising &= ~bit;
+ mcu->mask &= ~bit;
+ gpiochip_disable_irq(gc, hwirq);
+}
+
+static void omnia_irq_unmask(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+ irq_hw_number_t hwirq = irqd_to_hwirq(d);
+ u32 bit = omnia_gpios[hwirq].int_bit;
+
+ gpiochip_enable_irq(gc, hwirq);
+ mcu->mask |= bit;
+ if (!omnia_gpios[hwirq].cmd)
+ mcu->rising |= bit;
+}
+
+static int omnia_irq_set_type(struct irq_data *d, unsigned int type)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+ irq_hw_number_t hwirq = irqd_to_hwirq(d);
+ u32 bit = omnia_gpios[hwirq].int_bit;
+
+ if (!(type & IRQ_TYPE_EDGE_BOTH)) {
+ dev_err(&mcu->client->dev, "irq %u: unsupported type %u\n",
+ d->irq, type);
+ return -EINVAL;
+ }
+
+ if (type & IRQ_TYPE_EDGE_RISING)
+ mcu->rising |= bit;
+ else
+ mcu->rising &= ~bit;
+
+ if (type & IRQ_TYPE_EDGE_FALLING)
+ mcu->falling |= bit;
+ else
+ mcu->falling &= ~bit;
+
+ return 0;
+}
+
+static void omnia_irq_bus_lock(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+
+ /* nothing to do if MCU firmware does not support new interrupt API */
+ if (!(mcu->features & FEAT_NEW_INT_API))
+ return;
+
+ mutex_lock(&mcu->lock);
+}
+
+static void omnia_irq_bus_sync_unlock(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+ u32 rising, falling;
+ u8 cmd[9];
+ int err;
+
+ /* nothing to do if MCU firmware does not support new interrupt API */
+ if (!(mcu->features & FEAT_NEW_INT_API))
+ return;
+
+ cmd[0] = CMD_SET_INT_MASK;
+
+ rising = mcu->rising & mcu->mask;
+ falling = mcu->falling & mcu->mask;
+
+ cmd[1] = rising;
+ cmd[2] = falling;
+ cmd[3] = rising >> 8;
+ cmd[4] = falling >> 8;
+ cmd[5] = rising >> 16;
+ cmd[6] = falling >> 16;
+ cmd[7] = rising >> 24;
+ cmd[8] = falling >> 24;
+
+ dev_dbg(&mcu->client->dev,
+ "set int mask %02x %02x %02x %02x %02x %02x %02x %02x\n",
+ cmd[1], cmd[2], cmd[3], cmd[4], cmd[5], cmd[6], cmd[7], cmd[8]);
+
+ err = omnia_cmd_write(mcu->client, cmd, sizeof(cmd));
+ if (err)
+ dev_err(&mcu->client->dev, "Cannot set mask: %d\n", err);
+
+ /*
+ * Remember which GPIOs have both rising and falling interrupts enabled.
+ * For those we will cache their value so that .get() method is faster.
+ * We also need to forget cached values of GPIOs that aren't cached
+ * anymore.
+ */
+ if (!err) {
+ mcu->both = rising & falling;
+ mcu->is_cached &= mcu->both;
+ }
+
+ mutex_unlock(&mcu->lock);
+}
+
+static const struct irq_chip omnia_mcu_irq_chip = {
+ .name = "Turris Omnia MCU interrupts",
+ .irq_shutdown = omnia_irq_shutdown,
+ .irq_mask = omnia_irq_mask,
+ .irq_unmask = omnia_irq_unmask,
+ .irq_set_type = omnia_irq_set_type,
+ .irq_bus_lock = omnia_irq_bus_lock,
+ .irq_bus_sync_unlock = omnia_irq_bus_sync_unlock,
+ .flags = IRQCHIP_IMMUTABLE,
+ GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static void omnia_irq_init_valid_mask(struct gpio_chip *gc,
+ unsigned long *valid_mask,
+ unsigned int ngpios)
+{
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+
+ bitmap_zero(valid_mask, ngpios);
+
+ for (int i = 0; i < ngpios; ++i) {
+ const struct omnia_gpio *gpio = &omnia_gpios[i];
+
+ if (!gpio->int_bit)
+ continue;
+
+ if (omnia_gpio_available(mcu, gpio))
+ set_bit(i, valid_mask);
+ }
+}
+
+static int omnia_irq_init_hw(struct gpio_chip *gc)
+{
+ struct omnia_mcu *mcu = gpiochip_get_data(gc);
+ u8 cmd[9] = {};
+
+ cmd[0] = CMD_SET_INT_MASK;
+
+ return omnia_cmd_write(mcu->client, cmd, sizeof(cmd));
+}
+
+static bool omnia_irq_read_pending_new(struct omnia_mcu *mcu,
+ unsigned long *pending)
+{
+ u32 rising, falling;
+ u8 reply[8] = {};
+ size_t len;
+ int err;
+
+ /*
+ * Determine how many bytes we need to read from the reply to the
+ * CMD_GET_INT_AND_CLEAR command in order to retrieve all unmasked
+ * interrupts.
+ */
+ len = max(((ilog2(mcu->rising & mcu->mask) >> 3) << 1) + 1,
+ ((ilog2(mcu->falling & mcu->mask) >> 3) << 1) + 2);
+
+ mutex_lock(&mcu->lock);
+
+ err = omnia_cmd_read(mcu->client, CMD_GET_INT_AND_CLEAR, reply,
+ len);
+ if (err) {
+ mutex_unlock(&mcu->lock);
+ dev_err(&mcu->client->dev, "Cannot read pending IRQs: %d\n",
+ err);
+ return false;
+ }
+
+ rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
+ (reply[6] << 24);
+ falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
+ (reply[7] << 24);
+
+ rising &= mcu->mask;
+ falling &= mcu->mask;
+ *pending = rising | falling;
+
+ /* cache values for GPIOs that have both edges enabled */
+ mcu->is_cached &= ~(rising & falling);
+ mcu->is_cached |= mcu->both & (rising ^ falling);
+ mcu->cached = (mcu->cached | rising) & ~falling;
+
+ mutex_unlock(&mcu->lock);
+
+ return true;
+}
+
+static int omnia_read_status_word_old_fw(struct omnia_mcu *mcu, u16 *status)
+{
+ int ret = omnia_cmd_read_u16(mcu->client, CMD_GET_STATUS_WORD);
+
+ if (ret < 0)
+ return ret;
+
+ /*
+ * Old firmware has a bug wherein it never resets the USB port
+ * overcurrent bits back to zero. Ignore them.
+ */
+ *status = ret & ~(STS_USB30_OVC | STS_USB31_OVC);
+
+ return 0;
+}
+
+static void button_release_emul_fn(struct work_struct *work)
+{
+ struct omnia_mcu *mcu = container_of(to_delayed_work(work),
+ struct omnia_mcu,
+ button_release_emul_work);
+
+ mcu->button_pressed_emul = false;
+ generic_handle_irq_safe(mcu->client->irq);
+}
+
+static void fill_int_from_sts(u32 *rising, u32 *falling, u16 rising_sts,
+ u16 falling_sts, u16 sts_bit, u32 int_bit)
+{
+ if (rising_sts & sts_bit)
+ *rising |= int_bit;
+ if (falling_sts & sts_bit)
+ *falling |= int_bit;
+}
+
+static bool omnia_irq_read_pending_old(struct omnia_mcu *mcu,
+ unsigned long *pending)
+{
+ u16 status, rising_sts, falling_sts;
+ u32 rising, falling;
+ int ret;
+
+ mutex_lock(&mcu->lock);
+
+ ret = omnia_read_status_word_old_fw(mcu, &status);
+ if (ret < 0) {
+ mutex_unlock(&mcu->lock);
+ dev_err(&mcu->client->dev, "Cannot read pending IRQs: %d\n",
+ ret);
+ return false;
+ }
+
+ /*
+ * The old firmware triggers an interrupt whenever status word changes,
+ * but does not inform about which bits rose or fell. We need to compute
+ * this here by comparing with the last status word value.
+ *
+ * The STS_BUTTON_PRESSED bit needs special handling, because the old
+ * firmware clears the STS_BUTTON_PRESSED bit on successful completion
+ * of the CMD_GET_STATUS_WORD command, resulting in another interrupt:
+ * - first we get an interrupt, we read the status word where
+ * STS_BUTTON_PRESSED is present,
+ * - MCU clears the STS_BUTTON_PRESSED bit because we read the status
+ * word,
+ * - we get another interrupt because the status word changed again
+ * (STS_BUTTON_PRESSED was cleared).
+ *
+ * The gpiolib-cdev, gpiolib-sysfs and gpio-keys input driver all call
+ * the gpiochip's .get() method after an edge event on a requested GPIO
+ * occurs.
+ *
+ * We ensure that the .get() method reads 1 for the button GPIO for some
+ * time.
+ */
+
+ if (status & STS_BUTTON_PRESSED) {
+ mcu->button_pressed_emul = true;
+ mod_delayed_work(system_wq, &mcu->button_release_emul_work,
+ msecs_to_jiffies(50));
+ } else if (mcu->button_pressed_emul) {
+ status |= STS_BUTTON_PRESSED;
+ }
+
+ rising_sts = ~mcu->last_status & status;
+ falling_sts = mcu->last_status & ~status;
+
+ mcu->last_status = status;
+
+ /*
+ * Fill in the relevant interrupt bits from status bits for CARD_DET,
+ * MSATA_IND and BUTTON_PRESSED.
+ */
+ rising = 0;
+ falling = 0;
+ fill_int_from_sts(&rising, &falling, rising_sts, falling_sts,
+ STS_CARD_DET, INT_CARD_DET);
+ fill_int_from_sts(&rising, &falling, rising_sts, falling_sts,
+ STS_MSATA_IND, INT_MSATA_IND);
+ fill_int_from_sts(&rising, &falling, rising_sts, falling_sts,
+ STS_BUTTON_PRESSED, INT_BUTTON_PRESSED);
+
+ /* Use only bits that are enabled */
+ rising &= mcu->rising & mcu->mask;
+ falling &= mcu->falling & mcu->mask;
+ *pending = rising | falling;
+
+ mutex_unlock(&mcu->lock);
+
+ return true;
+}
+
+static bool omnia_irq_read_pending(struct omnia_mcu *mcu,
+ unsigned long *pending)
+{
+ if (mcu->features & FEAT_NEW_INT_API)
+ return omnia_irq_read_pending_new(mcu, pending);
+ else
+ return omnia_irq_read_pending_old(mcu, pending);
+}
+
+static irqreturn_t omnia_irq_thread_handler(int irq, void *dev_id)
+{
+ struct omnia_mcu *mcu = dev_id;
+ irqreturn_t res = IRQ_NONE;
+ unsigned long pending;
+ int i;
+
+ if (!omnia_irq_read_pending(mcu, &pending))
+ return IRQ_NONE;
+
+ for_each_set_bit(i, &pending, 32) {
+ unsigned int nested_irq =
+ irq_find_mapping(mcu->gc.irq.domain,
+ omnia_int_to_gpio_idx[i]);
+
+ handle_nested_irq(nested_irq);
+
+ res = IRQ_HANDLED;
+ }
+
+ return res;
+}
+
+static ssize_t front_button_mode_show(struct device *dev,
+ struct device_attribute *a, char *buf)
+{
+ struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
+ int val;
+
+ if (mcu->features & FEAT_NEW_INT_API)
+ val = omnia_cmd_read_bit(mcu->client, CMD_GET_STATUS_WORD,
+ STS_BUTTON_MODE);
+ else
+ val = !!(mcu->last_status & STS_BUTTON_MODE);
+
+ if (val < 0)
+ return val;
+
+ return sysfs_emit(buf, "%s\n", val ? "cpu" : "mcu");
+}
+
+static ssize_t front_button_mode_store(struct device *dev,
+ struct device_attribute *a,
+ const char *buf, size_t count)
+{
+ struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
+ u8 mask, val;
+ int err;
+
+ mask = CTL_BUTTON_MODE;
+
+ if (sysfs_streq(buf, "mcu"))
+ val = 0;
+ else if (sysfs_streq(buf, "cpu"))
+ val = mask;
+ else
+ return -EINVAL;
+
+ err = omnia_ctl_cmd_unlocked(mcu, CMD_GENERAL_CONTROL, val, mask);
+ if (err)
+ return err;
+
+ return count;
+}
+static DEVICE_ATTR_RW(front_button_mode);
+
+static struct attribute *omnia_mcu_gpio_attrs[] = {
+ &dev_attr_front_button_mode.attr,
+ NULL,
+};
+
+static const struct attribute_group omnia_mcu_gpio_group = {
+ .attrs = omnia_mcu_gpio_attrs,
+};
+
+int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu)
+{
+ bool new_api = mcu->features & FEAT_NEW_INT_API;
+ struct device *dev = &mcu->client->dev;
+ unsigned long irqflags;
+ int err;
+
+ mutex_init(&mcu->lock);
+
+ mcu->gc.request = omnia_gpio_request;
+ mcu->gc.get_direction = omnia_gpio_get_direction;
+ mcu->gc.direction_input = omnia_gpio_direction_input;
+ mcu->gc.direction_output = omnia_gpio_direction_output;
+ mcu->gc.get = omnia_gpio_get;
+ mcu->gc.get_multiple = omnia_gpio_get_multiple;
+ mcu->gc.set = omnia_gpio_set;
+ mcu->gc.set_multiple = omnia_gpio_set_multiple;
+ mcu->gc.init_valid_mask = omnia_gpio_init_valid_mask;
+ mcu->gc.can_sleep = true;
+ mcu->gc.names = omnia_mcu_gpio_names;
+ mcu->gc.base = -1;
+ mcu->gc.ngpio = ARRAY_SIZE(omnia_gpios);
+ mcu->gc.label = "Turris Omnia MCU GPIOs";
+ mcu->gc.parent = dev;
+ mcu->gc.owner = THIS_MODULE;
+
+ gpio_irq_chip_set_chip(&mcu->gc.irq, &omnia_mcu_irq_chip);
+ /* This will let us handle the parent IRQ in the driver */
+ mcu->gc.irq.parent_handler = NULL;
+ mcu->gc.irq.num_parents = 0;
+ mcu->gc.irq.parents = NULL;
+ mcu->gc.irq.default_type = IRQ_TYPE_NONE;
+ mcu->gc.irq.handler = handle_bad_irq;
+ mcu->gc.irq.threaded = true;
+ if (new_api)
+ mcu->gc.irq.init_hw = omnia_irq_init_hw;
+ mcu->gc.irq.init_valid_mask = omnia_irq_init_valid_mask;
+
+ err = devm_gpiochip_add_data(dev, &mcu->gc, mcu);
+ if (err) {
+ dev_err(dev, "Cannot add GPIO chip: %d\n", err);
+ return err;
+ }
+
+ /*
+ * Before requesting the interrupt, if firmware does not support the new
+ * interrupt API, we need to cache the value of the status word, so that
+ * when it changes, we may compare the new value with the cached one in
+ * the interrupt handler.
+ */
+ if (!new_api) {
+ err = omnia_read_status_word_old_fw(mcu, &mcu->last_status);
+ if (err < 0) {
+ dev_err(dev, "Cannot read status word: %d\n", err);
+ return err;
+ }
+
+ INIT_DELAYED_WORK(&mcu->button_release_emul_work,
+ button_release_emul_fn);
+ }
+
+ irqflags = IRQF_ONESHOT;
+ if (new_api)
+ irqflags |= IRQF_TRIGGER_LOW;
+ else
+ irqflags |= IRQF_TRIGGER_FALLING;
+
+ err = devm_request_threaded_irq(dev, mcu->client->irq, NULL,
+ omnia_irq_thread_handler, irqflags,
+ "turris-omnia-mcu", mcu);
+ if (err) {
+ dev_err(dev, "Cannot request IRQ: %d\n", err);
+ return err;
+ }
+
+ err = devm_device_add_group(dev, &omnia_mcu_gpio_group);
+ if (err)
+ dev_err(dev, "Cannot add GPIO sysfs attribute group: %d\n",
+ err);
+
+ return err;
+}
+
+void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu)
+{
+ if (!(mcu->features & FEAT_NEW_INT_API))
+ cancel_delayed_work_sync(&mcu->button_release_emul_work);
+
+ mutex_destroy(&mcu->lock);
+}
diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h
index a045b114e293..b5802240aa8d 100644
--- a/drivers/platform/cznic/turris-omnia-mcu.h
+++ b/drivers/platform/cznic/turris-omnia-mcu.h
@@ -9,15 +9,36 @@
#define __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
#include <asm/byteorder.h>
+#include <linux/gpio/driver.h>
#include <linux/i2c.h>
+#include <linux/log2.h>
+#include <linux/mutex.h>
#include <linux/turris-omnia-mcu-interface.h>
+#include <linux/workqueue.h>
struct omnia_mcu {
struct i2c_client *client;
const char *type;
u16 features;
+
+ /* GPIO chip */
+ struct gpio_chip gc;
+ struct mutex lock;
+ u32 mask, rising, falling, both, cached, is_cached;
+ /* Old MCU firmware handling needs the following */
+ u16 last_status;
+ struct delayed_work button_release_emul_work;
+ bool button_pressed_emul;
};
+static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
+ size_t len)
+{
+ int ret = i2c_master_send(client, cmd, len);
+
+ return ret < 0 ? ret : 0;
+}
+
static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
unsigned int len)
{
@@ -42,6 +63,38 @@ static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *
return -EIO;
}
+/* Returns 0 on success */
+static inline int omnia_cmd_read_bits(const struct i2c_client *client, u8 cmd,
+ u32 bits, u32 *reply)
+{
+ int ret;
+
+ if (!bits) {
+ *reply = 0;
+ return 0;
+ }
+
+ ret = omnia_cmd_read(client, cmd, reply, (ilog2(bits) >> 3) + 1);
+
+ if (ret >= 0)
+ *reply = le32_to_cpu(*reply) & bits;
+
+ return ret < 0 ? ret : 0;
+}
+
+static inline int omnia_cmd_read_bit(const struct i2c_client *client, u8 cmd,
+ u32 bit)
+{
+ u32 reply;
+ int err;
+
+ err = omnia_cmd_read_bits(client, cmd, bit, &reply);
+ if (err)
+ return err;
+
+ return !!reply;
+}
+
static inline int omnia_cmd_read_u16(const struct i2c_client *client, u8 cmd)
{
u16 reply;
@@ -62,4 +115,7 @@ static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
return ret < 0 ? ret : reply;
}
+int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu);
+void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu);
+
#endif /* __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H */
--
2.41.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 4/7] platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup
2023-09-19 10:38 [PATCH v2 0/7] Turris Omnia MCU driver Marek Behún
` (2 preceding siblings ...)
2023-09-19 10:38 ` [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs Marek Behún
@ 2023-09-19 10:38 ` Marek Behún
2023-09-19 10:38 ` [PATCH v2 5/7] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog Marek Behún
` (2 subsequent siblings)
6 siblings, 0 replies; 30+ messages in thread
From: Marek Behún @ 2023-09-19 10:38 UTC (permalink / raw)
To: Gregory CLEMENT, Arnd Bergmann, soc, arm, Alessandro Zummo,
Alexandre Belloni, linux-rtc
Cc: Marek Behún
Add support for true board poweroff (MCU can disable all unnecessary
voltage regulators) and wakeup at a specified time, implemented via a
RTC driver so that the rtcwake utility can be used to configure it.
Signed-off-by: Marek Behún <kabel@kernel.org>
---
.../sysfs-bus-i2c-devices-turris-omnia-mcu | 16 ++
drivers/platform/cznic/Kconfig | 4 +
drivers/platform/cznic/Makefile | 1 +
.../platform/cznic/turris-omnia-mcu-base.c | 4 +
.../platform/cznic/turris-omnia-mcu-gpio.c | 2 -
.../cznic/turris-omnia-mcu-sys-off-wakeup.c | 245 ++++++++++++++++++
drivers/platform/cznic/turris-omnia-mcu.h | 19 ++
7 files changed, 289 insertions(+), 2 deletions(-)
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c
diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
index c0efdf1b3803..d668b8e999e6 100644
--- a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
@@ -14,6 +14,22 @@ Description: (RW) The front button on the Turris Omnia router can be
Format: %s.
+What: /sys/bus/i2c/devices/<mcu_device>/front_button_poweron
+Date: August 2023
+KernelVersion: 6.6
+Contact: Marek Behún <kabel@kernel.org>
+Description: (RW) Newer versions of the microcontroller firmware of the
+ Turris Omnia router support powering off the router into true
+ low power mode. The router can be powered on by pressing the
+ front button.
+
+ This file configures whether front button power on is enabled.
+
+ This file is present only if the power off feature is supported
+ by the firmware.
+
+ Format: %i.
+
What: /sys/bus/i2c/devices/<mcu_device>/fw_features
Date: August 2023
KernelVersion: 6.6
diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig
index 3a8c3edcd7e6..0a752aa654fa 100644
--- a/drivers/platform/cznic/Kconfig
+++ b/drivers/platform/cznic/Kconfig
@@ -19,10 +19,14 @@ config TURRIS_OMNIA_MCU
depends on I2C
select GPIOLIB
select GPIOLIB_IRQCHIP
+ select RTC_CLASS
help
Say Y here to add support for the features implemented by the
microcontroller on the CZ.NIC's Turris Omnia SOHO router.
The features include:
+ - board poweroff into true low power mode (with voltage regulators
+ disabled) and the ability to configure wake up from this mode (via
+ rtcwake)
- 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 a6177f5b4fff..6f1470d1f673 100644
--- a/drivers/platform/cznic/Makefile
+++ b/drivers/platform/cznic/Makefile
@@ -3,3 +3,4 @@
obj-$(CONFIG_TURRIS_OMNIA_MCU) += turris-omnia-mcu.o
turris-omnia-mcu-objs := turris-omnia-mcu-base.o
turris-omnia-mcu-objs += turris-omnia-mcu-gpio.o
+turris-omnia-mcu-objs += turris-omnia-mcu-sys-off-wakeup.o
diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c
index 2954151468e5..b507f81ebe0a 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-base.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
@@ -226,6 +226,10 @@ static int omnia_mcu_probe(struct i2c_client *client)
if (ret)
return ret;
+ ret = omnia_mcu_register_sys_off_and_wakeup(mcu);
+ if (ret)
+ return ret;
+
return 0;
}
diff --git a/drivers/platform/cznic/turris-omnia-mcu-gpio.c b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
index e8d5eeb4eb31..3d34a32bc7b0 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-gpio.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
@@ -5,8 +5,6 @@
* 2023 by Marek Behún <kabel@kernel.org>
*/
-#include <asm/unaligned.h>
-
#include "turris-omnia-mcu.h"
static const char * const omnia_mcu_gpio_names[64] = {
diff --git a/drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c b/drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c
new file mode 100644
index 000000000000..07ae92d819e3
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c
@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CZ.NIC's Turris Omnia MCU system off and RTC wakeup driver
+ *
+ * This is not a true RTC driver (in the sense that it does not provide a
+ * real-time clock), rather the MCU implements a wakeup from powered off state
+ * at a specified time relative to MCU boot, and we expose this feature via RTC
+ * alarm, so that it can be used via the rtcwake command, which is the standard
+ * Linux command for this.
+ *
+ * 2023 by Marek Behún <kabel@kernel.org>
+ */
+
+#include <linux/crc32.h>
+#include <linux/delay.h>
+#include <linux/reboot.h>
+
+#include "turris-omnia-mcu.h"
+
+static int omnia_get_uptime_wakeup(const struct i2c_client *client, u32 *uptime,
+ u32 *wakeup)
+{
+ u32 reply[2];
+ int err;
+
+ err = omnia_cmd_read(client, CMD_GET_UPTIME_AND_WAKEUP, reply,
+ sizeof(reply));
+ if (err)
+ return err;
+
+ if (uptime)
+ *uptime = le32_to_cpu(reply[0]);
+
+ if (wakeup)
+ *wakeup = le32_to_cpu(reply[1]);
+
+ return 0;
+}
+
+static int omnia_read_time(struct device *dev, struct rtc_time *tm)
+{
+ u32 uptime;
+ int err;
+
+ err = omnia_get_uptime_wakeup(to_i2c_client(dev), &uptime, NULL);
+ if (err)
+ return err;
+
+ rtc_time64_to_tm(uptime, tm);
+
+ return 0;
+}
+
+static int omnia_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct omnia_mcu *mcu = i2c_get_clientdata(client);
+ u32 wakeup;
+ int err;
+
+ err = omnia_get_uptime_wakeup(client, NULL, &wakeup);
+ if (err)
+ return err;
+
+ alrm->enabled = !!wakeup;
+ rtc_time64_to_tm(wakeup ?: mcu->rtc_alarm, &alrm->time);
+
+ return 0;
+}
+
+static int omnia_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct omnia_mcu *mcu = i2c_get_clientdata(client);
+
+ mcu->rtc_alarm = rtc_tm_to_time64(&alrm->time);
+
+ if (alrm->enabled)
+ return omnia_cmd_write_u32(client, CMD_SET_WAKEUP,
+ mcu->rtc_alarm);
+ else
+ return 0;
+}
+
+static int omnia_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct omnia_mcu *mcu = i2c_get_clientdata(client);
+
+ return omnia_cmd_write_u32(client, CMD_SET_WAKEUP,
+ enabled ? mcu->rtc_alarm : 0);
+}
+
+static const struct rtc_class_ops omnia_rtc_ops = {
+ .read_time = omnia_read_time,
+ .read_alarm = omnia_read_alarm,
+ .set_alarm = omnia_set_alarm,
+ .alarm_irq_enable = omnia_alarm_irq_enable,
+};
+
+static int omnia_power_off(struct sys_off_data *data)
+{
+ struct omnia_mcu *mcu = data->cb_data;
+ u8 cmd[9];
+ u32 tmp;
+ u16 arg;
+ int err;
+
+ if (mcu->front_button_poweron)
+ arg = CMD_POWER_OFF_POWERON_BUTTON;
+ else
+ arg = 0;
+
+ cmd[0] = CMD_POWER_OFF;
+ put_unaligned_le16(CMD_POWER_OFF_MAGIC, &cmd[1]);
+ put_unaligned_le16(arg, &cmd[3]);
+
+ tmp = cpu_to_be32(get_unaligned_le32(&cmd[1]));
+ put_unaligned_le32(crc32_be(0xffffffff, (void *)&tmp, sizeof(tmp)),
+ &cmd[5]);
+
+ err = omnia_cmd_write(mcu->client, cmd, sizeof(cmd));
+ if (err)
+ dev_err(&mcu->client->dev,
+ "Unable to send the poweroff command: %d\n", err);
+
+ return NOTIFY_DONE;
+}
+
+static int omnia_restart(struct sys_off_data *data)
+{
+ struct omnia_mcu *mcu = data->cb_data;
+ u8 cmd[3];
+ int err;
+
+ cmd[0] = CMD_GENERAL_CONTROL;
+
+ if (reboot_mode == REBOOT_HARD)
+ cmd[1] = cmd[2] = CTL_HARD_RST;
+ else
+ cmd[1] = cmd[2] = CTL_LIGHT_RST;
+
+ err = omnia_cmd_write(mcu->client, cmd, sizeof(cmd));
+ if (err)
+ dev_err(&mcu->client->dev,
+ "Unable to send the restart command: %d\n", err);
+
+ /*
+ * MCU needs a little bit to process the I2C command, otherwise it will
+ * do a light reset based on SOC SYSRES_OUT pin.
+ */
+ mdelay(1);
+
+ return NOTIFY_DONE;
+}
+
+static ssize_t front_button_poweron_show(struct device *dev,
+ struct device_attribute *a, char *buf)
+{
+ struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
+
+ return sysfs_emit(buf, "%d\n", mcu->front_button_poweron);
+}
+
+static ssize_t front_button_poweron_store(struct device *dev,
+ struct device_attribute *a,
+ const char *buf, size_t count)
+{
+ struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
+ bool val;
+
+ if (kstrtobool(buf, &val) < 0)
+ return -EINVAL;
+
+ mcu->front_button_poweron = val;
+
+ return count;
+}
+static DEVICE_ATTR_RW(front_button_poweron);
+
+static struct attribute *omnia_mcu_poweroff_attrs[] = {
+ &dev_attr_front_button_poweron.attr,
+ NULL,
+};
+
+static const struct attribute_group omnia_mcu_poweroff_group = {
+ .attrs = omnia_mcu_poweroff_attrs,
+};
+
+int omnia_mcu_register_sys_off_and_wakeup(struct omnia_mcu *mcu)
+{
+ struct device *dev = &mcu->client->dev;
+ int err;
+
+ /* MCU restart is always available */
+ err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART,
+ SYS_OFF_PRIO_FIRMWARE,
+ omnia_restart, mcu);
+ if (err) {
+ dev_err(dev, "Cannot register system restart handler: %d\n",
+ err);
+ return err;
+ }
+
+ /*
+ * poweroff and wakeup are available only if POWEROFF_WAKEUP feature is
+ * present
+ */
+ if (!(mcu->features & FEAT_POWEROFF_WAKEUP))
+ return 0;
+
+ err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF,
+ SYS_OFF_PRIO_FIRMWARE,
+ omnia_power_off, mcu);
+ if (err) {
+ dev_err(dev, "Cannot register system power off handler: %d\n",
+ err);
+ return err;
+ }
+
+ mcu->rtcdev = devm_rtc_allocate_device(dev);
+ if (IS_ERR(mcu->rtcdev)) {
+ err = PTR_ERR(mcu->rtcdev);
+ dev_err(dev, "Cannot allocate RTC device: %d\n", err);
+ return err;
+ }
+
+ mcu->rtcdev->ops = &omnia_rtc_ops;
+ mcu->rtcdev->range_max = U32_MAX;
+
+ err = devm_rtc_register_device(mcu->rtcdev);
+ if (err) {
+ dev_err(dev, "Cannot register RTC device: %d\n", err);
+ return err;
+ }
+
+ mcu->front_button_poweron = true;
+
+ err = devm_device_add_group(dev, &omnia_mcu_poweroff_group);
+ if (err)
+ dev_err(dev, "Cannot add poweroff sysfs attribute group: %d\n",
+ err);
+
+ return err;
+}
diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h
index b5802240aa8d..c40ac07be5f5 100644
--- a/drivers/platform/cznic/turris-omnia-mcu.h
+++ b/drivers/platform/cznic/turris-omnia-mcu.h
@@ -9,10 +9,12 @@
#define __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
#include <asm/byteorder.h>
+#include <asm/unaligned.h>
#include <linux/gpio/driver.h>
#include <linux/i2c.h>
#include <linux/log2.h>
#include <linux/mutex.h>
+#include <linux/rtc.h>
#include <linux/turris-omnia-mcu-interface.h>
#include <linux/workqueue.h>
@@ -29,6 +31,11 @@ struct omnia_mcu {
u16 last_status;
struct delayed_work button_release_emul_work;
bool button_pressed_emul;
+
+ /* RTC device for configuring wake-up */
+ struct rtc_device *rtcdev;
+ u32 rtc_alarm;
+ bool front_button_poweron;
};
static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
@@ -39,6 +46,17 @@ static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
return ret < 0 ? ret : 0;
}
+static inline int omnia_cmd_write_u32(const struct i2c_client *client, u8 cmd,
+ u32 val)
+{
+ u8 buf[5];
+
+ buf[0] = cmd;
+ put_unaligned_le32(val, &buf[1]);
+
+ return omnia_cmd_write(client, buf, sizeof(buf));
+}
+
static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
unsigned int len)
{
@@ -117,5 +135,6 @@ static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu);
void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu);
+int omnia_mcu_register_sys_off_and_wakeup(struct omnia_mcu *mcu);
#endif /* __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H */
--
2.41.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 5/7] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog
2023-09-19 10:38 [PATCH v2 0/7] Turris Omnia MCU driver Marek Behún
` (3 preceding siblings ...)
2023-09-19 10:38 ` [PATCH v2 4/7] platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup Marek Behún
@ 2023-09-19 10:38 ` Marek Behún
2023-09-19 13:50 ` Guenter Roeck
2023-09-19 10:38 ` [PATCH v2 6/7] ARM: dts: turris-omnia: Add MCU system-controller node Marek Behún
2023-09-19 10:38 ` [PATCH v2 7/7] ARM: dts: turris-omnia: Add GPIO key node for front button Marek Behún
6 siblings, 1 reply; 30+ messages in thread
From: Marek Behún @ 2023-09-19 10:38 UTC (permalink / raw)
To: Gregory CLEMENT, Arnd Bergmann, soc, arm, Wim Van Sebroeck,
Guenter Roeck, linux-watchdog
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 | 126 ++++++++++++++++++
drivers/platform/cznic/turris-omnia-mcu.h | 24 ++++
5 files changed, 157 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 6f1470d1f673..a43997a12d74 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-objs := turris-omnia-mcu-base.o
turris-omnia-mcu-objs += turris-omnia-mcu-gpio.o
turris-omnia-mcu-objs += turris-omnia-mcu-sys-off-wakeup.o
+turris-omnia-mcu-objs += 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 b507f81ebe0a..708265203cc1 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-base.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
@@ -230,6 +230,10 @@ static int omnia_mcu_probe(struct i2c_client *client)
if (ret)
return ret;
+ ret = omnia_mcu_register_watchdog(mcu);
+ if (ret)
+ return ret;
+
return 0;
}
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..2238b2167c45
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu-watchdog.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CZ.NIC's Turris Omnia MCU watchdog driver
+ *
+ * 2023 by Marek Behún <kabel@kernel.org>
+ */
+
+#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);
+
+ /*
+ * The MCU also implements LED controller, and the LED driver may in
+ * some cases require more I2C bandwidth. In order to prevent stealing
+ * that bandwidth from the LED driver, configure minimum time between
+ * pings to 10% of the configured timeout (for 120 second timeout we
+ * will be pinging at most every 12 seconds).
+ */
+ mcu->wdt.min_hw_heartbeat_ms = 100 * mcu->wdt.timeout;
+
+ 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) {
+ dev_err(dev, "Cannot get MCU watchdog state: %d\n", ret);
+ return ret;
+ }
+
+ 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)
+ dev_err(dev, "Cannot register MCU watchdog: %d\n", ret);
+
+ return ret;
+}
diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h
index c40ac07be5f5..89fe39f4255f 100644
--- a/drivers/platform/cznic/turris-omnia-mcu.h
+++ b/drivers/platform/cznic/turris-omnia-mcu.h
@@ -16,6 +16,7 @@
#include <linux/mutex.h>
#include <linux/rtc.h>
#include <linux/turris-omnia-mcu-interface.h>
+#include <linux/watchdog.h>
#include <linux/workqueue.h>
struct omnia_mcu {
@@ -36,6 +37,9 @@ struct omnia_mcu {
struct rtc_device *rtcdev;
u32 rtc_alarm;
bool front_button_poweron;
+
+ /* MCU watchdog */
+ struct watchdog_device wdt;
};
static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
@@ -46,6 +50,25 @@ static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
return ret < 0 ? ret : 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)
{
@@ -136,5 +159,6 @@ static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu);
void omnia_mcu_unregister_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 /* __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H */
--
2.41.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 6/7] ARM: dts: turris-omnia: Add MCU system-controller node
2023-09-19 10:38 [PATCH v2 0/7] Turris Omnia MCU driver Marek Behún
` (4 preceding siblings ...)
2023-09-19 10:38 ` [PATCH v2 5/7] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog Marek Behún
@ 2023-09-19 10:38 ` Marek Behún
2023-09-19 10:38 ` [PATCH v2 7/7] ARM: dts: turris-omnia: Add GPIO key node for front button Marek Behún
6 siblings, 0 replies; 30+ messages in thread
From: Marek Behún @ 2023-09-19 10:38 UTC (permalink / raw)
To: Gregory CLEMENT, Arnd Bergmann, soc, arm; +Cc: Marek Behún
Turris Omnia's MCU provides various features that can be configured over
I2C at address 0x2a. Add device-tree node.
Fixes: 26ca8b52d6e1 ("ARM: dts: add support for Turris Omnia")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
.../dts/marvell/armada-385-turris-omnia.dts | 22 ++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts b/arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts
index 2d8d319bec83..7682fda4000a 100644
--- a/arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts
+++ b/arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts
@@ -218,7 +218,22 @@ i2c@0 {
#size-cells = <0>;
reg = <0>;
- /* STM32F0 command interface at address 0x2a */
+ mcu: system-controller@2a {
+ compatible = "cznic,turris-omnia-mcu";
+ reg = <0x2a>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&mcu_pins>;
+
+ interrupt-parent = <&gpio1>;
+ interrupts = <11 IRQ_TYPE_NONE>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ };
led-controller@2b {
compatible = "cznic,turris-omnia-leds";
@@ -503,6 +518,11 @@ fixed-link {
};
&pinctrl {
+ mcu_pins: mcu-pins {
+ marvell,pins = "mpp43";
+ marvell,function = "gpio";
+ };
+
pcawan_pins: pcawan-pins {
marvell,pins = "mpp46";
marvell,function = "gpio";
--
2.41.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 7/7] ARM: dts: turris-omnia: Add GPIO key node for front button
2023-09-19 10:38 [PATCH v2 0/7] Turris Omnia MCU driver Marek Behún
` (5 preceding siblings ...)
2023-09-19 10:38 ` [PATCH v2 6/7] ARM: dts: turris-omnia: Add MCU system-controller node Marek Behún
@ 2023-09-19 10:38 ` Marek Behún
6 siblings, 0 replies; 30+ messages in thread
From: Marek Behún @ 2023-09-19 10:38 UTC (permalink / raw)
To: Gregory CLEMENT, Arnd Bergmann, soc, arm; +Cc: Marek Behún
Now that we have the MCU device-tree node, which acts as a GPIO
controller, add GPIO key node for the front button.
Fixes: 26ca8b52d6e1 ("ARM: dts: add support for Turris Omnia")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
.../boot/dts/marvell/armada-385-turris-omnia.dts | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts b/arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts
index 7682fda4000a..016adfa0b470 100644
--- a/arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts
+++ b/arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts
@@ -112,6 +112,19 @@ sfp: sfp {
status = "disabled";
};
+ gpio-keys {
+ compatible = "gpio-keys";
+
+ front-button {
+ label = "Front Button";
+ linux,code = <KEY_VENDOR>;
+ linux,can-disable;
+ gpios = <&mcu 12 GPIO_ACTIVE_HIGH>;
+ /* debouncing is done by the microcontroller */
+ debounce-interval = <0>;
+ };
+ };
+
sound {
compatible = "simple-audio-card";
simple-audio-card,name = "SPDIF";
--
2.41.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU
2023-09-19 10:38 ` [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU Marek Behún
@ 2023-09-19 12:29 ` Andy Shevchenko
2023-09-19 15:16 ` Marek Behún
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-09-19 12:29 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
linux-watchdog
On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:
> Add a new platform driver that exposes the features provided by the
> microcontroller on the Turris Omnia board.
> This commit adds the basic skeleton for the driver.
Read "Submitting Patches" documentation (find "This patch" in it) and amend
your commit message accordingly.
...
> +Date: August 2023
> +KernelVersion: 6.6
Wrong and outdated. Use phb-crystall-ball to find the closest possible values
for both parameters here.
Ditto for all others with the same issue.
Note, it likely might be part of v6.7, not earlier.
...
> @@ -11,3 +11,4 @@ obj-$(CONFIG_OLPC_EC) += olpc/
> obj-$(CONFIG_GOLDFISH) += goldfish/
> obj-$(CONFIG_CHROME_PLATFORMS) += chrome/
> obj-$(CONFIG_SURFACE_PLATFORMS) += surface/
> +obj-$(CONFIG_CZNIC_PLATFORMS) += cznic/
Why not ordered (to some extent) here (as you did in the other file)?
...
> +turris-omnia-mcu-objs := turris-omnia-mcu-base.o
objs is for user space tools. Kernel code should use m,y.
...
> +#include <linux/hex.h>
> +#include <linux/module.h>
Missing types.h, sysfs.h, mod_devicetable.h, i2c.h, ...
...
> +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> + u8 *version)
> +{
> + u8 reply[20];
> + int ret;
> +
> + ret = omnia_cmd_read(mcu->client, bootloader ? CMD_GET_FW_VERSION_BOOT :
> + CMD_GET_FW_VERSION_APP,
> + reply, sizeof(reply));
> + if (ret < 0)
Can it return positive value? What would be the meaning of it?
> + return ret;
> + version[40] = '\0';
How do you know the version has enough space?
> + bin2hex(version, reply, 20);
> +
> + return 0;
> +}
> +
> +static ssize_t fw_version_hash_show(struct device *dev, char *buf,
> + bool bootloader)
> +{
> + struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
> + u8 version[41];
> + int err;
In two near functions you already inconsistent in the naming of the return code
variable. Be consistent across all your code, i.e. choose one name and use it
everywhere.
> + err = omnia_get_version_hash(mcu, bootloader, version);
> + if (err)
> + return err;
> +
> + return sysfs_emit(buf, "%s\n", version);
> +}
...
> + return sysfs_emit(buf, "%#x\n", mcu->features);
"0" will be printed differently, is this on purpose?
...
> + int ret = omnia_cmd_read_u8(to_i2c_client(dev), CMD_GET_RESET);
> +
> + if (ret < 0)
> + return ret;
Better from maintenance perspective to have
int ret;
ret = omnia_cmd_read_u8(to_i2c_client(dev), CMD_GET_RESET);
if (ret < 0)
return ret;
...
> +static struct attribute *omnia_mcu_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,
> + NULL,
No comma for the terminator line. It will make your code robust at compile time
against some misplaced entries.
> +};
> +
Unneeded blank line.
> +ATTRIBUTE_GROUPS(omnia_mcu);
...
> + u8 version[41];
This magic sizes should go away. Use predefined constant, or allocate on the
heap, depending on the case(s) you have.
...
> + int status;
My gosh, it's a _third_ name for the same!
> + status = omnia_cmd_read_u16(mcu->client, CMD_GET_STATUS_WORD);
> + if (status < 0)
> + return status;
...
> + for (int i = 0; i < ARRAY_SIZE(features); ++i) {
Why signed?
Why pre-increment?
> + if (!(mcu->features & features[i].mask)) {
Wouldn't be better
if (mcu->features & features[i].mask)
continue;
?
> + omnia_info_missing_feature(dev, features[i].name);
> + suggest_fw_upgrade = true;
> + }
> + }
...
> + dev_info(dev,
> + "Consider upgrading MCU firmware with the omnia-mcutool utility.\n");
You have so-o many dev_info() calls, are you sure you not abusing use of that?
...
> + if (ret) {
> + dev_err(dev, "Cannot determine MCU supported features: %d\n",
> + ret);
> + return ret;
return dev_err_probe(...);
> + }
...
> + if (!client->irq) {
> + dev_err(dev, "IRQ resource not found\n");
> + return -ENODATA;
> + }
Why you need to allocate memory, go through the initial checks, etc and then
fail? What's wrong with checking this first?
Why -ENODATA?!
...
> +static const struct of_device_id of_omnia_mcu_match[] = {
> + { .compatible = "cznic,turris-omnia-mcu", },
Inner comma is not needed.
> + {},
No comma for the terminator entry.
> +};
...
> +static const struct i2c_device_id omnia_mcu_id[] = {
> + { "turris-omnia-mcu", 0 },
', 0' part is redundant.
> + { }
> +};
...
> +static struct i2c_driver omnia_mcu_driver = {
> + .probe = omnia_mcu_probe,
> + .id_table = omnia_mcu_id,
> + .driver = {
> + .name = "turris-omnia-mcu",
> + .of_match_table = of_omnia_mcu_match,
> + .dev_groups = omnia_mcu_groups,
> + },
> +};
> +
Redundant blank line.
> +module_i2c_driver(omnia_mcu_driver);
...
> +#ifndef __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
> +#define __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
WE_LIKE_VERY_LONG_AND_OVERFLOWED_DEFINITIONS_H!
...
> +#include <asm/byteorder.h>
This usually goes after linux/*.h.
> +#include <linux/i2c.h>
Missing types.h, errno.h, ...
+ blank line?
> +#include <linux/turris-omnia-mcu-interface.h>
...
> + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> + if (likely(ret == ARRAY_SIZE(msgs)))
Why likely()? Please, justify.
> + return len;
> + else if (ret < 0)
> + return ret;
> + else
> + return -EIO;
In both cases the 'else' is redundant. Moreover, the usual pattern is to check
for the errors first.
> +}
...
> +#ifndef __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H
> +#define __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H
My gosh!
...
> +#include <linux/bits.h>
> +enum commands_e {
Are you sure the name is unique enough / properly namespaced?
Same Q to all enums.
...
> + /*CTL_RESERVED = BIT(2),*/
Missing spaces?
Also, needs a comment why this is commented out.
...
> + CTL_BOOTLOADER = BIT(7)
Add trailing comma.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-19 10:38 ` [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs Marek Behún
@ 2023-09-19 13:00 ` Andy Shevchenko
2023-09-20 17:08 ` Marek Behún
2023-09-21 18:42 ` Marek Behún
2023-09-20 11:58 ` Linus Walleij
1 sibling, 2 replies; 30+ messages in thread
From: Andy Shevchenko @ 2023-09-19 13:00 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio
On Tue, Sep 19, 2023 at 12:38:11PM +0200, Marek Behún wrote:
> Add support for GPIOs connected to the MCU on the Turris Omnia board.
>
> This include:
includes
> - front button pin
> - enable pins for USB regulators
> - MiniPCIe / mSATA card presence pins in MiniPCIe port 0
> - LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports
> - on board revisions 32+ also various peripheral resets and another
> voltage regulator enable pin
Can the main driver provide a regmap and all other use it?
...
> +Date: August 2023
> +KernelVersion: 6.6
Same as per previous patch review.
...
> + ret = omnia_mcu_register_gpiochip(mcu);
> + if (ret)
> + return ret;
> +
> return 0;
return ..._gpiochip(...);
?
...
> + switch (cmd) {
> + case CMD_GENERAL_CONTROL:
> + buf[1] = val;
> + buf[2] = mask;
> + len = 3;
> + break;
> +
> + case CMD_EXT_CONTROL:
> + put_unaligned_le16(val, &buf[1]);
> + put_unaligned_le16(mask, &buf[3]);
> + len = 5;
> + break;
> +
> + default:
> + unreachable();
You meant BUG_ON()?
> + }
...
> +static int omnia_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct omnia_mcu *mcu = gpiochip_get_data(gc);
> +
> + if (offset == OMNIA_GPIO_PHY_SFP_OFFSET) {
> + int val;
> +
> + mutex_lock(&mcu->lock);
> + val = omnia_cmd_read_bit(mcu->client,
> + CMD_GET_EXT_CONTROL_STATUS,
> + EXT_CTL_PHY_SFP_AUTO);
> + mutex_unlock(&mcu->lock);
> +
> + if (val < 0)
> + return val;
> +
> + if (val)
> + return GPIO_LINE_DIRECTION_IN;
> + else
Redundant 'else'.
> + return GPIO_LINE_DIRECTION_OUT;
> + }
> +
> + if (omnia_gpios[offset].ctl_cmd)
> + return GPIO_LINE_DIRECTION_OUT;
> + else
Ditto.
> + return GPIO_LINE_DIRECTION_IN;
> +}
...
> + if (offset == OMNIA_GPIO_PHY_SFP_OFFSET)
> + return omnia_ctl_cmd(mcu, CMD_EXT_CONTROL, EXT_CTL_PHY_SFP_AUTO,
> + EXT_CTL_PHY_SFP_AUTO);
> +
> + if (gpio->ctl_cmd)
> + return -EOPNOTSUPP;
I believe internally we use ENOTSUPP.
Ditto for all cases like this.
...
> + uint16_t val, mask;
So, why uintXX_t out of a sudden?
...
> + if (gpio->ctl_cmd)
> + return omnia_ctl_cmd(mcu, gpio->ctl_cmd, val, mask);
> + else
Redundant 'else'.
> + return -EOPNOTSUPP;
> +}
...
> +static void omnia_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> + const struct omnia_gpio *gpio = &omnia_gpios[offset];
> + struct omnia_mcu *mcu = gpiochip_get_data(gc);
> + u16 val, mask;
> + int err = 0;
Redundant assignment.
> + if (!gpio->ctl_cmd)
> + return;
> +
> + mask = gpio->ctl_bit;
> + val = value ? mask : 0;
> +
> + err = omnia_ctl_cmd(mcu, gpio->ctl_cmd, val, mask);
> + if (err)
> + dev_err(&mcu->client->dev, "Cannot set GPIO %u: %d\n",
> + offset, err);
> +}
...
> + mutex_lock(&mcu->lock);
> +
> + if (ctl_mask)
> + err = omnia_ctl_cmd_unlocked(mcu, CMD_GENERAL_CONTROL, ctl,
> + ctl_mask);
> + if (!err && ext_ctl_mask)
> + err = omnia_ctl_cmd_unlocked(mcu, CMD_EXT_CONTROL, ext_ctl,
> + ext_ctl_mask);
Can it be
if (err)
goto out_unlock;
if (_mask)
...
?
> + mutex_unlock(&mcu->lock);
> + if (err)
> + dev_err(&mcu->client->dev, "Cannot set GPIOs: %d\n", err);
How is useful this one?
...
> +static bool omnia_gpio_available(struct omnia_mcu *mcu,
> + const struct omnia_gpio *gpio)
> +{
> + if (gpio->feat_mask)
> + return (mcu->features & gpio->feat_mask) == gpio->feat;
> + else if (gpio->feat)
> + return mcu->features & gpio->feat;
> + else
Redundant 'else':s.
> + return true;
> +}
...
> +static int omnia_gpio_init_valid_mask(struct gpio_chip *gc,
> + unsigned long *valid_mask,
> + unsigned int ngpios)
> +{
> + struct omnia_mcu *mcu = gpiochip_get_data(gc);
> + bitmap_zero(valid_mask, ngpios);
No need.
Also do you have bitops.h included?
> + for (int i = 0; i < ngpios; ++i) {
> + const struct omnia_gpio *gpio = &omnia_gpios[i];
> +
> + if (!gpio->cmd && !gpio->int_bit)
> + continue;
Use clear_bit() here...
> + if (omnia_gpio_available(mcu, gpio))
> + set_bit(i, valid_mask);
...and assign_bit() here.
> + }
> +
> + return 0;
> +}
...
> + dev_dbg(&mcu->client->dev,
> + "set int mask %02x %02x %02x %02x %02x %02x %02x %02x\n",
> + cmd[1], cmd[2], cmd[3], cmd[4], cmd[5], cmd[6], cmd[7], cmd[8]);
%8ph
...
> + /*
> + * Remember which GPIOs have both rising and falling interrupts enabled.
> + * For those we will cache their value so that .get() method is faster.
> + * We also need to forget cached values of GPIOs that aren't cached
> + * anymore.
> + */
> + if (!err) {
if (err)
goto out_unlock;
> + mcu->both = rising & falling;
> + mcu->is_cached &= mcu->both;
> + }
> +
> + mutex_unlock(&mcu->lock);
...
> +static void omnia_irq_init_valid_mask(struct gpio_chip *gc,
> + unsigned long *valid_mask,
> + unsigned int ngpios)
> +{
> + struct omnia_mcu *mcu = gpiochip_get_data(gc);
> + bitmap_zero(valid_mask, ngpios);
No need (see above how).
> + for (int i = 0; i < ngpios; ++i) {
> + const struct omnia_gpio *gpio = &omnia_gpios[i];
> +
> + if (!gpio->int_bit)
> + continue;
> +
> + if (omnia_gpio_available(mcu, gpio))
> + set_bit(i, valid_mask);
> + }
> +}
...
> + u8 cmd[9] = {};
Magic number in a few places. Please, use self-explanatory pre-defined constant
instead.
...
> + dev_err(&mcu->client->dev, "Cannot read pending IRQs: %d\n",
> + err);
In all functions with help of
struct device *dev = &mcu->client->dev;
you can make code shorter.
...
> + rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
> + (reply[6] << 24);
> + falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
> + (reply[7] << 24);
With a help of two masks, you can access to the both edges as to 64-bit value
and simplify the code.
...
> + int ret = omnia_cmd_read_u16(mcu->client, CMD_GET_STATUS_WORD);
Please, split this for better maintenance.
> + if (ret < 0)
> + return ret;
...
> +static irqreturn_t omnia_irq_thread_handler(int irq, void *dev_id)
> +{
> + struct omnia_mcu *mcu = dev_id;
> + irqreturn_t res = IRQ_NONE;
> + unsigned long pending;
> + int i;
> +
> + if (!omnia_irq_read_pending(mcu, &pending))
> + return IRQ_NONE;
> +
> + for_each_set_bit(i, &pending, 32) {
> + unsigned int nested_irq =
> + irq_find_mapping(mcu->gc.irq.domain,
> + omnia_int_to_gpio_idx[i]);
It's much better to read in a form
unsigned int nested_irq;
domain = ...
nested_irq = irq_find_mapping(domain, omnia_int_to_gpio_idx[i]);
(exactly 80 characters, btw).
> + handle_nested_irq(nested_irq);
> + res = IRQ_HANDLED;
No need.
> + }
> + return res;
return IRQ_RETVAL(pending);
> +}
...
> +static ssize_t front_button_mode_show(struct device *dev,
> + struct device_attribute *a, char *buf)
> +{
> + struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
> + int val;
> +
> + if (mcu->features & FEAT_NEW_INT_API)
> + val = omnia_cmd_read_bit(mcu->client, CMD_GET_STATUS_WORD,
> + STS_BUTTON_MODE);
> + else
> + val = !!(mcu->last_status & STS_BUTTON_MODE);
> + if (val < 0)
Can be true only in one case, why to check for the second oner?/
> + return val;
> + return sysfs_emit(buf, "%s\n", val ? "cpu" : "mcu");
With a static const array of string literals...
> +}
...
> + if (sysfs_streq(buf, "mcu"))
> + val = 0;
> + else if (sysfs_streq(buf, "cpu"))
> + val = mask;
> + else
> + return -EINVAL;
...and help of sysfs_match_string() you can simplify the code.
...
> +static struct attribute *omnia_mcu_gpio_attrs[] = {
> + &dev_attr_front_button_mode.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group omnia_mcu_gpio_group = {
> + .attrs = omnia_mcu_gpio_attrs,
> +};
ATTRIBUTE_GROUPS() ?
...
> + err = devm_gpiochip_add_data(dev, &mcu->gc, mcu);
> + if (err) {
> + dev_err(dev, "Cannot add GPIO chip: %d\n", err);
> + return err;
return dev_err_probe(...);
Ditto for other places like this in the probe stage.
> + }
...
> + err = devm_device_add_group(dev, &omnia_mcu_gpio_group);
No way, no-one should use the API scheduled for removal.
What's wrong with .dev_groups ?
...
> +void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu)
> +{
> + if (!(mcu->features & FEAT_NEW_INT_API))
> + cancel_delayed_work_sync(&mcu->button_release_emul_work);
> +
> + mutex_destroy(&mcu->lock);
Wrong order?
> +}
...
> struct omnia_mcu {
> struct i2c_client *client;
> const char *type;
> u16 features;
> +
> + /* GPIO chip */
> + struct gpio_chip gc;
Making this a first member may lead to the better code. Check with bloat-o-meter.
> + struct mutex lock;
> + u32 mask, rising, falling, both, cached, is_cached;
> + /* Old MCU firmware handling needs the following */
> + u16 last_status;
> + struct delayed_work button_release_emul_work;
Swapping these two may free a few bytes. Check with pahole tool.
> + bool button_pressed_emul;
> };
...
> + if (!bits) {
> + *reply = 0;
> + return 0;
> + }
> +
> + ret = omnia_cmd_read(client, cmd, reply, (ilog2(bits) >> 3) + 1);
> +
Redundant blank line.
> + if (ret >= 0)
> + *reply = le32_to_cpu(*reply) & bits;
Huh?! Please, check your code with sparse like
make W=1 C=1 CF=-D__CHECK_ENDIAN__ ...
> + return ret < 0 ? ret : 0;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 5/7] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog
2023-09-19 10:38 ` [PATCH v2 5/7] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog Marek Behún
@ 2023-09-19 13:50 ` Guenter Roeck
2023-09-22 11:46 ` Marek Behún
0 siblings, 1 reply; 30+ messages in thread
From: Guenter Roeck @ 2023-09-19 13:50 UTC (permalink / raw)
To: Marek Behún, Gregory CLEMENT, Arnd Bergmann, soc, arm,
Wim Van Sebroeck, linux-watchdog
On 9/19/23 03:38, Marek Behún wrote:
> 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 | 126 ++++++++++++++++++
> drivers/platform/cznic/turris-omnia-mcu.h | 24 ++++
> 5 files changed, 157 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 6f1470d1f673..a43997a12d74 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-objs := turris-omnia-mcu-base.o
> turris-omnia-mcu-objs += turris-omnia-mcu-gpio.o
> turris-omnia-mcu-objs += turris-omnia-mcu-sys-off-wakeup.o
> +turris-omnia-mcu-objs += 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 b507f81ebe0a..708265203cc1 100644
> --- a/drivers/platform/cznic/turris-omnia-mcu-base.c
> +++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
> @@ -230,6 +230,10 @@ static int omnia_mcu_probe(struct i2c_client *client)
> if (ret)
> return ret;
>
> + ret = omnia_mcu_register_watchdog(mcu);
> + if (ret)
> + return ret;
> +
> return 0;
> }
>
> 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..2238b2167c45
> --- /dev/null
> +++ b/drivers/platform/cznic/turris-omnia-mcu-watchdog.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CZ.NIC's Turris Omnia MCU watchdog driver
> + *
> + * 2023 by Marek Behún <kabel@kernel.org>
> + */
> +
> +#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);
> +
> + /*
> + * The MCU also implements LED controller, and the LED driver may in
> + * some cases require more I2C bandwidth. In order to prevent stealing
> + * that bandwidth from the LED driver, configure minimum time between
> + * pings to 10% of the configured timeout (for 120 second timeout we
> + * will be pinging at most every 12 seconds).
> + */
> + mcu->wdt.min_hw_heartbeat_ms = 100 * mcu->wdt.timeout;
This is not what min_hw_heartbeat_ms is supposed to be used for, and it is
an abuse of the API which is not guaranteed to work. The value is exopected
to be set at registration time, and it is not supposed to change dynamically.
It is also competely unnecessary since no sane watchdog daemon will ping
at such a high rate. Please drop.
Guenter
> +
> + 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) {
> + dev_err(dev, "Cannot get MCU watchdog state: %d\n", ret);
> + return ret;
> + }
> +
> + 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)
> + dev_err(dev, "Cannot register MCU watchdog: %d\n", ret);
> +
> + return ret;
> +}
> diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h
> index c40ac07be5f5..89fe39f4255f 100644
> --- a/drivers/platform/cznic/turris-omnia-mcu.h
> +++ b/drivers/platform/cznic/turris-omnia-mcu.h
> @@ -16,6 +16,7 @@
> #include <linux/mutex.h>
> #include <linux/rtc.h>
> #include <linux/turris-omnia-mcu-interface.h>
> +#include <linux/watchdog.h>
> #include <linux/workqueue.h>
>
> struct omnia_mcu {
> @@ -36,6 +37,9 @@ struct omnia_mcu {
> struct rtc_device *rtcdev;
> u32 rtc_alarm;
> bool front_button_poweron;
> +
> + /* MCU watchdog */
> + struct watchdog_device wdt;
> };
>
> static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
> @@ -46,6 +50,25 @@ static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
> return ret < 0 ? ret : 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)
> {
> @@ -136,5 +159,6 @@ static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
> int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu);
> void omnia_mcu_unregister_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 /* __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H */
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU
2023-09-19 12:29 ` Andy Shevchenko
@ 2023-09-19 15:16 ` Marek Behún
2023-09-19 18:27 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Marek Behún @ 2023-09-19 15:16 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
linux-watchdog
Hello Andy,
thank you for your review. I have some questions regarding your
comments.
On Tue, 19 Sep 2023 15:29:08 +0300
Andy Shevchenko <andy@kernel.org> wrote:
> On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:
> > Add a new platform driver that exposes the features provided by the
> > microcontroller on the Turris Omnia board.
>
> > This commit adds the basic skeleton for the driver.
>
> Read "Submitting Patches" documentation (find "This patch" in it) and amend
> your commit message accordingly.
OK.
> > +Date: August 2023
> > +KernelVersion: 6.6
>
> Wrong and outdated. Use phb-crystall-ball to find the closest possible values
> for both parameters here.
>
> Ditto for all others with the same issue.
>
> Note, it likely might be part of v6.7, not earlier.
OK
> > obj-$(CONFIG_GOLDFISH) += goldfish/
> > obj-$(CONFIG_CHROME_PLATFORMS) += chrome/
> > obj-$(CONFIG_SURFACE_PLATFORMS) += surface/
> > +obj-$(CONFIG_CZNIC_PLATFORMS) += cznic/
>
> Why not ordered (to some extent) here (as you did in the other file)?
Where should I put it? The other entries are not ordered, see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/Makefile?h=v6.6-rc2
> > +turris-omnia-mcu-objs := turris-omnia-mcu-base.o
>
> objs is for user space tools. Kernel code should use m,y.
I don't understand. There are plenty driver using -objs. The driver
consists of multiple C source files. I thought this should be done as
obj-$(CONFIG_XXX) += xxx.o
xxx-objs := xxx-a.o xxx-b.o xxx-c.o
If you choose CONFIG_XXX = y, then the driver will be compiled into
kernel, if you choose CONFIG_XXX = m, then the driver will be a module
xxx.ko.
> > +#include <linux/hex.h>
> > +#include <linux/module.h>
>
> Missing types.h, sysfs.h, mod_devicetable.h, i2c.h, ...
i2c.h comes from turris-omnia-mcu.h. The others I will fix. Does the
the .c file also include all those headers if it includes a local
header already?
> > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool
> > bootloader,
> > + u8 *version)
> > +{
> > + u8 reply[20];
> > + int ret;
> > +
> > + ret = omnia_cmd_read(mcu->client, bootloader ?
> > CMD_GET_FW_VERSION_BOOT :
> > +n average development ti
> > CMD_GET_FW_VERSION_APP,
> > + reply, sizeof(reply));
>
> > + if (ret < 0)
>
> Can it return positive value? What would be the meaning of it?
Originally I had it return 0 on success, but recently I've had a
discussion about this same thing with LED subsystem maintainer about
some LED patches, Lee Jones, who requested the _read function to return
number of bytes read on success. See at
https://lore.kernel.org/linux-leds/20230821122644.GJ1380343@google.com/
where he says:
Read and write APIs, even abstracted ones such as these generally
return the number of bytes successfully read and written respectively.
If you are going to normalise, then please do so against this
standard.
I do not agree with him, since this is a local command accessor which
only succeeds if all the bytes are read/written as requested. But I
adjusted my code as per his request since he is the maintainer.
I will change the code back so that it returns 0 on success instead of
the number of bytes written.
> > + return ret;
>
> > + version[40] = '\0';
>
> How do you know the version has enough space?
The function is only use within the file, and both users know how much
space it needs. But ok, I shall enforce it with the static keyword as
static int
omnia_get_version_hash(...,
u8 version[static 2*OMNIA_FW_VERSION_LEN + 1]);
Would that be okay?
> > + bin2hex(version, reply, 20);
> > +
> > + return 0;
> > +}
> > +
> > +static ssize_t fw_version_hash_show(struct device *dev, char *buf,
> > + bool bootloader)
> > +{
> > + struct omnia_mcu *mcu =
> > i2c_get_clientdata(to_i2c_client(dev));
> > + u8 version[41];
>
> > + int err;
>
> In two near functions you already inconsistent in the naming of the
> return code variable. Be consistent across all your code, i.e. choose
> one name and use it everywhere.
I use
ret - when the return value can also be positive and mean something
err - when the return value is either 0 (no error) or negative errno
Is this inconsistent?
> > + err = omnia_get_version_hash(mcu, bootloader, version);
> > + if (err)
> > + return err;
> > +
> > + return sysfs_emit(buf, "%s\n", version);
> > +}
>
> ...
>
> > + return sysfs_emit(buf, "%#x\n", mcu->features);
>
> "0" will be printed differently, is this on purpose?
No, I wanted it to print 0x0, didn't know '#' would print just 0.
Thanks.
> > + int ret = omnia_cmd_read_u8(to_i2c_client(dev),
> > CMD_GET_RESET); +
> > + if (ret < 0)
> > + return ret;
>
> Better from maintenance perspective to have
>
> int ret;
>
> ret = omnia_cmd_read_u8(to_i2c_client(dev), CMD_GET_RESET);
> if (ret < 0)
> return ret;
OK
> > +static struct attribute *omnia_mcu_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,
>
> > + NULL,
>
> No comma for the terminator line. It will make your code robust at
> compile time against some misplaced entries.
OK
> > +};
>
> > +
>
> Unneeded blank line.
OK
> > +ATTRIBUTE_GROUPS(omnia_mcu);
>
> ...
>
> > + u8 version[41];
>
> This magic sizes should go away. Use predefined constant, or allocate
> on the heap, depending on the case(s) you have.
OK
> > + int status;
>
> My gosh, it's a _third_ name for the same!
The variable holds value of the status register, which is needed at
another point in this function. I thought naming it just "ret" would
cause confusions... Do you really propose to rename it such?
> > + status = omnia_cmd_read_u16(mcu->client,
> > CMD_GET_STATUS_WORD);
> > + if (status < 0)
> > + return status;
>
> ...
>
> > + for (int i = 0; i < ARRAY_SIZE(features); ++i) {
>
> Why signed?
Because it is shorter and makes no difference and there are tons of
for (int i = 0; i < ARRAY_SIZE(x); i++)
in the kernel already. But if you really want, I can change it. Please
let me know.
> Why pre-increment?
Again, it makes no difference, is widely used in the kernel and I
personally prefer it. But I will change it if you want.
>
> > + if (!(mcu->features & features[i].mask)) {
>
> Wouldn't be better
>
> if (mcu->features & features[i].mask)
> continue;
>
> ?
OK
> > + omnia_info_missing_feature(dev,
> > features[i].name);
> > + suggest_fw_upgrade = true;
> > + }
> > + }
>
> ...
>
> > + dev_info(dev,
> > + "Consider upgrading MCU firmware with the
> > omnia-mcutool utility.\n");
>
> You have so-o many dev_info() calls, are you sure you not abusing use
> of that?
I want the users to upgrade the MCU firmware. These infos are only
printed during probe time, and won't be printed at all if the firmware
is upgraded. Is this a problem?
>
> ...
>
> > + if (ret) {
> > + dev_err(dev, "Cannot determine MCU supported
> > features: %d\n",
> > + ret);
> > + return ret;
>
> return dev_err_probe(...);
Thx. Didn't know this one.
> > + }
>
> ...
>
> > + if (!client->irq) {
> > + dev_err(dev, "IRQ resource not found\n");
> > + return -ENODATA;
> > + }
>
> Why you need to allocate memory, go through the initial checks, etc
> and then fail? What's wrong with checking this first?
>
> Why -ENODATA?!
OK
> > +static const struct of_device_id of_omnia_mcu_match[] = {
> > + { .compatible = "cznic,turris-omnia-mcu", },
>
> Inner comma is not needed.
OK
> > + {},
>
> No comma for the terminator entry.
OK
> > +};
>
> ...
>
> > +static const struct i2c_device_id omnia_mcu_id[] = {
> > + { "turris-omnia-mcu", 0 },
>
> ', 0' part is redundant.
OK
> > + { }
> > +};
>
> ...
>
> > +static struct i2c_driver omnia_mcu_driver = {
> > + .probe = omnia_mcu_probe,
> > + .id_table = omnia_mcu_id,
> > + .driver = {
> > + .name = "turris-omnia-mcu",
> > + .of_match_table = of_omnia_mcu_match,
> > + .dev_groups = omnia_mcu_groups,
> > + },
> > +};
>
> > +
>
> Redundant blank line.
OK
> > +module_i2c_driver(omnia_mcu_driver);
>
> ...
>
> > +#ifndef __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
> > +#define __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
>
> WE_LIKE_VERY_LONG_AND_OVERFLOWED_DEFINITIONS_H!
Sigh... OK, I will change it :)
> > +#include <asm/byteorder.h>
>
> This usually goes after linux/*.h.
OK
>
> > +#include <linux/i2c.h>
>
> Missing types.h, errno.h, ...
>
> + blank line?
Will change.
> > +#include <linux/turris-omnia-mcu-interface.h>
>
> ...
>
> > + ret = i2c_transfer(client->adapter, msgs,
> > ARRAY_SIZE(msgs));
> > + if (likely(ret == ARRAY_SIZE(msgs)))
>
> Why likely()? Please, justify.
Becuase it is unlikely the I2C transaction will fail. In most cases, it
does not.
> > + return len;
>
> > + else if (ret < 0)
> > + return ret;
> > + else
> > + return -EIO;
>
> In both cases the 'else' is redundant. Moreover, the usual pattern is
> to check for the errors first.
>
> > +}
>
> ...
>
> > +#ifndef __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H
> > +#define __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H
>
> My gosh!
:)
> > +#include <linux/bits.h>
>
> > +enum commands_e {
>
> Are you sure the name is unique enough / properly namespaced?
> Same Q to all enums.
I can change it. I wanted it to be compatible with the header in the
firmware repository.
>
> ...
>
> > + /*CTL_RESERVED = BIT(2),*/
>
> Missing spaces?
> Also, needs a comment why this is commented out.
OK
> > + CTL_BOOTLOADER = BIT(7)
>
> Add trailing comma.
OK
Marek
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU
2023-09-19 15:16 ` Marek Behún
@ 2023-09-19 18:27 ` Andy Shevchenko
2023-09-20 14:19 ` Marek Behún
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-09-19 18:27 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
linux-watchdog
On Tue, Sep 19, 2023 at 05:16:38PM +0200, Marek Behún wrote:
> On Tue, 19 Sep 2023 15:29:08 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:
...
> > > obj-$(CONFIG_GOLDFISH) += goldfish/
> > > obj-$(CONFIG_CHROME_PLATFORMS) += chrome/
> > > obj-$(CONFIG_SURFACE_PLATFORMS) += surface/
> > > +obj-$(CONFIG_CZNIC_PLATFORMS) += cznic/
> >
> > Why not ordered (to some extent) here (as you did in the other file)?
>
> Where should I put it? The other entries are not ordered, see
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/Makefile?h=v6.6-rc2
With the visible context above, at least after chrome already much better, no?
...
> > > +turris-omnia-mcu-objs := turris-omnia-mcu-base.o
> >
> > objs is for user space tools. Kernel code should use m,y.
>
> I don't understand. There are plenty driver using -objs. The driver
> consists of multiple C source files. I thought this should be done as
> obj-$(CONFIG_XXX) += xxx.o
> xxx-objs := xxx-a.o xxx-b.o xxx-c.o
>
> If you choose CONFIG_XXX = y, then the driver will be compiled into
> kernel, if you choose CONFIG_XXX = m, then the driver will be a module
> xxx.ko.
Hmm... Maybe I mixed something... Let me check.
For now than it's okay.
...
> > > +#include <linux/hex.h>
> > > +#include <linux/module.h>
> >
> > Missing types.h, sysfs.h, mod_devicetable.h, i2c.h, ...
>
> i2c.h comes from turris-omnia-mcu.h. The others I will fix. Does the
> the .c file also include all those headers if it includes a local
> header already?
For generic ones, yes.
...
> > > + if (ret < 0)
> >
> > Can it return positive value? What would be the meaning of it?
>
> Originally I had it return 0 on success, but recently I've had a
> discussion about this same thing with LED subsystem maintainer about
> some LED patches, Lee Jones, who requested the _read function to return
> number of bytes read on success. See at
> https://lore.kernel.org/linux-leds/20230821122644.GJ1380343@google.com/
> where he says:
> Read and write APIs, even abstracted ones such as these generally
> return the number of bytes successfully read and written respectively.
> If you are going to normalise, then please do so against this
> standard.
> I do not agree with him, since this is a local command accessor which
> only succeeds if all the bytes are read/written as requested. But I
> adjusted my code as per his request since he is the maintainer.
This is strange. For example, regmap APIs never returns amount of data written
or read. I think it's solely depends on the API. It might be useful for i²c
APIs, in case you can do something about it. but if you have wrappers on top
of that already (meaning not using directly the i2c_*() calls, I dunno the
positive return is anyhow useful.
> I will change the code back so that it returns 0 on success instead of
> the number of bytes written.
> > > + return ret;
...
> > > + version[40] = '\0';
> >
> > How do you know the version has enough space?
>
> The function is only use within the file, and both users know how much
> space it needs. But ok, I shall enforce it with the static keyword as
> static int
> omnia_get_version_hash(...,
> u8 version[static 2*OMNIA_FW_VERSION_LEN + 1]);
> Would that be okay?
Perhaps, it's a thing in a category "TIL", and it seems we have so few callers
of that.
...
> > > + int err;
> >
> > In two near functions you already inconsistent in the naming of the
> > return code variable. Be consistent across all your code, i.e. choose
> > one name and use it everywhere.
>
> I use
> ret - when the return value can also be positive and mean something
> err - when the return value is either 0 (no error) or negative errno
> Is this inconsistent?
TBH sounds good, but I close to never had heard about such a distinction in
a Linux kernel source file(s).
...
> > > + int status;
> >
> > My gosh, it's a _third_ name for the same!
>
> The variable holds value of the status register, which is needed at
> another point in this function. I thought naming it just "ret" would
> cause confusions... Do you really propose to rename it such?
It's not bad per se, just put yourself on the place of somebody who would like
to understand the semantics of all these variables. Maybe you can add a comment
on top explaining that (in short)?
...
> > > + for (int i = 0; i < ARRAY_SIZE(features); ++i) {
> >
> > Why signed?
>
> Because it is shorter and makes no difference and there are tons of
> for (int i = 0; i < ARRAY_SIZE(x); i++)
> in the kernel already. But if you really want, I can change it. Please
> let me know.
It just makes harder to get the code, because of possible unneeded questions
like "if it's a signed type, maybe it's on purpose?"
> > Why pre-increment?
>
> Again, it makes no difference, is widely used in the kernel and I
> personally prefer it. But I will change it if you want.
Pre-increment usually also a sign of something special about the iterator
variable. In non-special cases we use post-increment. And if you count
the use of each you will see the difference.
...
> > > + dev_info(dev,
> > > + "Consider upgrading MCU firmware with the
> > > omnia-mcutool utility.\n");
> >
> > You have so-o many dev_info() calls, are you sure you not abusing use
> > of that?
>
> I want the users to upgrade the MCU firmware. These infos are only
> printed during probe time, and won't be printed at all if the firmware
> is upgraded. Is this a problem?
Depends how really useful they are. If you think it's way to go, go for it.
...
> > > + if (likely(ret == ARRAY_SIZE(msgs)))
> >
> > Why likely()? Please, justify.
>
> Becuase it is unlikely the I2C transaction will fail. In most cases, it
> does not.
Yes, but why likely() is needed? So, i.o.w. what's the benefit in _this_ case?
> > > + return len;
...
> > > +enum commands_e {
> >
> > Are you sure the name is unique enough / properly namespaced?
> > Same Q to all enums.
>
> I can change it. I wanted it to be compatible with the header in the
> firmware repository.
Are these being generated, or also statically written in that repo?
If the latter, I think better to use more unique naming schema in
the kernel.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-19 10:38 ` [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs Marek Behún
2023-09-19 13:00 ` Andy Shevchenko
@ 2023-09-20 11:58 ` Linus Walleij
2023-09-20 13:36 ` Andy Shevchenko
2023-09-21 19:45 ` Marek Behún
1 sibling, 2 replies; 30+ messages in thread
From: Linus Walleij @ 2023-09-20 11:58 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Bartosz Golaszewski,
Andy Shevchenko, linux-gpio, Dmitry Torokhov
Hi Marek,
thanks for your patch! This is a very interesting hardware.
On Tue, Sep 19, 2023 at 12:38 PM Marek Behún <kabel@kernel.org> wrote:
> Add support for GPIOs connected to the MCU on the Turris Omnia board.
>
> This include:
> - front button pin
> - enable pins for USB regulators
> - MiniPCIe / mSATA card presence pins in MiniPCIe port 0
> - LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports
> - on board revisions 32+ also various peripheral resets and another
> voltage regulator enable pin
>
> Signed-off-by: Marek Behún <kabel@kernel.org>
OK all pretty straight-forward devices built on top of GPIO
in something like device tree or ACPI etc.
> --- a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
> +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
> @@ -1,3 +1,19 @@
> +What: /sys/bus/i2c/devices/<mcu_device>/front_button_mode
> +Date: August 2023
> +KernelVersion: 6.6
> +Contact: Marek Behún <kabel@kernel.org>
> +Description: (RW) The front button on the Turris Omnia router can be
> + configured either to change the intensity of all the LEDs on the
> + front panel, or to send the press event to the CPU as an
> + interrupt.
> +
> + This file switches between these two modes:
> + - "mcu" makes the button press event be handled by the MCU to
> + change the LEDs panel intensity.
> + - "cpu" makes the button press event be handled by the CPU.
> +
> + Format: %s.
I understand what this does, but why does this have to be configured
at runtime from userspace sysfs? Why can't this just be a property in
device tree or ACPI (etc)? I don't imagine a user is going to change
this back and forth are they? They likely want one or another.
> --- a/drivers/platform/cznic/turris-omnia-mcu-base.c
> +++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
> @@ -222,9 +222,20 @@ static int omnia_mcu_probe(struct i2c_client *client)
> return -ENODATA;
> }
>
> + ret = omnia_mcu_register_gpiochip(mcu);
> + if (ret)
> + return ret;
I guess it's OK to have a GPIOchip with the rest of the stuff, there
are a few precedents, such as drivers/bcma.
> +static const char * const omnia_mcu_gpio_names[64] = {
I would name these _templates since it is not the final names
that will be used. Very nice with all debug names though!
> +#define _DEF_GPIO(_cmd, _ctl_cmd, _bit, _ctl_bit, _int_bit, _feat, _feat_mask) \
> + { \
> + .cmd = _cmd, \
> + .ctl_cmd = _ctl_cmd, \
> + .bit = _bit, \
> + .ctl_bit = _ctl_bit, \
> + .int_bit = _int_bit, \
> + .feat = _feat, \
> + .feat_mask = _feat_mask, \
> + }
> +#define _DEF_GPIO_STS(_name) \
> + _DEF_GPIO(CMD_GET_STATUS_WORD, 0, STS_ ## _name, 0, INT_ ## _name, 0, 0)
> +#define _DEF_GPIO_CTL(_name) \
> + _DEF_GPIO(CMD_GET_STATUS_WORD, CMD_GENERAL_CONTROL, STS_ ## _name, \
> + CTL_ ## _name, 0, 0, 0)
> +#define _DEF_GPIO_EXT_STS(_name, _feat) \
> + _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \
> + INT_ ## _name, FEAT_ ## _feat | FEAT_EXT_CMDS, \
> + FEAT_ ## _feat | FEAT_EXT_CMDS)
> +#define _DEF_GPIO_EXT_STS_LED(_name, _ledext) \
> + _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \
> + INT_ ## _name, FEAT_LED_STATE_ ## _ledext, \
> + FEAT_LED_STATE_EXT_MASK)
> +#define _DEF_GPIO_EXT_STS_LEDALL(_name) \
> + _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \
> + INT_ ## _name, FEAT_LED_STATE_EXT_MASK, 0)
> +#define _DEF_GPIO_EXT_CTL(_name, _feat) \
> + _DEF_GPIO(CMD_GET_EXT_CONTROL_STATUS, CMD_EXT_CONTROL, \
> + EXT_CTL_ ## _name, EXT_CTL_ ## _name, 0, \
> + FEAT_ ## _feat | FEAT_EXT_CMDS, \
> + FEAT_ ## _feat | FEAT_EXT_CMDS)
> +#define _DEF_INT(_name) \
> + _DEF_GPIO(0, 0, 0, 0, INT_ ## _name, 0, 0)
Ugh that's really hard to read and understand, but I can't think of
anything better
so I guess we live with it.
> +static const struct omnia_gpio {
> + u8 cmd, ctl_cmd;
> + u32 bit, ctl_bit;
> + u32 int_bit;
If they are really just ONE bit I would actually use an int, encode
the bit number rather than the mask, and then use BIT( ..->int_bit)
from <linux/bits.h> everywhere I need a mask. No strong preference
though.
> +/* mapping from interrupts to indexes of GPIOs in the omnia_gpios array */
> +static const unsigned int omnia_int_to_gpio_idx[32] = {
> + [ilog2(INT_CARD_DET)] = 4,
> + [ilog2(INT_MSATA_IND)] = 5,
> + [ilog2(INT_USB30_OVC)] = 6,
ilog2 is a bit unituitive for a programmer, are you a schooled mathematician
Marek? ;)
It is also a consequence of using a bitmask rather than a bit number in
the struct, all ilog2 does is fls-1. So what about just putting the bit number
in the struct and then all of these assignments will look natural as well.
> +/* index of PHY_SFP GPIO in the omnia_gpios array */
> +enum {
> + OMNIA_GPIO_PHY_SFP_OFFSET = 54,
> +};
I would just use a define for this, the enum isn't very useful for
singular values.
> +static int omnia_ctl_cmd(struct omnia_mcu *mcu, u8 cmd, u16 val, u16 mask)
> +{
> + int err;
> +
> + mutex_lock(&mcu->lock);
> + err = omnia_ctl_cmd_unlocked(mcu, cmd, val, mask);
We just discussed this on an unrelated topic: *_unlocked means it should
be called without need for the appropriate mutex to be locked, since it clearly
requires &mcu->lock to be taken, this should be
omnia_ctl_cmd_locked() as in "call this with the lock held".
I did a quick grep _locked to convince myself about this.
Obvious exampled include wait_event_interruptible_locked_irq()
or wake_up_locked() in the core kernel infrastructure that clearly states
(include/linux/wait.h for wait_event_interruptible_locked():
"It must be called with wq.lock being held."
> +static int omnia_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
(...)
> + if (gpio->cmd == CMD_GET_STATUS_WORD &&
> + !(mcu->features & FEAT_NEW_INT_API))
> + return !!(mcu->last_status & gpio->bit);
So I expect something like return !!(mcu->last_status & BIT(gpio->bit));
> +static int omnia_gpio_get_multiple(struct gpio_chip *gc, unsigned long *mask,
> + unsigned long *bits)
> +{
> + mutex_lock(&mcu->lock);
(...)
> + if (err)
> + goto err_unlock;
(...)
> + if (err)
> + goto err_unlock;
(...)
> + if (err)
> + goto err_unlock;
> +
> + mutex_unlock(&mcu->lock);
This function is kind of a good example of where using scoped guards
from <linux/cleanup.h> can simplify the code. After the initial lock you
can just return on error and let the cleanup handler unlock the mutex,
just guard(mutex)(&mcu->lock);
In the beginning and then it's done, the lock will be held until the
function is exited.
> +static void omnia_gpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> + unsigned long *bits)
> +{
(...)
same here.
> +static int omnia_gpio_init_valid_mask(struct gpio_chip *gc,
> + unsigned long *valid_mask,
> + unsigned int ngpios)
> +{
> + struct omnia_mcu *mcu = gpiochip_get_data(gc);
> +
> + bitmap_zero(valid_mask, ngpios);
> +
> + for (int i = 0; i < ngpios; ++i) {
> + const struct omnia_gpio *gpio = &omnia_gpios[i];
> +
> + if (!gpio->cmd && !gpio->int_bit)
> + continue;
> +
> + if (omnia_gpio_available(mcu, gpio))
> + set_bit(i, valid_mask);
Speaking of locks, isn't this where you should use __set_bit()
rather than set_bit()?
__set_bit() is incomprehensible shorthand for "set_bit_nonatomic".
> +static void omnia_irq_shutdown(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct omnia_mcu *mcu = gpiochip_get_data(gc);
> + irq_hw_number_t hwirq = irqd_to_hwirq(d);
> + u32 bit = omnia_gpios[hwirq].int_bit;
> +
> + mcu->rising &= ~bit;
> + mcu->falling &= ~bit;
Yeah so here it would be BIT(bit) etc.
> + if (type & IRQ_TYPE_EDGE_RISING)
> + mcu->rising |= bit;
> + else
> + mcu->rising &= ~bit;
And with bits in place of bitmasks these would be
if ()
__set_bit(bit, mcu->rising);
else
__clear_bit(bit, mcu->rising);
etc
> +static void omnia_irq_init_valid_mask(struct gpio_chip *gc,
> + unsigned long *valid_mask,
> + unsigned int ngpios)
> +{
> + struct omnia_mcu *mcu = gpiochip_get_data(gc);
> +
> + bitmap_zero(valid_mask, ngpios);
> +
> + for (int i = 0; i < ngpios; ++i) {
> + const struct omnia_gpio *gpio = &omnia_gpios[i];
> +
> + if (!gpio->int_bit)
> + continue;
> +
> + if (omnia_gpio_available(mcu, gpio))
> + set_bit(i, valid_mask);
__set_bit()
> +static bool omnia_irq_read_pending_new(struct omnia_mcu *mcu,
> + unsigned long *pending)
> +{
> + u32 rising, falling;
> + u8 reply[8] = {};
> + size_t len;
> + int err;
> +
> + /*
> + * Determine how many bytes we need to read from the reply to the
> + * CMD_GET_INT_AND_CLEAR command in order to retrieve all unmasked
> + * interrupts.
> + */
> + len = max(((ilog2(mcu->rising & mcu->mask) >> 3) << 1) + 1,
> + ((ilog2(mcu->falling & mcu->mask) >> 3) << 1) + 2);
Really hard to read, and again easier if we store the bit number
rather than the bitmask.
> + mutex_lock(&mcu->lock);
Scoped guard?
> +static void button_release_emul_fn(struct work_struct *work)
> +{
> + struct omnia_mcu *mcu = container_of(to_delayed_work(work),
> + struct omnia_mcu,
> + button_release_emul_work);
> +
> + mcu->button_pressed_emul = false;
> + generic_handle_irq_safe(mcu->client->irq);
> +}
(...)
> + /*
> + * The old firmware triggers an interrupt whenever status word changes,
> + * but does not inform about which bits rose or fell. We need to compute
> + * this here by comparing with the last status word value.
> + *
> + * The STS_BUTTON_PRESSED bit needs special handling, because the old
> + * firmware clears the STS_BUTTON_PRESSED bit on successful completion
> + * of the CMD_GET_STATUS_WORD command, resulting in another interrupt:
> + * - first we get an interrupt, we read the status word where
> + * STS_BUTTON_PRESSED is present,
> + * - MCU clears the STS_BUTTON_PRESSED bit because we read the status
> + * word,
> + * - we get another interrupt because the status word changed again
> + * (STS_BUTTON_PRESSED was cleared).
> + *
> + * The gpiolib-cdev, gpiolib-sysfs and gpio-keys input driver all call
> + * the gpiochip's .get() method after an edge event on a requested GPIO
> + * occurs.
> + *
> + * We ensure that the .get() method reads 1 for the button GPIO for some
> + * time.
> + */
> +
> + if (status & STS_BUTTON_PRESSED) {
> + mcu->button_pressed_emul = true;
> + mod_delayed_work(system_wq, &mcu->button_release_emul_work,
> + msecs_to_jiffies(50));
> + } else if (mcu->button_pressed_emul) {
> + status |= STS_BUTTON_PRESSED;
> + }
I feel a bit icky about this, would be best if Dmitry Torokhov review
such input subsystem-related things.
It looks like a reimplementation of drivers/input/keyboard/gpio_keys.c
so why not just connect a gpio-keys device using either device tree
or a software node? (Dmitry knows all about how to create proper
software nodes for stuff like this, DT is self-evident.)
> + mcu->gc.names = omnia_mcu_gpio_names;
Do we really support format strings in these names? I don't
think so, or I'm wrong about my own code... (Which wouldn't
be the first time.)
> #include <asm/byteorder.h>
> +#include <linux/gpio/driver.h>
> #include <linux/i2c.h>
> +#include <linux/log2.h>
I think we want to get rid of log2.h from this driver.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/7] dt-bindings: arm: add cznic,turris-omnia-mcu binding
2023-09-19 10:38 ` [PATCH v2 1/7] dt-bindings: arm: add cznic,turris-omnia-mcu binding Marek Behún
@ 2023-09-20 12:37 ` Krzysztof Kozlowski
0 siblings, 0 replies; 30+ messages in thread
From: Krzysztof Kozlowski @ 2023-09-20 12:37 UTC (permalink / raw)
To: Marek Behún, Gregory CLEMENT, Arnd Bergmann, soc, arm,
Rob Herring, Krzysztof Kozlowski, devicetree
On 19/09/2023 12:38, Marek Behún wrote:
> Add binding for cznic,turris-omnia-mcu, the device-tree node
> representing the system-controller features provided by the MCU on the
> Turris Omnia router.
>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-20 11:58 ` Linus Walleij
@ 2023-09-20 13:36 ` Andy Shevchenko
2023-09-21 19:45 ` Marek Behún
1 sibling, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2023-09-20 13:36 UTC (permalink / raw)
To: Linus Walleij
Cc: Marek Behún, Gregory CLEMENT, Arnd Bergmann, soc, arm,
Bartosz Golaszewski, linux-gpio, Dmitry Torokhov
On Wed, Sep 20, 2023 at 01:58:37PM +0200, Linus Walleij wrote:
> On Tue, Sep 19, 2023 at 12:38 PM Marek Behún <kabel@kernel.org> wrote:
...
> > + if (type & IRQ_TYPE_EDGE_RISING)
> > + mcu->rising |= bit;
> > + else
> > + mcu->rising &= ~bit;
>
> And with bits in place of bitmasks these would be
>
> if ()
> __set_bit(bit, mcu->rising);
> else
> __clear_bit(bit, mcu->rising);
More precisely __assign_bit() in this case.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU
2023-09-19 18:27 ` Andy Shevchenko
@ 2023-09-20 14:19 ` Marek Behún
2023-09-20 14:47 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Marek Behún @ 2023-09-20 14:19 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
linux-watchdog
Hello Andy,
On Tue, 19 Sep 2023 21:27:04 +0300
Andy Shevchenko <andy@kernel.org> wrote:
> On Tue, Sep 19, 2023 at 05:16:38PM +0200, Marek Behún wrote:
> > On Tue, 19 Sep 2023 15:29:08 +0300
> > Andy Shevchenko <andy@kernel.org> wrote:
> > > On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:
>
> ...
>
> > > > obj-$(CONFIG_GOLDFISH) += goldfish/
> > > > obj-$(CONFIG_CHROME_PLATFORMS) += chrome/
> > > > obj-$(CONFIG_SURFACE_PLATFORMS) += surface/
> > > > +obj-$(CONFIG_CZNIC_PLATFORMS) += cznic/
> > >
> > > Why not ordered (to some extent) here (as you did in the other file)?
> >
> > Where should I put it? The other entries are not ordered, see
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/Makefile?h=v6.6-rc2
>
> With the visible context above, at least after chrome already much better, no?
OK
...
> > > Missing types.h, sysfs.h, mod_devicetable.h, i2c.h, ...
> >
> > i2c.h comes from turris-omnia-mcu.h. The others I will fix. Does the
> > the .c file also include all those headers if it includes a local
> > header already?
>
> For generic ones, yes.
OK
> > > > + if (ret < 0)
> > >
> > > Can it return positive value? What would be the meaning of it?
> >
> > Originally I had it return 0 on success, but recently I've had a
> > discussion about this same thing with LED subsystem maintainer about
> > some LED patches, Lee Jones, who requested the _read function to return
> > number of bytes read on success. See at
> > https://lore.kernel.org/linux-leds/20230821122644.GJ1380343@google.com/
> > where he says:
> > Read and write APIs, even abstracted ones such as these generally
> > return the number of bytes successfully read and written respectively.
> > If you are going to normalise, then please do so against this
> > standard.
> > I do not agree with him, since this is a local command accessor which
> > only succeeds if all the bytes are read/written as requested. But I
> > adjusted my code as per his request since he is the maintainer.
>
> This is strange. For example, regmap APIs never returns amount of data written
> or read. I think it's solely depends on the API. It might be useful for i²c
> APIs, in case you can do something about it. but if you have wrappers on top
> of that already (meaning not using directly the i2c_*() calls, I dunno the
> positive return is anyhow useful.
I shall change it back then.
...
> > > > + int status;
> > >
> > > My gosh, it's a _third_ name for the same!
> >
> > The variable holds value of the status register, which is needed at
> > another point in this function. I thought naming it just "ret" would
> > cause confusions... Do you really propose to rename it such?
>
> It's not bad per se, just put yourself on the place of somebody who would like
> to understand the semantics of all these variables. Maybe you can add a comment
> on top explaining that (in short)?
OK
> > > > + for (int i = 0; i < ARRAY_SIZE(features); ++i) {
> > >
> > > Why signed?
> >
> > Because it is shorter and makes no difference and there are tons of
> > for (int i = 0; i < ARRAY_SIZE(x); i++)
> > in the kernel already. But if you really want, I can change it. Please
> > let me know.
>
> It just makes harder to get the code, because of possible unneeded questions
> like "if it's a signed type, maybe it's on purpose?"
OK
> > > Why pre-increment?
> >
> > Again, it makes no difference, is widely used in the kernel and I
> > personally prefer it. But I will change it if you want.
>
> Pre-increment usually also a sign of something special about the iterator
> variable. In non-special cases we use post-increment. And if you count
> the use of each you will see the difference.
OK
...
> > > > + if (likely(ret == ARRAY_SIZE(msgs)))
> > >
> > > Why likely()? Please, justify.
> >
> > Becuase it is unlikely the I2C transaction will fail. In most cases, it
> > does not.
>
> Yes, but why likely() is needed? So, i.o.w. what's the benefit in _this_ case?
Compiler optimization (one branch avoided). But I guess this isn't a
hot path, since I2C is insanely slow anyway. OK, I shall remove the
likely() usage.
> ...
>
> > > > +enum commands_e {
> > >
> > > Are you sure the name is unique enough / properly namespaced?
> > > Same Q to all enums.
> >
> > I can change it. I wanted it to be compatible with the header in the
> > firmware repository.
>
> Are these being generated, or also statically written in that repo?
> If the latter, I think better to use more unique naming schema in
> the kernel.
OK, I shall add some prefix.
Marek
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU
2023-09-20 14:19 ` Marek Behún
@ 2023-09-20 14:47 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2023-09-20 14:47 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
linux-watchdog
On Wed, Sep 20, 2023 at 04:19:53PM +0200, Marek Behún wrote:
> On Tue, 19 Sep 2023 21:27:04 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Tue, Sep 19, 2023 at 05:16:38PM +0200, Marek Behún wrote:
> > > On Tue, 19 Sep 2023 15:29:08 +0300
> > > Andy Shevchenko <andy@kernel.org> wrote:
> > > > On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:
...
> > > > > + if (likely(ret == ARRAY_SIZE(msgs)))
> > > >
> > > > Why likely()? Please, justify.
> > >
> > > Becuase it is unlikely the I2C transaction will fail. In most cases, it
> > > does not.
> >
> > Yes, but why likely() is needed? So, i.o.w. what's the benefit in _this_ case?
>
> Compiler optimization (one branch avoided). But I guess this isn't a
> hot path, since I2C is insanely slow anyway. OK, I shall remove the
> likely() usage.
Have you seen the difference in the generated code, btw?
I don't think it will get you one independently on the hot/slow
path.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-19 13:00 ` Andy Shevchenko
@ 2023-09-20 17:08 ` Marek Behún
2023-09-21 9:55 ` Andy Shevchenko
2023-09-21 18:42 ` Marek Behún
1 sibling, 1 reply; 30+ messages in thread
From: Marek Behún @ 2023-09-20 17:08 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio
Hi Andy,
I'm sending reply to some of your comments. For those to which I do not
reply I will simply incorporate your suggestions.
On Tue, 19 Sep 2023 16:00:39 +0300
Andy Shevchenko <andy@kernel.org> wrote:
> On Tue, Sep 19, 2023 at 12:38:11PM +0200, Marek Behún wrote:
> > - front button pin
> > - enable pins for USB regulators
> > - MiniPCIe / mSATA card presence pins in MiniPCIe port 0
> > - LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports
> > - on board revisions 32+ also various peripheral resets and another
> > voltage regulator enable pin
>
> Can the main driver provide a regmap and all other use it?
It can't. These are not registers accessible via I2C at specific
addresses, but commands with different-length arguments. This was
already discussed 4 years ago with Jacek, see
https://www.spinics.net/lists/linux-leds/msg11587.html
There, Jacek suggested writing regmap API for the leds-turris-omnia
driver (the MCU is also a LED controller and has same command
interface, only at different I2C address).
> > + if (gpio->ctl_cmd)
> > + return -EOPNOTSUPP;
>
> I believe internally we use ENOTSUPP.
> Ditto for all cases like this.
checkpatch warns:
ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> > + mutex_lock(&mcu->lock);
> > +
> > + if (ctl_mask)
> > + err = omnia_ctl_cmd_unlocked(mcu,
> > CMD_GENERAL_CONTROL, ctl,
> > + ctl_mask);
>
> > + if (!err && ext_ctl_mask)
> > + err = omnia_ctl_cmd_unlocked(mcu, CMD_EXT_CONTROL,
> > ext_ctl,
> > + ext_ctl_mask);
>
> Can it be
>
> if (err)
> goto out_unlock;
>
> if (_mask)
> ...
Linus suggested using guards, so I will refactor this away.
> > + mutex_unlock(&mcu->lock);
>
> > + if (err)
> > + dev_err(&mcu->client->dev, "Cannot set GPIOs:
> > %d\n", err);
>
> How is useful this one?
The function does not return, this way we will know something has gone
wrong. I am used to do this from the networking subsystem, where people
at least from mv88e6xxx wanted such behavior. Do you want me to drop
this?
> > +static int omnia_gpio_init_valid_mask(struct gpio_chip *gc,
> > + unsigned long *valid_mask,
> > + unsigned int ngpios)
> > +{
> > + struct omnia_mcu *mcu = gpiochip_get_data(gc);
>
> > + bitmap_zero(valid_mask, ngpios);
>
> No need.
>
> Also do you have bitops.h included?
bitmap.h actually for this
> > + rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
> > + (reply[6] << 24);
> > + falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
> > + (reply[7] << 24);
>
> With a help of two masks, you can access to the both edges as to
> 64-bit value and simplify the code.
Huh? As in
rising = reply & 0x00ff00ff00ff00ff;
falling = reply & 0xff00ff00ff00ff00;
?
But then I can't or the rising bit with the corresponding falling bit
to get pending...
Or I guess i can with:
pending = rising & (pending >> 8);
Am I understanding you correctly?
But then I would need to store the mask in driver data as a 64-bit
value with half the data not used. Also the CPU is 32-bit.
> > +static struct attribute *omnia_mcu_gpio_attrs[] = {
> > + &dev_attr_front_button_mode.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group omnia_mcu_gpio_group = {
> > + .attrs = omnia_mcu_gpio_attrs,
> > +};
>
> ATTRIBUTE_GROUPS() ?
I only want to create ane attribute_group. (I am using
devm_device_add_group(), not devm_device_add_groups()).
> > + err = devm_device_add_group(dev, &omnia_mcu_gpio_group);
>
> No way, no-one should use the API scheduled for removal.
> What's wrong with .dev_groups ?
Can I add them conditionally? GPIO chip is always created, but the
patch 4/7 only adds the attributes conditionally. Is it possible via
.dev_groups?
>
> ...
>
> > +void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu)
> > +{
> > + if (!(mcu->features & FEAT_NEW_INT_API))
> > +
> > cancel_delayed_work_sync(&mcu->button_release_emul_work); +
> > + mutex_destroy(&mcu->lock);
>
> Wrong order?
No, the mutex may be used in the work. Can't destroy it first. Or am I
misunderstanding something?
> > struct omnia_mcu {
> > struct i2c_client *client;
> > const char *type;
> > u16 features;
> > +
> > + /* GPIO chip */
> > + struct gpio_chip gc;
>
> Making this a first member may lead to the better code. Check with
> bloat-o-meter.
kabel@dellmb ~/linux $ ./scripts/bloat-o-meter \
turris-omnia-mcu.o turris-omnia-mcu-gpiochip-first.o
add/remove: 0/0 grow/shrink: 9/0 up/down: 52/0 (52)
Function old new delta
omnia_mcu_register_gpiochip 840 852 +12
omnia_mcu_probe 696 708 +12
omnia_mcu_unregister_gpiochip 20 24 +4
omnia_irq_bus_sync_unlock 256 260 +4
omnia_irq_bus_lock 36 40 +4
omnia_gpio_get_multiple 872 876 +4
omnia_gpio_get 372 376 +4
fw_features_show 28 32 +4
front_button_mode_show 260 264 +4
Total: Before=10468, After=10520, chg +0.50%
Seems the code grew when I swapped it.
Marek
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-20 17:08 ` Marek Behún
@ 2023-09-21 9:55 ` Andy Shevchenko
2023-09-21 20:25 ` Marek Behún
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-09-21 9:55 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio
On Wed, Sep 20, 2023 at 07:08:18PM +0200, Marek Behún wrote:
> On Tue, 19 Sep 2023 16:00:39 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Tue, Sep 19, 2023 at 12:38:11PM +0200, Marek Behún wrote:
...
> > Ditto for all cases like this.
>
> checkpatch warns:
> ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
So, fix checkpatch then. It has false positives, because it doesn't know
if the actual error code will sneak outside the kernel or not. But in
the subsystem we internally use ENOTSUPP.
...
> Linus suggested using guards, so I will refactor this away.
Even better.
...
> > > + dev_err(&mcu->client->dev, "Cannot set GPIOs:
> > > %d\n", err);
> >
> > How is useful this one?
>
> The function does not return, this way we will know something has gone
> wrong. I am used to do this from the networking subsystem, where people
> at least from mv88e6xxx wanted such behavior. Do you want me to drop
> this?
You are the developer and owner of the specific hardware, if you have a good
justification for this message _in production_, then leave it, otherwise drop.
...
> > > + bitmap_zero(valid_mask, ngpios);
> >
> > No need.
> >
> > Also do you have bitops.h included?
>
> bitmap.h actually for this
Right, but I already thought a step forward, when you drop bitmap APIs,
the bitops are still in place.
...
> > > + rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
> > > + (reply[6] << 24);
> > > + falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
> > > + (reply[7] << 24);
> >
> > With a help of two masks, you can access to the both edges as to
> > 64-bit value and simplify the code.
>
> Huh? As in
> rising = reply & 0x00ff00ff00ff00ff;
> falling = reply & 0xff00ff00ff00ff00;
> ?
> But then I can't or the rising bit with the corresponding falling bit
> to get pending...
> Or I guess i can with:
> pending = rising & (pending >> 8);
>
> Am I understanding you correctly?
>
> But then I would need to store the mask in driver data as a 64-bit
> value with half the data not used. Also the CPU is 32-bit.
If you use proper bitmaps, perhaps this will be easier. You can use one for
each and merge them whenever you want (with bitmap_or() call) or split (with
bitmap_and() respectively):
bitmap_or(full, raising, failing); // merge
bitmap_and(raising, full, rasing_mask); // split
...
> > ATTRIBUTE_GROUPS() ?
>
> I only want to create ane attribute_group. (I am using
> devm_device_add_group(), not devm_device_add_groups()).
...and you should not use that APIs.
> > > + err = devm_device_add_group(dev, &omnia_mcu_gpio_group);
> >
> > No way, no-one should use the API scheduled for removal.
> > What's wrong with .dev_groups ?
>
> Can I add them conditionally? GPIO chip is always created, but the
> patch 4/7 only adds the attributes conditionally. Is it possible via
> .dev_groups?
Yes. You use __ATTRIBUTE_GROUPS() and .is_visible callback.
...
> > > +void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu)
> > > +{
> > > + if (!(mcu->features & FEAT_NEW_INT_API))
> > > +
> > > cancel_delayed_work_sync(&mcu->button_release_emul_work); +
> > > + mutex_destroy(&mcu->lock);
> >
> > Wrong order?
>
> No, the mutex may be used in the work. Can't destroy it first. Or am I
> misunderstanding something?
I mean you are using a lot of devm(), can mutex be used in IRQ or whatever
that can be triggered after this call?
...
> > > struct i2c_client *client;
> > > const char *type;
> > > u16 features;
> > > +
> > > + /* GPIO chip */
> > > + struct gpio_chip gc;
> >
> > Making this a first member may lead to the better code. Check with
> > bloat-o-meter.
>
> kabel@dellmb ~/linux $ ./scripts/bloat-o-meter \
> turris-omnia-mcu.o turris-omnia-mcu-gpiochip-first.o
> add/remove: 0/0 grow/shrink: 9/0 up/down: 52/0 (52)
> Function old new delta
> omnia_mcu_register_gpiochip 840 852 +12
> omnia_mcu_probe 696 708 +12
> omnia_mcu_unregister_gpiochip 20 24 +4
> omnia_irq_bus_sync_unlock 256 260 +4
> omnia_irq_bus_lock 36 40 +4
> omnia_gpio_get_multiple 872 876 +4
> omnia_gpio_get 372 376 +4
> fw_features_show 28 32 +4
> front_button_mode_show 260 264 +4
> Total: Before=10468, After=10520, chg +0.50%
>
> Seems the code grew when I swapped it.
Thanks for checking! It means that access to client pointer is needed
more often.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-19 13:00 ` Andy Shevchenko
2023-09-20 17:08 ` Marek Behún
@ 2023-09-21 18:42 ` Marek Behún
2023-09-22 14:14 ` Andy Shevchenko
1 sibling, 1 reply; 30+ messages in thread
From: Marek Behún @ 2023-09-21 18:42 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio
On Tue, 19 Sep 2023 16:00:39 +0300
Andy Shevchenko <andy@kernel.org> wrote:
> > + mutex_lock(&mcu->lock);
> > +
> > + if (ctl_mask)
> > + err = omnia_ctl_cmd_unlocked(mcu, CMD_GENERAL_CONTROL, ctl,
> > + ctl_mask);
>
> > + if (!err && ext_ctl_mask)
> > + err = omnia_ctl_cmd_unlocked(mcu, CMD_EXT_CONTROL, ext_ctl,
> > + ext_ctl_mask);
>
> Can it be
>
> if (err)
> goto out_unlock;
>
> if (_mask)
> ...
>
> ?
Hi Andy,
so I am refactoring this to use guard(mutex), but now I have this:
guard(mutex, &mcu->lock);
if (ctl_mask) {
err = ...;
if (err)
goto out_err;
}
if (ext_ctl_mask) {
err = ...;
if (err)
goto out_err;
}
return;
out_err:
dev_err(dev, "Cannot set GPIOs: %d\n", err);
which clearly is not any better... or at least the original
if (!err && ext_ctl_mask)
is better IMO.
Compare with:
guard(mutex, &mcu->lock);
if (ctl_mask)
err = ...;
if (!err && ext_ctl_mask)
err = ...;
if (err)
dev_err(dev, "Cannot set GPIOs: %d\n", err);
Do you have a better suggestion?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-20 11:58 ` Linus Walleij
2023-09-20 13:36 ` Andy Shevchenko
@ 2023-09-21 19:45 ` Marek Behún
2023-09-21 20:14 ` Marek Behún
1 sibling, 1 reply; 30+ messages in thread
From: Marek Behún @ 2023-09-21 19:45 UTC (permalink / raw)
To: Linus Walleij
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Bartosz Golaszewski,
Andy Shevchenko, linux-gpio, Dmitry Torokhov
Hello Linus,
I am sending some comments/questions to your review. To those of
your comments that I do not reply, I will incorporate your suggestion.
On Wed, 20 Sep 2023 13:58:37 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:
> > --- a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
> > +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
> > @@ -1,3 +1,19 @@
> > +What: /sys/bus/i2c/devices/<mcu_device>/front_button_mode
> > +Date: August 2023
> > +KernelVersion: 6.6
> > +Contact: Marek Behún <kabel@kernel.org>
> > +Description: (RW) The front button on the Turris Omnia router can be
> > + configured either to change the intensity of all the LEDs on the
> > + front panel, or to send the press event to the CPU as an
> > + interrupt.
> > +
> > + This file switches between these two modes:
> > + - "mcu" makes the button press event be handled by the MCU to
> > + change the LEDs panel intensity.
> > + - "cpu" makes the button press event be handled by the CPU.
> > +
> > + Format: %s.
>
> I understand what this does, but why does this have to be configured
> at runtime from userspace sysfs? Why can't this just be a property in
> device tree or ACPI (etc)? I don't imagine a user is going to change
> this back and forth are they? They likely want one or another.
Yes, we really do expect this, we even want to have some applications
on the router to do this.
> > +static const struct omnia_gpio {
> > + u8 cmd, ctl_cmd;
> > + u32 bit, ctl_bit;
> > + u32 int_bit;
>
> If they are really just ONE bit I would actually use an int, encode
> the bit number rather than the mask, and then use BIT( ..->int_bit)
> from <linux/bits.h> everywhere I need a mask. No strong preference
> though.
See below.
> > +/* mapping from interrupts to indexes of GPIOs in the omnia_gpios array */
> > +static const unsigned int omnia_int_to_gpio_idx[32] = {
> > + [ilog2(INT_CARD_DET)] = 4,
> > + [ilog2(INT_MSATA_IND)] = 5,
> > + [ilog2(INT_USB30_OVC)] = 6,
>
> ilog2 is a bit unituitive for a programmer, are you a schooled mathematician
> Marek? ;)
>
> It is also a consequence of using a bitmask rather than a bit number in
> the struct, all ilog2 does is fls-1. So what about just putting the bit number
> in the struct and then all of these assignments will look natural as well.
Not really, altough I studied advanced analysis :) I guess ilog2 might
confuse some people. Would bf_shf() (as in bitfield shift) from
linux/bitfield.h be better?
So, the reasoning behind why the bits are defined as such in
linux/turris-omnia-mcu-interface.h (from patch 2/7):
- if you look for example at enum sts_word_e, the entries correspond
to fields withing the word. And not all fields are 1-bit wide. For
example the STS_MCU_TYPE field is 2 bits wide.
Similarly the FEAT_LED_STATE_EXT_MASK field in enum features_e
- I have seen similar code within many parts of the kernel and have
adopted it. I think many developers are accustomed to it. Look for
example into how register fields are defined in the mv88e6xxx driver
header files
In order to make maintaining smoother, I would like to have the
turris-omnia-mcu-interface.h file be one-to-one copy of the
corresponding file within the MCU firmware repository
(https://gitlab.nic.cz/turris/hw/omnia_hw_ctrl/-/blob/master/src/drivers/i2c_iface.h)
wherein the constants are defined this way and used throughout the
codebase.
Since I am the author and maintainer of the (current versions of) MCU
firmware, I could take your suggestion and change the constants, and
rewrite the firmware.
But taking into consideration the above two points, I am reluctant to
do so.
So... do you insist on this change? I would rather keep it as it is,
but I can change the ilog2() to bf_shf() to make it easier for
non-mathematicians.
> > +static bool omnia_irq_read_pending_new(struct omnia_mcu *mcu,
> > + unsigned long *pending)
> > +{
> > + u32 rising, falling;
> > + u8 reply[8] = {};
> > + size_t len;
> > + int err;
> > +
> > + /*
> > + * Determine how many bytes we need to read from the reply to the
> > + * CMD_GET_INT_AND_CLEAR command in order to retrieve all unmasked
> > + * interrupts.
> > + */
> > + len = max(((ilog2(mcu->rising & mcu->mask) >> 3) << 1) + 1,
> > + ((ilog2(mcu->falling & mcu->mask) >> 3) << 1) + 2);
>
> Really hard to read, and again easier if we store the bit number
> rather than the bitmask.
No. Even if I take your suggestion and rewrite this to store bit offsets
instead of bit masks, here I really need to compute this, since there
might be multiple bits set. For example there might be bits 4, 5, and
14 set in mcu->rising (mcu->rising = 0x4030).
I need to determine how many bytes of the I2C reply I will need, and
for that I need to know the position of the first set bit.
(I could keep a track of the maximum set bit, but honestly this seems
to me much easier).
I could use ffs(x) instead of ilog2(x) + 1.
> > +static void button_release_emul_fn(struct work_struct *work)
> > +{
> > + struct omnia_mcu *mcu = container_of(to_delayed_work(work),
> > + struct omnia_mcu,
> > + button_release_emul_work);
> > +
> > + mcu->button_pressed_emul = false;
> > + generic_handle_irq_safe(mcu->client->irq);
> > +}
> (...)
> > + /*
> > + * The old firmware triggers an interrupt whenever status word changes,
> > + * but does not inform about which bits rose or fell. We need to compute
> > + * this here by comparing with the last status word value.
> > + *
> > + * The STS_BUTTON_PRESSED bit needs special handling, because the old
> > + * firmware clears the STS_BUTTON_PRESSED bit on successful completion
> > + * of the CMD_GET_STATUS_WORD command, resulting in another interrupt:
> > + * - first we get an interrupt, we read the status word where
> > + * STS_BUTTON_PRESSED is present,
> > + * - MCU clears the STS_BUTTON_PRESSED bit because we read the status
> > + * word,
> > + * - we get another interrupt because the status word changed again
> > + * (STS_BUTTON_PRESSED was cleared).
> > + *
> > + * The gpiolib-cdev, gpiolib-sysfs and gpio-keys input driver all call
> > + * the gpiochip's .get() method after an edge event on a requested GPIO
> > + * occurs.
> > + *
> > + * We ensure that the .get() method reads 1 for the button GPIO for some
> > + * time.
> > + */
> > +
> > + if (status & STS_BUTTON_PRESSED) {
> > + mcu->button_pressed_emul = true;
> > + mod_delayed_work(system_wq, &mcu->button_release_emul_work,
> > + msecs_to_jiffies(50));
> > + } else if (mcu->button_pressed_emul) {
> > + status |= STS_BUTTON_PRESSED;
> > + }
>
> I feel a bit icky about this, would be best if Dmitry Torokhov review
> such input subsystem-related things.
>
> It looks like a reimplementation of drivers/input/keyboard/gpio_keys.c
> so why not just connect a gpio-keys device using either device tree
> or a software node? (Dmitry knows all about how to create proper
> software nodes for stuff like this, DT is self-evident.)
You have it the other way around :-) This is done in order so that the
gpio-keys driver can use it. Let me explain:
The new versions of the MCU firmware support rising and falling edges
for GPIO events. So when button is pressed, we get an event, and when
it is released, we get an event. gpio-keys works perfectly.
The problem with old MCU firmware is that the interrupt API was
tragically designed, which resulted in that on button press event, we
get an interrupt, and when we read the status word via I2C, the bit gets
cleared immediately (and we get another interrupt because the bit is
cleared). There is no interrupt when the button is truly released.
This fake "button release interrupt" comes so fast that the gpio_keys
driver ignores it even if we set debounce_interval to smallest possible
value of 1ms (if I understand the gpio_keys driver correctly).
So this workaround is there so that we generate a GPIO release event
for the button only after 50ms, if the MCU is running old firmware.
A similar thing is implemented in the gpio-keys driver when there is no
release event: if the event source is an interrupt descriptor instead
of a gpio descriptor, the gpio-keys will emulate button release.
I could use this, but then we would need to change the device-tree
based on whether the MCU is running old firmware vs new, which I want
to avoid. I also want to avoid software nodes, since I want the button
to be properly described in device-tree.
Honestly, because of this whole debacle I was also considering writing
my own small input driver within this turris-omnia-mcu driver...
> > + mcu->gc.names = omnia_mcu_gpio_names;
>
> Do we really support format strings in these names? I don't
> think so, or I'm wrong about my own code... (Which wouldn't
> be the first time.)
Yes we do, but only one :) The number of the GPIO.
> > #include <asm/byteorder.h>
> > +#include <linux/gpio/driver.h>
> > #include <linux/i2c.h>
> > +#include <linux/log2.h>
>
> I think we want to get rid of log2.h from this driver.
See above, even if I take your suggestion, I will still need the
position of the first set bit within word in
omnia_irq_read_pending_new().
Marek
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-21 19:45 ` Marek Behún
@ 2023-09-21 20:14 ` Marek Behún
2023-09-22 14:21 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Marek Behún @ 2023-09-21 20:14 UTC (permalink / raw)
To: Linus Walleij
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Bartosz Golaszewski,
Andy Shevchenko, linux-gpio, Dmitry Torokhov
On Thu, 21 Sep 2023 21:45:57 +0200
Marek Behún <kabel@kernel.org> wrote:
> I could use ffs(x) instead of ilog2(x) + 1.
Pardon me, I meant fls(). Or maybe get_bitmask_order() from
linux/bitops.h.
Marek
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-21 9:55 ` Andy Shevchenko
@ 2023-09-21 20:25 ` Marek Behún
2023-09-22 14:18 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Marek Behún @ 2023-09-21 20:25 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio
Hello Andy,
On Thu, 21 Sep 2023 12:55:08 +0300
Andy Shevchenko <andy@kernel.org> wrote:
> > checkpatch warns:
> > ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
>
> So, fix checkpatch then. It has false positives, because it doesn't know
> if the actual error code will sneak outside the kernel or not. But in
> the subsystem we internally use ENOTSUPP.
I will change it, sorry, checkpatch warning confused me.
> > > > + rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
> > > > + (reply[6] << 24);
> > > > + falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
> > > > + (reply[7] << 24);
> > >
> > > With a help of two masks, you can access to the both edges as to
> > > 64-bit value and simplify the code.
> >
> > Huh? As in
> > rising = reply & 0x00ff00ff00ff00ff;
> > falling = reply & 0xff00ff00ff00ff00;
> > ?
> > But then I can't or the rising bit with the corresponding falling bit
> > to get pending...
> > Or I guess i can with:
> > pending = rising & (pending >> 8);
> >
> > Am I understanding you correctly?
> >
> > But then I would need to store the mask in driver data as a 64-bit
> > value with half the data not used. Also the CPU is 32-bit.
>
> If you use proper bitmaps, perhaps this will be easier. You can use one for
> each and merge them whenever you want (with bitmap_or() call) or split (with
> bitmap_and() respectively):
>
> bitmap_or(full, raising, failing); // merge
> bitmap_and(raising, full, rasing_mask); // split
Hmm. But then what? I or the result and use it as pending interrupt
bitmap, to be iterated over. The indexes of the bits correspond to the
constants in the MCU API.
So after your suggestion I have rising and falling containgin
rising = 00rr00rr00rr00rr; /* r means rising bits */
falling = 00ff00ff00ff00ff; /* f means falling bits */
pending = rising | falling;
which means:
pending = pp00pp00pp00pp; /* p means pending bits */
But these bit positions do not correspond to the interrupt number
anymore.
I still think the de-interleaving of the buffer from
rr ff rr ff rr ff rr ff
into two words:
rising = rrrrrrrr;
falling = ffffffff;
is simpler...
Or am I wrong?
> > > > +void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu)
> > > > +{
> > > > + if (!(mcu->features & FEAT_NEW_INT_API))
> > > > +
> > > > cancel_delayed_work_sync(&mcu->button_release_emul_work); +
> > > > + mutex_destroy(&mcu->lock);
> > >
> > > Wrong order?
> >
> > No, the mutex may be used in the work. Can't destroy it first. Or am I
> > misunderstanding something?
>
> I mean you are using a lot of devm(), can mutex be used in IRQ or whatever
> that can be triggered after this call?
OK, I think I need to free the irq before canceling the work. Thank you!
Marek
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 5/7] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog
2023-09-19 13:50 ` Guenter Roeck
@ 2023-09-22 11:46 ` Marek Behún
0 siblings, 0 replies; 30+ messages in thread
From: Marek Behún @ 2023-09-22 11:46 UTC (permalink / raw)
To: Guenter Roeck
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Wim Van Sebroeck,
linux-watchdog
> > +static int omnia_wdt_start(struct watchdog_device *wdt)
> > +{
> > + struct omnia_mcu *mcu = watchdog_get_drvdata(wdt);
> > +
> > + /*
> > + * The MCU also implements LED controller, and the LED driver may in
> > + * some cases require more I2C bandwidth. In order to prevent stealing
> > + * that bandwidth from the LED driver, configure minimum time between
> > + * pings to 10% of the configured timeout (for 120 second timeout we
> > + * will be pinging at most every 12 seconds).
> > + */
> > + mcu->wdt.min_hw_heartbeat_ms = 100 * mcu->wdt.timeout;
>
> This is not what min_hw_heartbeat_ms is supposed to be used for, and it is
> an abuse of the API which is not guaranteed to work. The value is exopected
> to be set at registration time, and it is not supposed to change dynamically.
> It is also competely unnecessary since no sane watchdog daemon will ping
> at such a high rate. Please drop.
Hello Guenter, thanks for the review.
Sorry about this, I thought this was correct cause rti_wdt driver and
feared that watchdog daemons would actually ping at such rate.
I will drop it in the next version of the series.
Marek
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-21 18:42 ` Marek Behún
@ 2023-09-22 14:14 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2023-09-22 14:14 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio
On Thu, Sep 21, 2023 at 08:42:43PM +0200, Marek Behún wrote:
> On Tue, 19 Sep 2023 16:00:39 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
...
> > > + mutex_lock(&mcu->lock);
> > > +
> > > + if (ctl_mask)
> > > + err = omnia_ctl_cmd_unlocked(mcu, CMD_GENERAL_CONTROL, ctl,
> > > + ctl_mask);
> >
> > > + if (!err && ext_ctl_mask)
> > > + err = omnia_ctl_cmd_unlocked(mcu, CMD_EXT_CONTROL, ext_ctl,
> > > + ext_ctl_mask);
> >
> > Can it be
> >
> > if (err)
> > goto out_unlock;
> >
> > if (_mask)
> > ...
> >
> > ?
>
> Hi Andy,
>
> so I am refactoring this to use guard(mutex), but now I have this:
>
> guard(mutex, &mcu->lock);
>
> if (ctl_mask) {
> err = ...;
> if (err)
> goto out_err;
> }
>
> if (ext_ctl_mask) {
> err = ...;
> if (err)
> goto out_err;
> }
>
> return;
> out_err:
> dev_err(dev, "Cannot set GPIOs: %d\n", err);
>
> which clearly is not any better... or at least the original
...which rather means that the design of above is not so good, i.e.
why do you need the same message in the different situations?
> if (!err && ext_ctl_mask)
> is better IMO.
I disagree.
> Compare with:
>
> guard(mutex, &mcu->lock);
>
> if (ctl_mask)
> err = ...;
>
> if (!err && ext_ctl_mask)
> err = ...;
>
> if (err)
> dev_err(dev, "Cannot set GPIOs: %d\n", err);
>
>
> Do you have a better suggestion?
Use different messages (if even needed) for different situations.
With cleanup.h in place you shouldn't supposed to have goto:s
(in simple cases like yours).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-21 20:25 ` Marek Behún
@ 2023-09-22 14:18 ` Andy Shevchenko
2023-09-25 10:03 ` Marek Behún
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-09-22 14:18 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio
On Thu, Sep 21, 2023 at 10:25:01PM +0200, Marek Behún wrote:
> On Thu, 21 Sep 2023 12:55:08 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
...
> > > > > + rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
> > > > > + (reply[6] << 24);
> > > > > + falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
> > > > > + (reply[7] << 24);
> > > >
> > > > With a help of two masks, you can access to the both edges as to
> > > > 64-bit value and simplify the code.
> > >
> > > Huh? As in
> > > rising = reply & 0x00ff00ff00ff00ff;
> > > falling = reply & 0xff00ff00ff00ff00;
> > > ?
> > > But then I can't or the rising bit with the corresponding falling bit
> > > to get pending...
> > > Or I guess i can with:
> > > pending = rising & (pending >> 8);
> > >
> > > Am I understanding you correctly?
> > >
> > > But then I would need to store the mask in driver data as a 64-bit
> > > value with half the data not used. Also the CPU is 32-bit.
> >
> > If you use proper bitmaps, perhaps this will be easier. You can use one for
> > each and merge them whenever you want (with bitmap_or() call) or split (with
> > bitmap_and() respectively):
> >
> > bitmap_or(full, raising, failing); // merge
> > bitmap_and(raising, full, rasing_mask); // split
>
> Hmm. But then what? I or the result and use it as pending interrupt
> bitmap, to be iterated over. The indexes of the bits correspond to the
> constants in the MCU API.
>
> So after your suggestion I have rising and falling containgin
> rising = 00rr00rr00rr00rr; /* r means rising bits */
> falling = 00ff00ff00ff00ff; /* f means falling bits */
> pending = rising | falling;
> which means:
> pending = pp00pp00pp00pp; /* p means pending bits */
> But these bit positions do not correspond to the interrupt number
> anymore.
>
> I still think the de-interleaving of the buffer from
> rr ff rr ff rr ff rr ff
> into two words:
> rising = rrrrrrrr;
> falling = ffffffff;
> is simpler...
There are two sides of this: OS and hardware. See Xilinx GPIO driver how it's
made there. But before going that way, check on
https://lore.kernel.org/all/ZOMmuZuhdjA6mdIG@smile.fi.intel.com/
That APIs you would need I am pretty sure.
...
> > > > > + if (!(mcu->features & FEAT_NEW_INT_API))
> > > > > +
> > > > > cancel_delayed_work_sync(&mcu->button_release_emul_work); +
> > > > > + mutex_destroy(&mcu->lock);
> > > >
> > > > Wrong order?
> > >
> > > No, the mutex may be used in the work. Can't destroy it first. Or am I
> > > misunderstanding something?
> >
> > I mean you are using a lot of devm(), can mutex be used in IRQ or whatever
> > that can be triggered after this call?
>
> OK, I think I need to free the irq before canceling the work. Thank you!
Can you rather switch everything to be devm managed?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-21 20:14 ` Marek Behún
@ 2023-09-22 14:21 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2023-09-22 14:21 UTC (permalink / raw)
To: Marek Behún
Cc: Linus Walleij, Gregory CLEMENT, Arnd Bergmann, soc, arm,
Bartosz Golaszewski, linux-gpio, Dmitry Torokhov
On Thu, Sep 21, 2023 at 10:14:09PM +0200, Marek Behún wrote:
> On Thu, 21 Sep 2023 21:45:57 +0200
> Marek Behún <kabel@kernel.org> wrote:
>
> > I could use ffs(x) instead of ilog2(x) + 1.
>
> Pardon me, I meant fls(). Or maybe get_bitmask_order() from
> linux/bitops.h.
In any case it's bitops.h APIs that you will need and I think it's fine and
Linus will approve that.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-22 14:18 ` Andy Shevchenko
@ 2023-09-25 10:03 ` Marek Behún
2023-09-25 10:29 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Marek Behún @ 2023-09-25 10:03 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio
On Fri, 22 Sep 2023 17:18:56 +0300
Andy Shevchenko <andy@kernel.org> wrote:
> On Thu, Sep 21, 2023 at 10:25:01PM +0200, Marek Behún wrote:
> > On Thu, 21 Sep 2023 12:55:08 +0300
> > Andy Shevchenko <andy@kernel.org> wrote:
>
> ...
>
> > > > > > + rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
> > > > > > + (reply[6] << 24);
> > > > > > + falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
> > > > > > + (reply[7] << 24);
> > > > >
> > > > > With a help of two masks, you can access to the both edges as to
> > > > > 64-bit value and simplify the code.
> > > >
> > > > Huh? As in
> > > > rising = reply & 0x00ff00ff00ff00ff;
> > > > falling = reply & 0xff00ff00ff00ff00;
> > > > ?
> > > > But then I can't or the rising bit with the corresponding falling bit
> > > > to get pending...
> > > > Or I guess i can with:
> > > > pending = rising & (pending >> 8);
> > > >
> > > > Am I understanding you correctly?
> > > >
> > > > But then I would need to store the mask in driver data as a 64-bit
> > > > value with half the data not used. Also the CPU is 32-bit.
> > >
> > > If you use proper bitmaps, perhaps this will be easier. You can use one for
> > > each and merge them whenever you want (with bitmap_or() call) or split (with
> > > bitmap_and() respectively):
> > >
> > > bitmap_or(full, raising, failing); // merge
> > > bitmap_and(raising, full, rasing_mask); // split
> >
> > Hmm. But then what? I or the result and use it as pending interrupt
> > bitmap, to be iterated over. The indexes of the bits correspond to the
> > constants in the MCU API.
> >
> > So after your suggestion I have rising and falling containgin
> > rising = 00rr00rr00rr00rr; /* r means rising bits */
> > falling = 00ff00ff00ff00ff; /* f means falling bits */
> > pending = rising | falling;
> > which means:
> > pending = pp00pp00pp00pp; /* p means pending bits */
> > But these bit positions do not correspond to the interrupt number
> > anymore.
> >
> > I still think the de-interleaving of the buffer from
> > rr ff rr ff rr ff rr ff
> > into two words:
> > rising = rrrrrrrr;
> > falling = ffffffff;
> > is simpler...
>
> There are two sides of this: OS and hardware. See Xilinx GPIO driver how it's
> made there. But before going that way, check on
> https://lore.kernel.org/all/ZOMmuZuhdjA6mdIG@smile.fi.intel.com/
> That APIs you would need I am pretty sure.
Andy, thank you for patience in reviewing this.
Hmm. I like the names, scatter and gather. In the firmware, I used
interleave and deinterleave, see
https://gitlab.nic.cz/turris/hw/omnia_hw_ctrl/-/blob/master/src/drivers/i2c_iface.c#L360
But those functions work bit-wise. I realize that the I2C transfers in
the driver are so slow that such bit-wise cycling over a bitmap won't
matter much, but I still find my original proposal more simple and
straight-forward. But I will cave if you insist. Please let me know
(and can I then send your local patch in the series?)
> > > > > > + if (!(mcu->features & FEAT_NEW_INT_API))
> > > > > > +
> > > > > > cancel_delayed_work_sync(&mcu->button_release_emul_work); +
> > > > > > + mutex_destroy(&mcu->lock);
> > > > >
> > > > > Wrong order?
> > > >
> > > > No, the mutex may be used in the work. Can't destroy it first. Or am I
> > > > misunderstanding something?
> > >
> > > I mean you are using a lot of devm(), can mutex be used in IRQ or whatever
> > > that can be triggered after this call?
> >
> > OK, I think I need to free the irq before canceling the work. Thank you!
>
> Can you rather switch everything to be devm managed?
There are no devm_ calls for mutex and work initialization. Are you
suggesting that I should write a release function for the gpio
sub-driver? Something like
static void omnia_gpiochip_release(dev, res)
{
cancel_work();
mutex_destroy();
}
int omnia_mcu_register_gpiochip(mcu)
{
...
x = devres_alloc(omnia_gpiochip_release);
devres_add(dev, x);
...
}
Marek
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
2023-09-25 10:03 ` Marek Behún
@ 2023-09-25 10:29 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2023-09-25 10:29 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio
On Mon, Sep 25, 2023 at 12:03:56PM +0200, Marek Behún wrote:
> On Fri, 22 Sep 2023 17:18:56 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Thu, Sep 21, 2023 at 10:25:01PM +0200, Marek Behún wrote:
> > > On Thu, 21 Sep 2023 12:55:08 +0300
> > > Andy Shevchenko <andy@kernel.org> wrote:
...
> > > > > > > + rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
> > > > > > > + (reply[6] << 24);
> > > > > > > + falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
> > > > > > > + (reply[7] << 24);
> > > > > >
> > > > > > With a help of two masks, you can access to the both edges as to
> > > > > > 64-bit value and simplify the code.
> > > > >
> > > > > Huh? As in
> > > > > rising = reply & 0x00ff00ff00ff00ff;
> > > > > falling = reply & 0xff00ff00ff00ff00;
> > > > > ?
> > > > > But then I can't or the rising bit with the corresponding falling bit
> > > > > to get pending...
> > > > > Or I guess i can with:
> > > > > pending = rising & (pending >> 8);
> > > > >
> > > > > Am I understanding you correctly?
> > > > >
> > > > > But then I would need to store the mask in driver data as a 64-bit
> > > > > value with half the data not used. Also the CPU is 32-bit.
> > > >
> > > > If you use proper bitmaps, perhaps this will be easier. You can use one for
> > > > each and merge them whenever you want (with bitmap_or() call) or split (with
> > > > bitmap_and() respectively):
> > > >
> > > > bitmap_or(full, raising, failing); // merge
> > > > bitmap_and(raising, full, rasing_mask); // split
> > >
> > > Hmm. But then what? I or the result and use it as pending interrupt
> > > bitmap, to be iterated over. The indexes of the bits correspond to the
> > > constants in the MCU API.
> > >
> > > So after your suggestion I have rising and falling containgin
> > > rising = 00rr00rr00rr00rr; /* r means rising bits */
> > > falling = 00ff00ff00ff00ff; /* f means falling bits */
> > > pending = rising | falling;
> > > which means:
> > > pending = pp00pp00pp00pp; /* p means pending bits */
> > > But these bit positions do not correspond to the interrupt number
> > > anymore.
> > >
> > > I still think the de-interleaving of the buffer from
> > > rr ff rr ff rr ff rr ff
> > > into two words:
> > > rising = rrrrrrrr;
> > > falling = ffffffff;
> > > is simpler...
> >
> > There are two sides of this: OS and hardware. See Xilinx GPIO driver how it's
> > made there. But before going that way, check on
> > https://lore.kernel.org/all/ZOMmuZuhdjA6mdIG@smile.fi.intel.com/
> > That APIs you would need I am pretty sure.
>
> Andy, thank you for patience in reviewing this.
>
> Hmm. I like the names, scatter and gather. In the firmware, I used
> interleave and deinterleave, see
> https://gitlab.nic.cz/turris/hw/omnia_hw_ctrl/-/blob/master/src/drivers/i2c_iface.c#L360
>
> But those functions work bit-wise. I realize that the I2C transfers in
> the driver are so slow that such bit-wise cycling over a bitmap won't
> matter much, but I still find my original proposal more simple and
> straight-forward. But I will cave if you insist. Please let me know
> (and can I then send your local patch in the series?)
You can. but I need to add test cases there.
Yes, I think the best is to have hardware values and Linux cached ones
to be separated. Let me try my best and send it out this week.
...
> > > > > > > + if (!(mcu->features & FEAT_NEW_INT_API))
> > > > > > > +
> > > > > > > cancel_delayed_work_sync(&mcu->button_release_emul_work); +
> > > > > > > + mutex_destroy(&mcu->lock);
> > > > > >
> > > > > > Wrong order?
> > > > >
> > > > > No, the mutex may be used in the work. Can't destroy it first. Or am I
> > > > > misunderstanding something?
> > > >
> > > > I mean you are using a lot of devm(), can mutex be used in IRQ or whatever
> > > > that can be triggered after this call?
> > >
> > > OK, I think I need to free the irq before canceling the work. Thank you!
> >
> > Can you rather switch everything to be devm managed?
>
> There are no devm_ calls for mutex and work initialization. Are you
> suggesting that I should write a release function for the gpio
> sub-driver? Something like
>
> static void omnia_gpiochip_release(dev, res)
> {
> cancel_work();
> mutex_destroy();
> }
Not together, but
- for mutex use devm_add_action_or_reset() as done in many other drivers
for the same reason;
- for the work we have devm_work_autocancel()
(you need to include devm-helpers.h)
> int omnia_mcu_register_gpiochip(mcu)
> {
> ...
> x = devres_alloc(omnia_gpiochip_release);
> devres_add(dev, x);
> ...
> }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2023-09-25 10:30 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-19 10:38 [PATCH v2 0/7] Turris Omnia MCU driver Marek Behún
2023-09-19 10:38 ` [PATCH v2 1/7] dt-bindings: arm: add cznic,turris-omnia-mcu binding Marek Behún
2023-09-20 12:37 ` Krzysztof Kozlowski
2023-09-19 10:38 ` [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU Marek Behún
2023-09-19 12:29 ` Andy Shevchenko
2023-09-19 15:16 ` Marek Behún
2023-09-19 18:27 ` Andy Shevchenko
2023-09-20 14:19 ` Marek Behún
2023-09-20 14:47 ` Andy Shevchenko
2023-09-19 10:38 ` [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs Marek Behún
2023-09-19 13:00 ` Andy Shevchenko
2023-09-20 17:08 ` Marek Behún
2023-09-21 9:55 ` Andy Shevchenko
2023-09-21 20:25 ` Marek Behún
2023-09-22 14:18 ` Andy Shevchenko
2023-09-25 10:03 ` Marek Behún
2023-09-25 10:29 ` Andy Shevchenko
2023-09-21 18:42 ` Marek Behún
2023-09-22 14:14 ` Andy Shevchenko
2023-09-20 11:58 ` Linus Walleij
2023-09-20 13:36 ` Andy Shevchenko
2023-09-21 19:45 ` Marek Behún
2023-09-21 20:14 ` Marek Behún
2023-09-22 14:21 ` Andy Shevchenko
2023-09-19 10:38 ` [PATCH v2 4/7] platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup Marek Behún
2023-09-19 10:38 ` [PATCH v2 5/7] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog Marek Behún
2023-09-19 13:50 ` Guenter Roeck
2023-09-22 11:46 ` Marek Behún
2023-09-19 10:38 ` [PATCH v2 6/7] ARM: dts: turris-omnia: Add MCU system-controller node Marek Behún
2023-09-19 10:38 ` [PATCH v2 7/7] ARM: dts: turris-omnia: Add GPIO key node for front button 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).