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

[Sean Anderson]

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
>