diff mbox series

[v2,1/3] ARM: dts: rockchip: Add psci for rk3036

Message ID 20231218105523.2478315-2-andyshrk@163.com
State Superseded
Headers show
Series Fix mainline support for rk3036 kylin board | expand

Commit Message

Andy Yan Dec. 18, 2023, 10:55 a.m. UTC
From: Andy Yan <andy.yan@rock-chips.com>

The system will hang at bringup secondary CPUs
without psci node.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

(no changes since v1)

 arch/arm/boot/dts/rockchip/rk3036.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Alex Bee Dec. 21, 2023, 3:58 p.m. UTC | #1
Hi Andy,

Am 21.12.23 um 02:07 schrieb Andy Yan:
> Hi Alex:
>
> On 12/20/23 19:16, Alex Bee wrote:
>> Hi Andy,
>> Am 18.12.23 um 11:55 schrieb Andy Yan:
>>> From: Andy Yan <andy.yan@rock-chips.com>
>>>
>>> The system will hang at bringup secondary CPUs
>>> without psci node.
>>>
>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>>
>>> ---
>>>
>>> (no changes since v1)
>>>
>>>   arch/arm/boot/dts/rockchip/rk3036.dtsi | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/rockchip/rk3036.dtsi 
>>> b/arch/arm/boot/dts/rockchip/rk3036.dtsi
>>> index 78686fc72ce6..5344803442a1 100644
>>> --- a/arch/arm/boot/dts/rockchip/rk3036.dtsi
>>> +++ b/arch/arm/boot/dts/rockchip/rk3036.dtsi
>>> @@ -67,6 +67,11 @@ display-subsystem {
>>>           ports = <&vop_out>;
>>>       };
>>> +    psci {
>>> +        compatible = "arm,psci-1.0";
>>> +        method = "smc";
>>> +    };
>>> +
>> I don't think that's an good idea. 
>
> Why?
It's only what I've been told before: We shoudn't add properties which
depend on non-upstream software (if an upstream alternative exists). Also
I'm not sure what happens if somebody doesn't use downstream bootloader
and PSCI can't be found: Would the board still be able to boot?
>
>> You most likely need that because you have downstream bootloader 
>> installed on this board. PSCI implementation takes place in TEE-OS 
>> for Rockchips ARM SoCs. There is no support for RK3036 in upstream 
>> op-tee OS. It's pretty much the same for RK3128 and RK3288.
>> If you use upstream u-boot it should be good as-is.
>
> Unfortunately, upstream u-boot also cannot boot up on this board.
> At present, I haven't had time to debug what is going on.
>
> Another reason I want to use downstream u-boot it is: I try run
> this board with mainline just because i want to test some community
> patches about inno-hdmi driver, as you said "the inno-hdmi driver 
> currently gets a lot of attention"[0]
>
Thanks for helping testing this.

Alex

> With a downstream u-boot I can easy switch between upstream kernel and 
> downstream kernel(no need to replace other components)
> if I found some function is not work as expected.
>
>
> [0]https://patchwork.kernel.org/project/linux-rockchip/cover/20231219170100.188800-1-knaerzche@gmail.com/ 
>
>>
>> Alex
>>>       timer {
>>>           compatible = "arm,armv7-timer";
>>>           arm,cpu-registers-not-fw-configured;
>>
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Andy Yan Dec. 22, 2023, 10:37 a.m. UTC | #2
Hi Alex,

On 12/21/23 23:58, Alex Bee wrote:
> Hi Andy,
> 
> Am 21.12.23 um 02:07 schrieb Andy Yan:
>> Hi Alex:
>>
>> On 12/20/23 19:16, Alex Bee wrote:
>>> Hi Andy,
>>> Am 18.12.23 um 11:55 schrieb Andy Yan:
>>>> From: Andy Yan <andy.yan@rock-chips.com>
>>>>
>>>> The system will hang at bringup secondary CPUs
>>>> without psci node.
>>>>
>>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>>>
>>>> ---
>>>>
>>>> (no changes since v1)
>>>>
>>>>   arch/arm/boot/dts/rockchip/rk3036.dtsi | 5 +++++
>>>>   1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/rockchip/rk3036.dtsi b/arch/arm/boot/dts/rockchip/rk3036.dtsi
>>>> index 78686fc72ce6..5344803442a1 100644
>>>> --- a/arch/arm/boot/dts/rockchip/rk3036.dtsi
>>>> +++ b/arch/arm/boot/dts/rockchip/rk3036.dtsi
>>>> @@ -67,6 +67,11 @@ display-subsystem {
>>>>           ports = <&vop_out>;
>>>>       };
>>>> +    psci {
>>>> +        compatible = "arm,psci-1.0";
>>>> +        method = "smc";
>>>> +    };
>>>> +
>>> I don't think that's an good idea. 
>>
>> Why?
> It's only what I've been told before: We shoudn't add properties which
> depend on non-upstream software (if an upstream alternative exists). Also

> I'm not sure what happens if somebody doesn't use downstream bootloader
> and PSCI can't be found: Would the board still be able to boot?

The psci is released herel[0].
And also, there are only two rk3036 based boards in mainline:
rk3036-evb,rk3036-keylin, both of them from rockchip.
And the mainline support of these boards broken from linux 5.7(2020, see PATCH 3),
no one report this broken until I try to run this board now.
So maybe there is no such somebody will run into this situation.


[0]https://github.com/rockchip-linux/rkbin/tree/master/bin/rk30

>>
>>> You most likely need that because you have downstream bootloader installed on this board. PSCI implementation takes place in TEE-OS for Rockchips ARM SoCs. There is no support for RK3036 in upstream op-tee OS. It's pretty much the same for RK3128 and RK3288.
>>> If you use upstream u-boot it should be good as-is.
>>
>> Unfortunately, upstream u-boot also cannot boot up on this board.
>> At present, I haven't had time to debug what is going on.
>>
>> Another reason I want to use downstream u-boot it is: I try run
>> this board with mainline just because i want to test some community
>> patches about inno-hdmi driver, as you said "the inno-hdmi driver currently gets a lot of attention"[0]
>>
> Thanks for helping testing this.
> 
> Alex
> 
>> With a downstream u-boot I can easy switch between upstream kernel and downstream kernel(no need to replace other components)
>> if I found some function is not work as expected.
>>
>>
>> [0]https://patchwork.kernel.org/project/linux-rockchip/cover/20231219170100.188800-1-knaerzche@gmail.com/
>>>
>>> Alex
>>>>       timer {
>>>>           compatible = "arm,armv7-timer";
>>>>           arm,cpu-registers-not-fw-configured;
>>>
>>>
>>> _______________________________________________
>>> Linux-rockchip mailing list
>>> Linux-rockchip@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Alex Bee Dec. 22, 2023, 3:37 p.m. UTC | #3
Hi Andy,

Am 22.12.23 um 11:37 schrieb Andy Yan:
> Hi Alex,
>
> On 12/21/23 23:58, Alex Bee wrote:
>> Hi Andy,
>>
>> Am 21.12.23 um 02:07 schrieb Andy Yan:
>>> Hi Alex:
>>>
>>> On 12/20/23 19:16, Alex Bee wrote:
>>>> Hi Andy,
>>>> Am 18.12.23 um 11:55 schrieb Andy Yan:
>>>>> From: Andy Yan <andy.yan@rock-chips.com>
>>>>>
>>>>> The system will hang at bringup secondary CPUs
>>>>> without psci node.
>>>>>
>>>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>>>>
>>>>> ---
>>>>>
>>>>> (no changes since v1)
>>>>>
>>>>>   arch/arm/boot/dts/rockchip/rk3036.dtsi | 5 +++++
>>>>>   1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/rockchip/rk3036.dtsi 
>>>>> b/arch/arm/boot/dts/rockchip/rk3036.dtsi
>>>>> index 78686fc72ce6..5344803442a1 100644
>>>>> --- a/arch/arm/boot/dts/rockchip/rk3036.dtsi
>>>>> +++ b/arch/arm/boot/dts/rockchip/rk3036.dtsi
>>>>> @@ -67,6 +67,11 @@ display-subsystem {
>>>>>           ports = <&vop_out>;
>>>>>       };
>>>>> +    psci {
>>>>> +        compatible = "arm,psci-1.0";
>>>>> +        method = "smc";
>>>>> +    };
>>>>> +
>>>> I don't think that's an good idea. 
>>>
>>> Why?
>> It's only what I've been told before: We shoudn't add properties which
>> depend on non-upstream software (if an upstream alternative exists). 
>> Also
>
>> I'm not sure what happens if somebody doesn't use downstream bootloader
>> and PSCI can't be found: Would the board still be able to boot?
>
> The psci is released herel[0].
> And also, there are only two rk3036 based boards in mainline:
> rk3036-evb,rk3036-keylin, both of them from rockchip.
> And the mainline support of these boards broken from linux 5.7(2020, 
> see PATCH 3),
> no one report this broken until I try to run this board now.
> So maybe there is no such somebody will run into this situation.
>
>
> [0]https://github.com/rockchip-linux/rkbin/tree/master/bin/rk30
>
Yeah sure, there are binary release for downstream optee implementations,
but the won't work with upstream u-boot, since they are using a calling
convention different from upstream optee os. I tried to add support for
those, but it was rejected [0].

Upstream bootflow doesn't implement/need PSCI as it uses a SMP bringup
method which is done in kernel only (see enable-method in cpu section). I
guess that it doesn't work for you is most likely because the downstream
tee os makes the cpu registers the upstream enable-method uses only
accessible in a higher exception level.

I can undust my RK3036 evb board during christmas holidays and check whats
broken for RK3036 in u-boot. Everything was working fine (including SMP
bringup w/o PSCI and upstream u-boot) when I was adding VPU support in 5.14
[1]. I likely booted from TF-card rather than eMMC.

[0] 
https://patchwork.ozlabs.org/project/uboot/patch/20230718145715.1280201-6-knaerzche@gmail.com/
[1] 
https://patchwork.kernel.org/project/linux-rockchip/patch/20210527154455.358869-11-knaerzche@gmail.com/

Nice holidays!

Alex

>>>
>>>> You most likely need that because you have downstream bootloader 
>>>> installed on this board. PSCI implementation takes place in TEE-OS 
>>>> for Rockchips ARM SoCs. There is no support for RK3036 in upstream 
>>>> op-tee OS. It's pretty much the same for RK3128 and RK3288.
>>>> If you use upstream u-boot it should be good as-is.
>>>
>>> Unfortunately, upstream u-boot also cannot boot up on this board.
>>> At present, I haven't had time to debug what is going on.
>>>
>>> Another reason I want to use downstream u-boot it is: I try run
>>> this board with mainline just because i want to test some community
>>> patches about inno-hdmi driver, as you said "the inno-hdmi driver 
>>> currently gets a lot of attention"[0]
>>>
>> Thanks for helping testing this.
>>
>> Alex
>>
>>> With a downstream u-boot I can easy switch between upstream kernel 
>>> and downstream kernel(no need to replace other components)
>>> if I found some function is not work as expected.
>>>
>>>
>>> [0]https://patchwork.kernel.org/project/linux-rockchip/cover/20231219170100.188800-1-knaerzche@gmail.com/ 
>>>
>>>>
>>>> Alex
>>>>>       timer {
>>>>>           compatible = "arm,armv7-timer";
>>>>>           arm,cpu-registers-not-fw-configured;
>>>>
>>>>
>>>> _______________________________________________
>>>> Linux-rockchip mailing list
>>>> Linux-rockchip@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Andy Yan Dec. 23, 2023, 11:14 a.m. UTC | #4
Hi Alex,

On 12/22/23 23:37, Alex Bee wrote:
> Hi Andy,
> 
> Am 22.12.23 um 11:37 schrieb Andy Yan:
>> Hi Alex,
>>
>> On 12/21/23 23:58, Alex Bee wrote:
>>> Hi Andy,
>>>
>>> Am 21.12.23 um 02:07 schrieb Andy Yan:
>>>> Hi Alex:
>>>>
>>>> On 12/20/23 19:16, Alex Bee wrote:
>>>>> Hi Andy,
>>>>> Am 18.12.23 um 11:55 schrieb Andy Yan:
>>>>>> From: Andy Yan <andy.yan@rock-chips.com>
>>>>>>
>>>>>> The system will hang at bringup secondary CPUs
>>>>>> without psci node.
>>>>>>
>>>>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> (no changes since v1)
>>>>>>
>>>>>>   arch/arm/boot/dts/rockchip/rk3036.dtsi | 5 +++++
>>>>>>   1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/rockchip/rk3036.dtsi b/arch/arm/boot/dts/rockchip/rk3036.dtsi
>>>>>> index 78686fc72ce6..5344803442a1 100644
>>>>>> --- a/arch/arm/boot/dts/rockchip/rk3036.dtsi
>>>>>> +++ b/arch/arm/boot/dts/rockchip/rk3036.dtsi
>>>>>> @@ -67,6 +67,11 @@ display-subsystem {
>>>>>>           ports = <&vop_out>;
>>>>>>       };
>>>>>> +    psci {
>>>>>> +        compatible = "arm,psci-1.0";
>>>>>> +        method = "smc";
>>>>>> +    };
>>>>>> +
>>>>> I don't think that's an good idea. 
>>>>
>>>> Why?
>>> It's only what I've been told before: We shoudn't add properties which
>>> depend on non-upstream software (if an upstream alternative exists). Also
>>
>>> I'm not sure what happens if somebody doesn't use downstream bootloader
>>> and PSCI can't be found: Would the board still be able to boot?
>>
>> The psci is released herel[0].
>> And also, there are only two rk3036 based boards in mainline:
>> rk3036-evb,rk3036-keylin, both of them from rockchip.
>> And the mainline support of these boards broken from linux 5.7(2020, see PATCH 3),
>> no one report this broken until I try to run this board now.
>> So maybe there is no such somebody will run into this situation.
>>
>>
>> [0]https://github.com/rockchip-linux/rkbin/tree/master/bin/rk30
>>
> Yeah sure, there are binary release for downstream optee implementations,
> but the won't work with upstream u-boot, since they are using a calling
> convention different from upstream optee os. I tried to add support for
> those, but it was rejected [0].
> 
> Upstream bootflow doesn't implement/need PSCI as it uses a SMP bringup
> method which is done in kernel only (see enable-method in cpu section). I
> guess that it doesn't work for you is most likely because the downstream
> tee os makes the cpu registers the upstream enable-method uses only
> accessible in a higher exception level.
> 
> I can undust my RK3036 evb board during christmas holidays and check whats
> broken for RK3036 in u-boot. Everything was working fine (including SMP

With one day bisect, I finally fond the mainline u-boot support for rk3036
was broken by this commit f113d7d30346 (" Convert CONFIG_SPL_STACK to Kconfig")

It makes rk3036 can't set SPL_STACK as rk3036 can't enable SPL_FRAMEWORK(very small sram)

I still don't have a proper patch to fix this issue.

Thank you very much for you explanation.
I think i should not add psci to mainline before the tee binary can be used by mainline u-boot.

Nice holidays!
> bringup w/o PSCI and upstream u-boot) when I was adding VPU support in 5.14
> [1]. I likely booted from TF-card rather than eMMC.
> 
> [0] https://patchwork.ozlabs.org/project/uboot/patch/20230718145715.1280201-6-knaerzche@gmail.com/
> [1] https://patchwork.kernel.org/project/linux-rockchip/patch/20210527154455.358869-11-knaerzche@gmail.com/
> 
> Nice holidays!
> 
> Alex
> 
>>>>
>>>>> You most likely need that because you have downstream bootloader installed on this board. PSCI implementation takes place in TEE-OS for Rockchips ARM SoCs. There is no support for RK3036 in upstream op-tee OS. It's pretty much the same for RK3128 and RK3288.
>>>>> If you use upstream u-boot it should be good as-is.
>>>>
>>>> Unfortunately, upstream u-boot also cannot boot up on this board.
>>>> At present, I haven't had time to debug what is going on.
>>>>
>>>> Another reason I want to use downstream u-boot it is: I try run
>>>> this board with mainline just because i want to test some community
>>>> patches about inno-hdmi driver, as you said "the inno-hdmi driver currently gets a lot of attention"[0]
>>>>
>>> Thanks for helping testing this.
>>>
>>> Alex
>>>
>>>> With a downstream u-boot I can easy switch between upstream kernel and downstream kernel(no need to replace other components)
>>>> if I found some function is not work as expected.
>>>>
>>>>
>>>> [0]https://patchwork.kernel.org/project/linux-rockchip/cover/20231219170100.188800-1-knaerzche@gmail.com/
>>>>>
>>>>> Alex
>>>>>>       timer {
>>>>>>           compatible = "arm,armv7-timer";
>>>>>>           arm,cpu-registers-not-fw-configured;
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Linux-rockchip mailing list
>>>>> Linux-rockchip@lists.infradead.org
>>>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/rockchip/rk3036.dtsi b/arch/arm/boot/dts/rockchip/rk3036.dtsi
index 78686fc72ce6..5344803442a1 100644
--- a/arch/arm/boot/dts/rockchip/rk3036.dtsi
+++ b/arch/arm/boot/dts/rockchip/rk3036.dtsi
@@ -67,6 +67,11 @@  display-subsystem {
 		ports = <&vop_out>;
 	};
 
+	psci {
+		compatible = "arm,psci-1.0";
+		method = "smc";
+	};
+
 	timer {
 		compatible = "arm,armv7-timer";
 		arm,cpu-registers-not-fw-configured;