Re: [PATCH v3 6/6] thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort tunneled

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

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