[PATCH 0/2] Enable VLV to work in BIOS-less system.

[PATCH 0/2] Enable VLV to work in BIOS-less system.

[Chon Ming Lee]

In non PC system, such as IVI, may not use BIOS, instead it uses boot 
loader with only minimal system initialization.  Most of the time, boot 
loader doesn't come with VBIOS, and depends on device driver to fully 
initialize the display controller and GPU.

For Valleyview, without VBIOS, i915 fails to work.  The patch add some 
missing init code, such as doing a DPIO CMNRESET and program the GMBUS 
frequency. 

Chon Ming Lee (2):
  drm/i915: Send a DPIO cmnreset during driver load or system resume.
  drm/i915: Program GMBUS Frequency based on the CDCLK

 drivers/gpu/drm/i915/i915_reg.h     |    8 ++++++
 drivers/gpu/drm/i915/intel_i2c.c    |   43 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uncore.c |   15 ++++++++++++
 3 files changed, 66 insertions(+), 0 deletions(-)

-- 
1.7.7.6

[PATCH 1/2] drm/i915: Send a DPIO cmnreset during driver load or system resume.

[Chon Ming Lee]

Without the DPIO cmnreset, the PLL fail to lock.  This should have
done by BIOS.

v2: Move this to intel_uncore_sanitize to allow it to get call during
resume path. (Daniel)

Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 8649f1c..b1f53f3 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -276,10 +276,25 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev)
 
 void intel_uncore_sanitize(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 reg_val;
+
 	intel_uncore_forcewake_reset(dev);
 
 	/* BIOS often leaves RC6 enabled, but disable it for hw init */
 	intel_disable_gt_powersave(dev);
+
+	/* Trigger DPIO CMN RESET, require especially in BIOS less
+	 * system
+	 */
+	if (IS_VALLEYVIEW(dev)) {
+		reg_val = I915_READ(DPIO_CTL);
+		if (!(reg_val & 0x1)) {
+			I915_WRITE(DPIO_CTL, 0x0);
+			I915_WRITE(DPIO_CTL, 0x1);
+			POSTING_READ(DPIO_CTL);
+		}
+	}
 }
 
 /*
-- 
1.7.7.6

Re: [PATCH 1/2] drm/i915: Send a DPIO cmnreset during driver load or system resume.

[Ville Syrjälä]

On Fri, Sep 13, 2013 at 02:39:20PM +0800, Chon Ming Lee wrote:
> Without the DPIO cmnreset, the PLL fail to lock.  This should have
> done by BIOS.
> 
> v2: Move this to intel_uncore_sanitize to allow it to get call during
> resume path. (Daniel)
> 
> Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 8649f1c..b1f53f3 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -276,10 +276,25 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev)
>  
>  void intel_uncore_sanitize(struct drm_device *dev)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 reg_val;
> +
>  	intel_uncore_forcewake_reset(dev);
>  
>  	/* BIOS often leaves RC6 enabled, but disable it for hw init */
>  	intel_disable_gt_powersave(dev);
> +
> +	/* Trigger DPIO CMN RESET, require especially in BIOS less
> +	 * system
> +	 */
> +	if (IS_VALLEYVIEW(dev)) {
> +		reg_val = I915_READ(DPIO_CTL);
> +		if (!(reg_val & 0x1)) {
> +			I915_WRITE(DPIO_CTL, 0x0);

The relevant bit is already 0. Does this write do something useful? I
get the impression it just directly controls the cmnreset line, so if
it's already 0 the reset is already asserted and rewriting w/ 0 does
nothing.

Also we have a name for the bit in i915_reg.h, so might as well use it.

The spec makes one mention that we could assert cmnreset again after all
DPLL and lanes are unused, but in another place it explcitly says we
shouldn't touch it after the initial setup. I guess we just have to trust
that not touching more than once is the right option.

And BTW this is also step 6 in the enable sequence. Don't you need the
earlier steps (some clock setup stuff from the looks of it)?

> +			I915_WRITE(DPIO_CTL, 0x1);
> +			POSTING_READ(DPIO_CTL);
> +		}
> +	}
>  }
>  
>  /*
> -- 
> 1.7.7.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH 1/2] drm/i915: Send a DPIO cmnreset during driver load or system resume.

[Chon Ming Lee]

Without the DPIO cmnreset, the PLL fail to lock.  This should have
done by BIOS.

v2: Move this to intel_uncore_sanitize to allow it to get call during
resume path. (Daniel)
v3: Remove redundant write 0 to DPIO_CTL, and use DPIO_RESET instead of
just 0x1 (Ville)
    Without BIOS, DPIO/render well/media well may still power gated.
Turn it off.

Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     |    9 +++++++++
 drivers/gpu/drm/i915/intel_uncore.c |   23 +++++++++++++++++++++++
 2 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c4f9bef..c02f893 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -361,6 +361,15 @@
 #define PUNIT_OPCODE_REG_READ			6
 #define PUNIT_OPCODE_REG_WRITE			7
 
+#define PUNIT_REG_PWRGT_CTRL			0x60
+#define PUNIT_REG_PWRGT_STATUS			0x61
+#define	  PUNIT_CLK_GATE			1
+#define	  PUNIT_PWR_RESET			2
+#define	  PUNIT_PWR_GATE			3
+#define	  RENDER_PWRGT				(PUNIT_PWR_GATE << 0)
+#define	  MEDIA_PWRGT				(PUNIT_PWR_GATE << 2)
+#define	  DPIO_PWRGT				(PUNIT_PWR_GATE << 6)
+
 #define PUNIT_REG_GPU_LFM			0xd3
 #define PUNIT_REG_GPU_FREQ_REQ			0xd4
 #define PUNIT_REG_GPU_FREQ_STS			0xd8
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 8649f1c..6923b4d 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -276,10 +276,33 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev)
 
 void intel_uncore_sanitize(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 reg_val;
+
 	intel_uncore_forcewake_reset(dev);
 
 	/* BIOS often leaves RC6 enabled, but disable it for hw init */
 	intel_disable_gt_powersave(dev);
+
+	/* Trigger DPIO CMN RESET and turn off power gate, require
+	 * especially in BIOS less system
+	 */
+	if (IS_VALLEYVIEW(dev)) {
+
+		mutex_lock(&dev_priv->rps.hw_lock);
+		reg_val = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS);
+
+		if (reg_val & (RENDER_PWRGT | MEDIA_PWRGT | DPIO_PWRGT))
+			vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL, 0x0);
+
+		mutex_unlock(&dev_priv->rps.hw_lock);
+
+		reg_val = I915_READ(DPIO_CTL);
+		if (!(reg_val & DPIO_RESET)) {
+			I915_WRITE(DPIO_CTL, DPIO_RESET);
+			POSTING_READ(DPIO_CTL);
+		}
+	}
 }
 
 /*
-- 
1.7.7.6

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

[Chon Ming Lee]

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@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)
+
 /*
  * 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 };
+	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;
+
+	/* 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;
+
+	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

Re: [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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: Program GMBUS Frequency based on the CDCLK

[Daniel Vetter]

On Fri, Sep 13, 2013 at 1:23 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> 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.

I think that should be done in the patch series which enables dynamic
cdclk changes (if we get there). Then we can also think about what we
need to do with the dp aux stuff, especially since we already have
quite a bit of logic in there to deal with different dp ref clocks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 2/2] drm/i915: Program GMBUS Frequency based on the CDCLK for VLV.

[Chon Ming Lee]

CDCLK is used to generate the gmbus clock.  This is normally done by
BIOS. Program the value if the BIOS-less system doesn't do it.

v2: Move this to intel_i2c_reset to allow reprogram the gmbus frequency
during resume. (Daniel)

v3: Change GMBUS_FREQ to GMBUSFREQ_VLV, and use VLV_DISPLAY_BASE.
(Ville).
	Remove cdclk_ratio[] table, and calculate the cdclk ratio instead.
(Ville).
 	Change the shift then mask for reg read, to mask first, then shift.
(Ville).
	Remove the gmbus frequency calculation = cdclk/1.01.  Based on BIOS
programming, gmbus frequency = cdclk frequency. (Ville)
	Add get_disp_clk_div, which can use to get cdclk/czclk divide.

Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |    8 +++++
 drivers/gpu/drm/i915/intel_i2c.c |   60 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c02f893..e8315c6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -391,6 +391,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)
@@ -1438,6 +1440,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 << CDCLK_FREQ_SHIFT)
+#define   CZCLK_FREQ_MASK	0xf
+#define GMBUSFREQ_VLV		(VLV_DISPLAY_BASE + 0x6510)
+
 /*
  * Palette regs
  */
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d1c1e0f7..b225d60 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -34,6 +34,11 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+enum disp_clk {
+	CDCLK = 0,
+	CZCLK
+};
+
 struct gmbus_port {
 	const char *name;
 	int reg;
@@ -58,10 +63,65 @@ to_intel_gmbus(struct i2c_adapter *i2c)
 	return container_of(i2c, struct intel_gmbus, adapter);
 }
 
+static int get_disp_clk_div(struct drm_i915_private *dev_priv, enum disp_clk clk)
+{
+	u32 reg_val;
+	int clk_ratio;
+
+	reg_val = I915_READ(CZCLK_CDCLK_FREQ_RATIO);
+
+	if (clk == CDCLK)
+		clk_ratio = ((reg_val & CDCLK_FREQ_MASK) >> CDCLK_FREQ_SHIFT) + 1;
+	else
+		clk_ratio = (reg_val & CZCLK_FREQ_MASK) + 1;
+
+	return clk_ratio * 5;
+}
+
+static void gmbus_set_freq(struct drm_i915_private *dev_priv)
+{
+	int vco_freq[] = { 800, 1600, 2000, 2400 };
+	int gmbus_freq = 0, cdclk_div, hpll_freq;
+	u32 reg_val;
+
+	BUG_ON(!IS_VALLEYVIEW(dev_priv->dev));
+
+	/* Skip setting the gmbus freq if BIOS has already programmed it */
+	if (I915_READ(GMBUSFREQ_VLV) != 0xA0)
+		return;
+
+	/* 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 divide ratio */
+	cdclk_div = get_disp_clk_div(dev_priv, CDCLK);
+
+	/* Program the gmbus_freq based on the cdclk frequency */
+	if (cdclk_div)
+		gmbus_freq = vco_freq[hpll_freq] * 10 / cdclk_div;
+
+	WARN_ON(gmbus_freq == 0);
+
+	if (gmbus_freq != 0)
+		I915_WRITE(GMBUSFREQ_VLV, 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

Re: [PATCH 2/2] drm/i915: Program GMBUS Frequency based on the CDCLK for VLV.

[Ville Syrjälä]

On Tue, Sep 24, 2013 at 05:47:57PM +0800, Chon Ming Lee wrote:
> CDCLK is used to generate the gmbus clock.  This is normally done by
> BIOS. Program the value if the BIOS-less system doesn't do it.
> 
> v2: Move this to intel_i2c_reset to allow reprogram the gmbus frequency
> during resume. (Daniel)
> 
> v3: Change GMBUS_FREQ to GMBUSFREQ_VLV, and use VLV_DISPLAY_BASE.
> (Ville).
> 	Remove cdclk_ratio[] table, and calculate the cdclk ratio instead.
> (Ville).
>  	Change the shift then mask for reg read, to mask first, then shift.
> (Ville).
> 	Remove the gmbus frequency calculation = cdclk/1.01.  Based on BIOS
> programming, gmbus frequency = cdclk frequency. (Ville)
> 	Add get_disp_clk_div, which can use to get cdclk/czclk divide.
> 
> Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |    8 +++++
>  drivers/gpu/drm/i915/intel_i2c.c |   60 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c02f893..e8315c6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -391,6 +391,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)
> @@ -1438,6 +1440,12 @@
>  
>  #define MI_ARB_VLV		(VLV_DISPLAY_BASE + 0x6504)
>  
> +#define CZCLK_CDCLK_FREQ_RATIO	(dev_priv->info->display_mmio_offset + 0x6508)
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This too could be VLV_DISPLAY_BASE.

> +#define   CDCLK_FREQ_SHIFT	4
> +#define   CDCLK_FREQ_MASK	(0x1f << CDCLK_FREQ_SHIFT)
> +#define   CZCLK_FREQ_MASK	0xf
> +#define GMBUSFREQ_VLV		(VLV_DISPLAY_BASE + 0x6510)
> +
>  /*
>   * Palette regs
>   */
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index d1c1e0f7..b225d60 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -34,6 +34,11 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  
> +enum disp_clk {
> +	CDCLK = 0,
             ^^^^

That part is not necessary. Enums start at 0 by default.

> +	CZCLK
> +};
> +
>  struct gmbus_port {
>  	const char *name;
>  	int reg;
> @@ -58,10 +63,65 @@ to_intel_gmbus(struct i2c_adapter *i2c)
>  	return container_of(i2c, struct intel_gmbus, adapter);
>  }
>  
> +static int get_disp_clk_div(struct drm_i915_private *dev_priv, enum disp_clk clk)
> +{
> +	u32 reg_val;
> +	int clk_ratio;
> +
> +	reg_val = I915_READ(CZCLK_CDCLK_FREQ_RATIO);
> +
> +	if (clk == CDCLK)
> +		clk_ratio = ((reg_val & CDCLK_FREQ_MASK) >> CDCLK_FREQ_SHIFT) + 1;
> +	else
> +		clk_ratio = (reg_val & CZCLK_FREQ_MASK) + 1;
> +
> +	return clk_ratio * 5;

This factor 5 multiplication is a bit pointless.

> +}
> +
> +static void gmbus_set_freq(struct drm_i915_private *dev_priv)
> +{
> +	int vco_freq[] = { 800, 1600, 2000, 2400 };
> +	int gmbus_freq = 0, cdclk_div, hpll_freq;
> +	u32 reg_val;
> +
> +	BUG_ON(!IS_VALLEYVIEW(dev_priv->dev));
> +
> +	/* Skip setting the gmbus freq if BIOS has already programmed it */
> +	if (I915_READ(GMBUSFREQ_VLV) != 0xA0)
> +		return;
> +
> +	/* 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 divide ratio */
> +	cdclk_div = get_disp_clk_div(dev_priv, CDCLK);
> +
> +	/* Program the gmbus_freq based on the cdclk frequency */
> +	if (cdclk_div)
> +		gmbus_freq = vco_freq[hpll_freq] * 10 / cdclk_div;

Should be '... / (4 * cdclk_div)' to get the 4MHz.

> +
> +	WARN_ON(gmbus_freq == 0);
> +
> +	if (gmbus_freq != 0)
> +		I915_WRITE(GMBUSFREQ_VLV, gmbus_freq);

We could avoid the double comparison like so:

	if (WARN_ON(gmbus_freq == 0))
		return;

	I915_WRITE(GMBUSFREQ_VLV, 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.
> +	 */

The style police may complain about this multiline comment. The correct
format is:

/*
 * foo
 * bar
 */

> +	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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: Program GMBUS Frequency based on the CDCLK for VLV.

[Lee, Chon Ming]

On 09/24 15:54, Ville Syrjälä wrote:
> On Tue, Sep 24, 2013 at 05:47:57PM +0800, Chon Ming Lee wrote:
> > CDCLK is used to generate the gmbus clock.  This is normally done by
> > BIOS. Program the value if the BIOS-less system doesn't do it.
> > 
> > v2: Move this to intel_i2c_reset to allow reprogram the gmbus frequency
> > during resume. (Daniel)
> > 
> > v3: Change GMBUS_FREQ to GMBUSFREQ_VLV, and use VLV_DISPLAY_BASE.
> > (Ville).
> > 	Remove cdclk_ratio[] table, and calculate the cdclk ratio instead.
> > (Ville).
> >  	Change the shift then mask for reg read, to mask first, then shift.
> > (Ville).
> > 	Remove the gmbus frequency calculation = cdclk/1.01.  Based on BIOS
> > programming, gmbus frequency = cdclk frequency. (Ville)
> > 	Add get_disp_clk_div, which can use to get cdclk/czclk divide.
> > 
> > Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  |    8 +++++
> >  drivers/gpu/drm/i915/intel_i2c.c |   60 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 68 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index c02f893..e8315c6 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -391,6 +391,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)
> > @@ -1438,6 +1440,12 @@
> >  
> >  #define MI_ARB_VLV		(VLV_DISPLAY_BASE + 0x6504)
> >  
> > +#define CZCLK_CDCLK_FREQ_RATIO	(dev_priv->info->display_mmio_offset + 0x6508)
>                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> This too could be VLV_DISPLAY_BASE.

ah, thanks for catching this. 
> 
> > +#define   CDCLK_FREQ_SHIFT	4
> > +#define   CDCLK_FREQ_MASK	(0x1f << CDCLK_FREQ_SHIFT)
> > +#define   CZCLK_FREQ_MASK	0xf
> > +#define GMBUSFREQ_VLV		(VLV_DISPLAY_BASE + 0x6510)
> > +
> >  /*
> >   * Palette regs
> >   */
> > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > index d1c1e0f7..b225d60 100644
> > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > @@ -34,6 +34,11 @@
> >  #include <drm/i915_drm.h>
> >  #include "i915_drv.h"
> >  
> > +enum disp_clk {
> > +	CDCLK = 0,
>              ^^^^
> 
> That part is not necessary. Enums start at 0 by default.
> 
> > +	CZCLK
> > +};
> > +
> >  struct gmbus_port {
> >  	const char *name;
> >  	int reg;
> > @@ -58,10 +63,65 @@ to_intel_gmbus(struct i2c_adapter *i2c)
> >  	return container_of(i2c, struct intel_gmbus, adapter);
> >  }
> >  
> > +static int get_disp_clk_div(struct drm_i915_private *dev_priv, enum disp_clk clk)
> > +{
> > +	u32 reg_val;
> > +	int clk_ratio;
> > +
> > +	reg_val = I915_READ(CZCLK_CDCLK_FREQ_RATIO);
> > +
> > +	if (clk == CDCLK)
> > +		clk_ratio = ((reg_val & CDCLK_FREQ_MASK) >> CDCLK_FREQ_SHIFT) + 1;
> > +	else
> > +		clk_ratio = (reg_val & CZCLK_FREQ_MASK) + 1;
> > +
> > +	return clk_ratio * 5;
> 
> This factor 5 multiplication is a bit pointless.
> 
This is to get the divide ratio.  I don't understand why you think it is not
needed.  


> > +}
> > +
> > +static void gmbus_set_freq(struct drm_i915_private *dev_priv)
> > +{
> > +	int vco_freq[] = { 800, 1600, 2000, 2400 };
> > +	int gmbus_freq = 0, cdclk_div, hpll_freq;
> > +	u32 reg_val;
> > +
> > +	BUG_ON(!IS_VALLEYVIEW(dev_priv->dev));
> > +
> > +	/* Skip setting the gmbus freq if BIOS has already programmed it */
> > +	if (I915_READ(GMBUSFREQ_VLV) != 0xA0)
> > +		return;
> > +
> > +	/* 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 divide ratio */
> > +	cdclk_div = get_disp_clk_div(dev_priv, CDCLK);
> > +
> > +	/* Program the gmbus_freq based on the cdclk frequency */
> > +	if (cdclk_div)
> > +		gmbus_freq = vco_freq[hpll_freq] * 10 / cdclk_div;
> 
> Should be '... / (4 * cdclk_div)' to get the 4MHz.
> 
Actually, the calculation should be correct.  I am getting the same result when
compare to BIOS. The spec is quite confuse when mention about 4MHz.

> > +
> > +	WARN_ON(gmbus_freq == 0);
> > +
> > +	if (gmbus_freq != 0)
> > +		I915_WRITE(GMBUSFREQ_VLV, gmbus_freq);
> 
> We could avoid the double comparison like so:
> 
> 	if (WARN_ON(gmbus_freq == 0))
> 		return;
> 
> 	I915_WRITE(GMBUSFREQ_VLV, gmbus_freq);
> 
Noted on this.  
> > +
> > +}
> > +
> >  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.
> > +	 */
> 
> The style police may complain about this multiline comment. The correct
> format is:
> 
> /*
>  * foo
>  * bar
>  */
> 
> > +	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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

Re: [PATCH 2/2] drm/i915: Program GMBUS Frequency based on the CDCLK for VLV.

[Ville Syrjälä]

On Tue, Sep 24, 2013 at 09:18:57PM +0800, Lee, Chon Ming wrote:
> On 09/24 15:54, Ville Syrjälä wrote:
> > On Tue, Sep 24, 2013 at 05:47:57PM +0800, Chon Ming Lee wrote:
> > > CDCLK is used to generate the gmbus clock.  This is normally done by
> > > BIOS. Program the value if the BIOS-less system doesn't do it.
> > > 
> > > v2: Move this to intel_i2c_reset to allow reprogram the gmbus frequency
> > > during resume. (Daniel)
> > > 
> > > v3: Change GMBUS_FREQ to GMBUSFREQ_VLV, and use VLV_DISPLAY_BASE.
> > > (Ville).
> > > 	Remove cdclk_ratio[] table, and calculate the cdclk ratio instead.
> > > (Ville).
> > >  	Change the shift then mask for reg read, to mask first, then shift.
> > > (Ville).
> > > 	Remove the gmbus frequency calculation = cdclk/1.01.  Based on BIOS
> > > programming, gmbus frequency = cdclk frequency. (Ville)
> > > 	Add get_disp_clk_div, which can use to get cdclk/czclk divide.
> > > 
> > > Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h  |    8 +++++
> > >  drivers/gpu/drm/i915/intel_i2c.c |   60 ++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 68 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index c02f893..e8315c6 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -391,6 +391,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)
> > > @@ -1438,6 +1440,12 @@
> > >  
> > >  #define MI_ARB_VLV		(VLV_DISPLAY_BASE + 0x6504)
> > >  
> > > +#define CZCLK_CDCLK_FREQ_RATIO	(dev_priv->info->display_mmio_offset + 0x6508)
> >                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > This too could be VLV_DISPLAY_BASE.
> 
> ah, thanks for catching this. 
> > 
> > > +#define   CDCLK_FREQ_SHIFT	4
> > > +#define   CDCLK_FREQ_MASK	(0x1f << CDCLK_FREQ_SHIFT)
> > > +#define   CZCLK_FREQ_MASK	0xf
> > > +#define GMBUSFREQ_VLV		(VLV_DISPLAY_BASE + 0x6510)
> > > +
> > >  /*
> > >   * Palette regs
> > >   */
> > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > > index d1c1e0f7..b225d60 100644
> > > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > > @@ -34,6 +34,11 @@
> > >  #include <drm/i915_drm.h>
> > >  #include "i915_drv.h"
> > >  
> > > +enum disp_clk {
> > > +	CDCLK = 0,
> >              ^^^^
> > 
> > That part is not necessary. Enums start at 0 by default.
> > 
> > > +	CZCLK
> > > +};
> > > +
> > >  struct gmbus_port {
> > >  	const char *name;
> > >  	int reg;
> > > @@ -58,10 +63,65 @@ to_intel_gmbus(struct i2c_adapter *i2c)
> > >  	return container_of(i2c, struct intel_gmbus, adapter);
> > >  }
> > >  
> > > +static int get_disp_clk_div(struct drm_i915_private *dev_priv, enum disp_clk clk)
> > > +{
> > > +	u32 reg_val;
> > > +	int clk_ratio;
> > > +
> > > +	reg_val = I915_READ(CZCLK_CDCLK_FREQ_RATIO);
> > > +
> > > +	if (clk == CDCLK)
> > > +		clk_ratio = ((reg_val & CDCLK_FREQ_MASK) >> CDCLK_FREQ_SHIFT) + 1;
> > > +	else
> > > +		clk_ratio = (reg_val & CZCLK_FREQ_MASK) + 1;
> > > +
> > > +	return clk_ratio * 5;
> > 
> > This factor 5 multiplication is a bit pointless.
> > 
> This is to get the divide ratio.  I don't understand why you think it is not
> needed.  

We can leave it as .1 binary fixed point. The *5 just convertes it
into .1 decimal fixed point. We don't need the extra precision.

Then you just replace the '* 10' in the gmbus_freq calculation with
'<< 1'.

> 
> 
> > > +}
> > > +
> > > +static void gmbus_set_freq(struct drm_i915_private *dev_priv)
> > > +{
> > > +	int vco_freq[] = { 800, 1600, 2000, 2400 };
> > > +	int gmbus_freq = 0, cdclk_div, hpll_freq;
> > > +	u32 reg_val;
> > > +
> > > +	BUG_ON(!IS_VALLEYVIEW(dev_priv->dev));
> > > +
> > > +	/* Skip setting the gmbus freq if BIOS has already programmed it */
> > > +	if (I915_READ(GMBUSFREQ_VLV) != 0xA0)
> > > +		return;
> > > +
> > > +	/* 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 divide ratio */
> > > +	cdclk_div = get_disp_clk_div(dev_priv, CDCLK);
> > > +
> > > +	/* Program the gmbus_freq based on the cdclk frequency */
> > > +	if (cdclk_div)
> > > +		gmbus_freq = vco_freq[hpll_freq] * 10 / cdclk_div;
> > 
> > Should be '... / (4 * cdclk_div)' to get the 4MHz.
> > 
> Actually, the calculation should be correct.  I am getting the same result when
> compare to BIOS. The spec is quite confuse when mention about 4MHz.

Is the BIOS correct? And is the spec correct? The calculation you
do here would produce 1MHz reference frequency for GMBUS based on
the spec.

The default value 0xa0 would give us 1.25-2 MHz depending on the CDCLK.

If we program in the wrong value the GMBUS rate select bits will not
give us the frequency we want. Might be easier to hook it up to a scope
and see what it actually does.

> 
> > > +
> > > +	WARN_ON(gmbus_freq == 0);
> > > +
> > > +	if (gmbus_freq != 0)
> > > +		I915_WRITE(GMBUSFREQ_VLV, gmbus_freq);
> > 
> > We could avoid the double comparison like so:
> > 
> > 	if (WARN_ON(gmbus_freq == 0))
> > 		return;
> > 
> > 	I915_WRITE(GMBUSFREQ_VLV, gmbus_freq);
> > 
> Noted on this.  
> > > +
> > > +}
> > > +
> > >  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.
> > > +	 */
> > 
> > The style police may complain about this multiline comment. The correct
> > format is:
> > 
> > /*
> >  * foo
> >  * bar
> >  */
> > 
> > > +	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@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC

[PATCH 2/2] drm/i915: Program GMBUS Frequency based on the CDCLK for VLV.

[Chon Ming Lee]

CDCLK is used to generate the gmbus clock.  This is normally done by
BIOS. Program the value if the BIOS-less system doesn't do it.

v2: Move this to intel_i2c_reset to allow reprogram the gmbus frequency
during resume. (Daniel)

v3: Change GMBUS_FREQ to GMBUSFREQ_VLV, and use VLV_DISPLAY_BASE.
(Ville).
	Remove cdclk_ratio[] table, and calculate the cdclk ratio instead.
(Ville).
 	Change the shift then mask for reg read, to mask first, then shift.
(Ville).
	Remove the gmbus frequency calculation = cdclk/1.01.  Based on BIOS
programming, gmbus frequency = cdclk frequency. (Ville)
	Add get_disp_clk_div, which can use to get cdclk/czclk divide.

v4: Fix the mmio_offset base for CZCLK_CDCLK_FREQ_RATIO, gmbus_freq
calculation, and duplicate check for gmbus_freq. (Ville)

In VLV, the spec is wrong about 4Mhz reference frequency for GMBUS. It
should be 1Mhz.

Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |    8 +++++
 drivers/gpu/drm/i915/intel_i2c.c |   57 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index a0a9811..b37dfd8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -391,6 +391,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)
