Re: [PATCH v2 2/5] ARM: dts: rockchip: Add power-controller for RK3128

[Alex Bee <knaerzche@gmail.com>]

Hi Heiko,

Am 03.12.23 um 17:42 schrieb Heiko Stübner:
> Hi Alex,
>
> Am Sonntag, 3. Dezember 2023, 17:05:47 CET schrieb Alex Bee:
>> Am 02.12.23 um 18:46 schrieb Heiko Stübner:
>>> Am Samstag, 2. Dezember 2023, 17:36:15 CET schrieb Alex Bee:
>>>> Am 02.12.23 um 16:51 schrieb Heiko Stübner:
>>>>> Am Samstag, 2. Dezember 2023, 13:51:41 CET schrieb Alex Bee:
>>>>>> Add power controller and qos nodes for RK3128 in order to use
>>>>>> them as powerdomains.
>>>>> does the power-domain controller work with the incomplete set of
>>>>> pm-domains too?
>>>> Yes, it does - the missing domains can request idle only and can't be
>>>> powered on/off - if no one requests idle they are just up all the time.
>>>>
>>>>> What I have in mind is
>>>>> - adding the power-controller node with the existing set of power-domains
>>>>> - the gpu pm-domain is in there
>>>>> - adding the gpu parts
>>>> My main concern about adding them later was the change of the ABI after
>>>> they've been exposed in the SoC DT. If that's not an issue - sure: I can
>>>> add them in a separate series.
>>> An ABI change would be _changing_ the domain-ids in the rk3128-power.h
>>> I think :-) .
>> Well, an addition is still a change.
>>> Right now the existing domain ids in the header are already exposed to the
>>> world, so someone could already use them, but not the new ones.
>> I'm fully aware that nothing would ever hard fail anywhere if the new
>> domain ids get added later.
>>
>> Nevertheless we start using here an ABI which is known to be incomplete.
>> For no reason, as the patches (which I am now asked to remove from this
>> series) for completion are already there (here).
>>
>> Anyway, if you prefer it this way: I'm pleased to do so.
> I was more thinking of accelerating the gpu-part of the series, as that
> really is just waiting for the power-domain node that already has driver
> support and domain-ids present.
>
> It looks like you're feeling more strongly about that though, so I'll
> definitly not pressure you ;-) .

I'm really not insisting on this - I just didn't understand why you 
would want this. And honestly I haven't considered merging timeline as 
an argument.

So sure: Let's get the low hanging fruits merged for 6.8. Having the 
power domains in the DT will also help for my upcoming hdmi / vop series.

Alex

>
> But I guess the split into IDs and driver change should still be
> done, especially as the dt-binding-header likely will want an Ack
> from the DT maintainers.
>
> And the power-domain change will go through the new pmdomain
> subsystem.
>
>
> Heiko
>
>
>>>>> And a second series with
>>>>> - patch1 from here
>>>>> - a dts patch adding the additional pm-domains to rk3128.dtsi
>>>>> - I guess patch1 also should be split into a patch adding the binding-ids
>>>>>      and a separate patch for the code addition.
>>>> Yeah, I noticed this also :)
>>>>
>>>> Regards,
>>>>
>>>> Alex
>>>>
>>>>> Heiko
>>>>>
>>>>>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>>>>>> ---
>>>>>>     arch/arm/boot/dts/rockchip/rk3128.dtsi | 101 +++++++++++++++++++++++++
>>>>>>     1 file changed, 101 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/rockchip/rk3128.dtsi b/arch/arm/boot/dts/rockchip/rk3128.dtsi
>>>>>> index 4e8b38604ecd..b72905db04f7 100644
>>>>>> --- a/arch/arm/boot/dts/rockchip/rk3128.dtsi
>>>>>> +++ b/arch/arm/boot/dts/rockchip/rk3128.dtsi
>>>>>> @@ -8,6 +8,7 @@
>>>>>>     #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>>>>     #include <dt-bindings/interrupt-controller/irq.h>
>>>>>>     #include <dt-bindings/pinctrl/rockchip.h>
>>>>>> +#include <dt-bindings/power/rk3128-power.h>
>>>>>>     
>>>>>>     / {
>>>>>>     	compatible = "rockchip,rk3128";
>>>>>> @@ -133,6 +134,106 @@ smp-sram@0 {
>>>>>>     	pmu: syscon@100a0000 {
>>>>>>     		compatible = "rockchip,rk3128-pmu", "syscon", "simple-mfd";
>>>>>>     		reg = <0x100a0000 0x1000>;
>>>>>> +
>>>>>> +		power: power-controller {
>>>>>> +			compatible = "rockchip,rk3128-power-controller";
>>>>>> +			#power-domain-cells = <1>;
>>>>>> +			#address-cells = <1>;
>>>>>> +			#size-cells = <0>;
>>>>>> +
>>>>>> +			power-domain@RK3128_PD_VIO {
>>>>>> +				reg = <RK3128_PD_VIO>;
>>>>>> +				clocks = <&cru ACLK_CIF>,
>>>>>> +					 <&cru HCLK_CIF>,
>>>>>> +					 <&cru DCLK_EBC>,
>>>>>> +					 <&cru HCLK_EBC>,
>>>>>> +					 <&cru ACLK_IEP>,
>>>>>> +					 <&cru HCLK_IEP>,
>>>>>> +					 <&cru ACLK_LCDC0>,
>>>>>> +					 <&cru HCLK_LCDC0>,
>>>>>> +					 <&cru PCLK_MIPI>,
>>>>>> +					 <&cru ACLK_RGA>,
>>>>>> +					 <&cru HCLK_RGA>,
>>>>>> +					 <&cru ACLK_VIO0>,
>>>>>> +					 <&cru ACLK_VIO1>,
>>>>>> +					 <&cru HCLK_VIO>,
>>>>>> +					 <&cru HCLK_VIO_H2P>,
>>>>>> +					 <&cru DCLK_VOP>,
>>>>>> +					 <&cru SCLK_VOP>;
>>>>>> +				pm_qos = <&qos_ebc>,
>>>>>> +					 <&qos_iep>,
>>>>>> +					 <&qos_lcdc>,
>>>>>> +					 <&qos_rga>,
>>>>>> +					 <&qos_vip>;
>>>>>> +				#power-domain-cells = <0>;
>>>>>> +			};
>>>>>> +
>>>>>> +			power-domain@RK3128_PD_VIDEO {
>>>>>> +				reg = <RK3128_PD_VIDEO>;
>>>>>> +				clocks = <&cru ACLK_VDPU>,
>>>>>> +					 <&cru HCLK_VDPU>,
>>>>>> +					 <&cru ACLK_VEPU>,
>>>>>> +					 <&cru HCLK_VEPU>,
>>>>>> +					 <&cru SCLK_HEVC_CORE>;
>>>>>> +				pm_qos = <&qos_vpu>;
>>>>>> +				#power-domain-cells = <0>;
>>>>>> +			};
>>>>>> +
>>>>>> +			power-domain@RK3128_PD_GPU {
>>>>>> +				reg = <RK3128_PD_GPU>;
>>>>>> +				clocks = <&cru ACLK_GPU>;
>>>>>> +				pm_qos = <&qos_gpu>;
>>>>>> +				#power-domain-cells = <0>;
>>>>>> +			};
>>>>>> +
>>>>>> +			power-domain@RK3128_PD_CRYPTO {
>>>>>> +				reg = <RK3128_PD_CRYPTO>;
>>>>>> +				clocks = <&cru HCLK_CRYPTO>,
>>>>>> +					 <&cru SCLK_CRYPTO>;
>>>>>> +				pm_qos = <&qos_crypto>;
>>>>>> +				#power-domain-cells = <0>;
>>>>>> +			};
>>>>>> +		};
>>>>>> +	};
>>>>>> +
>>>>>> +	qos_crypto: qos@10128080 {
>>>>>> +		compatible = "rockchip,rk3128-qos", "syscon";
>>>>>> +		reg = <0x10128080 0x20>;
>>>>>> +	};
>>>>>> +
>>>>>> +	qos_gpu: qos@1012d000 {
>>>>>> +		compatible = "rockchip,rk3128-qos", "syscon";
>>>>>> +		reg = <0x1012d000 0x20>;
>>>>>> +	};
>>>>>> +
>>>>>> +	qos_vpu: qos@1012e000 {
>>>>>> +		compatible = "rockchip,rk3128-qos", "syscon";
>>>>>> +		reg = <0x1012e000 0x20>;
>>>>>> +	};
>>>>>> +
>>>>>> +	qos_rga: qos@1012f000 {
>>>>>> +		compatible = "rockchip,rk3128-qos", "syscon";
>>>>>> +		reg = <0x1012f000 0x20>;
>>>>>> +	};
>>>>>> +
>>>>>> +	qos_ebc: qos@1012f080 {
>>>>>> +		compatible = "rockchip,rk3128-qos", "syscon";
>>>>>> +		reg = <0x1012f080 0x20>;
>>>>>> +	};
>>>>>> +
>>>>>> +	qos_iep: qos@1012f100 {
>>>>>> +		compatible = "rockchip,rk3128-qos", "syscon";
>>>>>> +		reg = <0x1012f100 0x20>;
>>>>>> +	};
>>>>>> +
>>>>>> +	qos_lcdc: qos@1012f180 {
>>>>>> +		compatible = "rockchip,rk3128-qos", "syscon";
>>>>>> +		reg = <0x1012f180 0x20>;
>>>>>> +	};
>>>>>> +
>>>>>> +	qos_vip: qos@1012f200 {
>>>>>> +		compatible = "rockchip,rk3128-qos", "syscon";
>>>>>> +		reg = <0x1012f200 0x20>;
>>>>>>     	};
>>>>>>     
>>>>>>     	gic: interrupt-controller@10139000 {
>>>>>>
>>>>>
>>>
>>>
>
>
>