Message-ID: <a9716af2-2403-44a4-b1a7-c3ab832c3c1c@samsung.com>
Date: Tue, 1 Apr 2025 18:45:31 +0200
From: Michal Wilczynski <m.wilczynski@...sung.com>
To: Matt Coster <matt.coster@...tec.com>, Frank Binns
	<frank.binns@...tec.com>, David Airlie <airlied@...il.com>, Simona Vetter
	<simona@...ll.ch>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Nishanth Menon <nm@...com>, Vignesh
	Raghavendra <vigneshr@...com>, Tero Kristo <kristo@...nel.org>
Cc: dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, Randolph
	Sapp <rs@...com>, Darren Etheridge <detheridge@...com>, Alessio Belle
	<alessio.belle@...tec.com>, Alexandru Dadu <alexandru.dadu@...tec.com>
Subject: Re: [PATCH v5 01/18] dt-bindings: gpu: img: Future-proofing
 enhancements



On 3/26/25 17:48, Matt Coster wrote:
> The first compatible strings added for the AXE-1-16M are not sufficient to
> accurately describe all the IMG Rogue GPUs. The current "img,img-axe"
> string refers to the entire family of Series AXE GPUs, but this is
> primarily a marketing term and does not denote a level of hardware
> similarity any greater than just "Rogue".
> 
> The more specific "img,img-axe-1-16m" string refers to individual AXE-1-16M
> GPU. For example, unlike the rest of the Series AXE GPUs, the AXE-1-16M
> only uses a single power domain.
> 
> The situation is actually slightly worse than described in the first
> paragraph, since many "series" (such as Series BXS found in the TI AM68
> among others and added later in this series) contain cores with both Rogue
> and Volcanic architectures.
> 
> Besides attempting to move away from vague groupings defined only
> by marketing terms, we want to draw a line between properties inherent to
> the IP core and choices made by the silicon vendor at integration time.
> For instance, the number of power domains is a property of the IP core,
> whereas the decision to use one or multiple clocks is a vendor one.

Hi,
We’ve had an interesting discussion on how to synchronize clock and
reset management for the T-HEAD SoC [1] with regard to the Imagination
GPU. Long story short, there is a preference to place SoC-specific
elements in the generic PM driver - however since drm/imagination driver
currently attempts to manage clocks itself, we would need to provide
some empty stubs, which does seem like an ugly solution.

I agree that a number of clocks is a vendor decision, but the clocks
doesn't have to necessarily be managed from the consumer driver. Would
you be open to a patch which makes the clock management from the
drm/imagination driver optional ? Same way I've proposed an optional
reset [2] previously.

This way if the clocks/reset are not provided, it would be assumed that
there is something SoC specific to how they're mangaged and they would
be implemented in the generic PM driver.	

Regards,
Michał

[1] - https://lore.kernel.org/all/ef17e5d1-b364-41e1-ab8b-86140cbe69b2@samsung.com/
[2] - https://lore.kernel.org/all/20250219140239.1378758-14-m.wilczynski@samsung.com/

> 
> In the original compatible strings, we must use "ti,am62-gpu" to constrain
> both of these properties since the number of power domains cannot be fixed
> for "img,img-axe".
> 
> Work is currently underway to add support for volcanic-based Imagination
> GPUs, for which bindings will be added in "img,powervr-volcanic.yaml".
> As alluded to previously, the split between rogue and volcanic cores is
> non-obvious at times, so add a generic top-level "img,img-rogue" compatible
> string here to allow for simpler differentiation in devicetrees without
> referring back to the bindings.
> 
> The currently supported GPU (AXE-1-16M) only requires a single power
> domain. Subsequent patches will add support for BXS-4-64 MC1, which has
> two power domains. Add infrastructure now to allow for this.
> 
> Also allow the dma-coherent property to be added to IMG Rogue GPUs, which
> are DMA devices. The decision for coherency is made at integration time and
> this property should be applied wherever it accurately describes the
> vendor integration.
> 
> Note that the new required properties for power domains are conditional on
> the new base compatible string to avoid an ABI break.
> 
> Signed-off-by: Matt Coster <matt.coster@...tec.com>
> ---
> Changes in v5:
> - Remove extraneous (and error-causing) power-domains minItems constraint
> - Link to v4: https://lore.kernel.org/r/20250320-sets-bxs-4-64-patch-v1-v4-1-d987cf4ca439@imgtec.com
> Changes in v4:
> - Add img,img-rogue back to ti,am62-gpu compatible strings to allow
>   compatibility with older kernels
> - Revert change to power-domains property and add proper constraint
> - Link to v3: https://lore.kernel.org/r/20250310-sets-bxs-4-64-patch-v1-v3-1-143b3dbef02f@imgtec.com
> Changes in v3:
> - Remove unnecessary example
> - Remove second power domain details, add these where they're used instead
> - Avoid ABI breaks by limiting new required properties to new compatible
>   strings and making all binding changes in a single patch.
> - Links to v2:
>   https://lore.kernel.org/r/20241118-sets-bxs-4-64-patch-v1-v2-1-3fd45d9fb0cf@imgtec.com
>   https://lore.kernel.org/r/20241118-sets-bxs-4-64-patch-v1-v2-3-3fd45d9fb0cf@imgtec.com
>   https://lore.kernel.org/r/20241118-sets-bxs-4-64-patch-v1-v2-4-3fd45d9fb0cf@imgtec.com
> ---
>  .../devicetree/bindings/gpu/img,powervr-rogue.yaml | 43 +++++++++++++++++++---
>  1 file changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> index 256e252f8087fa0d6081f771a01601d34b66fe19..e1056bf2af84c3eb43733bdc91124a66aaf51d35 100644
> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> @@ -12,10 +12,23 @@ maintainers:
>  
>  properties:
>    compatible:
> -    items:
> -      - enum:
> -          - ti,am62-gpu
> -      - const: img,img-axe # IMG AXE GPU model/revision is fully discoverable
> +    oneOf:
> +      - items:
> +          - enum:
> +              - ti,am62-gpu
> +          - const: img,img-axe-1-16m
> +          # This deprecated element must be kept around to allow old kernels to
> +          # work with newer dts.
> +          - const: img,img-axe
> +          - const: img,img-rogue
> +
> +      # This legacy combination of compatible strings was introduced early on
> +      # before the more specific GPU identifiers were used.
> +      - items:
> +          - enum:
> +              - ti,am62-gpu
> +          - const: img,img-axe
> +        deprecated: true
>  
>    reg:
>      maxItems: 1
> @@ -37,6 +50,12 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  power-domain-names:
> +    items:
> +      - const: a
> +
> +  dma-coherent: true
> +
>  required:
>    - compatible
>    - reg
> @@ -47,6 +66,18 @@ required:
>  additionalProperties: false
>  
>  allOf:
> +  # Constraints added alongside the new compatible strings that would otherwise
> +  # create an ABI break.
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: img,img-rogue
> +    then:
> +      required:
> +        - power-domains
> +        - power-domain-names
> +
>    - if:
>        properties:
>          compatible:
> @@ -64,10 +95,12 @@ examples:
>      #include <dt-bindings/soc/ti,sci_pm_domain.h>
>  
>      gpu@...0000 {
> -        compatible = "ti,am62-gpu", "img,img-axe";
> +        compatible = "ti,am62-gpu", "img,img-axe-1-16m", "img,img-axe",
> +                     "img,img-rogue";
>          reg = <0x0fd00000 0x20000>;
>          clocks = <&k3_clks 187 0>;
>          clock-names = "core";
>          interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
>          power-domains = <&k3_pds 187 TI_SCI_PD_EXCLUSIVE>;
> +        power-domain-names = "a";
>      };
> 

Powered by blists - more mailing lists