Re: [Intel-gfx] [PATCH 02/17] drm/i915/icl: add ICL support to cnl_set_procmon_ref_values

2018-02-02 Thread James Ausmus 
On Fri, Feb 02, 2018 at 02:23:04PM -0200, Paulo Zanoni wrote:
> On ICL we have two sets of registers: one for port A and another for
> port B. The set of port A registers is the same as the CNL registers.
> 
> Since the procmon table on ICL is the same we want to reuse the CNL
> function. To do that we add a port argument and make CNL always call
> the function passing port A. This way, we'll be able to easily reuse
> the function on ICL when we add icl_display_core_init().
> 
> v2: Don't use _PICK() when you can use a ternary operator.
> v3: Don't use a ternary operation when you can use _MMIO_PORT (Ville).
> Add an extra comment about why we're passing PORT_A (James).
> 
> Signed-off-by: Paulo Zanoni 

Reviewed-by: James Ausmus 

> ---
>  drivers/gpu/drm/i915/i915_reg.h | 22 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 22 +++---
>  2 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 65ba10ad1fe5..f6e1677e8211 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2104,6 +2104,28 @@ enum i915_power_well_id {
>  #define CNL_PORT_COMP_DW9_MMIO(0x162124)
>  #define CNL_PORT_COMP_DW10   _MMIO(0x162128)
>  
> +#define _ICL_PORT_COMP_DW0_A 0x162100
> +#define _ICL_PORT_COMP_DW0_B 0x6C100
> +#define ICL_PORT_COMP_DW0(port)  _MMIO_PORT(port, 
> _ICL_PORT_COMP_DW0_A, \
> +  _ICL_PORT_COMP_DW0_B)
> +#define _ICL_PORT_COMP_DW1_A 0x162104
> +#define _ICL_PORT_COMP_DW1_B 0x6C104
> +#define ICL_PORT_COMP_DW1(port)  _MMIO_PORT(port, 
> _ICL_PORT_COMP_DW1_A, \
> +  _ICL_PORT_COMP_DW1_B)
> +#define _ICL_PORT_COMP_DW3_A 0x16210C
> +#define _ICL_PORT_COMP_DW3_B 0x6C10C
> +#define ICL_PORT_COMP_DW3(port)  _MMIO_PORT(port, 
> _ICL_PORT_COMP_DW3_A, \
> +  _ICL_PORT_COMP_DW3_B)
> +#define _ICL_PORT_COMP_DW9_A 0x162124
> +#define _ICL_PORT_COMP_DW9_B 0x6C124
> +#define ICL_PORT_COMP_DW9(port)  _MMIO_PORT(port, 
> _ICL_PORT_COMP_DW9_A, \
> +  _ICL_PORT_COMP_DW9_B)
> +#define _ICL_PORT_COMP_DW10_A0x162128
> +#define _ICL_PORT_COMP_DW10_B0x6C128
> +#define ICL_PORT_COMP_DW10(port) _MMIO_PORT(port, \
> +_ICL_PORT_COMP_DW10_A, \
> +_ICL_PORT_COMP_DW10_B)
> +
>  /* BXT PHY Ref registers */
>  #define _PORT_REF_DW3_A  0x16218C
>  #define _PORT_REF_DW3_BC 0x6C18C
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 70e659772a7a..b4ef7875f055 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2794,12 +2794,19 @@ static const struct cnl_procmon {
>   { .dw1 = 0x0044, .dw9 = 0x9A00AB25, .dw10 = 0x8AE38FF1, },
>  };
>  
> -static void cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv)
> +/*
> + * CNL has just one set of registers, while ICL has two sets: one for port A 
> and
> + * the other for port B. The CNL registers are equivalent to the ICL port A
> + * registers, that's why we call the ICL macros even though the function has 
> CNL
> + * on its name.
> + */
> +static void cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv,
> +enum port port)
>  {
>   const struct cnl_procmon *procmon;
>   u32 val;
>  
> - val = I915_READ(CNL_PORT_COMP_DW3);
> + val = I915_READ(ICL_PORT_COMP_DW3(port));
>   switch (val & (PROCESS_INFO_MASK | VOLTAGE_INFO_MASK)) {
>   default:
>   MISSING_CASE(val);
> @@ -2820,13 +2827,13 @@ static void cnl_set_procmon_ref_values(struct 
> drm_i915_private *dev_priv)
>   break;
>   }
>  
> - val = I915_READ(CNL_PORT_COMP_DW1);
> + val = I915_READ(ICL_PORT_COMP_DW1(port));
>   val &= ~((0xff << 16) | 0xff);
>   val |= procmon->dw1;
> - I915_WRITE(CNL_PORT_COMP_DW1, val);
> + I915_WRITE(ICL_PORT_COMP_DW1(port), val);
>  
> - I915_WRITE(CNL_PORT_COMP_DW9, procmon->dw9);
> - I915_WRITE(CNL_PORT_COMP_DW10, procmon->dw10);
> + I915_WRITE(ICL_PORT_COMP_DW9(port), procmon->dw9);
> + I915_WRITE(ICL_PORT_COMP_DW10(port), procmon->dw10);
>  }
>  
>  static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool 
> resume)
> @@ -2847,7 +2854,8 @@ static void cnl_display_core_init(struct 
> drm_i915_private *dev_priv, bool resume
>   val &= ~CNL_COMP_PWR_DOWN;
>   I915_WRITE(CHICKEN_MISC_2, val);
>  
> -

[Intel-gfx] [PATCH 02/17] drm/i915/icl: add ICL support to cnl_set_procmon_ref_values

2018-02-02 Thread Paulo Zanoni 
On ICL we have two sets of registers: one for port A and another for
port B. The set of port A registers is the same as the CNL registers.

Since the procmon table on ICL is the same we want to reuse the CNL
function. To do that we add a port argument and make CNL always call
the function passing port A. This way, we'll be able to easily reuse
the function on ICL when we add icl_display_core_init().

v2: Don't use _PICK() when you can use a ternary operator.
v3: Don't use a ternary operation when you can use _MMIO_PORT (Ville).
Add an extra comment about why we're passing PORT_A (James).

Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/i915_reg.h | 22 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 22 +++---
 2 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 65ba10ad1fe5..f6e1677e8211 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2104,6 +2104,28 @@ enum i915_power_well_id {
 #define CNL_PORT_COMP_DW9  _MMIO(0x162124)
 #define CNL_PORT_COMP_DW10 _MMIO(0x162128)
 
+#define _ICL_PORT_COMP_DW0_A   0x162100
+#define _ICL_PORT_COMP_DW0_B   0x6C100
+#define ICL_PORT_COMP_DW0(port)_MMIO_PORT(port, 
_ICL_PORT_COMP_DW0_A, \
+_ICL_PORT_COMP_DW0_B)
+#define _ICL_PORT_COMP_DW1_A   0x162104
+#define _ICL_PORT_COMP_DW1_B   0x6C104
+#define ICL_PORT_COMP_DW1(port)_MMIO_PORT(port, 
_ICL_PORT_COMP_DW1_A, \
+_ICL_PORT_COMP_DW1_B)
+#define _ICL_PORT_COMP_DW3_A   0x16210C
+#define _ICL_PORT_COMP_DW3_B   0x6C10C
+#define ICL_PORT_COMP_DW3(port)_MMIO_PORT(port, 
_ICL_PORT_COMP_DW3_A, \
+_ICL_PORT_COMP_DW3_B)
+#define _ICL_PORT_COMP_DW9_A   0x162124
+#define _ICL_PORT_COMP_DW9_B   0x6C124
+#define ICL_PORT_COMP_DW9(port)_MMIO_PORT(port, 
_ICL_PORT_COMP_DW9_A, \
+_ICL_PORT_COMP_DW9_B)
+#define _ICL_PORT_COMP_DW10_A  0x162128
+#define _ICL_PORT_COMP_DW10_B  0x6C128
+#define ICL_PORT_COMP_DW10(port)   _MMIO_PORT(port, \
+  _ICL_PORT_COMP_DW10_A, \
+  _ICL_PORT_COMP_DW10_B)
+
 /* BXT PHY Ref registers */
 #define _PORT_REF_DW3_A0x16218C
 #define _PORT_REF_DW3_BC   0x6C18C
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 70e659772a7a..b4ef7875f055 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2794,12 +2794,19 @@ static const struct cnl_procmon {
{ .dw1 = 0x0044, .dw9 = 0x9A00AB25, .dw10 = 0x8AE38FF1, },
 };
 
-static void cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv)
+/*
+ * CNL has just one set of registers, while ICL has two sets: one for port A 
and
+ * the other for port B. The CNL registers are equivalent to the ICL port A
+ * registers, that's why we call the ICL macros even though the function has 
CNL
+ * on its name.
+ */
+static void cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv,
+  enum port port)
 {
const struct cnl_procmon *procmon;
u32 val;
 
-   val = I915_READ(CNL_PORT_COMP_DW3);
+   val = I915_READ(ICL_PORT_COMP_DW3(port));
switch (val & (PROCESS_INFO_MASK | VOLTAGE_INFO_MASK)) {
default:
MISSING_CASE(val);
@@ -2820,13 +2827,13 @@ static void cnl_set_procmon_ref_values(struct 
drm_i915_private *dev_priv)
break;
}
 
-   val = I915_READ(CNL_PORT_COMP_DW1);
+   val = I915_READ(ICL_PORT_COMP_DW1(port));
val &= ~((0xff << 16) | 0xff);
val |= procmon->dw1;
-   I915_WRITE(CNL_PORT_COMP_DW1, val);
+   I915_WRITE(ICL_PORT_COMP_DW1(port), val);
 
-   I915_WRITE(CNL_PORT_COMP_DW9, procmon->dw9);
-   I915_WRITE(CNL_PORT_COMP_DW10, procmon->dw10);
+   I915_WRITE(ICL_PORT_COMP_DW9(port), procmon->dw9);
+   I915_WRITE(ICL_PORT_COMP_DW10(port), procmon->dw10);
 }
 
 static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool 
resume)
@@ -2847,7 +2854,8 @@ static void cnl_display_core_init(struct drm_i915_private 
*dev_priv, bool resume
val &= ~CNL_COMP_PWR_DOWN;
I915_WRITE(CHICKEN_MISC_2, val);
 
-   cnl_set_procmon_ref_values(dev_priv);
+   /* Dummy PORT_A to get the correct CNL register from the ICL macro */
+   cnl_set_procmon_ref_values(dev_priv, PORT_A);
 
val = I915_READ(CNL_PORT_COMP_DW0);
val |= COMP_INIT;
-- 
2.14.3

Re: [Intel-gfx] [PATCH 02/17] drm/i915/icl: add ICL support to cnl_set_procmon_ref_values

2018-01-26 Thread James Ausmus 
On Fri, Jan 26, 2018 at 06:24:32PM -0200, Paulo Zanoni wrote:
> Em Ter, 2018-01-23 às 16:32 -0800, James Ausmus escreveu:
> > On Tue, Jan 23, 2018 at 05:05:21PM -0200, Paulo Zanoni wrote:
> > > On ICL we have two sets of registers: one for port A and another
> > > for
> > > port B. The set of port A registers is the same as the CNL
> > > registers.
> > > 
> > > Since the procmon table on ICL is the same we want to reuse the CNL
> > > function. To do that we add a port argument and make CNL always
> > > call
> > > the function passing port A. This way, we'll be able to easily
> > > reuse
> > > the function on ICL when we add icl_display_core_init().
> > > 
> > > v2: Don't use _PICK() when you can use a ternary operator.
> > > 
> > > Signed-off-by: Paulo Zanoni 
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h | 26
> > > ++
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 21 ++---
> > >  2 files changed, 40 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index d72e206b2b9f..ebf6261d30fd 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -2102,6 +2102,32 @@ enum i915_power_well_id {
> > >  #define CNL_PORT_COMP_DW9_MMIO(0x162124)
> > >  #define CNL_PORT_COMP_DW10   _MMIO(0x162128)
> > >  
> > > +#define _ICL_PORT_COMP_DW0_A 0x162100
> > > +#define _ICL_PORT_COMP_DW0_B 0x6C100
> > > +#define ICL_PORT_COMP_DW0(port)  _MMIO((port ==
> > > PORT_A) ? \
> > > +   _ICL_PORT_COMP_DW0_A
> > > : \
> > > +   _ICL_PORT_COMP_DW0_B
> > > )
> > > +#define _ICL_PORT_COMP_DW1_A 0x162104
> > > +#define _ICL_PORT_COMP_DW1_B 0x6C104
> > > +#define ICL_PORT_COMP_DW1(port)  _MMIO((port ==
> > > PORT_A) ? \
> > > +   _ICL_PORT_COMP_DW1_A
> > > : \
> > > +   _ICL_PORT_COMP_DW1_B
> > > )
> > > +#define _ICL_PORT_COMP_DW3_A 0x16210C
> > > +#define _ICL_PORT_COMP_DW3_B 0x6C10C
> > > +#define ICL_PORT_COMP_DW3(port)  _MMIO((port ==
> > > PORT_A) ? \
> > > +   _ICL_PORT_COMP_DW3_A
> > > : \
> > > +   _ICL_PORT_COMP_DW3_B
> > > )
> > > +#define _ICL_PORT_COMP_DW9_A 0x162124
> > > +#define _ICL_PORT_COMP_DW9_B 0x6C124
> > > +#define ICL_PORT_COMP_DW9(port)  _MMIO((port ==
> > > PORT_A) ? \
> > > +   _ICL_PORT_COMP_DW9_A
> > > : \
> > > +   _ICL_PORT_COMP_DW9_B
> > > )
> > > +#define _ICL_PORT_COMP_DW10_A0x162128
> > > +#define _ICL_PORT_COMP_DW10_B0x6C128
> > > +#define ICL_PORT_COMP_DW10(port) _MMIO((port == PORT_A) ?
> > > \
> > > +   _ICL_PORT_COMP_DW10_
> > > A :   \
> > > +   _ICL_PORT_COMP_DW10_
> > > B)
> > > +
> > >  /* BXT PHY Ref registers */
> > >  #define _PORT_REF_DW3_A  0x16218C
> > >  #define _PORT_REF_DW3_BC 0x6C18C
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 5b1aa4b9c72c..73dd525d241a 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -2758,12 +2758,19 @@ static const struct cnl_procmon {
> > >   { .dw1 = 0x0044, .dw9 = 0x9A00AB25, .dw10 =
> > > 0x8AE38FF1, },
> > >  };
> > >  
> > > -static void cnl_set_procmon_ref_values(struct drm_i915_private
> > > *dev_priv)
> > > +/*
> > > + * CNL has just one set of registers, while ICL has two sets: one
> > > for port A and
> > > + * the other for port B. The CNL registers are equivalent to the
> > > ICL port A
> > > + * registers, that's why we call the ICL macros even though the
> > > function has CNL
> > > + * on its name.
> > > + */
> 
> My reply below refers to this comment here ^.
> 
> 
> 
> 
> > > +static void cnl_set_procmon_ref_values(struct drm_i915_private
> > > *dev_priv,
> > > +enum port port)
> > >  {
> > >   const struct cnl_procmon *procmon;
> > >   u32 val;
> > >  
> > > - val = I915_READ(CNL_PORT_COMP_DW3);
> > > + val = I915_READ(ICL_PORT_COMP_DW3(port));
> > >   switch (val & (PROCESS_INFO_MASK | VOLTAGE_INFO_MASK)) {
> > >   default:
> > >   MISSING_CASE(val);
> > > @@ -2784,13 +2791,13 @@ static void
> > > cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv)
> > >   break;
> > >   }
> > >  
> > > - val = I915_READ(CNL_PORT_COMP_DW1);
> > > + val = I915_READ(ICL_PORT_COMP_DW1(port));
> > >   val &= ~((0xff << 16) | 0xff);
> > >   val |= procmon->dw1;
> > > -

Re: [Intel-gfx] [PATCH 02/17] drm/i915/icl: add ICL support to cnl_set_procmon_ref_values

2018-01-26 Thread Ville Syrjälä 
On Tue, Jan 23, 2018 at 05:05:21PM -0200, Paulo Zanoni wrote:
> On ICL we have two sets of registers: one for port A and another for
> port B. The set of port A registers is the same as the CNL registers.
> 
> Since the procmon table on ICL is the same we want to reuse the CNL
> function. To do that we add a port argument and make CNL always call
> the function passing port A. This way, we'll be able to easily reuse
> the function on ICL when we add icl_display_core_init().
> 
> v2: Don't use _PICK() when you can use a ternary operator.
> 
> Signed-off-by: Paulo Zanoni 
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 26 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 21 ++---
>  2 files changed, 40 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index d72e206b2b9f..ebf6261d30fd 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2102,6 +2102,32 @@ enum i915_power_well_id {
>  #define CNL_PORT_COMP_DW9_MMIO(0x162124)
>  #define CNL_PORT_COMP_DW10   _MMIO(0x162128)
>  
> +#define _ICL_PORT_COMP_DW0_A 0x162100
> +#define _ICL_PORT_COMP_DW0_B 0x6C100
> +#define ICL_PORT_COMP_DW0(port)  _MMIO((port == PORT_A) ?
> \
> +   _ICL_PORT_COMP_DW0_A :\
> +   _ICL_PORT_COMP_DW0_B)

Why not just _MMIO_PORT() ?

