Message-ID: <20180430093907.GA2476@ulmo>
Date:   Mon, 30 Apr 2018 11:39:08 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     "Wesley W. Terpstra" <wesley@...ive.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Andreas Färber <afaerber@...e.de>,
        Noralf Trønnes <noralf@...nnes.org>,
        David Lechner <david@...hnology.com>,
        Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
        SZ Lin <sz.lin@...a.com>, linux-pwm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Wesley W . Terpstra" <wesley@...pstra.ca>
Subject: Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

On Fri, Apr 27, 2018 at 03:59:58PM -0700, Wesley W. Terpstra wrote:
> SiFive SoCs can contain one or more PWM IP blocks.
> This adds a driver for them. Tested on the HiFive Unleashed.
> 
> Signed-off-by: Wesley W. Terpstra <wesley@...pstra.ca>
> ---
>  drivers/pwm/Kconfig      |  11 ++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-sifive.c | 259 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 271 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sifive.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 763ee50..49e9824 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -387,6 +387,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
> +	help
> +	  Generic PWM framework driver for SiFive SoCs, such as those
> +          found on the HiFive Unleashed series boards.

There's an inconsistent use of tabs and spaces for indentation here.

> +
> +	  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 0258a74..17c5eb9 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -38,6 +38,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..587d4d4
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,259 @@
> +/*
> + * SiFive PWM driver
> + *
> + * Copyright (C) 2018 SiFive, Inc
> + *
> + * Author: Wesley W. Terpstra <wesley@...ive.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2, as published by
> + * the Free Software Foundation.
> + */

Please include a SPDX license identifier here. That's the preferred way
to specify the license these days.

> +
> +#include <dt-bindings/pwm/pwm.h>

There should be no need to include this in a driver.

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +
> +#define MAX_PWM			4
> +
> +/* Register offsets */
> +#define REG_PWMCFG		0x0
> +#define REG_PWMCOUNT		0x8
> +#define REG_PWMS		0x10
> +#define	REG_PWMCMP0		0x20
> +
> +/* PWMCFG fields */
> +#define BIT_PWM_SCALE		0
> +#define BIT_PWM_STICKY		8
> +#define BIT_PWM_ZERO_ZMP	9
> +#define BIT_PWM_DEGLITCH	10
> +#define BIT_PWM_EN_ALWAYS	12
> +#define BIT_PWM_EN_ONCE		13
> +#define BIT_PWM0_CENTER		16
> +#define BIT_PWM0_GANG		24
> +#define BIT_PWM0_IP		28
> +
> +#define SIZE_PWMCMP		4
> +#define MASK_PWM_SCALE		0xf
> +
> +struct sifive_pwm_device {
> +	struct pwm_chip		chip;
> +	struct notifier_block	notifier;
> +	struct clk		*clk;
> +	void __iomem		*regs;
> +	int			irq;
> +	unsigned int		approx_period;
> +	unsigned int		real_period;
> +};
> +
> +static inline struct sifive_pwm_device *chip_to_sifive(struct pwm_chip *c)
> +{
> +	return container_of(c, struct sifive_pwm_device, chip);
> +}
> +
> +static inline struct sifive_pwm_device *notifier_to_sifive(struct notifier_block *nb)
> +{
> +	return container_of(nb, struct sifive_pwm_device, notifier);
> +}
> +
> +static int sifive_pwm_apply(struct pwm_chip *chip,
> +			    struct pwm_device *dev,
> +			    struct pwm_state *state)
> +{
> +	struct sifive_pwm_device *pwm = chip_to_sifive(chip);
> +	unsigned int duty_cycle;
> +	u32 frac;
> +
> +	duty_cycle = state->duty_cycle;
> +	if (!state->enabled)
> +		duty_cycle = 0;
> +	if (state->polarity == PWM_POLARITY_NORMAL)
> +		duty_cycle = state->period - duty_cycle;
> +
> +	frac = ((u64)duty_cycle << 16) / state->period;
> +	frac = min(frac, 0xFFFFU);
> +
> +	iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
> +
> +	if (state->enabled) {
> +		state->period = pwm->real_period;
> +		state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
> +		if (state->polarity == PWM_POLARITY_NORMAL)
> +			state->duty_cycle = state->period - state->duty_cycle;

That's not the right way to handle polarity. The above often has the
same effect as inversed polarity, but it's not generally the right thing
to do. Please only support polarity if your hardware can actually really
reverse it. The above is something that PWM consumers (such as the
backlight driver) should take care of.

> +	}
> +
> +	return 0;
> +}
> +
> +static void sifive_pwm_get_state(struct pwm_chip *chip,
> +				 struct pwm_device *dev,
> +				 struct pwm_state *state)
> +{
> +	struct sifive_pwm_device *pwm = chip_to_sifive(chip);
> +	unsigned long duty;
> +
> +	duty = ioread32(pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
> +
> +	state->period     = pwm->real_period;
> +	state->duty_cycle = ((u64)duty * pwm->real_period) >> 16;
> +	state->polarity   = PWM_POLARITY_INVERSED;

Is the polarity really reversed by default on this IP?

> +	state->enabled    = duty > 0;
> +}
> +
> +static const struct pwm_ops sifive_pwm_ops = {
> +	.get_state	= sifive_pwm_get_state,
> +	.apply		= sifive_pwm_apply,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip,
> +					   const struct of_phandle_args *args)
> +{
> +	struct sifive_pwm_device *pwm = chip_to_sifive(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_INVERTED)
> +		dev->args.polarity = PWM_POLARITY_INVERSED;
> +
> +	return dev;
> +}
> +
> +static void sifive_pwm_update_clock(struct sifive_pwm_device *pwm,
> +				    unsigned long rate)
> +{
> +	/* (1 << (16+scale)) * 10^9/rate = real_period */
> +	unsigned long scalePow = (pwm->approx_period * (u64)rate) / 1000000000;
> +	int scale = ilog2(scalePow) - 16;
> +
> +	scale = clamp(scale, 0, 0xf);
> +	iowrite32((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE),
> +		  pwm->regs + REG_PWMCFG);
> +
> +	pwm->real_period = (1000000000ULL << (16 + scale)) / rate;
> +}
> +
> +static int sifive_pwm_clock_notifier(struct notifier_block *nb,
> +				     unsigned long event,
> +				     void *data)
> +{
> +	struct clk_notifier_data *ndata = data;
> +	struct sifive_pwm_device *pwm = notifier_to_sifive(nb);
> +
> +	if (event == POST_RATE_CHANGE)
> +		sifive_pwm_update_clock(pwm, ndata->new_rate);
> +
> +	return NOTIFY_OK;
> +}

I don't think this is a good idea. Nobody should be allowed to just
change the PWM period without letting the PWM controller have a say in
the matter. Is this ever really an issue? Does the PWM controller use
a shared clock that changes rate frequently?

> +static int sifive_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = pdev->dev.of_node;
> +	struct sifive_pwm_device *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 = &sifive_pwm_ops;
> +	chip->of_xlate = sifive_pwm_xlate;
> +	chip->of_pwm_n_cells = 2;
> +	chip->base = -1;
> +
> +	ret = of_property_read_u32(node, "sifive,npwm", &chip->npwm);
> +	if (ret < 0 || chip->npwm > MAX_PWM)
> +		chip->npwm = MAX_PWM;

I don't see this documented in the DT bindings. I also don't see a need
for it. Under what circumstances would you want to change this?

> +
> +	ret = of_property_read_u32(node, "sifive,approx-period",
> +				   &pwm->approx_period);
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to read sifive,approx-period from DTS\n");
> +		return -ENOENT;
> +	}
> +
> +	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)) {
> +		dev_err(dev, "Unable to find controller clock\n");
> +		return PTR_ERR(pwm->clk);
> +	}
> +
> +	pwm->irq = platform_get_irq(pdev, 0);
> +	if (pwm->irq < 0) {
> +		dev_err(dev, "Unable to find interrupt\n");
> +		return pwm->irq;
> +	}

You don't do anything with this, why bother retrieving it?

> +
> +	/* Watch for changes to underlying clock frequency */
> +	pwm->notifier.notifier_call = sifive_pwm_clock_notifier;
> +	clk_notifier_register(pwm->clk, &pwm->notifier);
> +
> +	/* Initialize PWM config */
> +	sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk));
> +
> +	ret = pwmchip_add(chip);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot register PWM: %d\n", ret);
> +		clk_notifier_unregister(pwm->clk, &pwm->notifier);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pwm);
> +	dev_info(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);

Please don't do this. Having informational messages like this show up in
the boot log only makes it more difficult to find actually relevant
messages like warnings or errors.

Note that it is expected that a driver will succeed to probe, that's not
something to brag about. Let the user know when something unexpected
happens.

If you really want to have this for debugging purposes, make it a
dev_dbg() message. But I'd still suggest dropping it, there are better
ways to check that your driver was successfully loaded (by inspecting
sysfs for example).

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists