Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 23 Sep 2022 12:16:48 +0300
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Mario Limonciello <mario.limonciello@amd.com>
Cc:     Andreas Noever <andreas.noever@gmail.com>,
        Michael Jamet <michael.jamet@intel.com>,
        Yehezkel Bernat <YehezkelShB@gmail.com>,
        Mehta Sanju <Sanju.Mehta@amd.com>, stable@vger.kernel.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] thunderbolt: Explicitly enable lane adapter hotplug
 events at startup
Message-ID: <Yy15gKzHyMcitY/N@black.fi.intel.com>
References: <20220922160730.898-1-mario.limonciello@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220922160730.898-1-mario.limonciello@amd.com>
Precedence: bulk

Hi Mario,

On Thu, Sep 22, 2022 at 11:07:29AM -0500, Mario Limonciello wrote:
> Software that has run before the USB4 CM in Linux runs may have disabled
> hotplug events for a given lane adapter.
> 
> Other CMs such as that one distributed with Windows 11 will enable hotplug
> events. Do the same thing in the Linux CM which fixes hotplug events on
> "AMD Pink Sardine".
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v1->v2:
>  * Only send second patch as first was merged already
>  * s/usb4_enable_hotplug/usb4_port_hotplug_enable/
>  * Clarify intended users in documentation comment
>  * Only call for lane adapters
>  * Add stable tag
> 
>  drivers/thunderbolt/switch.c  |  4 ++++
>  drivers/thunderbolt/tb.h      |  1 +
>  drivers/thunderbolt/tb_regs.h |  1 +
>  drivers/thunderbolt/usb4.c    | 20 ++++++++++++++++++++
>  4 files changed, 26 insertions(+)
> 
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 77d7f07ca075..3213239d12c8 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -778,6 +778,10 @@ static int tb_init_port(struct tb_port *port)
>  
>  			if (!tb_port_read(port, &hop, TB_CFG_HOPS, 0, 2))
>  				port->ctl_credits = hop.initial_credits;
> +
> +			res = usb4_port_hotplug_enable(port);
> +			if (res)

I think this does not belong here in tb_init_port(). This is called from
both FW and SW CM paths and we don't want to confuse the FW CM more than
necessary ;-)

So instead I think this should be added to tb_plug_events_active().

Otherwise looks good to me.

> +				return res;
>  		}
>  		if (!port->ctl_credits)
>  			port->ctl_credits = 2;
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index 5db76de40cc1..332159f984fc 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -1174,6 +1174,7 @@ int usb4_switch_add_ports(struct tb_switch *sw);
>  void usb4_switch_remove_ports(struct tb_switch *sw);
>  
>  int usb4_port_unlock(struct tb_port *port);
> +int usb4_port_hotplug_enable(struct tb_port *port);
>  int usb4_port_configure(struct tb_port *port);
>  void usb4_port_unconfigure(struct tb_port *port);
>  int usb4_port_configure_xdomain(struct tb_port *port);
> diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
> index 166054110388..bbe38b2d9057 100644
> --- a/drivers/thunderbolt/tb_regs.h
> +++ b/drivers/thunderbolt/tb_regs.h
> @@ -308,6 +308,7 @@ struct tb_regs_port_header {
>  #define ADP_CS_5				0x05
>  #define ADP_CS_5_LCA_MASK			GENMASK(28, 22)
>  #define ADP_CS_5_LCA_SHIFT			22
> +#define ADP_CS_5_DHP				BIT(31)
>  
>  /* TMU adapter registers */
>  #define TMU_ADP_CS_3				0x03
> diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
> index 3a2e7126db9d..f0b5a8f1ed3a 100644
> --- a/drivers/thunderbolt/usb4.c
> +++ b/drivers/thunderbolt/usb4.c
> @@ -1046,6 +1046,26 @@ int usb4_port_unlock(struct tb_port *port)
>  	return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_4, 1);
>  }
>  
> +/**
> + * usb4_port_hotplug_enable() - Enables hotplug for a port
> + * @port: USB4 port to operate on
> + *
> + * Enables hot plug events on a given port. This is only intended
> + * to be used on lane, DP-IN, and DP-OUT adapters.
> + */
> +int usb4_port_hotplug_enable(struct tb_port *port)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = tb_port_read(port, &val, TB_CFG_PORT, ADP_CS_5, 1);
> +	if (ret)
> +		return ret;
> +
> +	val &= ~ADP_CS_5_DHP;
> +	return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_5, 1);
> +}
> +
>  static int usb4_port_set_configured(struct tb_port *port, bool configured)
>  {
>  	int ret;
> -- 
> 2.34.1