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

[PATCH 1/2] 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 many 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 properties
must be kept as they are.

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

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

diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
new file mode 100644
index 000000000000..3a0abd940336
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
@@ -0,0 +1,91 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
+
+maintainers:
+  - Sean Anderson <sean.anderson@seco.com>
+
+properties:
+  compatible:
+    items:
+      - const: xlnx,axi-timer-2.0
+      - const: xlnx,xps-timer-1.00.a
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: s_axi_aclk
+
+  reg:
+    maxItems: 1
+
+  xlnx,count-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 8
+    maximum: 32
+    description:
+      The width of the counters, in bits.
+
+  xlnx,gen0-assert:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    description:
+      The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
+
+  xlnx,gen1-assert:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    description:
+      The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
+
+  xlnx,one-timer-only:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    description:
+      Whether only one timer is present in this block.
+
+  xlnx,trig0-assert:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    description:
+      The polarity of the capturetrig0 signal. 0 for active-low, 1 for active-high.
+
+  xlnx,trig1-assert:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    description:
+      The polarity of the capturetrig1 signal. 0 for active-low, 1 for active-high.
+
+required:
+  - compatible
+  - clocks
+  - reg
+  - xlnx,count-width
+  - xlnx,gen0-assert
+  - xlnx,gen1-assert
+  - xlnx,one-timer-only
+  - xlnx,trig0-assert
+  - xlnx,trig1-assert
+
+additionalProperties: true
+
+examples:
+  - |
+    axi_timer_0: timer@800e0000 {
+        clock-frequency = <99999001>;
+        clock-names = "s_axi_aclk";
+        clocks = <&zynqmp_clk 71>;
+        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
+        reg = <0x0 0x800e0000 0x0 0x10000>;
+        xlnx,count-width = <0x20>;
+        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 2/2] 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. There is another driver for this device located
at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
This driver was written with reference to Xilinx DS764 for v1.03.a [1].

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

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

---

 arch/arm64/configs/defconfig |   1 +
 drivers/pwm/Kconfig          |  11 ++
 drivers/pwm/Makefile         |   1 +
 drivers/pwm/pwm-xilinx.c     | 322 +++++++++++++++++++++++++++++++++++
 4 files changed, 335 insertions(+)
 create mode 100644 drivers/pwm/pwm-xilinx.c

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 08c6f769df9a..81794209f287 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y
 CONFIG_PWM_SL28CPLD=m
 CONFIG_PWM_SUN4I=m
 CONFIG_PWM_TEGRA=m
+CONFIG_PWM_XILINX=m
 CONFIG_SL28CPLD_INTC=y
 CONFIG_QCOM_PDC=y
 CONFIG_RESET_IMX7=y
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index d3371ac7b871..01e62928f4bf 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -628,4 +628,15 @@ 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 !MICROBLAZE
+	help
+	  PWM framework driver for Xilinx LogiCORE IP AXI Timers. This
+	  timer is typically a soft core which may be present in Xilinx
+	  FPGAs. 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 d3879619bd76..fc1bd6ccc9ed 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -59,3 +59,4 @@ obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
 obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.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..240bd2993f97
--- /dev/null
+++ b/drivers/pwm/pwm-xilinx.c
@@ -0,0 +1,322 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
+ */
+#include <asm/io.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/debugfs.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.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)
+
+/* Bits we need to set/clear to enable PWM mode, not including CASC */
+#define TCSR_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)
+#define TCSR_CLEAR (TCSR_MDT | TCSR_LOAD)
+
+#define NSEC_PER_SEC_ULL 1000000000ULL
+
+/**
+ * struct xilinx_pwm_device - Driver data for Xilinx AXI timer PWM driver
+ * @chip: PWM controller chip
+ * @clk: Parent clock
+ * @regs: Base address of this device
+ * @width: Width of the counters, in bits
+ */
+struct xilinx_pwm_device {
+	struct pwm_chip chip;
+	struct clk *clk;
+	void __iomem *regs;
+	unsigned int width;
+};
+
+static inline struct xilinx_pwm_device *xilinx_pwm_chip_to_device(struct pwm_chip *chip)
+{
+	return container_of(chip, struct xilinx_pwm_device, chip);
+}
+
+static bool xilinx_pwm_is_enabled(u32 tcsr0, u32 tcsr1)
+{
+	return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) ||
+		 ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR);
+}
+
+static u32 xilinx_pwm_calc_tlr(struct xilinx_pwm_device *pwm, u32 tcsr,
+			       unsigned int period)
+{
+	u64 cycles = DIV_ROUND_DOWN_ULL(period * clk_get_rate(pwm->clk),
+					NSEC_PER_SEC_ULL);
+
+	if (tcsr & TCSR_UDT)
+		return cycles - 2;
+	else
+		return (BIT_ULL(pwm->width) - 1) - cycles + 2;
+}
+
+static unsigned int xilinx_pwm_get_period(struct xilinx_pwm_device *pwm,
+					  u32 tlr, u32 tcsr)
+{
+	u64 cycles;
+
+	if (tcsr & TCSR_UDT)
+		cycles = tlr + 2;
+	else
+		cycles = (BIT_ULL(pwm->width) - 1) - tlr + 2;
+
+	return DIV_ROUND_DOWN_ULL(cycles * NSEC_PER_SEC_ULL,
+				  clk_get_rate(pwm->clk));
+}
+
+static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
+			    const struct pwm_state *state)
+{
+	struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip);
+	u32 tlr0, tlr1;
+	u32 tcsr0 = readl(pwm->regs + TCSR0);
+	u32 tcsr1 = readl(pwm->regs + TCSR1);
+	bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	if (!enabled && state->enabled)
+		clk_rate_exclusive_get(pwm->clk);
+
+	tlr0 = xilinx_pwm_calc_tlr(pwm, tcsr0, state->period);
+	tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle);
+	writel(tlr0, pwm->regs + TLR0);
+	writel(tlr1, pwm->regs + TLR1);
+
+	if (state->enabled) {
+		/* Only touch the TCSRs if we aren't already running */
+		if (!enabled) {
+			/* Load TLR into TCR */
+			writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0);
+			writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1);
+			/* Enable timers all at once with ENALL */
+			tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
+			tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
+			writel(tcsr0, pwm->regs + TCSR0);
+			writel(tcsr1, pwm->regs + TCSR1);
+		}
+	} else {
+		writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0);
+		writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1);
+	}
+
+	if (enabled && !state->enabled)
+		clk_rate_exclusive_put(pwm->clk);
+
+	return 0;
+}
+
+static void xilinx_pwm_get_state(struct pwm_chip *chip,
+				 struct pwm_device *unused,
+				 struct pwm_state *state)
+{
+	struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip);
+	u32 tlr0, tlr1, tcsr0, tcsr1;
+
+	tlr0 = readl(pwm->regs + TLR0);
+	tlr1 = readl(pwm->regs + TLR1);
+	tcsr0 = readl(pwm->regs + TCSR0);
+	tcsr1 = readl(pwm->regs + TCSR1);
+
+	state->period = xilinx_pwm_get_period(pwm, tlr0, tcsr0);
+	state->duty_cycle = xilinx_pwm_get_period(pwm, tlr1, tcsr1);
+	state->enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
+	state->polarity = PWM_POLARITY_NORMAL;
+}
+
+static const struct pwm_ops xilinx_pwm_ops = {
+	.apply = xilinx_pwm_apply,
+	.get_state = xilinx_pwm_get_state,
+};
+
+static struct dentry *debug_dir;
+
+#define DEBUG_REG(reg) { .name = #reg, .offset = (reg), }
+static struct debugfs_reg32 xilinx_pwm_reg32[] = {
+	DEBUG_REG(TCSR0),
+	DEBUG_REG(TLR0),
+	DEBUG_REG(TCR0),
+	DEBUG_REG(TCSR1),
+	DEBUG_REG(TLR1),
+	DEBUG_REG(TCR1),
+};
+
+static int xilinx_pwm_debug_create(struct xilinx_pwm_device *pwm)
+{
+	struct debugfs_regset32 *regset;
+
+	if (!IS_ENABLED(CONFIG_DEBUG_FS))
+		return 0;
+
+	regset = devm_kzalloc(pwm->chip.dev, sizeof(*regset), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	regset->regs = xilinx_pwm_reg32;
+	regset->nregs = ARRAY_SIZE(xilinx_pwm_reg32);
+	regset->base = pwm->regs;
+
+	debugfs_create_regset32(dev_name(pwm->chip.dev), 0400, debug_dir,
+				regset);
+	return 0;
+}
+
+static int xilinx_pwm_probe(struct platform_device *pdev)
+{
+	bool enabled;
+	int i, ret;
+	struct device *dev = &pdev->dev;
+	struct xilinx_pwm_device *pwm;
+	u32 one_timer;
+
+	ret = of_property_read_u32(dev->of_node, "xlnx,one-timer-only",
+				   &one_timer);
+	if (ret || one_timer) {
+		dev_err(dev, "two timers are needed for PWM mode\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < 2; i++) {
+		char fmt[] = "xlnx,gen%u-assert";
+		char buf[sizeof(fmt)];
+		u32 gen;
+
+		snprintf(buf, sizeof(buf), fmt, i);
+		ret = of_property_read_u32(dev->of_node, buf, &gen);
+		if (ret || !gen) {
+			dev_err(dev, "generateout%u must be active high\n", i);
+			return -EINVAL;
+		}
+	}
+
+	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, pwm);
+
+	pwm->chip.dev = &pdev->dev;
+	pwm->chip.ops = &xilinx_pwm_ops;
+	pwm->chip.base = -1;
+	pwm->chip.npwm = 1;
+
+	pwm->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pwm->regs))
+		return PTR_ERR(pwm->regs);
+
+	ret = of_property_read_u32(dev->of_node, "xlnx,count-width", &pwm->width);
+	if (ret) {
+		dev_err(dev, "missing counter width\n");
+		return -EINVAL;
+	}
+
+	pwm->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pwm->clk))
+		return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n");
+
+	ret = clk_prepare_enable(pwm->clk);
+	if (ret) {
+		dev_err(dev, "clock enable failed\n");
+		return ret;
+	}
+
+	ret = xilinx_pwm_debug_create(pwm);
+	if (ret) {
+		clk_disable_unprepare(pwm->clk);
+		return ret;
+	}
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret) {
+		clk_disable_unprepare(pwm->clk);
+		return ret;
+	}
+
+	enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
+					readl(pwm->regs + TCSR1));
+	if (enabled)
+		clk_rate_exclusive_get(pwm->clk);
+
+	return ret;
+}
+
+static int xilinx_pwm_remove(struct platform_device *pdev)
+{
+	struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
+	bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
+					     readl(pwm->regs + TCSR1));
+
+	if (enabled)
+		clk_rate_exclusive_put(pwm->clk);
+	clk_disable_unprepare(pwm->clk);
+	return pwmchip_remove(&pwm->chip);
+}
+
+static const struct of_device_id xilinx_pwm_of_match[] = {
+	{ .compatible = "xlnx,xps-timer-1.00.a" },
+	{ .compatible = "xlnx,axi-timer-2.0" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xilinx_pwm_of_match);
+
+static struct platform_driver xilinx_pwm_driver = {
+	.probe = xilinx_pwm_probe,
+	.remove = xilinx_pwm_remove,
+	.driver = {
+		.name = "xilinx-pwm",
+		.of_match_table = of_match_ptr(xilinx_pwm_of_match),
+	},
+};
+
+static int __init xilinx_pwm_init(void)
+{
+	int ret = platform_driver_register(&xilinx_pwm_driver);
+
+	if (ret)
+		return ret;
+
+	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
+		debug_dir = debugfs_create_dir(xilinx_pwm_driver.driver.name, NULL);
+		if (IS_ERR(debug_dir)) {
+			platform_driver_unregister(&xilinx_pwm_driver);
+			return PTR_ERR(debug_dir);
+		}
+	}
+	return 0;
+}
+module_init(xilinx_pwm_init);
+
+static void __exit xilinx_pwm_exit(void)
+{
+	platform_driver_unregister(&xilinx_pwm_driver);
+	if (IS_ENABLED(CONFIG_DEBUG_FS))
+		debugfs_remove_recursive(debug_dir);
+}
+module_exit(xilinx_pwm_exit);
+
+MODULE_ALIAS("platform:xilinx-pwm");
+MODULE_DESCRIPTION("Xilinx LogiCORE IP AXI Timer PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

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

[kernel test robot]

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

Hi Sean,

Thank you for the patch! Yet something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210504-054451
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/164c8df701eef0c9509d544501a1de5c406e93cb
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210504-054451
        git checkout 164c8df701eef0c9509d544501a1de5c406e93cb
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=sh 

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

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

ERROR: modpost: "__delay" [drivers/net/mdio/mdio-cavium.ko] undefined!
>> ERROR: modpost: "clk_rate_exclusive_get" [drivers/pwm/pwm-xilinx.ko] undefined!
>> ERROR: modpost: "clk_rate_exclusive_put" [drivers/pwm/pwm-xilinx.ko] undefined!
ERROR: modpost: "__udivdi3" [fs/btrfs/btrfs.ko] undefined!
ERROR: modpost: "__umoddi3" [fs/btrfs/btrfs.ko] undefined!

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for SND_ATMEL_SOC_PDC
   Depends on SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && HAS_DMA
   Selected by
   - SND_ATMEL_SOC_SSC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC
   - SND_ATMEL_SOC_SSC_PDC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && ATMEL_SSC

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

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

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

[Alvaro Gamez]

[ Sorry if you receive this twice, I think gmail is skipping the
emails I send via mutt so I'm resending this via the web interface ]

Hi,

On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote:
> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
> found on Xilinx FPGAs. There is another driver for this device located
> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
>
> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>

I wrote a patch to support this back in 2018, being the main difference that
I did try to make this PWM driver available also for Microblaze systems. Of
course, since a lot of time has passed since, even the PWM API seems to have
changed, so my original patch is probably no longer applicable.

You can see that old discussion here

https://lore.kernel.org/patchwork/patch/803890/
https://lore.kernel.org/patchwork/patch/935728/

It didn't went much further because no one from the device tree list
gave any feedback to my proposal, so it just kind of died there since I
didn't have the time to push further. Also, I didn't really send a patch as
such to devicetree list, since it looked like we needed some previous
discussion about it.

> ---
>
>  arch/arm64/configs/defconfig |   1 +
>  drivers/pwm/Kconfig          |  11 ++
>  drivers/pwm/Makefile         |   1 +
>  drivers/pwm/pwm-xilinx.c     | 322 +++++++++++++++++++++++++++++++++++
>  4 files changed, 335 insertions(+)
>  create mode 100644 drivers/pwm/pwm-xilinx.c
>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 08c6f769df9a..81794209f287 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y
>  CONFIG_PWM_SL28CPLD=m
>  CONFIG_PWM_SUN4I=m
>  CONFIG_PWM_TEGRA=m
> +CONFIG_PWM_XILINX=m
>  CONFIG_SL28CPLD_INTC=y
>  CONFIG_QCOM_PDC=y
>  CONFIG_RESET_IMX7=y
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index d3371ac7b871..01e62928f4bf 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -628,4 +628,15 @@ 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 !MICROBLAZE

Here is probably the main difference between your patch and mine which
instead of depending on !MICROBLAZE had a dependency on ARCH_ZYNQ || MICROBLAZE
-apart from mine being too old to apply and useless now , of course-

> + help
> +  PWM framework driver for Xilinx LogiCORE IP AXI Timers. This
> +  timer is typically a soft core which may be present in Xilinx
> +  FPGAs. 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 d3879619bd76..fc1bd6ccc9ed 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -59,3 +59,4 @@ obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
>  obj-$(CONFIG_PWM_TWL) += pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.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..240bd2993f97
> --- /dev/null
> +++ b/drivers/pwm/pwm-xilinx.c
> @@ -0,0 +1,322 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
> + */
> +#include <asm/io.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/debugfs.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.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)
> +
> +/* Bits we need to set/clear to enable PWM mode, not including CASC */
> +#define TCSR_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)
> +#define TCSR_CLEAR (TCSR_MDT | TCSR_LOAD)
> +
> +#define NSEC_PER_SEC_ULL 1000000000ULL
> +
> +/**
> + * struct xilinx_pwm_device - Driver data for Xilinx AXI timer PWM driver
> + * @chip: PWM controller chip
> + * @clk: Parent clock
> + * @regs: Base address of this device
> + * @width: Width of the counters, in bits
> + */
> +struct xilinx_pwm_device {
> + struct pwm_chip chip;
> + struct clk *clk;
> + void __iomem *regs;
> + unsigned int width;
> +};
> +
> +static inline struct xilinx_pwm_device *xilinx_pwm_chip_to_device(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct xilinx_pwm_device, chip);
> +}
> +
> +static bool xilinx_pwm_is_enabled(u32 tcsr0, u32 tcsr1)
> +{
> + return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) ||
> + ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR);
> +}
> +
> +static u32 xilinx_pwm_calc_tlr(struct xilinx_pwm_device *pwm, u32 tcsr,
> +       unsigned int period)
> +{
> + u64 cycles = DIV_ROUND_DOWN_ULL(period * clk_get_rate(pwm->clk),
> + NSEC_PER_SEC_ULL);
> +
> + if (tcsr & TCSR_UDT)
> + return cycles - 2;
> + else
> + return (BIT_ULL(pwm->width) - 1) - cycles + 2;
> +}
> +
> +static unsigned int xilinx_pwm_get_period(struct xilinx_pwm_device *pwm,
> +  u32 tlr, u32 tcsr)
> +{
> + u64 cycles;
> +
> + if (tcsr & TCSR_UDT)
> + cycles = tlr + 2;
> + else
> + cycles = (BIT_ULL(pwm->width) - 1) - tlr + 2;
> +
> + return DIV_ROUND_DOWN_ULL(cycles * NSEC_PER_SEC_ULL,
> +  clk_get_rate(pwm->clk));
> +}
> +
> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> +    const struct pwm_state *state)
> +{
> + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip);
> + u32 tlr0, tlr1;
> + u32 tcsr0 = readl(pwm->regs + TCSR0);
> + u32 tcsr1 = readl(pwm->regs + TCSR1);
> + bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
> +
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -EINVAL;
> +
> + if (!enabled && state->enabled)
> + clk_rate_exclusive_get(pwm->clk);
> +
> + tlr0 = xilinx_pwm_calc_tlr(pwm, tcsr0, state->period);
> + tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle);
> + writel(tlr0, pwm->regs + TLR0);
> + writel(tlr1, pwm->regs + TLR1);
> +
> + if (state->enabled) {
> + /* Only touch the TCSRs if we aren't already running */
> + if (!enabled) {
> + /* Load TLR into TCR */
> + writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0);
> + writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1);
> + /* Enable timers all at once with ENALL */
> + tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
> + tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
> + writel(tcsr0, pwm->regs + TCSR0);
> + writel(tcsr1, pwm->regs + TCSR1);
> + }
> + } else {
> + writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0);
> + writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1);
> + }
> +
> + if (enabled && !state->enabled)
> + clk_rate_exclusive_put(pwm->clk);
> +
> + return 0;
> +}
> +
> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *unused,
> + struct pwm_state *state)
> +{
> + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip);
> + u32 tlr0, tlr1, tcsr0, tcsr1;
> +
> + tlr0 = readl(pwm->regs + TLR0);
> + tlr1 = readl(pwm->regs + TLR1);
> + tcsr0 = readl(pwm->regs + TCSR0);
> + tcsr1 = readl(pwm->regs + TCSR1);
> +
> + state->period = xilinx_pwm_get_period(pwm, tlr0, tcsr0);
> + state->duty_cycle = xilinx_pwm_get_period(pwm, tlr1, tcsr1);
> + state->enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
> + state->polarity = PWM_POLARITY_NORMAL;
> +}
> +
> +static const struct pwm_ops xilinx_pwm_ops = {
> + .apply = xilinx_pwm_apply,
> + .get_state = xilinx_pwm_get_state,
> +};
> +
> +static struct dentry *debug_dir;
> +
> +#define DEBUG_REG(reg) { .name = #reg, .offset = (reg), }
> +static struct debugfs_reg32 xilinx_pwm_reg32[] = {
> + DEBUG_REG(TCSR0),
> + DEBUG_REG(TLR0),
> + DEBUG_REG(TCR0),
> + DEBUG_REG(TCSR1),
> + DEBUG_REG(TLR1),
> + DEBUG_REG(TCR1),
> +};

I didn't add any DEBUG_FS entries, so I really like this. It would have
helped me while developing my own version of this and will probably be
useful for people debugging their hardware.

> +
> +static int xilinx_pwm_debug_create(struct xilinx_pwm_device *pwm)
> +{
> + struct debugfs_regset32 *regset;
> +
> + if (!IS_ENABLED(CONFIG_DEBUG_FS))
> + return 0;
> +
> + regset = devm_kzalloc(pwm->chip.dev, sizeof(*regset), GFP_KERNEL);
> + if (!pwm)
> + return -ENOMEM;
> +
> + regset->regs = xilinx_pwm_reg32;
> + regset->nregs = ARRAY_SIZE(xilinx_pwm_reg32);
> + regset->base = pwm->regs;
> +
> + debugfs_create_regset32(dev_name(pwm->chip.dev), 0400, debug_dir,
> + regset);
> + return 0;
> +}
> +
> +static int xilinx_pwm_probe(struct platform_device *pdev)
> +{
> + bool enabled;
> + int i, ret;
> + struct device *dev = &pdev->dev;
> + struct xilinx_pwm_device *pwm;
> + u32 one_timer;
> +
> + ret = of_property_read_u32(dev->of_node, "xlnx,one-timer-only",
> +   &one_timer);
> + if (ret || one_timer) {
> + dev_err(dev, "two timers are needed for PWM mode\n");
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < 2; i++) {
> + char fmt[] = "xlnx,gen%u-assert";
> + char buf[sizeof(fmt)];
> + u32 gen;
> +
> + snprintf(buf, sizeof(buf), fmt, i);
> + ret = of_property_read_u32(dev->of_node, buf, &gen);
> + if (ret || !gen) {
> + dev_err(dev, "generateout%u must be active high\n", i);
> + return -EINVAL;
> + }
> + }
> +
> + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> + if (!pwm)
> + return -ENOMEM;
> + platform_set_drvdata(pdev, pwm);
> +
> + pwm->chip.dev = &pdev->dev;
> + pwm->chip.ops = &xilinx_pwm_ops;
> + pwm->chip.base = -1;
> + pwm->chip.npwm = 1;
> +
> + pwm->regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(pwm->regs))
> + return PTR_ERR(pwm->regs);
> +
> + ret = of_property_read_u32(dev->of_node, "xlnx,count-width", &pwm->width);
> + if (ret) {
> + dev_err(dev, "missing counter width\n");
> + return -EINVAL;
> + }
> +
> + pwm->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(pwm->clk))
> + return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n");
> +
> + ret = clk_prepare_enable(pwm->clk);
> + if (ret) {
> + dev_err(dev, "clock enable failed\n");
> + return ret;
> + }
> +
> + ret = xilinx_pwm_debug_create(pwm);
> + if (ret) {
> + clk_disable_unprepare(pwm->clk);
> + return ret;
> + }
> +
> + ret = pwmchip_add(&pwm->chip);
> + if (ret) {
> + clk_disable_unprepare(pwm->clk);
> + return ret;
> + }
> +
> + enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
> + readl(pwm->regs + TCSR1));
> + if (enabled)
> + clk_rate_exclusive_get(pwm->clk);
> +
> + return ret;
> +}
> +
> +static int xilinx_pwm_remove(struct platform_device *pdev)
> +{
> + struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
> + bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
> +     readl(pwm->regs + TCSR1));
> +
> + if (enabled)
> + clk_rate_exclusive_put(pwm->clk);
> + clk_disable_unprepare(pwm->clk);
> + return pwmchip_remove(&pwm->chip);
> +}
> +
> +static const struct of_device_id xilinx_pwm_of_match[] = {
> + { .compatible = "xlnx,xps-timer-1.00.a" },
> + { .compatible = "xlnx,axi-timer-2.0" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, xilinx_pwm_of_match);

And here lies culprit of my patch not being accepted, since defining the
driver to be "xlnx,axi-timer-2.0" compatible while still allowing MICROBLAZE
to be set, did make the kernel to misconfigure the interrupt timer used by
the processor.


> +
> +static struct platform_driver xilinx_pwm_driver = {
> + .probe = xilinx_pwm_probe,
> + .remove = xilinx_pwm_remove,
> + .driver = {
> + .name = "xilinx-pwm",
> + .of_match_table = of_match_ptr(xilinx_pwm_of_match),
> + },
> +};
> +
> +static int __init xilinx_pwm_init(void)
> +{
> + int ret = platform_driver_register(&xilinx_pwm_driver);
> +
> + if (ret)
> + return ret;
> +
> + if (IS_ENABLED(CONFIG_DEBUG_FS)) {
> + debug_dir = debugfs_create_dir(xilinx_pwm_driver.driver.name, NULL);
> + if (IS_ERR(debug_dir)) {
> + platform_driver_unregister(&xilinx_pwm_driver);
> + return PTR_ERR(debug_dir);
> + }
> + }
> + return 0;
> +}
> +module_init(xilinx_pwm_init);
> +
> +static void __exit xilinx_pwm_exit(void)
> +{
> + platform_driver_unregister(&xilinx_pwm_driver);
> + if (IS_ENABLED(CONFIG_DEBUG_FS))
> + debugfs_remove_recursive(debug_dir);
> +}
> +module_exit(xilinx_pwm_exit);
> +
> +MODULE_ALIAS("platform:xilinx-pwm");
> +MODULE_DESCRIPTION("Xilinx LogiCORE IP AXI Timer PWM driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.25.1
>

All in all, I'd be very glad if this got accepted (I haven't tested it), but
I'd be still happier if some compromise could be found within the device
tree so that this driver can be used also for Microblaze systems without
interfering with the main timer used by such systems.

Best regards,

-- 
Alvaro G. M.

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

[Sean Anderson]

On 5/3/21 10:24 PM, kernel test robot wrote:
 > Hi Sean,
 >
 > Thank you for the patch! Yet something to improve:
 >
 > [auto build test ERROR on arm64/for-next/core]
 > [also build test ERROR on linux/master linus/master v5.12 next-20210503]
 > [cannot apply to pwm/for-next xlnx/master]
 > [If your patch is applied to the wrong git tree, kindly drop us a note.
 > And when submitting patch, we suggest to use '--base' as documented in
 > https://git-scm.com/docs/git-format-patch]
 >
 > url:    https://github.com/0day-ci/linux/commits/Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210504-054451
 > base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
 > config: sh-allmodconfig (attached as .config)
 > compiler: sh4-linux-gcc (GCC) 9.3.0
 > reproduce (this is a W=1 build):
 >          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
 >          chmod +x ~/bin/make.cross
 >          # https://github.com/0day-ci/linux/commit/164c8df701eef0c9509d544501a1de5c406e93cb
 >          git remote add linux-review https://github.com/0day-ci/linux
 >          git fetch --no-tags linux-review Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210504-054451
 >          git checkout 164c8df701eef0c9509d544501a1de5c406e93cb
 >          # save the attached .config to linux build tree
 >          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=sh
 >
 > If you fix the issue, kindly add following tag as appropriate
 > Reported-by: kernel test robot <lkp@intel.com>
 >
 > All errors (new ones prefixed by >>, old ones prefixed by <<):
 >
 > ERROR: modpost: "__delay" [drivers/net/mdio/mdio-cavium.ko] undefined!
 >>> ERROR: modpost: "clk_rate_exclusive_get" [drivers/pwm/pwm-xilinx.ko] undefined!
 >>> ERROR: modpost: "clk_rate_exclusive_put" [drivers/pwm/pwm-xilinx.ko] undefined!

These should be defined in drivers/sh/clk/core.c, because
CONFIG_HAVE_CLK is selected (see the definition in include/linux/clk.h)
but they are not.

+CC SH maintainers

--Sean

 > ERROR: modpost: "__udivdi3" [fs/btrfs/btrfs.ko] undefined!
 > ERROR: modpost: "__umoddi3" [fs/btrfs/btrfs.ko] undefined!
 >
 > Kconfig warnings: (for reference only)
 >     WARNING: unmet direct dependencies detected for SND_ATMEL_SOC_PDC
 >     Depends on SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && HAS_DMA
 >     Selected by
 >     - SND_ATMEL_SOC_SSC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC
 >     - SND_ATMEL_SOC_SSC_PDC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && ATMEL_SSC
 >
 > ---
 > 0-DAY CI Kernel Test Service, Intel Corporation
 > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
 >

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

[Alvaro G. M.]

Hi,

On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote:
> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
> found on Xilinx FPGAs. There is another driver for this device located
> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
> 
> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> 

I wrote a patch to support this back in 2018, being the main difference that
I did try to make this PWM driver available also for Microblaze systems. Of
course, since a lot of time has passed since, even the PWM API seems to have
changed, so my original patch is probably no longer applicable.

You can see that old discussion here

https://lore.kernel.org/patchwork/patch/803890/
https://lore.kernel.org/patchwork/patch/935728/

It didn't went much further because no one from the device tree list
gave any feedback to my proposal, so it just kind of died there since I
didn't have the time to push further. Also, I didn't really send a patch as
such to devicetree list, since it looked like we needed some previous
discussion about it.

> ---
> 
>  arch/arm64/configs/defconfig |   1 +
>  drivers/pwm/Kconfig          |  11 ++
>  drivers/pwm/Makefile         |   1 +
>  drivers/pwm/pwm-xilinx.c     | 322 +++++++++++++++++++++++++++++++++++
>  4 files changed, 335 insertions(+)
>  create mode 100644 drivers/pwm/pwm-xilinx.c
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 08c6f769df9a..81794209f287 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y
>  CONFIG_PWM_SL28CPLD=m
>  CONFIG_PWM_SUN4I=m
>  CONFIG_PWM_TEGRA=m
> +CONFIG_PWM_XILINX=m
>  CONFIG_SL28CPLD_INTC=y
>  CONFIG_QCOM_PDC=y
>  CONFIG_RESET_IMX7=y
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index d3371ac7b871..01e62928f4bf 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -628,4 +628,15 @@ 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 !MICROBLAZE

Here is probably the main difference between your patch and mine which
instead of depending on !MICROBLAZE had a dependency on ARCH_ZYNQ || MICROBLAZE
-apart from mine being too old to apply and useless now , of course-

> +	help
> +	  PWM framework driver for Xilinx LogiCORE IP AXI Timers. This
> +	  timer is typically a soft core which may be present in Xilinx
> +	  FPGAs. 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 d3879619bd76..fc1bd6ccc9ed 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -59,3 +59,4 @@ obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
>  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.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..240bd2993f97
> --- /dev/null
> +++ b/drivers/pwm/pwm-xilinx.c
> @@ -0,0 +1,322 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
> + */
> +#include <asm/io.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/debugfs.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.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)
> +
> +/* Bits we need to set/clear to enable PWM mode, not including CASC */
> +#define TCSR_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)
> +#define TCSR_CLEAR (TCSR_MDT | TCSR_LOAD)
> +
> +#define NSEC_PER_SEC_ULL 1000000000ULL
> +
> +/**
> + * struct xilinx_pwm_device - Driver data for Xilinx AXI timer PWM driver
> + * @chip: PWM controller chip
> + * @clk: Parent clock
> + * @regs: Base address of this device
> + * @width: Width of the counters, in bits
> + */
> +struct xilinx_pwm_device {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	void __iomem *regs;
> +	unsigned int width;
> +};
> +
> +static inline struct xilinx_pwm_device *xilinx_pwm_chip_to_device(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct xilinx_pwm_device, chip);
> +}
> +
> +static bool xilinx_pwm_is_enabled(u32 tcsr0, u32 tcsr1)
> +{
> +	return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) ||
> +		 ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR);
> +}
> +
> +static u32 xilinx_pwm_calc_tlr(struct xilinx_pwm_device *pwm, u32 tcsr,
> +			       unsigned int period)
> +{
> +	u64 cycles = DIV_ROUND_DOWN_ULL(period * clk_get_rate(pwm->clk),
> +					NSEC_PER_SEC_ULL);
> +
> +	if (tcsr & TCSR_UDT)
> +		return cycles - 2;
> +	else
> +		return (BIT_ULL(pwm->width) - 1) - cycles + 2;
> +}
> +
> +static unsigned int xilinx_pwm_get_period(struct xilinx_pwm_device *pwm,
> +					  u32 tlr, u32 tcsr)
> +{
> +	u64 cycles;
> +
> +	if (tcsr & TCSR_UDT)
> +		cycles = tlr + 2;
> +	else
> +		cycles = (BIT_ULL(pwm->width) - 1) - tlr + 2;
> +
> +	return DIV_ROUND_DOWN_ULL(cycles * NSEC_PER_SEC_ULL,
> +				  clk_get_rate(pwm->clk));
> +}
> +
> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> +			    const struct pwm_state *state)
> +{
> +	struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip);
> +	u32 tlr0, tlr1;
> +	u32 tcsr0 = readl(pwm->regs + TCSR0);
> +	u32 tcsr1 = readl(pwm->regs + TCSR1);
> +	bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	if (!enabled && state->enabled)
> +		clk_rate_exclusive_get(pwm->clk);
> +
> +	tlr0 = xilinx_pwm_calc_tlr(pwm, tcsr0, state->period);
> +	tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle);
> +	writel(tlr0, pwm->regs + TLR0);
> +	writel(tlr1, pwm->regs + TLR1);
> +
> +	if (state->enabled) {
> +		/* Only touch the TCSRs if we aren't already running */
> +		if (!enabled) {
> +			/* Load TLR into TCR */
> +			writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0);
> +			writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1);
> +			/* Enable timers all at once with ENALL */
> +			tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
> +			tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
> +			writel(tcsr0, pwm->regs + TCSR0);
> +			writel(tcsr1, pwm->regs + TCSR1);
> +		}
> +	} else {
> +		writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0);
> +		writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1);
> +	}
> +
> +	if (enabled && !state->enabled)
> +		clk_rate_exclusive_put(pwm->clk);
> +
> +	return 0;
> +}
> +
> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
> +				 struct pwm_device *unused,
> +				 struct pwm_state *state)
> +{
> +	struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip);
> +	u32 tlr0, tlr1, tcsr0, tcsr1;
> +
> +	tlr0 = readl(pwm->regs + TLR0);
> +	tlr1 = readl(pwm->regs + TLR1);
> +	tcsr0 = readl(pwm->regs + TCSR0);
> +	tcsr1 = readl(pwm->regs + TCSR1);
> +
> +	state->period = xilinx_pwm_get_period(pwm, tlr0, tcsr0);
> +	state->duty_cycle = xilinx_pwm_get_period(pwm, tlr1, tcsr1);
> +	state->enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
> +	state->polarity = PWM_POLARITY_NORMAL;
> +}
> +
> +static const struct pwm_ops xilinx_pwm_ops = {
> +	.apply = xilinx_pwm_apply,
> +	.get_state = xilinx_pwm_get_state,
> +};
> +
> +static struct dentry *debug_dir;
> +
> +#define DEBUG_REG(reg) { .name = #reg, .offset = (reg), }
> +static struct debugfs_reg32 xilinx_pwm_reg32[] = {
> +	DEBUG_REG(TCSR0),
> +	DEBUG_REG(TLR0),
> +	DEBUG_REG(TCR0),
> +	DEBUG_REG(TCSR1),
> +	DEBUG_REG(TLR1),
> +	DEBUG_REG(TCR1),
> +};

I didn't add any DEBUG_FS entries, so I really like this. It would have
helped me while developing my own version of this and will probably be
useful for people debugging their hardware.

> +
> +static int xilinx_pwm_debug_create(struct xilinx_pwm_device *pwm)
> +{
> +	struct debugfs_regset32 *regset;
> +
> +	if (!IS_ENABLED(CONFIG_DEBUG_FS))
> +		return 0;
> +
> +	regset = devm_kzalloc(pwm->chip.dev, sizeof(*regset), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	regset->regs = xilinx_pwm_reg32;
> +	regset->nregs = ARRAY_SIZE(xilinx_pwm_reg32);
> +	regset->base = pwm->regs;
> +
> +	debugfs_create_regset32(dev_name(pwm->chip.dev), 0400, debug_dir,
> +				regset);
> +	return 0;
> +}
> +
> +static int xilinx_pwm_probe(struct platform_device *pdev)
> +{
> +	bool enabled;
> +	int i, ret;
> +	struct device *dev = &pdev->dev;
> +	struct xilinx_pwm_device *pwm;
> +	u32 one_timer;
> +
> +	ret = of_property_read_u32(dev->of_node, "xlnx,one-timer-only",
> +				   &one_timer);
> +	if (ret || one_timer) {
> +		dev_err(dev, "two timers are needed for PWM mode\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < 2; i++) {
> +		char fmt[] = "xlnx,gen%u-assert";
> +		char buf[sizeof(fmt)];
> +		u32 gen;
> +
> +		snprintf(buf, sizeof(buf), fmt, i);
> +		ret = of_property_read_u32(dev->of_node, buf, &gen);
> +		if (ret || !gen) {
> +			dev_err(dev, "generateout%u must be active high\n", i);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, pwm);
> +
> +	pwm->chip.dev = &pdev->dev;
> +	pwm->chip.ops = &xilinx_pwm_ops;
> +	pwm->chip.base = -1;
> +	pwm->chip.npwm = 1;
> +
> +	pwm->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pwm->regs))
> +		return PTR_ERR(pwm->regs);
> +
> +	ret = of_property_read_u32(dev->of_node, "xlnx,count-width", &pwm->width);
> +	if (ret) {
> +		dev_err(dev, "missing counter width\n");
> +		return -EINVAL;
> +	}
> +
> +	pwm->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(pwm->clk))
> +		return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n");
> +
> +	ret = clk_prepare_enable(pwm->clk);
> +	if (ret) {
> +		dev_err(dev, "clock enable failed\n");
> +		return ret;
> +	}
> +
> +	ret = xilinx_pwm_debug_create(pwm);
> +	if (ret) {
> +		clk_disable_unprepare(pwm->clk);
> +		return ret;
> +	}
> +
> +	ret = pwmchip_add(&pwm->chip);
> +	if (ret) {
> +		clk_disable_unprepare(pwm->clk);
> +		return ret;
> +	}
> +
> +	enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
> +					readl(pwm->regs + TCSR1));
> +	if (enabled)
> +		clk_rate_exclusive_get(pwm->clk);
> +
> +	return ret;
> +}
> +
> +static int xilinx_pwm_remove(struct platform_device *pdev)
> +{
> +	struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
> +	bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
> +					     readl(pwm->regs + TCSR1));
> +
> +	if (enabled)
> +		clk_rate_exclusive_put(pwm->clk);
> +	clk_disable_unprepare(pwm->clk);
> +	return pwmchip_remove(&pwm->chip);
> +}
> +
> +static const struct of_device_id xilinx_pwm_of_match[] = {
> +	{ .compatible = "xlnx,xps-timer-1.00.a" },
> +	{ .compatible = "xlnx,axi-timer-2.0" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, xilinx_pwm_of_match);

And here lies culprit of my patch not being accepted, since defining the
driver to be "xlnx,axi-timer-2.0" compatible while still allowing MICROBLAZE
to be set, did make the kernel to misconfigure the interrupt timer used by
the processor.


> +
> +static struct platform_driver xilinx_pwm_driver = {
> +	.probe = xilinx_pwm_probe,
> +	.remove = xilinx_pwm_remove,
> +	.driver = {
> +		.name = "xilinx-pwm",
> +		.of_match_table = of_match_ptr(xilinx_pwm_of_match),
> +	},
> +};
> +
> +static int __init xilinx_pwm_init(void)
> +{
> +	int ret = platform_driver_register(&xilinx_pwm_driver);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
> +		debug_dir = debugfs_create_dir(xilinx_pwm_driver.driver.name, NULL);
> +		if (IS_ERR(debug_dir)) {
> +			platform_driver_unregister(&xilinx_pwm_driver);
> +			return PTR_ERR(debug_dir);
> +		}
> +	}
> +	return 0;
> +}
> +module_init(xilinx_pwm_init);
> +
> +static void __exit xilinx_pwm_exit(void)
> +{
> +	platform_driver_unregister(&xilinx_pwm_driver);
> +	if (IS_ENABLED(CONFIG_DEBUG_FS))
> +		debugfs_remove_recursive(debug_dir);
> +}
> +module_exit(xilinx_pwm_exit);
> +
> +MODULE_ALIAS("platform:xilinx-pwm");
> +MODULE_DESCRIPTION("Xilinx LogiCORE IP AXI Timer PWM driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.25.1
> 

All in all, I'd be very glad if this got accepted (I haven't tested it), but
I'd be still happier if some compromise could be found within the device
tree so that this driver can be used also for Microblaze systems without
interfering with the main timer used by such systems.

