[PATCH v2 0/3] Add Display support for AM62P SoC

[PATCH v2 0/3] Add Display support for AM62P SoC

[Swamil Jain]

Display Controller Overview:
TI's AM62P[1] SoC has two instances of TI's Display Subsystem (DSS).
Each instance contains two video ports. Combined, both instances support
up to three independent video streams: OLDI, DPI, and DSI.

This series:
  1. Updates bindings (PATCH 1/3)
    - Adds "ti,am62p-dss" compatible string
    - Modifies power-domain requirements
  2. Updates driver (PATCH 2/3 and 3/3)
    - Adds power management for attached PM domains
    - Enables AM62P DSS support by adding compatible to the driver

Note:
  - Device-tree changes will be submitted after this series is merged.  
  - The device-tree patches are available here[2]

[1]: https://www.ti.com/product/AM62P
[2]: https://github.com/swamiljain/linux-next/tree/AM62P_J722S_DSS_v1
---
Changelog:
v1->v2:
  - PATCH 1/3: - Remove unnecessary example
               - Use "am62p-dss" compatible check for multiple
                 power-domains
  - PATCH 2/3:   Add Signed-off-by tag

Link to v1:
https://lore.kernel.org/all/20251114064336.3683731-1-s-jain1@ti.com/
---
Devarsh Thakkar (1):
  drm/tidss: Power up attached PM domains on probe

Swamil Jain (2):
  dt-bindings: display: ti,am65x-dss: Add am62p dss compatible
  drm: tidss: tidss_drv: Add support for AM62P display subsystem

 .../bindings/display/ti/ti,am65x-dss.yaml     | 25 ++++++
 drivers/gpu/drm/tidss/tidss_drv.c             | 89 ++++++++++++++++++-
 drivers/gpu/drm/tidss/tidss_drv.h             |  4 +
 3 files changed, 115 insertions(+), 3 deletions(-)

[PATCH v2 1/3] dt-bindings: display: ti,am65x-dss: Add am62p dss compatible

[Swamil Jain]

TI's AM62P SoC contains two instances of the TI Keystone Display
SubSystem (DSS), each with two video ports and two video planes. These
instances support up to three independent video streams through OLDI,
DPI, and DSI interfaces.

DSS0 (first instance) supports:
 - Two OLDI transmitters on video port 1, configurable in dual-link or
   single-link mode.
 - DPI output on video port 2.

DSS1 (second instance) supports:
 - One OLDI transmitter on video port 1 (single-link mode only).
 - DSI controller output on video port 2.

The two OLDI transmitters can be configured in clone mode to drive a
pair of identical OLDI single-link displays. DPI outputs from
DSS0 VP2, DSS1 VP1, and DSS1 VP2 are multiplexed, allowing only one
DPI output at a time.

Add the compatible string "ti,am62p-dss" and update related
description accordingly.

AM62P has different power domains for DSS and OLDI compared to other
Keystone SoCs. Therefore, add 'minItems' and set to 1 and 'maxItems'
field in the power-domains property to 3 for the "ti,am62p-dss"
compatible entry to reflect this hardware difference.

Signed-off-by: Swamil Jain <s-jain1@ti.com>
---
 .../bindings/display/ti/ti,am65x-dss.yaml     | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 361e9cae6896..3945ae048b8f 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -24,6 +24,19 @@ description: |
   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.
+  The AM62P has two instances of TI Keystone Display SubSystem, each with two
+  video ports and two video planes. These instances can support up to 3
+  independent video streams through OLDI, DPI, and DSI interfaces.
+  DSS0 (first instance) supports:
+    - Two OLDI TXes on video port 1, configurable in dual-link or
+      single link clone mode
+    - DPI output on video port 2
+  DSS1 (second instance) supports:
+    - One OLDI TX on video port 1 (single-link mode only)
+    - DSI controller output on video port 2
+  The two OLDI TXes can be configured in clone mode to drive a pair of
+  identical OLDI single-link displays. DPI outputs from DSS0 VP2, DSS1 VP1,
+  and DSS1 VP2 are muxed, allowing only one DPI output at a time.
 
 properties:
   compatible:
@@ -31,6 +44,7 @@ properties:
       - ti,am625-dss
       - ti,am62a7-dss
       - ti,am62l-dss
+      - ti,am62p-dss
       - ti,am65x-dss
 
   reg:
@@ -197,6 +211,17 @@ allOf:
               properties:
                 endpoint@1: false
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: ti,am62p-dss
+    then:
+      properties:
+        power-domains:
+          minItems: 1
+          maxItems: 3
+
 required:
   - compatible
   - reg

[PATCH v2 2/3] drm/tidss: Power up attached PM domains on probe

[Swamil Jain]

From: Devarsh Thakkar <devarsht@ti.com>

Some SoC's such as AM62P have dedicated power domains
for OLDI which need to be powered on separately along
with display controller.

So during driver probe, power up all attached PM domains
enumerated in devicetree node for DSS.

This also prepares base to add display support for AM62P.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
[j-choudhary@ti.com: fix PM call sequence causing kernel crash in OLDI]
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
Signed-off-by: Swamil Jain <s-jain1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_drv.c | 88 +++++++++++++++++++++++++++++--
 drivers/gpu/drm/tidss/tidss_drv.h |  4 ++
 2 files changed, 89 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index 1c8cc18bc53c..50158281715f 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -8,6 +8,7 @@
 #include <linux/of.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_domain.h>
 #include <linux/aperture.h>
 
 #include <drm/clients/drm_client_setup.h>
@@ -107,6 +108,72 @@ static const struct drm_driver tidss_driver = {
 	.minor			= 0,
 };
 
+static int tidss_detach_pm_domains(struct tidss_device *tidss)
+{
+	int i;
+
+	if (tidss->num_domains <= 1)
+		return 0;
+
+	for (i = 0; i < tidss->num_domains; i++) {
+		if (tidss->pd_link[i] && !IS_ERR(tidss->pd_link[i]))
+			device_link_del(tidss->pd_link[i]);
+		if (tidss->pd_dev[i] && !IS_ERR(tidss->pd_dev[i]))
+			dev_pm_domain_detach(tidss->pd_dev[i], true);
+		tidss->pd_dev[i] = NULL;
+		tidss->pd_link[i] = NULL;
+	}
+
+	return 0;
+}
+
+static int tidss_attach_pm_domains(struct tidss_device *tidss)
+{
+	struct device *dev = tidss->dev;
+	int i;
+	int ret;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct device_node *np = pdev->dev.of_node;
+
+	tidss->num_domains = of_count_phandle_with_args(np, "power-domains",
+							"#power-domain-cells");
+	if (tidss->num_domains <= 1) {
+		dev_dbg(dev, "One or less power domains, no need to do attach domains\n");
+		return 0;
+	}
+
+	tidss->pd_dev = devm_kmalloc_array(dev, tidss->num_domains,
+					   sizeof(*tidss->pd_dev), GFP_KERNEL);
+	if (!tidss->pd_dev)
+		return -ENOMEM;
+
+	tidss->pd_link = devm_kmalloc_array(dev, tidss->num_domains,
+					    sizeof(*tidss->pd_link), GFP_KERNEL);
+	if (!tidss->pd_link)
+		return -ENOMEM;
+
+	for (i = 0; i < tidss->num_domains; i++) {
+		tidss->pd_dev[i] = dev_pm_domain_attach_by_id(dev, i);
+		if (IS_ERR(tidss->pd_dev[i])) {
+			ret = PTR_ERR(tidss->pd_dev[i]);
+			goto fail;
+		}
+
+		tidss->pd_link[i] = device_link_add(dev, tidss->pd_dev[i],
+						    DL_FLAG_STATELESS |
+						    DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
+		if (!tidss->pd_link[i]) {
+			ret = -EINVAL;
+			goto fail;
+		}
+	}
+
+	return 0;
+fail:
+	tidss_detach_pm_domains(tidss);
+	return ret;
+}
+
 static int tidss_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -129,15 +196,24 @@ static int tidss_probe(struct platform_device *pdev)
 
 	spin_lock_init(&tidss->irq_lock);
 
+	/* powering up associated OLDI domains */
+	ret = tidss_attach_pm_domains(tidss);
+	if (ret < 0) {
+		dev_err(dev, "failed to attach power domains %d\n", ret);
+		goto err_detach_pm_domains;
+	}
+
 	ret = dispc_init(tidss);
 	if (ret) {
 		dev_err(dev, "failed to initialize dispc: %d\n", ret);
-		return ret;
+		goto err_detach_pm_domains;
 	}
 
 	ret = tidss_oldi_init(tidss);
-	if (ret)
-		return dev_err_probe(dev, ret, "failed to init OLDI\n");
+	if (ret) {
+		dev_dbg(dev, "failed to init OLDI: %d\n", ret);
+		goto err_oldi_deinit;
+	}
 
 	pm_runtime_enable(dev);
 
@@ -203,8 +279,12 @@ static int tidss_probe(struct platform_device *pdev)
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 
+err_oldi_deinit:
 	tidss_oldi_deinit(tidss);
 
+err_detach_pm_domains:
+	tidss_detach_pm_domains(tidss);
+
 	return ret;
 }
 
