All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 1/4] PCI: rockchip: Provide captured slot power limit and scale
@ 2016-10-18  1:41 Shawn Lin
  2016-10-18  1:41 ` [PATCH v4 2/4] PCI: rockchip: Mark RC as common clock architecture Shawn Lin
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Shawn Lin @ 2016-10-18  1:41 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: linux-pci, linux-rockchip, Wenrui Li, Brian Norris, Shawn Lin

If vpcie3v3 is available, we could provide these information
via RC's configure register to make EP able to know the power
limit.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v4:
- rebase on next branch

Changes in v3:
- rebase the code since it isn't cleanly applied again.

Changes in v2:
- rebase the code since it isn't cleanly applied after Bjorn's cleanup

 drivers/pci/host/pcie-rockchip.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index e0b22da..3ede865 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -135,6 +135,10 @@
 #define PCIE_RC_CONFIG_VENDOR		(PCIE_RC_CONFIG_BASE + 0x00)
 #define PCIE_RC_CONFIG_RID_CCR		(PCIE_RC_CONFIG_BASE + 0x08)
 #define   PCIE_RC_CONFIG_SCC_SHIFT		16
+#define PCIE_RC_CONFIG_DCR		(PCIE_RC_CONFIG_BASE + 0xc4)
+#define   PCIE_RC_CONFIG_DCR_CSPL_SHIFT		18
+#define   PCIE_RC_CONFIG_DCR_CSPL_LIMIT		0xff
+#define   PCIE_RC_CONFIG_DCR_CPLS_SHIFT		26
 #define PCIE_RC_CONFIG_LCS		(PCIE_RC_CONFIG_BASE + 0xd0)
 #define   PCIE_RC_CONFIG_LCS_RETRAIN_LINK	BIT(5)
 #define   PCIE_RC_CONFIG_LCS_LBMIE		BIT(10)
@@ -395,6 +399,40 @@ static struct pci_ops rockchip_pcie_ops = {
 	.write = rockchip_pcie_wr_conf,
 };
 
+static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
+{
+	u32 status, curr, scale, power;
+
+	if (IS_ERR(rockchip->vpcie3v3))
+		return;
+
+	/*
+	 * Set RC's captured slot power limit and scale if
+	 * vpcie3v3 available. The default values are both zero
+	 * which means the software should set these two according
+	 * to the actual power supply.
+	 */
+	curr = regulator_get_current_limit(rockchip->vpcie3v3);
+	if (curr > 0) {
+		scale = 3; /* 0.001x */
+		curr = curr / 1000; /* convert to mA */
+		power = (curr * 3300) / 1000; /* milliwatt */
+		while (power > PCIE_RC_CONFIG_DCR_CSPL_LIMIT) {
+			if (!scale) {
+				dev_warn(rockchip->dev, "invalid power supply\n");
+				return;
+			}
+			scale--;
+			power = power / 10;
+		}
+
+		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DCR);
+		status |= (power << PCIE_RC_CONFIG_DCR_CSPL_SHIFT) |
+			  (scale << PCIE_RC_CONFIG_DCR_CPLS_SHIFT);
+		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCR);
+	}
+}
+
 /**
  * rockchip_pcie_init_port - Initialize hardware
  * @rockchip: PCIe port information
@@ -496,6 +534,8 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 		 (PCIE_CORE_CTRL_PLC1_FTS_CNT << PCIE_CORE_CTRL_PLC1_FTS_SHIFT);
 	rockchip_pcie_write(rockchip, status, PCIE_CORE_CTRL_PLC1);
 
+	rockchip_pcie_set_power_limit(rockchip);
+
 	/* Enable Gen1 training */
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
 			    PCIE_CLIENT_CONFIG);
-- 
2.3.7



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v4 2/4] PCI: rockchip: Mark RC as common clock architecture
  2016-10-18  1:41 [PATCH v4 1/4] PCI: rockchip: Provide captured slot power limit and scale Shawn Lin
@ 2016-10-18  1:41 ` Shawn Lin
  2016-11-11 22:09     ` Bjorn Helgaas
  2016-10-18  1:41 ` [PATCH v4 3/4] PCI: rockchip: add COMPILE_TEST for Kconfig Shawn Lin
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Shawn Lin @ 2016-10-18  1:41 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: linux-pci, linux-rockchip, Wenrui Li, Brian Norris, Shawn Lin

The default value of common clock configuration is
zero indicating Rockchip's RC is using asynchronous
clock architecture but actually we are using common
clock. This will confuses some EP drivers if they
need some different settings referring to this value.
So let's fix it.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v4:
- rebase on the next branch

Changes in v3:
- rebase the code since it isn't cleanly applied again

Changes in v2:
- rebase the code since it isn't cleanly applied after Bjorn's cleanup

 drivers/pci/host/pcie-rockchip.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 3ede865..8e260d2 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -141,6 +141,7 @@
 #define   PCIE_RC_CONFIG_DCR_CPLS_SHIFT		26
 #define PCIE_RC_CONFIG_LCS		(PCIE_RC_CONFIG_BASE + 0xd0)
 #define   PCIE_RC_CONFIG_LCS_RETRAIN_LINK	BIT(5)
+#define   PCIE_RC_CONFIG_LCS_CCC		BIT(6)
 #define   PCIE_RC_CONFIG_LCS_LBMIE		BIT(10)
 #define   PCIE_RC_CONFIG_LCS_LABIE		BIT(11)
 #define   PCIE_RC_CONFIG_LCS_LBMS		BIT(30)
@@ -536,6 +537,11 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 
 	rockchip_pcie_set_power_limit(rockchip);
 
+	/* Set RC's clock architecture as common clock */
+	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
+	status |= PCIE_RC_CONFIG_LCS_CCC;
+	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
+
 	/* Enable Gen1 training */
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
 			    PCIE_CLIENT_CONFIG);
-- 
2.3.7



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v4 3/4] PCI: rockchip: add COMPILE_TEST for Kconfig
  2016-10-18  1:41 [PATCH v4 1/4] PCI: rockchip: Provide captured slot power limit and scale Shawn Lin
  2016-10-18  1:41 ` [PATCH v4 2/4] PCI: rockchip: Mark RC as common clock architecture Shawn Lin
@ 2016-10-18  1:41 ` Shawn Lin
  2016-10-18  1:41 ` [PATCH v4 4/4] PCI: rockchip: fix wrong negotiated lanes calculation Shawn Lin
  2016-11-11 22:03 ` [PATCH v4 1/4] PCI: rockchip: Provide captured slot power limit and scale Bjorn Helgaas
  3 siblings, 0 replies; 8+ messages in thread
From: Shawn Lin @ 2016-10-18  1:41 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: linux-pci, linux-rockchip, Wenrui Li, Brian Norris, Shawn Lin

Add this could help build the driver if we enable
CONFIG_COMPILE_TEST.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/host/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index d7e7c0a..096440e 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -276,7 +276,7 @@ config PCIE_ARTPEC6
 
 config PCIE_ROCKCHIP
 	bool "Rockchip PCIe controller"
-	depends on ARCH_ROCKCHIP
+	depends on ARCH_ROCKCHIP || COMPILE_TEST
 	depends on OF
 	depends on PCI_MSI_IRQ_DOMAIN
 	select MFD_SYSCON
-- 
2.3.7



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v4 4/4] PCI: rockchip: fix wrong negotiated lanes calculation
  2016-10-18  1:41 [PATCH v4 1/4] PCI: rockchip: Provide captured slot power limit and scale Shawn Lin
  2016-10-18  1:41 ` [PATCH v4 2/4] PCI: rockchip: Mark RC as common clock architecture Shawn Lin
  2016-10-18  1:41 ` [PATCH v4 3/4] PCI: rockchip: add COMPILE_TEST for Kconfig Shawn Lin
@ 2016-10-18  1:41 ` Shawn Lin
  2016-11-11 22:03 ` [PATCH v4 1/4] PCI: rockchip: Provide captured slot power limit and scale Bjorn Helgaas
  3 siblings, 0 replies; 8+ messages in thread
From: Shawn Lin @ 2016-10-18  1:41 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: linux-pci, linux-rockchip, Wenrui Li, Brian Norris, Shawn Lin

The calculation of negotiated lanes is wrong since it should
be shifted by PCIE_CORE_PL_CONF_LANE_SHIFT, but it is shifted
by PCIE_CORE_PL_CONF_LANE_MASK. Let's fix it.

Fixes: commit e77f847df54c ("PCI: rockchip: Add Rockchip PCIe controller support")
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/host/pcie-rockchip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 8e260d2..279ef17 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -595,8 +595,8 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 
 	/* Check the final link width from negotiated lane counter from MGMT */
 	status = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL);
-	status =  0x1 << ((status & PCIE_CORE_PL_CONF_LANE_MASK) >>
-			  PCIE_CORE_PL_CONF_LANE_MASK);
+	status = 0x1 << ((status & PCIE_CORE_PL_CONF_LANE_MASK) >>
+			  PCIE_CORE_PL_CONF_LANE_SHIFT);
 	dev_dbg(dev, "current link width is x%d\n", status);
 
 	rockchip_pcie_write(rockchip, ROCKCHIP_VENDOR_ID,
-- 
2.3.7



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 1/4] PCI: rockchip: Provide captured slot power limit and scale
  2016-10-18  1:41 [PATCH v4 1/4] PCI: rockchip: Provide captured slot power limit and scale Shawn Lin
                   ` (2 preceding siblings ...)
  2016-10-18  1:41 ` [PATCH v4 4/4] PCI: rockchip: fix wrong negotiated lanes calculation Shawn Lin
@ 2016-11-11 22:03 ` Bjorn Helgaas
  3 siblings, 0 replies; 8+ messages in thread
From: Bjorn Helgaas @ 2016-11-11 22:03 UTC (permalink / raw)
  To: Shawn Lin
  Cc: Bjorn Helgaas, linux-pci, linux-rockchip, Wenrui Li, Brian Norris

On Tue, Oct 18, 2016 at 09:41:26AM +0800, Shawn Lin wrote:
> If vpcie3v3 is available, we could provide these information
> via RC's configure register to make EP able to know the power
> limit.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

I applied all four of these to pci/host-rockchip for v4.10, thanks!

> ---
> 
> Changes in v4:
> - rebase on next branch
> 
> Changes in v3:
> - rebase the code since it isn't cleanly applied again.
> 
> Changes in v2:
> - rebase the code since it isn't cleanly applied after Bjorn's cleanup
> 
>  drivers/pci/host/pcie-rockchip.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index e0b22da..3ede865 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -135,6 +135,10 @@
>  #define PCIE_RC_CONFIG_VENDOR		(PCIE_RC_CONFIG_BASE + 0x00)
>  #define PCIE_RC_CONFIG_RID_CCR		(PCIE_RC_CONFIG_BASE + 0x08)
>  #define   PCIE_RC_CONFIG_SCC_SHIFT		16
> +#define PCIE_RC_CONFIG_DCR		(PCIE_RC_CONFIG_BASE + 0xc4)
> +#define   PCIE_RC_CONFIG_DCR_CSPL_SHIFT		18
> +#define   PCIE_RC_CONFIG_DCR_CSPL_LIMIT		0xff
> +#define   PCIE_RC_CONFIG_DCR_CPLS_SHIFT		26
>  #define PCIE_RC_CONFIG_LCS		(PCIE_RC_CONFIG_BASE + 0xd0)
>  #define   PCIE_RC_CONFIG_LCS_RETRAIN_LINK	BIT(5)
>  #define   PCIE_RC_CONFIG_LCS_LBMIE		BIT(10)
> @@ -395,6 +399,40 @@ static struct pci_ops rockchip_pcie_ops = {
>  	.write = rockchip_pcie_wr_conf,
>  };
>  
> +static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
> +{
> +	u32 status, curr, scale, power;
> +
> +	if (IS_ERR(rockchip->vpcie3v3))
> +		return;
> +
> +	/*
> +	 * Set RC's captured slot power limit and scale if
> +	 * vpcie3v3 available. The default values are both zero
> +	 * which means the software should set these two according
> +	 * to the actual power supply.
> +	 */
> +	curr = regulator_get_current_limit(rockchip->vpcie3v3);
> +	if (curr > 0) {
> +		scale = 3; /* 0.001x */
> +		curr = curr / 1000; /* convert to mA */
> +		power = (curr * 3300) / 1000; /* milliwatt */
> +		while (power > PCIE_RC_CONFIG_DCR_CSPL_LIMIT) {
> +			if (!scale) {
> +				dev_warn(rockchip->dev, "invalid power supply\n");
> +				return;
> +			}
> +			scale--;
> +			power = power / 10;
> +		}
> +
> +		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DCR);
> +		status |= (power << PCIE_RC_CONFIG_DCR_CSPL_SHIFT) |
> +			  (scale << PCIE_RC_CONFIG_DCR_CPLS_SHIFT);
> +		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCR);
> +	}
> +}
> +
>  /**
>   * rockchip_pcie_init_port - Initialize hardware
>   * @rockchip: PCIe port information
> @@ -496,6 +534,8 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>  		 (PCIE_CORE_CTRL_PLC1_FTS_CNT << PCIE_CORE_CTRL_PLC1_FTS_SHIFT);
>  	rockchip_pcie_write(rockchip, status, PCIE_CORE_CTRL_PLC1);
>  
> +	rockchip_pcie_set_power_limit(rockchip);
> +
>  	/* Enable Gen1 training */
>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
>  			    PCIE_CLIENT_CONFIG);
> -- 
> 2.3.7
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 2/4] PCI: rockchip: Mark RC as common clock architecture
@ 2016-11-11 22:09     ` Bjorn Helgaas
  0 siblings, 0 replies; 8+ messages in thread
From: Bjorn Helgaas @ 2016-11-11 22:09 UTC (permalink / raw)
  To: Shawn Lin
  Cc: Bjorn Helgaas, linux-pci, linux-rockchip, Wenrui Li, Brian Norris

On Tue, Oct 18, 2016 at 09:41:27AM +0800, Shawn Lin wrote:
> The default value of common clock configuration is
> zero indicating Rockchip's RC is using asynchronous
> clock architecture but actually we are using common
> clock. This will confuses some EP drivers if they
> need some different settings referring to this value.
> So let's fix it.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
> Changes in v4:
> - rebase on the next branch
> 
> Changes in v3:
> - rebase the code since it isn't cleanly applied again
> 
> Changes in v2:
> - rebase the code since it isn't cleanly applied after Bjorn's cleanup
> 
>  drivers/pci/host/pcie-rockchip.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 3ede865..8e260d2 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -141,6 +141,7 @@
>  #define   PCIE_RC_CONFIG_DCR_CPLS_SHIFT		26
>  #define PCIE_RC_CONFIG_LCS		(PCIE_RC_CONFIG_BASE + 0xd0)
>  #define   PCIE_RC_CONFIG_LCS_RETRAIN_LINK	BIT(5)
> +#define   PCIE_RC_CONFIG_LCS_CCC		BIT(6)
>  #define   PCIE_RC_CONFIG_LCS_LBMIE		BIT(10)
>  #define   PCIE_RC_CONFIG_LCS_LABIE		BIT(11)
>  #define   PCIE_RC_CONFIG_LCS_LBMS		BIT(30)

These look an awful lot like these generic PCIe definitions:

  #define PCI_EXP_LNKCTL          16      /* Link Control */
  #define  PCI_EXP_LNKCTL_ASPMC   0x0003  /* ASPM Control */
  #define  PCI_EXP_LNKCTL_ASPM_L0S 0x0001 /* L0s Enable */
  #define  PCI_EXP_LNKCTL_ASPM_L1  0x0002 /* L1 Enable */
  #define  PCI_EXP_LNKCTL_RCB     0x0008  /* Read Completion Boundary */
  #define  PCI_EXP_LNKCTL_LD      0x0010  /* Link Disable */
  #define  PCI_EXP_LNKCTL_RL      0x0020  /* Retrain Link */
  #define  PCI_EXP_LNKCTL_CCC     0x0040  /* Common Clock Configuration */
  #define  PCI_EXP_LNKCTL_ES      0x0080  /* Extended Synch */
  #define  PCI_EXP_LNKCTL_CLKREQ_EN 0x0100 /* Enable clkreq */
  #define  PCI_EXP_LNKCTL_HAWD    0x0200  /* Hardware Autonomous Width Disable */
  #define  PCI_EXP_LNKCTL_LBMIE   0x0400  /* Link Bandwidth Management Interrupt Enable */
  #define  PCI_EXP_LNKCTL_LABIE   0x0800  /* Link Autonomous Bandwidth Interrupt Enable */

If these are indeed the same, I'd really like it if you could use the
generic definitions, e.g., by something like:

  #define PCIE_RC_PCIE_CAP_BASE    (PCIE_RC_CONFIG_BASE + 0xc0)

  status = rockchip_pcie_read(rockchip, PCIE_RC_PCIE_CAP_BASE + PCI_EXP_LNKCTL);
  status |= PCI_EXP_LNKCTL_CCC;

That way grep has a chance to find related pieces.

If they only *look* similar but are not really the same, then
disregard this, of course.

> @@ -536,6 +537,11 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>  
>  	rockchip_pcie_set_power_limit(rockchip);
>  
> +	/* Set RC's clock architecture as common clock */
> +	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> +	status |= PCIE_RC_CONFIG_LCS_CCC;
> +	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
> +
>  	/* Enable Gen1 training */
>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
>  			    PCIE_CLIENT_CONFIG);
> -- 
> 2.3.7
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 2/4] PCI: rockchip: Mark RC as common clock architecture
@ 2016-11-11 22:09     ` Bjorn Helgaas
  0 siblings, 0 replies; 8+ messages in thread
From: Bjorn Helgaas @ 2016-11-11 22:09 UTC (permalink / raw)
  To: Shawn Lin
  Cc: Bjorn Helgaas, linux-pci-u79uwXL29TY76Z2rM5mHXA, Wenrui Li,
	Brian Norris, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

On Tue, Oct 18, 2016 at 09:41:27AM +0800, Shawn Lin wrote:
> The default value of common clock configuration is
> zero indicating Rockchip's RC is using asynchronous
> clock architecture but actually we are using common
> clock. This will confuses some EP drivers if they
> need some different settings referring to this value.
> So let's fix it.
> 
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> 
> ---
> 
> Changes in v4:
> - rebase on the next branch
> 
> Changes in v3:
> - rebase the code since it isn't cleanly applied again
> 
> Changes in v2:
> - rebase the code since it isn't cleanly applied after Bjorn's cleanup
> 
>  drivers/pci/host/pcie-rockchip.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 3ede865..8e260d2 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -141,6 +141,7 @@
>  #define   PCIE_RC_CONFIG_DCR_CPLS_SHIFT		26
>  #define PCIE_RC_CONFIG_LCS		(PCIE_RC_CONFIG_BASE + 0xd0)
>  #define   PCIE_RC_CONFIG_LCS_RETRAIN_LINK	BIT(5)
> +#define   PCIE_RC_CONFIG_LCS_CCC		BIT(6)
>  #define   PCIE_RC_CONFIG_LCS_LBMIE		BIT(10)
>  #define   PCIE_RC_CONFIG_LCS_LABIE		BIT(11)
>  #define   PCIE_RC_CONFIG_LCS_LBMS		BIT(30)

These look an awful lot like these generic PCIe definitions:

  #define PCI_EXP_LNKCTL          16      /* Link Control */
  #define  PCI_EXP_LNKCTL_ASPMC   0x0003  /* ASPM Control */
  #define  PCI_EXP_LNKCTL_ASPM_L0S 0x0001 /* L0s Enable */
  #define  PCI_EXP_LNKCTL_ASPM_L1  0x0002 /* L1 Enable */
  #define  PCI_EXP_LNKCTL_RCB     0x0008  /* Read Completion Boundary */
  #define  PCI_EXP_LNKCTL_LD      0x0010  /* Link Disable */
  #define  PCI_EXP_LNKCTL_RL      0x0020  /* Retrain Link */
  #define  PCI_EXP_LNKCTL_CCC     0x0040  /* Common Clock Configuration */
  #define  PCI_EXP_LNKCTL_ES      0x0080  /* Extended Synch */
  #define  PCI_EXP_LNKCTL_CLKREQ_EN 0x0100 /* Enable clkreq */
  #define  PCI_EXP_LNKCTL_HAWD    0x0200  /* Hardware Autonomous Width Disable */
  #define  PCI_EXP_LNKCTL_LBMIE   0x0400  /* Link Bandwidth Management Interrupt Enable */
  #define  PCI_EXP_LNKCTL_LABIE   0x0800  /* Link Autonomous Bandwidth Interrupt Enable */

If these are indeed the same, I'd really like it if you could use the
generic definitions, e.g., by something like:

  #define PCIE_RC_PCIE_CAP_BASE    (PCIE_RC_CONFIG_BASE + 0xc0)

  status = rockchip_pcie_read(rockchip, PCIE_RC_PCIE_CAP_BASE + PCI_EXP_LNKCTL);
  status |= PCI_EXP_LNKCTL_CCC;

That way grep has a chance to find related pieces.

If they only *look* similar but are not really the same, then
disregard this, of course.

> @@ -536,6 +537,11 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>  
>  	rockchip_pcie_set_power_limit(rockchip);
>  
> +	/* Set RC's clock architecture as common clock */
> +	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> +	status |= PCIE_RC_CONFIG_LCS_CCC;
> +	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
> +
>  	/* Enable Gen1 training */
>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
>  			    PCIE_CLIENT_CONFIG);
> -- 
> 2.3.7
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 2/4] PCI: rockchip: Mark RC as common clock architecture
  2016-11-11 22:09     ` Bjorn Helgaas
  (?)
@ 2016-11-12  1:54     ` Shawn Lin
  -1 siblings, 0 replies; 8+ messages in thread
From: Shawn Lin @ 2016-11-12  1:54 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: shawn.lin, Bjorn Helgaas, linux-pci, linux-rockchip, Wenrui Li,
	Brian Norris

On 2016/11/12 6:09, Bjorn Helgaas wrote:
> On Tue, Oct 18, 2016 at 09:41:27AM +0800, Shawn Lin wrote:
>> The default value of common clock configuration is
>> zero indicating Rockchip's RC is using asynchronous
>> clock architecture but actually we are using common
>> clock. This will confuses some EP drivers if they
>> need some different settings referring to this value.
>> So let's fix it.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>> Changes in v4:
>> - rebase on the next branch
>>
>> Changes in v3:
>> - rebase the code since it isn't cleanly applied again
>>
>> Changes in v2:
>> - rebase the code since it isn't cleanly applied after Bjorn's cleanup
>>
>>  drivers/pci/host/pcie-rockchip.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
>> index 3ede865..8e260d2 100644
>> --- a/drivers/pci/host/pcie-rockchip.c
>> +++ b/drivers/pci/host/pcie-rockchip.c
>> @@ -141,6 +141,7 @@
>>  #define   PCIE_RC_CONFIG_DCR_CPLS_SHIFT		26
>>  #define PCIE_RC_CONFIG_LCS		(PCIE_RC_CONFIG_BASE + 0xd0)
>>  #define   PCIE_RC_CONFIG_LCS_RETRAIN_LINK	BIT(5)
>> +#define   PCIE_RC_CONFIG_LCS_CCC		BIT(6)
>>  #define   PCIE_RC_CONFIG_LCS_LBMIE		BIT(10)
>>  #define   PCIE_RC_CONFIG_LCS_LABIE		BIT(11)
>>  #define   PCIE_RC_CONFIG_LCS_LBMS		BIT(30)
>
> These look an awful lot like these generic PCIe definitions:
>
>   #define PCI_EXP_LNKCTL          16      /* Link Control */
>   #define  PCI_EXP_LNKCTL_ASPMC   0x0003  /* ASPM Control */
>   #define  PCI_EXP_LNKCTL_ASPM_L0S 0x0001 /* L0s Enable */
>   #define  PCI_EXP_LNKCTL_ASPM_L1  0x0002 /* L1 Enable */
>   #define  PCI_EXP_LNKCTL_RCB     0x0008  /* Read Completion Boundary */
>   #define  PCI_EXP_LNKCTL_LD      0x0010  /* Link Disable */
>   #define  PCI_EXP_LNKCTL_RL      0x0020  /* Retrain Link */
>   #define  PCI_EXP_LNKCTL_CCC     0x0040  /* Common Clock Configuration */
>   #define  PCI_EXP_LNKCTL_ES      0x0080  /* Extended Synch */
>   #define  PCI_EXP_LNKCTL_CLKREQ_EN 0x0100 /* Enable clkreq */
>   #define  PCI_EXP_LNKCTL_HAWD    0x0200  /* Hardware Autonomous Width Disable */
>   #define  PCI_EXP_LNKCTL_LBMIE   0x0400  /* Link Bandwidth Management Interrupt Enable */
>   #define  PCI_EXP_LNKCTL_LABIE   0x0800  /* Link Autonomous Bandwidth Interrupt Enable */
>
> If these are indeed the same, I'd really like it if you could use the
> generic definitions, e.g., by something like:
>

Ahh, thanks for sharing this. PCIE_RC_CONFIG_LCS is short for link
control and status register for RC. So the register layout looks
the same for that of PCI_EXP_LNKCTL after checking the TRM again.

I will come up a cleanup patch to use the existed PCI_EXP_LNKCTL_XXX
instead. :)


>   #define PCIE_RC_PCIE_CAP_BASE    (PCIE_RC_CONFIG_BASE + 0xc0)
>
>   status = rockchip_pcie_read(rockchip, PCIE_RC_PCIE_CAP_BASE + PCI_EXP_LNKCTL);
>   status |= PCI_EXP_LNKCTL_CCC;
>
> That way grep has a chance to find related pieces.
>
> If they only *look* similar but are not really the same, then
> disregard this, of course.
>
>> @@ -536,6 +537,11 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>>
>>  	rockchip_pcie_set_power_limit(rockchip);
>>
>> +	/* Set RC's clock architecture as common clock */
>> +	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
>> +	status |= PCIE_RC_CONFIG_LCS_CCC;
>> +	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
>> +
>>  	/* Enable Gen1 training */
>>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
>>  			    PCIE_CLIENT_CONFIG);
>> --
>> 2.3.7
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Best Regards
Shawn Lin

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-11-12  1:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-18  1:41 [PATCH v4 1/4] PCI: rockchip: Provide captured slot power limit and scale Shawn Lin
2016-10-18  1:41 ` [PATCH v4 2/4] PCI: rockchip: Mark RC as common clock architecture Shawn Lin
2016-11-11 22:09   ` Bjorn Helgaas
2016-11-11 22:09     ` Bjorn Helgaas
2016-11-12  1:54     ` Shawn Lin
2016-10-18  1:41 ` [PATCH v4 3/4] PCI: rockchip: add COMPILE_TEST for Kconfig Shawn Lin
2016-10-18  1:41 ` [PATCH v4 4/4] PCI: rockchip: fix wrong negotiated lanes calculation Shawn Lin
2016-11-11 22:03 ` [PATCH v4 1/4] PCI: rockchip: Provide captured slot power limit and scale Bjorn Helgaas

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.