| Message ID | 20230811210735.159529-4-utkarsh.h.patel@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Displayport Alternate Mode 2.1 Support | expand |
Hi Utkarsh, Thanks for the patch. Please include the chrome-platform mailing list to each patch in the series; at the very least, patches which touch drivers/platform/chrome should definitely have the mailing list (chrome-platform@lists.linux.dev). Otherwise, we don't have enough context about what changes are being made across the series. On Aug 11 14:07, Utkarsh Patel 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 data as well as from > the existing EC PD host command interface. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-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 | 30 +++++++++++++++++++++++++ > drivers/platform/chrome/cros_ec_typec.h | 1 + > 2 files changed, 31 insertions(+) > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c > index d0b4d3fc40ed..eb4a1cb584a2 100644 > --- a/drivers/platform/chrome/cros_ec_typec.c > +++ b/drivers/platform/chrome/cros_ec_typec.c > @@ -485,6 +485,32 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec, > return typec_mux_set(port->mux, &port->state); > } > > +static int cros_typec_dp21_support(struct cros_typec_port *port, > + struct typec_displayport_data dp21_data, > + struct ec_response_usb_pd_control_v2 *pd_ctrl) > +{ > + u32 cable_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_DP_SID); > + > + if (cable_vdo & DP_CAP_DPAM_VERSION) { > + dp21_data.conf |= cable_vdo; > + } else { > + /* Cable Speed */ > + dp21_data.conf |= pd_ctrl->cable_speed << DP_CONF_SIGNALLING_SHIFT; > + > + /* Cable Type */ > + if (pd_ctrl->cable_gen & USB_PD_CTRL_OPTICAL_CABLE) > + dp21_data.conf |= DP_CONF_CABLE_TYPE_OPTICAL << DP_CONF_CABLE_TYPE_SHIFT; > + else if (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID) & TBT_CABLE_RETIMER) > + dp21_data.conf |= DP_CONF_CABLE_TYPE_RE_TIMER << DP_CONF_CABLE_TYPE_SHIFT; > + else if (pd_ctrl->cable_gen & USB_PD_CTRL_ACTIVE_CABLE) > + dp21_data.conf |= DP_CONF_CABLE_TYPE_RE_DRIVER << DP_CONF_CABLE_TYPE_SHIFT; > + } I don't understand why the conf VDO is being recreated here. cable_vdo should already contain the necessary bits. Just use the cable_vdo that you get from cros_typec_get_cable_vdo(); it will have all the bits set correctly already (the EC should be doing that). The "if" condition should also be unnecessary. You are already doing something similar in the other patch for "active retimer cable support" [1] > + > + port->state.data = &dp21_data; > + > + return typec_mux_set(port->mux, &port->state); Note that now you have reversed the order in which the muxes are set (which leads to subtle timing issues with Burnside Bridge and other similar retimers). So please don't do this. > +} > + > /* Spoof the VDOs that were likely communicated by the partner. */ > static int cros_typec_enable_dp(struct cros_typec_data *typec, > int port_num, > @@ -524,6 +550,9 @@ 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)); > > + if (typec->typec_dp21_supported) > + return cros_typec_dp21_support(port, dp_data, pd_ctrl); > + > ret = cros_typec_retimer_set(port->retimer, port->state); > if (!ret) > ret = typec_mux_set(port->mux, &port->state); > @@ -1196,6 +1225,7 @@ static int cros_typec_probe(struct platform_device *pdev) > > typec->typec_cmd_supported = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_CMD); > typec->needs_mux_ack = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK); > + typec->typec_dp21_supported = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_DP2_1); This entire feature isn't necessary. Regardless of whether dp2.1 is supported or not, the port driver just needs to forward the cable_vdo it receives faithfully to the mux driver, which can deal with internal details (based on whether *it* supports DP 2.1 or not). Thanks, -Prashant [1] https://lore.kernel.org/linux-usb/20230718024703.1013367-1-utkarsh.h.patel@intel.com/T/#m950b24e7874d34f11081f252ba3ef4e752628529
Hi Prashant, Thank you for the review and feedback. > -----Original Message----- > From: Prashant Malani <pmalani@chromium.org> > Sent: Friday, August 18, 2023 10:16 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; andriy.shevchenko@linux.intel.com; > bleung@chromium.org > Subject: Re: [PATCH 3/4] platform/chrome: cros_ec_typec: Add Displayport > Alternatemode 2.1 Support > > Hi Utkarsh, > > Thanks for the patch. Please include the chrome-platform mailing list to each > patch in the series; at the very least, patches which touch > drivers/platform/chrome should definitely have the mailing list (chrome- > platform@lists.linux.dev). Otherwise, we don't have enough context about > what changes are being made across the series. Ack. I will add this mailing list. > > > > +static int cros_typec_dp21_support(struct cros_typec_port *port, > > + struct typec_displayport_data dp21_data, > > + struct ec_response_usb_pd_control_v2 > *pd_ctrl) { > > + u32 cable_vdo = cros_typec_get_cable_vdo(port, > USB_TYPEC_DP_SID); > > + > > + if (cable_vdo & DP_CAP_DPAM_VERSION) { > > + dp21_data.conf |= cable_vdo; > > + } else { > > + /* Cable Speed */ > > + dp21_data.conf |= pd_ctrl->cable_speed << > DP_CONF_SIGNALLING_SHIFT; > > + > > + /* Cable Type */ > > + if (pd_ctrl->cable_gen & USB_PD_CTRL_OPTICAL_CABLE) > > + dp21_data.conf |= DP_CONF_CABLE_TYPE_OPTICAL > << DP_CONF_CABLE_TYPE_SHIFT; > > + else if (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID) > & TBT_CABLE_RETIMER) > > + dp21_data.conf |= DP_CONF_CABLE_TYPE_RE_TIMER > << DP_CONF_CABLE_TYPE_SHIFT; > > + else if (pd_ctrl->cable_gen & USB_PD_CTRL_ACTIVE_CABLE) > > + dp21_data.conf |= > DP_CONF_CABLE_TYPE_RE_DRIVER << DP_CONF_CABLE_TYPE_SHIFT; > > + } > > I don't understand why the conf VDO is being recreated here. cable_vdo > should already contain the necessary bits. Just use the cable_vdo that you get > from cros_typec_get_cable_vdo(); it will have all the bits set correctly already > (the EC should be doing that). > > The "if" condition should also be unnecessary. > > You are already doing something similar in the other patch for "active retimer > cable support" [1] "if" condition is necessary because there are cables (LRD Gen3) with DPSID but does not support DPAM 2.1 and in that case we need to check TBTSID of the cable and use the cable properties e.g. SPEED and Type. Since that information is already available in pd_ctrl, we are leveraging it. I will remove the part where it's checking retimer cable as DP2.1 is anyway not supported. > > > + > > + port->state.data = &dp21_data; > > + > > + return typec_mux_set(port->mux, &port->state); > > Note that now you have reversed the order in which the muxes are set (which > leads to subtle timing issues with Burnside Bridge and other similar retimers). > So please don't do this. Are you suggesting to add same order here? ret = cros_typec_retimer_set(port->retimer, port->state); if (!ret) ret = typec_mux_set(port->mux, &port->state); > > > +} > > + > > /* Spoof the VDOs that were likely communicated by the partner. */ > > static int cros_typec_enable_dp(struct cros_typec_data *typec, > > int port_num, > > @@ -524,6 +550,9 @@ 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)); > > > > + if (typec->typec_dp21_supported) > > + return cros_typec_dp21_support(port, dp_data, pd_ctrl); > > + > > ret = cros_typec_retimer_set(port->retimer, port->state); > > if (!ret) > > ret = typec_mux_set(port->mux, &port->state); @@ -1196,6 > +1225,7 @@ > > static int cros_typec_probe(struct platform_device *pdev) > > > > typec->typec_cmd_supported = cros_ec_check_features(ec_dev, > EC_FEATURE_TYPEC_CMD); > > typec->needs_mux_ack = cros_ec_check_features(ec_dev, > > EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK); > > + typec->typec_dp21_supported = cros_ec_check_features(ec_dev, > > +EC_FEATURE_TYPEC_DP2_1); > > This entire feature isn't necessary. Regardless of whether dp2.1 is supported > or not, the port driver just needs to forward the cable_vdo it receives faithfully > to the mux driver, which can deal with internal details (based on whether *it* > supports DP 2.1 or not). I am Okay with that. We thought we can avoid unnecessary check for the cable_vdo for DP alternate mode on platform where DP2.1 is not supported. Sincerely, Utkarsh Patel.
Hi Utkarsh, On Mon, Aug 21, 2023 at 10:34 AM Patel, Utkarsh H <utkarsh.h.patel@intel.com> wrote: > > Hi Prashant, > > Thank you for the review and feedback. > > > -----Original Message----- > > From: Prashant Malani <pmalani@chromium.org> >> > > I don't understand why the conf VDO is being recreated here. cable_vdo > > should already contain the necessary bits. Just use the cable_vdo that you get > > from cros_typec_get_cable_vdo(); it will have all the bits set correctly already > > (the EC should be doing that). > > > > The "if" condition should also be unnecessary. > > > > You are already doing something similar in the other patch for "active retimer > > cable support" [1] > > "if" condition is necessary because there are cables (LRD Gen3) with DPSID but does not support DPAM 2.1 and in that case we need to check TBTSID of the cable and use the cable properties e.g. SPEED and Type. Ok, then grab the two VDOs distinctly (cable_dp_vdo and cable_tbt_vdo). Also, the explanation you gave above seems like a good candidate for a comment above the "if" block. > Since that information is already available in pd_ctrl, we are leveraging it. I will remove the part where it's checking retimer cable as DP2.1 is anyway not supported. As mentioned in earlier patches related to this, we want to avoid using EC-specific data structures as much as possible moving forward, especially if the information can be gleaned from the available DiscSVID VDOs. So, please use the required cable VDO (whether it is DP or TBT SID). > > > > > > + > > > + port->state.data = &dp21_data; > > > + > > > + return typec_mux_set(port->mux, &port->state); > > > > Note that now you have reversed the order in which the muxes are set (which > > leads to subtle timing issues with Burnside Bridge and other similar retimers). > > So please don't do this. > > Are you suggesting to add same order here? Specifically, breaking out a separate function for DP 2.1 and then embedding that in the overall enable_dp() function makes things unnecessarily complex. Just craft the VDOs onew time, within enable_dp(), and then use the existing locations where cros_retimer_set and typec_mux_set() are called. This will become more clear once you rebase this commit on top of the changes in [1] Effectively cros_typec_enable_dp() should handle all DP cases, without needing a special function broken out for DP 2.1 > > This entire feature isn't necessary. Regardless of whether dp2.1 is supported > > or not, the port driver just needs to forward the cable_vdo it receives faithfully > > to the mux driver, which can deal with internal details (based on whether *it* > > supports DP 2.1 or not). > > I am Okay with that. > We thought we can avoid unnecessary check for the cable_vdo for DP alternate mode on platform where DP2.1 is not supported. The optimization is miniscule enough to not be worth it the added code verbosity.
Hi Prashant, > > > I don't understand why the conf VDO is being recreated here. > > > cable_vdo should already contain the necessary bits. Just use the > > > cable_vdo that you get from cros_typec_get_cable_vdo(); it will have > > > all the bits set correctly already (the EC should be doing that). > > > > > > The "if" condition should also be unnecessary. > > > > > > You are already doing something similar in the other patch for > > > "active retimer cable support" [1] > > > > "if" condition is necessary because there are cables (LRD Gen3) with DPSID > but does not support DPAM 2.1 and in that case we need to check TBTSID of > the cable and use the cable properties e.g. SPEED and Type. > > Ok, then grab the two VDOs distinctly (cable_dp_vdo and cable_tbt_vdo). > Also, the explanation you gave above seems like a good candidate for a > comment above the "if" block. > > > Since that information is already available in pd_ctrl, we are leveraging it. I > will remove the part where it's checking retimer cable as DP2.1 is anyway not > supported. > > As mentioned in earlier patches related to this, we want to avoid using EC- > specific data structures as much as possible moving forward, especially if the > information can be gleaned from the available DiscSVID VDOs. So, please use > the required cable VDO (whether it is DP or TBT SID). Ack. > > > > > > > > > > + > > > > + port->state.data = &dp21_data; > > > > + > > > > + return typec_mux_set(port->mux, &port->state); > > > > > > Note that now you have reversed the order in which the muxes are set > > > (which leads to subtle timing issues with Burnside Bridge and other similar > retimers). > > > So please don't do this. > > > > Are you suggesting to add same order here? > > Specifically, breaking out a separate function for DP 2.1 and then embedding > that in the overall enable_dp() function makes things unnecessarily complex. > > Just craft the VDOs onew time, within enable_dp(), and then use the existing > locations where cros_retimer_set and typec_mux_set() are called. > > This will become more clear once you rebase this commit on top of the > changes in [1] > > Effectively cros_typec_enable_dp() should handle all DP cases, without > needing a special function broken out for DP 2.1 Ack. > > > > This entire feature isn't necessary. Regardless of whether dp2.1 is > > > supported or not, the port driver just needs to forward the > > > cable_vdo it receives faithfully to the mux driver, which can deal > > > with internal details (based on whether *it* supports DP 2.1 or not). > > > > I am Okay with that. > > We thought we can avoid unnecessary check for the cable_vdo for DP > alternate mode on platform where DP2.1 is not supported. > The optimization is miniscule enough to not be worth it the added code > verbosity. Ack. I will remove it. Sincerely, Utkarsh Patel.
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c index d0b4d3fc40ed..eb4a1cb584a2 100644 --- a/drivers/platform/chrome/cros_ec_typec.c +++ b/drivers/platform/chrome/cros_ec_typec.c @@ -485,6 +485,32 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec, return typec_mux_set(port->mux, &port->state); } +static int cros_typec_dp21_support(struct cros_typec_port *port, + struct typec_displayport_data dp21_data, + struct ec_response_usb_pd_control_v2 *pd_ctrl) +{ + u32 cable_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_DP_SID); + + if (cable_vdo & DP_CAP_DPAM_VERSION) { + dp21_data.conf |= cable_vdo; + } else { + /* Cable Speed */ + dp21_data.conf |= pd_ctrl->cable_speed << DP_CONF_SIGNALLING_SHIFT; + + /* Cable Type */ + if (pd_ctrl->cable_gen & USB_PD_CTRL_OPTICAL_CABLE) + dp21_data.conf |= DP_CONF_CABLE_TYPE_OPTICAL << DP_CONF_CABLE_TYPE_SHIFT; + else if (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID) & TBT_CABLE_RETIMER) + dp21_data.conf |= DP_CONF_CABLE_TYPE_RE_TIMER << DP_CONF_CABLE_TYPE_SHIFT; + else if (pd_ctrl->cable_gen & USB_PD_CTRL_ACTIVE_CABLE) + dp21_data.conf |= DP_CONF_CABLE_TYPE_RE_DRIVER << DP_CONF_CABLE_TYPE_SHIFT; + } + + port->state.data = &dp21_data; + + return typec_mux_set(port->mux, &port->state); +} + /* Spoof the VDOs that were likely communicated by the partner. */ static int cros_typec_enable_dp(struct cros_typec_data *typec, int port_num, @@ -524,6 +550,9 @@ 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)); + if (typec->typec_dp21_supported) + return cros_typec_dp21_support(port, dp_data, pd_ctrl); + ret = cros_typec_retimer_set(port->retimer, port->state); if (!ret) ret = typec_mux_set(port->mux, &port->state); @@ -1196,6 +1225,7 @@ static int cros_typec_probe(struct platform_device *pdev) typec->typec_cmd_supported = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_CMD); typec->needs_mux_ack = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK); + typec->typec_dp21_supported = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_DP2_1); ret = cros_ec_cmd(typec->ec, 0, EC_CMD_USB_PD_PORTS, NULL, 0, &resp, sizeof(resp)); diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h index deda180a646f..a588b2780823 100644 --- a/drivers/platform/chrome/cros_ec_typec.h +++ b/drivers/platform/chrome/cros_ec_typec.h @@ -37,6 +37,7 @@ struct cros_typec_data { struct cros_typec_port *ports[EC_USB_PD_MAX_PORTS]; struct notifier_block nb; struct work_struct port_work; + bool typec_dp21_supported; bool typec_cmd_supported; bool needs_mux_ack; };