[PATCH v7 0/6] Add DSS support for AM625 SoC

[PATCH v7 0/6] Add DSS support for AM625 SoC

[Aradhya Bhatia]

This patch series adds a new compatible for the Display SubSystem (DSS)
controller on TI's AM625 SoC. It further adds the required support for
the same in the tidss driver.

The AM625-DSS is a newer version of the DSS from the AM65X version with
the major change being the addition of another OLDI TX. With the help of
2 OLDI TXes, the AM625 DSS can support dual-linked OLDI displays with a
resolution of up-to 2K or WUXGA (1920x1200@60fps) at half the OLDI clock
frequency or even cloned video outputs on each of the TXes.

TODO:
  - The pixel clock for the OLDI VP passes through a clock divider, which
    was being explicitly set in previous versions, but that was not the
    right way. That patch was dropped and a newer implementation is in
    works.

Note:
  - I have not picked up Tomi Valkeinen's reviewed-by tag in Patch 5/6
    of this series because I did not implement one of his comments
    which suggested to remove the 'oldi_supported' variable. While the
    oldi support is indeed based on SoC variations, keeping that
    variable helps take into account the case where an OLDI supporting
    SoC by-passes OLDI TXes and gives out DPI video signals straight
    from DSS.

- V6: https://patchwork.freedesktop.org/series/111106/
- V5: https://patchwork.freedesktop.org/series/109194/

Changelog:
V7:
  - Rebase to current linux-next.
  - Address Tomi Valkeinen's comments.
    1. Separate the DSS VP and output port coupling.
       v6 introduced 'output_port_bus_type' in addition to 'vp_bus_type'
       but having both of the variables was redundant. Hence, in v7
       the 'output_port_bus_type' essentially replaces 'vp_bus_type'.
    2. Break Patch v6 2/5 into 2 separate patches (v7 1/6 and v7 3/6).
    3. Change in name and addition of OLDI mode macros.
    4. Other minor changes.

V6:
  - Rebase for current merge window.
  - Add 'allOf:' condition in the DT binding.
  - Address Tomi Valkeinen's comments.
    1. Combine DT binding patches for new compatible and 3rd DSS port.
    2. Further separate DSS VPs and output ports.
    3. Separate OLDI mode discovery logic from the panel/bridge
       discovery (which allowed support for OLDI bridges as well.)
    4. Organize OLDI IO control register macros platform wise.

V5:
  - Rebase for current merge window.
  - Add max DT ports in DSS features.
  - Combine the OLDI support series.

(Changes from OLDI support series v1)
  - Address Tomi Valkeinen's comments.
    1. Update the OLDI link detection approach.
    2. Add port #3 for 2nd OLDI TX.
    3. Configure 2 panel-bridges for cloned panels.
    4. Drop the OLDI clock set patch.
    5. Drop rgb565-to-888 patch.

V4:
  - Rebase for current merge window.
  - Add acked and reviewed by tags.

V3:
  - Change yaml enum in alphabetical order.
  - Correct a typo.

V2:
  - Remove redundant register array.

History:
  - The roots of this patch set can be found in the following OLDI
    support series. -> https://patchwork.freedesktop.org/series/106471/

    The changes in this above-mentioned series forced some re-works in
    the current series, and since all the patches were better understood
    as a single set, both the series were combined in V5, as shown in
    change-log above.

  - The OLDI Support series v1 (and subsequently v5 of the current
    series) couldn't take into account OLDI bridges that worked with
    Clone / Dual Link Mode. That was been rectified in the v6 of the
    series. That became possible because the OLDI mode discovery was
    separated from the panel/bridge discovery loop during modeset
    initialization.

Aradhya Bhatia (6):
  drm/tidss: Remove Video Port to Output Port coupling
  dt-bindings: display: ti,am65x-dss: Add support for am625 dss
  drm/tidss: Add support for AM625 DSS
  drm/tidss: Add support to configure OLDI mode for am625-dss.
  drm/tidss: Add IO CTRL and Power support for OLDI TX in am625
  drm/tidss: Enable Dual and Duplicate Modes for OLDI

 .../bindings/display/ti/ti,am65x-dss.yaml     |  23 +-
 drivers/gpu/drm/tidss/tidss_dispc.c           | 209 +++++++++++++---
 drivers/gpu/drm/tidss/tidss_dispc.h           |  35 ++-
 drivers/gpu/drm/tidss/tidss_dispc_regs.h      |  40 ++-
 drivers/gpu/drm/tidss/tidss_drv.c             |   1 +
 drivers/gpu/drm/tidss/tidss_drv.h             |   8 +-
 drivers/gpu/drm/tidss/tidss_encoder.c         |   4 +-
 drivers/gpu/drm/tidss/tidss_encoder.h         |   3 +-
 drivers/gpu/drm/tidss/tidss_irq.h             |   2 +-
 drivers/gpu/drm/tidss/tidss_kms.c             | 231 ++++++++++++++++--
 10 files changed, 471 insertions(+), 85 deletions(-)

-- 
2.39.0

[PATCH v7 1/6] drm/tidss: Remove Video Port to Output Port coupling

[Aradhya Bhatia]

Make DSS Video Ports agnostic of output bus types.

DSS controllers have had a 1-to-1 coupling between its VPs and its
output ports. This no longer stands true for the new AM625 DSS. This
coupling, hence, has been removed by renaming the 'vp_bus_type' to
'output_port_bus_type' because the VPs are essentially agnostic of the
bus type and it is the output ports which have a bus type.

The AM625 DSS has 2 VPs but requires 3 output ports to support its
Dual-Link OLDI video output coming from a single VP.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------
 drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------
 drivers/gpu/drm/tidss/tidss_drv.h   |  5 +--
 drivers/gpu/drm/tidss/tidss_irq.h   |  2 +-
 drivers/gpu/drm/tidss/tidss_kms.c   | 12 ++++----
 5 files changed, 48 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 165365b515e1..c1c4faccbddc 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = {
 	.min_pclk_khz = 4375,
 
 	.max_pclk_khz = {
-		[DISPC_VP_DPI] = 150000,
+		[DISPC_PORT_DPI] = 150000,
 	},
 
 	/*
@@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = {
 	.vp_name = { "vp1" },
 	.ovr_name = { "ovr1" },
 	.vpclk_name =  { "vp1" },
-	.vp_bus_type = { DISPC_VP_DPI },
 
 	.vp_feat = { .color = {
 			.has_ctm = true,
@@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = {
 	.vid_name = { "vid1" },
 	.vid_lite = { false },
 	.vid_order = { 0 },
+
+	.num_output_ports = 1,
+	.output_port_bus_type = { DISPC_PORT_DPI },
 };
 
 static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
@@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
 
 const struct dispc_features dispc_am65x_feats = {
 	.max_pclk_khz = {
-		[DISPC_VP_DPI] = 165000,
-		[DISPC_VP_OLDI] = 165000,
+		[DISPC_PORT_DPI] = 165000,
+		[DISPC_PORT_OLDI] = 165000,
 	},
 
 	.scaling = {
@@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = {
 	.vp_name = { "vp1", "vp2" },
 	.ovr_name = { "ovr1", "ovr2" },
 	.vpclk_name =  { "vp1", "vp2" },
-	.vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
 
 	.vp_feat = { .color = {
 			.has_ctm = true,
@@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = {
 	.vid_name = { "vid", "vidl1" },
 	.vid_lite = { false, true, },
 	.vid_order = { 1, 0 },
+
+	.num_output_ports = 2,
+	.output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI },
 };
 
 static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
@@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
 
 const struct dispc_features dispc_j721e_feats = {
 	.max_pclk_khz = {
-		[DISPC_VP_DPI] = 170000,
-		[DISPC_VP_INTERNAL] = 600000,
+		[DISPC_PORT_DPI] = 170000,
+		[DISPC_PORT_INTERNAL] = 600000,
 	},
 
 	.scaling = {
@@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = {
 	.vp_name = { "vp1", "vp2", "vp3", "vp4" },
 	.ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" },
 	.vpclk_name = { "vp1", "vp2", "vp3", "vp4" },
-	/* Currently hard coded VP routing (see dispc_initial_config()) */
-	.vp_bus_type =	{ DISPC_VP_INTERNAL, DISPC_VP_DPI,
-			  DISPC_VP_INTERNAL, DISPC_VP_DPI, },
+
 	.vp_feat = { .color = {
 			.has_ctm = true,
 			.gamma_size = 1024,
@@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = {
 	.vid_name = { "vid1", "vidl1", "vid2", "vidl2" },
 	.vid_lite = { 0, 1, 0, 1, },
 	.vid_order = { 1, 3, 0, 2 },
+
+	.num_output_ports = 4,
+	/* Currently hard coded VP routing (see dispc_initial_config()) */
+	.output_port_bus_type =	{ DISPC_PORT_INTERNAL, DISPC_PORT_DPI,
+			  DISPC_PORT_INTERNAL, DISPC_PORT_DPI, },
 };
 
 static const u16 *dispc_common_regmap;
@@ -287,12 +294,12 @@ struct dispc_device {
 
 	void __iomem *base_common;
 	void __iomem *base_vid[TIDSS_MAX_PLANES];
-	void __iomem *base_ovr[TIDSS_MAX_PORTS];
-	void __iomem *base_vp[TIDSS_MAX_PORTS];
+	void __iomem *base_ovr[TIDSS_MAX_VPS];
+	void __iomem *base_vp[TIDSS_MAX_VPS];
 
 	struct regmap *oldi_io_ctrl;
 
-	struct clk *vp_clk[TIDSS_MAX_PORTS];
+	struct clk *vp_clk[TIDSS_MAX_VPS];
 
 	const struct dispc_features *feat;
 
@@ -300,7 +307,7 @@ struct dispc_device {
 
 	bool is_enabled;
 
-	struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
+	struct dss_vp_data vp_data[TIDSS_MAX_VPS];
 
 	u32 *fourccs;
 	u32 num_fourccs;
@@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport,
 		return -EINVAL;
 	}
 
-	if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI &&
+	if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI &&
 	    fmt->is_oldi_fmt) {
 		dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n",
 			__func__, dispc->feat->vp_name[hw_videoport]);
@@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
 	if (WARN_ON(!fmt))
 		return;
 
-	if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) {
+	if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) {
 		dispc_oldi_tx_power(dispc, true);
 
 		dispc_enable_oldi(dispc, hw_videoport, fmt);
@@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
 	align = true;
 
 	/* always use DE_HIGH for OLDI */
-	if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI)
+	if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI)
 		ieo = false;
 
 	dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ,
@@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport)
 
 void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport)
 {
-	if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) {
+	if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) {
 		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0);
 
 		dispc_oldi_tx_power(dispc, false);
