[PATCH 0/7] pwm: jz4740: Driver update

[PATCH 0/7] pwm: jz4740: Driver update

[Paul Cercueil]

Hi,

Patches 1-5 come from a bigger patchset that was cut in smaller
pieces for easier integration to mainline.
(The patchset was https://lkml.org/lkml/2019/3/27/1837)

These patches are the exact same as the ones found in the patchset
shown above, with the exception of patch [1/7] which now uses
device_node_to_regmap(). This function was added in a prior patch,
now merged in the MIPS tree.

For that reason this patchset is based on the ingenic-tcu-v5.4 branch of
the MIPS tree
(git://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git).

Thanks,
-Paul

[PATCH 1/7] pwm: jz4740: Obtain regmap from parent node

[Paul Cercueil]

The TCU registers are shared between a handful of drivers, accessing
them through the same regmap.

While this driver is devicetree-compatible, it is never (as of now)
probed from devicetree, so this change does not introduce a ABI problem
with current devicetree files.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Mathieu Malaterre <malat@debian.org>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---
 drivers/pwm/Kconfig      |  1 +
 drivers/pwm/pwm-jz4740.c | 80 ++++++++++++++++++++++++++--------------
 2 files changed, 53 insertions(+), 28 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index a7e57516959e..cc4df0ec978a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -225,6 +225,7 @@ config PWM_IMX_TPM
 config PWM_JZ4740
 	tristate "Ingenic JZ47xx PWM support"
 	depends on MACH_INGENIC
+	select MFD_SYSCON
 	help
 	  Generic PWM framework driver for Ingenic JZ47xx based
 	  machines.
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index f901e8a0d33d..7aea5e0c6e18 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -8,18 +8,20 @@
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/kernel.h>
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
-
-#include <asm/mach-jz4740/timer.h>
+#include <linux/regmap.h>
 
 #define NUM_PWM 8
 
 struct jz4740_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clk;
+	struct regmap *map;
 };
 
 static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
@@ -29,6 +31,8 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
 
 static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
+	struct jz4740_pwm_chip *jz = to_jz4740(chip);
+
 	/*
 	 * Timers 0 and 1 are used for system tasks, so they are unavailable
 	 * for use as PWMs.
@@ -36,50 +40,53 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 	if (pwm->hwpwm < 2)
 		return -EBUSY;
 
-	jz4740_timer_start(pwm->hwpwm);
+	regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm));
 
 	return 0;
 }
 
 static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
+	struct jz4740_pwm_chip *jz = to_jz4740(chip);
 
-	jz4740_timer_stop(pwm->hwpwm);
+	regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm));
 }
 
 static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->pwm);
+	struct jz4740_pwm_chip *jz = to_jz4740(chip);
+
+	/* Enable PWM output */
+	regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
+			   TCU_TCSR_PWM_EN, TCU_TCSR_PWM_EN);
 
-	ctrl |= JZ_TIMER_CTRL_PWM_ENABLE;
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
-	jz4740_timer_enable(pwm->hwpwm);
+	/* Start counter */
+	regmap_write(jz->map, TCU_REG_TESR, BIT(pwm->hwpwm));
 
 	return 0;
 }
 
 static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
+	struct jz4740_pwm_chip *jz = to_jz4740(chip);
 
 	/*
 	 * Set duty > period. This trick allows the TCU channels in TCU2 mode to
 	 * properly return to their init level.
 	 */
-	jz4740_timer_set_duty(pwm->hwpwm, 0xffff);
-	jz4740_timer_set_period(pwm->hwpwm, 0x0);
+	regmap_write(jz->map, TCU_REG_TDHRc(pwm->hwpwm), 0xffff);
+	regmap_write(jz->map, TCU_REG_TDFRc(pwm->hwpwm), 0x0);
 
 	/*
 	 * Disable PWM output.
 	 * In TCU2 mode (channel 1/2 on JZ4750+), this must be done before the
 	 * counter is stopped, while in TCU1 mode the order does not matter.
 	 */
-	ctrl &= ~JZ_TIMER_CTRL_PWM_ENABLE;
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
+	regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
+			   TCU_TCSR_PWM_EN, 0);
 
 	/* Stop counter */
-	jz4740_timer_disable(pwm->hwpwm);
+	regmap_write(jz->map, TCU_REG_TECR, BIT(pwm->hwpwm));
 }
 
 static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -89,7 +96,6 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long long tmp;
 	unsigned long period, duty;
 	unsigned int prescaler = 0;
-	uint16_t ctrl;
 
 	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * state->period;
 	do_div(tmp, 1000000000);
@@ -112,26 +118,37 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	jz4740_pwm_disable(chip, pwm);
 
-	jz4740_timer_set_count(pwm->hwpwm, 0);
-	jz4740_timer_set_duty(pwm->hwpwm, duty);
-	jz4740_timer_set_period(pwm->hwpwm, period);
+	/* Set abrupt shutdown */
+	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
+			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
+
+	/* Set clock prescale */
+	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
+			   TCU_TCSR_PRESCALE_MASK,
+			   prescaler << TCU_TCSR_PRESCALE_LSB);
+
+	/* Reset counter to 0 */
+	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
 
-	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
-		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
+	/* Set duty */
+	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
 
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
+	/* Set period */
+	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
 
+	/* Set polarity */
 	switch (state->polarity) {
 	case PWM_POLARITY_NORMAL:
-		ctrl &= ~JZ_TIMER_CTRL_PWM_ACTIVE_LOW;
+		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
+				   TCU_TCSR_PWM_INITL_HIGH, 0);
 		break;
 	case PWM_POLARITY_INVERSED:
-		ctrl |= JZ_TIMER_CTRL_PWM_ACTIVE_LOW;
+		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
+				   TCU_TCSR_PWM_INITL_HIGH,
+				   TCU_TCSR_PWM_INITL_HIGH);
 		break;
 	}
 
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
-
 	if (state->enabled)
 		jz4740_pwm_enable(chip, pwm);
 
@@ -148,8 +165,9 @@ static const struct pwm_ops jz4740_pwm_ops = {
 static int jz4740_pwm_probe(struct platform_device *pdev)
 {
 	struct jz4740_pwm_chip *jz4740;
+	struct device *dev = &pdev->dev;
 
-	jz4740 = devm_kzalloc(&pdev->dev, sizeof(*jz4740), GFP_KERNEL);
+	jz4740 = devm_kzalloc(dev, sizeof(*jz4740), GFP_KERNEL);
 	if (!jz4740)
 		return -ENOMEM;
 
@@ -157,7 +175,13 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(jz4740->clk))
 		return PTR_ERR(jz4740->clk);
 
-	jz4740->chip.dev = &pdev->dev;
+	jz4740->map = device_node_to_regmap(dev->parent->of_node);
+	if (!jz4740->map) {
+		dev_err(dev, "regmap not found\n");
+		return -EINVAL;
+	}
+
+	jz4740->chip.dev = dev;
 	jz4740->chip.ops = &jz4740_pwm_ops;
 	jz4740->chip.npwm = NUM_PWM;
 	jz4740->chip.base = -1;
-- 
2.21.0.593.g511ec345e18

[PATCH 2/7] pwm: jz4740: Use clocks from TCU driver

[Paul Cercueil]

The ingenic-timer "TCU" driver provides us with clocks, that can be
(un)gated, reparented or reclocked from devicetree, instead of having
these settings hardcoded in this driver.

While this driver is devicetree-compatible, it is never (as of now)
probed from devicetree, so this change does not introduce a ABI problem
with current devicetree files.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Mathieu Malaterre <malat@debian.org>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---
 drivers/pwm/Kconfig      |  1 +
 drivers/pwm/pwm-jz4740.c | 40 ++++++++++++++++++++++++++--------------
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index cc4df0ec978a..d2557c6fcf65 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -225,6 +225,7 @@ config PWM_IMX_TPM
 config PWM_JZ4740
 	tristate "Ingenic JZ47xx PWM support"
 	depends on MACH_INGENIC
+	depends on COMMON_CLK
 	select MFD_SYSCON
 	help
 	  Generic PWM framework driver for Ingenic JZ47xx based
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 7aea5e0c6e18..6ec8794d3b99 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -20,7 +20,6 @@
 
 struct jz4740_pwm_chip {
 	struct pwm_chip chip;
-	struct clk *clk;
 	struct regmap *map;
 };
 
@@ -32,6 +31,9 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
 static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct jz4740_pwm_chip *jz = to_jz4740(chip);
+	struct clk *clk;
+	char clk_name[16];
+	int ret;
 
 	/*
 	 * Timers 0 and 1 are used for system tasks, so they are unavailable
@@ -40,16 +42,29 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 	if (pwm->hwpwm < 2)
 		return -EBUSY;
 
-	regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm));
+	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
+
+	clk = clk_get(chip->dev, clk_name);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		clk_put(clk);
+		return ret;
+	}
+
+	pwm_set_chip_data(pwm, clk);
 
 	return 0;
 }
 
 static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	struct jz4740_pwm_chip *jz = to_jz4740(chip);
+	struct clk *clk = pwm_get_chip_data(pwm);
 
-	regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm));
+	clk_disable_unprepare(clk);
+	clk_put(clk);
 }
 
 static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -93,16 +108,20 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			    struct pwm_state *state)
 {
 	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
+	struct clk *clk = pwm_get_chip_data(pwm),
+		   *parent_clk = clk_get_parent(clk);
+	unsigned long rate, period, duty;
 	unsigned long long tmp;
-	unsigned long period, duty;
 	unsigned int prescaler = 0;
 
-	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * state->period;
+	rate = clk_get_rate(parent_clk);
+	tmp = (unsigned long long)rate * state->period;
 	do_div(tmp, 1000000000);
 	period = tmp;
 
 	while (period > 0xffff && prescaler < 6) {
 		period >>= 2;
+		rate >>= 2;
 		++prescaler;
 	}
 
@@ -122,10 +141,7 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
 			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
 
-	/* Set clock prescale */
-	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
-			   TCU_TCSR_PRESCALE_MASK,
-			   prescaler << TCU_TCSR_PRESCALE_LSB);
+	clk_set_rate(clk, rate);
 
 	/* Reset counter to 0 */
 	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
@@ -171,10 +187,6 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
 	if (!jz4740)
 		return -ENOMEM;
 
-	jz4740->clk = devm_clk_get(&pdev->dev, "ext");
-	if (IS_ERR(jz4740->clk))
-		return PTR_ERR(jz4740->clk);
-
 	jz4740->map = device_node_to_regmap(dev->parent->of_node);
 	if (!jz4740->map) {
 		dev_err(dev, "regmap not found\n");
-- 
2.21.0.593.g511ec345e18

[PATCH 3/7] pwm: jz4740: Drop dependency on MACH_INGENIC

[Paul Cercueil]

Depending on MACH_INGENIC prevent us from creating a generic kernel that
works on more than one MIPS board. Instead, we just depend on MIPS being
set.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/pwm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index d2557c6fcf65..82a75e0b72e5 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -224,7 +224,7 @@ config PWM_IMX_TPM
 
 config PWM_JZ4740
 	tristate "Ingenic JZ47xx PWM support"
-	depends on MACH_INGENIC
+	depends on MIPS
 	depends on COMMON_CLK
 	select MFD_SYSCON
 	help
-- 
2.21.0.593.g511ec345e18

[PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Paul Cercueil]

The previous algorithm hardcoded details about how the TCU clocks work.
The new algorithm will use clk_round_rate to find the perfect clock rate
for the PWM channel.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Mathieu Malaterre <malat@debian.org>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---
 drivers/pwm/pwm-jz4740.c | 60 +++++++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 16 deletions(-)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 6ec8794d3b99..f20dc2e19240 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -110,24 +110,56 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
 	struct clk *clk = pwm_get_chip_data(pwm),
 		   *parent_clk = clk_get_parent(clk);
-	unsigned long rate, period, duty;
+	unsigned long rate, parent_rate, period, duty;
 	unsigned long long tmp;
-	unsigned int prescaler = 0;
+	int ret;
 
-	rate = clk_get_rate(parent_clk);
-	tmp = (unsigned long long)rate * state->period;
-	do_div(tmp, 1000000000);
-	period = tmp;
+	parent_rate = clk_get_rate(parent_clk);
+
+	jz4740_pwm_disable(chip, pwm);
 
-	while (period > 0xffff && prescaler < 6) {
-		period >>= 2;
-		rate >>= 2;
-		++prescaler;
+	/* Reset the clock to the maximum rate, and we'll reduce it if needed */
+	ret = clk_set_max_rate(clk, parent_rate);
+	if (ret) {
+		dev_err(chip->dev, "Unable to set max rate: %d\n", ret);
+		return ret;
 	}
 
-	if (prescaler == 6)
-		return -EINVAL;
+	ret = clk_set_rate(clk, parent_rate);
+	if (ret) {
+		dev_err(chip->dev, "Unable to reset to parent rate (%lu Hz)",
+			parent_rate);
+		return ret;
+	}
+
+	/*
+	 * Limit the clock to a maximum rate that still gives us a period value
+	 * which fits in 16 bits.
+	 */
+	tmp = 0xffffull * NSEC_PER_SEC;
+	do_div(tmp, state->period);
 
+	ret = clk_set_max_rate(clk, tmp);
+	if (ret) {
+		dev_err(chip->dev, "Unable to set max rate: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Read back the clock rate, as it may have been modified by
+	 * clk_set_max_rate()
+	 */
+	rate = clk_get_rate(clk);
+
+	if (rate != parent_rate)
+		dev_dbg(chip->dev, "PWM clock updated to %lu Hz\n", rate);
+
+	/* Calculate period value */
+	tmp = (unsigned long long)rate * state->period;
+	do_div(tmp, NSEC_PER_SEC);
+	period = (unsigned long)tmp;
+
+	/* Calculate duty value */
 	tmp = (unsigned long long)period * state->duty_cycle;
 	do_div(tmp, state->period);
 	duty = period - tmp;
@@ -135,14 +167,10 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (duty >= period)
 		duty = period - 1;
 
-	jz4740_pwm_disable(chip, pwm);
-
 	/* Set abrupt shutdown */
 	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
 			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
 
-	clk_set_rate(clk, rate);
-
 	/* Reset counter to 0 */
 	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
 
-- 
2.21.0.593.g511ec345e18

[PATCH 5/7] pwm: jz4740: Allow selection of PWM channels 0 and 1

[Paul Cercueil]

The TCU channels 0 and 1 were previously reserved for system tasks, and
thus unavailable for PWM.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Mathieu Malaterre <malat@debian.org>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---
 drivers/pwm/pwm-jz4740.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index f20dc2e19240..85e2110aae4f 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -28,6 +28,19 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
 	return container_of(chip, struct jz4740_pwm_chip, chip);
 }
 
+static bool jz4740_pwm_can_use_chn(struct jz4740_pwm_chip *jz,
+				   unsigned int channel)
+{
+	/* Enable all TCU channels for PWM use by default except channels 0/1 */
+	u32 pwm_channels_mask = GENMASK(NUM_PWM - 1, 2);
+
+	device_property_read_u32(jz->chip.dev->parent,
+				 "ingenic,pwm-channels-mask",
+				 &pwm_channels_mask);
+
+	return !!(pwm_channels_mask & BIT(channel));
+}
+
 static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct jz4740_pwm_chip *jz = to_jz4740(chip);
@@ -35,11 +48,7 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 	char clk_name[16];
 	int ret;
 
-	/*
-	 * Timers 0 and 1 are used for system tasks, so they are unavailable
-	 * for use as PWMs.
-	 */
-	if (pwm->hwpwm < 2)
+	if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
 		return -EBUSY;
 
 	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
-- 
2.21.0.593.g511ec345e18

[PATCH 6/7] pwm: jz4740: Make PWM start with the active part

[Paul Cercueil]

The PWM will always start with the inactive part. To counter that,
when PWM is enabled we switch the configured polarity, and use
'period - duty + 1' as the real duty.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/pwm/pwm-jz4740.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 85e2110aae4f..8df898429d47 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -121,6 +121,7 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		   *parent_clk = clk_get_parent(clk);
 	unsigned long rate, parent_rate, period, duty;
 	unsigned long long tmp;
+	bool polarity_inversed;
 	int ret;
 
 	parent_rate = clk_get_rate(parent_clk);
@@ -183,24 +184,27 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	/* Reset counter to 0 */
 	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
 
-	/* Set duty */
-	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
-
 	/* Set period */
 	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
 
+	/*
+	 * The PWM will always start with the inactive part. To counter that,
+	 * when PWM is enabled we switch the configured polarity, and use
+	 * 'period - duty + 1' as the real duty.
+	 */
+
+	/* Set duty */
+	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), period - duty + 1);
+
 	/* Set polarity */
-	switch (state->polarity) {
-	case PWM_POLARITY_NORMAL:
+	polarity_inversed = state->polarity == PWM_POLARITY_INVERSED;
+	if (!polarity_inversed ^ state->enabled)
 		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
 				   TCU_TCSR_PWM_INITL_HIGH, 0);
-		break;
-	case PWM_POLARITY_INVERSED:
+	else
 		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
 				   TCU_TCSR_PWM_INITL_HIGH,
 				   TCU_TCSR_PWM_INITL_HIGH);
-		break;
-	}
 
 	if (state->enabled)
 		jz4740_pwm_enable(chip, pwm);
-- 
2.21.0.593.g511ec345e18

[PATCH 7/7] pwm: jz4740: document known limitations

[Paul Cercueil]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

The jz4740 PMW implementation doesn't fulfill the (up to now
insufficiently documented) requirements of the PWM API. At least
document them in the driver.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/pwm/pwm-jz4740.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 8df898429d47..b331a73a9d5d 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -2,6 +2,10 @@
 /*
  *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
  *  JZ4740 platform PWM support
+ *
+ * Limitations:
+ * - The .apply callback doesn't complete the currently running period before
+ *   reconfiguring the hardware.
  */
 
 #include <linux/clk.h>
-- 
2.21.0.593.g511ec345e18

Re: [PATCH 3/7] pwm: jz4740: Drop dependency on MACH_INGENIC

[Uwe Kleine-König]

On Fri, Aug 09, 2019 at 02:30:27PM +0200, Paul Cercueil wrote:
> Depending on MACH_INGENIC prevent us from creating a generic kernel that
> works on more than one MIPS board. Instead, we just depend on MIPS being
> set.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/pwm/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index d2557c6fcf65..82a75e0b72e5 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -224,7 +224,7 @@ config PWM_IMX_TPM
>  
>  config PWM_JZ4740
>  	tristate "Ingenic JZ47xx PWM support"
> -	depends on MACH_INGENIC
> +	depends on MIPS

If this isn't actually useful on MIPS without MACH_INGENIC this is
better expressed using:

	depends on MIPS
	depends on MACH_INGENIC || COMPILE_TEST

This way some configuring a mips kernel without INGENIC isn't bothered
by this question.

Best regards
Uwe

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

Re: [PATCH 1/7] pwm: jz4740: Obtain regmap from parent node

[Uwe Kleine-König]

On Fri, Aug 09, 2019 at 02:30:25PM +0200, Paul Cercueil wrote:
> The TCU registers are shared between a handful of drivers, accessing
> them through the same regmap.
> 
> While this driver is devicetree-compatible, it is never (as of now)
> probed from devicetree, so this change does not introduce a ABI problem
> with current devicetree files.

If you adapt the binding also update the binding doc please.

> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Mathieu Malaterre <malat@debian.org>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>

nitpick: put your S-o-b line after the other tags you added.

> ---
>  drivers/pwm/Kconfig      |  1 +
>  drivers/pwm/pwm-jz4740.c | 80 ++++++++++++++++++++++++++--------------
>  2 files changed, 53 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a7e57516959e..cc4df0ec978a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -225,6 +225,7 @@ config PWM_IMX_TPM
>  config PWM_JZ4740
>  	tristate "Ingenic JZ47xx PWM support"
>  	depends on MACH_INGENIC
> +	select MFD_SYSCON
>  	help
>  	  Generic PWM framework driver for Ingenic JZ47xx based
>  	  machines.
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index f901e8a0d33d..7aea5e0c6e18 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -8,18 +8,20 @@
>  #include <linux/err.h>
>  #include <linux/gpio.h>
>  #include <linux/kernel.h>
> +#include <linux/mfd/ingenic-tcu.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
> -
> -#include <asm/mach-jz4740/timer.h>
> +#include <linux/regmap.h>
>  
>  #define NUM_PWM 8
>  
>  struct jz4740_pwm_chip {
>  	struct pwm_chip chip;
>  	struct clk *clk;
> +	struct regmap *map;
>  };
>  
>  static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
> @@ -29,6 +31,8 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
>  
>  static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
> +	struct jz4740_pwm_chip *jz = to_jz4740(chip);
> +
>  	/*
>  	 * Timers 0 and 1 are used for system tasks, so they are unavailable
>  	 * for use as PWMs.
> @@ -36,50 +40,53 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  	if (pwm->hwpwm < 2)
>  		return -EBUSY;
>  
> -	jz4740_timer_start(pwm->hwpwm);
> +	regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm));

jz4740_timer_start does

	writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR);

with

#define JZ_REG_TIMER_STOP_CLEAR         0x2C

and

#define TCU_REG_TSCR            0x3c

I wonder why the offsets are different.

>  
>  	return 0;
>  }
>  
>  static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
> -	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
> +	struct jz4740_pwm_chip *jz = to_jz4740(chip);
>  
> -	jz4740_timer_stop(pwm->hwpwm);
> +	regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm));

jz4740_timer_set_ctrl writes to offset (((pwm->hwpwm) * 0x10) + 0x3C)
and jz4740_timer_stop to offset 0x1c. The regmap_write doesn't do both
of them but instead writes to offset 0x3c.

So this doesn't seem to be a 1:1 conversion. This either needs fixing,
splitting into several patches or a better commit log.

Stopping my review here.

Best regards
Uwe

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

Re: [PATCH 2/7] pwm: jz4740: Use clocks from TCU driver

[Uwe Kleine-König]

On Fri, Aug 09, 2019 at 02:30:26PM +0200, Paul Cercueil wrote:
> The ingenic-timer "TCU" driver provides us with clocks, that can be
> (un)gated, reparented or reclocked from devicetree, instead of having
> these settings hardcoded in this driver.
> 
> While this driver is devicetree-compatible, it is never (as of now)
> probed from devicetree, so this change does not introduce a ABI problem
> with current devicetree files.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Mathieu Malaterre <malat@debian.org>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>
> ---
>  drivers/pwm/Kconfig      |  1 +
>  drivers/pwm/pwm-jz4740.c | 40 ++++++++++++++++++++++++++--------------
>  2 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index cc4df0ec978a..d2557c6fcf65 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -225,6 +225,7 @@ config PWM_IMX_TPM
>  config PWM_JZ4740
>  	tristate "Ingenic JZ47xx PWM support"
>  	depends on MACH_INGENIC
> +	depends on COMMON_CLK
>  	select MFD_SYSCON
>  	help
>  	  Generic PWM framework driver for Ingenic JZ47xx based
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index 7aea5e0c6e18..6ec8794d3b99 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -20,7 +20,6 @@
>  
>  struct jz4740_pwm_chip {
>  	struct pwm_chip chip;
> -	struct clk *clk;
>  	struct regmap *map;
>  };
>  
> @@ -32,6 +31,9 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
>  static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct jz4740_pwm_chip *jz = to_jz4740(chip);
> +	struct clk *clk;
> +	char clk_name[16];
> +	int ret;
>  
>  	/*
>  	 * Timers 0 and 1 are used for system tasks, so they are unavailable
> @@ -40,16 +42,29 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  	if (pwm->hwpwm < 2)
>  		return -EBUSY;
>  
> -	regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm));
> +	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);

This undoes stuff from the previous patch. Maybe swap the order of the
two patches?

Other than that the patch looks fine on a quick glance.

Best regards
Uwe

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

Re: [PATCH 1/7] pwm: jz4740: Obtain regmap from parent node

[Paul Cercueil]

Hi Uwe,


Le ven. 9 août 2019 à 18:51, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> On Fri, Aug 09, 2019 at 02:30:25PM +0200, Paul Cercueil wrote:
>>  The TCU registers are shared between a handful of drivers, accessing
>>  them through the same regmap.
>> 
>>  While this driver is devicetree-compatible, it is never (as of now)
>>  probed from devicetree, so this change does not introduce a ABI 
>> problem
>>  with current devicetree files.
> 
> If you adapt the binding also update the binding doc please.

It's already updated, in mips-next, so it'll be in the next -rc1.


>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Tested-by: Mathieu Malaterre <malat@debian.org>
>>  Tested-by: Artur Rojek <contact@artur-rojek.eu>
> 
> nitpick: put your S-o-b line after the other tags you added.
> 
>>  ---
>>   drivers/pwm/Kconfig      |  1 +
>>   drivers/pwm/pwm-jz4740.c | 80 
>> ++++++++++++++++++++++++++--------------
>>   2 files changed, 53 insertions(+), 28 deletions(-)
>> 
>>  diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>  index a7e57516959e..cc4df0ec978a 100644
>>  --- a/drivers/pwm/Kconfig
>>  +++ b/drivers/pwm/Kconfig
>>  @@ -225,6 +225,7 @@ config PWM_IMX_TPM
>>   config PWM_JZ4740
>>   	tristate "Ingenic JZ47xx PWM support"
>>   	depends on MACH_INGENIC
>>  +	select MFD_SYSCON
>>   	help
>>   	  Generic PWM framework driver for Ingenic JZ47xx based
>>   	  machines.
>>  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
>>  index f901e8a0d33d..7aea5e0c6e18 100644
>>  --- a/drivers/pwm/pwm-jz4740.c
>>  +++ b/drivers/pwm/pwm-jz4740.c
>>  @@ -8,18 +8,20 @@
>>   #include <linux/err.h>
>>   #include <linux/gpio.h>
>>   #include <linux/kernel.h>
>>  +#include <linux/mfd/ingenic-tcu.h>
>>  +#include <linux/mfd/syscon.h>
>>   #include <linux/module.h>
>>   #include <linux/of_device.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/pwm.h>
>>  -
>>  -#include <asm/mach-jz4740/timer.h>
>>  +#include <linux/regmap.h>
>> 
>>   #define NUM_PWM 8
>> 
>>   struct jz4740_pwm_chip {
>>   	struct pwm_chip chip;
>>   	struct clk *clk;
>>  +	struct regmap *map;
>>   };
>> 
>>   static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip 
>> *chip)
>>  @@ -29,6 +31,8 @@ static inline struct jz4740_pwm_chip 
>> *to_jz4740(struct pwm_chip *chip)
>> 
>>   static int jz4740_pwm_request(struct pwm_chip *chip, struct 
>> pwm_device *pwm)
>>   {
>>  +	struct jz4740_pwm_chip *jz = to_jz4740(chip);
>>  +
>>   	/*
>>   	 * Timers 0 and 1 are used for system tasks, so they are 
>> unavailable
>>   	 * for use as PWMs.
>>  @@ -36,50 +40,53 @@ static int jz4740_pwm_request(struct pwm_chip 
>> *chip, struct pwm_device *pwm)
>>   	if (pwm->hwpwm < 2)
>>   		return -EBUSY;
>> 
>>  -	jz4740_timer_start(pwm->hwpwm);
>>  +	regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm));
> 
> jz4740_timer_start does
> 
> 	writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR);
> 
> with
> 
> #define JZ_REG_TIMER_STOP_CLEAR         0x2C
> 
> and
> 
> #define TCU_REG_TSCR            0x3c
> 
> I wonder why the offsets are different.

The offset are different because the base is different.


>> 
>>   	return 0;
>>   }
>> 
>>   static void jz4740_pwm_free(struct pwm_chip *chip, struct 
>> pwm_device *pwm)
>>   {
>>  -	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
>>  +	struct jz4740_pwm_chip *jz = to_jz4740(chip);
>> 
>>  -	jz4740_timer_stop(pwm->hwpwm);
>>  +	regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm));
> 
> jz4740_timer_set_ctrl writes to offset (((pwm->hwpwm) * 0x10) + 0x3C)
> and jz4740_timer_stop to offset 0x1c. The regmap_write doesn't do both
> of them but instead writes to offset 0x3c.

I guess it should have been TCU_REG_TSSR ("Timer Stop Set Register") and
I didn't notice because the next patch drops this write anyway.

I'll do as you suggested in your other reply and swap the two patches if
it makes things easier, it'll get rid of this write.


> So this doesn't seem to be a 1:1 conversion. This either needs fixing,
> splitting into several patches or a better commit log.
> 
> Stopping my review here.
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Uwe Kleine-König]

On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil wrote:
> The previous algorithm hardcoded details about how the TCU clocks work.
> The new algorithm will use clk_round_rate to find the perfect clock rate
> for the PWM channel.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Mathieu Malaterre <malat@debian.org>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>
> ---
>  drivers/pwm/pwm-jz4740.c | 60 +++++++++++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index 6ec8794d3b99..f20dc2e19240 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -110,24 +110,56 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
>  	struct clk *clk = pwm_get_chip_data(pwm),
>  		   *parent_clk = clk_get_parent(clk);
> -	unsigned long rate, period, duty;
> +	unsigned long rate, parent_rate, period, duty;
>  	unsigned long long tmp;
> -	unsigned int prescaler = 0;
> +	int ret;
>  
> -	rate = clk_get_rate(parent_clk);
> -	tmp = (unsigned long long)rate * state->period;
> -	do_div(tmp, 1000000000);
> -	period = tmp;
> +	parent_rate = clk_get_rate(parent_clk);
> +
> +	jz4740_pwm_disable(chip, pwm);
>  
> -	while (period > 0xffff && prescaler < 6) {
> -		period >>= 2;
> -		rate >>= 2;
> -		++prescaler;
> +	/* Reset the clock to the maximum rate, and we'll reduce it if needed */
> +	ret = clk_set_max_rate(clk, parent_rate);

What is the purpose of this call? IIUC this limits the allowed range of
rates for clk. I assume the idea is to prevent other consumers to change
the rate in a way that makes it unsuitable for this pwm. But this only
makes sense if you had a notifier for clk changes, doesn't it? I'm
confused.

I think this doesn't match the commit log, you didn't even introduced a
call to clk_round_rate().

> +	if (ret) {
> +		dev_err(chip->dev, "Unable to set max rate: %d\n", ret);
> +		return ret;
>  	}
>  
> -	if (prescaler == 6)
> -		return -EINVAL;
> +	ret = clk_set_rate(clk, parent_rate);
> +	if (ret) {
> +		dev_err(chip->dev, "Unable to reset to parent rate (%lu Hz)",
> +			parent_rate);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Limit the clock to a maximum rate that still gives us a period value
> +	 * which fits in 16 bits.
> +	 */
> +	tmp = 0xffffull * NSEC_PER_SEC;
> +	do_div(tmp, state->period);
>  
> +	ret = clk_set_max_rate(clk, tmp);

And now you change the maximal rate again?

> +	if (ret) {
> +		dev_err(chip->dev, "Unable to set max rate: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Read back the clock rate, as it may have been modified by
> +	 * clk_set_max_rate()
> +	 */
> +	rate = clk_get_rate(clk);
> +
> +	if (rate != parent_rate)
> +		dev_dbg(chip->dev, "PWM clock updated to %lu Hz\n", rate);
> +
> +	/* Calculate period value */
> +	tmp = (unsigned long long)rate * state->period;
> +	do_div(tmp, NSEC_PER_SEC);
> +	period = (unsigned long)tmp;
> +
> +	/* Calculate duty value */
>  	tmp = (unsigned long long)period * state->duty_cycle;
>  	do_div(tmp, state->period);
>  	duty = period - tmp;
> @@ -135,14 +167,10 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (duty >= period)
>  		duty = period - 1;
>  
> -	jz4740_pwm_disable(chip, pwm);
> -
>  	/* Set abrupt shutdown */
>  	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>  			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
>  
> -	clk_set_rate(clk, rate);
> -

It's not obvious to me why removing these two lines belong in the
current patch.

Best regards
Uwe

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

Re: [PATCH 6/7] pwm: jz4740: Make PWM start with the active part

[Uwe Kleine-König]

On Fri, Aug 09, 2019 at 02:30:30PM +0200, Paul Cercueil wrote:
> The PWM will always start with the inactive part. To counter that,
> when PWM is enabled we switch the configured polarity, and use
> 'period - duty + 1' as the real duty.

Where does the + 1 come from? This looks wrong. (So if duty=0 is
requested you use duty = period + 1?)

> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/pwm/pwm-jz4740.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index 85e2110aae4f..8df898429d47 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -121,6 +121,7 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  		   *parent_clk = clk_get_parent(clk);
>  	unsigned long rate, parent_rate, period, duty;
>  	unsigned long long tmp;
> +	bool polarity_inversed;
>  	int ret;
>  
>  	parent_rate = clk_get_rate(parent_clk);
> @@ -183,24 +184,27 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	/* Reset counter to 0 */
>  	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
>  
> -	/* Set duty */
> -	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
> -
>  	/* Set period */
>  	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
>  
> +	/*
> +	 * The PWM will always start with the inactive part. To counter that,
> +	 * when PWM is enabled we switch the configured polarity, and use
> +	 * 'period - duty + 1' as the real duty.
> +	 */
> +
> +	/* Set duty */
> +	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), period - duty + 1);
> +

Before you set duty first, then period, now you do it the other way
round. Is there a good reason?

>  	/* Set polarity */
> -	switch (state->polarity) {
> -	case PWM_POLARITY_NORMAL:
> +	polarity_inversed = state->polarity == PWM_POLARITY_INVERSED;
> +	if (!polarity_inversed ^ state->enabled)

Why does state->enabled suddenly matter here?

>  		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>  				   TCU_TCSR_PWM_INITL_HIGH, 0);
> -		break;
> -	case PWM_POLARITY_INVERSED:
> +	else
>  		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>  				   TCU_TCSR_PWM_INITL_HIGH,
>  				   TCU_TCSR_PWM_INITL_HIGH);
> -		break;
> -	}
>  
>  	if (state->enabled)
>  		jz4740_pwm_enable(chip, pwm);

Best regards
Uwe

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

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Paul Cercueil]

Le ven. 9 août 2019 à 19:05, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil wrote:
>>  The previous algorithm hardcoded details about how the TCU clocks 
>> work.
>>  The new algorithm will use clk_round_rate to find the perfect clock 
>> rate
>>  for the PWM channel.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Tested-by: Mathieu Malaterre <malat@debian.org>
>>  Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>  ---
>>   drivers/pwm/pwm-jz4740.c | 60 
>> +++++++++++++++++++++++++++++-----------
>>   1 file changed, 44 insertions(+), 16 deletions(-)
>> 
>>  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
>>  index 6ec8794d3b99..f20dc2e19240 100644
>>  --- a/drivers/pwm/pwm-jz4740.c
>>  +++ b/drivers/pwm/pwm-jz4740.c
>>  @@ -110,24 +110,56 @@ static int jz4740_pwm_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
>>   	struct clk *clk = pwm_get_chip_data(pwm),
>>   		   *parent_clk = clk_get_parent(clk);
>>  -	unsigned long rate, period, duty;
>>  +	unsigned long rate, parent_rate, period, duty;
>>   	unsigned long long tmp;
>>  -	unsigned int prescaler = 0;
>>  +	int ret;
>> 
>>  -	rate = clk_get_rate(parent_clk);
>>  -	tmp = (unsigned long long)rate * state->period;
>>  -	do_div(tmp, 1000000000);
>>  -	period = tmp;
>>  +	parent_rate = clk_get_rate(parent_clk);
>>  +
>>  +	jz4740_pwm_disable(chip, pwm);
>> 
>>  -	while (period > 0xffff && prescaler < 6) {
>>  -		period >>= 2;
>>  -		rate >>= 2;
>>  -		++prescaler;
>>  +	/* Reset the clock to the maximum rate, and we'll reduce it if 
>> needed */
>>  +	ret = clk_set_max_rate(clk, parent_rate);
> 
> What is the purpose of this call? IIUC this limits the allowed range 
> of
> rates for clk. I assume the idea is to prevent other consumers to 
> change
> the rate in a way that makes it unsuitable for this pwm. But this only
> makes sense if you had a notifier for clk changes, doesn't it? I'm
> confused.

Nothing like that. The second call to clk_set_max_rate() might have set
a maximum clock rate that's lower than the parent's rate, and we want to
undo that.


> I think this doesn't match the commit log, you didn't even introduced 
> a
> call to clk_round_rate().

Right, I'll edit the commit message.


>>  +	if (ret) {
>>  +		dev_err(chip->dev, "Unable to set max rate: %d\n", ret);
>>  +		return ret;
>>   	}
>> 
>>  -	if (prescaler == 6)
>>  -		return -EINVAL;
>>  +	ret = clk_set_rate(clk, parent_rate);
>>  +	if (ret) {
>>  +		dev_err(chip->dev, "Unable to reset to parent rate (%lu Hz)",
>>  +			parent_rate);
>>  +		return ret;
>>  +	}
>>  +
>>  +	/*
>>  +	 * Limit the clock to a maximum rate that still gives us a period 
>> value
>>  +	 * which fits in 16 bits.
>>  +	 */
>>  +	tmp = 0xffffull * NSEC_PER_SEC;
>>  +	do_div(tmp, state->period);
>> 
>>  +	ret = clk_set_max_rate(clk, tmp);
> 
> And now you change the maximal rate again?

Basically, we start from the maximum clock rate we can get for that PWM
- which is the rate of the parent clk - and from that compute the 
maximum
clock rate that we can support that still gives us < 16-bits hardware
values for the period and duty.

We then pass that computed maximum clock rate to clk_set_max_rate(), 
which
may or may not update the current PWM clock's rate to match the new 
limits.
Finally we read back the PWM clock's rate and compute the period and 
duty
from that.


>>  +	if (ret) {
>>  +		dev_err(chip->dev, "Unable to set max rate: %d\n", ret);
>>  +		return ret;
>>  +	}
>>  +
>>  +	/*
>>  +	 * Read back the clock rate, as it may have been modified by
>>  +	 * clk_set_max_rate()
>>  +	 */
>>  +	rate = clk_get_rate(clk);
>>  +
>>  +	if (rate != parent_rate)
>>  +		dev_dbg(chip->dev, "PWM clock updated to %lu Hz\n", rate);
>>  +
>>  +	/* Calculate period value */
>>  +	tmp = (unsigned long long)rate * state->period;
>>  +	do_div(tmp, NSEC_PER_SEC);
>>  +	period = (unsigned long)tmp;
>>  +
>>  +	/* Calculate duty value */
>>   	tmp = (unsigned long long)period * state->duty_cycle;
>>   	do_div(tmp, state->period);
>>   	duty = period - tmp;
>>  @@ -135,14 +167,10 @@ static int jz4740_pwm_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   	if (duty >= period)
>>   		duty = period - 1;
>> 
>>  -	jz4740_pwm_disable(chip, pwm);
>>  -
>>   	/* Set abrupt shutdown */
>>   	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>   			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
>> 
>>  -	clk_set_rate(clk, rate);
>>  -
> 
> It's not obvious to me why removing these two lines belong in the
> current patch.

They're not removed, they're both moved up in the function.


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

Re: [PATCH 6/7] pwm: jz4740: Make PWM start with the active part

[Paul Cercueil]

Le ven. 9 août 2019 à 19:10, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> On Fri, Aug 09, 2019 at 02:30:30PM +0200, Paul Cercueil wrote:
>>  The PWM will always start with the inactive part. To counter that,
>>  when PWM is enabled we switch the configured polarity, and use
>>  'period - duty + 1' as the real duty.
> 
> Where does the + 1 come from? This looks wrong. (So if duty=0 is
> requested you use duty = period + 1?)

You'd never request duty == 0, would you?

Your duty must always be in the inclusive range [1, period]
(hardware values, not ns). A duty of 0 is a hardware fault
(on the jz4740 it is).

If you request duty == 1 (the minimum), then the new duty is equal
to (period - 1 + 1) == period, which is the maximum of your range.

If you request duty == period (the maximum), then the new duty
calculated is equal to (period - period + 1) == 1, which is the
minimum of your range.


>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/pwm/pwm-jz4740.c | 22 +++++++++++++---------
>>   1 file changed, 13 insertions(+), 9 deletions(-)
>> 
>>  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
>>  index 85e2110aae4f..8df898429d47 100644
>>  --- a/drivers/pwm/pwm-jz4740.c
>>  +++ b/drivers/pwm/pwm-jz4740.c
>>  @@ -121,6 +121,7 @@ static int jz4740_pwm_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   		   *parent_clk = clk_get_parent(clk);
>>   	unsigned long rate, parent_rate, period, duty;
>>   	unsigned long long tmp;
>>  +	bool polarity_inversed;
>>   	int ret;
>> 
>>   	parent_rate = clk_get_rate(parent_clk);
>>  @@ -183,24 +184,27 @@ static int jz4740_pwm_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   	/* Reset counter to 0 */
>>   	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
>> 
>>  -	/* Set duty */
>>  -	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
>>  -
>>   	/* Set period */
>>   	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
>> 
>>  +	/*
>>  +	 * The PWM will always start with the inactive part. To counter 
>> that,
>>  +	 * when PWM is enabled we switch the configured polarity, and use
>>  +	 * 'period - duty + 1' as the real duty.
>>  +	 */
>>  +
>>  +	/* Set duty */
>>  +	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), period - 
>> duty + 1);
>>  +
> 
> Before you set duty first, then period, now you do it the other way
> round. Is there a good reason?

To move it below the comment that explains why we use 'period - duty + 
1'.

We modify that line anyway, so it's not like it makes the patch much 
more
verbose.


> 
>>   	/* Set polarity */
>>  -	switch (state->polarity) {
>>  -	case PWM_POLARITY_NORMAL:
>>  +	polarity_inversed = state->polarity == PWM_POLARITY_INVERSED;
>>  +	if (!polarity_inversed ^ state->enabled)
> 
> Why does state->enabled suddenly matter here?

The pin stay inactive when the PWM is disabled, but the level of the
inactive state depends on the polarity of the pin. So we need to switch
the polarity only when the PWM is enabled.


>>   		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>   				   TCU_TCSR_PWM_INITL_HIGH, 0);
>>  -		break;
>>  -	case PWM_POLARITY_INVERSED:
>>  +	else
>>   		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>   				   TCU_TCSR_PWM_INITL_HIGH,
>>   				   TCU_TCSR_PWM_INITL_HIGH);
>>  -		break;
>>  -	}
>> 
>>   	if (state->enabled)
>>   		jz4740_pwm_enable(chip, pwm);
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |

Re: [PATCH 3/7] pwm: jz4740: Drop dependency on MACH_INGENIC

[Paul Cercueil]

Le ven. 9 août 2019 à 18:41, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> On Fri, Aug 09, 2019 at 02:30:27PM +0200, Paul Cercueil wrote:
>>  Depending on MACH_INGENIC prevent us from creating a generic kernel 
>> that
>>  works on more than one MIPS board. Instead, we just depend on MIPS 
>> being
>>  set.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/pwm/Kconfig | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>>  diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>  index d2557c6fcf65..82a75e0b72e5 100644
>>  --- a/drivers/pwm/Kconfig
>>  +++ b/drivers/pwm/Kconfig
>>  @@ -224,7 +224,7 @@ config PWM_IMX_TPM
>> 
>>   config PWM_JZ4740
>>   	tristate "Ingenic JZ47xx PWM support"
>>  -	depends on MACH_INGENIC
>>  +	depends on MIPS
> 
> If this isn't actually useful on MIPS without MACH_INGENIC this is
> better expressed using:
> 
> 	depends on MIPS
> 	depends on MACH_INGENIC || COMPILE_TEST
> 
> This way some configuring a mips kernel without INGENIC isn't bothered
> by this question.

As said in the commit message, it is useful on MIPS without 
MACH_INGENIC,
if you want to create a generic kernel that works on more than one MIPS
board.


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

Re: [PATCH 6/7] pwm: jz4740: Make PWM start with the active part

[Uwe Kleine-König]

On Fri, Aug 09, 2019 at 07:33:24PM +0200, Paul Cercueil wrote:
> 
> 
> Le ven. 9 août 2019 à 19:10, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Fri, Aug 09, 2019 at 02:30:30PM +0200, Paul Cercueil wrote:
> > >  The PWM will always start with the inactive part. To counter that,
> > >  when PWM is enabled we switch the configured polarity, and use
> > >  'period - duty + 1' as the real duty.
> > 
> > Where does the + 1 come from? This looks wrong. (So if duty=0 is
> > requested you use duty = period + 1?)
> 
> You'd never request duty == 0, would you?
> 
> Your duty must always be in the inclusive range [1, period]
> (hardware values, not ns). A duty of 0 is a hardware fault
> (on the jz4740 it is).

From the PWM framework's POV duty cycle = 0 is perfectly valid. Similar
to duty == period. Not supporting dutz cycle 0 is another limitation of
your PWM that should be documented.

For actual use cases of duty cycle = 0 see drivers/hwmon/pwm-fan.c or
drivers/leds/leds-pwm.c.

> If you request duty == 1 (the minimum), then the new duty is equal
> to (period - 1 + 1) == period, which is the maximum of your range.
> 
> If you request duty == period (the maximum), then the new duty
> calculated is equal to (period - period + 1) == 1, which is the
> minimum of your range.
> 
> 
> > > 
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  ---
> > >   drivers/pwm/pwm-jz4740.c | 22 +++++++++++++---------
> > >   1 file changed, 13 insertions(+), 9 deletions(-)
> > > 
> > >  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> > >  index 85e2110aae4f..8df898429d47 100644
> > >  --- a/drivers/pwm/pwm-jz4740.c
> > >  +++ b/drivers/pwm/pwm-jz4740.c
> > >  @@ -121,6 +121,7 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > *chip, struct pwm_device *pwm,
> > >   		   *parent_clk = clk_get_parent(clk);
> > >   	unsigned long rate, parent_rate, period, duty;
> > >   	unsigned long long tmp;
> > >  +	bool polarity_inversed;
> > >   	int ret;
> > > 
> > >   	parent_rate = clk_get_rate(parent_clk);
> > >  @@ -183,24 +184,27 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > *chip, struct pwm_device *pwm,
> > >   	/* Reset counter to 0 */
> > >   	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
> > > 
> > >  -	/* Set duty */
> > >  -	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
> > >  -
> > >   	/* Set period */
> > >   	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
> > > 
> > >  +	/*
> > >  +	 * The PWM will always start with the inactive part. To counter that,
> > >  +	 * when PWM is enabled we switch the configured polarity, and use
> > >  +	 * 'period - duty + 1' as the real duty.
> > >  +	 */
> > >  +
> > >  +	/* Set duty */
> > >  +	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), period - duty + 1);
> > >  +
> > 
> > Before you set duty first, then period, now you do it the other way
> > round. Is there a good reason?
> 
> To move it below the comment that explains why we use 'period - duty + 1'.
> 
> We modify that line anyway, so it's not like it makes the patch much more
> verbose.

It doesn't make it more verbose, but that's not the background of my
question. For most(?) PWM implementation the order of hardware accesses
matters and introducing such a difference as an unneeded side effect
isn't optimal.

Why not add the comment above the line that already used to set the duty
in hardware?

> > >   	/* Set polarity */
> > >  -	switch (state->polarity) {
> > >  -	case PWM_POLARITY_NORMAL:
> > >  +	polarity_inversed = state->polarity == PWM_POLARITY_INVERSED;
> > >  +	if (!polarity_inversed ^ state->enabled)
> > 
> > Why does state->enabled suddenly matter here?
> 
> The pin stay inactive when the PWM is disabled, but the level of the
> inactive state depends on the polarity of the pin. So we need to switch
> the polarity only when the PWM is enabled.

After some thought I got that. When knowing this, this is already
mentioned in the comment you introduced as you write about enabled PWMs
only. Maybe it's just me, but mentioning that case more explicit would
have helped me. Something like:

	/*
	 * The hardware always starts a period with the inactive part.
	 * So invert polarity and duty cycle to yield the output that is
	 * expected by the PWM framework and its users. This inversion
	 * must not be done for a disabled PWM however because otherwise
	 * it outputs a constant active level.
	 */

Best regards
Uwe

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

Re: [PATCH 3/7] pwm: jz4740: Drop dependency on MACH_INGENIC

[Uwe Kleine-König]

On Fri, Aug 09, 2019 at 11:40:59PM +0200, Paul Cercueil wrote:
> 
> 
> Le ven. 9 août 2019 à 18:41, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Fri, Aug 09, 2019 at 02:30:27PM +0200, Paul Cercueil wrote:
> > >  Depending on MACH_INGENIC prevent us from creating a generic kernel
> > > that
> > >  works on more than one MIPS board. Instead, we just depend on MIPS
> > > being
> > >  set.
> > > 
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  ---
> > >   drivers/pwm/Kconfig | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > >  diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > >  index d2557c6fcf65..82a75e0b72e5 100644
> > >  --- a/drivers/pwm/Kconfig
> > >  +++ b/drivers/pwm/Kconfig
> > >  @@ -224,7 +224,7 @@ config PWM_IMX_TPM
> > > 
> > >   config PWM_JZ4740
> > >   	tristate "Ingenic JZ47xx PWM support"
> > >  -	depends on MACH_INGENIC
> > >  +	depends on MIPS
> > 
> > If this isn't actually useful on MIPS without MACH_INGENIC this is
> > better expressed using:
> > 
> > 	depends on MIPS
> > 	depends on MACH_INGENIC || COMPILE_TEST
> > 
> > This way some configuring a mips kernel without INGENIC isn't bothered
> > by this question.
> 
> As said in the commit message, it is useful on MIPS without MACH_INGENIC,
> if you want to create a generic kernel that works on more than one MIPS
> board.

Ah, mips is different than arm here. If you configure a generic kernel
that supports arm/imx and some others the Kconfig settings for imx are still
available. Would it make more sense to do:

	depends on MACH_INGENIC || MIPS_GENERIC || COMPILE_TEST

then? Or switch mips to the (IMHO) more intuitive approach that is taken
by arm, too? (i.e. ensure that MACH_INGENIC is =y for both a generic
kernel that supports this machine type among others and a tailored
kernel that only supports Ingenic.)

Best regards
Uwe

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

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Uwe Kleine-König]

Hello Paul,

