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

[Jordan Crouse <jcrouse@codeaurora.org>]

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

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel