diff mbox series

[v6,2/2] arm64: dts: sdm845: Add gpu and gmu device nodes

Message ID 20181212211848.26768-3-jcrouse@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series arm64: dts: Add sdm845 GPU/GMU and SMMU | expand

Commit Message

Jordan Crouse Dec. 12, 2018, 9:18 p.m. UTC
Add the nodes to describe the Adreno GPU and GMU devices.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 123 +++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

Comments

Doug Anderson Dec. 13, 2018, 6:40 p.m. UTC | #1
Hi,

On Wed, Dec 12, 2018 at 1:18 PM Jordan Crouse <jcrouse@codeaurora.org> wrote:
>
> Add the nodes to describe the Adreno GPU and GMU devices.
>
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 123 +++++++++++++++++++++++++++
>  1 file changed, 123 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 233a5898ebc2..a608afed502e 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -11,6 +11,7 @@
>  #include <dt-bindings/clock/qcom,rpmh.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/phy/phy-qcom-qusb2.h>
> +#include <dt-bindings/power/qcom-rpmpd.h>
>  #include <dt-bindings/reset/qcom,sdm845-aoss.h>
>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
>  #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> @@ -1349,6 +1350,128 @@
>                         };
>                 };
>
> +
> +               gpu@5000000 {

nit that you're adding an extra blank line here that you don't need.
Given the quantity of outstanding dts patches though, it's almost
certain that Andy will need to manually resolve conflicts when
applying this patch so presumably he can fix up when he lands.

In any case, feel free to add:

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>

-Doug
Viresh Kumar Dec. 14, 2018, 4:40 a.m. UTC | #2
On 12-12-18, 14:18, Jordan Crouse wrote:
> +			gpu_opp_table: opp-table {
> +				compatible = "operating-points-v2-qcom-level";

I think you need to mention "operating-points-v2" as well here.
Doug Anderson Dec. 14, 2018, 5:04 p.m. UTC | #3
Hi,

On Thu, Dec 13, 2018 at 8:41 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 12-12-18, 14:18, Jordan Crouse wrote:
> > +                     gpu_opp_table: opp-table {
> > +                             compatible = "operating-points-v2-qcom-level";
>
> I think you need to mention "operating-points-v2" as well here.

Are you saying the above should be:
  compatible = "operating-points-v2-qcom-level", "operating-points-v2";

If so I'm not sure I agree.  It's _not_ really compatible with the
"operating-points-v2" binding.  If you had a driver that had never
heard of "operating-points-v2-qcom-level" and had only heard of
"operating-points-v2" and it took a look at this node it would have no
idea what to do with it.

I'll also note that other instances posted to the list don't list both:

https://patchwork.kernel.org/patch/10725801/ - RPM / RPMH PD bindings
https://patchwork.kernel.org/patch/10725793/ - RPMH PD device tree for sdm845

The bindings patch also makes no mention of needing
"operating-points-v2".  I think the common case when a fallback is
required it is explicitly called out in the bindings:

https://patchwork.kernel.org/patch/10725803/ - qcom-opp bindings


-Doug
Viresh Kumar Dec. 17, 2018, 7:06 a.m. UTC | #4
+Rob +Stephen,

On 14-12-18, 09:04, Doug Anderson wrote:
> Hi,
> 
> On Thu, Dec 13, 2018 at 8:41 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 12-12-18, 14:18, Jordan Crouse wrote:
> > > +                     gpu_opp_table: opp-table {
> > > +                             compatible = "operating-points-v2-qcom-level";
> >
> > I think you need to mention "operating-points-v2" as well here.
> 
> Are you saying the above should be:
>   compatible = "operating-points-v2-qcom-level", "operating-points-v2";
> 
> If so I'm not sure I agree.

Well I have my doubts as well on this. This is where the ordering was discussed
earlier:

https://lore.kernel.org/lkml/152328979897.180276.15963925877442657158@swboyd.mtv.corp.google.com/

@Rob/Stephen: Should the opp-table node above also have "operating-points-v2"
string in the compatible property ?

> It's _not_ really compatible with the
> "operating-points-v2" binding.  If you had a driver that had never
> heard of "operating-points-v2-qcom-level" and had only heard of
> "operating-points-v2" and it took a look at this node it would have no
> idea what to do with it.

Well it will parse everything apart from the qcom,level thing, so it can
actually parse stuff here.

> I'll also note that other instances posted to the list don't list both:
> 
> https://patchwork.kernel.org/patch/10725801/ - RPM / RPMH PD bindings
> https://patchwork.kernel.org/patch/10725793/ - RPMH PD device tree for sdm845
> 
> The bindings patch also makes no mention of needing
> "operating-points-v2".  I think the common case when a fallback is
> required it is explicitly called out in the bindings:
> 
> https://patchwork.kernel.org/patch/10725803/ - qcom-opp bindings

Sure, maybe I am wrong but its better to get some clarity on it from Rob/Stephen.
Doug Anderson Dec. 18, 2018, 12:34 a.m. UTC | #5
Hi,

On Sun, Dec 16, 2018 at 11:06 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> +Rob +Stephen,
>
> On 14-12-18, 09:04, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Dec 13, 2018 at 8:41 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 12-12-18, 14:18, Jordan Crouse wrote:
> > > > +                     gpu_opp_table: opp-table {
> > > > +                             compatible = "operating-points-v2-qcom-level";
> > >
> > > I think you need to mention "operating-points-v2" as well here.
> >
> > Are you saying the above should be:
> >   compatible = "operating-points-v2-qcom-level", "operating-points-v2";
> >
> > If so I'm not sure I agree.
>
> Well I have my doubts as well on this. This is where the ordering was discussed
> earlier:
>
> https://lore.kernel.org/lkml/152328979897.180276.15963925877442657158@swboyd.mtv.corp.google.com/
>
> @Rob/Stephen: Should the opp-table node above also have "operating-points-v2"
> string in the compatible property ?
>
> > It's _not_ really compatible with the
> > "operating-points-v2" binding.  If you had a driver that had never
> > heard of "operating-points-v2-qcom-level" and had only heard of
> > "operating-points-v2" and it took a look at this node it would have no
> > idea what to do with it.
>
> Well it will parse everything apart from the qcom,level thing, so it can
> actually parse stuff here.
>
> > I'll also note that other instances posted to the list don't list both:
> >
> > https://patchwork.kernel.org/patch/10725801/ - RPM / RPMH PD bindings
> > https://patchwork.kernel.org/patch/10725793/ - RPMH PD device tree for sdm845
> >
> > The bindings patch also makes no mention of needing
> > "operating-points-v2".  I think the common case when a fallback is
> > required it is explicitly called out in the bindings:
> >
> > https://patchwork.kernel.org/patch/10725803/ - qcom-opp bindings
>
> Sure, maybe I am wrong but its better to get some clarity on it from Rob/Stephen.

In case it helps:

https://lkml.kernel.org/r/20181217210849.GA15490@bogus

...shows Rob giving a Reviewed-by with just

  compatible = "operating-points-v2-qcom-level";

...and not:

  compatible = "operating-points-v2-qcom-level", "operating-points-v2";


-Doug
Stephen Boyd Dec. 18, 2018, 6:40 p.m. UTC | #6
Quoting Doug Anderson (2018-12-17 16:34:31)
> Hi,
> 
> On Sun, Dec 16, 2018 at 11:06 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > +Rob +Stephen,
> >
> > On 14-12-18, 09:04, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Thu, Dec 13, 2018 at 8:41 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >
> > > > On 12-12-18, 14:18, Jordan Crouse wrote:
> > > > > +                     gpu_opp_table: opp-table {
> > > > > +                             compatible = "operating-points-v2-qcom-level";
> > > >
> > > > I think you need to mention "operating-points-v2" as well here.
> > >
> > > Are you saying the above should be:
> > >   compatible = "operating-points-v2-qcom-level", "operating-points-v2";
> > >
> > > If so I'm not sure I agree.
> >
> > Well I have my doubts as well on this. This is where the ordering was discussed
> > earlier:
> >
> > https://lore.kernel.org/lkml/152328979897.180276.15963925877442657158@swboyd.mtv.corp.google.com/
> >
> > @Rob/Stephen: Should the opp-table node above also have "operating-points-v2"
> > string in the compatible property ?
> >
> > > It's _not_ really compatible with the
> > > "operating-points-v2" binding.  If you had a driver that had never
> > > heard of "operating-points-v2-qcom-level" and had only heard of
> > > "operating-points-v2" and it took a look at this node it would have no
> > > idea what to do with it.
> >
> > Well it will parse everything apart from the qcom,level thing, so it can
> > actually parse stuff here.
> >
> > > I'll also note that other instances posted to the list don't list both:
> > >
> > > https://patchwork.kernel.org/patch/10725801/ - RPM / RPMH PD bindings
> > > https://patchwork.kernel.org/patch/10725793/ - RPMH PD device tree for sdm845
> > >
> > > The bindings patch also makes no mention of needing
> > > "operating-points-v2".  I think the common case when a fallback is
> > > required it is explicitly called out in the bindings:
> > >
> > > https://patchwork.kernel.org/patch/10725803/ - qcom-opp bindings
> >
> > Sure, maybe I am wrong but its better to get some clarity on it from Rob/Stephen.
> 
> In case it helps:
> 
> https://lkml.kernel.org/r/20181217210849.GA15490@bogus
> 
> ...shows Rob giving a Reviewed-by with just
> 
>   compatible = "operating-points-v2-qcom-level";
> 
> ...and not:
> 
>   compatible = "operating-points-v2-qcom-level", "operating-points-v2";
> 

I don't see a problem if both compatible strings are there. Does that
change anything? I suppose the platform bus populate code won't create a
device for the subnode if it has 'operating-points-v2', so maybe the
fallback compatible string should be there? And then the generic OPP
code can parse out the frequency at least without having to know that
it's a qcom specific OPP table.
Doug Anderson Dec. 18, 2018, 7:05 p.m. UTC | #7
Hi,

On Tue, Dec 18, 2018 at 10:40 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Doug Anderson (2018-12-17 16:34:31)
> > Hi,
> >
> > On Sun, Dec 16, 2018 at 11:06 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > +Rob +Stephen,
> > >
> > > On 14-12-18, 09:04, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Thu, Dec 13, 2018 at 8:41 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > >
> > > > > On 12-12-18, 14:18, Jordan Crouse wrote:
> > > > > > +                     gpu_opp_table: opp-table {
> > > > > > +                             compatible = "operating-points-v2-qcom-level";
> > > > >
> > > > > I think you need to mention "operating-points-v2" as well here.
> > > >
> > > > Are you saying the above should be:
> > > >   compatible = "operating-points-v2-qcom-level", "operating-points-v2";
> > > >
> > > > If so I'm not sure I agree.
> > >
> > > Well I have my doubts as well on this. This is where the ordering was discussed
> > > earlier:
> > >
> > > https://lore.kernel.org/lkml/152328979897.180276.15963925877442657158@swboyd.mtv.corp.google.com/
> > >
> > > @Rob/Stephen: Should the opp-table node above also have "operating-points-v2"
> > > string in the compatible property ?
> > >
> > > > It's _not_ really compatible with the
> > > > "operating-points-v2" binding.  If you had a driver that had never
> > > > heard of "operating-points-v2-qcom-level" and had only heard of
> > > > "operating-points-v2" and it took a look at this node it would have no
> > > > idea what to do with it.
> > >
> > > Well it will parse everything apart from the qcom,level thing, so it can
> > > actually parse stuff here.
> > >
> > > > I'll also note that other instances posted to the list don't list both:
> > > >
> > > > https://patchwork.kernel.org/patch/10725801/ - RPM / RPMH PD bindings
> > > > https://patchwork.kernel.org/patch/10725793/ - RPMH PD device tree for sdm845
> > > >
> > > > The bindings patch also makes no mention of needing
> > > > "operating-points-v2".  I think the common case when a fallback is
> > > > required it is explicitly called out in the bindings:
> > > >
> > > > https://patchwork.kernel.org/patch/10725803/ - qcom-opp bindings
> > >
> > > Sure, maybe I am wrong but its better to get some clarity on it from Rob/Stephen.
> >
> > In case it helps:
> >
> > https://lkml.kernel.org/r/20181217210849.GA15490@bogus
> >
> > ...shows Rob giving a Reviewed-by with just
> >
> >   compatible = "operating-points-v2-qcom-level";
> >
> > ...and not:
> >
> >   compatible = "operating-points-v2-qcom-level", "operating-points-v2";
> >
>
> I don't see a problem if both compatible strings are there. Does that
> change anything? I suppose the platform bus populate code won't create a
> device for the subnode if it has 'operating-points-v2', so maybe the
> fallback compatible string should be there? And then the generic OPP
> code can parse out the frequency at least without having to know that
> it's a qcom specific OPP table.

OK, it's fine with me to have the fallback, but if we do we should be
consistent about it and make sure it's in all the bindings and device
tree files...

-Doug
Viresh Kumar Dec. 19, 2018, 4:49 a.m. UTC | #8
On 18-12-18, 11:05, Doug Anderson wrote:
> OK, it's fine with me to have the fallback, but if we do we should be
> consistent about it and make sure it's in all the bindings and device
> tree files...

Sure.

I am not sure what's the right way to do it is, i.e. should we keep the
"operating-points-v2" string or not. Another example I see is (which can be
compared here) is the board specific DT files. Normally the root node's
compatible is a long list of strings like:

compatible = "nvidia,p2371-2180", "nvidia,p2180", "nvidia,tegra210";

which starts from platform-specific string, then mach, then board, etc..

We do keep all of them in those cases and that makes me wonder why the same
shouldn't be done for OPP bindings.
Rob Herring (Arm) Dec. 19, 2018, 8:08 p.m. UTC | #9
On Tue, Dec 18, 2018 at 10:49 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 18-12-18, 11:05, Doug Anderson wrote:
> > OK, it's fine with me to have the fallback, but if we do we should be
> > consistent about it and make sure it's in all the bindings and device
> > tree files...
>
> Sure.
>
> I am not sure what's the right way to do it is, i.e. should we keep the
> "operating-points-v2" string or not.

Does having it buy you anything? Given the QCom one doesn't have any
frequency or voltage, I don't see how it would be useful to have it.

Rob
Doug Anderson Dec. 19, 2018, 8:40 p.m. UTC | #10
Hi,

On Wed, Dec 19, 2018 at 12:09 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Dec 18, 2018 at 10:49 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 18-12-18, 11:05, Doug Anderson wrote:
> > > OK, it's fine with me to have the fallback, but if we do we should be
> > > consistent about it and make sure it's in all the bindings and device
> > > tree files...
> >
> > Sure.
> >
> > I am not sure what's the right way to do it is, i.e. should we keep the
> > "operating-points-v2" string or not.
>
> Does having it buy you anything? Given the QCom one doesn't have any
> frequency or voltage, I don't see how it would be useful to have it.

...but it does have a frequency, doesn't it?

+   compatible = "operating-points-v2-qcom-level";
+
+   opp-710000000 {
+     opp-hz = /bits/ 64 <710000000>;
+     qcom,level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
+   };

-Doug
Doug Anderson Dec. 19, 2018, 10:40 p.m. UTC | #11
Hi,

On Wed, Dec 19, 2018 at 12:40 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Dec 19, 2018 at 12:09 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Dec 18, 2018 at 10:49 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 18-12-18, 11:05, Doug Anderson wrote:
> > > > OK, it's fine with me to have the fallback, but if we do we should be
> > > > consistent about it and make sure it's in all the bindings and device
> > > > tree files...
> > >
> > > Sure.
> > >
> > > I am not sure what's the right way to do it is, i.e. should we keep the
> > > "operating-points-v2" string or not.
> >
> > Does having it buy you anything? Given the QCom one doesn't have any
> > frequency or voltage, I don't see how it would be useful to have it.
>
> ...but it does have a frequency, doesn't it?
>
> +   compatible = "operating-points-v2-qcom-level";
> +
> +   opp-710000000 {
> +     opp-hz = /bits/ 64 <710000000>;
> +     qcom,level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
> +   };

Ah, I perhaps see the confusion.  So Rajendra's usage of
"operating-points-v2-qcom-level" [1] doesn't have a frequency but
Jordan's do.  So I guess it makes sense that Jordan's have the
fallback compatible but Rajendra's don't?

[1] https://patchwork.kernel.org/patch/10725793/

-Doug
Rob Herring (Arm) Dec. 19, 2018, 11:47 p.m. UTC | #12
On Wed, Dec 19, 2018 at 4:40 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Dec 19, 2018 at 12:40 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Dec 19, 2018 at 12:09 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Tue, Dec 18, 2018 at 10:49 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >
> > > > On 18-12-18, 11:05, Doug Anderson wrote:
> > > > > OK, it's fine with me to have the fallback, but if we do we should be
> > > > > consistent about it and make sure it's in all the bindings and device
> > > > > tree files...
> > > >
> > > > Sure.
> > > >
> > > > I am not sure what's the right way to do it is, i.e. should we keep the
> > > > "operating-points-v2" string or not.
> > >
> > > Does having it buy you anything? Given the QCom one doesn't have any
> > > frequency or voltage, I don't see how it would be useful to have it.
> >
> > ...but it does have a frequency, doesn't it?
> >
> > +   compatible = "operating-points-v2-qcom-level";
> > +
> > +   opp-710000000 {
> > +     opp-hz = /bits/ 64 <710000000>;
> > +     qcom,level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
> > +   };
>
> Ah, I perhaps see the confusion.  So Rajendra's usage of
> "operating-points-v2-qcom-level" [1] doesn't have a frequency but
> Jordan's do.  So I guess it makes sense that Jordan's have the
> fallback compatible but Rajendra's don't?

Is having it useful to s/w that doesn't understand
"operating-points-v2-qcom-level"? If so, then add
"operating-points-v2". If not, then don't.

I don't really care either way. Just don't do both ways and document
which way is correct.

Rob
Stephen Boyd Dec. 20, 2018, 9:29 p.m. UTC | #13
Quoting Rob Herring (2018-12-19 15:47:25)
> On Wed, Dec 19, 2018 at 4:40 PM Doug Anderson <dianders@chromium.org> wrote:
> > On Wed, Dec 19, 2018 at 12:40 PM Doug Anderson <dianders@chromium.org> wrote:
> > > On Wed, Dec 19, 2018 at 12:09 PM Rob Herring <robh@kernel.org> wrote:
> > > ...but it does have a frequency, doesn't it?
> > >
> > > +   compatible = "operating-points-v2-qcom-level";
> > > +
> > > +   opp-710000000 {
> > > +     opp-hz = /bits/ 64 <710000000>;
> > > +     qcom,level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
> > > +   };
> >
> > Ah, I perhaps see the confusion.  So Rajendra's usage of
> > "operating-points-v2-qcom-level" [1] doesn't have a frequency but
> > Jordan's do.  So I guess it makes sense that Jordan's have the
> > fallback compatible but Rajendra's don't?
> 
> Is having it useful to s/w that doesn't understand
> "operating-points-v2-qcom-level"? If so, then add
> "operating-points-v2". If not, then don't.

The only benefit I see in having "operating-points-v2" is that we don't
need to update the of_skipped_node_table[] in drivers/platform/of.c to
have all the variants of operating-points-v2-* when they decide to not
use anything from the "base" binding.

If that fails to work because opp-hz is required for the
"operating-points-v2" binding but sometimes
operating-points-v2-qcom-level doesn't require it I guess we need to
update the skip table or make some generic property like
'this-is-not-a-device' that these various data tables in DT can be
marked with so we don't make platform devices for them.

Regardless of the above, we should update the binding for
operating-points-v2-qcom-level to say that opp-hz isn't always required
when the qcom-level compatible is present. It looks like it just says
that it builds on top of the opp binding so that's not obvious.
Rajendra Nayak Dec. 21, 2018, 4:52 a.m. UTC | #14
On 12/21/2018 2:59 AM, Stephen Boyd wrote:
> Quoting Rob Herring (2018-12-19 15:47:25)
>> On Wed, Dec 19, 2018 at 4:40 PM Doug Anderson <dianders@chromium.org> wrote:
>>> On Wed, Dec 19, 2018 at 12:40 PM Doug Anderson <dianders@chromium.org> wrote:
>>>> On Wed, Dec 19, 2018 at 12:09 PM Rob Herring <robh@kernel.org> wrote:
>>>> ...but it does have a frequency, doesn't it?
>>>>
>>>> +   compatible = "operating-points-v2-qcom-level";
>>>> +
>>>> +   opp-710000000 {
>>>> +     opp-hz = /bits/ 64 <710000000>;
>>>> +     qcom,level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
>>>> +   };
>>>
>>> Ah, I perhaps see the confusion.  So Rajendra's usage of
>>> "operating-points-v2-qcom-level" [1] doesn't have a frequency but
>>> Jordan's do.  So I guess it makes sense that Jordan's have the
>>> fallback compatible but Rajendra's don't?
>>
>> Is having it useful to s/w that doesn't understand
>> "operating-points-v2-qcom-level"? If so, then add
>> "operating-points-v2". If not, then don't.
> 
> The only benefit I see in having "operating-points-v2" is that we don't
> need to update the of_skipped_node_table[] in drivers/platform/of.c to
> have all the variants of operating-points-v2-* when they decide to not
> use anything from the "base" binding.
> 
> If that fails to work because opp-hz is required for the
> "operating-points-v2" binding but sometimes
> operating-points-v2-qcom-level doesn't require it I guess we need to
> update the skip table or make some generic property like
> 'this-is-not-a-device' that these various data tables in DT can be
> marked with so we don't make platform devices for them.
> 
> Regardless of the above, we should update the binding for
> operating-points-v2-qcom-level to say that opp-hz isn't always required
> when the qcom-level compatible is present. It looks like it just says
> that it builds on top of the opp binding so that's not obvious.

Sure, I can respin with those details added in.
So I am guessing the conclusion is to use a fallback "operating-points-v2"
compatible *only* when we do have opp-hz along with qcom,level (as in the
case with gpu) and not have a fallback compatible in cases when we don't
have opp-hz (as in the case of rpm power domains)?
That seems a little inconsistent, and given Rob said either way is fine,
just do one way or the other and not both, I am inclined to think we should
just have a "operating-points-v2-qcom-level" and no fallback compatible.
Does that make sense?

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Stephen Boyd Dec. 29, 2018, 1:29 a.m. UTC | #15
Quoting Rajendra Nayak (2018-12-20 20:52:34)
> 
> On 12/21/2018 2:59 AM, Stephen Boyd wrote:
> > Quoting Rob Herring (2018-12-19 15:47:25)
> >> On Wed, Dec 19, 2018 at 4:40 PM Doug Anderson <dianders@chromium.org> wrote:
> >>> On Wed, Dec 19, 2018 at 12:40 PM Doug Anderson <dianders@chromium.org> wrote:
> >>>> On Wed, Dec 19, 2018 at 12:09 PM Rob Herring <robh@kernel.org> wrote:
> >>>> ...but it does have a frequency, doesn't it?
> >>>>
> >>>> +   compatible = "operating-points-v2-qcom-level";
> >>>> +
> >>>> +   opp-710000000 {
> >>>> +     opp-hz = /bits/ 64 <710000000>;
> >>>> +     qcom,level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
> >>>> +   };
> >>>
> >>> Ah, I perhaps see the confusion.  So Rajendra's usage of
> >>> "operating-points-v2-qcom-level" [1] doesn't have a frequency but
> >>> Jordan's do.  So I guess it makes sense that Jordan's have the
> >>> fallback compatible but Rajendra's don't?
> >>
> >> Is having it useful to s/w that doesn't understand
> >> "operating-points-v2-qcom-level"? If so, then add
> >> "operating-points-v2". If not, then don't.
> > 
> > The only benefit I see in having "operating-points-v2" is that we don't
> > need to update the of_skipped_node_table[] in drivers/platform/of.c to
> > have all the variants of operating-points-v2-* when they decide to not
> > use anything from the "base" binding.
> > 
> > If that fails to work because opp-hz is required for the
> > "operating-points-v2" binding but sometimes
> > operating-points-v2-qcom-level doesn't require it I guess we need to
> > update the skip table or make some generic property like
> > 'this-is-not-a-device' that these various data tables in DT can be
> > marked with so we don't make platform devices for them.
> > 
> > Regardless of the above, we should update the binding for
> > operating-points-v2-qcom-level to say that opp-hz isn't always required
> > when the qcom-level compatible is present. It looks like it just says
> > that it builds on top of the opp binding so that's not obvious.
> 
> Sure, I can respin with those details added in.

Ok.

> So I am guessing the conclusion is to use a fallback "operating-points-v2"
> compatible *only* when we do have opp-hz along with qcom,level (as in the
> case with gpu) and not have a fallback compatible in cases when we don't
> have opp-hz (as in the case of rpm power domains)?
> That seems a little inconsistent, and given Rob said either way is fine,
> just do one way or the other and not both, I am inclined to think we should
> just have a "operating-points-v2-qcom-level" and no fallback compatible.
> Does that make sense?
> 

Are you going to update the skip table to not create platform devices?
Or introduce some generic property to indicate that this is just data
and not a device node?
Rajendra Nayak Jan. 3, 2019, 8:45 a.m. UTC | #16
On 12/29/2018 6:59 AM, Stephen Boyd wrote:
>> So I am guessing the conclusion is to use a fallback "operating-points-v2"
>> compatible*only*  when we do have opp-hz along with qcom,level (as in the
>> case with gpu) and not have a fallback compatible in cases when we don't
>> have opp-hz (as in the case of rpm power domains)?
>> That seems a little inconsistent, and given Rob said either way is fine,
>> just do one way or the other and not both, I am inclined to think we should
>> just have a "operating-points-v2-qcom-level" and no fallback compatible.
>> Does that make sense?
>>
> Are you going to update the skip table to not create platform devices?
> Or introduce some generic property to indicate that this is just data
> and not a device node?

Is any of it really needed, given the bindings specify that the OPP table
should actually be a child node of the device/power domain supporting
it? I don't see who would end up creating platform devices for them.
Stephen Boyd Jan. 4, 2019, 8:59 p.m. UTC | #17
Quoting Rajendra Nayak (2019-01-03 00:45:53)
> 
> On 12/29/2018 6:59 AM, Stephen Boyd wrote:
> >> So I am guessing the conclusion is to use a fallback "operating-points-v2"
> >> compatible*only*  when we do have opp-hz along with qcom,level (as in the
> >> case with gpu) and not have a fallback compatible in cases when we don't
> >> have opp-hz (as in the case of rpm power domains)?
> >> That seems a little inconsistent, and given Rob said either way is fine,
> >> just do one way or the other and not both, I am inclined to think we should
> >> just have a "operating-points-v2-qcom-level" and no fallback compatible.
> >> Does that make sense?
> >>
> > Are you going to update the skip table to not create platform devices?
> > Or introduce some generic property to indicate that this is just data
> > and not a device node?
> 
> Is any of it really needed, given the bindings specify that the OPP table
> should actually be a child node of the device/power domain supporting
> it? I don't see who would end up creating platform devices for them.
> 

Good point. If it's a child node then almost all the time we won't be
trying to populate the OPP node as another platform device so this isn't
really important to handle until that happens, which is probably never.
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 233a5898ebc2..a608afed502e 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -11,6 +11,7 @@ 
 #include <dt-bindings/clock/qcom,rpmh.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/phy/phy-qcom-qusb2.h>
+#include <dt-bindings/power/qcom-rpmpd.h>
 #include <dt-bindings/reset/qcom,sdm845-aoss.h>
 #include <dt-bindings/soc/qcom,rpmh-rsc.h>
 #include <dt-bindings/clock/qcom,gcc-sdm845.h>
@@ -1349,6 +1350,128 @@ 
 			};
 		};
 
+
+		gpu@5000000 {
+			compatible = "qcom,adreno-630.2", "qcom,adreno";
+			#stream-id-cells = <16>;
+
+			reg = <0x5000000 0x40000>, <0x509e000 0x10>;
+			reg-names = "kgsl_3d0_reg_memory", "cx_mem";
+
+			/*
+			 * Look ma, no clocks! The GPU clocks and power are
+			 * controlled entirely by the GMU
+			 */
+
+			interrupts = <GIC_SPI 300 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "kgsl_3d0_irq";
+
+			iommus = <&adreno_smmu 0>;
+
+			operating-points-v2 = <&gpu_opp_table>;
+
+			qcom,gmu = <&gmu>;
+
+			gpu_opp_table: opp-table {
+				compatible = "operating-points-v2-qcom-level";
+
+				opp-710000000 {
+					opp-hz = /bits/ 64 <710000000>;
+					qcom,level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
+				};
+
+				opp-675000000 {
+					opp-hz = /bits/ 64 <675000000>;
+					qcom,level = <RPMH_REGULATOR_LEVEL_TURBO>;
+				};
+
+				opp-596000000 {
+					opp-hz = /bits/ 64 <596000000>;
+					qcom,level = <RPMH_REGULATOR_LEVEL_NOM_L1>;
+				};
+
+				opp-520000000 {
+					opp-hz = /bits/ 64 <520000000>;
+					qcom,level = <RPMH_REGULATOR_LEVEL_NOM>;
+				};
+
+				opp-414000000 {
+					opp-hz = /bits/ 64 <414000000>;
+					qcom,level = <RPMH_REGULATOR_LEVEL_SVS_L1>;
+				};
+
+				opp-342000000 {
+					opp-hz = /bits/ 64 <342000000>;
+					qcom,level = <RPMH_REGULATOR_LEVEL_SVS>;
+				};
+
+				opp-257000000 {
+					opp-hz = /bits/ 64 <257000000>;
+					qcom,level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
+				};
+			};
+		};
+
+		adreno_smmu: iommu@5040000 {
+			compatible = "qcom,sdm845-smmu-v2", "qcom,smmu-v2";
+			reg = <0x5040000 0x10000>;
+			#iommu-cells = <1>;
+			#global-interrupts = <2>;
+			interrupts = <GIC_SPI 229 IRQ_TYPE_LEVEL_HIGH>,
+					<GIC_SPI 231 IRQ_TYPE_LEVEL_HIGH>,
+					<GIC_SPI 364 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 365 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 366 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 367 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 368 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 369 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 370 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 371 IRQ_TYPE_EDGE_RISING>;
+			clocks = <&gcc GCC_GPU_MEMNOC_GFX_CLK>,
+				<&gcc GCC_GPU_CFG_AHB_CLK>;
+			clock-names = "bus", "iface";
+
+			power-domains = <&gpucc GPU_CX_GDSC>;
+		};
+
+		gmu: gmu@506a000 {
+			compatible="qcom,adreno-gmu";
+
+			reg = <0x506a000 0x30000>,
+				<0xb280000 0x10000>,
+				<0xb480000 0x10000>;
+			reg-names = "gmu", "gmu_pdc", "gmu_pdc_seq";
+
+			interrupts = <GIC_SPI 304 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "hfi", "gmu";
+
+			clocks = <&gpucc GPU_CC_CX_GMU_CLK>,
+				<&gpucc GPU_CC_CXO_CLK>,
+				<&gcc GCC_DDRSS_GPU_AXI_CLK>,
+				<&gcc GCC_GPU_MEMNOC_GFX_CLK>;
+			clock-names = "gmu", "cxo", "axi", "memnoc";
+
+			power-domains = <&gpucc GPU_CX_GDSC>;
+			iommus = <&adreno_smmu 5>;
+
+			operating-points-v2 = <&gmu_opp_table>;
+
+			gmu_opp_table: opp-table {
+				compatible = "operating-points-v2-qcom-level";
+
+				opp-400000000 {
+					opp-hz = /bits/ 64 <400000000>;
+					qcom,level = <RPMH_REGULATOR_LEVEL_SVS>;
+				};
+
+				opp-200000000 {
+					opp-hz = /bits/ 64 <200000000>;
+					qcom,level = <RPMH_REGULATOR_LEVEL_MIN_SVS>;
+				};
+			};
+		};
+
 		gpucc: clock-controller@5090000 {
 			compatible = "qcom,sdm845-gpucc";
 			reg = <0x5090000 0x9000>;