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@xxxxxxxxxxxxxx> wrote:
> >
> > Document the device tree bindings for the Adreno GMU device
> > available on Adreno a6xx targets.
> >
> > Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
> > Signed-off-by: Jordan Crouse <jcrouse@xxxxxxxxxxxxxx>
> > ---
> > .../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
