Hi Mika,
On Mon, May 02, 2022 at 12:50:57PM +0300, Mika Westerberg wrote:
> Hi Gil,
>
> On Sun, May 01, 2022 at 11:33:17PM +0300, Gil Fine wrote:
> > We can't enable CLx if it is not supported either by the host or device,
> > or by the USB4/TBT link (e.g. when an optical cable is used).
> > We silently ignore CLx enabling in this case.
> >
> > Signed-off-by: Gil Fine <gil.fine@xxxxxxxxx>
> > ---
> > drivers/thunderbolt/tb.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> > index 44d04b651a8b..7419cd1aefba 100644
> > --- a/drivers/thunderbolt/tb.c
> > +++ b/drivers/thunderbolt/tb.c
> > @@ -581,6 +581,7 @@ static void tb_scan_port(struct tb_port *port)
> > struct tb_cm *tcm = tb_priv(port->sw->tb);
> > struct tb_port *upstream_port;
> > struct tb_switch *sw;
> > + int ret;
> >
> > if (tb_is_upstream_port(port))
> > return;
> > @@ -669,7 +670,9 @@ static void tb_scan_port(struct tb_port *port)
> > tb_switch_lane_bonding_enable(sw);
> > /* Set the link configured */
> > tb_switch_configure_link(sw);
> > - if (tb_switch_enable_clx(sw, TB_CL0S))
> > + /* Silently ignore CLx enabling in case CLx is not supported */
> > + ret = tb_switch_enable_clx(sw, TB_CL0S);
> > + if (ret && ret != -EOPNOTSUPP)
>
> I think we can debug log this and also:
>
> > tb_sw_warn(sw, "failed to enable CLx on upstream port\n");
>
> can we use something like "failed to enable CL%d on upstream port\n" and
> pass the CL state there too? I think it is useful to see what what CL
> state failed.
Not sure is necessary because:
for CL0s/CL1 from SW (Driver) perspective, they are actually enabled and supported together.
I mean from HW/FW perspective, entry to CL1 e.g. can be objected and only
allow entry to CL0s. But SW behaviour is similar for both.
I mean, that if SW CM fails to enable CL1, it will also fail enabling CL0s and
vice-versa.
So, it may be relevant if some-day we add CL2 support (which probably will not
happen, because the introduce of the adaptive mode in USB4 v2)
I am thinking to merge both of them to be one ENUM and to name it someting like
"TB_CL0s_CL1":
enum tb_clx {
»·······TB_CLX_DISABLE,
»·······TB_CL0s_CL1,
»·······TB_CL2,
}
And then do this:
ret = tb_switch_enable_clx(sw, TB_CL0s_CL1);
if (ret && ret != -EOPNOTSUPP)
tb_sw_warn(sw, "failed to enable %s on upstream port\n", tb_switch_clx_name(TB_CL0S_CL1));
What do you think?
--
Thanks,
Gil
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
