[PATCH v2 00/18] drm/bridge: cdns-dsi: Make it work a bit better

[PATCH v2 00/18] drm/bridge: cdns-dsi: Make it work a bit better

[Tomi Valkeinen]

While trying to get the cdns-dsi to work on Toradex's AM69 Aquila
platform, I hit multiple issues in the driver. Basicaly nothing worked
for with the board.

This series fixes those issues. While I itch to make much larger changes
to the cdns-dsi driver, I opted to keep this series relatively simple to
make the fixes more clear and possibly help with backporting.

The series also touches tidss, but those changes are not strictly
needed, and can be merged separately. And the series also touches
cdns-dphy, and those changes are needed.

This has been tested on Toradex AM69 Aquila, with:
- HDMI output using lontium lt8912b
- LVDS panel (sn65dsi84 + panel-lvds)

 Tomi

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Changes in v2:
- Change the tidss clock adjustment from mode_fixup() to atomic_check()
- Link to v1: https://lore.kernel.org/r/20250320-cdns-dsi-impro-v1-0-725277c5f43b@ideasonboard.com

---
Tomi Valkeinen (18):
      drm/tidss: Fix missing includes and struct decls
      drm/tidss: Use the crtc_* timings when programming the HW
      drm/tidss: Adjust the pclk based on the HW capabilities
      phy: cdns-dphy: Store hs_clk_rate and return it
      phy: cdns-dphy: Remove leftover code
      drm/bridge: cdns-dsi: Adjust mode to negative syncs
      drm/bridge: cdns-dsi: Fail if HS rate changed when validating PHY config
      drm/bridge: cdns-dsi: Clean up cdns_dsi_mode2cfg()
      drm/bridge: cdns-dsi: Fix REG_WAKEUP_TIME value
      drm/bridge: cdns-dsi: Fix event mode
      drm/bridge: cdns-dsi: Remove broken fifo emptying check
      drm/bridge: cdns-dsi: Drop checks that shouldn't be in .mode_valid()
      drm/bridge: cdns-dsi: Do not use crtc_* values
      drm/bridge: cdns-dsi: Use videomode internally
      drm/bridge: cdns-dsi: Tune adjusted_mode->clock according to dsi needs
      drm/bridge: cdns-dsi: Update htotal in cdns_dsi_mode2cfg()
      drm/bridge: cdns-dsi: Drop cdns_dsi_adjust_phy_config()
      drm/bridge: cdns-dsi: Don't fail on MIPI_DSI_MODE_VIDEO_BURST

 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 224 +++++++++++--------------
 drivers/gpu/drm/tidss/tidss_crtc.c             |  25 ++-
 drivers/gpu/drm/tidss/tidss_dispc.c            |  22 ++-
 drivers/gpu/drm/tidss/tidss_dispc.h            |   5 +
 drivers/gpu/drm/tidss/tidss_drv.h              |   2 +
 drivers/gpu/drm/tidss/tidss_plane.h            |   2 +
 drivers/gpu/drm/tidss/tidss_scale_coefs.h      |   2 +
 drivers/phy/cadence/cdns-dphy.c                |  24 +--
 8 files changed, 155 insertions(+), 151 deletions(-)
---
base-commit: 10646ddac2917b31c985ceff0e4982c42a9c924b
change-id: 20250320-cdns-dsi-impro-3d8fbd7848d1
prerequisite-message-id: 20250226155228.564289-1-aradhya.bhatia@linux.dev
prerequisite-patch-id: 46845a8d15042dd343a29a17fc0b9d0eec2605f5
prerequisite-patch-id: 7ce82c26cb9e18884492d2282a72ff2a760aefda
prerequisite-patch-id: ec2071425cab81da72e0805ad92fc52731d7a24d
prerequisite-patch-id: 32cde02288e0c36ed687f67778937a61f78b2d90
prerequisite-patch-id: 5f302e2bead8994763699a909ad0b5501f09ed9f
prerequisite-patch-id: 30611df6ef38c7872107d6bf6dde4504d46ab224
prerequisite-patch-id: 99831bcaa13e25b957d83a6320f34bcec223b939
prerequisite-patch-id: b0ad38bc6b323ceea7a1d2266b0fab8deaa6b05e
prerequisite-patch-id: 38dbce2b9302a764be9dbdc551578f02d797dfcc
prerequisite-patch-id: 133f8b1dab4f47d429b1924df981564ce3736233
prerequisite-patch-id: 879c546693a53e4b72c1ee25954c894ae57a441f
prerequisite-patch-id: 3e7edc818ac078a138f0e42e3f47fd685fffb84f
prerequisite-patch-id: 673b9f0b1936b5a49973b71cab5d13774583de38

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 01/18] drm/tidss: Fix missing includes and struct decls

[Tomi Valkeinen]

Fix missing includes and struct declarations. Even if these don't cause
any compile issues at the moment, it's good to have them correct.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.h       | 3 +++
 drivers/gpu/drm/tidss/tidss_drv.h         | 2 ++
 drivers/gpu/drm/tidss/tidss_plane.h       | 2 ++
 drivers/gpu/drm/tidss/tidss_scale_coefs.h | 2 ++
 4 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index 086327d51a90..c31b477a18b0 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -7,11 +7,14 @@
 #ifndef __TIDSS_DISPC_H__
 #define __TIDSS_DISPC_H__
 
+#include <drm/drm_color_mgmt.h>
+
 #include "tidss_drv.h"
 
 struct dispc_device;
 
 struct drm_crtc_state;
+struct drm_plane_state;
 
 enum tidss_gamma_type { TIDSS_GAMMA_8BIT, TIDSS_GAMMA_10BIT };
 
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
index 7f4f4282bc04..56a2020e20d0 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -9,6 +9,8 @@
 
 #include <linux/spinlock.h>
 
+#include <drm/drm_device.h>
+
 #define TIDSS_MAX_PORTS 4
 #define TIDSS_MAX_PLANES 4
 
diff --git a/drivers/gpu/drm/tidss/tidss_plane.h b/drivers/gpu/drm/tidss/tidss_plane.h
index aecaf2728406..92c560c3a621 100644
--- a/drivers/gpu/drm/tidss/tidss_plane.h
+++ b/drivers/gpu/drm/tidss/tidss_plane.h
@@ -7,6 +7,8 @@
 #ifndef __TIDSS_PLANE_H__
 #define __TIDSS_PLANE_H__
 
+#include <drm/drm_plane.h>
+
 #define to_tidss_plane(p) container_of((p), struct tidss_plane, plane)
 
 struct tidss_device;
diff --git a/drivers/gpu/drm/tidss/tidss_scale_coefs.h b/drivers/gpu/drm/tidss/tidss_scale_coefs.h
index 9c560d0fdac0..9824d02d9d1f 100644
--- a/drivers/gpu/drm/tidss/tidss_scale_coefs.h
+++ b/drivers/gpu/drm/tidss/tidss_scale_coefs.h
@@ -9,6 +9,8 @@
 
 #include <linux/types.h>
 
+struct device;
+
 struct tidss_scale_coefs {
 	s16 c2[16];
 	s16 c1[16];

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 02/18] drm/tidss: Use the crtc_* timings when programming the HW

[Tomi Valkeinen]

Use the crtc_* fields from drm_display_mode, instead of the "logical"
fields. This shouldn't change anything in practice, but afaiu the crtc_*
fields are the correct ones to use here.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_crtc.c  |  2 +-
 drivers/gpu/drm/tidss/tidss_dispc.c | 16 ++++++++--------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index 94f8e3178df5..1604eca265ef 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -225,7 +225,7 @@ static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
 	tidss_runtime_get(tidss);
 
 	r = dispc_vp_set_clk_rate(tidss->dispc, tcrtc->hw_videoport,
-				  mode->clock * 1000);
+				  mode->crtc_clock * 1000);
 	if (r != 0)
 		return;
 
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index cacb5f3d8085..a5107f2732b1 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1084,13 +1084,13 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
 
 	dispc_set_num_datalines(dispc, hw_videoport, fmt->data_width);
 
-	hfp = mode->hsync_start - mode->hdisplay;
-	hsw = mode->hsync_end - mode->hsync_start;
-	hbp = mode->htotal - mode->hsync_end;
+	hfp = mode->crtc_hsync_start - mode->crtc_hdisplay;
+	hsw = mode->crtc_hsync_end - mode->crtc_hsync_start;
+	hbp = mode->crtc_htotal - mode->crtc_hsync_end;
 
-	vfp = mode->vsync_start - mode->vdisplay;
-	vsw = mode->vsync_end - mode->vsync_start;
-	vbp = mode->vtotal - mode->vsync_end;
+	vfp = mode->crtc_vsync_start - mode->crtc_vdisplay;
+	vsw = mode->crtc_vsync_end - mode->crtc_vsync_start;
+	vbp = mode->crtc_vtotal - mode->crtc_vsync_end;
 
 	dispc_vp_write(dispc, hw_videoport, DISPC_VP_TIMING_H,
 		       FLD_VAL(hsw - 1, 7, 0) |
@@ -1132,8 +1132,8 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
 		       FLD_VAL(ivs, 12, 12));
 
 	dispc_vp_write(dispc, hw_videoport, DISPC_VP_SIZE_SCREEN,
-		       FLD_VAL(mode->hdisplay - 1, 11, 0) |
-		       FLD_VAL(mode->vdisplay - 1, 27, 16));
+		       FLD_VAL(mode->crtc_hdisplay - 1, 11, 0) |
+		       FLD_VAL(mode->crtc_vdisplay - 1, 27, 16));
 
 	VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1, 0, 0);
 }

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 03/18] drm/tidss: Adjust the pclk based on the HW capabilities

[Tomi Valkeinen]

At the moment the driver just sets the clock rate with clk_set_rate(),
and if the resulting rate is not the same as requested, prints a debug
print, but nothing else.

Add functionality to atomic_check(), in which the clk_round_rate() is
used to get the "rounded" rate, and set that to the adjusted_mode.

In practice, with the current K3 SoCs, the display PLL is capable of
producing very exact clocks, so most likely the rounded rate is the same
as the original one.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_crtc.c  | 23 +++++++++++++++++++----
 drivers/gpu/drm/tidss/tidss_dispc.c |  6 ++++++
 drivers/gpu/drm/tidss/tidss_dispc.h |  2 ++
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index 1604eca265ef..6c3967f70510 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -91,7 +91,7 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
 	struct dispc_device *dispc = tidss->dispc;
 	struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
 	u32 hw_videoport = tcrtc->hw_videoport;
-	const struct drm_display_mode *mode;
+	struct drm_display_mode *adjusted_mode;
 	enum drm_mode_status ok;
 
 	dev_dbg(ddev->dev, "%s\n", __func__);
@@ -99,12 +99,27 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
 	if (!crtc_state->enable)
 		return 0;
 
-	mode = &crtc_state->adjusted_mode;
+	adjusted_mode = &crtc_state->adjusted_mode;
 
-	ok = dispc_vp_mode_valid(dispc, hw_videoport, mode);
+	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
+		long rate;
+
+		rate = dispc_vp_round_clk_rate(tidss->dispc,
+					       tcrtc->hw_videoport,
+					       adjusted_mode->clock * 1000);
+		if (rate < 0)
+			return -EINVAL;
+
+		adjusted_mode->clock = rate / 1000;
+
+		drm_mode_set_crtcinfo(adjusted_mode, 0);
+	}
+
+	ok = dispc_vp_mode_valid(dispc, hw_videoport, adjusted_mode);
 	if (ok != MODE_OK) {
 		dev_dbg(ddev->dev, "%s: bad mode: %ux%u pclk %u kHz\n",
-			__func__, mode->hdisplay, mode->vdisplay, mode->clock);
+			__func__, adjusted_mode->hdisplay,
+			adjusted_mode->vdisplay, adjusted_mode->clock);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index a5107f2732b1..3930fb7f03c2 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1318,6 +1318,12 @@ unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate)
 	return (unsigned int)(abs(((rr - r) * 100) / r));
 }
 
+long dispc_vp_round_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
+			     unsigned long rate)
+{
+	return clk_round_rate(dispc->vp_clk[hw_videoport], rate);
+}
+
 int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
 			  unsigned long rate)
 {
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index c31b477a18b0..d4c335e918fb 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -120,6 +120,8 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
 					 const struct drm_display_mode *mode);
 int dispc_vp_enable_clk(struct dispc_device *dispc, u32 hw_videoport);
 void dispc_vp_disable_clk(struct dispc_device *dispc, u32 hw_videoport);
+long dispc_vp_round_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
+			     unsigned long rate);
 int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
 			  unsigned long rate);
 void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 04/18] phy: cdns-dphy: Store hs_clk_rate and return it

[Tomi Valkeinen]

The DPHY driver does not return the actual hs_clk_rate, so the DSI
driver has no idea what clock was actually achieved. Set the realized
hs_clk_rate to the opts struct, so that the DSI driver gets it back.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/phy/cadence/cdns-dphy.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
index ed87a3970f83..f79ec4fab409 100644
--- a/drivers/phy/cadence/cdns-dphy.c
+++ b/drivers/phy/cadence/cdns-dphy.c
@@ -79,6 +79,7 @@ struct cdns_dphy_cfg {
 	u8 pll_ipdiv;
 	u8 pll_opdiv;
 	u16 pll_fbdiv;
+	u32 hs_clk_rate;
 	unsigned int nlanes;
 };
 
@@ -154,6 +155,9 @@ static int cdns_dsi_get_dphy_pll_cfg(struct cdns_dphy *dphy,
 					  cfg->pll_ipdiv,
 					  pll_ref_hz);
 
+	cfg->hs_clk_rate = div_u64((u64)pll_ref_hz * cfg->pll_fbdiv,
+				   2 * cfg->pll_opdiv * cfg->pll_ipdiv);
+
 	return 0;
 }
 
@@ -297,6 +301,7 @@ static int cdns_dphy_config_from_opts(struct phy *phy,
 	if (ret)
 		return ret;
 
+	opts->hs_clk_rate = cfg->hs_clk_rate;
 	opts->wakeup = cdns_dphy_get_wakeup_time_ns(dphy) / 1000;
 
 	return 0;

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 05/18] phy: cdns-dphy: Remove leftover code

[Tomi Valkeinen]

The code in cdns-dphy has probably been part of a DSI driver in the
past. Remove DSI defines and variables which are not used or do not
actually do anything. Also rename cdns_dsi_get_dphy_pll_cfg() to
cdns_get_dphy_pll_cfg(), i.e. drop the "dsi", as it's not relevant here.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/phy/cadence/cdns-dphy.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
index f79ec4fab409..7f8b70ec10c5 100644
--- a/drivers/phy/cadence/cdns-dphy.c
+++ b/drivers/phy/cadence/cdns-dphy.c
@@ -55,14 +55,6 @@
 #define DPHY_PSM_CFG_FROM_REG		BIT(0)
 #define DPHY_PSM_CLK_DIV(x)		((x) << 1)
 
-#define DSI_HBP_FRAME_OVERHEAD		12
-#define DSI_HSA_FRAME_OVERHEAD		14
-#define DSI_HFP_FRAME_OVERHEAD		6
-#define DSI_HSS_VSS_VSE_FRAME_OVERHEAD	4
-#define DSI_BLANKING_FRAME_OVERHEAD	6
-#define DSI_NULL_FRAME_OVERHEAD		6
-#define DSI_EOT_PKT_SIZE		4
-
 #define DPHY_TX_J721E_WIZ_PLL_CTRL	0xF04
 #define DPHY_TX_J721E_WIZ_STATUS	0xF08
 #define DPHY_TX_J721E_WIZ_RST_CTRL	0xF0C
@@ -117,10 +109,9 @@ static const unsigned int tx_bands[] = {
 	870, 950, 1000, 1200, 1400, 1600, 1800, 2000, 2200, 2500
 };
 
-static int cdns_dsi_get_dphy_pll_cfg(struct cdns_dphy *dphy,
-				     struct cdns_dphy_cfg *cfg,
-				     struct phy_configure_opts_mipi_dphy *opts,
-				     unsigned int *dsi_hfp_ext)
+static int cdns_get_dphy_pll_cfg(struct cdns_dphy *dphy,
+				 struct cdns_dphy_cfg *cfg,
+				 struct phy_configure_opts_mipi_dphy *opts)
 {
 	unsigned long pll_ref_hz = clk_get_rate(dphy->pll_ref_clk);
 	u64 dlane_bps;
@@ -289,15 +280,13 @@ static int cdns_dphy_config_from_opts(struct phy *phy,
 				      struct cdns_dphy_cfg *cfg)
 {
 	struct cdns_dphy *dphy = phy_get_drvdata(phy);
-	unsigned int dsi_hfp_ext = 0;
 	int ret;
 
 	ret = phy_mipi_dphy_config_validate(opts);
 	if (ret)
 		return ret;
 
-	ret = cdns_dsi_get_dphy_pll_cfg(dphy, cfg,
-					opts, &dsi_hfp_ext);
+	ret = cdns_get_dphy_pll_cfg(dphy, cfg, opts);
 	if (ret)
 		return ret;
 

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 06/18] drm/bridge: cdns-dsi: Adjust mode to negative syncs

[Tomi Valkeinen]

The Cadence DSI requires negative syncs from the incoming video signal,
but at the moment that requirement is not expressed in any way. If the
crtc decides to use positive syncs, things break down.

Use the adjusted_mode in atomic_check to set the sync flags to negative
ones.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 8a320bd4d34d..53322407c1b0 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -992,6 +992,11 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 	struct cdns_dsi_bridge_state *dsi_state = to_cdns_dsi_bridge_state(bridge_state);
 	const struct drm_display_mode *mode = &crtc_state->mode;
 	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
+	struct drm_display_mode *adjusted_crtc_mode = &crtc_state->adjusted_mode;
+
+	/* cdns-dsi requires negative syncs */
+	adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
+	adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
 
 	return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
 }

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 07/18] drm/bridge: cdns-dsi: Fail if HS rate changed when validating PHY config

[Tomi Valkeinen]

The phy_validate() can change the HS clock rate we passed to it in the
PHY config, depending on what the HW can actually do. The driver just
ignores this at the moment, but if the actual HS clock rate is different
than the requested one, the pipeline will fail as all the DSI timing
calculations will be incorrect.

There are ways to improve DSI operation for various clock rates, but for
now, just add a check to see if the rate changed, and return an error if
that happens.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 53322407c1b0..9238acf69823 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -580,6 +580,7 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 	unsigned long dsi_hss_hsa_hse_hbp;
 	unsigned int nlanes = output->dev->lanes;
 	int mode_clock = (mode_valid_check ? mode->clock : mode->crtc_clock);
+	unsigned long req_hs_clk_rate;
 	int ret;
 
 	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
@@ -596,10 +597,20 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 	if (ret)
 		return ret;
 
+	req_hs_clk_rate = output->phy_opts.mipi_dphy.hs_clk_rate;
 	ret = phy_validate(dsi->dphy, PHY_MODE_MIPI_DPHY, 0, &output->phy_opts);
 	if (ret)
 		return ret;
 
+	if (req_hs_clk_rate != output->phy_opts.mipi_dphy.hs_clk_rate) {
+		dev_err(&dsi->dphy->dev,
+			"validation changed hs_clk_rate from %lu to %lu, diff %lu\n",
+			req_hs_clk_rate, output->phy_opts.mipi_dphy.hs_clk_rate,
+			output->phy_opts.mipi_dphy.hs_clk_rate -
+				req_hs_clk_rate);
+		return -EINVAL;
+	}
+
 	dsi_hss_hsa_hse_hbp = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
 	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
 		dsi_hss_hsa_hse_hbp += dsi_cfg->hsa + DSI_HSA_FRAME_OVERHEAD;

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 08/18] drm/bridge: cdns-dsi: Clean up cdns_dsi_mode2cfg()

[Tomi Valkeinen]

Clean up the function a bit, mainly by doing the mode_valid_check dance
once in the beginning of the function, and grouping the calculations
wrt. sync/event mode a bit better.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 48 ++++++++++++--------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 9238acf69823..0aaa1d06b21c 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -481,42 +481,38 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 			     bool mode_valid_check)
 {
 	struct cdns_dsi_output *output = &dsi->output;
-	unsigned int tmp;
-	bool sync_pulse = false;
+	u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
+	bool sync_pulse;
 	int bpp;
 
+	if (mode_valid_check) {
+		dpi_hsa = mode->hsync_end - mode->hsync_start;
+		dpi_hbp = mode->htotal - mode->hsync_end;
+		dpi_hfp = mode->hsync_start - mode->hdisplay;
+		dpi_hact = mode->hdisplay;
+	} else {
+		dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
+		dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
+		dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
+		dpi_hact = mode->crtc_hdisplay;
+	}
+
 	memset(dsi_cfg, 0, sizeof(*dsi_cfg));
 
-	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
-		sync_pulse = true;
+	sync_pulse = output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
 
 	bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
 
-	if (mode_valid_check)
-		tmp = mode->htotal -
-		      (sync_pulse ? mode->hsync_end : mode->hsync_start);
-	else
-		tmp = mode->crtc_htotal -
-		      (sync_pulse ?
-		       mode->crtc_hsync_end : mode->crtc_hsync_start);
-
-	dsi_cfg->hbp = dpi_to_dsi_timing(tmp, bpp, DSI_HBP_FRAME_OVERHEAD);
+	dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp + (sync_pulse ? 0 : dpi_hsa),
+					 bpp, DSI_HBP_FRAME_OVERHEAD);
 
-	if (sync_pulse) {
-		if (mode_valid_check)
-			tmp = mode->hsync_end - mode->hsync_start;
-		else
-			tmp = mode->crtc_hsync_end - mode->crtc_hsync_start;
+	if (sync_pulse)
+		dsi_cfg->hsa =
+			dpi_to_dsi_timing(dpi_hsa, bpp, DSI_HSA_FRAME_OVERHEAD);
 
-		dsi_cfg->hsa = dpi_to_dsi_timing(tmp, bpp,
-						 DSI_HSA_FRAME_OVERHEAD);
-	}
+	dsi_cfg->hact = dpi_to_dsi_timing(dpi_hact, bpp, 0);
 
-	dsi_cfg->hact = dpi_to_dsi_timing(mode_valid_check ?
-					  mode->hdisplay : mode->crtc_hdisplay,
-					  bpp, 0);
-	dsi_cfg->hfp = dpi_to_dsi_timing(mode_to_dpi_hfp(mode, mode_valid_check),
-					 bpp, DSI_HFP_FRAME_OVERHEAD);
+	dsi_cfg->hfp = dpi_to_dsi_timing(dpi_hfp, bpp, DSI_HFP_FRAME_OVERHEAD);
 
 	return 0;
 }

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 09/18] drm/bridge: cdns-dsi: Fix REG_WAKEUP_TIME value

[Tomi Valkeinen]

The driver tries to calculate the value for REG_WAKEUP_TIME. However,
the calculation itself is not correct, and to add on it, the resulting
value is almost always larger than the field's size, so the actual
result is more or less random.

According to the docs, figuring out the value for REG_WAKEUP_TIME
requires HW characterization and there's no way to have a generic
algorithm to come up with the value. That doesn't help at all...

However, we know that the value must be smaller than the line time, and,
at least in my understanding, the proper value for it is quite small.
Testing shows that setting it to 1/10 of the line time seems to work
well. All video modes from my HDMI monitor work with this algorithm.

Hopefully we'll get more information on how to calculate the value, and
we can then update this.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 0aaa1d06b21c..62811631341b 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -882,7 +882,13 @@ static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 
 	tx_byte_period = DIV_ROUND_DOWN_ULL((u64)NSEC_PER_SEC * 8,
 					    phy_cfg->hs_clk_rate);
-	reg_wakeup = (phy_cfg->hs_prepare + phy_cfg->hs_zero) / tx_byte_period;
+
+	/*
+	 * Estimated time [in clock cycles] to perform LP->HS on D-PHY.
+	 * It is not clear how to calculate this, so for now,
+	 * set it to 1/10 of the total number of clocks in a line.
+	 */
+	reg_wakeup = dsi_cfg.htotal / nlanes / 10;
 	writel(REG_WAKEUP_TIME(reg_wakeup) | REG_LINE_DURATION(tmp),
 	       dsi->regs + VID_DPHY_TIME);
 

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 10/18] drm/bridge: cdns-dsi: Fix event mode

[Tomi Valkeinen]

The timings calculation gets it wrong for DSI event mode, resulting in
too large hbp value. Fix the issue by taking into account the
pulse/event mode difference.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 33 ++++++++++++++++++--------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 62811631341b..9797e6faa29d 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -417,7 +417,8 @@
 #define DSI_OUTPUT_PORT			0
 #define DSI_INPUT_PORT(inputid)		(1 + (inputid))
 
-#define DSI_HBP_FRAME_OVERHEAD		12
+#define DSI_HBP_FRAME_PULSE_OVERHEAD	12
+#define DSI_HBP_FRAME_EVENT_OVERHEAD	16
 #define DSI_HSA_FRAME_OVERHEAD		14
 #define DSI_HFP_FRAME_OVERHEAD		6
 #define DSI_HSS_VSS_VSE_FRAME_OVERHEAD	4
@@ -503,12 +504,18 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 
 	bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
 
-	dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp + (sync_pulse ? 0 : dpi_hsa),
-					 bpp, DSI_HBP_FRAME_OVERHEAD);
+	if (sync_pulse) {
+		dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp, bpp,
+						 DSI_HBP_FRAME_PULSE_OVERHEAD);
 
-	if (sync_pulse)
-		dsi_cfg->hsa =
-			dpi_to_dsi_timing(dpi_hsa, bpp, DSI_HSA_FRAME_OVERHEAD);
+		dsi_cfg->hsa = dpi_to_dsi_timing(dpi_hsa, bpp,
+						 DSI_HSA_FRAME_OVERHEAD);
+	} else {
+		dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp + dpi_hsa, bpp,
+						 DSI_HBP_FRAME_EVENT_OVERHEAD);
+
+		dsi_cfg->hsa = 0;
+	}
 
 	dsi_cfg->hact = dpi_to_dsi_timing(dpi_hact, bpp, 0);
 
@@ -532,9 +539,12 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 	unsigned int dsi_hfp_ext;
 	unsigned int lanes = output->dev->lanes;
 
-	dsi_htotal = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
-	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
+	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) {
+		dsi_htotal = dsi_cfg->hbp + DSI_HBP_FRAME_PULSE_OVERHEAD;
 		dsi_htotal += dsi_cfg->hsa + DSI_HSA_FRAME_OVERHEAD;
+	} else {
+		dsi_htotal = dsi_cfg->hbp + DSI_HBP_FRAME_EVENT_OVERHEAD;
+	}
 
 	dsi_htotal += dsi_cfg->hact;
 	dsi_htotal += dsi_cfg->hfp + DSI_HFP_FRAME_OVERHEAD;
@@ -607,9 +617,12 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 		return -EINVAL;
 	}
 
-	dsi_hss_hsa_hse_hbp = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
-	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
+	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) {
+		dsi_hss_hsa_hse_hbp = dsi_cfg->hbp + DSI_HBP_FRAME_PULSE_OVERHEAD;
 		dsi_hss_hsa_hse_hbp += dsi_cfg->hsa + DSI_HSA_FRAME_OVERHEAD;
+	} else {
+		dsi_hss_hsa_hse_hbp = dsi_cfg->hbp + DSI_HBP_FRAME_EVENT_OVERHEAD;
+	}
 
 	/*
 	 * Make sure DPI(HFP) > DSI(HSS+HSA+HSE+HBP) to guarantee that the FIFO

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 11/18] drm/bridge: cdns-dsi: Remove broken fifo emptying check

[Tomi Valkeinen]

The driver check if "DPI(HFP) > DSI(HSS+HSA+HSE+HBP)", and rejects the
mode if not.

However, testing shows that this doesn't hold at all. I can set the hfp
to very small values, with no errors. The feedback from the HW team also
was that the check is not right, although it's not clear if there's a
way to validate the FIFO emptying.

The check rejects quite a lot of modes, apparently for no good reason,
so drop the check.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 28 --------------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 9797e6faa29d..e85c8652c96e 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -453,15 +453,6 @@ bridge_to_cdns_dsi_input(struct drm_bridge *bridge)
 	return container_of(bridge, struct cdns_dsi_input, bridge);
 }
 
-static unsigned int mode_to_dpi_hfp(const struct drm_display_mode *mode,
-				    bool mode_valid_check)
-{
-	if (mode_valid_check)
-		return mode->hsync_start - mode->hdisplay;
-
-	return mode->crtc_hsync_start - mode->crtc_hdisplay;
-}
-
 static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
 				      unsigned int dpi_bpp,
 				      unsigned int dsi_pkt_overhead)
@@ -583,7 +574,6 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 {
 	struct cdns_dsi_output *output = &dsi->output;
 	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
-	unsigned long dsi_hss_hsa_hse_hbp;
 	unsigned int nlanes = output->dev->lanes;
 	int mode_clock = (mode_valid_check ? mode->clock : mode->crtc_clock);
 	unsigned long req_hs_clk_rate;
@@ -617,24 +607,6 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 		return -EINVAL;
 	}
 
-	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) {
-		dsi_hss_hsa_hse_hbp = dsi_cfg->hbp + DSI_HBP_FRAME_PULSE_OVERHEAD;
-		dsi_hss_hsa_hse_hbp += dsi_cfg->hsa + DSI_HSA_FRAME_OVERHEAD;
-	} else {
-		dsi_hss_hsa_hse_hbp = dsi_cfg->hbp + DSI_HBP_FRAME_EVENT_OVERHEAD;
-	}
-
-	/*
-	 * Make sure DPI(HFP) > DSI(HSS+HSA+HSE+HBP) to guarantee that the FIFO
-	 * is empty before we start a receiving a new line on the DPI
-	 * interface.
-	 */
-	if ((u64)phy_cfg->hs_clk_rate *
-	    mode_to_dpi_hfp(mode, mode_valid_check) * nlanes <
-	    (u64)dsi_hss_hsa_hse_hbp *
-	    (mode_valid_check ? mode->clock : mode->crtc_clock) * 1000)
-		return -EINVAL;
-
 	return 0;
 }
 

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 12/18] drm/bridge: cdns-dsi: Drop checks that shouldn't be in .mode_valid()

[Tomi Valkeinen]

The docs say about mode_valid():

"it is not allowed to look at anything else but the passed-in mode, and
validate it against configuration-invariant hardware constraints"

We're doing a lot more than just looking at the mode. The main issue
here is that we're doing checks based on the pixel clock, before we know
what the pixel clock from the crtc actually is.

So, drop the checks from .mode_valid(). This also allows us to remove
the 'mode_valid_check' parameter from internal functions, and the
related code.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 44 ++++++++------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index e85c8652c96e..cf783680b1b4 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -469,25 +469,17 @@ static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
 
 static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 			     const struct drm_display_mode *mode,
-			     struct cdns_dsi_cfg *dsi_cfg,
-			     bool mode_valid_check)
+			     struct cdns_dsi_cfg *dsi_cfg)
 {
 	struct cdns_dsi_output *output = &dsi->output;
 	u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
 	bool sync_pulse;
 	int bpp;
 
-	if (mode_valid_check) {
-		dpi_hsa = mode->hsync_end - mode->hsync_start;
-		dpi_hbp = mode->htotal - mode->hsync_end;
-		dpi_hfp = mode->hsync_start - mode->hdisplay;
-		dpi_hact = mode->hdisplay;
-	} else {
-		dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
-		dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
-		dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
-		dpi_hact = mode->crtc_hdisplay;
-	}
+	dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
+	dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
+	dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
+	dpi_hact = mode->crtc_hdisplay;
 
 	memset(dsi_cfg, 0, sizeof(*dsi_cfg));
 
@@ -518,8 +510,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 			      struct cdns_dsi_cfg *dsi_cfg,
 			      struct phy_configure_opts_mipi_dphy *phy_cfg,
-			      const struct drm_display_mode *mode,
-			      bool mode_valid_check)
+			      const struct drm_display_mode *mode)
 {
 	struct cdns_dsi_output *output = &dsi->output;
 	unsigned long long dlane_bps;
@@ -549,11 +540,11 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 	if (dsi_htotal % lanes)
 		adj_dsi_htotal += lanes - (dsi_htotal % lanes);
 
-	dpi_hz = (mode_valid_check ? mode->clock : mode->crtc_clock) * 1000;
+	dpi_hz = mode->crtc_clock * 1000;
 	dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
 
 	/* data rate in bytes/sec is not an integer, refuse the mode. */
-	dpi_htotal = mode_valid_check ? mode->htotal : mode->crtc_htotal;
+	dpi_htotal = mode->crtc_htotal;
 	if (do_div(dlane_bps, lanes * dpi_htotal))
 		return -EINVAL;
 
@@ -569,27 +560,25 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 
 static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 			       const struct drm_display_mode *mode,
-			       struct cdns_dsi_cfg *dsi_cfg,
-			       bool mode_valid_check)
+			       struct cdns_dsi_cfg *dsi_cfg)
 {
 	struct cdns_dsi_output *output = &dsi->output;
 	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
 	unsigned int nlanes = output->dev->lanes;
-	int mode_clock = (mode_valid_check ? mode->clock : mode->crtc_clock);
 	unsigned long req_hs_clk_rate;
 	int ret;
 
-	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
+	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
 	if (ret)
 		return ret;
 
-	ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
+	ret = phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
 					       mipi_dsi_pixel_format_to_bpp(output->dev->format),
 					       nlanes, phy_cfg);
 	if (ret)
 		return ret;
 
-	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode, mode_valid_check);
+	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
 	if (ret)
 		return ret;
 
@@ -635,8 +624,7 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
 	struct cdns_dsi_output *output = &dsi->output;
-	struct cdns_dsi_cfg dsi_cfg;
-	int bpp, ret;
+	int bpp;
 
 	/*
 	 * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
@@ -654,10 +642,6 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	if ((mode->hdisplay * bpp) % 32)
 		return MODE_H_ILLEGAL;
 
-	ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg, true);
-	if (ret)
-		return MODE_BAD;
-
 	return MODE_OK;
 }
 
@@ -996,7 +980,7 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 	adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
 	adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
 
-	return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
+	return cdns_dsi_check_conf(dsi, mode, dsi_cfg);
 }
 
 static struct drm_bridge_state *

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 13/18] drm/bridge: cdns-dsi: Do not use crtc_* values

[Tomi Valkeinen]

The driver uses crtc_* fields from the mode. While I think in the
enable-path this would be correct, I do not think it's correct in the
check phase, as the crtc hasn't had a chance to update the crtc_* fields
yet.

Overall, my understanding is that the crtc_* fields are relevant only in
cases where we have things like interlace or double pixel mode, where we
have a logical and real timings. We never use those with DSI, so the
crtc_* fields should just always match the normal values.

So, drop the use of crtc_* values.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index cf783680b1b4..220213f5cb09 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -476,10 +476,10 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 	bool sync_pulse;
 	int bpp;
 
-	dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
-	dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
-	dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
-	dpi_hact = mode->crtc_hdisplay;
+	dpi_hsa = mode->hsync_end - mode->hsync_start;
+	dpi_hbp = mode->htotal - mode->hsync_end;
+	dpi_hfp =  mode->hsync_start - mode->hdisplay;
+	dpi_hact = mode->hdisplay;
 
 	memset(dsi_cfg, 0, sizeof(*dsi_cfg));
 
@@ -540,11 +540,11 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 	if (dsi_htotal % lanes)
 		adj_dsi_htotal += lanes - (dsi_htotal % lanes);
 
-	dpi_hz = mode->crtc_clock * 1000;
+	dpi_hz = mode->clock * 1000;
 	dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
 
 	/* data rate in bytes/sec is not an integer, refuse the mode. */
-	dpi_htotal = mode->crtc_htotal;
+	dpi_htotal = mode->htotal;
 	if (do_div(dlane_bps, lanes * dpi_htotal))
 		return -EINVAL;
 
@@ -572,7 +572,7 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 	if (ret)
 		return ret;
 
-	ret = phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
+	ret = phy_mipi_dphy_get_default_config(mode->clock * 1000,
 					       mipi_dsi_pixel_format_to_bpp(output->dev->format),
 					       nlanes, phy_cfg);
 	if (ret)
@@ -822,11 +822,11 @@ static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 	writel(HFP_LEN(dsi_cfg.hfp) | HACT_LEN(dsi_cfg.hact),
 	       dsi->regs + VID_HSIZE2);
 
-	writel(VBP_LEN(mode->crtc_vtotal - mode->crtc_vsync_end - 1) |
-	       VFP_LEN(mode->crtc_vsync_start - mode->crtc_vdisplay) |
-	       VSA_LEN(mode->crtc_vsync_end - mode->crtc_vsync_start + 1),
+	writel(VBP_LEN(mode->vtotal - mode->vsync_end - 1) |
+	       VFP_LEN(mode->vsync_start - mode->vdisplay) |
+	       VSA_LEN(mode->vsync_end - mode->vsync_start + 1),
 	       dsi->regs + VID_VSIZE1);
-	writel(mode->crtc_vdisplay, dsi->regs + VID_VSIZE2);
+	writel(mode->vdisplay, dsi->regs + VID_VSIZE2);
 
 	tmp = dsi_cfg.htotal -
 	      (dsi_cfg.hsa + DSI_BLANKING_FRAME_OVERHEAD +

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 14/18] drm/bridge: cdns-dsi: Use videomode internally

[Tomi Valkeinen]

The drm_display_mode is a bit difficult one to use (we need hfp, hbp,
... instead of hsync_start, hsync_end, ...) and understand (when to use
crtc_* fields).

To simplify the code, use struct videomode internally which cleans up
the code. If in the future we want to use crtc_* fields in some code
patchs, that can be easily achieved by creating a videomode from the
crtc_* fields, and no change to the functions that do the work is
needed.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 31 +++++++++++++++-----------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 220213f5cb09..1a30e2f7d402 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -9,6 +9,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_probe_helper.h>
 #include <video/mipi_display.h>
+#include <video/videomode.h>
 
 #include <linux/clk.h>
 #include <linux/interrupt.h>
@@ -468,7 +469,7 @@ static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
 }
 
 static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
-			     const struct drm_display_mode *mode,
+			     const struct videomode *vm,
 			     struct cdns_dsi_cfg *dsi_cfg)
 {
 	struct cdns_dsi_output *output = &dsi->output;
@@ -476,10 +477,10 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 	bool sync_pulse;
 	int bpp;
 
-	dpi_hsa = mode->hsync_end - mode->hsync_start;
-	dpi_hbp = mode->htotal - mode->hsync_end;
-	dpi_hfp =  mode->hsync_start - mode->hdisplay;
-	dpi_hact = mode->hdisplay;
+	dpi_hsa = vm->hsync_len;
+	dpi_hbp = vm->hback_porch;
+	dpi_hfp = vm->hfront_porch;
+	dpi_hact = vm->hactive;
 
 	memset(dsi_cfg, 0, sizeof(*dsi_cfg));
 
@@ -510,7 +511,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 			      struct cdns_dsi_cfg *dsi_cfg,
 			      struct phy_configure_opts_mipi_dphy *phy_cfg,
-			      const struct drm_display_mode *mode)
+			      const struct videomode *vm)
 {
 	struct cdns_dsi_output *output = &dsi->output;
 	unsigned long long dlane_bps;
@@ -540,11 +541,12 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 	if (dsi_htotal % lanes)
 		adj_dsi_htotal += lanes - (dsi_htotal % lanes);
 
-	dpi_hz = mode->clock * 1000;
+	dpi_hz = vm->pixelclock;
 	dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
 
 	/* data rate in bytes/sec is not an integer, refuse the mode. */
-	dpi_htotal = mode->htotal;
+	dpi_htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
+		     vm->hsync_len;
 	if (do_div(dlane_bps, lanes * dpi_htotal))
 		return -EINVAL;
 
@@ -559,7 +561,7 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 }
 
 static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
