Date:   Thu, 7 Feb 2019 11:16:57 +0100
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Yash Shah <yash.shah@...ive.com>
Cc:     palmer@...ive.com, linux-pwm@...r.kernel.org,
        linux-riscv@...ts.infradead.org, thierry.reding@...il.com,
        robh+dt@...nel.org, mark.rutland@....com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        sachin.ghadi@...ive.com, paul.walmsley@...ive.com
Subject: Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

Hello,

On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> 
> Signed-off-by: Wesley W. Terpstra <wesley@...ive.com>
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra <atish.patra@....com>
> Signed-off-by: Yash Shah <yash.shah@...ive.com>
> ---
>  drivers/pwm/Kconfig      |  11 ++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-sifive.c | 261 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 273 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sifive.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a8f47df..4a61d1a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -380,6 +380,17 @@ config PWM_SAMSUNG
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-samsung.
>  
> +config PWM_SIFIVE
> +	tristate "SiFive PWM support"
> +	depends on OF
> +	depends on COMMON_CLK
> +	depends on RISCV || COMPILE_TEST
> +	help
> +	  Generic PWM framework driver for SiFive SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-sifive.
> +
>  config PWM_SPEAR
>  	tristate "STMicroelectronics SPEAr PWM support"
>  	depends on PLAT_SPEAR
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9c676a0..30089ca 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
>  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
>  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> +obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
>  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
>  obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
>  obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> new file mode 100644
> index 0000000..2b516f7
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,261 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017-2018 SiFive
> + * For SiFive's PWM IP block documentation please refer Chapter 14 of
> + * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> + */
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +/* Register offsets */
> +#define PWM_SIFIVE_PWMCFG		0x0
> +#define PWM_SIFIVE_PWMCOUNT		0x8
> +#define PWM_SIFIVE_PWMS			0x10
> +#define PWM_SIFIVE_PWMCMP0		0x20
> +
> +/* PWMCFG fields */
> +#define PWM_SIFIVE_PWMCFG_SCALE		0
> +#define PWM_SIFIVE_PWMCFG_STICKY	8
> +#define PWM_SIFIVE_PWMCFG_ZERO_CMP	9
> +#define PWM_SIFIVE_PWMCFG_DEGLITCH	10
> +#define PWM_SIFIVE_PWMCFG_EN_ALWAYS	12
> +#define PWM_SIFIVE_PWMCFG_EN_ONCE	13
> +#define PWM_SIFIVE_PWMCFG_CENTER	16
> +#define PWM_SIFIVE_PWMCFG_GANG		24
> +#define PWM_SIFIVE_PWMCFG_IP		28
> +
> +/* SIZE_PWMCMP is used to calculate the offset for pwmcmpX registers */
> +#define SIZE_PWMCMP		4
> +#define CMPWIDTH		16

Please add a PWM_SIFIVE_ prefix to these two.

> +struct pwm_sifive_ddata {
> +	struct pwm_chip	chip;
> +	struct notifier_block notifier;
> +	struct clk *clk;
> +	void __iomem *regs;
> +	unsigned int approx_period;
> +	unsigned int real_period;
> +};
> +
> +static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)

even though this is inlined I would like this to have use the
pwm_sifive_ prefix, too.

