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

From: Zhou Yanjie
Date: Sat Jul 18 2020 - 09:42:40 EST


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.

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