[PATCH 0/5] thunderbolt: CL1 support for USB4 and Titan Ridge

[PATCH 0/5] thunderbolt: CL1 support for USB4 and Titan Ridge

[Gil Fine]

In this series of patches, first, we address several issues in the CL0s
implementation.
Then, we add support for a second low power state of the
link: CL1. Low power states (called collectively CLx) are used to reduce
transmitter and receiver power when a high-speed lane is idle.
We enable it, if both sides of the link support it, and only for the
first hop router (i.e. the first device that connected to the
host router). This is needed for better thermal management.
CL1 improves power management that was intoduced by CL0s.
Also, we add support of dynamic change of TMU mode to Hifi-Uni once DP
tunnel is created. This enables CL0s while DP tunnel exists.
Due to Intel HW limitation, once we changed the TMU mode to Hifi-Uni
(when DP tunnel exists), we don't change TMU mode back to Normal-Uni,
even if DP tunnel is teared-down later.

Gil Fine (5):
  thunderbolt: Silently ignore CLx enabling in case CLx is not supported
  thunderbolt: CLx disable before system suspend only if previously
    enabled
  thunderbolt: Change downstream router's TMU rate in both TMU uni/bidir
    mode
  thunderbolt: Add CL1 support for USB4 and Titan Ridge routers
  thunderbolt: Change TMU mode to Hifi-Uni once DP tunneled

 drivers/thunderbolt/switch.c  |  85 ++++++++-------
 drivers/thunderbolt/tb.c      |  70 +++++++++++--
 drivers/thunderbolt/tb.h      |  22 +++-
 drivers/thunderbolt/tb_regs.h |   8 ++
 drivers/thunderbolt/tmu.c     | 192 ++++++++++++++++++++++++++++------
 5 files changed, 295 insertions(+), 82 deletions(-)

-- 
2.17.1

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

[PATCH 1/5] thunderbolt: Silently ignore CLx enabling in case CLx is not supported

[Gil Fine]

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@intel.com>
---
 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)
 		tb_sw_warn(sw, "failed to enable CLx on upstream port\n");
 
 	tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI,
@@ -1452,12 +1455,15 @@ static int tb_suspend_noirq(struct tb *tb)
 static void tb_restore_children(struct tb_switch *sw)
 {
 	struct tb_port *port;
+	int ret;
 
 	/* No need to restore if the router is already unplugged */
 	if (sw->is_unplugged)
 		return;
 
-	if (tb_switch_enable_clx(sw, TB_CL0S))
+	/* Silently ignore CLx re-enabling in case CLx is not supported */
+	ret = tb_switch_enable_clx(sw, TB_CL0S);
+	if (ret && ret != -EOPNOTSUPP)
 		tb_sw_warn(sw, "failed to re-enable CLx on upstream port\n");
 
 	/*
-- 
2.17.1

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

Re: [PATCH 1/5] thunderbolt: Silently ignore CLx enabling in case CLx is not supported

[Mika Westerberg]

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@intel.com>
> ---
>  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.

Re: [PATCH 1/5] thunderbolt: Silently ignore CLx enabling in case CLx is not supported

[Gil Fine]

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@intel.com>
> > ---
> >  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.

Re: [PATCH 1/5] thunderbolt: Silently ignore CLx enabling in case CLx is not supported

[Mika Westerberg]

Hi,

On Wed, May 04, 2022 at 01:51:27PM +0300, Gil Fine wrote:
> 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@intel.com>
> > > ---
> > >  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.

OK

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

If they are always enabled together then sure but I suggest we call the
state then TB_CL1 and that includes both states (we can add comment if
needed, probably not if that's clear from the USB4 spec).

[PATCH 2/5] thunderbolt: CLx disable before system suspend only if previously enabled

[Gil Fine]

Disable CLx before system suspended only if previously was enabled.
Also fix few typos.

Signed-off-by: Gil Fine <gil.fine@intel.com>
---
 drivers/thunderbolt/switch.c | 8 +++++---
 drivers/thunderbolt/tmu.c    | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index ac87e8b50e52..42b7daaf9c4d 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -3063,8 +3063,10 @@ void tb_switch_suspend(struct tb_switch *sw, bool runtime)
 	 * Actually only needed for Titan Ridge but for simplicity can be
 	 * done for USB4 device too as CLx is re-enabled at resume.
 	 */
-	if (tb_switch_disable_clx(sw, TB_CL0S))
-		tb_sw_warn(sw, "failed to disable CLx on upstream port\n");
+	if (tb_switch_is_clx_enabled(sw)) {
+		if (tb_switch_disable_clx(sw, TB_CL0S))
+			tb_sw_warn(sw, "failed to disable CLx on upstream port\n");
+	}
 
 	err = tb_plug_events_active(sw, false);
 	if (err)
@@ -3483,7 +3485,7 @@ static int tb_switch_enable_cl0s(struct tb_switch *sw)
  * to improve performance. CLx is enabled only if both sides of the link
  * support CLx, and if both sides of the link are not configured as two
  * single lane links and only if the link is not inter-domain link. The
- * complete set of conditions is descibed in CM Guide 1.0 section 8.1.
+ * complete set of conditions is described in CM Guide 1.0 section 8.1.
  *
  * Return: Returns 0 on success or an error code on failure.
  */
diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c
index e4a07a26f693..b656659d02fb 100644
--- a/drivers/thunderbolt/tmu.c
+++ b/drivers/thunderbolt/tmu.c
@@ -606,7 +606,7 @@ int tb_switch_tmu_enable(struct tb_switch *sw)
 /**
  * tb_switch_tmu_configure() - Configure the TMU rate and directionality
  * @sw: Router whose mode to change
- * @rate: Rate to configure Off/LowRes/HiFi
+ * @rate: Rate to configure Off/Normal/HiFi
  * @unidirectional: If uni-directional (bi-directional otherwise)
  *
  * Selects the rate of the TMU and directionality (uni-directional or
-- 
2.17.1

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

Re: [PATCH 2/5] thunderbolt: CLx disable before system suspend only if previously enabled

[Mika Westerberg]

Hi Gil,

On Sun, May 01, 2022 at 11:33:18PM +0300, Gil Fine wrote:
> Disable CLx before system suspended only if previously was enabled.
> Also fix few typos.

Can you make that a separate patch?

Re: [PATCH 2/5] thunderbolt: CLx disable before system suspend only if previously enabled

[Gil Fine]

Hi Mika,

On Mon, May 02, 2022 at 12:52:19PM +0300, Mika Westerberg wrote:
> Hi Gil,
> 
> On Sun, May 01, 2022 at 11:33:18PM +0300, Gil Fine wrote:
> > Disable CLx before system suspended only if previously was enabled.
> > Also fix few typos.
> 
> Can you make that a separate patch?

Sure, will do.
-- 
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.

[PATCH 3/5] thunderbolt: Change downstream router's TMU rate in both TMU uni/bidir mode

[Gil Fine]

In case of uni-directional time sync, TMU handshake is
initiated by upstream router. In case of bi-directional
time sync, TMU handshake is initiated by downstream router.
In order to handle correctly the case of uni-directional mode,
we avoid changing the upstream router's rate to off,
because it might have another downstream router plugged that is set to
uni-directional mode (and we don't want to change its mode).
Instead, we always change downstream router's rate.

Signed-off-by: Gil Fine <gil.fine@intel.com>
---
 drivers/thunderbolt/tmu.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c
index b656659d02fb..985ca43b8f39 100644
--- a/drivers/thunderbolt/tmu.c
+++ b/drivers/thunderbolt/tmu.c
@@ -359,13 +359,14 @@ int tb_switch_tmu_disable(struct tb_switch *sw)
 		 * In case of uni-directional time sync, TMU handshake is
 		 * initiated by upstream router. In case of bi-directional
 		 * time sync, TMU handshake is initiated by downstream router.
-		 * Therefore, we change the rate to off in the respective
-		 * router.
+		 * We change downstream router's rate to off for both uni/bidir
+		 * cases although it is needed only for the bi-directional mode.
+		 * We avoid changing upstream router's mode since it might
+		 * have another downstream router plugged, that is set to
+		 * uni-directional mode and we don't want to change it's TMU
+		 * mode.
 		 */
-		if (unidirectional)
-			tb_switch_tmu_rate_write(parent, TB_SWITCH_TMU_RATE_OFF);
-		else
-			tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_OFF);
+		tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_OFF);
 
 		tb_port_tmu_time_sync_disable(up);
 		ret = tb_port_tmu_time_sync_disable(down);
-- 
2.17.1

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

[PATCH 4/5] thunderbolt: Add CL1 support for USB4 and Titan Ridge routers

[Gil Fine]

In this patch we add support for a second low power state of the link: CL1.
Low power states (called collectively CLx) are used to reduce
transmitter and receiver power when a high-speed lane is idle.
We enable it, if both sides of the link support it,
and only for the first hop router (i.e. the first device that connected
to the host router). This is needed for better thermal management.

Signed-off-by: Gil Fine <gil.fine@intel.com>
---
 drivers/thunderbolt/switch.c  |  79 ++++++++-------
 drivers/thunderbolt/tb.c      |  24 +++--
 drivers/thunderbolt/tb.h      |  22 ++++-
 drivers/thunderbolt/tb_regs.h |   8 ++
 drivers/thunderbolt/tmu.c     | 177 ++++++++++++++++++++++++++++------
 5 files changed, 237 insertions(+), 73 deletions(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 42b7daaf9c4d..4288cb5550f8 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -3064,7 +3064,7 @@ void tb_switch_suspend(struct tb_switch *sw, bool runtime)
 	 * done for USB4 device too as CLx is re-enabled at resume.
 	 */
 	if (tb_switch_is_clx_enabled(sw)) {
-		if (tb_switch_disable_clx(sw, TB_CL0S))
+		if (tb_switch_disable_clx(sw, TB_CL1))
 			tb_sw_warn(sw, "failed to disable CLx on upstream port\n");
 	}
 
@@ -3358,12 +3358,12 @@ static bool tb_port_clx_supported(struct tb_port *port, enum tb_clx clx)
 
 	switch (clx) {
 	case TB_CL0S:
+	case TB_CL1:
 		/* CL0s support requires also CL1 support */
 		mask = LANE_ADP_CS_0_CL0S_SUPPORT | LANE_ADP_CS_0_CL1_SUPPORT;
 		break;
 
-	/* For now we support only CL0s. Not CL1, CL2 */
-	case TB_CL1:
+	/* For now we support only CL0s and CL1. Not CL2 */
 	case TB_CL2:
 	default:
 		return false;
@@ -3377,12 +3377,7 @@ static bool tb_port_clx_supported(struct tb_port *port, enum tb_clx clx)
 	return !!(val & mask);
 }
 
-static inline bool tb_port_cl0s_supported(struct tb_port *port)
-{
-	return tb_port_clx_supported(port, TB_CL0S);
-}
-
-static int __tb_port_cl0s_set(struct tb_port *port, bool enable)
+static int __tb_port_cl0s_cl1_set(struct tb_port *port, bool enable)
 {
 	u32 phy, mask;
 	int ret;
@@ -3403,20 +3398,32 @@ static int __tb_port_cl0s_set(struct tb_port *port, bool enable)
 			     port->cap_phy + LANE_ADP_CS_1, 1);
 }
 
-static int tb_port_cl0s_disable(struct tb_port *port)
+static int tb_port_cl0s_cl1_disable(struct tb_port *port)
+{
+	return __tb_port_cl0s_cl1_set(port, false);
+}
+
+static int tb_port_cl0s_cl1_enable(struct tb_port *port)
 {
-	return __tb_port_cl0s_set(port, false);
+	return __tb_port_cl0s_cl1_set(port, true);
 }
 
-static int tb_port_cl0s_enable(struct tb_port *port)
+static const char *tb_switch_clx_name(enum tb_clx clx)
 {
-	return __tb_port_cl0s_set(port, true);
+	switch (clx) {
+	case TB_CL0S:
+		return "CL0s";
+	case TB_CL1:
+		return "CL1";
+	default:
+		return "Unknown";
+	}
 }
 
-static int tb_switch_enable_cl0s(struct tb_switch *sw)
+static int __tb_switch_enable_clx(struct tb_switch *sw, enum tb_clx clx)
 {
 	struct tb_switch *parent = tb_switch_parent(sw);
-	bool up_cl0s_support, down_cl0s_support;
+	bool up_clx_support, down_clx_support;
 	struct tb_port *up, *down;
 	int ret;
 
@@ -3441,37 +3448,37 @@ static int tb_switch_enable_cl0s(struct tb_switch *sw)
 	up = tb_upstream_port(sw);
 	down = tb_port_at(tb_route(sw), parent);
 
-	up_cl0s_support = tb_port_cl0s_supported(up);
-	down_cl0s_support = tb_port_cl0s_supported(down);
+	up_clx_support = tb_port_clx_supported(up, clx);
+	down_clx_support = tb_port_clx_supported(down, clx);
 
-	tb_port_dbg(up, "CL0s %ssupported\n",
-		    up_cl0s_support ? "" : "not ");
-	tb_port_dbg(down, "CL0s %ssupported\n",
-		    down_cl0s_support ? "" : "not ");
+	tb_port_dbg(up, "%s %ssupported\n", tb_switch_clx_name(clx),
+		    up_clx_support ? "" : "not ");
+	tb_port_dbg(down, "%s %ssupported\n", tb_switch_clx_name(clx),
+		    down_clx_support ? "" : "not ");
 
-	if (!up_cl0s_support || !down_cl0s_support)
+	if (!up_clx_support || !down_clx_support)
 		return -EOPNOTSUPP;
 
-	ret = tb_port_cl0s_enable(up);
+	ret = tb_port_cl0s_cl1_enable(up);
 	if (ret)
 		return ret;
 
-	ret = tb_port_cl0s_enable(down);
+	ret = tb_port_cl0s_cl1_enable(down);
 	if (ret) {
-		tb_port_cl0s_disable(up);
+		tb_port_cl0s_cl1_disable(up);
 		return ret;
 	}
 
 	ret = tb_switch_mask_clx_objections(sw);
 	if (ret) {
-		tb_port_cl0s_disable(up);
-		tb_port_cl0s_disable(down);
+		tb_port_cl0s_cl1_disable(up);
+		tb_port_cl0s_cl1_disable(down);
 		return ret;
 	}
 
-	sw->clx = TB_CL0S;
+	sw->clx = clx;
 
-	tb_port_dbg(up, "CL0s enabled\n");
+	tb_port_dbg(up, "%s enabled\n", tb_switch_clx_name(clx));
 	return 0;
 }
 
@@ -3505,14 +3512,15 @@ int tb_switch_enable_clx(struct tb_switch *sw, enum tb_clx clx)
 
 	switch (clx) {
 	case TB_CL0S:
-		return tb_switch_enable_cl0s(sw);
+	case TB_CL1:
+		return __tb_switch_enable_clx(sw, clx);
 
 	default:
 		return -EOPNOTSUPP;
 	}
 }
 
-static int tb_switch_disable_cl0s(struct tb_switch *sw)
+static int tb_switch_disable_cl0s_cl1(struct tb_switch *sw)
 {
 	struct tb_switch *parent = tb_switch_parent(sw);
 	struct tb_port *up, *down;
@@ -3534,17 +3542,17 @@ static int tb_switch_disable_cl0s(struct tb_switch *sw)
 
 	up = tb_upstream_port(sw);
 	down = tb_port_at(tb_route(sw), parent);
-	ret = tb_port_cl0s_disable(up);
+	ret = tb_port_cl0s_cl1_disable(up);
 	if (ret)
 		return ret;
 
-	ret = tb_port_cl0s_disable(down);
+	ret = tb_port_cl0s_cl1_disable(down);
 	if (ret)
 		return ret;
 
 	sw->clx = TB_CLX_DISABLE;
 
-	tb_port_dbg(up, "CL0s disabled\n");
+	tb_port_dbg(up, "CL0s, CL1 disabled\n");
 	return 0;
 }
 
