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

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

[Aradhya Bhatia]

This patch series adds a new compatible for the Display SubSyetem
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 supports
OLDI displays with a resolution of upto 2K.

This patch set further adds support for the OLDI on AM625, and is, in
essence, the second version of the following patch series:
https://patchwork.kernel.org/project/dri-devel/list/?series=660970&state=%2A&archive=both

The changes in the above-mentioned series forced some re-works in this
series, and are better reviewed as a single set.

TODO:
  - Support for OLDI Bridges that work on clone / dual-link modes is yet
    to be added.

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

Note:
  - Due to lack of hardware, only dual-link mode has been tested.

Changelog:
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 regsiter array

Aradhya Bhatia (6):
  dt-bindings: display: ti,am65x-dss: Add am625 dss compatible
  dt-bindings: display: ti: am65x-dss: Add new port 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     |  22 ++-
 drivers/gpu/drm/tidss/tidss_dispc.c           | 155 ++++++++++++++++--
 drivers/gpu/drm/tidss/tidss_dispc.h           |  11 ++
 drivers/gpu/drm/tidss/tidss_dispc_regs.h      |   6 +
 drivers/gpu/drm/tidss/tidss_drv.c             |   1 +
 drivers/gpu/drm/tidss/tidss_drv.h             |   3 +
 drivers/gpu/drm/tidss/tidss_kms.c             | 146 ++++++++++++++---
 7 files changed, 304 insertions(+), 40 deletions(-)

-- 
2.37.0

[RFC PATCH v5 1/6] dt-bindings: display: ti,am65x-dss: Add am625 dss compatible

[Aradhya Bhatia]

Add ti,am625-dss compatible string.
The DSS IP on TI's AM625 SoC is an update from the DSS on TI's AM65X
SoC. The former has an additional OLDI TX to enable a 2K resolution on
OLDI displays or enable 2 duplicated displays with a smaller resolution.

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

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 5c7d2cbc4aac..6bbce921479d 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:
-- 
2.37.0

Re: [RFC PATCH v5 1/6] dt-bindings: display: ti,am65x-dss: Add am625 dss compatible

[Tomi Valkeinen]

On 28/09/2022 20:52, Aradhya Bhatia wrote:
> Add ti,am625-dss compatible string.
> The DSS IP on TI's AM625 SoC is an update from the DSS on TI's AM65X
> SoC. The former has an additional OLDI TX to enable a 2K resolution on
> OLDI displays or enable 2 duplicated displays with a smaller resolution.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> Reviewed-by: Rahul T R <r-ravikumar@ti.com>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>   .../devicetree/bindings/display/ti/ti,am65x-dss.yaml          | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 5c7d2cbc4aac..6bbce921479d 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:

I think you can squash this and the next one together.

  Tomi

[RFC PATCH v5 2/6] dt-bindings: display: ti: am65x-dss: Add new port for am625-dss

[Aradhya Bhatia]

Add 3rd "port" property for am625-dss.
This port represents the output from the 2nd OLDI TX (OLDI TX 1) latched
onto the first video port (VP0) from the DSS controller on AM625 SOC.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 .../bindings/display/ti/ti,am65x-dss.yaml      | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 6bbce921479d..99576c6ec108 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -82,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:
@@ -104,6 +109,17 @@ properties:
       Input memory (from main memory to dispc) bandwidth limit in
       bytes per second
 
+if:
+  properties:
+    compatible:
+      contains:
+        const: ti,am65x-dss
+then:
+  properties:
+    ports:
+      properties:
+        port@2: false
+
 required:
   - compatible
   - reg
-- 
2.37.0

Re: [RFC PATCH v5 2/6] dt-bindings: display: ti: am65x-dss: Add new port for am625-dss

[Krzysztof Kozlowski]

On 28/09/2022 19:52, Aradhya Bhatia wrote:
> Add 3rd "port" property for am625-dss.
> This port represents the output from the 2nd OLDI TX (OLDI TX 1) latched
> onto the first video port (VP0) from the DSS controller on AM625 SOC.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>  .../bindings/display/ti/ti,am65x-dss.yaml      | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 6bbce921479d..99576c6ec108 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -82,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:
> @@ -104,6 +109,17 @@ properties:
>        Input memory (from main memory to dispc) bandwidth limit in
>        bytes per second
>  
> +if:

Entire if: block is usually put under allOf:, so when the list of
conditions grow, you do not need to change indentation.

With the change:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[RFC PATCH v5 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 port has been added with "DISPC_VP_OLDI" bus type.

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

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index dd3c6a606ae2..34f0da4bb3e3 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -93,6 +93,7 @@ const struct dispc_features dispc_k2g_feats = {
 	.common_regs = tidss_k2g_common_regs,
 
 	.num_vps = 1,
+	.num_max_ports = 1,
 	.vp_name = { "vp1" },
 	.ovr_name = { "ovr1" },
 	.vpclk_name =  { "vp1" },
@@ -168,6 +169,7 @@ const struct dispc_features dispc_am65x_feats = {
 	.common_regs = tidss_am65x_common_regs,
 
 	.num_vps = 2,
+	.num_max_ports = 2,
 	.vp_name = { "vp1", "vp2" },
 	.ovr_name = { "ovr1", "ovr2" },
 	.vpclk_name =  { "vp1", "vp2" },
@@ -257,6 +259,7 @@ const struct dispc_features dispc_j721e_feats = {
 	.common_regs = tidss_j721e_common_regs,
 
 	.num_vps = 4,
+	.num_max_ports = 4,
 	.vp_name = { "vp1", "vp2", "vp3", "vp4" },
 	.ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" },
 	.vpclk_name = { "vp1", "vp2", "vp3", "vp4" },
@@ -275,6 +278,57 @@ const struct dispc_features dispc_j721e_feats = {
 	.vid_order = { 1, 3, 0, 2 },
 };
 
+const struct dispc_features dispc_am625_feats = {
+	.max_pclk_khz = {
+		[DISPC_VP_DPI] = 165000,
+		[DISPC_VP_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,
+	/* note: the 3rd port is not representative of a 3rd pipeline */
+	.num_max_ports = 3,
+	.vp_name = { "vp1", "vp2" },
+	.ovr_name = { "ovr1", "ovr2" },
+	.vpclk_name =  { "vp1", "vp2" },
+	.vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI, DISPC_VP_OLDI },
+
+	.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 },
+};
+
 static const u16 *dispc_common_regmap;
 
 struct dss_vp_data {
@@ -778,6 +832,7 @@ dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc)
 		return dispc_k2g_read_and_clear_irqstatus(dispc);
 	case DISPC_AM65X:
 	case DISPC_J721E:
+	case DISPC_AM625:
 		return dispc_k3_read_and_clear_irqstatus(dispc);
 	default:
 		WARN_ON(1);
@@ -793,6 +848,7 @@ void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
 		break;
 	case DISPC_AM65X:
 	case DISPC_J721E:
+	case DISPC_AM625:
 		dispc_k3_set_irqenable(dispc, mask);
 		break;
 	default:
@@ -1282,6 +1338,7 @@ void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane,
 					x, y, layer);
 		break;
 	case DISPC_AM65X:
+	case DISPC_AM625:
 		dispc_am65x_ovr_set_plane(dispc, hw_plane, hw_videoport,
 					  x, y, layer);
 		break;
@@ -2205,6 +2262,7 @@ static void dispc_plane_init(struct dispc_device *dispc)
 		break;
 	case DISPC_AM65X:
 	case DISPC_J721E:
+	case DISPC_AM625:
 		dispc_k3_plane_init(dispc);
 		break;
 	default:
@@ -2310,6 +2368,7 @@ static void dispc_vp_write_gamma_table(struct dispc_device *dispc,
 		dispc_k2g_vp_write_gamma_table(dispc, hw_videoport);
 		break;
 	case DISPC_AM65X:
+	case DISPC_AM625:
 		dispc_am65x_vp_write_gamma_table(dispc, hw_videoport);
 		break;
 	case DISPC_J721E:
@@ -2583,7 +2642,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->subrev == DISPC_AM65X || dispc->feat->subrev == DISPC_AM625)
 		dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n",
 			REG_GET(dispc, DSS_SYSSTATUS, 5, 5),
 			REG_GET(dispc, DSS_SYSSTATUS, 6, 6),
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index e49432f0abf5..b66418e583ee 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -61,6 +61,7 @@ enum dispc_dss_subrevision {
 	DISPC_K2G,
 	DISPC_AM65X,
 	DISPC_J721E,
+	DISPC_AM625,
 };
 
 struct dispc_features {
@@ -74,6 +75,7 @@ struct dispc_features {
 	const char *common;
 	const u16 *common_regs;
 	u32 num_vps;
+	u32 num_max_ports;
 	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 */
@@ -88,6 +90,7 @@ struct dispc_features {
 extern const struct dispc_features dispc_k2g_feats;
 extern const struct dispc_features dispc_am65x_feats;
 extern const struct dispc_features dispc_j721e_feats;
+extern const struct dispc_features dispc_am625_feats;
 
 void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask);
 dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc);
diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index 04cfff89ee51..326059e99696 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -235,6 +235,7 @@ static const struct of_device_id tidss_of_table[] = {
 	{ .compatible = "ti,k2g-dss", .data = &dispc_k2g_feats, },
 	{ .compatible = "ti,am65x-dss", .data = &dispc_am65x_feats, },
 	{ .compatible = "ti,j721e-dss", .data = &dispc_j721e_feats, },
+	{ .compatible = "ti,am625-dss", .data = &dispc_am625_feats, },
 	{ }
 };
 
-- 
2.37.0

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

[Tomi Valkeinen]

On 28/09/2022 20:52, 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 port has been added with "DISPC_VP_OLDI" bus type.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c | 61 ++++++++++++++++++++++++++++-
>   drivers/gpu/drm/tidss/tidss_dispc.h |  3 ++
>   drivers/gpu/drm/tidss/tidss_drv.c   |  1 +
>   3 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index dd3c6a606ae2..34f0da4bb3e3 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -93,6 +93,7 @@ const struct dispc_features dispc_k2g_feats = {
>   	.common_regs = tidss_k2g_common_regs,
>   
>   	.num_vps = 1,
> +	.num_max_ports = 1,

I would just use "num_ports" here, as it describes the HW (e.g. we have 
num_vps = 4 for J7, even if we'd only use one of the VPs).

I would also rename "TIDSS_MAX_PORTS". Maybe TIDSS_MAX_VPS. And check 
carefully that it's never used for the output ports.

Or maybe this field should be "num_output_ports", and try to use "output 
port" term in the driver for the..., well, output ports, to make the 
distinction between the videoport and output port more clear.

  Tomi

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

[Tomi Valkeinen]

On 28/09/2022 20:52, 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 port has been added with "DISPC_VP_OLDI" bus type.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c | 61 ++++++++++++++++++++++++++++-
>   drivers/gpu/drm/tidss/tidss_dispc.h |  3 ++
>   drivers/gpu/drm/tidss/tidss_drv.c   |  1 +
>   3 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index dd3c6a606ae2..34f0da4bb3e3 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -93,6 +93,7 @@ const struct dispc_features dispc_k2g_feats = {
>   	.common_regs = tidss_k2g_common_regs,
>   
>   	.num_vps = 1,
> +	.num_max_ports = 1,
>   	.vp_name = { "vp1" },
>   	.ovr_name = { "ovr1" },
>   	.vpclk_name =  { "vp1" },
> @@ -168,6 +169,7 @@ const struct dispc_features dispc_am65x_feats = {
>   	.common_regs = tidss_am65x_common_regs,
>   
>   	.num_vps = 2,
> +	.num_max_ports = 2,
>   	.vp_name = { "vp1", "vp2" },
>   	.ovr_name = { "ovr1", "ovr2" },
>   	.vpclk_name =  { "vp1", "vp2" },
> @@ -257,6 +259,7 @@ const struct dispc_features dispc_j721e_feats = {
>   	.common_regs = tidss_j721e_common_regs,
>   
>   	.num_vps = 4,
> +	.num_max_ports = 4,
>   	.vp_name = { "vp1", "vp2", "vp3", "vp4" },
>   	.ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" },
>   	.vpclk_name = { "vp1", "vp2", "vp3", "vp4" },
> @@ -275,6 +278,57 @@ const struct dispc_features dispc_j721e_feats = {
>   	.vid_order = { 1, 3, 0, 2 },
>   };
>   
> +const struct dispc_features dispc_am625_feats = {
> +	.max_pclk_khz = {
> +		[DISPC_VP_DPI] = 165000,
> +		[DISPC_VP_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,
> +	/* note: the 3rd port is not representative of a 3rd pipeline */
> +	.num_max_ports = 3,
> +	.vp_name = { "vp1", "vp2" },
> +	.ovr_name = { "ovr1", "ovr2" },
> +	.vpclk_name =  { "vp1", "vp2" },
> +	.vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI, DISPC_VP_OLDI },

Here, for example, we have an issue with the VP vs port. vp_bus_type is 
of size TIDSS_MAX_PORTS (VPs), but you're using it for output ports. If 
we did not have J7, which has 4 VPs, we'd have an overflow bug here.

The meaning of vp_sub_type also becomes a bit unclear. I guess it's 
rather "output_port_bus_type"?

  Tomi

[RFC PATCH v5 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 confgure 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 |  11 +++
 drivers/gpu/drm/tidss/tidss_dispc.h |   8 ++
 drivers/gpu/drm/tidss/tidss_drv.h   |   3 +
 drivers/gpu/drm/tidss/tidss_kms.c   | 146 +++++++++++++++++++++++-----
 4 files changed, 145 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 34f0da4bb3e3..88008ad39b55 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -354,6 +354,8 @@ struct dispc_device {
 
 	bool is_enabled;
 
+	enum dispc_oldi_modes oldi_mode;
+
 	struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
 
 	u32 *fourccs;
@@ -1958,6 +1960,15 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len)
 	return dispc->fourccs;
 }
 
+int dispc_configure_oldi_mode(struct dispc_device *dispc,
+			      enum dispc_oldi_modes oldi_mode)
+{
+	WARN_ON(!dispc);
+
+	dispc->oldi_mode = oldi_mode;
+	return 0;
+}
+
 static s32 pixinc(int pixels, u8 ps)
 {
 	if (pixels == 1)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index b66418e583ee..45cce1054832 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -64,6 +64,13 @@ enum dispc_dss_subrevision {
 	DISPC_AM625,
 };
 
+enum dispc_oldi_modes {
+	OLDI_MODE_OFF,				/* OLDI turned off / tied off in IP. */
+	OLDI_SINGLE_LINK_SINGLE_MODE,		/* Single Output over OLDI 0. */
+	OLDI_SINGLE_LINK_CLONE_MODE,		/* Duplicate Output over OLDI 0 and 1. */
+	OLDI_DUAL_LINK_MODE,			/* Combined Output over OLDI 0 and 1. */
+};
+
 struct dispc_features {
 	int min_pclk_khz;
 	int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
@@ -131,6 +138,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);
+int dispc_configure_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 d7f27b0b0315..2252ba0222ca 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -12,6 +12,9 @@
 #define TIDSS_MAX_PORTS 4
 #define TIDSS_MAX_PLANES 4
 
+/* For AM625-DSS with 2 OLDI TXes */
+#define TIDSS_MAX_BRIDGE_PER_PIPE	2
+
 typedef u32 dispc_irq_t;
 
 struct tidss_device {
diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
index 666e527a0acf..73afe390f36d 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -107,32 +107,84 @@ static const struct drm_mode_config_funcs mode_config_funcs = {
 	.atomic_commit = drm_atomic_helper_commit,
 };
 
+static int tidss_get_oldi_mode(struct tidss_device *tidss)
+{
+	int pixel_order;
+	struct device_node *dss_ports, *oldi0_port, *oldi1_port;
+
+	dss_ports = of_get_next_child(tidss->dev->of_node, NULL);
+	oldi0_port = of_graph_get_port_by_id(dss_ports, 0);
+	oldi1_port = of_graph_get_port_by_id(dss_ports, 2);
+
+	if (!(oldi0_port && oldi1_port))
+		return OLDI_SINGLE_LINK_SINGLE_MODE;
+
+	/*
+	 * 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.
+		 */
+		return OLDI_SINGLE_LINK_CLONE_MODE;
+
+	case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
+	case DRM_LVDS_DUAL_LINK_ODD_EVEN_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 IP and an cannot be change vis SW. These
+		 * properties have been used to merely identify if a Dual Link
+		 * configuration is required. Swapping this property in the panel
+		 * port DT nodes will not make any difference.
+		 */
+		return OLDI_DUAL_LINK_MODE;
+
+	default:
+		return OLDI_MODE_OFF;
+	}
+}
+
 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_BRIDGE_PER_PIPE];
 		u32 enc_type;
+		u32 num_bridges;
 	};
 
 	const struct dispc_features *feat = tidss->feat;
-	u32 max_vps = feat->num_vps;
+	u32 max_ports = feat->num_max_ports;
 	u32 max_planes = feat->num_planes;
 
 	struct pipe pipes[TIDSS_MAX_PORTS];
 	u32 num_pipes = 0;
+	u32 pipe_number = 0;
 	u32 crtc_mask;
+	u32 num_oldi = 0;
+	u32 oldi0_port = 0;
+	u32 hw_vp = 0;
+	enum dispc_oldi_modes oldi_mode;
 
 	/* first find all the connected panels & bridges */
 
-	for (i = 0; i < max_vps; i++) {
+	for (i = 0; i < max_ports; i++) {
 		struct drm_panel *panel;
 		struct drm_bridge *bridge;
+		bool bridge_req = true;
 		u32 enc_type = DRM_MODE_ENCODER_NONE;
 		int ret;
 
@@ -146,6 +198,11 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 			return ret;
 		}
 
+		/* default number of bridges required for a panel/bridge*/
+		pipe_number = num_pipes;
+		pipes[pipe_number].num_bridges = 1;
+		hw_vp = i;
+
 		if (panel) {
 			u32 conn_type;
 
@@ -155,7 +212,43 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 			case DISPC_VP_OLDI:
 				enc_type = DRM_MODE_ENCODER_LVDS;
 				conn_type = DRM_MODE_CONNECTOR_LVDS;
+
+				/*
+				 * A single DSS controller cannot support 2
+				 * independent displays. If 2nd node is detected,
+				 * it is for Dual Link Mode or Clone Mode.
+				 *
+				 * A new pipe instance is not required.
+				 */
+				if (++num_oldi == 2) {
+					pipe_number = oldi0_port;
+					hw_vp = i;
+
+					/* 2nd OLDI DT node detected. Get its mode */
+					oldi_mode = tidss_get_oldi_mode(tidss);
+					bridge_req = false;
+
+					/*
+					 * A separate panel bridge will only be
+					 * required if 2 panels are connected for
+					 * the OLDI Clone Mode.
+					 */
+					if (oldi_mode == OLDI_SINGLE_LINK_CLONE_MODE) {
+						bridge_req = true;
+						(pipes[pipe_number].num_bridges)++;
+					}
+				} else {
+					/*
+					 * First OLDI DT node detected. Save it
+					 * in case there is another node for Dual
+					 * Link Mode or Clone Mode.
+					 */
+					oldi0_port = i;
+					oldi_mode = OLDI_SINGLE_LINK_SINGLE_MODE;
+				}
+				dispc_configure_oldi_mode(tidss->dispc, oldi_mode);
 				break;
+
 			case DISPC_VP_DPI:
 				enc_type = DRM_MODE_ENCODER_DPI;
 				conn_type = DRM_MODE_CONNECTOR_DPI;
@@ -173,19 +266,23 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 				return -EINVAL;
 			}
 
-			bridge = devm_drm_panel_bridge_add(dev, panel);
-			if (IS_ERR(bridge)) {
-				dev_err(dev,
-					"failed to set up panel bridge for port %d\n",
-					i);
-				return PTR_ERR(bridge);
+			if (bridge_req) {
+				bridge = devm_drm_panel_bridge_add(dev, panel);
+				if (IS_ERR(bridge)) {
+					dev_err(dev,
+						"failed to set up panel bridge for port %d\n",
+						i);
+					return PTR_ERR(bridge);
+				}
 			}
 		}
 
-		pipes[num_pipes].hw_videoport = i;
-		pipes[num_pipes].bridge = bridge;
-		pipes[num_pipes].enc_type = enc_type;
-		num_pipes++;
+		if (bridge_req) {
+			pipes[pipe_number].hw_videoport = hw_vp;
+			pipes[pipe_number].bridge[pipes[pipe_number].num_bridges - 1] = bridge;
+			pipes[pipe_number].enc_type = enc_type;
+			num_pipes++;
+		}
 	}
 
 	/* all planes can be on any crtc */
@@ -200,6 +297,7 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 		u32 hw_plane_id = feat->vid_order[tidss->num_planes];
 		int ret;
 
+		/* Creating planes and CRTCs only for real pipes */
 		tplane = tidss_plane_create(tidss, hw_plane_id,
 					    DRM_PLANE_TYPE_PRIMARY, crtc_mask,
 					    fourccs, fourccs_len);
@@ -219,16 +317,18 @@ 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);
-		}
+		for (j = 0; j < pipes[i].num_bridges; j++) {
+			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);
+			}
 
-		ret = drm_bridge_attach(enc, pipes[i].bridge, NULL, 0);
-		if (ret)
-			return ret;
+			ret = drm_bridge_attach(enc, pipes[i].bridge[j], NULL, 0);
+			if (ret)
+				return ret;
+		}
 	}
 
 	/* create overlay planes of the leftover planes */
-- 
2.37.0

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

[Tomi Valkeinen]

On 28/09/2022 20:52, 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 |
> |      |        |         |      |       |
> +------+        +---------+      +-------+

Can you have single link on OLDI 1 (OLDI 0 off)? I don't know if that 
make sense on this platform, but if the pins for OLDI 0 and 1 are 
different, there might be a reason on some cases for that.

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

I think you've got a tab in the line above, but otherwise use spaces.

>             |    +---------+      +-------+
>             |    |         |      |       |
>             +--->| 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 confgure the OLDI modes using of and lvds DRM helper

"configuring"

> functions.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c |  11 +++
>   drivers/gpu/drm/tidss/tidss_dispc.h |   8 ++
>   drivers/gpu/drm/tidss/tidss_drv.h   |   3 +
>   drivers/gpu/drm/tidss/tidss_kms.c   | 146 +++++++++++++++++++++++-----
>   4 files changed, 145 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 34f0da4bb3e3..88008ad39b55 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -354,6 +354,8 @@ struct dispc_device {
>   
>   	bool is_enabled;
>   
> +	enum dispc_oldi_modes oldi_mode;
> +
>   	struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
>   
>   	u32 *fourccs;
> @@ -1958,6 +1960,15 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len)
>   	return dispc->fourccs;
>   }
>   
> +int dispc_configure_oldi_mode(struct dispc_device *dispc,
> +			      enum dispc_oldi_modes oldi_mode)
> +{
> +	WARN_ON(!dispc);
> +
> +	dispc->oldi_mode = oldi_mode;
> +	return 0;
> +}

I think "configure" means more than just storing the value. Maybe 
dispc_set_oldi_mode(). And an empty line above the return.

> +
>   static s32 pixinc(int pixels, u8 ps)
>   {
>   	if (pixels == 1)
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
> index b66418e583ee..45cce1054832 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -64,6 +64,13 @@ enum dispc_dss_subrevision {
>   	DISPC_AM625,
>   };
>   
> +enum dispc_oldi_modes {
> +	OLDI_MODE_OFF,				/* OLDI turned off / tied off in IP. */
> +	OLDI_SINGLE_LINK_SINGLE_MODE,		/* Single Output over OLDI 0. */
> +	OLDI_SINGLE_LINK_CLONE_MODE,		/* Duplicate Output over OLDI 0 and 1. */
> +	OLDI_DUAL_LINK_MODE,			/* Combined Output over OLDI 0 and 1. */
> +};
> +
>   struct dispc_features {
>   	int min_pclk_khz;
>   	int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
> @@ -131,6 +138,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);
> +int dispc_configure_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 d7f27b0b0315..2252ba0222ca 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.h
> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
> @@ -12,6 +12,9 @@
>   #define TIDSS_MAX_PORTS 4
>   #define TIDSS_MAX_PLANES 4
>   
> +/* For AM625-DSS with 2 OLDI TXes */
> +#define TIDSS_MAX_BRIDGE_PER_PIPE	2

"BRIDGES"?

> +
>   typedef u32 dispc_irq_t;
>   
>   struct tidss_device {
> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index 666e527a0acf..73afe390f36d 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> @@ -107,32 +107,84 @@ static const struct drm_mode_config_funcs mode_config_funcs = {
>   	.atomic_commit = drm_atomic_helper_commit,
>   };
>   
> +static int tidss_get_oldi_mode(struct tidss_device *tidss)

Return enum dispc_oldi_modes, not int.

> +{
> +	int pixel_order;
> +	struct device_node *dss_ports, *oldi0_port, *oldi1_port;
> +
> +	dss_ports = of_get_next_child(tidss->dev->of_node, NULL);

Hmm you get the next child and hope that it's the ports node?

In any case, I think you can call of_graph_get_port_by_id() with the 
tidss->dev->of_node and it'll do the right thing.

> +	oldi0_port = of_graph_get_port_by_id(dss_ports, 0);
> +	oldi1_port = of_graph_get_port_by_id(dss_ports, 2);

I think you need to of_put these at some point.

> +	if (!(oldi0_port && oldi1_port))
> +		return OLDI_SINGLE_LINK_SINGLE_MODE;

This one matches also for !oldi0 && oldi1. If oldi1 cannot be used in 
single-link mode, the above should take it into account.

> +
> +	/*
> +	 * 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.
> +		 */
> +		return OLDI_SINGLE_LINK_CLONE_MODE;
> +
> +	case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
> +	case DRM_LVDS_DUAL_LINK_ODD_EVEN_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 IP and an cannot be change vis SW. These
> +		 * properties have been used to merely identify if a Dual Link
> +		 * configuration is required. Swapping this property in the panel
> +		 * port DT nodes will not make any difference.
> +		 */

But if they are in the wrong order, shouldn't we fail or at least give a 
warning?

> +		return OLDI_DUAL_LINK_MODE;
> +
> +	default:
> +		return OLDI_MODE_OFF;
> +	}
> +}
> +
>   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_BRIDGE_PER_PIPE];
>   		u32 enc_type;
> +		u32 num_bridges;
>   	};
>   
>   	const struct dispc_features *feat = tidss->feat;
> -	u32 max_vps = feat->num_vps;
> +	u32 max_ports = feat->num_max_ports;
>   	u32 max_planes = feat->num_planes;
>   
>   	struct pipe pipes[TIDSS_MAX_PORTS];
>   	u32 num_pipes = 0;
> +	u32 pipe_number = 0;
>   	u32 crtc_mask;
> +	u32 num_oldi = 0;
> +	u32 oldi0_port = 0;
> +	u32 hw_vp = 0;
> +	enum dispc_oldi_modes oldi_mode;
>   
>   	/* first find all the connected panels & bridges */
>   
> -	for (i = 0; i < max_vps; i++) {
> +	for (i = 0; i < max_ports; i++) {
>   		struct drm_panel *panel;
>   		struct drm_bridge *bridge;
> +		bool bridge_req = true;
>   		u32 enc_type = DRM_MODE_ENCODER_NONE;
>   		int ret;
>   
> @@ -146,6 +198,11 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>   			return ret;
>   		}
>   
> +		/* default number of bridges required for a panel/bridge*/
> +		pipe_number = num_pipes;
> +		pipes[pipe_number].num_bridges = 1;
> +		hw_vp = i;
> +
>   		if (panel) {
>   			u32 conn_type;
>   
> @@ -155,7 +212,43 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>   			case DISPC_VP_OLDI:
>   				enc_type = DRM_MODE_ENCODER_LVDS;
>   				conn_type = DRM_MODE_CONNECTOR_LVDS;
> +
> +				/*
> +				 * A single DSS controller cannot support 2
> +				 * independent displays. If 2nd node is detected,
> +				 * it is for Dual Link Mode or Clone Mode.
> +				 *
> +				 * A new pipe instance is not required.
> +				 */
> +				if (++num_oldi == 2) {
> +					pipe_number = oldi0_port;
> +					hw_vp = i;
> +
> +					/* 2nd OLDI DT node detected. Get its mode */
> +					oldi_mode = tidss_get_oldi_mode(tidss);
> +					bridge_req = false;
> +
> +					/*
> +					 * A separate panel bridge will only be
> +					 * required if 2 panels are connected for
> +					 * the OLDI Clone Mode.
> +					 */
> +					if (oldi_mode == OLDI_SINGLE_LINK_CLONE_MODE) {
> +						bridge_req = true;
> +						(pipes[pipe_number].num_bridges)++;
> +					}
> +				} else {
> +					/*
> +					 * First OLDI DT node detected. Save it
> +					 * in case there is another node for Dual
> +					 * Link Mode or Clone Mode.
> +					 */
> +					oldi0_port = i;
> +					oldi_mode = OLDI_SINGLE_LINK_SINGLE_MODE;
> +				}
> +				dispc_configure_oldi_mode(tidss->dispc, oldi_mode);
>   				break;
> +
>   			case DISPC_VP_DPI:
>   				enc_type = DRM_MODE_ENCODER_DPI;
>   				conn_type = DRM_MODE_CONNECTOR_DPI;
> @@ -173,19 +266,23 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>   				return -EINVAL;
>   			}
>   
> -			bridge = devm_drm_panel_bridge_add(dev, panel);
> -			if (IS_ERR(bridge)) {
> -				dev_err(dev,
> -					"failed to set up panel bridge for port %d\n",
> -					i);
> -				return PTR_ERR(bridge);
> +			if (bridge_req) {
> +				bridge = devm_drm_panel_bridge_add(dev, panel);
> +				if (IS_ERR(bridge)) {
> +					dev_err(dev,
> +						"failed to set up panel bridge for port %d\n",
> +						i);
> +					return PTR_ERR(bridge);
> +				}
>   			}
>   		}
>   
> -		pipes[num_pipes].hw_videoport = i;
> -		pipes[num_pipes].bridge = bridge;
> -		pipes[num_pipes].enc_type = enc_type;
> -		num_pipes++;
> +		if (bridge_req) {
> +			pipes[pipe_number].hw_videoport = hw_vp;
> +			pipes[pipe_number].bridge[pipes[pipe_number].num_bridges - 1] = bridge;
> +			pipes[pipe_number].enc_type = enc_type;
> +			num_pipes++;
> +		}

I need to look at this with better time. But I started to wonder, would 
it be clearer to first figure out the oldi setup before the loop, rather 
than figuring it out inside the loop. I'm not sure if it would help 
much, though.

  Tomi

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

[Aradhya Bhatia]

Hi Tomi

Thank you for the comprehensive feedback across all the patches. I am
working on them.

I do have some concerns which I have talked about, below.

On 12-Oct-22 17:53, Tomi Valkeinen wrote:
> On 28/09/2022 20:52, 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 |
>> |      |        |         |      |       |
>> +------+        +---------+      +-------+
> 
> Can you have single link on OLDI 1 (OLDI 0 off)? I don't know if that 
> make sense on this platform, but if the pins for OLDI 0 and 1 are 
> different, there might be a reason on some cases for that.

HW does not support a case where single link is enabled over OLDI 1 with
OLDI 0 off, even though the pins are different.

One could still put 2 panel nodes in DT to set OLDI in a Clone Mode and
simply not use OLDI 0 pins, but I dont think that is a valid case that
should be supported.

> 
>> 2. OLDI_SINGLE_LINK_CLONE_MODE
>> Duplicate Output over OLDI 0 and 1.
>> +------+        +---------+      +-------+
>> |      |        |         |      |       |
>> | CRTC +---+--->| ENCODER +----->| PANEL |
>> |      |   |    |         |      |       |
>> +------+   |    +---------+      +-------+
>>        |
> 
> I think you've got a tab in the line above, but otherwise use spaces.
> 
>>             |    +---------+      +-------+
>>             |    |         |      |       |
>>             +--->| 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 confgure the OLDI modes using of and lvds DRM helper
> 
> "configuring"
> 
>> functions.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c |  11 +++
>>   drivers/gpu/drm/tidss/tidss_dispc.h |   8 ++
>>   drivers/gpu/drm/tidss/tidss_drv.h   |   3 +
>>   drivers/gpu/drm/tidss/tidss_kms.c   | 146 +++++++++++++++++++++++-----
>>   4 files changed, 145 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index 34f0da4bb3e3..88008ad39b55 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -354,6 +354,8 @@ struct dispc_device {
>>       bool is_enabled;
>> +    enum dispc_oldi_modes oldi_mode;
>> +
>>       struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
>>       u32 *fourccs;
>> @@ -1958,6 +1960,15 @@ const u32 *dispc_plane_formats(struct 
>> dispc_device *dispc, unsigned int *len)
>>       return dispc->fourccs;
>>   }
>> +int dispc_configure_oldi_mode(struct dispc_device *dispc,
>> +                  enum dispc_oldi_modes oldi_mode)
>> +{
>> +    WARN_ON(!dispc);
>> +
>> +    dispc->oldi_mode = oldi_mode;
>> +    return 0;
>> +}
> 
> I think "configure" means more than just storing the value. Maybe 
> dispc_set_oldi_mode(). And an empty line above the return.
> 
>> +
>>   static s32 pixinc(int pixels, u8 ps)
>>   {
>>       if (pixels == 1)
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h 
>> b/drivers/gpu/drm/tidss/tidss_dispc.h
>> index b66418e583ee..45cce1054832 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>> @@ -64,6 +64,13 @@ enum dispc_dss_subrevision {
>>       DISPC_AM625,
>>   };
>> +enum dispc_oldi_modes {
>> +    OLDI_MODE_OFF,                /* OLDI turned off / tied off in 
>> IP. */
>> +    OLDI_SINGLE_LINK_SINGLE_MODE,        /* Single Output over OLDI 
>> 0. */
>> +    OLDI_SINGLE_LINK_CLONE_MODE,        /* Duplicate Output over OLDI 
>> 0 and 1. */
>> +    OLDI_DUAL_LINK_MODE,            /* Combined Output over OLDI 0 
>> and 1. */
>> +};
>> +
>>   struct dispc_features {
>>       int min_pclk_khz;
>>       int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
>> @@ -131,6 +138,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);
>> +int dispc_configure_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 d7f27b0b0315..2252ba0222ca 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.h
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
>> @@ -12,6 +12,9 @@
>>   #define TIDSS_MAX_PORTS 4
>>   #define TIDSS_MAX_PLANES 4
>> +/* For AM625-DSS with 2 OLDI TXes */
>> +#define TIDSS_MAX_BRIDGE_PER_PIPE    2
> 
> "BRIDGES"?
> 
>> +
>>   typedef u32 dispc_irq_t;
>>   struct tidss_device {
>> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c 
>> b/drivers/gpu/drm/tidss/tidss_kms.c
>> index 666e527a0acf..73afe390f36d 100644
>> --- a/drivers/gpu/drm/tidss/tidss_kms.c
>> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
>> @@ -107,32 +107,84 @@ static const struct drm_mode_config_funcs 
>> mode_config_funcs = {
>>       .atomic_commit = drm_atomic_helper_commit,
>>   };
>> +static int tidss_get_oldi_mode(struct tidss_device *tidss)
> 
> Return enum dispc_oldi_modes, not int.
> 
>> +{
>> +    int pixel_order;
>> +    struct device_node *dss_ports, *oldi0_port, *oldi1_port;
>> +
>> +    dss_ports = of_get_next_child(tidss->dev->of_node, NULL);
> 
> Hmm you get the next child and hope that it's the ports node?
> 
> In any case, I think you can call of_graph_get_port_by_id() with the 
> tidss->dev->of_node and it'll do the right thing.
I think this will only work if the child of dss node is just "ports",
but we've been using "dss_ports" as the child.

However, you are right. I shouldn't expect the first child to be
dss_ports. I will use the "of_get_child_by_name" helper to get the
dss_ports node.

> 
>> +    oldi0_port = of_graph_get_port_by_id(dss_ports, 0);
>> +    oldi1_port = of_graph_get_port_by_id(dss_ports, 2);
> 
> I think you need to of_put these at some point.
> 
>> +    if (!(oldi0_port && oldi1_port))
>> +        return OLDI_SINGLE_LINK_SINGLE_MODE;
> 
> This one matches also for !oldi0 && oldi1. If oldi1 cannot be used in 
> single-link mode, the above should take it into account.

Right. I will print a warning if somebody's trying to use (!oldi0 &&
oldi1) but since its a single link requirement, I will still set the
OLDI for single link single mode.

> 
>> +
>> +    /*
>> +     * 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.
>> +         */
>> +        return OLDI_SINGLE_LINK_CLONE_MODE;
>> +
>> +    case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
>> +    case DRM_LVDS_DUAL_LINK_ODD_EVEN_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 IP and an cannot be change vis SW. These
>> +         * properties have been used to merely identify if a Dual Link
>> +         * configuration is required. Swapping this property in the 
>> panel
>> +         * port DT nodes will not make any difference.
>> +         */
> 
> But if they are in the wrong order, shouldn't we fail or at least give a 
> warning?
>  >> +        return OLDI_DUAL_LINK_MODE;
>> +
>> +    default:
>> +        return OLDI_MODE_OFF;
>> +    }
>> +}
>> +
>>   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_BRIDGE_PER_PIPE];
>>           u32 enc_type;
>> +        u32 num_bridges;
>>       };
>>       const struct dispc_features *feat = tidss->feat;
>> -    u32 max_vps = feat->num_vps;
>> +    u32 max_ports = feat->num_max_ports;
>>       u32 max_planes = feat->num_planes;
>>       struct pipe pipes[TIDSS_MAX_PORTS];
>>       u32 num_pipes = 0;
>> +    u32 pipe_number = 0;
>>       u32 crtc_mask;
>> +    u32 num_oldi = 0;
>> +    u32 oldi0_port = 0;
>> +    u32 hw_vp = 0;
>> +    enum dispc_oldi_modes oldi_mode;
>>       /* first find all the connected panels & bridges */
>> -    for (i = 0; i < max_vps; i++) {
>> +    for (i = 0; i < max_ports; i++) {
>>           struct drm_panel *panel;
>>           struct drm_bridge *bridge;
>> +        bool bridge_req = true;
>>           u32 enc_type = DRM_MODE_ENCODER_NONE;
>>           int ret;
>> @@ -146,6 +198,11 @@ static int tidss_dispc_modeset_init(struct 
>> tidss_device *tidss)
>>               return ret;
>>           }
>> +        /* default number of bridges required for a panel/bridge*/
>> +        pipe_number = num_pipes;
>> +        pipes[pipe_number].num_bridges = 1;
>> +        hw_vp = i;
>> +
>>           if (panel) {
>>               u32 conn_type;
>> @@ -155,7 +212,43 @@ static int tidss_dispc_modeset_init(struct 
>> tidss_device *tidss)
>>               case DISPC_VP_OLDI:
>>                   enc_type = DRM_MODE_ENCODER_LVDS;
>>                   conn_type = DRM_MODE_CONNECTOR_LVDS;
>> +
>> +                /*
>> +                 * A single DSS controller cannot support 2
>> +                 * independent displays. If 2nd node is detected,
>> +                 * it is for Dual Link Mode or Clone Mode.
>> +                 *
>> +                 * A new pipe instance is not required.
>> +                 */
>> +                if (++num_oldi == 2) {
>> +                    pipe_number = oldi0_port;
>> +                    hw_vp = i;
>> +
>> +                    /* 2nd OLDI DT node detected. Get its mode */
>> +                    oldi_mode = tidss_get_oldi_mode(tidss);
>> +                    bridge_req = false;
>> +
>> +                    /*
>> +                     * A separate panel bridge will only be
>> +                     * required if 2 panels are connected for
>> +                     * the OLDI Clone Mode.
>> +                     */
>> +                    if (oldi_mode == OLDI_SINGLE_LINK_CLONE_MODE) {
>> +                        bridge_req = true;
>> +                        (pipes[pipe_number].num_bridges)++;
>> +                    }
>> +                } else {
>> +                    /*
>> +                     * First OLDI DT node detected. Save it
>> +                     * in case there is another node for Dual
>> +                     * Link Mode or Clone Mode.
>> +                     */
>> +                    oldi0_port = i;
>> +                    oldi_mode = OLDI_SINGLE_LINK_SINGLE_MODE;
>> +                }
>> +                dispc_configure_oldi_mode(tidss->dispc, oldi_mode);
>>                   break;
>> +
>>               case DISPC_VP_DPI:
>>                   enc_type = DRM_MODE_ENCODER_DPI;
>>                   conn_type = DRM_MODE_CONNECTOR_DPI;
>> @@ -173,19 +266,23 @@ static int tidss_dispc_modeset_init(struct 
>> tidss_device *tidss)
>>                   return -EINVAL;
>>               }
>> -            bridge = devm_drm_panel_bridge_add(dev, panel);
>> -            if (IS_ERR(bridge)) {
>> -                dev_err(dev,
>> -                    "failed to set up panel bridge for port %d\n",
>> -                    i);
>> -                return PTR_ERR(bridge);
>> +            if (bridge_req) {
>> +                bridge = devm_drm_panel_bridge_add(dev, panel);
>> +                if (IS_ERR(bridge)) {
>> +                    dev_err(dev,
>> +                        "failed to set up panel bridge for port %d\n",
>> +                        i);
>> +                    return PTR_ERR(bridge);
>> +                }
>>               }
>>           }
>> -        pipes[num_pipes].hw_videoport = i;
>> -        pipes[num_pipes].bridge = bridge;
>> -        pipes[num_pipes].enc_type = enc_type;
>> -        num_pipes++;
>> +        if (bridge_req) {
>> +            pipes[pipe_number].hw_videoport = hw_vp;
>> +            pipes[pipe_number].bridge[pipes[pipe_number].num_bridges 
>> - 1] = bridge;
>> +            pipes[pipe_number].enc_type = enc_type;
>> +            num_pipes++;
>> +        }
> 
> I need to look at this with better time. But I started to wonder, would 
> it be clearer to first figure out the oldi setup before the loop, rather 
> than figuring it out inside the loop. I'm not sure if it would help 
> much, though.
> 
I had not thought about taking this approach, but it might actually be
better.

These patches, at the moment, do not support a case where a clone mode
or dual link mode is used on a bridge instead of a panel. My edits
inside the loop are panel dependent. If we do have oldi setup
information prior to the beginning of the loop, the panel dependency can
be removed and some commond code can be written to support an additional
encoder - bridge connection should it be required.

Let me know what you think!

If this apparch is better indeed, I will make these changes before
sending out the next revision.


Regards
Aradhya

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

[Tomi Valkeinen]

On 18/10/2022 10:00, Aradhya Bhatia wrote:
> Hi Tomi
> 
> Thank you for the comprehensive feedback across all the patches. I am
> working on them.
> 
> I do have some concerns which I have talked about, below.
> 
> On 12-Oct-22 17:53, Tomi Valkeinen wrote:
>> On 28/09/2022 20:52, 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 |
>>> |      |        |         |      |       |
>>> +------+        +---------+      +-------+
>>
>> Can you have single link on OLDI 1 (OLDI 0 off)? I don't know if that 
>> make sense on this platform, but if the pins for OLDI 0 and 1 are 
>> different, there might be a reason on some cases for that.
> 
> HW does not support a case where single link is enabled over OLDI 1 with
> OLDI 0 off, even though the pins are different.
> 
> One could still put 2 panel nodes in DT to set OLDI in a Clone Mode and
> simply not use OLDI 0 pins, but I dont think that is a valid case that
> should be supported.
> 
>>
>>> 2. OLDI_SINGLE_LINK_CLONE_MODE
>>> Duplicate Output over OLDI 0 and 1.
>>> +------+        +---------+      +-------+
>>> |      |        |         |      |       |
>>> | CRTC +---+--->| ENCODER +----->| PANEL |
>>> |      |   |    |         |      |       |
>>> +------+   |    +---------+      +-------+
>>>        |
>>
>> I think you've got a tab in the line above, but otherwise use spaces.
>>
>>>             |    +---------+      +-------+
>>>             |    |         |      |       |
>>>             +--->| 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 confgure the OLDI modes using of and lvds DRM helper
>>
>> "configuring"
>>
>>> functions.
>>>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> ---
>>>   drivers/gpu/drm/tidss/tidss_dispc.c |  11 +++
>>>   drivers/gpu/drm/tidss/tidss_dispc.h |   8 ++
>>>   drivers/gpu/drm/tidss/tidss_drv.h   |   3 +
>>>   drivers/gpu/drm/tidss/tidss_kms.c   | 146 +++++++++++++++++++++++-----
>>>   4 files changed, 145 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> index 34f0da4bb3e3..88008ad39b55 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> @@ -354,6 +354,8 @@ struct dispc_device {
>>>       bool is_enabled;
>>> +    enum dispc_oldi_modes oldi_mode;
>>> +
>>>       struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
>>>       u32 *fourccs;
>>> @@ -1958,6 +1960,15 @@ const u32 *dispc_plane_formats(struct 
>>> dispc_device *dispc, unsigned int *len)
>>>       return dispc->fourccs;
>>>   }
>>> +int dispc_configure_oldi_mode(struct dispc_device *dispc,
>>> +                  enum dispc_oldi_modes oldi_mode)
>>> +{
>>> +    WARN_ON(!dispc);
>>> +
>>> +    dispc->oldi_mode = oldi_mode;
>>> +    return 0;
>>> +}
>>
>> I think "configure" means more than just storing the value. Maybe 
>> dispc_set_oldi_mode(). And an empty line above the return.
>>
>>> +
>>>   static s32 pixinc(int pixels, u8 ps)
>>>   {
>>>       if (pixels == 1)
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h 
>>> b/drivers/gpu/drm/tidss/tidss_dispc.h
>>> index b66418e583ee..45cce1054832 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>>> @@ -64,6 +64,13 @@ enum dispc_dss_subrevision {
>>>       DISPC_AM625,
>>>   };
>>> +enum dispc_oldi_modes {
>>> +    OLDI_MODE_OFF,                /* OLDI turned off / tied off in 
>>> IP. */
>>> +    OLDI_SINGLE_LINK_SINGLE_MODE,        /* Single Output over OLDI 
>>> 0. */
>>> +    OLDI_SINGLE_LINK_CLONE_MODE,        /* Duplicate Output over 
>>> OLDI 0 and 1. */
>>> +    OLDI_DUAL_LINK_MODE,            /* Combined Output over OLDI 0 
>>> and 1. */
>>> +};
>>> +
>>>   struct dispc_features {
>>>       int min_pclk_khz;
>>>       int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
>>> @@ -131,6 +138,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);
>>> +int dispc_configure_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 d7f27b0b0315..2252ba0222ca 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_drv.h
>>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
>>> @@ -12,6 +12,9 @@
>>>   #define TIDSS_MAX_PORTS 4
>>>   #define TIDSS_MAX_PLANES 4
>>> +/* For AM625-DSS with 2 OLDI TXes */
>>> +#define TIDSS_MAX_BRIDGE_PER_PIPE    2
>>
>> "BRIDGES"?
>>
>>> +
>>>   typedef u32 dispc_irq_t;
>>>   struct tidss_device {
>>> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c 
>>> b/drivers/gpu/drm/tidss/tidss_kms.c
>>> index 666e527a0acf..73afe390f36d 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_kms.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
>>> @@ -107,32 +107,84 @@ static const struct drm_mode_config_funcs 
>>> mode_config_funcs = {
>>>       .atomic_commit = drm_atomic_helper_commit,
>>>   };
>>> +static int tidss_get_oldi_mode(struct tidss_device *tidss)
>>
>> Return enum dispc_oldi_modes, not int.
>>
>>> +{
>>> +    int pixel_order;
>>> +    struct device_node *dss_ports, *oldi0_port, *oldi1_port;
>>> +
>>> +    dss_ports = of_get_next_child(tidss->dev->of_node, NULL);
>>
>> Hmm you get the next child and hope that it's the ports node?
>>
>> In any case, I think you can call of_graph_get_port_by_id() with the 
>> tidss->dev->of_node and it'll do the right thing.
> I think this will only work if the child of dss node is just "ports",
> but we've been using "dss_ports" as the child.
> 
> However, you are right. I shouldn't expect the first child to be
> dss_ports. I will use the "of_get_child_by_name" helper to get the
> dss_ports node.

I don't think you need to. of_graph_get_port_by_id() should work fine 
with the dss node.

>>> +    oldi0_port = of_graph_get_port_by_id(dss_ports, 0);
>>> +    oldi1_port = of_graph_get_port_by_id(dss_ports, 2);
>>
>> I think you need to of_put these at some point.
>>
>>> +    if (!(oldi0_port && oldi1_port))
>>> +        return OLDI_SINGLE_LINK_SINGLE_MODE;
>>
>> This one matches also for !oldi0 && oldi1. If oldi1 cannot be used in 
>> single-link mode, the above should take it into account.
> 
> Right. I will print a warning if somebody's trying to use (!oldi0 &&
> oldi1) but since its a single link requirement, I will still set the
> OLDI for single link single mode.
> 
>>
>>> +
>>> +    /*
>>> +     * 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.
>>> +         */
>>> +        return OLDI_SINGLE_LINK_CLONE_MODE;
>>> +
>>> +    case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
>>> +    case DRM_LVDS_DUAL_LINK_ODD_EVEN_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 IP and an cannot be change vis SW. 
>>> These
>>> +         * properties have been used to merely identify if a Dual Link
>>> +         * configuration is required. Swapping this property in the 
>>> panel
>>> +         * port DT nodes will not make any difference.
>>> +         */
>>
>> But if they are in the wrong order, shouldn't we fail or at least give 
>> a warning?
>>  >> +        return OLDI_DUAL_LINK_MODE;
>>> +
>>> +    default:
>>> +        return OLDI_MODE_OFF;
>>> +    }
>>> +}
>>> +
>>>   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_BRIDGE_PER_PIPE];
>>>           u32 enc_type;
>>> +        u32 num_bridges;
>>>       };
>>>       const struct dispc_features *feat = tidss->feat;
>>> -    u32 max_vps = feat->num_vps;
>>> +    u32 max_ports = feat->num_max_ports;
>>>       u32 max_planes = feat->num_planes;
>>>       struct pipe pipes[TIDSS_MAX_PORTS];
>>>       u32 num_pipes = 0;
>>> +    u32 pipe_number = 0;
>>>       u32 crtc_mask;
>>> +    u32 num_oldi = 0;
>>> +    u32 oldi0_port = 0;
>>> +    u32 hw_vp = 0;
>>> +    enum dispc_oldi_modes oldi_mode;
>>>       /* first find all the connected panels & bridges */
>>> -    for (i = 0; i < max_vps; i++) {
>>> +    for (i = 0; i < max_ports; i++) {
>>>           struct drm_panel *panel;
>>>           struct drm_bridge *bridge;
>>> +        bool bridge_req = true;
>>>           u32 enc_type = DRM_MODE_ENCODER_NONE;
>>>           int ret;
>>> @@ -146,6 +198,11 @@ static int tidss_dispc_modeset_init(struct 
>>> tidss_device *tidss)
>>>               return ret;
>>>           }
>>> +        /* default number of bridges required for a panel/bridge*/
>>> +        pipe_number = num_pipes;
>>> +        pipes[pipe_number].num_bridges = 1;
>>> +        hw_vp = i;
>>> +
>>>           if (panel) {
>>>               u32 conn_type;
>>> @@ -155,7 +212,43 @@ static int tidss_dispc_modeset_init(struct 
>>> tidss_device *tidss)
>>>               case DISPC_VP_OLDI:
>>>                   enc_type = DRM_MODE_ENCODER_LVDS;
>>>                   conn_type = DRM_MODE_CONNECTOR_LVDS;
>>> +
>>> +                /*
>>> +                 * A single DSS controller cannot support 2
>>> +                 * independent displays. If 2nd node is detected,
>>> +                 * it is for Dual Link Mode or Clone Mode.
>>> +                 *
>>> +                 * A new pipe instance is not required.
>>> +                 */
>>> +                if (++num_oldi == 2) {
>>> +                    pipe_number = oldi0_port;
>>> +                    hw_vp = i;
>>> +
>>> +                    /* 2nd OLDI DT node detected. Get its mode */
>>> +                    oldi_mode = tidss_get_oldi_mode(tidss);
>>> +                    bridge_req = false;
>>> +
>>> +                    /*
>>> +                     * A separate panel bridge will only be
>>> +                     * required if 2 panels are connected for
>>> +                     * the OLDI Clone Mode.
>>> +                     */
>>> +                    if (oldi_mode == OLDI_SINGLE_LINK_CLONE_MODE) {
>>> +                        bridge_req = true;
>>> +                        (pipes[pipe_number].num_bridges)++;
>>> +                    }
>>> +                } else {
>>> +                    /*
>>> +                     * First OLDI DT node detected. Save it
>>> +                     * in case there is another node for Dual
>>> +                     * Link Mode or Clone Mode.
>>> +                     */
>>> +                    oldi0_port = i;
>>> +                    oldi_mode = OLDI_SINGLE_LINK_SINGLE_MODE;
>>> +                }
>>> +                dispc_configure_oldi_mode(tidss->dispc, oldi_mode);
>>>                   break;
>>> +
>>>               case DISPC_VP_DPI:
>>>                   enc_type = DRM_MODE_ENCODER_DPI;
>>>                   conn_type = DRM_MODE_CONNECTOR_DPI;
>>> @@ -173,19 +266,23 @@ static int tidss_dispc_modeset_init(struct 
>>> tidss_device *tidss)
>>>                   return -EINVAL;
>>>               }
>>> -            bridge = devm_drm_panel_bridge_add(dev, panel);
>>> -            if (IS_ERR(bridge)) {
>>> -                dev_err(dev,
>>> -                    "failed to set up panel bridge for port %d\n",
>>> -                    i);
>>> -                return PTR_ERR(bridge);
>>> +            if (bridge_req) {
>>> +                bridge = devm_drm_panel_bridge_add(dev, panel);
>>> +                if (IS_ERR(bridge)) {
>>> +                    dev_err(dev,
>>> +                        "failed to set up panel bridge for port %d\n",
>>> +                        i);
>>> +                    return PTR_ERR(bridge);
>>> +                }
>>>               }
>>>           }
>>> -        pipes[num_pipes].hw_videoport = i;
>>> -        pipes[num_pipes].bridge = bridge;
>>> -        pipes[num_pipes].enc_type = enc_type;
>>> -        num_pipes++;
>>> +        if (bridge_req) {
>>> +            pipes[pipe_number].hw_videoport = hw_vp;
>>> +            pipes[pipe_number].bridge[pipes[pipe_number].num_bridges 
>>> - 1] = bridge;
>>> +            pipes[pipe_number].enc_type = enc_type;
>>> +            num_pipes++;
>>> +        }
>>
>> I need to look at this with better time. But I started to wonder, 
>> would it be clearer to first figure out the oldi setup before the 
>> loop, rather than figuring it out inside the loop. I'm not sure if it 
>> would help much, though.
>>
> I had not thought about taking this approach, but it might actually be
> better.
> 
> These patches, at the moment, do not support a case where a clone mode
> or dual link mode is used on a bridge instead of a panel. My edits
> inside the loop are panel dependent. If we do have oldi setup
> information prior to the beginning of the loop, the panel dependency can
> be removed and some commond code can be written to support an additional
> encoder - bridge connection should it be required.
> 
> Let me know what you think!
> 
> If this apparch is better indeed, I will make these changes before
> sending out the next revision.

I'll say if it's better when I see the code =). But generally speaking, 
I think it's often better to first figure out what is needed and only 
after that do the actual work. Especially in probe-time code where it's 
not a big deal if you iterate over the ports multiple times, instead of 
doing all in a single loop.

  Tomi

[RFC PATCH v5 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.

Add IO CTRL support and control the OLDI TX power for AM625.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c      | 55 ++++++++++++++++++------
 drivers/gpu/drm/tidss/tidss_dispc_regs.h |  6 +++
 2 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 88008ad39b55..68444e0cd8d7 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -921,21 +921,52 @@ 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 : OLDI_PWRDN_TX;
+
+		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);
+
+	} else if (dispc->feat->subrev == DISPC_AM625) {
+		if (power) {
+			switch (dispc->oldi_mode) {
+			case OLDI_SINGLE_LINK_SINGLE_MODE:
+				/* Power down OLDI TX 1 */
+				val = OLDI1_PWRDN_TX;
+				break;
+
+			case OLDI_SINGLE_LINK_CLONE_MODE:
+			case OLDI_DUAL_LINK_MODE:
+				/* No Power down */
+				val = 0;
+				break;
+
+			default:
+				/* Power down both the OLDI TXes */
+				val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX;
+				break;
+			}
+		} else {
+			/* Power down both the OLDI TXes */
+			val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX;
+		}
+
+		regmap_update_bits(dispc->oldi_io_ctrl, OLDI_PD_CTRL,
+				   OLDI0_PWRDN_TX | OLDI1_PWRDN_TX, val);
+	}
 }
 
 static void dispc_set_num_datalines(struct dispc_device *dispc,
@@ -2831,7 +2862,7 @@ int dispc_init(struct tidss_device *tidss)
 		dispc->vp_data[i].gamma_table = gamma_table;
 	}
 
-	if (feat->subrev == DISPC_AM65X) {
+	if (feat->subrev == DISPC_AM65X || feat->subrev == DISPC_AM625) {
 		r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
 		if (r)
 			return r;
diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
index 13feedfe5d6d..510bee70b3b8 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
@@ -238,6 +238,12 @@ enum dispc_common_regs {
 #define OLDI_DAT3_IO_CTRL			0x0C
 #define OLDI_CLK_IO_CTRL			0x10
 
+/* Only for AM625 OLDI TX */
+#define OLDI_PD_CTRL				0x100
+#define OLDI_LB_CTRL				0x104
+
 #define OLDI_PWRDN_TX				BIT(8)
+#define OLDI0_PWRDN_TX				BIT(0)
+#define OLDI1_PWRDN_TX				BIT(1)
 
 #endif /* __TIDSS_DISPC_REGS_H */
-- 
2.37.0

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

[Tomi Valkeinen]

On 28/09/2022 20:52, 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.
> 
> Add IO CTRL support and control the OLDI TX power for AM625.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c      | 55 ++++++++++++++++++------
>   drivers/gpu/drm/tidss/tidss_dispc_regs.h |  6 +++
>   2 files changed, 49 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 88008ad39b55..68444e0cd8d7 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -921,21 +921,52 @@ 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 : OLDI_PWRDN_TX;
> +
> +		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);
> +
> +	} else if (dispc->feat->subrev == DISPC_AM625) {
> +		if (power) {
> +			switch (dispc->oldi_mode) {
> +			case OLDI_SINGLE_LINK_SINGLE_MODE:
> +				/* Power down OLDI TX 1 */
> +				val = OLDI1_PWRDN_TX;
> +				break;
> +
> +			case OLDI_SINGLE_LINK_CLONE_MODE:
> +			case OLDI_DUAL_LINK_MODE:
> +				/* No Power down */
> +				val = 0;
> +				break;
> +
> +			default:
> +				/* Power down both the OLDI TXes */
> +				val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX;
> +				break;
> +			}
> +		} else {
> +			/* Power down both the OLDI TXes */
> +			val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX;
> +		}

Ugh, I hate power-down bits. So you "enable" it to disable it =). What's 
the default value or the register here? Or will this always be called? 
I.e. if we only use DPI, do we power down the OLDIs somewhere (or does 
it matter)?

> +
> +		regmap_update_bits(dispc->oldi_io_ctrl, OLDI_PD_CTRL,
> +				   OLDI0_PWRDN_TX | OLDI1_PWRDN_TX, val);
> +	}
>   }
>   
>   static void dispc_set_num_datalines(struct dispc_device *dispc,
> @@ -2831,7 +2862,7 @@ int dispc_init(struct tidss_device *tidss)
>   		dispc->vp_data[i].gamma_table = gamma_table;
>   	}
>   
> -	if (feat->subrev == DISPC_AM65X) {
> +	if (feat->subrev == DISPC_AM65X || feat->subrev == DISPC_AM625) {
>   		r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
>   		if (r)
>   			return r;
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
> index 13feedfe5d6d..510bee70b3b8 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
> @@ -238,6 +238,12 @@ enum dispc_common_regs {
>   #define OLDI_DAT3_IO_CTRL			0x0C
>   #define OLDI_CLK_IO_CTRL			0x10
>   
> +/* Only for AM625 OLDI TX */
> +#define OLDI_PD_CTRL				0x100
> +#define OLDI_LB_CTRL				0x104
> +
>   #define OLDI_PWRDN_TX				BIT(8)
> +#define OLDI0_PWRDN_TX				BIT(0)
> +#define OLDI1_PWRDN_TX				BIT(1)

Maybe these (the new and old ones) should be platform-prefixed. And 
organized so that the register and its bits are together.

  Tomi

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

[Aradhya Bhatia]

Hi Tomi

On 12-Oct-22 17:59, Tomi Valkeinen wrote:
> On 28/09/2022 20:52, 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.
>>
>> Add IO CTRL support and control the OLDI TX power for AM625.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c      | 55 ++++++++++++++++++------
>>   drivers/gpu/drm/tidss/tidss_dispc_regs.h |  6 +++
>>   2 files changed, 49 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index 88008ad39b55..68444e0cd8d7 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -921,21 +921,52 @@ 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 : OLDI_PWRDN_TX;
>> +
>> +        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);
>> +
>> +    } else if (dispc->feat->subrev == DISPC_AM625) {
>> +        if (power) {
>> +            switch (dispc->oldi_mode) {
>> +            case OLDI_SINGLE_LINK_SINGLE_MODE:
>> +                /* Power down OLDI TX 1 */
>> +                val = OLDI1_PWRDN_TX;
>> +                break;
>> +
>> +            case OLDI_SINGLE_LINK_CLONE_MODE:
>> +            case OLDI_DUAL_LINK_MODE:
>> +                /* No Power down */
>> +                val = 0;
>> +                break;
>> +
>> +            default:
>> +                /* Power down both the OLDI TXes */
>> +                val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX;
>> +                break;
>> +            }
>> +        } else {
>> +            /* Power down both the OLDI TXes */
>> +            val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX;
>> +        }
> 
> Ugh, I hate power-down bits. So you "enable" it to disable it =). What's 
> the default value or the register here? Or will this always be called? 
> I.e. if we only use DPI, do we power down the OLDIs somewhere (or does 
> it matter)?
> 

The power bits are defaulted to keep the OLDI TXes powered off, and they
have to be turned ON.

This function is also called to power off the OLDI TXes, from
dispc_vp_unprepare. And that happens with "power" variable as false. So
the else condition above is indeed required.

You are right about the other scenario though. If DPI is only used, the
mode will be set to OLDI_MODE_OFF and in that case, the dispc_vp_prepare
function will NOT be called for the OLDI VP thereby rendering the
"default" clause of the switch statement, for powering down the OLDIs,
essentially useless. I just put it there because of convention.

>> +
>> +        regmap_update_bits(dispc->oldi_io_ctrl, OLDI_PD_CTRL,
>> +                   OLDI0_PWRDN_TX | OLDI1_PWRDN_TX, val);
>> +    }
>>   }
>>   static void dispc_set_num_datalines(struct dispc_device *dispc,
>> @@ -2831,7 +2862,7 @@ int dispc_init(struct tidss_device *tidss)
>>           dispc->vp_data[i].gamma_table = gamma_table;
>>       }
>> -    if (feat->subrev == DISPC_AM65X) {
>> +    if (feat->subrev == DISPC_AM65X || feat->subrev == DISPC_AM625) {
>>           r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
>>           if (r)
>>               return r;
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h 
>> b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
>> index 13feedfe5d6d..510bee70b3b8 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
>> @@ -238,6 +238,12 @@ enum dispc_common_regs {
>>   #define OLDI_DAT3_IO_CTRL            0x0C
>>   #define OLDI_CLK_IO_CTRL            0x10
>> +/* Only for AM625 OLDI TX */
>> +#define OLDI_PD_CTRL                0x100
>> +#define OLDI_LB_CTRL                0x104
>> +
>>   #define OLDI_PWRDN_TX                BIT(8)
>> +#define OLDI0_PWRDN_TX                BIT(0)
>> +#define OLDI1_PWRDN_TX                BIT(1)
> 
> Maybe these (the new and old ones) should be platform-prefixed. And 
> organized so that the register and its bits are together.
Okay, will do.


Regards
Aradhya

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

[Aradhya Bhatia]

The AM625 DSS IP contains 2 OLDI TXes which can work to enable 2
duplicated displays of smaller resolutions or enable a single Dual Link
display with a higher resolution (1920x1200).

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

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 68444e0cd8d7..fd7f49535f0c 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1003,8 +1003,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
+	 * set statically to 0.
 	 */
 
 	if (fmt->data_width == 24)
@@ -1021,6 +1021,30 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
 
 	oldi_cfg |= BIT(0); /* ENABLE */
 
+	switch (dispc->oldi_mode) {
+	case OLDI_MODE_OFF:
+		oldi_cfg &= ~BIT(0); /* DISABLE */
+		break;
+
+	case OLDI_SINGLE_LINK_SINGLE_MODE:
+		/* All configuration is done for this mode.  */
+		break;
+
+	case OLDI_SINGLE_LINK_CLONE_MODE:
+		oldi_cfg |= BIT(5); /* CLONE MODE */
+		break;
+
+	case OLDI_DUAL_LINK_MODE:
+		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.37.0

Re: [RFC PATCH v5 6/6] drm/tidss: Enable Dual and Duplicate Modes for OLDI

[Tomi Valkeinen]

On 28/09/2022 20:52, Aradhya Bhatia wrote:
> The AM625 DSS IP contains 2 OLDI TXes which can work to enable 2
> duplicated displays of smaller resolutions or enable a single Dual Link
> display with a higher resolution (1920x1200).
> 
> Configure the necessary register to enable and disable the OLDI TXes
> with necessary modes configurations.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c | 28 ++++++++++++++++++++++++++--
>   1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 68444e0cd8d7..fd7f49535f0c 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -1003,8 +1003,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
> +	 * set statically to 0.
>   	 */

While at it, maybe say "are always set to 0". At least to me that's much 
more understandable.

>   
>   	if (fmt->data_width == 24)
> @@ -1021,6 +1021,30 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
>   
>   	oldi_cfg |= BIT(0); /* ENABLE */
>   
> +	switch (dispc->oldi_mode) {
> +	case OLDI_MODE_OFF:
> +		oldi_cfg &= ~BIT(0); /* DISABLE */

Hmm, do we call this if OLDI_MODE_OFF? The current code always enabled 
OLDI here, so I presume we call this only when we actually want to 
enable it.

> +		break;
> +
> +	case OLDI_SINGLE_LINK_SINGLE_MODE:
> +		/* All configuration is done for this mode.  */
> +		break;
> +
> +	case OLDI_SINGLE_LINK_CLONE_MODE:
> +		oldi_cfg |= BIT(5); /* CLONE MODE */
> +		break;
> +
> +	case OLDI_DUAL_LINK_MODE:
> +		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)) &&