> +#define _ICL_PORT_COMP_DW1_A 0x162104
> +#define _ICL_PORT_COMP_DW1_B 0x6C104
> +#define ICL_PORT_COMP_DW1(port)  _MMIO((port == PORT_A) ?
> \
> +   _ICL_PORT_COMP_DW1_A :\
> +   _ICL_PORT_COMP_DW1_B)
> +#define _ICL_PORT_COMP_DW3_A 0x16210C
> +#define _ICL_PORT_COMP_DW3_B 0x6C10C
> +#define ICL_PORT_COMP_DW3(port)  _MMIO((port == PORT_A) ?
> \
> +   _ICL_PORT_COMP_DW3_A :\
> +   _ICL_PORT_COMP_DW3_B)
> +#define _ICL_PORT_COMP_DW9_A 0x162124
> +#define _ICL_PORT_COMP_DW9_B 0x6C124
> +#define ICL_PORT_COMP_DW9(port)  _MMIO((port == PORT_A) ?
> \
> +   _ICL_PORT_COMP_DW9_A :\
> +   _ICL_PORT_COMP_DW9_B)
> +#define _ICL_PORT_COMP_DW10_A0x162128
> +#define _ICL_PORT_COMP_DW10_B0x6C128
> +#define ICL_PORT_COMP_DW10(port) _MMIO((port == PORT_A) ?\
> +   _ICL_PORT_COMP_DW10_A :   \
> +   _ICL_PORT_COMP_DW10_B)
> +
>  /* BXT PHY Ref registers */
>  #define _PORT_REF_DW3_A  0x16218C
>  #define _PORT_REF_DW3_BC 0x6C18C
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 5b1aa4b9c72c..73dd525d241a 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2758,12 +2758,19 @@ static const struct cnl_procmon {
>   { .dw1 = 0x0044, .dw9 = 0x9A00AB25, .dw10 = 0x8AE38FF1, },
>  };
>  
> -static void cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv)
> +/*
> + * CNL has just one set of registers, while ICL has two sets: one for port A 
> and
> + * the other for port B. The CNL registers are equivalent to the ICL port A
> + * registers, that's why we call the ICL macros even though the function has 
> CNL
> + * on its name.
> + */
> +static void cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv,
> +enum port port)
>  {
>   const struct cnl_procmon *procmon;
>   u32 val;
>  
> - val = I915_READ(CNL_PORT_COMP_DW3);
> + val = I915_READ(ICL_PORT_COMP_DW3(port));
>   switch (val & (PROCESS_INFO_MASK | VOLTAGE_INFO_MASK)) {
>   default:
>   MISSING_CASE(val);
> @@ -2784,13 +2791,13 @@ static void cnl_set_procmon_ref_values(struct 
> drm_i915_private *dev_priv)
>   break;
>   }
>  
> - val = I915_READ(CNL_PORT_COMP_DW1);
> + val = I915_READ(ICL_PORT_COMP_DW1(port));
>   val &= ~((0xff << 16) | 0xff);
>   val |= procmon->dw1;
> - I915_WRITE(CNL_PORT_COMP_DW1, val);
> + I915_WRITE(ICL_PORT_COMP_DW1(port), val);
>  
> - I915_WRITE(CNL_PORT_COMP_DW9, procmon->dw9);
> - I915_WRITE(CNL_PORT_COMP_DW10, procmon->dw10);
> + I915_WRITE(ICL_PORT_COMP_DW9(port), procmon->dw9);
> + I915_WRITE(ICL_PORT_COMP_DW10(port), procmon->dw10);
>  }
>  
>  static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool 
> resume)
> @@ -2811,7 +2818,7 @@ static void cnl_display_core_init(struct 
> drm_i915_private *dev_priv, bool resume
>   val &=

Re: [Intel-gfx] [PATCH 02/17] drm/i915/icl: add ICL support to cnl_set_procmon_ref_values

2018-01-26 Thread Paulo Zanoni 
Em Ter, 2018-01-23 às 16:32 -0800, James Ausmus escreveu:
> On Tue, Jan 23, 2018 at 05:05:21PM -0200, Paulo Zanoni wrote:
> > On ICL we have two sets of registers: one for port A and another
> > for
> > port B. The set of port A registers is the same as the CNL
> > registers.
> > 
> > Since the procmon table on ICL is the same we want to reuse the CNL
> > function. To do that we add a port argument and make CNL always
> > call
> > the function passing port A. This way, we'll be able to easily
> > reuse
> > the function on ICL when we add icl_display_core_init().
> > 
> > v2: Don't use _PICK() when you can use a ternary operator.
> > 
> > Signed-off-by: Paulo Zanoni 
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h | 26
> > ++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 21 ++---
> >  2 files changed, 40 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index d72e206b2b9f..ebf6261d30fd 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -2102,6 +2102,32 @@ enum i915_power_well_id {
> >  #define CNL_PORT_COMP_DW9  _MMIO(0x162124)
> >  #define CNL_PORT_COMP_DW10 _MMIO(0x162128)
> >  
> > +#define _ICL_PORT_COMP_DW0_A   0x162100
> > +#define _ICL_PORT_COMP_DW0_B   0x6C100
> > +#define ICL_PORT_COMP_DW0(port)_MMIO((port ==
> > PORT_A) ?   \
> > + _ICL_PORT_COMP_DW0_A
> > :   \
> > + _ICL_PORT_COMP_DW0_B
> > )
> > +#define _ICL_PORT_COMP_DW1_A   0x162104
> > +#define _ICL_PORT_COMP_DW1_B   0x6C104
> > +#define ICL_PORT_COMP_DW1(port)_MMIO((port ==
> > PORT_A) ?   \
> > + _ICL_PORT_COMP_DW1_A
> > :   \
> > + _ICL_PORT_COMP_DW1_B
> > )
> > +#define _ICL_PORT_COMP_DW3_A   0x16210C
> > +#define _ICL_PORT_COMP_DW3_B   0x6C10C
> > +#define ICL_PORT_COMP_DW3(port)_MMIO((port ==
> > PORT_A) ?   \
> > + _ICL_PORT_COMP_DW3_A
> > :   \
> > + _ICL_PORT_COMP_DW3_B
> > )
> > +#define _ICL_PORT_COMP_DW9_A   0x162124
> > +#define _ICL_PORT_COMP_DW9_B   0x6C124
> > +#define ICL_PORT_COMP_DW9(port)_MMIO((port ==
> > PORT_A) ?   \
> > + _ICL_PORT_COMP_DW9_A
> > :   \
> > + _ICL_PORT_COMP_DW9_B
> > )
> > +#define _ICL_PORT_COMP_DW10_A  0x162128
> > +#define _ICL_PORT_COMP_DW10_B  0x6C128
> > +#define ICL_PORT_COMP_DW10(port)   _MMIO((port == PORT_A) ?
> > \
> > + _ICL_PORT_COMP_DW10_
> > A : \
> > + _ICL_PORT_COMP_DW10_
> > B)
> > +
> >  /* BXT PHY Ref registers */
> >  #define _PORT_REF_DW3_A0x16218C
> >  #define _PORT_REF_DW3_BC   0x6C18C
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 5b1aa4b9c72c..73dd525d241a 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -2758,12 +2758,19 @@ static const struct cnl_procmon {
> > { .dw1 = 0x0044, .dw9 = 0x9A00AB25, .dw10 =
> > 0x8AE38FF1, },
> >  };
> >  
> > -static void cnl_set_procmon_ref_values(struct drm_i915_private
> > *dev_priv)
> > +/*
> > + * CNL has just one set of registers, while ICL has two sets: one
> > for port A and
> > + * the other for port B. The CNL registers are equivalent to the
> > ICL port A
> > + * registers, that's why we call the ICL macros even though the
> > function has CNL
> > + * on its name.
> > + */

