From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shobhit Kumar Subject: Re: [PATCH v2 5/7] drm/i915: Reorganize the DSI enable/disable sequence Date: Wed, 20 Nov 2013 07:09:00 +0530 Message-ID: <528C12B4.2080002@intel.com> References: <1383990548-30737-1-git-send-email-shobhit.kumar@intel.com> <1383990548-30737-6-git-send-email-shobhit.kumar@intel.com> <87r4aigjde.fsf@intel.com> <20131115085540.GN22741@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 399FD1057BA for ; Tue, 19 Nov 2013 17:39:05 -0800 (PST) In-Reply-To: <20131115085540.GN22741@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Daniel Vetter , Jani Nikula Cc: vijayakumar.balakrishnan@intel.com, intel-gfx , yogesh.mohan.marimuthu@intel.com List-Id: intel-gfx@lists.freedesktop.org On Friday 15 November 2013 02:25 PM, Daniel Vetter wrote: > On Fri, Nov 15, 2013 at 10:27:25AM +0200, Jani Nikula wrote: >> On Sat, 09 Nov 2013, Shobhit Kumar 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 >>> Signed-off-by: Shobhit Kumar >>> --- >>> 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. Ok. Will work on updating the patch accordingly and take care of other comments as well in next patch set update soon. Regards Shobhit