Messages in this thread Patch in this message | | | Date | Sat, 18 Jul 2020 17:44:52 +0200 | From | Paul Cercueil <> | Subject | Re: [PATCH v6 2/2] clocksource: Ingenic: Add support for the Ingenic X1000 OST. |
| |
Hi Zhou,
Le sam. 18 juil. 2020 à 21:42, Zhou Yanjie <zhouyanjie@wanyeetech.com> 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@linaro.org> 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@foxmail.com> >>>>>> Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com> >>>>>> Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com> >>>>>> Signed-off-by: 周琰杰 (Zhou Yanjie) >>>>>> <zhouyanjie@wanyeetech.com> >>>>>> Reviewed-by: Paul Cercueil <paul@crapouillou.net> >>>>>> --- >>>>>> >>>>>> Notes: >>>>>> v1->v2: >>>>>> Fix compile warnings. >>>>>> Reported-by: kernel test robot <lkp@intel.com> >>>>>> 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@crapouillou.net> >>>>>> 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.
If it turns out OST is really needed, then it should be selected from MACH_X1000/X1830, not MACH_INGENIC.
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 >>
| |