linux-kernel - Re: [PATCH 2/2] pwm: Add clock based PWM output driver

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] [day] [month] [year] [list]
Message-ID: <20211211163314.azn562syhybmjk6q@pengutronix.de>
Date:   Sat, 11 Dec 2021 17:33:14 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Nikita Travkin <nikita@...n.ru>
Cc:     linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
        sboyd@...nel.org, linus.walleij@...aro.org,
        linux-kernel@...r.kernel.org, robh+dt@...nel.org,
        thierry.reding@...il.com, ~postmarketos/upstreaming@...ts.sr.ht,
        kernel@...gutronix.de, pza@...gutronix.de, lee.jones@...aro.org,
        masneyb@...tation.org
Subject: Re: [PATCH 2/2] pwm: Add clock based PWM output driver

Hello,

On Fri, Dec 10, 2021 at 06:13:34PM +0500, Nikita Travkin wrote:
> Uwe Kleine-König писал(а) 10.12.2021 03:05:
> > Hello,
> > 
> > On Thu, Dec 09, 2021 at 09:20:20PM +0500, Nikita Travkin wrote:
> >> +#define to_pwm_clk_chip(_chip) container_of(_chip, struct pwm_clk_chip, chip)
> >> +
> >> +static int pwm_clk_apply(struct pwm_chip *pwm_chip, struct pwm_device *pwm,
> >> +			 const struct pwm_state *state)
> >> +{
> >> +	struct pwm_clk_chip *chip = to_pwm_clk_chip(pwm_chip);
> >> +	int ret;
> >> +	u32 rate;
> >> +
> >> +	if (!state->enabled && !pwm->state.enabled)
> >> +		return 0;
> >> +
> >> +	if (state->enabled && !pwm->state.enabled) {
> >> +		ret = clk_enable(chip->clk);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	if (!state->enabled && pwm->state.enabled) {
> >> +		clk_disable(chip->clk);
> >> +		return 0;
> >> +	}
> > 
> > This can be written a bit more compact as:
> > 
> > 	if (!state->enabled) {
> > 		if (pwm->state.enabled)
> > 			clk_disable(chip->clk);
> > 		return 0;
> > 	} else if (!pwm->state.enabled) {
> > 		ret = clk_enable(chip->clk);
> > 		if (ret)
> > 			return ret;
> > 	}
> > 
> > personally I find my version also easier to read, but that might be
> > subjective.
> 
> Having three discrete checks for three possible outcomes is a bit
> easier for me to understand, but I have no preference and can change
> it to your version. 
> 
> > Missing handling for polarity. Either refuse inverted polarity, or set
> > the duty_cycle to state->period - state->duty_cycle in the inverted
> > case.
> 
> Will add the latter.
> 
> > 
> >> +	rate = div64_u64(NSEC_PER_SEC, state->period);
> > 
> > Please round up here, as .apply() should never implement a period bigger
> > than requested. This also automatically improves the behaviour if
> > state->period > NSEC_PER_SEC.
> 
> Will do. I'm not sure if the underlying clock drivers guarantee the
> chosen rate to be rounded in line with that however, e.g. qcom SoC

This is a problem that most drivers have. Very few use clk_set_rate, but
also clk_get_rate is subject to (much smaller) rounding issues.
There doesn't seem to be an agreement that this is important enough to
address.

> clocks that I target use lookup tables to find the closest rate
> with known M/N config values and set that. (So unless one makes sure
> the table has all the required rates, period is not guaranteed.)
> 
> This is not an issue for my use cases though: Don't think any
> of the led or haptic motor controllers I've seen in my devices
> need a perfect rate.
> 
> I think this is another line into the "Limitations" description
> that was suggested later.
> 
> > 
> >> +	ret = clk_set_rate(chip->clk, rate);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	return clk_set_duty_cycle(chip->clk, state->duty_cycle, state->period);
> > 
> > Is it possible to enable only after the duty cycle is set? This way we
> > could prevent in some cases that a wrong setting makes it to the output.
> > 
> 
> Will move clk_enable() as the last action. After that it makes more
> sense to "squash" two leftover checks for disabled state so will do
> that as well.
> 
> ... Except moving it caused a nice big WARNING from my clock controller...
> 
> [   76.353557] gcc_camss_gp1_clk status stuck at 'off'
> [   76.353593] WARNING: CPU: 2 PID: 97 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x144/0x160
> (...)
> [   76.571531] Call trace:
> [   76.578644]  clk_branch_wait+0x144/0x160
> [   76.580903]  clk_branch2_enable+0x34/0x44
> [   76.585069]  clk_core_enable+0x6c/0xc0
> [   76.588974]  clk_enable+0x30/0x50
> [   76.592620]  pwm_clk_apply+0xb0/0xe4
> [   76.596007]  pwm_apply_state+0x6c/0x1ec
> [   76.599651]  sgm3140_brightness_set+0xb4/0x190 [leds_sgm3140]
> 
> (Which doesn't stop it from working afaict, but very much undesirable
> for me.)
> Unsure if this is something common or just a quirk of this specific
> driver but I'd rather take a little glitch on the output than
> make clock driver unhappy knowing how picky this hardware sometimes is...

I think clk_set_rate for a disabled clk isn't well defined.

Best regards
Uwe

-- 
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)