Re: [PATCH V3 05/12] PCI: tegra: Program UPHY electrical settings in Tegra210

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5367 bytes --]

On Mon, Oct 30, 2017 at 07:27:16PM +0530, Manikanta Maddireddy wrote:
> UPHY electrical programming guidelines are documented in Tegra210 TRM.
> Program these electrical settings for proper eye diagram in all link
> speeds.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> V3:
> * Added shift operation for readability
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 97 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 46896aaab81d..c862facfd6e9 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -184,6 +184,32 @@
>  
>  #define AFI_PEXBIAS_CTRL_0		0x168
>  
> +#define RP_ECTL_2_R1	0xe84
> +#define  RP_ECTL_2_R1_RX_CTLE_1C_MASK		0xffff
> +
> +#define RP_ECTL_4_R1	0xe8c
> +#define  RP_ECTL_4_R1_RX_CDR_CTRL_1C_MASK	(0xffff << 16)
> +#define  RP_ECTL_4_R1_RX_CDR_CTRL_1C_SHIFT	16
> +
> +#define RP_ECTL_5_R1	0xe90
> +#define  RP_ECTL_5_R1_RX_EQ_CTRL_L_1C_MASK	0xffffffff
> +
> +#define RP_ECTL_6_R1	0xe94
> +#define  RP_ECTL_6_R1_RX_EQ_CTRL_H_1C_MASK	0xffffffff
> +
> +#define RP_ECTL_2_R2	0xea4
> +#define  RP_ECTL_2_R2_RX_CTLE_1C_MASK		0xffff
> +
> +#define RP_ECTL_4_R2	0xeac
> +#define  RP_ECTL_4_R2_RX_CDR_CTRL_1C_MASK	(0xffff << 16)
> +#define  RP_ECTL_4_R2_RX_CDR_CTRL_1C_SHIFT	16
> +
> +#define RP_ECTL_5_R2	0xeb0
> +#define  RP_ECTL_5_R2_RX_EQ_CTRL_L_1C_MASK	0xffffffff
> +
> +#define RP_ECTL_6_R2	0xeb4
> +#define  RP_ECTL_6_R2_RX_EQ_CTRL_H_1C_MASK	0xffffffff
> +
>  #define RP_VEND_XP	0x00000f00
>  #define  RP_VEND_XP_DL_UP	(1 << 30)
>  
> @@ -254,6 +280,14 @@ struct tegra_pcie_soc {
>  	u32 tx_ref_sel;
>  	u32 pads_refclk_cfg0;
>  	u32 pads_refclk_cfg1;
> +	u32 rp_ectl_2_r1;
> +	u32 rp_ectl_4_r1;
> +	u32 rp_ectl_5_r1;
> +	u32 rp_ectl_6_r1;
> +	u32 rp_ectl_2_r2;
> +	u32 rp_ectl_4_r2;
> +	u32 rp_ectl_5_r2;
> +	u32 rp_ectl_6_r2;
>  	bool has_pex_clkreq_en;
>  	bool has_pex_bias_ctrl;
>  	bool has_intr_prsnt_sense;
> @@ -261,6 +295,7 @@ struct tegra_pcie_soc {
>  	bool has_gen2;
>  	bool force_pca_enable;
>  	bool program_uphy;
> +	bool program_ectl_settings;

Could you move all of the ECTL related fields under a substructure for
better readability? Something like:

	struct {
		struct {
			u32 ...;
			...
		} regs;

		bool enable;
	} ectl;

>  };
>  
>  static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
> @@ -2058,6 +2093,52 @@ static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
>  		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
>  }
>  
> +static void tegra_pcie_program_ectl_settings(struct tegra_pcie_port *port)
> +{
> +	unsigned long value;

u32, please.

> +	const struct tegra_pcie_soc *soc = port->pcie->soc;
> +
> +	value = readl(port->base + RP_ECTL_2_R1);
> +	value &= ~RP_ECTL_2_R1_RX_CTLE_1C_MASK;
> +	value |= soc->rp_ectl_2_r1;
> +	writel(value, port->base + RP_ECTL_2_R1);
> +
> +	value = readl(port->base + RP_ECTL_4_R1);
> +	value &= ~RP_ECTL_4_R1_RX_CDR_CTRL_1C_MASK;
> +	value |= (soc->rp_ectl_4_r1 << RP_ECTL_4_R1_RX_CDR_CTRL_1C_SHIFT);
> +	writel(value, port->base + RP_ECTL_4_R1);
> +
> +	value = readl(port->base + RP_ECTL_5_R1);
> +	value &= ~RP_ECTL_5_R1_RX_EQ_CTRL_L_1C_MASK;
> +	value |= soc->rp_ectl_5_r1;
> +	writel(value, port->base + RP_ECTL_5_R1);
> +
> +	value = readl(port->base + RP_ECTL_6_R1);
> +	value &= ~RP_ECTL_6_R1_RX_EQ_CTRL_H_1C_MASK;
> +	value |= soc->rp_ectl_6_r1;
> +	writel(value, port->base + RP_ECTL_6_R1);
> +
> +	value = readl(port->base + RP_ECTL_2_R2);
> +	value &= ~RP_ECTL_2_R2_RX_CTLE_1C_MASK;
> +	value |= soc->rp_ectl_2_r2;
> +	writel(value, port->base + RP_ECTL_2_R2);
> +
> +	value = readl(port->base + RP_ECTL_4_R2);
> +	value &= ~RP_ECTL_4_R2_RX_CDR_CTRL_1C_MASK;
> +	value |= (soc->rp_ectl_4_r2 << RP_ECTL_4_R2_RX_CDR_CTRL_1C_SHIFT);
> +	writel(value, port->base + RP_ECTL_4_R2);
> +
> +	value = readl(port->base + RP_ECTL_5_R2);
> +	value &= ~RP_ECTL_5_R2_RX_EQ_CTRL_L_1C_MASK;
> +	value |= soc->rp_ectl_5_r2;
> +	writel(value, port->base + RP_ECTL_5_R2);
> +
> +	value = readl(port->base + RP_ECTL_6_R2);
> +	value &= ~RP_ECTL_6_R2_RX_EQ_CTRL_H_1C_MASK;
> +	value |= soc->rp_ectl_6_r2;
> +	writel(value, port->base + RP_ECTL_6_R2);
> +}
> +
>  static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
>  {
>  	unsigned long value;
> @@ -2125,6 +2206,7 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
>  	struct tegra_pcie_port *port, *tmp;
> +	const struct tegra_pcie_soc *soc = pcie->soc;
>  
>  	tegra_pcie_apply_pad_settings(pcie);
>  
> @@ -2133,6 +2215,8 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>  			 port->index, port->lanes);
>  
>  		tegra_pcie_port_enable(port);
> +		if (soc->program_ectl_settings)
> +			tegra_pcie_program_ectl_settings(port);

Perhaps this should be moved into tegra_pcie_port_enable()? That way the
programming is done in one central location. If ever we need to enable
the ports from somewhere else we could simply call it instead of having
to duplicate enabling all the extra features.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]