diff mbox

[20/43] drm/i915: Use paramtrized WRPLL_CTL()

Message ID 1442595836-23981-21-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä Sept. 18, 2015, 5:03 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      | 2 +-
 drivers/gpu/drm/i915/intel_ddi.c     | 8 ++++----
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

Comments

Jani Nikula Sept. 30, 2015, 1:58 p.m. UTC | #1 
On Fri, 18 Sep 2015, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      | 2 +-
>  drivers/gpu/drm/i915/intel_ddi.c     | 8 ++++----
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 25864ae..a8fb5f7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7253,7 +7253,7 @@ enum skl_disp_power_wells {
>  /* WRPLL */
>  #define WRPLL_CTL1			0x46040
>  #define WRPLL_CTL2			0x46060
> -#define WRPLL_CTL(pll)			(pll == 0 ? WRPLL_CTL1 : WRPLL_CTL2)
> +#define WRPLL_CTL(pll)			_PIPE(pll, WRPLL_CTL1, WRPLL_CTL2)

I hope we never accidentally pass pll >= 2 here... I guess we'll find
out. ;)

I know we do reuse _PIPE like this, but maybe we should have a different
macro that emphasizes the parameter is really not pipe based, but rather
an ID or enumeration. There's value in self-documentating code, and
*gasp* maybe we'll eventually make this stuff type safe too! Basically
it would be an alias for _PIPE(); see _PLANE(). Or maybe _PIPE and
_PLANE could be based on the new one. Anyway, just a thought, this is

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  #define  WRPLL_PLL_ENABLE		(1<<31)
>  #define  WRPLL_PLL_SSC			(1<<28)
>  #define  WRPLL_PLL_NON_SSC		(2<<28)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 399e70e..fb456a4 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1094,10 +1094,10 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
>  		link_clock = 270000;
>  		break;
>  	case PORT_CLK_SEL_WRPLL1:
> -		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL1);
> +		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL(0));
>  		break;
>  	case PORT_CLK_SEL_WRPLL2:
> -		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL2);
> +		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL(1));
>  		break;
>  	case PORT_CLK_SEL_SPLL:
>  		pll = I915_READ(SPLL_CTL) & SPLL_PLL_FREQ_MASK;
> @@ -2485,13 +2485,13 @@ static const struct skl_dpll_regs skl_dpll_regs[3] = {
>  	},
>  	{
>  		/* DPLL 2 */
> -		.ctl = WRPLL_CTL1,
> +		.ctl = WRPLL_CTL(0),
>  		.cfgcr1 = DPLL_CFGCR1(2),
>  		.cfgcr2 = DPLL_CFGCR2(2),
>  	},
>  	{
>  		/* DPLL 3 */
> -		.ctl = WRPLL_CTL2,
> +		.ctl = WRPLL_CTL(1),
>  		.cfgcr1 = DPLL_CFGCR1(3),
>  		.cfgcr2 = DPLL_CFGCR2(3),
>  	},
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 63cf5eb..fa7c10fa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9237,8 +9237,8 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
>  
>  	I915_STATE_WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
>  	I915_STATE_WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL enabled\n");
> -	I915_STATE_WARN(I915_READ(WRPLL_CTL1) & WRPLL_PLL_ENABLE, "WRPLL1 enabled\n");
> -	I915_STATE_WARN(I915_READ(WRPLL_CTL2) & WRPLL_PLL_ENABLE, "WRPLL2 enabled\n");
> +	I915_STATE_WARN(I915_READ(WRPLL_CTL(0)) & WRPLL_PLL_ENABLE, "WRPLL1 enabled\n");
> +	I915_STATE_WARN(I915_READ(WRPLL_CTL(1)) & WRPLL_PLL_ENABLE, "WRPLL2 enabled\n");
>  	I915_STATE_WARN(I915_READ(PCH_PP_STATUS) & PP_ON, "Panel power on\n");
>  	I915_STATE_WARN(I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE,
>  	     "CPU PWM1 enabled\n");
> -- 
> 2.4.6
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Sept. 30, 2015, 2 p.m. UTC | #2 
On Wed, Sep 30, 2015 at 04:58:34PM +0300, Jani Nikula wrote:
> On Fri, 18 Sep 2015, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      | 2 +-
> >  drivers/gpu/drm/i915/intel_ddi.c     | 8 ++++----
> >  drivers/gpu/drm/i915/intel_display.c | 4 ++--
> >  3 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 25864ae..a8fb5f7 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7253,7 +7253,7 @@ enum skl_disp_power_wells {
> >  /* WRPLL */
> >  #define WRPLL_CTL1			0x46040
> >  #define WRPLL_CTL2			0x46060
> > -#define WRPLL_CTL(pll)			(pll == 0 ? WRPLL_CTL1 : WRPLL_CTL2)
> > +#define WRPLL_CTL(pll)			_PIPE(pll, WRPLL_CTL1, WRPLL_CTL2)
> 
> I hope we never accidentally pass pll >= 2 here... I guess we'll find
> out. ;)
> 
> I know we do reuse _PIPE like this, but maybe we should have a different
> macro that emphasizes the parameter is really not pipe based, but rather
> an ID or enumeration. There's value in self-documentating code, and
> *gasp* maybe we'll eventually make this stuff type safe too! Basically
> it would be an alias for _PIPE(); see _PLANE(). Or maybe _PIPE and
> _PLANE could be based on the new one.

