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

[Paul Cercueil <paul@crapouillou.net>]

Le sam. 18 juil. 2020 à 23:55, Zhou Yanjie <zhouyanjie@wanyeetech.com> 
a écrit :
> Hi Paul,
> 
> 在 2020/7/18 下午11:44, Paul Cercueil 写道:
>> Hi Zhou,
>> 
>> Le sam. 18 juil. 2020 à 21:42, Zhou Yanjie 
>> <zhouyanjie@wanyeetech.com> \x7fa écrit :
>>> Hello Paul and Daniel,
>>> 
>>> 在 2020/7/18 下午9:12, Paul Cercueil 写道:
>>>> Hi Daniel,
>>>> 
>>>> Le ven. 17 juil. 2020 à 10:02, Daniel Lezcano 
>>>> \x7f\x7f\x7f\x7f<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 
>>>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7fseparate
>>>>>>>>  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 
>>>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fb/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 
>>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fenable 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 
>>>>> \x7f\x7f\x7f\x7f\x7f\x7fCOMPILE_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 
>>>> \x7f\x7f\x7fMACH_INGENIC \x7fmeans "this kernel will support Ingenic SoCs", 
>>>> but not \x7f\x7f\x7fthat it will \x7fonly support these. Hence the depends on 
>>>> MIPS / \x7f\x7f\x7fdefault MACH_INGENIC.
>>>> 
>>>> As for the select INGENIC_SYSOST, this driver only applies to a 
>>>> few \x7f\x7f\x7f\x7fSoCs, I certainly don't want it to be force-enabled. I 
>>>> don't even \x7f\x7f\x7fwait \x7fit 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 \x7f\x7fdirectly apply the v6 version.
>>> 
>>> If we need to use the silent options, maybe we can enable them 
>>> \x7f\x7fseparately according to 
>>> \x7f\x7fMACH_JZ4740/MACH_JZ4770/MACH_JZ4780/MACH_X1000/MACH_X1830.
>>> 
>>> In fact, I think X1000 and X1830 need to enable this driver in most 
>>> \x7f\x7fcases, because the current test has found that use TCU to provide 
>>> \x7f\x7fclocksource and clockevent will cause data loss/error when 
>>> \x7f\x7ftransmitting data through spi or ethernet. And these errors no 
>>> longer \x7f\x7fappear after using OST.
>> 
>> This is likely because the clock is too fast, try to reduce it by a 
>> \x7ffactor 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 \x7fMACH_X1000/X1830, not MACH_INGENIC.
>> 
> 
> Sure, I will do it in v8.
> 
> Thanks and best regards!

Please do it in a separate patch then (but in the same patchset).

Cheers,
-Paul

> 
>> 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 
>>>>>> \x7f\x7f\x7f\x7f\x7f(both \x7f\x7f\x7fwith
>>>>>>  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 
>>>>>> \x7f\x7f\x7f\x7f\x7fgenerate
>>>>>>  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 
>>>>>> \x7f\x7f\x7f\x7f\x7fprecision).
>>>>>> 
>>>>>>  And the recent processors provide a SYSOST components, it is 
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7findependent
>>>>>>  from TCU, including a 64bit timer channel for clocksource and a 
>>>>>> \x7f\x7f\x7f\x7f\x7f32bit
>>>>>>  timer channel for clockevent. Although these processors can 
>>>>>> also use
>>>>>>  "INGENIC_TIMER" driver, but the better choice is completely 
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7findependent
>>>>>>  use of "INGENIC_SYSOST" driver to provide higher precision 
>>>>>> \x7f\x7f\x7f\x7f\x7fclocksource
>>>>>>  and clockevent.
>>>>> 
>>>>> Ok, the rating should do the job then.
>>>>> 
>>>>> Thanks for the explanation.
>>>>> 
>>>>>>  You may have already noticed that this independent SYSOST 
>>>>>> \x7f\x7f\x7f\x7f\x7fcomponent 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 \x7f\x7f\x7f\x7fSoCs
>>>>> 
>>>>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>>>>> <http://twitter.com/#!/linaroorg> Twitter |
>>>>> <http://www.linaro.org/linaro-blog/> Blog
>>>> 
>>