[PATCH 1/2] pwm: tegra: Read PWM clock source rate in driver init

[PATCH 1/2] pwm: tegra: Read PWM clock source rate in driver init

[Laxman Dewangan]

It is required to know the PWM clock source frequency to
calculate the PWM period.

In driver, the clock source frequency of the PWM does not
get change and, hence, get the clock source frequency in
driver initi. Get this values later for period calculation
from pwm_config().

This will help in avoiding the clock call for getting clock rate
in the pwm_config() each time.

Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/pwm/pwm-tegra.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index c040f87..8c6ed55 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -50,6 +50,8 @@ struct tegra_pwm_chip {
 	struct clk *clk;
 	struct reset_control*rst;
 
+	unsigned long clk_rate;
+
 	void __iomem *regs;
 
 	const struct tegra_pwm_soc *soc;
@@ -94,7 +96,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
 	 * cycles at the PWM clock rate will take period_ns nanoseconds.
 	 */
-	rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
+	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);
@@ -199,6 +201,9 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(pwm->clk))
 		return PTR_ERR(pwm->clk);
 
+	/* Read PWM clock rate from source */
+	pwm->clk_rate = clk_get_rate(pwm->clk);
+
 	pwm->rst = devm_reset_control_get(&pdev->dev, "pwm");
 	if (IS_ERR(pwm->rst)) {
 		ret = PTR_ERR(pwm->rst);
-- 
2.1.4

[PATCH 2/2] pwm: tegra: Set maximum pwm clock source per SoC tapeout

[Laxman Dewangan]

The PWM hardware IP is taped-out with different maximum frequency
on different SoCs.

From HW team:
	For Tegra210, it is 38.4MHz.
	For Tegra186, it is 102MHz.

Add support to limit the clock source frequency to the maximum IP
supported frequency. Provide these values via SoC chipdata.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/pwm/pwm-tegra.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 8c6ed55..7016c08 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -41,6 +41,9 @@
 
 struct tegra_pwm_soc {
 	unsigned int num_channels;
+
+	/* Maximum IP frequency for given SoCs */
+	unsigned long max_frequency;
 };
 
 struct tegra_pwm_chip {
@@ -204,6 +207,24 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 	/* Read PWM clock rate from source */
 	pwm->clk_rate = clk_get_rate(pwm->clk);
 
+	/* Make sure clock source freqeuncy must less than IP supported */
+	if (pwm->soc->max_frequency &&
+	    (pwm->soc->max_frequency < pwm->clk_rate)) {
+		ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "Failed to set max frequency: %d\n",
+				ret);
+			return ret;
+		}
+
+		/*
+		 * The requested and configured frequency may differ due to
+		 * clock register resolutions. Get the configured frequency
+		 * so that PWM period can be calculated more accurately.
+		 */
+		 pwm->clk_rate = clk_get_rate(pwm->clk);
+	}
+
 	pwm->rst = devm_reset_control_get(&pdev->dev, "pwm");
 	if (IS_ERR(pwm->rst)) {
 		ret = PTR_ERR(pwm->rst);
@@ -275,12 +296,19 @@ static const struct tegra_pwm_soc tegra20_pwm_soc = {
 	.num_channels = 4,
 };
 
+static const struct tegra_pwm_soc tegra210_pwm_soc = {
+	.num_channels = 4,
+	.max_frequency = 38400000UL, /* 38.4MHz */
+};
+
 static const struct tegra_pwm_soc tegra186_pwm_soc = {
 	.num_channels = 1,
+	.max_frequency = 102000000UL, /* 102MHz */
 };
 
 static const struct of_device_id tegra_pwm_of_match[] = {
 	{ .compatible = "nvidia,tegra20-pwm", .data = &tegra20_pwm_soc },
+	{ .compatible = "nvidia,tegra210-pwm", .data = &tegra210_pwm_soc },
 	{ .compatible = "nvidia,tegra186-pwm", .data = &tegra186_pwm_soc },
 	{ }
 };
-- 
2.1.4

Re: [PATCH 2/2] pwm: tegra: Set maximum pwm clock source per SoC tapeout

[Thierry Reding]

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

On Thu, Apr 13, 2017 at 07:40:28PM +0530, Laxman Dewangan wrote:
> The PWM hardware IP is taped-out with different maximum frequency
> on different SoCs.
> 
> From HW team:
> 	For Tegra210, it is 38.4MHz.
> 	For Tegra186, it is 102MHz.
> 
> Add support to limit the clock source frequency to the maximum IP
> supported frequency. Provide these values via SoC chipdata.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/pwm/pwm-tegra.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 8c6ed55..7016c08 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -41,6 +41,9 @@
>  
>  struct tegra_pwm_soc {
>  	unsigned int num_channels;
> +
> +	/* Maximum IP frequency for given SoCs */
> +	unsigned long max_frequency;
>  };
>  
>  struct tegra_pwm_chip {
> @@ -204,6 +207,24 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	/* Read PWM clock rate from source */
>  	pwm->clk_rate = clk_get_rate(pwm->clk);
>  
> +	/* Make sure clock source freqeuncy must less than IP supported */
> +	if (pwm->soc->max_frequency &&
> +	    (pwm->soc->max_frequency < pwm->clk_rate)) {
> +		ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "Failed to set max frequency: %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		/*
> +		 * The requested and configured frequency may differ due to
> +		 * clock register resolutions. Get the configured frequency
> +		 * so that PWM period can be calculated more accurately.
> +		 */
> +		 pwm->clk_rate = clk_get_rate(pwm->clk);
> +	}

Is there a reason to conditionalize this? Couldn't we simply set the
clock to the maximum frequency in all cases? Higher frequency means
higher precision, right? So just something like this perhaps:

	ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
	if (ret < 0) {
		...
	}

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

That of course means that we'd need to define a maximum frequency for
SoCs prior to Tegra210. Any chance we can get at them?

> +
>  	pwm->rst = devm_reset_control_get(&pdev->dev, "pwm");
>  	if (IS_ERR(pwm->rst)) {
>  		ret = PTR_ERR(pwm->rst);
> @@ -275,12 +296,19 @@ static const struct tegra_pwm_soc tegra20_pwm_soc = {
>  	.num_channels = 4,
>  };
>  
> +static const struct tegra_pwm_soc tegra210_pwm_soc = {
> +	.num_channels = 4,
> +	.max_frequency = 38400000UL, /* 38.4MHz */
> +};
> +
>  static const struct tegra_pwm_soc tegra186_pwm_soc = {
>  	.num_channels = 1,
> +	.max_frequency = 102000000UL, /* 102MHz */

I don't think we need these comments, it's fairly obvious what
frequencies you're specifying there. =)

Thierry

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

Re: [PATCH 2/2] pwm: tegra: Set maximum pwm clock source per SoC tapeout

[Laxman Dewangan]

On Thursday 13 April 2017 08:57 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Apr 13, 2017 at 07:40:28PM +0530, Laxman Dewangan wrote:
>> The PWM hardware IP is taped-out with different maximum frequency
>> on different SoCs.
>>
>>  From HW team:
>> 	For Tegra210, it is 38.4MHz.
>> 	For Tegra186, it is 102MHz.
>>
>> Add support to limit the clock source frequency to the maximum IP
>> supported frequency. Provide these values via SoC chipdata.
>>
>> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>   drivers/pwm/pwm-tegra.c | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
>> index 8c6ed55..7016c08 100644
>> --- a/drivers/pwm/pwm-tegra.c
>> +++ b/drivers/pwm/pwm-tegra.c
>> @@ -41,6 +41,9 @@
>>   
>>   struct tegra_pwm_soc {
>>   	unsigned int num_channels;
>> +
>> +	/* Maximum IP frequency for given SoCs */
>> +	unsigned long max_frequency;
>>   };
>>   
>>   struct tegra_pwm_chip {
>> @@ -204,6 +207,24 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>>   	/* Read PWM clock rate from source */
>>   	pwm->clk_rate = clk_get_rate(pwm->clk);
>>   
>> +	/* Make sure clock source freqeuncy must less than IP supported */
>> +	if (pwm->soc->max_frequency &&
>> +	    (pwm->soc->max_frequency < pwm->clk_rate)) {
>> +		ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
>> +		if (ret < 0) {
>> +			dev_err(&pdev->dev, "Failed to set max frequency: %d\n",
>> +				ret);
>> +			return ret;
>> +		}
>> +
>> +		/*
>> +		 * The requested and configured frequency may differ due to
>> +		 * clock register resolutions. Get the configured frequency
>> +		 * so that PWM period can be calculated more accurately.
>> +		 */
>> +		 pwm->clk_rate = clk_get_rate(pwm->clk);
>> +	}
> Is there a reason to conditionalize this? Couldn't we simply set the
> clock to the maximum frequency in all cases? Higher frequency means
> higher precision, right?

I think higher precision is not related directly to maximum frequency. 
Precision will much depends on the perfect multiples between period and 
clock source frequency.

If some usecases needed the perfect periods then the clock source 
frequency can be set via clock init table for clock driver to achieve 
the perfect PWM period. on this case, we should not change it in the PWM 
driver.
PWM driver should only worry about to limit the maximum.



>   So just something like this perhaps:
>
> 	ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
> 	if (ret < 0) {
> 		...
> 	}
>
> 	pwm->clk_rate = clk_get_rate(pwm->clk);
>
> That of course means that we'd need to define a maximum frequency for
> SoCs prior to Tegra210. Any chance we can get at them?


Getting information for the older SoCs are little bit difficult. Let me 
try if possible from HW team.

Otherwise, What we can do now as we can make 38.4MHz till T210 and 102M 
for T186.


>> +
>>   	pwm->rst = devm_reset_control_get(&pdev->dev, "pwm");
>>   	if (IS_ERR(pwm->rst)) {
>>   		ret = PTR_ERR(pwm->rst);
>> @@ -275,12 +296,19 @@ static const struct tegra_pwm_soc tegra20_pwm_soc = {
>>   	.num_channels = 4,
>>   };
>>   
>> +static const struct tegra_pwm_soc tegra210_pwm_soc = {
>> +	.num_channels = 4,
>> +	.max_frequency = 38400000UL, /* 38.4MHz */
>> +};
>> +
>>   static const struct tegra_pwm_soc tegra186_pwm_soc = {
>>   	.num_channels = 1,
>> +	.max_frequency = 102000000UL, /* 102MHz */
> I don't think we need these comments, it's fairly obvious what
> frequencies you're specifying there. =)
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1

Re: [PATCH 1/2] pwm: tegra: Read PWM clock source rate in driver init

[Thierry Reding]

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

On Thu, Apr 13, 2017 at 07:40:27PM +0530, Laxman Dewangan wrote:
> It is required to know the PWM clock source frequency to
> calculate the PWM period.
> 
> In driver, the clock source frequency of the PWM does not
> get change and, hence, get the clock source frequency in
> driver initi. Get this values later for period calculation
> from pwm_config().
> 
> This will help in avoiding the clock call for getting clock rate
> in the pwm_config() each time.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/pwm/pwm-tegra.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Applied, thanks.

Thierry

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