Message-ID: <mu6f7ru7wrxtbjra4hu4btlztg5zfrug2wxrbylfguvrv25sjl@ywxtxioeydqh>
Date: Thu, 24 Apr 2025 14:03:39 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Stephen Boyd <swboyd@...omium.org>
Cc: Tzung-Bi Shih <tzungbi@...nel.org>, linux-kernel@...r.kernel.org,
        patches@...ts.linux.dev, Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>, devicetree@...r.kernel.org,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Rob Herring <robh@...nel.org>, linux-arm-msm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        Conor Dooley <conor+dt@...nel.org>, Benson Leung <bleung@...omium.org>,
        chrome-platform@...ts.linux.dev, Pin-yen Lin <treapking@...omium.org>,
        Abhishek Pandit-Subedi <abhishekpandit@...omium.org>,
        Łukasz Bartosik <ukaszb@...omium.org>,
        Jameson Thies <jthies@...gle.com>,
        Andrei Kuchynski <akuchynski@...omium.org>
Subject: Re: [PATCH 7/7] platform/chrome: cros_ec_typec: Support DP muxing

On Tue, Apr 15, 2025 at 05:02:07PM -0700, Stephen Boyd wrote:
> Most ARM based chromebooks with two usb-c-connector nodes and one DP
> controller are muxing the DP lanes between the two USB ports. This is
> done so that the type-c ports are at least equal in capability if not
> functionality. Either an analog mux is used to steer the DP signal to
> one or the other port, or a DP bridge chip has two lanes (e.g. DP
> ML0/ML1) wired to one type-c port while the other two (e.g. DP ML2/ML3)
> are wired to another type-c port.
> 
> If a user connects a DP capable cable to both usb-c-connectors the EC
> likes to inform the AP that both ports have entered DP altmode, even
> though one of those ports can't actually display anything because the DP
> lanes aren't steered there. The answer to this problem is to look at the
> HPD bit in the EC messages. The port that isn't steered for DP won't
> ever see HPD be asserted, because the EC hides HPD state for the other
> port. This isn't a great solution though, because some EC firmwares
> don't even signal HPD state in the message at all. Oops! And it really
> does throw the whole type-c subsystem for a loop when the port has DP
> altmode present but it can't be entered properly.
> 
> Let's fix these problems by doing two things.
> 
> First, we'll only allow the port that's steered for DP to enter DP mode.
> Do that by checking the mux-gpios whenever we see that the EC tells us
> DP mode has been entered. If the mux isn't selecting this port, remove
> the flag from the message so that DP mode doesn't look to be entered.
> 
> Second, inject HPD into the EC message when the EC has busted firmware.
> In this case, DT authors add 'no-hpd' to the typec node (essentially
> only on Trogdor). Listen for HPD events from the drm_bridge and read the
> mux when HPD is asserted to figure out which port actually had HPD
> asserted on it. When the port state is processed, check the bit against
> the port and if DP mode is entered, i.e. the mux is still steering
> toward that port, check if HPD is asserted on that port and inject HPD.
> This is necessary so that the typec framework can update the HPD state
> in sysfs, and eventually call drm_connector_oob_hotplug_event() from the
> DP altmode driver.
> 
> Cc: Benson Leung <bleung@...omium.org>
> Cc: Tzung-Bi Shih <tzungbi@...nel.org>
> Cc: <chrome-platform@...ts.linux.dev>
> Cc: Pin-yen Lin <treapking@...omium.org>
> Cc: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
> Cc: Łukasz Bartosik <ukaszb@...omium.org>
> Cc: Jameson Thies <jthies@...gle.com>
> Cc: Andrei Kuchynski <akuchynski@...omium.org>
> Signed-off-by: Stephen Boyd <swboyd@...omium.org>
> ---
>  drivers/platform/chrome/cros_ec_typec.c      | 115 +++++++++++++++++++
>  drivers/platform/chrome/cros_ec_typec.h      |  14 +++
>  drivers/platform/chrome/cros_typec_altmode.c |   2 +
>  3 files changed, 131 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 27324cf0c0c6..10079129645d 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_graph.h>
> @@ -427,6 +428,41 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
>  	return ret;
>  }
>  
> +static void cros_typec_dp_bridge_hpd_notify(struct drm_bridge *bridge, enum drm_connector_status status)

Okay, I can see why you've implemented the bride on your own, but I
can't say that I'm happy with it. For example, for such bridges it's
relatively easy to miss the interlace_allowed and ycbcr_420_allowed
flags (which you did). Likewise it makes it harder for Luca and other
developers to review / rework bridge lifetime.

