Re: [PATCH 1/2] pwm: add microchip soft ip corePWM driver

[Conor.Dooley]

Hey Uwe, one last one for ya..

On 08/06/2022 16:13, Uwe Kleine-König wrote:
> Hello Conor,
> 
> On Wed, Jun 08, 2022 at 12:12:37PM +0000, Conor.Dooley@xxxxxxxxxxxxx wrote:
>> On 07/06/2022 21:07, Uwe Kleine-König wrote:
>>> On Tue, Jun 07, 2022 at 09:45:51AM +0100, Conor Dooley wrote:
>>>> Add a driver that supports the Microchip FPGA "soft" PWM IP core.
>>>>
>>>> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>>>> ---
---8<---
>>>> +struct mchp_core_pwm_registers {
>>>> +	u8 posedge;
>>>> +	u8 negedge;
>>>> +	u8 period_steps;
>>>> +	u8 prescale;
>>>
>>> these are the four registers for each channel, right? Can you add a
>>> short overview how these registers define the resulting output wave.
>>
>> Ehh, only the edges are per channel. Does that change anything about
>> your feedback?
>> I'll add an explanation for each, sure.
> 
> So the channels share the same period? If so you'll have to keep track
> of which PWM channels are enabled and only change the period if no other
> running channel is affected.

When I am capping the period (or not allowing it to be changed in this case
here) should I correct the duty cycle so that the the ratio is preserved?
Thanks,
Conor.

> 
>>>> +};
>>>> +
>>>> +struct mchp_core_pwm_chip {
>>>> +	struct pwm_chip chip;
>>>> +	struct clk *clk;
>>>> +	void __iomem *base;
>>>> +	struct mchp_core_pwm_registers *regs;
>>>> +};
>>>> +
>>>> +static inline struct mchp_core_pwm_chip *to_mchp_core_pwm(struct pwm_chip *chip)
>>>> +{
>>>> +	return container_of(chip, struct mchp_core_pwm_chip, chip);
>>>> +}
>>>> +
>>>> +static void mchp_core_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
>>>> +				 bool enable)
>>>> +{
>>>> +	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 = PWM_EN_LOW_REG + (pwm->hwpwm >> 3) * sizeof(u32);
>>>> +	shift = pwm->hwpwm > 7 ? pwm->hwpwm - 8 : pwm->hwpwm;
>>>> +
>>>> +	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);
>>>> +}
>>>> +
>>>> +static void mchp_core_pwm_calculate_duty(struct pwm_chip *chip,
>>>> +					 const struct pwm_state *desired_state,
>>>> +					 struct mchp_core_pwm_registers *regs)
>>>> +{
>>>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>>>> +	u64 clk_period = NSEC_PER_SEC;
>>>> +	u64 duty_steps;
>>>> +
>>>> +	/* Calculate the clk period and then the duty cycle edges */
>>>> +	do_div(clk_period, clk_get_rate(mchp_core_pwm->clk));
>>>> +
>>>> +	duty_steps = desired_state->duty_cycle * PREG_TO_VAL(regs->period_steps);
>>>> +	do_div(duty_steps, (clk_period * PREG_TO_VAL(regs->period_steps)));
>>>
>>> Don't divide by a result of a division.
>>>
>>>> +	if (desired_state->polarity == PWM_POLARITY_INVERSED) {
>>>> +		regs->negedge = 0u;
>>>> +		regs->posedge = duty_steps;
>>>> +	} else {
>>>> +		regs->posedge = 0u;
>>>> +		regs->negedge = duty_steps;
>>>> +	}
>>>> +}
>>>> +
>>>> +static void mchp_core_pwm_apply_duty(const u8 channel,
>>>> +				     struct mchp_core_pwm_chip *pwm_chip,
>>>> +				     struct mchp_core_pwm_registers *regs)
>>>> +{
>>>> +	void __iomem *channel_base = pwm_chip->base + channel * CHANNEL_OFFSET;
>>>> +
>>>> +	writel_relaxed(regs->posedge, channel_base + POSEDGE_OFFSET);
>>>> +	writel_relaxed(regs->negedge, channel_base + NEGEDGE_OFFSET);
>>>> +}
>>>> +
>>>> +static void mchp_core_pwm_apply_period(struct mchp_core_pwm_chip *pwm_chip,
>>>> +				       struct mchp_core_pwm_registers *regs)
>>>> +{
>>>> +	writel_relaxed(regs->prescale, pwm_chip->base + PRESCALE_REG);
>>>> +	writel_relaxed(regs->period_steps, pwm_chip->base + PERIOD_REG);
>>>> +}
>>>> +
>>>> +static int mchp_core_pwm_calculate_base(struct pwm_chip *chip,
>>>> +					const struct pwm_state *desired_state,
>>>> +					u8 *period_steps_r, u8 *prescale_r)
>>>> +{
>>>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>>>> +	u64 tmp = desired_state->period;
>>>> +
>>>> +	/* Calculate the period cycles and prescale value */
>>>> +	tmp *= clk_get_rate(mchp_core_pwm->clk);
>>>> +	do_div(tmp, NSEC_PER_SEC);
>>>> +
>>>> +	if (tmp > 65535) {
>>>
>>> If a too long period is requested, please configure the biggest possible
>>> period.
>>>
>>>> +		dev_err(chip->dev,
>>>> +			"requested prescale exceeds the maximum possible\n");
>>>
>>> No error message in .apply() please.
>>
>> No /error/ or no error /message/?
> 
> No error message. Otherwise a userspace application might easily trash
> the kernel log.
> 
>> As in, can I make it a dev_warn or do you want it removed entirely
>> and replaced by capping at the max value?
> 
> Yes, just cap to the max value. So the rule is always to pick the
> biggest possible period not bigger than the requested period. And for
> that one pick the biggest duty_cycle not bigger than the requested
> duty_cycle.
> 
>>>> +	if (desired_state->enabled) {
>>>> +		if (current_state.enabled &&
>>>> +		    current_state.period == desired_state->period &&
>>>> +		    current_state.polarity == desired_state->polarity) {
>>>
>>> If everything is as before, why are you doing something at all?
>>
>> This is a change in duty without any other change.
>> Could just remove this & recalculate everything when apply is called
>> to simply the logic?
> 
> Ah, right. A comment (e.g. "only duty cycle changed") would be good for
> such stupid readers like me :-)
> 
> I don't feel strong here. For many cases the period (and polarity) is
> kept constant and only duty_cycle changes. So optimizing for that case
> looks ok.
> 
>>>> +			mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs);
>>>> +			mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs);
>>>> +		} else {
>>>> +			ret = mchp_core_pwm_calculate_base(chip, desired_state, &period_steps_r,
>>>> +							   &prescale_r);
>>>> +			if (ret) {
>>>> +				dev_err(chip->dev, "failed to calculate base\n");
>>>
>>> mchp_core_pwm_calculate_base might already emit an error message. Apply
>>> shouldn't emit an error message at all.
>>>
>>>> +				return ret;
>>>> +			}
>>>> +
>>>> +			mchp_core_pwm->regs->period_steps = period_steps_r;
>>>> +			mchp_core_pwm->regs->prescale = prescale_r;
>>>> +
>>>> +			mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs);
>>>> +			mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs);
>>>> +			mchp_core_pwm_apply_period(mchp_core_pwm, mchp_core_pwm->regs);
>>>
>>> Is there a race where e.g. the output is defined by the previous period
>>> and the new duty_cycle?
>>
>> "Yes". It depends on how the IP block is configured. I'll add a write to
>> the sync register (which is a NOP if not configured for sync mode).
> 
> Several drivers have a "Limitations" section at the top of the driver.
> Something like that would be good to document there. Please stick to the
> format found in e.g. pwm-sl28cpld.c, that is: "Limitations:" (even if
> it's more about "Hardware Details") and then a list of items without
> empty line in between for easy greppability.
> 
>>>
>>>> +		}
>>>> +
>>>> +		if (mchp_core_pwm->regs->posedge == mchp_core_pwm->regs->negedge)
>>>> +			mchp_core_pwm_enable(chip, pwm, false);
>>>> +		else
>>>> +			mchp_core_pwm_enable(chip, pwm, true);
>>>
>>> Here is a race: If the PWM is running and it configured to be disabled
>>> with a different duty_cycle/period the duty_cycle/period might be
>>> (shortly) visible on the output with is undesirable.
>>
>> This is trying to work around some nasty behaviour in the IP block.
>> If negedge == posedge, it gives you a 50% duty cycle at twice the
>> period you asked for.
>> ie since the negedge and posedge are at the same time, it does
>> whichever edge is actually possible each time that period step is
>> reached.
> 
> I didn't understand the normal behaviour of these registers yet, so
> cannot comment. Usually it's a good idea to document corner cases in
> comments.
> 
>> If the state requested is disabled, it should be caught by the if()
>> prior to entering this & exit early & avoid this entirely.
>>
>> I'll put the sync reg write after this, so if the block is configured
>> to support sync updates, the output waveform won't do anything odd.
> 
> Sounds good.
> 
> Best regards
> Uwe
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-riscv