| Message ID | 1409830075-11139-59-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-09-04 8:27 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>: > From: Satheeshakrishna M <satheeshakrishna.m@intel.com> > > This patch defines the necessary SKL registers for implementing the > new clocking mechanism. > > v2: Addressed review comments by Damien > - Added code comment > - Introduced enum for WRPLL values > > v3: Rebase on top of nightly (minor conflict in i915_reg.h) > > v4: Use 0x, not 0X (Ville) Bikeshed: One of my worries with this patch is that names like CDCLK_CTL and DPLL_CTRL1 are too generic. Basically every platform has a CDCLK and DPLLs... Maybe adding _SKL in their names would help a little. Or maybe we don't need it, since the functions using these defines will already be surrounded by IS_SKL+ checks... > > Signed-off-by: Satheeshakrishna M <satheeshakrishna.m@intel.com> (v2) > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> (v3,v4) > --- > drivers/gpu/drm/i915/i915_reg.h | 84 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 84 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 417075d..2364ece 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6336,6 +6336,90 @@ enum punit_power_well { > #define LCPLL_CD_SOURCE_FCLK (1<<21) > #define LCPLL_CD_SOURCE_FCLK_DONE (1<<19) > > +/* > + * SKL Clocks > + */ > + > +/* CDCLK_CTL */ > +#define CDCLK_CTL 0x46000 > +#define CDCLK_FREQ_SEL_MASK (3<<26) > +#define CDCLK_FREQ_450_432 (0<<26) > +#define CDCLK_FREQ_540 (1<<26) > +#define CDCLK_FREQ_337_308 (2<<26) > +#define CDCLK_FREQ_675_617 (3<<26) > +#define CDCLK_FREQ_DECIMAL_MASK (0x7ff) <ocd> Please replace the tab for a space on the line above. </ocd> > + > +/* LCPLL_CTL */ > +#define LCPLL1_CTL 0x46010 > +#define LCPLL2_CTL 0x46014 > +#define LCPLL_PLL_ENABLE (1<<31) > + > +/* DPLL control1 */ > +#define DPLL_CTRL1 0x6C058 > +#define DPLL_CTRL1_HDMI_MODE(id) (1<<((id)*6+5)) > +#define DPLL_CTRL1_SSC(id) (1<<((id)*6+4)) > +#define DPLL_CRTL1_LINK_RATE_MASK(id) (7<<((id)*6+1)) > +#define DPLL_CRTL1_LINK_RATE(linkrate, id) ((linkrate)<<((id)*6+1)) > +#define DPLL_CTRL1_OVERRIDE(id) (1<<((id)*6)) > +#define DPLL_CRTL1_LINK_RATE_2700 0 > +#define DPLL_CRTL1_LINK_RATE_1350 1 > +#define DPLL_CRTL1_LINK_RATE_810 2 > +#define DPLL_CRTL1_LINK_RATE_1620 3 > +#define DPLL_CRTL1_LINK_RATE_1080 4 > +#define DPLL_CRTL1_LINK_RATE_2160 5 > + > +/* DPLL control2 */ > +#define DPLL_CTRL2 0x6C05C > +#define DPLL_CTRL2_DDI_CLK_OFF(port) (1<<(port+15)) > +#define DPLL_CTRL2_DDI_CLK_SEL_MASK(port) (3<<((port)*3+1)) > +#define DPLL_CTRL2_DDI_CLK_SEL(clk, port) (clk<<((port)*3+1)) > +#define DPLL_CTRL2_DDI_SEL_OVERRIDE(port) (1<<(port*3)) > + > +/* DPLL Status */ > +#define DPLL_STATUS 0x6C060 > +#define DPLL_LOCK(id) (1<<((id)*8)) Bad tab here too. > + > +/* DPLL cfg */ > +#define DPLL1_CFGCR1 0x6C040 > +#define DPLL2_CFGCR1 0x6C048 > +#define DPLL3_CFGCR1 0x6C050 > +#define DPLL_CFGCR1_FREQ_ENABLE (1<<31) > +#define DPLL_CFGCR1_DCO_FRACTION_MASK (0x7fff<<9) > +#define DPLL_CFGCR1_DCO_FRACTION(x) (x<<9) > +#define DPLL_CFGCR1_DCO_INTEGER_MASK (0x1ff) > + > +#define DPLL1_CFGCR2 0x6C044 > +#define DPLL2_CFGCR2 0x6C04C > +#define DPLL3_CFGCR2 0x6C054 > +#define DPLL_CFGCR2_QDIV_RATIO_MASK (0xff<<8) > +#define DPLL_CFGCR2_QDIV_RATIO(x) (x<<8) > +#define DPLL_CFGCR2_QDIV_MODE(x) (x<<7) > +#define DPLL_CFGCR2_KDIV_MASK (3<<5) > +#define DPLL_CFGCR2_KDIV(x) (x<<5) > +#define DPLL_CFGCR2_PDIV_MASK (7<<2) > +#define DPLL_CFGCR2_PDIV(x) (x<<2) > +#define DPLL_CFGCR2_CENTRAL_FREQ_MASK (3) > + > +enum central_freq { > + freq_9600 = 0, > + freq_9000 = 1, > + freq_8400 = 3, > +}; > + > +enum pdiv { > + pdiv_1 = 0, > + pdiv_2 = 1, > + pdiv_3 = 2, > + pdiv_7 = 4, > +}; > + > +enum kdiv { > + kdiv_5 = 0, > + kdiv_2 = 1, > + kdiv_3 = 2, > + kdiv_1 = 3, > +}; > + I find it weird that we're using enums here. We don't have the tradition to do this, so it's a deviation from the usual coding style. With or without changes: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register, > * since on HSW we can't write to it using I915_WRITE. */ > #define D_COMP_HSW (MCHBAR_MIRROR_BASE_SNB + 0x5F0C) > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 9/22/2014 11:47 PM, Paulo Zanoni wrote: > 2014-09-04 8:27 GMT-03:00 Damien Lespiau<damien.lespiau@intel.com>: >> From: Satheeshakrishna M<satheeshakrishna.m@intel.com> >> >> This patch defines the necessary SKL registers for implementing the >> new clocking mechanism. >> >> v2: Addressed review comments by Damien >> - Added code comment >> - Introduced enum for WRPLL values >> >> v3: Rebase on top of nightly (minor conflict in i915_reg.h) >> >> v4: Use 0x, not 0X (Ville) > Bikeshed: One of my worries with this patch is that names like > CDCLK_CTL and DPLL_CTRL1 are too generic. Basically every platform has > a CDCLK and DPLLs... Maybe adding _SKL in their names would help a > little. Or maybe we don't need it, since the functions using these > defines will already be surrounded by IS_SKL+ checks... Tried to keep the names to match bspec. I haven't any instance where register name remains same and offset changes from one platform to another. >> Signed-off-by: Satheeshakrishna M<satheeshakrishna.m@intel.com> (v2) >> Signed-off-by: Damien Lespiau<damien.lespiau@intel.com> (v3,v4) >> --- >> drivers/gpu/drm/i915/i915_reg.h | 84 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 84 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 417075d..2364ece 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -6336,6 +6336,90 @@ enum punit_power_well { >> #define LCPLL_CD_SOURCE_FCLK (1<<21) >> #define LCPLL_CD_SOURCE_FCLK_DONE (1<<19) >> >> +/* >> + * SKL Clocks >> + */ >> + >> +/* CDCLK_CTL */ >> +#define CDCLK_CTL 0x46000 >> +#define CDCLK_FREQ_SEL_MASK (3<<26) >> +#define CDCLK_FREQ_450_432 (0<<26) >> +#define CDCLK_FREQ_540 (1<<26) >> +#define CDCLK_FREQ_337_308 (2<<26) >> +#define CDCLK_FREQ_675_617 (3<<26) >> +#define CDCLK_FREQ_DECIMAL_MASK (0x7ff) > <ocd> Please replace the tab for a space on the line above. </ocd> ok >> + >> +/* LCPLL_CTL */ >> +#define LCPLL1_CTL 0x46010 >> +#define LCPLL2_CTL 0x46014 >> +#define LCPLL_PLL_ENABLE (1<<31) >> + >> +/* DPLL control1 */ >> +#define DPLL_CTRL1 0x6C058 >> +#define DPLL_CTRL1_HDMI_MODE(id) (1<<((id)*6+5)) >> +#define DPLL_CTRL1_SSC(id) (1<<((id)*6+4)) >> +#define DPLL_CRTL1_LINK_RATE_MASK(id) (7<<((id)*6+1)) >> +#define DPLL_CRTL1_LINK_RATE(linkrate, id) ((linkrate)<<((id)*6+1)) >> +#define DPLL_CTRL1_OVERRIDE(id) (1<<((id)*6)) >> +#define DPLL_CRTL1_LINK_RATE_2700 0 >> +#define DPLL_CRTL1_LINK_RATE_1350 1 >> +#define DPLL_CRTL1_LINK_RATE_810 2 >> +#define DPLL_CRTL1_LINK_RATE_1620 3 >> +#define DPLL_CRTL1_LINK_RATE_1080 4 >> +#define DPLL_CRTL1_LINK_RATE_2160 5 >> + >> +/* DPLL control2 */ >> +#define DPLL_CTRL2 0x6C05C >> +#define DPLL_CTRL2_DDI_CLK_OFF(port) (1<<(port+15)) >> +#define DPLL_CTRL2_DDI_CLK_SEL_MASK(port) (3<<((port)*3+1)) >> +#define DPLL_CTRL2_DDI_CLK_SEL(clk, port) (clk<<((port)*3+1)) >> +#define DPLL_CTRL2_DDI_SEL_OVERRIDE(port) (1<<(port*3)) >> + >> +/* DPLL Status */ >> +#define DPLL_STATUS 0x6C060 >> +#define DPLL_LOCK(id) (1<<((id)*8)) > Bad tab here too. ok, will take care of this. > >> + >> +/* DPLL cfg */ >> +#define DPLL1_CFGCR1 0x6C040 >> +#define DPLL2_CFGCR1 0x6C048 >> +#define DPLL3_CFGCR1 0x6C050 >> +#define DPLL_CFGCR1_FREQ_ENABLE (1<<31) >> +#define DPLL_CFGCR1_DCO_FRACTION_MASK (0x7fff<<9) >> +#define DPLL_CFGCR1_DCO_FRACTION(x) (x<<9) >> +#define DPLL_CFGCR1_DCO_INTEGER_MASK (0x1ff) >> + >> +#define DPLL1_CFGCR2 0x6C044 >> +#define DPLL2_CFGCR2 0x6C04C >> +#define DPLL3_CFGCR2 0x6C054 >> +#define DPLL_CFGCR2_QDIV_RATIO_MASK (0xff<<8) >> +#define DPLL_CFGCR2_QDIV_RATIO(x) (x<<8) >> +#define DPLL_CFGCR2_QDIV_MODE(x) (x<<7) >> +#define DPLL_CFGCR2_KDIV_MASK (3<<5) >> +#define DPLL_CFGCR2_KDIV(x) (x<<5) >> +#define DPLL_CFGCR2_PDIV_MASK (7<<2) >> +#define DPLL_CFGCR2_PDIV(x) (x<<2) >> +#define DPLL_CFGCR2_CENTRAL_FREQ_MASK (3) >> + >> +enum central_freq { >> + freq_9600 = 0, >> + freq_9000 = 1, >> + freq_8400 = 3, >> +}; >> + >> +enum pdiv { >> + pdiv_1 = 0, >> + pdiv_2 = 1, >> + pdiv_3 = 2, >> + pdiv_7 = 4, >> +}; >> + >> +enum kdiv { >> + kdiv_5 = 0, >> + kdiv_2 = 1, >> + kdiv_3 = 2, >> + kdiv_1 = 3, >> +}; >> + > I find it weird that we're using enums here. We don't have the > tradition to do this, so it's a deviation from the usual coding style. ok, will change this to #define > With or without changes: > Reviewed-by: Paulo Zanoni<paulo.r.zanoni@intel.com> > >> /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register, >> * since on HSW we can't write to it using I915_WRITE. */ >> #define D_COMP_HSW (MCHBAR_MIRROR_BASE_SNB + 0x5F0C) >> -- >> 1.8.3.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 417075d..2364ece 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6336,6 +6336,90 @@ enum punit_power_well { #define LCPLL_CD_SOURCE_FCLK (1<<21) #define LCPLL_CD_SOURCE_FCLK_DONE (1<<19) +/* + * SKL Clocks + */ + +/* CDCLK_CTL */ +#define CDCLK_CTL 0x46000 +#define CDCLK_FREQ_SEL_MASK (3<<26) +#define CDCLK_FREQ_450_432 (0<<26) +#define CDCLK_FREQ_540 (1<<26) +#define CDCLK_FREQ_337_308 (2<<26) +#define CDCLK_FREQ_675_617 (3<<26) +#define CDCLK_FREQ_DECIMAL_MASK (0x7ff) + +/* LCPLL_CTL */ +#define LCPLL1_CTL 0x46010 +#define LCPLL2_CTL 0x46014 +#define LCPLL_PLL_ENABLE (1<<31) + +/* DPLL control1 */ +#define DPLL_CTRL1 0x6C058 +#define DPLL_CTRL1_HDMI_MODE(id) (1<<((id)*6+5)) +#define DPLL_CTRL1_SSC(id) (1<<((id)*6+4)) +#define DPLL_CRTL1_LINK_RATE_MASK(id) (7<<((id)*6+1)) +#define DPLL_CRTL1_LINK_RATE(linkrate, id) ((linkrate)<<((id)*6+1)) +#define DPLL_CTRL1_OVERRIDE(id) (1<<((id)*6)) +#define DPLL_CRTL1_LINK_RATE_2700 0 +#define DPLL_CRTL1_LINK_RATE_1350 1 +#define DPLL_CRTL1_LINK_RATE_810 2 +#define DPLL_CRTL1_LINK_RATE_1620 3 +#define DPLL_CRTL1_LINK_RATE_1080 4 +#define DPLL_CRTL1_LINK_RATE_2160 5 + +/* DPLL control2 */ +#define DPLL_CTRL2 0x6C05C +#define DPLL_CTRL2_DDI_CLK_OFF(port) (1<<(port+15)) +#define DPLL_CTRL2_DDI_CLK_SEL_MASK(port) (3<<((port)*3+1)) +#define DPLL_CTRL2_DDI_CLK_SEL(clk, port) (clk<<((port)*3+1)) +#define DPLL_CTRL2_DDI_SEL_OVERRIDE(port) (1<<(port*3)) + +/* DPLL Status */ +#define DPLL_STATUS 0x6C060 +#define DPLL_LOCK(id) (1<<((id)*8)) + +/* DPLL cfg */ +#define DPLL1_CFGCR1 0x6C040 +#define DPLL2_CFGCR1 0x6C048 +#define DPLL3_CFGCR1 0x6C050 +#define DPLL_CFGCR1_FREQ_ENABLE (1<<31) +#define DPLL_CFGCR1_DCO_FRACTION_MASK (0x7fff<<9) +#define DPLL_CFGCR1_DCO_FRACTION(x) (x<<9) +#define DPLL_CFGCR1_DCO_INTEGER_MASK (0x1ff) + +#define DPLL1_CFGCR2 0x6C044 +#define DPLL2_CFGCR2 0x6C04C +#define DPLL3_CFGCR2 0x6C054 +#define DPLL_CFGCR2_QDIV_RATIO_MASK (0xff<<8) +#define DPLL_CFGCR2_QDIV_RATIO(x) (x<<8) +#define DPLL_CFGCR2_QDIV_MODE(x) (x<<7) +#define DPLL_CFGCR2_KDIV_MASK (3<<5) +#define DPLL_CFGCR2_KDIV(x) (x<<5) +#define DPLL_CFGCR2_PDIV_MASK (7<<2) +#define DPLL_CFGCR2_PDIV(x) (x<<2) +#define DPLL_CFGCR2_CENTRAL_FREQ_MASK (3) + +enum central_freq { + freq_9600 = 0, + freq_9000 = 1, + freq_8400 = 3, +}; + +enum pdiv { + pdiv_1 = 0, + pdiv_2 = 1, + pdiv_3 = 2, + pdiv_7 = 4, +}; + +enum kdiv { + kdiv_5 = 0, + kdiv_2 = 1, + kdiv_3 = 2, + kdiv_1 = 3, +}; + /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register, * since on HSW we can't write to it using I915_WRITE. */ #define D_COMP_HSW (MCHBAR_MIRROR_BASE_SNB + 0x5F0C)