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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1bfde199-617a-343c-10ed-4c436bfd908f@seco.com>
Date:   Thu, 6 May 2021 10:28:13 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Michal Simek <michal.simek@...inx.com>, linux-pwm@...r.kernel.org,
        devicetree@...r.kernel.org
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Alvaro Gamez <alvaro.gamez@...ent.com>,
        Lee Jones <lee.jones@...aro.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Subject: Re: [PATCH v2 2/2] pwm: Add support for Xilinx AXI Timer



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@...o.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).

--Sean