mbox series

[0/2] Add support to configure active retimer cable

Message ID 20230605010117.794691-1-utkarsh.h.patel@intel.com
Headers show
Series Add support to configure active retimer cable | expand

Message

Patel, Utkarsh H June 5, 2023, 1:01 a.m. UTC 
This change adds support to configure retimer cable type

Utkarsh Patel (2):
  platform/chrome: cros_ec_typec: Configure Retimer cable type
  usb: typec: intel_pmc_mux: Configure Active and Retimer Cable type

 drivers/platform/chrome/cros_ec_typec.c       |  8 +++++-
 drivers/usb/typec/mux/intel_pmc_mux.c         | 28 ++++++++++++++++---
 .../linux/platform_data/cros_ec_commands.h    |  2 ++
 3 files changed, 33 insertions(+), 5 deletions(-)

Comments

Prashant Malani June 15, 2023, 5:28 p.m. UTC | #1 
(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
Patel, Utkarsh H June 16, 2023, 9:57 p.m. UTC | #2 
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.
Prashant Malani June 16, 2023, 10:05 p.m. UTC | #3 
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,
Patel, Utkarsh H June 22, 2023, 5:40 p.m. UTC | #4 
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.
Prashant Malani June 22, 2023, 5:48 p.m. UTC | #5 
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