Re: [PATCH RFC 09/10] drm/panthor: devfreq: add pluggable devfreq providers

[Steven Price <steven.price@arm.com>]

On 05/09/2025 11:23, Nicolas Frattaroli wrote:
> On some devices, devfreq is not controlled directly by DT OPPs and the
> common clock framework, but through an external devfreq driver. To
> permit this type of usage, add the concept of devfreq providers.
> 
> Devfreq providers for panthor register themselves with panthor as a
> provider. panthor then gets whatever driver is suckling on its
> performance-node property, finds the registered devfreq provider init
> function for it, and calls it.
> 
> Should the probe order work out such that panthor probes before the
> devfreq provider is finished probing and registering itself, then we
> just defer the probe after adding a device link.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Looks fine to me, but I think there are some updates needed due to the
DT review.

Thanks,
Steve

> ---
>  drivers/gpu/drm/panthor/panthor_devfreq.c | 74 ++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/panthor/panthor_devfreq.h | 16 +++++++
>  2 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
> index d495dd856299826c4bddc205087d8914d01d7fc4..1b0c21f6f069b059b8b0e412a79556c602c5f1e7 100644
> --- a/drivers/gpu/drm/panthor/panthor_devfreq.c
> +++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
> @@ -4,6 +4,7 @@
>  #include <linux/clk.h>
>  #include <linux/devfreq.h>
>  #include <linux/devfreq_cooling.h>
> +#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_opp.h>
>  
> @@ -12,6 +13,34 @@
>  #include "panthor_devfreq.h"
>  #include "panthor_device.h"
>  
> +static LIST_HEAD(panthor_devfreq_providers);
> +static DEFINE_MUTEX(panthor_devfreq_providers_lock);
> +
> +int panthor_devfreq_register_provider(struct panthor_devfreq_provider *prov)
> +{
> +	guard(mutex)(&panthor_devfreq_providers_lock);
> +
> +	list_add(&prov->node, &panthor_devfreq_providers);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(panthor_devfreq_register_provider);
> +
> +static int panthor_devfreq_init_provider(struct panthor_device *ptdev,
> +					 struct device *provider_dev)
> +{
> +	struct panthor_devfreq_provider *prov;
> +
> +	guard(mutex)(&panthor_devfreq_providers_lock);
> +
> +	list_for_each_entry(prov, &panthor_devfreq_providers, node) {
> +		if (prov->dev == provider_dev)
> +			return prov->init(ptdev, provider_dev);
> +	}
> +
> +	return -EPROBE_DEFER;
> +}
> +
>  static void panthor_devfreq_update_utilization(struct panthor_devfreq *pdevfreq)
>  {
>  	ktime_t now, last;
> @@ -102,7 +131,7 @@ static struct devfreq_dev_profile panthor_devfreq_profile = {
>  	.get_cur_freq = panthor_devfreq_get_cur_freq,
>  };
>  
> -int panthor_devfreq_init(struct panthor_device *ptdev)
> +static int panthor_devfreq_init_of(struct panthor_device *ptdev)
>  {
>  	/* There's actually 2 regulators (mali and sram), but the OPP core only
>  	 * supports one.
> @@ -222,6 +251,49 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
>  	return 0;
>  }
>  
> +static int panthor_devfreq_init_platform(struct panthor_device *ptdev)
> +{
> +	struct device_node *pcnode;
> +	struct platform_device *pdev;
> +	struct device_link *link;
> +	int ret;
> +
> +	pcnode = of_parse_phandle(ptdev->base.dev->of_node,
> +				  "performance-controller", 0);
> +	if (!pcnode)
> +		return -EINVAL;
> +
> +	pdev = of_find_device_by_node(pcnode);
> +	of_node_put(pcnode);
> +	if (!pdev)
> +		return -ENODEV;
> +
> +	link = device_link_add(ptdev->base.dev, &pdev->dev,
> +			       DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
> +	if (!link) {
> +		dev_err(ptdev->base.dev, "failed to add device link\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = panthor_devfreq_init_provider(ptdev, &pdev->dev);
> +	if (ret)
> +		return dev_err_probe(ptdev->base.dev, ret,
> +				     "failed to initialize devfreq provider\n");
> +
> +	DRM_DEV_INFO(ptdev->base.dev, "initialized devfreq provider %s\n",
> +		     dev_name(&pdev->dev));
> +
> +	return 0;
> +}
> +
> +int panthor_devfreq_init(struct panthor_device *ptdev)
> +{
> +	if (!of_property_present(ptdev->base.dev->of_node, "performance-controller"))
> +		return panthor_devfreq_init_of(ptdev);
> +
> +	return panthor_devfreq_init_platform(ptdev);
> +}
> +
>  void panthor_devfreq_resume(struct panthor_device *ptdev)
>  {
>  	struct panthor_devfreq *pdevfreq = ptdev->devfreq;
> diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.h b/drivers/gpu/drm/panthor/panthor_devfreq.h
> index a891cb5fdc34636444f141e10f5d45828fc35b51..94c9768d5d038c4ba8516929edb565a1f13443fb 100644
> --- a/drivers/gpu/drm/panthor/panthor_devfreq.h
> +++ b/drivers/gpu/drm/panthor/panthor_devfreq.h
> @@ -8,6 +8,7 @@
>  
>  struct devfreq;
>  struct thermal_cooling_device;
> +struct platform_device;
>  
>  struct panthor_device;
>  
> @@ -43,6 +44,19 @@ struct panthor_devfreq {
>  	spinlock_t lock;
>  };
>  
> +struct panthor_devfreq_provider {
> +	/** @dev: device pointer to the provider device */
> +	struct device *dev;
> +	/**
> +	 * @init: the provider's init callback that allocates a
> +	 * &struct panthor_devfreq, adds it to panthor, and adds a devfreq
> +	 * device to panthor. Will be called during panthor's probe.
> +	 */
> +	int (*init)(struct panthor_device *ptdev, struct device *dev);
> +
> +	struct list_head node;
> +};
> +
>  
>  int panthor_devfreq_init(struct panthor_device *ptdev);
>  
> @@ -57,4 +71,6 @@ int panthor_devfreq_get_dev_status(struct device *dev,
>  
>  unsigned long panthor_devfreq_get_freq(struct panthor_device *ptdev);
>  
> +int panthor_devfreq_register_provider(struct panthor_devfreq_provider *prov);
> +
>  #endif /* __PANTHOR_DEVFREQ_H__ */
>