Re: [PATCH v11 2/2] pwm: Add support for Xilinx AXI Timer
From: Sean Anderson
Date: Mon Dec 13 2021 - 13:36:56 EST
Hi Uwe,
On 11/24/21 2:37 AM, Uwe Kleine-König wrote:
Hello Sean,
On Tue, Nov 23, 2021 at 06:25:36PM -0500, Sean Anderson wrote:
This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
found on Xilinx FPGAs. At the moment clock control is very basic: we
just enable the clock during probe and pin the frequency. In the future,
someone could add support for disabling the clock when not in use.
Some common code has been specially demarcated. While currently only
used by the PWM driver, it is anticipated that it may be split off in
the future to be used by the timer driver as well.
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>
Acked-by: Michal Simek <michal.simek@xxxxxxxxxx>
---
Changes in v11:
- Add comment about why we test for #pwm-cells
- Clarify comment on generate out signal
- Rename pwm variables to xilinx_pwm
- Round like Uwe wants...
- s/xilinx_timer/xilinx_pwm/ for non-common functions
I'm mostly happy with this driver now. Just a few minor comments below.
diff --git a/arch/microblaze/kernel/timer.c b/arch/microblaze/kernel/timer.c
index f8832cf49384..dea34a3d4aa4 100644
--- a/arch/microblaze/kernel/timer.c
+++ b/arch/microblaze/kernel/timer.c
@@ -251,6 +251,9 @@ static int __init xilinx_timer_init(struct device_node *timer)
u32 timer_num = 1;
int ret;
+ if (of_property_read_bool(timer, "#pwm-cells"))
+ return 0;
+
The pwm driver has a comment at the location where #pwm-cells is
checked. I suggest to add a matching comment here.
OK
if (initialized)
return -EINVAL;
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 21e3b05a5153..cefbf00b4c7e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -640,4 +640,18 @@ config PWM_VT8500
To compile this driver as a module, choose M here: the module
will be called pwm-vt8500.
+config PWM_XILINX
+ tristate "Xilinx AXI Timer PWM support"
+ depends on OF_ADDRESS
+ depends on COMMON_CLK
+ select REGMAP_MMIO
+ help
+ PWM driver for Xilinx LogiCORE IP AXI timers. This timer is
+ typically a soft core which may be present in Xilinx FPGAs.
+ This device may also be present in Microblaze soft processors.
+ If you don't have this IP in your design, choose N.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-xilinx.
+
endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 708840b7fba8..ea785480359b 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -60,3 +60,4 @@ obj-$(CONFIG_PWM_TWL) += pwm-twl.o
obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o
obj-$(CONFIG_PWM_VISCONTI) += pwm-visconti.o
obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o
+obj-$(CONFIG_PWM_XILINX) += pwm-xilinx.o
diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c
new file mode 100644
index 000000000000..b64735880c4c
--- /dev/null
+++ b/drivers/pwm/pwm-xilinx.c
@@ -0,0 +1,318 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2021 Sean Anderson <sean.anderson@xxxxxxxx>
+ *
+ * Limitations:
+ * - When changing both duty cycle and period, we may end up with one cycle
+ * with the old duty cycle and the new period. This is because the counters
+ * may only be reloaded by first stopping them, or by letting them be
+ * automatically reloaded at the end of a cycle. If this automatic reload
+ * happens after we set TLR0 but before we set TLR1 then we will have a
+ * bad cycle. This could probably be fixed by reading TCR0 just before
+ * reprogramming, but I think it would add complexity for little gain.
+ * - Cannot produce 100% duty cycle by configuring the TLRs. This might be
+ * possible by stopping the counters at an appropriate point in the cycle,
+ * but this is not (yet) implemented.
+ * - Only produces "normal" output.
+ * - Always produces low output if disabled.
+ */
+
+#include <clocksource/timer-xilinx.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+/*
+ * The following functions are "common" to drivers for this device, and may be
+ * exported at a future date.
+ */
+u32 xilinx_timer_tlr_cycles(struct xilinx_timer_priv *priv, u32 tcsr,
+ u64 cycles)
+{
+ WARN_ON(cycles < 2 || cycles - 2 > priv->max);
+
+ if (tcsr & TCSR_UDT)
+ return cycles - 2;
+ return priv->max - cycles + 2;
+}
+
+unsigned int xilinx_timer_get_period(struct xilinx_timer_priv *priv,
+ u32 tlr, u32 tcsr)
+{
+ u64 cycles;
+
+ if (tcsr & TCSR_UDT)
+ cycles = tlr + 2;
+ else
+ cycles = (u64)priv->max - tlr + 2;
+
+ /* cycles has a max of 2^32 + 2 */
+ return DIV64_U64_ROUND_UP(cycles * NSEC_PER_SEC,
+ clk_get_rate(priv->clk));
+}
+
+/*
+ * The idea here is to capture whether the PWM is actually running (e.g.
+ * because we or the bootloader set it up) and we need to be careful to ensure
+ * we don't cause a glitch. According to the data sheet, to enable the PWM we
+ * need to
+ *
+ * - Set both timers to generate mode (MDT=1)
+ * - Set both timers to PWM mode (PWMA=1)
+ * - Enable the generate out signals (GENT=1)
+ *
+ * In addition,
+ *
+ * - The timer must be running (ENT=1)
+ * - The timer must auto-reload TLR into TCR (ARHT=1)
+ * - We must not be in the process of loading TLR into TCR (LOAD=0)
+ * - Cascade mode must be disabled (CASC=0)
+ *
+ * If any of these differ from usual, then the PWM is either disabled, or is
+ * running in a mode that this driver does not support.
+ */
+#define TCSR_PWM_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)
+#define TCSR_PWM_CLEAR (TCSR_MDT | TCSR_LOAD)
+#define TCSR_PWM_MASK (TCSR_PWM_SET | TCSR_PWM_CLEAR)
+
+struct xilinx_pwm_device {
+ struct pwm_chip chip;
+ struct xilinx_timer_priv priv;
+};
+
+static inline struct xilinx_timer_priv
+*xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
+{
+ return &container_of(chip, struct xilinx_pwm_device, chip)->priv;
+}
+
+static bool xilinx_timer_pwm_enabled(u32 tcsr0, u32 tcsr1)
+{
+ return ((TCSR_PWM_MASK | TCSR_CASC) & tcsr0) == TCSR_PWM_SET &&
+ (TCSR_PWM_MASK & tcsr1) == TCSR_PWM_SET;
+}
+
+static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
+ const struct pwm_state *state)
+{
+ struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
+ u32 tlr0, tlr1, tcsr0, tcsr1;
+ u64 period_cycles, duty_cycles;
+ unsigned long rate;
+
+ if (state->polarity != PWM_POLARITY_NORMAL)
+ return -EINVAL;
+
+ /*
+ * To be representable by TLR, cycles must be between 2 and
+ * priv->max + 2. To enforce this we can reduce the duty
+ * cycle, but we may not increase it.
s/duty cycle/period/
replaced with "cycles", since this applies to both
+ */
+ rate = clk_get_rate(priv->clk);
+ /* Avoid overflow */
+ period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
+ period_cycles = mul_u64_u32_div(period_cycles, rate, NSEC_PER_SEC);
+ /* Clamp it for Uwe */
Hmm, not sure this comment is understandable in the long term.
Incorporated into the above
+ period_cycles = min_t(u64, period_cycles, priv->max + 2);
+ if (period_cycles < 2)
+ return -ERANGE;
+
+ /* Same thing for duty cycles */
s/duty cycles/duty cycle/, also for the variable name.
It is named this way to match period_cycles. "duty_cycle_cycles" seems rather redundant.
+ duty_cycles = min_t(u64, state->duty_cycle, ULONG_MAX * NSEC_PER_SEC);
+ duty_cycles = mul_u64_u32_div(duty_cycles, rate, NSEC_PER_SEC);
+ duty_cycles = min_t(u64, duty_cycles, priv->max + 2);
--Sean