[PATCH v4 0/3] Add initial support for TTC PWM driver

[PATCH v4 0/3] Add initial support for TTC PWM driver

[Mubin Sayyed]

Adds initial TTC PWM driver.In case of TTC configured as PWM device,
TTC clocksource driver calls probe of PWM functionality and registers TTC
device with PWM framework.  With these changes drivers/pwm/pwm-cadence.c would be 
part of drivers/clocksource/timer-cadence-ttc.c driver.

link for v2:
 https://lore.kernel.org/linux-arm-kernel/20231114124748.581850-2-mubin.sayyed@amd.com/T/#mfb786b5d23fc8948375b9b570f02a0e5a8095095

Note: Bindings are merged as part of v3 
 https://lore.kernel.org/lkml/46630fa5-381a-4006-ade4-2c18e76331ff@linaro.org/T/
 
Mubin Sayyed (3):
  clocksource: timer-cadence-ttc: Prepare to support TTC PWM
  clocksource: timer-cadence-ttc: Support TTC device configured as PWM
  pwm: pwm-cadence: Add support for TTC PWM

 drivers/clocksource/timer-cadence-ttc.c |  62 +++--
 drivers/pwm/Kconfig                     |  10 +
 drivers/pwm/Makefile                    |   1 +
 drivers/pwm/pwm-cadence.c               | 323 ++++++++++++++++++++++++
 include/linux/timer-cadence-ttc.h       |  55 ++++
 5 files changed, 423 insertions(+), 28 deletions(-)
 create mode 100644 drivers/pwm/pwm-cadence.c
 create mode 100644 include/linux/timer-cadence-ttc.h

-- 
2.25.1

[PATCH v4 1/3] clocksource: timer-cadence-ttc: Prepare to support TTC PWM

[Mubin Sayyed]

Cadence TTC IP supports timer as well as PWM feature.Existing driver
supports only timer functionality. PWM feature would be exposed through
separate file which is going to be added in drivers/pwm directory.
Move #defines related to TTC IP to timer-cadence-ttc.h, so that they
can be re-used by PWM part of the driver.

Signed-off-by: Mubin Sayyed <mubin.sayyed@amd.com>
---
Changes for v4:
  - New patch
---
 drivers/clocksource/timer-cadence-ttc.c | 30 +--------------------
 include/linux/timer-cadence-ttc.h       | 35 +++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 29 deletions(-)
 create mode 100644 include/linux/timer-cadence-ttc.h

diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
index b8a1cf59b9d6..2f33d4c40153 100644
--- a/drivers/clocksource/timer-cadence-ttc.c
+++ b/drivers/clocksource/timer-cadence-ttc.c
@@ -18,6 +18,7 @@
 #include <linux/sched_clock.h>
 #include <linux/module.h>
 #include <linux/of_platform.h>
+#include <linux/timer-cadence-ttc.h>
 
 /*
  * This driver configures the 2 16/32-bit count-up timers as follows:
@@ -34,35 +35,6 @@
  * obtained from device tree. The pre-scaler of 32 is used.
  */
 
-/*
- * Timer Register Offset Definitions of Timer 1, Increment base address by 4
- * and use same offsets for Timer 2
- */
-#define TTC_CLK_CNTRL_OFFSET		0x00 /* Clock Control Reg, RW */
-#define TTC_CNT_CNTRL_OFFSET		0x0C /* Counter Control Reg, RW */
-#define TTC_COUNT_VAL_OFFSET		0x18 /* Counter Value Reg, RO */
-#define TTC_INTR_VAL_OFFSET		0x24 /* Interval Count Reg, RW */
-#define TTC_ISR_OFFSET		0x54 /* Interrupt Status Reg, RO */
-#define TTC_IER_OFFSET		0x60 /* Interrupt Enable Reg, RW */
-
-#define TTC_CNT_CNTRL_DISABLE_MASK	0x1
-
-#define TTC_CLK_CNTRL_CSRC_MASK		(1 << 5)	/* clock source */
-#define TTC_CLK_CNTRL_PSV_MASK		0x1e
-#define TTC_CLK_CNTRL_PSV_SHIFT		1
-
-/*
- * Setup the timers to use pre-scaling, using a fixed value for now that will
- * work across most input frequency, but it may need to be more dynamic
- */
-#define PRESCALE_EXPONENT	11	/* 2 ^ PRESCALE_EXPONENT = PRESCALE */
-#define PRESCALE		2048	/* The exponent must match this */
-#define CLK_CNTRL_PRESCALE	((PRESCALE_EXPONENT - 1) << 1)
-#define CLK_CNTRL_PRESCALE_EN	1
-#define CNT_CNTRL_RESET		(1 << 4)
-
-#define MAX_F_ERR 50
-
 /**
  * struct ttc_timer - This definition defines local timer structure
  *
diff --git a/include/linux/timer-cadence-ttc.h b/include/linux/timer-cadence-ttc.h
new file mode 100644
index 000000000000..d938991371e5
--- /dev/null
+++ b/include/linux/timer-cadence-ttc.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ *Copyright (c) 2025 Advanced Micro Devices, Inc.
+ */
+
+/*
+ * Timer Register Offset Definitions of Timer 1, Increment base address by 4
+ * and use same offsets for Timer 2
+ */
+#define TTC_CLK_CNTRL_OFFSET            0x00 /* Clock Control Reg, RW */
+#define TTC_CNT_CNTRL_OFFSET            0x0C /* Counter Control Reg, RW */
+#define TTC_COUNT_VAL_OFFSET            0x18 /* Counter Value Reg, RO */
+#define TTC_INTR_VAL_OFFSET             0x24 /* Interval Count Reg, RW */
+#define TTC_ISR_OFFSET          0x54 /* Interrupt Status Reg, RO */
+#define TTC_IER_OFFSET          0x60 /* Interrupt Enable Reg, RW */
+
+#define TTC_CNT_CNTRL_DISABLE_MASK      0x1
+
+#define TTC_CLK_CNTRL_CSRC_MASK         (1 << 5)        /* clock source */
+#define TTC_CLK_CNTRL_PSV_MASK          0x1e
+#define TTC_CLK_CNTRL_PSV_SHIFT         1
+
+/*
+ * Setup the timers to use pre-scaling, using a fixed value for now that will
+ * work across most input frequency, but it may need to be more dynamic
+ */
+#define PRESCALE_EXPONENT       11      /* 2 ^ PRESCALE_EXPONENT = PRESCALE */
+#define PRESCALE                2048    /* The exponent must match this */
+#define CLK_CNTRL_PRESCALE      ((PRESCALE_EXPONENT - 1) << 1)
+#define CLK_CNTRL_PRESCALE_EN   1
+#define CNT_CNTRL_RESET         (1 << 4)
+
+#define MAX_F_ERR 50
+
-- 
2.25.1

[PATCH v4 2/3] clocksource: timer-cadence-ttc: Support TTC device configured as PWM

[Mubin Sayyed]

TTC device can act either as clocksource/clockevent or PWM generator,
it would be decided by pwm-cells property. If pwm-cells property is
present in TTC node, it would be treated as PWM device, and clocksource
driver just calls probe function for PWM functionality, so that TTC
device would be registered with PWM framework.

Signed-off-by: Mubin Sayyed <mubin.sayyed@amd.com>
---
Changes for v4:
    - In case of pwm-cells property call probe function for PWM
      feature instead of returning error.
Changes for v3:
    - None
Changes for v2:
    - Added comment regarding pwm-cells property
---
 drivers/clocksource/timer-cadence-ttc.c | 34 +++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
index 2f33d4c40153..c5ecad9332c9 100644
--- a/drivers/clocksource/timer-cadence-ttc.c
+++ b/drivers/clocksource/timer-cadence-ttc.c
@@ -35,6 +35,10 @@
  * obtained from device tree. The pre-scaler of 32 is used.
  */
 
+struct ttc_timer_config {
+	bool is_pwm_mode;
+};
+
 /**
  * struct ttc_timer - This definition defines local timer structure
  *
@@ -453,6 +457,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
 
 static int __init ttc_timer_probe(struct platform_device *pdev)
 {
+	struct ttc_timer_config *ttc_config;
 	unsigned int irq;
 	void __iomem *timer_baseaddr;
 	struct clk *clk_cs, *clk_ce;
@@ -461,6 +466,24 @@ static int __init ttc_timer_probe(struct platform_device *pdev)
 	u32 timer_width = 16;
 	struct device_node *timer = pdev->dev.of_node;
 
+	ttc_config = devm_kzalloc(&pdev->dev, sizeof(*ttc_config), GFP_KERNEL);
+	if (!ttc_config)
+		return -ENOMEM;
+
+	/*
+	 * If pwm-cells property is present in TTC node,
+	 * it would be treated as PWM device.
+	 */
+	if (of_property_read_bool(timer, "#pwm-cells")) {
+		#if defined(CONFIG_PWM_CADENCE)
+		ttc_config->is_pwm_mode = true;
+			return ttc_pwm_probe(pdev);
+		#else
+			return -ENODEV;
+		#endif
+	}
+	dev_set_drvdata(&pdev->dev, ttc_config);
+
 	if (initialized)
 		return 0;
 
