On Thu, 24 Mar 2016, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> Currently the machine hangs during booting while accessing the
> BXT_MIPI_PORT_CTRL register during pipe HW state readout. After some
> experimentation I found that the hang is caused by the DSI PLL being
> disabled, or it being enabled but with an incorrect divider
> configuration. Enabling the PLL got rid of the boot problem, so fix
> this by checking the PLL enabled state/configuration before attempting
> to read out the HW state.
>
> The DSI_PLL_ENABLE register is in the always-on power well, while the
> BXT_DSI_PLL_CTL is in power well 0. This isn't exactly matched by the
> transcoder power domain, but what we really need is just a runtime PM
> reference, which is provided by any power domain.
>
> Ville also found this dependency specified in BSpec, so I added a
> reference to that too.
>
> v2:
> - Make sure we hold a power reference while accessing the PLL registers.
> v3: (Jani)
> - Simplify check in bxt_get_dsi_transcoder_state()
> - Add comment explaining why we check for valid dividers in
> bxt_dsi_pll_is_enabled()
>
> CC: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> CC: Uma Shankar <uma.shankar@xxxxxxxxx>
> CC: Jani Nikula <jani.nikula@xxxxxxxxx>
> Fixes: c6c794a2fc5e ("drm/i915/bxt: Initialize MIPI DSI for BXT")
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
Thanks for fixing this.
Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 2 ++
> drivers/gpu/drm/i915/intel_display.c | 11 ++++++++++
> drivers/gpu/drm/i915/intel_dsi.c | 9 ++++++++
> drivers/gpu/drm/i915/intel_dsi.h | 1 +
> drivers/gpu/drm/i915/intel_dsi_pll.c | 40 ++++++++++++++++++++++++++++++++++++
> 5 files changed, 63 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f3ba43c..c839ce9 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7811,9 +7811,11 @@ enum skl_disp_power_wells {
> #define BXT_DSIC_16X_BY2 (1 << 10)
> #define BXT_DSIC_16X_BY3 (2 << 10)
> #define BXT_DSIC_16X_BY4 (3 << 10)
> +#define BXT_DSIC_16X_MASK (3 << 10)
> #define BXT_DSIA_16X_BY2 (1 << 8)
> #define BXT_DSIA_16X_BY3 (2 << 8)
> #define BXT_DSIA_16X_BY4 (3 << 8)
> +#define BXT_DSIA_16X_MASK (3 << 8)
> #define BXT_DSI_FREQ_SEL_SHIFT 8
> #define BXT_DSI_FREQ_SEL_MASK (0xF << BXT_DSI_FREQ_SEL_SHIFT)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 009b03b..e05393e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -36,6 +36,7 @@
> #include "intel_drv.h"
> #include <drm/i915_drm.h>
> #include "i915_drv.h"
> +#include "intel_dsi.h"
> #include "i915_trace.h"
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> @@ -9867,6 +9868,16 @@ static bool bxt_get_dsi_transcoder_state(struct intel_crtc *crtc,
> continue;
> *power_domain_mask |= BIT(power_domain);
>
> + /*
> + * The PLL needs to be enabled with a valid divider
> + * configuration, otherwise accessing DSI registers will hang
> + * the machine. See BSpec North Display Engine
> + * registers/MIPI[BXT]. We can break out here early, since we
> + * need the same DSI PLL to be enabled for both DSI ports.
> + */
> + if (!intel_dsi_pll_is_enabled(dev_priv))
> + break;
> +
> /* XXX: this works for video mode only */
> tmp = I915_READ(BXT_MIPI_PORT_CTRL(port));
> if (!(tmp & DPI_ENABLE))
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 96ea3f7..0de74e1 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -684,6 +684,14 @@ static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
> if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
> return false;
>
> + /*
> + * On Broxton the PLL needs to be enabled with a valid divider
> + * configuration, otherwise accessing DSI registers will hang the
> + * machine. See BSpec North Display Engine registers/MIPI[BXT].
> + */
> + if (IS_BROXTON(dev_priv) && !intel_dsi_pll_is_enabled(dev_priv))
> + goto out_put_power;
> +
> /* XXX: this only works for one DSI output */
> for_each_dsi_port(port, intel_dsi->ports) {
> i915_reg_t ctrl_reg = IS_BROXTON(dev) ?
> @@ -726,6 +734,7 @@ static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
> break;
> }
>
> +out_put_power:
> intel_display_power_put(dev_priv, power_domain);
>
> return active;
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index e582ef8..ec58ead 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -126,6 +126,7 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
> return container_of(encoder, struct intel_dsi, base.base);
> }
>
> +bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv);
> extern void intel_enable_dsi_pll(struct intel_encoder *encoder);
> extern void intel_disable_dsi_pll(struct intel_encoder *encoder);
> extern u32 intel_dsi_get_pclk(struct intel_encoder *encoder, int pipe_bpp);
> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
> index e3e343c..ce688f9 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
> @@ -192,6 +192,36 @@ static void vlv_disable_dsi_pll(struct intel_encoder *encoder)
> mutex_unlock(&dev_priv->sb_lock);
> }
>
> +static bool bxt_dsi_pll_is_enabled(struct drm_i915_private *dev_priv)
> +{
> + bool enabled;
> + u32 val;
> + u32 mask;
> +
> + mask = BXT_DSI_PLL_DO_ENABLE | BXT_DSI_PLL_LOCKED;
> + val = I915_READ(BXT_DSI_PLL_ENABLE);
> + enabled = (val & mask) == mask;
> +
> + if (!enabled)
> + return false;
> +
> + /*
> + * Both dividers must be programmed with valid values even if only one
> + * of the PLL is used, see BSpec/Broxton Clocks. Check this here for
> + * paranoia, since BIOS is known to misconfigure PLLs in this way at
> + * times, and since accessing DSI registers with invalid dividers
> + * causes a system hang.
> + */
> + val = I915_READ(BXT_DSI_PLL_CTL);
> + if (!(val & BXT_DSIA_16X_MASK) || !(val & BXT_DSIC_16X_MASK)) {
> + DRM_DEBUG_DRIVER("PLL is enabled with invalid divider settings "
> + "(%08x)\n", val);
> + enabled = false;
> + }
> +
> + return enabled;
> +}
> +
> static void bxt_disable_dsi_pll(struct intel_encoder *encoder)
> {
> struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> @@ -486,6 +516,16 @@ static void bxt_enable_dsi_pll(struct intel_encoder *encoder)
> DRM_DEBUG_KMS("DSI PLL locked\n");
> }
>
> +bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv)
> +{
> + if (IS_BROXTON(dev_priv))
> + return bxt_dsi_pll_is_enabled(dev_priv);
> +
> + MISSING_CASE(INTEL_DEVID(dev_priv));
> +
> + return false;
> +}
> +
> void intel_enable_dsi_pll(struct intel_encoder *encoder)
> {
> struct drm_device *dev = encoder->base.dev;
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
