Change in coreboot[master]: dsi: Fix data rate

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38827 )

Change subject: dsi: Fix data rate
......................................................................

dsi: Fix data rate

Change-Id: I23220d446833b956431006027bbc8cb20fc696a5
---
M src/soc/mediatek/common/dsi.c
1 file changed, 5 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/38827/1

diff --git a/src/soc/mediatek/common/dsi.c b/src/soc/mediatek/common/dsi.c
index 238b1eb..618a2d4 100644
--- a/src/soc/mediatek/common/dsi.c
+++ b/src/soc/mediatek/common/dsi.c
@@ -48,9 +48,11 @@
     * for older platforms which do not have complete implementation in HFP.
     * Newer platforms should just set that to 1.0 (100 / 100).
     */
-	int data_rate = (u64)edid->mode.pixel_clock * bits_per_pixel *
-			MTK_DSI_MIPI_RATIO_NUMERATOR /
-			(1000 * lanes * MTK_DSI_MIPI_RATIO_DENOMINATOR);
+	int data_rate = DIV_ROUND_UP((u64)edid->mode.pixel_clock *
+				     bits_per_pixel *
+				     MTK_DSI_MIPI_RATIO_NUMERATOR,
+				     1000ULL * lanes *
+				     MTK_DSI_MIPI_RATIO_DENOMINATOR);
    printk(BIOS_INFO, "DSI data_rate: %d Mbps\n", data_rate);

if (data_rate < MTK_DSI_DATA_RATE_MIN_MHZ) {

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38827 )

Change subject: dsi: Fix data rate
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/c/coreboot/+/38827/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38827/1//COMMIT_MSG@7 
PS1, Line 7: dsi: Fix data rate
Please use the correct prefix.

https://review.coreboot.org/c/coreboot/+/38827/1//COMMIT_MSG@8 
PS1, Line 8: 
Please describe the problem and solution.

https://review.coreboot.org/c/coreboot/+/38827/1//COMMIT_MSG@9 
PS1, Line 9: Change-Id: I23220d446833b956431006027bbc8cb20fc696a5
The Signed-off-by line is missing.

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38827 )

Change subject: dsi: Fix data rate
......................................................................

Abandoned

Doesn't fix b/149051882

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has restored this change. ( https://review.coreboot.org/c/coreboot/+/38827 )

Change subject: dsi: Fix data rate
......................................................................

Restored

Change in coreboot[master]: soc/mediatek: dsi: Increase pcw precision

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38827 )

Change subject: soc/mediatek: dsi: Increase pcw precision
......................................................................

Patch Set 2:

This change is ready for review.

Change in coreboot[master]: soc/mediatek: dsi: Increase pcw precision

[Yu-Ping Wu (Code Review)]

Hello Julius Werner, jitao shi, Hung-Te Lin, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38827

to look at the new patch set (#3).

Change subject: soc/mediatek: dsi: Increase pcw precision
......................................................................

soc/mediatek: dsi: Increase pcw precision

When configuring MIPI DSI Tx, the value of pcw was calculated from data
rate in MHz, leading to loss of precision. This patch changes to use
data rate in Hz for the calculation so that the resulting value should
be consistent with the one in kernel (CL:1786327).

In addition, change the type of data rate to u32, and calculation of
data rate from pixel clock is changed to use DIV_ROUND_UP for
consistency with kernel (CL:1761843).

Also remove unused variable txdiv.

BRANCH=kukui
BUG=b:149051882
TEST=emerge-jacuzzi coreboot
TEST=No scrolling issue on Juniper AUO and InnoLux panels

Change-Id: I23220d446833b956431006027bbc8cb20fc696a5
Signed-off-by: Yu-Ping Wu 
---
M src/soc/mediatek/common/dsi.c
M src/soc/mediatek/common/include/soc/dsi_common.h
M src/soc/mediatek/mt8173/dsi.c
M src/soc/mediatek/mt8183/dsi.c
4 files changed, 38 insertions(+), 37 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/38827/3

Change in coreboot[master]: soc/mediatek: dsi: Increase pcw precision

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38827 )

Change subject: soc/mediatek: dsi: Increase pcw precision
......................................................................

Patch Set 3:

(6 comments)

https://review.coreboot.org/c/coreboot/+/38827/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38827/1//COMMIT_MSG@7 
PS1, Line 7: dsi: Fix data rate
> Please use the correct prefix.

Ack

https://review.coreboot.org/c/coreboot/+/38827/1//COMMIT_MSG@8 
PS1, Line 8:
> Please describe the problem and solution.

Ack

https://review.coreboot.org/c/coreboot/+/38827/1//COMMIT_MSG@9 
PS1, Line 9: Change-Id: I23220d446833b956431006027bbc8cb20fc696a5
> The Signed-off-by line is missing.

Ack

https://review.coreboot.org/c/coreboot/+/38827/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38827/2//COMMIT_MSG@12 
PS2, Line 12: CL
chromium

https://review.coreboot.org/c/coreboot/+/38827/2//COMMIT_MSG@16 
PS2, Line 16: CL
chromium

https://review.coreboot.org/c/coreboot/+/38827/3/src/soc/mediatek/common/dsi... 
File src/soc/mediatek/common/dsi.c:

https://review.coreboot.org/c/coreboot/+/38827/3/src/soc/mediatek/common/dsi... 
PS3, Line 67: -1;
return 0

Change in coreboot[master]: soc/mediatek: dsi: Increase pcw precision

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38827 )

Change subject: soc/mediatek: dsi: Increase pcw precision
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38827/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38827/2//COMMIT_MSG@12 
PS2, Line 12: CL
> chromium

never mind, just realized CL: is also chromium: on coreboot.

https://review.coreboot.org/c/coreboot/+/38827/2//COMMIT_MSG@16 
PS2, Line 16: CL
> chromium

Ack

Change in coreboot[master]: soc/mediatek: dsi: Increase pcw precision

[Yu-Ping Wu (Code Review)]

Hello Julius Werner, jitao shi, Hung-Te Lin, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38827

to look at the new patch set (#4).

Change subject: soc/mediatek: dsi: Increase pcw precision
......................................................................

soc/mediatek: dsi: Increase pcw precision

When configuring MIPI DSI Tx, the value of pcw was calculated from data
rate in MHz, leading to loss of precision. This patch changes to use
data rate in Hz for the calculation so that the resulting value should
be consistent with the one in kernel (CL:1786327).

In addition, change the type of data rate to u32, and calculation of
data rate from pixel clock is changed to use DIV_ROUND_UP for
consistency with kernel (CL:1761843).

Also remove unused variable txdiv.

BRANCH=kukui
BUG=b:149051882
TEST=emerge-jacuzzi coreboot
TEST=No scrolling issue on Juniper AUO and InnoLux panels

Change-Id: I23220d446833b956431006027bbc8cb20fc696a5
Signed-off-by: Yu-Ping Wu 
---
M src/soc/mediatek/common/dsi.c
M src/soc/mediatek/common/include/soc/dsi_common.h
M src/soc/mediatek/mt8173/dsi.c
M src/soc/mediatek/mt8183/dsi.c
4 files changed, 39 insertions(+), 38 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/38827/4

Change in coreboot[master]: soc/mediatek: dsi: Increase pcw precision

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38827 )

Change subject: soc/mediatek: dsi: Increase pcw precision
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38827/3/src/soc/mediatek/common/dsi... 
File src/soc/mediatek/common/dsi.c:

https://review.coreboot.org/c/coreboot/+/38827/3/src/soc/mediatek/common/dsi... 
PS3, Line 67: -1;
> return 0

Done

Change in coreboot[master]: soc/mediatek: dsi: Increase pcw precision

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38827 )

Change subject: soc/mediatek: dsi: Increase pcw precision
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: soc/mediatek: dsi: Increase pcw precision

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38827 )

Change subject: soc/mediatek: dsi: Increase pcw precision
......................................................................

soc/mediatek: dsi: Increase pcw precision

When configuring MIPI DSI Tx, the value of pcw was calculated from data
rate in MHz, leading to loss of precision. This patch changes to use
data rate in Hz for the calculation so that the resulting value should
be consistent with the one in kernel (CL:1786327).

In addition, change the type of data rate to u32, and calculation of
data rate from pixel clock is changed to use DIV_ROUND_UP for
consistency with kernel (CL:1761843).

Also remove unused variable txdiv.

BRANCH=kukui
BUG=b:149051882
TEST=emerge-jacuzzi coreboot
TEST=No scrolling issue on Juniper AUO and InnoLux panels

Change-Id: I23220d446833b956431006027bbc8cb20fc696a5
Signed-off-by: Yu-Ping Wu 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38827
Reviewed-by: Hung-Te Lin 
Tested-by: build bot (Jenkins) 
---
M src/soc/mediatek/common/dsi.c
M src/soc/mediatek/common/include/soc/dsi_common.h
M src/soc/mediatek/mt8173/dsi.c
M src/soc/mediatek/mt8183/dsi.c
4 files changed, 39 insertions(+), 38 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Hung-Te Lin: Looks good to me, approved

diff --git a/src/soc/mediatek/common/dsi.c b/src/soc/mediatek/common/dsi.c
index 238b1eb..9222cb0 100644
--- a/src/soc/mediatek/common/dsi.c
+++ b/src/soc/mediatek/common/dsi.c
@@ -39,29 +39,32 @@
    return 24;
 }

