[PATCH 1/5] drivers/thunderbolt: Retry DROM reads for more failure scenarios

[PATCH 1/5] drivers/thunderbolt: Retry DROM reads for more failure scenarios

[Mario Limonciello]

Currently DROM reads are only retried in the case that parsing failed.
However if the size or CRC fails, then there should also be a retry.

This helps with reading the DROM on TBT3 devices connected to AMD
Yellow Carp which will sometimes fail on the first attempt.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/thunderbolt/eeprom.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index 470885e6f1c8..10cdbcb55df9 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -553,9 +553,9 @@ static int tb_drom_parse(struct tb_switch *sw)
 	crc = tb_crc8((u8 *) &header->uid, 8);
 	if (crc != header->uid_crc8) {
 		tb_sw_warn(sw,
-			"DROM UID CRC8 mismatch (expected: %#x, got: %#x), aborting\n",
+			"DROM UID CRC8 mismatch (expected: %#x, got: %#x)\n",
 			header->uid_crc8, crc);
-		return -EINVAL;
+		return -EILSEQ;
 	}
 	if (!sw->uid)
 		sw->uid = header->uid;
@@ -654,6 +654,7 @@ int tb_drom_read(struct tb_switch *sw)
 	sw->drom = kzalloc(size, GFP_KERNEL);
 	if (!sw->drom)
 		return -ENOMEM;
+read:
 	res = tb_drom_read_n(sw, 0, sw->drom, size);
 	if (res)
 		goto err;
@@ -662,7 +663,11 @@ int tb_drom_read(struct tb_switch *sw)
 	header = (void *) sw->drom;
 
 	if (header->data_len + TB_DROM_DATA_START != size) {
-		tb_sw_warn(sw, "drom size mismatch, aborting\n");
+		tb_sw_warn(sw, "drom size mismatch\n");
+		if (retries--) {
+			msleep(100);
+			goto read;
+		}
 		goto err;
 	}
 
@@ -683,11 +688,9 @@ int tb_drom_read(struct tb_switch *sw)
 
 	/* If the DROM parsing fails, wait a moment and retry once */
 	if (res == -EILSEQ && retries--) {
-		tb_sw_warn(sw, "parsing DROM failed, retrying\n");
+		tb_sw_warn(sw, "parsing DROM failed\n");
 		msleep(100);
-		res = tb_drom_read_n(sw, 0, sw->drom, size);
-		if (!res)
-			goto parse;
+		goto read;
 	}
 
 	if (!res)
-- 
2.34.1

[PATCH 2/5] drivers/thunderbolt: don't resume switches without uid set

[Mario Limonciello]

Switches might not have a uid set if the DROM read failed during
initialization previously.

Normally upon resume the uid is re-read to confirm it's the same
device connected.
* If the DROM read failed during init but then succeeded during
  resume it could either be a new device or faulty device
* If the DROM read failed during init and also failed during resume
  it might be a different device plugged in all together.

Detect this situation and prevent re-using the same configuration in
these cirucmstances.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/thunderbolt/switch.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index b5fb3e76ed09..294518af4ee4 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -2980,6 +2980,10 @@ int tb_switch_resume(struct tb_switch *sw)
 			return err;
 		}
 
+		/* We don't have any way to confirm this was the same device */
+		if (!sw->uid)
+			return -ENODEV;
+
 		if (tb_switch_is_usb4(sw))
 			err = usb4_switch_read_uid(sw, &uid);
 		else
-- 
2.34.1

Re: [PATCH 2/5] drivers/thunderbolt: don't resume switches without uid set

[Mika Westerberg]

Hi Mario,

On Wed, Mar 02, 2022 at 04:07:06PM -0600, Mario Limonciello wrote:
> Switches might not have a uid set if the DROM read failed during

That's "Routers" and "UID" :)

Also $subject should have prefix "thunderbolt: " not
"drivers/thunderbolt". Please capitalize the summary too:

  thunderbolt: Do not resume routers if UID is not set

The patch itself looks good to me.

> initialization previously.
> 
> Normally upon resume the uid is re-read to confirm it's the same
> device connected.
> * If the DROM read failed during init but then succeeded during
>   resume it could either be a new device or faulty device
> * If the DROM read failed during init and also failed during resume
>   it might be a different device plugged in all together.
> 
> Detect this situation and prevent re-using the same configuration in
> these cirucmstances.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/thunderbolt/switch.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index b5fb3e76ed09..294518af4ee4 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -2980,6 +2980,10 @@ int tb_switch_resume(struct tb_switch *sw)
>  			return err;
>  		}
>  
> +		/* We don't have any way to confirm this was the same device */
> +		if (!sw->uid)
> +			return -ENODEV;
> +
>  		if (tb_switch_is_usb4(sw))
>  			err = usb4_switch_read_uid(sw, &uid);
>  		else
> -- 
> 2.34.1

[PATCH 3/5] drivers/thunderbolt: Don't make DROM read success compulsory

[Mario Limonciello]

The USB4 specification doesn't make any requirements that reading
a device router's DROM is needed for the operation of the device.

On page 207 of the USB4 1.0 spec it does mention that a CM may use
the DROM to make decision though:
```
After enumerating a Router, the Connection Manager may read the
contents of the Router’s DROM. If, after reading the contents of
DROM, the Connection Manager decides that it does not want the
Router in its Domain...
```
Other connection manager solutions don't necessarily read it or gate
the usability of the device on whether it was read.

So make failures when reading the DROM show warnings but not
fail the initialization of the switch.

Link: https://www.usb.org/sites/default/files/USB4%20Specification%2020211116.zip
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/thunderbolt/switch.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 294518af4ee4..ac87e8b50e52 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -2784,10 +2784,8 @@ int tb_switch_add(struct tb_switch *sw)
 
 		/* read drom */
 		ret = tb_drom_read(sw);
-		if (ret) {
-			dev_err(&sw->dev, "reading DROM failed\n");
-			return ret;
-		}
+		if (ret)
+			dev_warn(&sw->dev, "reading DROM failed: %d\n", ret);
 		tb_sw_dbg(sw, "uid: %#llx\n", sw->uid);
 
 		tb_check_quirks(sw);
-- 
2.34.1

Re: [PATCH 3/5] drivers/thunderbolt: Don't make DROM read success compulsory

[Mika Westerberg]

Hi Mario,

On Wed, Mar 02, 2022 at 04:07:07PM -0600, Mario Limonciello wrote:
> The USB4 specification doesn't make any requirements that reading
> a device router's DROM is needed for the operation of the device.
> 
> On page 207 of the USB4 1.0 spec it does mention that a CM may use
> the DROM to make decision though:
> ```
> After enumerating a Router, the Connection Manager may read the
> contents of the Router’s DROM. If, after reading the contents of
> DROM, the Connection Manager decides that it does not want the
> Router in its Domain...
> ```

You don't need to quote the spec for Thunderbolt patches. I can read,
and it's not like it is going to change anyway ;-)

> Other connection manager solutions don't necessarily read it or gate
> the usability of the device on whether it was read.

Indeed.

> So make failures when reading the DROM show warnings but not
> fail the initialization of the switch.
> 
> Link: https://www.usb.org/sites/default/files/USB4%20Specification%2020211116.zip

Also no need to "link" the spec. I know where the spec can be
downloaded.

Ditto for all patches.

> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/thunderbolt/switch.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 294518af4ee4..ac87e8b50e52 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -2784,10 +2784,8 @@ int tb_switch_add(struct tb_switch *sw)
>  
>  		/* read drom */
>  		ret = tb_drom_read(sw);
> -		if (ret) {
> -			dev_err(&sw->dev, "reading DROM failed\n");
> -			return ret;
> -		}
> +		if (ret)
> +			dev_warn(&sw->dev, "reading DROM failed: %d\n", ret);
>  		tb_sw_dbg(sw, "uid: %#llx\n", sw->uid);
>  
>  		tb_check_quirks(sw);
> -- 
> 2.34.1