Yeah, I suppose some kind of base macro would be good, and then just give it
aliases. I was also thinking of having a macro for the typical
'((base) + (n) * 4' type of stuff.

> Anyway, just a thought, this is
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> >  #define  WRPLL_PLL_ENABLE		(1<<31)
> >  #define  WRPLL_PLL_SSC			(1<<28)
> >  #define  WRPLL_PLL_NON_SSC		(2<<28)
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 399e70e..fb456a4 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1094,10 +1094,10 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
> >  		link_clock = 270000;
> >  		break;
> >  	case PORT_CLK_SEL_WRPLL1:
> > -		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL1);
> > +		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL(0));
> >  		break;
> >  	case PORT_CLK_SEL_WRPLL2:
> > -		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL2);
> > +		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL(1));
> >  		break;
> >  	case PORT_CLK_SEL_SPLL:
> >  		pll = I915_READ(SPLL_CTL) & SPLL_PLL_FREQ_MASK;
> > @@ -2485,13 +2485,13 @@ static const struct skl_dpll_regs skl_dpll_regs[3] = {
> >  	},
> >  	{
> >  		/* DPLL 2 */
> > -		.ctl = WRPLL_CTL1,
> > +		.ctl = WRPLL_CTL(0),
> >  		.cfgcr1 = DPLL_CFGCR1(2),
> >  		.cfgcr2 = DPLL_CFGCR2(2),
> >  	},
> >  	{
> >  		/* DPLL 3 */
> > -		.ctl = WRPLL_CTL2,
> > +		.ctl = WRPLL_CTL(1),
> >  		.cfgcr1 = DPLL_CFGCR1(3),
> >  		.cfgcr2 = DPLL_CFGCR2(3),
> >  	},
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 63cf5eb..fa7c10fa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9237,8 +9237,8 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
> >  
> >  	I915_STATE_WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
> >  	I915_STATE_WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL enabled\n");
> > -	I915_STATE_WARN(I915_READ(WRPLL_CTL1) & WRPLL_PLL_ENABLE, "WRPLL1 enabled\n");
> > -	I915_STATE_WARN(I915_READ(WRPLL_CTL2) & WRPLL_PLL_ENABLE, "WRPLL2 enabled\n");
> > +	I915_STATE_WARN(I915_READ(WRPLL_CTL(0)) & WRPLL_PLL_ENABLE, "WRPLL1 enabled\n");
> > +	I915_STATE_WARN(I915_READ(WRPLL_CTL(1)) & WRPLL_PLL_ENABLE, "WRPLL2 enabled\n");
> >  	I915_STATE_WARN(I915_READ(PCH_PP_STATUS) & PP_ON, "Panel power on\n");
> >  	I915_STATE_WARN(I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE,
> >  	     "CPU PWM1 enabled\n");
> > -- 
> > 2.4.6
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
Ville Syrjälä Oct. 26, 2015, 2:49 p.m. UTC | #3 
On Wed, Sep 30, 2015 at 04:58:34PM +0300, Jani Nikula wrote:
> On Fri, 18 Sep 2015, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      | 2 +-
> >  drivers/gpu/drm/i915/intel_ddi.c     | 8 ++++----
> >  drivers/gpu/drm/i915/intel_display.c | 4 ++--
> >  3 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 25864ae..a8fb5f7 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7253,7 +7253,7 @@ enum skl_disp_power_wells {
> >  /* WRPLL */
> >  #define WRPLL_CTL1			0x46040
> >  #define WRPLL_CTL2			0x46060
> > -#define WRPLL_CTL(pll)			(pll == 0 ? WRPLL_CTL1 : WRPLL_CTL2)
> > +#define WRPLL_CTL(pll)			_PIPE(pll, WRPLL_CTL1, WRPLL_CTL2)
> 
> I hope we never accidentally pass pll >= 2 here... I guess we'll find
> out. ;)
> 
> I know we do reuse _PIPE like this, but maybe we should have a different
> macro that emphasizes the parameter is really not pipe based, but rather
> an ID or enumeration. There's value in self-documentating code, and
> *gasp* maybe we'll eventually make this stuff type safe too! Basically
> it would be an alias for _PIPE(); see _PLANE(). Or maybe _PIPE and
> _PLANE could be based on the new one. Anyway, just a thought, this is
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Merged this one. Thanks for the review.

Note that I had to rebase it a bit due to the SKL_DPLLx change in one
of the earlier patches.

