Re: [PATCH v4 11/14] drm/tidss: oldi: Add power domain support

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

Hi,

On 02/11/2026 15:45, Aradhya Bhatia wrote:
> Hi Tomi,
> 
> On 29/10/26 10:18, Tomi Valkeinen wrote:
>> The OLDI TXes on AM62P and J722S have power domains of their own,
>> described in the DT, unlike the earlier SoCs. The OLDI TX nodes are not
>> real devices, so the driver core does not attach these power domains,
>> and nothing in the core turns them on.
>>
>> Add support to attach the PDs with dev_pm_domain_attach_by_node(), and
>> manage the power state of the returned virtual devices with runtime PM
>> functions.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_oldi.c | 88 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 88 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.c b/drivers/gpu/drm/tidss/tidss_oldi.c
>> index 5552c1c51074..df2326db3302 100644
>> --- a/drivers/gpu/drm/tidss/tidss_oldi.c
>> +++ b/drivers/gpu/drm/tidss/tidss_oldi.c
>> @@ -10,6 +10,8 @@
>>   #include <linux/of_graph.h>
>>   #include <linux/mfd/syscon.h>
>>   #include <linux/media-bus-format.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/pm_runtime.h>
>>   #include <linux/regmap.h>
>>   
>>   #include <drm/drm_atomic_helper.h>
>> @@ -35,6 +37,12 @@ struct tidss_oldi {
>>   
>>   	struct clk *serial;
>>   	struct regmap *io_ctrl;
>> +
>> +	/*
>> +	 * The power domains of this OLDI TX and, in dual-link mode, of its
>> +	 * companion. NULL for an OLDI TX without a power domain of its own.
>> +	 */
>> +	struct device *pd_devs[2];
>>   };
>>   
>>   struct oldi_bus_format {
>> @@ -238,6 +246,22 @@ static int tidss_oldi_atomic_check(struct drm_bridge *bridge,
>>   	return 0;
>>   }
>>   
>> +static void tidss_oldi_power_get(struct tidss_oldi *oldi)
>> +{
>> +	for (unsigned int i = 0; i < ARRAY_SIZE(oldi->pd_devs); i++) {
>> +		if (oldi->pd_devs[i])
>> +			WARN_ON(pm_runtime_get_sync(oldi->pd_devs[i]) < 0);
>> +	}
>> +}
>> +
>> +static void tidss_oldi_power_put(struct tidss_oldi *oldi)
>> +{
>> +	for (unsigned int i = 0; i < ARRAY_SIZE(oldi->pd_devs); i++) {
>> +		if (oldi->pd_devs[i])
>> +			pm_runtime_put(oldi->pd_devs[i]);
>> +	}
>> +}
>> +
>>   static void tidss_oldi_atomic_pre_enable(struct drm_bridge *bridge,
>>   					 struct drm_atomic_commit *state)
>>   {
>> @@ -251,6 +275,8 @@ static void tidss_oldi_atomic_pre_enable(struct drm_bridge *bridge,
>>   	if (oldi->link_type == OLDI_MODE_SECONDARY_CLONE_SINGLE_LINK)
>>   		return;
>>   
>> +	tidss_oldi_power_get(oldi);
>> +
>>   	connector = drm_atomic_get_new_connector_for_encoder(state,
>>   							     bridge->encoder);
>>   	if (WARN_ON(!connector))
>> @@ -296,6 +322,8 @@ static void tidss_oldi_atomic_post_disable(struct drm_bridge *bridge,
>>   
>>   	/* Clear OLDI Config */
>>   	tidss_disable_oldi(oldi->tidss, oldi->parent_vp);
>> +
>> +	tidss_oldi_power_put(oldi);
>>   }
>>   
>>   #define MAX_INPUT_SEL_FORMATS	1
>> @@ -468,6 +496,60 @@ static int get_parent_dss_vp(struct device_node *oldi_tx, u32 *parent_vp)
>>   	return -ENODEV;
>>   }
>>   
>> +static void tidss_oldi_detach_pd(void *data)
>> +{
>> +	struct device *pd_dev = data;
>> +
>> +	dev_pm_domain_detach(pd_dev, true);
>> +}
>> +
>> +static struct device *tidss_oldi_create_pd_dev(struct tidss_device *tidss,
>> +					       struct device_node *oldi_tx)
>> +{
>> +	struct device *dev;
>> +	int ret;
>> +
>> +	dev = dev_pm_domain_attach_by_node(tidss->dev, oldi_tx, 0);
>> +	if (IS_ERR_OR_NULL(dev))
>> +		return dev;
>> +
>> +	ret = devm_add_action_or_reset(tidss->dev, tidss_oldi_detach_pd, dev);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	return dev;
>> +}
>> +
>> +static int tidss_oldi_create_pd_devs(struct tidss_device *tidss,
>> +				     struct tidss_oldi *oldi,
>> +				     struct device_node *oldi_tx)
>> +{
>> +	struct device_node *companion;
>> +	struct device *pd_dev;
>> +
>> +	pd_dev = tidss_oldi_create_pd_dev(tidss, oldi_tx);
>> +	if (IS_ERR(pd_dev))
>> +		return PTR_ERR(pd_dev);
>> +
>> +	oldi->pd_devs[0] = pd_dev;
>> +
>> +	if (oldi->companion_instance < 0)
>> +		return 0;
>> +
>> +	companion = of_parse_phandle(oldi_tx, "ti,companion-oldi", 0);
>> +	if (!companion)
>> +		return -EINVAL;
>> +
>> +	pd_dev = tidss_oldi_create_pd_dev(tidss, companion);
>> +	of_node_put(companion);
>> +	if (IS_ERR(pd_dev))
>> +		return PTR_ERR(pd_dev);
>> +
>> +	oldi->pd_devs[1] = pd_dev;
>> +
>> +	return 0;
>> +}
>> +
>>   void tidss_oldi_deinit(struct tidss_device *tidss)
>>   {
>>   	for (int i = 0; i < tidss->num_oldis; i++) {
>> @@ -610,6 +692,12 @@ static int tidss_oldi_init_one(struct tidss_device *tidss,
>>   		}
>>   	}
>>   
>> +	ret = tidss_oldi_create_pd_devs(tidss, oldi, oldi_tx);
>> +	if (ret)
>> +		return dev_err_probe(tidss->dev, ret,
>> +				     "OLDI%u: Failed to set up the power domains.\n",
>> +				     oldi_instance);
>> +
> 
> I agree with the what this patch is trying to do, but the
> tidss_oldi_create_pd_devs() should be within the if() block above.
> 
> It seems like you are giving the pd_dev of the secondary OLDI TX to the
> primary, but this path would be executed for secondary OLDI TX in clone
> mode as well, whenever it gets supported.
> Which is not ideal, because the primary OLDI TX in clone mode would
> already be taking care of the secondary's pd_dev.
> 
> The conditional block above with,
> `if (link_type != OLDI_MODE_SECONDARY_CLONE_SINGLE_LINK)`, is exactly
> for things that _can_ be setup per OLDI Tx, but shouldn't, like the
> clock and the IO control.

Yes, you're right, I'll move it.

  Tomi