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:   Thu, 22 Sep 2022 14:30:34 +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>, sanju.mehta@amd.com,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] thunderbolt: Explicitly enable lane adapter hotplug
 events at startup
Message-ID: <YyxHWgVkHOlswe4r@black.fi.intel.com>
References: <20220921145434.21659-1-mario.limonciello@amd.com>
 <20220921145434.21659-3-mario.limonciello@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220921145434.21659-3-mario.limonciello@amd.com>
Precedence: bulk

Hi Mario,

On Wed, Sep 21, 2022 at 09:54:33AM -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".
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/thunderbolt/switch.c  |  4 ++++
>  drivers/thunderbolt/tb.h      |  1 +
>  drivers/thunderbolt/tb_regs.h |  1 +
>  drivers/thunderbolt/usb4.c    | 19 +++++++++++++++++++
>  4 files changed, 25 insertions(+)
> 
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 77d7f07ca075..65f65cfe1101 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -792,6 +792,10 @@ static int tb_init_port(struct tb_port *port)
>  		(port->config.nfc_credits & ADP_CS_4_TOTAL_BUFFERS_MASK) >>
>  		ADP_CS_4_TOTAL_BUFFERS_SHIFT;
>  
> +	res = usb4_enable_hotplug(port);

Is this something that needs to be done only for the lane adapters? And
not say PCIe or USB adapter?

> +	if (res)
> +		return res;
> +
>  	tb_dump_port(port->sw->tb, port);
>  	return 0;
>  }
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index 5db76de40cc1..b66781ae9c16 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_enable_hotplug(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..40ba14e8e227 100644
> --- a/drivers/thunderbolt/usb4.c
> +++ b/drivers/thunderbolt/usb4.c
> @@ -1046,6 +1046,25 @@ int usb4_port_unlock(struct tb_port *port)
>  	return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_4, 1);
>  }
>  
> +/**
> + * usb4_enable_hotplug() - Enables hotplug for a port
> + * @port: USB4 port to operate on
> + *
> + * Enables hot plug events on a given port.
> + */
> +int usb4_enable_hotplug(struct tb_port *port)

usb4_port_hotplug_enable()

> +{
> +	int ret;
> +	u32 val;
> +

And this one needs to check if this is suitable adapter or if not then
at least mention it in the documentation.

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