Message-ID: <eefc366e-1d32-7565-0d6d-243b8addc381@microchip.com>
Date:   Wed, 8 Jun 2022 12:12:37 +0000
From:   <Conor.Dooley@...rochip.com>
To:     <u.kleine-koenig@...gutronix.de>
CC:     <thierry.reding@...il.com>, <lee.jones@...aro.org>,
        <Daire.McNamara@...rochip.com>, <linux-kernel@...r.kernel.org>,
        <linux-pwm@...r.kernel.org>, <linux-riscv@...ts.infradead.org>
Subject: Re: [PATCH 1/2] pwm: add microchip soft ip corePWM driver

Hey Uwe,
Anything I've ignored here I will change for v2 - just have
a few questions about your feedback/responses to questions.

On 07/06/2022 21:07, Uwe Kleine-König wrote:
> Hello,
> 
> 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@...rochip.com>
>> ---
>>   drivers/pwm/Kconfig              |  10 ++
>>   drivers/pwm/Makefile             |   1 +
>>   drivers/pwm/pwm-microchip-core.c | 289 +++++++++++++++++++++++++++++++
>>   3 files changed, 300 insertions(+)
>>   create mode 100644 drivers/pwm/pwm-microchip-core.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 21e3b05a5153..a651848e444b 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -383,6 +383,16 @@ config PWM_MEDIATEK
>>   	  To compile this driver as a module, choose M here: the module
>>   	  will be called pwm-mediatek.
>>   
>> +config PWM_MICROCHIP_CORE
>> +	tristate "Microchip corePWM PWM support"
>> +	depends on SOC_MICROCHIP_POLARFIRE || COMPILE_TEST
>> +	depends on HAS_IOMEM && OF
>> +	help
>> +	  PWM driver for Microchip FPGA soft IP core.
>> +
>> +	  To compile this driver as a module, choose M here: the module
>> +	  will be called pwm-microchip-core.
>> +
>>   config PWM_MXS
>>   	tristate "Freescale MXS PWM support"
>>   	depends on ARCH_MXS || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 708840b7fba8..d29754c20f91 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -33,6 +33,7 @@ obj-$(CONFIG_PWM_LPSS_PCI)	+= pwm-lpss-pci.o
>>   obj-$(CONFIG_PWM_LPSS_PLATFORM)	+= pwm-lpss-platform.o
>>   obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
>>   obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
>> +obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
>>   obj-$(CONFIG_PWM_MTK_DISP)	+= pwm-mtk-disp.o
>>   obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
>>   obj-$(CONFIG_PWM_NTXEC)		+= pwm-ntxec.o
>> diff --git a/drivers/pwm/pwm-microchip-core.c b/drivers/pwm/pwm-microchip-core.c
>> new file mode 100644
>> index 000000000000..2cc1de163bcc
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-microchip-core.c
>> @@ -0,0 +1,289 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * corePWM driver for Microchip FPGAs
>> + *
>> + * Copyright (c) 2021-2022 Microchip Corporation. All rights reserved.
>> + * Author: Conor Dooley <conor.dooley@...rochip.com>
>> + *
> 
> Is there a public datasheet for that hardware? If yes, please add a link
> here.

Not quite a datasheet since this is for an FPGA core not a "real" part.
I can link the following "handbook" for it though (direct download link):
https://www.microsemi.com/document-portal/doc_download/1245275-corepwm-hb

> 
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/math.h>
>> +
>> +#define PREG_TO_VAL(PREG) ((PREG) + 1)
>> +
>> +#define PRESCALE_REG	0x00u
>> +#define PERIOD_REG	0x04u
>> +#define PWM_EN_LOW_REG	0x08u
>> +#define PWM_EN_HIGH_REG	0x0Cu
>> +#define SYNC_UPD_REG	0xE4u
>> +#define POSEDGE_OFFSET	0x10u
>> +#define NEGEDGE_OFFSET	0x14u
>> +#define CHANNEL_OFFSET	0x08u
> 
> Can you please prefix these register symbols with a driver prefix?
> Unique register names are good for tools like ctags and then it's
> obvious to the reader, that the defines are driver specific.
> 
>> +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.

> 
>> +};
>> +
>> +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/?
As in, can I make it a dev_warn or do you want it removed entirely
and replaced by capping at the max value?

> 
>> +		return -EINVAL;
>> +	} else if (tmp <= 256) {
>> +		*prescale_r = 0;
>> +		*period_steps_r = tmp - 1;
>> +	} else {
>> +		*prescale_r = fls(tmp) - 8;
>> +		*period_steps_r = (tmp >> *prescale_r) - 1;
>> +	}
> 
> *prescale_r = fls(tmp >> 8);
> *period_steps_r = (tmp >> *prescale_r) - 1;
> 
> can be used in both branches with the same result.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			       const struct pwm_state *desired_state)
> 
> please s/desired_state/state/ for consistency with other drivers.
> 
>> +{
>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> +	struct pwm_state current_state;
>> +	u8 period_steps_r, prescale_r;
>> +	int ret;
>> +	u8 channel = pwm->hwpwm;
>> +
>> +	pwm_get_state(pwm, &current_state);
> 
> Please don't call pwm API functions in a PWM driver. Simply use
> pwm->state for the previously applied state.
> 
>> +	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?

> 
>> +			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).
> 
>> +		}
>> +
>> +		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.

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.

> 
>> +	} else if (!desired_state->enabled) {
> 
> This can be simplified to just "} else {"
> 
>> +		mchp_core_pwm_enable(chip, pwm, false);
>> +	}
>> +
>> +	return 0;
> 
> I think this can be considerably simplified by unrolling the helper
> functions.
> 
>> +}
>> +
>> +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);
>> +	void __iomem *channel_base = mchp_core_pwm->base + pwm->hwpwm * CHANNEL_OFFSET;
>> +	u64 clk_period = NSEC_PER_SEC;
>> +	u8 prescale, period_steps, duty_steps;
>> +	u8 posedge, negedge;
>> +	u16 channel_enabled;
>> +
>> +	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + PWM_EN_HIGH_REG) << 8) |
>> +		readb_relaxed(mchp_core_pwm->base + PWM_EN_LOW_REG));
>> +
>> +	posedge = readb_relaxed(channel_base + POSEDGE_OFFSET);
>> +	negedge = readb_relaxed(channel_base + NEGEDGE_OFFSET);
>> +
>> +	duty_steps = abs((s8)posedge - (s8)negedge);
>> +	state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
>> +
>> +	prescale = readb_relaxed(mchp_core_pwm->base + PRESCALE_REG);
>> +	period_steps = readb_relaxed(mchp_core_pwm->base + PERIOD_REG);
>> +
>> +	do_div(clk_period, clk_get_rate(mchp_core_pwm->clk));
>> +	state->duty_cycle = PREG_TO_VAL(prescale) * clk_period * duty_steps;
> 
> Dividing early results in rounding errors.
> 
>> +	state->period = PREG_TO_VAL(prescale) * clk_period * PREG_TO_VAL(period_steps);
>> +
>> +	if (channel_enabled & 1 << pwm->hwpwm)
>> +		state->enabled = true;
>> +	else
>> +		state->enabled = false;
>> +}
> 
> Have you tested your driver with PWM_DEBUG enabled? The rounding is
> wrong here. (The supposed rounding behaviour is that .apply rounds down
> and to make .apply(.getstate()) a noop, .get_state() must round up.

I have, but not exhaustively.

> 
> If you do something like:
> 
>          echo 0 > duty_cycle
> 
>          for i in $(seq 10000 -1 1); do
>                  echo $i > period
>          done
> 
>          for i in $(seq 1 10000); do
>                  echo $i > period
>          done
> 
>          for i in $(seq 10000 -1 1); do
>                  echo $i > duty_cycle
>          done
> 
>          for i in $(seq 1 10000); do
>                  echo $i > duty_cycle
>          done
> 
> with PWM_DEBUG enabled, you should catch most rounding errors. (Maybe
> adapt the boundaries according to your driver's capabilities.)

Cool, willdo thanks. Not sure why I didn't run this in a loop.
:facepalm:

Again, thanks for all the feedback Uwe.
Conor.

Powered by blists - more mailing lists