diff mbox series

[v5] thunderbolt: Add quirk to disable CLx

Message ID 1676404912-7048-1-git-send-email-Sanju.Mehta@amd.com (mailing list archive)
State New, archived
Headers show
Series [v5] thunderbolt: Add quirk to disable CLx | expand

Commit Message

Mehta, Sanju Feb. 14, 2023, 8:01 p.m. UTC 
From: Sanjay R Mehta <sanju.mehta@amd.com>

Add QUIRK_NO_CLX to disable the CLx state for hardware which
doesn't supports it.

AMD Yellow Carp and Pink Sardine don't support CLx state,
hence disabling it using QUIRK_NO_CLX.

Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
 drivers/thunderbolt/quirks.c | 19 +++++++++++++++++--
 drivers/thunderbolt/tb.h     | 11 ++++++++---
 2 files changed, 25 insertions(+), 5 deletions(-)

Comments

Mika Westerberg Feb. 15, 2023, 5:49 a.m. UTC | #1 
On Tue, Feb 14, 2023 at 02:01:52PM -0600, Sanjay R Mehta wrote:
>  void tb_check_quirks(struct tb_switch *sw)
>  {
> +	struct tb_switch *root_sw = sw->tb->root_switch;
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(tb_quirks); i++) {
>  		const struct tb_quirk *q = &tb_quirks[i];
>  
> -		if (q->hw_vendor_id && q->hw_vendor_id != sw->config.vendor_id)
> +		if (q->hw_vendor_id && (q->hw_vendor_id != sw->config.vendor_id &&
> +					q->hw_vendor_id != root_sw->config.vendor_id))

Again, why you need to change this?

If you have the AMD host router device ID in the list the quirk applies
and that makes the driver skip enabling CL states for that link. It
should not matter if we enable CL states in the deeper parts of the
topology (even if we actually do not at the moment) because that's
completely different link, right.
Basavaraj Natikar Feb. 15, 2023, 5:59 a.m. UTC | #2 
On 2/15/2023 11:19 AM, Mika Westerberg wrote:
> On Tue, Feb 14, 2023 at 02:01:52PM -0600, Sanjay R Mehta wrote:
>>  void tb_check_quirks(struct tb_switch *sw)
>>  {
>> +	struct tb_switch *root_sw = sw->tb->root_switch;
>>  	int i;
>>  
>>  	for (i = 0; i < ARRAY_SIZE(tb_quirks); i++) {
>>  		const struct tb_quirk *q = &tb_quirks[i];
>>  
>> -		if (q->hw_vendor_id && q->hw_vendor_id != sw->config.vendor_id)
>> +		if (q->hw_vendor_id && (q->hw_vendor_id != sw->config.vendor_id &&
>> +					q->hw_vendor_id != root_sw->config.vendor_id))
> Again, why you need to change this?
>
> If you have the AMD host router device ID in the list the quirk applies
> and that makes the driver skip enabling CL states for that link. It
> should not matter if we enable CL states in the deeper parts of the
> topology (even if we actually do not at the moment) because that's
> completely different link, right.

Thank you Mike for point it out then [PATCH v4] <https://lore.kernel.org/all/1676402030-4653-1-git-send-email-Sanju.Mehta@amd.com/#r> patch changes solves the purpose.

[PATCH v4] thunderbolt: Add quirk to disable CLx <https://lore.kernel.org/all/1676402030-4653-1-git-send-email-Sanju.Mehta@amd.com/#r>
https://lore.kernel.org/all/1676402030-4653-1-git-send-email-Sanju.Mehta@amd.com/

Please review the changes
Mika Westerberg Feb. 15, 2023, 6:16 a.m. UTC | #3 
On Wed, Feb 15, 2023 at 11:29:00AM +0530, Basavaraj Natikar wrote:
> 
> On 2/15/2023 11:19 AM, Mika Westerberg wrote:
> > On Tue, Feb 14, 2023 at 02:01:52PM -0600, Sanjay R Mehta wrote:
> >>  void tb_check_quirks(struct tb_switch *sw)
> >>  {
> >> +	struct tb_switch *root_sw = sw->tb->root_switch;
> >>  	int i;
> >>  
> >>  	for (i = 0; i < ARRAY_SIZE(tb_quirks); i++) {
> >>  		const struct tb_quirk *q = &tb_quirks[i];
> >>  
> >> -		if (q->hw_vendor_id && q->hw_vendor_id != sw->config.vendor_id)
> >> +		if (q->hw_vendor_id && (q->hw_vendor_id != sw->config.vendor_id &&
> >> +					q->hw_vendor_id != root_sw->config.vendor_id))
> > Again, why you need to change this?
> >
> > If you have the AMD host router device ID in the list the quirk applies
> > and that makes the driver skip enabling CL states for that link. It
> > should not matter if we enable CL states in the deeper parts of the
> > topology (even if we actually do not at the moment) because that's
> > completely different link, right.
> 
> Thank you Mike for point it out then [PATCH v4] <https://lore.kernel.org/all/1676402030-4653-1-git-send-email-Sanju.Mehta@amd.com/#r> patch changes solves the purpose.
> 
> [PATCH v4] thunderbolt: Add quirk to disable CLx <https://lore.kernel.org/all/1676402030-4653-1-git-send-email-Sanju.Mehta@amd.com/#r>
> https://lore.kernel.org/all/1676402030-4653-1-git-send-email-Sanju.Mehta@amd.com/
> 
> Please review the changes

Indeed v4 looks good. I just skipped it because there appeared several
versions of the patch in my inbox overnight so picked the last one for
review ;-)
Basavaraj Natikar Feb. 15, 2023, 6:23 a.m. UTC | #4 
On 2/15/2023 11:46 AM, Mika Westerberg wrote:
> On Wed, Feb 15, 2023 at 11:29:00AM +0530, Basavaraj Natikar wrote:
>> On 2/15/2023 11:19 AM, Mika Westerberg wrote:
>>> On Tue, Feb 14, 2023 at 02:01:52PM -0600, Sanjay R Mehta wrote:
>>>>  void tb_check_quirks(struct tb_switch *sw)
>>>>  {
>>>> +	struct tb_switch *root_sw = sw->tb->root_switch;
>>>>  	int i;
>>>>  
>>>>  	for (i = 0; i < ARRAY_SIZE(tb_quirks); i++) {
>>>>  		const struct tb_quirk *q = &tb_quirks[i];
>>>>  
>>>> -		if (q->hw_vendor_id && q->hw_vendor_id != sw->config.vendor_id)
>>>> +		if (q->hw_vendor_id && (q->hw_vendor_id != sw->config.vendor_id &&
>>>> +					q->hw_vendor_id != root_sw->config.vendor_id))
>>> Again, why you need to change this?
>>>
>>> If you have the AMD host router device ID in the list the quirk applies
>>> and that makes the driver skip enabling CL states for that link. It
>>> should not matter if we enable CL states in the deeper parts of the
>>> topology (even if we actually do not at the moment) because that's
>>> completely different link, right.
>> Thank you Mike for point it out then [PATCH v4] <https://lore.kernel.org/all/1676402030-4653-1-git-send-email-Sanju.Mehta@amd.com/#r> patch changes solves the purpose.
>>
>> [PATCH v4] thunderbolt: Add quirk to disable CLx <https://lore.kernel.org/all/1676402030-4653-1-git-send-email-Sanju.Mehta@amd.com/#r>
>> https://lore.kernel.org/all/1676402030-4653-1-git-send-email-Sanju.Mehta@amd.com/
>>
>> Please review the changes
> Indeed v4 looks good. I just skipped it because there appeared several
> versions of the patch in my inbox overnight so picked the last one for
> review ;-)

Sorry for the confusion. Please apply [PATCH v4]
Thank you Mika for quick and timely review.

Thanks,
--
Basavaraj
diff mbox series

Patch

diff --git a/drivers/thunderbolt/quirks.c b/drivers/thunderbolt/quirks.c
index b5f2ec7..3fc5474 100644
--- a/drivers/thunderbolt/quirks.c
+++ b/drivers/thunderbolt/quirks.c
@@ -20,6 +20,11 @@  static void quirk_dp_credit_allocation(struct tb_switch *sw)
 	}
 }
 
+static void quirk_clx_disable(struct tb_switch *sw)
+{
+	sw->quirks |= QUIRK_NO_CLX;
+}
+
 struct tb_quirk {
 	u16 hw_vendor_id;
 	u16 hw_device_id;
@@ -37,6 +42,13 @@  static const struct tb_quirk tb_quirks[] = {
 	 * DP buffers.
 	 */
 	{ 0x8087, 0x0b26, 0x0000, 0x0000, quirk_dp_credit_allocation },
+	/*
+	 * CLx is not supported on AMD USB4 Yellow Carp and Pink Sardine platforms.
+	 */
+	{ 0x0438, 0x0208, 0x0000, 0x0000, quirk_clx_disable },
+	{ 0x0438, 0x0209, 0x0000, 0x0000, quirk_clx_disable },
+	{ 0x0438, 0x020a, 0x0000, 0x0000, quirk_clx_disable },
+	{ 0x0438, 0x020b, 0x0000, 0x0000, quirk_clx_disable },
 };
 
 /**
@@ -47,14 +59,17 @@  static const struct tb_quirk tb_quirks[] = {
  */
 void tb_check_quirks(struct tb_switch *sw)
 {
+	struct tb_switch *root_sw = sw->tb->root_switch;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(tb_quirks); i++) {
 		const struct tb_quirk *q = &tb_quirks[i];
 
-		if (q->hw_vendor_id && q->hw_vendor_id != sw->config.vendor_id)
+		if (q->hw_vendor_id && (q->hw_vendor_id != sw->config.vendor_id &&
+					q->hw_vendor_id != root_sw->config.vendor_id))
 			continue;
-		if (q->hw_device_id && q->hw_device_id != sw->config.device_id)
+		if (q->hw_device_id && (q->hw_device_id != sw->config.device_id &&
+					q->hw_device_id != root_sw->config.device_id))
 			continue;
 		if (q->vendor && q->vendor != sw->vendor)
 			continue;
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index f978697..206759a 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -23,6 +23,11 @@ 
 #define NVM_MAX_SIZE		SZ_512K
 #define NVM_DATA_DWORDS		16
 
+/* Keep link controller awake during update */
+#define QUIRK_FORCE_POWER_LINK_CONTROLLER		BIT(0)
+/* Disable CLx if not supported */
+#define QUIRK_NO_CLX					BIT(1)
+
 /**
  * struct tb_nvm - Structure holding NVM information
  * @dev: Owner of the NVM
@@ -997,6 +1002,9 @@  static inline bool tb_switch_is_clx_enabled(const struct tb_switch *sw,
  */
 static inline bool tb_switch_is_clx_supported(const struct tb_switch *sw)
 {
+	if (sw->quirks & QUIRK_NO_CLX)
+		return false;
+
 	return tb_switch_is_usb4(sw) || tb_switch_is_titan_ridge(sw);
 }
 
@@ -1254,9 +1262,6 @@  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);
 
-/* Keep link controller awake during update */
-#define QUIRK_FORCE_POWER_LINK_CONTROLLER		BIT(0)
-
 void tb_check_quirks(struct tb_switch *sw);
 
 #ifdef CONFIG_ACPI