@@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
 					 const struct drm_display_mode *mode)
 {
 	u32 hsw, hfp, hbp, vsw, vfp, vbp;
-	enum dispc_vp_bus_type bus_type;
+	enum dispc_port_bus_type bus_type;
 	int max_pclk;
 
-	bus_type = dispc->feat->vp_bus_type[hw_videoport];
+	bus_type = dispc->feat->output_port_bus_type[hw_videoport];
 
 	max_pclk = dispc->feat->max_pclk_khz[bus_type];
 
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index e49432f0abf5..30fb44158347 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -50,11 +50,11 @@ struct dispc_errata {
 	bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */
 };
 
-enum dispc_vp_bus_type {
-	DISPC_VP_DPI,		/* DPI output */
-	DISPC_VP_OLDI,		/* OLDI (LVDS) output */
-	DISPC_VP_INTERNAL,	/* SoC internal routing */
-	DISPC_VP_MAX_BUS_TYPE,
+enum dispc_port_bus_type {
+	DISPC_PORT_DPI,			/* DPI output */
+	DISPC_PORT_OLDI,		/* OLDI (LVDS) output */
+	DISPC_PORT_INTERNAL,		/* SoC internal routing */
+	DISPC_PORT_MAX_BUS_TYPE,
 };
 
 enum dispc_dss_subrevision {
@@ -65,7 +65,7 @@ enum dispc_dss_subrevision {
 
 struct dispc_features {
 	int min_pclk_khz;
-	int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
+	int max_pclk_khz[DISPC_PORT_MAX_BUS_TYPE];
 
 	struct dispc_features_scaling scaling;
 
@@ -74,15 +74,16 @@ struct dispc_features {
 	const char *common;
 	const u16 *common_regs;
 	u32 num_vps;
-	const char *vp_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */
-	const char *ovr_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */
-	const char *vpclk_name[TIDSS_MAX_PORTS]; /* Should match dt clk names */
-	const enum dispc_vp_bus_type vp_bus_type[TIDSS_MAX_PORTS];
+	const char *vp_name[TIDSS_MAX_VPS]; /* Should match dt reg names */
+	const char *ovr_name[TIDSS_MAX_VPS]; /* Should match dt reg names */
+	const char *vpclk_name[TIDSS_MAX_VPS]; /* Should match dt clk names */
 	struct tidss_vp_feat vp_feat;
 	u32 num_planes;
 	const char *vid_name[TIDSS_MAX_PLANES]; /* Should match dt reg names */
 	bool vid_lite[TIDSS_MAX_PLANES];
 	u32 vid_order[TIDSS_MAX_PLANES];
+	u32 num_output_ports;
+	const enum dispc_port_bus_type output_port_bus_type[TIDSS_MAX_OUTPUT_PORTS];
 };
 
 extern const struct dispc_features dispc_k2g_feats;
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
index d7f27b0b0315..0ce7ee5ccd5b 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -9,8 +9,9 @@
 
 #include <linux/spinlock.h>
 
-#define TIDSS_MAX_PORTS 4
+#define TIDSS_MAX_VPS 4
 #define TIDSS_MAX_PLANES 4
+#define TIDSS_MAX_OUTPUT_PORTS 4
 
 typedef u32 dispc_irq_t;
 
@@ -22,7 +23,7 @@ struct tidss_device {
 	struct dispc_device *dispc;
 
 	unsigned int num_crtcs;
-	struct drm_crtc *crtcs[TIDSS_MAX_PORTS];
+	struct drm_crtc *crtcs[TIDSS_MAX_VPS];
 
 	unsigned int num_planes;
 	struct drm_plane *planes[TIDSS_MAX_PLANES];
diff --git a/drivers/gpu/drm/tidss/tidss_irq.h b/drivers/gpu/drm/tidss/tidss_irq.h
index b512614d5863..a753f5e3ce15 100644
--- a/drivers/gpu/drm/tidss/tidss_irq.h
+++ b/drivers/gpu/drm/tidss/tidss_irq.h
@@ -35,7 +35,7 @@
 
 #define DSS_IRQ_VP_BIT_N(ch, bit)	(4 + 4 * (ch) + (bit))
 #define DSS_IRQ_PLANE_BIT_N(plane, bit) \
-	(DSS_IRQ_VP_BIT_N(TIDSS_MAX_PORTS, 0) + 1 * (plane) + (bit))
+	(DSS_IRQ_VP_BIT_N(TIDSS_MAX_VPS, 0) + 1 * (plane) + (bit))
 
 #define DSS_IRQ_VP_BIT(ch, bit)	BIT(DSS_IRQ_VP_BIT_N((ch), (bit)))
 #define DSS_IRQ_PLANE_BIT(plane, bit) \
diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
index ad2fa3c3d4a7..d449131935d2 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -118,16 +118,16 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 	};
 
 	const struct dispc_features *feat = tidss->feat;
-	u32 max_vps = feat->num_vps;
+	u32 output_ports = feat->num_output_ports;
 	u32 max_planes = feat->num_planes;
 
-	struct pipe pipes[TIDSS_MAX_PORTS];
+	struct pipe pipes[TIDSS_MAX_VPS];
 	u32 num_pipes = 0;
 	u32 crtc_mask;
 
 	/* first find all the connected panels & bridges */
 
-	for (i = 0; i < max_vps; i++) {
+	for (i = 0; i < output_ports; i++) {
 		struct drm_panel *panel;
 		struct drm_bridge *bridge;
 		u32 enc_type = DRM_MODE_ENCODER_NONE;
@@ -148,12 +148,12 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 
 			dev_dbg(dev, "Setting up panel for port %d\n", i);
 
-			switch (feat->vp_bus_type[i]) {
-			case DISPC_VP_OLDI:
+			switch (feat->output_port_bus_type[i]) {
+			case DISPC_PORT_OLDI:
 				enc_type = DRM_MODE_ENCODER_LVDS;
 				conn_type = DRM_MODE_CONNECTOR_LVDS;
 				break;
-			case DISPC_VP_DPI:
+			case DISPC_PORT_DPI:
 				enc_type = DRM_MODE_ENCODER_DPI;
 				conn_type = DRM_MODE_CONNECTOR_DPI;
 				break;
-- 
2.39.0

[PATCH v7 2/6] dt-bindings: display: ti,am65x-dss: Add support for am625 dss

[Aradhya Bhatia]

The DSS controller on TI's AM625 SoC is an update from that on TI's
AM65X SoC. The former has an additional OLDI TX on its first video port
(VP0) that helps output cloned video or WUXGA (1920x1200@60fps)
resolution video output over a dual-link mode to reduce the required
OLDI clock output.

Add the new controller's compatible and a port property for the 2nd OLDI
TX (OLDI TX 1).

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Rahul T R <r-ravikumar@ti.com>
---
 .../bindings/display/ti/ti,am65x-dss.yaml     | 23 +++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 5c7d2cbc4aac..55ec91f11577 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -19,7 +19,9 @@ description: |
 
 properties:
   compatible:
-    const: ti,am65x-dss
+    enum:
+      - ti,am625-dss
+      - ti,am65x-dss
 
   reg:
     description:
@@ -80,13 +82,18 @@ properties:
       port@0:
         $ref: /schemas/graph.yaml#/properties/port
         description:
-          The DSS OLDI output port node form video port 1
+          The DSS OLDI output port node form video port 1 (OLDI TX 0).
 
       port@1:
         $ref: /schemas/graph.yaml#/properties/port
         description:
           The DSS DPI output port node from video port 2
 
+      port@2:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          The DSS OLDI output port node form video port 1 (OLDI TX 1).
+
   ti,am65x-oldi-io-ctrl:
     $ref: "/schemas/types.yaml#/definitions/phandle"
     description:
@@ -102,6 +109,18 @@ properties:
       Input memory (from main memory to dispc) bandwidth limit in
       bytes per second
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: ti,am65x-dss
+    then:
+      properties:
+        ports:
+          properties:
+            port@2: false
+
 required:
   - compatible
   - reg
-- 
2.39.0

[PATCH v7 3/6] drm/tidss: Add support for AM625 DSS

[Aradhya Bhatia]

Add support for the DSS controller on TI's new AM625 SoC in the tidss
driver.

The first video port (VP0) in am625-dss can output OLDI signals through
2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus
type.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 57 +++++++++++++++++++++++++++++
 drivers/gpu/drm/tidss/tidss_dispc.h |  2 +
 drivers/gpu/drm/tidss/tidss_drv.c   |  1 +
 3 files changed, 60 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index c1c4faccbddc..b55ccbcaa67f 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -140,6 +140,58 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
 	[DISPC_SECURE_DISABLE_OFF] =		0xac,
 };
 
+const struct dispc_features dispc_am625_feats = {
+	.max_pclk_khz = {
+		[DISPC_PORT_DPI] = 165000,
+		[DISPC_PORT_OLDI] = 165000,
+	},
+
+	.scaling = {
+		.in_width_max_5tap_rgb = 1280,
+		.in_width_max_3tap_rgb = 2560,
+		.in_width_max_5tap_yuv = 2560,
+		.in_width_max_3tap_yuv = 4096,
+		.upscale_limit = 16,
+		.downscale_limit_5tap = 4,
+		.downscale_limit_3tap = 2,
+		/*
+		 * The max supported pixel inc value is 255. The value
+		 * of pixel inc is calculated like this: 1+(xinc-1)*bpp.
+		 * The maximum bpp of all formats supported by the HW
+		 * is 8. So the maximum supported xinc value is 32,
+		 * because 1+(32-1)*8 < 255 < 1+(33-1)*4.
+		 */
+		.xinc_max = 32,
+	},
+
+	.subrev = DISPC_AM625,
+
+	.common = "common",
+	.common_regs = tidss_am65x_common_regs,
+
+	.num_vps = 2,
+	.vp_name = { "vp1", "vp2" },
+	.ovr_name = { "ovr1", "ovr2" },
+	.vpclk_name =  { "vp1", "vp2" },
+
+	.vp_feat = { .color = {
+			.has_ctm = true,
+			.gamma_size = 256,
+			.gamma_type = TIDSS_GAMMA_8BIT,
+		},
+	},
+
+	.num_planes = 2,
+	/* note: vid is plane_id 0 and vidl1 is plane_id 1 */
+	.vid_name = { "vid", "vidl1" },
+	.vid_lite = { false, true, },
+	.vid_order = { 1, 0 },
+
+	/* 3rd output port is not representative of a 3rd pipeline */
+	.num_output_ports = 3,
+	.output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI, DISPC_PORT_OLDI },
+};
+
 const struct dispc_features dispc_am65x_feats = {
 	.max_pclk_khz = {
 		[DISPC_PORT_DPI] = 165000,
@@ -783,6 +835,7 @@ dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc)
 	switch (dispc->feat->subrev) {
 	case DISPC_K2G:
 		return dispc_k2g_read_and_clear_irqstatus(dispc);
+	case DISPC_AM625:
 	case DISPC_AM65X:
 	case DISPC_J721E:
 		return dispc_k3_read_and_clear_irqstatus(dispc);
@@ -798,6 +851,7 @@ void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
 	case DISPC_K2G:
 		dispc_k2g_set_irqenable(dispc, mask);
 		break;
+	case DISPC_AM625:
 	case DISPC_AM65X:
 	case DISPC_J721E:
 		dispc_k3_set_irqenable(dispc, mask);
@@ -1288,6 +1342,7 @@ void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane,
 		dispc_k2g_ovr_set_plane(dispc, hw_plane, hw_videoport,
 					x, y, layer);
 		break;
+	case DISPC_AM625:
 	case DISPC_AM65X:
 		dispc_am65x_ovr_set_plane(dispc, hw_plane, hw_videoport,
 					  x, y, layer);
@@ -2210,6 +2265,7 @@ static void dispc_plane_init(struct dispc_device *dispc)
 	case DISPC_K2G:
 		dispc_k2g_plane_init(dispc);
 		break;
+	case DISPC_AM625:
 	case DISPC_AM65X:
 	case DISPC_J721E:
 		dispc_k3_plane_init(dispc);
@@ -2316,6 +2372,7 @@ static void dispc_vp_write_gamma_table(struct dispc_device *dispc,
 	case DISPC_K2G:
 		dispc_k2g_vp_write_gamma_table(dispc, hw_videoport);
 		break;
+	case DISPC_AM625:
 	case DISPC_AM65X:
 		dispc_am65x_vp_write_gamma_table(dispc, hw_videoport);
 		break;
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index 30fb44158347..971f2856f015 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -59,6 +59,7 @@ enum dispc_port_bus_type {
 
 enum dispc_dss_subrevision {
 	DISPC_K2G,
+	DISPC_AM625,
 	DISPC_AM65X,
 	DISPC_J721E,
 };
@@ -87,6 +88,7 @@ struct dispc_features {
 };
 
 extern const struct dispc_features dispc_k2g_feats;
+extern const struct dispc_features dispc_am625_feats;
 extern const struct dispc_features dispc_am65x_feats;
 extern const struct dispc_features dispc_j721e_feats;
 
diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index 2dac8727d2f4..8558dc6999d8 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -232,6 +232,7 @@ static void tidss_shutdown(struct platform_device *pdev)
 
 static const struct of_device_id tidss_of_table[] = {
 	{ .compatible = "ti,k2g-dss", .data = &dispc_k2g_feats, },
+	{ .compatible = "ti,am625-dss", .data = &dispc_am625_feats, },
 	{ .compatible = "ti,am65x-dss", .data = &dispc_am65x_feats, },
 	{ .compatible = "ti,j721e-dss", .data = &dispc_j721e_feats, },
 	{ }
-- 
2.39.0

[PATCH v7 4/6] drm/tidss: Add support to configure OLDI mode for am625-dss

[Aradhya Bhatia]

The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal.
These can be configured to support the following modes:

1. OLDI_SINGLE_LINK_SINGLE_MODE
Single Output over OLDI 0.
+------+        +---------+      +-------+
|      |        |         |      |       |
| CRTC +------->+ ENCODER +----->| PANEL |
|      |        |         |      |       |
+------+        +---------+      +-------+

2. OLDI_SINGLE_LINK_CLONE_MODE
Duplicate Output over OLDI 0 and 1.
+------+        +---------+      +-------+
|      |        |         |      |       |
| CRTC +---+--->| ENCODER +----->| PANEL |
|      |   |    |         |      |       |
+------+   |    +---------+      +-------+
           |
           |    +---------+      +-------+
           |    |         |      |       |
           +--->| ENCODER +----->| PANEL |
                |         |      |       |
                +---------+      +-------+

3. OLDI_DUAL_LINK_MODE
Combined Output over OLDI 0 and 1.
+------+        +---------+      +-------+
|      |        |         +----->|       |
| CRTC +------->+ ENCODER |      | PANEL |
|      |        |         +----->|       |
+------+        +---------+      +-------+

Following the above pathways for different modes, 2 encoder/panel-bridge
pipes get created for clone mode, and 1 pipe in cases of single link and
dual link mode.

Add support for confguring the OLDI modes using OF and LVDS DRM helper
functions.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c   |  24 ++-
 drivers/gpu/drm/tidss/tidss_dispc.h   |  12 ++
 drivers/gpu/drm/tidss/tidss_drv.h     |   3 +
 drivers/gpu/drm/tidss/tidss_encoder.c |   4 +-
 drivers/gpu/drm/tidss/tidss_encoder.h |   3 +-
 drivers/gpu/drm/tidss/tidss_kms.c     | 221 ++++++++++++++++++++++++--
 6 files changed, 245 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index b55ccbcaa67f..37a73e309330 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -88,6 +88,8 @@ const struct dispc_features dispc_k2g_feats = {
 
 	.subrev = DISPC_K2G,
 
+	.has_oldi = false,
+
 	.common = "common",
 
 	.common_regs = tidss_k2g_common_regs,
@@ -166,6 +168,8 @@ const struct dispc_features dispc_am625_feats = {
 
 	.subrev = DISPC_AM625,
 
+	.has_oldi = true,
+
 	.common = "common",
 	.common_regs = tidss_am65x_common_regs,
 
@@ -218,6 +222,8 @@ const struct dispc_features dispc_am65x_feats = {
 
 	.subrev = DISPC_AM65X,
 
+	.has_oldi = true,
+
 	.common = "common",
 	.common_regs = tidss_am65x_common_regs,
 
@@ -309,6 +315,8 @@ const struct dispc_features dispc_j721e_feats = {
 
 	.subrev = DISPC_J721E,
 
+	.has_oldi = false,
+
 	.common = "common_m",
 	.common_regs = tidss_j721e_common_regs,
 
@@ -361,6 +369,8 @@ struct dispc_device {
 
 	struct dss_vp_data vp_data[TIDSS_MAX_VPS];
 
+	enum dispc_oldi_modes oldi_mode;
+
 	u32 *fourccs;
 	u32 num_fourccs;
 
@@ -1963,6 +1973,12 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len)
 	return dispc->fourccs;
 }
 
+void dispc_set_oldi_mode(struct dispc_device *dispc,
+			 enum dispc_oldi_modes oldi_mode)
+{
+	dispc->oldi_mode = oldi_mode;
+}
+
 static s32 pixinc(int pixels, u8 ps)
 {
 	if (pixels == 1)
@@ -2647,7 +2663,7 @@ int dispc_runtime_resume(struct dispc_device *dispc)
 		REG_GET(dispc, DSS_SYSSTATUS, 2, 2),
 		REG_GET(dispc, DSS_SYSSTATUS, 3, 3));
 
-	if (dispc->feat->subrev == DISPC_AM65X)
+	if (dispc->feat->has_oldi)
 		dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n",
 			REG_GET(dispc, DSS_SYSSTATUS, 5, 5),
 			REG_GET(dispc, DSS_SYSSTATUS, 6, 6),
@@ -2688,7 +2704,7 @@ static int dispc_iomap_resource(struct platform_device *pdev, const char *name,
 	return 0;
 }
 
-static int dispc_init_am65x_oldi_io_ctrl(struct device *dev,
+static int dispc_init_am6xx_oldi_io_ctrl(struct device *dev,
 					 struct dispc_device *dispc)
 {
 	dispc->oldi_io_ctrl =
@@ -2827,8 +2843,8 @@ int dispc_init(struct tidss_device *tidss)
 		dispc->vp_data[i].gamma_table = gamma_table;
 	}
 
-	if (feat->subrev == DISPC_AM65X) {
-		r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
+	if (feat->has_oldi) {
+		r = dispc_init_am6xx_oldi_io_ctrl(dev, dispc);
 		if (r)
 			return r;
 	}
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index 971f2856f015..880bc7de68b3 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -64,6 +64,15 @@ enum dispc_dss_subrevision {
 	DISPC_J721E,
 };
 
+enum dispc_oldi_modes {
+	OLDI_MODE_SINGLE_LINK,		/* Single output over OLDI 0. */
+	OLDI_MODE_CLONE_SINGLE_LINK,	/* Cloned output over OLDI 0 and 1. */
+	OLDI_MODE_DUAL_LINK,		/* Combined output over OLDI 0 and 1. */
+	OLDI_MODE_OFF,			/* OLDI TXes not connected in OF. */
+	OLDI_MODE_UNSUPPORTED,		/* Unsupported OLDI configuration in OF. */
+	OLDI_MODE_UNAVAILABLE,		/* OLDI TXes not available in SoC. */
+};
+
 struct dispc_features {
 	int min_pclk_khz;
 	int max_pclk_khz[DISPC_PORT_MAX_BUS_TYPE];
@@ -72,6 +81,8 @@ struct dispc_features {
 
 	enum dispc_dss_subrevision subrev;
 
+	bool has_oldi;
+
 	const char *common;
 	const u16 *common_regs;
 	u32 num_vps;
@@ -131,6 +142,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
 		      u32 hw_videoport);
 int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable);
 const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len);
+void dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode);
 
 int dispc_init(struct tidss_device *tidss);
 void dispc_remove(struct tidss_device *tidss);
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
index 0ce7ee5ccd5b..58892f065c16 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -13,6 +13,9 @@
 #define TIDSS_MAX_PLANES 4
 #define TIDSS_MAX_OUTPUT_PORTS 4
 
+/* For AM625-DSS with 2 OLDI TXes */
+#define TIDSS_MAX_BRIDGES_PER_PIPE	2
+
 typedef u32 dispc_irq_t;
 
 struct tidss_device {
diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
index 0d4865e9c03d..bd2a7358d7b0 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.c
+++ b/drivers/gpu/drm/tidss/tidss_encoder.c
@@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = {
 };
 
 struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
-					 u32 encoder_type, u32 possible_crtcs)
+					 u32 encoder_type, u32 possible_crtcs,
+					 u32 possible_clones)
 {
 	struct drm_encoder *enc;
 	int ret;
@@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
 		return ERR_PTR(-ENOMEM);
 
 	enc->possible_crtcs = possible_crtcs;
+	enc->possible_clones = possible_clones;
 
 	ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs,
 			       encoder_type, NULL);
diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h
index ace877c0e0fd..01c62ba3ef16 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.h
+++ b/drivers/gpu/drm/tidss/tidss_encoder.h
@@ -12,6 +12,7 @@
 struct tidss_device;
 
 struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
-					 u32 encoder_type, u32 possible_crtcs);
+					 u32 encoder_type, u32 possible_crtcs,
+					 u32 possible_clones);
 
 #endif
diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
index d449131935d2..8322ee6310bf 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -13,6 +13,7 @@
 #include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_vblank.h>
+#include <linux/of.h>
 
 #include "tidss_crtc.h"
 #include "tidss_dispc.h"
@@ -104,26 +105,129 @@ static const struct drm_mode_config_funcs mode_config_funcs = {
 	.atomic_commit = drm_atomic_helper_commit,
 };
 
+static enum dispc_oldi_modes tidss_get_oldi_mode(struct tidss_device *tidss)
+{
+	int pixel_order;
+	enum dispc_oldi_modes oldi_mode;
+	struct device_node *oldi0_port, *oldi1_port;
+
+	/*
+	 * For am625-dss, the OLDI ports are expected at port reg = 0 and 2,
+	 * and for am65x-dss, the OLDI port is expected only at port reg = 0.
+	 */
+	const u32 portnum_oldi0 = 0, portnum_oldi1 = 2;
+
+	oldi0_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi0);
+	oldi1_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi1);
+
+	if (!(oldi0_port || oldi1_port)) {
+		/* Keep OLDI TXes OFF if neither OLDI port is present. */
+		oldi_mode = OLDI_MODE_OFF;
+	} else if (oldi0_port && !oldi1_port) {
+		/*
+		 * OLDI0 port found, but not OLDI1 port. Setting single
+		 * link output mode.
+		 */
+		oldi_mode = OLDI_MODE_SINGLE_LINK;
+	} else if (!oldi0_port && oldi1_port) {
+		/*
+		 * The 2nd OLDI TX cannot be operated alone. This use case is
+		 * not supported in the HW. Since the pins for OLDIs 0 and 1 are
+		 * separate, one could theoretically set a clone mode over OLDIs
+		 * 0 and 1 and just simply not use the OLDI 0. This is a hacky
+		 * way to enable only OLDI TX 1 and hence is not officially
+		 * supported.
+		 */
+		dev_warn(tidss->dev,
+			 "Single Mode over OLDI 1 is not supported in HW.\n");
+		oldi_mode = OLDI_MODE_UNSUPPORTED;
+	} else {
+		/*
+		 * OLDI Ports found for both the OLDI TXes. The DSS is to be
+		 * configured in either Dual Link or Clone Mode.
+		 */
+		pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port,
+								    oldi1_port);
+		switch (pixel_order) {
+		case -EINVAL:
+			/*
+			 * The dual link properties were not found in at least
+			 * one of the sink nodes. Since 2 OLDI ports are present
+			 * in the DT, it can be safely assumed that the required
+			 * configuration is Clone Mode.
+			 */
+			oldi_mode = OLDI_MODE_CLONE_SINGLE_LINK;
+			break;
+
+		case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
+			/*
+			 * Note that the OLDI TX 0 transmits the odd set of
+			 * pixels while the OLDI TX 1 transmits the even set.
+			 * This is a fixed configuration in the HW and an cannot
+			 * be change via SW.
+			 */
+			dev_warn(tidss->dev,
+				 "EVEN-ODD Dual-Link Mode is not supported in HW.\n");
+			oldi_mode = OLDI_MODE_UNSUPPORTED;
+			break;
+
+		case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
+			oldi_mode = OLDI_MODE_DUAL_LINK;
+			break;
+
+		default:
+			oldi_mode = OLDI_MODE_UNSUPPORTED;
+			break;
+		}
+	}
+
+	of_node_put(oldi0_port);
+	of_node_put(oldi1_port);
+
+	return oldi_mode;
+}
+
 static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 {
 	struct device *dev = tidss->dev;
 	unsigned int fourccs_len;
 	const u32 *fourccs = dispc_plane_formats(tidss->dispc, &fourccs_len);
-	unsigned int i;
+	unsigned int i, j;
 
 	struct pipe {
 		u32 hw_videoport;
-		struct drm_bridge *bridge;
+		struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE];
 		u32 enc_type;
+		u32 num_bridges;
 	};
 
 	const struct dispc_features *feat = tidss->feat;
 	u32 output_ports = feat->num_output_ports;
 	u32 max_planes = feat->num_planes;
 
-	struct pipe pipes[TIDSS_MAX_VPS];
+	struct pipe pipes[TIDSS_MAX_VPS] = {0};
+
 	u32 num_pipes = 0;
 	u32 crtc_mask;
+	enum dispc_oldi_modes oldi_mode = OLDI_MODE_UNAVAILABLE;
+	u32 num_oldi = 0;
+	u32 num_encoders = 0;
+	u32 oldi_pipe_index = 0;
+
+	if (feat->has_oldi) {
+		oldi_mode = tidss_get_oldi_mode(tidss);
+
+		if ((oldi_mode == OLDI_MODE_DUAL_LINK ||
+		     oldi_mode == OLDI_MODE_CLONE_SINGLE_LINK) &&
+		    feat->subrev == DISPC_AM65X) {
+			dev_warn(tidss->dev,
+				 "am65x-dss does not support this OLDI mode.\n");
+
+			oldi_mode = OLDI_MODE_UNSUPPORTED;
+		}
+
+		dispc_set_oldi_mode(tidss->dispc, oldi_mode);
+	}
 
 	/* first find all the connected panels & bridges */
 
@@ -179,10 +283,87 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 			}
 		}
 
-		pipes[num_pipes].hw_videoport = i;
-		pipes[num_pipes].bridge = bridge;
-		pipes[num_pipes].enc_type = enc_type;
-		num_pipes++;
+		if (feat->output_port_bus_type[i] == DISPC_PORT_OLDI) {
+			switch (oldi_mode) {
+			case OLDI_MODE_UNSUPPORTED:
+			case OLDI_MODE_OFF:
+				/*
+				 * Either the OLDI ports are not connected in
+				 * OF, or their configuration mode is not
+				 * supported.
+				 * In both the cases, the OLDI sink ports shall
+				 * not be logically connected to DSS ports.
+				 *
+				 * However, since other dss ports might still
+				 * be in use (eg, for DPI), the driver shall
+				 * continue to find the next connected sink in
+				 * OF.
+				 */
+				dev_dbg(dev, "OLDI disconnected on port %d\n", i);
+				continue;
+
+			case OLDI_MODE_DUAL_LINK:
+				/*
+				 * The 2nd OLDI port of a dual-link sink does
+				 * not require a separate bridge entity.
+				 */
+				if (num_oldi) {
+					drm_panel_bridge_remove(bridge);
+					continue;
+				}
+
+				fallthrough;
+
+			case OLDI_MODE_CLONE_SINGLE_LINK:
+			case OLDI_MODE_SINGLE_LINK:
+				/*
+				 * Setting up pipe parameters when 1st OLDI
+				 * port is detected.
+				 */
+				if (!num_oldi) {
+					pipes[num_pipes].hw_videoport = i;
+					pipes[num_pipes].enc_type = enc_type;
+
+					/*
+					 * Saving the pipe index in case its
+					 * required for 2nd OLDI Port.
+					 */
+					oldi_pipe_index = num_pipes;
+
+					/*
+					 * Incrememnt num_pipe when 1st oldi
+					 * port is discovered. For the 2nd OLDI
+					 * port, num_pipe need not be
+					 * incremented because the 2nd
+					 * Encoder-to-Bridge connection will
+					 * still be the part of the first OLDI
+					 * Port pipe.
+					 */
+					num_pipes++;
+				}
+
+				/*
+				 * Bridge is required to be added only if the
+				 * detected port is the first OLDI port (of any
+				 * mode) or a subsequent port in Clone Mode.
+				 */
+				pipes[oldi_pipe_index].bridge[num_oldi] = bridge;
+				pipes[oldi_pipe_index].num_bridges++;
+				num_oldi++;
+				break;
+
+			case OLDI_MODE_UNAVAILABLE:
+			default:
+				dev_dbg(dev, "OLDI unavailable on this device.\n");
+				break;
+			}
+		} else {
+			pipes[num_pipes].hw_videoport = i;
+			pipes[num_pipes].bridge[0] = bridge;
+			pipes[num_pipes].num_bridges++;
+			pipes[num_pipes].enc_type = enc_type;
+			num_pipes++;
+		}
 	}
 
 	/* all planes can be on any crtc */
@@ -194,6 +375,7 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 		struct tidss_plane *tplane;
 		struct tidss_crtc *tcrtc;
 		struct drm_encoder *enc;
+		u32 possible_clones = 0;
 		u32 hw_plane_id = feat->vid_order[tidss->num_planes];
 		int ret;
 
@@ -216,16 +398,23 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 
 		tidss->crtcs[tidss->num_crtcs++] = &tcrtc->crtc;
 
-		enc = tidss_encoder_create(tidss, pipes[i].enc_type,
-					   1 << tcrtc->crtc.index);
-		if (IS_ERR(enc)) {
-			dev_err(tidss->dev, "encoder create failed\n");
-			return PTR_ERR(enc);
-		}
+		possible_clones = (((1 << pipes[i].num_bridges) - 1)
+				   << num_encoders);
 
-		ret = drm_bridge_attach(enc, pipes[i].bridge, NULL, 0);
-		if (ret)
-			return ret;
+		for (j = 0; j < pipes[i].num_bridges; j++) {
+			enc = tidss_encoder_create(tidss, pipes[i].enc_type,
+						   1 << tcrtc->crtc.index,
+						   possible_clones);
+			if (IS_ERR(enc)) {
+				dev_err(tidss->dev, "encoder create failed\n");
+				return PTR_ERR(enc);
+			}
+
+			ret = drm_bridge_attach(enc, pipes[i].bridge[j], NULL, 0);
+			if (ret)
+				return ret;
+		}
+		num_encoders += pipes[i].num_bridges;
 	}
 
 	/* create overlay planes of the leftover planes */
-- 
2.39.0

[PATCH v7 5/6] drm/tidss: Add IO CTRL and Power support for OLDI TX in am625

[Aradhya Bhatia]

The ctrl mmr module of the AM625 is different from the AM65X SoC. Thus
the ctrl mmr registers that supported the OLDI TX power have become
different in AM625 SoC.

The common mode voltage of the LVDS buffers becomes random when the
bandgap reference is turned off. This causes uncertainity in the LVDS
Data and Clock signal outputs, making it behave differently under
different conditions and panel setups. The bandgap reference must be
powered on before using the OLDI IOs, to keep the common voltage trimmed
down to desired levels.

Add support to enable/disable OLDI IO signals as well as the bandgap
reference circuit for the LVDS signals.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---

Note:
- Dropped Tomi Valkeinen's reviewed-by tag in this patch because I did
  not implement one of his comments which suggested to remove the
  'oldi_supported' variable. While the oldi support is indeed based on
  SoC variations, keeping that variable helps take into account the case
  where an OLDI supporting SoC by-passes OLDI TXes and gives out DPI
  video signals straight from DSS.

 drivers/gpu/drm/tidss/tidss_dispc.c      | 57 +++++++++++++++++++-----
 drivers/gpu/drm/tidss/tidss_dispc_regs.h | 40 ++++++++++++-----
 2 files changed, 76 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 37a73e309330..0e03557bc142 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -934,21 +934,56 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport,
 
 static void dispc_oldi_tx_power(struct dispc_device *dispc, bool power)
 {
-	u32 val = power ? 0 : OLDI_PWRDN_TX;
+	u32 val;
 
 	if (WARN_ON(!dispc->oldi_io_ctrl))
 		return;
 
-	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT0_IO_CTRL,
-			   OLDI_PWRDN_TX, val);
-	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT1_IO_CTRL,
-			   OLDI_PWRDN_TX, val);
-	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT2_IO_CTRL,
-			   OLDI_PWRDN_TX, val);
-	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT3_IO_CTRL,
-			   OLDI_PWRDN_TX, val);
-	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_CLK_IO_CTRL,
-			   OLDI_PWRDN_TX, val);
+	if (dispc->feat->subrev == DISPC_AM65X) {
+		val = power ? 0 : AM65X_OLDI_PWRDN_TX;
+
+		regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT0_IO_CTRL,
+				   AM65X_OLDI_PWRDN_TX, val);
+		regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT1_IO_CTRL,
+				   AM65X_OLDI_PWRDN_TX, val);
+		regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT2_IO_CTRL,
+				   AM65X_OLDI_PWRDN_TX, val);
+		regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT3_IO_CTRL,
+				   AM65X_OLDI_PWRDN_TX, val);
+		regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_CLK_IO_CTRL,
+				   AM65X_OLDI_PWRDN_TX, val);
+
+	} else if (dispc->feat->subrev == DISPC_AM625) {
+		if (power) {
+			switch (dispc->oldi_mode) {
+			case OLDI_MODE_SINGLE_LINK:
+				/* Power down OLDI TX 1 */
+				val = AM625_OLDI1_PWRDN_TX;
+				break;
+
+			case OLDI_MODE_CLONE_SINGLE_LINK:
+			case OLDI_MODE_DUAL_LINK:
+				/* No Power down */
+				val = 0;
+				break;
+
+			default:
+				/* Power down both OLDI TXes and LVDS Bandgap */
+				val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX |
+				      AM625_OLDI_PWRDN_BG;
+				break;
+			}
+
+		} else {
+			/* Power down both OLDI TXes and LVDS Bandgap */
+			val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX |
+			      AM625_OLDI_PWRDN_BG;
+		}
+
+		regmap_update_bits(dispc->oldi_io_ctrl, AM625_OLDI_PD_CTRL,
+				   AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX |
+				   AM625_OLDI_PWRDN_BG, val);
+	}
 }
 
 static void dispc_set_num_datalines(struct dispc_device *dispc,
diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
index 13feedfe5d6d..b2a148e96022 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
@@ -227,17 +227,37 @@ enum dispc_common_regs {
 #define DISPC_VP_DSS_DMA_THREADSIZE_STATUS	0x174 /* J721E */
 
 /*
- * OLDI IO_CTRL register offsets. On AM654 the registers are found
- * from CTRL_MMR0, there the syscon regmap should map 0x14 bytes from
- * CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL
- * register range.
+ * OLDI IO and PD CTRL register offsets.
+ * These registers are found in the CTRL_MMR0, where the syscon regmap should map
+ *
+ * 1. 0x14 bytes from CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL
+ * register range for the AM65X DSS, and
+ *
+ * 2. 0x200 bytes from OLDI0_DAT0_IO_CTRL to OLDI_LB_CTRL register range for the
+ * AM625 DSS.
  */
-#define OLDI_DAT0_IO_CTRL			0x00
-#define OLDI_DAT1_IO_CTRL			0x04
-#define OLDI_DAT2_IO_CTRL			0x08
-#define OLDI_DAT3_IO_CTRL			0x0C
-#define OLDI_CLK_IO_CTRL			0x10
 
-#define OLDI_PWRDN_TX				BIT(8)
+/* -- For AM65X OLDI TX -- */
+/* Register offsets */
+#define AM65X_OLDI_DAT0_IO_CTRL			0x00
+#define AM65X_OLDI_DAT1_IO_CTRL			0x04
+#define AM65X_OLDI_DAT2_IO_CTRL			0x08
+#define AM65X_OLDI_DAT3_IO_CTRL			0x0C
+#define AM65X_OLDI_CLK_IO_CTRL			0x10
+
+/* Power control bits */
+#define AM65X_OLDI_PWRDN_TX			BIT(8)
+
+/* -- For AM625 OLDI TX -- */
+/* Register offsets */
+#define AM625_OLDI_PD_CTRL			0x100
+#define AM625_OLDI_LB_CTRL			0x104
+
+/* Power control bits */
+#define AM625_OLDI0_PWRDN_TX			BIT(0)
+#define AM625_OLDI1_PWRDN_TX			BIT(1)
+
+/* LVDS Bandgap reference Enable/Disable */
+#define AM625_OLDI_PWRDN_BG			BIT(8)
 
 #endif /* __TIDSS_DISPC_REGS_H */
-- 
2.39.0

[PATCH v7 6/6] drm/tidss: Enable Dual and Duplicate Modes for OLDI

[Aradhya Bhatia]

The AM625 DSS IP contains 2 OLDI TXes which can work together to enable 2
cloned displays of or even a single dual-link display with higher
resolutions like WUXGA (1920x1200@60fps) with a reduced OLDI clock
frequency.

Configure the necessary register to enable and disable the OLDI TXes
with required modes configurations.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 0e03557bc142..79ad9743a93b 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1020,8 +1020,8 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
 	int count = 0;
 
 	/*
-	 * For the moment DUALMODESYNC, MASTERSLAVE, MODE, and SRC
-	 * bits of DISPC_VP_DSS_OLDI_CFG are set statically to 0.
+	 * For the moment MASTERSLAVE, and SRC bits of DISPC_VP_DSS_OLDI_CFG are
+	 * always set to 0.
 	 */
 
 	if (fmt->data_width == 24)
@@ -1038,6 +1038,26 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
 
 	oldi_cfg |= BIT(0); /* ENABLE */
 
+	switch (dispc->oldi_mode) {
+	case OLDI_MODE_SINGLE_LINK:
+		/* All configuration is done for this mode.  */
+		break;
+
+	case OLDI_MODE_CLONE_SINGLE_LINK:
+		oldi_cfg |= BIT(5); /* CLONE MODE */
+		break;
+
+	case OLDI_MODE_DUAL_LINK:
+		oldi_cfg |= BIT(11); /* DUALMODESYNC */
+		oldi_cfg |= BIT(3); /* data-mapping field also indicates dual-link mode */
+		break;
+
+	default:
+		dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
+			 __func__);
+		return;
+	}
+
 	dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
 
 	while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
-- 
2.39.0

Re: [PATCH v7 2/6] dt-bindings: display: ti,am65x-dss: Add support for am625 dss

[Rob Herring]

On Wed, Jan 25, 2023 at 05:05:25PM +0530, Aradhya Bhatia wrote:
> The DSS controller on TI's AM625 SoC is an update from that on TI's
> AM65X SoC. The former has an additional OLDI TX on its first video port
> (VP0) that helps output cloned video or WUXGA (1920x1200@60fps)
> resolution video output over a dual-link mode to reduce the required
> OLDI clock output.
> 
> Add the new controller's compatible and a port property for the 2nd OLDI
> TX (OLDI TX 1).
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Rahul T R <r-ravikumar@ti.com>
> ---
>  .../bindings/display/ti/ti,am65x-dss.yaml     | 23 +++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 5c7d2cbc4aac..55ec91f11577 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -19,7 +19,9 @@ description: |
>  
>  properties:
>    compatible:
> -    const: ti,am65x-dss
> +    enum:
> +      - ti,am625-dss
> +      - ti,am65x-dss
>  
>    reg:
>      description:
> @@ -80,13 +82,18 @@ properties:
>        port@0:
>          $ref: /schemas/graph.yaml#/properties/port
>          description:
> -          The DSS OLDI output port node form video port 1
> +          The DSS OLDI output port node form video port 1 (OLDI TX 0).

s/form/from/

>  
>        port@1:
>          $ref: /schemas/graph.yaml#/properties/port
>          description:
>            The DSS DPI output port node from video port 2
>  
> +      port@2:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description:
> +          The DSS OLDI output port node form video port 1 (OLDI TX 1).

s/form/from/


> +
>    ti,am65x-oldi-io-ctrl:
>      $ref: "/schemas/types.yaml#/definitions/phandle"
>      description:
> @@ -102,6 +109,18 @@ properties:
>        Input memory (from main memory to dispc) bandwidth limit in
>        bytes per second
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: ti,am65x-dss
> +    then:
> +      properties:
> +        ports:
> +          properties:
> +            port@2: false
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.39.0
> 

Re: [PATCH v7 1/6] drm/tidss: Remove Video Port to Output Port coupling

[Tomi Valkeinen]

On 25/01/2023 13:35, Aradhya Bhatia wrote:
> Make DSS Video Ports agnostic of output bus types.
> 
> DSS controllers have had a 1-to-1 coupling between its VPs and its
> output ports. This no longer stands true for the new AM625 DSS. This
> coupling, hence, has been removed by renaming the 'vp_bus_type' to
> 'output_port_bus_type' because the VPs are essentially agnostic of the
> bus type and it is the output ports which have a bus type.
> 
> The AM625 DSS has 2 VPs but requires 3 output ports to support its
> Dual-Link OLDI video output coming from a single VP.

Not a biggie, but this sentence is a bit odd here at the end. Shouldn't 
it be after the "...stands true for the new AM625 DSS."?

> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------
>   drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------
>   drivers/gpu/drm/tidss/tidss_drv.h   |  5 +--
>   drivers/gpu/drm/tidss/tidss_irq.h   |  2 +-
>   drivers/gpu/drm/tidss/tidss_kms.c   | 12 ++++----
>   5 files changed, 48 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 165365b515e1..c1c4faccbddc 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = {
>   	.min_pclk_khz = 4375,
>   
>   	.max_pclk_khz = {
> -		[DISPC_VP_DPI] = 150000,
> +		[DISPC_PORT_DPI] = 150000,
>   	},
>   
>   	/*
> @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = {
>   	.vp_name = { "vp1" },
>   	.ovr_name = { "ovr1" },
>   	.vpclk_name =  { "vp1" },
> -	.vp_bus_type = { DISPC_VP_DPI },
>   
>   	.vp_feat = { .color = {
>   			.has_ctm = true,
> @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = {
>   	.vid_name = { "vid1" },
>   	.vid_lite = { false },
>   	.vid_order = { 0 },
> +
> +	.num_output_ports = 1,
> +	.output_port_bus_type = { DISPC_PORT_DPI },
>   };

Just thinking out loud, as these will get more complex in the future, 
maybe we should finally group them with struct. E.g. we could define 
struct array for vps, like (just hacky example):

	struct {
		const char *name;
		const char *clkname;
		struct tidss_vp_feat feat;
	} vps[TIDSS_MAX_PORTS];

and then use them as:

	.vps = {
		{
			.name = "kala",
			.clkname = "kissa",
			.feat.color.has_ctm = true,
		}, {
			.name = "kala2",
			.clkname = "kissa2",
			.feat.color.has_ctm = false,
		},
	},

Perhaps something to try in the future.

>   static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
> @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>   
>   const struct dispc_features dispc_am65x_feats = {
>   	.max_pclk_khz = {
> -		[DISPC_VP_DPI] = 165000,
> -		[DISPC_VP_OLDI] = 165000,
> +		[DISPC_PORT_DPI] = 165000,
> +		[DISPC_PORT_OLDI] = 165000,
>   	},
>   
>   	.scaling = {
> @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = {
>   	.vp_name = { "vp1", "vp2" },
>   	.ovr_name = { "ovr1", "ovr2" },
>   	.vpclk_name =  { "vp1", "vp2" },
> -	.vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
>   
>   	.vp_feat = { .color = {
>   			.has_ctm = true,
> @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = {
>   	.vid_name = { "vid", "vidl1" },
>   	.vid_lite = { false, true, },
>   	.vid_order = { 1, 0 },
> +
> +	.num_output_ports = 2,
> +	.output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI },
>   };
>   
>   static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
> @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>   
>   const struct dispc_features dispc_j721e_feats = {
>   	.max_pclk_khz = {
> -		[DISPC_VP_DPI] = 170000,
> -		[DISPC_VP_INTERNAL] = 600000,
> +		[DISPC_PORT_DPI] = 170000,
> +		[DISPC_PORT_INTERNAL] = 600000,
>   	},
>   
>   	.scaling = {
> @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = {
>   	.vp_name = { "vp1", "vp2", "vp3", "vp4" },
>   	.ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" },
>   	.vpclk_name = { "vp1", "vp2", "vp3", "vp4" },
> -	/* Currently hard coded VP routing (see dispc_initial_config()) */
> -	.vp_bus_type =	{ DISPC_VP_INTERNAL, DISPC_VP_DPI,
> -			  DISPC_VP_INTERNAL, DISPC_VP_DPI, },
> +

I think this line feed is extra.

>   	.vp_feat = { .color = {
>   			.has_ctm = true,
>   			.gamma_size = 1024,
> @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = {
>   	.vid_name = { "vid1", "vidl1", "vid2", "vidl2" },
>   	.vid_lite = { 0, 1, 0, 1, },
>   	.vid_order = { 1, 3, 0, 2 },
> +
> +	.num_output_ports = 4,
> +	/* Currently hard coded VP routing (see dispc_initial_config()) */
> +	.output_port_bus_type =	{ DISPC_PORT_INTERNAL, DISPC_PORT_DPI,
> +			  DISPC_PORT_INTERNAL, DISPC_PORT_DPI, },

Indent doesn't look right (but it might be just because this is a diff).

>   };
>   
>   static const u16 *dispc_common_regmap;
> @@ -287,12 +294,12 @@ struct dispc_device {
>   
>   	void __iomem *base_common;
>   	void __iomem *base_vid[TIDSS_MAX_PLANES];
> -	void __iomem *base_ovr[TIDSS_MAX_PORTS];
> -	void __iomem *base_vp[TIDSS_MAX_PORTS];
> +	void __iomem *base_ovr[TIDSS_MAX_VPS];
> +	void __iomem *base_vp[TIDSS_MAX_VPS];
>   
>   	struct regmap *oldi_io_ctrl;
>   
> -	struct clk *vp_clk[TIDSS_MAX_PORTS];
> +	struct clk *vp_clk[TIDSS_MAX_VPS];
>   
>   	const struct dispc_features *feat;
>   
> @@ -300,7 +307,7 @@ struct dispc_device {
>   
>   	bool is_enabled;
>   
> -	struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
> +	struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>   
>   	u32 *fourccs;
>   	u32 num_fourccs;
> @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport,
>   		return -EINVAL;
>   	}
>   
> -	if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI &&
> +	if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI &&

Hmm, so is the hw_videoport a vp index or an output index? Sounds like 
the former, so it's not right, even if at the moment they're identical. 
We need some kind of mapping between those.

If the mapping can be changed (or just defined in the DT), I think we 
need a variable in struct dispc_device, which tells the output to which 
a videoport is connected to. Or vice versa, I'm not sure which direction 
we need more. If the mapping is always the same on all SoC (but I don't 
think so), we can have it in the feats.

Also, I wonder if output_port is a good name as it has "port" in it 
(like video port), and it's a bit long-ish. Would just "output" be 
enough? We could, of course, shorten it to OP, but that looks odd to me =).

>   	    fmt->is_oldi_fmt) {
>   		dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n",
>   			__func__, dispc->feat->vp_name[hw_videoport]);
> @@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
>   	if (WARN_ON(!fmt))
>   		return;
>   
> -	if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) {
> +	if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) {
>   		dispc_oldi_tx_power(dispc, true);
>   
>   		dispc_enable_oldi(dispc, hw_videoport, fmt);
> @@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
>   	align = true;
>   
>   	/* always use DE_HIGH for OLDI */
> -	if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI)
> +	if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI)
>   		ieo = false;
>   
>   	dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ,
> @@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport)
>   
>   void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport)
>   {
> -	if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) {
> +	if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) {
>   		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0);
>   
>   		dispc_oldi_tx_power(dispc, false);
> @@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
>   					 const struct drm_display_mode *mode)
>   {
>   	u32 hsw, hfp, hbp, vsw, vfp, vbp;
> -	enum dispc_vp_bus_type bus_type;
> +	enum dispc_port_bus_type bus_type;
>   	int max_pclk;
>   
> -	bus_type = dispc->feat->vp_bus_type[hw_videoport];
> +	bus_type = dispc->feat->output_port_bus_type[hw_videoport];
>   
>   	max_pclk = dispc->feat->max_pclk_khz[bus_type];
>   
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
> index e49432f0abf5..30fb44158347 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -50,11 +50,11 @@ struct dispc_errata {
>   	bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */
>   };
>   
> -enum dispc_vp_bus_type {
> -	DISPC_VP_DPI,		/* DPI output */
> -	DISPC_VP_OLDI,		/* OLDI (LVDS) output */
> -	DISPC_VP_INTERNAL,	/* SoC internal routing */
> -	DISPC_VP_MAX_BUS_TYPE,
> +enum dispc_port_bus_type {
> +	DISPC_PORT_DPI,			/* DPI output */
> +	DISPC_PORT_OLDI,		/* OLDI (LVDS) output */
> +	DISPC_PORT_INTERNAL,		/* SoC internal routing */
> +	DISPC_PORT_MAX_BUS_TYPE,

Okay, so here you have just "port", not "output_port". In the DT, 
they're ports, so... Maybe we could use that name too, and for video 
port always use "vp". The current "hw_videoport" could be easily 
mistaken with "port".

I don't recall why I chose to use "hw" prefix there. I think I wanted to 
separate it from some other videoport, but... I don't know what that 
"other" is =).

  Tomi

Re: [PATCH v7 4/6] drm/tidss: Add support to configure OLDI mode for am625-dss

[Tomi Valkeinen]

On 25/01/2023 13:35, Aradhya Bhatia wrote:
> The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal.
> These can be configured to support the following modes:
> 
> 1. OLDI_SINGLE_LINK_SINGLE_MODE
> Single Output over OLDI 0.
> +------+        +---------+      +-------+
> |      |        |         |      |       |
> | CRTC +------->+ ENCODER +----->| PANEL |
> |      |        |         |      |       |
> +------+        +---------+      +-------+
> 
> 2. OLDI_SINGLE_LINK_CLONE_MODE
> Duplicate Output over OLDI 0 and 1.
> +------+        +---------+      +-------+
> |      |        |         |      |       |
> | CRTC +---+--->| ENCODER +----->| PANEL |
> |      |   |    |         |      |       |
> +------+   |    +---------+      +-------+
>             |
>             |    +---------+      +-------+
>             |    |         |      |       |
>             +--->| ENCODER +----->| PANEL |
>                  |         |      |       |
>                  +---------+      +-------+
> 
> 3. OLDI_DUAL_LINK_MODE
> Combined Output over OLDI 0 and 1.
> +------+        +---------+      +-------+
> |      |        |         +----->|       |
> | CRTC +------->+ ENCODER |      | PANEL |
> |      |        |         +----->|       |
> +------+        +---------+      +-------+
> 
> Following the above pathways for different modes, 2 encoder/panel-bridge
> pipes get created for clone mode, and 1 pipe in cases of single link and
> dual link mode.
> 
> Add support for confguring the OLDI modes using OF and LVDS DRM helper
> functions.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c   |  24 ++-
>   drivers/gpu/drm/tidss/tidss_dispc.h   |  12 ++
>   drivers/gpu/drm/tidss/tidss_drv.h     |   3 +
>   drivers/gpu/drm/tidss/tidss_encoder.c |   4 +-
>   drivers/gpu/drm/tidss/tidss_encoder.h |   3 +-
>   drivers/gpu/drm/tidss/tidss_kms.c     | 221 ++++++++++++++++++++++++--
>   6 files changed, 245 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index b55ccbcaa67f..37a73e309330 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -88,6 +88,8 @@ const struct dispc_features dispc_k2g_feats = {
>   
>   	.subrev = DISPC_K2G,
>   
> +	.has_oldi = false,
> +
>   	.common = "common",
>   
>   	.common_regs = tidss_k2g_common_regs,
> @@ -166,6 +168,8 @@ const struct dispc_features dispc_am625_feats = {
>   
>   	.subrev = DISPC_AM625,
>   
> +	.has_oldi = true,
> +
>   	.common = "common",
>   	.common_regs = tidss_am65x_common_regs,
>   
> @@ -218,6 +222,8 @@ const struct dispc_features dispc_am65x_feats = {
>   
>   	.subrev = DISPC_AM65X,
>   
> +	.has_oldi = true,
> +
>   	.common = "common",
>   	.common_regs = tidss_am65x_common_regs,
>   
> @@ -309,6 +315,8 @@ const struct dispc_features dispc_j721e_feats = {
>   
>   	.subrev = DISPC_J721E,
>   
> +	.has_oldi = false,
> +
>   	.common = "common_m",
>   	.common_regs = tidss_j721e_common_regs,
>   
> @@ -361,6 +369,8 @@ struct dispc_device {
>   
>   	struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>   
> +	enum dispc_oldi_modes oldi_mode;
> +
>   	u32 *fourccs;
>   	u32 num_fourccs;
>   
> @@ -1963,6 +1973,12 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len)
>   	return dispc->fourccs;
>   }
>   
> +void dispc_set_oldi_mode(struct dispc_device *dispc,
> +			 enum dispc_oldi_modes oldi_mode)
> +{
> +	dispc->oldi_mode = oldi_mode;
> +}
> +
>   static s32 pixinc(int pixels, u8 ps)
>   {
>   	if (pixels == 1)
> @@ -2647,7 +2663,7 @@ int dispc_runtime_resume(struct dispc_device *dispc)
>   		REG_GET(dispc, DSS_SYSSTATUS, 2, 2),
>   		REG_GET(dispc, DSS_SYSSTATUS, 3, 3));
>   
> -	if (dispc->feat->subrev == DISPC_AM65X)
> +	if (dispc->feat->has_oldi)
>   		dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n",
>   			REG_GET(dispc, DSS_SYSSTATUS, 5, 5),
>   			REG_GET(dispc, DSS_SYSSTATUS, 6, 6),
> @@ -2688,7 +2704,7 @@ static int dispc_iomap_resource(struct platform_device *pdev, const char *name,
>   	return 0;
>   }
>   
> -static int dispc_init_am65x_oldi_io_ctrl(struct device *dev,
> +static int dispc_init_am6xx_oldi_io_ctrl(struct device *dev,
>   					 struct dispc_device *dispc)
>   {
>   	dispc->oldi_io_ctrl =
> @@ -2827,8 +2843,8 @@ int dispc_init(struct tidss_device *tidss)
>   		dispc->vp_data[i].gamma_table = gamma_table;
>   	}
>   
> -	if (feat->subrev == DISPC_AM65X) {
> -		r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
> +	if (feat->has_oldi) {
> +		r = dispc_init_am6xx_oldi_io_ctrl(dev, dispc);
>   		if (r)
>   			return r;
>   	}
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
> index 971f2856f015..880bc7de68b3 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -64,6 +64,15 @@ enum dispc_dss_subrevision {
>   	DISPC_J721E,
>   };
>   
> +enum dispc_oldi_modes {
> +	OLDI_MODE_SINGLE_LINK,		/* Single output over OLDI 0. */
> +	OLDI_MODE_CLONE_SINGLE_LINK,	/* Cloned output over OLDI 0 and 1. */
> +	OLDI_MODE_DUAL_LINK,		/* Combined output over OLDI 0 and 1. */
> +	OLDI_MODE_OFF,			/* OLDI TXes not connected in OF. */
> +	OLDI_MODE_UNSUPPORTED,		/* Unsupported OLDI configuration in OF. */
> +	OLDI_MODE_UNAVAILABLE,		/* OLDI TXes not available in SoC. */
> +};
> +
>   struct dispc_features {
>   	int min_pclk_khz;
>   	int max_pclk_khz[DISPC_PORT_MAX_BUS_TYPE];
> @@ -72,6 +81,8 @@ struct dispc_features {
>   
>   	enum dispc_dss_subrevision subrev;
>   
> +	bool has_oldi;
> +
>   	const char *common;
>   	const u16 *common_regs;
>   	u32 num_vps;
> @@ -131,6 +142,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
>   		      u32 hw_videoport);
>   int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable);
>   const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len);
> +void dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode);
>   
>   int dispc_init(struct tidss_device *tidss);
>   void dispc_remove(struct tidss_device *tidss);
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
> index 0ce7ee5ccd5b..58892f065c16 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.h
> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
> @@ -13,6 +13,9 @@
>   #define TIDSS_MAX_PLANES 4
>   #define TIDSS_MAX_OUTPUT_PORTS 4
>   
> +/* For AM625-DSS with 2 OLDI TXes */
> +#define TIDSS_MAX_BRIDGES_PER_PIPE	2
> +
>   typedef u32 dispc_irq_t;
>   
>   struct tidss_device {
> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
> index 0d4865e9c03d..bd2a7358d7b0 100644
> --- a/drivers/gpu/drm/tidss/tidss_encoder.c
> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
> @@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = {
>   };
>   
>   struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
> -					 u32 encoder_type, u32 possible_crtcs)
> +					 u32 encoder_type, u32 possible_crtcs,
> +					 u32 possible_clones)
>   {
>   	struct drm_encoder *enc;
>   	int ret;
> @@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>   		return ERR_PTR(-ENOMEM);
>   
>   	enc->possible_crtcs = possible_crtcs;
> +	enc->possible_clones = possible_clones;
>   
>   	ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs,
>   			       encoder_type, NULL);
> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h
> index ace877c0e0fd..01c62ba3ef16 100644
> --- a/drivers/gpu/drm/tidss/tidss_encoder.h
> +++ b/drivers/gpu/drm/tidss/tidss_encoder.h
> @@ -12,6 +12,7 @@
>   struct tidss_device;
>   
>   struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
> -					 u32 encoder_type, u32 possible_crtcs);
> +					 u32 encoder_type, u32 possible_crtcs,
> +					 u32 possible_clones);
>   
>   #endif
> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index d449131935d2..8322ee6310bf 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> @@ -13,6 +13,7 @@
>   #include <drm/drm_of.h>
>   #include <drm/drm_panel.h>
>   #include <drm/drm_vblank.h>
> +#include <linux/of.h>
>   
>   #include "tidss_crtc.h"
>   #include "tidss_dispc.h"
> @@ -104,26 +105,129 @@ static const struct drm_mode_config_funcs mode_config_funcs = {
>   	.atomic_commit = drm_atomic_helper_commit,
>   };
>   
> +static enum dispc_oldi_modes tidss_get_oldi_mode(struct tidss_device *tidss)
> +{
> +	int pixel_order;
> +	enum dispc_oldi_modes oldi_mode;
> +	struct device_node *oldi0_port, *oldi1_port;
> +
> +	/*
> +	 * For am625-dss, the OLDI ports are expected at port reg = 0 and 2,
> +	 * and for am65x-dss, the OLDI port is expected only at port reg = 0.
> +	 */
> +	const u32 portnum_oldi0 = 0, portnum_oldi1 = 2;
> +
> +	oldi0_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi0);
> +	oldi1_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi1);
> +
> +	if (!(oldi0_port || oldi1_port)) {
> +		/* Keep OLDI TXes OFF if neither OLDI port is present. */
> +		oldi_mode = OLDI_MODE_OFF;
> +	} else if (oldi0_port && !oldi1_port) {
> +		/*
> +		 * OLDI0 port found, but not OLDI1 port. Setting single
> +		 * link output mode.
> +		 */
> +		oldi_mode = OLDI_MODE_SINGLE_LINK;
> +	} else if (!oldi0_port && oldi1_port) {
> +		/*
> +		 * The 2nd OLDI TX cannot be operated alone. This use case is
> +		 * not supported in the HW. Since the pins for OLDIs 0 and 1 are
> +		 * separate, one could theoretically set a clone mode over OLDIs
> +		 * 0 and 1 and just simply not use the OLDI 0. This is a hacky
> +		 * way to enable only OLDI TX 1 and hence is not officially
> +		 * supported.
> +		 */
> +		dev_warn(tidss->dev,
> +			 "Single Mode over OLDI 1 is not supported in HW.\n");
> +		oldi_mode = OLDI_MODE_UNSUPPORTED;
> +	} else {
> +		/*
> +		 * OLDI Ports found for both the OLDI TXes. The DSS is to be
> +		 * configured in either Dual Link or Clone Mode.
> +		 */
> +		pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port,
> +								    oldi1_port);
> +		switch (pixel_order) {
> +		case -EINVAL:
> +			/*
> +			 * The dual link properties were not found in at least
> +			 * one of the sink nodes. Since 2 OLDI ports are present
> +			 * in the DT, it can be safely assumed that the required
> +			 * configuration is Clone Mode.
> +			 */
> +			oldi_mode = OLDI_MODE_CLONE_SINGLE_LINK;
> +			break;
> +
> +		case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
> +			/*
> +			 * Note that the OLDI TX 0 transmits the odd set of
> +			 * pixels while the OLDI TX 1 transmits the even set.
> +			 * This is a fixed configuration in the HW and an cannot
> +			 * be change via SW.
> +			 */
> +			dev_warn(tidss->dev,
> +				 "EVEN-ODD Dual-Link Mode is not supported in HW.\n");
> +			oldi_mode = OLDI_MODE_UNSUPPORTED;
> +			break;
> +
> +		case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
> +			oldi_mode = OLDI_MODE_DUAL_LINK;
> +			break;
> +
> +		default:
> +			oldi_mode = OLDI_MODE_UNSUPPORTED;
> +			break;
> +		}
> +	}
> +
> +	of_node_put(oldi0_port);
> +	of_node_put(oldi1_port);
> +
> +	return oldi_mode;
> +}
> +
>   static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>   {
>   	struct device *dev = tidss->dev;
>   	unsigned int fourccs_len;
>   	const u32 *fourccs = dispc_plane_formats(tidss->dispc, &fourccs_len);
> -	unsigned int i;
> +	unsigned int i, j;
>   
>   	struct pipe {
>   		u32 hw_videoport;
> -		struct drm_bridge *bridge;
> +		struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE];
>   		u32 enc_type;
> +		u32 num_bridges;
>   	};
>   
>   	const struct dispc_features *feat = tidss->feat;
>   	u32 output_ports = feat->num_output_ports;
>   	u32 max_planes = feat->num_planes;
>   
> -	struct pipe pipes[TIDSS_MAX_VPS];
> +	struct pipe pipes[TIDSS_MAX_VPS] = {0};
> +
>   	u32 num_pipes = 0;
>   	u32 crtc_mask;
> +	enum dispc_oldi_modes oldi_mode = OLDI_MODE_UNAVAILABLE;
> +	u32 num_oldi = 0;
> +	u32 num_encoders = 0;
> +	u32 oldi_pipe_index = 0;
> +
> +	if (feat->has_oldi) {
> +		oldi_mode = tidss_get_oldi_mode(tidss);
> +
> +		if ((oldi_mode == OLDI_MODE_DUAL_LINK ||
> +		     oldi_mode == OLDI_MODE_CLONE_SINGLE_LINK) &&
> +		    feat->subrev == DISPC_AM65X) {
> +			dev_warn(tidss->dev,
> +				 "am65x-dss does not support this OLDI mode.\n");
> +
> +			oldi_mode = OLDI_MODE_UNSUPPORTED;
> +		}

Shouldn't OLDI_MODE_UNSUPPORTED be handled as an error? It means the DT 
is faulty, doesn't it? Maybe it could even be renamed to 
OLDI_MODE_ERROR. Or tidss_get_oldi_mode() could return a negative error 
code.

> +
> +		dispc_set_oldi_mode(tidss->dispc, oldi_mode);
> +	}

