diff mbox series

drm/i915/cnl: Fix CNL macros for Voltage Swing programming

Message ID 20181215003845.6344-1-aditya.swarup@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/cnl: Fix CNL macros for Voltage Swing programming | expand

Commit Message

Aditya Swarup Dec. 15, 2018, 12:38 a.m. UTC 
CNL macros for register groups CNL_PORT_TX_DW2_* / CNL_PORT_TX_DW5_* are
configured incorrectly wrt definition of _CNL_PORT_TX_DW_GRP.

Cc: Clint Taylor <clinton.a.taylor@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Aditya Swarup <aditya.swarup@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Jani Nikula Dec. 31, 2018, 9:03 a.m. UTC | #1 
On Fri, 14 Dec 2018, Aditya Swarup <aditya.swarup@intel.com> wrote:
> CNL macros for register groups CNL_PORT_TX_DW2_* / CNL_PORT_TX_DW5_* are
> configured incorrectly wrt definition of _CNL_PORT_TX_DW_GRP.

What a mess.

Please fix this by making CNL and ICL macros behave the same way. It's
silly to have the macro args reversed between them. I think dw first,
port second.

BR,
Jani.


>
> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Aditya Swarup <aditya.swarup@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0796526dc10f..db1332cd9dcd 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1836,8 +1836,8 @@ enum i915_power_well_id {
>  #define _ICL_PORT_TX_DW_LN(dw, ln, port) (_ICL_COMBOPHY(port) + \
>  					  _ICL_PORT_TX_LN(ln) + 4 * (dw))
>  
> -#define CNL_PORT_TX_DW2_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(2, port))
> -#define CNL_PORT_TX_DW2_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(2, port))
> +#define CNL_PORT_TX_DW2_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(port, 2))
> +#define CNL_PORT_TX_DW2_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(port, 2))
>  #define ICL_PORT_TX_DW2_AUX(port)	_MMIO(_ICL_PORT_TX_DW_AUX(2, port))
>  #define ICL_PORT_TX_DW2_GRP(port)	_MMIO(_ICL_PORT_TX_DW_GRP(2, port))
>  #define ICL_PORT_TX_DW2_LN0(port)	_MMIO(_ICL_PORT_TX_DW_LN(2, 0, port))
> @@ -1869,8 +1869,8 @@ enum i915_power_well_id {
>  #define   CURSOR_COEFF(x)		((x) << 0)
>  #define   CURSOR_COEFF_MASK		(0x3F << 0)
>  
> -#define CNL_PORT_TX_DW5_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(5, port))
> -#define CNL_PORT_TX_DW5_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(5, port))
> +#define CNL_PORT_TX_DW5_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(port, 5))
> +#define CNL_PORT_TX_DW5_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(port, 5))
>  #define ICL_PORT_TX_DW5_AUX(port)	_MMIO(_ICL_PORT_TX_DW_AUX(5, port))
>  #define ICL_PORT_TX_DW5_GRP(port)	_MMIO(_ICL_PORT_TX_DW_GRP(5, port))
>  #define ICL_PORT_TX_DW5_LN0(port)	_MMIO(_ICL_PORT_TX_DW_LN(5, 0, port))
Aditya Swarup Jan. 8, 2019, 7:13 a.m. UTC | #2 
On Mon, Dec 31, 2018 at 11:03:22AM +0200, Jani Nikula wrote:
> On Fri, 14 Dec 2018, Aditya Swarup <aditya.swarup@intel.com> wrote:
> > CNL macros for register groups CNL_PORT_TX_DW2_* / CNL_PORT_TX_DW5_* are
> > configured incorrectly wrt definition of _CNL_PORT_TX_DW_GRP.
> 
> What a mess.
> 
> Please fix this by making CNL and ICL macros behave the same way. It's
> silly to have the macro args reversed between them. I think dw first,
> port second.
> 
> BR,
> Jani.

I believe it should be the other way round for the macros to make sense.
Port first, followed by dw and then ln. For example we can see in the following
macro expansion:
#define _ICL_PORT_CL_DW(dw, port)       (_ICL_COMBOPHY(port) + \
                                         4 * (dw))

