2022-06-22 17:45:10

by Prashant Malani

[permalink] [raw]
Subject: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

From: Pin-Yen Lin <[email protected]>

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.

Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Reviewed-by: NĂ­colas F. R. A. Prado <[email protected]>
Tested-by: NĂ­colas F. R. A. Prado <[email protected]>
Signed-off-by: Pin-Yen Lin <[email protected]>
Signed-off-by: Prashant Malani <[email protected]>
---

Changes since v4:
- Patch moved to 5/9 position (since Patch v4 1/7 and 2/7 were
applied to usb-next).

Changes since v3:
- Added Reviewed-by tag from Angelo.

Changes since v2:
- Moved num_typec_switches check to beginning of function
- Made dp_connected assignments fit on one line (and removed unnecessary
parentheses)
- Added Reviewed-by and Tested-by tags.

Changes since v1:
- No changes.

drivers/gpu/drm/bridge/analogix/anx7625.c | 56 +++++++++++++++++++++++
drivers/gpu/drm/bridge/analogix/anx7625.h | 13 ++++++
2 files changed, 69 insertions(+)

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;
};
--
2.37.0.rc0.104.g0611611a94-goog


2022-06-28 19:52:03

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

Quoting Prashant Malani (2022-06-22 10:34:34)
> From: Pin-Yen Lin <[email protected]>
>
> 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.

> 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.

2022-06-28 19:52:25

by Prashant Malani

[permalink] [raw]
Subject: Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

On Tue, Jun 28, 2022 at 12:25 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Prashant Malani (2022-06-22 10:34:34)
> > From: Pin-Yen Lin <[email protected]>
> >
> > 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

2022-06-28 20:59:38

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

Quoting Prashant Malani (2022-06-28 12:48:11)
> On Tue, Jun 28, 2022 at 12:25 PM Stephen Boyd <[email protected]> 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!

2022-06-28 21:02:55

by Prashant Malani

[permalink] [raw]
Subject: Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

On Tue, Jun 28, 2022 at 1:40 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Prashant Malani (2022-06-28 12:48:11)
> > On Tue, Jun 28, 2022 at 12:25 PM Stephen Boyd <[email protected]> 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 reference this below, but in the 1 connector case, CC lines would also be
routed to the anx7625 from the usb-connector, so it will know when HPD
is asserted
or not.

>
> 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.

If the anx7625 is used just to route all lanes from 1 usb-c-connector (i.e
the USB+DP case), a mode-switch wouldn't be of much use, since one
would also route the CC lines to the built-in PD controller; so it will
already have knowledge of what mode the switch is in.

