[RFC PATCH v1 0/2] platform: toradex: Add toradex embedded controller

[RFC PATCH v1 0/2] platform: toradex: Add toradex embedded controller

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

This series adds support for the Toradex Embedded Controller, currently used
on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future.

The EC provides board power-off, reset and GPIO expander functionalities.

Sending it as an RFC to gather initial feedback on it before investing more
time in testing and adding the remaining functionalities, with that said both
the code and the binding are in condition to be wholly reviewed.

Emanuele Ghidoli (2):
  dt-bindings: firmware: add toradex,embedded-controller
  platform: toradex: add preliminary support for Embedded Controller

 .../firmware/toradex,embedded-controller.yaml |  44 +++++
 MAINTAINERS                                   |   7 +
 drivers/platform/Kconfig                      |   2 +
 drivers/platform/Makefile                     |   1 +
 drivers/platform/toradex/Kconfig              |  18 ++
 drivers/platform/toradex/Makefile             |   1 +
 drivers/platform/toradex/toradex-ec.c         | 155 ++++++++++++++++++
 7 files changed, 228 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/toradex,embedded-controller.yaml
 create mode 100644 drivers/platform/toradex/Kconfig
 create mode 100644 drivers/platform/toradex/Makefile
 create mode 100644 drivers/platform/toradex/toradex-ec.c

-- 
2.39.5

[RFC PATCH v1 1/2] dt-bindings: firmware: add toradex,embedded-controller

[Francesco Dolcini]

From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

The Toradex Embedded Controller serves as a system controller, providing
system power-off and restart functionalities
The two variants, Smarc iMX95 and Smarc iMX8P, have a compatible I2C interface.
Besides this, different compatible values are defined to allow for
future implementation differences.

Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 .../firmware/toradex,embedded-controller.yaml | 44 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/toradex,embedded-controller.yaml

