linux-kernel - RE: [PATCH V2 2/2] clk: imx: scu: add cpu frequency scaling support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <DB3PR0402MB39161D97A6D2B8DD0DD5D6ACF5660@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date:   Wed, 13 Feb 2019 14:02:54 +0000
From:   Anson Huang <anson.huang@....com>
To:     Leonard Crestez <leonard.crestez@....com>
CC:     dl-linux-imx <linux-imx@....com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        "mturquette@...libre.com" <mturquette@...libre.com>,
        "sboyd@...nel.org" <sboyd@...nel.org>,
        Aisheng Dong <aisheng.dong@....com>,
        Daniel Baluta <daniel.baluta@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        "viresh.kumar@...aro.org" <viresh.kumar@...aro.org>
Subject: RE: [PATCH V2 2/2] clk: imx: scu: add cpu frequency scaling support



Best Regards!
Anson Huang

> -----Original Message-----
> From: Leonard Crestez
> Sent: 2019年2月13日 21:53
> To: Anson Huang <anson.huang@....com>
> Cc: dl-linux-imx <linux-imx@....com>; robh+dt@...nel.org;
> mark.rutland@....com; shawnguo@...nel.org; s.hauer@...gutronix.de;
> kernel@...gutronix.de; festevam@...il.com; mturquette@...libre.com;
> sboyd@...nel.org; Aisheng Dong <aisheng.dong@....com>; Daniel Baluta
> <daniel.baluta@....com>; devicetree@...r.kernel.org; linux-arm-
> kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; linux-
> clk@...r.kernel.org; viresh.kumar@...aro.org
> Subject: Re: [PATCH V2 2/2] clk: imx: scu: add cpu frequency scaling support
> 
> On Wed, 2019-02-13 at 13:32 +0000, Anson Huang wrote:
> > On NXP's i.MX SoCs with system controller inside, CPU frequency
> > scaling can ONLY be done by system controller firmware, and it can
> > ONLY be requested from secure mode, so Linux kernel has to call ARM
> > SMC to trap to ARM-Trusted-Firmware to request system controller
> > firmware to do CPU frequency scaling.
> 
> > +#ifdef CONFIG_CPUFREQ_DT
> > +#define IMX_SIP_CPUFREQ			0xC2000001
> > +#define IMX_SIP_SET_CPUFREQ		0x00
> > +
> >  static struct imx_sc_ipc *ccm_ipc_handle;
> 
> Without CONFIG_CPUFREQ_DT the ccm_ipc_handle won't be defined and
> build will break.
> 
> But is there a good reason for ifdef? In general clock function are compiled
> even if no driver is calling them.

Oops, this is my mistake to put the ccm_ipc_handle inside the ifdef. Adding
Ifdef is just to save some code if CPUFREQ_DT is NOT defined, but I can remove
it in next version.

> 
> > +#ifdef CONFIG_CPUFREQ_DT
> > +	/* CPU clock can ONLY be done by TF-A */
> > +	if (clk->clk_type == IMX_SC_PM_CLK_CPU) {
> > +		struct arm_smccc_res res;
> > +		unsigned int cluster_id;
> > +		int i;
> > +
> > +		for (i = 0; i < ARRAY_SIZE(imx_sc_cpufreq_data); i++) {
> > +			if (!strcmp(clk_hw_get_name(hw),
> > +				imx_sc_cpufreq_data[i].clk_name)) {
> > +				cluster_id =
> imx_sc_cpufreq_data[i].cluster_id;
> > +				break;
> > +			}
> > +		}
> > +
> > +		/*
> > +		 * As some other clock types have same value as
> > +		 * IMX_SC_PM_CLK_CPU, so we need to double check
> > +		 * the clock being scaled is indeed CPU clock which
> > +		 * matches the table we define.
> > +		 */
> > +		if (i < ARRAY_SIZE(imx_sc_cpufreq_data)) {
> > +			arm_smccc_smc(IMX_SIP_CPUFREQ,
> IMX_SIP_SET_CPUFREQ,
> > +				cluster_id, rate, 0, 0, 0, 0, &res);
> > +			return 0;
> > +		}
> > +	}
> > +#endif
> 
> The code inside the ifdef would look better in a separate
> imx_clk_atf_set_rate function. Maybe even separate clk_ops?
I can add a separate function for CPU clock scaling, adding new clk_ops
changes too much, using a separate function should be good for now?

Thanks.
Anson.