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

[Devarsh Thakkar <devarsht@ti.com>]

Hi Maxime,

On 09/10/23 16:40, Devarsh Thakkar wrote:
> Hi Maxime,
> 
> Thanks for the review.
> 
> On 09/10/23 14:53, Maxime Ripard wrote:
>> Hi Devarsh,
>>
>> On Mon, Oct 09, 2023 at 01:20:18PM +0530, Devarsh Thakkar wrote:
>>> Some SoC's such as AM62P have dedicated power domains
>>> for OLDI which need to be powered on separetely 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>
>>> ---
>>>   drivers/gpu/drm/tidss/tidss_drv.c | 76 +++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/tidss/tidss_drv.h |  5 ++
>>>   2 files changed, 81 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
>>> index 4d063eb9cd0b..a703a27d17bf 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 <drm/drm_atomic.h>
>>>   #include <drm/drm_atomic_helper.h>
>>> @@ -114,6 +115,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;
>>> +}
>>
>> My understanding is that this will be done automatically at probe time.
>> Why do we need to roll our own there? A comment on top of the function
>> and the commit log would help.
> 
> By default, the TI SCI power domain controller driver only powers up one power
> domain associated with device, With AM62P, we now have separate power domains
> for OLDI Tx ports (for more efficient power-saving control) which is different
> from core DSS device power domain, so this patch powers on the associated
> power domains too if enumerated in device-tree.
> 

My bad, I think it is not the ti sci power domain controller driver but 
the kernel core itself which seems to have a check to only allow one 
power domain per device (thanks to Vignesh for pointing out) :

	/*
	 * Devices with multiple PM domains must be attached separately,
	 * as we can only attach one PM domain per device.
	 */
	if (of_count_phandle_with_args(dev->of_node, "power-domains",
				       "#power-domain-cells") != 1)
		return 0;

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/base/power/domain.c?h=next-20231012#n2828

But anyways, I talked to team internally and there are some plans to 
have separate OLDI bridge driver which would inherit these new power 
domains, so I guess we may not need this patch at all.

Regards
Devarsh


> Regards
> Devarsh
> 
>>
>> Thanks!
>> Maxime