mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH 0/2] Add support to configure active retimer cable
@ 2023-06-05  1:01 Utkarsh Patel
  2023-06-05  1:01 ` [PATCH 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type Utkarsh Patel
  2023-06-05  1:01 ` [PATCH 2/2] usb: typec: intel_pmc_mux: Configure Active and Retimer Cable type Utkarsh Patel
  0 siblings, 2 replies; 8+ messages in thread
From: Utkarsh Patel @ 2023-06-05  1:01 UTC (permalink / raw)
  To: linux-kernel, linux-usb, heikki.krogerus, pmalani, bleung; +Cc: Utkarsh Patel

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(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type
  2023-06-05  1:01 [PATCH 0/2] Add support to configure active retimer cable Utkarsh Patel
@ 2023-06-05  1:01 ` Utkarsh Patel
  2023-06-15 17:28   ` Prashant Malani
  2023-06-05  1:01 ` [PATCH 2/2] usb: typec: intel_pmc_mux: Configure Active and Retimer Cable type Utkarsh Patel
  1 sibling, 1 reply; 8+ messages in thread
From: Utkarsh Patel @ 2023-06-05  1:01 UTC (permalink / raw)
  To: linux-kernel, linux-usb, heikki.krogerus, pmalani, bleung; +Cc: Utkarsh Patel

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].

[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)
 
 struct ec_response_usb_pd_control_v2 {
 	uint8_t enabled;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] usb: typec: intel_pmc_mux: Configure Active and Retimer Cable type
  2023-06-05  1:01 [PATCH 0/2] Add support to configure active retimer cable Utkarsh Patel
  2023-06-05  1:01 ` [PATCH 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type Utkarsh Patel
@ 2023-06-05  1:01 ` Utkarsh Patel
  1 sibling, 0 replies; 8+ messages in thread
From: Utkarsh Patel @ 2023-06-05  1:01 UTC (permalink / raw)
  To: linux-kernel, linux-usb, heikki.krogerus, pmalani, bleung; +Cc: Utkarsh Patel

Cable type such as active and retimer received as a part of Thunderbolt3
or Thunderbolt4 cable discover mode VDO needs to be configured in the
thunderbolt alternate mode.

Configuring the register bits for this cable type is changed with Intel
Meteor Lake platform. BIT2 for Retimer/Redriver cable and BIT22 for
Active/Passive cable.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
---
 drivers/usb/typec/mux/intel_pmc_mux.c | 28 +++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
index 34e4188a40ff..352988132075 100644
--- a/drivers/usb/typec/mux/intel_pmc_mux.c
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -57,7 +57,7 @@ enum {
 };
 
 /* Common Mode Data bits */
-#define PMC_USB_ALTMODE_ACTIVE_CABLE	BIT(2)
+#define PMC_USB_ALTMODE_RETIMER_CABLE	BIT(2)
 
 #define PMC_USB_ALTMODE_ORI_SHIFT	1
 #define PMC_USB_ALTMODE_UFP_SHIFT	3
@@ -69,6 +69,7 @@ enum {
 #define PMC_USB_ALTMODE_TBT_TYPE	BIT(17)
 #define PMC_USB_ALTMODE_CABLE_TYPE	BIT(18)
 #define PMC_USB_ALTMODE_ACTIVE_LINK	BIT(20)
+#define PMC_USB_ALTMODE_ACTIVE_CABLE	BIT(22)
 #define PMC_USB_ALTMODE_FORCE_LSR	BIT(23)
 #define PMC_USB_ALTMODE_CABLE_SPD(_s_)	(((_s_) & GENMASK(2, 0)) << 25)
 #define   PMC_USB_ALTMODE_CABLE_USB31	1
@@ -313,8 +314,18 @@ pmc_usb_mux_tbt(struct pmc_usb_port *port, struct typec_mux_state *state)
 	if (data->cable_mode & TBT_CABLE_LINK_TRAINING)
 		req.mode_data |= PMC_USB_ALTMODE_ACTIVE_LINK;
 
-	if (data->enter_vdo & TBT_ENTER_MODE_ACTIVE_CABLE)
-		req.mode_data |= PMC_USB_ALTMODE_ACTIVE_CABLE;
+	if (acpi_dev_hid_uid_match(port->pmc->iom_adev, "INTC1072", NULL) ||
+	    acpi_dev_hid_uid_match(port->pmc->iom_adev, "INTC1079", NULL)) {
+		if ((data->enter_vdo & TBT_ENTER_MODE_ACTIVE_CABLE) ||
+		    (data->cable_mode & TBT_CABLE_RETIMER))
+			req.mode_data |= PMC_USB_ALTMODE_RETIMER_CABLE;
+	} else {
+		if (data->enter_vdo & TBT_ENTER_MODE_ACTIVE_CABLE)
+			req.mode_data |= PMC_USB_ALTMODE_ACTIVE_CABLE;
+
+		if (data->cable_mode & TBT_CABLE_RETIMER)
+			req.mode_data |= PMC_USB_ALTMODE_RETIMER_CABLE;
+	}
 
 	req.mode_data |= PMC_USB_ALTMODE_CABLE_SPD(cable_speed);
 
@@ -353,8 +364,17 @@ pmc_usb_mux_usb4(struct pmc_usb_port *port, struct typec_mux_state *state)
 	case EUDO_CABLE_TYPE_OPTICAL:
 		req.mode_data |= PMC_USB_ALTMODE_CABLE_TYPE;
 		fallthrough;
+	case EUDO_CABLE_TYPE_RE_TIMER:
+		if (!acpi_dev_hid_uid_match(port->pmc->iom_adev, "INTC1072", NULL) ||
+		    !acpi_dev_hid_uid_match(port->pmc->iom_adev, "INTC1079", NULL))
+			req.mode_data |= PMC_USB_ALTMODE_RETIMER_CABLE;
+		fallthrough;
 	default:
-		req.mode_data |= PMC_USB_ALTMODE_ACTIVE_CABLE;
+		if (acpi_dev_hid_uid_match(port->pmc->iom_adev, "INTC1072", NULL) ||
+		    acpi_dev_hid_uid_match(port->pmc->iom_adev, "INTC1079", NULL))
+			req.mode_data |= PMC_USB_ALTMODE_RETIMER_CABLE;
+		else
+			req.mode_data |= PMC_USB_ALTMODE_ACTIVE_CABLE;
 
 		/* Configure data rate to rounded in the case of Active TBT3
 		 * and USB4 cables.
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type
  2023-06-05  1:01 ` [PATCH 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type Utkarsh Patel
@ 2023-06-15 17:28   ` Prashant Malani
  2023-06-16 21:57     ` Patel, Utkarsh H
  0 siblings, 1 reply; 8+ messages in thread
From: Prashant Malani @ 2023-06-15 17:28 UTC (permalink / raw)
  To: Utkarsh Patel; +Cc: linux-kernel, linux-usb, heikki.krogerus, bleung

(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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type
  2023-06-15 17:28   ` Prashant Malani
@ 2023-06-16 21:57     ` Patel, Utkarsh H
  2023-06-16 22:05       ` Prashant Malani
  0 siblings, 1 reply; 8+ messages in thread
From: Patel, Utkarsh H @ 2023-06-16 21:57 UTC (permalink / raw)
  To: Prashant Malani; +Cc: linux-kernel, linux-usb, heikki.krogerus, bleung

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type
  2023-06-16 21:57     ` Patel, Utkarsh H
@ 2023-06-16 22:05       ` Prashant Malani
  2023-06-22 17:40         ` Patel, Utkarsh H
  0 siblings, 1 reply; 8+ messages in thread
From: Prashant Malani @ 2023-06-16 22:05 UTC (permalink / raw)
  To: Patel, Utkarsh H; +Cc: linux-kernel, linux-usb, heikki.krogerus, bleung

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,

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type
  2023-06-16 22:05       ` Prashant Malani
@ 2023-06-22 17:40         ` Patel, Utkarsh H
  2023-06-22 17:48           ` Prashant Malani
  0 siblings, 1 reply; 8+ messages in thread
From: Patel, Utkarsh H @ 2023-06-22 17:40 UTC (permalink / raw)
  To: Prashant Malani; +Cc: linux-kernel, linux-usb, heikki.krogerus, bleung

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type
  2023-06-22 17:40         ` Patel, Utkarsh H
@ 2023-06-22 17:48           ` Prashant Malani
  0 siblings, 0 replies; 8+ messages in thread
From: Prashant Malani @ 2023-06-22 17:48 UTC (permalink / raw)
  To: Patel, Utkarsh H; +Cc: linux-kernel, linux-usb, heikki.krogerus, bleung

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-06-22 17:49 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-05  1:01 [PATCH 0/2] Add support to configure active retimer cable Utkarsh Patel
2023-06-05  1:01 ` [PATCH 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type Utkarsh Patel
2023-06-15 17:28   ` Prashant Malani
2023-06-16 21:57     ` Patel, Utkarsh H
2023-06-16 22:05       ` Prashant Malani
2023-06-22 17:40         ` Patel, Utkarsh H
2023-06-22 17:48           ` Prashant Malani
2023-06-05  1:01 ` [PATCH 2/2] usb: typec: intel_pmc_mux: Configure Active and Retimer Cable type Utkarsh Patel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®