Re: [PATCH v3 4/4] MIPS: CI20: Add second percpu timer for SMP.

[Paul Cercueil]

Hi Zhou,

Le ven., juin 25 2021 at 23:19:42 +0800, 周琰杰 
<zhouyanjie@xxxxxxxxxxxxxx> a écrit :
> Hi Paul,
>
> 于 Fri, 25 Jun 2021 12:31:17 +0100
> Paul Cercueil <paul@xxxxxxxxxxxxxxx> 写道:
>
> >  Hi Zhou,
> >
> >  Le jeu., juin 24 2021 at 23:06:29 +0800, 周琰杰 (Zhou Yanjie)
> >  <zhouyanjie@xxxxxxxxxxxxxx> a écrit :
> >  > 1.Add a new TCU channel as the percpu timer of core1, this is to
> >  >   prepare for the subsequent SMP support. The newly added channel
> >  >   will not adversely affect the current single-core state.
> >  > 2.Adjust the position of TCU node to make it consistent with the
> >  >   order in jz4780.dtsi file.
> >
> >  That's a bit superfluous, the order matters when adding new nodes,
> >  but once they are added, moving them around only cause annoyance.
> >
> >  >
> >  > Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
> >  > ---
> >  >
> >  > Notes:
> >  >     v2:
> >  >     New patch.
> >  >
> >  >     v2->v3:
> >  >     No change.
> >  >
> >  >  arch/mips/boot/dts/ingenic/ci20.dts | 21 +++++++++++----------
> >  >  1 file changed, 11 insertions(+), 10 deletions(-)
> >  >
> >  > diff --git a/arch/mips/boot/dts/ingenic/ci20.dts
> >  > b/arch/mips/boot/dts/ingenic/ci20.dts
> >  > index 8877c62..70005cc 100644
> >  > --- a/arch/mips/boot/dts/ingenic/ci20.dts
> >  > +++ b/arch/mips/boot/dts/ingenic/ci20.dts
> >  > @@ -118,6 +118,17 @@
> >  >  	assigned-clock-rates = <48000000>;
> >  >  };
> >  >
> >  > +&tcu {
> >  > +	/*
> >  > +	 * 750 kHz for the system timers and 3 MHz for the
> >  > clocksources,
> >  > +	 * use channel #0 and #1 for the per cpu system timers,
> >  > and use
> >  > +	 * channel #2 for the clocksource.
> >  > +	 */
> >  > +	assigned-clocks = <&tcu TCU_CLK_TIMER0>, <&tcu
> >  > TCU_CLK_TIMER1>,
> >  > +					  <&tcu TCU_CLK_TIMER2>,
> >  > <&tcu TCU_CLK_OST>;
> >  > +	assigned-clock-rates = <750000>, <750000>, <3000000>,
> >  > <3000000>;
> >
> >  Ideally you'd set TIMER1 to 3 MHz and TIMER2 to 750 kHz, otherwise 
> > it
> >  kind of breaks support for older kernels (they would still boot, but
> >  with a very slow clocksource). So in the new DTS you could use the
> >  timer0 clock for CPU #0, timer1 for the clocksource, and timer2+ for
> >  cpus > 0.
>
> I checked the ingenic-timer driver, and it seems that the last TCU
> channel is always used as the clocksource in the driver, so it seems
> that we can only use timer2 as the clocksource in smp mode. Maybe we
> should add a note for smp is closed in the comment. And I found that I
> missed a problem, Nikolaus Schaller once reported that because the
> frequency of the tcu timer (only 16bit) used to provide the 
> clocksource
> is too high, there will be a chance that the system will get stuck
> before the clocksource is switched to ost. And reducing the 
> clocksource
> to 750kz can prevent it from happening. I will add this part to v4.
> When this part is added, both clockevent and clocksource will be
> 750kHz, but the 750kHz clocksource is only temporary, because it will
> then switch to the clocksource provided by ost, and ost works at 3MHz.

Ok, then first change the clocksource to 750 kHz, then update it with 
timer2.

Cheers,
-Paul

> Thanks and best regards!
>
> >  Cheers,
> >  -Paul
> >
> >  > +};
> >  > +
> >  >  &mmc0 {
> >  >  	status = "okay";
> >  >
> >  > @@ -522,13 +533,3 @@
> >  >  		bias-disable;
> >  >  	};
> >  >  };
> >  > -
> >  > -&tcu {
> >  > -	/*
> >  > -	 * 750 kHz for the system timer and 3 MHz for the
> >  > clocksource,
> >  > -	 * use channel #0 for the system timer, #1 for the
> >  > clocksource.
> >  > -	 */
> >  > -	assigned-clocks = <&tcu TCU_CLK_TIMER0>, <&tcu
> >  > TCU_CLK_TIMER1>,
> >  > -					  <&tcu TCU_CLK_OST>;
> >  > -	assigned-clock-rates = <750000>, <3000000>, <3000000>;
> >  > -};
> >  > --
> >  > 2.7.4
> >  >