Re: [PATCH v5 2/2] cpufreq: airoha: Add EN7581 CPUFreq SMCCC driver

[Christian Marangi]

On Wed, Dec 04, 2024 at 11:02:11AM +0530, Viresh Kumar wrote:
> On 03-12-24, 17:31, Christian Marangi wrote:
> > diff --git a/drivers/cpufreq/airoha-cpufreq.c b/drivers/cpufreq/airoha-cpufreq.c
> > +struct airoha_cpufreq_priv {
> > +	struct clk_hw hw;
> > +	struct generic_pm_domain pd;
> > +
> > +	int opp_token;
> > +	struct dev_pm_domain_list *pd_list;
> > +	struct platform_device *cpufreq_dt;
> > +};
> > +
> > +static long airoha_cpufreq_clk_round(struct clk_hw *hw, unsigned long rate,
> > +				     unsigned long *parent_rate)
> > +{
> > +	return rate;
> > +}
> > +
> > +static unsigned long airoha_cpufreq_clk_get(struct clk_hw *hw,
> > +					    unsigned long parent_rate)
> > +{
> > +	const struct arm_smccc_1_2_regs args = {
> > +		.a0 = AIROHA_SIP_AVS_HANDLE,
> > +		.a1 = AIROHA_AVS_OP_GET_FREQ,
> > +	};
> > +	struct arm_smccc_1_2_regs res;
> > +
> > +	arm_smccc_1_2_smc(&args, &res);
> > +
> > +	/* SMCCC returns freq in MHz */
> > +	return (int)(res.a0 * 1000 * 1000);
> 
> Why casting to "int" when we can return ulong ?
>

Leftover from old. Yes will drop. Coincidentally arm_smccc_1_2_regs
entry are already ulong.

> > +}
> > +
> > +/* Airoha CPU clk SMCC is always enabled */
> > +static int airoha_cpufreq_clk_is_enabled(struct clk_hw *hw)
> > +{
> > +	return true;
> > +}
> > +
> > +static const struct clk_ops airoha_cpufreq_clk_ops = {
> > +	.recalc_rate = airoha_cpufreq_clk_get,
> > +	.is_enabled = airoha_cpufreq_clk_is_enabled,
> > +	.round_rate = airoha_cpufreq_clk_round,
> > +};
> > +
> > +static const char * const airoha_cpufreq_clk_names[] = { "cpu", NULL };
> > +
> > +/* NOP function to disable OPP from setting clock */
> > +static int airoha_cpufreq_config_clks_nop(struct device *dev,
> > +					  struct opp_table *opp_table,
> > +					  struct dev_pm_opp *opp,
> > +					  void *data, bool scaling_down)
> > +{
> > +	return 0;
> > +}
> 
> I wonder whats better here. Provide this helper or provide a dummy clk-set-rate
> at the provider itself ?
>

The idea I prefer this is to save a few CPU cycle and also to prevent
bad usage of the CLK if anyone starts to use it. Returning 0 from a set_rate
would provide bad information. Or your idea was to declare a set_rate
and always return an error like -EINVAL? 

-- 
	Ansuel