diff mbox

[GLK,MIPI,DSI,V3,1/7] drm/i915/glk: Program dphy param reg for GLK

Message ID 1483361668-2095-2-git-send-email-madhav.chauhan@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chauhan, Madhav Jan. 2, 2017, 12:54 p.m. UTC 
From: Deepak M <m.deepak@intel.com>

For GEMINILAKE, dphy param reg values are programmed in terms
of HS byte clock count while for legacy platforms in terms of
HS ddr clk count.

Signed-off-by: Deepak M <m.deepak@intel.com>
Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 33 +++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

Comments

Jani Nikula Jan. 18, 2017, 11:09 a.m. UTC | #1 
On Mon, 02 Jan 2017, Madhav Chauhan <madhav.chauhan@intel.com> wrote:
> From: Deepak M <m.deepak@intel.com>
>
> For GEMINILAKE, dphy param reg values are programmed in terms
> of HS byte clock count while for legacy platforms in terms of
> HS ddr clk count.

No need to call everything before this one "legacy".

> Signed-off-by: Deepak M <m.deepak@intel.com>
> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 33 +++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 8f683b8..8059cbb 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -695,16 +695,26 @@ struct drm_panel *vbt_panel_init(struct intel_dsi *intel_dsi, u16 panel_id)
>  	/* count values in UI = (ns value) * (bitrate / (2 * 10^6))
>  	 *
>  	 * Since txddrclkhs_i is 2xUI, all the count values programmed in
> -	 * DPHY param register are divided by 2
> +	 * DPHY param register are divided by 2 except GEMINILAKE where it is
> +	 * programmed in terms of HS byte clock so divided by 8

Would you say these two hold?

1) HSDDR = 2 * HS

2) HS byte clock = HS / 8

So it would seem to me either the existing code or your patch is
wrong. (Or I'm seriously confused.)

If the register is in terms of clock *cycles*, not frequency, should the
HSDDR based clock (pre-GLK) actually have *twice* the clock cycles, not
*half*? Making the existing code wrong?

The existing code could use some serious cleanup to make it readable. :(

BR,
Jani.



>  	 *
>  	 * prepare count
>  	 */
>  	ths_prepare_ns = max(mipi_config->ths_prepare,
>  			     mipi_config->tclk_prepare);
> -	prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num * 2);
> +	if (IS_GEMINILAKE(dev_priv))
> +		prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num * 8);
> +	else
> +		prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num * 2);
>  
>  	/* exit zero count */
> -	exit_zero_cnt = DIV_ROUND_UP(
> +	if (IS_GEMINILAKE(dev_priv))
> +		exit_zero_cnt = DIV_ROUND_UP(
> +				(ths_prepare_hszero - ths_prepare_ns) * ui_den,
> +				ui_num * 8
> +				);
> +	else
> +		exit_zero_cnt = DIV_ROUND_UP(
>  				(ths_prepare_hszero - ths_prepare_ns) * ui_den,
>  				ui_num * 2
>  				);
> @@ -719,13 +729,22 @@ struct drm_panel *vbt_panel_init(struct intel_dsi *intel_dsi, u16 panel_id)
>  		exit_zero_cnt += 1;
>  
>  	/* clk zero count */
> -	clk_zero_cnt = DIV_ROUND_UP(
> -			(tclk_prepare_clkzero -	ths_prepare_ns)
> -			* ui_den, 2 * ui_num);
> +	if (IS_GEMINILAKE(dev_priv))
> +		clk_zero_cnt = DIV_ROUND_UP(
> +				(tclk_prepare_clkzero -	ths_prepare_ns)
> +				* ui_den, 8 * ui_num);
> +	else
> +		clk_zero_cnt = DIV_ROUND_UP(
> +				(tclk_prepare_clkzero -	ths_prepare_ns)
> +				* ui_den, 2 * ui_num);
>  
>  	/* trail count */
>  	tclk_trail_ns = max(mipi_config->tclk_trail, mipi_config->ths_trail);
> -	trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 2 * ui_num);
> +
> +	if (IS_GEMINILAKE(dev_priv))
> +		trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 8 * ui_num);
> +	else
> +		trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 2 * ui_num);
>  
>  	if (prepare_cnt > PREPARE_CNT_MAX ||
>  		exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
Jani Nikula Jan. 18, 2017, 2:10 p.m. UTC | #2 
On Wed, 18 Jan 2017, Jani Nikula <jani.nikula@intel.com> wrote:
> On Mon, 02 Jan 2017, Madhav Chauhan <madhav.chauhan@intel.com> wrote:
>> From: Deepak M <m.deepak@intel.com>
>>
>> For GEMINILAKE, dphy param reg values are programmed in terms
>> of HS byte clock count while for legacy platforms in terms of
>> HS ddr clk count.
>
> No need to call everything before this one "legacy".
>
>> Signed-off-by: Deepak M <m.deepak@intel.com>
>> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 33 +++++++++++++++++++++++-------
>>  1 file changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index 8f683b8..8059cbb 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> @@ -695,16 +695,26 @@ struct drm_panel *vbt_panel_init(struct intel_dsi *intel_dsi, u16 panel_id)
>>  	/* count values in UI = (ns value) * (bitrate / (2 * 10^6))
>>  	 *
>>  	 * Since txddrclkhs_i is 2xUI, all the count values programmed in
>> -	 * DPHY param register are divided by 2
>> +	 * DPHY param register are divided by 2 except GEMINILAKE where it is
>> +	 * programmed in terms of HS byte clock so divided by 8
>
> Would you say these two hold?
>
> 1) HSDDR = 2 * HS
>
> 2) HS byte clock = HS / 8
>
> So it would seem to me either the existing code or your patch is
> wrong. (Or I'm seriously confused.)
>
> If the register is in terms of clock *cycles*, not frequency, should the
> HSDDR based clock (pre-GLK) actually have *twice* the clock cycles, not
> *half*? Making the existing code wrong?
>
> The existing code could use some serious cleanup to make it readable. :(

Additionally, I think you could use a variable to handle this, to not
add so much duplicated code.

BR,
Jani.



>
> BR,
> Jani.
>
>
>
>>  	 *
>>  	 * prepare count
>>  	 */
>>  	ths_prepare_ns = max(mipi_config->ths_prepare,
>>  			     mipi_config->tclk_prepare);
>> -	prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num * 2);
>> +	if (IS_GEMINILAKE(dev_priv))
>> +		prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num * 8);
>> +	else
>> +		prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num * 2);
>>  
>>  	/* exit zero count */
>> -	exit_zero_cnt = DIV_ROUND_UP(
>> +	if (IS_GEMINILAKE(dev_priv))
>> +		exit_zero_cnt = DIV_ROUND_UP(
>> +				(ths_prepare_hszero - ths_prepare_ns) * ui_den,
>> +				ui_num * 8
>> +				);
>> +	else
>> +		exit_zero_cnt = DIV_ROUND_UP(
>>  				(ths_prepare_hszero - ths_prepare_ns) * ui_den,
>>  				ui_num * 2
>>  				);
>> @@ -719,13 +729,22 @@ struct drm_panel *vbt_panel_init(struct intel_dsi *intel_dsi, u16 panel_id)
>>  		exit_zero_cnt += 1;
>>  
>>  	/* clk zero count */
>> -	clk_zero_cnt = DIV_ROUND_UP(
>> -			(tclk_prepare_clkzero -	ths_prepare_ns)
>> -			* ui_den, 2 * ui_num);
>> +	if (IS_GEMINILAKE(dev_priv))
>> +		clk_zero_cnt = DIV_ROUND_UP(
>> +				(tclk_prepare_clkzero -	ths_prepare_ns)
>> +				* ui_den, 8 * ui_num);
>> +	else
>> +		clk_zero_cnt = DIV_ROUND_UP(
>> +				(tclk_prepare_clkzero -	ths_prepare_ns)
>> +				* ui_den, 2 * ui_num);
>>  
>>  	/* trail count */
>>  	tclk_trail_ns = max(mipi_config->tclk_trail, mipi_config->ths_trail);
>> -	trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 2 * ui_num);
>> +
>> +	if (IS_GEMINILAKE(dev_priv))
>> +		trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 8 * ui_num);
>> +	else
>> +		trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 2 * ui_num);
>>  
>>  	if (prepare_cnt > PREPARE_CNT_MAX ||
>>  		exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
Chauhan, Madhav Jan. 19, 2017, 1:39 p.m. UTC | #3 
> -----Original Message-----
> From: Nikula, Jani
> Sent: Wednesday, January 18, 2017 7:41 PM
> To: Chauhan, Madhav <madhav.chauhan@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Shankar, Uma <uma.shankar@intel.com>; Mukherjee, Indranil
> <indranil.mukherjee@intel.com>; Kamath, Sunil <sunil.kamath@intel.com>;
> Saarinen, Jani <jani.saarinen@intel.com>; Conselvan De Oliveira, Ander
> <ander.conselvan.de.oliveira@intel.com>; Konduru, Chandra
> <chandra.konduru@intel.com>; Kumar, Shobhit
> <shobhit.kumar@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Deepak
> M <m.deepak@intel.com>; Chauhan, Madhav
> <madhav.chauhan@intel.com>
> Subject: Re: [GLK MIPI DSI V3 1/7] drm/i915/glk: Program dphy param reg for
> GLK
> 
> On Wed, 18 Jan 2017, Jani Nikula <jani.nikula@intel.com> wrote:
> > On Mon, 02 Jan 2017, Madhav Chauhan <madhav.chauhan@intel.com>
> wrote:
> >> From: Deepak M <m.deepak@intel.com>
> >>
> >> For GEMINILAKE, dphy param reg values are programmed in terms of HS
> >> byte clock count while for legacy platforms in terms of HS ddr clk
> >> count.
> >
> > No need to call everything before this one "legacy".
> >
> >> Signed-off-by: Deepak M <m.deepak@intel.com>
> >> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 33
> >> +++++++++++++++++++++++-------
> >>  1 file changed, 26 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >> b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >> index 8f683b8..8059cbb 100644
> >> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >> @@ -695,16 +695,26 @@ struct drm_panel *vbt_panel_init(struct
> intel_dsi *intel_dsi, u16 panel_id)
> >>  	/* count values in UI = (ns value) * (bitrate / (2 * 10^6))
> >>  	 *
> >>  	 * Since txddrclkhs_i is 2xUI, all the count values programmed in
> >> -	 * DPHY param register are divided by 2
> >> +	 * DPHY param register are divided by 2 except GEMINILAKE where it
> is
> >> +	 * programmed in terms of HS byte clock so divided by 8
> >
> > Would you say these two hold?
> >
> > 1) HSDDR = 2 * HS
> >
> > 2) HS byte clock = HS / 8
> >
> > So it would seem to me either the existing code or your patch is
> > wrong. (Or I'm seriously confused.)
> >
> > If the register is in terms of clock *cycles*, not frequency, should
> > the HSDDR based clock (pre-GLK) actually have *twice* the clock
> > cycles, not *half*? Making the existing code wrong?
> >
> > The existing code could use some serious cleanup to make it readable.
> > :(
> 
> Additionally, I think you could use a variable to handle this, to not add so
> much duplicated code.

Old Comments are not very clear to justify this calculation.
Went through dphy/IP spec and here is my explanation
1. As per DPHY spec
	DDR Clock period = 1UI + 1UI = 2UI
2. 1UI in terms of time:
	1UI (sec) = 1/(Bitrate *10^3)	// bitrate is in KHZ
	1UI (nsec) = (1*10^9)/(Bitrate*10^3) = 10^6/Bitrate
3. DDR clock period is twice of UI period as per 1 = (2*10^6)/Bitrate
4. ths_prepare_ns (via mipi_config) is to be programmed in terms *DDR clock count*  to dphy register

	DDR clock count = ths_prepare_ns /((2*10^6)/Bitrate)) = (ths_prepare_ns*Bitrate)/(2*10^6)
> 
> BR,
> Jani.
> 
> 
> 
> >
> > BR,
> > Jani.
> >
> >
> >
> >>  	 *
> >>  	 * prepare count
> >>  	 */
> >>  	ths_prepare_ns = max(mipi_config->ths_prepare,
> >>  			     mipi_config->tclk_prepare);
> >> -	prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num *
> 2);
> >> +	if (IS_GEMINILAKE(dev_priv))
> >> +		prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den,
> ui_num * 8);
> >> +	else
> >> +		prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den,
> ui_num * 2);
> >>
> >>  	/* exit zero count */
> >> -	exit_zero_cnt = DIV_ROUND_UP(
> >> +	if (IS_GEMINILAKE(dev_priv))
> >> +		exit_zero_cnt = DIV_ROUND_UP(
> >> +				(ths_prepare_hszero - ths_prepare_ns) *
> ui_den,
> >> +				ui_num * 8
> >> +				);
> >> +	else
> >> +		exit_zero_cnt = DIV_ROUND_UP(
> >>  				(ths_prepare_hszero - ths_prepare_ns) *
> ui_den,
> >>  				ui_num * 2
> >>  				);
> >> @@ -719,13 +729,22 @@ struct drm_panel *vbt_panel_init(struct
> intel_dsi *intel_dsi, u16 panel_id)
> >>  		exit_zero_cnt += 1;
> >>
> >>  	/* clk zero count */
> >> -	clk_zero_cnt = DIV_ROUND_UP(
> >> -			(tclk_prepare_clkzero -	ths_prepare_ns)
> >> -			* ui_den, 2 * ui_num);
> >> +	if (IS_GEMINILAKE(dev_priv))
> >> +		clk_zero_cnt = DIV_ROUND_UP(
> >> +				(tclk_prepare_clkzero -	ths_prepare_ns)
> >> +				* ui_den, 8 * ui_num);
> >> +	else
> >> +		clk_zero_cnt = DIV_ROUND_UP(
> >> +				(tclk_prepare_clkzero -	ths_prepare_ns)
> >> +				* ui_den, 2 * ui_num);
> >>
> >>  	/* trail count */
> >>  	tclk_trail_ns = max(mipi_config->tclk_trail, mipi_config->ths_trail);
> >> -	trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 2 * ui_num);
> >> +
> >> +	if (IS_GEMINILAKE(dev_priv))
> >> +		trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 8 *
> ui_num);
> >> +	else
> >> +		trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 2 *
> ui_num);
> >>
> >>  	if (prepare_cnt > PREPARE_CNT_MAX ||
> >>  		exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
> 
> --
> Jani Nikula, Intel Open Source Technology Center
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 8f683b8..8059cbb 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -695,16 +695,26 @@  struct drm_panel *vbt_panel_init(struct intel_dsi *intel_dsi, u16 panel_id)
 	/* count values in UI = (ns value) * (bitrate / (2 * 10^6))
 	 *
 	 * Since txddrclkhs_i is 2xUI, all the count values programmed in
-	 * DPHY param register are divided by 2
+	 * DPHY param register are divided by 2 except GEMINILAKE where it is
+	 * programmed in terms of HS byte clock so divided by 8
 	 *
 	 * prepare count
 	 */
 	ths_prepare_ns = max(mipi_config->ths_prepare,
 			     mipi_config->tclk_prepare);
-	prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num * 2);
+	if (IS_GEMINILAKE(dev_priv))
+		prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num * 8);
+	else
+		prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num * 2);
 
 	/* exit zero count */
-	exit_zero_cnt = DIV_ROUND_UP(
+	if (IS_GEMINILAKE(dev_priv))
+		exit_zero_cnt = DIV_ROUND_UP(
+				(ths_prepare_hszero - ths_prepare_ns) * ui_den,
+				ui_num * 8
+				);
+	else
+		exit_zero_cnt = DIV_ROUND_UP(
 				(ths_prepare_hszero - ths_prepare_ns) * ui_den,
 				ui_num * 2
 				);
@@ -719,13 +729,22 @@  struct drm_panel *vbt_panel_init(struct intel_dsi *intel_dsi, u16 panel_id)
 		exit_zero_cnt += 1;
 
 	/* clk zero count */
-	clk_zero_cnt = DIV_ROUND_UP(
-			(tclk_prepare_clkzero -	ths_prepare_ns)
-			* ui_den, 2 * ui_num);
+	if (IS_GEMINILAKE(dev_priv))
+		clk_zero_cnt = DIV_ROUND_UP(
+				(tclk_prepare_clkzero -	ths_prepare_ns)
+				* ui_den, 8 * ui_num);
+	else
+		clk_zero_cnt = DIV_ROUND_UP(
+				(tclk_prepare_clkzero -	ths_prepare_ns)
+				* ui_den, 2 * ui_num);
 
 	/* trail count */
 	tclk_trail_ns = max(mipi_config->tclk_trail, mipi_config->ths_trail);
-	trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 2 * ui_num);
+
+	if (IS_GEMINILAKE(dev_priv))
+		trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 8 * ui_num);
+	else
+		trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 2 * ui_num);
 
 	if (prepare_cnt > PREPARE_CNT_MAX ||
 		exit_zero_cnt > EXIT_ZERO_CNT_MAX ||