devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 1/3] dt-bindings: pwm: Add Xilinx AXI Timer
@ 2021-07-19 22:13 Sean Anderson
  2021-07-19 22:13 ` [PATCH v5 2/3] clocksource: Rewrite Xilinx AXI timer driver Sean Anderson
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Sean Anderson @ 2021-07-19 22:13 UTC (permalink / raw)
  To: linux-pwm, devicetree, Thierry Reding
  Cc: Alvaro Gamez, Uwe Kleine-König, michal.simek, Lee Jones,
	linux-arm-kernel, linux-kernel, Sean Anderson, Rob Herring

This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is a
"soft" block, so it has some parameters which would not be configurable in
most hardware. This binding is usually automatically generated by Xilinx's
tools, so the names and values of some properties should be kept as they
are, if possible. In addition, this binding is already in the kernel at
arch/microblaze/boot/dts/system.dts, and in user software such as QEMU.

The existing driver uses the clock-frequency property, or alternatively the
/cpus/timebase-frequency property as its frequency input. Because these
properties are deprecated, they have not been included with this schema.
All new bindings should use the clocks/clock-names properties to specify
the parent clock.

Because we need to init timer devices so early in boot, we determine if we
should use the PWM driver or the clocksource/clockevent driver by the
presence/absence, respectively, of #pwm-cells. Because both counters are
used by the PWM, there is no need for a separate property specifying which
counters are to be used for the PWM.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v5:
- Update commit message to reflect revisions
- Fix indentation lint
- Add example for timer binding
- Remove xlnx,axi-timer-2.0 compatible string
- Move schema into the timer directory

Changes in v4:
- Remove references to generate polarity so this can get merged
- Predicate PWM driver on the presence of #pwm-cells
- Make some properties optional for clocksource drivers

Changes in v3:
- Mark all boolean-as-int properties as deprecated
- Add xlnx,pwm and xlnx,gen?-active-low properties.
- Make newer replacement properties mutually-exclusive with what they
  replace
- Add an example with non-deprecated properties only.

Changes in v2:
- Use 32-bit addresses for example binding

 .../bindings/timer/xlnx,xps-timer.yaml        | 91 +++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml

diff --git a/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml b/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
new file mode 100644
index 000000000000..e5439653dc03
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
@@ -0,0 +1,91 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
+
+maintainers:
+  - Sean Anderson <sean.anderson@seco.com>
+
+properties:
+  compatible:
+    contains:
+      const: xlnx,xps-timer-1.00.a
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: s_axi_aclk
+
+  interrupts:
+    maxItems: 1
+
+  reg:
+    maxItems: 1
+
+  xlnx,count-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 8
+    maximum: 32
+    default: 32
+    description:
+      The width of the counter(s), in bits.
+
+  xlnx,one-timer-only:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    description:
+      Whether only one timer is present in this block.
+
+required:
+  - compatible
+  - reg
+  - xlnx,one-timer-only
+
+allOf:
+  - if:
+      required:
+        - '#pwm-cells'
+    then:
+      allOf:
+        - required:
+            - clocks
+        - properties:
+            xlnx,one-timer-only:
+              const: 0
+    else:
+      required:
+        - interrupts
+  - if:
+      required:
+        - clocks
+    then:
+      required:
+        - clock-names
+
+additionalProperties: true
+
+examples:
+  - |
+    timer@800e0000 {
+        clock-names = "s_axi_aclk";
+        clocks = <&zynqmp_clk 71>;
+        compatible = "xlnx,xps-timer-1.00.a";
+        reg = <0x800e0000 0x10000>;
+        interrupts = <0 39 2>;
+        xlnx,count-width = <16>;
+        xlnx,one-timer-only = <0x0>;
+    };
+
+    timer@800f0000 {
+        #pwm-cells = <0>;
+        clock-names = "s_axi_aclk";
+        clocks = <&zynqmp_clk 71>;
+        compatible = "xlnx,xps-timer-1.00.a";
+        reg = <0x800e0000 0x10000>;
+        xlnx,count-width = <32>;
+        xlnx,one-timer-only = <0x0>;
+    };
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v5 2/3] clocksource: Rewrite Xilinx AXI timer driver
  2021-07-19 22:13 [PATCH v5 1/3] dt-bindings: pwm: Add Xilinx AXI Timer Sean Anderson
@ 2021-07-19 22:13 ` Sean Anderson
  2021-07-20 21:41   ` kernel test robot
  2021-07-21  2:35   ` kernel test robot
  2021-07-19 22:13 ` [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer Sean Anderson
  2021-07-20 13:11 ` [PATCH v5 1/3] dt-bindings: pwm: Add " Rob Herring
  2 siblings, 2 replies; 15+ messages in thread
From: Sean Anderson @ 2021-07-19 22:13 UTC (permalink / raw)
  To: linux-pwm, devicetree, Thierry Reding
  Cc: Alvaro Gamez, Uwe Kleine-König, michal.simek, Lee Jones,
	linux-arm-kernel, linux-kernel, Sean Anderson, Daniel Lezcano,
	Thomas Gleixner

This rewrites the Xilinx AXI timer driver to be more platform agnostic.
Some common code has been split off so it can be reused. These routines
currently live in drivers/mfd. The largest changes have taken place in the
initialization:

- We now support any number of timer devices, possibly with only one
  counter each. The first counter will be used as a clocksource. Every
  other counter will be used as a clockevent.
- We do not use timer_of_init because we need to perform some tasks in
  between different stages. For example, we must ensure that ->read and
  ->write are initialized before registering the irq. This can only happen
  after we have gotten the register base (to detect endianness). We also
  have a rather unusual clock initialization sequence in order to remain
  backwards compatible. Due to this, it's ok for the initial clock request
  to fail, and we do not want other initialization to be undone. Lastly, it
  is more convenient to do one allocation for xilinx_clockevent_device than
  to do one for timer_of and one for xilinx_timer_priv.
- We now pay attention to xlnx,count-width and handle smaller width timers.
  The default remains 32.
- We access registers using regmap. This automatically deals with
  endianness issues, so we no longer have to use our own wrappers. It
  also provides locking for clockevents which have to worry about being
  interrupted in the middle of a read/modify/write.

Note that while the existing timer driver always sets the cpumask to cpu
0, this version sets it to all possible CPUs. I believe this is correct
for multiprocessor systems where the timer is not physically wired to a
particular CPU's interrupt line. For uniprocessor systems (like most
microblaze systems) this makes no difference.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
This has been tested on microblaze qemu.

Changes in v5:
- Remove duplicate register definitions
- Remove xilinx_timer_tlr_period
- Require that callers check arguments to xilinx_timer_tlr_cycles
- Remove xlnx,axi-timer-2.0 compatible string
- Use regmap to deal with endianness issues as suggested by Lee
- Fix some overflows when setting the max value for clockevent and
  sched_clock
- Just use clk_register_fixed_rate instead of the "private" version

Changes in v4:
- Break out clock* drivers into their own file

 MAINTAINERS                               |   6 +
 arch/microblaze/kernel/Makefile           |   3 +-
 arch/microblaze/kernel/timer.c            | 326 ---------------------
 drivers/clocksource/Kconfig               |  12 +
 drivers/clocksource/Makefile              |   1 +
 drivers/clocksource/timer-xilinx-common.c |  67 +++++
 drivers/clocksource/timer-xilinx.c        | 331 ++++++++++++++++++++++
 include/clocksource/timer-xilinx.h        |  91 ++++++
 8 files changed, 509 insertions(+), 328 deletions(-)
 delete mode 100644 arch/microblaze/kernel/timer.c
 create mode 100644 drivers/clocksource/timer-xilinx-common.c
 create mode 100644 drivers/clocksource/timer-xilinx.c
 create mode 100644 include/clocksource/timer-xilinx.h

diff --git a/MAINTAINERS b/MAINTAINERS
index bd7aff0c120f..252d71addd18 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20046,6 +20046,12 @@ F:	drivers/misc/Makefile
 F:	drivers/misc/xilinx_sdfec.c
 F:	include/uapi/misc/xilinx_sdfec.h
 
+XILINX TIMER/PWM DRIVER
+M:	Sean Anderson <sean.anderson@seco.com>
+S:	Maintained
+F:	drivers/clocksource/timer-xilinx*
+F:	include/clocksource/timer-xilinx.h
+
 XILINX UARTLITE SERIAL DRIVER
 M:	Peter Korsgaard <jacmet@sunsite.dk>
 L:	linux-serial@vger.kernel.org
diff --git a/arch/microblaze/kernel/Makefile b/arch/microblaze/kernel/Makefile
index 15a20eb814ce..3b6d725398f8 100644
--- a/arch/microblaze/kernel/Makefile
+++ b/arch/microblaze/kernel/Makefile
@@ -5,7 +5,6 @@
 
 ifdef CONFIG_FUNCTION_TRACER
 # Do not trace early boot code and low level code
-CFLAGS_REMOVE_timer.o = -pg
 CFLAGS_REMOVE_intc.o = -pg
 CFLAGS_REMOVE_early_printk.o = -pg
 CFLAGS_REMOVE_ftrace.o = -pg
@@ -17,7 +16,7 @@ extra-y := head.o vmlinux.lds
 obj-y += dma.o exceptions.o \
 	hw_exception_handler.o irq.o \
 	process.o prom.o ptrace.o \
-	reset.o setup.o signal.o sys_microblaze.o timer.o traps.o unwind.o
+	reset.o setup.o signal.o sys_microblaze.o traps.o unwind.o
 
 obj-y += cpu/
 
diff --git a/arch/microblaze/kernel/timer.c b/arch/microblaze/kernel/timer.c
deleted file mode 100644
index f8832cf49384..000000000000
--- a/arch/microblaze/kernel/timer.c
+++ /dev/null
@@ -1,326 +0,0 @@
-/*
- * Copyright (C) 2007-2013 Michal Simek <monstr@monstr.eu>
- * Copyright (C) 2012-2013 Xilinx, Inc.
- * Copyright (C) 2007-2009 PetaLogix
- * Copyright (C) 2006 Atmark Techno, Inc.
- *
- * This file is subject to the terms and conditions of the GNU General Public
- * License. See the file "COPYING" in the main directory of this archive
- * for more details.
- */
-
-#include <linux/interrupt.h>
-#include <linux/delay.h>
-#include <linux/sched.h>
-#include <linux/sched/clock.h>
-#include <linux/sched_clock.h>
-#include <linux/clk.h>
-#include <linux/clockchips.h>
-#include <linux/of_address.h>
-#include <linux/of_irq.h>
-#include <linux/timecounter.h>
-#include <asm/cpuinfo.h>
-
-static void __iomem *timer_baseaddr;
-
-static unsigned int freq_div_hz;
-static unsigned int timer_clock_freq;
-
-#define TCSR0	(0x00)
-#define TLR0	(0x04)
-#define TCR0	(0x08)
-#define TCSR1	(0x10)
-#define TLR1	(0x14)
-#define TCR1	(0x18)
-
-#define TCSR_MDT	(1<<0)
-#define TCSR_UDT	(1<<1)
-#define TCSR_GENT	(1<<2)
-#define TCSR_CAPT	(1<<3)
-#define TCSR_ARHT	(1<<4)
-#define TCSR_LOAD	(1<<5)
-#define TCSR_ENIT	(1<<6)
-#define TCSR_ENT	(1<<7)
-#define TCSR_TINT	(1<<8)
-#define TCSR_PWMA	(1<<9)
-#define TCSR_ENALL	(1<<10)
-
-static unsigned int (*read_fn)(void __iomem *);
-static void (*write_fn)(u32, void __iomem *);
-
-static void timer_write32(u32 val, void __iomem *addr)
-{
-	iowrite32(val, addr);
-}
-
-static unsigned int timer_read32(void __iomem *addr)
-{
-	return ioread32(addr);
-}
-
-static void timer_write32_be(u32 val, void __iomem *addr)
-{
-	iowrite32be(val, addr);
-}
-
-static unsigned int timer_read32_be(void __iomem *addr)
-{
-	return ioread32be(addr);
-}
-
-static inline void xilinx_timer0_stop(void)
-{
-	write_fn(read_fn(timer_baseaddr + TCSR0) & ~TCSR_ENT,
-		 timer_baseaddr + TCSR0);
-}
-
-static inline void xilinx_timer0_start_periodic(unsigned long load_val)
-{
-	if (!load_val)
-		load_val = 1;
-	/* loading value to timer reg */
-	write_fn(load_val, timer_baseaddr + TLR0);
-
-	/* load the initial value */
-	write_fn(TCSR_LOAD, timer_baseaddr + TCSR0);
-
-	/* see timer data sheet for detail
-	 * !ENALL - don't enable 'em all
-	 * !PWMA - disable pwm
-	 * TINT - clear interrupt status
-	 * ENT- enable timer itself
-	 * ENIT - enable interrupt
-	 * !LOAD - clear the bit to let go
-	 * ARHT - auto reload
-	 * !CAPT - no external trigger
-	 * !GENT - no external signal
-	 * UDT - set the timer as down counter
-	 * !MDT0 - generate mode
-	 */
-	write_fn(TCSR_TINT|TCSR_ENIT|TCSR_ENT|TCSR_ARHT|TCSR_UDT,
-		 timer_baseaddr + TCSR0);
-}
-
-static inline void xilinx_timer0_start_oneshot(unsigned long load_val)
-{
-	if (!load_val)
-		load_val = 1;
-	/* loading value to timer reg */
-	write_fn(load_val, timer_baseaddr + TLR0);
-
-	/* load the initial value */
-	write_fn(TCSR_LOAD, timer_baseaddr + TCSR0);
-
-	write_fn(TCSR_TINT|TCSR_ENIT|TCSR_ENT|TCSR_ARHT|TCSR_UDT,
-		 timer_baseaddr + TCSR0);
-}
-
-static int xilinx_timer_set_next_event(unsigned long delta,
-					struct clock_event_device *dev)
-{
-	pr_debug("%s: next event, delta %x\n", __func__, (u32)delta);
-	xilinx_timer0_start_oneshot(delta);
-	return 0;
-}
-
-static int xilinx_timer_shutdown(struct clock_event_device *evt)
-{
-	pr_info("%s\n", __func__);
-	xilinx_timer0_stop();
-	return 0;
-}
-
-static int xilinx_timer_set_periodic(struct clock_event_device *evt)
-{
-	pr_info("%s\n", __func__);
-	xilinx_timer0_start_periodic(freq_div_hz);
-	return 0;
-}
-
-static struct clock_event_device clockevent_xilinx_timer = {
-	.name			= "xilinx_clockevent",
-	.features		= CLOCK_EVT_FEAT_ONESHOT |
-				  CLOCK_EVT_FEAT_PERIODIC,
-	.shift			= 8,
-	.rating			= 300,
-	.set_next_event		= xilinx_timer_set_next_event,
-	.set_state_shutdown	= xilinx_timer_shutdown,
-	.set_state_periodic	= xilinx_timer_set_periodic,
-};
-
-static inline void timer_ack(void)
-{
-	write_fn(read_fn(timer_baseaddr + TCSR0), timer_baseaddr + TCSR0);
-}
-
-static irqreturn_t timer_interrupt(int irq, void *dev_id)
-{
-	struct clock_event_device *evt = &clockevent_xilinx_timer;
-	timer_ack();
-	evt->event_handler(evt);
-	return IRQ_HANDLED;
-}
-
-static __init int xilinx_clockevent_init(void)
-{
-	clockevent_xilinx_timer.mult =
-		div_sc(timer_clock_freq, NSEC_PER_SEC,
-				clockevent_xilinx_timer.shift);
-	clockevent_xilinx_timer.max_delta_ns =
-		clockevent_delta2ns((u32)~0, &clockevent_xilinx_timer);
-	clockevent_xilinx_timer.max_delta_ticks = (u32)~0;
-	clockevent_xilinx_timer.min_delta_ns =
-		clockevent_delta2ns(1, &clockevent_xilinx_timer);
-	clockevent_xilinx_timer.min_delta_ticks = 1;
-	clockevent_xilinx_timer.cpumask = cpumask_of(0);
-	clockevents_register_device(&clockevent_xilinx_timer);
-
-	return 0;
-}
-
-static u64 xilinx_clock_read(void)
-{
-	return read_fn(timer_baseaddr + TCR1);
-}
-
-static u64 xilinx_read(struct clocksource *cs)
-{
-	/* reading actual value of timer 1 */
-	return (u64)xilinx_clock_read();
-}
-
-static struct timecounter xilinx_tc = {
-	.cc = NULL,
-};
-
-static u64 xilinx_cc_read(const struct cyclecounter *cc)
-{
-	return xilinx_read(NULL);
-}
-
-static struct cyclecounter xilinx_cc = {
-	.read = xilinx_cc_read,
-	.mask = CLOCKSOURCE_MASK(32),
-	.shift = 8,
-};
-
-static int __init init_xilinx_timecounter(void)
-{
-	xilinx_cc.mult = div_sc(timer_clock_freq, NSEC_PER_SEC,
-				xilinx_cc.shift);
-
-	timecounter_init(&xilinx_tc, &xilinx_cc, sched_clock());
-
-	return 0;
-}
-
-static struct clocksource clocksource_microblaze = {
-	.name		= "xilinx_clocksource",
-	.rating		= 300,
-	.read		= xilinx_read,
-	.mask		= CLOCKSOURCE_MASK(32),
-	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
-};
-
-static int __init xilinx_clocksource_init(void)
-{
-	int ret;
-
-	ret = clocksource_register_hz(&clocksource_microblaze,
-				      timer_clock_freq);
-	if (ret) {
-		pr_err("failed to register clocksource");
-		return ret;
-	}
-
-	/* stop timer1 */
-	write_fn(read_fn(timer_baseaddr + TCSR1) & ~TCSR_ENT,
-		 timer_baseaddr + TCSR1);
-	/* start timer1 - up counting without interrupt */
-	write_fn(TCSR_TINT|TCSR_ENT|TCSR_ARHT, timer_baseaddr + TCSR1);
-
-	/* register timecounter - for ftrace support */
-	return init_xilinx_timecounter();
-}
-
-static int __init xilinx_timer_init(struct device_node *timer)
-{
-	struct clk *clk;
-	static int initialized;
-	u32 irq;
-	u32 timer_num = 1;
-	int ret;
-
-	if (initialized)
-		return -EINVAL;
-
-	initialized = 1;
-
-	timer_baseaddr = of_iomap(timer, 0);
-	if (!timer_baseaddr) {
-		pr_err("ERROR: invalid timer base address\n");
-		return -ENXIO;
-	}
-
-	write_fn = timer_write32;
-	read_fn = timer_read32;
-
-	write_fn(TCSR_MDT, timer_baseaddr + TCSR0);
-	if (!(read_fn(timer_baseaddr + TCSR0) & TCSR_MDT)) {
-		write_fn = timer_write32_be;
-		read_fn = timer_read32_be;
-	}
-
-	irq = irq_of_parse_and_map(timer, 0);
-	if (irq <= 0) {
-		pr_err("Failed to parse and map irq");
-		return -EINVAL;
-	}
-
-	of_property_read_u32(timer, "xlnx,one-timer-only", &timer_num);
-	if (timer_num) {
-		pr_err("Please enable two timers in HW\n");
-		return -EINVAL;
-	}
-
-	pr_info("%pOF: irq=%d\n", timer, irq);
-
-	clk = of_clk_get(timer, 0);
-	if (IS_ERR(clk)) {
-		pr_err("ERROR: timer CCF input clock not found\n");
-		/* If there is clock-frequency property than use it */
-		of_property_read_u32(timer, "clock-frequency",
-				    &timer_clock_freq);
-	} else {
-		timer_clock_freq = clk_get_rate(clk);
-	}
-
-	if (!timer_clock_freq) {
-		pr_err("ERROR: Using CPU clock frequency\n");
-		timer_clock_freq = cpuinfo.cpu_clock_freq;
-	}
-
-	freq_div_hz = timer_clock_freq / HZ;
-
-	ret = request_irq(irq, timer_interrupt, IRQF_TIMER, "timer",
-			  &clockevent_xilinx_timer);
-	if (ret) {
-		pr_err("Failed to setup IRQ");
-		return ret;
-	}
-
-	ret = xilinx_clocksource_init();
-	if (ret)
-		return ret;
-
-	ret = xilinx_clockevent_init();
-	if (ret)
-		return ret;
-
-	sched_clock_register(xilinx_clock_read, 32, timer_clock_freq);
-
-	return 0;
-}
-
-TIMER_OF_DECLARE(xilinx_timer, "xlnx,xps-timer-1.00.a",
-		       xilinx_timer_init);
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 39aa21d01e05..daa9d55927ec 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -693,4 +693,16 @@ config MICROCHIP_PIT64B
 	  modes and high resolution. It is used as a clocksource
 	  and a clockevent.
 
+config XILINX_TIMER
+	bool "Xilinx AXI Timer support"
+	depends on COMMON_CLK
+	select REGMAP_MMIO
+	default y if MICROBLAZE
+	help
+	  Clocksource/clockevent 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.
+
 endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index c17ee32a7151..a7cba6ef5782 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -88,3 +88,4 @@ obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
 obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
 obj-$(CONFIG_HYPERV_TIMER)		+= hyperv_timer.o
 obj-$(CONFIG_MICROCHIP_PIT64B)		+= timer-microchip-pit64b.o
+obj-$(CONFIG_XILINX_TIMER)		+= timer-xilinx.o timer-xilinx-common.o
diff --git a/drivers/clocksource/timer-xilinx-common.c b/drivers/clocksource/timer-xilinx-common.c
new file mode 100644
index 000000000000..8873a1429954
--- /dev/null
+++ b/drivers/clocksource/timer-xilinx-common.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
+ *
+ * For Xilinx LogiCORE IP AXI Timer documentation, refer to DS764:
+ * https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
+ */
+
+#include <clocksource/timer-xilinx.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+
+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;
+	else
+		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 div_u64(cycles * NSEC_PER_SEC, clk_get_rate(priv->clk));
+}
+
+int xilinx_timer_common_init(struct device_node *np,
+			     struct xilinx_timer_priv *priv,
+			     u32 *one_timer)
+{
+	int ret;
+	u32 width;
+
+	ret = of_property_read_u32(np, "xlnx,one-timer-only", one_timer);
+	if (ret) {
+		pr_err("%pOF: err %d: xlnx,one-timer-only\n", np, ret);
+		return ret;
+	} else if (*one_timer && *one_timer != 1) {
+		pr_err("%pOF: xlnx,one-timer-only must be 0 or 1\n", np);
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32(np, "xlnx,count-width", &width);
+	if (ret == -EINVAL) {
+		width = 32;
+	} else if (ret) {
+		pr_err("%pOF: err %d: xlnx,count-width\n", np, ret);
+		return ret;
+	} else if (width < 8 || width > 32) {
+		pr_err("%pOF: invalid counter width\n", np);
+		return -EINVAL;
+	}
+	priv->max = BIT_ULL(width) - 1;
+
+	return 0;
+}
diff --git a/drivers/clocksource/timer-xilinx.c b/drivers/clocksource/timer-xilinx.c
new file mode 100644
index 000000000000..71dafc7b764c
--- /dev/null
+++ b/drivers/clocksource/timer-xilinx.c
@@ -0,0 +1,331 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
+ *
+ * Limitations:
+ * - When in cascade mode we cannot read the full 64-bit counter in one go
+ */
+
+#include <clocksource/timer-xilinx.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/log2.h>
+#include <linux/interrupt.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+#include <linux/sched_clock.h>
+#if IS_ENABLED(CONFIG_MICROBLAZE)
+#include <asm/cpuinfo.h>
+#endif
+
+struct xilinx_clocksource_device {
+	struct clocksource cs;
+	struct xilinx_timer_priv priv;
+};
+
+static inline struct xilinx_timer_priv
+*xilinx_clocksource_to_priv(struct clocksource *cs)
+{
+	return &container_of(cs, struct xilinx_clocksource_device, cs)->priv;
+}
+
+static u64 xilinx_clocksource_read(struct clocksource *cs)
+{
+	struct xilinx_timer_priv *priv = xilinx_clocksource_to_priv(cs);
+	unsigned int ret;
+
+	regmap_read(priv->map, TCR0, &ret);
+	return ret;
+}
+
+static struct xilinx_clocksource_device xilinx_clocksource = {
+	.cs = {
+		.name = "xilinx_clocksource",
+		.rating = 300,
+		.read = xilinx_clocksource_read,
+		.flags = CLOCK_SOURCE_IS_CONTINUOUS,
+		.owner = THIS_MODULE,
+	},
+};
+
+static u64 xilinx_sched_read(void)
+{
+	return xilinx_clocksource_read(&xilinx_clocksource.cs);
+}
+
+static int xilinx_clocksource_init(struct device_node *np)
+{
+	int ret;
+	struct xilinx_timer_priv *priv = &xilinx_clocksource.priv;
+	static const struct reg_sequence init[] = {
+		REG_SEQ0(TLR0, 0),
+		REG_SEQ0(TCSR0, TCSR_LOAD | TCSR_TINT),
+		REG_SEQ0(TCSR0, TCSR_ARHT | TCSR_ENT),
+	};
+
+	ret = regmap_multi_reg_write(priv->map, init, ARRAY_SIZE(init));
+	if (ret)
+		return ret;
+
+	xilinx_clocksource.cs.mask = priv->max;
+	ret = clocksource_register_hz(&xilinx_clocksource.cs,
+				      clk_get_rate(priv->clk));
+	if (!ret)
+		sched_clock_register(xilinx_sched_read,
+				     ilog2((u64)priv->max + 1),
+				     clk_get_rate(priv->clk));
+	else
+		pr_err("%pOF: err %d: could not register clocksource\n",
+		       np, ret);
+	return ret;
+}
+
+struct xilinx_clockevent_device {
+	struct clock_event_device ce;
+	struct xilinx_timer_priv priv;
+};
+
+static inline struct xilinx_timer_priv
+*xilinx_clockevent_to_priv(struct clock_event_device *ce)
+{
+	return &container_of(ce, struct xilinx_clockevent_device, ce)->priv;
+}
+
+static irqreturn_t xilinx_timer_handler(int irq, void *p)
+{
+	struct xilinx_clockevent_device *dev = p;
+
+	if (regmap_test_bits(dev->priv.map, TCSR0, TCSR_TINT) <= 0)
+		return IRQ_NONE;
+
+	regmap_clear_bits(dev->priv.map, TCSR0, TCSR_TINT);
+	dev->ce.event_handler(&dev->ce);
+	return IRQ_HANDLED;
+}
+
+static int xilinx_clockevent_next_event(unsigned long evt,
+					struct clock_event_device *ce)
+{
+	struct xilinx_timer_priv *priv = xilinx_clockevent_to_priv(ce);
+	struct reg_sequence next[] = {
+		REG_SEQ0(TLR0, evt - 2),
+		REG_SEQ0(TCSR0, TCSR_LOAD),
+		REG_SEQ0(TCSR0, TCSR_ENIT | TCSR_ENT),
+	};
+
+	return regmap_multi_reg_write(priv->map, next, ARRAY_SIZE(next));
+}
+
+static int xilinx_clockevent_state_periodic(struct clock_event_device *ce)
+{
+	struct xilinx_timer_priv *priv = xilinx_clockevent_to_priv(ce);
+	struct reg_sequence periodic[] = {
+		REG_SEQ0(TLR0, 0),
+		REG_SEQ0(TCSR0, TCSR_LOAD),
+		REG_SEQ0(TCSR0, TCSR_ARHT | TCSR_ENIT | TCSR_ENT),
+	};
+	unsigned long cycles = clk_get_rate(priv->clk) / HZ;
+
+	if (cycles < 2 || cycles - 2 > priv->max)
+		return -ERANGE;
+	periodic[0].def = xilinx_timer_tlr_cycles(priv, 0, cycles);
+
+	return regmap_multi_reg_write(priv->map, periodic, ARRAY_SIZE(periodic));
+}
+
+static int xilinx_clockevent_shutdown(struct clock_event_device *ce)
+{
+	struct xilinx_timer_priv *priv = xilinx_clockevent_to_priv(ce);
+
+	return regmap_write(priv->map, TCSR0, 0);
+}
+
+static const struct clock_event_device xilinx_clockevent_base = {
+	.rating = 300,
+	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_next_event = xilinx_clockevent_next_event,
+	.set_state_periodic = xilinx_clockevent_state_periodic,
+	.set_state_shutdown = xilinx_clockevent_shutdown,
+	.cpumask = cpu_possible_mask,
+	.owner = THIS_MODULE,
+};
+
+static const struct regmap_config xilinx_pwm_regmap_config = {
+	/* Actually 5, but I don't think this matters for mmio */
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+};
+
+static int xilinx_clockevent_init(struct device_node *np,
+				  struct xilinx_clockevent_device *dev)
+{
+	int irq, ret;
+
+	irq = ret = of_irq_get(np, 0);
+	if (ret < 0) {
+		pr_err("%pOF: err %d: could not get irq\n", np, ret);
+		return ret;
+	}
+
+	ret = request_irq(irq, xilinx_timer_handler, IRQF_TIMER,
+			  of_node_full_name(np), dev);
+	if (ret) {
+		pr_err("%pOF: err %d: could not request irq\n", np, ret);
+		return ret;
+	}
+
+	memcpy(&dev->ce, &xilinx_clockevent_base, sizeof(dev->ce));
+	dev->ce.name = of_node_full_name(np);
+	clockevents_config_and_register(&dev->ce, clk_get_rate(dev->priv.clk), 2,
+					min_t(u64, (u64)dev->priv.max + 2,
+					      ULONG_MAX));
+	return 0;
+}
+
+static const struct regmap_config xilinx_timer_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.max_register = TCR0,
+};
+
+static bool clocksource_uninitialized = true;
+
+static int __init xilinx_timer_init(struct device_node *np)
+{
+	bool artificial_clock = false;
+	int ret;
+	struct xilinx_timer_priv *priv;
+	struct xilinx_clockevent_device *dev;
+	u32 one_timer;
+	void __iomem *regs;
+
+	if (of_property_read_bool(np, "#pwm-cells"))
+		return 0;
+
+	regs = of_iomap(np, 0);
+	if (IS_ERR(regs)) {
+		ret = PTR_ERR(regs);
+		pr_err("%pOF: err %d: failed to map regs\n", np, ret);
+		return ret;
+	}
+
+	if (clocksource_uninitialized) {
+		priv = &xilinx_clocksource.priv;
+	} else {
+		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+		if (!dev) {
+			ret = -ENOMEM;
+			goto err_regs;
+		}
+		priv = &dev->priv;
+	}
+
+	priv->map = regmap_init_mmio(NULL, regs, &xilinx_timer_regmap_config);
+	if (IS_ERR(priv->map)) {
+		ret = PTR_ERR(priv->map);
+		goto err_priv;
+	}
+
+	ret = xilinx_timer_common_init(np, priv, &one_timer);
+	if (ret)
+		goto err_regmap;
+
+	priv->clk = of_clk_get_by_name(np, "s_axi_aclk");
+	if (IS_ERR(priv->clk)) {
+		u32 freq;
+
+		ret = PTR_ERR(priv->clk);
+		if (ret == -EPROBE_DEFER)
+			goto err_regs;
+
+		pr_warn("%pOF: missing s_axi_aclk, falling back to clock-frequency\n",
+			np);
+		ret = of_property_read_u32(np, "clock-frequency", &freq);
+		if (ret) {
+#if IS_ENABLED(CONFIG_MICROBLAZE)
+			pr_warn("%pOF: missing clock-frequency, falling back to /cpus/timebase-frequency\n",
+				np);
+			freq = cpuinfo.cpu_clock_freq;
+#else
+			goto err_regs;
+#endif
+		}
+
+		priv->clk = clk_register_fixed_rate(NULL, of_node_full_name(np),
+						    NULL, 0, freq);
+		if (IS_ERR(priv->clk)) {
+			ret = PTR_ERR(priv->clk);
+			goto err_regs;
+		}
+		artificial_clock = true;
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret) {
+		pr_err("%pOF: err %d: clock enable failed\n", np, ret);
+		goto err_clk_init;
+	}
+	clk_rate_exclusive_get(priv->clk);
+
+	if (clocksource_uninitialized) {
+		ret = xilinx_clocksource_init(np);
+		if (ret)
+			goto err_clk_enable;
+		clocksource_uninitialized = false;
+	} else {
+		ret = xilinx_clockevent_init(np, dev);
+		if (ret)
+			goto err_clk_enable;
+	}
+	of_node_set_flag(np, OF_POPULATED);
+
+	if (!one_timer) {
+		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+		if (!dev)
+			return -ENOMEM;
+
+		/*
+		 * We don't support removal, so don't bother enabling
+		 * the clock twice.
+		 */
+		memcpy(&dev->priv, priv, sizeof(dev->priv));
+		dev->priv.map = regmap_init_mmio(NULL, regs + TCSR1,
+						 &xilinx_timer_regmap_config);
+		if (!IS_ERR(dev->priv.map)) {
+			ret = xilinx_clockevent_init(np, dev);
+			if (!ret)
+				return 0;
+			regmap_exit(dev->priv.map);
+		} else {
+			ret = PTR_ERR(dev->priv.map);
+		}
+		kfree(dev);
+	}
+	return ret;
+
+err_clk_enable:
+	clk_rate_exclusive_put(priv->clk);
+	clk_disable_unprepare(priv->clk);
+err_clk_init:
+	if (artificial_clock)
+		clk_unregister_fixed_rate(priv->clk);
+	else
+		clk_put(priv->clk);
+err_regmap:
+	regmap_exit(priv->map);
+err_priv:
+	if (!clocksource_uninitialized)
+		kfree(dev);
+err_regs:
+	iounmap(regs);
+	return ret;
+}
+
+TIMER_OF_DECLARE(xilinx_xps_timer, "xlnx,xps-timer-1.00.a", xilinx_timer_init);
diff --git a/include/clocksource/timer-xilinx.h b/include/clocksource/timer-xilinx.h
new file mode 100644
index 000000000000..1f7757b84a5e
--- /dev/null
+++ b/include/clocksource/timer-xilinx.h
@@ -0,0 +1,91 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
+ */
+
+#ifndef XILINX_TIMER_H
+#define XILINX_TIMER_H
+
+#include <linux/compiler.h>
+
+#define TCSR0	0x00
+#define TLR0	0x04
+#define TCR0	0x08
+#define TCSR1	0x10
+#define TLR1	0x14
+#define TCR1	0x18
+
+#define TCSR_MDT	BIT(0)
+#define TCSR_UDT	BIT(1)
+#define TCSR_GENT	BIT(2)
+#define TCSR_CAPT	BIT(3)
+#define TCSR_ARHT	BIT(4)
+#define TCSR_LOAD	BIT(5)
+#define TCSR_ENIT	BIT(6)
+#define TCSR_ENT	BIT(7)
+#define TCSR_TINT	BIT(8)
+#define TCSR_PWMA	BIT(9)
+#define TCSR_ENALL	BIT(10)
+#define TCSR_CASC	BIT(11)
+
+struct clk;
+struct device_node;
+struct regmap;
+
+/**
+ * struct xilinx_timer_priv - Private data for Xilinx AXI timer drivers
+ * @map: Regmap of the device, possibly with an offset
+ * @clk: Parent clock
+ * @max: Maximum value of the counters
+ */
+struct xilinx_timer_priv {
+	struct regmap *map;
+	struct clk *clk;
+	u32 max;
+};
+
+/**
+ * xilinx_timer_tlr_cycles() - Calculate the TLR for a period specified
+ *                             in clock cycles
+ * @priv: The timer's private data
+ * @tcsr: The value of the TCSR register for this counter
+ * @cycles: The number of cycles in this period
+ *
+ * Callers of this function MUST ensure that @cycles is representable as
+ * a TLR.
+ *
+ * Return: The calculated value for TLR
+ */
+u32 xilinx_timer_tlr_cycles(struct xilinx_timer_priv *priv, u32 tcsr,
+			    u64 cycles);
+
+/**
+ * xilinx_timer_get_period() - Get the current period of a counter
+ * @priv: The timer's private data
+ * @tlr: The value of TLR for this counter
+ * @tcsr: The value of TCSR for this counter
+ *
+ * Return: The period, in ns
+ */
+unsigned int xilinx_timer_get_period(struct xilinx_timer_priv *priv,
+				     u32 tlr, u32 tcsr);
+
+/**
+ * xilinx_timer_common_init() - Perform common initialization for Xilinx
+ *                              AXI timer drivers.
+ * @priv: The timer's private data
+ * @np: The devicetree node for the timer
+ * @one_timer: Set to %1 if there is only one timer
+ *
+ * This performs common initialization, such as detecting endianness,
+ * and parsing devicetree properties. @priv->regs must be initialized
+ * before calling this function. This function initializes @priv->read,
+ * @priv->write, and @priv->width.
+ *
+ * Return: 0, or negative errno
+ */
+int xilinx_timer_common_init(struct device_node *np,
+			     struct xilinx_timer_priv *priv,
+			     u32 *one_timer);
+
+#endif /* XILINX_TIMER_H */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer
  2021-07-19 22:13 [PATCH v5 1/3] dt-bindings: pwm: Add Xilinx AXI Timer Sean Anderson
  2021-07-19 22:13 ` [PATCH v5 2/3] clocksource: Rewrite Xilinx AXI timer driver Sean Anderson
@ 2021-07-19 22:13 ` Sean Anderson
  2021-07-21 13:26   ` kernel test robot
                     ` (2 more replies)
  2021-07-20 13:11 ` [PATCH v5 1/3] dt-bindings: pwm: Add " Rob Herring
  2 siblings, 3 replies; 15+ messages in thread
From: Sean Anderson @ 2021-07-19 22:13 UTC (permalink / raw)
  To: linux-pwm, devicetree, Thierry Reding
  Cc: Alvaro Gamez, Uwe Kleine-König, michal.simek, Lee Jones,
	linux-arm-kernel, linux-kernel, Sean Anderson

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.

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@seco.com>
---
Uwe, I know we haven't finished our discussion on the previous series
wrt what checks/rounding apply_state should do, but I want to get some
feedback on the other changes I've made to this series in the meantime.

Changes in v5:
- Elaborate on limitation section
- Rework duty-cycle and period calculations with feedback from Uwe
- Use more verbose error messages
- Allow non-zero #pwm-cells
- Remove xlnx,axi-timer-2.0 compatible string
- Correctly set duty_cycle in get_state when TLR0=TLR1
- Perform some additional checks/rounding in apply_state
- Switch to regmap to abstract endianness issues

Changes in v4:
- Remove references to properties which are not good enough for Linux.
- Don't use volatile in read/write replacements. Some arches have it and
  some don't.
- Put common timer properties into their own struct to better reuse
  code.

Changes in v3:
- Add clockevent and clocksource support
- Rewrite probe to only use a device_node, since timers may need to be
  initialized before we have proper devices. This does bloat the code a bit
  since we can no longer rely on helpers such as dev_err_probe. We also
  cannot rely on device resources being free'd on failure, so we must free
  them manually.
- We now access registers through xilinx_timer_(read|write). This allows us
  to deal with endianness issues, as originally seen in the microblaze
  driver. CAVEAT EMPTOR: I have not tested this on big-endian!
- Remove old microblaze driver

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

 MAINTAINERS                  |   1 +
 drivers/clocksource/Makefile |   5 +-
 drivers/pwm/Kconfig          |  13 ++
 drivers/pwm/Makefile         |   1 +
 drivers/pwm/pwm-xilinx.c     | 268 +++++++++++++++++++++++++++++++++++
 5 files changed, 287 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pwm/pwm-xilinx.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 252d71addd18..6adafd3e7a09 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20050,6 +20050,7 @@ XILINX TIMER/PWM DRIVER
 M:	Sean Anderson <sean.anderson@seco.com>
 S:	Maintained
 F:	drivers/clocksource/timer-xilinx*
+F:	drivers/pwm/pwm-xilinx.c
 F:	include/clocksource/timer-xilinx.h
 
 XILINX UARTLITE SERIAL DRIVER
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index a7cba6ef5782..dcf434fdb726 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -88,4 +88,7 @@ obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
 obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
 obj-$(CONFIG_HYPERV_TIMER)		+= hyperv_timer.o
 obj-$(CONFIG_MICROCHIP_PIT64B)		+= timer-microchip-pit64b.o
-obj-$(CONFIG_XILINX_TIMER)		+= timer-xilinx.o timer-xilinx-common.o
+obj-$(CONFIG_XILINX_TIMER)		+= timer-xilinx.o
+ifneq ($(CONFIG_XILINX_TIMER$)(CONFIG_PWM_XILINX),)
+obj-y					+= timer-xilinx-common.o
+endif
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index c76adedd58c9..974774b7c987 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -637,4 +637,17 @@ 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 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..291577be992f
--- /dev/null
+++ b/drivers/pwm/pwm-xilinx.c
@@ -0,0 +1,268 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
+ *
+ * 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 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/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+/*
+ * 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)
+{
+	bool enabled;
+	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.
+	 */
+	rate = clk_get_rate(priv->clk);
+	period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC);
+	if (period_cycles - 2 > priv->max || period_cycles < 2)
+		return -ERANGE;
+	duty_cycles = mul_u64_u32_div(state->duty_cycle, rate, NSEC_PER_SEC);
+
+	/*
+	 * If we specify 100% duty cycle, we will get 0% instead, so decrease
+	 * the duty cycle count by one.
+	 */
+	if (period_cycles == duty_cycles)
+		duty_cycles--;
+
+	/* Round down to 0% duty cycle for unrepresentable duty cycles */
+	if (duty_cycles < 2)
+		duty_cycles = period_cycles;
+
+	regmap_read(priv->map, TCSR0, &tcsr0);
+	regmap_read(priv->map, TCSR1, &tcsr1);
+	tlr0 = xilinx_timer_tlr_cycles(priv, tcsr0, period_cycles);
+	tlr1 = xilinx_timer_tlr_cycles(priv, tcsr1, duty_cycles);
+	regmap_write(priv->map, TLR0, tlr0);
+	regmap_write(priv->map, TLR1, tlr1);
+
+	enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
+	if (state->enabled) {
+		/*
+		 * If the PWM is already running, then the counters will be
+		 * reloaded at the end of the current cycle.
+		 */
+		if (!enabled) {
+			/* Load TLR into TCR */
+			regmap_write(priv->map, TCSR0, tcsr0 | TCSR_LOAD);
+			regmap_write(priv->map, TCSR1, tcsr1 | TCSR_LOAD);
+			/* Enable timers all at once with ENALL */
+			tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
+			tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
+			regmap_write(priv->map, TCSR0, tcsr0);
+			regmap_write(priv->map, TCSR1, tcsr1);
+		}
+	} else {
+		regmap_write(priv->map, TCSR0, 0);
+		regmap_write(priv->map, TCSR1, 0);
+	}
+
+	return 0;
+}
+
+static void xilinx_pwm_get_state(struct pwm_chip *chip,
+				 struct pwm_device *unused,
+				 struct pwm_state *state)
+{
+	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
+	u32 tlr0, tlr1, tcsr0, tcsr1;
+
+	regmap_read(priv->map, TLR0, &tlr0);
+	regmap_read(priv->map, TLR1, &tlr1);
+	regmap_read(priv->map, TCSR0, &tcsr0);
+	regmap_read(priv->map, TCSR1, &tcsr1);
+	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
+	state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
+	state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
+	state->polarity = PWM_POLARITY_NORMAL;
+
+	/* 100% duty cycle results in constant low output */
+	if (state->period == state->duty_cycle)
+		state->duty_cycle = 0;
+}
+
+static const struct pwm_ops xilinx_pwm_ops = {
+	.apply = xilinx_pwm_apply,
+	.get_state = xilinx_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static const struct regmap_config xilinx_pwm_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.max_register = TCR1,
+	.disable_locking = true, /* The PWM subsystem handles locking */
+};
+
+static int xilinx_timer_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct xilinx_timer_priv *priv;
+	struct xilinx_pwm_device *pwm;
+	u32 pwm_cells, one_timer;
+	void __iomem *regs;
+
+	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
+	if (ret == -EINVAL)
+		return -ENODEV;
+	else if (ret)
+		return dev_err_probe(dev, ret, "could not read #pwm-cells\n");
+
+	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, pwm);
+	priv = &pwm->priv;
+
+	regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	priv->map = devm_regmap_init_mmio(dev, regs,
+					  &xilinx_pwm_regmap_config);
+	if (IS_ERR(priv->map))
+		return dev_err_probe(dev, PTR_ERR(priv->map),
+				     "could not create regmap\n");
+
+	ret = xilinx_timer_common_init(np, priv, &one_timer);
+	if (ret)
+		return ret;
+
+	if (one_timer)
+		return dev_err_probe(dev, -EINVAL,
+				     "two timers required for PWM mode\n");
+
+	/*
+	 * The polarity of the generate outputs must be active high for PWM
+	 * mode to work. We could determine this from the device tree, but
+	 * alas, such properties are not allowed to be used.
+	 */
+
+	priv->clk = devm_clk_get(dev, "s_axi_aclk");
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk),
+				     "could not get clock\n");
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "clock enable failed\n");
+	clk_rate_exclusive_get(priv->clk);
+
+	pwm->chip.dev = dev;
+	pwm->chip.ops = &xilinx_pwm_ops;
+	pwm->chip.npwm = 1;
+	ret = pwmchip_add(&pwm->chip);
+	if (ret) {
+		clk_rate_exclusive_put(priv->clk);
+		clk_disable_unprepare(priv->clk);
+		return dev_err_probe(dev, ret, "could not register pwm chip\n");
+	}
+
+	return 0;
+}
+
+static int xilinx_timer_remove(struct platform_device *pdev)
+{
+	struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&pwm->chip);
+	clk_rate_exclusive_put(pwm->priv.clk);
+	clk_disable_unprepare(pwm->priv.clk);
+	return 0;
+}
+
+static const struct of_device_id xilinx_timer_of_match[] = {
+	{ .compatible = "xlnx,xps-timer-1.00.a", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xilinx_timer_of_match);
+
+static struct platform_driver xilinx_timer_driver = {
+	.probe = xilinx_timer_probe,
+	.remove = xilinx_timer_remove,
+	.driver = {
+		.name = "xilinx-timer",
+		.of_match_table = of_match_ptr(xilinx_timer_of_match),
+	},
+};
+module_platform_driver(xilinx_timer_driver);
+
+MODULE_ALIAS("platform:xilinx-timer");
+MODULE_DESCRIPTION("Xilinx LogiCORE IP AXI Timer driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 1/3] dt-bindings: pwm: Add Xilinx AXI Timer
  2021-07-19 22:13 [PATCH v5 1/3] dt-bindings: pwm: Add Xilinx AXI Timer Sean Anderson
  2021-07-19 22:13 ` [PATCH v5 2/3] clocksource: Rewrite Xilinx AXI timer driver Sean Anderson
  2021-07-19 22:13 ` [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer Sean Anderson
@ 2021-07-20 13:11 ` Rob Herring
  2021-07-20 21:45   ` Sean Anderson
  2 siblings, 1 reply; 15+ messages in thread
From: Rob Herring @ 2021-07-20 13:11 UTC (permalink / raw)
  To: Sean Anderson
  Cc: Lee Jones, Alvaro Gamez, linux-kernel, Uwe Kleine-König,
	linux-arm-kernel, Thierry Reding, devicetree, michal.simek,
	linux-pwm

On Mon, 19 Jul 2021 18:13:20 -0400, Sean Anderson wrote:
> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is a
> "soft" block, so it has some parameters which would not be configurable in
> most hardware. This binding is usually automatically generated by Xilinx's
> tools, so the names and values of some properties should be kept as they
> are, if possible. In addition, this binding is already in the kernel at
> arch/microblaze/boot/dts/system.dts, and in user software such as QEMU.
> 
> The existing driver uses the clock-frequency property, or alternatively the
> /cpus/timebase-frequency property as its frequency input. Because these
> properties are deprecated, they have not been included with this schema.
> All new bindings should use the clocks/clock-names properties to specify
> the parent clock.
> 
> Because we need to init timer devices so early in boot, we determine if we
> should use the PWM driver or the clocksource/clockevent driver by the
> presence/absence, respectively, of #pwm-cells. Because both counters are
> used by the PWM, there is no need for a separate property specifying which
> counters are to be used for the PWM.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v5:
> - Update commit message to reflect revisions
> - Fix indentation lint
> - Add example for timer binding
> - Remove xlnx,axi-timer-2.0 compatible string
> - Move schema into the timer directory
> 
> Changes in v4:
> - Remove references to generate polarity so this can get merged
> - Predicate PWM driver on the presence of #pwm-cells
> - Make some properties optional for clocksource drivers
> 
> Changes in v3:
> - Mark all boolean-as-int properties as deprecated
> - Add xlnx,pwm and xlnx,gen?-active-low properties.
> - Make newer replacement properties mutually-exclusive with what they
>   replace
> - Add an example with non-deprecated properties only.
> 
> Changes in v2:
> - Use 32-bit addresses for example binding
> 
>  .../bindings/timer/xlnx,xps-timer.yaml        | 91 +++++++++++++++++++
>  1 file changed, 91 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/timer/xlnx,xps-timer.yaml#
\ndoc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1507329

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/3] clocksource: Rewrite Xilinx AXI timer driver
  2021-07-19 22:13 ` [PATCH v5 2/3] clocksource: Rewrite Xilinx AXI timer driver Sean Anderson
@ 2021-07-20 21:41   ` kernel test robot
  2021-07-20 21:44     ` Sean Anderson
  2021-07-21  2:35   ` kernel test robot
  1 sibling, 1 reply; 15+ messages in thread
From: kernel test robot @ 2021-07-20 21:41 UTC (permalink / raw)
  To: Sean Anderson, linux-pwm, devicetree, Thierry Reding
  Cc: kbuild-all, Alvaro Gamez, Uwe Kleine-König, michal.simek,
	Lee Jones, linux-arm-kernel, linux-kernel, Sean Anderson

[-- Attachment #1: Type: text/plain, Size: 2510 bytes --]

Hi Sean,

I love your patch! Perhaps something to improve:

[auto build test WARNING on tip/timers/core]
[also build test WARNING on pwm/for-next linus/master v5.14-rc2 next-20210720]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 2d0a9eb23ccfdf11308bec6db0bc007585d919d2
config: microblaze-randconfig-r003-20210720 (attached as .config)
compiler: microblaze-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/94fdaff6f9a931388dfbeecb76ea7b50eaa06711
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903
        git checkout 94fdaff6f9a931388dfbeecb76ea7b50eaa06711
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=microblaze SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> WARNING: modpost: vmlinux.o(.text.unlikely+0x530e8): Section mismatch in reference from the function xilinx_clocksource_init() to the function .init.text:sched_clock_register()
The function xilinx_clocksource_init() references
the function __init sched_clock_register().
This is often because xilinx_clocksource_init lacks a __init
annotation or the annotation of sched_clock_register is wrong.

The below error/warnings are from parent commit:
<< WARNING: modpost: vmlinux.o(.text+0xc650): Section mismatch in reference from the function pcibios_allocate_bus_resources() to the function .init.text:reparent_resources()
<< WARNING: modpost: vmlinux.o(.text.unlikely+0x1698): Section mismatch in reference from the function mm_cmdline_setup() to the variable .meminit.data:memblock

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31307 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/3] clocksource: Rewrite Xilinx AXI timer driver
  2021-07-20 21:41   ` kernel test robot
@ 2021-07-20 21:44     ` Sean Anderson
  0 siblings, 0 replies; 15+ messages in thread
From: Sean Anderson @ 2021-07-20 21:44 UTC (permalink / raw)
  To: kernel test robot, linux-pwm, devicetree, Thierry Reding
  Cc: kbuild-all, Alvaro Gamez, Uwe Kleine-König, michal.simek,
	Lee Jones, linux-arm-kernel, linux-kernel



On 7/20/21 5:41 PM, kernel test robot wrote:
> Hi Sean,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on tip/timers/core]
> [also build test WARNING on pwm/for-next linus/master v5.14-rc2 next-20210720]
> [cannot apply to xlnx/master]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 2d0a9eb23ccfdf11308bec6db0bc007585d919d2
> config: microblaze-randconfig-r003-20210720 (attached as .config)
> compiler: microblaze-linux-gcc (GCC) 10.3.0
> reproduce (this is a W=1 build):
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # https://github.com/0day-ci/linux/commit/94fdaff6f9a931388dfbeecb76ea7b50eaa06711
>          git remote add linux-review https://github.com/0day-ci/linux
>          git fetch --no-tags linux-review Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903
>          git checkout 94fdaff6f9a931388dfbeecb76ea7b50eaa06711
>          # save the attached .config to linux build tree
>          mkdir build_dir
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=microblaze SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>, old ones prefixed by <<):
> 
>>> WARNING: modpost: vmlinux.o(.text.unlikely+0x530e8): Section mismatch in reference from the function xilinx_clocksource_init() to the function .init.text:sched_clock_register()
> The function xilinx_clocksource_init() references
> the function __init sched_clock_register().
> This is often because xilinx_clocksource_init lacks a __init
> annotation or the annotation of sched_clock_register is wrong.

Will fix in v6.

> 
> The below error/warnings are from parent commit:
> << WARNING: modpost: vmlinux.o(.text+0xc650): Section mismatch in reference from the function pcibios_allocate_bus_resources() to the function .init.text:reparent_resources()
> << WARNING: modpost: vmlinux.o(.text.unlikely+0x1698): Section mismatch in reference from the function mm_cmdline_setup() to the variable .meminit.data:memblock
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 1/3] dt-bindings: pwm: Add Xilinx AXI Timer
  2021-07-20 13:11 ` [PATCH v5 1/3] dt-bindings: pwm: Add " Rob Herring
@ 2021-07-20 21:45   ` Sean Anderson
  0 siblings, 0 replies; 15+ messages in thread
From: Sean Anderson @ 2021-07-20 21:45 UTC (permalink / raw)
  To: Rob Herring
  Cc: Lee Jones, Alvaro Gamez, linux-kernel, Uwe Kleine-König,
	linux-arm-kernel, Thierry Reding, devicetree, michal.simek,
	linux-pwm



On 7/20/21 9:11 AM, Rob Herring wrote:
> On Mon, 19 Jul 2021 18:13:20 -0400, Sean Anderson wrote:
>> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is a
>> "soft" block, so it has some parameters which would not be configurable in
>> most hardware. This binding is usually automatically generated by Xilinx's
>> tools, so the names and values of some properties should be kept as they
>> are, if possible. In addition, this binding is already in the kernel at
>> arch/microblaze/boot/dts/system.dts, and in user software such as QEMU.
>> 
>> The existing driver uses the clock-frequency property, or alternatively the
>> /cpus/timebase-frequency property as its frequency input. Because these
>> properties are deprecated, they have not been included with this schema.
>> All new bindings should use the clocks/clock-names properties to specify
>> the parent clock.
>> 
>> Because we need to init timer devices so early in boot, we determine if we
>> should use the PWM driver or the clocksource/clockevent driver by the
>> presence/absence, respectively, of #pwm-cells. Because both counters are
>> used by the PWM, there is no need for a separate property specifying which
>> counters are to be used for the PWM.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>> Changes in v5:
>> - Update commit message to reflect revisions
>> - Fix indentation lint
>> - Add example for timer binding
>> - Remove xlnx,axi-timer-2.0 compatible string
>> - Move schema into the timer directory
>> 
>> Changes in v4:
>> - Remove references to generate polarity so this can get merged
>> - Predicate PWM driver on the presence of #pwm-cells
>> - Make some properties optional for clocksource drivers
>> 
>> Changes in v3:
>> - Mark all boolean-as-int properties as deprecated
>> - Add xlnx,pwm and xlnx,gen?-active-low properties.
>> - Make newer replacement properties mutually-exclusive with what they
>>   replace
>> - Add an example with non-deprecated properties only.
>> 
>> Changes in v2:
>> - Use 32-bit addresses for example binding
>> 
>>  .../bindings/timer/xlnx,xps-timer.yaml        | 91 +++++++++++++++++++
>>  1 file changed, 91 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
>> 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> ./Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml: $id: relative path/filename doesn't match actual path or filename
> 	expected: http://devicetree.org/schemas/timer/xlnx,xps-timer.yaml#
> \ndoc reference errors (make refcheckdocs):

Ah, looks like I neglected to re-run tests after moving this file. Will fix in v6.

--Sean

> 
> See https://patchwork.ozlabs.org/patch/1507329
> 
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit.
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/3] clocksource: Rewrite Xilinx AXI timer driver
  2021-07-19 22:13 ` [PATCH v5 2/3] clocksource: Rewrite Xilinx AXI timer driver Sean Anderson
  2021-07-20 21:41   ` kernel test robot
@ 2021-07-21  2:35   ` kernel test robot
  1 sibling, 0 replies; 15+ messages in thread
From: kernel test robot @ 2021-07-21  2:35 UTC (permalink / raw)
  To: Sean Anderson, linux-pwm, devicetree, Thierry Reding
  Cc: kbuild-all, Alvaro Gamez, Uwe Kleine-König, michal.simek,
	Lee Jones, linux-arm-kernel, linux-kernel, Sean Anderson

[-- Attachment #1: Type: text/plain, Size: 2846 bytes --]

Hi Sean,

I love your patch! Perhaps something to improve:

[auto build test WARNING on tip/timers/core]
[also build test WARNING on pwm/for-next linus/master v5.14-rc2 next-20210720]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 2d0a9eb23ccfdf11308bec6db0bc007585d919d2
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/94fdaff6f9a931388dfbeecb76ea7b50eaa06711
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903
        git checkout 94fdaff6f9a931388dfbeecb76ea7b50eaa06711
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=xtensa SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> WARNING: modpost: vmlinux.o(.text.unlikely+0x1cff34): Section mismatch in reference from the function mchp_pit64b_reset() to the function .init.text:sched_clock_register()
The function mchp_pit64b_reset() references
the function __init sched_clock_register().
This is often because mchp_pit64b_reset lacks a __init
annotation or the annotation of sched_clock_register is wrong.

The below error/warnings are from parent commit:
<< WARNING: modpost: vmlinux.o(.text.unlikely+0x6440): Section mismatch in reference from the function __trace_event_discard_commit() to the variable .init.data:initcall_level_names
<< WARNING: modpost: vmlinux.o(.data+0x15efaf0): Section mismatch in reference from the variable qed_mfw_legacy_maps to the variable .init.rodata:__setup_str_initcall_blacklist
<< WARNING: modpost: vmlinux.o(.data+0x15efc00): Section mismatch in reference from the variable qed_mfw_ext_maps to the variable .init.rodata:__setup_str_initcall_blacklist
<< WARNING: modpost: vmlinux.o(.data+0x15fa770): Section mismatch in reference from the variable qede_forced_speed_maps to the variable .init.rodata:__setup_str_initcall_blacklist

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67946 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer
  2021-07-19 22:13 ` [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer Sean Anderson
@ 2021-07-21 13:26   ` kernel test robot
  2021-07-22 19:18     ` Sean Anderson
  2021-08-09 14:48   ` Sean Anderson
  2021-08-14 20:47   ` Uwe Kleine-König
  2 siblings, 1 reply; 15+ messages in thread
From: kernel test robot @ 2021-07-21 13:26 UTC (permalink / raw)
  To: Sean Anderson, linux-pwm, devicetree, Thierry Reding
  Cc: kbuild-all, Alvaro Gamez, Uwe Kleine-König, michal.simek,
	Lee Jones, linux-arm-kernel, linux-kernel, Sean Anderson

[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]

Hi Sean,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/timers/core]
[also build test ERROR on pwm/for-next linus/master v5.14-rc2 next-20210720]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 2d0a9eb23ccfdf11308bec6db0bc007585d919d2
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/338d061c42411ef012c4dce517355fd1d61c1cab
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903
        git checkout 338d061c42411ef012c4dce517355fd1d61c1cab
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "xilinx_timer_tlr_cycles" [drivers/pwm/pwm-xilinx.ko] undefined!
>> ERROR: modpost: "xilinx_timer_common_init" [drivers/pwm/pwm-xilinx.ko] undefined!
>> ERROR: modpost: "xilinx_timer_get_period" [drivers/pwm/pwm-xilinx.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 60303 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer
  2021-07-21 13:26   ` kernel test robot
@ 2021-07-22 19:18     ` Sean Anderson
  0 siblings, 0 replies; 15+ messages in thread
From: Sean Anderson @ 2021-07-22 19:18 UTC (permalink / raw)
  To: kernel test robot, linux-pwm, devicetree, Thierry Reding
  Cc: kbuild-all, Alvaro Gamez, Uwe Kleine-König, michal.simek,
	Lee Jones, linux-arm-kernel, linux-kernel



On 7/21/21 9:26 AM, kernel test robot wrote:
> Hi Sean,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on tip/timers/core]
> [also build test ERROR on pwm/for-next linus/master v5.14-rc2 next-20210720]
> [cannot apply to xlnx/master]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 2d0a9eb23ccfdf11308bec6db0bc007585d919d2
> config: m68k-allmodconfig (attached as .config)
> compiler: m68k-linux-gcc (GCC) 10.3.0
> reproduce (this is a W=1 build):
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # https://github.com/0day-ci/linux/commit/338d061c42411ef012c4dce517355fd1d61c1cab
>          git remote add linux-review https://github.com/0day-ci/linux
>          git fetch --no-tags linux-review Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903
>          git checkout 338d061c42411ef012c4dce517355fd1d61c1cab
>          # save the attached .config to linux build tree
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=m68k
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>, old ones prefixed by <<):
> 
>>> ERROR: modpost: "xilinx_timer_tlr_cycles" [drivers/pwm/pwm-xilinx.ko] undefined!
>>> ERROR: modpost: "xilinx_timer_common_init" [drivers/pwm/pwm-xilinx.ko] undefined!
>>> ERROR: modpost: "xilinx_timer_get_period" [drivers/pwm/pwm-xilinx.ko] undefined!

Looks like I forgot my EXPORT_SYMBOLs. Will be fixed in v6.

--Sean

> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer
  2021-07-19 22:13 ` [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer Sean Anderson
  2021-07-21 13:26   ` kernel test robot
@ 2021-08-09 14:48   ` Sean Anderson
  2021-08-14 20:47   ` Uwe Kleine-König
  2 siblings, 0 replies; 15+ messages in thread
From: Sean Anderson @ 2021-08-09 14:48 UTC (permalink / raw)
  To: linux-pwm, devicetree, Thierry Reding
  Cc: Alvaro Gamez, Uwe Kleine-König, michal.simek, Lee Jones,
	linux-arm-kernel, linux-kernel

ping?

On 7/19/21 6:13 PM, 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.
> 
> 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@seco.com>
> ---
> Uwe, I know we haven't finished our discussion on the previous series
> wrt what checks/rounding apply_state should do, but I want to get some
> feedback on the other changes I've made to this series in the meantime.
> 
> Changes in v5:
> - Elaborate on limitation section
> - Rework duty-cycle and period calculations with feedback from Uwe
> - Use more verbose error messages
> - Allow non-zero #pwm-cells
> - Remove xlnx,axi-timer-2.0 compatible string
> - Correctly set duty_cycle in get_state when TLR0=TLR1
> - Perform some additional checks/rounding in apply_state
> - Switch to regmap to abstract endianness issues
> 
> Changes in v4:
> - Remove references to properties which are not good enough for Linux.
> - Don't use volatile in read/write replacements. Some arches have it and
>    some don't.
> - Put common timer properties into their own struct to better reuse
>    code.
> 
> Changes in v3:
> - Add clockevent and clocksource support
> - Rewrite probe to only use a device_node, since timers may need to be
>    initialized before we have proper devices. This does bloat the code a bit
>    since we can no longer rely on helpers such as dev_err_probe. We also
>    cannot rely on device resources being free'd on failure, so we must free
>    them manually.
> - We now access registers through xilinx_timer_(read|write). This allows us
>    to deal with endianness issues, as originally seen in the microblaze
>    driver. CAVEAT EMPTOR: I have not tested this on big-endian!
> - Remove old microblaze driver
> 
> 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
> 
>   MAINTAINERS                  |   1 +
>   drivers/clocksource/Makefile |   5 +-
>   drivers/pwm/Kconfig          |  13 ++
>   drivers/pwm/Makefile         |   1 +
>   drivers/pwm/pwm-xilinx.c     | 268 +++++++++++++++++++++++++++++++++++
>   5 files changed, 287 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/pwm/pwm-xilinx.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 252d71addd18..6adafd3e7a09 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20050,6 +20050,7 @@ XILINX TIMER/PWM DRIVER
>   M:	Sean Anderson <sean.anderson@seco.com>
>   S:	Maintained
>   F:	drivers/clocksource/timer-xilinx*
> +F:	drivers/pwm/pwm-xilinx.c
>   F:	include/clocksource/timer-xilinx.h
>   
>   XILINX UARTLITE SERIAL DRIVER
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index a7cba6ef5782..dcf434fdb726 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -88,4 +88,7 @@ obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
>   obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
>   obj-$(CONFIG_HYPERV_TIMER)		+= hyperv_timer.o
>   obj-$(CONFIG_MICROCHIP_PIT64B)		+= timer-microchip-pit64b.o
> -obj-$(CONFIG_XILINX_TIMER)		+= timer-xilinx.o timer-xilinx-common.o
> +obj-$(CONFIG_XILINX_TIMER)		+= timer-xilinx.o
> +ifneq ($(CONFIG_XILINX_TIMER$)(CONFIG_PWM_XILINX),)
> +obj-y					+= timer-xilinx-common.o
> +endif
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index c76adedd58c9..974774b7c987 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -637,4 +637,17 @@ 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 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..291577be992f
> --- /dev/null
> +++ b/drivers/pwm/pwm-xilinx.c
> @@ -0,0 +1,268 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
> + *
> + * 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 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/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * 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)
> +{
> +	bool enabled;
> +	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.
> +	 */
> +	rate = clk_get_rate(priv->clk);
> +	period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC);
> +	if (period_cycles - 2 > priv->max || period_cycles < 2)
> +		return -ERANGE;
> +	duty_cycles = mul_u64_u32_div(state->duty_cycle, rate, NSEC_PER_SEC);
> +
> +	/*
> +	 * If we specify 100% duty cycle, we will get 0% instead, so decrease
> +	 * the duty cycle count by one.
> +	 */
> +	if (period_cycles == duty_cycles)
> +		duty_cycles--;
> +
> +	/* Round down to 0% duty cycle for unrepresentable duty cycles */
> +	if (duty_cycles < 2)
> +		duty_cycles = period_cycles;
> +
> +	regmap_read(priv->map, TCSR0, &tcsr0);
> +	regmap_read(priv->map, TCSR1, &tcsr1);
> +	tlr0 = xilinx_timer_tlr_cycles(priv, tcsr0, period_cycles);
> +	tlr1 = xilinx_timer_tlr_cycles(priv, tcsr1, duty_cycles);
> +	regmap_write(priv->map, TLR0, tlr0);
> +	regmap_write(priv->map, TLR1, tlr1);
> +
> +	enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> +	if (state->enabled) {
> +		/*
> +		 * If the PWM is already running, then the counters will be
> +		 * reloaded at the end of the current cycle.
> +		 */
> +		if (!enabled) {
> +			/* Load TLR into TCR */
> +			regmap_write(priv->map, TCSR0, tcsr0 | TCSR_LOAD);
> +			regmap_write(priv->map, TCSR1, tcsr1 | TCSR_LOAD);
> +			/* Enable timers all at once with ENALL */
> +			tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
> +			tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
> +			regmap_write(priv->map, TCSR0, tcsr0);
> +			regmap_write(priv->map, TCSR1, tcsr1);
> +		}
> +	} else {
> +		regmap_write(priv->map, TCSR0, 0);
> +		regmap_write(priv->map, TCSR1, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
> +				 struct pwm_device *unused,
> +				 struct pwm_state *state)
> +{
> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +	u32 tlr0, tlr1, tcsr0, tcsr1;
> +
> +	regmap_read(priv->map, TLR0, &tlr0);
> +	regmap_read(priv->map, TLR1, &tlr1);
> +	regmap_read(priv->map, TCSR0, &tcsr0);
> +	regmap_read(priv->map, TCSR1, &tcsr1);
> +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
> +	state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
> +	state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> +	state->polarity = PWM_POLARITY_NORMAL;
> +
> +	/* 100% duty cycle results in constant low output */
> +	if (state->period == state->duty_cycle)
> +		state->duty_cycle = 0;
> +}
> +
> +static const struct pwm_ops xilinx_pwm_ops = {
> +	.apply = xilinx_pwm_apply,
> +	.get_state = xilinx_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static const struct regmap_config xilinx_pwm_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.max_register = TCR1,
> +	.disable_locking = true, /* The PWM subsystem handles locking */
> +};
> +
> +static int xilinx_timer_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct xilinx_timer_priv *priv;
> +	struct xilinx_pwm_device *pwm;
> +	u32 pwm_cells, one_timer;
> +	void __iomem *regs;
> +
> +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
> +	if (ret == -EINVAL)
> +		return -ENODEV;
> +	else if (ret)
> +		return dev_err_probe(dev, ret, "could not read #pwm-cells\n");
> +
> +	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, pwm);
> +	priv = &pwm->priv;
> +
> +	regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	priv->map = devm_regmap_init_mmio(dev, regs,
> +					  &xilinx_pwm_regmap_config);
> +	if (IS_ERR(priv->map))
> +		return dev_err_probe(dev, PTR_ERR(priv->map),
> +				     "could not create regmap\n");
> +
> +	ret = xilinx_timer_common_init(np, priv, &one_timer);
> +	if (ret)
> +		return ret;
> +
> +	if (one_timer)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "two timers required for PWM mode\n");
> +
> +	/*
> +	 * The polarity of the generate outputs must be active high for PWM
> +	 * mode to work. We could determine this from the device tree, but
> +	 * alas, such properties are not allowed to be used.
> +	 */
> +
> +	priv->clk = devm_clk_get(dev, "s_axi_aclk");
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> +				     "could not get clock\n");
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "clock enable failed\n");
> +	clk_rate_exclusive_get(priv->clk);
> +
> +	pwm->chip.dev = dev;
> +	pwm->chip.ops = &xilinx_pwm_ops;
> +	pwm->chip.npwm = 1;
> +	ret = pwmchip_add(&pwm->chip);
> +	if (ret) {
> +		clk_rate_exclusive_put(priv->clk);
> +		clk_disable_unprepare(priv->clk);
> +		return dev_err_probe(dev, ret, "could not register pwm chip\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static int xilinx_timer_remove(struct platform_device *pdev)
> +{
> +	struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
> +
> +	pwmchip_remove(&pwm->chip);
> +	clk_rate_exclusive_put(pwm->priv.clk);
> +	clk_disable_unprepare(pwm->priv.clk);
> +	return 0;
> +}
> +
> +static const struct of_device_id xilinx_timer_of_match[] = {
> +	{ .compatible = "xlnx,xps-timer-1.00.a", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, xilinx_timer_of_match);
> +
> +static struct platform_driver xilinx_timer_driver = {
> +	.probe = xilinx_timer_probe,
> +	.remove = xilinx_timer_remove,
> +	.driver = {
> +		.name = "xilinx-timer",
> +		.of_match_table = of_match_ptr(xilinx_timer_of_match),
> +	},
> +};
> +module_platform_driver(xilinx_timer_driver);
> +
> +MODULE_ALIAS("platform:xilinx-timer");
> +MODULE_DESCRIPTION("Xilinx LogiCORE IP AXI Timer driver");
> +MODULE_LICENSE("GPL v2");
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer
  2021-07-19 22:13 ` [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer Sean Anderson
  2021-07-21 13:26   ` kernel test robot
  2021-08-09 14:48   ` Sean Anderson
@ 2021-08-14 20:47   ` Uwe Kleine-König
  2021-08-16 23:51     ` Sean Anderson
  2 siblings, 1 reply; 15+ messages in thread
From: Uwe Kleine-König @ 2021-08-14 20:47 UTC (permalink / raw)
  To: Sean Anderson
  Cc: linux-pwm, devicetree, Thierry Reding, Alvaro Gamez,
	michal.simek, Lee Jones, linux-arm-kernel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 5622 bytes --]

Hello Sean,

sorry for having you let waiting so long. Now here some more feedback:

On Mon, Jul 19, 2021 at 06:13:22PM -0400, Sean Anderson wrote:
> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> +			    const struct pwm_state *state)
> +{
> +	bool enabled;
> +	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.
> +	 */
> +	rate = clk_get_rate(priv->clk);
> +	period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC);

cool, I didn't know mul_u64_u32_div.

Hmm, we still have a problem here if

	state->period * rate > 1000000000 * U64_MAX. 

So to be entirely save, we either need:

	/*
	 * To ensure that period * rate / NSEC_PER_SEC fits into an u64
	 * we need:
	 *            U64_MAX * NSEC_PER_SEC
	 *   period < ----------------------
	 *                    rate
         *
	 * . If rate is not bigger than NSEC_PER_SEC this is true for
	 * sure as the RHS is bigger than U64_MAX. Otherwise we can
	 * calculate the RHS using mul_u64_u32_div.
	 */
	if (rate > NSEC_PER_SEC)
		period = min(state->period, mul_u64_u32_div(U64_MAX, NSEC_PER_SEC, rate);
	else
		period = state->period;

or we go a step further and check the priv->max limit in the same step:

	period = min(state->period, ((u64)priv->max + 2) * NSEC_PER_SEC / rate)

. The latter is simpler and it's safe as priv->max is an u32 and so
there is no overflow.

> +	if (period_cycles - 2 > priv->max || period_cycles < 2)

I'd check for period_cycles < 2 first, because otherwise period_cycles -
2 might underflow. Nothing bad happens in this case, but reading from
left to right my first thought was I found a bug. Also please decrease
period_cycles if it's bigger than priv->max + 2. (With the suggestion
above you don't need to check for period_cycles - 2 > priv->max any more
however.)

> +		return -ERANGE;
> +	duty_cycles = mul_u64_u32_div(state->duty_cycle, rate, NSEC_PER_SEC);
> +
> +	/*
> +	 * If we specify 100% duty cycle, we will get 0% instead, so decrease
> +	 * the duty cycle count by one.
> +	 */
> +	if (period_cycles == duty_cycles)
> +		duty_cycles--;
> +
> +	/* Round down to 0% duty cycle for unrepresentable duty cycles */
> +	if (duty_cycles < 2)
> +		duty_cycles = period_cycles;
> +
> +	regmap_read(priv->map, TCSR0, &tcsr0);
> +	regmap_read(priv->map, TCSR1, &tcsr1);
> +	tlr0 = xilinx_timer_tlr_cycles(priv, tcsr0, period_cycles);
> +	tlr1 = xilinx_timer_tlr_cycles(priv, tcsr1, duty_cycles);
> +	regmap_write(priv->map, TLR0, tlr0);
> +	regmap_write(priv->map, TLR1, tlr1);
> +
> +	enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> +	if (state->enabled) {
> +		/*
> +		 * If the PWM is already running, then the counters will be
> +		 * reloaded at the end of the current cycle.
> +		 */

If state->enabled is false, $enabled isn't used, so you can move the
assignment into the if body and also limit the scope of $enabled.

> +		if (!enabled) {
> +			/* Load TLR into TCR */
> +			regmap_write(priv->map, TCSR0, tcsr0 | TCSR_LOAD);
> +			regmap_write(priv->map, TCSR1, tcsr1 | TCSR_LOAD);
> +			/* Enable timers all at once with ENALL */
> +			tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
> +			tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
> +			regmap_write(priv->map, TCSR0, tcsr0);
> +			regmap_write(priv->map, TCSR1, tcsr1);
> +		}
> +	} else {
> +		regmap_write(priv->map, TCSR0, 0);
> +		regmap_write(priv->map, TCSR1, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
> +				 struct pwm_device *unused,
> +				 struct pwm_state *state)
> +{
> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +	u32 tlr0, tlr1, tcsr0, tcsr1;
> +
> +	regmap_read(priv->map, TLR0, &tlr0);
> +	regmap_read(priv->map, TLR1, &tlr1);
> +	regmap_read(priv->map, TCSR0, &tcsr0);
> +	regmap_read(priv->map, TCSR1, &tcsr1);
> +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);

xilinx_timer_get_period rounds down, this is however wrong for
.get_state().

> +	state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);

ditto for duty_cycle.

> +	state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> +	state->polarity = PWM_POLARITY_NORMAL;
> [...]
> +static int xilinx_timer_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct xilinx_timer_priv *priv;
> +	struct xilinx_pwm_device *pwm;
> +	u32 pwm_cells, one_timer;
> +	void __iomem *regs;
> +
> +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
> +	if (ret == -EINVAL)
> +		return -ENODEV;
> +	else if (ret)
> +		return dev_err_probe(dev, ret, "could not read #pwm-cells\n");

Please capitalize error messages.

> [...]
> +	if (ret) {
> +		clk_rate_exclusive_put(priv->clk);
> +		clk_disable_unprepare(priv->clk);
> +		return dev_err_probe(dev, ret, "could not register pwm chip\n");

s/pwm/PWM/

> +	}
> +
> +	return 0;
> +}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer
  2021-08-14 20:47   ` Uwe Kleine-König
@ 2021-08-16 23:51     ` Sean Anderson
  2021-08-17 18:04       ` Uwe Kleine-König
  0 siblings, 1 reply; 15+ messages in thread
From: Sean Anderson @ 2021-08-16 23:51 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: linux-pwm, devicetree, Thierry Reding, Alvaro Gamez,
	michal.simek, Lee Jones, linux-arm-kernel, linux-kernel



On 8/14/21 4:47 PM, Uwe Kleine-König wrote:
> Hello Sean,
>
> sorry for having you let waiting so long. Now here some more feedback:
>
> On Mon, Jul 19, 2021 at 06:13:22PM -0400, Sean Anderson wrote:
>> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
>> +			    const struct pwm_state *state)
>> +{
>> +	bool enabled;
>> +	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.
>> +	 */
>> +	rate = clk_get_rate(priv->clk);
>> +	period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC);
>
> cool, I didn't know mul_u64_u32_div.

I didn't either. Alas, many useful functions like these have no
documentation...

>
> Hmm, we still have a problem here if
>
> 	state->period * rate > 1000000000 * U64_MAX.

Note that this can only occur with rate > 1GHz (and period = U64_MAX).
The highest fmax in the datasheet is 300 MHz (on a very expensive FPGA).

Maybe it is more prudent to do

	period = min(state->period, ULONG_MAX * NSEC_PER_SEC)

I think a period of 136 years is adequate :) This comparison also has
the advantage of being against const values.

> So to be entirely save, we either need:
>
> 	/*
> 	 * To ensure that period * rate / NSEC_PER_SEC fits into an u64
> 	 * we need:
> 	 *            U64_MAX * NSEC_PER_SEC
> 	 *   period < ----------------------
> 	 *                    rate
>           *
> 	 * . If rate is not bigger than NSEC_PER_SEC this is true for
> 	 * sure as the RHS is bigger than U64_MAX. Otherwise we can
> 	 * calculate the RHS using mul_u64_u32_div.
> 	 */
> 	if (rate > NSEC_PER_SEC)
> 		period = min(state->period, mul_u64_u32_div(U64_MAX, NSEC_PER_SEC, rate);
> 	else
> 		period = state->period;
>
> or we go a step further and check the priv->max limit in the same step:
>
> 	period = min(state->period, ((u64)priv->max + 2) * NSEC_PER_SEC / rate)
>
> . The latter is simpler and it's safe as priv->max is an u32 and so
> there is no overflow.
>
>> +	if (period_cycles - 2 > priv->max || period_cycles < 2)
>
> I'd check for period_cycles < 2 first, because otherwise period_cycles -
> 2 might underflow. Nothing bad happens in this case, but reading from
> left to right my first thought was I found a bug. Also please decrease
> period_cycles if it's bigger than priv->max + 2. (With the suggestion
> above you don't need to check for period_cycles - 2 > priv->max any more
> however.)

Ok, will swap.

>> +		return -ERANGE;
>> +	duty_cycles = mul_u64_u32_div(state->duty_cycle, rate, NSEC_PER_SEC);
>> +
>> +	/*
>> +	 * If we specify 100% duty cycle, we will get 0% instead, so decrease
>> +	 * the duty cycle count by one.
>> +	 */
>> +	if (period_cycles == duty_cycles)
>> +		duty_cycles--;
>> +
>> +	/* Round down to 0% duty cycle for unrepresentable duty cycles */
>> +	if (duty_cycles < 2)
>> +		duty_cycles = period_cycles;
>> +
>> +	regmap_read(priv->map, TCSR0, &tcsr0);
>> +	regmap_read(priv->map, TCSR1, &tcsr1);
>> +	tlr0 = xilinx_timer_tlr_cycles(priv, tcsr0, period_cycles);
>> +	tlr1 = xilinx_timer_tlr_cycles(priv, tcsr1, duty_cycles);
>> +	regmap_write(priv->map, TLR0, tlr0);
>> +	regmap_write(priv->map, TLR1, tlr1);
>> +
>> +	enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
>> +	if (state->enabled) {
>> +		/*
>> +		 * If the PWM is already running, then the counters will be
>> +		 * reloaded at the end of the current cycle.
>> +		 */
>
> If state->enabled is false, $enabled isn't used, so you can move the
> assignment into the if body and also limit the scope of $enabled.

Ok.

>> +		if (!enabled) {
>> +			/* Load TLR into TCR */
>> +			regmap_write(priv->map, TCSR0, tcsr0 | TCSR_LOAD);
>> +			regmap_write(priv->map, TCSR1, tcsr1 | TCSR_LOAD);
>> +			/* Enable timers all at once with ENALL */
>> +			tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
>> +			tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
>> +			regmap_write(priv->map, TCSR0, tcsr0);
>> +			regmap_write(priv->map, TCSR1, tcsr1);
>> +		}
>> +	} else {
>> +		regmap_write(priv->map, TCSR0, 0);
>> +		regmap_write(priv->map, TCSR1, 0);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
>> +				 struct pwm_device *unused,
>> +				 struct pwm_state *state)
>> +{
>> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
>> +	u32 tlr0, tlr1, tcsr0, tcsr1;
>> +
>> +	regmap_read(priv->map, TLR0, &tlr0);
>> +	regmap_read(priv->map, TLR1, &tlr1);
>> +	regmap_read(priv->map, TCSR0, &tcsr0);
>> +	regmap_read(priv->map, TCSR1, &tcsr1);
>> +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
>
> xilinx_timer_get_period rounds down, this is however wrong for
> .get_state().

Why is this wrong? I thought get_state should return values which would
not be rounded if passed to apply_state.

>> +	state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
>
> ditto for duty_cycle.
>
>> +	state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
>> +	state->polarity = PWM_POLARITY_NORMAL;
>> [...]
>> +static int xilinx_timer_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct xilinx_timer_priv *priv;
>> +	struct xilinx_pwm_device *pwm;
>> +	u32 pwm_cells, one_timer;
>> +	void __iomem *regs;
>> +
>> +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
>> +	if (ret == -EINVAL)
>> +		return -ENODEV;
>> +	else if (ret)
>> +		return dev_err_probe(dev, ret, "could not read #pwm-cells\n");
>
> Please capitalize error messages.

Ok.

>> [...]
>> +	if (ret) {
>> +		clk_rate_exclusive_put(priv->clk);
>> +		clk_disable_unprepare(priv->clk);
>> +		return dev_err_probe(dev, ret, "could not register pwm chip\n");
>
> s/pwm/PWM/

Ok.

Thanks for the review.

--Sean

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer
  2021-08-16 23:51     ` Sean Anderson
@ 2021-08-17 18:04       ` Uwe Kleine-König
  2021-08-17 22:56         ` Sean Anderson
  0 siblings, 1 reply; 15+ messages in thread
From: Uwe Kleine-König @ 2021-08-17 18:04 UTC (permalink / raw)
  To: Sean Anderson
  Cc: linux-pwm, devicetree, Thierry Reding, Alvaro Gamez,
	michal.simek, Lee Jones, linux-arm-kernel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3203 bytes --]

On Mon, Aug 16, 2021 at 07:51:17PM -0400, Sean Anderson wrote:
> 
> 
> On 8/14/21 4:47 PM, Uwe Kleine-König wrote:
> > Hello Sean,
> > 
> > sorry for having you let waiting so long. Now here some more feedback:
> > 
> > On Mon, Jul 19, 2021 at 06:13:22PM -0400, Sean Anderson wrote:
> > > +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> > > +			    const struct pwm_state *state)
> > > +{
> > > +	bool enabled;
> > > +	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.
> > > +	 */
> > > +	rate = clk_get_rate(priv->clk);
> > > +	period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC);
> > 
> > cool, I didn't know mul_u64_u32_div.
> 
> I didn't either. Alas, many useful functions like these have no
> documentation...
> 
> > 
> > Hmm, we still have a problem here if
> > 
> > 	state->period * rate > 1000000000 * U64_MAX.
> 
> Note that this can only occur with rate > 1GHz (and period = U64_MAX).
> The highest fmax in the datasheet is 300 MHz (on a very expensive FPGA).
> 
> Maybe it is more prudent to do
> 
> 	period = min(state->period, ULONG_MAX * NSEC_PER_SEC)

Together with a check for rate being <= 300 MHz to be safe that's fine.

> I think a period of 136 years is adequate :) This comparison also has
> the advantage of being against const values.

*nod*

> > > +static void xilinx_pwm_get_state(struct pwm_chip *chip,
> > > +				 struct pwm_device *unused,
> > > +				 struct pwm_state *state)
> > > +{
> > > +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> > > +	u32 tlr0, tlr1, tcsr0, tcsr1;
> > > +
> > > +	regmap_read(priv->map, TLR0, &tlr0);
> > > +	regmap_read(priv->map, TLR1, &tlr1);
> > > +	regmap_read(priv->map, TCSR0, &tcsr0);
> > > +	regmap_read(priv->map, TCSR1, &tcsr1);
> > > +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
> > 
> > xilinx_timer_get_period rounds down, this is however wrong for
> > .get_state().
> 
> Why is this wrong? I thought get_state should return values which would
> not be rounded if passed to apply_state.

Consider a PWM that yields a period of π * $regval ns when a certain
register is programmed with the value $regval.

Consider the HW is programmed with regval = 317. The exact period is
995.8848711879644. If now .get_state() rounds down and returns 995 ns and
you feed that value back into .apply the new regval (assuming round down
in .apply(), too) this yields regval = 316. If however .get_state()
rounds up and returns 996, putting this value back into .apply() you get
the desired 317.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer
  2021-08-17 18:04       ` Uwe Kleine-König
@ 2021-08-17 22:56         ` Sean Anderson
  0 siblings, 0 replies; 15+ messages in thread
From: Sean Anderson @ 2021-08-17 22:56 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: linux-pwm, devicetree, Thierry Reding, Alvaro Gamez,
	michal.simek, Lee Jones, linux-arm-kernel, linux-kernel



On 8/17/21 2:04 PM, Uwe Kleine-König wrote:
> On Mon, Aug 16, 2021 at 07:51:17PM -0400, Sean Anderson wrote:
>>
>>
>> On 8/14/21 4:47 PM, Uwe Kleine-König wrote:
>> > Hello Sean,
>> >
>> > sorry for having you let waiting so long. Now here some more feedback:
>> >
>> > On Mon, Jul 19, 2021 at 06:13:22PM -0400, Sean Anderson wrote:
>> > > +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
>> > > +			    const struct pwm_state *state)
>> > > +{
>> > > +	bool enabled;
>> > > +	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.
>> > > +	 */
>> > > +	rate = clk_get_rate(priv->clk);
>> > > +	period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC);
>> >
>> > cool, I didn't know mul_u64_u32_div.
>>
>> I didn't either. Alas, many useful functions like these have no
>> documentation...
>>
>> >
>> > Hmm, we still have a problem here if
>> >
>> > 	state->period * rate > 1000000000 * U64_MAX.
>>
>> Note that this can only occur with rate > 1GHz (and period = U64_MAX).
>> The highest fmax in the datasheet is 300 MHz (on a very expensive FPGA).
>>
>> Maybe it is more prudent to do
>>
>> 	period = min(state->period, ULONG_MAX * NSEC_PER_SEC)
>
> Together with a check for rate being <= 300 MHz to be safe that's fine.

That's what the ULONG_MAX is for; whatever we get back from
clk_get_rate, it has to fit in a ulong.

>
>> I think a period of 136 years is adequate :) This comparison also has
>> the advantage of being against const values.
>
> *nod*
>
>> > > +static void xilinx_pwm_get_state(struct pwm_chip *chip,
>> > > +				 struct pwm_device *unused,
>> > > +				 struct pwm_state *state)
>> > > +{
>> > > +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
>> > > +	u32 tlr0, tlr1, tcsr0, tcsr1;
>> > > +
>> > > +	regmap_read(priv->map, TLR0, &tlr0);
>> > > +	regmap_read(priv->map, TLR1, &tlr1);
>> > > +	regmap_read(priv->map, TCSR0, &tcsr0);
>> > > +	regmap_read(priv->map, TCSR1, &tcsr1);
>> > > +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
>> >
>> > xilinx_timer_get_period rounds down, this is however wrong for
>> > .get_state().
>>
>> Why is this wrong? I thought get_state should return values which would
>> not be rounded if passed to apply_state.
>
> Consider a PWM that yields a period of π * $regval ns when a certain
> register is programmed with the value $regval.
>
> Consider the HW is programmed with regval = 317. The exact period is
> 995.8848711879644. If now .get_state() rounds down and returns 995 ns and
> you feed that value back into .apply the new regval (assuming round down
> in .apply(), too) this yields regval = 316. If however .get_state()
> rounds up and returns 996, putting this value back into .apply() you get
> the desired 317.

Will fix for v6, but please document this somewhere :)

--Sean

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-08-17 22:56 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-19 22:13 [PATCH v5 1/3] dt-bindings: pwm: Add Xilinx AXI Timer Sean Anderson
2021-07-19 22:13 ` [PATCH v5 2/3] clocksource: Rewrite Xilinx AXI timer driver Sean Anderson
2021-07-20 21:41   ` kernel test robot
2021-07-20 21:44     ` Sean Anderson
2021-07-21  2:35   ` kernel test robot
2021-07-19 22:13 ` [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer Sean Anderson
2021-07-21 13:26   ` kernel test robot
2021-07-22 19:18     ` Sean Anderson
2021-08-09 14:48   ` Sean Anderson
2021-08-14 20:47   ` Uwe Kleine-König
2021-08-16 23:51     ` Sean Anderson
2021-08-17 18:04       ` Uwe Kleine-König
2021-08-17 22:56         ` Sean Anderson
2021-07-20 13:11 ` [PATCH v5 1/3] dt-bindings: pwm: Add " Rob Herring
2021-07-20 21:45   ` Sean Anderson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).