[Intel-gfx] [PATCH 61/89] drm/i915/skl: Determine enabled PLL and its linkrate/pixel clock
M, Satheeshakrishna
satheeshakrishna.m at intel.com
Wed Oct 1 12:51:51 CEST 2014
On 9/23/2014 1:42 AM, Paulo Zanoni wrote:
> 2014-09-04 8:27 GMT-03:00 Damien Lespiau<damien.lespiau at intel.com>:
>> From: Satheeshakrishna M<satheeshakrishna.m at intel.com>
>>
>> v2: Fixup compilation due to the removal of the intel_ddi_dpll_id enum.
>> And add a fixme about the abuse of pipe_config here.
>>
>> v3: Rebase on top of the hsw_ddi_clock_get() rename (Damien)
>>
>> Signed-off-by: Satheeshakrishna M<satheeshakrishna.m at intel.com> (v1)
>> Signed-off-by: Damien Lespiau<damien.lespiau at intel.com> (v3)
>> Signed-off-by: Daniel Vetter<daniel.vetter at ffwll.ch> (v2)
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 5 ++
>> drivers/gpu/drm/i915/intel_ddi.c | 114 ++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 118 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 2364ece..794d0ba 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6367,6 +6367,7 @@ enum punit_power_well {
>> #define DPLL_CRTL1_LINK_RATE_1620 3
>> #define DPLL_CRTL1_LINK_RATE_1080 4
>> #define DPLL_CRTL1_LINK_RATE_2160 5
>> +#define DPLL_CRTL1_LINK_RATE_SHIFT(id) ((id)*6+1)
> I'd move this to a few lines above, where the MASK and RATE
> definitions are. Possibly reimplement the other macros using the new
> one (if the lines don't look to big/ugly).
I will move it next to MASK and RATE definitions. I didn't really get
what you meant my reimplement here :(
>> /* DPLL control2 */
>> #define DPLL_CTRL2 0x6C05C
>> @@ -6374,6 +6375,7 @@ enum punit_power_well {
>> #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))
>> +#define DPLL_CTRL2_DDI_CLK_SEL_SHIFT(port) (port*3+1)
> Same here: move a few lines above, and possibly reimplement the others
> using the new one. Also, use "(port)" instead of "port", since we
> don't want to risk really-hard-to-debug bugs due to operator
> precedence on those "*" and "+" operations.
Will move up CLK_SEL and add parenthesis. Again didn't get the what to
reimplement here.
>> /* DPLL Status */
>> #define DPLL_STATUS 0x6C060
>> @@ -6400,6 +6402,9 @@ enum punit_power_well {
>> #define DPLL_CFGCR2_PDIV(x) (x<<2)
>> #define DPLL_CFGCR2_CENTRAL_FREQ_MASK (3)
>>
>> +#define GET_CFG_CR1_REG(id) (DPLL1_CFGCR1 + (id - 1) * 8)
>> +#define GET_CFG_CR2_REG(id) (DPLL1_CFGCR2 + (id - 1) * 8)
> The macros above are not really trivial due to the fact that the "id"
> is undefined and confusing. Please convert this to an inline function,
> since what we're actually expecting here is "enum intel_dpll_id",
> which has ID 0 for DPLL 1, which can be super confusing (imagine
> someone passing ID 1 for DPLL 1, not realizing it should be using the
> correct enum...). If we use a function we can specify the correct
> expected enum for the ID type, which helps the programmer find out
> what is the expected thing to pass to the function.
Expectation is that user of this macro should know value passed :)
Anyway, let me try to have a inline function here.
>
>> +
>> enum central_freq {
>> freq_9600 = 0,
>> freq_9000 = 1,
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index e7a5428..b5cfb07 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -649,6 +649,114 @@ static int intel_ddi_calc_wrpll_link(struct drm_i915_private *dev_priv,
>> return (refclk * n * 100) / (p * r);
>> }
>>
>> +static int skl_calc_wrpll_link(struct drm_i915_private *dev_priv,
>> + enum intel_dpll_id dpll)
>> +{
>> + uint32_t cfgcr1_reg, cfgcr2_reg;
>> + uint32_t cfgcr1, cfgcr2;
>> + uint32_t p0, p1, p2, dco_freq;
>> +
>> + cfgcr1_reg = GET_CFG_CR1_REG(dpll);
>> + cfgcr2_reg = GET_CFG_CR2_REG(dpll);
>> +
>> + cfgcr1 = I915_READ(cfgcr1_reg);
>> + cfgcr2 = I915_READ(cfgcr2_reg);
> Bikeshed: I'd probably call these cfgcr{1,2}_val to avoid confusion.
ok
>> +
>> + p0 = (cfgcr2 & DPLL_CFGCR2_PDIV_MASK) >> 2;
>> + p2 = (cfgcr2 & DPLL_CFGCR2_KDIV_MASK) >> 5;
>> +
>> + if (cfgcr2 & DPLL_CFGCR2_QDIV_MODE(1))
>> + p1 = (cfgcr2 & DPLL_CFGCR2_QDIV_RATIO_MASK) >> 8;
>> + else
>> + p1 = 1;
>> +
>> +
>> + switch (p0) {
>> + case pdiv_1:
>> + p0 = 1;
>> + break;
>> + case pdiv_2:
>> + p0 = 2;
>> + break;
>> + case pdiv_3:
>> + p0 = 3;
>> + break;
>> + case pdiv_7:
>> + p0 = 7;
>> + break;
>> + }
>> +
>> + switch (p2) {
>> + case kdiv_5:
>> + p2 = 5;
>> + break;
>> + case kdiv_2:
>> + p2 = 2;
>> + break;
>> + case kdiv_3:
>> + p2 = 3;
>> + break;
>> + case kdiv_1:
>> + p2 = 1;
>> + break;
>> + }
> I really think that if we had something like:
> #define DPLL_CFGCR2_PDIV_7 (4 << 2)
> we'd be able to avoid this "convert to enum and then get the value"
> part, making the function much simpler...
ok
>
>> +
>> + dco_freq = (cfgcr1 & DPLL_CFGCR1_DCO_INTEGER_MASK) * 24 * 1000;
>> +
>> + dco_freq += (((cfgcr1 & DPLL_CFGCR1_DCO_FRACTION_MASK) >> 9) * 24 *
>> + 1000) / 0x8000;
>> +
>> + return dco_freq / (p0 * p1 * p2 * 5);
>> +}
>> +
>> +
>> +static void skl_ddi_clock_get(struct intel_encoder *encoder,
>> + struct intel_crtc_config *pipe_config)
>> +{
>> + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>> + enum port port = intel_ddi_get_encoder_port(encoder);
>> + int link_clock = 0;
>> + uint32_t dpll_ctl1, dpll;
>> +
>> + /* FIXME: This should be tracked in the pipe config. */
>> + dpll = I915_READ(DPLL_CTRL2);
>> + dpll &= DPLL_CTRL2_DDI_CLK_SEL_MASK(port);
>> + dpll >>= DPLL_CTRL2_DDI_CLK_SEL_SHIFT(port);
>> +
>> + dpll_ctl1 = I915_READ(DPLL_CTRL1);
>> +
>> + if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(dpll)) {
>> + link_clock = skl_calc_wrpll_link(dev_priv, dpll);
>> + } else {
>> + link_clock = dpll_ctl1 & DPLL_CRTL1_LINK_RATE_MASK(dpll);
>> + link_clock >>= DPLL_CRTL1_LINK_RATE_SHIFT(dpll);
>> +
>> + switch (link_clock) {
>> + case DPLL_CRTL1_LINK_RATE_810:
>> + link_clock = 81000;
>> + break;
>> + case DPLL_CRTL1_LINK_RATE_1350:
>> + link_clock = 135000;
>> + break;
>> + case DPLL_CRTL1_LINK_RATE_2700:
>> + link_clock = 270000;
>> + break;
> What about 1620 and 1080?
>
>
>> + default:
>> + break;
> We're just silently failing here, which will probably result in later
> WARNs on the HW state readout/check code. So we should probably give a
> WARN() here to make debugging easier :)
Since link_clock is read out from the HW, we'll never end up in default
case. Anyway, I'll add a WARN()
>
>> + }
>> + link_clock *= 2;
>> + }
>> +
>> + pipe_config->port_clock = link_clock;
>> +
>> + if (pipe_config->has_dp_encoder)
>> + pipe_config->adjusted_mode.crtc_clock =
>> + intel_dotclock_calculate(pipe_config->port_clock,
>> + &pipe_config->dp_m_n);
>> + else
>> + pipe_config->adjusted_mode.crtc_clock = pipe_config->port_clock;
>> +}
>> +
>> static void hsw_ddi_clock_get(struct intel_encoder *encoder,
>> struct intel_crtc_config *pipe_config)
>> {
>> @@ -1535,6 +1643,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>> enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
>> u32 temp, flags = 0;
>> + struct drm_device *dev = dev_priv->dev;
>>
>> temp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
>> if (temp & TRANS_DDI_PHSYNC)
>> @@ -1606,7 +1715,10 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>> dev_priv->vbt.edp_bpp = pipe_config->pipe_bpp;
>> }
>>
>> - hsw_ddi_clock_get(encoder, pipe_config);
>> + if (INTEL_INFO(dev)->gen < 9)
> I'm sure Daniel would request a change to "<= 8" instead of "< 9" here :)
Couldn't figure of what the convention is. Will fix this instance :)
> I should probably also complain about the fact that clock calculation
> is a very confusing thing, and I never know which value should be
> assigned where, and I also never know when to multiply by 2 or 5 or
> divide by 10...
>
> Note: not everything mentioned above is a hard requirement for a R-B
> tag. Deciding what's a bikeshed and what's not is left as an exercise
> to the reader.
>
>> + hsw_ddi_clock_get(encoder, pipe_config);
>> + else
>> + skl_ddi_clock_get(encoder, pipe_config);
>> }
>>
>> static void intel_ddi_destroy(struct drm_encoder *encoder)
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list