Received-SPF: pass (google.com: domain of linux-kernel+bounces-134909-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Jayesh Choudhary <j-choudhary@ti.com>
To: <dianders@chromium.org>, <andrzej.hajda@intel.com>,
        <neil.armstrong@linaro.org>, <rfoss@kernel.org>,
        <Laurent.pinchart@ideasonboard.com>, <mripard@kernel.org>,
        <j-choudhary@ti.com>
CC: <jonas@kwiboo.se>, <jernej.skrabec@gmail.com>,
        <maarten.lankhorst@linux.intel.com>, <tzimmermann@suse.de>,
        <airlied@gmail.com>, <daniel@ffwll.ch>, <andersson@kernel.org>,
        <robdclark@gmail.com>, <dri-devel@lists.freedesktop.org>,
        <linux-kernel@vger.kernel.org>
Subject: [PATCH] drm/bridge: ti-sn65dsi86: Fix ti_sn_bridge_set_dsi_rate function
Date: Mon, 8 Apr 2024 13:06:23 +0530
Message-ID: <20240408073623.186489-1-j-choudhary@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain

Due to integer calculations, the rounding off can cause errors in the final
value propagated in the registers.
Considering the example of 1080p (very common resolution), the mode->clock
is 148500, dsi->lanes = 4, and bpp = 24, with the previous logic, the DSI
clock frequency would come as 444 when we are expecting the value 445.5
which would reflect in SN_DSIA_CLK_FREQ_REG.
So move the division to be the last operation where rounding off will not
impact the register value.

Also according to the SN65DSI86 datasheet[0], the minimum value for that
reg is 0x08 (inclusive) and the maximum value is 0x97 (exclusive). So add
check for that.

[0]: <https://www.ti.com/lit/gpn/sn65dsi86>

Fixes: ca1b885cbe9e ("drm/bridge: ti-sn65dsi86: Split the setting of the dp and dsi rates")
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 48 +++++++++++++++++++++------
 1 file changed, 37 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 84698a0b27a8..f9cf6b14d85e 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -111,7 +111,14 @@
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
 
-#define MIN_DSI_CLK_FREQ_MHZ	40
+/*
+ * NOTE: DSI clock frequency range: [40MHz,755MHz)
+ * DSI clock frequency range is in 5-MHz increments
+ * So minimum frequency 40MHz translates to 0x08
+ * And maximum frequency 755MHz translates to 0x97
+ */
+#define MIN_DSI_CLK_RANGE	0x8
+#define MAX_DSI_CLK_RANGE	0x97
 
 /* fudge factor required to account for 8b/10b encoding */
 #define DP_CLK_FUDGE_NUM	10
@@ -820,22 +827,37 @@ static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 0);
 }
 
-static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
+static int ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
 {
-	unsigned int bit_rate_mhz, clk_freq_mhz;
+	unsigned int bit_rate_khz;
 	unsigned int val;
 	struct drm_display_mode *mode =
 		&pdata->bridge.encoder->crtc->state->adjusted_mode;
 
-	/* set DSIA clk frequency */
-	bit_rate_mhz = (mode->clock / 1000) *
-			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
-	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
+	/*
+	 * Set DSIA clk frequency
+	 * Maximum supported value of bit_rate_khz turns out to be
+	 * 6040000 which can be put in 32-bit variable so no overflow
+	 * possible in this calculation.
+	 */
+	bit_rate_khz = mode->clock *
+		       mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
+
+	/*
+	 * For each increment in val, frequency increases by 5MHz
+	 * and the factor of 1000 comes from kHz to MHz conversion
+	 */
+	val = (bit_rate_khz / (pdata->dsi->lanes * 2 * 1000 * 5)) & 0xFF;
+
+	if (val >= MAX_DSI_CLK_RANGE || val < MIN_DSI_CLK_RANGE) {
+		drm_err(pdata->bridge.dev,
+			"DSI clock frequency not in the supported range\n");
+		return -EINVAL;
+	}
 
-	/* for each increment in val, frequency increases by 5MHz */
-	val = (MIN_DSI_CLK_FREQ_MHZ / 5) +
-		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
 	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
+
+	return 0;
 }
 
 static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
@@ -1104,7 +1126,11 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
 			   pdata->ln_polrs << LN_POLRS_OFFSET);
 
 	/* set dsi clk frequency value */
-	ti_sn_bridge_set_dsi_rate(pdata);
+	ret = ti_sn_bridge_set_dsi_rate(pdata);
+	if (ret) {
+		DRM_DEV_ERROR(pdata->dev, "Failed to set dsi rate :%d\n", ret);
+		return;
+	}
 
 	/*
 	 * The SN65DSI86 only supports ASSR Display Authentication method and
-- 
2.25.1