On 23/07/2025 20:25, Michal Wilczynski wrote: > > > On 7/23/25 18:50, Matt Coster wrote: >> On 23/07/2025 17:26, Michal Wilczynski wrote: >>> On 7/23/25 11:45, Matt Coster wrote: >>>> On 25/06/2025 15:41, Krzysztof Kozlowski wrote: >>>>> On 25/06/2025 16:18, Michal Wilczynski wrote: >>>>>> >>>>>> >>>>>> On 6/25/25 15:55, Krzysztof Kozlowski wrote: >>>>>>> On 25/06/2025 14:45, Michal Wilczynski wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 6/24/25 15:53, Matt Coster wrote: >>>>>>>>> On 23/06/2025 12:42, Michal Wilczynski wrote: >>>>>>>>>> Update the img,powervr-rogue.yaml to include the T-HEAD TH1520 SoC's >>>>>>>>>> specific GPU compatible string. >>>>>>>>>> >>>>>>>>>> The thead,th1520-gpu compatible, along with its full chain >>>>>>>>>> img,img-bxm-4-64, and img,img-rogue, is added to the >>>>>>>>>> list of recognized GPU types. >>>>>>>>>> >>>>>>>>>> The power-domains property requirement for img,img-bxm-4-64 is also >>>>>>>>>> ensured by adding it to the relevant allOf condition. >>>>>>>>>> >>>>>>>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlow...@linaro.org> >>>>>>>>>> Reviewed-by: Ulf Hansson <ulf.hans...@linaro.org> >>>>>>>>>> Reviewed-by: Bartosz Golaszewski <bartosz.golaszew...@linaro.org> >>>>>>>>>> Signed-off-by: Michal Wilczynski <m.wilczyn...@samsung.com> >>>>>>>>>> --- >>>>>>>>>> Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml | 9 >>>>>>>>>> ++++++++- >>>>>>>>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>>>>>>>> >>>>>>>>>> diff --git >>>>>>>>>> a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml >>>>>>>>>> b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml >>>>>>>>>> index >>>>>>>>>> 4450e2e73b3ccf74d29f0e31e2e6687d7cbe5d65..9b241a0c1f5941dc58a1e23970f6d3773d427c22 >>>>>>>>>> 100644 >>>>>>>>>> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml >>>>>>>>>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml >>>>>>>>>> @@ -21,6 +21,11 @@ properties: >>>>>>>>>> # work with newer dts. >>>>>>>>>> - const: img,img-axe >>>>>>>>>> - const: img,img-rogue >>>>>>>>>> + - items: >>>>>>>>>> + - enum: >>>>>>>>>> + - thead,th1520-gpu >>>>>>>>>> + - const: img,img-bxm-4-64 >>>>>>>>>> + - const: img,img-rogue >>>>>>>>>> - items: >>>>>>>>>> - enum: >>>>>>>>>> - ti,j721s2-gpu >>>>>>>>>> @@ -93,7 +98,9 @@ allOf: >>>>>>>>>> properties: >>>>>>>>>> compatible: >>>>>>>>>> contains: >>>>>>>>>> - const: img,img-axe-1-16m >>>>>>>>>> + enum: >>>>>>>>>> + - img,img-axe-1-16m >>>>>>>>>> + - img,img-bxm-4-64 >>>>>>>>> >>>>>>>>> This isn't right – BXM-4-64 has two power domains like BXS-4-64. I >>>>>>>>> don't >>>>>>>>> really know what the right way to handle that in devicetree is given >>>>>>>>> the >>>>>>>>> TH1520 appears to expose only a top-level domain for the entire GPU, >>>>>>>>> but >>>>>>>>> there are definitely two separate domains underneath that as far as >>>>>>>>> the >>>>>>>>> GPU is concerned (see the attached snippet from integration guide). >>>>>>>>> >>>>>>>>> Since power nodes are ref-counted anyway, do we just use the same node >>>>>>>>> for both domains and let the driver up/down-count it twice? >>>>>>>> >>>>>>>> Hi Matt, >>>>>>>> >>>>>>>> Thanks for the very helpful insight. That's a great point, it seems the >>>>>>>> SoC's design presents a tricky case for the bindings. >>>>>>>> >>>>>>>> I see what you mean about potentially using the same power domain node >>>>>>>> twice. My only hesitation is that it might be a bit unclear for someone >>>>>>>> reading the devicetree later. Perhaps another option could be to relax >>>>>>>> the constraint for this compatible? >>>>>>>> >>>>>>>> Krzysztof, we'd be grateful for your thoughts on how to best model this >>>>>>>> situation. >>>>>>> >>>>>>> >>>>>>> It's your hardware, you should tell us, not me. I don't know how many >>>>>>> power domains you have there, but for sure it is not one AND two domains >>>>>>> the same time. It is either one or two, because power domains are not >>>>>>> the same as regulator supplies. >>>>>> >>>>>> Hi Krzysztof, Matt, >>>>>> >>>>>> The img,bxm-4-64 GPU IP itself is designed with two separate power >>>>>> domains. The TH1520 SoC, which integrates this GPU, wires both of these >>>>>> to a single OS controllable power gate (controlled via mailbox and E902 >>>>>> co-processor). >>>>> >>>>> This helps... and also sounds a lot like regulator supplies, not power >>>>> domains. :/ >>>> >>>> Apologies for taking so long to get back to you with this, I wanted to >>>> make sure I had the whole picture from our side before commenting again. >>>> >>>> From the GPU side, a "typical" integration of BXM-4-64 would use two >>>> power domains. >>>> >>>> Typically, these domains exist in silicon, regardless of whether they >>>> are exposed to the host OS, because the SoC's power controller must have >>>> control over them. As part of normal operation, the GPU firmware (always >>>> in domain "a" on Rogue) will request the power-up/down of the other >>>> domains, including during the initial boot sequence. This all happens >>>> transparently to the OS. The GPU block itself has no power gating at >>>> that level, it relies entirely on the SoC integration. >>>> >>>> However, it turns out (unknown to me until very recently) that this >>>> functionality is optional. The integrator can opt to forego the >>>> power-saving functionality afforded by firmware-controlled power gating >>>> and just throw everything into a single domain, which appears to be >>>> what's happened here. >>>> >>>> My only remaining issue here, then, is the naming. Since this >>>> integration doesn't use discrete domains, saying it has one domain >>>> called "a" isn't correct*. We should either: >>>> >>>> - Drop the name altogether for this integration (and others like it >>>> that don't use the low-power functionality, if there are any), or >>> >>> Hi Matt, >>> >>> Thanks for the detailed explanation, that clears things up perfectly. >> >> I'm glad I could get to the bottom of this one, it was bothering me too! >> >>> >>> I agree with your assessment. Dropping the power-domain-names property >>> for this integration seems like the cleanest solution. As you pointed >>> out, since the OS sees only a single, undifferentiated power domain, >>> giving it a name like "gpu" would be redundant. This approach correctly >>> models the hardware without setting a potentially confusing precedent. >> >> That seems reasonable. I was very much on the fence for this one, so >> I'll happily go along with dropping the name altogether. >> >> Just to make sure I understand correctly, the change here would be to >> move "required: - power-domain-names" from "img,img-rogue" to every >> conditional block that constrains the number of domains? >> >> Can we add negative constraints in conditionals? Then we could add >> "power-domain-names: false" to the "thead,th1520-gpu" conditional block >> alongside "power-domains: maxItems: 1". > > Yeah the diff with v7 would look like so: > diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml > b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml > index 263c40c8438e..338746f39cbb 100644 > --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml > +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml > @@ -89,6 +89,11 @@ allOf: > compatible: > contains: > const: img,img-rogue > + not: > + properties: > + compatible: > + contains: > + const: thead,th1520-gpu
No, don't do that. Anyway, if you are going to rewrite patch, you MUST drop all tags, document it and explicitly ask for re-review. Best regards, Krzysztof