port is used before dw and we should follow that sequence. 

I have another patch ready which makes ICL macros consistent with CNL
macros if you approve the sequence of (port, dw, ln).

Let me know if you agree.

Regards,
Aditya Swarup
> 
> 
> >
> > Cc: Clint Taylor <clinton.a.taylor@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Aditya Swarup <aditya.swarup@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 0796526dc10f..db1332cd9dcd 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1836,8 +1836,8 @@ enum i915_power_well_id {
> >  #define _ICL_PORT_TX_DW_LN(dw, ln, port) (_ICL_COMBOPHY(port) + \
> >  					  _ICL_PORT_TX_LN(ln) + 4 * (dw))
> >  
> > -#define CNL_PORT_TX_DW2_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(2, port))
> > -#define CNL_PORT_TX_DW2_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(2, port))
> > +#define CNL_PORT_TX_DW2_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(port, 2))
> > +#define CNL_PORT_TX_DW2_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(port, 2))
> >  #define ICL_PORT_TX_DW2_AUX(port)	_MMIO(_ICL_PORT_TX_DW_AUX(2, port))
> >  #define ICL_PORT_TX_DW2_GRP(port)	_MMIO(_ICL_PORT_TX_DW_GRP(2, port))
> >  #define ICL_PORT_TX_DW2_LN0(port)	_MMIO(_ICL_PORT_TX_DW_LN(2, 0, port))
> > @@ -1869,8 +1869,8 @@ enum i915_power_well_id {
> >  #define   CURSOR_COEFF(x)		((x) << 0)
> >  #define   CURSOR_COEFF_MASK		(0x3F << 0)
> >  
> > -#define CNL_PORT_TX_DW5_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(5, port))
> > -#define CNL_PORT_TX_DW5_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(5, port))
> > +#define CNL_PORT_TX_DW5_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(port, 5))
> > +#define CNL_PORT_TX_DW5_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(port, 5))
> >  #define ICL_PORT_TX_DW5_AUX(port)	_MMIO(_ICL_PORT_TX_DW_AUX(5, port))
> >  #define ICL_PORT_TX_DW5_GRP(port)	_MMIO(_ICL_PORT_TX_DW_GRP(5, port))
> >  #define ICL_PORT_TX_DW5_LN0(port)	_MMIO(_ICL_PORT_TX_DW_LN(5, 0, port))
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
Jani Nikula Jan. 8, 2019, 7:47 a.m. UTC | #3 
On Mon, 07 Jan 2019, Aditya Swarup <aditya.swarup@intel.com> wrote:
> On Mon, Dec 31, 2018 at 11:03:22AM +0200, Jani Nikula wrote:
>> On Fri, 14 Dec 2018, Aditya Swarup <aditya.swarup@intel.com> wrote:
>> > CNL macros for register groups CNL_PORT_TX_DW2_* / CNL_PORT_TX_DW5_* are
>> > configured incorrectly wrt definition of _CNL_PORT_TX_DW_GRP.
>> 
>> What a mess.
>> 
>> Please fix this by making CNL and ICL macros behave the same way. It's
>> silly to have the macro args reversed between them. I think dw first,
>> port second.
>> 
>> BR,
>> Jani.
>
> I believe it should be the other way round for the macros to make sense.
> Port first, followed by dw and then ln. For example we can see in the following
> macro expansion:
> #define _ICL_PORT_CL_DW(dw, port)       (_ICL_COMBOPHY(port) + \
>                                          4 * (dw))
>
> port is used before dw and we should follow that sequence. 

It's not about the usage, it's about semantics. We group primarily by
function, secondarily by port/pipe/transcoder.

BR,
Jani.

