[v4,3/3] pwm: Add support for Xilinx AXI Timer

Commit Message

Sean Anderson May 28, 2021, 9:45 p.m. UTC

This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
found on Xilinx FPGAs.  At the moment clock control is very basic: we
just enable the clock during probe and pin the frequency. In the future,
someone could add support for disabling the clock when not in use.

This driver was written with reference to Xilinx DS764 for v1.03.a [1].

[1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf

Signed-off-by: Sean Anderson <[e-mail]>
---

Changes in v4:
- Remove references to properties which are not good enough for Linux.
- Don't use volatile in read/write replacements. Some arches have it and
  some don't.
- Put common timer properties into their own struct to better reuse
  code.

Changes in v3:
- Add clockevent and clocksource support
- Rewrite probe to only use a device_node, since timers may need to be
  initialized before we have proper devices. This does bloat the code a bit
  since we can no longer rely on helpers such as dev_err_probe. We also
  cannot rely on device resources being free'd on failure, so we must free
  them manually.
- We now access registers through xilinx_timer_(read|write). This allows us
  to deal with endianness issues, as originally seen in the microblaze
  driver. CAVEAT EMPTOR: I have not tested this on big-endian!
- Remove old microblaze driver

Changes in v2:
- Don't compile this module by default for arm64
- Add dependencies on COMMON_CLK and HAS_IOMEM
- Add comment explaining why we depend on !MICROBLAZE
- Add comment describing device
- Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
- Use NSEC_TO_SEC instead of defining our own
- Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
- Cast dividends to u64 to avoid overflow
- Check for over- and underflow when calculating TLR
- Set xilinx_pwm_ops.owner
- Don't set pwmchip.base to -1
- Check range of xlnx,count-width
- Ensure the clock is always running when the pwm is registered
- Remove debugfs file :l
- Report errors with dev_error_probe

 drivers/mfd/Makefile     |   2 +-
 drivers/pwm/Kconfig      |  12 +++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-xilinx.c | 219 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 233 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pwm/pwm-xilinx.c

Comments

Sean Anderson June 10, 2021, 4:15 p.m. UTC | #1

ping

Michal, I know you had some objections to previous spins of this series
which I have tried to address this time around. So I would appreciate if
you could review this.

--Sean

On 5/28/21 5:45 PM, Sean Anderson wrote:
 > This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
 > found on Xilinx FPGAs.  At the moment clock control is very basic: we
 > just enable the clock during probe and pin the frequency. In the future,
 > someone could add support for disabling the clock when not in use.
 >
 > This driver was written with reference to Xilinx DS764 for v1.03.a [1].
 >
 > [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
 >
 > Signed-off-by: Sean Anderson <[e-mail]>
 > ---
 >
 > Changes in v4:
 > - Remove references to properties which are not good enough for Linux.
 > - Don't use volatile in read/write replacements. Some arches have it and
 >    some don't.
 > - Put common timer properties into their own struct to better reuse
 >    code.
 >
 > Changes in v3:
 > - Add clockevent and clocksource support
 > - Rewrite probe to only use a device_node, since timers may need to be
 >    initialized before we have proper devices. This does bloat the code a bit
 >    since we can no longer rely on helpers such as dev_err_probe. We also
 >    cannot rely on device resources being free'd on failure, so we must free
 >    them manually.
 > - We now access registers through xilinx_timer_(read|write). This allows us
 >    to deal with endianness issues, as originally seen in the microblaze
 >    driver. CAVEAT EMPTOR: I have not tested this on big-endian!
 > - Remove old microblaze driver
 >
 > Changes in v2:
 > - Don't compile this module by default for arm64
 > - Add dependencies on COMMON_CLK and HAS_IOMEM
 > - Add comment explaining why we depend on !MICROBLAZE
 > - Add comment describing device
 > - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
 > - Use NSEC_TO_SEC instead of defining our own
 > - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
 > - Cast dividends to u64 to avoid overflow
 > - Check for over- and underflow when calculating TLR
 > - Set xilinx_pwm_ops.owner
 > - Don't set pwmchip.base to -1
 > - Check range of xlnx,count-width
 > - Ensure the clock is always running when the pwm is registered
 > - Remove debugfs file :l
 > - Report errors with dev_error_probe
 >
 >   drivers/mfd/Makefile     |   2 +-
 >   drivers/pwm/Kconfig      |  12 +++
 >   drivers/pwm/Makefile     |   1 +
 >   drivers/pwm/pwm-xilinx.c | 219 +++++++++++++++++++++++++++++++++++++++
 >   4 files changed, 233 insertions(+), 1 deletion(-)
 >   create mode 100644 drivers/pwm/pwm-xilinx.c
 >
 > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
 > index f0f9fbdde7dc..89769affe251 100644
 > --- a/drivers/mfd/Makefile
 > +++ b/drivers/mfd/Makefile
 > @@ -269,6 +269,6 @@ obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
 >   obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
 >   obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
 >
 > -ifneq ($(CONFIG_XILINX_TIMER),)
 > +ifneq ($(CONFIG_PWM_XILINX)$(CONFIG_XILINX_TIMER),)
 >   obj-y				+= xilinx-timer.o
 >   endif
 > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
 > index 8ae68d6203fb..ebf8d9014758 100644
 > --- a/drivers/pwm/Kconfig
 > +++ b/drivers/pwm/Kconfig
 > @@ -620,4 +620,16 @@ config PWM_VT8500
 >   	  To compile this driver as a module, choose M here: the module
 >   	  will be called pwm-vt8500.
 >
 > +config PWM_XILINX
 > +	tristate "Xilinx AXI Timer PWM support"
 > +	depends on HAS_IOMEM && COMMON_CLK
 > +	help
 > +	  PWM driver for Xilinx LogiCORE IP AXI timers. This timer is
 > +	  typically a soft core which may be present in Xilinx FPGAs.
 > +	  This device may also be present in Microblaze soft processors.
 > +	  If you don't have this IP in your design, choose N.
 > +
 > +	  To compile this driver as a module, choose M here: the module
 > +	  will be called pwm-xilinx.
 > +
 >   endif
 > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
 > index d43b1e17e8e1..655df169b895 100644
 > --- a/drivers/pwm/Makefile
 > +++ b/drivers/pwm/Makefile
 > @@ -58,3 +58,4 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 >   obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
 >   obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
 >   obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
 > +obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
 > diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c
 > new file mode 100644
 > index 000000000000..f05321496717
 > --- /dev/null
 > +++ b/drivers/pwm/pwm-xilinx.c
 > @@ -0,0 +1,219 @@
 > +// SPDX-License-Identifier: GPL-2.0+
 > +/*
 > + * Copyright (C) 2021 Sean Anderson <[e-mail]>
 > + *
 > + * Hardware limitations:
 > + * - When changing both duty cycle and period, we may end up with one cycle
 > + *   with the old duty cycle and the new period.
 > + * - Cannot produce 100% duty cycle.
 > + * - Only produces "normal" output.
 > + */
 > +
 > +#include <linux/clk.h>
 > +#include <linux/clk-provider.h>
 > +#include <linux/device.h>
 > +#include <linux/module.h>
 > +#include <linux/mfd/xilinx-timer.h>
 > +#include <linux/platform_device.h>
 > +#include <linux/pwm.h>
 > +
 > +/*
 > + * The idea here is to capture whether the PWM is actually running (e.g.
 > + * because we or the bootloader set it up) and we need to be careful to ensure
 > + * we don't cause a glitch. According to the data sheet, to enable the PWM we
 > + * need to
 > + *
 > + * - Set both timers to generate mode (MDT=1)
 > + * - Set both timers to PWM mode (PWMA=1)
 > + * - Enable the generate out signals (GENT=1)
 > + *
 > + * In addition,
 > + *
 > + * - The timer must be running (ENT=1)
 > + * - The timer must auto-reload TLR into TCR (ARHT=1)
 > + * - We must not be in the process of loading TLR into TCR (LOAD=0)
 > + * - Cascade mode must be disabled (CASC=0)
 > + *
 > + * If any of these differ from usual, then the PWM is either disabled, or is
 > + * running in a mode that this driver does not support.
 > + */
 > +#define TCSR_PWM_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)
 > +#define TCSR_PWM_CLEAR (TCSR_MDT | TCSR_LOAD)
 > +#define TCSR_PWM_MASK (TCSR_PWM_SET | TCSR_PWM_CLEAR)
 > +
 > +struct xilinx_pwm_device {
 > +	struct pwm_chip chip;
 > +	struct xilinx_timer_priv priv;
 > +};
 > +
 > +static inline struct xilinx_timer_priv
 > +*xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
 > +{
 > +	return &container_of(chip, struct xilinx_pwm_device, chip)->priv;
 > +}
 > +
 > +static bool xilinx_timer_pwm_enabled(u32 tcsr0, u32 tcsr1)
 > +{
 > +	return ((TCSR_PWM_MASK | TCSR_CASC) & tcsr0) == TCSR_PWM_SET &&
 > +		(TCSR_PWM_MASK & tcsr1) == TCSR_PWM_SET;
 > +}
 > +
 > +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
 > +			    const struct pwm_state *state)
 > +{
 > +	int ret;
 > +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
 > +	u32 tlr0, tlr1;
 > +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
 > +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
 > +	bool enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
 > +
 > +	if (state->polarity != PWM_POLARITY_NORMAL)
 > +		return -EINVAL;
 > +
 > +	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
 > +	if (ret)
 > +		return ret;
 > +
 > +	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
 > +	if (ret)
 > +		return ret;
 > +
 > +	xilinx_timer_write(priv, tlr0, TLR0);
 > +	xilinx_timer_write(priv, tlr1, TLR1);
 > +
 > +	if (state->enabled) {
 > +		/* Only touch the TCSRs if we aren't already running */
 > +		if (!enabled) {
 > +			/* Load TLR into TCR */
 > +			xilinx_timer_write(priv, tcsr0 | TCSR_LOAD, TCSR0);
 > +			xilinx_timer_write(priv, tcsr1 | TCSR_LOAD, TCSR1);
 > +			/* Enable timers all at once with ENALL */
 > +			tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
 > +			tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
 > +			xilinx_timer_write(priv, tcsr0, TCSR0);
 > +			xilinx_timer_write(priv, tcsr1, TCSR1);
 > +		}
 > +	} else {
 > +		xilinx_timer_write(priv, 0, TCSR0);
 > +		xilinx_timer_write(priv, 0, TCSR1);
 > +	}
 > +
 > +	return 0;
 > +}
 > +
 > +static void xilinx_pwm_get_state(struct pwm_chip *chip,
 > +				 struct pwm_device *unused,
 > +				 struct pwm_state *state)
 > +{
 > +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
 > +	u32 tlr0 = xilinx_timer_read(priv, TLR0);
 > +	u32 tlr1 = xilinx_timer_read(priv, TLR1);
 > +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
 > +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
 > +
 > +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
 > +	state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
 > +	state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
 > +	state->polarity = PWM_POLARITY_NORMAL;
 > +}
 > +
 > +static const struct pwm_ops xilinx_pwm_ops = {
 > +	.apply = xilinx_pwm_apply,
 > +	.get_state = xilinx_pwm_get_state,
 > +	.owner = THIS_MODULE,
 > +};
 > +
 > +static int xilinx_timer_probe(struct platform_device *pdev)
 > +{
 > +	int ret;
 > +	struct device *dev = &pdev->dev;
 > +	struct device_node *np = dev->of_node;
 > +	struct xilinx_timer_priv *priv;
 > +	struct xilinx_pwm_device *pwm;
 > +	u32 pwm_cells, one_timer;
 > +
 > +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
 > +	if (ret == -EINVAL)
 > +		return -ENODEV;
 > +	else if (ret)
 > +		return dev_err_probe(dev, ret, "#pwm-cells\n");
 > +	else if (pwm_cells)
 > +		return dev_err_probe(dev, -EINVAL, "#pwm-cells must be 0\n");
 > +
 > +	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
 > +	if (!pwm)
 > +		return -ENOMEM;
 > +	platform_set_drvdata(pdev, pwm);
 > +	priv = &pwm->priv;
 > +
 > +	priv->regs = devm_platform_ioremap_resource(pdev, 0);
 > +	if (IS_ERR(priv->regs))
 > +		return PTR_ERR(priv->regs);
 > +
 > +	ret = xilinx_timer_common_init(np, priv, &one_timer);
 > +	if (ret)
 > +		return ret;
 > +
 > +	if (one_timer)
 > +		return dev_err_probe(dev, -EINVAL,
 > +				     "two timers required for PWM mode\n");
 > +
 > +	/*
 > +	 * The polarity of the generate outputs must be active high for PWM
 > +	 * mode to work. We could determine this from the device tree, but
 > +	 * alas, such properties are not allowed to be used.
 > +	 */
 > +
 > +	priv->clk = devm_clk_get(dev, "s_axi_aclk");
 > +	if (IS_ERR(priv->clk))
 > +		return dev_err_probe(dev, PTR_ERR(priv->clk), "clock\n");
 > +
 > +	ret = clk_prepare_enable(priv->clk);
 > +	if (ret)
 > +		return dev_err_probe(dev, ret, "clock enable failed\n");
 > +	clk_rate_exclusive_get(priv->clk);
 > +
 > +	pwm->chip.dev = dev;
 > +	pwm->chip.ops = &xilinx_pwm_ops;
 > +	pwm->chip.npwm = 1;
 > +	ret = pwmchip_add(&pwm->chip);
 > +	if (ret) {
 > +		clk_rate_exclusive_put(priv->clk);
 > +		clk_disable_unprepare(priv->clk);
 > +		return dev_err_probe(dev, ret, "could not register pwm chip\n");
 > +	}
 > +
 > +	return 0;
 > +}
 > +
 > +static int xilinx_timer_remove(struct platform_device *pdev)
 > +{
 > +	struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
 > +
 > +	pwmchip_remove(&pwm->chip);
 > +	clk_rate_exclusive_put(pwm->priv.clk);
 > +	clk_disable_unprepare(pwm->priv.clk);
 > +	return 0;
 > +}
 > +
 > +static const struct of_device_id xilinx_timer_of_match[] = {
 > +	{ .compatible = "xlnx,xps-timer-1.00.a", },
 > +	{ .compatible = "xlnx,axi-timer-2.0" },
 > +	{},
 > +};
 > +MODULE_DEVICE_TABLE(of, xilinx_timer_of_match);
 > +
 > +static struct platform_driver xilinx_timer_driver = {
 > +	.probe = xilinx_timer_probe,
 > +	.remove = xilinx_timer_remove,
 > +	.driver = {
 > +		.name = "xilinx-timer",
 > +		.of_match_table = of_match_ptr(xilinx_timer_of_match),
 > +	},
 > +};
 > +module_platform_driver(xilinx_timer_driver);
 > +
 > +MODULE_ALIAS("platform:xilinx-timer");
 > +MODULE_DESCRIPTION("Xilinx LogiCORE IP AXI Timer driver");
 > +MODULE_LICENSE("GPL v2");
 >

Uwe Kleine-König June 25, 2021, 6:19 a.m. UTC | #2

On Fri, May 28, 2021 at 05:45:22PM -0400, Sean Anderson wrote:
> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
> found on Xilinx FPGAs.  At the moment clock control is very basic: we
> just enable the clock during probe and pin the frequency. In the future,
> someone could add support for disabling the clock when not in use.
> 
> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
> 
> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
> 
> Signed-off-by: Sean Anderson <[e-mail]>
> ---
> 
> Changes in v4:
> - Remove references to properties which are not good enough for Linux.
> - Don't use volatile in read/write replacements. Some arches have it and
>   some don't.
> - Put common timer properties into their own struct to better reuse
>   code.
> 
> Changes in v3:
> - Add clockevent and clocksource support
> - Rewrite probe to only use a device_node, since timers may need to be
>   initialized before we have proper devices. This does bloat the code a bit
>   since we can no longer rely on helpers such as dev_err_probe. We also
>   cannot rely on device resources being free'd on failure, so we must free
>   them manually.
> - We now access registers through xilinx_timer_(read|write). This allows us
>   to deal with endianness issues, as originally seen in the microblaze
>   driver. CAVEAT EMPTOR: I have not tested this on big-endian!
> - Remove old microblaze driver
> 
> Changes in v2:
> - Don't compile this module by default for arm64
> - Add dependencies on COMMON_CLK and HAS_IOMEM
> - Add comment explaining why we depend on !MICROBLAZE
> - Add comment describing device
> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
> - Use NSEC_TO_SEC instead of defining our own
> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
> - Cast dividends to u64 to avoid overflow
> - Check for over- and underflow when calculating TLR
> - Set xilinx_pwm_ops.owner
> - Don't set pwmchip.base to -1
> - Check range of xlnx,count-width
> - Ensure the clock is always running when the pwm is registered
> - Remove debugfs file :l
> - Report errors with dev_error_probe
> 
>  drivers/mfd/Makefile     |   2 +-
>  drivers/pwm/Kconfig      |  12 +++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-xilinx.c | 219 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 233 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pwm/pwm-xilinx.c
> 
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f0f9fbdde7dc..89769affe251 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -269,6 +269,6 @@ obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
>  obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
>  obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
>  
> -ifneq ($(CONFIG_XILINX_TIMER),)
> +ifneq ($(CONFIG_PWM_XILINX)$(CONFIG_XILINX_TIMER),)
>  obj-y				+= xilinx-timer.o
>  endif
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 8ae68d6203fb..ebf8d9014758 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -620,4 +620,16 @@ config PWM_VT8500
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-vt8500.
>  
> +config PWM_XILINX
> +	tristate "Xilinx AXI Timer PWM support"
> +	depends on HAS_IOMEM && COMMON_CLK
> +	help
> +	  PWM driver for Xilinx LogiCORE IP AXI timers. This timer is
> +	  typically a soft core which may be present in Xilinx FPGAs.
> +	  This device may also be present in Microblaze soft processors.
> +	  If you don't have this IP in your design, choose N.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-xilinx.
> +
>  endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index d43b1e17e8e1..655df169b895 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -58,3 +58,4 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>  obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> +obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
> diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c
> new file mode 100644
> index 000000000000..f05321496717
> --- /dev/null
> +++ b/drivers/pwm/pwm-xilinx.c
> @@ -0,0 +1,219 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Sean Anderson <[e-mail]>
> + *
> + * Hardware limitations:
> + * - When changing both duty cycle and period, we may end up with one cycle
> + *   with the old duty cycle and the new period.

That means it doesn't reset the counter when a new period is set, right?

> + * - Cannot produce 100% duty cycle.

Can it produce a 0% duty cycle? Below you're calling
xilinx_timer_tlr_period(..., ..., ..., 0) then which returns -ERANGE.

> + * - Only produces "normal" output.

Does the output emit a low level when it's disabled?

> + */
> +
> [...]
> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> +			    const struct pwm_state *state)
> +{
> +	int ret;
> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +	u32 tlr0, tlr1;
> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
> +	bool enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
> +	if (ret)
> +		return ret;

The implementation of xilinx_timer_tlr_period (in patch 2/3) returns
-ERANGE for big periods. The good behaviour to implement is to cap to
the biggest period possible in this case.

Also note that state->period is an u64 but it is casted to unsigned int
as this is the type of the forth parameter of xilinx_timer_tlr_period.

> +	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
> +	if (ret)
> +		return ret;
> +
> +	xilinx_timer_write(priv, tlr0, TLR0);
> +	xilinx_timer_write(priv, tlr1, TLR1);
> +
> +	if (state->enabled) {
> +		/* Only touch the TCSRs if we aren't already running */
> +		if (!enabled) {
> +			/* Load TLR into TCR */
> +			xilinx_timer_write(priv, tcsr0 | TCSR_LOAD, TCSR0);
> +			xilinx_timer_write(priv, tcsr1 | TCSR_LOAD, TCSR1);
> +			/* Enable timers all at once with ENALL */
> +			tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
> +			tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
> +			xilinx_timer_write(priv, tcsr0, TCSR0);
> +			xilinx_timer_write(priv, tcsr1, TCSR1);
> +		}
> +	} else {
> +		xilinx_timer_write(priv, 0, TCSR0);
> +		xilinx_timer_write(priv, 0, TCSR1);
> +	}
> +
> +	return 0;
> +}
> +
> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
> +				 struct pwm_device *unused,
> +				 struct pwm_state *state)
> +{
> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +	u32 tlr0 = xilinx_timer_read(priv, TLR0);
> +	u32 tlr1 = xilinx_timer_read(priv, TLR1);
> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
> +
> +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
> +	state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
> +	state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> +	state->polarity = PWM_POLARITY_NORMAL;

Are the values returned here sensible if the hardware isn't in PWM mode?

> +}
> +
> +static const struct pwm_ops xilinx_pwm_ops = {
> +	.apply = xilinx_pwm_apply,
> +	.get_state = xilinx_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int xilinx_timer_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct xilinx_timer_priv *priv;
> +	struct xilinx_pwm_device *pwm;
> +	u32 pwm_cells, one_timer;
> +
> +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
> +	if (ret == -EINVAL)
> +		return -ENODEV;
> +	else if (ret)
> +		return dev_err_probe(dev, ret, "#pwm-cells\n");

Very sparse error message.

> +	else if (pwm_cells)
> +		return dev_err_probe(dev, -EINVAL, "#pwm-cells must be 0\n");

What is the rationale here to not support #pwm-cells = <2>?

> +	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, pwm);
> +	priv = &pwm->priv;
> +
> +	priv->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->regs))
> +		return PTR_ERR(priv->regs);
> +
> +	ret = xilinx_timer_common_init(np, priv, &one_timer);
> +	if (ret)
> +		return ret;
> +
> +	if (one_timer)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "two timers required for PWM mode\n");
> +
> +	/*
> +	 * The polarity of the generate outputs must be active high for PWM
> +	 * mode to work. We could determine this from the device tree, but
> +	 * alas, such properties are not allowed to be used.
> +	 */
> +
> +	priv->clk = devm_clk_get(dev, "s_axi_aclk");
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk), "clock\n");

again a sparse error message.

> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "clock enable failed\n");
> +	clk_rate_exclusive_get(priv->clk);
> +
> +	pwm->chip.dev = dev;
> +	pwm->chip.ops = &xilinx_pwm_ops;
> +	pwm->chip.npwm = 1;
> +	ret = pwmchip_add(&pwm->chip);
> +	if (ret) {
> +		clk_rate_exclusive_put(priv->clk);
> +		clk_disable_unprepare(priv->clk);
> +		return dev_err_probe(dev, ret, "could not register pwm chip\n");
> +	}
> +
> +	return 0;
> +}

Sean Anderson June 25, 2021, 3:13 p.m. UTC | #3

On 6/25/21 2:19 AM, Uwe Kleine-König wrote:
 > On Fri, May 28, 2021 at 05:45:22PM -0400, Sean Anderson wrote:
 >> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
 >> found on Xilinx FPGAs.  At the moment clock control is very basic: we
 >> just enable the clock during probe and pin the frequency. In the future,
 >> someone could add support for disabling the clock when not in use.
 >>
 >> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
 >>
 >> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
 >>
 >> Signed-off-by: Sean Anderson <[e-mail]>
 >> ---
 >>
 >> Changes in v4:
 >> - Remove references to properties which are not good enough for Linux.
 >> - Don't use volatile in read/write replacements. Some arches have it and
 >>   some don't.
 >> - Put common timer properties into their own struct to better reuse
 >>   code.
 >>
 >> Changes in v3:
 >> - Add clockevent and clocksource support
 >> - Rewrite probe to only use a device_node, since timers may need to be
 >>   initialized before we have proper devices. This does bloat the code a bit
 >>   since we can no longer rely on helpers such as dev_err_probe. We also
 >>   cannot rely on device resources being free'd on failure, so we must free
 >>   them manually.
 >> - We now access registers through xilinx_timer_(read|write). This allows us
 >>   to deal with endianness issues, as originally seen in the microblaze
 >>   driver. CAVEAT EMPTOR: I have not tested this on big-endian!
 >> - Remove old microblaze driver
 >>
 >> Changes in v2:
 >> - Don't compile this module by default for arm64
 >> - Add dependencies on COMMON_CLK and HAS_IOMEM
 >> - Add comment explaining why we depend on !MICROBLAZE
 >> - Add comment describing device
 >> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
 >> - Use NSEC_TO_SEC instead of defining our own
 >> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
 >> - Cast dividends to u64 to avoid overflow
 >> - Check for over- and underflow when calculating TLR
 >> - Set xilinx_pwm_ops.owner
 >> - Don't set pwmchip.base to -1
 >> - Check range of xlnx,count-width
 >> - Ensure the clock is always running when the pwm is registered
 >> - Remove debugfs file :l
 >> - Report errors with dev_error_probe
 >>
 >>  drivers/mfd/Makefile     |   2 +-
 >>  drivers/pwm/Kconfig      |  12 +++
 >>  drivers/pwm/Makefile     |   1 +
 >>  drivers/pwm/pwm-xilinx.c | 219 +++++++++++++++++++++++++++++++++++++++
 >>  4 files changed, 233 insertions(+), 1 deletion(-)
 >>  create mode 100644 drivers/pwm/pwm-xilinx.c
 >>
 >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
 >> index f0f9fbdde7dc..89769affe251 100644
 >> --- a/drivers/mfd/Makefile
 >> +++ b/drivers/mfd/Makefile
 >> @@ -269,6 +269,6 @@ obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
 >>  obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
 >>  obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
 >>
 >> -ifneq ($(CONFIG_XILINX_TIMER),)
 >> +ifneq ($(CONFIG_PWM_XILINX)$(CONFIG_XILINX_TIMER),)
 >>  obj-y				+= xilinx-timer.o
 >>  endif
 >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
 >> index 8ae68d6203fb..ebf8d9014758 100644
 >> --- a/drivers/pwm/Kconfig
 >> +++ b/drivers/pwm/Kconfig
 >> @@ -620,4 +620,16 @@ config PWM_VT8500
 >>  	  To compile this driver as a module, choose M here: the module
 >>  	  will be called pwm-vt8500.
 >>
 >> +config PWM_XILINX
 >> +	tristate "Xilinx AXI Timer PWM support"
 >> +	depends on HAS_IOMEM && COMMON_CLK
 >> +	help
 >> +	  PWM driver for Xilinx LogiCORE IP AXI timers. This timer is
 >> +	  typically a soft core which may be present in Xilinx FPGAs.
 >> +	  This device may also be present in Microblaze soft processors.
 >> +	  If you don't have this IP in your design, choose N.
 >> +
 >> +	  To compile this driver as a module, choose M here: the module
 >> +	  will be called pwm-xilinx.
 >> +
 >>  endif
 >> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
 >> index d43b1e17e8e1..655df169b895 100644
 >> --- a/drivers/pwm/Makefile
 >> +++ b/drivers/pwm/Makefile
 >> @@ -58,3 +58,4 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 >>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
 >>  obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
 >>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
 >> +obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
 >> diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c
 >> new file mode 100644
 >> index 000000000000..f05321496717
 >> --- /dev/null
 >> +++ b/drivers/pwm/pwm-xilinx.c
 >> @@ -0,0 +1,219 @@
 >> +// SPDX-License-Identifier: GPL-2.0+
 >> +/*
 >> + * Copyright (C) 2021 Sean Anderson <[e-mail]>
 >> + *
 >> + * Hardware limitations:
 >> + * - When changing both duty cycle and period, we may end up with one cycle
 >> + *   with the old duty cycle and the new period.
 >
 > That means it doesn't reset the counter when a new period is set, right?

Correct. The only way to write to the counter is to stop the timer and
restart it.

 >
 >> + * - Cannot produce 100% duty cycle.
 >
 > Can it produce a 0% duty cycle? Below you're calling
 > xilinx_timer_tlr_period(..., ..., ..., 0) then which returns -ERANGE.

Yes. This is what you get when you try to specify 100% duty cycle (e.g.
TLR0 == TLR1).

 >
 >> + * - Only produces "normal" output.
 >
 > Does the output emit a low level when it's disabled?

I believe so.

 >
 >> + */
 >> +
 >> [...]
 >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
 >> +			    const struct pwm_state *state)
 >> +{
 >> +	int ret;
 >> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
 >> +	u32 tlr0, tlr1;
 >> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
 >> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
 >> +	bool enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
 >> +
 >> +	if (state->polarity != PWM_POLARITY_NORMAL)
 >> +		return -EINVAL;
 >> +
 >> +	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
 >> +	if (ret)
 >> +		return ret;
 >
 > The implementation of xilinx_timer_tlr_period (in patch 2/3) returns
 > -ERANGE for big periods. The good behaviour to implement is to cap to
 > the biggest period possible in this case.

Ok. Is this documented anywhere? And wouldn't this result in the wrong
duty cycle? E.g. say the max value is 100 and I try to apply a period of
150 and a duty_cycle of 75 (for a 50% duty cycle). If we cap at 100,
then I will instead have a 75% duty cycle, and there will be no error.
So I will silently get the wrong duty cycle, even when that duty cycle
is probably more important than the period.

 >
 > Also note that state->period is an u64 but it is casted to unsigned int
 > as this is the type of the forth parameter of xilinx_timer_tlr_period.

Hm, it looks like I immediately cast period to a u64. I will change the
signature for this function next revision.

 >
 >> +	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
 >> +	if (ret)
 >> +		return ret;
 >> +
 >> +	xilinx_timer_write(priv, tlr0, TLR0);
 >> +	xilinx_timer_write(priv, tlr1, TLR1);
 >> +
 >> +	if (state->enabled) {
 >> +		/* Only touch the TCSRs if we aren't already running */
 >> +		if (!enabled) {
 >> +			/* Load TLR into TCR */
 >> +			xilinx_timer_write(priv, tcsr0 | TCSR_LOAD, TCSR0);
 >> +			xilinx_timer_write(priv, tcsr1 | TCSR_LOAD, TCSR1);
 >> +			/* Enable timers all at once with ENALL */
 >> +			tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
 >> +			tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
 >> +			xilinx_timer_write(priv, tcsr0, TCSR0);
 >> +			xilinx_timer_write(priv, tcsr1, TCSR1);
 >> +		}
 >> +	} else {
 >> +		xilinx_timer_write(priv, 0, TCSR0);
 >> +		xilinx_timer_write(priv, 0, TCSR1);
 >> +	}
 >> +
 >> +	return 0;
 >> +}
 >> +
 >> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
 >> +				 struct pwm_device *unused,
 >> +				 struct pwm_state *state)
 >> +{
 >> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
 >> +	u32 tlr0 = xilinx_timer_read(priv, TLR0);
 >> +	u32 tlr1 = xilinx_timer_read(priv, TLR1);
 >> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
 >> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
 >> +
 >> +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
 >> +	state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
 >> +	state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
 >> +	state->polarity = PWM_POLARITY_NORMAL;
 >
 > Are the values returned here sensible if the hardware isn't in PWM mode?

Yes. If the hardware isn't in PWM mode, then state->enabled will be
false.

 >
 >> +}
 >> +
 >> +static const struct pwm_ops xilinx_pwm_ops = {
 >> +	.apply = xilinx_pwm_apply,
 >> +	.get_state = xilinx_pwm_get_state,
 >> +	.owner = THIS_MODULE,
 >> +};
 >> +
 >> +static int xilinx_timer_probe(struct platform_device *pdev)
 >> +{
 >> +	int ret;
 >> +	struct device *dev = &pdev->dev;
 >> +	struct device_node *np = dev->of_node;
 >> +	struct xilinx_timer_priv *priv;
 >> +	struct xilinx_pwm_device *pwm;
 >> +	u32 pwm_cells, one_timer;
 >> +
 >> +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
 >> +	if (ret == -EINVAL)
 >> +		return -ENODEV;
 >> +	else if (ret)
 >> +		return dev_err_probe(dev, ret, "#pwm-cells\n");
 >
 > Very sparse error message.

Ok, will elaborate.

 >
 >> +	else if (pwm_cells)
 >> +		return dev_err_probe(dev, -EINVAL, "#pwm-cells must be 0\n");
 >
 > What is the rationale here to not support #pwm-cells = <2>?

Only one PWM is supported. But otherwise there is no particular
reason.

 >> +	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
 >> +	if (!pwm)
 >> +		return -ENOMEM;
 >> +	platform_set_drvdata(pdev, pwm);
 >> +	priv = &pwm->priv;
 >> +
 >> +	priv->regs = devm_platform_ioremap_resource(pdev, 0);
 >> +	if (IS_ERR(priv->regs))
 >> +		return PTR_ERR(priv->regs);
 >> +
 >> +	ret = xilinx_timer_common_init(np, priv, &one_timer);
 >> +	if (ret)
 >> +		return ret;
 >> +
 >> +	if (one_timer)
 >> +		return dev_err_probe(dev, -EINVAL,
 >> +				     "two timers required for PWM mode\n");
 >> +
 >> +	/*
 >> +	 * The polarity of the generate outputs must be active high for PWM
 >> +	 * mode to work. We could determine this from the device tree, but
 >> +	 * alas, such properties are not allowed to be used.
 >> +	 */
 >> +
 >> +	priv->clk = devm_clk_get(dev, "s_axi_aclk");
 >> +	if (IS_ERR(priv->clk))
 >> +		return dev_err_probe(dev, PTR_ERR(priv->clk), "clock\n");
 >
 > again a sparse error message.
 >
 >> +
 >> +	ret = clk_prepare_enable(priv->clk);
 >> +	if (ret)
 >> +		return dev_err_probe(dev, ret, "clock enable failed\n");
 >> +	clk_rate_exclusive_get(priv->clk);
 >> +
 >> +	pwm->chip.dev = dev;
 >> +	pwm->chip.ops = &xilinx_pwm_ops;
 >> +	pwm->chip.npwm = 1;
 >> +	ret = pwmchip_add(&pwm->chip);
 >> +	if (ret) {
 >> +		clk_rate_exclusive_put(priv->clk);
 >> +		clk_disable_unprepare(priv->clk);
 >> +		return dev_err_probe(dev, ret, "could not register pwm chip\n");
 >> +	}
 >> +
 >> +	return 0;
 >> +}

Thanks for the review.

--Sean

Uwe Kleine-König June 25, 2021, 4:56 p.m. UTC | #4

Hello Sean,

On Fri, Jun 25, 2021 at 11:13:33AM -0400, Sean Anderson wrote:
> On 6/25/21 2:19 AM, Uwe Kleine-König wrote:
> > On Fri, May 28, 2021 at 05:45:22PM -0400, Sean Anderson wrote:
> >> + * Hardware limitations:

Please make this "* Limitations:" to match what the other drivers do and
so ease grepping for this info.

> >> + * - When changing both duty cycle and period, we may end up with one cycle
> >> + *   with the old duty cycle and the new period.
> >
> > That means it doesn't reset the counter when a new period is set, right?
> 
> Correct. The only way to write to the counter is to stop the timer and
> restart it.

ok.

> >> + * - Cannot produce 100% duty cycle.
> >
> > Can it produce a 0% duty cycle? Below you're calling
> > xilinx_timer_tlr_period(..., ..., ..., 0) then which returns -ERANGE.
> 
> Yes. This is what you get when you try to specify 100% duty cycle (e.g.
> TLR0 == TLR1).

OK, so the hardware can do it, but your driver doesn't make use of it,
right?

> >> + * - Only produces "normal" output.
> >
> > Does the output emit a low level when it's disabled?
> 
> I believe so.

Is there a possibility to be sure? I'd like to know that to complete my
picture about the behaviour of the supported PWMs.

> >> + */
> >> +
> >> [...]
> >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> >> +			    const struct pwm_state *state)
> >> +{
> >> +	int ret;
> >> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> >> +	u32 tlr0, tlr1;
> >> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
> >> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
> >> +	bool enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> >> +
> >> +	if (state->polarity != PWM_POLARITY_NORMAL)
> >> +		return -EINVAL;
> >> +
> >> +	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
> >> +	if (ret)
> >> +		return ret;
> >
> > The implementation of xilinx_timer_tlr_period (in patch 2/3) returns
> > -ERANGE for big periods. The good behaviour to implement is to cap to
> > the biggest period possible in this case.
> 
> Ok. Is this documented anywhere?

I tried but Thierry didn't like the result and I didn't retry. The
problem is also that many drivers we already have in the tree don't
behave like this (because for a long time nobody cared). That new
drivers should behave this way is my effort to get some consistent
behaviour.

> And wouldn't this result in the wrong duty cycle? E.g. say the max
> value is 100 and I try to apply a period of 150 and a duty_cycle of 75
> (for a 50% duty cycle). If we cap at 100, then I will instead have a
> 75% duty cycle, and there will be no error.

Yes that is right. That there is no feedback is a problem that we have
for a long time. I have a prototype patch that implements a
pwm_round_state() function that lets a consumer know the result of
applying a certain pwm_state in advance. But we're not there yet.

> So I will silently get the wrong duty cycle, even when that duty cycle
> is probably more important than the period.

It depends on the use case and every policy is wrong for some cases. So
I picked the policy I already explained because it is a) easy to
implement for lowlevel drivers and b) it's easy to work with for
consumers once we have pwm_round_state().

> > Also note that state->period is an u64 but it is casted to unsigned int
> > as this is the type of the forth parameter of xilinx_timer_tlr_period.
> 
> Hm, it looks like I immediately cast period to a u64. I will change the
> signature for this function next revision.

Then note that period * clk_get_rate(priv->clk) might overflow.
 
> >> +	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	xilinx_timer_write(priv, tlr0, TLR0);
> >> +	xilinx_timer_write(priv, tlr1, TLR1);
> >> +
> >> +	if (state->enabled) {
> >> +		/* Only touch the TCSRs if we aren't already running */
> >> +		if (!enabled) {
> >> +			/* Load TLR into TCR */
> >> +			xilinx_timer_write(priv, tcsr0 | TCSR_LOAD, TCSR0);
> >> +			xilinx_timer_write(priv, tcsr1 | TCSR_LOAD, TCSR1);
> >> +			/* Enable timers all at once with ENALL */
> >> +			tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
> >> +			tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
> >> +			xilinx_timer_write(priv, tcsr0, TCSR0);
> >> +			xilinx_timer_write(priv, tcsr1, TCSR1);
> >> +		}
> >> +	} else {
> >> +		xilinx_timer_write(priv, 0, TCSR0);
> >> +		xilinx_timer_write(priv, 0, TCSR1);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
> >> +				 struct pwm_device *unused,
> >> +				 struct pwm_state *state)
> >> +{
> >> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> >> +	u32 tlr0 = xilinx_timer_read(priv, TLR0);
> >> +	u32 tlr1 = xilinx_timer_read(priv, TLR1);
> >> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
> >> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
> >> +
> >> +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
> >> +	state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
> >> +	state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> >> +	state->polarity = PWM_POLARITY_NORMAL;
> >
> > Are the values returned here sensible if the hardware isn't in PWM mode?
> 
> Yes. If the hardware isn't in PWM mode, then state->enabled will be
> false.

Ah right. Good enough.

> >> +	else if (pwm_cells)
> >> +		return dev_err_probe(dev, -EINVAL, "#pwm-cells must be 0\n");
> >
> > What is the rationale here to not support #pwm-cells = <2>?
> 
> Only one PWM is supported. But otherwise there is no particular
> reason.

The usual binding is to have 3 additional parameters.
 1) chip-local pwm number (which can only be 0 for a pwmchip having
    .npwm = 1)
 2) the "typical" period
 3) some flags (like PWM_POLARITY_*)

I don't care much if you implement it with or without 1), but 2) and 3)
should IMHO be here. If you don't want 1),
http://patchwork.ozlabs.org/project/linux-pwm/patch/[e-mail]/
might be interesting for you. (But note, Thierry didn't give feedback to
this yet, it might be possible he wants 1)-3) for new drivers.)

Best regards
Uwe

Sean Anderson June 25, 2021, 5:46 p.m. UTC | #5

On 6/25/21 12:56 PM, Uwe Kleine-König wrote:
> Hello Sean,
>
> On Fri, Jun 25, 2021 at 11:13:33AM -0400, Sean Anderson wrote:
>> On 6/25/21 2:19 AM, Uwe Kleine-König wrote:
>> > On Fri, May 28, 2021 at 05:45:22PM -0400, Sean Anderson wrote:
>> >> + * Hardware limitations:
>
> Please make this "* Limitations:" to match what the other drivers do and
> so ease grepping for this info.
>
>> >> + * - When changing both duty cycle and period, we may end up with one cycle
>> >> + *   with the old duty cycle and the new period.
>> >
>> > That means it doesn't reset the counter when a new period is set, right?
>>
>> Correct. The only way to write to the counter is to stop the timer and
>> restart it.
>
> ok.
>
>> >> + * - Cannot produce 100% duty cycle.
>> >
>> > Can it produce a 0% duty cycle? Below you're calling
>> > xilinx_timer_tlr_period(..., ..., ..., 0) then which returns -ERANGE.
>>
>> Yes. This is what you get when you try to specify 100% duty cycle (e.g.
>> TLR0 == TLR1).
>
> OK, so the hardware can do it, but your driver doesn't make use of it,
> right?

So to clarify, I have observed the following behavior

* When TCSR == 0, the output is constant low.
* When TLR1 (duty_cycle) >= TLR0 (period), the output is constant low.

It might be possible to achieve constant high output by stopping the
counters during the high time, but leaving the PWM bit set. However, I
do not have a use case for this. I think it might be a nice follow-up
for someone who wants that feature.

That being said, is there a standard way to specify 100% or 0% duty
cycle? It seems like one would have to detect these situations and use a
different code path.

>
>> >> + * - Only produces "normal" output.
>> >
>> > Does the output emit a low level when it's disabled?
>>
>> I believe so.
>
> Is there a possibility to be sure? I'd like to know that to complete my
> picture about the behaviour of the supported PWMs.

See above.

>
>> >> + */
>> >> +
>> >> [...]
>> >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
>> >> +			    const struct pwm_state *state)
>> >> +{
>> >> +	int ret;
>> >> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
>> >> +	u32 tlr0, tlr1;
>> >> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
>> >> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
>> >> +	bool enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
>> >> +
>> >> +	if (state->polarity != PWM_POLARITY_NORMAL)
>> >> +		return -EINVAL;
>> >> +
>> >> +	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
>> >> +	if (ret)
>> >> +		return ret;
>> >
>> > The implementation of xilinx_timer_tlr_period (in patch 2/3) returns
>> > -ERANGE for big periods. The good behaviour to implement is to cap to
>> > the biggest period possible in this case.
>>
>> Ok. Is this documented anywhere?
>
> I tried but Thierry didn't like the result and I didn't retry. The
> problem is also that many drivers we already have in the tree don't
> behave like this (because for a long time nobody cared). That new
> drivers should behave this way is my effort to get some consistent
> behaviour.

Do you have a link to the thread? IMO if you would like to specify
behavior like this, is is very helpful to write it down so new authors
don't have to get to v4 before finding out about it ;)

>> And wouldn't this result in the wrong duty cycle? E.g. say the max
>> value is 100 and I try to apply a period of 150 and a duty_cycle of 75
>> (for a 50% duty cycle). If we cap at 100, then I will instead have a
>> 75% duty cycle, and there will be no error.
>
> Yes that is right. That there is no feedback is a problem that we have
> for a long time. I have a prototype patch that implements a
> pwm_round_state() function that lets a consumer know the result of
> applying a certain pwm_state in advance. But we're not there yet.

So for the moment, why not give an error? This will be legal code both
now and after round_state is implemented.

>
>> So I will silently get the wrong duty cycle, even when that duty cycle
>> is probably more important than the period.
>
> It depends on the use case and every policy is wrong for some cases. So
> I picked the policy I already explained because it is a) easy to
> implement for lowlevel drivers and b) it's easy to work with for
> consumers once we have pwm_round_state().

What about sysfs? Right now if you try to specify an inexpressible
period you get an error message. I saw [1], but unfortunately there do
not appear to be any patches associated with it. Do you have plans to
implement such an interface?

[1] https://lore.kernel.org/linux-pwm/[e-mail]/

>
>> > Also note that state->period is an u64 but it is casted to unsigned int
>> > as this is the type of the forth parameter of xilinx_timer_tlr_period.
>>
>> Hm, it looks like I immediately cast period to a u64. I will change the
>> signature for this function next revision.
>
> Then note that period * clk_get_rate(priv->clk) might overflow.

Ok, so is mult_frac the correct macro to use here?

>> >> +	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
>> >> +	if (ret)
>> >> +		return ret;

Perhaps I should add

	if (tlr0 <= tlr1)
		return -EINVAL;

here to prevent accidentally getting 0% duty cycle.

>> >> +
>> >> +	xilinx_timer_write(priv, tlr0, TLR0);
>> >> +	xilinx_timer_write(priv, tlr1, TLR1);
>> >> +
>> >> +	if (state->enabled) {
>> >> +		/* Only touch the TCSRs if we aren't already running */
>> >> +		if (!enabled) {
>> >> +			/* Load TLR into TCR */
>> >> +			xilinx_timer_write(priv, tcsr0 | TCSR_LOAD, TCSR0);
>> >> +			xilinx_timer_write(priv, tcsr1 | TCSR_LOAD, TCSR1);
>> >> +			/* Enable timers all at once with ENALL */
>> >> +			tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
>> >> +			tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
>> >> +			xilinx_timer_write(priv, tcsr0, TCSR0);
>> >> +			xilinx_timer_write(priv, tcsr1, TCSR1);
>> >> +		}
>> >> +	} else {
>> >> +		xilinx_timer_write(priv, 0, TCSR0);
>> >> +		xilinx_timer_write(priv, 0, TCSR1);
>> >> +	}
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
>> >> +				 struct pwm_device *unused,
>> >> +				 struct pwm_state *state)
>> >> +{
>> >> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
>> >> +	u32 tlr0 = xilinx_timer_read(priv, TLR0);
>> >> +	u32 tlr1 = xilinx_timer_read(priv, TLR1);
>> >> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
>> >> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
>> >> +
>> >> +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
>> >> +	state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
>> >> +	state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
>> >> +	state->polarity = PWM_POLARITY_NORMAL;
>> >
>> > Are the values returned here sensible if the hardware isn't in PWM mode?
>>
>> Yes. If the hardware isn't in PWM mode, then state->enabled will be
>> false.
>
> Ah right. Good enough.
>
>> >> +	else if (pwm_cells)
>> >> +		return dev_err_probe(dev, -EINVAL, "#pwm-cells must be 0\n");
>> >
>> > What is the rationale here to not support #pwm-cells = <2>?
>>
>> Only one PWM is supported. But otherwise there is no particular
>> reason.
>
> The usual binding is to have 3 additional parameters.
>   1) chip-local pwm number (which can only be 0 for a pwmchip having
>      .npwm = 1)
>   2) the "typical" period
>   3) some flags (like PWM_POLARITY_*)
>
> I don't care much if you implement it with or without 1), but 2) and 3)
> should IMHO be here. If you don't want 1),
> http://patchwork.ozlabs.org/project/linux-pwm/patch/[e-mail]/
> might be interesting for you. (But note, Thierry didn't give feedback to
> this yet, it might be possible he wants 1)-3) for new drivers.)

Ok, so since I let of_pwmchip_add set xlate, I don't need to change
anything in this driver? Then I will remove this check for v5.

--Sean

Sean Anderson June 25, 2021, 5:46 p.m. UTC | #6

On 6/25/21 12:56 PM, Uwe Kleine-König wrote:
> Hello Sean,
>
> On Fri, Jun 25, 2021 at 11:13:33AM -0400, Sean Anderson wrote:
>> On 6/25/21 2:19 AM, Uwe Kleine-König wrote:
>> > On Fri, May 28, 2021 at 05:45:22PM -0400, Sean Anderson wrote:
>> >> + * Hardware limitations:
>
> Please make this "* Limitations:" to match what the other drivers do and
> so ease grepping for this info.
>
>> >> + * - When changing both duty cycle and period, we may end up with one cycle
>> >> + *   with the old duty cycle and the new period.
>> >
>> > That means it doesn't reset the counter when a new period is set, right?
>>
>> Correct. The only way to write to the counter is to stop the timer and
>> restart it.
>
> ok.
>
>> >> + * - Cannot produce 100% duty cycle.
>> >
>> > Can it produce a 0% duty cycle? Below you're calling
>> > xilinx_timer_tlr_period(..., ..., ..., 0) then which returns -ERANGE.
>>
>> Yes. This is what you get when you try to specify 100% duty cycle (e.g.
>> TLR0 == TLR1).
>
> OK, so the hardware can do it, but your driver doesn't make use of it,
> right?

So to clarify, I have observed the following behavior

* When TCSR == 0, the output is constant low.
* When TLR1 (duty_cycle) >= TLR0 (period), the output is constant low.

It might be possible to achieve constant high output by stopping the
counters during the high time, but leaving the PWM bit set. However, I
do not have a use case for this. I think it might be a nice follow-up
for someone who wants that feature.

That being said, is there a standard way to specify 100% or 0% duty
cycle? It seems like one would have to detect these situations and use a
different code path.

>
>> >> + * - Only produces "normal" output.
>> >
>> > Does the output emit a low level when it's disabled?
>>
>> I believe so.
>
> Is there a possibility to be sure? I'd like to know that to complete my
> picture about the behaviour of the supported PWMs.

See above.

>
>> >> + */
>> >> +
>> >> [...]
>> >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
>> >> +			    const struct pwm_state *state)
>> >> +{
>> >> +	int ret;
>> >> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
>> >> +	u32 tlr0, tlr1;
>> >> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
>> >> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
>> >> +	bool enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
>> >> +
>> >> +	if (state->polarity != PWM_POLARITY_NORMAL)
>> >> +		return -EINVAL;
>> >> +
>> >> +	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
>> >> +	if (ret)
>> >> +		return ret;
>> >
>> > The implementation of xilinx_timer_tlr_period (in patch 2/3) returns
>> > -ERANGE for big periods. The good behaviour to implement is to cap to
>> > the biggest period possible in this case.
>>
>> Ok. Is this documented anywhere?
>
> I tried but Thierry didn't like the result and I didn't retry. The
> problem is also that many drivers we already have in the tree don't
> behave like this (because for a long time nobody cared). That new
> drivers should behave this way is my effort to get some consistent
> behaviour.

Do you have a link to the thread? IMO if you would like to specify
behavior like this, is is very helpful to write it down so new authors
don't have to get to v4 before finding out about it ;)

>> And wouldn't this result in the wrong duty cycle? E.g. say the max
>> value is 100 and I try to apply a period of 150 and a duty_cycle of 75
>> (for a 50% duty cycle). If we cap at 100, then I will instead have a
>> 75% duty cycle, and there will be no error.
>
> Yes that is right. That there is no feedback is a problem that we have
> for a long time. I have a prototype patch that implements a
> pwm_round_state() function that lets a consumer know the result of
> applying a certain pwm_state in advance. But we're not there yet.

So for the moment, why not give an error? This will be legal code both
now and after round_state is implemented.

>
>> So I will silently get the wrong duty cycle, even when that duty cycle
>> is probably more important than the period.
>
> It depends on the use case and every policy is wrong for some cases. So
> I picked the policy I already explained because it is a) easy to
> implement for lowlevel drivers and b) it's easy to work with for
> consumers once we have pwm_round_state().

What about sysfs? Right now if you try to specify an inexpressible
period you get an error message. I saw [1], but unfortunately there do
not appear to be any patches associated with it. Do you have plans to
implement such an interface?

[1] https://lore.kernel.org/linux-pwm/[e-mail]/

>
>> > Also note that state->period is an u64 but it is casted to unsigned int
>> > as this is the type of the forth parameter of xilinx_timer_tlr_period.
>>
>> Hm, it looks like I immediately cast period to a u64. I will change the
>> signature for this function next revision.
>
> Then note that period * clk_get_rate(priv->clk) might overflow.

Ok, so is mult_frac the correct macro to use here?

>> >> +	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
>> >> +	if (ret)
>> >> +		return ret;

Perhaps I should add

	if (tlr0 <= tlr1)
		return -EINVAL;

here to prevent accidentally getting 0% duty cycle.

>> >> +
>> >> +	xilinx_timer_write(priv, tlr0, TLR0);
>> >> +	xilinx_timer_write(priv, tlr1, TLR1);
>> >> +
>> >> +	if (state->enabled) {
>> >> +		/* Only touch the TCSRs if we aren't already running */
>> >> +		if (!enabled) {
>> >> +			/* Load TLR into TCR */
>> >> +			xilinx_timer_write(priv, tcsr0 | TCSR_LOAD, TCSR0);
>> >> +			xilinx_timer_write(priv, tcsr1 | TCSR_LOAD, TCSR1);
>> >> +			/* Enable timers all at once with ENALL */
>> >> +			tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
>> >> +			tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
>> >> +			xilinx_timer_write(priv, tcsr0, TCSR0);
>> >> +			xilinx_timer_write(priv, tcsr1, TCSR1);
>> >> +		}
>> >> +	} else {
>> >> +		xilinx_timer_write(priv, 0, TCSR0);
>> >> +		xilinx_timer_write(priv, 0, TCSR1);
>> >> +	}
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
>> >> +				 struct pwm_device *unused,
>> >> +				 struct pwm_state *state)
>> >> +{
>> >> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
>> >> +	u32 tlr0 = xilinx_timer_read(priv, TLR0);
>> >> +	u32 tlr1 = xilinx_timer_read(priv, TLR1);
>> >> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
>> >> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
>> >> +
>> >> +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
>> >> +	state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
>> >> +	state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
>> >> +	state->polarity = PWM_POLARITY_NORMAL;
>> >
>> > Are the values returned here sensible if the hardware isn't in PWM mode?
>>
>> Yes. If the hardware isn't in PWM mode, then state->enabled will be
>> false.
>
> Ah right. Good enough.
>
>> >> +	else if (pwm_cells)
>> >> +		return dev_err_probe(dev, -EINVAL, "#pwm-cells must be 0\n");
>> >
>> > What is the rationale here to not support #pwm-cells = <2>?
>>
>> Only one PWM is supported. But otherwise there is no particular
>> reason.
>
> The usual binding is to have 3 additional parameters.
>   1) chip-local pwm number (which can only be 0 for a pwmchip having
>      .npwm = 1)
>   2) the "typical" period
>   3) some flags (like PWM_POLARITY_*)
>
> I don't care much if you implement it with or without 1), but 2) and 3)
> should IMHO be here. If you don't want 1),
> http://patchwork.ozlabs.org/project/linux-pwm/patch/[e-mail]/
> might be interesting for you. (But note, Thierry didn't give feedback to
> this yet, it might be possible he wants 1)-3) for new drivers.)

Ok, so since I let of_pwmchip_add set xlate, I don't need to change
anything in this driver? Then I will remove this check for v5.

--Sean

Uwe Kleine-König June 27, 2021, 6:19 p.m. UTC | #7

Hello Sean,

On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
> On 6/25/21 12:56 PM, Uwe Kleine-König wrote:
> > On Fri, Jun 25, 2021 at 11:13:33AM -0400, Sean Anderson wrote:
> > > On 6/25/21 2:19 AM, Uwe Kleine-König wrote:
> > > > On Fri, May 28, 2021 at 05:45:22PM -0400, Sean Anderson wrote:
> > > >> + * - Cannot produce 100% duty cycle.
> > > >
> > > > Can it produce a 0% duty cycle? Below you're calling
> > > > xilinx_timer_tlr_period(..., ..., ..., 0) then which returns -ERANGE.
> > > 
> > > Yes. This is what you get when you try to specify 100% duty cycle (e.g.
> > > TLR0 == TLR1).
> > 
> > OK, so the hardware can do it, but your driver doesn't make use of it,
> > right?
> 
> So to clarify, I have observed the following behavior
> 
> * When TCSR == 0, the output is constant low.
> * When TLR1 (duty_cycle) >= TLR0 (period), the output is constant low.
> 
> It might be possible to achieve constant high output by stopping the
> counters during the high time, but leaving the PWM bit set. However, I
> do not have a use case for this. I think it might be a nice follow-up
> for someone who wants that feature.

A typical use case is having an LED or a backlight connected to the PWM
and and want it to be fully on.

> That being said, is there a standard way to specify 100% or 0% duty
> cycle? It seems like one would have to detect these situations and use a
> different code path.

I don't understand that question. If pwm_apply_state is called with a
state having .duty_cycle = 0 (and .enabled = true) you're supposed to
emit a 0% relative duty. If .duty_cycle = .period is passed you're
supposed to emit a 100% relative duty.

> > > >> + * - Only produces "normal" output.
> > > >
> > > > Does the output emit a low level when it's disabled?
> > > 
> > > I believe so.
> > 
> > Is there a possibility to be sure? I'd like to know that to complete my
> > picture about the behaviour of the supported PWMs.
> 
> See above.
> 
> > > >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> > > >> +			    const struct pwm_state *state)
> > > >> +{
> > > >> +	int ret;
> > > >> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> > > >> +	u32 tlr0, tlr1;
> > > >> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
> > > >> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
> > > >> +	bool enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> > > >> +
> > > >> +	if (state->polarity != PWM_POLARITY_NORMAL)
> > > >> +		return -EINVAL;
> > > >> +
> > > >> +	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
> > > >> +	if (ret)
> > > >> +		return ret;
> > > >
> > > > The implementation of xilinx_timer_tlr_period (in patch 2/3) returns
> > > > -ERANGE for big periods. The good behaviour to implement is to cap to
> > > > the biggest period possible in this case.
> > > 
> > > Ok. Is this documented anywhere?
> > 
> > I tried but Thierry didn't like the result and I didn't retry. The
> > problem is also that many drivers we already have in the tree don't
> > behave like this (because for a long time nobody cared). That new
> > drivers should behave this way is my effort to get some consistent
> > behaviour.
> 
> Do you have a link to the thread? IMO if you would like to specify
> behavior like this, is is very helpful to write it down so new authors
> don't have to get to v4 before finding out about it ;)

I misremembered, the last time I wanted to improve the documentation I
didn't write anything about the policy with the goal to improve the
documentation without hitting the a bit controversial policy stuff. The
thread is available at

	https://lore.kernel.org/r/[e-mail]

. Thierry didn't reply to this thread yet.

My intension was to build on this one and formulate the expected policy
for new drivers.

The situation I'm in here wanting to install this policy is: On one hand
Thierry argues that consumers don't care much about how .apply rounds
because most consumers just don't have so exact requirements. And on the
other hand he doesn't want to change the existing behaviour for already
existing drivers to not break consumers. So the best I can currently to
do work on a more consistent behaviour is to enforce this for new
drivers.

> > > And wouldn't this result in the wrong duty cycle? E.g. say the max
> > > value is 100 and I try to apply a period of 150 and a duty_cycle of 75
> > > (for a 50% duty cycle). If we cap at 100, then I will instead have a
> > > 75% duty cycle, and there will be no error.
> > 
> > Yes that is right. That there is no feedback is a problem that we have
> > for a long time. I have a prototype patch that implements a
> > pwm_round_state() function that lets a consumer know the result of
> > applying a certain pwm_state in advance. But we're not there yet.
> 
> So for the moment, why not give an error? This will be legal code both
> now and after round_state is implemented.

The problem is where to draw the line. To stay with your example: If a
request for period = 150 ns comes in, and let X be the biggest period <=
150 ns that the hardware can configure. For which values of X should an
error be returned and for which values the setting should be
implemented.

In my eyes the only sensible thing to implement here is to tell the
consumer about X and let it decide if it's good enough. If you have a
better idea let me hear about it.

> > > So I will silently get the wrong duty cycle, even when that duty cycle
> > > is probably more important than the period.
> > 
> > It depends on the use case and every policy is wrong for some cases. So
> > I picked the policy I already explained because it is a) easy to
> > implement for lowlevel drivers and b) it's easy to work with for
> > consumers once we have pwm_round_state().
> 
> What about sysfs? Right now if you try to specify an inexpressible
> period you get an error message. I saw [1], but unfortunately there do
> not appear to be any patches associated with it. Do you have plans to
> implement such an interface?
> 
> [1] https://lore.kernel.org/linux-pwm/[e-mail]/

In my eyes we better implement a pwmctl interface that doesn't work via
sysfs but using an ioctl; similar what gpio did with gpioctl for various
reasons.

> > > > Also note that state->period is an u64 but it is casted to unsigned int
> > > > as this is the type of the forth parameter of xilinx_timer_tlr_period.
> > > 
> > > Hm, it looks like I immediately cast period to a u64. I will change the
> > > signature for this function next revision.
> > 
> > Then note that period * clk_get_rate(priv->clk) might overflow.
> 
> Ok, so is mult_frac the correct macro to use here?
> 
> > > >> +	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
> > > >> +	if (ret)
> > > >> +		return ret;
> 
> Perhaps I should add
> 
> 	if (tlr0 <= tlr1)
> 		return -EINVAL;
> 
> here to prevent accidentally getting 0% duty cycle.

You can assume that duty_cycle <= period when .apply is called.

> > > >> +	else if (pwm_cells)
> > > >> +		return dev_err_probe(dev, -EINVAL, "#pwm-cells must be 0\n");
> > > >
> > > > What is the rationale here to not support #pwm-cells = <2>?
> > > 
> > > Only one PWM is supported. But otherwise there is no particular
> > > reason.
> > 
> > The usual binding is to have 3 additional parameters.
> >   1) chip-local pwm number (which can only be 0 for a pwmchip having
> >      .npwm = 1)
> >   2) the "typical" period
> >   3) some flags (like PWM_POLARITY_*)
> > 
> > I don't care much if you implement it with or without 1), but 2) and 3)
> > should IMHO be here. If you don't want 1),
> > http://patchwork.ozlabs.org/project/linux-pwm/patch/[e-mail]/
> > might be interesting for you. (But note, Thierry didn't give feedback to
> > this yet, it might be possible he wants 1)-3) for new drivers.)
> 
> Ok, so since I let of_pwmchip_add set xlate, I don't need to change
> anything in this driver? Then I will remove this check for v5.

Yes.

Best regards
Uwe

Sean Anderson June 28, 2021, 3:50 p.m. UTC | #8

On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
> Hello Sean,
>
> On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
>> On 6/25/21 12:56 PM, Uwe Kleine-König wrote:
>> > On Fri, Jun 25, 2021 at 11:13:33AM -0400, Sean Anderson wrote:
>> > > On 6/25/21 2:19 AM, Uwe Kleine-König wrote:
>> > > > On Fri, May 28, 2021 at 05:45:22PM -0400, Sean Anderson wrote:
>> > > >> + * - Cannot produce 100% duty cycle.
>> > > >
>> > > > Can it produce a 0% duty cycle? Below you're calling
>> > > > xilinx_timer_tlr_period(..., ..., ..., 0) then which returns -ERANGE.
>> > >
>> > > Yes. This is what you get when you try to specify 100% duty cycle (e.g.
>> > > TLR0 == TLR1).
>> >
>> > OK, so the hardware can do it, but your driver doesn't make use of it,
>> > right?
>>
>> So to clarify, I have observed the following behavior
>>
>> * When TCSR == 0, the output is constant low.
>> * When TLR1 (duty_cycle) >= TLR0 (period), the output is constant low.
>>
>> It might be possible to achieve constant high output by stopping the
>> counters during the high time, but leaving the PWM bit set. However, I
>> do not have a use case for this. I think it might be a nice follow-up
>> for someone who wants that feature.
>
> A typical use case is having an LED or a backlight connected to the PWM
> and and want it to be fully on.

I mean that I personally do not need this feature for my current
project, though I could see how someone might want this.

>
>> That being said, is there a standard way to specify 100% or 0% duty
>> cycle? It seems like one would have to detect these situations and use a
>> different code path.
>
> I don't understand that question. If pwm_apply_state is called with a
> state having .duty_cycle = 0 (and .enabled = true) you're supposed to
> emit a 0% relative duty. If .duty_cycle = .period is passed you're
> supposed to emit a 100% relative duty.

Ok.

>
>> > > >> + * - Only produces "normal" output.
>> > > >
>> > > > Does the output emit a low level when it's disabled?
>> > >
>> > > I believe so.
>> >
>> > Is there a possibility to be sure? I'd like to know that to complete my
>> > picture about the behaviour of the supported PWMs.
>>
>> See above.
>>
>> > > >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
>> > > >> +			    const struct pwm_state *state)
>> > > >> +{
>> > > >> +	int ret;
>> > > >> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
>> > > >> +	u32 tlr0, tlr1;
>> > > >> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
>> > > >> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
>> > > >> +	bool enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
>> > > >> +
>> > > >> +	if (state->polarity != PWM_POLARITY_NORMAL)
>> > > >> +		return -EINVAL;
>> > > >> +
>> > > >> +	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
>> > > >> +	if (ret)
>> > > >> +		return ret;
>> > > >
>> > > > The implementation of xilinx_timer_tlr_period (in patch 2/3) returns
>> > > > -ERANGE for big periods. The good behaviour to implement is to cap to
>> > > > the biggest period possible in this case.
>> > >
>> > > Ok. Is this documented anywhere?
>> >
>> > I tried but Thierry didn't like the result and I didn't retry. The
>> > problem is also that many drivers we already have in the tree don't
>> > behave like this (because for a long time nobody cared). That new
>> > drivers should behave this way is my effort to get some consistent
>> > behaviour.
>>
>> Do you have a link to the thread? IMO if you would like to specify
>> behavior like this, is is very helpful to write it down so new authors
>> don't have to get to v4 before finding out about it ;)
>
> I misremembered, the last time I wanted to improve the documentation I
> didn't write anything about the policy with the goal to improve the
> documentation without hitting the a bit controversial policy stuff. The
> thread is available at
>
> 	https://lore.kernel.org/r/[e-mail]

This link does not work, but I was able to find the patch at

	https://patchwork.ozlabs.org/project/linux-pwm/patch/[e-mail]/

However, it has no mention of rounding the rate, or what to do if a
given configuration cannot be applied.

> . Thierry didn't reply to this thread yet.

It seems the patch is marked as "Rejected" which is odd with no
feedback. Perhaps you should resend.

>
> My intension was to build on this one and formulate the expected policy
> for new drivers.
>
> The situation I'm in here wanting to install this policy is: On one hand
> Thierry argues that consumers don't care much about how .apply rounds
> because most consumers just don't have so exact requirements. And on the
> other hand he doesn't want to change the existing behaviour for already
> existing drivers to not break consumers. So the best I can currently to
> do work on a more consistent behaviour is to enforce this for new
> drivers.
>
>> > > And wouldn't this result in the wrong duty cycle? E.g. say the max
>> > > value is 100 and I try to apply a period of 150 and a duty_cycle of 75
>> > > (for a 50% duty cycle). If we cap at 100, then I will instead have a
>> > > 75% duty cycle, and there will be no error.
>> >
>> > Yes that is right. That there is no feedback is a problem that we have
>> > for a long time. I have a prototype patch that implements a
>> > pwm_round_state() function that lets a consumer know the result of
>> > applying a certain pwm_state in advance. But we're not there yet.
>>
>> So for the moment, why not give an error? This will be legal code both
>> now and after round_state is implemented.
>
> The problem is where to draw the line. To stay with your example: If a
> request for period = 150 ns comes in, and let X be the biggest period <=
> 150 ns that the hardware can configure. For which values of X should an
> error be returned and for which values the setting should be
> implemented.
>
> In my eyes the only sensible thing to implement here is to tell the
> consumer about X and let it decide if it's good enough. If you have a
> better idea let me hear about it.

Sure. And I think it's ok to tell the consumer that X is the best we can
do. But if they go along and request an unconfigurable state anyway, we
should tell them as much. IMO, this is the best way to prevent surprising
results in the API.

The real issue here is that it is impossible to determine the correct
way to round the PWM a priori, and in particular, without considering
both duty_cycle and period. If a consumer requests very small
period/duty cycle which we cannot produce, how should it be rounded?
Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
the least period? Or should we try and increase the period to better
approximate the % duty cycle? And both of these decisions must be made
knowing both parameters. We cannot (for example) just always round up,
since we may produce a configuration with TLR0 == TLR1, which would
produce 0% duty cycle instead of whatever was requested. Rounding rate
will introduce significant complexity into the driver. Most of the time
if a consumer requests an invalid rate, it is due to misconfiguration
which is best solved by fixing the configuration.

>> > > So I will silently get the wrong duty cycle, even when that duty cycle
>> > > is probably more important than the period.
>> >
>> > It depends on the use case and every policy is wrong for some cases. So
>> > I picked the policy I already explained because it is a) easy to
>> > implement for lowlevel drivers and b) it's easy to work with for
>> > consumers once we have pwm_round_state().
>>
>> What about sysfs? Right now if you try to specify an inexpressible
>> period you get an error message. I saw [1], but unfortunately there do
>> not appear to be any patches associated with it. Do you have plans to
>> implement such an interface?
>>
>> [1] https://lore.kernel.org/linux-pwm/[e-mail]/
>
> In my eyes we better implement a pwmctl interface that doesn't work via
> sysfs but using an ioctl; similar what gpio did with gpioctl for various
> reasons.
>
>> > > > Also note that state->period is an u64 but it is casted to unsigned int
>> > > > as this is the type of the forth parameter of xilinx_timer_tlr_period.
>> > >
>> > > Hm, it looks like I immediately cast period to a u64. I will change the
>> > > signature for this function next revision.
>> >
>> > Then note that period * clk_get_rate(priv->clk) might overflow.
>>
>> Ok, so is mult_frac the correct macro to use here?
>>
>> > > >> +	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
>> > > >> +	if (ret)
>> > > >> +		return ret;
>>
>> Perhaps I should add
>>
>> 	if (tlr0 <= tlr1)
>> 		return -EINVAL;
>>
>> here to prevent accidentally getting 0% duty cycle.
>
> You can assume that duty_cycle <= period when .apply is called.

Ok, I will only check for == then.

--Sean

Uwe Kleine-König June 28, 2021, 4:24 p.m. UTC | #9

Hello Sean,

On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
> On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
> > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
> > > So for the moment, why not give an error? This will be legal code both
> > > now and after round_state is implemented.
> > 
> > The problem is where to draw the line. To stay with your example: If a
> > request for period = 150 ns comes in, and let X be the biggest period <=
> > 150 ns that the hardware can configure. For which values of X should an
> > error be returned and for which values the setting should be
> > implemented.
> > 
> > In my eyes the only sensible thing to implement here is to tell the
> > consumer about X and let it decide if it's good enough. If you have a
> > better idea let me hear about it.
> 
> Sure. And I think it's ok to tell the consumer that X is the best we can
> do. But if they go along and request an unconfigurable state anyway, we
> should tell them as much.

I have the impression you didn't understand where I see the problem. If
you request 150 ns and the controller can only do 149 ns (or 149.6667 ns)
should we refuse? If yes: This is very unusable, e.g. the led-pwm driver
expects that it can configure the duty_cycle in 1/256 steps of the
period, and then maybe only steps 27 and 213 of the 256 possible steps
work. (This example doesn't really match because the led-pwm driver
varies duty_cycle and not period, but the principle becomes clear I
assume.) If no: Should we accept 151 ns? Isn't that ridiculous?

> IMO, this is the best way to prevent surprising results in the API.

I think it's not possible in practise to refuse "near" misses and every
definition of "near" is in some case ridiculous. Also if you consider
the pwm_round_state() case you don't want to refuse any request to tell
as much as possible about your controller's capabilities. And then it's
straight forward to let apply behave in the same way to keep complexity
low.

> The real issue here is that it is impossible to determine the correct
> way to round the PWM a priori, and in particular, without considering
> both duty_cycle and period. If a consumer requests very small
> period/duty cycle which we cannot produce, how should it be rounded?

Yeah, because there is no obviously right one, I picked one that is as
wrong as the other possibilities but is easy to work with.

> Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
> the least period? Or should we try and increase the period to better
> approximate the % duty cycle? And both of these decisions must be made
> knowing both parameters. We cannot (for example) just always round up,
> since we may produce a configuration with TLR0 == TLR1, which would
> produce 0% duty cycle instead of whatever was requested. Rounding rate
> will introduce significant complexity into the driver. Most of the time
> if a consumer requests an invalid rate, it is due to misconfiguration
> which is best solved by fixing the configuration.

In the first step pick the biggest period not bigger than the requested
and then pick the biggest duty cycle that is not bigger than the
requested and that can be set with the just picked period. That is the
behaviour that all new drivers should do. This is somewhat arbitrary but
after quite some thought the most sensible in my eyes.

> > > Perhaps I should add
> > > 
> > > 	if (tlr0 <= tlr1)
> > > 		return -EINVAL;
> > > 
> > > here to prevent accidentally getting 0% duty cycle.
> > 
> > You can assume that duty_cycle <= period when .apply is called.
> 
> Ok, I will only check for == then.

You just have to pay attention to the case that you had to decrement
.period to the next possible value. Then .duty_cycle might be bigger
than the corrected period.

Best regards
Uwe

Sean Anderson June 28, 2021, 4:35 p.m. UTC | #10

On 6/28/21 12:24 PM, Uwe Kleine-König wrote:
> Hello Sean,
>
> On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
>> On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
>> > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
>> > > So for the moment, why not give an error? This will be legal code both
>> > > now and after round_state is implemented.
>> >
>> > The problem is where to draw the line. To stay with your example: If a
>> > request for period = 150 ns comes in, and let X be the biggest period <=
>> > 150 ns that the hardware can configure. For which values of X should an
>> > error be returned and for which values the setting should be
>> > implemented.
>> >
>> > In my eyes the only sensible thing to implement here is to tell the
>> > consumer about X and let it decide if it's good enough. If you have a
>> > better idea let me hear about it.
>>
>> Sure. And I think it's ok to tell the consumer that X is the best we can
>> do. But if they go along and request an unconfigurable state anyway, we
>> should tell them as much.
>
> I have the impression you didn't understand where I see the problem. If
> you request 150 ns and the controller can only do 149 ns (or 149.6667 ns)
> should we refuse? If yes: This is very unusable, e.g. the led-pwm driver
> expects that it can configure the duty_cycle in 1/256 steps of the
> period, and then maybe only steps 27 and 213 of the 256 possible steps
> work. (This example doesn't really match because the led-pwm driver
> varies duty_cycle and not period, but the principle becomes clear I
> assume.) If no: Should we accept 151 ns? Isn't that ridiculous?

I am fine with this sort of rounding. The part I take issue with is when
the consumer requests (e.g.) a 10ns period, but the best we can do is
20ns. Or at the other end if they request a 4s period but the best we
can do is 2s. Here, there is no obvious way to round it, so I think we
should just say "come back with a reasonable period" and let whoever
wrote the device tree pick a better period.

>> IMO, this is the best way to prevent surprising results in the API.
>
> I think it's not possible in practise to refuse "near" misses and every
> definition of "near" is in some case ridiculous. Also if you consider
> the pwm_round_state() case you don't want to refuse any request to tell
> as much as possible about your controller's capabilities. And then it's
> straight forward to let apply behave in the same way to keep complexity
> low.
>
>> The real issue here is that it is impossible to determine the correct
>> way to round the PWM a priori, and in particular, without considering
>> both duty_cycle and period. If a consumer requests very small
>> period/duty cycle which we cannot produce, how should it be rounded?
>
> Yeah, because there is no obviously right one, I picked one that is as
> wrong as the other possibilities but is easy to work with.
>
>> Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
>> the least period? Or should we try and increase the period to better
>> approximate the % duty cycle? And both of these decisions must be made
>> knowing both parameters. We cannot (for example) just always round up,
>> since we may produce a configuration with TLR0 == TLR1, which would
>> produce 0% duty cycle instead of whatever was requested. Rounding rate
>> will introduce significant complexity into the driver. Most of the time
>> if a consumer requests an invalid rate, it is due to misconfiguration
>> which is best solved by fixing the configuration.
>
> In the first step pick the biggest period not bigger than the requested
> and then pick the biggest duty cycle that is not bigger than the
> requested and that can be set with the just picked period. That is the
> behaviour that all new drivers should do. This is somewhat arbitrary but
> after quite some thought the most sensible in my eyes.

And if there are no periods smaller than the requested period?

Any way you slice it, there will be situations where there is nothing
reasonable to do other than return an error.

>> > > Perhaps I should add
>> > >
>> > > 	if (tlr0 <= tlr1)
>> > > 		return -EINVAL;
>> > >
>> > > here to prevent accidentally getting 0% duty cycle.
>> >
>> > You can assume that duty_cycle <= period when .apply is called.
>>
>> Ok, I will only check for == then.
>
> You just have to pay attention to the case that you had to decrement
> .period to the next possible value. Then .duty_cycle might be bigger
> than the corrected period.

This is specifically to prevent 100% duty cycle from turning into 0%. My
current draft is

	/*
	 * If TLR0 == TLR1, then we will produce 0% duty cycle instead of 100%
	 * duty cycle. Try and reduce the high time to compensate. If we can't
	 * do that because the high time is already 0 cycles, then just error
	 * out.
	 */
	if (tlr0 == tlr1 && !tlr1--)
		return -EINVAL;

--Sean

Uwe Kleine-König June 28, 2021, 5:20 p.m. UTC | #11

Hello Sean,

On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
> On 6/28/21 12:24 PM, Uwe Kleine-König wrote:
> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
> > > On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
> > > > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
> > > > > So for the moment, why not give an error? This will be legal code both
> > > > > now and after round_state is implemented.
> > > >
> > > > The problem is where to draw the line. To stay with your example: If a
> > > > request for period = 150 ns comes in, and let X be the biggest period <=
> > > > 150 ns that the hardware can configure. For which values of X should an
> > > > error be returned and for which values the setting should be
> > > > implemented.
> > > >
> > > > In my eyes the only sensible thing to implement here is to tell the
> > > > consumer about X and let it decide if it's good enough. If you have a
> > > > better idea let me hear about it.
> > > 
> > > Sure. And I think it's ok to tell the consumer that X is the best we can
> > > do. But if they go along and request an unconfigurable state anyway, we
> > > should tell them as much.
> > 
> > I have the impression you didn't understand where I see the problem. If
> > you request 150 ns and the controller can only do 149 ns (or 149.6667 ns)
> > should we refuse? If yes: This is very unusable, e.g. the led-pwm driver
> > expects that it can configure the duty_cycle in 1/256 steps of the
> > period, and then maybe only steps 27 and 213 of the 256 possible steps
> > work. (This example doesn't really match because the led-pwm driver
> > varies duty_cycle and not period, but the principle becomes clear I
> > assume.) If no: Should we accept 151 ns? Isn't that ridiculous?
> 
> I am fine with this sort of rounding. The part I take issue with is when
> the consumer requests (e.g.) a 10ns period, but the best we can do is
> 20ns. Or at the other end if they request a 4s period but the best we
> can do is 2s. Here, there is no obvious way to round it, so I think we
> should just say "come back with a reasonable period" and let whoever
> wrote the device tree pick a better period.

Note that giving ridiculus examples is easy, but this doesn't help to
actually implement something sensible. Please tell us for your example
where the driver can only implement 20 ns what is the smallest requested
period the driver should accept.

> > > IMO, this is the best way to prevent surprising results in the API.
> > 
> > I think it's not possible in practise to refuse "near" misses and every
> > definition of "near" is in some case ridiculous. Also if you consider
> > the pwm_round_state() case you don't want to refuse any request to tell
> > as much as possible about your controller's capabilities. And then it's
> > straight forward to let apply behave in the same way to keep complexity
> > low.
> > 
> > > The real issue here is that it is impossible to determine the correct
> > > way to round the PWM a priori, and in particular, without considering
> > > both duty_cycle and period. If a consumer requests very small
> > > period/duty cycle which we cannot produce, how should it be rounded?
> > 
> > Yeah, because there is no obviously right one, I picked one that is as
> > wrong as the other possibilities but is easy to work with.
> > 
> > > Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
> > > the least period? Or should we try and increase the period to better
> > > approximate the % duty cycle? And both of these decisions must be made
> > > knowing both parameters. We cannot (for example) just always round up,
> > > since we may produce a configuration with TLR0 == TLR1, which would
> > > produce 0% duty cycle instead of whatever was requested. Rounding rate
> > > will introduce significant complexity into the driver. Most of the time
> > > if a consumer requests an invalid rate, it is due to misconfiguration
> > > which is best solved by fixing the configuration.
> > 
> > In the first step pick the biggest period not bigger than the requested
> > and then pick the biggest duty cycle that is not bigger than the
> > requested and that can be set with the just picked period. That is the
> > behaviour that all new drivers should do. This is somewhat arbitrary but
> > after quite some thought the most sensible in my eyes.
> 
> And if there are no periods smaller than the requested period?

Then return -ERANGE.

> Any way you slice it, there will be situations where there is nothing
> reasonable to do other than return an error.

ack.

> > > > > Perhaps I should add
> > > > >
> > > > > 	if (tlr0 <= tlr1)
> > > > > 		return -EINVAL;
> > > > >
> > > > > here to prevent accidentally getting 0% duty cycle.
> > > >
> > > > You can assume that duty_cycle <= period when .apply is called.
> > > 
> > > Ok, I will only check for == then.
> > 
> > You just have to pay attention to the case that you had to decrement
> > .period to the next possible value. Then .duty_cycle might be bigger
> > than the corrected period.
> 
> This is specifically to prevent 100% duty cycle from turning into 0%. My
> current draft is
> 
> 	/*
> 	 * If TLR0 == TLR1, then we will produce 0% duty cycle instead of 100%
> 	 * duty cycle. Try and reduce the high time to compensate. If we can't
> 	 * do that because the high time is already 0 cycles, then just error
> 	 * out.
> 	 */
> 	if (tlr0 == tlr1 && !tlr1--)
> 		return -EINVAL;

If you follow my suggested policy this isn't an error and you should
yield the biggest duty_cycle here even if it is zero.

Best regards
Uwe

Sean Anderson June 28, 2021, 5:41 p.m. UTC | #12

On 6/28/21 1:20 PM, Uwe Kleine-König wrote:
 > Hello Sean,
 >
 > On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
 >> On 6/28/21 12:24 PM, Uwe Kleine-König wrote:
 >> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
 >> > > On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
 >> > > > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
 >> > > > > So for the moment, why not give an error? This will be legal code both
 >> > > > > now and after round_state is implemented.
 >> > > >
 >> > > > The problem is where to draw the line. To stay with your example: If a
 >> > > > request for period = 150 ns comes in, and let X be the biggest period <=
 >> > > > 150 ns that the hardware can configure. For which values of X should an
 >> > > > error be returned and for which values the setting should be
 >> > > > implemented.
 >> > > >
 >> > > > In my eyes the only sensible thing to implement here is to tell the
 >> > > > consumer about X and let it decide if it's good enough. If you have a
 >> > > > better idea let me hear about it.
 >> > >
 >> > > Sure. And I think it's ok to tell the consumer that X is the best we can
 >> > > do. But if they go along and request an unconfigurable state anyway, we
 >> > > should tell them as much.
 >> >
 >> > I have the impression you didn't understand where I see the problem. If
 >> > you request 150 ns and the controller can only do 149 ns (or 149.6667 ns)
 >> > should we refuse? If yes: This is very unusable, e.g. the led-pwm driver
 >> > expects that it can configure the duty_cycle in 1/256 steps of the
 >> > period, and then maybe only steps 27 and 213 of the 256 possible steps
 >> > work. (This example doesn't really match because the led-pwm driver
 >> > varies duty_cycle and not period, but the principle becomes clear I
 >> > assume.) If no: Should we accept 151 ns? Isn't that ridiculous?
 >>
 >> I am fine with this sort of rounding. The part I take issue with is when
 >> the consumer requests (e.g.) a 10ns period, but the best we can do is
 >> 20ns. Or at the other end if they request a 4s period but the best we
 >> can do is 2s. Here, there is no obvious way to round it, so I think we
 >> should just say "come back with a reasonable period" and let whoever
 >> wrote the device tree pick a better period.
 >
 > Note that giving ridiculus examples is easy, but this doesn't help to
 > actually implement something sensible. Please tell us for your example
 > where the driver can only implement 20 ns what is the smallest requested
 > period the driver should accept.

20ns :)

In the case of this device, that would result in 0% duty cycle with a
100MHz input. So the smallest reasonable period is 30ns with a duty
cycle of 20ns.

 >
 >> > > IMO, this is the best way to prevent surprising results in the API.
 >> >
 >> > I think it's not possible in practise to refuse "near" misses and every
 >> > definition of "near" is in some case ridiculous. Also if you consider
 >> > the pwm_round_state() case you don't want to refuse any request to tell
 >> > as much as possible about your controller's capabilities. And then it's
 >> > straight forward to let apply behave in the same way to keep complexity
 >> > low.
 >> >
 >> > > The real issue here is that it is impossible to determine the correct
 >> > > way to round the PWM a priori, and in particular, without considering
 >> > > both duty_cycle and period. If a consumer requests very small
 >> > > period/duty cycle which we cannot produce, how should it be rounded?
 >> >
 >> > Yeah, because there is no obviously right one, I picked one that is as
 >> > wrong as the other possibilities but is easy to work with.
 >> >
 >> > > Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
 >> > > the least period? Or should we try and increase the period to better
 >> > > approximate the % duty cycle? And both of these decisions must be made
 >> > > knowing both parameters. We cannot (for example) just always round up,
 >> > > since we may produce a configuration with TLR0 == TLR1, which would
 >> > > produce 0% duty cycle instead of whatever was requested. Rounding rate
 >> > > will introduce significant complexity into the driver. Most of the time
 >> > > if a consumer requests an invalid rate, it is due to misconfiguration
 >> > > which is best solved by fixing the configuration.
 >> >
 >> > In the first step pick the biggest period not bigger than the requested
 >> > and then pick the biggest duty cycle that is not bigger than the
 >> > requested and that can be set with the just picked period. That is the
 >> > behaviour that all new drivers should do. This is somewhat arbitrary but
 >> > after quite some thought the most sensible in my eyes.
 >>
 >> And if there are no periods smaller than the requested period?
 >
 > Then return -ERANGE.

Ok, so instead of

	if (cycles < 2 || cycles > priv->max + 2)
		return -ERANGE;

you would prefer

	if (cycles < 2)
		return -ERANGE;
	else if (cycles > priv->max + 2)
		cycles = priv->max;

But if we do the above clamping for TLR0, then we have to recalculate
the duty cycle for TLR1. Which I guess means doing something like

	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
	if (ret)
		return ret;

	state->duty_cycle = mult_frac(state->duty_cycle,
				      xilinx_timer_get_period(priv, tlr0, tcsr0),
				      state->period);

	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
	if (ret)
		return ret;



 >
 >> Any way you slice it, there will be situations where there is nothing
 >> reasonable to do other than return an error.
 >
 > ack.
 >
 >> > > > > Perhaps I should add
 >> > > > >
 >> > > > > 	if (tlr0 <= tlr1)
 >> > > > > 		return -EINVAL;
 >> > > > >
 >> > > > > here to prevent accidentally getting 0% duty cycle.
 >> > > >
 >> > > > You can assume that duty_cycle <= period when .apply is called.
 >> > >
 >> > > Ok, I will only check for == then.
 >> >
 >> > You just have to pay attention to the case that you had to decrement
 >> > .period to the next possible value. Then .duty_cycle might be bigger
 >> > than the corrected period.
 >>
 >> This is specifically to prevent 100% duty cycle from turning into 0%. My
 >> current draft is
 >>
 >> 	/*
 >> 	 * If TLR0 == TLR1, then we will produce 0% duty cycle instead of 100%
 >> 	 * duty cycle. Try and reduce the high time to compensate. If we can't
 >> 	 * do that because the high time is already 0 cycles, then just error
 >> 	 * out.
 >> 	 */
 >> 	if (tlr0 == tlr1 && !tlr1--)
 >> 		return -EINVAL;
 >
 > If you follow my suggested policy this isn't an error and you should
 > yield the biggest duty_cycle here even if it is zero.

So like this?

	if (tlr0 == tlr1) {
		if (tlr1)
			tlr1--;
		else if (tlr0 != priv->max)
			tlr0++;
		else
			return -ERANGE;
	}

And I would really appreciate if you could write up some documentation
with common errors and how to handle them. It's not at all obvious to me
what all the implications of the above guidelines are.

--Sean

Uwe Kleine-König June 29, 2021, 8:31 a.m. UTC | #13

Hello Sean,

On Mon, Jun 28, 2021 at 01:41:43PM -0400, Sean Anderson wrote:
> On 6/28/21 1:20 PM, Uwe Kleine-König wrote:
> > On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
> >> On 6/28/21 12:24 PM, Uwe Kleine-König wrote:
> >> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
> >> > > On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
> >> > > > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
> >> > > > > So for the moment, why not give an error? This will be legal code both
> >> > > > > now and after round_state is implemented.
> >> > > >
> >> > > > The problem is where to draw the line. To stay with your example: If a
> >> > > > request for period = 150 ns comes in, and let X be the biggest period <=
> >> > > > 150 ns that the hardware can configure. For which values of X should an
> >> > > > error be returned and for which values the setting should be
> >> > > > implemented.
> >> > > >
> >> > > > In my eyes the only sensible thing to implement here is to tell the
> >> > > > consumer about X and let it decide if it's good enough. If you have a
> >> > > > better idea let me hear about it.
> >> > >
> >> > > Sure. And I think it's ok to tell the consumer that X is the best we can
> >> > > do. But if they go along and request an unconfigurable state anyway, we
> >> > > should tell them as much.
> >> >
> >> > I have the impression you didn't understand where I see the problem. If
> >> > you request 150 ns and the controller can only do 149 ns (or 149.6667 ns)
> >> > should we refuse? If yes: This is very unusable, e.g. the led-pwm driver
> >> > expects that it can configure the duty_cycle in 1/256 steps of the
> >> > period, and then maybe only steps 27 and 213 of the 256 possible steps
> >> > work. (This example doesn't really match because the led-pwm driver
> >> > varies duty_cycle and not period, but the principle becomes clear I
> >> > assume.) If no: Should we accept 151 ns? Isn't that ridiculous?
> >>
> >> I am fine with this sort of rounding. The part I take issue with is when
> >> the consumer requests (e.g.) a 10ns period, but the best we can do is
> >> 20ns. Or at the other end if they request a 4s period but the best we
> >> can do is 2s. Here, there is no obvious way to round it, so I think we
> >> should just say "come back with a reasonable period" and let whoever
> >> wrote the device tree pick a better period.
> >
> > Note that giving ridiculus examples is easy, but this doesn't help to
> > actually implement something sensible. Please tell us for your example
> > where the driver can only implement 20 ns what is the smallest requested
> > period the driver should accept.
> 
> 20ns :)
> 
> In the case of this device, that would result in 0% duty cycle with a
> 100MHz input. So the smallest reasonable period is 30ns with a duty
> cycle of 20ns.

I took the time to understand the hardware a bit better, also to be able
to reply to your formulae below. So to recap (and simplify slightly
assuming TCSR_UDT = 1):


              TLR0 + 2
 period     = --------
              clkrate

              TLR1 + 2
 duty_cycle = -------- if TLR1 < TLR0, else 0
              clkrate


where TLRx has the range [0..0xffffffff] (for some devices the range is
smaller). So clkrate seems to be 100 MHz?

> >> > > IMO, this is the best way to prevent surprising results in the API.
> >> >
> >> > I think it's not possible in practise to refuse "near" misses and every
> >> > definition of "near" is in some case ridiculous. Also if you consider
> >> > the pwm_round_state() case you don't want to refuse any request to tell
> >> > as much as possible about your controller's capabilities. And then it's
> >> > straight forward to let apply behave in the same way to keep complexity
> >> > low.
> >> >
> >> > > The real issue here is that it is impossible to determine the correct
> >> > > way to round the PWM a priori, and in particular, without considering
> >> > > both duty_cycle and period. If a consumer requests very small
> >> > > period/duty cycle which we cannot produce, how should it be rounded?
> >> >
> >> > Yeah, because there is no obviously right one, I picked one that is as
> >> > wrong as the other possibilities but is easy to work with.
> >> >
> >> > > Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
> >> > > the least period? Or should we try and increase the period to better
> >> > > approximate the % duty cycle? And both of these decisions must be made
> >> > > knowing both parameters. We cannot (for example) just always round up,
> >> > > since we may produce a configuration with TLR0 == TLR1, which would
> >> > > produce 0% duty cycle instead of whatever was requested. Rounding rate
> >> > > will introduce significant complexity into the driver. Most of the time
> >> > > if a consumer requests an invalid rate, it is due to misconfiguration
> >> > > which is best solved by fixing the configuration.
> >> >
> >> > In the first step pick the biggest period not bigger than the requested
> >> > and then pick the biggest duty cycle that is not bigger than the
> >> > requested and that can be set with the just picked period. That is the
> >> > behaviour that all new drivers should do. This is somewhat arbitrary but
> >> > after quite some thought the most sensible in my eyes.
> >>
> >> And if there are no periods smaller than the requested period?
> >
> > Then return -ERANGE.
> 
> Ok, so instead of
> 
> 	if (cycles < 2 || cycles > priv->max + 2)
> 		return -ERANGE;
> 
> you would prefer
> 
> 	if (cycles < 2)
> 		return -ERANGE;
> 	else if (cycles > priv->max + 2)
> 		cycles = priv->max;

The actual calculation is a bit harder to handle TCSR_UDT = 0 but in
principle, yes, but see below.

> But if we do the above clamping for TLR0, then we have to recalculate
> the duty cycle for TLR1. Which I guess means doing something like
> 
> 	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
> 	if (ret)
> 		return ret;
> 
> 	state->duty_cycle = mult_frac(state->duty_cycle,
> 				      xilinx_timer_get_period(priv, tlr0, tcsr0),
> 				      state->period);
> 
> 	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
> 	if (ret)
> 		return ret;

No, you need something like:

	/*
	 * The multiplication cannot overflow as both priv_max and
	 * NSEC_PER_SEC fit into an u32.
	 */
	max_period = div64_ul((u64)priv->max * NSEC_PER_SEC, clkrate);

	/* cap period to the maximal possible value */
	if (state->period > max_period)
		period = max_period;
	else
		period = state->period;

	/* cap duty_cycle to the maximal possible value */
	if (state->duty_cycle > max_period)
		duty_cycle = max_period;
	else
		duty_cycle = state->duty_cycle;

	period_cycles = period * clkrate / NSEC_PER_SEC;

	if (period_cycles < 2)
		return -ERANGE;

	duty_cycles = duty_cycle * clkrate / NSEC_PER_SEC;

	/*
	 * The hardware cannot emit a 100% relative duty cycle, if
	 * duty_cycle >= period_cycles is programmed the hardware emits
	 * a 0% relative duty cycle.
	 */
	if (duty_cycle == period_cycles)
		duty_cycles = period_cycles - 1;

	/*
	 * The hardware cannot emit a duty_cycle of one clk step, so
	 * emit 0 instead.
	 */
	if (duty_cycles < 2)
		duty_cycles = period_cycles;

> >> > > > > Perhaps I should add
> >> > > > >
> >> > > > > 	if (tlr0 <= tlr1)
> >> > > > > 		return -EINVAL;
> >> > > > >
> >> > > > > here to prevent accidentally getting 0% duty cycle.
> >> > > >
> >> > > > You can assume that duty_cycle <= period when .apply is called.
> >> > >
> >> > > Ok, I will only check for == then.
> >> >
> >> > You just have to pay attention to the case that you had to decrement
> >> > .period to the next possible value. Then .duty_cycle might be bigger
> >> > than the corrected period.
> >>
> >> This is specifically to prevent 100% duty cycle from turning into 0%. My
> >> current draft is
> >>
> >> 	/*
> >> 	 * If TLR0 == TLR1, then we will produce 0% duty cycle instead of 100%
> >> 	 * duty cycle. Try and reduce the high time to compensate. If we can't
> >> 	 * do that because the high time is already 0 cycles, then just error
> >> 	 * out.
> >> 	 */
> >> 	if (tlr0 == tlr1 && !tlr1--)
> >> 		return -EINVAL;
> >
> > If you follow my suggested policy this isn't an error and you should
> > yield the biggest duty_cycle here even if it is zero.
> 
> So like this?
> 
> 	if (tlr0 == tlr1) {
> 		if (tlr1)
> 			tlr1--;
> 		else if (tlr0 != priv->max)
> 			tlr0++;
> 		else
> 			return -ERANGE;
> 	}

No, this is wrong as it configures a longer period than requested in
some cases.

> And I would really appreciate if you could write up some documentation
> with common errors and how to handle them. It's not at all obvious to me
> what all the implications of the above guidelines are.

Yes, I fully agree this should be documented and doing that is on my
todo list. Until I come around to do this, enabling PWM_DEBUG should
help you getting this right (assuming you test extensively and read the
resulting kernel messages).

Best regards
Uwe

Sean Anderson June 29, 2021, 6:01 p.m. UTC | #14

On 6/29/21 4:31 AM, Uwe Kleine-König wrote:
 > Hello Sean,
 >
 > On Mon, Jun 28, 2021 at 01:41:43PM -0400, Sean Anderson wrote:
 >> On 6/28/21 1:20 PM, Uwe Kleine-König wrote:
 >> > On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
 >> >> On 6/28/21 12:24 PM, Uwe Kleine-König wrote:
 >> >> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
 >> >> > > On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
 >> >> > > > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
 >> >> > > > > So for the moment, why not give an error? This will be legal code both
 >> >> > > > > now and after round_state is implemented.
 >> >> > > >
 >> >> > > > The problem is where to draw the line. To stay with your example: If a
 >> >> > > > request for period = 150 ns comes in, and let X be the biggest period <=
 >> >> > > > 150 ns that the hardware can configure. For which values of X should an
 >> >> > > > error be returned and for which values the setting should be
 >> >> > > > implemented.
 >> >> > > >
 >> >> > > > In my eyes the only sensible thing to implement here is to tell the
 >> >> > > > consumer about X and let it decide if it's good enough. If you have a
 >> >> > > > better idea let me hear about it.
 >> >> > >
 >> >> > > Sure. And I think it's ok to tell the consumer that X is the best we can
 >> >> > > do. But if they go along and request an unconfigurable state anyway, we
 >> >> > > should tell them as much.
 >> >> >
 >> >> > I have the impression you didn't understand where I see the problem. If
 >> >> > you request 150 ns and the controller can only do 149 ns (or 149.6667 ns)
 >> >> > should we refuse? If yes: This is very unusable, e.g. the led-pwm driver
 >> >> > expects that it can configure the duty_cycle in 1/256 steps of the
 >> >> > period, and then maybe only steps 27 and 213 of the 256 possible steps
 >> >> > work. (This example doesn't really match because the led-pwm driver
 >> >> > varies duty_cycle and not period, but the principle becomes clear I
 >> >> > assume.) If no: Should we accept 151 ns? Isn't that ridiculous?
 >> >>
 >> >> I am fine with this sort of rounding. The part I take issue with is when
 >> >> the consumer requests (e.g.) a 10ns period, but the best we can do is
 >> >> 20ns. Or at the other end if they request a 4s period but the best we
 >> >> can do is 2s. Here, there is no obvious way to round it, so I think we
 >> >> should just say "come back with a reasonable period" and let whoever
 >> >> wrote the device tree pick a better period.
 >> >
 >> > Note that giving ridiculus examples is easy, but this doesn't help to
 >> > actually implement something sensible. Please tell us for your example
 >> > where the driver can only implement 20 ns what is the smallest requested
 >> > period the driver should accept.
 >>
 >> 20ns :)
 >>
 >> In the case of this device, that would result in 0% duty cycle with a
 >> 100MHz input. So the smallest reasonable period is 30ns with a duty
 >> cycle of 20ns.
 >
 > I took the time to understand the hardware a bit better, also to be able
 > to reply to your formulae below. So to recap (and simplify slightly
 > assuming TCSR_UDT = 1):
 >
 >
 >                TLR0 + 2
 >   period     = --------
 >                clkrate
 >
 >                TLR1 + 2
 >   duty_cycle = -------- if TLR1 < TLR0, else 0
 >                clkrate
 >
 >
 > where TLRx has the range [0..0xffffffff] (for some devices the range is
 > smaller). So clkrate seems to be 100 MHz?

On my system, yes.

 >
 >> >> > > IMO, this is the best way to prevent surprising results in the API.
 >> >> >
 >> >> > I think it's not possible in practise to refuse "near" misses and every
 >> >> > definition of "near" is in some case ridiculous. Also if you consider
 >> >> > the pwm_round_state() case you don't want to refuse any request to tell
 >> >> > as much as possible about your controller's capabilities. And then it's
 >> >> > straight forward to let apply behave in the same way to keep complexity
 >> >> > low.
 >> >> >
 >> >> > > The real issue here is that it is impossible to determine the correct
 >> >> > > way to round the PWM a priori, and in particular, without considering
 >> >> > > both duty_cycle and period. If a consumer requests very small
 >> >> > > period/duty cycle which we cannot produce, how should it be rounded?
 >> >> >
 >> >> > Yeah, because there is no obviously right one, I picked one that is as
 >> >> > wrong as the other possibilities but is easy to work with.
 >> >> >
 >> >> > > Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
 >> >> > > the least period? Or should we try and increase the period to better
 >> >> > > approximate the % duty cycle? And both of these decisions must be made
 >> >> > > knowing both parameters. We cannot (for example) just always round up,
 >> >> > > since we may produce a configuration with TLR0 == TLR1, which would
 >> >> > > produce 0% duty cycle instead of whatever was requested. Rounding rate
 >> >> > > will introduce significant complexity into the driver. Most of the time
 >> >> > > if a consumer requests an invalid rate, it is due to misconfiguration
 >> >> > > which is best solved by fixing the configuration.
 >> >> >
 >> >> > In the first step pick the biggest period not bigger than the requested
 >> >> > and then pick the biggest duty cycle that is not bigger than the
 >> >> > requested and that can be set with the just picked period. That is the
 >> >> > behaviour that all new drivers should do. This is somewhat arbitrary but
 >> >> > after quite some thought the most sensible in my eyes.
 >> >>
 >> >> And if there are no periods smaller than the requested period?
 >> >
 >> > Then return -ERANGE.
 >>
 >> Ok, so instead of
 >>
 >> 	if (cycles < 2 || cycles > priv->max + 2)
 >> 		return -ERANGE;
 >>
 >> you would prefer
 >>
 >> 	if (cycles < 2)
 >> 		return -ERANGE;
 >> 	else if (cycles > priv->max + 2)
 >> 		cycles = priv->max;
 >
 > The actual calculation is a bit harder to handle TCSR_UDT = 0 but in
 > principle, yes, but see below.
 >
 >> But if we do the above clamping for TLR0, then we have to recalculate
 >> the duty cycle for TLR1. Which I guess means doing something like
 >>
 >> 	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
 >> 	if (ret)
 >> 		return ret;
 >>
 >> 	state->duty_cycle = mult_frac(state->duty_cycle,
 >> 				      xilinx_timer_get_period(priv, tlr0, tcsr0),
 >> 				      state->period);
 >>
 >> 	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
 >> 	if (ret)
 >> 		return ret;
 >
 > No, you need something like:
 >
 > 	/*
 > 	 * The multiplication cannot overflow as both priv_max and
 > 	 * NSEC_PER_SEC fit into an u32.
 > 	 */
 > 	max_period = div64_ul((u64)priv->max * NSEC_PER_SEC, clkrate);
 >
 > 	/* cap period to the maximal possible value */
 > 	if (state->period > max_period)
 > 		period = max_period;
 > 	else
 > 		period = state->period;
 >
 > 	/* cap duty_cycle to the maximal possible value */
 > 	if (state->duty_cycle > max_period)
 > 		duty_cycle = max_period;
 > 	else
 > 		duty_cycle = state->duty_cycle;

These caps may increase the % duty cycle. For example, consider when the
max is 100, and the user requests a period of 150 and a duty cycle of
75, for a % duty cycle of 50%. The current logic is equivalent to

	period = min(state->period, max_period);
	duty_cycle = min(state->duty_cycle, max_period);

Which will result in a period of 100 and a duty cycle of 75, for a 75%
duty cycle. Instead, we should do

	period = min(state->period, max_period);
	duty_cycle = mult_frac(state->duty_cycle, period, state->period);

which will result in a period of 100 and a duty cycle of 50.

 > 	period_cycles = period * clkrate / NSEC_PER_SEC;
 >
 > 	if (period_cycles < 2)
 > 		return -ERANGE;
 >
 > 	duty_cycles = duty_cycle * clkrate / NSEC_PER_SEC;
 >
 > 	/*
 > 	 * The hardware cannot emit a 100% relative duty cycle, if
 > 	 * duty_cycle >= period_cycles is programmed the hardware emits
 > 	 * a 0% relative duty cycle.
 > 	 */
 > 	if (duty_cycle == period_cycles)
 > 		duty_cycles = period_cycles - 1;
 >
 > 	/*
 > 	 * The hardware cannot emit a duty_cycle of one clk step, so
 > 	 * emit 0 instead.
 > 	 */
 > 	if (duty_cycles < 2)
 > 		duty_cycles = period_cycles;

Of course, the above may result in 100% duty cycle being rounded down to
0%. I feel like that is too big of a jump to ignore. Perhaps if we
cannot return -ERANGE we should at least dev_warn.

--Sean

 >> >> > > > > Perhaps I should add
 >> >> > > > >
 >> >> > > > > 	if (tlr0 <= tlr1)
 >> >> > > > > 		return -EINVAL;
 >> >> > > > >
 >> >> > > > > here to prevent accidentally getting 0% duty cycle.
 >> >> > > >
 >> >> > > > You can assume that duty_cycle <= period when .apply is called.
 >> >> > >
 >> >> > > Ok, I will only check for == then.
 >> >> >
 >> >> > You just have to pay attention to the case that you had to decrement
 >> >> > .period to the next possible value. Then .duty_cycle might be bigger
 >> >> > than the corrected period.
 >> >>
 >> >> This is specifically to prevent 100% duty cycle from turning into 0%. My
 >> >> current draft is
 >> >>
 >> >> 	/*
 >> >> 	 * If TLR0 == TLR1, then we will produce 0% duty cycle instead of 100%
 >> >> 	 * duty cycle. Try and reduce the high time to compensate. If we can't
 >> >> 	 * do that because the high time is already 0 cycles, then just error
 >> >> 	 * out.
 >> >> 	 */
 >> >> 	if (tlr0 == tlr1 && !tlr1--)
 >> >> 		return -EINVAL;
 >> >
 >> > If you follow my suggested policy this isn't an error and you should
 >> > yield the biggest duty_cycle here even if it is zero.
 >>
 >> So like this?
 >>
 >> 	if (tlr0 == tlr1) {
 >> 		if (tlr1)
 >> 			tlr1--;
 >> 		else if (tlr0 != priv->max)
 >> 			tlr0++;
 >> 		else
 >> 			return -ERANGE;
 >> 	}
 >
 > No, this is wrong as it configures a longer period than requested in
 > some cases.
 >
 >> And I would really appreciate if you could write up some documentation
 >> with common errors and how to handle them. It's not at all obvious to me
 >> what all the implications of the above guidelines are.
 >
 > Yes, I fully agree this should be documented and doing that is on my
 > todo list. Until I come around to do this, enabling PWM_DEBUG should
 > help you getting this right (assuming you test extensively and read the
 > resulting kernel messages).
 >
 > Best regards
 > Uwe
 >

Uwe Kleine-König June 29, 2021, 8:51 p.m. UTC | #15

Hello Sean,

On Tue, Jun 29, 2021 at 02:01:31PM -0400, Sean Anderson wrote:
> On 6/29/21 4:31 AM, Uwe Kleine-König wrote:
> > Hello Sean,
> >
> > On Mon, Jun 28, 2021 at 01:41:43PM -0400, Sean Anderson wrote:
> >> On 6/28/21 1:20 PM, Uwe Kleine-König wrote:
> >> > On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
> >> >> On 6/28/21 12:24 PM, Uwe Kleine-König wrote:
> >> >> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
> >> >> > > On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
> >> >> > > > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
> >> >> > > > > So for the moment, why not give an error? This will be legal code both
> >> >> > > > > now and after round_state is implemented.
> >> >> > > >
> >> >> > > > The problem is where to draw the line. To stay with your example: If a
> >> >> > > > request for period = 150 ns comes in, and let X be the biggest period <=
> >> >> > > > 150 ns that the hardware can configure. For which values of X should an
> >> >> > > > error be returned and for which values the setting should be
> >> >> > > > implemented.
> >> >> > > >
> >> >> > > > In my eyes the only sensible thing to implement here is to tell the
> >> >> > > > consumer about X and let it decide if it's good enough. If you have a
> >> >> > > > better idea let me hear about it.
> >> >> > >
> >> >> > > Sure. And I think it's ok to tell the consumer that X is the best we can
> >> >> > > do. But if they go along and request an unconfigurable state anyway, we
> >> >> > > should tell them as much.
> >> >> >
> >> >> > I have the impression you didn't understand where I see the problem. If
> >> >> > you request 150 ns and the controller can only do 149 ns (or 149.6667 ns)
> >> >> > should we refuse? If yes: This is very unusable, e.g. the led-pwm driver
> >> >> > expects that it can configure the duty_cycle in 1/256 steps of the
> >> >> > period, and then maybe only steps 27 and 213 of the 256 possible steps
> >> >> > work. (This example doesn't really match because the led-pwm driver
> >> >> > varies duty_cycle and not period, but the principle becomes clear I
> >> >> > assume.) If no: Should we accept 151 ns? Isn't that ridiculous?
> >> >>
> >> >> I am fine with this sort of rounding. The part I take issue with is when
> >> >> the consumer requests (e.g.) a 10ns period, but the best we can do is
> >> >> 20ns. Or at the other end if they request a 4s period but the best we
> >> >> can do is 2s. Here, there is no obvious way to round it, so I think we
> >> >> should just say "come back with a reasonable period" and let whoever
> >> >> wrote the device tree pick a better period.
> >> >
> >> > Note that giving ridiculus examples is easy, but this doesn't help to
> >> > actually implement something sensible. Please tell us for your example
> >> > where the driver can only implement 20 ns what is the smallest requested
> >> > period the driver should accept.
> >>
> >> 20ns :)
> >>
> >> In the case of this device, that would result in 0% duty cycle with a
> >> 100MHz input. So the smallest reasonable period is 30ns with a duty
> >> cycle of 20ns.
> >
> > I took the time to understand the hardware a bit better, also to be able
> > to reply to your formulae below. So to recap (and simplify slightly
> > assuming TCSR_UDT = 1):
> >
> >
> >                TLR0 + 2
> >   period     = --------
> >                clkrate
> >
> >                TLR1 + 2
> >   duty_cycle = -------- if TLR1 < TLR0, else 0
> >                clkrate
> >
> >
> > where TLRx has the range [0..0xffffffff] (for some devices the range is
> > smaller). So clkrate seems to be 100 MHz?
> 
> On my system, yes.
> 
> >
> >> >> > > IMO, this is the best way to prevent surprising results in the API.
> >> >> >
> >> >> > I think it's not possible in practise to refuse "near" misses and every
> >> >> > definition of "near" is in some case ridiculous. Also if you consider
> >> >> > the pwm_round_state() case you don't want to refuse any request to tell
> >> >> > as much as possible about your controller's capabilities. And then it's
> >> >> > straight forward to let apply behave in the same way to keep complexity
> >> >> > low.
> >> >> >
> >> >> > > The real issue here is that it is impossible to determine the correct
> >> >> > > way to round the PWM a priori, and in particular, without considering
> >> >> > > both duty_cycle and period. If a consumer requests very small
> >> >> > > period/duty cycle which we cannot produce, how should it be rounded?
> >> >> >
> >> >> > Yeah, because there is no obviously right one, I picked one that is as
> >> >> > wrong as the other possibilities but is easy to work with.
> >> >> >
> >> >> > > Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
> >> >> > > the least period? Or should we try and increase the period to better
> >> >> > > approximate the % duty cycle? And both of these decisions must be made
> >> >> > > knowing both parameters. We cannot (for example) just always round up,
> >> >> > > since we may produce a configuration with TLR0 == TLR1, which would
> >> >> > > produce 0% duty cycle instead of whatever was requested. Rounding rate
> >> >> > > will introduce significant complexity into the driver. Most of the time
> >> >> > > if a consumer requests an invalid rate, it is due to misconfiguration
> >> >> > > which is best solved by fixing the configuration.
> >> >> >
> >> >> > In the first step pick the biggest period not bigger than the requested
> >> >> > and then pick the biggest duty cycle that is not bigger than the
> >> >> > requested and that can be set with the just picked period. That is the
> >> >> > behaviour that all new drivers should do. This is somewhat arbitrary but
> >> >> > after quite some thought the most sensible in my eyes.
> >> >>
> >> >> And if there are no periods smaller than the requested period?
> >> >
> >> > Then return -ERANGE.
> >>
> >> Ok, so instead of
> >>
> >> 	if (cycles < 2 || cycles > priv->max + 2)
> >> 		return -ERANGE;
> >>
> >> you would prefer
> >>
> >> 	if (cycles < 2)
> >> 		return -ERANGE;
> >> 	else if (cycles > priv->max + 2)
> >> 		cycles = priv->max;
> >
> > The actual calculation is a bit harder to handle TCSR_UDT = 0 but in
> > principle, yes, but see below.
> >
> >> But if we do the above clamping for TLR0, then we have to recalculate
> >> the duty cycle for TLR1. Which I guess means doing something like
> >>
> >> 	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
> >> 	if (ret)
> >> 		return ret;
> >>
> >> 	state->duty_cycle = mult_frac(state->duty_cycle,
> >> 				      xilinx_timer_get_period(priv, tlr0, tcsr0),
> >> 				      state->period);
> >>
> >> 	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
> >> 	if (ret)
> >> 		return ret;
> >
> > No, you need something like:
> >
> > 	/*
> > 	 * The multiplication cannot overflow as both priv_max and
> > 	 * NSEC_PER_SEC fit into an u32.
> > 	 */
> > 	max_period = div64_ul((u64)priv->max * NSEC_PER_SEC, clkrate);
> >
> > 	/* cap period to the maximal possible value */
> > 	if (state->period > max_period)
> > 		period = max_period;
> > 	else
> > 		period = state->period;
> >
> > 	/* cap duty_cycle to the maximal possible value */
> > 	if (state->duty_cycle > max_period)
> > 		duty_cycle = max_period;
> > 	else
> > 		duty_cycle = state->duty_cycle;
> 
> These caps may increase the % duty cycle.

Correct.

For some usecases keeping the relative duty cycle might be better, for
others it might not. I'm still convinced that in general my solution
makes sense, is computationally cheaper and easier to work with.

> 
> > 	period_cycles = period * clkrate / NSEC_PER_SEC;
> >
> > 	if (period_cycles < 2)
> > 		return -ERANGE;
> >
> > 	duty_cycles = duty_cycle * clkrate / NSEC_PER_SEC;
> >
> > 	/*
> > 	 * The hardware cannot emit a 100% relative duty cycle, if
> > 	 * duty_cycle >= period_cycles is programmed the hardware emits
> > 	 * a 0% relative duty cycle.
> > 	 */
> > 	if (duty_cycle == period_cycles)
> > 		duty_cycles = period_cycles - 1;
> >
> > 	/*
> > 	 * The hardware cannot emit a duty_cycle of one clk step, so
> > 	 * emit 0 instead.
> > 	 */
> > 	if (duty_cycles < 2)
> > 		duty_cycles = period_cycles;
> 
> Of course, the above may result in 100% duty cycle being rounded down to
> 0%. I feel like that is too big of a jump to ignore. Perhaps if we
> cannot return -ERANGE we should at least dev_warn.

You did it again. You picked one single case that you consider bad but
didn't provide a constructive way to make it better.

Assume there was already a pwm_round_state function (that returns the
state that pwm_apply_state would implement for a given request) Consider
a consumer that wants say a 50% relative duty together with a small
period. So it first might call:

	ret = pwm_round_rate(pwm, { .period = 20, .duty_cycle = 20, ... }, &rounded_state)

to find out if .period = 20 can be implemented with the given PWM. If
this returns rounded state as:

	.period = 20
	.duty_cycle = 0

this says quite a lot about the pwm if the driver implements my policy.
(i.e.: The driver can do 20ns, but the biggest duty_cycle is only 0).
If however it returns -ERANGE this means (assuming the driver implements
the policy I try to convice you to be the right one) it means: The
hardware cannot implement 20 ns (or something smaller) and so the next
call probably tries 40 ns.

With your suggested semantic -ERANGE might mean:

 - The driver doesn't support .period = 20 ns
   (Follow up questions: What period should be tried next? 10 ns? 40
   ns? What if this returns -ERANGE again?)
 - The driver supports .period = 20 ns, but the biggest possible
   duty_cycle is "too different from 20 ns to ignore".

Then how should the search continue?

So: As soon as there is a pwm_round_rate function this can be catched
and then it's important that the drivers adhere to the expected policy.
Implementing this is a big thing and believe me I already spend quite
some brain cycles into it. Once the core is extended accordingly I will
be happy about each driver already doing the right thing.

Best regards
Uwe

Sean Anderson June 29, 2021, 10:21 p.m. UTC | #16

On 6/29/21 4:51 PM, Uwe Kleine-König wrote:
> Hello Sean,
>
> On Tue, Jun 29, 2021 at 02:01:31PM -0400, Sean Anderson wrote:
>> On 6/29/21 4:31 AM, Uwe Kleine-König wrote:
>> > Hello Sean,
>> >
>> > On Mon, Jun 28, 2021 at 01:41:43PM -0400, Sean Anderson wrote:
>> >> On 6/28/21 1:20 PM, Uwe Kleine-König wrote:
>> >> > On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
>> >> >> On 6/28/21 12:24 PM, Uwe Kleine-König wrote:
>> >> >> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
>> >> >> > > On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
>> >> >> > > > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
>> >> >> > > IMO, this is the best way to prevent surprising results in the API.
>> >> >> >
>> >> >> > I think it's not possible in practise to refuse "near" misses and every
>> >> >> > definition of "near" is in some case ridiculous. Also if you consider
>> >> >> > the pwm_round_state() case you don't want to refuse any request to tell
>> >> >> > as much as possible about your controller's capabilities. And then it's
>> >> >> > straight forward to let apply behave in the same way to keep complexity
>> >> >> > low.
>> >> >> >
>> >> >> > > The real issue here is that it is impossible to determine the correct
>> >> >> > > way to round the PWM a priori, and in particular, without considering
>> >> >> > > both duty_cycle and period. If a consumer requests very small
>> >> >> > > period/duty cycle which we cannot produce, how should it be rounded?
>> >> >> >
>> >> >> > Yeah, because there is no obviously right one, I picked one that is as
>> >> >> > wrong as the other possibilities but is easy to work with.
>> >> >> >
>> >> >> > > Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
>> >> >> > > the least period? Or should we try and increase the period to better
>> >> >> > > approximate the % duty cycle? And both of these decisions must be made
>> >> >> > > knowing both parameters. We cannot (for example) just always round up,
>> >> >> > > since we may produce a configuration with TLR0 == TLR1, which would
>> >> >> > > produce 0% duty cycle instead of whatever was requested. Rounding rate
>> >> >> > > will introduce significant complexity into the driver. Most of the time
>> >> >> > > if a consumer requests an invalid rate, it is due to misconfiguration
>> >> >> > > which is best solved by fixing the configuration.
>> >> >> >
>> >> >> > In the first step pick the biggest period not bigger than the requested
>> >> >> > and then pick the biggest duty cycle that is not bigger than the
>> >> >> > requested and that can be set with the just picked period. That is the
>> >> >> > behaviour that all new drivers should do. This is somewhat arbitrary but
>> >> >> > after quite some thought the most sensible in my eyes.
>> >> >>
>> >> >> And if there are no periods smaller than the requested period?
>> >> >
>> >> > Then return -ERANGE.
>> >>
>> >> Ok, so instead of
>> >>
>> >> 	if (cycles < 2 || cycles > priv->max + 2)
>> >> 		return -ERANGE;
>> >>
>> >> you would prefer
>> >>
>> >> 	if (cycles < 2)
>> >> 		return -ERANGE;
>> >> 	else if (cycles > priv->max + 2)
>> >> 		cycles = priv->max;
>> >
>> > The actual calculation is a bit harder to handle TCSR_UDT = 0 but in
>> > principle, yes, but see below.
>> >
>> >> But if we do the above clamping for TLR0, then we have to recalculate
>> >> the duty cycle for TLR1. Which I guess means doing something like
>> >>
>> >> 	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
>> >> 	if (ret)
>> >> 		return ret;
>> >>
>> >> 	state->duty_cycle = mult_frac(state->duty_cycle,
>> >> 				      xilinx_timer_get_period(priv, tlr0, tcsr0),
>> >> 				      state->period);
>> >>
>> >> 	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
>> >> 	if (ret)
>> >> 		return ret;
>> >
>> > No, you need something like:
>> >
>> > 	/*
>> > 	 * The multiplication cannot overflow as both priv_max and
>> > 	 * NSEC_PER_SEC fit into an u32.
>> > 	 */
>> > 	max_period = div64_ul((u64)priv->max * NSEC_PER_SEC, clkrate);
>> >
>> > 	/* cap period to the maximal possible value */
>> > 	if (state->period > max_period)
>> > 		period = max_period;
>> > 	else
>> > 		period = state->period;
>> >
>> > 	/* cap duty_cycle to the maximal possible value */
>> > 	if (state->duty_cycle > max_period)
>> > 		duty_cycle = max_period;
>> > 	else
>> > 		duty_cycle = state->duty_cycle;
>>
>> These caps may increase the % duty cycle.
>
> Correct.
>
> For some usecases keeping the relative duty cycle might be better, for
> others it might not. I'm still convinced that in general my solution
> makes sense, is computationally cheaper and easier to work with.

Can you please describe one of those use cases? Every PWM user I looked
(grepping for pwm_apply_state and pwm_config) set the duty cycle as a
percentage of the period, and not as an absolute time. Keeping the high
time the same while changing the duty cycle runs contrary to the
assumptions of all of those users.

>>
>> > 	period_cycles = period * clkrate / NSEC_PER_SEC;
>> >
>> > 	if (period_cycles < 2)
>> > 		return -ERANGE;
>> >
>> > 	duty_cycles = duty_cycle * clkrate / NSEC_PER_SEC;
>> >
>> > 	/*
>> > 	 * The hardware cannot emit a 100% relative duty cycle, if
>> > 	 * duty_cycle >= period_cycles is programmed the hardware emits
>> > 	 * a 0% relative duty cycle.
>> > 	 */
>> > 	if (duty_cycle == period_cycles)
>> > 		duty_cycles = period_cycles - 1;
>> >
>> > 	/*
>> > 	 * The hardware cannot emit a duty_cycle of one clk step, so
>> > 	 * emit 0 instead.
>> > 	 */
>> > 	if (duty_cycles < 2)
>> > 		duty_cycles = period_cycles;
>>
>> Of course, the above may result in 100% duty cycle being rounded down to
>> 0%. I feel like that is too big of a jump to ignore. Perhaps if we
>> cannot return -ERANGE we should at least dev_warn.
>
> You did it again. You picked one single case that you consider bad but
> didn't provide a constructive way to make it better.

Sure I did. I suggested that we warn. Something like

if (duty_cycles == period_cycles)
	if (--duty_cycles < 2)
		dev_warn(chip->dev, "Rounding 100%% duty cycle down to 0%%; pick a longer period\n");

or

if (period_cycles < 2)
	return -ERANGE;
else if (period_cycles < 10)
	dev_notice(chip->dev,
		   "very short period of %u cycles; duty cycle may be rounded to 0%%\n",
		   period_cycles);

Because 90% of the time, if a user requests such a short period it is
due to a typo or something similar. And if they really are doing it
intentionally, then they should just set duty_cycle=0.

> Assume there was already a pwm_round_state function (that returns the
> state that pwm_apply_state would implement for a given request) Consider
> a consumer that wants say a 50% relative duty together with a small
> period. So it first might call:
>
> 	ret = pwm_round_rate(pwm, { .period = 20, .duty_cycle = 20, ... }, &rounded_state)
>
> to find out if .period = 20 can be implemented with the given PWM. If
> this returns rounded state as:
>
> 	.period = 20
> 	.duty_cycle = 0
>
> this says quite a lot about the pwm if the driver implements my policy.
> (i.e.: The driver can do 20ns, but the biggest duty_cycle is only 0).
> If however it returns -ERANGE this means (assuming the driver implements
> the policy I try to convice you to be the right one) it means: The
> hardware cannot implement 20 ns (or something smaller) and so the next
> call probably tries 40 ns.
>
> With your suggested semantic -ERANGE might mean:
>
>   - The driver doesn't support .period = 20 ns
>     (Follow up questions: What period should be tried next? 10 ns? 40
>     ns? What if this returns -ERANGE again?)
>   - The driver supports .period = 20 ns, but the biggest possible
>     duty_cycle is "too different from 20 ns to ignore".
>
> Then how should the search continue?

round_rate does not have to use the same logic as apply_state. That is,

	ret = pwm_round_rate(pwm, { .period = 20, .duty_cycle = 20, ... }, &rounded_state)

could be rounded to

	{ .period = 20, .duty_cycle = 0 }

but calling

	ret = pwm_apply_state(pwm, { .period = 20, .duty_cycle = 20, ... })

could return -ERANGE. However, calling

	ret = pwm_apply_state(pwm, { .period = 20, .duty_cycle = 0, ... })

should work just fine, as the caller clearly knows what they are getting
into. IMO this is the best way to allow hypothetical round_rate users to
find out the edges of the PWM while still protecting existing users.

It's perfectly fine to round

	{ .period = 150, .duty_cycle = 75 }

to

	{ .period = 100, .duty_cycle = 75 }

in round_rate. But doing the same thing for apply_state would be very
surprising to every existing PWM user.

IMO the following invariant should hold

	apply_state(round_rate(x))
	assert(x == get_state())

but the following should not necessarily hold

	apply_state(x)
	assert(round_rate(x) == get_state())

Of course, where it is reasonable to round down, we should do so. But
where the result may be surprising, then the caller should specify the
rounded state specifically. It is better to fail loudly and noisily than
to silently accept garbage.

--Sean

>
> So: As soon as there is a pwm_round_rate function this can be catched
> and then it's important that the drivers adhere to the expected policy.
> Implementing this is a big thing and believe me I already spend quite
> some brain cycles into it. Once the core is extended accordingly I will
> be happy about each driver already doing the right thing.
>
> Best regards
> Uwe
>

Sean Anderson June 29, 2021, 10:26 p.m. UTC | #17

On 6/29/21 6:21 PM, Sean Anderson wrote:
> 
> 
> On 6/29/21 4:51 PM, Uwe Kleine-König wrote:
>> Hello Sean,
>>
>> On Tue, Jun 29, 2021 at 02:01:31PM -0400, Sean Anderson wrote:
>>> On 6/29/21 4:31 AM, Uwe Kleine-König wrote:
>>> > Hello Sean,
>>> >
>>> > On Mon, Jun 28, 2021 at 01:41:43PM -0400, Sean Anderson wrote:
>>> >> On 6/28/21 1:20 PM, Uwe Kleine-König wrote:
>>> >> > On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
>>> >> >> On 6/28/21 12:24 PM, Uwe Kleine-König wrote:
>>> >> >> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
>>> >> >> > > On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
>>> >> >> > > > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
>>> >> >> > > IMO, this is the best way to prevent surprising results in the API.
>>> >> >> >
>>> >> >> > I think it's not possible in practise to refuse "near" misses and every
>>> >> >> > definition of "near" is in some case ridiculous. Also if you consider
>>> >> >> > the pwm_round_state() case you don't want to refuse any request to tell
>>> >> >> > as much as possible about your controller's capabilities. And then it's
>>> >> >> > straight forward to let apply behave in the same way to keep complexity
>>> >> >> > low.
>>> >> >> >
>>> >> >> > > The real issue here is that it is impossible to determine the correct
>>> >> >> > > way to round the PWM a priori, and in particular, without considering
>>> >> >> > > both duty_cycle and period. If a consumer requests very small
>>> >> >> > > period/duty cycle which we cannot produce, how should it be rounded?
>>> >> >> >
>>> >> >> > Yeah, because there is no obviously right one, I picked one that is as
>>> >> >> > wrong as the other possibilities but is easy to work with.
>>> >> >> >
>>> >> >> > > Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
>>> >> >> > > the least period? Or should we try and increase the period to better
>>> >> >> > > approximate the % duty cycle? And both of these decisions must be made
>>> >> >> > > knowing both parameters. We cannot (for example) just always round up,
>>> >> >> > > since we may produce a configuration with TLR0 == TLR1, which would
>>> >> >> > > produce 0% duty cycle instead of whatever was requested. Rounding rate
>>> >> >> > > will introduce significant complexity into the driver. Most of the time
>>> >> >> > > if a consumer requests an invalid rate, it is due to misconfiguration
>>> >> >> > > which is best solved by fixing the configuration.
>>> >> >> >
>>> >> >> > In the first step pick the biggest period not bigger than the requested
>>> >> >> > and then pick the biggest duty cycle that is not bigger than the
>>> >> >> > requested and that can be set with the just picked period. That is the
>>> >> >> > behaviour that all new drivers should do. This is somewhat arbitrary but
>>> >> >> > after quite some thought the most sensible in my eyes.
>>> >> >>
>>> >> >> And if there are no periods smaller than the requested period?
>>> >> >
>>> >> > Then return -ERANGE.
>>> >>
>>> >> Ok, so instead of
>>> >>
>>> >>     if (cycles < 2 || cycles > priv->max + 2)
>>> >>         return -ERANGE;
>>> >>
>>> >> you would prefer
>>> >>
>>> >>     if (cycles < 2)
>>> >>         return -ERANGE;
>>> >>     else if (cycles > priv->max + 2)
>>> >>         cycles = priv->max;
>>> >
>>> > The actual calculation is a bit harder to handle TCSR_UDT = 0 but in
>>> > principle, yes, but see below.
>>> >
>>> >> But if we do the above clamping for TLR0, then we have to recalculate
>>> >> the duty cycle for TLR1. Which I guess means doing something like
>>> >>
>>> >>     ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
>>> >>     if (ret)
>>> >>         return ret;
>>> >>
>>> >>     state->duty_cycle = mult_frac(state->duty_cycle,
>>> >>                       xilinx_timer_get_period(priv, tlr0, tcsr0),
>>> >>                       state->period);
>>> >>
>>> >>     ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
>>> >>     if (ret)
>>> >>         return ret;
>>> >
>>> > No, you need something like:
>>> >
>>> >     /*
>>> >      * The multiplication cannot overflow as both priv_max and
>>> >      * NSEC_PER_SEC fit into an u32.
>>> >      */
>>> >     max_period = div64_ul((u64)priv->max * NSEC_PER_SEC, clkrate);
>>> >
>>> >     /* cap period to the maximal possible value */
>>> >     if (state->period > max_period)
>>> >         period = max_period;
>>> >     else
>>> >         period = state->period;
>>> >
>>> >     /* cap duty_cycle to the maximal possible value */
>>> >     if (state->duty_cycle > max_period)
>>> >         duty_cycle = max_period;
>>> >     else
>>> >         duty_cycle = state->duty_cycle;
>>>
>>> These caps may increase the % duty cycle.
>>
>> Correct.
>>
>> For some usecases keeping the relative duty cycle might be better, for
>> others it might not. I'm still convinced that in general my solution
>> makes sense, is computationally cheaper and easier to work with.
> 
> Can you please describe one of those use cases? Every PWM user I looked
> (grepping for pwm_apply_state and pwm_config) set the duty cycle as a
> percentage of the period, and not as an absolute time. Keeping the high
> time the same while changing the duty cycle runs contrary to the
> assumptions of all of those users.
> 
>>>
>>> >     period_cycles = period * clkrate / NSEC_PER_SEC;
>>> >
>>> >     if (period_cycles < 2)
>>> >         return -ERANGE;
>>> >
>>> >     duty_cycles = duty_cycle * clkrate / NSEC_PER_SEC;
>>> >
>>> >     /*
>>> >      * The hardware cannot emit a 100% relative duty cycle, if
>>> >      * duty_cycle >= period_cycles is programmed the hardware emits
>>> >      * a 0% relative duty cycle.
>>> >      */
>>> >     if (duty_cycle == period_cycles)
>>> >         duty_cycles = period_cycles - 1;
>>> >
>>> >     /*
>>> >      * The hardware cannot emit a duty_cycle of one clk step, so
>>> >      * emit 0 instead.
>>> >      */
>>> >     if (duty_cycles < 2)
>>> >         duty_cycles = period_cycles;
>>>
>>> Of course, the above may result in 100% duty cycle being rounded down to
>>> 0%. I feel like that is too big of a jump to ignore. Perhaps if we
>>> cannot return -ERANGE we should at least dev_warn.
>>
>> You did it again. You picked one single case that you consider bad but
>> didn't provide a constructive way to make it better.
> 
> Sure I did. I suggested that we warn. Something like
> 
> if (duty_cycles == period_cycles)
>      if (--duty_cycles < 2)
>          dev_warn(chip->dev, "Rounding 100%% duty cycle down to 0%%; pick a longer period\n");
> 
> or
> 
> if (period_cycles < 2)
>      return -ERANGE;
> else if (period_cycles < 10)
>      dev_notice(chip->dev,
>             "very short period of %u cycles; duty cycle may be rounded to 0%%\n",
>             period_cycles);
> 
> Because 90% of the time, if a user requests such a short period it is
> due to a typo or something similar. And if they really are doing it
> intentionally, then they should just set duty_cycle=0.
> 
>> Assume there was already a pwm_round_state function (that returns the
>> state that pwm_apply_state would implement for a given request) Consider
>> a consumer that wants say a 50% relative duty together with a small
>> period. So it first might call:
>>
>>     ret = pwm_round_rate(pwm, { .period = 20, .duty_cycle = 20, ... }, &rounded_state)
>>
>> to find out if .period = 20 can be implemented with the given PWM. If
>> this returns rounded state as:
>>
>>     .period = 20
>>     .duty_cycle = 0
>>
>> this says quite a lot about the pwm if the driver implements my policy.
>> (i.e.: The driver can do 20ns, but the biggest duty_cycle is only 0).
>> If however it returns -ERANGE this means (assuming the driver implements
>> the policy I try to convice you to be the right one) it means: The
>> hardware cannot implement 20 ns (or something smaller) and so the next
>> call probably tries 40 ns.
>>
>> With your suggested semantic -ERANGE might mean:
>>
>>   - The driver doesn't support .period = 20 ns
>>     (Follow up questions: What period should be tried next? 10 ns? 40
>>     ns? What if this returns -ERANGE again?)
>>   - The driver supports .period = 20 ns, but the biggest possible
>>     duty_cycle is "too different from 20 ns to ignore".
>>
>> Then how should the search continue?
> 
> round_rate does not have to use the same logic as apply_state. That is,
> 
>      ret = pwm_round_rate(pwm, { .period = 20, .duty_cycle = 20, ... }, &rounded_state)
> 
> could be rounded to
> 
>      { .period = 20, .duty_cycle = 0 }
> 
> but calling
> 
>      ret = pwm_apply_state(pwm, { .period = 20, .duty_cycle = 20, ... })
> 
> could return -ERANGE. However, calling
> 
>      ret = pwm_apply_state(pwm, { .period = 20, .duty_cycle = 0, ... })
> 
> should work just fine, as the caller clearly knows what they are getting
> into. IMO this is the best way to allow hypothetical round_rate users to
> find out the edges of the PWM while still protecting existing users.
> 
> It's perfectly fine to round
> 
>      { .period = 150, .duty_cycle = 75 }
> 
> to
> 
>      { .period = 100, .duty_cycle = 75 }
> 
> in round_rate. But doing the same thing for apply_state would be very
> surprising to every existing PWM user.
> 
> IMO the following invariant should hold
> 
>      apply_state(round_rate(x))
>      assert(x == get_state())

this should be

       apply_state(round_rate(x))
       assert(round_rate(x) == get_state())

> 
> but the following should not necessarily hold
> 
>      apply_state(x)
>      assert(round_rate(x) == get_state())
> 
> Of course, where it is reasonable to round down, we should do so. But
> where the result may be surprising, then the caller should specify the
> rounded state specifically. It is better to fail loudly and noisily than
> to silently accept garbage.
> 
> --Sean
> 
>>
>> So: As soon as there is a pwm_round_rate function this can be catched
>> and then it's important that the drivers adhere to the expected policy.
>> Implementing this is a big thing and believe me I already spend quite
>> some brain cycles into it. Once the core is extended accordingly I will
>> be happy about each driver already doing the right thing.
>>
>> Best regards
>> Uwe
>>

Uwe Kleine-König June 30, 2021, 8:35 a.m. UTC | #18

Hello Sean,

I often mistype the name of the rounding function as "pwm_round_rate",
the better name is "pwm_round_state" of course. That's just me thinking
about clk_round_rate where ".._rate" is the right term. I'll try harder
to get this right from now on.

On Tue, Jun 29, 2021 at 06:21:15PM -0400, Sean Anderson wrote:
> On 6/29/21 4:51 PM, Uwe Kleine-König wrote:
> > On Tue, Jun 29, 2021 at 02:01:31PM -0400, Sean Anderson wrote:
> > > On 6/29/21 4:31 AM, Uwe Kleine-König wrote:
> > > > On Mon, Jun 28, 2021 at 01:41:43PM -0400, Sean Anderson wrote:
> > > >> On 6/28/21 1:20 PM, Uwe Kleine-König wrote:
> > > >> > On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
> > > >> >> On 6/28/21 12:24 PM, Uwe Kleine-König wrote:
> > > >> >> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
> > > >> >> > > On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
> > > >> >> > > > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
> > > >> >> > > IMO, this is the best way to prevent surprising results in the API.
> > > >> >> >
> > > >> >> > I think it's not possible in practise to refuse "near" misses and every
> > > >> >> > definition of "near" is in some case ridiculous. Also if you consider
> > > >> >> > the pwm_round_state() case you don't want to refuse any request to tell
> > > >> >> > as much as possible about your controller's capabilities. And then it's
> > > >> >> > straight forward to let apply behave in the same way to keep complexity
> > > >> >> > low.
> > > >> >> >
> > > >> >> > > The real issue here is that it is impossible to determine the correct
> > > >> >> > > way to round the PWM a priori, and in particular, without considering
> > > >> >> > > both duty_cycle and period. If a consumer requests very small
> > > >> >> > > period/duty cycle which we cannot produce, how should it be rounded?
> > > >> >> >
> > > >> >> > Yeah, because there is no obviously right one, I picked one that is as
> > > >> >> > wrong as the other possibilities but is easy to work with.
> > > >> >> >
> > > >> >> > > Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
> > > >> >> > > the least period? Or should we try and increase the period to better
> > > >> >> > > approximate the % duty cycle? And both of these decisions must be made
> > > >> >> > > knowing both parameters. We cannot (for example) just always round up,
> > > >> >> > > since we may produce a configuration with TLR0 == TLR1, which would
> > > >> >> > > produce 0% duty cycle instead of whatever was requested. Rounding rate
> > > >> >> > > will introduce significant complexity into the driver. Most of the time
> > > >> >> > > if a consumer requests an invalid rate, it is due to misconfiguration
> > > >> >> > > which is best solved by fixing the configuration.
> > > >> >> >
> > > >> >> > In the first step pick the biggest period not bigger than the requested
> > > >> >> > and then pick the biggest duty cycle that is not bigger than the
> > > >> >> > requested and that can be set with the just picked period. That is the
> > > >> >> > behaviour that all new drivers should do. This is somewhat arbitrary but
> > > >> >> > after quite some thought the most sensible in my eyes.
> > > >> >>
> > > >> >> And if there are no periods smaller than the requested period?
> > > >> >
> > > >> > Then return -ERANGE.
> > > >>
> > > >> Ok, so instead of
> > > >>
> > > >> 	if (cycles < 2 || cycles > priv->max + 2)
> > > >> 		return -ERANGE;
> > > >>
> > > >> you would prefer
> > > >>
> > > >> 	if (cycles < 2)
> > > >> 		return -ERANGE;
> > > >> 	else if (cycles > priv->max + 2)
> > > >> 		cycles = priv->max;
> > > >
> > > > The actual calculation is a bit harder to handle TCSR_UDT = 0 but in
> > > > principle, yes, but see below.
> > > >
> > > >> But if we do the above clamping for TLR0, then we have to recalculate
> > > >> the duty cycle for TLR1. Which I guess means doing something like
> > > >>
> > > >> 	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
> > > >> 	if (ret)
> > > >> 		return ret;
> > > >>
> > > >> 	state->duty_cycle = mult_frac(state->duty_cycle,
> > > >> 				      xilinx_timer_get_period(priv, tlr0, tcsr0),
> > > >> 				      state->period);
> > > >>
> > > >> 	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
> > > >> 	if (ret)
> > > >> 		return ret;
> > > >
> > > > No, you need something like:
> > > >
> > > > 	/*
> > > > 	 * The multiplication cannot overflow as both priv_max and
> > > > 	 * NSEC_PER_SEC fit into an u32.
> > > > 	 */
> > > > 	max_period = div64_ul((u64)priv->max * NSEC_PER_SEC, clkrate);
> > > >
> > > > 	/* cap period to the maximal possible value */
> > > > 	if (state->period > max_period)
> > > > 		period = max_period;
> > > > 	else
> > > > 		period = state->period;
> > > >
> > > > 	/* cap duty_cycle to the maximal possible value */
> > > > 	if (state->duty_cycle > max_period)
> > > > 		duty_cycle = max_period;
> > > > 	else
> > > > 		duty_cycle = state->duty_cycle;
> > > 
> > > These caps may increase the % duty cycle.
> > 
> > Correct.
> > 
> > For some usecases keeping the relative duty cycle might be better, for
> > others it might not. I'm still convinced that in general my solution
> > makes sense, is computationally cheaper and easier to work with.
> 
> Can you please describe one of those use cases? Every PWM user I looked
> (grepping for pwm_apply_state and pwm_config) set the duty cycle as a
> percentage of the period, and not as an absolute time. Keeping the high
> time the same while changing the duty cycle runs contrary to the
> assumptions of all of those users.

Indeed there is no mainline driver that relies on this. There are some
smart LED controllers (e.g. WS2812B) where the duty_cycle is more
important than the period. (I admit a PWM is not really the right driver
for that one as it could only completely enable and complete disable
white color.) Also there are some servo motor chips where the absolute
duty is relevant but the period isn't (in some range). (See
https://www.mikrocontroller.net/articles/Modellbauservo_Ansteuerung#Signalaufbau
for a article about that (in German though).)

In case you want to argue that out-of-mainline users don't count: I
think in the design of an API they do count to place the bar to enter
the mainline low. Frameworks should be generic enough to cover as much
use cases as possible.

And note that if you want a nearest to (say) 50% relative duty cycle and
don't care much about the period it doesn't really matter if you scale
duty_cycle in pwm_round_state() to the period change or not because in
general you need several calls to pwm_round_state() anyhow to find a
setting with 51% if the next lower possibility is 47%. So in the end you
save (I think) one call in generic PWM code.

In contrast the math gets quite a bit more complicated because there is
rounding involved in scaling the duty cycle. Consider a PWM that can
configure period and duty in 16.4 ns steps and you ask for

	.period = 100 ns
	.duty_cycle = 50 ns

Then the best period you can provide is 98.4 ns, so you return .period =
99 from pwm_round_state(). (Yes, you don't return 98, because
round-nearest is much harder to handle than round down.) To determine
the adapted duty_cycle you have to do

	50 * realperiod / 100

which independently of choosing 98, 98.4 or 99 for realperiod is 49. Then
to approximate 49 without rounding up you end up with 32.8 while 49.2
would have be perfectly fine.

You might find a way around that (maybe you have to round up in the
adaption of duty_cycle, I didn't convince myself this is good enough
though).

So your suggestion to adapt the duty_cycle to keep the relative
duty_cycle constant (as good as possible within the bounds the hardware
dictates) implies additional complication at the driver level.

From a framework maintainer's point of view (and also from a low-level
driver maintainer's point of view) I prefer one complication in a
generic function over a complication that I have to care for in each and
every low-level driver by a big margin.

So unless you volunteer to complete the math above and promise to review
low-level drivers for that aspect in the future (or alternatively
convince me that math is easy and I missed something) I would like to
end this discussion here and stay with the policy I explained.

> > > > 	period_cycles = period * clkrate / NSEC_PER_SEC;
> > > >
> > > > 	if (period_cycles < 2)
> > > > 		return -ERANGE;
> > > >
> > > > 	duty_cycles = duty_cycle * clkrate / NSEC_PER_SEC;
> > > >
> > > > 	/*
> > > > 	 * The hardware cannot emit a 100% relative duty cycle, if
> > > > 	 * duty_cycle >= period_cycles is programmed the hardware emits
> > > > 	 * a 0% relative duty cycle.
> > > > 	 */
> > > > 	if (duty_cycle == period_cycles)
> > > > 		duty_cycles = period_cycles - 1;
> > > >
> > > > 	/*
> > > > 	 * The hardware cannot emit a duty_cycle of one clk step, so
> > > > 	 * emit 0 instead.
> > > > 	 */
> > > > 	if (duty_cycles < 2)
> > > > 		duty_cycles = period_cycles;
> > > 
> > > Of course, the above may result in 100% duty cycle being rounded down to
> > > 0%. I feel like that is too big of a jump to ignore. Perhaps if we
> > > cannot return -ERANGE we should at least dev_warn.
> > 
> > You did it again. You picked one single case that you consider bad but
> > didn't provide a constructive way to make it better.
> 
> Sure I did. I suggested that we warn. Something like
> 
> if (duty_cycles == period_cycles)
> 	if (--duty_cycles < 2)
> 		dev_warn(chip->dev, "Rounding 100%% duty cycle down to 0%%; pick a longer period\n");
> 
> or
> 
> if (period_cycles < 2)
> 	return -ERANGE;
> else if (period_cycles < 10)
> 	dev_notice(chip->dev,
> 		   "very short period of %u cycles; duty cycle may be rounded to 0%%\n",
> 		   period_cycles);

Ah ok, so only a 100% jump warrants that warning. I think adding that
has no practical relevance, so I don't oppose to that. Add it if you
want. (But note that if it triggers indeed it might flood the kernel log
if your consumer wants to start a motor but notices it doesn't run fast
enough and so configures 100% in a tight loop. So I would recommend some
rate limiting.)

> Because 90% of the time, if a user requests such a short period it is
> due to a typo or something similar. And if they really are doing it
> intentionally, then they should just set duty_cycle=0.

Uh, don't you think that a warning that is wrong in 10% of the cases is
bad?

> > Assume there was already a pwm_round_state function (that returns the
> > state that pwm_apply_state would implement for a given request) Consider
> > a consumer that wants say a 50% relative duty together with a small
> > period. So it first might call:
> > 
> > 	ret = pwm_round_rate(pwm, { .period = 20, .duty_cycle = 20, ... }, &rounded_state)
> > 
> > to find out if .period = 20 can be implemented with the given PWM. If
> > this returns rounded state as:
> > 
> > 	.period = 20
> > 	.duty_cycle = 0
> > 
> > this says quite a lot about the pwm if the driver implements my policy.
> > (i.e.: The driver can do 20ns, but the biggest duty_cycle is only 0).
> > If however it returns -ERANGE this means (assuming the driver implements
> > the policy I try to convice you to be the right one) it means: The
> > hardware cannot implement 20 ns (or something smaller) and so the next
> > call probably tries 40 ns.
> > 
> > With your suggested semantic -ERANGE might mean:
> > 
> >   - The driver doesn't support .period = 20 ns
> >     (Follow up questions: What period should be tried next? 10 ns? 40
> >     ns? What if this returns -ERANGE again?)
> >   - The driver supports .period = 20 ns, but the biggest possible
> >     duty_cycle is "too different from 20 ns to ignore".
> > 
> > Then how should the search continue?
> 
> round_rate does not have to use the same logic as apply_state.

I want to have .round_state() and .apply() (i.e. the driver callbacks)
to behave identically. If we indeed come to the conclusion that
pwm_apply_state needs to have some precautions, I'd like to have them
implemented in pwm_apply_state() only and not in every driver.

> However, calling
> 
> 	ret = pwm_apply_state(pwm, { .period = 20, .duty_cycle = 0, ... })
> 
> should work just fine, as the caller clearly knows what they are getting
> into. IMO this is the best way to allow hypothetical round_rate users to
> find out the edges of the PWM while still protecting existing users.
> 
> It's perfectly fine to round
> 
> 	{ .period = 150, .duty_cycle = 75 }
> 
> to
> 
> 	{ .period = 100, .duty_cycle = 75 }
> 
> in round_rate. But doing the same thing for apply_state would be very
> surprising to every existing PWM user.
> 
> IMO the following invariant should hold
> 
> 	apply_state(round_rate(x))
> 	assert(round_rate(x) == get_state())

(merged your correction of the follow up mail into the quote above)

(Fun fact: Only needing this one would allow a generic implementation of
round_state, it just had to return a pwm_state that doesn't depend on x
:o)

> but the following should not necessarily hold
> 
> 	apply_state(x)
> 	assert(round_rate(x) == get_state())
> 
> Of course, where it is reasonable to round down, we should do so.
> 
> But where the result may be surprising, then the caller should specify
> the rounded state specifically. It is better to fail loudly and
> noisily than
> to silently accept garbage.

Can you please come up with an algorithm to judge if a given deviation
is reasonable or surprising? I agree there are surprises and some of
them are obviously bad. For most cases however the judgement depends on
the use case so I fail to see how someone should program such a check
that should cover all consumers and use cases. I prefer no precautions +
an easy relation between pwm_round_state and pwm_apply_state (i.e.
behave identically) over a most of the time(?) useless precaution and
some policy defined differences between pwm_round_state and
pwm_apply_state

Best regards
Uwe

Sean Anderson July 8, 2021, 4:59 p.m. UTC | #19

On 6/30/21 4:35 AM, Uwe Kleine-König wrote:
> Hello Sean,
>
> I often mistype the name of the rounding function as "pwm_round_rate",
> the better name is "pwm_round_state" of course. That's just me thinking
> about clk_round_rate where ".._rate" is the right term. I'll try harder
> to get this right from now on.
>
> On Tue, Jun 29, 2021 at 06:21:15PM -0400, Sean Anderson wrote:
>> On 6/29/21 4:51 PM, Uwe Kleine-König wrote:
>> > On Tue, Jun 29, 2021 at 02:01:31PM -0400, Sean Anderson wrote:
>> > > On 6/29/21 4:31 AM, Uwe Kleine-König wrote:
>> > > > On Mon, Jun 28, 2021 at 01:41:43PM -0400, Sean Anderson wrote:
>> > > >> On 6/28/21 1:20 PM, Uwe Kleine-König wrote:
>> > > >> > On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
>> > > >> >> On 6/28/21 12:24 PM, Uwe Kleine-König wrote:
>> > > >> >> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
>> > > >> >> > > On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
>> > > >> >> > > > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
>> > > >> >> > > IMO, this is the best way to prevent surprising results in the API.
>> > > >> >> >
>> > > >> >> > I think it's not possible in practise to refuse "near" misses and every
>> > > >> >> > definition of "near" is in some case ridiculous. Also if you consider
>> > > >> >> > the pwm_round_state() case you don't want to refuse any request to tell
>> > > >> >> > as much as possible about your controller's capabilities. And then it's
>> > > >> >> > straight forward to let apply behave in the same way to keep complexity
>> > > >> >> > low.
>> > > >> >> >
>> > > >> >> > > The real issue here is that it is impossible to determine the correct
>> > > >> >> > > way to round the PWM a priori, and in particular, without considering
>> > > >> >> > > both duty_cycle and period. If a consumer requests very small
>> > > >> >> > > period/duty cycle which we cannot produce, how should it be rounded?
>> > > >> >> >
>> > > >> >> > Yeah, because there is no obviously right one, I picked one that is as
>> > > >> >> > wrong as the other possibilities but is easy to work with.
>> > > >> >> >
>> > > >> >> > > Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
>> > > >> >> > > the least period? Or should we try and increase the period to better
>> > > >> >> > > approximate the % duty cycle? And both of these decisions must be made
>> > > >> >> > > knowing both parameters. We cannot (for example) just always round up,
>> > > >> >> > > since we may produce a configuration with TLR0 == TLR1, which would
>> > > >> >> > > produce 0% duty cycle instead of whatever was requested. Rounding rate
>> > > >> >> > > will introduce significant complexity into the driver. Most of the time
>> > > >> >> > > if a consumer requests an invalid rate, it is due to misconfiguration
>> > > >> >> > > which is best solved by fixing the configuration.
>> > > >> >> >
>> > > >> >> > In the first step pick the biggest period not bigger than the requested
>> > > >> >> > and then pick the biggest duty cycle that is not bigger than the
>> > > >> >> > requested and that can be set with the just picked period. That is the
>> > > >> >> > behaviour that all new drivers should do. This is somewhat arbitrary but
>> > > >> >> > after quite some thought the most sensible in my eyes.
>> > > >> >>
>> > > >> >> And if there are no periods smaller than the requested period?
>> > > >> >
>> > > >> > Then return -ERANGE.
>> > > >>
>> > > >> Ok, so instead of
>> > > >>
>> > > >> 	if (cycles < 2 || cycles > priv->max + 2)
>> > > >> 		return -ERANGE;
>> > > >>
>> > > >> you would prefer
>> > > >>
>> > > >> 	if (cycles < 2)
>> > > >> 		return -ERANGE;
>> > > >> 	else if (cycles > priv->max + 2)
>> > > >> 		cycles = priv->max;
>> > > >
>> > > > The actual calculation is a bit harder to handle TCSR_UDT = 0 but in
>> > > > principle, yes, but see below.
>> > > >
>> > > >> But if we do the above clamping for TLR0, then we have to recalculate
>> > > >> the duty cycle for TLR1. Which I guess means doing something like
>> > > >>
>> > > >> 	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
>> > > >> 	if (ret)
>> > > >> 		return ret;
>> > > >>
>> > > >> 	state->duty_cycle = mult_frac(state->duty_cycle,
>> > > >> 				      xilinx_timer_get_period(priv, tlr0, tcsr0),
>> > > >> 				      state->period);
>> > > >>
>> > > >> 	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
>> > > >> 	if (ret)
>> > > >> 		return ret;
>> > > >
>> > > > No, you need something like:
>> > > >
>> > > > 	/*
>> > > > 	 * The multiplication cannot overflow as both priv_max and
>> > > > 	 * NSEC_PER_SEC fit into an u32.
>> > > > 	 */
>> > > > 	max_period = div64_ul((u64)priv->max * NSEC_PER_SEC, clkrate);
>> > > >
>> > > > 	/* cap period to the maximal possible value */
>> > > > 	if (state->period > max_period)
>> > > > 		period = max_period;
>> > > > 	else
>> > > > 		period = state->period;
>> > > >
>> > > > 	/* cap duty_cycle to the maximal possible value */
>> > > > 	if (state->duty_cycle > max_period)
>> > > > 		duty_cycle = max_period;
>> > > > 	else
>> > > > 		duty_cycle = state->duty_cycle;
>> > >
>> > > These caps may increase the % duty cycle.
>> >
>> > Correct.
>> >
>> > For some usecases keeping the relative duty cycle might be better, for
>> > others it might not. I'm still convinced that in general my solution
>> > makes sense, is computationally cheaper and easier to work with.
>>
>> Can you please describe one of those use cases? Every PWM user I looked
>> (grepping for pwm_apply_state and pwm_config) set the duty cycle as a
>> percentage of the period, and not as an absolute time. Keeping the high
>> time the same while changing the duty cycle runs contrary to the
>> assumptions of all of those users.
>
> Indeed there is no mainline driver that relies on this. There are some
> smart LED controllers (e.g. WS2812B) where the duty_cycle is more
> important than the period. (I admit a PWM is not really the right driver
> for that one as it could only completely enable and complete disable
> white color.) Also there are some servo motor chips where the absolute
> duty is relevant but the period isn't (in some range). (See
> https://www.mikrocontroller.net/articles/Modellbauservo_Ansteuerung#Signalaufbau
> for a article about that (in German though).)

> In case you want to argue that out-of-mainline users don't count:

They don't :)

This is a kernel-internal API.

But my point is that the vast majority of PWM users, and 100% of the
in-kernel users care about the % duty cycle and not about the absolute
high time. We should design around the common use case first and
foremost.

> I think in the design of an API they do count to place the bar to
> enter the mainline low. Frameworks should be generic enough to cover
> as much use cases as possible.

They can cover many use-cases as possible, but they should be ergonomic
firstly for the most common use case.

> And note that if you want a nearest to (say) 50% relative duty cycle and
> don't care much about the period it doesn't really matter if you scale
> duty_cycle in pwm_round_state() to the period change or not because in
> general you need several calls to pwm_round_state() anyhow to find a
> setting with 51% if the next lower possibility is 47%. So in the end you
> save (I think) one call in generic PWM code.
>
> In contrast the math gets quite a bit more complicated because there is
> rounding involved in scaling the duty cycle. Consider a PWM that can
> configure period and duty in 16.4 ns steps and you ask for
>
> 	.period = 100 ns
> 	.duty_cycle = 50 ns
>
> Then the best period you can provide is 98.4 ns, so you return .period =
> 99 from pwm_round_state(). (Yes, you don't return 98, because
> round-nearest is much harder to handle than round down.)
> To determine the adapted duty_cycle you have to do
>
> 	50 * realperiod / 100
>
> which independently of choosing 98, 98.4 or 99 for realperiod is 49. Then
> to approximate 49 without rounding up you end up with 32.8 while 49.2
> would have be perfectly fine.

And what if the consumer comes and requests 49 for their period in the
first place? You have the same problem. The rescaling made it worse in
this instance, but this is just an unfortunate case study.

> You might find a way around that (maybe you have to round up in the
> adaption of duty_cycle, I didn't convince myself this is good enough
> though).
>
> So your suggestion to adapt the duty_cycle to keep the relative
> duty_cycle constant (as good as possible within the bounds the hardware
> dictates) implies additional complication at the driver level.
>
>  From a framework maintainer's point of view (and also from a low-level
> driver maintainer's point of view) I prefer one complication in a
> generic function over a complication that I have to care for in each and
> every low-level driver by a big margin.

FWIW what you're suggesting is also complex for the low-level driver.

>
> So unless you volunteer to complete the math above and promise to review
> low-level drivers for that aspect in the future (or alternatively
> convince me that math is easy and I missed something) I would like to
> end this discussion here and stay with the policy I explained.

see below

>> > > > 	period_cycles = period * clkrate / NSEC_PER_SEC;
>> > > >
>> > > > 	if (period_cycles < 2)
>> > > > 		return -ERANGE;
>> > > >
>> > > > 	duty_cycles = duty_cycle * clkrate / NSEC_PER_SEC;
>> > > >
>> > > > 	/*
>> > > > 	 * The hardware cannot emit a 100% relative duty cycle, if
>> > > > 	 * duty_cycle >= period_cycles is programmed the hardware emits
>> > > > 	 * a 0% relative duty cycle.
>> > > > 	 */
>> > > > 	if (duty_cycle == period_cycles)
>> > > > 		duty_cycles = period_cycles - 1;
>> > > >
>> > > > 	/*
>> > > > 	 * The hardware cannot emit a duty_cycle of one clk step, so
>> > > > 	 * emit 0 instead.
>> > > > 	 */
>> > > > 	if (duty_cycles < 2)
>> > > > 		duty_cycles = period_cycles;
>> > >
>> > > Of course, the above may result in 100% duty cycle being rounded down to
>> > > 0%. I feel like that is too big of a jump to ignore. Perhaps if we
>> > > cannot return -ERANGE we should at least dev_warn.
>> >
>> > You did it again. You picked one single case that you consider bad but
>> > didn't provide a constructive way to make it better.
>>
>> Sure I did. I suggested that we warn. Something like
>>
>> if (duty_cycles == period_cycles)
>> 	if (--duty_cycles < 2)
>> 		dev_warn(chip->dev, "Rounding 100%% duty cycle down to 0%%; pick a longer period\n");
>>
>> or
>>
>> if (period_cycles < 2)
>> 	return -ERANGE;
>> else if (period_cycles < 10)
>> 	dev_notice(chip->dev,
>> 		   "very short period of %u cycles; duty cycle may be rounded to 0%%\n",
>> 		   period_cycles);
>
> Ah ok, so only a 100% jump warrants that warning.
> I think adding that
> has no practical relevance, so I don't oppose to that. Add it if you
> want. (But note that if it triggers indeed it might flood the kernel log
> if your consumer wants to start a motor but notices it doesn't run fast
> enough and so configures 100% in a tight loop. So I would recommend some
> rate limiting.)
>
>> Because 90% of the time, if a user requests such a short period it is
>> due to a typo or something similar. And if they really are doing it
>> intentionally, then they should just set duty_cycle=0.
>
> Uh, don't you think that a warning that is wrong in 10% of the cases is
> bad?

Yes. Which is why I would prefer to use the first warning.

>> > Assume there was already a pwm_round_state function (that returns the
>> > state that pwm_apply_state would implement for a given request) Consider
>> > a consumer that wants say a 50% relative duty together with a small
>> > period. So it first might call:
>> >
>> > 	ret = pwm_round_rate(pwm, { .period = 20, .duty_cycle = 20, ... }, &rounded_state)
>> >
>> > to find out if .period = 20 can be implemented with the given PWM. If
>> > this returns rounded state as:
>> >
>> > 	.period = 20
>> > 	.duty_cycle = 0
>> >
>> > this says quite a lot about the pwm if the driver implements my policy.
>> > (i.e.: The driver can do 20ns, but the biggest duty_cycle is only 0).
>> > If however it returns -ERANGE this means (assuming the driver implements
>> > the policy I try to convice you to be the right one) it means: The
>> > hardware cannot implement 20 ns (or something smaller) and so the next
>> > call probably tries 40 ns.
>> >
>> > With your suggested semantic -ERANGE might mean:
>> >
>> >   - The driver doesn't support .period = 20 ns
>> >     (Follow up questions: What period should be tried next? 10 ns? 40
>> >     ns? What if this returns -ERANGE again?)
>> >   - The driver supports .period = 20 ns, but the biggest possible
>> >     duty_cycle is "too different from 20 ns to ignore".
>> >
>> > Then how should the search continue?
>>
>> round_rate does not have to use the same logic as apply_state.
>
> I want to have .round_state() and .apply() (i.e. the driver callbacks)
> to behave identically. If we indeed come to the conclusion that
> pwm_apply_state needs to have some precautions, I'd like to have them
> implemented in pwm_apply_state() only and not in every driver.
>
>> However, calling
>>
>> 	ret = pwm_apply_state(pwm, { .period = 20, .duty_cycle = 0, ... })
>>
>> should work just fine, as the caller clearly knows what they are getting
>> into. IMO this is the best way to allow hypothetical round_rate users to
>> find out the edges of the PWM while still protecting existing users.
>>
>> It's perfectly fine to round
>>
>> 	{ .period = 150, .duty_cycle = 75 }
>>
>> to
>>
>> 	{ .period = 100, .duty_cycle = 75 }
>>
>> in round_rate. But doing the same thing for apply_state would be very
>> surprising to every existing PWM user.
>>
>> IMO the following invariant should hold
>>
>> 	apply_state(round_rate(x))
>> 	assert(round_rate(x) == get_state())
>
> (merged your correction of the follow up mail into the quote above)
>
> (Fun fact: Only needing this one would allow a generic implementation of
> round_state, it just had to return a pwm_state that doesn't depend on x
> :o)

Maybe. The only constraint is that the state returned by round_rate is
possible to implement on the hardware without modification. See below
for further discussion.

>> but the following should not necessarily hold
>>
>> 	apply_state(x)
>> 	assert(round_rate(x) == get_state())
>>
>> Of course, where it is reasonable to round down, we should do so.
>>
>> But where the result may be surprising, then the caller should specify
>> the rounded state specifically. It is better to fail loudly and
>> noisily than
>> to silently accept garbage.
>
> Can you please come up with an algorithm to judge if a given deviation
> is reasonable or surprising? I agree there are surprises and some of
> them are obviously bad. For most cases however the judgement depends on
> the use case so I fail to see how someone should program such a check
> that should cover all consumers and use cases. I prefer no precautions +
> an easy relation between pwm_round_state and pwm_apply_state (i.e.
> behave identically) over a most of the time(?) useless precaution and
> some policy defined differences between pwm_round_state and
> pwm_apply_state

After thinking it over, I believe I agree with you on most things, but I
think your proposed API has room for additional checks without any loss
of generality.

The PWM subsystem has several major players:

* Existing users of the PWM API. Most of these do not especially care
   about the PWM period, usually just leaving at the default. The
   exception is of course the pwm-clk driver. Many of these users care
   about % duty cycle, and they all calculate the high time based on the
   configured period of the PWM. I suspect that while many of these users
   have substantial leeway in what accuracy they expect from the % duty
   cycle, significant errors (in the 25-50% range) are probably unusual
   and indicative of a misconfigured period. Unfortunately, we cannot
   make a general judgement about what sort of accuracy is OK in most
   cases.

* Hypothetical future users of some kind of round_state function. These
   users have some kind of algorithm which determines whether a PWM state
   is acceptable for the driver. Most of the time this will be some kind
   of accuracy check. What the round_state function returns is not
   particularly important, because users have the opportunity to revise
   their request based on what the state is rounded to. However, it is
   important that each round rate function is consistent in manner that
   it rounds so that these users

* Existing drivers for the PWM subsystem. These drivers must implement
   an apply_state function which is correct for both existing and future
   users. In addition, they may implement some kind of round_state
   function in the future. it is important to reduce the complexity of
   the calculations these drivers perform so that it is easier to
   implement and review them.

I believe the following requirements satisfy the above constraints:

* The round_state function shall round the period to the largest period
   representable by the PWM less than the requested period. It shall also
   round the duty cycle to the largest duty cycle representable by the
   PWM less than the requested duty cycle. No attempt shall be made to
   preserve the % duty cycle.
* The apply_state function shall only round the requested period down, and
   may do so by no more than one unit cycle. If the requested period is
   unrepresentable by the PWM, the apply_state function shall return
   -ERANGE.
* The apply_state function shall only round the requested duty cycle
   down. The apply_state function shall not return an error unless there
   is no duty cycle less than the requested duty cycle which is
   representable by the PWM.
* After applying a state returned by round_state with apply_state,
   get_state must return that state.

The reason that we must return an error when the period is
unrepresentable is that generally the duty cycle is calculated based on
the period. This change has no affect on future users of round_state,
since that function will only return valid periods. Those users will
have the opportunity to detect that the period has changed and determine
if the duty cycle is still acceptable. However, for existing users, we
should also provide the same opportunity. This requirement simplifies
the behavior of apply_state, since there is no longer any chance that
the % duty cycle is rounded up. This requirement is easy to implement in
drivers as well. Instead of writing something like

	period = clamp(period, min_period, max_period);

they will instead write

	if (period < min_period || period > max_period)
		return -ERANGE;

Instead of viewing round_state as "what get_state would return if I
passed this state to apply_state", it is better to view it as "what is
the closest exactly representable state with parameters less than this
state." I believe that this latter representation is effectively
identical for users of round_state, but it allows for implementations of
apply_state which provide saner defaults for existing users.

--Sean

Uwe Kleine-König July 8, 2021, 7:43 p.m. UTC | #20

Hello Sean,

On Thu, Jul 08, 2021 at 12:59:18PM -0400, Sean Anderson wrote:
> On 6/30/21 4:35 AM, Uwe Kleine-König wrote:
> > I often mistype the name of the rounding function as "pwm_round_rate",
> > the better name is "pwm_round_state" of course. That's just me thinking
> > about clk_round_rate where ".._rate" is the right term. I'll try harder
> > to get this right from now on.
> > 
> > On Tue, Jun 29, 2021 at 06:21:15PM -0400, Sean Anderson wrote:
> > > On 6/29/21 4:51 PM, Uwe Kleine-König wrote:
> > > > On Tue, Jun 29, 2021 at 02:01:31PM -0400, Sean Anderson wrote:
> > > > > On 6/29/21 4:31 AM, Uwe Kleine-König wrote:
> > > > > > On Mon, Jun 28, 2021 at 01:41:43PM -0400, Sean Anderson wrote:
> > > > > >> On 6/28/21 1:20 PM, Uwe Kleine-König wrote:
> > > > > >> > On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
> > > > > >> >> On 6/28/21 12:24 PM, Uwe Kleine-König wrote:
> > > > > >> >> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
> > > > > >> >> > > On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
> > > > > >> >> > > > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
> > > > > >> >> > > IMO, this is the best way to prevent surprising results in the API.
> > > > > >> >> >
> > > > > >> >> > I think it's not possible in practise to refuse "near" misses and every
> > > > > >> >> > definition of "near" is in some case ridiculous. Also if you consider
> > > > > >> >> > the pwm_round_state() case you don't want to refuse any request to tell
> > > > > >> >> > as much as possible about your controller's capabilities. And then it's
> > > > > >> >> > straight forward to let apply behave in the same way to keep complexity
> > > > > >> >> > low.
> > > > > >> >> >
> > > > > >> >> > > The real issue here is that it is impossible to determine the correct
> > > > > >> >> > > way to round the PWM a priori, and in particular, without considering
> > > > > >> >> > > both duty_cycle and period. If a consumer requests very small
> > > > > >> >> > > period/duty cycle which we cannot produce, how should it be rounded?
> > > > > >> >> >
> > > > > >> >> > Yeah, because there is no obviously right one, I picked one that is as
> > > > > >> >> > wrong as the other possibilities but is easy to work with.
> > > > > >> >> >
> > > > > >> >> > > Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
> > > > > >> >> > > the least period? Or should we try and increase the period to better
> > > > > >> >> > > approximate the % duty cycle? And both of these decisions must be made
> > > > > >> >> > > knowing both parameters. We cannot (for example) just always round up,
> > > > > >> >> > > since we may produce a configuration with TLR0 == TLR1, which would
> > > > > >> >> > > produce 0% duty cycle instead of whatever was requested. Rounding rate
> > > > > >> >> > > will introduce significant complexity into the driver. Most of the time
> > > > > >> >> > > if a consumer requests an invalid rate, it is due to misconfiguration
> > > > > >> >> > > which is best solved by fixing the configuration.
> > > > > >> >> >
> > > > > >> >> > In the first step pick the biggest period not bigger than the requested
> > > > > >> >> > and then pick the biggest duty cycle that is not bigger than the
> > > > > >> >> > requested and that can be set with the just picked period. That is the
> > > > > >> >> > behaviour that all new drivers should do. This is somewhat arbitrary but
> > > > > >> >> > after quite some thought the most sensible in my eyes.
> > > > > >> >>
> > > > > >> >> And if there are no periods smaller than the requested period?
> > > > > >> >
> > > > > >> > Then return -ERANGE.
> > > > > >>
> > > > > >> Ok, so instead of
> > > > > >>
> > > > > >> 	if (cycles < 2 || cycles > priv->max + 2)
> > > > > >> 		return -ERANGE;
> > > > > >>
> > > > > >> you would prefer
> > > > > >>
> > > > > >> 	if (cycles < 2)
> > > > > >> 		return -ERANGE;
> > > > > >> 	else if (cycles > priv->max + 2)
> > > > > >> 		cycles = priv->max;
> > > > > >
> > > > > > The actual calculation is a bit harder to handle TCSR_UDT = 0 but in
> > > > > > principle, yes, but see below.
> > > > > >
> > > > > >> But if we do the above clamping for TLR0, then we have to recalculate
> > > > > >> the duty cycle for TLR1. Which I guess means doing something like
> > > > > >>
> > > > > >> 	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
> > > > > >> 	if (ret)
> > > > > >> 		return ret;
> > > > > >>
> > > > > >> 	state->duty_cycle = mult_frac(state->duty_cycle,
> > > > > >> 				      xilinx_timer_get_period(priv, tlr0, tcsr0),
> > > > > >> 				      state->period);
> > > > > >>
> > > > > >> 	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
> > > > > >> 	if (ret)
> > > > > >> 		return ret;
> > > > > >
> > > > > > No, you need something like:
> > > > > >
> > > > > > 	/*
> > > > > > 	 * The multiplication cannot overflow as both priv_max and
> > > > > > 	 * NSEC_PER_SEC fit into an u32.
> > > > > > 	 */
> > > > > > 	max_period = div64_ul((u64)priv->max * NSEC_PER_SEC, clkrate);
> > > > > >
> > > > > > 	/* cap period to the maximal possible value */
> > > > > > 	if (state->period > max_period)
> > > > > > 		period = max_period;
> > > > > > 	else
> > > > > > 		period = state->period;
> > > > > >
> > > > > > 	/* cap duty_cycle to the maximal possible value */
> > > > > > 	if (state->duty_cycle > max_period)
> > > > > > 		duty_cycle = max_period;
> > > > > > 	else
> > > > > > 		duty_cycle = state->duty_cycle;
> > > > >
> > > > > These caps may increase the % duty cycle.
> > > >
> > > > Correct.
> > > >
> > > > For some usecases keeping the relative duty cycle might be better, for
> > > > others it might not. I'm still convinced that in general my solution
> > > > makes sense, is computationally cheaper and easier to work with.
> > > 
> > > Can you please describe one of those use cases? Every PWM user I looked
> > > (grepping for pwm_apply_state and pwm_config) set the duty cycle as a
> > > percentage of the period, and not as an absolute time. Keeping the high
> > > time the same while changing the duty cycle runs contrary to the
> > > assumptions of all of those users.
> > 
> > Indeed there is no mainline driver that relies on this. There are some
> > smart LED controllers (e.g. WS2812B) where the duty_cycle is more
> > important than the period. (I admit a PWM is not really the right driver
> > for that one as it could only completely enable and complete disable
> > white color.) Also there are some servo motor chips where the absolute
> > duty is relevant but the period isn't (in some range). (See
> > https://www.mikrocontroller.net/articles/Modellbauservo_Ansteuerung#Signalaufbau
> > for a article about that (in German though).)
> 
> > In case you want to argue that out-of-mainline users don't count:
> 
> They don't :)
> 
> This is a kernel-internal API.
> 
> But my point is that the vast majority of PWM users, and 100% of the
> in-kernel users care about the % duty cycle and not about the absolute
> high time. We should design around the common use case first and
> foremost.
> 
> > I think in the design of an API they do count to place the bar to
> > enter the mainline low. Frameworks should be generic enough to cover
> > as much use cases as possible.
> 
> They can cover many use-cases as possible, but they should be ergonomic
> firstly for the most common use case.

First I think going the easy way now and accepting to have to spend
quite more work later isn't a sensible approach. Second: Your approach
isn't even easier. And third: I agree that it should be easy for
consumers who care more about relative duty cycle than absolute numers.
But that doesn't imply that each and every device driver must provide
exactly that algorithm. The possible ways forward are:

 a) The framework has no complexity and all lowlevel drivers have to do
    complicated maths; 

 b) The framework contains the complexity and the lowlevel drivers are
    simpler.

I'd choose b) at any time.

> > And note that if you want a nearest to (say) 50% relative duty cycle and
> > don't care much about the period it doesn't really matter if you scale
> > duty_cycle in pwm_round_state() to the period change or not because in
> > general you need several calls to pwm_round_state() anyhow to find a
> > setting with 51% if the next lower possibility is 47%. So in the end you
> > save (I think) one call in generic PWM code.
> > 
> > In contrast the math gets quite a bit more complicated because there is
> > rounding involved in scaling the duty cycle. Consider a PWM that can
> > configure period and duty in 16.4 ns steps and you ask for
> > 
> > 	.period = 100 ns
> > 	.duty_cycle = 50 ns
> > 
> > Then the best period you can provide is 98.4 ns, so you return .period =
> > 99 from pwm_round_state(). (Yes, you don't return 98, because
> > round-nearest is much harder to handle than round down.)
> > To determine the adapted duty_cycle you have to do
> > 
> > 	50 * realperiod / 100
> > 
> > which independently of choosing 98, 98.4 or 99 for realperiod is 49. Then
> > to approximate 49 without rounding up you end up with 32.8 while 49.2
> > would have be perfectly fine.
> 
> And what if the consumer comes and requests 49 for their period in the
> first place? You have the same problem. The rescaling made it worse in
> this instance, but this is just an unfortunate case study.

I cannot follow. There are cases that are easy and others are hard.
Obviously I presented a hard case, and just because there are simpler
cases, too, doesn't mean that implementing the algorithm that must cover
all cases becomes simple, too. Maybe I just didn't understand what you
want to say?!

> > You might find a way around that (maybe you have to round up in the
> > adaption of duty_cycle, I didn't convince myself this is good enough
> > though).
> > 
> > So your suggestion to adapt the duty_cycle to keep the relative
> > duty_cycle constant (as good as possible within the bounds the hardware
> > dictates) implies additional complication at the driver level.
> > 
> >  From a framework maintainer's point of view (and also from a low-level
> > driver maintainer's point of view) I prefer one complication in a
> > generic function over a complication that I have to care for in each and
> > every low-level driver by a big margin.
> 
> FWIW what you're suggesting is also complex for the low-level driver.

Well, it is as complex as necessary and simpler than adapting the
duty_cycle as you suggested.

> [...]
> > Can you please come up with an algorithm to judge if a given deviation
> > is reasonable or surprising? I agree there are surprises and some of
> > them are obviously bad. For most cases however the judgement depends on
> > the use case so I fail to see how someone should program such a check
> > that should cover all consumers and use cases. I prefer no precautions +
> > an easy relation between pwm_round_state and pwm_apply_state (i.e.
> > behave identically) over a most of the time(?) useless precaution and
> > some policy defined differences between pwm_round_state and
> > pwm_apply_state
> 
> After thinking it over, I believe I agree with you on most things, but I
> think your proposed API has room for additional checks without any loss
> of generality.

\o/

> The PWM subsystem has several major players:
> 
> * Existing users of the PWM API. Most of these do not especially care
>   about the PWM period, usually just leaving at the default. The
>   exception is of course the pwm-clk driver. Many of these users care
>   about % duty cycle, and they all calculate the high time based on the
>   configured period of the PWM. I suspect that while many of these users
>   have substantial leeway in what accuracy they expect from the % duty
>   cycle, significant errors (in the 25-50% range) are probably unusual
>   and indicative of a misconfigured period. Unfortunately, we cannot
>   make a general judgement about what sort of accuracy is OK in most
>   cases.

ack.

> * Hypothetical future users of some kind of round_state function. These
>   users have some kind of algorithm which determines whether a PWM state
>   is acceptable for the driver. Most of the time this will be some kind
>   of accuracy check. What the round_state function returns is not
>   particularly important, because users have the opportunity to revise
>   their request based on what the state is rounded to. However, it is
>   important that each round rate function is consistent in manner that
>   it rounds so that these users

This sentence isn't complete, is it? One thing I consider important is
that there is a policy which of the implementable states is returned for
a given request to make it efficient to search for a best state
(depending on what the consumer driver considers best). Otherwise this
yields to too much distinctions of cases.

> * Existing drivers for the PWM subsystem. These drivers must implement
>   an apply_state function which is correct for both existing and future
>   users. In addition, they may implement some kind of round_state
>   function in the future. it is important to reduce the complexity of
>   the calculations these drivers perform so that it is easier to
>   implement and review them.

It's hard to know what "correct" means. But ack for "They should not be
more complex than necessary".

> I believe the following requirements satisfy the above constraints:
> 
> * The round_state function shall round the period to the largest period
>   representable by the PWM less than the requested period. It shall also
>   round the duty cycle to the largest duty cycle representable by the
>   PWM less than the requested duty cycle. No attempt shall be made to
>   preserve the % duty cycle.

ack if you replace "less" by "less or equal" twice.

> * The apply_state function shall only round the requested period down, and
>   may do so by no more than one unit cycle. If the requested period is
>   unrepresentable by the PWM, the apply_state function shall return
>   -ERANGE.

I don't understand what you mean by "more than one unit cycle", but that
doesn't really matter for what I think is wrong with that approach:

I think this is a bad idea if with "apply_state" you mean the callback
each driver has to implement: Once you made all drivers conformant to
this, someone will argue that one unit cycle is too strict. Or that it's
ok to increase the period iff the duty_cycle is 0.

Then you have to adapt all 50 or so drivers to adapt the policy.
Better let .apply_state() do the same as .round_state() and then you can
have in the core (i.e. in a single place):

	def pwm_apply_state(pwm, state):
	    rounded_state = pwm_round_state(pwm, state)
	    if some_condition(rounded_state, state):
	    	return -ERANGE
	    else:
	    	pwm->apply(pwm, state)

Having said that I think some_condition should always return False, but
independant of the discussion how some_condition should actually behave
this is definitively better than to hardcode some_condition in each
driver.

> * The apply_state function shall only round the requested duty cycle
>   down. The apply_state function shall not return an error unless there
>   is no duty cycle less than the requested duty cycle which is
>   representable by the PWM.

ack. (Side note: Most drivers can implement duty_cycle = 0, so for them
duty_cycle isn't a critical thing.)

> * After applying a state returned by round_state with apply_state,
>   get_state must return that state.

ack.

> The reason that we must return an error when the period is
> unrepresentable is that generally the duty cycle is calculated based on
> the period. This change has no affect on future users of round_state,
> since that function will only return valid periods. Those users will
> have the opportunity to detect that the period has changed and determine
> if the duty cycle is still acceptable.

ack up to here.

> However, for existing users, we
> should also provide the same opportunity.

Here you say: If the period has changed they should get a return value
of -ERANGE, right? Now what should they do with that. Either they give
up (which is bad) or they need to resort to pwm_round_state to
find a possible way forward. So they have to belong in the group of
round_state users and so they can do this from the start and then don't
need to care about some_condition at all.

> This requirement simplifies
> the behavior of apply_state, since there is no longer any chance that
> the % duty cycle is rounded up.

This is either wrong, or I didn't understand you. For my hypothetical
hardware that can implement periods and duty_cycles that are multiples
of 16.4 ns the following request:

	period = 1650
	duty_cycle = 164

(with relative duty_cycle = 9.9393939393939 %)
will be round to:

	period = 1640
	duty_cycle = 164

which has a higher relative duty_cycle (i.e. 10%).

> This requirement is easy to implement in
> drivers as well. Instead of writing something like
> 
> 	period = clamp(period, min_period, max_period);
> 
> they will instead write
> 
> 	if (period < min_period || period > max_period)
> 		return -ERANGE;

Are you aware what this means for drivers that only support a single
fixed period?

I still think it should be:

	if (period < min_period)
		return -ERANGE;
	
	if (period > max_period)
		period = max_period;

There are two reasons for this compared to your suggestion:

 a) Consider again the 16.4 ns driver and that it is capable to
    implement periods up to 16400 ns. With your approach a request of
    16404 ns will yield -ERANGE.
    Now compare that with a different 16.4 ns driver with max_period =
    164000 ns. The request of 16404 ns will yield 16400 ns, just because
    this driver could also do 16416.4 ns. This is strange, because the
    possibility to do 16416.4 ns is totally irrelevant here, isn't it?

 b) If a consumer asked for a certain state and gets back -ENORANGE they
    don't know if they should increase or decrease the period to guess a
    state that might be implementable instead.

(Hmm, or are you only talking about .apply_state and only .round_state
should do if (period < min_period) return -ERANGE; if (period >
max_period) period = max_period;? If so, I'd like to have this in the
framework, not in each driver. Then .round_state and .apply_state can be
identical which is good for reducing complexity.)

> Instead of viewing round_state as "what get_state would return if I
> passed this state to apply_state", it is better to view it as "what is
> the closest exactly representable state with parameters less than this
> state."
> I believe that this latter representation is effectively identical for
> users of round_state, but it allows for implementations of apply_state
> which provide saner defaults for existing users.

I look forward to how you modify your claim here after reading my
reasoning above.

Best regards
Uwe

Sean Anderson July 12, 2021, 4:26 p.m. UTC | #21

On 7/8/21 3:43 PM, Uwe Kleine-König wrote:
> Hello Sean,
>
> On Thu, Jul 08, 2021 at 12:59:18PM -0400, Sean Anderson wrote:
>> And what if the consumer comes and requests 49 for their period in the
>> first place? You have the same problem. The rescaling made it worse in
>> this instance, but this is just an unfortunate case study.
>
> I cannot follow. There are cases that are easy and others are hard.
> Obviously I presented a hard case, and just because there are simpler
> cases, too, doesn't mean that implementing the algorithm that must cover
> all cases becomes simple, too. Maybe I just didn't understand what you
> want to say?!

My point is that you cannot just pick a bad case and call the whole
process poor. I can do the same thing for your proposed process.
In any case, I don't wish to propose that drivers do rescaling in this
manner; I hoped that my below discussion had made that clear.

Though I really would like if we could pick a different name for "the
duration of the initial part of the PWM's cycle". I know "high time" is
not strictly correct for inverted polarity, but duty cycle refers to
percentage everywhere but the Linux kernel...

>> > You might find a way around that (maybe you have to round up in the
>> > adaption of duty_cycle, I didn't convince myself this is good enough
>> > though).
>> >
>> > So your suggestion to adapt the duty_cycle to keep the relative
>> > duty_cycle constant (as good as possible within the bounds the hardware
>> > dictates) implies additional complication at the driver level.
>> >
>> >  From a framework maintainer's point of view (and also from a low-level
>> > driver maintainer's point of view) I prefer one complication in a
>> > generic function over a complication that I have to care for in each and
>> > every low-level driver by a big margin.
>>
>> FWIW what you're suggesting is also complex for the low-level driver.
>
> Well, it is as complex as necessary and simpler than adapting the
> duty_cycle as you suggested.
>> [...]
>> > Can you please come up with an algorithm to judge if a given deviation
>> > is reasonable or surprising? I agree there are surprises and some of
>> > them are obviously bad. For most cases however the judgement depends on
>> > the use case so I fail to see how someone should program such a check
>> > that should cover all consumers and use cases. I prefer no precautions +
>> > an easy relation between pwm_round_state and pwm_apply_state (i.e.
>> > behave identically) over a most of the time(?) useless precaution and
>> > some policy defined differences between pwm_round_state and
>> > pwm_apply_state
>>
>> After thinking it over, I believe I agree with you on most things, but I
>> think your proposed API has room for additional checks without any loss
>> of generality.
>
> \o/
>
>> The PWM subsystem has several major players:
>>
>> * Existing users of the PWM API. Most of these do not especially care
>>   about the PWM period, usually just leaving at the default. The
>>   exception is of course the pwm-clk driver. Many of these users care
>>   about % duty cycle, and they all calculate the high time based on the
>>   configured period of the PWM. I suspect that while many of these users
>>   have substantial leeway in what accuracy they expect from the % duty
>>   cycle, significant errors (in the 25-50% range) are probably unusual
>>   and indicative of a misconfigured period. Unfortunately, we cannot
>>   make a general judgement about what sort of accuracy is OK in most
>>   cases.
>
> ack.
>
>> * Hypothetical future users of some kind of round_state function. These
>>   users have some kind of algorithm which determines whether a PWM state
>>   is acceptable for the driver. Most of the time this will be some kind
>>   of accuracy check. What the round_state function returns is not
>>   particularly important, because users have the opportunity to revise
>>   their request based on what the state is rounded to. However, it is
>>   important that each round rate function is consistent in manner that
>>   it rounds so that these users
>
> This sentence isn't complete, is it?

this should be finished like

	... can manipulate it programmatically.

> One thing I consider important is
> that there is a policy which of the implementable states is returned for
> a given request to make it efficient to search for a best state
> (depending on what the consumer driver considers best). Otherwise this
> yields to too much distinctions of cases.
>
>> * Existing drivers for the PWM subsystem. These drivers must implement
>>   an apply_state function which is correct for both existing and future
>>   users. In addition, they may implement some kind of round_state
>>   function in the future. it is important to reduce the complexity of
>>   the calculations these drivers perform so that it is easier to
>>   implement and review them.
>
> It's hard to know what "correct" means. But ack for "They should not be
> more complex than necessary".
>
>> I believe the following requirements satisfy the above constraints:
>>
>> * The round_state function shall round the period to the largest period
>>   representable by the PWM less than the requested period. It shall also
>>   round the duty cycle to the largest duty cycle representable by the
>>   PWM less than the requested duty cycle. No attempt shall be made to
>>   preserve the % duty cycle.
>
> ack if you replace "less" by "less or equal" twice.

Yes.

>> * The apply_state function shall only round the requested period down, and
>>   may do so by no more than one unit cycle. If the requested period is
>>   unrepresentable by the PWM, the apply_state function shall return
>>   -ERANGE.
>
> I don't understand what you mean by "more than one unit cycle", but
> that doesn't really matter for what I think is wrong with that
> approach: I think this is a bad idea if with "apply_state" you mean
> the callback each driver has to implement: Once you made all drivers
> conformant to this, someone will argue that one unit cycle is too
> strict.

The intent here is to provide guidance against drivers which round
excessively. That is, a driver which always rounded down to its minimum
period would not be very interesting. And neither would a driver which
did not make a very good effort (such as always rounding to multiples of
10 when it could round to multiples of 3 or whatever). So perhaps
s/shall/should/.

> Or that it's ok to increase the period iff the duty_cycle is 0.

IMO it doesn't matter what the period is for a duty cycle of 0 or 100.
Whatever policy we decide on, the behavior in that case will

> Then you have to adapt all 50 or so drivers to adapt the policy.

Of course, as I understand it, this must be done for your policy as
well.

> Better let .apply_state() do the same as .round_state() and then you can
> have in the core (i.e. in a single place):
>
> 	def pwm_apply_state(pwm, state):
> 	    rounded_state = pwm_round_state(pwm, state)
> 	    if some_condition(rounded_state, state):
> 	    	return -ERANGE
> 	    else:
> 	    	pwm->apply(pwm, state)
>
> Having said that I think some_condition should always return False, but
> independant of the discussion how some_condition should actually behave
> this is definitively better than to hardcode some_condition in each
> driver.

And IMO the condition should just be "is the period different"?

I think a nice interface for many existing users would be something like

	# this ignores polarity and intermediate errors, but that should
	# not be terribly difficult to add
	def pwm_apply_relative_duty_cycle(pwm, duty_cycle, scale):
	    state = pwm_get_state(pwm)
	    state.enabled = True
	    state = pwm_set_relative_duty_cycle(state, duty_cycle, scale)
	    rounded_state = pwm_round_state(pwm, state)
	    if rounded_state.period != state.period:
	        state = pwm_set_relative_duty_cycle(rounded_state, duty_cycle, scale)
		rounded_state = pwm_round_state(pwm, state)
             if duty_cycle and not rounded_state.duty_cycle:
	        return -ERANGE
	    return pwm_apply_state(pwm, rounded_state)

which of course could be implemented both with your proposed semantics
or with mine.

>> * The apply_state function shall only round the requested duty cycle
>>   down. The apply_state function shall not return an error unless there
>>   is no duty cycle less than the requested duty cycle which is
>>   representable by the PWM.
>
> ack. (Side note: Most drivers can implement duty_cycle = 0, so for them
> duty_cycle isn't a critical thing.)

Yes, and unfortunately the decision is not as clear-cut as for period.

>> * After applying a state returned by round_state with apply_state,
>>   get_state must return that state.
>
> ack.
>
>> The reason that we must return an error when the period is
>> unrepresentable is that generally the duty cycle is calculated based on
>> the period. This change has no affect on future users of round_state,
>> since that function will only return valid periods. Those users will
>> have the opportunity to detect that the period has changed and determine
>> if the duty cycle is still acceptable.
>
> ack up to here.
>
>> However, for existing users, we
>> should also provide the same opportunity.
>
> Here you say: If the period has changed they should get a return value
> of -ERANGE, right? Now what should they do with that. Either they give
> up (which is bad)

No, this is exactly what we want. Consider how period is set. Either
it is whatever the default is (e.g. set by PoR or the bootloader), in
which case it is a driver bug if we think it is unrepresentable, or it
is set from the device tree (or platform info), in which case it is a
bug in the configuration. This is not something like duty cycle where
you could make a case depending on the user, but an actual case of
misconfiguration.

> or they need to resort to pwm_round_state to
> find a possible way forward. So they have to belong in the group of
> round_state users and so they can do this from the start and then don't
> need to care about some_condition at all.
>
>> This requirement simplifies
>> the behavior of apply_state, since there is no longer any chance that
>> the % duty cycle is rounded up.
>
> This is either wrong, or I didn't understand you. For my hypothetical
> hardware that can implement periods and duty_cycles that are multiples
> of 16.4 ns the following request:
>
> 	period = 1650
> 	duty_cycle = 164
>
> (with relative duty_cycle = 9.9393939393939 %)
> will be round to:
>
> 	period = 1640
> 	duty_cycle = 164
>
> which has a higher relative duty_cycle (i.e. 10%).

This is effectively bound by the clause above to be no more than the
underlying precision of the PWM.  Existing users expect to be able to
pass unrounded periods/duty cycles, so we need to round in some manner.
Any way we round is OK, as long as it is not terribly excessive (hence
the clause above). We could have chosen to round up (and in fact this is
exactly what happens for inverted polarity PWMs). But I think that for
ease of implementation is is better to mostly round in the same manner
as round_state.

>> This requirement is easy to implement in
>> drivers as well. Instead of writing something like
>>
>> 	period = clamp(period, min_period, max_period);
>>
>> they will instead write
>>
>> 	if (period < min_period || period > max_period)
>> 		return -ERANGE;
>
> Are you aware what this means for drivers that only support a single
> fixed period?

This is working as designed. Either the period comes from configuration
(e.g. pwm_init_state), which is specialized to the board in question, in
which case it is OK to return an error because the writer of the dts
either should leave it as the default or specify it correctly, or it
comes from pwm_get_state in which case it is a driver error for
returning a a period which that driver cannot support.

There are two exceptions to the above. First, a fixed period PWM driver
could have its period changed by the parent clock frequency changing.
But I think such driver should just clk_rate_exclusive_get because
otherwise all bets are off. You just have to hope your consumer doesn't
care about the period.

The other exception is pwm_clk. In this case, I think it is reasonable
to pass an error on if the user tries to change the frequency of what is
effectively a fixed-rate clock.

> I still think it should be:
>
> 	if (period < min_period)
> 		return -ERANGE;
> 	
> 	if (period > max_period)
> 		period = max_period;
>
> There are two reasons for this compared to your suggestion:
>
>   a) Consider again the 16.4 ns driver and that it is capable to
>      implement periods up to 16400 ns. With your approach a request of
>      16404 ns will yield -ERANGE.
>      Now compare that with a different 16.4 ns driver with max_period =
>      164000 ns. The request of 16404 ns will yield 16400 ns, just because
>      this driver could also do 16416.4 ns. This is strange, because the
>      possibility to do 16416.4 ns is totally irrelevant here, isn't it?

Ah, it looks like I mis-specified this a little bit. My intent was

	The apply_state function shall only round the requested period
	down, and should do so by no more than one unit cycle. If the
	period *rounded as such* is unrepresentable by the PWM, the
	apply_state function shall return -ERANGE.

>   b) If a consumer asked for a certain state and gets back -ENORANGE they
>      don't know if they should increase or decrease the period to guess a
>      state that might be implementable instead.

Because I believe this is effectively a configuration issue, it should
be obvious to the user which direction they need to go. Programmatic
users which want to automatically pick a better period would need to use
round_state instead.

> (Hmm, or are you only talking about .apply_state and only .round_state
> should do if (period < min_period) return -ERANGE; if (period >
> max_period) period = max_period;?

Yes.

> If so, I'd like to have this in the framework, not in each driver.
> Then .round_state and .apply_state can be identical which is good for
> reducing complexity.)

So you would like each PWM driver to have a "max_period" and
"min_period" parameter? And what if the clock rate changes? Otherwise,
how do you propose that the framework detect when a requested period is
out of range?

>> Instead of viewing round_state as "what get_state would return if I
>> passed this state to apply_state", it is better to view it as "what is
>> the closest exactly representable state with parameters less than this
>> state."
>> I believe that this latter representation is effectively identical for
>> users of round_state, but it allows for implementations of apply_state
>> which provide saner defaults for existing users.
>
> I look forward to how you modify your claim here after reading my
> reasoning above.

I believe it stands as-is.

--Sean

Uwe Kleine-König July 12, 2021, 7:49 p.m. UTC | #22

Hello Sean,

On Mon, Jul 12, 2021 at 12:26:47PM -0400, Sean Anderson wrote:
> On 7/8/21 3:43 PM, Uwe Kleine-König wrote:
> > Hello Sean,
> > 
> > On Thu, Jul 08, 2021 at 12:59:18PM -0400, Sean Anderson wrote:
> > > And what if the consumer comes and requests 49 for their period in the
> > > first place? You have the same problem. The rescaling made it worse in
> > > this instance, but this is just an unfortunate case study.
> > 
> > I cannot follow. There are cases that are easy and others are hard.
> > Obviously I presented a hard case, and just because there are simpler
> > cases, too, doesn't mean that implementing the algorithm that must cover
> > all cases becomes simple, too. Maybe I just didn't understand what you
> > want to say?!
> 
> My point is that you cannot just pick a bad case and call the whole
> process poor.

Yeah, there are surprises with both approaches. My point is mostly that
if you cannot prevent these surprises with a more complicate algorithm,
then better live with the surprises and stick to the simple algorithm.

(For the sake of completeness: If the request for my 16.4 ns PWM was

	.period = 100 ns
	.duty_cycle = 49 ns

the hardware should be configured with

	.period = 98.4 ns
	.duty_cycle = 32.8 ns

. The key difference to the scaling approach is: The computation is easy
and it's clear how it should be implemented. So I don't see how my
approach yields problems.)

> I can do the same thing for your proposed process.
> In any case, I don't wish to propose that drivers do rescaling in this
> manner; I hoped that my below discussion had made that clear.

Yes, After reading the start of your mail I was positively surprised to
see you reasoning for nearly the same things as I do :-)

> Though I really would like if we could pick a different name for "the
> duration of the initial part of the PWM's cycle". I know "high time" is
> not strictly correct for inverted polarity, but duty cycle refers to
> percentage everywhere but the Linux kernel...

"active time" would be a term that should be fine. But I think
"duty-cycle" in contrast to "relative duty-cycle" is fine enough and
IMHO we should keep the terms as they are.

> > > * Hypothetical future users of some kind of round_state function. These
> > >   users have some kind of algorithm which determines whether a PWM state
> > >   is acceptable for the driver. Most of the time this will be some kind
> > >   of accuracy check. What the round_state function returns is not
> > >   particularly important, because users have the opportunity to revise
> > >   their request based on what the state is rounded to. However, it is
> > >   important that each round rate function is consistent in manner that
> > >   it rounds so that these users
> > 
> > This sentence isn't complete, is it?
> 
> this should be finished like
> 
> 	... can manipulate it programmatically.

Then ack.

> > > * The apply_state function shall only round the requested period down, and
> > >   may do so by no more than one unit cycle. If the requested period is
> > >   unrepresentable by the PWM, the apply_state function shall return
> > >   -ERANGE.
> > 
> > I don't understand what you mean by "more than one unit cycle", but
> > that doesn't really matter for what I think is wrong with that
> > approach: I think this is a bad idea if with "apply_state" you mean
> > the callback each driver has to implement: Once you made all drivers
> > conformant to this, someone will argue that one unit cycle is too
> > strict.
> 
> The intent here is to provide guidance against drivers which round
> excessively. That is, a driver which always rounded down to its minimum
> period would not be very interesting. And neither would a driver which
> did not make a very good effort (such as always rounding to multiples of
> 10 when it could round to multiples of 3 or whatever). So perhaps
> s/shall/should/.

Ah, that's what I formalized as "return the *biggest possible* period
not bigger than the request". fine.
 
> > Or that it's ok to increase the period iff the duty_cycle is 0.
> 
> IMO it doesn't matter what the period is for a duty cycle of 0 or 100.
> Whatever policy we decide on, the behavior in that case will

So it might even be you who thinks that the request

	.period = 15
	.duty_cycle = 0

should not be refused just because the smallest implementable period is
16.4 ns :-)

> > Then you have to adapt all 50 or so drivers to adapt the policy.
> 
> Of course, as I understand it, this must be done for your policy as
> well.

Well to be fair, yes. But the advantage of my policy is that it
returns success in more situations and so the core (and so the consumer)
can work with that in more cases.

> > Better let .apply_state() do the same as .round_state() and then you can
> > have in the core (i.e. in a single place):
> > 
> > 	def pwm_apply_state(pwm, state):
> > 	    rounded_state = pwm_round_state(pwm, state)
> > 	    if some_condition(rounded_state, state):
> > 	    	return -ERANGE
> > 	    else:
> > 	    	pwm->apply(pwm, state)
> > 
> > Having said that I think some_condition should always return False, but
> > independant of the discussion how some_condition should actually behave
> > this is definitively better than to hardcode some_condition in each
> > driver.
> 
> And IMO the condition should just be "is the period different"?

So a request of .period = X must result in a real period that's bigger
than X - 1 and not bigger than X, correct?

> I think a nice interface for many existing users would be something like
> 
>         # this ignores polarity and intermediate errors, but that should
>         # not be terribly difficult to add
>         def pwm_apply_relative_duty_cycle(pwm, duty_cycle, scale):
>             state = pwm_get_state(pwm)
>             state.enabled = True
>             state = pwm_set_relative_duty_cycle(state, duty_cycle, scale)
>             rounded_state = pwm_round_state(pwm, state)
>             if rounded_state.period != state.period:
>                 state = pwm_set_relative_duty_cycle(rounded_state, duty_cycle, scale)
>                 rounded_state = pwm_round_state(pwm, state)

This should be rounded_state, right?! -----------------^^^^^

>             if duty_cycle and not rounded_state.duty_cycle:
>                 return -ERANGE
>             return pwm_apply_state(pwm, rounded_state)

(Fixed tabs vs space indention)

I oppose to the duty_cycle and not rounded_state.duty_cycle check. Zero
shouldn't be handled differently to other values. If it's ok to round 32
ns down to 16.4 ns, rounding down 2 ns to 0 should be fine, too.

Also for these consumers it might make sense to allow rounding up
period, so if a consumer requests .period = 32 ns, better yield 32.8 ns
instead of 16.4 ns.

> which of course could be implemented both with your proposed semantics
> or with mine.

Yeah, and each pwm_state that doesn't yield an error is an advantage.
(OK, you could argue now that period should be round up if a too small
value for period is requested. That's a weighing to reduce complexity in
the lowlevel drivers.)

> > > * The apply_state function shall only round the requested duty cycle
> > >   down. The apply_state function shall not return an error unless there
> > >   is no duty cycle less than the requested duty cycle which is
> > >   representable by the PWM.
> > 
> > ack. (Side note: Most drivers can implement duty_cycle = 0, so for them
> > duty_cycle isn't a critical thing.)
> 
> Yes, and unfortunately the decision is not as clear-cut as for period.

Oh, note that up to now we consider different options as the right thing
to do with period. That's not what I would call clear-cut :-)

> > > * After applying a state returned by round_state with apply_state,
> > >   get_state must return that state.
> > 
> > ack.
> > 
> > > The reason that we must return an error when the period is
> > > unrepresentable is that generally the duty cycle is calculated based on
> > > the period. This change has no affect on future users of round_state,
> > > since that function will only return valid periods. Those users will
> > > have the opportunity to detect that the period has changed and determine
> > > if the duty cycle is still acceptable.
> > 
> > ack up to here.
> > 
> > > However, for existing users, we
> > > should also provide the same opportunity.
> > 
> > Here you say: If the period has changed they should get a return value
> > of -ERANGE, right? Now what should they do with that. Either they give
> > up (which is bad)
> 
> No, this is exactly what we want. Consider how period is set. Either
> it is whatever the default is (e.g. set by PoR or the bootloader), in
> which case it is a driver bug if we think it is unrepresentable, or it
> is set from the device tree (or platform info), in which case it is a
> bug in the configuration. This is not something like duty cycle where
> you could make a case depending on the user, but an actual case of
> misconfiguration.

That is very little cooperative. The result is that the pwm-led driver
fails to blink because today the UART driver was probed before the
pwm-led and changed a clk in a way that the pwm-led cannot achieve the
configured period any more.

> > or they need to resort to pwm_round_state to
> > find a possible way forward. So they have to belong in the group of
> > round_state users and so they can do this from the start and then don't
> > need to care about some_condition at all.
> > 
> > > This requirement simplifies
> > > the behavior of apply_state, since there is no longer any chance that
> > > the % duty cycle is rounded up.
> > 
> > This is either wrong, or I didn't understand you. For my hypothetical
> > hardware that can implement periods and duty_cycles that are multiples
> > of 16.4 ns the following request:
> > 
> > 	period = 1650
> > 	duty_cycle = 164
> > 
> > (with relative duty_cycle = 9.9393939393939 %)
> > will be round to:
> > 
> > 	period = 1640
> > 	duty_cycle = 164
> > 
> > which has a higher relative duty_cycle (i.e. 10%).
> 
> This is effectively bound by the clause above to be no more than the
> underlying precision of the PWM.  Existing users expect to be able to
> pass unrounded periods/duty cycles, so we need to round in some manner.
> Any way we round is OK, as long as it is not terribly excessive (hence
> the clause above). We could have chosen to round up (and in fact this is
> exactly what happens for inverted polarity PWMs). But I think that for
> ease of implementation is is better to mostly round in the same manner
> as round_state.
> 
> > > This requirement is easy to implement in
> > > drivers as well. Instead of writing something like
> > > 
> > > 	period = clamp(period, min_period, max_period);
> > > 
> > > they will instead write
> > > 
> > > 	if (period < min_period || period > max_period)
> > > 		return -ERANGE;
> > 
> > Are you aware what this means for drivers that only support a single
> > fixed period?
> 
> This is working as designed. Either the period comes from configuration
> (e.g. pwm_init_state), which is specialized to the board in question, in
> which case it is OK to return an error because the writer of the dts
> either should leave it as the default or specify it correctly, or it
> comes from pwm_get_state in which case it is a driver error for
> returning a a period which that driver cannot support.
> 
> There are two exceptions to the above. First, a fixed period PWM driver
> could have its period changed by the parent clock frequency changing.
> But I think such driver should just clk_rate_exclusive_get because
> otherwise all bets are off. You just have to hope your consumer doesn't
> care about the period.
> 
> The other exception is pwm_clk. In this case, I think it is reasonable
> to pass an error on if the user tries to change the frequency of what is
> effectively a fixed-rate clock.
> 
> > I still think it should be:
> > 
> > 	if (period < min_period)
> > 		return -ERANGE;
> > 	
> > 	if (period > max_period)
> > 		period = max_period;
> > 
> > There are two reasons for this compared to your suggestion:
> > 
> >   a) Consider again the 16.4 ns driver and that it is capable to
> >      implement periods up to 16400 ns. With your approach a request of
> >      16404 ns will yield -ERANGE.
> >      Now compare that with a different 16.4 ns driver with max_period =
> >      164000 ns. The request of 16404 ns will yield 16400 ns, just because
> >      this driver could also do 16416.4 ns. This is strange, because the
> >      possibility to do 16416.4 ns is totally irrelevant here, isn't it?
> 
> Ah, it looks like I mis-specified this a little bit. My intent was
> 
> 	The apply_state function shall only round the requested period
> 	down, and should do so by no more than one unit cycle. If the
> 	period *rounded as such* is unrepresentable by the PWM, the
> 	apply_state function shall return -ERANGE.

I don't understand "one unit cycle". What is a unit cycle for a PWM that
can implement periods in the form 10 s / X for X in [1, ... 4096]? What
is a unit cycle for a fixed period PWM?

> >   b) If a consumer asked for a certain state and gets back -ENORANGE they
> >      don't know if they should increase or decrease the period to guess a
> >      state that might be implementable instead.
> 
> Because I believe this is effectively a configuration issue, it should
> be obvious to the user which direction they need to go. Programmatic
> users which want to automatically pick a better period would need to use
> round_state instead.

You only consider consumers with a fixed period. Do you want to
explicitly configure all possible periods for a a driver that uses ~ 50%
relative duty cycle but varies period (e.g. the pwm-ir-tx driver)?
(OK, these drivers could use pwm_round_rate(), but then that argument
could be applied to all consumers and the result of an unrounded request
doesn't really matter any more.)

> > (Hmm, or are you only talking about .apply_state and only .round_state
> > should do if (period < min_period) return -ERANGE; if (period >
> > max_period) period = max_period;?
> 
> Yes.

I really want to have .apply_state() and .round_state() to behave
exactly the same. Everything else I don't want to ask from driver
authors. I don't believe you can argue enough that I will drop this
claim.

> > If so, I'd like to have this in the framework, not in each driver.
> > Then .round_state and .apply_state can be identical which is good for
> > reducing complexity.)
> 
> So you would like each PWM driver to have a "max_period" and
> "min_period" parameter?

No, I don't want this explicitly. What did I write to make you think
that? With .round_state() these values can be found out though.

> And what if the clock rate changes? Otherwise, how do you propose that
> the framework detect when a requested period is out of range?

I call round_rate() with

	.period = requested_period
	.duty_cycle = requested_period

and if that returns -ERANGE the PWM doesn't support this rate (and
smaller ones). And a big requested period is never out of range.

Best regards
Uwe

Sean Anderson July 13, 2021, 9:49 p.m. UTC | #23

On 7/12/21 3:49 PM, Uwe Kleine-König wrote:
> Hello Sean,
>
> On Mon, Jul 12, 2021 at 12:26:47PM -0400, Sean Anderson wrote:
>> On 7/8/21 3:43 PM, Uwe Kleine-König wrote:
>> > Hello Sean,
>> >
>> > On Thu, Jul 08, 2021 at 12:59:18PM -0400, Sean Anderson wrote:
>> > > And what if the consumer comes and requests 49 for their period in the
>> > > first place? You have the same problem. The rescaling made it worse in
>> > > this instance, but this is just an unfortunate case study.
>> >
>> > I cannot follow. There are cases that are easy and others are hard.
>> > Obviously I presented a hard case, and just because there are simpler
>> > cases, too, doesn't mean that implementing the algorithm that must cover
>> > all cases becomes simple, too. Maybe I just didn't understand what you
>> > want to say?!
>>
>> My point is that you cannot just pick a bad case and call the whole
>> process poor.
>
> Yeah, there are surprises with both approaches. My point is mostly that
> if you cannot prevent these surprises with a more complicate algorithm,
> then better live with the surprises and stick to the simple algorithm.
>
> (For the sake of completeness: If the request for my 16.4 ns PWM was
>
> 	.period = 100 ns
> 	.duty_cycle = 49 ns
>
> the hardware should be configured with
>
> 	.period = 98.4 ns
> 	.duty_cycle = 32.8 ns
>
> . The key difference to the scaling approach is: The computation is easy
> and it's clear how it should be implemented. So I don't see how my
> approach yields problems.)

It doesn't, which is why I suggested using it below.

>> I can do the same thing for your proposed process.
>> In any case, I don't wish to propose that drivers do rescaling in this
>> manner; I hoped that my below discussion had made that clear.
>
> Yes, After reading the start of your mail I was positively surprised to
> see you reasoning for nearly the same things as I do :-)
>
>> Though I really would like if we could pick a different name for "the
>> duration of the initial part of the PWM's cycle". I know "high time" is
>> not strictly correct for inverted polarity, but duty cycle refers to
>> percentage everywhere but the Linux kernel...
>
> "active time" would be a term that should be fine. But I think
> "duty-cycle" in contrast to "relative duty-cycle" is fine enough and
> IMHO we should keep the terms as they are.

It is very easy to misread one for the other, as I did above. I think
active time is a good term.

>> > > * Hypothetical future users of some kind of round_state function. These
>> > >   users have some kind of algorithm which determines whether a PWM state
>> > >   is acceptable for the driver. Most of the time this will be some kind
>> > >   of accuracy check. What the round_state function returns is not
>> > >   particularly important, because users have the opportunity to revise
>> > >   their request based on what the state is rounded to. However, it is
>> > >   important that each round rate function is consistent in manner that
>> > >   it rounds so that these users
>> >
>> > This sentence isn't complete, is it?
>>
>> this should be finished like
>>
>> 	... can manipulate it programmatically.
>
> Then ack.
>
>> > > * The apply_state function shall only round the requested period down, and
>> > >   may do so by no more than one unit cycle. If the requested period is
>> > >   unrepresentable by the PWM, the apply_state function shall return
>> > >   -ERANGE.
>> >
>> > I don't understand what you mean by "more than one unit cycle", but
>> > that doesn't really matter for what I think is wrong with that
>> > approach: I think this is a bad idea if with "apply_state" you mean
>> > the callback each driver has to implement: Once you made all drivers
>> > conformant to this, someone will argue that one unit cycle is too
>> > strict.
>>
>> The intent here is to provide guidance against drivers which round
>> excessively. That is, a driver which always rounded down to its minimum
>> period would not be very interesting. And neither would a driver which
>> did not make a very good effort (such as always rounding to multiples of
>> 10 when it could round to multiples of 3 or whatever). So perhaps
>> s/shall/should/.
>
> Ah, that's what I formalized as "return the *biggest possible* period
> not bigger than the request". fine.
>
>> > Or that it's ok to increase the period iff the duty_cycle is 0.
>>
>> IMO it doesn't matter what the period is for a duty cycle of 0 or 100.
>> Whatever policy we decide on, the behavior in that case will
>
> So it might even be you who thinks that the request
>
> 	.period = 15
> 	.duty_cycle = 0
>
> should not be refused just because the smallest implementable period is
> 16.4 ns :-)

If the general policy is to refuse periods less than the minimum
representable, then this should be refused. Otherwise, it should be
allowed. This edge case is not worth complicating drivers.

>> > Then you have to adapt all 50 or so drivers to adapt the policy.
>>
>> Of course, as I understand it, this must be done for your policy as
>> well.
>
> Well to be fair, yes. But the advantage of my policy is that it
> returns success in more situations and so the core (and so the consumer)
> can work with that in more cases.

Failure is as important for working with an API as success is.

>> > Better let .apply_state() do the same as .round_state() and then you can
>> > have in the core (i.e. in a single place):
>> >
>> > 	def pwm_apply_state(pwm, state):
>> > 	    rounded_state = pwm_round_state(pwm, state)
>> > 	    if some_condition(rounded_state, state):
>> > 	    	return -ERANGE
>> > 	    else:
>> > 	    	pwm->apply(pwm, state)
>> >
>> > Having said that I think some_condition should always return False, but
>> > independant of the discussion how some_condition should actually behave
>> > this is definitively better than to hardcode some_condition in each
>> > driver.
>>
>> And IMO the condition should just be "is the period different"?
>
> So a request of .period = X must result in a real period that's bigger
> than X - 1 and not bigger than X, correct?

Yes, if 1 is replaced by a suitable

>> I think a nice interface for many existing users would be something like
>>
>>         # this ignores polarity and intermediate errors, but that should
>>         # not be terribly difficult to add
>>         def pwm_apply_relative_duty_cycle(pwm, duty_cycle, scale):
>>             state = pwm_get_state(pwm)
>>             state.enabled = True
>>             state = pwm_set_relative_duty_cycle(state, duty_cycle, scale)
>>             rounded_state = pwm_round_state(pwm, state)
>>             if rounded_state.period != state.period:
>>                 state = pwm_set_relative_duty_cycle(rounded_state, duty_cycle, scale)
>>                 rounded_state = pwm_round_state(pwm, state)
>
> This should be rounded_state, right?! -----------------^^^^^

No, since we just recalculated the duty cycle on the line above.

>
>>             if duty_cycle and not rounded_state.duty_cycle:
>>                 return -ERANGE
>>             return pwm_apply_state(pwm, rounded_state)
>
> (Fixed tabs vs space indention)
>
> I oppose to the duty_cycle and not rounded_state.duty_cycle check. Zero
> shouldn't be handled differently to other values. If it's ok to round 32
> ns down to 16.4 ns, rounding down 2 ns to 0 should be fine, too.
>
> Also for these consumers it might make sense to allow rounding up
> period, so if a consumer requests .period = 32 ns, better yield 32.8 ns
> instead of 16.4 ns.

It might. Allowing apply_state to round closest while round_state rounds
down would be a good useability improvement IMO. The exact rounding
behavior of apply_state is not as important as round_state, so the
requirements detailed below could certainly be loosened.

>> which of course could be implemented both with your proposed semantics
>> or with mine.
>
> Yeah, and each pwm_state that doesn't yield an error is an advantage.
> (OK, you could argue now that period should be round up if a too small
> value for period is requested. That's a weighing to reduce complexity in
> the lowlevel drivers.)

Again, I don't think we should round period in apply_state, because that
throws % duty cycle (which most drivers care about) out the window. And
we both agree that rescaling duty cycle adds too much complexity for
drivers.

>> > > * The apply_state function shall only round the requested duty cycle
>> > >   down. The apply_state function shall not return an error unless there
>> > >   is no duty cycle less than the requested duty cycle which is
>> > >   representable by the PWM.
>> >
>> > ack. (Side note: Most drivers can implement duty_cycle = 0, so for them
>> > duty_cycle isn't a critical thing.)
>>
>> Yes, and unfortunately the decision is not as clear-cut as for period.
>
> Oh, note that up to now we consider different options as the right thing
> to do with period. That's not what I would call clear-cut :-)
>
>> > > * After applying a state returned by round_state with apply_state,
>> > >   get_state must return that state.
>> >
>> > ack.
>> >
>> > > The reason that we must return an error when the period is
>> > > unrepresentable is that generally the duty cycle is calculated based on
>> > > the period. This change has no affect on future users of round_state,
>> > > since that function will only return valid periods. Those users will
>> > > have the opportunity to detect that the period has changed and determine
>> > > if the duty cycle is still acceptable.
>> >
>> > ack up to here.
>> >
>> > > However, for existing users, we
>> > > should also provide the same opportunity.
>> >
>> > Here you say: If the period has changed they should get a return value
>> > of -ERANGE, right? Now what should they do with that. Either they give
>> > up (which is bad)
>>
>> No, this is exactly what we want. Consider how period is set. Either
>> it is whatever the default is (e.g. set by PoR or the bootloader), in
>> which case it is a driver bug if we think it is unrepresentable, or it
>> is set from the device tree (or platform info), in which case it is a
>> bug in the configuration. This is not something like duty cycle where
>> you could make a case depending on the user, but an actual case of
>> misconfiguration.
>
> That is very little cooperative. The result is that the pwm-led driver
> fails to blink because today the UART driver was probed before the
> pwm-led and changed a clk in a way that the pwm-led cannot achieve the
> configured period any more.

This is fine. There are two options for such a board. First, don't set
the period explicitly. You can always just leave the period at whatever
the default is. If you don't actually care about the period (since it
could change every boot), then you shouldn't be setting it explicitly.
Second, add an `assigned-clock-frequencies` property to the clocks node
and remove the race condition.

If you don't grab the clock rate, then you can easily have some
interaction like

     pwm_round_rate()
                        clk_set_rate()
     pwm_apply_state()

where suddenly the period you requested might be unattainable, or could
be rounded completely differently.

>> > or they need to resort to pwm_round_state to
>> > find a possible way forward. So they have to belong in the group of
>> > round_state users and so they can do this from the start and then don't
>> > need to care about some_condition at all.
>> >
>> > > This requirement simplifies
>> > > the behavior of apply_state, since there is no longer any chance that
>> > > the % duty cycle is rounded up.
>> >
>> > This is either wrong, or I didn't understand you. For my hypothetical
>> > hardware that can implement periods and duty_cycles that are multiples
>> > of 16.4 ns the following request:
>> >
>> > 	period = 1650
>> > 	duty_cycle = 164
>> >
>> > (with relative duty_cycle = 9.9393939393939 %)
>> > will be round to:
>> >
>> > 	period = 1640
>> > 	duty_cycle = 164
>> >
>> > which has a higher relative duty_cycle (i.e. 10%).
>>
>> This is effectively bound by the clause above to be no more than the
>> underlying precision of the PWM.  Existing users expect to be able to
>> pass unrounded periods/duty cycles, so we need to round in some manner.
>> Any way we round is OK, as long as it is not terribly excessive (hence
>> the clause above). We could have chosen to round up (and in fact this is
>> exactly what happens for inverted polarity PWMs). But I think that for
>> ease of implementation is is better to mostly round in the same manner
>> as round_state.
>>
>> > > This requirement is easy to implement in
>> > > drivers as well. Instead of writing something like
>> > >
>> > > 	period = clamp(period, min_period, max_period);
>> > >
>> > > they will instead write
>> > >
>> > > 	if (period < min_period || period > max_period)
>> > > 		return -ERANGE;
>> >
>> > Are you aware what this means for drivers that only support a single
>> > fixed period?
>>
>> This is working as designed. Either the period comes from configuration
>> (e.g. pwm_init_state), which is specialized to the board in question, in
>> which case it is OK to return an error because the writer of the dts
>> either should leave it as the default or specify it correctly, or it
>> comes from pwm_get_state in which case it is a driver error for
>> returning a a period which that driver cannot support.
>>
>> There are two exceptions to the above. First, a fixed period PWM driver
>> could have its period changed by the parent clock frequency changing.
>> But I think such driver should just clk_rate_exclusive_get because
>> otherwise all bets are off. You just have to hope your consumer doesn't
>> care about the period.
>>
>> The other exception is pwm_clk. In this case, I think it is reasonable
>> to pass an error on if the user tries to change the frequency of what is
>> effectively a fixed-rate clock.
>>
>> > I still think it should be:
>> >
>> > 	if (period < min_period)
>> > 		return -ERANGE;
>> > 	
>> > 	if (period > max_period)
>> > 		period = max_period;
>> >
>> > There are two reasons for this compared to your suggestion:
>> >
>> >   a) Consider again the 16.4 ns driver and that it is capable to
>> >      implement periods up to 16400 ns. With your approach a request of
>> >      16404 ns will yield -ERANGE.
>> >      Now compare that with a different 16.4 ns driver with max_period =
>> >      164000 ns. The request of 16404 ns will yield 16400 ns, just because
>> >      this driver could also do 16416.4 ns. This is strange, because the
>> >      possibility to do 16416.4 ns is totally irrelevant here, isn't it?
>>
>> Ah, it looks like I mis-specified this a little bit. My intent was
>>
>> 	The apply_state function shall only round the requested period
>> 	down, and should do so by no more than one unit cycle. If the
>> 	period *rounded as such* is unrepresentable by the PWM, the
>> 	apply_state function shall return -ERANGE.
>
> I don't understand "one unit cycle". What is a unit cycle for a PWM that
> can implement periods in the form 10 s / X for X in [1, ... 4096]? What
> is a unit cycle for a fixed period PWM?

One... quanta? step? detent? The idea is that the driver should give its
best approximation which doesn't just round everything down to the
maximum period. So if I were to better capture my above sentiment, I
would suggest

	The apply_state function shall round the requested period down
	to the largest representable period less than or equal to the
	requested period. However, it may not round a period larger than
	the largest representable period by more than the difference
	between the largest representable period and the next largest
	representable period. If no such period exists, or the period is
	too large to be rounded, the apply_state function shall return
	-ERANGE.

In your example, periods of 15s would be rounded down to 10s, since the
difference between the largest period (10s) and the next largest (5s) is
5s. Unfortunately, when the difference between adjacent periods is a
substantial fraction of the period itself, whatever the user requested
goes out the window if they don't calculate their duty cycle with a
period which can be represented exactly.

But the above specification would necessitate implementations like

	if (period < 15ULL * NSEC_PER_SEC)
		period = min(period, 10ULL * NSEC_PER_SEC);
	X = 10ULL * NSEC_PER_SEC / period;

which is arbitrary and requires extra instructions to enforce. So I
would prefer this clause as originally stated, since it makes for easier
implementation. And I suspect that the number of users hitting this edge
case is fairly slim.

>> >   b) If a consumer asked for a certain state and gets back -ENORANGE they
>> >      don't know if they should increase or decrease the period to guess a
>> >      state that might be implementable instead.
>>
>> Because I believe this is effectively a configuration issue, it should
>> be obvious to the user which direction they need to go. Programmatic
>> users which want to automatically pick a better period would need to use
>> round_state instead.
>
> You only consider consumers with a fixed period. Do you want to
> explicitly configure all possible periods for a a driver that uses ~ 50%
> relative duty cycle but varies period (e.g. the pwm-ir-tx driver)?

 From what I can tell this driver doesn't really change the period very
much (mostly just to change standard). I think if you are using a
fixed-period PWM as a carrier frequency, your PWM had better be very
nearly the frequency it should. I would certainly rather get an error
about how my pwm was 40KHz instead of 36KHz than have the driver merrily
continue on. This driver is a very good candidate for round_state,
because it (should) care about what the period is. Even normal PWM
rounding could cause you to completely miss your target frequency (e.g.
request 38KHz, rounded to 36KHz). Of course, this driver doesn't even
check the result of pwm_config, so all bets are off.

And yes, I admit that if you had (e.g.) a driver with a fixed-period PWM
which you were using for IR, and you applied these rules, then it would
break your setup. But I think it is fairly easy to hold off on
converting fixed-frequency PWMs used for IR until the consumer drivers
were converted to round_state.

> (OK, these drivers could use pwm_round_rate(), but then that argument
> could be applied to all consumers and the result of an unrounded request
> doesn't really matter any more.)
>
>> > (Hmm, or are you only talking about .apply_state and only .round_state
>> > should do if (period < min_period) return -ERANGE; if (period >
>> > max_period) period = max_period;?
>>
>> Yes.
>
> I really want to have .apply_state() and .round_state() to behave
> exactly the same. Everything else I don't want to ask from driver
> authors. I don't believe you can argue enough that I will drop this
> claim.

And yet, round_state does not exist yet. Why should we match apply_state
behavior with something which is not implemented? You've noted how
certain checks can be implemented with round_state, but until then it is
better to raise an error in the driver.

>> > If so, I'd like to have this in the framework, not in each driver.
>> > Then .round_state and .apply_state can be identical which is good for
>> > reducing complexity.)
>>
>> So you would like each PWM driver to have a "max_period" and
>> "min_period" parameter?
>
> No, I don't want this explicitly. What did I write to make you think
> that? With .round_state() these values can be found out though.
>> And what if the clock rate changes?

See discussion above about exclusive get.

>> Otherwise, how do you propose that
>> the framework detect when a requested period is out of range?
>
> I call round_rate() with
>
> 	.period = requested_period
> 	.duty_cycle = requested_period
>
> and if that returns -ERANGE the PWM doesn't support this rate (and
> smaller ones). And a big requested period is never out of range.

And what do you do about e.g. pwm-sifive where the min/max can change at
any time?

--Sean

Patch

diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index f0f9fbdde7dc..89769affe251 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -269,6 +269,6 @@  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
 obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
 obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
 
-ifneq ($(CONFIG_XILINX_TIMER),)
+ifneq ($(CONFIG_PWM_XILINX)$(CONFIG_XILINX_TIMER),)
 obj-y				+= xilinx-timer.o
 endif
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8ae68d6203fb..ebf8d9014758 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -620,4 +620,16 @@  config PWM_VT8500
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-vt8500.
 
+config PWM_XILINX
+	tristate "Xilinx AXI Timer PWM support"
+	depends on HAS_IOMEM && COMMON_CLK
+	help
+	  PWM driver for Xilinx LogiCORE IP AXI timers. This timer is
+	  typically a soft core which may be present in Xilinx FPGAs.
+	  This device may also be present in Microblaze soft processors.
+	  If you don't have this IP in your design, choose N.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-xilinx.
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index d43b1e17e8e1..655df169b895 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -58,3 +58,4 @@  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
 obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
 obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
+obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c
new file mode 100644
index 000000000000..f05321496717
--- /dev/null
+++ b/drivers/pwm/pwm-xilinx.c
@@ -0,0 +1,219 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2021 Sean Anderson <[e-mail]>
+ *
+ * Hardware limitations:
+ * - When changing both duty cycle and period, we may end up with one cycle
+ *   with the old duty cycle and the new period.
+ * - Cannot produce 100% duty cycle.
+ * - Only produces "normal" output.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/mfd/xilinx-timer.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+/*
+ * The idea here is to capture whether the PWM is actually running (e.g.
+ * because we or the bootloader set it up) and we need to be careful to ensure
+ * we don't cause a glitch. According to the data sheet, to enable the PWM we
+ * need to
+ *
+ * - Set both timers to generate mode (MDT=1)
+ * - Set both timers to PWM mode (PWMA=1)
+ * - Enable the generate out signals (GENT=1)
+ *
+ * In addition,
+ *
+ * - The timer must be running (ENT=1)
+ * - The timer must auto-reload TLR into TCR (ARHT=1)
+ * - We must not be in the process of loading TLR into TCR (LOAD=0)
+ * - Cascade mode must be disabled (CASC=0)
+ *
+ * If any of these differ from usual, then the PWM is either disabled, or is
+ * running in a mode that this driver does not support.
+ */
+#define TCSR_PWM_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)
+#define TCSR_PWM_CLEAR (TCSR_MDT | TCSR_LOAD)
+#define TCSR_PWM_MASK (TCSR_PWM_SET | TCSR_PWM_CLEAR)
+
+struct xilinx_pwm_device {
+	struct pwm_chip chip;
+	struct xilinx_timer_priv priv;
+};
+
+static inline struct xilinx_timer_priv
+*xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
+{
+	return &container_of(chip, struct xilinx_pwm_device, chip)->priv;
+}
+
+static bool xilinx_timer_pwm_enabled(u32 tcsr0, u32 tcsr1)
+{
+	return ((TCSR_PWM_MASK | TCSR_CASC) & tcsr0) == TCSR_PWM_SET &&
+		(TCSR_PWM_MASK & tcsr1) == TCSR_PWM_SET;
+}
+
+static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
+			    const struct pwm_state *state)
+{
+	int ret;
+	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
+	u32 tlr0, tlr1;
+	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
+	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
+	bool enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
+	if (ret)
+		return ret;
+
+	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
+	if (ret)
+		return ret;
+
+	xilinx_timer_write(priv, tlr0, TLR0);
+	xilinx_timer_write(priv, tlr1, TLR1);
+
+	if (state->enabled) {
+		/* Only touch the TCSRs if we aren't already running */
+		if (!enabled) {
+			/* Load TLR into TCR */
+			xilinx_timer_write(priv, tcsr0 | TCSR_LOAD, TCSR0);
+			xilinx_timer_write(priv, tcsr1 | TCSR_LOAD, TCSR1);
+			/* Enable timers all at once with ENALL */
+			tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
+			tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
+			xilinx_timer_write(priv, tcsr0, TCSR0);
+			xilinx_timer_write(priv, tcsr1, TCSR1);
+		}
+	} else {
+		xilinx_timer_write(priv, 0, TCSR0);
+		xilinx_timer_write(priv, 0, TCSR1);
+	}
+
+	return 0;
+}
+
+static void xilinx_pwm_get_state(struct pwm_chip *chip,
+				 struct pwm_device *unused,
+				 struct pwm_state *state)
+{
+	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
+	u32 tlr0 = xilinx_timer_read(priv, TLR0);
+	u32 tlr1 = xilinx_timer_read(priv, TLR1);
+	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
+	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
+
+	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
+	state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
+	state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
+	state->polarity = PWM_POLARITY_NORMAL;
+}
+
+static const struct pwm_ops xilinx_pwm_ops = {
+	.apply = xilinx_pwm_apply,
+	.get_state = xilinx_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int xilinx_timer_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct xilinx_timer_priv *priv;
+	struct xilinx_pwm_device *pwm;
+	u32 pwm_cells, one_timer;
+
+	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
+	if (ret == -EINVAL)
+		return -ENODEV;
+	else if (ret)
+		return dev_err_probe(dev, ret, "#pwm-cells\n");
+	else if (pwm_cells)
+		return dev_err_probe(dev, -EINVAL, "#pwm-cells must be 0\n");
+
+	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, pwm);
+	priv = &pwm->priv;
+
+	priv->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->regs))
+		return PTR_ERR(priv->regs);
+
+	ret = xilinx_timer_common_init(np, priv, &one_timer);
+	if (ret)
+		return ret;
+
+	if (one_timer)
+		return dev_err_probe(dev, -EINVAL,
+				     "two timers required for PWM mode\n");
+
+	/*
+	 * The polarity of the generate outputs must be active high for PWM
+	 * mode to work. We could determine this from the device tree, but
+	 * alas, such properties are not allowed to be used.
+	 */
+
+	priv->clk = devm_clk_get(dev, "s_axi_aclk");
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk), "clock\n");
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "clock enable failed\n");
+	clk_rate_exclusive_get(priv->clk);
+
+	pwm->chip.dev = dev;
+	pwm->chip.ops = &xilinx_pwm_ops;
+	pwm->chip.npwm = 1;
+	ret = pwmchip_add(&pwm->chip);
+	if (ret) {
+		clk_rate_exclusive_put(priv->clk);
+		clk_disable_unprepare(priv->clk);
+		return dev_err_probe(dev, ret, "could not register pwm chip\n");
+	}
+
+	return 0;
+}
+
+static int xilinx_timer_remove(struct platform_device *pdev)
+{
+	struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&pwm->chip);
+	clk_rate_exclusive_put(pwm->priv.clk);
+	clk_disable_unprepare(pwm->priv.clk);
+	return 0;
+}
+
+static const struct of_device_id xilinx_timer_of_match[] = {
+	{ .compatible = "xlnx,xps-timer-1.00.a", },
+	{ .compatible = "xlnx,axi-timer-2.0" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xilinx_timer_of_match);
+
+static struct platform_driver xilinx_timer_driver = {
+	.probe = xilinx_timer_probe,
+	.remove = xilinx_timer_remove,
+	.driver = {
+		.name = "xilinx-timer",
+		.of_match_table = of_match_ptr(xilinx_timer_of_match),
+	},
+};
+module_platform_driver(xilinx_timer_driver);
+
+MODULE_ALIAS("platform:xilinx-timer");
+MODULE_DESCRIPTION("Xilinx LogiCORE IP AXI Timer driver");
+MODULE_LICENSE("GPL v2");