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

Gil Fine
2022-05-15T20:18:10Z

Hi Mika,

On Fri, May 13, 2022 at 12:53:25PM +0300, Mika Westerberg wrote:
> 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.

Sure, right.

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

Right, will fix

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

Right, will add
>
> >
> > +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?
You mean the names, right?
Something like:
int tb_switch_tmu_config_enable(struct device *parent, void *rate)
If so, yes, I will

>
> Can it be const too?
IIUC, it shall be a function pointer with specified signature otherwise it will fail
at compilation

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

OK

>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * tb_switch_tmu_configure() - Configure the TMU rate and directionality
> > * @sw: Router whose mode to change
> > --
> > 2.17.1

--
Thanks,
Gil