From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.kernel.org ([198.145.29.136]:55806 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932236AbcIBPoP (ORCPT ); Fri, 2 Sep 2016 11:44:15 -0400 Date: Fri, 2 Sep 2016 10:44:09 -0500 From: Bjorn Helgaas To: Brian Norris Cc: Shawn Lin , Guenter Roeck , Bjorn Helgaas , Marc Zyngier , linux-pci@vger.kernel.org, Arnd Bergmann , linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, Heiko Stuebner , Doug Anderson , Wenrui Li , Rob Herring , devicetree@vger.kernel.org Subject: Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support Message-ID: <20160902154409.GA8370@localhost> References: <1471570498-3284-1-git-send-email-shawn.lin@rock-chips.com> <20160831181754.GA7460@roeck-us.net> <9075a2c1-c787-986f-31b4-4d5b7c92b771@rock-chips.com> <20160901163455.GA9471@localhost> <20160901171400.GA4093@localhost> <20160901174852.GC9471@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <20160901174852.GC9471@localhost> Sender: linux-pci-owner@vger.kernel.org List-ID: On Thu, Sep 01, 2016 at 12:48:52PM -0500, Bjorn Helgaas wrote: > On Thu, Sep 01, 2016 at 10:14:01AM -0700, Brian Norris wrote: > > 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()?). 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);