-			       const struct drm_display_mode *mode,
+			       const struct videomode *vm,
 			       struct cdns_dsi_cfg *dsi_cfg)
 {
 	struct cdns_dsi_output *output = &dsi->output;
@@ -568,17 +570,17 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 	unsigned long req_hs_clk_rate;
 	int ret;
 
-	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
+	ret = cdns_dsi_mode2cfg(dsi, vm, dsi_cfg);
 	if (ret)
 		return ret;
 
-	ret = phy_mipi_dphy_get_default_config(mode->clock * 1000,
+	ret = phy_mipi_dphy_get_default_config(vm->pixelclock,
 					       mipi_dsi_pixel_format_to_bpp(output->dev->format),
 					       nlanes, phy_cfg);
 	if (ret)
 		return ret;
 
-	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
+	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, vm);
 	if (ret)
 		return ret;
 
@@ -975,12 +977,15 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 	const struct drm_display_mode *mode = &crtc_state->mode;
 	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
 	struct drm_display_mode *adjusted_crtc_mode = &crtc_state->adjusted_mode;
+	struct videomode vm;
 
 	/* cdns-dsi requires negative syncs */
 	adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
 	adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
 
-	return cdns_dsi_check_conf(dsi, mode, dsi_cfg);
+	drm_display_mode_to_videomode(mode, &vm);
+
+	return cdns_dsi_check_conf(dsi, &vm, dsi_cfg);
 }
 
 static struct drm_bridge_state *

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 15/18] drm/bridge: cdns-dsi: Tune adjusted_mode->clock according to dsi needs

[Tomi Valkeinen]

The driver currently expects the pixel clock and the HS clock to be
compatible, but the DPHY PLL doesn't give very finely grained rates.
This often leads to the situation where the pipeline just fails, as the
resulting HS clock is just too off.

We could change the driver to do a better job on adjusting the DSI
blanking values, hopefully getting a working pipeline even if the pclk
and HS clocks are not exactly compatible. But that is a bigger work.

What we can do easily is to see in .atomic_check() what HS clock rate we
can get, based on the pixel clock rate, and then convert the HS clock
rate back to pixel clock rate and ask that rate from the crtc. If the
crtc has a good PLL (which is the case for TI K3 SoCs), this will fix
any issues wrt. the clock rates.

If the crtc cannot provide the requested clock, well, we're no worse off
with this patch than what we have at the moment.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 39 +++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 1a30e2f7d402..9f4f7b6c8330 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -966,6 +966,28 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 	return input_fmts;
 }
 
+static long cdns_dsi_round_pclk(struct cdns_dsi *dsi, unsigned long pclk)
+{
+	struct cdns_dsi_output *output = &dsi->output;
+	unsigned int nlanes = output->dev->lanes;
+	union phy_configure_opts phy_opts = { 0 };
+	u32 bitspp;
+	int ret;
+
+	bitspp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
+
+	ret = phy_mipi_dphy_get_default_config(pclk, bitspp, nlanes,
+					       &phy_opts.mipi_dphy);
+	if (ret)
+		return ret;
+
+	ret = phy_validate(dsi->dphy, PHY_MODE_MIPI_DPHY, 0, &phy_opts);
+	if (ret)
+		return ret;
+
+	return div64_u64((u64)phy_opts.mipi_dphy.hs_clk_rate * nlanes, bitspp);
+}
+
 static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 					struct drm_bridge_state *bridge_state,
 					struct drm_crtc_state *crtc_state,
@@ -978,12 +1000,27 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
 	struct drm_display_mode *adjusted_crtc_mode = &crtc_state->adjusted_mode;
 	struct videomode vm;
+	long pclk;
 
 	/* cdns-dsi requires negative syncs */
 	adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
 	adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
 
-	drm_display_mode_to_videomode(mode, &vm);
+	/*
+	 * The DPHY PLL has quite a coarsely grained clock rate options. See
+	 * what hsclk rate we can achieve based on the pixel clock, convert it
+	 * back to pixel clock, set that to the adjusted_mode->clock. This is
+	 * all in hopes that the CRTC will be able to provide us the requested
+	 * clock, as otherwise the DPI and DSI clocks will be out of sync.
+	 */
+
+	pclk = cdns_dsi_round_pclk(dsi, mode->clock * 1000);
+	if (pclk < 0)
+		return (int)pclk;
+
+	adjusted_crtc_mode->clock = pclk / 1000;
+
+	drm_display_mode_to_videomode(adjusted_crtc_mode, &vm);
 
 	return cdns_dsi_check_conf(dsi, &vm, dsi_cfg);
 }

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 16/18] drm/bridge: cdns-dsi: Update htotal in cdns_dsi_mode2cfg()

[Tomi Valkeinen]

cdns_dsi_mode2cfg() calculates the dsi timings, but for some reason
doesn't set the htotal based on those timings. It is set only later, in
cdns_dsi_adjust_phy_config().

As cdns_dsi_mode2cfg() is the logical place to calculate it, let's move
it there. Especially as the following patch will remove
cdns_dsi_adjust_phy_config().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 9f4f7b6c8330..2a272fd8ea3e 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -505,6 +505,15 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 
 	dsi_cfg->hfp = dpi_to_dsi_timing(dpi_hfp, bpp, DSI_HFP_FRAME_OVERHEAD);
 
+	dsi_cfg->htotal = dsi_cfg->hact + dsi_cfg->hfp + DSI_HFP_FRAME_OVERHEAD;
+
+	if (sync_pulse) {
+		dsi_cfg->htotal += dsi_cfg->hbp + DSI_HBP_FRAME_PULSE_OVERHEAD;
+		dsi_cfg->htotal += dsi_cfg->hsa + DSI_HSA_FRAME_OVERHEAD;
+	} else {
+		dsi_cfg->htotal += dsi_cfg->hbp + DSI_HBP_FRAME_EVENT_OVERHEAD;
+	}
+
 	return 0;
 }
 
@@ -522,15 +531,7 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 	unsigned int dsi_hfp_ext;
 	unsigned int lanes = output->dev->lanes;
 
-	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) {
-		dsi_htotal = dsi_cfg->hbp + DSI_HBP_FRAME_PULSE_OVERHEAD;
-		dsi_htotal += dsi_cfg->hsa + DSI_HSA_FRAME_OVERHEAD;
-	} else {
-		dsi_htotal = dsi_cfg->hbp + DSI_HBP_FRAME_EVENT_OVERHEAD;
-	}
-
-	dsi_htotal += dsi_cfg->hact;
-	dsi_htotal += dsi_cfg->hfp + DSI_HFP_FRAME_OVERHEAD;
+	dsi_htotal = dsi_cfg->htotal;
 
 	/*
 	 * Make sure DSI htotal is aligned on a lane boundary when calculating

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 17/18] drm/bridge: cdns-dsi: Drop cdns_dsi_adjust_phy_config()

[Tomi Valkeinen]

cdns_dsi_adjust_phy_config() is called from
cdns_dsi_adjust_phy_config(), which is called from .atomic_check(). It
checks the DSI htotal and adjusts it to align on the DSI lane boundary
by changing hfp and then recalculating htotal and HS clock rate.

This has a few problems.

First is the fact that the whole thing is not needed: we do not need to
align on the lane boundary. The whole frame is sent in HS mode, and it
is fine if the line's last byte clock tick fills, say, only 2 of the 4
lanes. The next line will just continue from there. Assuming the
DSI timing values have been calculated to match the incoming DPI stream,
and the HS clock is compatible with the DPI pixel clock, the "uneven"
DSI lines will even out when multiple lines are being sent.

But we could do the align, aligning is not a problem as such. However,
adding more bytes to the hfp, as the function currently does, makes the
DSI line time longer, so the function then adjusts the HS clock rate.
This is where things fail: we don't know what rates we can get from the
HS clock, and at least in TI K3 SoC case the rates are quite coarsely
grained. Thus small adjustment to hfp will lead to a big change in HS
clock rate, and things break down.

We could do a loop here, adjusting hfp, adjusting clock, checking clock
rate, adjusting hfp again, etc., but considering that the whole
adjustment shouldn't be needed at all, it's easier to just remove the
function.

Something like this function should be added back later, when adding
burst mode support, but that's a bigger change and I don't think this
function would help that work in any way.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 48 --------------------------
 1 file changed, 48 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 2a272fd8ea3e..0bb55584cb44 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -517,50 +517,6 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 	return 0;
 }
 
-static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
-			      struct cdns_dsi_cfg *dsi_cfg,
-			      struct phy_configure_opts_mipi_dphy *phy_cfg,
-			      const struct videomode *vm)
-{
-	struct cdns_dsi_output *output = &dsi->output;
-	unsigned long long dlane_bps;
-	unsigned long adj_dsi_htotal;
-	unsigned long dsi_htotal;
-	unsigned long dpi_htotal;
-	unsigned long dpi_hz;
-	unsigned int dsi_hfp_ext;
-	unsigned int lanes = output->dev->lanes;
-
-	dsi_htotal = dsi_cfg->htotal;
-
-	/*
-	 * Make sure DSI htotal is aligned on a lane boundary when calculating
-	 * the expected data rate. This is done by extending HFP in case of
-	 * misalignment.
-	 */
-	adj_dsi_htotal = dsi_htotal;
-	if (dsi_htotal % lanes)
-		adj_dsi_htotal += lanes - (dsi_htotal % lanes);
-
-	dpi_hz = vm->pixelclock;
-	dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
-
-	/* data rate in bytes/sec is not an integer, refuse the mode. */
-	dpi_htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
-		     vm->hsync_len;
-	if (do_div(dlane_bps, lanes * dpi_htotal))
-		return -EINVAL;
-
-	/* data rate was in bytes/sec, convert to bits/sec. */
-	phy_cfg->hs_clk_rate = dlane_bps * 8;
-
-	dsi_hfp_ext = adj_dsi_htotal - dsi_htotal;
-	dsi_cfg->hfp += dsi_hfp_ext;
-	dsi_cfg->htotal = dsi_htotal + dsi_hfp_ext;
-
-	return 0;
-}
-
 static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 			       const struct videomode *vm,
 			       struct cdns_dsi_cfg *dsi_cfg)
@@ -581,10 +537,6 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 	if (ret)
 		return ret;
 
-	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, vm);
-	if (ret)
-		return ret;
-
 	req_hs_clk_rate = output->phy_opts.mipi_dphy.hs_clk_rate;
 	ret = phy_validate(dsi->dphy, PHY_MODE_MIPI_DPHY, 0, &output->phy_opts);
 	if (ret)

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 18/18] drm/bridge: cdns-dsi: Don't fail on MIPI_DSI_MODE_VIDEO_BURST

[Tomi Valkeinen]

While the cdns-dsi does not support DSI burst mode, the burst mode is
essentially DSI event mode with more versatile clocking and timings.
Thus cdns-dsi doesn't need to fail if the DSI peripheral driver requests
MIPI_DSI_MODE_VIDEO_BURST.

In my particular use case, this allows the use of ti-sn65dsi83 driver.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 0bb55584cb44..4b77a203b608 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -1057,10 +1057,6 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
 	if (output->dev)
 		return -EBUSY;
 