The mode-switch is likely only relevant for this hardware configuration(
it's "DP only" in the sense that the USB pins to the SoC never go anywhere).
One only has 2 SS lanes each (from each usb-c-connector).

Since there is no CC-line, the anx7625 needs to know which one has DP
enabled on it.

>
> >
> > 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.

Ack. Will add a comment in v6.

>
> >
> > 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!

2022-06-30 23:35:37

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

Quoting Prashant Malani (2022-06-28 13:56:22)
> On Tue, Jun 28, 2022 at 1:40 PM Stephen Boyd <[email protected]> wrote:
> >
> > 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.
>
> If the anx7625 is used just to route all lanes from 1 usb-c-connector (i.e
> the USB+DP case), a mode-switch wouldn't be of much use, since one
> would also route the CC lines to the built-in PD controller; so it will
> already have knowledge of what mode the switch is in.
>
> The mode-switch is likely only relevant for this hardware configuration(
> it's "DP only" in the sense that the USB pins to the SoC never go anywhere).
> One only has 2 SS lanes each (from each usb-c-connector).
>
> Since there is no CC-line, the anx7625 needs to know which one has DP
> enabled on it.

Can the CC line be "captured" and not actually sent to the anx7625? I
imagine if that is possible, maybe the CC lines would go to some
micro-controller or something that did more typec management things and
then the anx7625 driver would need to do software control of the mode
and orientation control.

2022-06-30 23:50:45

by Prashant Malani

[permalink] [raw]
Subject: Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

On Thu, Jun 30, 2022 at 4:21 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Prashant Malani (2022-06-28 13:56:22)
> > On Tue, Jun 28, 2022 at 1:40 PM Stephen Boyd <[email protected]> wrote:
> > >
> > > 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.
> >
> > If the anx7625 is used just to route all lanes from 1 usb-c-connector (i.e
> > the USB+DP case), a mode-switch wouldn't be of much use, since one
> > would also route the CC lines to the built-in PD controller; so it will
> > already have knowledge of what mode the switch is in.
> >
> > The mode-switch is likely only relevant for this hardware configuration(
> > it's "DP only" in the sense that the USB pins to the SoC never go anywhere).
> > One only has 2 SS lanes each (from each usb-c-connector).
> >
> > Since there is no CC-line, the anx7625 needs to know which one has DP
> > enabled on it.
>
> Can the CC line be "captured" and not actually sent to the anx7625?

That's what happens on Chrome OS. The cc line goes to the EC (and is "consumed"
by the TCPM (Type C Port Manager)) and signals are then sent to the AP
over the Host command interface to `cros-ec-typec`. The signals here being all
the PD messages communicated between the peripheral and the port.

> I imagine if that is possible, maybe the CC lines would go to some
> micro-controller or something that did more typec management things and
> then the anx7625 driver would need to do software control of the mode
> and orientation control.

I _guess_ that is possible (though it would seem odd to not use all the PD
control hardware in that configuration)? If an system implements it in
such a way
then:
1. mode-switch: Can be updated to do something when num_typec_switches == 1 (
in the mux_set function imp.l I haven't looked into what registers
need to be configured, since we
don't have this hardware implementation.
2. orientation-switch: This should be registered, and then flip the
lanes when the port
driver tells it the orientation is one way or another.

So, if someone uses it that way, I think the driver needs only minor
updates to support it.

2022-07-06 18:39:39

by Prashant Malani

[permalink] [raw]
Subject: Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

On Thu, Jun 30, 2022 at 4:38 PM Prashant Malani <[email protected]> wrote:
>
> On Thu, Jun 30, 2022 at 4:21 PM Stephen Boyd <[email protected]> wrote:
> >
> > Quoting Prashant Malani (2022-06-28 13:56:22)
> > > On Tue, Jun 28, 2022 at 1:40 PM Stephen Boyd <[email protected]> wrote:
> > > >
> > > > 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.
> > >
> > > If the anx7625 is used just to route all lanes from 1 usb-c-connector (i.e
> > > the USB+DP case), a mode-switch wouldn't be of much use, since one
> > > would also route the CC lines to the built-in PD controller; so it will
> > > already have knowledge of what mode the switch is in.
> > >
> > > The mode-switch is likely only relevant for this hardware configuration(
> > > it's "DP only" in the sense that the USB pins to the SoC never go anywhere).
> > > One only has 2 SS lanes each (from each usb-c-connector).
> > >
> > > Since there is no CC-line, the anx7625 needs to know which one has DP
> > > enabled on it.
> >
> > Can the CC line be "captured" and not actually sent to the anx7625?
>
> That's what happens on Chrome OS. The cc line goes to the EC (and is "consumed"
> by the TCPM (Type C Port Manager)) and signals are then sent to the AP
> over the Host command interface to `cros-ec-typec`. The signals here being all
> the PD messages communicated between the peripheral and the port.
>
> > I imagine if that is possible, maybe the CC lines would go to some
> > micro-controller or something that did more typec management things and
> > then the anx7625 driver would need to do software control of the mode
> > and orientation control.
>
> I _guess_ that is possible (though it would seem odd to not use all the PD
> control hardware in that configuration)? If an system implements it in
> such a way
> then:
> 1. mode-switch: Can be updated to do something when num_typec_switches == 1 (
> in the mux_set function imp.l I haven't looked into what registers
> need to be configured, since we
> don't have this hardware implementation.
> 2. orientation-switch: This should be registered, and then flip the
> lanes when the port
> driver tells it the orientation is one way or another.
>
> So, if someone uses it that way, I think the driver needs only minor
> updates to support it.

Stephen, any 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.

2022-07-07 00:43:53

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

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.

2022-07-12 10:35:24

by Pin-yen Lin

[permalink] [raw]
Subject: Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

On Thu, Jul 7, 2022 at 8:17 AM Stephen Boyd <[email protected]> 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.