On Tue, 30 Oct 2018, Lucas De Marchi <lucas.de.marchi@xxxxxxxxx> wrote:
> On Tue, Oct 30, 2018 at 6:56 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
>>
>> On Mon, 29 Oct 2018, Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> 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@xxxxxxxxx>
>> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
>> > ---
>> > 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@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
