Re: [PATCH RFC 24/24] arm64: dts: qcom: x1e80100: Describe GPU_CC power plumbing requirements

[Akhil P Oommen <akhilpo@oss.qualcomm.com>]

On 7/29/2025 9:14 PM, Stephan Gerhold wrote:
> On Tue, Jul 29, 2025 at 03:28:55PM +0200, Konrad Dybcio wrote:
>> On 7/29/25 10:23 AM, Konrad Dybcio wrote:
>>> On 7/29/25 8:34 AM, Stephan Gerhold wrote:
>>>> On Mon, Jul 28, 2025 at 11:31:10PM +0200, Konrad Dybcio wrote:
>>>>> On 7/28/25 7:10 PM, Stephan Gerhold wrote:
>>>>>> On Mon, Jul 28, 2025 at 06:16:24PM +0200, Konrad Dybcio wrote:
>>>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>>
>>>>>>> A number of power rails must be powered on in order for GPU_CC to
>>>>>>> function. Ensure that's conveyed to the OS.
>>>>>>>
>>>>>>> Fixes: 721e38301b79 ("arm64: dts: qcom: x1e80100: Add gpu support")
>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>> ---
>>>>>>>  arch/arm64/boot/dts/qcom/x1e80100.dtsi | 6 ++++++
>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>>>>> index 5e9a8fa3cf96468b12775f91192cbd779d5ce946..6620517fbb0f3ed715c4901ec53dcbc6235be88f 100644
>>>>>>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>>>>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>>>>> @@ -3928,6 +3928,12 @@ gpucc: clock-controller@3d90000 {
>>>>>>>  			clocks = <&bi_tcxo_div2>,
>>>>>>>  				 <&gcc GCC_GPU_GPLL0_CPH_CLK_SRC>,
>>>>>>>  				 <&gcc GCC_GPU_GPLL0_DIV_CPH_CLK_SRC>;
>>>>>>> +
>>>>>>> +			power-domains = <&rpmhpd RPMHPD_CX>,
>>>>>>> +					<&rpmhpd RPMHPD_MX>,
>>>>>>> +					<&rpmhpd RPMHPD_GFX>,
>>>>>>> +					<&rpmhpd RPMHPD_GMXC>;
>>>>>>> +
>>>>>>>  			#clock-cells = <1>;
>>>>>>>  			#reset-cells = <1>;
>>>>>>>  			#power-domain-cells = <1>;
>>>>>>>
>>>>>>
>>>>>> To repeat your own message from a couple of months back [1]:
>>>>>>
>>>>>>> You shouldn't be messing with VDD_GFX on platforms with a GMU.
>>>>>>>
>>>>>>> Parts of the clock controller are backed by one of the MX rails,
>>>>>>> with some logic depending on CX/GFX, but handling of the latter is
>>>>>>> fully deferred to the GMU firmware.
>>>>>>>
>>>>>>> Konrad
>>>>>>
>>>>>> Please describe somewhere in the cover letter or the individual patches
>>>>>> how this relates to the responsibilities of the GMU. I searched for
>>>>>> "GMU" in the patch series and couldn't find any note about this.
>>>>>>
>>>>>> Also: How much is a plain "power on" votes (without a corresponding
>>>>>> "required-opps") really worth nowadays? An arbitrary low voltage level
>>>>>> on those rails won't be sufficient to make the GPU_CC actually
>>>>>> "function". Do you need "required-opps" here? In the videocc/camcc case
>>>>>> we have those.
>>>>>
>>>>> Right, I failed to capture this.
>>>>>
>>>>> The GFX rail should be powered on before unclamping the GX_GDSC (as
>>>>> per the programming guide). The clock controller HPG however doesn't
>>>>> seem to have a concept of RPMh, so it says something that amounts to
>>>>> "tell the PMIC to supply power on this rail". In Linux, since Commit
>>>>> e3e56c050ab6 ("soc: qcom: rpmhpd: Make power_on actually enable the
>>>>> domain") we don't really need a defined level for this (perhaps it's
>>>>> more ""portable"" across potential fuse-bins if we don't hardcode the
>>>>> lowest level anyway?).
>>>>
>>>> Thanks, I forgot that we have this commit.
>>>>
>>>>>
>>>>> However after that happens, the level scaling is done by the GMU
>>>>> firmware. This holds for allOf CX/MX/GFX. I'm not super sure if
>>>>> both MX and (G)MXC need to both be captured together - downstream
>>>>> seems to describe MXC as a child of MX (in socname-regulators.dtsi),
>>>>> but I'm not really sure this is true in hardware.
>>>>>
>>>>> The GPU driver currently first enables the GX_GDSC and only then
>>>>> does it kickstart the GMU firmware. Downstream seems to do that as
>>>>> well. So on a second thought, since we've not seen any errors so
>>>>> far, it calls into question what role the GFX rail plays in the
>>>>> GX_GDSC's powering up..
>>>>>
>>>>
>>>> It might play a role, but we wouldn't know since AFAICT we don't support
>>>> enabling the GX_GDSC. Look at the beautiful gdsc_gx_do_nothing_enable()
>>>> function, it basically just defers the entire task to the GMU. The GDSC
>>>> just exists in Linux so we can turn it *off* during GMU crashes. :D
>>>
>>> OHHHHH snap! I, on the other hand, forgot we have *that* commit..
>>>
>>>> I think we should identify precisely which votes we are missing, instead
>>>> of making blanket votes for all the power rails somehow related to the
>>>> GPU. In this case this means: Which rails do we need to vote for to make
>>>> the GMU turn on? If there are no votes necessary after the GMU is on,
>>>> it's better to have none IMO.
>>>
>>> The GMU pokes at RPMh directly (see a6xx_hfi.c), so we indeed just
>>> need to make sure that it can turn on.. Which in short means the
>>> *C*X_GDSC must be able to power up, which doesn't have any special
>>> requirements. The only question that's left is basically whether
>>> MX_C must be on. I'll try testing that in practice.
>>
>> So this is apparently difficult, at least on SC8280XP, where something
>> seems to be voting on MXC and it only seems to shut down when entering
>> CXPC. I would imagine/hope this is not the case on newer platforms, but
>> I don't have a way to fully confirm this at the moment..
>>
> 
> If in doubt, I would suggest to leave everything as-is for now until
> someone actually runs into an issue caused by this (if this is even
> possible). There are plenty other actual gaps to address. ;)

Konrad,

GMU is allowed to collapse some of the rails in some cases (GX/MXC/GXMXC
etc). So there should not be any other vote for these resources on
behalf of GPU/GMU from the KMD side. You may have to vote some resources
for GPUCC block itself (I know it is in AO domain, but still). I don't
know the specifics. Can you reach out to QC clk team (Taniya/Jagadeesh
etc) for necessary details? We should be careful here. Just boot up
testing is not sufficient when it comes to clk/power.

-Akhil.

> 
> Stephan