> +{
> +	return container_of(c, struct pwm_sifive_ddata, chip);
> +}
> +
> +static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> +				 struct pwm_state *state)
> +{
> +	struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> +	u32 duty;
> +
> +	duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> +
> +	state->period = pwm->real_period;
> +	state->duty_cycle = ((u64)duty * pwm->real_period) >> CMPWIDTH;
> +	state->polarity = PWM_POLARITY_INVERSED;
> +	state->enabled = duty > 0;
> +}
> +
> +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> +			    struct pwm_state *state)
> +{
> +	struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> +	unsigned int duty_cycle;
> +	u32 frac, val;
> +
> +	if (state->polarity != PWM_POLARITY_INVERSED)
> +		return -EINVAL;
> +
> +	if (state->period != pwm->real_period)
> +		return -EINVAL;
> +
> +	duty_cycle = state->duty_cycle;
> +	if (!state->enabled)
> +		duty_cycle = 0;
> +
> +	frac = div_u64((u64)duty_cycle * 0xFFFF, state->period);
> +	frac = min(frac, 0xFFFFU);

If CMPWIDTH was only 8, we'd need 0xff here instead of 0xffff, right? So
better use

	(1 << PWM_SIFIVE_CMPWIDTH) - 1

here, maybe "hidden" behind a cpp define.

In a previous review round I had doubts about the calculation of frac
and I think they were addressed wrongly.
Looking at the figure at the start of chapter 14.9 in the reference
manual: They use (different than the driver here) pwmcmp0=6 and
pwmzerocmp=1. Then a period is 7 clock cycles long and pwmcmpX must be 7
(for X > 0) to yield a 100% signal. So pwmcmpX must be greater than or
equal to the period (in clock cycles).

Here we're using pwmzerocmp=0, still the constraint

	pwmcmpX >= period

must be true for a 100% cycle. (Right?) With pwmzerocmp=0 the period is
0x10000 clock cycles, so pwmcmpX must be >= 0x10000 to yield a 100%
signal. As this is not possible (pwmcmpX is a 16 bit value that can only
hold up to 0xffff) the output cannot yield 100%.

So I think the most correct implementation here is:

	frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 << PWM_SIFIVE_CMPWIDTH) / 2) / period;
	/* Sorry, we cannot do 100% :-( */
	frac = min(frac, (1 << PWM_SIFIVE_CMPWIDTH) - 1);

Thierry: What is the correct rounding strategy? I assumed "round to
nearest" (with round half up). If we want something like clk_round_rate
for PWMs in the future rounding down might be easier to handle.

Note that pwm_sifive_get_state is already (well, or still) using this as
it has:

	state->duty_cycle = ((u64)duty * pwm->real_period) >> CMPWIDTH;

which always yields something smaller than read_period as duty is
smaller than 1 << PWM_SIFIVE_CMPWIDTH.

I tried to understand the IP source at
https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/pwm/PWM.scala
to confirm (or disprove) this theory, but I failed.

Assuming my analysis is true, I'd like to have it documented
prominently in the driver that the hardware cannot generate a 100% duty
cycle.

> +	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> +	val |= (1 << PWM_SIFIVE_PWMCFG_DEGLITCH);

No parenthesis necessary. You might want to consider using the BIT macro
here, maybe already in the definition of PWM_SIFIVE_PWMCFG_DEGLITCH.

> +	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> +	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> +
> +	val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
> +	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> +	pwm_sifive_get_state(chip, dev, state);

Thierry: This changes the pwm_state. Is this how this should be done?

> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops pwm_sifive_ops = {
> +	.get_state = pwm_sifive_get_state,
> +	.apply = pwm_sifive_apply,
> +	.owner = THIS_MODULE,
> +};
> +
> +static struct pwm_device *pwm_sifive_xlate(struct pwm_chip *chip,
> +					   const struct of_phandle_args *args)
> +{
> +	struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> +	struct pwm_device *dev;
> +
> +	if (args->args[0] >= chip->npwm)
> +		return ERR_PTR(-EINVAL);
> +
> +	dev = pwm_request_from_chip(chip, args->args[0], NULL);
> +	if (IS_ERR(dev))
> +		return dev;
> +
> +	/* The period cannot be changed on a per-PWM basis */
> +	dev->args.period = pwm->real_period;
> +	dev->args.polarity = PWM_POLARITY_NORMAL;
> +	if (args->args[1] & PWM_POLARITY_INVERSED)
> +		dev->args.polarity = PWM_POLARITY_INVERSED;

The driver does only support PWM_POLARITY_INVERSED. So I guess this
should be:

	dev->args.polarity = PWM_POLARITY_INVERSED;
	if (!(args->args[1] & PWM_POLARITY_INVERSED))
		dev_warn(dev, "...");

> +
> +	return dev;
> +}
> +
> +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> +				    unsigned long rate)
> +{
> +	/* (1 << (16+scale)) * 10^9/rate = real_period */
> +	unsigned long scale_pow =
> +			(pwm->approx_period * (u64)rate) / NSEC_PER_SEC;
> +	int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);

is this 16 in reality PWM_SIFIVE_CMPWIDTH?

> +	writel((1 << PWM_SIFIVE_PWMCFG_EN_ALWAYS) | (scale <<
> +	       PWM_SIFIVE_PWMCFG_SCALE), pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> +	/* As scale <= 15 the shift operation cannot overflow. */
> +	pwm->real_period = div64_ul(1000000000ULL << (16 + scale), rate);
> +	dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> +}
> +
> +static int pwm_sifive_clock_notifier(struct notifier_block *nb,
> +				     unsigned long event, void *data)
> +{
> +	struct clk_notifier_data *ndata = data;
> +	struct pwm_sifive_ddata *pwm =
> +		container_of(nb, struct pwm_sifive_ddata, notifier);
> +
> +	if (event == POST_RATE_CHANGE)
> +		pwm_sifive_update_clock(pwm, ndata->new_rate);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int pwm_sifive_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = pdev->dev.of_node;
> +	struct pwm_sifive_ddata *pwm;
> +	struct pwm_chip *chip;
> +	struct resource *res;
> +	int ret;
> +
> +	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	chip = &pwm->chip;
> +	chip->dev = dev;
> +	chip->ops = &pwm_sifive_ops;
> +	chip->of_xlate = pwm_sifive_xlate;
> +	chip->of_pwm_n_cells = 2;
> +	chip->base = -1;
> +	chip->npwm = 4;
> +
> +	ret = of_property_read_u32(node, "sifive,period-ns",
> +				   &pwm->approx_period);
> +
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to read sifive,period-ns from DTS\n");
> +		return ret;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pwm->regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(pwm->regs)) {
> +		dev_err(dev, "Unable to map IO resources\n");
> +		return PTR_ERR(pwm->regs);
> +	}
> +
> +	pwm->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(pwm->clk)) {
> +		if (PTR_ERR(pwm->clk) != -EPROBE_DEFER)
> +			dev_err(dev, "Unable to find controller clock\n");
> +		return PTR_ERR(pwm->clk);
> +	}
> +
> +	ret = clk_prepare_enable(pwm->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clock for pwm: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Watch for changes to underlying clock frequency */
> +	pwm->notifier.notifier_call = pwm_sifive_clock_notifier;
> +	ret = clk_notifier_register(pwm->clk, &pwm->notifier);
> +	if (ret) {
> +		dev_err(dev, "failed to register clock notifier: %d\n", ret);
> +		goto disable_clk;
> +	}

Would it make sense to only enable the clock when the pwm is enabled?
(If yes, how does the output behave if the clock is disabled while the
hardware is enabled? I assume the output just freezes at the state where
it happens to be?)

> +	/* Initialize PWM config */
> +	pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> +
> +	ret = pwmchip_add(chip);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot register PWM: %d\n", ret);
> +		goto unregister_clk;
> +	}
> +
> +	platform_set_drvdata(pdev, pwm);
> +	dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
> +
> +	return 0;
> +
> +unregister_clk:
> +	clk_notifier_unregister(pwm->clk, &pwm->notifier);
> +disable_clk:
> +	clk_disable_unprepare(pwm->clk);
> +
> +	return ret;
> +}
> +
> +static int pwm_sifive_remove(struct platform_device *dev)
> +{
> +	struct pwm_sifive_ddata *pwm = platform_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pwmchip_remove(&pwm->chip);
> +	clk_notifier_unregister(pwm->clk, &pwm->notifier);
> +	clk_disable_unprepare(pwm->clk);
> +	return ret;
> +}
> +
> +static const struct of_device_id pwm_sifive_of_match[] = {
> +	{ .compatible = "sifive,pwm0" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, pwm_sifive_of_match);
> +
> +static struct platform_driver pwm_sifive_driver = {
> +	.probe = pwm_sifive_probe,
> +	.remove = pwm_sifive_remove,
> +	.driver = {
> +		.name = "pwm-sifive",
> +		.of_match_table = pwm_sifive_of_match,
> +	},
> +};
> +module_platform_driver(pwm_sifive_driver);
> +
> +MODULE_DESCRIPTION("SiFive PWM driver");
> +MODULE_LICENSE("GPL v2");

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Powered by blists - more mailing lists