diff mbox series

[6/9] PM / OPP: dt-bindings: Add opp-interconnect-bw

Message ID 20180827151112.25211-7-jcrouse@codeaurora.org (mailing list archive)
State Not Applicable, archived
Delegated to: Andy Gross
Headers show
Series Add interconnect support + bindings for A630 GPU | expand

Commit Message

Jordan Crouse Aug. 27, 2018, 3:11 p.m. UTC
Add the "opp-interconnect-bw" property to specify the
average and peak bandwidth for an interconnect path for
a specific operating power point. A separate bandwidth
pair can be specified for each of the interconnects
defined for the device by appending the interconnect
name to the property.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 Documentation/devicetree/bindings/opp/opp.txt | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

Comments

Georgi Djakov Sept. 27, 2018, 8:23 a.m. UTC | #1
Hi Jordan,

Thanks for the patch!

On 08/27/2018 06:11 PM, Jordan Crouse wrote:
> Add the "opp-interconnect-bw" property to specify the
> average and peak bandwidth for an interconnect path for
> a specific operating power point. A separate bandwidth
> pair can be specified for each of the interconnects
> defined for the device by appending the interconnect
> name to the property.
> 
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/opp/opp.txt | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> index c396c4c0af92..d714c084f36d 100644
> --- a/Documentation/devicetree/bindings/opp/opp.txt
> +++ b/Documentation/devicetree/bindings/opp/opp.txt
> @@ -170,6 +170,11 @@ Optional properties:
>    functioning of the current device at the current OPP (where this property is
>    present).
>  
> +- opp-interconnect-bw-<name>: This is an array of pairs specifying the average
> +  and peak bandwidth in bytes per second for the interconnect path known by
> +  'name'.  This should match the name(s) specified by interconnect-names in the
> +  device definition.
> +
>  Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together.
>  
>  / {
> @@ -543,3 +548,34 @@ Example 6: opp-microvolt-<name>, opp-microamp-<name>:
>  		};
>  	};
>  };
> +
> +Example 7: opp-interconnect-bw:
> +(example: leaf device with frequency and BW quotas)
> +
> +/ {
> +	soc {
> +		gpu@5000000 {
> +			...
> +			interconnects = <&qnoc 26 &qnoc 512>;
> +			interconnect-names = "port0";
> +			...
> +			operating-points-v2 = <&gpu_opp_table>;
> +		};
> +	};
> +
> +	gpu_opp_table: opp_table0 {
> +		compatible = "operating-points-v2";
> +
> +		opp-710000000 {
> +			op-hz = /bits/ 64 <710000000>;
> +			/* Set peak bandwidth @ 7216000 KB/s */
> +			opp-interconnect-bw-port0 = /bits/ 64 <0 7216000000>;

This seems a bit long. I would suggest the following instead.
If there is only one path:
	/* average bandwidth = 0 KB/s,  peak bandwidth = 7216000 KB/s */
	opp-bw-KBps = <0 7216000>;
or 	
	opp-bw-MBps = <0 7216>;

If there are multiple paths:
	opp-bw-KBps-port0 = <0 7216000>;
	opp-bw-KBps-port1 = <0 1000000>;

The above follows a convention similar to opp-microvolt, where at
runtime the platform can pick a <name> and a matching opp-bw-KBps-<name>
property. If the platform doesn't pick a specific <name> or <name> does
not match with any of the opp-bw-KBps-<name> properties, then
opp-bw-KBps shall be used if present.
For now supporting only KBps values seems enough to cover all present
platforms, so we can start with this and based on the requirements of
future platforms we can add MBps etc.

Thanks,
Georgi

> +		};
> +
> +		opp-210000000 {
> +			op-hz = /bits/ 64 <210000000>;
> +			/* Set peak bandwidth @ 1200000 KB/s */
> +			opp-interconnect-bw-port0 = /bits/ 64 <0 1200000000>;
> +		};
> +	};
> +};
>
Viresh Kumar Oct. 10, 2018, 9:59 a.m. UTC | #2
On 27-09-18, 11:23, Georgi Djakov wrote:
> On 08/27/2018 06:11 PM, Jordan Crouse wrote:
> > Add the "opp-interconnect-bw" property to specify the
> > average and peak bandwidth for an interconnect path for
> > a specific operating power point. A separate bandwidth
> > pair can be specified for each of the interconnects
> > defined for the device by appending the interconnect
> > name to the property.
> > 
> > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> > ---
> >  Documentation/devicetree/bindings/opp/opp.txt | 36 +++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> > index c396c4c0af92..d714c084f36d 100644
> > --- a/Documentation/devicetree/bindings/opp/opp.txt
> > +++ b/Documentation/devicetree/bindings/opp/opp.txt
> > @@ -170,6 +170,11 @@ Optional properties:
> >    functioning of the current device at the current OPP (where this property is
> >    present).
> >  
> > +- opp-interconnect-bw-<name>: This is an array of pairs specifying the average
> > +  and peak bandwidth in bytes per second for the interconnect path known by
> > +  'name'.  This should match the name(s) specified by interconnect-names in the
> > +  device definition.
> > +
> >  Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together.
> >  
> >  / {
> > @@ -543,3 +548,34 @@ Example 6: opp-microvolt-<name>, opp-microamp-<name>:
> >  		};
> >  	};
> >  };
> > +
> > +Example 7: opp-interconnect-bw:
> > +(example: leaf device with frequency and BW quotas)
> > +
> > +/ {
> > +	soc {
> > +		gpu@5000000 {
> > +			...
> > +			interconnects = <&qnoc 26 &qnoc 512>;
> > +			interconnect-names = "port0";
> > +			...
> > +			operating-points-v2 = <&gpu_opp_table>;
> > +		};
> > +	};
> > +
> > +	gpu_opp_table: opp_table0 {
> > +		compatible = "operating-points-v2";
> > +
> > +		opp-710000000 {
> > +			op-hz = /bits/ 64 <710000000>;
> > +			/* Set peak bandwidth @ 7216000 KB/s */
> > +			opp-interconnect-bw-port0 = /bits/ 64 <0 7216000000>;
> 
> This seems a bit long. I would suggest the following instead.
> If there is only one path:
> 	/* average bandwidth = 0 KB/s,  peak bandwidth = 7216000 KB/s */
> 	opp-bw-KBps = <0 7216000>;
> or 	
> 	opp-bw-MBps = <0 7216>;
> 
> If there are multiple paths:
> 	opp-bw-KBps-port0 = <0 7216000>;
> 	opp-bw-KBps-port1 = <0 1000000>;
> 
> The above follows a convention similar to opp-microvolt, where at
> runtime the platform can pick a <name> and a matching opp-bw-KBps-<name>
> property. If the platform doesn't pick a specific <name> or <name> does
> not match with any of the opp-bw-KBps-<name> properties, then
> opp-bw-KBps shall be used if present.
> For now supporting only KBps values seems enough to cover all present
> platforms, so we can start with this and based on the requirements of
> future platforms we can add MBps etc.

+1

And yes I am fine with such bindings getting introduced to the OPP
core.

I am not sure if this would fit well, but have a look at
"required-opps" property in OPP bindings and see if that can be used
instead of adding new bindings here. Again, I am not sure if that
should be done :)
Jordan Crouse Oct. 10, 2018, 2:27 p.m. UTC | #3
On Wed, Oct 10, 2018 at 03:29:51PM +0530, Viresh Kumar wrote:
> On 27-09-18, 11:23, Georgi Djakov wrote:
> > On 08/27/2018 06:11 PM, Jordan Crouse wrote:
> > > Add the "opp-interconnect-bw" property to specify the
> > > average and peak bandwidth for an interconnect path for
> > > a specific operating power point. A separate bandwidth
> > > pair can be specified for each of the interconnects
> > > defined for the device by appending the interconnect
> > > name to the property.
> > > 
> > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> > > ---
> > >  Documentation/devicetree/bindings/opp/opp.txt | 36 +++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> > > index c396c4c0af92..d714c084f36d 100644
> > > --- a/Documentation/devicetree/bindings/opp/opp.txt
> > > +++ b/Documentation/devicetree/bindings/opp/opp.txt
> > > @@ -170,6 +170,11 @@ Optional properties:
> > >    functioning of the current device at the current OPP (where this property is
> > >    present).
> > >  
> > > +- opp-interconnect-bw-<name>: This is an array of pairs specifying the average
> > > +  and peak bandwidth in bytes per second for the interconnect path known by
> > > +  'name'.  This should match the name(s) specified by interconnect-names in the
> > > +  device definition.
> > > +
> > >  Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together.
> > >  
> > >  / {
> > > @@ -543,3 +548,34 @@ Example 6: opp-microvolt-<name>, opp-microamp-<name>:
> > >  		};
> > >  	};
> > >  };
> > > +
> > > +Example 7: opp-interconnect-bw:
> > > +(example: leaf device with frequency and BW quotas)
> > > +
> > > +/ {
> > > +	soc {
> > > +		gpu@5000000 {
> > > +			...
> > > +			interconnects = <&qnoc 26 &qnoc 512>;
> > > +			interconnect-names = "port0";
> > > +			...
> > > +			operating-points-v2 = <&gpu_opp_table>;
> > > +		};
> > > +	};
> > > +
> > > +	gpu_opp_table: opp_table0 {
> > > +		compatible = "operating-points-v2";
> > > +
> > > +		opp-710000000 {
> > > +			op-hz = /bits/ 64 <710000000>;
> > > +			/* Set peak bandwidth @ 7216000 KB/s */
> > > +			opp-interconnect-bw-port0 = /bits/ 64 <0 7216000000>;
> > 
> > This seems a bit long. I would suggest the following instead.
> > If there is only one path:
> > 	/* average bandwidth = 0 KB/s,  peak bandwidth = 7216000 KB/s */
> > 	opp-bw-KBps = <0 7216000>;
> > or 	
> > 	opp-bw-MBps = <0 7216>;
> > 
> > If there are multiple paths:
> > 	opp-bw-KBps-port0 = <0 7216000>;
> > 	opp-bw-KBps-port1 = <0 1000000>;
> > 
> > The above follows a convention similar to opp-microvolt, where at
> > runtime the platform can pick a <name> and a matching opp-bw-KBps-<name>
> > property. If the platform doesn't pick a specific <name> or <name> does
> > not match with any of the opp-bw-KBps-<name> properties, then
> > opp-bw-KBps shall be used if present.
> > For now supporting only KBps values seems enough to cover all present
> > platforms, so we can start with this and based on the requirements of
> > future platforms we can add MBps etc.
> 
> +1
> 
> And yes I am fine with such bindings getting introduced to the OPP
> core.
> 
> I am not sure if this would fit well, but have a look at
> "required-opps" property in OPP bindings and see if that can be used
> instead of adding new bindings here. Again, I am not sure if that
> should be done :)

I'm not convinced that required-opps would work. I'm worried that the
format is too vague if we need to use it for multiple paths and possibly
other uses too, consider this:

required-opp = <&mdp_path0_opp3, &mdp_path1_opp5, &mdp_rpmh_opp1>;

This has ordering problems and IMO pollutes the DT namespace for no
great technical advantage. I appreciate the hesitation for opening up
the flood gates for new OPP bindings but we are entering a new era
of hyper power aware devices and some concessions will need to be made.

Jordan
Viresh Kumar Oct. 10, 2018, 2:29 p.m. UTC | #4
On 10-10-18, 08:27, Jordan Crouse wrote:
> I'm not convinced that required-opps would work. I'm worried that the
> format is too vague if we need to use it for multiple paths and possibly
> other uses too, consider this:
> 
> required-opp = <&mdp_path0_opp3, &mdp_path1_opp5, &mdp_rpmh_opp1>;
> 
> This has ordering problems and IMO pollutes the DT namespace for no
> great technical advantage. I appreciate the hesitation for opening up
> the flood gates for new OPP bindings but we are entering a new era
> of hyper power aware devices and some concessions will need to be made.

Sure.
Rob Herring (Arm) Oct. 15, 2018, 2:34 p.m. UTC | #5
On Mon, Aug 27, 2018 at 09:11:09AM -0600, Jordan Crouse wrote:
> Add the "opp-interconnect-bw" property to specify the
> average and peak bandwidth for an interconnect path for
> a specific operating power point. A separate bandwidth
> pair can be specified for each of the interconnects
> defined for the device by appending the interconnect
> name to the property.
> 
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/opp/opp.txt | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> index c396c4c0af92..d714c084f36d 100644
> --- a/Documentation/devicetree/bindings/opp/opp.txt
> +++ b/Documentation/devicetree/bindings/opp/opp.txt
> @@ -170,6 +170,11 @@ Optional properties:
>    functioning of the current device at the current OPP (where this property is
>    present).
>  
> +- opp-interconnect-bw-<name>: This is an array of pairs specifying the average
> +  and peak bandwidth in bytes per second for the interconnect path known by
> +  'name'.  This should match the name(s) specified by interconnect-names in the
> +  device definition.
> +

I don't think this is good design with the name defined in one node and 
then used in the OPP table. First, '*-names' is typically the a name 
local to that node/device. If you had 2 instances of a device with a 
shared OPP table for the 2 instances, then you are going to have to 
make the names unique. Second, how exactly would having multiple b/w 
entries work? A given OPP frequency supports the sum of the b/w entries? 
What if some devices for b/w entries aren't currently active?

This also seems like a mixture of using OPP table for setting GPU's 
bandwidth/freq and then the interconnect binding to set the 
interconnect's bandwidth. Perhaps we need a more unified approach. Not 
sure what that would look like though.

Rob
Jordan Crouse Oct. 15, 2018, 3:12 p.m. UTC | #6
On Mon, Oct 15, 2018 at 09:34:20AM -0500, Rob Herring wrote:
> On Mon, Aug 27, 2018 at 09:11:09AM -0600, Jordan Crouse wrote:
> > Add the "opp-interconnect-bw" property to specify the
> > average and peak bandwidth for an interconnect path for
> > a specific operating power point. A separate bandwidth
> > pair can be specified for each of the interconnects
> > defined for the device by appending the interconnect
> > name to the property.
> > 
> > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> > ---
> >  Documentation/devicetree/bindings/opp/opp.txt | 36 +++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> > index c396c4c0af92..d714c084f36d 100644
> > --- a/Documentation/devicetree/bindings/opp/opp.txt
> > +++ b/Documentation/devicetree/bindings/opp/opp.txt
> > @@ -170,6 +170,11 @@ Optional properties:
> >    functioning of the current device at the current OPP (where this property is
> >    present).
> >  
> > +- opp-interconnect-bw-<name>: This is an array of pairs specifying the average
> > +  and peak bandwidth in bytes per second for the interconnect path known by
> > +  'name'.  This should match the name(s) specified by interconnect-names in the
> > +  device definition.
> > +
> 
> I don't think this is good design with the name defined in one node and 
> then used in the OPP table. First, '*-names' is typically the a name 
> local to that node/device. If you had 2 instances of a device with a 
> shared OPP table for the 2 instances, then you are going to have to 
> make the names unique. Second, how exactly would having multiple b/w 
> entries work? A given OPP frequency supports the sum of the b/w entries? 
> What if some devices for b/w entries aren't currently active?

For device that is fully scalable a given OPP frequency could need a
different quota for multiple interconnect entries - each one would be
programmed individually, something like this:

set_opp() {
    set_device_frequency(opp);
    for each interconnect name {
        get_device_bw(opp, name);
	icc_set(name, bw);
    }
}

And the naming scheme gives you the flexibility to choose the icc names
you need.  For example if port0 was always fixed and port1 scaled you could
specify the b/w for port1 without having to also have a dummy vote for port0.

The main reason I included the names was that I felt it had a potential to
be confusing if we used absolute indexes or some other implied scheme - at
least for the driver developer it makes slightly more sense to look up the
bandwidth for "port0" with the same name you used to register the interconnect.

> This also seems like a mixture of using OPP table for setting GPU's 
> bandwidth/freq and then the interconnect binding to set the 
> interconnect's bandwidth. Perhaps we need a more unified approach. Not 
> sure what that would look like though.

In a perfect world the interconnect(s) would be left to scale on their own
and they wouldn't be tied to an individual device frequency. But that would
involve significantly more infrastructure for a given device.

Also in RE to our other discussion with Viresh on the qcom,level for the
GPU; the GPU frequency vote is done by the microcontroller but the
interconnect bandwidth is set by the CPU on the sdm845 so we're already
not unified by design. There could be something out there that uses
magic and callbacks but I also don't know what that looks like.

Jordan
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index c396c4c0af92..d714c084f36d 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -170,6 +170,11 @@  Optional properties:
   functioning of the current device at the current OPP (where this property is
   present).
 
+- opp-interconnect-bw-<name>: This is an array of pairs specifying the average
+  and peak bandwidth in bytes per second for the interconnect path known by
+  'name'.  This should match the name(s) specified by interconnect-names in the
+  device definition.
+
 Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together.
 
 / {
@@ -543,3 +548,34 @@  Example 6: opp-microvolt-<name>, opp-microamp-<name>:
 		};
 	};
 };
+
+Example 7: opp-interconnect-bw:
+(example: leaf device with frequency and BW quotas)
+
+/ {
+	soc {
+		gpu@5000000 {
+			...
+			interconnects = <&qnoc 26 &qnoc 512>;
+			interconnect-names = "port0";
+			...
+			operating-points-v2 = <&gpu_opp_table>;
+		};
+	};
+
+	gpu_opp_table: opp_table0 {
+		compatible = "operating-points-v2";
+
+		opp-710000000 {
+			op-hz = /bits/ 64 <710000000>;
+			/* Set peak bandwidth @ 7216000 KB/s */
+			opp-interconnect-bw-port0 = /bits/ 64 <0 7216000000>;
+		};
+
+		opp-210000000 {
+			op-hz = /bits/ 64 <210000000>;
+			/* Set peak bandwidth @ 1200000 KB/s */
+			opp-interconnect-bw-port0 = /bits/ 64 <0 1200000000>;
+		};
+	};
+};