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

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

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

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