@@ -1438,6 +1440,12 @@
 
 #define MI_ARB_VLV		(VLV_DISPLAY_BASE + 0x6504)
 
+#define CZCLK_CDCLK_FREQ_RATIO	(VLV_DISPLAY_BASE + 0x6508)
+#define   CDCLK_FREQ_SHIFT	4
+#define   CDCLK_FREQ_MASK	(0x1f << CDCLK_FREQ_SHIFT)
+#define   CZCLK_FREQ_MASK	0xf
+#define GMBUSFREQ_VLV		(VLV_DISPLAY_BASE + 0x6510)
+
 /*
  * Palette regs
  */
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d1c1e0f7..b579ffd 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -34,6 +34,11 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+enum disp_clk {
+	CDCLK,
+	CZCLK
+};
+
 struct gmbus_port {
 	const char *name;
 	int reg;
@@ -58,10 +63,62 @@ to_intel_gmbus(struct i2c_adapter *i2c)
 	return container_of(i2c, struct intel_gmbus, adapter);
 }
 
+static int get_disp_clk_div(struct drm_i915_private *dev_priv, enum disp_clk clk)
+{
+	u32 reg_val;
+	int clk_ratio;
+
+	reg_val = I915_READ(CZCLK_CDCLK_FREQ_RATIO);
+
+	if (clk == CDCLK)
+		clk_ratio = ((reg_val & CDCLK_FREQ_MASK) >> CDCLK_FREQ_SHIFT) + 1;
+	else
+		clk_ratio = (reg_val & CZCLK_FREQ_MASK) + 1;
+
+	return clk_ratio;
+}
+
+static void gmbus_set_freq(struct drm_i915_private *dev_priv)
+{
+	int vco_freq[] = { 800, 1600, 2000, 2400 };
+	int gmbus_freq = 0, cdclk_div, hpll_freq;
+
+	BUG_ON(!IS_VALLEYVIEW(dev_priv->dev));
+
+	/* Skip setting the gmbus freq if BIOS has already programmed it */
+	if (I915_READ(GMBUSFREQ_VLV) != 0xA0)
+		return;
+
+	/* Obtain SKU information */
+	mutex_lock(&dev_priv->dpio_lock);
+	hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) & CCK_FUSE_HPLL_FREQ_MASK;
+	mutex_unlock(&dev_priv->dpio_lock);
+
+	/* Get the CDCLK divide ratio */
+	cdclk_div = get_disp_clk_div(dev_priv, CDCLK);
+
+	/* Program the gmbus_freq based on the cdclk frequency */
+	if (cdclk_div)
+		gmbus_freq = (vco_freq[hpll_freq] << 1) / cdclk_div;
+
+	if (WARN_ON(gmbus_freq == 0))
+		return;
+
+	I915_WRITE(GMBUSFREQ_VLV, 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

Re: [PATCH 2/2] drm/i915: Program GMBUS Frequency based on the CDCLK for VLV.

[Ville Syrjälä]

On Fri, Sep 27, 2013 at 03:31:00PM +0800, Chon Ming Lee wrote:
> CDCLK is used to generate the gmbus clock.  This is normally done by
> BIOS. Program the value if the BIOS-less system doesn't do it.
> 
> v2: Move this to intel_i2c_reset to allow reprogram the gmbus frequency
> during resume. (Daniel)
> 
> v3: Change GMBUS_FREQ to GMBUSFREQ_VLV, and use VLV_DISPLAY_BASE.
> (Ville).
> 	Remove cdclk_ratio[] table, and calculate the cdclk ratio instead.
> (Ville).
>  	Change the shift then mask for reg read, to mask first, then shift.
> (Ville).
> 	Remove the gmbus frequency calculation = cdclk/1.01.  Based on BIOS
> programming, gmbus frequency = cdclk frequency. (Ville)
> 	Add get_disp_clk_div, which can use to get cdclk/czclk divide.
> 
> v4: Fix the mmio_offset base for CZCLK_CDCLK_FREQ_RATIO, gmbus_freq
> calculation, and duplicate check for gmbus_freq. (Ville)
> 
> In VLV, the spec is wrong about 4Mhz reference frequency for GMBUS. It
> should be 1Mhz.
> 
> Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |    8 +++++
>  drivers/gpu/drm/i915/intel_i2c.c |   57 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a0a9811..b37dfd8 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -391,6 +391,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)
> @@ -1438,6 +1440,12 @@
>  
>  #define MI_ARB_VLV		(VLV_DISPLAY_BASE + 0x6504)
>  
> +#define CZCLK_CDCLK_FREQ_RATIO	(VLV_DISPLAY_BASE + 0x6508)
> +#define   CDCLK_FREQ_SHIFT	4
> +#define   CDCLK_FREQ_MASK	(0x1f << CDCLK_FREQ_SHIFT)
> +#define   CZCLK_FREQ_MASK	0xf
> +#define GMBUSFREQ_VLV		(VLV_DISPLAY_BASE + 0x6510)
> +
>  /*
>   * Palette regs
>   */
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index d1c1e0f7..b579ffd 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -34,6 +34,11 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  
> +enum disp_clk {
> +	CDCLK,
> +	CZCLK
> +};
> +
>  struct gmbus_port {
>  	const char *name;
>  	int reg;
> @@ -58,10 +63,62 @@ to_intel_gmbus(struct i2c_adapter *i2c)
>  	return container_of(i2c, struct intel_gmbus, adapter);
>  }
>  
> +static int get_disp_clk_div(struct drm_i915_private *dev_priv, enum disp_clk clk)
> +{
> +	u32 reg_val;
> +	int clk_ratio;
> +
> +	reg_val = I915_READ(CZCLK_CDCLK_FREQ_RATIO);
> +
> +	if (clk == CDCLK)
> +		clk_ratio = ((reg_val & CDCLK_FREQ_MASK) >> CDCLK_FREQ_SHIFT) + 1;
> +	else
> +		clk_ratio = (reg_val & CZCLK_FREQ_MASK) + 1;
> +
> +	return clk_ratio;
> +}
> +
> +static void gmbus_set_freq(struct drm_i915_private *dev_priv)
> +{
> +	int vco_freq[] = { 800, 1600, 2000, 2400 };
> +	int gmbus_freq = 0, cdclk_div, hpll_freq;
> +
> +	BUG_ON(!IS_VALLEYVIEW(dev_priv->dev));
> +
> +	/* Skip setting the gmbus freq if BIOS has already programmed it */
> +	if (I915_READ(GMBUSFREQ_VLV) != 0xA0)
> +		return;
> +
> +	/* Obtain SKU information */
> +	mutex_lock(&dev_priv->dpio_lock);
> +	hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) & CCK_FUSE_HPLL_FREQ_MASK;
> +	mutex_unlock(&dev_priv->dpio_lock);
> +
> +	/* Get the CDCLK divide ratio */
> +	cdclk_div = get_disp_clk_div(dev_priv, CDCLK);
> +
> +	/* Program the gmbus_freq based on the cdclk frequency */
> +	if (cdclk_div)
> +		gmbus_freq = (vco_freq[hpll_freq] << 1) / cdclk_div;


