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