[Intel-gfx] [PATCH 2/2] drm/i915: Program GMBUS Frequency based on the CDCLK

[Ville Syrjälä]

On Fri, Sep 13, 2013 at 02:39:21PM +0800, Chon Ming Lee wrote:
> CDCLK is used to generate the gmbus clock.  This is normally done by
> BIOS. This is only for valleyview platform.
> 
> v2: Move this to intel_i2c_reset to allow reprogram the gmbus frequency
> during resume. (Daniel)
> 
> Signed-off-by: Chon Ming Lee <chon.ming.lee at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |    8 +++++++
>  drivers/gpu/drm/i915/intel_i2c.c |   43 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index bcee89b..8ddf58a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -382,6 +382,8 @@
>  #define   FB_FMAX_VMIN_FREQ_LO_MASK		0xf8000000
>  
>  /* vlv2 north clock has */
> +#define CCK_FUSE_REG				0x8
> +#define  CCK_FUSE_HPLL_FREQ_MASK		0x3
>  #define CCK_REG_DSI_PLL_FUSE			0x44
>  #define CCK_REG_DSI_PLL_CONTROL			0x48
>  #define  DSI_PLL_VCO_EN				(1 << 31)
> @@ -1424,6 +1426,12 @@
>  
>  #define MI_ARB_VLV		(VLV_DISPLAY_BASE + 0x6504)
>  
> +#define CZCLK_CDCLK_FREQ_RATIO	(dev_priv->info->display_mmio_offset + 0x6508)
> +#define   CDCLK_FREQ_SHIFT	4
> +#define   CDCLK_FREQ_MASK	0x1f
> +#define   CZCLK_FREQ_MASK	0xf
> +#define GMBUS_FREQ		(dev_priv->info->display_mmio_offset + 0x6510)

I believe both of these registers are VLV specific, so you can use
(VLV_DISPLAY_BASE + 0x..)

There's a GMBUSFREQ on CTG at least, but it's a PCI config register
at offset 0xcc. But still, probably best to call the VLV register
GMBUSFREQ_VLV.

> +
>  /*
>   * Palette regs
>   */
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index d1c1e0f7..a8c4165 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -58,10 +58,53 @@ to_intel_gmbus(struct i2c_adapter *i2c)
>  	return container_of(i2c, struct intel_gmbus, adapter);
>  }
>  
> +static void gmbus_set_freq(struct drm_i915_private *dev_priv)
> +{
> +	int cdclk_ratio[] = { 10, 15, 20, 25, 30, 0, 40, 45, 50, 0,
> +			60, 0, 0, 75, 80, 0, 90, 0, 100, 0,
> +			0, 0, 120, 0, 0, 0, 0, 0, 150, 0, 160 };

We can avoid the table. That's just (10 + i * 5), or could be just (2 + i)
and use 2 as the divider.

> +	int vco_freq[] = { 800, 1600, 2000, 2400 };
> +	int gmbus_freq = 0, cdclk, hpll_freq;
> +	u32 reg_val;
> +
> +	BUG_ON(!IS_VALLEYVIEW(dev_priv->dev));
> +
> +	/* Obtain SKU information to determine the correct CDCLK */
> +	mutex_lock(&dev_priv->dpio_lock);
> +	reg_val = vlv_cck_read(dev_priv, CCK_FUSE_REG);
> +	mutex_unlock(&dev_priv->dpio_lock);
> +
> +	hpll_freq = reg_val & CCK_FUSE_HPLL_FREQ_MASK;
> +
> +	/* Get the CDCLK frequency */
> +	reg_val = I915_READ(CZCLK_CDCLK_FREQ_RATIO);
> +
> +	cdclk = ((reg_val >> CDCLK_FREQ_SHIFT) & CDCLK_FREQ_MASK) - 1;

I think we tend to define the masks to be shifted. So mask first, then
shift.

And as stated, we don't need the table. So just make it:

/* in .1 fixed point */
cdclk_div = ((reg_val & MASK) >> SHIFT) + 1;

> +
> +	/* To enable hotplug detect, the gmbus frequency need to set as
> +	 * cdclk/1.01
> +	 */
> +	if (cdclk_ratio[cdclk])
> +		gmbus_freq = vco_freq[hpll_freq] / cdclk_ratio[cdclk] * 101 / 10;

Where does that 1.01 come from? For 200MHz CDCLK GMBUS would be running
at 198MHz? Also I think what you might be actually achieving here is is
a GMBUS frequency of 1/1.01 MHz, since the register supposedly takes the
number of CDCLKs per GMBUS clock.

The spec s a big poorly worded, but I think what it's saying is that
we need to calculate the number of CDCLKs per a 4MHz GMBUS reference
clock. So something like should do the right thing:

/* how many CDCLKs for 4MHz GMBUS reference frequency */
gmbus_freq = ((vco_freq << 1) / (cdclk_div * 4);

Not sure if we should round up or down.

Obviosuly we need to recalculate this as needed when we start to change
CDCLK dynamically. Same for the AUX clock. Actually we should be
able to use the same function to calculate both. The AUX clock is just
2MHz instead of 4MHz.

So you might want to turn this function into some generic function to
calculate both GMBUS and AUX clocks for VLV. You could just take the
target frequency (2 or 4 in these cases) as a parameter and return the
calculated divider.

> +
> +	WARN_ON(gmbus_freq == 0);
> +
> +	if (gmbus_freq != 0)
> +		I915_WRITE(GMBUS_FREQ, gmbus_freq);
> +
> +}
> +
>  void
>  intel_i2c_reset(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* In BIOS-less system, program the correct gmbus frequency
> +	 * before reading edid.
> +	 */
> +	if (IS_VALLEYVIEW(dev))
> +		gmbus_set_freq(dev_priv);
> +
>  	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS0, 0);
>  	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0);
>  }
> -- 
> 1.7.7.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC