pwm: tegra: dynamic clk freq configuration by PWM driver

PATCH Applied Sandipan Patra 4 participants 5 messages Jun 1, 2020 – Jun 8, 2020

Subsystems (3)

Tegra architecture support PWM Tegra PWM driver

lore

Share & tools

lore

Sandipan Patra

author

2020-06-01 05:20 UTC #0 lore raw

PATCH v4 1/1 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]>

---
PATCH V4:
1. Code comments fixes

PATCH V3:
1. Return -EINVAL if requested period does not fall inside limit.
2. Store the new clock rate for further references.
3. Variable name change reverted.
4. Comments corrected and new comments are added.

PATCH V2:
1. Maximum frequency calculation is moved to probe.
2. Added descriptions for PWM register bits and functional behavior of the controller when new configuration is applied.
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 | 80 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 76 insertions(+), 4 deletions(-)

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

index d26ed8f..1daf591 100644
@@ -4,8 +4,36 @@
  *
  * Tegra pulse-width-modulation controller driver
  *
- * Copyright (c) 2010, NVIDIA Corporation.
+ * Copyright (c) 2010-2020, NVIDIA Corporation.
  * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer <[email protected]>
+ *
+ * Overview of Tegra Pulse Width Modulator Register:
+ * 1. 13-bit: Frequency division (SCALE)
+ * 2. 8-bit : Pulse 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.
+ * e.g. if source clock rate is 408 MHz, maximum output frequency can 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:
+ * -	When PWM is disabled, the output is driven to inactive.
+ * -	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 complete the currently running period.
+ *
+ * -	If the user input duty is beyond acceptible limits,
+ *	-EINVAL is returned.
  */
 
 #include <linux/clk.h>

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

@@ -68,7 +97,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
 	unsigned long long c = duty_ns, hz;
-	unsigned long rate;
+	unsigned long rate, required_clk_rate;
 	u32 val = 0;
 	int err;

@@ -83,9 +112,47 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	val = (u32)c << PWM_DUTY_SHIFT;
 
 	/*
+	 *  min period = max clock limit >> PWM_DUTY_WIDTH
+	 */
+	if (period_ns < pc->min_period_ns)
+		return -EINVAL;
+
+	/*
 	 * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
 	 * cycles at the PWM clock rate will take period_ns nanoseconds.
+	 *
+	 * num_channels: If single instance of PWM controller has multiple
+	 * channels (e.g. Tegra210 or older) then it is not possible to
+	 * configure separate clock rates to each of the channels, in such
+	 * case the value stored during probe will be referred.
+	 *
+	 * If every PWM controller instance has one channel respectively, i.e.
+	 * nums_channels == 1 then only the clock rate can be modified
+	 * dynamically (e.g. Tegra186 or Tegra194).
 	 */
+	if (pc->soc->num_channels == 1) {
+		/*
+		 * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it matches
+		 * with the maximum possible rate that the controller can
+		 * provide. Any further lower value can be derived by setting
+		 * PFM bits[0:12].
+		 *
+		 * required_clk_rate is a reference rate for source clock and
+		 * it is derived based on user requested period. By setting the
+		 * source clock rate as required_clk_rate, PWM controller will
+		 * be able to configure the requested period.
+		 */
+		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;
+
+		/* Store the new rate for further references */
+		pc->clk_rate = clk_get_rate(pc->clk);
+	}
+
 	rate = pc->clk_rate >> PWM_DUTY_WIDTH;
 
 	/* Consider precision in PWM_SCALE_WIDTH rate calculation */

@@ -94,7 +161,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	/*
 	 * 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)

@@ -205,6 +272,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);

@@ -312,5 +383,6 @@ static struct platform_driver tegra_pwm_driver = {
 module_platform_driver(tegra_pwm_driver);
 
 MODULE_LICENSE("GPL");
-MODULE_AUTHOR("NVIDIA Corporation");
+MODULE_AUTHOR("Sandipan Patra <[email protected]>");
+MODULE_DESCRIPTION("Tegra PWM controller driver");
 MODULE_ALIAS("platform:tegra-pwm");

Signature · 1 line

2.7.4

Jon Hunter

maintainer Reviewed-by on v4 1/1

2020-06-01 12:06 UTC #1 lore raw

Quoted 173 lines · Sandipan Patra

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.

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

---
PATCH V4:
1. Code comments fixes

 drivers/pwm/pwm-tegra.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 76 insertions(+), 4 deletions(-)

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

index d26ed8f..1daf591 100644
@@ -4,8 +4,36 @@
  *
  * Tegra pulse-width-modulation controller driver
  *
- * Copyright (c) 2010, NVIDIA Corporation.
+ * Copyright (c) 2010-2020, NVIDIA Corporation.
  * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer <[email protected]>
+ *
+ * Overview of Tegra Pulse Width Modulator Register:
+ * 1. 13-bit: Frequency division (SCALE)
+ * 2. 8-bit : Pulse 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.
+ * e.g. if source clock rate is 408 MHz, maximum output frequency can 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:
+ * -	When PWM is disabled, the output is driven to inactive.
+ * -	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 complete the currently running period.
+ *
+ * -	If the user input duty is beyond acceptible limits,
+ *	-EINVAL is returned.
  */
 
 #include <linux/clk.h>

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

@@ -68,7 +97,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
 	unsigned long long c = duty_ns, hz;
-	unsigned long rate;
+	unsigned long rate, required_clk_rate;
 	u32 val = 0;
 	int err;

@@ -83,9 +112,47 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	val = (u32)c << PWM_DUTY_SHIFT;
 
 	/*
+	 *  min period = max clock limit >> PWM_DUTY_WIDTH
+	 */
+	if (period_ns < pc->min_period_ns)
+		return -EINVAL;
+
+	/*
 	 * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
 	 * cycles at the PWM clock rate will take period_ns nanoseconds.
+	 *
+	 * num_channels: If single instance of PWM controller has multiple
+	 * channels (e.g. Tegra210 or older) then it is not possible to
+	 * configure separate clock rates to each of the channels, in such
+	 * case the value stored during probe will be referred.
+	 *
+	 * If every PWM controller instance has one channel respectively, i.e.
+	 * nums_channels == 1 then only the clock rate can be modified
+	 * dynamically (e.g. Tegra186 or Tegra194).
 	 */
+	if (pc->soc->num_channels == 1) {
+		/*
+		 * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it matches
+		 * with the maximum possible rate that the controller can
+		 * provide. Any further lower value can be derived by setting
+		 * PFM bits[0:12].
+		 *
+		 * required_clk_rate is a reference rate for source clock and
+		 * it is derived based on user requested period. By setting the
+		 * source clock rate as required_clk_rate, PWM controller will
+		 * be able to configure the requested period.
+		 */
+		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;
+
+		/* Store the new rate for further references */
+		pc->clk_rate = clk_get_rate(pc->clk);
+	}
+
 	rate = pc->clk_rate >> PWM_DUTY_WIDTH;
 
 	/* Consider precision in PWM_SCALE_WIDTH rate calculation */