Would it be better to move the above dispc_set_oldi_mode() to be outside 
the if block? Then oldi mode would be set to OLDI_MODE_UNAVAILABLE on 
SoCs that don't have OLDI.

tidss_get_oldi_mode and dispc_set_oldi_mode sound like opposites, but 
they're totally different things. Maybe tidss_get_oldi_mode should 
rather be something about parsing oldi dt properties or such.

>   	/* first find all the connected panels & bridges */
>   
> @@ -179,10 +283,87 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>   			}
>   		}
>   
> -		pipes[num_pipes].hw_videoport = i;
> -		pipes[num_pipes].bridge = bridge;
> -		pipes[num_pipes].enc_type = enc_type;
> -		num_pipes++;
> +		if (feat->output_port_bus_type[i] == DISPC_PORT_OLDI) {
> +			switch (oldi_mode) {
> +			case OLDI_MODE_UNSUPPORTED:
> +			case OLDI_MODE_OFF:
> +				/*
> +				 * Either the OLDI ports are not connected in
> +				 * OF, or their configuration mode is not
> +				 * supported.
> +				 * In both the cases, the OLDI sink ports shall
> +				 * not be logically connected to DSS ports.
> +				 *
> +				 * However, since other dss ports might still
> +				 * be in use (eg, for DPI), the driver shall
> +				 * continue to find the next connected sink in
> +				 * OF.
> +				 */
> +				dev_dbg(dev, "OLDI disconnected on port %d\n", i);
> +				continue;
> +
> +			case OLDI_MODE_DUAL_LINK:
> +				/*
> +				 * The 2nd OLDI port of a dual-link sink does
> +				 * not require a separate bridge entity.
> +				 */
> +				if (num_oldi) {

I think if (num_oldi > 0) makes it more readable.

> +					drm_panel_bridge_remove(bridge);
> +					continue;
> +				}
> +
> +				fallthrough;
> +
> +			case OLDI_MODE_CLONE_SINGLE_LINK:
> +			case OLDI_MODE_SINGLE_LINK:
> +				/*
> +				 * Setting up pipe parameters when 1st OLDI
> +				 * port is detected.
> +				 */
> +				if (!num_oldi) {

And here if (num_oldi == 0).

> +					pipes[num_pipes].hw_videoport = i;
> +					pipes[num_pipes].enc_type = enc_type;
> +
> +					/*
> +					 * Saving the pipe index in case its
> +					 * required for 2nd OLDI Port.
> +					 */
> +					oldi_pipe_index = num_pipes;
> +
> +					/*
> +					 * Incrememnt num_pipe when 1st oldi
> +					 * port is discovered. For the 2nd OLDI
> +					 * port, num_pipe need not be
> +					 * incremented because the 2nd
> +					 * Encoder-to-Bridge connection will
> +					 * still be the part of the first OLDI
> +					 * Port pipe.
> +					 */
> +					num_pipes++;
> +				}
> +
> +				/*
> +				 * Bridge is required to be added only if the
> +				 * detected port is the first OLDI port (of any
> +				 * mode) or a subsequent port in Clone Mode.
> +				 */

If I understand right, you're saying that bridge is not required to be 
added for the second port for dual link? It maybe be clearer to state 
that. But if so, isn't this still always adding the bridge here, for all 
modes? Or what is the comment referring to?

  Tomi

Re: [PATCH v7 5/6] drm/tidss: Add IO CTRL and Power support for OLDI TX in am625

[Tomi Valkeinen]

On 25/01/2023 13:35, Aradhya Bhatia wrote:
> The ctrl mmr module of the AM625 is different from the AM65X SoC. Thus
> the ctrl mmr registers that supported the OLDI TX power have become
> different in AM625 SoC.
> 
> The common mode voltage of the LVDS buffers becomes random when the
> bandgap reference is turned off. This causes uncertainity in the LVDS
> Data and Clock signal outputs, making it behave differently under
> different conditions and panel setups. The bandgap reference must be
> powered on before using the OLDI IOs, to keep the common voltage trimmed
> down to desired levels.
> 
> Add support to enable/disable OLDI IO signals as well as the bandgap
> reference circuit for the LVDS signals.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
> 
> Note:
> - Dropped Tomi Valkeinen's reviewed-by tag in this patch because I did
>    not implement one of his comments which suggested to remove the
>    'oldi_supported' variable. While the oldi support is indeed based on
>    SoC variations, keeping that variable helps take into account the case
>    where an OLDI supporting SoC by-passes OLDI TXes and gives out DPI
>    video signals straight from DSS.

Hmm why is that relevent for this patch? It doesn't use oldi_supported 
or the new has_oldi.

>   drivers/gpu/drm/tidss/tidss_dispc.c      | 57 +++++++++++++++++++-----
>   drivers/gpu/drm/tidss/tidss_dispc_regs.h | 40 ++++++++++++-----
>   2 files changed, 76 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 37a73e309330..0e03557bc142 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -934,21 +934,56 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport,
>   
>   static void dispc_oldi_tx_power(struct dispc_device *dispc, bool power)
>   {
> -	u32 val = power ? 0 : OLDI_PWRDN_TX;
> +	u32 val;
>   
>   	if (WARN_ON(!dispc->oldi_io_ctrl))
>   		return;
>   
> -	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT0_IO_CTRL,
> -			   OLDI_PWRDN_TX, val);
> -	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT1_IO_CTRL,
> -			   OLDI_PWRDN_TX, val);
> -	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT2_IO_CTRL,
> -			   OLDI_PWRDN_TX, val);
> -	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT3_IO_CTRL,
> -			   OLDI_PWRDN_TX, val);
> -	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_CLK_IO_CTRL,
> -			   OLDI_PWRDN_TX, val);
> +	if (dispc->feat->subrev == DISPC_AM65X) {

Slight nitpick, but I think switch-case makes sense for the subrev. Even 
if there are just two options here, using switch makes the structure 
clearer.

> +		val = power ? 0 : AM65X_OLDI_PWRDN_TX;
> +
> +		regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT0_IO_CTRL,
> +				   AM65X_OLDI_PWRDN_TX, val);
> +		regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT1_IO_CTRL,
> +				   AM65X_OLDI_PWRDN_TX, val);
> +		regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT2_IO_CTRL,
> +				   AM65X_OLDI_PWRDN_TX, val);
> +		regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT3_IO_CTRL,
> +				   AM65X_OLDI_PWRDN_TX, val);
> +		regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_CLK_IO_CTRL,
> +				   AM65X_OLDI_PWRDN_TX, val);
> +
> +	} else if (dispc->feat->subrev == DISPC_AM625) {
> +		if (power) {
> +			switch (dispc->oldi_mode) {
> +			case OLDI_MODE_SINGLE_LINK:
> +				/* Power down OLDI TX 1 */
> +				val = AM625_OLDI1_PWRDN_TX;
> +				break;
> +
> +			case OLDI_MODE_CLONE_SINGLE_LINK:
> +			case OLDI_MODE_DUAL_LINK:
> +				/* No Power down */
> +				val = 0;
> +				break;
> +
> +			default:
> +				/* Power down both OLDI TXes and LVDS Bandgap */
> +				val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX |
> +				      AM625_OLDI_PWRDN_BG;
> +				break;
> +			}
> +
> +		} else {
> +			/* Power down both OLDI TXes and LVDS Bandgap */
> +			val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX |
> +			      AM625_OLDI_PWRDN_BG;
> +		}
> +
> +		regmap_update_bits(dispc->oldi_io_ctrl, AM625_OLDI_PD_CTRL,
> +				   AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX |
> +				   AM625_OLDI_PWRDN_BG, val);
> +	}
>   }
>   
>   static void dispc_set_num_datalines(struct dispc_device *dispc,
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
> index 13feedfe5d6d..b2a148e96022 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
> @@ -227,17 +227,37 @@ enum dispc_common_regs {
>   #define DISPC_VP_DSS_DMA_THREADSIZE_STATUS	0x174 /* J721E */
>   
>   /*
> - * OLDI IO_CTRL register offsets. On AM654 the registers are found
> - * from CTRL_MMR0, there the syscon regmap should map 0x14 bytes from
> - * CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL
> - * register range.
> + * OLDI IO and PD CTRL register offsets.
> + * These registers are found in the CTRL_MMR0, where the syscon regmap should map
> + *
> + * 1. 0x14 bytes from CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL
> + * register range for the AM65X DSS, and
> + *
> + * 2. 0x200 bytes from OLDI0_DAT0_IO_CTRL to OLDI_LB_CTRL register range for the
> + * AM625 DSS.
>    */
> -#define OLDI_DAT0_IO_CTRL			0x00
> -#define OLDI_DAT1_IO_CTRL			0x04
> -#define OLDI_DAT2_IO_CTRL			0x08
> -#define OLDI_DAT3_IO_CTRL			0x0C
> -#define OLDI_CLK_IO_CTRL			0x10
>   
> -#define OLDI_PWRDN_TX				BIT(8)
> +/* -- For AM65X OLDI TX -- */
> +/* Register offsets */
> +#define AM65X_OLDI_DAT0_IO_CTRL			0x00
> +#define AM65X_OLDI_DAT1_IO_CTRL			0x04
> +#define AM65X_OLDI_DAT2_IO_CTRL			0x08
> +#define AM65X_OLDI_DAT3_IO_CTRL			0x0C
> +#define AM65X_OLDI_CLK_IO_CTRL			0x10
> +
> +/* Power control bits */
> +#define AM65X_OLDI_PWRDN_TX			BIT(8)
> +
> +/* -- For AM625 OLDI TX -- */
> +/* Register offsets */
> +#define AM625_OLDI_PD_CTRL			0x100
> +#define AM625_OLDI_LB_CTRL			0x104
> +
> +/* Power control bits */
> +#define AM625_OLDI0_PWRDN_TX			BIT(0)
> +#define AM625_OLDI1_PWRDN_TX			BIT(1)
> +
> +/* LVDS Bandgap reference Enable/Disable */
> +#define AM625_OLDI_PWRDN_BG			BIT(8)
>   
>   #endif /* __TIDSS_DISPC_REGS_H */

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH v7 3/6] drm/tidss: Add support for AM625 DSS

[Tomi Valkeinen]

On 25/01/2023 13:35, Aradhya Bhatia wrote:
> Add support for the DSS controller on TI's new AM625 SoC in the tidss
> driver.
> 
> The first video port (VP0) in am625-dss can output OLDI signals through
> 2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus
> type.

Not a big thing here as you add support for a new SoC, but the ordering 
of the patches is not optimal. Here you add the AM625 DSS support, but 
then you continue actually adding the DSS support (well, mainly OLDI) in 
the following patches.

I think patch 6 could be before this patch. Parts of patch 4 could also 
be before this patch. The AM65X renames from patch 5 could be before 
this patch.

I'm mainly thinking of a case where someone uses AM625 and is bisecting 
a problem. What happens if his board uses OLDI, and he happens to hit 
one of these patches during bisect? If the display just stays black, but 
otherwise everything works fine, then no problem. But if it crashes or 
starts spamming sync losts or such or gives errors, it's not so nice.

  Tomi

Re: [PATCH v7 1/6] drm/tidss: Remove Video Port to Output Port coupling

[Aradhya Bhatia]

Hi Tomi,

Thanks for the review!

On 03-Feb-23 16:53, Tomi Valkeinen wrote:
> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>> Make DSS Video Ports agnostic of output bus types.
>>
>> DSS controllers have had a 1-to-1 coupling between its VPs and its
>> output ports. This no longer stands true for the new AM625 DSS. This
>> coupling, hence, has been removed by renaming the 'vp_bus_type' to
>> 'output_port_bus_type' because the VPs are essentially agnostic of the
>> bus type and it is the output ports which have a bus type.
>>
>> The AM625 DSS has 2 VPs but requires 3 output ports to support its
>> Dual-Link OLDI video output coming from a single VP.
> 
> Not a biggie, but this sentence is a bit odd here at the end. Shouldn't
> it be after the "...stands true for the new AM625 DSS."?

Yes! It should be. Will make the edit.

> 
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------
>>   drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------
>>   drivers/gpu/drm/tidss/tidss_drv.h   |  5 +--
>>   drivers/gpu/drm/tidss/tidss_irq.h   |  2 +-
>>   drivers/gpu/drm/tidss/tidss_kms.c   | 12 ++++----
>>   5 files changed, 48 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index 165365b515e1..c1c4faccbddc 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = {
>>       .min_pclk_khz = 4375,
>>         .max_pclk_khz = {
>> -        [DISPC_VP_DPI] = 150000,
>> +        [DISPC_PORT_DPI] = 150000,
>>       },
>>         /*
>> @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = {
>>       .vp_name = { "vp1" },
>>       .ovr_name = { "ovr1" },
>>       .vpclk_name =  { "vp1" },
>> -    .vp_bus_type = { DISPC_VP_DPI },
>>         .vp_feat = { .color = {
>>               .has_ctm = true,
>> @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = {
>>       .vid_name = { "vid1" },
>>       .vid_lite = { false },
>>       .vid_order = { 0 },
>> +
>> +    .num_output_ports = 1,
>> +    .output_port_bus_type = { DISPC_PORT_DPI },
>>   };
> 
> Just thinking out loud, as these will get more complex in the future,
> maybe we should finally group them with struct. E.g. we could define
> struct array for vps, like (just hacky example):
> 
>     struct {
>         const char *name;
>         const char *clkname;
>         struct tidss_vp_feat feat;
>     } vps[TIDSS_MAX_PORTS];
> 
> and then use them as:
> 
>     .vps = {
>         {
>             .name = "kala",
>             .clkname = "kissa",
>             .feat.color.has_ctm = true,
>         }, {
>             .name = "kala2",
>             .clkname = "kissa2",
>             .feat.color.has_ctm = false,
>         },
>     },
> 
> Perhaps something to try in the future.
>

Yes, agreed! Having that structure will tidy this up.
I will keep this under future work.

>>   static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>> @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>     const struct dispc_features dispc_am65x_feats = {
>>       .max_pclk_khz = {
>> -        [DISPC_VP_DPI] = 165000,
>> -        [DISPC_VP_OLDI] = 165000,
>> +        [DISPC_PORT_DPI] = 165000,
>> +        [DISPC_PORT_OLDI] = 165000,
>>       },
>>         .scaling = {
>> @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = {
>>       .vp_name = { "vp1", "vp2" },
>>       .ovr_name = { "ovr1", "ovr2" },
>>       .vpclk_name =  { "vp1", "vp2" },
>> -     .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
>>       .vp_feat = { .color = {
>>               .has_ctm = true,
>> @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = {
>>       .vid_name = { "vid", "vidl1" },
>>       .vid_lite = { false, true, },
>>       .vid_order = { 1, 0 },
>> +
>> +    .num_output_ports = 2,
>> +    .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI },
>>   };
>>     static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>> @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>     const struct dispc_features dispc_j721e_feats = {
>>       .max_pclk_khz = {
>> -        [DISPC_VP_DPI] = 170000,
>> -        [DISPC_VP_INTERNAL] = 600000,
>> +        [DISPC_PORT_DPI] = 170000,
>> +        [DISPC_PORT_INTERNAL] = 600000,
>>       },
>>         .scaling = {
>> @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = {
>>       .vp_name = { "vp1", "vp2", "vp3", "vp4" },
>>       .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" },
>>       .vpclk_name = { "vp1", "vp2", "vp3", "vp4" },
>> -    /* Currently hard coded VP routing (see dispc_initial_config()) */
>> -    .vp_bus_type =    { DISPC_VP_INTERNAL, DISPC_VP_DPI,
>> -              DISPC_VP_INTERNAL, DISPC_VP_DPI, },
>> +
> 
> I think this line feed is extra.

Okay! Will remove that from all SoC feat structs.

> 
>>       .vp_feat = { .color = {
>>               .has_ctm = true,
>>               .gamma_size = 1024,
>> @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = {
>>       .vid_name = { "vid1", "vidl1", "vid2", "vidl2" },
>>       .vid_lite = { 0, 1, 0, 1, },
>>       .vid_order = { 1, 3, 0, 2 },
>> +
>> +    .num_output_ports = 4,
>> +    /* Currently hard coded VP routing (see dispc_initial_config()) */
>> +    .output_port_bus_type =    { DISPC_PORT_INTERNAL, DISPC_PORT_DPI,
>> +              DISPC_PORT_INTERNAL, DISPC_PORT_DPI, },
> 
> Indent doesn't look right (but it might be just because this is a diff).

I may have missed indenting this.

> 
>>   };
>>     static const u16 *dispc_common_regmap;
>> @@ -287,12 +294,12 @@ struct dispc_device {
>>      void __iomem *base_common;
>>      void __iomem *base_vid[TIDSS_MAX_PLANES];
>> -    void __iomem *base_ovr[TIDSS_MAX_PORTS];
>> -    void __iomem *base_vp[TIDSS_MAX_PORTS];
>> +    void __iomem *base_ovr[TIDSS_MAX_VPS];
>> +    void __iomem *base_vp[TIDSS_MAX_VPS];
>>      struct regmap *oldi_io_ctrl;
>> -    struct clk *vp_clk[TIDSS_MAX_PORTS];
>> +    struct clk *vp_clk[TIDSS_MAX_VPS];
>>         const struct dispc_features *feat;
>>   @@ -300,7 +307,7 @@ struct dispc_device {
>>         bool is_enabled;
>> -    struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
>> +    struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>>         u32 *fourccs;
>>       u32 num_fourccs;
>> @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport,
>>           return -EINVAL;
>>       }
>> -    if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI &&
>> +    if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI &&
> 
> Hmm, so is the hw_videoport a vp index or an output index? Sounds like
> the former, so it's not right, even if at the moment they're identical.
> We need some kind of mapping between those.
> 

It is indeed a vp index. And yes, I can come up with a mapping mechanism.

> If the mapping can be changed (or just defined in the DT), I think we
> need a variable in struct dispc_device, which tells the output to which
> a videoport is connected to. Or vice versa, I'm not sure which direction
> we need more. If the mapping is always the same on all SoC (but I don't
> think so), we can have it in the feats.
> 

As of now, the mapping is always same. But I would like to make is
generalized for future. Hence, I am considering to keep the variable in
struct dispc_device.

My question though would be, how would one be able to find which kind
of device is the port connected to, if it is connected to a bridge? For
example, in case of panels, we have a "connector_type" variable in
drm_panel which tells what kind of sink it is. But there is no such
thing in drm_bridge.

This is required because what if we can connect an videoport to either
an LVDS/OLDI bridge or a DPI bridge.

Also, implementing this might mean removal of the part of code which
confirms that the panel's "connector_type" indeed expects what the VP
can provide, unless there is a way to find out what the sink is before
calling the drm_of_find_panel_or_bridge API.


On the direction, the primary requirement of hw_videoport has been in
the tidss_dispc.c file where the HW registers are getting configured.
'hw_videoport' is a frequently passed parameter in function calls. So,
at a first glance the former option might makes more sense for
direction, i.e. to have a variable which tells the output to which a
videoport is connected to.

And while, there is also the tidss_kms.c file, which deals with
initializing encoders and attaching bridges. This is where the
output_port is required more often. But I am yet to think of a case
where the above direction could be an issue.


> Also, I wonder if output_port is a good name as it has "port" in it
> (like video port), and it's a bit long-ish. Would just "output" be
> enough? We could, of course, shorten it to OP, but that looks odd to me =).
> 

>>           fmt->is_oldi_fmt) {
>>           dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n",
>>               __func__, dispc->feat->vp_name[hw_videoport]);
>> @@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
>>       if (WARN_ON(!fmt))
>>           return;
>> -    if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) {
>> +    if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) {
>>           dispc_oldi_tx_power(dispc, true);
>>             dispc_enable_oldi(dispc, hw_videoport, fmt);
>> @@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
>>       align = true;
>>         /* always use DE_HIGH for OLDI */
>> -    if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI)
>> +    if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI)
>>           ieo = false;
>>         dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ,
>> @@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport)
>>     void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport)
>>   {
>> -    if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) {
>> +    if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) {
>>           dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0);
>>             dispc_oldi_tx_power(dispc, false);
>> @@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
>>                        const struct drm_display_mode *mode)
>>   {
>>      u32 hsw, hfp, hbp, vsw, vfp, vbp;
>> -    enum dispc_vp_bus_type bus_type;
>> +    enum dispc_port_bus_type bus_type;
>>      int max_pclk;
>> -    bus_type = dispc->feat->vp_bus_type[hw_videoport];
>> +    bus_type = dispc->feat->output_port_bus_type[hw_videoport];
>>      max_pclk = dispc->feat->max_pclk_khz[bus_type];
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
>> index e49432f0abf5..30fb44158347 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>> @@ -50,11 +50,11 @@ struct dispc_errata {
>>       bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */
>>   };
>> -enum dispc_vp_bus_type {
>> -    DISPC_VP_DPI,        /* DPI output */
>> -    DISPC_VP_OLDI,        /* OLDI (LVDS) output */
>> -    DISPC_VP_INTERNAL,    /* SoC internal routing */
>> -    DISPC_VP_MAX_BUS_TYPE,
>> +enum dispc_port_bus_type {
>> +    DISPC_PORT_DPI,            /* DPI output */
>> +    DISPC_PORT_OLDI,        /* OLDI (LVDS) output */
>> +    DISPC_PORT_INTERNAL,        /* SoC internal routing */
>> +    DISPC_PORT_MAX_BUS_TYPE,
> 
> Okay, so here you have just "port", not "output_port". In the DT,
> they're ports, so... Maybe we could use that name too, and for video
> port always use "vp". The current "hw_videoport" could be easily
> mistaken with "port".

I see what you are saying and how somebody could confusre hw_videoport
for a physical connection (i.e. port). I have always understoof
hw_videoport to be a thing of the actual VP inside the SoC, but that may
be because I have been working on this, and not just trying to
understand the code from a high level.

How about if I change the output_port to "out_port"? I am okay to keep
"output" as the name to. I am saying this becuase I think, only keeping
"port" might just confuse more, as you mentioned above.

And then we can change "hw_videoport" to "vp_index", perhaps, or even
keep that as it is? Becuase if we do have a VP structure in future
like you suggested above, "vps" and "vp" would become a close overlap.
For eg, "vps[vp]".

How do these sound to you?

> 
> I don't recall why I chose to use "hw" prefix there. I think I wanted to
> separate it from some other videoport, but... I don't know what that
> "other" is =).
> 

Perhaps because just a "videoport" might be confused with a connector on
the board, as in "HDMI port"? And the prefix might be a way to clarify
that its the DSS controller's VP. =)


Regards
Aradhya

Re: [PATCH v7 4/6] drm/tidss: Add support to configure OLDI mode for am625-dss

[Aradhya Bhatia]

Hi Tomi,

On 03-Feb-23 20:42, Tomi Valkeinen wrote:
> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>> The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal.
>> These can be configured to support the following modes:
>>
>> 1. OLDI_SINGLE_LINK_SINGLE_MODE
>> Single Output over OLDI 0.
>> +------+        +---------+      +-------+
>> |      |        |         |      |       |
>> | CRTC +------->+ ENCODER +----->| PANEL |
>> |      |        |         |      |       |
>> +------+        +---------+      +-------+
>>
>> 2. OLDI_SINGLE_LINK_CLONE_MODE
>> Duplicate Output over OLDI 0 and 1.
>> +------+        +---------+      +-------+
>> |      |        |         |      |       |
>> | CRTC +---+--->| ENCODER +----->| PANEL |
>> |      |   |    |         |      |       |
>> +------+   |    +---------+      +-------+
>>             |
>>             |    +---------+      +-------+
>>             |    |         |      |       |
>>             +--->| ENCODER +----->| PANEL |
>>                  |         |      |       |
>>                  +---------+      +-------+
>>
>> 3. OLDI_DUAL_LINK_MODE
>> Combined Output over OLDI 0 and 1.
>> +------+        +---------+      +-------+
>> |      |        |         +----->|       |
>> | CRTC +------->+ ENCODER |      | PANEL |
>> |      |        |         +----->|       |
>> +------+        +---------+      +-------+
>>
>> Following the above pathways for different modes, 2 encoder/panel-bridge
>> pipes get created for clone mode, and 1 pipe in cases of single link and
>> dual link mode.
>>
>> Add support for confguring the OLDI modes using OF and LVDS DRM helper
>> functions.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c   |  24 ++-
>>   drivers/gpu/drm/tidss/tidss_dispc.h   |  12 ++
>>   drivers/gpu/drm/tidss/tidss_drv.h     |   3 +
>>   drivers/gpu/drm/tidss/tidss_encoder.c |   4 +-
>>   drivers/gpu/drm/tidss/tidss_encoder.h |   3 +-
>>   drivers/gpu/drm/tidss/tidss_kms.c     | 221 ++++++++++++++++++++++++--
>>   6 files changed, 245 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index b55ccbcaa67f..37a73e309330 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -88,6 +88,8 @@ const struct dispc_features dispc_k2g_feats = {
>>         .subrev = DISPC_K2G,
>>   +    .has_oldi = false,
>> +
>>       .common = "common",
>>         .common_regs = tidss_k2g_common_regs,
>> @@ -166,6 +168,8 @@ const struct dispc_features dispc_am625_feats = {
>>         .subrev = DISPC_AM625,
>>   +    .has_oldi = true,
>> +
>>       .common = "common",
>>       .common_regs = tidss_am65x_common_regs,
>>   @@ -218,6 +222,8 @@ const struct dispc_features dispc_am65x_feats = {
>>         .subrev = DISPC_AM65X,
>>   +    .has_oldi = true,
>> +
>>       .common = "common",
>>       .common_regs = tidss_am65x_common_regs,
>>   @@ -309,6 +315,8 @@ const struct dispc_features dispc_j721e_feats = {
>>         .subrev = DISPC_J721E,
>>   +    .has_oldi = false,
>> +
>>       .common = "common_m",
>>       .common_regs = tidss_j721e_common_regs,
>>   @@ -361,6 +369,8 @@ struct dispc_device {
>>         struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>>   +    enum dispc_oldi_modes oldi_mode;
>> +
>>       u32 *fourccs;
>>       u32 num_fourccs;
>>   @@ -1963,6 +1973,12 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len)
>>       return dispc->fourccs;
>>   }
>>   +void dispc_set_oldi_mode(struct dispc_device *dispc,
>> +             enum dispc_oldi_modes oldi_mode)
>> +{
>> +    dispc->oldi_mode = oldi_mode;
>> +}
>> +
>>   static s32 pixinc(int pixels, u8 ps)
>>   {
>>       if (pixels == 1)
>> @@ -2647,7 +2663,7 @@ int dispc_runtime_resume(struct dispc_device *dispc)
>>           REG_GET(dispc, DSS_SYSSTATUS, 2, 2),
>>           REG_GET(dispc, DSS_SYSSTATUS, 3, 3));
>>   -    if (dispc->feat->subrev == DISPC_AM65X)
>> +    if (dispc->feat->has_oldi)
>>           dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n",
>>               REG_GET(dispc, DSS_SYSSTATUS, 5, 5),
>>               REG_GET(dispc, DSS_SYSSTATUS, 6, 6),
>> @@ -2688,7 +2704,7 @@ static int dispc_iomap_resource(struct platform_device *pdev, const char *name,
>>       return 0;
>>   }
>>   -static int dispc_init_am65x_oldi_io_ctrl(struct device *dev,
>> +static int dispc_init_am6xx_oldi_io_ctrl(struct device *dev,
>>                        struct dispc_device *dispc)
>>   {
>>       dispc->oldi_io_ctrl =
>> @@ -2827,8 +2843,8 @@ int dispc_init(struct tidss_device *tidss)
>>           dispc->vp_data[i].gamma_table = gamma_table;
>>       }
>>   -    if (feat->subrev == DISPC_AM65X) {
>> -        r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
>> +    if (feat->has_oldi) {
>> +        r = dispc_init_am6xx_oldi_io_ctrl(dev, dispc);
>>           if (r)
>>               return r;
>>       }
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
>> index 971f2856f015..880bc7de68b3 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>> @@ -64,6 +64,15 @@ enum dispc_dss_subrevision {
>>       DISPC_J721E,
>>   };
>>   +enum dispc_oldi_modes {
>> +    OLDI_MODE_SINGLE_LINK,        /* Single output over OLDI 0. */
>> +    OLDI_MODE_CLONE_SINGLE_LINK,    /* Cloned output over OLDI 0 and 1. */
>> +    OLDI_MODE_DUAL_LINK,        /* Combined output over OLDI 0 and 1. */
>> +    OLDI_MODE_OFF,            /* OLDI TXes not connected in OF. */
>> +    OLDI_MODE_UNSUPPORTED,        /* Unsupported OLDI configuration in OF. */
>> +    OLDI_MODE_UNAVAILABLE,        /* OLDI TXes not available in SoC. */
>> +};
>> +
>>   struct dispc_features {
>>       int min_pclk_khz;
>>       int max_pclk_khz[DISPC_PORT_MAX_BUS_TYPE];
>> @@ -72,6 +81,8 @@ struct dispc_features {
>>         enum dispc_dss_subrevision subrev;
>>   +    bool has_oldi;
>> +
>>       const char *common;
>>       const u16 *common_regs;
>>       u32 num_vps;
>> @@ -131,6 +142,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
>>                 u32 hw_videoport);
>>   int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable);
>>   const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len);
>> +void dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode);
>>     int dispc_init(struct tidss_device *tidss);
>>   void dispc_remove(struct tidss_device *tidss);
>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
>> index 0ce7ee5ccd5b..58892f065c16 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.h
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
>> @@ -13,6 +13,9 @@
>>   #define TIDSS_MAX_PLANES 4
>>   #define TIDSS_MAX_OUTPUT_PORTS 4
>>   +/* For AM625-DSS with 2 OLDI TXes */
>> +#define TIDSS_MAX_BRIDGES_PER_PIPE    2
>> +
>>   typedef u32 dispc_irq_t;
>>     struct tidss_device {
>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
>> index 0d4865e9c03d..bd2a7358d7b0 100644
>> --- a/drivers/gpu/drm/tidss/tidss_encoder.c
>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
>> @@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = {
>>   };
>>     struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>> -                     u32 encoder_type, u32 possible_crtcs)
>> +                     u32 encoder_type, u32 possible_crtcs,
>> +                     u32 possible_clones)
>>   {
>>       struct drm_encoder *enc;
>>       int ret;
>> @@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>>           return ERR_PTR(-ENOMEM);
>>         enc->possible_crtcs = possible_crtcs;
>> +    enc->possible_clones = possible_clones;
>>         ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs,
>>                      encoder_type, NULL);
>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h
>> index ace877c0e0fd..01c62ba3ef16 100644
>> --- a/drivers/gpu/drm/tidss/tidss_encoder.h
>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.h
>> @@ -12,6 +12,7 @@
>>   struct tidss_device;
>>     struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>> -                     u32 encoder_type, u32 possible_crtcs);
>> +                     u32 encoder_type, u32 possible_crtcs,
>> +                     u32 possible_clones);
>>     #endif
>> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
>> index d449131935d2..8322ee6310bf 100644
>> --- a/drivers/gpu/drm/tidss/tidss_kms.c
>> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
>> @@ -13,6 +13,7 @@
>>   #include <drm/drm_of.h>
>>   #include <drm/drm_panel.h>
>>   #include <drm/drm_vblank.h>
>> +#include <linux/of.h>
>>     #include "tidss_crtc.h"
>>   #include "tidss_dispc.h"
>> @@ -104,26 +105,129 @@ static const struct drm_mode_config_funcs mode_config_funcs = {
>>       .atomic_commit = drm_atomic_helper_commit,
>>   };
>>   +static enum dispc_oldi_modes tidss_get_oldi_mode(struct tidss_device *tidss)
>> +{
>> +    int pixel_order;
>> +    enum dispc_oldi_modes oldi_mode;
>> +    struct device_node *oldi0_port, *oldi1_port;
>> +
>> +    /*
>> +     * For am625-dss, the OLDI ports are expected at port reg = 0 and 2,
>> +     * and for am65x-dss, the OLDI port is expected only at port reg = 0.
>> +     */
>> +    const u32 portnum_oldi0 = 0, portnum_oldi1 = 2;
>> +
>> +    oldi0_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi0);
>> +    oldi1_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi1);
>> +
>> +    if (!(oldi0_port || oldi1_port)) {
>> +        /* Keep OLDI TXes OFF if neither OLDI port is present. */
>> +        oldi_mode = OLDI_MODE_OFF;
>> +    } else if (oldi0_port && !oldi1_port) {
>> +        /*
>> +         * OLDI0 port found, but not OLDI1 port. Setting single
>> +         * link output mode.
>> +         */
>> +        oldi_mode = OLDI_MODE_SINGLE_LINK;
>> +    } else if (!oldi0_port && oldi1_port) {
>> +        /*
>> +         * The 2nd OLDI TX cannot be operated alone. This use case is
>> +         * not supported in the HW. Since the pins for OLDIs 0 and 1 are
>> +         * separate, one could theoretically set a clone mode over OLDIs
>> +         * 0 and 1 and just simply not use the OLDI 0. This is a hacky
>> +         * way to enable only OLDI TX 1 and hence is not officially
>> +         * supported.
>> +         */
>> +        dev_warn(tidss->dev,
>> +             "Single Mode over OLDI 1 is not supported in HW.\n");
>> +        oldi_mode = OLDI_MODE_UNSUPPORTED;
>> +    } else {
>> +        /*
>> +         * OLDI Ports found for both the OLDI TXes. The DSS is to be
>> +         * configured in either Dual Link or Clone Mode.
>> +         */
>> +        pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port,
>> +                                    oldi1_port);
>> +        switch (pixel_order) {
>> +        case -EINVAL:
>> +            /*
>> +             * The dual link properties were not found in at least
>> +             * one of the sink nodes. Since 2 OLDI ports are present
>> +             * in the DT, it can be safely assumed that the required
>> +             * configuration is Clone Mode.
>> +             */
>> +            oldi_mode = OLDI_MODE_CLONE_SINGLE_LINK;
>> +            break;
>> +
>> +        case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
>> +            /*
>> +             * Note that the OLDI TX 0 transmits the odd set of
>> +             * pixels while the OLDI TX 1 transmits the even set.
>> +             * This is a fixed configuration in the HW and an cannot
>> +             * be change via SW.
>> +             */
>> +            dev_warn(tidss->dev,
>> +                 "EVEN-ODD Dual-Link Mode is not supported in HW.\n");
>> +            oldi_mode = OLDI_MODE_UNSUPPORTED;
>> +            break;
>> +
>> +        case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
>> +            oldi_mode = OLDI_MODE_DUAL_LINK;
>> +            break;
>> +
>> +        default:
>> +            oldi_mode = OLDI_MODE_UNSUPPORTED;
>> +            break;
>> +        }
>> +    }
>> +
>> +    of_node_put(oldi0_port);
>> +    of_node_put(oldi1_port);
>> +
>> +    return oldi_mode;
>> +}
>> +
>>   static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>>   {
>>       struct device *dev = tidss->dev;
>>       unsigned int fourccs_len;
>>       const u32 *fourccs = dispc_plane_formats(tidss->dispc, &fourccs_len);
>> -    unsigned int i;
>> +    unsigned int i, j;
>>         struct pipe {
>>           u32 hw_videoport;
>> -        struct drm_bridge *bridge;
>> +        struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE];
>>           u32 enc_type;
>> +        u32 num_bridges;
>>       };
>>         const struct dispc_features *feat = tidss->feat;
>>       u32 output_ports = feat->num_output_ports;
>>       u32 max_planes = feat->num_planes;
>>   -    struct pipe pipes[TIDSS_MAX_VPS];
>> +    struct pipe pipes[TIDSS_MAX_VPS] = {0};
>> +
>>       u32 num_pipes = 0;
>>       u32 crtc_mask;
>> +    enum dispc_oldi_modes oldi_mode = OLDI_MODE_UNAVAILABLE;
>> +    u32 num_oldi = 0;
>> +    u32 num_encoders = 0;
>> +    u32 oldi_pipe_index = 0;
>> +
>> +    if (feat->has_oldi) {
>> +        oldi_mode = tidss_get_oldi_mode(tidss);
>> +
>> +        if ((oldi_mode == OLDI_MODE_DUAL_LINK ||
>> +             oldi_mode == OLDI_MODE_CLONE_SINGLE_LINK) &&
>> +            feat->subrev == DISPC_AM65X) {
>> +            dev_warn(tidss->dev,
>> +                 "am65x-dss does not support this OLDI mode.\n");
>> +
>> +            oldi_mode = OLDI_MODE_UNSUPPORTED;
>> +        }
> 
> Shouldn't OLDI_MODE_UNSUPPORTED be handled as an error? It means the DT
> is faulty, doesn't it? Maybe it could even be renamed to
> OLDI_MODE_ERROR. Or tidss_get_oldi_mode() could return a negative error
> code.
> 

The idea was to let the framework continue configuring the 2nd videoport
for DPI, even if the OLDI DT is wrong. But I have come across more
examples recently where that is not the case. DT error for one pipe has
resulted in returning of an error code.

Will make the change.

>> +
>> +        dispc_set_oldi_mode(tidss->dispc, oldi_mode);
>> +    }
> 
> Would it be better to move the above dispc_set_oldi_mode() to be outside
> the if block? Then oldi mode would be set to OLDI_MODE_UNAVAILABLE on
> SoCs that don't have OLDI.