> 
> 
> >  #define  WRPLL_PLL_ENABLE		(1<<31)
> >  #define  WRPLL_PLL_SSC			(1<<28)
> >  #define  WRPLL_PLL_NON_SSC		(2<<28)
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 399e70e..fb456a4 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1094,10 +1094,10 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
> >  		link_clock = 270000;
> >  		break;
> >  	case PORT_CLK_SEL_WRPLL1:
> > -		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL1);
> > +		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL(0));
> >  		break;
> >  	case PORT_CLK_SEL_WRPLL2:
> > -		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL2);
> > +		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL(1));
> >  		break;
> >  	case PORT_CLK_SEL_SPLL:
> >  		pll = I915_READ(SPLL_CTL) & SPLL_PLL_FREQ_MASK;
> > @@ -2485,13 +2485,13 @@ static const struct skl_dpll_regs skl_dpll_regs[3] = {
> >  	},
> >  	{
> >  		/* DPLL 2 */
> > -		.ctl = WRPLL_CTL1,
> > +		.ctl = WRPLL_CTL(0),
> >  		.cfgcr1 = DPLL_CFGCR1(2),
> >  		.cfgcr2 = DPLL_CFGCR2(2),
> >  	},
> >  	{
> >  		/* DPLL 3 */
> > -		.ctl = WRPLL_CTL2,
> > +		.ctl = WRPLL_CTL(1),
> >  		.cfgcr1 = DPLL_CFGCR1(3),
> >  		.cfgcr2 = DPLL_CFGCR2(3),
> >  	},
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 63cf5eb..fa7c10fa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9237,8 +9237,8 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
> >  
> >  	I915_STATE_WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
> >  	I915_STATE_WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL enabled\n");
> > -	I915_STATE_WARN(I915_READ(WRPLL_CTL1) & WRPLL_PLL_ENABLE, "WRPLL1 enabled\n");
> > -	I915_STATE_WARN(I915_READ(WRPLL_CTL2) & WRPLL_PLL_ENABLE, "WRPLL2 enabled\n");
> > +	I915_STATE_WARN(I915_READ(WRPLL_CTL(0)) & WRPLL_PLL_ENABLE, "WRPLL1 enabled\n");
> > +	I915_STATE_WARN(I915_READ(WRPLL_CTL(1)) & WRPLL_PLL_ENABLE, "WRPLL2 enabled\n");
> >  	I915_STATE_WARN(I915_READ(PCH_PP_STATUS) & PP_ON, "Panel power on\n");
> >  	I915_STATE_WARN(I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE,
> >  	     "CPU PWM1 enabled\n");
> > -- 
> > 2.4.6
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 25864ae..a8fb5f7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7253,7 +7253,7 @@  enum skl_disp_power_wells {
 /* WRPLL */
 #define WRPLL_CTL1			0x46040
 #define WRPLL_CTL2			0x46060
-#define WRPLL_CTL(pll)			(pll == 0 ? WRPLL_CTL1 : WRPLL_CTL2)
+#define WRPLL_CTL(pll)			_PIPE(pll, WRPLL_CTL1, WRPLL_CTL2)
 #define  WRPLL_PLL_ENABLE		(1<<31)
 #define  WRPLL_PLL_SSC			(1<<28)
 #define  WRPLL_PLL_NON_SSC		(2<<28)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 399e70e..fb456a4 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1094,10 +1094,10 @@  static void hsw_ddi_clock_get(struct intel_encoder *encoder,
 		link_clock = 270000;
 		break;
 	case PORT_CLK_SEL_WRPLL1:
-		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL1);
+		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL(0));
 		break;
 	case PORT_CLK_SEL_WRPLL2:
-		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL2);
+		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL(1));
 		break;
 	case PORT_CLK_SEL_SPLL:
 		pll = I915_READ(SPLL_CTL) & SPLL_PLL_FREQ_MASK;
@@ -2485,13 +2485,13 @@  static const struct skl_dpll_regs skl_dpll_regs[3] = {
 	},
 	{
 		/* DPLL 2 */
-		.ctl = WRPLL_CTL1,
+		.ctl = WRPLL_CTL(0),
 		.cfgcr1 = DPLL_CFGCR1(2),
 		.cfgcr2 = DPLL_CFGCR2(2),
 	},
 	{
 		/* DPLL 3 */
-		.ctl = WRPLL_CTL2,
+		.ctl = WRPLL_CTL(1),
 		.cfgcr1 = DPLL_CFGCR1(3),
 		.cfgcr2 = DPLL_CFGCR2(3),
 	},
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 63cf5eb..fa7c10fa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9237,8 +9237,8 @@  static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 
 	I915_STATE_WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
 	I915_STATE_WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL enabled\n");
-	I915_STATE_WARN(I915_READ(WRPLL_CTL1) & WRPLL_PLL_ENABLE, "WRPLL1 enabled\n");
-	I915_STATE_WARN(I915_READ(WRPLL_CTL2) & WRPLL_PLL_ENABLE, "WRPLL2 enabled\n");
+	I915_STATE_WARN(I915_READ(WRPLL_CTL(0)) & WRPLL_PLL_ENABLE, "WRPLL1 enabled\n");
+	I915_STATE_WARN(I915_READ(WRPLL_CTL(1)) & WRPLL_PLL_ENABLE, "WRPLL2 enabled\n");
 	I915_STATE_WARN(I915_READ(PCH_PP_STATUS) & PP_ON, "Panel power on\n");
 	I915_STATE_WARN(I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE,
 	     "CPU PWM1 enabled\n");