diff mbox series

[5/9] arm64: dts: sdm845: Add gpu and gmu device nodes

Message ID 20180827151112.25211-6-jcrouse@codeaurora.org (mailing list archive)
State New, archived
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 nodes to describe the Adreno GPU and GMU devices.

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

Comments

Vivek Gautam Aug. 28, 2018, 10:30 a.m. UTC | #1
Hi Jordan,

On Mon, Aug 27, 2018 at 8:42 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 | 121 +++++++++++++++++++++++++++
>  1 file changed, 121 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index cdaabeb3c995..10db0ceb3699 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -192,6 +192,59 @@
>                 method = "smc";
>         };
>
> +gpu_opp_table: adreno-opp-table {
> +               compatible = "operating-points-v2-qcom-level";
> +
> +               opp-710000000 {
> +                       opp-hz = /bits/ 64 <710000000>;
> +                       qcom,level = <416>;
> +               };
> +
> +               opp-675000000 {
> +                       opp-hz = /bits/ 64 <675000000>;
> +                       qcom,level = <384>;
> +               };
> +
> +               opp-596000000 {
> +                       opp-hz = /bits/ 64 <596000000>;
> +                       qcom,level = <320>;
> +               };
> +
> +               opp-520000000 {
> +                       opp-hz = /bits/ 64 <520000000>;
> +                       qcom,level = <256>;
> +               };
> +
> +               opp-414000000 {
> +                       opp-hz = /bits/ 64 <414000000>;
> +                       qcom,level = <192>;
> +               };
> +
> +               opp-342000000 {
> +                       opp-hz = /bits/ 64 <342000000>;
> +                       qcom,level = <128>;
> +               };
> +
> +               opp-257000000 {
> +                       opp-hz = /bits/ 64 <257000000>;
> +                       qcom,level = <64>;
> +               };
> +       };
> +
> +       gmu_opp_table: adreno-gmu-opp-table {
> +               compatible = "operating-points-v2-qcom-level";
> +
> +               opp-400000000 {
> +                       opp-hz = /bits/ 64 <400000000>;
> +                       qcom,level = <128>;
> +               };
> +
> +               opp-200000000 {
> +                       opp-hz = /bits/ 64 <200000000>;
> +                       qcom,level = <48>;
> +               };
> +       };
> +
>         soc: soc {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
> @@ -323,5 +376,73 @@
>                                 status = "disabled";
>                         };
>                 };
> +
> +               adreno_smmu: adreno-smmu@5040000 {

iommu@5040000 as pointed out by Rob in [1]

> +                       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>;

and for this you need to include the gpucc dt-bindings header which is coming
from Amit's gpucc driver patch.

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

Best regards
Vivek

[snip]
Viresh Kumar Oct. 10, 2018, 9:46 a.m. UTC | #2
On 27-08-18, 09:11, Jordan Crouse 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 | 121 +++++++++++++++++++++++++++
>  1 file changed, 121 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index cdaabeb3c995..10db0ceb3699 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -192,6 +192,59 @@
>  		method = "smc";
>  	};
>  
> +gpu_opp_table: adreno-opp-table {
> +		compatible = "operating-points-v2-qcom-level";
> +
> +		opp-710000000 {
> +			opp-hz = /bits/ 64 <710000000>;
> +			qcom,level = <416>;

What is qcom,level here ? Is it different than the RPM voting thing ?

If not then you need to follow what Rajendra, Niklas are doing as
well. There needs to be a genpd which needs to carry this value and
the gpu's table will have required-opps entry to point to it.
Jordan Crouse Oct. 10, 2018, 2:29 p.m. UTC | #3
On Wed, Oct 10, 2018 at 03:16:28PM +0530, Viresh Kumar wrote:
> On 27-08-18, 09:11, Jordan Crouse 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 | 121 +++++++++++++++++++++++++++
> >  1 file changed, 121 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index cdaabeb3c995..10db0ceb3699 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -192,6 +192,59 @@
> >  		method = "smc";
> >  	};
> >  
> > +gpu_opp_table: adreno-opp-table {
> > +		compatible = "operating-points-v2-qcom-level";
> > +
> > +		opp-710000000 {
> > +			opp-hz = /bits/ 64 <710000000>;
> > +			qcom,level = <416>;
> 
> What is qcom,level here ? Is it different than the RPM voting thing ?
> 
> If not then you need to follow what Rajendra, Niklas are doing as
> well. There needs to be a genpd which needs to carry this value and
> the gpu's table will have required-opps entry to point to it.

I don't think it is the same (we have some special considerations here)
but I missed the new work from the other folks and I want to review it
before I conclude one way or the other. Is there a link to the latest
and greatest that I can use to get caught up?

Jordan
Viresh Kumar Oct. 10, 2018, 2:31 p.m. UTC | #4
On 10-10-18, 08:29, Jordan Crouse wrote:
> On Wed, Oct 10, 2018 at 03:16:28PM +0530, Viresh Kumar wrote:
> > On 27-08-18, 09:11, Jordan Crouse 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 | 121 +++++++++++++++++++++++++++
> > >  1 file changed, 121 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > index cdaabeb3c995..10db0ceb3699 100644
> > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > @@ -192,6 +192,59 @@
> > >  		method = "smc";
> > >  	};
> > >  
> > > +gpu_opp_table: adreno-opp-table {
> > > +		compatible = "operating-points-v2-qcom-level";
> > > +
> > > +		opp-710000000 {
> > > +			opp-hz = /bits/ 64 <710000000>;
> > > +			qcom,level = <416>;
> > 
> > What is qcom,level here ? Is it different than the RPM voting thing ?
> > 
> > If not then you need to follow what Rajendra, Niklas are doing as
> > well. There needs to be a genpd which needs to carry this value and
> > the gpu's table will have required-opps entry to point to it.
> 
> I don't think it is the same (we have some special considerations here)
> but I missed the new work from the other folks and I want to review it
> before I conclude one way or the other. Is there a link to the latest
> and greatest that I can use to get caught up?

lkml.kernel.org/r/20180627045234.27403-1-rnayak@codeaurora.org

+Rajendra/Niklas, please review Jordan's work as well to see if the
qcom,level thing is similar to what you guys are using.
Jordan Crouse Oct. 10, 2018, 2:48 p.m. UTC | #5
On Wed, Oct 10, 2018 at 08:01:49PM +0530, Viresh Kumar wrote:
> On 10-10-18, 08:29, Jordan Crouse wrote:
> > On Wed, Oct 10, 2018 at 03:16:28PM +0530, Viresh Kumar wrote:
> > > On 27-08-18, 09:11, Jordan Crouse 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 | 121 +++++++++++++++++++++++++++
> > > >  1 file changed, 121 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > > index cdaabeb3c995..10db0ceb3699 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > > @@ -192,6 +192,59 @@
> > > >  		method = "smc";
> > > >  	};
> > > >  
> > > > +gpu_opp_table: adreno-opp-table {
> > > > +		compatible = "operating-points-v2-qcom-level";
> > > > +
> > > > +		opp-710000000 {
> > > > +			opp-hz = /bits/ 64 <710000000>;
> > > > +			qcom,level = <416>;
> > > 
> > > What is qcom,level here ? Is it different than the RPM voting thing ?
> > > 
> > > If not then you need to follow what Rajendra, Niklas are doing as
> > > well. There needs to be a genpd which needs to carry this value and
> > > the gpu's table will have required-opps entry to point to it.
> > 
> > I don't think it is the same (we have some special considerations here)
> > but I missed the new work from the other folks and I want to review it
> > before I conclude one way or the other. Is there a link to the latest
> > and greatest that I can use to get caught up?
> 
> lkml.kernel.org/r/20180627045234.27403-1-rnayak@codeaurora.org
> 
> +Rajendra/Niklas, please review Jordan's work as well to see if the
> qcom,level thing is similar to what you guys are using.

qcom,level comes straight from:

https://lore.kernel.org/lkml/20180627045234.27403-2-rnayak@codeaurora.org/

But in this case instead of using the CPU to program the RPMh we are passing
the value to a microprocessor (the GMU) and that will do the vote on our behalf
(Technically we use the value to look up the vote in the cmd-db database and
pass that to the GMU)

This is why the qcom,level was added in the first place so we could at least
share the nomenclature with the rpmhd if not the implementation.

Jordan

> -- 
> viresh
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
Viresh Kumar Oct. 10, 2018, 2:51 p.m. UTC | #6
On 10-10-18, 08:48, Jordan Crouse wrote:
> qcom,level comes straight from:
> 
> https://lore.kernel.org/lkml/20180627045234.27403-2-rnayak@codeaurora.org/
> 
> But in this case instead of using the CPU to program the RPMh we are passing
> the value to a microprocessor (the GMU) and that will do the vote on our behalf
> (Technically we use the value to look up the vote in the cmd-db database and
> pass that to the GMU)
> 
> This is why the qcom,level was added in the first place so we could at least
> share the nomenclature with the rpmhd if not the implementation.

How you actually pass the vote to the underlying hardware, RPMh or
GMU, is irrelevant to the whole thing. What is important is how we
describe that in DT and how we represent the whole thing.

We have chosen genpd + OPP to do this and same should be used by you
as well. Another benefit is that the genpd core will do vote
aggregation for you here.
Jordan Crouse Oct. 10, 2018, 3:10 p.m. UTC | #7
On Wed, Oct 10, 2018 at 08:21:39PM +0530, Viresh Kumar wrote:
> On 10-10-18, 08:48, Jordan Crouse wrote:
> > qcom,level comes straight from:
> > 
> > https://lore.kernel.org/lkml/20180627045234.27403-2-rnayak@codeaurora.org/
> > 
> > But in this case instead of using the CPU to program the RPMh we are passing
> > the value to a microprocessor (the GMU) and that will do the vote on our behalf
> > (Technically we use the value to look up the vote in the cmd-db database and
> > pass that to the GMU)
> > 
> > This is why the qcom,level was added in the first place so we could at least
> > share the nomenclature with the rpmhd if not the implementation.
> 
> How you actually pass the vote to the underlying hardware, RPMh or
> GMU, is irrelevant to the whole thing. What is important is how we
> describe that in DT and how we represent the whole thing.
> 
> We have chosen genpd + OPP to do this and same should be used by you
> as well. Another benefit is that the genpd core will do vote
> aggregation for you here.

I'm not sure what you are suggesting? The vote is represented in DT exactly as
described in the bindings. 

Jordan

> -- 
> viresh
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
Viresh Kumar Oct. 11, 2018, 5:02 a.m. UTC | #8
On 10-10-18, 09:10, Jordan Crouse wrote:
> On Wed, Oct 10, 2018 at 08:21:39PM +0530, Viresh Kumar wrote:
> > On 10-10-18, 08:48, Jordan Crouse wrote:
> > > qcom,level comes straight from:
> > > 
> > > https://lore.kernel.org/lkml/20180627045234.27403-2-rnayak@codeaurora.org/
> > > 
> > > But in this case instead of using the CPU to program the RPMh we are passing
> > > the value to a microprocessor (the GMU) and that will do the vote on our behalf
> > > (Technically we use the value to look up the vote in the cmd-db database and
> > > pass that to the GMU)
> > > 
> > > This is why the qcom,level was added in the first place so we could at least
> > > share the nomenclature with the rpmhd if not the implementation.
> > 
> > How you actually pass the vote to the underlying hardware, RPMh or
> > GMU, is irrelevant to the whole thing. What is important is how we
> > describe that in DT and how we represent the whole thing.
> > 
> > We have chosen genpd + OPP to do this and same should be used by you
> > as well. Another benefit is that the genpd core will do vote
> > aggregation for you here.
> 
> I'm not sure what you are suggesting? The vote is represented in DT exactly as
> described in the bindings. 

Look at how Rajendra has done it to see the difference.

https://lore.kernel.org/lkml/20180627045234.27403-3-rnayak@codeaurora.org/
Jordan Crouse Oct. 11, 2018, 2:54 p.m. UTC | #9
On Thu, Oct 11, 2018 at 10:32:16AM +0530, Viresh Kumar wrote:
> On 10-10-18, 09:10, Jordan Crouse wrote:
> > On Wed, Oct 10, 2018 at 08:21:39PM +0530, Viresh Kumar wrote:
> > > On 10-10-18, 08:48, Jordan Crouse wrote:
> > > > qcom,level comes straight from:
> > > > 
> > > > https://lore.kernel.org/lkml/20180627045234.27403-2-rnayak@codeaurora.org/
> > > > 
> > > > But in this case instead of using the CPU to program the RPMh we are passing
> > > > the value to a microprocessor (the GMU) and that will do the vote on our behalf
> > > > (Technically we use the value to look up the vote in the cmd-db database and
> > > > pass that to the GMU)
> > > > 
> > > > This is why the qcom,level was added in the first place so we could at least
> > > > share the nomenclature with the rpmhd if not the implementation.
> > > 
> > > How you actually pass the vote to the underlying hardware, RPMh or
> > > GMU, is irrelevant to the whole thing. What is important is how we
> > > describe that in DT and how we represent the whole thing.
> > > 
> > > We have chosen genpd + OPP to do this and same should be used by you
> > > as well. Another benefit is that the genpd core will do vote
> > > aggregation for you here.
> > 
> > I'm not sure what you are suggesting? The vote is represented in DT exactly as
> > described in the bindings. 
> 
> Look at how Rajendra has done it to see the difference.
> 
> https://lore.kernel.org/lkml/20180627045234.27403-3-rnayak@codeaurora.org/

The GPU domain is completely controlled by the GMU hardware and is powered
independently of the CPU. For various reasons the GMU can't come up with
the vote itself so we need to construct a vote and send it during
initialization. For this we duplicate the code that rmphd does to query the
cmd-db and build the values using qcom,level as a guide. This is necessary
copypasta as the alternative would be to add the hooks into genpd or add a
side hook into the rpmhd to get the values we need and none of that is worth
it for a few lines of walking an array.

qcom,level serves the purpose for us in this case because we can get the value
we need and construct the vote. If we move to using required-opp, thats just
another step of parsing for the driver and it establishes a relationship with
rmphd on the CPU that shouldn't exist.

I do see a good argument for using the symbolic bindings (i.e.
RPMH_REGULATOR_LEVEL_TURBO_L1) and we can do that easily but beyond that I
don't think that we need the extra parsing step.

Thanks,
Jordan
Viresh Kumar Oct. 15, 2018, 10:03 a.m. UTC | #10
On 11-10-18, 08:54, Jordan Crouse wrote:

I understand what you are trying to say Jordan and I agree with those
expectations. But what I am looking for is consistency across Qcom
code using the same feature. Which enables better support for the code
going forward, etc. And if we are going to go a different way, there
must be a very good reason to do that.

But let me try to understand the hardware a bit first..

> The GPU domain is completely controlled by the GMU hardware and is powered
> independently of the CPU. For various reasons the GMU can't come up with
> the vote itself so we need to construct a vote and send it during
> initialization.

So it is the kernel which needs to send this vote on behalf of the GMU
to RPM ? Who does the vote aggregation in this case ? I thought there
has to be a single vote going from CPU side to the RPM. Isn't the GMU
vote part of that ?

> For this we duplicate the code that rmphd does to query the
> cmd-db and build the values using qcom,level as a guide. This is necessary
> copypasta as the alternative would be to add the hooks into genpd or add a
> side hook into the rpmhd to get the values we need and none of that is worth
> it for a few lines of walking an array.

Initially when I was designing this qcom,level or generically called
"performance-state" thingy, I kept the values directly in the OPP node
of the consumer (the way you have done it), but then there were
discussions which forced us to move this to the genpd level. For
example, what will you do if you also need to pass voltage/current in
addition to performance-state to the RPM? StephenB actually said we
may or may not know these values and we must support both the cases.

The opp-microvolt properties of the consumer device (GPU here) should
be used to handle the regulators which are controlled by kernel itself
and so they can't additionally handle the RPMs data. And so we created
separate OPP table for the RPM and represented that as a genpd and we
now handle the aggregation in genpd core itself on behalf of all the
consumers.

> qcom,level serves the purpose for us in this case because we can get the value
> we need and construct the vote. If we move to using required-opp, thats just
> another step of parsing for the driver and

The OPP core shall have all the infrastructure to parse these things
and we should keep all such code there to avoid duplication.

> it establishes a relationship with
> rmphd on the CPU that shouldn't exist.

Sure. I am not suggesting that the representation in software should
be different from what the hardware is, maybe I didn't understood the
hardware design well.

> I do see a good argument for using the symbolic bindings (i.e.
> RPMH_REGULATOR_LEVEL_TURBO_L1) and we can do that easily but beyond that I
> don't think that we need the extra parsing step.
Jordan Crouse Oct. 15, 2018, 2:34 p.m. UTC | #11
On Mon, Oct 15, 2018 at 03:33:27PM +0530, Viresh Kumar wrote:
> On 11-10-18, 08:54, Jordan Crouse wrote:
> 
> I understand what you are trying to say Jordan and I agree with those
> expectations. But what I am looking for is consistency across Qcom
> code using the same feature. Which enables better support for the code
> going forward, etc. And if we are going to go a different way, there
> must be a very good reason to do that.

I agree that consistency is good. But the GPU is by design outside of the
control of the genpd universe so it is by design not using the same features.
It unfortunately does happen to use a similar number in an OPP binding to
construct the level mapping but since we can't read the cmd-db from the GMU
space this is a necessary evil.

> But let me try to understand the hardware a bit first..
> 
> > The GPU domain is completely controlled by the GMU hardware and is powered
> > independently of the CPU. For various reasons the GMU can't come up with
> > the vote itself so we need to construct a vote and send it during
> > initialization.
> 
> So it is the kernel which needs to send this vote on behalf of the GMU
> to RPM ? Who does the vote aggregation in this case ? I thought there
> has to be a single vote going from CPU side to the RPM. Isn't the GMU
> vote part of that ?

For clarity the GMU and GPU are in different domains. The GMU (which is
controlled by the CPU) is responsible for generating and sending the vote on
behalf of the GPU. From an RPMh perspective The GPU is considered to be
separate from the CPU vote.

Also, I probably confused you by saying that the kernel constructs the vote
for the GPU. It actually constructs the mapping that the GMU can use to send 
the vote. This is equivalent to rpmhpd_update_level_mapping() in the rpmhpd
driver if that helps.

> > For this we duplicate the code that rmphd does to query the
> > cmd-db and build the values using qcom,level as a guide. This is necessary
> > copypasta as the alternative would be to add the hooks into genpd or add a
> > side hook into the rpmhd to get the values we need and none of that is worth
> > it for a few lines of walking an array.
> 
> Initially when I was designing this qcom,level or generically called
> "performance-state" thingy, I kept the values directly in the OPP node
> of the consumer (the way you have done it), but then there were
> discussions which forced us to move this to the genpd level. For
> example, what will you do if you also need to pass voltage/current in
> addition to performance-state to the RPM? StephenB actually said we
> may or may not know these values and we must support both the cases.

I agree that genpd has many responsibilities because it has to deal with many
different devices.

The GMU / GPU is built for a single purpose and the hardware has been designed
accordingly. For the foreseeable future we will not need to know anything
beyond the level mapping to operate the GPU.

> The opp-microvolt properties of the consumer device (GPU here) should
> be used to handle the regulators which are controlled by kernel itself
> and so they can't additionally handle the RPMs data. And so we created
> separate OPP table for the RPM and represented that as a genpd and we
> now handle the aggregation in genpd core itself on behalf of all the
> consumers.

Yes and it should continue to be that way. This just happens to be a
situation where one of the consumers is out of your area of control by
design.

> > qcom,level serves the purpose for us in this case because we can get the value
> > we need and construct the vote. If we move to using required-opp, thats just
> > another step of parsing for the driver and
> 
> The OPP core shall have all the infrastructure to parse these things
> and we should keep all such code there to avoid duplication.

Using required-opp to look up another opp to look up the qcom,level is
by definition additional parsing. It doesn't imply that there would be
code duplication.

> > it establishes a relationship with
> > rmphd on the CPU that shouldn't exist.
> 
> Sure. I am not suggesting that the representation in software should
> be different from what the hardware is, maybe I didn't understood the
> hardware design well.
> 
> > I do see a good argument for using the symbolic bindings (i.e.
> > RPMH_REGULATOR_LEVEL_TURBO_L1) and we can do that easily but beyond that I
> > don't think that we need the extra parsing step.

Jordan
Doug Anderson Oct. 17, 2018, 6:28 p.m. UTC | #12
Hi,

On Mon, Aug 27, 2018 at 8:11 AM Jordan Crouse <jcrouse@codeaurora.org> wrote:
> +               gpu@5000000 {
> +                       compatible = "qcom,adreno-630.2", "qcom,adreno";
> +                       #stream-id-cells = <16>;
> +
> +                       reg = <0x5000000 0x40000>;
> +                       reg-names = "kgsl_3d0_reg_memory";
> +
> +                       /*
> +                        * Look ma, no clocks! The GPU clocks and power are
> +                        * controlled entirely by the GMU
> +                        */
> +
> +                       interrupts = <0 300 0>;
> +                       interrupt-names = "kgsl_3d0_irq";

Drive-by feedback here.  The "interrupts" above should be:

interrupts = <GIC_SPI 300 IRQ_TYPE_LEVEL_HIGH>;

* GIC_SPI is 0, but GIC_SPI is more documenting.

* having 0 for the final element means 'IRQ_TYPE_NONE'.  On newer
kernels commit 6ef6386ef7c1 ("irqchip/gic-v3: Loudly complain about
the use of IRQ_TYPE_NONE") will cause loud yells if you do this.  This
turns out to be a bit silly because (IIUC) the flags in the device
tree are totally ignored if the driver passes in flags itself [1].
...but I guess we should do it right.  I believe the code requesting
this irq is a6xx_gmu_get_irq() which requests a level high interrupt,
so we should list level high here.


[1] https://lore.kernel.org/lkml/CAD=FV=XmiOh0Mg0f4a=W0NCH8eb--OQhP2jNAv2ZMpUBOn9n6Q@mail.gmail.com/T/#u


-Doug
Viresh Kumar Oct. 22, 2018, 10:38 a.m. UTC | #13
On 15-10-18, 08:34, Jordan Crouse wrote:
> I agree that consistency is good. But the GPU is by design outside of the
> control of the genpd universe so it is by design not using the same features.
> It unfortunately does happen to use a similar number in an OPP binding to
> construct the level mapping but since we can't read the cmd-db from the GMU
> space this is a necessary evil.

Where do you define how to use this binding in case of GPU? I mean
some DT binding doc must have some information to avoid confusion as
all other users will have the qcom,level thing in the genpd's OPP
table which GPU will have it directly within its OPP table.
Niklas Cassel Oct. 22, 2018, 1:20 p.m. UTC | #14
On Mon, Oct 22, 2018 at 04:08:11PM +0530, Viresh Kumar wrote:
> On 15-10-18, 08:34, Jordan Crouse wrote:
> > I agree that consistency is good. But the GPU is by design outside of the
> > control of the genpd universe so it is by design not using the same features.
> > It unfortunately does happen to use a similar number in an OPP binding to
> > construct the level mapping but since we can't read the cmd-db from the GMU
> > space this is a necessary evil.
> 
> Where do you define how to use this binding in case of GPU? I mean
> some DT binding doc must have some information to avoid confusion as
> all other users will have the qcom,level thing in the genpd's OPP
> table which GPU will have it directly within its OPP table.

Jordan suggested to use the RPMH_REGULATOR_LEVEL_* defines.
These are defined in include/dt-bindings/power/qcom-rpmpd.h.

This header is only referenced in
Documentation/devicetree/bindings/power/qcom,rpmpd.txt
(Which this patch series does not seem to use.)

This patch series does use
Documentation/devicetree/bindings/opp/qcom-opp.txt
but it does not reference include/dt-bindings/power/qcom-rpmpd.h.

So to further avoid confusion, perhaps it is better to create new
defines, instead of reusing the RPMH_REGULATOR_LEVEL_* defines?


Kind regards,
Niklas
Jordan Crouse Oct. 22, 2018, 2:34 p.m. UTC | #15
On Mon, Oct 22, 2018 at 04:08:11PM +0530, Viresh Kumar wrote:
> On 15-10-18, 08:34, Jordan Crouse wrote:
> > I agree that consistency is good. But the GPU is by design outside of the
> > control of the genpd universe so it is by design not using the same features.
> > It unfortunately does happen to use a similar number in an OPP binding to
> > construct the level mapping but since we can't read the cmd-db from the GMU
> > space this is a necessary evil.
> 
> Where do you define how to use this binding in case of GPU? I mean
> some DT binding doc must have some information to avoid confusion as
> all other users will have the qcom,level thing in the genpd's OPP
> table which GPU will have it directly within its OPP table.

When I first made the patch I was just using existing OPP bindings for  GPU
levels as the wording was generic enough to cover both cases. I would be happy
to revisit that and indicate that the OPP bindings apply to RPMh and GPU/GMU
as we've discussed in this thread.

Jordan
Jordan Crouse Oct. 22, 2018, 2:37 p.m. UTC | #16
On Mon, Oct 22, 2018 at 03:20:27PM +0200, Niklas Cassel wrote:
> On Mon, Oct 22, 2018 at 04:08:11PM +0530, Viresh Kumar wrote:
> > On 15-10-18, 08:34, Jordan Crouse wrote:
> > > I agree that consistency is good. But the GPU is by design outside of the
> > > control of the genpd universe so it is by design not using the same features.
> > > It unfortunately does happen to use a similar number in an OPP binding to
> > > construct the level mapping but since we can't read the cmd-db from the GMU
> > > space this is a necessary evil.
> > 
> > Where do you define how to use this binding in case of GPU? I mean
> > some DT binding doc must have some information to avoid confusion as
> > all other users will have the qcom,level thing in the genpd's OPP
> > table which GPU will have it directly within its OPP table.
> 
> Jordan suggested to use the RPMH_REGULATOR_LEVEL_* defines.
> These are defined in include/dt-bindings/power/qcom-rpmpd.h.
> 
> This header is only referenced in
> Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> (Which this patch series does not seem to use.)
> 
> This patch series does use
> Documentation/devicetree/bindings/opp/qcom-opp.txt
> but it does not reference include/dt-bindings/power/qcom-rpmpd.h.
> 
> So to further avoid confusion, perhaps it is better to create new
> defines, instead of reusing the RPMH_REGULATOR_LEVEL_* defines?

I would be fine with that. I'm also okay with using the raw values, but I
figure it would resolve Viresh's concerns if it was made clear that the
two use cases were using the same raw values. It would also help the GPU
folks visualize the expected level for each frequency entry.

Jordan
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index cdaabeb3c995..10db0ceb3699 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -192,6 +192,59 @@ 
 		method = "smc";
 	};
 
+gpu_opp_table: adreno-opp-table {
+		compatible = "operating-points-v2-qcom-level";
+
+		opp-710000000 {
+			opp-hz = /bits/ 64 <710000000>;
+			qcom,level = <416>;
+		};
+
+		opp-675000000 {
+			opp-hz = /bits/ 64 <675000000>;
+			qcom,level = <384>;
+		};
+
+		opp-596000000 {
+			opp-hz = /bits/ 64 <596000000>;
+			qcom,level = <320>;
+		};
+
+		opp-520000000 {
+			opp-hz = /bits/ 64 <520000000>;
+			qcom,level = <256>;
+		};
+
+		opp-414000000 {
+			opp-hz = /bits/ 64 <414000000>;
+			qcom,level = <192>;
+		};
+
+		opp-342000000 {
+			opp-hz = /bits/ 64 <342000000>;
+			qcom,level = <128>;
+		};
+
+		opp-257000000 {
+			opp-hz = /bits/ 64 <257000000>;
+			qcom,level = <64>;
+		};
+	};
+
+	gmu_opp_table: adreno-gmu-opp-table {
+		compatible = "operating-points-v2-qcom-level";
+
+		opp-400000000 {
+			opp-hz = /bits/ 64 <400000000>;
+			qcom,level = <128>;
+		};
+
+		opp-200000000 {
+			opp-hz = /bits/ 64 <200000000>;
+			qcom,level = <48>;
+		};
+	};
+
 	soc: soc {
 		#address-cells = <1>;
 		#size-cells = <1>;
@@ -323,5 +376,73 @@ 
 				status = "disabled";
 			};
 		};
+
+		adreno_smmu: adreno-smmu@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>;
+		};
+
+		gpu@5000000 {
+			compatible = "qcom,adreno-630.2", "qcom,adreno";
+			#stream-id-cells = <16>;
+
+			reg = <0x5000000 0x40000>;
+			reg-names = "kgsl_3d0_reg_memory";
+
+			/*
+			 * Look ma, no clocks! The GPU clocks and power are
+			 * controlled entirely by the GMU
+			 */
+
+			interrupts = <0 300 0>;
+			interrupt-names = "kgsl_3d0_irq";
+
+			iommus = <&adreno_smmu 0>;
+
+			operating-points-v2 = <&gpu_opp_table>;
+
+			qcom,gmu = <&gmu>;
+		};
+
+		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>;
+		};
 	};
 };