Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Guenter Roeck]

On Thu, Sep 01, 2016 at 09:57:58AM -0700, Brian Norris wrote:
> On Thu, Sep 01, 2016 at 11:34:55AM -0500, Bjorn Helgaas wrote:
> > I can't conveniently build it, so I'm sure I've broken things.  I
> 
> Indeed, you have :)
> 
> > pushed the current work-in-progress branch to pci/host-rockchip-wip.
> > After we fix build errors and other thinkos, I'll rename it and put it
> > in -next.
> 
> I'll append a patch that gets things building and working for me. With
> that:
> 
> Tested-by: Brian Norris 
> 
> I haven't examined all the changes in detail, but they mostly seem
> reasonable.
> 
> > I'll also post the broken-out patches for the changes I made on top of
> > the previous v10 (2098142ae87d).  I'll eventually squash them all into
> > the original commit so we don't have the clutter in the logs.
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c 
> b/drivers/pci/host/pcie-rockchip.c
> index 6623598679f2..ac846bab7396 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -128,9 +128,9 @@
>  #define PCIE_CLIENT_CONF_ENABLE  (0x0001 | 0x0001)
>  #define PCIE_CLIENT_LINK_TRAIN_ENABLE(0x0002 | 0x0002)
>  #define PCIE_CLIENT_ARI_ENABLE   (0x0008 | 0x0008)
> -#define PCIE_CLIENT_CONF_LANE_NUM(x) (0x0030 | (((x >> 1) & 3) << 4)
> +#define PCIE_CLIENT_CONF_LANE_NUM(x) (0x0030 | (((x >> 1) & 3) << 4))

I would suggest to use (x) for those.

>  #define PCIE_CLIENT_MODE_RC  (0x0040 | 0x0040)
> -#define PCIE_CLIENT_GEN_SEL(x)   (0x0080 | ((x & 1) << 7)
> +#define PCIE_CLIENT_GEN_SEL(x)   (0x0080 | ((x & 1) << 7))
>  #define  PCIE_CLIENT_GEN_SEL_0   0
>  #define  PCIE_CLIENT_GEN_SEL_2   1
>  
> @@ -643,14 +643,13 @@ static irqreturn_t rockchip_pcie_client_irq_handler(int 
> irq, void *arg)
>  static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
>  {
>   struct irq_chip *chip = irq_desc_get_chip(desc);
> - struct rockchip_pcie *rockchip;
> + struct rockchip_pcie *rockchip = irq_desc_get_handler_data(desc);
>   struct device *dev = rockchip->dev;
>   u32 reg;
>   u32 hwirq;
>   u32 virq;
>  
>   chained_irq_enter(chip, desc);
> - rockchip = irq_desc_get_handler_data(desc);
>  
>   reg = rockchip_pcie_read(rockchip, PCIE_CLIENT_INT_STATUS);
>   reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >>

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Guenter Roeck]

On Thu, Sep 01, 2016 at 09:57:58AM -0700, Brian Norris wrote:
> On Thu, Sep 01, 2016 at 11:34:55AM -0500, Bjorn Helgaas wrote:
> > I can't conveniently build it, so I'm sure I've broken things.  I
> 
> Indeed, you have :)
> 
> > pushed the current work-in-progress branch to pci/host-rockchip-wip.
> > After we fix build errors and other thinkos, I'll rename it and put it
> > in -next.
> 
> I'll append a patch that gets things building and working for me. With
> that:
> 
> Tested-by: Brian Norris 
> 
> I haven't examined all the changes in detail, but they mostly seem
> reasonable.
> 
> > I'll also post the broken-out patches for the changes I made on top of
> > the previous v10 (2098142ae87d).  I'll eventually squash them all into
> > the original commit so we don't have the clutter in the logs.
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c 
> b/drivers/pci/host/pcie-rockchip.c
> index 6623598679f2..ac846bab7396 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -128,9 +128,9 @@
>  #define PCIE_CLIENT_CONF_ENABLE  (0x0001 | 0x0001)
>  #define PCIE_CLIENT_LINK_TRAIN_ENABLE(0x0002 | 0x0002)
>  #define PCIE_CLIENT_ARI_ENABLE   (0x0008 | 0x0008)
> -#define PCIE_CLIENT_CONF_LANE_NUM(x) (0x0030 | (((x >> 1) & 3) << 4)
> +#define PCIE_CLIENT_CONF_LANE_NUM(x) (0x0030 | (((x >> 1) & 3) << 4))

I would suggest to use (x) for those.

>  #define PCIE_CLIENT_MODE_RC  (0x0040 | 0x0040)
> -#define PCIE_CLIENT_GEN_SEL(x)   (0x0080 | ((x & 1) << 7)
> +#define PCIE_CLIENT_GEN_SEL(x)   (0x0080 | ((x & 1) << 7))
>  #define  PCIE_CLIENT_GEN_SEL_0   0
>  #define  PCIE_CLIENT_GEN_SEL_2   1
>  
> @@ -643,14 +643,13 @@ static irqreturn_t rockchip_pcie_client_irq_handler(int 
> irq, void *arg)
>  static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
>  {
>   struct irq_chip *chip = irq_desc_get_chip(desc);
> - struct rockchip_pcie *rockchip;
> + struct rockchip_pcie *rockchip = irq_desc_get_handler_data(desc);
>   struct device *dev = rockchip->dev;
>   u32 reg;
>   u32 hwirq;
>   u32 virq;
>  
>   chained_irq_enter(chip, desc);
> - rockchip = irq_desc_get_handler_data(desc);
>  
>   reg = rockchip_pcie_read(rockchip, PCIE_CLIENT_INT_STATUS);
>   reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >>

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Heiko Stübner]

Am Freitag, 2. September 2016, 10:44:09 schrieb Bjorn Helgaas:
> 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_SHIFT0
> -#define PCIE_CLIENT_CONF_ENABLE_MASK 0x1
> -#define PCIE_CLIENT_LINK_TRAIN_ENABLE1
> -#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_MASK0x1
> -#define PCIE_CLIENT_GEN_SEL_21
> -#define PCIE_CLIENT_GEN_SEL_SHIFT7
> -#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_ENABLEHIWORD_UPDATE(0x0002, 0x0002)
> +#define PCIE_CLIENT_CONF_LANE_NUM(x) HIWORD_UPDATE(0x0030, ENCODE_LANES(x))
> +#define PCIE_CLIENT_GEN_SEL_2HIWORD_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);

I like this new approach :-)
Improves the readability in the code but also future readability of the defined 
constants, when comparing with register descriptions


Thanks
Heiko

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Heiko Stübner]

Am Freitag, 2. September 2016, 10:44:09 schrieb Bjorn Helgaas:
> 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_SHIFT0
> -#define PCIE_CLIENT_CONF_ENABLE_MASK 0x1
> -#define PCIE_CLIENT_LINK_TRAIN_ENABLE1
> -#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_MASK0x1
> -#define PCIE_CLIENT_GEN_SEL_21
> -#define PCIE_CLIENT_GEN_SEL_SHIFT7
> -#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_ENABLEHIWORD_UPDATE(0x0002, 0x0002)
> +#define PCIE_CLIENT_CONF_LANE_NUM(x) HIWORD_UPDATE(0x0030, ENCODE_LANES(x))
> +#define PCIE_CLIENT_GEN_SEL_2HIWORD_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);

I like this new approach :-)
Improves the readability in the code but also future readability of the defined 
constants, when comparing with register descriptions


Thanks
Heiko

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Bjorn Helgaas]

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_ENABLEBIT(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_MASK0x1
-#define PCIE_CLIENT_ARI_ENABLE BIT(0)
-#define PCIE_CLIENT_ARI_ENABLE_SHIFT   3
-#define PCIE_CLIENT_ARI_ENABLE_MASK0x1
-#define PCIE_CLIENT_CONF_LANE_NUM(x)   (x / 2)
-#define PCIE_CLIENT_CONF_LANE_NUM_SHIFT4
-#define PCIE_CLIENT_CONF_LANE_NUM_MASK 0x3
-#define PCIE_CLIENT_MODE_RCBIT(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_ENABLEHIWORD_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);

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Bjorn Helgaas]

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_ENABLEBIT(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_MASK0x1
-#define PCIE_CLIENT_ARI_ENABLE BIT(0)
-#define PCIE_CLIENT_ARI_ENABLE_SHIFT   3
-#define PCIE_CLIENT_ARI_ENABLE_MASK0x1
-#define PCIE_CLIENT_CONF_LANE_NUM(x)   (x / 2)
-#define PCIE_CLIENT_CONF_LANE_NUM_SHIFT4
-#define PCIE_CLIENT_CONF_LANE_NUM_MASK 0x3
-#define PCIE_CLIENT_MODE_RCBIT(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_ENABLEHIWORD_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);

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Brian Norris]

On Thu, Sep 01, 2016 at 11:34:55AM -0500, Bjorn Helgaas wrote:
> I can't conveniently build it, so I'm sure I've broken things.  I

Indeed, you have :)

> pushed the current work-in-progress branch to pci/host-rockchip-wip.
> After we fix build errors and other thinkos, I'll rename it and put it
> in -next.

I'll append a patch that gets things building and working for me. With
that:

Tested-by: Brian Norris 

I haven't examined all the changes in detail, but they mostly seem
reasonable.

> I'll also post the broken-out patches for the changes I made on top of
> the previous v10 (2098142ae87d).  I'll eventually squash them all into
> the original commit so we don't have the clutter in the logs.

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 6623598679f2..ac846bab7396 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -128,9 +128,9 @@
 #define PCIE_CLIENT_CONF_ENABLE(0x0001 | 0x0001)
 #define PCIE_CLIENT_LINK_TRAIN_ENABLE  (0x0002 | 0x0002)
 #define PCIE_CLIENT_ARI_ENABLE (0x0008 | 0x0008)
-#define PCIE_CLIENT_CONF_LANE_NUM(x)   (0x0030 | (((x >> 1) & 3) << 4)
+#define PCIE_CLIENT_CONF_LANE_NUM(x)   (0x0030 | (((x >> 1) & 3) << 4))
 #define PCIE_CLIENT_MODE_RC(0x0040 | 0x0040)
-#define PCIE_CLIENT_GEN_SEL(x) (0x0080 | ((x & 1) << 7)
+#define PCIE_CLIENT_GEN_SEL(x) (0x0080 | ((x & 1) << 7))
 #define  PCIE_CLIENT_GEN_SEL_0 0
 #define  PCIE_CLIENT_GEN_SEL_2 1
 
@@ -643,14 +643,13 @@ static irqreturn_t rockchip_pcie_client_irq_handler(int 
irq, void *arg)
 static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
 {
struct irq_chip *chip = irq_desc_get_chip(desc);
-   struct rockchip_pcie *rockchip;
+   struct rockchip_pcie *rockchip = irq_desc_get_handler_data(desc);
struct device *dev = rockchip->dev;
u32 reg;
u32 hwirq;
u32 virq;
 
chained_irq_enter(chip, desc);
-   rockchip = irq_desc_get_handler_data(desc);
 
reg = rockchip_pcie_read(rockchip, PCIE_CLIENT_INT_STATUS);
reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >>

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Brian Norris]

On Thu, Sep 01, 2016 at 11:34:55AM -0500, Bjorn Helgaas wrote:
> I can't conveniently build it, so I'm sure I've broken things.  I

Indeed, you have :)

> pushed the current work-in-progress branch to pci/host-rockchip-wip.
> After we fix build errors and other thinkos, I'll rename it and put it
> in -next.

I'll append a patch that gets things building and working for me. With
that:

Tested-by: Brian Norris 

I haven't examined all the changes in detail, but they mostly seem
reasonable.

> I'll also post the broken-out patches for the changes I made on top of
> the previous v10 (2098142ae87d).  I'll eventually squash them all into
> the original commit so we don't have the clutter in the logs.

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 6623598679f2..ac846bab7396 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -128,9 +128,9 @@
 #define PCIE_CLIENT_CONF_ENABLE(0x0001 | 0x0001)
 #define PCIE_CLIENT_LINK_TRAIN_ENABLE  (0x0002 | 0x0002)
 #define PCIE_CLIENT_ARI_ENABLE (0x0008 | 0x0008)
-#define PCIE_CLIENT_CONF_LANE_NUM(x)   (0x0030 | (((x >> 1) & 3) << 4)
+#define PCIE_CLIENT_CONF_LANE_NUM(x)   (0x0030 | (((x >> 1) & 3) << 4))
 #define PCIE_CLIENT_MODE_RC(0x0040 | 0x0040)
-#define PCIE_CLIENT_GEN_SEL(x) (0x0080 | ((x & 1) << 7)
+#define PCIE_CLIENT_GEN_SEL(x) (0x0080 | ((x & 1) << 7))
 #define  PCIE_CLIENT_GEN_SEL_0 0
 #define  PCIE_CLIENT_GEN_SEL_2 1
 
@@ -643,14 +643,13 @@ static irqreturn_t rockchip_pcie_client_irq_handler(int 
irq, void *arg)
 static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
 {
struct irq_chip *chip = irq_desc_get_chip(desc);
-   struct rockchip_pcie *rockchip;
+   struct rockchip_pcie *rockchip = irq_desc_get_handler_data(desc);
struct device *dev = rockchip->dev;
u32 reg;
u32 hwirq;
u32 virq;
 
chained_irq_enter(chip, desc);
-   rockchip = irq_desc_get_handler_data(desc);
 
reg = rockchip_pcie_read(rockchip, PCIE_CLIENT_INT_STATUS);
reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >>

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Bjorn Helgaas]

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 
> 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(0x0001 | 0x0001)
> +#define PCIE_CLIENT_LINK_TRAIN_ENABLE  (0x0002 | 0x0002)
> +#define PCIE_CLIENT_ARI_ENABLE (0x0008 | 0x0008)
> +#define PCIE_CLIENT_CONF_LANE_NUM(x)   (0x0030 | (((x >> 1) & 3) << 4)
> +#define PCIE_CLIENT_MODE_RC(0x0040 | 0x0040)
> +#define PCIE_CLIENT_GEN_SEL(x) (0x0080 | ((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_ENABLE0x1
  #define PCIE_CLIENT_CONF_ENABLE_SHIFT  0
  #define PCIE_CLIENT_CONF_ENABLE_MASK   0x1

 HIWORD_UPDATE_CONSTANT(PCIE_CLIENT_CONF_ENABLE) |

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Bjorn Helgaas]

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 
> 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(0x0001 | 0x0001)
> +#define PCIE_CLIENT_LINK_TRAIN_ENABLE  (0x0002 | 0x0002)
> +#define PCIE_CLIENT_ARI_ENABLE (0x0008 | 0x0008)
> +#define PCIE_CLIENT_CONF_LANE_NUM(x)   (0x0030 | (((x >> 1) & 3) << 4)
> +#define PCIE_CLIENT_MODE_RC(0x0040 | 0x0040)
> +#define PCIE_CLIENT_GEN_SEL(x) (0x0080 | ((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_ENABLE0x1
  #define PCIE_CLIENT_CONF_ENABLE_SHIFT  0
  #define PCIE_CLIENT_CONF_ENABLE_MASK   0x1

 HIWORD_UPDATE_CONSTANT(PCIE_CLIENT_CONF_ENABLE) |

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Brian Norris]

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?

commit 88cb3f59d73e261a3cd1957ff392f98c9916a5d1
Author: Bjorn Helgaas 
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(0x0001 | 0x0001)
+#define PCIE_CLIENT_LINK_TRAIN_ENABLE  (0x0002 | 0x0002)
+#define PCIE_CLIENT_ARI_ENABLE (0x0008 | 0x0008)
+#define PCIE_CLIENT_CONF_LANE_NUM(x)   (0x0030 | (((x >> 1) & 3) << 4)
+#define PCIE_CLIENT_MODE_RC(0x0040 | 0x0040)
+#define PCIE_CLIENT_GEN_SEL(x) (0x0080 | ((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.

Brian

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Brian Norris]

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?

commit 88cb3f59d73e261a3cd1957ff392f98c9916a5d1
Author: Bjorn Helgaas 
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(0x0001 | 0x0001)
+#define PCIE_CLIENT_LINK_TRAIN_ENABLE  (0x0002 | 0x0002)
+#define PCIE_CLIENT_ARI_ENABLE (0x0008 | 0x0008)
+#define PCIE_CLIENT_CONF_LANE_NUM(x)   (0x0030 | (((x >> 1) & 3) << 4)
+#define PCIE_CLIENT_MODE_RC(0x0040 | 0x0040)
+#define PCIE_CLIENT_GEN_SEL(x) (0x0080 | ((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.

Brian

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Heiko Stübner]

Am Donnerstag, 1. September 2016, 10:14:01 schrieb Brian Norris:
> 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?
> 
> commit 88cb3f59d73e261a3cd1957ff392f98c9916a5d1
> Author: Bjorn Helgaas 
> 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(0x0001 | 0x0001)
> +#define PCIE_CLIENT_LINK_TRAIN_ENABLE  (0x0002 | 0x0002)
> +#define PCIE_CLIENT_ARI_ENABLE (0x0008 | 0x0008)
> +#define PCIE_CLIENT_CONF_LANE_NUM(x)   (0x0030 | (((x >> 1) & 3) << 4)
> +#define PCIE_CLIENT_MODE_RC(0x0040 | 0x0040)
> +#define PCIE_CLIENT_GEN_SEL(x) (0x0080 | ((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.