diff --git a/Documentation/devicetree/bindings/firmware/toradex,embedded-controller.yaml b/Documentation/devicetree/bindings/firmware/toradex,embedded-controller.yaml
new file mode 100644
index 000000000000..626e5e653c0d
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/toradex,embedded-controller.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/firmware/toradex,embedded-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Toradex Embedded Controller
+
+maintainers:
+  - Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
+  - Francesco Dolcini <francesco.dolcini@toradex.com>
+
+description:
+  The Toradex Embedded Controller acts as a system controller which provides
+  system power off and restart.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - toradex,smarc-imx95-ec
+          - toradex,smarc-imx8mp-ec
+      - const: toradex,embedded-controller
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        embedded-controller@28 {
+            compatible = "toradex,smarc-imx95-ec", "toradex,embedded-controller";
+            reg = <0x28>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index ed7aa6867674..19d7c17c0115 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23972,6 +23972,12 @@ L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
 F:	drivers/platform/x86/topstar-laptop.c
 
+TORADEX EMBEDDED CONTROLLER DRIVER
+M:	Emanuele Ghidoli <ghidoliemanuele@gmail.com>
+M:	Francesco Dolcini <francesco@dolcini.it>
+S:	Maintained
+F:	Documentation/devicetree/bindings/firmware/toradex,embedded-controller.yaml
+
 TORTURE-TEST MODULES
 M:	Davidlohr Bueso <dave@stgolabs.net>
 M:	"Paul E. McKenney" <paulmck@kernel.org>
-- 
2.39.5

[RFC PATCH v1 2/2] platform: toradex: add preliminary support for Embedded Controller

[Francesco Dolcini]

From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

Add new platform driver for the Embedded Controller currently used
in Toradex SMARC iMX8MP and SMARC iMX95.
It currently provides power-off and restart (reset) handlers.

Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 MAINTAINERS                           |   1 +
 drivers/platform/Kconfig              |   2 +
 drivers/platform/Makefile             |   1 +
 drivers/platform/toradex/Kconfig      |  18 +++
 drivers/platform/toradex/Makefile     |   1 +
 drivers/platform/toradex/toradex-ec.c | 155 ++++++++++++++++++++++++++
 6 files changed, 178 insertions(+)
 create mode 100644 drivers/platform/toradex/Kconfig
 create mode 100644 drivers/platform/toradex/Makefile
 create mode 100644 drivers/platform/toradex/toradex-ec.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 19d7c17c0115..fa1630f0f725 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23977,6 +23977,7 @@ M:	Emanuele Ghidoli <ghidoliemanuele@gmail.com>
 M:	Francesco Dolcini <francesco@dolcini.it>
 S:	Maintained
 F:	Documentation/devicetree/bindings/firmware/toradex,embedded-controller.yaml
+F:	drivers/platform/toradex/toradex-ec.c
 
 TORTURE-TEST MODULES
 M:	Davidlohr Bueso <dave@stgolabs.net>
diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
index 960fd6a82450..84aabb88fb4a 100644
--- a/drivers/platform/Kconfig
+++ b/drivers/platform/Kconfig
@@ -15,6 +15,8 @@ source "drivers/platform/olpc/Kconfig"
 
 source "drivers/platform/surface/Kconfig"
 
+source "drivers/platform/toradex/Kconfig"
+
 source "drivers/platform/x86/Kconfig"
 
 source "drivers/platform/arm64/Kconfig"
diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile
index 19ac54648586..2d849a8f3ccf 100644
--- a/drivers/platform/Makefile
+++ b/drivers/platform/Makefile
@@ -13,3 +13,4 @@ obj-$(CONFIG_CHROME_PLATFORMS)	+= chrome/
 obj-$(CONFIG_CZNIC_PLATFORMS)	+= cznic/
 obj-$(CONFIG_SURFACE_PLATFORMS)	+= surface/
 obj-$(CONFIG_ARM64_PLATFORM_DEVICES)	+= arm64/
+obj-$(CONFIG_TORADEX_PLATFORMS)	+= toradex/
diff --git a/drivers/platform/toradex/Kconfig b/drivers/platform/toradex/Kconfig
new file mode 100644
index 000000000000..635d955df79d
--- /dev/null
+++ b/drivers/platform/toradex/Kconfig
@@ -0,0 +1,18 @@
+menuconfig TORADEX_PLATFORMS
+	bool "Platform support for Toradex hardware"
+	help
+	  Say Y here to be able to choose driver support for Toradex
+	  devices. This option alone does not add any kernel code.
+
+if TORADEX_PLATFORMS
+config TORADEX_EC
+	tristate "Toradex Embedded Controller driver"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say Y here to add support for the features implemented by the
+	  Embedded Controller on the Toradex Modules.
+	  To compile this driver as a module, choose M here; the module will be
+	  called toradex-ec.
+
+endif # TORADEX_PLATFORMS
diff --git a/drivers/platform/toradex/Makefile b/drivers/platform/toradex/Makefile
new file mode 100644
index 000000000000..eb918819ad35
--- /dev/null
+++ b/drivers/platform/toradex/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_TORADEX_EC)			+= toradex-ec.o
diff --git a/drivers/platform/toradex/toradex-ec.c b/drivers/platform/toradex/toradex-ec.c
new file mode 100644
index 000000000000..f01addf2a0a6
--- /dev/null
+++ b/drivers/platform/toradex/toradex-ec.c
@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Toradex Embedded Controller driver
+ *
+ * Copyright (C) 2025 Toradex
+ *
+ * Author: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/reboot.h>
+#include <linux/regmap.h>
+
+#define EC_CHIP_ID_REG                  0x00
+#define EC_VERSION_REG_MAJOR            0x01
+#define EC_VERSION_REG_MINOR            0x02
+#define EC_CMD_REG                      0xD0
+#define EC_REG_MAX                      0xD0
+
+#define EC_CHIP_ID_SMARC_IMX95          0x11
+#define EC_CHIP_ID_SMARC_IMX8MP         0x12
+
+#define EC_POWEROFF_CMD                 0x01
+#define EC_RESET_CMD                    0x02
+
+#define EC_ID_VERSION_LEN               3
+
+struct tdx_ec {
+	struct i2c_client *client;
+	struct regmap *regmap;
+};
+
+static const struct regmap_range volatile_ranges[] = {
+	regmap_reg_range(EC_CMD_REG, EC_CMD_REG),
+};
+
+static const struct regmap_access_table volatile_table = {
+	.yes_ranges	= volatile_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(volatile_ranges),
+};
+
+static const struct regmap_range read_ranges[] = {
+	regmap_reg_range(EC_CHIP_ID_REG, EC_VERSION_REG_MINOR),
+};
+
+static const struct regmap_access_table read_table = {
+	.yes_ranges	= read_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(read_ranges),
+};
+
+static const struct regmap_config regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= EC_REG_MAX,
+	.cache_type	= REGCACHE_RBTREE,
+	.rd_table	= &read_table,
+	.volatile_table = &volatile_table,
+};
+
+static int tdx_ec_cmd(struct tdx_ec *ec, u8 cmd)
+{
+	int err = regmap_write(ec->regmap, EC_CMD_REG, cmd);
+
+	if (err)
+		dev_err(&ec->client->dev, "Failed to send command 0x%02X: %d\n", cmd, err);
+	return err;
+}
+
+static int tdx_ec_power_off(struct sys_off_data *data)
+{
+	struct tdx_ec *ec = data->cb_data;
+	int err;
+
+	err = tdx_ec_cmd(ec, EC_POWEROFF_CMD);
+	return err ? NOTIFY_BAD : NOTIFY_DONE;
+}
+
+static int tdx_ec_restart(struct sys_off_data *data)
+{
+	struct tdx_ec *ec = data->cb_data;
+	int err;
+
+	err = tdx_ec_cmd(ec, EC_RESET_CMD);
+	return err ? NOTIFY_BAD : NOTIFY_DONE;
+}
+
+static int tdx_ec_register_power_off_restart(struct device *dev, struct tdx_ec *ec)
+{
+	int err;
+
+	err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART,
+					    SYS_OFF_PRIO_FIRMWARE,
+					    tdx_ec_restart, ec);
+	if (err)
+		return err;
+
+	err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF,
+					    SYS_OFF_PRIO_FIRMWARE,
+					    tdx_ec_power_off, ec);
+	return err;
+}
+
+static int tdx_ec_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	u8 reg_val[EC_ID_VERSION_LEN];
+	struct tdx_ec *ec;
+	int err;
+
+	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
+	if (!ec)
+		return -ENOMEM;
+
+	ec->client = client;
+	i2c_set_clientdata(client, ec);
+
+	ec->regmap = devm_regmap_init_i2c(client, &regmap_config);
+	if (IS_ERR(ec->regmap))
+		return PTR_ERR(ec->regmap);
+
+	err = regmap_bulk_read(ec->regmap, EC_CHIP_ID_REG, &reg_val, EC_ID_VERSION_LEN);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "Cannot read id and version registers\n");
+
+	dev_info(dev, "Toradex Embedded Controller id %x - Firmware %d.%d\n",
+		 reg_val[0], reg_val[1], reg_val[2]);
+
+	err = tdx_ec_register_power_off_restart(dev, ec);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "Cannot register system restart handler\n");
+
+	return 0;
+}
+
+static const struct of_device_id __maybe_unused of_tdx_ec_match[] = {
+	{ .compatible = "toradex,embedded-controller" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_tdx_ec_match);
+
+static struct i2c_driver toradex_ec_driver = {
+	.probe			= tdx_ec_probe,
+	.driver			= {
+		.name		= "toradex-ec",
+		.of_match_table = of_tdx_ec_match,
+	},
+};
+module_i2c_driver(toradex_ec_driver);
+
+MODULE_AUTHOR("Emanuele Ghidoli <emanuele.ghidoli@toradex.com>");
+MODULE_DESCRIPTION("Toradex Embedded Controller driver");
+MODULE_LICENSE("GPL");
-- 
2.39.5

Re: [RFC PATCH v1 2/2] platform: toradex: add preliminary support for Embedded Controller

[Andy Shevchenko]

On Thu, Mar 13, 2025 at 03:43:31PM +0100, Francesco Dolcini wrote:

> Add new platform driver for the Embedded Controller currently used
> in Toradex SMARC iMX8MP and SMARC iMX95.
> It currently provides power-off and restart (reset) handlers.

...

+ array_size.h
+ device.h
+ err.h

> +#include <linux/i2c.h>

+ mod_devicetable,h

> +#include <linux/module.h>
> +#include <linux/reboot.h>
> +#include <linux/regmap.h>

+ types.h

...

> +#define EC_CHIP_ID_REG                  0x00
> +#define EC_VERSION_REG_MAJOR            0x01
> +#define EC_VERSION_REG_MINOR            0x02
> +#define EC_CMD_REG                      0xD0
> +#define EC_REG_MAX                      0xD0
> +
> +#define EC_CHIP_ID_SMARC_IMX95          0x11
> +#define EC_CHIP_ID_SMARC_IMX8MP         0x12
> +
> +#define EC_POWEROFF_CMD                 0x01
> +#define EC_RESET_CMD                    0x02

Can you interleave the register offsets with the values in them, so we can
easily see the relationship?

...

> +struct tdx_ec {

> +	struct i2c_client *client;

Why do you need this? What for?..

> +	struct regmap *regmap;

...note, the device pointer you may retrieve from the regmap.

> +};

...

> +static int tdx_ec_register_power_off_restart(struct device *dev, struct tdx_ec *ec)
> +{
> +	int err;
> +
> +	err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART,
> +					    SYS_OFF_PRIO_FIRMWARE,
> +					    tdx_ec_restart, ec);
> +	if (err)
> +		return err;

> +	err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF,
> +					    SYS_OFF_PRIO_FIRMWARE,
> +					    tdx_ec_power_off, ec);
> +	return err;

	return devm_...

> +}

...

> +static int tdx_ec_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	u8 reg_val[EC_ID_VERSION_LEN];
> +	struct tdx_ec *ec;
> +	int err;
> +
> +	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> +	if (!ec)
> +		return -ENOMEM;
> +
> +	ec->client = client;

> +	i2c_set_clientdata(client, ec);

What for?

> +	ec->regmap = devm_regmap_init_i2c(client, &regmap_config);
> +	if (IS_ERR(ec->regmap))
> +		return PTR_ERR(ec->regmap);
> +
> +	err = regmap_bulk_read(ec->regmap, EC_CHIP_ID_REG, &reg_val, EC_ID_VERSION_LEN);
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "Cannot read id and version registers\n");
> +
> +	dev_info(dev, "Toradex Embedded Controller id %x - Firmware %d.%d\n",

Specifiers are semirandom. Why signed? Why x and not %u?

> +		 reg_val[0], reg_val[1], reg_val[2]);

> +	err = tdx_ec_register_power_off_restart(dev, ec);
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "Cannot register system restart handler\n");
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 2/2] platform: toradex: add preliminary support for Embedded Controller

[Francesco Dolcini]

Hello Andy,
thanks for the review.

On Thu, Mar 13, 2025 at 04:53:35PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 13, 2025 at 03:43:31PM +0100, Francesco Dolcini wrote:
> 
> > Add new platform driver for the Embedded Controller currently used
> > in Toradex SMARC iMX8MP and SMARC iMX95.
> > It currently provides power-off and restart (reset) handlers.

...

> > +	err = regmap_bulk_read(ec->regmap, EC_CHIP_ID_REG, &reg_val, EC_ID_VERSION_LEN);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "Cannot read id and version registers\n");
> > +
> > +	dev_info(dev, "Toradex Embedded Controller id %x - Firmware %d.%d\n",
> 
> Specifiers are semirandom. Why signed? Why x and not %u?

The firmware version ("Firmware %d.%d") is two unsigned decimal number,
so yes, I will change to "Firmware %u.%u".

The ID is just an identifier that is documented as hex, therefore I
think that the most convenient way to display it is as a hex number.

Francesco

Re: [RFC PATCH v1 2/2] platform: toradex: add preliminary support for Embedded Controller

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 09:39:11AM +0100, Francesco Dolcini wrote:
> On Thu, Mar 13, 2025 at 04:53:35PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 13, 2025 at 03:43:31PM +0100, Francesco Dolcini wrote:

...

> > > +	err = regmap_bulk_read(ec->regmap, EC_CHIP_ID_REG, &reg_val, EC_ID_VERSION_LEN);
> > > +	if (err)
> > > +		return dev_err_probe(dev, err,
> > > +				     "Cannot read id and version registers\n");
> > > +
> > > +	dev_info(dev, "Toradex Embedded Controller id %x - Firmware %d.%d\n",
> > 
> > Specifiers are semirandom. Why signed? Why x and not %u?
> 
> The firmware version ("Firmware %d.%d") is two unsigned decimal number,
> so yes, I will change to "Firmware %u.%u".
> 
> The ID is just an identifier that is documented as hex, therefore I
> think that the most convenient way to display it is as a hex number.

Thanks, don't forget to summarize this in the commit message, it seems
useful information.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 0/2] platform: toradex: Add toradex embedded controller

[Andy Shevchenko]

On Thu, Mar 13, 2025 at 03:43:29PM +0100, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> This series adds support for the Toradex Embedded Controller, currently used
> on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future.

How many do you have that will come with like 99% guarantee?

> The EC provides board power-off, reset and GPIO expander functionalities.
> 
> Sending it as an RFC to gather initial feedback on it before investing more
> time in testing and adding the remaining functionalities, with that said both
> the code and the binding are in condition to be wholly reviewed.

This doesn't explain why you need a separate folder.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 0/2] platform: toradex: Add toradex embedded controller

[Francesco Dolcini]

On Thu, Mar 13, 2025 at 04:54:59PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 13, 2025 at 03:43:29PM +0100, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > This series adds support for the Toradex Embedded Controller, currently used
> > on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future.
> 
> How many do you have that will come with like 99% guarantee?

None? What I know is that

 - Toradex is building SoM since 20 years and not planning to stop any
   time soon
 - Recently we decided to get into the SMARC market
 - From an engineering side we decided that it was the best decision to
   have a small microcontroller to act as an embedded controller, sitting
   between the SoC and the PMIC, handling reset, power-up/down sequence,
   strapping options, I/Os and potentially more
 - In our roadmap we have more SMARC based products planned
 - The firmware interface is designed to be generic and handle future
   boards
 - We expect to use the same driver for any upcoming board using such
   embedded controller, the EC firmware and the DT will be the only
   differences.

But, I do not have a third product in my hands now, so, I have no such a
thing as 99% guarantee. Honestly I have only one thing in my future with
such a high probability ;-)

> > The EC provides board power-off, reset and GPIO expander functionalities.
> > 
> > Sending it as an RFC to gather initial feedback on it before investing more
> > time in testing and adding the remaining functionalities, with that said both
> > the code and the binding are in condition to be wholly reviewed.
> 
> This doesn't explain why you need a separate folder.

Can you be more specific here? You mean the `toradex` directory, within
`drivers/platform`? The only reason is that maybe we'll break the driver
in multiple files, but we can as well just get rid of it. We did not
think much at it.

BTW, the idea to have this driver in such a way was partially inspired
by drivers/platform/cznic/, that was merged a few months ago.

Francesco

Re: [RFC PATCH v1 0/2] platform: toradex: Add toradex embedded controller

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 10:31:27AM +0100, Francesco Dolcini wrote:
> On Thu, Mar 13, 2025 at 04:54:59PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 13, 2025 at 03:43:29PM +0100, Francesco Dolcini wrote:
> > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > 
> > > This series adds support for the Toradex Embedded Controller, currently used
> > > on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future.
> > 
> > How many do you have that will come with like 99% guarantee?
> 
> None? What I know is that
> 
>  - Toradex is building SoM since 20 years and not planning to stop any
>    time soon
>  - Recently we decided to get into the SMARC market
>  - From an engineering side we decided that it was the best decision to
>    have a small microcontroller to act as an embedded controller, sitting
>    between the SoC and the PMIC, handling reset, power-up/down sequence,
>    strapping options, I/Os and potentially more
>  - In our roadmap we have more SMARC based products planned
>  - The firmware interface is designed to be generic and handle future
>    boards
>  - We expect to use the same driver for any upcoming board using such
>    embedded controller, the EC firmware and the DT will be the only
>    differences.
> 
> But, I do not have a third product in my hands now, so, I have no such a
> thing as 99% guarantee. Honestly I have only one thing in my future with
> such a high probability ;-)

So, perhaps it is not a good start for a brand new folder right now
as we might not see any new products.

> > > The EC provides board power-off, reset and GPIO expander functionalities.
> > > 
> > > Sending it as an RFC to gather initial feedback on it before investing more
> > > time in testing and adding the remaining functionalities, with that said both
> > > the code and the binding are in condition to be wholly reviewed.
> > 
> > This doesn't explain why you need a separate folder.
> 
> Can you be more specific here? You mean the `toradex` directory, within
> `drivers/platform`? The only reason is that maybe we'll break the driver
> in multiple files, but we can as well just get rid of it. We did not
> think much at it.

Yes, kinda. I mean the folder in par of, e.g., Chrome OS related HW.

> BTW, the idea to have this driver in such a way was partially inspired
> by drivers/platform/cznic/, that was merged a few months ago.

Ah, I see. They have already 3 devices on the market.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 0/2] platform: toradex: Add toradex embedded controller

[Hans de Goede]

Hi Francesco,

On 13-Mar-25 3:43 PM, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> This series adds support for the Toradex Embedded Controller, currently used
> on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future.
> 
> The EC provides board power-off, reset and GPIO expander functionalities.
> 
> Sending it as an RFC to gather initial feedback on it before investing more
> time in testing and adding the remaining functionalities, with that said both
> the code and the binding are in condition to be wholly reviewed.
> 
> Emanuele Ghidoli (2):
>   dt-bindings: firmware: add toradex,embedded-controller
>   platform: toradex: add preliminary support for Embedded Controller

Thank you for your patches.

2 remarks, as Andy already hinted at drivers/platform/arm64/ likely
is a better location for this.

But as the commit first adding that directory indicates:

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=363c8aea25728604537b170a1cc24e2f46844896

The reason for having ARM EC drivers there is that these are for
x86-pc-like laptops with all the typical laptops bells and whistles
like EC handled battery charging limits / spk/mic mute-leds built
into keys on the keyboards. Special key handling (like mute, kbd
backlight) done by the EC etc.

Since all the experience for dealing with those laptop-esque features
and exporting them to userspace with a consistent userspace API is
in hands of the maintainers of drivers/platform/x86 it was decided to
add a new drivers/platform/arm64 directory maintained by the same folks.

If this EC driver's only functionality is: "The EC provides board
power-off, reset and GPIO expander functionalities." I'm not sure
that drivers/platform/arm64 is the best place for this.

Also you mention GPIO expander, but that does not seem to be
supported yet? The GPIO functionality really should be in its
own GPIO driver. So it seems to me that what would be a better
fit here would be:

1. A drivers/mfd/ MFD driver with the regmap stuff,
   registering "board-reset" and "gpio" cells

2. A drivers/power/reset/ driver for the "board-reset" cell,
quoting from the Kconfig help text for that dir:

menuconfig POWER_RESET
        bool "Board level reset or power off"
        help
          Provides a number of drivers which either reset a complete board
          or shut it down, by manipulating the main power supply on the board.

          Say Y here to enable board reset and power off

which seems to match exactly with your current EC driver functionality

3. A drivers/gpio/ driver to expose the GPIOs using the standard
GPIO framework, this can be added later in a follow up patch.

Regards,

Hans

Re: [RFC PATCH v1 0/2] platform: toradex: Add toradex embedded controller

[Francesco Dolcini]

Hello Hans,
first thanks for the feedback.

On Thu, Mar 13, 2025 at 04:08:14PM +0100, Hans de Goede wrote:
> On 13-Mar-25 3:43 PM, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > This series adds support for the Toradex Embedded Controller, currently used
> > on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future.
> > 
> > The EC provides board power-off, reset and GPIO expander functionalities.
> > 
> > Sending it as an RFC to gather initial feedback on it before investing more
> > time in testing and adding the remaining functionalities, with that said both
> > the code and the binding are in condition to be wholly reviewed.
> > 
> > Emanuele Ghidoli (2):
> >   dt-bindings: firmware: add toradex,embedded-controller
> >   platform: toradex: add preliminary support for Embedded Controller
> 
> Thank you for your patches.
> 
> 2 remarks, as Andy already hinted at drivers/platform/arm64/ likely
> is a better location for this.

Ack.

This driver is not going to be specific of ARM64, but today we have only
ARM64 systems that would benefit from it. We might as well use it on a
RISCV based SoM in a few years.

With that said we'll move it there, we can always move it out if
anything changes on this regard.

> The reason for having ARM EC drivers there is that these are for
> x86-pc-like laptops with all the typical laptops bells and whistles
> like EC handled battery charging limits / spk/mic mute-leds built
> into keys on the keyboards. Special key handling (like mute, kbd
> backlight) done by the EC etc.
> 
> Since all the experience for dealing with those laptop-esque features
> and exporting them to userspace with a consistent userspace API is
> in hands of the maintainers of drivers/platform/x86 it was decided to
> add a new drivers/platform/arm64 directory maintained by the same folks.
> 
> If this EC driver's only functionality is: "The EC provides board
> power-off, reset and GPIO expander functionalities." I'm not sure
> that drivers/platform/arm64 is the best place for this.

The directory decision / architecture was mainly inspired by
`drivers/platform/cznic`.

This EC is used on a SMARC SoM [1][2], so we are not talking about a
laptop nor a device with a keyboard.

But we do have a power button, a LID switch, some handling and
coordination of low power mode and more.
This device is between the SOC, the PMIC, and various IOs used for
low-power, power-up/down, boot configuration (selecting the boot
device), ...

The short term goal is just the 2 basic functionalities mentioned in
the cover letter available to the driver:
 - power/reset (already implemented)
 - GPIO (working on it as we speak)

Starting small, and adding features when/if required.

> Also you mention GPIO expander, but that does not seem to be
> supported yet?
Correct, coming soon.

> 1. A drivers/mfd/ MFD driver with the regmap stuff,
>    registering "board-reset" and "gpio" cells

So, we considered the idea of going with an MFD driver, but looking at
drivers/platform/cznic, that is doing something relatively close to what
we are doing (just more feature rich, as of now), drivers/platform/
seemed a better fit.

I am not 100% sure what's Andy opinion on this topic, from what I can
understand his concerns are about the toradex directory (that we'll get
rid of), not the drivers/platform/ parent you are concerned about.

Francesco

Re: [RFC PATCH v1 0/2] platform: toradex: Add toradex embedded controller

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 11:08:56AM +0100, Francesco Dolcini wrote:
> On Thu, Mar 13, 2025 at 04:08:14PM +0100, Hans de Goede wrote:
> > On 13-Mar-25 3:43 PM, Francesco Dolcini wrote:

...

> > 1. A drivers/mfd/ MFD driver with the regmap stuff,
> >    registering "board-reset" and "gpio" cells
> 
> So, we considered the idea of going with an MFD driver, but looking at
> drivers/platform/cznic, that is doing something relatively close to what
> we are doing (just more feature rich, as of now), drivers/platform/
> seemed a better fit.
> 
> I am not 100% sure what's Andy opinion on this topic, from what I can
> understand his concerns are about the toradex directory (that we'll get
> rid of), not the drivers/platform/ parent you are concerned about.

Yes, my point is to have this inside existing folder whatever you decide
with the maintainers of the respective subsystem to be.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 0/2] platform: toradex: Add toradex embedded controller

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 12:14:33PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 17, 2025 at 11:08:56AM +0100, Francesco Dolcini wrote:
> > On Thu, Mar 13, 2025 at 04:08:14PM +0100, Hans de Goede wrote:
> > > On 13-Mar-25 3:43 PM, Francesco Dolcini wrote:

...

> > > 1. A drivers/mfd/ MFD driver with the regmap stuff,
> > >    registering "board-reset" and "gpio" cells
> > 
> > So, we considered the idea of going with an MFD driver, but looking at
> > drivers/platform/cznic, that is doing something relatively close to what
> > we are doing (just more feature rich, as of now), drivers/platform/
> > seemed a better fit.
> > 
> > I am not 100% sure what's Andy opinion on this topic, from what I can
> > understand his concerns are about the toradex directory (that we'll get
> > rid of), not the drivers/platform/ parent you are concerned about.
> 
> Yes, my point is to have this inside existing folder whatever you decide
> with the maintainers of the respective subsystem to be.

But if you have more than one module, it might be still good to have at some
level, and drivers/platform might be too low for it (too broad).