@@ -3562,7 +3570,8 @@ int tb_switch_disable_clx(struct tb_switch *sw, enum tb_clx clx)
 
 	switch (clx) {
 	case TB_CL0S:
-		return tb_switch_disable_cl0s(sw);
+	case TB_CL1:
+		return tb_switch_disable_cl0s_cl1(sw);
 
 	default:
 		return -EOPNOTSUPP;
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 7419cd1aefba..05a084e3e9f6 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -221,7 +221,7 @@ static int tb_enable_tmu(struct tb_switch *sw)
 	int ret;
 
 	/* If it is already enabled in correct mode, don't touch it */
-	if (tb_switch_tmu_hifi_is_enabled(sw, sw->tmu.unidirectional_request))
+	if (tb_switch_tmu_is_enabled(sw, sw->tmu.unidirectional_request))
 		return 0;
 
 	ret = tb_switch_tmu_disable(sw);
@@ -671,12 +671,19 @@ static void tb_scan_port(struct tb_port *port)
 	/* Set the link configured */
 	tb_switch_configure_link(sw);
 	/* Silently ignore CLx enabling in case CLx is not supported */
-	ret = tb_switch_enable_clx(sw, TB_CL0S);
+	ret = tb_switch_enable_clx(sw, TB_CL1);
 	if (ret && ret != -EOPNOTSUPP)
 		tb_sw_warn(sw, "failed to enable CLx on upstream port\n");
 
-	tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI,
-				tb_switch_is_clx_enabled(sw));
+	if (tb_switch_is_clx_enabled(sw))
+		/*
+		 * To support highest CLx state, we set host router's TMU to
+		 * Normal-Uni mode.
+		 */
+		tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_NORMAL, true);
+	else
+		/* If CLx disabled, configure router's TMU to HiFi-Bidir mode*/
+		tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI, false);
 
 	if (tb_enable_tmu(sw))
 		tb_sw_warn(sw, "failed to enable TMU\n");
@@ -1416,7 +1423,12 @@ static int tb_start(struct tb *tb)
 		return ret;
 	}
 
-	tb_switch_tmu_configure(tb->root_switch, TB_SWITCH_TMU_RATE_HIFI, false);
+	/*
+	 * To support highest CLx state, we set host router's TMU to
+	 * Normal mode.
+	 */
+	tb_switch_tmu_configure(tb->root_switch, TB_SWITCH_TMU_RATE_NORMAL,
+				false);
 	/* Enable TMU if it is off */
 	tb_switch_tmu_enable(tb->root_switch);
 	/* Full scan to discover devices added before the driver was loaded. */
@@ -1462,7 +1474,7 @@ static void tb_restore_children(struct tb_switch *sw)
 		return;
 
 	/* Silently ignore CLx re-enabling in case CLx is not supported */
-	ret = tb_switch_enable_clx(sw, TB_CL0S);
+	ret = tb_switch_enable_clx(sw, TB_CL1);
 	if (ret && ret != -EOPNOTSUPP)
 		tb_sw_warn(sw, "failed to re-enable CLx on upstream port\n");
 
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index ad025ff142ba..6a7204cf67f1 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -934,17 +934,17 @@ void tb_switch_tmu_configure(struct tb_switch *sw,
 			     enum tb_switch_tmu_rate rate,
 			     bool unidirectional);
 /**
- * tb_switch_tmu_hifi_is_enabled() - Checks if the specified TMU mode is enabled
+ * tb_switch_tmu_is_enabled() - Checks if the specified TMU mode is enabled
  * @sw: Router whose TMU mode to check
  * @unidirectional: If uni-directional (bi-directional otherwise)
  *
  * Return true if hardware TMU configuration matches the one passed in
- * as parameter. That is HiFi and either uni-directional or bi-directional.
+ * as parameter. That is HiFi/Normal and either uni-directional or bi-directional.
  */
-static inline bool tb_switch_tmu_hifi_is_enabled(const struct tb_switch *sw,
-						 bool unidirectional)
+static inline bool tb_switch_tmu_is_enabled(const struct tb_switch *sw,
+					    bool unidirectional)
 {
-	return sw->tmu.rate == TB_SWITCH_TMU_RATE_HIFI &&
+	return sw->tmu.rate == sw->tmu.rate_request &&
 	       sw->tmu.unidirectional == unidirectional;
 }
 
@@ -975,6 +975,18 @@ static inline bool tb_switch_is_cl0s_enabled(const struct tb_switch *sw)
 	return sw->clx == TB_CL0S;
 }
 
+/**
+ * tb_switch_is_cl1_enabled() - Checks if the CL1 is enabled
+ * @sw: Router to check for the CL1
+ *
+ * Checks if the CL1 is enabled on the router upstream link.
+ * Not applicable for a host router.
+ */
+static inline bool tb_switch_is_cl1_enabled(const struct tb_switch *sw)
+{
+	return sw->clx == TB_CL1;
+}
+
 /**
  * tb_switch_is_clx_supported() - Is CLx supported on this type of router
  * @sw: The router to check CLx support for
diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
index b301eeb0c89b..f88dd27e4f62 100644
--- a/drivers/thunderbolt/tb_regs.h
+++ b/drivers/thunderbolt/tb_regs.h
@@ -234,6 +234,8 @@ enum usb4_switch_op {
 
 /* Router TMU configuration */
 #define TMU_RTR_CS_0				0x00
+#define TMU_RTR_CS_0_FREQ_WIND_MASK		GENMASK(26, 16)
+#define TMU_RTR_CS_0_FREQ_WIND_SHIFT		16
 #define TMU_RTR_CS_0_TD				BIT(27)
 #define TMU_RTR_CS_0_UCAP			BIT(30)
 #define TMU_RTR_CS_1				0x01
@@ -244,6 +246,12 @@ enum usb4_switch_op {
 #define TMU_RTR_CS_3_LOCAL_TIME_NS_MASK		GENMASK(15, 0)
 #define TMU_RTR_CS_3_TS_PACKET_INTERVAL_MASK	GENMASK(31, 16)
 #define TMU_RTR_CS_3_TS_PACKET_INTERVAL_SHIFT	16
+#define TMU_RTR_CS_15				0xf
+#define TMU_RTR_CS_15_AVG_MASK			GENMASK(5, 0)
+#define TMU_RTR_CS_15_FREQ_AVG_SHIFT		0
+#define TMU_RTR_CS_15_DELAY_AVG_SHIFT		6
+#define TMU_RTR_CS_15_OFFSET_AVG_SHIFT		12
+#define TMU_RTR_CS_15_ERROR_AVG_SHIFT		18
 #define TMU_RTR_CS_22				0x16
 #define TMU_RTR_CS_24				0x18
 #define TMU_RTR_CS_25				0x19
diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c
index 985ca43b8f39..a03c57a8728c 100644
--- a/drivers/thunderbolt/tmu.c
+++ b/drivers/thunderbolt/tmu.c
@@ -11,6 +11,55 @@
 
 #include "tb.h"
 
+static int tb_switch_set_tmu_mode_params(struct tb_switch *sw,
+					 enum tb_switch_tmu_rate rate)
+{
+	u32 freq_meas_wind[2] = { 30, 800 };
+	u32 avg_const[2] = { 4, 8 };
+	u32 freq, avg, val;
+	int ret;
+
+	if (rate == TB_SWITCH_TMU_RATE_NORMAL) {
+		freq = freq_meas_wind[0];
+		avg = avg_const[0];
+	} else if (rate == TB_SWITCH_TMU_RATE_HIFI) {
+		freq = freq_meas_wind[1];
+		avg = avg_const[1];
+	} else {
+		return 0;
+	}
+
+	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH,
+			 sw->tmu.cap + TMU_RTR_CS_0, 1);
+	if (ret)
+		return ret;
+
+	val &= ~TMU_RTR_CS_0_FREQ_WIND_MASK;
+	val |= freq << TMU_RTR_CS_0_FREQ_WIND_SHIFT;
+
+	ret = tb_sw_write(sw, &val, TB_CFG_SWITCH,
+			  sw->tmu.cap + TMU_RTR_CS_0, 1);
+	if (ret)
+		return ret;
+
+	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH,
+			 sw->tmu.cap + TMU_RTR_CS_15, 1);
+	if (ret)
+		return ret;
+
+	val &= ~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_FREQ_AVG_SHIFT &
+		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_DELAY_AVG_SHIFT &
+		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_OFFSET_AVG_SHIFT &
+		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_ERROR_AVG_SHIFT;
+	val |= avg << TMU_RTR_CS_15_FREQ_AVG_SHIFT |
+		avg << TMU_RTR_CS_15_DELAY_AVG_SHIFT |
+		avg << TMU_RTR_CS_15_OFFSET_AVG_SHIFT |
+		avg << TMU_RTR_CS_15_ERROR_AVG_SHIFT;
+
+	return tb_sw_write(sw, &val, TB_CFG_SWITCH,
+			   sw->tmu.cap + TMU_RTR_CS_15, 1);
+}
+
 static const char *tb_switch_tmu_mode_name(const struct tb_switch *sw)
 {
 	bool root_switch = !tb_route(sw);
@@ -348,7 +397,7 @@ int tb_switch_tmu_disable(struct tb_switch *sw)
 
 
 	if (tb_route(sw)) {
-		bool unidirectional = tb_switch_tmu_hifi_is_enabled(sw, true);
+		bool unidirectional = sw->tmu.unidirectional;
 		struct tb_switch *parent = tb_switch_parent(sw);
 		struct tb_port *down, *up;
 		int ret;
@@ -412,6 +461,7 @@ static void __tb_switch_tmu_off(struct tb_switch *sw, bool unidirectional)
 	else
 		tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_OFF);
 
+	tb_switch_set_tmu_mode_params(sw, sw->tmu.rate);
 	tb_port_tmu_unidirectional_disable(down);
 	tb_port_tmu_unidirectional_disable(up);
 }
@@ -493,7 +543,11 @@ static int __tb_switch_tmu_enable_unidirectional(struct tb_switch *sw)
 
 	up = tb_upstream_port(sw);
 	down = tb_port_at(tb_route(sw), parent);
-	ret = tb_switch_tmu_rate_write(parent, TB_SWITCH_TMU_RATE_HIFI);
+	ret = tb_switch_tmu_rate_write(parent, sw->tmu.rate_request);
+	if (ret)
+		return ret;
+
+	ret = tb_switch_set_tmu_mode_params(sw, sw->tmu.rate_request);
 	if (ret)
 		return ret;
 
@@ -520,7 +574,85 @@ static int __tb_switch_tmu_enable_unidirectional(struct tb_switch *sw)
 	return ret;
 }
 
-static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
+static void __tb_switch_tmu_change_mode_prev(struct tb_switch *sw)
+{
+	struct tb_switch *parent = tb_switch_parent(sw);
+	struct tb_port *down, *up;
+
+	down = tb_port_at(tb_route(sw), parent);
+	up = tb_upstream_port(sw);
+	/*
+	 * In case of any failure in one of the steps when change mode,
+	 * get back to the TMU configurations in previous mode.
+	 * In case of additional failures in the functions below,
+	 * ignore them since the caller shall already report a failure.
+	 */
+	tb_port_tmu_set_unidirectional(down, sw->tmu.unidirectional);
+	if (sw->tmu.unidirectional_request)
+		tb_switch_tmu_rate_write(parent, sw->tmu.rate);
+	else
+		tb_switch_tmu_rate_write(sw, sw->tmu.rate);
+
+	tb_switch_set_tmu_mode_params(sw, sw->tmu.rate);
+	tb_port_tmu_set_unidirectional(up, sw->tmu.unidirectional);
+}
+
+static int __tb_switch_tmu_change_mode(struct tb_switch *sw)
+{
+	struct tb_switch *parent = tb_switch_parent(sw);
+	struct tb_port *up, *down;
+	int ret;
+
+	up = tb_upstream_port(sw);
+	down = tb_port_at(tb_route(sw), parent);
+	ret = tb_port_tmu_set_unidirectional(down,
+					     sw->tmu.unidirectional_request);
+	if (ret)
+		goto out;
+
+	if (sw->tmu.unidirectional_request)
+		ret = tb_switch_tmu_rate_write(parent, sw->tmu.rate_request);
+	else
+		ret = tb_switch_tmu_rate_write(sw, sw->tmu.rate_request);
+	if (ret)
+		return ret;
+
+	ret = tb_switch_set_tmu_mode_params(sw, sw->tmu.rate_request);
+	if (ret)
+		return ret;
+
+	ret = tb_port_tmu_set_unidirectional(up,
+					     sw->tmu.unidirectional_request);
+	if (ret)
+		goto out;
+
+	ret = tb_port_tmu_time_sync_enable(down);
+	if (ret)
+		goto out;
+
+	ret = tb_port_tmu_time_sync_enable(up);
+	if (ret)
+		goto out;
+
+	return 0;
+
+out:
+	__tb_switch_tmu_change_mode_prev(sw);
+	return ret;
+}
+
+/**
+ * tb_switch_tmu_enable() - Enable TMU on a router
+ * @sw: Router whose TMU to enable
+ *
+ * Enables TMU of a router to be in uni-directional Normal/Hifi
+ * or bi-directional HiFi mode. Calling tb_switch_tmu_configure() is required
+ * before calling this function, to select the mode Normal/HiFi and
+ * directionality (uni-directional/bi-directional).
+ * In HiFi mode all tunneling should work. In Normal mode, DP tunneling can't
+ * work. Uni-directional mode is required for CLx (Link Low-Power) to work.
+ */
+int tb_switch_tmu_enable(struct tb_switch *sw)
 {
 	bool unidirectional = sw->tmu.unidirectional_request;
 	int ret;
@@ -536,12 +668,13 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
 	if (!tb_switch_is_clx_supported(sw))
 		return 0;
 
-	if (tb_switch_tmu_hifi_is_enabled(sw, sw->tmu.unidirectional_request))
+	if (tb_switch_tmu_is_enabled(sw, sw->tmu.unidirectional_request))
 		return 0;
 
 	if (tb_switch_is_titan_ridge(sw) && unidirectional) {
-		/* Titan Ridge supports only CL0s */
-		if (!tb_switch_is_cl0s_enabled(sw))
+		/* Titan Ridge supports CL0s and CL1 only */
+		if (!tb_switch_is_cl0s_enabled(sw) &&
+		    !tb_switch_is_cl1_enabled(sw))
 			return -EOPNOTSUPP;
 
 		ret = tb_switch_tmu_objection_mask(sw);
@@ -558,7 +691,11 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
 		return ret;
 
 	if (tb_route(sw)) {
-		/* The used mode changes are from OFF to HiFi-Uni/HiFi-BiDir */
+		/*
+		 * The used mode changes are from OFF to
+		 * HiFi-Uni/HiFi-BiDir/Normal-Uni or from Normal-Uni to
+		 * Hifi-Uni.
+		 */
 		if (sw->tmu.rate == TB_SWITCH_TMU_RATE_OFF) {
 			if (unidirectional)
 				ret = __tb_switch_tmu_enable_unidirectional(sw);
@@ -566,6 +703,10 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
 				ret = __tb_switch_tmu_enable_bidirectional(sw);
 			if (ret)
 				return ret;
+		} else if (sw->tmu.rate == TB_SWITCH_TMU_RATE_NORMAL) {
+			ret = __tb_switch_tmu_change_mode(sw);
+			if (ret)
+				return ret;
 		}
 		sw->tmu.unidirectional = unidirectional;
 	} else {
@@ -575,35 +716,17 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
 		 * of the child node - see above.
 		 * Here only the host router' rate configuration is written.
 		 */
-		ret = tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_HIFI);
+		ret = tb_switch_tmu_rate_write(sw, sw->tmu.rate_request);
 		if (ret)
 			return ret;
 	}
 
-	sw->tmu.rate = TB_SWITCH_TMU_RATE_HIFI;
+	sw->tmu.rate = sw->tmu.rate_request;
 
 	tb_sw_dbg(sw, "TMU: mode set to: %s\n", tb_switch_tmu_mode_name(sw));
 	return tb_switch_tmu_set_time_disruption(sw, false);
 }
 
-/**
- * tb_switch_tmu_enable() - Enable TMU on a router
- * @sw: Router whose TMU to enable
- *
- * Enables TMU of a router to be in uni-directional or bi-directional HiFi mode.
- * Calling tb_switch_tmu_configure() is required before calling this function,
- * to select the mode HiFi and directionality (uni-directional/bi-directional).
- * In both modes all tunneling should work. Uni-directional mode is required for
- * CLx (Link Low-Power) to work.
- */
-int tb_switch_tmu_enable(struct tb_switch *sw)
-{
-	if (sw->tmu.rate_request == TB_SWITCH_TMU_RATE_NORMAL)
-		return -EOPNOTSUPP;
-
-	return tb_switch_tmu_hifi_enable(sw);
-}
-
 /**
  * tb_switch_tmu_configure() - Configure the TMU rate and directionality
  * @sw: Router whose mode to change
-- 
2.17.1

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

Re: [PATCH 4/5] thunderbolt: Add CL1 support for USB4 and Titan Ridge routers

[Mika Westerberg]

Hi Gil,

On Sun, May 01, 2022 at 11:33:20PM +0300, Gil Fine wrote:
> In this patch we add support for a second low power state of the link: CL1.
> Low power states (called collectively CLx) are used to reduce
> transmitter and receiver power when a high-speed lane is idle.
> We enable it, if both sides of the link support it,
> and only for the first hop router (i.e. the first device that connected
> to the host router). This is needed for better thermal management.
> 
> Signed-off-by: Gil Fine <gil.fine@intel.com>
> ---
>  drivers/thunderbolt/switch.c  |  79 ++++++++-------
>  drivers/thunderbolt/tb.c      |  24 +++--
>  drivers/thunderbolt/tb.h      |  22 ++++-
>  drivers/thunderbolt/tb_regs.h |   8 ++
>  drivers/thunderbolt/tmu.c     | 177 ++++++++++++++++++++++++++++------
>  5 files changed, 237 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 42b7daaf9c4d..4288cb5550f8 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -3064,7 +3064,7 @@ void tb_switch_suspend(struct tb_switch *sw, bool runtime)
>  	 * done for USB4 device too as CLx is re-enabled at resume.
>  	 */
>  	if (tb_switch_is_clx_enabled(sw)) {
> -		if (tb_switch_disable_clx(sw, TB_CL0S))
> +		if (tb_switch_disable_clx(sw, TB_CL1))

Hm, what if the router only supports CL0s? Will this still enable CL0s
then?

>  			tb_sw_warn(sw, "failed to disable CLx on upstream port\n");
>  	}
>  
> @@ -3358,12 +3358,12 @@ static bool tb_port_clx_supported(struct tb_port *port, enum tb_clx clx)
>  
>  	switch (clx) {
>  	case TB_CL0S:
> +	case TB_CL1:
>  		/* CL0s support requires also CL1 support */
>  		mask = LANE_ADP_CS_0_CL0S_SUPPORT | LANE_ADP_CS_0_CL1_SUPPORT;
>  		break;
>  
> -	/* For now we support only CL0s. Not CL1, CL2 */
> -	case TB_CL1:
> +	/* For now we support only CL0s and CL1. Not CL2 */
>  	case TB_CL2:
>  	default:
>  		return false;
> @@ -3377,12 +3377,7 @@ static bool tb_port_clx_supported(struct tb_port *port, enum tb_clx clx)
>  	return !!(val & mask);
>  }
>  
> -static inline bool tb_port_cl0s_supported(struct tb_port *port)
> -{
> -	return tb_port_clx_supported(port, TB_CL0S);
> -}
> -
> -static int __tb_port_cl0s_set(struct tb_port *port, bool enable)
> +static int __tb_port_cl0s_cl1_set(struct tb_port *port, bool enable)
>  {
>  	u32 phy, mask;
>  	int ret;
> @@ -3403,20 +3398,32 @@ static int __tb_port_cl0s_set(struct tb_port *port, bool enable)
>  			     port->cap_phy + LANE_ADP_CS_1, 1);
>  }
>  
> -static int tb_port_cl0s_disable(struct tb_port *port)
> +static int tb_port_cl0s_cl1_disable(struct tb_port *port)
> +{
> +	return __tb_port_cl0s_cl1_set(port, false);
> +}
> +
> +static int tb_port_cl0s_cl1_enable(struct tb_port *port)
>  {
> -	return __tb_port_cl0s_set(port, false);
> +	return __tb_port_cl0s_cl1_set(port, true);
>  }
>  
> -static int tb_port_cl0s_enable(struct tb_port *port)
> +static const char *tb_switch_clx_name(enum tb_clx clx)
>  {
> -	return __tb_port_cl0s_set(port, true);
> +	switch (clx) {
> +	case TB_CL0S:
> +		return "CL0s";
> +	case TB_CL1:
> +		return "CL1";
> +	default:
> +		return "Unknown";
> +	}
>  }
>  
> -static int tb_switch_enable_cl0s(struct tb_switch *sw)
> +static int __tb_switch_enable_clx(struct tb_switch *sw, enum tb_clx clx)
>  {
>  	struct tb_switch *parent = tb_switch_parent(sw);
> -	bool up_cl0s_support, down_cl0s_support;
> +	bool up_clx_support, down_clx_support;
>  	struct tb_port *up, *down;
>  	int ret;
>  
> @@ -3441,37 +3448,37 @@ static int tb_switch_enable_cl0s(struct tb_switch *sw)
>  	up = tb_upstream_port(sw);
>  	down = tb_port_at(tb_route(sw), parent);
>  
> -	up_cl0s_support = tb_port_cl0s_supported(up);
> -	down_cl0s_support = tb_port_cl0s_supported(down);
> +	up_clx_support = tb_port_clx_supported(up, clx);
> +	down_clx_support = tb_port_clx_supported(down, clx);
>  
> -	tb_port_dbg(up, "CL0s %ssupported\n",
> -		    up_cl0s_support ? "" : "not ");
> -	tb_port_dbg(down, "CL0s %ssupported\n",
> -		    down_cl0s_support ? "" : "not ");
> +	tb_port_dbg(up, "%s %ssupported\n", tb_switch_clx_name(clx),
> +		    up_clx_support ? "" : "not ");
> +	tb_port_dbg(down, "%s %ssupported\n", tb_switch_clx_name(clx),
> +		    down_clx_support ? "" : "not ");
>  
> -	if (!up_cl0s_support || !down_cl0s_support)
> +	if (!up_clx_support || !down_clx_support)
>  		return -EOPNOTSUPP;
>  
> -	ret = tb_port_cl0s_enable(up);
> +	ret = tb_port_cl0s_cl1_enable(up);
>  	if (ret)
>  		return ret;
>  
> -	ret = tb_port_cl0s_enable(down);
> +	ret = tb_port_cl0s_cl1_enable(down);
>  	if (ret) {
> -		tb_port_cl0s_disable(up);
> +		tb_port_cl0s_cl1_disable(up);
>  		return ret;
>  	}
>  
>  	ret = tb_switch_mask_clx_objections(sw);
>  	if (ret) {
> -		tb_port_cl0s_disable(up);
> -		tb_port_cl0s_disable(down);
> +		tb_port_cl0s_cl1_disable(up);
> +		tb_port_cl0s_cl1_disable(down);
>  		return ret;
>  	}
>  
> -	sw->clx = TB_CL0S;
> +	sw->clx = clx;
>  
> -	tb_port_dbg(up, "CL0s enabled\n");
> +	tb_port_dbg(up, "%s enabled\n", tb_switch_clx_name(clx));
>  	return 0;
>  }
>  
> @@ -3505,14 +3512,15 @@ int tb_switch_enable_clx(struct tb_switch *sw, enum tb_clx clx)
>  
>  	switch (clx) {
>  	case TB_CL0S:
> -		return tb_switch_enable_cl0s(sw);
> +	case TB_CL1:
> +		return __tb_switch_enable_clx(sw, clx);
>  
>  	default:
>  		return -EOPNOTSUPP;
>  	}
>  }
>  
> -static int tb_switch_disable_cl0s(struct tb_switch *sw)
> +static int tb_switch_disable_cl0s_cl1(struct tb_switch *sw)
>  {
>  	struct tb_switch *parent = tb_switch_parent(sw);
>  	struct tb_port *up, *down;
> @@ -3534,17 +3542,17 @@ static int tb_switch_disable_cl0s(struct tb_switch *sw)
>  
>  	up = tb_upstream_port(sw);
>  	down = tb_port_at(tb_route(sw), parent);
> -	ret = tb_port_cl0s_disable(up);
> +	ret = tb_port_cl0s_cl1_disable(up);
>  	if (ret)
>  		return ret;
>  
> -	ret = tb_port_cl0s_disable(down);
> +	ret = tb_port_cl0s_cl1_disable(down);
>  	if (ret)
>  		return ret;
>  
>  	sw->clx = TB_CLX_DISABLE;
>  
> -	tb_port_dbg(up, "CL0s disabled\n");
> +	tb_port_dbg(up, "CL0s, CL1 disabled\n");
>  	return 0;
>  }
>  
> @@ -3562,7 +3570,8 @@ int tb_switch_disable_clx(struct tb_switch *sw, enum tb_clx clx)
>  
>  	switch (clx) {
>  	case TB_CL0S:
> -		return tb_switch_disable_cl0s(sw);
> +	case TB_CL1:
> +		return tb_switch_disable_cl0s_cl1(sw);
>  
>  	default:
>  		return -EOPNOTSUPP;
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 7419cd1aefba..05a084e3e9f6 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -221,7 +221,7 @@ static int tb_enable_tmu(struct tb_switch *sw)
>  	int ret;
>  
>  	/* If it is already enabled in correct mode, don't touch it */
> -	if (tb_switch_tmu_hifi_is_enabled(sw, sw->tmu.unidirectional_request))
> +	if (tb_switch_tmu_is_enabled(sw, sw->tmu.unidirectional_request))
>  		return 0;
>  
>  	ret = tb_switch_tmu_disable(sw);
> @@ -671,12 +671,19 @@ static void tb_scan_port(struct tb_port *port)
>  	/* Set the link configured */
>  	tb_switch_configure_link(sw);
>  	/* Silently ignore CLx enabling in case CLx is not supported */
> -	ret = tb_switch_enable_clx(sw, TB_CL0S);
> +	ret = tb_switch_enable_clx(sw, TB_CL1);
>  	if (ret && ret != -EOPNOTSUPP)
>  		tb_sw_warn(sw, "failed to enable CLx on upstream port\n");
>  
> -	tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI,
> -				tb_switch_is_clx_enabled(sw));
> +	if (tb_switch_is_clx_enabled(sw))
> +		/*
> +		 * To support highest CLx state, we set host router's TMU to
> +		 * Normal-Uni mode.
> +		 */
> +		tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_NORMAL, true);
> +	else
> +		/* If CLx disabled, configure router's TMU to HiFi-Bidir mode*/
> +		tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI, false);
>  
>  	if (tb_enable_tmu(sw))
>  		tb_sw_warn(sw, "failed to enable TMU\n");
> @@ -1416,7 +1423,12 @@ static int tb_start(struct tb *tb)
>  		return ret;
>  	}
>  
> -	tb_switch_tmu_configure(tb->root_switch, TB_SWITCH_TMU_RATE_HIFI, false);
> +	/*
> +	 * To support highest CLx state, we set host router's TMU to
> +	 * Normal mode.
> +	 */
> +	tb_switch_tmu_configure(tb->root_switch, TB_SWITCH_TMU_RATE_NORMAL,
> +				false);
>  	/* Enable TMU if it is off */
>  	tb_switch_tmu_enable(tb->root_switch);
>  	/* Full scan to discover devices added before the driver was loaded. */
> @@ -1462,7 +1474,7 @@ static void tb_restore_children(struct tb_switch *sw)
>  		return;
>  
>  	/* Silently ignore CLx re-enabling in case CLx is not supported */
> -	ret = tb_switch_enable_clx(sw, TB_CL0S);
> +	ret = tb_switch_enable_clx(sw, TB_CL1);
>  	if (ret && ret != -EOPNOTSUPP)
>  		tb_sw_warn(sw, "failed to re-enable CLx on upstream port\n");
>  
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index ad025ff142ba..6a7204cf67f1 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -934,17 +934,17 @@ void tb_switch_tmu_configure(struct tb_switch *sw,
>  			     enum tb_switch_tmu_rate rate,
>  			     bool unidirectional);
>  /**
> - * tb_switch_tmu_hifi_is_enabled() - Checks if the specified TMU mode is enabled
> + * tb_switch_tmu_is_enabled() - Checks if the specified TMU mode is enabled
>   * @sw: Router whose TMU mode to check
>   * @unidirectional: If uni-directional (bi-directional otherwise)
>   *
>   * Return true if hardware TMU configuration matches the one passed in
> - * as parameter. That is HiFi and either uni-directional or bi-directional.
> + * as parameter. That is HiFi/Normal and either uni-directional or bi-directional.
>   */
> -static inline bool tb_switch_tmu_hifi_is_enabled(const struct tb_switch *sw,
> -						 bool unidirectional)
> +static inline bool tb_switch_tmu_is_enabled(const struct tb_switch *sw,
> +					    bool unidirectional)
>  {
> -	return sw->tmu.rate == TB_SWITCH_TMU_RATE_HIFI &&
> +	return sw->tmu.rate == sw->tmu.rate_request &&
>  	       sw->tmu.unidirectional == unidirectional;
>  }
>  
> @@ -975,6 +975,18 @@ static inline bool tb_switch_is_cl0s_enabled(const struct tb_switch *sw)
>  	return sw->clx == TB_CL0S;
>  }
>  
> +/**
> + * tb_switch_is_cl1_enabled() - Checks if the CL1 is enabled
> + * @sw: Router to check for the CL1
> + *
> + * Checks if the CL1 is enabled on the router upstream link.
> + * Not applicable for a host router.
> + */
> +static inline bool tb_switch_is_cl1_enabled(const struct tb_switch *sw)
> +{
> +	return sw->clx == TB_CL1;
> +}
> +
>  /**
>   * tb_switch_is_clx_supported() - Is CLx supported on this type of router
>   * @sw: The router to check CLx support for
> diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
> index b301eeb0c89b..f88dd27e4f62 100644
> --- a/drivers/thunderbolt/tb_regs.h
> +++ b/drivers/thunderbolt/tb_regs.h
> @@ -234,6 +234,8 @@ enum usb4_switch_op {
>  
>  /* Router TMU configuration */
>  #define TMU_RTR_CS_0				0x00
> +#define TMU_RTR_CS_0_FREQ_WIND_MASK		GENMASK(26, 16)
> +#define TMU_RTR_CS_0_FREQ_WIND_SHIFT		16
>  #define TMU_RTR_CS_0_TD				BIT(27)
>  #define TMU_RTR_CS_0_UCAP			BIT(30)
>  #define TMU_RTR_CS_1				0x01
> @@ -244,6 +246,12 @@ enum usb4_switch_op {
>  #define TMU_RTR_CS_3_LOCAL_TIME_NS_MASK		GENMASK(15, 0)
>  #define TMU_RTR_CS_3_TS_PACKET_INTERVAL_MASK	GENMASK(31, 16)
>  #define TMU_RTR_CS_3_TS_PACKET_INTERVAL_SHIFT	16
> +#define TMU_RTR_CS_15				0xf
> +#define TMU_RTR_CS_15_AVG_MASK			GENMASK(5, 0)
> +#define TMU_RTR_CS_15_FREQ_AVG_SHIFT		0
> +#define TMU_RTR_CS_15_DELAY_AVG_SHIFT		6
> +#define TMU_RTR_CS_15_OFFSET_AVG_SHIFT		12
> +#define TMU_RTR_CS_15_ERROR_AVG_SHIFT		18
>  #define TMU_RTR_CS_22				0x16
>  #define TMU_RTR_CS_24				0x18
>  #define TMU_RTR_CS_25				0x19
> diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c
> index 985ca43b8f39..a03c57a8728c 100644
> --- a/drivers/thunderbolt/tmu.c
> +++ b/drivers/thunderbolt/tmu.c
> @@ -11,6 +11,55 @@
>  
>  #include "tb.h"
>  
> +static int tb_switch_set_tmu_mode_params(struct tb_switch *sw,
> +					 enum tb_switch_tmu_rate rate)
> +{
> +	u32 freq_meas_wind[2] = { 30, 800 };
> +	u32 avg_const[2] = { 4, 8 };
> +	u32 freq, avg, val;
> +	int ret;
> +
> +	if (rate == TB_SWITCH_TMU_RATE_NORMAL) {
> +		freq = freq_meas_wind[0];
> +		avg = avg_const[0];
> +	} else if (rate == TB_SWITCH_TMU_RATE_HIFI) {
> +		freq = freq_meas_wind[1];
> +		avg = avg_const[1];
> +	} else {
> +		return 0;
> +	}
> +
> +	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH,
> +			 sw->tmu.cap + TMU_RTR_CS_0, 1);
> +	if (ret)
> +		return ret;
> +
> +	val &= ~TMU_RTR_CS_0_FREQ_WIND_MASK;
> +	val |= freq << TMU_RTR_CS_0_FREQ_WIND_SHIFT;
> +
> +	ret = tb_sw_write(sw, &val, TB_CFG_SWITCH,
> +			  sw->tmu.cap + TMU_RTR_CS_0, 1);
> +	if (ret)
> +		return ret;
> +
> +	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH,
> +			 sw->tmu.cap + TMU_RTR_CS_15, 1);
> +	if (ret)
> +		return ret;
> +
> +	val &= ~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_FREQ_AVG_SHIFT &
> +		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_DELAY_AVG_SHIFT &
> +		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_OFFSET_AVG_SHIFT &
> +		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_ERROR_AVG_SHIFT;
> +	val |= avg << TMU_RTR_CS_15_FREQ_AVG_SHIFT |
> +		avg << TMU_RTR_CS_15_DELAY_AVG_SHIFT |
> +		avg << TMU_RTR_CS_15_OFFSET_AVG_SHIFT |
> +		avg << TMU_RTR_CS_15_ERROR_AVG_SHIFT;

Perhaps use FIELD_x macros from include/linux/bitfield.h?

