[PATCH 0/4] usb: typec: ucsi: add support for stm32g0

[PATCH 0/4] usb: typec: ucsi: add support for stm32g0

[Fabrice Gasnier]

STM32G0 provides an integrated USB Type-C and power delivery interface [1].
It can be programmed with a firmware [2] to act as a PPM. Currently it
implements UCSI protocol over I2C interface. A GPIO is used as an interrupt
line.

This series adds a driver to support it, including:
- dt-bindings documentation
- optional STM32G0 firmware control and update, over a secondary I2C address
- power management

[1] https://wiki.st.com/stm32mcu/wiki/Introduction_to_USB_Power_Delivery_with_STM32
[2] https://github.com/STMicroelectronics/x-cube-ucsi

Fabrice Gasnier (4):
  dt-bindings: usb: typec: add bindings for stm32g0 controller
  usb: typec: ucsi: stm32g0: add support for stm32g0 i2c controller
  usb: typec: ucsi: stm32g0: add bootloader support
  usb: typec: ucsi: stm32g0: add support for power management

 .../bindings/usb/st,typec-stm32g0.yaml        |  83 ++
 drivers/usb/typec/ucsi/Kconfig                |  10 +
 drivers/usb/typec/ucsi/Makefile               |   1 +
 drivers/usb/typec/ucsi/ucsi_stm32g0.c         | 777 ++++++++++++++++++
 4 files changed, 871 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
 create mode 100644 drivers/usb/typec/ucsi/ucsi_stm32g0.c

-- 
2.25.1

[PATCH 1/4] dt-bindings: usb: typec: add bindings for stm32g0 controller

[Fabrice Gasnier]

This patch adds DT schema documentation for the STM32G0 Type-C controller.
STM32G0 provides an integrated USB Type-C and power delivery interface.
It can be programmed with a firmware to handle UCSI protocol over I2C
interface. A GPIO is used as an interrupt line.
It may be used as a wakeup source, so use optional "wakeup-source" and
"power-domains" properties to support wakeup.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 .../bindings/usb/st,typec-stm32g0.yaml        | 83 +++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml

diff --git a/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
new file mode 100644
index 0000000000000..b2729bd015a1a
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
@@ -0,0 +1,83 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/usb/st,typec-stm32g0.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: STMicroelectronics STM32G0 Type-C controller bindings
+
+description: |
+  The STM32G0 MCU can be programmed to control Type-C connector(s) through I2C
+  typically using the UCSI protocol over I2C, with a dedicated alert
+  (interrupt) pin.
+
+maintainers:
+  - Fabrice Gasnier <fabrice.gasnier@foss.st.com>
+
+properties:
+  compatible:
+    const: st,stm32g0-typec
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  connector:
+    type: object
+    allOf:
+      - $ref: ../connector/usb-connector.yaml#
+
+  firmware-name:
+    description: |
+      Should contain the name of the default firmware image
+      file located on the firmware search path
+
+  wakeup-source: true
+  power-domains: true
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c5 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      stm32g0@53 {
+        compatible = "st,stm32g0-typec";
+        reg = <0x53>;
+        /* Alert pin on GPIO PE12 */
+        interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
+        interrupt-parent = <&gpioe>;
+
+        /* Example with one type-C connector */
+        connector {
+          compatible = "usb-c-connector";
+          label = "USB-C";
+
+          port {
+            con_usb_c_ep: endpoint {
+              remote-endpoint = <&usbotg_hs_ep>;
+            };
+          };
+        };
+      };
+    };
+
+    usbotg_hs {
+      usb-role-switch;
+      port {
+        usbotg_hs_ep: endpoint {
+          remote-endpoint = <&con_usb_c_ep>;
+        };
+      };
+    };
+...
-- 
2.25.1

[PATCH 2/4] usb: typec: ucsi: stm32g0: add support for stm32g0 i2c controller

[Fabrice Gasnier]

STM32G0 provides an integrated USB Type-C and power delivery interface.
It can be programmed with a firmware to handle UCSI protocol over I2C
interface. A GPIO is used as an interrupt line.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/usb/typec/ucsi/Kconfig        |  10 ++
 drivers/usb/typec/ucsi/Makefile       |   1 +
 drivers/usb/typec/ucsi/ucsi_stm32g0.c | 218 ++++++++++++++++++++++++++
 3 files changed, 229 insertions(+)
 create mode 100644 drivers/usb/typec/ucsi/ucsi_stm32g0.c

diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
index 5e9b37b3f25e1..8f9c4b9f31f79 100644
--- a/drivers/usb/typec/ucsi/Kconfig
+++ b/drivers/usb/typec/ucsi/Kconfig
@@ -48,4 +48,14 @@ config UCSI_ACPI
 	  To compile the driver as a module, choose M here: the module will be
 	  called ucsi_acpi
 
+config UCSI_STM32G0
+	tristate "UCSI Interface Driver for STM32G0"
+	depends on I2C
+	help
+	  This driver enables UCSI support on platforms that expose a STM32G0
+	  Type-C controller over I2C interface.
+
+	  To compile the driver as a module, choose M here: the module will be
+	  called ucsi_stm32g0.
+
 endif
diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
index 8a8eb5cb8e0f0..480d533d762fe 100644
--- a/drivers/usb/typec/ucsi/Makefile
+++ b/drivers/usb/typec/ucsi/Makefile
@@ -17,3 +17,4 @@ endif
 
 obj-$(CONFIG_UCSI_ACPI)			+= ucsi_acpi.o
 obj-$(CONFIG_UCSI_CCG)			+= ucsi_ccg.o
+obj-$(CONFIG_UCSI_STM32G0)		+= ucsi_stm32g0.o
diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
new file mode 100644
index 0000000000000..d1f22cee8c6c9
--- /dev/null
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -0,0 +1,218 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+/*
+ * UCSI driver for STMicroelectronics STM32G0 Type-C controller
+ *
+ * Copyright (C) 2022, STMicroelectronics - All Rights Reserved
+ * Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>.
+ */
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include "ucsi.h"
+
+struct ucsi_stm32g0 {
+	struct i2c_client *client;
+	struct completion complete;
+	struct device *dev;
+	unsigned long flags;
+	struct ucsi *ucsi;
+};
+
+static int ucsi_stm32g0_read(struct ucsi *ucsi, unsigned int offset, void *val, size_t len)
+{
+	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+	struct i2c_client *client = g0->client;
+	u8 reg = offset;
+	struct i2c_msg msg[] = {
+		{
+			.addr	= client->addr,
+			.flags  = 0,
+			.len	= 1,
+			.buf	= &reg,
+		},
+		{
+			.addr	= client->addr,
+			.flags  = I2C_M_RD,
+			.len	= len,
+			.buf	= val,
+		},
+	};
+	int ret;
+
+	ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
+	if (ret != ARRAY_SIZE(msg)) {
+		dev_err(g0->dev, "i2c read %02x, %02x error: %d\n", client->addr, reg, ret);
+
+		return ret < 0 ? ret : -EIO;
+	}
+
+	return 0;
+}
+
+static int ucsi_stm32g0_async_write(struct ucsi *ucsi, unsigned int offset, const void *val,
+				    size_t len)
+{
+	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+	struct i2c_client *client = g0->client;
+	struct i2c_msg msg[] = {
+		{
+			.addr	= client->addr,
+			.flags  = 0,
+		}
+	};
+	unsigned char *buf;
+	int ret;
+
+	buf = kzalloc(len + 1, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	buf[0] = offset;
+	memcpy(&buf[1], val, len);
+	msg[0].len = len + 1;
+	msg[0].buf = buf;
+
+	ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
+	kfree(buf);
+	if (ret != ARRAY_SIZE(msg)) {
+		dev_err(g0->dev, "i2c write %02x, %02x error: %d\n", client->addr, buf[0], ret);
+
+		return ret < 0 ? ret : -EIO;
+	}
+
+	return 0;
+}
+
+static int ucsi_stm32g0_sync_write(struct ucsi *ucsi, unsigned int offset, const void *val,
+				   size_t len)
+{
+	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+	int ret;
+
+	set_bit(COMMAND_PENDING, &g0->flags);
+
+	ret = ucsi_stm32g0_async_write(ucsi, offset, val, len);
+	if (ret)
+		goto out_clear_bit;
+
+	if (!wait_for_completion_timeout(&g0->complete, msecs_to_jiffies(5000)))
+		ret = -ETIMEDOUT;
+
+out_clear_bit:
+	clear_bit(COMMAND_PENDING, &g0->flags);
+
+	return ret;
+}
+
+static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
+{
+	struct ucsi_stm32g0 *g0 = data;
+	u32 cci;
+	int ret;
+
+	ret = ucsi_stm32g0_read(g0->ucsi, UCSI_CCI, &cci, sizeof(cci));
+	if (ret)
+		return IRQ_NONE;
+
+	if (UCSI_CCI_CONNECTOR(cci))
+		ucsi_connector_change(g0->ucsi, UCSI_CCI_CONNECTOR(cci));
+
+	if (test_bit(COMMAND_PENDING, &g0->flags) &&
+	    cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))
+		complete(&g0->complete);
+
+	return IRQ_HANDLED;
+}
+
+static const struct ucsi_operations ucsi_stm32g0_ops = {
+	.read = ucsi_stm32g0_read,
+	.sync_write = ucsi_stm32g0_sync_write,
+	.async_write = ucsi_stm32g0_async_write,
+};
+
+static int ucsi_stm32g0_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct ucsi_stm32g0 *g0;
+	int ret;
+
+	g0 = devm_kzalloc(dev, sizeof(*g0), GFP_KERNEL);
+	if (!g0)
+		return -ENOMEM;
+
+	g0->dev = dev;
+	g0->client = client;
+	init_completion(&g0->complete);
+	i2c_set_clientdata(client, g0);
+
+	g0->ucsi = ucsi_create(dev, &ucsi_stm32g0_ops);
+	if (IS_ERR(g0->ucsi))
+		return PTR_ERR(g0->ucsi);
+
+	ucsi_set_drvdata(g0->ucsi, g0);
+
+	/* Request alert interrupt */
+	ret = request_threaded_irq(client->irq, NULL, ucsi_stm32g0_irq_handler, IRQF_ONESHOT,
+				   dev_name(&client->dev), g0);
+	if (ret) {
+		dev_err_probe(dev, ret, "request IRQ failed\n");
+		goto destroy;
+	}
+
+	ret = ucsi_register(g0->ucsi);
+	if (ret) {
+		dev_err_probe(dev, ret, "ucsi_register failed\n");
+		goto freeirq;
+	}
+
+	return 0;
+
+freeirq:
+	free_irq(client->irq, g0);
+destroy:
+	ucsi_destroy(g0->ucsi);
+
+	return ret;
+}
+
+static int ucsi_stm32g0_remove(struct i2c_client *client)
+{
+	struct ucsi_stm32g0 *g0 = i2c_get_clientdata(client);
+
+	ucsi_unregister(g0->ucsi);
+	free_irq(client->irq, g0);
+	ucsi_destroy(g0->ucsi);
+
+	return 0;
+}
+
+static const struct of_device_id __maybe_unused ucsi_stm32g0_typec_of_match[] = {
+	{ .compatible = "st,stm32g0-typec" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ucsi_stm32g0_typec_of_match);
+
+static const struct i2c_device_id ucsi_stm32g0_typec_i2c_devid[] = {
+	{"stm32g0-typec", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, ucsi_stm32g0_typec_i2c_devid);
+
+static struct i2c_driver ucsi_stm32g0_i2c_driver = {
+	.driver = {
+		.name = "ucsi-stm32g0-i2c",
+		.of_match_table = of_match_ptr(ucsi_stm32g0_typec_of_match),
+	},
+	.probe = ucsi_stm32g0_probe,
+	.remove = ucsi_stm32g0_remove,
+	.id_table = ucsi_stm32g0_typec_i2c_devid
+};
+module_i2c_driver(ucsi_stm32g0_i2c_driver);
+
+MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@foss.st.com>");
+MODULE_DESCRIPTION("STMicroelectronics STM32G0 Type-C controller");
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_ALIAS("platform:ucsi-stm32g0");
-- 
2.25.1

[PATCH 3/4] usb: typec: ucsi: stm32g0: add bootloader support

[Fabrice Gasnier]

STM32G0 comes with STM32 bootloader in its system memory. Add support
for some I2C bootloader commands as described in application notes
AN2606 and AN4221, to enable STM32G0 UCSI firmware update.

Upon probing, the driver needs to know the STM32G0 state:
- In bootloader mode, STM32 G0 answers at i2c addr 0x51.
- In running mode, STM32 G0 firmware may answer at two address.
  - The main address specified in DT is used for UCSI.
  - 0x51 addr can be re-used for FW controls like getting software version
    or jump to booloader request.

So probe using the main firmware i2c address first, before attempting
bootloader address (e.g. check for blank, erased or previously aborted
firmware update).

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/usb/typec/ucsi/ucsi_stm32g0.c | 533 +++++++++++++++++++++++++-
 1 file changed, 520 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
index d1f22cee8c6c9..91222f4e07aee 100644
--- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -6,21 +6,324 @@
  * Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>.
  */
 
+#include <linux/delay.h>
+#include <linux/firmware.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <asm/unaligned.h>
 
 #include "ucsi.h"
 
+/* STM32G0 I2C bootloader addr: 0b1010001x (See AN2606) */
+#define STM32G0_I2C_BL_ADDR	(0xa2 >> 1)
+
+/* STM32G0 I2C bootloader max data size */
+#define STM32G0_I2C_BL_SZ	256
+
+/* STM32 I2C bootloader commands (See AN4221) */
+#define STM32_CMD_GVR		0x01	/* Gets the bootloader version */
+#define STM32_CMD_GVR_LEN	1
+#define STM32_CMD_RM		0x11	/* Reag memory */
+#define STM32_CMD_WM		0x31	/* Write memory */
+#define STM32_CMD_ADDR_LEN	5	/* Address len for go, mem write... */
+#define STM32_CMD_ERASE		0x44	/* Erase page, bank or all */
+#define STM32_CMD_ERASE_SPECIAL_LEN	3
+#define STM32_CMD_GLOBAL_MASS_ERASE	0xffff /* All-bank erase */
+
+/* STM32 I2C bootloader answer status */
+#define STM32G0_I2C_BL_ACK	0x79
+#define STM32G0_I2C_BL_NACK	0x1f
+#define STM32G0_I2C_BL_BUSY	0x76
+
+/* STM32G0 flash definitions */
+#define STM32G0_USER_OPTION_BYTES	0x1fff7800
+#define STM32G0_USER_OB_NBOOT0		BIT(26)
+#define STM32G0_USER_OB_NBOOT_SEL	BIT(24)
+#define STM32G0_USER_OB_BOOT_MAIN	(STM32G0_USER_OB_NBOOT0 | STM32G0_USER_OB_NBOOT_SEL)
+#define STM32G0_MAIN_MEM_ADDR		0x08000000
+
+/* STM32 Firmware definitions: additional commands */
+#define STM32G0_FW_GETVER	0x00	/* Gets the firmware version */
+#define STM32G0_FW_GETVER_LEN	4
+#define STM32G0_FW_RSTGOBL	0x21	/* Reset and go to bootloader */
+#define STM32G0_FW_KEYWORD	0xa56959a6
+
+/* ucsi_stm32g0_fw_info located at the end of the firmware */
+struct ucsi_stm32g0_fw_info {
+	u32 version;
+	u32 keyword;
+};
+
 struct ucsi_stm32g0 {
 	struct i2c_client *client;
+	struct i2c_client *i2c_bl;
+	bool in_bootloader;
+	u8 bl_version;
 	struct completion complete;
 	struct device *dev;
 	unsigned long flags;
+	const char *fw_name;
 	struct ucsi *ucsi;
 };
 
+/*
+ * Bootloader commands helpers:
+ * - send command (2 bytes)
+ * - check ack
+ * Then either:
+ * - receive data
+ * - receive data + check ack
+ * - send data + check ack
+ * These operations depends on the command and have various length.
+ */
+static int ucsi_stm32g0_bl_check_ack(struct ucsi *ucsi)
+{
+	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+	struct i2c_client *client = g0->i2c_bl;
+	unsigned char ack;
+	struct i2c_msg msg[] = {
+		{
+			.addr	= client->addr,
+			.flags  = I2C_M_RD,
+			.len	= 1,
+			.buf	= &ack,
+		},
+	};
+	int ret;
+
+	ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
+	if (ret != ARRAY_SIZE(msg)) {
+		dev_err(g0->dev, "i2c bl ack (%02x), error: %d\n", client->addr, ret);
+
+		return ret < 0 ? ret : -EIO;
+	}
+
+	/* The 'ack' byte should contain bootloader answer: ack/nack/busy */
+	switch (ack) {
+	case STM32G0_I2C_BL_ACK:
+		return 0;
+	case STM32G0_I2C_BL_NACK:
+		return -ENOENT;
+	case STM32G0_I2C_BL_BUSY:
+		return -EBUSY;
+	default:
+		dev_err(g0->dev, "i2c bl ack (%02x), invalid byte: %02x\n",
+			client->addr, ack);
+		return -EINVAL;
+	}
+}
+
+static int ucsi_stm32g0_bl_cmd_check_ack(struct ucsi *ucsi, unsigned int cmd, bool check_ack)
+{
+	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+	struct i2c_client *client = g0->i2c_bl;
+	unsigned char buf[2];
+	struct i2c_msg msg[] = {
+		{
+			.addr	= client->addr,
+			.flags  = 0,
+			.len	= sizeof(buf),
+			.buf	= buf,
+		},
+	};
+	int ret;
+
+	/*
+	 * Send STM32 bootloader command format is two bytes:
+	 * - command code
+	 * - XOR'ed command code
+	 */
+	buf[0] = cmd;
+	buf[1] = cmd ^ 0xff;
+
+	ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
+	if (ret != ARRAY_SIZE(msg)) {
+		dev_dbg(g0->dev, "i2c bl cmd %d (%02x), error: %d\n", cmd, client->addr, ret);
+
+		return ret < 0 ? ret : -EIO;
+	}
+
+	if (check_ack)
+		return ucsi_stm32g0_bl_check_ack(ucsi);
+
+	return 0;
+}
+
+static int ucsi_stm32g0_bl_cmd(struct ucsi *ucsi, unsigned int cmd)
+{
+	return ucsi_stm32g0_bl_cmd_check_ack(ucsi, cmd, true);
+}
+
+static int ucsi_stm32g0_bl_rcv_check_ack(struct ucsi *ucsi, void *data, size_t len, bool check_ack)
+{
+	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+	struct i2c_client *client = g0->i2c_bl;
+	struct i2c_msg msg[] = {
+		{
+			.addr	= client->addr,
+			.flags  = I2C_M_RD,
+			.len	= len,
+			.buf	= data,
+		},
+	};
+	int ret;
+
+	ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
+	if (ret != ARRAY_SIZE(msg)) {
+		dev_err(g0->dev, "i2c bl rcv %02x, error: %d\n", client->addr, ret);
+
+		return ret < 0 ? ret : -EIO;
+	}
+
+	if (check_ack)
+		return ucsi_stm32g0_bl_check_ack(ucsi);
+
+	return 0;
+}
+
+static int ucsi_stm32g0_bl_rcv(struct ucsi *ucsi, void *data, size_t len)
+{
+	return ucsi_stm32g0_bl_rcv_check_ack(ucsi, data, len, true);
+}
+
+static int ucsi_stm32g0_bl_rcv_woack(struct ucsi *ucsi, void *data, size_t len)
+{
+	return ucsi_stm32g0_bl_rcv_check_ack(ucsi, data, len, false);
+}
+
+static int ucsi_stm32g0_bl_send(struct ucsi *ucsi, void *data, size_t len)
+{
+	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+	struct i2c_client *client = g0->i2c_bl;
+	struct i2c_msg msg[] = {
+		{
+			.addr	= client->addr,
+			.flags  = 0,
+			.len	= len,
+			.buf	= data,
+		},
+	};
+	int ret;
+
+	ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
+	if (ret != ARRAY_SIZE(msg)) {
+		dev_err(g0->dev, "i2c bl send %02x, error: %d\n", client->addr, ret);
+
+		return ret < 0 ? ret : -EIO;
+	}
+
+	return ucsi_stm32g0_bl_check_ack(ucsi);
+}
+
+/* Bootloader commands */
+static int ucsi_stm32g0_bl_get_version(struct ucsi *ucsi, u8 *bl_version)
+{
+	int ret;
+
+	ret = ucsi_stm32g0_bl_cmd(ucsi, STM32_CMD_GVR);
+	if (ret)
+		return ret;
+
+	return ucsi_stm32g0_bl_rcv(ucsi, bl_version, STM32_CMD_GVR_LEN);
+}
+
+static int ucsi_stm32g0_bl_send_addr(struct ucsi *ucsi, u32 addr)
+{
+	u8 data8[STM32_CMD_ADDR_LEN];
+
+	/* Address format: 4 bytes addr (MSB first) + XOR'ed addr bytes */
+	put_unaligned_be32(addr, data8);
+	data8[4] = data8[0] ^ data8[1] ^ data8[2] ^ data8[3];
+
+	return ucsi_stm32g0_bl_send(ucsi, data8, STM32_CMD_ADDR_LEN);
+}
+
+static int ucsi_stm32g0_bl_global_mass_erase(struct ucsi *ucsi)
+{
+	u8 data8[4];
+	u16 *data16 = (u16 *)&data8[0];
+	int ret;
+
+	data16[0] = STM32_CMD_GLOBAL_MASS_ERASE;
+	data8[2] = data8[0] ^ data8[1];
+
+	ret = ucsi_stm32g0_bl_cmd(ucsi, STM32_CMD_ERASE);
+	if (ret)
+		return ret;
+
+	return ucsi_stm32g0_bl_send(ucsi, data8, STM32_CMD_ERASE_SPECIAL_LEN);
+}
+
+static int ucsi_stm32g0_bl_write(struct ucsi *ucsi, u32 addr, const void *data, size_t len)
+{
+	u8 *data8;
+	int i, ret;
+
+	if (!len || len > STM32G0_I2C_BL_SZ)
+		return -EINVAL;
+
+	/* Write memory: len bytes -1, data up to 256 bytes + XOR'ed bytes */
+	data8 = kzalloc(STM32G0_I2C_BL_SZ + 2, GFP_KERNEL);
+	if (!data8)
+		return -ENOMEM;
+
+	ret = ucsi_stm32g0_bl_cmd(ucsi, STM32_CMD_WM);
+	if (ret)
+		goto free;
+
+	ret = ucsi_stm32g0_bl_send_addr(ucsi, addr);
+	if (ret)
+		goto free;
+
+	data8[0] = len - 1;
+	memcpy(data8 + 1, data, len);
+	data8[len + 1] = data8[0];
+	for (i = 1; i <= len; i++)
+		data8[len + 1] ^= data8[i];
+
+	ret = ucsi_stm32g0_bl_send(ucsi, data8, len + 2);
+free:
+	kfree(data8);
+
+	return ret;
+}
+
+static int ucsi_stm32g0_bl_read(struct ucsi *ucsi, u32 addr, void *data, size_t len)
+{
+	int ret;
+
+	if (!len || len > STM32G0_I2C_BL_SZ)
+		return -EINVAL;
+
+	ret = ucsi_stm32g0_bl_cmd(ucsi, STM32_CMD_RM);
+	if (ret)
+		return ret;
+
+	ret = ucsi_stm32g0_bl_send_addr(ucsi, addr);
+	if (ret)
+		return ret;
+
+	ret = ucsi_stm32g0_bl_cmd(ucsi, len - 1);
+	if (ret)
+		return ret;
+
+	return ucsi_stm32g0_bl_rcv_woack(ucsi, data, len);
+}
+
+/* Firmware commands (the same address as the bootloader) */
+static int ucsi_stm32g0_fw_cmd(struct ucsi *ucsi, unsigned int cmd)
+{
+	return ucsi_stm32g0_bl_cmd_check_ack(ucsi, cmd, false);
+}
+
+static int ucsi_stm32g0_fw_rcv(struct ucsi *ucsi, void *data, size_t len)
+{
+	return ucsi_stm32g0_bl_rcv_woack(ucsi, data, len);
+}
+
+/* UCSI ops */
 static int ucsi_stm32g0_read(struct ucsi *ucsi, unsigned int offset, void *val, size_t len)
 {
 	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
@@ -133,6 +436,191 @@ static const struct ucsi_operations ucsi_stm32g0_ops = {
 	.async_write = ucsi_stm32g0_async_write,
 };
 
+static int ucsi_stm32g0_register(struct ucsi *ucsi)
+{
+	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+	struct i2c_client *client = g0->client;
+	int ret;
+
+	/* Request alert interrupt */
+	ret = request_threaded_irq(client->irq, NULL, ucsi_stm32g0_irq_handler, IRQF_ONESHOT,
+				   dev_name(g0->dev), g0);
+	if (ret) {
+		dev_err(g0->dev, "request IRQ failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = ucsi_register(ucsi);
+	if (ret) {
+		dev_err_probe(g0->dev, ret, "ucsi_register failed\n");
+		free_irq(client->irq, g0);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void ucsi_stm32g0_unregister(struct ucsi *ucsi)
+{
+	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+	struct i2c_client *client = g0->client;
+
+	ucsi_unregister(ucsi);
+	free_irq(client->irq, g0);
+}
+
+static void ucsi_stm32g0_fw_cb(const struct firmware *fw, void *context)
+{
+	struct ucsi_stm32g0 *g0;
+	const u8 *data, *end;
+	const struct ucsi_stm32g0_fw_info *fw_info;
+	u32 addr = STM32G0_MAIN_MEM_ADDR, ob, fw_version;
+	int ret, size;
+
+	if (!context)
+		return;
+
+	g0 = ucsi_get_drvdata(context);
+
+	if (!fw)
+		goto fw_release;
+
+	fw_info = (struct ucsi_stm32g0_fw_info *)(fw->data + fw->size - sizeof(*fw_info));
+
+	if (!g0->in_bootloader) {
+		/* Read running firmware version */
+		ret = ucsi_stm32g0_fw_cmd(g0->ucsi, STM32G0_FW_GETVER);
+		if (ret) {
+			dev_err(g0->dev, "Get version cmd failed %d\n", ret);
+			goto fw_release;
+		}
+		ret = ucsi_stm32g0_fw_rcv(g0->ucsi, &fw_version,
+					  STM32G0_FW_GETVER_LEN);
+		if (ret) {
+			dev_err(g0->dev, "Get version failed %d\n", ret);
+			goto fw_release;
+		}
+
+		/* Sanity check on keyword and firmware version */
+		if (fw_info->keyword != STM32G0_FW_KEYWORD || fw_info->version == fw_version)
+			goto fw_release;
+
+		dev_info(g0->dev, "Flashing FW: %08x (%08x cur)\n", fw_info->version, fw_version);
+
+		/* Switch to bootloader mode */
+		ucsi_stm32g0_unregister(g0->ucsi);
+		ret = ucsi_stm32g0_fw_cmd(g0->ucsi, STM32G0_FW_RSTGOBL);
+		if (ret) {
+			dev_err(g0->dev, "bootloader cmd failed %d\n", ret);
+			goto fw_release;
+		}
+		g0->in_bootloader = true;
+
+		/* STM32G0 reboot delay */
+		msleep(100);
+	}
+
+	ret = ucsi_stm32g0_bl_global_mass_erase(g0->ucsi);
+	if (ret) {
+		dev_err(g0->dev, "Erase failed %d\n", ret);
+		goto fw_release;
+	}
+
+	data = fw->data;
+	end = fw->data + fw->size;
+	while (data < end) {
+		if ((end - data) < STM32G0_I2C_BL_SZ)
+			size = end - data;
+		else
+			size = STM32G0_I2C_BL_SZ;
+
+		ret = ucsi_stm32g0_bl_write(g0->ucsi, addr, data, size);
+		if (ret) {
+			dev_err(g0->dev, "Write failed %d\n", ret);
+			goto fw_release;
+		}
+		addr += size;
+		data += size;
+	}
+
+	dev_dbg(g0->dev, "Configure to boot from main flash\n");
+
+	ret = ucsi_stm32g0_bl_read(g0->ucsi, STM32G0_USER_OPTION_BYTES, &ob, sizeof(ob));
+	if (ret) {
+		dev_err(g0->dev, "read user option bytes failed %d\n", ret);
+		goto fw_release;
+	}
+
+	dev_dbg(g0->dev, "STM32G0_USER_OPTION_BYTES 0x%08x\n", ob);
+
+	/* Configure user option bytes to boot from main flash next time */
+	ob |= STM32G0_USER_OB_BOOT_MAIN;
+
+	/* Writing option bytes will also reset G0 for updates to be loaded */
+	ret = ucsi_stm32g0_bl_write(g0->ucsi, STM32G0_USER_OPTION_BYTES, &ob, sizeof(ob));
+	if (ret) {
+		dev_err(g0->dev, "write user option bytes failed %d\n", ret);
+		goto fw_release;
+	}
+
+	dev_info(g0->dev, "Starting, option bytes:0x%08x\n", ob);
+
+	/* STM32G0 FW boot delay */
+	msleep(500);
+
+	/* Register UCSI interface */
+	if (!ucsi_stm32g0_register(g0->ucsi))
+		g0->in_bootloader = false;
+
+fw_release:
+	release_firmware(fw);
+}
+
+static int ucsi_stm32g0_probe_bootloader(struct ucsi *ucsi)
+{
+	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+	int ret;
+	u16 ucsi_version;
+
+	/* firmware-name is optional */
+	if (device_property_present(g0->dev, "firmware-name")) {
+		ret = device_property_read_string(g0->dev, "firmware-name", &g0->fw_name);
+		if (ret < 0)
+			return dev_err_probe(g0->dev, ret, "Error reading firmware-name\n");
+	}
+
+	if (g0->fw_name) {
+		/* STM32G0 in bootloader mode communicates at reserved address 0x51 */
+		g0->i2c_bl = i2c_new_dummy_device(g0->client->adapter, STM32G0_I2C_BL_ADDR);
+		if (IS_ERR(g0->i2c_bl)) {
+			ret = dev_err_probe(g0->dev, PTR_ERR(g0->i2c_bl),
+					    "Failed to register booloader I2C address\n");
+			return ret;
+		}
+	}
+
+	/*
+	 * Try to guess if the STM32G0 is running a UCSI firmware. First probe the UCSI FW at its
+	 * i2c address. Fallback to bootloader i2c address only if firmware-name is specified.
+	 */
+	ret = ucsi_stm32g0_read(ucsi, UCSI_VERSION, &ucsi_version, sizeof(ucsi_version));
+	if (!ret || !g0->fw_name)
+		return ret;
+
+	/* Speculatively read the bootloader version that has a known length. */
+	ret = ucsi_stm32g0_bl_get_version(ucsi, &g0->bl_version);
+	if (ret < 0) {
+		i2c_unregister_device(g0->i2c_bl);
+		return ret;
+	}
+
+	/* Device in bootloader mode */
+	g0->in_bootloader = true;
+	dev_info(g0->dev, "Bootloader Version 0x%02x\n", g0->bl_version);
+
+	return 0;
+}
+
 static int ucsi_stm32g0_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
@@ -154,24 +642,41 @@ static int ucsi_stm32g0_probe(struct i2c_client *client, const struct i2c_device
 
 	ucsi_set_drvdata(g0->ucsi, g0);
 
-	/* Request alert interrupt */
-	ret = request_threaded_irq(client->irq, NULL, ucsi_stm32g0_irq_handler, IRQF_ONESHOT,
-				   dev_name(&client->dev), g0);
-	if (ret) {
-		dev_err_probe(dev, ret, "request IRQ failed\n");
+	ret = ucsi_stm32g0_probe_bootloader(g0->ucsi);
+	if (ret < 0)
 		goto destroy;
+
+	/*
+	 * Don't register in bootloader mode: wait for the firmware to be loaded and started before
+	 * registering UCSI device.
+	 */
+	if (!g0->in_bootloader) {
+		ret = ucsi_stm32g0_register(g0->ucsi);
+		if (ret < 0)
+			goto freei2c;
 	}
 
-	ret = ucsi_register(g0->ucsi);
-	if (ret) {
-		dev_err_probe(dev, ret, "ucsi_register failed\n");
-		goto freeirq;
+	if (g0->fw_name) {
+		/*
+		 * Asynchronously flash (e.g. bootloader mode) or update the running firmware,
+		 * not to hang the boot process
+		 */
+		ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT, g0->fw_name, g0->dev,
+					      GFP_KERNEL, g0->ucsi, ucsi_stm32g0_fw_cb);
+		if (ret < 0) {
+			dev_err_probe(dev, ret, "firmware request failed\n");
+			goto unregister;
+		}
 	}
 
 	return 0;
 
-freeirq:
-	free_irq(client->irq, g0);
+unregister:
+	if (!g0->in_bootloader)
+		ucsi_stm32g0_unregister(g0->ucsi);
+freei2c:
+	if (g0->fw_name)
+		i2c_unregister_device(g0->i2c_bl);
 destroy:
 	ucsi_destroy(g0->ucsi);
 
@@ -182,8 +687,10 @@ static int ucsi_stm32g0_remove(struct i2c_client *client)
 {
 	struct ucsi_stm32g0 *g0 = i2c_get_clientdata(client);
 
-	ucsi_unregister(g0->ucsi);
-	free_irq(client->irq, g0);
+	if (!g0->in_bootloader)
+		ucsi_stm32g0_unregister(g0->ucsi);
+	if (g0->fw_name)
+		i2c_unregister_device(g0->i2c_bl);
 	ucsi_destroy(g0->ucsi);
 
 	return 0;
-- 
2.25.1

[PATCH 4/4] usb: typec: ucsi: stm32g0: add support for power management

[Fabrice Gasnier]

Type-C connector can be used as a wakeup source (typically to detect
changes on the port, attach or detach...).
Add suspend / resume routines to enable wake irqs, and signal a wakeup
event in case the IRQ has fired while in suspend.
The i2c core is doing the necessary initialization when the "wakeup-source"
flag is provided.
Note: the interrupt handler shouldn't be called before the i2c bus resumes.
So, the interrupts are disabled during suspend period, and re-enabled
upon resume, to avoid i2c transfer while suspended, from the irq handler.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/usb/typec/ucsi/ucsi_stm32g0.c | 52 +++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
index 91222f4e07aee..1c7f4b92eba7c 100644
--- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -66,6 +66,8 @@ struct ucsi_stm32g0 {
 	unsigned long flags;
 	const char *fw_name;
 	struct ucsi *ucsi;
+	bool suspended;
+	bool wakeup_event;
 };
 
 /*
@@ -416,6 +418,9 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
 	u32 cci;
 	int ret;
 
+	if (g0->suspended)
+		g0->wakeup_event = true;
+
 	ret = ucsi_stm32g0_read(g0->ucsi, UCSI_CCI, &cci, sizeof(cci));
 	if (ret)
 		return IRQ_NONE;
@@ -696,6 +701,52 @@ static int ucsi_stm32g0_remove(struct i2c_client *client)
 	return 0;
 }
 
+static int ucsi_stm32g0_suspend(struct device *dev)
+{
+	struct ucsi_stm32g0 *g0 = dev_get_drvdata(dev);
+	struct i2c_client *client = g0->client;
+
+	if (g0->in_bootloader)
+		return 0;
+
+	/* Keep the interrupt disabled until the i2c bus has been resumed */
+	disable_irq(client->irq);
+
+	g0->suspended = true;
+	g0->wakeup_event = false;
+
+	if (device_may_wakeup(dev) || device_wakeup_path(dev))
+		enable_irq_wake(client->irq);
+
+	return 0;
+}
+
+static int ucsi_stm32g0_resume(struct device *dev)
+{
+	struct ucsi_stm32g0 *g0 = dev_get_drvdata(dev);
+	struct i2c_client *client = g0->client;
+
+	if (g0->in_bootloader)
+		return 0;
+
+	if (device_may_wakeup(dev) || device_wakeup_path(dev))
+		disable_irq_wake(client->irq);
+
+	enable_irq(client->irq);
+
+	/* Enforce any pending handler gets called to signal a wakeup_event */
+	synchronize_irq(client->irq);
+
+	if (g0->wakeup_event)
+		pm_wakeup_event(g0->dev, 0);
+
+	g0->suspended = false;
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(ucsi_stm32g0_pm_ops, ucsi_stm32g0_suspend, ucsi_stm32g0_resume);
+
 static const struct of_device_id __maybe_unused ucsi_stm32g0_typec_of_match[] = {
 	{ .compatible = "st,stm32g0-typec" },
 	{},
@@ -712,6 +763,7 @@ static struct i2c_driver ucsi_stm32g0_i2c_driver = {
 	.driver = {
 		.name = "ucsi-stm32g0-i2c",
 		.of_match_table = of_match_ptr(ucsi_stm32g0_typec_of_match),
+		.pm = pm_sleep_ptr(&ucsi_stm32g0_pm_ops),
 	},
 	.probe = ucsi_stm32g0_probe,
 	.remove = ucsi_stm32g0_remove,
-- 
2.25.1

Re: [PATCH 1/4] dt-bindings: usb: typec: add bindings for stm32g0 controller

[Krzysztof Kozlowski]

On 24/06/2022 17:54, Fabrice Gasnier wrote:
> This patch adds DT schema documentation for the STM32G0 Type-C controller.

No "This patch"

https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> STM32G0 provides an integrated USB Type-C and power delivery interface.
> It can be programmed with a firmware to handle UCSI protocol over I2C
> interface. A GPIO is used as an interrupt line.
> It may be used as a wakeup source, so use optional "wakeup-source" and
> "power-domains" properties to support wakeup.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> ---
>  .../bindings/usb/st,typec-stm32g0.yaml        | 83 +++++++++++++++++++
>  1 file changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
> new file mode 100644
> index 0000000000000..b2729bd015a1a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
> @@ -0,0 +1,83 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/usb/st,typec-stm32g0.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

No quotes.

> +
> +title: STMicroelectronics STM32G0 Type-C controller bindings

s/bindings//

> +
> +description: |
> +  The STM32G0 MCU can be programmed to control Type-C connector(s) through I2C
> +  typically using the UCSI protocol over I2C, with a dedicated alert
> +  (interrupt) pin.
> +
> +maintainers:
> +  - Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> +
> +properties:
> +  compatible:
> +    const: st,stm32g0-typec
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  connector:
> +    type: object> +    allOf:
> +      - $ref: ../connector/usb-connector.yaml#

Full path, so /schemas/connector/...

unevaluatedProperties: false

> +
> +  firmware-name:
> +    description: |
> +      Should contain the name of the default firmware image
> +      file located on the firmware search path
> +
> +  wakeup-source: true
> +  power-domains: true

maxItems

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c5 {

Just "i2c"

> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      stm32g0@53 {

Generic node name describing class of the device.

> +        compatible = "st,stm32g0-typec";
> +        reg = <0x53>;
> +        /* Alert pin on GPIO PE12 */
> +        interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
> +        interrupt-parent = <&gpioe>;
> +
> +        /* Example with one type-C connector */
> +        connector {
> +          compatible = "usb-c-connector";
> +          label = "USB-C";
> +
> +          port {

This does not look like proper schema of connector.yaml.

> +            con_usb_c_ep: endpoint {
> +              remote-endpoint = <&usbotg_hs_ep>;
> +            };
> +          };
> +        };
> +      };
> +    };
> +
> +    usbotg_hs {

Generic node names, no underscores in node names.

> +      usb-role-switch;
> +      port {
> +        usbotg_hs_ep: endpoint {
> +          remote-endpoint = <&con_usb_c_ep>;
> +        };
> +      };
> +    };
> +...


Best regards,
Krzysztof

Re: [PATCH 2/4] usb: typec: ucsi: stm32g0: add support for stm32g0 i2c controller

[Christophe JAILLET]

Le 24/06/2022 à 17:54, Fabrice Gasnier a écrit :
> STM32G0 provides an integrated USB Type-C and power delivery interface.
> It can be programmed with a firmware to handle UCSI protocol over I2C
> interface. A GPIO is used as an interrupt line.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier-rj0Iel/JR4NBDgjK7y7TUQ@public.gmane.org>
> ---
>   drivers/usb/typec/ucsi/Kconfig        |  10 ++
>   drivers/usb/typec/ucsi/Makefile       |   1 +
>   drivers/usb/typec/ucsi/ucsi_stm32g0.c | 218 ++++++++++++++++++++++++++
>   3 files changed, 229 insertions(+)
>   create mode 100644 drivers/usb/typec/ucsi/ucsi_stm32g0.c
> 

[...]

> +static int ucsi_stm32g0_async_write(struct ucsi *ucsi, unsigned int offset, const void *val,
> +				    size_t len)
> +{
> +	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
> +	struct i2c_client *client = g0->client;
> +	struct i2c_msg msg[] = {
> +		{
> +			.addr	= client->addr,
> +			.flags  = 0,
> +		}
> +	};
> +	unsigned char *buf;
> +	int ret;
> +
> +	buf = kzalloc(len + 1, GFP_KERNEL);

Hi,

Nit: kmalloc() would be enough here, the whole buffer is written just a 
few lines after.

> +	if (!buf)
> +		return -ENOMEM;
> +
> +	buf[0] = offset;
> +	memcpy(&buf[1], val, len);
> +	msg[0].len = len + 1;
> +	msg[0].buf = buf;
> +
> +	ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> +	kfree(buf);
> +	if (ret != ARRAY_SIZE(msg)) {
> +		dev_err(g0->dev, "i2c write %02x, %02x error: %d\n", client->addr, buf[0], ret);

Use-after-free of buf.

> +
> +		return ret < 0 ? ret : -EIO;
> +	}
> +
> +	return 0;
> +}
> +

Just my 2c,
CJ

[...]

Re: [PATCH 2/4] usb: typec: ucsi: stm32g0: add support for stm32g0 i2c controller

[Heikki Krogerus]

Hi,

On Fri, Jun 24, 2022 at 05:54:11PM +0200, Fabrice Gasnier wrote:
> +static int ucsi_stm32g0_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct ucsi_stm32g0 *g0;
> +	int ret;
> +
> +	g0 = devm_kzalloc(dev, sizeof(*g0), GFP_KERNEL);
> +	if (!g0)
> +		return -ENOMEM;
> +
> +	g0->dev = dev;
> +	g0->client = client;
> +	init_completion(&g0->complete);
> +	i2c_set_clientdata(client, g0);
> +
> +	g0->ucsi = ucsi_create(dev, &ucsi_stm32g0_ops);
> +	if (IS_ERR(g0->ucsi))
> +		return PTR_ERR(g0->ucsi);
> +
> +	ucsi_set_drvdata(g0->ucsi, g0);
> +
> +	/* Request alert interrupt */
> +	ret = request_threaded_irq(client->irq, NULL, ucsi_stm32g0_irq_handler, IRQF_ONESHOT,
> +				   dev_name(&client->dev), g0);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "request IRQ failed\n");
> +		goto destroy;
> +	}
> +
> +	ret = ucsi_register(g0->ucsi);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "ucsi_register failed\n");
> +		goto freeirq;
> +	}

If there isn't UCSI firmware, then ucsi_register() will always safely
fail here, right?


> +	return 0;
> +
> +freeirq:
> +	free_irq(client->irq, g0);
> +destroy:
> +	ucsi_destroy(g0->ucsi);
> +
> +	return ret;
> +}


thanks,

-- 
heikki

Re: [PATCH 1/4] dt-bindings: usb: typec: add bindings for stm32g0 controller

[Fabrice Gasnier]

On 6/24/22 18:16, Krzysztof Kozlowski wrote:
> On 24/06/2022 17:54, Fabrice Gasnier wrote:
>> This patch adds DT schema documentation for the STM32G0 Type-C controller.
> 
> No "This patch"

Hi Krzysztof,

ack,

> 
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> 
>> STM32G0 provides an integrated USB Type-C and power delivery interface.
>> It can be programmed with a firmware to handle UCSI protocol over I2C
>> interface. A GPIO is used as an interrupt line.
>> It may be used as a wakeup source, so use optional "wakeup-source" and
>> "power-domains" properties to support wakeup.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
>> ---
>>  .../bindings/usb/st,typec-stm32g0.yaml        | 83 +++++++++++++++++++
>>  1 file changed, 83 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
>> new file mode 100644
>> index 0000000000000..b2729bd015a1a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
>> @@ -0,0 +1,83 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/usb/st,typec-stm32g0.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> 
> No quotes.

ack,

> 
>> +
>> +title: STMicroelectronics STM32G0 Type-C controller bindings
> 
> s/bindings//

ack,

> 
>> +
>> +description: |
>> +  The STM32G0 MCU can be programmed to control Type-C connector(s) through I2C
>> +  typically using the UCSI protocol over I2C, with a dedicated alert
>> +  (interrupt) pin.
>> +
>> +maintainers:
>> +  - Fabrice Gasnier <fabrice.gasnier@foss.st.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: st,stm32g0-typec
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  connector:
>> +    type: object> +    allOf:
>> +      - $ref: ../connector/usb-connector.yaml#
> 
> Full path, so /schemas/connector/...
> 
> unevaluatedProperties: false

ack,

> 
>> +
>> +  firmware-name:
>> +    description: |
>> +      Should contain the name of the default firmware image
>> +      file located on the firmware search path
>> +
>> +  wakeup-source: true
>> +  power-domains: true
> 
> maxItems

Do you mean maxItems regarding the "power-domains" property ?
This will depend on the user platform, where it's used as an I2C device.
So I'm not sure this can / should be specified here.
Could please you clarify ?

> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    i2c5 {
> 
> Just "i2c"

ack,

> 
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      stm32g0@53 {
> 
> Generic node name describing class of the device.


I wasn't aware of generic node name for an I2C device (not talking of
the controller). I may have missed it.

Could you please clarify ?

> 
>> +        compatible = "st,stm32g0-typec";
>> +        reg = <0x53>;
>> +        /* Alert pin on GPIO PE12 */
>> +        interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
>> +        interrupt-parent = <&gpioe>;
>> +
>> +        /* Example with one type-C connector */
>> +        connector {
>> +          compatible = "usb-c-connector";
>> +          label = "USB-C";
>> +
>> +          port {
> 
> This does not look like proper schema of connector.yaml.

This refers to graph.yaml [1], where similar example is seen [2].

https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L79

https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L207

    device-1 {
        port {
            device_1_output: endpoint {
                remote-endpoint = <&device_2_input>;
            };
        };
    };
    device-2 {
        port {
            device_2_input: endpoint {
                remote-endpoint = <&device_1_output>;
            };
        };
    };


Could you please clarify this point too ?

> 
>> +            con_usb_c_ep: endpoint {
>> +              remote-endpoint = <&usbotg_hs_ep>;
>> +            };
>> +          };
>> +        };
>> +      };
>> +    };
>> +
>> +    usbotg_hs {
> 
> Generic node names, no underscores in node names.

ack, I guess you'd recommend "usb" here. I'll update it.

Thanks for reviewing,
Best Regards,
Fabrice

> 
>> +      usb-role-switch;
>> +      port {
>> +        usbotg_hs_ep: endpoint {
>> +          remote-endpoint = <&con_usb_c_ep>;
>> +        };
>> +      };
>> +    };
>> +...
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 2/4] usb: typec: ucsi: stm32g0: add support for stm32g0 i2c controller

[Fabrice Gasnier]

On 6/27/22 15:17, Heikki Krogerus wrote:
> Hi,
> 
> On Fri, Jun 24, 2022 at 05:54:11PM +0200, Fabrice Gasnier wrote:
>> +static int ucsi_stm32g0_probe(struct i2c_client *client, const struct i2c_device_id *id)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct ucsi_stm32g0 *g0;
>> +	int ret;
>> +
>> +	g0 = devm_kzalloc(dev, sizeof(*g0), GFP_KERNEL);
>> +	if (!g0)
>> +		return -ENOMEM;
>> +
>> +	g0->dev = dev;
>> +	g0->client = client;
>> +	init_completion(&g0->complete);
>> +	i2c_set_clientdata(client, g0);
>> +
>> +	g0->ucsi = ucsi_create(dev, &ucsi_stm32g0_ops);
>> +	if (IS_ERR(g0->ucsi))
>> +		return PTR_ERR(g0->ucsi);
>> +
>> +	ucsi_set_drvdata(g0->ucsi, g0);
>> +
>> +	/* Request alert interrupt */
>> +	ret = request_threaded_irq(client->irq, NULL, ucsi_stm32g0_irq_handler, IRQF_ONESHOT,
>> +				   dev_name(&client->dev), g0);
>> +	if (ret) {
>> +		dev_err_probe(dev, ret, "request IRQ failed\n");
>> +		goto destroy;
>> +	}
>> +
>> +	ret = ucsi_register(g0->ucsi);
>> +	if (ret) {
>> +		dev_err_probe(dev, ret, "ucsi_register failed\n");
>> +		goto freeirq;
>> +	}
> 
> If there isn't UCSI firmware, then ucsi_register() will always safely
> fail here, right?

Hi Heikki,

Yes, in such a case, the first i2c read (UCSI_VERSION) in
ucsi_register() will return an error and safely fail here.

Thanks for reviewing,
Best Regards,
Fabrice

> 
> 
>> +	return 0;
>> +
>> +freeirq:
>> +	free_irq(client->irq, g0);
>> +destroy:
>> +	ucsi_destroy(g0->ucsi);
>> +
>> +	return ret;
>> +}
> 
> 
> thanks,
> 

Re: [PATCH 2/4] usb: typec: ucsi: stm32g0: add support for stm32g0 i2c controller

[Heikki Krogerus]

On Tue, Jun 28, 2022 at 09:21:12AM +0200, Fabrice Gasnier wrote:
> On 6/27/22 15:17, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Fri, Jun 24, 2022 at 05:54:11PM +0200, Fabrice Gasnier wrote:
> >> +static int ucsi_stm32g0_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >> +{
> >> +	struct device *dev = &client->dev;
> >> +	struct ucsi_stm32g0 *g0;
> >> +	int ret;
> >> +
> >> +	g0 = devm_kzalloc(dev, sizeof(*g0), GFP_KERNEL);
> >> +	if (!g0)
> >> +		return -ENOMEM;
> >> +
> >> +	g0->dev = dev;
> >> +	g0->client = client;
> >> +	init_completion(&g0->complete);
> >> +	i2c_set_clientdata(client, g0);
> >> +
> >> +	g0->ucsi = ucsi_create(dev, &ucsi_stm32g0_ops);
> >> +	if (IS_ERR(g0->ucsi))
> >> +		return PTR_ERR(g0->ucsi);
> >> +
> >> +	ucsi_set_drvdata(g0->ucsi, g0);
> >> +
> >> +	/* Request alert interrupt */
> >> +	ret = request_threaded_irq(client->irq, NULL, ucsi_stm32g0_irq_handler, IRQF_ONESHOT,
> >> +				   dev_name(&client->dev), g0);
> >> +	if (ret) {
> >> +		dev_err_probe(dev, ret, "request IRQ failed\n");
> >> +		goto destroy;
> >> +	}
> >> +
> >> +	ret = ucsi_register(g0->ucsi);
> >> +	if (ret) {
> >> +		dev_err_probe(dev, ret, "ucsi_register failed\n");
> >> +		goto freeirq;
> >> +	}
> > 
> > If there isn't UCSI firmware, then ucsi_register() will always safely
> > fail here, right?
> 
> Hi Heikki,
> 
> Yes, in such a case, the first i2c read (UCSI_VERSION) in
> ucsi_register() will return an error and safely fail here.

Okay, thanks.

-- 
heikki

Re: [PATCH 1/4] dt-bindings: usb: typec: add bindings for stm32g0 controller

[Krzysztof Kozlowski]

On 27/06/2022 16:21, Fabrice Gasnier wrote:
> On 6/24/22 18:16, Krzysztof Kozlowski wrote:
>> On 24/06/2022 17:54, Fabrice Gasnier wrote:
>>> This patch adds DT schema documentation for the STM32G0 Type-C controller.
>>
>> No "This patch"
> 
> Hi Krzysztof,
> 
> ack,
> 
>>
>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>
>>> STM32G0 provides an integrated USB Type-C and power delivery interface.
>>> It can be programmed with a firmware to handle UCSI protocol over I2C
>>> interface. A GPIO is used as an interrupt line.
>>> It may be used as a wakeup source, so use optional "wakeup-source" and
>>> "power-domains" properties to support wakeup.
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
>>> ---
>>>  .../bindings/usb/st,typec-stm32g0.yaml        | 83 +++++++++++++++++++
>>>  1 file changed, 83 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
>>> new file mode 100644
>>> index 0000000000000..b2729bd015a1a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
>>> @@ -0,0 +1,83 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: "http://devicetree.org/schemas/usb/st,typec-stm32g0.yaml#"
>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>>
>> No quotes.
> 
> ack,
> 
>>
>>> +
>>> +title: STMicroelectronics STM32G0 Type-C controller bindings
>>
>> s/bindings//
> 
> ack,
> 
>>
>>> +
>>> +description: |
>>> +  The STM32G0 MCU can be programmed to control Type-C connector(s) through I2C
>>> +  typically using the UCSI protocol over I2C, with a dedicated alert
>>> +  (interrupt) pin.
>>> +
>>> +maintainers:
>>> +  - Fabrice Gasnier <fabrice.gasnier@foss.st.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: st,stm32g0-typec
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  connector:
>>> +    type: object> +    allOf:
>>> +      - $ref: ../connector/usb-connector.yaml#
>>
>> Full path, so /schemas/connector/...
>>
>> unevaluatedProperties: false
> 
> ack,
> 
>>
>>> +
>>> +  firmware-name:
>>> +    description: |
>>> +      Should contain the name of the default firmware image
>>> +      file located on the firmware search path
>>> +
>>> +  wakeup-source: true
>>> +  power-domains: true
>>
>> maxItems
> 
> Do you mean maxItems regarding the "power-domains" property ?

Yes.

> This will depend on the user platform, where it's used as an I2C device.
> So I'm not sure this can / should be specified here.
> Could please you clarify ?

Then maybe this property is not valid here. Power domains usually are
used for blocks of a SoC, having common power source and power gating.
In your case it looks much more like a regulator supply.

> 
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>> +    i2c5 {
>>
>> Just "i2c"
> 
> ack,
> 
>>
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +
>>> +      stm32g0@53 {
>>
>> Generic node name describing class of the device.
> 
> 
> I wasn't aware of generic node name for an I2C device (not talking of
> the controller). I may have missed it.
> 
> Could you please clarify ?

The class of a device is not a I2C device. I2C is just a bus. For
example the generic name for Power Management IC connected over I2C
(quite common case) is "pmic".

For USB HCD controllers the generic name is "usb". For USB
ports/connectors this is "connector". So what is your hardware?
"interface" is a bit too unspecific to figure it out.

> 
>>
>>> +        compatible = "st,stm32g0-typec";
>>> +        reg = <0x53>;
>>> +        /* Alert pin on GPIO PE12 */
>>> +        interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
>>> +        interrupt-parent = <&gpioe>;
>>> +
>>> +        /* Example with one type-C connector */
>>> +        connector {
>>> +          compatible = "usb-c-connector";
>>> +          label = "USB-C";
>>> +
>>> +          port {
>>
>> This does not look like proper schema of connector.yaml.
> 
> This refers to graph.yaml [1], where similar example is seen [2].
> 
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L79
> 
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L207

Just look at the usb-conector schema. It's different. You miss ports.
Maybe other properties as well.

> 
>     device-1 {
>         port {
>             device_1_output: endpoint {
>                 remote-endpoint = <&device_2_input>;
>             };
>         };
>     };
>     device-2 {
>         port {
>             device_2_input: endpoint {
>                 remote-endpoint = <&device_1_output>;
>             };
>         };
>     };
> 
> 
> Could you please clarify this point too ?
> 
>>
>>> +            con_usb_c_ep: endpoint {
>>> +              remote-endpoint = <&usbotg_hs_ep>;
>>> +            };
>>> +          };
>>> +        };
>>> +      };
>>> +    };
>>> +
>>> +    usbotg_hs {
>>
>> Generic node names, no underscores in node names.
> 
> ack, I guess you'd recommend "usb" here. I'll update it.

Yes, looks like usb.


Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: usb: typec: add bindings for stm32g0 controller

[Fabrice Gasnier]

On 6/28/22 12:28, Krzysztof Kozlowski wrote:
> On 27/06/2022 16:21, Fabrice Gasnier wrote:
>> On 6/24/22 18:16, Krzysztof Kozlowski wrote:
>>> On 24/06/2022 17:54, Fabrice Gasnier wrote:
>>>> This patch adds DT schema documentation for the STM32G0 Type-C controller.
>>>
>>> No "This patch"
>>
>> Hi Krzysztof,
>>
>> ack,
>>
>>>
>>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>>
>>>> STM32G0 provides an integrated USB Type-C and power delivery interface.
>>>> It can be programmed with a firmware to handle UCSI protocol over I2C
>>>> interface. A GPIO is used as an interrupt line.
>>>> It may be used as a wakeup source, so use optional "wakeup-source" and
>>>> "power-domains" properties to support wakeup.
>>>>
>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
>>>> ---
>>>>  .../bindings/usb/st,typec-stm32g0.yaml        | 83 +++++++++++++++++++
>>>>  1 file changed, 83 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
>>>> new file mode 100644
>>>> index 0000000000000..b2729bd015a1a
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
>>>> @@ -0,0 +1,83 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: "http://devicetree.org/schemas/usb/st,typec-stm32g0.yaml#"
>>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>>>
>>> No quotes.
>>
>> ack,
>>
>>>
>>>> +
>>>> +title: STMicroelectronics STM32G0 Type-C controller bindings
>>>
>>> s/bindings//
>>
>> ack,
>>
>>>
>>>> +
>>>> +description: |
>>>> +  The STM32G0 MCU can be programmed to control Type-C connector(s) through I2C
>>>> +  typically using the UCSI protocol over I2C, with a dedicated alert
>>>> +  (interrupt) pin.
>>>> +
>>>> +maintainers:
>>>> +  - Fabrice Gasnier <fabrice.gasnier@foss.st.com>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: st,stm32g0-typec
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  connector:
>>>> +    type: object> +    allOf:
>>>> +      - $ref: ../connector/usb-connector.yaml#
>>>
>>> Full path, so /schemas/connector/...
>>>
>>> unevaluatedProperties: false

Hi Krzysztof,

I Just figured out usb-connector schema has "additionalProperties:
true". Adding "unevaluatedProperties: false" here seem to be useless.
At least at my end, this make any dummy property added in the example
below to be validated without error by the schema.

Should this be updated in usb-connector.yaml instead ?

Shall I omit it here in the end ?

>>
>> ack,
>>
>>>
>>>> +
>>>> +  firmware-name:
>>>> +    description: |
>>>> +      Should contain the name of the default firmware image
>>>> +      file located on the firmware search path
>>>> +
>>>> +  wakeup-source: true
>>>> +  power-domains: true
>>>
>>> maxItems
>>
>> Do you mean maxItems regarding the "power-domains" property ?
> 
> Yes.
> 
>> This will depend on the user platform, where it's used as an I2C device.
>> So I'm not sure this can / should be specified here.
>> Could please you clarify ?
> 
> Then maybe this property is not valid here. Power domains usually are
> used for blocks of a SoC, having common power source and power gating.
> In your case it looks much more like a regulator supply.

This property is used in our implementation to refer to SOC PM domain
for GPIO that is used to wakeup the system. This isn't only a regulator,
this PM domain serves various IPs such as I2C, GPIO, UART... (it manages
regulator and clocks used in low power).

I can limit to 1 item if this is fine for you ?

e.g. maxItems: 1

> 
>>
>>>
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - interrupts
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>> +    i2c5 {
>>>
>>> Just "i2c"
>>
>> ack,
>>
>>>
>>>> +      #address-cells = <1>;
>>>> +      #size-cells = <0>;
>>>> +
>>>> +      stm32g0@53 {
>>>
>>> Generic node name describing class of the device.
>>
>>
>> I wasn't aware of generic node name for an I2C device (not talking of
>> the controller). I may have missed it.
>>
>> Could you please clarify ?
> 
> The class of a device is not a I2C device. I2C is just a bus. For
> example the generic name for Power Management IC connected over I2C
> (quite common case) is "pmic".
> 
> For USB HCD controllers the generic name is "usb". For USB
> ports/connectors this is "connector". So what is your hardware?
> "interface" is a bit too unspecific to figure it out.

Thanks, I better understand your point now.

A common definition for the hardware here could be "USB Type-C PD
controller". I'll improve this schema title by the way.

I had a quick look in various .dts files. I could find mainly:
- typec-portc@hh
- usb-typec@hh
- typec@hh

Not sure if this has already been discussed in other reviews, it lacks
the "controller" idea in the naming IMHO.
Perhaps something like "typec-pd-controller" or
"usb-typec-pd-controller" could be used here ?

Otherwise, I could adopt the shortest "typec" name if it's fine for you ?

> 
>>
>>>
>>>> +        compatible = "st,stm32g0-typec";
>>>> +        reg = <0x53>;
>>>> +        /* Alert pin on GPIO PE12 */
>>>> +        interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
>>>> +        interrupt-parent = <&gpioe>;
>>>> +
>>>> +        /* Example with one type-C connector */
>>>> +        connector {
>>>> +          compatible = "usb-c-connector";
>>>> +          label = "USB-C";
>>>> +
>>>> +          port {
>>>
>>> This does not look like proper schema of connector.yaml.
>>
>> This refers to graph.yaml [1], where similar example is seen [2].
>>
>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L79
>>
>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L207
> 
> Just look at the usb-conector schema. It's different. You miss ports.
> Maybe other properties as well.


(I may miss something, and got confused around port/ports earlier)
The graph properties seems to allow both the 'port' and 'ports' syntax
thanks to the graph definition.
The "port" syntax is also used in other typec controller schemas.

There's only one port in this example. Of course other example could use
two or more ports (like for USB HS / SS / aux) which would require using
the "ports" node (with port@0/1/2 childs).

I can adopt the "ports" node if you prefer. As I see it just doesn't
bring much in the current example (The only drawback is this adds one
indentation/node level w.r.t. the bellow example, so not a big deal).

Please advise,

Thanks for reviewing,
Best Regards,
Fabrice

> 
>>
>>     device-1 {
>>         port {
>>             device_1_output: endpoint {
>>                 remote-endpoint = <&device_2_input>;
>>             };
>>         };
>>     };
>>     device-2 {
>>         port {
>>             device_2_input: endpoint {
>>                 remote-endpoint = <&device_1_output>;
>>             };
>>         };
>>     };
>>
>>
>> Could you please clarify this point too ?
>>
>>>
>>>> +            con_usb_c_ep: endpoint {
>>>> +              remote-endpoint = <&usbotg_hs_ep>;
>>>> +            };
>>>> +          };
>>>> +        };
>>>> +      };
>>>> +    };
>>>> +
>>>> +    usbotg_hs {
>>>
>>> Generic node names, no underscores in node names.
>>
>> ack, I guess you'd recommend "usb" here. I'll update it.
> 
> Yes, looks like usb.
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/4] dt-bindings: usb: typec: add bindings for stm32g0 controller

[Krzysztof Kozlowski]

On 28/06/2022 19:01, Fabrice Gasnier wrote:

>>>>> +  connector:
>>>>> +    type: object> +    allOf:
>>>>> +      - $ref: ../connector/usb-connector.yaml#
>>>>
>>>> Full path, so /schemas/connector/...
>>>>
>>>> unevaluatedProperties: false
> 
> Hi Krzysztof,
> 
> I Just figured out usb-connector schema has "additionalProperties:
> true". Adding "unevaluatedProperties: false" here seem to be useless.
> At least at my end, this make any dummy property added in the example
> below to be validated without error by the schema.

No, it's expected. The common schema allows additional properties. You
specific device schema (including common) should not allow anything more
and this is expressed like you mentioned.

However depending on the version of dtschema, the
unevaluatedProperties:false might still be not implemented. AFAIK, Rob
added it quite recently.

> 
> Should this be updated in usb-connector.yaml instead ?

No

> 
> Shall I omit it here in the end ?

You need to add here unevaluatedProperties: false (on the level of this
$ref)

> 
>>>
>>> ack,
>>>
>>>>
>>>>> +
>>>>> +  firmware-name:
>>>>> +    description: |
>>>>> +      Should contain the name of the default firmware image
>>>>> +      file located on the firmware search path
>>>>> +
>>>>> +  wakeup-source: true
>>>>> +  power-domains: true
>>>>
>>>> maxItems
>>>
>>> Do you mean maxItems regarding the "power-domains" property ?
>>
>> Yes.
>>
>>> This will depend on the user platform, where it's used as an I2C device.
>>> So I'm not sure this can / should be specified here.
>>> Could please you clarify ?
>>
>> Then maybe this property is not valid here. Power domains usually are
>> used for blocks of a SoC, having common power source and power gating.
>> In your case it looks much more like a regulator supply.
> 
> This property is used in our implementation to refer to SOC PM domain
> for GPIO that is used to wakeup the system. This isn't only a regulator,
> this PM domain serves various IPs such as I2C, GPIO, UART... (it manages
> regulator and clocks used in low power).
> 
> I can limit to 1 item if this is fine for you ?
> 
> e.g. maxItems: 1

Yes, it's good (assuming it is true :) ).

> 
>>
>>>
>>>>
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>> +  - reg
>>>>> +  - interrupts
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>>> +    i2c5 {
>>>>
>>>> Just "i2c"
>>>
>>> ack,
>>>
>>>>
>>>>> +      #address-cells = <1>;
>>>>> +      #size-cells = <0>;
>>>>> +
>>>>> +      stm32g0@53 {
>>>>
>>>> Generic node name describing class of the device.
>>>
>>>
>>> I wasn't aware of generic node name for an I2C device (not talking of
>>> the controller). I may have missed it.
>>>
>>> Could you please clarify ?
>>
>> The class of a device is not a I2C device. I2C is just a bus. For
>> example the generic name for Power Management IC connected over I2C
>> (quite common case) is "pmic".
>>
>> For USB HCD controllers the generic name is "usb". For USB
>> ports/connectors this is "connector". So what is your hardware?
>> "interface" is a bit too unspecific to figure it out.
> 
> Thanks, I better understand your point now.
> 
> A common definition for the hardware here could be "USB Type-C PD
> controller". I'll improve this schema title by the way.
> 
> I had a quick look in various .dts files. I could find mainly:
> - typec-portc@hh
> - usb-typec@hh
> - typec@hh
> 
> Not sure if this has already been discussed in other reviews, it lacks
> the "controller" idea in the naming IMHO.
> Perhaps something like "typec-pd-controller" or
> "usb-typec-pd-controller" could be used here ?
> 
> Otherwise, I could adopt the shortest "typec" name if it's fine for you ?

typec sounds good.

> 
>>
>>>
>>>>
>>>>> +        compatible = "st,stm32g0-typec";
>>>>> +        reg = <0x53>;
>>>>> +        /* Alert pin on GPIO PE12 */
>>>>> +        interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
>>>>> +        interrupt-parent = <&gpioe>;
>>>>> +
>>>>> +        /* Example with one type-C connector */
>>>>> +        connector {
>>>>> +          compatible = "usb-c-connector";
>>>>> +          label = "USB-C";
>>>>> +
>>>>> +          port {
>>>>
>>>> This does not look like proper schema of connector.yaml.
>>>
>>> This refers to graph.yaml [1], where similar example is seen [2].
>>>
>>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L79
>>>
>>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L207
>>
>> Just look at the usb-conector schema. It's different. You miss ports.
>> Maybe other properties as well.
> 
> 
> (I may miss something, and got confused around port/ports earlier)
> The graph properties seems to allow both the 'port' and 'ports' syntax
> thanks to the graph definition.
> The "port" syntax is also used in other typec controller schemas.
> 
> There's only one port in this example. Of course other example could use
> two or more ports (like for USB HS / SS / aux) which would require using
> the "ports" node (with port@0/1/2 childs).
> 
> I can adopt the "ports" node if you prefer. As I see it just doesn't
> bring much in the current example (The only drawback is this adds one
> indentation/node level w.r.t. the bellow example, so not a big deal).

The graph schema allows, but you include here usb-connector schema which
requires to put it under "ports". You should not use it differently, so
I expect here "ports" property, even with one port.

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: usb: typec: add bindings for stm32g0 controller

[Fabrice Gasnier]

On 6/29/22 07:54, Krzysztof Kozlowski wrote:
> On 28/06/2022 19:01, Fabrice Gasnier wrote:
> 
>>>>>> +  connector:
>>>>>> +    type: object> +    allOf:
>>>>>> +      - $ref: ../connector/usb-connector.yaml#
>>>>>
>>>>> Full path, so /schemas/connector/...
>>>>>
>>>>> unevaluatedProperties: false
>>
>> Hi Krzysztof,
>>
>> I Just figured out usb-connector schema has "additionalProperties:
>> true". Adding "unevaluatedProperties: false" here seem to be useless.
>> At least at my end, this make any dummy property added in the example
>> below to be validated without error by the schema.
> 
> No, it's expected. The common schema allows additional properties. You
> specific device schema (including common) should not allow anything more
> and this is expressed like you mentioned.
> 
> However depending on the version of dtschema, the
> unevaluatedProperties:false might still be not implemented. AFAIK, Rob
> added it quite recently.
> 
>>
>> Should this be updated in usb-connector.yaml instead ?
> 
> No
> 
>>
>> Shall I omit it here in the end ?
> 
> You need to add here unevaluatedProperties: false (on the level of this
> $ref)
> 
>>
>>>>
>>>> ack,
>>>>
>>>>>
>>>>>> +
>>>>>> +  firmware-name:
>>>>>> +    description: |
>>>>>> +      Should contain the name of the default firmware image
>>>>>> +      file located on the firmware search path
>>>>>> +
>>>>>> +  wakeup-source: true
>>>>>> +  power-domains: true
>>>>>
>>>>> maxItems
>>>>
>>>> Do you mean maxItems regarding the "power-domains" property ?
>>>
>>> Yes.
>>>
>>>> This will depend on the user platform, where it's used as an I2C device.
>>>> So I'm not sure this can / should be specified here.
>>>> Could please you clarify ?
>>>
>>> Then maybe this property is not valid here. Power domains usually are
>>> used for blocks of a SoC, having common power source and power gating.
>>> In your case it looks much more like a regulator supply.
>>
>> This property is used in our implementation to refer to SOC PM domain
>> for GPIO that is used to wakeup the system. This isn't only a regulator,
>> this PM domain serves various IPs such as I2C, GPIO, UART... (it manages
>> regulator and clocks used in low power).
>>
>> I can limit to 1 item if this is fine for you ?
>>
>> e.g. maxItems: 1
> 
> Yes, it's good (assuming it is true :) ).
> 
>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +required:
>>>>>> +  - compatible
>>>>>> +  - reg
>>>>>> +  - interrupts
>>>>>> +
>>>>>> +additionalProperties: false
>>>>>> +
>>>>>> +examples:
>>>>>> +  - |
>>>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>>>> +    i2c5 {
>>>>>
>>>>> Just "i2c"
>>>>
>>>> ack,
>>>>
>>>>>
>>>>>> +      #address-cells = <1>;
>>>>>> +      #size-cells = <0>;
>>>>>> +
>>>>>> +      stm32g0@53 {
>>>>>
>>>>> Generic node name describing class of the device.
>>>>
>>>>
>>>> I wasn't aware of generic node name for an I2C device (not talking of
>>>> the controller). I may have missed it.
>>>>
>>>> Could you please clarify ?
>>>
>>> The class of a device is not a I2C device. I2C is just a bus. For
>>> example the generic name for Power Management IC connected over I2C
>>> (quite common case) is "pmic".
>>>
>>> For USB HCD controllers the generic name is "usb". For USB
>>> ports/connectors this is "connector". So what is your hardware?
>>> "interface" is a bit too unspecific to figure it out.
>>
>> Thanks, I better understand your point now.
>>
>> A common definition for the hardware here could be "USB Type-C PD
>> controller". I'll improve this schema title by the way.
>>
>> I had a quick look in various .dts files. I could find mainly:
>> - typec-portc@hh
>> - usb-typec@hh
>> - typec@hh
>>
>> Not sure if this has already been discussed in other reviews, it lacks
>> the "controller" idea in the naming IMHO.
>> Perhaps something like "typec-pd-controller" or
>> "usb-typec-pd-controller" could be used here ?
>>
>> Otherwise, I could adopt the shortest "typec" name if it's fine for you ?
> 
> typec sounds good.
> 
>>
>>>
>>>>
>>>>>
>>>>>> +        compatible = "st,stm32g0-typec";
>>>>>> +        reg = <0x53>;
>>>>>> +        /* Alert pin on GPIO PE12 */
>>>>>> +        interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
>>>>>> +        interrupt-parent = <&gpioe>;
>>>>>> +
>>>>>> +        /* Example with one type-C connector */
>>>>>> +        connector {
>>>>>> +          compatible = "usb-c-connector";
>>>>>> +          label = "USB-C";
>>>>>> +
>>>>>> +          port {
>>>>>
>>>>> This does not look like proper schema of connector.yaml.
>>>>
>>>> This refers to graph.yaml [1], where similar example is seen [2].
>>>>
>>>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L79
>>>>
>>>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L207
>>>
>>> Just look at the usb-conector schema. It's different. You miss ports.
>>> Maybe other properties as well.
>>
>>
>> (I may miss something, and got confused around port/ports earlier)
>> The graph properties seems to allow both the 'port' and 'ports' syntax
>> thanks to the graph definition.
>> The "port" syntax is also used in other typec controller schemas.
>>
>> There's only one port in this example. Of course other example could use
>> two or more ports (like for USB HS / SS / aux) which would require using
>> the "ports" node (with port@0/1/2 childs).
>>
>> I can adopt the "ports" node if you prefer. As I see it just doesn't
>> bring much in the current example (The only drawback is this adds one
>> indentation/node level w.r.t. the bellow example, so not a big deal).
> 
> The graph schema allows, but you include here usb-connector schema which
> requires to put it under "ports". You should not use it differently, so
> I expect here "ports" property, even with one port.

Hi Krzysztof,

This makes senses. I've updated this locally and also put this in .dts
file (not sent yet with this series as I lack some dependencies not yet
upstream).

I'm able to validate the schema, with your statement, by using
dt_binding_check.

/* Example with one type-C connector */
connector {
  compatible = "usb-c-connector";
  label = "USB-C";

  ports {
    #address-cells = <1>;
    #size-cells = <0>;
    port@0 {
      reg = <0>;
      con_usb_c_ep: endpoint {
        remote-endpoint = <&usb_ep>;
      };
    };
  };
};

Still when build the .dts (in my downstream, with W=1) I observe various
case, for a single port usage (e.g. USB HS only).


With above example I get:
---
Warning (graph_child_address): /soc/..../connector/ports: graph node has
single child node 'port@0', #address-cells/#size-cells are not necessary

Remove them as not necessary (suggested by this warning):
---
/* Example with one type-C connector */
connector {
  compatible = "usb-c-connector";
  label = "USB-C";

  ports {
    port {
      con_usb_c_ep: endpoint {
        remote-endpoint = <&usb_ep>;
      };
    };
  };
};

Then I no longer get this warning upon build. But the dtbs_check complains:
---
connector: ports: 'port@0' is a required property
	From schema: ..
Documentation/devicetree/bindings/connector/usb-connector.yaml

So It looks like to me there's something missing to handle the single
port case in usb-connector.yaml, when using the "ports".

Maybe usb-connector could be updated to handle "port" (w/o unit-addr) ?
I'm talking about:
    required:
      - port@0

So, I came up with:

--- a/Documentation/devicetree/bindings/connector/usb-connector.yaml
+++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml
@@ -176,6 +176,9 @@ properties:
       port number as described below.

     properties:
+      port:
+        $ref: /schemas/graph.yaml#/properties/port
+
       port@0:
         $ref: /schemas/graph.yaml#/properties/port
         description: High Speed (HS), present in all connectors.
@@ -189,8 +192,11 @@ properties:
         description: Sideband Use (SBU), present in USB-C. This
describes the
           alternate mode connection of which SBU is a part.

-    required:
-      - port@0
+    oneOf:
+      - required:
+          - port
+      - required:
+          - port@0


Do you agree on this approach ? (I can add a pre-cursor patch to this
series, to handle the single port case)


Please advise,
Best Regards,
Fabrice

> 
> Best regards,
> Krzysztof

Re: [PATCH 1/4] dt-bindings: usb: typec: add bindings for stm32g0 controller

[Krzysztof Kozlowski]

On 01/07/2022 12:04, Fabrice Gasnier wrote:
> 
> Then I no longer get this warning upon build. But the dtbs_check complains:
> ---
> connector: ports: 'port@0' is a required property
> 	From schema: ..
> Documentation/devicetree/bindings/connector/usb-connector.yaml
> 
> So It looks like to me there's something missing to handle the single
> port case in usb-connector.yaml, when using the "ports".
> 
> Maybe usb-connector could be updated to handle "port" (w/o unit-addr) ?

Not really, the dtc warning looks false-positive. Especially that you
need port@1 for USB 3.0 (super speed), unless you do not support it?

> I'm talking about:
>     required:
>       - port@0
> 
> So, I came up with:
> 
> --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml
> @@ -176,6 +176,9 @@ properties:
>        port number as described below.
> 
>      properties:
> +      port:
> +        $ref: /schemas/graph.yaml#/properties/port
> +
>        port@0:
>          $ref: /schemas/graph.yaml#/properties/port
>          description: High Speed (HS), present in all connectors.
> @@ -189,8 +192,11 @@ properties:
>          description: Sideband Use (SBU), present in USB-C. This
> describes the
>            alternate mode connection of which SBU is a part.
> 
> -    required:
> -      - port@0
> +    oneOf:
> +      - required:
> +          - port
> +      - required:
> +          - port@0
> 
> 
> Do you agree on this approach ? (I can add a pre-cursor patch to this
> series, to handle the single port case)



Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: usb: typec: add bindings for stm32g0 controller

[Fabrice Gasnier]

On 7/4/22 09:55, Krzysztof Kozlowski wrote:
> On 01/07/2022 12:04, Fabrice Gasnier wrote:
>>
>> Then I no longer get this warning upon build. But the dtbs_check complains:
>> ---
>> connector: ports: 'port@0' is a required property
>> 	From schema: ..
>> Documentation/devicetree/bindings/connector/usb-connector.yaml
>>
>> So It looks like to me there's something missing to handle the single
>> port case in usb-connector.yaml, when using the "ports".
>>
>> Maybe usb-connector could be updated to handle "port" (w/o unit-addr) ?
> 
> Not really, the dtc warning looks false-positive. Especially that you
> need port@1 for USB 3.0 (super speed), unless you do not support it?

Hi Krzysztof,

Having USB2.0 High speed port only is perfectly valid. port@1 is
optional to support USB3.0 as you mention.

I've no opinion regarding a possible false positive warning. I'd like to
sort this out, perhaps Rob has some recommendation regarding this ?

Please advise,
Best regards,
Fabrice

> 
>> I'm talking about:
>>     required:
>>       - port@0
>>
>> So, I came up with:
>>
>> --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml
>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml
>> @@ -176,6 +176,9 @@ properties:
>>        port number as described below.
>>
>>      properties:
>> +      port:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +
>>        port@0:
>>          $ref: /schemas/graph.yaml#/properties/port
>>          description: High Speed (HS), present in all connectors.
>> @@ -189,8 +192,11 @@ properties:
>>          description: Sideband Use (SBU), present in USB-C. This
>> describes the
>>            alternate mode connection of which SBU is a part.
>>
>> -    required:
>> -      - port@0
>> +    oneOf:
>> +      - required:
>> +          - port
>> +      - required:
>> +          - port@0
>>
>>
>> Do you agree on this approach ? (I can add a pre-cursor patch to this
>> series, to handle the single port case)
> 
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/4] dt-bindings: usb: typec: add bindings for stm32g0 controller

[Krzysztof Kozlowski]

On 04/07/2022 11:08, Fabrice Gasnier wrote:
> On 7/4/22 09:55, Krzysztof Kozlowski wrote:
>> On 01/07/2022 12:04, Fabrice Gasnier wrote:
>>>
>>> Then I no longer get this warning upon build. But the dtbs_check complains:
>>> ---
>>> connector: ports: 'port@0' is a required property
>>> 	From schema: ..
>>> Documentation/devicetree/bindings/connector/usb-connector.yaml
>>>
>>> So It looks like to me there's something missing to handle the single
>>> port case in usb-connector.yaml, when using the "ports".
>>>
>>> Maybe usb-connector could be updated to handle "port" (w/o unit-addr) ?
>>
>> Not really, the dtc warning looks false-positive. Especially that you
>> need port@1 for USB 3.0 (super speed), unless you do not support it?
> 
> Hi Krzysztof,
> 
> Having USB2.0 High speed port only is perfectly valid. port@1 is
> optional to support USB3.0 as you mention.
> 
> I've no opinion regarding a possible false positive warning. I'd like to
> sort this out, perhaps Rob has some recommendation regarding this ?

I would propose to skip the DTC warning and stick to the schema with
only one port@0.


Best regards,
Krzysztof