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

[Michal Simek]

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.
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