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

[Michal Simek]

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.

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