Re: [PATCH v3 13/14] dt-bindings: gpu: mali-valhall-csf: Add support for Arm Mali CSF GPUs

[Boris Brezillon <boris.brezillon@collabora.com>]

Hi Rob,

We didn't hear back from you, so I assumed you were happy with Liviu's
explanations and sent a v4 with just the s/space/tab/ formatting fix.
Please let us know if you have any concerns with v4 binding docs.

Thanks,

Boris

On Wed, 6 Dec 2023 10:59:38 +0000
Liviu Dudau <Liviu.Dudau@arm.com> wrote:

> Hi Rob,
> 
> Thanks for reviewing this!
> 
> On Tue, Dec 05, 2023 at 02:48:27PM -0600, Rob Herring wrote:
> > On Mon, Dec 04, 2023 at 06:33:06PM +0100, Boris Brezillon wrote:  
> > > From: Liviu Dudau <liviu.dudau@arm.com>
> > > 
> > > Arm has introduced a new v10 GPU architecture that replaces the Job Manager
> > > interface with a new Command Stream Frontend. It adds firmware driven
> > > command stream queues that can be used by kernel and user space to submit
> > > jobs to the GPU.
> > > 
> > > Add the initial schema for the device tree that is based on support for
> > > RK3588 SoC. The minimum number of clocks is one for the IP, but on Rockchip
> > > platforms they will tend to expose the semi-independent clocks for better
> > > power management.
> > > 
> > > v3:
> > > - Cleanup commit message to remove redundant text
> > > - Added opp-table property and re-ordered entries
> > > - Clarified power-domains and power-domain-names requirements for RK3588.
> > > - Cleaned up example
> > > 
> > > Note: power-domains and power-domain-names requirements for other platforms
> > > are still work in progress, hence the bindings are left incomplete here.
> > > 
> > > v2:
> > > - New commit
> > > 
> > > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
> > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Conor Dooley <conor+dt@kernel.org>
> > > Cc: devicetree@vger.kernel.org
> > > ---
> > >  .../bindings/gpu/arm,mali-valhall-csf.yaml    | 147 ++++++++++++++++++
> > >  1 file changed, 147 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
> > > new file mode 100644
> > > index 000000000000..d72de094c8ea
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
> > > @@ -0,0 +1,147 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/gpu/arm,mali-valhall-csf.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ARM Mali Valhall GPU
> > > +
> > > +maintainers:
> > > +  - Liviu Dudau <liviu.dudau@arm.com>
> > > +  - Boris Brezillon <boris.brezillon@collabora.com>
> > > +
> > > +properties:
> > > +  $nodename:
> > > +    pattern: '^gpu@[a-f0-9]+$'
> > > +
> > > +  compatible:
> > > +    oneOf:  
> > 
> > Don't need oneOf.  
> 
> This has come up on the review of the previous version. We're planning on adding support for more
> SoCs once the initial panthor driver gets merged, but I don't think it's worth advertising it now.
> Krzyszof raised the same point and then agreed to keep it, as seen here[1].
> 
> >   
> > > +      - items:
> > > +          - enum:
> > > +              - rockchip,rk3588-mali
> > > +          - const: arm,mali-valhall-csf   # Mali Valhall GPU model/revision is fully discoverable
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    items:
> > > +      - description: Job interrupt
> > > +      - description: MMU interrupt
> > > +      - description: GPU interrupt
> > > +
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: job
> > > +      - const: mmu
> > > +      - const: gpu
> > > +
> > > +  clocks:
> > > +    minItems: 1
> > > +    maxItems: 3  
> > 
> > The function of each clock based on just the names below aren't too 
> > evident. 'core' is, but then what is 'stacks'? Please add some 
> > descriptions.
> >   
> The names match the hardware architecture, where the core clock powers
> most of the GPU, the 'stacks' clock is for shader core stacks and the
> 'coregroup' is used by stacks and tilers. All this is defined as "logical
> power domains" and the implementer of the IP can decide to map them to
> individual physical power domains or to group everything into a minimum of
> one power domain. Hence the flexibility in describing the hardware.
> 
> As for describing what the function of each power domain is, I'm afraid we
> need to keep it at "matches the architecture" for now and I will look into
> what more information can be added later. At the high level, the more
> power domains you have the more you can fine tune the power consumption of
> the GPU.
> 
> > I expect there is better visibility into what's correct here than we had 
> > on earlier h/w. IOW, I don't want to see different clocks for every SoC. 
> > Same applies to power-domains.  
> 
> Afraid that's up to the SoC implementation to decide how they wire the
> power domains. Hardware is capable to automatically powering up the domains
> needed, as long as the relevant clocks are provided.
>