> +
> +	return tb_sw_write(sw, &val, TB_CFG_SWITCH,
> +			   sw->tmu.cap + TMU_RTR_CS_15, 1);
> +}
> +
>  static const char *tb_switch_tmu_mode_name(const struct tb_switch *sw)
>  {
>  	bool root_switch = !tb_route(sw);
> @@ -348,7 +397,7 @@ int tb_switch_tmu_disable(struct tb_switch *sw)
>  
>  
>  	if (tb_route(sw)) {
> -		bool unidirectional = tb_switch_tmu_hifi_is_enabled(sw, true);
> +		bool unidirectional = sw->tmu.unidirectional;
>  		struct tb_switch *parent = tb_switch_parent(sw);
>  		struct tb_port *down, *up;
>  		int ret;
> @@ -412,6 +461,7 @@ static void __tb_switch_tmu_off(struct tb_switch *sw, bool unidirectional)
>  	else
>  		tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_OFF);
>  
> +	tb_switch_set_tmu_mode_params(sw, sw->tmu.rate);
>  	tb_port_tmu_unidirectional_disable(down);
>  	tb_port_tmu_unidirectional_disable(up);
>  }
> @@ -493,7 +543,11 @@ static int __tb_switch_tmu_enable_unidirectional(struct tb_switch *sw)
>  
>  	up = tb_upstream_port(sw);
>  	down = tb_port_at(tb_route(sw), parent);
> -	ret = tb_switch_tmu_rate_write(parent, TB_SWITCH_TMU_RATE_HIFI);
> +	ret = tb_switch_tmu_rate_write(parent, sw->tmu.rate_request);
> +	if (ret)
> +		return ret;
> +
> +	ret = tb_switch_set_tmu_mode_params(sw, sw->tmu.rate_request);
>  	if (ret)
>  		return ret;
>  
> @@ -520,7 +574,85 @@ static int __tb_switch_tmu_enable_unidirectional(struct tb_switch *sw)
>  	return ret;
>  }
>  
> -static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
> +static void __tb_switch_tmu_change_mode_prev(struct tb_switch *sw)
> +{
> +	struct tb_switch *parent = tb_switch_parent(sw);
> +	struct tb_port *down, *up;
> +
> +	down = tb_port_at(tb_route(sw), parent);
> +	up = tb_upstream_port(sw);
> +	/*
> +	 * In case of any failure in one of the steps when change mode,
> +	 * get back to the TMU configurations in previous mode.
> +	 * In case of additional failures in the functions below,
> +	 * ignore them since the caller shall already report a failure.
> +	 */
> +	tb_port_tmu_set_unidirectional(down, sw->tmu.unidirectional);
> +	if (sw->tmu.unidirectional_request)
> +		tb_switch_tmu_rate_write(parent, sw->tmu.rate);
> +	else
> +		tb_switch_tmu_rate_write(sw, sw->tmu.rate);
> +
> +	tb_switch_set_tmu_mode_params(sw, sw->tmu.rate);
> +	tb_port_tmu_set_unidirectional(up, sw->tmu.unidirectional);
> +}
> +
> +static int __tb_switch_tmu_change_mode(struct tb_switch *sw)
> +{
> +	struct tb_switch *parent = tb_switch_parent(sw);
> +	struct tb_port *up, *down;
> +	int ret;
> +
> +	up = tb_upstream_port(sw);
> +	down = tb_port_at(tb_route(sw), parent);
> +	ret = tb_port_tmu_set_unidirectional(down,
> +					     sw->tmu.unidirectional_request);

This looks better if you put them single line:

	ret = tb_port_tmu_set_unidirectional(down, sw->tmu.unidirectional_request);

> +	if (ret)
> +		goto out;
> +
> +	if (sw->tmu.unidirectional_request)
> +		ret = tb_switch_tmu_rate_write(parent, sw->tmu.rate_request);
> +	else
> +		ret = tb_switch_tmu_rate_write(sw, sw->tmu.rate_request);
> +	if (ret)
> +		return ret;
> +
> +	ret = tb_switch_set_tmu_mode_params(sw, sw->tmu.rate_request);
> +	if (ret)
> +		return ret;
> +
> +	ret = tb_port_tmu_set_unidirectional(up,
> +					     sw->tmu.unidirectional_request);

Ditto here.

> +	if (ret)
> +		goto out;
> +
> +	ret = tb_port_tmu_time_sync_enable(down);
> +	if (ret)
> +		goto out;
> +
> +	ret = tb_port_tmu_time_sync_enable(up);
> +	if (ret)
> +		goto out;
> +
> +	return 0;
> +
> +out:
> +	__tb_switch_tmu_change_mode_prev(sw);
> +	return ret;
> +}
> +
> +/**
> + * tb_switch_tmu_enable() - Enable TMU on a router
> + * @sw: Router whose TMU to enable
> + *
> + * Enables TMU of a router to be in uni-directional Normal/Hifi
> + * or bi-directional HiFi mode. Calling tb_switch_tmu_configure() is required
> + * before calling this function, to select the mode Normal/HiFi and
> + * directionality (uni-directional/bi-directional).
> + * In HiFi mode all tunneling should work. In Normal mode, DP tunneling can't
> + * work. Uni-directional mode is required for CLx (Link Low-Power) to work.
> + */
> +int tb_switch_tmu_enable(struct tb_switch *sw)
>  {
>  	bool unidirectional = sw->tmu.unidirectional_request;
>  	int ret;
> @@ -536,12 +668,13 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
>  	if (!tb_switch_is_clx_supported(sw))
>  		return 0;
>  
> -	if (tb_switch_tmu_hifi_is_enabled(sw, sw->tmu.unidirectional_request))
> +	if (tb_switch_tmu_is_enabled(sw, sw->tmu.unidirectional_request))
>  		return 0;
>  
>  	if (tb_switch_is_titan_ridge(sw) && unidirectional) {
> -		/* Titan Ridge supports only CL0s */
> -		if (!tb_switch_is_cl0s_enabled(sw))
> +		/* Titan Ridge supports CL0s and CL1 only */
> +		if (!tb_switch_is_cl0s_enabled(sw) &&
> +		    !tb_switch_is_cl1_enabled(sw))

I wonder if we can have function like tb_switch_clx_is_enabled() that
takes bitmask:

	if (!tb_switch_clx_is_enabled(sw, TB_CL0S | TB_CL1))
		...

>  			return -EOPNOTSUPP;
>  
>  		ret = tb_switch_tmu_objection_mask(sw);
> @@ -558,7 +691,11 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
>  		return ret;
>  
>  	if (tb_route(sw)) {
> -		/* The used mode changes are from OFF to HiFi-Uni/HiFi-BiDir */
> +		/*
> +		 * The used mode changes are from OFF to
> +		 * HiFi-Uni/HiFi-BiDir/Normal-Uni or from Normal-Uni to
> +		 * Hifi-Uni.
> +		 */
>  		if (sw->tmu.rate == TB_SWITCH_TMU_RATE_OFF) {
>  			if (unidirectional)
>  				ret = __tb_switch_tmu_enable_unidirectional(sw);
> @@ -566,6 +703,10 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
>  				ret = __tb_switch_tmu_enable_bidirectional(sw);
>  			if (ret)
>  				return ret;
> +		} else if (sw->tmu.rate == TB_SWITCH_TMU_RATE_NORMAL) {
> +			ret = __tb_switch_tmu_change_mode(sw);
> +			if (ret)
> +				return ret;
>  		}
>  		sw->tmu.unidirectional = unidirectional;
>  	} else {
> @@ -575,35 +716,17 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
>  		 * of the child node - see above.
>  		 * Here only the host router' rate configuration is written.
>  		 */
> -		ret = tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_HIFI);
> +		ret = tb_switch_tmu_rate_write(sw, sw->tmu.rate_request);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	sw->tmu.rate = TB_SWITCH_TMU_RATE_HIFI;
> +	sw->tmu.rate = sw->tmu.rate_request;
>  
>  	tb_sw_dbg(sw, "TMU: mode set to: %s\n", tb_switch_tmu_mode_name(sw));
>  	return tb_switch_tmu_set_time_disruption(sw, false);
>  }
>  
> -/**
> - * tb_switch_tmu_enable() - Enable TMU on a router
> - * @sw: Router whose TMU to enable
> - *
> - * Enables TMU of a router to be in uni-directional or bi-directional HiFi mode.
> - * Calling tb_switch_tmu_configure() is required before calling this function,
> - * to select the mode HiFi and directionality (uni-directional/bi-directional).
> - * In both modes all tunneling should work. Uni-directional mode is required for
> - * CLx (Link Low-Power) to work.
> - */
> -int tb_switch_tmu_enable(struct tb_switch *sw)
> -{
> -	if (sw->tmu.rate_request == TB_SWITCH_TMU_RATE_NORMAL)
> -		return -EOPNOTSUPP;
> -
> -	return tb_switch_tmu_hifi_enable(sw);
> -}
> -
>  /**
>   * tb_switch_tmu_configure() - Configure the TMU rate and directionality
>   * @sw: Router whose mode to change
> -- 
> 2.17.1

Re: [PATCH 4/5] thunderbolt: Add CL1 support for USB4 and Titan Ridge routers

[Gil Fine]

On Mon, May 02, 2022 at 01:04:11PM +0300, Mika Westerberg wrote:
> Hi Gil,
> 
> On Sun, May 01, 2022 at 11:33:20PM +0300, Gil Fine wrote:
> > In this patch we add support for a second low power state of the link: CL1.
> > Low power states (called collectively CLx) are used to reduce
> > transmitter and receiver power when a high-speed lane is idle.
> > We enable it, if both sides of the link support it,
> > and only for the first hop router (i.e. the first device that connected
> > to the host router). This is needed for better thermal management.
> > 
> > Signed-off-by: Gil Fine <gil.fine@intel.com>
> > ---
> >  drivers/thunderbolt/switch.c  |  79 ++++++++-------
> >  drivers/thunderbolt/tb.c      |  24 +++--
> >  drivers/thunderbolt/tb.h      |  22 ++++-
> >  drivers/thunderbolt/tb_regs.h |   8 ++
> >  drivers/thunderbolt/tmu.c     | 177 ++++++++++++++++++++++++++++------
> >  5 files changed, 237 insertions(+), 73 deletions(-)
> > 
> > diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> > index 42b7daaf9c4d..4288cb5550f8 100644
> > --- a/drivers/thunderbolt/switch.c
> > +++ b/drivers/thunderbolt/switch.c
> > @@ -3064,7 +3064,7 @@ void tb_switch_suspend(struct tb_switch *sw, bool runtime)
> >  	 * done for USB4 device too as CLx is re-enabled at resume.
> >  	 */
> >  	if (tb_switch_is_clx_enabled(sw)) {
> > -		if (tb_switch_disable_clx(sw, TB_CL0S))
> > +		if (tb_switch_disable_clx(sw, TB_CL1))
> 
> Hm, what if the router only supports CL0s? Will this still enable CL0s
> then?

Actually as I just understood, both can't be split and shall be enabled together.
Even if we only enable CL0s, we shall also enable CL1.
Also, the "CL0x/CL1 Support" bits set by HW together.
Seems as the spec is confusing on this...
Seems like we can merge both of them to be one ENUM and call it someting like
"TB_CL0s_CL1":
enum tb_clx {
	TB_CLX_DISABLE,
	TB_CL0s_CL1,
	TB_CL2,
}
To do it like this:

if (tb_switch_is_clx_enabled(sw, TB_CL0S_CL1)) {
	if (tb_switch_disable_clx(sw, TB_CL0S_CL1))
		tb_sw_warn(sw, "failed to disable %s on upstream port\n",
			   tb_switch_clx_name(TB_CL0S_CL1));
}

What do you think?

> 
> >  			tb_sw_warn(sw, "failed to disable CLx on upstream port\n");
> >  	}
> >  
> > @@ -3358,12 +3358,12 @@ static bool tb_port_clx_supported(struct tb_port *port, enum tb_clx clx)
> >  
> >  	switch (clx) {
> >  	case TB_CL0S:
> > +	case TB_CL1:
> >  		/* CL0s support requires also CL1 support */
> >  		mask = LANE_ADP_CS_0_CL0S_SUPPORT | LANE_ADP_CS_0_CL1_SUPPORT;
> >  		break;
> >  
> > -	/* For now we support only CL0s. Not CL1, CL2 */
> > -	case TB_CL1:
> > +	/* For now we support only CL0s and CL1. Not CL2 */
> >  	case TB_CL2:
> >  	default:
> >  		return false;
> > @@ -3377,12 +3377,7 @@ static bool tb_port_clx_supported(struct tb_port *port, enum tb_clx clx)
> >  	return !!(val & mask);
> >  }
> >  
> > -static inline bool tb_port_cl0s_supported(struct tb_port *port)
> > -{
> > -	return tb_port_clx_supported(port, TB_CL0S);
> > -}
> > -
> > -static int __tb_port_cl0s_set(struct tb_port *port, bool enable)
> > +static int __tb_port_cl0s_cl1_set(struct tb_port *port, bool enable)
> >  {
> >  	u32 phy, mask;
> >  	int ret;
> > @@ -3403,20 +3398,32 @@ static int __tb_port_cl0s_set(struct tb_port *port, bool enable)
> >  			     port->cap_phy + LANE_ADP_CS_1, 1);
> >  }
> >  
> > -static int tb_port_cl0s_disable(struct tb_port *port)
> > +static int tb_port_cl0s_cl1_disable(struct tb_port *port)
> > +{
> > +	return __tb_port_cl0s_cl1_set(port, false);
> > +}
> > +
> > +static int tb_port_cl0s_cl1_enable(struct tb_port *port)
> >  {
> > -	return __tb_port_cl0s_set(port, false);
> > +	return __tb_port_cl0s_cl1_set(port, true);
> >  }
> >  
> > -static int tb_port_cl0s_enable(struct tb_port *port)
> > +static const char *tb_switch_clx_name(enum tb_clx clx)
> >  {
> > -	return __tb_port_cl0s_set(port, true);
> > +	switch (clx) {
> > +	case TB_CL0S:
> > +		return "CL0s";
> > +	case TB_CL1:
> > +		return "CL1";
> > +	default:
> > +		return "Unknown";
> > +	}
> >  }
> >  
> > -static int tb_switch_enable_cl0s(struct tb_switch *sw)
> > +static int __tb_switch_enable_clx(struct tb_switch *sw, enum tb_clx clx)
> >  {
> >  	struct tb_switch *parent = tb_switch_parent(sw);
> > -	bool up_cl0s_support, down_cl0s_support;
> > +	bool up_clx_support, down_clx_support;
> >  	struct tb_port *up, *down;
> >  	int ret;
> >  
> > @@ -3441,37 +3448,37 @@ static int tb_switch_enable_cl0s(struct tb_switch *sw)
> >  	up = tb_upstream_port(sw);
> >  	down = tb_port_at(tb_route(sw), parent);
> >  
> > -	up_cl0s_support = tb_port_cl0s_supported(up);
> > -	down_cl0s_support = tb_port_cl0s_supported(down);
> > +	up_clx_support = tb_port_clx_supported(up, clx);
> > +	down_clx_support = tb_port_clx_supported(down, clx);
> >  
> > -	tb_port_dbg(up, "CL0s %ssupported\n",
> > -		    up_cl0s_support ? "" : "not ");
> > -	tb_port_dbg(down, "CL0s %ssupported\n",
> > -		    down_cl0s_support ? "" : "not ");
> > +	tb_port_dbg(up, "%s %ssupported\n", tb_switch_clx_name(clx),
> > +		    up_clx_support ? "" : "not ");
> > +	tb_port_dbg(down, "%s %ssupported\n", tb_switch_clx_name(clx),
> > +		    down_clx_support ? "" : "not ");
> >  
> > -	if (!up_cl0s_support || !down_cl0s_support)
> > +	if (!up_clx_support || !down_clx_support)
> >  		return -EOPNOTSUPP;
> >  
> > -	ret = tb_port_cl0s_enable(up);
> > +	ret = tb_port_cl0s_cl1_enable(up);
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = tb_port_cl0s_enable(down);
> > +	ret = tb_port_cl0s_cl1_enable(down);
> >  	if (ret) {
> > -		tb_port_cl0s_disable(up);
> > +		tb_port_cl0s_cl1_disable(up);
> >  		return ret;
> >  	}
> >  
> >  	ret = tb_switch_mask_clx_objections(sw);
> >  	if (ret) {
> > -		tb_port_cl0s_disable(up);
> > -		tb_port_cl0s_disable(down);
> > +		tb_port_cl0s_cl1_disable(up);
> > +		tb_port_cl0s_cl1_disable(down);
> >  		return ret;
> >  	}
> >  
> > -	sw->clx = TB_CL0S;
> > +	sw->clx = clx;
> >  
> > -	tb_port_dbg(up, "CL0s enabled\n");
> > +	tb_port_dbg(up, "%s enabled\n", tb_switch_clx_name(clx));
> >  	return 0;
> >  }
> >  
> > @@ -3505,14 +3512,15 @@ int tb_switch_enable_clx(struct tb_switch *sw, enum tb_clx clx)
> >  
> >  	switch (clx) {
> >  	case TB_CL0S:
> > -		return tb_switch_enable_cl0s(sw);
> > +	case TB_CL1:
> > +		return __tb_switch_enable_clx(sw, clx);
> >  
> >  	default:
> >  		return -EOPNOTSUPP;
> >  	}
> >  }
> >  
> > -static int tb_switch_disable_cl0s(struct tb_switch *sw)
> > +static int tb_switch_disable_cl0s_cl1(struct tb_switch *sw)
> >  {
> >  	struct tb_switch *parent = tb_switch_parent(sw);
> >  	struct tb_port *up, *down;
> > @@ -3534,17 +3542,17 @@ static int tb_switch_disable_cl0s(struct tb_switch *sw)
> >  
> >  	up = tb_upstream_port(sw);
> >  	down = tb_port_at(tb_route(sw), parent);
> > -	ret = tb_port_cl0s_disable(up);
> > +	ret = tb_port_cl0s_cl1_disable(up);
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = tb_port_cl0s_disable(down);
> > +	ret = tb_port_cl0s_cl1_disable(down);
> >  	if (ret)
> >  		return ret;
> >  
> >  	sw->clx = TB_CLX_DISABLE;
> >  
> > -	tb_port_dbg(up, "CL0s disabled\n");
> > +	tb_port_dbg(up, "CL0s, CL1 disabled\n");
> >  	return 0;
> >  }
> >  
> > @@ -3562,7 +3570,8 @@ int tb_switch_disable_clx(struct tb_switch *sw, enum tb_clx clx)
> >  
> >  	switch (clx) {
> >  	case TB_CL0S:
> > -		return tb_switch_disable_cl0s(sw);
> > +	case TB_CL1:
> > +		return tb_switch_disable_cl0s_cl1(sw);
> >  
> >  	default:
> >  		return -EOPNOTSUPP;
> > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> > index 7419cd1aefba..05a084e3e9f6 100644
> > --- a/drivers/thunderbolt/tb.c
> > +++ b/drivers/thunderbolt/tb.c
> > @@ -221,7 +221,7 @@ static int tb_enable_tmu(struct tb_switch *sw)
> >  	int ret;
> >  
> >  	/* If it is already enabled in correct mode, don't touch it */
> > -	if (tb_switch_tmu_hifi_is_enabled(sw, sw->tmu.unidirectional_request))
> > +	if (tb_switch_tmu_is_enabled(sw, sw->tmu.unidirectional_request))
> >  		return 0;
> >  
> >  	ret = tb_switch_tmu_disable(sw);
> > @@ -671,12 +671,19 @@ static void tb_scan_port(struct tb_port *port)
> >  	/* Set the link configured */
> >  	tb_switch_configure_link(sw);
> >  	/* Silently ignore CLx enabling in case CLx is not supported */
> > -	ret = tb_switch_enable_clx(sw, TB_CL0S);
> > +	ret = tb_switch_enable_clx(sw, TB_CL1);
> >  	if (ret && ret != -EOPNOTSUPP)
> >  		tb_sw_warn(sw, "failed to enable CLx on upstream port\n");
> >  
> > -	tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI,
> > -				tb_switch_is_clx_enabled(sw));
> > +	if (tb_switch_is_clx_enabled(sw))
> > +		/*
> > +		 * To support highest CLx state, we set host router's TMU to
> > +		 * Normal-Uni mode.
> > +		 */
> > +		tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_NORMAL, true);
> > +	else
> > +		/* If CLx disabled, configure router's TMU to HiFi-Bidir mode*/
> > +		tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI, false);
> >  
> >  	if (tb_enable_tmu(sw))
> >  		tb_sw_warn(sw, "failed to enable TMU\n");
> > @@ -1416,7 +1423,12 @@ static int tb_start(struct tb *tb)
> >  		return ret;
> >  	}
> >  
> > -	tb_switch_tmu_configure(tb->root_switch, TB_SWITCH_TMU_RATE_HIFI, false);
> > +	/*
> > +	 * To support highest CLx state, we set host router's TMU to
> > +	 * Normal mode.
> > +	 */
> > +	tb_switch_tmu_configure(tb->root_switch, TB_SWITCH_TMU_RATE_NORMAL,
> > +				false);
> >  	/* Enable TMU if it is off */
> >  	tb_switch_tmu_enable(tb->root_switch);
> >  	/* Full scan to discover devices added before the driver was loaded. */
> > @@ -1462,7 +1474,7 @@ static void tb_restore_children(struct tb_switch *sw)
> >  		return;
> >  
> >  	/* Silently ignore CLx re-enabling in case CLx is not supported */
> > -	ret = tb_switch_enable_clx(sw, TB_CL0S);
> > +	ret = tb_switch_enable_clx(sw, TB_CL1);
> >  	if (ret && ret != -EOPNOTSUPP)
> >  		tb_sw_warn(sw, "failed to re-enable CLx on upstream port\n");
> >  
> > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > index ad025ff142ba..6a7204cf67f1 100644
> > --- a/drivers/thunderbolt/tb.h
> > +++ b/drivers/thunderbolt/tb.h
> > @@ -934,17 +934,17 @@ void tb_switch_tmu_configure(struct tb_switch *sw,
> >  			     enum tb_switch_tmu_rate rate,
> >  			     bool unidirectional);
> >  /**
> > - * tb_switch_tmu_hifi_is_enabled() - Checks if the specified TMU mode is enabled
> > + * tb_switch_tmu_is_enabled() - Checks if the specified TMU mode is enabled
> >   * @sw: Router whose TMU mode to check
> >   * @unidirectional: If uni-directional (bi-directional otherwise)
> >   *
> >   * Return true if hardware TMU configuration matches the one passed in
> > - * as parameter. That is HiFi and either uni-directional or bi-directional.
> > + * as parameter. That is HiFi/Normal and either uni-directional or bi-directional.
> >   */
> > -static inline bool tb_switch_tmu_hifi_is_enabled(const struct tb_switch *sw,
> > -						 bool unidirectional)
> > +static inline bool tb_switch_tmu_is_enabled(const struct tb_switch *sw,
> > +					    bool unidirectional)
> >  {
> > -	return sw->tmu.rate == TB_SWITCH_TMU_RATE_HIFI &&
> > +	return sw->tmu.rate == sw->tmu.rate_request &&
> >  	       sw->tmu.unidirectional == unidirectional;
> >  }
> >  
> > @@ -975,6 +975,18 @@ static inline bool tb_switch_is_cl0s_enabled(const struct tb_switch *sw)
> >  	return sw->clx == TB_CL0S;
> >  }
> >  
> > +/**
> > + * tb_switch_is_cl1_enabled() - Checks if the CL1 is enabled
> > + * @sw: Router to check for the CL1
> > + *
> > + * Checks if the CL1 is enabled on the router upstream link.
> > + * Not applicable for a host router.
> > + */
> > +static inline bool tb_switch_is_cl1_enabled(const struct tb_switch *sw)
> > +{
> > +	return sw->clx == TB_CL1;
> > +}
> > +
> >  /**
> >   * tb_switch_is_clx_supported() - Is CLx supported on this type of router
> >   * @sw: The router to check CLx support for
> > diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
> > index b301eeb0c89b..f88dd27e4f62 100644
> > --- a/drivers/thunderbolt/tb_regs.h
> > +++ b/drivers/thunderbolt/tb_regs.h
> > @@ -234,6 +234,8 @@ enum usb4_switch_op {
> >  
> >  /* Router TMU configuration */
> >  #define TMU_RTR_CS_0				0x00
> > +#define TMU_RTR_CS_0_FREQ_WIND_MASK		GENMASK(26, 16)
> > +#define TMU_RTR_CS_0_FREQ_WIND_SHIFT		16
> >  #define TMU_RTR_CS_0_TD				BIT(27)
> >  #define TMU_RTR_CS_0_UCAP			BIT(30)
> >  #define TMU_RTR_CS_1				0x01
> > @@ -244,6 +246,12 @@ enum usb4_switch_op {
> >  #define TMU_RTR_CS_3_LOCAL_TIME_NS_MASK		GENMASK(15, 0)
> >  #define TMU_RTR_CS_3_TS_PACKET_INTERVAL_MASK	GENMASK(31, 16)
> >  #define TMU_RTR_CS_3_TS_PACKET_INTERVAL_SHIFT	16
> > +#define TMU_RTR_CS_15				0xf
> > +#define TMU_RTR_CS_15_AVG_MASK			GENMASK(5, 0)
> > +#define TMU_RTR_CS_15_FREQ_AVG_SHIFT		0
> > +#define TMU_RTR_CS_15_DELAY_AVG_SHIFT		6
> > +#define TMU_RTR_CS_15_OFFSET_AVG_SHIFT		12
> > +#define TMU_RTR_CS_15_ERROR_AVG_SHIFT		18
> >  #define TMU_RTR_CS_22				0x16
> >  #define TMU_RTR_CS_24				0x18
> >  #define TMU_RTR_CS_25				0x19
> > diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c
> > index 985ca43b8f39..a03c57a8728c 100644
> > --- a/drivers/thunderbolt/tmu.c
> > +++ b/drivers/thunderbolt/tmu.c
> > @@ -11,6 +11,55 @@
> >  
> >  #include "tb.h"
> >  
> > +static int tb_switch_set_tmu_mode_params(struct tb_switch *sw,
> > +					 enum tb_switch_tmu_rate rate)
> > +{
> > +	u32 freq_meas_wind[2] = { 30, 800 };
> > +	u32 avg_const[2] = { 4, 8 };
> > +	u32 freq, avg, val;
> > +	int ret;
> > +
> > +	if (rate == TB_SWITCH_TMU_RATE_NORMAL) {
> > +		freq = freq_meas_wind[0];
> > +		avg = avg_const[0];
> > +	} else if (rate == TB_SWITCH_TMU_RATE_HIFI) {
> > +		freq = freq_meas_wind[1];
> > +		avg = avg_const[1];
> > +	} else {
> > +		return 0;
> > +	}
> > +
> > +	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH,
> > +			 sw->tmu.cap + TMU_RTR_CS_0, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	val &= ~TMU_RTR_CS_0_FREQ_WIND_MASK;
> > +	val |= freq << TMU_RTR_CS_0_FREQ_WIND_SHIFT;
> > +
> > +	ret = tb_sw_write(sw, &val, TB_CFG_SWITCH,
> > +			  sw->tmu.cap + TMU_RTR_CS_0, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH,
> > +			 sw->tmu.cap + TMU_RTR_CS_15, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	val &= ~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_FREQ_AVG_SHIFT &
> > +		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_DELAY_AVG_SHIFT &
> > +		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_OFFSET_AVG_SHIFT &
> > +		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_ERROR_AVG_SHIFT;
> > +	val |= avg << TMU_RTR_CS_15_FREQ_AVG_SHIFT |
> > +		avg << TMU_RTR_CS_15_DELAY_AVG_SHIFT |
> > +		avg << TMU_RTR_CS_15_OFFSET_AVG_SHIFT |
> > +		avg << TMU_RTR_CS_15_ERROR_AVG_SHIFT;
> 
> Perhaps use FIELD_x macros from include/linux/bitfield.h?
> 
Yes, you are right. I will fix it in v2 series.

> > +
> > +	return tb_sw_write(sw, &val, TB_CFG_SWITCH,
> > +			   sw->tmu.cap + TMU_RTR_CS_15, 1);
> > +}
> > +
> >  static const char *tb_switch_tmu_mode_name(const struct tb_switch *sw)
> >  {
> >  	bool root_switch = !tb_route(sw);
> > @@ -348,7 +397,7 @@ int tb_switch_tmu_disable(struct tb_switch *sw)
> >  
> >  
> >  	if (tb_route(sw)) {
> > -		bool unidirectional = tb_switch_tmu_hifi_is_enabled(sw, true);
> > +		bool unidirectional = sw->tmu.unidirectional;
> >  		struct tb_switch *parent = tb_switch_parent(sw);
> >  		struct tb_port *down, *up;
> >  		int ret;
> > @@ -412,6 +461,7 @@ static void __tb_switch_tmu_off(struct tb_switch *sw, bool unidirectional)
> >  	else
> >  		tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_OFF);
> >  
> > +	tb_switch_set_tmu_mode_params(sw, sw->tmu.rate);
> >  	tb_port_tmu_unidirectional_disable(down);
> >  	tb_port_tmu_unidirectional_disable(up);
> >  }
> > @@ -493,7 +543,11 @@ static int __tb_switch_tmu_enable_unidirectional(struct tb_switch *sw)
> >  
> >  	up = tb_upstream_port(sw);
> >  	down = tb_port_at(tb_route(sw), parent);
> > -	ret = tb_switch_tmu_rate_write(parent, TB_SWITCH_TMU_RATE_HIFI);
> > +	ret = tb_switch_tmu_rate_write(parent, sw->tmu.rate_request);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = tb_switch_set_tmu_mode_params(sw, sw->tmu.rate_request);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -520,7 +574,85 @@ static int __tb_switch_tmu_enable_unidirectional(struct tb_switch *sw)
> >  	return ret;
> >  }
> >  
> > -static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
> > +static void __tb_switch_tmu_change_mode_prev(struct tb_switch *sw)
> > +{
> > +	struct tb_switch *parent = tb_switch_parent(sw);
> > +	struct tb_port *down, *up;
> > +
> > +	down = tb_port_at(tb_route(sw), parent);
> > +	up = tb_upstream_port(sw);
> > +	/*
> > +	 * In case of any failure in one of the steps when change mode,
> > +	 * get back to the TMU configurations in previous mode.
> > +	 * In case of additional failures in the functions below,
> > +	 * ignore them since the caller shall already report a failure.
> > +	 */
> > +	tb_port_tmu_set_unidirectional(down, sw->tmu.unidirectional);
> > +	if (sw->tmu.unidirectional_request)
> > +		tb_switch_tmu_rate_write(parent, sw->tmu.rate);
> > +	else
> > +		tb_switch_tmu_rate_write(sw, sw->tmu.rate);
> > +
> > +	tb_switch_set_tmu_mode_params(sw, sw->tmu.rate);
> > +	tb_port_tmu_set_unidirectional(up, sw->tmu.unidirectional);
> > +}
> > +
> > +static int __tb_switch_tmu_change_mode(struct tb_switch *sw)
> > +{
> > +	struct tb_switch *parent = tb_switch_parent(sw);
> > +	struct tb_port *up, *down;
> > +	int ret;
> > +
> > +	up = tb_upstream_port(sw);
> > +	down = tb_port_at(tb_route(sw), parent);
> > +	ret = tb_port_tmu_set_unidirectional(down,
> > +					     sw->tmu.unidirectional_request);
> 
> This looks better if you put them single line:
> 
> 	ret = tb_port_tmu_set_unidirectional(down, sw->tmu.unidirectional_request);
> 
Yes indeed, but in this case it doesn't fit to 80 chars per line :)

> > +	if (ret)
> > +		goto out;
> > +
> > +	if (sw->tmu.unidirectional_request)
> > +		ret = tb_switch_tmu_rate_write(parent, sw->tmu.rate_request);
> > +	else
> > +		ret = tb_switch_tmu_rate_write(sw, sw->tmu.rate_request);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = tb_switch_set_tmu_mode_params(sw, sw->tmu.rate_request);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = tb_port_tmu_set_unidirectional(up,
> > +					     sw->tmu.unidirectional_request);
> 
> Ditto here.
Same doesn't fit to 80 chars per line :)

> 
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = tb_port_tmu_time_sync_enable(down);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = tb_port_tmu_time_sync_enable(up);
> > +	if (ret)
> > +		goto out;
> > +
> > +	return 0;
> > +
> > +out:
> > +	__tb_switch_tmu_change_mode_prev(sw);
> > +	return ret;
> > +}
> > +
> > +/**
> > + * tb_switch_tmu_enable() - Enable TMU on a router
> > + * @sw: Router whose TMU to enable
> > + *
> > + * Enables TMU of a router to be in uni-directional Normal/Hifi
> > + * or bi-directional HiFi mode. Calling tb_switch_tmu_configure() is required
> > + * before calling this function, to select the mode Normal/HiFi and
> > + * directionality (uni-directional/bi-directional).
> > + * In HiFi mode all tunneling should work. In Normal mode, DP tunneling can't
> > + * work. Uni-directional mode is required for CLx (Link Low-Power) to work.
> > + */
> > +int tb_switch_tmu_enable(struct tb_switch *sw)
> >  {
> >  	bool unidirectional = sw->tmu.unidirectional_request;
> >  	int ret;
> > @@ -536,12 +668,13 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
> >  	if (!tb_switch_is_clx_supported(sw))
> >  		return 0;
> >  
> > -	if (tb_switch_tmu_hifi_is_enabled(sw, sw->tmu.unidirectional_request))
> > +	if (tb_switch_tmu_is_enabled(sw, sw->tmu.unidirectional_request))
> >  		return 0;
> >  
> >  	if (tb_switch_is_titan_ridge(sw) && unidirectional) {
> > -		/* Titan Ridge supports only CL0s */
> > -		if (!tb_switch_is_cl0s_enabled(sw))
> > +		/* Titan Ridge supports CL0s and CL1 only */
> > +		if (!tb_switch_is_cl0s_enabled(sw) &&
> > +		    !tb_switch_is_cl1_enabled(sw))
> 
> I wonder if we can have function like tb_switch_clx_is_enabled() that
> takes bitmask:
> 
> 	if (!tb_switch_clx_is_enabled(sw, TB_CL0S | TB_CL1))
> 		...
> 

Actually both can't be split and shall be enabled together.
Even if we only enable CL0s, we shall also enable CL1.
Also, the "CL0x/CL1 Support" bits shall be set together.
The spec is confusing on this...
Seems like we can merge both of them to be one ENUM and call it someting like
"TB_CL0s_CL1":
enum tb_clx {
	TB_CLX_DISABLE,
	TB_CL0s_CL1,
	TB_CL2,
}
To do it like this:
if (!tb_switch_clx_is_enabled(sw, TB_CL0s_CL1)
...

> >  			return -EOPNOTSUPP;
> >  
> >  		ret = tb_switch_tmu_objection_mask(sw);
> > @@ -558,7 +691,11 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
> >  		return ret;
> >  
> >  	if (tb_route(sw)) {
> > -		/* The used mode changes are from OFF to HiFi-Uni/HiFi-BiDir */
> > +		/*
> > +		 * The used mode changes are from OFF to
> > +		 * HiFi-Uni/HiFi-BiDir/Normal-Uni or from Normal-Uni to
> > +		 * Hifi-Uni.
> > +		 */
> >  		if (sw->tmu.rate == TB_SWITCH_TMU_RATE_OFF) {
> >  			if (unidirectional)
> >  				ret = __tb_switch_tmu_enable_unidirectional(sw);
> > @@ -566,6 +703,10 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
> >  				ret = __tb_switch_tmu_enable_bidirectional(sw);
> >  			if (ret)
> >  				return ret;
> > +		} else if (sw->tmu.rate == TB_SWITCH_TMU_RATE_NORMAL) {
> > +			ret = __tb_switch_tmu_change_mode(sw);
> > +			if (ret)
> > +				return ret;
> >  		}
> >  		sw->tmu.unidirectional = unidirectional;
> >  	} else {
> > @@ -575,35 +716,17 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
> >  		 * of the child node - see above.
> >  		 * Here only the host router' rate configuration is written.
> >  		 */
> > -		ret = tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_HIFI);
> > +		ret = tb_switch_tmu_rate_write(sw, sw->tmu.rate_request);
> >  		if (ret)
> >  			return ret;
> >  	}
> >  
> > -	sw->tmu.rate = TB_SWITCH_TMU_RATE_HIFI;
> > +	sw->tmu.rate = sw->tmu.rate_request;
> >  
> >  	tb_sw_dbg(sw, "TMU: mode set to: %s\n", tb_switch_tmu_mode_name(sw));
> >  	return tb_switch_tmu_set_time_disruption(sw, false);
> >  }
> >  
> > -/**
> > - * tb_switch_tmu_enable() - Enable TMU on a router
> > - * @sw: Router whose TMU to enable
> > - *
> > - * Enables TMU of a router to be in uni-directional or bi-directional HiFi mode.
> > - * Calling tb_switch_tmu_configure() is required before calling this function,
> > - * to select the mode HiFi and directionality (uni-directional/bi-directional).
> > - * In both modes all tunneling should work. Uni-directional mode is required for
> > - * CLx (Link Low-Power) to work.
> > - */
> > -int tb_switch_tmu_enable(struct tb_switch *sw)
> > -{
> > -	if (sw->tmu.rate_request == TB_SWITCH_TMU_RATE_NORMAL)
> > -		return -EOPNOTSUPP;
> > -
> > -	return tb_switch_tmu_hifi_enable(sw);
> > -}
> > -
> >  /**
> >   * tb_switch_tmu_configure() - Configure the TMU rate and directionality
> >   * @sw: Router whose mode to change
> > -- 
> > 2.17.1

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

Re: [PATCH 4/5] thunderbolt: Add CL1 support for USB4 and Titan Ridge routers

[Mika Westerberg]

Hi,

On Wed, May 04, 2022 at 01:52:07PM +0300, Gil Fine wrote:
> On Mon, May 02, 2022 at 01:04:11PM +0300, Mika Westerberg wrote:
> > Hi Gil,
> > 
> > On Sun, May 01, 2022 at 11:33:20PM +0300, Gil Fine wrote:
> > > In this patch we add support for a second low power state of the link: CL1.
> > > Low power states (called collectively CLx) are used to reduce
> > > transmitter and receiver power when a high-speed lane is idle.
> > > We enable it, if both sides of the link support it,
> > > and only for the first hop router (i.e. the first device that connected
> > > to the host router). This is needed for better thermal management.
> > > 
> > > Signed-off-by: Gil Fine <gil.fine@intel.com>
> > > ---
> > >  drivers/thunderbolt/switch.c  |  79 ++++++++-------
> > >  drivers/thunderbolt/tb.c      |  24 +++--
> > >  drivers/thunderbolt/tb.h      |  22 ++++-
> > >  drivers/thunderbolt/tb_regs.h |   8 ++
> > >  drivers/thunderbolt/tmu.c     | 177 ++++++++++++++++++++++++++++------
> > >  5 files changed, 237 insertions(+), 73 deletions(-)
> > > 
> > > diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> > > index 42b7daaf9c4d..4288cb5550f8 100644
> > > --- a/drivers/thunderbolt/switch.c
> > > +++ b/drivers/thunderbolt/switch.c
> > > @@ -3064,7 +3064,7 @@ void tb_switch_suspend(struct tb_switch *sw, bool runtime)
> > >  	 * done for USB4 device too as CLx is re-enabled at resume.
> > >  	 */
> > >  	if (tb_switch_is_clx_enabled(sw)) {
> > > -		if (tb_switch_disable_clx(sw, TB_CL0S))
> > > +		if (tb_switch_disable_clx(sw, TB_CL1))
> > 
> > Hm, what if the router only supports CL0s? Will this still enable CL0s
> > then?
> 
> Actually as I just understood, both can't be split and shall be enabled together.
> Even if we only enable CL0s, we shall also enable CL1.
> Also, the "CL0x/CL1 Support" bits set by HW together.
> Seems as the spec is confusing on this...
> Seems like we can merge both of them to be one ENUM and call it someting like
> "TB_CL0s_CL1":
> enum tb_clx {
> 	TB_CLX_DISABLE,
> 	TB_CL0s_CL1,
> 	TB_CL2,
> }
> To do it like this:
> 
> if (tb_switch_is_clx_enabled(sw, TB_CL0S_CL1)) {
> 	if (tb_switch_disable_clx(sw, TB_CL0S_CL1))
> 		tb_sw_warn(sw, "failed to disable %s on upstream port\n",
> 			   tb_switch_clx_name(TB_CL0S_CL1));
> }
> 
> What do you think?

OK but see my answer to the previous email :)

> > 
> > >  			tb_sw_warn(sw, "failed to disable CLx on upstream port\n");
> > >  	}
> > >  
> > > @@ -3358,12 +3358,12 @@ static bool tb_port_clx_supported(struct tb_port *port, enum tb_clx clx)
> > >  
> > >  	switch (clx) {
> > >  	case TB_CL0S:
> > > +	case TB_CL1:
> > >  		/* CL0s support requires also CL1 support */
> > >  		mask = LANE_ADP_CS_0_CL0S_SUPPORT | LANE_ADP_CS_0_CL1_SUPPORT;
> > >  		break;
> > >  
> > > -	/* For now we support only CL0s. Not CL1, CL2 */
> > > -	case TB_CL1:
> > > +	/* For now we support only CL0s and CL1. Not CL2 */
> > >  	case TB_CL2:
> > >  	default:
> > >  		return false;
> > > @@ -3377,12 +3377,7 @@ static bool tb_port_clx_supported(struct tb_port *port, enum tb_clx clx)
> > >  	return !!(val & mask);
> > >  }
> > >  
> > > -static inline bool tb_port_cl0s_supported(struct tb_port *port)
> > > -{
> > > -	return tb_port_clx_supported(port, TB_CL0S);
> > > -}
> > > -
> > > -static int __tb_port_cl0s_set(struct tb_port *port, bool enable)
> > > +static int __tb_port_cl0s_cl1_set(struct tb_port *port, bool enable)
> > >  {
> > >  	u32 phy, mask;
> > >  	int ret;
> > > @@ -3403,20 +3398,32 @@ static int __tb_port_cl0s_set(struct tb_port *port, bool enable)
> > >  			     port->cap_phy + LANE_ADP_CS_1, 1);
> > >  }
> > >  
> > > -static int tb_port_cl0s_disable(struct tb_port *port)
> > > +static int tb_port_cl0s_cl1_disable(struct tb_port *port)
> > > +{
> > > +	return __tb_port_cl0s_cl1_set(port, false);
> > > +}
> > > +
> > > +static int tb_port_cl0s_cl1_enable(struct tb_port *port)
> > >  {
> > > -	return __tb_port_cl0s_set(port, false);
> > > +	return __tb_port_cl0s_cl1_set(port, true);
> > >  }
> > >  
> > > -static int tb_port_cl0s_enable(struct tb_port *port)
> > > +static const char *tb_switch_clx_name(enum tb_clx clx)
> > >  {
> > > -	return __tb_port_cl0s_set(port, true);
> > > +	switch (clx) {
> > > +	case TB_CL0S:
> > > +		return "CL0s";
> > > +	case TB_CL1:
> > > +		return "CL1";
> > > +	default:
> > > +		return "Unknown";
> > > +	}
> > >  }
> > >  
> > > -static int tb_switch_enable_cl0s(struct tb_switch *sw)
> > > +static int __tb_switch_enable_clx(struct tb_switch *sw, enum tb_clx clx)
> > >  {
> > >  	struct tb_switch *parent = tb_switch_parent(sw);
> > > -	bool up_cl0s_support, down_cl0s_support;
> > > +	bool up_clx_support, down_clx_support;
> > >  	struct tb_port *up, *down;
> > >  	int ret;
> > >  
> > > @@ -3441,37 +3448,37 @@ static int tb_switch_enable_cl0s(struct tb_switch *sw)
> > >  	up = tb_upstream_port(sw);
> > >  	down = tb_port_at(tb_route(sw), parent);
> > >  
> > > -	up_cl0s_support = tb_port_cl0s_supported(up);
> > > -	down_cl0s_support = tb_port_cl0s_supported(down);
> > > +	up_clx_support = tb_port_clx_supported(up, clx);
> > > +	down_clx_support = tb_port_clx_supported(down, clx);
> > >  
> > > -	tb_port_dbg(up, "CL0s %ssupported\n",
> > > -		    up_cl0s_support ? "" : "not ");
> > > -	tb_port_dbg(down, "CL0s %ssupported\n",
> > > -		    down_cl0s_support ? "" : "not ");
> > > +	tb_port_dbg(up, "%s %ssupported\n", tb_switch_clx_name(clx),
> > > +		    up_clx_support ? "" : "not ");
> > > +	tb_port_dbg(down, "%s %ssupported\n", tb_switch_clx_name(clx),
> > > +		    down_clx_support ? "" : "not ");
> > >  
> > > -	if (!up_cl0s_support || !down_cl0s_support)
> > > +	if (!up_clx_support || !down_clx_support)
> > >  		return -EOPNOTSUPP;
> > >  
> > > -	ret = tb_port_cl0s_enable(up);
> > > +	ret = tb_port_cl0s_cl1_enable(up);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = tb_port_cl0s_enable(down);
> > > +	ret = tb_port_cl0s_cl1_enable(down);
> > >  	if (ret) {
> > > -		tb_port_cl0s_disable(up);
> > > +		tb_port_cl0s_cl1_disable(up);
> > >  		return ret;
> > >  	}
> > >  
> > >  	ret = tb_switch_mask_clx_objections(sw);
> > >  	if (ret) {
> > > -		tb_port_cl0s_disable(up);
> > > -		tb_port_cl0s_disable(down);
> > > +		tb_port_cl0s_cl1_disable(up);
> > > +		tb_port_cl0s_cl1_disable(down);
> > >  		return ret;
> > >  	}
> > >  
> > > -	sw->clx = TB_CL0S;
> > > +	sw->clx = clx;
> > >  
> > > -	tb_port_dbg(up, "CL0s enabled\n");
> > > +	tb_port_dbg(up, "%s enabled\n", tb_switch_clx_name(clx));
> > >  	return 0;
> > >  }
> > >  
> > > @@ -3505,14 +3512,15 @@ int tb_switch_enable_clx(struct tb_switch *sw, enum tb_clx clx)
> > >  
> > >  	switch (clx) {
> > >  	case TB_CL0S:
> > > -		return tb_switch_enable_cl0s(sw);
> > > +	case TB_CL1:
> > > +		return __tb_switch_enable_clx(sw, clx);
> > >  
> > >  	default:
> > >  		return -EOPNOTSUPP;
> > >  	}
> > >  }
> > >  
> > > -static int tb_switch_disable_cl0s(struct tb_switch *sw)
> > > +static int tb_switch_disable_cl0s_cl1(struct tb_switch *sw)
> > >  {
> > >  	struct tb_switch *parent = tb_switch_parent(sw);
> > >  	struct tb_port *up, *down;
> > > @@ -3534,17 +3542,17 @@ static int tb_switch_disable_cl0s(struct tb_switch *sw)
> > >  
> > >  	up = tb_upstream_port(sw);
> > >  	down = tb_port_at(tb_route(sw), parent);
> > > -	ret = tb_port_cl0s_disable(up);
> > > +	ret = tb_port_cl0s_cl1_disable(up);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = tb_port_cl0s_disable(down);
> > > +	ret = tb_port_cl0s_cl1_disable(down);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > >  	sw->clx = TB_CLX_DISABLE;
> > >  
> > > -	tb_port_dbg(up, "CL0s disabled\n");
> > > +	tb_port_dbg(up, "CL0s, CL1 disabled\n");
> > >  	return 0;
> > >  }
> > >  
> > > @@ -3562,7 +3570,8 @@ int tb_switch_disable_clx(struct tb_switch *sw, enum tb_clx clx)
> > >  
> > >  	switch (clx) {
> > >  	case TB_CL0S:
> > > -		return tb_switch_disable_cl0s(sw);
> > > +	case TB_CL1:
> > > +		return tb_switch_disable_cl0s_cl1(sw);
> > >  
> > >  	default:
> > >  		return -EOPNOTSUPP;
> > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> > > index 7419cd1aefba..05a084e3e9f6 100644
> > > --- a/drivers/thunderbolt/tb.c
> > > +++ b/drivers/thunderbolt/tb.c
> > > @@ -221,7 +221,7 @@ static int tb_enable_tmu(struct tb_switch *sw)
> > >  	int ret;
> > >  
> > >  	/* If it is already enabled in correct mode, don't touch it */
> > > -	if (tb_switch_tmu_hifi_is_enabled(sw, sw->tmu.unidirectional_request))
> > > +	if (tb_switch_tmu_is_enabled(sw, sw->tmu.unidirectional_request))
> > >  		return 0;
> > >  
> > >  	ret = tb_switch_tmu_disable(sw);
> > > @@ -671,12 +671,19 @@ static void tb_scan_port(struct tb_port *port)
> > >  	/* Set the link configured */
> > >  	tb_switch_configure_link(sw);
> > >  	/* Silently ignore CLx enabling in case CLx is not supported */
> > > -	ret = tb_switch_enable_clx(sw, TB_CL0S);
> > > +	ret = tb_switch_enable_clx(sw, TB_CL1);
> > >  	if (ret && ret != -EOPNOTSUPP)
> > >  		tb_sw_warn(sw, "failed to enable CLx on upstream port\n");
> > >  
> > > -	tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI,
> > > -				tb_switch_is_clx_enabled(sw));
> > > +	if (tb_switch_is_clx_enabled(sw))
> > > +		/*
> > > +		 * To support highest CLx state, we set host router's TMU to
> > > +		 * Normal-Uni mode.
> > > +		 */
> > > +		tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_NORMAL, true);
> > > +	else
> > > +		/* If CLx disabled, configure router's TMU to HiFi-Bidir mode*/
> > > +		tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI, false);
> > >  
> > >  	if (tb_enable_tmu(sw))
> > >  		tb_sw_warn(sw, "failed to enable TMU\n");
> > > @@ -1416,7 +1423,12 @@ static int tb_start(struct tb *tb)
> > >  		return ret;
> > >  	}
> > >  
> > > -	tb_switch_tmu_configure(tb->root_switch, TB_SWITCH_TMU_RATE_HIFI, false);
> > > +	/*
> > > +	 * To support highest CLx state, we set host router's TMU to
> > > +	 * Normal mode.
> > > +	 */
> > > +	tb_switch_tmu_configure(tb->root_switch, TB_SWITCH_TMU_RATE_NORMAL,
> > > +				false);
> > >  	/* Enable TMU if it is off */
> > >  	tb_switch_tmu_enable(tb->root_switch);
> > >  	/* Full scan to discover devices added before the driver was loaded. */
> > > @@ -1462,7 +1474,7 @@ static void tb_restore_children(struct tb_switch *sw)
> > >  		return;
> > >  
> > >  	/* Silently ignore CLx re-enabling in case CLx is not supported */
> > > -	ret = tb_switch_enable_clx(sw, TB_CL0S);
> > > +	ret = tb_switch_enable_clx(sw, TB_CL1);
> > >  	if (ret && ret != -EOPNOTSUPP)
> > >  		tb_sw_warn(sw, "failed to re-enable CLx on upstream port\n");
> > >  
> > > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > > index ad025ff142ba..6a7204cf67f1 100644
> > > --- a/drivers/thunderbolt/tb.h
> > > +++ b/drivers/thunderbolt/tb.h
> > > @@ -934,17 +934,17 @@ void tb_switch_tmu_configure(struct tb_switch *sw,
> > >  			     enum tb_switch_tmu_rate rate,
> > >  			     bool unidirectional);
> > >  /**
> > > - * tb_switch_tmu_hifi_is_enabled() - Checks if the specified TMU mode is enabled
> > > + * tb_switch_tmu_is_enabled() - Checks if the specified TMU mode is enabled
> > >   * @sw: Router whose TMU mode to check
> > >   * @unidirectional: If uni-directional (bi-directional otherwise)
> > >   *
> > >   * Return true if hardware TMU configuration matches the one passed in
> > > - * as parameter. That is HiFi and either uni-directional or bi-directional.
> > > + * as parameter. That is HiFi/Normal and either uni-directional or bi-directional.
> > >   */
> > > -static inline bool tb_switch_tmu_hifi_is_enabled(const struct tb_switch *sw,
> > > -						 bool unidirectional)
> > > +static inline bool tb_switch_tmu_is_enabled(const struct tb_switch *sw,
> > > +					    bool unidirectional)
> > >  {
> > > -	return sw->tmu.rate == TB_SWITCH_TMU_RATE_HIFI &&
> > > +	return sw->tmu.rate == sw->tmu.rate_request &&
> > >  	       sw->tmu.unidirectional == unidirectional;
> > >  }
> > >  
> > > @@ -975,6 +975,18 @@ static inline bool tb_switch_is_cl0s_enabled(const struct tb_switch *sw)
> > >  	return sw->clx == TB_CL0S;
> > >  }
> > >  
> > > +/**
> > > + * tb_switch_is_cl1_enabled() - Checks if the CL1 is enabled
> > > + * @sw: Router to check for the CL1
> > > + *
> > > + * Checks if the CL1 is enabled on the router upstream link.
> > > + * Not applicable for a host router.
> > > + */
> > > +static inline bool tb_switch_is_cl1_enabled(const struct tb_switch *sw)
> > > +{
> > > +	return sw->clx == TB_CL1;
> > > +}
> > > +
> > >  /**
> > >   * tb_switch_is_clx_supported() - Is CLx supported on this type of router
> > >   * @sw: The router to check CLx support for
> > > diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
> > > index b301eeb0c89b..f88dd27e4f62 100644
> > > --- a/drivers/thunderbolt/tb_regs.h
> > > +++ b/drivers/thunderbolt/tb_regs.h
> > > @@ -234,6 +234,8 @@ enum usb4_switch_op {
> > >  
> > >  /* Router TMU configuration */
> > >  #define TMU_RTR_CS_0				0x00
> > > +#define TMU_RTR_CS_0_FREQ_WIND_MASK		GENMASK(26, 16)
> > > +#define TMU_RTR_CS_0_FREQ_WIND_SHIFT		16
> > >  #define TMU_RTR_CS_0_TD				BIT(27)
> > >  #define TMU_RTR_CS_0_UCAP			BIT(30)
> > >  #define TMU_RTR_CS_1				0x01
> > > @@ -244,6 +246,12 @@ enum usb4_switch_op {
> > >  #define TMU_RTR_CS_3_LOCAL_TIME_NS_MASK		GENMASK(15, 0)
> > >  #define TMU_RTR_CS_3_TS_PACKET_INTERVAL_MASK	GENMASK(31, 16)
> > >  #define TMU_RTR_CS_3_TS_PACKET_INTERVAL_SHIFT	16
> > > +#define TMU_RTR_CS_15				0xf
> > > +#define TMU_RTR_CS_15_AVG_MASK			GENMASK(5, 0)
> > > +#define TMU_RTR_CS_15_FREQ_AVG_SHIFT		0
> > > +#define TMU_RTR_CS_15_DELAY_AVG_SHIFT		6
> > > +#define TMU_RTR_CS_15_OFFSET_AVG_SHIFT		12
> > > +#define TMU_RTR_CS_15_ERROR_AVG_SHIFT		18
> > >  #define TMU_RTR_CS_22				0x16
> > >  #define TMU_RTR_CS_24				0x18
> > >  #define TMU_RTR_CS_25				0x19
> > > diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c
> > > index 985ca43b8f39..a03c57a8728c 100644
> > > --- a/drivers/thunderbolt/tmu.c
> > > +++ b/drivers/thunderbolt/tmu.c
> > > @@ -11,6 +11,55 @@
> > >  
> > >  #include "tb.h"
> > >  
> > > +static int tb_switch_set_tmu_mode_params(struct tb_switch *sw,
> > > +					 enum tb_switch_tmu_rate rate)
> > > +{
> > > +	u32 freq_meas_wind[2] = { 30, 800 };
> > > +	u32 avg_const[2] = { 4, 8 };
> > > +	u32 freq, avg, val;
> > > +	int ret;
> > > +
> > > +	if (rate == TB_SWITCH_TMU_RATE_NORMAL) {
> > > +		freq = freq_meas_wind[0];
> > > +		avg = avg_const[0];
> > > +	} else if (rate == TB_SWITCH_TMU_RATE_HIFI) {
> > > +		freq = freq_meas_wind[1];
> > > +		avg = avg_const[1];
> > > +	} else {
> > > +		return 0;
> > > +	}
> > > +
> > > +	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH,
> > > +			 sw->tmu.cap + TMU_RTR_CS_0, 1);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	val &= ~TMU_RTR_CS_0_FREQ_WIND_MASK;
> > > +	val |= freq << TMU_RTR_CS_0_FREQ_WIND_SHIFT;
> > > +
> > > +	ret = tb_sw_write(sw, &val, TB_CFG_SWITCH,
> > > +			  sw->tmu.cap + TMU_RTR_CS_0, 1);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH,
> > > +			 sw->tmu.cap + TMU_RTR_CS_15, 1);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	val &= ~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_FREQ_AVG_SHIFT &
> > > +		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_DELAY_AVG_SHIFT &
> > > +		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_OFFSET_AVG_SHIFT &
> > > +		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_ERROR_AVG_SHIFT;
> > > +	val |= avg << TMU_RTR_CS_15_FREQ_AVG_SHIFT |
> > > +		avg << TMU_RTR_CS_15_DELAY_AVG_SHIFT |
> > > +		avg << TMU_RTR_CS_15_OFFSET_AVG_SHIFT |
> > > +		avg << TMU_RTR_CS_15_ERROR_AVG_SHIFT;
> > 
> > Perhaps use FIELD_x macros from include/linux/bitfield.h?
> > 
> Yes, you are right. I will fix it in v2 series.
> 
> > > +
> > > +	return tb_sw_write(sw, &val, TB_CFG_SWITCH,
> > > +			   sw->tmu.cap + TMU_RTR_CS_15, 1);
> > > +}
> > > +
> > >  static const char *tb_switch_tmu_mode_name(const struct tb_switch *sw)
> > >  {
> > >  	bool root_switch = !tb_route(sw);
> > > @@ -348,7 +397,7 @@ int tb_switch_tmu_disable(struct tb_switch *sw)
> > >  
> > >  
> > >  	if (tb_route(sw)) {
> > > -		bool unidirectional = tb_switch_tmu_hifi_is_enabled(sw, true);
> > > +		bool unidirectional = sw->tmu.unidirectional;
> > >  		struct tb_switch *parent = tb_switch_parent(sw);
> > >  		struct tb_port *down, *up;
> > >  		int ret;
> > > @@ -412,6 +461,7 @@ static void __tb_switch_tmu_off(struct tb_switch *sw, bool unidirectional)
> > >  	else
> > >  		tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_OFF);
> > >  
> > > +	tb_switch_set_tmu_mode_params(sw, sw->tmu.rate);
> > >  	tb_port_tmu_unidirectional_disable(down);
> > >  	tb_port_tmu_unidirectional_disable(up);
> > >  }
> > > @@ -493,7 +543,11 @@ static int __tb_switch_tmu_enable_unidirectional(struct tb_switch *sw)
> > >  
> > >  	up = tb_upstream_port(sw);
> > >  	down = tb_port_at(tb_route(sw), parent);
> > > -	ret = tb_switch_tmu_rate_write(parent, TB_SWITCH_TMU_RATE_HIFI);
> > > +	ret = tb_switch_tmu_rate_write(parent, sw->tmu.rate_request);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = tb_switch_set_tmu_mode_params(sw, sw->tmu.rate_request);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > @@ -520,7 +574,85 @@ static int __tb_switch_tmu_enable_unidirectional(struct tb_switch *sw)
> > >  	return ret;
> > >  }
> > >  
> > > -static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
> > > +static void __tb_switch_tmu_change_mode_prev(struct tb_switch *sw)
> > > +{
> > > +	struct tb_switch *parent = tb_switch_parent(sw);
> > > +	struct tb_port *down, *up;
> > > +
> > > +	down = tb_port_at(tb_route(sw), parent);
> > > +	up = tb_upstream_port(sw);
> > > +	/*
> > > +	 * In case of any failure in one of the steps when change mode,
> > > +	 * get back to the TMU configurations in previous mode.
> > > +	 * In case of additional failures in the functions below,
> > > +	 * ignore them since the caller shall already report a failure.
> > > +	 */
> > > +	tb_port_tmu_set_unidirectional(down, sw->tmu.unidirectional);
> > > +	if (sw->tmu.unidirectional_request)
> > > +		tb_switch_tmu_rate_write(parent, sw->tmu.rate);
> > > +	else
> > > +		tb_switch_tmu_rate_write(sw, sw->tmu.rate);
> > > +
> > > +	tb_switch_set_tmu_mode_params(sw, sw->tmu.rate);
> > > +	tb_port_tmu_set_unidirectional(up, sw->tmu.unidirectional);
> > > +}
> > > +
> > > +static int __tb_switch_tmu_change_mode(struct tb_switch *sw)
> > > +{
> > > +	struct tb_switch *parent = tb_switch_parent(sw);
> > > +	struct tb_port *up, *down;
> > > +	int ret;
> > > +
> > > +	up = tb_upstream_port(sw);
> > > +	down = tb_port_at(tb_route(sw), parent);
> > > +	ret = tb_port_tmu_set_unidirectional(down,
> > > +					     sw->tmu.unidirectional_request);
> > 
> > This looks better if you put them single line:
> > 
> > 	ret = tb_port_tmu_set_unidirectional(down, sw->tmu.unidirectional_request);
> > 
> Yes indeed, but in this case it doesn't fit to 80 chars per line :)

It does not matter - 80 chars is not a hard-limit, it's more like
a guidance :)

[PATCH 5/5] thunderbolt: Change TMU mode to Hifi-Uni once DP tunneled

[Gil Fine]

Here we configure TMU mode to Hifi-Uni once DP tunnel is created.
This is due to accuracy requirement for DP tunneling as appears in
CM guide 1.0, section 7.3.2
Due to Intel HW limitation, once we changed the TMU mode to Hifi-Uni
(when DP is tunnel exists), we don't change TMU mode back to Normal-Uni,
even if DP tunnel is teared-down later.

Signed-off-by: Gil Fine <gil.fine@intel.com>
---
 drivers/thunderbolt/tb.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 05a084e3e9f6..efe53d221ca8 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -50,6 +50,8 @@ struct tb_hotplug_event {
 };
 
 static void tb_handle_hotplug(struct work_struct *work);
+static int tb_enable_tmu_1st_child(struct tb *tb,
+				   enum tb_switch_tmu_rate rate);
 
 static void tb_queue_hotplug(struct tb *tb, u64 route, u8 port, bool unplug)
 {
@@ -118,6 +120,13 @@ static void tb_switch_discover_tunnels(struct tb_switch *sw,
 		switch (port->config.type) {
 		case TB_TYPE_DP_HDMI_IN:
 			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);
+			/*
+			 * In case of DP tunnel exists, change TMU mode to
+			 * HiFi for CL0s to work.
+			 */
+			if (tunnel)
+				tb_enable_tmu_1st_child(tb,
+						TB_SWITCH_TMU_RATE_HIFI);
 			break;
 
 		case TB_TYPE_PCIE_DOWN:
@@ -235,6 +244,31 @@ static int tb_enable_tmu(struct tb_switch *sw)
 	return tb_switch_tmu_enable(sw);
 }
 
+/*
+ * Once a DP tunnel exists in the domain, we set the TMU mode so that
+ * it meets the accuracy requirements and also enables CLx entry (CL0s).
+ * We set the TMU mode of the first depth router(s) for CL0s to work.
+ */
+static int tb_enable_tmu_1st_child(struct tb *tb, enum tb_switch_tmu_rate rate)
+{
+	struct tb_switch *root_sw = tb->root_switch;
+	struct tb_port *port;
+
+	tb_switch_for_each_port(root_sw, port) {
+		struct tb_switch *sw;
+		int ret;
+
+		if (!tb_port_has_remote(port) || !tb_port_is_null(port))
+			continue;
+		sw = port->remote->sw;
+		tb_switch_tmu_configure(sw, rate, tb_switch_is_clx_enabled(sw));
+		if (tb_switch_tmu_enable(sw))
+			tb_dbg(tb, "Fail switching TMU to HiFi for 1st depth router %d\n", ret);
+	}
+
+	return 0;
+}
+
 /**
  * tb_find_unused_port() - return the first inactive port on @sw
  * @sw: Switch to find the port on
@@ -981,6 +1015,12 @@ static void tb_tunnel_dp(struct tb *tb)
 
 	list_add_tail(&tunnel->list, &tcm->tunnel_list);
 	tb_reclaim_usb3_bandwidth(tb, in, out);
+	/*
+	 * In case of DP tunnel exists, change TMU mode to
+	 * HiFi for CL0s to work.
+	 */
+	tb_enable_tmu_1st_child(tb, TB_SWITCH_TMU_RATE_HIFI);
+
 	return;
 
 err_free:
-- 
2.17.1

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

Re: [PATCH 5/5] thunderbolt: Change TMU mode to Hifi-Uni once DP tunneled

[kernel test robot]

Hi Gil,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.18-rc5 next-20220429]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Gil-Fine/thunderbolt-CL1-support-for-USB4-and-Titan-Ridge/20220502-042620
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git b2da7df52e16110c8d8dda0602db81c15711e7ff
config: riscv-randconfig-r003-20220501 (https://download.01.org/0day-ci/archive/20220502/202205020718.NyhJst1y-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 09325d36061e42b495d1f4c7e933e260eac260ed)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/6693a7867a8fe56f34792011896bb3486803f511
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Gil-Fine/thunderbolt-CL1-support-for-USB4-and-Titan-Ridge/20220502-042620
        git checkout 6693a7867a8fe56f34792011896bb3486803f511
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/thunderbolt/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/thunderbolt/tb.c:15:
   In file included from drivers/thunderbolt/tb.h:13:
   In file included from include/linux/pci.h:38:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/thunderbolt/tb.c:15:
   In file included from drivers/thunderbolt/tb.h:13:
   In file included from include/linux/pci.h:38:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/thunderbolt/tb.c:15:
   In file included from drivers/thunderbolt/tb.h:13:
   In file included from include/linux/pci.h:38:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:1024:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
                                                     ~~~~~~~~~~ ^
>> drivers/thunderbolt/tb.c:266:71: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
                           tb_dbg(tb, "Fail switching TMU to HiFi for 1st depth router %d\n", ret);
                                                                                              ^~~
   drivers/thunderbolt/tb.h:661:72: note: expanded from macro 'tb_dbg'
   #define tb_dbg(tb, fmt, arg...) dev_dbg(&(tb)->nhi->pdev->dev, fmt, ## arg)
                                                                          ^~~
   include/linux/dev_printk.h:155:39: note: expanded from macro 'dev_dbg'
           dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
                                                ^~~~~~~~~~~
   include/linux/dynamic_debug.h:167:19: note: expanded from macro 'dynamic_dev_dbg'
                              dev, fmt, ##__VA_ARGS__)
                                          ^~~~~~~~~~~
   include/linux/dynamic_debug.h:152:56: note: expanded from macro '_dynamic_func_call'
           __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
                                                                 ^~~~~~~~~~~
   include/linux/dynamic_debug.h:134:15: note: expanded from macro '__dynamic_func_call'
                   func(&id, ##__VA_ARGS__);               \
                               ^~~~~~~~~~~
   drivers/thunderbolt/tb.c:259:10: note: initialize the variable 'ret' to silence this warning
                   int ret;
                          ^
                           = 0
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   error: A dwo section may not contain relocations
   fatal error: too many errors emitted, stopping now [-ferror-limit=]
   8 warnings and 20 errors generated.


vim +/ret +266 drivers/thunderbolt/tb.c

   246	
   247	/*
   248	 * Once a DP tunnel exists in the domain, we set the TMU mode so that
   249	 * it meets the accuracy requirements and also enables CLx entry (CL0s).
   250	 * We set the TMU mode of the first depth router(s) for CL0s to work.
   251	 */
   252	static int tb_enable_tmu_1st_child(struct tb *tb, enum tb_switch_tmu_rate rate)
   253	{
   254		struct tb_switch *root_sw = tb->root_switch;
   255		struct tb_port *port;
   256	
   257		tb_switch_for_each_port(root_sw, port) {
   258			struct tb_switch *sw;
   259			int ret;
   260	
   261			if (!tb_port_has_remote(port) || !tb_port_is_null(port))
   262				continue;
   263			sw = port->remote->sw;
   264			tb_switch_tmu_configure(sw, rate, tb_switch_is_clx_enabled(sw));
   265			if (tb_switch_tmu_enable(sw))
 > 266				tb_dbg(tb, "Fail switching TMU to HiFi for 1st depth router %d\n", ret);
   267		}
   268	
   269		return 0;
   270	}
   271	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 5/5] thunderbolt: Change TMU mode to Hifi-Uni once DP tunneled

[Mika Westerberg]

On Sun, May 01, 2022 at 11:33:21PM +0300, Gil Fine wrote:
> Here we configure TMU mode to Hifi-Uni once DP tunnel is created.

DisplayPort (at least use that in the $subject).

> This is due to accuracy requirement for DP tunneling as appears in
> CM guide 1.0, section 7.3.2
> Due to Intel HW limitation, once we changed the TMU mode to Hifi-Uni

HW -> hardware

HiFi uni-directional

> (when DP is tunnel exists), we don't change TMU mode back to Normal-Uni,

normal un-idirectional

> even if DP tunnel is teared-down later.

torn down

> 
> Signed-off-by: Gil Fine <gil.fine@intel.com>
> ---
>  drivers/thunderbolt/tb.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 05a084e3e9f6..efe53d221ca8 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -50,6 +50,8 @@ struct tb_hotplug_event {
>  };
>  
>  static void tb_handle_hotplug(struct work_struct *work);
> +static int tb_enable_tmu_1st_child(struct tb *tb,
> +				   enum tb_switch_tmu_rate rate);
>  
>  static void tb_queue_hotplug(struct tb *tb, u64 route, u8 port, bool unplug)
>  {
> @@ -118,6 +120,13 @@ static void tb_switch_discover_tunnels(struct tb_switch *sw,
>  		switch (port->config.type) {
>  		case TB_TYPE_DP_HDMI_IN:
>  			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);
> +			/*
> +			 * In case of DP tunnel exists, change TMU mode to
> +			 * HiFi for CL0s to work.
> +			 */
> +			if (tunnel)
> +				tb_enable_tmu_1st_child(tb,
> +						TB_SWITCH_TMU_RATE_HIFI);
>  			break;
>  
>  		case TB_TYPE_PCIE_DOWN:
> @@ -235,6 +244,31 @@ static int tb_enable_tmu(struct tb_switch *sw)
>  	return tb_switch_tmu_enable(sw);
>  }
>  
> +/*
> + * Once a DP tunnel exists in the domain, we set the TMU mode so that
> + * it meets the accuracy requirements and also enables CLx entry (CL0s).
> + * We set the TMU mode of the first depth router(s) for CL0s to work.
> + */
> +static int tb_enable_tmu_1st_child(struct tb *tb, enum tb_switch_tmu_rate rate)
> +{
> +	struct tb_switch *root_sw = tb->root_switch;
> +	struct tb_port *port;
> +
> +	tb_switch_for_each_port(root_sw, port) {

Can't you use device_for_each_child() here?

> +		struct tb_switch *sw;
> +		int ret;
> +
> +		if (!tb_port_has_remote(port) || !tb_port_is_null(port))
> +			continue;
> +		sw = port->remote->sw;
> +		tb_switch_tmu_configure(sw, rate, tb_switch_is_clx_enabled(sw));
> +		if (tb_switch_tmu_enable(sw))
> +			tb_dbg(tb, "Fail switching TMU to HiFi for 1st depth router %d\n", ret);
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * tb_find_unused_port() - return the first inactive port on @sw
>   * @sw: Switch to find the port on
> @@ -981,6 +1015,12 @@ static void tb_tunnel_dp(struct tb *tb)
>  
>  	list_add_tail(&tunnel->list, &tcm->tunnel_list);
>  	tb_reclaim_usb3_bandwidth(tb, in, out);
> +	/*
> +	 * In case of DP tunnel exists, change TMU mode to
> +	 * HiFi for CL0s to work.
> +	 */
> +	tb_enable_tmu_1st_child(tb, TB_SWITCH_TMU_RATE_HIFI);
> +
>  	return;
>  
>  err_free:
> -- 
> 2.17.1

Re: [PATCH 5/5] thunderbolt: Change TMU mode to Hifi-Uni once DP tunneled

[Gil Fine]

Hi Mika,

On Mon, May 02, 2022 at 01:09:41PM +0300, Mika Westerberg wrote:
> On Sun, May 01, 2022 at 11:33:21PM +0300, Gil Fine wrote:
> > Here we configure TMU mode to Hifi-Uni once DP tunnel is created.
> 
> DisplayPort (at least use that in the $subject).
> 
> > This is due to accuracy requirement for DP tunneling as appears in
> > CM guide 1.0, section 7.3.2
> > Due to Intel HW limitation, once we changed the TMU mode to Hifi-Uni
> 
> HW -> hardware
> 
> HiFi uni-directional
> 
> > (when DP is tunnel exists), we don't change TMU mode back to Normal-Uni,
> 
> normal un-idirectional
> 
> > even if DP tunnel is teared-down later.
> 
> torn down

I will fix in v2 patch.

> 
> > 
> > Signed-off-by: Gil Fine <gil.fine@intel.com>
> > ---
> >  drivers/thunderbolt/tb.c | 40 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> > 
> > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> > index 05a084e3e9f6..efe53d221ca8 100644
> > --- a/drivers/thunderbolt/tb.c
> > +++ b/drivers/thunderbolt/tb.c
> > @@ -50,6 +50,8 @@ struct tb_hotplug_event {
> >  };
> >  
> >  static void tb_handle_hotplug(struct work_struct *work);
> > +static int tb_enable_tmu_1st_child(struct tb *tb,
> > +				   enum tb_switch_tmu_rate rate);
> >  
> >  static void tb_queue_hotplug(struct tb *tb, u64 route, u8 port, bool unplug)
> >  {
> > @@ -118,6 +120,13 @@ static void tb_switch_discover_tunnels(struct tb_switch *sw,
> >  		switch (port->config.type) {
> >  		case TB_TYPE_DP_HDMI_IN:
> >  			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);
> > +			/*
> > +			 * In case of DP tunnel exists, change TMU mode to
> > +			 * HiFi for CL0s to work.
> > +			 */
> > +			if (tunnel)
> > +				tb_enable_tmu_1st_child(tb,
> > +						TB_SWITCH_TMU_RATE_HIFI);
> >  			break;
> >  
> >  		case TB_TYPE_PCIE_DOWN:
> > @@ -235,6 +244,31 @@ static int tb_enable_tmu(struct tb_switch *sw)
> >  	return tb_switch_tmu_enable(sw);
> >  }
> >  
> > +/*
> > + * Once a DP tunnel exists in the domain, we set the TMU mode so that
> > + * it meets the accuracy requirements and also enables CLx entry (CL0s).
> > + * We set the TMU mode of the first depth router(s) for CL0s to work.
> > + */
> > +static int tb_enable_tmu_1st_child(struct tb *tb, enum tb_switch_tmu_rate rate)
> > +{
> > +	struct tb_switch *root_sw = tb->root_switch;
> > +	struct tb_port *port;
> > +
> > +	tb_switch_for_each_port(root_sw, port) {
> 
> Can't you use device_for_each_child() here?

Yes sure,I wasn't aware of it, will do in v2 patch.

> 
> > +		struct tb_switch *sw;
> > +		int ret;
> > +
> > +		if (!tb_port_has_remote(port) || !tb_port_is_null(port))
> > +			continue;
> > +		sw = port->remote->sw;
> > +		tb_switch_tmu_configure(sw, rate, tb_switch_is_clx_enabled(sw));
> > +		if (tb_switch_tmu_enable(sw))
> > +			tb_dbg(tb, "Fail switching TMU to HiFi for 1st depth router %d\n", ret);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * tb_find_unused_port() - return the first inactive port on @sw
> >   * @sw: Switch to find the port on
> > @@ -981,6 +1015,12 @@ static void tb_tunnel_dp(struct tb *tb)
> >  
> >  	list_add_tail(&tunnel->list, &tcm->tunnel_list);
> >  	tb_reclaim_usb3_bandwidth(tb, in, out);
> > +	/*
> > +	 * In case of DP tunnel exists, change TMU mode to
> > +	 * HiFi for CL0s to work.
> > +	 */
> > +	tb_enable_tmu_1st_child(tb, TB_SWITCH_TMU_RATE_HIFI);
> > +
> >  	return;
> >  
> >  err_free:
> > -- 
> > 2.17.1

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

Re: [PATCH 4/5] thunderbolt: Add CL1 support for USB4 and Titan Ridge routers

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5597 bytes --]

CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220501203321.19021-5-gil.fine@intel.com>
References: <20220501203321.19021-5-gil.fine@intel.com>
TO: Gil Fine <gil.fine@intel.com>
TO: andreas.noever(a)gmail.com
TO: michael.jamet(a)intel.com
TO: mika.westerberg(a)linux.intel.com
TO: YehezkelShB(a)gmail.com
CC: gil.fine(a)intel.com
CC: linux-usb(a)vger.kernel.org
CC: lukas(a)wunner.de

Hi Gil,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.18-rc5 next-20220429]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Gil-Fine/thunderbolt-CL1-support-for-USB4-and-Titan-Ridge/20220502-042620
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git b2da7df52e16110c8d8dda0602db81c15711e7ff
:::::: branch date: 3 hours ago
:::::: commit date: 3 hours ago
compiler: sparc64-linux-gcc (GCC) 11.3.0
reproduce (cppcheck warning):
        # apt-get install cppcheck
        git checkout f775f468178fe45bda2e74cc13c066679de592a8
        cppcheck --quiet --enable=style,performance,portability --template=gcc FILE

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


cppcheck possible warnings: (new ones prefixed by >>, may not real problems)

>> drivers/thunderbolt/tmu.c:17:6: warning: Variable 'freq_meas_wind' can be declared with const [constVariable]
    u32 freq_meas_wind[2] = { 30, 800 };
        ^
>> drivers/thunderbolt/tmu.c:18:6: warning: Variable 'avg_const' can be declared with const [constVariable]
    u32 avg_const[2] = { 4, 8 };
        ^

vim +/freq_meas_wind +17 drivers/thunderbolt/tmu.c

cf29b9afb12149 Rajmohan Mani 2019-12-17  13  
f775f468178fe4 Gil Fine      2022-05-01  14  static int tb_switch_set_tmu_mode_params(struct tb_switch *sw,
f775f468178fe4 Gil Fine      2022-05-01  15  					 enum tb_switch_tmu_rate rate)
f775f468178fe4 Gil Fine      2022-05-01  16  {
f775f468178fe4 Gil Fine      2022-05-01 @17  	u32 freq_meas_wind[2] = { 30, 800 };
f775f468178fe4 Gil Fine      2022-05-01 @18  	u32 avg_const[2] = { 4, 8 };
f775f468178fe4 Gil Fine      2022-05-01  19  	u32 freq, avg, val;
f775f468178fe4 Gil Fine      2022-05-01  20  	int ret;
f775f468178fe4 Gil Fine      2022-05-01  21  
f775f468178fe4 Gil Fine      2022-05-01  22  	if (rate == TB_SWITCH_TMU_RATE_NORMAL) {
f775f468178fe4 Gil Fine      2022-05-01  23  		freq = freq_meas_wind[0];
f775f468178fe4 Gil Fine      2022-05-01  24  		avg = avg_const[0];
f775f468178fe4 Gil Fine      2022-05-01  25  	} else if (rate == TB_SWITCH_TMU_RATE_HIFI) {
f775f468178fe4 Gil Fine      2022-05-01  26  		freq = freq_meas_wind[1];
f775f468178fe4 Gil Fine      2022-05-01  27  		avg = avg_const[1];
f775f468178fe4 Gil Fine      2022-05-01  28  	} else {
f775f468178fe4 Gil Fine      2022-05-01  29  		return 0;
f775f468178fe4 Gil Fine      2022-05-01  30  	}
f775f468178fe4 Gil Fine      2022-05-01  31  
f775f468178fe4 Gil Fine      2022-05-01  32  	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH,
f775f468178fe4 Gil Fine      2022-05-01  33  			 sw->tmu.cap + TMU_RTR_CS_0, 1);
f775f468178fe4 Gil Fine      2022-05-01  34  	if (ret)
f775f468178fe4 Gil Fine      2022-05-01  35  		return ret;
f775f468178fe4 Gil Fine      2022-05-01  36  
f775f468178fe4 Gil Fine      2022-05-01  37  	val &= ~TMU_RTR_CS_0_FREQ_WIND_MASK;
f775f468178fe4 Gil Fine      2022-05-01  38  	val |= freq << TMU_RTR_CS_0_FREQ_WIND_SHIFT;
f775f468178fe4 Gil Fine      2022-05-01  39  
f775f468178fe4 Gil Fine      2022-05-01  40  	ret = tb_sw_write(sw, &val, TB_CFG_SWITCH,
f775f468178fe4 Gil Fine      2022-05-01  41  			  sw->tmu.cap + TMU_RTR_CS_0, 1);
f775f468178fe4 Gil Fine      2022-05-01  42  	if (ret)
f775f468178fe4 Gil Fine      2022-05-01  43  		return ret;
f775f468178fe4 Gil Fine      2022-05-01  44  
f775f468178fe4 Gil Fine      2022-05-01  45  	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH,
f775f468178fe4 Gil Fine      2022-05-01  46  			 sw->tmu.cap + TMU_RTR_CS_15, 1);
f775f468178fe4 Gil Fine      2022-05-01  47  	if (ret)
f775f468178fe4 Gil Fine      2022-05-01  48  		return ret;
f775f468178fe4 Gil Fine      2022-05-01  49  
f775f468178fe4 Gil Fine      2022-05-01  50  	val &= ~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_FREQ_AVG_SHIFT &
f775f468178fe4 Gil Fine      2022-05-01  51  		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_DELAY_AVG_SHIFT &
f775f468178fe4 Gil Fine      2022-05-01  52  		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_OFFSET_AVG_SHIFT &
f775f468178fe4 Gil Fine      2022-05-01  53  		~TMU_RTR_CS_15_AVG_MASK << TMU_RTR_CS_15_ERROR_AVG_SHIFT;
f775f468178fe4 Gil Fine      2022-05-01  54  	val |= avg << TMU_RTR_CS_15_FREQ_AVG_SHIFT |
f775f468178fe4 Gil Fine      2022-05-01  55  		avg << TMU_RTR_CS_15_DELAY_AVG_SHIFT |
f775f468178fe4 Gil Fine      2022-05-01  56  		avg << TMU_RTR_CS_15_OFFSET_AVG_SHIFT |
f775f468178fe4 Gil Fine      2022-05-01  57  		avg << TMU_RTR_CS_15_ERROR_AVG_SHIFT;
f775f468178fe4 Gil Fine      2022-05-01  58  
f775f468178fe4 Gil Fine      2022-05-01  59  	return tb_sw_write(sw, &val, TB_CFG_SWITCH,
f775f468178fe4 Gil Fine      2022-05-01  60  			   sw->tmu.cap + TMU_RTR_CS_15, 1);
f775f468178fe4 Gil Fine      2022-05-01  61  }
f775f468178fe4 Gil Fine      2022-05-01  62  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp