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

[Sean Anderson <sean.anderson@seco.com>]

On 5/10/21 6:20 AM, Michal Simek wrote:
 > Hi,
 >
 > On 5/7/21 12:36 AM, Sean Anderson wrote:
 >>
 >>
 >> 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@seco.com>
 >>>>>> ---
 >>>>>> 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?
 >
 >
 > You can look at Guenter's files which he uses for testing here.
 > http://server.roeck-us.net/qemu/microblazeel/

Thanks! These really helped when developing. In particular, I was unable
to get a working system with GCC 9, but I haven't looked into it
further.

--Sean

 >
 > Or you can use Xilinx petalinux distribution or Yocto layer which should
 > have qemu integrated.
 >
 > Thanks,
 > Michal
 >