We are developing a UCSI ChromeOS EC transport driver. The ChromeOS EC
implements a UCSI PPM. This driver is being developed in
drivers/platform/chrome since
1) Most other drivers which depend on ChromeOS EC reside there.
2) Our architecture might undergo a few revisions rapidly, so
platform/chrome seems like a good place while we finalize our
design.
This patch series creates a public include/usb/ucsi.h that can be used
by transport drivers outside drivers/usb/typec/ucsi. Then, we use this
interface and ChromeOS EC host commands to send UCSI commands in
drivers/platform/chrome/cros_ec_ucsi.c.
Signed-off-by: Pavan Holla <[email protected]>
---
Changes in v2:
- No code or commit message changes.
- Added drivers/platform/chrome maintainers for review.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Pavan Holla (3):
usb: typec: ucsi: Provide interface for UCSI transport
usb: typec: ucsi: Import interface for UCSI transport
platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver
MAINTAINERS | 1 +
drivers/platform/chrome/Kconfig | 14 ++
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_ec_ucsi.c | 247 +++++++++++++++++++++++++
drivers/usb/typec/ucsi/ucsi.h | 54 +-----
include/linux/platform_data/cros_ec_commands.h | 19 ++
include/linux/usb/ucsi.h | 66 +++++++
7 files changed, 349 insertions(+), 53 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240325-public-ucsi-h-3ecee4106a58
Best regards,
--
Pavan Holla <[email protected]>
Import include/linux/usb/ucsi.h and remove any duplicate declarations.
Signed-off-by: Pavan Holla <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.h | 54 +------------------------------------------
1 file changed, 1 insertion(+), 53 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 32daf5f58650..167951538030 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -10,22 +10,13 @@
#include <linux/usb/typec.h>
#include <linux/usb/pd.h>
#include <linux/usb/role.h>
+#include <linux/usb/ucsi.h>
#include <asm/unaligned.h>
/* -------------------------------------------------------------------------- */
-struct ucsi;
-struct ucsi_altmode;
struct dentry;
-/* UCSI offsets (Bytes) */
-#define UCSI_VERSION 0
-#define UCSI_CCI 4
-#define UCSI_CONTROL 8
-#define UCSI_MESSAGE_IN 16
-#define UCSI_MESSAGE_OUT 32
-#define UCSIv2_MESSAGE_OUT 272
-
/* UCSI versions */
#define UCSI_VERSION_1_2 0x0120
#define UCSI_VERSION_2_0 0x0200
@@ -42,48 +33,6 @@ struct dentry;
*/
#define UCSI_SPEC_REVISION_TO_BCD(_v_) (((_v_) + 1) << 8)
-/* Command Status and Connector Change Indication (CCI) bits */
-#define UCSI_CCI_CONNECTOR(_c_) (((_c_) & GENMASK(7, 1)) >> 1)
-#define UCSI_CCI_LENGTH(_c_) (((_c_) & GENMASK(15, 8)) >> 8)
-#define UCSI_CCI_NOT_SUPPORTED BIT(25)
-#define UCSI_CCI_CANCEL_COMPLETE BIT(26)
-#define UCSI_CCI_RESET_COMPLETE BIT(27)
-#define UCSI_CCI_BUSY BIT(28)
-#define UCSI_CCI_ACK_COMPLETE BIT(29)
-#define UCSI_CCI_ERROR BIT(30)
-#define UCSI_CCI_COMMAND_COMPLETE BIT(31)
-
-/**
- * struct ucsi_operations - UCSI I/O operations
- * @read: Read operation
- * @sync_write: Blocking write operation
- * @async_write: Non-blocking write operation
- * @update_altmodes: Squashes duplicate DP altmodes
- *
- * Read and write routines for UCSI interface. @sync_write must wait for the
- * Command Completion Event from the PPM before returning, and @async_write must
- * return immediately after sending the data to the PPM.
- */
-struct ucsi_operations {
- int (*read)(struct ucsi *ucsi, unsigned int offset,
- void *val, size_t val_len);
- int (*sync_write)(struct ucsi *ucsi, unsigned int offset,
- const void *val, size_t val_len);
- int (*async_write)(struct ucsi *ucsi, unsigned int offset,
- const void *val, size_t val_len);
- bool (*update_altmodes)(struct ucsi *ucsi, struct ucsi_altmode *orig,
- struct ucsi_altmode *updated);
-};
-
-struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops);
-void ucsi_destroy(struct ucsi *ucsi);
-int ucsi_register(struct ucsi *ucsi);
-void ucsi_unregister(struct ucsi *ucsi);
-void *ucsi_get_drvdata(struct ucsi *ucsi);
-void ucsi_set_drvdata(struct ucsi *ucsi, void *data);
-
-void ucsi_connector_change(struct ucsi *ucsi, u8 num);
-
/* -------------------------------------------------------------------------- */
/* Commands */
@@ -465,7 +414,6 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command,
void *retval, size_t size);
void ucsi_altmode_update_active(struct ucsi_connector *con);
-int ucsi_resume(struct ucsi *ucsi);
#if IS_ENABLED(CONFIG_POWER_SUPPLY)
int ucsi_register_port_psy(struct ucsi_connector *con);
--
2.44.0.396.g6e790dbe36-goog
Implementation of transport driver for UCSI. This driver will be used
if the ChromeOS EC implements a PPM.
Signed-off-by: Pavan Holla <[email protected]>
---
drivers/platform/chrome/Kconfig | 14 ++
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_ec_ucsi.c | 247 +++++++++++++++++++++++++
include/linux/platform_data/cros_ec_commands.h | 19 ++
4 files changed, 281 insertions(+)
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 7a83346bfa53..67ab79a6815b 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -238,6 +238,20 @@ config CROS_EC_TYPEC
To compile this driver as a module, choose M here: the module will be
called cros-ec-typec.
+config CROS_EC_UCSI
+ tristate "UCSI Driver for ChromeOS EC"
+ depends on MFD_CROS_EC_DEV
+ depends on CROS_USBPD_NOTIFY
+ depends on TYPEC_UCSI
+ depends on !EXTCON_TCSS_CROS_EC
+ default MFD_CROS_EC_DEV
+ help
+ This driver enables UCSI support for a ChromeOS EC. The EC is
+ expected to implement a PPM.
+
+ To compile the driver as a module, choose M here: the module
+ will be called cros_ec_ucsi.
+
config CROS_HPS_I2C
tristate "ChromeOS HPS device"
depends on HID && I2C && PM
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 2dcc6ccc2302..85964e165a70 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_CROS_EC_UART) += cros_ec_uart.o
cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_mec.o
cros-ec-typec-objs := cros_ec_typec.o cros_typec_vdm.o
obj-$(CONFIG_CROS_EC_TYPEC) += cros-ec-typec.o
+obj-$(CONFIG_CROS_EC_UCSI) += cros_ec_ucsi.o
obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.o cros_ec_trace.o
obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
diff --git a/drivers/platform/chrome/cros_ec_ucsi.c b/drivers/platform/chrome/cros_ec_ucsi.c
new file mode 100644
index 000000000000..9fee300a3d93
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_ucsi.c
@@ -0,0 +1,247 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UCSI driver for ChromeOS EC
+ *
+ * Copyright 2024 Google LLC.
+ */
+
+#include <linux/container_of.h>
+#include <linux/dev_printk.h>
+#include <linux/module.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_usbpd_notify.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/usb/ucsi.h>
+#include <linux/wait.h>
+
+#define DRV_NAME "cros-ec-ucsi"
+
+#define MAX_EC_DATA_SIZE 256
+#define WRITE_TMO_MS 500
+
+#define COMMAND_PENDING 1
+#define ACK_PENDING 2
+
+#define UCSI_COMMAND(_cmd_) ((_cmd_) & 0xff)
+#define UCSI_ACK_CC_CI 0x04
+
+struct cros_ucsi_data {
+ struct device *dev;
+ struct ucsi *ucsi;
+
+ struct cros_ec_device *ec;
+ struct notifier_block nb;
+ struct work_struct work;
+
+ struct completion complete;
+ unsigned long flags;
+};
+
+static int cros_ucsi_read(struct ucsi *ucsi, unsigned int offset, void *val,
+ size_t val_len)
+{
+ struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
+ struct ec_params_ucsi_ppm_get req = {
+ .offset = offset,
+ .size = val_len,
+ };
+ int ret;
+
+ if (val_len > MAX_EC_DATA_SIZE) {
+ dev_err(udata->dev, "Can't read %zu bytes. Too big.", val_len);
+ return -EINVAL;
+ }
+
+ ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_GET,
+ &req, sizeof(req), val, val_len);
+ if (ret < 0) {
+ dev_warn(udata->dev, "Failed to send EC message UCSI_PPM_GET: error=%d", ret);
+ return ret;
+ }
+ return 0;
+}
+
+static int cros_ucsi_async_write(struct ucsi *ucsi, unsigned int offset,
+ const void *val, size_t val_len)
+{
+ struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
+ uint8_t ec_buffer[MAX_EC_DATA_SIZE + sizeof(struct ec_params_ucsi_ppm_set)];
+ struct ec_params_ucsi_ppm_set *req = (struct ec_params_ucsi_ppm_set *)ec_buffer;
+ int ret = 0;
+
+ if (val_len > MAX_EC_DATA_SIZE) {
+ dev_err(udata->dev, "Can't write %zu bytes. Too big.", val_len);
+ return -EINVAL;
+ }
+
+ memset(req, 0, sizeof(ec_buffer));
+ req->offset = offset;
+ memcpy(req->data, val, val_len);
+ ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_SET,
+ req, sizeof(struct ec_params_ucsi_ppm_set) + val_len, NULL, 0);
+
+ if (ret < 0) {
+ dev_warn(udata->dev, "Failed to send EC message UCSI_PPM_SET: error=%d", ret);
+ return ret;
+ }
+ return 0;
+}
+
+static int cros_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset,
+ const void *val, size_t val_len)
+{
+ struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
+ bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI;
+ int ret;
+
+ if (ack)
+ set_bit(ACK_PENDING, &udata->flags);
+ else
+ set_bit(COMMAND_PENDING, &udata->flags);
+
+ ret = cros_ucsi_async_write(ucsi, offset, val, val_len);
+ if (ret)
+ goto out;
+
+ if (!wait_for_completion_timeout(&udata->complete, WRITE_TMO_MS))
+ ret = -ETIMEDOUT;
+
+out:
+ if (ack)
+ clear_bit(ACK_PENDING, &udata->flags);
+ else
+ clear_bit(COMMAND_PENDING, &udata->flags);
+ return ret;
+}
+
+struct ucsi_operations cros_ucsi_ops = {
+ .read = cros_ucsi_read,
+ .async_write = cros_ucsi_async_write,
+ .sync_write = cros_ucsi_sync_write,
+};
+
+static void cros_ucsi_work(struct work_struct *work)
+{
+ struct cros_ucsi_data *udata = container_of(work, struct cros_ucsi_data, work);
+ u32 cci;
+ int ret;
+
+ ret = cros_ucsi_read(udata->ucsi, UCSI_CCI, &cci, sizeof(cci));
+ if (ret)
+ return;
+
+ if (UCSI_CCI_CONNECTOR(cci))
+ ucsi_connector_change(udata->ucsi, UCSI_CCI_CONNECTOR(cci));
+
+ if (cci & UCSI_CCI_ACK_COMPLETE && test_bit(ACK_PENDING, &udata->flags))
+ complete(&udata->complete);
+ if (cci & UCSI_CCI_COMMAND_COMPLETE &&
+ test_bit(COMMAND_PENDING, &udata->flags))
+ complete(&udata->complete);
+}
+
+static int cros_ucsi_event(struct notifier_block *nb,
+ unsigned long host_event, void *_notify)
+{
+ struct cros_ucsi_data *udata = container_of(nb, struct cros_ucsi_data, nb);
+
+ if (!(host_event & PD_EVENT_PPM))
+ return NOTIFY_OK;
+
+ dev_dbg(udata->dev, "UCSI notification received");
+ flush_work(&udata->work);
+ schedule_work(&udata->work);
+
+ return NOTIFY_OK;
+}
+
+static int cros_ucsi_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cros_ucsi_data *udata;
+ int ret;
+
+ udata = devm_kzalloc(dev, sizeof(*udata), GFP_KERNEL);
+ if (!udata)
+ return -ENOMEM;
+
+ udata->dev = dev;
+
+ udata->ec = ((struct cros_ec_dev *) dev_get_drvdata(pdev->dev.parent))->ec_dev;
+ if (!udata->ec) {
+ dev_err(dev, "couldn't find parent EC device\n");
+ return -ENODEV;
+ }
+
+ platform_set_drvdata(pdev, udata);
+
+ INIT_WORK(&udata->work, cros_ucsi_work);
+ init_completion(&udata->complete);
+
+ udata->ucsi = ucsi_create(udata->dev, &cros_ucsi_ops);
+ if (IS_ERR(udata->ucsi)) {
+ dev_err(dev, "failed to allocate UCSI instance\n");
+ return PTR_ERR(udata->ucsi);
+ }
+
+ ucsi_set_drvdata(udata->ucsi, udata);
+
+ ret = ucsi_register(udata->ucsi);
+ if (ret) {
+ ucsi_destroy(udata->ucsi);
+ udata->ucsi = NULL;
+ return ret;
+ }
+
+ udata->nb.notifier_call = cros_ucsi_event;
+ ret = cros_usbpd_register_notify(&udata->nb);
+ return ret;
+}
+
+static int cros_ucsi_remove(struct platform_device *dev)
+{
+ struct cros_ucsi_data *udata = platform_get_drvdata(dev);
+
+ if (udata->ucsi) {
+ ucsi_unregister(udata->ucsi);
+ ucsi_destroy(udata->ucsi);
+ udata->ucsi = NULL;
+ }
+ return 0;
+}
+
+static int cros_ucsi_suspend(struct device *dev)
+{
+ struct cros_ucsi_data *udata = dev_get_drvdata(dev);
+
+ cancel_work_sync(&udata->work);
+
+ return 0;
+}
+
+static int cros_ucsi_resume(struct device *dev)
+{
+ struct cros_ucsi_data *udata = dev_get_drvdata(dev);
+
+ return ucsi_resume(udata->ucsi);
+}
+
+static SIMPLE_DEV_PM_OPS(cros_ucsi_pm_ops, cros_ucsi_suspend,
+ cros_ucsi_resume);
+
+static struct platform_driver cros_ucsi_driver = {
+ .driver = {
+ .name = DRV_NAME,
+ .pm = &cros_ucsi_pm_ops,
+ },
+ .probe = cros_ucsi_probe,
+ .remove = cros_ucsi_remove,
+};
+
+module_platform_driver(cros_ucsi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("UCSI driver for ChromeOS EC.");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index ecc47d5fe239..39f6f54bbe4c 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -4933,6 +4933,7 @@ struct ec_response_pd_status {
#define PD_EVENT_POWER_CHANGE BIT(1)
#define PD_EVENT_IDENTITY_RECEIVED BIT(2)
#define PD_EVENT_DATA_SWAP BIT(3)
+#define PD_EVENT_PPM BIT(5)
struct ec_response_host_event_status {
uint32_t status; /* PD MCU host event status */
} __ec_align4;
@@ -5994,6 +5995,24 @@ struct ec_response_typec_vdm_response {
#undef VDO_MAX_SIZE
+/*
+ * Read/write interface for UCSI OPM <-> PPM communication.
+ */
+#define EC_CMD_UCSI_PPM_SET 0x0140
+
+/* The data size is stored in the host command protocol header. */
+struct ec_params_ucsi_ppm_set {
+ uint16_t offset;
+ uint8_t data[];
+} __ec_align2;
+
+#define EC_CMD_UCSI_PPM_GET 0x0141
+
+struct ec_params_ucsi_ppm_get {
+ uint16_t offset;
+ uint8_t size;
+} __ec_align2;
+
/*****************************************************************************/
/* The command range 0x200-0x2FF is reserved for Rotor. */
--
2.44.0.396.g6e790dbe36-goog
The ucsi.h include can be used by driver implementations that
provide transport for UCSI commands.
Signed-off-by: Pavan Holla <[email protected]>
---
MAINTAINERS | 1 +
include/linux/usb/ucsi.h | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 67 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index aa3b947fb080..e799d67a8fa5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22987,6 +22987,7 @@ F: Documentation/ABI/testing/sysfs-class-typec
F: Documentation/driver-api/usb/typec.rst
F: drivers/usb/typec/
F: include/linux/usb/typec.h
+F: include/linux/usb/ucsi.h
USB TYPEC INTEL PMC MUX DRIVER
M: Heikki Krogerus <[email protected]>
diff --git a/include/linux/usb/ucsi.h b/include/linux/usb/ucsi.h
new file mode 100644
index 000000000000..3ec1db968070
--- /dev/null
+++ b/include/linux/usb/ucsi.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __LINUX_USB_UCSI_H
+#define __LINUX_USB_UCSI_H
+
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/types.h>
+
+/* -------------------------------------------------------------------------- */
+
+struct ucsi;
+struct ucsi_altmode;
+
+/* UCSI offsets (Bytes) */
+#define UCSI_VERSION 0
+#define UCSI_CCI 4
+#define UCSI_CONTROL 8
+#define UCSI_MESSAGE_IN 16
+#define UCSI_MESSAGE_OUT 32
+#define UCSIv2_MESSAGE_OUT 272
+
+/* Command Status and Connector Change Indication (CCI) bits */
+#define UCSI_CCI_CONNECTOR(_c_) (((_c_) & GENMASK(7, 1)) >> 1)
+#define UCSI_CCI_LENGTH(_c_) (((_c_) & GENMASK(15, 8)) >> 8)
+#define UCSI_CCI_NOT_SUPPORTED BIT(25)
+#define UCSI_CCI_CANCEL_COMPLETE BIT(26)
+#define UCSI_CCI_RESET_COMPLETE BIT(27)
+#define UCSI_CCI_BUSY BIT(28)
+#define UCSI_CCI_ACK_COMPLETE BIT(29)
+#define UCSI_CCI_ERROR BIT(30)
+#define UCSI_CCI_COMMAND_COMPLETE BIT(31)
+
+/**
+ * struct ucsi_operations - UCSI I/O operations
+ * @read: Read operation
+ * @sync_write: Blocking write operation
+ * @async_write: Non-blocking write operation
+ * @update_altmodes: Squashes duplicate DP altmodes
+ *
+ * Read and write routines for UCSI interface. @sync_write must wait for the
+ * Command Completion Event from the PPM before returning, and @async_write must
+ * return immediately after sending the data to the PPM.
+ */
+struct ucsi_operations {
+ int (*read)(struct ucsi *ucsi, unsigned int offset,
+ void *val, size_t val_len);
+ int (*sync_write)(struct ucsi *ucsi, unsigned int offset,
+ const void *val, size_t val_len);
+ int (*async_write)(struct ucsi *ucsi, unsigned int offset,
+ const void *val, size_t val_len);
+ bool (*update_altmodes)(struct ucsi *ucsi, struct ucsi_altmode *orig,
+ struct ucsi_altmode *updated);
+};
+
+struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops);
+void ucsi_destroy(struct ucsi *ucsi);
+int ucsi_register(struct ucsi *ucsi);
+void ucsi_unregister(struct ucsi *ucsi);
+int ucsi_resume(struct ucsi *ucsi);
+void *ucsi_get_drvdata(struct ucsi *ucsi);
+void ucsi_set_drvdata(struct ucsi *ucsi, void *data);
+
+void ucsi_connector_change(struct ucsi *ucsi, u8 num);
+
+#endif /* __LINUX_USB_UCSI_H */
--
2.44.0.396.g6e790dbe36-goog
On 26/03/2024 00:42, Pavan Holla wrote:
> Implementation of transport driver for UCSI. This driver will be used
> if the ChromeOS EC implements a PPM.
>
> +static struct platform_driver cros_ucsi_driver = {
> + .driver = {
> + .name = DRV_NAME,
> + .pm = &cros_ucsi_pm_ops,
> + },
> + .probe = cros_ucsi_probe,
> + .remove = cros_ucsi_remove,
> +};
> +
> +module_platform_driver(cros_ucsi_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("UCSI driver for ChromeOS EC.");
> +MODULE_ALIAS("platform:" DRV_NAME);
Nothing improved.
One patchset per 24h. Allow people to actually review your code.
Best regards,
Krzysztof
Hi Krzysztof,
Thanks for the review.
On Tue, Mar 26, 2024 at 1:47 AM Krzysztof Kozlowski <[email protected]> wrote:
> Nothing improved.
Yes. I only added maintainers of drivers/platform/chrome in v2. I am
still investigating why MODULE_ALIAS() is required.
> One patchset per 24h. Allow people to actually review your code.
Thanks for letting me know. I will throttle future patch uploads.
Best regards,
Pavan
On 27/03/2024 04:39, Pavan Holla wrote:
> Hi Krzysztof,
>
> Thanks for the review.
>
> On Tue, Mar 26, 2024 at 1:47 AM Krzysztof Kozlowski <[email protected]> wrote:
>> Nothing improved.
>
> Yes. I only added maintainers of drivers/platform/chrome in v2. I am
> still investigating why MODULE_ALIAS() is required.
Heh, I wrote why. You miss ID table.
Best regards,
Krzysztof
On Mon, Mar 25, 2024 at 11:42:26PM +0000, Pavan Holla wrote:
> Import include/linux/usb/ucsi.h and remove any duplicate declarations.
>
> Signed-off-by: Pavan Holla <[email protected]>
> ---
> drivers/usb/typec/ucsi/ucsi.h | 54 +------------------------------------------
> 1 file changed, 1 insertion(+), 53 deletions(-)
I'm pretty sure somebody already told you to merge this into 1/3, so
why haven't you done that?
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 32daf5f58650..167951538030 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -10,22 +10,13 @@
> #include <linux/usb/typec.h>
> #include <linux/usb/pd.h>
> #include <linux/usb/role.h>
> +#include <linux/usb/ucsi.h>
> #include <asm/unaligned.h>
>
> /* -------------------------------------------------------------------------- */
>
> -struct ucsi;
> -struct ucsi_altmode;
> struct dentry;
>
> -/* UCSI offsets (Bytes) */
> -#define UCSI_VERSION 0
> -#define UCSI_CCI 4
> -#define UCSI_CONTROL 8
> -#define UCSI_MESSAGE_IN 16
> -#define UCSI_MESSAGE_OUT 32
> -#define UCSIv2_MESSAGE_OUT 272
> -
> /* UCSI versions */
> #define UCSI_VERSION_1_2 0x0120
> #define UCSI_VERSION_2_0 0x0200
> @@ -42,48 +33,6 @@ struct dentry;
> */
> #define UCSI_SPEC_REVISION_TO_BCD(_v_) (((_v_) + 1) << 8)
>
> -/* Command Status and Connector Change Indication (CCI) bits */
> -#define UCSI_CCI_CONNECTOR(_c_) (((_c_) & GENMASK(7, 1)) >> 1)
> -#define UCSI_CCI_LENGTH(_c_) (((_c_) & GENMASK(15, 8)) >> 8)
> -#define UCSI_CCI_NOT_SUPPORTED BIT(25)
> -#define UCSI_CCI_CANCEL_COMPLETE BIT(26)
> -#define UCSI_CCI_RESET_COMPLETE BIT(27)
> -#define UCSI_CCI_BUSY BIT(28)
> -#define UCSI_CCI_ACK_COMPLETE BIT(29)
> -#define UCSI_CCI_ERROR BIT(30)
> -#define UCSI_CCI_COMMAND_COMPLETE BIT(31)
> -
> -/**
> - * struct ucsi_operations - UCSI I/O operations
> - * @read: Read operation
> - * @sync_write: Blocking write operation
> - * @async_write: Non-blocking write operation
> - * @update_altmodes: Squashes duplicate DP altmodes
> - *
> - * Read and write routines for UCSI interface. @sync_write must wait for the
> - * Command Completion Event from the PPM before returning, and @async_write must
> - * return immediately after sending the data to the PPM.
> - */
> -struct ucsi_operations {
> - int (*read)(struct ucsi *ucsi, unsigned int offset,
> - void *val, size_t val_len);
> - int (*sync_write)(struct ucsi *ucsi, unsigned int offset,
> - const void *val, size_t val_len);
> - int (*async_write)(struct ucsi *ucsi, unsigned int offset,
> - const void *val, size_t val_len);
> - bool (*update_altmodes)(struct ucsi *ucsi, struct ucsi_altmode *orig,
> - struct ucsi_altmode *updated);
> -};
> -
> -struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops);
> -void ucsi_destroy(struct ucsi *ucsi);
> -int ucsi_register(struct ucsi *ucsi);
> -void ucsi_unregister(struct ucsi *ucsi);
> -void *ucsi_get_drvdata(struct ucsi *ucsi);
> -void ucsi_set_drvdata(struct ucsi *ucsi, void *data);
> -
> -void ucsi_connector_change(struct ucsi *ucsi, u8 num);
> -
> /* -------------------------------------------------------------------------- */
>
> /* Commands */
> @@ -465,7 +414,6 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command,
> void *retval, size_t size);
>
> void ucsi_altmode_update_active(struct ucsi_connector *con);
> -int ucsi_resume(struct ucsi *ucsi);
>
> #if IS_ENABLED(CONFIG_POWER_SUPPLY)
> int ucsi_register_port_psy(struct ucsi_connector *con);
>
> --
> 2.44.0.396.g6e790dbe36-goog
--
heikki
On Wed, Mar 27, 2024 at 4:22 AM Heikki Krogerus
<[email protected]> wrote:
> I'm pretty sure somebody already told you to merge this into 1/3, so
> why haven't you done that?
v2 was uploaded a few minutes after v1, and only added a few
maintainers of drivers/platform/chrome to the email chain. I was asked
to merge commits after uploading the last version (v2). In hindsight,
I could have waited for a review on v1, or added new email recipients
in a reply to v1.
I am working on some comments from an internal review for commit 3/3,
after which I will upload v3.
Thanks,
Pavan
On Tue, Mar 26, 2024 at 9:59 PM Krzysztof Kozlowski <[email protected]> wrote:
>
> On 27/03/2024 04:39, Pavan Holla wrote:
> > Hi Krzysztof,
> >
> > Thanks for the review.
> >
> > On Tue, Mar 26, 2024 at 1:47 AM Krzysztof Kozlowski <[email protected]> wrote:
> >> Nothing improved.
> >
> > Yes. I only added maintainers of drivers/platform/chrome in v2. I am
> > still investigating why MODULE_ALIAS() is required.
>
> Heh, I wrote why. You miss ID table.
This driver is going to be used by the cros_ec_dev.c MFD. The UCSI device doesn’t
have an ACPI or OF entry, so I am not sure how I can use MODULE_DEVICE_TABLE
here. If I don’t use MODULE_ALIAS(“platform:” DRV_NAME),
https://elixir.bootlin.com/linux/latest/source/drivers/mfd/cros_ec_dev.c#L206
isn’t able to automatically associate the driver with the device at boot.
I haven’t upstreamed the change in cros_ec_dev.c yet, but the code is similar to
existing code for drivers/platform/chrome/cros_usbpd_logger.c. There are many
other occurrences of the same MODULE_ALIAS pattern:
~/v6.6$ grep -r 'MODULE_ALIAS("platform:" DRV_NAME)' * | wc -l
93
I am still trying to figure out why the “platform:” string allows cros_ec_dev.c to
load the driver. I suspect it might be due to
https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L1257.
Thanks,
Pavan
On 28/03/2024 03:32, Pavan Holla wrote:
> On Tue, Mar 26, 2024 at 9:59 PM Krzysztof Kozlowski <[email protected]> wrote:
>>
>> On 27/03/2024 04:39, Pavan Holla wrote:
>>> Hi Krzysztof,
>>>
>>> Thanks for the review.
>>>
>>> On Tue, Mar 26, 2024 at 1:47 AM Krzysztof Kozlowski <[email protected]> wrote:
>>>> Nothing improved.
>>>
>>> Yes. I only added maintainers of drivers/platform/chrome in v2. I am
>>> still investigating why MODULE_ALIAS() is required.
>>
>> Heh, I wrote why. You miss ID table.
>
> This driver is going to be used by the cros_ec_dev.c MFD. The UCSI device doesn’t
> have an ACPI or OF entry, so I am not sure how I can use MODULE_DEVICE_TABLE
> here. If I don’t use MODULE_ALIAS(“platform:” DRV_NAME),
> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/cros_ec_dev.c#L206
> isn’t able to automatically associate the driver with the device at boot.
> I haven’t upstreamed the change in cros_ec_dev.c yet, but the code is similar to
> existing code for drivers/platform/chrome/cros_usbpd_logger.c. There are many
> other occurrences of the same MODULE_ALIAS pattern:
Just open other platform drivers and look how it is done there. Or ask
colleagues. There is absolutely no one in entire Chromium/google who
ever wrote platform_driver? platform_driver has ID table for matching.
Otherwise how do you expect this to be matched? How your driver is being
matched and device bound? By fallback, right? So what is the primary method?
Best regards,
Krzysztof
On Thu, Mar 28, 2024 at 09:36:54AM +0100, Krzysztof Kozlowski wrote:
> On 28/03/2024 03:32, Pavan Holla wrote:
> > On Tue, Mar 26, 2024 at 9:59 PM Krzysztof Kozlowski <[email protected]> wrote:
> >>
> >> On 27/03/2024 04:39, Pavan Holla wrote:
> >>> Hi Krzysztof,
> >>>
> >>> Thanks for the review.
> >>>
> >>> On Tue, Mar 26, 2024 at 1:47 AM Krzysztof Kozlowski <[email protected]> wrote:
> >>>> Nothing improved.
> >>>
> >>> Yes. I only added maintainers of drivers/platform/chrome in v2. I am
> >>> still investigating why MODULE_ALIAS() is required.
> >>
> >> Heh, I wrote why. You miss ID table.
> >
> > This driver is going to be used by the cros_ec_dev.c MFD. The UCSI device doesn’t
> > have an ACPI or OF entry, so I am not sure how I can use MODULE_DEVICE_TABLE
> > here. If I don’t use MODULE_ALIAS(“platform:” DRV_NAME),
> > https://elixir.bootlin.com/linux/latest/source/drivers/mfd/cros_ec_dev.c#L206
> > isn’t able to automatically associate the driver with the device at boot.
> > I haven’t upstreamed the change in cros_ec_dev.c yet, but the code is similar to
> > existing code for drivers/platform/chrome/cros_usbpd_logger.c. There are many
> > other occurrences of the same MODULE_ALIAS pattern:
>
> Just open other platform drivers and look how it is done there. Or ask
> colleagues. There is absolutely no one in entire Chromium/google who
> ever wrote platform_driver? platform_driver has ID table for matching.
>
> Otherwise how do you expect this to be matched? How your driver is being
> matched and device bound? By fallback, right? So what is the primary method?
Those platform devices are adding in drivers/mfd/cros_ec_dev.c via
mfd_add_hotplug_devices().
By looking other use cases of mfd_add_hotplug_devices():
$ grep -R --files-with-matches mfd_add_hotplug_devices drivers/mfd/
drivers/mfd/dln2.c
drivers/mfd/cros_ec_dev.c
drivers/mfd/viperboard.c
They also have no ID tables and need MODULE_ALIAS().
- drivers/gpio/gpio-dln2.c
- drivers/i2c/busses/i2c-dln2.c
- drivers/spi/spi-dln2.c
- drivers/iio/adc/dln2-adc.c
- drivers/gpio/gpio-viperboard.c
- drivers/i2c/busses/i2c-viperboard.c
- drivers/iio/adc/viperboard_adc.c
I'm not sure whether using the path results in:
- Lack of device ID table.
- Need MODULE_ALIAS().
in the platform device drivers. And perhaps it relies on the fallback match?
On 28/03/2024 10:57, Tzung-Bi Shih wrote:
> On Thu, Mar 28, 2024 at 09:36:54AM +0100, Krzysztof Kozlowski wrote:
>> On 28/03/2024 03:32, Pavan Holla wrote:
>>> On Tue, Mar 26, 2024 at 9:59 PM Krzysztof Kozlowski <[email protected]> wrote:
>>>>
>>>> On 27/03/2024 04:39, Pavan Holla wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>> Thanks for the review.
>>>>>
>>>>> On Tue, Mar 26, 2024 at 1:47 AM Krzysztof Kozlowski <[email protected]> wrote:
>>>>>> Nothing improved.
>>>>>
>>>>> Yes. I only added maintainers of drivers/platform/chrome in v2. I am
>>>>> still investigating why MODULE_ALIAS() is required.
>>>>
>>>> Heh, I wrote why. You miss ID table.
>>>
>>> This driver is going to be used by the cros_ec_dev.c MFD. The UCSI device doesn’t
>>> have an ACPI or OF entry, so I am not sure how I can use MODULE_DEVICE_TABLE
>>> here. If I don’t use MODULE_ALIAS(“platform:” DRV_NAME),
>>> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/cros_ec_dev.c#L206
>>> isn’t able to automatically associate the driver with the device at boot.
>>> I haven’t upstreamed the change in cros_ec_dev.c yet, but the code is similar to
>>> existing code for drivers/platform/chrome/cros_usbpd_logger.c. There are many
>>> other occurrences of the same MODULE_ALIAS pattern:
>>
>> Just open other platform drivers and look how it is done there. Or ask
>> colleagues. There is absolutely no one in entire Chromium/google who
>> ever wrote platform_driver? platform_driver has ID table for matching.
>>
>> Otherwise how do you expect this to be matched? How your driver is being
>> matched and device bound? By fallback, right? So what is the primary method?
>
> Those platform devices are adding in drivers/mfd/cros_ec_dev.c via
> mfd_add_hotplug_devices().
This is not matching.
>
> By looking other use cases of mfd_add_hotplug_devices():
> $ grep -R --files-with-matches mfd_add_hotplug_devices drivers/mfd/
> drivers/mfd/dln2.c
> drivers/mfd/cros_ec_dev.c
> drivers/mfd/viperboard.c
>
> They also have no ID tables and need MODULE_ALIAS().
> - drivers/gpio/gpio-dln2.c
> - drivers/i2c/busses/i2c-dln2.c
> - drivers/spi/spi-dln2.c
> - drivers/iio/adc/dln2-adc.c
> - drivers/gpio/gpio-viperboard.c
> - drivers/i2c/busses/i2c-viperboard.c
> - drivers/iio/adc/viperboard_adc.c
So if there is a bug in some driver, you are allowed to add it? :) There
is plenty of poor examples, so what I was suggesting to look for good
examples. I agree that itself might be a tricky task.
> I'm not sure whether using the path results in:
> - Lack of device ID table.
> - Need MODULE_ALIAS().
> in the platform device drivers. And perhaps it relies on the fallback match?
Guys, think for a sec. If you are adding module alias being equivalent
to platform ID table entry, then why you are not using the platform ID
table entry in the first? That's the entire point.
So to repeat myself:
If you need it, usually it means your device ID table is wrong (e.g.
misses either entries or MODULE_DEVICE_TABLE()).
MODULE_ALIAS() is not a substitute for incomplete ID table.
Best regards,
Krzysztof
On 26/03/2024 01:42, Pavan Holla wrote:
> Implementation of transport driver for UCSI. This driver will be used
> if the ChromeOS EC implements a PPM.
>
> Signed-off-by: Pavan Holla <[email protected]>
> ---
> drivers/platform/chrome/Kconfig | 14 ++
> drivers/platform/chrome/Makefile | 1 +
> drivers/platform/chrome/cros_ec_ucsi.c | 247 +++++++++++++++++++++++++
> include/linux/platform_data/cros_ec_commands.h | 19 ++
While it's fine to use platform/chrome for platform drivers, please
place drivers which have a subsystem into the subsystem dir. I think we
don't want to hunt UCSI implementations all over the codebase. Please
use drivers/usb/typec/ucsi/ location for your driver. This also removes
a need for a global header.
--
With best wishes
Dmitry
On Thu, Mar 28, 2024 at 01:16:36PM +0100, Krzysztof Kozlowski wrote:
> On 28/03/2024 10:57, Tzung-Bi Shih wrote:
> > By looking other use cases of mfd_add_hotplug_devices():
> > $ grep -R --files-with-matches mfd_add_hotplug_devices drivers/mfd/
> > drivers/mfd/dln2.c
> > drivers/mfd/cros_ec_dev.c
> > drivers/mfd/viperboard.c
> >
> > They also have no ID tables and need MODULE_ALIAS().
> > - drivers/gpio/gpio-dln2.c
> > - drivers/i2c/busses/i2c-dln2.c
> > - drivers/spi/spi-dln2.c
> > - drivers/iio/adc/dln2-adc.c
> > - drivers/gpio/gpio-viperboard.c
> > - drivers/i2c/busses/i2c-viperboard.c
> > - drivers/iio/adc/viperboard_adc.c
>
> So if there is a bug in some driver, you are allowed to add it? :) There
> is plenty of poor examples, so what I was suggesting to look for good
> examples. I agree that itself might be a tricky task.
>
> > I'm not sure whether using the path results in:
> > - Lack of device ID table.
> > - Need MODULE_ALIAS().
> > in the platform device drivers. And perhaps it relies on the fallback match?
>
> Guys, think for a sec. If you are adding module alias being equivalent
> to platform ID table entry, then why you are not using the platform ID
> table entry in the first? That's the entire point.
>
> So to repeat myself:
> If you need it, usually it means your device ID table is wrong (e.g.
> misses either entries or MODULE_DEVICE_TABLE()).
>
> MODULE_ALIAS() is not a substitute for incomplete ID table.
Thanks. I see.
Yet another example in: drivers/mfd/max8997.c and drivers/rtc/rtc-max8997.c.
We should be able to use a platform_device_id, MODULE_DEVICE_TABLE(), and
`.id_table` for matching.
If that works, I think we need to turn most MODULE_ALIAS() use cases in
drivers/platform/chrome/ to this way afterward.
Hi Dmitry,
Thanks for the review.
On Thu, Mar 28, 2024 at 8:32 AM Dmitry Baryshkov
<[email protected]> wrote:
> While it's fine to use platform/chrome for platform drivers, please
> place drivers which have a subsystem into the subsystem dir. I think we
> don't want to hunt UCSI implementations all over the codebase. Please
> use drivers/usb/typec/ucsi/ location for your driver. This also removes
> a need for a global header.
I agree with your assessment that drivers/usb/typec/ucsi/ is a good
location for the driver currently. However, the driver is still in
early stages of development. We may have to account for PDC/ EC quirks
(we have multiple vendors), add FW update functionality outside the
UCSI spec, or add PDC logging functionality. While I'd like to write
separate drivers for those, some of this functionality will need to
acquire a lock over communication to the PDC. Even if I were to write
separate drivers for new functionality related to the PDC, maybe it's
better for all ChromeOS PDC related drivers to reside in the same
location. I am not sure what form this driver will take in the near
future, thus I would prefer to keep it in platform/chrome. Maybe
cros_ec_ucsi isn't the best name here, cros_ec_pdc probably conveys
the intention better.
Thanks,
Pavan
On Fri, 29 Mar 2024 at 17:09, Pavan Holla <[email protected]> wrote:
>
> Hi Dmitry,
>
> Thanks for the review.
>
> On Thu, Mar 28, 2024 at 8:32 AM Dmitry Baryshkov
> <[email protected]> wrote:
> > While it's fine to use platform/chrome for platform drivers, please
> > place drivers which have a subsystem into the subsystem dir. I think we
> > don't want to hunt UCSI implementations all over the codebase. Please
> > use drivers/usb/typec/ucsi/ location for your driver. This also removes
> > a need for a global header.
>
> I agree with your assessment that drivers/usb/typec/ucsi/ is a good
> location for the driver currently. However, the driver is still in
> early stages of development. We may have to account for PDC/ EC quirks
> (we have multiple vendors), add FW update functionality outside the
> UCSI spec, or add PDC logging functionality. While I'd like to write
> separate drivers for those, some of this functionality will need to
> acquire a lock over communication to the PDC. Even if I were to write
> separate drivers for new functionality related to the PDC, maybe it's
> better for all ChromeOS PDC related drivers to reside in the same
> location. I am not sure what form this driver will take in the near
> future, thus I would prefer to keep it in platform/chrome. Maybe
> cros_ec_ucsi isn't the best name here, cros_ec_pdc probably conveys
> the intention better.
In such a case please consider using auxilliary device bus or MFD
cells to describe pdc / ucsi relationship. See how this is handled for
pmic-glink / ucsi_glink.
The drivers/platform should really be limited to simple drivers, which
don't have a better place. Otherwise it becomes a nightmare to handle
driver changes (this applies not only to the UCSI but also to other
drivers which have their own subsystem tree).
--
With best wishes
Dmitry
On Fri, Mar 29, 2024 at 8:13 AM Dmitry Baryshkov
<[email protected]> wrote:
>
> On Fri, 29 Mar 2024 at 17:09, Pavan Holla <[email protected]> wrote:
> >
> > Hi Dmitry,
> >
> > Thanks for the review.
> >
> > On Thu, Mar 28, 2024 at 8:32 AM Dmitry Baryshkov
> > <[email protected]> wrote:
> > > While it's fine to use platform/chrome for platform drivers, please
> > > place drivers which have a subsystem into the subsystem dir. I think we
> > > don't want to hunt UCSI implementations all over the codebase. Please
> > > use drivers/usb/typec/ucsi/ location for your driver. This also removes
> > > a need for a global header.
> >
> > I agree with your assessment that drivers/usb/typec/ucsi/ is a good
> > location for the driver currently. However, the driver is still in
> > early stages of development. We may have to account for PDC/ EC quirks
> > (we have multiple vendors), add FW update functionality outside the
> > UCSI spec, or add PDC logging functionality. While I'd like to write
> > separate drivers for those, some of this functionality will need to
> > acquire a lock over communication to the PDC. Even if I were to write
> > separate drivers for new functionality related to the PDC, maybe it's
> > better for all ChromeOS PDC related drivers to reside in the same
> > location. I am not sure what form this driver will take in the near
> > future, thus I would prefer to keep it in platform/chrome. Maybe
> > cros_ec_ucsi isn't the best name here, cros_ec_pdc probably conveys
> > the intention better.
>
> In such a case please consider using auxilliary device bus or MFD
> cells to describe pdc / ucsi relationship. See how this is handled for
> pmic-glink / ucsi_glink.
> The drivers/platform should really be limited to simple drivers, which
> don't have a better place. Otherwise it becomes a nightmare to handle
> driver changes (this applies not only to the UCSI but also to other
> drivers which have their own subsystem tree).
Thanks for the pointers. I will move the driver to drivers/usb/typec/ucsi/