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

[Aradhya Bhatia <aradhya.bhatia@linux.dev>]

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.

-- 
Regards
Aradhya