>
> I have another patch ready which makes ICL macros consistent with CNL
> macros if you approve the sequence of (port, dw, ln).
>
> Let me know if you agree.
>
> Regards,
> Aditya Swarup
>> 
>> 
>> >
>> > Cc: Clint Taylor <clinton.a.taylor@intel.com>
>> > Cc: Imre Deak <imre.deak@intel.com>
>> > Signed-off-by: Aditya Swarup <aditya.swarup@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_reg.h | 8 ++++----
>> >  1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> > index 0796526dc10f..db1332cd9dcd 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -1836,8 +1836,8 @@ enum i915_power_well_id {
>> >  #define _ICL_PORT_TX_DW_LN(dw, ln, port) (_ICL_COMBOPHY(port) + \
>> >  					  _ICL_PORT_TX_LN(ln) + 4 * (dw))
>> >  
>> > -#define CNL_PORT_TX_DW2_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(2, port))
>> > -#define CNL_PORT_TX_DW2_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(2, port))
>> > +#define CNL_PORT_TX_DW2_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(port, 2))
>> > +#define CNL_PORT_TX_DW2_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(port, 2))
>> >  #define ICL_PORT_TX_DW2_AUX(port)	_MMIO(_ICL_PORT_TX_DW_AUX(2, port))
>> >  #define ICL_PORT_TX_DW2_GRP(port)	_MMIO(_ICL_PORT_TX_DW_GRP(2, port))
>> >  #define ICL_PORT_TX_DW2_LN0(port)	_MMIO(_ICL_PORT_TX_DW_LN(2, 0, port))
>> > @@ -1869,8 +1869,8 @@ enum i915_power_well_id {
>> >  #define   CURSOR_COEFF(x)		((x) << 0)
>> >  #define   CURSOR_COEFF_MASK		(0x3F << 0)
>> >  
>> > -#define CNL_PORT_TX_DW5_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(5, port))
>> > -#define CNL_PORT_TX_DW5_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(5, port))
>> > +#define CNL_PORT_TX_DW5_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(port, 5))
>> > +#define CNL_PORT_TX_DW5_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(port, 5))
>> >  #define ICL_PORT_TX_DW5_AUX(port)	_MMIO(_ICL_PORT_TX_DW_AUX(5, port))
>> >  #define ICL_PORT_TX_DW5_GRP(port)	_MMIO(_ICL_PORT_TX_DW_GRP(5, port))
>> >  #define ICL_PORT_TX_DW5_LN0(port)	_MMIO(_ICL_PORT_TX_DW_LN(5, 0, port))
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0796526dc10f..db1332cd9dcd 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1836,8 +1836,8 @@  enum i915_power_well_id {
 #define _ICL_PORT_TX_DW_LN(dw, ln, port) (_ICL_COMBOPHY(port) + \
 					  _ICL_PORT_TX_LN(ln) + 4 * (dw))
 
-#define CNL_PORT_TX_DW2_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(2, port))
-#define CNL_PORT_TX_DW2_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(2, port))
+#define CNL_PORT_TX_DW2_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(port, 2))
+#define CNL_PORT_TX_DW2_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(port, 2))
 #define ICL_PORT_TX_DW2_AUX(port)	_MMIO(_ICL_PORT_TX_DW_AUX(2, port))
 #define ICL_PORT_TX_DW2_GRP(port)	_MMIO(_ICL_PORT_TX_DW_GRP(2, port))
 #define ICL_PORT_TX_DW2_LN0(port)	_MMIO(_ICL_PORT_TX_DW_LN(2, 0, port))
@@ -1869,8 +1869,8 @@  enum i915_power_well_id {
 #define   CURSOR_COEFF(x)		((x) << 0)
 #define   CURSOR_COEFF_MASK		(0x3F << 0)
 
-#define CNL_PORT_TX_DW5_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(5, port))
-#define CNL_PORT_TX_DW5_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(5, port))
+#define CNL_PORT_TX_DW5_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP(port, 5))
+#define CNL_PORT_TX_DW5_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0(port, 5))
 #define ICL_PORT_TX_DW5_AUX(port)	_MMIO(_ICL_PORT_TX_DW_AUX(5, port))
 #define ICL_PORT_TX_DW5_GRP(port)	_MMIO(_ICL_PORT_TX_DW_GRP(5, port))
 #define ICL_PORT_TX_DW5_LN0(port)	_MMIO(_ICL_PORT_TX_DW_LN(5, 0, port))