So, either path/to/foo/toradex.c, or path/to/foo/toradex/*.[ch].


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 0/2] platform: toradex: Add toradex embedded controller

[Hans de Goede]

Hi,

On 17-Mar-25 11:08, Francesco Dolcini wrote:
> Hello Hans,
> first thanks for the feedback.
> 
> On Thu, Mar 13, 2025 at 04:08:14PM +0100, Hans de Goede wrote:
>> On 13-Mar-25 3:43 PM, Francesco Dolcini wrote:
>>> From: Francesco Dolcini <francesco.dolcini@toradex.com>
>>>
>>> This series adds support for the Toradex Embedded Controller, currently used
>>> on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future.
>>>
>>> The EC provides board power-off, reset and GPIO expander functionalities.
>>>
>>> Sending it as an RFC to gather initial feedback on it before investing more
>>> time in testing and adding the remaining functionalities, with that said both
>>> the code and the binding are in condition to be wholly reviewed.
>>>
>>> Emanuele Ghidoli (2):
>>>   dt-bindings: firmware: add toradex,embedded-controller
>>>   platform: toradex: add preliminary support for Embedded Controller
>>
>> Thank you for your patches.
>>
>> 2 remarks, as Andy already hinted at drivers/platform/arm64/ likely
>> is a better location for this.
> 
> Ack.
> 
> This driver is not going to be specific of ARM64, but today we have only
> ARM64 systems that would benefit from it. We might as well use it on a
> RISCV based SoM in a few years.
> 
> With that said we'll move it there, we can always move it out if
> anything changes on this regard.
> 
>> The reason for having ARM EC drivers there is that these are for
>> x86-pc-like laptops with all the typical laptops bells and whistles
>> like EC handled battery charging limits / spk/mic mute-leds built
>> into keys on the keyboards. Special key handling (like mute, kbd
>> backlight) done by the EC etc.
>>
>> Since all the experience for dealing with those laptop-esque features
>> and exporting them to userspace with a consistent userspace API is
>> in hands of the maintainers of drivers/platform/x86 it was decided to
>> add a new drivers/platform/arm64 directory maintained by the same folks.
>>
>> If this EC driver's only functionality is: "The EC provides board
>> power-off, reset and GPIO expander functionalities." I'm not sure
>> that drivers/platform/arm64 is the best place for this.
> 
> The directory decision / architecture was mainly inspired by
> `drivers/platform/cznic`.
> 
> This EC is used on a SMARC SoM [1][2], so we are not talking about a
> laptop nor a device with a keyboard.
> 
> But we do have a power button, a LID switch, some handling and
> coordination of low power mode and more.
> This device is between the SOC, the PMIC, and various IOs used for
> low-power, power-up/down, boot configuration (selecting the boot
> device), ...
> 
> The short term goal is just the 2 basic functionalities mentioned in
> the cover letter available to the driver:
>  - power/reset (already implemented)
>  - GPIO (working on it as we speak)
> 
> Starting small, and adding features when/if required.
> 
>> Also you mention GPIO expander, but that does not seem to be
>> supported yet?
> Correct, coming soon.
> 
>> 1. A drivers/mfd/ MFD driver with the regmap stuff,
>>    registering "board-reset" and "gpio" cells
> 
> So, we considered the idea of going with an MFD driver, but looking at
> drivers/platform/cznic, that is doing something relatively close to what
> we are doing (just more feature rich, as of now), drivers/platform/
> seemed a better fit.

So looking at drivers/platform/cznic this does not look like a good
example how to do things. I see multiple sub-drivers each in their
own file, but it is all directly linked using symbols from the modules
to each other.

The cznic code looks like a classic example where a MFD approach
creating child devices for each functions and having separate
drivers bind to each function would be a much cleaner way to
do things, especially since they now also have #ifdef's to allow
disabling some functions where as with the MFD approach one can
simply e.g. not build the GPIO driver at all.

Especially if you plan to re-use the baseboard / EC with possible
SOMs of a different architecture, in which case drivers/platform/aarch64
also seems like a bad fit.

The example of the MFD approach is that e.g. your GPIO driver can live
under drivers/gpio and thus will be reviewed by the GPIO subsystem
maintainers benefiting from all their experience with GPIO drivers.

But if Andy and Ilpo are happy to take this as a more monolithic
driver under drivers/platform/aarch64 I'm not going to nack that
approach.

Regards,

Hans


 

Re: [RFC PATCH v1 0/2] platform: toradex: Add toradex embedded controller

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 11:39:14AM +0100, Hans de Goede wrote:
> On 17-Mar-25 11:08, Francesco Dolcini wrote:

...

> But if Andy and Ilpo are happy to take this as a more monolithic
> driver under drivers/platform/aarch64 I'm not going to nack that
> approach.

I'm okay with the choice as long as it's suffice the purpose.
I agree that aarch64 maybe not a good choice after all, but
we should start from somewhere. Later on we can move to agnostic
folder if needed. The question here is more about MFD/not-MFD.
If the former becomes the case, it would need to be under drivers/mfd
as Lee asked for that (or even required).

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 0/2] platform: toradex: Add toradex embedded controller

[Hans de Goede]

Hi,

On 17-Mar-25 13:06, Andy Shevchenko wrote:
> On Mon, Mar 17, 2025 at 11:39:14AM +0100, Hans de Goede wrote:
>> On 17-Mar-25 11:08, Francesco Dolcini wrote:
> 
> ...
> 
>> But if Andy and Ilpo are happy to take this as a more monolithic
>> driver under drivers/platform/aarch64 I'm not going to nack that
>> approach.
> 
> I'm okay with the choice as long as it's suffice the purpose.
> I agree that aarch64 maybe not a good choice after all, but
> we should start from somewhere. Later on we can move to agnostic
> folder if needed. The question here is more about MFD/not-MFD.
> If the former becomes the case, it would need to be under drivers/mfd
> as Lee asked for that (or even required).

Right if we go with MFD (which IMHO would be the cleaner approach)
then the base driver registering the regmap + cells / child devices
would live under drivers/mfd, there reboot/shutdown driver under
drivers/power/reset/ , gpio under drivers/gpio and any new
functionalities in the correct places for those functionalities.

Regards,

Hans