Re: [PATCH v7 2/3] clocksource: Rewrite Xilinx AXI timer driver

[Michal Simek]

Dear Sean,

On 9/16/21 8:05 PM, Sean Anderson wrote:
> This rewrites the Xilinx AXI timer driver to be more platform agnostic.
> Some common code has been split off so it can be reused. These routines
> currently live in drivers/mfd. The largest changes are summarized below:
> 
> - We now support any number of timer devices, possibly with only one
>   counter each. The first counter will be used as a clocksource. Every
>   other counter will be used as a clockevent. This allocation scheme was
>   chosen arbitrarily.
> - We do not use timer_of_init because we need to perform some tasks in
>   between different stages. For example, we must ensure that ->read and
>   ->write are initialized before registering the irq. This can only happen
>   after we have gotten the register base (to detect endianness). We also
>   have a rather unusual clock initialization sequence in order to remain
>   backwards compatible. Due to this, it's ok for the initial clock request
>   to fail, and we do not want other initialization to be undone. Lastly, it
>   is more convenient to do one allocation for xilinx_clockevent_device than
>   to do one for timer_of and one for xilinx_timer_priv.
> - We now pay attention to xlnx,count-width and handle smaller width timers.
>   The default remains 32.
> - We access registers using regmap. This automatically deals with
>   endianness issues, so we no longer have to use our own wrappers. It
>   also provides locking for clockevents which have to worry about being
>   interrupted in the middle of a read/modify/write.
> 
> Note that while the existing timer driver always sets the cpumask to cpu
> 0, this version sets it to all possible CPUs. I believe this is correct
> for multiprocessor systems where the timer is not physically wired to a
> particular CPU's interrupt line. For uniprocessor systems (like most
> microblaze systems) this makes no difference.
> 
> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxx>
> ---
> This has been tested on microblaze qemu.
> 
> Changes in v7:
> - Add dependency on OF_ADDRESS
> 
> Changes in v6:
> - Add __init* attributes
> - Export common symbols
> - Fix goto'ing incorrect label for cleanup
> - Remove duplicate regmap_config
> - Round to closest period in xilinx_timer_get_period to ensure proper
>   semantics for xilinx_pwm_get_state
> 
> Changes in v5:
> - Fix some overflows when setting the max value for clockevent and
>   sched_clock
> - Just use clk_register_fixed_rate instead of the "private" version
> - Remove duplicate register definitions
> - Remove xilinx_timer_tlr_period
> - Remove xlnx,axi-timer-2.0 compatible string
> - Require that callers check arguments to xilinx_timer_tlr_cycles
> - Use regmap to deal with endianness issues as suggested by Lee
> 
> Changes in v4:
> - Break out clock* drivers into their own file
> 
>  MAINTAINERS                               |   6 +
>  arch/microblaze/kernel/Makefile           |   3 +-
>  arch/microblaze/kernel/timer.c            | 326 ----------------------
>  drivers/clocksource/Kconfig               |  13 +
>  drivers/clocksource/Makefile              |   1 +
>  drivers/clocksource/timer-xilinx-common.c |  71 +++++
>  drivers/clocksource/timer-xilinx.c        | 323 +++++++++++++++++++++
>  include/clocksource/timer-xilinx.h        |  91 ++++++
>  8 files changed, 506 insertions(+), 328 deletions(-)
>  delete mode 100644 arch/microblaze/kernel/timer.c
>  create mode 100644 drivers/clocksource/timer-xilinx-common.c
>  create mode 100644 drivers/clocksource/timer-xilinx.c
>  create mode 100644 include/clocksource/timer-xilinx.h


I have said it couple of times. I won't accept this in this form.
I have no problem to move this driver out of microblaze. But I want to
see transition from current state to new state and check it with baby
steps which are bisectable if any problem happens.
Because in this style we end in this patch and it will take some time to
find out what it is failing.

That's why my NACK.

Thanks,
Michal