Ahh, yes! Will make the change.

> 
> tidss_get_oldi_mode and dispc_set_oldi_mode sound like opposites, but
> they're totally different things. Maybe tidss_get_oldi_mode should
> rather be something about parsing oldi dt properties or such.

Okay! Is 'tidss_parse_oldi_properties' acceptable? This is just
something I came up with now. I can think of more if this is not good.

> 
>>       /* first find all the connected panels & bridges */
>>   @@ -179,10 +283,87 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>>               }
>>           }
>>   -        pipes[num_pipes].hw_videoport = i;
>> -        pipes[num_pipes].bridge = bridge;
>> -        pipes[num_pipes].enc_type = enc_type;
>> -        num_pipes++;
>> +        if (feat->output_port_bus_type[i] == DISPC_PORT_OLDI) {
>> +            switch (oldi_mode) {
>> +            case OLDI_MODE_UNSUPPORTED:
>> +            case OLDI_MODE_OFF:
>> +                /*
>> +                 * Either the OLDI ports are not connected in
>> +                 * OF, or their configuration mode is not
>> +                 * supported.
>> +                 * In both the cases, the OLDI sink ports shall
>> +                 * not be logically connected to DSS ports.
>> +                 *
>> +                 * However, since other dss ports might still
>> +                 * be in use (eg, for DPI), the driver shall
>> +                 * continue to find the next connected sink in
>> +                 * OF.
>> +                 */
>> +                dev_dbg(dev, "OLDI disconnected on port %d\n", i);
>> +                continue;
>> +
>> +            case OLDI_MODE_DUAL_LINK:
>> +                /*
>> +                 * The 2nd OLDI port of a dual-link sink does
>> +                 * not require a separate bridge entity.
>> +                 */
>> +                if (num_oldi) {
> 
> I think if (num_oldi > 0) makes it more readable.

Alright!

> 
>> +                    drm_panel_bridge_remove(bridge);
>> +                    continue;
>> +                }
>> +
>> +                fallthrough;
>> +
>> +            case OLDI_MODE_CLONE_SINGLE_LINK:
>> +            case OLDI_MODE_SINGLE_LINK:
>> +                /*
>> +                 * Setting up pipe parameters when 1st OLDI
>> +                 * port is detected.
>> +                 */
>> +                if (!num_oldi) {
> 
> And here if (num_oldi == 0).
Okay!

> 
>> +                    pipes[num_pipes].hw_videoport = i;
>> +                    pipes[num_pipes].enc_type = enc_type;
>> +
>> +                    /*
>> +                     * Saving the pipe index in case its
>> +                     * required for 2nd OLDI Port.
>> +                     */
>> +                    oldi_pipe_index = num_pipes;
>> +
>> +                    /*
>> +                     * Incrememnt num_pipe when 1st oldi
>> +                     * port is discovered. For the 2nd OLDI
>> +                     * port, num_pipe need not be
>> +                     * incremented because the 2nd
>> +                     * Encoder-to-Bridge connection will
>> +                     * still be the part of the first OLDI
>> +                     * Port pipe.
>> +                     */
>> +                    num_pipes++;
>> +                }
>> +
>> +                /*
>> +                 * Bridge is required to be added only if the
>> +                 * detected port is the first OLDI port (of any
>> +                 * mode) or a subsequent port in Clone Mode.
>> +                 */
> 
> If I understand right, you're saying that bridge is not required to be
> added for the second port for dual link? It maybe be clearer to state
> that. But if so, isn't this still always adding the bridge here, for all
> modes? Or what is the comment referring to?
> 

Well, yes. I am saying that the bridge should be added only when either
one of the following 2 DT cases are detected.

i.  First oldi DT (of any mode, including clone) OR
ii. Second oldi DT only under clone mode

Bridge is not required for the 2nd DT connection of Dual Link mode.
I will make the comment clearer.

If the loop is parsing the port under dual link mode, this part of the
code will not be reached because of the "continue" clause above under
the OLDI_MODE_DUAL_LINK case. Moreover, there won't be a 2nd port to
parse if the mode is single link.

That said, I can provide an if condition which ensures that, that code
is only run when above conditions are met.


Regards
Aradhya

Re: [PATCH v7 5/6] drm/tidss: Add IO CTRL and Power support for OLDI TX in am625

[Aradhya Bhatia]

On 03-Feb-23 20:49, Tomi Valkeinen wrote:
> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>> The ctrl mmr module of the AM625 is different from the AM65X SoC. Thus
>> the ctrl mmr registers that supported the OLDI TX power have become
>> different in AM625 SoC.
>>
>> The common mode voltage of the LVDS buffers becomes random when the
>> bandgap reference is turned off. This causes uncertainity in the LVDS
>> Data and Clock signal outputs, making it behave differently under
>> different conditions and panel setups. The bandgap reference must be
>> powered on before using the OLDI IOs, to keep the common voltage trimmed
>> down to desired levels.
>>
>> Add support to enable/disable OLDI IO signals as well as the bandgap
>> reference circuit for the LVDS signals.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>
>> Note:
>> - Dropped Tomi Valkeinen's reviewed-by tag in this patch because I did
>>    not implement one of his comments which suggested to remove the
>>    'oldi_supported' variable. While the oldi support is indeed based on
>>    SoC variations, keeping that variable helps take into account the case
>>    where an OLDI supporting SoC by-passes OLDI TXes and gives out DPI
>>    video signals straight from DSS.
> 
> Hmm why is that relevent for this patch? It doesn't use oldi_supported
> or the new has_oldi.

It doesn't. Not directly atleast. In the previous version of this patch,
there was a mention of 'oldi_supported' and your tag was conditional to
that variable getting removed. Instead, I renamed the variable.

> 
>>   drivers/gpu/drm/tidss/tidss_dispc.c      | 57 +++++++++++++++++++-----
>>   drivers/gpu/drm/tidss/tidss_dispc_regs.h | 40 ++++++++++++-----
>>   2 files changed, 76 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index 37a73e309330..0e03557bc142 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -934,21 +934,56 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport,
>>     static void dispc_oldi_tx_power(struct dispc_device *dispc, bool power)
>>   {
>> -    u32 val = power ? 0 : OLDI_PWRDN_TX;
>> +    u32 val;
>>         if (WARN_ON(!dispc->oldi_io_ctrl))
>>           return;
>>   -    regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT0_IO_CTRL,
>> -               OLDI_PWRDN_TX, val);
>> -    regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT1_IO_CTRL,
>> -               OLDI_PWRDN_TX, val);
>> -    regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT2_IO_CTRL,
>> -               OLDI_PWRDN_TX, val);
>> -    regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT3_IO_CTRL,
>> -               OLDI_PWRDN_TX, val);
>> -    regmap_update_bits(dispc->oldi_io_ctrl, OLDI_CLK_IO_CTRL,
>> -               OLDI_PWRDN_TX, val);
>> +    if (dispc->feat->subrev == DISPC_AM65X) {
> 
> Slight nitpick, but I think switch-case makes sense for the subrev. Even
> if there are just two options here, using switch makes the structure clearer.

Alright. I will make the edit.

> 
>> +        val = power ? 0 : AM65X_OLDI_PWRDN_TX;
>> +
>> +        regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT0_IO_CTRL,
>> +                   AM65X_OLDI_PWRDN_TX, val);
>> +        regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT1_IO_CTRL,
>> +                   AM65X_OLDI_PWRDN_TX, val);
>> +        regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT2_IO_CTRL,
>> +                   AM65X_OLDI_PWRDN_TX, val);
>> +        regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT3_IO_CTRL,
>> +                   AM65X_OLDI_PWRDN_TX, val);
>> +        regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_CLK_IO_CTRL,
>> +                   AM65X_OLDI_PWRDN_TX, val);
>> +
>> +    } else if (dispc->feat->subrev == DISPC_AM625) {
>> +        if (power) {
>> +            switch (dispc->oldi_mode) {
>> +            case OLDI_MODE_SINGLE_LINK:
>> +                /* Power down OLDI TX 1 */
>> +                val = AM625_OLDI1_PWRDN_TX;
>> +                break;
>> +
>> +            case OLDI_MODE_CLONE_SINGLE_LINK:
>> +            case OLDI_MODE_DUAL_LINK:
>> +                /* No Power down */
>> +                val = 0;
>> +                break;
>> +
>> +            default:
>> +                /* Power down both OLDI TXes and LVDS Bandgap */
>> +                val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX |
>> +                      AM625_OLDI_PWRDN_BG;
>> +                break;
>> +            }
>> +
>> +        } else {
>> +            /* Power down both OLDI TXes and LVDS Bandgap */
>> +            val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX |
>> +                  AM625_OLDI_PWRDN_BG;
>> +        }
>> +
>> +        regmap_update_bits(dispc->oldi_io_ctrl, AM625_OLDI_PD_CTRL,
>> +                   AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX |
>> +                   AM625_OLDI_PWRDN_BG, val);
>> +    }
>>   }
>>     static void dispc_set_num_datalines(struct dispc_device *dispc,
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
>> index 13feedfe5d6d..b2a148e96022 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
>> @@ -227,17 +227,37 @@ enum dispc_common_regs {
>>   #define DISPC_VP_DSS_DMA_THREADSIZE_STATUS    0x174 /* J721E */
>>     /*
>> - * OLDI IO_CTRL register offsets. On AM654 the registers are found
>> - * from CTRL_MMR0, there the syscon regmap should map 0x14 bytes from
>> - * CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL
>> - * register range.
>> + * OLDI IO and PD CTRL register offsets.
>> + * These registers are found in the CTRL_MMR0, where the syscon regmap should map
>> + *
>> + * 1. 0x14 bytes from CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL
>> + * register range for the AM65X DSS, and
>> + *
>> + * 2. 0x200 bytes from OLDI0_DAT0_IO_CTRL to OLDI_LB_CTRL register range for the
>> + * AM625 DSS.
>>    */
>> -#define OLDI_DAT0_IO_CTRL            0x00
>> -#define OLDI_DAT1_IO_CTRL            0x04
>> -#define OLDI_DAT2_IO_CTRL            0x08
>> -#define OLDI_DAT3_IO_CTRL            0x0C
>> -#define OLDI_CLK_IO_CTRL            0x10
>>   -#define OLDI_PWRDN_TX                BIT(8)
>> +/* -- For AM65X OLDI TX -- */
>> +/* Register offsets */
>> +#define AM65X_OLDI_DAT0_IO_CTRL            0x00
>> +#define AM65X_OLDI_DAT1_IO_CTRL            0x04
>> +#define AM65X_OLDI_DAT2_IO_CTRL            0x08
>> +#define AM65X_OLDI_DAT3_IO_CTRL            0x0C
>> +#define AM65X_OLDI_CLK_IO_CTRL            0x10
>> +
>> +/* Power control bits */
>> +#define AM65X_OLDI_PWRDN_TX            BIT(8)
>> +
>> +/* -- For AM625 OLDI TX -- */
>> +/* Register offsets */
>> +#define AM625_OLDI_PD_CTRL            0x100
>> +#define AM625_OLDI_LB_CTRL            0x104
>> +
>> +/* Power control bits */
>> +#define AM625_OLDI0_PWRDN_TX            BIT(0)
>> +#define AM625_OLDI1_PWRDN_TX            BIT(1)
>> +
>> +/* LVDS Bandgap reference Enable/Disable */
>> +#define AM625_OLDI_PWRDN_BG            BIT(8)
>>     #endif /* __TIDSS_DISPC_REGS_H */
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
>  Tomi
> 
Regards
Aradhya

Re: [PATCH v7 3/6] drm/tidss: Add support for AM625 DSS

[Aradhya Bhatia]

On 03-Feb-23 21:03, Tomi Valkeinen wrote:
> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>> Add support for the DSS controller on TI's new AM625 SoC in the tidss
>> driver.
>>
>> The first video port (VP0) in am625-dss can output OLDI signals through
>> 2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus
>> type.
> 
> Not a big thing here as you add support for a new SoC, but the ordering
> of the patches is not optimal. Here you add the AM625 DSS support, but
> then you continue actually adding the DSS support (well, mainly OLDI) in
> the following patches.
> 
> I think patch 6 could be before this patch. Parts of patch 4 could also
> be before this patch. The AM65X renames from patch 5 could be before
> this patch.

I can move whole of Patch 6 and even of Patch 4 before this one. I have
mentioned 'AM625-DSS' in a couple comments which I can make generic,
and the rest everything is SoC-agnostic.

I haven't tried this, but my concern is if we break patch 5 into 2
separate patches,

i. AM65X rename plus SoC based switch case, and
ii. Addition of AM625 SoC case

then I might have to overwrite some changes implemented during (i) in
(ii). I don't suppose that would be okay, would it?

Also, is it important to keep the compatible-addition patches of
DT-binding and driver next to each other in the series? Or should
the DT-binding patches should be the first ones? Just curious! =)

> 
> I'm mainly thinking of a case where someone uses AM625 and is bisecting
> a problem. What happens if his board uses OLDI, and he happens to hit
> one of these patches during bisect? If the display just stays black, but
> otherwise everything works fine, then no problem. But if it crashes or
> starts spamming sync losts or such or gives errors, it's not so nice.
> 
You are right! This certainly makes sense.


Regards
Aradhya

Re: [PATCH v7 3/6] drm/tidss: Add support for AM625 DSS

[Tomi Valkeinen]

On 05/02/2023 16:31, Aradhya Bhatia wrote:
> 
> 
> On 03-Feb-23 21:03, Tomi Valkeinen wrote:
>> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>>> Add support for the DSS controller on TI's new AM625 SoC in the tidss
>>> driver.
>>>
>>> The first video port (VP0) in am625-dss can output OLDI signals through
>>> 2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus
>>> type.
>>
>> Not a big thing here as you add support for a new SoC, but the ordering
>> of the patches is not optimal. Here you add the AM625 DSS support, but
>> then you continue actually adding the DSS support (well, mainly OLDI) in
>> the following patches.
>>
>> I think patch 6 could be before this patch. Parts of patch 4 could also
>> be before this patch. The AM65X renames from patch 5 could be before
>> this patch.
> 
> I can move whole of Patch 6 and even of Patch 4 before this one. I have
> mentioned 'AM625-DSS' in a couple comments which I can make generic,
> and the rest everything is SoC-agnostic.
> 
> I haven't tried this, but my concern is if we break patch 5 into 2
> separate patches,
> 
> i. AM65X rename plus SoC based switch case, and
> ii. Addition of AM625 SoC case
> 
> then I might have to overwrite some changes implemented during (i) in
> (ii). I don't suppose that would be okay, would it?

I'm not sure I follow here. Wouldn't (i) be a valid patch in its own? 
Nothing wrong in expanding that later (even if you end up changing a lot 
of it).

That said, I don't think this is a very important topic. There are only 
a few commits in the history that might be problematic. A simple fix 
would be to add all the features first, and only last add the compatible 
string for am625.

Or do all the changes for am625 in a single patch, and try to implement 
all the generic restructuring work before that.

Here we do have to change the vp-to-output mapping management, so maybe 
the second option won't be simple enough, and it's better to do the 
am625 changes in pieces, as in the first option.

So, it's really up to you. Just wanted to raise this possible issue so 
that you are aware of it and can do any easy fixes (if there are such).

> Also, is it important to keep the compatible-addition patches of
> DT-binding and driver next to each other in the series? Or should
> the DT-binding patches should be the first ones? Just curious! =)

I believe the convention is to have the DT-binding changes before you 
add the compatible string to the driver (if I recall right checkpatch or 
some other checking tool complains if you add a driver for a compatible 
that doesn't have a DT binding). Generic restructurings could be before 
the DT patch, of course, but usually I like to keep the DT binding 
changes at the very beginning of the series.

  Tomi

Re: [PATCH v7 1/6] drm/tidss: Remove Video Port to Output Port coupling

[Tomi Valkeinen]

On 05/02/2023 15:08, Aradhya Bhatia wrote:
> Hi Tomi,
> 
> Thanks for the review!
> 
> On 03-Feb-23 16:53, Tomi Valkeinen wrote:
>> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>>> Make DSS Video Ports agnostic of output bus types.
>>>
>>> DSS controllers have had a 1-to-1 coupling between its VPs and its
>>> output ports. This no longer stands true for the new AM625 DSS. This
>>> coupling, hence, has been removed by renaming the 'vp_bus_type' to
>>> 'output_port_bus_type' because the VPs are essentially agnostic of the
>>> bus type and it is the output ports which have a bus type.
>>>
>>> The AM625 DSS has 2 VPs but requires 3 output ports to support its
>>> Dual-Link OLDI video output coming from a single VP.
>>
>> Not a biggie, but this sentence is a bit odd here at the end. Shouldn't
>> it be after the "...stands true for the new AM625 DSS."?
> 
> Yes! It should be. Will make the edit.
> 
>>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> ---
>>>    drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------
>>>    drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------
>>>    drivers/gpu/drm/tidss/tidss_drv.h   |  5 +--
>>>    drivers/gpu/drm/tidss/tidss_irq.h   |  2 +-
>>>    drivers/gpu/drm/tidss/tidss_kms.c   | 12 ++++----
>>>    5 files changed, 48 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> index 165365b515e1..c1c4faccbddc 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = {
>>>        .min_pclk_khz = 4375,
>>>          .max_pclk_khz = {
>>> -        [DISPC_VP_DPI] = 150000,
>>> +        [DISPC_PORT_DPI] = 150000,
>>>        },
>>>          /*
>>> @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = {
>>>        .vp_name = { "vp1" },
>>>        .ovr_name = { "ovr1" },
>>>        .vpclk_name =  { "vp1" },
>>> -    .vp_bus_type = { DISPC_VP_DPI },
>>>          .vp_feat = { .color = {
>>>                .has_ctm = true,
>>> @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = {
>>>        .vid_name = { "vid1" },
>>>        .vid_lite = { false },
>>>        .vid_order = { 0 },
>>> +
>>> +    .num_output_ports = 1,
>>> +    .output_port_bus_type = { DISPC_PORT_DPI },
>>>    };
>>
>> Just thinking out loud, as these will get more complex in the future,
>> maybe we should finally group them with struct. E.g. we could define
>> struct array for vps, like (just hacky example):
>>
>>      struct {
>>          const char *name;
>>          const char *clkname;
>>          struct tidss_vp_feat feat;
>>      } vps[TIDSS_MAX_PORTS];
>>
>> and then use them as:
>>
>>      .vps = {
>>          {
>>              .name = "kala",
>>              .clkname = "kissa",
>>              .feat.color.has_ctm = true,
>>          }, {
>>              .name = "kala2",
>>              .clkname = "kissa2",
>>              .feat.color.has_ctm = false,
>>          },
>>      },
>>
>> Perhaps something to try in the future.
>>
> 
> Yes, agreed! Having that structure will tidy this up.
> I will keep this under future work.
> 
>>>    static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>> @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>>      const struct dispc_features dispc_am65x_feats = {
>>>        .max_pclk_khz = {
>>> -        [DISPC_VP_DPI] = 165000,
>>> -        [DISPC_VP_OLDI] = 165000,
>>> +        [DISPC_PORT_DPI] = 165000,
>>> +        [DISPC_PORT_OLDI] = 165000,
>>>        },
>>>          .scaling = {
>>> @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = {
>>>        .vp_name = { "vp1", "vp2" },
>>>        .ovr_name = { "ovr1", "ovr2" },
>>>        .vpclk_name =  { "vp1", "vp2" },
>>> -     .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
>>>        .vp_feat = { .color = {
>>>                .has_ctm = true,
>>> @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = {
>>>        .vid_name = { "vid", "vidl1" },
>>>        .vid_lite = { false, true, },
>>>        .vid_order = { 1, 0 },
>>> +
>>> +    .num_output_ports = 2,
>>> +    .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI },
>>>    };
>>>      static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>> @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>>      const struct dispc_features dispc_j721e_feats = {
>>>        .max_pclk_khz = {
>>> -        [DISPC_VP_DPI] = 170000,
>>> -        [DISPC_VP_INTERNAL] = 600000,
>>> +        [DISPC_PORT_DPI] = 170000,
>>> +        [DISPC_PORT_INTERNAL] = 600000,
>>>        },
>>>          .scaling = {
>>> @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = {
>>>        .vp_name = { "vp1", "vp2", "vp3", "vp4" },
>>>        .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" },
>>>        .vpclk_name = { "vp1", "vp2", "vp3", "vp4" },
>>> -    /* Currently hard coded VP routing (see dispc_initial_config()) */
>>> -    .vp_bus_type =    { DISPC_VP_INTERNAL, DISPC_VP_DPI,
>>> -              DISPC_VP_INTERNAL, DISPC_VP_DPI, },
>>> +
>>
>> I think this line feed is extra.
> 
> Okay! Will remove that from all SoC feat structs.
> 
>>
>>>        .vp_feat = { .color = {
>>>                .has_ctm = true,
>>>                .gamma_size = 1024,
>>> @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = {
>>>        .vid_name = { "vid1", "vidl1", "vid2", "vidl2" },
>>>        .vid_lite = { 0, 1, 0, 1, },
>>>        .vid_order = { 1, 3, 0, 2 },
>>> +
>>> +    .num_output_ports = 4,
>>> +    /* Currently hard coded VP routing (see dispc_initial_config()) */
>>> +    .output_port_bus_type =    { DISPC_PORT_INTERNAL, DISPC_PORT_DPI,
>>> +              DISPC_PORT_INTERNAL, DISPC_PORT_DPI, },
>>
>> Indent doesn't look right (but it might be just because this is a diff).
> 
> I may have missed indenting this.
> 
>>
>>>    };
>>>      static const u16 *dispc_common_regmap;
>>> @@ -287,12 +294,12 @@ struct dispc_device {
>>>       void __iomem *base_common;
>>>       void __iomem *base_vid[TIDSS_MAX_PLANES];
>>> -    void __iomem *base_ovr[TIDSS_MAX_PORTS];
>>> -    void __iomem *base_vp[TIDSS_MAX_PORTS];
>>> +    void __iomem *base_ovr[TIDSS_MAX_VPS];
>>> +    void __iomem *base_vp[TIDSS_MAX_VPS];
>>>       struct regmap *oldi_io_ctrl;
>>> -    struct clk *vp_clk[TIDSS_MAX_PORTS];
>>> +    struct clk *vp_clk[TIDSS_MAX_VPS];
>>>          const struct dispc_features *feat;
>>>    @@ -300,7 +307,7 @@ struct dispc_device {
>>>          bool is_enabled;
>>> -    struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
>>> +    struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>>>          u32 *fourccs;
>>>        u32 num_fourccs;
>>> @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport,
>>>            return -EINVAL;
>>>        }
>>> -    if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI &&
>>> +    if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI &&
>>
>> Hmm, so is the hw_videoport a vp index or an output index? Sounds like
>> the former, so it's not right, even if at the moment they're identical.
>> We need some kind of mapping between those.
>>
> 
> It is indeed a vp index. And yes, I can come up with a mapping mechanism.
> 
>> If the mapping can be changed (or just defined in the DT), I think we
>> need a variable in struct dispc_device, which tells the output to which
>> a videoport is connected to. Or vice versa, I'm not sure which direction
>> we need more. If the mapping is always the same on all SoC (but I don't
>> think so), we can have it in the feats.
>>
> 
> As of now, the mapping is always same. But I would like to make is
> generalized for future. Hence, I am considering to keep the variable in
> struct dispc_device.
> 
> My question though would be, how would one be able to find which kind
> of device is the port connected to, if it is connected to a bridge? For
> example, in case of panels, we have a "connector_type" variable in
> drm_panel which tells what kind of sink it is. But there is no such
> thing in drm_bridge.
> 
> This is required because what if we can connect an videoport to either
> an LVDS/OLDI bridge or a DPI bridge.

The connector type shouldn't matter.

The DSS has VPs and outputs. The VPs are "generic" and identical to each 
other, except in their possible connections to the outputs. The outputs, 
at least at the moment, are DPI, LVDS and internal, where internal is 
basically just DPI.

Those are the three different cases we are interested in within the dss 
driver, right? Does it matter where the DPI or LVDS output goes?

So what I'm saying is that the DSS device tree data should already 
define what kind of configuration we need, and there's no need to look 
further into the panel/bridge nodes.

> Also, implementing this might mean removal of the part of code which
> confirms that the panel's "connector_type" indeed expects what the VP
> can provide, unless there is a way to find out what the sink is before
> calling the drm_of_find_panel_or_bridge API.

Hmm, well, each DSS output (port in DT) is of a certain type, so we 
should be able to validate that the output and the panel's 
connector_type match.

> On the direction, the primary requirement of hw_videoport has been in
> the tidss_dispc.c file where the HW registers are getting configured.
> 'hw_videoport' is a frequently passed parameter in function calls. So,
> at a first glance the former option might makes more sense for
> direction, i.e. to have a variable which tells the output to which a
> videoport is connected to.

Makes sense.

> And while, there is also the tidss_kms.c file, which deals with
> initializing encoders and attaching bridges. This is where the
> output_port is required more often. But I am yet to think of a case
> where the above direction could be an issue.
> 
> 
>> Also, I wonder if output_port is a good name as it has "port" in it
>> (like video port), and it's a bit long-ish. Would just "output" be
>> enough? We could, of course, shorten it to OP, but that looks odd to me =).
>>
> 
>>>            fmt->is_oldi_fmt) {
>>>            dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n",
>>>                __func__, dispc->feat->vp_name[hw_videoport]);
>>> @@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
>>>        if (WARN_ON(!fmt))
>>>            return;
>>> -    if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) {
>>> +    if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) {
>>>            dispc_oldi_tx_power(dispc, true);
>>>              dispc_enable_oldi(dispc, hw_videoport, fmt);
>>> @@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
>>>        align = true;
>>>          /* always use DE_HIGH for OLDI */
>>> -    if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI)
>>> +    if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI)
>>>            ieo = false;
>>>          dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ,
>>> @@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport)
>>>      void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport)
>>>    {
>>> -    if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) {
>>> +    if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) {
>>>            dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0);
>>>              dispc_oldi_tx_power(dispc, false);
>>> @@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
>>>                         const struct drm_display_mode *mode)
>>>    {
>>>       u32 hsw, hfp, hbp, vsw, vfp, vbp;
>>> -    enum dispc_vp_bus_type bus_type;
>>> +    enum dispc_port_bus_type bus_type;
>>>       int max_pclk;
>>> -    bus_type = dispc->feat->vp_bus_type[hw_videoport];
>>> +    bus_type = dispc->feat->output_port_bus_type[hw_videoport];
>>>       max_pclk = dispc->feat->max_pclk_khz[bus_type];
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
>>> index e49432f0abf5..30fb44158347 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>>> @@ -50,11 +50,11 @@ struct dispc_errata {
>>>        bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */
>>>    };
>>> -enum dispc_vp_bus_type {
>>> -    DISPC_VP_DPI,        /* DPI output */
>>> -    DISPC_VP_OLDI,        /* OLDI (LVDS) output */
>>> -    DISPC_VP_INTERNAL,    /* SoC internal routing */
>>> -    DISPC_VP_MAX_BUS_TYPE,
>>> +enum dispc_port_bus_type {
>>> +    DISPC_PORT_DPI,            /* DPI output */
>>> +    DISPC_PORT_OLDI,        /* OLDI (LVDS) output */
>>> +    DISPC_PORT_INTERNAL,        /* SoC internal routing */
>>> +    DISPC_PORT_MAX_BUS_TYPE,
>>
>> Okay, so here you have just "port", not "output_port". In the DT,
>> they're ports, so... Maybe we could use that name too, and for video
>> port always use "vp". The current "hw_videoport" could be easily
>> mistaken with "port".
> 
> I see what you are saying and how somebody could confusre hw_videoport
> for a physical connection (i.e. port). I have always understoof
> hw_videoport to be a thing of the actual VP inside the SoC, but that may
> be because I have been working on this, and not just trying to
> understand the code from a high level.
> 
> How about if I change the output_port to "out_port"? I am okay to keep
> "output" as the name to. I am saying this becuase I think, only keeping
> "port" might just confuse more, as you mentioned above.

Yes, I agree "port" is not good. Other than that, no strong opinions. 
Whatever name you pick, someone will find it confusing ;). Just keep it 
consistent, so that all enums, parameters, etc. use it a consistent 
manner. If I had to choose, I'd go with the "output", but I'm fine with 
other names too.

> And then we can change "hw_videoport" to "vp_index", perhaps, or even
> keep that as it is? Becuase if we do have a VP structure in future
> like you suggested above, "vps" and "vp" would become a close overlap.
> For eg, "vps[vp]".

That is true. I think that was the reason I chose hw_videoport instead 
of videoport or vp, although vps[hw_videoport] is only barely better.

vp_index is ok for me, or maybe vp_idx or vp_num to have it a bit shorter.

  Tomi

Re: [PATCH v7 4/6] drm/tidss: Add support to configure OLDI mode for am625-dss

[Tomi Valkeinen]

On 05/02/2023 15:42, Aradhya Bhatia wrote:
> Hi Tomi,
> 
> On 03-Feb-23 20:42, Tomi Valkeinen wrote:
>> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>>> The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal.
>>> These can be configured to support the following modes:
>>>
>>> 1. OLDI_SINGLE_LINK_SINGLE_MODE
>>> Single Output over OLDI 0.
>>> +------+        +---------+      +-------+
>>> |      |        |         |      |       |
>>> | CRTC +------->+ ENCODER +----->| PANEL |
>>> |      |        |         |      |       |
>>> +------+        +---------+      +-------+
>>>
>>> 2. OLDI_SINGLE_LINK_CLONE_MODE
>>> Duplicate Output over OLDI 0 and 1.
>>> +------+        +---------+      +-------+
>>> |      |        |         |      |       |
>>> | CRTC +---+--->| ENCODER +----->| PANEL |
>>> |      |   |    |         |      |       |
>>> +------+   |    +---------+      +-------+
>>>              |
>>>              |    +---------+      +-------+
>>>              |    |         |      |       |
>>>              +--->| ENCODER +----->| PANEL |
>>>                   |         |      |       |
>>>                   +---------+      +-------+
>>>
>>> 3. OLDI_DUAL_LINK_MODE
>>> Combined Output over OLDI 0 and 1.
>>> +------+        +---------+      +-------+
>>> |      |        |         +----->|       |
>>> | CRTC +------->+ ENCODER |      | PANEL |
>>> |      |        |         +----->|       |
>>> +------+        +---------+      +-------+
>>>
>>> Following the above pathways for different modes, 2 encoder/panel-bridge
>>> pipes get created for clone mode, and 1 pipe in cases of single link and
>>> dual link mode.
>>>
>>> Add support for confguring the OLDI modes using OF and LVDS DRM helper
>>> functions.
>>>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> ---
>>>    drivers/gpu/drm/tidss/tidss_dispc.c   |  24 ++-
>>>    drivers/gpu/drm/tidss/tidss_dispc.h   |  12 ++
>>>    drivers/gpu/drm/tidss/tidss_drv.h     |   3 +
>>>    drivers/gpu/drm/tidss/tidss_encoder.c |   4 +-
>>>    drivers/gpu/drm/tidss/tidss_encoder.h |   3 +-
>>>    drivers/gpu/drm/tidss/tidss_kms.c     | 221 ++++++++++++++++++++++++--
>>>    6 files changed, 245 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> index b55ccbcaa67f..37a73e309330 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> @@ -88,6 +88,8 @@ const struct dispc_features dispc_k2g_feats = {
>>>          .subrev = DISPC_K2G,
>>>    +    .has_oldi = false,
>>> +
>>>        .common = "common",
>>>          .common_regs = tidss_k2g_common_regs,
>>> @@ -166,6 +168,8 @@ const struct dispc_features dispc_am625_feats = {
>>>          .subrev = DISPC_AM625,
>>>    +    .has_oldi = true,
>>> +
>>>        .common = "common",
>>>        .common_regs = tidss_am65x_common_regs,
>>>    @@ -218,6 +222,8 @@ const struct dispc_features dispc_am65x_feats = {
>>>          .subrev = DISPC_AM65X,
>>>    +    .has_oldi = true,
>>> +
>>>        .common = "common",
>>>        .common_regs = tidss_am65x_common_regs,
>>>    @@ -309,6 +315,8 @@ const struct dispc_features dispc_j721e_feats = {
>>>          .subrev = DISPC_J721E,
>>>    +    .has_oldi = false,
>>> +
>>>        .common = "common_m",
>>>        .common_regs = tidss_j721e_common_regs,
>>>    @@ -361,6 +369,8 @@ struct dispc_device {
>>>          struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>>>    +    enum dispc_oldi_modes oldi_mode;
>>> +
>>>        u32 *fourccs;
>>>        u32 num_fourccs;
>>>    @@ -1963,6 +1973,12 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len)
>>>        return dispc->fourccs;
>>>    }
>>>    +void dispc_set_oldi_mode(struct dispc_device *dispc,
>>> +             enum dispc_oldi_modes oldi_mode)
>>> +{
>>> +    dispc->oldi_mode = oldi_mode;
>>> +}
>>> +
>>>    static s32 pixinc(int pixels, u8 ps)
>>>    {
>>>        if (pixels == 1)
>>> @@ -2647,7 +2663,7 @@ int dispc_runtime_resume(struct dispc_device *dispc)
>>>            REG_GET(dispc, DSS_SYSSTATUS, 2, 2),
>>>            REG_GET(dispc, DSS_SYSSTATUS, 3, 3));
>>>    -    if (dispc->feat->subrev == DISPC_AM65X)
>>> +    if (dispc->feat->has_oldi)
>>>            dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n",
>>>                REG_GET(dispc, DSS_SYSSTATUS, 5, 5),
>>>                REG_GET(dispc, DSS_SYSSTATUS, 6, 6),
>>> @@ -2688,7 +2704,7 @@ static int dispc_iomap_resource(struct platform_device *pdev, const char *name,
>>>        return 0;
>>>    }
>>>    -static int dispc_init_am65x_oldi_io_ctrl(struct device *dev,
>>> +static int dispc_init_am6xx_oldi_io_ctrl(struct device *dev,
>>>                         struct dispc_device *dispc)
>>>    {
>>>        dispc->oldi_io_ctrl =
>>> @@ -2827,8 +2843,8 @@ int dispc_init(struct tidss_device *tidss)
>>>            dispc->vp_data[i].gamma_table = gamma_table;
>>>        }
>>>    -    if (feat->subrev == DISPC_AM65X) {
>>> -        r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
>>> +    if (feat->has_oldi) {
>>> +        r = dispc_init_am6xx_oldi_io_ctrl(dev, dispc);
>>>            if (r)
>>>                return r;
>>>        }
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
>>> index 971f2856f015..880bc7de68b3 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>>> @@ -64,6 +64,15 @@ enum dispc_dss_subrevision {
>>>        DISPC_J721E,
>>>    };
>>>    +enum dispc_oldi_modes {
>>> +    OLDI_MODE_SINGLE_LINK,        /* Single output over OLDI 0. */
>>> +    OLDI_MODE_CLONE_SINGLE_LINK,    /* Cloned output over OLDI 0 and 1. */
>>> +    OLDI_MODE_DUAL_LINK,        /* Combined output over OLDI 0 and 1. */
>>> +    OLDI_MODE_OFF,            /* OLDI TXes not connected in OF. */
>>> +    OLDI_MODE_UNSUPPORTED,        /* Unsupported OLDI configuration in OF. */
>>> +    OLDI_MODE_UNAVAILABLE,        /* OLDI TXes not available in SoC. */
>>> +};
>>> +
>>>    struct dispc_features {
>>>        int min_pclk_khz;
>>>        int max_pclk_khz[DISPC_PORT_MAX_BUS_TYPE];
>>> @@ -72,6 +81,8 @@ struct dispc_features {
>>>          enum dispc_dss_subrevision subrev;
>>>    +    bool has_oldi;
>>> +
>>>        const char *common;
>>>        const u16 *common_regs;
>>>        u32 num_vps;
>>> @@ -131,6 +142,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
>>>                  u32 hw_videoport);
>>>    int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable);
>>>    const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len);
>>> +void dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode);
>>>      int dispc_init(struct tidss_device *tidss);
>>>    void dispc_remove(struct tidss_device *tidss);
>>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
>>> index 0ce7ee5ccd5b..58892f065c16 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_drv.h
>>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
>>> @@ -13,6 +13,9 @@
>>>    #define TIDSS_MAX_PLANES 4
>>>    #define TIDSS_MAX_OUTPUT_PORTS 4
>>>    +/* For AM625-DSS with 2 OLDI TXes */
>>> +#define TIDSS_MAX_BRIDGES_PER_PIPE    2
>>> +
>>>    typedef u32 dispc_irq_t;
>>>      struct tidss_device {
>>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
>>> index 0d4865e9c03d..bd2a7358d7b0 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_encoder.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
>>> @@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = {
>>>    };
>>>      struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>>> -                     u32 encoder_type, u32 possible_crtcs)
>>> +                     u32 encoder_type, u32 possible_crtcs,
>>> +                     u32 possible_clones)
>>>    {
>>>        struct drm_encoder *enc;
>>>        int ret;
>>> @@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>>>            return ERR_PTR(-ENOMEM);
>>>          enc->possible_crtcs = possible_crtcs;
>>> +    enc->possible_clones = possible_clones;
>>>          ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs,
>>>                       encoder_type, NULL);
>>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h
>>> index ace877c0e0fd..01c62ba3ef16 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_encoder.h
>>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.h
>>> @@ -12,6 +12,7 @@
>>>    struct tidss_device;
>>>      struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>>> -                     u32 encoder_type, u32 possible_crtcs);
>>> +                     u32 encoder_type, u32 possible_crtcs,
>>> +                     u32 possible_clones);
>>>      #endif
>>> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
>>> index d449131935d2..8322ee6310bf 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_kms.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
>>> @@ -13,6 +13,7 @@
>>>    #include <drm/drm_of.h>
>>>    #include <drm/drm_panel.h>
>>>    #include <drm/drm_vblank.h>
>>> +#include <linux/of.h>
>>>      #include "tidss_crtc.h"
>>>    #include "tidss_dispc.h"
>>> @@ -104,26 +105,129 @@ static const struct drm_mode_config_funcs mode_config_funcs = {
>>>        .atomic_commit = drm_atomic_helper_commit,
>>>    };
>>>    +static enum dispc_oldi_modes tidss_get_oldi_mode(struct tidss_device *tidss)
>>> +{
>>> +    int pixel_order;
>>> +    enum dispc_oldi_modes oldi_mode;
>>> +    struct device_node *oldi0_port, *oldi1_port;
>>> +
>>> +    /*
>>> +     * For am625-dss, the OLDI ports are expected at port reg = 0 and 2,
>>> +     * and for am65x-dss, the OLDI port is expected only at port reg = 0.
>>> +     */
>>> +    const u32 portnum_oldi0 = 0, portnum_oldi1 = 2;
>>> +
>>> +    oldi0_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi0);
>>> +    oldi1_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi1);
>>> +
>>> +    if (!(oldi0_port || oldi1_port)) {
>>> +        /* Keep OLDI TXes OFF if neither OLDI port is present. */
>>> +        oldi_mode = OLDI_MODE_OFF;
>>> +    } else if (oldi0_port && !oldi1_port) {
>>> +        /*
>>> +         * OLDI0 port found, but not OLDI1 port. Setting single
>>> +         * link output mode.
>>> +         */
>>> +        oldi_mode = OLDI_MODE_SINGLE_LINK;
>>> +    } else if (!oldi0_port && oldi1_port) {
>>> +        /*
>>> +         * The 2nd OLDI TX cannot be operated alone. This use case is
>>> +         * not supported in the HW. Since the pins for OLDIs 0 and 1 are
>>> +         * separate, one could theoretically set a clone mode over OLDIs
>>> +         * 0 and 1 and just simply not use the OLDI 0. This is a hacky
>>> +         * way to enable only OLDI TX 1 and hence is not officially
>>> +         * supported.
>>> +         */
>>> +        dev_warn(tidss->dev,
>>> +             "Single Mode over OLDI 1 is not supported in HW.\n");
>>> +        oldi_mode = OLDI_MODE_UNSUPPORTED;
>>> +    } else {
>>> +        /*
>>> +         * OLDI Ports found for both the OLDI TXes. The DSS is to be
>>> +         * configured in either Dual Link or Clone Mode.
>>> +         */
>>> +        pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port,
>>> +                                    oldi1_port);
>>> +        switch (pixel_order) {
>>> +        case -EINVAL:
>>> +            /*
>>> +             * The dual link properties were not found in at least
>>> +             * one of the sink nodes. Since 2 OLDI ports are present
>>> +             * in the DT, it can be safely assumed that the required
>>> +             * configuration is Clone Mode.
>>> +             */
>>> +            oldi_mode = OLDI_MODE_CLONE_SINGLE_LINK;
>>> +            break;
>>> +
>>> +        case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
>>> +            /*
>>> +             * Note that the OLDI TX 0 transmits the odd set of
>>> +             * pixels while the OLDI TX 1 transmits the even set.
>>> +             * This is a fixed configuration in the HW and an cannot
>>> +             * be change via SW.
>>> +             */
>>> +            dev_warn(tidss->dev,
>>> +                 "EVEN-ODD Dual-Link Mode is not supported in HW.\n");
>>> +            oldi_mode = OLDI_MODE_UNSUPPORTED;
>>> +            break;
>>> +
>>> +        case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
>>> +            oldi_mode = OLDI_MODE_DUAL_LINK;
>>> +            break;
>>> +
>>> +        default:
>>> +            oldi_mode = OLDI_MODE_UNSUPPORTED;
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    of_node_put(oldi0_port);
>>> +    of_node_put(oldi1_port);
>>> +
>>> +    return oldi_mode;
>>> +}
>>> +
>>>    static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>>>    {
>>>        struct device *dev = tidss->dev;
>>>        unsigned int fourccs_len;
>>>        const u32 *fourccs = dispc_plane_formats(tidss->dispc, &fourccs_len);
>>> -    unsigned int i;
>>> +    unsigned int i, j;
>>>          struct pipe {
>>>            u32 hw_videoport;
>>> -        struct drm_bridge *bridge;
>>> +        struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE];
>>>            u32 enc_type;
>>> +        u32 num_bridges;
>>>        };
>>>          const struct dispc_features *feat = tidss->feat;
>>>        u32 output_ports = feat->num_output_ports;
>>>        u32 max_planes = feat->num_planes;
>>>    -    struct pipe pipes[TIDSS_MAX_VPS];
>>> +    struct pipe pipes[TIDSS_MAX_VPS] = {0};
>>> +
>>>        u32 num_pipes = 0;
>>>        u32 crtc_mask;
>>> +    enum dispc_oldi_modes oldi_mode = OLDI_MODE_UNAVAILABLE;
>>> +    u32 num_oldi = 0;
>>> +    u32 num_encoders = 0;
>>> +    u32 oldi_pipe_index = 0;
>>> +
>>> +    if (feat->has_oldi) {
>>> +        oldi_mode = tidss_get_oldi_mode(tidss);
>>> +
>>> +        if ((oldi_mode == OLDI_MODE_DUAL_LINK ||
>>> +             oldi_mode == OLDI_MODE_CLONE_SINGLE_LINK) &&
>>> +            feat->subrev == DISPC_AM65X) {
>>> +            dev_warn(tidss->dev,
>>> +                 "am65x-dss does not support this OLDI mode.\n");
>>> +
>>> +            oldi_mode = OLDI_MODE_UNSUPPORTED;
>>> +        }
>>
>> Shouldn't OLDI_MODE_UNSUPPORTED be handled as an error? It means the DT
>> is faulty, doesn't it? Maybe it could even be renamed to
>> OLDI_MODE_ERROR. Or tidss_get_oldi_mode() could return a negative error
>> code.
>>
> 
> The idea was to let the framework continue configuring the 2nd videoport
> for DPI, even if the OLDI DT is wrong. But I have come across more
> examples recently where that is not the case. DT error for one pipe has
> resulted in returning of an error code.
> 
> Will make the change.

My opinion is that the DT has to be correct. If it isn't, just fail and 
exit as soon as possible. There shouldn't be any reasons for the drivers 
to be trying to cope with a broken DT.

>>> +
>>> +        dispc_set_oldi_mode(tidss->dispc, oldi_mode);
>>> +    }
>>
>> Would it be better to move the above dispc_set_oldi_mode() to be outside
>> the if block? Then oldi mode would be set to OLDI_MODE_UNAVAILABLE on
>> SoCs that don't have OLDI.
> 
> Ahh, yes! Will make the change.
> 
>>
>> tidss_get_oldi_mode and dispc_set_oldi_mode sound like opposites, but
>> they're totally different things. Maybe tidss_get_oldi_mode should
>> rather be something about parsing oldi dt properties or such.
> 
> Okay! Is 'tidss_parse_oldi_properties' acceptable? This is just
> something I came up with now. I can think of more if this is not good.

Sounds fine.

  Tomi

Re: [PATCH v7 3/6] drm/tidss: Add support for AM625 DSS

[Aradhya Bhatia]

Hi Tomi,

On 06-Feb-23 16:28, Tomi Valkeinen wrote:
> On 05/02/2023 16:31, Aradhya Bhatia wrote:
>>
>>
>> On 03-Feb-23 21:03, Tomi Valkeinen wrote:
>>> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>>>> Add support for the DSS controller on TI's new AM625 SoC in the tidss
>>>> driver.
>>>>
>>>> The first video port (VP0) in am625-dss can output OLDI signals through
>>>> 2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus
>>>> type.
>>>
>>> Not a big thing here as you add support for a new SoC, but the ordering
>>> of the patches is not optimal. Here you add the AM625 DSS support, but
>>> then you continue actually adding the DSS support (well, mainly OLDI) in
>>> the following patches.
>>>
>>> I think patch 6 could be before this patch. Parts of patch 4 could also
>>> be before this patch. The AM65X renames from patch 5 could be before
>>> this patch.
>>
>> I can move whole of Patch 6 and even of Patch 4 before this one. I have
>> mentioned 'AM625-DSS' in a couple comments which I can make generic,
>> and the rest everything is SoC-agnostic.
>>
>> I haven't tried this, but my concern is if we break patch 5 into 2
>> separate patches,
>>
>> i. AM65X rename plus SoC based switch case, and
>> ii. Addition of AM625 SoC case
>>
>> then I might have to overwrite some changes implemented during (i) in
>> (ii). I don't suppose that would be okay, would it?
> 
> I'm not sure I follow here. Wouldn't (i) be a valid patch in its own?
> Nothing wrong in expanding that later (even if you end up changing a lot
> of it).
> 

(i) would be a valid patch, but implementing (ii) would over-write
certain changes done in (i), albeit small changes in terms of brackets
and indents. That didn't feel right initially and hence the question.

> That said, I don't think this is a very important topic. There are only
> a few commits in the history that might be problematic. A simple fix
> would be to add all the features first, and only last add the compatible
> string for am625.
> 
> Or do all the changes for am625 in a single patch, and try to implement
> all the generic restructuring work before that.
> 
> Here we do have to change the vp-to-output mapping management, so maybe
> the second option won't be simple enough, and it's better to do the
> am625 changes in pieces, as in the first option.
> 

Yeah, the first option does seem a little less complicated. Will try to
re-order this as much clearly as possible.

> So, it's really up to you. Just wanted to raise this possible issue so
> that you are aware of it and can do any easy fixes (if there are such).
> 
>> Also, is it important to keep the compatible-addition patches of
>> DT-binding and driver next to each other in the series? Or should
>> the DT-binding patches should be the first ones? Just curious! =)
> 
> I believe the convention is to have the DT-binding changes before you
> add the compatible string to the driver (if I recall right checkpatch or
> some other checking tool complains if you add a driver for a compatible
> that doesn't have a DT binding). Generic restructurings could be before
> the DT patch, of course, but usually I like to keep the DT binding
> changes at the very beginning of the series.
> 

Okay, I will keep the compatible-append in the binding as the first
patch in the series, before the other general structurings.

Thank you!


Regards
Aradhya

Re: [PATCH v7 1/6] drm/tidss: Remove Video Port to Output Port coupling

[Aradhya Bhatia]

On 06-Feb-23 18:35, Tomi Valkeinen wrote:
> On 05/02/2023 15:08, Aradhya Bhatia wrote:
>> Hi Tomi,
>>
>> Thanks for the review!
>>
>> On 03-Feb-23 16:53, Tomi Valkeinen wrote:
>>> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>>>> Make DSS Video Ports agnostic of output bus types.
>>>>
>>>> DSS controllers have had a 1-to-1 coupling between its VPs and its
>>>> output ports. This no longer stands true for the new AM625 DSS. This
>>>> coupling, hence, has been removed by renaming the 'vp_bus_type' to
>>>> 'output_port_bus_type' because the VPs are essentially agnostic of the
>>>> bus type and it is the output ports which have a bus type.
>>>>
>>>> The AM625 DSS has 2 VPs but requires 3 output ports to support its
>>>> Dual-Link OLDI video output coming from a single VP.
>>>
>>> Not a biggie, but this sentence is a bit odd here at the end. Shouldn't
>>> it be after the "...stands true for the new AM625 DSS."?
>>
>> Yes! It should be. Will make the edit.
>>
>>>
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>> ---
>>>>    drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------
>>>>    drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------
>>>>    drivers/gpu/drm/tidss/tidss_drv.h   |  5 +--
>>>>    drivers/gpu/drm/tidss/tidss_irq.h   |  2 +-
>>>>    drivers/gpu/drm/tidss/tidss_kms.c   | 12 ++++----
>>>>    5 files changed, 48 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> index 165365b515e1..c1c4faccbddc 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = {
>>>>        .min_pclk_khz = 4375,
>>>>          .max_pclk_khz = {
>>>> -        [DISPC_VP_DPI] = 150000,
>>>> +        [DISPC_PORT_DPI] = 150000,
>>>>        },
>>>>          /*
>>>> @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = {
>>>>        .vp_name = { "vp1" },
>>>>        .ovr_name = { "ovr1" },
>>>>        .vpclk_name =  { "vp1" },
>>>> -    .vp_bus_type = { DISPC_VP_DPI },
>>>>          .vp_feat = { .color = {
>>>>                .has_ctm = true,
>>>> @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = {
>>>>        .vid_name = { "vid1" },
>>>>        .vid_lite = { false },
>>>>        .vid_order = { 0 },
>>>> +
>>>> +    .num_output_ports = 1,
>>>> +    .output_port_bus_type = { DISPC_PORT_DPI },
>>>>    };
>>>
>>> Just thinking out loud, as these will get more complex in the future,
>>> maybe we should finally group them with struct. E.g. we could define
>>> struct array for vps, like (just hacky example):
>>>
>>>      struct {
>>>          const char *name;
>>>          const char *clkname;
>>>          struct tidss_vp_feat feat;
>>>      } vps[TIDSS_MAX_PORTS];
>>>
>>> and then use them as:
>>>
>>>      .vps = {
>>>          {
>>>              .name = "kala",
>>>              .clkname = "kissa",
>>>              .feat.color.has_ctm = true,
>>>          }, {
>>>              .name = "kala2",
>>>              .clkname = "kissa2",
>>>              .feat.color.has_ctm = false,
>>>          },
>>>      },
>>>
>>> Perhaps something to try in the future.
>>>
>>
>> Yes, agreed! Having that structure will tidy this up.
>> I will keep this under future work.
>>
>>>>    static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>>> @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>>>      const struct dispc_features dispc_am65x_feats = {
>>>>        .max_pclk_khz = {
>>>> -        [DISPC_VP_DPI] = 165000,
>>>> -        [DISPC_VP_OLDI] = 165000,
>>>> +        [DISPC_PORT_DPI] = 165000,
>>>> +        [DISPC_PORT_OLDI] = 165000,
>>>>        },
>>>>          .scaling = {
>>>> @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = {
>>>>        .vp_name = { "vp1", "vp2" },
>>>>        .ovr_name = { "ovr1", "ovr2" },
>>>>        .vpclk_name =  { "vp1", "vp2" },
>>>> -     .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
>>>>        .vp_feat = { .color = {
>>>>                .has_ctm = true,
>>>> @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = {
>>>>        .vid_name = { "vid", "vidl1" },
>>>>        .vid_lite = { false, true, },
>>>>        .vid_order = { 1, 0 },
>>>> +
>>>> +    .num_output_ports = 2,
>>>> +    .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI },
>>>>    };
>>>>      static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>>> @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>>>      const struct dispc_features dispc_j721e_feats = {
>>>>        .max_pclk_khz = {
>>>> -        [DISPC_VP_DPI] = 170000,
>>>> -        [DISPC_VP_INTERNAL] = 600000,
>>>> +        [DISPC_PORT_DPI] = 170000,
>>>> +        [DISPC_PORT_INTERNAL] = 600000,
>>>>        },
>>>>          .scaling = {
>>>> @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = {
>>>>        .vp_name = { "vp1", "vp2", "vp3", "vp4" },
>>>>        .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" },
>>>>        .vpclk_name = { "vp1", "vp2", "vp3", "vp4" },
>>>> -    /* Currently hard coded VP routing (see dispc_initial_config()) */
>>>> -    .vp_bus_type =    { DISPC_VP_INTERNAL, DISPC_VP_DPI,
>>>> -              DISPC_VP_INTERNAL, DISPC_VP_DPI, },
>>>> +
>>>
>>> I think this line feed is extra.
>>
>> Okay! Will remove that from all SoC feat structs.
>>
>>>
>>>>        .vp_feat = { .color = {
>>>>                .has_ctm = true,
>>>>                .gamma_size = 1024,
>>>> @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = {
>>>>        .vid_name = { "vid1", "vidl1", "vid2", "vidl2" },
>>>>        .vid_lite = { 0, 1, 0, 1, },
>>>>        .vid_order = { 1, 3, 0, 2 },
>>>> +
>>>> +    .num_output_ports = 4,
>>>> +    /* Currently hard coded VP routing (see dispc_initial_config()) */
>>>> +    .output_port_bus_type =    { DISPC_PORT_INTERNAL, DISPC_PORT_DPI,
>>>> +              DISPC_PORT_INTERNAL, DISPC_PORT_DPI, },
>>>
>>> Indent doesn't look right (but it might be just because this is a diff).
>>
>> I may have missed indenting this.
>>
>>>
>>>>    };
>>>>      static const u16 *dispc_common_regmap;
>>>> @@ -287,12 +294,12 @@ struct dispc_device {
>>>>       void __iomem *base_common;
>>>>       void __iomem *base_vid[TIDSS_MAX_PLANES];
>>>> -    void __iomem *base_ovr[TIDSS_MAX_PORTS];
>>>> -    void __iomem *base_vp[TIDSS_MAX_PORTS];
>>>> +    void __iomem *base_ovr[TIDSS_MAX_VPS];
>>>> +    void __iomem *base_vp[TIDSS_MAX_VPS];
>>>>       struct regmap *oldi_io_ctrl;
>>>> -    struct clk *vp_clk[TIDSS_MAX_PORTS];
>>>> +    struct clk *vp_clk[TIDSS_MAX_VPS];
>>>>          const struct dispc_features *feat;
>>>>    @@ -300,7 +307,7 @@ struct dispc_device {
>>>>          bool is_enabled;
>>>> -    struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
>>>> +    struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>>>>          u32 *fourccs;
>>>>        u32 num_fourccs;
>>>> @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport,
>>>>            return -EINVAL;
>>>>        }
>>>> -    if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI &&
>>>> +    if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI &&
>>>
>>> Hmm, so is the hw_videoport a vp index or an output index? Sounds like
>>> the former, so it's not right, even if at the moment they're identical.
>>> We need some kind of mapping between those.
>>>
>>
>> It is indeed a vp index. And yes, I can come up with a mapping mechanism.
>>
>>> If the mapping can be changed (or just defined in the DT), I think we
>>> need a variable in struct dispc_device, which tells the output to which
>>> a videoport is connected to. Or vice versa, I'm not sure which direction
>>> we need more. If the mapping is always the same on all SoC (but I don't
>>> think so), we can have it in the feats.
>>>
>>
>> As of now, the mapping is always same. But I would like to make is
>> generalized for future. Hence, I am considering to keep the variable in
>> struct dispc_device.
>>
>> My question though would be, how would one be able to find which kind
>> of device is the port connected to, if it is connected to a bridge? For
>> example, in case of panels, we have a "connector_type" variable in
>> drm_panel which tells what kind of sink it is. But there is no such
>> thing in drm_bridge.
>>
>> This is required because what if we can connect an videoport to either
>> an LVDS/OLDI bridge or a DPI bridge.
> 
> The connector type shouldn't matter.
> 
> The DSS has VPs and outputs. The VPs are "generic" and identical to each
> other, except in their possible connections to the outputs. The outputs,
> at least at the moment, are DPI, LVDS and internal, where internal is
> basically just DPI.
> 
> Those are the three different cases we are interested in within the dss
> driver, right? Does it matter where the DPI or LVDS output goes?
>

I believe it does. =)

While the VPs do always transmit DPI signals, the code in tidss_dispc.c
uses the information of the bus connected at the endpoint to configure
the OLDI parameters, and to turn OLDI IOs on and off in
dispc_vp_(prepare/unprepare).

Up until now, the outputs have been fixed (VP0 -> OLDI, VP1 -> DPI), and
the code used the enum dispc_vp_bus_type to differntiate between LVDS or
DPI requirements. But for a general case where output from VP0 could
either use the OLDI TXes and send out LVDS signals OR bypass the OLDI
TXes and send out DPI signals directly, we would need a mechanism to
find out which sink is present at the end, LVDS or DPI.

I assumed, with that mechanism, we could (re)configure the vp-to-output
mapping, which then would be used in the various places in
tidss_dispc.c.

> So what I'm saying is that the DSS device tree data should already
> define what kind of configuration we need, and there's no need to look
> further into the panel/bridge nodes.
> 
>> Also, implementing this might mean removal of the part of code which
>> confirms that the panel's "connector_type" indeed expects what the VP
>> can provide, unless there is a way to find out what the sink is before
>> calling the drm_of_find_panel_or_bridge API.
> 
> Hmm, well, each DSS output (port in DT) is of a certain type, so we
> should be able to validate that the output and the panel's
> connector_type match.
> 
>> On the direction, the primary requirement of hw_videoport has been in
>> the tidss_dispc.c file where the HW registers are getting configured.
>> 'hw_videoport' is a frequently passed parameter in function calls. So,
>> at a first glance the former option might makes more sense for
>> direction, i.e. to have a variable which tells the output to which a
>> videoport is connected to.
> 
> Makes sense.
> 
>> And while, there is also the tidss_kms.c file, which deals with
>> initializing encoders and attaching bridges. This is where the
>> output_port is required more often. But I am yet to think of a case
>> where the above direction could be an issue.
>>
>>
>>> Also, I wonder if output_port is a good name as it has "port" in it
>>> (like video port), and it's a bit long-ish. Would just "output" be
>>> enough? We could, of course, shorten it to OP, but that looks odd to me =).
>>>
>>
>>>>            fmt->is_oldi_fmt) {
>>>>            dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n",
>>>>                __func__, dispc->feat->vp_name[hw_videoport]);
>>>> @@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
>>>>        if (WARN_ON(!fmt))
>>>>            return;
>>>> -    if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) {
>>>> +    if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) {
>>>>            dispc_oldi_tx_power(dispc, true);
>>>>              dispc_enable_oldi(dispc, hw_videoport, fmt);
>>>> @@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
>>>>        align = true;
>>>>          /* always use DE_HIGH for OLDI */
>>>> -    if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI)
>>>> +    if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI)
>>>>            ieo = false;
>>>>          dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ,
>>>> @@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport)
>>>>      void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport)
>>>>    {
>>>> -    if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) {
>>>> +    if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) {
>>>>            dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0);
>>>>              dispc_oldi_tx_power(dispc, false);
>>>> @@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
>>>>                         const struct drm_display_mode *mode)
>>>>    {
>>>>       u32 hsw, hfp, hbp, vsw, vfp, vbp;
>>>> -    enum dispc_vp_bus_type bus_type;
>>>> +    enum dispc_port_bus_type bus_type;
>>>>       int max_pclk;
>>>> -    bus_type = dispc->feat->vp_bus_type[hw_videoport];
>>>> +    bus_type = dispc->feat->output_port_bus_type[hw_videoport];
>>>>       max_pclk = dispc->feat->max_pclk_khz[bus_type];
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
>>>> index e49432f0abf5..30fb44158347 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>>>> @@ -50,11 +50,11 @@ struct dispc_errata {
>>>>        bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */
>>>>    };
>>>> -enum dispc_vp_bus_type {
>>>> -    DISPC_VP_DPI,        /* DPI output */
>>>> -    DISPC_VP_OLDI,        /* OLDI (LVDS) output */
>>>> -    DISPC_VP_INTERNAL,    /* SoC internal routing */
>>>> -    DISPC_VP_MAX_BUS_TYPE,
>>>> +enum dispc_port_bus_type {
>>>> +    DISPC_PORT_DPI,            /* DPI output */
>>>> +    DISPC_PORT_OLDI,        /* OLDI (LVDS) output */
>>>> +    DISPC_PORT_INTERNAL,        /* SoC internal routing */
>>>> +    DISPC_PORT_MAX_BUS_TYPE,
>>>
>>> Okay, so here you have just "port", not "output_port". In the DT,
>>> they're ports, so... Maybe we could use that name too, and for video
>>> port always use "vp". The current "hw_videoport" could be easily
>>> mistaken with "port".
>>
>> I see what you are saying and how somebody could confusre hw_videoport
>> for a physical connection (i.e. port). I have always understoof
>> hw_videoport to be a thing of the actual VP inside the SoC, but that may
>> be because I have been working on this, and not just trying to
>> understand the code from a high level.
>>
>> How about if I change the output_port to "out_port"? I am okay to keep
>> "output" as the name to. I am saying this becuase I think, only keeping
>> "port" might just confuse more, as you mentioned above.
> 
> Yes, I agree "port" is not good. Other than that, no strong opinions.
> Whatever name you pick, someone will find it confusing ;). Just keep it
> consistent, so that all enums, parameters, etc. use it a consistent
> manner. If I had to choose, I'd go with the "output", but I'm fine with
> other names too.
> 

Alright! I am good with using "output".

>> And then we can change "hw_videoport" to "vp_index", perhaps, or even
>> keep that as it is? Becuase if we do have a VP structure in future
>> like you suggested above, "vps" and "vp" would become a close overlap.
>> For eg, "vps[vp]".
> 
> That is true. I think that was the reason I chose hw_videoport instead
> of videoport or vp, although vps[hw_videoport] is only barely better.
> 
> vp_index is ok for me, or maybe vp_idx or vp_num to have it a bit shorter.
> 
"vp_idx" seems better!


Regards
Aradhya

Re: [PATCH v7 4/6] drm/tidss: Add support to configure OLDI mode for am625-dss

[Aradhya Bhatia]

On 06-Feb-23 19:12, Tomi Valkeinen wrote:
> On 05/02/2023 15:42, Aradhya Bhatia wrote:
>> Hi Tomi,
>>
>> On 03-Feb-23 20:42, Tomi Valkeinen wrote:
>>> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>>>> The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal.
>>>> These can be configured to support the following modes:
>>>>
>>>> 1. OLDI_SINGLE_LINK_SINGLE_MODE
>>>> Single Output over OLDI 0.
>>>> +------+        +---------+      +-------+
>>>> |      |        |         |      |       |
>>>> | CRTC +------->+ ENCODER +----->| PANEL |
>>>> |      |        |         |      |       |
>>>> +------+        +---------+      +-------+
>>>>
>>>> 2. OLDI_SINGLE_LINK_CLONE_MODE
>>>> Duplicate Output over OLDI 0 and 1.
>>>> +------+        +---------+      +-------+
>>>> |      |        |         |      |       |
>>>> | CRTC +---+--->| ENCODER +----->| PANEL |
>>>> |      |   |    |         |      |       |
>>>> +------+   |    +---------+      +-------+
>>>>              |
>>>>              |    +---------+      +-------+
>>>>              |    |         |      |       |
>>>>              +--->| ENCODER +----->| PANEL |
>>>>                   |         |      |       |
>>>>                   +---------+      +-------+
>>>>
>>>> 3. OLDI_DUAL_LINK_MODE
>>>> Combined Output over OLDI 0 and 1.
>>>> +------+        +---------+      +-------+
>>>> |      |        |         +----->|       |
>>>> | CRTC +------->+ ENCODER |      | PANEL |
>>>> |      |        |         +----->|       |
>>>> +------+        +---------+      +-------+
>>>>
>>>> Following the above pathways for different modes, 2 encoder/panel-bridge
>>>> pipes get created for clone mode, and 1 pipe in cases of single link and
>>>> dual link mode.
>>>>
>>>> Add support for confguring the OLDI modes using OF and LVDS DRM helper
>>>> functions.
>>>>
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>> ---
>>>>    drivers/gpu/drm/tidss/tidss_dispc.c   |  24 ++-
>>>>    drivers/gpu/drm/tidss/tidss_dispc.h   |  12 ++
>>>>    drivers/gpu/drm/tidss/tidss_drv.h     |   3 +
>>>>    drivers/gpu/drm/tidss/tidss_encoder.c |   4 +-
>>>>    drivers/gpu/drm/tidss/tidss_encoder.h |   3 +-
>>>>    drivers/gpu/drm/tidss/tidss_kms.c     | 221 ++++++++++++++++++++++++--
>>>>    6 files changed, 245 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> index b55ccbcaa67f..37a73e309330 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> @@ -88,6 +88,8 @@ const struct dispc_features dispc_k2g_feats = {
>>>>          .subrev = DISPC_K2G,
>>>>    +    .has_oldi = false,
>>>> +
>>>>        .common = "common",
>>>>          .common_regs = tidss_k2g_common_regs,
>>>> @@ -166,6 +168,8 @@ const struct dispc_features dispc_am625_feats = {
>>>>          .subrev = DISPC_AM625,
>>>>    +    .has_oldi = true,
>>>> +
>>>>        .common = "common",
>>>>        .common_regs = tidss_am65x_common_regs,
>>>>    @@ -218,6 +222,8 @@ const struct dispc_features dispc_am65x_feats = {
>>>>          .subrev = DISPC_AM65X,
>>>>    +    .has_oldi = true,
>>>> +
>>>>        .common = "common",
>>>>        .common_regs = tidss_am65x_common_regs,
>>>>    @@ -309,6 +315,8 @@ const struct dispc_features dispc_j721e_feats = {
>>>>          .subrev = DISPC_J721E,
>>>>    +    .has_oldi = false,
>>>> +
>>>>        .common = "common_m",
>>>>        .common_regs = tidss_j721e_common_regs,
>>>>    @@ -361,6 +369,8 @@ struct dispc_device {
>>>>          struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>>>>    +    enum dispc_oldi_modes oldi_mode;
>>>> +
>>>>        u32 *fourccs;
>>>>        u32 num_fourccs;
>>>>    @@ -1963,6 +1973,12 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len)
>>>>        return dispc->fourccs;
>>>>    }
>>>>    +void dispc_set_oldi_mode(struct dispc_device *dispc,
>>>> +             enum dispc_oldi_modes oldi_mode)
>>>> +{
>>>> +    dispc->oldi_mode = oldi_mode;
>>>> +}
>>>> +
>>>>    static s32 pixinc(int pixels, u8 ps)
>>>>    {
>>>>        if (pixels == 1)
>>>> @@ -2647,7 +2663,7 @@ int dispc_runtime_resume(struct dispc_device *dispc)
>>>>            REG_GET(dispc, DSS_SYSSTATUS, 2, 2),
>>>>            REG_GET(dispc, DSS_SYSSTATUS, 3, 3));
>>>>    -    if (dispc->feat->subrev == DISPC_AM65X)
>>>> +    if (dispc->feat->has_oldi)
>>>>            dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n",
>>>>                REG_GET(dispc, DSS_SYSSTATUS, 5, 5),
>>>>                REG_GET(dispc, DSS_SYSSTATUS, 6, 6),
>>>> @@ -2688,7 +2704,7 @@ static int dispc_iomap_resource(struct platform_device *pdev, const char *name,
>>>>        return 0;
>>>>    }
>>>>    -static int dispc_init_am65x_oldi_io_ctrl(struct device *dev,
>>>> +static int dispc_init_am6xx_oldi_io_ctrl(struct device *dev,
>>>>                         struct dispc_device *dispc)
>>>>    {
>>>>        dispc->oldi_io_ctrl =
>>>> @@ -2827,8 +2843,8 @@ int dispc_init(struct tidss_device *tidss)
>>>>            dispc->vp_data[i].gamma_table = gamma_table;
>>>>        }
>>>>    -    if (feat->subrev == DISPC_AM65X) {
>>>> -        r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
>>>> +    if (feat->has_oldi) {
>>>> +        r = dispc_init_am6xx_oldi_io_ctrl(dev, dispc);
>>>>            if (r)
>>>>                return r;
>>>>        }
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
>>>> index 971f2856f015..880bc7de68b3 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>>>> @@ -64,6 +64,15 @@ enum dispc_dss_subrevision {
>>>>        DISPC_J721E,
>>>>    };
>>>>    +enum dispc_oldi_modes {
>>>> +    OLDI_MODE_SINGLE_LINK,        /* Single output over OLDI 0. */
>>>> +    OLDI_MODE_CLONE_SINGLE_LINK,    /* Cloned output over OLDI 0 and 1. */
>>>> +    OLDI_MODE_DUAL_LINK,        /* Combined output over OLDI 0 and 1. */
>>>> +    OLDI_MODE_OFF,            /* OLDI TXes not connected in OF. */
>>>> +    OLDI_MODE_UNSUPPORTED,        /* Unsupported OLDI configuration in OF. */
>>>> +    OLDI_MODE_UNAVAILABLE,        /* OLDI TXes not available in SoC. */
>>>> +};
>>>> +
>>>>    struct dispc_features {
>>>>        int min_pclk_khz;
>>>>        int max_pclk_khz[DISPC_PORT_MAX_BUS_TYPE];
>>>> @@ -72,6 +81,8 @@ struct dispc_features {
>>>>          enum dispc_dss_subrevision subrev;
>>>>    +    bool has_oldi;
>>>> +
>>>>        const char *common;
>>>>        const u16 *common_regs;
>>>>        u32 num_vps;
>>>> @@ -131,6 +142,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
>>>>                  u32 hw_videoport);
>>>>    int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable);
>>>>    const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len);
>>>> +void dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode);
>>>>      int dispc_init(struct tidss_device *tidss);
>>>>    void dispc_remove(struct tidss_device *tidss);
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
>>>> index 0ce7ee5ccd5b..58892f065c16 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_drv.h
>>>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
>>>> @@ -13,6 +13,9 @@
>>>>    #define TIDSS_MAX_PLANES 4
>>>>    #define TIDSS_MAX_OUTPUT_PORTS 4
>>>>    +/* For AM625-DSS with 2 OLDI TXes */
>>>> +#define TIDSS_MAX_BRIDGES_PER_PIPE    2
>>>> +
>>>>    typedef u32 dispc_irq_t;
>>>>      struct tidss_device {
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
>>>> index 0d4865e9c03d..bd2a7358d7b0 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_encoder.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
>>>> @@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = {
>>>>    };
>>>>      struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>>>> -                     u32 encoder_type, u32 possible_crtcs)
>>>> +                     u32 encoder_type, u32 possible_crtcs,
>>>> +                     u32 possible_clones)
>>>>    {
>>>>        struct drm_encoder *enc;
>>>>        int ret;
>>>> @@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>>>>            return ERR_PTR(-ENOMEM);
>>>>          enc->possible_crtcs = possible_crtcs;
>>>> +    enc->possible_clones = possible_clones;
>>>>          ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs,
>>>>                       encoder_type, NULL);
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h
>>>> index ace877c0e0fd..01c62ba3ef16 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_encoder.h
>>>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.h
>>>> @@ -12,6 +12,7 @@
>>>>    struct tidss_device;
>>>>      struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>>>> -                     u32 encoder_type, u32 possible_crtcs);
>>>> +                     u32 encoder_type, u32 possible_crtcs,
>>>> +                     u32 possible_clones);
>>>>      #endif
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
>>>> index d449131935d2..8322ee6310bf 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_kms.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
>>>> @@ -13,6 +13,7 @@
>>>>    #include <drm/drm_of.h>
>>>>    #include <drm/drm_panel.h>
>>>>    #include <drm/drm_vblank.h>
>>>> +#include <linux/of.h>
>>>>      #include "tidss_crtc.h"
>>>>    #include "tidss_dispc.h"
>>>> @@ -104,26 +105,129 @@ static const struct drm_mode_config_funcs mode_config_funcs = {
>>>>        .atomic_commit = drm_atomic_helper_commit,
>>>>    };
>>>>    +static enum dispc_oldi_modes tidss_get_oldi_mode(struct tidss_device *tidss)
>>>> +{
>>>> +    int pixel_order;
>>>> +    enum dispc_oldi_modes oldi_mode;
>>>> +    struct device_node *oldi0_port, *oldi1_port;
>>>> +
>>>> +    /*
>>>> +     * For am625-dss, the OLDI ports are expected at port reg = 0 and 2,
>>>> +     * and for am65x-dss, the OLDI port is expected only at port reg = 0.
>>>> +     */
>>>> +    const u32 portnum_oldi0 = 0, portnum_oldi1 = 2;
>>>> +
>>>> +    oldi0_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi0);
>>>> +    oldi1_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi1);
>>>> +
>>>> +    if (!(oldi0_port || oldi1_port)) {
>>>> +        /* Keep OLDI TXes OFF if neither OLDI port is present. */
>>>> +        oldi_mode = OLDI_MODE_OFF;
>>>> +    } else if (oldi0_port && !oldi1_port) {
>>>> +        /*
>>>> +         * OLDI0 port found, but not OLDI1 port. Setting single
>>>> +         * link output mode.
>>>> +         */
>>>> +        oldi_mode = OLDI_MODE_SINGLE_LINK;
>>>> +    } else if (!oldi0_port && oldi1_port) {
>>>> +        /*
>>>> +         * The 2nd OLDI TX cannot be operated alone. This use case is
>>>> +         * not supported in the HW. Since the pins for OLDIs 0 and 1 are
>>>> +         * separate, one could theoretically set a clone mode over OLDIs
>>>> +         * 0 and 1 and just simply not use the OLDI 0. This is a hacky
>>>> +         * way to enable only OLDI TX 1 and hence is not officially
>>>> +         * supported.
>>>> +         */
>>>> +        dev_warn(tidss->dev,
>>>> +             "Single Mode over OLDI 1 is not supported in HW.\n");
>>>> +        oldi_mode = OLDI_MODE_UNSUPPORTED;
>>>> +    } else {
>>>> +        /*
>>>> +         * OLDI Ports found for both the OLDI TXes. The DSS is to be
>>>> +         * configured in either Dual Link or Clone Mode.
>>>> +         */
>>>> +        pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port,
>>>> +                                    oldi1_port);
>>>> +        switch (pixel_order) {
>>>> +        case -EINVAL:
>>>> +            /*
>>>> +             * The dual link properties were not found in at least
>>>> +             * one of the sink nodes. Since 2 OLDI ports are present
>>>> +             * in the DT, it can be safely assumed that the required
>>>> +             * configuration is Clone Mode.
>>>> +             */
>>>> +            oldi_mode = OLDI_MODE_CLONE_SINGLE_LINK;
>>>> +            break;
>>>> +
>>>> +        case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
>>>> +            /*
>>>> +             * Note that the OLDI TX 0 transmits the odd set of
>>>> +             * pixels while the OLDI TX 1 transmits the even set.
>>>> +             * This is a fixed configuration in the HW and an cannot
>>>> +             * be change via SW.
>>>> +             */
>>>> +            dev_warn(tidss->dev,
>>>> +                 "EVEN-ODD Dual-Link Mode is not supported in HW.\n");
>>>> +            oldi_mode = OLDI_MODE_UNSUPPORTED;
>>>> +            break;
>>>> +
>>>> +        case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
>>>> +            oldi_mode = OLDI_MODE_DUAL_LINK;
>>>> +            break;
>>>> +
>>>> +        default:
>>>> +            oldi_mode = OLDI_MODE_UNSUPPORTED;
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    of_node_put(oldi0_port);
>>>> +    of_node_put(oldi1_port);
>>>> +
>>>> +    return oldi_mode;
>>>> +}
>>>> +
>>>>    static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>>>>    {
>>>>        struct device *dev = tidss->dev;
>>>>        unsigned int fourccs_len;
>>>>        const u32 *fourccs = dispc_plane_formats(tidss->dispc, &fourccs_len);
>>>> -    unsigned int i;
>>>> +    unsigned int i, j;
>>>>          struct pipe {
>>>>            u32 hw_videoport;
>>>> -        struct drm_bridge *bridge;
>>>> +        struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE];
>>>>            u32 enc_type;
>>>> +        u32 num_bridges;
>>>>        };
>>>>          const struct dispc_features *feat = tidss->feat;
>>>>        u32 output_ports = feat->num_output_ports;
>>>>        u32 max_planes = feat->num_planes;
>>>>    -    struct pipe pipes[TIDSS_MAX_VPS];
>>>> +    struct pipe pipes[TIDSS_MAX_VPS] = {0};
>>>> +
>>>>        u32 num_pipes = 0;
>>>>        u32 crtc_mask;
>>>> +    enum dispc_oldi_modes oldi_mode = OLDI_MODE_UNAVAILABLE;
>>>> +    u32 num_oldi = 0;
>>>> +    u32 num_encoders = 0;
>>>> +    u32 oldi_pipe_index = 0;
>>>> +
>>>> +    if (feat->has_oldi) {
>>>> +        oldi_mode = tidss_get_oldi_mode(tidss);
>>>> +
>>>> +        if ((oldi_mode == OLDI_MODE_DUAL_LINK ||
>>>> +             oldi_mode == OLDI_MODE_CLONE_SINGLE_LINK) &&
>>>> +            feat->subrev == DISPC_AM65X) {
>>>> +            dev_warn(tidss->dev,
>>>> +                 "am65x-dss does not support this OLDI mode.\n");
>>>> +
>>>> +            oldi_mode = OLDI_MODE_UNSUPPORTED;
>>>> +        }
>>>
>>> Shouldn't OLDI_MODE_UNSUPPORTED be handled as an error? It means the DT
>>> is faulty, doesn't it? Maybe it could even be renamed to
>>> OLDI_MODE_ERROR. Or tidss_get_oldi_mode() could return a negative error
>>> code.
>>>
>>
>> The idea was to let the framework continue configuring the 2nd videoport
>> for DPI, even if the OLDI DT is wrong. But I have come across more
>> examples recently where that is not the case. DT error for one pipe has
>> resulted in returning of an error code.
>>
>> Will make the change.
> 
> My opinion is that the DT has to be correct. If it isn't, just fail and
> exit as soon as possible. There shouldn't be any reasons for the drivers
> to be trying to cope with a broken DT.

Understood! Will error out in those cases!

> 
>>>> +
>>>> +        dispc_set_oldi_mode(tidss->dispc, oldi_mode);
>>>> +    }
>>>
>>> Would it be better to move the above dispc_set_oldi_mode() to be outside
>>> the if block? Then oldi mode would be set to OLDI_MODE_UNAVAILABLE on
>>> SoCs that don't have OLDI.
>>
>> Ahh, yes! Will make the change.
>>
>>>
>>> tidss_get_oldi_mode and dispc_set_oldi_mode sound like opposites, but
>>> they're totally different things. Maybe tidss_get_oldi_mode should
>>> rather be something about parsing oldi dt properties or such.
>>
>> Okay! Is 'tidss_parse_oldi_properties' acceptable? This is just
>> something I came up with now. I can think of more if this is not good.
> 
> Sounds fine.
>

Okay!
 

Regards
Aradhya

Re: [PATCH v7 1/6] drm/tidss: Remove Video Port to Output Port coupling

[Tomi Valkeinen]

On 06/02/2023 19:34, Aradhya Bhatia wrote:
> 
> On 06-Feb-23 18:35, Tomi Valkeinen wrote:
>> On 05/02/2023 15:08, Aradhya Bhatia wrote:
>>> Hi Tomi,
>>>
>>> Thanks for the review!
>>>
>>> On 03-Feb-23 16:53, Tomi Valkeinen wrote:
>>>> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>>>>> Make DSS Video Ports agnostic of output bus types.
>>>>>
>>>>> DSS controllers have had a 1-to-1 coupling between its VPs and its
>>>>> output ports. This no longer stands true for the new AM625 DSS. This
>>>>> coupling, hence, has been removed by renaming the 'vp_bus_type' to
>>>>> 'output_port_bus_type' because the VPs are essentially agnostic of the
>>>>> bus type and it is the output ports which have a bus type.
>>>>>
>>>>> The AM625 DSS has 2 VPs but requires 3 output ports to support its
>>>>> Dual-Link OLDI video output coming from a single VP.
>>>>
>>>> Not a biggie, but this sentence is a bit odd here at the end. Shouldn't
>>>> it be after the "...stands true for the new AM625 DSS."?
>>>
>>> Yes! It should be. Will make the edit.
>>>
>>>>
>>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>>> ---
>>>>>     drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------
>>>>>     drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------
>>>>>     drivers/gpu/drm/tidss/tidss_drv.h   |  5 +--
>>>>>     drivers/gpu/drm/tidss/tidss_irq.h   |  2 +-
>>>>>     drivers/gpu/drm/tidss/tidss_kms.c   | 12 ++++----
>>>>>     5 files changed, 48 insertions(+), 39 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>>> index 165365b515e1..c1c4faccbddc 100644
>>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>>> @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = {
>>>>>         .min_pclk_khz = 4375,
>>>>>           .max_pclk_khz = {
>>>>> -        [DISPC_VP_DPI] = 150000,
>>>>> +        [DISPC_PORT_DPI] = 150000,
>>>>>         },
>>>>>           /*
>>>>> @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = {
>>>>>         .vp_name = { "vp1" },
>>>>>         .ovr_name = { "ovr1" },
>>>>>         .vpclk_name =  { "vp1" },
>>>>> -    .vp_bus_type = { DISPC_VP_DPI },
>>>>>           .vp_feat = { .color = {
>>>>>                 .has_ctm = true,
>>>>> @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = {
>>>>>         .vid_name = { "vid1" },
>>>>>         .vid_lite = { false },
>>>>>         .vid_order = { 0 },
>>>>> +
>>>>> +    .num_output_ports = 1,
>>>>> +    .output_port_bus_type = { DISPC_PORT_DPI },
>>>>>     };
>>>>
>>>> Just thinking out loud, as these will get more complex in the future,
>>>> maybe we should finally group them with struct. E.g. we could define
>>>> struct array for vps, like (just hacky example):
>>>>
>>>>       struct {
>>>>           const char *name;
>>>>           const char *clkname;
>>>>           struct tidss_vp_feat feat;
>>>>       } vps[TIDSS_MAX_PORTS];
>>>>
>>>> and then use them as:
>>>>
>>>>       .vps = {
>>>>           {
>>>>               .name = "kala",
>>>>               .clkname = "kissa",
>>>>               .feat.color.has_ctm = true,
>>>>           }, {
>>>>               .name = "kala2",
>>>>               .clkname = "kissa2",
>>>>               .feat.color.has_ctm = false,
>>>>           },
>>>>       },
>>>>
>>>> Perhaps something to try in the future.
>>>>
>>>
>>> Yes, agreed! Having that structure will tidy this up.
>>> I will keep this under future work.
>>>
>>>>>     static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>>>> @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>>>>       const struct dispc_features dispc_am65x_feats = {
>>>>>         .max_pclk_khz = {
>>>>> -        [DISPC_VP_DPI] = 165000,
>>>>> -        [DISPC_VP_OLDI] = 165000,
>>>>> +        [DISPC_PORT_DPI] = 165000,
>>>>> +        [DISPC_PORT_OLDI] = 165000,
>>>>>         },
>>>>>           .scaling = {
>>>>> @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = {
>>>>>         .vp_name = { "vp1", "vp2" },
>>>>>         .ovr_name = { "ovr1", "ovr2" },
>>>>>         .vpclk_name =  { "vp1", "vp2" },
>>>>> -     .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
>>>>>         .vp_feat = { .color = {
>>>>>                 .has_ctm = true,
>>>>> @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = {
>>>>>         .vid_name = { "vid", "vidl1" },
>>>>>         .vid_lite = { false, true, },
>>>>>         .vid_order = { 1, 0 },
>>>>> +
>>>>> +    .num_output_ports = 2,
>>>>> +    .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI },
>>>>>     };
>>>>>       static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>>>> @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>>>>       const struct dispc_features dispc_j721e_feats = {
>>>>>         .max_pclk_khz = {
>>>>> -        [DISPC_VP_DPI] = 170000,
>>>>> -        [DISPC_VP_INTERNAL] = 600000,
>>>>> +        [DISPC_PORT_DPI] = 170000,
>>>>> +        [DISPC_PORT_INTERNAL] = 600000,
>>>>>         },
>>>>>           .scaling = {
>>>>> @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = {
>>>>>         .vp_name = { "vp1", "vp2", "vp3", "vp4" },
>>>>>         .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" },
>>>>>         .vpclk_name = { "vp1", "vp2", "vp3", "vp4" },
>>>>> -    /* Currently hard coded VP routing (see dispc_initial_config()) */
>>>>> -    .vp_bus_type =    { DISPC_VP_INTERNAL, DISPC_VP_DPI,
>>>>> -              DISPC_VP_INTERNAL, DISPC_VP_DPI, },
>>>>> +
>>>>
>>>> I think this line feed is extra.
>>>
>>> Okay! Will remove that from all SoC feat structs.
>>>
>>>>
>>>>>         .vp_feat = { .color = {
>>>>>                 .has_ctm = true,
>>>>>                 .gamma_size = 1024,
>>>>> @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = {
>>>>>         .vid_name = { "vid1", "vidl1", "vid2", "vidl2" },
>>>>>         .vid_lite = { 0, 1, 0, 1, },
>>>>>         .vid_order = { 1, 3, 0, 2 },
>>>>> +
>>>>> +    .num_output_ports = 4,
>>>>> +    /* Currently hard coded VP routing (see dispc_initial_config()) */
>>>>> +    .output_port_bus_type =    { DISPC_PORT_INTERNAL, DISPC_PORT_DPI,
>>>>> +              DISPC_PORT_INTERNAL, DISPC_PORT_DPI, },
>>>>
>>>> Indent doesn't look right (but it might be just because this is a diff).
>>>
>>> I may have missed indenting this.
>>>
>>>>
>>>>>     };
>>>>>       static const u16 *dispc_common_regmap;
>>>>> @@ -287,12 +294,12 @@ struct dispc_device {
>>>>>        void __iomem *base_common;
>>>>>        void __iomem *base_vid[TIDSS_MAX_PLANES];
>>>>> -    void __iomem *base_ovr[TIDSS_MAX_PORTS];
>>>>> -    void __iomem *base_vp[TIDSS_MAX_PORTS];
>>>>> +    void __iomem *base_ovr[TIDSS_MAX_VPS];
>>>>> +    void __iomem *base_vp[TIDSS_MAX_VPS];
>>>>>        struct regmap *oldi_io_ctrl;
>>>>> -    struct clk *vp_clk[TIDSS_MAX_PORTS];
>>>>> +    struct clk *vp_clk[TIDSS_MAX_VPS];
>>>>>           const struct dispc_features *feat;
>>>>>     @@ -300,7 +307,7 @@ struct dispc_device {
>>>>>           bool is_enabled;
>>>>> -    struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
>>>>> +    struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>>>>>           u32 *fourccs;
>>>>>         u32 num_fourccs;
>>>>> @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport,
>>>>>             return -EINVAL;
>>>>>         }
>>>>> -    if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI &&
>>>>> +    if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI &&
>>>>
>>>> Hmm, so is the hw_videoport a vp index or an output index? Sounds like
>>>> the former, so it's not right, even if at the moment they're identical.
>>>> We need some kind of mapping between those.
>>>>
>>>
>>> It is indeed a vp index. And yes, I can come up with a mapping mechanism.
>>>
>>>> If the mapping can be changed (or just defined in the DT), I think we
>>>> need a variable in struct dispc_device, which tells the output to which
>>>> a videoport is connected to. Or vice versa, I'm not sure which direction
>>>> we need more. If the mapping is always the same on all SoC (but I don't
>>>> think so), we can have it in the feats.
>>>>
>>>
>>> As of now, the mapping is always same. But I would like to make is
>>> generalized for future. Hence, I am considering to keep the variable in
>>> struct dispc_device.
>>>
>>> My question though would be, how would one be able to find which kind
>>> of device is the port connected to, if it is connected to a bridge? For
>>> example, in case of panels, we have a "connector_type" variable in
>>> drm_panel which tells what kind of sink it is. But there is no such
>>> thing in drm_bridge.
>>>
>>> This is required because what if we can connect an videoport to either
>>> an LVDS/OLDI bridge or a DPI bridge.
>>
>> The connector type shouldn't matter.
>>
>> The DSS has VPs and outputs. The VPs are "generic" and identical to each
>> other, except in their possible connections to the outputs. The outputs,
>> at least at the moment, are DPI, LVDS and internal, where internal is
>> basically just DPI.
>>
>> Those are the three different cases we are interested in within the dss
>> driver, right? Does it matter where the DPI or LVDS output goes?
>>
> 
> I believe it does. =)
> 
> While the VPs do always transmit DPI signals, the code in tidss_dispc.c
> uses the information of the bus connected at the endpoint to configure
> the OLDI parameters, and to turn OLDI IOs on and off in
> dispc_vp_(prepare/unprepare).
> 
> Up until now, the outputs have been fixed (VP0 -> OLDI, VP1 -> DPI), and
> the code used the enum dispc_vp_bus_type to differntiate between LVDS or
> DPI requirements. But for a general case where output from VP0 could
> either use the OLDI TXes and send out LVDS signals OR bypass the OLDI
> TXes and send out DPI signals directly, we would need a mechanism to
> find out which sink is present at the end, LVDS or DPI.
> 
> I assumed, with that mechanism, we could (re)configure the vp-to-output
> mapping, which then would be used in the various places in
> tidss_dispc.c.

But we should already know all that. Say, on AM625, we have three ports 
(or "outputs") (defined in DT), OLDI1, DPI, OLDI2. If there's an 
endpoint configured for the first port, we know we need to set up OLDI, 
and we need a VP for it. If the hardware mapping between VPs and outputs 
is hardcoded, we know directly that VP0 is needed, and it's used for 
OLDI. So now we have the mapping of VP0 -> OLDI (port1).

If (say) VP0 could alternatively be used for DPI output, then we'd see 
the second port, DPI, having an endpoint configured. Having both OLDI1 
and DPI endpoints would be invalid, of course.

So "how would one be able to find which kind of device is the port 
connected to" is irrelevant, I think. We know the output port, so we 
know the bus type, and we don't really need to know anything else about 
what's there behind the bus.

Or is there some detail I'm missing here?

  Tomi