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

[Michal Simek]

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/

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

Thanks,
Michal