| Message ID | 20250310-sets-bxs-4-64-patch-v1-v3-1-143b3dbef02f@imgtec.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Imagination BXS-4-64 MC1 GPU support | expand |
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. > + > + # 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. > + > + power-domain-names: > + items: > + - const: a That's not a useful name. Are you sure that datasheet calls it power domain A? > + > + dma-coherent: true Best regards, Krzysztof
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
On 11/03/2025 11:33, Matt Coster wrote: >>> 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? I was not precise/correct. This indeed is not an ABI break itself. However you will break the users of DTS when anyone applies such DTS. > > Would adding img,img-axe back into the updated list (bringing it to four > elements) be acceptable? Yes, you must keep all the compatibles. Affecting users because of marketing choices is a no-go. No one here cares about marketing. > >> >>> + >>> + # 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]. OK, that's fine. Best regards, Krzysztof
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 + + # 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 + + power-domain-names: + items: + - const: a + + dma-coherent: true required: - compatible @@ -47,6 +62,19 @@ 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 + + # Vendor integrations using a single clock domain - if: properties: compatible: @@ -64,10 +92,11 @@ examples: #include <dt-bindings/soc/ti,sci_pm_domain.h> gpu@fd00000 { - compatible = "ti,am62-gpu", "img,img-axe"; + compatible = "ti,am62-gpu", "img,img-axe-1-16m", "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"; };
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(-)