On 5/24/21 3:00 AM, Michal Simek wrote: > > > On 5/20/21 10:13 PM, Sean Anderson wrote: >> >> >> On 5/19/21 3:24 AM, Michal Simek wrote: >>> >>> >>> On 5/18/21 12:15 AM, Sean Anderson wrote: >>>> >>>> >>>> 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. >>> >>> Rob: Do we have any obligation to keep properties for other projects? >>> >>> >>>>> 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. >> >> 1. Many timers have separate clocksource and PWM drivers. E.g. samsung, >> renesas TPU, etc. It is completely reasonable to keep separate >> drivers for these purposes. There is no Linux requirement that each >> device have only one driver, especially if it has multiple functions >> or ways to be configured. > > It doesn't mean that it was done properly and correctly. Code > duplication is bad all the time. IMO after doing all this there is not too much which can be reused. We can reuse the read/write functions, the TLR calculations and the processing of xlnx,counter-width and xlnx,one-timer. The timer probe is likely much more cleanly implemented with timer_of_init. And not having a platform device greatly complicates the PWM probe. > >> 2. If you want to do work on a driver, I'm all for it. However, if you >> have not yet submitted that work to the list, you should not gate >> other work behind it. Saying that X feature must be gated behind Y >> *even if X works completely independently of Y* is just stifling >> development. > > I gave you guidance how I think this should be done. I am not gating you > from this work. Your patch is not working on Microblaze arch which is > what I maintain. I tested this on Microblaze qemu. What problems do you see? --Sean > And I don't want to go the route that we will have two > drivers for the same IP without integration. We were there in past and > it is just pain. > I am expecting that PWM guys will guide how this should be done > properly. I haven't heard any guidance on this yet. > Thierry/Uwe: Any comment? > > >> 3. There is a clear desire for a PWM driver for this device. You, I, and >> Alvaro have all written separate drivers for this device because we >> want to use it as a PWM. By preventing merging this driver, you are >> encouraging duplicate effort by the next person who wants to use this >> device as a PWM, and sees that there is no driver in the tree. > > We should do it cleanly that it will be easy to maintain which is not by > creating two separate drivers or by switching to completely new driver. > > Thanks, > Michal >
