Extending OPP bindings

Extending OPP bindings

[Sudeep Holla]

Hi all,

I am looking into a couple shortcomings in the current OPP bindings and
how to address them. Feel free to add to the list if you think of any more
issues that needs to be addressed or if and how any problem mentioned below
can be handled with the existing bindings.

1. indexing: currently there are no indices in the operating-points.
	It's assumed that the list is either in ascending or descending
	order of frequency but not explicit in the binding document.
	There are arch/arm/boot/dts/* files with opps in both styles.
	Few other bindings like thermal defines bindings like
	cooling-{min,max}-state assuming some order which is broken IMO.

	One such use-case that came up recently[0] is the c-state latencies
	which could be different for each OPP. It would be good if the
	latencies are specified with the indices to OPP table to avoid
	inconsistency between the bindings.

	It's mainly to avoid issues due to inconsistency and duplication
	on data(frequency) in multiple bindings requiring it.

	Once we have indices to each on the OPP entries, then other binding
	using it can refer to OPP with phandle and OPP index/specifier pairs
	very similar to clock provider and consumer.

2. sharing opps: I have tried to address this issue previously[1] but unable
	to conclude yet on this.

3. latencies(*): currently the latency that the CPU/memory access is unavailable
	during an OPP transition is generic i.e. same from any OPP to any
	other OPP. Does it make sense to have this per-OPP entry ?

4. power(*): A measure of maximum power dissipation in an OPP state.
	This might be useful measure for power aware scheduling ?

(*) these are already part of P-state in ACPI(refer struct acpi_processor_px
in include/acpi/processor.h)

Apart from these I have seen on-going discussion for Samsung Exynos CPUFreq[2]
which might have some feedback for OPP bindings.

It would be good to consolidate the shortcomings found so far, that could
help in extending the current OPP bindings.

Regards,
Sudeep

[0] http://www.spinics.net/lists/arm-kernel/msg301971.html
[1] http://www.spinics.net/lists/cpufreq/msg07911.html
[2] http://www.spinics.net/lists/cpufreq/msg09169.html

Extending OPP bindings

[Sudeep Holla]

Hi all,

I am looking into a couple shortcomings in the current OPP bindings and
how to address them. Feel free to add to the list if you think of any more
issues that needs to be addressed or if and how any problem mentioned below
can be handled with the existing bindings.

1. indexing: currently there are no indices in the operating-points.
	It's assumed that the list is either in ascending or descending
	order of frequency but not explicit in the binding document.
	There are arch/arm/boot/dts/* files with opps in both styles.
	Few other bindings like thermal defines bindings like
	cooling-{min,max}-state assuming some order which is broken IMO.

	One such use-case that came up recently[0] is the c-state latencies
	which could be different for each OPP. It would be good if the
	latencies are specified with the indices to OPP table to avoid
	inconsistency between the bindings.

	It's mainly to avoid issues due to inconsistency and duplication
	on data(frequency) in multiple bindings requiring it.

	Once we have indices to each on the OPP entries, then other binding
	using it can refer to OPP with phandle and OPP index/specifier pairs
	very similar to clock provider and consumer.

2. sharing opps: I have tried to address this issue previously[1] but unable
	to conclude yet on this.

3. latencies(*): currently the latency that the CPU/memory access is unavailable
	during an OPP transition is generic i.e. same from any OPP to any
	other OPP. Does it make sense to have this per-OPP entry ?

4. power(*): A measure of maximum power dissipation in an OPP state.
	This might be useful measure for power aware scheduling ?

(*) these are already part of P-state in ACPI(refer struct acpi_processor_px
in include/acpi/processor.h)

Apart from these I have seen on-going discussion for Samsung Exynos CPUFreq[2]
which might have some feedback for OPP bindings.

It would be good to consolidate the shortcomings found so far, that could
help in extending the current OPP bindings.

Regards,
Sudeep

[0] http://www.spinics.net/lists/arm-kernel/msg301971.html
[1] http://www.spinics.net/lists/cpufreq/msg07911.html
[2] http://www.spinics.net/lists/cpufreq/msg09169.html

Re: Extending OPP bindings

[Nishanth Menon]

Hi Sudeep,

On 01/30/2014 07:43 AM, Sudeep Holla wrote:

> 
> I am looking into a couple shortcomings in the current OPP bindings and
> how to address them. Feel free to add to the list if you think of any more
> issues that needs to be addressed or if and how any problem mentioned below
> can be handled with the existing bindings.
> 
> 1. indexing: currently there are no indices in the operating-points.
indexing is based on frequency which is why the accessors use
frequency to pull out the OPP data.

indexing is a horrible idea - on platforms where OPP may be disabled
or enabled for various reasons(see arch/arm/mach-imx/mach-imx6q.c,
arch/arm/mach-omap2/board-omap3beagle.c etc) - the indexing you see in
dts is just a myth that may not exist once the nodes are loaded and
operated upon depending on SoC variations (example efuse describing
which OPPs can be used and which not).

That said, the original OPP[1][2] series discussion started by trying
to kill indexing precisely for the same reason.. once you have it - it
becomes just crazy to deal with.

> 	It's assumed that the list is either in ascending or descending
> 	order of frequency but not explicit in the binding document.
> 	There are arch/arm/boot/dts/* files with opps in both styles.
it should not matter -> opp library should do an insertion sort and
organize it in ascending order once all the data is inserted. (line
449ish in opp.c)

if you see issues with the insertion sort not functioning, it is a bug
and should be easy to track down and fix.

> 	Few other bindings like thermal defines bindings like
> 	cooling-{min,max}-state assuming some order which is broken IMO.
Now that you bring it up, I missed it :(.. yeah, I might have
preferred it to be min frequency and max_frequency - I agree it is
probably broken. I'd let Eduardo comment more about it.

> 
> 	One such use-case that came up recently[0] is the c-state latencies
> 	which could be different for each OPP. It would be good if the
> 	latencies are specified with the indices to OPP table to avoid
> 	inconsistency between the bindings.

You can define C states based on frequencies as well - which really
makes sense - since that sounds really like our constraint (say
valid-at-frequency "xyz"

> 
> 	It's mainly to avoid issues due to inconsistency and duplication
> 	on data(frequency) in multiple bindings requiring it.
> 
> 	Once we have indices to each on the OPP entries, then other binding
> 	using it can refer to OPP with phandle and OPP index/specifier pairs
> 	very similar to clock provider and consumer.

Having used indexing in OMAP platforms, indexing is a problem waiting
to happen unfortunately :(

> 
> 2. sharing opps: I have tried to address this issue previously[1] but unable
> 	to conclude yet on this.

yes - more details in [3] - which is a more interesting discussion
there - lets revive it in that context.

It is a valid concern and IMHO a great idea - yeah we already have a
thread started.

> 
> 3. latencies(*): currently the latency that the CPU/memory access is unavailable
> 	during an OPP transition is generic i.e. same from any OPP to any
> 	other OPP. Does it make sense to have this per-OPP entry ?

Why modify OPP when you are describing something else? you are
describing "latency at a frequency" - just because an OPP definition
as it stands right now is {frequency, voltage} tuple, makes it a very
attractive target to keep extending it -> believe me, we have done
that in the past ->arch/arm/mach-omap2/opp4xxx_data.c efuse register
describing AVS per frequency is tempting..

why not have memory-latency-per-opp = <frequency latency>?

that allows OPP definitions to change in the future, but the
definition remain constant.

That said -> consider the following usecase: AM335x, OMAP3,4... (i
will use omap4 as an example)
MPU@300MHz and bus (on which LPDDR2 memory is) at 100MHz
AND
MPU@300MHz and bus (on which LPDDR2 memory is) at 200MHz

are both valid with different memory access latencies. tying it down
to OPP for MPU is just plain wrong - as it ignores other factors.

> 
> 4. power(*): A measure of maximum power dissipation in an OPP state.
> 	This might be useful measure for power aware scheduling ?
Umm.. this is a hard nut to crack -> I had considered that previously
as well -> In reality the leakage characteristics of the SoC
distribution varies dramatically depending on which end of the
distribution you look for a specific process node. in my company, we
typically use cold, hot,nominal devices, this is some form or other
(example - Samsung calls it "SoC's ASV group" [4]) - and every SoC
company comes up with some strategy or other to control it optimally
-> TI uses ABB[5], AVS[6] - etc... - not an unique problem -> so what
will "power" mean? we cannot create dts per SoC part.

> 
> (*) these are already part of P-state in ACPI(refer struct acpi_processor_px
> in include/acpi/processor.h)

Hmm.. what do we do with legacy processors that dont support ACPI or
what ever our latest ARM term is for the equivalent?

> 
> Apart from these I have seen on-going discussion for Samsung Exynos CPUFreq[2]
> which might have some feedback for OPP bindings.
> 
> It would be good to consolidate the shortcomings found so far, that could
> help in extending the current OPP bindings.

I hope this discussion helps. open to more views as well.

> [0] http://www.spinics.net/lists/arm-kernel/msg301971.html
> [1] http://www.spinics.net/lists/cpufreq/msg07911.html
> [2] http://www.spinics.net/lists/cpufreq/msg09169.html

[1] http://marc.info/?t=125546601600001&r=1&w=2
[2] http://marc.info/?l=linux-omap&m=125474840119392&w=2
[3] http://marc.info/?t=138063448000008&r=1&w=2
[4] http://marc.info/?l=linux-pm&m=138451581304412&w=2
[5]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/ti-abb-regulator.c
[6]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/power/avs/smartreflex.c
-- 
Regards,
Nishanth Menon

Extending OPP bindings

[Nishanth Menon]

Hi Sudeep,

On 01/30/2014 07:43 AM, Sudeep Holla wrote:

> 
> I am looking into a couple shortcomings in the current OPP bindings and
> how to address them. Feel free to add to the list if you think of any more
> issues that needs to be addressed or if and how any problem mentioned below
> can be handled with the existing bindings.
> 
> 1. indexing: currently there are no indices in the operating-points.
indexing is based on frequency which is why the accessors use
frequency to pull out the OPP data.

indexing is a horrible idea - on platforms where OPP may be disabled
or enabled for various reasons(see arch/arm/mach-imx/mach-imx6q.c,
arch/arm/mach-omap2/board-omap3beagle.c etc) - the indexing you see in
dts is just a myth that may not exist once the nodes are loaded and
operated upon depending on SoC variations (example efuse describing
which OPPs can be used and which not).

That said, the original OPP[1][2] series discussion started by trying
to kill indexing precisely for the same reason.. once you have it - it
becomes just crazy to deal with.

> 	It's assumed that the list is either in ascending or descending
> 	order of frequency but not explicit in the binding document.
> 	There are arch/arm/boot/dts/* files with opps in both styles.
it should not matter -> opp library should do an insertion sort and
organize it in ascending order once all the data is inserted. (line
449ish in opp.c)

if you see issues with the insertion sort not functioning, it is a bug
and should be easy to track down and fix.

> 	Few other bindings like thermal defines bindings like
> 	cooling-{min,max}-state assuming some order which is broken IMO.
Now that you bring it up, I missed it :(.. yeah, I might have
preferred it to be min frequency and max_frequency - I agree it is
probably broken. I'd let Eduardo comment more about it.

> 
> 	One such use-case that came up recently[0] is the c-state latencies
> 	which could be different for each OPP. It would be good if the
> 	latencies are specified with the indices to OPP table to avoid
> 	inconsistency between the bindings.

You can define C states based on frequencies as well - which really
makes sense - since that sounds really like our constraint (say
valid-at-frequency "xyz"

> 
> 	It's mainly to avoid issues due to inconsistency and duplication
> 	on data(frequency) in multiple bindings requiring it.
> 
> 	Once we have indices to each on the OPP entries, then other binding
> 	using it can refer to OPP with phandle and OPP index/specifier pairs
> 	very similar to clock provider and consumer.

Having used indexing in OMAP platforms, indexing is a problem waiting
to happen unfortunately :(

> 
> 2. sharing opps: I have tried to address this issue previously[1] but unable
> 	to conclude yet on this.

yes - more details in [3] - which is a more interesting discussion
there - lets revive it in that context.

It is a valid concern and IMHO a great idea - yeah we already have a
thread started.

> 
> 3. latencies(*): currently the latency that the CPU/memory access is unavailable
> 	during an OPP transition is generic i.e. same from any OPP to any
> 	other OPP. Does it make sense to have this per-OPP entry ?

Why modify OPP when you are describing something else? you are
describing "latency at a frequency" - just because an OPP definition
as it stands right now is {frequency, voltage} tuple, makes it a very
attractive target to keep extending it -> believe me, we have done
that in the past ->arch/arm/mach-omap2/opp4xxx_data.c efuse register
describing AVS per frequency is tempting..

why not have memory-latency-per-opp = <frequency latency>?

that allows OPP definitions to change in the future, but the
definition remain constant.

That said -> consider the following usecase: AM335x, OMAP3,4... (i
will use omap4 as an example)
MPU at 300MHz and bus (on which LPDDR2 memory is) at 100MHz
AND
MPU at 300MHz and bus (on which LPDDR2 memory is) at 200MHz

are both valid with different memory access latencies. tying it down
to OPP for MPU is just plain wrong - as it ignores other factors.

> 
> 4. power(*): A measure of maximum power dissipation in an OPP state.
> 	This might be useful measure for power aware scheduling ?
Umm.. this is a hard nut to crack -> I had considered that previously
as well -> In reality the leakage characteristics of the SoC
distribution varies dramatically depending on which end of the
distribution you look for a specific process node. in my company, we
typically use cold, hot,nominal devices, this is some form or other
(example - Samsung calls it "SoC's ASV group" [4]) - and every SoC
company comes up with some strategy or other to control it optimally
-> TI uses ABB[5], AVS[6] - etc... - not an unique problem -> so what
will "power" mean? we cannot create dts per SoC part.

> 
> (*) these are already part of P-state in ACPI(refer struct acpi_processor_px
> in include/acpi/processor.h)

Hmm.. what do we do with legacy processors that dont support ACPI or
what ever our latest ARM term is for the equivalent?

> 
> Apart from these I have seen on-going discussion for Samsung Exynos CPUFreq[2]
> which might have some feedback for OPP bindings.
> 
> It would be good to consolidate the shortcomings found so far, that could
> help in extending the current OPP bindings.

I hope this discussion helps. open to more views as well.

> [0] http://www.spinics.net/lists/arm-kernel/msg301971.html
> [1] http://www.spinics.net/lists/cpufreq/msg07911.html
> [2] http://www.spinics.net/lists/cpufreq/msg09169.html

[1] http://marc.info/?t=125546601600001&r=1&w=2
[2] http://marc.info/?l=linux-omap&m=125474840119392&w=2
[3] http://marc.info/?t=138063448000008&r=1&w=2
[4] http://marc.info/?l=linux-pm&m=138451581304412&w=2
[5]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/ti-abb-regulator.c
[6]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/power/avs/smartreflex.c
-- 
Regards,
Nishanth Menon

Re: Extending OPP bindings

[Lorenzo Pieralisi]

Hi Nishanth,

On Fri, Jan 31, 2014 at 12:43:54AM +0000, Nishanth Menon wrote:
> Hi Sudeep,
> 
> On 01/30/2014 07:43 AM, Sudeep Holla wrote:
> 
> > 
> > I am looking into a couple shortcomings in the current OPP bindings and
> > how to address them. Feel free to add to the list if you think of any more
> > issues that needs to be addressed or if and how any problem mentioned below
> > can be handled with the existing bindings.
> > 
> > 1. indexing: currently there are no indices in the operating-points.
> indexing is based on frequency which is why the accessors use
> frequency to pull out the OPP data.
> 
> indexing is a horrible idea - on platforms where OPP may be disabled
> or enabled for various reasons(see arch/arm/mach-imx/mach-imx6q.c,
> arch/arm/mach-omap2/board-omap3beagle.c etc) - the indexing you see in
> dts is just a myth that may not exist once the nodes are loaded and
> operated upon depending on SoC variations (example efuse describing
> which OPPs can be used and which not).

I do not understand why. As long as a mapping from DT to data structures
in the kernel is done at boot, I can see how by indexing device nodes
can refer to specific OPPs. If they are disabled, amen, as long as we
can point at specific operating points that should be ok.

Indexing does not mean that the index in the DT is the same as the one
allocated by the OS. Indexing is there to point at specific OPPs from
different DT nodes, a good example are clock bindings and that's exactly
how they work.

Can you provide me with an example where the indexing would go wrong
please ?

Certainly relying on implicit ordering is not great either, actually I
would say that it is broken.

> That said, the original OPP[1][2] series discussion started by trying
> to kill indexing precisely for the same reason.. once you have it - it
> becomes just crazy to deal with.

I could not find any "index killing" :) discussion in there, but I will
keep reading.

> > 	It's assumed that the list is either in ascending or descending
> > 	order of frequency but not explicit in the binding document.
> > 	There are arch/arm/boot/dts/* files with opps in both styles.
> it should not matter -> opp library should do an insertion sort and
> organize it in ascending order once all the data is inserted. (line
> 449ish in opp.c)

That's OS details and they must not be mandated by the bindings.
We cannot rely on word of mouth for things to work, I do not like
implicit rules, so the bindings should speficy the ordering or better
a way to reference OPPs.

> if you see issues with the insertion sort not functioning, it is a bug
> and should be easy to track down and fix.

Problem is not the insertion sort, problem is that DT bindings do not
define a way to refer to a specific OPP, unless we do that implicitly
and again, I rest my case, that is broken.

> > 	Few other bindings like thermal defines bindings like
> > 	cooling-{min,max}-state assuming some order which is broken IMO.
> Now that you bring it up, I missed it :(.. yeah, I might have
> preferred it to be min frequency and max_frequency - I agree it is
> probably broken. I'd let Eduardo comment more about it.
> 
> > 
> > 	One such use-case that came up recently[0] is the c-state latencies
> > 	which could be different for each OPP. It would be good if the
> > 	latencies are specified with the indices to OPP table to avoid
> > 	inconsistency between the bindings.
> 
> You can define C states based on frequencies as well - which really
> makes sense - since that sounds really like our constraint (say
> valid-at-frequency "xyz"

I do not think that's generic enough, I do not like the idea of looking
up frequencies (eg a C-state target_residency does not depend on the
frequency only - ie voltage and other factors play a role, it really
depends on an operating point- looking it up by frequency is misleading IMO).

> > 	It's mainly to avoid issues due to inconsistency and duplication
> > 	on data(frequency) in multiple bindings requiring it.
> > 
> > 	Once we have indices to each on the OPP entries, then other binding
> > 	using it can refer to OPP with phandle and OPP index/specifier pairs
> > 	very similar to clock provider and consumer.
> 
> Having used indexing in OMAP platforms, indexing is a problem waiting
> to happen unfortunately :(

Please be specific, I do not see why it is a problem.

[...]

> > 3. latencies(*): currently the latency that the CPU/memory access is unavailable
> > 	during an OPP transition is generic i.e. same from any OPP to any
> > 	other OPP. Does it make sense to have this per-OPP entry ?
> 
> Why modify OPP when you are describing something else? you are
> describing "latency at a frequency" - just because an OPP definition
> as it stands right now is {frequency, voltage} tuple, makes it a very
> attractive target to keep extending it -> believe me, we have done
> that in the past ->arch/arm/mach-omap2/opp4xxx_data.c efuse register
> describing AVS per frequency is tempting..
> 
> why not have memory-latency-per-opp = <frequency latency>?
> 
> that allows OPP definitions to change in the future, but the
> definition remain constant.
> 
> That said -> consider the following usecase: AM335x, OMAP3,4... (i
> will use omap4 as an example)
> MPU@300MHz and bus (on which LPDDR2 memory is) at 100MHz
> AND
> MPU@300MHz and bus (on which LPDDR2 memory is) at 200MHz
> 
> are both valid with different memory access latencies. tying it down
> to OPP for MPU is just plain wrong - as it ignores other factors.

I think it is time we were defining OPPs for the system as a whole, the
issue has been brought up for C-state dependencies as well.

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/228042.html

We can easily define a global node that encompasses OPPs for various
devices, and then map the OPPs for devices to a global OPP table.

(totally made-up DT bindings, just to get the message across)

opps {
	operating-points = <......>;
	operating-point-cells = <2>;

	cpu-opps {
	/* where index maps to a global OPP above where the CPU OPP is valid */
		operating-points = <...... index>;
	
	};
	memory-opp {
	/*
	   where index maps to a global OPP above where the memory OPP is
	   valid
	*/
		operating-points = <...... index>;
	};
};

> > 4. power(*): A measure of maximum power dissipation in an OPP state.
> > 	This might be useful measure for power aware scheduling ?
> Umm.. this is a hard nut to crack -> I had considered that previously
> as well -> In reality the leakage characteristics of the SoC
> distribution varies dramatically depending on which end of the
> distribution you look for a specific process node. in my company, we
> typically use cold, hot,nominal devices, this is some form or other
> (example - Samsung calls it "SoC's ASV group" [4]) - and every SoC
> company comes up with some strategy or other to control it optimally
> -> TI uses ABB[5], AVS[6] - etc... - not an unique problem -> so what
> will "power" mean? we cannot create dts per SoC part.

Yes, that's a hard nut to crack. Probably we can define a reference value,
to be debated.

> > (*) these are already part of P-state in ACPI(refer struct acpi_processor_px
> > in include/acpi/processor.h)
> 
> Hmm.. what do we do with legacy processors that dont support ACPI or
> what ever our latest ARM term is for the equivalent?
> 
> > 
> > Apart from these I have seen on-going discussion for Samsung Exynos CPUFreq[2]
> > which might have some feedback for OPP bindings.
> > 
> > It would be good to consolidate the shortcomings found so far, that could
> > help in extending the current OPP bindings.
> 
> I hope this discussion helps. open to more views as well.

It helps a lot, thank you, please keep it going.

Thanks,
Lorenzo

Extending OPP bindings

[Lorenzo Pieralisi]

Hi Nishanth,

On Fri, Jan 31, 2014 at 12:43:54AM +0000, Nishanth Menon wrote:
> Hi Sudeep,
> 
> On 01/30/2014 07:43 AM, Sudeep Holla wrote:
> 
> > 
> > I am looking into a couple shortcomings in the current OPP bindings and
> > how to address them. Feel free to add to the list if you think of any more
> > issues that needs to be addressed or if and how any problem mentioned below
> > can be handled with the existing bindings.
> > 
> > 1. indexing: currently there are no indices in the operating-points.
> indexing is based on frequency which is why the accessors use
> frequency to pull out the OPP data.
> 
> indexing is a horrible idea - on platforms where OPP may be disabled
> or enabled for various reasons(see arch/arm/mach-imx/mach-imx6q.c,
> arch/arm/mach-omap2/board-omap3beagle.c etc) - the indexing you see in
> dts is just a myth that may not exist once the nodes are loaded and
> operated upon depending on SoC variations (example efuse describing
> which OPPs can be used and which not).

I do not understand why. As long as a mapping from DT to data structures
in the kernel is done at boot, I can see how by indexing device nodes
can refer to specific OPPs. If they are disabled, amen, as long as we
can point at specific operating points that should be ok.

Indexing does not mean that the index in the DT is the same as the one
allocated by the OS. Indexing is there to point at specific OPPs from
different DT nodes, a good example are clock bindings and that's exactly
how they work.

Can you provide me with an example where the indexing would go wrong
please ?

Certainly relying on implicit ordering is not great either, actually I
would say that it is broken.

> That said, the original OPP[1][2] series discussion started by trying
> to kill indexing precisely for the same reason.. once you have it - it
> becomes just crazy to deal with.

I could not find any "index killing" :) discussion in there, but I will
keep reading.

> > 	It's assumed that the list is either in ascending or descending
> > 	order of frequency but not explicit in the binding document.
> > 	There are arch/arm/boot/dts/* files with opps in both styles.
> it should not matter -> opp library should do an insertion sort and
> organize it in ascending order once all the data is inserted. (line
> 449ish in opp.c)

That's OS details and they must not be mandated by the bindings.
We cannot rely on word of mouth for things to work, I do not like
implicit rules, so the bindings should speficy the ordering or better
a way to reference OPPs.

> if you see issues with the insertion sort not functioning, it is a bug
> and should be easy to track down and fix.

Problem is not the insertion sort, problem is that DT bindings do not
define a way to refer to a specific OPP, unless we do that implicitly
and again, I rest my case, that is broken.

> > 	Few other bindings like thermal defines bindings like
> > 	cooling-{min,max}-state assuming some order which is broken IMO.
> Now that you bring it up, I missed it :(.. yeah, I might have
> preferred it to be min frequency and max_frequency - I agree it is
> probably broken. I'd let Eduardo comment more about it.
> 
> > 
> > 	One such use-case that came up recently[0] is the c-state latencies
> > 	which could be different for each OPP. It would be good if the
> > 	latencies are specified with the indices to OPP table to avoid
> > 	inconsistency between the bindings.
> 
> You can define C states based on frequencies as well - which really
> makes sense - since that sounds really like our constraint (say
> valid-at-frequency "xyz"

I do not think that's generic enough, I do not like the idea of looking
up frequencies (eg a C-state target_residency does not depend on the
frequency only - ie voltage and other factors play a role, it really
depends on an operating point- looking it up by frequency is misleading IMO).

> > 	It's mainly to avoid issues due to inconsistency and duplication
> > 	on data(frequency) in multiple bindings requiring it.
> > 
> > 	Once we have indices to each on the OPP entries, then other binding
> > 	using it can refer to OPP with phandle and OPP index/specifier pairs
> > 	very similar to clock provider and consumer.
> 
> Having used indexing in OMAP platforms, indexing is a problem waiting
> to happen unfortunately :(

Please be specific, I do not see why it is a problem.

[...]

> > 3. latencies(*): currently the latency that the CPU/memory access is unavailable
> > 	during an OPP transition is generic i.e. same from any OPP to any
> > 	other OPP. Does it make sense to have this per-OPP entry ?
> 
> Why modify OPP when you are describing something else? you are
> describing "latency at a frequency" - just because an OPP definition
> as it stands right now is {frequency, voltage} tuple, makes it a very
> attractive target to keep extending it -> believe me, we have done
> that in the past ->arch/arm/mach-omap2/opp4xxx_data.c efuse register
> describing AVS per frequency is tempting..
> 
> why not have memory-latency-per-opp = <frequency latency>?
> 
> that allows OPP definitions to change in the future, but the
> definition remain constant.
> 
> That said -> consider the following usecase: AM335x, OMAP3,4... (i
> will use omap4 as an example)
> MPU at 300MHz and bus (on which LPDDR2 memory is) at 100MHz
> AND
> MPU at 300MHz and bus (on which LPDDR2 memory is) at 200MHz
> 
> are both valid with different memory access latencies. tying it down
> to OPP for MPU is just plain wrong - as it ignores other factors.

I think it is time we were defining OPPs for the system as a whole, the
issue has been brought up for C-state dependencies as well.

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/228042.html

We can easily define a global node that encompasses OPPs for various
devices, and then map the OPPs for devices to a global OPP table.

(totally made-up DT bindings, just to get the message across)

opps {
	operating-points = <......>;
	operating-point-cells = <2>;

	cpu-opps {
	/* where index maps to a global OPP above where the CPU OPP is valid */
		operating-points = <...... index>;
	
	};
	memory-opp {
	/*
	   where index maps to a global OPP above where the memory OPP is
	   valid
	*/
		operating-points = <...... index>;
	};
};

> > 4. power(*): A measure of maximum power dissipation in an OPP state.
> > 	This might be useful measure for power aware scheduling ?
> Umm.. this is a hard nut to crack -> I had considered that previously
> as well -> In reality the leakage characteristics of the SoC
> distribution varies dramatically depending on which end of the
> distribution you look for a specific process node. in my company, we
> typically use cold, hot,nominal devices, this is some form or other
> (example - Samsung calls it "SoC's ASV group" [4]) - and every SoC
> company comes up with some strategy or other to control it optimally
> -> TI uses ABB[5], AVS[6] - etc... - not an unique problem -> so what
> will "power" mean? we cannot create dts per SoC part.

Yes, that's a hard nut to crack. Probably we can define a reference value,
to be debated.

> > (*) these are already part of P-state in ACPI(refer struct acpi_processor_px
> > in include/acpi/processor.h)
> 
> Hmm.. what do we do with legacy processors that dont support ACPI or
> what ever our latest ARM term is for the equivalent?
> 
> > 
> > Apart from these I have seen on-going discussion for Samsung Exynos CPUFreq[2]
> > which might have some feedback for OPP bindings.
> > 
> > It would be good to consolidate the shortcomings found so far, that could
> > help in extending the current OPP bindings.
> 
> I hope this discussion helps. open to more views as well.

It helps a lot, thank you, please keep it going.

Thanks,
Lorenzo

Re: Extending OPP bindings

[Mark Brown]

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

On Fri, Jan 31, 2014 at 12:46:20PM +0000, Lorenzo Pieralisi wrote:

> Indexing does not mean that the index in the DT is the same as the one
> allocated by the OS. Indexing is there to point at specific OPPs from
> different DT nodes, a good example are clock bindings and that's exactly
> how they work.

> Can you provide me with an example where the indexing would go wrong
> please ?

This does depend on what you mean by indexing but if you're talking
about having bindings where you have things that refer to the nth
element in an array in that form then it can become hard to read the
resulting DT and the potential for errors is increased.  Changes to the
original table need to take into account all the references to the table
and (especially if the number of entries gets large) it can be prone to
miscounting when checking references.

With OPPs this might happen if new operating points are characterised or
some are removed, for example due to new SoC variants or binning.

> Certainly relying on implicit ordering is not great either, actually I
> would say that it is broken.

Yes, that too - some sort of explict reference does help.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Extending OPP bindings

[Mark Brown]

On Fri, Jan 31, 2014 at 12:46:20PM +0000, Lorenzo Pieralisi wrote:

> Indexing does not mean that the index in the DT is the same as the one
> allocated by the OS. Indexing is there to point at specific OPPs from
> different DT nodes, a good example are clock bindings and that's exactly
> how they work.

> Can you provide me with an example where the indexing would go wrong
> please ?

This does depend on what you mean by indexing but if you're talking
about having bindings where you have things that refer to the nth
element in an array in that form then it can become hard to read the
resulting DT and the potential for errors is increased.  Changes to the
original table need to take into account all the references to the table
and (especially if the number of entries gets large) it can be prone to
miscounting when checking references.

With OPPs this might happen if new operating points are characterised or
some are removed, for example due to new SoC variants or binning.

> Certainly relying on implicit ordering is not great either, actually I
> would say that it is broken.

Yes, that too - some sort of explict reference does help.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140131/69f7af5f/attachment.sig>

Re: Extending OPP bindings

[Rob Herring]

On Fri, Jan 31, 2014 at 6:46 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> Hi Nishanth,
>
> On Fri, Jan 31, 2014 at 12:43:54AM +0000, Nishanth Menon wrote:
>> Hi Sudeep,
>>
>> On 01/30/2014 07:43 AM, Sudeep Holla wrote:
>>
>> >
>> > I am looking into a couple shortcomings in the current OPP bindings and
>> > how to address them. Feel free to add to the list if you think of any more
>> > issues that needs to be addressed or if and how any problem mentioned below
>> > can be handled with the existing bindings.
>> >
>> > 1. indexing: currently there are no indices in the operating-points.
>> indexing is based on frequency which is why the accessors use
>> frequency to pull out the OPP data.
>>
>> indexing is a horrible idea - on platforms where OPP may be disabled
>> or enabled for various reasons(see arch/arm/mach-imx/mach-imx6q.c,
>> arch/arm/mach-omap2/board-omap3beagle.c etc) - the indexing you see in
>> dts is just a myth that may not exist once the nodes are loaded and
>> operated upon depending on SoC variations (example efuse describing
>> which OPPs can be used and which not).
>
> I do not understand why. As long as a mapping from DT to data structures
> in the kernel is done at boot, I can see how by indexing device nodes
> can refer to specific OPPs. If they are disabled, amen, as long as we
> can point at specific operating points that should be ok.
>
> Indexing does not mean that the index in the DT is the same as the one
> allocated by the OS. Indexing is there to point at specific OPPs from
> different DT nodes, a good example are clock bindings and that's exactly
> how they work.

That is not a good comparison. With clocks, you are describing which
physical signal coming out of a hardware block much like interrupts.
Granted the h/w is not typically documented that way for clocks
although the numbering could correspond to register bit locations as
interrupt numbers do. With OPP indexes, they are a totally made up
software construct.

Perhaps OPPs should be nodes so new fields can be added easily and
then you have a phandle for each OPP.

>
> Can you provide me with an example where the indexing would go wrong
> please ?

Putting s/w indexes into DT. Using cell-index for uarts to define
their tty number was one example.