On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote:
> Le ven. 9 août 2019 à 19:05, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil wrote:
> > >  The previous algorithm hardcoded details about how the TCU clocks
> > > work.
> > >  The new algorithm will use clk_round_rate to find the perfect clock
> > > rate
> > >  for the PWM channel.
> > > 
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  Tested-by: Mathieu Malaterre <malat@debian.org>
> > >  Tested-by: Artur Rojek <contact@artur-rojek.eu>
> > >  ---
> > >   drivers/pwm/pwm-jz4740.c | 60
> > > +++++++++++++++++++++++++++++-----------
> > >   1 file changed, 44 insertions(+), 16 deletions(-)
> > > 
> > >  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> > >  index 6ec8794d3b99..f20dc2e19240 100644
> > >  --- a/drivers/pwm/pwm-jz4740.c
> > >  +++ b/drivers/pwm/pwm-jz4740.c
> > >  @@ -110,24 +110,56 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > *chip, struct pwm_device *pwm,
> > >   	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
> > >   	struct clk *clk = pwm_get_chip_data(pwm),
> > >   		   *parent_clk = clk_get_parent(clk);
> > >  -	unsigned long rate, period, duty;
> > >  +	unsigned long rate, parent_rate, period, duty;
> > >   	unsigned long long tmp;
> > >  -	unsigned int prescaler = 0;
> > >  +	int ret;
> > > 
> > >  -	rate = clk_get_rate(parent_clk);
> > >  -	tmp = (unsigned long long)rate * state->period;
> > >  -	do_div(tmp, 1000000000);
> > >  -	period = tmp;
> > >  +	parent_rate = clk_get_rate(parent_clk);
> > >  +
> > >  +	jz4740_pwm_disable(chip, pwm);
> > > 
> > >  -	while (period > 0xffff && prescaler < 6) {
> > >  -		period >>= 2;
> > >  -		rate >>= 2;
> > >  -		++prescaler;
> > >  +	/* Reset the clock to the maximum rate, and we'll reduce it if needed */
> > >  +	ret = clk_set_max_rate(clk, parent_rate);
> > 
> > What is the purpose of this call? IIUC this limits the allowed range of
> > rates for clk. I assume the idea is to prevent other consumers to change
> > the rate in a way that makes it unsuitable for this pwm. But this only
> > makes sense if you had a notifier for clk changes, doesn't it? I'm
> > confused.
> 
> Nothing like that. The second call to clk_set_max_rate() might have set
> a maximum clock rate that's lower than the parent's rate, and we want to
> undo that.

I still don't get the purpose of this call. Why do you limit the clock
rate at all?

> > I think this doesn't match the commit log, you didn't even introduced a
> > call to clk_round_rate().
> 
> Right, I'll edit the commit message.
> 
> 
> > >  +	if (ret) {
> > >  +		dev_err(chip->dev, "Unable to set max rate: %d\n", ret);
> > >  +		return ret;
> > >   	}
> > > 
> > >  -	if (prescaler == 6)
> > >  -		return -EINVAL;
> > >  +	ret = clk_set_rate(clk, parent_rate);
> > >  +	if (ret) {
> > >  +		dev_err(chip->dev, "Unable to reset to parent rate (%lu Hz)",
> > >  +			parent_rate);
> > >  +		return ret;
> > >  +	}
> > >  +
> > >  +	/*
> > >  +	 * Limit the clock to a maximum rate that still gives us a period value
> > >  +	 * which fits in 16 bits.
> > >  +	 */
> > >  +	tmp = 0xffffull * NSEC_PER_SEC;
> > >  +	do_div(tmp, state->period);
> > > 
> > >  +	ret = clk_set_max_rate(clk, tmp);
> > 
> > And now you change the maximal rate again?
> 
> Basically, we start from the maximum clock rate we can get for that PWM
> - which is the rate of the parent clk - and from that compute the maximum
> clock rate that we can support that still gives us < 16-bits hardware
> values for the period and duty.
> 
> We then pass that computed maximum clock rate to clk_set_max_rate(), which
> may or may not update the current PWM clock's rate to match the new limits.
> Finally we read back the PWM clock's rate and compute the period and duty
> from that.

If you change the clk rate, is this externally visible on the PWM
output? Does this affect other PWM instances?

> > >  +	if (ret) {
> > >  +		dev_err(chip->dev, "Unable to set max rate: %d\n", ret);
> > >  +		return ret;
> > >  +	}
> > >  +
> > >  +	/*
> > >  +	 * Read back the clock rate, as it may have been modified by
> > >  +	 * clk_set_max_rate()
> > >  +	 */
> > >  +	rate = clk_get_rate(clk);
> > >  +
> > >  +	if (rate != parent_rate)
> > >  +		dev_dbg(chip->dev, "PWM clock updated to %lu Hz\n", rate);
> > >  +
> > >  +	/* Calculate period value */
> > >  +	tmp = (unsigned long long)rate * state->period;
> > >  +	do_div(tmp, NSEC_PER_SEC);
> > >  +	period = (unsigned long)tmp;
> > >  +
> > >  +	/* Calculate duty value */
> > >   	tmp = (unsigned long long)period * state->duty_cycle;
> > >   	do_div(tmp, state->period);
> > >   	duty = period - tmp;
> > >  @@ -135,14 +167,10 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > *chip, struct pwm_device *pwm,
> > >   	if (duty >= period)
> > >   		duty = period - 1;
> > > 
> > >  -	jz4740_pwm_disable(chip, pwm);
> > >  -
> > >   	/* Set abrupt shutdown */
> > >   	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
> > >   			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
> > > 
> > >  -	clk_set_rate(clk, rate);
> > >  -
> > 
> > It's not obvious to me why removing these two lines belong in the
> > current patch.
> 
> They're not removed, they're both moved up in the function.

OK, will look closer in the next iteration.

Best regards
Uwe

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

Re: [PATCH 1/7] pwm: jz4740: Obtain regmap from parent node

[Uwe Kleine-König]

Hello Paul,

On Fri, Aug 09, 2019 at 07:04:56PM +0200, Paul Cercueil wrote:
> Le ven. 9 août 2019 à 18:51, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Fri, Aug 09, 2019 at 02:30:25PM +0200, Paul Cercueil wrote:
> > >  The TCU registers are shared between a handful of drivers, accessing
> > >  them through the same regmap.
> > > 
> > >  While this driver is devicetree-compatible, it is never (as of now)
> > >  probed from devicetree, so this change does not introduce a ABI
> > > problem
> > >  with current devicetree files.
> > 
> > If you adapt the binding also update the binding doc please.
> 
> It's already updated, in mips-next, so it'll be in the next -rc1.

This is a useful information for the commit log.

> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  Tested-by: Mathieu Malaterre <malat@debian.org>
> > >  Tested-by: Artur Rojek <contact@artur-rojek.eu>
> > 
> > nitpick: put your S-o-b line after the other tags you added.
> > 
> > >  ---
> > >   drivers/pwm/Kconfig      |  1 +
> > >   drivers/pwm/pwm-jz4740.c | 80 ++++++++++++++++++++++++++--------------
> > >   2 files changed, 53 insertions(+), 28 deletions(-)
> > > 
> > >  diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > >  index a7e57516959e..cc4df0ec978a 100644
> > >  --- a/drivers/pwm/Kconfig
> > >  +++ b/drivers/pwm/Kconfig
> > >  @@ -225,6 +225,7 @@ config PWM_IMX_TPM
> > >   config PWM_JZ4740
> > >   	tristate "Ingenic JZ47xx PWM support"
> > >   	depends on MACH_INGENIC
> > >  +	select MFD_SYSCON
> > >   	help
> > >   	  Generic PWM framework driver for Ingenic JZ47xx based
> > >   	  machines.
> > >  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> > >  index f901e8a0d33d..7aea5e0c6e18 100644
> > >  --- a/drivers/pwm/pwm-jz4740.c
> > >  +++ b/drivers/pwm/pwm-jz4740.c
> > >  @@ -8,18 +8,20 @@
> > >   #include <linux/err.h>
> > >   #include <linux/gpio.h>
> > >   #include <linux/kernel.h>
> > >  +#include <linux/mfd/ingenic-tcu.h>
> > >  +#include <linux/mfd/syscon.h>
> > >   #include <linux/module.h>
> > >   #include <linux/of_device.h>
> > >   #include <linux/platform_device.h>
> > >   #include <linux/pwm.h>
> > >  -
> > >  -#include <asm/mach-jz4740/timer.h>
> > >  +#include <linux/regmap.h>
> > > 
> > >   #define NUM_PWM 8
> > > 
> > >   struct jz4740_pwm_chip {
> > >   	struct pwm_chip chip;
> > >   	struct clk *clk;
> > >  +	struct regmap *map;
> > >   };
> > > 
> > >   static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
> > >  @@ -29,6 +31,8 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
> > > 
> > >   static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > >   {
> > >  +	struct jz4740_pwm_chip *jz = to_jz4740(chip);
> > >  +
> > >   	/*
> > >   	 * Timers 0 and 1 are used for system tasks, so they are unavailable
> > >   	 * for use as PWMs.
> > >  @@ -36,50 +40,53 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > >   	if (pwm->hwpwm < 2)
> > >   		return -EBUSY;
> > > 
> > >  -	jz4740_timer_start(pwm->hwpwm);
> > >  +	regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm));
> > 
> > jz4740_timer_start does
> > 
> > 	writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR);
> > 
> > with
> > 
> > #define JZ_REG_TIMER_STOP_CLEAR         0x2C
> > 
> > and
> > 
> > #define TCU_REG_TSCR            0x3c
> > 
> > I wonder why the offsets are different.
> 
> The offset are different because the base is different.

This is a useful information for the commit log.
 
> > > 
> > >   	return 0;
> > >   }
> > > 
> > >   static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > >   {
> > >  -	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
> > >  +	struct jz4740_pwm_chip *jz = to_jz4740(chip);
> > > 
> > >  -	jz4740_timer_stop(pwm->hwpwm);
> > >  +	regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm));
> > 
> > jz4740_timer_set_ctrl writes to offset (((pwm->hwpwm) * 0x10) + 0x3C)
> > and jz4740_timer_stop to offset 0x1c. The regmap_write doesn't do both
> > of them but instead writes to offset 0x3c.
> 
> I guess it should have been TCU_REG_TSSR ("Timer Stop Set Register") and
> I didn't notice because the next patch drops this write anyway.
> 
> I'll do as you suggested in your other reply and swap the two patches if
> it makes things easier, it'll get rid of this write.

ok.

Best regards
Uwe

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

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Paul Cercueil]

Le lun. 12 août 2019 à 8:15, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello Paul,
> 
> On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote:
>>  Le ven. 9 août 2019 à 19:05, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil wrote:
>>  > >  The previous algorithm hardcoded details about how the TCU 
>> clocks
>>  > > work.
>>  > >  The new algorithm will use clk_round_rate to find the perfect 
>> clock
>>  > > rate
>>  > >  for the PWM channel.
>>  > >
>>  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  > >  Tested-by: Mathieu Malaterre <malat@debian.org>
>>  > >  Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>  > >  ---
>>  > >   drivers/pwm/pwm-jz4740.c | 60
>>  > > +++++++++++++++++++++++++++++-----------
>>  > >   1 file changed, 44 insertions(+), 16 deletions(-)
>>  > >
>>  > >  diff --git a/drivers/pwm/pwm-jz4740.c 
>> b/drivers/pwm/pwm-jz4740.c
>>  > >  index 6ec8794d3b99..f20dc2e19240 100644
>>  > >  --- a/drivers/pwm/pwm-jz4740.c
>>  > >  +++ b/drivers/pwm/pwm-jz4740.c
>>  > >  @@ -110,24 +110,56 @@ static int jz4740_pwm_apply(struct 
>> pwm_chip
>>  > > *chip, struct pwm_device *pwm,
>>  > >   	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
>>  > >   	struct clk *clk = pwm_get_chip_data(pwm),
>>  > >   		   *parent_clk = clk_get_parent(clk);
>>  > >  -	unsigned long rate, period, duty;
>>  > >  +	unsigned long rate, parent_rate, period, duty;
>>  > >   	unsigned long long tmp;
>>  > >  -	unsigned int prescaler = 0;
>>  > >  +	int ret;
>>  > >
>>  > >  -	rate = clk_get_rate(parent_clk);
>>  > >  -	tmp = (unsigned long long)rate * state->period;
>>  > >  -	do_div(tmp, 1000000000);
>>  > >  -	period = tmp;
>>  > >  +	parent_rate = clk_get_rate(parent_clk);
>>  > >  +
>>  > >  +	jz4740_pwm_disable(chip, pwm);
>>  > >
>>  > >  -	while (period > 0xffff && prescaler < 6) {
>>  > >  -		period >>= 2;
>>  > >  -		rate >>= 2;
>>  > >  -		++prescaler;
>>  > >  +	/* Reset the clock to the maximum rate, and we'll reduce it 
>> if needed */
>>  > >  +	ret = clk_set_max_rate(clk, parent_rate);
>>  >
>>  > What is the purpose of this call? IIUC this limits the allowed 
>> range of
>>  > rates for clk. I assume the idea is to prevent other consumers to 
>> change
>>  > the rate in a way that makes it unsuitable for this pwm. But this 
>> only
>>  > makes sense if you had a notifier for clk changes, doesn't it? I'm
>>  > confused.
>> 
>>  Nothing like that. The second call to clk_set_max_rate() might have 
>> set
>>  a maximum clock rate that's lower than the parent's rate, and we 
>> want to
>>  undo that.
> 
> I still don't get the purpose of this call. Why do you limit the clock
> rate at all?

As it says below, we "limit the clock to a maximum rate that still gives
us a period value which fits in 16 bits". So that the computed hardware
values won't overflow.

E.g. if at a rate of 12 MHz your computed hardware value for the period
is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the 
clock
rate must be reduced to the highest possible that will still give you a
< 16-bit value.

We always want the highest possible clock rate that works, for the sake 
of
precision.


>>  > I think this doesn't match the commit log, you didn't even 
>> introduced a
>>  > call to clk_round_rate().
>> 
>>  Right, I'll edit the commit message.
>> 
>> 
>>  > >  +	if (ret) {
>>  > >  +		dev_err(chip->dev, "Unable to set max rate: %d\n", ret);
>>  > >  +		return ret;
>>  > >   	}
>>  > >
>>  > >  -	if (prescaler == 6)
>>  > >  -		return -EINVAL;
>>  > >  +	ret = clk_set_rate(clk, parent_rate);
>>  > >  +	if (ret) {
>>  > >  +		dev_err(chip->dev, "Unable to reset to parent rate (%lu 
>> Hz)",
>>  > >  +			parent_rate);
>>  > >  +		return ret;
>>  > >  +	}
>>  > >  +
>>  > >  +	/*
>>  > >  +	 * Limit the clock to a maximum rate that still gives us a 
>> period value
>>  > >  +	 * which fits in 16 bits.
>>  > >  +	 */
>>  > >  +	tmp = 0xffffull * NSEC_PER_SEC;
>>  > >  +	do_div(tmp, state->period);
>>  > >
>>  > >  +	ret = clk_set_max_rate(clk, tmp);
>>  >
>>  > And now you change the maximal rate again?
>> 
>>  Basically, we start from the maximum clock rate we can get for that 
>> PWM
>>  - which is the rate of the parent clk - and from that compute the 
>> maximum
>>  clock rate that we can support that still gives us < 16-bits 
>> hardware
>>  values for the period and duty.
>> 
>>  We then pass that computed maximum clock rate to 
>> clk_set_max_rate(), which
>>  may or may not update the current PWM clock's rate to match the new 
>> limits.
>>  Finally we read back the PWM clock's rate and compute the period 
>> and duty
>>  from that.
> 
> If you change the clk rate, is this externally visible on the PWM
> output? Does this affect other PWM instances?

The clock rate doesn't change the PWM output because the hardware 
values for
the period and duty are adapted accordingly to reflect the change.


>>  > >  +	if (ret) {
>>  > >  +		dev_err(chip->dev, "Unable to set max rate: %d\n", ret);
>>  > >  +		return ret;
>>  > >  +	}
>>  > >  +
>>  > >  +	/*
>>  > >  +	 * Read back the clock rate, as it may have been modified by
>>  > >  +	 * clk_set_max_rate()
>>  > >  +	 */
>>  > >  +	rate = clk_get_rate(clk);
>>  > >  +
>>  > >  +	if (rate != parent_rate)
>>  > >  +		dev_dbg(chip->dev, "PWM clock updated to %lu Hz\n", rate);
>>  > >  +
>>  > >  +	/* Calculate period value */
>>  > >  +	tmp = (unsigned long long)rate * state->period;
>>  > >  +	do_div(tmp, NSEC_PER_SEC);
>>  > >  +	period = (unsigned long)tmp;
>>  > >  +
>>  > >  +	/* Calculate duty value */
>>  > >   	tmp = (unsigned long long)period * state->duty_cycle;
>>  > >   	do_div(tmp, state->period);
>>  > >   	duty = period - tmp;
>>  > >  @@ -135,14 +167,10 @@ static int jz4740_pwm_apply(struct 
>> pwm_chip
>>  > > *chip, struct pwm_device *pwm,
>>  > >   	if (duty >= period)
>>  > >   		duty = period - 1;
>>  > >
>>  > >  -	jz4740_pwm_disable(chip, pwm);
>>  > >  -
>>  > >   	/* Set abrupt shutdown */
>>  > >   	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>  > >   			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
>>  > >
>>  > >  -	clk_set_rate(clk, rate);
>>  > >  -
>>  >
>>  > It's not obvious to me why removing these two lines belong in the
>>  > current patch.
>> 
>>  They're not removed, they're both moved up in the function.
> 
> OK, will look closer in the next iteration.
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |

Re: [PATCH 6/7] pwm: jz4740: Make PWM start with the active part

[Paul Cercueil]

Le lun. 12 août 2019 à 7:55, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> On Fri, Aug 09, 2019 at 07:33:24PM +0200, Paul Cercueil wrote:
>> 
>> 
>>  Le ven. 9 août 2019 à 19:10, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > On Fri, Aug 09, 2019 at 02:30:30PM +0200, Paul Cercueil wrote:
>>  > >  The PWM will always start with the inactive part. To counter 
>> that,
>>  > >  when PWM is enabled we switch the configured polarity, and use
>>  > >  'period - duty + 1' as the real duty.
>>  >
>>  > Where does the + 1 come from? This looks wrong. (So if duty=0 is
>>  > requested you use duty = period + 1?)
>> 
>>  You'd never request duty == 0, would you?
>> 
>>  Your duty must always be in the inclusive range [1, period]
>>  (hardware values, not ns). A duty of 0 is a hardware fault
>>  (on the jz4740 it is).
> 
> From the PWM framework's POV duty cycle = 0 is perfectly valid. 
> Similar
> to duty == period. Not supporting dutz cycle 0 is another limitation 
> of
> your PWM that should be documented.
> 
> For actual use cases of duty cycle = 0 see drivers/hwmon/pwm-fan.c or
> drivers/leds/leds-pwm.c.

Perfectly valid for the PWM framework, maybe; but what is the expected
output then? A constant inactive state? Then I guess I can just disable
the PWM output in the driver when configured with duty == 0.


>>  If you request duty == 1 (the minimum), then the new duty is equal
>>  to (period - 1 + 1) == period, which is the maximum of your range.
>> 
>>  If you request duty == period (the maximum), then the new duty
>>  calculated is equal to (period - period + 1) == 1, which is the
>>  minimum of your range.
>> 
>> 
>>  > >
>>  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  > >  ---
>>  > >   drivers/pwm/pwm-jz4740.c | 22 +++++++++++++---------
>>  > >   1 file changed, 13 insertions(+), 9 deletions(-)
>>  > >
>>  > >  diff --git a/drivers/pwm/pwm-jz4740.c 
>> b/drivers/pwm/pwm-jz4740.c
>>  > >  index 85e2110aae4f..8df898429d47 100644
>>  > >  --- a/drivers/pwm/pwm-jz4740.c
>>  > >  +++ b/drivers/pwm/pwm-jz4740.c
>>  > >  @@ -121,6 +121,7 @@ static int jz4740_pwm_apply(struct pwm_chip
>>  > > *chip, struct pwm_device *pwm,
>>  > >   		   *parent_clk = clk_get_parent(clk);
>>  > >   	unsigned long rate, parent_rate, period, duty;
>>  > >   	unsigned long long tmp;
>>  > >  +	bool polarity_inversed;
>>  > >   	int ret;
>>  > >
>>  > >   	parent_rate = clk_get_rate(parent_clk);
>>  > >  @@ -183,24 +184,27 @@ static int jz4740_pwm_apply(struct 
>> pwm_chip
>>  > > *chip, struct pwm_device *pwm,
>>  > >   	/* Reset counter to 0 */
>>  > >   	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
>>  > >
>>  > >  -	/* Set duty */
>>  > >  -	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
>>  > >  -
>>  > >   	/* Set period */
>>  > >   	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
>>  > >
>>  > >  +	/*
>>  > >  +	 * The PWM will always start with the inactive part. To 
>> counter that,
>>  > >  +	 * when PWM is enabled we switch the configured polarity, 
>> and use
>>  > >  +	 * 'period - duty + 1' as the real duty.
>>  > >  +	 */
>>  > >  +
>>  > >  +	/* Set duty */
>>  > >  +	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), period 
>> - duty + 1);
>>  > >  +
>>  >
>>  > Before you set duty first, then period, now you do it the other 
>> way
>>  > round. Is there a good reason?
>> 
>>  To move it below the comment that explains why we use 'period - 
>> duty + 1'.
>> 
>>  We modify that line anyway, so it's not like it makes the patch 
>> much more
>>  verbose.
> 
> It doesn't make it more verbose, but that's not the background of my
> question. For most(?) PWM implementation the order of hardware 
> accesses
> matters and introducing such a difference as an unneeded side effect
> isn't optimal.

There's no side effect. The PWM is disabled when reconfigured.


> Why not add the comment above the line that already used to set the 
> duty
> in hardware?

I thought it made sense to have the two parts of the trick closer 
together
in the code, below the comment, so that it's clearer what it does.


>>  > >   	/* Set polarity */
>>  > >  -	switch (state->polarity) {
>>  > >  -	case PWM_POLARITY_NORMAL:
>>  > >  +	polarity_inversed = state->polarity == PWM_POLARITY_INVERSED;
>>  > >  +	if (!polarity_inversed ^ state->enabled)
>>  >
>>  > Why does state->enabled suddenly matter here?
>> 
>>  The pin stay inactive when the PWM is disabled, but the level of the
>>  inactive state depends on the polarity of the pin. So we need to 
>> switch
>>  the polarity only when the PWM is enabled.
> 
> After some thought I got that. When knowing this, this is already
> mentioned in the comment you introduced as you write about enabled 
> PWMs
> only. Maybe it's just me, but mentioning that case more explicit would
> have helped me. Something like:
> 
> 	/*
> 	 * The hardware always starts a period with the inactive part.
> 	 * So invert polarity and duty cycle to yield the output that is
> 	 * expected by the PWM framework and its users. This inversion
> 	 * must not be done for a disabled PWM however because otherwise
> 	 * it outputs a constant active level.
> 	 */

Ok.


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

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Uwe Kleine-König]

Hello Paul,

On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote:
> Le lun. 12 août 2019 à 8:15, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote:
> > >  Le ven. 9 août 2019 à 19:05, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> > >  <u.kleine-koenig@pengutronix.de> a écrit :
> > >  > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil wrote:
> > >  > > [...]
> > >  > >  +	/* Reset the clock to the maximum rate, and we'll reduce it if needed */
> > >  > >  +	ret = clk_set_max_rate(clk, parent_rate);
> > >  >
> > >  > What is the purpose of this call? IIUC this limits the allowed range of
> > >  > rates for clk. I assume the idea is to prevent other consumers to change
> > >  > the rate in a way that makes it unsuitable for this pwm. But this only
> > >  > makes sense if you had a notifier for clk changes, doesn't it? I'm
> > >  > confused.
> > > 
> > >  Nothing like that. The second call to clk_set_max_rate() might have set
> > >  a maximum clock rate that's lower than the parent's rate, and we want to
> > >  undo that.
> > 
> > I still don't get the purpose of this call. Why do you limit the clock
> > rate at all?
> 
> As it says below, we "limit the clock to a maximum rate that still gives
> us a period value which fits in 16 bits". So that the computed hardware
> values won't overflow.

But why not just using clk_set_rate? You want to have the clock running
at a certain rate, not any rate below that certain rate, don't you?
 
> E.g. if at a rate of 12 MHz your computed hardware value for the period
> is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the clock
> rate must be reduced to the highest possible that will still give you a
> < 16-bit value.
> 
> We always want the highest possible clock rate that works, for the sake of
> precision.

This is dubious; but ok to keep the driver simple. (Consider a PWM that
can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a duty
cycle of 40 ns is requested you can get an exact match with 25 MHz, but
not with 30 MHz.)

> > >  Basically, we start from the maximum clock rate we can get for that PWM
> > >  - which is the rate of the parent clk - and from that compute the maximum
> > >  clock rate that we can support that still gives us < 16-bits hardware
> > >  values for the period and duty.
> > > 
> > >  We then pass that computed maximum clock rate to clk_set_max_rate(), which
> > >  may or may not update the current PWM clock's rate to match the new limits.
> > >  Finally we read back the PWM clock's rate and compute the period and duty
> > >  from that.
> > 
> > If you change the clk rate, is this externally visible on the PWM
> > output? Does this affect other PWM instances?
> 
> The clock rate doesn't change the PWM output because the hardware values for
> the period and duty are adapted accordingly to reflect the change.

It doesn't change it in the end. But in the (short) time frame between
the call to change the clock and the update of the PWM registers there
is a glitch, right?

You didn't answer to the question about other PWM instances. Does that
mean others are not affected?

Best regards
Uwe

PS: It would be great if you could fix your mailer to not damage the
quoted mail. Also it doesn't seem to understand how my name is encoded
in the From line. I fixed up the quotes in my reply.

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

Re: [PATCH 6/7] pwm: jz4740: Make PWM start with the active part

[Uwe Kleine-König]

On Mon, Aug 12, 2019 at 10:50:01PM +0200, Paul Cercueil wrote:
> 
> 
> Le lun. 12 août 2019 à 7:55, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Fri, Aug 09, 2019 at 07:33:24PM +0200, Paul Cercueil wrote:
> > > 
> > > 
> > >  Le ven. 9 août 2019 à 19:10, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> > >  <u.kleine-koenig@pengutronix.de> a écrit :
> > >  > On Fri, Aug 09, 2019 at 02:30:30PM +0200, Paul Cercueil wrote:
> > >  > >  The PWM will always start with the inactive part. To counter
> > > that,
> > >  > >  when PWM is enabled we switch the configured polarity, and use
> > >  > >  'period - duty + 1' as the real duty.
> > >  >
> > >  > Where does the + 1 come from? This looks wrong. (So if duty=0 is
> > >  > requested you use duty = period + 1?)
> > > 
> > >  You'd never request duty == 0, would you?
> > > 
> > >  Your duty must always be in the inclusive range [1, period]
> > >  (hardware values, not ns). A duty of 0 is a hardware fault
> > >  (on the jz4740 it is).
> > 
> > From the PWM framework's POV duty cycle = 0 is perfectly valid. Similar
> > to duty == period. Not supporting dutz cycle 0 is another limitation of
> > your PWM that should be documented.
> > 
> > For actual use cases of duty cycle = 0 see drivers/hwmon/pwm-fan.c or
> > drivers/leds/leds-pwm.c.
> 
> Perfectly valid for the PWM framework, maybe; but what is the expected
> output then? A constant inactive state?

Yes, a constant inactive state is expected. This is consistent and in a
similar way when using duty == period an constant active output is
expected.

> Then I guess I can just disable the PWM output in the driver when
> configured with duty == 0.

Some time ago I argued with Thierry that we could drop the concept of
enabled/disabled for a PWM because a disabled PWM is supposed to behave
identically to duty=0. This is however only nearly true because with
duty=0 the time the PWM is inactive still is a multiple of the period.

I tend to agree that disabling the PWM when duty=0 is requested is
better than to fail the request (or configure for duty=1 $whateverunit).
I'm looking forward to what Thierry's opinion is here.

> > >  If you request duty == 1 (the minimum), then the new duty is equal
> > >  to (period - 1 + 1) == period, which is the maximum of your range.
> > > 
> > >  If you request duty == period (the maximum), then the new duty
> > >  calculated is equal to (period - period + 1) == 1, which is the
> > >  minimum of your range.

Note that the wrong border (because duty=0 is impossible for your
hardware) shifts the whole space. The right inverse of duty = period - 1
is duty = 1, isn't it?

> > > > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > > > >  ---
> > > > >   drivers/pwm/pwm-jz4740.c | 22 +++++++++++++---------
> > > > >   1 file changed, 13 insertions(+), 9 deletions(-)
> > > > >
> > > > >  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> > > > >  index 85e2110aae4f..8df898429d47 100644
> > > > >  --- a/drivers/pwm/pwm-jz4740.c
> > > > >  +++ b/drivers/pwm/pwm-jz4740.c
> > > > >  @@ -121,6 +121,7 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > >   		   *parent_clk = clk_get_parent(clk);
> > > > >   	unsigned long rate, parent_rate, period, duty;
> > > > >   	unsigned long long tmp;
> > > > >  +	bool polarity_inversed;
> > > > >   	int ret;
> > > > >
> > > > >   	parent_rate = clk_get_rate(parent_clk);
> > > > >  @@ -183,24 +184,27 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > > > *chip, struct pwm_device *pwm,
> > > > >   	/* Reset counter to 0 */
> > > > >   	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
> > > > >
> > > > >  -	/* Set duty */
> > > > >  -	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
> > > > >  -
> > > > >   	/* Set period */
> > > > >   	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
> > > > >
> > > > >  +	/*
> > > > >  +	 * The PWM will always start with the inactive part. To counter that,
> > > > >  +	 * when PWM is enabled we switch the configured polarity, and use
> > > > >  +	 * 'period - duty + 1' as the real duty.
> > > > >  +	 */
> > > > >  +
> > > > >  +	/* Set duty */
> > > > >  +	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), period - duty + 1);
> > > > >  +
> > > >
> > > > Before you set duty first, then period, now you do it the other way
> > > > round. Is there a good reason?
> > > 
> > >  To move it below the comment that explains why we use 'period - duty + 1'.
> > > 
> > >  We modify that line anyway, so it's not like it makes the patch much more
> > >  verbose.
> > 
> > It doesn't make it more verbose, but that's not the background of my
> > question. For most(?) PWM implementation the order of hardware accesses
> > matters and introducing such a difference as an unneeded side effect
> > isn't optimal.
> 
> There's no side effect. The PWM is disabled when reconfigured.