> +{
> +	struct cros_typec_dp_bridge *dp_bridge = bridge_to_cros_typec_dp_bridge(bridge);
> +	struct cros_typec_data *typec = dp_bridge->typec_data;
> +	struct gpio_desc *mux_gpio = dp_bridge->mux_gpio;
> +	int val;
> +	DECLARE_BITMAP(orig, EC_USB_PD_MAX_PORTS);
> +	DECLARE_BITMAP(changed, EC_USB_PD_MAX_PORTS);
> +
> +	if (!mux_gpio)
> +		return;
> +
> +	/* This bridge signals HPD so it must be able to detect HPD properly */
> +	if (dp_bridge->bridge.ops & DRM_BRIDGE_OP_HPD)
> +		return;
> +
> +	bitmap_copy(orig, dp_bridge->hpd_asserted, EC_USB_PD_MAX_PORTS);
> +	bitmap_zero(changed, EC_USB_PD_MAX_PORTS);
> +
> +	if (status == connector_status_connected) {
> +		val = gpiod_get_value_cansleep(mux_gpio);
> +		if (val < 0) {
> +			dev_err(typec->dev, "Failed to read mux gpio\n");
> +			return;
> +		}
> +		__set_bit(val, changed);
> +	}
> +
> +	bitmap_copy(dp_bridge->hpd_asserted, changed, EC_USB_PD_MAX_PORTS);

This looks like a home-made reimplementation of test_and_set_bit() /
test_and_clear_bit(). Can those functions be used instead?
Or simply store GPIO value under the lock?

> +
> +	/* Refresh port state. */
> +	if (!bitmap_equal(orig, changed, EC_USB_PD_MAX_PORTS))
> +		schedule_work(&typec->port_work);
> +}
> +
>  static int cros_typec_dp_bridge_attach(struct drm_bridge *bridge,
>  				       enum drm_bridge_attach_flags flags)
>  {
> @@ -435,6 +471,7 @@ static int cros_typec_dp_bridge_attach(struct drm_bridge *bridge,
>  
>  static const struct drm_bridge_funcs cros_typec_dp_bridge_funcs = {
>  	.attach	= cros_typec_dp_bridge_attach,
> +	.hpd_notify = cros_typec_dp_bridge_hpd_notify,
>  };
>  
>  static int cros_typec_init_dp_bridge(struct cros_typec_data *typec)
> @@ -452,6 +489,11 @@ static int cros_typec_init_dp_bridge(struct cros_typec_data *typec)
>  	if (!dp_bridge)
>  		return -ENOMEM;
>  	typec->dp_bridge = dp_bridge;
> +	dp_bridge->typec_data = typec;
> +
> +	dp_bridge->mux_gpio = devm_gpiod_get_optional(dev, "mux", GPIOD_ASIS);
> +	if (IS_ERR(dp_bridge->mux_gpio))
> +		return dev_err_probe(dev, PTR_ERR(dp_bridge->mux_gpio), "failed to get mux gpio\n");
>  
>  	bridge = &dp_bridge->bridge;
>  	bridge->funcs = &cros_typec_dp_bridge_funcs;
> @@ -662,6 +704,77 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec,
>  	return typec_mux_set(port->mux, &port->state);
>  }
>  
> +/*
> + * Some ECs like to tell AP that both ports have DP enabled when that's
> + * impossible because the EC is muxing DP to one or the other port. Check the
> + * mux on the EC in this case and ignore what the EC tells us about DP on the
> + * port that isn't actually muxed for DP.
> + */
> +void cros_typec_check_dp(struct cros_typec_data *typec,
> +			 struct ec_response_usb_pd_mux_info *resp,
> +			 struct cros_typec_port *port)
> +{
> +	struct cros_typec_dp_bridge *dp_bridge = typec->dp_bridge;
> +	struct gpio_desc *mux_gpio;
> +	int val;
> +
> +	/* Never registered a drm_bridge. Skip. */
> +	if (!dp_bridge)
> +		return;
> +
> +	/* Don't need to override DP enabled when DP isn't enabled. */
> +	if (!(resp->flags & USB_PD_MUX_DP_ENABLED))
> +		return;
> +
> +	mux_gpio = dp_bridge->mux_gpio;
> +	/* EC mux is required to determine which port actually has DP on it. */
> +	if (!mux_gpio)
> +		return;
> +
> +	val = gpiod_get_value_cansleep(mux_gpio);
> +	if (val < 0) {
> +		dev_err(typec->dev, "Failed to read mux gpio\n");
> +		return;
> +	}
> +
> +	/* Only the muxed port can have DP enabled. Ignore. */
> +	if (val != port->port_num)
> +		resp->flags &= ~USB_PD_MUX_DP_ENABLED;
> +}
> +
> +/*
> + * Some ECs don't notify AP when HPD goes high or low because their firmware is
> + * broken. Capture the state of HPD in cros_typec_dp_bridge_hpd_notify() and
> + * inject the asserted state into the EC's response (deasserted is the
> + * default).
> + */
> +static void cros_typec_inject_hpd(struct cros_typec_data *typec,
> +				  struct ec_response_usb_pd_mux_info *resp,
> +				  struct cros_typec_port *port)
> +{
> +	struct cros_typec_dp_bridge *dp_bridge = typec->dp_bridge;
> +
> +	/* Never registered a drm_bridge. Skip. */
> +	if (!dp_bridge)
> +		return;
> +
> +	/* Don't need to inject HPD level when DP isn't enabled. */
> +	if (!(resp->flags & USB_PD_MUX_DP_ENABLED))
> +		return;
> +
> +	/* This bridge signals HPD so it doesn't need to be reinjected */
> +	if (dp_bridge->bridge.ops & DRM_BRIDGE_OP_HPD)
> +		return;
> +
> +	/*
> +	 * The default setting is HPD deasserted. Ignore if nothing to inject.
> +	 */
> +	if (!test_bit(port->port_num, dp_bridge->hpd_asserted))
> +		return;
> +
> +	resp->flags |= USB_PD_MUX_HPD_LVL;
> +}
> +
>  static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
>  				struct ec_response_usb_pd_control_v2 *pd_ctrl)
>  {
> @@ -682,6 +795,8 @@ static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
>  			 port_num, ret);
>  		return ret;
>  	}
> +	cros_typec_check_dp(typec, &resp, port);
> +	cros_typec_inject_hpd(typec, &resp, port);
>  
>  	/* No change needs to be made, let's exit early. */
>  	if (port->mux_flags == resp.flags && port->role == pd_ctrl->role)
> diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h
> index 090f8f5c0492..b4b331aa5dc7 100644
> --- a/drivers/platform/chrome/cros_ec_typec.h
> +++ b/drivers/platform/chrome/cros_ec_typec.h
> @@ -6,6 +6,7 @@
>  #include <linux/list.h>
>  #include <linux/notifier.h>
>  #include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/types.h>
>  #include <linux/usb/pd.h>
>  #include <linux/usb/role.h>
>  #include <linux/usb/typec.h>
> @@ -88,6 +89,19 @@ struct cros_typec_port {
>  
>  struct cros_typec_dp_bridge {
>  	struct drm_bridge bridge;
> +	struct cros_typec_data *typec_data;
> +	struct gpio_desc *mux_gpio;
> +	DECLARE_BITMAP(hpd_asserted, EC_USB_PD_MAX_PORTS);
>  };
>  
> +static inline struct cros_typec_dp_bridge *
> +bridge_to_cros_typec_dp_bridge(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct cros_typec_dp_bridge, bridge);
> +}
> +
> +void cros_typec_check_dp(struct cros_typec_data *typec,
> +			 struct ec_response_usb_pd_mux_info *resp,
> +			 struct cros_typec_port *port);
> +
>  #endif /*  __CROS_EC_TYPEC__ */
> diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
> index 97ca4cfabbc0..10d21da592f1 100644
> --- a/drivers/platform/chrome/cros_typec_altmode.c
> +++ b/drivers/platform/chrome/cros_typec_altmode.c
> @@ -82,6 +82,7 @@ static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo)
>  		if (ret < 0)
>  			return ret;
>  
> +		cros_typec_check_dp(adata->port->typec_data, &resp, adata->port);
>  		if (!(resp.flags & flags))
>  			return -EINVAL;
>  	} else {
> @@ -147,6 +148,7 @@ static int cros_typec_altmode_exit(struct typec_altmode *alt)
>  		if (ret < 0)
>  			return ret;
>  
> +		cros_typec_check_dp(adata->port->typec_data, &resp, adata->port);
>  		if (resp.flags & flags)
>  			return -EINVAL;
>  	} else {
> -- 
> https://chromeos.dev
> 

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists