Re: [PATCH] drm/i915/fia: FIA registers offset implementation.

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Lucas De Marchi <lucas.de.marchi@gmail.com>
Cc: intel-gfx@lists.freedesktop.org,
	Lucas De Marchi <lucas.demarchi@intel.com>
Subject: Re: [PATCH] drm/i915/fia: FIA registers offset implementation.
Date: Wed, 31 Oct 2018 11:28:47 +0200	[thread overview]
Message-ID: <877ehympm8.fsf@intel.com> (raw)
In-Reply-To: <CAKi4VAJpC9NU6dFujwPi1Uqew0my0bVH2coJsTSJN-GJqncT4Q@mail.gmail.com>

On Tue, 30 Oct 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> On Tue, Oct 30, 2018 at 6:56 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>
>> On Mon, 29 Oct 2018, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote:
>> > The registers DPCSSS,DPSP,DPMLE1 and DPPMS are all at an offset
>> > from the base - which is the FLexi IO Adaptor. Lets follow the
>> > offset calculation while accessing these registers.
>>
>> Why?
>>
>> If I search the specs or i915_reg.h for, say, 0x1638c0 I'll find what
>> I'm looking for.
>>
>> We generally don't follow this type of definitions for registers. We may
>> have some, but those are exceptions.
>
> Because spec 29550 treats those registers as a base + offset to be more
> future proof regarding a change of the base. And yes, I think something
> like that needs to be stated in the commit message.  Is this enough?

Fair enough. Please also see comments in-line.

>
> Lucas De Marchi
>
>>
>> Please don't do this without some pretty good rationale written in the
>> commit message.
>>
>> BR,
>> Jani.
>>
>> >
>> > v2:
>> > - Follow spec for numbering - s/0/1(Lucas)
>> > - s/FIA_1/FIA1_BASE (Anusha)
>> >
>> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_reg.h | 15 +++++++++++----
>> >  1 file changed, 11 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> > index bcee91bcfba6..dd74bc01c64e 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -2057,8 +2057,15 @@ enum i915_power_well_id {
>> >  #define BXT_PORT_CL2CM_DW6(phy)              _BXT_PHY((phy), _PORT_CL2CM_DW6_BC)
>> >  #define   DW6_OLDO_DYN_PWR_DOWN_EN   (1 << 28)
>> >
>> > +/* FIA Offsets */
>> > +#define FIA1_BASE                    0x163000

Ok.

>> > +#define PORT_TX_DFLEXDPMLE1_OFFSET   0x008C0
>> > +#define PORT_TX_DFLEXDPPMS_OFFSET    0x00890
>> > +#define PORT_TX_DFLEXDPCSSS_OFFSET   0x00894
>> > +#define PORT_TX_DFLEXDPSP_OFFSET     0x008A0

These should not be grouped here, instead please add above each macro
below or just leave out, see below.

>> > +
>> >  /* ICL PHY DFLEX registers */
>> > -#define PORT_TX_DFLEXDPMLE1          _MMIO(0x1638C0)
>> > +#define PORT_TX_DFLEXDPMLE1          _MMIO(FIA1_BASE + PORT_TX_DFLEXDPMLE1_OFFSET)

IMO either:

#define _PORT_TX_DFLEXDPMLE1		0x008C0
#define PORT_TX_DFLEXDPMLE1		_MMIO(FIA1_BASE + _PORT_TX_DFLEXDPMLE1)

or just:

#define PORT_TX_DFLEXDPMLE1		_MMIO(FIA1_BASE + 0x008C0)

BR,
Jani.

>> >  #define   DFLEXDPMLE1_DPMLETC_MASK(n)        (0xf << (4 * (n)))
>> >  #define   DFLEXDPMLE1_DPMLETC(n, x)  ((x) << (4 * (n)))
>> >
>> > @@ -10988,17 +10995,17 @@ enum skl_power_gate {
>> >                                               _ICL_DSC1_RC_BUF_THRESH_1_UDW_PB, \
>> >                                               _ICL_DSC1_RC_BUF_THRESH_1_UDW_PC)
>> >
>> > -#define PORT_TX_DFLEXDPSP                    _MMIO(0x1638A0)
>> > +#define PORT_TX_DFLEXDPSP                    _MMIO(FIA1_BASE + PORT_TX_DFLEXDPSP_OFFSET)
>> >  #define   TC_LIVE_STATE_TBT(tc_port)         (1 << ((tc_port) * 8 + 6))
>> >  #define   TC_LIVE_STATE_TC(tc_port)          (1 << ((tc_port) * 8 + 5))
>> >  #define   DP_LANE_ASSIGNMENT_SHIFT(tc_port)  ((tc_port) * 8)
>> >  #define   DP_LANE_ASSIGNMENT_MASK(tc_port)   (0xf << ((tc_port) * 8))
>> >  #define   DP_LANE_ASSIGNMENT(tc_port, x)     ((x) << ((tc_port) * 8))
>> >
>> > -#define PORT_TX_DFLEXDPPMS                           _MMIO(0x163890)
>> > +#define PORT_TX_DFLEXDPPMS                           _MMIO(FIA1_BASE + PORT_TX_DFLEXDPPMS_OFFSET)
>> >  #define   DP_PHY_MODE_STATUS_COMPLETED(tc_port)              (1 << (tc_port))
>> >
>> > -#define PORT_TX_DFLEXDPCSSS                          _MMIO(0x163894)
>> > +#define PORT_TX_DFLEXDPCSSS                  _MMIO(FIA1_BASE + PORT_TX_DFLEXDPCSSS_OFFSET)
>> >  #define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)               (1 << (tc_port))
>> >
>> >  #endif /* _I915_REG_H_ */
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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