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

[Sean Anderson]

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@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?
>
>
> 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
>