@@ -521,6 +544,16 @@ static int __init ttc_timer_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static void ttc_timer_remove(struct platform_device *pdev)
+{
+	#if defined(CONFIG_PWM_CADENCE)
+	struct ttc_timer_config *ttc_config = dev_get_drvdata(&pdev->dev);
+
+	if (ttc_config->is_pwm_mode)
+		ttc_pwm_remove(pdev);
+	#endif
+}
+
 static const struct of_device_id ttc_timer_of_match[] = {
 	{.compatible = "cdns,ttc"},
 	{},
@@ -529,6 +562,7 @@ static const struct of_device_id ttc_timer_of_match[] = {
 MODULE_DEVICE_TABLE(of, ttc_timer_of_match);
 
 static struct platform_driver ttc_timer_driver = {
+	.remove = ttc_timer_remove,
 	.driver = {
 		.name	= "cdns_ttc_timer",
 		.of_match_table = ttc_timer_of_match,
-- 
2.25.1

[PATCH v4 3/3] pwm: pwm-cadence: Add support for TTC PWM

[Mubin Sayyed]

Cadence TTC timer can be configured as clocksource/clockevent or PWM
device.Specific TTC device would be configured as PWM device, if
pwm-cells property is present in the device tree node.

In case of Zynq, ZynqMP and Versal SoC's, each TTC device has 3
timers/counters, so maximum 3 PWM channels can be configured for each TTC
IP instance. Also, output of 0th PWM channel of each TTC device can be
routed to MIO or EMIO, and output of 2nd and 3rd PWM channel can be
routed only to EMIO.

Period for given PWM channel is configured through interval timer and
duty cycle through match counter.

Details for cadence TTC IP can be found in Zynq UltraScale+ TRM.

Signed-off-by: Mubin Sayyed <mubin.sayyed@amd.com>
---
Refer link given below for Zynq UltraScale+ TRM
https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm

Changes for v4:
 Configure it as part of TTC clocksource/clockevent driver
 drivers/clocksource/timer-cadence-ttc.c.
 Move probe/remove function to timer-cadence-ttc.c.
Changes for v3:
 None
Changes for v2:
 Use maybe_unused attribute for ttc_pwm_of_match_driver structure
 Add new function ttc_pwm_set_polarity
 Removed calls to pwm_get_state
 Replace DIV_ROUNF_CLOSEST with mul_u64_u64_div_u64
 Modify ttc_pwm_apply to remove while loop in prescalar logic
 and avoid glitch
 Calculate rate in probe and add it to private structure for further
 Drop ttc_pwm_of_xlate
 Replace of_clk_get with devm_clk_get_enabled
 Drop _OFFSET and _MASK from definitions
 Keep Kconfig and Makefile changes alphabetically sorted
 Use remove_new instead of remove
 Document limitations in driver file
---
 drivers/pwm/Kconfig               |  10 +
 drivers/pwm/Makefile              |   1 +
 drivers/pwm/pwm-cadence.c         | 323 ++++++++++++++++++++++++++++++
 include/linux/timer-cadence-ttc.h |  22 +-
 4 files changed, 355 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pwm/pwm-cadence.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 0915c1e7df16..b418e5d8fa42 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -202,6 +202,16 @@ config PWM_CROS_EC
 	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
 	  Controller.
 
+config PWM_CADENCE
+	bool "Cadence TTC PWM driver"
+	depends on CADENCE_TTC_TIMER
+	help
+	  Generic PWM framework driver for cadence TTC IP found on
+          Xilinx Zynq/ZynqMP/Versal SOCs. Each TTC device has 3 PWM
+          channels. Output of 0th PWM channel of each TTC device can
+          be routed to MIO or EMIO, and output of 1st and 2nd PWM
+          channels can be routed only to EMIO.
+
 config PWM_DWC_CORE
 	tristate
 	depends on HAS_IOMEM
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9081e0c0e9e0..246380391a63 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_PWM_BCM_KONA)	+= pwm-bcm-kona.o
 obj-$(CONFIG_PWM_BCM2835)	+= pwm-bcm2835.o
 obj-$(CONFIG_PWM_BERLIN)	+= pwm-berlin.o
 obj-$(CONFIG_PWM_BRCMSTB)	+= pwm-brcmstb.o
+obj-$(CONFIG_PWM_CADENCE)	+= pwm-cadence.o
 obj-$(CONFIG_PWM_CLK)		+= pwm-clk.o
 obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
diff --git a/drivers/pwm/pwm-cadence.c b/drivers/pwm/pwm-cadence.c
new file mode 100644
index 000000000000..e7c337fe956b
--- /dev/null
+++ b/drivers/pwm/pwm-cadence.c
@@ -0,0 +1,323 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver to configure cadence TTC timer as PWM
+ * generator
+ *
+ * Limitations:
+ * - When PWM is stopped, timer counter gets stopped immediately. This
+ *   doesn't allow the current PWM period to complete and stops abruptly.
+ * - Disabled PWM emits inactive level.
+ * - When user requests a change in  any parameter of PWM (period/duty cycle/polarity)
+ *   while PWM is in enabled state:
+ *	- PWM is stopped abruptly.
+ *	- Requested parameter is changed.
+ *	- Fresh PWM cycle is started.
+ *
+ * Copyright (C) 2025, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/of_address.h>
+#include <linux/timer-cadence-ttc.h>
+
+/**
+ * struct ttc_pwm_priv - Private data for TTC PWM drivers
+ * @chip:	PWM chip structure representing PWM controller
+ * @clk:	TTC input clock
+ * @rate:	TTC input clock rate
+ * @max:	Maximum value of the counters
+ * @base:	Base address of TTC instance
+ */
+struct ttc_pwm_priv {
+	struct pwm_chip chip;
+	struct clk *clk;
+	unsigned long rate;
+	u32 max;
+	void __iomem *base;
+};
+
+static inline u32 ttc_pwm_readl(struct ttc_pwm_priv *priv,
+				unsigned long offset)
+{
+	return readl_relaxed(priv->base + offset);
+}
+
+static inline void ttc_pwm_writel(struct ttc_pwm_priv *priv,
+				  unsigned long offset,
+				  unsigned long val)
+{
+	writel_relaxed(val, priv->base + offset);
+}
+
+static inline u32 ttc_pwm_ch_readl(struct ttc_pwm_priv *priv,
+				   unsigned int chnum,
+				   unsigned long offset)
+{
+	unsigned long pwm_ch_offset = offset +
+				       (TTC_PWM_CHANNEL * chnum);
+
+	return ttc_pwm_readl(priv, pwm_ch_offset);
+}
+
+static inline void ttc_pwm_ch_writel(struct ttc_pwm_priv *priv,
+				     unsigned int chnum,
+				     unsigned long offset,
+				     unsigned long val)
+{
+	unsigned long pwm_ch_offset = offset +
+				       (TTC_PWM_CHANNEL * chnum);
+
+	ttc_pwm_writel(priv, pwm_ch_offset, val);
+}
+
+static inline struct ttc_pwm_priv *xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static void ttc_pwm_enable(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
+{
+	u32 ctrl_reg;
+
+	ctrl_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET);
+	ctrl_reg |= (TTC_CNTR_CTRL_INTR_MODE_EN
+				 | TTC_CNTR_CTRL_MATCH_MODE_EN | TTC_CNTR_CTRL_RST);
+	ctrl_reg &= ~(TTC_CNTR_CTRL_DIS | TTC_CNTR_CTRL_WAVE_EN);
+	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
+}
+
+static void ttc_pwm_disable(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
+{
+	u32 ctrl_reg;
+
+	ctrl_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET);
+	ctrl_reg |= TTC_CNTR_CTRL_DIS;
+
+	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
+}
+
+static void ttc_pwm_set_polarity(struct ttc_pwm_priv *priv, struct pwm_device *pwm,
+				 enum pwm_polarity polarity)
+{
+	u32 ctrl_reg;
+
+	ctrl_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET);
+
+	if (polarity == PWM_POLARITY_NORMAL)
+		ctrl_reg |= TTC_CNTR_CTRL_WAVE_POL;
+	else
+		ctrl_reg &= (~TTC_CNTR_CTRL_WAVE_POL);
+
+	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
+}
+
+static void ttc_pwm_set_counters(struct ttc_pwm_priv *priv,
+				 struct pwm_device *pwm,
+				 u32 period_cycles,
+				 u32 duty_cycles)
+{
+	/* Set up period */
+	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_INTR_VAL_OFFSET, period_cycles);
+
+	/* Set up duty cycle */
+	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_MATCH_CNT_VAL_OFFSET, duty_cycles);
+}
+
+static void ttc_pwm_set_prescalar(struct ttc_pwm_priv *priv,
+				  struct pwm_device *pwm,
+				  u32 div, bool is_enable)
+{
+	u32 clk_reg;
+
+	if (is_enable) {
+		/* Set up prescalar */
+		clk_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET);
+		clk_reg &= ~TTC_CLK_CNTRL_PSV_MASK;
+		clk_reg |= (div << TTC_CNTR_CTRL_PRESCALE_SHIFT);
+		clk_reg |= TTC_CLK_CNTRL_PS_EN;
+		ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET, clk_reg);
+	} else {
+		/* Disable prescalar */
+		clk_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET);
+		clk_reg &= ~TTC_CLK_CNTRL_PS_EN;
+		ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET, clk_reg);
+	}
+}
+
+static int ttc_pwm_apply(struct pwm_chip *chip,
+			 struct pwm_device *pwm,
+			 const struct pwm_state *state)
+{
+	struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
+	u64 duty_cycles, period_cycles;
+	struct pwm_state cstate;
+	unsigned long rate;
+	bool flag = false;
+	u32 div = 0;
+
+	cstate = pwm->state;
+
+	if (state->polarity != cstate.polarity) {
+		if (cstate.enabled)
+			ttc_pwm_disable(priv, pwm);
+
+		ttc_pwm_set_polarity(priv, pwm, state->polarity);
+	}
+
+	rate = priv->rate;
+
+	/* Prevent overflow by limiting to the maximum possible period */
+	period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
+	period_cycles = mul_u64_u64_div_u64(period_cycles, rate, NSEC_PER_SEC);
+
+	if (period_cycles > priv->max) {
+		/*
+		 * Prescale frequency to fit requested period cycles within limit.
+		 * Prescalar divides input clock by 2^(prescale_value + 1). Maximum
+		 * supported prescalar value is 15.
+		 */
+		div = mul_u64_u64_div_u64(state->period, rate, (NSEC_PER_SEC * priv->max));
+		div = order_base_2(div);
+		if (div)
+			div -= 1;
+
+		if (div > 15)
+			return -ERANGE;
+
+		rate = DIV_ROUND_CLOSEST(rate, BIT(div + 1));
+		period_cycles = mul_u64_u64_div_u64(state->period, rate,
+						    NSEC_PER_SEC);
+		flag = true;
+	}
+
+	if (cstate.enabled)
+		ttc_pwm_disable(priv, pwm);
+
+	duty_cycles = mul_u64_u64_div_u64(state->duty_cycle, rate,
+					  NSEC_PER_SEC);
+	ttc_pwm_set_counters(priv, pwm, period_cycles, duty_cycles);
+
+	ttc_pwm_set_prescalar(priv, pwm, div, flag);
+
+	if (state->enabled)
+		ttc_pwm_enable(priv, pwm);
+	else
+		ttc_pwm_disable(priv, pwm);
+
+	return 0;
+}
+
+static int ttc_pwm_get_state(struct pwm_chip *chip,
+			     struct pwm_device *pwm,
+			     struct pwm_state *state)
+{
+	struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
+	u32 value, pres_en, pres = 1;
+	unsigned long rate;
+	u64 tmp;
+
+	value = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET);
+
+	if (value & TTC_CNTR_CTRL_WAVE_POL)
+		state->polarity = PWM_POLARITY_NORMAL;
+	else
+		state->polarity = PWM_POLARITY_INVERSED;
+
+	if (value & TTC_CNTR_CTRL_DIS)
+		state->enabled = false;
+	else
+		state->enabled = true;
+
+	rate = priv->rate;
+
+	pres_en = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET);
+	pres_en	&= TTC_CLK_CNTRL_PS_EN;
+
+	if (pres_en) {
+		pres = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET)
+					& TTC_CLK_CNTRL_PSV_MASK;
+		pres >>= TTC_CNTR_CTRL_PRESCALE_SHIFT;
+		/* If prescale is enabled, the count rate is divided by 2^(pres + 1) */
+		pres = BIT(pres + 1);
+	}
+
+	tmp = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_INTR_VAL_OFFSET);
+	tmp *= pres;
+	state->period = DIV64_U64_ROUND_UP(tmp * NSEC_PER_SEC, rate);
+
+	tmp = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_MATCH_CNT_VAL_OFFSET);
+	tmp *= pres;
+	state->duty_cycle = DIV64_U64_ROUND_UP(tmp * NSEC_PER_SEC, rate);
+
+	return 0;
+}
+
+static const struct pwm_ops ttc_pwm_ops = {
+	.apply = ttc_pwm_apply,
+	.get_state = ttc_pwm_get_state,
+};
+
+int ttc_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct ttc_pwm_priv *priv;
+	struct pwm_chip *chip;
+	u32 timer_width;
+	int ret;
+
+	ret = of_property_read_u32(np, "timer-width", &timer_width);
+	if (ret)
+		timer_width = 16;
+
+	chip = devm_pwmchip_alloc(dev, TTC_PWM_MAX_CH, sizeof(*priv));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	priv = xilinx_pwm_chip_to_priv(chip);
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->max = BIT(timer_width) - 1;
+
+	priv->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		return dev_err_probe(dev, PTR_ERR(priv->clk),
+				     "ERROR: timer input clock not found\n");
+	}
+
+	priv->rate = clk_get_rate(priv->clk);
+
+	clk_rate_exclusive_get(priv->clk);
+
+	chip->ops = &ttc_pwm_ops;
+	chip->npwm = TTC_PWM_MAX_CH;
+
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret) {
+		clk_rate_exclusive_put(priv->clk);
+		return dev_err_probe(dev, ret, "Could not register PWM chip\n");
+	}
+
+	platform_set_drvdata(pdev, priv);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ttc_pwm_probe);
+
+void ttc_pwm_remove(struct platform_device *pdev)
+{
+	struct ttc_pwm_priv *priv = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&priv->chip);
+	clk_rate_exclusive_put(priv->clk);
+}
+EXPORT_SYMBOL_GPL(ttc_pwm_remove);
+
+MODULE_AUTHOR("Mubin Sayyed <mubin.sayyed@amd.com>");
+MODULE_DESCRIPTION("Cadence TTC PWM driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/timer-cadence-ttc.h b/include/linux/timer-cadence-ttc.h
index d938991371e5..6b6135d0ba0c 100644
--- a/include/linux/timer-cadence-ttc.h
+++ b/include/linux/timer-cadence-ttc.h
@@ -12,13 +12,14 @@
 #define TTC_CNT_CNTRL_OFFSET            0x0C /* Counter Control Reg, RW */
 #define TTC_COUNT_VAL_OFFSET            0x18 /* Counter Value Reg, RO */
 #define TTC_INTR_VAL_OFFSET             0x24 /* Interval Count Reg, RW */
+#define TTC_MATCH_CNT_VAL_OFFSET        0x30 /* Match Count Reg, RW */
 #define TTC_ISR_OFFSET          0x54 /* Interrupt Status Reg, RO */
 #define TTC_IER_OFFSET          0x60 /* Interrupt Enable Reg, RW */
 
 #define TTC_CNT_CNTRL_DISABLE_MASK      0x1
 
 #define TTC_CLK_CNTRL_CSRC_MASK         (1 << 5)        /* clock source */
-#define TTC_CLK_CNTRL_PSV_MASK          0x1e
+#define TTC_CLK_CNTRL_PSV_MASK		0x1e
 #define TTC_CLK_CNTRL_PSV_SHIFT         1
 
 /*
@@ -33,3 +34,22 @@
 
 #define MAX_F_ERR 50
 
+#define TTC_PWM_CHANNEL         0x4
+
+#define TTC_CLK_CNTRL_CSRC              BIT(5)
+#define TTC_CLK_CNTRL_PS_EN             BIT(0)
+#define TTC_CNTR_CTRL_DIS               BIT(0)
+#define TTC_CNTR_CTRL_INTR_MODE_EN      BIT(1)
+#define TTC_CNTR_CTRL_MATCH_MODE_EN     BIT(3)
+#define TTC_CNTR_CTRL_RST               BIT(4)
+#define TTC_CNTR_CTRL_WAVE_EN   BIT(5)
+#define TTC_CNTR_CTRL_WAVE_POL  BIT(6)
+#define TTC_CNTR_CTRL_WAVE_POL_SHIFT    6
+#define TTC_CNTR_CTRL_PRESCALE_SHIFT    1
+#define TTC_PWM_MAX_CH	3
+
+#if defined(CONFIG_PWM_CADENCE)
+int ttc_pwm_probe(struct platform_device *pdev);
+void ttc_pwm_remove(struct platform_device *pdev);
+#endif
+
-- 
2.25.1

Re: [PATCH v4 3/3] pwm: pwm-cadence: Add support for TTC PWM

[Uwe Kleine-König]

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

Hello,

On Wed, Jan 15, 2025 at 05:05:56PM +0530, Mubin Sayyed wrote:
> Cadence TTC timer can be configured as clocksource/clockevent or PWM
> device.Specific TTC device would be configured as PWM device, if
> pwm-cells property is present in the device tree node.
> 
> In case of Zynq, ZynqMP and Versal SoC's, each TTC device has 3
> timers/counters, so maximum 3 PWM channels can be configured for each TTC
> IP instance. Also, output of 0th PWM channel of each TTC device can be
> routed to MIO or EMIO, and output of 2nd and 3rd PWM channel can be
> routed only to EMIO.
> 
> Period for given PWM channel is configured through interval timer and
> duty cycle through match counter.
> 
> Details for cadence TTC IP can be found in Zynq UltraScale+ TRM.
> 
> Signed-off-by: Mubin Sayyed <mubin.sayyed@amd.com>
> ---
> Refer link given below for Zynq UltraScale+ TRM
> https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm

I would prefer to have a link to the TRM in the source file. When I
follow that link today however I only get "The document you are looking
for has been moved or deleted" :-\

> Changes for v4:
>  Configure it as part of TTC clocksource/clockevent driver
>  drivers/clocksource/timer-cadence-ttc.c.
>  Move probe/remove function to timer-cadence-ttc.c.
> Changes for v3:
>  None
> Changes for v2:
>  Use maybe_unused attribute for ttc_pwm_of_match_driver structure
>  Add new function ttc_pwm_set_polarity
>  Removed calls to pwm_get_state
>  Replace DIV_ROUNF_CLOSEST with mul_u64_u64_div_u64
>  Modify ttc_pwm_apply to remove while loop in prescalar logic
>  and avoid glitch
>  Calculate rate in probe and add it to private structure for further
>  Drop ttc_pwm_of_xlate
>  Replace of_clk_get with devm_clk_get_enabled
>  Drop _OFFSET and _MASK from definitions
>  Keep Kconfig and Makefile changes alphabetically sorted
>  Use remove_new instead of remove
>  Document limitations in driver file
> ---
>  drivers/pwm/Kconfig               |  10 +
>  drivers/pwm/Makefile              |   1 +
>  drivers/pwm/pwm-cadence.c         | 323 ++++++++++++++++++++++++++++++
>  include/linux/timer-cadence-ttc.h |  22 +-
>  4 files changed, 355 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pwm/pwm-cadence.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0915c1e7df16..b418e5d8fa42 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -202,6 +202,16 @@ config PWM_CROS_EC
>  	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
>  	  Controller.
>  
> +config PWM_CADENCE
> +	bool "Cadence TTC PWM driver"

tristate please

> +	depends on CADENCE_TTC_TIMER
> +	help
> +	  Generic PWM framework driver for cadence TTC IP found on
> +          Xilinx Zynq/ZynqMP/Versal SOCs. Each TTC device has 3 PWM
> +          channels. Output of 0th PWM channel of each TTC device can
> +          be routed to MIO or EMIO, and output of 1st and 2nd PWM
> +          channels can be routed only to EMIO.
> +
>  config PWM_DWC_CORE
>  	tristate
>  	depends on HAS_IOMEM
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9081e0c0e9e0..246380391a63 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_PWM_BCM_KONA)	+= pwm-bcm-kona.o
>  obj-$(CONFIG_PWM_BCM2835)	+= pwm-bcm2835.o
>  obj-$(CONFIG_PWM_BERLIN)	+= pwm-berlin.o
>  obj-$(CONFIG_PWM_BRCMSTB)	+= pwm-brcmstb.o
> +obj-$(CONFIG_PWM_CADENCE)	+= pwm-cadence.o
>  obj-$(CONFIG_PWM_CLK)		+= pwm-clk.o
>  obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
>  obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
> diff --git a/drivers/pwm/pwm-cadence.c b/drivers/pwm/pwm-cadence.c
> new file mode 100644
> index 000000000000..e7c337fe956b
> --- /dev/null
> +++ b/drivers/pwm/pwm-cadence.c
> @@ -0,0 +1,323 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver to configure cadence TTC timer as PWM
> + * generator
> + *
> + * Limitations:
> + * - When PWM is stopped, timer counter gets stopped immediately. This
> + *   doesn't allow the current PWM period to complete and stops abruptly.
> + * - Disabled PWM emits inactive level.
> + * - When user requests a change in  any parameter of PWM (period/duty cycle/polarity)

s/  / /

> + *   while PWM is in enabled state:
> + *	- PWM is stopped abruptly.
> + *	- Requested parameter is changed.
> + *	- Fresh PWM cycle is started.
> + *
> + * Copyright (C) 2025, Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>

I didn't check, but <linux/device.h> is unusual. Do you really need it?

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/of_address.h>

Also I think this can be dropped.

> +#include <linux/timer-cadence-ttc.h>
> +
> +/**
> + * struct ttc_pwm_priv - Private data for TTC PWM drivers
> + * @chip:	PWM chip structure representing PWM controller
> + * @clk:	TTC input clock
> + * @rate:	TTC input clock rate
> + * @max:	Maximum value of the counters
> + * @base:	Base address of TTC instance
> + */
> +struct ttc_pwm_priv {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	unsigned long rate;
> +	u32 max;
> +	void __iomem *base;
> +};
> +
> +static inline u32 ttc_pwm_readl(struct ttc_pwm_priv *priv,
> +				unsigned long offset)
> +{
> +	return readl_relaxed(priv->base + offset);
> +}
> +
> +static inline void ttc_pwm_writel(struct ttc_pwm_priv *priv,
> +				  unsigned long offset,
> +				  unsigned long val)
> +{
> +	writel_relaxed(val, priv->base + offset);
> +}
> +
> +static inline u32 ttc_pwm_ch_readl(struct ttc_pwm_priv *priv,
> +				   unsigned int chnum,
> +				   unsigned long offset)
> +{
> +	unsigned long pwm_ch_offset = offset +
> +				       (TTC_PWM_CHANNEL * chnum);
> +
> +	return ttc_pwm_readl(priv, pwm_ch_offset);
> +}
> +
> +static inline void ttc_pwm_ch_writel(struct ttc_pwm_priv *priv,
> +				     unsigned int chnum,
> +				     unsigned long offset,
> +				     unsigned long val)
> +{
> +	unsigned long pwm_ch_offset = offset +
> +				       (TTC_PWM_CHANNEL * chnum);
> +
> +	ttc_pwm_writel(priv, pwm_ch_offset, val);
> +}
> +
> +static inline struct ttc_pwm_priv *xilinx_pwm_chip_to_priv(struct pwm_chip *chip)

Can you please stick to a single unique function prefix? The involved
prefixes here are ttc_pwm, xilinx_pwm and the driver is called
"cadence". Unifying them all to a single name would be good.

> +{
> +	return pwmchip_get_drvdata(chip);
> +}
> +
> +static void ttc_pwm_enable(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
> +{
> +	u32 ctrl_reg;
> +
> +	ctrl_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET);
> +	ctrl_reg |= (TTC_CNTR_CTRL_INTR_MODE_EN
> +				 | TTC_CNTR_CTRL_MATCH_MODE_EN | TTC_CNTR_CTRL_RST);
> +	ctrl_reg &= ~(TTC_CNTR_CTRL_DIS | TTC_CNTR_CTRL_WAVE_EN);
> +	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
> +}
> +
> +static void ttc_pwm_disable(struct ttc_pwm_priv *priv, struct pwm_device *pwm)

This function only needs .hwpwm from *pwm. Maybe pass hwpwm as parameter
instead of pwm?

> +{
> +	u32 ctrl_reg;
> +
> +	ctrl_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET);
> +	ctrl_reg |= TTC_CNTR_CTRL_DIS;
> +
> +	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
> +}
> +
> +static void ttc_pwm_set_polarity(struct ttc_pwm_priv *priv, struct pwm_device *pwm,
> +				 enum pwm_polarity polarity)
> +{
> +	u32 ctrl_reg;
> +
> +	ctrl_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET);
> +
> +	if (polarity == PWM_POLARITY_NORMAL)
> +		ctrl_reg |= TTC_CNTR_CTRL_WAVE_POL;
> +	else
> +		ctrl_reg &= (~TTC_CNTR_CTRL_WAVE_POL);

