Re: [PATCH v5 3/6] pwm: Add rockchip PWMv4 driver

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7615 bytes --]

Hello Nicolas,

On Mon, Apr 20, 2026 at 03:52:40PM +0200, Nicolas Frattaroli wrote:
> [...]
> +/**
> + * rockchip_pwm_v4_round_single - convert a PWM parameter to hardware
> + * @rate: clock rate of the PWM clock, as per clk_get_rate
> + *        Assumed to be <= 1GHz for overflow considerations
> + * @in_val: parameter in nanoseconds to convert
> + *
> + * Returns the rounded value, saturating at U32_MAX if too large
> + */
> +static u32 rockchip_pwm_v4_round_single(unsigned long rate, u64 in_val)

very picky: if you name this function rockchip_pwm_v4_ns_to_ticks it
becomes clearer what the purpose is.

> +{
> +	u64 tmp;
> +
> +	tmp = mul_u64_u64_div_u64(rate, in_val, NSEC_PER_SEC);
> +	if (tmp > U32_MAX)
> +		tmp = U32_MAX;
> +
> +	return tmp;
> +}
> +
> +/**
> + * rockchip_pwm_v4_round_params - convert PWM parameters to hardware
> + * @rate: PWM clock rate to do the calculations at
> + * @wf: pointer to the generic &struct pwm_waveform input parameters
> + * @wfhw: pointer to the hardware-specific &struct rockchip_pwm_v4_wf output
> + *        parameters that the results will be stored in
> + *
> + * Convert nanosecond-based duty/period/offset parameters to the PWM hardware's
> + * native rounded representation in number of cycles at clock rate @rate. Should
> + * any of the input parameters be out of range for the hardware, the
> + * corresponding output parameter is the maximum permissible value for said
> + * parameter with considerations to the others.
> + */
> +static void rockchip_pwm_v4_round_params(unsigned long rate,
> +					 const struct pwm_waveform *wf,
> +					 struct rockchip_pwm_v4_wf *wfhw)
> +{
> +	wfhw->period = rockchip_pwm_v4_round_single(rate, wf->period_length_ns);
> +
> +	wfhw->duty = rockchip_pwm_v4_round_single(rate, wf->duty_length_ns);
> +
> +	/* As per TRM, PWM_OFFSET: "The value ranges from 0 to (period-duty)" */

Have you tried what happens if you break this rule? That seems like a
very arbitrary restriction that might just originate from the
documentation author.

> +	wfhw->offset = rockchip_pwm_v4_round_single(rate, wf->duty_offset_ns);
> +	if (!wfhw->period) /* Don't underflow when pwm disabled */
> +		wfhw->offset = 0;
> +	else if (wfhw->offset > wfhw->period - wfhw->duty)
> +		wfhw->offset = wfhw->period - wfhw->duty;

You don't enforce wfhw->period >= wfhw->duty, which however seems like a
reasonable restriction. When knowing that you can drop the explicit
check for !wfhw->period and just keep the else-if branch.

> +}
> [...]
> +static int rockchip_pwm_v4_write_wf(struct pwm_chip *chip, struct pwm_device *pwm,
> +				    const void *_wfhw)
> +{
> +	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
> +	const struct rockchip_pwm_v4_wf *wfhw = _wfhw;
> +	bool was_enabled;
> +	int ret;
> +
> +	ret = mfpwm_acquire(pc->pwmf);
> +	if (ret)
> +		return ret;
> +
> +	was_enabled = rockchip_pwm_v4_is_enabled(mfpwm_reg_read(pc->pwmf->base,
> +								PWMV4_REG_ENABLE));
> +
> +	/*
> +	 * "But Nicolas", you ask with valid concerns, "why would you enable the
> +	 * PWM before setting all the parameter registers?"
> +	 *
> +	 * Excellent question, Mr. Reader M. Strawman! The RK3576 TRM Part 1
> +	 * Section 34.6.3 specifies that this is the intended order of writes.
> +	 * Doing the PWM_EN and PWM_CLK_EN writes after the params but before
> +	 * the CTRL_UPDATE_EN, or even after the CTRL_UPDATE_EN, results in
> +	 * erratic behaviour where repeated turning on and off of the PWM may
> +	 * not turn it off under all circumstances. This is also why we don't
> +	 * use relaxed writes; it's not worth the footgun.

I wonder if it's worth however to delay setting PWMV4_CTRL_UPDATE_EN
until after the clkrate is handled to (maybe) prevent glitches?

> +	 */
> +	if (wfhw->rate)
> +		mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
> +				FIELD_PREP_WM16(PWMV4_EN_BOTH_MASK,
> +						PWMV4_EN_BOTH_MASK));
> +	else
> +		mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
> +				FIELD_PREP_WM16(PWMV4_EN_BOTH_MASK, 0));
> +
> +	mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_PERIOD, wfhw->period);
> +	mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_DUTY, wfhw->duty);
> +	mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_OFFSET, wfhw->offset);
> +
> +	mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_CTRL, PWMV4_CTRL_CONT_FLAGS);
> +
> +	/* Commit new configuration to hardware output. */
> +	mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
> +			PWMV4_CTRL_UPDATE_EN);
> +
> +	if (wfhw->rate) {
> +		if (!was_enabled) {
> +			dev_dbg(&chip->dev, "Enabling PWM output\n");
> +			ret = clk_enable(pc->pwmf->core);
> +			if (ret)
> +				goto err_mfpwm_release;
> +			ret = clk_set_rate_exclusive(pc->pwmf->core, wfhw->rate);
> +			if (ret) {
> +				clk_disable(pc->pwmf->core);
> +				goto err_mfpwm_release;
> +			}
> +
> +			/*
> +			 * Output should be on now, acquire device to guarantee
> +			 * exclusion with other device functions while it's on.
> +			 *
> +			 * It's highly unlikely that this fails, as mfpwm has
> +			 * already been acquired before, and this is just a
> +			 * usage counter increase. Not worth the added
> +			 * complexity of clearing the PWMV4_REG_ENABLE again,
> +			 * especially considering the CTRL_UPDATE_EN behaviour.
> +			 */
> +			ret = mfpwm_acquire(pc->pwmf);
> +			if (ret) {
> +				clk_rate_exclusive_put(pc->pwmf->core);
> +				clk_disable(pc->pwmf->core);
> +				goto err_mfpwm_release;
> +			}
> +		}

Can it happen that we have

	wfhw->rate && was_enabled && wfhw->rate != clk_get_rate()

?

> +	} else if (was_enabled) {
> +		dev_dbg(&chip->dev, "Disabling PWM output\n");
> +		clk_rate_exclusive_put(pc->pwmf->core);
> +		clk_disable(pc->pwmf->core);
> +		/* Output is off now, extra release to balance extra acquire */
> +		mfpwm_release(pc->pwmf);
> +	}
> +
> +err_mfpwm_release:
> +	mfpwm_release(pc->pwmf);
> +
> +	return ret;
> +}
> [...]
> +static int rockchip_pwm_v4_probe(struct platform_device *pdev)
> +{
> +	struct rockchip_mfpwm_func *pwmf = dev_get_platdata(&pdev->dev);
> +	struct rockchip_pwm_v4 *pc;
> +	struct pwm_chip *chip;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	/*
> +	 * For referencing the PWM in the DT to work, we need the parent MFD
> +	 * device's OF node.
> +	 */
> +	dev->of_node_reused = true;
> +	device_set_node(dev, of_fwnode_handle(dev->parent->of_node));
> +
> +	chip = devm_pwmchip_alloc(dev, 1, sizeof(*pc));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	pc = to_rockchip_pwm_v4(chip);
> +	pc->pwmf = pwmf;
> +
> +	ret = mfpwm_acquire(pwmf);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Couldn't acquire mfpwm in probe\n");
> +
> +	if (!rockchip_pwm_v4_on_and_continuous(pc))
> +		mfpwm_release(pwmf);
> +	else {
> +		dev_dbg(dev, "PWM was already on at probe time\n");
> +		ret = clk_enable(pwmf->core);
> +		if (ret) {
> +			dev_err_probe(dev, ret, "Enabling pwm clock failed\n");
> +			goto err_mfpwm_release;
> +		}
> +		ret = clk_rate_exclusive_get(pc->pwmf->core);
> +		if (ret) {
> +			dev_err_probe(dev, ret, "Protecting pwm clock failed\n");
> +			goto err_clk_disable;
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, chip);

This is unused.

> +
> +	chip->ops = &rockchip_pwm_v4_ops;
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to add PWM chip\n");
> +		if (rockchip_pwm_v4_on_and_continuous(pc))
> +			goto err_rate_put;
> +
> +		return ret;
> +	}
> +
> +	return 0;
> +
> +err_rate_put:
> +	clk_rate_exclusive_put(pwmf->core);
> +err_clk_disable:
> +	clk_disable(pwmf->core);
> +err_mfpwm_release:
> +	mfpwm_release(pwmf);
> +
> +	return ret;
> +}

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]