Best regards,

-- 
Alvaro G. M.

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

[Uwe Kleine-König]

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

Hello,

On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote:
> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
> found on Xilinx FPGAs. There is another driver for this device located
> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
> 
> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
>  arch/arm64/configs/defconfig |   1 +
>  drivers/pwm/Kconfig          |  11 ++
>  drivers/pwm/Makefile         |   1 +
>  drivers/pwm/pwm-xilinx.c     | 322 +++++++++++++++++++++++++++++++++++
>  4 files changed, 335 insertions(+)
>  create mode 100644 drivers/pwm/pwm-xilinx.c
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 08c6f769df9a..81794209f287 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y
>  CONFIG_PWM_SL28CPLD=m
>  CONFIG_PWM_SUN4I=m
>  CONFIG_PWM_TEGRA=m
> +CONFIG_PWM_XILINX=m
>  CONFIG_SL28CPLD_INTC=y
>  CONFIG_QCOM_PDC=y
>  CONFIG_RESET_IMX7=y

I think this should go into a separate patch once this driver is
accepted. This can then go via the ARM people.

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index d3371ac7b871..01e62928f4bf 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -628,4 +628,15 @@ 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 !MICROBLAZE

I don't understand this dependency.

> +	help
> +	  PWM framework driver for Xilinx LogiCORE IP AXI Timers. This
> +	  timer is typically a soft core which may be present in Xilinx
> +	  FPGAs. 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 d3879619bd76..fc1bd6ccc9ed 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -59,3 +59,4 @@ obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
>  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.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..240bd2993f97
> --- /dev/null
> +++ b/drivers/pwm/pwm-xilinx.c
> @@ -0,0 +1,322 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
> + */

Please add a link to a reference manual here (if available) and add a
paragraph about hardware properties: Does the hardware complete cycles
on reconfiguration or disable? Are there limitiation like "Cannot run
with 100% duty cycle"? Please look into e.g. pwm-sifive how this is done
and stick to the same format for easy grepping.

> +#include <asm/io.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/debugfs.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.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)

I'd like to see a prefix for these defines, something like XILINX_PWM_
maybe?

> +/* Bits we need to set/clear to enable PWM mode, not including CASC */
> +#define TCSR_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)
> +#define TCSR_CLEAR (TCSR_MDT | TCSR_LOAD)

Maybe call this XILINX_PWM_TCSR_DEFAULT?

> +#define NSEC_PER_SEC_ULL 1000000000ULL
> +
> +/**
> + * struct xilinx_pwm_device - Driver data for Xilinx AXI timer PWM driver
> + * @chip: PWM controller chip
> + * @clk: Parent clock
> + * @regs: Base address of this device
> + * @width: Width of the counters, in bits
> + */
> +struct xilinx_pwm_device {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	void __iomem *regs;
> +	unsigned int width;
> +};
> +
> +static inline struct xilinx_pwm_device *xilinx_pwm_chip_to_device(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct xilinx_pwm_device, chip);
> +}
> +
> +static bool xilinx_pwm_is_enabled(u32 tcsr0, u32 tcsr1)
> +{
> +	return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) ||
> +		 ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR);
> +}
> +
> +static u32 xilinx_pwm_calc_tlr(struct xilinx_pwm_device *pwm, u32 tcsr,
> +			       unsigned int period)
> +{
> +	u64 cycles = DIV_ROUND_DOWN_ULL(period * clk_get_rate(pwm->clk),
> +					NSEC_PER_SEC_ULL);

NSEC_PER_SEC should work just fine here.

> +
> +	if (tcsr & TCSR_UDT)
> +		return cycles - 2;

What happens if cycles <= 1?

> +	else
> +		return (BIT_ULL(pwm->width) - 1) - cycles + 2;

What happens if cycles > BIT_ULL(pwm->width)?

> +}
> +
> +static unsigned int xilinx_pwm_get_period(struct xilinx_pwm_device *pwm,
> +					  u32 tlr, u32 tcsr)
> +{
> +	u64 cycles;
> +
> +	if (tcsr & TCSR_UDT)
> +		cycles = tlr + 2;
> +	else
> +		cycles = (BIT_ULL(pwm->width) - 1) - tlr + 2;
> +
> +	return DIV_ROUND_DOWN_ULL(cycles * NSEC_PER_SEC_ULL,
> +				  clk_get_rate(pwm->clk));

Please round up here. The idea is that applying the result from
.get_state() should not modify the configuration. You might want to
enable PWM_DEBUG to test your driver.

> +}
> +
> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> +			    const struct pwm_state *state)
> +{
> +	struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip);
> +	u32 tlr0, tlr1;
> +	u32 tcsr0 = readl(pwm->regs + TCSR0);
> +	u32 tcsr1 = readl(pwm->regs + TCSR1);
> +	bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	if (!enabled && state->enabled)
> +		clk_rate_exclusive_get(pwm->clk);

Missing error checking.

> +
> +	tlr0 = xilinx_pwm_calc_tlr(pwm, tcsr0, state->period);

Please care for overflowing. If state->period >= U64_MAX /
clk_get_rate(pwm->clk) the multiplication in xilinx_pwm_calc_tlr is
susceptible. Please do something like:

	... calculate maximal period ...

	if (state->period > max_period)
		period = max_period
	else
		period = state->period;

	...

The algorithm to implement is: Configure the biggest possible period not
bigger than state->period. With that period configure the biggest
duty_cycle not bigger than state->duty_cycle.

> +	tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle);
> +	writel(tlr0, pwm->regs + TLR0);
> +	writel(tlr1, pwm->regs + TLR1);

How does the hardware behave here? E.g. can it happen that after writing
TLR0 we can see a cycle with the new period but the old duty_cycle? Does
it complete the currently running period?

If state->enabled == 0 you want to disable first to prevent that the
(maybe) new values for period and duty_cycle are emitted by the
hardware.

> +	if (state->enabled) {
> +		/* Only touch the TCSRs if we aren't already running */
> +		if (!enabled) {
> +			/* Load TLR into TCR */
> +			writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0);
> +			writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1);
> +			/* Enable timers all at once with ENALL */
> +			tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
> +			tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
> +			writel(tcsr0, pwm->regs + TCSR0);
> +			writel(tcsr1, pwm->regs + TCSR1);
> +		}
> +	} else {
> +		writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0);
> +		writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1);

How does the hardware behave on disable? Does it always emit a low
level? Does it complete the currently running period?

> +	}
> +
> +	if (enabled && !state->enabled)
> +		clk_rate_exclusive_put(pwm->clk);
> +
> +	return 0;
> +}
> +
> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
> +				 struct pwm_device *unused,
> +				 struct pwm_state *state)
> +{
> +	struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip);
> +	u32 tlr0, tlr1, tcsr0, tcsr1;
> +
> +	tlr0 = readl(pwm->regs + TLR0);
> +	tlr1 = readl(pwm->regs + TLR1);
> +	tcsr0 = readl(pwm->regs + TCSR0);
> +	tcsr1 = readl(pwm->regs + TCSR1);
> +
> +	state->period = xilinx_pwm_get_period(pwm, tlr0, tcsr0);
> +	state->duty_cycle = xilinx_pwm_get_period(pwm, tlr1, tcsr1);
> +	state->enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
> +	state->polarity = PWM_POLARITY_NORMAL;
> +}
> +
> +static const struct pwm_ops xilinx_pwm_ops = {
> +	.apply = xilinx_pwm_apply,
> +	.get_state = xilinx_pwm_get_state,

.owner is missing

> +};
> +
> +static struct dentry *debug_dir;
> +
> +#define DEBUG_REG(reg) { .name = #reg, .offset = (reg), }
> +static struct debugfs_reg32 xilinx_pwm_reg32[] = {
> +	DEBUG_REG(TCSR0),
> +	DEBUG_REG(TLR0),
> +	DEBUG_REG(TCR0),
> +	DEBUG_REG(TCSR1),
> +	DEBUG_REG(TLR1),
> +	DEBUG_REG(TCR1),
> +};
> +
> +static int xilinx_pwm_debug_create(struct xilinx_pwm_device *pwm)
> +{
> +	struct debugfs_regset32 *regset;
> +
> +	if (!IS_ENABLED(CONFIG_DEBUG_FS))
> +		return 0;
> +
> +	regset = devm_kzalloc(pwm->chip.dev, sizeof(*regset), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	regset->regs = xilinx_pwm_reg32;
> +	regset->nregs = ARRAY_SIZE(xilinx_pwm_reg32);
> +	regset->base = pwm->regs;
> +
> +	debugfs_create_regset32(dev_name(pwm->chip.dev), 0400, debug_dir,
> +				regset);

debug_dir might not yet be initialized here.

> +	return 0;
> +}

This is unusual to have in a driver. Something like memtool[1] should
work just fine, then you don't need this debugfs file. Together with pwm
tracing this should be good enough.

[1] https://github.com/pengutronix/memtool

> +static int xilinx_pwm_probe(struct platform_device *pdev)
> +{
> +	bool enabled;
> +	int i, ret;
> +	struct device *dev = &pdev->dev;
> +	struct xilinx_pwm_device *pwm;
> +	u32 one_timer;
> +
> +	ret = of_property_read_u32(dev->of_node, "xlnx,one-timer-only",
> +				   &one_timer);
> +	if (ret || one_timer) {
> +		dev_err(dev, "two timers are needed for PWM mode\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < 2; i++) {
> +		char fmt[] = "xlnx,gen%u-assert";
> +		char buf[sizeof(fmt)];
> +		u32 gen;
> +
> +		snprintf(buf, sizeof(buf), fmt, i);
> +		ret = of_property_read_u32(dev->of_node, buf, &gen);
> +		if (ret || !gen) {
> +			dev_err(dev, "generateout%u must be active high\n", i);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, pwm);
> +
> +	pwm->chip.dev = &pdev->dev;
> +	pwm->chip.ops = &xilinx_pwm_ops;
> +	pwm->chip.base = -1;

Please drop the assignment to .base. See commit
f9a8ee8c8bcd118e800d88772c6457381db45224 in next.

> +	pwm->chip.npwm = 1;
> +
> +	pwm->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pwm->regs))
> +		return PTR_ERR(pwm->regs);
> +
> +	ret = of_property_read_u32(dev->of_node, "xlnx,count-width", &pwm->width);
> +	if (ret) {
> +		dev_err(dev, "missing counter width\n");
> +		return -EINVAL;
> +	}

Would it make sense to error out on some values of pwm->width? Values
bigger than 32 don't make sense?

> +	pwm->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(pwm->clk))
> +		return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n");
> +
> +	ret = clk_prepare_enable(pwm->clk);
> +	if (ret) {
> +		dev_err(dev, "clock enable failed\n");
> +		return ret;

You can use dev_err_probe here, too, for consistency.

> +	}
> +
> +	ret = xilinx_pwm_debug_create(pwm);
> +	if (ret) {
> +		clk_disable_unprepare(pwm->clk);
> +		return ret;
> +	}
> +
> +	ret = pwmchip_add(&pwm->chip);
> +	if (ret) {
> +		clk_disable_unprepare(pwm->clk);

Error message please.

> +		return ret;
> +	}
> +
> +	enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
> +					readl(pwm->regs + TCSR1));
> +	if (enabled)
> +		clk_rate_exclusive_get(pwm->clk);

This needs to happen before calling pwmchip_add(), doesn't it?

> +
> +	return ret;
> +}
> +
> +static int xilinx_pwm_remove(struct platform_device *pdev)
> +{
> +	struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
> +	bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
> +					     readl(pwm->regs + TCSR1));
> +
> +	if (enabled)
> +		clk_rate_exclusive_put(pwm->clk);

This looks wrong. You should rely on the consumer that they disable the
PWM.

> +	clk_disable_unprepare(pwm->clk);

I assume this stops the PWM, so this must happen after calling
pwmchip_remove.

> +	return pwmchip_remove(&pwm->chip);

Note, pwmchip_remove always returns 0 and I plan to change it to return
void instead. So please don't check the return value here.

> +}
> +

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 2/2] pwm: Add support for Xilinx AXI Timer

[Michal Simek]

On 5/4/21 10:51 AM, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote:
>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
>> found on Xilinx FPGAs. There is another driver for this device located
>> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
>> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
>>
>> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>>
>>  arch/arm64/configs/defconfig |   1 +
>>  drivers/pwm/Kconfig          |  11 ++
>>  drivers/pwm/Makefile         |   1 +
>>  drivers/pwm/pwm-xilinx.c     | 322 +++++++++++++++++++++++++++++++++++
>>  4 files changed, 335 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-xilinx.c
>>
>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>> index 08c6f769df9a..81794209f287 100644
>> --- a/arch/arm64/configs/defconfig
>> +++ b/arch/arm64/configs/defconfig
>> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y
>>  CONFIG_PWM_SL28CPLD=m
>>  CONFIG_PWM_SUN4I=m
>>  CONFIG_PWM_TEGRA=m
>> +CONFIG_PWM_XILINX=m
>>  CONFIG_SL28CPLD_INTC=y
>>  CONFIG_QCOM_PDC=y
>>  CONFIG_RESET_IMX7=y
> 
> I think this should go into a separate patch once this driver is
> accepted. This can then go via the ARM people.
> 
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index d3371ac7b871..01e62928f4bf 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -628,4 +628,15 @@ 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 !MICROBLAZE
> 
> I don't understand this dependency.

The dependency is clear here because microblaze has already driver for
this timer here arch/microblaze/kernel/timer.c.

And that's exactly pointing to the way how this should be done.
IP itself is single or dual timer and in case of dual timer you can
select if there is pwm output and use it for PWM generation.

It means it is timer with PMW together.
I didn't have a time but Uwe likely knows this better how to design it.

I see that gpio-mvebu driver instantiate pwm driver. Maybe that's the
way to go.

Step first is move axi timer driver from microblaze to generic location.
Figured it out how to add PWM (with DT flag) and then write support for it.

Thanks,
Michal

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

[Sean Anderson]

On 5/4/21 8:32 AM, Michal Simek wrote:
 >
 >
 > On 5/4/21 10:51 AM, Uwe Kleine-König wrote:
 >> Hello,
 >>
 >> On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote:
 >>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
 >>> found on Xilinx FPGAs. There is another driver for this device located
 >>> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
 >>> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
 >>>
 >>> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
 >>>
 >>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >>> ---
 >>>
 >>>   arch/arm64/configs/defconfig |   1 +
 >>>   drivers/pwm/Kconfig          |  11 ++
 >>>   drivers/pwm/Makefile         |   1 +
 >>>   drivers/pwm/pwm-xilinx.c     | 322 +++++++++++++++++++++++++++++++++++
 >>>   4 files changed, 335 insertions(+)
 >>>   create mode 100644 drivers/pwm/pwm-xilinx.c
 >>>
 >>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
 >>> index 08c6f769df9a..81794209f287 100644
 >>> --- a/arch/arm64/configs/defconfig
 >>> +++ b/arch/arm64/configs/defconfig
 >>> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y
 >>>   CONFIG_PWM_SL28CPLD=m
 >>>   CONFIG_PWM_SUN4I=m
 >>>   CONFIG_PWM_TEGRA=m
 >>> +CONFIG_PWM_XILINX=m
 >>>   CONFIG_SL28CPLD_INTC=y
 >>>   CONFIG_QCOM_PDC=y
 >>>   CONFIG_RESET_IMX7=y
 >>
 >> I think this should go into a separate patch once this driver is
 >> accepted. This can then go via the ARM people.
 >>
 >>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
 >>> index d3371ac7b871..01e62928f4bf 100644
 >>> --- a/drivers/pwm/Kconfig
 >>> +++ b/drivers/pwm/Kconfig
 >>> @@ -628,4 +628,15 @@ 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 !MICROBLAZE
 >>
 >> I don't understand this dependency.
 >
 > The dependency is clear here because microblaze has already driver for
 > this timer here arch/microblaze/kernel/timer.c.
 >
 > And that's exactly pointing to the way how this should be done.
 > IP itself is single or dual timer and in case of dual timer you can
 > select if there is pwm output and use it for PWM generation.
 >
 > It means it is timer with PMW together.
 > I didn't have a time but Uwe likely knows this better how to design it.
 >
 > I see that gpio-mvebu driver instantiate pwm driver. Maybe that's the
 > way to go.

I think drivers/clocksource/samsung_pwm_timer.c and
drivers/pwm/pwm-samsung.c provide another example for how to go about
this.

 > Step first is move axi timer driver from microblaze to generic location.

Yes. However, I don't have a microblaze setup, so I have just added the
PWM driver.

 > Figured it out how to add PWM (with DT flag) and then write support for it.

I would really like to see a standard way of doing this. Many timers
also have PWM support (e.g. samsung, and also DW). I think it's unusual
that this problem has not already been addressed.

--Sean

 >
 > Thanks,
 > Michal
 >

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

[Uwe Kleine-König]

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

Hello Sean,

[Adding Rob to the list of recipents, as he for sure has a valuable
opinion on this matter.]

On Tue, May 04, 2021 at 11:57:20AM -0400, Sean Anderson wrote:
> On 5/4/21 8:32 AM, Michal Simek wrote:
> > On 5/4/21 10:51 AM, Uwe Kleine-König wrote:
> >> On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote:
> >>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
> >>> found on Xilinx FPGAs. There is another driver for this device located
> >>> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
> >>> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
> >>>
> >>> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
> >>>
> >>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> >>> ---
> >>>
> >>>   arch/arm64/configs/defconfig |   1 +
> >>>   drivers/pwm/Kconfig          |  11 ++
> >>>   drivers/pwm/Makefile         |   1 +
> >>>   drivers/pwm/pwm-xilinx.c     | 322 +++++++++++++++++++++++++++++++++++
> >>>   4 files changed, 335 insertions(+)
> >>>   create mode 100644 drivers/pwm/pwm-xilinx.c
> >>>
> >>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> >>> index 08c6f769df9a..81794209f287 100644
> >>> --- a/arch/arm64/configs/defconfig
> >>> +++ b/arch/arm64/configs/defconfig
> >>> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y
> >>>   CONFIG_PWM_SL28CPLD=m
> >>>   CONFIG_PWM_SUN4I=m
> >>>   CONFIG_PWM_TEGRA=m
> >>> +CONFIG_PWM_XILINX=m
> >>>   CONFIG_SL28CPLD_INTC=y
> >>>   CONFIG_QCOM_PDC=y
> >>>   CONFIG_RESET_IMX7=y
> >>
> >> I think this should go into a separate patch once this driver is
> >> accepted. This can then go via the ARM people.
> >>
> >>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >>> index d3371ac7b871..01e62928f4bf 100644
> >>> --- a/drivers/pwm/Kconfig
> >>> +++ b/drivers/pwm/Kconfig
> >>> @@ -628,4 +628,15 @@ 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 !MICROBLAZE
> >>
> >> I don't understand this dependency.
> >
> > The dependency is clear here because microblaze has already driver for
> > this timer here arch/microblaze/kernel/timer.c.

Then at least add a comment.

> > And that's exactly pointing to the way how this should be done.
> > IP itself is single or dual timer and in case of dual timer you can
> > select if there is pwm output and use it for PWM generation.
> >
> > It means it is timer with PMW together.
> > I didn't have a time but Uwe likely knows this better how to design it.
> >
> > I see that gpio-mvebu driver instantiate pwm driver. Maybe that's the
> > way to go.
> 
> I think drivers/clocksource/samsung_pwm_timer.c and
> drivers/pwm/pwm-samsung.c provide another example for how to go about
> this.

I recently had a similar problem (with code that isn't (yet) in
mainline), where a timer can be used as a counter. I chose to change the
compatible. Transferred to this example this would mean to use e.g.

	static const struct of_device_id xilinx_pwm_of_match[] = {
		{ .compatible = "xlnx,xps-timer-pwm-1.00.a" },
		...
	};

and if you want to use the hardware as a PWM, you overwrite the
compatible in your machine.dts.

Not sure however that this is nice enough to be accepted by the dt
people?!

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 2/2] pwm: Add support for Xilinx AXI Timer

[Michal Simek]

Hi,

On 5/4/21 6:13 PM, Uwe Kleine-König wrote:
> Hello Sean,
> 
> [Adding Rob to the list of recipents, as he for sure has a valuable
> opinion on this matter.]
> 
> On Tue, May 04, 2021 at 11:57:20AM -0400, Sean Anderson wrote:
>> On 5/4/21 8:32 AM, Michal Simek wrote:
>>> On 5/4/21 10:51 AM, Uwe Kleine-König wrote:
>>>> On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote:
>>>>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
>>>>> found on Xilinx FPGAs. There is another driver for this device located
>>>>> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
>>>>> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
>>>>>
>>>>> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
>>>>>
>>>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>>>> ---
>>>>>
>>>>>   arch/arm64/configs/defconfig |   1 +
>>>>>   drivers/pwm/Kconfig          |  11 ++
>>>>>   drivers/pwm/Makefile         |   1 +
>>>>>   drivers/pwm/pwm-xilinx.c     | 322 +++++++++++++++++++++++++++++++++++
>>>>>   4 files changed, 335 insertions(+)
>>>>>   create mode 100644 drivers/pwm/pwm-xilinx.c
>>>>>
>>>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>>>>> index 08c6f769df9a..81794209f287 100644
>>>>> --- a/arch/arm64/configs/defconfig
>>>>> +++ b/arch/arm64/configs/defconfig
>>>>> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y
>>>>>   CONFIG_PWM_SL28CPLD=m
>>>>>   CONFIG_PWM_SUN4I=m
>>>>>   CONFIG_PWM_TEGRA=m
>>>>> +CONFIG_PWM_XILINX=m
>>>>>   CONFIG_SL28CPLD_INTC=y
>>>>>   CONFIG_QCOM_PDC=y
>>>>>   CONFIG_RESET_IMX7=y
>>>>
>>>> I think this should go into a separate patch once this driver is
>>>> accepted. This can then go via the ARM people.
>>>>
>>>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>>>> index d3371ac7b871..01e62928f4bf 100644
>>>>> --- a/drivers/pwm/Kconfig
>>>>> +++ b/drivers/pwm/Kconfig
>>>>> @@ -628,4 +628,15 @@ 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 !MICROBLAZE
>>>>
>>>> I don't understand this dependency.
>>>
>>> The dependency is clear here because microblaze has already driver for
>>> this timer here arch/microblaze/kernel/timer.c.
> 
> Then at least add a comment.

I don't think that comment will really solve this. We should never
duplicate driver for the same IP to two locations.

Maybe this should be MFD driver.


> 
>>> And that's exactly pointing to the way how this should be done.
>>> IP itself is single or dual timer and in case of dual timer you can
>>> select if there is pwm output and use it for PWM generation.
>>>
>>> It means it is timer with PMW together.
>>> I didn't have a time but Uwe likely knows this better how to design it.
>>>
>>> I see that gpio-mvebu driver instantiate pwm driver. Maybe that's the
>>> way to go.
>>
>> I think drivers/clocksource/samsung_pwm_timer.c and
>> drivers/pwm/pwm-samsung.c provide another example for how to go about
>> this.
> 
> I recently had a similar problem (with code that isn't (yet) in
> mainline), where a timer can be used as a counter. I chose to change the
> compatible. Transferred to this example this would mean to use e.g.
> 
> 	static const struct of_device_id xilinx_pwm_of_match[] = {
> 		{ .compatible = "xlnx,xps-timer-pwm-1.00.a" },
> 		...
> 	};
> 
> and if you want to use the hardware as a PWM, you overwrite the
> compatible in your machine.dts.

It is HW selection inside that IP. It means you will get dt properly
when PWM output is selected. I understand that this shortcut can work
but I don't think it is proper design.


> Not sure however that this is nice enough to be accepted by the dt
> people?!

up to Rob.

Thanks,
Michal

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

[Sean Anderson]

On 5/4/21 4:51 AM, Uwe Kleine-König wrote:
 > Hello,
 >
 > On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote:
 >> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
 >> found on Xilinx FPGAs. There is another driver for this device located
 >> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
 >> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
 >>
 >> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
 >>
 >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >> ---
 >>
 >>   arch/arm64/configs/defconfig |   1 +
 >>   drivers/pwm/Kconfig          |  11 ++
 >>   drivers/pwm/Makefile         |   1 +
 >>   drivers/pwm/pwm-xilinx.c     | 322 +++++++++++++++++++++++++++++++++++
 >>   4 files changed, 335 insertions(+)
 >>   create mode 100644 drivers/pwm/pwm-xilinx.c
 >>
 >> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
 >> index 08c6f769df9a..81794209f287 100644
 >> --- a/arch/arm64/configs/defconfig
 >> +++ b/arch/arm64/configs/defconfig
 >> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y
 >>   CONFIG_PWM_SL28CPLD=m
 >>   CONFIG_PWM_SUN4I=m
 >>   CONFIG_PWM_TEGRA=m
 >> +CONFIG_PWM_XILINX=m
 >>   CONFIG_SL28CPLD_INTC=y
 >>   CONFIG_QCOM_PDC=y
 >>   CONFIG_RESET_IMX7=y
 >
 > I think this should go into a separate patch once this driver is
 > accepted. This can then go via the ARM people.

Sure.

 >
 >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
 >> index d3371ac7b871..01e62928f4bf 100644
 >> --- a/drivers/pwm/Kconfig
 >> +++ b/drivers/pwm/Kconfig
 >> @@ -628,4 +628,15 @@ 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 !MICROBLAZE
 >
 > I don't understand this dependency.

As noted in the commit message, there is already a driver for this
device for microblaze systems. To prevent conflicts, this driver is
disabled on microblaze.

 >
 >> +	help
 >> +	  PWM framework driver for Xilinx LogiCORE IP AXI Timers. This
 >> +	  timer is typically a soft core which may be present in Xilinx
 >> +	  FPGAs. 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 d3879619bd76..fc1bd6ccc9ed 100644
 >> --- a/drivers/pwm/Makefile
 >> +++ b/drivers/pwm/Makefile
 >> @@ -59,3 +59,4 @@ obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
 >>   obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 >>   obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.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..240bd2993f97
 >> --- /dev/null
 >> +++ b/drivers/pwm/pwm-xilinx.c
 >> @@ -0,0 +1,322 @@
 >> +// SPDX-License-Identifier: GPL-2.0+
 >> +/*
 >> + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
 >> + */
 >
 > Please add a link to a reference manual here (if available) and add a
 > paragraph about hardware properties: Does the hardware complete cycles
 > on reconfiguration or disable? Are there limitiation like "Cannot run
 > with 100% duty cycle"? Please look into e.g. pwm-sifive how this is done
 > and stick to the same format for easy grepping.

Ok, will add.

 >
 >> +#include <asm/io.h>
 >> +#include <linux/clk.h>
 >> +#include <linux/device.h>
 >> +#include <linux/debugfs.h>
 >> +#include <linux/module.h>
 >> +#include <linux/platform_device.h>
 >> +#include <linux/pwm.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)
 >
 > I'd like to see a prefix for these defines, something like XILINX_PWM_
 > maybe?

I don't think that's necessary, since these defines are used only for
this file. In particular, adding a prefix like that would significantly
lengthen lines which add several flags.

 >
 >> +/* Bits we need to set/clear to enable PWM mode, not including CASC */
 >> +#define TCSR_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)
 >> +#define TCSR_CLEAR (TCSR_MDT | TCSR_LOAD)
 >
 > Maybe call this XILINX_PWM_TCSR_DEFAULT?
 >
 >> +#define NSEC_PER_SEC_ULL 1000000000ULL
 >> +
 >> +/**
 >> + * struct xilinx_pwm_device - Driver data for Xilinx AXI timer PWM driver
 >> + * @chip: PWM controller chip
 >> + * @clk: Parent clock
 >> + * @regs: Base address of this device
 >> + * @width: Width of the counters, in bits
 >> + */
 >> +struct xilinx_pwm_device {
 >> +	struct pwm_chip chip;
 >> +	struct clk *clk;
 >> +	void __iomem *regs;
 >> +	unsigned int width;
 >> +};
 >> +
 >> +static inline struct xilinx_pwm_device *xilinx_pwm_chip_to_device(struct pwm_chip *chip)
 >> +{
 >> +	return container_of(chip, struct xilinx_pwm_device, chip);
 >> +}
 >> +
 >> +static bool xilinx_pwm_is_enabled(u32 tcsr0, u32 tcsr1)
 >> +{
 >> +	return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) ||
 >> +		 ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR);
 >> +}
 >> +
 >> +static u32 xilinx_pwm_calc_tlr(struct xilinx_pwm_device *pwm, u32 tcsr,
 >> +			       unsigned int period)
 >> +{
 >> +	u64 cycles = DIV_ROUND_DOWN_ULL(period * clk_get_rate(pwm->clk),
 >> +					NSEC_PER_SEC_ULL);
 >
 > NSEC_PER_SEC should work just fine here.

Ok.

 >
 >> +
 >> +	if (tcsr & TCSR_UDT)
 >> +		return cycles - 2;
 >
 > What happens if cycles <= 1?

Then we have a problem :)

I will add a check for this.

 >
 >> +	else
 >> +		return (BIT_ULL(pwm->width) - 1) - cycles + 2;
 >
 > What happens if cycles > BIT_ULL(pwm->width)?

ditto.

 >
 >> +}
 >> +
 >> +static unsigned int xilinx_pwm_get_period(struct xilinx_pwm_device *pwm,
 >> +					  u32 tlr, u32 tcsr)
 >> +{
 >> +	u64 cycles;
 >> +
 >> +	if (tcsr & TCSR_UDT)
 >> +		cycles = tlr + 2;
 >> +	else
 >> +		cycles = (BIT_ULL(pwm->width) - 1) - tlr + 2;
 >> +
 >> +	return DIV_ROUND_DOWN_ULL(cycles * NSEC_PER_SEC_ULL,
 >> +				  clk_get_rate(pwm->clk));
 >
 > Please round up here. The idea is that applying the result from
 > .get_state() should not modify the configuration. You might want to
 > enable PWM_DEBUG to test your driver.

Ok.

 >
 >> +}
 >> +
 >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
 >> +			    const struct pwm_state *state)
 >> +{
 >> +	struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip);
 >> +	u32 tlr0, tlr1;
 >> +	u32 tcsr0 = readl(pwm->regs + TCSR0);
 >> +	u32 tcsr1 = readl(pwm->regs + TCSR1);
 >> +	bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
 >> +
 >> +	if (state->polarity != PWM_POLARITY_NORMAL)
 >> +		return -EINVAL;
 >> +
 >> +	if (!enabled && state->enabled)
 >> +		clk_rate_exclusive_get(pwm->clk);
 >
 > Missing error checking.

This cannot fail.

 >
 >> +
 >> +	tlr0 = xilinx_pwm_calc_tlr(pwm, tcsr0, state->period);
 >
 > Please care for overflowing. If state->period >= U64_MAX /
 > clk_get_rate(pwm->clk) the multiplication in xilinx_pwm_calc_tlr is
 > susceptible. Please do something like:
 >
 > 	... calculate maximal period ...
 >
 > 	if (state->period > max_period)
 > 		period = max_period
 > 	else
 > 		period = state->period;
 >
 > 	...
 >
 > The algorithm to implement is: Configure the biggest possible period not
 > bigger than state->period. With that period configure the biggest
 > duty_cycle not bigger than state->duty_cycle.
 >
 >> +	tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle);
 >> +	writel(tlr0, pwm->regs + TLR0);
 >> +	writel(tlr1, pwm->regs + TLR1);
 >
 > How does the hardware behave here? E.g. can it happen that after writing
 > TLR0 we can see a cycle with the new period but the old duty_cycle? Does
 > it complete the currently running period?

TLR is the value loaded into TCR when the counter resets. I believe this
happens at the rising edge for TCR0 and at the falling edge for TCR1,
but the exact behavior is not documented. Therefore, changing the value
of TLR will not affect the current cycle.

 >
 > If state->enabled == 0 you want to disable first to prevent that the
 > (maybe) new values for period and duty_cycle are emitted by the
 > hardware.
 >
 >> +	if (state->enabled) {
 >> +		/* Only touch the TCSRs if we aren't already running */
 >> +		if (!enabled) {
 >> +			/* Load TLR into TCR */
 >> +			writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0);
 >> +			writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1);
 >> +			/* Enable timers all at once with ENALL */
 >> +			tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
 >> +			tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
 >> +			writel(tcsr0, pwm->regs + TCSR0);
 >> +			writel(tcsr1, pwm->regs + TCSR1);
 >> +		}
 >> +	} else {
 >> +		writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0);
 >> +		writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1);
 >
 > How does the hardware behave on disable? Does it always emit a low
 > level? Does it complete the currently running period?

I don't know. My suspicion is that the output stops at whatever its
current value is.

 >
 >> +	}
 >> +
 >> +	if (enabled && !state->enabled)
 >> +		clk_rate_exclusive_put(pwm->clk);
 >> +
 >> +	return 0;
 >> +}
 >> +
 >> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
 >> +				 struct pwm_device *unused,
 >> +				 struct pwm_state *state)
 >> +{
 >> +	struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip);
 >> +	u32 tlr0, tlr1, tcsr0, tcsr1;
 >> +
 >> +	tlr0 = readl(pwm->regs + TLR0);
 >> +	tlr1 = readl(pwm->regs + TLR1);
 >> +	tcsr0 = readl(pwm->regs + TCSR0);
 >> +	tcsr1 = readl(pwm->regs + TCSR1);
 >> +
 >> +	state->period = xilinx_pwm_get_period(pwm, tlr0, tcsr0);
 >> +	state->duty_cycle = xilinx_pwm_get_period(pwm, tlr1, tcsr1);
 >> +	state->enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
 >> +	state->polarity = PWM_POLARITY_NORMAL;
 >> +}
 >> +
 >> +static const struct pwm_ops xilinx_pwm_ops = {
 >> +	.apply = xilinx_pwm_apply,
 >> +	.get_state = xilinx_pwm_get_state,
 >
 > .owner is missing

OK, will add.

 >
 >> +};
 >> +
 >> +static struct dentry *debug_dir;
 >> +
 >> +#define DEBUG_REG(reg) { .name = #reg, .offset = (reg), }
 >> +static struct debugfs_reg32 xilinx_pwm_reg32[] = {
 >> +	DEBUG_REG(TCSR0),
 >> +	DEBUG_REG(TLR0),
 >> +	DEBUG_REG(TCR0),
 >> +	DEBUG_REG(TCSR1),
 >> +	DEBUG_REG(TLR1),
 >> +	DEBUG_REG(TCR1),
 >> +};
 >> +
 >> +static int xilinx_pwm_debug_create(struct xilinx_pwm_device *pwm)
 >> +{
 >> +	struct debugfs_regset32 *regset;
 >> +
 >> +	if (!IS_ENABLED(CONFIG_DEBUG_FS))
 >> +		return 0;
 >> +
 >> +	regset = devm_kzalloc(pwm->chip.dev, sizeof(*regset), GFP_KERNEL);
 >> +	if (!pwm)
 >> +		return -ENOMEM;
 >> +
 >> +	regset->regs = xilinx_pwm_reg32;
 >> +	regset->nregs = ARRAY_SIZE(xilinx_pwm_reg32);
 >> +	regset->base = pwm->regs;
 >> +
 >> +	debugfs_create_regset32(dev_name(pwm->chip.dev), 0400, debug_dir,
 >> +				regset);
 >
 > debug_dir might not yet be initialized here.

Ok, I will move the initialization of the directory before registering
this driver in module init.

 >
 >> +	return 0;
 >> +}
 >
 > This is unusual to have in a driver. Something like memtool[1] should
 > work just fine, then you don't need this debugfs file. Together with pwm
 > tracing this should be good enough.

I didn't have memtool on my test system, so I added this. I can remove
it from the next patch if you'd like.

 >
 > [1] https://github.com/pengutronix/memtool
 >
 >> +static int xilinx_pwm_probe(struct platform_device *pdev)
 >> +{
 >> +	bool enabled;
 >> +	int i, ret;
 >> +	struct device *dev = &pdev->dev;
 >> +	struct xilinx_pwm_device *pwm;
 >> +	u32 one_timer;
 >> +
 >> +	ret = of_property_read_u32(dev->of_node, "xlnx,one-timer-only",
 >> +				   &one_timer);
 >> +	if (ret || one_timer) {
 >> +		dev_err(dev, "two timers are needed for PWM mode\n");
 >> +		return -EINVAL;
 >> +	}
 >> +
 >> +	for (i = 0; i < 2; i++) {
 >> +		char fmt[] = "xlnx,gen%u-assert";
 >> +		char buf[sizeof(fmt)];
 >> +		u32 gen;
 >> +
 >> +		snprintf(buf, sizeof(buf), fmt, i);
 >> +		ret = of_property_read_u32(dev->of_node, buf, &gen);
 >> +		if (ret || !gen) {
 >> +			dev_err(dev, "generateout%u must be active high\n", i);
 >> +			return -EINVAL;
 >> +		}
 >> +	}
 >> +
 >> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
 >> +	if (!pwm)
 >> +		return -ENOMEM;
 >> +	platform_set_drvdata(pdev, pwm);
 >> +
 >> +	pwm->chip.dev = &pdev->dev;
 >> +	pwm->chip.ops = &xilinx_pwm_ops;
 >> +	pwm->chip.base = -1;
 >
 > Please drop the assignment to .base. See commit
 > f9a8ee8c8bcd118e800d88772c6457381db45224 in next.

Ok.

 >> +	pwm->chip.npwm = 1;
 >> +
 >> +	pwm->regs = devm_platform_ioremap_resource(pdev, 0);
 >> +	if (IS_ERR(pwm->regs))
 >> +		return PTR_ERR(pwm->regs);
 >> +
 >> +	ret = of_property_read_u32(dev->of_node, "xlnx,count-width", &pwm->width);
 >> +	if (ret) {
 >> +		dev_err(dev, "missing counter width\n");
 >> +		return -EINVAL;
 >> +	}
 >
 > Would it make sense to error out on some values of pwm->width? Values
 > bigger than 32 don't make sense?

Yes. I will add a range check next revision.

 >
 >> +	pwm->clk = devm_clk_get(dev, NULL);
 >> +	if (IS_ERR(pwm->clk))
 >> +		return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n");
 >> +
 >> +	ret = clk_prepare_enable(pwm->clk);
 >> +	if (ret) {
 >> +		dev_err(dev, "clock enable failed\n");
 >> +		return ret;
 >
 > You can use dev_err_probe here, too, for consistency.

Ok. Though in this case we will never get -EPROBE_DEFER from
clk_prepare_enable, so dev_err_probe is not as useful.

 >
 >> +	}
 >> +
 >> +	ret = xilinx_pwm_debug_create(pwm);
 >> +	if (ret) {
 >> +		clk_disable_unprepare(pwm->clk);
 >> +		return ret;
 >> +	}
 >> +
 >> +	ret = pwmchip_add(&pwm->chip);
 >> +	if (ret) {
 >> +		clk_disable_unprepare(pwm->clk);
 >
 > Error message please.

Ok.

 >
 >> +		return ret;
 >> +	}
 >> +
 >> +	enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
 >> +					readl(pwm->regs + TCSR1));
 >> +	if (enabled)
 >> +		clk_rate_exclusive_get(pwm->clk);
 >
 > This needs to happen before calling pwmchip_add(), doesn't it?

Yes.

 >
 >> +
 >> +	return ret;
 >> +}
 >> +
 >> +static int xilinx_pwm_remove(struct platform_device *pdev)
 >> +{
 >> +	struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
 >> +	bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
 >> +					     readl(pwm->regs + TCSR1));
 >> +
 >> +	if (enabled)
 >> +		clk_rate_exclusive_put(pwm->clk);
 >
 > This looks wrong. You should rely on the consumer that they disable the
 > PWM.

What about a PWM regulator with always-on?

This is mostly to match the exclusive_get in probe, in case there are
misbehaving consumers.

 >
 >> +	clk_disable_unprepare(pwm->clk);
 >
 > I assume this stops the PWM, so this must happen after calling
 > pwmchip_remove.

Ok.

 >
 >> +	return pwmchip_remove(&pwm->chip);
 >
 > Note, pwmchip_remove always returns 0 and I plan to change it to return
 > void instead. So please don't check the return value here.

Ok.

Thanks for the detailed review.

--Sean

 >
 >> +}
 >> +
 >
 > Best regards
 > Uwe
 >

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

[Sean Anderson]

On 5/4/21 10:46 AM, Sean Anderson wrote:
 > On 5/4/21 4:51 AM, Uwe Kleine-König wrote:
 >  >> +static int xilinx_pwm_remove(struct platform_device *pdev)
 >  >> +{
 >  >> +    struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
 >  >> +    bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
 >  >> +                         readl(pwm->regs + TCSR1));
 >  >> +
 >  >> +    if (enabled)
 >  >> +        clk_rate_exclusive_put(pwm->clk);
 >  >
 >  > This looks wrong. You should rely on the consumer that they disable the
 >  > PWM.
 >
 > What about a PWM regulator with always-on?
 >
 > This is mostly to match the exclusive_get in probe, in case there are
 > misbehaving consumers.

Since we always get the rate exclusively, we must always put the rate
exclusively here. So if the PWM is probed, but no one ever does anything
with it (and therefore no one turns it off), we must release the
exclusive rate in remove.

--Sean

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

[Uwe Kleine-König]

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

Hello Sean,

On Tue, May 04, 2021 at 10:46:39AM -0400, Sean Anderson wrote:
> On 5/4/21 4:51 AM, Uwe Kleine-König wrote:
> > On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote:
> >> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
> >> found on Xilinx FPGAs. There is another driver for this device located
> >> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
> >> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
> >>
> >> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> >> ---
> >>
> >>   arch/arm64/configs/defconfig |   1 +
> >>   drivers/pwm/Kconfig          |  11 ++
> >>   drivers/pwm/Makefile         |   1 +
> >>   drivers/pwm/pwm-xilinx.c     | 322 +++++++++++++++++++++++++++++++++++
> >>   4 files changed, 335 insertions(+)
> >>   create mode 100644 drivers/pwm/pwm-xilinx.c
> >>
> >> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> >> index 08c6f769df9a..81794209f287 100644
> >> --- a/arch/arm64/configs/defconfig
> >> +++ b/arch/arm64/configs/defconfig
> >> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y
> >>   CONFIG_PWM_SL28CPLD=m
> >>   CONFIG_PWM_SUN4I=m
> >>   CONFIG_PWM_TEGRA=m
> >> +CONFIG_PWM_XILINX=m
> >>   CONFIG_SL28CPLD_INTC=y
> >>   CONFIG_QCOM_PDC=y
> >>   CONFIG_RESET_IMX7=y
> >
> > I think this should go into a separate patch once this driver is
> > accepted. This can then go via the ARM people.
> 
> Sure.
> 
> >
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index d3371ac7b871..01e62928f4bf 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -628,4 +628,15 @@ 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 !MICROBLAZE
> >
> > I don't understand this dependency.
> 
> As noted in the commit message, there is already a driver for this
> device for microblaze systems. To prevent conflicts, this driver is
> disabled on microblaze.

OK, already understood that after reading the other replies to this
thread. As noted there, please add a comment describing the problem. You
probably also need some more dependencies, at least COMMON_CLK for
clk_rate_exclusive_get() and probably also HAS_IOMEM for readl/writel.

> >> +#include <asm/io.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/device.h>
> >> +#include <linux/debugfs.h>
> >> +#include <linux/module.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pwm.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)
> >
> > I'd like to see a prefix for these defines, something like XILINX_PWM_
> > maybe?
> 
> I don't think that's necessary, since these defines are used only for
> this file. In particular, adding a prefix like that would significantly
> lengthen lines which add several flags.

This is something where Thierry and I don't agree, so you can at least
expect support for your point. Still I see a value that is usually worth
the additional horizontal space.

The prefix would make it obvious that this is a local symbol. It also
helps to jump to the right definition if you use ctags or something
similar.

The most affected code lines are probably:

+	return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) ||
+		 ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR);

which IMHO is already hard to read and adding a newline after each ||
doesn't hurt.

Or write it as:

	#define XILINX_PWM_TCSR_RUN_SET			\
			(XILINX_PWM_TCSR_GENT |		\
			 XILINX_PWM_TCSR_ARHT |		\
			 XILINX_PWM_TCSR_ENT |		\
			 XILINX_PWM_TCSR_PWMA)

	#define XILINX_PWM_TCSR_RUN_CLEAR		\
			 (XILINX_PWM_TCSR_MDT | \
			 XILINX_PWM_TCSR_LOAD)

	#define XILINX_PWM_TCSR_RUN_MASK		\
			(XILINX_PWM_TCSR_RUN_SET | XILINX_PWM_TCSR_RUN_CLEAR)


	...

	/*
	 * Both TCSR registers must be setup according to the running
	 * value, TCSR0 must also have the CASC bit set.
	 */
	if ((tcsr0 & XILINX_PWM_TCSR_RUN_MASK) != XILINX_PWM_TCSR_RUN_SET ||
	    !(tcsr0 & TCSR_CASC) ||
	    (tcsr1 & XILINX_PWM_TCSR_RUN_MASK) != XILINX_PWM_TCSR_RUN_SET)

which IMHO is better understandable and still fits the line length.
(Or simplify the check to only test for a single bit?)

+			tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
+			tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);

is similar. I wonder if it makes sense to drop ENT from the default bit
mask and if after that xilinx_pwm_is_enabled() yields true. I also don't
understand the semantic of the UDT bit. You never modify it. Assuming it
has a relevant meaning the driver's behaviour depends on the value the
bootloader initialized that bit to?

So these lines might need some explanation anyhow and having to write it
over several lines doesn't actually hurt.

	bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
					     readl(pwm->regs + TCSR1));

could be rewritten as

	u32 tcsr0 = readl(pwm->regs + XILINX_PWM_TCSR0);
	u32 tcsr1 = readl(pwm->regs + XILINX_PWM_TCSR1);
	bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);

which IMHO reads better and the required horizontal space is even less
than with your lines even tough I have added a prefix.

> >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> >> +			    const struct pwm_state *state)
> >> +{
> >> +	struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip);
> >> +	u32 tlr0, tlr1;
> >> +	u32 tcsr0 = readl(pwm->regs + TCSR0);
> >> +	u32 tcsr1 = readl(pwm->regs + TCSR1);
> >> +	bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
> >> +
> >> +	if (state->polarity != PWM_POLARITY_NORMAL)
> >> +		return -EINVAL;
> >> +
> >> +	if (!enabled && state->enabled)
> >> +		clk_rate_exclusive_get(pwm->clk);
> >
> > Missing error checking.
> 
> This cannot fail.

Oh indeed. IMHO it should be changed to return void and the comment
"Returns 0 on success, -EERROR otherwise" should be adapted to reality.

> >> +	tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle);
> >> +	writel(tlr0, pwm->regs + TLR0);
> >> +	writel(tlr1, pwm->regs + TLR1);
> >
> > How does the hardware behave here? E.g. can it happen that after writing
> > TLR0 we can see a cycle with the new period but the old duty_cycle? Does
> > it complete the currently running period?
> 
> TLR is the value loaded into TCR when the counter resets. I believe this
> happens at the rising edge for TCR0 and at the falling edge for TCR1,
> but the exact behavior is not documented. Therefore, changing the value
> of TLR will not affect the current cycle.

So we might still get a mixed period if the counter resets between the
two writel commands, right?

> > If state->enabled == 0 you want to disable first to prevent that the
> > (maybe) new values for period and duty_cycle are emitted by the
> > hardware.
> >
> >> +	if (state->enabled) {
> >> +		/* Only touch the TCSRs if we aren't already running */
> >> +		if (!enabled) {
> >> +			/* Load TLR into TCR */
> >> +			writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0);
> >> +			writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1);
> >> +			/* Enable timers all at once with ENALL */
> >> +			tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
> >> +			tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
> >> +			writel(tcsr0, pwm->regs + TCSR0);
> >> +			writel(tcsr1, pwm->regs + TCSR1);
> >> +		}
> >> +	} else {
> >> +		writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0);
> >> +		writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1);
> >
> > How does the hardware behave on disable? Does it always emit a low
> > level? Does it complete the currently running period?
> 
> I don't know. My suspicion is that the output stops at whatever its
> current value is.

Would be great if you could find out that. Please document that in the
Limitations paragraph at the top of the driver.
 
> > This is unusual to have in a driver. Something like memtool[1] should
> > work just fine, then you don't need this debugfs file. Together with pwm
> > tracing this should be good enough.
> 
> I didn't have memtool on my test system, so I added this. I can remove
> it from the next patch if you'd like.

Yes, please remove it. (And install memtool or one of its alternatives
:-).

> >> +	pwm->clk = devm_clk_get(dev, NULL);
> >> +	if (IS_ERR(pwm->clk))
> >> +		return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n");
> >> +
> >> +	ret = clk_prepare_enable(pwm->clk);
> >> +	if (ret) {
> >> +		dev_err(dev, "clock enable failed\n");
> >> +		return ret;
> >
> > You can use dev_err_probe here, too, for consistency.
> 
> Ok. Though in this case we will never get -EPROBE_DEFER from
> clk_prepare_enable, so dev_err_probe is not as useful.

Everything it does here is useful, and the overhead is only the check
against -EPROBE_DEFER. In return you get consistent error handling and
the messages are also all in the same format. (And compared to your
version the error message also gives a hint about the actual problem as
it emits the error code.)

> >> +static int xilinx_pwm_remove(struct platform_device *pdev)
> >> +{
> >> +	struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
> >> +	bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
> >> +					     readl(pwm->regs + TCSR1));
> >> +
> >> +	if (enabled)
> >> +		clk_rate_exclusive_put(pwm->clk);

Note that I misunderstood what clk_rate_exclusive_put does. This only
drops the lock on the clock that prevents that others modify the clock
rate. So that call is fine.

> > This looks wrong. You should rely on the consumer that they disable the
> > PWM.
> 
> What about a PWM regulator with always-on?

So in general: What do you think should happen? When the remove function
is called the PWM regulator driver is already gone, so there is no way
to get the information that the PWM should stay on ...
 
> >> +	clk_disable_unprepare(pwm->clk);

... and still you stop the PWM here (or does that only affect access to
its registers?)

So it's a delicate topic and there isn't the single right thing to do
here. So usually I request to free all resources allocated in .probe()
and otherwise leave the hardware alone. Now that I understood
clk_rate_exclusive_put() the status quo is fine for me. Just call
pwmchip_remove first and only then release the resources needed to
operate the PWM.

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 2/2] pwm: Add support for Xilinx AXI Timer

[Sean Anderson]

On 5/4/21 1:15 PM, Uwe Kleine-König wrote:
 > Hello Sean,
 >
 > On Tue, May 04, 2021 at 10:46:39AM -0400, Sean Anderson wrote:
 >> On 5/4/21 4:51 AM, Uwe Kleine-König wrote:
 >>> On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote:
 >>>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
 >>>> found on Xilinx FPGAs. There is another driver for this device located
 >>>> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
 >>>> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
 >>>>
 >>>> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
 >>>>
 >>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >>>> ---
 >>>>
 >>>>    arch/arm64/configs/defconfig |   1 +
 >>>>    drivers/pwm/Kconfig          |  11 ++
 >>>>    drivers/pwm/Makefile         |   1 +
 >>>>    drivers/pwm/pwm-xilinx.c     | 322 +++++++++++++++++++++++++++++++++++
 >>>>    4 files changed, 335 insertions(+)
 >>>>    create mode 100644 drivers/pwm/pwm-xilinx.c
 >>>>
 >>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
 >>>> index 08c6f769df9a..81794209f287 100644
 >>>> --- a/arch/arm64/configs/defconfig
 >>>> +++ b/arch/arm64/configs/defconfig
 >>>> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y
 >>>>    CONFIG_PWM_SL28CPLD=m
 >>>>    CONFIG_PWM_SUN4I=m
 >>>>    CONFIG_PWM_TEGRA=m
 >>>> +CONFIG_PWM_XILINX=m
 >>>>    CONFIG_SL28CPLD_INTC=y
 >>>>    CONFIG_QCOM_PDC=y
 >>>>    CONFIG_RESET_IMX7=y
 >>>
 >>> I think this should go into a separate patch once this driver is
 >>> accepted. This can then go via the ARM people.
 >>
 >> Sure.
 >>
 >>>
 >>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
 >>>> index d3371ac7b871..01e62928f4bf 100644
 >>>> --- a/drivers/pwm/Kconfig
 >>>> +++ b/drivers/pwm/Kconfig
 >>>> @@ -628,4 +628,15 @@ 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 !MICROBLAZE
 >>>
 >>> I don't understand this dependency.
 >>
 >> As noted in the commit message, there is already a driver for this
 >> device for microblaze systems. To prevent conflicts, this driver is
 >> disabled on microblaze.
 >
 > OK, already understood that after reading the other replies to this
 > thread. As noted there, please add a comment describing the problem.

Added.

 > You probably also need some more dependencies, at least COMMON_CLK for
 > clk_rate_exclusive_get() and probably also HAS_IOMEM for readl/writel.

I will add a dependency on HAS_IOMEM. COMMON_CLK should not be
necessary, because clk_exclusive_(get|put) should be defined by the
platform if they define CONFIG_HAVE_CLK (otherwise we get a stub as
define in the linux clock header). But of course, this is not the case
on SH4... I wish this sort of thing were documented somewhere. I can add
this if necessary.

 >
 >>>> +#include <asm/io.h>
 >>>> +#include <linux/clk.h>
 >>>> +#include <linux/device.h>
 >>>> +#include <linux/debugfs.h>
 >>>> +#include <linux/module.h>
 >>>> +#include <linux/platform_device.h>
 >>>> +#include <linux/pwm.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)
 >>>
 >>> I'd like to see a prefix for these defines, something like XILINX_PWM_
 >>> maybe?
 >>
 >> I don't think that's necessary, since these defines are used only for
 >> this file. In particular, adding a prefix like that would significantly
 >> lengthen lines which add several flags.
 >
 > This is something where Thierry and I don't agree, so you can at least
 > expect support for your point. Still I see a value that is usually worth
 > the additional horizontal space.
 >
 > The prefix would make it obvious that this is a local symbol. It also
 > helps to jump to the right definition if you use ctags or something
 > similar.
 >
 > The most affected code lines are probably:
 >
 > +	return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) ||
 > +		 ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR);
 >
 > which IMHO is already hard to read and adding a newline after each ||
 > doesn't hurt.

This is done intentionally to highlight that we are doing the same
checks for TCSR0 and TCSR1.

 >
 > Or write it as:
 >
 > 	#define XILINX_PWM_TCSR_RUN_SET			\
 > 			(XILINX_PWM_TCSR_GENT |		\
 > 			 XILINX_PWM_TCSR_ARHT |		\
 > 			 XILINX_PWM_TCSR_ENT |		\
 > 			 XILINX_PWM_TCSR_PWMA)
 >
 > 	#define XILINX_PWM_TCSR_RUN_CLEAR		\
 > 			 (XILINX_PWM_TCSR_MDT | \
 > 			 XILINX_PWM_TCSR_LOAD)
 >
 > 	#define XILINX_PWM_TCSR_RUN_MASK		\
 > 			(XILINX_PWM_TCSR_RUN_SET | XILINX_PWM_TCSR_RUN_CLEAR)
 >
 >
 > 	...
 >
 > 	/*
 > 	 * Both TCSR registers must be setup according to the running
 > 	 * value, TCSR0 must also have the CASC bit set.
 > 	 */
 > 	if ((tcsr0 & XILINX_PWM_TCSR_RUN_MASK) != XILINX_PWM_TCSR_RUN_SET ||
 > 	    !(tcsr0 & TCSR_CASC) ||
 > 	    (tcsr1 & XILINX_PWM_TCSR_RUN_MASK) != XILINX_PWM_TCSR_RUN_SET)
 >
 > which IMHO is better understandable and still fits the line length.
 > (Or simplify the check to only test for a single bit?)
 >
 > +			tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
 > +			tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
 >
 > is similar. I wonder if it makes sense to drop ENT from the default bit
 > mask and if after that xilinx_pwm_is_enabled() yields true.

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

 > I also don't understand the semantic of the UDT bit. You never modify
 > it. Assuming it has a relevant meaning the driver's behaviour depends
 > on the value the bootloader initialized that bit to?

UDT is whether the timer counts upward or downward. We keep whatever
the bootloader (or the power-on reset) set so we don't cause an
incorrect pulse between when we write TLR and when we write TCSR.

 >
 > So these lines might need some explanation anyhow and having to write it
 > over several lines doesn't actually hurt.
 >
 > 	bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
 > 					     readl(pwm->regs + TCSR1));
 >
 > could be rewritten as
 >
 > 	u32 tcsr0 = readl(pwm->regs + XILINX_PWM_TCSR0);
 > 	u32 tcsr1 = readl(pwm->regs + XILINX_PWM_TCSR1);
 > 	bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
 >
 > which IMHO reads better and the required horizontal space is even less
 > than with your lines even tough I have added a prefix.
 >
 >>>> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
 >>>> +			    const struct pwm_state *state)
 >>>> +{
 >>>> +	struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip);
 >>>> +	u32 tlr0, tlr1;
 >>>> +	u32 tcsr0 = readl(pwm->regs + TCSR0);
 >>>> +	u32 tcsr1 = readl(pwm->regs + TCSR1);
 >>>> +	bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
 >>>> +
 >>>> +	if (state->polarity != PWM_POLARITY_NORMAL)
 >>>> +		return -EINVAL;
 >>>> +
 >>>> +	if (!enabled && state->enabled)
 >>>> +		clk_rate_exclusive_get(pwm->clk);
 >>>
 >>> Missing error checking.
 >>
 >> This cannot fail.
 >
 > Oh indeed. IMHO it should be changed to return void and the comment
 > "Returns 0 on success, -EERROR otherwise" should be adapted to reality.
 >
 >>>> +	tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle);
 >>>> +	writel(tlr0, pwm->regs + TLR0);
 >>>> +	writel(tlr1, pwm->regs + TLR1);
 >>>
 >>> How does the hardware behave here? E.g. can it happen that after writing
 >>> TLR0 we can see a cycle with the new period but the old duty_cycle? Does
 >>> it complete the currently running period?
 >>
 >> TLR is the value loaded into TCR when the counter resets. I believe this
 >> happens at the rising edge for TCR0 and at the falling edge for TCR1,
 >> but the exact behavior is not documented. Therefore, changing the value
 >> of TLR will not affect the current cycle.
 >
 > So we might still get a mixed period if the counter resets between the
 > two writel commands, right?

Yes. I have noted this in V2.

 >>> If state->enabled == 0 you want to disable first to prevent that the
 >>> (maybe) new values for period and duty_cycle are emitted by the
 >>> hardware.
 >>>
 >>>> +	if (state->enabled) {
 >>>> +		/* Only touch the TCSRs if we aren't already running */
 >>>> +		if (!enabled) {
 >>>> +			/* Load TLR into TCR */
 >>>> +			writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0);
 >>>> +			writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1);
 >>>> +			/* Enable timers all at once with ENALL */
 >>>> +			tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
 >>>> +			tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
 >>>> +			writel(tcsr0, pwm->regs + TCSR0);
 >>>> +			writel(tcsr1, pwm->regs + TCSR1);
 >>>> +		}
 >>>> +	} else {
 >>>> +		writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0);
 >>>> +		writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1);
 >>>
 >>> How does the hardware behave on disable? Does it always emit a low
 >>> level? Does it complete the currently running period?
 >>
 >> I don't know. My suspicion is that the output stops at whatever its
 >> current value is.
 >
 > Would be great if you could find out that. Please document that in the
 > Limitations paragraph at the top of the driver.

Well, as it turns out, just clearing ENT doesn't disable the PWM output
(!). However, clearing everything in TCSR_SET does disable the PWM. The
output is low when it is disabled.

 >>> This is unusual to have in a driver. Something like memtool[1] should
 >>> work just fine, then you don't need this debugfs file. Together with pwm
 >>> tracing this should be good enough.
 >>
 >> I didn't have memtool on my test system, so I added this. I can remove
 >> it from the next patch if you'd like.
 >
 > Yes, please remove it. (And install memtool or one of its alternatives
 > :-).

I'll be sad to see it go. It is very convenient for debugging.

 >>>> +	pwm->clk = devm_clk_get(dev, NULL);
 >>>> +	if (IS_ERR(pwm->clk))
 >>>> +		return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n");
 >>>> +
 >>>> +	ret = clk_prepare_enable(pwm->clk);
 >>>> +	if (ret) {
 >>>> +		dev_err(dev, "clock enable failed\n");
 >>>> +		return ret;
 >>>
 >>> You can use dev_err_probe here, too, for consistency.
 >>
 >> Ok. Though in this case we will never get -EPROBE_DEFER from
 >> clk_prepare_enable, so dev_err_probe is not as useful.
 >
 > Everything it does here is useful, and the overhead is only the check
 > against -EPROBE_DEFER. In return you get consistent error handling and
 > the messages are also all in the same format. (And compared to your
 > version the error message also gives a hint about the actual problem as
 > it emits the error code.)

Shouldn't the error return be printed by whoever called this probe?

 >>>> +static int xilinx_pwm_remove(struct platform_device *pdev)
 >>>> +{
 >>>> +	struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
 >>>> +	bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
 >>>> +					     readl(pwm->regs + TCSR1));
 >>>> +
 >>>> +	if (enabled)
 >>>> +		clk_rate_exclusive_put(pwm->clk);
 >
 > Note that I misunderstood what clk_rate_exclusive_put does. This only
 > drops the lock on the clock that prevents that others modify the clock
 > rate. So that call is fine.
 >
 >>> This looks wrong. You should rely on the consumer that they disable the
 >>> PWM.
 >>
 >> What about a PWM regulator with always-on?
 >
 > So in general: What do you think should happen? When the remove function
 > is called the PWM regulator driver is already gone, so there is no way
 > to get the information that the PWM should stay on ...

Well, we can still tell whether the PWM is on or off. If it should turn
off, then someone should have disabled it.

 >>>> +	clk_disable_unprepare(pwm->clk);
 >
 > ... and still you stop the PWM here (or does that only affect access to
 > its registers?)

This will turn off the PWM if we are the last user of this clock and no
one else has set e.g. CLK_IS_CRITICAL (which is what I would expect if
this powered a critical regulator).

--Sean

 >
 > So it's a delicate topic and there isn't the single right thing to do
 > here. So usually I request to free all resources allocated in .probe()
 > and otherwise leave the hardware alone. Now that I understood
 > clk_rate_exclusive_put() the status quo is fine for me. Just call
 > pwmchip_remove first and only then release the resources needed to
 > operate the PWM.
 >
 > Best regards
 > Uwe
 >

Re: [PATCH 1/2] dt-bindings: pwm: Add Xilinx AXI Timer

[Rob Herring]

On Mon, 03 May 2021 17:44:12 -0400, Sean Anderson wrote:
> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
> a "soft" block, so it has many 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 properties
> must be kept as they are.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
>  .../bindings/pwm/xlnx,axi-timer.yaml          | 91 +++++++++++++++++++
>  1 file changed, 91 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.example.dt.yaml: example-0: timer@800e0000:reg:0: [0, 2148401152, 0, 65536] is too long
	From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml

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

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

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

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 1/2] dt-bindings: pwm: Add Xilinx AXI Timer

[Sean Anderson]

On 5/3/21 8:21 PM, Rob Herring wrote:
 > On Mon, 03 May 2021 17:44:12 -0400, Sean Anderson wrote:
 >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
 >> a "soft" block, so it has many 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 properties
 >> must be kept as they are.
 >>
 >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >> ---
 >>
 >>   .../bindings/pwm/xlnx,axi-timer.yaml          | 91 +++++++++++++++++++
 >>   1 file changed, 91 insertions(+)
 >>   create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >>
 >
 > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
 > on your patch (DT_CHECKER_FLAGS is new in v5.13):
 >
 > yamllint warnings/errors:
 >
 > dtschema/dtc warnings/errors:
 > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.example.dt.yaml: example-0: timer@800e0000:reg:0: [0, 2148401152, 0, 65536] is too long
 > 	From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml

What is the correct way to specify the 'reg' property? I see many
schemas doing what I did here which is 'reg: maxItems: 1'. Should I
instead use 'reg: true'?

--Sean

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