[Intel-gfx] [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver

[Damien Lespiau]

On Mon, Apr 14, 2014 at 11:18:27AM +0530, Shobhit Kumar wrote:
> +#define NS_MHZ_RATIO 1000000

[...]

> +static bool generic_init(struct intel_dsi_device *dsi)
> +{

[...]

> +	/*
> +	 * ui(s) = 1/f [f in hz]
> +	 * ui(ns) = 10^9/f*10^6 [f in Mhz] -> 10^3/f(Mhz)

ui(ns) = 10^9/(f*10^6)

> +	 *
> +	 * LP byte clock = TLPX/8ui

Mind putting that comment just above the appropriate computation?
Also, LP byte clock = Tlpx / (8UI)

> +	 *
> +	 * Since txddrclkhs_i is 2xUI, the count values programmed in
> +	 * DPHY param registers are divided by 2

That looks like a general comment that apply to a bunch of calculations
below, probably worth separating it from the UI comment.

> +	 *
> +	 */
> +
> +	/* in Kbps */
> +	ui_num = bitrate;
> +	ui_den = NS_MHZ_RATIO;

I'm a bit confused here, most likely missing something, care to clarify?

- IIUC, you want the computations to happen in ns. I'm a bit puzzled by
  that NS_MHZ_RATIO constant name when we're dealing with Kbps.

  That constant is 10^6 which seems to be OK for KHz. So maybe just a
  naming problem?

- UI is a period, so is homogeneous to time (s), but ui_num being in
  s^-1 and ui_den a constant, ui_num/ui_den looks like a frequency. Or
  could it be that UI = ui_den / ui_num? would be confusing, but the
  code below would make more sense. In which case could we have UI =
  ui_num / ui_den?

> +
> +	tclk_prepare_clkzero = mipi_config->tclk_prepare_clkzero;
> +	ths_prepare_hszero = mipi_config->ths_prepare_hszero;
> +
> +	/* B060 */
> +	intel_dsi->lp_byte_clk = CEIL_DIV(tlpx_ns * ui_num, 8 * ui_den);
> +
> +	/* count values in UI = (ns value) * (bitrate / (2 * 10^6)) */
> +	/* prepare count */
> +	ths_prepare_ns =
> +		(mipi_config->ths_prepare >  mipi_config->tclk_prepare) ?
> +				mipi_config->ths_prepare :
> +				mipi_config->tclk_prepare;

That looks like max()

> +
> +	prepare_cnt = CEIL_DIV(ths_prepare_ns * ui_num,	ui_den * 2);

The formula above has a 10^6, why is that OK not to have it there? (in
which unit is bitrate in the formula? MHz?) Is this something like:

  Count in UI = count(ns) / UI(ns)
  
and then as UI = ui_den/ui_num (?!) we end up with:

  Count in UI = count(ns) * ui_num / ui_den

And then the / 2 comment applies.

> +
> +	/* exit zero count */
> +	exit_zero_cnt = CEIL_DIV(
> +				(ths_prepare_hszero - ths_prepare_ns) * ui_num,
> +				ui_den * 2
> +				);
> +
> +	/*
> +	 * Exit zero  is unified val ths_zero and ths_exit
> +	 * minimum value for ths_exit = 110ns
> +	 * min (exit_zero_cnt * 2) = 110/UI
> +	 * exit_zero_cnt = 55/UI
> +	 */
> +	 if (exit_zero_cnt < (55 * ui_num / ui_den))
> +		if ((55 * ui_num) % ui_den)
> +			exit_zero_cnt += 1;

I'm not sure what we're achieving with the +=1 here, mind explaining?

> +
> +	/* clk zero count */
> +	clk_zero_cnt = CEIL_DIV(
> +			(tclk_prepare_clkzero -	ths_prepare_ns)
> +			* ui_num, 2 * ui_den);
> +
> +	/* trail count */
> +	tclk_trail_ns = (mipi_config->tclk_trail > mipi_config->ths_trail) ?
> +			mipi_config->tclk_trail : mipi_config->ths_trail;

max() 

> +	trail_cnt = CEIL_DIV(tclk_trail_ns * ui_num, 2 * ui_den);
> +
> +	if (prepare_cnt > PREPARE_CNT_MAX ||
> +		exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
> +		clk_zero_cnt > CLK_ZERO_CNT_MAX ||
> +		trail_cnt > TRAIL_CNT_MAX)
> +		DRM_DEBUG_DRIVER("Values crossing maximum limits\n");

Is that a situation that may happen in a normal case? or should we go
with a DRM_ERROR() here and potentially abort the modeset?

> +
> +	if (prepare_cnt > PREPARE_CNT_MAX)
> +		prepare_cnt = PREPARE_CNT_MAX;
> +
> +	if (exit_zero_cnt > EXIT_ZERO_CNT_MAX)
> +		exit_zero_cnt = EXIT_ZERO_CNT_MAX;
> +
> +	if (clk_zero_cnt > CLK_ZERO_CNT_MAX)
> +		clk_zero_cnt = CLK_ZERO_CNT_MAX;
> +
> +	if (trail_cnt > TRAIL_CNT_MAX)
> +		trail_cnt = TRAIL_CNT_MAX;
> +
> +	/* B080 */
> +	intel_dsi->dphy_reg = exit_zero_cnt << 24 | trail_cnt << 16 |
> +						clk_zero_cnt << 8 | prepare_cnt;
> +
> +	/*
> +	 * LP to HS switch count = 4TLPX + PREP_COUNT * 2 + EXIT_ZERO_COUNT * 2
> +	 *					+ 10UI + Extra Byte Count
> +	 *
> +	 * HS to LP switch count = THS-TRAIL + 2TLPX + Extra Byte Count
> +	 * Extra Byte Count is calculated according to number of lanes.
> +	 * High Low Switch Count is the Max of LP to HS and
> +	 * HS to LP switch count
> +	 *
> +	 */
> +	tlpx_ui = CEIL_DIV(tlpx_ns * ui_num, ui_den);
> +
> +	/* B044 */
> +	intel_dsi->hs_to_lp_count =
> +		CEIL_DIV(
> +			4 * tlpx_ui + prepare_cnt * 2 +
> +			exit_zero_cnt * 2 + 10,
> +			8);

The previous was before I tried to look at the spec too closely. Mind
explaining why we don't look at the HS to LP switch count? ie why HS to
LP switch cound is always smaller than the LP to HS one?

> +
> +	intel_dsi->hs_to_lp_count += extra_byte_count;
> +
> +	/* B088 */
> +	/* LP -> HS for clock lanes
> +	 * LP clk sync + LP11 + LP01 + tclk_prepare + tclk_zero +
> +	 *						extra byte count
> +	 * 2TPLX + 1TLPX + 1 TPLX(in ns) + prepare_cnt * 2 + clk_zero_cnt *
> +	 *					2(in UI) + extra byte count
> +	 * In byteclks = (4TLPX + prepare_cnt * 2 + clk_zero_cnt *2 (in UI)) /
> +	 *					8 + extra byte count
> +	 */
> +	intel_dsi->clk_lp_to_hs_count =
> +		CEIL_DIV(
> +			4 * tlpx_ui + prepare_cnt * 2 +
> +			clk_zero_cnt * 2,
> +			8);
> +
> +	intel_dsi->clk_lp_to_hs_count += extra_byte_count;
> +
> +	/* HS->LP for Clock Lanes
> +	 * Low Power clock synchronisations + 1Tx byteclk + tclk_trail +
> +	 *						Extra byte count
> +	 * 2TLPX + 8UI + (trail_count*2)(in UI) + Extra byte count
> +	 * In byteclks = (2*TLpx(in UI) + trail_count*2 +8)(in UI)/8 +
> +	 *						Extra byte count
> +	 */
> +	intel_dsi->clk_hs_to_lp_count =
> +		CEIL_DIV(2 * tlpx_ui + trail_cnt * 2 + 8,
> +			8);
> +	intel_dsi->clk_hs_to_lp_count += extra_byte_count;
> +
> +	DRM_DEBUG_KMS("EOT %s\n", intel_dsi->eotp_pkt ? "ENABLED" : "DISABLED");
> +	DRM_DEBUG_KMS("CLOCKSTOP %s\n", intel_dsi->clock_stop ?
> +						"ENABLED" : "DISABLED");
> +	DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "COMMAND" : "VIDEO");
> +	DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format);
> +	DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div);
> +	DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout);
> +	DRM_DEBUG_KMS("Turnaround Timeout 0x%x\n", intel_dsi->turn_arnd_val);
> +	DRM_DEBUG_KMS("Init Count 0x%x\n", intel_dsi->init_count);
> +	DRM_DEBUG_KMS("HS to LP Count 0x%x\n", intel_dsi->hs_to_lp_count);
> +	DRM_DEBUG_KMS("LP Byte Clock %d\n", intel_dsi->lp_byte_clk);
> +	DRM_DEBUG_KMS("DBI BW Timer 0x%x\n", intel_dsi->bw_timer);
> +	DRM_DEBUG_KMS("LP to HS Clock Count 0x%x\n", intel_dsi->clk_lp_to_hs_count);
> +	DRM_DEBUG_KMS("HS to LP Clock Count 0x%x\n", intel_dsi->clk_hs_to_lp_count);
> +	DRM_DEBUG_KMS("BTA %s\n",
> +			intel_dsi->video_frmt_cfg_bits & DISABLE_VIDEO_BTA ?
> +			"DISABLED" : "ENABLED");
> +	DRM_DEBUG_KMS("B060 = 0x%Xx, B080 = 0x%x, B044 = 0x%x, B088 = 0x%x\n",
> +			intel_dsi->lp_byte_clk, intel_dsi->dphy_reg, intel_dsi->hs_to_lp_count,
> +			(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));
> +
> +	/* delays in VBT are in unit of 100us, so need to convert
> +	 * here in ms
> +	 * Delay (100us) * 100 /1000 = Delay / 10 (ms) */
> +	intel_dsi->backlight_off_delay = pps->bl_disable_delay / 10;
> +	intel_dsi->backlight_on_delay = pps->bl_enable_delay / 10;
> +	intel_dsi->panel_on_delay = pps->panel_on_delay / 10;
> +	intel_dsi->panel_off_delay = pps->panel_off_delay / 10;
> +	intel_dsi->panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 10;
> +
> +	return true;
> +}