On 22/05/2020 12:01, Sandipan Patra wrote:
> Thanks Jonathan,
> Please help reviewing further with my replies inline.
>
>
> Thanks & Regards,
> Sandipan
>
>> -----Original Message-----
>> From: Jonathan Hunter <jonathanh@xxxxxxxxxx>
>> Sent: Friday, May 22, 2020 3:54 PM
>> To: Sandipan Patra <spatra@xxxxxxxxxx>; Thierry Reding
>> <treding@xxxxxxxxxx>; robh+dt@xxxxxxxxxx; u.kleine-koenig@xxxxxxxxxxxxxx
>> Cc: Bibek Basu <bbasu@xxxxxxxxxx>; Laxman Dewangan
>> <ldewangan@xxxxxxxxxx>; linux-pwm@xxxxxxxxxxxxxxx;
>> devicetree@xxxxxxxxxxxxxxx; linux-tegra@xxxxxxxxxxxxxxx; linux-
>> kernel@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM
>> driver
>>
>>
>> On 20/04/2020 16:54, Sandipan Patra wrote:
>>> 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 <spatra@xxxxxxxxxx>
>>> ---
>>> 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(-)
>>>
>>> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c index
>>> d26ed8f..7a36325 100644
>>> --- a/drivers/pwm/pwm-tegra.c
>>> +++ b/drivers/pwm/pwm-tegra.c
>>> @@ -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
>>> <s.hauer@xxxxxxxxxxxxxx>
>>> + * 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;
>>>
>>> @@ -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 */
>>
>> 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.
I was just wondering if was necessary to change 'c' to 'p_width'. This
could reduce the diff a bit.
>>
>>> 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
>>> + */
>>> + 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?
>
> 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.
>
>>
>>> + }
>>>
>>> /*
>>> * 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.
OK, then that makes sense. Maybe add this detail to the comment about
why num_channels is used.
>>
>>> + /*
>>> + * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it
>> matches
>>> + * with the hieghest applicable rate that the controller can
>>
>> s/hieghest/highest/
>
> Got it.
>
>>
>>> + * 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.
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;
>>
>>> + 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.
OK sounds fine.
>>
>>> + } 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 <spatra@xxxxxxxxxx>");
>>> MODULE_ALIAS("platform:tegra-pwm");
>>>
>>
>> --
>> nvpublic
--
nvpublic
