diff mbox

staging/rdma/hfi1: set Gen3 half-swing for integrated devices

Message ID 1446689168-21620-1-git-send-email-ira.weiny@intel.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Ira Weiny Nov. 5, 2015, 2:06 a.m. UTC 
From: Dean Luick <dean.luick@intel.com>

Correctly set half-swing for integrated devices.  A0 needs all fields set for
CcePcieCtrl.  B0 and later only need a few fields set.

Reviewed-by: Stuart Summers <john.s.summers@intel.com>
Signed-off-by: Dean Luick <dean.luick@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/staging/rdma/hfi1/chip_registers.h | 11 +++++
 drivers/staging/rdma/hfi1/pcie.c           | 74 ++++++++++++++++++++++++++++--
 2 files changed, 81 insertions(+), 4 deletions(-)

Comments

Dan Carpenter Nov. 5, 2015, 7:34 a.m. UTC | #1 
On Wed, Nov 04, 2015 at 09:06:08PM -0500, ira.weiny@intel.com wrote:

> +#define CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_MASK 0x1ull

There is absolutely no point to having a variable name which is nine
bajillion characters long since we are not able to use it and have to
instead use macro majic to hide the first twelve zillion characters.  If
we can't even see the name when it is used, then why does it exist? Also
macro magic breaks grep and cscope.  Just think of a better name to
begin with and get rid of the PC macro.

regards,
dan carpenter

--
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
Ira Weiny Nov. 5, 2015, 4:41 p.m. UTC | #2 
On Thu, Nov 05, 2015 at 10:34:48AM +0300, Dan Carpenter wrote:
> On Wed, Nov 04, 2015 at 09:06:08PM -0500, ira.weiny@intel.com wrote:
> 
> > +#define CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_MASK 0x1ull
> 
> There is absolutely no point to having a variable name which is nine
> bajillion characters long since we are not able to use it and have to
> instead use macro majic to hide the first twelve zillion characters.
>

The long names are directly from the hardware specification.

>
> If
> we can't even see the name when it is used, then why does it exist? Also
> macro magic breaks grep and cscope.  Just think of a better name to
> begin with and get rid of the PC macro.

We did not want the PC macro but using the "real" names caused checkpatch
failures.  Indeed using the PC macro reduces the readability of the code in
terms of following the hardware spec because we lose the "real" name.

Is this an example where we should ignore the line lengths of checkpatch to
preserve the readability of the code?

Thanks,
Ira

--
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
Dan Carpenter Nov. 5, 2015, 6:42 p.m. UTC | #3 
On Thu, Nov 05, 2015 at 11:41:23AM -0500, ira.weiny wrote:
> Is this an example where we should ignore the line lengths of checkpatch to
> preserve the readability of the code?

Honestly, you can't fight checkpatch, especially in staging.  Rejecting
patches is a drain on your emotions and your energy.  We accept all
checkpatch fixes if they are half way decent.

Maybe just put a comment in the header about the full hardware name?

This patch breaks cscope and we don't end up actually using the hardware
name in the end...

regards,
dan carpenter
--
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
diff mbox

Patch

diff --git a/drivers/staging/rdma/hfi1/chip_registers.h b/drivers/staging/rdma/hfi1/chip_registers.h
index bf45de29d8bd..d0deb2278635 100644
--- a/drivers/staging/rdma/hfi1/chip_registers.h
+++ b/drivers/staging/rdma/hfi1/chip_registers.h
@@ -549,6 +549,17 @@ 
 #define CCE_MSIX_TABLE_UPPER (CCE + 0x000000100008)
 #define CCE_MSIX_TABLE_UPPER_RESETCSR 0x0000000100000000ull
 #define CCE_MSIX_VEC_CLR_WITHOUT_INT (CCE + 0x000000110400)
+#define CCE_PCIE_CTRL (CCE + 0x0000000000C0)
+#define CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_MASK 0x3ull
+#define CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_SHIFT 0
+#define CCE_PCIE_CTRL_PCIE_LANE_DELAY_MASK 0xFull
+#define CCE_PCIE_CTRL_PCIE_LANE_DELAY_SHIFT 2
+#define CCE_PCIE_CTRL_XMT_MARGIN_OVERWRITE_ENABLE_SHIFT 8
+#define CCE_PCIE_CTRL_XMT_MARGIN_SHIFT 9
+#define CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_MASK 0x1ull
+#define CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_SHIFT 12
+#define CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_MASK 0x7ull
+#define CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_SHIFT 13
 #define CCE_REVISION (CCE + 0x000000000000)
 #define CCE_REVISION2 (CCE + 0x000000000008)
 #define CCE_REVISION2_HFI_ID_MASK 0x1ull