> Certainly relying on implicit ordering is not great either, actually I
> would say that it is broken.
>
>> That said, the original OPP[1][2] series discussion started by trying
>> to kill indexing precisely for the same reason.. once you have it - it
>> becomes just crazy to deal with.
>
> I could not find any "index killing" :) discussion in there, but I will
> keep reading.
>
>> >     It's assumed that the list is either in ascending or descending
>> >     order of frequency but not explicit in the binding document.
>> >     There are arch/arm/boot/dts/* files with opps in both styles.
>> it should not matter -> opp library should do an insertion sort and
>> organize it in ascending order once all the data is inserted. (line
>> 449ish in opp.c)
>
> That's OS details and they must not be mandated by the bindings.
> We cannot rely on word of mouth for things to work, I do not like
> implicit rules, so the bindings should speficy the ordering or better
> a way to reference OPPs.
>
>> if you see issues with the insertion sort not functioning, it is a bug
>> and should be easy to track down and fix.
>
> Problem is not the insertion sort, problem is that DT bindings do not
> define a way to refer to a specific OPP, unless we do that implicitly
> and again, I rest my case, that is broken.
>
>> >     Few other bindings like thermal defines bindings like
>> >     cooling-{min,max}-state assuming some order which is broken IMO.
>> Now that you bring it up, I missed it :(.. yeah, I might have
>> preferred it to be min frequency and max_frequency - I agree it is
>> probably broken. I'd let Eduardo comment more about it.
>>
>> >
>> >     One such use-case that came up recently[0] is the c-state latencies
>> >     which could be different for each OPP. It would be good if the
>> >     latencies are specified with the indices to OPP table to avoid
>> >     inconsistency between the bindings.
>>
>> You can define C states based on frequencies as well - which really
>> makes sense - since that sounds really like our constraint (say
>> valid-at-frequency "xyz"
>
> I do not think that's generic enough, I do not like the idea of looking
> up frequencies (eg a C-state target_residency does not depend on the
> frequency only - ie voltage and other factors play a role, it really
> depends on an operating point- looking it up by frequency is misleading IMO).

It seems we are trying to fit a square peg into a round hole here.

Rob

Extending OPP bindings

[Rob Herring]

On Fri, Jan 31, 2014 at 6:46 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> Hi Nishanth,
>
> On Fri, Jan 31, 2014 at 12:43:54AM +0000, Nishanth Menon wrote:
>> Hi Sudeep,
>>
>> On 01/30/2014 07:43 AM, Sudeep Holla wrote:
>>
>> >
>> > I am looking into a couple shortcomings in the current OPP bindings and
>> > how to address them. Feel free to add to the list if you think of any more
>> > issues that needs to be addressed or if and how any problem mentioned below
>> > can be handled with the existing bindings.
>> >
>> > 1. indexing: currently there are no indices in the operating-points.
>> indexing is based on frequency which is why the accessors use
>> frequency to pull out the OPP data.
>>
>> indexing is a horrible idea - on platforms where OPP may be disabled
>> or enabled for various reasons(see arch/arm/mach-imx/mach-imx6q.c,
>> arch/arm/mach-omap2/board-omap3beagle.c etc) - the indexing you see in
>> dts is just a myth that may not exist once the nodes are loaded and
>> operated upon depending on SoC variations (example efuse describing
>> which OPPs can be used and which not).
>
> I do not understand why. As long as a mapping from DT to data structures
> in the kernel is done at boot, I can see how by indexing device nodes
> can refer to specific OPPs. If they are disabled, amen, as long as we
> can point at specific operating points that should be ok.
>
> Indexing does not mean that the index in the DT is the same as the one
> allocated by the OS. Indexing is there to point at specific OPPs from
> different DT nodes, a good example are clock bindings and that's exactly
> how they work.

That is not a good comparison. With clocks, you are describing which
physical signal coming out of a hardware block much like interrupts.
Granted the h/w is not typically documented that way for clocks
although the numbering could correspond to register bit locations as
interrupt numbers do. With OPP indexes, they are a totally made up
software construct.

Perhaps OPPs should be nodes so new fields can be added easily and
then you have a phandle for each OPP.

>
> Can you provide me with an example where the indexing would go wrong
> please ?

Putting s/w indexes into DT. Using cell-index for uarts to define
their tty number was one example.

> Certainly relying on implicit ordering is not great either, actually I
> would say that it is broken.
>
>> That said, the original OPP[1][2] series discussion started by trying
>> to kill indexing precisely for the same reason.. once you have it - it
>> becomes just crazy to deal with.
>
> I could not find any "index killing" :) discussion in there, but I will
> keep reading.
>
>> >     It's assumed that the list is either in ascending or descending
>> >     order of frequency but not explicit in the binding document.
>> >     There are arch/arm/boot/dts/* files with opps in both styles.
>> it should not matter -> opp library should do an insertion sort and
>> organize it in ascending order once all the data is inserted. (line
>> 449ish in opp.c)
>
> That's OS details and they must not be mandated by the bindings.
> We cannot rely on word of mouth for things to work, I do not like
> implicit rules, so the bindings should speficy the ordering or better
> a way to reference OPPs.
>
>> if you see issues with the insertion sort not functioning, it is a bug
>> and should be easy to track down and fix.
>
> Problem is not the insertion sort, problem is that DT bindings do not
> define a way to refer to a specific OPP, unless we do that implicitly
> and again, I rest my case, that is broken.
>
>> >     Few other bindings like thermal defines bindings like
>> >     cooling-{min,max}-state assuming some order which is broken IMO.
>> Now that you bring it up, I missed it :(.. yeah, I might have
>> preferred it to be min frequency and max_frequency - I agree it is
>> probably broken. I'd let Eduardo comment more about it.
>>
>> >
>> >     One such use-case that came up recently[0] is the c-state latencies
>> >     which could be different for each OPP. It would be good if the
>> >     latencies are specified with the indices to OPP table to avoid
>> >     inconsistency between the bindings.
>>
>> You can define C states based on frequencies as well - which really
>> makes sense - since that sounds really like our constraint (say
>> valid-at-frequency "xyz"
>
> I do not think that's generic enough, I do not like the idea of looking
> up frequencies (eg a C-state target_residency does not depend on the
> frequency only - ie voltage and other factors play a role, it really
> depends on an operating point- looking it up by frequency is misleading IMO).

It seems we are trying to fit a square peg into a round hole here.

Rob

Re: Extending OPP bindings

[Lorenzo Pieralisi]

Hi Rob,

thanks for having a look.

On Fri, Jan 31, 2014 at 05:17:51PM +0000, Rob Herring wrote:
> On Fri, Jan 31, 2014 at 6:46 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > Hi Nishanth,
> >
> > On Fri, Jan 31, 2014 at 12:43:54AM +0000, Nishanth Menon wrote:
> >> Hi Sudeep,
> >>
> >> On 01/30/2014 07:43 AM, Sudeep Holla wrote:
> >>
> >> >
> >> > I am looking into a couple shortcomings in the current OPP bindings and
> >> > how to address them. Feel free to add to the list if you think of any more
> >> > issues that needs to be addressed or if and how any problem mentioned below
> >> > can be handled with the existing bindings.
> >> >
> >> > 1. indexing: currently there are no indices in the operating-points.
> >> indexing is based on frequency which is why the accessors use
> >> frequency to pull out the OPP data.
> >>
> >> indexing is a horrible idea - on platforms where OPP may be disabled
> >> or enabled for various reasons(see arch/arm/mach-imx/mach-imx6q.c,
> >> arch/arm/mach-omap2/board-omap3beagle.c etc) - the indexing you see in
> >> dts is just a myth that may not exist once the nodes are loaded and
> >> operated upon depending on SoC variations (example efuse describing
> >> which OPPs can be used and which not).
> >
> > I do not understand why. As long as a mapping from DT to data structures
> > in the kernel is done at boot, I can see how by indexing device nodes
> > can refer to specific OPPs. If they are disabled, amen, as long as we
> > can point at specific operating points that should be ok.
> >
> > Indexing does not mean that the index in the DT is the same as the one
> > allocated by the OS. Indexing is there to point at specific OPPs from
> > different DT nodes, a good example are clock bindings and that's exactly
> > how they work.
> 
> That is not a good comparison. With clocks, you are describing which
> physical signal coming out of a hardware block much like interrupts.
> Granted the h/w is not typically documented that way for clocks
> although the numbering could correspond to register bit locations as
> interrupt numbers do. With OPP indexes, they are a totally made up
> software construct.

Well ok, it might be, what I know is that current OPPs do not allow
other nodes to reference their properties properly, I am not sure that adding
an index make things worse, actually they are already broken as they are.

> Perhaps OPPs should be nodes so new fields can be added easily and
> then you have a phandle for each OPP.

Yeah, but the end result is the same, it has more to do with how to
express dependencies with DT than the question on whether to use a phandle
or an index. If we have to add phandles so be it, it is still just a way
to reference properties for me.

> > Can you provide me with an example where the indexing would go wrong
> > please ?
> 
> Putting s/w indexes into DT. Using cell-index for uarts to define
> their tty number was one example.

The idea was not related to using that index in the kernel at all, but
since you are all against using this mechanism, I think that's for a good
reason so we will not do that. How to do it, it has to be seen though.

> > Certainly relying on implicit ordering is not great either, actually I
> > would say that it is broken.
> >
> >> That said, the original OPP[1][2] series discussion started by trying
> >> to kill indexing precisely for the same reason.. once you have it - it
> >> becomes just crazy to deal with.
> >
> > I could not find any "index killing" :) discussion in there, but I will
> > keep reading.
> >
> >> >     It's assumed that the list is either in ascending or descending
> >> >     order of frequency but not explicit in the binding document.
> >> >     There are arch/arm/boot/dts/* files with opps in both styles.
> >> it should not matter -> opp library should do an insertion sort and
> >> organize it in ascending order once all the data is inserted. (line
> >> 449ish in opp.c)
> >
> > That's OS details and they must not be mandated by the bindings.
> > We cannot rely on word of mouth for things to work, I do not like
> > implicit rules, so the bindings should speficy the ordering or better
> > a way to reference OPPs.
> >
> >> if you see issues with the insertion sort not functioning, it is a bug
> >> and should be easy to track down and fix.
> >
> > Problem is not the insertion sort, problem is that DT bindings do not
> > define a way to refer to a specific OPP, unless we do that implicitly
> > and again, I rest my case, that is broken.
> >
> >> >     Few other bindings like thermal defines bindings like
> >> >     cooling-{min,max}-state assuming some order which is broken IMO.
> >> Now that you bring it up, I missed it :(.. yeah, I might have
> >> preferred it to be min frequency and max_frequency - I agree it is
> >> probably broken. I'd let Eduardo comment more about it.
> >>
> >> >
> >> >     One such use-case that came up recently[0] is the c-state latencies
> >> >     which could be different for each OPP. It would be good if the
> >> >     latencies are specified with the indices to OPP table to avoid
> >> >     inconsistency between the bindings.
> >>
> >> You can define C states based on frequencies as well - which really
> >> makes sense - since that sounds really like our constraint (say
> >> valid-at-frequency "xyz"
> >
> > I do not think that's generic enough, I do not like the idea of looking
> > up frequencies (eg a C-state target_residency does not depend on the
> > frequency only - ie voltage and other factors play a role, it really
> > depends on an operating point- looking it up by frequency is misleading IMO).
> 
> It seems we are trying to fit a square peg into a round hole here.

Do you mean that this is something not worth describing ? That it has to
be described differently ? Or that current DT semantics is not suitable for
describing it ?

We have to find a solution and we are keen on reworking it, otherwise we just
pretend there are no dependencies on the OPPs from other DT nodes, but I
am afraid that's already the case now.

Thank you,
Lorenzo

Extending OPP bindings

[Lorenzo Pieralisi]

Hi Rob,

thanks for having a look.

On Fri, Jan 31, 2014 at 05:17:51PM +0000, Rob Herring wrote:
> On Fri, Jan 31, 2014 at 6:46 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > Hi Nishanth,
> >
> > On Fri, Jan 31, 2014 at 12:43:54AM +0000, Nishanth Menon wrote:
> >> Hi Sudeep,
> >>
> >> On 01/30/2014 07:43 AM, Sudeep Holla wrote:
> >>
> >> >
> >> > I am looking into a couple shortcomings in the current OPP bindings and
> >> > how to address them. Feel free to add to the list if you think of any more
> >> > issues that needs to be addressed or if and how any problem mentioned below
> >> > can be handled with the existing bindings.
> >> >
> >> > 1. indexing: currently there are no indices in the operating-points.
> >> indexing is based on frequency which is why the accessors use
> >> frequency to pull out the OPP data.
> >>
> >> indexing is a horrible idea - on platforms where OPP may be disabled
> >> or enabled for various reasons(see arch/arm/mach-imx/mach-imx6q.c,
> >> arch/arm/mach-omap2/board-omap3beagle.c etc) - the indexing you see in
> >> dts is just a myth that may not exist once the nodes are loaded and
> >> operated upon depending on SoC variations (example efuse describing
> >> which OPPs can be used and which not).
> >
> > I do not understand why. As long as a mapping from DT to data structures
> > in the kernel is done at boot, I can see how by indexing device nodes
> > can refer to specific OPPs. If they are disabled, amen, as long as we
> > can point at specific operating points that should be ok.
> >
> > Indexing does not mean that the index in the DT is the same as the one
> > allocated by the OS. Indexing is there to point at specific OPPs from
> > different DT nodes, a good example are clock bindings and that's exactly
> > how they work.
> 
> That is not a good comparison. With clocks, you are describing which
> physical signal coming out of a hardware block much like interrupts.
> Granted the h/w is not typically documented that way for clocks
> although the numbering could correspond to register bit locations as
> interrupt numbers do. With OPP indexes, they are a totally made up
> software construct.

Well ok, it might be, what I know is that current OPPs do not allow
other nodes to reference their properties properly, I am not sure that adding
an index make things worse, actually they are already broken as they are.

> Perhaps OPPs should be nodes so new fields can be added easily and
> then you have a phandle for each OPP.

Yeah, but the end result is the same, it has more to do with how to
express dependencies with DT than the question on whether to use a phandle
or an index. If we have to add phandles so be it, it is still just a way
to reference properties for me.

> > Can you provide me with an example where the indexing would go wrong
> > please ?
> 
> Putting s/w indexes into DT. Using cell-index for uarts to define
> their tty number was one example.

The idea was not related to using that index in the kernel at all, but
since you are all against using this mechanism, I think that's for a good
reason so we will not do that. How to do it, it has to be seen though.

> > Certainly relying on implicit ordering is not great either, actually I
> > would say that it is broken.
> >
> >> That said, the original OPP[1][2] series discussion started by trying
> >> to kill indexing precisely for the same reason.. once you have it - it
> >> becomes just crazy to deal with.
> >
> > I could not find any "index killing" :) discussion in there, but I will
> > keep reading.
> >
> >> >     It's assumed that the list is either in ascending or descending
> >> >     order of frequency but not explicit in the binding document.
> >> >     There are arch/arm/boot/dts/* files with opps in both styles.
> >> it should not matter -> opp library should do an insertion sort and
> >> organize it in ascending order once all the data is inserted. (line
> >> 449ish in opp.c)
> >
> > That's OS details and they must not be mandated by the bindings.
> > We cannot rely on word of mouth for things to work, I do not like
> > implicit rules, so the bindings should speficy the ordering or better
> > a way to reference OPPs.
> >
> >> if you see issues with the insertion sort not functioning, it is a bug
> >> and should be easy to track down and fix.
> >
> > Problem is not the insertion sort, problem is that DT bindings do not
> > define a way to refer to a specific OPP, unless we do that implicitly
> > and again, I rest my case, that is broken.
> >
> >> >     Few other bindings like thermal defines bindings like
> >> >     cooling-{min,max}-state assuming some order which is broken IMO.
> >> Now that you bring it up, I missed it :(.. yeah, I might have
> >> preferred it to be min frequency and max_frequency - I agree it is
> >> probably broken. I'd let Eduardo comment more about it.
> >>
> >> >
> >> >     One such use-case that came up recently[0] is the c-state latencies
> >> >     which could be different for each OPP. It would be good if the
> >> >     latencies are specified with the indices to OPP table to avoid
> >> >     inconsistency between the bindings.
> >>
> >> You can define C states based on frequencies as well - which really
> >> makes sense - since that sounds really like our constraint (say
> >> valid-at-frequency "xyz"
> >
> > I do not think that's generic enough, I do not like the idea of looking
> > up frequencies (eg a C-state target_residency does not depend on the
> > frequency only - ie voltage and other factors play a role, it really
> > depends on an operating point- looking it up by frequency is misleading IMO).
> 
> It seems we are trying to fit a square peg into a round hole here.

Do you mean that this is something not worth describing ? That it has to
be described differently ? Or that current DT semantics is not suitable for
describing it ?

We have to find a solution and we are keen on reworking it, otherwise we just
pretend there are no dependencies on the OPPs from other DT nodes, but I
am afraid that's already the case now.

Thank you,
Lorenzo

Re: Extending OPP bindings

[Nishanth Menon]

On 01/31/2014 12:09 PM, Lorenzo Pieralisi wrote:
> Hi Rob,
> 
> thanks for having a look.
> 
> On Fri, Jan 31, 2014 at 05:17:51PM +0000, Rob Herring wrote:
>> On Fri, Jan 31, 2014 at 6:46 AM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>>> Hi Nishanth,
>>>
>>> On Fri, Jan 31, 2014 at 12:43:54AM +0000, Nishanth Menon wrote:
>>>> Hi Sudeep,
>>>>
>>>> On 01/30/2014 07:43 AM, Sudeep Holla wrote:
>>>>
>>>>>
>>>>> I am looking into a couple shortcomings in the current OPP bindings and
>>>>> how to address them. Feel free to add to the list if you think of any more
>>>>> issues that needs to be addressed or if and how any problem mentioned below
>>>>> can be handled with the existing bindings.
>>>>>
>>>>> 1. indexing: currently there are no indices in the operating-points.
>>>> indexing is based on frequency which is why the accessors use
>>>> frequency to pull out the OPP data.
>>>>
>>>> indexing is a horrible idea - on platforms where OPP may be disabled
>>>> or enabled for various reasons(see arch/arm/mach-imx/mach-imx6q.c,
>>>> arch/arm/mach-omap2/board-omap3beagle.c etc) - the indexing you see in
>>>> dts is just a myth that may not exist once the nodes are loaded and
>>>> operated upon depending on SoC variations (example efuse describing
>>>> which OPPs can be used and which not).
>>>
>>> I do not understand why. As long as a mapping from DT to data structures
>>> in the kernel is done at boot, I can see how by indexing device nodes
>>> can refer to specific OPPs. If they are disabled, amen, as long as we
>>> can point at specific operating points that should be ok.
>>>
>>> Indexing does not mean that the index in the DT is the same as the one
>>> allocated by the OS. Indexing is there to point at specific OPPs from
>>> different DT nodes, a good example are clock bindings and that's exactly
>>> how they work.
>>
>> That is not a good comparison. With clocks, you are describing which
>> physical signal coming out of a hardware block much like interrupts.
>> Granted the h/w is not typically documented that way for clocks
>> although the numbering could correspond to register bit locations as
>> interrupt numbers do. With OPP indexes, they are a totally made up
>> software construct.
> 
> Well ok, it might be, what I know is that current OPPs do not allow
> other nodes to reference their properties properly, I am not sure that adding
> an index make things worse, actually they are already broken as they are.

Let me look at this in two parts:
A: In kernel data representation:
If I were to use analogy of OPP library to database, A data base needs
a key to pick out data stored inside it -> the key in opp definition
as it stands right now is frequency. you can pick up any of the data
based on that key. proposal for using index into that table adds no
real value here.

Now, we can represent OPP data in many different forms. consider two
other properties per OPP (propx and propy)

data set #0:
   +------------+------------+---------+-------------+
   |   freq     | voltage    | propx   |  propy      |
   +-------------------------------------------------+
   | Fa         |  Va        |   PXa   |   PYa       |
   | Fb         |  Vb        |   PXb   |   PYb       |
   | Fc         |  Vc        |   PXc   |   PYc       |
   | Fd         |  Vd        |   PXd   |   PYd       |
   +------------+------------+---------+-------------+

Can then be represented as:
in opp library, considering it to be the least common denominator:
(data set #1)
   +------------+------------+
   |   freq     | voltage    |
   +--------------------------
   | Fa         |  Va        |
   | Fb         |  Vb        |
   | Fc         |  Vc        |
   | Fd         |  Vd        |
   +------------+------------+

where ever propx makes sense. (data set #2)
   +------------+---------+
   |   freq     | propx   |
   +-----------------------
   | Fa         |   PXa   |
   | Fb         |   PXb   |
   | Fc         |   PXc   |
   | Fd         |   PXd   |
   +------------+---------+

where ever propy makes sense (data set #3)
   +------------+-------------+
   |   freq     |  propy      |
   +--------------------------+
   | Fa         |   PYa       |
   | Fb         |   PYb       |
   | Fc         |   PYc       |
   | Fd         |   PYd       |
   +------------+-------------+

If my memory serves me right, in database terminology, this'd be first
normal form.

This also allows for data set #2 and #3 to modify or add a data set #4
with a propz at a later point in time without impacting set #1-3.

propx,y,z can be anything - efuse bits, cpuidle c state definition, etc..

As long as the key to the data sets are all the same (frequency),
information in data set #0 is maintained. It would be in our common
long term interest to maintain the split.

>> Perhaps OPPs should be nodes so new fields can be added easily and
>> then you have a phandle for each OPP.
> 
> Yeah, but the end result is the same, it has more to do with how to
> express dependencies with DT than the question on whether to use a phandle
> or an index. If we have to add phandles so be it, it is still just a way
> to reference properties for me.

B) Device tree representation of an OPP:
Here we have a ton of flexibility- I love the idea of representing
each OPP as a phandle - However, a phandle has been traditionally
meant to represent a device, I might be wrong, but i am not sure if we
have a precedence where phandle represents a property.

Having each OPP as a phandle does provide a lots of representation,
extensibility and reuse opportunity. but the same phandle will have to
be parsed by different drivers to pull out relevant data.

However when Sudeep introduced phandle approach, we did debate it's
representation and appropriate topology. if we can consider OPP as
it's own phandle and operating-points a list of OPP phandles, the pain
that we face in various driver usage can be simplified.


-- 
Regards,
Nishanth Menon

Extending OPP bindings

[Nishanth Menon]

On 01/31/2014 12:09 PM, Lorenzo Pieralisi wrote:
> Hi Rob,
> 
> thanks for having a look.
> 
> On Fri, Jan 31, 2014 at 05:17:51PM +0000, Rob Herring wrote:
>> On Fri, Jan 31, 2014 at 6:46 AM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>>> Hi Nishanth,
>>>
>>> On Fri, Jan 31, 2014 at 12:43:54AM +0000, Nishanth Menon wrote:
>>>> Hi Sudeep,
>>>>
>>>> On 01/30/2014 07:43 AM, Sudeep Holla wrote:
>>>>
>>>>>
>>>>> I am looking into a couple shortcomings in the current OPP bindings and
>>>>> how to address them. Feel free to add to the list if you think of any more
>>>>> issues that needs to be addressed or if and how any problem mentioned below
>>>>> can be handled with the existing bindings.
>>>>>
>>>>> 1. indexing: currently there are no indices in the operating-points.
>>>> indexing is based on frequency which is why the accessors use
>>>> frequency to pull out the OPP data.
>>>>
>>>> indexing is a horrible idea - on platforms where OPP may be disabled
>>>> or enabled for various reasons(see arch/arm/mach-imx/mach-imx6q.c,
>>>> arch/arm/mach-omap2/board-omap3beagle.c etc) - the indexing you see in
>>>> dts is just a myth that may not exist once the nodes are loaded and
>>>> operated upon depending on SoC variations (example efuse describing
>>>> which OPPs can be used and which not).
>>>
>>> I do not understand why. As long as a mapping from DT to data structures
>>> in the kernel is done at boot, I can see how by indexing device nodes
>>> can refer to specific OPPs. If they are disabled, amen, as long as we
>>> can point at specific operating points that should be ok.
>>>
>>> Indexing does not mean that the index in the DT is the same as the one
>>> allocated by the OS. Indexing is there to point at specific OPPs from
>>> different DT nodes, a good example are clock bindings and that's exactly
>>> how they work.
>>
>> That is not a good comparison. With clocks, you are describing which
>> physical signal coming out of a hardware block much like interrupts.
>> Granted the h/w is not typically documented that way for clocks
>> although the numbering could correspond to register bit locations as
>> interrupt numbers do. With OPP indexes, they are a totally made up
>> software construct.
> 
> Well ok, it might be, what I know is that current OPPs do not allow
> other nodes to reference their properties properly, I am not sure that adding
> an index make things worse, actually they are already broken as they are.

Let me look at this in two parts:
A: In kernel data representation:
If I were to use analogy of OPP library to database, A data base needs
a key to pick out data stored inside it -> the key in opp definition
as it stands right now is frequency. you can pick up any of the data
based on that key. proposal for using index into that table adds no
real value here.

Now, we can represent OPP data in many different forms. consider two
other properties per OPP (propx and propy)

data set #0:
   +------------+------------+---------+-------------+
   |   freq     | voltage    | propx   |  propy      |
   +-------------------------------------------------+
   | Fa         |  Va        |   PXa   |   PYa       |
   | Fb         |  Vb        |   PXb   |   PYb       |
   | Fc         |  Vc        |   PXc   |   PYc       |
   | Fd         |  Vd        |   PXd   |   PYd       |
   +------------+------------+---------+-------------+

Can then be represented as:
in opp library, considering it to be the least common denominator:
(data set #1)
   +------------+------------+
   |   freq     | voltage    |
   +--------------------------
   | Fa         |  Va        |
   | Fb         |  Vb        |
   | Fc         |  Vc        |
   | Fd         |  Vd        |
   +------------+------------+

where ever propx makes sense. (data set #2)
   +------------+---------+
   |   freq     | propx   |
   +-----------------------
   | Fa         |   PXa   |
   | Fb         |   PXb   |
   | Fc         |   PXc   |
   | Fd         |   PXd   |
   +------------+---------+

where ever propy makes sense (data set #3)
   +------------+-------------+
   |   freq     |  propy      |
   +--------------------------+
   | Fa         |   PYa       |
   | Fb         |   PYb       |
   | Fc         |   PYc       |
   | Fd         |   PYd       |
   +------------+-------------+

If my memory serves me right, in database terminology, this'd be first
normal form.

This also allows for data set #2 and #3 to modify or add a data set #4
with a propz at a later point in time without impacting set #1-3.

propx,y,z can be anything - efuse bits, cpuidle c state definition, etc..

As long as the key to the data sets are all the same (frequency),
information in data set #0 is maintained. It would be in our common
long term interest to maintain the split.

>> Perhaps OPPs should be nodes so new fields can be added easily and
>> then you have a phandle for each OPP.
> 
> Yeah, but the end result is the same, it has more to do with how to
> express dependencies with DT than the question on whether to use a phandle
> or an index. If we have to add phandles so be it, it is still just a way
> to reference properties for me.

B) Device tree representation of an OPP:
Here we have a ton of flexibility- I love the idea of representing
each OPP as a phandle - However, a phandle has been traditionally
meant to represent a device, I might be wrong, but i am not sure if we
have a precedence where phandle represents a property.

Having each OPP as a phandle does provide a lots of representation,
extensibility and reuse opportunity. but the same phandle will have to
be parsed by different drivers to pull out relevant data.

However when Sudeep introduced phandle approach, we did debate it's
representation and appropriate topology. if we can consider OPP as
it's own phandle and operating-points a list of OPP phandles, the pain
that we face in various driver usage can be simplified.


-- 
Regards,
Nishanth Menon

Re: Extending OPP bindings

[Mark Brown]

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

On Tue, Feb 04, 2014 at 12:01:11PM -0600, Nishanth Menon wrote:

> As long as the key to the data sets are all the same (frequency),
> information in data set #0 is maintained. It would be in our common
> long term interest to maintain the split.

You're assuming that the frequency is a unique key here.  That may not
be the case, for example two OPPs might have the same CPU clock
(assuming that's the frequency you're referring to) but different bus
clocking and of course the CPUs or CPU clusters might be individually
scalable (this is common in big.LITTLE designs I think).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Extending OPP bindings

[Mark Brown]

On Tue, Feb 04, 2014 at 12:01:11PM -0600, Nishanth Menon wrote:

> As long as the key to the data sets are all the same (frequency),
> information in data set #0 is maintained. It would be in our common
> long term interest to maintain the split.

You're assuming that the frequency is a unique key here.  That may not
be the case, for example two OPPs might have the same CPU clock
(assuming that's the frequency you're referring to) but different bus
clocking and of course the CPUs or CPU clusters might be individually
scalable (this is common in big.LITTLE designs I think).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140204/8dd2600f/attachment.sig>

Re: Extending OPP bindings

[Nishanth Menon]

On 02/04/2014 12:22 PM, Mark Brown wrote:
> On Tue, Feb 04, 2014 at 12:01:11PM -0600, Nishanth Menon wrote:
> 
>> As long as the key to the data sets are all the same (frequency),
>> information in data set #0 is maintained. It would be in our common
>> long term interest to maintain the split.
> 
> You're assuming that the frequency is a unique key here.  That may not
> be the case, for example two OPPs might have the same CPU clock
> (assuming that's the frequency you're referring to) but different bus
> clocking and of course the CPUs or CPU clusters might be individually
> scalable (this is common in big.LITTLE designs I think).
> 
Which is why OPPs are maintained per device, bus OPPs belong to bus
device (in TI terminology, we'd be talking of cross domain dependency
here for maintaining asynchronous bridge timing closure constraints -
but ofcourse, other SoCs may or maynot have such constraints). For
scaling bus frequency, we already have infrastructure in place - clock
notifiers - discussion of using that is much deeper topic of it's own.

for each processor that is uniquely transitioning, we'd have it's own
sets of OPPs - the correct representation of the device node is the
key there.

-- 
Regards,
Nishanth Menon

Extending OPP bindings

[Nishanth Menon]

On 02/04/2014 12:22 PM, Mark Brown wrote:
> On Tue, Feb 04, 2014 at 12:01:11PM -0600, Nishanth Menon wrote:
> 
>> As long as the key to the data sets are all the same (frequency),
>> information in data set #0 is maintained. It would be in our common
>> long term interest to maintain the split.
> 
> You're assuming that the frequency is a unique key here.  That may not
> be the case, for example two OPPs might have the same CPU clock
> (assuming that's the frequency you're referring to) but different bus
> clocking and of course the CPUs or CPU clusters might be individually
> scalable (this is common in big.LITTLE designs I think).
> 
Which is why OPPs are maintained per device, bus OPPs belong to bus
device (in TI terminology, we'd be talking of cross domain dependency
here for maintaining asynchronous bridge timing closure constraints -
but ofcourse, other SoCs may or maynot have such constraints). For
scaling bus frequency, we already have infrastructure in place - clock
notifiers - discussion of using that is much deeper topic of it's own.

for each processor that is uniquely transitioning, we'd have it's own
sets of OPPs - the correct representation of the device node is the
key there.

-- 
Regards,
Nishanth Menon

Re: Extending OPP bindings

[Mark Brown]

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

On Tue, Feb 04, 2014 at 01:28:20PM -0600, Nishanth Menon wrote:
> On 02/04/2014 12:22 PM, Mark Brown wrote:

> > You're assuming that the frequency is a unique key here.  That may not
> > be the case, for example two OPPs might have the same CPU clock
> > (assuming that's the frequency you're referring to) but different bus
> > clocking and of course the CPUs or CPU clusters might be individually
> > scalable (this is common in big.LITTLE designs I think).

> Which is why OPPs are maintained per device, bus OPPs belong to bus
> device (in TI terminology, we'd be talking of cross domain dependency
> here for maintaining asynchronous bridge timing closure constraints -
> but ofcourse, other SoCs may or maynot have such constraints). For
> scaling bus frequency, we already have infrastructure in place - clock
> notifiers - discussion of using that is much deeper topic of it's own.

> for each processor that is uniquely transitioning, we'd have it's own
> sets of OPPs - the correct representation of the device node is the
> key there.

I've seen some SoCs characterised over the whole device rather than with
individual parts of the SoC done separately.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Extending OPP bindings

[Mark Brown]

On Tue, Feb 04, 2014 at 01:28:20PM -0600, Nishanth Menon wrote:
> On 02/04/2014 12:22 PM, Mark Brown wrote:

> > You're assuming that the frequency is a unique key here.  That may not
> > be the case, for example two OPPs might have the same CPU clock
> > (assuming that's the frequency you're referring to) but different bus
> > clocking and of course the CPUs or CPU clusters might be individually
> > scalable (this is common in big.LITTLE designs I think).

> Which is why OPPs are maintained per device, bus OPPs belong to bus
> device (in TI terminology, we'd be talking of cross domain dependency
> here for maintaining asynchronous bridge timing closure constraints -
> but ofcourse, other SoCs may or maynot have such constraints). For
> scaling bus frequency, we already have infrastructure in place - clock
> notifiers - discussion of using that is much deeper topic of it's own.

> for each processor that is uniquely transitioning, we'd have it's own
> sets of OPPs - the correct representation of the device node is the
> key there.

I've seen some SoCs characterised over the whole device rather than with
individual parts of the SoC done separately.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140204/90067f8e/attachment.sig>

Re: Extending OPP bindings

[Nishanth Menon]

On 02/04/2014 02:11 PM, Mark Brown wrote:
> On Tue, Feb 04, 2014 at 01:28:20PM -0600, Nishanth Menon wrote:
>> On 02/04/2014 12:22 PM, Mark Brown wrote:
> 
>>> You're assuming that the frequency is a unique key here.  That may not
>>> be the case, for example two OPPs might have the same CPU clock
>>> (assuming that's the frequency you're referring to) but different bus
>>> clocking and of course the CPUs or CPU clusters might be individually
>>> scalable (this is common in big.LITTLE designs I think).
> 
>> Which is why OPPs are maintained per device, bus OPPs belong to bus
>> device (in TI terminology, we'd be talking of cross domain dependency
>> here for maintaining asynchronous bridge timing closure constraints -
>> but ofcourse, other SoCs may or maynot have such constraints). For
>> scaling bus frequency, we already have infrastructure in place - clock
>> notifiers - discussion of using that is much deeper topic of it's own.
> 
>> for each processor that is uniquely transitioning, we'd have it's own
>> sets of OPPs - the correct representation of the device node is the
>> key there.
> 
> I've seen some SoCs characterised over the whole device rather than with
> individual parts of the SoC done separately.
> 
Fair enough - however, the data characterized will imply individual
processor/bus specific tuples/parameters - the specific parameters
might be very unique for SoC, but we have ability to abstract it per
SoC already.

-- 
Regards,
Nishanth Menon

Extending OPP bindings

[Nishanth Menon]

On 02/04/2014 02:11 PM, Mark Brown wrote:
> On Tue, Feb 04, 2014 at 01:28:20PM -0600, Nishanth Menon wrote:
>> On 02/04/2014 12:22 PM, Mark Brown wrote:
> 
>>> You're assuming that the frequency is a unique key here.  That may not
>>> be the case, for example two OPPs might have the same CPU clock
>>> (assuming that's the frequency you're referring to) but different bus
>>> clocking and of course the CPUs or CPU clusters might be individually
>>> scalable (this is common in big.LITTLE designs I think).
> 
>> Which is why OPPs are maintained per device, bus OPPs belong to bus
>> device (in TI terminology, we'd be talking of cross domain dependency
>> here for maintaining asynchronous bridge timing closure constraints -
>> but ofcourse, other SoCs may or maynot have such constraints). For
>> scaling bus frequency, we already have infrastructure in place - clock
>> notifiers - discussion of using that is much deeper topic of it's own.
> 
>> for each processor that is uniquely transitioning, we'd have it's own
>> sets of OPPs - the correct representation of the device node is the
>> key there.
> 
> I've seen some SoCs characterised over the whole device rather than with
> individual parts of the SoC done separately.
> 
Fair enough - however, the data characterized will imply individual
processor/bus specific tuples/parameters - the specific parameters
might be very unique for SoC, but we have ability to abstract it per
SoC already.

-- 
Regards,
Nishanth Menon