This series consists of various cleanups and improvements for the
pwm-meson driver.
Patches 1 to 6 are small code cleanups with the goal of making the code
easier to read.
Patches 7 to 9 are reworking the way the per-channel settings are
accessed. This is a first preparation step for adding full support to
meson_pwm_get_state() in the pwm-meson driver. Patch 7 makes struct
meson_pwm_channel accessible from struct meson_pwm because
meson_pwm_get_state() cannot use pwm_get_chip_data(). Patch 8 removes
redundant switch/case statements and ensures that we don't have to
add another redundant one for the upcoming full meson_pwm_get_state()
implementation. Patch 9 gets rid of meson_pwm_add_channels() and moves
the pwm_set_chip_data() call to meson_pwm_request() (like all other PWM
drivers do - except two).
Patch 10 is based on a suggestion by Uwe to simplify the calculation of
the values which the PWM IP requires. The nice benefit of this is that
we have an easier calculation which we can do "in reverse" for the
meson_pwm_get_state() (which calculates nanoseconds from the hardware
values).
Patch 11 implements reading the period and duty cycle in the
meson_pwm_get_state() callback.
Patch 12 removes some internal caching which we don't need anymore now
meson_pwm_get_state() is fully implemented. The PWM core now takes care
of not calling pwm_ops.apply() if "nothing has changed".
Patch 13 adds support for PWM_POLARITY_INVERSED when disabling the
output as suggested by Uwe.
Patch 14 completes this series by adding some documentation to the
driver. Thanks to Neil for summarizing how the hardware works
internally.
Due to the changed PWM calculation in patch 10 I have verified that
we don't break any existing boards. The patch itself contains two
examples which show that the new calculation improves precision. I
made screenshots of the measurements in pulseview [0] for the second
case ("PWM LED on Khadas VIM"):
- old algorithm: [1]
- old algorithm: [2]
Dependencies:
This series applies on top of Neil's patch "pwm: pwm-meson: update with
SPDX Licence identifier" [3]
[0] https://sigrok.org/wiki/PulseView
[1] https://abload.de/img/old-algormjs9.png
[2] https://abload.de/img/new-algo4ckjo.png
[3] https://patchwork.kernel.org/patch/10951319/
Martin Blumenstingl (14):
pwm: meson: unify the parameter list of meson_pwm_{enable,disable}
pwm: meson: use devm_clk_get_optional() to get the input clock
pwm: meson: use GENMASK and FIELD_PREP for the lo and hi values
pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK
pwm: meson: don't duplicate the polarity internally
pwm: meson: pass struct pwm_device to meson_pwm_calc()
pwm: meson: add the meson_pwm_channel data to struct meson_pwm
pwm: meson: add the per-channel register offsets and bits in a struct
pwm: meson: move pwm_set_chip_data() to meson_pwm_request()
pwm: meson: simplify the calculation of the pre-divider and count
pwm: meson: read the full hardware state in meson_pwm_get_state()
pwm: meson: don't cache struct pwm_state internally
pwm: meson: add support PWM_POLARITY_INVERSED when disabling
pwm: meson: add documentation to the driver
drivers/pwm/pwm-meson.c | 323 +++++++++++++++++++++-------------------
1 file changed, 169 insertions(+), 154 deletions(-)
--
2.21.0
meson_pwm_apply() has to consider the PWM polarity when disabling the
output.
With enabled=false and polarity=PWM_POLARITY_NORMAL the output needs to
be LOW. The driver already supports this.
With enabled=false and polarity=PWM_POLARITY_INVERSED the output needs
to be HIGH. Implement this in the driver by internally enabling the
output with the same settings that we already use for "period == duty".
This fixes a PWM API violation which expects that the driver honors the
polarity also for enabled=false. Due to the IP block not supporting this
natively we only get "an as close as possible" to 100% HIGH signal (in
my test setup with input clock of 24MHz and measuring the output with a
logic analyzer at 24MHz sampling rate I got a duty cycle of 99.998475%
on a Khadas VIM).
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/pwm/pwm-meson.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 900d362ec3c9..bb48ba85f756 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -245,6 +245,7 @@ static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state)
{
+ struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
struct meson_pwm *meson = to_meson_pwm(chip);
int err = 0;
@@ -252,7 +253,27 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return -EINVAL;
if (!state->enabled) {
- meson_pwm_disable(meson, pwm);
+ if (state->polarity == PWM_POLARITY_INVERSED) {
+ /*
+ * This IP block revision doesn't have an "always high"
+ * setting which we can use for "inverted disabled".
+ * Instead we achieve this using the same settings
+ * that we use a pre_div of 0 (to get the shortest
+ * possible duration for one "count") and
+ * "period == duty_cycle". This results in a signal
+ * which is LOW for one "count", while being HIGH for
+ * the rest of the (so the signal is HIGH for slightly
+ * less than 100% of the period, but this is the best
+ * we can achieve).
+ */
+ channel->pre_div = 0;
+ channel->hi = ~0;
+ channel->lo = 0;
+
+ meson_pwm_enable(meson, pwm);
+ } else {
+ meson_pwm_disable(meson, pwm);
+ }
} else {
err = meson_pwm_calc(meson, pwm, state);
if (err < 0)
--
2.21.0
Replace the loop to calculate the pre-divider and count with two
separate div64_u64() calculations. This makes the code easier to read
and improves the precision.
Two example cases:
1) 32.768kHz LPO clock for the SDIO wifi chip on Khadas VIM
clock input: 500MHz (FCLK_DIV4)
period: 30518ns
duty cycle: 15259ns
old algorithm: pre_div=0, cnt=15259
new algorithm: pre_div=0, cnt=15259
(no difference in calculated values)
2) PWM LED on Khadas VIM
clock input: 24MHz (XTAL)
period: 7812500ns
duty cycle: 7812500ns
old algorithm: pre_div=2, cnt=62004
new algorithm: pre_div=2, cnt=62500
Using a scope (24MHz sampling rate) shows the actual difference:
- old: 7753000ns, off by -59500ns (0.7616%)
- new: 7815000ns, off by +2500ns (0.032%)
Suggested-by: Uwe Kleine-König <[email protected]>
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/pwm/pwm-meson.c | 25 ++++++++++---------------
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 27915d6475e3..9afa1e5aaebf 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -12,6 +12,7 @@
#include <linux/err.h>
#include <linux/io.h>
#include <linux/kernel.h>
+#include <linux/math64.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
@@ -145,7 +146,6 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
unsigned int duty, period, pre_div, cnt, duty_cnt;
unsigned long fin_freq = -1;
- u64 fin_ps;
duty = state->duty_cycle;
period = state->period;
@@ -164,24 +164,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
}
dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
- fin_ps = (u64)NSEC_PER_SEC * 1000;
- do_div(fin_ps, fin_freq);
-
- /* Calc pre_div with the period */
- for (pre_div = 0; pre_div <= MISC_CLK_DIV_MASK; pre_div++) {
- cnt = DIV_ROUND_CLOSEST_ULL((u64)period * 1000,
- fin_ps * (pre_div + 1));
- dev_dbg(meson->chip.dev, "fin_ps=%llu pre_div=%u cnt=%u\n",
- fin_ps, pre_div, cnt);
- if (cnt <= 0xffff)
- break;
- }
+ pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL);
if (pre_div > MISC_CLK_DIV_MASK) {
dev_err(meson->chip.dev, "unable to get period pre_div\n");
return -EINVAL;
}
+ cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1));
+ if (cnt > 0xffff) {
+ dev_err(meson->chip.dev, "unable to get period cnt\n");
+ return -EINVAL;
+ }
+
dev_dbg(meson->chip.dev, "period=%u pre_div=%u cnt=%u\n", period,
pre_div, cnt);
@@ -195,8 +190,8 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
channel->lo = cnt;
} else {
/* Then check is we can have the duty with the same pre_div */
- duty_cnt = DIV_ROUND_CLOSEST_ULL((u64)duty * 1000,
- fin_ps * (pre_div + 1));
+ duty_cnt = div64_u64(fin_freq * (u64)duty,
+ NSEC_PER_SEC * (pre_div + 1));
if (duty_cnt > 0xffff) {
dev_err(meson->chip.dev, "unable to get duty cycle\n");
return -EINVAL;
--
2.21.0
Update the meson_pwm_get_state() implementation to take care of all
information in the registers instead of only reading the "enabled"
state.
The PWM output is only enabled if two conditions are met:
1. the per-channel clock is enabled
2. the PWM output is enabled
Calculate the PWM period and duty cycle using the reverse formula which
we already have in meson_pwm_calc() and update struct pwm_state with the
results.
As result of this /sys/kernel/debug/pwm now shows the PWM state set by
the bootloader (or firmware) after booting Linux.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/pwm/pwm-meson.c | 52 ++++++++++++++++++++++++++++++++++++++---
1 file changed, 49 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 9afa1e5aaebf..010212166d5d 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -287,19 +287,65 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;
}
+static unsigned int meson_pwm_cnt_to_ns(struct pwm_chip *chip,
+ struct pwm_device *pwm, u32 cnt)
+{
+ struct meson_pwm *meson = to_meson_pwm(chip);
+ struct meson_pwm_channel *channel;
+ unsigned long fin_freq;
+ u32 fin_ns;
+
+ /* to_meson_pwm() can only be used after .get_state() is called */
+ channel = &meson->channels[pwm->hwpwm];
+
+ fin_freq = clk_get_rate(channel->clk);
+ if (fin_freq == 0)
+ return 0;
+
+ fin_ns = div_u64(NSEC_PER_SEC, fin_freq);
+
+ return cnt * fin_ns * (channel->pre_div + 1);
+}
+
static void meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state)
{
struct meson_pwm *meson = to_meson_pwm(chip);
- u32 value, mask;
+ struct meson_pwm_channel_data *channel_data;
+ struct meson_pwm_channel *channel;
+ u32 value, tmp;
if (!state)
return;
- mask = meson_pwm_per_channel_data[pwm->hwpwm].pwm_en_mask;
+ channel = &meson->channels[pwm->hwpwm];
+ channel_data = &meson_pwm_per_channel_data[pwm->hwpwm];
value = readl(meson->base + REG_MISC_AB);
- state->enabled = (value & mask) != 0;
+
+ tmp = channel_data->pwm_en_mask | channel_data->clk_en_mask;
+ state->enabled = (value & tmp) == tmp;
+
+ tmp = value >> channel_data->clk_div_shift;
+ channel->pre_div = FIELD_GET(MISC_CLK_DIV_MASK, tmp);
+
+ value = readl(meson->base + channel_data->reg_offset);
+
+ channel->lo = FIELD_GET(PWM_LOW_MASK, value);
+ channel->hi = FIELD_GET(PWM_HIGH_MASK, value);
+
+ if (channel->lo == 0) {
+ state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->hi);
+ state->duty_cycle = state->period;
+ } else if (channel->lo >= channel->hi) {
+ state->period = meson_pwm_cnt_to_ns(chip, pwm,
+ channel->lo + channel->hi);
+ state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm,
+ channel->hi);
+ } else {
+ state->period = 0;
+ state->duty_cycle = 0;
+ }
}
static const struct pwm_ops meson_pwm_ops = {
--
2.21.0
MISC_CLK_SEL_WIDTH is only used in one place where it's converted into
a bit-mask. Rename and change the macro to be a bit-mask so that
conversion is not needed anymore. No functional changes intended.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/pwm/pwm-meson.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index c62a3ac924d0..84b28ba0f903 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -33,7 +33,7 @@
#define MISC_A_CLK_DIV_SHIFT 8
#define MISC_B_CLK_SEL_SHIFT 6
#define MISC_A_CLK_SEL_SHIFT 4
-#define MISC_CLK_SEL_WIDTH 2
+#define MISC_CLK_SEL_MASK 0x3
#define MISC_B_EN BIT(1)
#define MISC_A_EN BIT(0)
@@ -463,7 +463,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson,
channel->mux.reg = meson->base + REG_MISC_AB;
channel->mux.shift = mux_reg_shifts[i];
- channel->mux.mask = BIT(MISC_CLK_SEL_WIDTH) - 1;
+ channel->mux.mask = MISC_CLK_SEL_MASK;
channel->mux.flags = 0;
channel->mux.lock = &meson->lock;
channel->mux.table = NULL;
--
2.21.0
Simplify the code which fetches the input clock for a PWM channel by
using devm_clk_get_optional().
This comes with a small functional change: previously all errors except
EPROBE_DEFER were ignored. Now all other errors are also treated as
errors. If no input clock is present devm_clk_get_optional() will return
NULL instead of an error which matches the behavior of the old code.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/pwm/pwm-meson.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 3fbbc4128ce8..35b38c7201c3 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -474,14 +474,9 @@ static int meson_pwm_init_channels(struct meson_pwm *meson,
snprintf(name, sizeof(name), "clkin%u", i);
- channel->clk_parent = devm_clk_get(dev, name);
- if (IS_ERR(channel->clk_parent)) {
- err = PTR_ERR(channel->clk_parent);
- if (err == -EPROBE_DEFER)
- return err;
-
- channel->clk_parent = NULL;
- }
+ channel->clk_parent = devm_clk_get_optional(dev, name);
+ if (IS_ERR(channel->clk_parent))
+ return PTR_ERR(channel->clk_parent);
}
return 0;
--
2.21.0
meson_pwm_calc() is the last function that accepts a struct
meson_pwm_channel. meson_pwm_enable(), meson_pwm_disable() and
meson_pwm_apply() for example are all taking a struct pwm_device as
parameter. When they need the struct meson_pwm_channel these functions
simply call pwm_get_chip_data() internally.
Make meson_pwm_calc() consistent with the other functions in the
meson-pwm driver by passing struct pwm_device to it as well. The value
of the "id" parameter is actually pwm->hwpwm, but the driver never read
the "id" parameter, which is why there's no replacement for it in the
new code.
No functional changes.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/pwm/pwm-meson.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 39ea119add7b..d6eb4d04d5c9 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -114,10 +114,10 @@ static void meson_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
clk_disable_unprepare(channel->clk);
}
-static int meson_pwm_calc(struct meson_pwm *meson,
- struct meson_pwm_channel *channel,
+static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
struct pwm_state *state)
{
+ struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
unsigned int duty, period, pre_div, cnt, duty_cnt;
unsigned long fin_freq = -1;
u64 fin_ps;
@@ -280,7 +280,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
if (state->period != channel->state.period ||
state->duty_cycle != channel->state.duty_cycle ||
state->polarity != channel->state.polarity) {
- err = meson_pwm_calc(meson, channel, state);
+ err = meson_pwm_calc(meson, pwm, state);
if (err < 0)
return err;
--
2.21.0
This is a preparation for a future cleanup. Pass struct pwm_device
instead of passing the individual values required by each function as
these can be obtained for each struct pwm_device instance.
As a nice side-effect the driver now uses "switch (pwm->hwpwm)"
everywhere. Before some functions used "switch (id)" while others used
"switch (pwm->hwpwm)".
No functional changes.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/pwm/pwm-meson.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 5fef7e925282..3fbbc4128ce8 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -183,15 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson,
return 0;
}
-static void meson_pwm_enable(struct meson_pwm *meson,
- struct meson_pwm_channel *channel,
- unsigned int id)
+static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
{
+ struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
u32 value, clk_shift, clk_enable, enable;
unsigned int offset;
unsigned long flags;
- switch (id) {
+ switch (pwm->hwpwm) {
case 0:
clk_shift = MISC_A_CLK_DIV_SHIFT;
clk_enable = MISC_A_CLK_EN;
@@ -228,12 +227,12 @@ static void meson_pwm_enable(struct meson_pwm *meson,
spin_unlock_irqrestore(&meson->lock, flags);
}
-static void meson_pwm_disable(struct meson_pwm *meson, unsigned int id)
+static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
{
u32 value, enable;
unsigned long flags;
- switch (id) {
+ switch (pwm->hwpwm) {
case 0:
enable = MISC_A_EN;
break;
@@ -266,7 +265,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return -EINVAL;
if (!state->enabled) {
- meson_pwm_disable(meson, pwm->hwpwm);
+ meson_pwm_disable(meson, pwm);
channel->state.enabled = false;
return 0;
@@ -293,7 +292,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
}
if (state->enabled && !channel->state.enabled) {
- meson_pwm_enable(meson, channel, pwm->hwpwm);
+ meson_pwm_enable(meson, pwm);
channel->state.enabled = true;
}
--
2.21.0
Let meson_pwm_calc() use the polarity from struct pwm_state directly.
This removes a level of indirection where meson_pwm_apply() first had to
set a driver-internal inverter mask which was then only used by
meson_pwm_calc().
Instead of adding the polarity as parameter to meson_pwm_calc() switch
to struct pwm_state directly to make it easier to see where the
parameters are actually coming from.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/pwm/pwm-meson.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 84b28ba0f903..39ea119add7b 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -63,7 +63,6 @@ struct meson_pwm {
struct pwm_chip chip;
const struct meson_pwm_data *data;
void __iomem *base;
- u8 inverter_mask;
/*
* Protects register (write) access to the REG_MISC_AB register
* that is shared between the two PWMs.
@@ -116,14 +115,17 @@ static void meson_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
}
static int meson_pwm_calc(struct meson_pwm *meson,
- struct meson_pwm_channel *channel, unsigned int id,
- unsigned int duty, unsigned int period)
+ struct meson_pwm_channel *channel,
+ struct pwm_state *state)
{
- unsigned int pre_div, cnt, duty_cnt;
+ unsigned int duty, period, pre_div, cnt, duty_cnt;
unsigned long fin_freq = -1;
u64 fin_ps;
- if (~(meson->inverter_mask >> id) & 0x1)
+ duty = state->duty_cycle;
+ period = state->period;
+
+ if (state->polarity == PWM_POLARITY_INVERSED)
duty = period - duty;
if (period == channel->state.period &&
@@ -278,15 +280,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
if (state->period != channel->state.period ||
state->duty_cycle != channel->state.duty_cycle ||
state->polarity != channel->state.polarity) {
- if (state->polarity != channel->state.polarity) {
- if (state->polarity == PWM_POLARITY_NORMAL)
- meson->inverter_mask |= BIT(pwm->hwpwm);
- else
- meson->inverter_mask &= ~BIT(pwm->hwpwm);
- }
-
- err = meson_pwm_calc(meson, channel, pwm->hwpwm,
- state->duty_cycle, state->period);
+ err = meson_pwm_calc(meson, channel, state);
if (err < 0)
return err;
@@ -520,7 +514,6 @@ static int meson_pwm_probe(struct platform_device *pdev)
meson->chip.of_pwm_n_cells = 3;
meson->data = of_device_get_match_data(&pdev->dev);
- meson->inverter_mask = BIT(meson->chip.npwm) - 1;
channels = devm_kcalloc(&pdev->dev, meson->chip.npwm,
sizeof(*channels), GFP_KERNEL);
--
2.21.0
All existing PWM drivers (except pwm-meson and two other ones) call
pwm_set_chip_data() from their pwm_ops.request() callback. Now that we
can access the struct meson_pwm_channel from struct meson_pwm we can do
the same.
Move the call to pwm_set_chip_data() to meson_pwm_request() and drop the
custom meson_pwm_add_channels(). This makes the implementation
consistent with other drivers and makes it slightly more obvious
thatpwm_get_chip_data() cannot be used from pwm_ops.get_state() (because
that's called by the PWM core before pwm_ops.request()).
No functional changes intended.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/pwm/pwm-meson.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index ac7e188155fd..27915d6475e3 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -98,12 +98,16 @@ static inline struct meson_pwm *to_meson_pwm(struct pwm_chip *chip)
static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
{
- struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
+ struct meson_pwm *meson = to_meson_pwm(chip);
+ struct meson_pwm_channel *channel;
struct device *dev = chip->dev;
int err;
- if (!channel)
- return -ENODEV;
+ channel = pwm_get_chip_data(pwm);
+ if (channel)
+ return 0;
+
+ channel = &meson->channels[pwm->hwpwm];
if (channel->clk_parent) {
err = clk_set_parent(channel->clk, channel->clk_parent);
@@ -124,7 +128,7 @@ static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
chip->ops->get_state(chip, pwm, &channel->state);
- return 0;
+ return pwm_set_chip_data(pwm, channel);
}
static void meson_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -460,14 +464,6 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
return 0;
}
-static void meson_pwm_add_channels(struct meson_pwm *meson)
-{
- unsigned int i;
-
- for (i = 0; i < meson->chip.npwm; i++)
- pwm_set_chip_data(&meson->chip.pwms[i], &meson->channels[i]);
-}
-
static int meson_pwm_probe(struct platform_device *pdev)
{
struct meson_pwm *meson;
@@ -503,8 +499,6 @@ static int meson_pwm_probe(struct platform_device *pdev)
return err;
}
- meson_pwm_add_channels(meson);
-
platform_set_drvdata(pdev, meson);
return 0;
--
2.21.0
On Sat, May 25, 2019 at 08:11:29PM +0200, Martin Blumenstingl wrote:
> Replace the loop to calculate the pre-divider and count with two
> separate div64_u64() calculations. This makes the code easier to read
> and improves the precision.
>
> Two example cases:
> 1) 32.768kHz LPO clock for the SDIO wifi chip on Khadas VIM
> clock input: 500MHz (FCLK_DIV4)
> period: 30518ns
> duty cycle: 15259ns
> old algorithm: pre_div=0, cnt=15259
> new algorithm: pre_div=0, cnt=15259
> (no difference in calculated values)
>
> 2) PWM LED on Khadas VIM
> clock input: 24MHz (XTAL)
> period: 7812500ns
> duty cycle: 7812500ns
> old algorithm: pre_div=2, cnt=62004
> new algorithm: pre_div=2, cnt=62500
> Using a scope (24MHz sampling rate) shows the actual difference:
> - old: 7753000ns, off by -59500ns (0.7616%)
> - new: 7815000ns, off by +2500ns (0.032%)
>
> Suggested-by: Uwe Kleine-K?nig <[email protected]>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> drivers/pwm/pwm-meson.c | 25 ++++++++++---------------
> 1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 27915d6475e3..9afa1e5aaebf 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -12,6 +12,7 @@
> #include <linux/err.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> +#include <linux/math64.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> @@ -145,7 +146,6 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
> struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
> unsigned int duty, period, pre_div, cnt, duty_cnt;
> unsigned long fin_freq = -1;
> - u64 fin_ps;
>
> duty = state->duty_cycle;
> period = state->period;
> @@ -164,24 +164,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
> }
>
> dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
> - fin_ps = (u64)NSEC_PER_SEC * 1000;
> - do_div(fin_ps, fin_freq);
> -
> - /* Calc pre_div with the period */
> - for (pre_div = 0; pre_div <= MISC_CLK_DIV_MASK; pre_div++) {
> - cnt = DIV_ROUND_CLOSEST_ULL((u64)period * 1000,
> - fin_ps * (pre_div + 1));
> - dev_dbg(meson->chip.dev, "fin_ps=%llu pre_div=%u cnt=%u\n",
> - fin_ps, pre_div, cnt);
> - if (cnt <= 0xffff)
> - break;
> - }
>
> + pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL);
> if (pre_div > MISC_CLK_DIV_MASK) {
> dev_err(meson->chip.dev, "unable to get period pre_div\n");
> return -EINVAL;
> }
>
> + cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1));
> + if (cnt > 0xffff) {
> + dev_err(meson->chip.dev, "unable to get period cnt\n");
> + return -EINVAL;
> + }
> +
There is a slight modification in the calculation of pre_div that isn't
catched by the examples above.
Before this patch we had:
pick smallest pre_div such that
round_closest(period * 1000 / (round_down(1e12 / fin_freq) * (pre_div + 1)) <= 0xffff
New approach is:
pre_div = round_down(fin_freq * period / (1e9 * 0xffff))
An advantage of the new approach is better as it rounds only once and is
easier.
Consider fin_freq = 99990001 and period = 655355, then the old algorithm
picks pre_div = 1 while the new picks pre_div = 0.
I didn't continue here to check which are the resulting waveforms, I
assume they are different though.
As there is currently no definition what is a "better" approximation for
a given requested pair (duty_cycle, period) I cannot say if these
changes are good or not.
And that's a pity, so I still think there should be a documented
definition that lays down how a lowlevel driver should round. Without
that a consumer that cares about fine differences can not rely an the
abstraction provided by the PWM framework because each low-level driver
might behave differently.
@Thierry: So can you please continue the discussion about this topic.
The longer this is delayed the more patches are created and submitted
that eventually might be wrong which is a waste of developer and
reviewer time.
Assuming the people who care about meson don't object after reading this
I wouldn't want to stop this patch going in though. So:
Acked-by: Uwe Kleine-K?nig <[email protected]>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On 25/05/2019 20:11, Martin Blumenstingl wrote:
> Simplify the code which fetches the input clock for a PWM channel by
> using devm_clk_get_optional().
> This comes with a small functional change: previously all errors except
> EPROBE_DEFER were ignored. Now all other errors are also treated as
> errors. If no input clock is present devm_clk_get_optional() will return
> NULL instead of an error which matches the behavior of the old code.
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> drivers/pwm/pwm-meson.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 3fbbc4128ce8..35b38c7201c3 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -474,14 +474,9 @@ static int meson_pwm_init_channels(struct meson_pwm *meson,
>
> snprintf(name, sizeof(name), "clkin%u", i);
>
> - channel->clk_parent = devm_clk_get(dev, name);
> - if (IS_ERR(channel->clk_parent)) {
> - err = PTR_ERR(channel->clk_parent);
> - if (err == -EPROBE_DEFER)
> - return err;
> -
> - channel->clk_parent = NULL;
> - }
> + channel->clk_parent = devm_clk_get_optional(dev, name);
> + if (IS_ERR(channel->clk_parent))
> + return PTR_ERR(channel->clk_parent);
> }
>
> return 0;
>
Reviewed-by: Neil Armstrong <[email protected]>
On 25/05/2019 20:11, Martin Blumenstingl wrote:
> This is a preparation for a future cleanup. Pass struct pwm_device
> instead of passing the individual values required by each function as
> these can be obtained for each struct pwm_device instance.
>
> As a nice side-effect the driver now uses "switch (pwm->hwpwm)"
> everywhere. Before some functions used "switch (id)" while others used
> "switch (pwm->hwpwm)".
>
> No functional changes.
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> drivers/pwm/pwm-meson.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 5fef7e925282..3fbbc4128ce8 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -183,15 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson,
> return 0;
> }
>
> -static void meson_pwm_enable(struct meson_pwm *meson,
> - struct meson_pwm_channel *channel,
> - unsigned int id)
> +static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
> {
> + struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
> u32 value, clk_shift, clk_enable, enable;
> unsigned int offset;
> unsigned long flags;
>
> - switch (id) {
> + switch (pwm->hwpwm) {
> case 0:
> clk_shift = MISC_A_CLK_DIV_SHIFT;
> clk_enable = MISC_A_CLK_EN;
> @@ -228,12 +227,12 @@ static void meson_pwm_enable(struct meson_pwm *meson,
> spin_unlock_irqrestore(&meson->lock, flags);
> }
>
> -static void meson_pwm_disable(struct meson_pwm *meson, unsigned int id)
> +static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
> {
> u32 value, enable;
> unsigned long flags;
>
> - switch (id) {
> + switch (pwm->hwpwm) {
> case 0:
> enable = MISC_A_EN;
> break;
> @@ -266,7 +265,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> return -EINVAL;
>
> if (!state->enabled) {
> - meson_pwm_disable(meson, pwm->hwpwm);
> + meson_pwm_disable(meson, pwm);
> channel->state.enabled = false;
>
> return 0;
> @@ -293,7 +292,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> }
>
> if (state->enabled && !channel->state.enabled) {
> - meson_pwm_enable(meson, channel, pwm->hwpwm);
> + meson_pwm_enable(meson, pwm);
> channel->state.enabled = true;
> }
>
>
Reviewed-by: Neil Armstrong <[email protected]>
On 25/05/2019 20:11, Martin Blumenstingl wrote:
> MISC_CLK_SEL_WIDTH is only used in one place where it's converted into
> a bit-mask. Rename and change the macro to be a bit-mask so that
> conversion is not needed anymore. No functional changes intended.
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> drivers/pwm/pwm-meson.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index c62a3ac924d0..84b28ba0f903 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -33,7 +33,7 @@
> #define MISC_A_CLK_DIV_SHIFT 8
> #define MISC_B_CLK_SEL_SHIFT 6
> #define MISC_A_CLK_SEL_SHIFT 4
> -#define MISC_CLK_SEL_WIDTH 2
> +#define MISC_CLK_SEL_MASK 0x3
NIT I would have used GENMASK here
> #define MISC_B_EN BIT(1)
> #define MISC_A_EN BIT(0)
>
> @@ -463,7 +463,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson,
>
> channel->mux.reg = meson->base + REG_MISC_AB;
> channel->mux.shift = mux_reg_shifts[i];
> - channel->mux.mask = BIT(MISC_CLK_SEL_WIDTH) - 1;
> + channel->mux.mask = MISC_CLK_SEL_MASK;
> channel->mux.flags = 0;
> channel->mux.lock = &meson->lock;
> channel->mux.table = NULL;
>
Anyway, it's still correct :
Reviewed-by: Neil Armstrong <[email protected]>
On 25/05/2019 20:11, Martin Blumenstingl wrote:
> Let meson_pwm_calc() use the polarity from struct pwm_state directly.
> This removes a level of indirection where meson_pwm_apply() first had to
> set a driver-internal inverter mask which was then only used by
> meson_pwm_calc().
>
> Instead of adding the polarity as parameter to meson_pwm_calc() switch
> to struct pwm_state directly to make it easier to see where the
> parameters are actually coming from.
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> drivers/pwm/pwm-meson.c | 23 ++++++++---------------
> 1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 84b28ba0f903..39ea119add7b 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -63,7 +63,6 @@ struct meson_pwm {
> struct pwm_chip chip;
> const struct meson_pwm_data *data;
> void __iomem *base;
> - u8 inverter_mask;
> /*
> * Protects register (write) access to the REG_MISC_AB register
> * that is shared between the two PWMs.
> @@ -116,14 +115,17 @@ static void meson_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> }
>
> static int meson_pwm_calc(struct meson_pwm *meson,
> - struct meson_pwm_channel *channel, unsigned int id,
> - unsigned int duty, unsigned int period)
> + struct meson_pwm_channel *channel,
> + struct pwm_state *state)
> {
> - unsigned int pre_div, cnt, duty_cnt;
> + unsigned int duty, period, pre_div, cnt, duty_cnt;
> unsigned long fin_freq = -1;
> u64 fin_ps;
>
> - if (~(meson->inverter_mask >> id) & 0x1)
> + duty = state->duty_cycle;
> + period = state->period;
> +
> + if (state->polarity == PWM_POLARITY_INVERSED)
> duty = period - duty;
>
> if (period == channel->state.period &&
> @@ -278,15 +280,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> if (state->period != channel->state.period ||
> state->duty_cycle != channel->state.duty_cycle ||
> state->polarity != channel->state.polarity) {
> - if (state->polarity != channel->state.polarity) {
> - if (state->polarity == PWM_POLARITY_NORMAL)
> - meson->inverter_mask |= BIT(pwm->hwpwm);
> - else
> - meson->inverter_mask &= ~BIT(pwm->hwpwm);
> - }
> -
> - err = meson_pwm_calc(meson, channel, pwm->hwpwm,
> - state->duty_cycle, state->period);
> + err = meson_pwm_calc(meson, channel, state);
> if (err < 0)
> return err;
>
> @@ -520,7 +514,6 @@ static int meson_pwm_probe(struct platform_device *pdev)
> meson->chip.of_pwm_n_cells = 3;
>
> meson->data = of_device_get_match_data(&pdev->dev);
> - meson->inverter_mask = BIT(meson->chip.npwm) - 1;
>
> channels = devm_kcalloc(&pdev->dev, meson->chip.npwm,
> sizeof(*channels), GFP_KERNEL);
>
Reviewed-by: Neil Armstrong <[email protected]>
On 25/05/2019 20:11, Martin Blumenstingl wrote:
> Update the meson_pwm_get_state() implementation to take care of all
> information in the registers instead of only reading the "enabled"
> state.
>
> The PWM output is only enabled if two conditions are met:
> 1. the per-channel clock is enabled
> 2. the PWM output is enabled
>
> Calculate the PWM period and duty cycle using the reverse formula which
> we already have in meson_pwm_calc() and update struct pwm_state with the
> results.
>
> As result of this /sys/kernel/debug/pwm now shows the PWM state set by
> the bootloader (or firmware) after booting Linux.
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> drivers/pwm/pwm-meson.c | 52 ++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 9afa1e5aaebf..010212166d5d 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -287,19 +287,65 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> return 0;
> }
>
> +static unsigned int meson_pwm_cnt_to_ns(struct pwm_chip *chip,
> + struct pwm_device *pwm, u32 cnt)
> +{
> + struct meson_pwm *meson = to_meson_pwm(chip);
> + struct meson_pwm_channel *channel;
> + unsigned long fin_freq;
> + u32 fin_ns;
> +
> + /* to_meson_pwm() can only be used after .get_state() is called */
> + channel = &meson->channels[pwm->hwpwm];
> +
> + fin_freq = clk_get_rate(channel->clk);
> + if (fin_freq == 0)
> + return 0;
> +
> + fin_ns = div_u64(NSEC_PER_SEC, fin_freq);
> +
> + return cnt * fin_ns * (channel->pre_div + 1);
> +}
> +
> static void meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> struct pwm_state *state)
> {
> struct meson_pwm *meson = to_meson_pwm(chip);
> - u32 value, mask;
> + struct meson_pwm_channel_data *channel_data;
> + struct meson_pwm_channel *channel;
> + u32 value, tmp;
>
> if (!state)
> return;
>
> - mask = meson_pwm_per_channel_data[pwm->hwpwm].pwm_en_mask;
> + channel = &meson->channels[pwm->hwpwm];
> + channel_data = &meson_pwm_per_channel_data[pwm->hwpwm];
>
> value = readl(meson->base + REG_MISC_AB);
> - state->enabled = (value & mask) != 0;
> +
> + tmp = channel_data->pwm_en_mask | channel_data->clk_en_mask;
> + state->enabled = (value & tmp) == tmp;
> +
> + tmp = value >> channel_data->clk_div_shift;
> + channel->pre_div = FIELD_GET(MISC_CLK_DIV_MASK, tmp);
> +
> + value = readl(meson->base + channel_data->reg_offset);
> +
> + channel->lo = FIELD_GET(PWM_LOW_MASK, value);
> + channel->hi = FIELD_GET(PWM_HIGH_MASK, value);
> +
> + if (channel->lo == 0) {
> + state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->hi);
> + state->duty_cycle = state->period;
> + } else if (channel->lo >= channel->hi) {
> + state->period = meson_pwm_cnt_to_ns(chip, pwm,
> + channel->lo + channel->hi);
> + state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm,
> + channel->hi);
> + } else {
> + state->period = 0;
> + state->duty_cycle = 0;
> + }
> }
>
> static const struct pwm_ops meson_pwm_ops = {
>
Thanks for that !!!
Reviewed-by: Neil Armstrong <[email protected]>
On 25/05/2019 20:11, Martin Blumenstingl wrote:
> All existing PWM drivers (except pwm-meson and two other ones) call
> pwm_set_chip_data() from their pwm_ops.request() callback. Now that we
> can access the struct meson_pwm_channel from struct meson_pwm we can do
> the same.
>
> Move the call to pwm_set_chip_data() to meson_pwm_request() and drop the
> custom meson_pwm_add_channels(). This makes the implementation
> consistent with other drivers and makes it slightly more obvious
> thatpwm_get_chip_data() cannot be used from pwm_ops.get_state() (because
> that's called by the PWM core before pwm_ops.request()).
>
> No functional changes intended.
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> drivers/pwm/pwm-meson.c | 22 ++++++++--------------
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index ac7e188155fd..27915d6475e3 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -98,12 +98,16 @@ static inline struct meson_pwm *to_meson_pwm(struct pwm_chip *chip)
>
> static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> - struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
> + struct meson_pwm *meson = to_meson_pwm(chip);
> + struct meson_pwm_channel *channel;
> struct device *dev = chip->dev;
> int err;
>
> - if (!channel)
> - return -ENODEV;
> + channel = pwm_get_chip_data(pwm);
> + if (channel)
> + return 0;
> +
> + channel = &meson->channels[pwm->hwpwm];
>
> if (channel->clk_parent) {
> err = clk_set_parent(channel->clk, channel->clk_parent);
> @@ -124,7 +128,7 @@ static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>
> chip->ops->get_state(chip, pwm, &channel->state);
>
> - return 0;
> + return pwm_set_chip_data(pwm, channel);
> }
>
> static void meson_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -460,14 +464,6 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
> return 0;
> }
>
> -static void meson_pwm_add_channels(struct meson_pwm *meson)
> -{
> - unsigned int i;
> -
> - for (i = 0; i < meson->chip.npwm; i++)
> - pwm_set_chip_data(&meson->chip.pwms[i], &meson->channels[i]);
> -}
> -
> static int meson_pwm_probe(struct platform_device *pdev)
> {
> struct meson_pwm *meson;
> @@ -503,8 +499,6 @@ static int meson_pwm_probe(struct platform_device *pdev)
> return err;
> }
>
> - meson_pwm_add_channels(meson);
> -
> platform_set_drvdata(pdev, meson);
>
> return 0;
>
Reviewed-by: Neil Armstrong <[email protected]>
On 25/05/2019 20:11, Martin Blumenstingl wrote:
> meson_pwm_apply() has to consider the PWM polarity when disabling the
> output.
> With enabled=false and polarity=PWM_POLARITY_NORMAL the output needs to
> be LOW. The driver already supports this.
> With enabled=false and polarity=PWM_POLARITY_INVERSED the output needs
> to be HIGH. Implement this in the driver by internally enabling the
> output with the same settings that we already use for "period == duty".
>
> This fixes a PWM API violation which expects that the driver honors the
> polarity also for enabled=false. Due to the IP block not supporting this
> natively we only get "an as close as possible" to 100% HIGH signal (in
> my test setup with input clock of 24MHz and measuring the output with a
> logic analyzer at 24MHz sampling rate I got a duty cycle of 99.998475%
> on a Khadas VIM).
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> drivers/pwm/pwm-meson.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 900d362ec3c9..bb48ba85f756 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -245,6 +245,7 @@ static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
> static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> struct pwm_state *state)
> {
> + struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
> struct meson_pwm *meson = to_meson_pwm(chip);
> int err = 0;
>
> @@ -252,7 +253,27 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> return -EINVAL;
>
> if (!state->enabled) {
> - meson_pwm_disable(meson, pwm);
> + if (state->polarity == PWM_POLARITY_INVERSED) {
> + /*
> + * This IP block revision doesn't have an "always high"
> + * setting which we can use for "inverted disabled".
> + * Instead we achieve this using the same settings
> + * that we use a pre_div of 0 (to get the shortest
> + * possible duration for one "count") and
> + * "period == duty_cycle". This results in a signal
> + * which is LOW for one "count", while being HIGH for
> + * the rest of the (so the signal is HIGH for slightly
> + * less than 100% of the period, but this is the best
> + * we can achieve).
> + */
> + channel->pre_div = 0;
> + channel->hi = ~0;
> + channel->lo = 0;
> +
> + meson_pwm_enable(meson, pwm);
> + } else {
> + meson_pwm_disable(meson, pwm);
> + }
> } else {
> err = meson_pwm_calc(meson, pwm, state);
> if (err < 0)
>
While not perfect, it almost fills the gap.
Another way would be to use a specific pinctrl state setting the pin
in GPIO output in high level, but this implementation could stay
if the pinctrl state isn't available.
Reviewed-by: Neil Armstrong <[email protected]>
On 26/05/2019 21:41, Uwe Kleine-König wrote:
> On Sat, May 25, 2019 at 08:11:29PM +0200, Martin Blumenstingl wrote:
>> Replace the loop to calculate the pre-divider and count with two
>> separate div64_u64() calculations. This makes the code easier to read
>> and improves the precision.
>>
>> Two example cases:
>> 1) 32.768kHz LPO clock for the SDIO wifi chip on Khadas VIM
>> clock input: 500MHz (FCLK_DIV4)
>> period: 30518ns
>> duty cycle: 15259ns
>> old algorithm: pre_div=0, cnt=15259
>> new algorithm: pre_div=0, cnt=15259
>> (no difference in calculated values)
>>
>> 2) PWM LED on Khadas VIM
>> clock input: 24MHz (XTAL)
>> period: 7812500ns
>> duty cycle: 7812500ns
>> old algorithm: pre_div=2, cnt=62004
>> new algorithm: pre_div=2, cnt=62500
>> Using a scope (24MHz sampling rate) shows the actual difference:
>> - old: 7753000ns, off by -59500ns (0.7616%)
>> - new: 7815000ns, off by +2500ns (0.032%)
>>
>> Suggested-by: Uwe Kleine-König <[email protected]>
>> Signed-off-by: Martin Blumenstingl <[email protected]>
>> ---
>> drivers/pwm/pwm-meson.c | 25 ++++++++++---------------
>> 1 file changed, 10 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index 27915d6475e3..9afa1e5aaebf 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -12,6 +12,7 @@
>> #include <linux/err.h>
>> #include <linux/io.h>
>> #include <linux/kernel.h>
>> +#include <linux/math64.h>
>> #include <linux/module.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> @@ -145,7 +146,6 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>> struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
>> unsigned int duty, period, pre_div, cnt, duty_cnt;
>> unsigned long fin_freq = -1;
>> - u64 fin_ps;
>>
>> duty = state->duty_cycle;
>> period = state->period;
>> @@ -164,24 +164,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>> }
>>
>> dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
>> - fin_ps = (u64)NSEC_PER_SEC * 1000;
>> - do_div(fin_ps, fin_freq);
>> -
>> - /* Calc pre_div with the period */
>> - for (pre_div = 0; pre_div <= MISC_CLK_DIV_MASK; pre_div++) {
>> - cnt = DIV_ROUND_CLOSEST_ULL((u64)period * 1000,
>> - fin_ps * (pre_div + 1));
>> - dev_dbg(meson->chip.dev, "fin_ps=%llu pre_div=%u cnt=%u\n",
>> - fin_ps, pre_div, cnt);
>> - if (cnt <= 0xffff)
>> - break;
>> - }
>>
>> + pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL);
>> if (pre_div > MISC_CLK_DIV_MASK) {
>> dev_err(meson->chip.dev, "unable to get period pre_div\n");
>> return -EINVAL;
>> }
>>
>> + cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1));
>> + if (cnt > 0xffff) {
>> + dev_err(meson->chip.dev, "unable to get period cnt\n");
>> + return -EINVAL;
>> + }
>> +
>
> There is a slight modification in the calculation of pre_div that isn't
> catched by the examples above.
>
> Before this patch we had:
>
> pick smallest pre_div such that
> round_closest(period * 1000 / (round_down(1e12 / fin_freq) * (pre_div + 1)) <= 0xffff
>
> New approach is:
>
> pre_div = round_down(fin_freq * period / (1e9 * 0xffff))
>
> An advantage of the new approach is better as it rounds only once and is
> easier.
>
> Consider fin_freq = 99990001 and period = 655355, then the old algorithm
> picks pre_div = 1 while the new picks pre_div = 0.
>
> I didn't continue here to check which are the resulting waveforms, I
> assume they are different though.
>
> As there is currently no definition what is a "better" approximation for
> a given requested pair (duty_cycle, period) I cannot say if these
> changes are good or not.
>
> And that's a pity, so I still think there should be a documented
> definition that lays down how a lowlevel driver should round. Without
> that a consumer that cares about fine differences can not rely an the
> abstraction provided by the PWM framework because each low-level driver
> might behave differently.
>
> @Thierry: So can you please continue the discussion about this topic.
> The longer this is delayed the more patches are created and submitted
> that eventually might be wrong which is a waste of developer and
> reviewer time.
>
> Assuming the people who care about meson don't object after reading this
> I wouldn't want to stop this patch going in though. So:
>
> Acked-by: Uwe Kleine-König <[email protected]>
>
> Best regards
> Uwe
>
I don't have a strong view on this, Martin showed similar or much greater
accuracy in the 2 principal use cases of the driver, so I'm ok with it.
Reviewed-by: Neil Armstrong <[email protected]>
On 25/05/2019 20:11, Martin Blumenstingl wrote:
> meson_pwm_calc() is the last function that accepts a struct
> meson_pwm_channel. meson_pwm_enable(), meson_pwm_disable() and
> meson_pwm_apply() for example are all taking a struct pwm_device as
> parameter. When they need the struct meson_pwm_channel these functions
> simply call pwm_get_chip_data() internally.
>
> Make meson_pwm_calc() consistent with the other functions in the
> meson-pwm driver by passing struct pwm_device to it as well. The value
> of the "id" parameter is actually pwm->hwpwm, but the driver never read
> the "id" parameter, which is why there's no replacement for it in the
> new code.
>
> No functional changes.
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> drivers/pwm/pwm-meson.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 39ea119add7b..d6eb4d04d5c9 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -114,10 +114,10 @@ static void meson_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> clk_disable_unprepare(channel->clk);
> }
>
> -static int meson_pwm_calc(struct meson_pwm *meson,
> - struct meson_pwm_channel *channel,
> +static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
> struct pwm_state *state)
> {
> + struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
> unsigned int duty, period, pre_div, cnt, duty_cnt;
> unsigned long fin_freq = -1;
> u64 fin_ps;
> @@ -280,7 +280,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> if (state->period != channel->state.period ||
> state->duty_cycle != channel->state.duty_cycle ||
> state->polarity != channel->state.polarity) {
> - err = meson_pwm_calc(meson, channel, state);
> + err = meson_pwm_calc(meson, pwm, state);
> if (err < 0)
> return err;
>
>
Reviewed-by: Neil Armstrong <[email protected]>
Hi Neil,
On Mon, May 27, 2019 at 2:26 PM Neil Armstrong <[email protected]> wrote:
>
> On 25/05/2019 20:11, Martin Blumenstingl wrote:
> > MISC_CLK_SEL_WIDTH is only used in one place where it's converted into
> > a bit-mask. Rename and change the macro to be a bit-mask so that
> > conversion is not needed anymore. No functional changes intended.
> >
> > Signed-off-by: Martin Blumenstingl <[email protected]>
> > ---
> > drivers/pwm/pwm-meson.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> > index c62a3ac924d0..84b28ba0f903 100644
> > --- a/drivers/pwm/pwm-meson.c
> > +++ b/drivers/pwm/pwm-meson.c
> > @@ -33,7 +33,7 @@
> > #define MISC_A_CLK_DIV_SHIFT 8
> > #define MISC_B_CLK_SEL_SHIFT 6
> > #define MISC_A_CLK_SEL_SHIFT 4
> > -#define MISC_CLK_SEL_WIDTH 2
> > +#define MISC_CLK_SEL_MASK 0x3
>
> NIT I would have used GENMASK here
that was my initial idea but I decided against it.
the variant I came up with was: #define MISC_CLK_SEL_MASK GENMASK(1, 0)
however, the actual offset is either 4 or 6 (depending on the PWM channel)
and I felt that duplicating the macro would just make it more complicated
so instead I chose to be consistent with MISC_CLK_DIV_MASK
Let me know if you would like me to change it (then I prefer to update
MISC_CLK_DIV_MASK as well).
Martin
Hi Neil,
On Mon, May 27, 2019 at 2:33 PM Neil Armstrong <[email protected]> wrote:
>
> On 25/05/2019 20:11, Martin Blumenstingl wrote:
> > meson_pwm_apply() has to consider the PWM polarity when disabling the
> > output.
> > With enabled=false and polarity=PWM_POLARITY_NORMAL the output needs to
> > be LOW. The driver already supports this.
> > With enabled=false and polarity=PWM_POLARITY_INVERSED the output needs
> > to be HIGH. Implement this in the driver by internally enabling the
> > output with the same settings that we already use for "period == duty".
> >
> > This fixes a PWM API violation which expects that the driver honors the
> > polarity also for enabled=false. Due to the IP block not supporting this
> > natively we only get "an as close as possible" to 100% HIGH signal (in
> > my test setup with input clock of 24MHz and measuring the output with a
> > logic analyzer at 24MHz sampling rate I got a duty cycle of 99.998475%
> > on a Khadas VIM).
> >
> > Signed-off-by: Martin Blumenstingl <[email protected]>
> > ---
> > drivers/pwm/pwm-meson.c | 23 ++++++++++++++++++++++-
> > 1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> > index 900d362ec3c9..bb48ba85f756 100644
> > --- a/drivers/pwm/pwm-meson.c
> > +++ b/drivers/pwm/pwm-meson.c
> > @@ -245,6 +245,7 @@ static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
> > static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > struct pwm_state *state)
> > {
> > + struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
> > struct meson_pwm *meson = to_meson_pwm(chip);
> > int err = 0;
> >
> > @@ -252,7 +253,27 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > return -EINVAL;
> >
> > if (!state->enabled) {
> > - meson_pwm_disable(meson, pwm);
> > + if (state->polarity == PWM_POLARITY_INVERSED) {
> > + /*
> > + * This IP block revision doesn't have an "always high"
> > + * setting which we can use for "inverted disabled".
> > + * Instead we achieve this using the same settings
> > + * that we use a pre_div of 0 (to get the shortest
> > + * possible duration for one "count") and
> > + * "period == duty_cycle". This results in a signal
> > + * which is LOW for one "count", while being HIGH for
> > + * the rest of the (so the signal is HIGH for slightly
> > + * less than 100% of the period, but this is the best
> > + * we can achieve).
> > + */
> > + channel->pre_div = 0;
> > + channel->hi = ~0;
> > + channel->lo = 0;
> > +
> > + meson_pwm_enable(meson, pwm);
> > + } else {
> > + meson_pwm_disable(meson, pwm);
> > + }
> > } else {
> > err = meson_pwm_calc(meson, pwm, state);
> > if (err < 0)
> >
>
> While not perfect, it almost fills the gap.
> Another way would be to use a specific pinctrl state setting the pin
> in GPIO output in high level, but this implementation could stay
> if the pinctrl state isn't available.
I just noticed that Amlogic updated the PWM IP block in G12A:
it now supports "constant enable" (REG_MISC_AB bits 28 and 29) as well
as PWM_POLARITY_INVERSED (REG_MISC_AB bits 26 and 27) natively!
I like your idea of having a specific pinctrl state.
we can implement that for anything older than G12A once we actually need it.
for G12A we can do better thanks to the updated IP block
Martin
On Mon, May 27, 2019 at 07:46:43PM +0200, Martin Blumenstingl wrote:
> Hi Neil,
>
> On Mon, May 27, 2019 at 2:26 PM Neil Armstrong <[email protected]> wrote:
> >
> > On 25/05/2019 20:11, Martin Blumenstingl wrote:
> > > MISC_CLK_SEL_WIDTH is only used in one place where it's converted into
> > > a bit-mask. Rename and change the macro to be a bit-mask so that
> > > conversion is not needed anymore. No functional changes intended.
> > >
> > > Signed-off-by: Martin Blumenstingl <[email protected]>
> > > ---
> > > drivers/pwm/pwm-meson.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> > > index c62a3ac924d0..84b28ba0f903 100644
> > > --- a/drivers/pwm/pwm-meson.c
> > > +++ b/drivers/pwm/pwm-meson.c
> > > @@ -33,7 +33,7 @@
> > > #define MISC_A_CLK_DIV_SHIFT 8
> > > #define MISC_B_CLK_SEL_SHIFT 6
> > > #define MISC_A_CLK_SEL_SHIFT 4
> > > -#define MISC_CLK_SEL_WIDTH 2
> > > +#define MISC_CLK_SEL_MASK 0x3
> >
> > NIT I would have used GENMASK here
> that was my initial idea but I decided against it.
> the variant I came up with was: #define MISC_CLK_SEL_MASK GENMASK(1, 0)
>
> however, the actual offset is either 4 or 6 (depending on the PWM channel)
> and I felt that duplicating the macro would just make it more complicated
> so instead I chose to be consistent with MISC_CLK_DIV_MASK
An option would be:
#define MISC_CLK_SEL_MASK(hwid) GENMASK(1 + 4 * (hwid), 0 + 4 * (hwid))
(Note I didn't check a manual to the 4 above is probably wrong.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Uwe,
On Mon, May 27, 2019 at 8:00 PM Uwe Kleine-König
<[email protected]> wrote:
>
> On Mon, May 27, 2019 at 07:46:43PM +0200, Martin Blumenstingl wrote:
> > Hi Neil,
> >
> > On Mon, May 27, 2019 at 2:26 PM Neil Armstrong <[email protected]> wrote:
> > >
> > > On 25/05/2019 20:11, Martin Blumenstingl wrote:
> > > > MISC_CLK_SEL_WIDTH is only used in one place where it's converted into
> > > > a bit-mask. Rename and change the macro to be a bit-mask so that
> > > > conversion is not needed anymore. No functional changes intended.
> > > >
> > > > Signed-off-by: Martin Blumenstingl <[email protected]>
> > > > ---
> > > > drivers/pwm/pwm-meson.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> > > > index c62a3ac924d0..84b28ba0f903 100644
> > > > --- a/drivers/pwm/pwm-meson.c
> > > > +++ b/drivers/pwm/pwm-meson.c
> > > > @@ -33,7 +33,7 @@
> > > > #define MISC_A_CLK_DIV_SHIFT 8
> > > > #define MISC_B_CLK_SEL_SHIFT 6
> > > > #define MISC_A_CLK_SEL_SHIFT 4
> > > > -#define MISC_CLK_SEL_WIDTH 2
> > > > +#define MISC_CLK_SEL_MASK 0x3
> > >
> > > NIT I would have used GENMASK here
> > that was my initial idea but I decided against it.
> > the variant I came up with was: #define MISC_CLK_SEL_MASK GENMASK(1, 0)
> >
> > however, the actual offset is either 4 or 6 (depending on the PWM channel)
> > and I felt that duplicating the macro would just make it more complicated
> > so instead I chose to be consistent with MISC_CLK_DIV_MASK
>
> An option would be:
>
> #define MISC_CLK_SEL_MASK(hwid) GENMASK(1 + 4 * (hwid), 0 + 4 * (hwid))
>
> (Note I didn't check a manual to the 4 above is probably wrong.)
that (or at least something similar) will work
the catch here is: we use it to initialize the mux clock and the
common clock framework expects us to set "shift" and "mask", while
mask starts at bit 0 instead of shift
this is how the current code is being used at the moment:
channel->mux.shift = meson_pwm_per_channel_data[i].clk_sel_shift;
channel->mux.mask = MISC_CLK_SEL_MASK;
so with MISC_CLK_SEL_MASK this would become:
channel->mux.shift = meson_pwm_per_channel_data[i].clk_sel_shift;
channel->mux.mask = MISC_CLK_SEL_MASK(i) >> channel->mux.shift;
or we could dynamically determine the "shift" using ffs or friends
my own brain parses the variant from the patch best
I'm happy to change it though if we can find something "better"
Martin
