| Message ID | 20220622173605.1168416-6-pmalani@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | usb: typec: Introduce typec-switch binding | expand |
On Tue, Jun 28, 2022 at 12:25 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Prashant Malani (2022-06-22 10:34:34) > > From: Pin-Yen Lin <treapking@chromium.org> > > > > Add the callback function when the driver receives state > > changes of the Type-C port. The callback function configures the > > crosspoint switch of the anx7625 bridge chip, which can change the > > output pins of the signals according to the port state. > > Can this be combined with the previous two patches? They really don't > stand alone because the previous two patches are adding stubs that are > filled out later. I split it out for ease of reviewing, but sure, I will combine it if there is a v6. > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c > > index bd21f159b973..5992fc8beeeb 100644 > > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c > > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c > > @@ -15,6 +15,7 @@ > > #include <linux/regulator/consumer.h> > > #include <linux/slab.h> > > #include <linux/types.h> > > +#include <linux/usb/typec_dp.h> > > #include <linux/usb/typec_mux.h> > > #include <linux/workqueue.h> > > > > @@ -2582,9 +2583,64 @@ static void anx7625_runtime_disable(void *data) > > pm_runtime_disable(data); > > } > > > > +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx, > > + enum typec_orientation orientation) > > +{ > > + if (orientation == TYPEC_ORIENTATION_NORMAL) { > > + anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0, > > + SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2); > > + anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1, > > + SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2); > > + } else if (orientation == TYPEC_ORIENTATION_REVERSE) { > > + anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0, > > + SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1); > > + anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1, > > + SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1); > > + } > > +} > > + > > +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx) > > +{ > > + if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected) > > + /* Both ports available, do nothing to retain the current one. */ > > + return; > > + else if (ctx->typec_ports[0].dp_connected) > > + anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL); > > + else if (ctx->typec_ports[1].dp_connected) > > + anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE); > > +} > > + > > static int anx7625_typec_mux_set(struct typec_mux_dev *mux, > > struct typec_mux_state *state) > > { > > + struct anx7625_port_data *data = typec_mux_get_drvdata(mux); > > + struct anx7625_data *ctx = data->ctx; > > + struct device *dev = &ctx->client->dev; > > + bool new_dp_connected, old_dp_connected; > > + > > + if (ctx->num_typec_switches == 1) > > How do we handle the case where the usb-c-connector is directly > connected to the RX1/TX1 and RX2/TX2 pins? This device would be an > orientation (normal/reverse) and mode switch (usb/dp) in that scenario, > but this code is written in a way that the orientation switch isn't > going to flip the crosspoint switch for the different pin assignments. If all 4 SS lanes are connected to 1 usb-c-connector; there would be just 1 "typec-switch" node. In that case, the DT would only specify it as an "orientation-switch" and register an orientation-switch with the Type-C framework. The orientation switch would pretty much do what the mode-switch callback does here (configuring the crosspoint switch). One could also register a "mode-switch" there but it wouldn't do anything (all 4 lanes are already connected so there is nothing to re-route in the crosspoint switch). Hence the above "if" check. Unfortunately, I don't have hardware which connects all 4 SS lanes from 1 Type-C port to the anx7625, so I didn't add the orientation switch handling to the driver (since I have no way of verifying it). Regarding DP alt-mode pin assignments : I think anx7625 will only support Pin D (only 2 lane DP, no 4 lane DP). BR, -Prashant
Quoting Prashant Malani (2022-06-28 12:48:11) > On Tue, Jun 28, 2022 at 12:25 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Prashant Malani (2022-06-22 10:34:34) > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c > > > index bd21f159b973..5992fc8beeeb 100644 > > > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c > > > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c [..] > > > + > > > + if (ctx->num_typec_switches == 1) > > > > How do we handle the case where the usb-c-connector is directly > > connected to the RX1/TX1 and RX2/TX2 pins? This device would be an > > orientation (normal/reverse) and mode switch (usb/dp) in that scenario, > > but this code is written in a way that the orientation switch isn't > > going to flip the crosspoint switch for the different pin assignments. > > If all 4 SS lanes are connected to 1 usb-c-connector; there would be > just 1 "typec-switch" node. > In that case, the DT would only specify it as an "orientation-switch" > and register > an orientation-switch with the Type-C framework. The orientation switch would > pretty much do what the mode-switch callback does here (configuring > the crosspoint > switch). > One could also register a "mode-switch" there but it wouldn't do > anything (all 4 lanes are already > connected so there is nothing to re-route in the crosspoint switch). > Hence the above "if" check. Would we still want to route the DP traffic out if the pin assignment didn't have DP? Does the hardware support some mode where the DP traffic is shutdown? Or maybe the HPD pin needs to be quieted unless DP is assigned? I suppose none of those things matter though as long as there is some typec switch registered here so that the driver can be informed of the pin assignment. Is it right that the "mode-switch" property is only required in DT if this device is going to control the mode of the connector, i.e. USB+DP, or just DP? Where this device can't do that because it doesn't support only DP. > > Unfortunately, I don't have hardware which connects all 4 SS lanes > from 1 Type-C port > to the anx7625, so I didn't add the orientation switch handling to the > driver (since I have no way of verifying it). Alright. Maybe add a TODO then so it's more obvious that orientation isn't handled. > > Regarding DP alt-mode pin assignments : I think anx7625 will only support Pin D > (only 2 lane DP, no 4 lane DP). > Makes sense. Thanks!
Quoting Prashant Malani (2022-07-06 11:26:19) > > Stephen, any pending concerns? No more pending concerns. > If not,I will post a v6 series with the suggested changes: > - Drop typec-switch binding; instead add a new top-level port with > end-points for each Type-C connector's switch. > - Drop it6505 patches. > - Squash anx7625 driver patches into one patch. > - Add a comment mentioning that we aren't registering the orientation-switch. Ok. I'll take a look on v6.
On Thu, Jul 7, 2022 at 8:17 AM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Prashant Malani (2022-07-06 11:26:19) > > > > Stephen, any pending concerns? > > No more pending concerns. > > > If not,I will post a v6 series with the suggested changes: > > - Drop typec-switch binding; instead add a new top-level port with > > end-points for each Type-C connector's switch. > > - Drop it6505 patches. > > - Squash anx7625 driver patches into one patch. > > - Add a comment mentioning that we aren't registering the orientation-switch. We've been working on these changes, and the new DT node looks like this: ``` anx_bridge_dp: anx7625-dp@58 { [...] mode-switch; ports { [...] typec_switches: port@2 { #adderss-cells = <1>; #size-cells = <0>; reg = <2>; anx_typec0: endpoint@0 { reg = <0>; remote-endpoint = <&typec_port0>; }; anx_typec1: endpoint@1 { reg = <1>; remote-endpoint = <&typec_port1>; }; }; }; ``` However we found some issues with that approach: 1. The current typec mux API does not allow us to put muxes into `ports` directly. `fwnode_typec_mux_get` searches for the parent node behind the port(s) nodes, so we cannot register the muxes with the port nodes unless we change the interface. 2. We need a compatible string between the `endpoint` nodes and the parent node (anx7625-dp@58). This is because when the driver core builds the device links, they only add links on nodes with a compatible string for `remote-endpoint` properties[1]. Without a compatible string, the parent node of `typec_port0` (cros-ec-typec in our case) has to be probed before anx7625, but this leads to a deadlock because cros-ec-typec requires anx7625 to register the typec_mux drivers first. I'm not sure if this is cros-ec-typec specific, though. *Any* compatible string fixes this issue, and it doesn't have to be "typec-switch". -- Alternatively, can we split the two muxes into two sub-nodes, like the following snippet? ``` anx_bridge_dp: anx7625-dp@58 { [...] mode-switch; anx_mux0 { compatible = "typec-switch"; reg = <0>; port { anx_typec0: endpoint { remote-endpoint = <&typec_port0>; }; }; }; anx_mux1 { compatible = "typec-switch"; reg = <1>; port { anx_typec1: endpoint { remote-endpoint = <&typec_port1>; }; }; }; ``` This eliminates the additional "switches" node in the devicetree. The sub-nodes also describe our hardware design, which split the DP lanes of anx7625 to two type-c ports. [1]: The `node_not_dev` property searches for a node with a compatible string: https://elixir.bootlin.com/linux/latest/source/drivers/of/property.c#L1390 > > Ok. I'll take a look on v6.
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c index bd21f159b973..5992fc8beeeb 100644 --- a/drivers/gpu/drm/bridge/analogix/anx7625.c +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c @@ -15,6 +15,7 @@ #include <linux/regulator/consumer.h> #include <linux/slab.h> #include <linux/types.h> +#include <linux/usb/typec_dp.h> #include <linux/usb/typec_mux.h> #include <linux/workqueue.h> @@ -2582,9 +2583,64 @@ static void anx7625_runtime_disable(void *data) pm_runtime_disable(data); } +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx, + enum typec_orientation orientation) +{ + if (orientation == TYPEC_ORIENTATION_NORMAL) { + anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0, + SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2); + anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1, + SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2); + } else if (orientation == TYPEC_ORIENTATION_REVERSE) { + anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0, + SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1); + anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1, + SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1); + } +} + +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx) +{ + if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected) + /* Both ports available, do nothing to retain the current one. */ + return; + else if (ctx->typec_ports[0].dp_connected) + anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL); + else if (ctx->typec_ports[1].dp_connected) + anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE); +} + static int anx7625_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *state) { + struct anx7625_port_data *data = typec_mux_get_drvdata(mux); + struct anx7625_data *ctx = data->ctx; + struct device *dev = &ctx->client->dev; + bool new_dp_connected, old_dp_connected; + + if (ctx->num_typec_switches == 1) + return 0; + + old_dp_connected = ctx->typec_ports[0].dp_connected || ctx->typec_ports[1].dp_connected; + + dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n", + ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected); + + data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID && + state->alt->mode == USB_TYPEC_DP_MODE); + + new_dp_connected = ctx->typec_ports[0].dp_connected || ctx->typec_ports[1].dp_connected; + + /* dp on, power on first */ + if (!old_dp_connected && new_dp_connected) + pm_runtime_get_sync(dev); + + anx7625_typec_two_ports_update(ctx); + + /* dp off, power off last */ + if (old_dp_connected && !new_dp_connected) + pm_runtime_put_sync(dev); + return 0; } diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h index 76cfc64f7574..7d6c6fdf9a3a 100644 --- a/drivers/gpu/drm/bridge/analogix/anx7625.h +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h @@ -55,6 +55,18 @@ #define HPD_STATUS_CHANGE 0x80 #define HPD_STATUS 0x80 +#define TCPC_SWITCH_0 0xB4 +#define SW_SEL1_DPTX0_RX2 BIT(0) +#define SW_SEL1_DPTX0_RX1 BIT(1) +#define SW_SEL1_SSRX_RX2 BIT(4) +#define SW_SEL1_SSRX_RX1 BIT(5) + +#define TCPC_SWITCH_1 0xB5 +#define SW_SEL2_DPTX1_TX2 BIT(0) +#define SW_SEL2_DPTX1_TX1 BIT(1) +#define SW_SEL2_SSTX_TX2 BIT(4) +#define SW_SEL2_SSTX_TX1 BIT(5) + /******** END of I2C Address 0x58 ********/ /***************************************************************/ @@ -444,6 +456,7 @@ struct anx7625_i2c_client { }; struct anx7625_port_data { + bool dp_connected; struct typec_mux_dev *typec_mux; struct anx7625_data *ctx; };