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

    >
    >> 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 15:13    [W:4.104 / U:0.772 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site