[v12 2/2] pwm: Add Aspeed ast2600 PWM support

On Mon, Sep 06, 2021 at 10:43:39AM +0800, Billy Tsai wrote:

From reading this it's not clear (to me that is) what's the difference
between "period duration" and "PERIOD". Maybe rewrite the comment to
something like:

	The hardware operates in time quantities of length

		Q := (DIV_L + 1) << DIV_H / input-clk

	The length of a PWM period is (DUTY_CYCLE_PERIOD + 1) * Q.
	The maximal value for DUTY_CYCLE_PERIOD is used here to provide
	a fine grained selection for the duty cycle.

	This driver uses DUTY_CYCLE_RISING_POINT = 0, so from the start of a
	period the output is active until DUTY_CYCLE_FALLING_POINT * Q. Note
	that if DUTY_CYCLE_RISING_POINT = DUTY_CYCLE_FALLING_POINT the output is
	always active.

This is a bit more high-level and still gives all the details.

Maybe the special case with DUTY_CYCLE_RISING_POINT =
DUTY_CYCLE_FALLING_POINT would be a bit more natural if the driver used
inverse logic: Assume the output to be inverted with CTRL_INVERSE = 0,
fix DUTY_CYCLE_FALLING_POINT to 0 and adapt DUTY_CYCLE_RISING_POINT
dependant on duty cycle? At least then DUTY_CYCLE_RISING_POINT = 0
corresponds to duty_cycle = 0. Just an idea ... which might or might not
work depending on the hardware.

+ * Register usage:
+ * PIN_ENABLE: When it is unset the pwm controller will emit inactive level to the extern.
+ * Use to determine whether the PWM channel is enabled or disabled
+ * CLK_ENABLE: When it is unset the pwm controller will assert the duty counter reset and
+ * emit inactive level to the PIN_ENABLE mux after that the driver can still change the pwm period
+ * and duty and the value will apply when CLK_ENABLE be set again.
+ * Use to determine whether duty_cycle bigger than 0.
+ * PWM_ASPEED_CTRL_INVERSE: When it is toggled the output value will inverse immediately.
+ * PWM_ASPEED_DUTY_CYCLE_FALLING_POINT/PWM_ASPEED_DUTY_CYCLE_RISING_POINT: When these two
+ * values are equal it means the duty cycle = 100%.
+ *
+ * The glitch may generate at:
+ * - Enabled changing when the duty_cycle bigger than 0% and less than 100%.
+ * - Polarity changing when the duty_cycle bigger than 0% and less than 100%.
+ * - Set duty cycle to 0% from other values.
+ *
+ * Limitations:
+ * - When changing both duty cycle and period, we cannot prevent in
+ *   software that the output might produce a period with mixed
+ *   settings.
+ * - Disabling the PWM doesn't complete the current period.
+ *
+ * Improvements:
+ * - When only changing one of duty cycle or period, our pwm controller will not
+ *   generate the glitch, the configure will change at next cycle of pwm.
+ *   This improvement can disable/enable through PWM_ASPEED_CTRL_DUTY_SYNC_DISABLE.
+ */
+
[...]
+static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			    const struct pwm_state *state)
+{
+	struct device *dev = chip->dev;
+	struct aspeed_pwm_data *priv = aspeed_pwm_chip_to_data(chip);
+	u32 hwpwm = pwm->hwpwm, duty_pt;
+	unsigned long rate;
+	u64 div_h, div_l, divisor;
+	bool clk_en;
+
+	dev_dbg(dev, "expect period: %lldns, duty_cycle: %lldns", state->period,
+		state->duty_cycle);
+
+	rate = clk_get_rate(priv->clk);
+	if (state->period > div64_u64(ULLONG_MAX, (u64)rate))
+		return -ERANGE;

Given that the biggest possible period <= requested period should be
configured, this check + return is wrong.

+	/*
+	 * Pick the smallest value for div_h so that div_l can be the biggest
+	 * which results in a finer resolution near the target period value.
+	 */
+	divisor = (u64)NSEC_PER_SEC * (PWM_ASPEED_FIXED_PERIOD + 1) *
+		  (FIELD_MAX(PWM_ASPEED_CTRL_CLK_DIV_L) + 1);
+	div_h = order_base_2(DIV64_U64_ROUND_UP(rate * state->period, divisor));
+	if (div_h > 0xf)
+		div_h = 0xf;
+
+	divisor = ((u64)NSEC_PER_SEC * (PWM_ASPEED_FIXED_PERIOD + 1)) << div_h;
+	div_l = div64_u64(rate * state->period, divisor);
+
+	if (div_l == 0)
+		return -ERANGE;
+
+	div_l -= 1;
+
+	if (div_l > 255)
+		div_l = 255;
+
+ [...]

I didn't check the remaining part here in detail, it does look right and
I think we already discussed this in the earlier revisions.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20211011/f6f63bf0/attachment.sig>