[v8,34/38] drm/i915/icl: Add changes to program DSI panel GPIOs

Submitted by Jani Nikula on Oct. 30, 2018, 11:56 a.m.

Details

Message ID 7687f8e90fa88b95c8acf617af5e33c06ed68152.1540900289.git.jani.nikula@intel.com
State New
Headers show
Series "drm/i915/icl: dsi enabling" ( rev: 3 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Jani Nikula Oct. 30, 2018, 11:56 a.m.
From: Madhav Chauhan <madhav.chauhan@intel.com>

For ICELAKE DSI, Display Pins are the only GPIOs
that need to be programmed. So DSI driver should have
its own implementation to toggle these pins based on
GPIO info coming from VBT sequences instead of using
platform specific GPIO driver.

Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_vbt.c | 46 +++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
index 8177305b9c87..04423248bbd7 100644
--- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
@@ -334,6 +334,48 @@  static void bxt_exec_gpio(struct drm_i915_private *dev_priv,
 	gpiod_set_value(gpio_desc, value);
 }
 
+static void icl_exec_gpio(struct drm_i915_private *dev_priv,
+			  u8 gpio_source, u8 gpio_index, bool value)
+{
+	u32 val;
+
+	switch (gpio_index) {
+	case ICL_GPIO_DDSP_HPD_A:
+		val = I915_READ(SHOTPLUG_CTL_DDI);
+		val &= ~ICP_DDIA_HPD_ENABLE;
+		I915_WRITE(SHOTPLUG_CTL_DDI, val);
+		val = I915_READ(SHOTPLUG_CTL_DDI);
+
+		if (value)
+			val |= ICP_DDIA_HPD_OP_DRIVE_1;
+		else
+			val &= ~ICP_DDIA_HPD_OP_DRIVE_1;
+
+		I915_WRITE(SHOTPLUG_CTL_DDI, val);
+		break;
+	case ICL_GPIO_L_VDDEN_1:
+		val = I915_READ(ICP_PP_CONTROL(1));
+		if (value)
+			val |= PWR_STATE_TARGET;
+		else
+			val &= ~PWR_STATE_TARGET;
+		I915_WRITE(ICP_PP_CONTROL(1), val);
+		break;
+	case ICL_GPIO_L_BKLTEN_1:
+		val = I915_READ(ICP_PP_CONTROL(1));
+		if (value)
+			val |= BACKLIGHT_ENABLE;
+		else
+			val &= ~BACKLIGHT_ENABLE;
+		I915_WRITE(ICP_PP_CONTROL(1), val);
+		break;
+	default:
+		/* TODO: Add support for remaining GPIOs */
+		DRM_ERROR("Invalid GPIO no from VBT\n");
+		break;
+	}
+}
+
 static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
 {
 	struct drm_device *dev = intel_dsi->base.base.dev;
@@ -357,7 +399,9 @@  static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
 	/* pull up/down */
 	value = *data++ & 1;
 
-	if (IS_VALLEYVIEW(dev_priv))
+	if (IS_ICELAKE(dev_priv))
+		icl_exec_gpio(dev_priv, gpio_source, gpio_index, value);
+	else if (IS_VALLEYVIEW(dev_priv))
 		vlv_exec_gpio(dev_priv, gpio_source, gpio_number, value);
 	else if (IS_CHERRYVIEW(dev_priv))
 		chv_exec_gpio(dev_priv, gpio_source, gpio_number, value);

Comments

On Tue, Oct 30, 2018 at 01:56:40PM +0200, Jani Nikula wrote:
> From: Madhav Chauhan <madhav.chauhan@intel.com>
> 
> For ICELAKE DSI, Display Pins are the only GPIOs
> that need to be programmed. So DSI driver should have
> its own implementation to toggle these pins based on
> GPIO info coming from VBT sequences instead of using
> platform specific GPIO driver.
> 
> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_vbt.c | 46 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
> index 8177305b9c87..04423248bbd7 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
> @@ -334,6 +334,48 @@ static void bxt_exec_gpio(struct drm_i915_private *dev_priv,
>  	gpiod_set_value(gpio_desc, value);
>  }
>  
> +static void icl_exec_gpio(struct drm_i915_private *dev_priv,
> +			  u8 gpio_source, u8 gpio_index, bool value)
> +{
> +	u32 val;
> +
> +	switch (gpio_index) {
> +	case ICL_GPIO_DDSP_HPD_A:
> +		val = I915_READ(SHOTPLUG_CTL_DDI);
> +		val &= ~ICP_DDIA_HPD_ENABLE;
> +		I915_WRITE(SHOTPLUG_CTL_DDI, val);
> +		val = I915_READ(SHOTPLUG_CTL_DDI);
> +		if (value)
> +			val |= ICP_DDIA_HPD_OP_DRIVE_1;
> +		else
> +			val &= ~ICP_DDIA_HPD_OP_DRIVE_1;
> +
> +		I915_WRITE(SHOTPLUG_CTL_DDI, val);

How badly is this thing going to race with the hotplug irq handler?

> +		break;
> +	case ICL_GPIO_L_VDDEN_1:
> +		val = I915_READ(ICP_PP_CONTROL(1));
> +		if (value)
> +			val |= PWR_STATE_TARGET;
> +		else
> +			val &= ~PWR_STATE_TARGET;
> +		I915_WRITE(ICP_PP_CONTROL(1), val);
> +		break;
> +	case ICL_GPIO_L_BKLTEN_1:
> +		val = I915_READ(ICP_PP_CONTROL(1));
> +		if (value)
> +			val |= BACKLIGHT_ENABLE;
> +		else
> +			val &= ~BACKLIGHT_ENABLE;
> +		I915_WRITE(ICP_PP_CONTROL(1), val);

:( What a horror show. So basically we're trying to pretend the power 
sequencer state machine doesn't even exist. Is there some bit somewhere
we can actually use to disable the state machine? If not I think this
thing needs much more care.

> +		break;
> +	default:
> +		/* TODO: Add support for remaining GPIOs */
> +		DRM_ERROR("Invalid GPIO no from VBT\n");
> +		break;
> +	}
> +}
> +
>  static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>  {
>  	struct drm_device *dev = intel_dsi->base.base.dev;
> @@ -357,7 +399,9 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>  	/* pull up/down */
>  	value = *data++ & 1;
>  
> -	if (IS_VALLEYVIEW(dev_priv))
> +	if (IS_ICELAKE(dev_priv))
> +		icl_exec_gpio(dev_priv, gpio_source, gpio_index, value);
> +	else if (IS_VALLEYVIEW(dev_priv))
>  		vlv_exec_gpio(dev_priv, gpio_source, gpio_number, value);
>  	else if (IS_CHERRYVIEW(dev_priv))
>  		chv_exec_gpio(dev_priv, gpio_source, gpio_number, value);
> -- 
> 2.11.0
On Tue, 30 Oct 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Oct 30, 2018 at 01:56:40PM +0200, Jani Nikula wrote:
>> From: Madhav Chauhan <madhav.chauhan@intel.com>
>> 
>> For ICELAKE DSI, Display Pins are the only GPIOs
>> that need to be programmed. So DSI driver should have
>> its own implementation to toggle these pins based on
>> GPIO info coming from VBT sequences instead of using
>> platform specific GPIO driver.
>> 
>> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi_vbt.c | 46 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 45 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
>> index 8177305b9c87..04423248bbd7 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
>> @@ -334,6 +334,48 @@ static void bxt_exec_gpio(struct drm_i915_private *dev_priv,
>>  	gpiod_set_value(gpio_desc, value);
>>  }
>>  
>> +static void icl_exec_gpio(struct drm_i915_private *dev_priv,
>> +			  u8 gpio_source, u8 gpio_index, bool value)
>> +{
>> +	u32 val;
>> +
>> +	switch (gpio_index) {
>> +	case ICL_GPIO_DDSP_HPD_A:
>> +		val = I915_READ(SHOTPLUG_CTL_DDI);
>> +		val &= ~ICP_DDIA_HPD_ENABLE;
>> +		I915_WRITE(SHOTPLUG_CTL_DDI, val);
>> +		val = I915_READ(SHOTPLUG_CTL_DDI);
>> +		if (value)
>> +			val |= ICP_DDIA_HPD_OP_DRIVE_1;
>> +		else
>> +			val &= ~ICP_DDIA_HPD_OP_DRIVE_1;
>> +
>> +		I915_WRITE(SHOTPLUG_CTL_DDI, val);
>
> How badly is this thing going to race with the hotplug irq handler?
>
>> +		break;
>> +	case ICL_GPIO_L_VDDEN_1:
>> +		val = I915_READ(ICP_PP_CONTROL(1));
>> +		if (value)
>> +			val |= PWR_STATE_TARGET;
>> +		else
>> +			val &= ~PWR_STATE_TARGET;
>> +		I915_WRITE(ICP_PP_CONTROL(1), val);
>> +		break;
>> +	case ICL_GPIO_L_BKLTEN_1:
>> +		val = I915_READ(ICP_PP_CONTROL(1));
>> +		if (value)
>> +			val |= BACKLIGHT_ENABLE;
>> +		else
>> +			val &= ~BACKLIGHT_ENABLE;
>> +		I915_WRITE(ICP_PP_CONTROL(1), val);
>
> :( What a horror show. So basically we're trying to pretend the power 
> sequencer state machine doesn't even exist. Is there some bit somewhere
> we can actually use to disable the state machine? If not I think this
> thing needs much more care.

Frankly I didn't look at the patches towards the end of the series all
that much. Just included them all for completeness.

Agreed, looks pretty bad. :(

BR,
Jani.

>
>> +		break;
>> +	default:
>> +		/* TODO: Add support for remaining GPIOs */
>> +		DRM_ERROR("Invalid GPIO no from VBT\n");
>> +		break;
>> +	}
>> +}
>> +
>>  static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>>  {
>>  	struct drm_device *dev = intel_dsi->base.base.dev;
>> @@ -357,7 +399,9 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>>  	/* pull up/down */
>>  	value = *data++ & 1;
>>  
>> -	if (IS_VALLEYVIEW(dev_priv))
>> +	if (IS_ICELAKE(dev_priv))
>> +		icl_exec_gpio(dev_priv, gpio_source, gpio_index, value);
>> +	else if (IS_VALLEYVIEW(dev_priv))
>>  		vlv_exec_gpio(dev_priv, gpio_source, gpio_number, value);
>>  	else if (IS_CHERRYVIEW(dev_priv))
>>  		chv_exec_gpio(dev_priv, gpio_source, gpio_number, value);
>> -- 
>> 2.11.0
On 10/30/2018 7:31 PM, Ville Syrjälä wrote:
> On Tue, Oct 30, 2018 at 01:56:40PM +0200, Jani Nikula wrote:
>> From: Madhav Chauhan <madhav.chauhan@intel.com>
>>
>> For ICELAKE DSI, Display Pins are the only GPIOs
>> that need to be programmed. So DSI driver should have
>> its own implementation to toggle these pins based on
>> GPIO info coming from VBT sequences instead of using
>> platform specific GPIO driver.
>>
>> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dsi_vbt.c | 46 +++++++++++++++++++++++++++++++++++-
>>   1 file changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
>> index 8177305b9c87..04423248bbd7 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
>> @@ -334,6 +334,48 @@ static void bxt_exec_gpio(struct drm_i915_private *dev_priv,
>>   	gpiod_set_value(gpio_desc, value);
>>   }
>>   
>> +static void icl_exec_gpio(struct drm_i915_private *dev_priv,
>> +			  u8 gpio_source, u8 gpio_index, bool value)
>> +{
>> +	u32 val;
>> +
>> +	switch (gpio_index) {
>> +	case ICL_GPIO_DDSP_HPD_A:
>> +		val = I915_READ(SHOTPLUG_CTL_DDI);
>> +		val &= ~ICP_DDIA_HPD_ENABLE;
>> +		I915_WRITE(SHOTPLUG_CTL_DDI, val);
>> +		val = I915_READ(SHOTPLUG_CTL_DDI);
>> +		if (value)
>> +			val |= ICP_DDIA_HPD_OP_DRIVE_1;
>> +		else
>> +			val &= ~ICP_DDIA_HPD_OP_DRIVE_1;
>> +
>> +		I915_WRITE(SHOTPLUG_CTL_DDI, val);
> How badly is this thing going to race with the hotplug irq handler?

Agree, icp_irq_handler will create race condition  here (was aware).

Actually, behavior of GPIO pin like what to do for a particular
GPIO was not available in BSPEC during power-on.
So just added this patch(tmp method) to complete power ON.

My bad, should have added that info in patch description.

>
>> +		break;
>> +	case ICL_GPIO_L_VDDEN_1:
>> +		val = I915_READ(ICP_PP_CONTROL(1));
>> +		if (value)
>> +			val |= PWR_STATE_TARGET;
>> +		else
>> +			val &= ~PWR_STATE_TARGET;
>> +		I915_WRITE(ICP_PP_CONTROL(1), val);
>> +		break;
>> +	case ICL_GPIO_L_BKLTEN_1:
>> +		val = I915_READ(ICP_PP_CONTROL(1));
>> +		if (value)
>> +			val |= BACKLIGHT_ENABLE;
>> +		else
>> +			val &= ~BACKLIGHT_ENABLE;
>> +		I915_WRITE(ICP_PP_CONTROL(1), val);
> :( What a horror show. So basically we're trying to pretend the power
> sequencer state machine doesn't even exist. Is there some bit somewhere
> we can actually use to disable the state machine? If not I think this
> thing needs much more care.
Yes :(,  We need to definitely think about if we can achieve this using 
existing panel backlight
framework functions.
Need discussion to have more clarity on BITS and behavior with VBT team 
and then decide
final approach.

Regards,
Madhav


>
>> +		break;
>> +	default:
>> +		/* TODO: Add support for remaining GPIOs */
>> +		DRM_ERROR("Invalid GPIO no from VBT\n");
>> +		break;
>> +	}
>> +}
>> +
>>   static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>>   {
>>   	struct drm_device *dev = intel_dsi->base.base.dev;
>> @@ -357,7 +399,9 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>>   	/* pull up/down */
>>   	value = *data++ & 1;
>>   
>> -	if (IS_VALLEYVIEW(dev_priv))
>> +	if (IS_ICELAKE(dev_priv))
>> +		icl_exec_gpio(dev_priv, gpio_source, gpio_index, value);
>> +	else if (IS_VALLEYVIEW(dev_priv))
>>   		vlv_exec_gpio(dev_priv, gpio_source, gpio_number, value);
>>   	else if (IS_CHERRYVIEW(dev_priv))
>>   		chv_exec_gpio(dev_priv, gpio_source, gpio_number, value);
>> -- 
>> 2.11.0