[Intel-gfx] [PATCH] drm/i915/adl_s: Add gmbus pin mapping

[Ram Moon, AnandX]

Hi Aditya,

Thanks for your review comments.

-----Original Message-----
From: Aditya Swarup <aditya.swarup at intel.com> 
Sent: Wednesday, February 10, 2021 9:54 PM
To: Ram Moon, AnandX <anandx.ram.moon at intel.com>; intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; Roper, Matthew D <matthew.d.roper at intel.com>; Auld, Matthew <matthew.auld at intel.com>; Surendrakumar Upadhyay, TejaskumarX <tejaskumarx.surendrakumar.upadhyay at intel.com>
Subject: Re: [PATCH] drm/i915/adl_s: Add gmbus pin mapping

On 2/10/21 3:54 AM, Anand Moon wrote:
> Add table to map the GMBUS pin pairs to GPIO registers and port to DDC 
> mapping for ADL_S as per below Bspec.

Has this patch been tested on an ADLS system? Upstream CI AFAIK doesn't have support for ADL-S. Also comments below..

Reason I send this patch so that I could get more review comments for this below changes.
I have gone through the Bspec 20124 and I have debug this patch earlier,  
so that the mapping with DDC pin is correctly mapped
with respect to the configuration table in the Bspec, 
but still we observe GMBUS i2c handshake failure. 
How can we debug this further. 

> 
> Bspec:20124, 53597.
> 
> Cc: Aditya Swarup <aditya.swarup at intel.com>
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> Signed-off-by: Anand Moon <anandx.ram.moon at intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_gmbus.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c 
> b/drivers/gpu/drm/i915/display/intel_gmbus.c
> index 0c952e1d720e..58b8e42d4f90 100644
> --- a/drivers/gpu/drm/i915/display/intel_gmbus.c
> +++ b/drivers/gpu/drm/i915/display/intel_gmbus.c
> @@ -52,6 +52,14 @@ static const struct gmbus_pin gmbus_pins[] = {
>  	[GMBUS_PIN_DPD] = { "dpd", GPIOF },
>  };
>  
> +static const struct gmbus_pin gmbus_pins_adls[] = {
> +	[GMBUS_PIN_1_BXT] = { "edp", GPIOA },

I am pretty sure that GMBUS_PIN_1_BXT should map to GPIOB(1) and not GPIOA(0) like what we have for ICP.

Ok, will update this in next version.

> +	[GMBUS_PIN_9_TC1_ICP] = { "tc1", GPIOD },
> +	[GMBUS_PIN_10_TC2_ICP] = { "tc2", GPIOE },
> +	[GMBUS_PIN_11_TC3_ICP] = { "tc3", GPIOF },
> +	[GMBUS_PIN_12_TC4_ICP] = { "tc4", GPIOG }, };
> +
>  static const struct gmbus_pin gmbus_pins_bdw[] = {
>  	[GMBUS_PIN_VGADDC] = { "vga", GPIOA },
>  	[GMBUS_PIN_DPC] = { "dpc", GPIOD },
> @@ -101,7 +109,9 @@ static const struct gmbus_pin gmbus_pins_dg1[] = {  
> static const struct gmbus_pin *get_gmbus_pin(struct drm_i915_private *dev_priv,
>  					     unsigned int pin)
>  {
> -	if (INTEL_PCH_TYPE(dev_priv) >= PCH_DG1)
> +	if (INTEL_PCH_TYPE(dev_priv) == PCH_ADP)

This check should be converted to IS_ALDERLAKE_S(dev_priv) instead of PCH check for PCH_ADP.
Unfortunately, we are reusing PCH_ADP for ADLS and ADLP which have different mappings but the same ADP PCH.
This will break ADLP. 

The PCH_ADP check for ADLS in intel_bios.c will also be changed in the ADLP patches that are going to be submitted upstream.

So might as well do the correct thing now and change this to IS_ALDERLAKE_S(dev_priv).

Ok will update this in next version.

> +		return &gmbus_pins_adls[pin];
> +	else if (INTEL_PCH_TYPE(dev_priv) >= PCH_DG1)
>  		return &gmbus_pins_dg1[pin];
>  	else if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
>  		return &gmbus_pins_icp[pin];
> @@ -122,7 +132,9 @@ bool intel_gmbus_is_valid_pin(struct 
> drm_i915_private *dev_priv,  {
>  	unsigned int size;
>  
> -	if (INTEL_PCH_TYPE(dev_priv) >= PCH_DG1)
> +	if (INTEL_PCH_TYPE(dev_priv) == PCH_ADP)

See comment above. Change to IS_ALDERLAKE_S(dev_priv)

Ok.

Aditya Swarup

> +		size = ARRAY_SIZE(gmbus_pins_adls);
> +	else if (INTEL_PCH_TYPE(dev_priv) >= PCH_DG1)
>  		size = ARRAY_SIZE(gmbus_pins_dg1);
>  	else if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
>  		size = ARRAY_SIZE(gmbus_pins_icp);
> 

Thanks
-Anand