personally I find it nicer to explicitly state, this is the common HIWORD-
update mechanism we have in a lot of places and not some other register 
voodoo.

Because right now, GRF and PMU syscons even contain most registers using that 
mechanism, but some that don't.

But I guess I won't protest to loudly if you like it differently in your 
subsystem ;-)


Heiko

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Heiko Stübner]

Am Donnerstag, 1. September 2016, 10:14:01 schrieb Brian Norris:
> 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?
> 
> commit 88cb3f59d73e261a3cd1957ff392f98c9916a5d1
> Author: Bjorn Helgaas 
> 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(0x0001 | 0x0001)
> +#define PCIE_CLIENT_LINK_TRAIN_ENABLE  (0x0002 | 0x0002)
> +#define PCIE_CLIENT_ARI_ENABLE (0x0008 | 0x0008)
> +#define PCIE_CLIENT_CONF_LANE_NUM(x)   (0x0030 | (((x >> 1) & 3) << 4)
> +#define PCIE_CLIENT_MODE_RC(0x0040 | 0x0040)
> +#define PCIE_CLIENT_GEN_SEL(x) (0x0080 | ((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.

personally I find it nicer to explicitly state, this is the common HIWORD-
update mechanism we have in a lot of places and not some other register 
voodoo.

Because right now, GRF and PMU syscons even contain most registers using that 
mechanism, but some that don't.

But I guess I won't protest to loudly if you like it differently in your 
subsystem ;-)


Heiko

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Bjorn Helgaas]

On Thu, Sep 01, 2016 at 09:57:58AM -0700, Brian Norris wrote:
> On Thu, Sep 01, 2016 at 11:34:55AM -0500, Bjorn Helgaas wrote:
> > I can't conveniently build it, so I'm sure I've broken things.  I
> 
> Indeed, you have :)
> 
> > pushed the current work-in-progress branch to pci/host-rockchip-wip.
> > After we fix build errors and other thinkos, I'll rename it and put it
> > in -next.
> 
> I'll append a patch that gets things building and working for me. With
> that:
> 
> Tested-by: Brian Norris 

Thanks, I added your fixes.

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Bjorn Helgaas]

On Thu, Sep 01, 2016 at 09:57:58AM -0700, Brian Norris wrote:
> On Thu, Sep 01, 2016 at 11:34:55AM -0500, Bjorn Helgaas wrote:
> > I can't conveniently build it, so I'm sure I've broken things.  I
> 
> Indeed, you have :)
> 
> > pushed the current work-in-progress branch to pci/host-rockchip-wip.
> > After we fix build errors and other thinkos, I'll rename it and put it
> > in -next.
> 
> I'll append a patch that gets things building and working for me. With
> that:
> 
> Tested-by: Brian Norris 

Thanks, I added your fixes.

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Bjorn Helgaas]

On Thu, Sep 01, 2016 at 11:39:41AM +0800, Shawn Lin wrote:
> Hi Guenter,
> 
> Thanks for your review, and I think it still not too late
> for nitpicking as it isn't merged to next branch. :)
> 
> We have amend the code a bit, so probably we fixed some of
> the minor issues against V10. But some of them are really
> personal taste, if you still insist on that, I will be ok
> to modify them.
> 
> I will push patch to fix them after your feedback. :)

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 can't conveniently build it, so I'm sure I've broken things.  I
pushed the current work-in-progress branch to pci/host-rockchip-wip.
After we fix build errors and other thinkos, I'll rename it and put it
in -next.

I'll also post the broken-out patches for the changes I made on top of
the previous v10 (2098142ae87d).  I'll eventually squash them all into
the original commit so we don't have the clutter in the logs.

> On 2016/9/1 2:17, Guenter Roeck wrote:
> >On Fri, Aug 19, 2016 at 09:34:58AM +0800, Shawn Lin wrote:
> 
> [...]
> 
> >>+   rockchip_pcie_enable_interrupts(port);
> >>+
> >
> >There is no matching disable_interrupts call in error handling after this.
> >Is that ok ?
> >
> 
> From test, probably ok since the genpd will be off and all of them
> wil be cleared.
> 
> >Also, is it ok to enable interrupts this early (before the rest of the
> >initialization is complete) ?
> >
> 
> The training starts, so we some client irq should be handled now, and we
> already register isr.
> 
> >>+   err = rockchip_pcie_init_irq_domain(port);
> >>+   if (err < 0)
> >>+   goto err_vpcie;
> >>+
> >>+   err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
> >>+  , _base);
> >>+   if (err)
> >>+   goto err_vpcie;
> >>+
> >>+   err = devm_request_pci_bus_resources(dev, );
> >>+   if (err)
> >>+   goto err_vpcie;
> >>+
> >>+   /* Get the I/O and memory ranges from DT */
> >>+   resource_list_for_each_entry(win, ) {
> >>+   switch (resource_type(win->res)) {
> >>+   case IORESOURCE_IO:
> >>+   io = win->res;
> >>+   io->name = "I/O";
> >>+   io_size = resource_size(io);
> >>+   io_bus_addr = io->start - win->offset;
> >>+   err = pci_remap_iospace(io, io_base);
> >>+   if (err) {
> >>+   dev_warn(port->dev, "error %d: failed to map 
> >>resource %pR\n",
> >>+err, io);
> >>+   continue;
> >>+   }
> >>+   break;
> >>+   case IORESOURCE_MEM:
> >>+   mem = win->res;
> >>+   mem->name = "MEM";
> >>+   mem_size = resource_size(mem);
> >>+   mem_bus_addr = mem->start - win->offset;
> >>+   break;
> >>+   case IORESOURCE_BUS:
> >>+   busn = win->res;
> >>+   break;
> >>+   default:
> >>+   continue;
> >>+   }
> >>+   }
> >>+
> >>+   if (mem_size)
> >
> >While strictly speaking not needed, I would recommend { }
> >here to improve readability.
> >
> >>+   for (reg_no = 0; reg_no < (mem_size >> 20); reg_no++) {
> >
> >This doesn't support mem sizes smaller than 1 << 20 (and clips the size
> >if it is not aligned to 1M). Is this intentional ?
> 
> yes, we don't support.
> 
> >
> >>+   err = rockchip_pcie_prog_ob_atu(port, reg_no + 1,
> >>+   AXI_WRAPPER_MEM_WRITE,
> >>+   20 - 1,
> >>+   mem_bus_addr +
> >>+   (reg_no << 20),
> >>+   0);
> >>+   if (err) {
> >>+   dev_err(dev, "program RC mem outbound ATU 
> >>failed\n");
> >>+   goto err_vpcie;
> >>+   }
> >>+   }
> 
> >
> >>+   err = rockchip_pcie_prog_ob_atu(port,
> >>+   reg_no + 1 + offset,
> >>+   AXI_WRAPPER_IO_WRITE,
> >>+   20 - 1,
> >>+   io_bus_addr +
> >>+   (reg_no << 20),
> >>+   0);
> >>+   if (err) {
> >>+   dev_err(dev, "program RC io outbound ATU 
> >>failed\n");
> >>+   goto err_vpcie;
> >>+   }
> >>+   }
> 
> >>+
> >>+   pci_bus_add_devices(bus);
> >>+
> >>+   dev_warn(dev, "only 

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Bjorn Helgaas]

On Thu, Sep 01, 2016 at 11:39:41AM +0800, Shawn Lin wrote:
> Hi Guenter,
> 
> Thanks for your review, and I think it still not too late
> for nitpicking as it isn't merged to next branch. :)
> 
> We have amend the code a bit, so probably we fixed some of
> the minor issues against V10. But some of them are really
> personal taste, if you still insist on that, I will be ok
> to modify them.
> 
> I will push patch to fix them after your feedback. :)

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 can't conveniently build it, so I'm sure I've broken things.  I
pushed the current work-in-progress branch to pci/host-rockchip-wip.
After we fix build errors and other thinkos, I'll rename it and put it
in -next.

I'll also post the broken-out patches for the changes I made on top of
the previous v10 (2098142ae87d).  I'll eventually squash them all into
the original commit so we don't have the clutter in the logs.

> On 2016/9/1 2:17, Guenter Roeck wrote:
> >On Fri, Aug 19, 2016 at 09:34:58AM +0800, Shawn Lin wrote:
> 
> [...]
> 
> >>+   rockchip_pcie_enable_interrupts(port);
> >>+
> >
> >There is no matching disable_interrupts call in error handling after this.
> >Is that ok ?
> >
> 
> From test, probably ok since the genpd will be off and all of them
> wil be cleared.
> 
> >Also, is it ok to enable interrupts this early (before the rest of the
> >initialization is complete) ?
> >
> 
> The training starts, so we some client irq should be handled now, and we
> already register isr.
> 
> >>+   err = rockchip_pcie_init_irq_domain(port);
> >>+   if (err < 0)
> >>+   goto err_vpcie;
> >>+
> >>+   err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
> >>+  , _base);
> >>+   if (err)
> >>+   goto err_vpcie;
> >>+
> >>+   err = devm_request_pci_bus_resources(dev, );
> >>+   if (err)
> >>+   goto err_vpcie;
> >>+
> >>+   /* Get the I/O and memory ranges from DT */
> >>+   resource_list_for_each_entry(win, ) {
> >>+   switch (resource_type(win->res)) {
> >>+   case IORESOURCE_IO:
> >>+   io = win->res;
> >>+   io->name = "I/O";
> >>+   io_size = resource_size(io);
> >>+   io_bus_addr = io->start - win->offset;
> >>+   err = pci_remap_iospace(io, io_base);
> >>+   if (err) {
> >>+   dev_warn(port->dev, "error %d: failed to map 
> >>resource %pR\n",
> >>+err, io);
> >>+   continue;
> >>+   }
> >>+   break;
> >>+   case IORESOURCE_MEM:
> >>+   mem = win->res;
> >>+   mem->name = "MEM";
> >>+   mem_size = resource_size(mem);
> >>+   mem_bus_addr = mem->start - win->offset;
> >>+   break;
> >>+   case IORESOURCE_BUS:
> >>+   busn = win->res;
> >>+   break;
> >>+   default:
> >>+   continue;
> >>+   }
> >>+   }
> >>+
> >>+   if (mem_size)
> >
> >While strictly speaking not needed, I would recommend { }
> >here to improve readability.
> >
> >>+   for (reg_no = 0; reg_no < (mem_size >> 20); reg_no++) {
> >
> >This doesn't support mem sizes smaller than 1 << 20 (and clips the size
> >if it is not aligned to 1M). Is this intentional ?
> 
> yes, we don't support.
> 
> >
> >>+   err = rockchip_pcie_prog_ob_atu(port, reg_no + 1,
> >>+   AXI_WRAPPER_MEM_WRITE,
> >>+   20 - 1,
> >>+   mem_bus_addr +
> >>+   (reg_no << 20),
> >>+   0);
> >>+   if (err) {
> >>+   dev_err(dev, "program RC mem outbound ATU 
> >>failed\n");
> >>+   goto err_vpcie;
> >>+   }
> >>+   }
> 
> >
> >>+   err = rockchip_pcie_prog_ob_atu(port,
> >>+   reg_no + 1 + offset,
> >>+   AXI_WRAPPER_IO_WRITE,
> >>+   20 - 1,
> >>+   io_bus_addr +
> >>+   (reg_no << 20),
> >>+   0);
> >>+   if (err) {
> >>+   dev_err(dev, "program RC io outbound ATU 
> >>failed\n");
> >>+   goto err_vpcie;
> >>+   }
> >>+   }
> 
> >>+
> >>+   pci_bus_add_devices(bus);
> >>+
> >>+   dev_warn(dev, "only 

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Guenter Roeck]

On 08/31/2016 08:39 PM, Shawn Lin wrote:

Hi Guenter,

Thanks for your review, and I think it still not too late
for nitpicking as it isn't merged to next branch. :)

We have amend the code a bit, so probably we fixed some of
the minor issues against V10. But some of them are really
personal taste, if you still insist on that, I will be ok
to modify them.



Take it as suggestions; I won't insist on anything.

Guenter

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Guenter Roeck]

On 08/31/2016 08:39 PM, Shawn Lin wrote:

Hi Guenter,

Thanks for your review, and I think it still not too late
for nitpicking as it isn't merged to next branch. :)

We have amend the code a bit, so probably we fixed some of
the minor issues against V10. But some of them are really
personal taste, if you still insist on that, I will be ok
to modify them.



Take it as suggestions; I won't insist on anything.

Guenter

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Shawn Lin]

Hi Guenter,

Thanks for your review, and I think it still not too late
for nitpicking as it isn't merged to next branch. :)

We have amend the code a bit, so probably we fixed some of
the minor issues against V10. But some of them are really
personal taste, if you still insist on that, I will be ok
to modify them.

I will push patch to fix them after your feedback. :)


On 2016/9/1 2:17, Guenter Roeck wrote:

On Fri, Aug 19, 2016 at 09:34:58AM +0800, Shawn Lin wrote:


[...]


+   rockchip_pcie_enable_interrupts(port);
+


There is no matching disable_interrupts call in error handling after this.
Is that ok ?



From test, probably ok since the genpd will be off and all of them wil 
be cleared.



Also, is it ok to enable interrupts this early (before the rest of the
initialization is complete) ?



The training starts, so we some client irq should be handled now, and we
already register isr.


+   err = rockchip_pcie_init_irq_domain(port);
+   if (err < 0)
+   goto err_vpcie;
+
+   err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
+  , _base);
+   if (err)
+   goto err_vpcie;
+
+   err = devm_request_pci_bus_resources(dev, );
+   if (err)
+   goto err_vpcie;
+
+   /* Get the I/O and memory ranges from DT */
+   resource_list_for_each_entry(win, ) {
+   switch (resource_type(win->res)) {
+   case IORESOURCE_IO:
+   io = win->res;
+   io->name = "I/O";
+   io_size = resource_size(io);
+   io_bus_addr = io->start - win->offset;
+   err = pci_remap_iospace(io, io_base);
+   if (err) {
+   dev_warn(port->dev, "error %d: failed to map 
resource %pR\n",
+err, io);
+   continue;
+   }
+   break;
+   case IORESOURCE_MEM:
+   mem = win->res;
+   mem->name = "MEM";
+   mem_size = resource_size(mem);
+   mem_bus_addr = mem->start - win->offset;
+   break;
+   case IORESOURCE_BUS:
+   busn = win->res;
+   break;
+   default:
+   continue;
+   }
+   }
+
+   if (mem_size)


While strictly speaking not needed, I would recommend { }
here to improve readability.


+   for (reg_no = 0; reg_no < (mem_size >> 20); reg_no++) {


This doesn't support mem sizes smaller than 1 << 20 (and clips the size
if it is not aligned to 1M). Is this intentional ?


yes, we don't support.




+   err = rockchip_pcie_prog_ob_atu(port, reg_no + 1,
+   AXI_WRAPPER_MEM_WRITE,
+   20 - 1,
+   mem_bus_addr +
+   (reg_no << 20),
+   0);
+   if (err) {
+   dev_err(dev, "program RC mem outbound ATU 
failed\n");
+   goto err_vpcie;
+   }
+   }





+   err = rockchip_pcie_prog_ob_atu(port,
+   reg_no + 1 + offset,
+   AXI_WRAPPER_IO_WRITE,
+   20 - 1,
+   io_bus_addr +
+   (reg_no << 20),
+   0);
+   if (err) {
+   dev_err(dev, "program RC io outbound ATU 
failed\n");
+   goto err_vpcie;
+   }
+   }



+
+   pci_bus_add_devices(bus);
+
+   dev_warn(dev, "only 32-bit config accesses supported; smaller writes may 
corrupt adjacent RW1C fields\n");
+


A warning which is always displayed seems to be a bit useless. If this is a
concern, maybe the driver should provide its own config space access functions
and map smaller accesses into 32 bit accesses. But isn't that already done ?



No, that is just for normal code path. When users comfigure it via some
user-space tool, it is sane to make them know this limitation. So we was 
suggested to do this.



+   return err;
+
+err_vpcie:
+   if (!IS_ERR(port->vpcie3v3))
+   regulator_disable(port->vpcie3v3);
+   if (!IS_ERR(port->vpcie1v8))
+   regulator_disable(port->vpcie1v8);
+   if (!IS_ERR(port->vpcie0v9))
+   

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Shawn Lin]

Hi Guenter,

Thanks for your review, and I think it still not too late
for nitpicking as it isn't merged to next branch. :)

We have amend the code a bit, so probably we fixed some of
the minor issues against V10. But some of them are really
personal taste, if you still insist on that, I will be ok
to modify them.

I will push patch to fix them after your feedback. :)


On 2016/9/1 2:17, Guenter Roeck wrote:

On Fri, Aug 19, 2016 at 09:34:58AM +0800, Shawn Lin wrote:


[...]


+   rockchip_pcie_enable_interrupts(port);
+


There is no matching disable_interrupts call in error handling after this.
Is that ok ?



From test, probably ok since the genpd will be off and all of them wil 
be cleared.



Also, is it ok to enable interrupts this early (before the rest of the
initialization is complete) ?



The training starts, so we some client irq should be handled now, and we
already register isr.


+   err = rockchip_pcie_init_irq_domain(port);
+   if (err < 0)
+   goto err_vpcie;
+
+   err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
+  , _base);
+   if (err)
+   goto err_vpcie;
+
+   err = devm_request_pci_bus_resources(dev, );
+   if (err)
+   goto err_vpcie;
+
+   /* Get the I/O and memory ranges from DT */
+   resource_list_for_each_entry(win, ) {
+   switch (resource_type(win->res)) {
+   case IORESOURCE_IO:
+   io = win->res;
+   io->name = "I/O";
+   io_size = resource_size(io);
+   io_bus_addr = io->start - win->offset;
+   err = pci_remap_iospace(io, io_base);
+   if (err) {
+   dev_warn(port->dev, "error %d: failed to map 
resource %pR\n",
+err, io);
+   continue;
+   }
+   break;
+   case IORESOURCE_MEM:
+   mem = win->res;
+   mem->name = "MEM";
+   mem_size = resource_size(mem);
+   mem_bus_addr = mem->start - win->offset;
+   break;
+   case IORESOURCE_BUS:
+   busn = win->res;
+   break;
+   default:
+   continue;
+   }
+   }
+
+   if (mem_size)


While strictly speaking not needed, I would recommend { }
here to improve readability.


+   for (reg_no = 0; reg_no < (mem_size >> 20); reg_no++) {


This doesn't support mem sizes smaller than 1 << 20 (and clips the size
if it is not aligned to 1M). Is this intentional ?


yes, we don't support.




+   err = rockchip_pcie_prog_ob_atu(port, reg_no + 1,
+   AXI_WRAPPER_MEM_WRITE,
+   20 - 1,
+   mem_bus_addr +
+   (reg_no << 20),
+   0);
+   if (err) {
+   dev_err(dev, "program RC mem outbound ATU 
failed\n");
+   goto err_vpcie;
+   }
+   }





+   err = rockchip_pcie_prog_ob_atu(port,
+   reg_no + 1 + offset,
+   AXI_WRAPPER_IO_WRITE,
+   20 - 1,
+   io_bus_addr +
+   (reg_no << 20),
+   0);
+   if (err) {
+   dev_err(dev, "program RC io outbound ATU 
failed\n");
+   goto err_vpcie;
+   }
+   }



+
+   pci_bus_add_devices(bus);
+
+   dev_warn(dev, "only 32-bit config accesses supported; smaller writes may 
corrupt adjacent RW1C fields\n");
+


A warning which is always displayed seems to be a bit useless. If this is a
concern, maybe the driver should provide its own config space access functions
and map smaller accesses into 32 bit accesses. But isn't that already done ?



No, that is just for normal code path. When users comfigure it via some
user-space tool, it is sane to make them know this limitation. So we was 
suggested to do this.



+   return err;
+
+err_vpcie:
+   if (!IS_ERR(port->vpcie3v3))
+   regulator_disable(port->vpcie3v3);
+   if (!IS_ERR(port->vpcie1v8))
+   regulator_disable(port->vpcie1v8);
+   if (!IS_ERR(port->vpcie0v9))
+   

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Guenter Roeck]

On Fri, Aug 19, 2016 at 09:34:58AM +0800, Shawn Lin wrote:
> This patch adds Rockchip PCIe controller support found
> on RK3399 Soc platform.
> 
> Signed-off-by: Shawn Lin 
> 
> Reviewed-by: Brian Norris 
> ---
> 
> Changes in v10:
> - remove ARM64 dependencies from Kconfig
> 
> Changes in v9:
> - ident definition of constants with tabs instead of spaces
> - remove some unused constans and remove the 'U' postfix of contants
> - use BIT(X) consistently as possible
> - indent the field definitions with two spaces to show the association
>   with the register name
> - improve rockchip_pcie_rd_own_conf and rockchip_pcie_wr_conf
> - fix misleading indentation
> - remove a checking for ib_atu
> - check the initialization of variables to make sure they would not
>   be used without initialization
> - some minor upper-case fix and blank lines fix.
> 
> Changes in v8:
> - fix all the commets suggested by Bjorn Helgaas in v7[1]
>   [1] https://patchwork.kernel.org/patch/9233887/
> - enable bandwith interrupt for debug
> 
> Changes in v7:
> - make it as a build-in driver
> - improve gen1/2 training timeout checking
> - only clear known interrupt
> - fix INTx for 0-base index
> 
> Changes in v6:
> - use "depends on PCI_MSI_IRQ_DOMAIN" suggested by Arnd
> 
> Changes in v5:
> - handle multiple pending INTx at the same time
>   suggested by Marc
> 
> Changes in v4:
> - address the comments from Brain
> 
> Changes in v3:
> - remove header file
> - remove struct msi_controller and move most of variables
>   of rockchip_pcie_port to become the local ones.
> - allow legacy int even if enabling MSI
> - drop regulator set voltage operation suggested by Doug
> 
> Changes in v2:
> - remove phy related stuff and call phy API
> - add new head file and define lots of macro to make
>   the code more readable
> - remove lots msi related code suggested by Marc
> - add IO window address translation
> - init_port and parse_dt reconstruction suggested by Bharat
> - improve wr_own_conf suggested by Arnd
> - make pcie as an interrupt controller and fix wrong int handler
>   suggested by Marc
> - remove PCI_PROBE_ONLY suggested by Lorenzo
> 
>  drivers/pci/host/Kconfig |   11 +
>  drivers/pci/host/Makefile|1 +
>  drivers/pci/host/pcie-rockchip.c | 1251 
> ++
>  3 files changed, 1263 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-rockchip.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 9b485d8..90f5e89 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -274,4 +274,15 @@ config PCIE_ARTPEC6
> Say Y here to enable PCIe controller support on Axis ARTPEC-6
> SoCs.  This PCIe controller uses the DesignWare core.
>  
> +config PCIE_ROCKCHIP
> + bool "Rockchip PCIe controller"
> + depends on ARCH_ROCKCHIP
> + depends on OF
> + depends on PCI_MSI_IRQ_DOMAIN
> + select MFD_SYSCON
> + help
> +   Say Y here if you want internal PCI support on Rockchip SoC.
> +   There is 1 internal PCIe port available to support GEN2 with
> +   4 slots.
> +
>  endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 8843410..a8afc16 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
>  obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> +obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
> diff --git a/drivers/pci/host/pcie-rockchip.c 
> b/drivers/pci/host/pcie-rockchip.c
> new file mode 100644
> index 000..eaea84d
> --- /dev/null
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -0,0 +1,1251 @@
> +/*
> + * Rockchip AXI PCIe host controller driver
> + *
> + * Copyright (c) 2016 Rockchip, Inc.
> + *
> + * Author: Shawn Lin 
> + * Wenrui Li 
> + *
> + * Bits taken from Synopsys Designware Host controller driver and
> + * ARM PCI Host generic driver.
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define PCIE_CLIENT_BASE 0x0
> +#define PCIE_RC_CONFIG_NORMAL_BASE   0x80
> +#define PCIE_RC_CONFIG_BASE  0xa0
> +#define PCIE_RC_CONFIG_LCSR  0xd0
> +#define  PCIE_RC_CONFIG_LCSR_LBMIE   

Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Guenter Roeck]

On Fri, Aug 19, 2016 at 09:34:58AM +0800, Shawn Lin wrote:
> This patch adds Rockchip PCIe controller support found
> on RK3399 Soc platform.
> 
> Signed-off-by: Shawn Lin 
> 
> Reviewed-by: Brian Norris 
> ---
> 
> Changes in v10:
> - remove ARM64 dependencies from Kconfig
> 
> Changes in v9:
> - ident definition of constants with tabs instead of spaces
> - remove some unused constans and remove the 'U' postfix of contants
> - use BIT(X) consistently as possible
> - indent the field definitions with two spaces to show the association
>   with the register name
> - improve rockchip_pcie_rd_own_conf and rockchip_pcie_wr_conf
> - fix misleading indentation
> - remove a checking for ib_atu
> - check the initialization of variables to make sure they would not
>   be used without initialization
> - some minor upper-case fix and blank lines fix.
> 
> Changes in v8:
> - fix all the commets suggested by Bjorn Helgaas in v7[1]
>   [1] https://patchwork.kernel.org/patch/9233887/
> - enable bandwith interrupt for debug
> 
> Changes in v7:
> - make it as a build-in driver
> - improve gen1/2 training timeout checking
> - only clear known interrupt
> - fix INTx for 0-base index
> 
> Changes in v6:
> - use "depends on PCI_MSI_IRQ_DOMAIN" suggested by Arnd
> 
> Changes in v5:
> - handle multiple pending INTx at the same time
>   suggested by Marc
> 
> Changes in v4:
> - address the comments from Brain
> 
> Changes in v3:
> - remove header file
> - remove struct msi_controller and move most of variables
>   of rockchip_pcie_port to become the local ones.
> - allow legacy int even if enabling MSI
> - drop regulator set voltage operation suggested by Doug
> 
> Changes in v2:
> - remove phy related stuff and call phy API
> - add new head file and define lots of macro to make
>   the code more readable
> - remove lots msi related code suggested by Marc
> - add IO window address translation
> - init_port and parse_dt reconstruction suggested by Bharat
> - improve wr_own_conf suggested by Arnd
> - make pcie as an interrupt controller and fix wrong int handler
>   suggested by Marc
> - remove PCI_PROBE_ONLY suggested by Lorenzo
> 
>  drivers/pci/host/Kconfig |   11 +
>  drivers/pci/host/Makefile|1 +
>  drivers/pci/host/pcie-rockchip.c | 1251 
> ++
>  3 files changed, 1263 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-rockchip.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 9b485d8..90f5e89 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -274,4 +274,15 @@ config PCIE_ARTPEC6
> Say Y here to enable PCIe controller support on Axis ARTPEC-6
> SoCs.  This PCIe controller uses the DesignWare core.
>  
> +config PCIE_ROCKCHIP
> + bool "Rockchip PCIe controller"
> + depends on ARCH_ROCKCHIP
> + depends on OF
> + depends on PCI_MSI_IRQ_DOMAIN
> + select MFD_SYSCON
> + help
> +   Say Y here if you want internal PCI support on Rockchip SoC.
> +   There is 1 internal PCIe port available to support GEN2 with
> +   4 slots.
> +
>  endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 8843410..a8afc16 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
>  obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> +obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
> diff --git a/drivers/pci/host/pcie-rockchip.c 
> b/drivers/pci/host/pcie-rockchip.c
> new file mode 100644
> index 000..eaea84d
> --- /dev/null
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -0,0 +1,1251 @@
> +/*
> + * Rockchip AXI PCIe host controller driver
> + *
> + * Copyright (c) 2016 Rockchip, Inc.
> + *
> + * Author: Shawn Lin 
> + * Wenrui Li 
> + *
> + * Bits taken from Synopsys Designware Host controller driver and
> + * ARM PCI Host generic driver.
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define PCIE_CLIENT_BASE 0x0
> +#define PCIE_RC_CONFIG_NORMAL_BASE   0x80
> +#define PCIE_RC_CONFIG_BASE  0xa0
> +#define PCIE_RC_CONFIG_LCSR  0xd0
> +#define  PCIE_RC_CONFIG_LCSR_LBMIE   BIT(10)
> +#define  PCIE_RC_CONFIG_LCSR_LABIE   BIT(11)
> +#define  PCIE_RC_CONFIG_LCSR_LBMS