Re: [PATCH v2 2/2] pwm: Add support for Xilinx AXI Timer

[Sean Anderson]

On 5/6/21 12:54 PM, Michal Simek wrote:
> Hi,
>
> On 5/6/21 4:28 PM, Sean Anderson wrote:
>>
>>
>> On 5/5/21 2:37 AM, Michal Simek wrote:
>>>
>>>
>>> On 5/4/21 8:49 PM, Sean Anderson wrote:
>>>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
>>>> found on Xilinx FPGAs. There is another driver for this device located
>>>> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
>>>> 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>
>>>> ---
>>>> I tried adding a XILINX_PWM_ prefix to all the defines, but IMO it
>>>> really hurt readability. That prefix almost doubles the size the
>>>> defines, and is particularly excessive in something like
>>>> XILINX_PWM_TCSR_RUN_MASK.
>>>>
>>>> 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
>>>>
>>>>     drivers/pwm/Kconfig      |  13 ++
>>>>     drivers/pwm/Makefile     |   1 +
>>>>     drivers/pwm/pwm-xilinx.c | 301 +++++++++++++++++++++++++++++++++++++++
>>>>     3 files changed, 315 insertions(+)
>>>>     create mode 100644 drivers/pwm/pwm-xilinx.c
>>>
>>> Without looking below another driver which target the same IP is just
>>> wrong that's why NACK from me.
>>
>> Can you elaborate on this position a bit more? I don't think a rework of
>> the microblaze driver should hold back this one. They cannot be enabled
>> at the same time. I think it is OK to leave the work of making them
>> coexist for a future series (written by someone with microblaze hardware
>> to test on).
>
> I am here to test it on Microblaze. In a lot of cases you don't have
> access to all HW you should test things on but that's why others can
> help with this.

Ok, can you convert the microblaze driver then? I'm afraid I can't work
on a driver if I don't have a system to test it on. There are too many
small bugs which can creep in without anything to work with. If you are
insistant that there must be no driver duplication (even temporarily),
then you should help with the deduplication :)

I would also be willing to try and get a microblaze qemu setup working,
but I have found no good instructions for doing so with mainline linux.
The best I found was [1]. Do you have a working setup for this?

--Sean

[1] https://blog.waldemar-brodkorb.de/index.php?/archives/10-qemu-microblaze-system-emulation-tipps.html

> As I said in previous thread driver duplication is not good way to go
> and never was.
>
> This patch targets axi timer IP which is already in the tree just for
> Microblaze. You want to use it on other HW which is good but it needs to
> be done properly which is not create another copy.
> The right way is to get axi timer out of arch/microblaze to
> drivers/clocksource (or any other driver folder) and add PMW
> functionality on the top of it.
> I would expect that PWM guys will say how to add PWM support to timer
> driver which is not unique configuration.
>
> Thanks,
> Michal
>