| Message ID | 1447112288-28327-1-git-send-email-ira.weiny@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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 -- 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
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- 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 --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__);