| Message ID | 20240901040658.157425-19-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | platform/chrome: Add DT USB/DP muxing/topology support | expand |
On Sat, Aug 31, 2024 at 09:06:56PM -0700, Stephen Boyd wrote: > +static void cros_typec_inject_hpd(struct cros_typec_data *typec, > + struct ec_response_usb_pd_mux_info *resp, > + struct cros_typec_port *port) > +{ [...] > + /* > + * Only read the mux GPIO setting if we need to change the active port. > + * Otherwise, an active port is already set and HPD going high or low > + * doesn't change the muxed port until DP mode is exited. > + */ > + if (!typec->active_dp_port) { Given that cros_typec_inject_hpd() is called before `typec->active_dp_port` would be set (from previous patch "platform/chrome: ... Support DP muxing"), would it possibly wrongly fall into here at the beginning? (E.g.: cros_typec_probe() -> cros_typec_port_update() -> cros_typec_configure_mux() -> cros_typec_inject_hpd().) > [...] > + /* Inject HPD from the GPIO state if EC firmware is broken. */ > + if (typec->hpd_asserted) > + resp->flags |= USB_PD_MUX_HPD_LVL; `typec->hpd_asserted` is shared between all typec->ports[...]. Would it be possible that a HPD is asserted for another port but not current `port`? E.g.: cros_typec_inject_hpd() for port 2 and cros_typec_dp_bridge_hpd_notify() gets called due to port 1 at the same time?
Quoting Tzung-Bi Shih (2024-09-04 02:36:45) > On Sat, Aug 31, 2024 at 09:06:56PM -0700, Stephen Boyd wrote: > > +static void cros_typec_inject_hpd(struct cros_typec_data *typec, > > + struct ec_response_usb_pd_mux_info *resp, > > + struct cros_typec_port *port) > > +{ > [...] > > + /* > > + * Only read the mux GPIO setting if we need to change the active port. > > + * Otherwise, an active port is already set and HPD going high or low > > + * doesn't change the muxed port until DP mode is exited. > > + */ > > + if (!typec->active_dp_port) { > > Given that cros_typec_inject_hpd() is called before `typec->active_dp_port` > would be set (from previous patch "platform/chrome: ... Support DP muxing"), > would it possibly wrongly fall into here at the beginning? (E.g.: > cros_typec_probe() -> cros_typec_port_update() -> cros_typec_configure_mux() > -> cros_typec_inject_hpd().) We wouldn't get here if 'hpd_asserted' is false though. We want to fall into this case in the beginning, i.e. 'active_dp_port' is NULL, so that we can read the mux and figure out which port is actually selected. If we don't have a mux gpio we assume that we aren't muxing and that there's only one port to begin with. I'll add a comment after the if (mux_gpio) condition with this info. > > > [...] > > + /* Inject HPD from the GPIO state if EC firmware is broken. */ > > + if (typec->hpd_asserted) > > + resp->flags |= USB_PD_MUX_HPD_LVL; > > `typec->hpd_asserted` is shared between all typec->ports[...]. Would it be > possible that a HPD is asserted for another port but not current `port`? > E.g.: cros_typec_inject_hpd() for port 2 and cros_typec_dp_bridge_hpd_notify() > gets called due to port 1 at the same time? I'd like to avoid synchronizing the hpd notify and this injection code, if that's what you're asking. Thinking about this though, I've realized that it's broken even when HPD is working on the EC. Consider this scenario with two type-c ports C0 and C1: Plug in C1 EC notifies AP AP queues cros_typec_port_work() HPD asserted EC picks C1 for DP // First to have hpd asserted EC notifies AP AP tries to queue cros_typec_port_work() but it's pending. Skip. Plug in C0 EC notifies AP AP tries to queue cros_typec_port_work() but it's pending. Skip. HPD asserted EC notifies AP AP tries to queue cros_typec_port_work() but it's pending. Skip. Finally cros_typec_port_work() runs! for (i = 0; i < typec->num_ports; i++) // typec->num_ports = 2 cros_typec_port_update(port_num=0) cros_ec_cmd(EC_CMD_USB_PD_CONTROL.port=0) // In DP mode cros_typec_configure_mux(port_num=0) cros_ec_cmd(EC_CMD_USB_PD_MUX_INFO.port=0) // hpd asserted if (!active_dp_port) active_dp_port = port0 This is bad. The worker could be significantly delayed, although it's really unlikely in practice. It would be better if the EC pushed a message to AP about what happened, instead of having to query the EC about the state of USB. Or the EC could have a sequence number or something so AP could ask for the history of events. We can't fix all the EC firmwares though, so we get what we get. I think one solution would be to read the mux all the time and ignore tracking the active port based on hpd state. If we do that then we don't get tripped up by a delayed work iterating over both typec ports. The logic will be a bit more complicated though, because we'll have to consider all the ports when entering and exiting DP mode on one port so that we don't assign DP to the wrong port. Also, when hpd is broken on the EC I see an error message when I unplug the DP cable. It's the "No valid DP mode provided." error from cros_typec_enable_dp(). When I inject hpd that error goes away. I'll need to look closer to understand why, but I suspect I'll need to keep injecting hpd to avoid it.
On Wed, Sep 04, 2024 at 02:45:36PM -0700, Stephen Boyd wrote: > Quoting Tzung-Bi Shih (2024-09-04 02:36:45) > > On Sat, Aug 31, 2024 at 09:06:56PM -0700, Stephen Boyd wrote: > > > +static void cros_typec_inject_hpd(struct cros_typec_data *typec, > > > + struct ec_response_usb_pd_mux_info *resp, > > > + struct cros_typec_port *port) > > > +{ > > [...] > > > + /* > > > + * Only read the mux GPIO setting if we need to change the active port. > > > + * Otherwise, an active port is already set and HPD going high or low > > > + * doesn't change the muxed port until DP mode is exited. > > > + */ > > > + if (!typec->active_dp_port) { > > > > Given that cros_typec_inject_hpd() is called before `typec->active_dp_port` > > would be set (from previous patch "platform/chrome: ... Support DP muxing"), > > would it possibly wrongly fall into here at the beginning? (E.g.: > > cros_typec_probe() -> cros_typec_port_update() -> cros_typec_configure_mux() > > -> cros_typec_inject_hpd().) > > We wouldn't get here if 'hpd_asserted' is false though. We want to fall Ack, I overlooked that. > > > [...] > > > + /* Inject HPD from the GPIO state if EC firmware is broken. */ > > > + if (typec->hpd_asserted) > > > + resp->flags |= USB_PD_MUX_HPD_LVL; > > > > `typec->hpd_asserted` is shared between all typec->ports[...]. Would it be > > possible that a HPD is asserted for another port but not current `port`? > > E.g.: cros_typec_inject_hpd() for port 2 and cros_typec_dp_bridge_hpd_notify() > > gets called due to port 1 at the same time? > > I'd like to avoid synchronizing the hpd notify and this injection code, > if that's what you're asking. Thinking about this though, I've realized > that it's broken even when HPD is working on the EC. Consider this > scenario with two type-c ports C0 and C1: > > [...] I understood it more: originally, I was wondering if it needs an array `typec->hpd_asserted[...]` for storing HPD for each port. But, no. What cros_typec_dp_bridge_hpd_notify() get is just a connected/disconnected signal. It kicks off cros_typec_port_work() for finding which port is supposed to trigger the event (with some logic with `active_dp_port`, `mux_gpio`, and `hpd_asserted`). Curious about one more scenario, is it possible: Initially, no DP port and no mux is using: active_dp_port = NULL hpd_asserted = false mux_gpio = NULL CPU A CPU B ------------------------------------------------------------------------------ cros_typec_port_work() cros_typec_port_update(port_num=0) [C0 connected] cros_typec_dp_bridge_hpd_notify() hpd_asserted = true cros_typec_port_update(port_num=1) cros_typec_configure_mux(port_num=1) cros_typec_inject_hpd() active_dp_port = C1
Quoting Tzung-Bi Shih (2024-09-06 01:18:21) > On Wed, Sep 04, 2024 at 02:45:36PM -0700, Stephen Boyd wrote: > > Quoting Tzung-Bi Shih (2024-09-04 02:36:45) > > > On Sat, Aug 31, 2024 at 09:06:56PM -0700, Stephen Boyd wrote: > > > > + /* Inject HPD from the GPIO state if EC firmware is broken. */ > > > > + if (typec->hpd_asserted) > > > > + resp->flags |= USB_PD_MUX_HPD_LVL; > > > > > > `typec->hpd_asserted` is shared between all typec->ports[...]. Would it be > > > possible that a HPD is asserted for another port but not current `port`? > > > E.g.: cros_typec_inject_hpd() for port 2 and cros_typec_dp_bridge_hpd_notify() > > > gets called due to port 1 at the same time? > > > > I'd like to avoid synchronizing the hpd notify and this injection code, > > if that's what you're asking. Thinking about this though, I've realized > > that it's broken even when HPD is working on the EC. Consider this > > scenario with two type-c ports C0 and C1: > > > > [...] > > I understood it more: originally, I was wondering if it needs an array > `typec->hpd_asserted[...]` for storing HPD for each port. But, no. > > What cros_typec_dp_bridge_hpd_notify() get is just a connected/disconnected > signal. It kicks off cros_typec_port_work() for finding which port is > supposed to trigger the event (with some logic with `active_dp_port`, > `mux_gpio`, and `hpd_asserted`). Ok, cool. I intend to split this device into multiple devices, one per DP bridge. I haven't done that though because I don't have any device that has two independent DP controllers. > > > Curious about one more scenario, is it possible: > > Initially, no DP port and no mux is using: > active_dp_port = NULL > hpd_asserted = false > mux_gpio = NULL > > CPU A CPU B > ------------------------------------------------------------------------------ > cros_typec_port_work() > cros_typec_port_update(port_num=0) > [C0 connected] > cros_typec_dp_bridge_hpd_notify() > hpd_asserted = true The work is queued again here because it's already running. > cros_typec_port_update(port_num=1) > cros_typec_configure_mux(port_num=1) > cros_typec_inject_hpd() > active_dp_port = C1 Yeah it's a problem because we need to read the mux_gpio to figure out which way it's steering. We can't recreate the "first to assert HPD" logic that the EC has because we can't control when the worker runs. At least we can skip reading the mux if only one port has entered DP mode. I'm hoping that the scenario where both ports are in DP mode almost never happens, but if it does then we'll have to read the mux when hpd is asserted to figure out which port DP is muxed to.
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c index 57d1484ce1ef..731b485634af 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> @@ -423,6 +424,17 @@ static int cros_typec_init_ports(struct cros_typec_data *typec) return ret; } +static void cros_typec_dp_bridge_hpd_notify(void *data, enum drm_connector_status status) +{ + struct cros_typec_data *typec = data; + + /* Proxy the connector status as the HPD state to replay later. */ + typec->hpd_asserted = status == connector_status_connected; + + /* Refresh port state. */ + schedule_work(&typec->port_work); +} + static int cros_typec_init_dp_bridge(struct cros_typec_data *typec) { struct device *dev = typec->dev; @@ -432,9 +444,17 @@ static int cros_typec_init_dp_bridge(struct cros_typec_data *typec) if (!fwnode_property_read_bool(dev_fwnode(dev), "mode-switch")) return 0; + typec->mux_gpio = devm_gpiod_get_optional(dev, "mux", GPIOD_ASIS); + if (IS_ERR(typec->mux_gpio)) + return dev_err_probe(dev, PTR_ERR(typec->mux_gpio), "failed to get mux gpio\n"); + dp_dev = devm_drm_dp_typec_bridge_alloc(dev, dev->of_node); if (IS_ERR(dp_dev)) return PTR_ERR(dp_dev); + + if (fwnode_property_read_bool(dev_fwnode(dev), "no-hpd")) + drm_dp_typec_bridge_add_hpd_notify(dp_dev, cros_typec_dp_bridge_hpd_notify, typec); + typec->dp_bridge = dp_dev; return devm_drm_dp_typec_bridge_add(dev, dp_dev); @@ -635,6 +655,59 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec, return typec_mux_set(port->mux, &port->state); } +/* + * 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 gpio_desc *mux_gpio = typec->mux_gpio; + int val; + + /* Never registered a drm_bridge. Skip. */ + if (!typec->dp_bridge) + return; + + /* Don't need to inject HPD level when DP isn't enabled. */ + if (!(resp->flags & USB_PD_MUX_DP_ENABLED)) + return; + + /* + * The default setting is HPD deasserted. Ignore if nothing to inject. + */ + if (!typec->hpd_asserted) + return; + + /* + * Only read the mux GPIO setting if we need to change the active port. + * Otherwise, an active port is already set and HPD going high or low + * doesn't change the muxed port until DP mode is exited. + */ + if (!typec->active_dp_port) { + if (mux_gpio) { + val = gpiod_get_value_cansleep(mux_gpio); + if (val < 0) { + dev_err(typec->dev, "Failed to read mux gpio\n"); + return; + } + /* Ignore HPD changes for non-active port. */ + if (typec->ports[val] != port) + return; + } + } else if (port != typec->active_dp_port) { + /* Ignore HPD changes for non-active port. */ + return; + } + + /* Inject HPD from the GPIO state if EC firmware is broken. */ + if (typec->hpd_asserted) + 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) { @@ -656,6 +729,7 @@ static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num, port_num, ret); return ret; } + 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 f3a2b67df07c..4ccd3d014aa6 100644 --- a/drivers/platform/chrome/cros_ec_typec.h +++ b/drivers/platform/chrome/cros_ec_typec.h @@ -37,6 +37,8 @@ struct cros_typec_data { struct cros_typec_port *ports[EC_USB_PD_MAX_PORTS]; struct drm_dp_typec_bridge_dev *dp_bridge; struct cros_typec_port *active_dp_port; + struct gpio_desc *mux_gpio; + bool hpd_asserted; struct notifier_block nb; struct work_struct port_work; bool typec_cmd_supported;
Some EC firmwares on Trogdor/Strongbad boards don't properly indicate the state of DP HPD on a type-c port. Instead, the EC only indicates that a type-c port has entered or exited DP mode. To make matters worse, on these boards the DP signal is muxed between two USB type-c connectors, so we can't use the DP entry of a port to figure out which type-c port is actually displaying DP. Stash the HPD state in this case whenever the drm_bridge is notified of a connector status change and kick off the port worker so that the type-c port state can be re-evaluated. If an analog mux is in use, read the mux to figure out which type-c port signaled HPD. Once we know which port is actually signaling HPD, inject that state into the message received from the EC. This simplifies the rest of the logic as it can all stay the same with respect to picking the first port to assert HPD, etc. Cc: Prashant Malani <pmalani@chromium.org> Cc: Benson Leung <bleung@chromium.org> Cc: Tzung-Bi Shih <tzungbi@kernel.org> Cc: <chrome-platform@lists.linux.dev> Cc: Pin-yen Lin <treapking@chromium.org> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/platform/chrome/cros_ec_typec.c | 74 +++++++++++++++++++++++++ drivers/platform/chrome/cros_ec_typec.h | 2 + 2 files changed, 76 insertions(+)