linux-kernel - Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201207081628.tm3yg7az5k5sbivu@pengutronix.de>
Date:   Mon, 7 Dec 2020 09:16:28 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Sean Young <sean@...s.org>
Cc:     Lino Sanfilippo <LinoSanfilippo@....de>, thierry.reding@...il.com,
        lee.jones@...aro.org, nsaenzjulienne@...e.de, f.fainelli@...il.com,
        rjui@...adcom.com, sbranden@...adcom.com,
        bcm-kernel-feedback-list@...adcom.com, linux-pwm@...r.kernel.org,
        linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic
 configuration

On Sun, Dec 06, 2020 at 02:19:41PM +0000, Sean Young wrote:
> Hello Uwe,
> 
> On Sat, Dec 05, 2020 at 08:25:10PM +0100, Uwe Kleine-König wrote:
> > On Sat, Dec 05, 2020 at 05:34:44PM +0000, Sean Young wrote:
> > > What real life uses-cases are there for round down? If you want to round
> > > down, is there any need for round up?
> > 
> > The scenario I have in mind is for driving a motor. I have to admit
> > however that usually the period doesn't matter much and it's the
> > duty_cycle that defines the motor's speed. So for this case the
> > conservative behaviour is round-down to not make the motor run faster
> > than expected.
> 
> I am reading here that for driving motors, only the duty cycle matters,
> not the period.

There is an upper limit (usually around 1 ms) for the period, but if you
choose 0.1 ms or 0.001 ms doesn't matter much AFAICT.

@Thierry: Do you have further use cases in mind?

> > For other usecases (fan, backlight, LED) exactness typically doesn't
> > matter that much.
> 
> So, the use-cases you have are driving motor, fan, backlight, and led.
> And in all these cases the exact Hz does not matter.
> 
> The only uses case where the exact Hz does matter is pwm-ir-tx. 
> 
> So, I gather there are no use-cases for round-down. Yes, should round-down
> be needed, then this is more difficult to implement if the driver always
> does a round-closest. But, since there is no reason to have round-down,
> this is all academic.
> 
> Your policy of forcing new pwm drivers to use round-down is breaking
> pwm-ir-tx.

So you're indeed suggesting that the "right" rounding strategy for
lowlevel drivers should be:

 - Use the period length closest to the requested period (in doubt round
   down?)
 - With the chosen period length use the biggest duty_cycle not bigger
   than the requested duty_cycle.

While this seems technically fine I think for maintenance this is a
nightmare.

My preference would be to stick to the rounding strategy we used so far
(i.e.:

 - Use the biggest period length not bigger than the requested period
 - With the chosen period length use the biggest duty_cycle not bigger
   than the requested duty_cycle.

) and for pwm-ir-tx add support to the PWM API to still make it possible
(and easy) to select the best setting.

The reasons why I think that this rounding-down strategy is the best
are (in order of importance):

 - It is easier to implement correctly [1]
 - Same rounding method for period and duty cycle
 - most drivers already do this (I think)

The (IMHO nice) result would then mean:

 - All consumers can get the setting they want; and
 - Code in lowlevel drivers is simple and the complexity is in common
   code and so a single place.

And it would also allow the pwm-ir-tx driver to notice if the PWM to be
used can for example only support frequencies under 400 kHz.

Best regards
Uwe

[1] Consider a PWM with a parent frequency of 66 MHz, to select the
    period you can pick an integer divider "div" resulting in the period
    4096 / (pclk * d). So the obvious implementation for round-nearest
    would be:

    	pclk = clk_get_rate(myclk);
	div = DIV_ROUND_CLOSEST(NSEC_PER_SEC * 4096, targetperiod * pclk);

    , right?

    With targetperiod = 2641 ns this picks div = 23 and so a period of
    2698.2872200263505 ns (delta = 57.2872200263505 ns).
    The optimal divider however is div = 24. (implemented period =
    2585.8585858585857 ns, delta = 55.14141414141448 ns)

    For round-down the correct implementation is:

    	pclk = clk_get_rate(myclk);
	div = DIV_ROUND_UP(NSEC_PER_SEC * 4096, targetperiod * pclk);

    Exercise for the reader: Come up with a correct implementation for
    "round-nearest" and compare its complexity to the round-down code.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)