Messages in this thread | | | From | Sean Anderson <> | Subject | Re: [PATCH v7 2/3] clocksource: Rewrite Xilinx AXI timer driver | Date | Tue, 5 Oct 2021 18:08:24 -0400 |
| |
On 9/24/21 2:52 AM, Michal Simek wrote: > 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@seco.com> >> --- >> 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.
Unfortunately, I do not have the time do do this at the moment. Because these drivers are independent in nature, I propose to drop these changes to the timer driver, but leave the common functions split out. In the future, I (or you) may come back and make the changes in this patch in an incremental fashion. The only change necessary for this driver would be to check for #pwm-cells.
--Sean
| |