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

From: Michal Simek <hidden>
Date: 2021-05-19 07:24:51
Also in: linux-arm-kernel, linux-pwm, lkml


On 5/18/21 12:15 AM, Sean Anderson wrote:


On 5/17/21 3:54 AM, Michal Simek wrote:

quoted


On 5/14/21 4:40 PM, Sean Anderson wrote:

quoted


On 5/14/21 4:59 AM, Michal Simek wrote:

quoted


On 5/11/21 9:12 PM, Sean Anderson wrote:

quoted

This adds generic clocksource and clockevent support for Xilinx

LogiCORE IP

quoted

AXI soft timers commonly found on Xilinx FPGAs. This timer is also the
primary timer for Microblaze processors. This commit also adds

support for

quoted

configuring this timer as a PWM (though this could be split off if
necessary). This whole driver lives in clocksource because it is

primarily

quoted

clocksource stuff now (even though it started out as a PWM driver). I

think

quoted

teasing apart the driver would not be worth it since they share so

many

quoted

functions.

This driver configures timer 0 (which is always present) as a

clocksource,

quoted

and timer 1 (which might be missing) as a clockevent. I don't know if

this

quoted

is the correct priority for these timers, or whether we should be

using a

quoted

more dynamic allocation scheme.

At the moment clock control is very basic: we just enable the clock

during

quoted

probe and pin the frequency. In the future, someone could add support

for

quoted

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

quoted

quoted
quoted
quoted
[1]

https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf

quoted

Signed-off-by: Sean Anderson <redacted>
---
Please let me know if I should organize this differently or if it

should

quoted

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

quoted

     since we can no longer rely on helpers such as dev_err_probe.

We also

quoted

     cannot rely on device resources being free'd on failure, so we

must free

quoted

     them manually.
- We now access registers through xilinx_timer_(read|write). This

allows us

quoted

     to deal with endianness issues, as originally seen in the

microblaze

quoted

     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

quoted

- 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

+++++++++++++++++++++++++++++

quoted

    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

quoted

adding/fixing to be able to review every change separately.
When any issue happens it can be bisected and exact patch is

identified.

quoted

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.

Rob: Do we have any obligation to keep properties for other projects?

quoted

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.

I understand. I am actually using axi timer as pwm driver in one of my
project but never had time to upstream it because of couple of steps above.
We need to do it right based on steps listed above. If this is too much
work it will have to wait. I will NACK all attempts to add separate
driver for IP which we already support in the tree.

Thanks,
Michal

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help