| 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 |
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>; > + }; > + }; > +}; >
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 :)
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
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.
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
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 --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>; + }; + }; +};
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(+)