Message-ID: <ea2968fb-d77b-de66-155f-a76694d4d705@seco.com>
Date:   Mon, 24 May 2021 14:34:13 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Michal Simek <michal.simek@...inx.com>, linux-pwm@...r.kernel.org,
        devicetree@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Alvaro Gamez <alvaro.gamez@...ent.com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Lee Jones <lee.jones@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v3 2/2] clocksource: Add support for Xilinx AXI Timer



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@...o.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.
 >>>
 >>> 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
 >

Powered by blists - more mailing lists