@@ -232,6 +312,8 @@ static void tidss_remove(struct platform_device *pdev)
 	/* devm allocated dispc goes away with the dev so mark it NULL */
 	dispc_remove(tidss);
 
+	tidss_detach_pm_domains(tidss);
+
 	dev_dbg(dev, "%s done\n", __func__);
 }
 
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
index e1c1f41d8b4b..6eb17cb32043 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -41,6 +41,10 @@ struct tidss_device {
 	/* protects the irq masks field and irqenable/irqstatus registers */
 	spinlock_t irq_lock;
 	dispc_irq_t irq_mask;	/* enabled irqs */
+
+	int num_domains; /* Handle attached PM domains */
+	struct device **pd_dev;
+	struct device_link **pd_link;
 };
 
 #define to_tidss(__dev) container_of(__dev, struct tidss_device, ddev)

[PATCH v2 3/3] drm: tidss: tidss_drv: Add support for AM62P display subsystem

[Swamil Jain]

The DSS controller on TI's AM62P SoC features two instances of the TI
DSS. Each DSS instance supports two video ports, similar to the DSS
controller found on the TI AM62X SoC. This allows three independent
video streams to be supported: OLDI, DPI, and DSI.

Since the DSS instances on AM62P are architecturally similar to those
on the AM62X DSS controller, the existing dispc_am625_feats
configuration can be reused for the AM62P DSS support.

This patch adds the necessary device tree compatibility entry for
"ti,am62p-dss" in the tidss driver, pointing to dispc_am625_feats,
thereby enabling DSS support on AM62P devices.

Signed-off-by: Swamil Jain <s-jain1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index 50158281715f..620d0af478f8 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -327,6 +327,7 @@ static const struct of_device_id tidss_of_table[] = {
 	{ .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,am62p-dss", .data = &dispc_am625_feats, },
 	{ .compatible = "ti,am65x-dss", .data = &dispc_am65x_feats, },
 	{ .compatible = "ti,j721e-dss", .data = &dispc_j721e_feats, },
 	{ }

Re: [PATCH v2 1/3] dt-bindings: display: ti,am65x-dss: Add am62p dss compatible

[Krzysztof Kozlowski]

On Tue, Nov 25, 2025 at 10:29:40PM +0530, Swamil Jain wrote:
> TI's AM62P SoC contains two instances of the TI Keystone Display
> SubSystem (DSS), each with two video ports and two video planes. These
> instances support up to three independent video streams through OLDI,
> DPI, and DSI interfaces.
> 
> DSS0 (first instance) supports:
>  - Two OLDI transmitters on video port 1, configurable in dual-link or
>    single-link mode.
>  - DPI output on video port 2.
> 
> DSS1 (second instance) supports:
>  - One OLDI transmitter on video port 1 (single-link mode only).
>  - DSI controller output on video port 2.
> 
> The two OLDI transmitters can be configured in clone mode to drive a
> pair of identical OLDI single-link displays. DPI outputs from
> DSS0 VP2, DSS1 VP1, and DSS1 VP2 are multiplexed, allowing only one
> DPI output at a time.
> 
> Add the compatible string "ti,am62p-dss" and update related
> description accordingly.
> 
> AM62P has different power domains for DSS and OLDI compared to other
> Keystone SoCs. Therefore, add 'minItems' and set to 1 and 'maxItems'
> field in the power-domains property to 3 for the "ti,am62p-dss"
> compatible entry to reflect this hardware difference.
> 
> Signed-off-by: Swamil Jain <s-jain1@ti.com>
> ---
>  .../bindings/display/ti/ti,am65x-dss.yaml     | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 361e9cae6896..3945ae048b8f 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -24,6 +24,19 @@ description: |
>    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.
> +  The AM62P has two instances of TI Keystone Display SubSystem, each with two
> +  video ports and two video planes. These instances can support up to 3
> +  independent video streams through OLDI, DPI, and DSI interfaces.
> +  DSS0 (first instance) supports:
> +    - Two OLDI TXes on video port 1, configurable in dual-link or
> +      single link clone mode
> +    - DPI output on video port 2
> +  DSS1 (second instance) supports:
> +    - One OLDI TX on video port 1 (single-link mode only)
> +    - DSI controller output on video port 2
> +  The two OLDI TXes can be configured in clone mode to drive a pair of
> +  identical OLDI single-link displays. DPI outputs from DSS0 VP2, DSS1 VP1,
> +  and DSS1 VP2 are muxed, allowing only one DPI output at a time.
>  
>  properties:
>    compatible:
> @@ -31,6 +44,7 @@ properties:
>        - ti,am625-dss
>        - ti,am62a7-dss
>        - ti,am62l-dss
> +      - ti,am62p-dss
>        - ti,am65x-dss
>  
>    reg:
> @@ -197,6 +211,17 @@ allOf:
>                properties:
>                  endpoint@1: false
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: ti,am62p-dss
> +    then:
> +      properties:
> +        power-domains:
> +          minItems: 1
> +          maxItems: 3

This is conflicting with top-level constraints. You need to update these
and then narrow each variants. See also writing schema (Property Schema
chapter).

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: display: ti,am65x-dss: Add am62p dss compatible

[Swamil Jain]

On 11/27/25 13:15, Krzysztof Kozlowski wrote:
> On Tue, Nov 25, 2025 at 10:29:40PM +0530, Swamil Jain wrote:
>> TI's AM62P SoC contains two instances of the TI Keystone Display
>> SubSystem (DSS), each with two video ports and two video planes. These
>> instances support up to three independent video streams through OLDI,
>> DPI, and DSI interfaces.
>>
>> DSS0 (first instance) supports:
>>   - Two OLDI transmitters on video port 1, configurable in dual-link or
>>     single-link mode.
>>   - DPI output on video port 2.
>>
>> DSS1 (second instance) supports:
>>   - One OLDI transmitter on video port 1 (single-link mode only).
>>   - DSI controller output on video port 2.
>>
>> The two OLDI transmitters can be configured in clone mode to drive a
>> pair of identical OLDI single-link displays. DPI outputs from
>> DSS0 VP2, DSS1 VP1, and DSS1 VP2 are multiplexed, allowing only one
>> DPI output at a time.
>>
>> Add the compatible string "ti,am62p-dss" and update related
>> description accordingly.
>>
>> AM62P has different power domains for DSS and OLDI compared to other
>> Keystone SoCs. Therefore, add 'minItems' and set to 1 and 'maxItems'
>> field in the power-domains property to 3 for the "ti,am62p-dss"
>> compatible entry to reflect this hardware difference.
>>
>> Signed-off-by: Swamil Jain <s-jain1@ti.com>
>> ---
>>   .../bindings/display/ti/ti,am65x-dss.yaml     | 25 +++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> index 361e9cae6896..3945ae048b8f 100644
>> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> @@ -24,6 +24,19 @@ description: |
>>     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.
>> +  The AM62P has two instances of TI Keystone Display SubSystem, each with two
>> +  video ports and two video planes. These instances can support up to 3
>> +  independent video streams through OLDI, DPI, and DSI interfaces.
>> +  DSS0 (first instance) supports:
>> +    - Two OLDI TXes on video port 1, configurable in dual-link or
>> +      single link clone mode
>> +    - DPI output on video port 2
>> +  DSS1 (second instance) supports:
>> +    - One OLDI TX on video port 1 (single-link mode only)
>> +    - DSI controller output on video port 2
>> +  The two OLDI TXes can be configured in clone mode to drive a pair of
>> +  identical OLDI single-link displays. DPI outputs from DSS0 VP2, DSS1 VP1,
>> +  and DSS1 VP2 are muxed, allowing only one DPI output at a time.
>>   
>>   properties:
>>     compatible:
>> @@ -31,6 +44,7 @@ properties:
>>         - ti,am625-dss
>>         - ti,am62a7-dss
>>         - ti,am62l-dss
>> +      - ti,am62p-dss
>>         - ti,am65x-dss
>>   
>>     reg:
>> @@ -197,6 +211,17 @@ allOf:
>>                 properties:
>>                   endpoint@1: false
>>   
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: ti,am62p-dss
>> +    then:
>> +      properties:
>> +        power-domains:
>> +          minItems: 1
>> +          maxItems: 3
> 
> This is conflicting with top-level constraints. You need to update these
> and then narrow each variants. See also writing schema (Property Schema
> chapter).

Thanks Krzysztof, will update it in the next revision.

Regards,
Swamil

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 3/3] drm: tidss: tidss_drv: Add support for AM62P display subsystem

[Tomi Valkeinen]

Hi,

On 25/11/2025 18:59, Swamil Jain wrote:
> The DSS controller on TI's AM62P SoC features two instances of the TI
> DSS. Each DSS instance supports two video ports, similar to the DSS
> controller found on the TI AM62X SoC. This allows three independent
> video streams to be supported: OLDI, DPI, and DSI.
> 
> Since the DSS instances on AM62P are architecturally similar to those
> on the AM62X DSS controller, the existing dispc_am625_feats
> configuration can be reused for the AM62P DSS support.
> 
> This patch adds the necessary device tree compatibility entry for
> "ti,am62p-dss" in the tidss driver, pointing to dispc_am625_feats,
> thereby enabling DSS support on AM62P devices.
> 
> Signed-off-by: Swamil Jain <s-jain1@ti.com>
> ---
>  drivers/gpu/drm/tidss/tidss_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index 50158281715f..620d0af478f8 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -327,6 +327,7 @@ static const struct of_device_id tidss_of_table[] = {
>  	{ .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,am62p-dss", .data = &dispc_am625_feats, },
>  	{ .compatible = "ti,am65x-dss", .data = &dispc_am65x_feats, },
>  	{ .compatible = "ti,j721e-dss", .data = &dispc_j721e_feats, },
>  	{ }

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

 Tomi

Re: [PATCH v2 2/3] drm/tidss: Power up attached PM domains on probe

[Tomi Valkeinen]

Hi,

On 25/11/2025 18:59, Swamil Jain wrote:
> From: Devarsh Thakkar <devarsht@ti.com>
> 
> Some SoC's such as AM62P have dedicated power domains
> for OLDI which need to be powered on separately along
> with display controller.
> 
> So during driver probe, power up all attached PM domains
> enumerated in devicetree node for DSS.
> 
> This also prepares base to add display support for AM62P.
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> [j-choudhary@ti.com: fix PM call sequence causing kernel crash in OLDI]
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> Signed-off-by: Swamil Jain <s-jain1@ti.com>
> ---
>  drivers/gpu/drm/tidss/tidss_drv.c | 88 +++++++++++++++++++++++++++++--
>  drivers/gpu/drm/tidss/tidss_drv.h |  4 ++
>  2 files changed, 89 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index 1c8cc18bc53c..50158281715f 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -8,6 +8,7 @@
>  #include <linux/of.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pm_domain.h>
>  #include <linux/aperture.h>
>  
>  #include <drm/clients/drm_client_setup.h>
> @@ -107,6 +108,72 @@ static const struct drm_driver tidss_driver = {
>  	.minor			= 0,
>  };
>  
> +static int tidss_detach_pm_domains(struct tidss_device *tidss)
> +{
> +	int i;
> +
> +	if (tidss->num_domains <= 1)
> +		return 0;
> +
> +	for (i = 0; i < tidss->num_domains; i++) {
> +		if (tidss->pd_link[i] && !IS_ERR(tidss->pd_link[i]))
> +			device_link_del(tidss->pd_link[i]);
> +		if (tidss->pd_dev[i] && !IS_ERR(tidss->pd_dev[i]))
> +			dev_pm_domain_detach(tidss->pd_dev[i], true);

There's IS_ERR_OR_NULL()

> +		tidss->pd_dev[i] = NULL;
> +		tidss->pd_link[i] = NULL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tidss_attach_pm_domains(struct tidss_device *tidss)
> +{
> +	struct device *dev = tidss->dev;
> +	int i;
> +	int ret;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	tidss->num_domains = of_count_phandle_with_args(np, "power-domains",
> +							"#power-domain-cells");
> +	if (tidss->num_domains <= 1) {
> +		dev_dbg(dev, "One or less power domains, no need to do attach domains\n");

I don't think this print is needed. It would be printed on almost all
platforms with DSS.

> +		return 0;
> +	}
> +
> +	tidss->pd_dev = devm_kmalloc_array(dev, tidss->num_domains,
> +					   sizeof(*tidss->pd_dev), GFP_KERNEL);
> +	if (!tidss->pd_dev)
> +		return -ENOMEM;
> +
> +	tidss->pd_link = devm_kmalloc_array(dev, tidss->num_domains,
> +					    sizeof(*tidss->pd_link), GFP_KERNEL);
> +	if (!tidss->pd_link)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < tidss->num_domains; i++) {
> +		tidss->pd_dev[i] = dev_pm_domain_attach_by_id(dev, i);
> +		if (IS_ERR(tidss->pd_dev[i])) {
> +			ret = PTR_ERR(tidss->pd_dev[i]);
> +			goto fail;
> +		}
> +
> +		tidss->pd_link[i] = device_link_add(dev, tidss->pd_dev[i],
> +						    DL_FLAG_STATELESS |
> +						    DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
> +		if (!tidss->pd_link[i]) {
> +			ret = -EINVAL;
> +			goto fail;
> +		}
> +	}
> +
> +	return 0;
> +fail:
> +	tidss_detach_pm_domains(tidss);
> +	return ret;
> +}
> +
>  static int tidss_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -129,15 +196,24 @@ static int tidss_probe(struct platform_device *pdev)
>  
>  	spin_lock_init(&tidss->irq_lock);
>  
> +	/* powering up associated OLDI domains */

I think the function name is self-explanatory, no comment needed.

> +	ret = tidss_attach_pm_domains(tidss);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to attach power domains %d\n", ret);
> +		goto err_detach_pm_domains;

This is not correct. If tidss_attach_pm_domains() fails, it should do
its own cleanup, thus there's no need to goto err_detach_pm_domains.

> +	}
> +
>  	ret = dispc_init(tidss);
>  	if (ret) {
>  		dev_err(dev, "failed to initialize dispc: %d\n", ret);
> -		return ret;
> +		goto err_detach_pm_domains;
>  	}
>  
>  	ret = tidss_oldi_init(tidss);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "failed to init OLDI\n");
> +	if (ret) {
> +		dev_dbg(dev, "failed to init OLDI: %d\n", ret);
> +		goto err_oldi_deinit;

Same here, this is just not correct. Please go through the error
handling paths with your patch. This also changes dev_err_probe() to
dev_dbg().

> +	}
>  
>  	pm_runtime_enable(dev);
>  
> @@ -203,8 +279,12 @@ static int tidss_probe(struct platform_device *pdev)
>  	pm_runtime_dont_use_autosuspend(dev);
>  	pm_runtime_disable(dev);
>  
> +err_oldi_deinit:
>  	tidss_oldi_deinit(tidss);
>  
> +err_detach_pm_domains:
> +	tidss_detach_pm_domains(tidss);
> +
>  	return ret;
>  }
>  
> @@ -232,6 +312,8 @@ static void tidss_remove(struct platform_device *pdev)
>  	/* devm allocated dispc goes away with the dev so mark it NULL */
>  	dispc_remove(tidss);
>  
> +	tidss_detach_pm_domains(tidss);
> +
>  	dev_dbg(dev, "%s done\n", __func__);
>  }
>  
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
> index e1c1f41d8b4b..6eb17cb32043 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.h
> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
> @@ -41,6 +41,10 @@ struct tidss_device {
>  	/* protects the irq masks field and irqenable/irqstatus registers */
>  	spinlock_t irq_lock;
>  	dispc_irq_t irq_mask;	/* enabled irqs */
> +
> +	int num_domains; /* Handle attached PM domains */

What does the comment mean?

 Tomi

> +	struct device **pd_dev;
> +	struct device_link **pd_link;
>  };
>  
>  #define to_tidss(__dev) container_of(__dev, struct tidss_device, ddev)

Re: [PATCH v2 2/3] drm/tidss: Power up attached PM domains on probe

[Swamil Jain]

Hi Tomi,
Thanks for the feedback.

On 12/1/25 18:10, Tomi Valkeinen wrote:
> Hi,
> 
> On 25/11/2025 18:59, Swamil Jain wrote:
>> From: Devarsh Thakkar <devarsht@ti.com>
>>
>> Some SoC's such as AM62P have dedicated power domains
>> for OLDI which need to be powered on separately along
>> with display controller.
>>
>> So during driver probe, power up all attached PM domains
>> enumerated in devicetree node for DSS.
>>
>> This also prepares base to add display support for AM62P.
>>
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> [j-choudhary@ti.com: fix PM call sequence causing kernel crash in OLDI]
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> Signed-off-by: Swamil Jain <s-jain1@ti.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_drv.c | 88 +++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/tidss/tidss_drv.h |  4 ++
>>   2 files changed, 89 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
>> index 1c8cc18bc53c..50158281715f 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.c
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
>> @@ -8,6 +8,7 @@
>>   #include <linux/of.h>
>>   #include <linux/module.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/pm_domain.h>
>>   #include <linux/aperture.h>
>>   
>>   #include <drm/clients/drm_client_setup.h>
>> @@ -107,6 +108,72 @@ static const struct drm_driver tidss_driver = {
>>   	.minor			= 0,
>>   };
>>   
>> +static int tidss_detach_pm_domains(struct tidss_device *tidss)
>> +{
>> +	int i;
>> +
>> +	if (tidss->num_domains <= 1)
>> +		return 0;
>> +
>> +	for (i = 0; i < tidss->num_domains; i++) {
>> +		if (tidss->pd_link[i] && !IS_ERR(tidss->pd_link[i]))
>> +			device_link_del(tidss->pd_link[i]);
>> +		if (tidss->pd_dev[i] && !IS_ERR(tidss->pd_dev[i]))
>> +			dev_pm_domain_detach(tidss->pd_dev[i], true);
> 
> There's IS_ERR_OR_NULL()

Ack, will replace with IS_ERR_OR_NULL.

> 
>> +		tidss->pd_dev[i] = NULL;
>> +		tidss->pd_link[i] = NULL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int tidss_attach_pm_domains(struct tidss_device *tidss)
>> +{
>> +	struct device *dev = tidss->dev;
>> +	int i;
>> +	int ret;
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct device_node *np = pdev->dev.of_node;
>> +
>> +	tidss->num_domains = of_count_phandle_with_args(np, "power-domains",
>> +							"#power-domain-cells");
>> +	if (tidss->num_domains <= 1) {
>> +		dev_dbg(dev, "One or less power domains, no need to do attach domains\n");
> 
> I don't think this print is needed. It would be printed on almost all
> platforms with DSS.

Yeah, true, will remove this in the next version.

> 
>> +		return 0;
>> +	}
>> +
>> +	tidss->pd_dev = devm_kmalloc_array(dev, tidss->num_domains,
>> +					   sizeof(*tidss->pd_dev), GFP_KERNEL);
>> +	if (!tidss->pd_dev)
>> +		return -ENOMEM;
>> +
>> +	tidss->pd_link = devm_kmalloc_array(dev, tidss->num_domains,
>> +					    sizeof(*tidss->pd_link), GFP_KERNEL);
>> +	if (!tidss->pd_link)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < tidss->num_domains; i++) {
>> +		tidss->pd_dev[i] = dev_pm_domain_attach_by_id(dev, i);
>> +		if (IS_ERR(tidss->pd_dev[i])) {
>> +			ret = PTR_ERR(tidss->pd_dev[i]);
>> +			goto fail;
>> +		}
>> +
>> +		tidss->pd_link[i] = device_link_add(dev, tidss->pd_dev[i],
>> +						    DL_FLAG_STATELESS |
>> +						    DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
>> +		if (!tidss->pd_link[i]) {
>> +			ret = -EINVAL;
>> +			goto fail;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +fail:
>> +	tidss_detach_pm_domains(tidss);
>> +	return ret;
>> +}
>> +
>>   static int tidss_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> @@ -129,15 +196,24 @@ static int tidss_probe(struct platform_device *pdev)
>>   
>>   	spin_lock_init(&tidss->irq_lock);
>>   
>> +	/* powering up associated OLDI domains */
> 
> I think the function name is self-explanatory, no comment needed.

Will remove the comment.

> 
>> +	ret = tidss_attach_pm_domains(tidss);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to attach power domains %d\n", ret);
>> +		goto err_detach_pm_domains;
> 
> This is not correct. If tidss_attach_pm_domains() fails, it should do
> its own cleanup, thus there's no need to goto err_detach_pm_domains.
>

Ok, will make tidss_attach_pm_domains do it's own cleanup.


>> +	}
>> +
>>   	ret = dispc_init(tidss);
>>   	if (ret) {
>>   		dev_err(dev, "failed to initialize dispc: %d\n", ret);
>> -		return ret;
>> +		goto err_detach_pm_domains;
>>   	}
>>   
>>   	ret = tidss_oldi_init(tidss);
>> -	if (ret)
>> -		return dev_err_probe(dev, ret, "failed to init OLDI\n");
>> +	if (ret) {
>> +		dev_dbg(dev, "failed to init OLDI: %d\n", ret);
>> +		goto err_oldi_deinit;
> 
> Same here, this is just not correct. Please go through the error
> handling paths with your patch. This also changes dev_err_probe() to
> dev_dbg().

Sure.

> 
>> +	}
>>   
>>   	pm_runtime_enable(dev);
>>   
>> @@ -203,8 +279,12 @@ static int tidss_probe(struct platform_device *pdev)
>>   	pm_runtime_dont_use_autosuspend(dev);
>>   	pm_runtime_disable(dev);
>>   
>> +err_oldi_deinit:
>>   	tidss_oldi_deinit(tidss);
>>   
>> +err_detach_pm_domains:
>> +	tidss_detach_pm_domains(tidss);
>> +
>>   	return ret;
>>   }
>>   
>> @@ -232,6 +312,8 @@ static void tidss_remove(struct platform_device *pdev)
>>   	/* devm allocated dispc goes away with the dev so mark it NULL */
>>   	dispc_remove(tidss);
>>   
>> +	tidss_detach_pm_domains(tidss);
>> +
>>   	dev_dbg(dev, "%s done\n", __func__);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
>> index e1c1f41d8b4b..6eb17cb32043 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.h
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
>> @@ -41,6 +41,10 @@ struct tidss_device {
>>   	/* protects the irq masks field and irqenable/irqstatus registers */
>>   	spinlock_t irq_lock;
>>   	dispc_irq_t irq_mask;	/* enabled irqs */
>> +
>> +	int num_domains; /* Handle attached PM domains */
> 
> What does the comment mean?

The number of power domains that needs to be handled, will rephrase it
to make it more understandable.

> 
>   Tomi
> 
>> +	struct device **pd_dev;
>> +	struct device_link **pd_link;
>>   };
>>   
>>   #define to_tidss(__dev) container_of(__dev, struct tidss_device, ddev)
>