Date:   Sun, 19 Jul 2020 00:18:18 +0800
From:   Zhou Yanjie <zhouyanjie@...yeetech.com>
To:     Paul Cercueil <paul@...pouillou.net>
Cc:     Daniel Lezcano <daniel.lezcano@...aro.org>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        robh+dt@...nel.org, tglx@...utronix.de, dongsheng.qiu@...enic.com,
        aric.pzqi@...enic.com, rick.tyliu@...enic.com,
        yanfei.li@...enic.com, sernia.zhou@...mail.com,
        zhenwenjin@...il.com
Subject: Re: [PATCH v6 2/2] clocksource: Ingenic: Add support for the Ingenic
 X1000 OST.


在 2020/7/19 上午12:11, Paul Cercueil 写道:
>
>
> Le sam. 18 juil. 2020 à 23:55, Zhou Yanjie <zhouyanjie@...yeetech.com> 
> a écrit :
>> Hi Paul,
>>
>> 在 2020/7/18 下午11:44, Paul Cercueil 写道:
>>> Hi Zhou,
>>>
>>> Le sam. 18 juil. 2020 à 21:42, Zhou Yanjie 
>>> <zhouyanjie@...yeetech.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@...aro.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@...mail.com>
>>>>>>>>>  Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@...enic.com>
>>>>>>>>>  Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@...enic.com>
>>>>>>>>>  Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@...yeetech.com>
>>>>>>>>>  Reviewed-by: Paul Cercueil <paul@...pouillou.net>
>>>>>>>>>  ---
>>>>>>>>>
>>>>>>>>>  Notes:
>>>>>>>>>       v1->v2:
>>>>>>>>>       Fix compile warnings.
>>>>>>>>>       Reported-by: kernel test robot <lkp@...el.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@...pouillou.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.
>>>
>>
>> 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!
>
> Please do it in a separate patch then (but in the same patchset).
>

Okay, I will remove "default MACH_INGENIC" first, and then enable OST later.


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

Powered by blists - more mailing lists