[RFC] drm/msm/adreno: clean up gpu bindings

[Rob Herring]

On Thu, Jan 26, 2017 at 1:51 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Thu, Jan 26, 2017 at 2:11 PM, Rob Herring <robh at kernel.org> wrote:
>> On Tue, Jan 24, 2017 at 11:11 AM, Rob Clark <robdclark at gmail.com> wrote:
>>> So, cleaning up the GPU bindings is something that has been on my TODO
>>> list for a while, but always $bigger_fires.  Existing bindings are a bit
>>> ugly, but served a purpose when too many of the other drivers the GPU
>>> depends on where still working their way upstream.  But now enough of
>>> that is in place for a few devices, that we should start trying to get
>>> the GPU nodes into the upstream dts files.
>>>
>>> This drops the "qcom,chipid" property and parses the GPU revision out of
>>> the compat string.  It does mean you need to list both "qcom,adreno" and
>>> the more specific string, for example "qcom,adreno-530.2".  I'm not sure
>>> if that is "weird" or anyone has better ideas (hence this RFC).
>>
>> Is version and SoC close to a 1-1 mapping? If one version is in lots
>> of different SoCs, then you should have an SoC specific compatible.
>
> I'm not sure how common it is, but I'm pretty sure in the past I've
> seen same gpu (but maybe not same patchlevel).
>
> Also, there tend to be multiple revisions of the SoC which may or may
> not bump the gpu patchlevel.  I think it is quite likely to be
> insanity to try and figure out gpu revision from SoC..

I'm not saying you should. My point is gpu revision alone may not be
enough. Things can get integrated or configured differently. You could
use an SoC based compatible, read the revision registers for the
revision, and override wrong revision registers based on SoC
compatible (assuming that is rare).

>>> It also drops "qcom,gpu-pwrlevels".  Jordan tells me we can probably
>>> replace that w/ OPP bindings, which seems more sane.  For now, the code
>>> just falls back to a super-slow safe clock until we get the OPP bindings
>>> sorted.
>>>
>>> In both cases, the code is still compatible with the old bindings, so
>>> dts files that exist on top of upstream will still work.  But it is
>>> removed from the documentation, and for merging GPU nodes in upstream
>>> dts files, we should use the new bindings.
>>
>> Good.
>
> jfyi, I don't believe we have any adreno gpu nodes upstream..

Okay, missed that detail. In that case, one more thing below.

> (but as long as it is easy, I like to keep backwards compat since
> otherwise I'll end up helping someone debug a bindings mismatch issue
> sooner or later.. and I'm lazy that way ;-))

Glad *someone* sees the value.

>>> (I'll ofc split out the .dtsi changes, but figured for an RFC it was
>>> easier to include them in this patch for discussion.)
>>>
>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>> ---
>>>  .../devicetree/bindings/display/msm/gpu.txt        | 24 +++----------
>>>  arch/arm64/boot/dts/qcom/msm8916.dtsi              |  3 +-
>>>  arch/arm64/boot/dts/qcom/msm8996.dtsi              |  8 +----
>>
>> Bindings need to go to DT list.
>>
>>>  drivers/gpu/drm/msm/adreno/adreno_device.c         | 42 ++++++++++++++++++++--
>>>  drivers/gpu/drm/msm/msm_drv.c                      |  1 +
>>>  5 files changed, 47 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt
>>> index 67d0a58..760194d 100644
>>> --- a/Documentation/devicetree/bindings/display/msm/gpu.txt
>>> +++ b/Documentation/devicetree/bindings/display/msm/gpu.txt
>>> @@ -1,7 +1,11 @@
>>>  Qualcomm adreno/snapdragon GPU
>>>
>>>  Required properties:
>>> -- compatible: "qcom,adreno-3xx"
>>> +- compatible: "qcom,adreno-XYZ.W", "qcom,adreno"
>>> +    for example: "qcom,adreno-306.0", "qcom,adreno"
>>> +  Note that you need to list the less specific "qcom,adreno" (since this
>>> +  is what the device is matched on), in addition to the more specific
>>> +  with the chip-id.
>>>  - reg: Physical base address and length of the controller's registers.
>>>  - interrupts: The interrupt signal from the gpu.
>>>  - clocks: device clocks
>>> @@ -10,14 +14,6 @@ Required properties:
>>>    * "core_clk"
>>>    * "iface_clk"
>>>    * "mem_iface_clk"

"_clk" here is redundant.