Re: [PATCH RFC net-next] net: airoha: npu: attach passive devfreq scaling

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4421 bytes --]

On Nov 04, Christian Marangi wrote:
> The NPU can scale to different operating state from idle to normal to
> turbo. Attach a passive devfreq that will scale the NPU frequency with the
> CPU with the use of OPP table and opp-required properties.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
> 
> Posted as RFC to understand if it's the correct way to handle this...
> 
> DT patch will come later.
> 
>  drivers/net/ethernet/airoha/airoha_npu.c  | 61 ++++++++++++++++++++++-
>  include/linux/soc/airoha/airoha_offload.h |  3 ++
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_npu.c b/drivers/net/ethernet/airoha/airoha_npu.c
> index 4d3195eb00f7..f8055ca65ec4 100644
> --- a/drivers/net/ethernet/airoha/airoha_npu.c
> +++ b/drivers/net/ethernet/airoha/airoha_npu.c
> @@ -12,6 +12,8 @@
>  #include <linux/of_platform.h>
>  #include <linux/of_reserved_mem.h>
>  #include <linux/regmap.h>
> +#include <linux/pm_opp.h>
> +#include <linux/clk.h>
>  
>  #include "airoha_eth.h"
>  
> @@ -656,6 +658,62 @@ void airoha_npu_put(struct airoha_npu *npu)
>  }
>  EXPORT_SYMBOL_GPL(airoha_npu_put);
>  
> +static int airoha_npu_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
> +{
> +	struct airoha_npu *npu = dev_get_drvdata(dev);
> +
> +	*freq = clk_get_rate(npu->clk);
> +
> +	return 0;
> +}
> +
> +static int airoha_npu_devfreq_target(struct device *dev, unsigned long *freq,
> +				     u32 flags)
> +{
> +	struct airoha_npu *npu = dev_get_drvdata(dev);
> +
> +	return clk_set_rate(npu->clk, *freq);
> +}
> +
> +static int airoha_npu_devfreq_get_dev_status(struct device *dev,
> +					     struct devfreq_dev_status *stat)
> +{
> +	struct airoha_npu *npu = dev_get_drvdata(dev);
> +
> +	stat->busy_time = 0;
> +	stat->total_time = 0;
> +	stat->current_frequency = clk_get_rate(npu->clk);
> +
> +	return 0;
> +}
> +
> +static struct devfreq_dev_profile airoha_npu_devfreq_profile = {
> +	.target = airoha_npu_devfreq_target,
> +	.get_dev_status = airoha_npu_devfreq_get_dev_status,
> +	.get_cur_freq = airoha_npu_devfreq_get_cur_freq
> +};
> +
> +static int airoha_npu_devfreq_probe(struct device *dev)
> +{
> +	struct airoha_npu *npu = dev_get_drvdata(dev);
> +	struct devfreq *devfreq;
> +	int ret;
> +
> +	npu->clk = devm_clk_get_enabled(dev, NULL);

I guess this should be devm_clk_get_optional_enabled() otherwise this patch
will break backward compatibility. Moreover, we should bail out early if
npu->clk is NULL.

> +	if (IS_ERR(npu->clk))
> +		return dev_err_probe(dev, PTR_ERR(npu->clk), "failed to get clk\n");
> +
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to parse NPU freq thresholds\n");
> +
> +	npu->gov_data.parent_type = CPUFREQ_PARENT_DEV;
> +	devfreq = devm_devfreq_add_device(dev, &airoha_npu_devfreq_profile,
> +					  DEVFREQ_GOV_PASSIVE, &npu->gov_data);
> +
> +	return PTR_ERR_OR_ZERO(devfreq);
> +}
> +
>  static const struct airoha_npu_soc_data en7581_npu_soc_data = {
>  	.fw_rv32 = {
>  		.name = NPU_EN7581_FIRMWARE_RV32,
> @@ -803,8 +861,9 @@ static int airoha_npu_probe(struct platform_device *pdev)
>  			 (val >> 16) & 0xffff, val & 0xffff);
>  
>  	platform_set_drvdata(pdev, npu);
> +	dev_set_drvdata(dev, npu);

why do we need that one? I guess this is exactly what platform_set_drvdata() is
doing, right?

Regards,
Lorenzo

>  
> -	return 0;
> +	return airoha_npu_devfreq_probe(dev);
>  }
>  
>  static struct platform_driver airoha_npu_driver = {
> diff --git a/include/linux/soc/airoha/airoha_offload.h b/include/linux/soc/airoha/airoha_offload.h
> index 7589fccfeef6..ac776dc31487 100644
> --- a/include/linux/soc/airoha/airoha_offload.h
> +++ b/include/linux/soc/airoha/airoha_offload.h
> @@ -6,6 +6,7 @@
>  #ifndef AIROHA_OFFLOAD_H
>  #define AIROHA_OFFLOAD_H
>  
> +#include <linux/devfreq.h>
>  #include <linux/skbuff.h>
>  #include <linux/spinlock.h>
>  #include <linux/workqueue.h>
> @@ -167,6 +168,8 @@ struct airoha_npu {
>  #if (IS_BUILTIN(CONFIG_NET_AIROHA_NPU) || IS_MODULE(CONFIG_NET_AIROHA_NPU))
>  	struct device *dev;
>  	struct regmap *regmap;
> +	struct clk *clk;
> +	struct devfreq_passive_data gov_data;
>  
>  	struct airoha_npu_core {
>  		struct airoha_npu *npu;
> -- 
> 2.55.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]