Re: [PATCH 58/89] drm/i915/skl: Register definitions for SKL Clocks

["M, Satheeshakrishna" <satheeshakrishna.m@intel.com>]

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
>