drm/i915/fia: FIA registers offset implementation.
diff mbox series

Message ID 20181029232315.15539-1-anusha.srivatsa@intel.com
State New
Headers show
Series
  • drm/i915/fia: FIA registers offset implementation.
Related show

Commit Message

Srivatsa, Anusha Oct. 29, 2018, 11:23 p.m. UTC
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.

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(-)

Comments

Lucas De Marchi Oct. 29, 2018, 11:34 p.m. UTC | #1
On Mon, Oct 29, 2018 at 04:23:15PM -0700, Anusha Srivatsa 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.
> 
> 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>

Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

> ---
>  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
> +#define PORT_TX_DFLEXDPMLE1_OFFSET	0x008C0
> +#define PORT_TX_DFLEXDPPMS_OFFSET	0x00890
> +#define PORT_TX_DFLEXDPCSSS_OFFSET	0x00894
> +#define PORT_TX_DFLEXDPSP_OFFSET	0x008A0
> +
>  /* ICL PHY DFLEX registers */
> -#define PORT_TX_DFLEXDPMLE1		_MMIO(0x1638C0)
> +#define PORT_TX_DFLEXDPMLE1		_MMIO(FIA1_BASE + PORT_TX_DFLEXDPMLE1_OFFSET)
>  #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_ */
> -- 
> 2.17.1
>
Jani Nikula Oct. 30, 2018, 1:57 p.m. UTC | #2
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.

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
> +#define PORT_TX_DFLEXDPMLE1_OFFSET	0x008C0
> +#define PORT_TX_DFLEXDPPMS_OFFSET	0x00890
> +#define PORT_TX_DFLEXDPCSSS_OFFSET	0x00894
> +#define PORT_TX_DFLEXDPSP_OFFSET	0x008A0
> +
>  /* ICL PHY DFLEX registers */
> -#define PORT_TX_DFLEXDPMLE1		_MMIO(0x1638C0)
> +#define PORT_TX_DFLEXDPMLE1		_MMIO(FIA1_BASE + PORT_TX_DFLEXDPMLE1_OFFSET)
>  #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_ */
Lucas De Marchi Oct. 30, 2018, 3:15 p.m. UTC | #3
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?

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
> > +#define PORT_TX_DFLEXDPMLE1_OFFSET   0x008C0
> > +#define PORT_TX_DFLEXDPPMS_OFFSET    0x00890
> > +#define PORT_TX_DFLEXDPCSSS_OFFSET   0x00894
> > +#define PORT_TX_DFLEXDPSP_OFFSET     0x008A0
> > +
> >  /* ICL PHY DFLEX registers */
> > -#define PORT_TX_DFLEXDPMLE1          _MMIO(0x1638C0)
> > +#define PORT_TX_DFLEXDPMLE1          _MMIO(FIA1_BASE + PORT_TX_DFLEXDPMLE1_OFFSET)
> >  #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 Oct. 31, 2018, 9:28 a.m. UTC | #4
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
Srivatsa, Anusha Oct. 31, 2018, 4:58 p.m. UTC | #5
>-----Original Message-----
>From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>Sent: Wednesday, October 31, 2018 2:29 AM
>To: Lucas De Marchi <lucas.de.marchi@gmail.com>
>Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
>gfx@lists.freedesktop.org; De Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: Re: [Intel-gfx] [PATCH] drm/i915/fia: FIA registers offset
>implementation.
>
>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)

Makes sense. I ll make this change...

Anusha
>
>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
Lucas De Marchi Oct. 31, 2018, 5:23 p.m. UTC | #6
On Wed, Oct 31, 2018 at 04:58:27PM +0000, Srivatsa, Anusha wrote:
> >>> >  /* 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)
> 
> Makes sense. I ll make this change...

I think the second option makes more sense. The additional underscored name
doesn't improve readability.

Lucas De Marchi
Srivatsa, Anusha Oct. 31, 2018, 5:33 p.m. UTC | #7
>-----Original Message-----
>From: Lucas De Marchi [mailto:lucas.de.marchi@gmail.com]
>Sent: Wednesday, October 31, 2018 10:23 AM
>To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
>Cc: Jani Nikula <jani.nikula@linux.intel.com>; intel-gfx@lists.freedesktop.org; De
>Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: Re: [Intel-gfx] [PATCH] drm/i915/fia: FIA registers offset
>implementation.
>
>On Wed, Oct 31, 2018 at 04:58:27PM +0000, Srivatsa, Anusha wrote:
>> >>> >  /* 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)
>>
>> Makes sense. I ll make this change...
>
>I think the second option makes more sense. The additional underscored name
>doesn't improve readability.

Yes, so no special additional defines for offsets, just - 
#define PORT_TX_DFLEXDPMLE1		_MMIO(FIA1_BASE + 0x008C0)

Anusha 
>Lucas De Marchi

Patch
diff mbox series

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
+#define PORT_TX_DFLEXDPMLE1_OFFSET	0x008C0
+#define PORT_TX_DFLEXDPPMS_OFFSET	0x00890
+#define PORT_TX_DFLEXDPCSSS_OFFSET	0x00894
+#define PORT_TX_DFLEXDPSP_OFFSET	0x008A0
+
 /* ICL PHY DFLEX registers */
-#define PORT_TX_DFLEXDPMLE1		_MMIO(0x1638C0)
+#define PORT_TX_DFLEXDPMLE1		_MMIO(FIA1_BASE + PORT_TX_DFLEXDPMLE1_OFFSET)
 #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_ */