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 <[email protected]>
> ---
> 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 {
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 <[email protected]>
>> ---
>> 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 {