On Thu, Sep 01, 2016 at 10:14:01AM -0700, Brian Norris wrote: > On Thu, Sep 01, 2016 at 11:34:55AM -0500, Bjorn Helgaas wrote: > > In the interest of making progress, I made most of the changes Guenter > > suggested (thanks very much, Guenter!) and made some more of my own on > > top of those. > > I'm not sure, but was this change your idea Bjorn? Yep. > commit 88cb3f59d73e261a3cd1957ff392f98c9916a5d1 > Author: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > Date: Thu Sep 1 10:27:44 2016 -0500 > > Simplify the confusing HIWORD_UPDATE scheme. > > [Which I'll summarize here; you're replacing the macro: > > -/* > - * The higher 16-bit of this register is used for write protection > - * only if BIT(x + 16) set to 1 the BIT(x) can be written. > - */ > -#define HIWORD_UPDATE(val, mask, shift) \ > - ((val) << (shift) | (mask) << ((shift) + 16)) > > with its expansions: > > + > +/* > + * The higher 16-bit of this register is used for write protection > + * only if BIT(x + 16) set to 1 the BIT(x) can be written. > + */ > +#define PCIE_CLIENT_CONF_ENABLE (0x00010000 | 0x0001) > +#define PCIE_CLIENT_LINK_TRAIN_ENABLE (0x00020000 | 0x0002) > +#define PCIE_CLIENT_ARI_ENABLE (0x00080000 | 0x0008) > +#define PCIE_CLIENT_CONF_LANE_NUM(x) (0x00300000 | (((x >> 1) & 3) << 4) > +#define PCIE_CLIENT_MODE_RC (0x00400000 | 0x0040) > +#define PCIE_CLIENT_GEN_SEL(x) (0x00800000 | ((x & 1) << 7) > +#define PCIE_CLIENT_GEN_SEL_0 0 > +#define PCIE_CLIENT_GEN_SEL_2 1 > > Full change can be had by: > git fetch git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git pci/host-rockchip-wip > ] > > > The use of HIWORD_UPDATE can indeed be a bit confusing, IMO, but this is > really a common Rockchip-ism that, once you read several of their > drivers, can make a little more sense. If you grep around, it's in at > least their clock, ethernet, SDHCI, PHY, and DP/DRM drivers. I might > defer to Heiko (upstream maintainer of Rockchip code) for a decision. > Maybe there's some intermediate ground where we keep the HIWORK_UPDATE() > logic (it does make sure we get the 16-bit shift right, I think) while > still refactoring a few other bits (like PCIE_CLIENT_CONF_LANE_NUM() and > PCIE_CLIENT_GEN_SEL() for wrapping HIWORK_UPDATE()?). > > Anyway, just a thought. If it really is most understandable to write it > this way, then maybe it's fine to be different than the other 16 > rockchip files that have this pattern. Hmm. I did look at the other files, so I know this isn't a PCIe special. HIWORD_UPDATE is ugly as sin, especially in cases like this where we're always writing a constant value and it takes three #defines plus HIWORD_UPDATE itself, e.g., - HIWORD_UPDATE(PCIE_CLIENT_CONF_ENABLE, - PCIE_CLIENT_CONF_ENABLE_MASK, - PCIE_CLIENT_CONF_ENABLE_SHIFT) | vs this: + PCIE_CLIENT_CONF_ENABLE | For these constant cases, a HIWORD_UPDATE that generated the associated macro names would help readability a bit, though we'd still need the ugly #defines: #define HIWORD_UPDATE_CONSTANT(x) (HIWORD_UPDATE(x, x ## _MASK, x ## _SHIFT) #define PCIE_CLIENT_CONF_ENABLE 0x1 #define PCIE_CLIENT_CONF_ENABLE_SHIFT 0 #define PCIE_CLIENT_CONF_ENABLE_MASK 0x1 HIWORD_UPDATE_CONSTANT(PCIE_CLIENT_CONF_ENABLE) | -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html