-	/* We do not support burst mode yet. */
-	if (dev->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
-		return -ENOTSUPP;
-
 	/*
 	 * The host <-> device link might be described using an OF-graph
 	 * representation, in this case we extract the device of_node from

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 01/18] drm/tidss: Fix missing includes and struct decls

[Aradhya Bhatia]

On 02/04/25 19:00, Tomi Valkeinen wrote:
> Fix missing includes and struct declarations. Even if these don't cause
> any compile issues at the moment, it's good to have them correct.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.h       | 3 +++
>  drivers/gpu/drm/tidss/tidss_drv.h         | 2 ++
>  drivers/gpu/drm/tidss/tidss_plane.h       | 2 ++
>  drivers/gpu/drm/tidss/tidss_scale_coefs.h | 2 ++
>  4 files changed, 9 insertions(+)
> 

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

-- 
Regards
Aradhya


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 03/18] drm/tidss: Adjust the pclk based on the HW capabilities

[Aradhya Bhatia]

On 02/04/25 19:00, Tomi Valkeinen wrote:
> At the moment the driver just sets the clock rate with clk_set_rate(),
> and if the resulting rate is not the same as requested, prints a debug
> print, but nothing else.
> 
> Add functionality to atomic_check(), in which the clk_round_rate() is
> used to get the "rounded" rate, and set that to the adjusted_mode.
> 
> In practice, with the current K3 SoCs, the display PLL is capable of
> producing very exact clocks, so most likely the rounded rate is the same
> as the original one.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/tidss/tidss_crtc.c  | 23 +++++++++++++++++++----
>  drivers/gpu/drm/tidss/tidss_dispc.c |  6 ++++++
>  drivers/gpu/drm/tidss/tidss_dispc.h |  2 ++
>  3 files changed, 27 insertions(+), 4 deletions(-)
> 

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

-- 
Regards
Aradhya


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 05/18] phy: cdns-dphy: Remove leftover code

[Aradhya Bhatia]

Hi Tomi,

Thank you for the patches!

On 02/04/25 19:00, Tomi Valkeinen wrote:
> The code in cdns-dphy has probably been part of a DSI driver in the
> past. Remove DSI defines and variables which are not used or do not
> actually do anything. Also rename cdns_dsi_get_dphy_pll_cfg() to
> cdns_get_dphy_pll_cfg(), i.e. drop the "dsi", as it's not relevant here.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/phy/cadence/cdns-dphy.c | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
> index f79ec4fab409..7f8b70ec10c5 100644
> --- a/drivers/phy/cadence/cdns-dphy.c
> +++ b/drivers/phy/cadence/cdns-dphy.c
> @@ -55,14 +55,6 @@
>  #define DPHY_PSM_CFG_FROM_REG		BIT(0)
>  #define DPHY_PSM_CLK_DIV(x)		((x) << 1)
>  
> -#define DSI_HBP_FRAME_OVERHEAD		12
> -#define DSI_HSA_FRAME_OVERHEAD		14
> -#define DSI_HFP_FRAME_OVERHEAD		6
> -#define DSI_HSS_VSS_VSE_FRAME_OVERHEAD	4
> -#define DSI_BLANKING_FRAME_OVERHEAD	6
> -#define DSI_NULL_FRAME_OVERHEAD		6
> -#define DSI_EOT_PKT_SIZE		4
> -
>  #define DPHY_TX_J721E_WIZ_PLL_CTRL	0xF04
>  #define DPHY_TX_J721E_WIZ_STATUS	0xF08
>  #define DPHY_TX_J721E_WIZ_RST_CTRL	0xF0C
> @@ -117,10 +109,9 @@ static const unsigned int tx_bands[] = {
>  	870, 950, 1000, 1200, 1400, 1600, 1800, 2000, 2200, 2500
>  };
>  
> -static int cdns_dsi_get_dphy_pll_cfg(struct cdns_dphy *dphy,
> -				     struct cdns_dphy_cfg *cfg,
> -				     struct phy_configure_opts_mipi_dphy *opts,
> -				     unsigned int *dsi_hfp_ext)
> +static int cdns_get_dphy_pll_cfg(struct cdns_dphy *dphy,

nit: cdns_dphy_get_pll_cfg seems more appropriate. We even have a
cdns_dphy_set_pll_cfg.

> +				 struct cdns_dphy_cfg *cfg,
> +				 struct phy_configure_opts_mipi_dphy *opts)
>  {
>  	unsigned long pll_ref_hz = clk_get_rate(dphy->pll_ref_clk);
>  	u64 dlane_bps;
> @@ -289,15 +280,13 @@ static int cdns_dphy_config_from_opts(struct phy *phy,
>  				      struct cdns_dphy_cfg *cfg)
>  {
>  	struct cdns_dphy *dphy = phy_get_drvdata(phy);
> -	unsigned int dsi_hfp_ext = 0;
>  	int ret;
>  
>  	ret = phy_mipi_dphy_config_validate(opts);
>  	if (ret)
>  		return ret;
>  
> -	ret = cdns_dsi_get_dphy_pll_cfg(dphy, cfg,
> -					opts, &dsi_hfp_ext);
> +	ret = cdns_get_dphy_pll_cfg(dphy, cfg, opts);
>  	if (ret)
>  		return ret;
>  
> 

Apart from the small comment above,

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

-- 
Regards
Aradhya


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 07/18] drm/bridge: cdns-dsi: Fail if HS rate changed when validating PHY config

[Aradhya Bhatia]

On 02/04/25 19:00, Tomi Valkeinen wrote:
> The phy_validate() can change the HS clock rate we passed to it in the
> PHY config, depending on what the HW can actually do. The driver just
> ignores this at the moment, but if the actual HS clock rate is different
> than the requested one, the pipeline will fail as all the DSI timing
> calculations will be incorrect.
> 
> There are ways to improve DSI operation for various clock rates, but for
> now, just add a check to see if the rate changed, and return an error if
> that happens.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

-- 
Regards
Aradhya


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 08/18] drm/bridge: cdns-dsi: Clean up cdns_dsi_mode2cfg()

[Aradhya Bhatia]

On 02/04/25 19:00, Tomi Valkeinen wrote:
> Clean up the function a bit, mainly by doing the mode_valid_check dance
> once in the beginning of the function, and grouping the calculations
> wrt. sync/event mode a bit better.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 48 ++++++++++++--------------
>  1 file changed, 22 insertions(+), 26 deletions(-)

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

-- 
Regards
Aradhya


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 12/18] drm/bridge: cdns-dsi: Drop checks that shouldn't be in .mode_valid()

[Aradhya Bhatia]

Hi Tomi,

On 02/04/25 19:00, Tomi Valkeinen wrote:
> The docs say about mode_valid():
> 
> "it is not allowed to look at anything else but the passed-in mode, and
> validate it against configuration-invariant hardware constraints"
> 
> We're doing a lot more than just looking at the mode. The main issue
> here is that we're doing checks based on the pixel clock, before we know
> what the pixel clock from the crtc actually is.
> 
> So, drop the checks from .mode_valid(). This also allows us to remove
> the 'mode_valid_check' parameter from internal functions, and the
> related code.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 44 ++++++++------------------
>  1 file changed, 14 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index e85c8652c96e..cf783680b1b4 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -469,25 +469,17 @@ static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
>  
>  static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>  			     const struct drm_display_mode *mode,
> -			     struct cdns_dsi_cfg *dsi_cfg,
> -			     bool mode_valid_check)
> +			     struct cdns_dsi_cfg *dsi_cfg)
>  {
>  	struct cdns_dsi_output *output = &dsi->output;
>  	u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
>  	bool sync_pulse;
>  	int bpp;
>  
> -	if (mode_valid_check) {
> -		dpi_hsa = mode->hsync_end - mode->hsync_start;
> -		dpi_hbp = mode->htotal - mode->hsync_end;
> -		dpi_hfp = mode->hsync_start - mode->hdisplay;
> -		dpi_hact = mode->hdisplay;
> -	} else {
> -		dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
> -		dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
> -		dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
> -		dpi_hact = mode->crtc_hdisplay;
> -	}
> +	dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
> +	dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
> +	dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
> +	dpi_hact = mode->crtc_hdisplay;
>  
>  	memset(dsi_cfg, 0, sizeof(*dsi_cfg));
>  
> @@ -518,8 +510,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>  static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>  			      struct cdns_dsi_cfg *dsi_cfg,
>  			      struct phy_configure_opts_mipi_dphy *phy_cfg,
> -			      const struct drm_display_mode *mode,
> -			      bool mode_valid_check)
> +			      const struct drm_display_mode *mode)
>  {
>  	struct cdns_dsi_output *output = &dsi->output;
>  	unsigned long long dlane_bps;
> @@ -549,11 +540,11 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>  	if (dsi_htotal % lanes)
>  		adj_dsi_htotal += lanes - (dsi_htotal % lanes);
>  
> -	dpi_hz = (mode_valid_check ? mode->clock : mode->crtc_clock) * 1000;
> +	dpi_hz = mode->crtc_clock * 1000;
>  	dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
>  
>  	/* data rate in bytes/sec is not an integer, refuse the mode. */
> -	dpi_htotal = mode_valid_check ? mode->htotal : mode->crtc_htotal;
> +	dpi_htotal = mode->crtc_htotal;
>  	if (do_div(dlane_bps, lanes * dpi_htotal))
>  		return -EINVAL;
>  
> @@ -569,27 +560,25 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>  
>  static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
>  			       const struct drm_display_mode *mode,
> -			       struct cdns_dsi_cfg *dsi_cfg,
> -			       bool mode_valid_check)
> +			       struct cdns_dsi_cfg *dsi_cfg)
>  {
>  	struct cdns_dsi_output *output = &dsi->output;
>  	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
>  	unsigned int nlanes = output->dev->lanes;
> -	int mode_clock = (mode_valid_check ? mode->clock : mode->crtc_clock);
>  	unsigned long req_hs_clk_rate;
>  	int ret;
>  
> -	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
> +	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
>  	if (ret)
>  		return ret;
>  
> -	ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
> +	ret = phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
>  					       mipi_dsi_pixel_format_to_bpp(output->dev->format),
>  					       nlanes, phy_cfg);
>  	if (ret)
>  		return ret;
>  
> -	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode, mode_valid_check);
> +	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
>  	if (ret)
>  		return ret;
>  
> @@ -635,8 +624,7 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>  	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>  	struct cdns_dsi *dsi = input_to_dsi(input);
>  	struct cdns_dsi_output *output = &dsi->output;
> -	struct cdns_dsi_cfg dsi_cfg;
> -	int bpp, ret;
> +	int bpp;
>  
>  	/*
>  	 * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
> @@ -654,10 +642,6 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>  	if ((mode->hdisplay * bpp) % 32)
>  		return MODE_H_ILLEGAL;
>  
> -	ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg, true);
> -	if (ret)
> -		return MODE_BAD;
> -
>  	return MODE_OK;
>  }
>  
> @@ -996,7 +980,7 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
>  	adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>  	adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
>  
> -	return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
> +	return cdns_dsi_check_conf(dsi, mode, dsi_cfg);

With this patch, the driver shifts to using the crtc_* values during the
check phase, and then, it is brought back to using non crtc_* values in
the next patch.

Should this patch just drop the cdns_dsi_check_conf() check from
.mode_valid() instead, and let the next patch phase out the
mdoe_valid_check parameter as the driver simultaneously shifts to using
the non crtc_* values throughout?


-- 
Regards
Aradhya


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 12/18] drm/bridge: cdns-dsi: Drop checks that shouldn't be in .mode_valid()

[Tomi Valkeinen]

Hi,

On 07/04/2025 20:59, Aradhya Bhatia wrote:
> Hi Tomi,
> 
> On 02/04/25 19:00, Tomi Valkeinen wrote:
>> The docs say about mode_valid():
>>
>> "it is not allowed to look at anything else but the passed-in mode, and
>> validate it against configuration-invariant hardware constraints"
>>
>> We're doing a lot more than just looking at the mode. The main issue
>> here is that we're doing checks based on the pixel clock, before we know
>> what the pixel clock from the crtc actually is.
>>
>> So, drop the checks from .mode_valid(). This also allows us to remove
>> the 'mode_valid_check' parameter from internal functions, and the
>> related code.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 44 ++++++++------------------
>>   1 file changed, 14 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index e85c8652c96e..cf783680b1b4 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -469,25 +469,17 @@ static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
>>   
>>   static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>   			     const struct drm_display_mode *mode,
>> -			     struct cdns_dsi_cfg *dsi_cfg,
>> -			     bool mode_valid_check)
>> +			     struct cdns_dsi_cfg *dsi_cfg)
>>   {
>>   	struct cdns_dsi_output *output = &dsi->output;
>>   	u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
>>   	bool sync_pulse;
>>   	int bpp;
>>   
>> -	if (mode_valid_check) {
>> -		dpi_hsa = mode->hsync_end - mode->hsync_start;
>> -		dpi_hbp = mode->htotal - mode->hsync_end;
>> -		dpi_hfp = mode->hsync_start - mode->hdisplay;
>> -		dpi_hact = mode->hdisplay;
>> -	} else {
>> -		dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>> -		dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>> -		dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
>> -		dpi_hact = mode->crtc_hdisplay;
>> -	}
>> +	dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>> +	dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>> +	dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
>> +	dpi_hact = mode->crtc_hdisplay;
>>   
>>   	memset(dsi_cfg, 0, sizeof(*dsi_cfg));
>>   
>> @@ -518,8 +510,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>   static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>>   			      struct cdns_dsi_cfg *dsi_cfg,
>>   			      struct phy_configure_opts_mipi_dphy *phy_cfg,
>> -			      const struct drm_display_mode *mode,
>> -			      bool mode_valid_check)
>> +			      const struct drm_display_mode *mode)
>>   {
>>   	struct cdns_dsi_output *output = &dsi->output;
>>   	unsigned long long dlane_bps;
>> @@ -549,11 +540,11 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>>   	if (dsi_htotal % lanes)
>>   		adj_dsi_htotal += lanes - (dsi_htotal % lanes);
>>   
>> -	dpi_hz = (mode_valid_check ? mode->clock : mode->crtc_clock) * 1000;
>> +	dpi_hz = mode->crtc_clock * 1000;
>>   	dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
>>   
>>   	/* data rate in bytes/sec is not an integer, refuse the mode. */
>> -	dpi_htotal = mode_valid_check ? mode->htotal : mode->crtc_htotal;
>> +	dpi_htotal = mode->crtc_htotal;
>>   	if (do_div(dlane_bps, lanes * dpi_htotal))
>>   		return -EINVAL;
>>   
>> @@ -569,27 +560,25 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>>   
>>   static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
>>   			       const struct drm_display_mode *mode,
>> -			       struct cdns_dsi_cfg *dsi_cfg,
>> -			       bool mode_valid_check)
>> +			       struct cdns_dsi_cfg *dsi_cfg)
>>   {
>>   	struct cdns_dsi_output *output = &dsi->output;
>>   	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
>>   	unsigned int nlanes = output->dev->lanes;
>> -	int mode_clock = (mode_valid_check ? mode->clock : mode->crtc_clock);
>>   	unsigned long req_hs_clk_rate;
>>   	int ret;
>>   
>> -	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
>> +	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
>> +	ret = phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
>>   					       mipi_dsi_pixel_format_to_bpp(output->dev->format),
>>   					       nlanes, phy_cfg);
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode, mode_valid_check);
>> +	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
>>   	if (ret)
>>   		return ret;
>>   
>> @@ -635,8 +624,7 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>>   	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>>   	struct cdns_dsi *dsi = input_to_dsi(input);
>>   	struct cdns_dsi_output *output = &dsi->output;
>> -	struct cdns_dsi_cfg dsi_cfg;
>> -	int bpp, ret;
>> +	int bpp;
>>   
>>   	/*
>>   	 * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
>> @@ -654,10 +642,6 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>>   	if ((mode->hdisplay * bpp) % 32)
>>   		return MODE_H_ILLEGAL;
>>   
>> -	ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg, true);
>> -	if (ret)
>> -		return MODE_BAD;
>> -
>>   	return MODE_OK;
>>   }
>>   
>> @@ -996,7 +980,7 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
>>   	adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>>   	adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
>>   
>> -	return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
>> +	return cdns_dsi_check_conf(dsi, mode, dsi_cfg);
> 
> With this patch, the driver shifts to using the crtc_* values during the
> check phase, and then, it is brought back to using non crtc_* values in
> the next patch.
> 
> Should this patch just drop the cdns_dsi_check_conf() check from
> .mode_valid() instead, and let the next patch phase out the
> mdoe_valid_check parameter as the driver simultaneously shifts to using
> the non crtc_* values throughout?

Yes.

  Tomi


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 12/18] drm/bridge: cdns-dsi: Drop checks that shouldn't be in .mode_valid()

[Tomi Valkeinen]

Hi,

On 08/04/2025 09:09, Tomi Valkeinen wrote:
> Hi,
> 
> On 07/04/2025 20:59, Aradhya Bhatia wrote:
>> Hi Tomi,
>>
>> On 02/04/25 19:00, Tomi Valkeinen wrote:
>>> The docs say about mode_valid():
>>>
>>> "it is not allowed to look at anything else but the passed-in mode, and
>>> validate it against configuration-invariant hardware constraints"
>>>
>>> We're doing a lot more than just looking at the mode. The main issue
>>> here is that we're doing checks based on the pixel clock, before we know
>>> what the pixel clock from the crtc actually is.
>>>
>>> So, drop the checks from .mode_valid(). This also allows us to remove
>>> the 'mode_valid_check' parameter from internal functions, and the
>>> related code.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> ---
>>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 44 +++++++ 
>>> +------------------
>>>   1 file changed, 14 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/ 
>>> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> index e85c8652c96e..cf783680b1b4 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> @@ -469,25 +469,17 @@ static unsigned int dpi_to_dsi_timing(unsigned 
>>> int dpi_timing,
>>>   static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>>                    const struct drm_display_mode *mode,
>>> -                 struct cdns_dsi_cfg *dsi_cfg,
>>> -                 bool mode_valid_check)
>>> +                 struct cdns_dsi_cfg *dsi_cfg)
>>>   {
>>>       struct cdns_dsi_output *output = &dsi->output;
>>>       u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
>>>       bool sync_pulse;
>>>       int bpp;
>>> -    if (mode_valid_check) {
>>> -        dpi_hsa = mode->hsync_end - mode->hsync_start;
>>> -        dpi_hbp = mode->htotal - mode->hsync_end;
>>> -        dpi_hfp = mode->hsync_start - mode->hdisplay;
>>> -        dpi_hact = mode->hdisplay;
>>> -    } else {
>>> -        dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>>> -        dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>>> -        dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
>>> -        dpi_hact = mode->crtc_hdisplay;
>>> -    }
>>> +    dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>>> +    dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>>> +    dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
>>> +    dpi_hact = mode->crtc_hdisplay;
>>>       memset(dsi_cfg, 0, sizeof(*dsi_cfg));
>>> @@ -518,8 +510,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>>   static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>>>                     struct cdns_dsi_cfg *dsi_cfg,
>>>                     struct phy_configure_opts_mipi_dphy *phy_cfg,
>>> -                  const struct drm_display_mode *mode,
>>> -                  bool mode_valid_check)
>>> +                  const struct drm_display_mode *mode)
>>>   {
>>>       struct cdns_dsi_output *output = &dsi->output;
>>>       unsigned long long dlane_bps;
>>> @@ -549,11 +540,11 @@ static int cdns_dsi_adjust_phy_config(struct 
>>> cdns_dsi *dsi,
>>>       if (dsi_htotal % lanes)
>>>           adj_dsi_htotal += lanes - (dsi_htotal % lanes);
>>> -    dpi_hz = (mode_valid_check ? mode->clock : mode->crtc_clock) * 
>>> 1000;
>>> +    dpi_hz = mode->crtc_clock * 1000;
>>>       dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
>>>       /* data rate in bytes/sec is not an integer, refuse the mode. */
>>> -    dpi_htotal = mode_valid_check ? mode->htotal : mode->crtc_htotal;
>>> +    dpi_htotal = mode->crtc_htotal;
>>>       if (do_div(dlane_bps, lanes * dpi_htotal))
>>>           return -EINVAL;
>>> @@ -569,27 +560,25 @@ static int cdns_dsi_adjust_phy_config(struct 
>>> cdns_dsi *dsi,
>>>   static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
>>>                      const struct drm_display_mode *mode,
>>> -                   struct cdns_dsi_cfg *dsi_cfg,
>>> -                   bool mode_valid_check)
>>> +                   struct cdns_dsi_cfg *dsi_cfg)
>>>   {
>>>       struct cdns_dsi_output *output = &dsi->output;
>>>       struct phy_configure_opts_mipi_dphy *phy_cfg = &output- 
>>> >phy_opts.mipi_dphy;
>>>       unsigned int nlanes = output->dev->lanes;
>>> -    int mode_clock = (mode_valid_check ? mode->clock : mode- 
>>> >crtc_clock);
>>>       unsigned long req_hs_clk_rate;
>>>       int ret;
>>> -    ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
>>> +    ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
>>>       if (ret)
>>>           return ret;
>>> -    ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
>>> +    ret = phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
>>>                              mipi_dsi_pixel_format_to_bpp(output- 
>>> >dev->format),
>>>                              nlanes, phy_cfg);
>>>       if (ret)
>>>           return ret;
>>> -    ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode, 
>>> mode_valid_check);
>>> +    ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
>>>       if (ret)
>>>           return ret;
>>> @@ -635,8 +624,7 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge 
>>> *bridge,
>>>       struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>>>       struct cdns_dsi *dsi = input_to_dsi(input);
>>>       struct cdns_dsi_output *output = &dsi->output;
>>> -    struct cdns_dsi_cfg dsi_cfg;
>>> -    int bpp, ret;
>>> +    int bpp;
>>>       /*
>>>        * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
>>> @@ -654,10 +642,6 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge 
>>> *bridge,
>>>       if ((mode->hdisplay * bpp) % 32)
>>>           return MODE_H_ILLEGAL;
>>> -    ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg, true);
>>> -    if (ret)
>>> -        return MODE_BAD;
>>> -
>>>       return MODE_OK;
>>>   }
>>> @@ -996,7 +980,7 @@ static int cdns_dsi_bridge_atomic_check(struct 
>>> drm_bridge *bridge,
>>>       adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | 
>>> DRM_MODE_FLAG_PVSYNC);
>>>       adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC | 
>>> DRM_MODE_FLAG_NVSYNC;
>>> -    return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
>>> +    return cdns_dsi_check_conf(dsi, mode, dsi_cfg);
>>
>> With this patch, the driver shifts to using the crtc_* values during the
>> check phase, and then, it is brought back to using non crtc_* values in
>> the next patch.
>>
>> Should this patch just drop the cdns_dsi_check_conf() check from
>> .mode_valid() instead, and let the next patch phase out the
>> mdoe_valid_check parameter as the driver simultaneously shifts to using
>> the non crtc_* values throughout?
> 
> Yes.

Actually, this patch doesn't change the crtc_* vs non-crtc_* behavior. 
After dropping the cdns_dsi_check_conf() call in mode_valid(), the 
'mode_valid_check' is always false. So this patch removes the parameter, 
and any code paths for the true-case.

Should the atomic_check() have been using 'true' for the 
'mode_valid_check'? The atomic_check code was added in 
a53d987756eab40678f241d7cd0eb7e1ca42bba7.

  Tomi


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 12/18] drm/bridge: cdns-dsi: Drop checks that shouldn't be in .mode_valid()

[Aradhya Bhatia]

Hi Tomi,

On 08/04/25 12:39, Tomi Valkeinen wrote:
> Hi,
> 
> On 08/04/2025 09:09, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 07/04/2025 20:59, Aradhya Bhatia wrote:
>>> Hi Tomi,
>>>
>>> On 02/04/25 19:00, Tomi Valkeinen wrote:
>>>> The docs say about mode_valid():
>>>>
>>>> "it is not allowed to look at anything else but the passed-in mode, and
>>>> validate it against configuration-invariant hardware constraints"
>>>>
>>>> We're doing a lot more than just looking at the mode. The main issue
>>>> here is that we're doing checks based on the pixel clock, before we
>>>> know
>>>> what the pixel clock from the crtc actually is.
>>>>
>>>> So, drop the checks from .mode_valid(). This also allows us to remove
>>>> the 'mode_valid_check' parameter from internal functions, and the
>>>> related code.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> ---
>>>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 44 +++++++
>>>> +------------------
>>>>   1 file changed, 14 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/
>>>> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>> index e85c8652c96e..cf783680b1b4 100644
>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>> @@ -469,25 +469,17 @@ static unsigned int dpi_to_dsi_timing(unsigned
>>>> int dpi_timing,
>>>>   static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>>>                    const struct drm_display_mode *mode,
>>>> -                 struct cdns_dsi_cfg *dsi_cfg,
>>>> -                 bool mode_valid_check)
>>>> +                 struct cdns_dsi_cfg *dsi_cfg)
>>>>   {
>>>>       struct cdns_dsi_output *output = &dsi->output;
>>>>       u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
>>>>       bool sync_pulse;
>>>>       int bpp;
>>>> -    if (mode_valid_check) {
>>>> -        dpi_hsa = mode->hsync_end - mode->hsync_start;
>>>> -        dpi_hbp = mode->htotal - mode->hsync_end;
>>>> -        dpi_hfp = mode->hsync_start - mode->hdisplay;
>>>> -        dpi_hact = mode->hdisplay;
>>>> -    } else {
>>>> -        dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>>>> -        dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>>>> -        dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
>>>> -        dpi_hact = mode->crtc_hdisplay;
>>>> -    }
>>>> +    dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>>>> +    dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>>>> +    dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
>>>> +    dpi_hact = mode->crtc_hdisplay;
>>>>       memset(dsi_cfg, 0, sizeof(*dsi_cfg));
>>>> @@ -518,8 +510,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>>>   static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>>>>                     struct cdns_dsi_cfg *dsi_cfg,
>>>>                     struct phy_configure_opts_mipi_dphy *phy_cfg,
>>>> -                  const struct drm_display_mode *mode,
>>>> -                  bool mode_valid_check)
>>>> +                  const struct drm_display_mode *mode)
>>>>   {
>>>>       struct cdns_dsi_output *output = &dsi->output;
>>>>       unsigned long long dlane_bps;
>>>> @@ -549,11 +540,11 @@ static int cdns_dsi_adjust_phy_config(struct
>>>> cdns_dsi *dsi,
>>>>       if (dsi_htotal % lanes)
>>>>           adj_dsi_htotal += lanes - (dsi_htotal % lanes);
>>>> -    dpi_hz = (mode_valid_check ? mode->clock : mode->crtc_clock) *
>>>> 1000;
>>>> +    dpi_hz = mode->crtc_clock * 1000;
>>>>       dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
>>>>       /* data rate in bytes/sec is not an integer, refuse the mode. */
>>>> -    dpi_htotal = mode_valid_check ? mode->htotal : mode->crtc_htotal;
>>>> +    dpi_htotal = mode->crtc_htotal;
>>>>       if (do_div(dlane_bps, lanes * dpi_htotal))
>>>>           return -EINVAL;
>>>> @@ -569,27 +560,25 @@ static int cdns_dsi_adjust_phy_config(struct
>>>> cdns_dsi *dsi,
>>>>   static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
>>>>                      const struct drm_display_mode *mode,
>>>> -                   struct cdns_dsi_cfg *dsi_cfg,
>>>> -                   bool mode_valid_check)
>>>> +                   struct cdns_dsi_cfg *dsi_cfg)
>>>>   {
>>>>       struct cdns_dsi_output *output = &dsi->output;
>>>>       struct phy_configure_opts_mipi_dphy *phy_cfg = &output-
>>>> >phy_opts.mipi_dphy;
>>>>       unsigned int nlanes = output->dev->lanes;
>>>> -    int mode_clock = (mode_valid_check ? mode->clock : mode-
>>>> >crtc_clock);
>>>>       unsigned long req_hs_clk_rate;
>>>>       int ret;
>>>> -    ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
>>>> +    ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
>>>>       if (ret)
>>>>           return ret;
>>>> -    ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
>>>> +    ret = phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
>>>>                              mipi_dsi_pixel_format_to_bpp(output-
>>>> >dev->format),
>>>>                              nlanes, phy_cfg);
>>>>       if (ret)
>>>>           return ret;
>>>> -    ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode,
>>>> mode_valid_check);
>>>> +    ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
>>>>       if (ret)
>>>>           return ret;
>>>> @@ -635,8 +624,7 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge
>>>> *bridge,
>>>>       struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>>>>       struct cdns_dsi *dsi = input_to_dsi(input);
>>>>       struct cdns_dsi_output *output = &dsi->output;
>>>> -    struct cdns_dsi_cfg dsi_cfg;
>>>> -    int bpp, ret;
>>>> +    int bpp;
>>>>       /*
>>>>        * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
>>>> @@ -654,10 +642,6 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge
>>>> *bridge,
>>>>       if ((mode->hdisplay * bpp) % 32)
>>>>           return MODE_H_ILLEGAL;
>>>> -    ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg, true);
>>>> -    if (ret)
>>>> -        return MODE_BAD;
>>>> -
>>>>       return MODE_OK;
>>>>   }
>>>> @@ -996,7 +980,7 @@ static int cdns_dsi_bridge_atomic_check(struct
>>>> drm_bridge *bridge,
>>>>       adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC |
>>>> DRM_MODE_FLAG_PVSYNC);
>>>>       adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC |
>>>> DRM_MODE_FLAG_NVSYNC;
>>>> -    return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
>>>> +    return cdns_dsi_check_conf(dsi, mode, dsi_cfg);
>>>
>>> With this patch, the driver shifts to using the crtc_* values during the
>>> check phase, and then, it is brought back to using non crtc_* values in
>>> the next patch.
>>>
>>> Should this patch just drop the cdns_dsi_check_conf() check from
>>> .mode_valid() instead, and let the next patch phase out the
>>> mdoe_valid_check parameter as the driver simultaneously shifts to using
>>> the non crtc_* values throughout?
>>
>> Yes.
> 
> Actually, this patch doesn't change the crtc_* vs non-crtc_* behavior.
> After dropping the cdns_dsi_check_conf() call in mode_valid(), the
> 'mode_valid_check' is always false. So this patch removes the parameter,
> and any code paths for the true-case.
> 
> Should the atomic_check() have been using 'true' for the
> 'mode_valid_check'? The atomic_check code was added in
> a53d987756eab40678f241d7cd0eb7e1ca42bba7.
> 

You are right. I had lost a bit of context there.

Upon seeing the change logs, and history for my DSI patches, it seems
that the _atomic_check() was added as a replacement for the check that
was taking place in the _(atomic)_enable(), because the enable-path was
not the right place to do so.

Since the enable-path worked on crtc_* values, the check used to happen
with crtc_* values. And then the _atomic_check() continued to use the
crtc_* values after the patch.

But, since the crtc_* values don't get populated before the bridge's
check-phase, the crtc_* values shouldn't be used at this stage for any
checks.
They are getting populated in this case, via the fbdev_client_setup, if
I understand that right. But it's not right to depend on fbdev as it can
be disabled.

So, it would make sense to use 'true' for the `mode_valid_check`
parameter here. And, I will post a fix for this.


However, we have another question. How would the driver verify the
crtc_* values then, if not during the enable-path? Effectively, it might
not matter for this driver, but there should be a general guideline.


--
Regards
Aradhya

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 10/18] drm/bridge: cdns-dsi: Fix event mode

[Jayesh Choudhary]

Hello Tomi,

On 02/04/25 19:00, Tomi Valkeinen wrote:
> The timings calculation gets it wrong for DSI event mode, resulting in
> too large hbp value. Fix the issue by taking into account the
> pulse/event mode difference.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Jayesh Choudhary <j-choudhary@ti.com>

> ---
>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 33 ++++++++++++++++++--------
>   1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index 62811631341b..9797e6faa29d 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -417,7 +417,8 @@
>   #define DSI_OUTPUT_PORT			0
>   #define DSI_INPUT_PORT(inputid)		(1 + (inputid))
>   
> -#define DSI_HBP_FRAME_OVERHEAD		12
> +#define DSI_HBP_FRAME_PULSE_OVERHEAD	12
> +#define DSI_HBP_FRAME_EVENT_OVERHEAD	16


I have also observed this 4B discrepancy for event-mode after comparing
the dsi packet sizes for each line type as per the dsi specifications.

>   #define DSI_HSA_FRAME_OVERHEAD		14
>   #define DSI_HFP_FRAME_OVERHEAD		6
>   #define DSI_HSS_VSS_VSE_FRAME_OVERHEAD	4
> @@ -503,12 +504,18 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>   
>   	bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
>   
> -	dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp + (sync_pulse ? 0 : dpi_hsa),
> -					 bpp, DSI_HBP_FRAME_OVERHEAD);
> +	if (sync_pulse) {
> +		dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp, bpp,
> +						 DSI_HBP_FRAME_PULSE_OVERHEAD);
>   
> -	if (sync_pulse)
> -		dsi_cfg->hsa =
> -			dpi_to_dsi_timing(dpi_hsa, bpp, DSI_HSA_FRAME_OVERHEAD);
> +		dsi_cfg->hsa = dpi_to_dsi_timing(dpi_hsa, bpp,
> +						 DSI_HSA_FRAME_OVERHEAD);
> +	} else {
> +		dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp + dpi_hsa, bpp,
> +						 DSI_HBP_FRAME_EVENT_OVERHEAD);
> +
> +		dsi_cfg->hsa = 0;
> +	}
>   
>   	dsi_cfg->hact = dpi_to_dsi_timing(dpi_hact, bpp, 0);
>   
> @@ -532,9 +539,12 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>   	unsigned int dsi_hfp_ext;
>   	unsigned int lanes = output->dev->lanes;
>   
> -	dsi_htotal = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
> -	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> +	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) {
> +		dsi_htotal = dsi_cfg->hbp + DSI_HBP_FRAME_PULSE_OVERHEAD;
>   		dsi_htotal += dsi_cfg->hsa + DSI_HSA_FRAME_OVERHEAD;
> +	} else {
> +		dsi_htotal = dsi_cfg->hbp + DSI_HBP_FRAME_EVENT_OVERHEAD;
> +	}
>   
>   	dsi_htotal += dsi_cfg->hact;
>   	dsi_htotal += dsi_cfg->hfp + DSI_HFP_FRAME_OVERHEAD;
> @@ -607,9 +617,12 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
>   		return -EINVAL;
>   	}
>   
> -	dsi_hss_hsa_hse_hbp = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
> -	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> +	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) {
> +		dsi_hss_hsa_hse_hbp = dsi_cfg->hbp + DSI_HBP_FRAME_PULSE_OVERHEAD;
>   		dsi_hss_hsa_hse_hbp += dsi_cfg->hsa + DSI_HSA_FRAME_OVERHEAD;
> +	} else {
> +		dsi_hss_hsa_hse_hbp = dsi_cfg->hbp + DSI_HBP_FRAME_EVENT_OVERHEAD;
> +	}
>   
>   	/*
>   	 * Make sure DPI(HFP) > DSI(HSS+HSA+HSE+HBP) to guarantee that the FIFO
> 

Thanks,
Jayesh

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 00/18] drm/bridge: cdns-dsi: Make it work a bit better

[Jayesh Choudhary]

Hello Tomi,

On 02/04/25 19:00, Tomi Valkeinen wrote:
> While trying to get the cdns-dsi to work on Toradex's AM69 Aquila
> platform, I hit multiple issues in the driver. Basicaly nothing worked
> for with the board.
> 
> This series fixes those issues. While I itch to make much larger changes
> to the cdns-dsi driver, I opted to keep this series relatively simple to
> make the fixes more clear and possibly help with backporting.
> 
> The series also touches tidss, but those changes are not strictly
> needed, and can be merged separately. And the series also touches
> cdns-dphy, and those changes are needed.
> 
> This has been tested on Toradex AM69 Aquila, with:
> - HDMI output using lontium lt8912b
> - LVDS panel (sn65dsi84 + panel-lvds)
> 
>   Tomi
> 

I have tested this series for TI SoCs J784S4 and J721S2 as well where
we use the following pipeline:
DSS -> CDNS_DSI -> SN65DSI86 -> DisplayConnector
(https://lore.kernel.org/all/20250411105155.303657-1-j-choudhary@ti.com/)


Tested-by: Jayesh Choudhary <j-choudhary@ti.com>


> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> Changes in v2:
> - Change the tidss clock adjustment from mode_fixup() to atomic_check()
> - Link to v1: https://lore.kernel.org/r/20250320-cdns-dsi-impro-v1-0-725277c5f43b@ideasonboard.com
> 
> ---
> Tomi Valkeinen (18):
>        drm/tidss: Fix missing includes and struct decls
>        drm/tidss: Use the crtc_* timings when programming the HW
>        drm/tidss: Adjust the pclk based on the HW capabilities
>        phy: cdns-dphy: Store hs_clk_rate and return it
>        phy: cdns-dphy: Remove leftover code
>        drm/bridge: cdns-dsi: Adjust mode to negative syncs
>        drm/bridge: cdns-dsi: Fail if HS rate changed when validating PHY config
>        drm/bridge: cdns-dsi: Clean up cdns_dsi_mode2cfg()
>        drm/bridge: cdns-dsi: Fix REG_WAKEUP_TIME value
>        drm/bridge: cdns-dsi: Fix event mode
>        drm/bridge: cdns-dsi: Remove broken fifo emptying check
>        drm/bridge: cdns-dsi: Drop checks that shouldn't be in .mode_valid()
>        drm/bridge: cdns-dsi: Do not use crtc_* values
>        drm/bridge: cdns-dsi: Use videomode internally
>        drm/bridge: cdns-dsi: Tune adjusted_mode->clock according to dsi needs
>        drm/bridge: cdns-dsi: Update htotal in cdns_dsi_mode2cfg()
>        drm/bridge: cdns-dsi: Drop cdns_dsi_adjust_phy_config()
>        drm/bridge: cdns-dsi: Don't fail on MIPI_DSI_MODE_VIDEO_BURST
> 
>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 224 +++++++++++--------------
>   drivers/gpu/drm/tidss/tidss_crtc.c             |  25 ++-
>   drivers/gpu/drm/tidss/tidss_dispc.c            |  22 ++-
>   drivers/gpu/drm/tidss/tidss_dispc.h            |   5 +
>   drivers/gpu/drm/tidss/tidss_drv.h              |   2 +
>   drivers/gpu/drm/tidss/tidss_plane.h            |   2 +
>   drivers/gpu/drm/tidss/tidss_scale_coefs.h      |   2 +
>   drivers/phy/cadence/cdns-dphy.c                |  24 +--
>   8 files changed, 155 insertions(+), 151 deletions(-)
> ---
> base-commit: 10646ddac2917b31c985ceff0e4982c42a9c924b
> change-id: 20250320-cdns-dsi-impro-3d8fbd7848d1
> prerequisite-message-id: 20250226155228.564289-1-aradhya.bhatia@linux.dev
> prerequisite-patch-id: 46845a8d15042dd343a29a17fc0b9d0eec2605f5
> prerequisite-patch-id: 7ce82c26cb9e18884492d2282a72ff2a760aefda
> prerequisite-patch-id: ec2071425cab81da72e0805ad92fc52731d7a24d
> prerequisite-patch-id: 32cde02288e0c36ed687f67778937a61f78b2d90
> prerequisite-patch-id: 5f302e2bead8994763699a909ad0b5501f09ed9f
> prerequisite-patch-id: 30611df6ef38c7872107d6bf6dde4504d46ab224
> prerequisite-patch-id: 99831bcaa13e25b957d83a6320f34bcec223b939
> prerequisite-patch-id: b0ad38bc6b323ceea7a1d2266b0fab8deaa6b05e
> prerequisite-patch-id: 38dbce2b9302a764be9dbdc551578f02d797dfcc
> prerequisite-patch-id: 133f8b1dab4f47d429b1924df981564ce3736233
> prerequisite-patch-id: 879c546693a53e4b72c1ee25954c894ae57a441f
> prerequisite-patch-id: 3e7edc818ac078a138f0e42e3f47fd685fffb84f
> prerequisite-patch-id: 673b9f0b1936b5a49973b71cab5d13774583de38
> 
> Best regards,

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 03/18] drm/tidss: Adjust the pclk based on the HW capabilities

[Michael Walle]

[-- Attachment #1.1: Type: text/plain, Size: 4578 bytes --]

Hi Tomi,

While testing Aardhya's OLDI support patches [1], I've noticed that
the resulting LVDS clock is wrong if this patch is applied.

> In practice, with the current K3 SoCs, the display PLL is capable of
> producing very exact clocks, so most likely the rounded rate is the same
> as the original one.

This is now what I'm seeing. Most SoCs have that fixed clock thingy
for (some?) VPs, e.g. [2]. And clk_round_rate() will return the
fixed clock rate for this clock, which will then result in an LVDS
clock which is way off.

I'm testing on an AM67A (J722S) and I've backported some of the
patches as well as dtsi fragmets from downstream. Thus, it might be
as well the case that the fixed-factor-clock node is wrong here.
OTOH other K3 SoCs do this in mainline as well.

>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/tidss/tidss_crtc.c  | 23 +++++++++++++++++++----
>  drivers/gpu/drm/tidss/tidss_dispc.c |  6 ++++++
>  drivers/gpu/drm/tidss/tidss_dispc.h |  2 ++
>  3 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 1604eca265ef..6c3967f70510 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -91,7 +91,7 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
>  	struct dispc_device *dispc = tidss->dispc;
>  	struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
>  	u32 hw_videoport = tcrtc->hw_videoport;
> -	const struct drm_display_mode *mode;
> +	struct drm_display_mode *adjusted_mode;
>  	enum drm_mode_status ok;
>  
>  	dev_dbg(ddev->dev, "%s\n", __func__);
> @@ -99,12 +99,27 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
>  	if (!crtc_state->enable)
>  		return 0;
>  
> -	mode = &crtc_state->adjusted_mode;
> +	adjusted_mode = &crtc_state->adjusted_mode;

Here, adjusted_mode->clock is still the correct pixel clock.

> -	ok = dispc_vp_mode_valid(dispc, hw_videoport, mode);
> +	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> +		long rate;
> +
> +		rate = dispc_vp_round_clk_rate(tidss->dispc,
> +					       tcrtc->hw_videoport,
> +					       adjusted_mode->clock * 1000);
> +		if (rate < 0)
> +			return -EINVAL;
> +
> +		adjusted_mode->clock = rate / 1000;

While after this statement, adjusted_mode->clock is 300MHz in my
case (the VP1 clock seems to be 2.1GHz, divided by 7).

-michael

[1] https://lore.kernel.org/all/20250525151721.567042-1-aradhya.bhatia@linux.dev/
[2] https://elixir.bootlin.com/linux/v6.15/source/arch/arm64/boot/dts/ti/k3-am62.dtsi#L110

> +
> +		drm_mode_set_crtcinfo(adjusted_mode, 0);
> +	}
> +
> +	ok = dispc_vp_mode_valid(dispc, hw_videoport, adjusted_mode);
>  	if (ok != MODE_OK) {
>  		dev_dbg(ddev->dev, "%s: bad mode: %ux%u pclk %u kHz\n",
> -			__func__, mode->hdisplay, mode->vdisplay, mode->clock);
> +			__func__, adjusted_mode->hdisplay,
> +			adjusted_mode->vdisplay, adjusted_mode->clock); >  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index a5107f2732b1..3930fb7f03c2 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -1318,6 +1318,12 @@ unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate)
>  	return (unsigned int)(abs(((rr - r) * 100) / r));
>  }
>  
> +long dispc_vp_round_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
> +			     unsigned long rate)
> +{
> +	return clk_round_rate(dispc->vp_clk[hw_videoport], rate);
> +}
> +
>  int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
>  			  unsigned long rate)
>  {
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
> index c31b477a18b0..d4c335e918fb 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -120,6 +120,8 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
>  					 const struct drm_display_mode *mode);
>  int dispc_vp_enable_clk(struct dispc_device *dispc, u32 hw_videoport);
>  void dispc_vp_disable_clk(struct dispc_device *dispc, u32 hw_videoport);
> +long dispc_vp_round_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
> +			     unsigned long rate);
>  int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
>  			  unsigned long rate);
>  void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 03/18] drm/tidss: Adjust the pclk based on the HW capabilities

[Tomi Valkeinen]

Hi Michael (and Aradhya, Devarsh),

On 27/05/2025 12:13, Michael Walle wrote:
> Hi Tomi,
> 
> While testing Aardhya's OLDI support patches [1], I've noticed that
> the resulting LVDS clock is wrong if this patch is applied.
> 
>> In practice, with the current K3 SoCs, the display PLL is capable of
>> producing very exact clocks, so most likely the rounded rate is the same
>> as the original one.
> 
> This is now what I'm seeing. Most SoCs have that fixed clock thingy
> for (some?) VPs, e.g. [2]. And clk_round_rate() will return the
> fixed clock rate for this clock, which will then result in an LVDS
> clock which is way off.
> 
> I'm testing on an AM67A (J722S) and I've backported some of the
> patches as well as dtsi fragmets from downstream. Thus, it might be
> as well the case that the fixed-factor-clock node is wrong here.
> OTOH other K3 SoCs do this in mainline as well.

Thanks for findings this (It's not a fixed clock, but a (fixed)
divider). I can reproduce on my AM62 SK's OLDI output.

I didn't see AM625 TRM explaining the DSS + OLDI clocking. I remember it
was a bit "interesting". Afaics from testing, the VP clock is derived
from the OLDI serial clock divided by 7. To change the VP clock, we need
to set the OLDI clock's rate. But the code we have at the moment is
using clk_round_rate/set_rate to the VP clock.

And we get the crtc atomic_check called before setting the OLDI clock
rate, so it doesn't even work by luck (i.e. if the OLDI clock was set
earlier, the VP clock would already have the right rate, and it would
seem that everything is ok). In the atomic_check we see the OLDI bypass
clock (25 MHz), which results in 3571428 Hz VP clock.

And with this patch, the code then decides that 3571428 Hz is what the
HW can do, and uses it as the pixel clock.

Aradhya, Devarsh, do you remember how the clocking goes here? Or if it's
in the TRM, please point me to it...

 Tomi

>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>  drivers/gpu/drm/tidss/tidss_crtc.c  | 23 +++++++++++++++++++----
>>  drivers/gpu/drm/tidss/tidss_dispc.c |  6 ++++++
>>  drivers/gpu/drm/tidss/tidss_dispc.h |  2 ++
>>  3 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
>> index 1604eca265ef..6c3967f70510 100644
>> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
>> @@ -91,7 +91,7 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
>>  	struct dispc_device *dispc = tidss->dispc;
>>  	struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
>>  	u32 hw_videoport = tcrtc->hw_videoport;
>> -	const struct drm_display_mode *mode;
>> +	struct drm_display_mode *adjusted_mode;
>>  	enum drm_mode_status ok;
>>  
>>  	dev_dbg(ddev->dev, "%s\n", __func__);
>> @@ -99,12 +99,27 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
>>  	if (!crtc_state->enable)
>>  		return 0;
>>  
>> -	mode = &crtc_state->adjusted_mode;
>> +	adjusted_mode = &crtc_state->adjusted_mode;
> 
> Here, adjusted_mode->clock is still the correct pixel clock.
> 
>> -	ok = dispc_vp_mode_valid(dispc, hw_videoport, mode);
>> +	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>> +		long rate;
>> +
>> +		rate = dispc_vp_round_clk_rate(tidss->dispc,
>> +					       tcrtc->hw_videoport,
>> +					       adjusted_mode->clock * 1000);
>> +		if (rate < 0)
>> +			return -EINVAL;
>> +
>> +		adjusted_mode->clock = rate / 1000;
> 
> While after this statement, adjusted_mode->clock is 300MHz in my
> case (the VP1 clock seems to be 2.1GHz, divided by 7).
> 
> -michael
> 
> [1] https://lore.kernel.org/all/20250525151721.567042-1-aradhya.bhatia@linux.dev/
> [2] https://elixir.bootlin.com/linux/v6.15/source/arch/arm64/boot/dts/ti/k3-am62.dtsi#L110
> 
>> +
>> +		drm_mode_set_crtcinfo(adjusted_mode, 0);
>> +	}
>> +
>> +	ok = dispc_vp_mode_valid(dispc, hw_videoport, adjusted_mode);
>>  	if (ok != MODE_OK) {
>>  		dev_dbg(ddev->dev, "%s: bad mode: %ux%u pclk %u kHz\n",
>> -			__func__, mode->hdisplay, mode->vdisplay, mode->clock);
>> +			__func__, adjusted_mode->hdisplay,
>> +			adjusted_mode->vdisplay, adjusted_mode->clock); >  		return -EINVAL;
>>  	}
>>  
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index a5107f2732b1..3930fb7f03c2 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -1318,6 +1318,12 @@ unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate)
>>  	return (unsigned int)(abs(((rr - r) * 100) / r));
>>  }
>>  
>> +long dispc_vp_round_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
>> +			     unsigned long rate)
>> +{
>> +	return clk_round_rate(dispc->vp_clk[hw_videoport], rate);
>> +}
>> +
>>  int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
>>  			  unsigned long rate)
>>  {
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
>> index c31b477a18b0..d4c335e918fb 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>> @@ -120,6 +120,8 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
>>  					 const struct drm_display_mode *mode);
>>  int dispc_vp_enable_clk(struct dispc_device *dispc, u32 hw_videoport);
>>  void dispc_vp_disable_clk(struct dispc_device *dispc, u32 hw_videoport);
>> +long dispc_vp_round_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
>> +			     unsigned long rate);
>>  int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
>>  			  unsigned long rate);
>>  void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 03/18] drm/tidss: Adjust the pclk based on the HW capabilities

[Devarsh Thakkar]

Hi Michael, Tomi,

On 27/05/25 18:03, Tomi Valkeinen wrote:
> Hi Michael (and Aradhya, Devarsh),
> 
> On 27/05/2025 12:13, Michael Walle wrote:
>> Hi Tomi,
>>
>> While testing Aardhya's OLDI support patches [1], I've noticed that
>> the resulting LVDS clock is wrong if this patch is applied.
>>
>>> In practice, with the current K3 SoCs, the display PLL is capable of
>>> producing very exact clocks, so most likely the rounded rate is the same
>>> as the original one.
>>

Yes, display PLL is flexible and device manager should set exact 
frequency. Please note that there was a bug in device manager in earlier 
releases which would prevent setting it to exact clocks. You should try 
with latest SDK release firmware binaries (11.0...(10.1 should also work 
though)) if seeing any misbehaviour in that regard.

>> This is now what I'm seeing. Most SoCs have that fixed clock thingy
>> for (some?) VPs, e.g. [2]. And clk_round_rate() will return the
>> fixed clock rate for this clock, which will then result in an LVDS
>> clock which is way off.
>>
>> I'm testing on an AM67A (J722S) and I've backported some of the
>> patches as well as dtsi fragmets from downstream. Thus, it might be
>> as well the case that the fixed-factor-clock node is wrong here.
>> OTOH other K3 SoCs do this in mainline as well.
> 
> Thanks for findings this (It's not a fixed clock, but a (fixed)
> divider). I can reproduce on my AM62 SK's OLDI output.
> 
> I didn't see AM625 TRM explaining the DSS + OLDI clocking. I remember it
> was a bit "interesting". Afaics from testing, the VP clock is derived
> from the OLDI serial clock divided by 7. To change the VP clock, we need
> to set the OLDI clock's rate. But the code we have at the moment is
> using clk_round_rate/set_rate to the VP clock.
> 

This is correct. The pixel clock is derived as OLDI clock/7 when OLDI is 
enabled.

> And we get the crtc atomic_check called before setting the OLDI clock
> rate, so it doesn't even work by luck (i.e. if the OLDI clock was set
> earlier, the VP clock would already have the right rate, and it would
> seem that everything is ok). In the atomic_check we see the OLDI bypass
> clock (25 MHz), which results in 3571428 Hz VP clock.
> 
> And with this patch, the code then decides that 3571428 Hz is what the
> HW can do, and uses it as the pixel clock.
> 
> Aradhya, Devarsh, do you remember how the clocking goes here? Or if it's
> in the TRM, please point me to it...
> 

I think what you described is correct, if any specific questions I can 
help check. But any misbehaviour you are seeing w.r.t clock setting 
(i.e. what driver is trying to set versus what actually is getting set)
then please dump the dss clock tree along with relevant details of test 
done:

k3conf dump clock <display_device_id>

You can get the device ID via TISCI Doc [1]

[1]: 
https://downloads.ti.com/tisci/esd/latest/5_soc_doc/am62x/clocks.html#clock-for-am62x-device

Regards
Devarsh

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 03/18] drm/tidss: Adjust the pclk based on the HW capabilities

[Michael Walle]

Hi Devarsh, Hi Tomi,

>>> While testing Aardhya's OLDI support patches [1], I've noticed that
>>> the resulting LVDS clock is wrong if this patch is applied.
>>> 
>>>> In practice, with the current K3 SoCs, the display PLL is capable of
>>>> producing very exact clocks, so most likely the rounded rate is the 
>>>> same
>>>> as the original one.
>>> 
> 
> Yes, display PLL is flexible and device manager should set exact 
> frequency.
> Please note that there was a bug in device manager in earlier releases 
> which
> would prevent setting it to exact clocks. You should try with latest 
> SDK
> release firmware binaries (11.0...(10.1 should also work though)) if 
> seeing
> any misbehaviour in that regard.

Yes, I don't doubt that. But it's the way the driver is handling the
clock w.r.t. to the fixed-clock-divider in the LVDS case which is 
causing
problems.

>>> This is now what I'm seeing. Most SoCs have that fixed clock thingy
>>> for (some?) VPs, e.g. [2]. And clk_round_rate() will return the
>>> fixed clock rate for this clock, which will then result in an LVDS
>>> clock which is way off.
>>> 
>>> I'm testing on an AM67A (J722S) and I've backported some of the
>>> patches as well as dtsi fragmets from downstream. Thus, it might be
>>> as well the case that the fixed-factor-clock node is wrong here.
>>> OTOH other K3 SoCs do this in mainline as well.
>> 
>> Thanks for findings this (It's not a fixed clock, but a (fixed)
>> divider). I can reproduce on my AM62 SK's OLDI output.
>> 
>> I didn't see AM625 TRM explaining the DSS + OLDI clocking. I remember 
>> it
>> was a bit "interesting". Afaics from testing, the VP clock is derived
>> from the OLDI serial clock divided by 7. To change the VP clock, we 
>> need
>> to set the OLDI clock's rate. But the code we have at the moment is
>> using clk_round_rate/set_rate to the VP clock.
>> 
> 
> This is correct. The pixel clock is derived as OLDI clock/7 when OLDI 
> is
> enabled.

What means "if OLDI is enabled"? Is there any other clock otherwise or
none at all? Reading the clock ID desciption in the TISCI documentation,
I presume there is no configurable clock for the first VP at all. I.e.
if one compares it with DSS1 which has two muxed clocks, DSS0 only has
one muxed clock (DEV_DSS0_DPI_1_IN_CLK) and DEV_DSS0_DPI_0_IN_CLK is 
fixed,
presumely thats the fixed /7 clock.

Also how is this handled for the DSS1 (on an AM67A)? The TI downstream
kernel also has a fixed-clock-divider for the VP1 on DSS1, but I think
that is wrong, because it's actually configurable (if OLDI is 
disabled?).

>> And we get the crtc atomic_check called before setting the OLDI clock
>> rate, so it doesn't even work by luck (i.e. if the OLDI clock was set
>> earlier, the VP clock would already have the right rate, and it would
>> seem that everything is ok). In the atomic_check we see the OLDI 
>> bypass
>> clock (25 MHz), which results in 3571428 Hz VP clock.
>> 
>> And with this patch, the code then decides that 3571428 Hz is what the
>> HW can do, and uses it as the pixel clock.

FWIW, I'm seeing exactly 300MHz on the AM67A (the FCLK is 2.1GHz 
according
to k3conf).

-michael

>> 
>> Aradhya, Devarsh, do you remember how the clocking goes here? Or if 
>> it's
>> in the TRM, please point me to it...
>> 
> 
> I think what you described is correct, if any specific questions I can 
> help
> check. But any misbehaviour you are seeing w.r.t clock setting (i.e. 
> what
> driver is trying to set versus what actually is getting set)
> then please dump the dss clock tree along with relevant details of test 
> done:
> 
> k3conf dump clock <display_device_id>
> 
> You can get the device ID via TISCI Doc [1]
> 
> [1]: 
> https://downloads.ti.com/tisci/esd/latest/5_soc_doc/am62x/clocks.html#clock-for-am62x-device
> 
> Regards
> Devarsh

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy