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: Fri, 06 Dec 2013 16:50:21 +0530 Message-ID: <52A1B2F5.8060303@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> <528C12B4.2080002@intel.com> 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 EAC8DFB9F0 for ; Fri, 6 Dec 2013 03:20:26 -0800 (PST) In-Reply-To: <528C12B4.2080002@intel.com> 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 Wednesday 20 November 2013 07:09 AM, Shobhit Kumar wrote: > 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. Sorry took me more time than I anticipated to rework on this due to other critical stuff. Looking at the code and doing more testing I now confirmed that there is no READ/Modify/WRITE needed for ULPS and hence I will convert I915_WRITE_BITS to normal I915_WRITE. Will be sending updated patches on Monday Regards Shobhit