diff mbox

drm/i915/bxt: WA for swapped HPD pins in A stepping

Message ID 1436776809-24315-1-git-send-email-sonika.jindal@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

sonika.jindal@intel.com July 13, 2015, 8:40 a.m. UTC 
As per bspec, on BXT A0/A1, sw needs to activate DDIA HPD logic
and interrupts to check the external panel connection.
And remove the redundant comment.

v2: Remove redundant IS_BROXTON check, Add comment about port C not
connected, and rephrase the commit message to include only what we
are doing here (Imre)
v3: Add comment about the WA, move 'hpd' initialization outside for
loop (Siva)
Also, remove few redundant comments from hpd handler (me)

Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c |   46 ++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 15 deletions(-)

Comments

Daniel Vetter July 13, 2015, 9:40 a.m. UTC | #1 
On Mon, Jul 13, 2015 at 02:10:09PM +0530, Sonika Jindal wrote:
> As per bspec, on BXT A0/A1, sw needs to activate DDIA HPD logic
> and interrupts to check the external panel connection.
> And remove the redundant comment.
> 
> v2: Remove redundant IS_BROXTON check, Add comment about port C not
> connected, and rephrase the commit message to include only what we
> are doing here (Imre)
> v3: Add comment about the WA, move 'hpd' initialization outside for
> loop (Siva)
> Also, remove few redundant comments from hpd handler (me)
> 
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>

Can't we do this in two steps:
- Wire up port A hpd in a generic way.
- Add wa for bxt to use port A hpd in the various encoder setup functions
  where we assign intel_encoder->hpd_pin.

