Re: [Intel-gfx] [PATCH] drm/i915: Add MIPI_IO WA and program DSI regulators

2017-02-01 Thread Jani Nikula 
On Tue, 31 Jan 2017, Mika Kahola  wrote:
> Looks ok.
>
> Acked-by: Mika Kahola 

Pushed to drm-intel-next-queued, thanks for the patch.

BR,
Jani.

>
> On Wed, 2017-01-25 at 19:43 +0530, Vidya Srinivas wrote:
>> From: Uma Shankar 
>> 
>> Enable MIPI IO WA for BXT DSI as per bspec and
>> program the DSI regulators.
>> 
>> v2: Moved IO enable to pre-enable as per Mika's
>> review comments. Also reused the existing register
>> definition for BXT_P_CR_GT_DISP_PWRON.
>> 
>> v3: Added Programming the DSI regulators as per disable/enable
>> sequences.
>> 
>> v4: Restricting regulator changes to BXT as suggested by
>> Jani/Mika
>> 
>> v5: Removed redundant read/modify for regulator register as
>> per Jani's comment. Maintain enable/disable symmetry as per spec.
>> 
>> Signed-off-by: Uma Shankar 
>> Signed-off-by: Vidya Srinivas 
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h  |  7 +++
>>  drivers/gpu/drm/i915/intel_dsi.c | 24 
>>  2 files changed, 31 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 00970aa..0a9ad44 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1553,6 +1553,7 @@ enum skl_disp_power_wells {
>>  _MMIO(_BXT_PHY_CH(phy, ch, reg_ch0, reg_ch1))
>>  
>>  #define BXT_P_CR_GT_DISP_PWRON  _MMIO(0x138090)
>> +#define  MIPIO_RST_CTRL (1 << 2)
>>  
>>  #define _BXT_PHY_CTL_DDI_A  0x64C00
>>  #define _BXT_PHY_CTL_DDI_B  0x64C10
>> @@ -8301,6 +8302,12 @@ enum {
>>  #define _BXT_MIPIC_PORT_CTRL0x6B8C0
>>  #define BXT_MIPI_PORT_CTRL(tc)  _MMIO_MIPI(tc,
>> _BXT_MIPIA_PORT_CTRL, _BXT_MIPIC_PORT_CTRL)
>>  
>> +#define BXT_P_DSI_REGULATOR_CFG _MMIO(0x16002
>> 0)
>> +#define  STAP_SELECT(1 << 0)
>> +
>> +#define BXT_P_DSI_REGULATOR_TX_CTRL _MMIO(0x160054)
>> +#define  HS_IO_CTRL_SELECT  (1 << 0)
>> +
>>  #define  DPI_ENABLE (1 << 31)
>> /* A + C */
>>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT  27
>>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_MASK   (0xf << 27)
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>> b/drivers/gpu/drm/i915/intel_dsi.c
>> index 16732e7..c98234e 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -548,6 +548,7 @@ static void intel_dsi_pre_enable(struct
>> intel_encoder *encoder,
>>  struct drm_i915_private *dev_priv = to_i915(encoder-
>> >base.dev);
>>  struct intel_dsi *intel_dsi = enc_to_intel_dsi(
>> >base);
>>  enum port port;
>> +u32 val;
>>  
>>  DRM_DEBUG_KMS("\n");
>>  
>> @@ -558,6 +559,17 @@ static void intel_dsi_pre_enable(struct
>> intel_encoder *encoder,
>>  intel_disable_dsi_pll(encoder);
>>  intel_enable_dsi_pll(encoder, pipe_config);
>>  
>> +if (IS_BROXTON(dev_priv)) {
>> +/* Add MIPI IO reset programming for modeset */
>> +val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
>> +I915_WRITE(BXT_P_CR_GT_DISP_PWRON,
>> +val | MIPIO_RST_CTRL);
>> +
>> +/* Power up DSI regulator */
>> +I915_WRITE(BXT_P_DSI_REGULATOR_CFG, STAP_SELECT);
>> +I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL, 0);
>> +}
>> +
>>  intel_dsi_prepare(encoder, pipe_config);
>>  
>>  /* Panel Enable over CRC PMIC */
>> @@ -707,6 +719,7 @@ static void intel_dsi_post_disable(struct
>> intel_encoder *encoder,
>>  {
>>  struct drm_i915_private *dev_priv = to_i915(encoder-
>> >base.dev);
>>  struct intel_dsi *intel_dsi = enc_to_intel_dsi(
>> >base);
>> +u32 val;
>>  
>>  DRM_DEBUG_KMS("\n");
>>  
>> @@ -714,6 +727,17 @@ static void intel_dsi_post_disable(struct
>> intel_encoder *encoder,
>>  
>>  intel_dsi_clear_device_ready(encoder);
>>  
>> +if (IS_BROXTON(dev_priv)) {
>> +/* Power down DSI regulator to save power */
>> +I915_WRITE(BXT_P_DSI_REGULATOR_CFG, STAP_SELECT);
>> +I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL,
>> HS_IO_CTRL_SELECT);
>> +
>> +/* Add MIPI IO reset programming for modeset */
>> +val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
>> +I915_WRITE(BXT_P_CR_GT_DISP_PWRON,
>> +val & ~MIPIO_RST_CTRL);
>> +}
>> +
>>  intel_disable_dsi_pll(encoder);
>>  
>>  if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Add MIPI_IO WA and program DSI regulators

2017-01-31 Thread Mika Kahola 
Looks ok.

Acked-by: Mika Kahola 

On Wed, 2017-01-25 at 19:43 +0530, Vidya Srinivas wrote:
> From: Uma Shankar 
> 
> Enable MIPI IO WA for BXT DSI as per bspec and
> program the DSI regulators.
> 
> v2: Moved IO enable to pre-enable as per Mika's
> review comments. Also reused the existing register
> definition for BXT_P_CR_GT_DISP_PWRON.
> 
> v3: Added Programming the DSI regulators as per disable/enable
> sequences.
> 
> v4: Restricting regulator changes to BXT as suggested by
> Jani/Mika
> 
> v5: Removed redundant read/modify for regulator register as
> per Jani's comment. Maintain enable/disable symmetry as per spec.
> 
> Signed-off-by: Uma Shankar 
> Signed-off-by: Vidya Srinivas 
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  7 +++
>  drivers/gpu/drm/i915/intel_dsi.c | 24 
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 00970aa..0a9ad44 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1553,6 +1553,7 @@ enum skl_disp_power_wells {
>   _MMIO(_BXT_PHY_CH(phy, ch, reg_ch0, reg_ch1))
>  
>  #define BXT_P_CR_GT_DISP_PWRON   _MMIO(0x138090)
> +#define  MIPIO_RST_CTRL  (1 << 2)
>  
>  #define _BXT_PHY_CTL_DDI_A   0x64C00
>  #define _BXT_PHY_CTL_DDI_B   0x64C10
> @@ -8301,6 +8302,12 @@ enum {
>  #define _BXT_MIPIC_PORT_CTRL 0x6B8C0
>  #define BXT_MIPI_PORT_CTRL(tc)   _MMIO_MIPI(tc,
> _BXT_MIPIA_PORT_CTRL, _BXT_MIPIC_PORT_CTRL)
>  
> +#define BXT_P_DSI_REGULATOR_CFG  _MMIO(0x16002
> 0)
> +#define  STAP_SELECT (1 << 0)
> +
> +#define BXT_P_DSI_REGULATOR_TX_CTRL  _MMIO(0x160054)
> +#define  HS_IO_CTRL_SELECT   (1 << 0)
> +
>  #define  DPI_ENABLE  (1 << 31)
> /* A + C */
>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT   27
>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_MASK(0xf << 27)
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> b/drivers/gpu/drm/i915/intel_dsi.c
> index 16732e7..c98234e 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -548,6 +548,7 @@ static void intel_dsi_pre_enable(struct
> intel_encoder *encoder,
>   struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
>   struct intel_dsi *intel_dsi = enc_to_intel_dsi(
> >base);
>   enum port port;
> + u32 val;
>  
>   DRM_DEBUG_KMS("\n");
>  
> @@ -558,6 +559,17 @@ static void intel_dsi_pre_enable(struct
> intel_encoder *encoder,
>   intel_disable_dsi_pll(encoder);
>   intel_enable_dsi_pll(encoder, pipe_config);
>  
> + if (IS_BROXTON(dev_priv)) {
> + /* Add MIPI IO reset programming for modeset */
> + val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
> + I915_WRITE(BXT_P_CR_GT_DISP_PWRON,
> + val | MIPIO_RST_CTRL);
> +
> + /* Power up DSI regulator */
> + I915_WRITE(BXT_P_DSI_REGULATOR_CFG, STAP_SELECT);
> + I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL, 0);
> + }
> +
>   intel_dsi_prepare(encoder, pipe_config);
>  
>   /* Panel Enable over CRC PMIC */
> @@ -707,6 +719,7 @@ static void intel_dsi_post_disable(struct
> intel_encoder *encoder,
>  {
>   struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
>   struct intel_dsi *intel_dsi = enc_to_intel_dsi(
> >base);
> + u32 val;
>  
>   DRM_DEBUG_KMS("\n");
>  
> @@ -714,6 +727,17 @@ static void intel_dsi_post_disable(struct
> intel_encoder *encoder,
>  
>   intel_dsi_clear_device_ready(encoder);
>  
> + if (IS_BROXTON(dev_priv)) {
> + /* Power down DSI regulator to save power */
> + I915_WRITE(BXT_P_DSI_REGULATOR_CFG, STAP_SELECT);
> + I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL,
> HS_IO_CTRL_SELECT);
> +
> + /* Add MIPI IO reset programming for modeset */
> + val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
> + I915_WRITE(BXT_P_CR_GT_DISP_PWRON,
> + val & ~MIPIO_RST_CTRL);
> + }
> +
>   intel_disable_dsi_pll(encoder);
>  
>   if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
-- 
Mika Kahola - Intel OTC

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Add MIPI_IO WA and program DSI regulators

2017-01-31 Thread Srinivas, Vidya 
Gentle remainder - could you kindly check the patch please? Thank you.

> -Original Message-
> From: Srinivas, Vidya
> Sent: Wednesday, January 25, 2017 7:43 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Shankar, Uma ; Nikula, Jani
> ; Syrjala, Ville ; Kahola,
> Mika ; Srinivas, Vidya 
> Subject: [PATCH] drm/i915: Add MIPI_IO WA and program DSI regulators
> 
> From: Uma Shankar 
> 
> Enable MIPI IO WA for BXT DSI as per bspec and program the DSI regulators.
> 
> v2: Moved IO enable to pre-enable as per Mika's review comments. Also
> reused the existing register definition for BXT_P_CR_GT_DISP_PWRON.
> 
> v3: Added Programming the DSI regulators as per disable/enable sequences.
> 
> v4: Restricting regulator changes to BXT as suggested by Jani/Mika
> 
> v5: Removed redundant read/modify for regulator register as per Jani's
> comment. Maintain enable/disable symmetry as per spec.
> 
> Signed-off-by: Uma Shankar 
> Signed-off-by: Vidya Srinivas 
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  7 +++
> drivers/gpu/drm/i915/intel_dsi.c | 24 
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h index 00970aa..0a9ad44 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1553,6 +1553,7 @@ enum skl_disp_power_wells {
>   _MMIO(_BXT_PHY_CH(phy, ch, reg_ch0, reg_ch1))
> 
>  #define BXT_P_CR_GT_DISP_PWRON   _MMIO(0x138090)
> +#define  MIPIO_RST_CTRL  (1 << 2)
> 
>  #define _BXT_PHY_CTL_DDI_A   0x64C00
>  #define _BXT_PHY_CTL_DDI_B   0x64C10
> @@ -8301,6 +8302,12 @@ enum {
>  #define _BXT_MIPIC_PORT_CTRL 0x6B8C0
>  #define BXT_MIPI_PORT_CTRL(tc)   _MMIO_MIPI(tc,
> _BXT_MIPIA_PORT_CTRL, _BXT_MIPIC_PORT_CTRL)
> 
> +#define BXT_P_DSI_REGULATOR_CFG  _MMIO(0x160020)
> +#define  STAP_SELECT (1 << 0)
> +
> +#define BXT_P_DSI_REGULATOR_TX_CTRL  _MMIO(0x160054)
> +#define  HS_IO_CTRL_SELECT   (1 << 0)
> +
>  #define  DPI_ENABLE  (1 << 31) /* A + C */
>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT   27
>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_MASK(0xf << 27)
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> b/drivers/gpu/drm/i915/intel_dsi.c
> index 16732e7..c98234e 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -548,6 +548,7 @@ static void intel_dsi_pre_enable(struct
> intel_encoder *encoder,
>   struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>   struct intel_dsi *intel_dsi = enc_to_intel_dsi(>base);
>   enum port port;
> + u32 val;
> 
>   DRM_DEBUG_KMS("\n");
> 
> @@ -558,6 +559,17 @@ static void intel_dsi_pre_enable(struct
> intel_encoder *encoder,
>   intel_disable_dsi_pll(encoder);
>   intel_enable_dsi_pll(encoder, pipe_config);
> 
> + if (IS_BROXTON(dev_priv)) {
> + /* Add MIPI IO reset programming for modeset */
> + val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
> + I915_WRITE(BXT_P_CR_GT_DISP_PWRON,
> + val | MIPIO_RST_CTRL);
> +
> + /* Power up DSI regulator */
> + I915_WRITE(BXT_P_DSI_REGULATOR_CFG, STAP_SELECT);
> + I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL, 0);
> + }
> +
>   intel_dsi_prepare(encoder, pipe_config);
> 
>   /* Panel Enable over CRC PMIC */
> @@ -707,6 +719,7 @@ static void intel_dsi_post_disable(struct
> intel_encoder *encoder,  {
>   struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>   struct intel_dsi *intel_dsi = enc_to_intel_dsi(>base);
> + u32 val;
> 
>   DRM_DEBUG_KMS("\n");
> 
> @@ -714,6 +727,17 @@ static void intel_dsi_post_disable(struct
> intel_encoder *encoder,
> 
>   intel_dsi_clear_device_ready(encoder);
> 
> + if (IS_BROXTON(dev_priv)) {
> + /* Power down DSI regulator to save power */
> + I915_WRITE(BXT_P_DSI_REGULATOR_CFG, STAP_SELECT);
> + I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL,
> HS_IO_CTRL_SELECT);
> +
> + /* Add MIPI IO reset programming for modeset */
> + val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
> + I915_WRITE(BXT_P_CR_GT_DISP_PWRON,
> + val & ~MIPIO_RST_CTRL);
> + }
> +
>   intel_disable_dsi_pll(encoder);
> 
>   if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> --
> 1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Add MIPI_IO WA and program DSI regulators

2017-01-25 Thread Shankar, Uma 


>-Original Message-
>From: Nikula, Jani
>Sent: Thursday, January 19, 2017 4:34 PM
>To: Srinivas, Vidya ; intel-
>g...@lists.freedesktop.org
>Cc: Shankar, Uma ; Syrjala, Ville
>
>Subject: Re: [PATCH] drm/i915: Add MIPI_IO WA and program DSI regulators
>
>On Thu, 19 Jan 2017, Vidya Srinivas  wrote:
>> From: Uma Shankar 
>>
>> Enable MIPI IO WA for BXT DSI as per bspec and program the DSI
>> regulators.
>>
>> v2: Moved IO enable to pre-enable as per Mika's review comments. Also
>> reused the existing register definition for BXT_P_CR_GT_DISP_PWRON.
>>
>> v3: Added Programming the DSI regulators as per disable/enable
>> sequences.
>>
>> v4: Restricting regulator changes to BXT as suggested by Jani/Mika
>
>This applies to BXT_P_CR_GT_DISP_PWRON changes as well.
Yes, this should be under IS_BXT.

>
>One other question inline.
>
>>
>> Signed-off-by: Uma Shankar 
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h  |  7 +++
>> drivers/gpu/drm/i915/intel_dsi.c | 25 +
>>  2 files changed, 32 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index 00970aa..0a9ad44 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1553,6 +1553,7 @@ enum skl_disp_power_wells {
>>  _MMIO(_BXT_PHY_CH(phy, ch, reg_ch0, reg_ch1))
>>
>>  #define BXT_P_CR_GT_DISP_PWRON  _MMIO(0x138090)
>> +#define  MIPIO_RST_CTRL (1 << 2)
>>
>>  #define _BXT_PHY_CTL_DDI_A  0x64C00
>>  #define _BXT_PHY_CTL_DDI_B  0x64C10
>> @@ -8301,6 +8302,12 @@ enum {
>>  #define _BXT_MIPIC_PORT_CTRL0x6B8C0
>>  #define BXT_MIPI_PORT_CTRL(tc)  _MMIO_MIPI(tc,
>_BXT_MIPIA_PORT_CTRL, _BXT_MIPIC_PORT_CTRL)
>>
>> +#define BXT_P_DSI_REGULATOR_CFG
>   _MMIO(0x160020)
>> +#define  STAP_SELECT(1 << 0)
>> +
>> +#define BXT_P_DSI_REGULATOR_TX_CTRL _MMIO(0x160054)
>> +#define  HS_IO_CTRL_SELECT  (1 << 0)
>> +
>>  #define  DPI_ENABLE (1 << 31) /* A
>+ C */
>>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT  27
>>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_MASK   (0xf << 27)
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>> b/drivers/gpu/drm/i915/intel_dsi.c
>> index 16732e7..4dc1293 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -548,6 +548,7 @@ static void intel_dsi_pre_enable(struct intel_encoder
>*encoder,
>>  struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>  struct intel_dsi *intel_dsi = enc_to_intel_dsi(>base);
>>  enum port port;
>> +u32 val;
>>
>>  DRM_DEBUG_KMS("\n");
>>
>> @@ -558,6 +559,11 @@ static void intel_dsi_pre_enable(struct
>intel_encoder *encoder,
>>  intel_disable_dsi_pll(encoder);
>>  intel_enable_dsi_pll(encoder, pipe_config);
>>
>> +/* Add MIPI IO reset programming for modeset */
>> +val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
>> +I915_WRITE(BXT_P_CR_GT_DISP_PWRON,
>> +val | MIPIO_RST_CTRL);
>> +
>>  intel_dsi_prepare(encoder, pipe_config);
>>
>>  /* Panel Enable over CRC PMIC */
>> @@ -575,6 +581,14 @@ static void intel_dsi_pre_enable(struct
>intel_encoder *encoder,
>>  I915_WRITE(DSPCLK_GATE_D, val);
>>  }
>>
>> +/* Power up DSI regulator */
>> +if (IS_BROXTON(dev_priv)) {
>> +I915_WRITE(BXT_P_DSI_REGULATOR_CFG, STAP_SELECT);
>> +val = I915_READ(BXT_P_DSI_REGULATOR_TX_CTRL);
>> +val &= ~HS_IO_CTRL_SELECT;
>> +I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL, val);
>
>Why does this specific change warrant a read-modify-write when the other
>regulator changes in this patch do a full register write?
Checked this in bspec and looks like we can avoid a read/modify operation.

>
>Also, the enable and disable sequences seem a bit asymmetric with these
>changes, i.e. you enable and disable things in different steps of the
>sequences. That's a bit surprising.
Yes, will update this to maintain the symmetry and re-send.
Thanks Jani for all your valuable inputs.

>
>(These might have an answer in bspec, but I don't seem to be able to access
>that right now.)
>
>BR,
>Jani.
>
>> +}
>> +
>>  /* put device in ready state */
>>  intel_dsi_device_ready(encoder);
>>
>> @@ -707,6 +721,7 @@ static void intel_dsi_post_disable(struct
>> intel_encoder *encoder,  {
>>  struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>  struct intel_dsi *intel_dsi = enc_to_intel_dsi(>base);
>> +u32 val;
>>
>>  DRM_DEBUG_KMS("\n");
>>
>> @@ -714,8 +729,18 @@ static void intel_dsi_post_disable(struct
>> intel_encoder *encoder,
>>
>>  intel_dsi_clear_device_ready(encoder);
>>
>> +

Re: [Intel-gfx] [PATCH] drm/i915: Add MIPI_IO WA and program DSI regulators

2017-01-19 Thread Jani Nikula 
On Thu, 19 Jan 2017, Vidya Srinivas  wrote:
> From: Uma Shankar 
>
> Enable MIPI IO WA for BXT DSI as per bspec and
> program the DSI regulators.
>
> v2: Moved IO enable to pre-enable as per Mika's
> review comments. Also reused the existing register
> definition for BXT_P_CR_GT_DISP_PWRON.
>
> v3: Added Programming the DSI regulators as per disable/enable
> sequences.
>
> v4: Restricting regulator changes to BXT as suggested by
> Jani/Mika

This applies to BXT_P_CR_GT_DISP_PWRON changes as well.

One other question inline.

>
> Signed-off-by: Uma Shankar 
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  7 +++
>  drivers/gpu/drm/i915/intel_dsi.c | 25 +
>  2 files changed, 32 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 00970aa..0a9ad44 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1553,6 +1553,7 @@ enum skl_disp_power_wells {
>   _MMIO(_BXT_PHY_CH(phy, ch, reg_ch0, reg_ch1))
>  
>  #define BXT_P_CR_GT_DISP_PWRON   _MMIO(0x138090)
> +#define  MIPIO_RST_CTRL  (1 << 2)
>  
>  #define _BXT_PHY_CTL_DDI_A   0x64C00
>  #define _BXT_PHY_CTL_DDI_B   0x64C10
> @@ -8301,6 +8302,12 @@ enum {
>  #define _BXT_MIPIC_PORT_CTRL 0x6B8C0
>  #define BXT_MIPI_PORT_CTRL(tc)   _MMIO_MIPI(tc, _BXT_MIPIA_PORT_CTRL, 
> _BXT_MIPIC_PORT_CTRL)
>  
> +#define BXT_P_DSI_REGULATOR_CFG  _MMIO(0x160020)
> +#define  STAP_SELECT (1 << 0)
> +
> +#define BXT_P_DSI_REGULATOR_TX_CTRL  _MMIO(0x160054)
> +#define  HS_IO_CTRL_SELECT   (1 << 0)
> +
>  #define  DPI_ENABLE  (1 << 31) /* A + C */
>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT   27
>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_MASK(0xf << 27)
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c 
> b/drivers/gpu/drm/i915/intel_dsi.c
> index 16732e7..4dc1293 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -548,6 +548,7 @@ static void intel_dsi_pre_enable(struct intel_encoder 
> *encoder,
>   struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>   struct intel_dsi *intel_dsi = enc_to_intel_dsi(>base);
>   enum port port;
> + u32 val;
>  
>   DRM_DEBUG_KMS("\n");
>  
> @@ -558,6 +559,11 @@ static void intel_dsi_pre_enable(struct intel_encoder 
> *encoder,
>   intel_disable_dsi_pll(encoder);
>   intel_enable_dsi_pll(encoder, pipe_config);
>  
> + /* Add MIPI IO reset programming for modeset */
> + val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
> + I915_WRITE(BXT_P_CR_GT_DISP_PWRON,
> + val | MIPIO_RST_CTRL);
> +
>   intel_dsi_prepare(encoder, pipe_config);
>  
>   /* Panel Enable over CRC PMIC */
> @@ -575,6 +581,14 @@ static void intel_dsi_pre_enable(struct intel_encoder 
> *encoder,
>   I915_WRITE(DSPCLK_GATE_D, val);
>   }
>  
> + /* Power up DSI regulator */
> + if (IS_BROXTON(dev_priv)) {
> + I915_WRITE(BXT_P_DSI_REGULATOR_CFG, STAP_SELECT);
> + val = I915_READ(BXT_P_DSI_REGULATOR_TX_CTRL);
> + val &= ~HS_IO_CTRL_SELECT;
> + I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL, val);

Why does this specific change warrant a read-modify-write when the other
regulator changes in this patch do a full register write?

Also, the enable and disable sequences seem a bit asymmetric with these
changes, i.e. you enable and disable things in different steps of the
sequences. That's a bit surprising.

(These might have an answer in bspec, but I don't seem to be able to
access that right now.)


BR,
Jani.

> + }
> +
>   /* put device in ready state */
>   intel_dsi_device_ready(encoder);
>  
> @@ -707,6 +721,7 @@ static void intel_dsi_post_disable(struct intel_encoder 
> *encoder,
>  {
>   struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>   struct intel_dsi *intel_dsi = enc_to_intel_dsi(>base);
> + u32 val;
>  
>   DRM_DEBUG_KMS("\n");
>  
> @@ -714,8 +729,18 @@ static void intel_dsi_post_disable(struct intel_encoder 
> *encoder,
>  
>   intel_dsi_clear_device_ready(encoder);
>  
> + val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
> + I915_WRITE(BXT_P_CR_GT_DISP_PWRON,
> + val & ~MIPIO_RST_CTRL);
> +
>   intel_disable_dsi_pll(encoder);
>  
> + if (IS_BROXTON(dev_priv)) {
> + /* Power down DSI regulator to save power */
> + I915_WRITE(BXT_P_DSI_REGULATOR_CFG, STAP_SELECT);
> + I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL, HS_IO_CTRL_SELECT);
> + }
> +
>   if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>   u32 val;

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH] drm/i915: Add MIPI_IO WA and program DSI regulators

2017-01-19 Thread Jani Nikula 
On Thu, 19 Jan 2017, Mika Kahola  wrote:
> On Thu, 2017-01-19 at 11:41 +0530, Vidya Srinivas wrote:
>> From: Uma Shankar 
>> 
>> Enable MIPI IO WA for BXT DSI as per bspec and
>> program the DSI regulators.
>> 
>> v2: Moved IO enable to pre-enable as per Mika's
>> review comments. Also reused the existing register
>> definition for BXT_P_CR_GT_DISP_PWRON.
>> 
>> v3: Added Programming the DSI regulators as per disable/enable
>> sequences.
>> 
>> Signed-off-by: Uma Shankar 
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h  |  7 +++
>>  drivers/gpu/drm/i915/intel_dsi.c | 21 +
>>  2 files changed, 28 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 00970aa..0a9ad44 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1553,6 +1553,7 @@ enum skl_disp_power_wells {
>>  _MMIO(_BXT_PHY_CH(phy, ch, reg_ch0, reg_ch1))
>>  
>>  #define BXT_P_CR_GT_DISP_PWRON  _MMIO(0x138090)
>> +#define  MIPIO_RST_CTRL (1 << 2)
>>  
>>  #define _BXT_PHY_CTL_DDI_A  0x64C00
>>  #define _BXT_PHY_CTL_DDI_B  0x64C10
>> @@ -8301,6 +8302,12 @@ enum {
>>  #define _BXT_MIPIC_PORT_CTRL0x6B8C0
>>  #define BXT_MIPI_PORT_CTRL(tc)  _MMIO_MIPI(tc,
>> _BXT_MIPIA_PORT_CTRL, _BXT_MIPIC_PORT_CTRL)
>>  
>> +#define BXT_P_DSI_REGULATOR_CFG _MMIO(0x16002
>> 0)
>> +#define  STAP_SELECT(1 << 0)
>> +
>> +#define BXT_P_DSI_REGULATOR_TX_CTRL _MMIO(0x160054)
>> +#define  HS_IO_CTRL_SELECT  (1 << 0)
>> +
>>  #define  DPI_ENABLE (1 << 31)
>> /* A + C */
>>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT  27
>>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_MASK   (0xf << 27)
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>> b/drivers/gpu/drm/i915/intel_dsi.c
>> index 16732e7..043441e 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -548,6 +548,7 @@ static void intel_dsi_pre_enable(struct
>> intel_encoder *encoder,
>>  struct drm_i915_private *dev_priv = to_i915(encoder-
>> >base.dev);
>>  struct intel_dsi *intel_dsi = enc_to_intel_dsi(
>> >base);
>>  enum port port;
>> +u32 val;
>>  
>>  DRM_DEBUG_KMS("\n");
>>  
>> @@ -558,6 +559,11 @@ static void intel_dsi_pre_enable(struct
>> intel_encoder *encoder,
>>  intel_disable_dsi_pll(encoder);
>>  intel_enable_dsi_pll(encoder, pipe_config);
>>  
>> +/* Add MIPI IO reset programming for modeset */
>> +val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
>> +I915_WRITE(BXT_P_CR_GT_DISP_PWRON,
>> +val | MIPIO_RST_CTRL);
>> +
> Looking good but overall we still need to address Jani's comment to
> limit these changes only to Broxton platform.

Please don't send this in reply to the current thread...

BR,
Jani.


>
>>  intel_dsi_prepare(encoder, pipe_config);
>>  
>>  /* Panel Enable over CRC PMIC */
>> @@ -575,6 +581,12 @@ static void intel_dsi_pre_enable(struct
>> intel_encoder *encoder,
>>  I915_WRITE(DSPCLK_GATE_D, val);
>>  }
>>  
>> +/* Power up DSI regulator */
>> +I915_WRITE(BXT_P_DSI_REGULATOR_CFG, STAP_SELECT);
>> +val = I915_READ(BXT_P_DSI_REGULATOR_TX_CTRL);
>> +val &= ~HS_IO_CTRL_SELECT;
>> +I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL, val);
>> +    /* put device in ready state */
>>  intel_dsi_device_ready(encoder);
>>  
>> @@ -707,6 +719,7 @@ static void intel_dsi_post_disable(struct
>> intel_encoder *encoder,
>>  {
>>  struct drm_i915_private *dev_priv = to_i915(encoder-
>> >base.dev);
>>  struct intel_dsi *intel_dsi = enc_to_intel_dsi(
>> >base);
>> +u32 val;
>>  
>>  DRM_DEBUG_KMS("\n");
>>  
>> @@ -714,8 +727,16 @@ static void intel_dsi_post_disable(struct
>> intel_encoder *encoder,
>>  
>>  intel_dsi_clear_device_ready(encoder);
>>  
>> +val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
>> +I915_WRITE(BXT_P_CR_GT_DISP_PWRON,
>> +val & ~MIPIO_RST_CTRL);
>> +
>>  intel_disable_dsi_pll(encoder);
>>  
>> +/* Power down DSI regulator to save power */
>> +I915_WRITE(BXT_P_DSI_REGULATOR_CFG, STAP_SELECT);
>> +I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL, HS_IO_CTRL_SELECT);
>> +
>>  if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>>  u32 val;
>>  

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Add MIPI_IO WA and program DSI regulators

2017-01-19 Thread Mika Kahola 
On Thu, 2017-01-19 at 11:41 +0530, Vidya Srinivas wrote:
> From: Uma Shankar 
> 
> Enable MIPI IO WA for BXT DSI as per bspec and
> program the DSI regulators.
> 
> v2: Moved IO enable to pre-enable as per Mika's
> review comments. Also reused the existing register
> definition for BXT_P_CR_GT_DISP_PWRON.
> 
> v3: Added Programming the DSI regulators as per disable/enable
> sequences.
> 
> Signed-off-by: Uma Shankar 
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  7 +++
>  drivers/gpu/drm/i915/intel_dsi.c | 21 +
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 00970aa..0a9ad44 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1553,6 +1553,7 @@ enum skl_disp_power_wells {
>   _MMIO(_BXT_PHY_CH(phy, ch, reg_ch0, reg_ch1))
>  
>  #define BXT_P_CR_GT_DISP_PWRON   _MMIO(0x138090)
> +#define  MIPIO_RST_CTRL  (1 << 2)
>  
>  #define _BXT_PHY_CTL_DDI_A   0x64C00
>  #define _BXT_PHY_CTL_DDI_B   0x64C10
> @@ -8301,6 +8302,12 @@ enum {
>  #define _BXT_MIPIC_PORT_CTRL 0x6B8C0
>  #define BXT_MIPI_PORT_CTRL(tc)   _MMIO_MIPI(tc,
> _BXT_MIPIA_PORT_CTRL, _BXT_MIPIC_PORT_CTRL)
>  
> +#define BXT_P_DSI_REGULATOR_CFG  _MMIO(0x16002
> 0)
> +#define  STAP_SELECT (1 << 0)
> +
> +#define BXT_P_DSI_REGULATOR_TX_CTRL  _MMIO(0x160054)
> +#define  HS_IO_CTRL_SELECT   (1 << 0)
> +
>  #define  DPI_ENABLE  (1 << 31)
> /* A + C */
>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT   27
>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_MASK(0xf << 27)
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> b/drivers/gpu/drm/i915/intel_dsi.c
> index 16732e7..043441e 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -548,6 +548,7 @@ static void intel_dsi_pre_enable(struct
> intel_encoder *encoder,
>   struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
>   struct intel_dsi *intel_dsi = enc_to_intel_dsi(
> >base);
>   enum port port;
> + u32 val;
>  
>   DRM_DEBUG_KMS("\n");
>  
> @@ -558,6 +559,11 @@ static void intel_dsi_pre_enable(struct
> intel_encoder *encoder,
>   intel_disable_dsi_pll(encoder);
>   intel_enable_dsi_pll(encoder, pipe_config);
>  
> + /* Add MIPI IO reset programming for modeset */
> + val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
> + I915_WRITE(BXT_P_CR_GT_DISP_PWRON,
> + val | MIPIO_RST_CTRL);
> +
Looking good but overall we still need to address Jani's comment to
limit these changes only to Broxton platform.

>   intel_dsi_prepare(encoder, pipe_config);
>  
>   /* Panel Enable over CRC PMIC */
> @@ -575,6 +581,12 @@ static void intel_dsi_pre_enable(struct
> intel_encoder *encoder,
>   I915_WRITE(DSPCLK_GATE_D, val);
>   }
>  
> + /* Power up DSI regulator */
> + I915_WRITE(BXT_P_DSI_REGULATOR_CFG, STAP_SELECT);
> + val = I915_READ(BXT_P_DSI_REGULATOR_TX_CTRL);
> + val &= ~HS_IO_CTRL_SELECT;
> + I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL, val);
> + /* put device in ready state */
>   intel_dsi_device_ready(encoder);
>  
> @@ -707,6 +719,7 @@ static void intel_dsi_post_disable(struct
> intel_encoder *encoder,
>  {
>   struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
>   struct intel_dsi *intel_dsi = enc_to_intel_dsi(
> >base);
> + u32 val;
>  
>   DRM_DEBUG_KMS("\n");
>  
> @@ -714,8 +727,16 @@ static void intel_dsi_post_disable(struct
> intel_encoder *encoder,
>  
>   intel_dsi_clear_device_ready(encoder);
>  
> + val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
> + I915_WRITE(BXT_P_CR_GT_DISP_PWRON,
> + val & ~MIPIO_RST_CTRL);
> +
>   intel_disable_dsi_pll(encoder);
>  
> + /* Power down DSI regulator to save power */
> + I915_WRITE(BXT_P_DSI_REGULATOR_CFG, STAP_SELECT);
> + I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL, HS_IO_CTRL_SELECT);
> +
>   if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>   u32 val;
>  
-- 
Mika Kahola - Intel OTC

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx