All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration
@ 2018-04-17  0:28 Frederick Lawler
  2018-04-17  0:28 ` [PATCH v3 1/2] PCI: Add PCI_EXP_LNKCTL2_TLS* macros Frederick Lawler
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Frederick Lawler @ 2018-04-17  0:28 UTC (permalink / raw)
  To: bhelgaas, mike.marciniszyn, dennis.dalessandro, dledford, jgg
  Cc: linux-pci, linux-rdma, Frederick Lawler

The IB/hfi1 driver uses custom macros to configure the target link speed. Some 
macros were previously replaced, but not fully. This patch series addresses the
configuration inconsistencies by adding PCI_EXP_LNKCTL2_TLS* macros to PCI,
and then use them in the following IB/hfi1 patch.

V3: 
	* PCI: Add PCI_EXP_LNKCTL2_TLS_* macros
	* Drop patch to use extract_speed() in do_pcie_gen3_transition()
V2:
	* s/LINK/LNK

Frederick Lawler (2):
  PCI: Add PCI_EXP_LNKCTL2_TLS* macros
  IB/hfi1: Replace custom hfi1 macros with PCIe macros

 drivers/infiniband/hw/hfi1/pcie.c | 24 ++++++++----------------
 include/uapi/linux/pci_regs.h     |  5 +++++
 2 files changed, 13 insertions(+), 16 deletions(-)

-- 
2.17.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 1/2] PCI: Add PCI_EXP_LNKCTL2_TLS* macros
  2018-04-17  0:28 [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration Frederick Lawler
@ 2018-04-17  0:28 ` Frederick Lawler
  2018-04-17  0:28 ` [PATCH v3 2/2] IB/hfi1: Replace custom hfi1 macros with PCIe macros Frederick Lawler
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Frederick Lawler @ 2018-04-17  0:28 UTC (permalink / raw)
  To: bhelgaas, mike.marciniszyn, dennis.dalessandro, dledford, jgg
  Cc: linux-pci, linux-rdma, Frederick Lawler

The Link Control 2 register is missing macros for Target Link Speeds.
Add those in.

Signed-off-by: Frederick Lawler <fred@fredlawl.com>
---
 include/uapi/linux/pci_regs.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 103ba797a8f3..18f9acacca95 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -655,6 +655,11 @@
 #define  PCI_EXP_LNKCAP2_SLS_16_0GB	0x00000010 /* Supported Speed 16GT/s */
 #define  PCI_EXP_LNKCAP2_CROSSLINK	0x00000100 /* Crosslink supported */
 #define PCI_EXP_LNKCTL2		48	/* Link Control 2 */
+#define PCI_EXP_LNKCTL2_TLS		0x000f
+#define PCI_EXP_LNKCTL2_TLS_2_5GB	0x0001 /* Supported Speed 2.5GT/s */
+#define PCI_EXP_LNKCTL2_TLS_5_0GB	0x0002 /* Supported Speed 5GT/s */
+#define PCI_EXP_LNKCTL2_TLS_8_0GB	0x0003 /* Supported Speed 8GT/s */
+#define PCI_EXP_LNKCTL2_TLS_16_0GB	0x0004 /* Supported Speed 16GT/s */
 #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 */
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 2/2] IB/hfi1: Replace custom hfi1 macros with PCIe macros
  2018-04-17  0:28 [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration Frederick Lawler
  2018-04-17  0:28 ` [PATCH v3 1/2] PCI: Add PCI_EXP_LNKCTL2_TLS* macros Frederick Lawler
@ 2018-04-17  0:28 ` Frederick Lawler
  2018-04-20 18:21   ` Ruhl, Michael J
  2018-04-17  6:53 ` [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration Christoph Hellwig
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Frederick Lawler @ 2018-04-17  0:28 UTC (permalink / raw)
  To: bhelgaas, mike.marciniszyn, dennis.dalessandro, dledford, jgg
  Cc: linux-pci, linux-rdma, Frederick Lawler

IB/hfi1 contains custom macros for PCIe link configuration. Remove the
custom macros in favor of the PCIe link macros. No functional change
intended.

Signed-off-by: Frederick Lawler <fred@fredlawl.com>
--
V3: Use PCI_EXP_LNKCTL2_TLS* macros
V2: s/LINK/LNK
---
 drivers/infiniband/hw/hfi1/pcie.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c
index 83d66e862207..fe2a6d602856 100644
--- a/drivers/infiniband/hw/hfi1/pcie.c
+++ b/drivers/infiniband/hw/hfi1/pcie.c
@@ -56,11 +56,6 @@
 #include "chip_registers.h"
 #include "aspm.h"
 
-/* link speed vector for Gen3 speed - not in Linux headers */
-#define GEN1_SPEED_VECTOR 0x1
-#define GEN2_SPEED_VECTOR 0x2
-#define GEN3_SPEED_VECTOR 0x3
-
 /*
  * This file contains PCIe utility routines.
  */
@@ -265,7 +260,7 @@ static u32 extract_speed(u16 linkstat)
 	case PCI_EXP_LNKSTA_CLS_5_0GB:
 		speed = 5000; /* Gen 2, 5GHz */
 		break;
-	case GEN3_SPEED_VECTOR:
+	case PCI_EXP_LNKSTA_CLS_8_0GB:
 		speed = 8000; /* Gen 3, 8GHz */
 		break;
 	}
@@ -320,7 +315,7 @@ int pcie_speeds(struct hfi1_devdata *dd)
 		return ret;
 	}
 
-	if ((linkcap & PCI_EXP_LNKCAP_SLS) != GEN3_SPEED_VECTOR) {
+	if ((linkcap & PCI_EXP_LNKCAP_SLS) != PCI_EXP_LNKCAP_SLS_8_0GB) {
 		dd_dev_info(dd,
 			    "This HFI is not Gen3 capable, max speed 0x%x, need 0x3\n",
 			    linkcap & PCI_EXP_LNKCAP_SLS);
@@ -697,9 +692,6 @@ const struct pci_error_handlers hfi1_pci_err_handler = {
 /* gasket block secondary bus reset delay */
 #define SBR_DELAY_US 200000	/* 200ms */
 
-/* mask for PCIe capability register lnkctl2 target link speed */
-#define LNKCTL2_TARGET_LINK_SPEED_MASK 0xf
-
 static uint pcie_target = 3;
 module_param(pcie_target, uint, S_IRUGO);
 MODULE_PARM_DESC(pcie_target, "PCIe target speed (0 skip, 1-3 Gen1-3)");
@@ -1048,13 +1040,13 @@ int do_pcie_gen3_transition(struct hfi1_devdata *dd)
 		return 0;
 
 	if (pcie_target == 1) {			/* target Gen1 */
-		target_vector = GEN1_SPEED_VECTOR;
+		target_vector = PCI_EXP_LNKCTL2_TLS_2_5GB;
 		target_speed = 2500;
 	} else if (pcie_target == 2) {		/* target Gen2 */
-		target_vector = GEN2_SPEED_VECTOR;
+		target_vector = PCI_EXP_LNKCTL2_TLS_5_0GB;
 		target_speed = 5000;
 	} else if (pcie_target == 3) {		/* target Gen3 */
-		target_vector = GEN3_SPEED_VECTOR;
+		target_vector = PCI_EXP_LNKCTL2_TLS_8_0GB;
 		target_speed = 8000;
 	} else {
 		/* off or invalid target - skip */
@@ -1293,8 +1285,8 @@ int do_pcie_gen3_transition(struct hfi1_devdata *dd)
 	dd_dev_info(dd, "%s: ..old link control2: 0x%x\n", __func__,
 		    (u32)lnkctl2);
 	/* only write to parent if target is not as high as ours */
-	if ((lnkctl2 & LNKCTL2_TARGET_LINK_SPEED_MASK) < target_vector) {
-		lnkctl2 &= ~LNKCTL2_TARGET_LINK_SPEED_MASK;
+	if ((lnkctl2 & PCI_EXP_LNKCTL2_TLS) < target_vector) {
+		lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS;
 		lnkctl2 |= target_vector;
 		dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__,
 			    (u32)lnkctl2);
@@ -1319,7 +1311,7 @@ int do_pcie_gen3_transition(struct hfi1_devdata *dd)
 
 	dd_dev_info(dd, "%s: ..old link control2: 0x%x\n", __func__,
 		    (u32)lnkctl2);
-	lnkctl2 &= ~LNKCTL2_TARGET_LINK_SPEED_MASK;
+	lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS;
 	lnkctl2 |= target_vector;
 	dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__,
 		    (u32)lnkctl2);
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration
  2018-04-17  0:28 [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration Frederick Lawler
  2018-04-17  0:28 ` [PATCH v3 1/2] PCI: Add PCI_EXP_LNKCTL2_TLS* macros Frederick Lawler
  2018-04-17  0:28 ` [PATCH v3 2/2] IB/hfi1: Replace custom hfi1 macros with PCIe macros Frederick Lawler
@ 2018-04-17  6:53 ` Christoph Hellwig
  2018-04-17 14:32   ` Bjorn Helgaas
  2018-04-17 15:56   ` Marciniszyn, Mike
  2018-04-27 16:14 ` Doug Ledford
  2018-04-27 17:58 ` Bjorn Helgaas
  4 siblings, 2 replies; 13+ messages in thread
From: Christoph Hellwig @ 2018-04-17  6:53 UTC (permalink / raw)
  To: Frederick Lawler
  Cc: bhelgaas, mike.marciniszyn, dennis.dalessandro, dledford, jgg,
	linux-pci, linux-rdma

On Mon, Apr 16, 2018 at 07:28:23PM -0500, Frederick Lawler wrote:
> The IB/hfi1 driver uses custom macros to configure the target link speed. Some 
> macros were previously replaced, but not fully. This patch series addresses the
> configuration inconsistencies by adding PCI_EXP_LNKCTL2_TLS* macros to PCI,
> and then use them in the following IB/hfi1 patch.

Btw, why is the driver configuring the PCIe link speed?  Isn't this
something we should be handling in the PCI core?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration
  2018-04-17  6:53 ` [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration Christoph Hellwig
@ 2018-04-17 14:32   ` Bjorn Helgaas
  2018-04-18 21:53     ` Dennis Dalessandro
  2018-04-17 15:56   ` Marciniszyn, Mike
  1 sibling, 1 reply; 13+ messages in thread
From: Bjorn Helgaas @ 2018-04-17 14:32 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Frederick Lawler, bhelgaas, mike.marciniszyn, dennis.dalessandro,
	dledford, jgg, linux-pci, linux-rdma, linux-kernel,
	Bartlomiej Dudek

[+cc Bartlomiej, who recently changed do_pcie_gen3_transition(), LKML]

On Mon, Apr 16, 2018 at 11:53:43PM -0700, Christoph Hellwig wrote:
> On Mon, Apr 16, 2018 at 07:28:23PM -0500, Frederick Lawler wrote:
> > The IB/hfi1 driver uses custom macros to configure the target link speed. Some 
> > macros were previously replaced, but not fully. This patch series addresses the
> > configuration inconsistencies by adding PCI_EXP_LNKCTL2_TLS* macros to PCI,
> > and then use them in the following IB/hfi1 patch.
> 
> Btw, why is the driver configuring the PCIe link speed?  Isn't this
> something we should be handling in the PCI core?

Good question.  I think this sort of code definitely should not be in
drivers unless it's to work around some kind of defect in the device.

I think the intent of the spec is that neither the OS nor the driver
has to deal with link training and the link should train to the
highest speed supported by both ends.  For example, PCIe r4.0, sec 1.2
says

  Initialization – During hardware initialization, each PCI Express
  Link is set up following a negotiation of Lane widths and frequency
  of operation by the two agents at each end of the Link. No firmware
  or operating system software is involved.

Maybe the Intel guys can comment on why HFI needs this code.
Presumably they didn't write it just for fun, so I assume there's
some reason.

Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration
  2018-04-17  6:53 ` [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration Christoph Hellwig
  2018-04-17 14:32   ` Bjorn Helgaas
@ 2018-04-17 15:56   ` Marciniszyn, Mike
  2018-04-17 17:19     ` Bjorn Helgaas
  1 sibling, 1 reply; 13+ messages in thread
From: Marciniszyn, Mike @ 2018-04-17 15:56 UTC (permalink / raw)
  To: Christoph Hellwig, Frederick Lawler
  Cc: bhelgaas, Dalessandro, Dennis, dledford, jgg, linux-pci, linux-rdma

> 
> Btw, why is the driver configuring the PCIe link speed?  Isn't this
> something we should be handling in the PCI core?

The device comes out of reset at the 5GT/s speed. The driver downloads device firmware, programs PCIe registers, and co-ordinates the transition to 8GT/s.

This recipe is device specific and is therefore implemented in the hfi1 driver built on top of PCI core functions and macros.

Mike

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration
  2018-04-17 15:56   ` Marciniszyn, Mike
@ 2018-04-17 17:19     ` Bjorn Helgaas
  2018-04-20 17:03       ` Dennis Dalessandro
  0 siblings, 1 reply; 13+ messages in thread
From: Bjorn Helgaas @ 2018-04-17 17:19 UTC (permalink / raw)
  To: Marciniszyn, Mike
  Cc: Christoph Hellwig, Frederick Lawler, bhelgaas, Dalessandro,
	Dennis, dledford, jgg, linux-pci, linux-rdma

On Tue, Apr 17, 2018 at 03:56:13PM +0000, Marciniszyn, Mike wrote:
> > 
> > Btw, why is the driver configuring the PCIe link speed?  Isn't this
> > something we should be handling in the PCI core?
> 
> The device comes out of reset at the 5GT/s speed. The driver
> downloads device firmware, programs PCIe registers, and co-ordinates
> the transition to 8GT/s.
> 
> This recipe is device specific and is therefore implemented in the
> hfi1 driver built on top of PCI core functions and macros.

Do you think this behavior conforms to the spec, or is this a
workaround for a hardware erratum?

I don't think it's feasible to have every driver deal with this level
of PCIe detail.  Do you have to do something similar in the Windows
driver?

Is there something we can do in the PCI core to do the
reconfiguration?  If you can't go to 8GT/s before doing some
device-specific initialization, could the driver do that setup and
then use a generic PCI core interface to reconfigure the link?

Or maybe if the driver finds the device at 5GT/s, it could download
the firmware and reset the device.  Would the device then negotiate at
8GT/s?

Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration
  2018-04-17 14:32   ` Bjorn Helgaas
@ 2018-04-18 21:53     ` Dennis Dalessandro
  0 siblings, 0 replies; 13+ messages in thread
From: Dennis Dalessandro @ 2018-04-18 21:53 UTC (permalink / raw)
  To: Bjorn Helgaas, Christoph Hellwig
  Cc: Frederick Lawler, bhelgaas, mike.marciniszyn, dledford, jgg,
	linux-pci, linux-rdma, linux-kernel, Bartlomiej Dudek

On 4/17/2018 10:32 AM, Bjorn Helgaas wrote:
> [+cc Bartlomiej, who recently changed do_pcie_gen3_transition(), LKML]
> 
> On Mon, Apr 16, 2018 at 11:53:43PM -0700, Christoph Hellwig wrote:
>> On Mon, Apr 16, 2018 at 07:28:23PM -0500, Frederick Lawler wrote:
>>> The IB/hfi1 driver uses custom macros to configure the target link speed. Some
>>> macros were previously replaced, but not fully. This patch series addresses the
>>> configuration inconsistencies by adding PCI_EXP_LNKCTL2_TLS* macros to PCI,
>>> and then use them in the following IB/hfi1 patch.
>>
>> Btw, why is the driver configuring the PCIe link speed?  Isn't this
>> something we should be handling in the PCI core?
> 
> Good question.  I think this sort of code definitely should not be in
> drivers unless it's to work around some kind of defect in the device.
> 
> I think the intent of the spec is that neither the OS nor the driver
> has to deal with link training and the link should train to the
> highest speed supported by both ends.  For example, PCIe r4.0, sec 1.2
> says
> 
>    Initialization – During hardware initialization, each PCI Express
>    Link is set up following a negotiation of Lane widths and frequency
>    of operation by the two agents at each end of the Link. No firmware
>    or operating system software is involved.
> 
> Maybe the Intel guys can comment on why HFI needs this code.
> Presumably they didn't write it just for fun, so I assume there's
> some reason.

Yeah there was a reason. It just escapes me what it was right now. Let 
me find out and I'll update.

-Denny

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration
  2018-04-17 17:19     ` Bjorn Helgaas
