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

[Sandipan Patra]

Hello Uwe,

Gentle reminder to review my replies.
It will help me to push next cleaner patch.


Thanks & Regards,
Sandipan

> -----Original Message-----
> From: Sandipan Patra
> Sent: Thursday, May 7, 2020 1:40 PM
> To: Uwe Kleine-KÃnig <u.kleine-koenig@xxxxxxxxxxxxxx>
> 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
> 
> 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/ |