Then please mention it in the commit log.

Best regards
Uwe

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

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Paul Cercueil]

[Re-send my message in plain text, as it was bounced by the
lists - sorry about that]


Le lun. 12 août 2019 à 23:48, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello Paul,
> 
> On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote:
>>  Le lun. 12 août 2019 à 8:15, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote:
>>  > >  Le ven. 9 août 2019 à 19:05, Uwe 
>> =?iso-8859-1?q?Kleine-K=F6nig?=
>>  > >  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > >  > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil 
>> wrote:
>>  > >  > > [...]
>>  > >  > >  +	/* Reset the clock to the maximum rate, and we'll 
>> reduce it if needed */
>>  > >  > >  +	ret = clk_set_max_rate(clk, parent_rate);
>>  > >  >
>>  > >  > What is the purpose of this call? IIUC this limits the 
>> allowed range of
>>  > >  > rates for clk. I assume the idea is to prevent other 
>> consumers to change
>>  > >  > the rate in a way that makes it unsuitable for this pwm. But 
>> this only
>>  > >  > makes sense if you had a notifier for clk changes, doesn't 
>> it? I'm
>>  > >  > confused.
>>  > >
>>  > >  Nothing like that. The second call to clk_set_max_rate() might 
>> have set
>>  > >  a maximum clock rate that's lower than the parent's rate, and 
>> we want to
>>  > >  undo that.
>>  >
>>  > I still don't get the purpose of this call. Why do you limit the 
>> clock
>>  > rate at all?
>> 
>>  As it says below, we "limit the clock to a maximum rate that still 
>> gives
>>  us a period value which fits in 16 bits". So that the computed 
>> hardware
>>  values won't overflow.
> 
> But why not just using clk_set_rate? You want to have the clock 
> running
> at a certain rate, not any rate below that certain rate, don't you?

I'll let yourself answer yourself:
https://patchwork.ozlabs.org/patch/1018969/

It's enough to run it below a certain rate, yes. The actual rate doesn't
actually matter that much.


> 
>>  E.g. if at a rate of 12 MHz your computed hardware value for the 
>> period
>>  is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the 
>> clock
>>  rate must be reduced to the highest possible that will still give 
>> you a
>>  < 16-bit value.
>> 
>>  We always want the highest possible clock rate that works, for the 
>> sake of
>>  precision.
> 
> This is dubious; but ok to keep the driver simple. (Consider a PWM 
> that
> can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a duty
> cycle of 40 ns is requested you can get an exact match with 25 MHz, 
> but
> not with 30 MHz.)

The clock rate is actually (parent_rate >> (2 * x) )
for x = 0, 1, 2, ...

So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, and the
next one is 1.875 MHz. It'd be very unlikely that you get a better 
match at a
lower clock.


>>  > >  Basically, we start from the maximum clock rate we can get for 
>> that PWM
>>  > >  - which is the rate of the parent clk - and from that compute 
>> the maximum
>>  > >  clock rate that we can support that still gives us < 16-bits 
>> hardware
>>  > >  values for the period and duty.
>>  > >
>>  > >  We then pass that computed maximum clock rate to 
>> clk_set_max_rate(), which
>>  > >  may or may not update the current PWM clock's rate to match 
>> the new limits.
>>  > >  Finally we read back the PWM clock's rate and compute the 
>> period and duty
>>  > >  from that.
>>  >
>>  > If you change the clk rate, is this externally visible on the PWM
>>  > output? Does this affect other PWM instances?
>> 
>>  The clock rate doesn't change the PWM output because the hardware 
>> values for
>>  the period and duty are adapted accordingly to reflect the change.
> 
> It doesn't change it in the end. But in the (short) time frame between
> the call to change the clock and the update of the PWM registers there
> is a glitch, right?

The PWM is disabled, so the line is in inactive state, and will be in 
that state
until the PWM is enabled again. No glitch to fear.


> You didn't answer to the question about other PWM instances. Does that
> mean others are not affected?

Sorry. Yes, they are not affected - all PWM channels are independent.


> Best regards
> Uwe
> 
> PS: It would be great if you could fix your mailer to not damage the
> quoted mail. Also it doesn't seem to understand how my name is encoded
> in the From line. I fixed up the quotes in my reply.

I guess I'll submit a bug report to Geary then.


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

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Uwe Kleine-König]

Hello Paul,

[adding Stephen Boyd to Cc]

On Tue, Aug 13, 2019 at 12:16:23AM +0200, Paul Cercueil wrote:
> Le lun. 12 août 2019 à 23:48, Uwe Kleine-König a écrit :
> > On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote:
> > > Le lun. 12 août 2019 à 8:15, Uwe Kleine-König a écrit :
> > > > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote:
> > > > > Le ven. 9 août 2019 à 19:05, Uwe Kleine-König a écrit :
> > > > > > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil wrote:
> > > > > > > [...]
> > > > > > >  +	/* Reset the clock to the maximum rate, and we'll reduce it if needed */
> > > > > > >  +	ret = clk_set_max_rate(clk, parent_rate);
> > > > > >
> > > > > > What is the purpose of this call? IIUC this limits the allowed range of
> > > > > > rates for clk. I assume the idea is to prevent other consumers to change
> > > > > > the rate in a way that makes it unsuitable for this pwm. But this only
> > > > > > makes sense if you had a notifier for clk changes, doesn't it? I'm
> > > > > > confused.
> > > > >
> > > > > Nothing like that. The second call to clk_set_max_rate() might have set
> > > > > a maximum clock rate that's lower than the parent's rate, and we want to
> > > > > undo that.
> > > >
> > > > I still don't get the purpose of this call. Why do you limit the clock
> > > > rate at all?
> > >
> > > As it says below, we "limit the clock to a maximum rate that still gives
> > > us a period value which fits in 16 bits". So that the computed hardware
> > > values won't overflow.
> > 
> > But why not just using clk_set_rate? You want to have the clock running
> > at a certain rate, not any rate below that certain rate, don't you?
> 
> I'll let yourself answer yourself:
> https://patchwork.ozlabs.org/patch/1018969/

In that thread I claimed that you used clk_round_rate wrongly, not that
you should use clk_set_max_rate(). (The claim was somewhat weakend by
Stephen, but still I think that clk_round_rate is the right approach.)

The upside of clk_round_rate is that it allows you to test for the
capabilities of the clock without actually changing it before you found
a setting you consider to be good.

> It's enough to run it below a certain rate, yes. The actual rate doesn't
> actually matter that much.

1 Hz would be fine? I doubt it.

> > >  E.g. if at a rate of 12 MHz your computed hardware value for the period
> > >  is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the clock
> > >  rate must be reduced to the highest possible that will still give you a
> > >  < 16-bit value.
> > > 
> > >  We always want the highest possible clock rate that works, for the sake of
> > >  precision.
> > 
> > This is dubious; but ok to keep the driver simple. (Consider a PWM that
> > can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a duty
> > cycle of 40 ns is requested you can get an exact match with 25 MHz, but
> > not with 30 MHz.)
> 
> The clock rate is actually (parent_rate >> (2 * x) )
> for x = 0, 1, 2, ...
> 
> So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, and the
> next one is 1.875 MHz. It'd be very unlikely that you get a better match at
> a lower clock.

If the smaller freqs are all dividers of the fastest that's fine. Please
note in a code comment that you're assuming this.
 
> > >  > >  Basically, we start from the maximum clock rate we can get for that PWM
> > >  > >  - which is the rate of the parent clk - and from that compute the maximum
> > >  > >  clock rate that we can support that still gives us < 16-bits hardware
> > >  > >  values for the period and duty.
> > >  > >
> > >  > >  We then pass that computed maximum clock rate to clk_set_max_rate(), which
> > >  > >  may or may not update the current PWM clock's rate to match the new limits.
> > >  > >  Finally we read back the PWM clock's rate and compute the period and duty
> > >  > >  from that.
> > >  >
> > >  > If you change the clk rate, is this externally visible on the PWM
> > >  > output? Does this affect other PWM instances?
> > > 
> > >  The clock rate doesn't change the PWM output because the hardware values for
> > >  the period and duty are adapted accordingly to reflect the change.
> > 
> > It doesn't change it in the end. But in the (short) time frame between
> > the call to change the clock and the update of the PWM registers there
> > is a glitch, right?
> 
> The PWM is disabled, so the line is in inactive state, and will be in that state
> until the PWM is enabled again. No glitch to fear.

ok, please note in the commit log that the reordering doesn't affect the
output because the PWM is off and are done to make it more obvious what
happens.

> > You didn't answer to the question about other PWM instances. Does that
> > mean others are not affected?
> 
> Sorry. Yes, they are not affected - all PWM channels are independent.

ok.

> > PS: It would be great if you could fix your mailer to not damage the
> > quoted mail. Also it doesn't seem to understand how my name is encoded
> > in the From line. I fixed up the quotes in my reply.
> 
> I switched Geary to "rich text". Is that better?

No. It looks exactly like the copy you bounced to the list. See
https://patchwork.ozlabs.org/comment/2236355/ for how it looks.

Best regards
Uwe

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

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Paul Cercueil]

Le mar. 13 août 2019 à 7:27, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello Paul,
> 
> [adding Stephen Boyd to Cc]
> 
> On Tue, Aug 13, 2019 at 12:16:23AM +0200, Paul Cercueil wrote:
>>  Le lun. 12 août 2019 à 23:48, Uwe Kleine-König a écrit :
>>  > On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote:
>>  > > Le lun. 12 août 2019 à 8:15, Uwe Kleine-König a écrit :
>>  > > > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote:
>>  > > > > Le ven. 9 août 2019 à 19:05, Uwe Kleine-König a écrit :
>>  > > > > > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil 
>> wrote:
>>  > > > > > > [...]
>>  > > > > > >  +	/* Reset the clock to the maximum rate, and we'll 
>> reduce it if needed */
>>  > > > > > >  +	ret = clk_set_max_rate(clk, parent_rate);
>>  > > > > >
>>  > > > > > What is the purpose of this call? IIUC this limits the 
>> allowed range of
>>  > > > > > rates for clk. I assume the idea is to prevent other 
>> consumers to change
>>  > > > > > the rate in a way that makes it unsuitable for this pwm. 
>> But this only
>>  > > > > > makes sense if you had a notifier for clk changes, 
>> doesn't it? I'm
>>  > > > > > confused.
>>  > > > >
>>  > > > > Nothing like that. The second call to clk_set_max_rate() 
>> might have set
>>  > > > > a maximum clock rate that's lower than the parent's rate, 
>> and we want to
>>  > > > > undo that.
>>  > > >
>>  > > > I still don't get the purpose of this call. Why do you limit 
>> the clock
>>  > > > rate at all?
>>  > >
>>  > > As it says below, we "limit the clock to a maximum rate that 
>> still gives
>>  > > us a period value which fits in 16 bits". So that the computed 
>> hardware
>>  > > values won't overflow.
>>  >
>>  > But why not just using clk_set_rate? You want to have the clock 
>> running
>>  > at a certain rate, not any rate below that certain rate, don't 
>> you?
>> 
>>  I'll let yourself answer yourself:
>>  https://patchwork.ozlabs.org/patch/1018969/
> 
> In that thread I claimed that you used clk_round_rate wrongly, not 
> that
> you should use clk_set_max_rate(). (The claim was somewhat weakend by
> Stephen, but still I think that clk_round_rate is the right approach.)

Well, you said that I shouln't rely on the fact that clk_round_rate() 
will round down. That completely defeats the previous algorithm. So 
please tell me how to use it correctly, because I don't see it.

I came up with a much smarter alternative, that doesn't rely on the 
rounding method of clk_round_rate, and which is better overall (no loop 
needed). It sounds to me like you're bashing the code without making 
the effort to understand what it does.

Thierry called it a "neat trick" 
(https://patchwork.kernel.org/patch/10836879/) so it cannot be as bad 
as you say.


> 
> The upside of clk_round_rate is that it allows you to test for the
> capabilities of the clock without actually changing it before you 
> found
> a setting you consider to be good.

I know what clk_round_rate() is for. But here we don't do 
trial-and-error to find the first highest clock rate that works, we 
compute the maximum clock we can use and limit the clock rate to that.


> 
>>  It's enough to run it below a certain rate, yes. The actual rate 
>> doesn't
>>  actually matter that much.
> 
> 1 Hz would be fine? I doubt it.

We use the highest possible clock rate. We wouldn't use 1 Hz unless 
it's the highest clock rate available.


> 
>>  > >  E.g. if at a rate of 12 MHz your computed hardware value for 
>> the period
>>  > >  is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. 
>> So the clock
>>  > >  rate must be reduced to the highest possible that will still 
>> give you a
>>  > >  < 16-bit value.
>>  > >
>>  > >  We always want the highest possible clock rate that works, for 
>> the sake of
>>  > >  precision.
>>  >
>>  > This is dubious; but ok to keep the driver simple. (Consider a 
>> PWM that
>>  > can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a 
>> duty
>>  > cycle of 40 ns is requested you can get an exact match with 25 
>> MHz, but
>>  > not with 30 MHz.)
>> 
>>  The clock rate is actually (parent_rate >> (2 * x) )
>>  for x = 0, 1, 2, ...
>> 
>>  So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, and 
>> the
>>  next one is 1.875 MHz. It'd be very unlikely that you get a better 
>> match at
>>  a lower clock.
> 
> If the smaller freqs are all dividers of the fastest that's fine. 
> Please
> note in a code comment that you're assuming this.

No, I am not assuming this. The current driver just picks the highest 
clock rate that works. We're not changing the behaviour here.


> 
>>  > >  > >  Basically, we start from the maximum clock rate we can 
>> get for that PWM
>>  > >  > >  - which is the rate of the parent clk - and from that 
>> compute the maximum
>>  > >  > >  clock rate that we can support that still gives us < 
>> 16-bits hardware
>>  > >  > >  values for the period and duty.
>>  > >  > >
>>  > >  > >  We then pass that computed maximum clock rate to 
>> clk_set_max_rate(), which
>>  > >  > >  may or may not update the current PWM clock's rate to 
>> match the new limits.
>>  > >  > >  Finally we read back the PWM clock's rate and compute the 
>> period and duty
>>  > >  > >  from that.
>>  > >  >
>>  > >  > If you change the clk rate, is this externally visible on 
>> the PWM
>>  > >  > output? Does this affect other PWM instances?
>>  > >
>>  > >  The clock rate doesn't change the PWM output because the 
>> hardware values for
>>  > >  the period and duty are adapted accordingly to reflect the 
>> change.
>>  >
>>  > It doesn't change it in the end. But in the (short) time frame 
>> between
>>  > the call to change the clock and the update of the PWM registers 
>> there
>>  > is a glitch, right?
>> 
>>  The PWM is disabled, so the line is in inactive state, and will be 
>> in that state
>>  until the PWM is enabled again. No glitch to fear.
> 
> ok, please note in the commit log that the reordering doesn't affect 
> the
> output because the PWM is off and are done to make it more obvious 
> what
> happens.
> 
>>  > You didn't answer to the question about other PWM instances. Does 
>> that
>>  > mean others are not affected?
>> 
>>  Sorry. Yes, they are not affected - all PWM channels are 
>> independent.
> 
> ok.
> 
>>  > PS: It would be great if you could fix your mailer to not damage 
>> the
>>  > quoted mail. Also it doesn't seem to understand how my name is 
>> encoded
>>  > in the From line. I fixed up the quotes in my reply.
>> 
>>  I switched Geary to "rich text". Is that better?
> 
> No. It looks exactly like the copy you bounced to the list. See
> https://patchwork.ozlabs.org/comment/2236355/ for how it looks.
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Uwe Kleine-König]

Hello Paul,

On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote:
> Le mar. 13 août 2019 à 7:27, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> <u.kleine-koenig@pengutronix.de> a écrit :
> > [adding Stephen Boyd to Cc]
> > 
> > On Tue, Aug 13, 2019 at 12:16:23AM +0200, Paul Cercueil wrote:
> > > Le lun. 12 août 2019 à 23:48, Uwe Kleine-König a écrit :
> > > > On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote:
> > > > > Le lun. 12 août 2019 à 8:15, Uwe Kleine-König a écrit :
> > > > > > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote:
> > > > > > > Le ven. 9 août 2019 à 19:05, Uwe Kleine-König a écrit :
> > > > > > > > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil wrote:
> > > > > > > > > [...]
> > > > > > > > >  +	/* Reset the clock to the maximum rate, and we'll reduce it if needed */
> > > > > > > > >  +	ret = clk_set_max_rate(clk, parent_rate);
> > > > > > > >
> > > > > > > > What is the purpose of this call? IIUC this limits the allowed range of
> > > > > > > > rates for clk. I assume the idea is to prevent other consumers to change
> > > > > > > > the rate in a way that makes it unsuitable for this pwm. But this only
> > > > > > > > makes sense if you had a notifier for clk changes, doesn't it? I'm
> > > > > > > > confused.
> > > > > > >
> > > > > > > Nothing like that. The second call to clk_set_max_rate() might have set
> > > > > > > a maximum clock rate that's lower than the parent's rate, and we want to
> > > > > > > undo that.
> > > > > >
> > > > > > I still don't get the purpose of this call. Why do you limit the clock
> > > > > > rate at all?
> > > > >
> > > > > As it says below, we "limit the clock to a maximum rate that still gives
> > > > > us a period value which fits in 16 bits". So that the computed hardware
> > > > > values won't overflow.
> > > >
> > > > But why not just using clk_set_rate? You want to have the clock running
> > > > at a certain rate, not any rate below that certain rate, don't you?
> > > 
> > >  I'll let yourself answer yourself:
> > >  https://patchwork.ozlabs.org/patch/1018969/
> > 
> > In that thread I claimed that you used clk_round_rate wrongly, not that
> > you should use clk_set_max_rate(). (The claim was somewhat weakend by
> > Stephen, but still I think that clk_round_rate is the right approach.)
> 
> Well, you said that I shouln't rely on the fact that clk_round_rate() will
> round down. That completely defeats the previous algorithm. So please tell
> me how to use it correctly, because I don't see it.

Using clk_round_rate correctly without additional knowledge is hard. If
you assume at least some sane behaviour you'd still have to call it
multiple times. Assuming maxrate is the maximal rate you can handle
without overflowing your PWM registers you have to do:

	rate = maxrate;
	rounded_rate = clk_round_rate(clk, rate);
	while (rounded_rate > rate) {
		if (rate < rounded_rate - rate) {
			/*
			 * clk doesn't support a rate smaller than
			 * maxrate (or the round_rate callback doesn't
			 * round consistently).
			 */
			 return -ESOMETHING;
		}
		rate = rate - (rounded_rate - rate)
		rounded_rate = clk_round_rate(clk, rate);
	}

	return rate;

Probably it would be sensible to put that in a function provided by the
clk framework (maybe call it clk_round_rate_down and maybe with
additional checks).

> I came up with a much smarter alternative, that doesn't rely on the rounding
> method of clk_round_rate, and which is better overall (no loop needed). It
> sounds to me like you're bashing the code without making the effort to
> understand what it does.
> 
> Thierry called it a "neat trick"
> (https://patchwork.kernel.org/patch/10836879/) so it cannot be as bad as you
> say.

Either that or Thierry failed to see the downside. The obvious downside
is that once you set the period to something long (and so the clk was
limited to a small frequency) you never make the clock any faster
afterwards.

Also I wonder how clk_set_max_rate() is supposed to be used like that or
if instead some work should be invested to make it easier for clk
consumers to use clk_round_rate() (e.g. by providing helper functions
like the above). Stephen, can you shed some light into this?
 
> > The upside of clk_round_rate is that it allows you to test for the
> > capabilities of the clock without actually changing it before you found
> > a setting you consider to be good.
> 
> I know what clk_round_rate() is for. But here we don't do trial-and-error to
> find the first highest clock rate that works, we compute the maximum clock
> we can use and limit the clock rate to that.
> 
> > 
> > >  It's enough to run it below a certain rate, yes. The actual rate
> > > doesn't
> > >  actually matter that much.
> > 
> > 1 Hz would be fine? I doubt it.
> 
> We use the highest possible clock rate. We wouldn't use 1 Hz unless it's the
> highest clock rate available.

That's wrong. If the clk already runs at 1 Hz and you call
clk_set_max_rate(rate, somethingincrediblehigh); it still runs at 1 Hz
afterwards. (Unless I missed something.)

> > > > >  E.g. if at a rate of 12 MHz your computed hardware value for the period
> > > > >  is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the clock
> > > > >  rate must be reduced to the highest possible that will still give you a
> > > > >  < 16-bit value.
> > > > >
> > > > >  We always want the highest possible clock rate that works, for the sake of
> > > > >  precision.
> > > >
> > > > This is dubious; but ok to keep the driver simple. (Consider a PWM that
> > > > can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a duty
> > > > cycle of 40 ns is requested you can get an exact match with 25 MHz, but
> > > > not with 30 MHz.)
> > >
> > > The clock rate is actually (parent_rate >> (2 * x) )
> > > for x = 0, 1, 2, ...
> > >
> > > So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, and the
> > > next one is 1.875 MHz. It'd be very unlikely that you get a better match at
> > > a lower clock.
> > 
> > If the smaller freqs are all dividers of the fastest that's fine. Please
> > note in a code comment that you're assuming this.
> 
> No, I am not assuming this. The current driver just picks the highest clock
> rate that works. We're not changing the behaviour here.

But you hide it behind clk API functions that don't guarantee this
behaviour. And even if it works for you it might not for the next person
who copies your code to support another hardware.

Best regards
Uwe

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

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Paul Cercueil]

Le mar. 13 août 2019 à 14:33, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello Paul,
> 
> On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote:
>>  Le mar. 13 août 2019 à 7:27, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > [adding Stephen Boyd to Cc]
>>  >
>>  > On Tue, Aug 13, 2019 at 12:16:23AM +0200, Paul Cercueil wrote:
>>  > > Le lun. 12 août 2019 à 23:48, Uwe Kleine-König a écrit :
>>  > > > On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote:
>>  > > > > Le lun. 12 août 2019 à 8:15, Uwe Kleine-König a écrit :
>>  > > > > > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil 
>> wrote:
>>  > > > > > > Le ven. 9 août 2019 à 19:05, Uwe Kleine-König a 
>> écrit :
>>  > > > > > > > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul 
>> Cercueil wrote:
>>  > > > > > > > > [...]
>>  > > > > > > > >  +	/* Reset the clock to the maximum rate, and 
>> we'll reduce it if needed */
>>  > > > > > > > >  +	ret = clk_set_max_rate(clk, parent_rate);
>>  > > > > > > >
>>  > > > > > > > What is the purpose of this call? IIUC this limits 
>> the allowed range of
>>  > > > > > > > rates for clk. I assume the idea is to prevent other 
>> consumers to change
>>  > > > > > > > the rate in a way that makes it unsuitable for this 
>> pwm. But this only
>>  > > > > > > > makes sense if you had a notifier for clk changes, 
>> doesn't it? I'm
>>  > > > > > > > confused.
>>  > > > > > >
>>  > > > > > > Nothing like that. The second call to 
>> clk_set_max_rate() might have set
>>  > > > > > > a maximum clock rate that's lower than the parent's 
>> rate, and we want to
>>  > > > > > > undo that.
>>  > > > > >
>>  > > > > > I still don't get the purpose of this call. Why do you 
>> limit the clock
>>  > > > > > rate at all?
>>  > > > >
>>  > > > > As it says below, we "limit the clock to a maximum rate 
>> that still gives
>>  > > > > us a period value which fits in 16 bits". So that the 
>> computed hardware
>>  > > > > values won't overflow.
>>  > > >
>>  > > > But why not just using clk_set_rate? You want to have the 
>> clock running
>>  > > > at a certain rate, not any rate below that certain rate, 
>> don't you?
>>  > >
>>  > >  I'll let yourself answer yourself:
>>  > >  https://patchwork.ozlabs.org/patch/1018969/
>>  >
>>  > In that thread I claimed that you used clk_round_rate wrongly, 
>> not that
>>  > you should use clk_set_max_rate(). (The claim was somewhat 
>> weakend by
>>  > Stephen, but still I think that clk_round_rate is the right 
>> approach.)
>> 
>>  Well, you said that I shouln't rely on the fact that 
>> clk_round_rate() will
>>  round down. That completely defeats the previous algorithm. So 
>> please tell
>>  me how to use it correctly, because I don't see it.
> 
> Using clk_round_rate correctly without additional knowledge is hard. 
> If
> you assume at least some sane behaviour you'd still have to call it
> multiple times. Assuming maxrate is the maximal rate you can handle
> without overflowing your PWM registers you have to do:
> 
> 	rate = maxrate;
> 	rounded_rate = clk_round_rate(clk, rate);
> 	while (rounded_rate > rate) {
> 		if (rate < rounded_rate - rate) {
> 			/*
> 			 * clk doesn't support a rate smaller than
> 			 * maxrate (or the round_rate callback doesn't
> 			 * round consistently).
> 			 */
> 			 return -ESOMETHING;
> 		}
> 		rate = rate - (rounded_rate - rate)
> 		rounded_rate = clk_round_rate(clk, rate);
> 	}
> 
> 	return rate;
> 
> Probably it would be sensible to put that in a function provided by 
> the
> clk framework (maybe call it clk_round_rate_down and maybe with
> additional checks).

clk_round_rate_down() has been refused multiple times in the past for 
reasons that Stephen can explain.


> 
>>  I came up with a much smarter alternative, that doesn't rely on the 
>> rounding
>>  method of clk_round_rate, and which is better overall (no loop 
>> needed). It
>>  sounds to me like you're bashing the code without making the effort 
>> to
>>  understand what it does.
>> 
>>  Thierry called it a "neat trick"
>>  (https://patchwork.kernel.org/patch/10836879/) so it cannot be as 
>> bad as you
>>  say.
> 
> Either that or Thierry failed to see the downside. The obvious 
> downside
> is that once you set the period to something long (and so the clk was
> limited to a small frequency) you never make the clock any faster
> afterwards.

Read the algorithm again.


> 
> Also I wonder how clk_set_max_rate() is supposed to be used like that 
> or
> if instead some work should be invested to make it easier for clk
> consumers to use clk_round_rate() (e.g. by providing helper functions
> like the above). Stephen, can you shed some light into this?
> 
>>  > The upside of clk_round_rate is that it allows you to test for the
>>  > capabilities of the clock without actually changing it before you 
>> found
>>  > a setting you consider to be good.
>> 
>>  I know what clk_round_rate() is for. But here we don't do 
>> trial-and-error to
>>  find the first highest clock rate that works, we compute the 
>> maximum clock
>>  we can use and limit the clock rate to that.
>> 
>>  >
>>  > >  It's enough to run it below a certain rate, yes. The actual 
>> rate
>>  > > doesn't
>>  > >  actually matter that much.
>>  >
>>  > 1 Hz would be fine? I doubt it.
>> 
>>  We use the highest possible clock rate. We wouldn't use 1 Hz unless 
>> it's the
>>  highest clock rate available.
> 
> That's wrong. If the clk already runs at 1 Hz and you call
> clk_set_max_rate(rate, somethingincrediblehigh); it still runs at 1 Hz
> afterwards. (Unless I missed something.)

You missed something. I reset the max rate to the parent clock's rate 
at the beginning of the algorithm. It works just fine.


> 
>>  > > > >  E.g. if at a rate of 12 MHz your computed hardware value 
>> for the period
>>  > > > >  is 0xf000, then at a rate of 24 MHz it won't fit in 16 
>> bits. So the clock
>>  > > > >  rate must be reduced to the highest possible that will 
>> still give you a
>>  > > > >  < 16-bit value.
>>  > > > >
>>  > > > >  We always want the highest possible clock rate that works, 
>> for the sake of
>>  > > > >  precision.
>>  > > >
>>  > > > This is dubious; but ok to keep the driver simple. (Consider 
>> a PWM that
>>  > > > can run at i MHz for i in [1, .. 30]. If a period of 120 ns 
>> and a duty
>>  > > > cycle of 40 ns is requested you can get an exact match with 
>> 25 MHz, but
>>  > > > not with 30 MHz.)
>>  > >
>>  > > The clock rate is actually (parent_rate >> (2 * x) )
>>  > > for x = 0, 1, 2, ...
>>  > >
>>  > > So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, 
>> and the
>>  > > next one is 1.875 MHz. It'd be very unlikely that you get a 
>> better match at
>>  > > a lower clock.
>>  >
>>  > If the smaller freqs are all dividers of the fastest that's fine. 
>> Please
>>  > note in a code comment that you're assuming this.
>> 
>>  No, I am not assuming this. The current driver just picks the 
>> highest clock
>>  rate that works. We're not changing the behaviour here.
> 
> But you hide it behind clk API functions that don't guarantee this
> behaviour. And even if it works for you it might not for the next 
> person
> who copies your code to support another hardware.

Again, I'm not *trying* to guarantee this behaviour.


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

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Uwe Kleine-König]

Hello Paul,

On Tue, Aug 13, 2019 at 02:47:28PM +0200, Paul Cercueil wrote:
> Le mar. 13 août 2019 à 14:33, Uwe Kleine-König a écrit :
> > On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote:
> > > Well, you said that I shouln't rely on the fact that clk_round_rate() will
> > > round down. That completely defeats the previous algorithm. So please tell
> > > me how to use it correctly, because I don't see it.
> > 
> > Using clk_round_rate correctly without additional knowledge is hard. If
> > you assume at least some sane behaviour you'd still have to call it
> > multiple times. Assuming maxrate is the maximal rate you can handle
> > without overflowing your PWM registers you have to do:
> > 
> > 	rate = maxrate;
> > 	rounded_rate = clk_round_rate(clk, rate);
> > 	while (rounded_rate > rate) {
> > 		if (rate < rounded_rate - rate) {
> > 			/*
> > 			 * clk doesn't support a rate smaller than
> > 			 * maxrate (or the round_rate callback doesn't
> > 			 * round consistently).
> > 			 */
> > 			 return -ESOMETHING;
> > 		}
> > 		rate = rate - (rounded_rate - rate)
> > 		rounded_rate = clk_round_rate(clk, rate);
> > 	}
> > 
> > 	return rate;
> > 
> > Probably it would be sensible to put that in a function provided by the
> > clk framework (maybe call it clk_round_rate_down and maybe with
> > additional checks).
> 
> clk_round_rate_down() has been refused multiple times in the past for
> reasons that Stephen can explain.

I'd be really interested in these reasons as I think the clk framework
should make it easy to solve common tasks related to clocks. And finding
out the biggest supported rate not bigger than a given maxrate is
something I consider such a common task.

The first hit I found when searching was
https://lkml.org/lkml/2010/7/14/260 . In there Stephen suggested that
clk_round_rate with the current semantic is hardly useful and suggested
clk_round_rate_up() and clk_round_rate_down() himself.
 
> > >  I came up with a much smarter alternative, that doesn't rely on the rounding
> > >  method of clk_round_rate, and which is better overall (no loop needed). It
> > >  sounds to me like you're bashing the code without making the effort to
> > >  understand what it does.
> > > 
> > >  Thierry called it a "neat trick"
> > >  (https://patchwork.kernel.org/patch/10836879/) so it cannot be as bad as you
> > >  say.
> > 
> > Either that or Thierry failed to see the downside. The obvious downside
> > is that once you set the period to something long (and so the clk was
> > limited to a small frequency) you never make the clock any faster
> > afterwards.
> 
> Read the algorithm again.

I indeed missed a call to clk_set_rate(clk, parent_rate). I thought I
grepped for clk_set_rate before claiming the code was broken. Sorry.

So I think the code works indeed, but it feels like abusing
clk_set_max_rate. So I'd like to see some words from Stephen about this
procedure.

Also I think this is kind of inelegant to set the maximal rate twice. At
least call clk_set_max_rate only once please.

> > > > > > >  E.g. if at a rate of 12 MHz your computed hardware value for the period
> > > > > > >  is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the clock
> > > > > > >  rate must be reduced to the highest possible that will still give you a
> > > > > > >  < 16-bit value.
> > > > > > >
> > > > > > >  We always want the highest possible clock rate that works, for the sake of
> > > > > > >  precision.
> > > > > >
> > > > > > This is dubious; but ok to keep the driver simple. (Consider a PWM that
> > > > > > can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a duty
> > > > > > cycle of 40 ns is requested you can get an exact match with 25 MHz, but
> > > > > > not with 30 MHz.)
> > > > >
> > > > > The clock rate is actually (parent_rate >> (2 * x) )
> > > > > for x = 0, 1, 2, ...
> > > > >
> > > > > So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, and the
> > > > > next one is 1.875 MHz. It'd be very unlikely that you get a better match at
> > > > > a lower clock.
> > > >
> > > > If the smaller freqs are all dividers of the fastest that's fine. Please
> > > > note in a code comment that you're assuming this.
> > > 
> > >  No, I am not assuming this. The current driver just picks the highest clock
> > >  rate that works. We're not changing the behaviour here.
> > 
> > But you hide it behind clk API functions that don't guarantee this
> > behaviour. And even if it works for you it might not for the next person
> > who copies your code to support another hardware.
> 
> Again, I'm not *trying* to guarantee this behaviour.

I didn't request you should guarantee this behaviour. I want you to make
it obvious for readers of your code that you rely on something that
isn't guaranteed. That your code works today isn't a good enough excuse.
There are various examples like these. If you want a few:

 - printf("string: %s\n", NULL); works fine with glibc, but segfaults on
   other libcs.
 - setenv("MYVAR", NULL) used to work (and was equivalent to
   setenv("MYVAR", "")) but that was never guaranteed. Then at some
   point of time it started to segfault.
 - Look into commits like a4435febd4c0f14b25159dca249ecf91301c7c76. This
   used to work fine until compilers were changed to optimize more
   aggressively.

Now if you use a clk and know that all rates smaller than the requested
one are divisors of the fast one and your code only works (here: is
optimal) when this condition is given, you're walking on thin ice just
because this fact it's not guaranteed.
The least you can do is to add a code comment to make people aware who
debug the breakage or copy your code.

I admit this wasn't optimal already before, but at least the logic was
in the same code and not hidden behind the clk API.

Please do people who review or copy your code the favour to document the
assumptions you're relying on. And if it's only to save some time for
someone who stumbles over your code who knows the clk API and starts
thinking about improving the driver.

Best regards
Uwe

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

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Paul Cercueil]

Hi Uwe,


Le mar. 13 août 2019 à 16:09, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello Paul,
> 
> On Tue, Aug 13, 2019 at 02:47:28PM +0200, Paul Cercueil wrote:
>>  Le mar. 13 août 2019 à 14:33, Uwe Kleine-König a écrit :
>>  > On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote:
>>  > > Well, you said that I shouln't rely on the fact that 
>> clk_round_rate() will
>>  > > round down. That completely defeats the previous algorithm. So 
>> please tell
>>  > > me how to use it correctly, because I don't see it.
>>  >
>>  > Using clk_round_rate correctly without additional knowledge is 
>> hard. If
>>  > you assume at least some sane behaviour you'd still have to call 
>> it
>>  > multiple times. Assuming maxrate is the maximal rate you can 
>> handle
>>  > without overflowing your PWM registers you have to do:
>>  >
>>  > 	rate = maxrate;
>>  > 	rounded_rate = clk_round_rate(clk, rate);
>>  > 	while (rounded_rate > rate) {
>>  > 		if (rate < rounded_rate - rate) {
>>  > 			/*
>>  > 			 * clk doesn't support a rate smaller than
>>  > 			 * maxrate (or the round_rate callback doesn't
>>  > 			 * round consistently).
>>  > 			 */
>>  > 			 return -ESOMETHING;
>>  > 		}
>>  > 		rate = rate - (rounded_rate - rate)
>>  > 		rounded_rate = clk_round_rate(clk, rate);
>>  > 	}
>>  >
>>  > 	return rate;
>>  >
>>  > Probably it would be sensible to put that in a function provided 
>> by the
>>  > clk framework (maybe call it clk_round_rate_down and maybe with
>>  > additional checks).
>> 
>>  clk_round_rate_down() has been refused multiple times in the past 
>> for
>>  reasons that Stephen can explain.
> 
> I'd be really interested in these reasons as I think the clk framework
> should make it easy to solve common tasks related to clocks. And 
> finding
> out the biggest supported rate not bigger than a given maxrate is
> something I consider such a common task.
> 
> The first hit I found when searching was
> https://lkml.org/lkml/2010/7/14/260 . In there Stephen suggested that
> clk_round_rate with the current semantic is hardly useful and 
> suggested
> clk_round_rate_up() and clk_round_rate_down() himself.

That's from 2010, though.

I agree that clk_round_rate_up() and clk_round_rate_down() should 
exist. Even if they return -ENOSYS if it's not implemented for a given 
clock controller.

> 
>>  > >  I came up with a much smarter alternative, that doesn't rely 
>> on the rounding
>>  > >  method of clk_round_rate, and which is better overall (no loop 
>> needed). It
>>  > >  sounds to me like you're bashing the code without making the 
>> effort to
>>  > >  understand what it does.
>>  > >
>>  > >  Thierry called it a "neat trick"
>>  > >  (https://patchwork.kernel.org/patch/10836879/) so it cannot be 
>> as bad as you
>>  > >  say.
>>  >
>>  > Either that or Thierry failed to see the downside. The obvious 
>> downside
>>  > is that once you set the period to something long (and so the clk 
>> was
>>  > limited to a small frequency) you never make the clock any faster
>>  > afterwards.
>> 
>>  Read the algorithm again.
> 
> I indeed missed a call to clk_set_rate(clk, parent_rate). I thought I
> grepped for clk_set_rate before claiming the code was broken. Sorry.
> 
> So I think the code works indeed, but it feels like abusing
> clk_set_max_rate. So I'd like to see some words from Stephen about 
> this
> procedure.
> 
> Also I think this is kind of inelegant to set the maximal rate twice. 
> At
> least call clk_set_max_rate only once please.

Ok. I can do that.

> 
>>  > > > > > >  E.g. if at a rate of 12 MHz your computed hardware 
>> value for the period
>>  > > > > > >  is 0xf000, then at a rate of 24 MHz it won't fit in 16 
>> bits. So the clock
>>  > > > > > >  rate must be reduced to the highest possible that will 
>> still give you a
>>  > > > > > >  < 16-bit value.
>>  > > > > > >
>>  > > > > > >  We always want the highest possible clock rate that 
>> works, for the sake of
>>  > > > > > >  precision.
>>  > > > > >
>>  > > > > > This is dubious; but ok to keep the driver simple. 
>> (Consider a PWM that
>>  > > > > > can run at i MHz for i in [1, .. 30]. If a period of 120 
>> ns and a duty
>>  > > > > > cycle of 40 ns is requested you can get an exact match 
>> with 25 MHz, but
>>  > > > > > not with 30 MHz.)
>>  > > > >
>>  > > > > The clock rate is actually (parent_rate >> (2 * x) )
>>  > > > > for x = 0, 1, 2, ...
>>  > > > >
>>  > > > > So if your parent_rate is 30 MHz the next valid one is 7.5 
>> MHz, and the
>>  > > > > next one is 1.875 MHz. It'd be very unlikely that you get a 
>> better match at
>>  > > > > a lower clock.
>>  > > >
>>  > > > If the smaller freqs are all dividers of the fastest that's 
>> fine. Please
>>  > > > note in a code comment that you're assuming this.
>>  > >
>>  > >  No, I am not assuming this. The current driver just picks the 
>> highest clock
>>  > >  rate that works. We're not changing the behaviour here.
>>  >
>>  > But you hide it behind clk API functions that don't guarantee this
>>  > behaviour. And even if it works for you it might not for the next 
>> person
>>  > who copies your code to support another hardware.
>> 
>>  Again, I'm not *trying* to guarantee this behaviour.
> 
> I didn't request you should guarantee this behaviour. I want you to 
> make
> it obvious for readers of your code that you rely on something that
> isn't guaranteed. That your code works today isn't a good enough 
> excuse.
> There are various examples like these. If you want a few:
> 
>  - printf("string: %s\n", NULL); works fine with glibc, but segfaults 
> on
>    other libcs.
>  - setenv("MYVAR", NULL) used to work (and was equivalent to
>    setenv("MYVAR", "")) but that was never guaranteed. Then at some
>    point of time it started to segfault.
>  - Look into commits like a4435febd4c0f14b25159dca249ecf91301c7c76. 
> This
>    used to work fine until compilers were changed to optimize more
>    aggressively.
> 
> Now if you use a clk and know that all rates smaller than the 
> requested
> one are divisors of the fast one and your code only works (here: is
> optimal) when this condition is given, you're walking on thin ice just
> because this fact it's not guaranteed.
> The least you can do is to add a code comment to make people aware who
> debug the breakage or copy your code.

If I was assuming something, it's not that the requested clock rates 
are always integer dividers of the parent rate - but rather that the 
difference in precision between two possible clock rates (even 
non-integer-dividers) is so tiny that we just don't care.

> 
> I admit this wasn't optimal already before, but at least the logic was
> in the same code and not hidden behind the clk API.
> 
> Please do people who review or copy your code the favour to document 
> the
> assumptions you're relying on. And if it's only to save some time for
> someone who stumbles over your code who knows the clk API and starts
> thinking about improving the driver.

Ok. I can add a comment.

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

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Uwe Kleine-König]

Hello Paul,

On Wed, Aug 14, 2019 at 06:10:35PM +0200, Paul Cercueil wrote:
> Le mar. 13 août 2019 à 16:09, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= a écrit :
> > On Tue, Aug 13, 2019 at 02:47:28PM +0200, Paul Cercueil wrote:
> > > Le mar. 13 août 2019 à 14:33, Uwe Kleine-König a écrit :
> > > > On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote:
> > > > > Well, you said that I shouln't rely on the fact that clk_round_rate() will
> > > > > round down. That completely defeats the previous algorithm. So please tell
> > > > > me how to use it correctly, because I don't see it.
> > > >
> > > > Using clk_round_rate correctly without additional knowledge is hard. If
> > > > you assume at least some sane behaviour you'd still have to call it
> > > > multiple times. Assuming maxrate is the maximal rate you can handle
> > > > without overflowing your PWM registers you have to do:
> > > >
> > > > 	rate = maxrate;
> > > > 	rounded_rate = clk_round_rate(clk, rate);
> > > > 	while (rounded_rate > rate) {
> > > > 		if (rate < rounded_rate - rate) {
> > > > 			/*
> > > > 			 * clk doesn't support a rate smaller than
> > > > 			 * maxrate (or the round_rate callback doesn't
> > > > 			 * round consistently).
> > > > 			 */
> > > > 			 return -ESOMETHING;
> > > > 		}
> > > > 		rate = rate - (rounded_rate - rate)
> > > > 		rounded_rate = clk_round_rate(clk, rate);
> > > > 	}
> > > >
> > > > 	return rate;
> > > >
> > > > Probably it would be sensible to put that in a function provided by the
> > > > clk framework (maybe call it clk_round_rate_down and maybe with
> > > > additional checks).
> > > 
> > >  clk_round_rate_down() has been refused multiple times in the past for
> > >  reasons that Stephen can explain.
> > 
> > I'd be really interested in these reasons as I think the clk framework
> > should make it easy to solve common tasks related to clocks. And finding
> > out the biggest supported rate not bigger than a given maxrate is
> > something I consider such a common task.
> > 
> > The first hit I found when searching was
> > https://lkml.org/lkml/2010/7/14/260 . In there Stephen suggested that
> > clk_round_rate with the current semantic is hardly useful and suggested
> > clk_round_rate_up() and clk_round_rate_down() himself.
> 
> That's from 2010, though.

If you have a better link please tell me.

> I agree that clk_round_rate_up() and clk_round_rate_down() should exist.
> Even if they return -ENOSYS if it's not implemented for a given clock
> controller.

ack.

> > > > > I came up with a much smarter alternative, that doesn't rely on the rounding
> > > > > method of clk_round_rate, and which is better overall (no loop needed). It
> > > > > sounds to me like you're bashing the code without making the effort to
> > > > > understand what it does.
> > > > >
> > > > > Thierry called it a "neat trick"
> > > > > (https://patchwork.kernel.org/patch/10836879/) so it cannot be as bad as you
> > > > > say.
> > > >
> > > > Either that or Thierry failed to see the downside. The obvious downside
> > > > is that once you set the period to something long (and so the clk was
> > > > limited to a small frequency) you never make the clock any faster
> > > > afterwards.
> > >
> > >  Read the algorithm again.
> > 
> > I indeed missed a call to clk_set_rate(clk, parent_rate). I thought I
> > grepped for clk_set_rate before claiming the code was broken. Sorry.
> > 
> > So I think the code works indeed, but it feels like abusing
> > clk_set_max_rate. So I'd like to see some words from Stephen about this
> > procedure.
> > 
> > Also I think this is kind of inelegant to set the maximal rate twice. At
> > least call clk_set_max_rate only once please.
> 
> Ok. I can do that.

I would still prefer to hear from Stephen about this approach. It seems
wrong to have two different ways to achieve the same goal and my
impression is that clk_round_rate is the function designed for this use
case.
 
> > > > > > > > > E.g. if at a rate of 12 MHz your computed hardware value for the period
> > > > > > > > > is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the clock
> > > > > > > > > rate must be reduced to the highest possible that will still give you a
> > > > > > > > > < 16-bit value.
> > > > > > > > >
> > > > > > > > > We always want the highest possible clock rate that works, for the sake of
> > > > > > > > > precision.
> > > > > > > >
> > > > > > > > This is dubious; but ok to keep the driver simple.> (Consider a PWM that
> > > > > > > > can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a duty
> > > > > > > > cycle of 40 ns is requested you can get an exact match with 25 MHz, but
> > > > > > > > not with 30 MHz.)
> > > > > > >
> > > > > > > The clock rate is actually (parent_rate >> (2 * x) )
> > > > > > > for x = 0, 1, 2, ...
> > > > > > >
> > > > > > > So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, and the
> > > > > > > next one is 1.875 MHz. It'd be very unlikely that you get a better match at
> > > > > > > a lower clock.
> > > > > >
> > > > > > If the smaller freqs are all dividers of the fastest that's fine. Please
> > > > > > note in a code comment that you're assuming this.
> > > > >
> > > > >  No, I am not assuming this. The current driver just picks the highest clock
> > > > >  rate that works. We're not changing the behaviour here.
> > > >
> > > > But you hide it behind clk API functions that don't guarantee this
> > > > behaviour. And even if it works for you it might not for the next person
> > > > who copies your code to support another hardware.
> > > 
> > >  Again, I'm not *trying* to guarantee this behaviour.
> > 
> > I didn't request you should guarantee this behaviour. I want you to make
> > it obvious for readers of your code that you rely on something that
> > isn't guaranteed. That your code works today isn't a good enough excuse.
> > There are various examples like these. If you want a few:
> > 
> >  - printf("string: %s\n", NULL); works fine with glibc, but segfaults on
> >    other libcs.
> >  - setenv("MYVAR", NULL) used to work (and was equivalent to
> >    setenv("MYVAR", "")) but that was never guaranteed. Then at some
> >    point of time it started to segfault.
> >  - Look into commits like a4435febd4c0f14b25159dca249ecf91301c7c76. This
> >    used to work fine until compilers were changed to optimize more
> >    aggressively.
> > 
> > Now if you use a clk and know that all rates smaller than the requested
> > one are divisors of the fast one and your code only works (here: is
> > optimal) when this condition is given, you're walking on thin ice just
> > because this fact it's not guaranteed.
> > The least you can do is to add a code comment to make people aware who
> > debug the breakage or copy your code.
> 
> If I was assuming something, it's not that the requested clock rates are
> always integer dividers of the parent rate - but rather that the difference
> in precision between two possible clock rates (even non-integer-dividers) is
> so tiny that we just don't care.

I'm more exacting here. If you are asked for X and can provide X - 2 you
shouldn't provide X - 12. Depending on the use case the consumer is happy
about every bit of accuracy they can get. So if you deliberately provide
X - 12 because it is easier to do and good enough for you, at least
document this laziness to not waste other people's time more than
necessary.

Best regards
Uwe

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

Re: [PATCH 6/7] pwm: jz4740: Make PWM start with the active part

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2530 bytes --]

On Mon, Aug 12, 2019 at 11:58:53PM +0200, Uwe Kleine-König wrote:
> On Mon, Aug 12, 2019 at 10:50:01PM +0200, Paul Cercueil wrote:
> > 
> > 
> > Le lun. 12 août 2019 à 7:55, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> > <u.kleine-koenig@pengutronix.de> a écrit :
> > > On Fri, Aug 09, 2019 at 07:33:24PM +0200, Paul Cercueil wrote:
> > > > 
> > > > 
> > > >  Le ven. 9 août 2019 à 19:10, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> > > >  <u.kleine-koenig@pengutronix.de> a écrit :
> > > >  > On Fri, Aug 09, 2019 at 02:30:30PM +0200, Paul Cercueil wrote:
> > > >  > >  The PWM will always start with the inactive part. To counter
> > > > that,
> > > >  > >  when PWM is enabled we switch the configured polarity, and use
> > > >  > >  'period - duty + 1' as the real duty.
> > > >  >
> > > >  > Where does the + 1 come from? This looks wrong. (So if duty=0 is
> > > >  > requested you use duty = period + 1?)
> > > > 
> > > >  You'd never request duty == 0, would you?
> > > > 
> > > >  Your duty must always be in the inclusive range [1, period]
> > > >  (hardware values, not ns). A duty of 0 is a hardware fault
> > > >  (on the jz4740 it is).
> > > 
> > > From the PWM framework's POV duty cycle = 0 is perfectly valid. Similar
> > > to duty == period. Not supporting dutz cycle 0 is another limitation of
> > > your PWM that should be documented.
> > > 
> > > For actual use cases of duty cycle = 0 see drivers/hwmon/pwm-fan.c or
> > > drivers/leds/leds-pwm.c.
> > 
> > Perfectly valid for the PWM framework, maybe; but what is the expected
> > output then? A constant inactive state?
> 
> Yes, a constant inactive state is expected. This is consistent and in a
> similar way when using duty == period an constant active output is
> expected.
> 
> > Then I guess I can just disable the PWM output in the driver when
> > configured with duty == 0.
> 
> Some time ago I argued with Thierry that we could drop the concept of
> enabled/disabled for a PWM because a disabled PWM is supposed to behave
> identically to duty=0. This is however only nearly true because with
> duty=0 the time the PWM is inactive still is a multiple of the period.
> 
> I tend to agree that disabling the PWM when duty=0 is requested is
> better than to fail the request (or configure for duty=1 $whateverunit).
> I'm looking forward to what Thierry's opinion is here.

Agreed. If in order to meet the expectations of duty == 0 you have to
disable the PWM, then that's what you should do.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Paul Cercueil]

Hi,


Le mer., août 14, 2019 at 19:32, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello Paul,
> 
> On Wed, Aug 14, 2019 at 06:10:35PM +0200, Paul Cercueil wrote:
>>  Le mar. 13 août 2019 à 16:09, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
>> a écrit :
>>  > On Tue, Aug 13, 2019 at 02:47:28PM +0200, Paul Cercueil wrote:
>>  > > Le mar. 13 août 2019 à 14:33, Uwe Kleine-König a écrit :
>>  > > > On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote:
>>  > > > > Well, you said that I shouln't rely on the fact that 
>> clk_round_rate() will
>>  > > > > round down. That completely defeats the previous algorithm. 
>> So please tell
>>  > > > > me how to use it correctly, because I don't see it.
>>  > > >
>>  > > > Using clk_round_rate correctly without additional knowledge 
>> is hard. If
>>  > > > you assume at least some sane behaviour you'd still have to 
>> call it
>>  > > > multiple times. Assuming maxrate is the maximal rate you can 
>> handle
>>  > > > without overflowing your PWM registers you have to do:
>>  > > >
>>  > > > 	rate = maxrate;
>>  > > > 	rounded_rate = clk_round_rate(clk, rate);
>>  > > > 	while (rounded_rate > rate) {
>>  > > > 		if (rate < rounded_rate - rate) {
>>  > > > 			/*
>>  > > > 			 * clk doesn't support a rate smaller than
>>  > > > 			 * maxrate (or the round_rate callback doesn't
>>  > > > 			 * round consistently).
>>  > > > 			 */
>>  > > > 			 return -ESOMETHING;
>>  > > > 		}
>>  > > > 		rate = rate - (rounded_rate - rate)
>>  > > > 		rounded_rate = clk_round_rate(clk, rate);
>>  > > > 	}
>>  > > >
>>  > > > 	return rate;
>>  > > >
>>  > > > Probably it would be sensible to put that in a function 
>> provided by the
>>  > > > clk framework (maybe call it clk_round_rate_down and maybe 
>> with
>>  > > > additional checks).
>>  > >
>>  > >  clk_round_rate_down() has been refused multiple times in the 
>> past for
>>  > >  reasons that Stephen can explain.
>>  >
>>  > I'd be really interested in these reasons as I think the clk 
>> framework
>>  > should make it easy to solve common tasks related to clocks. And 
>> finding
>>  > out the biggest supported rate not bigger than a given maxrate is
>>  > something I consider such a common task.
>>  >
>>  > The first hit I found when searching was
>>  > https://lkml.org/lkml/2010/7/14/260 . In there Stephen suggested 
>> that
>>  > clk_round_rate with the current semantic is hardly useful and 
>> suggested
>>  > clk_round_rate_up() and clk_round_rate_down() himself.
>> 
>>  That's from 2010, though.
> 
> If you have a better link please tell me.
> 
>>  I agree that clk_round_rate_up() and clk_round_rate_down() should 
>> exist.
>>  Even if they return -ENOSYS if it's not implemented for a given 
>> clock
>>  controller.
> 
> ack.
> 
>>  > > > > I came up with a much smarter alternative, that doesn't 
>> rely on the rounding
>>  > > > > method of clk_round_rate, and which is better overall (no 
>> loop needed). It
>>  > > > > sounds to me like you're bashing the code without making 
>> the effort to
>>  > > > > understand what it does.
>>  > > > >
>>  > > > > Thierry called it a "neat trick"
>>  > > > > (https://patchwork.kernel.org/patch/10836879/) so it cannot 
>> be as bad as you
>>  > > > > say.
>>  > > >
>>  > > > Either that or Thierry failed to see the downside. The 
>> obvious downside
>>  > > > is that once you set the period to something long (and so the 
>> clk was
>>  > > > limited to a small frequency) you never make the clock any 
>> faster
>>  > > > afterwards.
>>  > >
>>  > >  Read the algorithm again.
>>  >
>>  > I indeed missed a call to clk_set_rate(clk, parent_rate). I 
>> thought I
>>  > grepped for clk_set_rate before claiming the code was broken. 
>> Sorry.
>>  >
>>  > So I think the code works indeed, but it feels like abusing
>>  > clk_set_max_rate. So I'd like to see some words from Stephen 
>> about this
>>  > procedure.
>>  >
>>  > Also I think this is kind of inelegant to set the maximal rate 
>> twice. At
>>  > least call clk_set_max_rate only once please.
>> 
>>  Ok. I can do that.
> 
> I would still prefer to hear from Stephen about this approach. It 
> seems
> wrong to have two different ways to achieve the same goal and my
> impression is that clk_round_rate is the function designed for this 
> use
> case.

Stephen, any feedback?
I'm still stuck here.


>>  > > > > > > > > E.g. if at a rate of 12 MHz your computed hardware 
>> value for the period
>>  > > > > > > > > is 0xf000, then at a rate of 24 MHz it won't fit in 
>> 16 bits. So the clock
>>  > > > > > > > > rate must be reduced to the highest possible that 
>> will still give you a
>>  > > > > > > > > < 16-bit value.
>>  > > > > > > > >
>>  > > > > > > > > We always want the highest possible clock rate that 
>> works, for the sake of
>>  > > > > > > > > precision.
>>  > > > > > > >
>>  > > > > > > > This is dubious; but ok to keep the driver simple.> 
>> (Consider a PWM that
>>  > > > > > > > can run at i MHz for i in [1, .. 30]. If a period of 
>> 120 ns and a duty
>>  > > > > > > > cycle of 40 ns is requested you can get an exact 
>> match with 25 MHz, but
>>  > > > > > > > not with 30 MHz.)
>>  > > > > > >
>>  > > > > > > The clock rate is actually (parent_rate >> (2 * x) )
>>  > > > > > > for x = 0, 1, 2, ...
>>  > > > > > >
>>  > > > > > > So if your parent_rate is 30 MHz the next valid one is 
>> 7.5 MHz, and the
>>  > > > > > > next one is 1.875 MHz. It'd be very unlikely that you 
>> get a better match at
>>  > > > > > > a lower clock.
>>  > > > > >
>>  > > > > > If the smaller freqs are all dividers of the fastest 
>> that's fine. Please
>>  > > > > > note in a code comment that you're assuming this.
>>  > > > >
>>  > > > >  No, I am not assuming this. The current driver just picks 
>> the highest clock
>>  > > > >  rate that works. We're not changing the behaviour here.
>>  > > >
>>  > > > But you hide it behind clk API functions that don't guarantee 
>> this
>>  > > > behaviour. And even if it works for you it might not for the 
>> next person
>>  > > > who copies your code to support another hardware.
>>  > >
>>  > >  Again, I'm not *trying* to guarantee this behaviour.
>>  >
>>  > I didn't request you should guarantee this behaviour. I want you 
>> to make
>>  > it obvious for readers of your code that you rely on something 
>> that
>>  > isn't guaranteed. That your code works today isn't a good enough 
>> excuse.
>>  > There are various examples like these. If you want a few:
>>  >
>>  >  - printf("string: %s\n", NULL); works fine with glibc, but 
>> segfaults on
>>  >    other libcs.
>>  >  - setenv("MYVAR", NULL) used to work (and was equivalent to
>>  >    setenv("MYVAR", "")) but that was never guaranteed. Then at 
>> some
>>  >    point of time it started to segfault.
>>  >  - Look into commits like 
>> a4435febd4c0f14b25159dca249ecf91301c7c76. This
>>  >    used to work fine until compilers were changed to optimize more
>>  >    aggressively.
>>  >
>>  > Now if you use a clk and know that all rates smaller than the 
>> requested
>>  > one are divisors of the fast one and your code only works (here: 
>> is
>>  > optimal) when this condition is given, you're walking on thin ice 
>> just
>>  > because this fact it's not guaranteed.
>>  > The least you can do is to add a code comment to make people 
>> aware who
>>  > debug the breakage or copy your code.
>> 
>>  If I was assuming something, it's not that the requested clock 
>> rates are
>>  always integer dividers of the parent rate - but rather that the 
>> difference
>>  in precision between two possible clock rates (even 
>> non-integer-dividers) is
>>  so tiny that we just don't care.
> 
> I'm more exacting here. If you are asked for X and can provide X - 2 
> you
> shouldn't provide X - 12. Depending on the use case the consumer is 
> happy
> about every bit of accuracy they can get. So if you deliberately 
> provide
> X - 12 because it is easier to do and good enough for you, at least
> document this laziness to not waste other people's time more than
> necessary.
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |

About rounding in the clk framework [Was: Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation]

[Uwe Kleine-König]

Hello Stephen, hello Michael,

first some words about the context for the newcomers in this thread (or
those who already got the earlier mails some time ago and obliterated the
details):

The task at hand is to set the frequency of a parent clock to be able to
setup a PWM to yield a certain period and duty cycle. For that there is
an upper limit of the frequency and otherwise we want the clock to run
as fast as possible[1].

On Mon, Oct 21, 2019 at 02:47:57PM +0200, Paul Cercueil wrote:
> Le mer., août 14, 2019 at 19:32, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Wed, Aug 14, 2019 at 06:10:35PM +0200, Paul Cercueil wrote:
> > > Le mar. 13 août 2019 à 16:09, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= a écrit :
> > > > On Tue, Aug 13, 2019 at 02:47:28PM +0200, Paul Cercueil wrote:
> > > > > Le mar. 13 août 2019 à 14:33, Uwe Kleine-König a écrit :
> > > > > > Using clk_round_rate correctly without additional knowledge is hard. If
> > > > > > you assume at least some sane behaviour you'd still have to call it
> > > > > > multiple times. Assuming maxrate is the maximal rate you can handle
> > > > > > without overflowing your PWM registers you have to do:
> > > > > >
> > > > > > 	rate = maxrate;
> > > > > > 	rounded_rate = clk_round_rate(clk, rate);
> > > > > > 	while (rounded_rate > rate) {
> > > > > > 		if (rate < rounded_rate - rate) {
> > > > > > 			/*
> > > > > > 			 * clk doesn't support a rate smaller than
> > > > > > 			 * maxrate (or the round_rate callback doesn't
> > > > > > 			 * round consistently).
> > > > > > 			 */
> > > > > > 			 return -ESOMETHING;
> > > > > > 		}
> > > > > > 		rate = rate - (rounded_rate - rate)
> > > > > > 		rounded_rate = clk_round_rate(clk, rate);
> > > > > > 	}
> > > > > >
> > > > > > 	return rate;
> > > > > >
> > > > > > Probably it would be sensible to put that in a function provided by the
> > > > > > clk framework (maybe call it clk_round_rate_down and maybe with
> > > > > > additional checks).
> > > > >
> > > > > clk_round_rate_down() has been refused multiple times in the past for
> > > > > reasons that Stephen can explain.
> > > >
> > > > I'd be really interested in these reasons as I think the clk framework
> > > > should make it easy to solve common tasks related to clocks. And finding
> > > > out the biggest supported rate not bigger than a given maxrate is
> > > > something I consider such a common task.
> > > >
> > > > The first hit I found when searching was
> > > > https://lkml.org/lkml/2010/7/14/260 . In there Stephen suggested that
> > > > clk_round_rate with the current semantic is hardly useful and suggested
> > > > clk_round_rate_up() and clk_round_rate_down() himself.
> > > 
> > > That's from 2010, though.
> > 
> > If you have a better link please tell me.
> > 
> > > I agree that clk_round_rate_up() and clk_round_rate_down() should exist.
> > > Even if they return -ENOSYS if it's not implemented for a given clock
> > > controller.
> > 
> > ack.

Can you please explain what is the reason why clk_round_rate_up/down()
is a bad idea? Would it help to create a patch that introduces these
functions to get the discussion going?

> > > > > > > I came up with a much smarter alternative, that doesn't rely on the rounding
> > > > > > > method of clk_round_rate, and which is better overall (no loop needed). It
> > > > > > > sounds to me like you're bashing the code without making the effort to
> > > > > > > understand what it does.
> > > > > > >
> > > > > > > Thierry called it a "neat trick"
> > > > > > > (https://patchwork.kernel.org/patch/10836879/) so it cannot be as bad as you
> > > > > > > say.
> > > > > > [...]
> > > > > [...]
> > > >
> > > > So I think the code works indeed, but it feels like abusing
> > > > clk_set_max_rate. So I'd like to see some words from Stephen about this
> > > > procedure.

The approach here was as follows:

	clk_set_rate(clk, parentrate);
	clk_set_max_rate(clk, maxfreq);

I don't know what the exact purpose of clk_set_max_rate() is, but it
seems questionable to me if it is supposed to be used like that. (As a
side note: According to the FIXME in clk_set_rate_range() it doesn't even
guarantee that the rate of clk is <= maxfreq after the call.
clk_round_rate_down() would help here, too ...)

Best regards
Uwe

[1] This isn't necessarily the best clk freq, but a reasonable to work
    with.

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

Re: About rounding in the clk framework [Was: Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation]

[Uwe Kleine-König]

Hello Stephen, hello Michael,

On Wed, Feb 12, 2020 at 08:29:11AM +0100, Uwe Kleine-König wrote:
> Can you please explain what is the reason why clk_round_rate_up/down()
> is a bad idea? Would it help to create a patch that introduces these
> functions to get the discussion going?

I didn't get any feedback on my mail. Are you to busy working on more
important stuff? Is the answer so obvious that you don't consider it
worth your time to answer?

Looking a bit through the code I see there are two callbacks hwclks can
provide to implement rounding (determine_rate and round_rate). The docs
for both use the term "return the closes rate actually supported". Does
that mean "round-closest" is already the official policy and other
strategies in lowlevel drivers are a bug?

Best regards
Uwe

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

Re: About rounding in the clk framework [Was: Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation]

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]

Hello,

On Tue, Apr 14, 2020 at 11:24:12AM +0200, Uwe Kleine-König wrote:
> Hello Stephen, hello Michael,
> 
> On Wed, Feb 12, 2020 at 08:29:11AM +0100, Uwe Kleine-König wrote:
> > Can you please explain what is the reason why clk_round_rate_up/down()
> > is a bad idea? Would it help to create a patch that introduces these
> > functions to get the discussion going?
> 
> I didn't get any feedback on my mail. Are you to busy working on more
> important stuff? Is the answer so obvious that you don't consider it
> worth your time to answer?
> 
> Looking a bit through the code I see there are two callbacks hwclks can
> provide to implement rounding (determine_rate and round_rate). The docs
> for both use the term "return the closes rate actually supported". Does
> that mean "round-closest" is already the official policy and other
> strategies in lowlevel drivers are a bug?

Feedback here would be really appreciated. I intend to unify the rounding
behaviour of PWMs to always round down. If there was a similar
constraint for clks, some corner cases might be a bit simpler.

Looking forward to read about your thoughts,
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]