| Message ID | 20230605010117.794691-2-utkarsh.h.patel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support to configure active retimer cable | expand |
(Sorry, I've been out on leave so haven't had a chance to look at this earlier) On Sun, Jun 4, 2023 at 6:00 PM Utkarsh Patel <utkarsh.h.patel@intel.com> wrote: > > Connector class driver only configure cable type active or passive. > With this change it will also configure if the cable type is retimer or > redriver if required by AP. This details will be provided by Chrome EC > as a part of cable discover mode VDO. > > This change also brings in corresponding EC header updates from the EC > code base [1]. Please separate this into another patch. > > [1]: > https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/master/include/ec_commands.h > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com> > --- > drivers/platform/chrome/cros_ec_typec.c | 8 +++++++- > include/linux/platform_data/cros_ec_commands.h | 2 ++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c > index a673c3342470..9c18b1df64a7 100644 > --- a/drivers/platform/chrome/cros_ec_typec.c > +++ b/drivers/platform/chrome/cros_ec_typec.c > @@ -448,6 +448,9 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec, > if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE) > data.enter_vdo |= TBT_ENTER_MODE_ACTIVE_CABLE; > > + if (pd_ctrl->control_flags & USB_PD_CTRL_RETIMER_CABLE) > + data.enter_vdo |= TBT_CABLE_RETIMER; > + > if (!port->state.alt) { > port->state.alt = port->port_altmode[CROS_EC_ALTMODE_TBT]; > ret = cros_typec_usb_safe_state(port); > @@ -522,8 +525,11 @@ 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 (pd_ctrl->control_flags & USB_PD_CTRL_RETIMER_CABLE) > data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT; > + else if (!(pd_ctrl->control_flags & USB_PD_CTRL_RETIMER_CABLE) && > + (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); > diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h > index ab721cf13a98..c9aa5495c666 100644 > --- a/include/linux/platform_data/cros_ec_commands.h > +++ b/include/linux/platform_data/cros_ec_commands.h > @@ -4963,6 +4963,8 @@ struct ec_response_usb_pd_control_v1 { > #define USB_PD_CTRL_TBT_LEGACY_ADAPTER BIT(2) > /* Active Link Uni-Direction */ > #define USB_PD_CTRL_ACTIVE_LINK_UNIDIR BIT(3) > +/* Retimer/Redriver cable */ > +#define USB_PD_CTRL_RETIMER_CABLE BIT(4) Why are we adding this to this host commands interface? Is this information not available from the Cable (plug)'s Identity information? We register all of that in the port driver already [1], so we should just use that, instead of changing the host command interface. Thanks, [1] https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/platform/chrome/cros_ec_typec.c#L768
Hi Prashant, Thank you for the review and feedback. > > Connector class driver only configure cable type active or passive. > > With this change it will also configure if the cable type is retimer > > or redriver if required by AP. This details will be provided by Chrome > > EC as a part of cable discover mode VDO. > > > > This change also brings in corresponding EC header updates from the EC > > code base [1]. > > Please separate this into another patch. I can do that but since it's just one line change and related, kept it together. > > a/include/linux/platform_data/cros_ec_commands.h > > b/include/linux/platform_data/cros_ec_commands.h > > index ab721cf13a98..c9aa5495c666 100644 > > --- a/include/linux/platform_data/cros_ec_commands.h > > +++ b/include/linux/platform_data/cros_ec_commands.h > > @@ -4963,6 +4963,8 @@ struct ec_response_usb_pd_control_v1 { #define > > USB_PD_CTRL_TBT_LEGACY_ADAPTER BIT(2) > > /* Active Link Uni-Direction */ > > #define USB_PD_CTRL_ACTIVE_LINK_UNIDIR BIT(3) > > +/* Retimer/Redriver cable */ > > +#define USB_PD_CTRL_RETIMER_CABLE BIT(4) > > Why are we adding this to this host commands interface? Is this information > not available from the Cable (plug)'s Identity information? We register all of > that in the port driver already [1], so we should just use that, instead of > changing the host command interface. All the cable details used to configure Alternate mode and USB4 mode in this driver are provided from EC host command. To stay consistent with the existing implementation, it was added to the existing host command. Sincerely, Utkarsh Patel.
Hi Utkarsh, On Fri, Jun 16, 2023 at 2:57 PM Patel, Utkarsh H <utkarsh.h.patel@intel.com> wrote: > > Hi Prashant, > > Thank you for the review and feedback. > > > > Connector class driver only configure cable type active or passive. > > > With this change it will also configure if the cable type is retimer > > > or redriver if required by AP. This details will be provided by Chrome > > > EC as a part of cable discover mode VDO. > > > > > > This change also brings in corresponding EC header updates from the EC > > > code base [1]. > > > > Please separate this into another patch. > > I can do that but since it's just one line change and related, kept it together. It's fine to have a 1 line patch. That said (see below)... > > > > a/include/linux/platform_data/cros_ec_commands.h > > > b/include/linux/platform_data/cros_ec_commands.h > > > index ab721cf13a98..c9aa5495c666 100644 > > > --- a/include/linux/platform_data/cros_ec_commands.h > > > +++ b/include/linux/platform_data/cros_ec_commands.h > > > @@ -4963,6 +4963,8 @@ struct ec_response_usb_pd_control_v1 { #define > > > USB_PD_CTRL_TBT_LEGACY_ADAPTER BIT(2) > > > /* Active Link Uni-Direction */ > > > #define USB_PD_CTRL_ACTIVE_LINK_UNIDIR BIT(3) > > > +/* Retimer/Redriver cable */ > > > +#define USB_PD_CTRL_RETIMER_CABLE BIT(4) > > > > Why are we adding this to this host commands interface? Is this information > > not available from the Cable (plug)'s Identity information? We register all of > > that in the port driver already [1], so we should just use that, instead of > > changing the host command interface. > > All the cable details used to configure Alternate mode and USB4 mode in this driver are provided from EC host command. > To stay consistent with the existing implementation, it was added to the existing host command. I think it's fine to use the cable VDO from the registered port plug for this. It's less disruptive than introducing another (superfluous) host command modification. It is also more clear; right now we don't know what information populates the USB_PD_CTRL_RETIMER_CABLE. Please use the existing cable VDO from the cable struct for this purpose. Thanks,
Hi Prashant, > > Hi Utkarsh, > > On Fri, Jun 16, 2023 at 2:57 PM Patel, Utkarsh H <utkarsh.h.patel@intel.com> > wrote: > > > > Hi Prashant, > > > > Thank you for the review and feedback. > > > > > > Connector class driver only configure cable type active or passive. > > > > With this change it will also configure if the cable type is > > > > retimer or redriver if required by AP. This details will be > > > > provided by Chrome EC as a part of cable discover mode VDO. > > > > > > > > This change also brings in corresponding EC header updates from > > > > the EC code base [1]. > > > > > > Please separate this into another patch. > > > > I can do that but since it's just one line change and related, kept it together. > > It's fine to have a 1 line patch. That said (see below)... Ack. > > > > > > > a/include/linux/platform_data/cros_ec_commands.h > > > > b/include/linux/platform_data/cros_ec_commands.h > > > > index ab721cf13a98..c9aa5495c666 100644 > > > > --- a/include/linux/platform_data/cros_ec_commands.h > > > > +++ b/include/linux/platform_data/cros_ec_commands.h > > > > @@ -4963,6 +4963,8 @@ struct ec_response_usb_pd_control_v1 { > > > > #define USB_PD_CTRL_TBT_LEGACY_ADAPTER BIT(2) > > > > /* Active Link Uni-Direction */ > > > > #define USB_PD_CTRL_ACTIVE_LINK_UNIDIR BIT(3) > > > > +/* Retimer/Redriver cable */ > > > > +#define USB_PD_CTRL_RETIMER_CABLE BIT(4) > > > > > > Why are we adding this to this host commands interface? Is this > > > information not available from the Cable (plug)'s Identity > > > information? We register all of that in the port driver already [1], > > > so we should just use that, instead of changing the host command interface. > > > > All the cable details used to configure Alternate mode and USB4 mode in this > driver are provided from EC host command. > > To stay consistent with the existing implementation, it was added to the > existing host command. > > I think it's fine to use the cable VDO from the registered port plug for this. It's > less disruptive than introducing another (superfluous) host command > modification. > It is also more clear; right now we don't know what information populates the > USB_PD_CTRL_RETIMER_CABLE. Please use the existing cable VDO from the > cable struct for this purpose. This information is provided from EC where it gets TBT3 Cable Discover Mode VDO Responses. Like any other cable capabilities e.g. active/passive, rounded support, cable speed, cable's retimer/redvier capabilities is also populated from this response in bit 22. (https://chromium.googlesource.com/chromiumos/platform/ec/+/HEAD/include/usb_pd_tbt.h#251) * <22> : Re-timer * 0 = Not re-timer * 1 = Re-timer This is already being used by EC to configure retimer with the cable capabilities and the goal of this patch here is to provide the same cable retimer/redriver capabilities to SOC. When all other cable details are populated from EC in alternate mode and usb4 mode and send it to SOC, it's better to stick to the same framework to use cable capabilities which is already available instead of new implementation just to get retimer/redriver cable capabilities. Sincerely, Utkarsh Patel.
Hi Utkarsh, On Thu, Jun 22, 2023 at 10:40 AM Patel, Utkarsh H <utkarsh.h.patel@intel.com> wrote: > > Hi Prashant, > > > > Hi Utkarsh, > > > > On Fri, Jun 16, 2023 at 2:57 PM Patel, Utkarsh H <utkarsh.h.patel@intel.com> > > wrote: > > > > > > Hi Prashant, > > > > > > Thank you for the review and feedback. > > > > > > > > Connector class driver only configure cable type active or passive. > > > > > With this change it will also configure if the cable type is > > > > > retimer or redriver if required by AP. This details will be > > > > > provided by Chrome EC as a part of cable discover mode VDO. > > > > > > > > > > This change also brings in corresponding EC header updates from > > > > > the EC code base [1]. > > > > > > > > Please separate this into another patch. > > > > > > I can do that but since it's just one line change and related, kept it together. > > > > It's fine to have a 1 line patch. That said (see below)... > > Ack. > > > > > > > > > > > a/include/linux/platform_data/cros_ec_commands.h > > > > > b/include/linux/platform_data/cros_ec_commands.h > > > > > index ab721cf13a98..c9aa5495c666 100644 > > > > > --- a/include/linux/platform_data/cros_ec_commands.h > > > > > +++ b/include/linux/platform_data/cros_ec_commands.h > > > > > @@ -4963,6 +4963,8 @@ struct ec_response_usb_pd_control_v1 { > > > > > #define USB_PD_CTRL_TBT_LEGACY_ADAPTER BIT(2) > > > > > /* Active Link Uni-Direction */ > > > > > #define USB_PD_CTRL_ACTIVE_LINK_UNIDIR BIT(3) > > > > > +/* Retimer/Redriver cable */ > > > > > +#define USB_PD_CTRL_RETIMER_CABLE BIT(4) > > > > > > > > Why are we adding this to this host commands interface? Is this > > > > information not available from the Cable (plug)'s Identity > > > > information? We register all of that in the port driver already [1], > > > > so we should just use that, instead of changing the host command interface. > > > > > > All the cable details used to configure Alternate mode and USB4 mode in this > > driver are provided from EC host command. > > > To stay consistent with the existing implementation, it was added to the > > existing host command. > > > > I think it's fine to use the cable VDO from the registered port plug for this. It's > > less disruptive than introducing another (superfluous) host command > > modification. > > It is also more clear; right now we don't know what information populates the > > USB_PD_CTRL_RETIMER_CABLE. Please use the existing cable VDO from the > > cable struct for this purpose. > > This information is provided from EC where it gets TBT3 Cable Discover Mode VDO Responses. > Like any other cable capabilities e.g. active/passive, rounded support, cable speed, cable's retimer/redvier capabilities is also > populated from this response in bit 22. (https://chromium.googlesource.com/chromiumos/platform/ec/+/HEAD/include/usb_pd_tbt.h#251) > * <22> : Re-timer > * 0 = Not re-timer > * 1 = Re-timer > > This is already being used by EC to configure retimer with the cable capabilities and the goal of this patch here is to provide the same cable retimer/redriver capabilities to SOC. > When all other cable details are populated from EC in alternate mode and usb4 mode and send it to SOC, it's better to stick to the same framework to use cable capabilities which is already available instead of > new implementation just to get retimer/redriver cable capabilities. Sorry, but that is not a sufficient justification. We used the old interface because when this code was first implemented, we didn't have the cable discovery information being sent to the AP/kernel. Now we do. Please just use the Cable Discover Mode VDO response (which is **already there in this driver**) to extract this information. What the EC is doing to fashion this information is not really relevant here, and if it is also pulling this information from the Cable Discover Mode VDO response as you say, then there is no difference. Please don't modify legacy EC host interfaces that we are trying hard to not modify any further. Regards, -Prashant
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c index a673c3342470..9c18b1df64a7 100644 --- a/drivers/platform/chrome/cros_ec_typec.c +++ b/drivers/platform/chrome/cros_ec_typec.c @@ -448,6 +448,9 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec, if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE) data.enter_vdo |= TBT_ENTER_MODE_ACTIVE_CABLE; + if (pd_ctrl->control_flags & USB_PD_CTRL_RETIMER_CABLE) + data.enter_vdo |= TBT_CABLE_RETIMER; + if (!port->state.alt) { port->state.alt = port->port_altmode[CROS_EC_ALTMODE_TBT]; ret = cros_typec_usb_safe_state(port); @@ -522,8 +525,11 @@ 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 (pd_ctrl->control_flags & USB_PD_CTRL_RETIMER_CABLE) data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT; + else if (!(pd_ctrl->control_flags & USB_PD_CTRL_RETIMER_CABLE) && + (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); diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h index ab721cf13a98..c9aa5495c666 100644 --- a/include/linux/platform_data/cros_ec_commands.h +++ b/include/linux/platform_data/cros_ec_commands.h @@ -4963,6 +4963,8 @@ struct ec_response_usb_pd_control_v1 { #define USB_PD_CTRL_TBT_LEGACY_ADAPTER BIT(2) /* Active Link Uni-Direction */ #define USB_PD_CTRL_ACTIVE_LINK_UNIDIR BIT(3) +/* Retimer/Redriver cable */ +#define USB_PD_CTRL_RETIMER_CABLE BIT(4) struct ec_response_usb_pd_control_v2 { uint8_t enabled;