@@ -94,7 +161,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	/*
 	 * 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)

@@ -205,6 +272,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);

drivers/pwm/pwm-tegra.c static struct platform_driver tegra_pwm_driver = { · line 388

Quoted 7 lines

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

@@ -312,5 +383,6 @@ static struct platform_driver tegra_pwm_driver = {
 module_platform_driver(tegra_pwm_driver);
 
 MODULE_LICENSE("GPL");
-MODULE_AUTHOR("NVIDIA Corporation");
+MODULE_AUTHOR("Sandipan Patra <[email protected]>");
+MODULE_DESCRIPTION("Tegra PWM controller driver");
 MODULE_ALIAS("platform:tegra-pwm");

Thanks. LGTM.

Reviewed-by Jon Hunter <[email protected]>

Cheers
Jon

Signature · 1 line

nvpublic

Thierry Reding

Applied

2020-06-02 12:47 UTC #2 lore raw

In reply to Sandipan Patra

Quoted 41 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]>
---
PATCH V4:
1. Code comments fixes
PATCH V3:
1. Return -EINVAL if requested period does not fall inside limit.
2. Store the new clock rate for further references.
3. Variable name change reverted.
4. Comments corrected and new comments are added.
PATCH V2:
1. Maximum frequency calculation is moved to probe.
2. Added descriptions for PWM register bits and functional behavior of the controller when new configuration is applied.
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

Quoted 2 lines

 drivers/pwm/pwm-tegra.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 76 insertions(+), 4 deletions(-)

Applied, thanks.

Thierry

Uwe Kleine-König

2020-06-03 16:29 UTC #3 lore raw

In reply to Sandipan Patra

Hello,

Quoted 24 lines · Sandipan Patra

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

index d26ed8f..1daf591 100644
@@ -4,8 +4,36 @@
  *
  * Tegra pulse-width-modulation controller driver
  *
- * Copyright (c) 2010, NVIDIA Corporation.
+ * Copyright (c) 2010-2020, NVIDIA Corporation.
  * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer <[email protected]>
+ *
+ * Overview of Tegra Pulse Width Modulator Register:
+ * 1. 13-bit: Frequency division (SCALE)
+ * 2. 8-bit : Pulse 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.
+ * e.g. if source clock rate is 408 MHz, maximum output frequency can be:
+ * 408 MHz/256 = 1.6 MHz.
+ * This 1.6 MHz frequency can further be divided using SCALE value in PWM.

drivers/pwm/pwm-tegra.c line 31

Quoted 10 lines

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

+ *
+ * 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:
+ * -	When PWM is disabled, the output is driven to inactive.
+ * -	It does not allow the current PWM period to complete and
+ *	stops abruptly.
+ *

I'd prefer to have no empty lines in the in Limitations paragraph to be able to get all infos using something like:

	sed -rn '/\* Limitations:/,/^ \*\/?$/p' drivers/pwm/pwm-tegra.c

drivers/pwm/pwm-tegra.c line 36

Quoted 5 lines

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

+ * -	If the register is reconfigured while PWM is running,
+ *	it does not complete the currently running period.
+ *
+ * -	If the user input duty is beyond acceptible limits,
+ *	-EINVAL is returned.

s/acceptible/acceptable/ (but in fact this isn't a limitation, so I'd drop this here, as pointed out in v2).

In v2 I mentioned a few things to add here.

Quoted 12 lines

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

  */
 
 #include <linux/clk.h>

@@ -41,6 +69,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 static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 100

Quoted 5 lines

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

@@ -68,7 +97,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
 	unsigned long long c = duty_ns, hz;
-	unsigned long rate;
+	unsigned long rate, required_clk_rate;

In v2 I requested to move this into the if block below. You replied to want to move it accordingly.

Quoted 21 lines

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

 	u32 val = 0;
 	int err;

@@ -83,9 +112,47 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	val = (u32)c << PWM_DUTY_SHIFT;
 
 	/*
+	 *  min period = max clock limit >> PWM_DUTY_WIDTH
+	 */
+	if (period_ns < pc->min_period_ns)
+		return -EINVAL;
+
+	/*
 	 * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
 	 * cycles at the PWM clock rate will take period_ns nanoseconds.
+	 *
+	 * num_channels: If single instance of PWM controller has multiple
+	 * channels (e.g. Tegra210 or older) then it is not possible to
+	 * configure separate clock rates to each of the channels, in such
+	 * case the value stored during probe will be referred.
+	 *

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

Quoted 10 lines

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

+	 * If every PWM controller instance has one channel respectively, i.e.
+	 * nums_channels == 1 then only the clock rate can be modified
+	 * dynamically (e.g. Tegra186 or Tegra194).
 	 */
+	if (pc->soc->num_channels == 1) {
+		/*
+		 * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it matches
+		 * with the maximum possible rate that the controller can
+		 * provide. Any further lower value can be derived by setting
+		 * PFM bits[0:12].

It looks a bit strange that the algorithm to calculate the clock settings depends on the number of channels. Looks like a wrong abstraction.

Quoted 21 lines

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

+		 *
+		 * required_clk_rate is a reference rate for source clock and
+		 * it is derived based on user requested period. By setting the
+		 * source clock rate as required_clk_rate, PWM controller will
+		 * be able to configure the requested period.
+		 */
+		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;
+
+		/* Store the new rate for further references */
+		pc->clk_rate = clk_get_rate(pc->clk);
+	}
+
 	rate = pc->clk_rate >> PWM_DUTY_WIDTH;
 
 	/* Consider precision in PWM_SCALE_WIDTH rate calculation */

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

Quoted 5 lines

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

@@ -94,7 +161,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	/*
 	 * 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

I would have put this in a separate change.

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

Quoted 10 lines

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

 	 * write to the register.
 	 */
 	if (rate > 0)

@@ -205,6 +272,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;

To ensure that required_clk_rate in tegra_pwm_config doesn't get bigger than pwm->soc->max_frequency this isn't the right formula I think. I'd use

	pwm->min_period_ns = DIV_ROUNDUP(NSEC_PER_SEC, pwm->soc->max_frequency >> PWM_DUTY_WIDTH);

. Can you confirm?

Best regards
Uwe

Sandipan Patra

author

2020-06-08 12:36 UTC #4 lore raw

Hi Uwe,

Quoted 56 lines

-----Original Message-----
From: Uwe Kleine-König <[email protected]>
Sent: Wednesday, June 3, 2020 9:59 PM
To: Sandipan Patra <[email protected]>
Cc: Thierry Reding <[email protected]>; Jonathan Hunter <[email protected]>; Bibek Basu <[email protected]>; Laxman Dewangan <[email protected]>; Krishna Yarlagadda <[email protected]>; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [PATCH V4] pwm: tegra: dynamic clk freq configuration by PWM driver
Hello,
Quoted 29 lines · Sandipan Patra
drivers/pwm/pwm-tegra.c index renamed from drivers/pwm/pwm-tegra.c +0 -0
d26ed8f..1daf591 100644
drivers/pwm/pwm-tegra.c +1 -1
@@ -4,8 +4,36 @@
  *
  * Tegra pulse-width-modulation controller driver
  *
- * Copyright (c) 2010, NVIDIA Corporation.
+ * Copyright (c) 2010-2020, NVIDIA Corporation.
  * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer
<[email protected]>
+ *
+ * Overview of Tegra Pulse Width Modulator Register:
+ * 1. 13-bit: Frequency division (SCALE)
+ * 2. 8-bit : Pulse 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.
+ * e.g. if source clock rate is 408 MHz, maximum output frequency can 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.
Quoted 8 lines
+ * 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:
+ * - When PWM is disabled, the output is driven to inactive.
+ * - It does not allow the current PWM period to complete and
+ * stops abruptly.
+ *
I'd prefer to have no empty lines in the in Limitations paragraph to be able to get all infos using something like:

Quoted 2 lines

	sed -rn '/\* Limitations:/,/^ \*\/?$/p' drivers/pwm/pwm-tegra.c

I'll push a change for this.

Quoted 10 lines

Quoted 5 lines
+ * - If the register is reconfigured while PWM is running,
+ * it does not complete the currently running period.
+ *
+ * - If the user input duty is beyond acceptible limits,
+ * -EINVAL is returned.
s/acceptible/acceptable/ (but in fact this isn't a limitation, so I'd drop this here, as pointed out in v2).
In v2 I mentioned a few things to add here.

Ok. Understood. I will make sure to follow this in new change.

Instead of this
- If the user input duty is beyond acceptible limits,
-EINVAL is returned.
I will add below info now and follow up on the atomic API implementation eventually.
- The driver doesn't implement the right rounding rules
- The driver needs updating to the atomic API

Quoted 12 lines

#include <linux/clk.h>

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

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

Quoted 9 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, · line 100
Quoted 6 lines
drivers/pwm/pwm-tegra.c +1 -1
@@ -68,7 +97,7 @@ static int tegra_pwm_config(struct pwm_chip *chip,
 struct pwm_device *pwm,  {
 	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
 	unsigned long long c = duty_ns, hz;
-	unsigned long rate;
+	unsigned long rate, required_clk_rate;
In v2 I requested to move this into the if block below. You replied to want to move it accordingly.

This got skipped. My mistake. I will push a change to move the definition of required_clk_rate inside if-block as a new change.

Quoted 31 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;

@@ -83,9 +112,47 @@ static int tegra_pwm_config(struct pwm_chip *chip,

struct pwm_device *pwm,

Quoted 14 lines

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

 	val = (u32)c << PWM_DUTY_SHIFT;
 
 	/*
+	 *  min period = max clock limit >> PWM_DUTY_WIDTH
+	 */
+	if (period_ns < pc->min_period_ns)
+		return -EINVAL;
+
+	/*
 	 * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
 	 * cycles at the PWM clock rate will take period_ns nanoseconds.
+	 *
+	 * num_channels: If single instance of PWM controller has multiple
+	 * channels (e.g. Tegra210 or older) then it is not possible to

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 +9 -0

+	 * configure separate clock rates to each of the channels, in such
+	 * case the value stored during probe will be referred.
+	 *
+	 * If every PWM controller instance has one channel respectively, i.e.
+	 * nums_channels == 1 then only the clock rate can be modified
+	 * dynamically (e.g. Tegra186 or Tegra194).
 	 */
+	if (pc->soc->num_channels == 1) {
+		/*
+		 * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it

matches

Quoted 6 lines

drivers/pwm/pwm-tegra.c static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 138
Quoted 3 lines
drivers/pwm/pwm-tegra.c +3 -0
+		 * with the maximum possible rate that the controller can
+		 * provide. Any further lower value can be derived by setting
+		 * PFM bits[0:12].
It looks a bit strange that the algorithm to calculate the clock settings depends on the number of channels. Looks like a wrong abstraction.

The calculation is added based on number of channels to support legacy HW. When the PWM controller has multiple channels and all of them are sourced by single clock, then dynamic clock configuration can not be supported. So the changes are allowed only when PWM controller has one channel.

Quoted 45 lines

Quoted 24 lines
drivers/pwm/pwm-tegra.c +17 -1
+		 *
+		 * required_clk_rate is a reference rate for source clock and
+		 * it is derived based on user requested period. By setting the
+		 * source clock rate as required_clk_rate, PWM controller will
+		 * be able to configure the requested period.
+		 */
+		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;
+
+		/* Store the new rate for further references */
+		pc->clk_rate = clk_get_rate(pc->clk);
+	}
+
 	rate = pc->clk_rate >> PWM_DUTY_WIDTH;
 
 	/* Consider precision in PWM_SCALE_WIDTH rate calculation */ @@
-94,7 +161,7 @@ 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 164
Quoted 4 lines
drivers/pwm/pwm-tegra.c +1 -1
 	/*
 	 * 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
I would have put this in a separate change.
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device
Quoted 4 lines
drivers/pwm/pwm-tegra.c +0 -0
 	 * write to the register.
 	 */
 	if (rate > 0)
@@ -205,6 +272,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 static int tegra_pwm_probe(struct platform_device · line 278
Quoted 1 line
drivers/pwm/pwm-tegra.c +1 -0
+1;

Quoted 7 lines

To ensure that required_clk_rate in tegra_pwm_config doesn't get bigger than pwm->soc->max_frequency this isn't the right formula I think. I'd use
	pwm->min_period_ns = DIV_ROUNDUP(NSEC_PER_SEC, pwm->soc-
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device · line 279
Quoted 1 line
drivers/pwm/pwm-tegra.c +0 -0
 max_frequency >> PWM_DUTY_WIDTH);
. Can you confirm?

As per formula, "+1" is required in the calculation.
Hence to reflect this in the code, manual division is done which by default ignores the fractional part of the integer division. And then +1 is added.

Your suggestion to use DIV_ROUNDUP is for a crisp calculation.
But it mandates to ignore "+1" part as the division calculation is taken care by considering celling value. When the division yields quotient with 0 fraction, then +1 is not considered.

In this patch, raw calculation is kept for a better readability, which can also help understanding about the accurate derivation of min_period_ns as per specification.

Please help me understanding if I am not aligned with your suggestion and if is there a better case where DIV_ROUNDUP is required in this case.

Thanks & Regards,
Sandipan

Quoted 3 lines

Best regards
Uwe