Date:   Sat, 18 Jul 2020 18:11:10 +0200
From:   Paul Cercueil <paul@...pouillou.net>
To:     Zhou Yanjie <zhouyanjie@...yeetech.com>
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.



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).

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