Re: [PATCH 2/7] thunderbolt: Add CL0s support for USB4

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Thu, Nov 25, 2021 at 04:38:16PM +0200, Gil Fine wrote:
> +static int tb_switch_enable_cl0s(struct tb_switch *sw)
> +{
> +	struct tb_switch *parent = tb_switch_parent(sw);
> +	bool up_cl0s_support, down_cl0s_support;
> +	struct tb_port *up, *down;
> +	int ret;
> +
> +	if (!tb_switch_is_usb4(sw))
> +		return 0;
> +
> +	/*
> +	 * Enable CLx for host router's downstream port as part of the
> +	 * downstream router enabling procedure.
> +	 */
> +	if (!tb_route(sw))
> +		return 0;
> +
> +	/* Enable CLx only for first hop router (depth = 1) */
> +	if (tb_route(parent))
> +		return 0;
> +
> +	if (tb_switch_pm_secondary_resolve(sw))
> +		return -EINVAL;
> +
> +	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);
> +
> +	tb_port_dbg(up, "CL0s %ssupported\n",
> +		    up_cl0s_support ? "" : "not ");
> +	tb_port_dbg(down, "CL0s %ssupported\n",
> +		    down_cl0s_support ? "" : "not ");
> +
> +	if (!up_cl0s_support || !down_cl0s_support)
> +		return -EOPNOTSUPP;
> +
> +	ret = tb_port_cl0s_enable(up);
> +	if (ret)
> +		return ret;
> +
> +	ret = tb_port_cl0s_enable(down);
> +	if (ret)

Better to get rid of the goto here and do:

	if (ret) {
		tb_port_cl0s_disable(up);
		return ret;
	}

> +		goto out;
> +
> +	sw->clx = TB_CL0S;
> +	tb_sw_dbg(sw, "enabled CL0s on upstream port\n");
> +	return 0;
> +out:
> +	tb_port_cl0s_disable(up);
> +	return ret;
> +}
> +
> +/**
> + * tb_switch_enable_clx() - Enable CLx on upstream port of specified router
> + * @sw: The switch to enable CLx for
> + * @clx: The CLx state to enable
> + *
> + * Enable CLx state only for first hop router. This is because of the HW
> + * limitation on Intel platforms.

Okay but in general do we need to enable it over the whole topology or
is it enough to enable it for the first hop router? I think most of this
is because it allows better thermal management which probably is
applicable for any USB4 host.

> + * 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 conditions are descibed in CM Guide 1.0 section 8.1.
> + *
> + * Return: Returns 0 on success or an error code on failure.
> + */
> +int tb_switch_enable_clx(struct tb_switch *sw, enum tb_clx clx)
> +{
> +	struct tb_switch *root_sw = sw->tb->root_switch;
> +
> +	/* CLx is not enabled and validated on USB4 platforms before ADL */
> +	if (root_sw->generation < 4 ||
> +	    tb_switch_is_tiger_lake(root_sw))
> +		return 0;
> +
> +	switch (clx) {
> +	case TB_CL0S:
> +		return tb_switch_enable_cl0s(sw);
> +
> +	case TB_CL1:
> +	case TB_CL2:

You can drpo the two lines above.

> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int tb_switch_disable_cl0s(struct tb_switch *sw)
> +{
> +	struct tb_switch *parent = tb_switch_parent(sw);
> +	struct tb_port *up, *down;
> +	int ret;
> +
> +	if (!tb_switch_is_usb4(sw))
> +		return 0;
> +
> +	/*
> +	 * Disable CLx for host router's downstream port as part of the
> +	 * downstream router enabling procedure.
> +	 */
> +	if (!tb_route(sw))
> +		return 0;
> +
> +	/* Disable CLx only for first hop router (depth = 1) */
> +	if (tb_route(parent))
> +		return 0;
> +
> +	up = tb_upstream_port(sw);
> +	down = tb_port_at(tb_route(sw), parent);
> +	ret = tb_port_cl0s_disable(up);
> +	if (ret)
> +		return ret;
> +
> +	ret = tb_port_cl0s_disable(down);
> +	if (ret)
> +		return ret;
> +
> +	sw->clx = TB_CLX_DISABLE;
> +	tb_sw_dbg(sw, "disabled CL0s on upstream port\n");
> +	return 0;
> +}
> +
> +/**
> + * tb_switch_disable_clx() - Disable CLx on upstream port of specified router
> + * @sw: The switch to disable CLx for
> + * @clx: The CLx state to disable
> + *
> + * Return: Returns 0 on success or an error code on failure.
> + */
> +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:
> +	case TB_CL2:
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +/**
> + * tb_switch_is_clx_enabled() - Checks if the CLx is enabled
> + * @sw: Router to check the CLx state for
> + *
> + * Checks if the CLx is enabled on the router upstream link.
> + * Not applicable for a host router.
> + */
> +bool tb_switch_is_clx_enabled(struct tb_switch *sw)
> +{
> +	return sw->clx != TB_CLX_DISABLE;
> +}
> +
> +/**
> + * tb_switch_is_cl0s_enabled() - Checks if the CL0s is enabled
> + * @sw: Router to check the CLx state for
> + *
> + * Checks if the CL0s is enabled on the router upstream link.
> + * Not applicable for a host router.
> + */
> +bool tb_switch_is_cl0s_enabled(struct tb_switch *sw)
> +{
> +	return sw->clx == TB_CL0S;
> +}
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 533fe48e85be..f241d42c1c6e 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -669,7 +669,11 @@ static void tb_scan_port(struct tb_port *port)
>  	tb_switch_lane_bonding_enable(sw);
>  	/* Set the link configured */
>  	tb_switch_configure_link(sw);
> -	tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI, false);
> +	if (tb_switch_enable_clx(sw, TB_CL0S))
> +		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) ? true : false);

tb_switch_is_clx_enabled() returns boolean so you can use it directly
here.