[PATCH v3 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[PATCH v3 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[Baolin Wang]

Add Spreadtrum PWM controller documentation.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes from v2:
 - Fix some typos.
 - Move assigned-clocks to be optional.

Changes from v1:
 - Use assigned-clock-parents and assigned-clocks to set PWM clock parent.
---
 Documentation/devicetree/bindings/pwm/pwm-sprd.txt |   40 ++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
new file mode 100644
index 0000000..16fa5a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
@@ -0,0 +1,40 @@
+Spreadtrum PWM controller
+
+Spreadtrum SoCs PWM controller provides 4 PWM channels.
+
+Required properties:
+- compatible : Should be "sprd,ums512-pwm".
+- reg: Physical base address and length of the controller's registers.
+- clocks: The phandle and specifier referencing the controller's clocks.
+- clock-names: Should contain following entries:
+  "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3).
+  "enablen": for PWM channel n enable clock (n range: 0 ~ 3).
+- #pwm-cells: Should be 2. See pwm.txt in this directory for a description of
+  the cells format.
+
+Optional properties:
+- assigned-clocks: Reference to the PWM clock entries.
+- assigned-clock-parents: The phandle of the parent clock of PWM clock.
+
+Example:
+	pwms: pwm@32260000 {
+		compatible = "sprd,ums512-pwm";
+		reg = <0 0x32260000 0 0x10000>;
+		clock-names = "pwm0", "enable0",
+			"pwm1", "enable1",
+			"pwm2", "enable2",
+			"pwm3", "enable3";
+		clocks = <&aon_clk CLK_PWM0>, <&aonapb_gate CLK_PWM0_EB>,
+		       <&aon_clk CLK_PWM1>, <&aonapb_gate CLK_PWM1_EB>,
+		       <&aon_clk CLK_PWM2>, <&aonapb_gate CLK_PWM2_EB>,
+		       <&aon_clk CLK_PWM3>, <&aonapb_gate CLK_PWM3_EB>;
+		assigned-clocks = <&aon_clk CLK_PWM0>,
+			<&aon_clk CLK_PWM1>,
+			<&aon_clk CLK_PWM2>,
+			<&aon_clk CLK_PWM3>;
+		assigned-clock-parents = <&ext_26m>,
+			<&ext_26m>,
+			<&ext_26m>,
+			<&ext_26m>;
+		#pwm-cells = <2>;
+	};
-- 
1.7.9.5

[PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support

[Baolin Wang]

This patch adds the Spreadtrum PWM support, which provides maximum 4
channels.

Signed-off-by: Neo Hou <neo.hou@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes from v2:
 - Optimize some macors' name.
 - Improve some comments.
 - Optimize the calculation of prescale.
 - Do not print error log if error numeber is EPROBE_DEFER.
 - Return -ENODEV if no available PWM channels.
 - Simplify the logics in sprd_pwm_clk_init().
 - Remove disabling PWM clocks in sprd_pwm_remove().
 - Remove pwm_get_state().

Changes from v1:
 - Add depending on HAS_IOMEM.
 - Rename parameters' names.
 - Implement .apply() instead of .config(), .enable() and .disable().
 - Use NSEC_PER_SEC instead of 1000000000ULL.
 - Add some comments to make code more readable.
 - Remove some redundant operation.
 - Use standard clock properties to set clock parent.
 - Other coding style optimization.
---
 drivers/pwm/Kconfig    |   11 ++
 drivers/pwm/Makefile   |    1 +
 drivers/pwm/pwm-sprd.c |  309 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 321 insertions(+)
 create mode 100644 drivers/pwm/pwm-sprd.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index a7e5751..31dfc88 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -423,6 +423,17 @@ config PWM_SPEAR
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-spear.
 
+config PWM_SPRD
+	tristate "Spreadtrum PWM support"
+	depends on ARCH_SPRD || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  Generic PWM framework driver for the PWM controller on
+	  Spreadtrum SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-sprd.
+
 config PWM_STI
 	tristate "STiH4xx PWM support"
 	depends on ARCH_STI
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 76b555b..26326ad 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
+obj-$(CONFIG_PWM_SPRD)		+= pwm-sprd.o
 obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
 obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
 obj-$(CONFIG_PWM_STM32_LP)	+= pwm-stm32-lp.o
diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
new file mode 100644
index 0000000..68c2d9f
--- /dev/null
+++ b/drivers/pwm/pwm-sprd.c
@@ -0,0 +1,309 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Spreadtrum Communications Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+#define SPRD_PWM_PRESCALE	0x0
+#define SPRD_PWM_MOD		0x4
+#define SPRD_PWM_DUTY		0x8
+#define SPRD_PWM_ENABLE		0x18
+
+#define SPRD_PWM_MOD_MAX	GENMASK(7, 0)
+#define SPRD_PWM_DUTY_MSK	GENMASK(15, 0)
+#define SPRD_PWM_PRESCALE_MSK	GENMASK(7, 0)
+#define SPRD_PWM_ENABLE_BIT	BIT(0)
+
+#define SPRD_PWM_CHN_NUM	4
+#define SPRD_PWM_REGS_SHIFT	5
+#define SPRD_PWM_CHN_CLKS_NUM	2
+#define SPRD_PWM_CHN_OUTPUT_CLK	1
+
+struct sprd_pwm_chn {
+	struct clk_bulk_data clks[SPRD_PWM_CHN_CLKS_NUM];
+	u32 clk_rate;
+};
+
+struct sprd_pwm_chip {
+	void __iomem *base;
+	struct device *dev;
+	struct pwm_chip chip;
+	int num_pwms;
+	struct sprd_pwm_chn chn[SPRD_PWM_CHN_NUM];
+};
+
+/*
+ * The list of clocks required by PWM channels, and each channel has 2 clocks:
+ * enable clock and pwm clock.
+ */
+static const char * const sprd_pwm_clks[] = {
+	"enable0", "pwm0",
+	"enable1", "pwm1",
+	"enable2", "pwm2",
+	"enable3", "pwm3",
+};
+
+static u32 sprd_pwm_read(struct sprd_pwm_chip *spc, u32 hwid, u32 reg)
+{
+	u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
+
+	return readl_relaxed(spc->base + offset);
+}
+
+static void sprd_pwm_write(struct sprd_pwm_chip *spc, u32 hwid,
+			   u32 reg, u32 val)
+{
+	u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
+
+	writel_relaxed(val, spc->base + offset);
+}
+
+static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			       struct pwm_state *state)
+{
+	struct sprd_pwm_chip *spc =
+		container_of(chip, struct sprd_pwm_chip, chip);
+	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
+	u32 val, duty, prescale;
+	u64 tmp;
+	int ret;
+
+	/*
+	 * The clocks to PWM channel has to be enabled first before
+	 * reading to the registers.
+	 */
+	ret = clk_bulk_prepare_enable(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
+	if (ret) {
+		dev_err(spc->dev, "failed to enable pwm%u clocks\n",
+			pwm->hwpwm);
+		return;
+	}
+
+	val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE);
+	if (val & SPRD_PWM_ENABLE_BIT)
+		state->enabled = true;
+	else
+		state->enabled = false;
+
+	/*
+	 * The hardware provides a counter that is feed by the source clock.
+	 * The period length is (PRESCALE + 1) * MOD counter steps.
+	 * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
+	 * Thus the period_ns and duty_ns calculation formula should be:
+	 * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
+	 * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
+	 */
+	val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
+	prescale = val & SPRD_PWM_PRESCALE_MSK;
+	tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
+	state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
+
+	val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
+	duty = val & SPRD_PWM_DUTY_MSK;
+	tmp = (prescale + 1) * NSEC_PER_SEC * duty;
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
+
+	/* Disable PWM clocks if the PWM channel is not in enable state. */
+	if (!state->enabled)
+		clk_bulk_disable_unprepare(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
+}
+
+static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
+			   int duty_ns, int period_ns)
+{
+	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
+	u32 prescale, duty;
+	u64 tmp;
+
+	/*
+	 * The hardware provides a counter that is feed by the source clock.
+	 * The period length is (PRESCALE + 1) * MOD counter steps.
+	 * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
+	 *
+	 * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
+	 * The value for PRESCALE is selected such that the resulting period
+	 * gets the maximal length not bigger than the requested one with the
+	 * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
+	 */
+	duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
+
+	tmp = (u64)chn->clk_rate * period_ns;
+	do_div(tmp, NSEC_PER_SEC);
+	prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
+	if (prescale > SPRD_PWM_PRESCALE_MSK)
+		prescale = SPRD_PWM_PRESCALE_MSK;
+
+	/*
+	 * Note: Writing DUTY triggers the hardware to actually apply the
+	 * values written to MOD and DUTY to the output, so must keep writing
+	 * DUTY last.
+	 *
+	 * The hardware can ensures that current running period is completed
+	 * before changing a new configuration to avoid mixed settings.
+	 */
+	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
+	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
+	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
+
+	return 0;
+}
+
+static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			  struct pwm_state *state)
+{
+	struct sprd_pwm_chip *spc =
+		container_of(chip, struct sprd_pwm_chip, chip);
+	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
+	struct pwm_state *cstate = &pwm->state;
+	int ret;
+
+	if (state->enabled) {
+		if (!cstate->enabled) {
+			/*
+			 * The clocks to PWM channel has to be enabled first
+			 * before writing to the registers.
+			 */
+			ret = clk_bulk_prepare_enable(SPRD_PWM_CHN_CLKS_NUM,
+						      chn->clks);
+			if (ret) {
+				dev_err(spc->dev,
+					"failed to enable pwm%u clocks\n",
+					pwm->hwpwm);
+				return ret;
+			}
+		}
+
+		if (state->period != cstate->period ||
+		    state->duty_cycle != cstate->duty_cycle) {
+			ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
+					      state->period);
+			if (ret)
+				return ret;
+		}
+
+		sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
+	} else if (cstate->enabled) {
+		/*
+		 * Note: After setting SPRD_PWM_ENABLE to zero, the controller
+		 * will not wait for current period to be completed, instead it
+		 * will stop the PWM channel immediately.
+		 */
+		sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0);
+
+		clk_bulk_disable_unprepare(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
+	}
+
+	return 0;
+}
+
+static const struct pwm_ops sprd_pwm_ops = {
+	.apply = sprd_pwm_apply,
+	.get_state = sprd_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
+{
+	struct clk *clk_pwm;
+	int ret, i;
+
+	for (i = 0; i < SPRD_PWM_CHN_NUM; i++) {
+		struct sprd_pwm_chn *chn = &spc->chn[i];
+		int j;
+
+		for (j = 0; j < SPRD_PWM_CHN_CLKS_NUM; ++j)
+			chn->clks[j].id =
+				sprd_pwm_clks[i * SPRD_PWM_CHN_CLKS_NUM + j];
+
+		ret = devm_clk_bulk_get(spc->dev, SPRD_PWM_CHN_CLKS_NUM,
+					chn->clks);
+		if (ret) {
+			if (ret == -ENOENT)
+				break;
+
+			if (ret != -EPROBE_DEFER)
+				dev_err(spc->dev,
+					"failed to get channel clocks\n");
+
+			return ret;
+		}
+
+		clk_pwm = chn->clks[SPRD_PWM_CHN_OUTPUT_CLK].clk;
+		chn->clk_rate = clk_get_rate(clk_pwm);
+	}
+
+	if (!i) {
+		dev_err(spc->dev, "no available PWM channels\n");
+		return -ENODEV;
+	}
+
+	spc->num_pwms = i;
+
+	return 0;
+}
+
+static int sprd_pwm_probe(struct platform_device *pdev)
+{
+	struct sprd_pwm_chip *spc;
+	int ret;
+
+	spc = devm_kzalloc(&pdev->dev, sizeof(*spc), GFP_KERNEL);
+	if (!spc)
+		return -ENOMEM;
+
+	spc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(spc->base))
+		return PTR_ERR(spc->base);
+
+	spc->dev = &pdev->dev;
+	platform_set_drvdata(pdev, spc);
+
+	ret = sprd_pwm_clk_init(spc);
+	if (ret)
+		return ret;
+
+	spc->chip.dev = &pdev->dev;
+	spc->chip.ops = &sprd_pwm_ops;
+	spc->chip.base = -1;
+	spc->chip.npwm = spc->num_pwms;
+
+	ret = pwmchip_add(&spc->chip);
+	if (ret)
+		dev_err(&pdev->dev, "failed to add PWM chip\n");
+
+	return ret;
+}
+
+static int sprd_pwm_remove(struct platform_device *pdev)
+{
+	struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&spc->chip);
+}
+
+static const struct of_device_id sprd_pwm_of_match[] = {
+	{ .compatible = "sprd,ums512-pwm", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, sprd_pwm_of_match);
+
+static struct platform_driver sprd_pwm_driver = {
+	.driver = {
+		.name = "sprd-pwm",
+		.of_match_table = sprd_pwm_of_match,
+	},
+	.probe = sprd_pwm_probe,
+	.remove = sprd_pwm_remove,
+};
+
+module_platform_driver(sprd_pwm_driver);
+
+MODULE_DESCRIPTION("Spreadtrum PWM Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support

[Uwe Kleine-König]

On Wed, Aug 14, 2019 at 08:46:11PM +0800, Baolin Wang wrote:
> This patch adds the Spreadtrum PWM support, which provides maximum 4
> channels.
> 
> Signed-off-by: Neo Hou <neo.hou@unisoc.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v2:
>  - Optimize some macors' name.
>  - Improve some comments.
>  - Optimize the calculation of prescale.
>  - Do not print error log if error numeber is EPROBE_DEFER.
>  - Return -ENODEV if no available PWM channels.
>  - Simplify the logics in sprd_pwm_clk_init().
>  - Remove disabling PWM clocks in sprd_pwm_remove().
>  - Remove pwm_get_state().
> 
> Changes from v1:
>  - Add depending on HAS_IOMEM.
>  - Rename parameters' names.
>  - Implement .apply() instead of .config(), .enable() and .disable().
>  - Use NSEC_PER_SEC instead of 1000000000ULL.
>  - Add some comments to make code more readable.
>  - Remove some redundant operation.
>  - Use standard clock properties to set clock parent.
>  - Other coding style optimization.
> ---
>  drivers/pwm/Kconfig    |   11 ++
>  drivers/pwm/Makefile   |    1 +
>  drivers/pwm/pwm-sprd.c |  309 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 321 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sprd.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a7e5751..31dfc88 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -423,6 +423,17 @@ config PWM_SPEAR
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-spear.
>  
> +config PWM_SPRD
> +	tristate "Spreadtrum PWM support"
> +	depends on ARCH_SPRD || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	help
> +	  Generic PWM framework driver for the PWM controller on
> +	  Spreadtrum SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-sprd.
> +
>  config PWM_STI
>  	tristate "STiH4xx PWM support"
>  	depends on ARCH_STI
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 76b555b..26326ad 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
>  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
> +obj-$(CONFIG_PWM_SPRD)		+= pwm-sprd.o
>  obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
>  obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
>  obj-$(CONFIG_PWM_STM32_LP)	+= pwm-stm32-lp.o
> diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
> new file mode 100644
> index 0000000..68c2d9f
> --- /dev/null
> +++ b/drivers/pwm/pwm-sprd.c
> @@ -0,0 +1,309 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 Spreadtrum Communications Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +#define SPRD_PWM_PRESCALE	0x0
> +#define SPRD_PWM_MOD		0x4
> +#define SPRD_PWM_DUTY		0x8
> +#define SPRD_PWM_ENABLE		0x18
> +
> +#define SPRD_PWM_MOD_MAX	GENMASK(7, 0)
> +#define SPRD_PWM_DUTY_MSK	GENMASK(15, 0)
> +#define SPRD_PWM_PRESCALE_MSK	GENMASK(7, 0)
> +#define SPRD_PWM_ENABLE_BIT	BIT(0)
> +
> +#define SPRD_PWM_CHN_NUM	4
> +#define SPRD_PWM_REGS_SHIFT	5
> +#define SPRD_PWM_CHN_CLKS_NUM	2
> +#define SPRD_PWM_CHN_OUTPUT_CLK	1
> +
> +struct sprd_pwm_chn {
> +	struct clk_bulk_data clks[SPRD_PWM_CHN_CLKS_NUM];
> +	u32 clk_rate;
> +};
> +
> +struct sprd_pwm_chip {
> +	void __iomem *base;
> +	struct device *dev;
> +	struct pwm_chip chip;
> +	int num_pwms;
> +	struct sprd_pwm_chn chn[SPRD_PWM_CHN_NUM];
> +};
> +
> +/*
> + * The list of clocks required by PWM channels, and each channel has 2 clocks:
> + * enable clock and pwm clock.
> + */
> +static const char * const sprd_pwm_clks[] = {
> +	"enable0", "pwm0",
> +	"enable1", "pwm1",
> +	"enable2", "pwm2",
> +	"enable3", "pwm3",
> +};
> +
> +static u32 sprd_pwm_read(struct sprd_pwm_chip *spc, u32 hwid, u32 reg)
> +{
> +	u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
> +
> +	return readl_relaxed(spc->base + offset);
> +}
> +
> +static void sprd_pwm_write(struct sprd_pwm_chip *spc, u32 hwid,
> +			   u32 reg, u32 val)
> +{
> +	u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
> +
> +	writel_relaxed(val, spc->base + offset);
> +}
> +
> +static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +			       struct pwm_state *state)
> +{
> +	struct sprd_pwm_chip *spc =
> +		container_of(chip, struct sprd_pwm_chip, chip);
> +	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> +	u32 val, duty, prescale;
> +	u64 tmp;
> +	int ret;
> +
> +	/*
> +	 * The clocks to PWM channel has to be enabled first before
> +	 * reading to the registers.
> +	 */
> +	ret = clk_bulk_prepare_enable(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
> +	if (ret) {
> +		dev_err(spc->dev, "failed to enable pwm%u clocks\n",
> +			pwm->hwpwm);
> +		return;
> +	}
> +
> +	val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE);
> +	if (val & SPRD_PWM_ENABLE_BIT)
> +		state->enabled = true;
> +	else
> +		state->enabled = false;
> +
> +	/*
> +	 * The hardware provides a counter that is feed by the source clock.
> +	 * The period length is (PRESCALE + 1) * MOD counter steps.
> +	 * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> +	 * Thus the period_ns and duty_ns calculation formula should be:
> +	 * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
> +	 * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
> +	 */
> +	val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> +	prescale = val & SPRD_PWM_PRESCALE_MSK;
> +	tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> +	state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> +
> +	val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> +	duty = val & SPRD_PWM_DUTY_MSK;
> +	tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> +
> +	/* Disable PWM clocks if the PWM channel is not in enable state. */
> +	if (!state->enabled)
> +		clk_bulk_disable_unprepare(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
> +}
> +
> +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> +			   int duty_ns, int period_ns)
> +{
> +	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> +	u32 prescale, duty;
> +	u64 tmp;
> +
> +	/*
> +	 * The hardware provides a counter that is feed by the source clock.
> +	 * The period length is (PRESCALE + 1) * MOD counter steps.
> +	 * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> +	 *
> +	 * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.

Did you spend some thoughts about how wrong your period can get because
of that "lazyness"?

Let's assume a clk rate of 100/3 MHz. Then the available period lengths
are:

	PRESCALE =  0  ->  period =   7.65 µs
	PRESCALE =  1  ->  period =  15.30 µs
	...
	PRESCALE = 17  ->  period = 137.70 µs
	PRESCALE = 18  ->  period = 145.35 µs

So the error can be up to (nearly) 7.65 µs (or in general 
255 / clk_rate) because if 145.34 µs is requested you configure
PRESCALE = 17 and so yield a period of 137.70 µs. If however you'd pick
PRESCALE = 18 and MOD = 254 you get a period of 144.78 µs and so the
error is only 0.56 µs which is a factor of 13 better.

Hmm.

> +	 * The value for PRESCALE is selected such that the resulting period
> +	 * gets the maximal length not bigger than the requested one with the
> +	 * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
> +	 */
> +	duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;

I wonder if you loose some precision here as you use period_ns but might
actually implement a shorter period.

Quick example, again consider clk_rate = 100 / 3 MHz,
period_ns = 145340, duty_ns = 72670. Then you end up with

	PRESCALE = 17
	MOD = 255
	DUTY = 127

That corresponds to period_ns = 137700, duty_ns = 68580. With DUTY = 134
you get 72360 ns which is still smaller than the requested 72670 ns.
(But then again it is not obvious which of the two is the "better"
approximation because Thierry doesn't seem to see the necessity to
discuss or define a policy here.)

(And to pick up the thoughts about not using SPRD_PWM_MOD_MAX
unconditionally, you could also use

	PRESCALE = 18
	MOD = 254
	DUTY = 127

yielding period_ns = 144780 and duty_ns = 72390. Summary:

	Request:                 72670 / 145340
	your result:             68580 / 137700
	also possible:           72390 / 144780

Judge yourself.)

> +	tmp = (u64)chn->clk_rate * period_ns;
> +	do_div(tmp, NSEC_PER_SEC);
> +	prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;

Now that you use DIV_ROUND_CLOSEST_ULL the comment is wrong because you
might provide a period bigger than the requested one. Also you divide
twice instead of once before. (I don't know what architecture your SoC
uses, but compared to a multiplication a division is usually expensive.)
Also the math is more complicated now as you have a round-down div and a
round-closest div.

My preference for how to fix that is to restore the behaviour of v2 that
matches the comment and adapt .get_state() instead.

For .get_state() this should then be:

	val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
	prescale = FIELD_GET(SPRD_PWM_PRESCALE_MSK, val);

	tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
	state->period = DIV_ROUND_UP_ULL(tmp, chn->clk_rate);

> +	if (prescale > SPRD_PWM_PRESCALE_MSK)
> +		prescale = SPRD_PWM_PRESCALE_MSK;
> +
> +	/*
> +	 * Note: Writing DUTY triggers the hardware to actually apply the
> +	 * values written to MOD and DUTY to the output, so must keep writing
> +	 * DUTY last.
> +	 *
> +	 * The hardware can ensures that current running period is completed
> +	 * before changing a new configuration to avoid mixed settings.

I'm not a native English speaker, but I would write:

	* The hardware ensures that currently running period is
	* completed before changing to the new configuration to avoid
	* mixed settings.

Does this mechanism also apply to PRESCALE? If yes, please include it in
your description. If not there is still a possibility for a wrong
period.

> +	 */
> +	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
> +	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> +	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
> +
> +	return 0;
> +}

Best regards
Uwe

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

Re: [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support

[Baolin Wang]

Hi Uwe,

On Wed, 14 Aug 2019 at 23:03, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Wed, Aug 14, 2019 at 08:46:11PM +0800, Baolin Wang wrote:

> > +
> > +     /*
> > +      * The hardware provides a counter that is feed by the source clock.
> > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > +      * Thus the period_ns and duty_ns calculation formula should be:
> > +      * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
> > +      * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
> > +      */
> > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> > +     prescale = val & SPRD_PWM_PRESCALE_MSK;
> > +     tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > +     state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > +
> > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> > +     duty = val & SPRD_PWM_DUTY_MSK;
> > +     tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> > +     state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > +
> > +     /* Disable PWM clocks if the PWM channel is not in enable state. */
> > +     if (!state->enabled)
> > +             clk_bulk_disable_unprepare(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
> > +}
> > +
> > +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> > +                        int duty_ns, int period_ns)
> > +{
> > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > +     u32 prescale, duty;
> > +     u64 tmp;
> > +
> > +     /*
> > +      * The hardware provides a counter that is feed by the source clock.
> > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > +      *
> > +      * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
>
> Did you spend some thoughts about how wrong your period can get because
> of that "lazyness"?
>
> Let's assume a clk rate of 100/3 MHz. Then the available period lengths
> are:
>
>         PRESCALE =  0  ->  period =   7.65 µs
>         PRESCALE =  1  ->  period =  15.30 µs
>         ...
>         PRESCALE = 17  ->  period = 137.70 µs
>         PRESCALE = 18  ->  period = 145.35 µs
>
> So the error can be up to (nearly) 7.65 µs (or in general

Yes, but for our use case (pwm backlight), the precision can meet our
requirement. Moreover, we usually do not change the period, just
adjust the duty to change the back light.

> 255 / clk_rate) because if 145.34 µs is requested you configure
> PRESCALE = 17 and so yield a period of 137.70 µs. If however you'd pick

I did not get you here, if period is 145.34, we still get the
corresponding PRESCALE = 18 by below formula:

tmp = (u64)chn->clk_rate * period_ns;
do_div(tmp, NSEC_PER_SEC);
prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;

> PRESCALE = 18 and MOD = 254 you get a period of 144.78 µs and so the
> error is only 0.56 µs which is a factor of 13 better.
>
> Hmm.
>
> > +      * The value for PRESCALE is selected such that the resulting period
> > +      * gets the maximal length not bigger than the requested one with the
> > +      * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
> > +      */
> > +     duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
>
> I wonder if you loose some precision here as you use period_ns but might
> actually implement a shorter period.
>
> Quick example, again consider clk_rate = 100 / 3 MHz,
> period_ns = 145340, duty_ns = 72670. Then you end up with
>
>         PRESCALE = 17
>         MOD = 255
>         DUTY = 127

Incorrect, we will get PRESCALE = 18,  MOD = 255, DUTY = 127.

> That corresponds to period_ns = 137700, duty_ns = 68580. With DUTY = 134
> you get 72360 ns which is still smaller than the requested 72670 ns.

Incorrect, with DUTY = 134 (PRESCALE = 18  ->  period = 145.35 µs),
duty_ns = 76380ns

> (But then again it is not obvious which of the two is the "better"
> approximation because Thierry doesn't seem to see the necessity to
> discuss or define a policy here.)

Like I said, this is the simple calculation formula which can meet our
requirement (we limit our DUTY value can only be 0 - 254).
duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;

>
> (And to pick up the thoughts about not using SPRD_PWM_MOD_MAX
> unconditionally, you could also use
>
>         PRESCALE = 18
>         MOD = 254
>         DUTY = 127
>
> yielding period_ns = 144780 and duty_ns = 72390. Summary:
>
>         Request:                 72670 / 145340
>         your result:             68580 / 137700
>         also possible:           72390 / 144780
>
> Judge yourself.)
>
> > +     tmp = (u64)chn->clk_rate * period_ns;
> > +     do_div(tmp, NSEC_PER_SEC);
> > +     prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
>
> Now that you use DIV_ROUND_CLOSEST_ULL the comment is wrong because you
> might provide a period bigger than the requested one. Also you divide

Again, that's the precision can meet our requirement.

> twice instead of once before. (I don't know what architecture your SoC
> uses, but compared to a multiplication a division is usually expensive.)
> Also the math is more complicated now as you have a round-down div and a
> round-closest div.
>
> My preference for how to fix that is to restore the behaviour of v2 that
> matches the comment and adapt .get_state() instead.

Using DIV_ROUND_CLOSEST_ULL can get a same prescale which matches with
.get_state().

>
> For .get_state() this should then be:
>
>         val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
>         prescale = FIELD_GET(SPRD_PWM_PRESCALE_MSK, val);
>
>         tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
>         state->period = DIV_ROUND_UP_ULL(tmp, chn->clk_rate);
>
> > +     if (prescale > SPRD_PWM_PRESCALE_MSK)
> > +             prescale = SPRD_PWM_PRESCALE_MSK;
> > +
> > +     /*
> > +      * Note: Writing DUTY triggers the hardware to actually apply the
> > +      * values written to MOD and DUTY to the output, so must keep writing
> > +      * DUTY last.
> > +      *
> > +      * The hardware can ensures that current running period is completed
> > +      * before changing a new configuration to avoid mixed settings.
>
> I'm not a native English speaker, but I would write:
>
>         * The hardware ensures that currently running period is
>         * completed before changing to the new configuration to avoid
>         * mixed settings.

Sure

>
> Does this mechanism also apply to PRESCALE? If yes, please include it in

Yes.

> your description. If not there is still a possibility for a wrong
> period.
>
> > +      */
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
> > +
> > +     return 0;
> > +}
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |



-- 
Baolin Wang
Best Regards

Re: [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support

[Uwe Kleine-König]

Hello Baolin,

On Thu, Aug 15, 2019 at 11:34:27AM +0800, Baolin Wang wrote:
> On Wed, 14 Aug 2019 at 23:03, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > On Wed, Aug 14, 2019 at 08:46:11PM +0800, Baolin Wang wrote:
> 
> > > +
> > > +     /*
> > > +      * The hardware provides a counter that is feed by the source clock.
> > > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > > +      * Thus the period_ns and duty_ns calculation formula should be:
> > > +      * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
> > > +      * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
> > > +      */
> > > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> > > +     prescale = val & SPRD_PWM_PRESCALE_MSK;
> > > +     tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > > +     state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > +
> > > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> > > +     duty = val & SPRD_PWM_DUTY_MSK;
> > > +     tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> > > +     state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > +
> > > +     /* Disable PWM clocks if the PWM channel is not in enable state. */
> > > +     if (!state->enabled)
> > > +             clk_bulk_disable_unprepare(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
> > > +}
> > > +
> > > +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> > > +                        int duty_ns, int period_ns)
> > > +{
> > > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > +     u32 prescale, duty;
> > > +     u64 tmp;
> > > +
> > > +     /*
> > > +      * The hardware provides a counter that is feed by the source clock.
> > > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > > +      *
> > > +      * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
> >
> > Did you spend some thoughts about how wrong your period can get because
> > of that "lazyness"?
> >
> > Let's assume a clk rate of 100/3 MHz. Then the available period lengths
> > are:
> >
> >         PRESCALE =  0  ->  period =   7.65 µs
> >         PRESCALE =  1  ->  period =  15.30 µs
> >         ...
> >         PRESCALE = 17  ->  period = 137.70 µs
> >         PRESCALE = 18  ->  period = 145.35 µs
> >
> > So the error can be up to (nearly) 7.65 µs (or in general
> 
> Yes, but for our use case (pwm backlight), the precision can meet our
> requirement. Moreover, we usually do not change the period, just
> adjust the duty to change the back light.

Is this a license requirement for you SoC to only drive a backlight with
the PWM? The idea of having a PWM driver on your platform is that it can
also be used to control a step motor or a laser.

> > 255 / clk_rate) because if 145.34 µs is requested you configure
> > PRESCALE = 17 and so yield a period of 137.70 µs. If however you'd pick
> 
> I did not get you here, if period is 145.34, we still get the
> corresponding PRESCALE = 18 by below formula:
> 
> tmp = (u64)chn->clk_rate * period_ns;
> do_div(tmp, NSEC_PER_SEC);
> prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;

I assumed you switch back to rounding down to match your comment and
which is how I think a pwm should behave. With rounding down we get
PRESCALE = 17 as I claimed. 

> > PRESCALE = 18 and MOD = 254 you get a period of 144.78 µs and so the
> > error is only 0.56 µs which is a factor of 13 better.
> >
> > Hmm.
> >
> > > +      * The value for PRESCALE is selected such that the resulting period
> > > +      * gets the maximal length not bigger than the requested one with the
> > > +      * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
> > > +      */
> > > +     duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> >
> > I wonder if you loose some precision here as you use period_ns but might
> > actually implement a shorter period.
> >
> > Quick example, again consider clk_rate = 100 / 3 MHz,
> > period_ns = 145340, duty_ns = 72670. Then you end up with
> >
> >         PRESCALE = 17
> >         MOD = 255
> >         DUTY = 127
> 
> Incorrect, we will get PRESCALE = 18,  MOD = 255, DUTY = 127.
> 
> > That corresponds to period_ns = 137700, duty_ns = 68580. With DUTY = 134
> > you get 72360 ns which is still smaller than the requested 72670 ns.
> 
> Incorrect, with DUTY = 134 (PRESCALE = 18  ->  period = 145.35 µs),
> duty_ns = 76380ns

Yes, as above. When using rounding-closest your error is not in [0, 7.65
µs] but in [-3.825 µs, 3.825 µs]. Doesn't make it better.
 
> > (But then again it is not obvious which of the two is the "better"
> > approximation because Thierry doesn't seem to see the necessity to
> > discuss or define a policy here.)
> 
> Like I said, this is the simple calculation formula which can meet our
> requirement (we limit our DUTY value can only be 0 - 254).
> duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;

simple is often good but sometimes different from correct. And even with
rounding closest instead of rounding down you're giving away precision
here and the size of the error interval is the same, it is just centered
around 0 instead of only positive. If I hadn't spend so much time with
pwm reviews this week I'd try to come up with an example.

> > (And to pick up the thoughts about not using SPRD_PWM_MOD_MAX
> > unconditionally, you could also use
> >
> >         PRESCALE = 18
> >         MOD = 254
> >         DUTY = 127
> >
> > yielding period_ns = 144780 and duty_ns = 72390. Summary:
> >
> >         Request:                 72670 / 145340
> >         your result:             68580 / 137700
> >         also possible:           72390 / 144780
> >
> > Judge yourself.)
> >
> > > +     tmp = (u64)chn->clk_rate * period_ns;
> > > +     do_div(tmp, NSEC_PER_SEC);
> > > +     prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
> >
> > Now that you use DIV_ROUND_CLOSEST_ULL the comment is wrong because you
> > might provide a period bigger than the requested one. Also you divide
> 
> Again, that's the precision can meet our requirement.

If you go back to rounding down, use the matching rounding up in
.get_state and adapt your comment describing you're sticking to MOD=255
to say explicitly that you're loosing precision I can live with that. I
even did the math for .get_state in my previous mail for you.

The idea of the requirement to round down is that I want to introduce
this as policy for the PWM framework to get some uniform behaviour from
all lowlevel drivers. If you do this now I won't bother you later when
the requirement is implemented in your driver. And the comment helps
someone who evaluates your SoC to judge if there is still work to do if
they have higher requirements for the PWM.

> > twice instead of once before. (I don't know what architecture your SoC
> > uses, but compared to a multiplication a division is usually expensive.)
> > Also the math is more complicated now as you have a round-down div and a
> > round-closest div.
> >
> > My preference for how to fix that is to restore the behaviour of v2 that
> > matches the comment and adapt .get_state() instead.
> 
> Using DIV_ROUND_CLOSEST_ULL can get a same prescale which matches with
> .get_state().

I don't get you here. Do you say that with DIV_ROUND_CLOSEST_ULL you get
the same result but DIV_ROUND_CLOSEST_ULL matches .get_state while
rounding down doesn't? I cannot follow.

Best regards
Uwe

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

Re: [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support

[Baolin Wang]

Hi Uwe,

On Thu, 15 Aug 2019 at 14:15, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Baolin,
>
> On Thu, Aug 15, 2019 at 11:34:27AM +0800, Baolin Wang wrote:
> > On Wed, 14 Aug 2019 at 23:03, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > On Wed, Aug 14, 2019 at 08:46:11PM +0800, Baolin Wang wrote:
> >
> > > > +
> > > > +     /*
> > > > +      * The hardware provides a counter that is feed by the source clock.
> > > > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > > > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > > > +      * Thus the period_ns and duty_ns calculation formula should be:
> > > > +      * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
> > > > +      * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
> > > > +      */
> > > > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> > > > +     prescale = val & SPRD_PWM_PRESCALE_MSK;
> > > > +     tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > > > +     state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > > +
> > > > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> > > > +     duty = val & SPRD_PWM_DUTY_MSK;
> > > > +     tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> > > > +     state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > > +
> > > > +     /* Disable PWM clocks if the PWM channel is not in enable state. */
> > > > +     if (!state->enabled)
> > > > +             clk_bulk_disable_unprepare(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
> > > > +}
> > > > +
> > > > +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> > > > +                        int duty_ns, int period_ns)
> > > > +{
> > > > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > > +     u32 prescale, duty;
> > > > +     u64 tmp;
> > > > +
> > > > +     /*
> > > > +      * The hardware provides a counter that is feed by the source clock.
> > > > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > > > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > > > +      *
> > > > +      * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
> > >
> > > Did you spend some thoughts about how wrong your period can get because
> > > of that "lazyness"?
> > >
> > > Let's assume a clk rate of 100/3 MHz. Then the available period lengths
> > > are:
> > >
> > >         PRESCALE =  0  ->  period =   7.65 µs
> > >         PRESCALE =  1  ->  period =  15.30 µs
> > >         ...
> > >         PRESCALE = 17  ->  period = 137.70 µs
> > >         PRESCALE = 18  ->  period = 145.35 µs
> > >
> > > So the error can be up to (nearly) 7.65 µs (or in general
> >
> > Yes, but for our use case (pwm backlight), the precision can meet our
> > requirement. Moreover, we usually do not change the period, just
> > adjust the duty to change the back light.
>
> Is this a license requirement for you SoC to only drive a backlight with
> the PWM? The idea of having a PWM driver on your platform is that it can
> also be used to control a step motor or a laser.

Not a license requirement. Until now we have not got any higher
precision requirements, and we've run this driver for many years in
our downstream kernel.

> > > 255 / clk_rate) because if 145.34 µs is requested you configure
> > > PRESCALE = 17 and so yield a period of 137.70 µs. If however you'd pick
> >
> > I did not get you here, if period is 145.34, we still get the
> > corresponding PRESCALE = 18 by below formula:
> >
> > tmp = (u64)chn->clk_rate * period_ns;
> > do_div(tmp, NSEC_PER_SEC);
> > prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
>
> I assumed you switch back to rounding down to match your comment and
> which is how I think a pwm should behave. With rounding down we get
> PRESCALE = 17 as I claimed.

OK.

>
> > > PRESCALE = 18 and MOD = 254 you get a period of 144.78 µs and so the
> > > error is only 0.56 µs which is a factor of 13 better.
> > >
> > > Hmm.
> > >
> > > > +      * The value for PRESCALE is selected such that the resulting period
> > > > +      * gets the maximal length not bigger than the requested one with the
> > > > +      * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
> > > > +      */
> > > > +     duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> > >
> > > I wonder if you loose some precision here as you use period_ns but might
> > > actually implement a shorter period.
> > >
> > > Quick example, again consider clk_rate = 100 / 3 MHz,
> > > period_ns = 145340, duty_ns = 72670. Then you end up with
> > >
> > >         PRESCALE = 17
> > >         MOD = 255
> > >         DUTY = 127
> >
> > Incorrect, we will get PRESCALE = 18,  MOD = 255, DUTY = 127.
> >
> > > That corresponds to period_ns = 137700, duty_ns = 68580. With DUTY = 134
> > > you get 72360 ns which is still smaller than the requested 72670 ns.
> >
> > Incorrect, with DUTY = 134 (PRESCALE = 18  ->  period = 145.35 µs),
> > duty_ns = 76380ns
>
> Yes, as above. When using rounding-closest your error is not in [0, 7.65
> µs] but in [-3.825 µs, 3.825 µs]. Doesn't make it better.

Actually our use case really dose not care about this error.

>
> > > (But then again it is not obvious which of the two is the "better"
> > > approximation because Thierry doesn't seem to see the necessity to
> > > discuss or define a policy here.)
> >
> > Like I said, this is the simple calculation formula which can meet our
> > requirement (we limit our DUTY value can only be 0 - 254).
> > duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
>
> simple is often good but sometimes different from correct. And even with

I do not think this is incorrect.

> rounding closest instead of rounding down you're giving away precision
> here and the size of the error interval is the same, it is just centered
> around 0 instead of only positive. If I hadn't spend so much time with
> pwm reviews this week I'd try to come up with an example.

I am very appreciated for your comments.

> > > (And to pick up the thoughts about not using SPRD_PWM_MOD_MAX
> > > unconditionally, you could also use
> > >
> > >         PRESCALE = 18
> > >         MOD = 254
> > >         DUTY = 127
> > >
> > > yielding period_ns = 144780 and duty_ns = 72390. Summary:
> > >
> > >         Request:                 72670 / 145340
> > >         your result:             68580 / 137700
> > >         also possible:           72390 / 144780
> > >
> > > Judge yourself.)
> > >
> > > > +     tmp = (u64)chn->clk_rate * period_ns;
> > > > +     do_div(tmp, NSEC_PER_SEC);
> > > > +     prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
> > >
> > > Now that you use DIV_ROUND_CLOSEST_ULL the comment is wrong because you
> > > might provide a period bigger than the requested one. Also you divide
> >
> > Again, that's the precision can meet our requirement.
>
> If you go back to rounding down, use the matching rounding up in
> .get_state and adapt your comment describing you're sticking to MOD=255
> to say explicitly that you're loosing precision I can live with that. I
> even did the math for .get_state in my previous mail for you.
>
> The idea of the requirement to round down is that I want to introduce
> this as policy for the PWM framework to get some uniform behaviour from

Have you made a consensus about this? Documented it?

> all lowlevel drivers. If you do this now I won't bother you later when
> the requirement is implemented in your driver. And the comment helps
> someone who evaluates your SoC to judge if there is still work to do if
> they have higher requirements for the PWM.

So what you asked is something like below, right?
diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
index 96f8aa0..1d3db94 100644
--- a/drivers/pwm/pwm-sprd.c
+++ b/drivers/pwm/pwm-sprd.c
@@ -103,12 +103,12 @@ static void sprd_pwm_get_state(struct pwm_chip
*chip, struct pwm_device *pwm,
        val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
        prescale = val & SPRD_PWM_PRESCALE_MSK;
        tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
-       state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
+       state->period = DIV_ROUND_UP_ULL(tmp, chn->clk_rate);

        val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
        duty = val & SPRD_PWM_DUTY_MSK;
        tmp = (prescale + 1) * NSEC_PER_SEC * duty;
-       state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
+       state->duty_cycle = DIV_ROUND_UP_ULL(tmp, chn->clk_rate);

        /* Disable PWM clocks if the PWM channel is not in enable state. */
        if (!state->enabled)
@@ -135,8 +135,8 @@ static int sprd_pwm_config(struct sprd_pwm_chip
*spc, struct pwm_device *pwm,
        duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;

        tmp = (u64)chn->clk_rate * period_ns;
-       do_div(tmp, NSEC_PER_SEC);
-       prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
+       div = 1000000000ULL * SPRD_PWM_MOD_MAX;
+       prescale = div64_u64(tmp, div) - 1;
        if (prescale > SPRD_PWM_PRESCALE_MSK)
                prescale = SPRD_PWM_PRESCALE_MSK;

But our MOD is constant, it did not help to improve the precision.
Instead, like you said, when period_ns = 145340, we will set PRESCALE
= 17, so in .get_state(), user will get period_ns = 137700 (error
is145340 -  137700).

But if we use DIV_ROUND_CLOSEST, in .get_state(), user will get
period_ns = 145350 (error is145350 -  145340).

> > > twice instead of once before. (I don't know what architecture your SoC
> > > uses, but compared to a multiplication a division is usually expensive.)
> > > Also the math is more complicated now as you have a round-down div and a
> > > round-closest div.
> > >
> > > My preference for how to fix that is to restore the behaviour of v2 that
> > > matches the comment and adapt .get_state() instead.
> >
> > Using DIV_ROUND_CLOSEST_ULL can get a same prescale which matches with
> > .get_state().
>
> I don't get you here. Do you say that with DIV_ROUND_CLOSEST_ULL you get
> the same result but DIV_ROUND_CLOSEST_ULL matches .get_state while
> rounding down doesn't? I cannot follow.

Yes, that's what I mean.


--
Baolin Wang
Best Regards

Re: [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support

[Uwe Kleine-König]

Hello Baolin,

On Thu, Aug 15, 2019 at 04:16:32PM +0800, Baolin Wang wrote:
> On Thu, 15 Aug 2019 at 14:15, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Thu, Aug 15, 2019 at 11:34:27AM +0800, Baolin Wang wrote:
> > > On Wed, 14 Aug 2019 at 23:03, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Wed, Aug 14, 2019 at 08:46:11PM +0800, Baolin Wang wrote:
> > > > > +     /*
> > > > > +      * The hardware provides a counter that is feed by the source clock.
> > > > > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > > > > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > > > > +      * Thus the period_ns and duty_ns calculation formula should be:
> > > > > +      * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
> > > > > +      * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
> > > > > +      */
> > > > > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> > > > > +     prescale = val & SPRD_PWM_PRESCALE_MSK;
> > > > > +     tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > > > > +     state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > > > +
> > > > > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> > > > > +     duty = val & SPRD_PWM_DUTY_MSK;
> > > > > +     tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> > > > > +     state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > > > +
> > > > > +     /* Disable PWM clocks if the PWM channel is not in enable state. */
> > > > > +     if (!state->enabled)
> > > > > +             clk_bulk_disable_unprepare(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
> > > > > +}
> > > > > +
> > > > > +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> > > > > +                        int duty_ns, int period_ns)
> > > > > +{
> > > > > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > > > +     u32 prescale, duty;
> > > > > +     u64 tmp;
> > > > > +
> > > > > +     /*
> > > > > +      * The hardware provides a counter that is feed by the source clock.
> > > > > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > > > > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > > > > +      *
> > > > > +      * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
> > > >
> > > > Did you spend some thoughts about how wrong your period can get because
> > > > of that "lazyness"?
> > > >
> > > > Let's assume a clk rate of 100/3 MHz. Then the available period lengths
> > > > are:
> > > >
> > > >         PRESCALE =  0  ->  period =   7.65 µs
> > > >         PRESCALE =  1  ->  period =  15.30 µs
> > > >         ...
> > > >         PRESCALE = 17  ->  period = 137.70 µs
> > > >         PRESCALE = 18  ->  period = 145.35 µs
> > > >
> > > > So the error can be up to (nearly) 7.65 µs (or in general
> > >
> > > Yes, but for our use case (pwm backlight), the precision can meet our
> > > requirement. Moreover, we usually do not change the period, just
> > > adjust the duty to change the back light.
> >
> > Is this a license requirement for you SoC to only drive a backlight with
> > the PWM? The idea of having a PWM driver on your platform is that it can
> > also be used to control a step motor or a laser.
> 
> Not a license requirement. Until now we have not got any higher
> precision requirements, and we've run this driver for many years in
> our downstream kernel.

I understood that you're not ambitious to do something better than "it
worked for years".

> > > > PRESCALE = 18 and MOD = 254 you get a period of 144.78 µs and so the
> > > > error is only 0.56 µs which is a factor of 13 better.
> > > >
> > > > Hmm.
> > > >
> > > > > +      * The value for PRESCALE is selected such that the resulting period
> > > > > +      * gets the maximal length not bigger than the requested one with the
> > > > > +      * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
> > > > > +      */
> > > > > +     duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> > > >
> > > > I wonder if you loose some precision here as you use period_ns but might
> > > > actually implement a shorter period.
> > > >
> > > > Quick example, again consider clk_rate = 100 / 3 MHz,
> > > > period_ns = 145340, duty_ns = 72670. Then you end up with
> > > >
> > > >         PRESCALE = 17
> > > >         MOD = 255
> > > >         DUTY = 127
> > >
> > > Incorrect, we will get PRESCALE = 18,  MOD = 255, DUTY = 127.
> > >
> > > > That corresponds to period_ns = 137700, duty_ns = 68580. With DUTY = 134
> > > > you get 72360 ns which is still smaller than the requested 72670 ns.
> > >
> > > Incorrect, with DUTY = 134 (PRESCALE = 18  ->  period = 145.35 µs),
> > > duty_ns = 76380ns
> >
> > Yes, as above. When using rounding-closest your error is not in [0, 7.65
> > µs] but in [-3.825 µs, 3.825 µs]. Doesn't make it better.
> 
> Actually our use case really dose not care about this error.

I assume that Thierry will apply your patch anyhow. But be prepared that
you get a patch from me then to improve precision. It would be a waste
of resources not to do that after doing all the necessary math already.

> > > > (But then again it is not obvious which of the two is the "better"
> > > > approximation because Thierry doesn't seem to see the necessity to
> > > > discuss or define a policy here.)
> > >
> > > Like I said, this is the simple calculation formula which can meet our
> > > requirement (we limit our DUTY value can only be 0 - 254).
> > > duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> >
> > simple is often good but sometimes different from correct. And even with
> 
> I do not think this is incorrect.

Well, "correct" is probably not the right term. The problem is that my
efforts to improve the PWM framework are not going forward. And so the
suggestions I made here are not normative (yet) and undocumented.

> > rounding closest instead of rounding down you're giving away precision
> > here and the size of the error interval is the same, it is just centered
> > around 0 instead of only positive. If I hadn't spend so much time with
> > pwm reviews this week I'd try to come up with an example.
> 
> I am very appreciated for your comments.
> 
> > > > (And to pick up the thoughts about not using SPRD_PWM_MOD_MAX
> > > > unconditionally, you could also use
> > > >
> > > >         PRESCALE = 18
> > > >         MOD = 254
> > > >         DUTY = 127
> > > >
> > > > yielding period_ns = 144780 and duty_ns = 72390. Summary:
> > > >
> > > >         Request:                 72670 / 145340
> > > >         your result:             68580 / 137700
> > > >         also possible:           72390 / 144780
> > > >
> > > > Judge yourself.)
> > > >
> > > > > +     tmp = (u64)chn->clk_rate * period_ns;
> > > > > +     do_div(tmp, NSEC_PER_SEC);
> > > > > +     prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
> > > >
> > > > Now that you use DIV_ROUND_CLOSEST_ULL the comment is wrong because you
> > > > might provide a period bigger than the requested one. Also you divide
> > >
> > > Again, that's the precision can meet our requirement.
> >
> > If you go back to rounding down, use the matching rounding up in
> > .get_state and adapt your comment describing you're sticking to MOD=255
> > to say explicitly that you're loosing precision I can live with that. I
> > even did the math for .get_state in my previous mail for you.
> >
> > The idea of the requirement to round down is that I want to introduce
> > this as policy for the PWM framework to get some uniform behaviour from
> 
> Have you made a consensus about this? Documented it?

I tried. Check the pwm patchwork, there are uncommented patches that
first try to document the current status quo. When these are "in" I
intend to discuss the improvements I have in mind. But don't expect this
to be quickly done as even the (AFAICT) noncontroversial documentation
patches[1] fail to get applied.

> > all lowlevel drivers. If you do this now I won't bother you later when
> > the requirement is implemented in your driver. And the comment helps
> > someone who evaluates your SoC to judge if there is still work to do if
> > they have higher requirements for the PWM.
> 
> So what you asked is something like below, right?
> diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
> index 96f8aa0..1d3db94 100644
> --- a/drivers/pwm/pwm-sprd.c
> +++ b/drivers/pwm/pwm-sprd.c
> @@ -103,12 +103,12 @@ static void sprd_pwm_get_state(struct pwm_chip
> *chip, struct pwm_device *pwm,
>         val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
>         prescale = val & SPRD_PWM_PRESCALE_MSK;
>         tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> -       state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> +       state->period = DIV_ROUND_UP_ULL(tmp, chn->clk_rate);
> 
>         val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
>         duty = val & SPRD_PWM_DUTY_MSK;
>         tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> -       state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> +       state->duty_cycle = DIV_ROUND_UP_ULL(tmp, chn->clk_rate);
> 
>         /* Disable PWM clocks if the PWM channel is not in enable state. */
>         if (!state->enabled)
> @@ -135,8 +135,8 @@ static int sprd_pwm_config(struct sprd_pwm_chip
> *spc, struct pwm_device *pwm,
>         duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> 
>         tmp = (u64)chn->clk_rate * period_ns;
> -       do_div(tmp, NSEC_PER_SEC);
> -       prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
> +       div = 1000000000ULL * SPRD_PWM_MOD_MAX;
> +       prescale = div64_u64(tmp, div) - 1;
>         if (prescale > SPRD_PWM_PRESCALE_MSK)
>                 prescale = SPRD_PWM_PRESCALE_MSK;

This goes in the right direction for sure.

Without taking paper and pencil I wouldn't be surprised if the
calculation of duty_cycle in .get_state didn't match the calculation of
DUTY in .apply yet though.

> But our MOD is constant, it did not help to improve the precision.
> Instead, like you said, when period_ns = 145340, we will set PRESCALE
> = 17, so in .get_state(), user will get period_ns = 137700 (error
> is145340 -  137700).
> 
> But if we use DIV_ROUND_CLOSEST, in .get_state(), user will get
> period_ns = 145350 (error is 145350 -  145340).

In this case DIV_ROUND_CLOSEST seems to get nearer to the requested
value than when rounding down. But this example was constructed to show
your original algorithm to be bad, and just because you modify your
algorithm to perform better on that constructed example doesn't imply
the new one is better. Moreover you implement a bigger period than
requested which is something I intend to forbid in the future.

And note that with PWMs there is no "objective" metric that can tell you
which of two implementable outputs better match a given request. It
depends on the use case, so the best we can do is to tell our users our
metric and with that in mind they can create a request that then fits
their needs.

> > > > twice instead of once before. (I don't know what architecture your SoC
> > > > uses, but compared to a multiplication a division is usually expensive.)
> > > > Also the math is more complicated now as you have a round-down div and a
> > > > round-closest div.
> > > >
> > > > My preference for how to fix that is to restore the behaviour of v2 that
> > > > matches the comment and adapt .get_state() instead.
> > >
> > > Using DIV_ROUND_CLOSEST_ULL can get a same prescale which matches with
> > > .get_state().
> >
> > I don't get you here. Do you say that with DIV_ROUND_CLOSEST_ULL you get
> > the same result but DIV_ROUND_CLOSEST_ULL matches .get_state while
> > rounding down doesn't? I cannot follow.
> 
> Yes, that's what I mean.

But that is logically broken. If both approaches yield the same
results it cannot be true that exactly one of them matches the inverse
of .get_state.

I'm still confused.

Best regards
Uwe

[1] https://patchwork.ozlabs.org/patch/1021566/

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

Re: [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support

[Baolin Wang]

On Thu, 15 Aug 2019 at 16:54, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Baolin,
>
> On Thu, Aug 15, 2019 at 04:16:32PM +0800, Baolin Wang wrote:
> > On Thu, 15 Aug 2019 at 14:15, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Thu, Aug 15, 2019 at 11:34:27AM +0800, Baolin Wang wrote:
> > > > On Wed, 14 Aug 2019 at 23:03, Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > On Wed, Aug 14, 2019 at 08:46:11PM +0800, Baolin Wang wrote:
> > > > > > +     /*
> > > > > > +      * The hardware provides a counter that is feed by the source clock.
> > > > > > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > > > > > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > > > > > +      * Thus the period_ns and duty_ns calculation formula should be:
> > > > > > +      * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
> > > > > > +      * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
> > > > > > +      */
> > > > > > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> > > > > > +     prescale = val & SPRD_PWM_PRESCALE_MSK;
> > > > > > +     tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > > > > > +     state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > > > > +
> > > > > > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> > > > > > +     duty = val & SPRD_PWM_DUTY_MSK;
> > > > > > +     tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> > > > > > +     state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > > > > +
> > > > > > +     /* Disable PWM clocks if the PWM channel is not in enable state. */
> > > > > > +     if (!state->enabled)
> > > > > > +             clk_bulk_disable_unprepare(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
> > > > > > +}
> > > > > > +
> > > > > > +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> > > > > > +                        int duty_ns, int period_ns)
> > > > > > +{
> > > > > > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > > > > +     u32 prescale, duty;
> > > > > > +     u64 tmp;
> > > > > > +
> > > > > > +     /*
> > > > > > +      * The hardware provides a counter that is feed by the source clock.
> > > > > > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > > > > > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > > > > > +      *
> > > > > > +      * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
> > > > >
> > > > > Did you spend some thoughts about how wrong your period can get because
> > > > > of that "lazyness"?
> > > > >
> > > > > Let's assume a clk rate of 100/3 MHz. Then the available period lengths
> > > > > are:
> > > > >
> > > > >         PRESCALE =  0  ->  period =   7.65 µs
> > > > >         PRESCALE =  1  ->  period =  15.30 µs
> > > > >         ...
> > > > >         PRESCALE = 17  ->  period = 137.70 µs
> > > > >         PRESCALE = 18  ->  period = 145.35 µs
> > > > >
> > > > > So the error can be up to (nearly) 7.65 µs (or in general
> > > >
> > > > Yes, but for our use case (pwm backlight), the precision can meet our
> > > > requirement. Moreover, we usually do not change the period, just
> > > > adjust the duty to change the back light.
> > >
> > > Is this a license requirement for you SoC to only drive a backlight with
> > > the PWM? The idea of having a PWM driver on your platform is that it can
> > > also be used to control a step motor or a laser.
> >
> > Not a license requirement. Until now we have not got any higher
> > precision requirements, and we've run this driver for many years in
> > our downstream kernel.
>
> I understood that you're not ambitious to do something better than "it
> worked for years".

How do you know that? If there are some cases expect a higher
precision, then we can analyze how precision asked by the user, then
we have a goal to improve it, even improve the hardware. But now, I
said there are no these use cases, why I should add more mathematics
to increase load and complication.

> > > > > PRESCALE = 18 and MOD = 254 you get a period of 144.78 µs and so the
> > > > > error is only 0.56 µs which is a factor of 13 better.
> > > > >
> > > > > Hmm.
> > > > >
> > > > > > +      * The value for PRESCALE is selected such that the resulting period
> > > > > > +      * gets the maximal length not bigger than the requested one with the
> > > > > > +      * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
> > > > > > +      */
> > > > > > +     duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> > > > >
> > > > > I wonder if you loose some precision here as you use period_ns but might
> > > > > actually implement a shorter period.
> > > > >
> > > > > Quick example, again consider clk_rate = 100 / 3 MHz,
> > > > > period_ns = 145340, duty_ns = 72670. Then you end up with
> > > > >
> > > > >         PRESCALE = 17
> > > > >         MOD = 255
> > > > >         DUTY = 127
> > > >
> > > > Incorrect, we will get PRESCALE = 18,  MOD = 255, DUTY = 127.
> > > >
> > > > > That corresponds to period_ns = 137700, duty_ns = 68580. With DUTY = 134
> > > > > you get 72360 ns which is still smaller than the requested 72670 ns.
> > > >
> > > > Incorrect, with DUTY = 134 (PRESCALE = 18  ->  period = 145.35 µs),
> > > > duty_ns = 76380ns
> > >
> > > Yes, as above. When using rounding-closest your error is not in [0, 7.65
> > > µs] but in [-3.825 µs, 3.825 µs]. Doesn't make it better.
> >
> > Actually our use case really dose not care about this error.
>
> I assume that Thierry will apply your patch anyhow. But be prepared that
> you get a patch from me then to improve precision. It would be a waste
> of resources not to do that after doing all the necessary math already.

Glad to see your improvement without introducing complicated and more
mathematics.

>
> > > > > (But then again it is not obvious which of the two is the "better"
> > > > > approximation because Thierry doesn't seem to see the necessity to
> > > > > discuss or define a policy here.)
> > > >
> > > > Like I said, this is the simple calculation formula which can meet our
> > > > requirement (we limit our DUTY value can only be 0 - 254).
> > > > duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> > >
> > > simple is often good but sometimes different from correct. And even with
> >
> > I do not think this is incorrect.
>
> Well, "correct" is probably not the right term. The problem is that my
> efforts to improve the PWM framework are not going forward. And so the
> suggestions I made here are not normative (yet) and undocumented.

OK.

>
> > > rounding closest instead of rounding down you're giving away precision
> > > here and the size of the error interval is the same, it is just centered
> > > around 0 instead of only positive. If I hadn't spend so much time with
> > > pwm reviews this week I'd try to come up with an example.
> >
> > I am very appreciated for your comments.
> >
> > > > > (And to pick up the thoughts about not using SPRD_PWM_MOD_MAX
> > > > > unconditionally, you could also use
> > > > >
> > > > >         PRESCALE = 18
> > > > >         MOD = 254
> > > > >         DUTY = 127
> > > > >
> > > > > yielding period_ns = 144780 and duty_ns = 72390. Summary:
> > > > >
> > > > >         Request:                 72670 / 145340
> > > > >         your result:             68580 / 137700
> > > > >         also possible:           72390 / 144780
> > > > >
> > > > > Judge yourself.)
> > > > >
> > > > > > +     tmp = (u64)chn->clk_rate * period_ns;
> > > > > > +     do_div(tmp, NSEC_PER_SEC);
> > > > > > +     prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
> > > > >
> > > > > Now that you use DIV_ROUND_CLOSEST_ULL the comment is wrong because you
> > > > > might provide a period bigger than the requested one. Also you divide
> > > >
> > > > Again, that's the precision can meet our requirement.
> > >
> > > If you go back to rounding down, use the matching rounding up in
> > > .get_state and adapt your comment describing you're sticking to MOD=255
> > > to say explicitly that you're loosing precision I can live with that. I
> > > even did the math for .get_state in my previous mail for you.
> > >
> > > The idea of the requirement to round down is that I want to introduce
> > > this as policy for the PWM framework to get some uniform behaviour from
> >
> > Have you made a consensus about this? Documented it?
>
> I tried. Check the pwm patchwork, there are uncommented patches that
> first try to document the current status quo. When these are "in" I
> intend to discuss the improvements I have in mind. But don't expect this
> to be quickly done as even the (AFAICT) noncontroversial documentation
> patches[1] fail to get applied.

OK.

>
> > > all lowlevel drivers. If you do this now I won't bother you later when
> > > the requirement is implemented in your driver. And the comment helps
> > > someone who evaluates your SoC to judge if there is still work to do if
> > > they have higher requirements for the PWM.
> >
> > So what you asked is something like below, right?
> > diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
> > index 96f8aa0..1d3db94 100644
> > --- a/drivers/pwm/pwm-sprd.c
> > +++ b/drivers/pwm/pwm-sprd.c
> > @@ -103,12 +103,12 @@ static void sprd_pwm_get_state(struct pwm_chip
> > *chip, struct pwm_device *pwm,
> >         val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> >         prescale = val & SPRD_PWM_PRESCALE_MSK;
> >         tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > -       state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > +       state->period = DIV_ROUND_UP_ULL(tmp, chn->clk_rate);
> >
> >         val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> >         duty = val & SPRD_PWM_DUTY_MSK;
> >         tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> > -       state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > +       state->duty_cycle = DIV_ROUND_UP_ULL(tmp, chn->clk_rate);
> >
> >         /* Disable PWM clocks if the PWM channel is not in enable state. */
> >         if (!state->enabled)
> > @@ -135,8 +135,8 @@ static int sprd_pwm_config(struct sprd_pwm_chip
> > *spc, struct pwm_device *pwm,
> >         duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> >
> >         tmp = (u64)chn->clk_rate * period_ns;
> > -       do_div(tmp, NSEC_PER_SEC);
> > -       prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
> > +       div = 1000000000ULL * SPRD_PWM_MOD_MAX;
> > +       prescale = div64_u64(tmp, div) - 1;
> >         if (prescale > SPRD_PWM_PRESCALE_MSK)
> >                 prescale = SPRD_PWM_PRESCALE_MSK;
>
> This goes in the right direction for sure.
>
> Without taking paper and pencil I wouldn't be surprised if the
> calculation of duty_cycle in .get_state didn't match the calculation of
> DUTY in .apply yet though.
>
> > But our MOD is constant, it did not help to improve the precision.
> > Instead, like you said, when period_ns = 145340, we will set PRESCALE
> > = 17, so in .get_state(), user will get period_ns = 137700 (error
> > is145340 -  137700).
> >
> > But if we use DIV_ROUND_CLOSEST, in .get_state(), user will get
> > period_ns = 145350 (error is 145350 -  145340).
>
> In this case DIV_ROUND_CLOSEST seems to get nearer to the requested
> value than when rounding down. But this example was constructed to show
> your original algorithm to be bad, and just because you modify your
> algorithm to perform better on that constructed example doesn't imply
> the new one is better. Moreover you implement a bigger period than
> requested which is something I intend to forbid in the future.
>
> And note that with PWMs there is no "objective" metric that can tell you
> which of two implementable outputs better match a given request. It
> depends on the use case, so the best we can do is to tell our users our
> metric and with that in mind they can create a request that then fits
> their needs.

Yes, that should be asked by the use case, some cases do not care a
little bigger period than requested.

As you said, what you asked did not get a consensus yet, so I'd like
to wait for Thierry's suggestion.

> > > > > twice instead of once before. (I don't know what architecture your SoC
> > > > > uses, but compared to a multiplication a division is usually expensive.)
> > > > > Also the math is more complicated now as you have a round-down div and a
> > > > > round-closest div.
> > > > >
> > > > > My preference for how to fix that is to restore the behaviour of v2 that
> > > > > matches the comment and adapt .get_state() instead.
> > > >
> > > > Using DIV_ROUND_CLOSEST_ULL can get a same prescale which matches with
> > > > .get_state().
> > >
> > > I don't get you here. Do you say that with DIV_ROUND_CLOSEST_ULL you get
> > > the same result but DIV_ROUND_CLOSEST_ULL matches .get_state while
> > > rounding down doesn't? I cannot follow.
> >
> > Yes, that's what I mean.
>
> But that is logically broken. If both approaches yield the same
> results it cannot be true that exactly one of them matches the inverse
> of .get_state.

What I mean is use DIV_ROUND_CLOSEST_ULL we can get a nearer value to
the requested like above example.

-- 
Baolin Wang
Best Regards

Re: [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support

[Uwe Kleine-König]

Hello,

On Thu, Aug 15, 2019 at 05:34:02PM +0800, Baolin Wang wrote:
> On Thu, 15 Aug 2019 at 16:54, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Thu, Aug 15, 2019 at 04:16:32PM +0800, Baolin Wang wrote:
> > > On Thu, 15 Aug 2019 at 14:15, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Thu, Aug 15, 2019 at 11:34:27AM +0800, Baolin Wang wrote:
> > > > > On Wed, 14 Aug 2019 at 23:03, Uwe Kleine-König
> > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > On Wed, Aug 14, 2019 at 08:46:11PM +0800, Baolin Wang wrote:
> > > > > > > +     /*
> > > > > > > +      * The hardware provides a counter that is feed by the source clock.
> > > > > > > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > > > > > > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > > > > > > +      * Thus the period_ns and duty_ns calculation formula should be:
> > > > > > > +      * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
> > > > > > > +      * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
> > > > > > > +      */
> > > > > > > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> > > > > > > +     prescale = val & SPRD_PWM_PRESCALE_MSK;
> > > > > > > +     tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > > > > > > +     state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > > > > > +
> > > > > > > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> > > > > > > +     duty = val & SPRD_PWM_DUTY_MSK;
> > > > > > > +     tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> > > > > > > +     state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > > > > > +
> > > > > > > +     /* Disable PWM clocks if the PWM channel is not in enable state. */
> > > > > > > +     if (!state->enabled)
> > > > > > > +             clk_bulk_disable_unprepare(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> > > > > > > +                        int duty_ns, int period_ns)
> > > > > > > +{
> > > > > > > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > > > > > +     u32 prescale, duty;
> > > > > > > +     u64 tmp;
> > > > > > > +
> > > > > > > +     /*
> > > > > > > +      * The hardware provides a counter that is feed by the source clock.
> > > > > > > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > > > > > > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > > > > > > +      *
> > > > > > > +      * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
> > > > > >
> > > > > > Did you spend some thoughts about how wrong your period can get because
> > > > > > of that "lazyness"?
> > > > > >
> > > > > > Let's assume a clk rate of 100/3 MHz. Then the available period lengths
> > > > > > are:
> > > > > >
> > > > > >         PRESCALE =  0  ->  period =   7.65 µs
> > > > > >         PRESCALE =  1  ->  period =  15.30 µs
> > > > > >         ...
> > > > > >         PRESCALE = 17  ->  period = 137.70 µs
> > > > > >         PRESCALE = 18  ->  period = 145.35 µs
> > > > > >
> > > > > > So the error can be up to (nearly) 7.65 µs (or in general
> > > > >
> > > > > Yes, but for our use case (pwm backlight), the precision can meet our
> > > > > requirement. Moreover, we usually do not change the period, just
> > > > > adjust the duty to change the back light.
> > > >
> > > > Is this a license requirement for you SoC to only drive a backlight with
> > > > the PWM? The idea of having a PWM driver on your platform is that it can
> > > > also be used to control a step motor or a laser.
> > >
> > > Not a license requirement. Until now we have not got any higher
> > > precision requirements, and we've run this driver for many years in
> > > our downstream kernel.
> >
> > I understood that you're not ambitious to do something better than "it
> > worked for years".
> 
> How do you know that?

I showed you how you could match the requested PWM output better and
you refused telling it worked for years and the added precision isn't
necessary for a backlight.

> If there are some cases expect a higher precision, then we can analyze
> how precision asked by the user, then we have a goal to improve it,
> even improve the hardware. But now, I said there are no these use
> cases, why I should add more mathematics to increase load and
> complication.
> 
> > > > > > PRESCALE = 18 and MOD = 254 you get a period of 144.78 µs and so the
> > > > > > error is only 0.56 µs which is a factor of 13 better.
> > > > > >
> > > > > > Hmm.
> > > > > >
> > > > > > > +      * The value for PRESCALE is selected such that the resulting period
> > > > > > > +      * gets the maximal length not bigger than the requested one with the
> > > > > > > +      * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
> > > > > > > +      */
> > > > > > > +     duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> > > > > >
> > > > > > I wonder if you loose some precision here as you use period_ns but might
> > > > > > actually implement a shorter period.
> > > > > >
> > > > > > Quick example, again consider clk_rate = 100 / 3 MHz,
> > > > > > period_ns = 145340, duty_ns = 72670. Then you end up with
> > > > > >
> > > > > >         PRESCALE = 17
> > > > > >         MOD = 255
> > > > > >         DUTY = 127
> > > > >
> > > > > Incorrect, we will get PRESCALE = 18,  MOD = 255, DUTY = 127.
> > > > >
> > > > > > That corresponds to period_ns = 137700, duty_ns = 68580. With DUTY = 134
> > > > > > you get 72360 ns which is still smaller than the requested 72670 ns.
> > > > >
> > > > > Incorrect, with DUTY = 134 (PRESCALE = 18  ->  period = 145.35 µs),
> > > > > duty_ns = 76380ns
> > > >
> > > > Yes, as above. When using rounding-closest your error is not in [0, 7.65
> > > > µs] but in [-3.825 µs, 3.825 µs]. Doesn't make it better.
> > >
> > > Actually our use case really dose not care about this error.
> >
> > I assume that Thierry will apply your patch anyhow. But be prepared that
> > you get a patch from me then to improve precision. It would be a waste
> > of resources not to do that after doing all the necessary math already.
> 
> Glad to see your improvement without introducing complicated and more
> mathematics.

I don't understand you. Either you or me will improve the precision. The
maths is the same for both cases. I would prefer you do it, otherwise I
will have the problem later that I must get you to invest the time to
test or I'd have to argue the change to go in untested.

> > > > all lowlevel drivers. If you do this now I won't bother you later when
> > > > the requirement is implemented in your driver. And the comment helps
> > > > someone who evaluates your SoC to judge if there is still work to do if
> > > > they have higher requirements for the PWM.
> > >
> > > So what you asked is something like below, right?
> > > diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
> > > index 96f8aa0..1d3db94 100644
> > > --- a/drivers/pwm/pwm-sprd.c
> > > +++ b/drivers/pwm/pwm-sprd.c
> > > @@ -103,12 +103,12 @@ static void sprd_pwm_get_state(struct pwm_chip
> > > *chip, struct pwm_device *pwm,
> > >         val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> > >         prescale = val & SPRD_PWM_PRESCALE_MSK;
> > >         tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > > -       state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > +       state->period = DIV_ROUND_UP_ULL(tmp, chn->clk_rate);
> > >
> > >         val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> > >         duty = val & SPRD_PWM_DUTY_MSK;
> > >         tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> > > -       state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > +       state->duty_cycle = DIV_ROUND_UP_ULL(tmp, chn->clk_rate);
> > >
> > >         /* Disable PWM clocks if the PWM channel is not in enable state. */
> > >         if (!state->enabled)
> > > @@ -135,8 +135,8 @@ static int sprd_pwm_config(struct sprd_pwm_chip
> > > *spc, struct pwm_device *pwm,
> > >         duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> > >
> > >         tmp = (u64)chn->clk_rate * period_ns;
> > > -       do_div(tmp, NSEC_PER_SEC);
> > > -       prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
> > > +       div = 1000000000ULL * SPRD_PWM_MOD_MAX;
> > > +       prescale = div64_u64(tmp, div) - 1;
> > >         if (prescale > SPRD_PWM_PRESCALE_MSK)
> > >                 prescale = SPRD_PWM_PRESCALE_MSK;
> >
> > This goes in the right direction for sure.
> >
> > Without taking paper and pencil I wouldn't be surprised if the
> > calculation of duty_cycle in .get_state didn't match the calculation of
> > DUTY in .apply yet though.
> >
> > > But our MOD is constant, it did not help to improve the precision.
> > > Instead, like you said, when period_ns = 145340, we will set PRESCALE
> > > = 17, so in .get_state(), user will get period_ns = 137700 (error
> > > is145340 -  137700).
> > >
> > > But if we use DIV_ROUND_CLOSEST, in .get_state(), user will get
> > > period_ns = 145350 (error is 145350 -  145340).
> >
> > In this case DIV_ROUND_CLOSEST seems to get nearer to the requested
> > value than when rounding down. But this example was constructed to show
> > your original algorithm to be bad, and just because you modify your
> > algorithm to perform better on that constructed example doesn't imply
> > the new one is better. Moreover you implement a bigger period than
> > requested which is something I intend to forbid in the future.
> >
> > And note that with PWMs there is no "objective" metric that can tell you
> > which of two implementable outputs better match a given request. It
> > depends on the use case, so the best we can do is to tell our users our
> > metric and with that in mind they can create a request that then fits
> > their needs.
> 
> Yes, that should be asked by the use case, some cases do not care a
> little bigger period than requested.

So for some cases it is beneficial to be predictable and for other it
isn't. So the only safe thing to do for a lowlevel driver is to be
predictable always because it cannot (and shouldn't) tell if the current
request is one of cases where precision matters.

> As you said, what you asked did not get a consensus yet, so I'd like
> to wait for Thierry's suggestion.
> 
> > > > > > twice instead of once before. (I don't know what architecture your SoC
> > > > > > uses, but compared to a multiplication a division is usually expensive.)
> > > > > > Also the math is more complicated now as you have a round-down div and a
> > > > > > round-closest div.
> > > > > >
> > > > > > My preference for how to fix that is to restore the behaviour of v2 that
> > > > > > matches the comment and adapt .get_state() instead.
> > > > >
> > > > > Using DIV_ROUND_CLOSEST_ULL can get a same prescale which matches with
> > > > > .get_state().
> > > >
> > > > I don't get you here. Do you say that with DIV_ROUND_CLOSEST_ULL you get
> > > > the same result but DIV_ROUND_CLOSEST_ULL matches .get_state while
> > > > rounding down doesn't? I cannot follow.
> > >
> > > Yes, that's what I mean.
> >
> > But that is logically broken. If both approaches yield the same
> > results it cannot be true that exactly one of them matches the inverse
> > of .get_state.
> 
> What I mean is use DIV_ROUND_CLOSEST_ULL we can get a nearer value to
> the requested like above example.

But given that it's unclear if 137700 ns or 145350 ns is better when
145340 ns was requested this is not a strong argument to use
DIV_ROUND_CLOSEST_ULL. With the global picture for the pwm framework in
mind it is sensible to request the same rounding from all drivers to get
a consistent behaviour. And I believe the maths with rounding down is
easier than when rounding up or nearest. That's why I argue in this
direction.

Best regards
Uwe

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

Re: [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support

[Baolin Wang]

On Thu, 15 Aug 2019 at 18:11, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Thu, Aug 15, 2019 at 05:34:02PM +0800, Baolin Wang wrote:
> > On Thu, 15 Aug 2019 at 16:54, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Thu, Aug 15, 2019 at 04:16:32PM +0800, Baolin Wang wrote:
> > > > On Thu, 15 Aug 2019 at 14:15, Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > On Thu, Aug 15, 2019 at 11:34:27AM +0800, Baolin Wang wrote:
> > > > > > On Wed, 14 Aug 2019 at 23:03, Uwe Kleine-König
> > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > > On Wed, Aug 14, 2019 at 08:46:11PM +0800, Baolin Wang wrote:
> > > > > > > > +     /*
> > > > > > > > +      * The hardware provides a counter that is feed by the source clock.
> > > > > > > > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > > > > > > > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > > > > > > > +      * Thus the period_ns and duty_ns calculation formula should be:
> > > > > > > > +      * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
> > > > > > > > +      * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
> > > > > > > > +      */
> > > > > > > > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> > > > > > > > +     prescale = val & SPRD_PWM_PRESCALE_MSK;
> > > > > > > > +     tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > > > > > > > +     state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > > > > > > +
> > > > > > > > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> > > > > > > > +     duty = val & SPRD_PWM_DUTY_MSK;
> > > > > > > > +     tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> > > > > > > > +     state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > > > > > > +
> > > > > > > > +     /* Disable PWM clocks if the PWM channel is not in enable state. */
> > > > > > > > +     if (!state->enabled)
> > > > > > > > +             clk_bulk_disable_unprepare(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> > > > > > > > +                        int duty_ns, int period_ns)
> > > > > > > > +{
> > > > > > > > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > > > > > > +     u32 prescale, duty;
> > > > > > > > +     u64 tmp;
> > > > > > > > +
> > > > > > > > +     /*
> > > > > > > > +      * The hardware provides a counter that is feed by the source clock.
> > > > > > > > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > > > > > > > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > > > > > > > +      *
> > > > > > > > +      * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
> > > > > > >
> > > > > > > Did you spend some thoughts about how wrong your period can get because
> > > > > > > of that "lazyness"?
> > > > > > >
> > > > > > > Let's assume a clk rate of 100/3 MHz. Then the available period lengths
> > > > > > > are:
> > > > > > >
> > > > > > >         PRESCALE =  0  ->  period =   7.65 µs
> > > > > > >         PRESCALE =  1  ->  period =  15.30 µs
> > > > > > >         ...
> > > > > > >         PRESCALE = 17  ->  period = 137.70 µs
> > > > > > >         PRESCALE = 18  ->  period = 145.35 µs
> > > > > > >
> > > > > > > So the error can be up to (nearly) 7.65 µs (or in general
> > > > > >
> > > > > > Yes, but for our use case (pwm backlight), the precision can meet our
> > > > > > requirement. Moreover, we usually do not change the period, just
> > > > > > adjust the duty to change the back light.
> > > > >
> > > > > Is this a license requirement for you SoC to only drive a backlight with
> > > > > the PWM? The idea of having a PWM driver on your platform is that it can
> > > > > also be used to control a step motor or a laser.
> > > >
> > > > Not a license requirement. Until now we have not got any higher
> > > > precision requirements, and we've run this driver for many years in
> > > > our downstream kernel.
> > >
> > > I understood that you're not ambitious to do something better than "it
> > > worked for years".
> >
> > How do you know that?
>
> I showed you how you could match the requested PWM output better and
> you refused telling it worked for years and the added precision isn't
> necessary for a backlight.

Please I said the reason, it is not that I do not want a better
precision. The problem is we do not know how much precision to be
asked by users if no use case, and if improve the precision, which
means we should not keep MOD as a constant and suitable value, we
should do more mathematics to get a suitable MOD and DUTY, but there
is no necessary for now.

> > If there are some cases expect a higher precision, then we can analyze
> > how precision asked by the user, then we have a goal to improve it,
> > even improve the hardware. But now, I said there are no these use
> > cases, why I should add more mathematics to increase load and
> > complication.
> >
> > > > > > > PRESCALE = 18 and MOD = 254 you get a period of 144.78 µs and so the
> > > > > > > error is only 0.56 µs which is a factor of 13 better.
> > > > > > >
> > > > > > > Hmm.
> > > > > > >
> > > > > > > > +      * The value for PRESCALE is selected such that the resulting period
> > > > > > > > +      * gets the maximal length not bigger than the requested one with the
> > > > > > > > +      * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
> > > > > > > > +      */
> > > > > > > > +     duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> > > > > > >
> > > > > > > I wonder if you loose some precision here as you use period_ns but might
> > > > > > > actually implement a shorter period.
> > > > > > >
> > > > > > > Quick example, again consider clk_rate = 100 / 3 MHz,
> > > > > > > period_ns = 145340, duty_ns = 72670. Then you end up with
> > > > > > >
> > > > > > >         PRESCALE = 17
> > > > > > >         MOD = 255
> > > > > > >         DUTY = 127
> > > > > >
> > > > > > Incorrect, we will get PRESCALE = 18,  MOD = 255, DUTY = 127.
> > > > > >
> > > > > > > That corresponds to period_ns = 137700, duty_ns = 68580. With DUTY = 134
> > > > > > > you get 72360 ns which is still smaller than the requested 72670 ns.
> > > > > >
> > > > > > Incorrect, with DUTY = 134 (PRESCALE = 18  ->  period = 145.35 µs),
> > > > > > duty_ns = 76380ns
> > > > >
> > > > > Yes, as above. When using rounding-closest your error is not in [0, 7.65
> > > > > µs] but in [-3.825 µs, 3.825 µs]. Doesn't make it better.
> > > >
> > > > Actually our use case really dose not care about this error.
> > >
> > > I assume that Thierry will apply your patch anyhow. But be prepared that
> > > you get a patch from me then to improve precision. It would be a waste
> > > of resources not to do that after doing all the necessary math already.
> >
> > Glad to see your improvement without introducing complicated and more
> > mathematics.
>
> I don't understand you. Either you or me will improve the precision. The
> maths is the same for both cases. I would prefer you do it, otherwise I
> will have the problem later that I must get you to invest the time to
> test or I'd have to argue the change to go in untested.
>
> > > > > all lowlevel drivers. If you do this now I won't bother you later when
> > > > > the requirement is implemented in your driver. And the comment helps
> > > > > someone who evaluates your SoC to judge if there is still work to do if
> > > > > they have higher requirements for the PWM.
> > > >
> > > > So what you asked is something like below, right?
> > > > diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
> > > > index 96f8aa0..1d3db94 100644
> > > > --- a/drivers/pwm/pwm-sprd.c
> > > > +++ b/drivers/pwm/pwm-sprd.c
> > > > @@ -103,12 +103,12 @@ static void sprd_pwm_get_state(struct pwm_chip
> > > > *chip, struct pwm_device *pwm,
> > > >         val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> > > >         prescale = val & SPRD_PWM_PRESCALE_MSK;
> > > >         tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > > > -       state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > > +       state->period = DIV_ROUND_UP_ULL(tmp, chn->clk_rate);
> > > >
> > > >         val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> > > >         duty = val & SPRD_PWM_DUTY_MSK;
> > > >         tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> > > > -       state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > > > +       state->duty_cycle = DIV_ROUND_UP_ULL(tmp, chn->clk_rate);
> > > >
> > > >         /* Disable PWM clocks if the PWM channel is not in enable state. */
> > > >         if (!state->enabled)
> > > > @@ -135,8 +135,8 @@ static int sprd_pwm_config(struct sprd_pwm_chip
> > > > *spc, struct pwm_device *pwm,
> > > >         duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> > > >
> > > >         tmp = (u64)chn->clk_rate * period_ns;
> > > > -       do_div(tmp, NSEC_PER_SEC);
> > > > -       prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
> > > > +       div = 1000000000ULL * SPRD_PWM_MOD_MAX;
> > > > +       prescale = div64_u64(tmp, div) - 1;
> > > >         if (prescale > SPRD_PWM_PRESCALE_MSK)
> > > >                 prescale = SPRD_PWM_PRESCALE_MSK;
> > >
> > > This goes in the right direction for sure.
> > >
> > > Without taking paper and pencil I wouldn't be surprised if the
> > > calculation of duty_cycle in .get_state didn't match the calculation of
> > > DUTY in .apply yet though.
> > >
> > > > But our MOD is constant, it did not help to improve the precision.
> > > > Instead, like you said, when period_ns = 145340, we will set PRESCALE
> > > > = 17, so in .get_state(), user will get period_ns = 137700 (error
> > > > is145340 -  137700).
> > > >
> > > > But if we use DIV_ROUND_CLOSEST, in .get_state(), user will get
> > > > period_ns = 145350 (error is 145350 -  145340).
> > >
> > > In this case DIV_ROUND_CLOSEST seems to get nearer to the requested
> > > value than when rounding down. But this example was constructed to show
> > > your original algorithm to be bad, and just because you modify your
> > > algorithm to perform better on that constructed example doesn't imply
> > > the new one is better. Moreover you implement a bigger period than
> > > requested which is something I intend to forbid in the future.
> > >
> > > And note that with PWMs there is no "objective" metric that can tell you
> > > which of two implementable outputs better match a given request. It
> > > depends on the use case, so the best we can do is to tell our users our
> > > metric and with that in mind they can create a request that then fits
> > > their needs.
> >
> > Yes, that should be asked by the use case, some cases do not care a
> > little bigger period than requested.
>
> So for some cases it is beneficial to be predictable and for other it
> isn't. So the only safe thing to do for a lowlevel driver is to be
> predictable always because it cannot (and shouldn't) tell if the current
> request is one of cases where precision matters.
>
> > As you said, what you asked did not get a consensus yet, so I'd like
> > to wait for Thierry's suggestion.
> >
> > > > > > > twice instead of once before. (I don't know what architecture your SoC
> > > > > > > uses, but compared to a multiplication a division is usually expensive.)
> > > > > > > Also the math is more complicated now as you have a round-down div and a
> > > > > > > round-closest div.
> > > > > > >
> > > > > > > My preference for how to fix that is to restore the behaviour of v2 that
> > > > > > > matches the comment and adapt .get_state() instead.
> > > > > >
> > > > > > Using DIV_ROUND_CLOSEST_ULL can get a same prescale which matches with
> > > > > > .get_state().
> > > > >
> > > > > I don't get you here. Do you say that with DIV_ROUND_CLOSEST_ULL you get
> > > > > the same result but DIV_ROUND_CLOSEST_ULL matches .get_state while
> > > > > rounding down doesn't? I cannot follow.
> > > >
> > > > Yes, that's what I mean.
> > >
> > > But that is logically broken. If both approaches yield the same
> > > results it cannot be true that exactly one of them matches the inverse
> > > of .get_state.
> >
> > What I mean is use DIV_ROUND_CLOSEST_ULL we can get a nearer value to
> > the requested like above example.
>
> But given that it's unclear if 137700 ns or 145350 ns is better when
> 145340 ns was requested this is not a strong argument to use
> DIV_ROUND_CLOSEST_ULL. With the global picture for the pwm framework in
> mind it is sensible to request the same rounding from all drivers to get
> a consistent behaviour. And I believe the maths with rounding down is
> easier than when rounding up or nearest. That's why I argue in this
> direction.

Let's wait for Thierry's suggestion to get a consensus firstly.

-- 
Baolin Wang
Best Regards

Re: [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support

[Uwe Kleine-König]

On Thu, Aug 15, 2019 at 07:05:53PM +0800, Baolin Wang wrote:
> On Thu, 15 Aug 2019 at 18:11, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > On Thu, Aug 15, 2019 at 05:34:02PM +0800, Baolin Wang wrote:
> > > On Thu, 15 Aug 2019 at 16:54, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Thu, Aug 15, 2019 at 04:16:32PM +0800, Baolin Wang wrote:
> > > > > On Thu, 15 Aug 2019 at 14:15, Uwe Kleine-König
> > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > On Thu, Aug 15, 2019 at 11:34:27AM +0800, Baolin Wang wrote:
> > > > > > > On Wed, 14 Aug 2019 at 23:03, Uwe Kleine-König
> > > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > > > On Wed, Aug 14, 2019 at 08:46:11PM +0800, Baolin Wang wrote:
> > > > > > > > > +      * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
> > > > > > > >
> > > > > > > > Did you spend some thoughts about how wrong your period can get because
> > > > > > > > of that "lazyness"?
> > > > > > > >
> > > > > > > > Let's assume a clk rate of 100/3 MHz. Then the available period lengths
> > > > > > > > are:
> > > > > > > >
> > > > > > > >         PRESCALE =  0  ->  period =   7.65 µs
> > > > > > > >         PRESCALE =  1  ->  period =  15.30 µs
> > > > > > > >         ...
> > > > > > > >         PRESCALE = 17  ->  period = 137.70 µs
> > > > > > > >         PRESCALE = 18  ->  period = 145.35 µs
> > > > > > > >
> > > > > > > > So the error can be up to (nearly) 7.65 µs (or in general
> > > > > > >
> > > > > > > Yes, but for our use case (pwm backlight), the precision can meet our
> > > > > > > requirement. Moreover, we usually do not change the period, just
> > > > > > > adjust the duty to change the back light.
> > > > > >
> > > > > > Is this a license requirement for you SoC to only drive a backlight with
> > > > > > the PWM? The idea of having a PWM driver on your platform is that it can
> > > > > > also be used to control a step motor or a laser.
> > > > >
> > > > > Not a license requirement. Until now we have not got any higher
> > > > > precision requirements, and we've run this driver for many years in
> > > > > our downstream kernel.
> > > >
> > > > I understood that you're not ambitious to do something better than "it
> > > > worked for years".
> > >
> > > How do you know that?
> >
> > I showed you how you could match the requested PWM output better and
> > you refused telling it worked for years and the added precision isn't
> > necessary for a backlight.
> 
> Please I said the reason, it is not that I do not want a better
> precision. The problem is we do not know how much precision to be
> asked by users if no use case

I don't understand the problem here. If you are asked for period =
145340 ns and configure the hardware to yield 137700 ns in reply to that
but you could provide 144780 ns I don't understand why you need a use
case as 144780 ns is objectively better than 137700 ns. A better match
has only upsides, it doesn't hurt people how don't care about a few
micro seconds in the one or the other direction. OK, your CPU needs a
few more cycles to find the better configuration but that's a poor
argument. With only a backlight as use case you could even hardcode
PRESCALE = 0 without any problems and have the needed calculations a bit
cheaper.

> > > What I mean is use DIV_ROUND_CLOSEST_ULL we can get a nearer value to
> > > the requested like above example.
> >
> > But given that it's unclear if 137700 ns or 145350 ns is better when
> > 145340 ns was requested this is not a strong argument to use
> > DIV_ROUND_CLOSEST_ULL. With the global picture for the pwm framework in
> > mind it is sensible to request the same rounding from all drivers to get
> > a consistent behaviour. And I believe the maths with rounding down is
> > easier than when rounding up or nearest. That's why I argue in this
> > direction.
> 
> Let's wait for Thierry's suggestion to get a consensus firstly.

OK. I'm not sure you want to wait until Thierry and I agree on a
solution here though :-)

Best regards
Uwe

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

Re: [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support

[Baolin Wang]

On Thu, 15 Aug 2019 at 20:25, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Thu, Aug 15, 2019 at 07:05:53PM +0800, Baolin Wang wrote:
> > On Thu, 15 Aug 2019 at 18:11, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > Hello,
> > >
> > > On Thu, Aug 15, 2019 at 05:34:02PM +0800, Baolin Wang wrote:
> > > > On Thu, 15 Aug 2019 at 16:54, Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > On Thu, Aug 15, 2019 at 04:16:32PM +0800, Baolin Wang wrote:
> > > > > > On Thu, 15 Aug 2019 at 14:15, Uwe Kleine-König
> > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > > On Thu, Aug 15, 2019 at 11:34:27AM +0800, Baolin Wang wrote:
> > > > > > > > On Wed, 14 Aug 2019 at 23:03, Uwe Kleine-König
> > > > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > > > > On Wed, Aug 14, 2019 at 08:46:11PM +0800, Baolin Wang wrote:
> > > > > > > > > > +      * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
> > > > > > > > >
> > > > > > > > > Did you spend some thoughts about how wrong your period can get because
> > > > > > > > > of that "lazyness"?
> > > > > > > > >
> > > > > > > > > Let's assume a clk rate of 100/3 MHz. Then the available period lengths
> > > > > > > > > are:
> > > > > > > > >
> > > > > > > > >         PRESCALE =  0  ->  period =   7.65 µs
> > > > > > > > >         PRESCALE =  1  ->  period =  15.30 µs
> > > > > > > > >         ...
> > > > > > > > >         PRESCALE = 17  ->  period = 137.70 µs
> > > > > > > > >         PRESCALE = 18  ->  period = 145.35 µs
> > > > > > > > >
> > > > > > > > > So the error can be up to (nearly) 7.65 µs (or in general
> > > > > > > >
> > > > > > > > Yes, but for our use case (pwm backlight), the precision can meet our
> > > > > > > > requirement. Moreover, we usually do not change the period, just
> > > > > > > > adjust the duty to change the back light.
> > > > > > >
> > > > > > > Is this a license requirement for you SoC to only drive a backlight with
> > > > > > > the PWM? The idea of having a PWM driver on your platform is that it can
> > > > > > > also be used to control a step motor or a laser.
> > > > > >
> > > > > > Not a license requirement. Until now we have not got any higher
> > > > > > precision requirements, and we've run this driver for many years in
> > > > > > our downstream kernel.
> > > > >
> > > > > I understood that you're not ambitious to do something better than "it
> > > > > worked for years".
> > > >
> > > > How do you know that?
> > >
> > > I showed you how you could match the requested PWM output better and
> > > you refused telling it worked for years and the added precision isn't
> > > necessary for a backlight.
> >
> > Please I said the reason, it is not that I do not want a better
> > precision. The problem is we do not know how much precision to be
> > asked by users if no use case
>
> I don't understand the problem here. If you are asked for period =
> 145340 ns and configure the hardware to yield 137700 ns in reply to that
> but you could provide 144780 ns I don't understand why you need a use
> case as 144780 ns is objectively better than 137700 ns. A better match

You are wrong, we will provide 145350 ns with
DIV_ROUND_CLOSEST_ULL()., which is better than your 144780.

> has only upsides, it doesn't hurt people how don't care about a few
> micro seconds in the one or the other direction. OK, your CPU needs a
> few more cycles to find the better configuration but that's a poor
> argument. With only a backlight as use case you could even hardcode
> PRESCALE = 0 without any problems and have the needed calculations a bit
> cheaper.
>
> > > > What I mean is use DIV_ROUND_CLOSEST_ULL we can get a nearer value to
> > > > the requested like above example.
> > >
> > > But given that it's unclear if 137700 ns or 145350 ns is better when
> > > 145340 ns was requested this is not a strong argument to use
> > > DIV_ROUND_CLOSEST_ULL. With the global picture for the pwm framework in
> > > mind it is sensible to request the same rounding from all drivers to get
> > > a consistent behaviour. And I believe the maths with rounding down is
> > > easier than when rounding up or nearest. That's why I argue in this
> > > direction.
> >
> > Let's wait for Thierry's suggestion to get a consensus firstly.
>
> OK. I'm not sure you want to wait until Thierry and I agree on a
> solution here though :-)
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |



-- 
Baolin Wang
Best Regards

Re: [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support

[Uwe Kleine-König]

Hello Baolin,

On Fri, Aug 16, 2019 at 10:44:41AM +0800, Baolin Wang wrote:
> On Thu, 15 Aug 2019 at 20:25, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > On Thu, Aug 15, 2019 at 07:05:53PM +0800, Baolin Wang wrote:
> > > On Thu, 15 Aug 2019 at 18:11, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > > > Hello,
> > > >
> > > > On Thu, Aug 15, 2019 at 05:34:02PM +0800, Baolin Wang wrote:
> > > > > On Thu, 15 Aug 2019 at 16:54, Uwe Kleine-König
> > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > On Thu, Aug 15, 2019 at 04:16:32PM +0800, Baolin Wang wrote:
> > > > > > > On Thu, 15 Aug 2019 at 14:15, Uwe Kleine-König
> > > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > > > On Thu, Aug 15, 2019 at 11:34:27AM +0800, Baolin Wang wrote:
> > > > > > > > > On Wed, 14 Aug 2019 at 23:03, Uwe Kleine-König
> > > > > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > > > > > On Wed, Aug 14, 2019 at 08:46:11PM +0800, Baolin Wang wrote:
> > > > > > > > > > > +      * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
> > > > > > > > > >
> > > > > > > > > > Did you spend some thoughts about how wrong your period can get because
> > > > > > > > > > of that "lazyness"?
> > > > > > > > > >
> > > > > > > > > > Let's assume a clk rate of 100/3 MHz. Then the available period lengths
> > > > > > > > > > are:
> > > > > > > > > >
> > > > > > > > > >         PRESCALE =  0  ->  period =   7.65 µs
> > > > > > > > > >         PRESCALE =  1  ->  period =  15.30 µs
> > > > > > > > > >         ...
> > > > > > > > > >         PRESCALE = 17  ->  period = 137.70 µs
> > > > > > > > > >         PRESCALE = 18  ->  period = 145.35 µs
> > > > > > > > > >
> > > > > > > > > > So the error can be up to (nearly) 7.65 µs (or in general
> > > > > > > > >
> > > > > > > > > Yes, but for our use case (pwm backlight), the precision can meet our
> > > > > > > > > requirement. Moreover, we usually do not change the period, just
> > > > > > > > > adjust the duty to change the back light.
> > > > > > > >
> > > > > > > > Is this a license requirement for you SoC to only drive a backlight with
> > > > > > > > the PWM? The idea of having a PWM driver on your platform is that it can
> > > > > > > > also be used to control a step motor or a laser.
> > > > > > >
> > > > > > > Not a license requirement. Until now we have not got any higher
> > > > > > > precision requirements, and we've run this driver for many years in
> > > > > > > our downstream kernel.
> > > > > >
> > > > > > I understood that you're not ambitious to do something better than "it
> > > > > > worked for years".
> > > > >
> > > > > How do you know that?
> > > >
> > > > I showed you how you could match the requested PWM output better and
> > > > you refused telling it worked for years and the added precision isn't
> > > > necessary for a backlight.
> > >
> > > Please I said the reason, it is not that I do not want a better
> > > precision. The problem is we do not know how much precision to be
> > > asked by users if no use case
> >
> > I don't understand the problem here. If you are asked for period =
> > 145340 ns and configure the hardware to yield 137700 ns in reply to that
> > but you could provide 144780 ns I don't understand why you need a use
> > case as 144780 ns is objectively better than 137700 ns. A better match
> 
> You are wrong, we will provide 145350 ns with
> DIV_ROUND_CLOSEST_ULL()., which is better than your 144780.

There are two problems with your statement:

 - You're ignoring the fact that I base my argumentation on not using
   DIV_ROUND_CLOSEST_ULL because it has downsides I pointed out to you.
   If my suggested algorithm targeted a closest match (and not
   closest-but-not-bigger) it would pick 145350 ns, too. (I didn't check
   if something in the interval [145331, 145349] could be achieved. If
   there is, this should of course be preferred.)
   I obviously would have to pick a different example request to show
   that "targeting nearest with MOD always 255" is for some requests
   worse (probably by a similar factor) than "targeting nearest".
   It's not surprising that your apple is a better apple than my orange.

 - It's not objective that when 145340 ns was requested 145350 ns is
   better than 144780 ns. Some approximations are obviously better than
   others, but these two are not comparable in a way that all PWM
   consumers agree.

I'm not perfect and I do mistakes of course. But I'm still convinced
that my argumentation here is right.

Best regards
Uwe

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

Re: [PATCH v3 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[Rob Herring]

On Wed, 14 Aug 2019 20:46:10 +0800, Baolin Wang wrote:
> Add Spreadtrum PWM controller documentation.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v2:
>  - Fix some typos.
>  - Move assigned-clocks to be optional.
> 
> Changes from v1:
>  - Use assigned-clock-parents and assigned-clocks to set PWM clock parent.
> ---
>  Documentation/devicetree/bindings/pwm/pwm-sprd.txt |   40 ++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>