@ 2018-04-20 17:03       ` Dennis Dalessandro
  0 siblings, 0 replies; 13+ messages in thread
From: Dennis Dalessandro @ 2018-04-20 17:03 UTC (permalink / raw)
  To: Bjorn Helgaas, Marciniszyn, Mike
  Cc: Christoph Hellwig, Frederick Lawler, bhelgaas, dledford, jgg,
	linux-pci, linux-rdma

On 4/17/2018 1:19 PM, Bjorn Helgaas wrote:
> On Tue, Apr 17, 2018 at 03:56:13PM +0000, Marciniszyn, Mike wrote:
>>>
>>> Btw, why is the driver configuring the PCIe link speed?  Isn't this
>>> something we should be handling in the PCI core?
>>
>> The device comes out of reset at the 5GT/s speed. The driver
>> downloads device firmware, programs PCIe registers, and co-ordinates
>> the transition to 8GT/s.
>>
>> This recipe is device specific and is therefore implemented in the
>> hfi1 driver built on top of PCI core functions and macros.
> 
> Do you think this behavior conforms to the spec, or is this a
> workaround for a hardware erratum?

Can't speculate as to why, but this is just the way this hardware works.

> I don't think it's feasible to have every driver deal with this level
> of PCIe detail.  Do you have to do something similar in the Windows
> driver?

No Windows driver.

> Is there something we can do in the PCI core to do the
> reconfiguration?  If you can't go to 8GT/s before doing some
> device-specific initialization, could the driver do that setup and
> then use a generic PCI core interface to reconfigure the link?
> 
> Or maybe if the driver finds the device at 5GT/s, it could download
> the firmware and reset the device.  Would the device then negotiate at
> 8GT/s?

Yes. In fact this is what we should be doing. We use the SBR to trigger 
renegotiation of the link to 8GT/s.

-Denny

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v3 2/2] IB/hfi1: Replace custom hfi1 macros with PCIe macros
  2018-04-17  0:28 ` [PATCH v3 2/2] IB/hfi1: Replace custom hfi1 macros with PCIe macros Frederick Lawler
@ 2018-04-20 18:21   ` Ruhl, Michael J
  0 siblings, 0 replies; 13+ messages in thread
From: Ruhl, Michael J @ 2018-04-20 18:21 UTC (permalink / raw)
  To: Frederick Lawler, bhelgaas, Marciniszyn, Mike, Dalessandro,
	Dennis, dledford, jgg
  Cc: linux-pci, linux-rdma

>-----Original Message-----
>From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
>owner@vger.kernel.org] On Behalf Of Frederick Lawler
>Sent: Monday, April 16, 2018 8:28 PM
>To: bhelgaas@google.com; Marciniszyn, Mike <mike.marciniszyn@intel.com>;
>Dalessandro, Dennis <dennis.dalessandro@intel.com>;
>dledford@redhat.com; jgg@mellanox.com
>Cc: linux-pci@vger.kernel.org; linux-rdma@vger.kernel.org; Frederick Lawler
><fred@fredlawl.com>
>Subject: [PATCH v3 2/2] IB/hfi1: Replace custom hfi1 macros with PCIe macros
>
>IB/hfi1 contains custom macros for PCIe link configuration. Remove the
>custom macros in favor of the PCIe link macros. No functional change
>intended.
>
>Signed-off-by: Frederick Lawler <fred@fredlawl.com>

This looks good.

Thanks!

Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>


>--
>V3: Use PCI_EXP_LNKCTL2_TLS* macros
>V2: s/LINK/LNK
>---
> drivers/infiniband/hw/hfi1/pcie.c | 24 ++++++++----------------
> 1 file changed, 8 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/infiniband/hw/hfi1/pcie.c
>b/drivers/infiniband/hw/hfi1/pcie.c
>index 83d66e862207..fe2a6d602856 100644
>--- a/drivers/infiniband/hw/hfi1/pcie.c
>+++ b/drivers/infiniband/hw/hfi1/pcie.c
>@@ -56,11 +56,6 @@
> #include "chip_registers.h"
> #include "aspm.h"
>
>-/* link speed vector for Gen3 speed - not in Linux headers */
>-#define GEN1_SPEED_VECTOR 0x1
>-#define GEN2_SPEED_VECTOR 0x2
>-#define GEN3_SPEED_VECTOR 0x3
>-
> /*
>  * This file contains PCIe utility routines.
>  */
>@@ -265,7 +260,7 @@ static u32 extract_speed(u16 linkstat)
> 	case PCI_EXP_LNKSTA_CLS_5_0GB:
> 		speed = 5000; /* Gen 2, 5GHz */
> 		break;
>-	case GEN3_SPEED_VECTOR:
>+	case PCI_EXP_LNKSTA_CLS_8_0GB:
> 		speed = 8000; /* Gen 3, 8GHz */
> 		break;
> 	}
>@@ -320,7 +315,7 @@ int pcie_speeds(struct hfi1_devdata *dd)
> 		return ret;
> 	}
>
>-	if ((linkcap & PCI_EXP_LNKCAP_SLS) != GEN3_SPEED_VECTOR) {
>+	if ((linkcap & PCI_EXP_LNKCAP_SLS) != PCI_EXP_LNKCAP_SLS_8_0GB)
>{
> 		dd_dev_info(dd,
> 			    "This HFI is not Gen3 capable, max speed 0x%x,
>need 0x3\n",
> 			    linkcap & PCI_EXP_LNKCAP_SLS);
>@@ -697,9 +692,6 @@ const struct pci_error_handlers hfi1_pci_err_handler =
>{
> /* gasket block secondary bus reset delay */
> #define SBR_DELAY_US 200000	/* 200ms */
>
>-/* mask for PCIe capability register lnkctl2 target link speed */
>-#define LNKCTL2_TARGET_LINK_SPEED_MASK 0xf
>-
> static uint pcie_target = 3;
> module_param(pcie_target, uint, S_IRUGO);
> MODULE_PARM_DESC(pcie_target, "PCIe target speed (0 skip, 1-3 Gen1-3)");
>@@ -1048,13 +1040,13 @@ int do_pcie_gen3_transition(struct hfi1_devdata
>*dd)
> 		return 0;
>
> 	if (pcie_target == 1) {			/* target Gen1 */
>-		target_vector = GEN1_SPEED_VECTOR;
>+		target_vector = PCI_EXP_LNKCTL2_TLS_2_5GB;
> 		target_speed = 2500;
> 	} else if (pcie_target == 2) {		/* target Gen2 */
>-		target_vector = GEN2_SPEED_VECTOR;
>+		target_vector = PCI_EXP_LNKCTL2_TLS_5_0GB;
> 		target_speed = 5000;
> 	} else if (pcie_target == 3) {		/* target Gen3 */
>-		target_vector = GEN3_SPEED_VECTOR;
>+		target_vector = PCI_EXP_LNKCTL2_TLS_8_0GB;
> 		target_speed = 8000;
> 	} else {
> 		/* off or invalid target - skip */
>@@ -1293,8 +1285,8 @@ int do_pcie_gen3_transition(struct hfi1_devdata
>*dd)
> 	dd_dev_info(dd, "%s: ..old link control2: 0x%x\n", __func__,
> 		    (u32)lnkctl2);
> 	/* only write to parent if target is not as high as ours */
>-	if ((lnkctl2 & LNKCTL2_TARGET_LINK_SPEED_MASK) < target_vector) {
>-		lnkctl2 &= ~LNKCTL2_TARGET_LINK_SPEED_MASK;
>+	if ((lnkctl2 & PCI_EXP_LNKCTL2_TLS) < target_vector) {
>+		lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS;
> 		lnkctl2 |= target_vector;
> 		dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__,
> 			    (u32)lnkctl2);
>@@ -1319,7 +1311,7 @@ int do_pcie_gen3_transition(struct hfi1_devdata
>*dd)
>
> 	dd_dev_info(dd, "%s: ..old link control2: 0x%x\n", __func__,
> 		    (u32)lnkctl2);
>-	lnkctl2 &= ~LNKCTL2_TARGET_LINK_SPEED_MASK;
>+	lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS;
> 	lnkctl2 |= target_vector;
> 	dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__,
> 		    (u32)lnkctl2);
>--
>2.17.0
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration
  2018-04-17  0:28 [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration Frederick Lawler
                   ` (2 preceding siblings ...)
  2018-04-17  6:53 ` [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration Christoph Hellwig
@ 2018-04-27 16:14 ` Doug Ledford
  2018-04-27 17:58 ` Bjorn Helgaas
  4 siblings, 0 replies; 13+ messages in thread
From: Doug Ledford @ 2018-04-27 16:14 UTC (permalink / raw)
  To: Frederick Lawler, bhelgaas, mike.marciniszyn, dennis.dalessandro, jgg
  Cc: linux-pci, linux-rdma

[-- Attachment #1: Type: text/plain, Size: 1497 bytes --]

On Mon, 2018-04-16 at 19:28 -0500, Frederick Lawler wrote:
> The IB/hfi1 driver uses custom macros to configure the target link speed. Some 
> macros were previously replaced, but not fully. This patch series addresses the
> configuration inconsistencies by adding PCI_EXP_LNKCTL2_TLS* macros to PCI,
> and then use them in the following IB/hfi1 patch.
> 
> V3: 
> 	* PCI: Add PCI_EXP_LNKCTL2_TLS_* macros
> 	* Drop patch to use extract_speed() in do_pcie_gen3_transition()
> V2:
> 	* s/LINK/LNK
> 
> Frederick Lawler (2):
>   PCI: Add PCI_EXP_LNKCTL2_TLS* macros
>   IB/hfi1: Replace custom hfi1 macros with PCIe macros
> 
>  drivers/infiniband/hw/hfi1/pcie.c | 24 ++++++++----------------
>  include/uapi/linux/pci_regs.h     |  5 +++++
>  2 files changed, 13 insertions(+), 16 deletions(-)
> 

Where do we stand on this series?  People talked about what the core
might be able to do, or how the driver might be able to do things a
little differently, but I didn't hear a clear conclusion on whether or
not to take this series as is (it seems to be a sensible cleanup and
possibly worthwhile even if we end up changing the hfi1 driver to do a
firmware updated followed by a device reset to get the same results). 
And more importantly as far as I'm concerned, is this sitting on my
plate or on the PCI people's plate?

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration
  2018-04-17  0:28 [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration Frederick Lawler
                   ` (3 preceding siblings ...)
  2018-04-27 16:14 ` Doug Ledford
@ 2018-04-27 17:58 ` Bjorn Helgaas
  2018-04-27 18:18   ` Doug Ledford
  4 siblings, 1 reply; 13+ messages in thread
From: Bjorn Helgaas @ 2018-04-27 17:58 UTC (permalink / raw)
  To: Frederick Lawler
  Cc: bhelgaas, mike.marciniszyn, dennis.dalessandro, dledford, jgg,
	linux-pci, linux-rdma

On Mon, Apr 16, 2018 at 07:28:23PM -0500, Frederick Lawler wrote:
> The IB/hfi1 driver uses custom macros to configure the target link speed. Some 
> macros were previously replaced, but not fully. This patch series addresses the
> configuration inconsistencies by adding PCI_EXP_LNKCTL2_TLS* macros to PCI,
> and then use them in the following IB/hfi1 patch.
> 
> V3: 
> 	* PCI: Add PCI_EXP_LNKCTL2_TLS_* macros
> 	* Drop patch to use extract_speed() in do_pcie_gen3_transition()
> V2:
> 	* s/LINK/LNK
> 
> Frederick Lawler (2):
>   PCI: Add PCI_EXP_LNKCTL2_TLS* macros
>   IB/hfi1: Replace custom hfi1 macros with PCIe macros
> 
>  drivers/infiniband/hw/hfi1/pcie.c | 24 ++++++++----------------
>  include/uapi/linux/pci_regs.h     |  5 +++++
>  2 files changed, 13 insertions(+), 16 deletions(-)

I applied these, with Michael's reviewed-by on the second, to
pci/misc for v4.18, thanks!

Ideally we could pull some of this link speed management code into the
PCI core someday, but I think these patches are useful as-is.

Oh, I did s/PCI_EXP_LNKCTL2_TLS_2_5GB/PCI_EXP_LNKCTL2_TLS_2_5GT/
because the link speeds are actually defined in terms of GT/s, not
GB/s.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration
  2018-04-27 17:58 ` Bjorn Helgaas
@ 2018-04-27 18:18   ` Doug Ledford
  0 siblings, 0 replies; 13+ messages in thread
From: Doug Ledford @ 2018-04-27 18:18 UTC (permalink / raw)
  To: Bjorn Helgaas, Frederick Lawler
  Cc: bhelgaas, mike.marciniszyn, dennis.dalessandro, jgg, linux-pci,
	linux-rdma

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

On Fri, 2018-04-27 at 12:58 -0500, Bjorn Helgaas wrote:
> On Mon, Apr 16, 2018 at 07:28:23PM -0500, Frederick Lawler wrote:
> > The IB/hfi1 driver uses custom macros to configure the target link speed. Some 
> > macros were previously replaced, but not fully. This patch series addresses the
> > configuration inconsistencies by adding PCI_EXP_LNKCTL2_TLS* macros to PCI,
> > and then use them in the following IB/hfi1 patch.
> > 
> > V3: 
> > 	* PCI: Add PCI_EXP_LNKCTL2_TLS_* macros
> > 	* Drop patch to use extract_speed() in do_pcie_gen3_transition()
> > V2:
> > 	* s/LINK/LNK
> > 
> > Frederick Lawler (2):
> >   PCI: Add PCI_EXP_LNKCTL2_TLS* macros
> >   IB/hfi1: Replace custom hfi1 macros with PCIe macros
> > 
> >  drivers/infiniband/hw/hfi1/pcie.c | 24 ++++++++----------------
> >  include/uapi/linux/pci_regs.h     |  5 +++++
> >  2 files changed, 13 insertions(+), 16 deletions(-)
> 
> I applied these, with Michael's reviewed-by on the second, to
> pci/misc for v4.18, thanks!
> 
> Ideally we could pull some of this link speed management code into the
> PCI core someday, but I think these patches are useful as-is.
> 
> Oh, I did s/PCI_EXP_LNKCTL2_TLS_2_5GB/PCI_EXP_LNKCTL2_TLS_2_5GT/
> because the link speeds are actually defined in terms of GT/s, not
> GB/s.

Thanks :-)

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-04-27 18:18 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-17  0:28 [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration Frederick Lawler
2018-04-17  0:28 ` [PATCH v3 1/2] PCI: Add PCI_EXP_LNKCTL2_TLS* macros Frederick Lawler
2018-04-17  0:28 ` [PATCH v3 2/2] IB/hfi1: Replace custom hfi1 macros with PCIe macros Frederick Lawler
2018-04-20 18:21   ` Ruhl, Michael J
2018-04-17  6:53 ` [PATCH v3 0/2] IB/hfi1: Cleanup PCIe link configuration Christoph Hellwig
2018-04-17 14:32   ` Bjorn Helgaas
2018-04-18 21:53     ` Dennis Dalessandro
2018-04-17 15:56   ` Marciniszyn, Mike
2018-04-17 17:19     ` Bjorn Helgaas
2018-04-20 17:03       ` Dennis Dalessandro
2018-04-27 16:14 ` Doug Ledford
2018-04-27 17:58 ` Bjorn Helgaas
2018-04-27 18:18   ` Doug Ledford

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.