Re: [PATCH v5 2/9] cpufreq: mediatek: Add opp notification support - Rex-BC Chen

From: Rex-BC Chen <rex-bc.chen@mediatek.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: <rafael@kernel.org>, <robh+dt@kernel.org>, <krzk+dt@kernel.org>,
	<matthias.bgg@gmail.com>, <jia-wei.chang@mediatek.com>,
	<roger.lu@mediatek.com>, <hsinyi@google.com>,
	<khilman@baylibre.com>, <angelogioacchino.delregno@collabora.com>,
	<linux-pm@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-mediatek@lists.infradead.org>,
	 <Project_Global_Chrome_Upstream_Group@mediatek.com>,
	Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
Subject: Re: [PATCH v5 2/9] cpufreq: mediatek: Add opp notification support
Date: Thu, 5 May 2022 17:44:17 +0800	[thread overview]
Message-ID: <b2dabd5e33e39fed2efcf458c26670f29ab83233.camel@mediatek.com> (raw)
In-Reply-To: <20220505084345.e3qt3ocdft75tbxv@vireshk-i7>

On Thu, 2022-05-05 at 14:13 +0530, Viresh Kumar wrote:
> On 04-05-22, 21:05, Rex-BC Chen wrote:
> > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
> > 
> > > From this opp notifier, cpufreq should listen to opp notification
> > > and do
> 
> What happened with the extra ">" here ?
> 

Hello Viresh,

Sorry for this. I don't know why it appear this ">".
In my patch, there is no ">" here.
I will check this..

> >  static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info,
> > int cpu)
> >  {
> >  	struct device *cpu_dev;
> > @@ -396,6 +458,17 @@ static int mtk_cpu_dvfs_info_init(struct
> > mtk_cpu_dvfs_info *info, int cpu)
> >  	info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
> >  	dev_pm_opp_put(opp);
> >  
> > +	info->opp_cpu = cpu;
> > +	info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier;
> > +	ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb);
> > +	if (ret) {
> > +		dev_err(cpu_dev, "cpu%d: failed to register opp
> > notifier\n", cpu);
> > +		goto out_disable_inter_clock;
> > +	}
> > +
> > +	mutex_init(&info->reg_lock);
> 
> You should always initialize a resource before its users. The
> notifier
> callback, which can get called right after
> dev_pm_opp_register_notifier() returns, will use this mutex.
> 

ok, I will move mutex_init(&info->reg_lock) before
dev_pm_opp_register_notifier().

> > +	info->opp_freq = clk_get_rate(info->cpu_clk);
> > +
> >  	/*
> >  	 * If SRAM regulator is present, software "voltage tracking" is
> > needed
> >  	 * for this CPU power domain.
> > @@ -451,6 +524,9 @@ static void mtk_cpu_dvfs_info_release(struct
> > mtk_cpu_dvfs_info *info)
> >  	}
> >  
> >  	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
> > +
> > +	if (!IS_ERR_OR_NULL(info->cpu_dev))
> 
> cpu_dev can never be error here.

ok, I will drop this.

> 
> > +		dev_pm_opp_unregister_notifier(info->cpu_dev, &info-
> > >opp_nb);
> >  }
> >  
> >  static int mtk_cpufreq_init(struct cpufreq_policy *policy)
> 
> I also asked you last time to stack things in a order so they are
> easier for me to apply. Bugfixes, followed by simple cleanups, which
> don't make behavioral changes, followed by real patches.
> 
> Now you have sent this patch at an early stage, which blocks me from
> applying anything after this.
> 
> I can see the earlier comments weren't all considered, and it doesn't
> look nice.
> 

Sorry for inconvenience.
I will move "cpufreq: mediatek: Move voltage limits to platform data"
before this patch.

The order will be:
//cleanup and bug fix
new patch for add "platform_device_unregister(cpufreq_pdev);"
cpufreq: mediatek: Move voltage limits to platform data

//driver refinement
cpufreq: mediatek: Refine mtk_cpufreq_voltage_tracking()

//new feature
cpufreq: mediatek: Add opp notification support
cpufreq: mediatek: Link CCI device to CPU

//support new soc
cpufreq: mediatek: Add support for MT8186

//dts
arm64: dts: mediatek: Add opp table and clock property for MT8183
cpufreq
arm64: dts: mediatek: Add MediaTek CCI node for MT8183
arm64: dts: mediatek: Add mediatek, cci property for MT8183 cpufreq

BRs,
Rex

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek