Hi Gil,
On Wed, May 11, 2022 at 05:05:49PM +0300, Gil Fine wrote:
> Here we configure TMU mode to HiFi uni-directional 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 hardware limitation, once we changed the TMU mode to HiFi
> uni-directional (when DP tunnel exists), we don't change TMU mode back to
> normal uni-directional, even if DP tunnel is torn down later.
>
> Signed-off-by: Gil Fine <gil.fine@xxxxxxxxx>
> ---
> drivers/thunderbolt/tb.c | 28 ++++++++++++++++++++++++++++
> drivers/thunderbolt/tb.h | 5 +++++
> drivers/thunderbolt/tmu.c | 14 ++++++++++++++
> 3 files changed, 47 insertions(+)
>
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index f512197e719b..d0f85a8c56de 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 void tb_enable_tmu_1st_child(struct tb *tb,
> + enum tb_switch_tmu_rate rate);
This forward declaration is not needed. You can just move the
implementation before you call it first time.
> 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,19 @@ 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 void tb_enable_tmu_1st_child(struct tb *tb, enum tb_switch_tmu_rate rate)
> +{
> + struct tb_sw_tmu_config tmu = { .rate = rate };
> +
> + device_for_each_child(&tb->root_switch->dev, &tmu,
> + tb_switch_tmu_config_enable);
> +}
> +
> /**
> * tb_find_unused_port() - return the first inactive port on @sw
> * @sw: Switch to find the port on
> @@ -985,6 +1007,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:
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index a16fffba9dd2..3dbd9d919d5f 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -110,6 +110,10 @@ struct tb_switch_tmu {
> enum tb_switch_tmu_rate rate_request;
> };
>
> +struct tb_sw_tmu_config {
> + enum tb_switch_tmu_rate rate;
> +};
Is this wrapper structure really needed?
> +
> enum tb_clx {
> TB_CLX_DISABLE,
> /* CL0s and CL1 are enabled and supported together */
> @@ -934,6 +938,7 @@ int tb_switch_tmu_enable(struct tb_switch *sw);
> void tb_switch_tmu_configure(struct tb_switch *sw,
> enum tb_switch_tmu_rate rate,
> bool unidirectional);
> +int tb_switch_tmu_config_enable(struct device *dev, void *data);
> /**
> * tb_switch_tmu_is_enabled() - Checks if the specified TMU mode is enabled
> * @sw: Router whose TMU mode to check
> diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c
> index e822ab90338b..b8ff9f64a71e 100644
> --- a/drivers/thunderbolt/tmu.c
> +++ b/drivers/thunderbolt/tmu.c
> @@ -727,6 +727,20 @@ int tb_switch_tmu_enable(struct tb_switch *sw)
> return tb_switch_tmu_set_time_disruption(sw, false);
> }
You are missing kernel-doc for the non-static function.
>
> +int tb_switch_tmu_config_enable(struct device *dev, void *data)
Also can we please make it take some real type and not something
arbitrary?
Can it be const too?
> +{
> + if (tb_is_switch(dev)) {
> + struct tb_switch *sw = tb_to_switch(dev);
> + struct tb_sw_tmu_config *tmu = data;
> +
> + tb_switch_tmu_configure(sw, tmu->rate, tb_switch_is_clx_enabled(sw, TB_CL1));
> + if (tb_switch_tmu_enable(sw))
> + tb_sw_dbg(sw, "Fail switching TMU to HiFi for 1st depth router\n");
Be consistent with the messaging so don't start with capital letter
here.
> + }
> +
> + return 0;
> +}
> +
> /**
> * tb_switch_tmu_configure() - Configure the TMU rate and directionality
> * @sw: Router whose mode to change
> --
> 2.17.1
