On Tue, Mar 17, 2015 at 11:39:56AM +0200, Imre Deak wrote:
> From: Vandana Kannan <vandana.kannan@xxxxxxxxx>
>
> Add display clock/PHY initialization sequence as per BSpec.
This should really be two patches I think. I'll go over the cdclk bits
first...
>
> Until GOP/VBIOS provides an upper limit value for CDCLK, comparing clock
> value with 624 MHz and returning 0 in case it exceeds.
>
> Note that the CD clock and PHY initialization/uninitialization are done
> at their current place only for simplicity, in a future patch - when more
> of the runtime PM features will be enabled - these will be moved to
> power well#1 and modeset encoder enabling/disabling hooks respectively.
> This also means that atm dynamic power gating power well #2 is
> effectively disabled.
>
> v1: Added function definitions in header files
> v2: Imre's review comments addressed
> - Moved CDCLK related definitions to i915_reg.h
> - Removed defintions for CDCLK frequency
> - Split uninit_cdclk() by adding a phy_uninit function
> - Calculate freq and decimal based on input frequency
> - Program SSA precharge based on input frequency
> - Use wait_for 1ms instead 200us udelay for DE PLL locking
> - Removed initial value for divider, freq, decimal, ratio.
> - Replaced polling loops with wait_for
> - Parameterized latency optim setting
> - Fix the parts where DE PLL has to be disabled.
> - Call CDCLK selection from mode set
>
> v3: (imre)
> - add note about the plan to move the cdclk/phy init to a better place
> - take rps.hw_lock around pcode access
> - fix DDI PHY timeout value
> - squash in Vandana's "PORT_CL2CM_DW6_A BUN fix",
> "DDI PHY programming register defn", "Do ddi_phy_init always",
> "Check CDCLK upper limit" patches
> - move PHY register macros next to the corresponding CHV/VLV macros
> - move DE PLL register macros here from another patch since they are
> used here first
> - add BXT_ prefix to CDCLK flags
> - s/COMMON_RESET/COMMON_RESET_DIS/ and clarify related code comments
> - fix incorrect read value for the RMW of BXT_PHY_CTL_FAMILY_DDI
> - fix using GT_DISPLAY_EDP_POWER_ON vs. GT_DISPLAY_DDI_POWER_ON
> when powering on DDI ports
> - fix incorrect port when setting BXT_PORT_TX_DW14_LN for DDI ports
> - add missing masking when programming CDCLK_FREQ_DECIMAL
> - add missing powering on for DDI-C port, rename OCL2_LDOFUSE_PWR_EN
> to OCL2_LDOFUSE_PWR_DIS to reduce confusion
> - add note about mismatch with bspec in the PORT_REF_DW6 fields
> - factor out PHY init code to a new function, so we can call it for
> PHY_A and PHY_BC, instead of open-coding the same
>
> Signed-off-by: Vandana Kannan <vandana.kannan@xxxxxxxxx> (v2)
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 126 +++++++++++++++
> drivers/gpu/drm/i915/intel_ddi.c | 291 +++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_display.c | 75 +++++++++
> drivers/gpu/drm/i915/intel_drv.h | 4 +
> 4 files changed, 496 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b4474d3..a3579c0 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
<snip>
> @@ -5326,6 +5430,9 @@ enum skl_disp_power_wells {
> #define DISP_FBC_WM_DIS (1<<15)
> #define DISP_ARB_CTL2 0x45004
> #define DISP_DATA_PARTITION_5_6 (1<<6)
> +#define DBUF_CTL 0x45008
> +#define DBUF_POWER_REQUEST (1<<31)
> +#define DBUF_POWER_STATE (1<<30)
> #define GEN7_MSG_CTL 0x45010
> #define WAIT_FOR_PCH_RESET_ACK (1<<1)
> #define WAIT_FOR_PCH_FLR_ACK (1<<0)
> @@ -6271,6 +6378,7 @@ enum skl_disp_power_wells {
> #define GEN6_PCODE_WRITE_D_COMP 0x11
> #define GEN6_ENCODE_RC6_VID(mv) (((mv) - 245) / 5)
> #define GEN6_DECODE_RC6_VID(vids) (((vids) * 5) + 245)
> +#define DISPLAY_PCU_CONTROL 0x17
I called this HSW_PCODE_DE_WRITE_FREQ_REQ in my HSW/BDW cdclk
patches, which matches the name in Bspec for HSW/BDW.
Given our established practice of naming things based on the oldest
platform supporting them, I'd go with the HSW/BDW name.
> #define DISPLAY_IPS_CONTROL 0x19
> #define HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL 0x1A
> #define GEN6_PCODE_DATA 0x138128
> @@ -6748,6 +6856,13 @@ enum skl_disp_power_wells {
> #define CDCLK_FREQ_675_617 (3<<26)
> #define CDCLK_FREQ_DECIMAL_MASK (0x7ff)
>
> +#define BXT_CDCLK_CD2X_DIV_SEL_MASK (3<<22)
> +#define BXT_CDCLK_CD2X_DIV_SEL_1 (0<<22)
> +#define BXT_CDCLK_CD2X_DIV_SEL_1_5 (1<<22)
> +#define BXT_CDCLK_CD2X_DIV_SEL_2 (2<<22)
> +#define BXT_CDCLK_CD2X_DIV_SEL_4 (3<<22)
> +#define BXT_CDCLK_SSA_PRECHARGE_ENABLE (1<<16)
> +
> /* LCPLL_CTL */
> #define LCPLL1_CTL 0x46010
> #define LCPLL2_CTL 0x46014
> @@ -6812,6 +6927,17 @@ enum skl_disp_power_wells {
> #define GET_CFG_CR1_REG(id) (DPLL1_CFGCR1 + (id - SKL_DPLL1) * 8)
> #define GET_CFG_CR2_REG(id) (DPLL1_CFGCR2 + (id - SKL_DPLL1) * 8)
>
> +/* BXT display engine PLL */
> +#define BXT_DE_PLL_CTL 0x6d000
> +#define BXT_DE_PLL_RATIO_1152 0x3c
> +#define BXT_DE_PLL_RATIO_1248 0x41
> +#define BXT_DE_PLL_RATIO_DEFAULT 0x64
These are just 60,65,100 decimal in hex. So I might make this
something like:
#define BXT_DE_PLL_RATIO(x) (x) /* {60,65,100} * 19.2MHz */
> +#define BXT_DE_PLL_RATIO_MASK 0x7f
0xff
> +
> +#define BXT_DE_PLL_ENABLE 0x46070
> +#define BXT_DE_PLL_PLL_ENABLE (1 << 31)
> +#define BXT_DE_PLL_LOCK (1 << 30)
> +
> /* 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)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index a203d9d..789682d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1957,6 +1957,294 @@ static void skl_shared_dplls_init(struct drm_i915_private *dev_priv)
<snip>
> +
> +/*
> + * It is the responsibility of the caller to ensure that
> + * criteria for changing the CD clk frequency is met.
Not sure that note helps anything. It applies to all platforms anyway,
not just BXT.
> + *
> + * This function only changes CD clock frequency.
> + * TODO:- 1. Add functions to change only the divider and
> + * 2. call impacted functions like backlight, WiDi, watermark.
This TODO can be dropped. We'll do all that stuff naturally as part of
the crtc enable, so nothing needed here I think.
> +*/
> +void bxt_select_cdclk_freq(struct drm_device *dev, u32 frequency)
We call it foo_set_cdclk() on other platforms. Also we usually use just
a plain old int for most frequency variables.
Also I'd put the code into intel_display.c as that's where we have the
rest of the cdclk stuff, and that's where I was going to put the HSW/BDW
code too.
If people are into that code movement stuff someone could extract all the
cdclk stuff to some new file to reduce intel_display.c a bit.
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + uint32_t cdclk_ctl, decimal, ratio;
> + uint32_t divider, freq, current_freq;
> + int ret;
> +
> + freq = (frequency / 1000 - 1) * 2;
This will lose <1MHz bits. So rewrite as
(frequency - 1000) / 500
or
((frequency - 1000) << 1) / 1000
if you want to advertize the .1 fixed point fact a bit more.
or maybe
frequency / 500 - (1 << 1)
or
(frequency << 1) / 1000 - (1 << 1)
> + decimal = DIV_ROUND_UP(frequency, 25000);
I'd call this pcu_freq or somesuch. 'decimal' doesn't really
make much sense.
> +
> + switch (frequency) {
> + case 144000:
> + divider = BXT_CDCLK_CD2X_DIV_SEL_4;
> + ratio = BXT_DE_PLL_RATIO_1152;
> + break;
> + case 288000:
> + divider = BXT_CDCLK_CD2X_DIV_SEL_2;
> + ratio = BXT_DE_PLL_RATIO_1152;
> + break;
> + case 384000:
> + divider = BXT_CDCLK_CD2X_DIV_SEL_1_5;
> + ratio = BXT_DE_PLL_RATIO_1152;
> + break;
> + case 576000:
> + divider = BXT_CDCLK_CD2X_DIV_SEL_1;
> + ratio = BXT_DE_PLL_RATIO_1152;
> + break;
> + case 624000:
> + divider = BXT_CDCLK_CD2X_DIV_SEL_1;
> + ratio = BXT_DE_PLL_RATIO_1248;
> + break;
> + case 0:
> + /* Incase incoming frequency is 0, only DE PLL has to be
> + * disabled, divider/ratio need not be programmed.
> + * Hence, initializing to 0.
> + */
> + divider = ratio = 0;
> + break;
> + default:
> + DRM_ERROR("Unsupported cd frequency %d enable request",
> + frequency);
> + return;
> + }
> +
> + current_freq = I915_READ(CDCLK_CTL) & CDCLK_FREQ_DECIMAL_MASK;
> + current_freq = ((current_freq / 2) + 1) * 1000;
Again we lose <1MHz bits here.
> +
> + mutex_lock(&dev_priv->rps.hw_lock);
> + /* Inform power controller of upcoming frequency change */
> + ret = sandybridge_pcode_write(dev_priv, DISPLAY_PCU_CONTROL,
> + 0x80000000);
I can't see any name for the magic value in Bspec. So I guess we'll
leave it as is.
> + mutex_unlock(&dev_priv->rps.hw_lock);
> +
> + if (ret) {
> + DRM_DEBUG_KMS("pcode write failed, leaving CDCLK unchanged (%d)\n",
> + ret);
Sounds like DRM_ERROR() would be in order.
> + return;
> + }
> +
> + /* DE PLL has to be disabled when input frequency is 0 or
> + * frequency is to be changed to 624MHz or changed from 624 MHz
> + */
> + if (!frequency || current_freq == 624000 || frequency == 624000) {
> + I915_WRITE(BXT_DE_PLL_ENABLE, ~BXT_DE_PLL_PLL_ENABLE);
> + WARN(wait_for(
> + !(I915_READ(BXT_DE_PLL_ENABLE) & BXT_DE_PLL_LOCK),
> + 1), "DE PLL locked\n");
DRM_ERROR() might be enough here. The backtrace won't give is much more
information I think. Maybe add a note that the timeout required is
at least 200us.
> + }
> +
> + if (frequency) {
> + I915_WRITE(BXT_DE_PLL_CTL, ratio);
The spec says we must not change the other fields in the register, so
perhaps we should do an RMW here? Although the default is documented to
be 0 for everything else, but maybe that could change on some devices.
> + I915_WRITE(BXT_DE_PLL_ENABLE, BXT_DE_PLL_PLL_ENABLE);
> + WARN(wait_for(
> + I915_READ(BXT_DE_PLL_ENABLE) & BXT_DE_PLL_LOCK, 1),
> + "DE PLL not locked\n");
Again, not sure the WARN buys us anything extra. Again might mention
that the timeout required is 200us.
> +
> + cdclk_ctl = I915_READ(CDCLK_CTL);
> + cdclk_ctl &= ~BXT_CDCLK_CD2X_DIV_SEL_MASK;
> + cdclk_ctl |= divider;
> +
> + /* Disable SSA Precharge when CD clock frequency < 500 MHz,
> + * enable otherwise.
> + */
> + cdclk_ctl &= ~BXT_CDCLK_SSA_PRECHARGE_ENABLE;
> + if (frequency >= 500000)
> + cdclk_ctl |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
> +
> + cdclk_ctl &= ~CDCLK_FREQ_DECIMAL_MASK;
> + cdclk_ctl |= freq;
> + I915_WRITE(CDCLK_CTL, cdclk_ctl);
> +
> + mutex_lock(&dev_priv->rps.hw_lock);
> + ret = sandybridge_pcode_write(dev_priv, DISPLAY_PCU_CONTROL,
> + decimal);
> + mutex_unlock(&dev_priv->rps.hw_lock);
> +
> + if (ret) {
> + DRM_DEBUG_KMS("pcode write failed. err = %d decimal = %d\n",
> + ret, decimal);
The debug message could be a bit more descriptive of what we were trying
to do here.
> + return;
> + }
> +
> + dev_priv->cdclk_freq = frequency;
> + } else {
> + mutex_lock(&dev_priv->rps.hw_lock);
> + ret = sandybridge_pcode_write(dev_priv, DISPLAY_PCU_CONTROL, 1);
> + mutex_unlock(&dev_priv->rps.hw_lock);
> +
> + if (ret)
> + DRM_DEBUG_KMS("pcode write failed. err = %d decimal = 1\n",
> + ret);
> + }
Looks like we could avoid the duplicated pcode write easily. Also the
spec says that cdclk will actually be 19.2MHz with DE PLL disabled. So
seems like we should use that instead of 0 as the value all around.
If we want to keep the set_cdclk(0) as a handy shorthand for "disable
the DE PLL" we could then have
if (frequency == 0)
frequency = 19200;
at the start of the function.
> +}
> +
> +void bxt_init_cdclk(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + /* NDE_RSTWRN_OPT RST PCH Handshake En must always be 0b on BXT
> + * or else the reset will hang because there is no PCH to respond.
> + * Move the handshake programming to initialization sequence.
> + * Previously was left up to BIOS.
> + */
> + u32 temp = I915_READ(HSW_NDE_RSTWRN_OPT);
> +
> + temp &= ~RESET_PCH_HANDSHAKE_ENABLE;
> + I915_WRITE(HSW_NDE_RSTWRN_OPT, temp);
> +
> + /* Enable PG1 for cdclk */
> + intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> +
> + /* check if cd clock is enabled */
> + if (I915_READ(BXT_DE_PLL_ENABLE) & BXT_DE_PLL_PLL_ENABLE) {
> + DRM_DEBUG_KMS("Display already initialized\n");
> + return;
> + }
> +
> + /* FIXME:- The initial CDCLK needs to be read from VBT.
> + * Need to make this change after VBT has changes for BXT.
> + */
> + bxt_select_cdclk_freq(dev, 624000);
Do we need to initialize this here? It would get initialized at the
first modeset anyway. Or is there some problem with AUX/something
with the low 19.2MHz cdclk we'd have w/o DE PLL?
> +
> + I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
> + udelay(10);
> +
> + if (!(I915_READ(DBUF_CTL) & DBUF_POWER_STATE))
> + DRM_ERROR("DBuf power enable timeout!\n");
> +}
> +
> +void bxt_uninit_cdclk(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + bxt_ddi_phy_uninit(dev);
> +
> + I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) & ~DBUF_POWER_REQUEST);
> + udelay(10);
> +
> + if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
> + DRM_ERROR("DBuf power disable timeout!\n");
> +
> + bxt_select_cdclk_freq(dev, 0);
> +
> + intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> +}
> +
> void intel_ddi_pll_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1973,6 +2261,9 @@ void intel_ddi_pll_init(struct drm_device *dev)
> if (IS_SKYLAKE(dev)) {
> if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> DRM_ERROR("LCPLL1 is disabled\n");
> + } else if (IS_BROXTON(dev)) {
> + bxt_init_cdclk(dev);
> + bxt_ddi_phy_init(dev);
> } else {
> /*
> * The LCPLL register should be turned on by the BIOS. For now
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b91862e..ba2d1ae 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8284,6 +8284,75 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
> intel_prepare_ddi(dev);
> }
>
> +static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
> + int max_pixclk)
> +{
> + /*
> + * CDclks are supported:
> + * 144MHz
> + * 288MHz
> + * 384MHz
> + * 576MHz
> + * 624MHz
> + * Check to see whether we're above 90% of the lower bin and
> + * adjust if needed.
> + */
> +
> + /* If max_pixclk is greater than the max allowed clock, return 0.
> + * FIXME:- The max clock allowed needs to be provided by GOP/VBIOS
> + * via a scratch pad register. Till that is enabled, use 624MHz as max.
> + */
> + if (max_pixclk > 624000)
> + return 0;
I think we can just ignore the max limit for now. That's what we do for
VLV/CHV. Once we get my cdclk series in we'll have max_cdclk and can
check such limits on all platforms, and in the apprpriate place where we
can still fail the operation / reject the mode.
This will avoid having the !req_cdclk special cases below.
> + else if (max_pixclk > 576000*9/10)
> + return 624000;
> + else if (max_pixclk > 384000*9/10)
> + return 576000;
> + else if (max_pixclk > 288000*9/10)
> + return 384000;
> + else if (max_pixclk > 144000*9/10)
> + return 288000;
Art confirmed that we don't need any guardband here, so the 9/10 factor
should be removed, and the comment adjusted.
> + else
> + return 144000;
Can we drop to DE PLL disabled/19.2MHz when there are no active pipes?
If so we should add such check here. So something like:
...
else if (max_pixclk > 0)
return 144000;
else
return 19200;
> +}
> +
> +static void broxton_modeset_global_pipes(struct drm_device *dev,
> + unsigned *prepare_pipes)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_crtc *intel_crtc;
> + int max_pixclk = intel_mode_max_pixclk(dev_priv);
> + int req_cdclk = broxton_calc_cdclk(dev_priv, max_pixclk);
> +
> + if (!req_cdclk) {
> + DRM_ERROR("CDCLK exceeds maximum allowable value\n");
> + return;
> + }
> +
> + if (req_cdclk == dev_priv->cdclk_freq)
> + return;
> +
> + /* disable/enable all currently active pipes while we change cdclk */
> + for_each_intel_crtc(dev, intel_crtc)
> + if (intel_crtc->base.enabled)
> + *prepare_pipes |= (1 << intel_crtc->pipe);
> +}
Might make sense to combine this with the VLV/CHV function so that
we don't have to add essentially duplicated blocks of code in
__intel_set_mode().
> +
> +static void broxton_modeset_global_resources(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + int max_pixclk = intel_mode_max_pixclk(dev_priv);
> + int req_cdclk = broxton_calc_cdclk(dev_priv, max_pixclk);
> +
> + if (!req_cdclk) {
> + DRM_ERROR("CDCLK exceeds maximum allowable value\n");
> + return;
> + }
> +
> + if (req_cdclk != dev_priv->cdclk_freq)
> + bxt_select_cdclk_freq(dev, req_cdclk);
> +}
> +
> static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
> struct intel_crtc_state *crtc_state)
> {
> @@ -11239,6 +11308,9 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>
> /* may have added more to prepare_pipes than we should */
> prepare_pipes &= ~disable_pipes;
> + } else if (IS_BROXTON(dev)) {
> + broxton_modeset_global_pipes(dev, &prepare_pipes);
> + prepare_pipes &= ~disable_pipes;
> }
>
> ret = __intel_set_mode_setup_plls(dev, modeset_pipes, disable_pipes);
> @@ -13133,6 +13205,9 @@ static void intel_init_display(struct drm_device *dev)
> } else if (IS_VALLEYVIEW(dev)) {
> dev_priv->display.modeset_global_resources =
> valleyview_modeset_global_resources;
> + } else if (IS_BROXTON(dev)) {
> + dev_priv->display.modeset_global_resources =
> + broxton_modeset_global_resources;
> }
>
> switch (INTEL_INFO(dev)->gen) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c77128c..4bc2041 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -873,6 +873,7 @@ void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder);
> void intel_ddi_clock_get(struct intel_encoder *encoder,
> struct intel_crtc_state *pipe_config);
> void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
> +void bxt_select_cdclk_freq(struct drm_device *dev, u32 frequency);
>
> /* intel_frontbuffer.c */
> void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> @@ -1020,6 +1021,9 @@ void intel_prepare_reset(struct drm_device *dev);
> void intel_finish_reset(struct drm_device *dev);
> void hsw_enable_pc8(struct drm_i915_private *dev_priv);
> void hsw_disable_pc8(struct drm_i915_private *dev_priv);
> +void bxt_init_cdclk(struct drm_device *dev);
> +void bxt_uninit_cdclk(struct drm_device *dev);
> +void bxt_ddi_phy_init(struct drm_device *dev);
> void intel_dp_get_m_n(struct intel_crtc *crtc,
> struct intel_crtc_state *pipe_config);
> void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n);
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