[PATCH 4/5] drivers/thunderbolt: Clarify/correct register offsets for tb_cap_plug_events

[Mario Limonciello]

The USB4 1.0 specification outlines the `cap_plug_events` structure in
table 13-14 located on page 507.  This shows that there was a mistake
in VSC_CS_1 where plug events disable should be 4 bits and
"TBT3-Compatible" should be 3 bits.

While correcting the mistake, update the names and comments to more
closely match the specification.  This should not change anything
functionally.

Link: https://www.usb.org/sites/default/files/USB4%20Specification%2020211116.zip
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/thunderbolt/tb_regs.h | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
index 70795a2aa9bb..8c42f8bc679e 100644
--- a/drivers/thunderbolt/tb_regs.h
+++ b/drivers/thunderbolt/tb_regs.h
@@ -146,14 +146,14 @@ struct tb_eeprom_ctl {
 
 struct tb_cap_plug_events {
 	struct tb_cap_extended_short cap_header;
-	u32 __unknown1:2;
-	u32 plug_events:5;
-	u32 __unknown2:25;
-	u32 __unknown3;
-	u32 __unknown4;
+	u32 __unknown1:3; /* VSC_CS_1 */
+	u32 plug_events:4; /* VSC_CS_1 */
+	u32 __unknown2:25; /* VSC_CS_1 */
+	u32 vsc_cs_2;
+	u32 vsc_cs_3;
 	struct tb_eeprom_ctl eeprom_ctl;
-	u32 __unknown5[7];
-	u32 drom_offset; /* 32 bit register, but eeprom addresses are 16 bit */
+	u32 __unknown5[7]; /* VSC_CS_5 -> VSC_CS_11 */
+	u32 drom_offset; /* VSC_CS_12: 32 bit register, but eeprom addresses are 16 bit */
 } __packed;
 
 /* device headers */
@@ -464,6 +464,10 @@ struct tb_regs_hop {
 
 /* Plug Events registers */
 #define TB_PLUG_EVENTS_USB_DISABLE		BIT(2)
+#define USB4_PLUG_EVENTS_LANE_DISABLE		BIT(3)
+#define USB4_PLUG_EVENTS_DPOUT_DISABLE		BIT(4)
+#define USB4_PLUG_EVENTS_LOW_DPIN_DISABLE	BIT(5)
+#define USB4_PLUG_EVENTS_HIGH_DPIN_DISABLE	BIT(6)
 
 #define TB_PLUG_EVENTS_PCIE_WR_DATA		0x1b
 #define TB_PLUG_EVENTS_PCIE_CMD			0x1c
-- 
2.34.1

Re: [PATCH 4/5] drivers/thunderbolt: Clarify/correct register offsets for tb_cap_plug_events

[Mika Westerberg]

On Wed, Mar 02, 2022 at 04:07:08PM -0600, Mario Limonciello wrote:
> The USB4 1.0 specification outlines the `cap_plug_events` structure in
> table 13-14 located on page 507.  This shows that there was a mistake
> in VSC_CS_1 where plug events disable should be 4 bits and
> "TBT3-Compatible" should be 3 bits.
> 
> While correcting the mistake, update the names and comments to more
> closely match the specification.  This should not change anything
> functionally.
> 
> Link: https://www.usb.org/sites/default/files/USB4%20Specification%2020211116.zip
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/thunderbolt/tb_regs.h | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
> index 70795a2aa9bb..8c42f8bc679e 100644
> --- a/drivers/thunderbolt/tb_regs.h
> +++ b/drivers/thunderbolt/tb_regs.h
> @@ -146,14 +146,14 @@ struct tb_eeprom_ctl {
>  
>  struct tb_cap_plug_events {
>  	struct tb_cap_extended_short cap_header;
> -	u32 __unknown1:2;
> -	u32 plug_events:5;
> -	u32 __unknown2:25;
> -	u32 __unknown3;
> -	u32 __unknown4;
> +	u32 __unknown1:3; /* VSC_CS_1 */
> +	u32 plug_events:4; /* VSC_CS_1 */
> +	u32 __unknown2:25; /* VSC_CS_1 */
> +	u32 vsc_cs_2;
> +	u32 vsc_cs_3;
>  	struct tb_eeprom_ctl eeprom_ctl;
> -	u32 __unknown5[7];
> -	u32 drom_offset; /* 32 bit register, but eeprom addresses are 16 bit */
> +	u32 __unknown5[7]; /* VSC_CS_5 -> VSC_CS_11 */
> +	u32 drom_offset; /* VSC_CS_12: 32 bit register, but eeprom addresses are 16 bit */
>  } __packed;
>  
>  /* device headers */
> @@ -464,6 +464,10 @@ struct tb_regs_hop {
>  
>  /* Plug Events registers */
>  #define TB_PLUG_EVENTS_USB_DISABLE		BIT(2)
> +#define USB4_PLUG_EVENTS_LANE_DISABLE		BIT(3)
> +#define USB4_PLUG_EVENTS_DPOUT_DISABLE		BIT(4)
> +#define USB4_PLUG_EVENTS_LOW_DPIN_DISABLE	BIT(5)
> +#define USB4_PLUG_EVENTS_HIGH_DPIN_DISABLE	BIT(6)

Since this is TBT3 "compatible" we should use TB_ prefix. What about:

TB_PLUG_EVENTS_CS_1_ ?

or 

TB_VSEC_3_CS_1_ ?

something that is easy to match with the register name in the USB4 spec.

[PATCH 5/5] drivers/thunderbolt: Rename EEPROM handling bits to match USB4 spec

[Mario Limonciello]

The structure `tb_eeprom_ctl` is used to show the bits accessed when
reading/writing EEPROM.

As this structure detail is specified in VSC_CS_4 on page 510 of the
USB4 specification in table 13-14, update the names and use of members
to match the specification. This should not change anything functionally.

Link: https://www.usb.org/sites/default/files/USB4%20Specification%2020211116.zip
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/thunderbolt/eeprom.c  | 24 ++++++++++++------------
 drivers/thunderbolt/tb_regs.h | 10 +++++-----
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index 10cdbcb55df9..c90d22f56d4e 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -17,7 +17,7 @@
  */
 static int tb_eeprom_ctl_write(struct tb_switch *sw, struct tb_eeprom_ctl *ctl)
 {
-	return tb_sw_write(sw, ctl, TB_CFG_SWITCH, sw->cap_plug_events + 4, 1);
+	return tb_sw_write(sw, ctl, TB_CFG_SWITCH, sw->cap_plug_events + ROUTER_CS_4, 1);
 }
 
 /*
@@ -25,7 +25,7 @@ static int tb_eeprom_ctl_write(struct tb_switch *sw, struct tb_eeprom_ctl *ctl)
  */
 static int tb_eeprom_ctl_read(struct tb_switch *sw, struct tb_eeprom_ctl *ctl)
 {
-	return tb_sw_read(sw, ctl, TB_CFG_SWITCH, sw->cap_plug_events + 4, 1);
+	return tb_sw_read(sw, ctl, TB_CFG_SWITCH, sw->cap_plug_events + ROUTER_CS_4, 1);
 }
 
 enum tb_eeprom_transfer {
@@ -46,18 +46,18 @@ static int tb_eeprom_active(struct tb_switch *sw, bool enable)
 	if (res)
 		return res;
 	if (enable) {
-		ctl.access_high = 1;
+		ctl.bit_banging_enable = 1;
 		res = tb_eeprom_ctl_write(sw, &ctl);
 		if (res)
 			return res;
-		ctl.access_low = 0;
+		ctl.fl_cs = 0;
 		return tb_eeprom_ctl_write(sw, &ctl);
 	} else {
-		ctl.access_low = 1;
+		ctl.fl_cs = 1;
 		res = tb_eeprom_ctl_write(sw, &ctl);
 		if (res)
 			return res;
-		ctl.access_high = 0;
+		ctl.bit_banging_enable = 0;
 		return tb_eeprom_ctl_write(sw, &ctl);
 	}
 }
@@ -65,8 +65,8 @@ static int tb_eeprom_active(struct tb_switch *sw, bool enable)
 /*
  * tb_eeprom_transfer - transfer one bit
  *
- * If TB_EEPROM_IN is passed, then the bit can be retrieved from ctl->data_in.
- * If TB_EEPROM_OUT is passed, then ctl->data_out will be written.
+ * If TB_EEPROM_IN is passed, then the bit can be retrieved from ctl->fl_do.
+ * If TB_EEPROM_OUT is passed, then ctl->fl_di will be written.
  */
 static int tb_eeprom_transfer(struct tb_switch *sw, struct tb_eeprom_ctl *ctl,
 			      enum tb_eeprom_transfer direction)
@@ -77,7 +77,7 @@ static int tb_eeprom_transfer(struct tb_switch *sw, struct tb_eeprom_ctl *ctl,
 		if (res)
 			return res;
 	}
-	ctl->clock = 1;
+	ctl->fl_sk = 1;
 	res = tb_eeprom_ctl_write(sw, ctl);
 	if (res)
 		return res;
@@ -86,7 +86,7 @@ static int tb_eeprom_transfer(struct tb_switch *sw, struct tb_eeprom_ctl *ctl,
 		if (res)
 			return res;
 	}
-	ctl->clock = 0;
+	ctl->fl_sk = 0;
 	return tb_eeprom_ctl_write(sw, ctl);
 }
 
@@ -101,7 +101,7 @@ static int tb_eeprom_out(struct tb_switch *sw, u8 val)
 	if (res)
 		return res;
 	for (i = 0; i < 8; i++) {
-		ctl.data_out = val & 0x80;
+		ctl.fl_di = val & 0x80;
 		res = tb_eeprom_transfer(sw, &ctl, TB_EEPROM_OUT);
 		if (res)
 			return res;
@@ -126,7 +126,7 @@ static int tb_eeprom_in(struct tb_switch *sw, u8 *val)
 		res = tb_eeprom_transfer(sw, &ctl, TB_EEPROM_IN);
 		if (res)
 			return res;
-		*val |= ctl.data_in;
+		*val |= ctl.fl_do;
 	}
 	return 0;
 }
diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
index 8c42f8bc679e..872d8977d5e9 100644
--- a/drivers/thunderbolt/tb_regs.h
+++ b/drivers/thunderbolt/tb_regs.h
@@ -133,11 +133,11 @@ struct tb_cap_phy {
 } __packed;
 
 struct tb_eeprom_ctl {
-	bool clock:1; /* send pulse to transfer one bit */
-	bool access_low:1; /* set to 0 before access */
-	bool data_out:1; /* to eeprom */
-	bool data_in:1; /* from eeprom */
-	bool access_high:1; /* set to 1 before access */
+	bool fl_sk:1; /* send pulse to transfer one bit */
+	bool fl_cs:1; /* set to 0 before access */
+	bool fl_di:1; /* to eeprom */
+	bool fl_do:1; /* from eeprom */
+	bool bit_banging_enable:1; /* set to 1 before access */
 	bool not_present:1; /* should be 0 */
 	bool unknown1:1;
 	bool present:1; /* should be 1 */
-- 
2.34.1