Re: [PATCH v6 2/2] clocksource: Ingenic: Add support for the Ingenic X1000 OST.

[Zhou Yanjie]

Hi Paul,

å 2020/7/18 äå11:44, Paul Cercueil åé:
> Hi Zhou,
>
> Le sam. 18 juil. 2020 Ã 21:42, Zhou Yanjie <zhouyanjie@xxxxxxxxxxxxxx> 
> a Ãcrit :
> > Hello Paul and Daniel,
> >
> > å 2020/7/18 äå9:12, Paul Cercueil åé:
> > > Hi Daniel,
> > >
> > > Le ven. 17 juil. 2020 Ã 10:02, Daniel Lezcano 
> > > <daniel.lezcano@xxxxxxxxxx> a Ãcrit :
> > > > On 17/07/2020 08:13, Zhou Yanjie wrote:
> > > > > ÂHi Daniel,
> > > > >
> > > > > Âå 2020/7/17 äå12:20, Daniel Lezcano åé:
> > > > > > ÂOn 10/07/2020 19:02, åçæ (Zhou Yanjie) wrote:
> > > > > > > ÂX1000 and SoCs after X1000 (such as X1500 and X1830) had a 
> > > > > > > separate
> > > > > > > ÂOST, it no longer belongs to TCU. This driver will register both a
> > > > > > > Âclocksource and a sched_clock to the system.
> > > > > > >
> > > > > > > ÂTested-by: åæ (Zhou Zheng) <sernia.zhou@xxxxxxxxxxx>
> > > > > > > ÂCo-developed-by: æéæ (Qi Pengzhen) <aric.pzqi@xxxxxxxxxxx>
> > > > > > > ÂSigned-off-by: æéæ (Qi Pengzhen) <aric.pzqi@xxxxxxxxxxx>
> > > > > > > ÂSigned-off-by: åçæ (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
> > > > > > > ÂReviewed-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > > > > > > Â---
> > > > > > >
> > > > > > > ÂNotes:
> > > > > > > ÂÂÂÂÂ v1->v2:
> > > > > > > ÂÂÂÂÂ Fix compile warnings.
> > > > > > > ÂÂÂÂÂ Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > > > > > > ÂÂÂÂÂÂÂÂÂÂ v2->v3:
> > > > > > > ÂÂÂÂÂ No change.
> > > > > > > ÂÂÂÂÂÂÂÂÂÂ v3->v4:
> > > > > > > ÂÂÂÂÂ 1.Rename "ost" to "sysost"
> > > > > > > ÂÂÂÂÂ 1.Remove unrelated changes.
> > > > > > > ÂÂÂÂÂ 2.Remove ost_clock_parent enum.
> > > > > > > ÂÂÂÂÂ 3.Remove ost->percpu_timer_channel/ost->global_timer_channel.
> > > > > > > ÂÂÂÂÂ 4.Set up independent .recalc_rate/.set_rate for percpu/global
> > > > > > > Âtimer.
> > > > > > > ÂÂÂÂÂ 5.No longer call functions in variable declarations.
> > > > > > > ÂÂÂÂÂÂÂÂÂÂ v4->v5:
> > > > > > > ÂÂÂÂÂ Use "of_io_request_and_map()" instead "of_iomap()".
> > > > > > > ÂÂÂÂÂ Suggested-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > > > > > > ÂÂÂÂÂÂÂÂÂÂ v5->v6:
> > > > > > > ÂÂÂÂÂ No change.
> > > > > > >
> > > > > > > ÂÂ drivers/clocksource/KconfigÂÂÂÂÂÂÂÂÂ |Â 11 +
> > > > > > > ÂÂ drivers/clocksource/MakefileÂÂÂÂÂÂÂÂ |ÂÂ 1 +
> > > > > > > ÂÂ drivers/clocksource/ingenic-sysost.c | 539
> > > > > > > Â+++++++++++++++++++++++++++++++++++
> > > > > > > ÂÂ 3 files changed, 551 insertions(+)
> > > > > > > ÂÂ create mode 100644 drivers/clocksource/ingenic-sysost.c
> > > > > > >
> > > > > > > Âdiff --git a/drivers/clocksource/Kconfig 
> > > > > > > b/drivers/clocksource/Kconfig
> > > > > > > Âindex 91418381fcd4..1bca8b8fb30f 100644
> > > > > > > Â--- a/drivers/clocksource/Kconfig
> > > > > > > Â+++ b/drivers/clocksource/Kconfig
> > > > > > > Â@@ -696,6 +696,17 @@ config INGENIC_TIMER
> > > > > > > ÂÂÂÂÂÂ help
> > > > > > > ÂÂÂÂÂÂÂÂ Support for the timer/counter unit of the Ingenic JZ SoCs.
> > > > > > > ÂÂ +config INGENIC_SYSOST
> > > > > > > Â+ÂÂÂ bool "Clocksource/timer using the SYSOST in Ingenic X SoCs"
> > > > > > ÂWe usually use silent options and let the platform's Kconfig 
> > > > > > enable it.
> > > > > > ÂWe show up the option only when COMPILE_TEST is enabled.
> > > > > >
> > > > > > ÂIs there a reason to do it differently?
> > > > >
> > > > >
> > > > > ÂDo you mean
> > > > >
> > > > > Âbool "Clocksource/timer using the SYSOST in Ingenic X SoCs"
> > > > >
> > > > > Âor
> > > > >
> > > > > Âdefault MACH_INGENIC ?
> > > >
> > > > Both, no default here.
> > > >
> > > > eg.
> > > >
> > > > bool "Clocksource/timer using the SYSOST in Ingenic X SoCs" if 
> > > > COMPILE_TEST
> > > >
> > > > and
> > > >
> > > > in arch/mips/Kconfig in the config MACH_INGENIC section :
> > > >
> > > > ...
> > > > select INGENIC_SYSOST
> > > > ...
> > >
> > > Disagreed. That's not how we do things on MIPS. Selecting 
> > > MACH_INGENIC means "this kernel will support Ingenic SoCs", but not 
> > > that it will only support these. Hence the depends on MIPS / 
> > > default MACH_INGENIC.
> > >
> > > As for the select INGENIC_SYSOST, this driver only applies to a few 
> > > SoCs, I certainly don't want it to be force-enabled. I don't even 
> > > wait it to be force-enabled on X1000, since it is optional there too.
> > >
> > > Cheers,
> > > -Paul
> >
> >
> > If we still need to keep the "default MACH_INGENIC", then Daniel can 
> > directly apply the v6 version.
> >
> > If we need to use the silent options, maybe we can enable them 
> > separately according to 
> > MACH_JZ4740/MACH_JZ4770/MACH_JZ4780/MACH_X1000/MACH_X1830.
> >
> > In fact, I think X1000 and X1830 need to enable this driver in most 
> > cases, because the current test has found that use TCU to provide 
> > clocksource and clockevent will cause data loss/error when 
> > transmitting data through spi or ethernet. And these errors no longer 
> > appear after using OST.
>
> This is likely because the clock is too fast, try to reduce it by a 
> factor 2 or 4, it should behave better.

Yes, it is indeed because the clock is too fast, especially the TCU only 
has 16-bit, which makes this problem more serious. I even reduced the 
timing clock to the lowest possible 23kHz (divided by 1024). The problem 
has been alleviated, but it cannot completely solved, and finally there 
is no problem after opening the OST.


> If it turns out OST is really needed, then it should be selected from 
> MACH_X1000/X1830, not MACH_INGENIC.

Sure, I will do it in v8.

Thanks and best regards!


> Cheers,
> -Paul
>
> > Thanks and best regards!
> >
> >
> > > > > ÂThis driver has some origins from "INGENIC_TIMER" driver and
> > > > > Â"INGENIC_OST" driver.
> > > > > ÂEarly Ingenic processors used TCU (timer/counter unit, has 6 or 8
> > > > > Âgeneric timer channels) to provide clocksource and clockevent 
> > > > > (both with
> > > > > Âonly 16bit precision). This part of the processor can only use
> > > > > Â"INGENIC_TIMER" driver.
> > > > >
> > > > > ÂLater processors provide an independent 32bit or 64bit timer channel
> > > > > Â(still under TCU, known as ost channel, this channel can not 
> > > > > generate
> > > > > Âinterrupt) to provid higher precision clocksource. The "INGENIC_OST"
> > > > > Âdriver is for this channel. These processors can use "INGENIC_TIMER"
> > > > > Âdriver, but using "INGENIC_OST" driver to provide higher precision
> > > > > Âclocksource would be a better choice (clockevent still needs to be
> > > > > Âprovided by generic timer channel of TCU, and still 16bit 
> > > > > precision).
> > > > >
> > > > > ÂAnd the recent processors provide a SYSOST components, it is 
> > > > > independent
> > > > > Âfrom TCU, including a 64bit timer channel for clocksource and a 
> > > > > 32bit
> > > > > Âtimer channel for clockevent. Although these processors can also use
> > > > > Â"INGENIC_TIMER" driver, but the better choice is completely 
> > > > > independent
> > > > > Âuse of "INGENIC_SYSOST" driver to provide higher precision 
> > > > > clocksource
> > > > > Âand clockevent.
> > > >
> > > > Ok, the rating should do the job then.
> > > >
> > > > Thanks for the explanation.
> > > >
> > > > > ÂYou may have already noticed that this independent SYSOST 
> > > > > component is
> > > > > Âlike an upgraded and streamlined TCU, which only retains one generic
> > > > > Âtimer channel that can generate interrupts, upgrade it from 16bit to
> > > > > Â32bit, and then retain the 64bit ost channel. so the driver code and
> > > > > ÂKconfig code of this patch is largely referenced
> > > > > Â"INGENIC_TIMER" driver and "INGENIC_OST" driver.
> > > > >
> > > > > ÂThanks and best regards!
> > > > >
> > > > > > > Â+ÂÂÂ default MACH_INGENIC
> > > > > > > Â+ÂÂÂ depends on MIPS || COMPILE_TEST
> > > > > > > Â+ÂÂÂ depends on COMMON_CLK
> > > > > > > Â+ÂÂÂ select MFD_SYSCON
> > > > > > > Â+ÂÂÂ select TIMER_OF
> > > > > > > Â+ÂÂÂ select IRQ_DOMAIN
> > > > > > > Â+ÂÂÂ help
> > > > > > > Â+ÂÂÂÂÂ Support for the SYSOST of the Ingenic X Series SoCs.
> > > > > > > Â+
> > > > > > Â[ ... ]
> > > >
> > > >
> > > > --
> > > > <http://www.linaro.org/> Linaro.org â Open source software for ARM 
> > > > SoCs
> > > >
> > > > Follow Linaro:Â <http://www.facebook.com/pages/Linaro> Facebook |
> > > > <http://twitter.com/#!/linaroorg> Twitter |
> > > > <http://www.linaro.org/linaro-blog/> Blog