The parenthesis can be dropped.

> +
> +	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
> +}
> +
> +static void ttc_pwm_set_counters(struct ttc_pwm_priv *priv,
> +				 struct pwm_device *pwm,
> +				 u32 period_cycles,
> +				 u32 duty_cycles)
> +{
> +	/* Set up period */
> +	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_INTR_VAL_OFFSET, period_cycles);
> +
> +	/* Set up duty cycle */
> +	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_MATCH_CNT_VAL_OFFSET, duty_cycles);
> +}
> +
> +static void ttc_pwm_set_prescalar(struct ttc_pwm_priv *priv,
> +				  struct pwm_device *pwm,
> +				  u32 div, bool is_enable)
> +{
> +	u32 clk_reg;
> +
> +	if (is_enable) {
> +		/* Set up prescalar */
> +		clk_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET);
> +		clk_reg &= ~TTC_CLK_CNTRL_PSV_MASK;
> +		clk_reg |= (div << TTC_CNTR_CTRL_PRESCALE_SHIFT);
> +		clk_reg |= TTC_CLK_CNTRL_PS_EN;
> +		ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET, clk_reg);
> +	} else {
> +		/* Disable prescalar */
> +		clk_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET);
> +		clk_reg &= ~TTC_CLK_CNTRL_PS_EN;
> +		ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET, clk_reg);
> +	}
> +}
> +
> +static int ttc_pwm_apply(struct pwm_chip *chip,
> +			 struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +	u64 duty_cycles, period_cycles;
> +	struct pwm_state cstate;
> +	unsigned long rate;
> +	bool flag = false;
> +	u32 div = 0;
> +
> +	cstate = pwm->state;

A pointer would be enough here. No need to copy the whole struct.

> +	if (state->polarity != cstate.polarity) {
> +		if (cstate.enabled)
> +			ttc_pwm_disable(priv, pwm);
> +
> +		ttc_pwm_set_polarity(priv, pwm, state->polarity);
> +	}
> +
> +	rate = priv->rate;
> +
> +	/* Prevent overflow by limiting to the maximum possible period */
> +	period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);

