drm: replace magic nuumbers

PATCH Superseded Bjorn Helgaas 4 participants 6 messages Nov 7, 2019

Subsystems (3)

DRM drivers PCI Radeon and amdgpu DRM drivers

lore

Share & tools

lore

Bjorn Helgaas

author

2019-11-07 22:20 UTC #0 lore raw

cover drm: replace magic nuumbers

From: Bjorn Helgaas <[email protected]>

amdgpu and radeon do a bit of mucking with the PCIe Link Control 2 register, some of it using hard-coded magic numbers. The idea here is to replace those with #defines.

I haven't signed off on these because the first one actually changes the bits involved because the existing code looks like it might have a typo. But I have no way to know for sure.

I don't intend the Target Link Speed patch to change anything, so it should be straightforward to review.

Bjorn Helgaas (2):
drm: replace Compliance/Margin magic numbers with PCI_EXP_LNKCTL2
definitions
drm: replace Target Link Speed magic numbers with PCI_EXP_LNKCTL2
definitions

 drivers/gpu/drm/amd/amdgpu/cik.c | 22 ++++++++++++++--------
 drivers/gpu/drm/amd/amdgpu/si.c  | 18 +++++++++++-------
 drivers/gpu/drm/radeon/cik.c     | 22 ++++++++++++++--------
 drivers/gpu/drm/radeon/si.c      | 22 ++++++++++++++--------
 include/uapi/linux/pci_regs.h    |  2 ++
 5 files changed, 55 insertions(+), 31 deletions(-)

Signature · 1 line

2.24.0.rc1.363.gb1bccd3e3d-goog

Bjorn Helgaas

author

2019-11-07 22:20 UTC #1 lore raw

PATCH 1/2 drm: replace Compliance/Margin magic numbers with PCI_EXP_LNKCTL2 definitions

From: Bjorn Helgaas <[email protected]>

Add definitions for these PCIe Link Control 2 register fields:

Enter Compliance
Transmit Margin

and use them in amdgpu and radeon.

NOTE: This is a functional change because "7 << 9" looks like it might be a typo. That mask includes the high order bit of Transmit Margin, the Enter Modified Compliance bit, and the Compliance SOS bit. I'm just guessing that what was intended was the 3-bit Transmit Margit field at bits 9:7. Please check to see whether this is correct.

---
 drivers/gpu/drm/amd/amdgpu/cik.c | 14 ++++++++++----
 drivers/gpu/drm/amd/amdgpu/si.c  | 10 +++++++---
 drivers/gpu/drm/radeon/cik.c     | 14 ++++++++++----
 drivers/gpu/drm/radeon/si.c      | 14 ++++++++++----
 include/uapi/linux/pci_regs.h    |  2 ++
 5 files changed, 39 insertions(+), 15 deletions(-)

drivers/gpu/drm/amd/amdgpu/cik.c +10 -4

index b81bb414fcb3..e4a595cdd4c1 100644
@@ -1498,13 +1498,19 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev)
 
 				/* linkctl2 */
 				pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, &tmp16);
-				tmp16 &= ~((1 << 4) | (7 << 9));
-				tmp16 |= (bridge_cfg2 & ((1 << 4) | (7 << 9)));
+				tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN);
+				tmp16 |= (bridge_cfg2 &
+					  (PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN));
 				pci_write_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, tmp16);
 
 				pci_read_config_word(adev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &tmp16);
-				tmp16 &= ~((1 << 4) | (7 << 9));
-				tmp16 |= (gpu_cfg2 & ((1 << 4) | (7 << 9)));
+				tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN);
+				tmp16 |= (gpu_cfg2 &
+					  (PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN));
 				pci_write_config_word(adev->pdev, gpu_pos + PCI_EXP_LNKCTL2, tmp16);
 
 				tmp = RREG32_PCIE(ixPCIE_LC_CNTL4);

drivers/gpu/drm/amd/amdgpu/si.c +7 -3

index 493af42152f2..cf543410a424 100644
@@ -1737,12 +1737,16 @@ static void si_pcie_gen3_enable(struct amdgpu_device *adev)
 				pci_write_config_word(adev->pdev, gpu_pos + PCI_EXP_LNKCTL, tmp16);
 
 				pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, &tmp16);
-				tmp16 &= ~((1 << 4) | (7 << 9));
-				tmp16 |= (bridge_cfg2 & ((1 << 4) | (7 << 9)));
+				tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN);
+				tmp16 |= (bridge_cfg2 &
+					  (PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN));
 				pci_write_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, tmp16);
 
 				pci_read_config_word(adev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &tmp16);
-				tmp16 &= ~((1 << 4) | (7 << 9));
+				tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN);
 				tmp16 |= (gpu_cfg2 & ((1 << 4) | (7 << 9)));
 				pci_write_config_word(adev->pdev, gpu_pos + PCI_EXP_LNKCTL2, tmp16);

drivers/gpu/drm/radeon/cik.c +10 -4

index 62eab82a64f9..95ffa0bff2d8 100644
@@ -9619,13 +9619,19 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev)
 
 				/* linkctl2 */
 				pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, &tmp16);
-				tmp16 &= ~((1 << 4) | (7 << 9));
-				tmp16 |= (bridge_cfg2 & ((1 << 4) | (7 << 9)));
+				tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN);
+				tmp16 |= (bridge_cfg2 &
+					  (PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN));
 				pci_write_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, tmp16);
 
 				pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &tmp16);
-				tmp16 &= ~((1 << 4) | (7 << 9));
-				tmp16 |= (gpu_cfg2 & ((1 << 4) | (7 << 9)));
+				tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN);
+				tmp16 |= (gpu_cfg2 &
+					  (PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN));
 				pci_write_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, tmp16);
 
 				tmp = RREG32_PCIE_PORT(PCIE_LC_CNTL4);

drivers/gpu/drm/radeon/si.c +10 -4

index 05894d198a79..69993d34d1e9 100644
@@ -7202,13 +7202,19 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev)
 
 				/* linkctl2 */
 				pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, &tmp16);
-				tmp16 &= ~((1 << 4) | (7 << 9));
-				tmp16 |= (bridge_cfg2 & ((1 << 4) | (7 << 9)));
+				tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN);
+				tmp16 |= (bridge_cfg2 &
+					  (PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN));
 				pci_write_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, tmp16);
 
 				pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &tmp16);
-				tmp16 &= ~((1 << 4) | (7 << 9));
-				tmp16 |= (gpu_cfg2 & ((1 << 4) | (7 << 9)));
+				tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN);
+				tmp16 |= (gpu_cfg2 &
+					  (PCI_EXP_LNKCTL2_ENTER_COMP |
+					   PCI_EXP_LNKCTL2_TX_MARGIN));
 				pci_write_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, tmp16);
 
 				tmp = RREG32_PCIE_PORT(PCIE_LC_CNTL4);

include/uapi/linux/pci_regs.h +2 -0

index 29d6e93fd15e..03446be8a7be 100644
@@ -673,6 +673,8 @@
 #define  PCI_EXP_LNKCTL2_TLS_8_0GT	0x0003 /* Supported Speed 8GT/s */
 #define  PCI_EXP_LNKCTL2_TLS_16_0GT	0x0004 /* Supported Speed 16GT/s */
 #define  PCI_EXP_LNKCTL2_TLS_32_0GT	0x0005 /* Supported Speed 32GT/s */
+#define  PCI_EXP_LNKCTL2_ENTER_COMP	0x0010 /* Enter Compliance */
+#define  PCI_EXP_LNKCTL2_TX_MARGIN	0x0380 /* Enter Compliance */
 #define PCI_EXP_LNKSTA2		50	/* Link Status 2 */
 #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2	52	/* v2 endpoints with link end here */
 #define PCI_EXP_SLTCAP2		52	/* Slot Capabilities 2 */

Signature · 1 line

2.24.0.rc1.363.gb1bccd3e3d-goog

Ilia Mirkin

2019-11-07 22:30 UTC #2 lore raw

Re: [PATCH 1/2] drm: replace Compliance/Margin magic numbers with PCI_EXP_LNKCTL2 definitions

include/uapi/linux/pci_regs.h line 677

Quoted 10 lines · Bjorn Helgaas

include/uapi/linux/pci_regs.h +2 -0

index 29d6e93fd15e..03446be8a7be 100644
@@ -673,6 +673,8 @@
 #define  PCI_EXP_LNKCTL2_TLS_8_0GT     0x0003 /* Supported Speed 8GT/s */
 #define  PCI_EXP_LNKCTL2_TLS_16_0GT    0x0004 /* Supported Speed 16GT/s */
 #define  PCI_EXP_LNKCTL2_TLS_32_0GT    0x0005 /* Supported Speed 32GT/s */
+#define  PCI_EXP_LNKCTL2_ENTER_COMP    0x0010 /* Enter Compliance */
+#define  PCI_EXP_LNKCTL2_TX_MARGIN     0x0380 /* Enter Compliance */

Without commenting on the meat of the patch, this comment should probably read "Transmit Margin" or something along those lines?

-ilia

Bjorn Helgaas

2019-11-07 22:49 UTC #3 lore raw

Re: [PATCH 1/2] drm: replace Compliance/Margin magic numbers with PCI_EXP_LNKCTL2 definitions

Quoted 15 lines · Ilia Mirkin

include/uapi/linux/pci_regs.h line 677

Quoted 10 lines · Bjorn Helgaas

include/uapi/linux/pci_regs.h +2 -0

index 29d6e93fd15e..03446be8a7be 100644
@@ -673,6 +673,8 @@
 #define  PCI_EXP_LNKCTL2_TLS_8_0GT     0x0003 /* Supported Speed 8GT/s */
 #define  PCI_EXP_LNKCTL2_TLS_16_0GT    0x0004 /* Supported Speed 16GT/s */
 #define  PCI_EXP_LNKCTL2_TLS_32_0GT    0x0005 /* Supported Speed 32GT/s */
+#define  PCI_EXP_LNKCTL2_ENTER_COMP    0x0010 /* Enter Compliance */
+#define  PCI_EXP_LNKCTL2_TX_MARGIN     0x0380 /* Enter Compliance */

Without commenting on the meat of the patch, this comment should probably read "Transmit Margin" or something along those lines?

Indeed, thank you!

Bjorn Helgaas

author

2019-11-07 22:20 UTC #4 lore raw

PATCH 2/2 drm: replace Target Link Speed magic numbers with PCI_EXP_LNKCTL2 definitions

In reply to Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

Replace hard-coded magic numbers with the descript PCI_EXP_LNKCTL2 definitions. No functional change intended.

---
 drivers/gpu/drm/amd/amdgpu/cik.c | 8 ++++----
 drivers/gpu/drm/amd/amdgpu/si.c  | 8 ++++----
 drivers/gpu/drm/radeon/cik.c     | 8 ++++----
 drivers/gpu/drm/radeon/si.c      | 8 ++++----
 4 files changed, 16 insertions(+), 16 deletions(-)

drivers/gpu/drm/amd/amdgpu/cik.c +4 -4

index e4a595cdd4c1..3067bb874032 100644
@@ -1527,13 +1527,13 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev)
 	WREG32_PCIE(ixPCIE_LC_SPEED_CNTL, speed_cntl);
 
 	pci_read_config_word(adev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &tmp16);
-	tmp16 &= ~0xf;
+	tmp16 &= ~PCI_EXP_LNKCTL2_TLS;
 	if (adev->pm.pcie_gen_mask & CAIL_PCIE_LINK_SPEED_SUPPORT_GEN3)
-		tmp16 |= 3; /* gen3 */
+		tmp16 |= PCI_EXP_LNKCTL2_TLS_8_0GT; /* gen3 */
 	else if (adev->pm.pcie_gen_mask & CAIL_PCIE_LINK_SPEED_SUPPORT_GEN2)
-		tmp16 |= 2; /* gen2 */
+		tmp16 |= PCI_EXP_LNKCTL2_TLS_5_0GT; /* gen2 */
 	else
-		tmp16 |= 1; /* gen1 */
+		tmp16 |= PCI_EXP_LNKCTL2_TLS_2_5GT; /* gen1 */
 	pci_write_config_word(adev->pdev, gpu_pos + PCI_EXP_LNKCTL2, tmp16);
 
 	speed_cntl = RREG32_PCIE(ixPCIE_LC_SPEED_CNTL);

drivers/gpu/drm/amd/amdgpu/si.c +4 -4

index cf543410a424..d5c83d82063b 100644
@@ -1762,13 +1762,13 @@ static void si_pcie_gen3_enable(struct amdgpu_device *adev)
 	WREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL, speed_cntl);
 
 	pci_read_config_word(adev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &tmp16);
-	tmp16 &= ~0xf;
+	tmp16 &= ~PCI_EXP_LNKCTL2_TLS;
 	if (adev->pm.pcie_gen_mask & CAIL_PCIE_LINK_SPEED_SUPPORT_GEN3)
-		tmp16 |= 3;
+		tmp16 |= PCI_EXP_LNKCTL2_TLS_8_0GT; /* gen3 */
 	else if (adev->pm.pcie_gen_mask & CAIL_PCIE_LINK_SPEED_SUPPORT_GEN2)
-		tmp16 |= 2;
+		tmp16 |= PCI_EXP_LNKCTL2_TLS_5_0GT; /* gen2 */
 	else
-		tmp16 |= 1;
+		tmp16 |= PCI_EXP_LNKCTL2_TLS_2_5GT; /* gen1 */
 	pci_write_config_word(adev->pdev, gpu_pos + PCI_EXP_LNKCTL2, tmp16);
 
 	speed_cntl = RREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL);

drivers/gpu/drm/radeon/cik.c +4 -4

index 95ffa0bff2d8..a280442c81aa 100644
@@ -9647,13 +9647,13 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev)
 	WREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL, speed_cntl);
 
 	pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &tmp16);
-	tmp16 &= ~0xf;
+	tmp16 &= ~PCI_EXP_LNKCTL2_TLS;
 	if (speed_cap == PCIE_SPEED_8_0GT)
-		tmp16 |= 3; /* gen3 */
+		tmp16 |= PCI_EXP_LNKCTL2_TLS_8_0GT; /* gen3 */
 	else if (speed_cap == PCIE_SPEED_5_0GT)
-		tmp16 |= 2; /* gen2 */
+		tmp16 |= PCI_EXP_LNKCTL2_TLS_5_0GT; /* gen2 */
 	else
-		tmp16 |= 1; /* gen1 */
+		tmp16 |= PCI_EXP_LNKCTL2_TLS_2_5GT; /* gen1 */
 	pci_write_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, tmp16);
 
 	speed_cntl = RREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL);

drivers/gpu/drm/radeon/si.c +4 -4

index 69993d34d1e9..529e70a42019 100644
@@ -7230,13 +7230,13 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev)
 	WREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL, speed_cntl);
 
 	pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &tmp16);
-	tmp16 &= ~0xf;
+	tmp16 &= ~PCI_EXP_LNKCTL2_TLS;
 	if (speed_cap == PCIE_SPEED_8_0GT)
-		tmp16 |= 3; /* gen3 */
+		tmp16 |= PCI_EXP_LNKCTL2_TLS_8_0GT; /* gen3 */
 	else if (speed_cap == PCIE_SPEED_5_0GT)
-		tmp16 |= 2; /* gen2 */
+		tmp16 |= PCI_EXP_LNKCTL2_TLS_5_0GT; /* gen2 */
 	else
-		tmp16 |= 1; /* gen1 */
+		tmp16 |= PCI_EXP_LNKCTL2_TLS_2_5GT; /* gen1 */
 	pci_write_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, tmp16);
 
 	speed_cntl = RREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL);

Signature · 6 lines

2.24.0.rc1.363.gb1bccd3e3d-goog

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Deucher, Alexander

maintainer Reviewed-by on the v1 series

2019-11-07 22:43 UTC #5 lore raw

In reply to Bjorn Helgaas

Quoted 15 lines

-----Original Message-----
From: amd-gfx <[email protected]> On Behalf Of Bjorn Helgaas
Sent: Thursday, November 7, 2019 5:21 PM
To: Deucher, Alexander <[email protected]>; Koenig, Christian <[email protected]>; Zhou, David(ChunMing) <[email protected]>; David Airlie <[email protected]>; Daniel Vetter <[email protected]>
Cc: Bjorn Helgaas <[email protected]>; [email protected]; [email protected]; Frederick Lawler <[email protected]>; [email protected]
Subject: [PATCH 0/2] drm: replace magic nuumbers
From: Bjorn Helgaas <[email protected]>

Quoted 8 lines

amdgpu and radeon do a bit of mucking with the PCIe Link Control 2 register, some of it using hard-coded magic numbers. The idea here is to replace those with #defines.
I haven't signed off on these because the first one actually changes the bits involved because the existing code looks like it might have a typo. But I have no way to know for sure.

It is a typo. Your patches look correct to me. The series is:

Reviewed-by Alex Deucher <[email protected]>

Quoted 16 lines

I don't intend the Target Link Speed patch to change anything, so it should be straightforward to review.
Bjorn Helgaas (2):
drm: replace Compliance/Margin magic numbers with PCI_EXP_LNKCTL2
definitions
drm: replace Target Link Speed magic numbers with PCI_EXP_LNKCTL2
definitions
 drivers/gpu/drm/amd/amdgpu/cik.c | 22 ++++++++++++++--------
drivers/gpu/drm/amd/amdgpu/si.c  | 18 +++++++++++-------
 drivers/gpu/drm/radeon/cik.c     | 22 ++++++++++++++--------
 drivers/gpu/drm/radeon/si.c      | 22 ++++++++++++++--------
 include/uapi/linux/pci_regs.h    |  2 ++
 5 files changed, 55 insertions(+), 31 deletions(-)

Quoted 7 lines

--
2.24.0.rc1.363.gb1bccd3e3d-goog
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx