diff mbox

[v4,2/4] PCI: rockchip: Mark RC as common clock architecture

Message ID 1476754889-21804-2-git-send-email-shawn.lin@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shawn Lin Oct. 18, 2016, 1:41 a.m. UTC 
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(+)

Comments

Bjorn Helgaas Nov. 11, 2016, 10:09 p.m. UTC | #1 
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
Shawn Lin Nov. 12, 2016, 1:54 a.m. UTC | #2 
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
>
diff mbox

Patch

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);