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

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Aditya Swarup <aditya.swarup@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915/cnl: Fix CNL macros for Voltage Swing programming
Date: Tue, 08 Jan 2019 09:47:57 +0200	[thread overview]
Message-ID: <875zuz8uaa.fsf@intel.com> (raw)
In-Reply-To: <20190108071314.GA7398@aswarup-desk>

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

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx