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.
