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

[Alex Bee <knaerzche@gmail.com>]

Hi Heiko,

Am 02.12.23 um 18:46 schrieb Heiko Stübner:
> Hi Alex,
>
> 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.

Alex

>
> 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 {
>>>>
>>>
>>>
>
>
>