ULONG_MAX * NSEC_PER_SEC overflows before it's casted to u64.

> +	period_cycles = mul_u64_u64_div_u64(period_cycles, rate, NSEC_PER_SEC);

mul_u64_u64_div_u64() doesn't overflow if rate <= NSEC_PER_SEC.

> +	if (period_cycles > priv->max) {
> +		/*
> +		 * Prescale frequency to fit requested period cycles within limit.
> +		 * Prescalar divides input clock by 2^(prescale_value + 1). Maximum
> +		 * supported prescalar value is 15.
> +		 */
> +		div = mul_u64_u64_div_u64(state->period, rate, (NSEC_PER_SEC * priv->max));

No parenthesis needed around the 3rd parameter. Can NSEC_PER_SEC *
priv->max overflow? Is it intended that you use state->period here and
don't cap the value first as you did above?

> +		div = order_base_2(div);
> +		if (div)
> +			div -= 1;
> +
> +		if (div > 15)
> +			return -ERANGE;
> +
> +		rate = DIV_ROUND_CLOSEST(rate, BIT(div + 1));
> +		period_cycles = mul_u64_u64_div_u64(state->period, rate,
> +						    NSEC_PER_SEC);

Dividing twice decreases precision. Also rounding to closest looks
wrong. I think this should just be:

	period_cycles = mul_u64_u64_div_u64(state->period, rate, NSEC_PER_SEC * BIT(div + 1));

Wouldn't it be simpler to just use:

	dif = order_base_2(period_cycles / priv->max);

(modulo correctness)?

> +		flag = true;
> +	}
> +
> +	if (cstate.enabled)
> +		ttc_pwm_disable(priv, pwm);
> +
> +	duty_cycles = mul_u64_u64_div_u64(state->duty_cycle, rate,
> +					  NSEC_PER_SEC);
> +	ttc_pwm_set_counters(priv, pwm, period_cycles, duty_cycles);
> +
> +	ttc_pwm_set_prescalar(priv, pwm, div, flag);
> +
> +	if (state->enabled)
> +		ttc_pwm_enable(priv, pwm);
> +	else
> +		ttc_pwm_disable(priv, pwm);

The hardware is already disabled, so I'd expect that this
ttc_pwm_disable() can be dropped?

> +	return 0;
> +}
> +
> +static int ttc_pwm_get_state(struct pwm_chip *chip,
> +			     struct pwm_device *pwm,
> +			     struct pwm_state *state)
> +{
> +	struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +	u32 value, pres_en, pres = 1;
> +	unsigned long rate;
> +	u64 tmp;
> +
> +	value = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET);
> +
> +	if (value & TTC_CNTR_CTRL_WAVE_POL)
> +		state->polarity = PWM_POLARITY_NORMAL;
> +	else
> +		state->polarity = PWM_POLARITY_INVERSED;
> +
> +	if (value & TTC_CNTR_CTRL_DIS)
> +		state->enabled = false;

You can exit early here.

> +	else
> +		state->enabled = true;
> +
> +	rate = priv->rate;
> +
> +	pres_en = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET);
> +	pres_en	&= TTC_CLK_CNTRL_PS_EN;
> +
> +	if (pres_en) {
> +		pres = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET)
> +					& TTC_CLK_CNTRL_PSV_MASK;
> +		pres >>= TTC_CNTR_CTRL_PRESCALE_SHIFT;

		pres = FIELD_GET(...)

> +		/* If prescale is enabled, the count rate is divided by 2^(pres + 1) */
> +		pres = BIT(pres + 1);
> +	}
> +
> +	tmp = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_INTR_VAL_OFFSET);
> +	tmp *= pres;

you can drop `pres = BIT(pres + 1);` above if you use

	tmp <<= pres + 1

here.

> +	state->period = DIV64_U64_ROUND_UP(tmp * NSEC_PER_SEC, rate);

Can this overflow?

> +	tmp = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_MATCH_CNT_VAL_OFFSET);
> +	tmp *= pres;
> +	state->duty_cycle = DIV64_U64_ROUND_UP(tmp * NSEC_PER_SEC, rate);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops ttc_pwm_ops = {
> +	.apply = ttc_pwm_apply,
> +	.get_state = ttc_pwm_get_state,
> +};
> +
> +int ttc_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct ttc_pwm_priv *priv;
> +	struct pwm_chip *chip;
> +	u32 timer_width;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "timer-width", &timer_width);
> +	if (ret)
> +		timer_width = 16;
> +
> +	chip = devm_pwmchip_alloc(dev, TTC_PWM_MAX_CH, sizeof(*priv));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	priv = xilinx_pwm_chip_to_priv(chip);
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->max = BIT(timer_width) - 1;
> +
> +	priv->clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> +				     "ERROR: timer input clock not found\n");
> +	}
> +
> +	priv->rate = clk_get_rate(priv->clk);
> +
> +	clk_rate_exclusive_get(priv->clk);

Only call clk_get_rate() after clk_rate_exclusive_get(). Also note there
is a devm variant of clk_rate_exclusive_get().

> +	chip->ops = &ttc_pwm_ops;
> +	chip->npwm = TTC_PWM_MAX_CH;
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret) {
> +		clk_rate_exclusive_put(priv->clk);
> +		return dev_err_probe(dev, ret, "Could not register PWM chip\n");
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(ttc_pwm_probe);

Putting these functions in a name space would be great.

> +void ttc_pwm_remove(struct platform_device *pdev)
> +{
> +	struct ttc_pwm_priv *priv = platform_get_drvdata(pdev);
> +
> +	pwmchip_remove(&priv->chip);
> +	clk_rate_exclusive_put(priv->clk);
> +}

Did you test the remove path? Hint: Don't call pwmchip_remove() if you
registered the chip using devm_pwmchip_add() and priv->chip is
uninitialized.

> +EXPORT_SYMBOL_GPL(ttc_pwm_remove);
> +
> +MODULE_AUTHOR("Mubin Sayyed <mubin.sayyed@amd.com>");
> +MODULE_DESCRIPTION("Cadence TTC PWM driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/timer-cadence-ttc.h b/include/linux/timer-cadence-ttc.h
> index d938991371e5..6b6135d0ba0c 100644
> --- a/include/linux/timer-cadence-ttc.h
> +++ b/include/linux/timer-cadence-ttc.h
> @@ -12,13 +12,14 @@
>  #define TTC_CNT_CNTRL_OFFSET            0x0C /* Counter Control Reg, RW */
>  #define TTC_COUNT_VAL_OFFSET            0x18 /* Counter Value Reg, RO */
>  #define TTC_INTR_VAL_OFFSET             0x24 /* Interval Count Reg, RW */
> +#define TTC_MATCH_CNT_VAL_OFFSET        0x30 /* Match Count Reg, RW */

I assume all these constants are register addresses. I'd drop _OFFSET
here. If you want a designator for these, I'd suggest REG or ADDR, but
IMHO plain TTC_MATCH_CNT_VAL is fine.

>  #define TTC_ISR_OFFSET          0x54 /* Interrupt Status Reg, RO */
>  #define TTC_IER_OFFSET          0x60 /* Interrupt Enable Reg, RW */
>  
>  #define TTC_CNT_CNTRL_DISABLE_MASK      0x1
>  
>  #define TTC_CLK_CNTRL_CSRC_MASK         (1 << 5)        /* clock source */
> -#define TTC_CLK_CNTRL_PSV_MASK          0x1e
> +#define TTC_CLK_CNTRL_PSV_MASK		0x1e
>  #define TTC_CLK_CNTRL_PSV_SHIFT         1
>  
>  /*
> @@ -33,3 +34,22 @@
>  
>  #define MAX_F_ERR 50
>  
> +#define TTC_PWM_CHANNEL         0x4

That define is misnamed IMHO. That's the offset between register ranges
for different channels. *Here* _OFFSET is fine.

> +
> +#define TTC_CLK_CNTRL_CSRC              BIT(5)
> +#define TTC_CLK_CNTRL_PS_EN             BIT(0)
> +#define TTC_CNTR_CTRL_DIS               BIT(0)
> +#define TTC_CNTR_CTRL_INTR_MODE_EN      BIT(1)
> +#define TTC_CNTR_CTRL_MATCH_MODE_EN     BIT(3)
> +#define TTC_CNTR_CTRL_RST               BIT(4)
> +#define TTC_CNTR_CTRL_WAVE_EN   BIT(5)
> +#define TTC_CNTR_CTRL_WAVE_POL  BIT(6)
> +#define TTC_CNTR_CTRL_WAVE_POL_SHIFT    6
> +#define TTC_CNTR_CTRL_PRESCALE_SHIFT    1
> +#define TTC_PWM_MAX_CH	3
> +
> +#if defined(CONFIG_PWM_CADENCE)
> +int ttc_pwm_probe(struct platform_device *pdev);
> +void ttc_pwm_remove(struct platform_device *pdev);
> +#endif

No need for the ifdef.

Best regards
Uwe

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

Re: [PATCH v4 2/3] clocksource: timer-cadence-ttc: Support TTC device configured as PWM

[Uwe Kleine-König]

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

On Wed, Jan 15, 2025 at 05:05:55PM +0530, Mubin Sayyed wrote:
> TTC device can act either as clocksource/clockevent or PWM generator,
> it would be decided by pwm-cells property. If pwm-cells property is
> present in TTC node, it would be treated as PWM device, and clocksource
> driver just calls probe function for PWM functionality, so that TTC
> device would be registered with PWM framework.
> 
> Signed-off-by: Mubin Sayyed <mubin.sayyed@amd.com>
> ---
> Changes for v4:
>     - In case of pwm-cells property call probe function for PWM
>       feature instead of returning error.
> Changes for v3:
>     - None
> Changes for v2:
>     - Added comment regarding pwm-cells property
> ---
>  drivers/clocksource/timer-cadence-ttc.c | 34 +++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
> index 2f33d4c40153..c5ecad9332c9 100644
> --- a/drivers/clocksource/timer-cadence-ttc.c
> +++ b/drivers/clocksource/timer-cadence-ttc.c
> @@ -35,6 +35,10 @@
>   * obtained from device tree. The pre-scaler of 32 is used.
>   */
>  
> +struct ttc_timer_config {
> +	bool is_pwm_mode;
> +};
> +
>  /**
>   * struct ttc_timer - This definition defines local timer structure
>   *
> @@ -453,6 +457,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
>  
>  static int __init ttc_timer_probe(struct platform_device *pdev)
>  {
> +	struct ttc_timer_config *ttc_config;
>  	unsigned int irq;
>  	void __iomem *timer_baseaddr;
>  	struct clk *clk_cs, *clk_ce;
> @@ -461,6 +466,24 @@ static int __init ttc_timer_probe(struct platform_device *pdev)
>  	u32 timer_width = 16;
>  	struct device_node *timer = pdev->dev.of_node;
>  
> +	ttc_config = devm_kzalloc(&pdev->dev, sizeof(*ttc_config), GFP_KERNEL);
> +	if (!ttc_config)
> +		return -ENOMEM;
> +
> +	/*
> +	 * If pwm-cells property is present in TTC node,
> +	 * it would be treated as PWM device.
> +	 */
> +	if (of_property_read_bool(timer, "#pwm-cells")) {
> +		#if defined(CONFIG_PWM_CADENCE)
> +		ttc_config->is_pwm_mode = true;
> +			return ttc_pwm_probe(pdev);

strange indention. Maybe use

	if (IS_REACHABLE(CONFIG_PWM_CADENCE))

This is an unusal way to bind the PWM driver. I'd prefer creation of separate
device in the PWM case. I wonder if it can happen that ttc_pwm_probe() is
called during boot before pwm_init() completed. Or use an auxbus device
to distinguish between timer and pwm?

> +		#else
> +			return -ENODEV;
> +		#endif
> +	}
> +	dev_set_drvdata(&pdev->dev, ttc_config);
> +
>  	if (initialized)
>  		return 0;
>  

Best regards
Uwe

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

RE: [PATCH v4 2/3] clocksource: timer-cadence-ttc: Support TTC device configured as PWM

[Rao, Appana Durga Kedareswara]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Uwe,

<Snip>
> > +   /*
> > +    * If pwm-cells property is present in TTC node,
> > +    * it would be treated as PWM device.
> > +    */
> > +   if (of_property_read_bool(timer, "#pwm-cells")) {
> > +           #if defined(CONFIG_PWM_CADENCE)
> > +           ttc_config->is_pwm_mode = true;
> > +                   return ttc_pwm_probe(pdev);
>
> strange indention. Maybe use
>
>       if (IS_REACHABLE(CONFIG_PWM_CADENCE))
>
> This is an unusal way to bind the PWM driver. I'd prefer creation of separate device
> in the PWM case. I wonder if it can happen that ttc_pwm_probe() is called during
> boot before pwm_init() completed. Or use an auxbus device to distinguish between
> timer and pwm?
>

Sorry for the delay in response the upstreaming of the TTC PWM driver was previously managed by Mubin. Since his departure from the organization, I will be taking over and continuing the upstreaming effort.

I have reviewed the v1 to v4 patch series along with all the associated feedback. In response to the concerns raised regarding the use of auxbus, I would like to propose an alternative solution: registering the PWM functionality as a separate platform driver using a distinct compatible string. This would allow the PWM driver to be probed only when the specific compatible string is present, while the timer driver would be used otherwise.

If this approach is agreeable, I will proceed with preparing the v5 patch accordingly otherwise will explore the auxbus device please let me know your thoughts.

Regards,
Kedar.

> > +           #else
> > +                   return -ENODEV;
> > +           #endif
> > +   }
> > +   dev_set_drvdata(&pdev->dev, ttc_config);
> > +
> >     if (initialized)
> >             return 0;
> >
>
> Best regards
> Uwe

RE: [PATCH v4 2/3] clocksource: timer-cadence-ttc: Support TTC device configured as PWM

[Rao, Appana Durga Kedareswara]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Uwe,

<Snip>
> > +   /*
> > +    * If pwm-cells property is present in TTC node,
> > +    * it would be treated as PWM device.
> > +    */
> > +   if (of_property_read_bool(timer, "#pwm-cells")) {
> > +           #if defined(CONFIG_PWM_CADENCE)
> > +           ttc_config->is_pwm_mode = true;
> > +                   return ttc_pwm_probe(pdev);
>
> strange indention. Maybe use
>
>       if (IS_REACHABLE(CONFIG_PWM_CADENCE))
>
> This is an unusal way to bind the PWM driver. I'd prefer creation of separate device
> in the PWM case. I wonder if it can happen that ttc_pwm_probe() is called during
> boot before pwm_init() completed. Or use an auxbus device to distinguish between
> timer and pwm?
>

Sorry for the delay in response the upstreaming of the TTC PWM driver was previously managed by Mubin. Since his departure from the organization, I will be taking over and continuing the upstreaming effort.

I have reviewed the v1 to v4 patch series along with all the associated feedback. In response to the concerns raised regarding the use of auxbus, I would like to propose an alternative solution: registering the PWM functionality as a separate platform driver using a distinct compatible string. This would allow the PWM driver to be probed only when the specific compatible string is present, while the timer driver would be used otherwise.

If this approach is agreeable, I will proceed with preparing the v5 patch accordingly(after fixing other review comments along with this change) otherwise will explore the auxbus device please let me know your thoughts.

Regards,
Kedar.
> > +           #else
> > +                   return -ENODEV;
> > +           #endif
> > +   }
> > +   dev_set_drvdata(&pdev->dev, ttc_config);
> > +
> >     if (initialized)
> >             return 0;
> >
>
> Best regards
> Uwe

RE: [PATCH v4 2/3] clocksource: timer-cadence-ttc: Support TTC device configured as PWM

[Rao, Appana Durga Kedareswara]

Ping!

<Snip>
> 
> Hi Uwe,
> 
> <Snip>
> > > +   /*
> > > +    * If pwm-cells property is present in TTC node,
> > > +    * it would be treated as PWM device.
> > > +    */
> > > +   if (of_property_read_bool(timer, "#pwm-cells")) {
> > > +           #if defined(CONFIG_PWM_CADENCE)
> > > +           ttc_config->is_pwm_mode = true;
> > > +                   return ttc_pwm_probe(pdev);
> >
> > strange indention. Maybe use
> >
> >       if (IS_REACHABLE(CONFIG_PWM_CADENCE))
> >
> > This is an unusal way to bind the PWM driver. I'd prefer creation of
> > separate device in the PWM case. I wonder if it can happen that
> > ttc_pwm_probe() is called during boot before pwm_init() completed. Or
> > use an auxbus device to distinguish between timer and pwm?
> >
> 
> Sorry for the delay in response the upstreaming of the TTC PWM driver was
> previously managed by Mubin. Since his departure from the organization, I
> will be taking over and continuing the upstreaming effort.
> 
> I have reviewed the v1 to v4 patch series along with all the associated
> feedback. In response to the concerns raised regarding the use of auxbus, I
> would like to propose an alternative solution: registering the PWM
> functionality as a separate platform driver using a distinct compatible string.
> This would allow the PWM driver to be probed only when the specific
> compatible string is present, while the timer driver would be used otherwise.
> 
> If this approach is agreeable, I will proceed with preparing the v5 patch
> accordingly(after fixing other review comments along with this change)
> otherwise will explore the auxbus device please let me know your thoughts.
> 
> Regards,
> Kedar.
> > > +           #else
> > > +                   return -ENODEV;
> > > +           #endif
> > > +   }
> > > +   dev_set_drvdata(&pdev->dev, ttc_config);
> > > +
> > >     if (initialized)
> > >             return 0;
> > >
> >
> > Best regards
> > Uwe

Re: [PATCH v4 2/3] clocksource: timer-cadence-ttc: Support TTC device configured as PWM

[Krzysztof Kozlowski]

On 09/07/2025 08:51, Rao, Appana Durga Kedareswara wrote:
> Ping!

You responded after three months and now you keep pinging on that? No,
implement entire feedback and then start working on recent Linux kernel.

Best regards,
Krzysztof