| Message ID | 20241206153813.v4.5.I142fc0c09df58689b98f0cebf1c5e48b9d4fa800@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Thunderbolt and DP altmode support for cros-ec-typec | expand |
Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16) > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c > index e3eabe5e42ac..0f3bc335f583 100644 > --- a/drivers/platform/chrome/cros_ec_typec.c > +++ b/drivers/platform/chrome/cros_ec_typec.c > @@ -18,6 +18,7 @@ > > #include "cros_ec_typec.h" > #include "cros_typec_vdm.h" > +#include "cros_typec_altmode.h" > > #define DRV_NAME "cros-ec-typec" > > @@ -290,15 +291,15 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec, > struct typec_altmode *amode; > > /* All PD capable CrOS devices are assumed to support DP altmode. */ > + memset(&desc, 0, sizeof(desc)); > desc.svid = USB_TYPEC_DP_SID; > desc.mode = USB_TYPEC_DP_MODE; > desc.vdo = DP_PORT_VDO; > - amode = typec_port_register_altmode(port->port, &desc); > + amode = cros_typec_register_displayport(port, &desc, > + typec->ap_driven_altmode); > if (IS_ERR(amode)) > return PTR_ERR(amode); > port->port_altmode[CROS_EC_ALTMODE_DP] = amode; > - typec_altmode_set_drvdata(amode, port); > - amode->ops = &port_amode_ops; > > /* > * Register TBT compatibility alt mode. The EC will not enter the mode > @@ -575,6 +576,10 @@ static int cros_typec_enable_dp(struct cros_typec_data *typec, > if (!ret) > ret = typec_mux_set(port->mux, &port->state); > > + if (!ret) > + cros_typec_displayport_status_update(port->state.alt, Should we forward the return value from here? > + port->state.data); > + > return ret; > } > > @@ -1254,6 +1259,8 @@ static int cros_typec_probe(struct platform_device *pdev) > > typec->typec_cmd_supported = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_CMD); > typec->needs_mux_ack = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK); > + typec->ap_driven_altmode = cros_ec_check_features( > + ec_dev, EC_FEATURE_TYPEC_REQUIRE_AP_MODE_ENTRY); > > ret = cros_ec_cmd(typec->ec, 0, EC_CMD_USB_PD_PORTS, NULL, 0, > &resp, sizeof(resp)); > diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h > index deda180a646f..9fd5342bb0ad 100644 > --- a/drivers/platform/chrome/cros_ec_typec.h > +++ b/drivers/platform/chrome/cros_ec_typec.h > @@ -39,6 +39,7 @@ struct cros_typec_data { > struct work_struct port_work; > bool typec_cmd_supported; > bool needs_mux_ack; > + bool ap_driven_altmode; Do we need to stash this? Can we cros_ec_check_features() in cros_typec_init_ports() and pass the bool to cros_typec_register_port_altmodes() instead to save a struct member? > }; > > /* Per port data. */ > diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c > new file mode 100644 > index 000000000000..bb7c7ad2ff6e > --- /dev/null > +++ b/drivers/platform/chrome/cros_typec_altmode.c > @@ -0,0 +1,281 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Alt-mode implementation on ChromeOS EC. > + * > + * Copyright 2024 Google LLC > + * Author: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > + */ > +#include "cros_ec_typec.h" > + > +#include <linux/usb/typec_dp.h> > +#include <linux/usb/pd_vdo.h> Please include workqueue.h, mutex.h, etc. for things used in this file. > + > +#include "cros_typec_altmode.h" > + > +struct cros_typec_altmode_data { > + struct work_struct work; > + struct cros_typec_port *port; > + struct typec_altmode *alt; > + bool ap_mode_entry; The UCSI driver (drivers/usb/typec/ucsi/displayport.c) calls this 'override', can it be named the same thing? I also see that the UCSI driver has two bools, 'override' and 'initialized', which seems to be to support a DP_CMD_CONFIGURE that will respond with an ACK and then the next DP_CMD_CONFIGURE will call ucsi_altmode_update_active() to set the altmode as active. Maybe the same method can be followed here so that on older chromebooks where EC is in control of mode entry we can emulate entering the mode? > + > + struct mutex lock; > + u32 header; > + u32 *vdo_data; > + u8 vdo_size; > + > + u16 sid; > + u8 mode; > +}; > + > +struct cros_typec_dp_data { > + struct cros_typec_altmode_data adata; > + struct typec_displayport_data data; > + bool configured; > + bool pending_status_update; > +}; > + > +static void cros_typec_altmode_work(struct work_struct *work) > +{ > + struct cros_typec_altmode_data *data = > + container_of(work, struct cros_typec_altmode_data, work); > + > + mutex_lock(&data->lock); > + > + if (typec_altmode_vdm(data->alt, data->header, data->vdo_data, > + data->vdo_size)) > + dev_err(&data->alt->dev, "VDM 0x%x failed", data->header); These printks need newlines. dev_err(&data->alt->dev, "VDM 0x%x failed\n", data->header); > + > + data->header = 0; > + data->vdo_data = NULL; > + data->vdo_size = 0; > + > + mutex_unlock(&data->lock); > +} > + > +static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo) > +{ > + struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt); > + struct ec_params_typec_control req = { > + .port = data->port->port_num, > + .command = TYPEC_CONTROL_COMMAND_ENTER_MODE, > + }; > + int svdm_version; > + int ret; > + > + if (!data->ap_mode_entry) { > + dev_warn(&alt->dev, > + "EC does not support AP driven mode entry\n"); Like this one. > + return -EOPNOTSUPP; > + } > + > + if (data->sid == USB_TYPEC_DP_SID) > + req.mode_to_enter = CROS_EC_ALTMODE_DP; > + else > + return -EOPNOTSUPP; > + > + ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL, Do we plan to delete drivers/platform/chrome/cros_typec_vdm.c file at some point? I ask because 'port_amode_ops' becomes unused after this series. > + &req, sizeof(req), NULL, 0); > + if (ret < 0) > + return ret; > + > + svdm_version = typec_altmode_get_svdm_version(alt); > + if (svdm_version < 0) > + return svdm_version; > + > + mutex_lock(&data->lock); > + > + data->header = VDO(data->sid, 1, svdm_version, CMD_ENTER_MODE); > + data->header |= VDO_OPOS(data->mode); > + data->header |= VDO_CMDT(CMDT_RSP_ACK); > + data->vdo_data = NULL; > + data->vdo_size = 1; > + schedule_work(&data->work); > + > + mutex_unlock(&data->lock); > + return ret; > +} > + > +static int cros_typec_altmode_exit(struct typec_altmode *alt) > +{ > + struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt); > + struct ec_params_typec_control req = { > + .port = data->port->port_num, > + .command = TYPEC_CONTROL_COMMAND_EXIT_MODES, > + }; > + int svdm_version; > + int ret; > + > + if (!data->ap_mode_entry) { > + dev_warn(&alt->dev, > + "EC does not support AP driven mode exit\n"); > + return -EOPNOTSUPP; > + } > + > + ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL, > + &req, sizeof(req), NULL, 0); > + > + if (ret < 0) > + return ret; > + > + svdm_version = typec_altmode_get_svdm_version(alt); > + if (svdm_version < 0) > + return svdm_version; > + > + mutex_lock(&data->lock); > + > + data->header = VDO(data->sid, 1, svdm_version, CMD_EXIT_MODE); > + data->header |= VDO_OPOS(data->mode); > + data->header |= VDO_CMDT(CMDT_RSP_ACK); > + data->vdo_data = NULL; > + data->vdo_size = 1; > + schedule_work(&data->work); > + > + mutex_unlock(&data->lock); > + return ret; > +} > + > +static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header, > + const u32 *data, int count) > +{ > + struct cros_typec_dp_data *dp_data = typec_altmode_get_drvdata(alt); > + struct cros_typec_altmode_data *adata = &dp_data->adata; > + > + > + int cmd_type = PD_VDO_CMDT(header); > + int cmd = PD_VDO_CMD(header); > + int svdm_version; > + > + svdm_version = typec_altmode_get_svdm_version(alt); > + if (svdm_version < 0) > + return svdm_version; > + > + mutex_lock(&adata->lock); > + > + switch (cmd_type) { > + case CMDT_INIT: > + if (PD_VDO_SVDM_VER(header) < svdm_version) { > + typec_partner_set_svdm_version(adata->port->partner, > + PD_VDO_SVDM_VER(header)); > + svdm_version = PD_VDO_SVDM_VER(header); > + } > + > + adata->header = VDO(adata->sid, 1, svdm_version, cmd); > + adata->header |= VDO_OPOS(adata->mode); > + > + /* > + * DP_CMD_CONFIGURE: We can't actually do anything with the > + * provided VDO yet so just send back an ACK. > + * > + * DP_CMD_STATUS_UPDATE: We wait for Mux changes to send > + * DPStatus Acks. > + */ > + switch (cmd) { > + case DP_CMD_CONFIGURE: > + dp_data->data.conf = *data; > + adata->header |= VDO_CMDT(CMDT_RSP_ACK); > + dp_data->configured = true; > + schedule_work(&adata->work); > + break; > + case DP_CMD_STATUS_UPDATE: > + dp_data->pending_status_update = true; > + break; > + default: > + adata->header |= VDO_CMDT(CMDT_RSP_ACK); > + schedule_work(&adata->work); > + break; > + } > + > + break; > + default: > + break; > + } > + > + mutex_unlock(&adata->lock); > + return 0; > +} > + > +static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header, > + const u32 *data, int count) > +{ > + struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt); > + > + if (!adata->ap_mode_entry) > + return -EOPNOTSUPP; > + > + if (adata->sid == USB_TYPEC_DP_SID) > + return cros_typec_displayport_vdm(alt, header, data, count); > + > + return -EINVAL; > +} > + > +static const struct typec_altmode_ops cros_typec_altmode_ops = { > + .enter = cros_typec_altmode_enter, > + .exit = cros_typec_altmode_exit, > + .vdm = cros_typec_altmode_vdm, > +}; > + > +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE) > +int cros_typec_displayport_status_update(struct typec_altmode *altmode, > + struct typec_displayport_data *data) > +{ > + struct cros_typec_dp_data *dp_data = > + typec_altmode_get_drvdata(altmode); > + struct cros_typec_altmode_data *adata = &dp_data->adata; > + > + if (!dp_data->pending_status_update) { > + dev_dbg(&altmode->dev, > + "Got DPStatus without a pending request"); > + return 0; > + } > + > + if (dp_data->configured && dp_data->data.conf != data->conf) > + dev_dbg(&altmode->dev, > + "DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x", > + dp_data->data.conf, data->conf); > + > + mutex_lock(&adata->lock); > + > + dp_data->data = *data; > + dp_data->pending_status_update = false; > + adata->header |= VDO_CMDT(CMDT_RSP_ACK); > + adata->vdo_data = &dp_data->data.status; > + adata->vdo_size = 2; > + schedule_work(&adata->work); > + > + mutex_unlock(&adata->lock); > + > + return 0; > +} > + > +struct typec_altmode * > +cros_typec_register_displayport(struct cros_typec_port *port, > + struct typec_altmode_desc *desc, > + bool ap_mode_entry) > +{ > + struct typec_altmode *alt; > + struct cros_typec_altmode_data *data; Can you name it 'adata' consistently? That makes it easy to search for 'adata' in this file and know it's always talking about a struct cros_typec_altmode_data type of data. > + > + alt = typec_port_register_altmode(port->port, desc); > + if (IS_ERR(alt)) > + return alt; > + > + data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL); > + if (!data) { > + typec_unregister_altmode(alt); > + return ERR_PTR(-ENOMEM); > + } > + > + INIT_WORK(&data->work, cros_typec_altmode_work); > + mutex_init(&data->lock); > + data->alt = alt; > + data->port = port; > + data->ap_mode_entry = ap_mode_entry; > + data->sid = desc->svid; > + data->mode = desc->mode; > + > + typec_altmode_set_ops(alt, &cros_typec_altmode_ops); > + typec_altmode_set_drvdata(alt, data); > + > + return alt; > +} > +#endif > diff --git a/drivers/platform/chrome/cros_typec_altmode.h b/drivers/platform/chrome/cros_typec_altmode.h > new file mode 100644 > index 000000000000..c6f8fb02c99c > --- /dev/null > +++ b/drivers/platform/chrome/cros_typec_altmode.h > @@ -0,0 +1,34 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#ifndef __CROS_TYPEC_ALTMODE_H__ > +#define __CROS_TYPEC_ALTMODE_H__ #include <linux/kconfig.h> for IS_ENABLED() #include <linux/usb/typec.h> for typec_port_register_altmode() > + > +struct cros_typec_port; > +struct typec_altmode; > +struct typec_altmode_desc; > +struct typec_displayport_data; > + > +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE) > +struct typec_altmode * > +cros_typec_register_displayport(struct cros_typec_port *port, > + struct typec_altmode_desc *desc, > + bool ap_mode_entry); > + > +int cros_typec_displayport_status_update(struct typec_altmode *altmode, > + struct typec_displayport_data *data); > +#else > +static inline struct typec_altmode * > +cros_typec_register_displayport(struct cros_typec_port *port, > + struct typec_altmode_desc *desc, > + bool ap_mode_entry) > +{ > + return typec_port_register_altmode(port->port, desc); > +}
Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16) > diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c > new file mode 100644 > index 000000000000..bb7c7ad2ff6e > --- /dev/null > +++ b/drivers/platform/chrome/cros_typec_altmode.c > @@ -0,0 +1,281 @@ [...] > + > +static const struct typec_altmode_ops cros_typec_altmode_ops = { > + .enter = cros_typec_altmode_enter, > + .exit = cros_typec_altmode_exit, > + .vdm = cros_typec_altmode_vdm, > +}; > + > +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE) > +int cros_typec_displayport_status_update(struct typec_altmode *altmode, > + struct typec_displayport_data *data) > +{ > + struct cros_typec_dp_data *dp_data = > + typec_altmode_get_drvdata(altmode); How does this work? I see that the type of the drvdata is struct cros_typec_altmode_data per the allocation in cros_typec_register_displayport(), but here we're treating it as the type struct cros_typec_dp_data, which has a struct cros_typec_altmode_data as the first member. The allocation is too small from what I can tell. The same problem looks to be there in cros_typec_displayport_vdm(). > + struct cros_typec_altmode_data *adata = &dp_data->adata; > + > + if (!dp_data->pending_status_update) { > + dev_dbg(&altmode->dev, > + "Got DPStatus without a pending request"); > + return 0; > + } > + > + if (dp_data->configured && dp_data->data.conf != data->conf) > + dev_dbg(&altmode->dev, > + "DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x", > + dp_data->data.conf, data->conf); > + > + mutex_lock(&adata->lock); > + > + dp_data->data = *data; > + dp_data->pending_status_update = false; > + adata->header |= VDO_CMDT(CMDT_RSP_ACK); > + adata->vdo_data = &dp_data->data.status; > + adata->vdo_size = 2; > + schedule_work(&adata->work); > + > + mutex_unlock(&adata->lock); > + > + return 0; > +} > + > +struct typec_altmode * > +cros_typec_register_displayport(struct cros_typec_port *port, > + struct typec_altmode_desc *desc, > + bool ap_mode_entry) > +{ > + struct typec_altmode *alt; > + struct cros_typec_altmode_data *data; > + > + alt = typec_port_register_altmode(port->port, desc); > + if (IS_ERR(alt)) > + return alt; > + > + data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL); > + if (!data) { > + typec_unregister_altmode(alt); > + return ERR_PTR(-ENOMEM); > + } > + > + INIT_WORK(&data->work, cros_typec_altmode_work); > + mutex_init(&data->lock); > + data->alt = alt; > + data->port = port; > + data->ap_mode_entry = ap_mode_entry; > + data->sid = desc->svid; > + data->mode = desc->mode; > + > + typec_altmode_set_ops(alt, &cros_typec_altmode_ops); > + typec_altmode_set_drvdata(alt, data); 'data' is of type struct cros_typec_altmode_data here > + > + return alt; > +}
On Tue, Dec 10, 2024 at 4:08 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16) > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c > > index e3eabe5e42ac..0f3bc335f583 100644 > > --- a/drivers/platform/chrome/cros_ec_typec.c > > +++ b/drivers/platform/chrome/cros_ec_typec.c > > @@ -18,6 +18,7 @@ > > > > #include "cros_ec_typec.h" > > #include "cros_typec_vdm.h" > > +#include "cros_typec_altmode.h" > > > > #define DRV_NAME "cros-ec-typec" > > > > @@ -290,15 +291,15 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec, > > struct typec_altmode *amode; > > > > /* All PD capable CrOS devices are assumed to support DP altmode. */ > > + memset(&desc, 0, sizeof(desc)); > > desc.svid = USB_TYPEC_DP_SID; > > desc.mode = USB_TYPEC_DP_MODE; > > desc.vdo = DP_PORT_VDO; > > - amode = typec_port_register_altmode(port->port, &desc); > > + amode = cros_typec_register_displayport(port, &desc, > > + typec->ap_driven_altmode); > > if (IS_ERR(amode)) > > return PTR_ERR(amode); > > port->port_altmode[CROS_EC_ALTMODE_DP] = amode; > > - typec_altmode_set_drvdata(amode, port); > > - amode->ops = &port_amode_ops; > > > > /* > > * Register TBT compatibility alt mode. The EC will not enter the mode > > @@ -575,6 +576,10 @@ static int cros_typec_enable_dp(struct cros_typec_data *typec, > > if (!ret) > > ret = typec_mux_set(port->mux, &port->state); > > > > + if (!ret) > > + cros_typec_displayport_status_update(port->state.alt, > > Should we forward the return value from here? Done > > > + port->state.data); > > + > > return ret; > > } > > > > @@ -1254,6 +1259,8 @@ static int cros_typec_probe(struct platform_device *pdev) > > > > typec->typec_cmd_supported = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_CMD); > > typec->needs_mux_ack = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK); > > + typec->ap_driven_altmode = cros_ec_check_features( > > + ec_dev, EC_FEATURE_TYPEC_REQUIRE_AP_MODE_ENTRY); > > > > ret = cros_ec_cmd(typec->ec, 0, EC_CMD_USB_PD_PORTS, NULL, 0, > > &resp, sizeof(resp)); > > diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h > > index deda180a646f..9fd5342bb0ad 100644 > > --- a/drivers/platform/chrome/cros_ec_typec.h > > +++ b/drivers/platform/chrome/cros_ec_typec.h > > @@ -39,6 +39,7 @@ struct cros_typec_data { > > struct work_struct port_work; > > bool typec_cmd_supported; > > bool needs_mux_ack; > > + bool ap_driven_altmode; > > Do we need to stash this? Can we cros_ec_check_features() in > cros_typec_init_ports() and pass the bool to > cros_typec_register_port_altmodes() instead to save a struct member? We don't need to stash this but it keeps the feature checks consistently in `cros_typec_probe`. > > > }; > > > > /* Per port data. */ > > diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c > > new file mode 100644 > > index 000000000000..bb7c7ad2ff6e > > --- /dev/null > > +++ b/drivers/platform/chrome/cros_typec_altmode.c > > @@ -0,0 +1,281 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Alt-mode implementation on ChromeOS EC. > > + * > > + * Copyright 2024 Google LLC > > + * Author: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > + */ > > +#include "cros_ec_typec.h" > > + > > +#include <linux/usb/typec_dp.h> > > +#include <linux/usb/pd_vdo.h> > > Please include workqueue.h, mutex.h, etc. for things used in this file. Done. Btw, is there a script that does this for you in the kernel like include-what-you-use does for userspace? > > > + > > +#include "cros_typec_altmode.h" > > + > > +struct cros_typec_altmode_data { > > + struct work_struct work; > > + struct cros_typec_port *port; > > + struct typec_altmode *alt; > > + bool ap_mode_entry; > > The UCSI driver (drivers/usb/typec/ucsi/displayport.c) calls this > 'override', can it be named the same thing? I also see that the UCSI > driver has two bools, 'override' and 'initialized', which seems to be to > support a DP_CMD_CONFIGURE that will respond with an ACK and then the > next DP_CMD_CONFIGURE will call ucsi_altmode_update_active() to set the > altmode as active. Maybe the same method can be followed here so that on > older chromebooks where EC is in control of mode entry we can emulate > entering the mode? The reason it's called `override` in UCSI is because the feature is called "alternate mode override supported". When this optional bit is set, the UCSI method "SET_NEW_CAM" can be used to change what alternate mode is active. However, it behaves differently from cros_ec because even when override is set, the PD controller can/will still autonomously enter a mode on connection. Whereas on cros_ec_typec, if you set "ap-driven-mode", the EC will not enter any modes until the AP tells it to. Also, the reason the UCSI driver does the DP_CMD_CONFIGURE dance is because the UCSI command, SET_NEW_CAM, requires the DP configuration VDO as a parameter. Since UCSI doesn't define a VDM mechanism, the UCSI driver fakes the ".entry" call and then uses the first DP_CONFIGURE to do the actual entry. This also doesn't match the cros_ec driver (either ap-driven or not) because the interface does not allow setting the DP VDO at all. DP_CONFIGURE will be generated and consumed entirely within the EC and all we can really use is the mux update to generate a status update for the DP state machine. Right now, EC driven Chromebooks will simply report active/inactive for DP without reporting any configuration/status info. If you want to handle DP_CONFIGURE and DP_STATUS from the altmode driver, you'll need to fake the interactions in the port driver in a subsequent CL. > > > + > > + struct mutex lock; > > + u32 header; > > + u32 *vdo_data; > > + u8 vdo_size; > > + > > + u16 sid; > > + u8 mode; > > +}; > > + > > +struct cros_typec_dp_data { > > + struct cros_typec_altmode_data adata; > > + struct typec_displayport_data data; > > + bool configured; > > + bool pending_status_update; > > +}; > > + > > +static void cros_typec_altmode_work(struct work_struct *work) > > +{ > > + struct cros_typec_altmode_data *data = > > + container_of(work, struct cros_typec_altmode_data, work); > > + > > + mutex_lock(&data->lock); > > + > > + if (typec_altmode_vdm(data->alt, data->header, data->vdo_data, > > + data->vdo_size)) > > + dev_err(&data->alt->dev, "VDM 0x%x failed", data->header); > > These printks need newlines. Done. > > dev_err(&data->alt->dev, "VDM 0x%x failed\n", data->header); > > > + > > + data->header = 0; > > + data->vdo_data = NULL; > > + data->vdo_size = 0; > > + > > + mutex_unlock(&data->lock); > > +} > > + > > +static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo) > > +{ > > + struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt); > > + struct ec_params_typec_control req = { > > + .port = data->port->port_num, > > + .command = TYPEC_CONTROL_COMMAND_ENTER_MODE, > > + }; > > + int svdm_version; > > + int ret; > > + > > + if (!data->ap_mode_entry) { > > + dev_warn(&alt->dev, > > + "EC does not support AP driven mode entry\n"); > > Like this one. > > > + return -EOPNOTSUPP; > > + } > > + > > + if (data->sid == USB_TYPEC_DP_SID) > > + req.mode_to_enter = CROS_EC_ALTMODE_DP; > > + else > > + return -EOPNOTSUPP; > > + > > + ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL, > > Do we plan to delete drivers/platform/chrome/cros_typec_vdm.c file at > some point? I ask because 'port_amode_ops' becomes unused after this > series. Yes - I don't think we ever launched with the VDM request/response feature enabled. I was going to do it as a follow-up to this CL to handle attention. > > > + &req, sizeof(req), NULL, 0); > > + if (ret < 0) > > + return ret; > > + > > + svdm_version = typec_altmode_get_svdm_version(alt); > > + if (svdm_version < 0) > > + return svdm_version; > > + > > + mutex_lock(&data->lock); > > + > > + data->header = VDO(data->sid, 1, svdm_version, CMD_ENTER_MODE); > > + data->header |= VDO_OPOS(data->mode); > > + data->header |= VDO_CMDT(CMDT_RSP_ACK); > > + data->vdo_data = NULL; > > + data->vdo_size = 1; > > + schedule_work(&data->work); > > + > > + mutex_unlock(&data->lock); > > + return ret; > > +} > > + > > +static int cros_typec_altmode_exit(struct typec_altmode *alt) > > +{ > > + struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt); > > + struct ec_params_typec_control req = { > > + .port = data->port->port_num, > > + .command = TYPEC_CONTROL_COMMAND_EXIT_MODES, > > + }; > > + int svdm_version; > > + int ret; > > + > > + if (!data->ap_mode_entry) { > > + dev_warn(&alt->dev, > > + "EC does not support AP driven mode exit\n"); > > + return -EOPNOTSUPP; > > + } > > + > > + ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL, > > + &req, sizeof(req), NULL, 0); > > + > > + if (ret < 0) > > + return ret; > > + > > + svdm_version = typec_altmode_get_svdm_version(alt); > > + if (svdm_version < 0) > > + return svdm_version; > > + > > + mutex_lock(&data->lock); > > + > > + data->header = VDO(data->sid, 1, svdm_version, CMD_EXIT_MODE); > > + data->header |= VDO_OPOS(data->mode); > > + data->header |= VDO_CMDT(CMDT_RSP_ACK); > > + data->vdo_data = NULL; > > + data->vdo_size = 1; > > + schedule_work(&data->work); > > + > > + mutex_unlock(&data->lock); > > + return ret; > > +} > > + > > +static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header, > > + const u32 *data, int count) > > +{ > > + struct cros_typec_dp_data *dp_data = typec_altmode_get_drvdata(alt); > > + struct cros_typec_altmode_data *adata = &dp_data->adata; > > + > > + > > + int cmd_type = PD_VDO_CMDT(header); > > + int cmd = PD_VDO_CMD(header); > > + int svdm_version; > > + > > + svdm_version = typec_altmode_get_svdm_version(alt); > > + if (svdm_version < 0) > > + return svdm_version; > > + > > + mutex_lock(&adata->lock); > > + > > + switch (cmd_type) { > > + case CMDT_INIT: > > + if (PD_VDO_SVDM_VER(header) < svdm_version) { > > + typec_partner_set_svdm_version(adata->port->partner, > > + PD_VDO_SVDM_VER(header)); > > + svdm_version = PD_VDO_SVDM_VER(header); > > + } > > + > > + adata->header = VDO(adata->sid, 1, svdm_version, cmd); > > + adata->header |= VDO_OPOS(adata->mode); > > + > > + /* > > + * DP_CMD_CONFIGURE: We can't actually do anything with the > > + * provided VDO yet so just send back an ACK. > > + * > > + * DP_CMD_STATUS_UPDATE: We wait for Mux changes to send > > + * DPStatus Acks. > > + */ > > + switch (cmd) { > > + case DP_CMD_CONFIGURE: > > + dp_data->data.conf = *data; > > + adata->header |= VDO_CMDT(CMDT_RSP_ACK); > > + dp_data->configured = true; > > + schedule_work(&adata->work); > > + break; > > + case DP_CMD_STATUS_UPDATE: > > + dp_data->pending_status_update = true; > > + break; > > + default: > > + adata->header |= VDO_CMDT(CMDT_RSP_ACK); > > + schedule_work(&adata->work); > > + break; > > + } > > + > > + break; > > + default: > > + break; > > + } > > + > > + mutex_unlock(&adata->lock); > > + return 0; > > +} > > + > > +static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header, > > + const u32 *data, int count) > > +{ > > + struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt); > > + > > + if (!adata->ap_mode_entry) > > + return -EOPNOTSUPP; > > + > > + if (adata->sid == USB_TYPEC_DP_SID) > > + return cros_typec_displayport_vdm(alt, header, data, count); > > + > > + return -EINVAL; > > +} > > + > > +static const struct typec_altmode_ops cros_typec_altmode_ops = { > > + .enter = cros_typec_altmode_enter, > > + .exit = cros_typec_altmode_exit, > > + .vdm = cros_typec_altmode_vdm, > > +}; > > + > > +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE) > > +int cros_typec_displayport_status_update(struct typec_altmode *altmode, > > + struct typec_displayport_data *data) > > +{ > > + struct cros_typec_dp_data *dp_data = > > + typec_altmode_get_drvdata(altmode); > > + struct cros_typec_altmode_data *adata = &dp_data->adata; > > + > > + if (!dp_data->pending_status_update) { > > + dev_dbg(&altmode->dev, > > + "Got DPStatus without a pending request"); > > + return 0; > > + } > > + > > + if (dp_data->configured && dp_data->data.conf != data->conf) > > + dev_dbg(&altmode->dev, > > + "DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x", > > + dp_data->data.conf, data->conf); > > + > > + mutex_lock(&adata->lock); > > + > > + dp_data->data = *data; > > + dp_data->pending_status_update = false; > > + adata->header |= VDO_CMDT(CMDT_RSP_ACK); > > + adata->vdo_data = &dp_data->data.status; > > + adata->vdo_size = 2; > > + schedule_work(&adata->work); > > + > > + mutex_unlock(&adata->lock); > > + > > + return 0; > > +} > > + > > +struct typec_altmode * > > +cros_typec_register_displayport(struct cros_typec_port *port, > > + struct typec_altmode_desc *desc, > > + bool ap_mode_entry) > > +{ > > + struct typec_altmode *alt; > > + struct cros_typec_altmode_data *data; > > Can you name it 'adata' consistently? That makes it easy to search for > 'adata' in this file and know it's always talking about a struct > cros_typec_altmode_data type of data. Done > > > + > > + alt = typec_port_register_altmode(port->port, desc); > > + if (IS_ERR(alt)) > > + return alt; > > + > > + data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) { > > + typec_unregister_altmode(alt); > > + return ERR_PTR(-ENOMEM); > > + } > > + > > + INIT_WORK(&data->work, cros_typec_altmode_work); > > + mutex_init(&data->lock); > > + data->alt = alt; > > + data->port = port; > > + data->ap_mode_entry = ap_mode_entry; > > + data->sid = desc->svid; > > + data->mode = desc->mode; > > + > > + typec_altmode_set_ops(alt, &cros_typec_altmode_ops); > > + typec_altmode_set_drvdata(alt, data); > > + > > + return alt; > > +} > > +#endif > > diff --git a/drivers/platform/chrome/cros_typec_altmode.h b/drivers/platform/chrome/cros_typec_altmode.h > > new file mode 100644 > > index 000000000000..c6f8fb02c99c > > --- /dev/null > > +++ b/drivers/platform/chrome/cros_typec_altmode.h > > @@ -0,0 +1,34 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > + > > +#ifndef __CROS_TYPEC_ALTMODE_H__ > > +#define __CROS_TYPEC_ALTMODE_H__ > > #include <linux/kconfig.h> for IS_ENABLED() > #include <linux/usb/typec.h> for typec_port_register_altmode() Done > > > + > > +struct cros_typec_port; > > +struct typec_altmode; > > +struct typec_altmode_desc; > > +struct typec_displayport_data; > > + > > +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE) > > +struct typec_altmode * > > +cros_typec_register_displayport(struct cros_typec_port *port, > > + struct typec_altmode_desc *desc, > > + bool ap_mode_entry); > > + > > +int cros_typec_displayport_status_update(struct typec_altmode *altmode, > > + struct typec_displayport_data *data); > > +#else > > +static inline struct typec_altmode * > > +cros_typec_register_displayport(struct cros_typec_port *port, > > + struct typec_altmode_desc *desc, > > + bool ap_mode_entry) > > +{ > > + return typec_port_register_altmode(port->port, desc); > > +}
On Wed, Dec 11, 2024 at 1:58 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16) > > diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c > > new file mode 100644 > > index 000000000000..bb7c7ad2ff6e > > --- /dev/null > > +++ b/drivers/platform/chrome/cros_typec_altmode.c > > @@ -0,0 +1,281 @@ > [...] > > + > > +static const struct typec_altmode_ops cros_typec_altmode_ops = { > > + .enter = cros_typec_altmode_enter, > > + .exit = cros_typec_altmode_exit, > > + .vdm = cros_typec_altmode_vdm, > > +}; > > + > > +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE) > > +int cros_typec_displayport_status_update(struct typec_altmode *altmode, > > + struct typec_displayport_data *data) > > +{ > > + struct cros_typec_dp_data *dp_data = > > + typec_altmode_get_drvdata(altmode); > > How does this work? I see that the type of the drvdata is struct > cros_typec_altmode_data per the allocation in > cros_typec_register_displayport(), but here we're treating it as the > type struct cros_typec_dp_data, which has a struct > cros_typec_altmode_data as the first member. The allocation is too small > from what I can tell. The same problem looks to be there in > cros_typec_displayport_vdm(). > > > + struct cros_typec_altmode_data *adata = &dp_data->adata; > > + > > + if (!dp_data->pending_status_update) { > > + dev_dbg(&altmode->dev, > > + "Got DPStatus without a pending request"); > > + return 0; > > + } > > + > > + if (dp_data->configured && dp_data->data.conf != data->conf) > > + dev_dbg(&altmode->dev, > > + "DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x", > > + dp_data->data.conf, data->conf); > > + > > + mutex_lock(&adata->lock); > > + > > + dp_data->data = *data; > > + dp_data->pending_status_update = false; > > + adata->header |= VDO_CMDT(CMDT_RSP_ACK); > > + adata->vdo_data = &dp_data->data.status; > > + adata->vdo_size = 2; > > + schedule_work(&adata->work); > > + > > + mutex_unlock(&adata->lock); > > + > > + return 0; > > +} > > + > > +struct typec_altmode * > > +cros_typec_register_displayport(struct cros_typec_port *port, > > + struct typec_altmode_desc *desc, > > + bool ap_mode_entry) > > +{ > > + struct typec_altmode *alt; > > + struct cros_typec_altmode_data *data; > > + > > + alt = typec_port_register_altmode(port->port, desc); > > + if (IS_ERR(alt)) > > + return alt; > > + > > + data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) { > > + typec_unregister_altmode(alt); > > + return ERR_PTR(-ENOMEM); > > + } > > + > > + INIT_WORK(&data->work, cros_typec_altmode_work); > > + mutex_init(&data->lock); > > + data->alt = alt; > > + data->port = port; > > + data->ap_mode_entry = ap_mode_entry; > > + data->sid = desc->svid; > > + data->mode = desc->mode; > > + > > + typec_altmode_set_ops(alt, &cros_typec_altmode_ops); > > + typec_altmode_set_drvdata(alt, data); > > 'data' is of type struct cros_typec_altmode_data here This should have been allocated as cros_typec_dp_data. Missed during a previous refactor that changed the type from a union to this format. > > > + > > + return alt; > > +}
Quoting Abhishek Pandit-Subedi (2024-12-13 10:29:51) > On Tue, Dec 10, 2024 at 4:08 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16) > > > diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c > > > new file mode 100644 > > > index 000000000000..bb7c7ad2ff6e > > > --- /dev/null > > > +++ b/drivers/platform/chrome/cros_typec_altmode.c > > > @@ -0,0 +1,281 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/* > > > + * Alt-mode implementation on ChromeOS EC. > > > + * > > > + * Copyright 2024 Google LLC > > > + * Author: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > + */ > > > +#include "cros_ec_typec.h" > > > + > > > +#include <linux/usb/typec_dp.h> > > > +#include <linux/usb/pd_vdo.h> > > > > Please include workqueue.h, mutex.h, etc. for things used in this file. > Done. Btw, is there a script that does this for you in the kernel like > include-what-you-use does for userspace? I'm not aware of one. > > > > > > + > > > +#include "cros_typec_altmode.h" > > > + > > > +struct cros_typec_altmode_data { > > > + struct work_struct work; > > > + struct cros_typec_port *port; > > > + struct typec_altmode *alt; > > > + bool ap_mode_entry; > > > > The UCSI driver (drivers/usb/typec/ucsi/displayport.c) calls this > > 'override', can it be named the same thing? I also see that the UCSI > > driver has two bools, 'override' and 'initialized', which seems to be to > > support a DP_CMD_CONFIGURE that will respond with an ACK and then the > > next DP_CMD_CONFIGURE will call ucsi_altmode_update_active() to set the > > altmode as active. Maybe the same method can be followed here so that on > > older chromebooks where EC is in control of mode entry we can emulate > > entering the mode? > > The reason it's called `override` in UCSI is because the feature is > called "alternate mode override supported". When this optional bit is > set, the UCSI method "SET_NEW_CAM" can be used to change what > alternate mode is active. However, it behaves differently from cros_ec > because even when override is set, the PD controller can/will still > autonomously enter a mode on connection. Whereas on cros_ec_typec, if > you set "ap-driven-mode", the EC will not enter any modes until the AP > tells it to. Ok, got it. > > Also, the reason the UCSI driver does the DP_CMD_CONFIGURE dance is > because the UCSI command, SET_NEW_CAM, requires the DP configuration > VDO as a parameter. Since UCSI doesn't define a VDM mechanism, the > UCSI driver fakes the ".entry" call and then uses the first > DP_CONFIGURE to do the actual entry. This also doesn't match the > cros_ec driver (either ap-driven or not) because the interface does > not allow setting the DP VDO at all. DP_CONFIGURE will be generated > and consumed entirely within the EC and all we can really use is the > mux update to generate a status update for the DP state machine. Is the 'initialized' dance the fake ".entry"? I want the not ap-driven (ec-driven?) mode to work with this series, and specifically not print an error message. I'm guessing that to do that we should fake ".entry" when the EC is in control of the mode entry. Or is there some way to jump the state machine forward for the port altmode so that it is already entered? I think to get the device to show the pin configuration and mode like "source" vs. "sink" or "usb" we need to spoof multiple VDMs. > > Right now, EC driven Chromebooks will simply report active/inactive > for DP without reporting any configuration/status info. If you want to > handle DP_CONFIGURE and DP_STATUS from the altmode driver, you'll need > to fake the interactions in the port driver in a subsequent CL. > Ok. I'd very much like to do that because I need to make the displayport partner altmode driver work to the point that it calls drm_connector_oob_hotplug_event(). Do we need to spoof a DP_CMD_STATUS_UPDATE on mode entry when the EC indicates that DP mode is entered on a port?
Quoting Abhishek Pandit-Subedi (2024-12-13 10:33:19) > On Wed, Dec 11, 2024 at 1:58 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16) > > > diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c > > > new file mode 100644 > > > index 000000000000..bb7c7ad2ff6e > > > --- /dev/null > > > +++ b/drivers/platform/chrome/cros_typec_altmode.c > > > @@ -0,0 +1,281 @@ [...] > > > +struct typec_altmode * > > > +cros_typec_register_displayport(struct cros_typec_port *port, > > > + struct typec_altmode_desc *desc, > > > + bool ap_mode_entry) > > > +{ > > > + struct typec_altmode *alt; > > > + struct cros_typec_altmode_data *data; > > > + > > > + alt = typec_port_register_altmode(port->port, desc); > > > + if (IS_ERR(alt)) > > > + return alt; > > > + > > > + data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL); > > > + if (!data) { > > > + typec_unregister_altmode(alt); > > > + return ERR_PTR(-ENOMEM); > > > + } > > > + > > > + INIT_WORK(&data->work, cros_typec_altmode_work); > > > + mutex_init(&data->lock); > > > + data->alt = alt; > > > + data->port = port; > > > + data->ap_mode_entry = ap_mode_entry; > > > + data->sid = desc->svid; > > > + data->mode = desc->mode; > > > + > > > + typec_altmode_set_ops(alt, &cros_typec_altmode_ops); > > > + typec_altmode_set_drvdata(alt, data); > > > > 'data' is of type struct cros_typec_altmode_data here > This should have been allocated as cros_typec_dp_data. Missed during a > previous refactor that changed the type from a union to this format. It would be good to have the cros_typec_altmode_data member be somewhere besides the first member of cros_typec_dp_data so that this fails faster when stashing the pointer into the drvdata and treating it as the wrong type on the other side.
diff --git a/MAINTAINERS b/MAINTAINERS index cd6aa609deba..5f9d8b8f1cb3 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5369,9 +5369,12 @@ F: include/linux/platform_data/cros_usbpd_notify.h CHROMEOS EC USB TYPE-C DRIVER M: Prashant Malani <pmalani@chromium.org> +M: Benson Leung <bleung@chromium.org> +M: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> L: chrome-platform@lists.linux.dev S: Maintained F: drivers/platform/chrome/cros_ec_typec.* +F: drivers/platform/chrome/cros_typec_altmode.* F: drivers/platform/chrome/cros_typec_switch.c F: drivers/platform/chrome/cros_typec_vdm.* diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig index 7dbeb786352a..984f843ea7a2 100644 --- a/drivers/platform/chrome/Kconfig +++ b/drivers/platform/chrome/Kconfig @@ -226,12 +226,18 @@ config CROS_EC_SYSFS To compile this driver as a module, choose M here: the module will be called cros_ec_sysfs. +config CROS_EC_TYPEC_ALTMODES + bool + help + Selectable symbol to enable altmodes. + config CROS_EC_TYPEC tristate "ChromeOS EC Type-C Connector Control" depends on MFD_CROS_EC_DEV && TYPEC depends on CROS_USBPD_NOTIFY depends on USB_ROLE_SWITCH default MFD_CROS_EC_DEV + select CROS_EC_TYPEC_ALTMODES if TYPEC_DP_ALTMODE help If you say Y here, you get support for accessing Type C connector information from the Chrome OS EC. diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile index 2dcc6ccc2302..aec2d043a0fe 100644 --- a/drivers/platform/chrome/Makefile +++ b/drivers/platform/chrome/Makefile @@ -18,7 +18,11 @@ obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o 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 +ifneq ($(CONFIG_CROS_EC_TYPEC_ALTMODES),) + cros-ec-typec-objs += cros_typec_altmode.o +endif obj-$(CONFIG_CROS_EC_TYPEC) += cros-ec-typec.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_typec.c b/drivers/platform/chrome/cros_ec_typec.c index e3eabe5e42ac..0f3bc335f583 100644 --- a/drivers/platform/chrome/cros_ec_typec.c +++ b/drivers/platform/chrome/cros_ec_typec.c @@ -18,6 +18,7 @@ #include "cros_ec_typec.h" #include "cros_typec_vdm.h" +#include "cros_typec_altmode.h" #define DRV_NAME "cros-ec-typec" @@ -290,15 +291,15 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec, struct typec_altmode *amode; /* All PD capable CrOS devices are assumed to support DP altmode. */ + memset(&desc, 0, sizeof(desc)); desc.svid = USB_TYPEC_DP_SID; desc.mode = USB_TYPEC_DP_MODE; desc.vdo = DP_PORT_VDO; - amode = typec_port_register_altmode(port->port, &desc); + amode = cros_typec_register_displayport(port, &desc, + typec->ap_driven_altmode); if (IS_ERR(amode)) return PTR_ERR(amode); port->port_altmode[CROS_EC_ALTMODE_DP] = amode; - typec_altmode_set_drvdata(amode, port); - amode->ops = &port_amode_ops; /* * Register TBT compatibility alt mode. The EC will not enter the mode @@ -575,6 +576,10 @@ static int cros_typec_enable_dp(struct cros_typec_data *typec, if (!ret) ret = typec_mux_set(port->mux, &port->state); + if (!ret) + cros_typec_displayport_status_update(port->state.alt, + port->state.data); + return ret; } @@ -1254,6 +1259,8 @@ static int cros_typec_probe(struct platform_device *pdev) typec->typec_cmd_supported = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_CMD); typec->needs_mux_ack = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK); + typec->ap_driven_altmode = cros_ec_check_features( + ec_dev, EC_FEATURE_TYPEC_REQUIRE_AP_MODE_ENTRY); ret = cros_ec_cmd(typec->ec, 0, EC_CMD_USB_PD_PORTS, NULL, 0, &resp, sizeof(resp)); diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h index deda180a646f..9fd5342bb0ad 100644 --- a/drivers/platform/chrome/cros_ec_typec.h +++ b/drivers/platform/chrome/cros_ec_typec.h @@ -39,6 +39,7 @@ struct cros_typec_data { struct work_struct port_work; bool typec_cmd_supported; bool needs_mux_ack; + bool ap_driven_altmode; }; /* Per port data. */ diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c new file mode 100644 index 000000000000..bb7c7ad2ff6e --- /dev/null +++ b/drivers/platform/chrome/cros_typec_altmode.c @@ -0,0 +1,281 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Alt-mode implementation on ChromeOS EC. + * + * Copyright 2024 Google LLC + * Author: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> + */ +#include "cros_ec_typec.h" + +#include <linux/usb/typec_dp.h> +#include <linux/usb/pd_vdo.h> + +#include "cros_typec_altmode.h" + +struct cros_typec_altmode_data { + struct work_struct work; + struct cros_typec_port *port; + struct typec_altmode *alt; + bool ap_mode_entry; + + struct mutex lock; + u32 header; + u32 *vdo_data; + u8 vdo_size; + + u16 sid; + u8 mode; +}; + +struct cros_typec_dp_data { + struct cros_typec_altmode_data adata; + struct typec_displayport_data data; + bool configured; + bool pending_status_update; +}; + +static void cros_typec_altmode_work(struct work_struct *work) +{ + struct cros_typec_altmode_data *data = + container_of(work, struct cros_typec_altmode_data, work); + + mutex_lock(&data->lock); + + if (typec_altmode_vdm(data->alt, data->header, data->vdo_data, + data->vdo_size)) + dev_err(&data->alt->dev, "VDM 0x%x failed", data->header); + + data->header = 0; + data->vdo_data = NULL; + data->vdo_size = 0; + + mutex_unlock(&data->lock); +} + +static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo) +{ + struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt); + struct ec_params_typec_control req = { + .port = data->port->port_num, + .command = TYPEC_CONTROL_COMMAND_ENTER_MODE, + }; + int svdm_version; + int ret; + + if (!data->ap_mode_entry) { + dev_warn(&alt->dev, + "EC does not support AP driven mode entry\n"); + return -EOPNOTSUPP; + } + + if (data->sid == USB_TYPEC_DP_SID) + req.mode_to_enter = CROS_EC_ALTMODE_DP; + else + return -EOPNOTSUPP; + + ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL, + &req, sizeof(req), NULL, 0); + if (ret < 0) + return ret; + + svdm_version = typec_altmode_get_svdm_version(alt); + if (svdm_version < 0) + return svdm_version; + + mutex_lock(&data->lock); + + data->header = VDO(data->sid, 1, svdm_version, CMD_ENTER_MODE); + data->header |= VDO_OPOS(data->mode); + data->header |= VDO_CMDT(CMDT_RSP_ACK); + data->vdo_data = NULL; + data->vdo_size = 1; + schedule_work(&data->work); + + mutex_unlock(&data->lock); + return ret; +} + +static int cros_typec_altmode_exit(struct typec_altmode *alt) +{ + struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt); + struct ec_params_typec_control req = { + .port = data->port->port_num, + .command = TYPEC_CONTROL_COMMAND_EXIT_MODES, + }; + int svdm_version; + int ret; + + if (!data->ap_mode_entry) { + dev_warn(&alt->dev, + "EC does not support AP driven mode exit\n"); + return -EOPNOTSUPP; + } + + ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL, + &req, sizeof(req), NULL, 0); + + if (ret < 0) + return ret; + + svdm_version = typec_altmode_get_svdm_version(alt); + if (svdm_version < 0) + return svdm_version; + + mutex_lock(&data->lock); + + data->header = VDO(data->sid, 1, svdm_version, CMD_EXIT_MODE); + data->header |= VDO_OPOS(data->mode); + data->header |= VDO_CMDT(CMDT_RSP_ACK); + data->vdo_data = NULL; + data->vdo_size = 1; + schedule_work(&data->work); + + mutex_unlock(&data->lock); + return ret; +} + +static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header, + const u32 *data, int count) +{ + struct cros_typec_dp_data *dp_data = typec_altmode_get_drvdata(alt); + struct cros_typec_altmode_data *adata = &dp_data->adata; + + + int cmd_type = PD_VDO_CMDT(header); + int cmd = PD_VDO_CMD(header); + int svdm_version; + + svdm_version = typec_altmode_get_svdm_version(alt); + if (svdm_version < 0) + return svdm_version; + + mutex_lock(&adata->lock); + + switch (cmd_type) { + case CMDT_INIT: + if (PD_VDO_SVDM_VER(header) < svdm_version) { + typec_partner_set_svdm_version(adata->port->partner, + PD_VDO_SVDM_VER(header)); + svdm_version = PD_VDO_SVDM_VER(header); + } + + adata->header = VDO(adata->sid, 1, svdm_version, cmd); + adata->header |= VDO_OPOS(adata->mode); + + /* + * DP_CMD_CONFIGURE: We can't actually do anything with the + * provided VDO yet so just send back an ACK. + * + * DP_CMD_STATUS_UPDATE: We wait for Mux changes to send + * DPStatus Acks. + */ + switch (cmd) { + case DP_CMD_CONFIGURE: + dp_data->data.conf = *data; + adata->header |= VDO_CMDT(CMDT_RSP_ACK); + dp_data->configured = true; + schedule_work(&adata->work); + break; + case DP_CMD_STATUS_UPDATE: + dp_data->pending_status_update = true; + break; + default: + adata->header |= VDO_CMDT(CMDT_RSP_ACK); + schedule_work(&adata->work); + break; + } + + break; + default: + break; + } + + mutex_unlock(&adata->lock); + return 0; +} + +static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header, + const u32 *data, int count) +{ + struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt); + + if (!adata->ap_mode_entry) + return -EOPNOTSUPP; + + if (adata->sid == USB_TYPEC_DP_SID) + return cros_typec_displayport_vdm(alt, header, data, count); + + return -EINVAL; +} + +static const struct typec_altmode_ops cros_typec_altmode_ops = { + .enter = cros_typec_altmode_enter, + .exit = cros_typec_altmode_exit, + .vdm = cros_typec_altmode_vdm, +}; + +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE) +int cros_typec_displayport_status_update(struct typec_altmode *altmode, + struct typec_displayport_data *data) +{ + struct cros_typec_dp_data *dp_data = + typec_altmode_get_drvdata(altmode); + struct cros_typec_altmode_data *adata = &dp_data->adata; + + if (!dp_data->pending_status_update) { + dev_dbg(&altmode->dev, + "Got DPStatus without a pending request"); + return 0; + } + + if (dp_data->configured && dp_data->data.conf != data->conf) + dev_dbg(&altmode->dev, + "DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x", + dp_data->data.conf, data->conf); + + mutex_lock(&adata->lock); + + dp_data->data = *data; + dp_data->pending_status_update = false; + adata->header |= VDO_CMDT(CMDT_RSP_ACK); + adata->vdo_data = &dp_data->data.status; + adata->vdo_size = 2; + schedule_work(&adata->work); + + mutex_unlock(&adata->lock); + + return 0; +} + +struct typec_altmode * +cros_typec_register_displayport(struct cros_typec_port *port, + struct typec_altmode_desc *desc, + bool ap_mode_entry) +{ + struct typec_altmode *alt; + struct cros_typec_altmode_data *data; + + alt = typec_port_register_altmode(port->port, desc); + if (IS_ERR(alt)) + return alt; + + data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL); + if (!data) { + typec_unregister_altmode(alt); + return ERR_PTR(-ENOMEM); + } + + INIT_WORK(&data->work, cros_typec_altmode_work); + mutex_init(&data->lock); + data->alt = alt; + data->port = port; + data->ap_mode_entry = ap_mode_entry; + data->sid = desc->svid; + data->mode = desc->mode; + + typec_altmode_set_ops(alt, &cros_typec_altmode_ops); + typec_altmode_set_drvdata(alt, data); + + return alt; +} +#endif diff --git a/drivers/platform/chrome/cros_typec_altmode.h b/drivers/platform/chrome/cros_typec_altmode.h new file mode 100644 index 000000000000..c6f8fb02c99c --- /dev/null +++ b/drivers/platform/chrome/cros_typec_altmode.h @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __CROS_TYPEC_ALTMODE_H__ +#define __CROS_TYPEC_ALTMODE_H__ + +struct cros_typec_port; +struct typec_altmode; +struct typec_altmode_desc; +struct typec_displayport_data; + +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE) +struct typec_altmode * +cros_typec_register_displayport(struct cros_typec_port *port, + struct typec_altmode_desc *desc, + bool ap_mode_entry); + +int cros_typec_displayport_status_update(struct typec_altmode *altmode, + struct typec_displayport_data *data); +#else +static inline struct typec_altmode * +cros_typec_register_displayport(struct cros_typec_port *port, + struct typec_altmode_desc *desc, + bool ap_mode_entry) +{ + return typec_port_register_altmode(port->port, desc); +} + +static inline int cros_typec_displayport_status_update(struct typec_altmode *altmode, + struct typec_displayport_data *data) +{ + return 0; +} +#endif +#endif /* __CROS_TYPEC_ALTMODE_H__ */
Add support for entering and exiting displayport alt-mode on systems using AP driven alt-mode. Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> --- Changes in v4: - memset struct typec_altmode_desc - Add CONFIG_CROS_EC_TYPEC_ALTMODES for Makefile use - Move ap_driven_altmode check to common vdm function - Add locking to protect shared data - Update enter/exit error messages Changes in v3: - Refactored typec_altmode_dp_data per review request - Removed unused vdm operations during altmode registration Changes in v2: - Refactored displayport into cros_typec_altmode.c to extract common implementation between altmodes MAINTAINERS | 3 + drivers/platform/chrome/Kconfig | 6 + drivers/platform/chrome/Makefile | 4 + drivers/platform/chrome/cros_ec_typec.c | 13 +- drivers/platform/chrome/cros_ec_typec.h | 1 + drivers/platform/chrome/cros_typec_altmode.c | 281 +++++++++++++++++++ drivers/platform/chrome/cros_typec_altmode.h | 34 +++ 7 files changed, 339 insertions(+), 3 deletions(-) create mode 100644 drivers/platform/chrome/cros_typec_altmode.c create mode 100644 drivers/platform/chrome/cros_typec_altmode.h