>-----Original Message----- >From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] >Sent: Wednesday, October 31, 2018 2:29 AM >To: Lucas De Marchi <lucas.de.marchi@xxxxxxxxx> >Cc: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; intel- >gfx@xxxxxxxxxxxxxxxxxxxxx; De Marchi, Lucas <lucas.demarchi@xxxxxxxxx> >Subject: Re: [PATCH] drm/i915/fia: FIA registers offset >implementation. > >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) 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@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