-static int mtk_dsi_get_data_rate(u32 bits_per_pixel, u32 lanes,
+static u32 mtk_dsi_get_data_rate(u32 bits_per_pixel, u32 lanes,
    			 const struct edid *edid)
 {
    /* data_rate = pixel_clock * bits_per_pixel * mipi_ratio / lanes
-	 * Note pixel_clock comes in kHz and returned data_rate is in Mbps.
+	 * Note pixel_clock comes in kHz and returned data_rate is in bps.
     * mipi_ratio is the clk coefficient to balance the pixel clk in MIPI
     * for older platforms which do not have complete implementation in HFP.
     * Newer platforms should just set that to 1.0 (100 / 100).
     */
-	int data_rate = (u64)edid->mode.pixel_clock * bits_per_pixel *
-			MTK_DSI_MIPI_RATIO_NUMERATOR /
-			(1000 * lanes * MTK_DSI_MIPI_RATIO_DENOMINATOR);
-	printk(BIOS_INFO, "DSI data_rate: %d Mbps\n", data_rate);
+	u32 data_rate = DIV_ROUND_UP((u64)edid->mode.pixel_clock *
+				     bits_per_pixel * 1000 *
+				     MTK_DSI_MIPI_RATIO_NUMERATOR,
+				     (u64)lanes *
+				     MTK_DSI_MIPI_RATIO_DENOMINATOR);
+	printk(BIOS_INFO, "DSI data_rate: %u bps\n", data_rate);

-	if (data_rate < MTK_DSI_DATA_RATE_MIN_MHZ) {
-		printk(BIOS_ERR, "data rate (%dMbps) must be >=%dMbps. "
-		       "Please check the pixel clock (%u), bits per pixel(%u), "
+	if (data_rate < MTK_DSI_DATA_RATE_MIN_MHZ * MHz) {
+		printk(BIOS_ERR, "data rate (%ubps) must be >= %ubps. "
+		       "Please check the pixel clock (%u), "
+		       "bits per pixel (%u), "
    	       "mipi_ratio (%d%%) and number of lanes (%d)\n",
-		       data_rate, MTK_DSI_DATA_RATE_MIN_MHZ,
+		       data_rate, MTK_DSI_DATA_RATE_MIN_MHZ * MHz,
    	       edid->mode.pixel_clock, bits_per_pixel,
    	       (100 * MTK_DSI_MIPI_RATIO_NUMERATOR /
    		MTK_DSI_MIPI_RATIO_DENOMINATOR), lanes);
-		return -1;
+		return 0;
    }
    return data_rate;
 }
@@ -71,12 +74,13 @@
    /* Do nothing. */
 }

-static void mtk_dsi_phy_timing(int data_rate, struct mtk_phy_timing *phy_timing)
+static void mtk_dsi_phy_timing(u32 data_rate, struct mtk_phy_timing *phy_timing)
 {
    u32 cycle_time, ui;
+	u32 data_rate_mhz = DIV_ROUND_UP(data_rate, MHz);

-	ui = 1000 / data_rate + 0x01;
-	cycle_time = 8000 / data_rate + 0x01;
+	ui = 1000 / data_rate_mhz + 0x01;
+	cycle_time = 8000 / data_rate_mhz + 0x01;

memset(phy_timing, 0, sizeof(*phy_timing));

@@ -401,11 +405,11 @@
 int mtk_dsi_init(u32 mode_flags, u32 format, u32 lanes, const struct edid *edid,
    	 const u8 *init_commands)
 {
-	int data_rate;
+	u32 data_rate;
    u32 bits_per_pixel = mtk_dsi_get_bits_per_pixel(format);

data_rate = mtk_dsi_get_data_rate(bits_per_pixel, lanes, edid);
-	if (data_rate < 0)
+	if (!data_rate)
    	return -1;

mtk_dsi_configure_mipi_tx(data_rate, lanes);
diff --git a/src/soc/mediatek/common/include/soc/dsi_common.h b/src/soc/mediatek/common/include/soc/dsi_common.h
index 3052689..25727b8 100644
--- a/src/soc/mediatek/common/include/soc/dsi_common.h
+++ b/src/soc/mediatek/common/include/soc/dsi_common.h
@@ -16,6 +16,7 @@
 #ifndef SOC_MEDIATEK_DSI_COMMON_H
 #define SOC_MEDIATEK_DSI_COMMON_H

+#include <commonlib/helpers.h>
 #include <edid.h>
 #include <types.h>
 #include <soc/addressmap.h>
@@ -358,7 +359,7 @@

/* Functions that each SOC should provide. */
 void mtk_dsi_reset(void);
-void mtk_dsi_configure_mipi_tx(int data_rate, u32 lanes);
+void mtk_dsi_configure_mipi_tx(u32 data_rate, u32 lanes);

/* Functions as weak no-ops that can be overridden. */
 void mtk_dsi_override_phy_timing(struct mtk_phy_timing *timing);
diff --git a/src/soc/mediatek/mt8173/dsi.c b/src/soc/mediatek/mt8173/dsi.c
index dae23f5..48bfbef 100644
--- a/src/soc/mediatek/mt8173/dsi.c
+++ b/src/soc/mediatek/mt8173/dsi.c
@@ -20,7 +20,7 @@
 #include <soc/dsi.h>
 #include <timer.h>

-void mtk_dsi_configure_mipi_tx(int data_rate, u32 lanes)
+void mtk_dsi_configure_mipi_tx(u32 data_rate, u32 lanes)
 {
    u32 txdiv0, txdiv1;
    u64 pcw;
@@ -51,21 +51,21 @@

clrbits32(&mipi_tx0->dsi_pll_con0, RG_DSI0_MPPLL_PLL_EN);

-	if (data_rate > 500) {
+	if (data_rate > 500 * MHz) {
    	txdiv0 = 0;
    	txdiv1 = 0;
-	} else if (data_rate >= 250) {
+	} else if (data_rate >= 250 * MHz) {
    	txdiv0 = 1;
    	txdiv1 = 0;
-	} else if (data_rate >= 125) {
+	} else if (data_rate >= 125 * MHz) {
    	txdiv0 = 2;
    	txdiv1 = 0;
-	} else if (data_rate >= 62) {
+	} else if (data_rate >= 62 * MHz) {
    	txdiv0 = 2;
    	txdiv1 = 1;
    } else {
    	/* MIN = 50 */
-		assert(data_rate >= MTK_DSI_DATA_RATE_MIN_MHZ);
+		assert(data_rate >= MTK_DSI_DATA_RATE_MIN_MHZ * MHz);
    	txdiv0 = 2;
    	txdiv1 = 2;
    }
@@ -83,7 +83,7 @@
     * Ref_clk is 26MHz
     */
    pcw = (u64)(data_rate * (1 << txdiv0) * (1 << txdiv1)) << 24;
-	pcw /= 13;
+	pcw /= 13 * MHz;
    write32(&mipi_tx0->dsi_pll_con2, pcw);

setbits32(&mipi_tx0->dsi_pll_con1, RG_DSI0_MPPLL_SDM_FRA_EN);
diff --git a/src/soc/mediatek/mt8183/dsi.c b/src/soc/mediatek/mt8183/dsi.c
index 7f5ac0a..3710fc65 100644
--- a/src/soc/mediatek/mt8183/dsi.c
+++ b/src/soc/mediatek/mt8183/dsi.c
@@ -19,32 +19,28 @@
 #include <soc/dsi.h>
 #include <soc/pll.h>

-void mtk_dsi_configure_mipi_tx(int data_rate, u32 lanes)
+void mtk_dsi_configure_mipi_tx(u32 data_rate, u32 lanes)
 {
-	unsigned int txdiv, txdiv0, txdiv1;
+	unsigned int txdiv0, txdiv1;
    u64 pcw;

-	if (data_rate >= 2000) {
-		txdiv = 1;
+	if (data_rate >= 2000 * MHz) {
    	txdiv0 = 0;
    	txdiv1 = 0;
-	} else if (data_rate >= 1000) {
-		txdiv = 2;
+	} else if (data_rate >= 1000 * MHz) {
    	txdiv0 = 1;
    	txdiv1 = 0;
-	} else if (data_rate >= 500) {
-		txdiv = 4;
+	} else if (data_rate >= 500 * MHz) {
    	txdiv0 = 2;
    	txdiv1 = 0;
-	} else if (data_rate > 250) {
-		/* Be aware that 250 is a special case that must use txdiv=4. */
-		txdiv = 8;
+	} else if (data_rate > 250 * MHz) {
+		/* (data_rate == 250MHz) is a special case that should go to the
+		   else-block below (txdiv0 = 4) */
    	txdiv0 = 3;
    	txdiv1 = 0;
    } else {
    	/* MIN = 125 */
-		assert(data_rate >= MTK_DSI_DATA_RATE_MIN_MHZ);
-		txdiv = 16;
+		assert(data_rate >= MTK_DSI_DATA_RATE_MIN_MHZ * MHz);
    	txdiv0 = 4;
    	txdiv1 = 0;
    }
@@ -56,7 +52,7 @@

pcw = (u64)data_rate * (1 << txdiv0) * (1 << txdiv1);
    pcw <<= 24;
-	pcw /= CLK26M_HZ / MHz;
+	pcw /= CLK26M_HZ;

write32(&mipi_tx->pll_con0, pcw);
    clrsetbits32(&mipi_tx->pll_con1, RG_DSI_PLL_POSDIV, txdiv0 << 8);