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

[Thierry Reding <thierry.reding@gmail.com>]

[-- 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 --]