Patchwork pci: tegra: set up PADS_REFCLK_CFG1

login
register
mail settings
Submitter Stephen Warren
Date June 27, 2013, 8:42 p.m.
Message ID <1372365722-3318-1-git-send-email-swarren@wwwdotorg.org>
Download mbox | patch
Permalink /patch/255224/
State Superseded, archived
Headers show

Comments

Stephen Warren - June 27, 2013, 8:42 p.m.
From: Stephen Warren <swarren@nvidia.com>

The registers PADS_REFCLK_CFG are an array of 16-bit data, one entry per
PCIe root port. For Tegra30, we therefore need to write a 3rd entry in
this array. Doing so mays the mini-PCIe slot on Beaver operate correctly.

While we're at it, add some #defines to partially document the fields
within these 16-bit values.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
This is for Thierry to squash into his Tegra PCIe driver staging branch.

 drivers/pci/host/pci-tegra.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)
Thierry Reding - June 28, 2013, 7:33 p.m.
On Thu, Jun 27, 2013 at 02:42:02PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> The registers PADS_REFCLK_CFG are an array of 16-bit data, one entry per
> PCIe root port. For Tegra30, we therefore need to write a 3rd entry in
> this array. Doing so mays the mini-PCIe slot on Beaver operate correctly.
> 
> While we're at it, add some #defines to partially document the fields
> within these 16-bit values.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> This is for Thierry to squash into his Tegra PCIe driver staging branch.
> 
>  drivers/pci/host/pci-tegra.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 2888307..cb069f5 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -195,6 +195,26 @@
>  #define PADS_REFCLK_CFG0			0x000000C8
>  #define PADS_REFCLK_CFG1			0x000000CC
>  
> +/*
> + * Fields in PADS_REFCLK_CFG*. Those registers form an array of 16-bit
> + * entries, one entry per PCIe port. These field definitions and desired
> + * values aren't in the TRM, but do come from NVIDIA.
> + */
> +
> +#define PADS_REFCLK_CFG_REFCLK2_TERM		2  /* 6:2 */
> +#define PADS_REFCLK_CFG_REFCLK2_E_TERM		7
> +#define PADS_REFCLK_CFG_REFCLK2_PREDI		8  /* 11:8 */
> +#define PADS_REFCLK_CFG_REFCLK2_DRVI		12 /* 15:12 */

I'd prefer to make these macros that mask and shift. Either that or
suffix them with _SHIFT to make it more explicit that they shift. Having
an additional mask encoded in the macro has the nice advantage that it
makes the comments obsolete. And why are they named *_REFCLK2_*? Could
they be named PADS_REFCLK_CFG_{TERM,E_TERM,PREDI,DRVI} instead?

Also can we make sure that these make it into the next version of the
TRM? It's good already to have symbolic names, but a description of what
they mean would be even better.

> +/* 0xfa5c */

Maybe change this comment into something like: "default value provided
by hardware engineering: 0xfa5c"?

> +#define PADS_REFCLK_CFG_VALUE \

Perhaps "PADS_REFCLK_CFG_DEFAULT"?

> +	( \
> +		(0x17 << PADS_REFCLK_CFG_REFCLK2_TERM)   | \
> +		(0    << PADS_REFCLK_CFG_REFCLK2_E_TERM) | \
> +		(0xa  << PADS_REFCLK_CFG_REFCLK2_PREDI)  | \
> +		(0xf  << PADS_REFCLK_CFG_REFCLK2_DRVI)     \
> +	)
> +
>  struct tegra_msi {
>  	struct msi_chip chip;
>  	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> @@ -814,11 +834,11 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
>  	value |= PADS_PLL_CTL_RST_B4SM;
>  	pads_writel(pcie, value, soc->pads_pll_ctl);
>  
> -	/*
> -	 * Hack, set the clock voltage to the DEFAULT provided by hw folks.
> -	 * This doesn't exist in the documentation.
> -	 */
> -	pads_writel(pcie, 0xfa5cfa5c, PADS_REFCLK_CFG0);
> +	/* Configure the reference clock driver */
> +	pads_writel(pcie,
> +		PADS_REFCLK_CFG_VALUE | (PADS_REFCLK_CFG_VALUE << 16),
> +		PADS_REFCLK_CFG0);

Nit: Other parts in the driver use a temporary variable to avoid having
to split these calls, like so:

	value = (PADS_REFCLK_CFG_DEFAULT << 16) | PADS_REFCLK_CFG_DEFAULT;
	pads_writel(pcie, value, PADS_REFCLK_CFG0);

> +	pads_writel(pcie, PADS_REFCLK_CFG_VALUE, PADS_REFCLK_CFG1);

If using an intermediary variable as shown above, would it hurt if we
wrote the same value again for ports 2 and 3 instead of leaving the
higher 16 bits unset? Given that we can write the entry for port 2 on
Tegra20 which doesn't have a third one I'd expect the same to be true
for port 3. That way, perhaps if some future generation of Tegra gets
a fourth port the code won't have to be updated. And of course we save
a few characters and instructions by reusing the "value" variable.

Thierry
Stephen Warren - July 1, 2013, 3:52 p.m.
On 06/28/2013 01:33 PM, Thierry Reding wrote:
> On Thu, Jun 27, 2013 at 02:42:02PM -0600, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>> 
>> The registers PADS_REFCLK_CFG are an array of 16-bit data, one
>> entry per PCIe root port. For Tegra30, we therefore need to write
>> a 3rd entry in this array. Doing so mays the mini-PCIe slot on
>> Beaver operate correctly.
>> 
>> While we're at it, add some #defines to partially document the
>> fields within these 16-bit values.

>> diff --git a/drivers/pci/host/pci-tegra.c
>> b/drivers/pci/host/pci-tegra.c

>> +#define PADS_REFCLK_CFG_REFCLK2_TERM		2  /* 6:2 */ +#define
>> PADS_REFCLK_CFG_REFCLK2_E_TERM		7 +#define
>> PADS_REFCLK_CFG_REFCLK2_PREDI		8  /* 11:8 */ +#define
>> PADS_REFCLK_CFG_REFCLK2_DRVI		12 /* 15:12 */
...
> Also can we make sure that these make it into the next version of
> the TRM? It's good already to have symbolic names, but a
> description of what they mean would be even better.

These registers will probably make it into a TRM for a future chip,
but it's quite unlikely we'll release updated TRMs for existing chips:-(

>> +	/* Configure the reference clock driver */ +	pads_writel(pcie, 
>> +		PADS_REFCLK_CFG_VALUE | (PADS_REFCLK_CFG_VALUE << 16), +
>> PADS_REFCLK_CFG0); +	pads_writel(pcie, PADS_REFCLK_CFG_VALUE,
>> PADS_REFCLK_CFG1);
> 
> If using an intermediary variable as shown above, would it hurt if
> we wrote the same value again for ports 2 and 3 instead of leaving
> the higher 16 bits unset? Given that we can write the entry for
> port 2 on Tegra20 which doesn't have a third one I'd expect the
> same to be true for port 3. That way, perhaps if some future
> generation of Tegra gets a fourth port the code won't have to be
> updated. And of course we save a few characters and instructions by
> reusing the "value" variable.

I actually meant to guard the additional write based on the number of
ports in HW. I'd certainly prefer not to write to port 3's registers
when port 3 doesn't exist. I doubt I'd be able to get a solid sign-off
that this would be guaranteed problem-free.

I'll fix up the other issues and repost.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 2888307..cb069f5 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -195,6 +195,26 @@ 
 #define PADS_REFCLK_CFG0			0x000000C8
 #define PADS_REFCLK_CFG1			0x000000CC
 
+/*
+ * Fields in PADS_REFCLK_CFG*. Those registers form an array of 16-bit
+ * entries, one entry per PCIe port. These field definitions and desired
+ * values aren't in the TRM, but do come from NVIDIA.
+ */
+
+#define PADS_REFCLK_CFG_REFCLK2_TERM		2  /* 6:2 */
+#define PADS_REFCLK_CFG_REFCLK2_E_TERM		7
+#define PADS_REFCLK_CFG_REFCLK2_PREDI		8  /* 11:8 */
+#define PADS_REFCLK_CFG_REFCLK2_DRVI		12 /* 15:12 */
+
+/* 0xfa5c */
+#define PADS_REFCLK_CFG_VALUE \
+	( \
+		(0x17 << PADS_REFCLK_CFG_REFCLK2_TERM)   | \
+		(0    << PADS_REFCLK_CFG_REFCLK2_E_TERM) | \
+		(0xa  << PADS_REFCLK_CFG_REFCLK2_PREDI)  | \
+		(0xf  << PADS_REFCLK_CFG_REFCLK2_DRVI)     \
+	)
+
 struct tegra_msi {
 	struct msi_chip chip;
 	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
@@ -814,11 +834,11 @@  static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 	value |= PADS_PLL_CTL_RST_B4SM;
 	pads_writel(pcie, value, soc->pads_pll_ctl);
 
-	/*
-	 * Hack, set the clock voltage to the DEFAULT provided by hw folks.
-	 * This doesn't exist in the documentation.
-	 */
-	pads_writel(pcie, 0xfa5cfa5c, PADS_REFCLK_CFG0);
+	/* Configure the reference clock driver */
+	pads_writel(pcie,
+		PADS_REFCLK_CFG_VALUE | (PADS_REFCLK_CFG_VALUE << 16),
+		PADS_REFCLK_CFG0);
+	pads_writel(pcie, PADS_REFCLK_CFG_VALUE, PADS_REFCLK_CFG1);
 
 	/* wait for the PLL to lock */
 	timeout = 300;