* [PATCH v2] staging/rdma/hfi1: set Gen3 half-swing for integrated devices
@ 2015-11-09 23:38 ira.weiny-ral2JQCrhuEAvxtiuMwx3w
2015-11-10 9:59 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: ira.weiny-ral2JQCrhuEAvxtiuMwx3w @ 2015-11-09 23:38 UTC (permalink / raw)
To: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b
Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w, Dean Luick, Ira Weiny
From: Dean Luick <dean.luick-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Dean Luick <dean.luick-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
Changes from V1:
Add comments concerning the very long names.
drivers/staging/rdma/hfi1/chip_registers.h | 16 +++++++
drivers/staging/rdma/hfi1/pcie.c | 77 ++++++++++++++++++++++++++++--
2 files changed, 89 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/rdma/hfi1/chip_registers.h b/drivers/staging/rdma/hfi1/chip_registers.h
index bf45de29d8bd..e4c1953203dd 100644
--- a/drivers/staging/rdma/hfi1/chip_registers.h
+++ b/drivers/staging/rdma/hfi1/chip_registers.h
@@ -51,6 +51,11 @@
*
*/
+/*
+ * NOTE: The names in this file are very long but they are meant to track the
+ * hardware specification names.
+ */
+
#define CORE 0x000000000000
#define CCE (CORE + 0x000000000000)
#define ASIC (CORE + 0x000000400000)
@@ -549,6 +554,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..fe17332e9239 100644
--- a/drivers/staging/rdma/hfi1/pcie.c
+++ b/drivers/staging/rdma/hfi1/pcie.c
@@ -865,6 +865,78 @@ static void arm_gasket_logic(struct hfi1_devdata *dd)
}
/*
+ * CcePcieCtrl long name helper
+ * Due to checkpatch checks we use this macro to shorten field macros names
+ */
+#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.
*/
int do_pcie_gen3_transition(struct hfi1_devdata *dd)
@@ -1072,11 +1144,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__);
--
1.8.2
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] staging/rdma/hfi1: set Gen3 half-swing for integrated devices
2015-11-09 23:38 [PATCH v2] staging/rdma/hfi1: set Gen3 half-swing for integrated devices ira.weiny-ral2JQCrhuEAvxtiuMwx3w
@ 2015-11-10 9:59 ` Dan Carpenter
2015-11-10 23:51 ` ira.weiny
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2015-11-10 9:59 UTC (permalink / raw)
To: ira.weiny; +Cc: devel, gregkh, dledford, Dean Luick, linux-rdma
Gar... No. Please please get rid of the PC() macro. It makes the code
impossible to understand because instead of hitting CTRL-[ you have
decode it and then manually type out
:cs find g CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_SHIFT
which is the length of a typical college essay. I meant just put a
comment like this:
/*
* In the hardware spec these are prefixed with:
* CCE_PCIE_CTRL_...
* But it is too long to use in code.
*/
#define XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_MASK 0x1ull
Or probably even better:
#define CCE_PCIE_CTRL (CCE + 0x0000000000C0)
#define LANE_BUNDLE_MASK 0x3ull /* CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_MASK */
#define LANE_BUNDLE_SHIFT 0 /* CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_SHIFT */
#define LANE_DELAY_MASK 0xFull /* CCE_PCIE_CTRL_PCIE_LANE_DELAY_MASK */
#define LANE_DELAY_SHIFT 2 /* CCE_PCIE_CTRL_PCIE_LANE_DELAY_SHIFT */
#define MARGIN_OVERWRITE_SHIFT 8 /* CCE_PCIE_CTRL_XMT_MARGIN_OVERWRITE_ENABLE_SHIFT */
#define MARGIN_SHIFT 9 /* CCE_PCIE_CTRL_XMT_MARGIN_SHIFT */
#define MARGIN_G1G2_OVERWRITE_MASK 0x1ull /* CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_MASK */
#define MARGIN_G1G2_OVERWRITE_SHIFT 12 /* CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_SHIFT */
#define MARGIN_G1G2_MASK 0x7ull /* CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_MASK */
#define MARGIN_G1G2_SHIFT 13 /* CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_SHIFT */
Those lines go over the 80 character limit but it's fine.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] staging/rdma/hfi1: set Gen3 half-swing for integrated devices
2015-11-10 9:59 ` Dan Carpenter
@ 2015-11-10 23:51 ` ira.weiny
2015-11-11 7:37 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: ira.weiny @ 2015-11-10 23:51 UTC (permalink / raw)
To: Dan Carpenter
Cc: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
dledford-H+wXaHxf7aLQT0dZR+AlfA, Dean Luick
On Tue, Nov 10, 2015 at 12:59:29PM +0300, Dan Carpenter wrote:
> Gar... No. Please please get rid of the PC() macro. It makes the code
> impossible to understand because instead of hitting CTRL-[ you have
> decode it and then manually type out
>
> :cs find g CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_SHIFT
>
> which is the length of a typical college essay. I meant just put a
> comment like this:
>
> /*
> * In the hardware spec these are prefixed with:
> * CCE_PCIE_CTRL_...
> * But it is too long to use in code.
> */
> #define XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_MASK 0x1ull
>
> Or probably even better:
>
> #define CCE_PCIE_CTRL (CCE + 0x0000000000C0)
> #define LANE_BUNDLE_MASK 0x3ull /* CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_MASK */
> #define LANE_BUNDLE_SHIFT 0 /* CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_SHIFT */
> #define LANE_DELAY_MASK 0xFull /* CCE_PCIE_CTRL_PCIE_LANE_DELAY_MASK */
> #define LANE_DELAY_SHIFT 2 /* CCE_PCIE_CTRL_PCIE_LANE_DELAY_SHIFT */
> #define MARGIN_OVERWRITE_SHIFT 8 /* CCE_PCIE_CTRL_XMT_MARGIN_OVERWRITE_ENABLE_SHIFT */
> #define MARGIN_SHIFT 9 /* CCE_PCIE_CTRL_XMT_MARGIN_SHIFT */
> #define MARGIN_G1G2_OVERWRITE_MASK 0x1ull /* CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_MASK */
> #define MARGIN_G1G2_OVERWRITE_SHIFT 12 /* CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_SHIFT */
> #define MARGIN_G1G2_MASK 0x7ull /* CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_MASK */
> #define MARGIN_G1G2_SHIFT 13 /* CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_SHIFT */
>
> Those lines go over the 80 character limit but it's fine.
My apologies for not understanding what you meant. I took your meaning to be
that we had to honor the checkpatch checks so while the PC macro was
undesirable it was ok if I just made some comments...
FWIW I don't like the PC macro either. But we have a tool which is generating
these names to be identical to the hardware spec. And we really want to
preserve those as a reference back to the spec. Creating additional names which
are in the code is a bit cumbersome but what if we do something like this:
<auto generated from spec>
...
#define CCE_PCIE_CTRL (CCE + 0x0000000000C0)
#define CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_MASK 0x3ull
#define CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_SHIFT 0
...
<Defined for use in the code>
#define LANE_BUNDLE_MASK CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_MASK
#define LANE_BUNDLE_SHIFT CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_SHIFT
...
?????
An alternative would be to define some helper functions such as:
static inline u64 extract_xmt_margin_g1g2(u64 reg)
{
return (reg >> CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_SHIFT)
& CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_MASK;
}
...
...
xmt_margin = extract_xmt_margin_g1g2(pcie_ctrl);
...
I prefer the second option as it preserves the register names right in the
code. So you can reference the hardware spec without looking anything up in a
header file.
I again apologize for misunderstanding your previous meaning.
Ira
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] staging/rdma/hfi1: set Gen3 half-swing for integrated devices
2015-11-10 23:51 ` ira.weiny
@ 2015-11-11 7:37 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2015-11-11 7:37 UTC (permalink / raw)
To: ira.weiny; +Cc: devel, gregkh, dledford, Dean Luick, linux-rdma
That's fine. I feel like the first option is better because those long
names really are cumbersome. I sometimes cut and paste bugs caused by
using really long names because it's hard to spot the differences
between two really long names which are 80% the same filler text. (I'm
using a static checker so the checker finds the bugs and I have to stare
at it for a very long time to spot the characters which are different).
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-11-11 7:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-09 23:38 [PATCH v2] staging/rdma/hfi1: set Gen3 half-swing for integrated devices ira.weiny-ral2JQCrhuEAvxtiuMwx3w
2015-11-10 9:59 ` Dan Carpenter
2015-11-10 23:51 ` ira.weiny
2015-11-11 7:37 ` Dan Carpenter
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.