pwm: tegra: dynamic clk freq configuration by PWM driver

-----Original Message-----
From: Sandipan Patra <[email protected]>
Sent: Monday, April 20, 2020 9:24 PM
To: Thierry Reding <[email protected]>; [email protected]; u.kleine- [email protected]; Jonathan Hunter <[email protected]>
Cc: Bibek Basu <[email protected]>; Laxman Dewangan <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]; Sandipan Patra <[email protected]>
Subject: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM driver

Added support for dynamic clock freq configuration in pwm kernel driver. Earlier the pwm driver used to cache boot time clock rate by pwm clock parent during probe. Hence dynamically changing pwm frequency was not possible for all the possible ranges. With this change, dynamic calculation is enabled and it is able to set the requested period from sysfs knob provided the value is supported by clock source.

Changes mainly have 2 parts:
- T186 and later chips [1]
- T210 and prior chips [2]

For [1] - Changes implemented to set pwm period dynamically and
also checks added to allow only if requested period(ns) is below or equals to higher range.

For [2] - Only checks if the requested period(ns) is below or equals
to higher range defined by max clock limit. The limitation in T210 or prior chips are due to the reason of having only one pwm-controller supporting multiple channels. But later chips have multiple pwm controller instances each having single channel support.

Signed-off-by Sandipan Patra <[email protected]>

---
V2:
1. Min period_ns calculation is moved to probe.
2. Added descriptioins for PWM register bits and regarding behaviour of the controller when new configuration is applied or pwm is disabled.
3. Setting period with possible value when supplied period is below limit.
4. Corrected the earlier code comment:
plus 1 instead of minus 1 during pwm calculation

 drivers/pwm/pwm-tegra.c | 110
+++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 94 insertions(+), 16 deletions(-)

drivers/pwm/pwm-tegra.c index renamed from drivers/pwm/pwm-tegra.c +0 -0

d26ed8f..7a36325 100644

drivers/pwm/pwm-tegra.c +0 -2

@@ -4,8 +4,39 @@
  *
  * Tegra pulse-width-modulation controller driver
  *
- * Copyright (c) 2010, NVIDIA Corporation.
- * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer

<[email protected]>
+ * Copyright (c) 2010-2020, NVIDIA Corporation.
+ *
+ * Overview of Tegra Pulse Width Modulator Register:
+ * 1. 13-bit: Frequency division (SCALE)
+ * 2. 8-bit : Puls division (DUTY)
+ * 3. 1-bit : Enable bit
+ *
+ * The PWM clock frequency is divided by 256 before subdividing it
+ based
+ * on the programmable frequency division value to generate the
+ required
+ * frequency for PWM output. The maximum output frequency that can be
+ * achieved is (max rate of source clock) / 256.
+ * i.e. if source clock rate is 408 MHz, maximum output frequency cab be:
+ * 408 MHz/256 = 1.6 MHz.
+ * This 1.6 MHz frequency can further be divided using SCALE value in PWM.
+ *
+ * PWM pulse width: 8 bits are usable [23:16] for varying pulse width.
+ * To achieve 100% duty cycle, program Bit [24] of this register to
+ * 1’b1. In which case the other bits [23:16] are set to don't care.
+ *
+ * Limitations and known facts:
+ * - When PWM is disabled, the output is driven to 0.
+ * - It does not allow the current PWM period to complete and
+ * stops abruptly.
+ *
+ * - If the register is reconfigured while pwm is running,
+ * It does not let the currently running period to complete.
+ *
+ * - Pulse width of the pwm can never be out of bound.
+ * It's taken care at HW and SW
+ * - If the user input duty is below limit, then driver sets it to
+ * minimum possible value.
+ * - If anything else goes wrong for setting duty or period,
+ * -EINVAL is returned.
*/

#include <linux/clk.h>

drivers/pwm/pwm-tegra.c +14 -5

@@ -41,6 +72,7 @@ struct tegra_pwm_chip {
 	struct reset_control*rst;
 
 	unsigned long clk_rate;
+	unsigned long min_period_ns;
 
 	void __iomem *regs;
 

@@ -67,8 +99,9 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct
 pwm_device *pwm,
 			    int duty_ns, int period_ns)
 {
 	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
-	unsigned long long c = duty_ns, hz;
-	unsigned long rate;
+	unsigned long long p_width = duty_ns, period_hz;
+	unsigned long rate, required_clk_rate;
+	unsigned long pfm; /* Frequency divider */
 	u32 val = 0;
 	int err;

@@ -77,37 +110,77 @@ static int tegra_pwm_config(struct pwm_chip *chip,
 struct pwm_device *pwm,
 	 * per (1 << PWM_DUTY_WIDTH) cycles and make sure to round to the
 	 * nearest integer during division.
 	 */
-	c *= (1 << PWM_DUTY_WIDTH);
-	c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
+	p_width *= (1 << PWM_DUTY_WIDTH);
+	p_width = DIV_ROUND_CLOSEST_ULL(p_width, period_ns);
 
-	val = (u32)c << PWM_DUTY_SHIFT;
+	val = (u32)p_width << PWM_DUTY_SHIFT;
+
+	/*
+	 *  Period in nano second has to be <= highest allowed period
+	 *  based on max clock rate of the pwm controller.
+	 *
+	 *  higher limit = max clock limit >> PWM_DUTY_WIDTH
+	 *  lower limit = min clock limit >> PWM_DUTY_WIDTH >>

PWM_SCALE_WIDTH

drivers/pwm/pwm-tegra.c +9 -1

+	 */
+	if (period_ns < pc->min_period_ns) {
+		period_ns = pc->min_period_ns;
+		pr_warn("Period is adjusted to allowed value (%d ns)\n",
+				period_ns);
+	}
 
 	/*
 	 * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
 	 * cycles at the PWM clock rate will take period_ns nanoseconds.
 	 */
-	rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+	if (pc->soc->num_channels == 1) {
+		/*
+		 * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it

matches

drivers/pwm/pwm-tegra.c +26 -2

+		 * with the hieghest applicable rate that the controller can
+		 * provide. Any further lower value can be derived by setting
+		 * PFM bits[0:12].
+		 * Higher mark is taken since BPMP has round-up mechanism
+		 * implemented.
+		 */
+		required_clk_rate =
+			(NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
+
+		err = clk_set_rate(pc->clk, required_clk_rate);
+		if (err < 0)
+			return -EINVAL;
+
+		rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
+	} else {
+		/*
+		 * This is the case for SoCs who support multiple channels:
+		 *
+		 * clk_set_rate() can not be called again in config because
+		 * T210 or any prior chip supports one pwm-controller and
+		 * multiple channels. Hence in this case cached clock rate
+		 * will be considered which was stored during probe.
+		 */
+		rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+	}
 
 	/* Consider precision in PWM_SCALE_WIDTH rate calculation */
-	hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
-	rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
+	period_hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC,

period_ns);

drivers/pwm/pwm-tegra.c +7 -6

+	pfm = DIV_ROUND_CLOSEST_ULL(100ULL * rate, period_hz);
 
 	/*
 	 * Since the actual PWM divider is the register's frequency divider
-	 * field minus 1, we need to decrement to get the correct value to
+	 * field plus 1, we need to decrement to get the correct value to
 	 * write to the register.
 	 */
-	if (rate > 0)
-		rate--;
+	if (pfm > 0)
+		pfm--;
 
 	/*
-	 * Make sure that the rate will fit in the register's frequency
+	 * Make sure that pfm will fit in the register's frequency
 	 * divider field.
 	 */
-	if (rate >> PWM_SCALE_WIDTH)
+	if (pfm >> PWM_SCALE_WIDTH)
 		return -EINVAL;
 
-	val |= rate << PWM_SCALE_SHIFT;
+	val |= pfm << PWM_SCALE_SHIFT;
 
 	/*
 	 * If the PWM channel is disabled, make sure to turn on the clock @@ -

205,6 +278,10 @@ static int tegra_pwm_probe(struct platform_device *pdev)

drivers/pwm/pwm-tegra.c +3 -0

 	 */
 	pwm->clk_rate = clk_get_rate(pwm->clk);
 
+	/* Set minimum limit of PWM period for the IP */
+	pwm->min_period_ns =
+	    (NSEC_PER_SEC / (pwm->soc->max_frequency >>

PWM_DUTY_WIDTH)) + 1;

drivers/pwm/pwm-tegra.c +2 -1

+
 	pwm->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
 	if (IS_ERR(pwm->rst)) {
 		ret = PTR_ERR(pwm->rst);

@@ -313,4 +390,5 @@ module_platform_driver(tegra_pwm_driver);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("NVIDIA Corporation");
+MODULE_AUTHOR("Sandipan Patra <[email protected]>");
 MODULE_ALIAS("platform:tegra-pwm");
--

2.7.4

Added support for dynamic clock freq configuration in pwm kernel driver. Earlier the pwm driver used to cache boot time clock rate by pwm clock parent during probe. Hence dynamically changing pwm frequency was not possible for all the possible ranges. With this change, dynamic calculation is enabled and it is able to set the requested period from sysfs knob provided the value is supported by clock source.

Changes mainly have 2 parts:
- T186 and later chips [1]
- T210 and prior chips [2]

For [1] - Changes implemented to set pwm period dynamically and
also checks added to allow only if requested period(ns) is below or equals to higher range.

For [2] - Only checks if the requested period(ns) is below or equals
to higher range defined by max clock limit. The limitation in T210 or prior chips are due to the reason of having only one pwm-controller supporting multiple channels. But later chips have multiple pwm controller instances each having single channel support.

Signed-off-by Sandipan Patra <[email protected]>

---
V2:
1. Min period_ns calculation is moved to probe.
2. Added descriptioins for PWM register bits and regarding behaviour of the controller when new configuration is applied or pwm is disabled.
3. Setting period with possible value when supplied period is below limit.
4. Corrected the earlier code comment:
plus 1 instead of minus 1 during pwm calculation

 drivers/pwm/pwm-tegra.c | 110 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 94 insertions(+), 16 deletions(-)

drivers/pwm/pwm-tegra.c +2 -2

index d26ed8f..7a36325 100644
@@ -4,8 +4,39 @@
  *
  * Tegra pulse-width-modulation controller driver
  *
- * Copyright (c) 2010, NVIDIA Corporation.
- * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer <[email protected]>
+ * Copyright (c) 2010-2020, NVIDIA Corporation.
+ *

drivers/pwm/pwm-tegra.c +9 -5

  */
 
 #include <linux/clk.h>

@@ -41,6 +72,7 @@ struct tegra_pwm_chip {
 	struct reset_control*rst;
 
 	unsigned long clk_rate;
+	unsigned long min_period_ns;
 
 	void __iomem *regs;
 

@@ -67,8 +99,9 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			    int duty_ns, int period_ns)
 {
 	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
-	unsigned long long c = duty_ns, hz;
-	unsigned long rate;
+	unsigned long long p_width = duty_ns, period_hz;
+	unsigned long rate, required_clk_rate;
+	unsigned long pfm; /* Frequency divider */
 	u32 val = 0;
 	int err;
 

@@ -77,37 +110,77 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * per (1 << PWM_DUTY_WIDTH) cycles and make sure to round to the
 	 * nearest integer during division.
 	 */
-	c *= (1 << PWM_DUTY_WIDTH);
-	c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
+	p_width *= (1 << PWM_DUTY_WIDTH);
+	p_width = DIV_ROUND_CLOSEST_ULL(p_width, period_ns);
 
-	val = (u32)c << PWM_DUTY_SHIFT;
+	val = (u32)p_width << PWM_DUTY_SHIFT;
+
+	/*

drivers/pwm/pwm-tegra.c +16 -8

+		 *
+		 * clk_set_rate() can not be called again in config because
+		 * T210 or any prior chip supports one pwm-controller and
+		 * multiple channels. Hence in this case cached clock rate
+		 * will be considered which was stored during probe.
+		 */
+		rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+	}
 
 	/* Consider precision in PWM_SCALE_WIDTH rate calculation */
-	hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
-	rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
+	period_hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
+	pfm = DIV_ROUND_CLOSEST_ULL(100ULL * rate, period_hz);
 
 	/*
 	 * Since the actual PWM divider is the register's frequency divider
-	 * field minus 1, we need to decrement to get the correct value to
+	 * field plus 1, we need to decrement to get the correct value to
 	 * write to the register.
 	 */
-	if (rate > 0)
-		rate--;
+	if (pfm > 0)
+		pfm--;
 
 	/*
-	 * Make sure that the rate will fit in the register's frequency
+	 * Make sure that pfm will fit in the register's frequency
 	 * divider field.
 	 */
-	if (rate >> PWM_SCALE_WIDTH)
+	if (pfm >> PWM_SCALE_WIDTH)
 		return -EINVAL;
 
-	val |= rate << PWM_SCALE_SHIFT;
+	val |= pfm << PWM_SCALE_SHIFT;
 
 	/*
 	 * If the PWM channel is disabled, make sure to turn on the clock

-----Original Message-----
From: Uwe Kleine-König <[email protected]>
Sent: Tuesday, May 5, 2020 1:42 AM
To: Sandipan Patra <[email protected]>
Cc: Thierry Reding <[email protected]>; [email protected]; Jonathan Hunter <[email protected]>; Bibek Basu <[email protected]>; Laxman Dewangan <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM driver

External email: Use caution opening links or attachments

Hello,

Quoted 55 lines · Sandipan Patra

Added support for dynamic clock freq configuration in pwm kernel driver. Earlier the pwm driver used to cache boot time clock rate by pwm clock parent during probe. Hence dynamically changing pwm frequency was not possible for all the possible ranges. With this change, dynamic calculation is enabled and it is able to set the requested period from sysfs knob provided the value is supported by clock source.

Changes mainly have 2 parts:
- T186 and later chips [1]
- T210 and prior chips [2]

For [1] - Changes implemented to set pwm period dynamically and
also checks added to allow only if requested period(ns) is below or equals to higher range.

For [2] - Only checks if the requested period(ns) is below or equals
to higher range defined by max clock limit. The limitation in T210 or prior chips are due to the reason of having only one pwm-controller supporting multiple channels. But later chips have multiple pwm controller instances each having
single channel support.

Signed-off-by Sandipan Patra <[email protected]>

---
V2:
1. Min period_ns calculation is moved to probe.
2. Added descriptioins for PWM register bits and regarding behaviour of the controller when new configuration is applied or pwm is disabled.
3. Setting period with possible value when supplied period is below limit.
4. Corrected the earlier code comment:
plus 1 instead of minus 1 during pwm calculation

 drivers/pwm/pwm-tegra.c | 110
+++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 94 insertions(+), 16 deletions(-)

drivers/pwm/pwm-tegra.c index renamed from drivers/pwm/pwm-tegra.c +0 -0

d26ed8f..7a36325 100644

drivers/pwm/pwm-tegra.c +0 -2

@@ -4,8 +4,39 @@
  *
  * Tegra pulse-width-modulation controller driver
  *
- * Copyright (c) 2010, NVIDIA Corporation.
- * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer

<[email protected]>
+ * Copyright (c) 2010-2020, NVIDIA Corporation.
+ *
+ * Overview of Tegra Pulse Width Modulator Register:
+ * 1. 13-bit: Frequency division (SCALE)
+ * 2. 8-bit : Puls division (DUTY)
+ * 3. 1-bit : Enable bit
+ *
+ * The PWM clock frequency is divided by 256 before subdividing it

Quoted 6 lines

+ based
+ * on the programmable frequency division value to generate the
+ required
+ * frequency for PWM output. The maximum output frequency that can be
+ * achieved is (max rate of source clock) / 256.
+ * i.e. if source clock rate is 408 MHz, maximum output frequency cab be:

s/i.e./e.g./, s/cab/can/

Quoted 9 lines

+ * 408 MHz/256 = 1.6 MHz.
+ * This 1.6 MHz frequency can further be divided using SCALE value in PWM.
+ *
+ * PWM pulse width: 8 bits are usable [23:16] for varying pulse width.
+ * To achieve 100% duty cycle, program Bit [24] of this register to
+ * 1’b1. In which case the other bits [23:16] are set to don't care.
+ *
+ * Limitations and known facts:

Please use "Limitations:" here to make this easier greppable.

Quoted 2 lines

+ * - When PWM is disabled, the output is driven to 0.

0 or inactive?

Quoted 5 lines

+ * - It does not allow the current PWM period to complete and
+ * stops abruptly.
+ *
+ * - If the register is reconfigured while pwm is running,

s/pwm/PWM/

Quoted 2 lines

+ * It does not let the currently running period to complete.

s/It/it/; s/let/complete/; s/ to complete//

Quoted 2 lines

+ *
+ * - Pulse width of the pwm can never be out of bound.

I don't understand that one.

Quoted 4 lines

+ * It's taken care at HW and SW
+ * - If the user input duty is below limit, then driver sets it to
+ * minimum possible value.

that is 0? Do you mean "input period"? If so, better refuse the request.

Quoted 3 lines

+ * - If anything else goes wrong for setting duty or period,
+ * -EINVAL is returned.

I wouldn't state this, too trivial. Instead the following are interesting:

- The driver doesn't implement the right rounding rules
- The driver needs updating to the atomic API

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip,

Quoted 12 lines

*/

#include <linux/clk.h>

drivers/pwm/pwm-tegra.c +1 -0

@@ -41,6 +72,7 @@ struct tegra_pwm_chip {
      struct reset_control*rst;
 
      unsigned long clk_rate;
+     unsigned long min_period_ns;
 
      void __iomem *regs;
 

@@ -67,8 +99,9 @@ static int tegra_pwm_config(struct pwm_chip *chip,

struct pwm_device *pwm,

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip,

Quoted 11 lines

drivers/pwm/pwm-tegra.c +3 -2

                          int duty_ns, int period_ns)  {
      struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
-     unsigned long long c = duty_ns, hz;
-     unsigned long rate;
+     unsigned long long p_width = duty_ns, period_hz;
+     unsigned long rate, required_clk_rate;
+     unsigned long pfm; /* Frequency divider */
      u32 val = 0;
      int err;
 

@@ -77,37 +110,77 @@ static int tegra_pwm_config(struct pwm_chip *chip,

struct pwm_device *pwm,

Quoted 7 lines

drivers/pwm/pwm-tegra.c +2 -2

       * per (1 << PWM_DUTY_WIDTH) cycles and make sure to round to the
       * nearest integer during division.
       */
-     c *= (1 << PWM_DUTY_WIDTH);
-     c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
+     p_width *= (1 << PWM_DUTY_WIDTH);
+     p_width = DIV_ROUND_CLOSEST_ULL(p_width, period_ns);

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 10 lines

drivers/pwm/pwm-tegra.c +8 -1

 
-     val = (u32)c << PWM_DUTY_SHIFT;
+     val = (u32)p_width << PWM_DUTY_SHIFT;
+
+     /*
+      *  Period in nano second has to be <= highest allowed period
+      *  based on max clock rate of the pwm controller.
+      *
+      *  higher limit = max clock limit >> PWM_DUTY_WIDTH
+      *  lower limit = min clock limit >> PWM_DUTY_WIDTH >>

PWM_SCALE_WIDTH

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 128

Quoted 5 lines

drivers/pwm/pwm-tegra.c +5 -0

+      */
+     if (period_ns < pc->min_period_ns) {
+             period_ns = pc->min_period_ns;
+             pr_warn("Period is adjusted to allowed value (%d ns)\n",
+                             period_ns);

That pr_warn is a bad idea as it spams the kernel log when the configuration is changed frequently. Wouldn't it be easier to calculate the frequency that is needed to achieve period_ns and check that against max_frequency?

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 135

Quoted 9 lines

drivers/pwm/pwm-tegra.c +2 -1

+     }
 
      /*
       * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
       * cycles at the PWM clock rate will take period_ns nanoseconds.
       */
-     rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+     if (pc->soc->num_channels == 1) {

required_clk_rate could be defined here, which is better as it narrows its scope.

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 4 lines

drivers/pwm/pwm-tegra.c +4 -0

+             /*
+              * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it matches
+              * with the hieghest applicable rate that the controller
+ can

s/hieghest/highest/

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 142

Quoted 5 lines

drivers/pwm/pwm-tegra.c +4 -0

+              * provide. Any further lower value can be derived by setting
+              * PFM bits[0:12].
+              * Higher mark is taken since BPMP has round-up mechanism
+              * implemented.

I don't understand the part with the round-up mechanism.

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 149

Quoted 8 lines

drivers/pwm/pwm-tegra.c +7 -0

+              */
+             required_clk_rate =
+                     (NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
+
+             err = clk_set_rate(pc->clk, required_clk_rate);
+             if (err < 0)
+                     return -EINVAL;

What happens if clk_set_rate configures a higher rate than requested?

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 154

Quoted 6 lines

drivers/pwm/pwm-tegra.c +5 -0

+
+             rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
+     } else {
+             /*
+              * This is the case for SoCs who support multiple channels:

s/who/that/

Quoted 4 lines

drivers/pwm/pwm-tegra.c +3 -0

+              *
+              * clk_set_rate() can not be called again in config because
+              * T210 or any prior chip supports one pwm-controller and

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 10 lines

drivers/pwm/pwm-tegra.c +6 -2

+              * multiple channels. Hence in this case cached clock rate
+              * will be considered which was stored during probe.
+              */
+             rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+     }
 
      /* Consider precision in PWM_SCALE_WIDTH rate calculation */
-     hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
-     rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
+     period_hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC,

period_ns);

Quoted 28 lines

drivers/pwm/pwm-tegra.c +8 -6

+     pfm = DIV_ROUND_CLOSEST_ULL(100ULL * rate, period_hz);
 
      /*
       * Since the actual PWM divider is the register's frequency divider
-      * field minus 1, we need to decrement to get the correct value to
+      * field plus 1, we need to decrement to get the correct value
+ to
       * write to the register.
       */
-     if (rate > 0)
-             rate--;
+     if (pfm > 0)
+             pfm--;
 
      /*
-      * Make sure that the rate will fit in the register's frequency
+      * Make sure that pfm will fit in the register's frequency
       * divider field.
       */
-     if (rate >> PWM_SCALE_WIDTH)
+     if (pfm >> PWM_SCALE_WIDTH)
              return -EINVAL;
 
-     val |= rate << PWM_SCALE_SHIFT;
+     val |= pfm << PWM_SCALE_SHIFT;
 
      /*
       * If the PWM channel is disabled, make sure to turn on the

Quoted 1 line

clock @@ -205,6 +278,10 @@ static int tegra_pwm_probe(struct

platform_device *pdev)

Quoted 3 lines

       */
      pwm->clk_rate = clk_get_rate(pwm->clk);

drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev)

Quoted 4 lines

drivers/pwm/pwm-tegra.c +4 -0

+     /* Set minimum limit of PWM period for the IP */
+     pwm->min_period_ns =
+         (NSEC_PER_SEC / (pwm->soc->max_frequency >> PWM_DUTY_WIDTH))
+ + 1;

With my suggestion above, you can drop the min_period_ns field.

Quoted 11 lines

+

      pwm->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
      if (IS_ERR(pwm->rst)) {
              ret = PTR_ERR(pwm->rst); @@ -313,4 +390,5 @@

module_platform_driver(tegra_pwm_driver);

MODULE_LICENSE("GPL");
MODULE_AUTHOR("NVIDIA Corporation");
+MODULE_AUTHOR("Sandipan Patra <[email protected]>");
MODULE_ALIAS("platform:tegra-pwm");

Best regards
Uwe

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

-----Original Message-----
From: Sandipan Patra
Sent: Thursday, May 7, 2020 1:40 PM
To: Uwe Kleine-König <[email protected]>
Cc: Thierry Reding <[email protected]>; [email protected]; Jonathan Hunter <[email protected]>; Bibek Basu <[email protected]>; Laxman Dewangan <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: RE: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM driver

Hello,

Quoted 74 lines

-----Original Message-----
From: Uwe Kleine-König <[email protected]>
Sent: Tuesday, May 5, 2020 1:42 AM
To: Sandipan Patra <[email protected]>
Cc: Thierry Reding <[email protected]>; [email protected]; Jonathan Hunter <[email protected]>; Bibek Basu <[email protected]>; Laxman Dewangan <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM driver

External email: Use caution opening links or attachments

Hello,

Quoted 56 lines · Sandipan Patra

Added support for dynamic clock freq configuration in pwm kernel driver. Earlier the pwm driver used to cache boot time clock rate by pwm clock parent during probe. Hence dynamically changing pwm frequency was not possible for all the possible ranges. With this change, dynamic calculation is enabled and it is able to set the requested period from sysfs knob provided the value is supported by clock source.

Changes mainly have 2 parts:
- T186 and later chips [1]
- T210 and prior chips [2]

For [1] - Changes implemented to set pwm period dynamically and
also checks added to allow only if requested period(ns) is below or equals to higher range.

For [2] - Only checks if the requested period(ns) is below or equals
to higher range defined by max clock limit. The limitation in T210 or prior chips are due to the reason of having only one pwm-controller supporting multiple channels. But later chips have multiple pwm controller instances each having
single channel support.

Signed-off-by Sandipan Patra <[email protected]>

---
V2:
1. Min period_ns calculation is moved to probe.
2. Added descriptioins for PWM register bits and regarding behaviour of the controller when new configuration is applied or pwm is disabled.
3. Setting period with possible value when supplied period is below limit.
4. Corrected the earlier code comment:
plus 1 instead of minus 1 during pwm calculation

 drivers/pwm/pwm-tegra.c | 110
+++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 94 insertions(+), 16 deletions(-)

drivers/pwm/pwm-tegra.c index renamed from drivers/pwm/pwm-tegra.c +0 -0

d26ed8f..7a36325 100644

drivers/pwm/pwm-tegra.c +0 -2

@@ -4,8 +4,39 @@
  *
  * Tegra pulse-width-modulation controller driver
  *
- * Copyright (c) 2010, NVIDIA Corporation.
- * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer

<[email protected]>
+ * Copyright (c) 2010-2020, NVIDIA Corporation.
+ *
+ * Overview of Tegra Pulse Width Modulator Register:
+ * 1. 13-bit: Frequency division (SCALE)
+ * 2. 8-bit : Puls division (DUTY)
+ * 3. 1-bit : Enable bit
+ *
+ * The PWM clock frequency is divided by 256 before subdividing it
+ based

Quoted 8 lines

Quoted 6 lines

+ * on the programmable frequency division value to generate the
+ required
+ * frequency for PWM output. The maximum output frequency that can
+ be
+ * achieved is (max rate of source clock) / 256.
+ * i.e. if source clock rate is 408 MHz, maximum output frequency cab be:

s/i.e./e.g./, s/cab/can/

Noted, correction in next patch.

Quoted 11 lines

Quoted 9 lines

+ * 408 MHz/256 = 1.6 MHz.
+ * This 1.6 MHz frequency can further be divided using SCALE value in PWM.
+ *
+ * PWM pulse width: 8 bits are usable [23:16] for varying pulse width.
+ * To achieve 100% duty cycle, program Bit [24] of this register to
+ * 1’b1. In which case the other bits [23:16] are set to don't care.
+ *
+ * Limitations and known facts:

Please use "Limitations:" here to make this easier greppable.

Will update in next patch.

Quoted 4 lines

Quoted 2 lines

+ * - When PWM is disabled, the output is driven to 0.

0 or inactive?

Yes, Inactive. When it is 0, it is disabled.
Will update it to "inactive".

Quoted 7 lines

Quoted 5 lines

+ * - It does not allow the current PWM period to complete and
+ * stops abruptly.
+ *
+ * - If the register is reconfigured while pwm is running,

s/pwm/PWM/

Noted, correction in next patch.

Quoted 5 lines

Quoted 2 lines

+ * It does not let the currently running period to complete.

s/It/it/; s/let/complete/; s/ to complete//

Noted, correction in next patch

Quoted 4 lines

Quoted 2 lines

+ *
+ * - Pulse width of the pwm can never be out of bound.

I don't understand that one.

As I understand:
Pulse width is configured on bits [23:16]. So any misconfiguration or overflow from Software will be restricted by the hardware and only the respective bits will be considered.
Also the explanation is added above during register bit field descriptions. Please advise if that doesn't help.

Quoted 6 lines

Quoted 4 lines

+ * It's taken care at HW and SW
+ * - If the user input duty is below limit, then driver sets it to
+ * minimum possible value.

that is 0? Do you mean "input period"? If so, better refuse the request.

This is for pwm duty. If user requested duty is below lower bound, then pwm driver configures to the min possible duty.
Lower bound and upper bound values are derived based on min and max clock rates respectively.

Quoted 60 lines

Quoted 3 lines

+ * - If anything else goes wrong for setting duty or period,
+ * -EINVAL is returned.

I wouldn't state this, too trivial. Instead the following are interesting:

- The driver doesn't implement the right rounding rules
- The driver needs updating to the atomic API

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip,

Quoted 12 lines

*/

#include <linux/clk.h>

drivers/pwm/pwm-tegra.c +1 -0

@@ -41,6 +72,7 @@ struct tegra_pwm_chip {
      struct reset_control*rst;
 
      unsigned long clk_rate;
+     unsigned long min_period_ns;
 
      void __iomem *regs;
 

@@ -67,8 +99,9 @@ static int tegra_pwm_config(struct pwm_chip *chip,

struct pwm_device *pwm,

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip · line 110

Quoted 12 lines

drivers/pwm/pwm-tegra.c +3 -2

                          int duty_ns, int period_ns)  {
      struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
-     unsigned long long c = duty_ns, hz;
-     unsigned long rate;
+     unsigned long long p_width = duty_ns, period_hz;
+     unsigned long rate, required_clk_rate;
+     unsigned long pfm; /* Frequency divider */
      u32 val = 0;
      int err;
 

@@ -77,37 +110,77 @@ static int tegra_pwm_config(struct pwm_chip
 *chip,

struct pwm_device *pwm,

Quoted 7 lines

drivers/pwm/pwm-tegra.c +2 -2

       * per (1 << PWM_DUTY_WIDTH) cycles and make sure to round to the
       * nearest integer during division.
       */
-     c *= (1 << PWM_DUTY_WIDTH);
-     c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
+     p_width *= (1 << PWM_DUTY_WIDTH);
+     p_width = DIV_ROUND_CLOSEST_ULL(p_width, period_ns);

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 10 lines

drivers/pwm/pwm-tegra.c +8 -1

 
-     val = (u32)c << PWM_DUTY_SHIFT;
+     val = (u32)p_width << PWM_DUTY_SHIFT;
+
+     /*
+      *  Period in nano second has to be <= highest allowed period
+      *  based on max clock rate of the pwm controller.
+      *
+      *  higher limit = max clock limit >> PWM_DUTY_WIDTH
+      *  lower limit = min clock limit >> PWM_DUTY_WIDTH >>

PWM_SCALE_WIDTH

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 128

Quoted 5 lines

drivers/pwm/pwm-tegra.c +5 -0

+      */
+     if (period_ns < pc->min_period_ns) {
+             period_ns = pc->min_period_ns;
+             pr_warn("Period is adjusted to allowed value (%d ns)\n",
+                             period_ns);

Quoted 3 lines

That pr_warn is a bad idea as it spams the kernel log when the configuration is changed frequently. Wouldn't it be easier to calculate the frequency that is needed to achieve period_ns and check that

against max_frequency?

I think you are suggesting which is done just next:
required_clk_rate = (NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH; clk_set_rate(pc->clk, required_clk_rate); Please let me know if I missed what you meant.

Quoted 11 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 135

Quoted 9 lines

drivers/pwm/pwm-tegra.c +2 -1

+     }
 
      /*
       * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
       * cycles at the PWM clock rate will take period_ns nanoseconds.
       */
-     rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+     if (pc->soc->num_channels == 1) {

required_clk_rate could be defined here, which is better as it narrows its

scope.

Quoted 1 line

Noted, will define here to limit its scope.

Quoted 6 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 4 lines

drivers/pwm/pwm-tegra.c +4 -0

+             /*
+              * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it matches
+              * with the hieghest applicable rate that the
+ controller can

s/hieghest/highest/

Noted, correction in next patch

Quoted 7 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 142

Quoted 5 lines

drivers/pwm/pwm-tegra.c +4 -0

+              * provide. Any further lower value can be derived by setting
+              * PFM bits[0:12].
+              * Higher mark is taken since BPMP has round-up mechanism
+              * implemented.

I don't understand the part with the round-up mechanism.

Understood. This comment is misleading.
I think it can be updated as below:
"required_clk_rate" is a reference rate for source clock and it is derived based on user requested period.
By successfully setting the source clock rate to required_clk_rate, pwm controller can configure the requested period.

Quoted 10 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 149

Quoted 8 lines

drivers/pwm/pwm-tegra.c +7 -0

+              */
+             required_clk_rate =
+                     (NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
+
+             err = clk_set_rate(pc->clk, required_clk_rate);
+             if (err < 0)
+                     return -EINVAL;

What happens if clk_set_rate configures a higher rate than requested?

The clock configuration is taken care in a separate R5 called BPMP. BPMP-FW does not round up the rate of PWM controller clock.
It rounds down the clock divider value that generates PWM controller clock.

Quoted 8 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 154

Quoted 6 lines

drivers/pwm/pwm-tegra.c +5 -0

+
+             rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
+     } else {
+             /*
+              * This is the case for SoCs who support multiple channels:

s/who/that/

Noted, correction in next patch.

Quoted 48 lines

Quoted 4 lines

drivers/pwm/pwm-tegra.c +3 -0

+              *
+              * clk_set_rate() can not be called again in config because
+              * T210 or any prior chip supports one pwm-controller and

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 10 lines

drivers/pwm/pwm-tegra.c +6 -2

+              * multiple channels. Hence in this case cached clock rate
+              * will be considered which was stored during probe.
+              */
+             rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+     }
 
      /* Consider precision in PWM_SCALE_WIDTH rate calculation */
-     hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
-     rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
+     period_hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC,

period_ns);

Quoted 28 lines

drivers/pwm/pwm-tegra.c +8 -6

+     pfm = DIV_ROUND_CLOSEST_ULL(100ULL * rate, period_hz);
 
      /*
       * Since the actual PWM divider is the register's frequency divider
-      * field minus 1, we need to decrement to get the correct value to
+      * field plus 1, we need to decrement to get the correct value
+ to
       * write to the register.
       */
-     if (rate > 0)
-             rate--;
+     if (pfm > 0)
+             pfm--;
 
      /*
-      * Make sure that the rate will fit in the register's frequency
+      * Make sure that pfm will fit in the register's frequency
       * divider field.
       */
-     if (rate >> PWM_SCALE_WIDTH)
+     if (pfm >> PWM_SCALE_WIDTH)
              return -EINVAL;
 
-     val |= rate << PWM_SCALE_SHIFT;
+     val |= pfm << PWM_SCALE_SHIFT;
 
      /*
       * If the PWM channel is disabled, make sure to turn on the

Quoted 1 line

clock @@ -205,6 +278,10 @@ static int tegra_pwm_probe(struct

platform_device *pdev)

Quoted 3 lines

       */
      pwm->clk_rate = clk_get_rate(pwm->clk);

Quoted 7 lines

drivers/pwm/pwm-tegra.c

Quoted 5 lines

drivers/pwm/pwm-tegra.c +5 -0

+     /* Set minimum limit of PWM period for the IP */
+     pwm->min_period_ns =
+         (NSEC_PER_SEC / (pwm->soc->max_frequency >>
+ PWM_DUTY_WIDTH))
+ + 1;

With my suggestion above, you can drop the min_period_ns field.

I have added some comments above. Please help me understand if they are not aligned with what you meant.

Quoted 18 lines

Quoted 11 lines

+

      pwm->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
      if (IS_ERR(pwm->rst)) {
              ret = PTR_ERR(pwm->rst); @@ -313,4 +390,5 @@

module_platform_driver(tegra_pwm_driver);

MODULE_LICENSE("GPL");
MODULE_AUTHOR("NVIDIA Corporation");
+MODULE_AUTHOR("Sandipan Patra <[email protected]>");
MODULE_ALIAS("platform:tegra-pwm");

Best regards
Uwe

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

Added support for dynamic clock freq configuration in pwm kernel driver. Earlier the pwm driver used to cache boot time clock rate by pwm clock parent during probe. Hence dynamically changing pwm frequency was not possible for all the possible ranges. With this change, dynamic calculation is enabled and it is able to set the requested period from sysfs knob provided the value is supported by clock source.

Changes mainly have 2 parts:
- T186 and later chips [1]
- T210 and prior chips [2]

For [1] - Changes implemented to set pwm period dynamically and
also checks added to allow only if requested period(ns) is below or equals to higher range.

For [2] - Only checks if the requested period(ns) is below or equals
to higher range defined by max clock limit. The limitation in T210 or prior chips are due to the reason of having only one pwm-controller supporting multiple channels. But later chips have multiple pwm controller instances each having single channel support.

Signed-off-by Sandipan Patra <[email protected]>

---
V2:
1. Min period_ns calculation is moved to probe.
2. Added descriptioins for PWM register bits and regarding behaviour of the controller when new configuration is applied or pwm is disabled.
3. Setting period with possible value when supplied period is below limit.
4. Corrected the earlier code comment:
plus 1 instead of minus 1 during pwm calculation

 drivers/pwm/pwm-tegra.c | 110 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 94 insertions(+), 16 deletions(-)

drivers/pwm/pwm-tegra.c +34 -2

index d26ed8f..7a36325 100644
@@ -4,8 +4,39 @@
  *
  * Tegra pulse-width-modulation controller driver
  *
- * Copyright (c) 2010, NVIDIA Corporation.
- * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer <[email protected]>
+ * Copyright (c) 2010-2020, NVIDIA Corporation.
+ *
+ * Overview of Tegra Pulse Width Modulator Register:
+ * 1. 13-bit: Frequency division (SCALE)
+ * 2. 8-bit : Puls division (DUTY)
+ * 3. 1-bit : Enable bit
+ *
+ * The PWM clock frequency is divided by 256 before subdividing it based
+ * on the programmable frequency division value to generate the required
+ * frequency for PWM output. The maximum output frequency that can be
+ * achieved is (max rate of source clock) / 256.
+ * i.e. if source clock rate is 408 MHz, maximum output frequency cab be:
+ * 408 MHz/256 = 1.6 MHz.
+ * This 1.6 MHz frequency can further be divided using SCALE value in PWM.
+ *
+ * PWM pulse width: 8 bits are usable [23:16] for varying pulse width.
+ * To achieve 100% duty cycle, program Bit [24] of this register to
+ * 1’b1. In which case the other bits [23:16] are set to don't care.
+ *
+ * Limitations and known facts:
+ * -	When PWM is disabled, the output is driven to 0.
+ * -	It does not allow the current PWM period to complete and
+ *	stops abruptly.
+ *
+ * -	If the register is reconfigured while pwm is running,
+ *	It does not let the currently running period to complete.
+ *
+ * -	Pulse width of the pwm can never be out of bound.
+ *	It's taken care at HW and SW
+ * -	If the user input duty is below limit, then driver sets it to
+ *	minimum possible value.
+ * -	If anything else goes wrong for setting duty or period,
+ *	-EINVAL is returned.
  */
 
 #include <linux/clk.h>

@@ -41,6 +72,7 @@ struct tegra_pwm_chip {
 	struct reset_control*rst;
 
 	unsigned long clk_rate;
+	unsigned long min_period_ns;
 
 	void __iomem *regs;
 

drivers/pwm/pwm-tegra.c +5 -3

 	u32 val = 0;
 	int err;
 

@@ -77,37 +110,77 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * per (1 << PWM_DUTY_WIDTH) cycles and make sure to round to the
 	 * nearest integer during division.
 	 */
-	c *= (1 << PWM_DUTY_WIDTH);
-	c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
+	p_width *= (1 << PWM_DUTY_WIDTH);
+	p_width = DIV_ROUND_CLOSEST_ULL(p_width, period_ns);
 
-	val = (u32)c << PWM_DUTY_SHIFT;
+	val = (u32)p_width << PWM_DUTY_SHIFT;
+
+	/*

drivers/pwm/pwm-tegra.c +24 -8

+	} else {
+		/*
+		 * This is the case for SoCs who support multiple channels:
+		 *
+		 * clk_set_rate() can not be called again in config because
+		 * T210 or any prior chip supports one pwm-controller and
+		 * multiple channels. Hence in this case cached clock rate
+		 * will be considered which was stored during probe.
+		 */
+		rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+	}
 
 	/* Consider precision in PWM_SCALE_WIDTH rate calculation */
-	hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
-	rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
+	period_hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
+	pfm = DIV_ROUND_CLOSEST_ULL(100ULL * rate, period_hz);
 
 	/*
 	 * Since the actual PWM divider is the register's frequency divider
-	 * field minus 1, we need to decrement to get the correct value to
+	 * field plus 1, we need to decrement to get the correct value to
 	 * write to the register.
 	 */
-	if (rate > 0)
-		rate--;
+	if (pfm > 0)
+		pfm--;
 
 	/*
-	 * Make sure that the rate will fit in the register's frequency
+	 * Make sure that pfm will fit in the register's frequency
 	 * divider field.
 	 */
-	if (rate >> PWM_SCALE_WIDTH)
+	if (pfm >> PWM_SCALE_WIDTH)
 		return -EINVAL;
 
-	val |= rate << PWM_SCALE_SHIFT;
+	val |= pfm << PWM_SCALE_SHIFT;
 
 	/*
 	 * If the PWM channel is disabled, make sure to turn on the clock

@@ -205,6 +278,10 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 	 */
 	pwm->clk_rate = clk_get_rate(pwm->clk);
 
+	/* Set minimum limit of PWM period for the IP */
+	pwm->min_period_ns =
+	    (NSEC_PER_SEC / (pwm->soc->max_frequency >> PWM_DUTY_WIDTH)) + 1;
+
 	pwm->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
 	if (IS_ERR(pwm->rst)) {
 		ret = PTR_ERR(pwm->rst);

@@ -313,4 +390,5 @@ module_platform_driver(tegra_pwm_driver);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("NVIDIA Corporation");
+MODULE_AUTHOR("Sandipan Patra <[email protected]>");
 MODULE_ALIAS("platform:tegra-pwm");

-----Original Message-----
From: Jonathan Hunter <[email protected]>
Sent: Friday, May 22, 2020 3:54 PM
To: Sandipan Patra <[email protected]>; Thierry Reding <[email protected]>; [email protected]; [email protected]
Cc: Bibek Basu <[email protected]>; Laxman Dewangan <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM driver

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip,

Quoted 94 lines · Sandipan Patra

Added support for dynamic clock freq configuration in pwm kernel driver. Earlier the pwm driver used to cache boot time clock rate by pwm clock parent during probe. Hence dynamically changing pwm frequency was not possible for all the possible ranges. With this change, dynamic calculation is enabled and it is able to set the requested period from sysfs knob provided the value is supported by clock source.

Changes mainly have 2 parts:
- T186 and later chips [1]
- T210 and prior chips [2]

For [1] - Changes implemented to set pwm period dynamically and
also checks added to allow only if requested period(ns) is below or equals to higher range.

For [2] - Only checks if the requested period(ns) is below or equals
to higher range defined by max clock limit. The limitation in T210 or prior chips are due to the reason of having only one pwm-controller supporting multiple channels. But later chips have multiple pwm controller instances each having single channel support.

Signed-off-by Sandipan Patra <[email protected]>

---
V2:
1. Min period_ns calculation is moved to probe.
2. Added descriptioins for PWM register bits and regarding behaviour of the controller when new configuration is applied or pwm is disabled.
3. Setting period with possible value when supplied period is below limit.
4. Corrected the earlier code comment:
plus 1 instead of minus 1 during pwm calculation

 drivers/pwm/pwm-tegra.c | 110
+++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 94 insertions(+), 16 deletions(-)

drivers/pwm/pwm-tegra.c index renamed from drivers/pwm/pwm-tegra.c +0 -0

d26ed8f..7a36325 100644

drivers/pwm/pwm-tegra.c +0 -2

@@ -4,8 +4,39 @@
  *
  * Tegra pulse-width-modulation controller driver
  *
- * Copyright (c) 2010, NVIDIA Corporation.
- * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer

<[email protected]>
+ * Copyright (c) 2010-2020, NVIDIA Corporation.
+ *
+ * Overview of Tegra Pulse Width Modulator Register:
+ * 1. 13-bit: Frequency division (SCALE)
+ * 2. 8-bit : Puls division (DUTY)
+ * 3. 1-bit : Enable bit
+ *
+ * The PWM clock frequency is divided by 256 before subdividing it
+ based
+ * on the programmable frequency division value to generate the
+ required
+ * frequency for PWM output. The maximum output frequency that can be
+ * achieved is (max rate of source clock) / 256.
+ * i.e. if source clock rate is 408 MHz, maximum output frequency cab be:
+ * 408 MHz/256 = 1.6 MHz.
+ * This 1.6 MHz frequency can further be divided using SCALE value in PWM.
+ *
+ * PWM pulse width: 8 bits are usable [23:16] for varying pulse width.
+ * To achieve 100% duty cycle, program Bit [24] of this register to
+ * 1’b1. In which case the other bits [23:16] are set to don't care.
+ *
+ * Limitations and known facts:
+ * - When PWM is disabled, the output is driven to 0.
+ * - It does not allow the current PWM period to complete and
+ * stops abruptly.
+ *
+ * - If the register is reconfigured while pwm is running,
+ * It does not let the currently running period to complete.
+ *
+ * - Pulse width of the pwm can never be out of bound.
+ * It's taken care at HW and SW
+ * - If the user input duty is below limit, then driver sets it to
+ * minimum possible value.
+ * - If anything else goes wrong for setting duty or period,
+ * -EINVAL is returned.
*/

#include <linux/clk.h>

drivers/pwm/pwm-tegra.c +1 -0

@@ -41,6 +72,7 @@ struct tegra_pwm_chip {
 	struct reset_control*rst;
 
 	unsigned long clk_rate;
+	unsigned long min_period_ns;
 
 	void __iomem *regs;
 

@@ -67,8 +99,9 @@ static int tegra_pwm_config(struct pwm_chip *chip,

struct pwm_device *pwm,

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 104

Quoted 8 lines

drivers/pwm/pwm-tegra.c +3 -2

 			    int duty_ns, int period_ns)
 {
 	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
-	unsigned long long c = duty_ns, hz;
-	unsigned long rate;
+	unsigned long long p_width = duty_ns, period_hz;
+	unsigned long rate, required_clk_rate;
+	unsigned long pfm; /* Frequency divider */

If it is not necessary to change the variable names, then I would prefer we keep them as is as then changes would be less.

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip,

Quoted 5 lines

drivers/pwm/pwm-tegra.c +0 -0

 	u32 val = 0;
 	int err;
 

@@ -77,37 +110,77 @@ static int tegra_pwm_config(struct pwm_chip *chip,

struct pwm_device *pwm,

Quoted 7 lines

drivers/pwm/pwm-tegra.c +2 -2

 	 * per (1 << PWM_DUTY_WIDTH) cycles and make sure to round to the
 	 * nearest integer during division.
 	 */
-	c *= (1 << PWM_DUTY_WIDTH);
-	c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
+	p_width *= (1 << PWM_DUTY_WIDTH);
+	p_width = DIV_ROUND_CLOSEST_ULL(p_width, period_ns);

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 10 lines

drivers/pwm/pwm-tegra.c +8 -1

 
-	val = (u32)c << PWM_DUTY_SHIFT;
+	val = (u32)p_width << PWM_DUTY_SHIFT;
+
+	/*
+	 *  Period in nano second has to be <= highest allowed period
+	 *  based on max clock rate of the pwm controller.
+	 *
+	 *  higher limit = max clock limit >> PWM_DUTY_WIDTH
+	 *  lower limit = min clock limit >> PWM_DUTY_WIDTH >>

PWM_SCALE_WIDTH

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 128

Quoted 5 lines

drivers/pwm/pwm-tegra.c +5 -0

+	 */
+	if (period_ns < pc->min_period_ns) {
+		period_ns = pc->min_period_ns;
+		pr_warn("Period is adjusted to allowed value (%d ns)\n",
+				period_ns);

I see that other drivers (pwm-img.c) consider this to be an error and return an error. I wonder if adjusting the period makes sense here?

I wonder if the handling of the min_period, should be a separate change?

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 135

Quoted 9 lines

drivers/pwm/pwm-tegra.c +2 -1

+	}
 
 	/*
 	 * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
 	 * cycles at the PWM clock rate will take period_ns nanoseconds.
 	 */
-	rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+	if (pc->soc->num_channels == 1) {

Are you using num_channels to determine if Tegra uses the BPMP? If so then the above is not really correct, because num_channels is not really related to what is being done here. So maybe you need a new SoC attribute in the soc data.

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, · line 137

Quoted 3 lines

drivers/pwm/pwm-tegra.c +2 -0

+		/*
+		 * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it

matches

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 138

Quoted 1 line

drivers/pwm/pwm-tegra.c +1 -0

+		 * with the hieghest applicable rate that the controller can

s/hieghest/highest/

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 146

Quoted 9 lines

drivers/pwm/pwm-tegra.c +8 -0

+		 * provide. Any further lower value can be derived by setting
+		 * PFM bits[0:12].
+		 * Higher mark is taken since BPMP has round-up mechanism
+		 * implemented.
+		 */
+		required_clk_rate =
+			(NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
+

Should be we checking the rate against the max rate supported?

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 151

Quoted 6 lines

drivers/pwm/pwm-tegra.c +5 -0

+		err = clk_set_rate(pc->clk, required_clk_rate);
+		if (err < 0)
+			return -EINVAL;
+
+		rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;

Do we need to update the pwm->clk_rate here?

Quoted 7 lines

drivers/pwm/pwm-tegra.c +6 -0

+	} else {
+		/*
+		 * This is the case for SoCs who support multiple channels:
+		 *
+		 * clk_set_rate() can not be called again in config because
+		 * T210 or any prior chip supports one pwm-controller and

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 10 lines

drivers/pwm/pwm-tegra.c +6 -2

+		 * multiple channels. Hence in this case cached clock rate
+		 * will be considered which was stored during probe.
+		 */
+		rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+	}
 
 	/* Consider precision in PWM_SCALE_WIDTH rate calculation */
-	hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
-	rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
+	period_hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC,

period_ns);

Quoted 18 lines

drivers/pwm/pwm-tegra.c +5 -4

+	pfm = DIV_ROUND_CLOSEST_ULL(100ULL * rate, period_hz);
 
 	/*
 	 * Since the actual PWM divider is the register's frequency divider
-	 * field minus 1, we need to decrement to get the correct value to
+	 * field plus 1, we need to decrement to get the correct value to
 	 * write to the register.
 	 */
-	if (rate > 0)
-		rate--;
+	if (pfm > 0)
+		pfm--;
 
 	/*
-	 * Make sure that the rate will fit in the register's frequency
+	 * Make sure that pfm will fit in the register's frequency
 	 * divider field.
 	 */

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 10 lines

drivers/pwm/pwm-tegra.c +2 -3

-	if (rate >> PWM_SCALE_WIDTH)
+	if (pfm >> PWM_SCALE_WIDTH)
 		return -EINVAL;
 
-	val |= rate << PWM_SCALE_SHIFT;
+	val |= pfm << PWM_SCALE_SHIFT;
 
 	/*
 	 * If the PWM channel is disabled, make sure to turn on the clock @@
-205,6 +278,10 @@ static int tegra_pwm_probe(struct platform_device

*pdev)

drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev)

Quoted 6 lines

drivers/pwm/pwm-tegra.c +3 -0

 	 */
 	pwm->clk_rate = clk_get_rate(pwm->clk);
 
+	/* Set minimum limit of PWM period for the IP */
+	pwm->min_period_ns =
+	    (NSEC_PER_SEC / (pwm->soc->max_frequency >>

PWM_DUTY_WIDTH)) +

drivers/pwm/pwm-tegra.c module_platform_driver(tegra_pwm_driver); · line 394

Quoted 12 lines

+1;

drivers/pwm/pwm-tegra.c +2 -0

+
 	pwm->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
 	if (IS_ERR(pwm->rst)) {
 		ret = PTR_ERR(pwm->rst);

@@ -313,4 +390,5 @@ module_platform_driver(tegra_pwm_driver);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("NVIDIA Corporation");
+MODULE_AUTHOR("Sandipan Patra <[email protected]>");
 MODULE_ALIAS("platform:tegra-pwm");

--
nvpublic

Thanks Jonathan,
Please help reviewing further with my replies inline.

Thanks & Regards,
Sandipan

Quoted 109 lines

-----Original Message-----
From: Jonathan Hunter <[email protected]>
Sent: Friday, May 22, 2020 3:54 PM
To: Sandipan Patra <[email protected]>; Thierry Reding <[email protected]>; [email protected]; [email protected]
Cc: Bibek Basu <[email protected]>; Laxman Dewangan <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM
driver

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip,

Quoted 94 lines · Sandipan Patra

Added support for dynamic clock freq configuration in pwm kernel driver. Earlier the pwm driver used to cache boot time clock rate by pwm clock parent during probe. Hence dynamically changing pwm frequency was not possible for all the possible ranges. With this change, dynamic calculation is enabled and it is able to set the requested period from sysfs knob provided the value is supported by clock source.

Changes mainly have 2 parts:
- T186 and later chips [1]
- T210 and prior chips [2]

For [1] - Changes implemented to set pwm period dynamically and
also checks added to allow only if requested period(ns) is below or equals to higher range.

For [2] - Only checks if the requested period(ns) is below or equals
to higher range defined by max clock limit. The limitation in T210 or prior chips are due to the reason of having only one pwm-controller supporting multiple channels. But later chips have multiple pwm controller instances each having single channel support.

Signed-off-by Sandipan Patra <[email protected]>

---
V2:
1. Min period_ns calculation is moved to probe.
2. Added descriptioins for PWM register bits and regarding behaviour of the controller when new configuration is applied or pwm is disabled.
3. Setting period with possible value when supplied period is below limit.
4. Corrected the earlier code comment:
plus 1 instead of minus 1 during pwm calculation

 drivers/pwm/pwm-tegra.c | 110
+++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 94 insertions(+), 16 deletions(-)

drivers/pwm/pwm-tegra.c index renamed from drivers/pwm/pwm-tegra.c +0 -0

d26ed8f..7a36325 100644

drivers/pwm/pwm-tegra.c +0 -2

@@ -4,8 +4,39 @@
  *
  * Tegra pulse-width-modulation controller driver
  *
- * Copyright (c) 2010, NVIDIA Corporation.
- * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer

<[email protected]>
+ * Copyright (c) 2010-2020, NVIDIA Corporation.
+ *
+ * Overview of Tegra Pulse Width Modulator Register:
+ * 1. 13-bit: Frequency division (SCALE)
+ * 2. 8-bit : Puls division (DUTY)
+ * 3. 1-bit : Enable bit
+ *
+ * The PWM clock frequency is divided by 256 before subdividing it
+ based
+ * on the programmable frequency division value to generate the
+ required
+ * frequency for PWM output. The maximum output frequency that can be
+ * achieved is (max rate of source clock) / 256.
+ * i.e. if source clock rate is 408 MHz, maximum output frequency cab be:
+ * 408 MHz/256 = 1.6 MHz.
+ * This 1.6 MHz frequency can further be divided using SCALE value in PWM.
+ *
+ * PWM pulse width: 8 bits are usable [23:16] for varying pulse width.
+ * To achieve 100% duty cycle, program Bit [24] of this register to
+ * 1’b1. In which case the other bits [23:16] are set to don't care.
+ *
+ * Limitations and known facts:
+ * - When PWM is disabled, the output is driven to 0.
+ * - It does not allow the current PWM period to complete and
+ * stops abruptly.
+ *
+ * - If the register is reconfigured while pwm is running,
+ * It does not let the currently running period to complete.
+ *
+ * - Pulse width of the pwm can never be out of bound.
+ * It's taken care at HW and SW
+ * - If the user input duty is below limit, then driver sets it to
+ * minimum possible value.
+ * - If anything else goes wrong for setting duty or period,
+ * -EINVAL is returned.
*/

#include <linux/clk.h>

drivers/pwm/pwm-tegra.c +1 -0

@@ -41,6 +72,7 @@ struct tegra_pwm_chip {
 	struct reset_control*rst;
 
 	unsigned long clk_rate;
+	unsigned long min_period_ns;
 
 	void __iomem *regs;
 

@@ -67,8 +99,9 @@ static int tegra_pwm_config(struct pwm_chip *chip,

struct pwm_device *pwm,

Quoted 11 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 104

Quoted 8 lines

drivers/pwm/pwm-tegra.c +3 -2

 			    int duty_ns, int period_ns)
 {
 	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
-	unsigned long long c = duty_ns, hz;
-	unsigned long rate;
+	unsigned long long p_width = duty_ns, period_hz;
+	unsigned long rate, required_clk_rate;
+	unsigned long pfm; /* Frequency divider */

If it is not necessary to change the variable names, then I would prefer we keep them as is as then changes would be less.

The earlier name was misleading so thought to use a specific name for which it can be helpful to follow up with the TRM. Since its recommended to retain the variable names, I will change this in next patch.

Quoted 33 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip,

Quoted 5 lines

drivers/pwm/pwm-tegra.c +0 -0

 	u32 val = 0;
 	int err;
 

@@ -77,37 +110,77 @@ static int tegra_pwm_config(struct pwm_chip *chip,

struct pwm_device *pwm,

Quoted 7 lines

drivers/pwm/pwm-tegra.c +2 -2

 	 * per (1 << PWM_DUTY_WIDTH) cycles and make sure to round to the
 	 * nearest integer during division.
 	 */
-	c *= (1 << PWM_DUTY_WIDTH);
-	c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
+	p_width *= (1 << PWM_DUTY_WIDTH);
+	p_width = DIV_ROUND_CLOSEST_ULL(p_width, period_ns);

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 10 lines

drivers/pwm/pwm-tegra.c +8 -1

 
-	val = (u32)c << PWM_DUTY_SHIFT;
+	val = (u32)p_width << PWM_DUTY_SHIFT;
+
+	/*
+	 *  Period in nano second has to be <= highest allowed period
+	 *  based on max clock rate of the pwm controller.
+	 *
+	 *  higher limit = max clock limit >> PWM_DUTY_WIDTH
+	 *  lower limit = min clock limit >> PWM_DUTY_WIDTH >>

PWM_SCALE_WIDTH

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 128

Quoted 5 lines

drivers/pwm/pwm-tegra.c +5 -0

+	 */
+	if (period_ns < pc->min_period_ns) {
+		period_ns = pc->min_period_ns;
+		pr_warn("Period is adjusted to allowed value (%d ns)\n",
+				period_ns);

I see that other drivers (pwm-img.c) consider this to be an error and return an error. I wonder if adjusting the period makes sense here?

Quoted 1 line

I wonder if the handling of the min_period, should be a separate change?

I think I misunderstood one of the discussions in initial patch and added this change to apply the minimum possible value. Understood and will revert this change with returning error in such case.

Quoted 13 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 135

Quoted 9 lines

drivers/pwm/pwm-tegra.c +2 -1

+	}
 
 	/*
 	 * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
 	 * cycles at the PWM clock rate will take period_ns nanoseconds.
 	 */
-	rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+	if (pc->soc->num_channels == 1) {

Are you using num_channels to determine if Tegra uses the BPMP? If so then the above is not really correct, because num_channels is not really related to what is being done here. So maybe you need a new SoC attribute in the soc data.

Here, it tries to find if pwm controller uses multiple channels (like in Tegra210 or older) or single channel for every pwm instance (i.e. T186, T194). If found multiple channels on a single controller then it is not correct to configure separate clock rates to each of the channels. So to distinguish the controller and channel type, num_channels is referred.

Quoted 7 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, · line 137

Quoted 3 lines

drivers/pwm/pwm-tegra.c +2 -0

+		/*
+		 * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it

matches

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 138

Quoted 1 line

drivers/pwm/pwm-tegra.c +1 -0

+		 * with the hieghest applicable rate that the controller can

s/hieghest/highest/

Got it.

Quoted 11 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 146

Quoted 9 lines

drivers/pwm/pwm-tegra.c +8 -0

+		 * provide. Any further lower value can be derived by setting
+		 * PFM bits[0:12].
+		 * Higher mark is taken since BPMP has round-up mechanism
+		 * implemented.
+		 */
+		required_clk_rate =
+			(NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
+

Should be we checking the rate against the max rate supported?

If the request rate is beyond max supported rate, then the clk_set_rate will be failing and can get caught with error check followed by. Otherwise it will fail through fitting in the register's frequency divider filed. So I think it is not required to check against max rate. Please advise if I am not able to follow with what you are suggesting.

Quoted 8 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 151

Quoted 6 lines

drivers/pwm/pwm-tegra.c +5 -0

+		err = clk_set_rate(pc->clk, required_clk_rate);
+		if (err < 0)
+			return -EINVAL;
+
+		rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;

Do we need to update the pwm->clk_rate here?

This return rate is basically from the factor that requested clk_set_rate and the actual rate set mostly will have a little deviation based on the clock divider and other factors while setting a new rate. So capturing the actual rate for further calculation and conversion to Hz. Whenever it is required to use pwm->clk_rate we are no longer depending upon the cached value for num_channels == 1. So in my opinion it does not need to be cached. However it is kept stored for the SoCs having num_channels > 1.
Please suggest if I am missing any case where we need to keep the value stored.

Quoted 69 lines

Quoted 7 lines

drivers/pwm/pwm-tegra.c +6 -0

+	} else {
+		/*
+		 * This is the case for SoCs who support multiple channels:
+		 *
+		 * clk_set_rate() can not be called again in config because
+		 * T210 or any prior chip supports one pwm-controller and

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 10 lines

drivers/pwm/pwm-tegra.c +6 -2

+		 * multiple channels. Hence in this case cached clock rate
+		 * will be considered which was stored during probe.
+		 */
+		rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+	}
 
 	/* Consider precision in PWM_SCALE_WIDTH rate calculation */
-	hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
-	rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
+	period_hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC,

period_ns);

Quoted 18 lines

drivers/pwm/pwm-tegra.c +5 -4

+	pfm = DIV_ROUND_CLOSEST_ULL(100ULL * rate, period_hz);
 
 	/*
 	 * Since the actual PWM divider is the register's frequency divider
-	 * field minus 1, we need to decrement to get the correct value to
+	 * field plus 1, we need to decrement to get the correct value to
 	 * write to the register.
 	 */
-	if (rate > 0)
-		rate--;
+	if (pfm > 0)
+		pfm--;
 
 	/*
-	 * Make sure that the rate will fit in the register's frequency
+	 * Make sure that pfm will fit in the register's frequency
 	 * divider field.
 	 */

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 10 lines

drivers/pwm/pwm-tegra.c +2 -3

-	if (rate >> PWM_SCALE_WIDTH)
+	if (pfm >> PWM_SCALE_WIDTH)
 		return -EINVAL;
 
-	val |= rate << PWM_SCALE_SHIFT;
+	val |= pfm << PWM_SCALE_SHIFT;
 
 	/*
 	 * If the PWM channel is disabled, make sure to turn on the clock @@
-205,6 +278,10 @@ static int tegra_pwm_probe(struct platform_device

*pdev)

drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev)

Quoted 6 lines

drivers/pwm/pwm-tegra.c +3 -0

 	 */
 	pwm->clk_rate = clk_get_rate(pwm->clk);
 
+	/* Set minimum limit of PWM period for the IP */
+	pwm->min_period_ns =
+	    (NSEC_PER_SEC / (pwm->soc->max_frequency >>

PWM_DUTY_WIDTH)) +

drivers/pwm/pwm-tegra.c module_platform_driver(tegra_pwm_driver); · line 394

Quoted 12 lines

+1;

drivers/pwm/pwm-tegra.c +2 -0

+
 	pwm->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
 	if (IS_ERR(pwm->rst)) {
 		ret = PTR_ERR(pwm->rst);

@@ -313,4 +390,5 @@ module_platform_driver(tegra_pwm_driver);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("NVIDIA Corporation");
+MODULE_AUTHOR("Sandipan Patra <[email protected]>");
 MODULE_ALIAS("platform:tegra-pwm");

--
nvpublic

-----Original Message-----
From: Jonathan Hunter <[email protected]>
Sent: Friday, May 22, 2020 5:20 PM
To: Sandipan Patra <[email protected]>; Thierry Reding <[email protected]>; [email protected]; [email protected]
Cc: Bibek Basu <[email protected]>; Laxman Dewangan <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM driver

Quoted 78 lines · Sandipan Patra

Thanks Jonathan,
Please help reviewing further with my replies inline.

Thanks & Regards,
Sandipan

Quoted 71 lines

-----Original Message-----
From: Jonathan Hunter <[email protected]>
Sent: Friday, May 22, 2020 3:54 PM
To: Sandipan Patra <[email protected]>; Thierry Reding <[email protected]>; [email protected]; [email protected]
Cc: Bibek Basu <[email protected]>; Laxman Dewangan <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM driver

Quoted 56 lines · Sandipan Patra

Added support for dynamic clock freq configuration in pwm kernel driver. Earlier the pwm driver used to cache boot time clock rate by pwm clock parent during probe. Hence dynamically changing pwm frequency was not possible for all the possible ranges. With this change, dynamic calculation is enabled and it is able to set the requested period from sysfs knob provided the value is supported by clock source.

Changes mainly have 2 parts:
- T186 and later chips [1]
- T210 and prior chips [2]

For [1] - Changes implemented to set pwm period dynamically and
also checks added to allow only if requested period(ns) is below or equals to higher range.

For [2] - Only checks if the requested period(ns) is below or equals
to higher range defined by max clock limit. The limitation in T210 or prior chips are due to the reason of having only one pwm-controller supporting multiple channels. But later chips have multiple pwm controller instances each having single channel support.

Signed-off-by Sandipan Patra <[email protected]>

---
V2:
1. Min period_ns calculation is moved to probe.
2. Added descriptioins for PWM register bits and regarding behaviour of the controller when new configuration is applied or pwm is disabled.
3. Setting period with possible value when supplied period is below limit.
4. Corrected the earlier code comment:
plus 1 instead of minus 1 during pwm calculation

 drivers/pwm/pwm-tegra.c | 110
+++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 94 insertions(+), 16 deletions(-)

drivers/pwm/pwm-tegra.c index renamed from drivers/pwm/pwm-tegra.c +0 -0

d26ed8f..7a36325 100644

drivers/pwm/pwm-tegra.c +0 -2

@@ -4,8 +4,39 @@
  *
  * Tegra pulse-width-modulation controller driver
  *
- * Copyright (c) 2010, NVIDIA Corporation.
- * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer

<[email protected]>
+ * Copyright (c) 2010-2020, NVIDIA Corporation.
+ *
+ * Overview of Tegra Pulse Width Modulator Register:
+ * 1. 13-bit: Frequency division (SCALE)
+ * 2. 8-bit : Puls division (DUTY)
+ * 3. 1-bit : Enable bit
+ *
+ * The PWM clock frequency is divided by 256 before subdividing it
+ based

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

+ * on the programmable frequency division value to generate the
+ required
+ * frequency for PWM output. The maximum output frequency that can
+ be
+ * achieved is (max rate of source clock) / 256.
+ * i.e. if source clock rate is 408 MHz, maximum output frequency cab be:
+ * 408 MHz/256 = 1.6 MHz.
+ * This 1.6 MHz frequency can further be divided using SCALE value in

PWM.

Quoted 44 lines

Quoted 43 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip,

Quoted 31 lines

+ *
+ * PWM pulse width: 8 bits are usable [23:16] for varying pulse width.
+ * To achieve 100% duty cycle, program Bit [24] of this register to
+ * 1’b1. In which case the other bits [23:16] are set to don't care.
+ *
+ * Limitations and known facts:
+ * - When PWM is disabled, the output is driven to 0.
+ * - It does not allow the current PWM period to complete and
+ * stops abruptly.
+ *
+ * - If the register is reconfigured while pwm is running,
+ * It does not let the currently running period to complete.
+ *
+ * - Pulse width of the pwm can never be out of bound.
+ * It's taken care at HW and SW
+ * - If the user input duty is below limit, then driver sets it to
+ * minimum possible value.
+ * - If anything else goes wrong for setting duty or period,
+ * -EINVAL is returned.
*/

#include <linux/clk.h>

drivers/pwm/pwm-tegra.c +1 -0

@@ -41,6 +72,7 @@ struct tegra_pwm_chip {
 	struct reset_control*rst;
 
 	unsigned long clk_rate;
+	unsigned long min_period_ns;
 
 	void __iomem *regs;
 

@@ -67,8 +99,9 @@ static int tegra_pwm_config(struct pwm_chip *chip,

struct pwm_device *pwm,

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 104

Quoted 8 lines

drivers/pwm/pwm-tegra.c +3 -2

 			    int duty_ns, int period_ns)
 {
 	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
-	unsigned long long c = duty_ns, hz;
-	unsigned long rate;
+	unsigned long long p_width = duty_ns, period_hz;
+	unsigned long rate, required_clk_rate;
+	unsigned long pfm; /* Frequency divider */

If it is not necessary to change the variable names, then I would prefer we keep them as is as then changes would be less.

Quoted 3 lines

The earlier name was misleading so thought to use a specific name for which it can be helpful to follow up with the TRM. Since its recommended to retain the variable names, I will change this in next patch.

I was just wondering if was necessary to change 'c' to 'p_width'. This could reduce the diff a bit.

Quoted 35 lines

Quoted 35 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip · line 110

Quoted 7 lines

drivers/pwm/pwm-tegra.c +0 -0

 	u32 val = 0;
 	int err;
 

@@ -77,37 +110,77 @@ static int tegra_pwm_config(struct pwm_chip
 *chip,

struct pwm_device *pwm,

Quoted 7 lines

drivers/pwm/pwm-tegra.c +2 -2

 	 * per (1 << PWM_DUTY_WIDTH) cycles and make sure to round to the
 	 * nearest integer during division.
 	 */
-	c *= (1 << PWM_DUTY_WIDTH);
-	c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
+	p_width *= (1 << PWM_DUTY_WIDTH);
+	p_width = DIV_ROUND_CLOSEST_ULL(p_width, period_ns);

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 10 lines

drivers/pwm/pwm-tegra.c +8 -1

 
-	val = (u32)c << PWM_DUTY_SHIFT;
+	val = (u32)p_width << PWM_DUTY_SHIFT;
+
+	/*
+	 *  Period in nano second has to be <= highest allowed period
+	 *  based on max clock rate of the pwm controller.
+	 *
+	 *  higher limit = max clock limit >> PWM_DUTY_WIDTH
+	 *  lower limit = min clock limit >> PWM_DUTY_WIDTH >>

PWM_SCALE_WIDTH

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 128

Quoted 5 lines

drivers/pwm/pwm-tegra.c +5 -0

+	 */
+	if (period_ns < pc->min_period_ns) {
+		period_ns = pc->min_period_ns;
+		pr_warn("Period is adjusted to allowed value (%d ns)\n",
+				period_ns);

I see that other drivers (pwm-img.c) consider this to be an error and return an error. I wonder if adjusting the period makes sense here?

Quoted 19 lines

Quoted 1 line

I wonder if the handling of the min_period, should be a separate change?

I think I misunderstood one of the discussions in initial patch and added this change to apply the minimum possible value. Understood and will revert this change with returning error in such case.

Quoted 13 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 135

Quoted 9 lines

drivers/pwm/pwm-tegra.c +2 -1

+	}
 
 	/*
 	 * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
 	 * cycles at the PWM clock rate will take period_ns nanoseconds.
 	 */
-	rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+	if (pc->soc->num_channels == 1) {

Are you using num_channels to determine if Tegra uses the BPMP? If so then the above is not really correct, because num_channels is not really related to what is being done here. So maybe you need a new SoC

attribute in the soc data.

Quoted 1 line

Quoted 4 lines

Here, it tries to find if pwm controller uses multiple channels (like in Tegra210 or older) or single channel for every pwm instance (i.e. T186, T194). If found multiple channels on a single controller then it is not correct to configure separate clock rates to each of the channels. So to

distinguish the controller and channel type, num_channels is referred.

OK, then that makes sense. Maybe add this detail to the comment about why num_channels is used.

Quoted 23 lines

Quoted 8 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip · line 138

Quoted 4 lines

drivers/pwm/pwm-tegra.c +2 -0

+		/*
+		 * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it

matches

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 138

Quoted 1 line

drivers/pwm/pwm-tegra.c +1 -0

+		 * with the hieghest applicable rate that the controller can

s/hieghest/highest/

Got it.

Quoted 11 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 146

Quoted 9 lines

drivers/pwm/pwm-tegra.c +8 -0

+		 * provide. Any further lower value can be derived by setting
+		 * PFM bits[0:12].
+		 * Higher mark is taken since BPMP has round-up mechanism
+		 * implemented.
+		 */
+		required_clk_rate =
+			(NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
+

Should be we checking the rate against the max rate supported?

Quoted 3 lines

If the request rate is beyond max supported rate, then the clk_set_rate will be failing and can get caught with error check followed by. Otherwise it will fail through fitting in the register's frequency

divider filed. So I think it is not required to check against max rate.

Quoted 1 line

Please advise if I am not able to follow with what you are suggesting.

I think that it would be better to update the cached value so that it is not incorrectly used else where by any future change. Furthermore, this simplifies matters a bit because you can do the following for all devices, but only update the clk_rate for those you wish to ...

    rate = pc->clk_rate >> PWM_DUTY_WIDTH;

Quoted 9 lines

Quoted 8 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 151

Quoted 6 lines

drivers/pwm/pwm-tegra.c +5 -0

+		err = clk_set_rate(pc->clk, required_clk_rate);
+		if (err < 0)
+			return -EINVAL;
+
+		rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;

Do we need to update the pwm->clk_rate here?

Quoted 3 lines

This return rate is basically from the factor that requested clk_set_rate and the actual rate set mostly will have a little deviation based on the clock divider and other factors while setting a new

rate. So capturing the actual rate for further calculation and conversion to Hz.

Quoted 3 lines

Whenever it is required to use pwm->clk_rate we are no longer depending upon the cached value for num_channels == 1. So in my opinion it does not need to be cached. However it is kept stored for the SoCs

having num_channels > 1.

Quoted 1 line

Please suggest if I am missing any case where we need to keep the value

stored.

OK sounds fine.

Quoted 62 lines

Quoted 62 lines

Quoted 7 lines

drivers/pwm/pwm-tegra.c +6 -0

+	} else {
+		/*
+		 * This is the case for SoCs who support multiple channels:
+		 *
+		 * clk_set_rate() can not be called again in config because
+		 * T210 or any prior chip supports one pwm-controller and

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Quoted 10 lines

drivers/pwm/pwm-tegra.c +6 -2

+		 * multiple channels. Hence in this case cached clock rate
+		 * will be considered which was stored during probe.
+		 */
+		rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+	}
 
 	/* Consider precision in PWM_SCALE_WIDTH rate calculation */
-	hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
-	rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
+	period_hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC,

period_ns);

Quoted 26 lines

drivers/pwm/pwm-tegra.c +7 -6

+	pfm = DIV_ROUND_CLOSEST_ULL(100ULL * rate, period_hz);
 
 	/*
 	 * Since the actual PWM divider is the register's frequency divider
-	 * field minus 1, we need to decrement to get the correct value to
+	 * field plus 1, we need to decrement to get the correct value to
 	 * write to the register.
 	 */
-	if (rate > 0)
-		rate--;
+	if (pfm > 0)
+		pfm--;
 
 	/*
-	 * Make sure that the rate will fit in the register's frequency
+	 * Make sure that pfm will fit in the register's frequency
 	 * divider field.
 	 */
-	if (rate >> PWM_SCALE_WIDTH)
+	if (pfm >> PWM_SCALE_WIDTH)
 		return -EINVAL;
 
-	val |= rate << PWM_SCALE_SHIFT;
+	val |= pfm << PWM_SCALE_SHIFT;
 
 	/*

Quoted 3 lines

* If the PWM channel is disabled, make sure to turn on the clock
@@
-205,6 +278,10 @@ static int tegra_pwm_probe(struct platform_device

*pdev)

drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev)

Quoted 6 lines

drivers/pwm/pwm-tegra.c +3 -0

 	 */
 	pwm->clk_rate = clk_get_rate(pwm->clk);
 
+	/* Set minimum limit of PWM period for the IP */
+	pwm->min_period_ns =
+	    (NSEC_PER_SEC / (pwm->soc->max_frequency >>

PWM_DUTY_WIDTH)) +

Quoted 7 lines

+1;

drivers/pwm/pwm-tegra.c +1 -0

+
 	pwm->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
 	if (IS_ERR(pwm->rst)) {
 		ret = PTR_ERR(pwm->rst);

@@ -313,4 +390,5 @@ module_platform_driver(tegra_pwm_driver);
 

Quoted 8 lines

Quoted 8 lines

drivers/pwm/pwm-tegra.c module_platform_driver(tegra_pwm_driver); · line 394

Quoted 5 lines

drivers/pwm/pwm-tegra.c +1 -0

 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("NVIDIA Corporation");
+MODULE_AUTHOR("Sandipan Patra <[email protected]>");
 MODULE_ALIAS("platform:tegra-pwm");

--
nvpublic

--
nvpublic

Quoted 12 lines

Quoted 10 lines

Quoted 10 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 135

Quoted 6 lines

drivers/pwm/pwm-tegra.c +1 -1

 	/*
 	 * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
 	 * cycles at the PWM clock rate will take period_ns nanoseconds.
 	 */
-	rate = pc->clk_rate >> PWM_DUTY_WIDTH;
+	if (pc->soc->num_channels == 1) {

Are you using num_channels to determine if Tegra uses the BPMP? If so then the above is not really correct, because num_channels is not really related to what is being done here. So maybe you need a new SoC

attribute in the soc data.

Quoted 1 line

Quoted 8 lines

Quoted 4 lines

Here, it tries to find if pwm controller uses multiple channels (like in Tegra210 or older) or single channel for every pwm instance (i.e. T186, T194). If found multiple channels on a single controller then it is not correct to configure separate clock rates to each of the channels. So to

distinguish the controller and channel type, num_channels is referred.

OK, then that makes sense. Maybe add this detail to the comment about why num_channels is used.

Ok. Will update comment.

Quoted 34 lines

Quoted 23 lines

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +2 -0

+		/*
+		 * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it

matches

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 138

Quoted 1 line

drivers/pwm/pwm-tegra.c +1 -0

+		 * with the hieghest applicable rate that the controller can

s/hieghest/highest/

Got it.

Quoted 11 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 146

Quoted 9 lines

drivers/pwm/pwm-tegra.c +8 -0

+		 * provide. Any further lower value can be derived by setting
+		 * PFM bits[0:12].
+		 * Higher mark is taken since BPMP has round-up mechanism
+		 * implemented.
+		 */
+		required_clk_rate =
+			(NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
+

Should be we checking the rate against the max rate supported?

Quoted 3 lines

If the request rate is beyond max supported rate, then the clk_set_rate will be failing and can get caught with error check followed by. Otherwise it will fail through fitting in the register's frequency

divider filed. So I think it is not required to check against max rate.

Quoted 1 line

Please advise if I am not able to follow with what you are suggesting.

I think that it would be better to update the cached value so that it is not incorrectly used else where by any future change. Furthermore, this simplifies matters a bit because you can do the following for all devices, but only update the clk_rate for those you wish to ...

Quoted 2 lines

    rate = pc->clk_rate >> PWM_DUTY_WIDTH;

What I understood from above is, we will always use max rate for any further configurations. If this is the suggestion above, then I think its not the right way.

If we consider only max rate then the pwm output can only be ranging from:
Possible max output rate: rate
Possible min output rate: rate/2^13 (13 bits frequency divisor)

But if we consider the min rate supported by the source clock then, min output rate can go beyond the current min possible and that should be considered for finding actual limit of min output rate.

Based on this, in the driver it tries to find a suitable clock rate to achieve requested output rate.
Please suggest if you think we can still improve this further.