OK based on the information that you dug up that we do need to aim for
1MHz GMBUS freq, the patch looks good to me. I think we should add a
comment here about the 1MHz vs. 4MHz what the spec says. Eg.:

/*
 * Program the gmbus_freq based on the cdclk frequency.
 * BSpec erroneously claims we should aim for 4MHz, but
 * in fact 1MHz is the correct frequency.
 */

Maybe Daniel can add that when applying the patch...

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +	if (WARN_ON(gmbus_freq == 0))
> +		return;
> +
> +	I915_WRITE(GMBUSFREQ_VLV, 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: Program GMBUS Frequency based on the CDCLK for VLV.

[Daniel Vetter]

On Fri, Sep 27, 2013 at 01:07:19PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 27, 2013 at 03:31:00PM +0800, Chon Ming Lee wrote:
> > CDCLK is used to generate the gmbus clock.  This is normally done by
> > BIOS. Program the value if the BIOS-less system doesn't do it.
> > 
> > v2: Move this to intel_i2c_reset to allow reprogram the gmbus frequency
> > during resume. (Daniel)
> > 
> > v3: Change GMBUS_FREQ to GMBUSFREQ_VLV, and use VLV_DISPLAY_BASE.
> > (Ville).
> > 	Remove cdclk_ratio[] table, and calculate the cdclk ratio instead.
> > (Ville).
> >  	Change the shift then mask for reg read, to mask first, then shift.
> > (Ville).
> > 	Remove the gmbus frequency calculation = cdclk/1.01.  Based on BIOS
> > programming, gmbus frequency = cdclk frequency. (Ville)
> > 	Add get_disp_clk_div, which can use to get cdclk/czclk divide.
> > 
> > v4: Fix the mmio_offset base for CZCLK_CDCLK_FREQ_RATIO, gmbus_freq
> > calculation, and duplicate check for gmbus_freq. (Ville)
> > 
> > In VLV, the spec is wrong about 4Mhz reference frequency for GMBUS. It
> > should be 1Mhz.
> > 
> > Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  |    8 +++++
> >  drivers/gpu/drm/i915/intel_i2c.c |   57 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index a0a9811..b37dfd8 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -391,6 +391,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)
> > @@ -1438,6 +1440,12 @@
> >  
> >  #define MI_ARB_VLV		(VLV_DISPLAY_BASE + 0x6504)
> >  
> > +#define CZCLK_CDCLK_FREQ_RATIO	(VLV_DISPLAY_BASE + 0x6508)
> > +#define   CDCLK_FREQ_SHIFT	4
> > +#define   CDCLK_FREQ_MASK	(0x1f << CDCLK_FREQ_SHIFT)
> > +#define   CZCLK_FREQ_MASK	0xf
> > +#define GMBUSFREQ_VLV		(VLV_DISPLAY_BASE + 0x6510)
> > +
> >  /*
> >   * Palette regs
> >   */
> > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > index d1c1e0f7..b579ffd 100644
> > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > @@ -34,6 +34,11 @@
> >  #include <drm/i915_drm.h>
> >  #include "i915_drv.h"
> >  
> > +enum disp_clk {
> > +	CDCLK,
> > +	CZCLK
> > +};
> > +
> >  struct gmbus_port {
> >  	const char *name;
> >  	int reg;
> > @@ -58,10 +63,62 @@ to_intel_gmbus(struct i2c_adapter *i2c)
> >  	return container_of(i2c, struct intel_gmbus, adapter);
> >  }
> >  
> > +static int get_disp_clk_div(struct drm_i915_private *dev_priv, enum disp_clk clk)
> > +{
> > +	u32 reg_val;
> > +	int clk_ratio;
> > +
> > +	reg_val = I915_READ(CZCLK_CDCLK_FREQ_RATIO);
> > +
> > +	if (clk == CDCLK)
> > +		clk_ratio = ((reg_val & CDCLK_FREQ_MASK) >> CDCLK_FREQ_SHIFT) + 1;
> > +	else
> > +		clk_ratio = (reg_val & CZCLK_FREQ_MASK) + 1;
> > +
> > +	return clk_ratio;
> > +}
> > +
> > +static void gmbus_set_freq(struct drm_i915_private *dev_priv)
> > +{
> > +	int vco_freq[] = { 800, 1600, 2000, 2400 };
> > +	int gmbus_freq = 0, cdclk_div, hpll_freq;
> > +
> > +	BUG_ON(!IS_VALLEYVIEW(dev_priv->dev));
> > +
> > +	/* Skip setting the gmbus freq if BIOS has already programmed it */
> > +	if (I915_READ(GMBUSFREQ_VLV) != 0xA0)
> > +		return;
> > +
> > +	/* Obtain SKU information */
> > +	mutex_lock(&dev_priv->dpio_lock);
> > +	hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) & CCK_FUSE_HPLL_FREQ_MASK;
> > +	mutex_unlock(&dev_priv->dpio_lock);
> > +
> > +	/* Get the CDCLK divide ratio */
> > +	cdclk_div = get_disp_clk_div(dev_priv, CDCLK);
> > +
> > +	/* Program the gmbus_freq based on the cdclk frequency */
> > +	if (cdclk_div)
> > +		gmbus_freq = (vco_freq[hpll_freq] << 1) / cdclk_div;
> 
> 
> OK based on the information that you dug up that we do need to aim for
> 1MHz GMBUS freq, the patch looks good to me. I think we should add a
> comment here about the 1MHz vs. 4MHz what the spec says. Eg.:
> 
> /*
>  * Program the gmbus_freq based on the cdclk frequency.
>  * BSpec erroneously claims we should aim for 4MHz, but
>  * in fact 1MHz is the correct frequency.
>  */
> 
> Maybe Daniel can add that when applying the patch...

Done. Also I've massaged the patch a bit to appease checkpatch. Please run
patches through that tool before submitting ...

> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch