On Thu, Mar 03, 2016 at 10:47:38AM +0200, Jani Nikula wrote:
> On Wed, 02 Mar 2016, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> >
> > Generalize rawclk handling by storing it in dev_priv.
> >
> > Presumably our hrawclk readout works at least for CTG and ELK
> > since we've been using it for DP AUX on those platforms. There
> > are no real docs anymore after configdb vanished, so the only
> > reference is the public CTG GMCH spec. What bits are listed in
> > that doc match our code. The ELK GMCH spec have no relevant
> > details unfortunately.
> >
> > The PNV situation is less clear. Starting from
> > commit aa17cdb4f836 ("drm/i915: initialize backlight max from VBT")
> > we assume that the CTG/ELK hrawclk readout works for PNV as well.
> > At least the results *seem* reasonable for one PNV machine (Lenovo
> > Ideapad S10-3t). Sadly the PNV GMCH spec doesn't have the goods on
> > the relevant register either.
> >
> > So let's keep assuming it works for PNV,ELK,CTG and read it out on
> > those platforms. G33 also has hrawclk according to some notes
> > in BSpec, but we don't actually need it for anything, so let's not
> > even try to read it out there.
>
> While the above is useful information, I don't think it's specifically
> related to the changes in this patch. The only thing that changes here
> is that the rawclk is read and stored once, instead of being read every
> time.
Sure. But I wanted those considerations on some record in case someone
ever wonders about this code.
>
> There are a few nitpicks below, but I don't insist on doing anything
> with them.
>
> Reviewed-by: Jani Nikula <jani.nik...@intel.com>
>
> >
> > v2: Rebase due to IS_VALLYVIEW vs. IS_CHERRYVIEW split
> > Use KHz() all over, and kill off a few useless temp variables
> >
> > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_display.c | 55
> > ++++++++++++++++++++++--------------
> > drivers/gpu/drm/i915/intel_dp.c | 16 +++++------
> > drivers/gpu/drm/i915/intel_drv.h | 2 --
> > drivers/gpu/drm/i915/intel_panel.c | 42 +++++++++++++--------------
> > 5 files changed, 62 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 2cb0a411c10e..b0e7f35a8be4 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1837,6 +1837,7 @@ struct drm_i915_private {
> > unsigned int skl_boot_cdclk;
> > unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
> > unsigned int max_dotclk_freq;
> > + unsigned int rawclk_freq;
> > unsigned int hpll_freq;
> > unsigned int czclk_freq;
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 44fcff0343f2..330528c1fb28 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -169,49 +169,61 @@ static int vlv_get_cck_clock_hpll(struct
> > drm_i915_private *dev_priv,
> > return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
> > }
> >
> > -int
> > -intel_pch_rawclk(struct drm_device *dev)
> > +static int
> > +intel_pch_rawclk(struct drm_i915_private *dev_priv)
>
> Now that you make these static, it feels a bit silly to have both intel
> and platform in the name. But meh, no need to resend over that.
Yeah, you commented on that before too I think, and I just forgot to
change it when I realized I should resend.
>
> > {
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> > - WARN_ON(!HAS_PCH_SPLIT(dev));
> > + return (I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK) * 1000;
> > +}
> >
> > - return I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
> > +static int
> > +intel_vlv_hrawclk(struct drm_i915_private *dev_priv)
> > +{
> > + return 200000;
> > }
> >
> > -/* hrawclock is 1/4 the FSB frequency */
> > -int intel_hrawclk(struct drm_device *dev)
> > +static int
> > +intel_g4x_hrawclk(struct drm_i915_private *dev_priv)
> > {
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > uint32_t clkcfg;
> >
> > - /* There is no CLKCFG reg in Valleyview. VLV hrawclk is 200 MHz */
> > - if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> > - return 200;
> > -
> > + /* hrawclock is 1/4 the FSB frequency */
> > clkcfg = I915_READ(CLKCFG);
> > switch (clkcfg & CLKCFG_FSB_MASK) {
> > case CLKCFG_FSB_400:
> > - return 100;
> > + return 100000;
> > case CLKCFG_FSB_533:
> > - return 133;
> > + return 133333;
> > case CLKCFG_FSB_667:
> > - return 166;
> > + return 166667;
> > case CLKCFG_FSB_800:
> > - return 200;
> > + return 200000;
> > case CLKCFG_FSB_1067:
> > - return 266;
> > + return 266667;
> > case CLKCFG_FSB_1333:
> > - return 333;
> > + return 333333;
> > /* these two are just a guess; one of them might be right */
> > case CLKCFG_FSB_1600:
> > case CLKCFG_FSB_1600_ALT:
> > - return 400;
> > + return 400000;
> > default:
> > - return 133;
> > + return 133333;
> > }
> > }
> >
> > +static void intel_update_rawclk(struct drm_i915_private *dev_priv)
> > +{
> > + if (HAS_PCH_SPLIT(dev_priv))
> > + dev_priv->rawclk_freq = intel_pch_rawclk(dev_priv);
> > + else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > + dev_priv->rawclk_freq = intel_vlv_hrawclk(dev_priv);
> > + else if (IS_G4X(dev_priv) || IS_PINEVIEW(dev_priv))
> > + dev_priv->rawclk_freq = intel_g4x_hrawclk(dev_priv);
> > + else
> > + return; /* no rawclk on other platforms, or no need to know it
> > */
> > +
> > + DRM_DEBUG_DRIVER("rawclk rate: %d kHz\n", dev_priv->rawclk_freq);
> > +}
> > +
> > static void intel_update_czclk(struct drm_i915_private *dev_priv)
> > {
> > if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)))
> > @@ -15617,6 +15629,7 @@ void intel_modeset_init(struct drm_device *dev)
> > }
> >
> > intel_update_czclk(dev_priv);
> > + intel_update_rawclk(dev_priv);
> > intel_update_cdclk(dev);
> >
> > intel_shared_dpll_init(dev);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index f272b3541e00..6afdfa720974 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -674,13 +674,13 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp,
> > bool has_aux_irq)
> > static uint32_t i9xx_get_aux_clock_divider(struct intel_dp *intel_dp, int
> > index)
> > {
> > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > - struct drm_device *dev = intel_dig_port->base.base.dev;
> > + struct drm_i915_private *dev_priv =
> > to_i915(intel_dig_port->base.base.dev);
> >
> > /*
> > * The clock divider is based off the hrawclk, and would like to run at
> > * 2MHz. So, take the hrawclk value and divide by 2 and use that
> > */
>
> You forget to fix the comment here, but I see that you fix it in a later
> patch... (You know I'm just commenting to show that I've actually read
> the patch properly. ;)
I think you had the exact same comment last time around :)
>
> > - return index ? 0 : DIV_ROUND_CLOSEST(intel_hrawclk(dev), 2);
> > + return index ? 0 : DIV_ROUND_CLOSEST(dev_priv->rawclk_freq, 2000);
> > }
> >
> > static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int
> > index)
> > @@ -692,12 +692,10 @@ static uint32_t ilk_get_aux_clock_divider(struct
> > intel_dp *intel_dp, int index)
> > if (index)
> > return 0;
> >
> > - if (intel_dig_port->port == PORT_A) {
> > + if (intel_dig_port->port == PORT_A)
> > return DIV_ROUND_CLOSEST(dev_priv->cdclk_freq, 2000);
> > -
> > - } else {
> > - return DIV_ROUND_CLOSEST(intel_pch_rawclk(dev), 2);
> > - }
> > + else
> > + return DIV_ROUND_CLOSEST(dev_priv->rawclk_freq, 2000);
> > }
> >
> > static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int
> > index)
> > @@ -718,7 +716,7 @@ static uint32_t hsw_get_aux_clock_divider(struct
> > intel_dp *intel_dp, int index)
> > default: return 0;
> > }
> > } else {
> > - return index ? 0 : DIV_ROUND_CLOSEST(intel_pch_rawclk(dev), 2);
> > + return index ? 0 : DIV_ROUND_CLOSEST(dev_priv->rawclk_freq,
> > 2000);
> > }
> > }
> >
> > @@ -5254,7 +5252,7 @@ intel_dp_init_panel_power_sequencer_registers(struct
> > drm_device *dev,
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > u32 pp_on, pp_off, pp_div, port_sel = 0;
> > - int div = HAS_PCH_SPLIT(dev) ? intel_pch_rawclk(dev) :
> > intel_hrawclk(dev);
> > + int div = dev_priv->rawclk_freq / 1000;
> > i915_reg_t pp_on_reg, pp_off_reg, pp_div_reg, pp_ctrl_reg;
> > enum port port = dp_to_dig_port(intel_dp)->port;
> > const struct edp_power_seq *seq = &intel_dp->pps_delays;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index cb413e246267..e7485ec34f63 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1085,8 +1085,6 @@ void i915_audio_component_cleanup(struct
> > drm_i915_private *dev_priv);
> > extern const struct drm_plane_funcs intel_plane_funcs;
> > unsigned int intel_rotation_info_size(const struct intel_rotation_info
> > *rot_info);
> > bool intel_has_pending_fb_unpin(struct drm_device *dev);
> > -int intel_pch_rawclk(struct drm_device *dev);
> > -int intel_hrawclk(struct drm_device *dev);
> > void intel_mark_busy(struct drm_device *dev);
> > void intel_mark_idle(struct drm_device *dev);
> > void intel_crtc_restore_mode(struct drm_crtc *crtc);
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c
> > b/drivers/gpu/drm/i915/intel_panel.c
> > index 21ee6477bf98..5cf377507162 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -1251,16 +1251,14 @@ static u32 bxt_hz_to_pwm(struct intel_connector
> > *connector, u32 pwm_freq_hz)
> > static u32 spt_hz_to_pwm(struct intel_connector *connector, u32
> > pwm_freq_hz)
> > {
> > struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > - u32 mul, clock;
> > + u32 mul;
> >
> > if (I915_READ(SOUTH_CHICKEN1) & SPT_PWM_GRANULARITY)
> > mul = 128;
> > else
> > mul = 16;
> >
> > - clock = MHz(24);
> > -
> > - return clock / (pwm_freq_hz * mul);
> > + return MHz(24) / (pwm_freq_hz * mul);
>
> Unrelated change, really.
You complained about this sort of stuff last time in the functions that
used rawclk/cdclk, so after I changed those the inconsistency drove me
mad and I had to change everything else as well.
>
> > }
> >
> > /*
> > @@ -1292,10 +1290,9 @@ static u32 lpt_hz_to_pwm(struct intel_connector
> > *connector, u32 pwm_freq_hz)
> > */
> > static u32 pch_hz_to_pwm(struct intel_connector *connector, u32
> > pwm_freq_hz)
> > {
> > - struct drm_device *dev = connector->base.dev;
> > - int clock = MHz(intel_pch_rawclk(dev));
> > + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> >
> > - return clock / (pwm_freq_hz * 128);
> > + return KHz(dev_priv->rawclk_freq) / (pwm_freq_hz * 128);
> > }
> >
> > /*
> > @@ -1308,14 +1305,13 @@ static u32 pch_hz_to_pwm(struct intel_connector
> > *connector, u32 pwm_freq_hz)
> > */
> > static u32 i9xx_hz_to_pwm(struct intel_connector *connector, u32
> > pwm_freq_hz)
> > {
> > - struct drm_device *dev = connector->base.dev;
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > int clock;
> >
> > - if (IS_PINEVIEW(dev))
> > - clock = MHz(intel_hrawclk(dev));
> > + if (IS_PINEVIEW(dev_priv))
> > + clock = KHz(dev_priv->rawclk_freq);
> > else
> > - clock = 1000 * dev_priv->cdclk_freq;
> > + clock = KHz(dev_priv->cdclk_freq);
> >
> > return clock / (pwm_freq_hz * 32);
> > }
> > @@ -1332,9 +1328,9 @@ static u32 i965_hz_to_pwm(struct intel_connector
> > *connector, u32 pwm_freq_hz)
> > int clock;
> >
> > if (IS_G4X(dev_priv))
> > - clock = MHz(intel_hrawclk(dev));
> > + clock = KHz(dev_priv->rawclk_freq);
> > else
> > - clock = 1000 * dev_priv->cdclk_freq;
> > + clock = KHz(dev_priv->cdclk_freq);
> >
> > return clock / (pwm_freq_hz * 128);
> > }
> > @@ -1346,19 +1342,21 @@ static u32 i965_hz_to_pwm(struct intel_connector
> > *connector, u32 pwm_freq_hz)
> > */
> > static u32 vlv_hz_to_pwm(struct intel_connector *connector, u32
> > pwm_freq_hz)
> > {
> > - struct drm_device *dev = connector->base.dev;
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - int clock;
> > + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > + int mul, clock;
> >
> > if ((I915_READ(CBR1_VLV) & CBR_PWM_CLOCK_MUX_SELECT) == 0) {
> > - if (IS_CHERRYVIEW(dev))
> > - return KHz(19200) / (pwm_freq_hz * 16);
> > + if (IS_CHERRYVIEW(dev_priv))
> > + clock = KHz(19200);
> > else
> > - return MHz(25) / (pwm_freq_hz * 16);
> > + clock = MHz(25);
> > + mul = 16;
> > } else {
> > - clock = intel_hrawclk(dev);
> > - return MHz(clock) / (pwm_freq_hz * 128);
> > + clock = KHz(dev_priv->rawclk_freq);
> > + mul = 128;
> > }
> > +
> > + return clock / (pwm_freq_hz * mul);
> > }
> >
> > static u32 get_backlight_max_vbt(struct intel_connector *connector)
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
