Re: [PATCH v3 2/2] clocksource: Add support for Xilinx AXI Timer

[Sean Anderson <sean.anderson@seco.com>]

On 5/17/21 3:54 AM, Michal Simek wrote:
 >
 >
 > On 5/14/21 4:40 PM, Sean Anderson wrote:
 >>
 >>
 >> On 5/14/21 4:59 AM, Michal Simek wrote:
 >>>
 >>>
 >>> On 5/11/21 9:12 PM, Sean Anderson wrote:
 >>>> This adds generic clocksource and clockevent support for Xilinx
 >> LogiCORE IP
 >>>> AXI soft timers commonly found on Xilinx FPGAs. This timer is also the
 >>>> primary timer for Microblaze processors. This commit also adds
 >> support for
 >>>> configuring this timer as a PWM (though this could be split off if
 >>>> necessary). This whole driver lives in clocksource because it is
 >> primarily
 >>>> clocksource stuff now (even though it started out as a PWM driver). I
 >> think
 >>>> teasing apart the driver would not be worth it since they share so many
 >>>> functions.
 >>>>
 >>>> This driver configures timer 0 (which is always present) as a
 >> clocksource,
 >>>> and timer 1 (which might be missing) as a clockevent. I don't know if
 >> this
 >>>> is the correct priority for these timers, or whether we should be
 >> using a
 >>>> more dynamic allocation scheme.
 >>>>
 >>>> At the moment clock control is very basic: we just enable the clock
 >> during
 >>>> probe and pin the frequency. In the future, someone could add support
 >> for
 >>>> disabling the clock when not in use. Cascade mode is also unsupported.
 >>>>
 >>>> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
 >>>>
 >>>> [1]
 >> https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
 >>
 >>>>
 >>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >>>> ---
 >>>> Please let me know if I should organize this differently or if it should
 >>>> be broken up.
 >>>>
 >>>> Changes in v3:
 >>>> - Add clockevent and clocksource support
 >>>> - Rewrite probe to only use a device_node, since timers may need to be
 >>>>      initialized before we have proper devices. This does bloat the
 >> code a bit
 >>>>      since we can no longer rely on helpers such as dev_err_probe. We also
 >>>>      cannot rely on device resources being free'd on failure, so we
 >> must free
 >>>>      them manually.
 >>>> - We now access registers through xilinx_timer_(read|write). This
 >> allows us
 >>>>      to deal with endianness issues, as originally seen in the microblaze
 >>>>      driver. CAVEAT EMPTOR: I have not tested this on big-endian!
 >>>> - Remove old microblaze driver
 >>>>
 >>>> Changes in v2:
 >>>> - Don't compile this module by default for arm64
 >>>> - Add dependencies on COMMON_CLK and HAS_IOMEM
 >>>> - Add comment explaining why we depend on !MICROBLAZE
 >>>> - Add comment describing device
 >>>> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
 >>>> - Use NSEC_TO_SEC instead of defining our own
 >>>> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
 >>>> - Cast dividends to u64 to avoid overflow
 >>>> - Check for over- and underflow when calculating TLR
 >>>> - Set xilinx_pwm_ops.owner
 >>>> - Don't set pwmchip.base to -1
 >>>> - Check range of xlnx,count-width
 >>>> - Ensure the clock is always running when the pwm is registered
 >>>> - Remove debugfs file :l
 >>>> - Report errors with dev_error_probe
 >>>>
 >>>>     arch/microblaze/kernel/Makefile    |   2 +-
 >>>>     arch/microblaze/kernel/timer.c     | 326 ---------------
 >>>>     drivers/clocksource/Kconfig        |  15 +
 >>>>     drivers/clocksource/Makefile       |   1 +
 >>>>     drivers/clocksource/timer-xilinx.c | 650 +++++++++++++++++++++++++++++
 >>>>     5 files changed, 667 insertions(+), 327 deletions(-)
 >>>>     delete mode 100644 arch/microblaze/kernel/timer.c
 >>>>     create mode 100644 drivers/clocksource/timer-xilinx.c
 >>>
 >>> I don't think this is the right way to go.
 >>> The first patch should be move current timer driver from microblaze to
 >>> generic location and then apply patches on the top based on what you are
 >>> adding/fixing to be able to review every change separately.
 >>> When any issue happens it can be bisected and exact patch is identified.
 >>> With this way we will end up in this patch and it will take a lot of
 >>> time to find where that problem is.
 >>
 >> What parts would you like to see split? Fundamentally, this current
 >> patch is a reimplementation of the driver. I think the only reasonable
 >> split would be to add PWM support in a separate patch.
 >>
 >> I do not think that genericizing the microblaze timer driver is an
 >> integral part of adding PWM support. This is especially since you seem
 >> opposed to using existing devicetree properties to inform the driver. I
 >> am inclined to just add a patch adding a check for '#-pwm-cells' to the
 >> existing driver and otherwise leave it untouched.
 >
 > As I said I think the patches should be like this.
 > 1. Cover existing DT binding based on current code.
 > 2. Move time out of arch/microblaze to drivers/clocksource/ and even
 > enable it via Kconfig just for Microblaze.
 > 3. Remove dependency on Microblaze and enable build for others. I have
 > seen at least one cpuinfo.cpu_clock_freq assignment. This code can be
 > likely completely removed or deprecate.

This could be deprecated, but cannot be removed since existing device
trees (e.g. qemu) have neither clocks nor clock-frequency properties.

 > 4. Make driver as module
 > 5. Do whatever changes you want before adding pwm support
 > 6. Extend DT binding doc for PWM support
 > 7. Add PWM support

Frankly, I am inclined to just leave the microblaze timer as-is. The PWM
driver is completely independent. I have already put too much effort into
this driver, and I don't have the energy to continue working on the
microblaze timer.

--Sean

 > I expect you know that some time ago we have also added support for
 > Microblaze SMP and this code has never been sent upstream. You should
 > just be aware about it.
 > https://github.com/Xilinx/linux-xlnx/blob/master/arch/microblaze/kernel/timer.c
 >
 > Thanks,
 > Michal
 >