Re: [PATCH 7/8] drm/i915: Use cdclk_state->voltage on CNL

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 19, 2017 at 01:56:38PM +0300, Mika Kahola wrote:
> On Thu, 2017-10-19 at 13:48 +0300, Ville Syrjälä wrote:
> > On Wed, Oct 18, 2017 at 02:50:59PM -0700, Rodrigo Vivi wrote:
> > > 
> > > On Wed, Oct 18, 2017 at 08:48:24PM +0000, Ville Syrjala wrote:
> > > > 
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Track the system agent voltage we request from pcode in the cdclk
> > > > state
> > > > on CNL. Annoyingly we can't actually read out the current value
> > > > since
> > > > there's no pcode command to do that, so we'll have to just assume
> > > > that
> > > > it worked.
> > > +static u32 cnl_cur_voltage(struct drm_i915_private *dev_priv)
> > > +{
> > > +       u32 voltage;
> > > +       sandybridge_pcode_read(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> > > &voltage);
> > I don't think that'll work. _pcode_read() and _pcode_write() are
> > actually
> > the same thing, the only difference is whether we return the
> > resulting value
> > or not. Thus if we actually tried to do this we'd end up setting the
> > voltage instead of reading it. 
> > 
> > The only way to actually read anything is for pcode to specify a
> > specific read command. It's a rather unfortunate design of the
> > mailbox interface :(
> > 
> > We should perhaps put some WARNS into the pcode read/write functions
> > so
> > that people don't accidentally use them in the wrong way.
> +1 for this. I was under the assumption that *read* means just reading
> out the value.

Do you want to volunteer for that?

Looking at the current _read() uses, the only one that looks a bit
suspicious is the DUTY_CYCLE_CONTROL. Not sure that's actually
working as expected... Hmm, looks like it's fine actually. Some HSW
PM doc I have says that data0=0x0 means "read dynamic RPe", and
data0=0x1 means "send PCU interrupt when we can go back to RPe".
We might want to add some defines for those magic values to make
the code look less suspicious.

Another thing we could do is deduplicate the code in the 
pcode_read/write functions. Eg. write() could just call read()
maybe, or pull the code into a common _rw() helper (like we
have for the VLV/CHV sideband stuff). Oh, and looks like
intel_sbi_read/write could use the same treatment as well.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx