public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	David Airlie <airlied@gmail.com>, Simona Vetter <simona@ffwll.ch>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>, Lee Jones <lee@kernel.org>,
	Aradhya Bhatia <aradhya.bhatia@linux.dev>,
	Nishanth Menon <nm@ti.com>, Vignesh Raghavendra <vigneshr@ti.com>,
	Swamil Jain <s-jain1@ti.com>, Devarsh Thakkar <devarsht@ti.com>,
	Louis Chauvet <louis.chauvet@bootlin.com>
Cc: devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Subject: [PATCH 11/15] drm/tidss: oldi: Fix OLDI signal polarities
Date: Mon, 20 Apr 2026 15:54:18 +0300	[thread overview]
Message-ID: <20260420-beagley-ai-display-v1-11-f628543dfd14@ideasonboard.com> (raw)
In-Reply-To: <20260420-beagley-ai-display-v1-0-f628543dfd14@ideasonboard.com>

OLDI has a few issues with how it handles the signal polarities:

- It always sets OLDI_DEPOL, which means DE active low
- It sets DRM_BUS_FLAG_DE_HIGH in struct drm_bridge_timings, i.e.
  reverse to the OLDI_DEPOL
- It sets DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE in struct drm_bridge_timings,
  but the TRM says "The DSS interface is clocked on the rising edge of
  OLDI_FWD_P_CLK pixel clock", which I read as "OLDI samples on rising
  edge".
- But the defined drm_bridge_timings is not actually used anywhere, even
  if it is set to bridge->timings, so the bus flags are just ignored.

However, based on my testing, OLDI_DEPOL bit or the edge on which data
and syncs are driven doesn't seem to affect the OLDI output. Possibly
it's not as robust, but I did not see any effect with an oscilloscope.

However, the code is still quite broken, so let's fix it:
- Remove drm_bridge_timings
- Set the correct input_bus_cfg.flags in tidss_oldi_atomic_check()
- Set OLDI_DEPOL based on the DE bus flag

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_oldi.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_oldi.c b/drivers/gpu/drm/tidss/tidss_oldi.c
index 17c535bfa057..e925ddaa4fd6 100644
--- a/drivers/gpu/drm/tidss/tidss_oldi.c
+++ b/drivers/gpu/drm/tidss/tidss_oldi.c
@@ -164,7 +164,8 @@ static void tidss_oldi_tx_power(struct tidss_oldi *oldi, bool enable)
 	regmap_update_bits(oldi->io_ctrl, OLDI_PD_CTRL, mask, enable ? 0 : mask);
 }
 
-static int tidss_oldi_config(struct tidss_oldi *oldi)
+static int tidss_oldi_config(struct tidss_oldi *oldi,
+			     struct drm_bridge_state *bridge_state)
 {
 	const struct oldi_bus_format *bus_fmt = NULL;
 	u32 oldi_cfg = 0;
@@ -183,7 +184,8 @@ static int tidss_oldi_config(struct tidss_oldi *oldi)
 			 "OLDI%u: DSS port width %d not supported\n",
 			 oldi->oldi_instance, bus_fmt->data_width);
 
-	oldi_cfg |= OLDI_DEPOL;
+	if (bridge_state->input_bus_cfg.flags & DRM_BUS_FLAG_DE_LOW)
+		oldi_cfg |= OLDI_DEPOL; /* 1 = active low */
 
 	oldi_cfg = (oldi_cfg & (~OLDI_MAP)) | (bus_fmt->oldi_mode_reg_val << 1);
 
@@ -220,6 +222,22 @@ static int tidss_oldi_config(struct tidss_oldi *oldi)
 	return ret;
 }
 
+static int tidss_oldi_atomic_check(struct drm_bridge *bridge,
+				   struct drm_bridge_state *bridge_state,
+				   struct drm_crtc_state *crtc_state,
+				   struct drm_connector_state *conn_state)
+{
+	bridge_state->input_bus_cfg.flags &=
+		~(DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
+		  DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE);
+
+	bridge_state->input_bus_cfg.flags |=
+		DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE |
+		DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE;
+
+	return 0;
+}
+
 static void tidss_oldi_atomic_pre_enable(struct drm_bridge *bridge,
 					 struct drm_atomic_state *state)
 {
@@ -228,6 +246,7 @@ static void tidss_oldi_atomic_pre_enable(struct drm_bridge *bridge,
 	struct drm_connector_state *conn_state;
 	struct drm_crtc_state *crtc_state;
 	struct drm_display_mode *mode;
+	struct drm_bridge_state *bridge_state;
 
 	if (oldi->link_type == OLDI_MODE_SECONDARY_CLONE_SINGLE_LINK)
 		return;
@@ -245,10 +264,14 @@ static void tidss_oldi_atomic_pre_enable(struct drm_bridge *bridge,
 	if (WARN_ON(!crtc_state))
 		return;
 
+	bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
+	if (WARN_ON(!bridge_state))
+		return;
+
 	mode = &crtc_state->adjusted_mode;
 
 	/* Configure the OLDI params*/
-	tidss_oldi_config(oldi);
+	tidss_oldi_config(oldi, bridge_state);
 
 	/* Set the OLDI serial clock (7 times the pixel clock) */
 	tidss_oldi_set_serial_clk(oldi, mode->clock * 7 * 1000);
@@ -329,7 +352,8 @@ tidss_oldi_mode_valid(struct drm_bridge *bridge,
 }
 
 static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = {
-	.attach	= tidss_oldi_bridge_attach,
+	.attach = tidss_oldi_bridge_attach,
+	.atomic_check = tidss_oldi_atomic_check,
 	.atomic_pre_enable = tidss_oldi_atomic_pre_enable,
 	.atomic_post_disable = tidss_oldi_atomic_post_disable,
 	.atomic_get_input_bus_fmts = tidss_oldi_atomic_get_input_bus_fmts,
@@ -440,11 +464,6 @@ static int get_parent_dss_vp(struct device_node *oldi_tx, u32 *parent_vp)
 	return -ENODEV;
 }
 
-static const struct drm_bridge_timings default_tidss_oldi_timings = {
-	.input_bus_flags = DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE
-			 | DRM_BUS_FLAG_DE_HIGH,
-};
-
 void tidss_oldi_deinit(struct tidss_device *tidss)
 {
 	for (int i = 0; i < tidss->num_oldis; i++) {
@@ -598,7 +617,6 @@ int tidss_oldi_init(struct tidss_device *tidss)
 		/* Register the bridge. */
 		oldi->bridge.of_node = child;
 		oldi->bridge.driver_private = oldi;
-		oldi->bridge.timings = &default_tidss_oldi_timings;
 
 		tidss->oldis[tidss->num_oldis++] = oldi;
 		tidss->is_ext_vp_clk[oldi->parent_vp] = true;

-- 
2.43.0


  parent reply	other threads:[~2026-04-20 12:55 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-20 12:54 [PATCH 00/15] drm/tidss: Add BeagleY-AI display support (and some more) Tomi Valkeinen
2026-04-20 12:54 ` [PATCH 01/15] dt-bindings: display: ti: Move ti,am62l-dss binding to a new binding file Tomi Valkeinen
2026-04-22 23:45   ` Claude review: " Claude Code Review Bot
2026-04-20 12:54 ` [PATCH 02/15] dt-bindings: display: ti,am65x-dss: Simplify binding Tomi Valkeinen
2026-04-22 23:45   ` Claude review: " Claude Code Review Bot
2026-04-20 12:54 ` [PATCH 03/15] dt-bindings: mfd: syscon: Add ti,am625-dss-dpi0-clk-ctrl compatible Tomi Valkeinen
2026-04-22 23:45   ` Claude review: " Claude Code Review Bot
2026-04-20 12:54 ` [PATCH 04/15] dt-bindings: display: ti,am625-oldi: Add optional power-domain for OLDI Tomi Valkeinen
2026-04-22 23:45   ` Claude review: " Claude Code Review Bot
2026-04-20 12:54 ` [PATCH 05/15] dt-bindings: display: ti,am65x-dss: Add AM62P DSS Tomi Valkeinen
2026-04-22 23:45   ` Claude review: " Claude Code Review Bot
2026-04-20 12:54 ` [PATCH 06/15] drm/tidss: Remove extra pm_runtime_mark_last_busy Tomi Valkeinen
2026-04-22 23:45   ` Claude review: " Claude Code Review Bot
2026-04-20 12:54 ` [PATCH 07/15] drm/tidss: oldi: Remove define for unused register OLDI_LB_CTRL Tomi Valkeinen
2026-04-22 23:45   ` Claude review: " Claude Code Review Bot
2026-04-20 12:54 ` [PATCH 08/15] drm/tidss: Add mechanism to detect DPI output Tomi Valkeinen
2026-04-22 23:45   ` Claude review: " Claude Code Review Bot
2026-04-20 12:54 ` [PATCH 09/15] drm/tidss: Add external data and sync signal edge configuration Tomi Valkeinen
2026-04-22 23:45   ` Claude review: " Claude Code Review Bot
2026-04-20 12:54 ` [PATCH 10/15] drm/tidss: Add support for DPIENABLE bit Tomi Valkeinen
2026-04-22 23:45   ` Claude review: " Claude Code Review Bot
2026-04-20 12:54 ` Tomi Valkeinen [this message]
2026-04-22 23:45   ` Claude review: drm/tidss: oldi: Fix OLDI signal polarities Claude Code Review Bot
2026-04-20 12:54 ` [PATCH 12/15] drm/tidss: oldi: Convert OLDI to an aux driver Tomi Valkeinen
2026-04-22 23:45   ` Claude review: " Claude Code Review Bot
2026-04-20 12:54 ` [PATCH 13/15] drm/tidss: Add support for AM62P display subsystem Tomi Valkeinen
2026-04-22 23:46   ` Claude review: " Claude Code Review Bot
2026-04-20 12:54 ` [PATCH 14/15] arm64: dts: ti: k3-am62p-j722s-common-main: Add support for DSS Tomi Valkeinen
2026-04-22 23:46   ` Claude review: " Claude Code Review Bot
2026-04-20 12:54 ` [PATCH 15/15] arm64: dts: ti: beagley-ai: Enable HDMI display and audio Tomi Valkeinen
2026-04-22 23:46   ` Claude review: " Claude Code Review Bot
2026-04-22 23:45 ` Claude review: drm/tidss: Add BeagleY-AI display support (and some more) Claude Code Review Bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260420-beagley-ai-display-v1-11-f628543dfd14@ideasonboard.com \
    --to=tomi.valkeinen@ideasonboard.com \
    --cc=airlied@gmail.com \
    --cc=aradhya.bhatia@linux.dev \
    --cc=conor+dt@kernel.org \
    --cc=devarsht@ti.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=krzk+dt@kernel.org \
    --cc=lee@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=louis.chauvet@bootlin.com \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mripard@kernel.org \
    --cc=nm@ti.com \
    --cc=robh@kernel.org \
    --cc=s-jain1@ti.com \
    --cc=simona@ffwll.ch \
    --cc=tzimmermann@suse.de \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox