From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Lezcano Subject: Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer Date: Fri, 5 Jan 2018 10:31:32 +0100 Message-ID: <1e75edb3-8f7b-796c-6871-1612b027050e@linaro.org> References: <1513057621-19084-1-git-send-email-rickchen36@gmail.com> <1513057621-19084-2-git-send-email-rickchen36@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org To: Greentime Hu Cc: Greentime , Rick Chen , Rick Chen , Linux Kernel Mailing List , Arnd Bergmann , Linus Walleij , linux-arch , Thomas Gleixner , Jason Cooper , Marc Zyngier , Rob Herring , netdev , Vincent Chen , DTML , Al Viro , David Howells , Will Deacon , linux-serial@vger.kernel.org List-Id: devicetree@vger.kernel.org On 05/01/2018 09:45, Greentime Hu wrote: > Hi, Daniel: [ ... ] >>>>>> [ ... ] >>>>>> >>>>>>> +config CLKSRC_ATCPIT100 >>>>>>> + bool "Clocksource for AE3XX platform" >>>>>>> + depends on NDS32 || COMPILE_TEST >>>>>>> + depends on HAS_IOMEM >>>>>>> + help >>>>>>> + This option enables support for the Andestech AE3XX platform timers. >>>>>> >>>>>> Hi Rick, >>>>>> >>>>>> the general rule for the Kconfig is: >>>>>> >>>>>> bool "Clocksource for AE3XX platform" if COMPILE_TEST >> >> BTW, select TIMER_OF is missing. > > We don't select here because we select TIMER_OF in arch/nds32/Kconfig > I am not sure if I still need to select TIMER_OF here? Actually, I want the drivers/clocksource/Kconfig to be consistent across all entries. As TIMER_OF is needed by the driver and nothing else, it must be selected in the TIMER entry. As there are a lot of timers and we do the changes little by little, there are still entries with different format. It should be something like that: config ASM9260_TIMER bool "ASM9260 timer driver" if COMPILE_TEST select CLKSRC_MMIO select TIMER_OF help Enables support for the ASM9260 timer. Move the select TIMER_OF to the timer option entry. >>>>>> and no deps on the platform. >>>>>> >>>>>> It is up to the platform Kconfig to select the option. >>>>>> >>>>>> We want here a silent option but make it selectable in case of >>>>>> compilation test coverage. >>>>> >>>>> >>>>> The way we like to use it is because >>>>> 1. This timer is a basic component to boot an nds32 CPU and it should >>>>> be able to select without COMPILE_TEST for nds32 architecture. >>>> >>>> Yes, so you don't need it to be selectable, you must select it from the >>>> platform's Kconfig. >>> >>> I am not sure that I get your point or not. >>> We don't have a CONFIG_PLAT_AE3XX. >>> Do you mean we should create one and select CLKSRC_ATCPIT100 under >>> CONFIG_PLAT_AE3XX? >> >> No. Can't you add in arch/ndis32/Kconfig ? >> >> +select TIMER_ATCPIT100 >> >> Like: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/Kconfig#n50 > > IMHO, it might be a little bit wierd if we select TIMER_ATCPIT100 in > arch/nds32/Kconfig because it is part of SoC instead of CPU. > If we change to another SoC with another timer, we need to select > another TIMER in arch/nds32/Kconfig and delete TIMER_ATCPIT100. > It seems more flexible to be selected in driver layer. > > It seems to be the timer is part of the arch to be selected in arch's Kconfig. > arch/arc/Kconfig: select ARC_TIMERS > arch/arc/Kconfig: select ARC_TIMERS_64BIT > arch/arm/Kconfig: select ARM_ARCH_TIMER > arch/arm64/Kconfig: select ARM_ARCH_TIMER > arch/blackfin/Kconfig: select BFIN_GPTIMERS No, the timer must be selected from the arch/soc's or whatever Kconfig. Not in the clocksource's Kconfig. eg. on ARM: arch/arm/mach-vt8500/Kconfig: select VT8500_TIMER arch/arm/mach-bcm/Kconfig: select BCM_KONA_TIMER arch/arm/mach-actions/Kconfig: select OWL_TIMER arch/arm/mach-digicolor/Kconfig: select DIGICOLOR_TIMER etc ... on ARM64: arch/arm64/Kconfig.platforms: select OWL_TIMER arch/arm64/Kconfig.platforms: select ARM_TIMER_SP804 arch/arm64/Kconfig.platforms: select MTK_TIMER etc ... Thanks. -- Daniel -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog