[PATCH 0/3] thunderbolt: Few improvements for retimer access

[PATCH 0/3] thunderbolt: Few improvements for retimer access

[Mika Westerberg]

Hi,

These 3 patches improve the way we access retimers over sideband in
various situations, and make it more compliant with the USB4 spec.

Mika Westerberg (3):
  thunderbolt: Read retimer NVM authentication status prior tb_retimer_set_inbound_sbtx()
  thunderbolt: Do not send UNSET_INBOUND_SBTX when retimer NVM authentication started
  thunderbolt: Enable/disable sideband depending on USB4 port offline mode

 drivers/thunderbolt/retimer.c | 58 +++++++++++++++++++++++++++++------
 drivers/thunderbolt/tb.h      |  5 +++
 2 files changed, 54 insertions(+), 9 deletions(-)

-- 
2.39.2

[PATCH 1/3] thunderbolt: Read retimer NVM authentication status prior tb_retimer_set_inbound_sbtx()

[Mika Westerberg]

According to the USB4 retimer guide the correct order is immediately
after sending ENUMERATE_RETIMERS so update the code to follow this.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/retimer.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/thunderbolt/retimer.c b/drivers/thunderbolt/retimer.c
index ccc2f0e7adba..fb8e113488db 100644
--- a/drivers/thunderbolt/retimer.c
+++ b/drivers/thunderbolt/retimer.c
@@ -187,6 +187,21 @@ static ssize_t nvm_authenticate_show(struct device *dev,
 	return ret;
 }
 
+static void tb_retimer_nvm_authenticate_status(struct tb_port *port, u32 *status)
+{
+	int i;
+
+	tb_port_dbg(port, "reading NVM authentication status of retimers\n");
+
+	/*
+	 * Before doing anything else, read the authentication status.
+	 * If the retimer has it set, store it for the new retimer
+	 * device instance.
+	 */
+	for (i = 1; i <= TB_MAX_RETIMER_INDEX; i++)
+		usb4_port_retimer_nvm_authenticate_status(port, i, &status[i]);
+}
+
 static void tb_retimer_set_inbound_sbtx(struct tb_port *port)
 {
 	int i;
@@ -449,18 +464,16 @@ int tb_retimer_scan(struct tb_port *port, bool add)
 		return ret;
 
 	/*
-	 * Enable sideband channel for each retimer. We can do this
-	 * regardless whether there is device connected or not.
+	 * Immediately after sending enumerate retimers read the
+	 * authentication status of each retimer.
 	 */
-	tb_retimer_set_inbound_sbtx(port);
+	tb_retimer_nvm_authenticate_status(port, status);
 
 	/*
-	 * Before doing anything else, read the authentication status.
-	 * If the retimer has it set, store it for the new retimer
-	 * device instance.
+	 * Enable sideband channel for each retimer. We can do this
+	 * regardless whether there is device connected or not.
 	 */
-	for (i = 1; i <= TB_MAX_RETIMER_INDEX; i++)
-		usb4_port_retimer_nvm_authenticate_status(port, i, &status[i]);
+	tb_retimer_set_inbound_sbtx(port);
 
 	for (i = 1; i <= TB_MAX_RETIMER_INDEX; i++) {
 		/*
-- 
2.39.2

[PATCH 2/3] thunderbolt: Do not send UNSET_INBOUND_SBTX when retimer NVM authentication started

[Mika Westerberg]

Once retimer NVM authentication is started, sending UNSET_INBOUND_SBTX
will fail so avoid doing that. Only send it when we are writing an image
with not authentication or when the authentication failed early.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/retimer.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/retimer.c b/drivers/thunderbolt/retimer.c
index fb8e113488db..a273fb02a02c 100644
--- a/drivers/thunderbolt/retimer.c
+++ b/drivers/thunderbolt/retimer.c
@@ -244,6 +244,13 @@ static ssize_t nvm_authenticate_store(struct device *dev,
 	rt->auth_status = 0;
 
 	if (val) {
+		/*
+		 * When NVM authentication starts the retimer is not
+		 * accessible so calling tb_retimer_unset_inbound_sbtx()
+		 * will fail and therefore we do not call it. Exception
+		 * is when the validation fails or we only write the new
+		 * NVM image without authentication.
+		 */
 		tb_retimer_set_inbound_sbtx(rt->port);
 		if (val == AUTHENTICATE_ONLY) {
 			ret = tb_retimer_nvm_authenticate(rt, true);
@@ -264,7 +271,8 @@ static ssize_t nvm_authenticate_store(struct device *dev,
 	}
 
 exit_unlock:
-	tb_retimer_unset_inbound_sbtx(rt->port);
+	if (ret || val == WRITE_ONLY)
+		tb_retimer_unset_inbound_sbtx(rt->port);
 	mutex_unlock(&rt->tb->lock);
 exit_rpm:
 	pm_runtime_mark_last_busy(&rt->dev);
-- 
2.39.2

[PATCH 3/3] thunderbolt: Enable/disable sideband depending on USB4 port offline mode

[Mika Westerberg]

When USB4 port is in offline mode (this mean there is no device
attached) we want to keep the sideband up to make it possible to
communicate with the retimers. In the same way there is no need to
enable sideband transactions when the USB4 port is not offline as they
are already up.

For this reason make the enabling/disabling depend on the USB4 port
offline status.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/retimer.c | 19 +++++++++++++++++++
 drivers/thunderbolt/tb.h      |  5 +++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/thunderbolt/retimer.c b/drivers/thunderbolt/retimer.c
index a273fb02a02c..47becb363ada 100644
--- a/drivers/thunderbolt/retimer.c
+++ b/drivers/thunderbolt/retimer.c
@@ -206,6 +206,15 @@ static void tb_retimer_set_inbound_sbtx(struct tb_port *port)
 {
 	int i;
 
+	/*
+	 * When USB4 port is online sideband communications are
+	 * already up.
+	 */
+	if (!usb4_port_device_is_offline(port->usb4))
+		return;
+
+	tb_port_dbg(port, "enabling sideband transactions\n");
+
 	for (i = 1; i <= TB_MAX_RETIMER_INDEX; i++)
 		usb4_port_retimer_set_inbound_sbtx(port, i);
 }
@@ -214,6 +223,16 @@ static void tb_retimer_unset_inbound_sbtx(struct tb_port *port)
 {
 	int i;
 
+	/*
+	 * When USB4 port is offline we need to keep the sideband
+	 * communications up to make it possible to communicate with
+	 * the connected retimers.
+	 */
+	if (usb4_port_device_is_offline(port->usb4))
+		return;
+
+	tb_port_dbg(port, "disabling sideband transactions\n");
+
 	for (i = TB_MAX_RETIMER_INDEX; i >= 1; i--)
 		usb4_port_retimer_unset_inbound_sbtx(port, i);
 }
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index a89e0eb399b0..71ca6d500032 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -1326,6 +1326,11 @@ struct usb4_port *usb4_port_device_add(struct tb_port *port);
 void usb4_port_device_remove(struct usb4_port *usb4);
 int usb4_port_device_resume(struct usb4_port *usb4);
 
+static inline bool usb4_port_device_is_offline(const struct usb4_port *usb4)
+{
+	return usb4->offline;
+}
+
 void tb_check_quirks(struct tb_switch *sw);
 
 #ifdef CONFIG_ACPI
-- 
2.39.2

Re: [PATCH 3/3] thunderbolt: Enable/disable sideband depending on USB4 port offline mode

[Yehezkel Bernat]

On Fri, Jun 2, 2023 at 12:11 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> When USB4 port is in offline mode (this mean there is no device
> attached) we want to keep the sideband up to make it possible to
> communicate with the retimers. In the same way there is no need to
> enable sideband transactions when the USB4 port is not offline as they
> are already up.
>
> For this reason make the enabling/disabling depend on the USB4 port
> offline status.

I'm probably missing something here, but if we don't allow disabling it when the
port is offline, and when the port is online the sideband is enabled, when can
it be disabled? If we can manually disable it when the port is online, on
enablement we can't assume that it's already enabled just because the port
is online, as we might have manually disabled it earlier.

Re: [PATCH 3/3] thunderbolt: Enable/disable sideband depending on USB4 port offline mode

[Mika Westerberg]

Hi,

On Sat, Jun 03, 2023 at 10:24:38PM +0300, Yehezkel Bernat wrote:
> On Fri, Jun 2, 2023 at 12:11 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > When USB4 port is in offline mode (this mean there is no device
> > attached) we want to keep the sideband up to make it possible to
> > communicate with the retimers. In the same way there is no need to
> > enable sideband transactions when the USB4 port is not offline as they
> > are already up.
> >
> > For this reason make the enabling/disabling depend on the USB4 port
> > offline status.
> 
> I'm probably missing something here, but if we don't allow disabling it when the
> port is offline, and when the port is online the sideband is enabled, when can
> it be disabled? If we can manually disable it when the port is online, on
> enablement we can't assume that it's already enabled just because the port
> is online, as we might have manually disabled it earlier.

We allow disabling them when the port is online. This all basically
separates how the device attached and non-device attached handle the
sideband communications.

Re: [PATCH 3/3] thunderbolt: Enable/disable sideband depending on USB4 port offline mode

[Yehezkel Bernat]

On Sun, Jun 4, 2023 at 8:11 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi,
>
> On Sat, Jun 03, 2023 at 10:24:38PM +0300, Yehezkel Bernat wrote:
> > On Fri, Jun 2, 2023 at 12:11 PM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > When USB4 port is in offline mode (this mean there is no device
> > > attached) we want to keep the sideband up to make it possible to
> > > communicate with the retimers. In the same way there is no need to
> > > enable sideband transactions when the USB4 port is not offline as they
> > > are already up.
> > >
> > > For this reason make the enabling/disabling depend on the USB4 port
> > > offline status.
> >
> > I'm probably missing something here, but if we don't allow disabling it when the
> > port is offline, and when the port is online the sideband is enabled, when can
> > it be disabled? If we can manually disable it when the port is online, on
> > enablement we can't assume that it's already enabled just because the port
> > is online, as we might have manually disabled it earlier.
>
> We allow disabling them when the port is online. This all basically
> separates how the device attached and non-device attached handle the
> sideband communications.

OK, but then we don't enable it back, as we assume it's enabled because the
port is online, even while the user might have disabled it earlier?

Re: [PATCH 3/3] thunderbolt: Enable/disable sideband depending on USB4 port offline mode

[Mika Westerberg]

Hi,

On Sun, Jun 04, 2023 at 12:16:18PM +0300, Yehezkel Bernat wrote:
> On Sun, Jun 4, 2023 at 8:11 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > Hi,
> >
> > On Sat, Jun 03, 2023 at 10:24:38PM +0300, Yehezkel Bernat wrote:
> > > On Fri, Jun 2, 2023 at 12:11 PM Mika Westerberg
> > > <mika.westerberg@linux.intel.com> wrote:
> > > >
> > > > When USB4 port is in offline mode (this mean there is no device
> > > > attached) we want to keep the sideband up to make it possible to
> > > > communicate with the retimers. In the same way there is no need to
> > > > enable sideband transactions when the USB4 port is not offline as they
> > > > are already up.
> > > >
> > > > For this reason make the enabling/disabling depend on the USB4 port
> > > > offline status.
> > >
> > > I'm probably missing something here, but if we don't allow disabling it when the
> > > port is offline, and when the port is online the sideband is enabled, when can
> > > it be disabled? If we can manually disable it when the port is online, on
> > > enablement we can't assume that it's already enabled just because the port
> > > is online, as we might have manually disabled it earlier.
> >
> > We allow disabling them when the port is online. This all basically
> > separates how the device attached and non-device attached handle the
> > sideband communications.
> 
> OK, but then we don't enable it back, as we assume it's enabled because the
> port is online, even while the user might have disabled it earlier?

Well there are two "modes" how they are accessed. Normal cases user
cannot offline the port so the sideband comes up when the link comes up
(e.g device is connected). In this case after the retimer enumeration
put them back to "passthrough" mode by sending the UNSET_INBOUND_SBTX.
This is needed to make the link come up after hot-reboot etc and was
recommended by our hardware folks.

The second case is when there is no device attached. This requires
special firmare too. In ChromeOS there is an ACPI _DSM method that does
this and if present we allow user to offline the port but only when
there is no device attached. In this mode we need to bring up the
sideband so we must send the SET_INBOUND_SBTX during enumeration but we
also need to keep them communicating so we cannot send
UNSET_INBOUND_SBTX. This mode is only used to upgrade the retimer NVM
firmware.

This is the idea behind these patches but let me know if I misssed
something.

Re: [PATCH 0/3] thunderbolt: Few improvements for retimer access

[Mika Westerberg]

On Fri, Jun 02, 2023 at 12:10:52PM +0300, Mika Westerberg wrote:
> Hi,
> 
> These 3 patches improve the way we access retimers over sideband in
> various situations, and make it more compliant with the USB4 spec.
> 
> Mika Westerberg (3):
>   thunderbolt: Read retimer NVM authentication status prior tb_retimer_set_inbound_sbtx()
>   thunderbolt: Do not send UNSET_INBOUND_SBTX when retimer NVM authentication started
>   thunderbolt: Enable/disable sideband depending on USB4 port offline mode

All applied to thunderbolt.git/next.

Yehezkel, let me know if you have any objections or want more
clarifications.