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

From: 周琰杰 <zhouyanjie@wanyeetech.com>
To: Paul Cercueil <paul@crapouillou.net>
Cc: tsbogend@alpha.franken.de, mturquette@baylibre.com,
	sboyd@kernel.org, robh+dt@kernel.org, linux-mips@vger.kernel.org,
	devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org, dongsheng.qiu@ingenic.com,
	aric.pzqi@ingenic.com, rick.tyliu@ingenic.com,
	sihui.liu@ingenic.com, jun.jiang@ingenic.com,
	sernia.zhou@foxmail.com
Subject: Re: [PATCH v3 4/4] MIPS: CI20: Add second percpu timer for SMP.
Date: Sat, 26 Jun 2021 00:32:51 +0800	[thread overview]
Message-ID: <20210626003251.02312e1e@zhouyanjie-virtual-machine> (raw)
In-Reply-To: <67L9VQ.H1SRDC272GKW@crapouillou.net>

Hi Paul,

于 Fri, 25 Jun 2021 16:47:30 +0100
Paul Cercueil <paul@crapouillou.net> 写道:

> Hi Zhou,
> 
> Le ven., juin 25 2021 at 23:19:42 +0800, 周琰杰 
> <zhouyanjie@wanyeetech.com> a écrit :
> > Hi Paul,
> > 
> > 于 Fri, 25 Jun 2021 12:31:17 +0100
> > Paul Cercueil <paul@crapouillou.net> 写道:
> >   
> >>  Hi Zhou,
> >> 
> >>  Le jeu., juin 24 2021 at 23:06:29 +0800, 周琰杰 (Zhou Yanjie)
> >>  <zhouyanjie@wanyeetech.com> 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@wanyeetech.com>
> >>  > ---
> >>  >
> >>  > 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.

Sure, I will do it in v4.

Thanks and best regards!

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