[PATCH 0/4] drm/i915: Baytrail MIPI DSI support Updated

[PATCH 0/4] drm/i915: Baytrail MIPI DSI support Updated

[Shobhit Kumar]

Hi All - 
These patches enhance the current support for MIPI DSI for Baytrail. They
continue on the sub-encoder design and adds few more dev_ops to handle
sequence correctly. Major changes are -
 1. DSI Clock calculation based on pixel clock
 2. Add new dev_ops for better sequencing the enable/disable path
 3. Parameterized the hardcoded DSI parameters. These also forms building
    block for the generic MIPI driver to come in future based on enhancements
    in VBT. All these parameters are initialized or computed in the sub-encoder
    driver. Some of them might look unneccesary for now.

I am also aware of the drm_bridge support now comming in and will in future
migrate from sub-encoder design to drm_bridge.

This DSI sequence has been validated with couple of test panels and is working now.
Still no sub-encoder driver is included and this support will be mostly be disabled
untill a panel sub-encoder driver is added. Proper detection or VBT is still pending.

Regards
Shobhit

Shobhit Kumar (4):
  drm/i915: Add more dev ops for MIPI sub encoder
  drm/i915: Use FLISDSI interface for band gap reset
  drm/i915: Compute dsi_clk from pixel clock
  drm/i915: Parameterize the MIPI enabling sequnece and adjust the
    sequence

 drivers/gpu/drm/i915/i915_drv.h       |   13 ++
 drivers/gpu/drm/i915/i915_reg.h       |    1 +
 drivers/gpu/drm/i915/intel_dsi.c      |  378 +++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_dsi.h      |   29 +++
 drivers/gpu/drm/i915/intel_dsi_pll.c  |   75 +++++--
 drivers/gpu/drm/i915/intel_sideband.c |   14 ++
 6 files changed, 318 insertions(+), 192 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] drm/i915: Add more dev ops for MIPI sub encoder

[Shobhit Kumar]

Also add new fields in intel_dsi to have all dphy related parameters.
These will be useful even when we go for pure generic MIPI design

Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c |    9 ++++++++-
 drivers/gpu/drm/i915/intel_dsi.h |   29 +++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 9a2fdd2..34e19b7 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -147,6 +147,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
 
 	DRM_DEBUG_KMS("\n");
 
+	if (intel_dsi->dev.dev_ops->panel_reset)
+		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
+
 	temp = I915_READ(MIPI_DEVICE_READY(pipe));
 	if ((temp & DEVICE_READY) == 0) {
 		temp &= ~ULPS_STATE_MASK;
@@ -162,6 +165,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
 		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
 	}
 
+	if (intel_dsi->dev.dev_ops->send_otp_cmds)
+		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
+
 	if (is_cmd_mode(intel_dsi))
 		I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
 
@@ -176,7 +182,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
 		POSTING_READ(MIPI_PORT_CTRL(pipe));
 	}
 
-	intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
+	if (intel_dsi->dev.dev_ops->enable)
+		intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
 }
 
 static void intel_dsi_disable(struct intel_encoder *encoder)
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index c7765f3..b71c9b3 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -39,6 +39,13 @@ struct intel_dsi_device {
 struct intel_dsi_dev_ops {
 	bool (*init)(struct intel_dsi_device *dsi);
 
+	void (*panel_reset)(struct intel_dsi_device *dsi);
+
+	void (*disable_panel_power)(struct intel_dsi_device *dsi);
+
+	/* send one time programmable commands */
+	void (*send_otp_cmds)(struct intel_dsi_device *dsi);
+
 	/* This callback must be able to assume DSI commands can be sent */
 	void (*enable)(struct intel_dsi_device *dsi);
 
@@ -89,6 +96,28 @@ struct intel_dsi {
 
 	/* eot for MIPI_EOT_DISABLE register */
 	u32 eot_disable;
+
+	u16 dsi_clock_freq;
+	u8 video_mode_type;
+	u32 data_width;
+	u8 dither;
+	u32 port_bits;
+	u8 escape_clk_div;
+	u32 lp_rx_timeout;
+	u8 turn_arnd_val;
+	u16 init_count;
+	u16 rst_timer_val;
+	u16 hs_to_lp_count;
+	u16 lp_byte_clk;
+	u32 bw_timer;
+	u16 clk_lp_to_hs_count;
+	u16 clk_hs_to_lp_count;
+	u32 video_frmt_cfg_bits;
+	u32 dphy_reg;
+
+	u8 backlight_off_delay; /*in ms*/
+	bool send_shutdown;
+	u8 shutdown_pkt_delay; /*in ms*/
 };
 
 static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
-- 
1.7.9.5

Re: [PATCH 1/4] drm/i915: Add more dev ops for MIPI sub encoder

[Jani Nikula]

Hi Shobhit -

On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> Also add new fields in intel_dsi to have all dphy related parameters.
> These will be useful even when we go for pure generic MIPI design

I feel like we have a different idea of what the ideal generic design
is. For me, the goal is that we change the struct intel_dsi_device to
struct drm_bridge, and those drm_bridge drivers are all about the panel,
and have as few details about i915 or our hardware as possible. Having
the bridge drivers fill in register values to be written by the core DSI
code does not fit that. Yes, I had some of those in my bridge conversion
patches too, but I did not intend we'd keep adding more.

I'd rather we provide generic helpers the bridge driver can call.

> Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c |    9 ++++++++-
>  drivers/gpu/drm/i915/intel_dsi.h |   29 +++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 9a2fdd2..34e19b7 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -147,6 +147,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  
>  	DRM_DEBUG_KMS("\n");
>  
> +	if (intel_dsi->dev.dev_ops->panel_reset)
> +		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);

Would this map to ->pre_enable in drm_bridge?

> +
>  	temp = I915_READ(MIPI_DEVICE_READY(pipe));
>  	if ((temp & DEVICE_READY) == 0) {
>  		temp &= ~ULPS_STATE_MASK;
> @@ -162,6 +165,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
>  	}
>  
> +	if (intel_dsi->dev.dev_ops->send_otp_cmds)
> +		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);

What is otp? one time programming? Why not in ->enable?

> +
>  	if (is_cmd_mode(intel_dsi))
>  		I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
>  
> @@ -176,7 +182,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  		POSTING_READ(MIPI_PORT_CTRL(pipe));
>  	}
>  
> -	intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
> +	if (intel_dsi->dev.dev_ops->enable)
> +		intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
>  }
>  
>  static void intel_dsi_disable(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index c7765f3..b71c9b3 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -39,6 +39,13 @@ struct intel_dsi_device {
>  struct intel_dsi_dev_ops {
>  	bool (*init)(struct intel_dsi_device *dsi);
>  
> +	void (*panel_reset)(struct intel_dsi_device *dsi);
> +
> +	void (*disable_panel_power)(struct intel_dsi_device *dsi);

What is the enabling counterpart to disable_panel_power? panel_reset?

> +
> +	/* send one time programmable commands */
> +	void (*send_otp_cmds)(struct intel_dsi_device *dsi);
> +
>  	/* This callback must be able to assume DSI commands can be sent */
>  	void (*enable)(struct intel_dsi_device *dsi);
>  
> @@ -89,6 +96,28 @@ struct intel_dsi {
>  
>  	/* eot for MIPI_EOT_DISABLE register */
>  	u32 eot_disable;
> +
> +	u16 dsi_clock_freq;
> +	u8 video_mode_type;
> +	u32 data_width;
> +	u8 dither;
> +	u32 port_bits;
> +	u8 escape_clk_div;
> +	u32 lp_rx_timeout;
> +	u8 turn_arnd_val;
> +	u16 init_count;
> +	u16 rst_timer_val;
> +	u16 hs_to_lp_count;
> +	u16 lp_byte_clk;
> +	u32 bw_timer;
> +	u16 clk_lp_to_hs_count;
> +	u16 clk_hs_to_lp_count;
> +	u32 video_frmt_cfg_bits;
> +	u32 dphy_reg;
> +
> +	u8 backlight_off_delay; /*in ms*/
> +	bool send_shutdown;
> +	u8 shutdown_pkt_delay; /*in ms*/
>  };
>  
>  static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
> -- 
> 1.7.9.5
>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/4] drm/i915: Add more dev ops for MIPI sub encoder

[Shobhit Kumar]

On 10/21/2013 6:57 PM, Jani Nikula wrote:
>
> Hi Shobhit -
>
> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> Also add new fields in intel_dsi to have all dphy related parameters.
>> These will be useful even when we go for pure generic MIPI design
>
> I feel like we have a different idea of what the ideal generic design
> is. For me, the goal is that we change the struct intel_dsi_device to
> struct drm_bridge, and those drm_bridge drivers are all about the panel,
> and have as few details about i915 or our hardware as possible. Having
> the bridge drivers fill in register values to be written by the core DSI
> code does not fit that. Yes, I had some of those in my bridge conversion
> patches too, but I did not intend we'd keep adding more.
>
> I'd rather we provide generic helpers the bridge driver can call.

Yeah, look like our ideas are different. In your goal with drm_bridge 
architecture, we will still end up having multiple bridge drivers for 
each different panel. But my goal is to have a single driver which can 
work for multiple panels. Since we already have enabled some panels with 
sub-encoder architecture for completeness I was planning to maintain 
generic driver as one sub-encoder. But actually we can do away with all 
sub-encoder and do not need even drm_bridge and all implementation will 
be in intel_dsi.c. Panel specific configurations or sequences will come 
from VBT which I have tried to convert as parameters.

All the parameters DSI/DPHY spec specific and none of them particularly 
relate to our hardware.

>
>> Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dsi.c |    9 ++++++++-
>>   drivers/gpu/drm/i915/intel_dsi.h |   29 +++++++++++++++++++++++++++++
>>   2 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 9a2fdd2..34e19b7 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -147,6 +147,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>>
>>   	DRM_DEBUG_KMS("\n");
>>
>> +	if (intel_dsi->dev.dev_ops->panel_reset)
>> +		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
>
> Would this map to ->pre_enable in drm_bridge?

I have not yet migrated to drm_bridge and need to check thes call flows 
for drm_bridge

>
>> +
>>   	temp = I915_READ(MIPI_DEVICE_READY(pipe));
>>   	if ((temp & DEVICE_READY) == 0) {
>>   		temp &= ~ULPS_STATE_MASK;
>> @@ -162,6 +165,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>>   		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
>>   	}
>>
>> +	if (intel_dsi->dev.dev_ops->send_otp_cmds)
>> +		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
>
> What is otp? one time programming? Why not in ->enable?

Yes. OTP is done before sending pixel stream and enable is done after we 
start pixel stream

>
>> +
>>   	if (is_cmd_mode(intel_dsi))
>>   		I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
>>
>> @@ -176,7 +182,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>>   		POSTING_READ(MIPI_PORT_CTRL(pipe));
>>   	}
>>
>> -	intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
>> +	if (intel_dsi->dev.dev_ops->enable)
>> +		intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
>>   }
>>
>>   static void intel_dsi_disable(struct intel_encoder *encoder)
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>> index c7765f3..b71c9b3 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>> @@ -39,6 +39,13 @@ struct intel_dsi_device {
>>   struct intel_dsi_dev_ops {
>>   	bool (*init)(struct intel_dsi_device *dsi);
>>
>> +	void (*panel_reset)(struct intel_dsi_device *dsi);
>> +
>> +	void (*disable_panel_power)(struct intel_dsi_device *dsi);
>
> What is the enabling counterpart to disable_panel_power? panel_reset?

Yes.

>
>> +
>> +	/* send one time programmable commands */
>> +	void (*send_otp_cmds)(struct intel_dsi_device *dsi);
>> +
>>   	/* This callback must be able to assume DSI commands can be sent */
>>   	void (*enable)(struct intel_dsi_device *dsi);
>>
>> @@ -89,6 +96,28 @@ struct intel_dsi {
>>
>>   	/* eot for MIPI_EOT_DISABLE register */
>>   	u32 eot_disable;
>> +
>> +	u16 dsi_clock_freq;
>> +	u8 video_mode_type;
>> +	u32 data_width;
>> +	u8 dither;
>> +	u32 port_bits;
>> +	u8 escape_clk_div;
>> +	u32 lp_rx_timeout;
>> +	u8 turn_arnd_val;
>> +	u16 init_count;
>> +	u16 rst_timer_val;
>> +	u16 hs_to_lp_count;
>> +	u16 lp_byte_clk;
>> +	u32 bw_timer;
>> +	u16 clk_lp_to_hs_count;
>> +	u16 clk_hs_to_lp_count;
>> +	u32 video_frmt_cfg_bits;
>> +	u32 dphy_reg;
>> +
>> +	u8 backlight_off_delay; /*in ms*/
>> +	bool send_shutdown;
>> +	u8 shutdown_pkt_delay; /*in ms*/
>>   };
>>
>>   static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>> --
>> 1.7.9.5
>>
>

Regards
Shobhit

Re: [PATCH 1/4] drm/i915: Add more dev ops for MIPI sub encoder

[Jani Nikula]

On Tue, 22 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> On 10/21/2013 6:57 PM, Jani Nikula wrote:
>>
>> Hi Shobhit -
>>
>> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>>> Also add new fields in intel_dsi to have all dphy related parameters.
>>> These will be useful even when we go for pure generic MIPI design
>>
>> I feel like we have a different idea of what the ideal generic design
>> is. For me, the goal is that we change the struct intel_dsi_device to
>> struct drm_bridge, and those drm_bridge drivers are all about the panel,
>> and have as few details about i915 or our hardware as possible. Having
>> the bridge drivers fill in register values to be written by the core DSI
>> code does not fit that. Yes, I had some of those in my bridge conversion
>> patches too, but I did not intend we'd keep adding more.
>>
>> I'd rather we provide generic helpers the bridge driver can call.
>
> Yeah, look like our ideas are different. In your goal with drm_bridge
> architecture, we will still end up having multiple bridge drivers for
> each different panel. But my goal is to have a single driver which can
> work for multiple panels.

I'm trying to look one or two steps further, and what it will mean to
the driver. Here's the long term goal in upstream as I see it: There
will be a framework in place that allows one to write a (DSI) panel
driver once, using generic APIs, and use that panel driver with any SoC
(that implements the other side of the framework).

We are obviously far away from that goal at the moment. But IMHO we
should keep that in mind as a guide to what we are doing now. Moving
towards a model with a clearly defined API between the DSI core and the
panels, where the panel specific things are abstracted away from the
core, or towards a model where the core and panel driver depend on the
implementation of each other, communicating via variables.

> Since we already have enabled some panels with sub-encoder
> architecture for completeness I was planning to maintain generic
> driver as one sub-encoder.

Nothing prevents you from doing that, as long as the separation between
the core and the panel drivers remains clear.

> But actually we can do away with all sub-encoder and do not need even
> drm_bridge and all implementation will be in intel_dsi.c. Panel
> specific configurations or sequences will come from VBT which I have
> tried to convert as parameters.

With this model it is all too easy to forget what is the panel driver
and what is the SoC driver. They *are* two separate things, and should
not be mixed. It will be all too easy to keep adding new parameters and
conditions in the code as new panel drivers appear to need them. It will
lead to code that is very difficult to understand and maintain.

A model similar to what I'm proposing has also been tried and tested,
with several panels: drivers/video/omap2/. It's not DRM, and the control
is in the panel drivers, but the separation is extremely clear (panel
drivers are separate kernel modules).

No doubt the clear separation between the core and the panel drivers
will be harder and more work in the short term, but it will pay off in
the long term. And it doesn't all have to happen at once, as long as we
work *towards* that goal, not away from it.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/4] drm/i915: Add more dev ops for MIPI sub encoder

[Shobhit Kumar]

Hi Jani,
On 10/22/2013 05:23 PM, Jani Nikula wrote:
> On Tue, 22 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> On 10/21/2013 6:57 PM, Jani Nikula wrote:
>>>
>>> Hi Shobhit -
>>>
>>> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>>>> Also add new fields in intel_dsi to have all dphy related parameters.
>>>> These will be useful even when we go for pure generic MIPI design
>>>
>>> I feel like we have a different idea of what the ideal generic design
>>> is. For me, the goal is that we change the struct intel_dsi_device to
>>> struct drm_bridge, and those drm_bridge drivers are all about the panel,
>>> and have as few details about i915 or our hardware as possible. Having
>>> the bridge drivers fill in register values to be written by the core DSI
>>> code does not fit that. Yes, I had some of those in my bridge conversion
>>> patches too, but I did not intend we'd keep adding more.
>>>
>>> I'd rather we provide generic helpers the bridge driver can call.
>>
>> Yeah, look like our ideas are different. In your goal with drm_bridge
>> architecture, we will still end up having multiple bridge drivers for
>> each different panel. But my goal is to have a single driver which can
>> work for multiple panels.
>
> I'm trying to look one or two steps further, and what it will mean to
> the driver. Here's the long term goal in upstream as I see it: There
> will be a framework in place that allows one to write a (DSI) panel
> driver once, using generic APIs, and use that panel driver with any SoC
> (that implements the other side of the framework).

To clarify in terms of what we have currently, in my opinion intel_dsi.c 
is one such SoC specific implementation for BYT and associated MIPI host 
controller. And sub-encoder drivers are other end and I want to unify 
sub-encoder side to have just one sub-encoder for any panel out there. 
What you are talking makes perfect sense if we are going to have each 
panel driver as a separate driver. But we can still achieve this 
separation with common panel driver as well.

>
> We are obviously far away from that goal at the moment. But IMHO we
> should keep that in mind as a guide to what we are doing now. Moving
> towards a model with a clearly defined API between the DSI core and the
> panels, where the panel specific things are abstracted away from the
> core, or towards a model where the core and panel driver depend on the
> implementation of each other, communicating via variables.

I think we are aligned on the goal and I feel there is a need for common 
DSI core to be separate

>
>> Since we already have enabled some panels with sub-encoder
>> architecture for completeness I was planning to maintain generic
>> driver as one sub-encoder.
>
> Nothing prevents you from doing that, as long as the separation between
> the core and the panel drivers remains clear.
>
>> But actually we can do away with all sub-encoder and do not need even
>> drm_bridge and all implementation will be in intel_dsi.c. Panel
>> specific configurations or sequences will come from VBT which I have
>> tried to convert as parameters.
>
> With this model it is all too easy to forget what is the panel driver
> and what is the SoC driver. They *are* two separate things, and should
> not be mixed. It will be all too easy to keep adding new parameters and
> conditions in the code as new panel drivers appear to need them. It will
> lead to code that is very difficult to understand and maintain.

I think here you have misunderstood my proposal. I still treat SoC 
driver and actual Panel driver as separate. And whatever parameters I 
have tried to add are all DSI/DPHY spec related. There is not even 
single parameter which is panel specific. If you are confusing this 
because of use of -

	if (intel_dsi->dsi_clock_freq)
		dsi_clk = intel_dsi->dsi_clock_freq;

I feel it is okay to have this parameter which provides provision for 
spec parameters to be hard-coded instead of calculating if a panel needs 
and this is *only* such parameter and I already have ways to remove it 
as well. IMHO it is a small price to pay to get one generic panel driver.

That is another thing if the DSI/DPHy specs themselves evolve and we 
need to modify our core, but that is unavoidable I guess.

>
> A model similar to what I'm proposing has also been tried and tested,
> with several panels: drivers/video/omap2/. It's not DRM, and the control
> is in the panel drivers, but the separation is extremely clear (panel
> drivers are separate kernel modules).

Understand, but again my idea is to have one single driver which can 
work for all panels and hence there is no need of multiple panel 
drivers. This works with SoC side of the framework. I have not yet 
described how we can achieve this, but first does it has any merit to 
have something like this ?

>
> No doubt the clear separation between the core and the panel drivers
> will be harder and more work in the short term, but it will pay off in
> the long term. And it doesn't all have to happen at once, as long as we
> work *towards* that goal, not away from it.

I think we should take into account the amount of effort required to 
develop and maintain bridge drivers for tens of MIPI panel out there Vs 
having one panel driver to maintain and make fully spec compliant taking 
care of open ends left by the specs in the best way we can to achieve 
this generality.

Regards
Shobhit

Re: [PATCH 1/4] drm/i915: Add more dev ops for MIPI sub encoder

[Jani Nikula]

On Wed, 23 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> Hi Jani,
> On 10/22/2013 05:23 PM, Jani Nikula wrote:
>> On Tue, 22 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>>> On 10/21/2013 6:57 PM, Jani Nikula wrote:
>>>>
>>>> Hi Shobhit -
>>>>
>>>> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>>>>> Also add new fields in intel_dsi to have all dphy related parameters.
>>>>> These will be useful even when we go for pure generic MIPI design
>>>>
>>>> I feel like we have a different idea of what the ideal generic design
>>>> is. For me, the goal is that we change the struct intel_dsi_device to
>>>> struct drm_bridge, and those drm_bridge drivers are all about the panel,
>>>> and have as few details about i915 or our hardware as possible. Having
>>>> the bridge drivers fill in register values to be written by the core DSI
>>>> code does not fit that. Yes, I had some of those in my bridge conversion
>>>> patches too, but I did not intend we'd keep adding more.
>>>>
>>>> I'd rather we provide generic helpers the bridge driver can call.
>>>
>>> Yeah, look like our ideas are different. In your goal with drm_bridge
>>> architecture, we will still end up having multiple bridge drivers for
>>> each different panel. But my goal is to have a single driver which can
>>> work for multiple panels.
>>
>> I'm trying to look one or two steps further, and what it will mean to
>> the driver. Here's the long term goal in upstream as I see it: There
>> will be a framework in place that allows one to write a (DSI) panel
>> driver once, using generic APIs, and use that panel driver with any SoC
>> (that implements the other side of the framework).
>
> To clarify in terms of what we have currently, in my opinion intel_dsi.c 
> is one such SoC specific implementation for BYT and associated MIPI host 
> controller. And sub-encoder drivers are other end and I want to unify 
> sub-encoder side to have just one sub-encoder for any panel out there. 
> What you are talking makes perfect sense if we are going to have each 
> panel driver as a separate driver. But we can still achieve this 
> separation with common panel driver as well.
>
>>
>> We are obviously far away from that goal at the moment. But IMHO we
>> should keep that in mind as a guide to what we are doing now. Moving
>> towards a model with a clearly defined API between the DSI core and the
>> panels, where the panel specific things are abstracted away from the
>> core, or towards a model where the core and panel driver depend on the
>> implementation of each other, communicating via variables.
>
> I think we are aligned on the goal and I feel there is a need for common 
> DSI core to be separate
>
>>
>>> Since we already have enabled some panels with sub-encoder
>>> architecture for completeness I was planning to maintain generic
>>> driver as one sub-encoder.
>>
>> Nothing prevents you from doing that, as long as the separation between
>> the core and the panel drivers remains clear.
>>
>>> But actually we can do away with all sub-encoder and do not need even
>>> drm_bridge and all implementation will be in intel_dsi.c. Panel
>>> specific configurations or sequences will come from VBT which I have
>>> tried to convert as parameters.
>>
>> With this model it is all too easy to forget what is the panel driver
>> and what is the SoC driver. They *are* two separate things, and should
>> not be mixed. It will be all too easy to keep adding new parameters and
>> conditions in the code as new panel drivers appear to need them. It will
>> lead to code that is very difficult to understand and maintain.
>
> I think here you have misunderstood my proposal. I still treat SoC 
> driver and actual Panel driver as separate. And whatever parameters I 
> have tried to add are all DSI/DPHY spec related. There is not even 
> single parameter which is panel specific. If you are confusing this 
> because of use of -
>
> 	if (intel_dsi->dsi_clock_freq)
> 		dsi_clk = intel_dsi->dsi_clock_freq;
>
> I feel it is okay to have this parameter which provides provision for 
> spec parameters to be hard-coded instead of calculating if a panel needs 
> and this is *only* such parameter and I already have ways to remove it 
> as well. IMHO it is a small price to pay to get one generic panel driver.
>
> That is another thing if the DSI/DPHy specs themselves evolve and we 
> need to modify our core, but that is unavoidable I guess.
>
>>
>> A model similar to what I'm proposing has also been tried and tested,
>> with several panels: drivers/video/omap2/. It's not DRM, and the control
>> is in the panel drivers, but the separation is extremely clear (panel
>> drivers are separate kernel modules).
>
> Understand, but again my idea is to have one single driver which can 
> work for all panels and hence there is no need of multiple panel 
> drivers. This works with SoC side of the framework. I have not yet 
> described how we can achieve this, but first does it has any merit to 
> have something like this ?

My educated guess is that a single panel driver is not going to work for
*all* panels, *but* it is of course benefitial to share each panel
driver for as many panels as technically makes sense.

So yes, I agree there's merit in having a panel driver that works on a
bunch of panels based on parameters from the VBT. Apologies if it
sounded like I'm against that.

Based on my (limited, but not insignificant) experience with DSI panels,
you can't parametrize everything, and you will need panel specific code
to handle them. I presume there will also be cases where we won't have
the parameters in VBT. The DSI specs do not cover everything, and some
panels are outright out of spec. And then there are DSI->LVDS bridges
etc. needing special attention.

So I think let's keep trying to find the right abstractions to separate
the DSI core and the panel drivers, make it possible to support several
panels with one driver, and make it possible to have independent drivers
for panels that don't fit the assumptions of the generic panel driver.

Does that conflict with your goals? Are we in agreement here?

>> No doubt the clear separation between the core and the panel drivers
>> will be harder and more work in the short term, but it will pay off in
>> the long term. And it doesn't all have to happen at once, as long as we
>> work *towards* that goal, not away from it.
>
> I think we should take into account the amount of effort required to 
> develop and maintain bridge drivers for tens of MIPI panel out there Vs 
> having one panel driver to maintain and make fully spec compliant taking 
> care of open ends left by the specs in the best way we can to achieve 
> this generality.

See above. I'm not proposing you split it all out to separate
drivers. Again, I'm sorry if I haven't been clear about that.


Now, the question is how do we achieve all this. First things first,
let's try to get any straightforward changes merged (small patches!), so
we can at least narrow the gap to upstream. And I need to have a look at
your panel driver code.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/4] drm/i915: Add more dev ops for MIPI sub encoder

[Shobhit Kumar]

On 10/23/2013 07:52 PM, Jani Nikula wrote:
> On Wed, 23 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> Hi Jani,
>> On 10/22/2013 05:23 PM, Jani Nikula wrote:
>>> On Tue, 22 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>>>> On 10/21/2013 6:57 PM, Jani Nikula wrote:
>>>>>
>>>>> Hi Shobhit -
>>>>>
>>>>> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>>>>>> Also add new fields in intel_dsi to have all dphy related parameters.
>>>>>> These will be useful even when we go for pure generic MIPI design
>>>>>
>>>>> I feel like we have a different idea of what the ideal generic design
>>>>> is. For me, the goal is that we change the struct intel_dsi_device to
>>>>> struct drm_bridge, and those drm_bridge drivers are all about the panel,
>>>>> and have as few details about i915 or our hardware as possible. Having
>>>>> the bridge drivers fill in register values to be written by the core DSI
>>>>> code does not fit that. Yes, I had some of those in my bridge conversion
>>>>> patches too, but I did not intend we'd keep adding more.
>>>>>
>>>>> I'd rather we provide generic helpers the bridge driver can call.
>>>>
>>>> Yeah, look like our ideas are different. In your goal with drm_bridge
>>>> architecture, we will still end up having multiple bridge drivers for
>>>> each different panel. But my goal is to have a single driver which can
>>>> work for multiple panels.
>>>
>>> I'm trying to look one or two steps further, and what it will mean to
>>> the driver. Here's the long term goal in upstream as I see it: There
>>> will be a framework in place that allows one to write a (DSI) panel
>>> driver once, using generic APIs, and use that panel driver with any SoC
>>> (that implements the other side of the framework).
>>
>> To clarify in terms of what we have currently, in my opinion intel_dsi.c
>> is one such SoC specific implementation for BYT and associated MIPI host
>> controller. And sub-encoder drivers are other end and I want to unify
>> sub-encoder side to have just one sub-encoder for any panel out there.
>> What you are talking makes perfect sense if we are going to have each
>> panel driver as a separate driver. But we can still achieve this
>> separation with common panel driver as well.
>>
>>>
>>> We are obviously far away from that goal at the moment. But IMHO we
>>> should keep that in mind as a guide to what we are doing now. Moving
>>> towards a model with a clearly defined API between the DSI core and the
>>> panels, where the panel specific things are abstracted away from the
>>> core, or towards a model where the core and panel driver depend on the
>>> implementation of each other, communicating via variables.
>>
>> I think we are aligned on the goal and I feel there is a need for common
>> DSI core to be separate
>>
>>>
>>>> Since we already have enabled some panels with sub-encoder
>>>> architecture for completeness I was planning to maintain generic
>>>> driver as one sub-encoder.
>>>
>>> Nothing prevents you from doing that, as long as the separation between
>>> the core and the panel drivers remains clear.
>>>
>>>> But actually we can do away with all sub-encoder and do not need even
>>>> drm_bridge and all implementation will be in intel_dsi.c. Panel
>>>> specific configurations or sequences will come from VBT which I have
>>>> tried to convert as parameters.
>>>
>>> With this model it is all too easy to forget what is the panel driver
>>> and what is the SoC driver. They *are* two separate things, and should
>>> not be mixed. It will be all too easy to keep adding new parameters and
>>> conditions in the code as new panel drivers appear to need them. It will
>>> lead to code that is very difficult to understand and maintain.
>>
>> I think here you have misunderstood my proposal. I still treat SoC
>> driver and actual Panel driver as separate. And whatever parameters I
>> have tried to add are all DSI/DPHY spec related. There is not even
>> single parameter which is panel specific. If you are confusing this
>> because of use of -
>>
>> 	if (intel_dsi->dsi_clock_freq)
>> 		dsi_clk = intel_dsi->dsi_clock_freq;
>>
>> I feel it is okay to have this parameter which provides provision for
>> spec parameters to be hard-coded instead of calculating if a panel needs
>> and this is *only* such parameter and I already have ways to remove it
>> as well. IMHO it is a small price to pay to get one generic panel driver.
>>
>> That is another thing if the DSI/DPHy specs themselves evolve and we
>> need to modify our core, but that is unavoidable I guess.
>>
>>>
>>> A model similar to what I'm proposing has also been tried and tested,
>>> with several panels: drivers/video/omap2/. It's not DRM, and the control
>>> is in the panel drivers, but the separation is extremely clear (panel
>>> drivers are separate kernel modules).
>>
>> Understand, but again my idea is to have one single driver which can
>> work for all panels and hence there is no need of multiple panel
>> drivers. This works with SoC side of the framework. I have not yet
>> described how we can achieve this, but first does it has any merit to
>> have something like this ?
>
> My educated guess is that a single panel driver is not going to work for
> *all* panels, *but* it is of course benefitial to share each panel
> driver for as many panels as technically makes sense.
>
> So yes, I agree there's merit in having a panel driver that works on a
> bunch of panels based on parameters from the VBT. Apologies if it
> sounded like I'm against that.
>
> Based on my (limited, but not insignificant) experience with DSI panels,
> you can't parametrize everything, and you will need panel specific code
> to handle them. I presume there will also be cases where we won't have
> the parameters in VBT. The DSI specs do not cover everything, and some
> panels are outright out of spec. And then there are DSI->LVDS bridges
> etc. needing special attention.

In my understanding there are two things - one is configuration 
parameters and the sencond is panel specific sequneces for say 
enable/disable itself. To support both of them there is a design update 
in VBT as such. So far as per our experience till now on whatever panels 
we have enabled, we could cover them with common driver + VBT. But you 
are right there might be some really quirky panels which might fall out 
of this driver and that is why I am all for having sub-encoder design as 
is today and bridge driver in near future and want to ensure that the 
generic driver is as per the sub-encoder for now and drm_bridge soon.

Initially I was only aiming for native MIPI, but now I see some needs 
for DSI->LVDS bridge also. So this is a good point to consider as well.

>
> So I think let's keep trying to find the right abstractions to separate
> the DSI core and the panel drivers, make it possible to support several
> panels with one driver, and make it possible to have independent drivers
> for panels that don't fit the assumptions of the generic panel driver.
>
> Does that conflict with your goals? Are we in agreement here?

Definetely we are in agreement and perfectly aligns with my goals. But 
is it okay to work towards pushing sub-encoder based design for 
immidiate short term and then work to convert on drm_bridge because I 
can see that drm_bridge callbacks will need additions defintely and it 
might take some time to get that done. In the meantime can we push 
current driver with already suggested changes to get atleast a working 
base ?

>
>>> No doubt the clear separation between the core and the panel drivers
>>> will be harder and more work in the short term, but it will pay off in
>>> the long term. And it doesn't all have to happen at once, as long as we
>>> work *towards* that goal, not away from it.
>>
>> I think we should take into account the amount of effort required to
>> develop and maintain bridge drivers for tens of MIPI panel out there Vs
>> having one panel driver to maintain and make fully spec compliant taking
>> care of open ends left by the specs in the best way we can to achieve
>> this generality.
>
> See above. I'm not proposing you split it all out to separate
> drivers. Again, I'm sorry if I haven't been clear about that.
>
>
> Now, the question is how do we achieve all this. First things first,
> let's try to get any straightforward changes merged (small patches!), so
> we can at least narrow the gap to upstream. And I need to have a look at
> your panel driver code.

Sure. And as I propsed above we can push smaller cleaner patches for 
current design itself to get working base. Generic panel driver comes 
after that.

Regards
Shobhit

Re: [PATCH 1/4] drm/i915: Add more dev ops for MIPI sub encoder

[Jani Nikula]

On Thu, 24 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> On 10/23/2013 07:52 PM, Jani Nikula wrote:
>> So I think let's keep trying to find the right abstractions to separate
>> the DSI core and the panel drivers, make it possible to support several
>> panels with one driver, and make it possible to have independent drivers
>> for panels that don't fit the assumptions of the generic panel driver.
>>
>> Does that conflict with your goals? Are we in agreement here?
>
> Definetely we are in agreement and perfectly aligns with my goals.

That's relieving, I'm happy we're on the same page now. :)

> But is it okay to work towards pushing sub-encoder based design for
> immidiate short term and then work to convert on drm_bridge because I
> can see that drm_bridge callbacks will need additions defintely and it
> might take some time to get that done. In the meantime can we push
> current driver with already suggested changes to get atleast a working
> base ?

I'm okay with this. Daniel is pushing for drm_bridge, and I'm also
optimistic about that, but perhaps we have to see what we really need
first. The current sub-encoder model is more flexible for that in the
short term.

However I, and others, will need to know where we are heading, so please
do pay attention to splitting up the patches and explaining why they are
needed. Sometimes it's helpful to provide draft/RFC patches on top just
for that.

Finally, I am glad you're contributing directly to upstream now. It
makes a huge difference in the long run.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/4] drm/i915: Add more dev ops for MIPI sub encoder

[Shobhit Kumar]

On 10/24/2013 01:54 PM, Jani Nikula wrote:
> On Thu, 24 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> On 10/23/2013 07:52 PM, Jani Nikula wrote:
>>> So I think let's keep trying to find the right abstractions to separate
>>> the DSI core and the panel drivers, make it possible to support several
>>> panels with one driver, and make it possible to have independent drivers
>>> for panels that don't fit the assumptions of the generic panel driver.
>>>
>>> Does that conflict with your goals? Are we in agreement here?
>>
>> Definetely we are in agreement and perfectly aligns with my goals.
>
> That's relieving, I'm happy we're on the same page now. :)

Me too :)

>
>> But is it okay to work towards pushing sub-encoder based design for
>> immidiate short term and then work to convert on drm_bridge because I
>> can see that drm_bridge callbacks will need additions defintely and it
>> might take some time to get that done. In the meantime can we push
>> current driver with already suggested changes to get atleast a working
>> base ?
>
> I'm okay with this. Daniel is pushing for drm_bridge, and I'm also
> optimistic about that, but perhaps we have to see what we really need
> first. The current sub-encoder model is more flexible for that in the
> short term.
>
> However I, and others, will need to know where we are heading, so please
> do pay attention to splitting up the patches and explaining why they are
> needed. Sometimes it's helpful to provide draft/RFC patches on top just
> for that.

I will push updated smaller patches with clear reasoning for them.

>
> Finally, I am glad you're contributing directly to upstream now. It
> makes a huge difference in the long run.
>

Yeah, I wanted to do earlier but sometimes its all matter of bandwidth 
and priorities. Hopefully I will be able to continue to do so, now that 
I have started.

Thanks for all your inputs.

Regards
Shobhit

[PATCH 2/4] drm/i915: Use FLISDSI interface for band gap reset

[Shobhit Kumar]

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |    2 ++
 drivers/gpu/drm/i915/i915_reg.h       |    1 +
 drivers/gpu/drm/i915/intel_dsi.c      |   47 ++++++---------------------------
 drivers/gpu/drm/i915/intel_sideband.c |   14 ++++++++++
 4 files changed, 25 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index faf4dc1..1c42bb4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2334,6 +2334,8 @@ u32 intel_sbi_read(struct drm_i915_private *dev_priv, u16 reg,
 		   enum intel_sbi_destination destination);
 void intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value,
 		     enum intel_sbi_destination destination);
+u32 vlv_flisdsi_read(struct drm_i915_private *dev_priv, u32 reg);
+void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
 
 int vlv_gpu_freq(int ddr_freq, int val);
 int vlv_freq_opcode(int ddr_freq, int val);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 852d3c4..172f490 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -355,6 +355,7 @@
 #define   IOSF_PORT_CCK				0x14
 #define   IOSF_PORT_CCU				0xA9
 #define   IOSF_PORT_GPS_CORE			0x48
+#define   IOSF_PORT_FLISDSI			0x1B
 #define VLV_IOSF_DATA				(VLV_DISPLAY_BASE + 0x2104)
 #define VLV_IOSF_ADDR				(VLV_DISPLAY_BASE + 0x2108)
 
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 34e19b7..5a9dbfd 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -37,49 +37,18 @@
 static const struct intel_dsi_device intel_dsi_devices[] = {
 };
 
-
-static void vlv_cck_modify(struct drm_i915_private *dev_priv, u32 reg, u32 val,
-			   u32 mask)
-{
-	u32 tmp = vlv_cck_read(dev_priv, reg);
-	tmp &= ~mask;
-	tmp |= val;
-	vlv_cck_write(dev_priv, reg, tmp);
-}
-
-static void band_gap_wa(struct drm_i915_private *dev_priv)
+static void band_gap_reset(struct drm_i915_private *dev_priv)
 {
 	mutex_lock(&dev_priv->dpio_lock);
 
-	/* Enable bandgap fix in GOP driver */
-	vlv_cck_modify(dev_priv, 0x6D, 0x00010000, 0x00030000);
-	msleep(20);
-	vlv_cck_modify(dev_priv, 0x6E, 0x00010000, 0x00030000);
-	msleep(20);
-	vlv_cck_modify(dev_priv, 0x6F, 0x00010000, 0x00030000);
-	msleep(20);
-	vlv_cck_modify(dev_priv, 0x00, 0x00008000, 0x00008000);
-	msleep(20);
-	vlv_cck_modify(dev_priv, 0x00, 0x00000000, 0x00008000);
-	msleep(20);
-
-	/* Turn Display Trunk on */
-	vlv_cck_modify(dev_priv, 0x6B, 0x00020000, 0x00030000);
-	msleep(20);
-
-	vlv_cck_modify(dev_priv, 0x6C, 0x00020000, 0x00030000);
-	msleep(20);
-
-	vlv_cck_modify(dev_priv, 0x6D, 0x00020000, 0x00030000);
-	msleep(20);
-	vlv_cck_modify(dev_priv, 0x6E, 0x00020000, 0x00030000);
-	msleep(20);
-	vlv_cck_modify(dev_priv, 0x6F, 0x00020000, 0x00030000);
+	vlv_flisdsi_write(dev_priv, 0x08, 0x0001);
+	vlv_flisdsi_write(dev_priv, 0x0F, 0x0005);
+	vlv_flisdsi_write(dev_priv, 0x0F, 0x0025);
+	udelay(150);
+	vlv_flisdsi_write(dev_priv, 0x0F, 0x0000);
+	vlv_flisdsi_write(dev_priv, 0x08, 0x0000);
 
 	mutex_unlock(&dev_priv->dpio_lock);
-
-	/* Need huge delay, otherwise clock is not stable */
-	msleep(100);
 }
 
 static struct intel_dsi *intel_attached_dsi(struct drm_connector *connector)
@@ -363,7 +332,7 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
 	vlv_enable_dsi_pll(intel_encoder);
 
 	/* XXX: Location of the call */
-	band_gap_wa(dev_priv);
+	band_gap_reset(dev_priv);
 
 	/* escape clock divider, 20MHz, shared for A and C. device ready must be
 	 * off when doing this! txclkesc? */
diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
index acd1cfe..e3f5210 100644
--- a/drivers/gpu/drm/i915/intel_sideband.c
+++ b/drivers/gpu/drm/i915/intel_sideband.c
@@ -239,3 +239,17 @@ void intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value,
 		return;
 	}
 }
+
+u32 vlv_flisdsi_read(struct drm_i915_private *dev_priv, u32 reg)
+{
+	u32 val = 0;
+	vlv_sideband_rw(dev_priv, DPIO_DEVFN, IOSF_PORT_FLISDSI,
+					DPIO_OPCODE_REG_READ, reg, &val);
+	return val;
+}
+
+void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
+{
+	vlv_sideband_rw(dev_priv, DPIO_DEVFN, IOSF_PORT_FLISDSI,
+					DPIO_OPCODE_REG_WRITE, reg, &val);
+}
-- 
1.7.9.5

Re: [PATCH 2/4] drm/i915: Use FLISDSI interface for band gap reset

[Jani Nikula]

Hmm, I don't think I have the spec for this one. So did not check the
values, but overall the code looks good.

With the above limitation,
Reviewed-by: Jani Nikula <jani.nikula@intel.com>


On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |    2 ++
>  drivers/gpu/drm/i915/i915_reg.h       |    1 +
>  drivers/gpu/drm/i915/intel_dsi.c      |   47 ++++++---------------------------
>  drivers/gpu/drm/i915/intel_sideband.c |   14 ++++++++++
>  4 files changed, 25 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index faf4dc1..1c42bb4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2334,6 +2334,8 @@ u32 intel_sbi_read(struct drm_i915_private *dev_priv, u16 reg,
>  		   enum intel_sbi_destination destination);
>  void intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value,
>  		     enum intel_sbi_destination destination);
> +u32 vlv_flisdsi_read(struct drm_i915_private *dev_priv, u32 reg);
> +void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
>  
>  int vlv_gpu_freq(int ddr_freq, int val);
>  int vlv_freq_opcode(int ddr_freq, int val);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 852d3c4..172f490 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -355,6 +355,7 @@
>  #define   IOSF_PORT_CCK				0x14
>  #define   IOSF_PORT_CCU				0xA9
>  #define   IOSF_PORT_GPS_CORE			0x48
> +#define   IOSF_PORT_FLISDSI			0x1B
>  #define VLV_IOSF_DATA				(VLV_DISPLAY_BASE + 0x2104)
>  #define VLV_IOSF_ADDR				(VLV_DISPLAY_BASE + 0x2108)
>  
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 34e19b7..5a9dbfd 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -37,49 +37,18 @@
>  static const struct intel_dsi_device intel_dsi_devices[] = {
>  };
>  
> -
> -static void vlv_cck_modify(struct drm_i915_private *dev_priv, u32 reg, u32 val,
> -			   u32 mask)
> -{
> -	u32 tmp = vlv_cck_read(dev_priv, reg);
> -	tmp &= ~mask;
> -	tmp |= val;
> -	vlv_cck_write(dev_priv, reg, tmp);
> -}
> -
> -static void band_gap_wa(struct drm_i915_private *dev_priv)
> +static void band_gap_reset(struct drm_i915_private *dev_priv)
>  {
>  	mutex_lock(&dev_priv->dpio_lock);
>  
> -	/* Enable bandgap fix in GOP driver */
> -	vlv_cck_modify(dev_priv, 0x6D, 0x00010000, 0x00030000);
> -	msleep(20);
> -	vlv_cck_modify(dev_priv, 0x6E, 0x00010000, 0x00030000);
> -	msleep(20);
> -	vlv_cck_modify(dev_priv, 0x6F, 0x00010000, 0x00030000);
> -	msleep(20);
> -	vlv_cck_modify(dev_priv, 0x00, 0x00008000, 0x00008000);
> -	msleep(20);
> -	vlv_cck_modify(dev_priv, 0x00, 0x00000000, 0x00008000);
> -	msleep(20);
> -
> -	/* Turn Display Trunk on */
> -	vlv_cck_modify(dev_priv, 0x6B, 0x00020000, 0x00030000);
> -	msleep(20);
> -
> -	vlv_cck_modify(dev_priv, 0x6C, 0x00020000, 0x00030000);
> -	msleep(20);
> -
> -	vlv_cck_modify(dev_priv, 0x6D, 0x00020000, 0x00030000);
> -	msleep(20);
> -	vlv_cck_modify(dev_priv, 0x6E, 0x00020000, 0x00030000);
> -	msleep(20);
> -	vlv_cck_modify(dev_priv, 0x6F, 0x00020000, 0x00030000);
> +	vlv_flisdsi_write(dev_priv, 0x08, 0x0001);
> +	vlv_flisdsi_write(dev_priv, 0x0F, 0x0005);
> +	vlv_flisdsi_write(dev_priv, 0x0F, 0x0025);
> +	udelay(150);
> +	vlv_flisdsi_write(dev_priv, 0x0F, 0x0000);
> +	vlv_flisdsi_write(dev_priv, 0x08, 0x0000);
>  
>  	mutex_unlock(&dev_priv->dpio_lock);
> -
> -	/* Need huge delay, otherwise clock is not stable */
> -	msleep(100);
>  }
>  
>  static struct intel_dsi *intel_attached_dsi(struct drm_connector *connector)
> @@ -363,7 +332,7 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>  	vlv_enable_dsi_pll(intel_encoder);
>  
>  	/* XXX: Location of the call */
> -	band_gap_wa(dev_priv);
> +	band_gap_reset(dev_priv);
>  
>  	/* escape clock divider, 20MHz, shared for A and C. device ready must be
>  	 * off when doing this! txclkesc? */
> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> index acd1cfe..e3f5210 100644
> --- a/drivers/gpu/drm/i915/intel_sideband.c
> +++ b/drivers/gpu/drm/i915/intel_sideband.c
> @@ -239,3 +239,17 @@ void intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value,
>  		return;
>  	}
>  }
> +
> +u32 vlv_flisdsi_read(struct drm_i915_private *dev_priv, u32 reg)
> +{
> +	u32 val = 0;
> +	vlv_sideband_rw(dev_priv, DPIO_DEVFN, IOSF_PORT_FLISDSI,
> +					DPIO_OPCODE_REG_READ, reg, &val);
> +	return val;
> +}
> +
> +void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
> +{
> +	vlv_sideband_rw(dev_priv, DPIO_DEVFN, IOSF_PORT_FLISDSI,
> +					DPIO_OPCODE_REG_WRITE, reg, &val);
> +}
> -- 
> 1.7.9.5
>

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH 3/4] drm/i915: Compute dsi_clk from pixel clock

[Shobhit Kumar]

Minor modification to m_n_p calculations as well

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_pll.c |   75 ++++++++++++++++++++++++++++------
 1 file changed, 63 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
index 44279b2..bf12335 100644
--- a/drivers/gpu/drm/i915/intel_dsi_pll.c
+++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
@@ -50,6 +50,8 @@ static const u32 lfsr_converts[] = {
 	71, 35							/* 91 - 92 */
 };
 
+#ifdef DSI_CLK_FROM_RR
+
 static u32 dsi_rr_formula(const struct drm_display_mode *mode,
 			  int pixel_format, int video_mode_format,
 			  int lane_count, bool eotp)
@@ -129,6 +131,40 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
 	return dsi_clk;
 }
 
+#else
+
+/* Get DSI clock from pixel clock */
+static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
+			  int pixel_format, int lane_count)
+{
+	u32 dsi_bit_clock_hz, dsi_clk;
+	u32 bpp;
+
+	switch (pixel_format) {
+	default:
+	case VID_MODE_FORMAT_RGB888:
+	case VID_MODE_FORMAT_RGB666_LOOSE:
+		bpp = 24;
+		break;
+	case VID_MODE_FORMAT_RGB666:
+		bpp = 18;
+		break;
+	case VID_MODE_FORMAT_RGB565:
+		bpp = 16;
+		break;
+	}
+
+	/* DSI data rate = pixel clock * bits per pixel / lane count
+	   pixel clock is converted from KHz to Hz */
+	dsi_bit_clock_hz = (((mode->clock * 1000) * bpp) / lane_count);
+
+	/* DSI clock rate */
+	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
+	return dsi_clk;
+}
+
+#endif
+
 #ifdef MNP_FROM_TABLE
 
 struct dsi_clock_table {
@@ -208,29 +244,42 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
 	ref_clk = 25000;
 	target_dsi_clk = dsi_clk * 1000;
 	error = 0xFFFFFFFF;
+	tmp_error = 0xFFFFFFFF;
 	calc_m = 0;
 	calc_p = 0;
 
 	for (m = 62; m <= 92; m++) {
 		for (p = 2; p <= 6; p++) {
-
+			/* Find the optimal m and p divisors
+			with minimal error +/- the required clock */
 			calc_dsi_clk = (m * ref_clk) / p;
-			if (calc_dsi_clk >= target_dsi_clk) {
+			if (calc_dsi_clk == target_dsi_clk) {
+				calc_m = m;
+				calc_p = p;
+				error = 0;
+				break;
+			} else if (calc_dsi_clk > target_dsi_clk)
 				tmp_error = calc_dsi_clk - target_dsi_clk;
-				if (tmp_error < error) {
-					error = tmp_error;
-					calc_m = m;
-					calc_p = p;
-				}
+			else
+				tmp_error = target_dsi_clk - calc_dsi_clk;
+
+			if (tmp_error < error) {
+				error = tmp_error;
+				calc_m = m;
+				calc_p = p;
 			}
 		}
+
+		if (error == 0)
+			break;
 	}
 
 	m_seed = lfsr_converts[calc_m - 62];
 	n = 1;
+
 	dsi_mnp->dsi_pll_ctrl = 1 << (DSI_PLL_P1_POST_DIV_SHIFT + calc_p - 2);
 	dsi_mnp->dsi_pll_div = (n - 1) << DSI_PLL_N1_DIV_SHIFT |
-		m_seed << DSI_PLL_M1_DIV_SHIFT;
+					m_seed << DSI_PLL_M1_DIV_SHIFT;
 
 	return 0;
 }
@@ -249,11 +298,13 @@ static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
 	int ret;
 	struct dsi_mnp dsi_mnp;
-	u32 dsi_clk;
+	u32 dsi_clk = 0;
 
-	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
-				 intel_dsi->video_mode_format,
-				 intel_dsi->lane_count, !intel_dsi->eot_disable);
+	if (intel_dsi->dsi_clock_freq)
+		dsi_clk = intel_dsi->dsi_clock_freq;
+	else
+		dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
+							intel_dsi->lane_count);
 
 	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
 	if (ret) {
-- 
1.7.9.5

Re: [PATCH 3/4] drm/i915: Compute dsi_clk from pixel clock

[Ville Syrjälä]

On Mon, Oct 21, 2013 at 05:51:06PM +0530, Shobhit Kumar wrote:
> Minor modification to m_n_p calculations as well
> 
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_pll.c |   75 ++++++++++++++++++++++++++++------
>  1 file changed, 63 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
> index 44279b2..bf12335 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
> @@ -50,6 +50,8 @@ static const u32 lfsr_converts[] = {
>  	71, 35							/* 91 - 92 */
>  };
>  
> +#ifdef DSI_CLK_FROM_RR
> +
>  static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>  			  int pixel_format, int video_mode_format,
>  			  int lane_count, bool eotp)
> @@ -129,6 +131,40 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>  	return dsi_clk;
>  }
>  
> +#else
> +
> +/* Get DSI clock from pixel clock */
> +static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
> +			  int pixel_format, int lane_count)
> +{
> +	u32 dsi_bit_clock_hz, dsi_clk;
> +	u32 bpp;
> +
> +	switch (pixel_format) {
> +	default:
> +	case VID_MODE_FORMAT_RGB888:
> +	case VID_MODE_FORMAT_RGB666_LOOSE:
> +		bpp = 24;
> +		break;
> +	case VID_MODE_FORMAT_RGB666:
> +		bpp = 18;
> +		break;
> +	case VID_MODE_FORMAT_RGB565:
> +		bpp = 16;
> +		break;
> +	}
> +
> +	/* DSI data rate = pixel clock * bits per pixel / lane count
> +	   pixel clock is converted from KHz to Hz */
> +	dsi_bit_clock_hz = (((mode->clock * 1000) * bpp) / lane_count);
> +
> +	/* DSI clock rate */
> +	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
> +	return dsi_clk;

And why is this better than the rr_formula thing that tries to
account for the packetization overhead?

Also I don't understand why you go from kHz to Hz and then to MHz.
I'd just do something like:
return DIV_ROUND_CLOSEST(mode->clock * bpp, lane_count);
and then change the rest of the code to work in kHz as well.

> +}
> +
> +#endif
> +
>  #ifdef MNP_FROM_TABLE
>  
>  struct dsi_clock_table {
> @@ -208,29 +244,42 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
>  	ref_clk = 25000;
>  	target_dsi_clk = dsi_clk * 1000;
>  	error = 0xFFFFFFFF;
> +	tmp_error = 0xFFFFFFFF;
>  	calc_m = 0;
>  	calc_p = 0;
>  
>  	for (m = 62; m <= 92; m++) {
>  		for (p = 2; p <= 6; p++) {
> -
> +			/* Find the optimal m and p divisors
> +			with minimal error +/- the required clock */
>  			calc_dsi_clk = (m * ref_clk) / p;
> -			if (calc_dsi_clk >= target_dsi_clk) {
> +			if (calc_dsi_clk == target_dsi_clk) {
> +				calc_m = m;
> +				calc_p = p;
> +				error = 0;
> +				break;
> +			} else if (calc_dsi_clk > target_dsi_clk)
>  				tmp_error = calc_dsi_clk - target_dsi_clk;
> -				if (tmp_error < error) {
> -					error = tmp_error;
> -					calc_m = m;
> -					calc_p = p;
> -				}
> +			else
> +				tmp_error = target_dsi_clk - calc_dsi_clk;
> +
> +			if (tmp_error < error) {
> +				error = tmp_error;
> +				calc_m = m;
> +				calc_p = p;
>  			}
>  		}
> +
> +		if (error == 0)
> +			break;
>  	}
>  
>  	m_seed = lfsr_converts[calc_m - 62];
>  	n = 1;
> +
>  	dsi_mnp->dsi_pll_ctrl = 1 << (DSI_PLL_P1_POST_DIV_SHIFT + calc_p - 2);
>  	dsi_mnp->dsi_pll_div = (n - 1) << DSI_PLL_N1_DIV_SHIFT |
> -		m_seed << DSI_PLL_M1_DIV_SHIFT;
> +					m_seed << DSI_PLL_M1_DIV_SHIFT;
>  
>  	return 0;
>  }
> @@ -249,11 +298,13 @@ static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	int ret;
>  	struct dsi_mnp dsi_mnp;
> -	u32 dsi_clk;
> +	u32 dsi_clk = 0;
>  
> -	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
> -				 intel_dsi->video_mode_format,
> -				 intel_dsi->lane_count, !intel_dsi->eot_disable);
> +	if (intel_dsi->dsi_clock_freq)
> +		dsi_clk = intel_dsi->dsi_clock_freq;
> +	else
> +		dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
> +							intel_dsi->lane_count);
>  
>  	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
>  	if (ret) {
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/4] drm/i915: Compute dsi_clk from pixel clock

[Shobhit Kumar]

On 10/21/2013 6:58 PM, Ville Syrjälä wrote:
> On Mon, Oct 21, 2013 at 05:51:06PM +0530, Shobhit Kumar wrote:
>> Minor modification to m_n_p calculations as well
>>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dsi_pll.c |   75 ++++++++++++++++++++++++++++------
>>   1 file changed, 63 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
>> index 44279b2..bf12335 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
>> @@ -50,6 +50,8 @@ static const u32 lfsr_converts[] = {
>>   	71, 35							/* 91 - 92 */
>>   };
>>
>> +#ifdef DSI_CLK_FROM_RR
>> +
>>   static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>>   			  int pixel_format, int video_mode_format,
>>   			  int lane_count, bool eotp)
>> @@ -129,6 +131,40 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>>   	return dsi_clk;
>>   }
>>
>> +#else
>> +
>> +/* Get DSI clock from pixel clock */
>> +static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
>> +			  int pixel_format, int lane_count)
>> +{
>> +	u32 dsi_bit_clock_hz, dsi_clk;
>> +	u32 bpp;
>> +
>> +	switch (pixel_format) {
>> +	default:
>> +	case VID_MODE_FORMAT_RGB888:
>> +	case VID_MODE_FORMAT_RGB666_LOOSE:
>> +		bpp = 24;
>> +		break;
>> +	case VID_MODE_FORMAT_RGB666:
>> +		bpp = 18;
>> +		break;
>> +	case VID_MODE_FORMAT_RGB565:
>> +		bpp = 16;
>> +		break;
>> +	}
>> +
>> +	/* DSI data rate = pixel clock * bits per pixel / lane count
>> +	   pixel clock is converted from KHz to Hz */
>> +	dsi_bit_clock_hz = (((mode->clock * 1000) * bpp) / lane_count);
>> +
>> +	/* DSI clock rate */
>> +	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
>> +	return dsi_clk;
>
> And why is this better than the rr_formula thing that tries to
> account for the packetization overhead?
>

Its been changed to pixel_clock based formula as MIPI host controller 
specification in our platform recommends this formula

> Also I don't understand why you go from kHz to Hz and then to MHz.
> I'd just do something like:
> return DIV_ROUND_CLOSEST(mode->clock * bpp, lane_count);
> and then change the rest of the code to work in kHz as well.

We wanted more accuracy, but on after thought anyway EDID spec specifies 
pixel_clock in 10 KHz and we already lost the precision, so it does not 
make sense to convert in to Hz and then revert to MHz. Will make the 
change accordingly.

>
>> +}
>> +
>> +#endif
>> +
>>   #ifdef MNP_FROM_TABLE
>>
>>   struct dsi_clock_table {
>> @@ -208,29 +244,42 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
>>   	ref_clk = 25000;
>>   	target_dsi_clk = dsi_clk * 1000;
>>   	error = 0xFFFFFFFF;
>> +	tmp_error = 0xFFFFFFFF;
>>   	calc_m = 0;
>>   	calc_p = 0;
>>
>>   	for (m = 62; m <= 92; m++) {
>>   		for (p = 2; p <= 6; p++) {
>> -
>> +			/* Find the optimal m and p divisors
>> +			with minimal error +/- the required clock */
>>   			calc_dsi_clk = (m * ref_clk) / p;
>> -			if (calc_dsi_clk >= target_dsi_clk) {
>> +			if (calc_dsi_clk == target_dsi_clk) {
>> +				calc_m = m;
>> +				calc_p = p;
>> +				error = 0;
>> +				break;
>> +			} else if (calc_dsi_clk > target_dsi_clk)
>>   				tmp_error = calc_dsi_clk - target_dsi_clk;
>> -				if (tmp_error < error) {
>> -					error = tmp_error;
>> -					calc_m = m;
>> -					calc_p = p;
>> -				}
>> +			else
>> +				tmp_error = target_dsi_clk - calc_dsi_clk;
>> +
>> +			if (tmp_error < error) {
>> +				error = tmp_error;
>> +				calc_m = m;
>> +				calc_p = p;
>>   			}
>>   		}
>> +
>> +		if (error == 0)
>> +			break;
>>   	}
>>
>>   	m_seed = lfsr_converts[calc_m - 62];
>>   	n = 1;
>> +
>>   	dsi_mnp->dsi_pll_ctrl = 1 << (DSI_PLL_P1_POST_DIV_SHIFT + calc_p - 2);
>>   	dsi_mnp->dsi_pll_div = (n - 1) << DSI_PLL_N1_DIV_SHIFT |
>> -		m_seed << DSI_PLL_M1_DIV_SHIFT;
>> +					m_seed << DSI_PLL_M1_DIV_SHIFT;
>>
>>   	return 0;
>>   }
>> @@ -249,11 +298,13 @@ static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
>>   	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>   	int ret;
>>   	struct dsi_mnp dsi_mnp;
>> -	u32 dsi_clk;
>> +	u32 dsi_clk = 0;
>>
>> -	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
>> -				 intel_dsi->video_mode_format,
>> -				 intel_dsi->lane_count, !intel_dsi->eot_disable);
>> +	if (intel_dsi->dsi_clock_freq)
>> +		dsi_clk = intel_dsi->dsi_clock_freq;
>> +	else
>> +		dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
>> +							intel_dsi->lane_count);
>>
>>   	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
>>   	if (ret) {
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Re: [PATCH 3/4] drm/i915: Compute dsi_clk from pixel clock

[Jani Nikula]

On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> Minor modification to m_n_p calculations as well

That should probably be a separate patch, unless it's a requirement for
what the main subject of this patch is. The commit message does not say.

> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_pll.c |   75 ++++++++++++++++++++++++++++------
>  1 file changed, 63 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
> index 44279b2..bf12335 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
> @@ -50,6 +50,8 @@ static const u32 lfsr_converts[] = {
>  	71, 35							/* 91 - 92 */
>  };
>  
> +#ifdef DSI_CLK_FROM_RR
> +
>  static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>  			  int pixel_format, int video_mode_format,
>  			  int lane_count, bool eotp)
> @@ -129,6 +131,40 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>  	return dsi_clk;
>  }
>  
> +#else
> +
> +/* Get DSI clock from pixel clock */
> +static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
> +			  int pixel_format, int lane_count)
> +{
> +	u32 dsi_bit_clock_hz, dsi_clk;
> +	u32 bpp;
> +
> +	switch (pixel_format) {
> +	default:
> +	case VID_MODE_FORMAT_RGB888:
> +	case VID_MODE_FORMAT_RGB666_LOOSE:
> +		bpp = 24;
> +		break;
> +	case VID_MODE_FORMAT_RGB666:
> +		bpp = 18;
> +		break;
> +	case VID_MODE_FORMAT_RGB565:
> +		bpp = 16;
> +		break;
> +	}
> +
> +	/* DSI data rate = pixel clock * bits per pixel / lane count
> +	   pixel clock is converted from KHz to Hz */
> +	dsi_bit_clock_hz = (((mode->clock * 1000) * bpp) / lane_count);
> +
> +	/* DSI clock rate */
> +	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
> +	return dsi_clk;
> +}
> +
> +#endif
> +
>  #ifdef MNP_FROM_TABLE
>  
>  struct dsi_clock_table {
> @@ -208,29 +244,42 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
>  	ref_clk = 25000;
>  	target_dsi_clk = dsi_clk * 1000;
>  	error = 0xFFFFFFFF;
> +	tmp_error = 0xFFFFFFFF;
>  	calc_m = 0;
>  	calc_p = 0;
>  
>  	for (m = 62; m <= 92; m++) {
>  		for (p = 2; p <= 6; p++) {
> -
> +			/* Find the optimal m and p divisors
> +			with minimal error +/- the required clock */
>  			calc_dsi_clk = (m * ref_clk) / p;
> -			if (calc_dsi_clk >= target_dsi_clk) {
> +			if (calc_dsi_clk == target_dsi_clk) {
> +				calc_m = m;
> +				calc_p = p;
> +				error = 0;
> +				break;
> +			} else if (calc_dsi_clk > target_dsi_clk)
>  				tmp_error = calc_dsi_clk - target_dsi_clk;
> -				if (tmp_error < error) {
> -					error = tmp_error;
> -					calc_m = m;
> -					calc_p = p;
> -				}
> +			else
> +				tmp_error = target_dsi_clk - calc_dsi_clk;
> +

Using abs() might clean this up a bit.

> +			if (tmp_error < error) {
> +				error = tmp_error;
> +				calc_m = m;
> +				calc_p = p;
>  			}
>  		}
> +
> +		if (error == 0)
> +			break;

The above changes should be a separate patch, with rationale in the
commit message.

>  	}
>  
>  	m_seed = lfsr_converts[calc_m - 62];
>  	n = 1;
> +
>  	dsi_mnp->dsi_pll_ctrl = 1 << (DSI_PLL_P1_POST_DIV_SHIFT + calc_p - 2);
>  	dsi_mnp->dsi_pll_div = (n - 1) << DSI_PLL_N1_DIV_SHIFT |
> -		m_seed << DSI_PLL_M1_DIV_SHIFT;
> +					m_seed << DSI_PLL_M1_DIV_SHIFT;

If really needed, style/whitespace changes should also be separated.

>  
>  	return 0;
>  }
> @@ -249,11 +298,13 @@ static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	int ret;
>  	struct dsi_mnp dsi_mnp;
> -	u32 dsi_clk;
> +	u32 dsi_clk = 0;
>  
> -	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
> -				 intel_dsi->video_mode_format,
> -				 intel_dsi->lane_count, !intel_dsi->eot_disable);
> +	if (intel_dsi->dsi_clock_freq)
> +		dsi_clk = intel_dsi->dsi_clock_freq;

This is the third major change in the patch. Should be separate, with
rationale, or possibly bundled with a different change which starts
using it. Who is responsible for setting and clearing this?

> +	else
> +		dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
> +							intel_dsi->lane_count);
>  
>  	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
>  	if (ret) {
> -- 
> 1.7.9.5
>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 3/4] drm/i915: Compute dsi_clk from pixel clock

[Shobhit Kumar]

On 10/21/2013 7:14 PM, Jani Nikula wrote:
> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> Minor modification to m_n_p calculations as well
>
> That should probably be a separate patch, unless it's a requirement for
> what the main subject of this patch is. The commit message does not say.

Will do the needful

>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dsi_pll.c |   75 ++++++++++++++++++++++++++++------
>>   1 file changed, 63 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
>> index 44279b2..bf12335 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
>> @@ -50,6 +50,8 @@ static const u32 lfsr_converts[] = {
>>   	71, 35							/* 91 - 92 */
>>   };
>>
>> +#ifdef DSI_CLK_FROM_RR
>> +
>>   static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>>   			  int pixel_format, int video_mode_format,
>>   			  int lane_count, bool eotp)
>> @@ -129,6 +131,40 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>>   	return dsi_clk;
>>   }
>>
>> +#else
>> +
>> +/* Get DSI clock from pixel clock */
>> +static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
>> +			  int pixel_format, int lane_count)
>> +{
>> +	u32 dsi_bit_clock_hz, dsi_clk;
>> +	u32 bpp;
>> +
>> +	switch (pixel_format) {
>> +	default:
>> +	case VID_MODE_FORMAT_RGB888:
>> +	case VID_MODE_FORMAT_RGB666_LOOSE:
>> +		bpp = 24;
>> +		break;
>> +	case VID_MODE_FORMAT_RGB666:
>> +		bpp = 18;
>> +		break;
>> +	case VID_MODE_FORMAT_RGB565:
>> +		bpp = 16;
>> +		break;
>> +	}
>> +
>> +	/* DSI data rate = pixel clock * bits per pixel / lane count
>> +	   pixel clock is converted from KHz to Hz */
>> +	dsi_bit_clock_hz = (((mode->clock * 1000) * bpp) / lane_count);
>> +
>> +	/* DSI clock rate */
>> +	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
>> +	return dsi_clk;
>> +}
>> +
>> +#endif
>> +
>>   #ifdef MNP_FROM_TABLE
>>
>>   struct dsi_clock_table {
>> @@ -208,29 +244,42 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
>>   	ref_clk = 25000;
>>   	target_dsi_clk = dsi_clk * 1000;
>>   	error = 0xFFFFFFFF;
>> +	tmp_error = 0xFFFFFFFF;
>>   	calc_m = 0;
>>   	calc_p = 0;
>>
>>   	for (m = 62; m <= 92; m++) {
>>   		for (p = 2; p <= 6; p++) {
>> -
>> +			/* Find the optimal m and p divisors
>> +			with minimal error +/- the required clock */
>>   			calc_dsi_clk = (m * ref_clk) / p;
>> -			if (calc_dsi_clk >= target_dsi_clk) {
>> +			if (calc_dsi_clk == target_dsi_clk) {
>> +				calc_m = m;
>> +				calc_p = p;
>> +				error = 0;
>> +				break;
>> +			} else if (calc_dsi_clk > target_dsi_clk)
>>   				tmp_error = calc_dsi_clk - target_dsi_clk;
>> -				if (tmp_error < error) {
>> -					error = tmp_error;
>> -					calc_m = m;
>> -					calc_p = p;
>> -				}
>> +			else
>> +				tmp_error = target_dsi_clk - calc_dsi_clk;
>> +
>
> Using abs() might clean this up a bit.
>
>> +			if (tmp_error < error) {
>> +				error = tmp_error;
>> +				calc_m = m;
>> +				calc_p = p;
>>   			}
>>   		}
>> +
>> +		if (error == 0)
>> +			break;
>
> The above changes should be a separate patch, with rationale in the
> commit message.

Will do the needful

>
>>   	}
>>
>>   	m_seed = lfsr_converts[calc_m - 62];
>>   	n = 1;
>> +
>>   	dsi_mnp->dsi_pll_ctrl = 1 << (DSI_PLL_P1_POST_DIV_SHIFT + calc_p - 2);
>>   	dsi_mnp->dsi_pll_div = (n - 1) << DSI_PLL_N1_DIV_SHIFT |
>> -		m_seed << DSI_PLL_M1_DIV_SHIFT;
>> +					m_seed << DSI_PLL_M1_DIV_SHIFT;
>
> If really needed, style/whitespace changes should also be separated.
>
>>
>>   	return 0;
>>   }
>> @@ -249,11 +298,13 @@ static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
>>   	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>   	int ret;
>>   	struct dsi_mnp dsi_mnp;
>> -	u32 dsi_clk;
>> +	u32 dsi_clk = 0;
>>
>> -	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
>> -				 intel_dsi->video_mode_format,
>> -				 intel_dsi->lane_count, !intel_dsi->eot_disable);
>> +	if (intel_dsi->dsi_clock_freq)
>> +		dsi_clk = intel_dsi->dsi_clock_freq;
>
> This is the third major change in the patch. Should be separate, with
> rationale, or possibly bundled with a different change which starts
> using it. Who is responsible for setting and clearing this?
>

Panel vendor recommended frequency value should override the formula 
based value and this parameter provides support for the same. We have 
come across panel where the vendor specified specific data rates.

>> +	else
>> +		dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
>> +							intel_dsi->lane_count);
>>
>>   	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
>>   	if (ret) {
>> --
>> 1.7.9.5
>>
>

Regards
Shobhit

[PATCH 4/4] drm/i915: Parameterize the MIPI enabling sequnece and adjust the sequence

[Shobhit Kumar]

Has been tested on couple of panels now.

Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
Signed-off-by: Vijaykumar Balakrishnan <vijayakumar.balakrishnan@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 |  334 +++++++++++++++++++++-----------------
 2 files changed, 199 insertions(+), 146 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1c42bb4..1c28d02 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2375,6 +2375,17 @@ __i915_write(64)
 #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)
+
+
 /* "Broadcast RGB" property */
 #define INTEL_BROADCAST_RGB_AUTO 0
 #define INTEL_BROADCAST_RGB_FULL 1
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 5a9dbfd..241bb55 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -21,6 +21,8 @@
  * DEALINGS IN THE SOFTWARE.
  *
  * Author: Jani Nikula <jani.nikula@intel.com>
+ *	 : Shobhit Kumar <shobhit.kumar@intel.com>
+ *	 : Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
  */
 
 #include <drm/drmP.h>
@@ -101,63 +103,80 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder)
 	vlv_enable_dsi_pll(encoder);
 }
 
-static void intel_dsi_pre_enable(struct intel_encoder *encoder)
-{
-	DRM_DEBUG_KMS("\n");
-}
-
-static void intel_dsi_enable(struct intel_encoder *encoder)
+void intel_dsi_device_ready(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
 	int pipe = intel_crtc->pipe;
-	u32 temp;
 
 	DRM_DEBUG_KMS("\n");
 
 	if (intel_dsi->dev.dev_ops->panel_reset)
 		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
 
-	temp = I915_READ(MIPI_DEVICE_READY(pipe));
-	if ((temp & DEVICE_READY) == 0) {
-		temp &= ~ULPS_STATE_MASK;
-		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | DEVICE_READY);
-	} else if (temp & ULPS_STATE_MASK) {
-		temp &= ~ULPS_STATE_MASK;
-		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | ULPS_STATE_EXIT);
-		/*
-		 * We need to ensure that there is a minimum of 1 ms time
-		 * available before clearing the UPLS exit state.
-		 */
-		msleep(2);
-		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
-	}
+	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), LP_OUTPUT_HOLD, LP_OUTPUT_HOLD);
+	usleep_range(1000, 1500);
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY |
+			ULPS_STATE_EXIT, DEVICE_READY | ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
+			DEVICE_READY | ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00,
+			DEVICE_READY | ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
+			DEVICE_READY | ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
 
 	if (intel_dsi->dev.dev_ops->send_otp_cmds)
 		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
 
+}
+static void intel_dsi_pre_enable(struct intel_encoder *encoder)
+{
+	DRM_DEBUG_KMS("\n");
+
+	/* put device in ready state */
+	intel_dsi_device_ready(encoder);
+}
+
+static void intel_dsi_enable(struct intel_encoder *encoder)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	int pipe = intel_crtc->pipe;
+	u32 temp;
+
+	DRM_DEBUG_KMS("\n");
+
 	if (is_cmd_mode(intel_dsi))
 		I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
-
-	if (is_vid_mode(intel_dsi)) {
+	else {
 		msleep(20); /* XXX */
 		dpi_send_cmd(intel_dsi, TURN_ON);
 		msleep(100);
 
 		/* assert ip_tg_enable signal */
 		temp = I915_READ(MIPI_PORT_CTRL(pipe));
+		temp = temp | intel_dsi->port_bits;
 		I915_WRITE(MIPI_PORT_CTRL(pipe), temp | DPI_ENABLE);
 		POSTING_READ(MIPI_PORT_CTRL(pipe));
 	}
 
 	if (intel_dsi->dev.dev_ops->enable)
 		intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
+
+	intel_panel_enable_backlight(dev, pipe);
 }
 
 static void intel_dsi_disable(struct intel_encoder *encoder)
 {
-	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
 	int pipe = intel_crtc->pipe;
@@ -165,11 +184,18 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
 
 	DRM_DEBUG_KMS("\n");
 
-	intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
+	intel_panel_disable_backlight(dev);
+	if (intel_dsi->backlight_off_delay >= 20)
+		msleep(intel_dsi->backlight_off_delay);
+	else
+		usleep_range(intel_dsi->backlight_off_delay * 1000,
+			(intel_dsi->backlight_off_delay * 1000) + 500);
 
 	if (is_vid_mode(intel_dsi)) {
-		dpi_send_cmd(intel_dsi, SHUTDOWN);
-		msleep(10);
+		if (intel_dsi->send_shutdown) {
+			dpi_send_cmd(intel_dsi, SHUTDOWN);
+			msleep(intel_dsi->shutdown_pkt_delay);
+		}
 
 		/* de-assert ip_tg_enable signal */
 		temp = I915_READ(MIPI_PORT_CTRL(pipe));
@@ -179,19 +205,52 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
 		msleep(2);
 	}
 
-	temp = I915_READ(MIPI_DEVICE_READY(pipe));
-	if (temp & DEVICE_READY) {
-		temp &= ~DEVICE_READY;
-		temp &= ~ULPS_STATE_MASK;
-		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
-	}
+	/* if disable packets are sent before sending shutdown packet then in
+	 * some next enable sequence send turn on packet error is observed */
+	if (intel_dsi->dev.dev_ops->disable)
+		intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
 }
 
-static void intel_dsi_post_disable(struct intel_encoder *encoder)
+void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
 {
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	int pipe = intel_crtc->pipe;
+
 	DRM_DEBUG_KMS("\n");
 
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
+							ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
+
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_EXIT,
+							ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
+
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
+							ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
+
+	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), 0, LP_OUTPUT_HOLD);
+	usleep_range(1000, 1500);
+
+	if (wait_for(((I915_READ(MIPI_PORT_CTRL(pipe)) & 0x20000)
+					== 0x00000), 30))
+		DRM_ERROR("DSI LP not going Low\n");
+
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, DEVICE_READY);
+	usleep_range(2000, 2500);
+
 	vlv_disable_dsi_pll(encoder);
+
+	if (intel_dsi->dev.dev_ops->disable_panel_power)
+		intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev);
+}
+static void intel_dsi_post_disable(struct intel_encoder *encoder)
+{
+	DRM_DEBUG_KMS("\n");
+	intel_dsi_clear_device_ready(encoder);
 }
 
 static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
@@ -251,20 +310,6 @@ static int intel_dsi_mode_valid(struct drm_connector *connector,
 	return intel_dsi->dev.dev_ops->mode_valid(&intel_dsi->dev, mode);
 }
 
-/* return txclkesc cycles in terms of divider and duration in us */
-static u16 txclkesc(u32 divider, unsigned int us)
-{
-	switch (divider) {
-	case ESCAPE_CLOCK_DIVIDER_1:
-	default:
-		return 20 * us;
-	case ESCAPE_CLOCK_DIVIDER_2:
-		return 10 * us;
-	case ESCAPE_CLOCK_DIVIDER_4:
-		return 5 * us;
-	}
-}
-
 /* return pixels in terms of txbyteclkhs */
 static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count)
 {
@@ -313,26 +358,16 @@ static void set_dsi_timings(struct drm_encoder *encoder,
 	I915_WRITE(MIPI_VBP_COUNT(pipe), vbp);
 }
 
-static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
+static void dsi_config(struct drm_encoder *encoder)
 {
-	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder);
-	struct drm_display_mode *adjusted_mode =
-		&intel_crtc->config.adjusted_mode;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
 	int pipe = intel_crtc->pipe;
-	unsigned int bpp = intel_crtc->config.pipe_bpp;
-	u32 val, tmp;
-
-	DRM_DEBUG_KMS("pipe %d\n", pipe);
+	u32 tmp;
 
-	/* Update the DSI PLL */
-	vlv_enable_dsi_pll(intel_encoder);
-
-	/* XXX: Location of the call */
-	band_gap_reset(dev_priv);
+	DRM_DEBUG_KMS("\n");
 
 	/* escape clock divider, 20MHz, shared for A and C. device ready must be
 	 * off when doing this! txclkesc? */
@@ -345,102 +380,108 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
 	tmp &= ~READ_REQUEST_PRIORITY_MASK;
 	I915_WRITE(MIPI_CTRL(pipe), tmp | READ_REQUEST_PRIORITY_HIGH);
 
-	/* XXX: why here, why like this? handling in irq handler?! */
-	I915_WRITE(MIPI_INTR_STAT(pipe), 0xffffffff);
 	I915_WRITE(MIPI_INTR_EN(pipe), 0xffffffff);
 
-	I915_WRITE(MIPI_DPHY_PARAM(pipe),
-		   0x3c << EXIT_ZERO_COUNT_SHIFT |
-		   0x1f << TRAIL_COUNT_SHIFT |
-		   0xc5 << CLK_ZERO_COUNT_SHIFT |
-		   0x1f << PREPARE_COUNT_SHIFT);
+	I915_WRITE(MIPI_DPHY_PARAM(pipe), intel_dsi->dphy_reg);
+}
 
-	I915_WRITE(MIPI_DPI_RESOLUTION(pipe),
-		   adjusted_mode->vdisplay << VERTICAL_ADDRESS_SHIFT |
-		   adjusted_mode->hdisplay << HORIZONTAL_ADDRESS_SHIFT);
+static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
+{
+	struct drm_encoder *encoder = &intel_encoder->base;
+	struct drm_device *dev = encoder->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
+	struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder);
+	struct drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
+	int pipe = intel_crtc->pipe;
+	unsigned int bpp = intel_crtc->config.pipe_bpp;
+	u32 val;
 
-	set_dsi_timings(encoder, adjusted_mode);
+	band_gap_reset(dev_priv);
 
-	val = intel_dsi->lane_count << DATA_LANES_PRG_REG_SHIFT;
-	if (is_cmd_mode(intel_dsi)) {
-		val |= intel_dsi->channel << CMD_MODE_CHANNEL_NUMBER_SHIFT;
-		val |= CMD_MODE_DATA_WIDTH_8_BIT; /* XXX */
-	} else {
-		val |= intel_dsi->channel << VID_MODE_CHANNEL_NUMBER_SHIFT;
+	dsi_config(encoder);
 
-		/* XXX: cross-check bpp vs. pixel format? */
-		val |= intel_dsi->pixel_format;
-	}
-	I915_WRITE(MIPI_DSI_FUNC_PRG(pipe), val);
-
-	/* timeouts for recovery. one frame IIUC. if counter expires, EOT and
-	 * stop state. */
-
-	/*
-	 * In burst mode, value greater than one DPI line Time in byte clock
-	 * (txbyteclkhs) To timeout this timer 1+ of the above said value is
-	 * recommended.
-	 *
-	 * In non-burst mode, Value greater than one DPI frame time in byte
-	 * clock(txbyteclkhs) To timeout this timer 1+ of the above said value
-	 * is recommended.
-	 *
-	 * In DBI only mode, value greater than one DBI frame time in byte
-	 * clock(txbyteclkhs) To timeout this timer 1+ of the above said value
-	 * is recommended.
-	 */
-
-	if (is_vid_mode(intel_dsi) &&
-	    intel_dsi->video_mode_format == VIDEO_MODE_BURST) {
-		I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
-			   txbyteclkhs(adjusted_mode->htotal, bpp,
-				       intel_dsi->lane_count) + 1);
-	} else {
-		I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
-			   txbyteclkhs(adjusted_mode->vtotal *
-				       adjusted_mode->htotal,
-				       bpp, intel_dsi->lane_count) + 1);
-	}
-	I915_WRITE(MIPI_LP_RX_TIMEOUT(pipe), 8309); /* max */
-	I915_WRITE(MIPI_TURN_AROUND_TIMEOUT(pipe), 0x14); /* max */
-	I915_WRITE(MIPI_DEVICE_RESET_TIMER(pipe), 0xffff); /* max */
+	I915_WRITE(MIPI_LP_RX_TIMEOUT(pipe), intel_dsi->lp_rx_timeout);
+	I915_WRITE(MIPI_TURN_AROUND_TIMEOUT(pipe),
+					intel_dsi->turn_arnd_val);
+	I915_WRITE(MIPI_DEVICE_RESET_TIMER(pipe),
+					intel_dsi->rst_timer_val);
+	/* in terms of low power clock */
+	I915_WRITE(MIPI_INIT_COUNT(pipe), intel_dsi->init_count);
 
-	/* dphy stuff */
+	if (intel_dsi->eot_disable)
+		I915_WRITE(MIPI_EOT_DISABLE(pipe), 0);
+	else
+		I915_WRITE(MIPI_EOT_DISABLE(pipe), 1);
 
-	/* in terms of low power clock */
-	I915_WRITE(MIPI_INIT_COUNT(pipe), txclkesc(ESCAPE_CLOCK_DIVIDER_1, 100));
-
-	/* recovery disables */
-	I915_WRITE(MIPI_EOT_DISABLE(pipe), intel_dsi->eot_disable);
-
-	/* in terms of txbyteclkhs. actual high to low switch +
-	 * MIPI_STOP_STATE_STALL * MIPI_LP_BYTECLK.
-	 *
-	 * XXX: write MIPI_STOP_STATE_STALL?
-	 */
-	I915_WRITE(MIPI_HIGH_LOW_SWITCH_COUNT(pipe), 0x46);
-
-	/* XXX: low power clock equivalence in terms of byte clock. the number
-	 * of byte clocks occupied in one low power clock. based on txbyteclkhs
-	 * and txclkesc. txclkesc time / txbyteclk time * (105 +
-	 * MIPI_STOP_STATE_STALL) / 105.???
-	 */
-	I915_WRITE(MIPI_LP_BYTECLK(pipe), 4);
-
-	/* the bw essential for transmitting 16 long packets containing 252
-	 * bytes meant for dcs write memory command is programmed in this
-	 * register in terms of byte clocks. based on dsi transfer rate and the
-	 * number of lanes configured the time taken to transmit 16 long packets
-	 * in a dsi stream varies. */
-	I915_WRITE(MIPI_DBI_BW_CTRL(pipe), 0x820);
+	I915_WRITE(MIPI_HIGH_LOW_SWITCH_COUNT(pipe), \
+					intel_dsi->hs_to_lp_count);
+	I915_WRITE(MIPI_LP_BYTECLK(pipe), intel_dsi->lp_byte_clk);
+
+	I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 0x64);
 
 	I915_WRITE(MIPI_CLK_LANE_SWITCH_TIME_CNT(pipe),
-		   0xa << LP_HS_SSW_CNT_SHIFT |
-		   0x14 << HS_LP_PWR_SW_CNT_SHIFT);
+		((u32)intel_dsi->clk_lp_to_hs_count
+		<< LP_HS_SSW_CNT_SHIFT) |
+		(intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT));
+
+	if (is_vid_mode(intel_dsi)) {
+		I915_WRITE(MIPI_DPI_RESOLUTION(pipe),
+			(adjusted_mode->vdisplay << VERTICAL_ADDRESS_SHIFT) |
+			(adjusted_mode->hdisplay << HORIZONTAL_ADDRESS_SHIFT));
+
+		set_dsi_timings(encoder, adjusted_mode);
+
+		val = intel_dsi->channel << VID_MODE_CHANNEL_NUMBER_SHIFT |
+			intel_dsi->lane_count << DATA_LANES_PRG_REG_SHIFT |
+			intel_dsi->pixel_format;
+		I915_WRITE(MIPI_DSI_FUNC_PRG(pipe), val);
+
+		/* timeouts for recovery. one frame IIUC. if counter expires, EOT and
+		 * stop state. */
+
+		/*
+		 * In burst mode, value greater than one DPI line Time in byte clock
+		 * (txbyteclkhs) To timeout this timer 1+ of the above said value is
+		 * recommended.
+		 *
+		 * In non-burst mode, Value greater than one DPI frame time in byte
+		 * clock(txbyteclkhs) To timeout this timer 1+ of the above said value
+		 * is recommended.
+		 *
+		 * In DBI only mode, value greater than one DBI frame time in byte
+		 * clock(txbyteclkhs) To timeout this timer 1+ of the above said value
+		 * is recommended.
+		 */
+
+		if (intel_dsi->video_mode_format == VIDEO_MODE_BURST) {
+			I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
+				   txbyteclkhs(adjusted_mode->htotal, bpp,
+					       intel_dsi->lane_count) + 1);
+		} else {
+			I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
+				   txbyteclkhs(adjusted_mode->vtotal *
+					       adjusted_mode->htotal,
+					       bpp, intel_dsi->lane_count) + 1);
+		}
 
-	if (is_vid_mode(intel_dsi))
 		I915_WRITE(MIPI_VIDEO_MODE_FORMAT(pipe),
-			   intel_dsi->video_mode_format);
+					intel_dsi->video_frmt_cfg_bits |
+					intel_dsi->video_mode_type);
+	} else {
+		val = intel_dsi->channel << CMD_MODE_CHANNEL_NUMBER_SHIFT |
+			intel_dsi->lane_count << DATA_LANES_PRG_REG_SHIFT |
+			intel_dsi->data_width;
+		I915_WRITE(MIPI_DSI_FUNC_PRG(pipe), val);
+
+		I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
+			   txbyteclkhs(adjusted_mode->vtotal *
+				       adjusted_mode->htotal,
+				       bpp, intel_dsi->lane_count) + 1);
+
+		I915_WRITE(MIPI_DBI_BW_CTRL(pipe), intel_dsi->bw_timer);
+	}
 }
 
 static enum drm_connector_status
@@ -584,6 +625,7 @@ bool intel_dsi_init(struct drm_device *dev)
 
 	fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
 	intel_panel_init(&intel_connector->panel, fixed_mode);
+	intel_panel_setup_backlight(connector);
 
 	return true;
 
-- 
1.7.9.5

Re: [PATCH 4/4] drm/i915: Parameterize the MIPI enabling sequnece and adjust the sequence

[Ville Syrjälä]

On Mon, Oct 21, 2013 at 05:51:07PM +0530, Shobhit Kumar wrote:
> Has been tested on couple of panels now.

While it's nice to get patches, I can't say I'm very happy about the
shape of this one.

The patch contains several changes in one patch. It should be split up
into several patches. Based on a cursory examination I would suggest
something like this:
- weird ULPS ping-pong
- add backlight support
- moving the ->disable() call
- each of the new intel_dsi->foo/bar/etc. parameter could probably
  be a separate patch

As far as the various timeout related parameters are concerned, to me
it would make more sense to specify them in usecs or some other real
world unit. Or you could provide/leave in some helper functions to
calculate the clock based values from some real world values.

And finally justficiation for each of these changes is missing from
the current patch. We want to know why the code has to change.

> 
> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
> Signed-off-by: Vijaykumar Balakrishnan <vijayakumar.balakrishnan@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 |  334 +++++++++++++++++++++-----------------
>  2 files changed, 199 insertions(+), 146 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1c42bb4..1c28d02 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2375,6 +2375,17 @@ __i915_write(64)
>  #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)
> +
> +
>  /* "Broadcast RGB" property */
>  #define INTEL_BROADCAST_RGB_AUTO 0
>  #define INTEL_BROADCAST_RGB_FULL 1
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 5a9dbfd..241bb55 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -21,6 +21,8 @@
>   * DEALINGS IN THE SOFTWARE.
>   *
>   * Author: Jani Nikula <jani.nikula@intel.com>
> + *	 : Shobhit Kumar <shobhit.kumar@intel.com>
> + *	 : Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
>   */
>  
>  #include <drm/drmP.h>
> @@ -101,63 +103,80 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder)
>  	vlv_enable_dsi_pll(encoder);
>  }
>  
> -static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> -{
> -	DRM_DEBUG_KMS("\n");
> -}
> -
> -static void intel_dsi_enable(struct intel_encoder *encoder)
> +void intel_dsi_device_ready(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	int pipe = intel_crtc->pipe;
> -	u32 temp;
>  
>  	DRM_DEBUG_KMS("\n");
>  
>  	if (intel_dsi->dev.dev_ops->panel_reset)
>  		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
>  
> -	temp = I915_READ(MIPI_DEVICE_READY(pipe));
> -	if ((temp & DEVICE_READY) == 0) {
> -		temp &= ~ULPS_STATE_MASK;
> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | DEVICE_READY);
> -	} else if (temp & ULPS_STATE_MASK) {
> -		temp &= ~ULPS_STATE_MASK;
> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | ULPS_STATE_EXIT);
> -		/*
> -		 * We need to ensure that there is a minimum of 1 ms time
> -		 * available before clearing the UPLS exit state.
> -		 */
> -		msleep(2);
> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
> -	}
> +	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), LP_OUTPUT_HOLD, LP_OUTPUT_HOLD);
> +	usleep_range(1000, 1500);
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY |
> +			ULPS_STATE_EXIT, DEVICE_READY | ULPS_STATE_MASK);
> +	usleep_range(2000, 2500);
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
> +			DEVICE_READY | ULPS_STATE_MASK);
> +	usleep_range(2000, 2500);
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00,
> +			DEVICE_READY | ULPS_STATE_MASK);
> +	usleep_range(2000, 2500);
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
> +			DEVICE_READY | ULPS_STATE_MASK);
> +	usleep_range(2000, 2500);
>  
>  	if (intel_dsi->dev.dev_ops->send_otp_cmds)
>  		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
>  
> +}
> +static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> +{
> +	DRM_DEBUG_KMS("\n");
> +
> +	/* put device in ready state */
> +	intel_dsi_device_ready(encoder);
> +}
> +
> +static void intel_dsi_enable(struct intel_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> +	int pipe = intel_crtc->pipe;
> +	u32 temp;
> +
> +	DRM_DEBUG_KMS("\n");
> +
>  	if (is_cmd_mode(intel_dsi))
>  		I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
> -
> -	if (is_vid_mode(intel_dsi)) {
> +	else {
>  		msleep(20); /* XXX */
>  		dpi_send_cmd(intel_dsi, TURN_ON);
>  		msleep(100);
>  
>  		/* assert ip_tg_enable signal */
>  		temp = I915_READ(MIPI_PORT_CTRL(pipe));
> +		temp = temp | intel_dsi->port_bits;
>  		I915_WRITE(MIPI_PORT_CTRL(pipe), temp | DPI_ENABLE);
>  		POSTING_READ(MIPI_PORT_CTRL(pipe));
>  	}
>  
>  	if (intel_dsi->dev.dev_ops->enable)
>  		intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
> +
> +	intel_panel_enable_backlight(dev, pipe);
>  }
>  
>  static void intel_dsi_disable(struct intel_encoder *encoder)
>  {
> -	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	int pipe = intel_crtc->pipe;
> @@ -165,11 +184,18 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>  
>  	DRM_DEBUG_KMS("\n");
>  
> -	intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
> +	intel_panel_disable_backlight(dev);
> +	if (intel_dsi->backlight_off_delay >= 20)
> +		msleep(intel_dsi->backlight_off_delay);
> +	else
> +		usleep_range(intel_dsi->backlight_off_delay * 1000,
> +			(intel_dsi->backlight_off_delay * 1000) + 500);
>  
>  	if (is_vid_mode(intel_dsi)) {
> -		dpi_send_cmd(intel_dsi, SHUTDOWN);
> -		msleep(10);
> +		if (intel_dsi->send_shutdown) {
> +			dpi_send_cmd(intel_dsi, SHUTDOWN);
> +			msleep(intel_dsi->shutdown_pkt_delay);
> +		}
>  
>  		/* de-assert ip_tg_enable signal */
>  		temp = I915_READ(MIPI_PORT_CTRL(pipe));
> @@ -179,19 +205,52 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>  		msleep(2);
>  	}
>  
> -	temp = I915_READ(MIPI_DEVICE_READY(pipe));
> -	if (temp & DEVICE_READY) {
> -		temp &= ~DEVICE_READY;
> -		temp &= ~ULPS_STATE_MASK;
> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
> -	}
> +	/* if disable packets are sent before sending shutdown packet then in
> +	 * some next enable sequence send turn on packet error is observed */
> +	if (intel_dsi->dev.dev_ops->disable)
> +		intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
>  }
>  
> -static void intel_dsi_post_disable(struct intel_encoder *encoder)
> +void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
>  {
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> +	int pipe = intel_crtc->pipe;
> +
>  	DRM_DEBUG_KMS("\n");
>  
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
> +							ULPS_STATE_MASK);
> +	usleep_range(2000, 2500);
> +
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_EXIT,
> +							ULPS_STATE_MASK);
> +	usleep_range(2000, 2500);
> +
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
> +							ULPS_STATE_MASK);
> +	usleep_range(2000, 2500);
> +
> +	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), 0, LP_OUTPUT_HOLD);
> +	usleep_range(1000, 1500);
> +
> +	if (wait_for(((I915_READ(MIPI_PORT_CTRL(pipe)) & 0x20000)
> +					== 0x00000), 30))
> +		DRM_ERROR("DSI LP not going Low\n");
> +
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, DEVICE_READY);
> +	usleep_range(2000, 2500);
> +
>  	vlv_disable_dsi_pll(encoder);
> +
> +	if (intel_dsi->dev.dev_ops->disable_panel_power)
> +		intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev);
> +}
> +static void intel_dsi_post_disable(struct intel_encoder *encoder)
> +{
> +	DRM_DEBUG_KMS("\n");
> +	intel_dsi_clear_device_ready(encoder);
>  }
>  
>  static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
> @@ -251,20 +310,6 @@ static int intel_dsi_mode_valid(struct drm_connector *connector,
>  	return intel_dsi->dev.dev_ops->mode_valid(&intel_dsi->dev, mode);
>  }
>  
> -/* return txclkesc cycles in terms of divider and duration in us */
> -static u16 txclkesc(u32 divider, unsigned int us)
> -{
> -	switch (divider) {
> -	case ESCAPE_CLOCK_DIVIDER_1:
> -	default:
> -		return 20 * us;
> -	case ESCAPE_CLOCK_DIVIDER_2:
> -		return 10 * us;
> -	case ESCAPE_CLOCK_DIVIDER_4:
> -		return 5 * us;
> -	}
> -}
> -
>  /* return pixels in terms of txbyteclkhs */
>  static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count)
>  {
> @@ -313,26 +358,16 @@ static void set_dsi_timings(struct drm_encoder *encoder,
>  	I915_WRITE(MIPI_VBP_COUNT(pipe), vbp);
>  }
>  
> -static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
> +static void dsi_config(struct drm_encoder *encoder)
>  {
> -	struct drm_encoder *encoder = &intel_encoder->base;
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder);
> -	struct drm_display_mode *adjusted_mode =
> -		&intel_crtc->config.adjusted_mode;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
>  	int pipe = intel_crtc->pipe;
> -	unsigned int bpp = intel_crtc->config.pipe_bpp;
> -	u32 val, tmp;
> -
> -	DRM_DEBUG_KMS("pipe %d\n", pipe);
> +	u32 tmp;
>  
> -	/* Update the DSI PLL */
> -	vlv_enable_dsi_pll(intel_encoder);
> -
> -	/* XXX: Location of the call */
> -	band_gap_reset(dev_priv);
> +	DRM_DEBUG_KMS("\n");
>  
>  	/* escape clock divider, 20MHz, shared for A and C. device ready must be
>  	 * off when doing this! txclkesc? */
> @@ -345,102 +380,108 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>  	tmp &= ~READ_REQUEST_PRIORITY_MASK;
>  	I915_WRITE(MIPI_CTRL(pipe), tmp | READ_REQUEST_PRIORITY_HIGH);
>  
> -	/* XXX: why here, why like this? handling in irq handler?! */
> -	I915_WRITE(MIPI_INTR_STAT(pipe), 0xffffffff);
>  	I915_WRITE(MIPI_INTR_EN(pipe), 0xffffffff);
>  
> -	I915_WRITE(MIPI_DPHY_PARAM(pipe),
> -		   0x3c << EXIT_ZERO_COUNT_SHIFT |
> -		   0x1f << TRAIL_COUNT_SHIFT |
> -		   0xc5 << CLK_ZERO_COUNT_SHIFT |
> -		   0x1f << PREPARE_COUNT_SHIFT);
> +	I915_WRITE(MIPI_DPHY_PARAM(pipe), intel_dsi->dphy_reg);
> +}
>  
> -	I915_WRITE(MIPI_DPI_RESOLUTION(pipe),
> -		   adjusted_mode->vdisplay << VERTICAL_ADDRESS_SHIFT |
> -		   adjusted_mode->hdisplay << HORIZONTAL_ADDRESS_SHIFT);
> +static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
> +{
> +	struct drm_encoder *encoder = &intel_encoder->base;
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder);
> +	struct drm_display_mode *adjusted_mode =
> +		&intel_crtc->config.adjusted_mode;
> +	int pipe = intel_crtc->pipe;
> +	unsigned int bpp = intel_crtc->config.pipe_bpp;
> +	u32 val;
>  
> -	set_dsi_timings(encoder, adjusted_mode);
> +	band_gap_reset(dev_priv);
>  
> -	val = intel_dsi->lane_count << DATA_LANES_PRG_REG_SHIFT;
> -	if (is_cmd_mode(intel_dsi)) {
> -		val |= intel_dsi->channel << CMD_MODE_CHANNEL_NUMBER_SHIFT;
> -		val |= CMD_MODE_DATA_WIDTH_8_BIT; /* XXX */
> -	} else {
> -		val |= intel_dsi->channel << VID_MODE_CHANNEL_NUMBER_SHIFT;
> +	dsi_config(encoder);
>  
> -		/* XXX: cross-check bpp vs. pixel format? */
> -		val |= intel_dsi->pixel_format;
> -	}
> -	I915_WRITE(MIPI_DSI_FUNC_PRG(pipe), val);
> -
> -	/* timeouts for recovery. one frame IIUC. if counter expires, EOT and
> -	 * stop state. */
> -
> -	/*
> -	 * In burst mode, value greater than one DPI line Time in byte clock
> -	 * (txbyteclkhs) To timeout this timer 1+ of the above said value is
> -	 * recommended.
> -	 *
> -	 * In non-burst mode, Value greater than one DPI frame time in byte
> -	 * clock(txbyteclkhs) To timeout this timer 1+ of the above said value
> -	 * is recommended.
> -	 *
> -	 * In DBI only mode, value greater than one DBI frame time in byte
> -	 * clock(txbyteclkhs) To timeout this timer 1+ of the above said value
> -	 * is recommended.
> -	 */
> -
> -	if (is_vid_mode(intel_dsi) &&
> -	    intel_dsi->video_mode_format == VIDEO_MODE_BURST) {
> -		I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
> -			   txbyteclkhs(adjusted_mode->htotal, bpp,
> -				       intel_dsi->lane_count) + 1);
> -	} else {
> -		I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
> -			   txbyteclkhs(adjusted_mode->vtotal *
> -				       adjusted_mode->htotal,
> -				       bpp, intel_dsi->lane_count) + 1);
> -	}
> -	I915_WRITE(MIPI_LP_RX_TIMEOUT(pipe), 8309); /* max */
> -	I915_WRITE(MIPI_TURN_AROUND_TIMEOUT(pipe), 0x14); /* max */
> -	I915_WRITE(MIPI_DEVICE_RESET_TIMER(pipe), 0xffff); /* max */
> +	I915_WRITE(MIPI_LP_RX_TIMEOUT(pipe), intel_dsi->lp_rx_timeout);
> +	I915_WRITE(MIPI_TURN_AROUND_TIMEOUT(pipe),
> +					intel_dsi->turn_arnd_val);
> +	I915_WRITE(MIPI_DEVICE_RESET_TIMER(pipe),
> +					intel_dsi->rst_timer_val);
> +	/* in terms of low power clock */
> +	I915_WRITE(MIPI_INIT_COUNT(pipe), intel_dsi->init_count);
>  
> -	/* dphy stuff */
> +	if (intel_dsi->eot_disable)
> +		I915_WRITE(MIPI_EOT_DISABLE(pipe), 0);
> +	else
> +		I915_WRITE(MIPI_EOT_DISABLE(pipe), 1);
>  
> -	/* in terms of low power clock */
> -	I915_WRITE(MIPI_INIT_COUNT(pipe), txclkesc(ESCAPE_CLOCK_DIVIDER_1, 100));
> -
> -	/* recovery disables */
> -	I915_WRITE(MIPI_EOT_DISABLE(pipe), intel_dsi->eot_disable);
> -
> -	/* in terms of txbyteclkhs. actual high to low switch +
> -	 * MIPI_STOP_STATE_STALL * MIPI_LP_BYTECLK.
> -	 *
> -	 * XXX: write MIPI_STOP_STATE_STALL?
> -	 */
> -	I915_WRITE(MIPI_HIGH_LOW_SWITCH_COUNT(pipe), 0x46);
> -
> -	/* XXX: low power clock equivalence in terms of byte clock. the number
> -	 * of byte clocks occupied in one low power clock. based on txbyteclkhs
> -	 * and txclkesc. txclkesc time / txbyteclk time * (105 +
> -	 * MIPI_STOP_STATE_STALL) / 105.???
> -	 */
> -	I915_WRITE(MIPI_LP_BYTECLK(pipe), 4);
> -
> -	/* the bw essential for transmitting 16 long packets containing 252
> -	 * bytes meant for dcs write memory command is programmed in this
> -	 * register in terms of byte clocks. based on dsi transfer rate and the
> -	 * number of lanes configured the time taken to transmit 16 long packets
> -	 * in a dsi stream varies. */
> -	I915_WRITE(MIPI_DBI_BW_CTRL(pipe), 0x820);
> +	I915_WRITE(MIPI_HIGH_LOW_SWITCH_COUNT(pipe), \
> +					intel_dsi->hs_to_lp_count);
> +	I915_WRITE(MIPI_LP_BYTECLK(pipe), intel_dsi->lp_byte_clk);
> +
> +	I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 0x64);
>  
>  	I915_WRITE(MIPI_CLK_LANE_SWITCH_TIME_CNT(pipe),
> -		   0xa << LP_HS_SSW_CNT_SHIFT |
> -		   0x14 << HS_LP_PWR_SW_CNT_SHIFT);
> +		((u32)intel_dsi->clk_lp_to_hs_count
> +		<< LP_HS_SSW_CNT_SHIFT) |
> +		(intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT));
> +
> +	if (is_vid_mode(intel_dsi)) {
> +		I915_WRITE(MIPI_DPI_RESOLUTION(pipe),
> +			(adjusted_mode->vdisplay << VERTICAL_ADDRESS_SHIFT) |
> +			(adjusted_mode->hdisplay << HORIZONTAL_ADDRESS_SHIFT));
> +
> +		set_dsi_timings(encoder, adjusted_mode);
> +
> +		val = intel_dsi->channel << VID_MODE_CHANNEL_NUMBER_SHIFT |
> +			intel_dsi->lane_count << DATA_LANES_PRG_REG_SHIFT |
> +			intel_dsi->pixel_format;
> +		I915_WRITE(MIPI_DSI_FUNC_PRG(pipe), val);
> +
> +		/* timeouts for recovery. one frame IIUC. if counter expires, EOT and
> +		 * stop state. */
> +
> +		/*
> +		 * In burst mode, value greater than one DPI line Time in byte clock
> +		 * (txbyteclkhs) To timeout this timer 1+ of the above said value is
> +		 * recommended.
> +		 *
> +		 * In non-burst mode, Value greater than one DPI frame time in byte
> +		 * clock(txbyteclkhs) To timeout this timer 1+ of the above said value
> +		 * is recommended.
> +		 *
> +		 * In DBI only mode, value greater than one DBI frame time in byte
> +		 * clock(txbyteclkhs) To timeout this timer 1+ of the above said value
> +		 * is recommended.
> +		 */
> +
> +		if (intel_dsi->video_mode_format == VIDEO_MODE_BURST) {
> +			I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
> +				   txbyteclkhs(adjusted_mode->htotal, bpp,
> +					       intel_dsi->lane_count) + 1);
> +		} else {
> +			I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
> +				   txbyteclkhs(adjusted_mode->vtotal *
> +					       adjusted_mode->htotal,
> +					       bpp, intel_dsi->lane_count) + 1);
> +		}
>  
> -	if (is_vid_mode(intel_dsi))
>  		I915_WRITE(MIPI_VIDEO_MODE_FORMAT(pipe),
> -			   intel_dsi->video_mode_format);
> +					intel_dsi->video_frmt_cfg_bits |
> +					intel_dsi->video_mode_type);
> +	} else {
> +		val = intel_dsi->channel << CMD_MODE_CHANNEL_NUMBER_SHIFT |
> +			intel_dsi->lane_count << DATA_LANES_PRG_REG_SHIFT |
> +			intel_dsi->data_width;
> +		I915_WRITE(MIPI_DSI_FUNC_PRG(pipe), val);
> +
> +		I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
> +			   txbyteclkhs(adjusted_mode->vtotal *
> +				       adjusted_mode->htotal,
> +				       bpp, intel_dsi->lane_count) + 1);
> +
> +		I915_WRITE(MIPI_DBI_BW_CTRL(pipe), intel_dsi->bw_timer);
> +	}
>  }
>  
>  static enum drm_connector_status
> @@ -584,6 +625,7 @@ bool intel_dsi_init(struct drm_device *dev)
>  
>  	fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
>  	intel_panel_init(&intel_connector->panel, fixed_mode);
> +	intel_panel_setup_backlight(connector);
>  
>  	return true;
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 4/4] drm/i915: Parameterize the MIPI enabling sequnece and adjust the sequence

[Shobhit Kumar]

On 10/21/2013 6:53 PM, Ville Syrjälä wrote:
> On Mon, Oct 21, 2013 at 05:51:07PM +0530, Shobhit Kumar wrote:
>> Has been tested on couple of panels now.
>
> While it's nice to get patches, I can't say I'm very happy about the
> shape of this one.
>
> The patch contains several changes in one patch. It should be split up
> into several patches. Based on a cursory examination I would suggest
> something like this:
> - weird ULPS ping-pong

Suggested and approved sequence by HW team for ULPS entry/exit 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

I will push this is new patch

> - add backlight support

Ok will push in new patch

> - moving the ->disable() call

Earlier disable was called at the beginning even before pixel stream was 
stopped. Ideal flow would be to disable pixel stream and then follow 
panel's required disable sequence

> - each of the new intel_dsi->foo/bar/etc. parameter could probably
>    be a separate patch

Ok, I can break all parameter changes into a separate patch

>
> As far as the various timeout related parameters are concerned, to me
> it would make more sense to specify them in usecs or some other real
> world unit. Or you could provide/leave in some helper functions to
> calculate the clock based values from some real world values.

Few timeouts are as per spec. Are you referring to back-light or 
shutdown packet delays ? If yes we can change them to usecs.

>
> And finally justficiation for each of these changes is missing from
> the current patch. We want to know why the code has to change.

I hope I have provided some clarifications above. I will work on 
splitting this patch into few more patches for more clarity.

Regards
Shobhit

>
>>
>> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
>> Signed-off-by: Vijaykumar Balakrishnan <vijayakumar.balakrishnan@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 |  334 +++++++++++++++++++++-----------------
>>   2 files changed, 199 insertions(+), 146 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 1c42bb4..1c28d02 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2375,6 +2375,17 @@ __i915_write(64)
>>   #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)
>> +
>> +
>>   /* "Broadcast RGB" property */
>>   #define INTEL_BROADCAST_RGB_AUTO 0
>>   #define INTEL_BROADCAST_RGB_FULL 1
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 5a9dbfd..241bb55 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -21,6 +21,8 @@
>>    * DEALINGS IN THE SOFTWARE.
>>    *
>>    * Author: Jani Nikula <jani.nikula@intel.com>
>> + *	 : Shobhit Kumar <shobhit.kumar@intel.com>
>> + *	 : Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
>>    */
>>
>>   #include <drm/drmP.h>
>> @@ -101,63 +103,80 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder)
>>   	vlv_enable_dsi_pll(encoder);
>>   }
>>
>> -static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>> -{
>> -	DRM_DEBUG_KMS("\n");
>> -}
>> -
>> -static void intel_dsi_enable(struct intel_encoder *encoder)
>> +void intel_dsi_device_ready(struct intel_encoder *encoder)
>>   {
>>   	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>>   	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>>   	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>   	int pipe = intel_crtc->pipe;
>> -	u32 temp;
>>
>>   	DRM_DEBUG_KMS("\n");
>>
>>   	if (intel_dsi->dev.dev_ops->panel_reset)
>>   		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
>>
>> -	temp = I915_READ(MIPI_DEVICE_READY(pipe));
>> -	if ((temp & DEVICE_READY) == 0) {
>> -		temp &= ~ULPS_STATE_MASK;
>> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | DEVICE_READY);
>> -	} else if (temp & ULPS_STATE_MASK) {
>> -		temp &= ~ULPS_STATE_MASK;
>> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | ULPS_STATE_EXIT);
>> -		/*
>> -		 * We need to ensure that there is a minimum of 1 ms time
>> -		 * available before clearing the UPLS exit state.
>> -		 */
>> -		msleep(2);
>> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
>> -	}
>> +	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), LP_OUTPUT_HOLD, LP_OUTPUT_HOLD);
>> +	usleep_range(1000, 1500);
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY |
>> +			ULPS_STATE_EXIT, DEVICE_READY | ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
>> +			DEVICE_READY | ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00,
>> +			DEVICE_READY | ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
>> +			DEVICE_READY | ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>>
>>   	if (intel_dsi->dev.dev_ops->send_otp_cmds)
>>   		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
>>
>> +}
>> +static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>> +{
>> +	DRM_DEBUG_KMS("\n");
>> +
>> +	/* put device in ready state */
>> +	intel_dsi_device_ready(encoder);
>> +}
>> +
>> +static void intel_dsi_enable(struct intel_encoder *encoder)
>> +{
>> +	struct drm_device *dev = encoder->base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>> +	int pipe = intel_crtc->pipe;
>> +	u32 temp;
>> +
>> +	DRM_DEBUG_KMS("\n");
>> +
>>   	if (is_cmd_mode(intel_dsi))
>>   		I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
>> -
>> -	if (is_vid_mode(intel_dsi)) {
>> +	else {
>>   		msleep(20); /* XXX */
>>   		dpi_send_cmd(intel_dsi, TURN_ON);
>>   		msleep(100);
>>
>>   		/* assert ip_tg_enable signal */
>>   		temp = I915_READ(MIPI_PORT_CTRL(pipe));
>> +		temp = temp | intel_dsi->port_bits;
>>   		I915_WRITE(MIPI_PORT_CTRL(pipe), temp | DPI_ENABLE);
>>   		POSTING_READ(MIPI_PORT_CTRL(pipe));
>>   	}
>>
>>   	if (intel_dsi->dev.dev_ops->enable)
>>   		intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
>> +
>> +	intel_panel_enable_backlight(dev, pipe);
>>   }
>>
>>   static void intel_dsi_disable(struct intel_encoder *encoder)
>>   {
>> -	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>> +	struct drm_device *dev = encoder->base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>>   	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>   	int pipe = intel_crtc->pipe;
>> @@ -165,11 +184,18 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>>
>>   	DRM_DEBUG_KMS("\n");
>>
>> -	intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
>> +	intel_panel_disable_backlight(dev);
>> +	if (intel_dsi->backlight_off_delay >= 20)
>> +		msleep(intel_dsi->backlight_off_delay);
>> +	else
>> +		usleep_range(intel_dsi->backlight_off_delay * 1000,
>> +			(intel_dsi->backlight_off_delay * 1000) + 500);
>>
>>   	if (is_vid_mode(intel_dsi)) {
>> -		dpi_send_cmd(intel_dsi, SHUTDOWN);
>> -		msleep(10);
>> +		if (intel_dsi->send_shutdown) {
>> +			dpi_send_cmd(intel_dsi, SHUTDOWN);
>> +			msleep(intel_dsi->shutdown_pkt_delay);
>> +		}
>>
>>   		/* de-assert ip_tg_enable signal */
>>   		temp = I915_READ(MIPI_PORT_CTRL(pipe));
>> @@ -179,19 +205,52 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>>   		msleep(2);
>>   	}
>>
>> -	temp = I915_READ(MIPI_DEVICE_READY(pipe));
>> -	if (temp & DEVICE_READY) {
>> -		temp &= ~DEVICE_READY;
>> -		temp &= ~ULPS_STATE_MASK;
>> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
>> -	}
>> +	/* if disable packets are sent before sending shutdown packet then in
>> +	 * some next enable sequence send turn on packet error is observed */
>> +	if (intel_dsi->dev.dev_ops->disable)
>> +		intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
>>   }
>>
>> -static void intel_dsi_post_disable(struct intel_encoder *encoder)
>> +void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
>>   {
>> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>> +	int pipe = intel_crtc->pipe;
>> +
>>   	DRM_DEBUG_KMS("\n");
>>
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
>> +							ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_EXIT,
>> +							ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
>> +							ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +
>> +	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), 0, LP_OUTPUT_HOLD);
>> +	usleep_range(1000, 1500);
>> +
>> +	if (wait_for(((I915_READ(MIPI_PORT_CTRL(pipe)) & 0x20000)
>> +					== 0x00000), 30))
>> +		DRM_ERROR("DSI LP not going Low\n");
>> +
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, DEVICE_READY);
>> +	usleep_range(2000, 2500);
>> +
>>   	vlv_disable_dsi_pll(encoder);
>> +
>> +	if (intel_dsi->dev.dev_ops->disable_panel_power)
>> +		intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev);
>> +}
>> +static void intel_dsi_post_disable(struct intel_encoder *encoder)
>> +{
>> +	DRM_DEBUG_KMS("\n");
>> +	intel_dsi_clear_device_ready(encoder);
>>   }
>>
>>   static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
>> @@ -251,20 +310,6 @@ static int intel_dsi_mode_valid(struct drm_connector *connector,
>>   	return intel_dsi->dev.dev_ops->mode_valid(&intel_dsi->dev, mode);
>>   }
>>
>> -/* return txclkesc cycles in terms of divider and duration in us */
>> -static u16 txclkesc(u32 divider, unsigned int us)
>> -{
>> -	switch (divider) {
>> -	case ESCAPE_CLOCK_DIVIDER_1:
>> -	default:
>> -		return 20 * us;
>> -	case ESCAPE_CLOCK_DIVIDER_2:
>> -		return 10 * us;
>> -	case ESCAPE_CLOCK_DIVIDER_4:
>> -		return 5 * us;
>> -	}
>> -}
>> -
>>   /* return pixels in terms of txbyteclkhs */
>>   static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count)
>>   {
>> @@ -313,26 +358,16 @@ static void set_dsi_timings(struct drm_encoder *encoder,
>>   	I915_WRITE(MIPI_VBP_COUNT(pipe), vbp);
>>   }
>>
>> -static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>> +static void dsi_config(struct drm_encoder *encoder)
>>   {
>> -	struct drm_encoder *encoder = &intel_encoder->base;
>>   	struct drm_device *dev = encoder->dev;
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
>>   	struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder);
>> -	struct drm_display_mode *adjusted_mode =
>> -		&intel_crtc->config.adjusted_mode;
>> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
>>   	int pipe = intel_crtc->pipe;
>> -	unsigned int bpp = intel_crtc->config.pipe_bpp;
>> -	u32 val, tmp;
>> -
>> -	DRM_DEBUG_KMS("pipe %d\n", pipe);
>> +	u32 tmp;
>>
>> -	/* Update the DSI PLL */
>> -	vlv_enable_dsi_pll(intel_encoder);
>> -
>> -	/* XXX: Location of the call */
>> -	band_gap_reset(dev_priv);
>> +	DRM_DEBUG_KMS("\n");
>>
>>   	/* escape clock divider, 20MHz, shared for A and C. device ready must be
>>   	 * off when doing this! txclkesc? */
>> @@ -345,102 +380,108 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>>   	tmp &= ~READ_REQUEST_PRIORITY_MASK;
>>   	I915_WRITE(MIPI_CTRL(pipe), tmp | READ_REQUEST_PRIORITY_HIGH);
>>
>> -	/* XXX: why here, why like this? handling in irq handler?! */
>> -	I915_WRITE(MIPI_INTR_STAT(pipe), 0xffffffff);
>>   	I915_WRITE(MIPI_INTR_EN(pipe), 0xffffffff);
>>
>> -	I915_WRITE(MIPI_DPHY_PARAM(pipe),
>> -		   0x3c << EXIT_ZERO_COUNT_SHIFT |
>> -		   0x1f << TRAIL_COUNT_SHIFT |
>> -		   0xc5 << CLK_ZERO_COUNT_SHIFT |
>> -		   0x1f << PREPARE_COUNT_SHIFT);
>> +	I915_WRITE(MIPI_DPHY_PARAM(pipe), intel_dsi->dphy_reg);
>> +}
>>
>> -	I915_WRITE(MIPI_DPI_RESOLUTION(pipe),
>> -		   adjusted_mode->vdisplay << VERTICAL_ADDRESS_SHIFT |
>> -		   adjusted_mode->hdisplay << HORIZONTAL_ADDRESS_SHIFT);
>> +static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>> +{
>> +	struct drm_encoder *encoder = &intel_encoder->base;
>> +	struct drm_device *dev = encoder->dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
>> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder);
>> +	struct drm_display_mode *adjusted_mode =
>> +		&intel_crtc->config.adjusted_mode;
>> +	int pipe = intel_crtc->pipe;
>> +	unsigned int bpp = intel_crtc->config.pipe_bpp;
>> +	u32 val;
>>
>> -	set_dsi_timings(encoder, adjusted_mode);
>> +	band_gap_reset(dev_priv);
>>
>> -	val = intel_dsi->lane_count << DATA_LANES_PRG_REG_SHIFT;
>> -	if (is_cmd_mode(intel_dsi)) {
>> -		val |= intel_dsi->channel << CMD_MODE_CHANNEL_NUMBER_SHIFT;
>> -		val |= CMD_MODE_DATA_WIDTH_8_BIT; /* XXX */
>> -	} else {
>> -		val |= intel_dsi->channel << VID_MODE_CHANNEL_NUMBER_SHIFT;
>> +	dsi_config(encoder);
>>
>> -		/* XXX: cross-check bpp vs. pixel format? */
>> -		val |= intel_dsi->pixel_format;
>> -	}
>> -	I915_WRITE(MIPI_DSI_FUNC_PRG(pipe), val);
>> -
>> -	/* timeouts for recovery. one frame IIUC. if counter expires, EOT and
>> -	 * stop state. */
>> -
>> -	/*
>> -	 * In burst mode, value greater than one DPI line Time in byte clock
>> -	 * (txbyteclkhs) To timeout this timer 1+ of the above said value is
>> -	 * recommended.
>> -	 *
>> -	 * In non-burst mode, Value greater than one DPI frame time in byte
>> -	 * clock(txbyteclkhs) To timeout this timer 1+ of the above said value
>> -	 * is recommended.
>> -	 *
>> -	 * In DBI only mode, value greater than one DBI frame time in byte
>> -	 * clock(txbyteclkhs) To timeout this timer 1+ of the above said value
>> -	 * is recommended.
>> -	 */
>> -
>> -	if (is_vid_mode(intel_dsi) &&
>> -	    intel_dsi->video_mode_format == VIDEO_MODE_BURST) {
>> -		I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
>> -			   txbyteclkhs(adjusted_mode->htotal, bpp,
>> -				       intel_dsi->lane_count) + 1);
>> -	} else {
>> -		I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
>> -			   txbyteclkhs(adjusted_mode->vtotal *
>> -				       adjusted_mode->htotal,
>> -				       bpp, intel_dsi->lane_count) + 1);
>> -	}
>> -	I915_WRITE(MIPI_LP_RX_TIMEOUT(pipe), 8309); /* max */
>> -	I915_WRITE(MIPI_TURN_AROUND_TIMEOUT(pipe), 0x14); /* max */
>> -	I915_WRITE(MIPI_DEVICE_RESET_TIMER(pipe), 0xffff); /* max */
>> +	I915_WRITE(MIPI_LP_RX_TIMEOUT(pipe), intel_dsi->lp_rx_timeout);
>> +	I915_WRITE(MIPI_TURN_AROUND_TIMEOUT(pipe),
>> +					intel_dsi->turn_arnd_val);
>> +	I915_WRITE(MIPI_DEVICE_RESET_TIMER(pipe),
>> +					intel_dsi->rst_timer_val);
>> +	/* in terms of low power clock */
>> +	I915_WRITE(MIPI_INIT_COUNT(pipe), intel_dsi->init_count);
>>
>> -	/* dphy stuff */
>> +	if (intel_dsi->eot_disable)
>> +		I915_WRITE(MIPI_EOT_DISABLE(pipe), 0);
>> +	else
>> +		I915_WRITE(MIPI_EOT_DISABLE(pipe), 1);
>>
>> -	/* in terms of low power clock */
>> -	I915_WRITE(MIPI_INIT_COUNT(pipe), txclkesc(ESCAPE_CLOCK_DIVIDER_1, 100));
>> -
>> -	/* recovery disables */
>> -	I915_WRITE(MIPI_EOT_DISABLE(pipe), intel_dsi->eot_disable);
>> -
>> -	/* in terms of txbyteclkhs. actual high to low switch +
>> -	 * MIPI_STOP_STATE_STALL * MIPI_LP_BYTECLK.
>> -	 *
>> -	 * XXX: write MIPI_STOP_STATE_STALL?
>> -	 */
>> -	I915_WRITE(MIPI_HIGH_LOW_SWITCH_COUNT(pipe), 0x46);
>> -
>> -	/* XXX: low power clock equivalence in terms of byte clock. the number
>> -	 * of byte clocks occupied in one low power clock. based on txbyteclkhs
>> -	 * and txclkesc. txclkesc time / txbyteclk time * (105 +
>> -	 * MIPI_STOP_STATE_STALL) / 105.???
>> -	 */
>> -	I915_WRITE(MIPI_LP_BYTECLK(pipe), 4);
>> -
>> -	/* the bw essential for transmitting 16 long packets containing 252
>> -	 * bytes meant for dcs write memory command is programmed in this
>> -	 * register in terms of byte clocks. based on dsi transfer rate and the
>> -	 * number of lanes configured the time taken to transmit 16 long packets
>> -	 * in a dsi stream varies. */
>> -	I915_WRITE(MIPI_DBI_BW_CTRL(pipe), 0x820);
>> +	I915_WRITE(MIPI_HIGH_LOW_SWITCH_COUNT(pipe), \
>> +					intel_dsi->hs_to_lp_count);
>> +	I915_WRITE(MIPI_LP_BYTECLK(pipe), intel_dsi->lp_byte_clk);
>> +
>> +	I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 0x64);
>>
>>   	I915_WRITE(MIPI_CLK_LANE_SWITCH_TIME_CNT(pipe),
>> -		   0xa << LP_HS_SSW_CNT_SHIFT |
>> -		   0x14 << HS_LP_PWR_SW_CNT_SHIFT);
>> +		((u32)intel_dsi->clk_lp_to_hs_count
>> +		<< LP_HS_SSW_CNT_SHIFT) |
>> +		(intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT));
>> +
>> +	if (is_vid_mode(intel_dsi)) {
>> +		I915_WRITE(MIPI_DPI_RESOLUTION(pipe),
>> +			(adjusted_mode->vdisplay << VERTICAL_ADDRESS_SHIFT) |
>> +			(adjusted_mode->hdisplay << HORIZONTAL_ADDRESS_SHIFT));
>> +
>> +		set_dsi_timings(encoder, adjusted_mode);
>> +
>> +		val = intel_dsi->channel << VID_MODE_CHANNEL_NUMBER_SHIFT |
>> +			intel_dsi->lane_count << DATA_LANES_PRG_REG_SHIFT |
>> +			intel_dsi->pixel_format;
>> +		I915_WRITE(MIPI_DSI_FUNC_PRG(pipe), val);
>> +
>> +		/* timeouts for recovery. one frame IIUC. if counter expires, EOT and
>> +		 * stop state. */
>> +
>> +		/*
>> +		 * In burst mode, value greater than one DPI line Time in byte clock
>> +		 * (txbyteclkhs) To timeout this timer 1+ of the above said value is
>> +		 * recommended.
>> +		 *
>> +		 * In non-burst mode, Value greater than one DPI frame time in byte
>> +		 * clock(txbyteclkhs) To timeout this timer 1+ of the above said value
>> +		 * is recommended.
>> +		 *
>> +		 * In DBI only mode, value greater than one DBI frame time in byte
>> +		 * clock(txbyteclkhs) To timeout this timer 1+ of the above said value
>> +		 * is recommended.
>> +		 */
>> +
>> +		if (intel_dsi->video_mode_format == VIDEO_MODE_BURST) {
>> +			I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
>> +				   txbyteclkhs(adjusted_mode->htotal, bpp,
>> +					       intel_dsi->lane_count) + 1);
>> +		} else {
>> +			I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
>> +				   txbyteclkhs(adjusted_mode->vtotal *
>> +					       adjusted_mode->htotal,
>> +					       bpp, intel_dsi->lane_count) + 1);
>> +		}
>>
>> -	if (is_vid_mode(intel_dsi))
>>   		I915_WRITE(MIPI_VIDEO_MODE_FORMAT(pipe),
>> -			   intel_dsi->video_mode_format);
>> +					intel_dsi->video_frmt_cfg_bits |
>> +					intel_dsi->video_mode_type);
>> +	} else {
>> +		val = intel_dsi->channel << CMD_MODE_CHANNEL_NUMBER_SHIFT |
>> +			intel_dsi->lane_count << DATA_LANES_PRG_REG_SHIFT |
>> +			intel_dsi->data_width;
>> +		I915_WRITE(MIPI_DSI_FUNC_PRG(pipe), val);
>> +
>> +		I915_WRITE(MIPI_HS_TX_TIMEOUT(pipe),
>> +			   txbyteclkhs(adjusted_mode->vtotal *
>> +				       adjusted_mode->htotal,
>> +				       bpp, intel_dsi->lane_count) + 1);
>> +
>> +		I915_WRITE(MIPI_DBI_BW_CTRL(pipe), intel_dsi->bw_timer);
>> +	}
>>   }
>>
>>   static enum drm_connector_status
>> @@ -584,6 +625,7 @@ bool intel_dsi_init(struct drm_device *dev)
>>
>>   	fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
>>   	intel_panel_init(&intel_connector->panel, fixed_mode);
>> +	intel_panel_setup_backlight(connector);
>>
>>   	return true;
>>
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Re: [PATCH 4/4] drm/i915: Parameterize the MIPI enabling sequnece and adjust the sequence

[Ville Syrjälä]

On Tue, Oct 22, 2013 at 02:36:18PM +0530, Shobhit Kumar wrote:
> On 10/21/2013 6:53 PM, Ville Syrjälä wrote:
> > On Mon, Oct 21, 2013 at 05:51:07PM +0530, Shobhit Kumar wrote:
> >> Has been tested on couple of panels now.
> >
> > While it's nice to get patches, I can't say I'm very happy about the
> > shape of this one.
> >
> > The patch contains several changes in one patch. It should be split up
> > into several patches. Based on a cursory examination I would suggest
> > something like this:
> > - weird ULPS ping-pong
> 
> Suggested and approved sequence by HW team for ULPS entry/exit 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
> 
> I will push this is new patch
> 
> > - add backlight support
> 
> Ok will push in new patch
> 
> > - moving the ->disable() call
> 
> Earlier disable was called at the beginning even before pixel stream was 
> stopped. Ideal flow would be to disable pixel stream and then follow 
> panel's required disable sequence
> 
> > - each of the new intel_dsi->foo/bar/etc. parameter could probably
> >    be a separate patch
> 
> Ok, I can break all parameter changes into a separate patch
> 
> >
> > As far as the various timeout related parameters are concerned, to me
> > it would make more sense to specify them in usecs or some other real
> > world unit. Or you could provide/leave in some helper functions to
> > calculate the clock based values from some real world values.
> 
> Few timeouts are as per spec. Are you referring to back-light or 
> shutdown packet delays ? If yes we can change them to usecs.

These at least:
MIPI_LP_RX_TIMEOUT
MIPI_TURN_AROUND_TIMEOUT
MIPI_DEVICE_RESET_TIMER
MIPI_INIT_COUNT
MIPI_HIGH_LOW_SWITCH_COUNT

It's been a while since I read the spec so I don't remember anymore how
all those were specified there. If the spec defines them in some clocks,
then that would be the best choice, but if they're specified in some
time units, then I would possibly follow that. At the very least you
should add some documentation about the units in the intel_dsi struct
(or whereever we expect these things to live).

> 
> >
> > And finally justficiation for each of these changes is missing from
> > the current patch. We want to know why the code has to change.
> 
> I hope I have provided some clarifications above. I will work on 
> splitting this patch into few more patches for more clarity.

Yeah, looks like good stuff. Looking forward to seeing the split up
patches. Thanks.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 4/4] drm/i915: Parameterize the MIPI enabling sequnece and adjust the sequence

[Shobhit Kumar]

On 10/22/2013 04:19 PM, Ville Syrjälä wrote:
> On Tue, Oct 22, 2013 at 02:36:18PM +0530, Shobhit Kumar wrote:
>> On 10/21/2013 6:53 PM, Ville Syrjälä wrote:
>>> On Mon, Oct 21, 2013 at 05:51:07PM +0530, Shobhit Kumar wrote:
>>>> Has been tested on couple of panels now.
>>>
>>> While it's nice to get patches, I can't say I'm very happy about the
>>> shape of this one.
>>>
>>> The patch contains several changes in one patch. It should be split up
>>> into several patches. Based on a cursory examination I would suggest
>>> something like this:
>>> - weird ULPS ping-pong
>>
>> Suggested and approved sequence by HW team for ULPS entry/exit 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
>>
>> I will push this is new patch
>>
>>> - add backlight support
>>
>> Ok will push in new patch
>>
>>> - moving the ->disable() call
>>
>> Earlier disable was called at the beginning even before pixel stream was
>> stopped. Ideal flow would be to disable pixel stream and then follow
>> panel's required disable sequence
>>
>>> - each of the new intel_dsi->foo/bar/etc. parameter could probably
>>>     be a separate patch
>>
>> Ok, I can break all parameter changes into a separate patch
>>
>>>
>>> As far as the various timeout related parameters are concerned, to me
>>> it would make more sense to specify them in usecs or some other real
>>> world unit. Or you could provide/leave in some helper functions to
>>> calculate the clock based values from some real world values.
>>
>> Few timeouts are as per spec. Are you referring to back-light or
>> shutdown packet delays ? If yes we can change them to usecs.
>
> These at least:
> MIPI_LP_RX_TIMEOUT
> MIPI_TURN_AROUND_TIMEOUT
> MIPI_DEVICE_RESET_TIMER
> MIPI_INIT_COUNT
> MIPI_HIGH_LOW_SWITCH_COUNT
>
> It's been a while since I read the spec so I don't remember anymore how
> all those were specified there. If the spec defines them in some clocks,
> then that would be the best choice, but if they're specified in some
> time units, then I would possibly follow that. At the very least you
> should add some documentation about the units in the intel_dsi struct
> (or whereever we expect these things to live).
>

Ok, got it. All the above are defined as byte clocks. Will take care as 
per your suggestions.

>>
>>>
>>> And finally justficiation for each of these changes is missing from
>>> the current patch. We want to know why the code has to change.
>>
>> I hope I have provided some clarifications above. I will work on
>> splitting this patch into few more patches for more clarity.
>
> Yeah, looks like good stuff. Looking forward to seeing the split up
> patches. Thanks.
>

WIP

Regards
Shobhit