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@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
