Re: [PATCH v2 5/7] drm/i915: Reorganize the DSI enable/disable sequence

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Nov 15, 2013 at 10:27:25AM +0200, Jani Nikula wrote:
> On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> > Basically ULPS handling during enable/disable has been moved to
> > pre_enable and post_disable phases. PLL and panel power disable
> > also has been moved to post_disable phase. The ULPS entry/exit
> > sequneces as suggested by HW team is as follows -
> >
> > During enable time -
> > set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY
> >
> > And during disable time to flush all FIFOs -
> > set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP
> >
> > Also during disbale sequnece sub-encoder disable is moved to the end
> > after port is disabled.
> >
> > v2: Based on comments from Ville
> >     - Detailed epxlaination in the commit messgae
> >     - Moved parameter changes out into another patch
> >     - Backlight enabling will be a new patch
> >
> > Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
> > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |   11 ++++
> >  drivers/gpu/drm/i915/intel_dsi.c |  111 ++++++++++++++++++++++++++------------
> >  drivers/gpu/drm/i915/intel_dsi.h |    2 +
> >  3 files changed, 91 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index a2bbff9..55c16cb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val);
> >  #define POSTING_READ(reg)	(void)I915_READ_NOTRACE(reg)
> >  #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
> >  
> > +#define I915_WRITE_BITS(reg, val, mask) \
> > +do { \
> > +	u32 tmp, data; \
> > +	tmp = I915_READ((reg)); \
> > +	tmp &= ~(mask); \
> > +	data = (val) & (mask); \
> > +	data = data | tmp; \
> > +	I915_WRITE((reg), data); \
> > +} while(0)
> 
> I would still prefer the explicit read, modify, and write in the code
> instead of this, but it's a matter of taste I'll leave for Daniel to
> call the shots on.

Yeah, this looks a bit funny. We could compute the tmp value once (where
the mask is mutliple times the same thing) and then just or in the right
bits.  That should make the I915_WRITE calls fit ont on line, too, which
helps readability.

Also we put POSTING_READs before any waits to ensure the write has
actually landed. It's mostly documentation.

And while I'm at it: We generally frown upon readbacks of register value
and prefer to just keep track of things in software well enough. The
reason for that is that register readbacks allows us too much flexibility
in adding subtile state-depencies. Which long-term makes the code a real
pain to maintain.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch