On Fri, Sep 2, 2016 at 8:44 AM, Bjorn Helgaas [off-list ref] wrote:

Here's a second proposal.  It retains HIWORD_UPDATE (though the structure
is different) so grep finds it along with the other Rockchip ones.

I'll post updated actual patches; this is just to give the idea:

-/*
-  * 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))

-#define PCIE_CLIENT_CONF_ENABLE                        BIT(0)
-#define PCIE_CLIENT_CONF_ENABLE_SHIFT          0
-#define PCIE_CLIENT_CONF_ENABLE_MASK           0x1
-#define PCIE_CLIENT_LINK_TRAIN_ENABLE          1
-#define PCIE_CLIENT_LINK_TRAIN_SHIFT           1
-#define PCIE_CLIENT_LINK_TRAIN_MASK            0x1
-#define PCIE_CLIENT_ARI_ENABLE                 BIT(0)
-#define PCIE_CLIENT_ARI_ENABLE_SHIFT           3
-#define PCIE_CLIENT_ARI_ENABLE_MASK            0x1
-#define PCIE_CLIENT_CONF_LANE_NUM(x)           (x / 2)
-#define PCIE_CLIENT_CONF_LANE_NUM_SHIFT                4
-#define PCIE_CLIENT_CONF_LANE_NUM_MASK         0x3
-#define PCIE_CLIENT_MODE_RC                    BIT(0)
-#define PCIE_CLIENT_MODE_SHIFT                 6
-#define PCIE_CLIENT_MODE_MASK                  0x1
-#define PCIE_CLIENT_GEN_SEL_2                  1
-#define PCIE_CLIENT_GEN_SEL_SHIFT              7
-#define PCIE_CLIENT_GEN_SEL_MASK               0x1

+/*
+ * The upper 16 bits of the PCIE_CLIENT registers are a write mask for the
+ * lower 16 bits.  This allows atomic updates of the register without
+ * locking.
+ */
+#define HIWORD_UPDATE(mask, val)       ((mask << 16) | val)
+
+#define ENCODE_LANES(x)                        (((x >> 1) & 3) << 4)
+
+#define PCIE_CLIENT_CONF_ENABLE                HIWORD_UPDATE(0x0001,
0x0001)
+#define PCIE_CLIENT_LINK_TRAIN_ENABLE  HIWORD_UPDATE(0x0002, 0x0002)
+#define PCIE_CLIENT_CONF_LANE_NUM(x)   HIWORD_UPDATE(0x0030,
ENCODE_LANES(x))
+#define PCIE_CLIENT_GEN_SEL_2          HIWORD_UPDATE(0x0040, 0x0040)

        rockchip_pcie_write(rockchip,
-                  HIWORD_UPDATE(PCIE_CLIENT_CONF_ENABLE,
-                                PCIE_CLIENT_CONF_ENABLE_MASK,
-                                PCIE_CLIENT_CONF_ENABLE_SHIFT) |
-                  HIWORD_UPDATE(PCIE_CLIENT_
CONF_LANE_NUM(rockchip->lanes),
-                                PCIE_CLIENT_CONF_LANE_NUM_MASK,
-                                PCIE_CLIENT_CONF_LANE_NUM_SHIFT) |
-                  HIWORD_UPDATE(PCIE_CLIENT_MODE_RC,
-                                PCIE_CLIENT_MODE_MASK,
-                                PCIE_CLIENT_MODE_SHIFT) |
-                  HIWORD_UPDATE(PCIE_CLIENT_ARI_ENABLE,
-                                PCIE_CLIENT_ARI_ENABLE_MASK,
-                                PCIE_CLIENT_ARI_ENABLE_SHIFT) |
-                  HIWORD_UPDATE(PCIE_CLIENT_GEN_SEL_2,
-                                PCIE_CLIENT_GEN_SEL_MASK,
-                                PCIE_CLIENT_GEN_SEL_SHIFT),
-                  PCIE_CLIENT_BASE);
+                           PCIE_CLIENT_CONF_ENABLE |
+                           PCIE_CLIENT_LINK_TRAIN_ENABLE |
+                           PCIE_CLIENT_ARI_ENABLE |
+                           PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes) |
+                           PCIE_CLIENT_MODE_RC |
+                           PCIE_CLIENT_GEN_SEL_2,
+                               PCIE_CLIENT_BASE);

That's pretty much along the lines of the compromise I was implying
earlier. Seems decent to me, for whatever my opinion's worth :)

Thanks,
Brian