lkml.org 
[lkml]   [2020]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v6 2/2] clocksource: Ingenic: Add support for the Ingenic X1000 OST.
From
Date

在 2020/7/19 上午12:11, Paul Cercueil 写道:
>
>
> 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> 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.
>>>
>>
>> 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
>>>>>
>>>
>

\
 
 \ /
  Last update: 2020-07-18 18:19    [W:0.058 / U:0.664 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site