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

[Rob Clark]

On Thu, Jan 26, 2017 at 4:09 PM, Rob Herring <robh at kernel.org> wrote:
> 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).

Hmm, I'll probably defer to Jordan on this, since he has seen more
combinations over the years.

I think any integration differences would just amount to which
clks/gdsc/etc are wired up.  At least what I've seen in downstream
kgsl driver, all the things that are conditional are purely keyed to
the revision+patchlevel.

As far as revision registers, IIRC back in the a3xx days they were not
to be trusted.  I'm not entirely sure when they became trustworthy.
Last I checked, android kgsl driver was not using them.

BR,
-R

>>>> 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.