Currently that switchover is spread all through low-level functions, which
makes this a bit confusion. Imo low-level code shouldn't treat hpd A as
anything but hpd A since that's just confusing. And we already have the
infrastructure for encoders to ask for any hpd pin they want really.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c |   46 ++++++++++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a897f68..13cabca 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -88,7 +88,14 @@ static const u32 hpd_status_i915[HPD_NUM_PINS] = {
>  	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
>  };
>  
> -/* BXT hpd list */
> +/*
> + * On BXT A0/A1, sw needs to activate DDIA HPD logic and interrupts to check
> + * the external panel connection. Port C is not connected on bxt A0/A1
> + */
> +static const u32 hpd_bxt_a0[HPD_NUM_PINS] = {
> +	[HPD_PORT_B] = BXT_DE_PORT_HP_DDIA
> +};
> +
>  static const u32 hpd_bxt[HPD_NUM_PINS] = {
>  	[HPD_PORT_B] = BXT_DE_PORT_HP_DDIB,
>  	[HPD_PORT_C] = BXT_DE_PORT_HP_DDIC
> @@ -1967,6 +1974,7 @@ static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 hp_control, hp_trigger;
>  	u32 pin_mask, long_mask;
> +	const u32 *hpd;
>  
>  	/* Get the status */
>  	hp_trigger = iir_status & BXT_DE_PORT_HOTPLUG_MASK;
> @@ -1981,7 +1989,12 @@ static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
>  	/* Clear sticky bits in hpd status */
>  	I915_WRITE(BXT_HOTPLUG_CTL, hp_control);
>  
> -	pch_get_hpd_pins(&pin_mask, &long_mask, hp_trigger, hp_control, hpd_bxt);
> +	if (INTEL_REVID(dev) < BXT_REVID_B0)
> +		hpd = hpd_bxt_a0;
> +	else
> +		hpd = hpd_bxt;
> +
> +	pch_get_hpd_pins(&pin_mask, &long_mask, hp_trigger, hp_control, hpd);
>  	intel_hpd_irq_handler(dev, pin_mask, long_mask);
>  }
>  
> @@ -3021,31 +3034,34 @@ static void bxt_hpd_irq_setup(struct drm_device *dev)
>  	struct intel_encoder *intel_encoder;
>  	u32 hotplug_port = 0;
>  	u32 hotplug_ctrl;
> +	const u32 *hpd;
>  
> -	/* Now, enable HPD */
> -	for_each_intel_encoder(dev, intel_encoder) {
> +	if (INTEL_REVID(dev) < BXT_REVID_B0)
> +		hpd = hpd_bxt_a0;
> +	else
> +		hpd = hpd_bxt;
> +
> +	for_each_intel_encoder(dev, intel_encoder)
>  		if (dev_priv->hotplug.stats[intel_encoder->hpd_pin].state
>  				== HPD_ENABLED)
> -			hotplug_port |= hpd_bxt[intel_encoder->hpd_pin];
> -	}
> +			hotplug_port |= hpd[intel_encoder->hpd_pin];
>  
> -	/* Mask all HPD control bits */
>  	hotplug_ctrl = I915_READ(BXT_HOTPLUG_CTL) & ~BXT_HOTPLUG_CTL_MASK;
>  
> -	/* Enable requested port in hotplug control */
>  	/* TODO: implement (short) HPD support on port A */
> -	WARN_ON_ONCE(hotplug_port & BXT_DE_PORT_HP_DDIA);
> -	if (hotplug_port & BXT_DE_PORT_HP_DDIB)
> -		hotplug_ctrl |= BXT_DDIB_HPD_ENABLE;
> -	if (hotplug_port & BXT_DE_PORT_HP_DDIC)
> -		hotplug_ctrl |= BXT_DDIC_HPD_ENABLE;
> +	if (INTEL_REVID(dev) < BXT_REVID_B0 && (hotplug_port & BXT_DE_PORT_HP_DDIA))
> +		hotplug_ctrl |= BXT_DDIA_HPD_ENABLE;
> +	else {
> +		if (hotplug_port & BXT_DE_PORT_HP_DDIB)
> +			hotplug_ctrl |= BXT_DDIB_HPD_ENABLE;
> +		if (hotplug_port & BXT_DE_PORT_HP_DDIC)
> +			hotplug_ctrl |= BXT_DDIC_HPD_ENABLE;
> +	}
>  	I915_WRITE(BXT_HOTPLUG_CTL, hotplug_ctrl);
>  
> -	/* Unmask DDI hotplug in IMR */
>  	hotplug_ctrl = I915_READ(GEN8_DE_PORT_IMR) & ~hotplug_port;
>  	I915_WRITE(GEN8_DE_PORT_IMR, hotplug_ctrl);
>  
> -	/* Enable DDI hotplug in IER */
>  	hotplug_ctrl = I915_READ(GEN8_DE_PORT_IER) | hotplug_port;
>  	I915_WRITE(GEN8_DE_PORT_IER, hotplug_ctrl);
>  	POSTING_READ(GEN8_DE_PORT_IER);
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Sivakumar Thulasimani July 13, 2015, 10:57 a.m. UTC | #2 
On 7/13/2015 3:10 PM, Daniel Vetter wrote:
> On Mon, Jul 13, 2015 at 02:10:09PM +0530, Sonika Jindal wrote:
>> As per bspec, on BXT A0/A1, sw needs to activate DDIA HPD logic
>> and interrupts to check the external panel connection.
>> And remove the redundant comment.
>>
>> v2: Remove redundant IS_BROXTON check, Add comment about port C not
>> connected, and rephrase the commit message to include only what we
>> are doing here (Imre)
>> v3: Add comment about the WA, move 'hpd' initialization outside for
>> loop (Siva)
>> Also, remove few redundant comments from hpd handler (me)
>>
>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> Can't we do this in two steps:
> - Wire up port A hpd in a generic way.
> - Add wa for bxt to use port A hpd in the various encoder setup functions
>    where we assign intel_encoder->hpd_pin.
>
> Currently that switchover is spread all through low-level functions, which
> makes this a bit confusion. Imo low-level code shouldn't treat hpd A as
> anything but hpd A since that's just confusing. And we already have the
> infrastructure for encoders to ask for any hpd pin they want really.
> -Daniel
>
>> ---
>>   drivers/gpu/drm/i915/i915_irq.c |   46 ++++++++++++++++++++++++++-------------
>>   1 file changed, 31 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index a897f68..13cabca 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -88,7 +88,14 @@ static const u32 hpd_status_i915[HPD_NUM_PINS] = {
>>   	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
>>   };
>>   
>> -/* BXT hpd list */
>> +/*
>> + * On BXT A0/A1, sw needs to activate DDIA HPD logic and interrupts to check
>> + * the external panel connection. Port C is not connected on bxt A0/A1
>> + */
>> +static const u32 hpd_bxt_a0[HPD_NUM_PINS] = {
>> +	[HPD_PORT_B] = BXT_DE_PORT_HP_DDIA
>> +};
>> +
>>   static const u32 hpd_bxt[HPD_NUM_PINS] = {
>>   	[HPD_PORT_B] = BXT_DE_PORT_HP_DDIB,
>>   	[HPD_PORT_C] = BXT_DE_PORT_HP_DDIC
>> @@ -1967,6 +1974,7 @@ static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	u32 hp_control, hp_trigger;
>>   	u32 pin_mask, long_mask;
>> +	const u32 *hpd;
>>   
>>   	/* Get the status */
>>   	hp_trigger = iir_status & BXT_DE_PORT_HOTPLUG_MASK;
>> @@ -1981,7 +1989,12 @@ static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
>>   	/* Clear sticky bits in hpd status */
>>   	I915_WRITE(BXT_HOTPLUG_CTL, hp_control);
>>   
>> -	pch_get_hpd_pins(&pin_mask, &long_mask, hp_trigger, hp_control, hpd_bxt);
>> +	if (INTEL_REVID(dev) < BXT_REVID_B0)
>> +		hpd = hpd_bxt_a0;
>> +	else
>> +		hpd = hpd_bxt;
>> +
>> +	pch_get_hpd_pins(&pin_mask, &long_mask, hp_trigger, hp_control, hpd);
>>   	intel_hpd_irq_handler(dev, pin_mask, long_mask);
>>   }
>>   
>> @@ -3021,31 +3034,34 @@ static void bxt_hpd_irq_setup(struct drm_device *dev)
>>   	struct intel_encoder *intel_encoder;
>>   	u32 hotplug_port = 0;
>>   	u32 hotplug_ctrl;
>> +	const u32 *hpd;
>>   
>> -	/* Now, enable HPD */
>> -	for_each_intel_encoder(dev, intel_encoder) {
>> +	if (INTEL_REVID(dev) < BXT_REVID_B0)
>> +		hpd = hpd_bxt_a0;
>> +	else
>> +		hpd = hpd_bxt;
>> +
>> +	for_each_intel_encoder(dev, intel_encoder)
>>   		if (dev_priv->hotplug.stats[intel_encoder->hpd_pin].state
>>   				== HPD_ENABLED)
>> -			hotplug_port |= hpd_bxt[intel_encoder->hpd_pin];
>> -	}
>> +			hotplug_port |= hpd[intel_encoder->hpd_pin];
>>   
>> -	/* Mask all HPD control bits */
>>   	hotplug_ctrl = I915_READ(BXT_HOTPLUG_CTL) & ~BXT_HOTPLUG_CTL_MASK;
>>   
>> -	/* Enable requested port in hotplug control */
>>   	/* TODO: implement (short) HPD support on port A */
>> -	WARN_ON_ONCE(hotplug_port & BXT_DE_PORT_HP_DDIA);
>> -	if (hotplug_port & BXT_DE_PORT_HP_DDIB)
>> -		hotplug_ctrl |= BXT_DDIB_HPD_ENABLE;
>> -	if (hotplug_port & BXT_DE_PORT_HP_DDIC)
>> -		hotplug_ctrl |= BXT_DDIC_HPD_ENABLE;
>> +	if (INTEL_REVID(dev) < BXT_REVID_B0 && (hotplug_port & BXT_DE_PORT_HP_DDIA))
>> +		hotplug_ctrl |= BXT_DDIA_HPD_ENABLE;
>> +	else {
>> +		if (hotplug_port & BXT_DE_PORT_HP_DDIB)
>> +			hotplug_ctrl |= BXT_DDIB_HPD_ENABLE;
>> +		if (hotplug_port & BXT_DE_PORT_HP_DDIC)
>> +			hotplug_ctrl |= BXT_DDIC_HPD_ENABLE;
>> +	}
>>   	I915_WRITE(BXT_HOTPLUG_CTL, hotplug_ctrl);
>>   
>> -	/* Unmask DDI hotplug in IMR */
>>   	hotplug_ctrl = I915_READ(GEN8_DE_PORT_IMR) & ~hotplug_port;
>>   	I915_WRITE(GEN8_DE_PORT_IMR, hotplug_ctrl);
>>   
>> -	/* Enable DDI hotplug in IER */
>>   	hotplug_ctrl = I915_READ(GEN8_DE_PORT_IER) | hotplug_port;
>>   	I915_WRITE(GEN8_DE_PORT_IER, hotplug_ctrl);
>>   	POSTING_READ(GEN8_DE_PORT_IER);
>> -- 
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
Agreed, that is better.

Sonika,
     lets split this into two patches,
one : will add HPD support for DDIA like it is already done for DDIB & C.
second, will just update hpd_pin for A0/A1 systems as required and avoid
explicit revision checks in irq.c file.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a897f68..13cabca 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -88,7 +88,14 @@  static const u32 hpd_status_i915[HPD_NUM_PINS] = {
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
-/* BXT hpd list */
+/*
+ * On BXT A0/A1, sw needs to activate DDIA HPD logic and interrupts to check
+ * the external panel connection. Port C is not connected on bxt A0/A1
+ */
+static const u32 hpd_bxt_a0[HPD_NUM_PINS] = {
+	[HPD_PORT_B] = BXT_DE_PORT_HP_DDIA
+};
+
 static const u32 hpd_bxt[HPD_NUM_PINS] = {
 	[HPD_PORT_B] = BXT_DE_PORT_HP_DDIB,
 	[HPD_PORT_C] = BXT_DE_PORT_HP_DDIC
@@ -1967,6 +1974,7 @@  static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 hp_control, hp_trigger;
 	u32 pin_mask, long_mask;
+	const u32 *hpd;
 
 	/* Get the status */
 	hp_trigger = iir_status & BXT_DE_PORT_HOTPLUG_MASK;
@@ -1981,7 +1989,12 @@  static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
 	/* Clear sticky bits in hpd status */
 	I915_WRITE(BXT_HOTPLUG_CTL, hp_control);
 
-	pch_get_hpd_pins(&pin_mask, &long_mask, hp_trigger, hp_control, hpd_bxt);
+	if (INTEL_REVID(dev) < BXT_REVID_B0)
+		hpd = hpd_bxt_a0;
+	else
+		hpd = hpd_bxt;
+
+	pch_get_hpd_pins(&pin_mask, &long_mask, hp_trigger, hp_control, hpd);
 	intel_hpd_irq_handler(dev, pin_mask, long_mask);
 }
 
@@ -3021,31 +3034,34 @@  static void bxt_hpd_irq_setup(struct drm_device *dev)
 	struct intel_encoder *intel_encoder;
 	u32 hotplug_port = 0;
 	u32 hotplug_ctrl;
+	const u32 *hpd;
 
-	/* Now, enable HPD */
-	for_each_intel_encoder(dev, intel_encoder) {
+	if (INTEL_REVID(dev) < BXT_REVID_B0)
+		hpd = hpd_bxt_a0;
+	else
+		hpd = hpd_bxt;
+
+	for_each_intel_encoder(dev, intel_encoder)
 		if (dev_priv->hotplug.stats[intel_encoder->hpd_pin].state
 				== HPD_ENABLED)
-			hotplug_port |= hpd_bxt[intel_encoder->hpd_pin];
-	}
+			hotplug_port |= hpd[intel_encoder->hpd_pin];
 
-	/* Mask all HPD control bits */
 	hotplug_ctrl = I915_READ(BXT_HOTPLUG_CTL) & ~BXT_HOTPLUG_CTL_MASK;
 
-	/* Enable requested port in hotplug control */
 	/* TODO: implement (short) HPD support on port A */
-	WARN_ON_ONCE(hotplug_port & BXT_DE_PORT_HP_DDIA);
-	if (hotplug_port & BXT_DE_PORT_HP_DDIB)
-		hotplug_ctrl |= BXT_DDIB_HPD_ENABLE;
-	if (hotplug_port & BXT_DE_PORT_HP_DDIC)
-		hotplug_ctrl |= BXT_DDIC_HPD_ENABLE;
+	if (INTEL_REVID(dev) < BXT_REVID_B0 && (hotplug_port & BXT_DE_PORT_HP_DDIA))
+		hotplug_ctrl |= BXT_DDIA_HPD_ENABLE;
+	else {
+		if (hotplug_port & BXT_DE_PORT_HP_DDIB)
+			hotplug_ctrl |= BXT_DDIB_HPD_ENABLE;
+		if (hotplug_port & BXT_DE_PORT_HP_DDIC)
+			hotplug_ctrl |= BXT_DDIC_HPD_ENABLE;
+	}
 	I915_WRITE(BXT_HOTPLUG_CTL, hotplug_ctrl);
 
-	/* Unmask DDI hotplug in IMR */
 	hotplug_ctrl = I915_READ(GEN8_DE_PORT_IMR) & ~hotplug_port;
 	I915_WRITE(GEN8_DE_PORT_IMR, hotplug_ctrl);
 
-	/* Enable DDI hotplug in IER */
 	hotplug_ctrl = I915_READ(GEN8_DE_PORT_IER) | hotplug_port;
 	I915_WRITE(GEN8_DE_PORT_IER, hotplug_ctrl);
 	POSTING_READ(GEN8_DE_PORT_IER);