diff --git a/drivers/staging/rdma/hfi1/pcie.c b/drivers/staging/rdma/hfi1/pcie.c
index a956044459a2..000f28f4b501 100644
--- a/drivers/staging/rdma/hfi1/pcie.c
+++ b/drivers/staging/rdma/hfi1/pcie.c
@@ -864,6 +864,75 @@  static void arm_gasket_logic(struct hfi1_devdata *dd)
 	read_csr(dd, ASIC_PCIE_SD_HOST_CMD);
 }
 
+/* CcePcieCtrl long name helper */
+#define PC(field) (CCE_PCIE_CTRL_##field)
+
+ /*
+  * Write xmt_margin for full-swing (WFR-B) or half-swing (WFR-C).
+  */
+static void write_xmt_margin(struct hfi1_devdata *dd, const char *fname)
+{
+	u64 pcie_ctrl;
+	u64 xmt_margin;
+	u64 xmt_margin_oe;
+	u64 lane_delay;
+	u64 lane_bundle;
+
+	pcie_ctrl = read_csr(dd, CCE_PCIE_CTRL);
+
+	/*
+	 * For Discrete, use full-swing.
+	 *  - PCIe TX defaults to full-swing.
+	 *    Leave this register as default.
+	 * For Integrated, use half-swing
+	 *  - Copy xmt_margin and xmt_margin_oe
+	 *    from Gen1/Gen2 to Gen3.
+	 */
+	if (dd->pcidev->device == PCI_DEVICE_ID_INTEL1) { /* integrated */
+		/* extract initial fields */
+		xmt_margin = (pcie_ctrl >> PC(XMT_MARGIN_GEN1_GEN2_SHIFT))
+			    & PC(XMT_MARGIN_GEN1_GEN2_MASK);
+		xmt_margin_oe =
+			(pcie_ctrl
+			  >> PC(XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_SHIFT))
+			& PC(XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_MASK);
+		lane_delay = (pcie_ctrl >> PC(PCIE_LANE_DELAY_SHIFT))
+			    & PC(PCIE_LANE_DELAY_MASK);
+		lane_bundle = (pcie_ctrl >> PC(PCIE_LANE_BUNDLE_SHIFT))
+			    & PC(PCIE_LANE_BUNDLE_MASK);
+
+		/*
+		 * For A0, EFUSE values are not set.  Override with the
+		 * correct values.
+		 */
+		if (is_a0(dd)) {
+			/*
+			 * xmt_margin and OverwiteEnabel should be the
+			 * same for Gen1/Gen2 and Gen3
+			 */
+			xmt_margin                = 0x5;
+			xmt_margin_oe = 0x1;
+			lane_delay                = 0xF; /* Delay 240ns. */
+			lane_bundle               = 0x0; /* Set to 1 lane. */
+		}
+
+		/* overwrite existing values */
+		pcie_ctrl = (xmt_margin << PC(XMT_MARGIN_GEN1_GEN2_SHIFT))
+			| (xmt_margin_oe <<
+				PC(XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_SHIFT))
+			| (xmt_margin << PC(XMT_MARGIN_SHIFT))
+			| (xmt_margin_oe <<
+				PC(XMT_MARGIN_OVERWRITE_ENABLE_SHIFT))
+			| (lane_delay << PC(PCIE_LANE_DELAY_SHIFT))
+			| (lane_bundle << PC(PCIE_LANE_BUNDLE_SHIFT));
+
+		write_csr(dd, CCE_PCIE_CTRL, pcie_ctrl);
+	}
+
+	dd_dev_info(dd, "%s: program XMT margin, CcePcieCtrl 0x%llx\n",
+		    fname, pcie_ctrl);
+}
+
 /*
  * Do all the steps needed to transition the PCIe link to Gen3 speed.
  */
@@ -1072,11 +1141,8 @@  retry:
 
 	/*
 	 * step 5d: program XMT margin
-	 * Right now, leave the default alone.  To change, do a
-	 * read-modify-write of:
-	 *	CcePcieCtrl.XmtMargin
-	 *	CcePcieCtrl.XmitMarginOverwriteEnable
 	 */
+	write_xmt_margin(dd, __func__);
 
 	/* step 5e: disable active state power management (ASPM) */
 	dd_dev_info(dd, "%s: clearing ASPM\n", __func__);