Hi Utkarsh,
On Mon, Sep 11, 2023 at 5:58 PM Utkarsh Patel <[email protected]> wrote:
>
> Displayport Alternatemode 2.1 requires cable capabilities such as cable
> signalling, cable type, DPAM version which then will be used by mux
> driver for displayport configuration. These capabilities can be derived
> from the Cable VDO.
> Added a helper macro to get the Type C cable speed when provided the
> cable VDO.
>
> Signed-off-by: Utkarsh Patel <[email protected]>
Thank you for addressing the comments. This LGTM; I have one minor suggestion,
but I'll leave it to USB maintainers for the final call on that comment, so:
Acked-by: Prashant Malani <[email protected]>
> ---
> Changes in v3:
> - Removed use of variable cable_seepd.
> - Added helper macro of pd_vdo.h in this patch as cros_ec_typec is the user.
>
> Changes in v2:
> - Remvoed feature flag specifice to DP2.1.
> - Remvoed seperate function for DP2.1.
> - Removed use of EC host coammnd to get cable details.
> - TBT cable VDO is used to get cable details.
> - Using VDO_CABLE_SPEED macro to get passive cable speed.
>
> drivers/platform/chrome/cros_ec_typec.c | 28 +++++++++++++++++++++++++
> include/linux/usb/pd_vdo.h | 1 +
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index d0b4d3fc40ed..cca913400b39 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -492,6 +492,8 @@ static int cros_typec_enable_dp(struct cros_typec_data *typec,
> {
> struct cros_typec_port *port = typec->ports[port_num];
> struct typec_displayport_data dp_data;
> + u32 cable_tbt_vdo;
> + u32 cable_dp_vdo;
> int ret;
>
> if (typec->pd_ctrl_ver < 2) {
> @@ -524,6 +526,32 @@ static int cros_typec_enable_dp(struct cros_typec_data *typec,
> port->state.data = &dp_data;
> port->state.mode = TYPEC_MODAL_STATE(ffs(pd_ctrl->dp_mode));
>
> + /* Get cable VDO for cables with DPSID to check DPAM2.1 is supported */
> + cable_dp_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_DP_SID);
> +
> + /**
> + * Get cable VDO for thunderbolt cables and cables with DPSID but does not
> + * support DPAM2.1.
> + */
> + cable_tbt_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);
> +
> + if (cable_dp_vdo & DP_CAP_DPAM_VERSION) {
> + dp_data.conf |= cable_dp_vdo;
> + } else if (cable_tbt_vdo) {
> + dp_data.conf |= TBT_CABLE_SPEED(cable_tbt_vdo) << DP_CONF_SIGNALLING_SHIFT;
> +
> + /* Cable Type */
> + if (cable_tbt_vdo & TBT_CABLE_OPTICAL)
> + dp_data.conf |= DP_CONF_CABLE_TYPE_OPTICAL << DP_CONF_CABLE_TYPE_SHIFT;
> + else if (cable_tbt_vdo & TBT_CABLE_RETIMER)
> + dp_data.conf |= DP_CONF_CABLE_TYPE_RE_TIMER << DP_CONF_CABLE_TYPE_SHIFT;
> + else if (cable_tbt_vdo & TBT_CABLE_ACTIVE_PASSIVE)
> + dp_data.conf |= DP_CONF_CABLE_TYPE_RE_DRIVER << DP_CONF_CABLE_TYPE_SHIFT;
> + } else if (PD_IDH_PTYPE(port->c_identity.id_header) == IDH_PTYPE_PCABLE) {
> + dp_data.conf |= VDO_TYPEC_CABLE_SPEED(port->c_identity.vdo[0]) <<
> + DP_CONF_SIGNALLING_SHIFT;
> + }
> +
> ret = cros_typec_retimer_set(port->retimer, port->state);
> if (!ret)
> ret = typec_mux_set(port->mux, &port->state);
> diff --git a/include/linux/usb/pd_vdo.h b/include/linux/usb/pd_vdo.h
> index b057250704e8..3a747938cdab 100644
> --- a/include/linux/usb/pd_vdo.h
> +++ b/include/linux/usb/pd_vdo.h
> @@ -376,6 +376,7 @@
> | ((vbm) & 0x3) << 9 | (sbu) << 8 | (sbut) << 7 | ((cur) & 0x3) << 5 \
> | (vbt) << 4 | (sopp) << 3 | ((spd) & 0x7))
>
> +#define VDO_TYPEC_CABLE_SPEED(vdo) ((vdo) & 0x7)
I would suggest putting this header modification in a separate patch;
if for some reason we have to revert
the Chrome part of the change, then we won't rip this part out too
(some other driver down the road may use
the macro and would break if it were to be removed). But I'll leave it
to Heikki to determine whether that is preferred.
Thanks,
-Prashant
Hi Prashant,
Thank you for the review.
> -----Original Message-----
> From: Prashant Malani <[email protected]>
> Sent: Monday, September 11, 2023 6:18 PM
> To: Patel, Utkarsh H <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v3 3/4] platform/chrome: cros_ec_typec: Add Displayport
> Alternatemode 2.1 Support
>
> Hi Utkarsh,
>
> On Mon, Sep 11, 2023 at 5:58 PM Utkarsh Patel <[email protected]>
> wrote:
> >
> > Displayport Alternatemode 2.1 requires cable capabilities such as
> > cable signalling, cable type, DPAM version which then will be used by
> > mux driver for displayport configuration. These capabilities can be
> > derived from the Cable VDO.
> > Added a helper macro to get the Type C cable speed when provided the
> > cable VDO.
> >
> > Signed-off-by: Utkarsh Patel <[email protected]>
>
> Thank you for addressing the comments. This LGTM; I have one minor
> suggestion, but I'll leave it to USB maintainers for the final call on that
> comment, so:
>
> Acked-by: Prashant Malani <[email protected]>
>
> > ---
> > Changes in v3:
> > - Removed use of variable cable_seepd.
> > - Added helper macro of pd_vdo.h in this patch as cros_ec_typec is the user.
> >
> > Changes in v2:
> > - Remvoed feature flag specifice to DP2.1.
> > - Remvoed seperate function for DP2.1.
> > - Removed use of EC host coammnd to get cable details.
> > - TBT cable VDO is used to get cable details.
> > - Using VDO_CABLE_SPEED macro to get passive cable speed.
> >
> > drivers/platform/chrome/cros_ec_typec.c | 28
> +++++++++++++++++++++++++
> > include/linux/usb/pd_vdo.h | 1 +
> > 2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c
> > b/drivers/platform/chrome/cros_ec_typec.c
> > index d0b4d3fc40ed..cca913400b39 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -492,6 +492,8 @@ static int cros_typec_enable_dp(struct
> > cros_typec_data *typec, {
> > struct cros_typec_port *port = typec->ports[port_num];
> > struct typec_displayport_data dp_data;
> > + u32 cable_tbt_vdo;
> > + u32 cable_dp_vdo;
> > int ret;
> >
> > if (typec->pd_ctrl_ver < 2) {
> > @@ -524,6 +526,32 @@ static int cros_typec_enable_dp(struct
> cros_typec_data *typec,
> > port->state.data = &dp_data;
> > port->state.mode = TYPEC_MODAL_STATE(ffs(pd_ctrl->dp_mode));
> >
> > + /* Get cable VDO for cables with DPSID to check DPAM2.1 is supported
> */
> > + cable_dp_vdo = cros_typec_get_cable_vdo(port,
> > + USB_TYPEC_DP_SID);
> > +
> > + /**
> > + * Get cable VDO for thunderbolt cables and cables with DPSID but does
> not
> > + * support DPAM2.1.
> > + */
> > + cable_tbt_vdo = cros_typec_get_cable_vdo(port,
> > + USB_TYPEC_TBT_SID);
> > +
> > + if (cable_dp_vdo & DP_CAP_DPAM_VERSION) {
> > + dp_data.conf |= cable_dp_vdo;
> > + } else if (cable_tbt_vdo) {
> > + dp_data.conf |= TBT_CABLE_SPEED(cable_tbt_vdo) <<
> > + DP_CONF_SIGNALLING_SHIFT;
> > +
> > + /* Cable Type */
> > + if (cable_tbt_vdo & TBT_CABLE_OPTICAL)
> > + dp_data.conf |= DP_CONF_CABLE_TYPE_OPTICAL <<
> DP_CONF_CABLE_TYPE_SHIFT;
> > + else if (cable_tbt_vdo & TBT_CABLE_RETIMER)
> > + dp_data.conf |= DP_CONF_CABLE_TYPE_RE_TIMER <<
> DP_CONF_CABLE_TYPE_SHIFT;
> > + else if (cable_tbt_vdo & TBT_CABLE_ACTIVE_PASSIVE)
> > + dp_data.conf |= DP_CONF_CABLE_TYPE_RE_DRIVER <<
> DP_CONF_CABLE_TYPE_SHIFT;
> > + } else if (PD_IDH_PTYPE(port->c_identity.id_header) ==
> IDH_PTYPE_PCABLE) {
> > + dp_data.conf |= VDO_TYPEC_CABLE_SPEED(port-
> >c_identity.vdo[0]) <<
> > + DP_CONF_SIGNALLING_SHIFT;
> > + }
> > +
> > ret = cros_typec_retimer_set(port->retimer, port->state);
> > if (!ret)
> > ret = typec_mux_set(port->mux, &port->state); diff
> > --git a/include/linux/usb/pd_vdo.h b/include/linux/usb/pd_vdo.h index
> > b057250704e8..3a747938cdab 100644
> > --- a/include/linux/usb/pd_vdo.h
> > +++ b/include/linux/usb/pd_vdo.h
> > @@ -376,6 +376,7 @@
> > | ((vbm) & 0x3) << 9 | (sbu) << 8 | (sbut) << 7 | ((cur) & 0x3) << 5 \
> > | (vbt) << 4 | (sopp) << 3 | ((spd) & 0x7))
> >
> > +#define VDO_TYPEC_CABLE_SPEED(vdo) ((vdo) & 0x7)
>
> I would suggest putting this header modification in a separate patch; if for
> some reason we have to revert the Chrome part of the change, then we won't
> rip this part out too (some other driver down the road may use the macro and
> would break if it were to be removed). But I'll leave it to Heikki to determine
> whether that is preferred.
>
Heikki, What's your preference here?
Sincerely,
Utkarsh Patel.
On Fri, Sep 15, 2023 at 04:01:44PM +0000, Patel, Utkarsh H wrote:
> > > b057250704e8..3a747938cdab 100644
> > > --- a/include/linux/usb/pd_vdo.h
> > > +++ b/include/linux/usb/pd_vdo.h
> > > @@ -376,6 +376,7 @@
> > > | ((vbm) & 0x3) << 9 | (sbu) << 8 | (sbut) << 7 | ((cur) & 0x3) << 5 \
> > > | (vbt) << 4 | (sopp) << 3 | ((spd) & 0x7))
> > >
> > > +#define VDO_TYPEC_CABLE_SPEED(vdo) ((vdo) & 0x7)
> >
> > I would suggest putting this header modification in a separate patch; if for
> > some reason we have to revert the Chrome part of the change, then we won't
> > rip this part out too (some other driver down the road may use the macro and
> > would break if it were to be removed). But I'll leave it to Heikki to determine
> > whether that is preferred.
> >
> Heikki, What's your preference here?
I think separate patch for this like Prashant said.
thanks,
--
heikki
Hi Heikki,
Thank you for the feedback.
> -----Original Message-----
> From: Heikki Krogerus <[email protected]>
> Sent: Monday, September 18, 2023 1:04 AM
> To: Patel, Utkarsh H <[email protected]>
> Cc: Prashant Malani <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v3 3/4] platform/chrome: cros_ec_typec: Add Displayport
> Alternatemode 2.1 Support
>
> On Fri, Sep 15, 2023 at 04:01:44PM +0000, Patel, Utkarsh H wrote:
>
> > > > b057250704e8..3a747938cdab 100644
> > > > --- a/include/linux/usb/pd_vdo.h
> > > > +++ b/include/linux/usb/pd_vdo.h
> > > > @@ -376,6 +376,7 @@
> > > > | ((vbm) & 0x3) << 9 | (sbu) << 8 | (sbut) << 7 | ((cur) & 0x3) << 5 \
> > > > | (vbt) << 4 | (sopp) << 3 | ((spd) & 0x7))
> > > >
> > > > +#define VDO_TYPEC_CABLE_SPEED(vdo) ((vdo) & 0x7)
> > >
> > > I would suggest putting this header modification in a separate
> > > patch; if for some reason we have to revert the Chrome part of the
> > > change, then we won't rip this part out too (some other driver down
> > > the road may use the macro and would break if it were to be
> > > removed). But I'll leave it to Heikki to determine whether that is preferred.
> > >
> > Heikki, What's your preference here?
>
> I think separate patch for this like Prashant said.
I will add and re-send the removed [PATCH v2 3/5] in the next version.
Sincerely,
Utkarsh Patel.