| Message ID | 20241105-sets-bxs-4-64-patch-v1-v1-1-4ed30e865892@imgtec.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Imagination BXS-4-64 MC1 GPU support | expand |
On Tue, Nov 05, 2024 at 03:58:07PM +0000, Matt Coster wrote: > The current compatible strings are not specific enough to constrain the > hardware in devicetree. For example, the current "img,img-axe" string > refers to the entire family of Series AXE GPUs. The more specific > "img,img-axe-1-16m" string refers to the AXE-1-16M GPU which, unlike the > rest of its family, only uses a single power domain. That's not true, you could apply the rules using the ti,am62-gpu compatible, no? > Work is currently underway to add support for volcanic-based Imagination > GPUs; also add a generic top-level "img,img-rogue" compatible string to > allow for simpler differentiation in devicetrees. This makes no sense, how does adding another fallback compatible make it simpler? I have to assume that this means there will be geothermally powered axes in the future and you want to tell the difference between them and those pesky rogue axes that chop the heads off of naughty children? > > Make these changes now before introducing more compatible strings to keep > the legacy versions to a minimum. > > Signed-off-by: Matt Coster <matt.coster@imgtec.com> > --- > .../devicetree/bindings/gpu/img,powervr-rogue.yaml | 19 ++++++++++++++----- > 1 file changed, 14 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..ef7070daf213277d0190fe319e202fdc597337d4 100644 > --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml > +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml > @@ -12,10 +12,19 @@ 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. Keep it around here for compatibility, but never use > + # "img,img-axe" in new devicetrees. > + - items: > + - const: ti,am62-gpu > + - const: img,img-axe > > reg: > maxItems: 1 > @@ -64,7 +73,7 @@ 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"; > > -- > 2.47.0 >
On 05/11/2024 18:13, Conor Dooley wrote: > On Tue, Nov 05, 2024 at 03:58:07PM +0000, Matt Coster wrote: >> The current compatible strings are not specific enough to constrain the >> hardware in devicetree. For example, the current "img,img-axe" string >> refers to the entire family of Series AXE GPUs. The more specific >> "img,img-axe-1-16m" string refers to the AXE-1-16M GPU which, unlike the >> rest of its family, only uses a single power domain. > > That's not true, you could apply the rules using the ti,am62-gpu > compatible, no? The intent here is to draw a line between properties inherent to the IP core and choices made by the silicon vendor at integration time. The number of power domains is a property of the IP core, whereas the decision to use one or three clocks (next patch) is a vendor choice. >> Work is currently underway to add support for volcanic-based Imagination >> GPUs; also add a generic top-level "img,img-rogue" compatible string to >> allow for simpler differentiation in devicetrees. > > This makes no sense, how does adding another fallback compatible make > it simpler? I have to assume that this means there will be geothermally > powered axes in the future and you want to tell the difference between > them and those pesky rogue axes that chop the heads off of naughty > children? The intent is to add Volcanic bindings in img,powervr-volcanic.yaml, but the split between Rogue and Volcanic is... a bit weird. The BXS-4-64 we're adding support for here is Rogue but, for example, the BXS-32-1024 is Volcanic. I figured it would be nice to be able to grok from the devicetree which architecture a core is using without having to refer back to the bindings. The primary differentiator from a dt perspective is power topology. In Rogue, there are few (poorly named) power domains. Volcanic has (a) better domain names and (b) typically more granularity in domains, leading to actual dependency trees we need to care about besides the trivial A->B->... in Rogue. Cheers, Matt >> Make these changes now before introducing more compatible strings to keep >> the legacy versions to a minimum. >> >> Signed-off-by: Matt Coster <matt.coster@imgtec.com> >> --- >> .../devicetree/bindings/gpu/img,powervr-rogue.yaml | 19 ++++++++++++++----- >> 1 file changed, 14 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..ef7070daf213277d0190fe319e202fdc597337d4 100644 >> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml >> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml >> @@ -12,10 +12,19 @@ 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. Keep it around here for compatibility, but never use >> + # "img,img-axe" in new devicetrees. >> + - items: >> + - const: ti,am62-gpu >> + - const: img,img-axe >> >> reg: >> maxItems: 1 >> @@ -64,7 +73,7 @@ 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"; >> >> -- >> 2.47.0 >>
On Wed, Nov 06, 2024 at 10:17:47AM +0000, Matt Coster wrote: > On 05/11/2024 18:13, Conor Dooley wrote: > > On Tue, Nov 05, 2024 at 03:58:07PM +0000, Matt Coster wrote: > >> The current compatible strings are not specific enough to constrain the > >> hardware in devicetree. For example, the current "img,img-axe" string > >> refers to the entire family of Series AXE GPUs. The more specific > >> "img,img-axe-1-16m" string refers to the AXE-1-16M GPU which, unlike the > >> rest of its family, only uses a single power domain. > > > > That's not true, you could apply the rules using the ti,am62-gpu > > compatible, no? > > The intent here is to draw a line between properties inherent to the IP > core and choices made by the silicon vendor at integration time. The > number of power domains is a property of the IP core, whereas the > decision to use one or three clocks (next patch) is a vendor choice. That's a different argument than the one in your commit message, but I will accept it. > >> Work is currently underway to add support for volcanic-based Imagination > >> GPUs; also add a generic top-level "img,img-rogue" compatible string to > >> allow for simpler differentiation in devicetrees. > > > > This makes no sense, how does adding another fallback compatible make > > it simpler? I have to assume that this means there will be geothermally > > powered axes in the future and you want to tell the difference between > > them and those pesky rogue axes that chop the heads off of naughty > > children? > > The intent is to add Volcanic bindings in img,powervr-volcanic.yaml, but > the split between Rogue and Volcanic is... a bit weird. The BXS-4-64 > we're adding support for here is Rogue but, for example, the BXS-32-1024 > is Volcanic. I figured it would be nice to be able to grok from the > devicetree which architecture a core is using without having to refer > back to the bindings. So not geothermally powered axes, but it was the rationale. > The primary differentiator from a dt perspective is power topology. In > Rogue, there are few (poorly named) power domains. Volcanic has (a) > better domain names and (b) typically more granularity in domains, > leading to actual dependency trees we need to care about besides the > trivial A->B->... in Rogue. Please add that detail to the commit message. Not all of it, but the bits in the first paragraph. Cheers, Conor.
diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml index 256e252f8087fa0d6081f771a01601d34b66fe19..ef7070daf213277d0190fe319e202fdc597337d4 100644 --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml @@ -12,10 +12,19 @@ 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. Keep it around here for compatibility, but never use + # "img,img-axe" in new devicetrees. + - items: + - const: ti,am62-gpu + - const: img,img-axe reg: maxItems: 1 @@ -64,7 +73,7 @@ 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";
The current compatible strings are not specific enough to constrain the hardware in devicetree. For example, the current "img,img-axe" string refers to the entire family of Series AXE GPUs. The more specific "img,img-axe-1-16m" string refers to the AXE-1-16M GPU which, unlike the rest of its family, only uses a single power domain. Work is currently underway to add support for volcanic-based Imagination GPUs; also add a generic top-level "img,img-rogue" compatible string to allow for simpler differentiation in devicetrees. Make these changes now before introducing more compatible strings to keep the legacy versions to a minimum. Signed-off-by: Matt Coster <matt.coster@imgtec.com> --- .../devicetree/bindings/gpu/img,powervr-rogue.yaml | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)