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

[Shobhit Kumar <shobhit.kumar@intel.com>]

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 <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.

Ok. Will work on updating the patch accordingly and take care of other 
comments as well in next patch set update soon.

Regards
Shobhit