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

[Sean Anderson]

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
>