| 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
| ||
|
Message-ID: <518142BD.2000804@arm.com>
Date: Wed, 01 May 2013 17:28:45 +0100
From: Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha@....com>
To: Nishanth Menon <nm@...com>
CC: Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha@....com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"grant.likely@...aro.org" <grant.likely@...aro.org>,
"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
Rob Landley <rob@...dley.net>,
"Rafael J. Wysocki" <rjw@...k.pl>,
Shawn Guo <shawn.guo@...aro.org>,
"devicetree-discuss@...ts.ozlabs.org"
<devicetree-discuss@...ts.ozlabs.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: Re: [PATCH 1/2] PM / OPP: add support to specify phandle of another
node for OPP
On 01/05/13 15:41, Nishanth Menon wrote:
> On 12:11-20130501, Sudeep.KarkadaNagesha@....com wrote:
>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@....com>
>>
>> If more than one similar devices share the same OPPs, currently we
>> need to replicate the OPP entries in all the nodes.
> Nice, thanks.
>>
>> Few drivers like cpufreq depend on physical cpu0 node to specify the
> cpufreq-cpu0?
Yes when I originally wrote this patch cpufreq-cpu0 was using cpu0 node.
But later sometimes it was changed to parse all the nodes.
>> OPPs and only that node is referred irrespective of the logical cpu
>> accessing it. Alternatively to support cpuhotplug path, few drivers
>> parse all the cpu nodes for OPPs. Instead we can specify the phandle
>> of the node with which the current node shares the operating points.
>>
>> This patch adds support to specify the phandle in the operating points
>> of any device node, where the node specified by the phandle holds the
>> actual OPPs.
>>
>> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@....com>
>> ---
>> Documentation/devicetree/bindings/power/opp.txt | 41 +++++++++++++++++++++++
>> drivers/base/power/opp.c | 30 ++++++++++++-----
>> 2 files changed, 63 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
>> index 74499e5..a659da4 100644
>> --- a/Documentation/devicetree/bindings/power/opp.txt
>> +++ b/Documentation/devicetree/bindings/power/opp.txt
>> @@ -23,3 +23,44 @@ cpu@0 {
>> 198000 850000
>> >;
>> };
>> +
> Definition of operating-points is now a little different in the
> original description - it still indicates tuple {freq,voltage}, where
> as, this patch allows phandle to a different device's operating-points
> to be used. - we might want to rephrase the description.
>
> btw, to device-tree folks, I am not sure if it is OK to have different formats
> for the same property like operating-points. At least I don't seem to
> quickly be able to find any precedence.
>
>> +If more than one device of same type share the same OPPs, e.g. all the CPUs on
> s/e.g/example?
Ok
>> +a SoC or in a single cluster on a SoC, then we need to avoid replicating the
>> +OPPs in all the nodes. We can specify the phandle of the node with which the
>> +current node shares the operating points instead.
>> +
>> +Examples:
>> +Consider an SMP with 4 CPUs all sharing the same OPPs.
> We might want to descr
I could not get what exactly you mean by 'descr', do you mean more
descriptive ? If so, what exactly you would like to add ?
>> +
>> +cpu0: cpu@0 {
>> + compatible = "arm,cortex-a9";
>> + reg = <0>;
>> + next-level-cache = <&L2>;
>> + operating-points = <
>> + /* kHz uV */
>> + 792000 1100000
>> + 396000 950000
>> + 198000 850000
>> + >;
>> +};
>> +
>> +cpu1: cpu@1 {
>> + compatible = "arm,cortex-a9";
>> + reg = <1>;
>> + next-level-cache = <&L2>;
>> + operating-points = <&cpu0>;
>> +};
>> +
>> +cpu2: cpu@2 {
>> + compatible = "arm,cortex-a9";
>> + reg = <2>;
>> + next-level-cache = <&L2>;
>> + operating-points = <&cpu0>;
>> +};
>> +
>> +cpu3: cpu@3 {
>> + compatible = "arm,cortex-a9";
>> + reg = <3>;
>> + next-level-cache = <&L2>;
>> + operating-points = <&cpu0>;
>> +};
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> index f0077cb..4dfdc01 100644
>> --- a/drivers/base/power/opp.c
>> +++ b/drivers/base/power/opp.c
>> @@ -698,19 +698,15 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev)
>> }
>>
>> #ifdef CONFIG_OF
>> -/**
>> - * of_init_opp_table() - Initialize opp table from device tree
>> - * @dev: device pointer used to lookup device OPPs.
>> - *
>> - * Register the initial OPP table with the OPP library for given device.
>> - */
>> -int of_init_opp_table(struct device *dev)
>> +static int of_init_opp_table_from_ofnode(struct device *dev,
>> + struct device_node *of_node)
> please provide kernel-doc documentation for static function as well -
> this is inline with the rest of the file.
Ok
>> {
>> + struct device_opp *dev_opp = NULL;
> dev_opp is not used until patch #2 - please introduce it in that patch.
Correct it was meant to be in patch#2, will move in next version
>> const struct property *prop;
>> const __be32 *val;
>> int nr;
>>
>> - prop = of_find_property(dev->of_node, "operating-points", NULL);
>> + prop = of_find_property(of_node, "operating-points", NULL);
>> if (!prop)
>> return -ENODEV;
>> if (!prop->value)
>> @@ -722,6 +718,14 @@ int of_init_opp_table(struct device *dev)
>> */
>> nr = prop->length / sizeof(u32);
>> if (nr % 2) {
>> + if (nr == 1) {
>> + struct device_node *opp_node;
>> + opp_node = of_parse_phandle(dev->of_node,
>> + "operating-points", 0);
>> + if (opp_node)
>> + return of_init_opp_table_from_ofnode(dev,
>> + opp_node);
>> + }
> if operating-points=<100000>, then we return Invalid OPP list
> if operating-points=<&uart3>; (some phandle that does not have
> operating-points), there is no helpful warning in log except -ENODEV is
> returned - we might want to add some info here?
Makes sense, will add that in next version.
>> dev_err(dev, "%s: Invalid OPP list\n", __func__);
>> return -EINVAL;
>> }
>> @@ -741,5 +745,15 @@ int of_init_opp_table(struct device *dev)
>>
>> return 0;
>> }
> missing EOL?
Ok
>> +/**
>> + * of_init_opp_table() - Initialize opp table from device tree
>> + * @dev: device pointer used to lookup device OPPs.
>> + *
>> + * Register the initial OPP table with the OPP library for given device.
>> + */
>> +int of_init_opp_table(struct device *dev)
>> +{
>> + return of_init_opp_table_from_ofnode(dev, dev->of_node);
>> +}
>> EXPORT_SYMBOL_GPL(of_init_opp_table);
>> #endif
>> --
>> 1.7.10.4
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists