[v5,1/2] dt-bindings: Document,qcom,adreno-gmu

Document the device tree bindings for the Adreno GMU device
available on Adreno a6xx targets.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 .../devicetree/bindings/display/msm/gmu.txt   | 54 +++++++++++++++++++
 .../devicetree/bindings/display/msm/gpu.txt   |  2 +
 2 files changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/msm/gmu.txt

Hi,

On Wed, Dec 12, 2018 at 9:31 AM Jordan Crouse <jcrouse@codeaurora.org> wrote:
>
> Document the device tree bindings for the Adreno GMU device
> available on Adreno a6xx targets.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>  .../devicetree/bindings/display/msm/gmu.txt   | 54 +++++++++++++++++++
>  .../devicetree/bindings/display/msm/gpu.txt   |  2 +
>  2 files changed, 56 insertions(+)

nit: Why does subject have a "," after "Document"?

nit: Should subject start "dt-bindings: drm/msm/a6xx" or something
like that?  I thought you wanted not just "dt-bindings" but also a
subsystem prefix?


>  create mode 100644 Documentation/devicetree/bindings/display/msm/gmu.txt
>
> diff --git a/Documentation/devicetree/bindings/display/msm/gmu.txt b/Documentation/devicetree/bindings/display/msm/gmu.txt
> new file mode 100644
> index 000000000000..f65bb49fff36
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/gmu.txt
> @@ -0,0 +1,54 @@
> +Qualcomm adreno/snapdragon GMU (Graphics management unit)
> +
> +The GMU is a programmable power controller for the GPU. the CPU controls the
> +GMU which in turn handles power controls for the GPU.
> +
> +Required properties:
> +- compatible:
> +  * "qcom,adreno-gmu"
> +- reg: Physical base address and length of the GMU registers.
> +- reg-names: Matching names for the register regions
> +  * "gmu"
> +  * "gmu_pdc"
> +- interrupts: The interrupt signals from the GMU.
> +- interrupt-names: Matching names for the interrupts
> +  * "hfi"
> +  * "gmu"
> +- clocks: phandles to the device clocks
> +- clock-names: Matching names for the clocks
> +   * "gmu"
> +   * "cxo"
> +   * "axi"
> +   * "mnoc"
> +- power-domains: should be <&clock_gpucc GPU_CX_GDSC>
> +- iommus: phandle to the adreno iommu
> +- operating-points-v2: phandle to the OPP operating points
> +
> +Example:
> +
> +/ {
> +       ...
> +
> +       gmu: gmu@506a000 {
> +               compatible="qcom,adreno-gmu";
> +
> +               reg = <0x506a000 0x30000>,
> +                       <0xb200000 0x300000>;
> +               reg-names = "gmu", "gmu_pdc";

Your implementation has 3 register ranges but your bindings have 2.
You also have a different address for "gmu_pdc" even though it looks
like your examples are supposed to be based on sdm845.

(you'd want to not only change the example but also the bindings above)


> +
> +               interrupts = <GIC_SPI 304 IRQ_TYPE_LEVEL_HIGH>,
> +                       <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>;
> +               interrupt-names = "hfi", "gmu";
> +
> +               clocks = <&clock_gpucc GPU_CC_CX_GMU_CLK>,
> +                       <&clock_gpucc GPU_CC_CXO_CLK>,
> +                       <&clock_gcc GCC_DDRSS_GPU_AXI_CLK>,
> +                       <&clock_gcc GCC_GPU_MEMNOC_GFX_CLK>;

nit: might as well update to "&gpucc" to match the style of clock
controller labels used in sdm845.dts.


> +               clock-names = "gmu", "cxo", "axi", "memnoc";
> +
> +               power-domains = <&clock_gpucc GPU_CX_GDSC>;
> +               iommus = <&adreno_smmu 5>;
> +
> +       i       operating-points-v2 = <&gmu_opp_table>;

Why "i"?

...also: worth actually including the operating table here in the example?


> +       };
> +};
> diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt
> index 43fac0fe09bb..754f6c6f34e5 100644
> --- a/Documentation/devicetree/bindings/display/msm/gpu.txt
> +++ b/Documentation/devicetree/bindings/display/msm/gpu.txt
> @@ -14,6 +14,8 @@ Required properties:
>    * "core"
>    * "iface"
>    * "mem_iface"
> +- qcom,gmu: For a6xx and newer targets a phandle to the GMU device that will
> +  control the power for the GPU

You seem to have lost something between the previous version of this
and the latest.  In the last version (and the version Rob gave his
review to) you added some extra text.  Diffing your old patch to your
new one:

-Optional properties.
-- clocks: device clocks. Required for a3xx, a4xx and a5xx targets. a6xx and
-  newer with a GMU attached do not have direct clock control from the CPU and
-  do not need to provide clock properties.
+- clocks: device clocks
   See ../clocks/clock-bindings.txt for details.
-- clock-names: the following clocks can be provided:
+- clock-names: the following clocks are required:

Did you mean to remove that?  Your current bindings now say that the
clocks are required but then your device tree patch for sdm845 says
they're not.

>  Example:

While you're touching this file, maybe update the "Example" so instead
of saying "qcom,kgsl-3d0@" it says "gpu@"


-Doug

On Wed, Dec 12, 2018 at 11:26:46AM -0800, Doug Anderson wrote:
> Hi,
> 
> On Wed, Dec 12, 2018 at 9:31 AM Jordan Crouse <jcrouse@codeaurora.org> wrote:
> >
> > Document the device tree bindings for the Adreno GMU device
> > available on Adreno a6xx targets.
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> > ---
> >  .../devicetree/bindings/display/msm/gmu.txt   | 54 +++++++++++++++++++
> >  .../devicetree/bindings/display/msm/gpu.txt   |  2 +
> >  2 files changed, 56 insertions(+)
> 
> nit: Why does subject have a "," after "Document"?

Typo.

> nit: Should subject start "dt-bindings: drm/msm/a6xx" or something
> like that?  I thought you wanted not just "dt-bindings" but also a
> subsystem prefix?

I was trying to reuse the subject from the previous versions, but I would be
happy to change it.

> 
> >  create mode 100644 Documentation/devicetree/bindings/display/msm/gmu.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/msm/gmu.txt b/Documentation/devicetree/bindings/display/msm/gmu.txt
> > new file mode 100644
> > index 000000000000..f65bb49fff36
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/msm/gmu.txt
> > @@ -0,0 +1,54 @@
> > +Qualcomm adreno/snapdragon GMU (Graphics management unit)
> > +
> > +The GMU is a programmable power controller for the GPU. the CPU controls the
> > +GMU which in turn handles power controls for the GPU.
> > +
> > +Required properties:
> > +- compatible:
> > +  * "qcom,adreno-gmu"
> > +- reg: Physical base address and length of the GMU registers.
> > +- reg-names: Matching names for the register regions
> > +  * "gmu"
> > +  * "gmu_pdc"
> > +- interrupts: The interrupt signals from the GMU.
> > +- interrupt-names: Matching names for the interrupts
> > +  * "hfi"
> > +  * "gmu"
> > +- clocks: phandles to the device clocks
> > +- clock-names: Matching names for the clocks
> > +   * "gmu"
> > +   * "cxo"
> > +   * "axi"
> > +   * "mnoc"
> > +- power-domains: should be <&clock_gpucc GPU_CX_GDSC>
> > +- iommus: phandle to the adreno iommu
> > +- operating-points-v2: phandle to the OPP operating points
> > +
> > +Example:
> > +
> > +/ {
> > +       ...
> > +
> > +       gmu: gmu@506a000 {
> > +               compatible="qcom,adreno-gmu";
> > +
> > +               reg = <0x506a000 0x30000>,
> > +                       <0xb200000 0x300000>;
> > +               reg-names = "gmu", "gmu_pdc";
> 
> Your implementation has 3 register ranges but your bindings have 2.
> You also have a different address for "gmu_pdc" even though it looks
> like your examples are supposed to be based on sdm845.
>
> (you'd want to not only change the example but also the bindings above)

I'll update the settings - a new region has indeed been added since the last
time we were here.

> 
> > +
> > +               interrupts = <GIC_SPI 304 IRQ_TYPE_LEVEL_HIGH>,
> > +                       <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>;
> > +               interrupt-names = "hfi", "gmu";
> > +
> > +               clocks = <&clock_gpucc GPU_CC_CX_GMU_CLK>,
> > +                       <&clock_gpucc GPU_CC_CXO_CLK>,
> > +                       <&clock_gcc GCC_DDRSS_GPU_AXI_CLK>,
> > +                       <&clock_gcc GCC_GPU_MEMNOC_GFX_CLK>;
> 
> nit: might as well update to "&gpucc" to match the style of clock
> controller labels used in sdm845.dts.
> 
> 
> > +               clock-names = "gmu", "cxo", "axi", "memnoc";
> > +
> > +               power-domains = <&clock_gpucc GPU_CX_GDSC>;
> > +               iommus = <&adreno_smmu 5>;
> > +
> > +       i       operating-points-v2 = <&gmu_opp_table>;
> 
> Why "i"?

Also a typo.

> ...also: worth actually including the operating table here in the example?

I say no.  It might good practice to have the opp tables in the child node but I
don't think it is mandatory from a bindings perspective.

The bindings are documented elsewhere and to me that seems enough for our
purposes - at least thats our story for smmu and gpucc and other phandles.

> 
> > +       };
> > +};
> > diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt
> > index 43fac0fe09bb..754f6c6f34e5 100644
> > --- a/Documentation/devicetree/bindings/display/msm/gpu.txt
> > +++ b/Documentation/devicetree/bindings/display/msm/gpu.txt
> > @@ -14,6 +14,8 @@ Required properties:
> >    * "core"
> >    * "iface"
> >    * "mem_iface"
> > +- qcom,gmu: For a6xx and newer targets a phandle to the GMU device that will
> > +  control the power for the GPU
> 
> You seem to have lost something between the previous version of this
> and the latest.  In the last version (and the version Rob gave his
> review to) you added some extra text.  Diffing your old patch to your
> new one:
> 
> -Optional properties.
> -- clocks: device clocks. Required for a3xx, a4xx and a5xx targets. a6xx and
> -  newer with a GMU attached do not have direct clock control from the CPU and
> -  do not need to provide clock properties.
> +- clocks: device clocks
>    See ../clocks/clock-bindings.txt for details.
> -- clock-names: the following clocks can be provided:
> +- clock-names: the following clocks are required:

You are right that "can be provided" is correct.

> Did you mean to remove that?  Your current bindings now say that the
> clocks are required but then your device tree patch for sdm845 says
> they're not.
> 
> >  Example:
> 
> While you're touching this file, maybe update the "Example" so instead
> of saying "qcom,kgsl-3d0@" it says "gpu@"

I don't want to much with the example too much lest we "break" it for older
targets. I'll see what generic stuff we can change without breaking the original
intent.

Jordan