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@xxxxxxxx> >>>> --- >>>> 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 >
