[2/2] pwm: sifive: Add a driver for SiFive SoC PWM

Message ID 1547194964-16718-3-git-send-email-yash.shah@sifive.com
State New
Headers show
Series
  • PWM support for HiFive Unleashed
Related show

Commit Message

Yash Shah Jan. 11, 2019, 8:22 a.m.
Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.

Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
[Atish: Various fixes and code cleanup]
Signed-off-by: Atish Patra <atish.patra@wdc.com>
Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
 drivers/pwm/Kconfig      |  10 ++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 257 insertions(+)
 create mode 100644 drivers/pwm/pwm-sifive.c

Comments

Christoph Hellwig Jan. 15, 2019, 3:27 p.m. | #1
From a general code quality point of view this looks fine to me.
I don't really know anything about the PWM subsystem, though:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Uwe Kleine-König Jan. 15, 2019, 10 p.m. | #2
Hello,

On Fri, Jan 11, 2019 at 01:52:44PM +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@sifive.com>
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
>  drivers/pwm/Kconfig      |  10 ++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 257 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sifive.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a8f47df..3bcaf6a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -380,6 +380,16 @@ 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

I'd say add:

	depends on MACH_SIFIVE || COMPILE_TEST

(I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)

> +	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..7fee809
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,246 @@
> +// 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

I wonder if such an instance should be only a single PWM instead of
four. Then you were more flexible with the period lengths (using
pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.

I didn't understand how the deglitch logic works yet. Currently it is
not used which might result in four edges in a single period (which is
bad).

> + */
> +#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 REG_PWMCFG		0x0
> +#define REG_PWMCOUNT		0x8
> +#define REG_PWMS		0x10
> +#define REG_PWMCMP0		0x20

I suggest a common prefix for these defines. Something like
PWM_SIFIVE_

> +
> +/* PWMCFG fields */
> +#define BIT_PWM_SCALE		0
> +#define BIT_PWM_STICKY		8
> +#define BIT_PWM_ZERO_ZMP	9

the manual calls this "pwmzerocmp".

> +#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

Also a common prefix please. Something like PWM_SIFIVE_PWMCFG_ seems
sensible.

> +#define SIZE_PWMCMP		4

Please describe what this constant means. I think this is "ncmp" in the
reference manual. If so, using PWM_SIFIVE_NCMP as name instead seems
adequate.

> +#define MASK_PWM_SCALE		0xf

MASK_PWM_SCALE is unused, please drop it.

> +struct sifive_pwm_device {
> +	struct pwm_chip	chip;
> +	struct notifier_block notifier;
> +	struct clk *clk;
> +	void __iomem *regs;
> +	unsigned int approx_period;
> +	unsigned int real_period;
> +};

I'd call this pwm_sifive_ddata. The prefix because the driver is called
pwm-sifive and ddata because this is driver data and not a device.

> +static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c)
> +{
> +	return container_of(c, struct sifive_pwm_device, chip);
> +}
> +
> +static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> +				 struct pwm_state *state)

given that the driver is called pwm-sifive, please use pwm_sifive as
function prefix.

> +{
> +	struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> +	u32 duty;
> +
> +	duty = readl(pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> +
> +	state->period = pwm->real_period;
> +	state->duty_cycle = ((u64)duty * pwm->real_period) >> 16;

In the reference manual this 16 is called "cmpwidth" I think. If I
understand correctly this might in theory be different from 16, so it
would be great if this would be at least a cpp symbol for now.

> +	state->polarity = PWM_POLARITY_INVERSED;
> +	state->enabled = duty > 0;
> +}
> +
> +static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
> +			    struct pwm_state *state)
> +{
> +	struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> +	unsigned int duty_cycle;
> +	u32 frac;
> +
> +	if (state->polarity != PWM_POLARITY_INVERSED)
> +		return -EINVAL;
> +
> +	duty_cycle = state->duty_cycle;
> +	if (!state->enabled)
> +		duty_cycle = 0;
> +
> +	frac = div_u64((u64)duty_cycle << 16, state->period);
> +	frac = min(frac, 0xFFFFU);

In the previous review round I asked here:

| Also if real_period is for example 10 ms and the consumer requests
| duty=12 ms + period=100 ms, the hardware is configured for duty=1.2 ms +
| period=10 ms, right?

which you confirmed. IMHO this is not acceptable. If the period is
fixed, you should return -EINVAL (I think) if a different period is
requested.

> +	writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);

If you get a constant inactive output with frac=0 and a constant active
output with frac=0xffff the calculation above seems wrong to me.
(A value i written to the pwmcmpX register means a duty cycle of
(i * period / 0xffff). Your calculation assumes a divisor of 0x10000
however.)

> +
> +	if (state->enabled)
> +		sifive_pwm_get_state(chip, dev, state);

@Thierry: Should we bless this correction of state?

> +
> +	return 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 = to_sifive_pwm_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;
> +
> +	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 scale_pow = (pwm->approx_period * (u64)rate) / 1000000000;

NSEC_PER_SEC instead of 1000000000

