[PATCH v10 1/3] microblaze: timer: Remove unused properties

[PATCH v10 1/3] microblaze: timer: Remove unused properties

[Sean Anderson]

This removes properties not used by either the PWM or timer drivers.
This lets us set additionalProperties: false.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
Acked-by: Michal Simek <michal.simek@xilinx.com>
---

(no changes since v8)

Changes in v8:
- Remove additional properties from microblaze device tree

 arch/microblaze/boot/dts/system.dts | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/microblaze/boot/dts/system.dts b/arch/microblaze/boot/dts/system.dts
index b7ee1056779e..22252451ec09 100644
--- a/arch/microblaze/boot/dts/system.dts
+++ b/arch/microblaze/boot/dts/system.dts
@@ -347,12 +347,7 @@ xps_timer_1: timer@83c00000 {
 			interrupts = < 3 2 >;
 			reg = < 0x83c00000 0x10000 >;
 			xlnx,count-width = <0x20>;
-			xlnx,family = "virtex5";
-			xlnx,gen0-assert = <0x1>;
-			xlnx,gen1-assert = <0x1>;
 			xlnx,one-timer-only = <0x0>;
-			xlnx,trig0-assert = <0x1>;
-			xlnx,trig1-assert = <0x1>;
 		} ;
 	} ;
 }  ;
-- 
2.25.1

[PATCH v10 2/3] dt-bindings: pwm: Add Xilinx AXI Timer

[Sean Anderson]

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>
Reviewed-by: Rob Herring <robh@kernel.org>
---

(no changes since v8)

Changes in v8:
- Set additionalProperties: false

Changes in v7:
- Add #pwm-cells to properties
- Document why additionalProperties is true

Changes in v6:
- Enumerate possible counter widths
- Fix incorrect schema id

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

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

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

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

 .../bindings/timer/xlnx,xps-timer.yaml        | 92 +++++++++++++++++++
 1 file changed, 92 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..dd168d41d2e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
@@ -0,0 +1,92 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/xlnx,xps-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
+
+  '#pwm-cells': true
+
+  xlnx,count-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [8, 16, 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: false
+
+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

[PATCH v10 3/3] pwm: Add support for Xilinx AXI Timer

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

Some common code has been specially demarcated. While currently only
used by the PWM driver, it is anticipated that it may be split off in
the future to be used by the timer driver as well.

This driver was written with reference to Xilinx DS764 for v1.03.a [1].

[1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf

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

Changes in v10:
- Fix compilation error in timer driver

Changes in v9:
- Refactor "if { return } else if { }" to "if { return } if { }"
- Remove drivers/clocksource/timer-xilinx-common.c from MAINTAINERS
- Remove xilinx_timer_common_init and integrate it into xilinx_timer_probe

Changes in v8:
- Drop new timer driver; it has been deferred for future series

Changes in v7:
- Add dependency on OF_ADDRESS
- Fix period_cycles calculation
- Fix typo in limitations

Changes in v6:
- Capitalize error messages
- Don't disable regmap locking to allow inspection of registers via
  debugfs
- Prevent overflow when calculating period_cycles
- Remove enabled variable from xilinx_pwm_apply
- Swap order of period_cycle range comparisons

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

Changes in v4:
- 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.
- Remove references to properties which are not good enough for Linux.

Changes in v3:
- Add clockevent and clocksource support
- Remove old microblaze driver
- 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!

Changes in v2:
- Add comment describing device
- Add comment explaining why we depend on !MICROBLAZE
- Add dependencies on COMMON_CLK and HAS_IOMEM
- Cast dividends to u64 to avoid overflow
- Check for over- and underflow when calculating TLR
- Check range of xlnx,count-width
- Don't compile this module by default for arm64
- Don't set pwmchip.base to -1
- Ensure the clock is always running when the pwm is registered
- Remove debugfs file :l
- Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
- Report errors with dev_error_probe
- Set xilinx_pwm_ops.owner
- Use NSEC_TO_SEC instead of defining our own
- Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe

 MAINTAINERS                        |   6 +
 arch/microblaze/kernel/timer.c     |   3 +
 drivers/pwm/Kconfig                |  14 ++
 drivers/pwm/Makefile               |   1 +
 drivers/pwm/pwm-xilinx.c           | 311 +++++++++++++++++++++++++++++
 include/clocksource/timer-xilinx.h |  91 +++++++++
 6 files changed, 426 insertions(+)
 create mode 100644 drivers/pwm/pwm-xilinx.c
 create mode 100644 include/clocksource/timer-xilinx.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 3b79fd441dde..6f0f57c041c4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20614,6 +20614,12 @@ F:	drivers/misc/Makefile
 F:	drivers/misc/xilinx_sdfec.c
 F:	include/uapi/misc/xilinx_sdfec.h
 
+XILINX PWM DRIVER
+M:	Sean Anderson <sean.anderson@seco.com>
+S:	Maintained
+F:	drivers/pwm/pwm-xilinx.c
+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/timer.c b/arch/microblaze/kernel/timer.c
index f8832cf49384..dea34a3d4aa4 100644
--- a/arch/microblaze/kernel/timer.c
+++ b/arch/microblaze/kernel/timer.c
@@ -251,6 +251,9 @@ static int __init xilinx_timer_init(struct device_node *timer)
 	u32 timer_num = 1;
 	int ret;
 
+	if (of_property_read_bool(timer, "#pwm-cells"))
+		return 0;
+
 	if (initialized)
 		return -EINVAL;
 
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index aa29841bbb79..47f25237754f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -638,4 +638,18 @@ config PWM_VT8500
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-vt8500.
 
+config PWM_XILINX
+	tristate "Xilinx AXI Timer PWM support"
+	depends on OF_ADDRESS
+	depends on COMMON_CLK
+	select REGMAP_MMIO
+	help
+	  PWM driver for Xilinx LogiCORE IP AXI timers. This timer is
+	  typically a soft core which may be present in Xilinx FPGAs.
+	  This device may also be present in Microblaze soft processors.
+	  If you don't have this IP in your design, choose N.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-xilinx.
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 708840b7fba8..ea785480359b 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -60,3 +60,4 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
 obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
 obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
+obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c
new file mode 100644
index 000000000000..d79ef202d62f
--- /dev/null
+++ b/drivers/pwm/pwm-xilinx.c
@@ -0,0 +1,311 @@
+// 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 have a
+ *   bad cycle. This could probably be fixed by reading TCR0 just before
+ *   reprogramming, but I think it would add complexity for little gain.
+ * - Cannot produce 100% duty cycle by configuring the TLRs. This might be
+ *   possible by stopping the counters at an appropriate point in the cycle,
+ *   but this is not (yet) implemented.
+ * - Only produces "normal" output.
+ * - Always produces low output if disabled.
+ */
+
+#include <clocksource/timer-xilinx.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+/*
+ * The following functions are "common" to drivers for this device, and may be
+ * exported at a future date.
+ */
+u32 xilinx_timer_tlr_cycles(struct xilinx_timer_priv *priv, u32 tcsr,
+			    u64 cycles)
+{
+	WARN_ON(cycles < 2 || cycles - 2 > priv->max);
+
+	if (tcsr & TCSR_UDT)
+		return cycles - 2;
+	return priv->max - cycles + 2;
+}
+
+unsigned int xilinx_timer_get_period(struct xilinx_timer_priv *priv,
+				     u32 tlr, u32 tcsr)
+{
+	u64 cycles;
+
+	if (tcsr & TCSR_UDT)
+		cycles = tlr + 2;
+	else
+		cycles = (u64)priv->max - tlr + 2;
+
+	/* cycles has a max of 2^32 + 2 */
+	return DIV64_U64_ROUND_CLOSEST(cycles * NSEC_PER_SEC,
+				       clk_get_rate(priv->clk));
+}
+
+/*
+ * The idea here is to capture whether the PWM is actually running (e.g.
+ * because we or the bootloader set it up) and we need to be careful to ensure
+ * we don't cause a glitch. According to the data sheet, to enable the PWM we
+ * need to
+ *
+ * - Set both timers to generate mode (MDT=1)
+ * - Set both timers to PWM mode (PWMA=1)
+ * - Enable the generate out signals (GENT=1)
+ *
+ * In addition,
+ *
+ * - The timer must be running (ENT=1)
+ * - The timer must auto-reload TLR into TCR (ARHT=1)
+ * - We must not be in the process of loading TLR into TCR (LOAD=0)
+ * - Cascade mode must be disabled (CASC=0)
+ *
+ * If any of these differ from usual, then the PWM is either disabled, or is
+ * running in a mode that this driver does not support.
+ */
+#define TCSR_PWM_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)
+#define TCSR_PWM_CLEAR (TCSR_MDT | TCSR_LOAD)
+#define TCSR_PWM_MASK (TCSR_PWM_SET | TCSR_PWM_CLEAR)
+
+struct xilinx_pwm_device {
+	struct pwm_chip chip;
+	struct xilinx_timer_priv priv;
+};
+
+static inline struct xilinx_timer_priv
+*xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
+{
+	return &container_of(chip, struct xilinx_pwm_device, chip)->priv;
+}
+
+static bool xilinx_timer_pwm_enabled(u32 tcsr0, u32 tcsr1)
+{
+	return ((TCSR_PWM_MASK | TCSR_CASC) & tcsr0) == TCSR_PWM_SET &&
+		(TCSR_PWM_MASK & tcsr1) == TCSR_PWM_SET;
+}
+
+static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
+			    const struct pwm_state *state)
+{
+	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
+	u32 tlr0, tlr1, tcsr0, tcsr1;
+	u64 period_cycles, duty_cycles;
+	unsigned long rate;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	/*
+	 * To be representable by TLR, cycles must be between 2 and
+	 * priv->max + 2. To enforce this we can reduce the duty
+	 * cycle, but we may not increase it.
+	 */
+	rate = clk_get_rate(priv->clk);
+	/* Prevent overflow by clamping to the worst case of rate */
+	period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
+	period_cycles = mul_u64_u32_div(period_cycles, rate, NSEC_PER_SEC);
+	if (period_cycles < 2 || period_cycles - 2 > priv->max)
+		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);
+
+	if (state->enabled) {
+		/*
+		 * If the PWM is already running, then the counters will be
+		 * reloaded at the end of the current cycle.
+		 */
+		if (!xilinx_timer_pwm_enabled(tcsr0, tcsr1)) {
+			/* 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,
+};
+
+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, width;
+	void __iomem *regs;
+
+	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
+	if (ret == -EINVAL)
+		return -ENODEV;
+	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 = of_property_read_u32(np, "xlnx,one-timer-only", &one_timer);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Could not read xlnx,one-timer-only\n");
+
+	if (one_timer)
+		return dev_err_probe(dev, -EINVAL,
+				     "Two timers required for PWM mode\n");
+
+
+	ret = of_property_read_u32(np, "xlnx,count-width", &width);
+	if (ret == -EINVAL)
+		width = 32;
+	else if (ret)
+		return dev_err_probe(dev, ret,
+				     "Could not read xlnx,count-width\n");
+
+	if (width != 8 && width != 16 && width != 32)
+		return dev_err_probe(dev, -EINVAL,
+				     "Invalid counter width %d\n", width);
+	priv->max = BIT_ULL(width) - 1;
+
+	/*
+	 * 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");
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

Re: [PATCH v10 3/3] pwm: Add support for Xilinx AXI Timer

[Thierry Reding]

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

On Fri, Nov 12, 2021 at 01:55:04PM -0500, Sean Anderson wrote:
> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
> found on Xilinx FPGAs. At the moment clock control is very basic: we
> just enable the clock during probe and pin the frequency. In the future,
> someone could add support for disabling the clock when not in use.
> 
> Some common code has been specially demarcated. While currently only
> used by the PWM driver, it is anticipated that it may be split off in
> the future to be used by the timer driver as well.
> 
> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
> 
> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v10:
> - Fix compilation error in timer driver
> 
> Changes in v9:
> - Refactor "if { return } else if { }" to "if { return } if { }"
> - Remove drivers/clocksource/timer-xilinx-common.c from MAINTAINERS
> - Remove xilinx_timer_common_init and integrate it into xilinx_timer_probe
> 
> Changes in v8:
> - Drop new timer driver; it has been deferred for future series
> 
> Changes in v7:
> - Add dependency on OF_ADDRESS
> - Fix period_cycles calculation
> - Fix typo in limitations
> 
> Changes in v6:
> - Capitalize error messages
> - Don't disable regmap locking to allow inspection of registers via
>   debugfs
> - Prevent overflow when calculating period_cycles
> - Remove enabled variable from xilinx_pwm_apply
> - Swap order of period_cycle range comparisons
> 
> Changes in v5:
> - Allow non-zero #pwm-cells
> - Correctly set duty_cycle in get_state when TLR0=TLR1
> - Elaborate on limitation section
> - Perform some additional checks/rounding in apply_state
> - Remove xlnx,axi-timer-2.0 compatible string
> - Rework duty-cycle and period calculations with feedback from Uwe
> - Switch to regmap to abstract endianness issues
> - Use more verbose error messages
> 
> Changes in v4:
> - 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.
> - Remove references to properties which are not good enough for Linux.
> 
> Changes in v3:
> - Add clockevent and clocksource support
> - Remove old microblaze driver
> - 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!
> 
> Changes in v2:
> - Add comment describing device
> - Add comment explaining why we depend on !MICROBLAZE
> - Add dependencies on COMMON_CLK and HAS_IOMEM
> - Cast dividends to u64 to avoid overflow
> - Check for over- and underflow when calculating TLR
> - Check range of xlnx,count-width
> - Don't compile this module by default for arm64
> - Don't set pwmchip.base to -1
> - Ensure the clock is always running when the pwm is registered
> - Remove debugfs file :l
> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
> - Report errors with dev_error_probe
> - Set xilinx_pwm_ops.owner
> - Use NSEC_TO_SEC instead of defining our own
> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
> 
>  MAINTAINERS                        |   6 +
>  arch/microblaze/kernel/timer.c     |   3 +

Michal,

do you mind giving an Acked-by for this part. It looks harmless enough,
but just making sure you're aware of this.

Thierry

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

Re: [PATCH v10 3/3] pwm: Add support for Xilinx AXI Timer

[Michal Simek]

On 11/17/21 17:23, Thierry Reding wrote:
> On Fri, Nov 12, 2021 at 01:55:04PM -0500, Sean Anderson wrote:
>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
>> found on Xilinx FPGAs. At the moment clock control is very basic: we
>> just enable the clock during probe and pin the frequency. In the future,
>> someone could add support for disabling the clock when not in use.
>>
>> Some common code has been specially demarcated. While currently only
>> used by the PWM driver, it is anticipated that it may be split off in
>> the future to be used by the timer driver as well.
>>
>> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
>>
>> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>>
>> Changes in v10:
>> - Fix compilation error in timer driver
>>
>> Changes in v9:
>> - Refactor "if { return } else if { }" to "if { return } if { }"
>> - Remove drivers/clocksource/timer-xilinx-common.c from MAINTAINERS
>> - Remove xilinx_timer_common_init and integrate it into xilinx_timer_probe
>>
>> Changes in v8:
>> - Drop new timer driver; it has been deferred for future series
>>
>> Changes in v7:
>> - Add dependency on OF_ADDRESS
>> - Fix period_cycles calculation
>> - Fix typo in limitations
>>
>> Changes in v6:
>> - Capitalize error messages
>> - Don't disable regmap locking to allow inspection of registers via
>>    debugfs
>> - Prevent overflow when calculating period_cycles
>> - Remove enabled variable from xilinx_pwm_apply
>> - Swap order of period_cycle range comparisons
>>
>> Changes in v5:
>> - Allow non-zero #pwm-cells
>> - Correctly set duty_cycle in get_state when TLR0=TLR1
>> - Elaborate on limitation section
>> - Perform some additional checks/rounding in apply_state
>> - Remove xlnx,axi-timer-2.0 compatible string
>> - Rework duty-cycle and period calculations with feedback from Uwe
>> - Switch to regmap to abstract endianness issues
>> - Use more verbose error messages
>>
>> Changes in v4:
>> - 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.
>> - Remove references to properties which are not good enough for Linux.
>>
>> Changes in v3:
>> - Add clockevent and clocksource support
>> - Remove old microblaze driver
>> - 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!
>>
>> Changes in v2:
>> - Add comment describing device
>> - Add comment explaining why we depend on !MICROBLAZE
>> - Add dependencies on COMMON_CLK and HAS_IOMEM
>> - Cast dividends to u64 to avoid overflow
>> - Check for over- and underflow when calculating TLR
>> - Check range of xlnx,count-width
>> - Don't compile this module by default for arm64
>> - Don't set pwmchip.base to -1
>> - Ensure the clock is always running when the pwm is registered
>> - Remove debugfs file :l
>> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
>> - Report errors with dev_error_probe
>> - Set xilinx_pwm_ops.owner
>> - Use NSEC_TO_SEC instead of defining our own
>> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
>>
>>   MAINTAINERS                        |   6 +
>>   arch/microblaze/kernel/timer.c     |   3 +
> 
> Michal,
> 
> do you mind giving an Acked-by for this part. It looks harmless enough,
> but just making sure you're aware of this.

That's fine for me.

Acked-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal

Re: [PATCH v10 3/3] pwm: Add support for Xilinx AXI Timer

[Uwe Kleine-König]

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

Hello,

On Fri, Nov 12, 2021 at 01:55:04PM -0500, Sean Anderson wrote:
> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
> found on Xilinx FPGAs. At the moment clock control is very basic: we
> just enable the clock during probe and pin the frequency. In the future,
> someone could add support for disabling the clock when not in use.
> 
> Some common code has been specially demarcated. While currently only
> used by the PWM driver, it is anticipated that it may be split off in
> the future to be used by the timer driver as well.
> 
> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
> 
> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v10:
> - Fix compilation error in timer driver
> 
> Changes in v9:
> - Refactor "if { return } else if { }" to "if { return } if { }"
> - Remove drivers/clocksource/timer-xilinx-common.c from MAINTAINERS
> - Remove xilinx_timer_common_init and integrate it into xilinx_timer_probe
> 
> Changes in v8:
> - Drop new timer driver; it has been deferred for future series
> 
> Changes in v7:
> - Add dependency on OF_ADDRESS
> - Fix period_cycles calculation
> - Fix typo in limitations
> 
> Changes in v6:
> - Capitalize error messages
> - Don't disable regmap locking to allow inspection of registers via
>   debugfs
> - Prevent overflow when calculating period_cycles
> - Remove enabled variable from xilinx_pwm_apply
> - Swap order of period_cycle range comparisons
> 
> Changes in v5:
> - Allow non-zero #pwm-cells
> - Correctly set duty_cycle in get_state when TLR0=TLR1
> - Elaborate on limitation section
> - Perform some additional checks/rounding in apply_state
> - Remove xlnx,axi-timer-2.0 compatible string
> - Rework duty-cycle and period calculations with feedback from Uwe
> - Switch to regmap to abstract endianness issues
> - Use more verbose error messages
> 
> Changes in v4:
> - 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.
> - Remove references to properties which are not good enough for Linux.
> 
> Changes in v3:
> - Add clockevent and clocksource support
> - Remove old microblaze driver
> - 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!
> 
> Changes in v2:
> - Add comment describing device
> - Add comment explaining why we depend on !MICROBLAZE
> - Add dependencies on COMMON_CLK and HAS_IOMEM
> - Cast dividends to u64 to avoid overflow
> - Check for over- and underflow when calculating TLR
> - Check range of xlnx,count-width
> - Don't compile this module by default for arm64
> - Don't set pwmchip.base to -1
> - Ensure the clock is always running when the pwm is registered
> - Remove debugfs file :l
> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
> - Report errors with dev_error_probe
> - Set xilinx_pwm_ops.owner
> - Use NSEC_TO_SEC instead of defining our own
> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
> 
>  MAINTAINERS                        |   6 +
>  arch/microblaze/kernel/timer.c     |   3 +
>  drivers/pwm/Kconfig                |  14 ++
>  drivers/pwm/Makefile               |   1 +
>  drivers/pwm/pwm-xilinx.c           | 311 +++++++++++++++++++++++++++++
>  include/clocksource/timer-xilinx.h |  91 +++++++++
>  6 files changed, 426 insertions(+)
>  create mode 100644 drivers/pwm/pwm-xilinx.c
>  create mode 100644 include/clocksource/timer-xilinx.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3b79fd441dde..6f0f57c041c4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20614,6 +20614,12 @@ F:	drivers/misc/Makefile
>  F:	drivers/misc/xilinx_sdfec.c
>  F:	include/uapi/misc/xilinx_sdfec.h
>  
> +XILINX PWM DRIVER
> +M:	Sean Anderson <sean.anderson@seco.com>
> +S:	Maintained
> +F:	drivers/pwm/pwm-xilinx.c
> +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/timer.c b/arch/microblaze/kernel/timer.c
> index f8832cf49384..dea34a3d4aa4 100644
> --- a/arch/microblaze/kernel/timer.c
> +++ b/arch/microblaze/kernel/timer.c
> @@ -251,6 +251,9 @@ static int __init xilinx_timer_init(struct device_node *timer)
>  	u32 timer_num = 1;
>  	int ret;
>  
> +	if (of_property_read_bool(timer, "#pwm-cells"))
> +		return 0;
> +
>  	if (initialized)
>  		return -EINVAL;
>  
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index aa29841bbb79..47f25237754f 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -638,4 +638,18 @@ config PWM_VT8500
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-vt8500.
>  
> +config PWM_XILINX
> +	tristate "Xilinx AXI Timer PWM support"
> +	depends on OF_ADDRESS
> +	depends on COMMON_CLK
> +	select REGMAP_MMIO
> +	help
> +	  PWM driver for Xilinx LogiCORE IP AXI timers. This timer is
> +	  typically a soft core which may be present in Xilinx FPGAs.
> +	  This device may also be present in Microblaze soft processors.
> +	  If you don't have this IP in your design, choose N.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-xilinx.
> +
>  endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 708840b7fba8..ea785480359b 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -60,3 +60,4 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>  obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> +obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
> diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c
> new file mode 100644
> index 000000000000..d79ef202d62f
> --- /dev/null
> +++ b/drivers/pwm/pwm-xilinx.c
> @@ -0,0 +1,311 @@
> +// 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 have a
> + *   bad cycle. This could probably be fixed by reading TCR0 just before
> + *   reprogramming, but I think it would add complexity for little gain.
> + * - Cannot produce 100% duty cycle by configuring the TLRs. This might be
> + *   possible by stopping the counters at an appropriate point in the cycle,
> + *   but this is not (yet) implemented.
> + * - Only produces "normal" output.
> + * - Always produces low output if disabled.
> + */
> +
> +#include <clocksource/timer-xilinx.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * The following functions are "common" to drivers for this device, and may be
> + * exported at a future date.
> + */
> +u32 xilinx_timer_tlr_cycles(struct xilinx_timer_priv *priv, u32 tcsr,
> +			    u64 cycles)
> +{
> +	WARN_ON(cycles < 2 || cycles - 2 > priv->max);
> +
> +	if (tcsr & TCSR_UDT)
> +		return cycles - 2;
> +	return priv->max - cycles + 2;
> +}
> +
> +unsigned int xilinx_timer_get_period(struct xilinx_timer_priv *priv,
> +				     u32 tlr, u32 tcsr)
> +{
> +	u64 cycles;
> +
> +	if (tcsr & TCSR_UDT)
> +		cycles = tlr + 2;
> +	else
> +		cycles = (u64)priv->max - tlr + 2;
> +
> +	/* cycles has a max of 2^32 + 2 */
> +	return DIV64_U64_ROUND_CLOSEST(cycles * NSEC_PER_SEC,
> +				       clk_get_rate(priv->clk));

Please round up here.

> +}
> +
> +/*
> + * The idea here is to capture whether the PWM is actually running (e.g.
> + * because we or the bootloader set it up) and we need to be careful to ensure
> + * we don't cause a glitch. According to the data sheet, to enable the PWM we
> + * need to
> + *
> + * - Set both timers to generate mode (MDT=1)
> + * - Set both timers to PWM mode (PWMA=1)
> + * - Enable the generate out signals (GENT=1)
> + *
> + * In addition,
> + *
> + * - The timer must be running (ENT=1)
> + * - The timer must auto-reload TLR into TCR (ARHT=1)
> + * - We must not be in the process of loading TLR into TCR (LOAD=0)
> + * - Cascade mode must be disabled (CASC=0)
> + *
> + * If any of these differ from usual, then the PWM is either disabled, or is
> + * running in a mode that this driver does not support.
> + */
> +#define TCSR_PWM_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)
> +#define TCSR_PWM_CLEAR (TCSR_MDT | TCSR_LOAD)
> +#define TCSR_PWM_MASK (TCSR_PWM_SET | TCSR_PWM_CLEAR)
> +
> +struct xilinx_pwm_device {
> +	struct pwm_chip chip;
> +	struct xilinx_timer_priv priv;
> +};
> +
> +static inline struct xilinx_timer_priv
> +*xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
> +{
> +	return &container_of(chip, struct xilinx_pwm_device, chip)->priv;
> +}
> +
> +static bool xilinx_timer_pwm_enabled(u32 tcsr0, u32 tcsr1)
> +{
> +	return ((TCSR_PWM_MASK | TCSR_CASC) & tcsr0) == TCSR_PWM_SET &&
> +		(TCSR_PWM_MASK & tcsr1) == TCSR_PWM_SET;
> +}
> +
> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> +			    const struct pwm_state *state)
> +{
> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +	u32 tlr0, tlr1, tcsr0, tcsr1;
> +	u64 period_cycles, duty_cycles;
> +	unsigned long rate;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	/*
> +	 * To be representable by TLR, cycles must be between 2 and
> +	 * priv->max + 2. To enforce this we can reduce the duty
> +	 * cycle, but we may not increase it.
> +	 */
> +	rate = clk_get_rate(priv->clk);
> +	/* Prevent overflow by clamping to the worst case of rate */

I wouldn't have called this "worst case of rate", maybe better use
"maximal rate"?

> +	period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
> +	period_cycles = mul_u64_u32_div(period_cycles, rate, NSEC_PER_SEC);
> +	if (period_cycles < 2 || period_cycles - 2 > priv->max)
> +		return -ERANGE;

if period_cycles - 2 > priv->max the right reaction is to do

	period_cycles = priv->max + 2

.

> +	duty_cycles = mul_u64_u32_div(state->duty_cycle, rate, NSEC_PER_SEC);

duty_cycle needs sanitation in case period was reduced and duty_cycle is
bigger now than period.

> +	/*
> +	 * 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);
> +
> +	if (state->enabled) {
> +		/*
> +		 * If the PWM is already running, then the counters will be
> +		 * reloaded at the end of the current cycle.
> +		 */
> +		if (!xilinx_timer_pwm_enabled(tcsr0, tcsr1)) {
> +			/* 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,
> +};
> +
> +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;

The name "pwm" is usually reserved for struct pwm_device pointers. A
typical name for this would be xlnxpwm or ddata.

> +	u32 pwm_cells, one_timer, width;
> +	void __iomem *regs;
> +
> +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
> +	if (ret == -EINVAL)
> +		return -ENODEV;

A comment about why this is done would be great.

> +	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 = of_property_read_u32(np, "xlnx,one-timer-only", &one_timer);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Could not read xlnx,one-timer-only\n");
> +
> +	if (one_timer)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Two timers required for PWM mode\n");
> +
> +
> +	ret = of_property_read_u32(np, "xlnx,count-width", &width);
> +	if (ret == -EINVAL)
> +		width = 32;
> +	else if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Could not read xlnx,count-width\n");
> +
> +	if (width != 8 && width != 16 && width != 32)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid counter width %d\n", width);
> +	priv->max = BIT_ULL(width) - 1;
> +
> +	/*
> +	 * The polarity of the generate outputs must be active high for PWM

s/generate/generated/

> +	 * 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",

Doesn't this give a wrong impression as this is a PWM driver, not a
timer driver?

> +		.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");
> 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
> 
> 

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

Re: [PATCH v10 3/3] pwm: Add support for Xilinx AXI Timer

[Sean Anderson]

Hi Uwe,

On 11/18/21 4:28 AM, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, Nov 12, 2021 at 01:55:04PM -0500, Sean Anderson wrote:
>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
>> found on Xilinx FPGAs. At the moment clock control is very basic: we
>> just enable the clock during probe and pin the frequency. In the future,
>> someone could add support for disabling the clock when not in use.
>>
>> Some common code has been specially demarcated. While currently only
>> used by the PWM driver, it is anticipated that it may be split off in
>> the future to be used by the timer driver as well.
>>
>> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
>>
>> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>>
>> Changes in v10:
>> - Fix compilation error in timer driver
>>
>> Changes in v9:
>> - Refactor "if { return } else if { }" to "if { return } if { }"
>> - Remove drivers/clocksource/timer-xilinx-common.c from MAINTAINERS
>> - Remove xilinx_timer_common_init and integrate it into xilinx_timer_probe
>>
>> Changes in v8:
>> - Drop new timer driver; it has been deferred for future series
>>
>> Changes in v7:
>> - Add dependency on OF_ADDRESS
>> - Fix period_cycles calculation
>> - Fix typo in limitations
>>
>> Changes in v6:
>> - Capitalize error messages
>> - Don't disable regmap locking to allow inspection of registers via
>>   debugfs
>> - Prevent overflow when calculating period_cycles
>> - Remove enabled variable from xilinx_pwm_apply
>> - Swap order of period_cycle range comparisons
>>
>> Changes in v5:
>> - Allow non-zero #pwm-cells
>> - Correctly set duty_cycle in get_state when TLR0=TLR1
>> - Elaborate on limitation section
>> - Perform some additional checks/rounding in apply_state
>> - Remove xlnx,axi-timer-2.0 compatible string
>> - Rework duty-cycle and period calculations with feedback from Uwe
>> - Switch to regmap to abstract endianness issues
>> - Use more verbose error messages
>>
>> Changes in v4:
>> - 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.
>> - Remove references to properties which are not good enough for Linux.
>>
>> Changes in v3:
>> - Add clockevent and clocksource support
>> - Remove old microblaze driver
>> - 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!
>>
>> Changes in v2:
>> - Add comment describing device
>> - Add comment explaining why we depend on !MICROBLAZE
>> - Add dependencies on COMMON_CLK and HAS_IOMEM
>> - Cast dividends to u64 to avoid overflow
>> - Check for over- and underflow when calculating TLR
>> - Check range of xlnx,count-width
>> - Don't compile this module by default for arm64
>> - Don't set pwmchip.base to -1
>> - Ensure the clock is always running when the pwm is registered
>> - Remove debugfs file :l
>> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
>> - Report errors with dev_error_probe
>> - Set xilinx_pwm_ops.owner
>> - Use NSEC_TO_SEC instead of defining our own
>> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
>>
>>  MAINTAINERS                        |   6 +
>>  arch/microblaze/kernel/timer.c     |   3 +
>>  drivers/pwm/Kconfig                |  14 ++
>>  drivers/pwm/Makefile               |   1 +
>>  drivers/pwm/pwm-xilinx.c           | 311 +++++++++++++++++++++++++++++
>>  include/clocksource/timer-xilinx.h |  91 +++++++++
>>  6 files changed, 426 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-xilinx.c
>>  create mode 100644 include/clocksource/timer-xilinx.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3b79fd441dde..6f0f57c041c4 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20614,6 +20614,12 @@ F:	drivers/misc/Makefile
>>  F:	drivers/misc/xilinx_sdfec.c
>>  F:	include/uapi/misc/xilinx_sdfec.h
>>
>> +XILINX PWM DRIVER
>> +M:	Sean Anderson <sean.anderson@seco.com>
>> +S:	Maintained
>> +F:	drivers/pwm/pwm-xilinx.c
>> +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/timer.c b/arch/microblaze/kernel/timer.c
>> index f8832cf49384..dea34a3d4aa4 100644
>> --- a/arch/microblaze/kernel/timer.c
>> +++ b/arch/microblaze/kernel/timer.c
>> @@ -251,6 +251,9 @@ static int __init xilinx_timer_init(struct device_node *timer)
>>  	u32 timer_num = 1;
>>  	int ret;
>>
>> +	if (of_property_read_bool(timer, "#pwm-cells"))
>> +		return 0;
>> +
>>  	if (initialized)
>>  		return -EINVAL;
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index aa29841bbb79..47f25237754f 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -638,4 +638,18 @@ config PWM_VT8500
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called pwm-vt8500.
>>
>> +config PWM_XILINX
>> +	tristate "Xilinx AXI Timer PWM support"
>> +	depends on OF_ADDRESS
>> +	depends on COMMON_CLK
>> +	select REGMAP_MMIO
>> +	help
>> +	  PWM driver for Xilinx LogiCORE IP AXI timers. This timer is
>> +	  typically a soft core which may be present in Xilinx FPGAs.
>> +	  This device may also be present in Microblaze soft processors.
>> +	  If you don't have this IP in your design, choose N.
>> +
>> +	  To compile this driver as a module, choose M here: the module
>> +	  will be called pwm-xilinx.
>> +
>>  endif
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 708840b7fba8..ea785480359b 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -60,3 +60,4 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>>  obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
>>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
>> +obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
>> diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c
>> new file mode 100644
>> index 000000000000..d79ef202d62f
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-xilinx.c
>> @@ -0,0 +1,311 @@
>> +// 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 have a
>> + *   bad cycle. This could probably be fixed by reading TCR0 just before
>> + *   reprogramming, but I think it would add complexity for little gain.
>> + * - Cannot produce 100% duty cycle by configuring the TLRs. This might be
>> + *   possible by stopping the counters at an appropriate point in the cycle,
>> + *   but this is not (yet) implemented.
>> + * - Only produces "normal" output.
>> + * - Always produces low output if disabled.
>> + */
>> +
>> +#include <clocksource/timer-xilinx.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/regmap.h>
>> +
>> +/*
>> + * The following functions are "common" to drivers for this device, and may be
>> + * exported at a future date.
>> + */
>> +u32 xilinx_timer_tlr_cycles(struct xilinx_timer_priv *priv, u32 tcsr,
>> +			    u64 cycles)
>> +{
>> +	WARN_ON(cycles < 2 || cycles - 2 > priv->max);
>> +
>> +	if (tcsr & TCSR_UDT)
>> +		return cycles - 2;
>> +	return priv->max - cycles + 2;
>> +}
>> +
>> +unsigned int xilinx_timer_get_period(struct xilinx_timer_priv *priv,
>> +				     u32 tlr, u32 tcsr)
>> +{
>> +	u64 cycles;
>> +
>> +	if (tcsr & TCSR_UDT)
>> +		cycles = tlr + 2;
>> +	else
>> +		cycles = (u64)priv->max - tlr + 2;
>> +
>> +	/* cycles has a max of 2^32 + 2 */
>> +	return DIV64_U64_ROUND_CLOSEST(cycles * NSEC_PER_SEC,
>> +				       clk_get_rate(priv->clk));
>
> Please round up here.

Please document the correct rounding mode you expect. The last time we
discussed this (3 months ago), you only said that rounding down was
incorrect...

>> +}
>> +
>> +/*
>> + * The idea here is to capture whether the PWM is actually running (e.g.
>> + * because we or the bootloader set it up) and we need to be careful to ensure
>> + * we don't cause a glitch. According to the data sheet, to enable the PWM we
>> + * need to
>> + *
>> + * - Set both timers to generate mode (MDT=1)
>> + * - Set both timers to PWM mode (PWMA=1)
>> + * - Enable the generate out signals (GENT=1)
>> + *
>> + * In addition,
>> + *
>> + * - The timer must be running (ENT=1)
>> + * - The timer must auto-reload TLR into TCR (ARHT=1)
>> + * - We must not be in the process of loading TLR into TCR (LOAD=0)
>> + * - Cascade mode must be disabled (CASC=0)
>> + *
>> + * If any of these differ from usual, then the PWM is either disabled, or is
>> + * running in a mode that this driver does not support.
>> + */
>> +#define TCSR_PWM_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)
>> +#define TCSR_PWM_CLEAR (TCSR_MDT | TCSR_LOAD)
>> +#define TCSR_PWM_MASK (TCSR_PWM_SET | TCSR_PWM_CLEAR)
>> +
>> +struct xilinx_pwm_device {
>> +	struct pwm_chip chip;
>> +	struct xilinx_timer_priv priv;
>> +};
>> +
>> +static inline struct xilinx_timer_priv
>> +*xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
>> +{
>> +	return &container_of(chip, struct xilinx_pwm_device, chip)->priv;
>> +}
>> +
>> +static bool xilinx_timer_pwm_enabled(u32 tcsr0, u32 tcsr1)
>> +{
>> +	return ((TCSR_PWM_MASK | TCSR_CASC) & tcsr0) == TCSR_PWM_SET &&
>> +		(TCSR_PWM_MASK & tcsr1) == TCSR_PWM_SET;
>> +}
>> +
>> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
>> +			    const struct pwm_state *state)
>> +{
>> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
>> +	u32 tlr0, tlr1, tcsr0, tcsr1;
>> +	u64 period_cycles, duty_cycles;
>> +	unsigned long rate;
>> +
>> +	if (state->polarity != PWM_POLARITY_NORMAL)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * To be representable by TLR, cycles must be between 2 and
>> +	 * priv->max + 2. To enforce this we can reduce the duty
>> +	 * cycle, but we may not increase it.
>> +	 */
>> +	rate = clk_get_rate(priv->clk);
>> +	/* Prevent overflow by clamping to the worst case of rate */
>
> I wouldn't have called this "worst case of rate", maybe better use
> "maximal rate"?

OK

>> +	period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
>> +	period_cycles = mul_u64_u32_div(period_cycles, rate, NSEC_PER_SEC);
>> +	if (period_cycles < 2 || period_cycles - 2 > priv->max)
>> +		return -ERANGE;
>
> if period_cycles - 2 > priv->max the right reaction is to do
>
> 	period_cycles = priv->max + 2

It has been 5 months since we last talked about this, and yet you have
not submitted any patches for a "pwm_round_rate" function. Forgive me if
I am reticent to implement forward compatibility for an API which shows
no signs of appearing.

>> +	duty_cycles = mul_u64_u32_div(state->duty_cycle, rate, NSEC_PER_SEC);
>
> duty_cycle needs sanitation in case period was reduced and duty_cycle is
> bigger now than period.

OK

>> +	/*
>> +	 * 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);
>> +
>> +	if (state->enabled) {
>> +		/*
>> +		 * If the PWM is already running, then the counters will be
>> +		 * reloaded at the end of the current cycle.
>> +		 */
>> +		if (!xilinx_timer_pwm_enabled(tcsr0, tcsr1)) {
>> +			/* 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,
>> +};
>> +
>> +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;
>
> The name "pwm" is usually reserved for struct pwm_device pointers. A
> typical name for this would be xlnxpwm or ddata.

I suppose. However, no variables of struct pwm_device are used in
this driver.

>> +	u32 pwm_cells, one_timer, width;
>> +	void __iomem *regs;
>> +
>> +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
>> +	if (ret == -EINVAL)
>> +		return -ENODEV;
>
> A comment about why this is done would be great.

OK. How about:

/* If there are no #pwm-cells, this binding is for a timer and not a PWM */

>> +	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 = of_property_read_u32(np, "xlnx,one-timer-only", &one_timer);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Could not read xlnx,one-timer-only\n");
>> +
>> +	if (one_timer)
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Two timers required for PWM mode\n");
>> +
>> +
>> +	ret = of_property_read_u32(np, "xlnx,count-width", &width);
>> +	if (ret == -EINVAL)
>> +		width = 32;
>> +	else if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Could not read xlnx,count-width\n");
>> +
>> +	if (width != 8 && width != 16 && width != 32)
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Invalid counter width %d\n", width);
>> +	priv->max = BIT_ULL(width) - 1;
>> +
>> +	/*
>> +	 * The polarity of the generate outputs must be active high for PWM
>
> s/generate/generated/

The signals I am referring to are called "GenerateOut0" and
"GenerateOut1".

>> +	 * 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",
>
> Doesn't this give a wrong impression as this is a PWM driver, not a
> timer driver?

Perhaps. Though this is the PWM driver for the Xilinx AXI timer, not the
Xilinx AXI PWM.

--Sean

>> +		.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");
>> 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
>>
>>
>

Re: [PATCH v10 3/3] pwm: Add support for Xilinx AXI Timer

[Uwe Kleine-König]

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

Hello Sean,

On Thu, Nov 18, 2021 at 04:08:45PM -0500, Sean Anderson wrote:
> On 11/18/21 4:28 AM, Uwe Kleine-König wrote:
> > On Fri, Nov 12, 2021 at 01:55:04PM -0500, Sean Anderson wrote:
> > > [...]
> > > +	/* cycles has a max of 2^32 + 2 */
> > > +	return DIV64_U64_ROUND_CLOSEST(cycles * NSEC_PER_SEC,
> > > +				       clk_get_rate(priv->clk));
> > 
> > Please round up here.
> 
> Please document the correct rounding mode you expect. The last time we
> discussed this (3 months ago), you only said that rounding down was
> incorrect...

I think you refer to
https://lore.kernel.org/linux-pwm/20210817180407.ru4prwu344dxpynu@pengutronix.de
here, right? I agree that I could have been a bit more explicit here.

.apply should first round down .period to the next achievable setting
and then .duty_cycle should be round down to the next achievable setting
(in combination with the chosen period).

To get .apply ∘ .get_state idempotent (i.e. if I apply the result from
get_state there are no changes), .get_state has to round up.

After our longer discussion about v4 I would have expected that this was
already obvious. There you wrote[1]:

> * The apply_state function shall only round the requested period down, and
>    may do so by no more than one unit cycle. If the requested period is
>    unrepresentable by the PWM, the apply_state function shall return
>    -ERANGE.
> * The apply_state function shall only round the requested duty cycle
>    down. The apply_state function shall not return an error unless there
>    is no duty cycle less than the requested duty cycle which is
>    representable by the PWM.
> * After applying a state returned by round_state with apply_state,
>    get_state must return that state.

The requirement to round up is a direct consequence of these three
points, which I confirmed (apart from some wording issues).

[1] https://lore.kernel.org/linux-pwm/ddd2ad0c-1dff-c437-17a6-4c7be72c2fce@seco.com

> > > +	period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
> > > +	period_cycles = mul_u64_u32_div(period_cycles, rate, NSEC_PER_SEC);
> > > +	if (period_cycles < 2 || period_cycles - 2 > priv->max)
> > > +		return -ERANGE;
> > 
> > if period_cycles - 2 > priv->max the right reaction is to do
> > 
> > 	period_cycles = priv->max + 2
> 
> It has been 5 months since we last talked about this, and yet you have
> not submitted any patches for a "pwm_round_rate" function. Forgive me if
> I am reticent to implement forward compatibility for an API which shows
> no signs of appearing.

This requirement is not only for round_state. It's also to get some
common behaviour for at least new drivers. The primary goal here is to
make the result for pwm_apply more predictable.

> > > +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;
> > 
> > The name "pwm" is usually reserved for struct pwm_device pointers. A
> > typical name for this would be xlnxpwm or ddata.
> 
> I suppose. However, no variables of struct pwm_device are used in
> this driver.

Still it provokes wrong expectations when reading

	platform_set_drvdata(pdev, pwm);

in a pwm driver.

> > > +	u32 pwm_cells, one_timer, width;
> > > +	void __iomem *regs;
> > > +
> > > +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
> > > +	if (ret == -EINVAL)
> > > +		return -ENODEV;
> > 
> > A comment about why this is done would be great.
> 
> OK. How about:
> 
> /* If there are no #pwm-cells, this binding is for a timer and not a PWM */

Fine. Does that mean the timer driver won't bind in the presence of the
#pwm-cells property, and the timer driver uses the same compatible?
Sounds a bit strange to me.

> > > +	/*
> > > +	 * The polarity of the generate outputs must be active high for PWM
> > 
> > s/generate/generated/
> 
> The signals I am referring to are called "GenerateOut0" and
> "GenerateOut1".

Then maybe:

	The polarity of the outputs "GenerateOut0" and "GenerateOut1"
	...

?

> > > +static struct platform_driver xilinx_timer_driver = {
> > > +	.probe = xilinx_timer_probe,
> > > +	.remove = xilinx_timer_remove,
> > > +	.driver = {
> > > +		.name = "xilinx-timer",
> > 
> > Doesn't this give a wrong impression as this is a PWM driver, not a
> > timer driver?

This directly relates to the fact that the timer driver and the pwm
driver (seem to) bind on the same compatible as already mentioned above.
The dt people didn't agree to this yet, did they?

> Perhaps. Though this is the PWM driver for the Xilinx AXI timer, not the
> Xilinx AXI PWM.

I would be happier with "xilinx-timer-pwm" then.

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

Re: [PATCH v10 3/3] pwm: Add support for Xilinx AXI Timer

[Sean Anderson]

Hi Uwe,

On 11/19/21 3:43 AM, Uwe Kleine-König wrote:
> Hello Sean,
>
> On Thu, Nov 18, 2021 at 04:08:45PM -0500, Sean Anderson wrote:
>> On 11/18/21 4:28 AM, Uwe Kleine-König wrote:
>> > On Fri, Nov 12, 2021 at 01:55:04PM -0500, Sean Anderson wrote:
>> > > [...]
>> > > +	/* cycles has a max of 2^32 + 2 */
>> > > +	return DIV64_U64_ROUND_CLOSEST(cycles * NSEC_PER_SEC,
>> > > +				       clk_get_rate(priv->clk));
>> >
>> > Please round up here.
>>
>> Please document the correct rounding mode you expect. The last time we
>> discussed this (3 months ago), you only said that rounding down was
>> incorrect...
>
> I think you refer to
> https://lore.kernel.org/linux-pwm/20210817180407.ru4prwu344dxpynu@pengutronix.de
> here, right? I agree that I could have been a bit more explicit here.
>
> .apply should first round down .period to the next achievable setting
> and then .duty_cycle should be round down to the next achievable setting
> (in combination with the chosen period).
>
> To get .apply ∘ .get_state idempotent (i.e. if I apply the result from
> get_state there are no changes), .get_state has to round up.
>
> After our longer discussion about v4 I would have expected that this was
> already obvious. There you wrote[1]:
>
>> * The apply_state function shall only round the requested period down, and
>>    may do so by no more than one unit cycle. If the requested period is
>>    unrepresentable by the PWM, the apply_state function shall return
>>    -ERANGE.
>> * The apply_state function shall only round the requested duty cycle
>>    down. The apply_state function shall not return an error unless there
>>    is no duty cycle less than the requested duty cycle which is
>>    representable by the PWM.
>> * After applying a state returned by round_state with apply_state,
>>    get_state must return that state.
>
> The requirement to round up is a direct consequence of these three
> points, which I confirmed (apart from some wording issues).
>
> [1] https://lore.kernel.org/linux-pwm/ddd2ad0c-1dff-c437-17a6-4c7be72c2fce@seco.com

Ok, will fix. But again, a little something in
Documentation/driver-api/pwm.rst would help a lot.

>> > > +	period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
>> > > +	period_cycles = mul_u64_u32_div(period_cycles, rate, NSEC_PER_SEC);
>> > > +	if (period_cycles < 2 || period_cycles - 2 > priv->max)
>> > > +		return -ERANGE;
>> >
>> > if period_cycles - 2 > priv->max the right reaction is to do
>> >
>> > 	period_cycles = priv->max + 2
>>
>> It has been 5 months since we last talked about this, and yet you have
>> not submitted any patches for a "pwm_round_rate" function. Forgive me if
>> I am reticent to implement forward compatibility for an API which shows
>> no signs of appearing.
>
> This requirement is not only for round_state. It's also to get some
> common behaviour for at least new drivers. The primary goal here is to
> make the result for pwm_apply more predictable.

The behavior you specify is *not* common. No drivers currently round in
the manner you specify here. In fact, returning -ERANGE or -EINVAL is
far more common than attempting to handle this case. If you would like
new drivers to use a new algorithm, I suggest first converting existing
drivers. I think it is unreasonable to hold new drivers to a standard
which no existing driver is held to.

>> > > +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;
>> >
>> > The name "pwm" is usually reserved for struct pwm_device pointers. A
>> > typical name for this would be xlnxpwm or ddata.
>>
>> I suppose. However, no variables of struct pwm_device are used in
>> this driver.
>
> Still it provokes wrong expectations when reading
>
> 	platform_set_drvdata(pdev, pwm);
>
> in a pwm driver.

The second most common use of this function in drivers/pwm is the above usage.

$ git grep -h platform_set_drvdata v5.15 drivers/pwm/ | sort | uniq -c | sort -n
       1 	platform_set_drvdata(pdev, atmel_pwm);
       1 	platform_set_drvdata(pdev, bpc);
       1 	platform_set_drvdata(pdev, ddata);
       1 	platform_set_drvdata(pdev, ec_pwm);
       1 	platform_set_drvdata(pdev, fpc);
       1 	platform_set_drvdata(pdev, ip);
       1 	platform_set_drvdata(pdev, lpc18xx_pwm);
       1 	platform_set_drvdata(pdev, lpwm);
       1 	platform_set_drvdata(pdev, mdp);
       1 	platform_set_drvdata(pdev, omap);
       1 	platform_set_drvdata(pdev, p);
       1 	platform_set_drvdata(pdev, pwm_chip);
       1 	platform_set_drvdata(pdev, rcar_pwm);
       1 	platform_set_drvdata(pdev, spc);
       1 	platform_set_drvdata(pdev, tcbpwm);
       1 	platform_set_drvdata(pdev, tpm);
       1 	platform_set_drvdata(pdev, tpu);
       3 	platform_set_drvdata(pdev, chip);
       3 	platform_set_drvdata(pdev, priv);
       4 	platform_set_drvdata(pdev, pwm);
       6 	platform_set_drvdata(pdev, pc);

With other contenders being "pc", "chip", "pwm_chip", and "p". This used
to be more common, but several examples have been converted to
devm_pwmchip_add. To say that such a variable (used once!) "provokes the
wrong expectations" would be to have expectations misaligned with the
corpus of existing drivers.

>> > > +	u32 pwm_cells, one_timer, width;
>> > > +	void __iomem *regs;
>> > > +
>> > > +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
>> > > +	if (ret == -EINVAL)
>> > > +		return -ENODEV;
>> >
>> > A comment about why this is done would be great.
>>
>> OK. How about:
>>
>> /* If there are no #pwm-cells, this binding is for a timer and not a PWM */
>
> Fine. Does that mean the timer driver won't bind in the presence of the
> #pwm-cells property, and the timer driver uses the same compatible?
> Sounds a bit strange to me.

Correct. See below.

>> > > +	/*
>> > > +	 * The polarity of the generate outputs must be active high for PWM
>> >
>> > s/generate/generated/
>>
>> The signals I am referring to are called "GenerateOut0" and
>> "GenerateOut1".
>
> Then maybe:
>
> 	The polarity of the outputs "GenerateOut0" and "GenerateOut1"
> 	...
>
> ?

The exact wording of the configuration option is

> Active state of Generate Out signal

with a drop-down to select between "Active High" and "Active Low". So
the most exact way to specify this would be

	The polarity of the Generate Out signals must be...

>> > > +static struct platform_driver xilinx_timer_driver = {
>> > > +	.probe = xilinx_timer_probe,
>> > > +	.remove = xilinx_timer_remove,
>> > > +	.driver = {
>> > > +		.name = "xilinx-timer",
>> >
>> > Doesn't this give a wrong impression as this is a PWM driver, not a
>> > timer driver?
>
> This directly relates to the fact that the timer driver and the pwm
> driver (seem to) bind on the same compatible as already mentioned above.
> The dt people didn't agree to this yet, did they?

Rob Herring has acked the binding. And switching based on the presence
of #pwm-cells was his idea in the first place:

> If a PWM, perhaps you want a '#pwm-cells' property which can serve as
> the hint to configure as a PWM.

As I understand it, the compatible should be the same if the hardware is
the same. Ideally, this sort of thing would be configurable by userspace
at runtime, but timers get probed so early that we have to use something
in the devicetree.

[1] https://lore.kernel.org/linux-pwm/20210513021631.GA878860@robh.at.kernel.org/

>> Perhaps. Though this is the PWM driver for the Xilinx AXI timer, not the
>> Xilinx AXI PWM.
>
> I would be happier with "xilinx-timer-pwm" then.

I've changed it to "xilinx_pwm".

--Sean

Re: [PATCH v10 3/3] pwm: Add support for Xilinx AXI Timer

[Uwe Kleine-König]

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

Hello Sean,

On Mon, Nov 22, 2021 at 12:32:19PM -0500, Sean Anderson wrote:
> On 11/19/21 3:43 AM, Uwe Kleine-König wrote:
> > On Thu, Nov 18, 2021 at 04:08:45PM -0500, Sean Anderson wrote:
> > > On 11/18/21 4:28 AM, Uwe Kleine-König wrote:
> > > > On Fri, Nov 12, 2021 at 01:55:04PM -0500, Sean Anderson wrote:
> > > > > [...]
> > > > > +	/* cycles has a max of 2^32 + 2 */
> > > > > +	return DIV64_U64_ROUND_CLOSEST(cycles * NSEC_PER_SEC,
> > > > > +				       clk_get_rate(priv->clk));
> > > >
> > > > Please round up here.
> > > 
> > > Please document the correct rounding mode you expect. The last time we
> > > discussed this (3 months ago), you only said that rounding down was
> > > incorrect...
> > 
> > I think you refer to
> > https://lore.kernel.org/linux-pwm/20210817180407.ru4prwu344dxpynu@pengutronix.de
> > here, right? I agree that I could have been a bit more explicit here.
> > 
> > .apply should first round down .period to the next achievable setting
> > and then .duty_cycle should be round down to the next achievable setting
> > (in combination with the chosen period).
> > 
> > To get .apply ∘ .get_state idempotent (i.e. if I apply the result from
> > get_state there are no changes), .get_state has to round up.
> > 
> > After our longer discussion about v4 I would have expected that this was
> > already obvious. There you wrote[1]:
> > 
> > > * The apply_state function shall only round the requested period down, and
> > >    may do so by no more than one unit cycle. If the requested period is
> > >    unrepresentable by the PWM, the apply_state function shall return
> > >    -ERANGE.
> > > * The apply_state function shall only round the requested duty cycle
> > >    down. The apply_state function shall not return an error unless there
> > >    is no duty cycle less than the requested duty cycle which is
> > >    representable by the PWM.
> > > * After applying a state returned by round_state with apply_state,
> > >    get_state must return that state.
> > 
> > The requirement to round up is a direct consequence of these three
> > points, which I confirmed (apart from some wording issues).
> > 
> > [1] https://lore.kernel.org/linux-pwm/ddd2ad0c-1dff-c437-17a6-4c7be72c2fce@seco.com
> 
> Ok, will fix. But again, a little something in
> Documentation/driver-api/pwm.rst would help a lot.

Ack, will take another attempt to care for that.

> > > > > +	period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
> > > > > +	period_cycles = mul_u64_u32_div(period_cycles, rate, NSEC_PER_SEC);
> > > > > +	if (period_cycles < 2 || period_cycles - 2 > priv->max)
> > > > > +		return -ERANGE;
> > > >
> > > > if period_cycles - 2 > priv->max the right reaction is to do
> > > >
> > > > 	period_cycles = priv->max + 2
> > > 
> > > It has been 5 months since we last talked about this, and yet you have
> > > not submitted any patches for a "pwm_round_rate" function. Forgive me if
> > > I am reticent to implement forward compatibility for an API which shows
> > > no signs of appearing.
> > 
> > This requirement is not only for round_state. It's also to get some
> > common behaviour for at least new drivers. The primary goal here is to
> > make the result for pwm_apply more predictable.
> 
> The behavior you specify is *not* common. No drivers currently round in
> the manner you specify here.

Hmm, if this is true I failed during the reviews before.

Looking through the last added drivers:

 - visconti: There is no division involved, so there is no rounding
   issue. Also period is round down if a too high value is requested.
 - ntxec: There is no get_state callback because the hardware state
   cannot be read out. Period is reduced as requested.
 - raspberrypi-poe: Rounds up in .get_state() and down in .apply(). (A
   bit special as this HW has a fixed period.)
 - intel-lgm: Rounds up in .get_state() and down in .apply(). Ditto this
   is a fixed-period driver and only too small periods are refused.
 - dwc: This is indeed wrong. I didn't review the finally merged
   version, but pointed out the driver being wrong in v2
   (https://lore.kernel.org/linux-pwm/20200524201116.pc7jmffr6jxlwren@pengutronix.de)
 - keembay: Rounds up in .get_state and dowan in .apply()
   (Though the detection of a too high period might be broken?! Didn't
   look into the details.)
 - pwm-sl28cpld is aligned to this behaviour (though this is a bit of a
   special case as there is no rounding involved). Refusing too big
   periods (only) is done right in it.
 - iqs620a: Is aligned to my request, too
 - pwm-sprd: This is wrong, too. My review comments were not addressed
   here either (e.g.
   https://lore.kernel.org/linux-pwm/20190814150304.x44lalde3cwp67ge@pengutronix.de)

So while the situation isn't ideal, it's not as worse as you picture it.

> In fact, returning -ERANGE or -EINVAL is
> far more common than attempting to handle this case. If you would like
> new drivers to use a new algorithm, I suggest first converting existing
> drivers. I think it is unreasonable to hold new drivers to a standard
> which no existing driver is held to.

In the past Thierry opposed to adapting existing drivers. :-\
Having said that being more strict for new drivers isn't that uncommon.
For example I expect it wouldn't be possible to get something like
drivers/tty/serial/8250 into mainline today.

> > > > > +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;
> > > >
> > > > The name "pwm" is usually reserved for struct pwm_device pointers. A
> > > > typical name for this would be xlnxpwm or ddata.
> > > 
> > > I suppose. However, no variables of struct pwm_device are used in
> > > this driver.
> > 
> > Still it provokes wrong expectations when reading
> > 
> > 	platform_set_drvdata(pdev, pwm);
> > 
> > in a pwm driver.
> 
> The second most common use of this function in drivers/pwm is the above usage.
> 
> $ git grep -h platform_set_drvdata v5.15 drivers/pwm/ | sort | uniq -c | sort -n
>       1 	platform_set_drvdata(pdev, atmel_pwm);
>       1 	platform_set_drvdata(pdev, bpc);
>       1 	platform_set_drvdata(pdev, ddata);
>       1 	platform_set_drvdata(pdev, ec_pwm);
>       1 	platform_set_drvdata(pdev, fpc);
>       1 	platform_set_drvdata(pdev, ip);
>       1 	platform_set_drvdata(pdev, lpc18xx_pwm);
>       1 	platform_set_drvdata(pdev, lpwm);
>       1 	platform_set_drvdata(pdev, mdp);
>       1 	platform_set_drvdata(pdev, omap);
>       1 	platform_set_drvdata(pdev, p);
>       1 	platform_set_drvdata(pdev, pwm_chip);
>       1 	platform_set_drvdata(pdev, rcar_pwm);
>       1 	platform_set_drvdata(pdev, spc);
>       1 	platform_set_drvdata(pdev, tcbpwm);
>       1 	platform_set_drvdata(pdev, tpm);
>       1 	platform_set_drvdata(pdev, tpu);
>       3 	platform_set_drvdata(pdev, chip);
>       3 	platform_set_drvdata(pdev, priv);
>       4 	platform_set_drvdata(pdev, pwm);
>       6 	platform_set_drvdata(pdev, pc);

Ack, this are all "old" drivers (i.e. img (2015), stmpe (2016), sun4i
(2015) and tegra(2012); "old" in the sense "before I engaged in pwm
reviews") ISTR that in this question Thierry agrees that only variables
with type struct pwm_chip * should be named pwm.

> With other contenders being "pc", "chip", "pwm_chip", and "p". This used
> to be more common, but several examples have been converted to
> devm_pwmchip_add. To say that such a variable (used once!) "provokes the
> wrong expectations" would be to have expectations misaligned with the
> corpus of existing drivers.

Yeah, I don't oppose to this interpretation. I'm not happy with the code
base. Reworking this is a big effort and difficult with the request to
not break assumption for existing drivers.

> > > > > +	u32 pwm_cells, one_timer, width;
> > > > > +	void __iomem *regs;
> > > > > +
> > > > > +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
> > > > > +	if (ret == -EINVAL)
> > > > > +		return -ENODEV;
> > > >
> > > > A comment about why this is done would be great.
> > > 
> > > OK. How about:
> > > 
> > > /* If there are no #pwm-cells, this binding is for a timer and not a PWM */
> > 
> > Fine. Does that mean the timer driver won't bind in the presence of the
> > #pwm-cells property, and the timer driver uses the same compatible?
> > Sounds a bit strange to me.
> 
> Correct. See below.
> 
> > > > > +	/*
> > > > > +	 * The polarity of the generate outputs must be active high for PWM
> > > >
> > > > s/generate/generated/
> > > 
> > > The signals I am referring to are called "GenerateOut0" and
> > > "GenerateOut1".
> > 
> > Then maybe:
> > 
> > 	The polarity of the outputs "GenerateOut0" and "GenerateOut1"
> > 	...
> > 
> > ?
> 
> The exact wording of the configuration option is
> 
> > Active state of Generate Out signal
> 
> with a drop-down to select between "Active High" and "Active Low". So
> the most exact way to specify this would be
> 
> 	The polarity of the Generate Out signals must be...
> 
> > > > > +static struct platform_driver xilinx_timer_driver = {
> > > > > +	.probe = xilinx_timer_probe,
> > > > > +	.remove = xilinx_timer_remove,
> > > > > +	.driver = {
> > > > > +		.name = "xilinx-timer",
> > > >
> > > > Doesn't this give a wrong impression as this is a PWM driver, not a
> > > > timer driver?
> > 
> > This directly relates to the fact that the timer driver and the pwm
> > driver (seem to) bind on the same compatible as already mentioned above.
> > The dt people didn't agree to this yet, did they?
> 
> Rob Herring has acked the binding. And switching based on the presence
> of #pwm-cells was his idea in the first place:
> 
> > If a PWM, perhaps you want a '#pwm-cells' property which can serve as
> > the hint to configure as a PWM.
> 
> As I understand it, the compatible should be the same if the hardware is
> the same. Ideally, this sort of thing would be configurable by userspace
> at runtime, but timers get probed so early that we have to use something
> in the devicetree.
> 
> [1] https://lore.kernel.org/linux-pwm/20210513021631.GA878860@robh.at.kernel.org/

OK, then my expectation that Rob won't agree was wrong. Fine then.

> > > Perhaps. Though this is the PWM driver for the Xilinx AXI timer, not the
> > > Xilinx AXI PWM.
> > 
> > I would be happier with "xilinx-timer-pwm" then.
> 
> I've changed it to "xilinx_pwm".

OK.

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

Re: [PATCH v10 1/3] microblaze: timer: Remove unused properties

[Michal Simek]

On 11/12/21 19:55, Sean Anderson wrote:
> This removes properties not used by either the PWM or timer drivers.
> This lets us set additionalProperties: false.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> Acked-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> (no changes since v8)
> 
> Changes in v8:
> - Remove additional properties from microblaze device tree
> 
>   arch/microblaze/boot/dts/system.dts | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/arch/microblaze/boot/dts/system.dts b/arch/microblaze/boot/dts/system.dts
> index b7ee1056779e..22252451ec09 100644
> --- a/arch/microblaze/boot/dts/system.dts
> +++ b/arch/microblaze/boot/dts/system.dts
> @@ -347,12 +347,7 @@ xps_timer_1: timer@83c00000 {
>   			interrupts = < 3 2 >;
>   			reg = < 0x83c00000 0x10000 >;
>   			xlnx,count-width = <0x20>;
> -			xlnx,family = "virtex5";
> -			xlnx,gen0-assert = <0x1>;
> -			xlnx,gen1-assert = <0x1>;
>   			xlnx,one-timer-only = <0x0>;
> -			xlnx,trig0-assert = <0x1>;
> -			xlnx,trig1-assert = <0x1>;
>   		} ;
>   	} ;
>   }  ;
> 


FYI: This is merged to Linus tree already. It means you can remove this 
patch from this series.

M

Re: [PATCH v10 1/3] microblaze: timer: Remove unused properties

[Sean Anderson]

On 11/15/21 4:27 AM, Michal Simek wrote:
> 
> 
> On 11/12/21 19:55, Sean Anderson wrote:
>> This removes properties not used by either the PWM or timer drivers.
>> This lets us set additionalProperties: false.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> Acked-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> (no changes since v8)
>>
>> Changes in v8:
>> - Remove additional properties from microblaze device tree
>>
>>   arch/microblaze/boot/dts/system.dts | 5 -----
>>   1 file changed, 5 deletions(-)
>>
>> diff --git a/arch/microblaze/boot/dts/system.dts b/arch/microblaze/boot/dts/system.dts
>> index b7ee1056779e..22252451ec09 100644
>> --- a/arch/microblaze/boot/dts/system.dts
>> +++ b/arch/microblaze/boot/dts/system.dts
>> @@ -347,12 +347,7 @@ xps_timer_1: timer@83c00000 {
>>               interrupts = < 3 2 >;
>>               reg = < 0x83c00000 0x10000 >;
>>               xlnx,count-width = <0x20>;
>> -            xlnx,family = "virtex5";
>> -            xlnx,gen0-assert = <0x1>;
>> -            xlnx,gen1-assert = <0x1>;
>>               xlnx,one-timer-only = <0x0>;
>> -            xlnx,trig0-assert = <0x1>;
>> -            xlnx,trig1-assert = <0x1>;
>>           } ;
>>       } ;
>>   }  ;
>>
> 
> 
> FYI: This is merged to Linus tree already. It means you can remove this patch from this series.
> 
> M

Ah, I didn't realize since I had not seen any message about it. Thanks for letting me know.

--Sean