Re: [PATCH v16 04/15] clocksource/drivers/arm_arch_timer: rename some enums and defines, and some cleanups.
Hi Mark,
On 19 November 2016 at 02:49, Mark Rutland wrote:
> On Wed, Nov 16, 2016 at 09:48:57PM +0800, fu@linaro.org wrote:
>> From: Fu Wei
>>
>> Rename some enums and defines, to unify the format of enums and defines
>> in arm_arch_timer.h, also update all the users of these enums and defines:
>> drivers/clocksource/arm_arch_timer.c
>> virt/kvm/arm/hyp/timer-sr.c
>
> I'm happy with making definitions use a consistent ARCH_TIMER_ prefix,
> given they're exposed in headers...
>
>> And do some cleanups, according to the suggestion from checkpatch.pl:
>> (1) using BIT(nr) instead of (1 << nr)
>> (2) using 'unsigned int' instead of 'unsigned'
>
> ... but these changes are pointless churn. They make the patch larger,
> hardwer to review, and more painful to merge.
>
> Please leave these as they are unless there is a functional problem. If
> there will be a functional problem unless these are changed, describe
> that in the commit message.
OK, Mark.
I will take these out of patch, thanks :-)
>
> Thanks,
> Mark.
>
>>
>> No functional change.
>>
>> Signed-off-by: Fu Wei
>> ---
>> drivers/clocksource/arm_arch_timer.c | 111
>> ++-
>> include/clocksource/arm_arch_timer.h | 40 ++---
>> virt/kvm/arm/hyp/timer-sr.c | 6 +-
>> 3 files changed, 81 insertions(+), 76 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c
>> b/drivers/clocksource/arm_arch_timer.c
>> index 15341cf..dd1040d 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -66,11 +66,11 @@ struct arch_timer {
>> #define to_arch_timer(e) container_of(e, struct arch_timer, evt)
>>
>> static u32 arch_timer_rate;
>> -static int arch_timer_ppi[MAX_TIMER_PPI];
>> +static int arch_timer_ppi[ARCH_TIMER_MAX_TIMER_PPI];
>>
>> static struct clock_event_device __percpu *arch_timer_evt;
>>
>> -static enum arch_timer_ppi_nr arch_timer_uses_ppi = VIRT_PPI;
>> +static enum arch_timer_ppi_nr arch_timer_uses_ppi = ARCH_TIMER_VIRT_PPI;
>> static bool arch_timer_c3stop;
>> static bool arch_timer_mem_use_virtual;
>>
>> @@ -340,7 +340,7 @@ static void fsl_a008585_set_sne(struct
>> clock_event_device *clk)
>> if (!static_branch_unlikely(&arch_timer_read_ool_enabled))
>> return;
>>
>> - if (arch_timer_uses_ppi == VIRT_PPI)
>> + if (arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI)
>> clk->set_next_event = fsl_a008585_set_next_event_virt;
>> else
>> clk->set_next_event = fsl_a008585_set_next_event_phys;
>> @@ -352,7 +352,7 @@ static void __arch_timer_setup(unsigned type,
>> {
>> clk->features = CLOCK_EVT_FEAT_ONESHOT;
>>
>> - if (type == ARCH_CP15_TIMER) {
>> + if (type == ARCH_TIMER_TYPE_CP15) {
>> if (arch_timer_c3stop)
>> clk->features |= CLOCK_EVT_FEAT_C3STOP;
>> clk->name = "arch_sys_timer";
>> @@ -360,14 +360,14 @@ static void __arch_timer_setup(unsigned type,
>> clk->cpumask = cpumask_of(smp_processor_id());
>> clk->irq = arch_timer_ppi[arch_timer_uses_ppi];
>> switch (arch_timer_uses_ppi) {
>> - case VIRT_PPI:
>> + case ARCH_TIMER_VIRT_PPI:
>> clk->set_state_shutdown = arch_timer_shutdown_virt;
>> clk->set_state_oneshot_stopped =
>> arch_timer_shutdown_virt;
>> clk->set_next_event = arch_timer_set_next_event_virt;
>> break;
>> - case PHYS_SECURE_PPI:
>> - case PHYS_NONSECURE_PPI:
>> - case HYP_PPI:
>> + case ARCH_TIMER_PHYS_SECURE_PPI:
>> + case ARCH_TIMER_PHYS_NONSECURE_PPI:
>> + case ARCH_TIMER_HYP_PPI:
>> clk->set_state_shutdown = arch_timer_shutdown_phys;
>> clk->set_state_oneshot_stopped =
>> arch_timer_shutdown_phys;
>> clk->set_next_event = arch_timer_set_next_event_phys;
>> @@ -447,8 +447,8 @@ static void arch_counter_set_user_access(void)
>>
>> static bool arch_timer_has_nonsecure_ppi(void)
>> {
>> - return (arch_timer_uses_ppi == PHYS_SECURE_PPI &&
>> - arch_timer_ppi[PHYS_NONSECURE_PPI]);
>> + return (arch_timer_uses_ppi == ARCH_TIMER_PHYS_SECURE_PPI &&
>> + arch_timer_ppi[ARCH_TIMER_PHYS_NONSECURE_PPI]);
>> }
>>
>> static u32 check_ppi_trigger(int irq)
>> @@ -469,14 +469,15 @@ static int arch_timer_starting_cpu(unsigned int cpu)
>> struct clock_event_device *clk = this_cpu_ptr(arch_timer_evt);
>> u32 flags;
>>
>> - __arch_timer_setup(ARCH_CP15_TIMER, clk);
>> + __arch_timer_setup(ARCH_TIMER_TYPE_CP15, clk);
>>
>> flags = check_ppi_trigger(arch_timer_ppi[arch_timer_uses_ppi]);
>> enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], flags);
>>
>> if (arch_timer_has_nonsecure_ppi()) {
>> - flags = check_ppi_trigger(arch_timer_
Re: [PATCH v16 04/15] clocksource/drivers/arm_arch_timer: rename some enums and defines, and some cleanups.
On Wed, Nov 16, 2016 at 09:48:57PM +0800, fu@linaro.org wrote:
> From: Fu Wei
>
> Rename some enums and defines, to unify the format of enums and defines
> in arm_arch_timer.h, also update all the users of these enums and defines:
> drivers/clocksource/arm_arch_timer.c
> virt/kvm/arm/hyp/timer-sr.c
I'm happy with making definitions use a consistent ARCH_TIMER_ prefix,
given they're exposed in headers...
> And do some cleanups, according to the suggestion from checkpatch.pl:
> (1) using BIT(nr) instead of (1 << nr)
> (2) using 'unsigned int' instead of 'unsigned'
... but these changes are pointless churn. They make the patch larger,
hardwer to review, and more painful to merge.
Please leave these as they are unless there is a functional problem. If
there will be a functional problem unless these are changed, describe
that in the commit message.
Thanks,
Mark.
>
> No functional change.
>
> Signed-off-by: Fu Wei
> ---
> drivers/clocksource/arm_arch_timer.c | 111
> ++-
> include/clocksource/arm_arch_timer.h | 40 ++---
> virt/kvm/arm/hyp/timer-sr.c | 6 +-
> 3 files changed, 81 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c
> b/drivers/clocksource/arm_arch_timer.c
> index 15341cf..dd1040d 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -66,11 +66,11 @@ struct arch_timer {
> #define to_arch_timer(e) container_of(e, struct arch_timer, evt)
>
> static u32 arch_timer_rate;
> -static int arch_timer_ppi[MAX_TIMER_PPI];
> +static int arch_timer_ppi[ARCH_TIMER_MAX_TIMER_PPI];
>
> static struct clock_event_device __percpu *arch_timer_evt;
>
> -static enum arch_timer_ppi_nr arch_timer_uses_ppi = VIRT_PPI;
> +static enum arch_timer_ppi_nr arch_timer_uses_ppi = ARCH_TIMER_VIRT_PPI;
> static bool arch_timer_c3stop;
> static bool arch_timer_mem_use_virtual;
>
> @@ -340,7 +340,7 @@ static void fsl_a008585_set_sne(struct clock_event_device
> *clk)
> if (!static_branch_unlikely(&arch_timer_read_ool_enabled))
> return;
>
> - if (arch_timer_uses_ppi == VIRT_PPI)
> + if (arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI)
> clk->set_next_event = fsl_a008585_set_next_event_virt;
> else
> clk->set_next_event = fsl_a008585_set_next_event_phys;
> @@ -352,7 +352,7 @@ static void __arch_timer_setup(unsigned type,
> {
> clk->features = CLOCK_EVT_FEAT_ONESHOT;
>
> - if (type == ARCH_CP15_TIMER) {
> + if (type == ARCH_TIMER_TYPE_CP15) {
> if (arch_timer_c3stop)
> clk->features |= CLOCK_EVT_FEAT_C3STOP;
> clk->name = "arch_sys_timer";
> @@ -360,14 +360,14 @@ static void __arch_timer_setup(unsigned type,
> clk->cpumask = cpumask_of(smp_processor_id());
> clk->irq = arch_timer_ppi[arch_timer_uses_ppi];
> switch (arch_timer_uses_ppi) {
> - case VIRT_PPI:
> + case ARCH_TIMER_VIRT_PPI:
> clk->set_state_shutdown = arch_timer_shutdown_virt;
> clk->set_state_oneshot_stopped =
> arch_timer_shutdown_virt;
> clk->set_next_event = arch_timer_set_next_event_virt;
> break;
> - case PHYS_SECURE_PPI:
> - case PHYS_NONSECURE_PPI:
> - case HYP_PPI:
> + case ARCH_TIMER_PHYS_SECURE_PPI:
> + case ARCH_TIMER_PHYS_NONSECURE_PPI:
> + case ARCH_TIMER_HYP_PPI:
> clk->set_state_shutdown = arch_timer_shutdown_phys;
> clk->set_state_oneshot_stopped =
> arch_timer_shutdown_phys;
> clk->set_next_event = arch_timer_set_next_event_phys;
> @@ -447,8 +447,8 @@ static void arch_counter_set_user_access(void)
>
> static bool arch_timer_has_nonsecure_ppi(void)
> {
> - return (arch_timer_uses_ppi == PHYS_SECURE_PPI &&
> - arch_timer_ppi[PHYS_NONSECURE_PPI]);
> + return (arch_timer_uses_ppi == ARCH_TIMER_PHYS_SECURE_PPI &&
> + arch_timer_ppi[ARCH_TIMER_PHYS_NONSECURE_PPI]);
> }
>
> static u32 check_ppi_trigger(int irq)
> @@ -469,14 +469,15 @@ static int arch_timer_starting_cpu(unsigned int cpu)
> struct clock_event_device *clk = this_cpu_ptr(arch_timer_evt);
> u32 flags;
>
> - __arch_timer_setup(ARCH_CP15_TIMER, clk);
> + __arch_timer_setup(ARCH_TIMER_TYPE_CP15, clk);
>
> flags = check_ppi_trigger(arch_timer_ppi[arch_timer_uses_ppi]);
> enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], flags);
>
> if (arch_timer_has_nonsecure_ppi()) {
> - flags = check_ppi_trigger(arch_timer_ppi[PHYS_NONSECURE_PPI]);
> - enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], flags);
> + flags =
> check_ppi_trigger(arch_timer_ppi[ARCH_TIMER_PHYS_NONSECURE_PPI]);
> + enable