> +	int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
> +
> +	writel((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE),
> +	       pwm->regs + REG_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 sifive_pwm_clock_notifier(struct notifier_block *nb,
> +				     unsigned long event, void *data)
> +{
> +	struct clk_notifier_data *ndata = data;
> +	struct sifive_pwm_device *pwm =
> +		container_of(nb, struct sifive_pwm_device, notifier);
> +
> +	if (event == POST_RATE_CHANGE)
> +		sifive_pwm_update_clock(pwm, ndata->new_rate);

Does this need locking? (Maybe not with the current state.)

> +
> +	return NOTIFY_OK;
> +}
> +
> +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;
> +	chip->npwm = 4;
> +
> +	ret = of_property_read_u32(node, "sifive,approx-period-ns",
> +				   &pwm->approx_period);
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to read sifive,approx-period 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 = sifive_pwm_clock_notifier;
> +	ret = clk_notifier_register(pwm->clk, &pwm->notifier);
> +	if (ret) {
> +		dev_err(dev, "failed to register clock notifier: %d\n", ret);
> +		clk_disable_unprepare(pwm->clk);
> +		return ret;
> +	}
> +
> +	/* 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);
> +		clk_disable_unprepare(pwm->clk);
> +		return ret;
> +	}

Can you please use a common error path using goto?

> +	platform_set_drvdata(pdev, pwm);
> +	dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
> +
> +	return 0;
> +}
> +
> +static int sifive_pwm_remove(struct platform_device *dev)
> +{
> +	struct sifive_pwm_device *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 sifive_pwm_of_match[] = {
> +	{ .compatible = "sifive,pwm0" },
> +	{ .compatible = "sifive,fu540-c000-pwm" },

Do you really need both compatible strings here?

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sifive_pwm_of_match);
> +
> +static struct platform_driver sifive_pwm_driver = {
> +	.probe = sifive_pwm_probe,
> +	.remove = sifive_pwm_remove,
> +	.driver = {
> +		.name = "pwm-sifive",
> +		.of_match_table = sifive_pwm_of_match,
> +	},
> +};
> +module_platform_driver(sifive_pwm_driver);
> +
> +MODULE_DESCRIPTION("SiFive PWM driver");
> +MODULE_LICENSE("GPL v2");

Best regards
Uwe
Yash Shah Jan. 16, 2019, 11:10 a.m. | #3
On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Fri, Jan 11, 2019 at 01:52:44PM +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@sifive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> >  drivers/pwm/Kconfig      |  10 ++
> >  drivers/pwm/Makefile     |   1 +
> >  drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 257 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-sifive.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index a8f47df..3bcaf6a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -380,6 +380,16 @@ 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
>
> I'd say add:
>
>         depends on MACH_SIFIVE || COMPILE_TEST
>
> (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)

As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
@Paul, Do you have any comments on this?

>
> > +     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..7fee809
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,246 @@
> > +// 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
>
> I wonder if such an instance should be only a single PWM instead of
> four. Then you were more flexible with the period lengths (using
> pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
>
> I didn't understand how the deglitch logic works yet. Currently it is
> not used which might result in four edges in a single period (which is
> bad).

I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
REG_PWMCFG. Will do that.

>
> > + */
> > +#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 REG_PWMCFG           0x0
> > +#define REG_PWMCOUNT         0x8
> > +#define REG_PWMS             0x10
> > +#define REG_PWMCMP0          0x20
>
> I suggest a common prefix for these defines. Something like
> PWM_SIFIVE_

Sure.

>
> > +
> > +/* PWMCFG fields */
> > +#define BIT_PWM_SCALE                0
> > +#define BIT_PWM_STICKY               8
> > +#define BIT_PWM_ZERO_ZMP     9
>
> the manual calls this "pwmzerocmp".

Will fix this.

>
> > +#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
>
> Also a common prefix please. Something like PWM_SIFIVE_PWMCFG_ seems
> sensible.

Sure.

>
> > +#define SIZE_PWMCMP          4
>
> Please describe what this constant means. I think this is "ncmp" in the
> reference manual. If so, using PWM_SIFIVE_NCMP as name instead seems
> adequate.

No, it is not ncmp. It is used to calculate the offset for pwmcmp registers.
I will add the description for the above constant.

>
> > +#define MASK_PWM_SCALE               0xf
>
> MASK_PWM_SCALE is unused, please drop it.

Sure.

>
> > +struct sifive_pwm_device {
> > +     struct pwm_chip chip;
> > +     struct notifier_block notifier;
> > +     struct clk *clk;
> > +     void __iomem *regs;
> > +     unsigned int approx_period;
> > +     unsigned int real_period;
> > +};
>
> I'd call this pwm_sifive_ddata. The prefix because the driver is called
> pwm-sifive and ddata because this is driver data and not a device.

Will be done.

>
> > +static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c)
> > +{
> > +     return container_of(c, struct sifive_pwm_device, chip);
> > +}
> > +
> > +static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> > +                              struct pwm_state *state)
>
> given that the driver is called pwm-sifive, please use pwm_sifive as
> function prefix.

Sure. Will change it for all functions.

>
> > +{
> > +     struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> > +     u32 duty;
> > +
> > +     duty = readl(pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > +
> > +     state->period = pwm->real_period;
> > +     state->duty_cycle = ((u64)duty * pwm->real_period) >> 16;
>
> In the reference manual this 16 is called "cmpwidth" I think. If I
> understand correctly this might in theory be different from 16, so it
> would be great if this would be at least a cpp symbol for now.

I assume you meant to add a macro for cmpwidth and use it here.
Will be done.

>
> > +     state->polarity = PWM_POLARITY_INVERSED;
> > +     state->enabled = duty > 0;
> > +}
> > +
> > +static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
> > +                         struct pwm_state *state)
> > +{
> > +     struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> > +     unsigned int duty_cycle;
> > +     u32 frac;
> > +
> > +     if (state->polarity != PWM_POLARITY_INVERSED)
> > +             return -EINVAL;
> > +
> > +     duty_cycle = state->duty_cycle;
> > +     if (!state->enabled)
> > +             duty_cycle = 0;
> > +
> > +     frac = div_u64((u64)duty_cycle << 16, state->period);
> > +     frac = min(frac, 0xFFFFU);
>
> In the previous review round I asked here:
>
> | Also if real_period is for example 10 ms and the consumer requests
> | duty=12 ms + period=100 ms, the hardware is configured for duty=1.2 ms +
> | period=10 ms, right?
>
> which you confirmed. IMHO this is not acceptable. If the period is
> fixed, you should return -EINVAL (I think) if a different period is
> requested.

Will return -EINVAL on state->period != pwm->real_period

>
> > +     writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
>
> If you get a constant inactive output with frac=0 and a constant active
> output with frac=0xffff the calculation above seems wrong to me.
> (A value i written to the pwmcmpX register means a duty cycle of
> (i * period / 0xffff). Your calculation assumes a divisor of 0x10000
> however.)

Not sure if I get you completely. But, if divisor of 0xffff is your concern then
does the below look ok?

frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period);

>
> > +
> > +     if (state->enabled)
> > +             sifive_pwm_get_state(chip, dev, state);
>
> @Thierry: Should we bless this correction of state?
>
> > +
> > +     return 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 = to_sifive_pwm_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;
> > +
> > +     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 scale_pow = (pwm->approx_period * (u64)rate) / 1000000000;
>
> NSEC_PER_SEC instead of 1000000000

Will be done.

>
> > +     int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
> > +
> > +     writel((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE),
> > +            pwm->regs + REG_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 sifive_pwm_clock_notifier(struct notifier_block *nb,
> > +                                  unsigned long event, void *data)
> > +{
> > +     struct clk_notifier_data *ndata = data;
> > +     struct sifive_pwm_device *pwm =
> > +             container_of(nb, struct sifive_pwm_device, notifier);
> > +
> > +     if (event == POST_RATE_CHANGE)
> > +             sifive_pwm_update_clock(pwm, ndata->new_rate);
>
> Does this need locking? (Maybe not with the current state.)

No. We can add it when required.

>
> > +
> > +     return NOTIFY_OK;
> > +}
> > +
> > +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;
> > +     chip->npwm = 4;
> > +
> > +     ret = of_property_read_u32(node, "sifive,approx-period-ns",
> > +                                &pwm->approx_period);
> > +     if (ret < 0) {
> > +             dev_err(dev, "Unable to read sifive,approx-period 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 = sifive_pwm_clock_notifier;
> > +     ret = clk_notifier_register(pwm->clk, &pwm->notifier);
> > +     if (ret) {
> > +             dev_err(dev, "failed to register clock notifier: %d\n", ret);
> > +             clk_disable_unprepare(pwm->clk);
> > +             return ret;
> > +     }
> > +
> > +     /* 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);
> > +             clk_disable_unprepare(pwm->clk);
> > +             return ret;
> > +     }
>
> Can you please use a common error path using goto?

Sure.

>
> > +     platform_set_drvdata(pdev, pwm);
> > +     dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
> > +
> > +     return 0;
> > +}
> > +
> > +static int sifive_pwm_remove(struct platform_device *dev)
> > +{
> > +     struct sifive_pwm_device *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 sifive_pwm_of_match[] = {
> > +     { .compatible = "sifive,pwm0" },
> > +     { .compatible = "sifive,fu540-c000-pwm" },
>
> Do you really need both compatible strings here?

I believe I can remove the second compatible string.
@Paul, Correct me if I am wrong.

>
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, sifive_pwm_of_match);
> > +
> > +static struct platform_driver sifive_pwm_driver = {
> > +     .probe = sifive_pwm_probe,
> > +     .remove = sifive_pwm_remove,
> > +     .driver = {
> > +             .name = "pwm-sifive",
> > +             .of_match_table = sifive_pwm_of_match,
> > +     },
> > +};
> > +module_platform_driver(sifive_pwm_driver);
> > +
> > +MODULE_DESCRIPTION("SiFive PWM driver");
> > +MODULE_LICENSE("GPL v2");
>
> Best regards
> Uwe

Thanks for the comments.

>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Uwe Kleine-König Jan. 16, 2019, 4:46 p.m. | #4
Hello,

On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Jan 11, 2019 at 01:52:44PM +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@sifive.com>
> > > [Atish: Various fixes and code cleanup]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > ---
> > >  drivers/pwm/Kconfig      |  10 ++
> > >  drivers/pwm/Makefile     |   1 +
> > >  drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 257 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-sifive.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index a8f47df..3bcaf6a 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -380,6 +380,16 @@ 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
> >
> > I'd say add:
> >
> >         depends on MACH_SIFIVE || COMPILE_TEST
> >
> > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> 
> As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> @Paul, Do you have any comments on this?

If this is not going to be available at least protect it by

	depends RISCV || COMPILE_TEST
 
> > I wonder if such an instance should be only a single PWM instead of
> > four. Then you were more flexible with the period lengths (using
> > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
> >
> > I didn't understand how the deglitch logic works yet. Currently it is
> > not used which might result in four edges in a single period (which is
> > bad).
> 
> I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
> REG_PWMCFG. Will do that.

This only works for the first pwm output though.

> > > +struct sifive_pwm_device {
> > > +     struct pwm_chip chip;
> > > +     struct notifier_block notifier;
> > > +     struct clk *clk;
> > > +     void __iomem *regs;
> > > +     unsigned int approx_period;

When thinking about this a bit more, I think the better approach would
be to let the consumer change the period iff there is only one consumer.
Then you can drop that approx-period stuff and the result is more
flexible. (Having said that I still prefer making the driver provide
only a single PWM with the ability to have periods other than powers of
two.)

> > > +     writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> >
> > If you get a constant inactive output with frac=0 and a constant active
> > output with frac=0xffff the calculation above seems wrong to me.
> > (A value i written to the pwmcmpX register means a duty cycle of
> > (i * period / 0xffff). Your calculation assumes a divisor of 0x10000
> > however.)
> 
> Not sure if I get you completely. But, if divisor of 0xffff is your concern then
> does the below look ok?
> 
> frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period);

This works (I think, didn't redo the maths), but I would prefer

	frac = div_u64((u64)duty_cycle * 0xffff, state->period);

for better readability. (Maybe the compiler is even clever enough to see
this can be calculated as you suggested.)

> > > +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 =
> > > +             container_of(nb, struct sifive_pwm_device, notifier);
> > > +
> > > +     if (event == POST_RATE_CHANGE)
> > > +             sifive_pwm_update_clock(pwm, ndata->new_rate);
> >
> > Does this need locking? (Maybe not with the current state.)
> 
> No. We can add it when required.

My thought was, that the clk freq might change while .apply is active.
But given that you only configure the relative duty cycle this is
independent of the actual clk freq.

Best regards
Uwe
Paul Walmsley Jan. 16, 2019, 5:18 p.m. | #5
On Wed, 16 Jan 2019, Uwe Kleine-König wrote:

> On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > index a8f47df..3bcaf6a 100644
> > > > --- a/drivers/pwm/Kconfig
> > > > +++ b/drivers/pwm/Kconfig
> > > > @@ -380,6 +380,16 @@ 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
> > >
> > > I'd say add:
> > >
> > >         depends on MACH_SIFIVE || COMPILE_TEST
> > >
> > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> > 
> > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > @Paul, Do you have any comments on this?
> 
> If this is not going to be available at least protect it by
> 
> 	depends RISCV || COMPILE_TEST

There's nothing RISC-V or SiFive SoC-specific about this driver or IP 
block.  The HDL for this IP block is open-source and posted on Github.  
The IP block and driver would work unchanged on an ARM or MIPS SoC, and in 
fact, SiFive does design ARM-based SoCs as well.  Likewise, any other SoC 
vendor could take the HDL for this IP block from the git tree and 
implement it on their own SoC.

More generally: it's a basic principle of Linux device drivers that they 
should be buildable for any architecture.  The idea here is to prevent 
developers from burying architecture or SoC-specific hacks into the 
driver.  So there shouldn't be any architecture or SoC-specific code in 
any device driver, unless it's abstracted in some way - ideally through a 
common framework.

So from this point of view, neither "depends MACH_SIFIVE" nor "depends 
RISCV" would be appropriate.  Similarly, the equivalents for other 
architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g., 
"MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device 
driver like this one.


- Paul
Uwe Kleine-König Jan. 16, 2019, 5:28 p.m. | #6
On Wed, Jan 16, 2019 at 09:18:45AM -0800, Paul Walmsley wrote:
> On Wed, 16 Jan 2019, Uwe Kleine-König wrote:
> 
> > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > index a8f47df..3bcaf6a 100644
> > > > > --- a/drivers/pwm/Kconfig
> > > > > +++ b/drivers/pwm/Kconfig
> > > > > @@ -380,6 +380,16 @@ 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
> > > >
> > > > I'd say add:
> > > >
> > > >         depends on MACH_SIFIVE || COMPILE_TEST
> > > >
> > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> > > 
> > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > > @Paul, Do you have any comments on this?
> > 
> > If this is not going to be available at least protect it by
> > 
> > 	depends RISCV || COMPILE_TEST
> 
> There's nothing RISC-V or SiFive SoC-specific about this driver or IP 
> block.  The HDL for this IP block is open-source and posted on Github.  
> The IP block and driver would work unchanged on an ARM or MIPS SoC, and in 
> fact, SiFive does design ARM-based SoCs as well.  Likewise, any other SoC 
> vendor could take the HDL for this IP block from the git tree and 
> implement it on their own SoC.
> 
> More generally: it's a basic principle of Linux device drivers that they 
> should be buildable for any architecture.  The idea here is to prevent 
> developers from burying architecture or SoC-specific hacks into the 
> driver.  So there shouldn't be any architecture or SoC-specific code in 
> any device driver, unless it's abstracted in some way - ideally through a 
> common framework.
> 
> So from this point of view, neither "depends MACH_SIFIVE" nor "depends 
> RISCV" would be appropriate.  Similarly, the equivalents for other 
> architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g., 
> "MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device 
> driver like this one.

The dependency serves two purposes:

 a) ensure that the build requirements are fulfilled.
 b) restrict configuration to actually existing machines

When considering b) it doesn't make sense to enable the driver for (say)
a machine that is powered by an ARM SoC by TI. If you still want to
compile test the sifive driver for ARM, enable COMPILE_TEST. That's what
this symbol is there for.

Best regards
Uwe
Paul Walmsley Jan. 16, 2019, 7:29 p.m. | #7
On Wed, 16 Jan 2019, Uwe Kleine-König wrote:

> On Wed, Jan 16, 2019 at 09:18:45AM -0800, Paul Walmsley wrote:
> > On Wed, 16 Jan 2019, Uwe Kleine-König wrote:
> > 
> > > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > > index a8f47df..3bcaf6a 100644
> > > > > > --- a/drivers/pwm/Kconfig
> > > > > > +++ b/drivers/pwm/Kconfig
> > > > > > @@ -380,6 +380,16 @@ 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
> > > > >
> > > > > I'd say add:
> > > > >
> > > > >         depends on MACH_SIFIVE || COMPILE_TEST
> > > > >
> > > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> > > > 
> > > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > > > @Paul, Do you have any comments on this?
> > > 
> > > If this is not going to be available at least protect it by
> > > 
> > > 	depends RISCV || COMPILE_TEST
> > 
> > There's nothing RISC-V or SiFive SoC-specific about this driver or IP 
> > block.  The HDL for this IP block is open-source and posted on Github.  
> > The IP block and driver would work unchanged on an ARM or MIPS SoC, and in 
> > fact, SiFive does design ARM-based SoCs as well.  Likewise, any other SoC 
> > vendor could take the HDL for this IP block from the git tree and 
> > implement it on their own SoC.
> > 
> > More generally: it's a basic principle of Linux device drivers that they 
> > should be buildable for any architecture.  The idea here is to prevent 
> > developers from burying architecture or SoC-specific hacks into the 
> > driver.  So there shouldn't be any architecture or SoC-specific code in 
> > any device driver, unless it's abstracted in some way - ideally through a 
> > common framework.
> > 
> > So from this point of view, neither "depends MACH_SIFIVE" nor "depends 
> > RISCV" would be appropriate.  Similarly, the equivalents for other 
> > architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g., 
> > "MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device 
> > driver like this one.
> 
> The dependency serves two purposes:
> 
>  a) ensure that the build requirements are fulfilled.
>  b) restrict configuration to actually existing machines
> 
> When considering b) it doesn't make sense to enable the driver for (say)
> a machine that is powered by an ARM SoC by TI. If you still want to
> compile test the sifive driver for ARM, enable COMPILE_TEST. That's what
> this symbol is there for.

COMPILE_TEST made slightly more sense before the broad availability of 
open-source soft cores, SoC integration, and IP; and before powerful, 
inexpensive FPGAs and SoCs with FPGA fabrics were common.

Even back then, COMPILE_TEST was starting to look questionable.  IP blocks 
from CPU- and SoC vendor-independent libraries, like DesignWare and the 
Cadence IP libraries, were integrated on SoCs across varying CPU 
architectures.  (Fortunately, looking at the tree, subsystem maintainers 
with DesignWare drivers seem to have largely avoided adding architecture 
or SoC-specific Kconfig restrictions there - and thus have also avoided 
the need for COMPILE_TEST.)  If an unnecessary architecture Kconfig 
dependency was added for a leaf driver, that Kconfig line would either 
need to be patched out by hand, or if present, COMPILE_TEST would need to 
be enabled.

This was less of a problem then.  There were very few FPGA Linux users, 
and they were mostly working on internal proprietary projects. FPGAs that 
could run Linux at a reasonable speed, including MMUs and FPUs, were 
expensive or were missing good tool support.  So most FPGA Linux 
development was restricted to ASIC vendors - the Samsungs, Qualcomms, 
NVIDIAs of the world - for prototyping, and those FPGA platforms never 
made it outside those companies.

The situation is different now.  The major FPGA vendors have inexpensive 
FPGA SoCs with full-featured CPU hard macros.  The development boards can 
be quite affordable (< USD 300 for the Xilinx Ultra96).  There are also 
now open-source SoC HDL implementations - including MMUs, FPUs, and 
peripherals like PWM and UART - for the conventional FPGA market.  These 
can run at mid-1990's clock rates - slow by modern standards but still 
quite usable.

So or vendor-specific IP blocks that are unlikely to ever be reused by 
anyone else, like the NVIDIA or TI PWM blocks, maybe there's still some 
justification for COMPILE_TEST.  But for drivers for open-source, 
SoC-independent peripheral IP blocks - or even for proprietary IP blocks 
from library vendors like Synopsys or Cadence - like this PWM driver, we 
shouldn't add unnecessary architecture, SoC vendor, or COMPILE_TEST 
Kconfig dependencies.  They will just force users to patch the kernel or 
enable COMPILE_TEST for kernels that are actually meant to run on real 
hardware.


- Paul
Yash Shah Jan. 17, 2019, 6:45 a.m. | #8
On Wed, Jan 16, 2019 at 10:16 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Fri, Jan 11, 2019 at 01:52:44PM +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@sifive.com>
> > > > [Atish: Various fixes and code cleanup]
> > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > > ---
> > > >  drivers/pwm/Kconfig      |  10 ++
> > > >  drivers/pwm/Makefile     |   1 +
> > > >  drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 257 insertions(+)
> > > >  create mode 100644 drivers/pwm/pwm-sifive.c
> > > >
> > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > index a8f47df..3bcaf6a 100644
> > > > --- a/drivers/pwm/Kconfig
> > > > +++ b/drivers/pwm/Kconfig
> > > > @@ -380,6 +380,16 @@ 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
> > >
> > > I'd say add:
> > >
> > >         depends on MACH_SIFIVE || COMPILE_TEST
> > >
> > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> >
> > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > @Paul, Do you have any comments on this?
>
> If this is not going to be available at least protect it by
>
>         depends RISCV || COMPILE_TEST
>
> > > I wonder if such an instance should be only a single PWM instead of
> > > four. Then you were more flexible with the period lengths (using
> > > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
> > >
> > > I didn't understand how the deglitch logic works yet. Currently it is
> > > not used which might result in four edges in a single period (which is
> > > bad).
> >
> > I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
> > REG_PWMCFG. Will do that.
>
> This only works for the first pwm output though.
>
> > > > +struct sifive_pwm_device {
> > > > +     struct pwm_chip chip;
> > > > +     struct notifier_block notifier;
> > > > +     struct clk *clk;
> > > > +     void __iomem *regs;
> > > > +     unsigned int approx_period;
>
> When thinking about this a bit more, I think the better approach would
> be to let the consumer change the period iff there is only one consumer.
> Then you can drop that approx-period stuff and the result is more
> flexible. (Having said that I still prefer making the driver provide
> only a single PWM with the ability to have periods other than powers of
> two.)
>

I am not confident about the implementation of the way you are suggesting.
As of now, I am going to stick with the current implementation.

> > > > +     writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > >
> > > If you get a constant inactive output with frac=0 and a constant active
> > > output with frac=0xffff the calculation above seems wrong to me.
> > > (A value i written to the pwmcmpX register means a duty cycle of
> > > (i * period / 0xffff). Your calculation assumes a divisor of 0x10000
> > > however.)
> >
> > Not sure if I get you completely. But, if divisor of 0xffff is your concern then
> > does the below look ok?
> >
> > frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period);
>
> This works (I think, didn't redo the maths), but I would prefer
>
>         frac = div_u64((u64)duty_cycle * 0xffff, state->period);
>
> for better readability. (Maybe the compiler is even clever enough to see
> this can be calculated as you suggested.)

Sure, will multiply it with 0xffff for better readability.

>
> > > > +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 =
> > > > +             container_of(nb, struct sifive_pwm_device, notifier);
> > > > +
> > > > +     if (event == POST_RATE_CHANGE)
> > > > +             sifive_pwm_update_clock(pwm, ndata->new_rate);
> > >
> > > Does this need locking? (Maybe not with the current state.)
> >
> > No. We can add it when required.
>
> My thought was, that the clk freq might change while .apply is active.
> But given that you only configure the relative duty cycle this is
> independent of the actual clk freq.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Uwe Kleine-König Jan. 17, 2019, 7:28 a.m. | #9
Hello,

On Thu, Jan 17, 2019 at 12:15:38PM +0530, Yash Shah wrote:
> On Wed, Jan 16, 2019 at 10:16 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Fri, Jan 11, 2019 at 01:52:44PM +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@sifive.com>
> > > > > [Atish: Various fixes and code cleanup]
> > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > > > ---
> > > > >  drivers/pwm/Kconfig      |  10 ++
> > > > >  drivers/pwm/Makefile     |   1 +
> > > > >  drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 257 insertions(+)
> > > > >  create mode 100644 drivers/pwm/pwm-sifive.c
> > > > >
> > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > index a8f47df..3bcaf6a 100644
> > > > > --- a/drivers/pwm/Kconfig
> > > > > +++ b/drivers/pwm/Kconfig
> > > > > @@ -380,6 +380,16 @@ 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
> > > >
> > > > I'd say add:
> > > >
> > > >         depends on MACH_SIFIVE || COMPILE_TEST
> > > >
> > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> > >
> > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > > @Paul, Do you have any comments on this?
> >
> > If this is not going to be available at least protect it by
> >
> >         depends RISCV || COMPILE_TEST
> >
> > > > I wonder if such an instance should be only a single PWM instead of
> > > > four. Then you were more flexible with the period lengths (using
> > > > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
> > > >
> > > > I didn't understand how the deglitch logic works yet. Currently it is
> > > > not used which might result in four edges in a single period (which is
> > > > bad).
> > >
> > > I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
> > > REG_PWMCFG. Will do that.
> >
> > This only works for the first pwm output though.

I mixed this up with pwmzerocmp; deglitch should work on all four
outputs.

> > > > > +struct sifive_pwm_device {
> > > > > +     struct pwm_chip chip;
> > > > > +     struct notifier_block notifier;
> > > > > +     struct clk *clk;
> > > > > +     void __iomem *regs;
> > > > > +     unsigned int approx_period;
> >
> > When thinking about this a bit more, I think the better approach would
> > be to let the consumer change the period iff there is only one consumer.
> > Then you can drop that approx-period stuff and the result is more
> > flexible. (Having said that I still prefer making the driver provide
> > only a single PWM with the ability to have periods other than powers of
> > two.)
> >
> 
> I am not confident about the implementation of the way you are suggesting.
> As of now, I am going to stick with the current implementation.

The idea is to count the users using the .request and .free callbacks.
Iff there is exactly one, allow changes to period.

But please consider moving to an abstraction that only provides a single
pwm instead.

Best regards
Uwe
Uwe Kleine-König Jan. 17, 2019, 8:19 a.m. | #10
Hello Paul,

On Wed, Jan 16, 2019 at 11:29:35AM -0800, Paul Walmsley wrote:
> COMPILE_TEST made slightly more sense before the broad availability of 
> open-source soft cores, SoC integration, and IP; and before powerful, 
> inexpensive FPGAs and SoCs with FPGA fabrics were common.
> 
> Even back then, COMPILE_TEST was starting to look questionable.  IP blocks 
> from CPU- and SoC vendor-independent libraries, like DesignWare and the 
> Cadence IP libraries, were integrated on SoCs across varying CPU 
> architectures.  (Fortunately, looking at the tree, subsystem maintainers 
> with DesignWare drivers seem to have largely avoided adding architecture 
> or SoC-specific Kconfig restrictions there - and thus have also avoided 
> the need for COMPILE_TEST.)  If an unnecessary architecture Kconfig 
> dependency was added for a leaf driver, that Kconfig line would either 
> need to be patched out by hand, or if present, COMPILE_TEST would need to 
> be enabled.
> 
> This was less of a problem then.  There were very few FPGA Linux users, 
> and they were mostly working on internal proprietary projects. FPGAs that 
> could run Linux at a reasonable speed, including MMUs and FPUs, were 
> expensive or were missing good tool support.  So most FPGA Linux 
> development was restricted to ASIC vendors - the Samsungs, Qualcomms, 
> NVIDIAs of the world - for prototyping, and those FPGA platforms never 
> made it outside those companies.
> 
> The situation is different now.  The major FPGA vendors have inexpensive 
> FPGA SoCs with full-featured CPU hard macros.  The development boards can 
> be quite affordable (< USD 300 for the Xilinx Ultra96).  There are also 
> now open-source SoC HDL implementations - including MMUs, FPUs, and 
> peripherals like PWM and UART - for the conventional FPGA market.  These 
> can run at mid-1990's clock rates - slow by modern standards but still 
> quite usable.

In my eyes it's better to make a driver not possible to enable out of
the box than offering to enable it while it most probably won't be used.

People who configure their own kernel and distribution kernel
maintainers will thank you. (Well ok, they won't notice, so they won't
actually tell you, but anyhow.) I'm a member of the Debian kernel team
and I see how many config items there are that are not explicitly
handled for the various different kernel images. If they were restricted
using COMPILE_TEST to just be possible to enable on machines where it is
known (or at least likely) to make sense that would help. Also when I do
a kernel version bump for a machine with a tailored kernel running (say)
on an ARM/i.MX SoC, I could more easily be careful about the relevant
changes when doing oldconfig if I were not asked about a whole bunch of
drivers that are sure to be irrelevant.

And if someone synthesizes this sifive PWM into an FPGA that is
connected to an OMAP, changing

	depends RISCV || COMPILE_TEST

to something like

	depends RISCV || MACH_OMAP || COMPILE_TEST

is a relatively low effort. (Or we introduce a symbol that tells that
the machine has an FPGA and based on that enable the sifive pwm driver.)
And if a single person comes up saying they need that driver on OMAP
I happily support such a change.

> So or vendor-specific IP blocks that are unlikely to ever be reused by 
> anyone else, like the NVIDIA or TI PWM blocks, maybe there's still some 
> justification for COMPILE_TEST.  But for drivers for open-source, 
> SoC-independent peripheral IP blocks - or even for proprietary IP blocks 
> from library vendors like Synopsys or Cadence - like this PWM driver, we 
> shouldn't add unnecessary architecture, SoC vendor, or COMPILE_TEST 
> Kconfig dependencies.  They will just force users to patch the kernel or 
> enable COMPILE_TEST for kernels that are actually meant to run on real 
> hardware.

I understand that downside. I just think that people who synthesize an
open source core into their machine and then run Linux on it are very
likely easily able to patch the Kconfig file to enable the needed
drivers and tell us. Also if you compare the number of people who hit
this problem to the number of people to have to decide if they need
PWM_SIFIVE and don't need it, I guess there will be an at least
big two-digit factor between them. And as soon as this OMAP machine with
the FPGA becomes mainstream enough that tutorials pop up in the web that
give you a copy&paste template to put the sifive pwm into it, I expect
the dependency is already fixed.

Yes, there is no big harm if you enable this driver and don't need
it. But there are hundreds of drivers, and together they do hurt.
Also if you support a "newbie" configuring their first kernel, this is
much less frightening and gives a much better impression if at least
most of the presented options matter. Also it is easier to pick your pwm
driver if it's presented in a list of ten driver than if the list has
100 items.

There are reasons for both approaches and we need to weight the
respective interests. In my eyes it's clear which is the better
approach, but obviously YMMV. So if you choose to not restrict the
kconfig symbol, at least do it consciously with the arguments for the
other side in mind. And please also consider that your position is
subjective because you're a kernel developer and personally don't care
much if configuring a kernel is difficult to a newcomer.

Best regards
Uwe
Thierry Reding Jan. 21, 2019, 11:30 a.m. | #11
On Tue, Jan 15, 2019 at 11:00:46PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Jan 11, 2019 at 01:52:44PM +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@sifive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> >  drivers/pwm/Kconfig      |  10 ++
> >  drivers/pwm/Makefile     |   1 +
> >  drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 257 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-sifive.c
> > 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index a8f47df..3bcaf6a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -380,6 +380,16 @@ 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
> 
> I'd say add:
> 
> 	depends on MACH_SIFIVE || COMPILE_TEST
> 
> (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> 
> > +	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..7fee809
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,246 @@
> > +// 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
> 
> I wonder if such an instance should be only a single PWM instead of
> four. Then you were more flexible with the period lengths (using
> pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.

I thought this IP only allowed a single period for all PWM channels in
the IP. If so, splitting this into four different devices is going to
complicate things because you'd have to coordinate between all four as
to which period is currently set.

> > +struct sifive_pwm_device {
> > +	struct pwm_chip	chip;
> > +	struct notifier_block notifier;
> > +	struct clk *clk;
> > +	void __iomem *regs;
> > +	unsigned int approx_period;
> > +	unsigned int real_period;
> > +};
> 
> I'd call this pwm_sifive_ddata. The prefix because the driver is called
> pwm-sifive and ddata because this is driver data and not a device.

I don't think there's a need to have an extra suffix. Just call this
sifive_pwm or pwm_sifive. There's no ambiguity in that name and it's
short and crisp.

> > +	if (state->enabled)
> > +		sifive_pwm_get_state(chip, dev, state);
> 
> @Thierry: Should we bless this correction of state?

I'm not sure I understand why this correction is necessary. Is it okay
to request a state to be applied and when we're not able to set that
state we just set anything as close as possible? Sounds a bit risky to
me. What if somebody wants to use this in a case where precision
matters?

Now, if you're saying that there can't be such cases and we should
support this, then why restrict the state correction to when the PWM is
enabled? What's wrong with correcting it in either case?

Thierry
Thierry Reding Jan. 21, 2019, 11:54 a.m. | #12
On Thu, Jan 17, 2019 at 09:19:56AM +0100, Uwe Kleine-König wrote:
> Hello Paul,
> 
> On Wed, Jan 16, 2019 at 11:29:35AM -0800, Paul Walmsley wrote:
> > COMPILE_TEST made slightly more sense before the broad availability of 
> > open-source soft cores, SoC integration, and IP; and before powerful, 
> > inexpensive FPGAs and SoCs with FPGA fabrics were common.
> > 
> > Even back then, COMPILE_TEST was starting to look questionable.  IP blocks 
> > from CPU- and SoC vendor-independent libraries, like DesignWare and the 
> > Cadence IP libraries, were integrated on SoCs across varying CPU 
> > architectures.  (Fortunately, looking at the tree, subsystem maintainers 
> > with DesignWare drivers seem to have largely avoided adding architecture 
> > or SoC-specific Kconfig restrictions there - and thus have also avoided 
> > the need for COMPILE_TEST.)  If an unnecessary architecture Kconfig 
> > dependency was added for a leaf driver, that Kconfig line would either 
> > need to be patched out by hand, or if present, COMPILE_TEST would need to 
> > be enabled.
> > 
> > This was less of a problem then.  There were very few FPGA Linux users, 
> > and they were mostly working on internal proprietary projects. FPGAs that 
> > could run Linux at a reasonable speed, including MMUs and FPUs, were 
> > expensive or were missing good tool support.  So most FPGA Linux 
> > development was restricted to ASIC vendors - the Samsungs, Qualcomms, 
> > NVIDIAs of the world - for prototyping, and those FPGA platforms never 
> > made it outside those companies.
> > 
> > The situation is different now.  The major FPGA vendors have inexpensive 
> > FPGA SoCs with full-featured CPU hard macros.  The development boards can 
> > be quite affordable (< USD 300 for the Xilinx Ultra96).  There are also 
> > now open-source SoC HDL implementations - including MMUs, FPUs, and 
> > peripherals like PWM and UART - for the conventional FPGA market.  These 
> > can run at mid-1990's clock rates - slow by modern standards but still 
> > quite usable.
> 
> In my eyes it's better to make a driver not possible to enable out of
> the box than offering to enable it while it most probably won't be used.

This might sound like a good idea in general, but it's also something
that is pretty much impossible to do. There's just no heuristic that
would allow you to determine with a sufficient degree of probability
that a driver won't be needed. Most of the PCI drivers that are
installed on my workstation aren't used, and I would venture to say
there are a lot of drivers that aren't used in, say, 95% of
installations. That doesn't mean that we should somehow make these
drivers difficult to install, or require someone to patch the kernel
to build them.

> People who configure their own kernel and distribution kernel
> maintainers will thank you. (Well ok, they won't notice, so they won't
> actually tell you, but anyhow.) I'm a member of the Debian kernel team
> and I see how many config items there are that are not explicitly
> handled for the various different kernel images. If they were restricted
> using COMPILE_TEST to just be possible to enable on machines where it is
> known (or at least likely) to make sense that would help. Also when I do
> a kernel version bump for a machine with a tailored kernel running (say)
> on an ARM/i.MX SoC, I could more easily be careful about the relevant
> changes when doing oldconfig if I were not asked about a whole bunch of
> drivers that are sure to be irrelevant.

I think the important thing here is that the driver is "default n". If
you're a developer and build your own kernels, you're pretty likely to
know already if a new kernel that you're installing contains that new
driver that you've been working on or waiting for. In that case you can
easily just enable it manually rather than go through make oldconfig and
wait for it to show up.

As for distribution kernel maintainers, are they really still building a
lot of different kernels? If so, it sounds like most of the multi-
platform efforts have been in vain. I would imagine that if, as a
distribution kernel team, you'd want to carefully evaluate for every
driver whether or not you'd want to include it. I would also imagine
that you'd want to provide your users with the most flexible kernel
possible, so that if they do have a system with an FPGA that you'd make
it possible for them to use pwm-sifive if they choose to synthesize it.

If you really want to create custom builds tailored to a single chip or
board, it's going to be a fair amount of work anyway. I've occasionally
done that in the past and usually just resorted to starting from an
allnoconfig configuration and then working my way up from there.

As for daily work as a developer, when I transition between kernel
versions, or from one linux-next to another, I typically end up doing
the manual equivalent of:

	$ yes '' | make oldconfig

if I know that I'm not interested in any new features. But I also often
just look at what's new because it's interesting to see what's been
going on elsewhere.

Thierry
Uwe Kleine-König Jan. 21, 2019, 1:23 p.m. | #13
On Mon, Jan 21, 2019 at 12:30:39PM +0100, Thierry Reding wrote:
> On Tue, Jan 15, 2019 at 11:00:46PM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Fri, Jan 11, 2019 at 01:52:44PM +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@sifive.com>
> > > [Atish: Various fixes and code cleanup]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > ---
> > >  drivers/pwm/Kconfig      |  10 ++
> > >  drivers/pwm/Makefile     |   1 +
> > >  drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 257 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-sifive.c
> > > 
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index a8f47df..3bcaf6a 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -380,6 +380,16 @@ 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
> > 
> > I'd say add:
> > 
> > 	depends on MACH_SIFIVE || COMPILE_TEST
> > 
> > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> > 
> > > +	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..7fee809
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sifive.c
> > > @@ -0,0 +1,246 @@
> > > +// 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
> > 
> > I wonder if such an instance should be only a single PWM instead of
> > four. Then you were more flexible with the period lengths (using
> > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
> 
> I thought this IP only allowed a single period for all PWM channels in
> the IP. If so, splitting this into four different devices is going to
> complicate things because you'd have to coordinate between all four as
> to which period is currently set.

Take a look at the above link, figure 6 is depicting how this IP works
(page 92 of the pdf). If you have pwmzerocmp=0 (and pwmcmpXgang=0) the
four outputs (pwmcmpXgpio) are independant and the counter only resets
on overflow of pwms. Then you have 4 outputs that can have their
duty_cycle (but not period) set individually. So period is restricted to
a count of clk cycles that is a power of two.

With pwmzerocmp=1, pwmcmp0 resets the counter with the following
effects:

 - pwmcmp0gpio is always 0 (bad?)
 - more finegrained control over the period length as the restriction to
   powers of two is gone (good)

The other three output can then either be used as 3 PWM outputs with the
more flexible (but identical) period or only pwmcmp1gpio is used as a
single output (my favourite) and with pwmcmp2, pwmcmp3 and pwmcmp2gang=1
the output pwmcmp2gpio can be used as a secondary output to implement
stuff that was called "complementary mode" or "Push-pull mode" by
Claudiu Beznea.

> > > +struct sifive_pwm_device {
> > > +	struct pwm_chip	chip;
> > > +	struct notifier_block notifier;
> > > +	struct clk *clk;
> > > +	void __iomem *regs;
> > > +	unsigned int approx_period;
> > > +	unsigned int real_period;
> > > +};
> > 
> > I'd call this pwm_sifive_ddata. The prefix because the driver is called
> > pwm-sifive and ddata because this is driver data and not a device.
> 
> I don't think there's a need to have an extra suffix. Just call this
> sifive_pwm or pwm_sifive. There's no ambiguity in that name and it's
> short and crisp.

fine for me if "_device" goes away.

> > > +	if (state->enabled)
> > > +		sifive_pwm_get_state(chip, dev, state);
> > 
> > @Thierry: Should we bless this correction of state?
> 
> I'm not sure I understand why this correction is necessary. Is it okay
> to request a state to be applied and when we're not able to set that
> state we just set anything as close as possible? Sounds a bit risky to
> me. What if somebody wants to use this in a case where precision
> matters?

There is always rounding involved. Where should we draw a line then?
Consider a parent clk running at 1000000001 Hz. Can you provide a duty
cycle of 1 ps? Of if the clk runs at 500000000 Hz, I can implement even
periods, but not the uneven. Should the driver fail if an uneven period
is requested? What should a user do if setting

	.duty_cycle = 55, .period = 100,

fails? Assume the pwm can set only even duty_cycles? Or maybe duty_cycle
and period must be dividable by 3? Or maybe only periods that are a
power of two are possible? To get both, an API that can actually be
worked with and that provides precision, the driver needs to be allowed
to round (preferably in some defined way that is used for all devices)
and we need a function to determine the result without actually
configuring. That's how it works in the clk_* universe. There is
clk_round_rate and clk_set_rate and the explicit condition that

	clk_set_rate(clk, somerate)

has the same effect as

	clk_set_rate(clk, clk_round_rate(clk, somerate))

and after one of them we have

	clk_get_rate(clk) = clk_round_rate(clk, somerate))

> Now, if you're saying that there can't be such cases and we should
> support this, then why restrict the state correction to when the PWM is
> enabled? What's wrong with correcting it in either case?

Either the correction should be done always or never.

Best regards
Uwe
Uwe Kleine-König Jan. 21, 2019, 3:11 p.m. | #14
Hello Thierry,

On Mon, Jan 21, 2019 at 12:54:55PM +0100, Thierry Reding wrote:
> On Thu, Jan 17, 2019 at 09:19:56AM +0100, Uwe Kleine-König wrote:
> > On Wed, Jan 16, 2019 at 11:29:35AM -0800, Paul Walmsley wrote:
> > > COMPILE_TEST made slightly more sense before the broad availability of 
> > > open-source soft cores, SoC integration, and IP; and before powerful, 
> > > inexpensive FPGAs and SoCs with FPGA fabrics were common.
> > > 
> > > Even back then, COMPILE_TEST was starting to look questionable.  IP blocks 
> > > from CPU- and SoC vendor-independent libraries, like DesignWare and the 
> > > Cadence IP libraries, were integrated on SoCs across varying CPU 
> > > architectures.  (Fortunately, looking at the tree, subsystem maintainers 
> > > with DesignWare drivers seem to have largely avoided adding architecture 
> > > or SoC-specific Kconfig restrictions there - and thus have also avoided 
> > > the need for COMPILE_TEST.)  If an unnecessary architecture Kconfig 
> > > dependency was added for a leaf driver, that Kconfig line would either 
> > > need to be patched out by hand, or if present, COMPILE_TEST would need to 
> > > be enabled.
> > > 
> > > This was less of a problem then.  There were very few FPGA Linux users, 
> > > and they were mostly working on internal proprietary projects. FPGAs that 
> > > could run Linux at a reasonable speed, including MMUs and FPUs, were 
> > > expensive or were missing good tool support.  So most FPGA Linux 
> > > development was restricted to ASIC vendors - the Samsungs, Qualcomms, 
> > > NVIDIAs of the world - for prototyping, and those FPGA platforms never 
> > > made it outside those companies.
> > > 
> > > The situation is different now.  The major FPGA vendors have inexpensive 
> > > FPGA SoCs with full-featured CPU hard macros.  The development boards can 
> > > be quite affordable (< USD 300 for the Xilinx Ultra96).  There are also 
> > > now open-source SoC HDL implementations - including MMUs, FPUs, and 
> > > peripherals like PWM and UART - for the conventional FPGA market.  These 
> > > can run at mid-1990's clock rates - slow by modern standards but still 
> > > quite usable.
> > 
> > In my eyes it's better to make a driver not possible to enable out of
> > the box than offering to enable it while it most probably won't be used.
> 
> This might sound like a good idea in general, but it's also something
> that is pretty much impossible to do. There's just no heuristic that
> would allow you to determine with a sufficient degree of probability
> that a driver won't be needed. Most of the PCI drivers that are
> installed on my workstation aren't used, and I would venture to say
> there are a lot of drivers that aren't used in, say, 95% of
> installations. That doesn't mean that we should somehow make these
> drivers difficult to install, or require someone to patch the kernel
> to build them.

If there is a PCI card that can be plugged into your machine, the
corresponding driver should be selectable for the matching kernel.

The case here is different though. We're talking about a piece of
hardware that up to now only exists in a riscv machine (IIUC). Yes, I'm
aware that the design is publicly available, still I think this driver
should not be available for a person configuring a kernel for their x86
machine. When you (or someone else) comes around claiming that they have
a real use for this driver on a non-riscv architecture I support
expanding the dependency accordingly.

What I want to make aware of is that being able to enable a driver comes
at a (small) cost. Using a too broad dependency is quite usual because
the person who introduces a given symbol usually doesn't have to think
long if it should be enabled for a given kernel config or not; so it's
"only" other people's time that is wasted.

> > People who configure their own kernel and distribution kernel
> > maintainers will thank you. (Well ok, they won't notice, so they won't
> > actually tell you, but anyhow.) I'm a member of the Debian kernel team
> > and I see how many config items there are that are not explicitly
> > handled for the various different kernel images. If they were restricted
> > using COMPILE_TEST to just be possible to enable on machines where it is
> > known (or at least likely) to make sense that would help. Also when I do
> > a kernel version bump for a machine with a tailored kernel running (say)
> > on an ARM/i.MX SoC, I could more easily be careful about the relevant
> > changes when doing oldconfig if I were not asked about a whole bunch of
> > drivers that are sure to be irrelevant.
> 
> I think the important thing here is that the driver is "default n". If
> you're a developer and build your own kernels, you're pretty likely to
> know already if a new kernel that you're installing contains that new
> driver that you've been working on or waiting for.

If you are a developer waiting for your driver you also would not miss
it because it was only selectable for riscv but you're the first who
synthesized it for an arm machine. So there is only little damage.

> In that case you can easily just enable it manually rather than go
> through make oldconfig and wait for it to show up.
> 
> As for distribution kernel maintainers, are they really still building a
> lot of different kernels?

In the Debian kernel there are as of now 39 kernel images. Some of them
only differ by stuff that is irrelevant for driver selection (like rt).
But apart from these there is still mostly only a single image available
for a given machine because multi-platform efforts are not good enough
to allow cross-architecture kernels. Or kernels that can handle both big
and little endian.

> If so, it sounds like most of the multi-platform efforts have been in
> vain. I would imagine that if, as a distribution kernel team, you'd
> want to carefully evaluate for every driver whether or not you'd want
> to include it.

The Debian distro kernel I'm running has a .config file with 7317 config
items (grep CONFIG /boot/config-$(uname -r) | wc -l). 1922 of them are
disabled (grep is\ not\ set /boot/config-$(uname -r) | wc -l). If you
find the time to only go through the 1922 options that are disabled for
this amd64 kernel, tell me, I provide you the config file.

> I would also imagine that you'd want to provide your users with the
> most flexible kernel possible, so that if they do have a system with
> an FPGA that you'd make it possible for them to use pwm-sifive if they
> choose to synthesize it.

I would today probably choose to not enable pwm-sifive for Debian on a
non-riscv arch kernel because nobody told to have a use for it and the
cost of enabling the driver is that it takes hard disk space for several
thousand machines without any use.

And given that we here have the knowledge that up to now PWM_SIFIVE=[ym]
is only usable on riscv, I think we should put that into the
condition that makes the driver selectable. It's not hard for a distro
maintainer to find the same information; say it takes 5 minutes (that
works here because the driver under discussion has a link to the
reference manual in the header). Multiply that by the count of drivers.

> If you really want to create custom builds tailored to a single chip or
> board, it's going to be a fair amount of work anyway.

Ah right, this is a hard job, so it doesn't make a difference when we
make it a little bit harder?

> I've occasionally done that in the past and usually just resorted to
> starting from an allnoconfig configuration and then working my way up
> from there.
> 
> As for daily work as a developer, when I transition between kernel
> versions, or from one linux-next to another, I typically end up doing
> the manual equivalent of:
> 
> 	$ yes '' | make oldconfig

I usually start with $(make oldconfig) without the yes part. But when I
get 10th question in a row that is completely irrelevant for my target
machine because it doesn't have the graphics core that is found only on
Samsung ARM SoCs or the nand controllers that can only be found on some
powerpc machines I'm annoyed and I press and hold the Enter key.
I'm well aware that I'm missing some interesting stuff then, though,
which is sad.

> if I know that I'm not interested in any new features. But I also often
> just look at what's new because it's interesting to see what's been
> going on elsewhere.

The kernel config as a way to see what is going on elsewhere is a use
case that is broken by my preferred approach. Be warned that you already
miss stuff today if you only look there.

Best regards
Uwe

Patch

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index a8f47df..3bcaf6a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -380,6 +380,16 @@  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.
+
+	  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..7fee809
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,246 @@ 
+// 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 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;
+	unsigned int approx_period;
+	unsigned int real_period;
+};
+
+static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c)
+{
+	return container_of(c, struct sifive_pwm_device, chip);
+}
+
+static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev,
+				 struct pwm_state *state)
+{
+	struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+	u32 duty;
+
+	duty = readl(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;
+	state->enabled = duty > 0;
+}
+
+static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
+			    struct pwm_state *state)
+{
+	struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+	unsigned int duty_cycle;
+	u32 frac;
+
+	if (state->polarity != PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	duty_cycle = state->duty_cycle;
+	if (!state->enabled)
+		duty_cycle = 0;
+
+	frac = div_u64((u64)duty_cycle << 16, state->period);
+	frac = min(frac, 0xFFFFU);
+
+	writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
+
+	if (state->enabled)
+		sifive_pwm_get_state(chip, dev, state);
+
+	return 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 = to_sifive_pwm_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;
+
+	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 scale_pow = (pwm->approx_period * (u64)rate) / 1000000000;
+	int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
+
+	writel((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE),
+	       pwm->regs + REG_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 sifive_pwm_clock_notifier(struct notifier_block *nb,
+				     unsigned long event, void *data)
+{
+	struct clk_notifier_data *ndata = data;
+	struct sifive_pwm_device *pwm =
+		container_of(nb, struct sifive_pwm_device, notifier);
+
+	if (event == POST_RATE_CHANGE)
+		sifive_pwm_update_clock(pwm, ndata->new_rate);
+
+	return NOTIFY_OK;
+}
+
+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;
+	chip->npwm = 4;
+
+	ret = of_property_read_u32(node, "sifive,approx-period-ns",
+				   &pwm->approx_period);
+	if (ret < 0) {
+		dev_err(dev, "Unable to read sifive,approx-period 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 = sifive_pwm_clock_notifier;
+	ret = clk_notifier_register(pwm->clk, &pwm->notifier);
+	if (ret) {
+		dev_err(dev, "failed to register clock notifier: %d\n", ret);
+		clk_disable_unprepare(pwm->clk);
+		return ret;
+	}
+
+	/* 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);
+		clk_disable_unprepare(pwm->clk);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pwm);
+	dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
+
+	return 0;
+}
+
+static int sifive_pwm_remove(struct platform_device *dev)
+{
+	struct sifive_pwm_device *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 sifive_pwm_of_match[] = {
+	{ .compatible = "sifive,pwm0" },
+	{ .compatible = "sifive,fu540-c000-pwm" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sifive_pwm_of_match);
+
+static struct platform_driver sifive_pwm_driver = {
+	.probe = sifive_pwm_probe,
+	.remove = sifive_pwm_remove,
+	.driver = {
+		.name = "pwm-sifive",
+		.of_match_table = sifive_pwm_of_match,
+	},
+};
+module_platform_driver(sifive_pwm_driver);
+
+MODULE_DESCRIPTION("SiFive PWM driver");
+MODULE_LICENSE("GPL v2");