This series implements a UCSI ChromeOS EC transport driver.
The ChromeOS EC is expected to implement a UCSI PPM.
Signed-off-by: Pavan Holla <[email protected]>
---
Changes in v4:
- Setup notifications before calling ucsi_register.
- Cancel work before destroying driver data.
- Link to v3: https://lore.kernel.org/r/[email protected]
Changes in v3:
- Moved driver from platform/chrome to usb/typec/ucsi.
- Used id_table instead of MODULE_ALIAS.
- Split EC header changes into seperate commit.
- Fixes from additional internal reviews and kernel bot warnings.
- Link to v2: https://lore.kernel.org/r/[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 (2):
platform/chrome: Update ChromeOS EC header for UCSI
usb: typec: ucsi: Implement ChromeOS UCSI driver
drivers/usb/typec/ucsi/Kconfig | 13 ++
drivers/usb/typec/ucsi/Makefile | 1 +
drivers/usb/typec/ucsi/cros_ec_ucsi.c | 259 +++++++++++++++++++++++++
include/linux/platform_data/cros_ec_commands.h | 20 ++
4 files changed, 293 insertions(+)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240325-public-ucsi-h-3ecee4106a58
Best regards,
--
Pavan Holla <[email protected]>
Add EC host commands for reading and writing UCSI structures
in the EC. The corresponding kernel driver is cros-ec-ucsi.
Also update PD events supported by the EC.
Signed-off-by: Pavan Holla <[email protected]>
---
include/linux/platform_data/cros_ec_commands.h | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index ecc47d5fe239..c0f6d054a566 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -4933,6 +4933,8 @@ 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_TYPEC BIT(4)
+#define PD_EVENT_PPM BIT(5)
struct ec_response_host_event_status {
uint32_t status; /* PD MCU host event status */
} __ec_align4;
@@ -5994,6 +5996,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.478.gd926399ef9-goog
Implementation of a UCSI transport driver for ChromeOS.
This driver will be loaded if the ChromeOS EC implements a PPM.
Signed-off-by: Pavan Holla <[email protected]>
---
drivers/usb/typec/ucsi/Kconfig | 13 ++
drivers/usb/typec/ucsi/Makefile | 1 +
drivers/usb/typec/ucsi/cros_ec_ucsi.c | 259 ++++++++++++++++++++++++++++++++++
3 files changed, 273 insertions(+)
diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
index bdcb1764cfae..4dceb14a66ee 100644
--- a/drivers/usb/typec/ucsi/Kconfig
+++ b/drivers/usb/typec/ucsi/Kconfig
@@ -69,4 +69,17 @@ config UCSI_PMIC_GLINK
To compile the driver as a module, choose M here: the module will be
called ucsi_glink.
+config CROS_EC_UCSI
+ tristate "UCSI Driver for ChromeOS EC"
+ depends on MFD_CROS_EC_DEV
+ depends on CROS_USBPD_NOTIFY
+ 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.
+
endif
diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
index b4679f94696b..cb336eee055c 100644
--- a/drivers/usb/typec/ucsi/Makefile
+++ b/drivers/usb/typec/ucsi/Makefile
@@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o
obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
obj-$(CONFIG_UCSI_STM32G0) += ucsi_stm32g0.o
obj-$(CONFIG_UCSI_PMIC_GLINK) += ucsi_glink.o
+obj-$(CONFIG_CROS_EC_UCSI) += cros_ec_ucsi.o
diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c
new file mode 100644
index 000000000000..487bcfac48fa
--- /dev/null
+++ b/drivers/usb/typec/ucsi/cros_ec_ucsi.c
@@ -0,0 +1,259 @@
+// 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/mod_devicetable.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/wait.h>
+
+#include "ucsi.h"
+
+#define DRV_NAME "cros-ec-ucsi"
+
+#define MAX_EC_DATA_SIZE 256
+#define WRITE_TMO_MS 500
+
+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;
+
+ if (val_len > MAX_EC_DATA_SIZE) {
+ dev_err(udata->dev, "Can't write %zu bytes. Too big.", val_len);
+ return -EINVAL;
+ }
+
+ req->offset = offset;
+ memcpy(req->data, val, val_len);
+ ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_SET,
+ req, sizeof(*req), 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 void cros_ucsi_destroy(struct cros_ucsi_data *udata)
+{
+ cros_usbpd_unregister_notify(&udata->nb);
+ cancel_work_sync(&udata->work);
+ ucsi_destroy(udata->ucsi);
+}
+
+static int cros_ucsi_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cros_ec_dev *ec_data = dev_get_drvdata(dev->parent);
+ struct cros_ucsi_data *udata;
+ int ret;
+
+ udata = devm_kzalloc(dev, sizeof(*udata), GFP_KERNEL);
+ if (!udata)
+ return -ENOMEM;
+
+ udata->dev = dev;
+
+ udata->ec = ec_data->ec_dev;
+ if (!udata->ec) {
+ dev_err(dev, "couldn't find parent EC device");
+ return -ENODEV;
+ }
+
+ platform_set_drvdata(pdev, udata);
+
+ INIT_WORK(&udata->work, cros_ucsi_work);
+ init_completion(&udata->complete);
+
+ udata->ucsi = ucsi_create(dev, &cros_ucsi_ops);
+ if (IS_ERR(udata->ucsi)) {
+ dev_err(dev, "failed to allocate UCSI instance");
+ return PTR_ERR(udata->ucsi);
+ }
+
+ ucsi_set_drvdata(udata->ucsi, udata);
+
+ udata->nb.notifier_call = cros_ucsi_event;
+ ret = cros_usbpd_register_notify(&udata->nb);
+ if (ret) {
+ dev_err(dev, "failed to register notifier: error=%d", ret);
+ ucsi_destroy(udata->ucsi);
+ return ret;
+ }
+
+ ret = ucsi_register(udata->ucsi);
+ if (ret) {
+ dev_err(dev, "failed to register UCSI: error=%d", ret);
+ cros_ucsi_destroy(udata);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int cros_ucsi_remove(struct platform_device *dev)
+{
+ struct cros_ucsi_data *udata = platform_get_drvdata(dev);
+
+ ucsi_unregister(udata->ucsi);
+ cros_ucsi_destroy(udata);
+ return 0;
+}
+
+static int __maybe_unused cros_ucsi_suspend(struct device *dev)
+{
+ struct cros_ucsi_data *udata = dev_get_drvdata(dev);
+
+ cancel_work_sync(&udata->work);
+
+ return 0;
+}
+
+static int __maybe_unused 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 const struct platform_device_id cros_ucsi_id[] = {
+ { DRV_NAME, 0},
+ {}
+};
+MODULE_DEVICE_TABLE(platform, cros_ucsi_id);
+
+static struct platform_driver cros_ucsi_driver = {
+ .driver = {
+ .name = DRV_NAME,
+ .pm = &cros_ucsi_pm_ops,
+ },
+ .id_table = cros_ucsi_id,
+ .probe = cros_ucsi_probe,
+ .remove = cros_ucsi_remove,
+};
+
+module_platform_driver(cros_ucsi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("UCSI driver for ChromeOS EC");
--
2.44.0.478.gd926399ef9-goog
On Tue, Apr 09, 2024 at 02:27:37AM +0000, Pavan Holla wrote:
> Implementation of a UCSI transport driver for ChromeOS.
The patch generally looks good to me just a few nits.
> This driver will be loaded if the ChromeOS EC implements a PPM.
Replied in v3, the patch (and the series) don't reflect the message.
> diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c
> [...]
> +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;
`ret` can be dropped.
> +static const struct platform_device_id cros_ucsi_id[] = {
> + { DRV_NAME, 0},
To be neat, append an extra space after `0`.
On Tue, Apr 09, 2024 at 02:27:37AM +0000, Pavan Holla wrote:
> +#define DRV_NAME "cros-ec-ucsi"
KBUILD_MODNAME?
> +
> +#define MAX_EC_DATA_SIZE 256
> +#define WRITE_TMO_MS 500
What are these and why these values? And tabs perhaps?
> +
> +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)];
That's a lot of data on the stack, are you sure you want to do that?
And are you sure you are allowed to have this data on the stack? It
never ends up being sent using DMA?
And please, don't use non-kernel types like "uint8_t", use "u8" like
intended. This isn't userspace (yes, I know a lot of kernel code uses
these, but as you are going to change this, might as well change that
too.)
thanks,
greg k-h
Ack to all comments.
I plan to upload the next version after related EC changes land.
I plan to upload the next version after related EC changes land on
ChromeOS. That might take a few weeks.
On Tue, Apr 9, 2024 at 8:16 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Tue, Apr 09, 2024 at 02:27:37AM +0000, Pavan Holla wrote:
> > +#define DRV_NAME "cros-ec-ucsi"
>
> KBUILD_MODNAME?
Will replace DRV_NAME with KBUILD_MODNAME.
> > +
> > +#define MAX_EC_DATA_SIZE 256
> > +#define WRITE_TMO_MS 500
>
> What are these and why these values? And tabs perhaps?
MAX_EC_DATA_SIZE is the number of bytes that can be read or written to in the
UCSI data structure using a single host command to the EC.
WRITE_TMO_MS is the time within which a cmd complete or ack notification must
arrive after a command is sent to the PPM.
Will add comments and tabs.
> > + uint8_t ec_buffer[MAX_EC_DATA_SIZE + sizeof(struct ec_params_ucsi_ppm_set)];
>
> That's a lot of data on the stack, are you sure you want to do that?
>
> And are you sure you are allowed to have this data on the stack? It
> never ends up being sent using DMA?
I confirmed that this data isn't DMA'ed. However, I don't mind putting
it on the heap, and will do so in the next version.
> And please, don't use non-kernel types like "uint8_t", use "u8" like
> intended. This isn't userspace (yes, I know a lot of kernel code uses
> these, but as you are going to change this, might as well change that
> too.)
Ack.
Thanks,
Pavan