| Message ID | 20230718024703.1013367-2-utkarsh.h.patel@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support to configure active retimer cable | expand |
Hi Prashant and Benson, Could you please help review? > > -----Original Message----- > > From: Patel, Utkarsh H <utkarsh.h.patel@intel.com> > > Sent: Monday, July 17, 2023 7:47 PM > > To: linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org; > > heikki.krogerus@linux.intel.com; pmalani@chromium.org; > > bleung@chromium.org > > Cc: Patel, Utkarsh H <utkarsh.h.patel@intel.com> > > Subject: [PATCH v4 1/2] platform/chrome: cros_ec_typec: Configure > > Retimer cable type > > > > Connector class driver only configure cable type active or passive. > > Configure if the cable type is retimer or redriver with this change. > > This detail will be provided as a part of cable discover mode VDO. > > > > Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com> > > --- > > Changes in v4: > > - Removed local variables and used inline assignemtns. > > - Added details about return values in cros_typec_get_cable_vdo. Sincerely, Utkarsh Patel.
Hi Utkarsh, Sorry I was out on leave, so didnt get to this earlier. On Jul 17 19:47, Utkarsh Patel wrote: > > +/** > + * cros_typec_get_cable_vdo() - Get Cable VDO of the connected cable > + * @port: Type-C port data > + * @svid: Standard or Vendor ID to match > + * > + * Returns the Cable VDO if match is found and returns 0 if match is not found. > + */ > +static int cros_typec_get_cable_vdo(struct cros_typec_port *port, u16 svid) return type should be u32. > +{ > + struct list_head *head = &port->plug_mode_list; > + struct cros_typec_altmode_node *node; > + u32 ret = 0; > + > + list_for_each_entry(node, head, list) { > + if (node->amode->svid == svid) > + return node->amode->vdo; > + } > + > + return ret; > +} > + > /* > * Spoof the VDOs that were likely communicated by the partner for TBT alt > * mode. > @@ -432,6 +453,9 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec, > > /* Cable Discover Mode VDO */ > data.cable_mode = TBT_MODE; > + > + data.cable_mode |= cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID); > + > data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed); > > if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) > @@ -522,8 +546,10 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec, > /* Cable Type */ > if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) > data.eudo |= EUDO_CABLE_TYPE_OPTICAL << EUDO_CABLE_TYPE_SHIFT; > - else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE) > + else if (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID) & TBT_CABLE_RETIMER) > data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT; We shouldn't need to call cros_typec_get_cable_vdo more than once. Either call it once earlier when you are crafting the data.cable_mode member and then re-use that variable here. Or don't call it there and just call it here. > + else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE) > + data.eudo |= EUDO_CABLE_TYPE_RE_DRIVER << EUDO_CABLE_TYPE_SHIFT; > > data.active_link_training = !!(pd_ctrl->control_flags & > USB_PD_CTRL_ACTIVE_LINK_UNIDIR); > -- > 2.25.1 >
Hi Prashant, Thank you for the review. > -----Original Message----- > From: Prashant Malani <pmalani@chromium.org> > Sent: Friday, August 18, 2023 10:02 AM > To: Patel, Utkarsh H <utkarsh.h.patel@intel.com> > Cc: linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org; > heikki.krogerus@linux.intel.com; bleung@chromium.org > Subject: Re: [PATCH v4 1/2] platform/chrome: cros_ec_typec: Configure > Retimer cable type > > > /* > > * Spoof the VDOs that were likely communicated by the partner for TBT alt > > * mode. > > @@ -432,6 +453,9 @@ static int cros_typec_enable_tbt(struct > > cros_typec_data *typec, > > > > /* Cable Discover Mode VDO */ > > data.cable_mode = TBT_MODE; > > + > > + data.cable_mode |= cros_typec_get_cable_vdo(port, > > +USB_TYPEC_TBT_SID); > > + > > data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed); > > > > if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) @@ - > 522,8 > > +546,10 @@ static int cros_typec_enable_usb4(struct cros_typec_data > *typec, > > /* Cable Type */ > > if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) > > data.eudo |= EUDO_CABLE_TYPE_OPTICAL << > EUDO_CABLE_TYPE_SHIFT; > > - else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE) > > + else if (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID) & > > +TBT_CABLE_RETIMER) > > data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << > EUDO_CABLE_TYPE_SHIFT; > We shouldn't need to call cros_typec_get_cable_vdo more than once. Either > call it once earlier when you are crafting the data.cable_mode member and > then re-use that variable here. Or don't call it there and just call it here. We are only calling it once depending upon which mode we enter TBT Alt or USB4. Sincerely, Utkarsh Patel.
On Mon, Aug 21, 2023 at 10:18 AM Patel, Utkarsh H <utkarsh.h.patel@intel.com> wrote: > > Hi Prashant, > > Thank you for the review. > > > -----Original Message----- > > From: Prashant Malani <pmalani@chromium.org> > > Sent: Friday, August 18, 2023 10:02 AM > > To: Patel, Utkarsh H <utkarsh.h.patel@intel.com> > > Cc: linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org; > > heikki.krogerus@linux.intel.com; bleung@chromium.org > > Subject: Re: [PATCH v4 1/2] platform/chrome: cros_ec_typec: Configure > > Retimer cable type > > > > > /* > > > * Spoof the VDOs that were likely communicated by the partner for TBT alt > > > * mode. > > > @@ -432,6 +453,9 @@ static int cros_typec_enable_tbt(struct > > > cros_typec_data *typec, > > > > > > /* Cable Discover Mode VDO */ > > > data.cable_mode = TBT_MODE; > > > + > > > + data.cable_mode |= cros_typec_get_cable_vdo(port, > > > +USB_TYPEC_TBT_SID); > > > + Here is the first call to cros_typec_get_cable_vdo(port, TBT).... > > > data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed); > > > > > > if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) @@ - > > 522,8 > > > +546,10 @@ static int cros_typec_enable_usb4(struct cros_typec_data > > *typec, > > > /* Cable Type */ > > > if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) > > > data.eudo |= EUDO_CABLE_TYPE_OPTICAL << > > EUDO_CABLE_TYPE_SHIFT; > > > - else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE) > > > + else if (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID) & > > > +TBT_CABLE_RETIMER) And here is the 2nd. > > > data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << > > EUDO_CABLE_TYPE_SHIFT; > > We shouldn't need to call cros_typec_get_cable_vdo more than once. Either > > call it once earlier when you are crafting the data.cable_mode member and > > then re-use that variable here. Or don't call it there and just call it here. > > We are only calling it once depending upon which mode we enter TBT Alt or USB4. There should only be 1 "call site" and that should be sufficient to grab the VDO from the framework for all circumstances. Whether the other invocation doesn't get called under certain circumstances isn't as relevant.
Hi Prashant, > -----Original Message----- > From: Prashant Malani <pmalani@chromium.org> > Sent: Monday, August 21, 2023 4:31 PM > To: Patel, Utkarsh H <utkarsh.h.patel@intel.com> > Cc: linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org; > heikki.krogerus@linux.intel.com; bleung@chromium.org > Subject: Re: [PATCH v4 1/2] platform/chrome: cros_ec_typec: Configure > Retimer cable type > > On Mon, Aug 21, 2023 at 10:18 AM Patel, Utkarsh H > <utkarsh.h.patel@intel.com> wrote: > > > > /* > > > > * Spoof the VDOs that were likely communicated by the partner for TBT > alt > > > > * mode. > > > > @@ -432,6 +453,9 @@ static int cros_typec_enable_tbt(struct > > > > cros_typec_data *typec, > > > > > > > > /* Cable Discover Mode VDO */ > > > > data.cable_mode = TBT_MODE; > > > > + > > > > + data.cable_mode |= cros_typec_get_cable_vdo(port, > > > > +USB_TYPEC_TBT_SID); > > > > + > > Here is the first call to cros_typec_get_cable_vdo(port, TBT).... > > > > > data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed); > > > > > > > > if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) @@ - > > > 522,8 > > > > +546,10 @@ static int cros_typec_enable_usb4(struct > > > > +cros_typec_data > > > *typec, > > > > /* Cable Type */ > > > > if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) > > > > data.eudo |= EUDO_CABLE_TYPE_OPTICAL << > > > EUDO_CABLE_TYPE_SHIFT; > > > > - else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE) > > > > + else if (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID) & > > > > +TBT_CABLE_RETIMER) > > And here is the 2nd. > > > > > data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << > > > EUDO_CABLE_TYPE_SHIFT; > > > We shouldn't need to call cros_typec_get_cable_vdo more than once. > > > Either call it once earlier when you are crafting the > > > data.cable_mode member and then re-use that variable here. Or don't call > it there and just call it here. > > > > We are only calling it once depending upon which mode we enter TBT Alt or > USB4. > > There should only be 1 "call site" and that should be sufficient to grab the VDO > from the framework for all circumstances. Whether the other invocation > doesn't get called under certain circumstances isn't as relevant. Are you suggesting something like this? static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,)... if (port->mux_flags & USB_PD_MUX_USB4_ENABLED || port->mux_flags & USB_PD_MUX_TBT_COMPAT_ENABLED) cable_tbt_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID); Sincerely, Utkarsh Patel.
Hi Utkarsh, On Aug 22 21:21, Patel, Utkarsh H wrote: > Hi Prashant, > > > -----Original Message----- > > From: Prashant Malani <pmalani@chromium.org> > > Sent: Monday, August 21, 2023 4:31 PM > > > > > > We are only calling it once depending upon which mode we enter TBT Alt or > > USB4. > > > > There should only be 1 "call site" and that should be sufficient to grab the VDO > > from the framework for all circumstances. Whether the other invocation > > doesn't get called under certain circumstances isn't as relevant. > > Are you suggesting something like this? > > static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,)... > > if (port->mux_flags & USB_PD_MUX_USB4_ENABLED || > port->mux_flags & USB_PD_MUX_TBT_COMPAT_ENABLED) > cable_tbt_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID); My apologies, I misread the patch. I think this looks good. Acked-by: Prashant Malani <pmalani@chromium.org>
Hi Greg, > > > > > > > > We are only calling it once depending upon which mode we enter TBT > > > > Alt or > > > USB4. > > > > > > There should only be 1 "call site" and that should be sufficient to > > > grab the VDO from the framework for all circumstances. Whether the > > > other invocation doesn't get called under certain circumstances isn't as > relevant. > > > > Are you suggesting something like this? > > > > static int cros_typec_configure_mux(struct cros_typec_data *typec, int > port_num,)... > > > > if (port->mux_flags & USB_PD_MUX_USB4_ENABLED || > > port->mux_flags & USB_PD_MUX_TBT_COMPAT_ENABLED) > > cable_tbt_vdo = cros_typec_get_cable_vdo(port, > USB_TYPEC_TBT_SID); > > My apologies, I misread the patch. I think this looks good. > > Acked-by: Prashant Malani <pmalani@chromium.org> Could you please add this patch to usb-next. Sincerely, Utkarsh Patel.
On Fri, Aug 25, 2023 at 09:04:29PM +0000, Patel, Utkarsh H wrote: > Hi Greg, > > > > > > > > > > > We are only calling it once depending upon which mode we enter TBT > > > > > Alt or > > > > USB4. > > > > > > > > There should only be 1 "call site" and that should be sufficient to > > > > grab the VDO from the framework for all circumstances. Whether the > > > > other invocation doesn't get called under certain circumstances isn't as > > relevant. > > > > > > Are you suggesting something like this? > > > > > > static int cros_typec_configure_mux(struct cros_typec_data *typec, int > > port_num,)... > > > > > > if (port->mux_flags & USB_PD_MUX_USB4_ENABLED || > > > port->mux_flags & USB_PD_MUX_TBT_COMPAT_ENABLED) > > > cable_tbt_vdo = cros_typec_get_cable_vdo(port, > > USB_TYPEC_TBT_SID); > > > > My apologies, I misread the patch. I think this looks good. > > > > Acked-by: Prashant Malani <pmalani@chromium.org> > > Could you please add this patch to usb-next. Ugh, it's late in the cycle, but this has been around for a while, let me go see...
On Sat, Aug 26, 2023 at 09:40:20AM +0200, Greg KH wrote: > On Fri, Aug 25, 2023 at 09:04:29PM +0000, Patel, Utkarsh H wrote: > > Hi Greg, > > > > > > > > > > > > > > We are only calling it once depending upon which mode we enter TBT > > > > > > Alt or > > > > > USB4. > > > > > > > > > > There should only be 1 "call site" and that should be sufficient to > > > > > grab the VDO from the framework for all circumstances. Whether the > > > > > other invocation doesn't get called under certain circumstances isn't as > > > relevant. > > > > > > > > Are you suggesting something like this? > > > > > > > > static int cros_typec_configure_mux(struct cros_typec_data *typec, int > > > port_num,)... > > > > > > > > if (port->mux_flags & USB_PD_MUX_USB4_ENABLED || > > > > port->mux_flags & USB_PD_MUX_TBT_COMPAT_ENABLED) > > > > cable_tbt_vdo = cros_typec_get_cable_vdo(port, > > > USB_TYPEC_TBT_SID); > > > > > > My apologies, I misread the patch. I think this looks good. > > > > > > Acked-by: Prashant Malani <pmalani@chromium.org> > > > > Could you please add this patch to usb-next. > > Ugh, it's late in the cycle, but this has been around for a while, let > me go see... Looks like patch 2/2 was already in my tree, so I've queued up this one now as well. thanks, greg k-h
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c index 25f9767c28e8..d0b4d3fc40ed 100644 --- a/drivers/platform/chrome/cros_ec_typec.c +++ b/drivers/platform/chrome/cros_ec_typec.c @@ -406,6 +406,27 @@ static int cros_typec_usb_safe_state(struct cros_typec_port *port) return ret; } +/** + * cros_typec_get_cable_vdo() - Get Cable VDO of the connected cable + * @port: Type-C port data + * @svid: Standard or Vendor ID to match + * + * Returns the Cable VDO if match is found and returns 0 if match is not found. + */ +static int cros_typec_get_cable_vdo(struct cros_typec_port *port, u16 svid) +{ + struct list_head *head = &port->plug_mode_list; + struct cros_typec_altmode_node *node; + u32 ret = 0; + + list_for_each_entry(node, head, list) { + if (node->amode->svid == svid) + return node->amode->vdo; + } + + return ret; +} + /* * Spoof the VDOs that were likely communicated by the partner for TBT alt * mode. @@ -432,6 +453,9 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec, /* Cable Discover Mode VDO */ data.cable_mode = TBT_MODE; + + data.cable_mode |= cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID); + data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed); if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) @@ -522,8 +546,10 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec, /* Cable Type */ if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) data.eudo |= EUDO_CABLE_TYPE_OPTICAL << EUDO_CABLE_TYPE_SHIFT; - else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE) + else if (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID) & TBT_CABLE_RETIMER) data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT; + else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE) + data.eudo |= EUDO_CABLE_TYPE_RE_DRIVER << EUDO_CABLE_TYPE_SHIFT; data.active_link_training = !!(pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_LINK_UNIDIR);
Connector class driver only configure cable type active or passive. Configure if the cable type is retimer or redriver with this change. This detail will be provided as a part of cable discover mode VDO. Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com> --- Changes in v4: - Removed local variables and used inline assignemtns. - Added details about return values in cros_typec_get_cable_vdo. Changes in v3: - Changed the return method in cros_typec_get_cable_vdo. - Changed passed parameters in cros_typec_get_cable_vdo. - Corrected definition for unsigned integers as kerenl standard. - Assigning cable_vdo values directly in to cable_mode. - Removed unncessary checks for Retimer cable type. Changes in v2: - Implemented use of cable discover mode vdo. - Removed adittional changes to host command. --- --- drivers/platform/chrome/cros_ec_typec.c | 28 ++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)