Message-ID: <e55e4a7f-b0bc-f48a-b555-d4b96d69bb87@microchip.com>
Date:   Tue, 2 Aug 2022 12:34:14 +0000
From:   <Conor.Dooley@...rochip.com>
To:     <u.kleine-koenig@...gutronix.de>
CC:     <Conor.Dooley@...rochip.com>, <Daire.McNamara@...rochip.com>,
        <devicetree@...r.kernel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <lee.jones@...aro.org>, <linux-kernel@...r.kernel.org>,
        <linux-pwm@...r.kernel.org>, <linux-riscv@...ts.infradead.org>,
        <robh+dt@...nel.org>, <thierry.reding@...il.com>
Subject: Re: [PATCH v7 3/4] pwm: add microchip soft ip corePWM driver



On 02/08/2022 09:46, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Jul 21, 2022 at 06:21:09PM +0100, Conor Dooley wrote:
>> From: Conor Dooley <conor.dooley@...rochip.com>
>>
>> Add a driver that supports the Microchip FPGA "soft" PWM IP core.
>>
>> Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
>> ---

>> +static void mchp_core_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
>> +				 bool enable, u64 period)
>> +{
>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> +	u8 channel_enable, reg_offset, shift;
>> +
>> +	/*
>> +	 * There are two adjacent 8 bit control regs, the lower reg controls
>> +	 * 0-7 and the upper reg 8-15. Check if the pwm is in the upper reg
>> +	 * and if so, offset by the bus width.
>> +	 */
>> +	reg_offset = MCHPCOREPWM_EN(pwm->hwpwm >> 3);
>> +	shift = pwm->hwpwm & 7;
>> +
>> +	channel_enable = readb_relaxed(mchp_core_pwm->base + reg_offset);
>> +	channel_enable &= ~(1 << shift);
>> +	channel_enable |= (enable << shift);
>> +
>> +	writel_relaxed(channel_enable, mchp_core_pwm->base + reg_offset);
>> +	mchp_core_pwm->channel_enabled &= ~BIT(pwm->hwpwm);
>> +	mchp_core_pwm->channel_enabled |= enable << pwm->hwpwm;
>> +
>> +	/*
>> +	 * Notify the block to update the waveform from the shadow registers.
>> +	 * The updated values will not appear on the bus until they have been
>> +	 * applied to the waveform at the beginning of the next period. We must
>> +	 * write these registers and wait for them to be applied before calling
>> +	 * enable().
> 
> What does "calling enable()" mean? There is no such function or callback
> with that name?!

I relocated the comment but forgot to proof read it!
s/calling enable()/considering the channel enabled/
I'm not sure where it comes from, but I keep thinking that there is an
enable() callback...

> 
>> +	 */
>> +	if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) {
>> +		writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD);
>> +		usleep_range(period, period * 2);
> 
> So if period = 5000 *ns* you sleep between 5000 and 10000 *us* here?

/facepalm

> 
>> +	}
>> +}
>> +
>> +static u64 mchp_core_pwm_calc_duty(struct pwm_chip *chip, struct pwm_device *pwm,
>> +				   const struct pwm_state *state, u8 prescale, u8 period_steps)
>> +{
>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> +	u64 duty_steps, period, tmp;
>> +	u16 prescale_val = PREG_TO_VAL(prescale);
>> +	u8 period_steps_val = PREG_TO_VAL(period_steps);
> 
> Can it happen that period_steps is 0xff? Then period_steps_val ends up
> being 0.

I guess that register could have a value it in from the bootloader etc and
therefore handling it is a good idea - but not in this function since it
is never used...

> 
>> +
>> +	period = period_steps_val * prescale_val * NSEC_PER_SEC;
>> +	period = DIV64_U64_ROUND_UP(period, clk_get_rate(mchp_core_pwm->clk));
> 
> The value you are calculating for period isn't used?!

huh, I am surprised that this was not caught by a W=1 C=1 build. Or maybe it
was and I just didn't notice - but I am 99% sure I made sure there were none.

> 
>> +
>> +	/*
>> +	 * Calculate the duty cycle in multiples of the prescaled period:
>> +	 * duty_steps = duty_in_ns / step_in_ns
>> +	 * step_in_ns = (prescale * NSEC_PER_SEC) / clk_rate
>> +	 * The code below is rearranged slightly to only divide once.
>> +	 */
>> +	duty_steps = state->duty_cycle * clk_get_rate(mchp_core_pwm->clk);
>> +	tmp = prescale_val * NSEC_PER_SEC;
>> +	return div64_u64(duty_steps, tmp);
>> +}
>> +
>> +static void mchp_core_pwm_apply_duty(struct pwm_chip *chip, struct pwm_device *pwm,
>> +				     const struct pwm_state *state, u64 duty_steps, u8 period_steps)
>> +{
>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> +	u8 posedge, negedge;
>> +	u8 period_steps_val = PREG_TO_VAL(period_steps);
>> +
>> +	/*
>> +	 * Turn the output on unless posedge == negedge, in which case the
>> +	 * duty is intended to be 0, but limitations of the IP block don't
>> +	 * allow a zero length duty cycle - so just set the max high/low time
>> +	 * respectively.
>> +	 */
> 
> I don't understand that comment. Maybe you mean?:
> 
> 	/*
> 	 * Setting posedge == negedge doesn't yield a constant output,
> 	 * so that's an unsuitable setting to model duty_steps = 0.
> 	 * In that case set the unwanted edge to a value that never
> 	 * triggers.
> 	 */

Yeah, this is a better comment. Thanks.

> 
>> +	if (state->polarity == PWM_POLARITY_INVERSED) {
>> +		negedge = !duty_steps ? period_steps_val : 0u;
>> +		posedge = duty_steps;
>> +	} else {
>> +		posedge = !duty_steps ? period_steps_val : 0u;
>> +		negedge = duty_steps;
>> +	}
>> +
>> +	writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
>> +	writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
>> +}
>> +
>> +static int mchp_core_pwm_calc_period(struct pwm_chip *chip, const struct pwm_state *state,
>> +				     u8 *prescale, u8 *period_steps)
>> +{
>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> +	u64 tmp, clk_rate;
>> +
>> +	/*
>> +	 * Calculate the period cycles and prescale values.
>> +	 * The registers are each 8 bits wide & multiplied to compute the period
>> +	 * using the formula:
>> +	 * (clock_period) * (prescale + 1) * (period_steps + 1)
>> +	 * so the maximum period that can be generated is 0x10000 times the
>> +	 * period of the input clock.
>> +	 * However, due to the design of the "hardware", it is not possible to
>> +	 * attain a 100% duty cycle if the full range of period_steps is used.
>> +	 * Therefore period_steps is restricted to 0xFE and the maximum multiple
>> +	 * of the clock period attainable is 0xFF00.
>> +	 */
>> +	clk_rate = clk_get_rate(mchp_core_pwm->clk);
>> +
>> +	/*
>> +	 * If clk_rate is too big, the following multiplication might overflow.
>> +	 * However this is implausible, as the fabric of current FPGAs cannot
>> +	 * provide clocks at a rate high enough.
>> +	 */
>> +	if (clk_rate >= NSEC_PER_SEC)
>> +		return -EINVAL;
>> +
>> +	tmp = mul_u64_u64_div_u64(state->period, clk_rate, NSEC_PER_SEC);
>> +
>> +	if (tmp >= MCHPCOREPWM_PERIOD_MAX) {
>> +		*prescale = MCHPCOREPWM_PRESCALE_MAX - 1;
> 
> why -1 here?

Because the hardware adds 1 to the register value. I had tried to explain
in the large comment above, but I will reword the comment for v8.

> 
>> +		*period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1;
>> +		return 0;
>> +	}
>> +
>> +	*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX);
>> +	/* PREG_TO_VAL() can produce a value larger than UINT8_MAX */
> 
> That should explain the cast to u32? If this were really necessary
> (hint: it isn't) it would IMHO be better to hide that cast in the macro.
> 
>> +	*period_steps = div_u64(tmp, PREG_TO_VAL((u32)*prescale)) - 1;
>> +
>> +	return 0;
>> +}
>> +
>> +static inline void mchp_core_pwm_apply_period(struct mchp_core_pwm_chip *mchp_core_pwm,
>> +					      u8 prescale, u8 period_steps)
>> +{
>> +	writel_relaxed(prescale, mchp_core_pwm->base + MCHPCOREPWM_PRESCALE);
>> +	writel_relaxed(period_steps, mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
>> +}
>> +
>> +static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			       const struct pwm_state *state)
>> +{
>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> +	struct pwm_state current_state = pwm->state;
>> +	bool period_locked;
>> +	u64 duty_steps;
>> +	u8 prescale, period_steps, hw_prescale, hw_period_steps;
>> +	int ret;
>> +
>> +	ret = mutex_lock_interruptible(&mchp_core_pwm->lock);
>> +	if (ret)
>> +		return ret;
> 
> I would have used mutex_lock() here. Why should a signal prevent
> reconfiguration of the PWM?

Cool, willdo.

> 
>> +
>> +	if (!state->enabled) {
>> +		mchp_core_pwm_enable(chip, pwm, false, current_state.period);
>> +		mutex_unlock(&mchp_core_pwm->lock);
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * If the only thing that has changed is the duty cycle or the polarity,
>> +	 * we can shortcut the calculations and just compute/apply the new duty
>> +	 * cycle pos & neg edges
>> +	 * As all the channels share the same period, do not allow it to be
>> +	 * changed if any other channels are enabled.
>> +	 * If the period is locked, it may not be possible to use a period
>> +	 * less than that requested. In that case, we just abort.
>> +	 */
>> +	period_locked = mchp_core_pwm->channel_enabled & ~(1 << pwm->hwpwm);
>> +
>> +	if (period_locked) {
>> +		mchp_core_pwm_calc_period(chip, state, &prescale, &period_steps);
>> +		hw_prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE);
>> +		hw_period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
>> +
>> +		if ((period_steps * prescale) < (hw_period_steps * hw_prescale)) {
> 
> You need
> 
> 	if ((period_steps + 1) * (prescale + 1) < (hw_period_steps + 1) * (hw_prescale + 1))
> 
> here, don't you?

Yikes, yeah...

> 
>> +			mutex_unlock(&mchp_core_pwm->lock);
>> +			return -EINVAL;
>> +		}
>> +
>> +		prescale = hw_prescale;
>> +		period_steps = hw_period_steps;
> 
> The two hw_* variables are only used in this branch. So their
> declaration can move into here.
> 
>> +	} else if (!current_state.enabled || current_state.period != state->period) {
>> +		ret = mchp_core_pwm_calc_period(chip, state, &prescale, &period_steps);
>> +		if (ret) {
>> +			mutex_unlock(&mchp_core_pwm->lock);
>> +			return ret;
>> +		}
>> +		mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps);
>> +	} else {
>> +		prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE);
>> +		period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
>> +	}
>> +
>> +	duty_steps = mchp_core_pwm_calc_duty(chip, pwm, state, prescale, period_steps);
>> +
>> +	/*
>> +	 * Because the period is per channel, it is possible that the requested
>> +	 * duty cycle is longer than the period, in which case cap it to the
>> +	 * period, IOW a 100% duty cycle.
>> +	 */
>> +	if (duty_steps > period_steps)
>> +		duty_steps = period_steps + 1;
>> +
>> +	mchp_core_pwm_apply_duty(chip, pwm, state, duty_steps, period_steps);
>> +
>> +	mchp_core_pwm_enable(chip, pwm, true, state->period);
>> +
>> +	mutex_unlock(&mchp_core_pwm->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static void mchp_core_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>> +				    struct pwm_state *state)
>> +{
>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> +	u16 prescale;
>> +	u8 period_steps, duty_steps, posedge, negedge;
>> +	int ret;
>> +
>> +	ret = mutex_lock_interruptible(&mchp_core_pwm->lock);
>> +	if (ret)
>> +		return;
>> +
>> +	if (mchp_core_pwm->channel_enabled & (1 << pwm->hwpwm))
> 
> channel_enabled is initialized to 0 in .probe(), so a PWM is never
> diagnosed to be running when the core initially wants to determine the
> current state.

Good point. I'll initialise it in probe.

> 
>> +		state->enabled = true;
>> +	else
>> +		state->enabled = false;
>> +
>> +	prescale = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE));
>> +
>> +	period_steps = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD));
>> +	state->period = period_steps * prescale * NSEC_PER_SEC;
>> +	state->period = DIV64_U64_ROUND_UP(state->period, clk_get_rate(mchp_core_pwm->clk));
>> +
>> +	posedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
>> +	negedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
>> +
>> +	if (negedge == posedge) {
>> +		state->duty_cycle = state->period / 2;
> 
> I thought that's:
> 
> 		state->duty_cycle = state->period;
> 		state->period *= 2;

Correct, as usual..
Thanks for your review Uwe! I'll fix it all up & submit v8 after -rc1.
Conor.

Powered by blists - more mailing lists