[PATCH v2 0/6] Device tree fixes for RK3128

[PATCH v2 0/6] Device tree fixes for RK3128

[Alex Bee]

This series contains several device tree fixes which currently prevent
the hardware from actually working.
Please see commit messages of the individual patches for details.

These patches have been part of the pretty huge RK3128 fix-up series [0]
which I've splitted up now, as suggested.

[0] https://lore.kernel.org/linux-rockchip/20230829171647.187787-1-knaerzche@gmail.com

Alex Bee (5):
  ARM: dts: rockchip: Fix i2c0 register address for RK3128
  ARM: dts: rockchip: Add missing arm timer interrupt for RK3128
  ARM: dts: rockchip: Add missing quirk for RK3128's dma engine
  ARM: dts: rockchip: Fix timer clocks for RK3128
  ARM: dts: rockchip: Disable non-required timers for RK3128

 arch/arm/boot/dts/rockchip/rk3128.dtsi | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)


base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
-- 
2.42.0

[PATCH v2 1/5] ARM: dts: rockchip: Fix i2c0 register address for RK3128

[Alex Bee]

The register address for i2c0 is missing a 0x to mark it as hex.

Fixes: a0201bff6259 ("ARM: dts: rockchip: add rk3128 soc dtsi")
Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 arch/arm/boot/dts/rockchip/rk3128.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/rockchip/rk3128.dtsi b/arch/arm/boot/dts/rockchip/rk3128.dtsi
index b63bd4ad3143..2e345097b9bd 100644
--- a/arch/arm/boot/dts/rockchip/rk3128.dtsi
+++ b/arch/arm/boot/dts/rockchip/rk3128.dtsi
@@ -426,7 +426,7 @@ saradc: saradc@2006c000 {
 
 	i2c0: i2c@20072000 {
 		compatible = "rockchip,rk3128-i2c", "rockchip,rk3288-i2c";
-		reg = <20072000 0x1000>;
+		reg = <0x20072000 0x1000>;
 		interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
 		clock-names = "i2c";
 		clocks = <&cru PCLK_I2C0>;
-- 
2.42.0

[PATCH v2 2/5] ARM: dts: rockchip: Add missing arm timer interrupt for RK3128

[Alex Bee]

The Cortex-A7 timer has 4 interrupts.
Add the missing one.

Fixes: a0201bff6259 ("ARM: dts: rockchip: add rk3128 soc dtsi")
Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 arch/arm/boot/dts/rockchip/rk3128.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/rockchip/rk3128.dtsi b/arch/arm/boot/dts/rockchip/rk3128.dtsi
index 2e345097b9bd..bf55d4575311 100644
--- a/arch/arm/boot/dts/rockchip/rk3128.dtsi
+++ b/arch/arm/boot/dts/rockchip/rk3128.dtsi
@@ -64,7 +64,8 @@ timer {
 		compatible = "arm,armv7-timer";
 		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
 			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
-			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
+			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
+			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
 		arm,cpu-registers-not-fw-configured;
 		clock-frequency = <24000000>;
 	};
-- 
2.42.0

[PATCH v2 3/5] ARM: dts: rockchip: Add missing quirk for RK3128's dma engine

[Alex Bee]

Like most other Rockchip ARM SoCs, the PL330 needs the
arm,pl330-periph-burst quirk in order to work as expected.
Add it.

Fixes: a0201bff6259 ("ARM: dts: rockchip: add rk3128 soc dtsi")
Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 arch/arm/boot/dts/rockchip/rk3128.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/rockchip/rk3128.dtsi b/arch/arm/boot/dts/rockchip/rk3128.dtsi
index bf55d4575311..9125bf22e971 100644
--- a/arch/arm/boot/dts/rockchip/rk3128.dtsi
+++ b/arch/arm/boot/dts/rockchip/rk3128.dtsi
@@ -459,6 +459,7 @@ pdma: dma-controller@20078000 {
 		interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
 			     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
 		arm,pl330-broken-no-flushp;
+		arm,pl330-periph-burst;
 		clocks = <&cru ACLK_DMAC>;
 		clock-names = "apb_pclk";
 		#dma-cells = <1>;
-- 
2.42.0

[PATCH v2 4/5] ARM: dts: rockchip: Fix timer clocks for RK3128

[Alex Bee]

Currently the Rockchip timer source clocks are set to xin24 for no obvious
reason and the actual timer clocks (SCLK_TIMER*) will get disabled during
boot process as the have no user. That will make the SoC stuck as no timer
source exists.

Fixes: a0201bff6259 ("ARM: dts: rockchip: add rk3128 soc dtsi")
Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 arch/arm/boot/dts/rockchip/rk3128.dtsi | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/rockchip/rk3128.dtsi b/arch/arm/boot/dts/rockchip/rk3128.dtsi
index 9125bf22e971..88a4b0d6d928 100644
--- a/arch/arm/boot/dts/rockchip/rk3128.dtsi
+++ b/arch/arm/boot/dts/rockchip/rk3128.dtsi
@@ -234,7 +234,7 @@ timer0: timer@20044000 {
 		compatible = "rockchip,rk3128-timer", "rockchip,rk3288-timer";
 		reg = <0x20044000 0x20>;
 		interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;
-		clocks = <&cru PCLK_TIMER>, <&xin24m>;
+		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER0>;
 		clock-names = "pclk", "timer";
 	};
 
@@ -242,7 +242,7 @@ timer1: timer@20044020 {
 		compatible = "rockchip,rk3128-timer", "rockchip,rk3288-timer";
 		reg = <0x20044020 0x20>;
 		interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
-		clocks = <&cru PCLK_TIMER>, <&xin24m>;
+		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER1>;
 		clock-names = "pclk", "timer";
 	};
 
@@ -250,7 +250,7 @@ timer2: timer@20044040 {
 		compatible = "rockchip,rk3128-timer", "rockchip,rk3288-timer";
 		reg = <0x20044040 0x20>;
 		interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
-		clocks = <&cru PCLK_TIMER>, <&xin24m>;
+		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER2>;
 		clock-names = "pclk", "timer";
 	};
 
@@ -258,7 +258,7 @@ timer3: timer@20044060 {
 		compatible = "rockchip,rk3128-timer", "rockchip,rk3288-timer";
 		reg = <0x20044060 0x20>;
 		interrupts = <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
-		clocks = <&cru PCLK_TIMER>, <&xin24m>;
+		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER3>;
 		clock-names = "pclk", "timer";
 	};
 
@@ -266,7 +266,7 @@ timer4: timer@20044080 {
 		compatible = "rockchip,rk3128-timer", "rockchip,rk3288-timer";
 		reg = <0x20044080 0x20>;
 		interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
-		clocks = <&cru PCLK_TIMER>, <&xin24m>;
+		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER4>;
 		clock-names = "pclk", "timer";
 	};
 
@@ -274,7 +274,7 @@ timer5: timer@200440a0 {
 		compatible = "rockchip,rk3128-timer", "rockchip,rk3288-timer";
 		reg = <0x200440a0 0x20>;
 		interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
-		clocks = <&cru PCLK_TIMER>, <&xin24m>;
+		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER5>;
 		clock-names = "pclk", "timer";
 	};
 
-- 
2.42.0

[PATCH v2 5/5] ARM: dts: rockchip: Disable non-required timers for RK3128

[Alex Bee]

The Rockchip timer linux driver can handle a maximum of 2 timers and will
get confused if more of them exist.
RK3128 only needs timer0, timer1 and timer5. The latter is the source
for the arm-timer and it's clock is prevented from being disabled in the
clock driver so it can get disabled in the device tree, too.

Fixes: a0201bff6259 ("ARM: dts: rockchip: add rk3128 soc dtsi")
Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 arch/arm/boot/dts/rockchip/rk3128.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/rockchip/rk3128.dtsi b/arch/arm/boot/dts/rockchip/rk3128.dtsi
index 88a4b0d6d928..f3f0788195d2 100644
--- a/arch/arm/boot/dts/rockchip/rk3128.dtsi
+++ b/arch/arm/boot/dts/rockchip/rk3128.dtsi
@@ -252,6 +252,7 @@ timer2: timer@20044040 {
 		interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER2>;
 		clock-names = "pclk", "timer";
+		status = "disabled";
 	};
 
 	timer3: timer@20044060 {
@@ -260,6 +261,7 @@ timer3: timer@20044060 {
 		interrupts = <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER3>;
 		clock-names = "pclk", "timer";
+		status = "disabled";
 	};
 
 	timer4: timer@20044080 {
@@ -268,6 +270,7 @@ timer4: timer@20044080 {
 		interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER4>;
 		clock-names = "pclk", "timer";
+		status = "disabled";
 	};
 
 	timer5: timer@200440a0 {
@@ -276,6 +279,7 @@ timer5: timer@200440a0 {
 		interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER5>;
 		clock-names = "pclk", "timer";
+		status = "disabled";
 	};
 
 	watchdog: watchdog@2004c000 {
-- 
2.42.0

Re: [PATCH v2 5/5] ARM: dts: rockchip: Disable non-required timers for RK3128

[Robin Murphy]

On 2023-08-29 21:37, Alex Bee wrote:
> The Rockchip timer linux driver can handle a maximum of 2 timers and will
> get confused if more of them exist.

Wouldn't it be better to fix that? It looks trivial to do, and frankly 
it's a behaviour that doesn't make sense anyway. Of course a system can 
have more hardware available than Linux wants to use; that's not an 
error, it's just Linux's choice to not use it! See commit a98399cbc1e0 
("clocksource/drivers/sp804: Avoid error on multiple instances") for 
example.

DTs shouldn't be treated like Linux board files, so curating them around 
Linux-specific driver behaviour is inappropriate; FreeBSD or U-Boot or 
whatever are perfectly entitled to make use of 5 timers at once if they can.

Thanks,
Robin.

> RK3128 only needs timer0, timer1 and timer5. The latter is the source
> for the arm-timer and it's clock is prevented from being disabled in the
> clock driver so it can get disabled in the device tree, too.
> 
> Fixes: a0201bff6259 ("ARM: dts: rockchip: add rk3128 soc dtsi")
> Signed-off-by: Alex Bee <knaerzche@gmail.com>
> ---
>   arch/arm/boot/dts/rockchip/rk3128.dtsi | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/rockchip/rk3128.dtsi b/arch/arm/boot/dts/rockchip/rk3128.dtsi
> index 88a4b0d6d928..f3f0788195d2 100644
> --- a/arch/arm/boot/dts/rockchip/rk3128.dtsi
> +++ b/arch/arm/boot/dts/rockchip/rk3128.dtsi
> @@ -252,6 +252,7 @@ timer2: timer@20044040 {
>   		interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
>   		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER2>;
>   		clock-names = "pclk", "timer";
> +		status = "disabled";
>   	};
>   
>   	timer3: timer@20044060 {
> @@ -260,6 +261,7 @@ timer3: timer@20044060 {
>   		interrupts = <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
>   		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER3>;
>   		clock-names = "pclk", "timer";
> +		status = "disabled";
>   	};
>   
>   	timer4: timer@20044080 {
> @@ -268,6 +270,7 @@ timer4: timer@20044080 {
>   		interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
>   		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER4>;
>   		clock-names = "pclk", "timer";
> +		status = "disabled";
>   	};
>   
>   	timer5: timer@200440a0 {
> @@ -276,6 +279,7 @@ timer5: timer@200440a0 {
>   		interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
>   		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER5>;
>   		clock-names = "pclk", "timer";
> +		status = "disabled";
>   	};
>   
>   	watchdog: watchdog@2004c000 {

Re: [PATCH v2 5/5] ARM: dts: rockchip: Disable non-required timers for RK3128

[Alex Bee]

Hi Robin, Hi Heiko,

Am 30.08.23 um 20:17 schrieb Robin Murphy:
> On 2023-08-29 21:37, Alex Bee wrote:
>> The Rockchip timer linux driver can handle a maximum of 2 timers and 
>> will
>> get confused if more of them exist.
>
> Wouldn't it be better to fix that? It looks trivial to do, and frankly 
> it's a behaviour that doesn't make sense anyway. Of course a system 
> can have more hardware available than Linux wants to use; that's not 
> an error, it's just Linux's choice to not use it! See commit 
> a98399cbc1e0 ("clocksource/drivers/sp804: Avoid error on multiple 
> instances") for example.
>
> DTs shouldn't be treated like Linux board files, so curating them 
> around Linux-specific driver behaviour is inappropriate; FreeBSD or 
> U-Boot or whatever are perfectly entitled to make use of 5 timers at 
> once if they can.

That's fully true, thanks for the hint.

The common Rockchip workaround currently seems to be to just expose the 
timer(s) in the DT which can be handled by the linux driver.  RK3288, 
for instance, has 7 timers but there's a single one present in the SoC 
DT ... and another one is enabled in the common RK mach-code, .... ups

Anyway: I'll have a look in the RK timer driver and try to fix it. 
Though, I'm not sure if just ignoring the others like sp804 driver does 
is sufficient, as we still will have to add workarounds to the clock 
driver in order to keep the clocks enabled for those timers which are 
not used by linux, but are required (as source for the arm timer, for 
instance).

The dw_apb_timer driver seems to register the second timer as 
clocksource- and all others as clockevent-timers .... that looks like 
the "better" approach around that issue.

Regards,

Alex

>
> Thanks,
> Robin.
>
>> RK3128 only needs timer0, timer1 and timer5. The latter is the source
>> for the arm-timer and it's clock is prevented from being disabled in the
>> clock driver so it can get disabled in the device tree, too.
>>
>> Fixes: a0201bff6259 ("ARM: dts: rockchip: add rk3128 soc dtsi")
>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>> ---
>>   arch/arm/boot/dts/rockchip/rk3128.dtsi | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/rockchip/rk3128.dtsi 
>> b/arch/arm/boot/dts/rockchip/rk3128.dtsi
>> index 88a4b0d6d928..f3f0788195d2 100644
>> --- a/arch/arm/boot/dts/rockchip/rk3128.dtsi
>> +++ b/arch/arm/boot/dts/rockchip/rk3128.dtsi
>> @@ -252,6 +252,7 @@ timer2: timer@20044040 {
>>           interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
>>           clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER2>;
>>           clock-names = "pclk", "timer";
>> +        status = "disabled";
>>       };
>>         timer3: timer@20044060 {
>> @@ -260,6 +261,7 @@ timer3: timer@20044060 {
>>           interrupts = <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
>>           clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER3>;
>>           clock-names = "pclk", "timer";
>> +        status = "disabled";
>>       };
>>         timer4: timer@20044080 {
>> @@ -268,6 +270,7 @@ timer4: timer@20044080 {
>>           interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
>>           clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER4>;
>>           clock-names = "pclk", "timer";
>> +        status = "disabled";
>>       };
>>         timer5: timer@200440a0 {
>> @@ -276,6 +279,7 @@ timer5: timer@200440a0 {
>>           interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
>>           clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER5>;
>>           clock-names = "pclk", "timer";
>> +        status = "disabled";
>>       };
>>         watchdog: watchdog@2004c000 {

Re: (subset) [PATCH v2 0/6] Device tree fixes for RK3128

[Heiko Stuebner]

On Tue, 29 Aug 2023 22:37:19 +0200, Alex Bee wrote:
> This series contains several device tree fixes which currently prevent
> the hardware from actually working.
> Please see commit messages of the individual patches for details.
> 
> These patches have been part of the pretty huge RK3128 fix-up series [0]
> which I've splitted up now, as suggested.
> 
> [...]

Applied, thanks!

[1/5] ARM: dts: rockchip: Fix i2c0 register address for RK3128
      commit: 2e9cbc4167da3134412ce47e4cdadbfdea30bbff
[2/5] ARM: dts: rockchip: Add missing arm timer interrupt for RK3128
      commit: 7e3be9ea299927e6d65242c247eca0a21bc26a58
[3/5] ARM: dts: rockchip: Add missing quirk for RK3128's dma engine
      commit: b0b4e978784943c4ed8412dbb475178f8c51ba8e
[4/5] ARM: dts: rockchip: Fix timer clocks for RK3128
      commit: 2c68d26f072b449bd45427241612cb3f8f997f82

As Robin mentioned, DT describes hardware not Linux's
specificalities, so I ignored patch6.


Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>