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

[Greentime Hu]

Hi, Daniel:

2018-01-05 3:48 GMT+08:00 Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>:
> On 04/01/2018 15:06, Greentime Hu wrote:
>> Hi, Daniel:
>>
>> 2018-01-04 21:50 GMT+08:00 Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>:
>>>
>>> Hi,
>>>
>>> sorry I missed your answer. Comments below.
>>>
>>> On 13/12/2017 07:06, Greentime Hu wrote:
>>>> Hi, Daniel:
>>>>
>>>> 2017-12-12 18:05 GMT+08:00 Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>:
>>>>> On 12/12/2017 06:46, Rick Chen wrote:
>>>>>> ATCPIT100 is often used on the Andes architecture,
>>>>>> This timer provide 4 PIT channels. Each PIT channel is a
>>>>>> multi-function timer, can be configured as 32,16,8 bit timers
>>>>>> or PWM as well.
>>>>>>
>>>>>> For system timer it will set channel 1 32-bit timer0 as clock
>>>>>> source and count downwards until underflow and restart again.
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>> +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?

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

>>>> 2. It seems conflict with debug info. I am not sure if there is
>>>> another way to debug kernel(with debug info) with COMPILE_TEST and
>>>> DEBUG_INFO because we need this driver for nds32 architecture.
>>>>
>>>> Symbol: DEBUG_INFO [=n]
>>>> Type  : boolean
>>>> Prompt: Compile the kernel with debug info
>>>>   Location:
>>>>     -> Kernel hacking
>>>>       -> Compile-time checks and compiler options
>>>>   Defined at lib/Kconfig.debug:140
>>>>   Depends on: DEBUG_KERNEL [=y] && !COMPILE_TEST [=n]
>>>
>>> The COMPILE_TEST option is only there to allow cross-compilation test
>>> coverage, it does not select or unselect the driver in usual way.
>>>
>>> If the COMPILE_TEST is enabled, then the option will appear in the
>>> menuconfig, so that gives the opportunity to select/unselect it.
>>>
>>> Otherwise, the Kconfig's platform selects automatically the driver and
>>> the user *can't* unselect it from the menuconfig as it is a silent
>>> option and that is certainly what you want.
>>>
>>>>> Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100
>>>>> to TIMER_ATCPIT100.
>>>>
>>>> Thanks. We will rename it in the next version patch.
>>>
>>> You just resend an entire series V5 for the architecture. I'm confused,
>>> what is the merging path ?
>>
>> Sorry. I didn't get your point.
>> We sent the timer patch and the architecture patch together because it
>> would be easier for reviewer to check the vdso implementations.
>> What do you mean about the merging path?
>
> I received a [Vx y/3] series and I received a [Vx y/39].
>
> The former from Rick Chen means to me "please pick them through your tree".
>
> The latter from you means to me "can you ack the patches so I can merge
> them through my tree". Note you will have to resend the entire arch
> series for every single review/comment (that could end up upset the
> Cc'ed people).
>
> Which one should I review ? I can not track different patchset
> implementing the same thing. Which one should I comment, review ? Are
> the comments I did on [Vx y/3] taken into account in the arch series ?
> etc ...
>
> Please clarify, it is confusing and impossible to review in this situation.
>
> I suggest we stick to the x/3 series, so I can comment it and you can
> resend a new version without resending the entire arch series. So I can
> merge it through my tree, and you get it via eg. a shared immutable
> branch. The arch series will be reduced by 3 patches.

Thanks for your explanation. :)
I will send these 2 patch set separately next time.