| Message ID | 20241030142833.v2.6.Ic61ced3cdfb5d6776435356061f12307da719829@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode | expand |
Hi Abhishek, kernel test robot noticed the following build errors: [auto build test ERROR on chrome-platform/for-next] [also build test ERROR on chrome-platform/for-firmware-next usb/usb-testing usb/usb-next usb/usb-linus westeri-thunderbolt/next linus/master v6.12-rc5 next-20241031] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Abhishek-Pandit-Subedi/usb-typec-Add-driver-for-Thunderbolt-3-Alternate-Mode/20241031-053304 base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next patch link: https://lore.kernel.org/r/20241030142833.v2.6.Ic61ced3cdfb5d6776435356061f12307da719829%40changeid patch subject: [PATCH v2 6/7] platform/chrome: cros_ec_typec: Thunderbolt support config: arm64-defconfig (https://download.01.org/0day-ci/archive/20241101/202411010039.QHl0lhBw-lkp@intel.com/config) compiler: aarch64-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241101/202411010039.QHl0lhBw-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202411010039.QHl0lhBw-lkp@intel.com/ All error/warnings (new ones prefixed by >>): In file included from drivers/platform/chrome/cros_ec_typec.c:21: >> drivers/platform/chrome/cros_typec_altmode.h:41:1: warning: no previous prototype for 'cros_typec_register_thunderbolt' [-Wmissing-prototypes] 41 | cros_typec_register_thunderbolt(struct cros_typec_port *port, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -- aarch64-linux-ld: drivers/platform/chrome/cros_typec_altmode.o: in function `cros_typec_register_thunderbolt': >> drivers/platform/chrome/cros_typec_altmode.h:43: multiple definition of `cros_typec_register_thunderbolt'; drivers/platform/chrome/cros_ec_typec.o:drivers/platform/chrome/cros_typec_altmode.h:43: first defined here vim +43 drivers/platform/chrome/cros_typec_altmode.h 34 35 #if IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE) 36 struct typec_altmode * 37 cros_typec_register_thunderbolt(struct cros_typec_port *port, 38 struct typec_altmode_desc *desc); 39 #else 40 struct typec_altmode * > 41 cros_typec_register_thunderbolt(struct cros_typec_port *port, 42 struct typec_altmode_desc *desc) > 43 { 44 return typec_port_register_altmode(port->port, desc); 45 } 46 #endif 47
On Wed, Oct 30, 2024 at 02:28:37PM -0700, Abhishek Pandit-Subedi wrote: > Add support for entering and exiting Thunderbolt alt-mode using AP > driven alt-mode. > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > --- > > Changes in v2: > - Refactored thunderbolt support into cros_typec_altmode.c > > drivers/platform/chrome/cros_ec_typec.c | 29 ++++--- > drivers/platform/chrome/cros_typec_altmode.c | 85 ++++++++++++++++++++ > drivers/platform/chrome/cros_typec_altmode.h | 14 ++++ > 3 files changed, 116 insertions(+), 12 deletions(-) > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c > index 7997e7136c4c..3e043b1c1cc8 100644 > --- a/drivers/platform/chrome/cros_ec_typec.c > +++ b/drivers/platform/chrome/cros_ec_typec.c > @@ -304,21 +304,26 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec, > typec_altmode_set_drvdata(amode, port); > amode->ops = &port_amode_ops; > #endif > - > /* > * Register TBT compatibility alt mode. The EC will not enter the mode > - * if it doesn't support it, so it's safe to register it unconditionally > - * here for now. > + * if it doesn't support it and it will not enter automatically by > + * design so we can use the |ap_driven_altmode| feature to check if we > + * should register it. > */ > - memset(&desc, 0, sizeof(desc)); > - desc.svid = USB_TYPEC_TBT_SID; > - desc.mode = TYPEC_ANY_MODE; > - amode = typec_port_register_altmode(port->port, &desc); > - if (IS_ERR(amode)) > - return PTR_ERR(amode); > - port->port_altmode[CROS_EC_ALTMODE_TBT] = amode; > - typec_altmode_set_drvdata(amode, port); > - amode->ops = &port_amode_ops; > + if (typec->ap_driven_altmode) { > + memset(&desc, 0, sizeof(desc)); > + desc.svid = USB_TYPEC_TBT_SID; > + desc.mode = TYPEC_ANY_MODE; > + amode = cros_typec_register_thunderbolt(port, &desc); > + if (IS_ERR(amode)) > + return PTR_ERR(amode); > + port->port_altmode[CROS_EC_ALTMODE_TBT] = amode; > + > +#if !IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE) > + typec_altmode_set_drvdata(amode, port); > + amode->ops = &port_amode_ops; > +#endif Why? Usually having the code block under an #if is a frowned upon practice. > + } > > port->state.alt = NULL; > port->state.mode = TYPEC_STATE_USB; [...] > diff --git a/drivers/platform/chrome/cros_typec_altmode.h b/drivers/platform/chrome/cros_typec_altmode.h > index c6f8fb02c99c..c71568314e3f 100644 > --- a/drivers/platform/chrome/cros_typec_altmode.h > +++ b/drivers/platform/chrome/cros_typec_altmode.h > @@ -31,4 +31,18 @@ static inline int cros_typec_displayport_status_update(struct typec_altmode *alt > return 0; > } > #endif > + > +#if IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE) > +struct typec_altmode * > +cros_typec_register_thunderbolt(struct cros_typec_port *port, > + struct typec_altmode_desc *desc); > +#else > +struct typec_altmode * static inline struct ... LGTM otherwise > +cros_typec_register_thunderbolt(struct cros_typec_port *port, > + struct typec_altmode_desc *desc) > +{ > + return typec_port_register_altmode(port->port, desc); > +} > +#endif > + > #endif /* __CROS_TYPEC_ALTMODE_H__ */ > -- > 2.47.0.163.g1226f6d8fa-goog >
On Thu, Oct 31, 2024 at 11:51 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Wed, Oct 30, 2024 at 02:28:37PM -0700, Abhishek Pandit-Subedi wrote: > > Add support for entering and exiting Thunderbolt alt-mode using AP > > driven alt-mode. > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > --- > > > > Changes in v2: > > - Refactored thunderbolt support into cros_typec_altmode.c > > > > drivers/platform/chrome/cros_ec_typec.c | 29 ++++--- > > drivers/platform/chrome/cros_typec_altmode.c | 85 ++++++++++++++++++++ > > drivers/platform/chrome/cros_typec_altmode.h | 14 ++++ > > 3 files changed, 116 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c > > index 7997e7136c4c..3e043b1c1cc8 100644 > > --- a/drivers/platform/chrome/cros_ec_typec.c > > +++ b/drivers/platform/chrome/cros_ec_typec.c > > @@ -304,21 +304,26 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec, > > typec_altmode_set_drvdata(amode, port); > > amode->ops = &port_amode_ops; > > #endif > > - > > /* > > * Register TBT compatibility alt mode. The EC will not enter the mode > > - * if it doesn't support it, so it's safe to register it unconditionally > > - * here for now. > > + * if it doesn't support it and it will not enter automatically by > > + * design so we can use the |ap_driven_altmode| feature to check if we > > + * should register it. > > */ > > - memset(&desc, 0, sizeof(desc)); > > - desc.svid = USB_TYPEC_TBT_SID; > > - desc.mode = TYPEC_ANY_MODE; > > - amode = typec_port_register_altmode(port->port, &desc); > > - if (IS_ERR(amode)) > > - return PTR_ERR(amode); > > - port->port_altmode[CROS_EC_ALTMODE_TBT] = amode; > > - typec_altmode_set_drvdata(amode, port); > > - amode->ops = &port_amode_ops; > > + if (typec->ap_driven_altmode) { > > + memset(&desc, 0, sizeof(desc)); > > + desc.svid = USB_TYPEC_TBT_SID; > > + desc.mode = TYPEC_ANY_MODE; > > + amode = cros_typec_register_thunderbolt(port, &desc); > > + if (IS_ERR(amode)) > > + return PTR_ERR(amode); > > + port->port_altmode[CROS_EC_ALTMODE_TBT] = amode; > > + > > +#if !IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE) > > + typec_altmode_set_drvdata(amode, port); > > + amode->ops = &port_amode_ops; > > +#endif > > Why? Usually having the code block under an #if is a frowned upon > practice. There are some CrosEC implementations that provide full VDM access for mode entry and this code was previously added to support that. I'm looking into whether this was fully deployed -- if not, I will remove this #if block entirely in my next patch series. Will do the same for displayport above. > > > + } > > > > port->state.alt = NULL; > > port->state.mode = TYPEC_STATE_USB; > > [...] > > > diff --git a/drivers/platform/chrome/cros_typec_altmode.h b/drivers/platform/chrome/cros_typec_altmode.h > > index c6f8fb02c99c..c71568314e3f 100644 > > --- a/drivers/platform/chrome/cros_typec_altmode.h > > +++ b/drivers/platform/chrome/cros_typec_altmode.h > > @@ -31,4 +31,18 @@ static inline int cros_typec_displayport_status_update(struct typec_altmode *alt > > return 0; > > } > > #endif > > + > > +#if IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE) > > +struct typec_altmode * > > +cros_typec_register_thunderbolt(struct cros_typec_port *port, > > + struct typec_altmode_desc *desc); > > +#else > > +struct typec_altmode * > > static inline struct ... > LGTM otherwise I ran allmodconfig and x86_64_defconfig but forgot to run allmodconfig - these features :( -- argh... I'll add this to my testing steps. > > > +cros_typec_register_thunderbolt(struct cros_typec_port *port, > > + struct typec_altmode_desc *desc) > > +{ > > + return typec_port_register_altmode(port->port, desc); > > +} > > +#endif > > + > > #endif /* __CROS_TYPEC_ALTMODE_H__ */ > > -- > > 2.47.0.163.g1226f6d8fa-goog > > > > -- > With best wishes > Dmitry
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c index 7997e7136c4c..3e043b1c1cc8 100644 --- a/drivers/platform/chrome/cros_ec_typec.c +++ b/drivers/platform/chrome/cros_ec_typec.c @@ -304,21 +304,26 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec, typec_altmode_set_drvdata(amode, port); amode->ops = &port_amode_ops; #endif - /* * Register TBT compatibility alt mode. The EC will not enter the mode - * if it doesn't support it, so it's safe to register it unconditionally - * here for now. + * if it doesn't support it and it will not enter automatically by + * design so we can use the |ap_driven_altmode| feature to check if we + * should register it. */ - memset(&desc, 0, sizeof(desc)); - desc.svid = USB_TYPEC_TBT_SID; - desc.mode = TYPEC_ANY_MODE; - amode = typec_port_register_altmode(port->port, &desc); - if (IS_ERR(amode)) - return PTR_ERR(amode); - port->port_altmode[CROS_EC_ALTMODE_TBT] = amode; - typec_altmode_set_drvdata(amode, port); - amode->ops = &port_amode_ops; + if (typec->ap_driven_altmode) { + memset(&desc, 0, sizeof(desc)); + desc.svid = USB_TYPEC_TBT_SID; + desc.mode = TYPEC_ANY_MODE; + amode = cros_typec_register_thunderbolt(port, &desc); + if (IS_ERR(amode)) + return PTR_ERR(amode); + port->port_altmode[CROS_EC_ALTMODE_TBT] = amode; + +#if !IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE) + typec_altmode_set_drvdata(amode, port); + amode->ops = &port_amode_ops; +#endif + } port->state.alt = NULL; port->state.mode = TYPEC_STATE_USB; diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c index af2f077674f1..6cb1e1320d6c 100644 --- a/drivers/platform/chrome/cros_typec_altmode.c +++ b/drivers/platform/chrome/cros_typec_altmode.c @@ -8,6 +8,7 @@ #include "cros_ec_typec.h" #include <linux/usb/typec_dp.h> +#include <linux/usb/typec_tbt.h> #include <linux/usb/pd_vdo.h> #include "cros_typec_altmode.h" @@ -71,6 +72,8 @@ static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo) if (data->sid == USB_TYPEC_DP_SID) req.mode_to_enter = CROS_EC_ALTMODE_DP; + else if (data->sid == USB_TYPEC_TBT_SID) + req.mode_to_enter = CROS_EC_ALTMODE_TBT; else return -EOPNOTSUPP; @@ -198,6 +201,53 @@ static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header, return 0; } +static int cros_typec_thunderbolt_vdm(struct typec_altmode *alt, u32 header, + const u32 *data, int count) +{ + struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt); + + 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; + + 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(USB_TYPEC_TBT_SID, 1, svdm_version, cmd); + adata->header |= VDO_OPOS(USB_TYPEC_TBT_MODE); + + switch (cmd) { + case CMD_ENTER_MODE: + /* Don't respond to the enter mode vdm because it + * triggers mux configuration. This is handled directly + * by the cros_ec_typec driver so the Thunderbolt driver + * doesn't need to be involved. + */ + break; + default: + adata->header |= VDO_CMDT(CMDT_RSP_ACK); + schedule_work(&adata->work); + break; + } + + break; + default: + break; + } + + return 0; +} + + static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header, const u32 *data, int count) { @@ -206,6 +256,9 @@ static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header, if (adata->sid == USB_TYPEC_DP_SID) return cros_typec_displayport_vdm(alt, header, data, count); + if (adata->sid == USB_TYPEC_TBT_SID) + return cros_typec_thunderbolt_vdm(alt, header, data, count); + return -EINVAL; } @@ -275,3 +328,35 @@ cros_typec_register_displayport(struct cros_typec_port *port, return alt; } #endif + +#if IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE) +struct typec_altmode * +cros_typec_register_thunderbolt(struct cros_typec_port *port, + struct typec_altmode_desc *desc) +{ + 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); + data->alt = alt; + data->port = port; + data->ap_mode_entry = true; + data->sid = USB_TYPEC_TBT_SID; + data->mode = USB_TYPEC_TBT_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 index c6f8fb02c99c..c71568314e3f 100644 --- a/drivers/platform/chrome/cros_typec_altmode.h +++ b/drivers/platform/chrome/cros_typec_altmode.h @@ -31,4 +31,18 @@ static inline int cros_typec_displayport_status_update(struct typec_altmode *alt return 0; } #endif + +#if IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE) +struct typec_altmode * +cros_typec_register_thunderbolt(struct cros_typec_port *port, + struct typec_altmode_desc *desc); +#else +struct typec_altmode * +cros_typec_register_thunderbolt(struct cros_typec_port *port, + struct typec_altmode_desc *desc) +{ + return typec_port_register_altmode(port->port, desc); +} +#endif + #endif /* __CROS_TYPEC_ALTMODE_H__ */
Add support for entering and exiting Thunderbolt alt-mode using AP driven alt-mode. Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> --- Changes in v2: - Refactored thunderbolt support into cros_typec_altmode.c drivers/platform/chrome/cros_ec_typec.c | 29 ++++--- drivers/platform/chrome/cros_typec_altmode.c | 85 ++++++++++++++++++++ drivers/platform/chrome/cros_typec_altmode.h | 14 ++++ 3 files changed, 116 insertions(+), 12 deletions(-)