Re: [PATCH v5] drm/i915: add bxt gmbus support

From: Imre Deak <imre.deak@intel.com>
To: Jani Nikula <jani.nikula@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v5] drm/i915: add bxt gmbus support
Date: Thu, 09 Apr 2015 14:12:15 +0300	[thread overview]
Message-ID: <1428577935.22233.10.camel@ideak-mobl> (raw)
In-Reply-To: <1427875085-31497-1-git-send-email-jani.nikula@intel.com>

On Wed, 2015-04-01 at 10:58 +0300, Jani Nikula wrote:
> For BXT gmbus is pulled from PCH to CPU. From implementation point of
> view only pin pair configuration will change. The existing
> implementation supports all platforms previous to GEN8 and also SKL. But
> for BXT pin pair configuration is completely different than SKL or other
> previous GEN's. This patch introduces the new pin pair configuration
> structure specific to BXT and also ensures every real gmbus port has a
> gpio pin.
> 
> v3 by Jani: with the platform independent prep work in place, the bxt
> enabling reduces to a fairly trivial patch. Credits are due Sunil for
> giving me the ideas (with his patches) what the platform independent
> parts should look like.
> 
> v4: Fix intel_hdmi_init_connector() for bxt. Abstract gmbus_pin access
> more. s/GPU/PCH/ in commit message.
> 
> v5: Rebase.
> 
> Issue: VIZ-3574
> Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h   |  3 +++
>  drivers/gpu/drm/i915/intel_hdmi.c | 14 +++++++++++---
>  drivers/gpu/drm/i915/intel_i2c.c  | 30 +++++++++++++++++++++++++++---
>  3 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b134fa39c5b8..a2889013088f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1797,6 +1797,9 @@ enum skl_disp_power_wells {
>  #define   GMBUS_PIN_DPB		5 /* SDVO, HDMIB */
>  #define   GMBUS_PIN_DPD		6 /* HDMID */
>  #define   GMBUS_PIN_RESERVED	7 /* 7 reserved */
> +#define   GMBUS_PIN_1_BXT	1
> +#define   GMBUS_PIN_2_BXT	2
> +#define   GMBUS_PIN_3_BXT	3

I would have called these GMBUS_PIN_DDI_{B,C,A}_BXT to have a more
descriptive name (and since the pin->port mapping is fixed). Either way
the patch looks ok:
Reviewed-by: Imre Deak <imre.deak@intel.com>

>  #define   GMBUS_NUM_PINS	7 /* including 0 */
>  #define GMBUS1			0x5104 /* command/status */
>  #define   GMBUS_SW_CLR_INT	(1<<31)
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 26222e6c1ff3..587bab6e5f60 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1681,15 +1681,23 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  
>  	switch (port) {
>  	case PORT_B:
> -		intel_hdmi->ddc_bus = GMBUS_PIN_DPB;
> +		if (IS_BROXTON(dev_priv))
> +			intel_hdmi->ddc_bus = GMBUS_PIN_1_BXT;
> +		else
> +			intel_hdmi->ddc_bus = GMBUS_PIN_DPB;
>  		intel_encoder->hpd_pin = HPD_PORT_B;
>  		break;
>  	case PORT_C:
> -		intel_hdmi->ddc_bus = GMBUS_PIN_DPC;
> +		if (IS_BROXTON(dev_priv))
> +			intel_hdmi->ddc_bus = GMBUS_PIN_2_BXT;
> +		else
> +			intel_hdmi->ddc_bus = GMBUS_PIN_DPC;
>  		intel_encoder->hpd_pin = HPD_PORT_C;
>  		break;
>  	case PORT_D:
> -		if (IS_CHERRYVIEW(dev))
> +		if (WARN_ON(IS_BROXTON(dev_priv)))
> +			intel_hdmi->ddc_bus = GMBUS_PIN_DISABLED;
> +		else if (IS_CHERRYVIEW(dev_priv))
>  			intel_hdmi->ddc_bus = GMBUS_PIN_DPD_CHV;
>  		else
>  			intel_hdmi->ddc_bus = GMBUS_PIN_DPD;
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index ec9cc8cf642e..cadbc17d2775 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -49,10 +49,33 @@ static const struct gmbus_pin gmbus_pins[] = {
>  	[GMBUS_PIN_DPD] = { "dpd", GPIOF },
>  };
>  
> +static const struct gmbus_pin gmbus_pins_bxt[] = {
> +	[GMBUS_PIN_1_BXT] = { "dpb", PCH_GPIOB },
> +	[GMBUS_PIN_2_BXT] = { "dpc", PCH_GPIOC },
> +	[GMBUS_PIN_3_BXT] = { "misc", PCH_GPIOD },
> +};
> +
> +/* pin is expected to be valid */
> +static const struct gmbus_pin *get_gmbus_pin(struct drm_i915_private *dev_priv,
> +					     unsigned int pin)
> +{
> +	if (IS_BROXTON(dev_priv))
> +		return &gmbus_pins_bxt[pin];
> +	else
> +		return &gmbus_pins[pin];
> +}
> +
>  bool intel_gmbus_is_valid_pin(struct drm_i915_private *dev_priv,
>  			      unsigned int pin)
>  {
> -	return pin < ARRAY_SIZE(gmbus_pins) && gmbus_pins[pin].reg;
> +	unsigned int size;
> +
> +	if (IS_BROXTON(dev_priv))
> +		size = ARRAY_SIZE(gmbus_pins_bxt);
> +	else
> +		size = ARRAY_SIZE(gmbus_pins);
> +
> +	return pin < size && get_gmbus_pin(dev_priv, pin)->reg;
>  }
>  
>  /* Intel GPIO access functions */
> @@ -196,7 +219,8 @@ intel_gpio_setup(struct intel_gmbus *bus, unsigned int pin)
>  
>  	algo = &bus->bit_algo;
>  
> -	bus->gpio_reg = dev_priv->gpio_mmio_base + gmbus_pins[pin].reg;
> +	bus->gpio_reg = dev_priv->gpio_mmio_base +
> +		get_gmbus_pin(dev_priv, pin)->reg;
>  
>  	bus->adapter.algo_data = algo;
>  	algo->setsda = set_data;
> @@ -550,7 +574,7 @@ int intel_setup_gmbus(struct drm_device *dev)
>  		snprintf(bus->adapter.name,
>  			 sizeof(bus->adapter.name),
>  			 "i915 gmbus %s",
> -			 gmbus_pins[pin].name);
> +			 get_gmbus_pin(dev_priv, pin)->name);
>  
>  		bus->adapter.dev.parent = &dev->pdev->dev;
>  		bus->dev_priv = dev_priv;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx