Re: [PATCH] drm/i915/cnl: Fix CNL macros for Voltage Swing programming

From: Aditya Swarup <aditya.swarup@intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915/cnl: Fix CNL macros for Voltage Swing programming
Date: Mon, 7 Jan 2019 23:13:14 -0800	[thread overview]
Message-ID: <20190108071314.GA7398@aswarup-desk> (raw)
In-Reply-To: <877efqukyt.fsf@intel.com>

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx