Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

[kbuild test robot]

Hi Wesley,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.17-rc3]
[cannot apply to next-20180503]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Wesley-W-Terpstra/SiFive-SoC-PWM-driver/20180429-174212
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/pwm/pwm-sifive.o: In function `sifive_pwm_apply':
>> pwm-sifive.c:(.text+0xe8): undefined reference to `__aeabi_uldivmod'
   drivers/pwm/pwm-sifive.o: In function `sifive_pwm_update_clock':
   pwm-sifive.c:(.text+0x1f0): undefined reference to `__aeabi_uldivmod'
   pwm-sifive.c:(.text+0x240): undefined reference to `__aeabi_uldivmod'

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

[kbuild test robot]

Hi Wesley,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.17-rc3]
[cannot apply to next-20180503]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Wesley-W-Terpstra/SiFive-SoC-PWM-driver/20180429-174212
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/pwm/pwm-sifive.o: In function `sifive_pwm_apply':
>> pwm-sifive.c:(.text+0xe8): undefined reference to `__aeabi_uldivmod'
   drivers/pwm/pwm-sifive.o: In function `sifive_pwm_update_clock':
   pwm-sifive.c:(.text+0x1f0): undefined reference to `__aeabi_uldivmod'
   pwm-sifive.c:(.text+0x240): undefined reference to `__aeabi_uldivmod'

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

[Wesley Terpstra]

First of all, thank you very much for this detailed review! I really
appreciate it, as this is just the first driver of several we would
like to upstream and getting the reviews front-loaded like this will
really help us improve the subsequent drivers before trying to
upstream them.

On Mon, Apr 30, 2018 at 2:39 AM, Thierry Reding
 wrote:
> Please include a SPDX license identifier here. That's the preferred way
> to specify the license these days.

Done

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

Done

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

I don't know how to declare non-support for polarity. How do I do that?

I still want DTS references to state whether or not the polarity
should be reversed, because the PWM might be connected with either
positive or negative logic to an LED, for example.

>> + state->polarity   = PWM_POLARITY_INVERSED;
> Is the polarity really reversed by default on this IP?

Yes. In the sense that the PWM is low while the counter is less than
the duty-cycle, and high when >= the duty-cycle.

Note that this also means it's possible to achieve a constant high
value, but impossible to achieve a constant low value, other than
approximate.

> 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.

I agree and I intend to fight hard to make sure that future chips don't do this.

> Is this ever really an issue?

Unfortunately, yes. This chip has only one PLL output that controls
almost everything. The core runs at the PLL's output. The bus runs at
half the PLL's output. Each peripheral clock is a divided down version
of the bus clock.

> Does the PWM controller use a shared clock that changes rate frequently?

When you want to change the CPU frequency, you have to update all the
peripheral device clock dividers. It sucks, but that's what has to
happen if the core clock is changed. Fortunately, this is not done
dynamically, but might be done during boot.

For PWM, this is not a big issue, because the period mostly does not
matter as it's broken already. However, for an active SPI transfer,
this can be problematic.

>> + ret = of_property_read_u32(node, "sifive,npwm", >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?

The PWM controller can have less than 4 channels. I could try to probe
how many there are by poking registers, but that seems risky if the
PWM is attached to something.

Shall I just add this to the dt-bindings?

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

Mostly to ensure that it is included in the DTS so that I can rely on
it being there in any future driver updates.

>> + dev_info(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
> Please don't do this.

Removed.

Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

[Wesley Terpstra]

First of all, thank you very much for this detailed review! I really
appreciate it, as this is just the first driver of several we would
like to upstream and getting the reviews front-loaded like this will
really help us improve the subsequent drivers before trying to
upstream them.

On Mon, Apr 30, 2018 at 2:39 AM, Thierry Reding
 wrote:
> Please include a SPDX license identifier here. That's the preferred way
> to specify the license these days.

Done

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

Done

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

I don't know how to declare non-support for polarity. How do I do that?

I still want DTS references to state whether or not the polarity
should be reversed, because the PWM might be connected with either
positive or negative logic to an LED, for example.

>> + state->polarity   = PWM_POLARITY_INVERSED;
> Is the polarity really reversed by default on this IP?

Yes. In the sense that the PWM is low while the counter is less than
the duty-cycle, and high when >= the duty-cycle.

Note that this also means it's possible to achieve a constant high
value, but impossible to achieve a constant low value, other than
approximate.

> 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.

I agree and I intend to fight hard to make sure that future chips don't do this.

> Is this ever really an issue?

Unfortunately, yes. This chip has only one PLL output that controls
almost everything. The core runs at the PLL's output. The bus runs at
half the PLL's output. Each peripheral clock is a divided down version
of the bus clock.

> Does the PWM controller use a shared clock that changes rate frequently?

When you want to change the CPU frequency, you have to update all the
peripheral device clock dividers. It sucks, but that's what has to
happen if the core clock is changed. Fortunately, this is not done
dynamically, but might be done during boot.

For PWM, this is not a big issue, because the period mostly does not
matter as it's broken already. However, for an active SPI transfer,
this can be problematic.

>> + ret = of_property_read_u32(node, "sifive,npwm", >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?

The PWM controller can have less than 4 channels. I could try to probe
how many there are by poking registers, but that seems risky if the
PWM is attached to something.

Shall I just add this to the dt-bindings?

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

Mostly to ensure that it is included in the DTS so that I can rely on
it being there in any future driver updates.

>> + dev_info(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
> Please don't do this.

Removed.

Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

[Thierry Reding]

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 
> ---
>  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 000..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 
> + *
> + * 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 

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

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#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_SCALE0
> +#define BIT_PWM_STICKY   8
> +#define BIT_PWM_ZERO_ZMP 9
> +#define BIT_PWM_DEGLITCH 10
> +#define BIT_PWM_EN_ALWAYS12
> +#define BIT_PWM_EN_ONCE  13
> +#define BIT_PWM0_CENTER  16
> +#define BIT_PWM0_GANG24
> +#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 intapprox_period;
> + unsigned intreal_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, 0xU);
> +
> + 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)
> + 

Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

[Thierry Reding]

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 
> ---
>  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 000..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 
> + *
> + * 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 

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

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#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_SCALE0
> +#define BIT_PWM_STICKY   8
> +#define BIT_PWM_ZERO_ZMP 9
> +#define BIT_PWM_DEGLITCH 10
> +#define BIT_PWM_EN_ALWAYS12
> +#define BIT_PWM_EN_ONCE  13
> +#define BIT_PWM0_CENTER  16
> +#define BIT_PWM0_GANG24
> +#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 intapprox_period;
> + unsigned intreal_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, 0xU);
> +
> + 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 -