My reply below refers to this comment here ^.




> > +static void cnl_set_procmon_ref_values(struct drm_i915_private
> > *dev_priv,
> > +  enum port port)
> >  {
> > const struct cnl_procmon *procmon;
> > u32 val;
> >  
> > -   val = I915_READ(CNL_PORT_COMP_DW3);
> > +   val = I915_READ(ICL_PORT_COMP_DW3(port));
> > switch (val & (PROCESS_INFO_MASK | VOLTAGE_INFO_MASK)) {
> > default:
> > MISSING_CASE(val);
> > @@ -2784,13 +2791,13 @@ static void
> > cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv)
> > break;
> > }
> >  
> > -   val = I915_READ(CNL_PORT_COMP_DW1);
> > +   val = I915_READ(ICL_PORT_COMP_DW1(port));
> > val &= ~((0xff << 16) | 0xff);
> > val |= procmon->dw1;
> > -   I915_WRITE(CNL_PORT_COMP_DW1, val);
> > +   I915_WRITE(ICL_PORT_COMP_DW1(port), val);
> >  
> > -   I915_WRITE(CNL_PORT_COMP_DW9, procmon->dw9);
> > -   I915_WRITE(CNL_PORT_COMP_DW10, procmon->dw10);
> > +   I915_WRITE(ICL_PORT_COMP_DW9(port),

Re: [Intel-gfx] [PATCH 02/17] drm/i915/icl: add ICL support to cnl_set_procmon_ref_values

2018-01-23 Thread James Ausmus 
On Tue, Jan 23, 2018 at 05:05:21PM -0200, Paulo Zanoni wrote:
> On ICL we have two sets of registers: one for port A and another for
> port B. The set of port A registers is the same as the CNL registers.
> 
> Since the procmon table on ICL is the same we want to reuse the CNL
> function. To do that we add a port argument and make CNL always call
> the function passing port A. This way, we'll be able to easily reuse
> the function on ICL when we add icl_display_core_init().
> 
> v2: Don't use _PICK() when you can use a ternary operator.
> 
> Signed-off-by: Paulo Zanoni 
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 26 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 21 ++---
>  2 files changed, 40 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index d72e206b2b9f..ebf6261d30fd 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2102,6 +2102,32 @@ enum i915_power_well_id {
>  #define CNL_PORT_COMP_DW9_MMIO(0x162124)
>  #define CNL_PORT_COMP_DW10   _MMIO(0x162128)
>  
> +#define _ICL_PORT_COMP_DW0_A 0x162100
> +#define _ICL_PORT_COMP_DW0_B 0x6C100
> +#define ICL_PORT_COMP_DW0(port)  _MMIO((port == PORT_A) ?
> \
> +   _ICL_PORT_COMP_DW0_A :\
> +   _ICL_PORT_COMP_DW0_B)
> +#define _ICL_PORT_COMP_DW1_A 0x162104
> +#define _ICL_PORT_COMP_DW1_B 0x6C104
> +#define ICL_PORT_COMP_DW1(port)  _MMIO((port == PORT_A) ?
> \
> +   _ICL_PORT_COMP_DW1_A :\
> +   _ICL_PORT_COMP_DW1_B)
> +#define _ICL_PORT_COMP_DW3_A 0x16210C
> +#define _ICL_PORT_COMP_DW3_B 0x6C10C
> +#define ICL_PORT_COMP_DW3(port)  _MMIO((port == PORT_A) ?
> \
> +   _ICL_PORT_COMP_DW3_A :\
> +   _ICL_PORT_COMP_DW3_B)
> +#define _ICL_PORT_COMP_DW9_A 0x162124
> +#define _ICL_PORT_COMP_DW9_B 0x6C124
> +#define ICL_PORT_COMP_DW9(port)  _MMIO((port == PORT_A) ?
> \
> +   _ICL_PORT_COMP_DW9_A :\
> +   _ICL_PORT_COMP_DW9_B)
> +#define _ICL_PORT_COMP_DW10_A0x162128
> +#define _ICL_PORT_COMP_DW10_B0x6C128
> +#define ICL_PORT_COMP_DW10(port) _MMIO((port == PORT_A) ?\
> +   _ICL_PORT_COMP_DW10_A :   \
> +   _ICL_PORT_COMP_DW10_B)
> +
>  /* BXT PHY Ref registers */
>  #define _PORT_REF_DW3_A  0x16218C
>  #define _PORT_REF_DW3_BC 0x6C18C
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 5b1aa4b9c72c..73dd525d241a 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2758,12 +2758,19 @@ static const struct cnl_procmon {
>   { .dw1 = 0x0044, .dw9 = 0x9A00AB25, .dw10 = 0x8AE38FF1, },
>  };
>  
> -static void cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv)
> +/*
> + * CNL has just one set of registers, while ICL has two sets: one for port A 
> and
> + * the other for port B. The CNL registers are equivalent to the ICL port A
> + * registers, that's why we call the ICL macros even though the function has 
> CNL
> + * on its name.
> + */
> +static void cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv,
> +enum port port)
>  {
>   const struct cnl_procmon *procmon;
>   u32 val;
>  
> - val = I915_READ(CNL_PORT_COMP_DW3);
> + val = I915_READ(ICL_PORT_COMP_DW3(port));
>   switch (val & (PROCESS_INFO_MASK | VOLTAGE_INFO_MASK)) {
>   default:
>   MISSING_CASE(val);
> @@ -2784,13 +2791,13 @@ static void cnl_set_procmon_ref_values(struct 
> drm_i915_private *dev_priv)
>   break;
>   }
>  
> - val = I915_READ(CNL_PORT_COMP_DW1);
> + val = I915_READ(ICL_PORT_COMP_DW1(port));
>   val &= ~((0xff << 16) | 0xff);
>   val |= procmon->dw1;
> - I915_WRITE(CNL_PORT_COMP_DW1, val);
> + I915_WRITE(ICL_PORT_COMP_DW1(port), val);
>  
> - I915_WRITE(CNL_PORT_COMP_DW9, procmon->dw9);
> - I915_WRITE(CNL_PORT_COMP_DW10, procmon->dw10);
> + I915_WRITE(ICL_PORT_COMP_DW9(port), procmon->dw9);
> + I915_WRITE(ICL_PORT_COMP_DW10(port), procmon->dw10);
>  }
>  
>  static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool 
> resume)
> @@ -2811,7 +2818,7 @@ static void cnl_display_core_init(struct 
> drm_i915_private *dev_priv, bool resume
>   val &= ~CNL_COMP_PWR_DOWN;
>

[Intel-gfx] [PATCH 02/17] drm/i915/icl: add ICL support to cnl_set_procmon_ref_values

2018-01-23 Thread Paulo Zanoni 
On ICL we have two sets of registers: one for port A and another for
port B. The set of port A registers is the same as the CNL registers.

Since the procmon table on ICL is the same we want to reuse the CNL
function. To do that we add a port argument and make CNL always call
the function passing port A. This way, we'll be able to easily reuse
the function on ICL when we add icl_display_core_init().

v2: Don't use _PICK() when you can use a ternary operator.

Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/i915_reg.h | 26 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 21 ++---
 2 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index d72e206b2b9f..ebf6261d30fd 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2102,6 +2102,32 @@ enum i915_power_well_id {
 #define CNL_PORT_COMP_DW9  _MMIO(0x162124)
 #define CNL_PORT_COMP_DW10 _MMIO(0x162128)
 
+#define _ICL_PORT_COMP_DW0_A   0x162100
+#define _ICL_PORT_COMP_DW0_B   0x6C100
+#define ICL_PORT_COMP_DW0(port)_MMIO((port == PORT_A) ?
\
+ _ICL_PORT_COMP_DW0_A :\
+ _ICL_PORT_COMP_DW0_B)
+#define _ICL_PORT_COMP_DW1_A   0x162104
+#define _ICL_PORT_COMP_DW1_B   0x6C104
+#define ICL_PORT_COMP_DW1(port)_MMIO((port == PORT_A) ?
\
+ _ICL_PORT_COMP_DW1_A :\
+ _ICL_PORT_COMP_DW1_B)
+#define _ICL_PORT_COMP_DW3_A   0x16210C
+#define _ICL_PORT_COMP_DW3_B   0x6C10C
+#define ICL_PORT_COMP_DW3(port)_MMIO((port == PORT_A) ?
\
+ _ICL_PORT_COMP_DW3_A :\
+ _ICL_PORT_COMP_DW3_B)
+#define _ICL_PORT_COMP_DW9_A   0x162124
+#define _ICL_PORT_COMP_DW9_B   0x6C124
+#define ICL_PORT_COMP_DW9(port)_MMIO((port == PORT_A) ?
\
+ _ICL_PORT_COMP_DW9_A :\
+ _ICL_PORT_COMP_DW9_B)
+#define _ICL_PORT_COMP_DW10_A  0x162128
+#define _ICL_PORT_COMP_DW10_B  0x6C128
+#define ICL_PORT_COMP_DW10(port)   _MMIO((port == PORT_A) ?\
+ _ICL_PORT_COMP_DW10_A :   \
+ _ICL_PORT_COMP_DW10_B)
+
 /* BXT PHY Ref registers */
 #define _PORT_REF_DW3_A0x16218C
 #define _PORT_REF_DW3_BC   0x6C18C
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 5b1aa4b9c72c..73dd525d241a 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2758,12 +2758,19 @@ static const struct cnl_procmon {
{ .dw1 = 0x0044, .dw9 = 0x9A00AB25, .dw10 = 0x8AE38FF1, },
 };
 
-static void cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv)
+/*
+ * CNL has just one set of registers, while ICL has two sets: one for port A 
and
+ * the other for port B. The CNL registers are equivalent to the ICL port A
+ * registers, that's why we call the ICL macros even though the function has 
CNL
+ * on its name.
+ */
+static void cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv,
+  enum port port)
 {
const struct cnl_procmon *procmon;
u32 val;
 
-   val = I915_READ(CNL_PORT_COMP_DW3);
+   val = I915_READ(ICL_PORT_COMP_DW3(port));
switch (val & (PROCESS_INFO_MASK | VOLTAGE_INFO_MASK)) {
default:
MISSING_CASE(val);
@@ -2784,13 +2791,13 @@ static void cnl_set_procmon_ref_values(struct 
drm_i915_private *dev_priv)
break;
}
 
-   val = I915_READ(CNL_PORT_COMP_DW1);
+   val = I915_READ(ICL_PORT_COMP_DW1(port));
val &= ~((0xff << 16) | 0xff);
val |= procmon->dw1;
-   I915_WRITE(CNL_PORT_COMP_DW1, val);
+   I915_WRITE(ICL_PORT_COMP_DW1(port), val);
 
-   I915_WRITE(CNL_PORT_COMP_DW9, procmon->dw9);
-   I915_WRITE(CNL_PORT_COMP_DW10, procmon->dw10);
+   I915_WRITE(ICL_PORT_COMP_DW9(port), procmon->dw9);
+   I915_WRITE(ICL_PORT_COMP_DW10(port), procmon->dw10);
 }
 
 static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool 
resume)
@@ -2811,7 +2818,7 @@ static void cnl_display_core_init(struct drm_i915_private 
*dev_priv, bool resume
val &= ~CNL_COMP_PWR_DOWN;
I915_WRITE(CHICKEN_MISC_2, val);
 
-   cnl_set_procmon_ref_values(dev_priv);
+   cnl_set_procmon_ref_values(dev_priv, PORT_A);
 
val = I915_READ(CNL_PORT_COMP_DW0);
val |= COMP_INIT;
--