Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer

[Daniel Lezcano]

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


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