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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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)