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

Hi,

On 6/18/21 11:24 PM, Sean Anderson wrote:

On 6/16/21 8:12 AM, Michal Simek wrote:

quoted

Hi Uwe,

On 5/25/21 8:11 AM, Uwe Kleine-König wrote:

quoted

Hello Sean, hello Michal,

On Mon, May 24, 2021 at 09:00:51AM +0200, Michal Simek wrote:

quoted

On 5/20/21 10:13 PM, Sean Anderson wrote:

quoted

On 5/19/21 3:24 AM, Michal Simek wrote:

quoted

On 5/18/21 12:15 AM, Sean Anderson wrote:

quoted

This could be deprecated, but cannot be removed since existing

device

quoted

quoted

quoted

quoted

quoted

quoted

trees (e.g. qemu) have neither clocks nor clock-frequency

properties.

quoted

quoted

quoted

quoted

quoted

Rob: Do we have any obligation to keep properties for other projects?

If a binding is in the wild and used to be documented, it has to stay.

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

quoted

driver is completely independent. I have already put too much

effort into

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

project but never had time to upstream it because of couple of

steps above.

quoted

quoted

quoted

quoted

quoted

We need to do it right based on steps listed above. If this is too

much

quoted

quoted

quoted

quoted

quoted

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,

quoted

quoted

quoted

quoted

    renesas TPU, etc. It is completely reasonable to keep separate
    drivers for these purposes. There is no Linux requirement that

each

quoted

quoted

quoted

quoted

    device have only one driver, especially if it has multiple

functions

quoted

quoted

quoted

quoted

    or ways to be configured.

It doesn't mean that it was done properly and correctly. Code
duplication is bad all the time.

IMHO it's not so much about code duplication. Yes, code duplication is
bad and should be prevented if possible. But it's more important to not
introduce surprises. So I think it should be obvious from reading the
device tree source which timer is used to provide the PWM. I don't care
much if this is from an extra property (like xilinx,provide-pwm),
overriding the compatible or some other explicit mechanism. IIUC in this
suggested patch the selection is implicit and so this isn't so nice.

quoted

quoted

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

quoted

quoted

quoted

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?

Not sure I can and want to provide guidance here. This is not Perl, but
still TIMTOWTDI. If it was me who cared here, I'd look into the
auxiliary bus (Documentation/driver-api/auxiliary_bus.rst) to check if
it can help to solve this problem.

I recently got patches for cadence TTC driver
(drivers/clocksource/timer-cadence-ttc.c) for PWM support too. It is the
second and very similar case. This driver is used on Zynq as clock
source and can be also use as PWM. I can't believe that there are no
other examples how to deal with these timers which are used for PWM
generation.

The approach I took in v4 is that probe functions and driver callbacks
live in drivers/timer and drivers/pwm, and common functions live in
drivers/mfd (although I may move them to drivers/timer since Lee Jones
doesn't like them there).

I would greatly appreciate if you could review v4. It has been on the
list for three weeks now with no comments from either you or Uwe.

I will take a look at it next week.

Thanks,
Michal