[PATCH 0/2] Add support for AM62L DSS

[PATCH 0/2] Add support for AM62L DSS

[Devarsh Thakkar]

This adds support for DSS subsystem present in TI's AM62L SoC
which supports single display pipeline with DPI output which
is also routed to DSI Tx controller within the SoC.

Devarsh Thakkar (2):
  dt-bindings: display: ti,am65x-dss: Add support for AM62L DSS
  drm/tidss: Add support for AM62L display subsystem

 .../bindings/display/ti/ti,am65x-dss.yaml     | 25 +++++++++++++-
 drivers/gpu/drm/tidss/tidss_dispc.c           | 34 +++++++++++++++++++
 drivers/gpu/drm/tidss/tidss_dispc.h           |  2 ++
 drivers/gpu/drm/tidss/tidss_drv.c             |  1 +
 4 files changed, 61 insertions(+), 1 deletion(-)

-- 
2.39.1

[PATCH 1/2] dt-bindings: display: ti,am65x-dss: Add support for AM62L DSS

[Devarsh Thakkar]

The DSS controller on TI's AM62L SoC is an update from that on TI's
AM625/AM65x/AM62A7 SoC. The AM62L DSS [1] only supports a single display
pipeline using a single overlay manager, single video port and a single
video lite pipeline which does not support scaling.

The output of video port is routed to SoC boundary via DPI interface and
the DPI signals from the video port are also routed to DSI Tx controller
present within the SoC.

[1]: Section 11.7 (Display Subsystem and Peripherals)
Link : https://www.ti.com/lit/pdf/sprujb4

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 .../bindings/display/ti/ti,am65x-dss.yaml     | 25 ++++++++++++++++++-
 1 file changed, 24 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 55e3e490d0e6..ba93c6bb02e8 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -12,18 +12,29 @@ maintainers:
   - Tomi Valkeinen <tomi.valkeinen@ti.com>
 
 description: |
-  The AM625 and AM65x TI Keystone Display SubSystem with two output
+  The AM625 and AM65x TI Keystone Display SubSystem has two output
   ports and two video planes. In AM65x DSS, the first video port
   supports 1 OLDI TX and in AM625 DSS, the first video port output is
   internally routed to 2 OLDI TXes. The second video port supports DPI
   format. The first plane is full video plane with all features and the
   second is a "lite plane" without scaling support.
+  The AM62A7 display subsystem has a single output port supporting DPI format
+  although similar to AM625 and AM65x Socs, it has two video planes where first
+  is full video plane with all features and second is a video "lite" plane which
+  does not support scaling.
+  The AM62L display subsystem also has a single output port which supports DPI
+  format but it only supports single video "lite plane" which does not support
+  scaling. The output port is routed to SoC boundary via DPI interface and same
+  DPI signals are also routed internally to DSI Tx controller present within the
+  SoC. Due to clocking limitations only one of the interface i.e. either DSI or
+  DPI can be used at once.
 
 properties:
   compatible:
     enum:
       - ti,am625-dss
       - ti,am62a7,dss
+      - ti,am62l,dss
       - ti,am65x-dss
 
   reg:
@@ -91,6 +102,8 @@ properties:
           For AM625 DSS, the internal DPI output port node from video
           port 1.
           For AM62A7 DSS, the port is tied off inside the SoC.
+          For AM62L DSS, the DSS DPI output port node from video port 1
+          or DSI Tx controller node connected to video port 1.
 
       port@1:
         $ref: /schemas/graph.yaml#/properties/port
@@ -123,6 +136,16 @@ allOf:
         ports:
           properties:
             port@0: false
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: ti,am62l-dss
+    then:
+      properties:
+        ports:
+          properties:
+            port@1: false
 
 required:
   - compatible
-- 
2.39.1

[PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

[Devarsh Thakkar]

Enable display for AM62L DSS [1] which supports only a single display
pipeline using a single overlay manager, single video port and a single
video lite pipeline which does not support scaling.

The output of video port is routed to SoC boundary via DPI interface and
the DPI signals from the video port are also routed to DSI Tx controller
present within the SoC.

[1]: Section 11.7 (Display Subsystem and Peripherals)
Link : https://www.ti.com/lit/pdf/sprujb4

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 34 +++++++++++++++++++++++++++++
 drivers/gpu/drm/tidss/tidss_dispc.h |  2 ++
 drivers/gpu/drm/tidss/tidss_drv.c   |  1 +
 3 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index cacb5f3d8085..cd322d60b825 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -376,6 +376,35 @@ const struct dispc_features dispc_am62a7_feats = {
 	.vid_order = { 1, 0 },
 };
 
+const struct dispc_features dispc_am62l_feats = {
+	.max_pclk_khz = {
+		[DISPC_VP_DPI] = 165000,
+	},
+
+	.subrev = DISPC_AM62L,
+
+	.common = "common",
+	.common_regs = tidss_am65x_common_regs,
+
+	.num_vps = 1,
+	.vp_name = { "vp1" },
+	.ovr_name = { "ovr1" },
+	.vpclk_name =  { "vp1" },
+	.vp_bus_type = { DISPC_VP_DPI },
+
+	.vp_feat = { .color = {
+			.has_ctm = true,
+			.gamma_size = 256,
+			.gamma_type = TIDSS_GAMMA_8BIT,
+		},
+	},
+
+	.num_planes = 1,
+	.vid_name = { "vidl1" },
+	.vid_lite = { true },
+	.vid_order = { 0 },
+};
+
 static const u16 *dispc_common_regmap;
 
 struct dss_vp_data {
@@ -879,6 +908,7 @@ dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc)
 		return dispc_k2g_read_and_clear_irqstatus(dispc);
 	case DISPC_AM625:
 	case DISPC_AM62A7:
+	case DISPC_AM62L:
 	case DISPC_AM65X:
 	case DISPC_J721E:
 		return dispc_k3_read_and_clear_irqstatus(dispc);
@@ -896,6 +926,7 @@ void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
 		break;
 	case DISPC_AM625:
 	case DISPC_AM62A7:
+	case DISPC_AM62L:
 	case DISPC_AM65X:
 	case DISPC_J721E:
 		dispc_k3_set_irqenable(dispc, mask);
@@ -1388,6 +1419,7 @@ void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane,
 		break;
 	case DISPC_AM625:
 	case DISPC_AM62A7:
+	case DISPC_AM62L:
 	case DISPC_AM65X:
 		dispc_am65x_ovr_set_plane(dispc, hw_plane, hw_videoport,
 					  x, y, layer);
@@ -2308,6 +2340,7 @@ static void dispc_plane_init(struct dispc_device *dispc)
 		break;
 	case DISPC_AM625:
 	case DISPC_AM62A7:
+	case DISPC_AM62L:
 	case DISPC_AM65X:
 	case DISPC_J721E:
 		dispc_k3_plane_init(dispc);
@@ -2416,6 +2449,7 @@ static void dispc_vp_write_gamma_table(struct dispc_device *dispc,
 		break;
 	case DISPC_AM625:
 	case DISPC_AM62A7:
+	case DISPC_AM62L:
 	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 086327d51a90..275de8d02b75 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -62,6 +62,7 @@ enum dispc_dss_subrevision {
 	DISPC_K2G,
 	DISPC_AM625,
 	DISPC_AM62A7,
+	DISPC_AM62L,
 	DISPC_AM65X,
 	DISPC_J721E,
 };
@@ -91,6 +92,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_am62a7_feats;
+extern const struct dispc_features dispc_am62l_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 d4652e8cc28c..f2a4f659f574 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -242,6 +242,7 @@ 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,am62a7-dss", .data = &dispc_am62a7_feats, },
+	{ .compatible = "ti,am62l-dss", .data = &dispc_am62l_feats, },
 	{ .compatible = "ti,am65x-dss", .data = &dispc_am65x_feats, },
 	{ .compatible = "ti,j721e-dss", .data = &dispc_j721e_feats, },
 	{ }
-- 
2.39.1

Re: [PATCH 1/2] dt-bindings: display: ti,am65x-dss: Add support for AM62L DSS

[Krzysztof Kozlowski]

On Tue, Dec 31, 2024 at 02:34:31PM +0530, Devarsh Thakkar wrote:
> The DSS controller on TI's AM62L SoC is an update from that on TI's
> AM625/AM65x/AM62A7 SoC. The AM62L DSS [1] only supports a single display
> pipeline using a single overlay manager, single video port and a single
> video lite pipeline which does not support scaling.
> 
> The output of video port is routed to SoC boundary via DPI interface and
> the DPI signals from the video port are also routed to DSI Tx controller
> present within the SoC.
> 
> [1]: Section 11.7 (Display Subsystem and Peripherals)
> Link : https://www.ti.com/lit/pdf/sprujb4
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
>  .../bindings/display/ti/ti,am65x-dss.yaml     | 25 ++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

[Tomi Valkeinen]

Hi,

On 31/12/2024 11:04, Devarsh Thakkar wrote:
> Enable display for AM62L DSS [1] which supports only a single display
> pipeline using a single overlay manager, single video port and a single
> video lite pipeline which does not support scaling.
> 
> The output of video port is routed to SoC boundary via DPI interface and
> the DPI signals from the video port are also routed to DSI Tx controller
> present within the SoC.
> 
> [1]: Section 11.7 (Display Subsystem and Peripherals)
> Link : https://www.ti.com/lit/pdf/sprujb4
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c | 34 +++++++++++++++++++++++++++++
>   drivers/gpu/drm/tidss/tidss_dispc.h |  2 ++
>   drivers/gpu/drm/tidss/tidss_drv.c   |  1 +
>   3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index cacb5f3d8085..cd322d60b825 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -376,6 +376,35 @@ const struct dispc_features dispc_am62a7_feats = {
>   	.vid_order = { 1, 0 },
>   };
>   
> +const struct dispc_features dispc_am62l_feats = {
> +	.max_pclk_khz = {
> +		[DISPC_VP_DPI] = 165000,
> +	},
> +
> +	.subrev = DISPC_AM62L,
> +
> +	.common = "common",
> +	.common_regs = tidss_am65x_common_regs,
> +
> +	.num_vps = 1,
> +	.vp_name = { "vp1" },
> +	.ovr_name = { "ovr1" },
> +	.vpclk_name =  { "vp1" },
> +	.vp_bus_type = { DISPC_VP_DPI },
> +
> +	.vp_feat = { .color = {
> +			.has_ctm = true,
> +			.gamma_size = 256,
> +			.gamma_type = TIDSS_GAMMA_8BIT,
> +		},
> +	},
> +
> +	.num_planes = 1,
> +	.vid_name = { "vidl1" },
> +	.vid_lite = { true },
> +	.vid_order = { 0 },
> +};
> +
>   static const u16 *dispc_common_regmap;
>   
>   struct dss_vp_data {
> @@ -879,6 +908,7 @@ dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc)
>   		return dispc_k2g_read_and_clear_irqstatus(dispc);
>   	case DISPC_AM625:
>   	case DISPC_AM62A7:
> +	case DISPC_AM62L:
>   	case DISPC_AM65X:
>   	case DISPC_J721E:

Not related to this patch, but these lists are getting a bit silly... We 
probably should handle them in a more centralized way. Maybe with the 
dispc_features.

  Tomi

>   		return dispc_k3_read_and_clear_irqstatus(dispc);
> @@ -896,6 +926,7 @@ void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
>   		break;
>   	case DISPC_AM625:
>   	case DISPC_AM62A7:
> +	case DISPC_AM62L:
>   	case DISPC_AM65X:
>   	case DISPC_J721E:
>   		dispc_k3_set_irqenable(dispc, mask);
> @@ -1388,6 +1419,7 @@ void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane,
>   		break;
>   	case DISPC_AM625:
>   	case DISPC_AM62A7:
> +	case DISPC_AM62L:
>   	case DISPC_AM65X:
>   		dispc_am65x_ovr_set_plane(dispc, hw_plane, hw_videoport,
>   					  x, y, layer);
> @@ -2308,6 +2340,7 @@ static void dispc_plane_init(struct dispc_device *dispc)
>   		break;
>   	case DISPC_AM625:
>   	case DISPC_AM62A7:
> +	case DISPC_AM62L:
>   	case DISPC_AM65X:
>   	case DISPC_J721E:
>   		dispc_k3_plane_init(dispc);
> @@ -2416,6 +2449,7 @@ static void dispc_vp_write_gamma_table(struct dispc_device *dispc,
>   		break;
>   	case DISPC_AM625:
>   	case DISPC_AM62A7:
> +	case DISPC_AM62L:
>   	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 086327d51a90..275de8d02b75 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -62,6 +62,7 @@ enum dispc_dss_subrevision {
>   	DISPC_K2G,
>   	DISPC_AM625,
>   	DISPC_AM62A7,
> +	DISPC_AM62L,
>   	DISPC_AM65X,
>   	DISPC_J721E,
>   };
> @@ -91,6 +92,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_am62a7_feats;
> +extern const struct dispc_features dispc_am62l_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 d4652e8cc28c..f2a4f659f574 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -242,6 +242,7 @@ 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,am62a7-dss", .data = &dispc_am62a7_feats, },
> +	{ .compatible = "ti,am62l-dss", .data = &dispc_am62l_feats, },
>   	{ .compatible = "ti,am65x-dss", .data = &dispc_am65x_feats, },
>   	{ .compatible = "ti,j721e-dss", .data = &dispc_j721e_feats, },
>   	{ }

Re: [PATCH 0/2] Add support for AM62L DSS

[Tomi Valkeinen]

Hi,

On 31/12/2024 11:04, Devarsh Thakkar wrote:
> This adds support for DSS subsystem present in TI's AM62L SoC
> which supports single display pipeline with DPI output which
> is also routed to DSI Tx controller within the SoC.
> 
> Devarsh Thakkar (2):
>    dt-bindings: display: ti,am65x-dss: Add support for AM62L DSS
>    drm/tidss: Add support for AM62L display subsystem
> 
>   .../bindings/display/ti/ti,am65x-dss.yaml     | 25 +++++++++++++-
>   drivers/gpu/drm/tidss/tidss_dispc.c           | 34 +++++++++++++++++++
>   drivers/gpu/drm/tidss/tidss_dispc.h           |  2 ++
>   drivers/gpu/drm/tidss/tidss_drv.c             |  1 +
>   4 files changed, 61 insertions(+), 1 deletion(-)
> 

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

  Tomi

Re: [PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

[Aradhya Bhatia]

Hi Devarsh,

Thanks for the patches.

On 31/12/24 14:34, Devarsh Thakkar wrote:
> Enable display for AM62L DSS [1] which supports only a single display
> pipeline using a single overlay manager, single video port and a single
> video lite pipeline which does not support scaling.
> 
> The output of video port is routed to SoC boundary via DPI interface and
> the DPI signals from the video port are also routed to DSI Tx controller
> present within the SoC.
> 
> [1]: Section 11.7 (Display Subsystem and Peripherals)
> Link : https://www.ti.com/lit/pdf/sprujb4
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 34 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/tidss/tidss_dispc.h |  2 ++
>  drivers/gpu/drm/tidss/tidss_drv.c   |  1 +
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index cacb5f3d8085..cd322d60b825 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -376,6 +376,35 @@ const struct dispc_features dispc_am62a7_feats = {
>  	.vid_order = { 1, 0 },
>  };
>  
> +const struct dispc_features dispc_am62l_feats = {
> +	.max_pclk_khz = {
> +		[DISPC_VP_DPI] = 165000,

The TRM mentions that the max the VP PLL can go is 150MHz, so maybe you
might need to update this.

That said, as far as I understand, the IP itself can support 165 MHz,
and I am wondering, what would we do if there comes out a new SoC that
uses AM62L DSS as is, but just bumps up the PLL capacity to 165MHz.

It would be odd to have a ditto feats structure with just the frequency
updated.

> +	},
> +
> +	.subrev = DISPC_AM62L,
> +
> +	.common = "common",
> +	.common_regs = tidss_am65x_common_regs,

Also, I don't think we should utilize this as is.

The AM62L common regions is different, and is, at best, a subset of the
AM65x/AM62x common register space.

For example, registers VID_IRQ{STATUS, ENABLE}_0 have been dropped,
along with VP_IRQ{STATUS, ENABLE}_1.

- Which brings to my next concern...

> +
> +	.num_vps = 1,
> +	.vp_name = { "vp1" },
> +	.ovr_name = { "ovr1" },
> +	.vpclk_name =  { "vp1" },
> +	.vp_bus_type = { DISPC_VP_DPI },
> +
> +	.vp_feat = { .color = {
> +			.has_ctm = true,
> +			.gamma_size = 256,
> +			.gamma_type = TIDSS_GAMMA_8BIT,
> +		},
> +	},
> +
> +	.num_planes = 1,
> +	.vid_name = { "vidl1" },
> +	.vid_lite = { true },
> +	.vid_order = { 0 },

...

Usually, VID1 is the first video pipeline, while VIDL1 remains the
second. Which is why vid1 occupies the index 0, and vidl1 occupies 1 -
even from the hardware perspective.

While AM62L has only one video (lite) pipeline - vidl1, and there is no
vid1, the hardware still treats vidl1 at index 1.

For example, the TRM has defined DISPC_VID_IRQ{STATUS, ENABLE}_1 (and
not _0) in the common region.

So, the vid_order here should be 1, not 0.


Regards
Aradhya

Re: [PATCH 1/2] dt-bindings: display: ti,am65x-dss: Add support for AM62L DSS

[Jayesh Choudhary]

Hello Devarsh,

On 31/12/24 14:34, Devarsh Thakkar wrote:
> The DSS controller on TI's AM62L SoC is an update from that on TI's
> AM625/AM65x/AM62A7 SoC. The AM62L DSS [1] only supports a single display
> pipeline using a single overlay manager, single video port and a single
> video lite pipeline which does not support scaling.
> 
> The output of video port is routed to SoC boundary via DPI interface and
> the DPI signals from the video port are also routed to DSI Tx controller
> present within the SoC.
> 
> [1]: Section 11.7 (Display Subsystem and Peripherals)
> Link : https://www.ti.com/lit/pdf/sprujb4
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
>   .../bindings/display/ti/ti,am65x-dss.yaml     | 25 ++++++++++++++++++-
>   1 file changed, 24 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 55e3e490d0e6..ba93c6bb02e8 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -12,18 +12,29 @@ maintainers:
>     - Tomi Valkeinen <tomi.valkeinen@ti.com>
>   
>   description: |
> -  The AM625 and AM65x TI Keystone Display SubSystem with two output
> +  The AM625 and AM65x TI Keystone Display SubSystem has two output
>     ports and two video planes. In AM65x DSS, the first video port
>     supports 1 OLDI TX and in AM625 DSS, the first video port output is
>     internally routed to 2 OLDI TXes. The second video port supports DPI
>     format. The first plane is full video plane with all features and the
>     second is a "lite plane" without scaling support.
> +  The AM62A7 display subsystem has a single output port supporting DPI format
> +  although similar to AM625 and AM65x Socs, it has two video planes where first
> +  is full video plane with all features and second is a video "lite" plane which
> +  does not support scaling.
> +  The AM62L display subsystem also has a single output port which supports DPI
> +  format but it only supports single video "lite plane" which does not support
> +  scaling. The output port is routed to SoC boundary via DPI interface and same
> +  DPI signals are also routed internally to DSI Tx controller present within the
> +  SoC. Due to clocking limitations only one of the interface i.e. either DSI or
> +  DPI can be used at once.
>   
>   properties:
>     compatible:
>       enum:
>         - ti,am625-dss
>         - ti,am62a7,dss
> +      - ti,am62l,dss


s/ti,am62l,dss/ti,am62l-dss

There seems to be inconsistency in the usage of the compatible name in
the conditional below.

Same is with the compatible "ti,am62a7,dss".
Can you fix that too?


With the above comments addressed,

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


>         - ti,am65x-dss
>   
>     reg:
> @@ -91,6 +102,8 @@ properties:
>             For AM625 DSS, the internal DPI output port node from video
>             port 1.
>             For AM62A7 DSS, the port is tied off inside the SoC.
> +          For AM62L DSS, the DSS DPI output port node from video port 1
> +          or DSI Tx controller node connected to video port 1.
>   
>         port@1:
>           $ref: /schemas/graph.yaml#/properties/port
> @@ -123,6 +136,16 @@ allOf:
>           ports:
>             properties:
>               port@0: false
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: ti,am62l-dss
> +    then:
> +      properties:
> +        ports:
> +          properties:
> +            port@1: false
>   
>   required:
>     - compatible

Warm Regards,
Jayesh

Re: [PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

[Devarsh Thakkar]

Hi Aradhya,

On 18/01/25 14:57, Aradhya Bhatia wrote:
> Hi Devarsh,
>
> Thanks for the patches.
>

Thanks for the review.

> On 31/12/24 14:34, Devarsh Thakkar wrote:
>> Enable display for AM62L DSS [1] which supports only a single display
>> pipeline using a single overlay manager, single video port and a single
>> video lite pipeline which does not support scaling.
>>
>> The output of video port is routed to SoC boundary via DPI interface and
>> the DPI signals from the video port are also routed to DSI Tx controller
>> present within the SoC.
>>
>> [1]: Section 11.7 (Display Subsystem and Peripherals)
>> Link : https://www.ti.com/lit/pdf/sprujb4
>>
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---

<snip>
>> 
>> +const struct dispc_features dispc_am62l_feats = {
>> +    .max_pclk_khz = {
>> +            [DISPC_VP_DPI] = 165000,
>
> The TRM mentions that the max the VP PLL can go is 150MHz, so maybe you
> might need to update this.
>
> That said, as far as I understand, the IP itself can support 165 MHz,
> and I am wondering, what would we do if there comes out a new SoC that
> uses AM62L DSS as is, but just bumps up the PLL capacity to 165MHz.
>
> It would be odd to have a ditto feats structure with just the frequency
> updated.

TRM mentions max VP PLL frequency as 165 Mhz and not 150 Mhz. Please refer
Table 11-343. DSS Signals for MIPI DPI 2.0 or BT.656/BT.1120 section in
section 11.7.1.2.1 DSS Parallel Interface of  AM62L TRM [1].
>
>> +    },
>> +
>> +    .subrev = DISPC_AM62L,
>> +
>> +    .common = "common",
>> +    .common_regs = tidss_am65x_common_regs,
>
> Also, I don't think we should utilize this as is.
>
> The AM62L common regions is different, and is, at best, a subset of the
> AM65x/AM62x common register space.
>
> For example, registers VID_IRQ{STATUS, ENABLE}_0 have been dropped,
> along with VP_IRQ{STATUS, ENABLE}_1.
>
> - Which brings to my next concern...
>

Thanks for pointing out, I probably missed this since the use-case still
worked since VP interrupts were still enabled and those were sufficient to
drive the display
but the VID underflow interrupts and VID specific bits were probably not
enabled at-all due to above miss, so agreed
we should probably go ahead with a different reg space for AM62L due to
aforementioned differences.

>> +
>> +    .num_vps = 1,
>> +    .vp_name = { "vp1" },
>> +    .ovr_name = { "ovr1" },
>> +    .vpclk_name =  { "vp1" },
>> +    .vp_bus_type = { DISPC_VP_DPI },
>> +
>> +    .vp_feat = { .color = {
>> +                    .has_ctm = true,
>> +                    .gamma_size = 256,
>> +                    .gamma_type = TIDSS_GAMMA_8BIT,
>> +            },
>> +    },
>> +
>> +    .num_planes = 1,
>> +    .vid_name = { "vidl1" },
>> +    .vid_lite = { true },
>> +    .vid_order = { 0 },
>
> ...
>
> Usually, VID1 is the first video pipeline, while VIDL1 remains the
> second. Which is why vid1 occupies the index 0, and vidl1 occupies 1 -
> even from the hardware perspective.
>
> While AM62L has only one video (lite) pipeline - vidl1, and there is no
> vid1, the hardware still treats vidl1 at index 1.
>
> For example, the TRM has defined DISPC_VID_IRQ{STATUS, ENABLE}_1 (and
> not _0) in the common region.
>
> So, the vid_order here should be 1, not 0.
>

We will have a separate register space for AM62L which would only have
DISPC_VID_IRQ{STATUS, ENABLE}_1 mapped,
so that should handle it.Also I think vid_order semantically maps to zorder
and since there is only a single plane available on AM62L
it is not correct to assign vid_order as 1 just to align with VIDL reg bit fields.
Furthermore, vid_order set to 1 won't work too, since driver also uses
vid_order for indexing the reg space for vid. For e.g.
if we use vid_order as 1 then hw_plane would get assigned as 1 too, then
dispc_vid* functions (as seen below) would fail as
there is no base_vid region at index 1 for AM62L as it has only single video
register space i.e. for VIDL.

static                                                                                                 
 
void dispc_vid_write(struct dispc_device *dispc, u32 hw_plane, u16 reg, u32
val)                         
{                                                                                                      
 
        void __iomem *base =
dispc->base_vid[hw_plane];                                                  
                                                                                                       
 
        iowrite32(val, base +
reg);                                                                      
}                                                                                                      
 

But with hw_plane set to 0 we also need to handle bit fields for VIDL which
are still at index 5 for registers DSS_COMMON_DISPC_IRQENABLE_SET,
DSS_COMMON_DISPC_IRQENABLE_CLR and DSS_COMMON_DISPC_IRQSTATUS_(RAW) and also
DSS_OVR1_ATTRIBUTES_0 which expects 1 to be set in bits 4:1,
and existing dispc_k3* functions managing these registers were handling it via
hw_plane set to 1.  To handle this and also since AM62L is the only one K3 SoC
which only supports VIDL plane, I will be adding separate wrapper implementer
functions dispc_am62l_ovr_set_plane, dispc_am62l_set_irqenable to handle this.

Regards
Devarsh

Re: [PATCH 1/2] dt-bindings: display: ti,am65x-dss: Add support for AM62L DSS

[Devarsh Thakkar]

Hi Jayesh,

On 24/01/25 13:50, Jayesh Choudhary wrote:
<snip>
>>     compatible:
>>       enum:
>>         - ti,am625-dss
>>         - ti,am62a7,dss
>> +      - ti,am62l,dss
> 
> 
> s/ti,am62l,dss/ti,am62l-dss
> 
> There seems to be inconsistency in the usage of the compatible name in
> the conditional below.
> 
> Same is with the compatible "ti,am62a7,dss".
> Can you fix that too?
> 
> 
> With the above comments addressed,
> 
> Reviewed-by: Jayesh Choudhary <j-choudhary@ti.com>
> 

Thanks for the comments, indeed above needs to be fixed, will include it in V2.

Regards
Devarsh

Re: [PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

[Tomi Valkeinen]

Hi,

On 28/01/2025 15:16, Devarsh Thakkar wrote:
> Hi Aradhya,
> 
> On 18/01/25 14:57, Aradhya Bhatia wrote:
>> Hi Devarsh,
>>
>> Thanks for the patches.
>>
> 
> Thanks for the review.
> 
>> On 31/12/24 14:34, Devarsh Thakkar wrote:
>>> Enable display for AM62L DSS [1] which supports only a single display
>>> pipeline using a single overlay manager, single video port and a single
>>> video lite pipeline which does not support scaling.
>>>
>>> The output of video port is routed to SoC boundary via DPI interface and
>>> the DPI signals from the video port are also routed to DSI Tx controller
>>> present within the SoC.
>>>
>>> [1]: Section 11.7 (Display Subsystem and Peripherals)
>>> Link : https://www.ti.com/lit/pdf/sprujb4
>>>
>>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>>> ---
> 
> <snip>
>>>   
>>> +const struct dispc_features dispc_am62l_feats = {
>>> +    .max_pclk_khz = {
>>> +            [DISPC_VP_DPI] = 165000,
>>
>> The TRM mentions that the max the VP PLL can go is 150MHz, so maybe you
>> might need to update this.
>>
>> That said, as far as I understand, the IP itself can support 165 MHz,
>> and I am wondering, what would we do if there comes out a new SoC that
>> uses AM62L DSS as is, but just bumps up the PLL capacity to 165MHz.
>>
>> It would be odd to have a ditto feats structure with just the frequency
>> updated.
> 
> TRM mentions max VP PLL frequency as 165 Mhz and not 150 Mhz. Please refer
> Table 11-343. DSS Signals for MIPI DPI 2.0 or BT.656/BT.1120 section in
> section 11.7.1.2.1 DSS Parallel Interface of  AM62L TRM [1].
>>
>>> +    },
>>> +
>>> +    .subrev = DISPC_AM62L,
>>> +
>>> +    .common = "common",
>>> +    .common_regs = tidss_am65x_common_regs,
>>
>> Also, I don't think we should utilize this as is.
>>
>> The AM62L common regions is different, and is, at best, a subset of the
>> AM65x/AM62x common register space.
>>
>> For example, registers VID_IRQ{STATUS, ENABLE}_0 have been dropped,
>> along with VP_IRQ{STATUS, ENABLE}_1.
>>
>> - Which brings to my next concern...
>>
> 
> Thanks for pointing out, I probably missed this since the use-case still
> worked since VP interrupts were still enabled and those were sufficient to
> drive the display
> but the VID underflow interrupts and VID specific bits were probably not
> enabled at-all due to above miss, so agreed
> we should probably go ahead with a different reg space for AM62L due to
> aforementioned differences.

I think I disagree here. Afaiu, AM62L has plane at hw index 1 (VIDL1), 
but the plane at hw index 0 (VID1) is not instantiated in the hardware. 
But the registers are the same, i.e. AM62L's registers for VIDL1 match 
AM65x/AM62x registers, right?

If so, we just need to tell the driver the hw index, instead of creating 
new register offsets as done in v2.

Or am I missing something here? (I haven't looked at the HW manual yet).

  Tomi

Re: [PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

[Devarsh Thakkar]

Hi Tomi

>> Thanks for pointing out, I probably missed this since the use-case still
>> worked since VP interrupts were still enabled and those were sufficient to
>> drive the display
>> but the VID underflow interrupts and VID specific bits were probably not
>> enabled at-all due to above miss, so agreed
>> we should probably go ahead with a different reg space for AM62L due to
>> aforementioned differences.
> 
> I think I disagree here. Afaiu, AM62L has plane at hw index 1 (VIDL1), but the
> plane at hw index 0 (VID1) is not instantiated in the hardware. But the
> registers are the same, i.e. AM62L's registers for VIDL1 match AM65x/AM62x
> registers, right?
> 
> If so, we just need to tell the driver the hw index, instead of creating new
> register offsets as done in v2.
> 
> Or am I missing something here? (I haven't looked at the HW manual yet).
> 

No that's not the only difference. For AM62L, the VID_IRQENABLE/STATUS
registers start at +0x4 as compared to AM65x/AM62x/ :

AM62L:
        [DISPC_VID_IRQENABLE_OFF] =             0x48,

        [DISPC_VID_IRQSTATUS_OFF] =             0x5c,

AM62x/AM65x:

        [DISPC_VID_IRQENABLE_OFF] =             0x44,

        [DISPC_VID_IRQSTATUS_OFF] =             0x58,

So it seems okay to me to have a separate reg offset struct.

Regards
Devarsh

Re: [PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

[Tomi Valkeinen]

Hi,

On 05/02/2025 07:53, Devarsh Thakkar wrote:
> Hi Tomi
> 
>>> Thanks for pointing out, I probably missed this since the use-case still
>>> worked since VP interrupts were still enabled and those were sufficient to
>>> drive the display
>>> but the VID underflow interrupts and VID specific bits were probably not
>>> enabled at-all due to above miss, so agreed
>>> we should probably go ahead with a different reg space for AM62L due to
>>> aforementioned differences.
>>
>> I think I disagree here. Afaiu, AM62L has plane at hw index 1 (VIDL1), but the
>> plane at hw index 0 (VID1) is not instantiated in the hardware. But the
>> registers are the same, i.e. AM62L's registers for VIDL1 match AM65x/AM62x
>> registers, right?
>>
>> If so, we just need to tell the driver the hw index, instead of creating new
>> register offsets as done in v2.
>>
>> Or am I missing something here? (I haven't looked at the HW manual yet).
>>
> 
> No that's not the only difference. For AM62L, the VID_IRQENABLE/STATUS
> registers start at +0x4 as compared to AM65x/AM62x/ :
> 
> AM62L:
>          [DISPC_VID_IRQENABLE_OFF] =             0x48,
> 
>          [DISPC_VID_IRQSTATUS_OFF] =             0x5c,
> 
> AM62x/AM65x:
> 
>          [DISPC_VID_IRQENABLE_OFF] =             0x44,
> 
>          [DISPC_VID_IRQSTATUS_OFF] =             0x58,

But they actually don't start at different offsets.

AM62L TRM has:

DSS_COMMON_VID_IRQENABLE_1  3020 0048h
DSS_COMMON_VID_IRQSTATUS_1  3020 005Ch

AM65X TRM has:

DSS0_COMMON_VID_IRQENABLE_1  04A0 0048h
DSS0_COMMON_VID_IRQSTATUS_1  04A0 005Ch

It's just that on AM62L the VID0 isn't instantiated in the hardware. 
VID1 is at the same place.

  Tomi