RE: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM driver

[Sandipan Patra]

Hello,

> -----Original Message-----
> From: Uwe Kleine-KÃnig <u.kleine-koenig@xxxxxxxxxxxxxx>
> Sent: Tuesday, May 5, 2020 1:42 AM
> To: Sandipan Patra <spatra@xxxxxxxxxx>
> Cc: Thierry Reding <treding@xxxxxxxxxx>; robh+dt@xxxxxxxxxx; Jonathan
> Hunter <jonathanh@xxxxxxxxxx>; 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
> 
> External email: Use caution opening links or attachments
> 
> 
> Hello,
> 
> On Mon, Apr 20, 2020 at 09:24:03PM +0530, 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:
> 
> s/i.e./e.g./, s/cab/can/

Noted, correction in next patch.

> 
> > + * 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.

> 
> > + * - 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".

> 
> > + * - 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.

> 
> > + *   It does not let the currently running period to complete.
> 
> s/It/it/; s/let/complete/; s/ to complete//
>

Noted, correction in next patch
 
> > + *
> > + * - 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.
 
> 
> > + *   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.

> 
> > + * - 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
> 
> >   */
> >
> >  #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;
> > +
> > +     /*
> > +      *  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);
> 
> 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.

> 
> > +     }
> >
> >       /*
> >        * 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.
> 

Noted, will define here to limit its scope.

> > +             /*
> > +              * 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

> 
> > +              * 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.

> 
> > +              */
> > +             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.
 
> 
> > +
> > +             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.
 
> 
> > +              *
> > +              * 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;
> 
> 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.
 
> 
> > +
> >       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");
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-KÃnig            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |