Re: [PATCH v3 01/18] dt-bindings: gpu: img: Future-proofing enhancements

[Matt Coster <Matt.Coster@imgtec.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5638 bytes --]

On 11/03/2025 07:50, Krzysztof Kozlowski wrote:
> On Mon, Mar 10, 2025 at 01:10:25PM +0000, 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.
>>
>> 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@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, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>> index 256e252f8087fa0d6081f771a01601d34b66fe19..5c16b2881447c9cda78e5bb46569e2f675d740c4 100644
>> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>> @@ -12,10 +12,20 @@ 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
>> +          - const: img,img-rogue
> 
> That's still ABI break. You got here NAK. You ust preserve img,img-axe.
> Your marketing troubles do not concern Linux.

I think I'm misunderstanding something here. Is keeping the existing
compatible string around in the deprecated item below not sufficient to
maintain the existing ABI?

Would adding img,img-axe back into the updated list (bringing it to four
elements) be acceptable?

> 
>> +
>> +      # 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
>> @@ -34,8 +44,13 @@ properties:
>>    interrupts:
>>      maxItems: 1
>>  
>> -  power-domains:
>> -    maxItems: 1
>> +  power-domains: true
> 
> No, widest constraints always stay here.

Ack

> 
>> +
>> +  power-domain-names:
>> +    items:
>> +      - const: a
> 
> That's not a useful name. Are you sure that datasheet calls it power
> domain A?

Sadly yes. With the Volcanic architecture the power domains get real
names, but until then we were stuck with abc. I shared a snipet from the
BXS-4-64 TRM with Conor in the replies to the V1 series in [1].

Cheers,
Matt

> 
>> +
>> +  dma-coherent: true
> 
> Best regards,
> Krzysztof
> 

[1]: https://lore.kernel.org/all/ff4e96e4-ebc2-4c50-9715-82ba3d7b8612@imgtec.com

-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]